New system with --stragegy ours may lead to data loss

[tobiasKaminsky]

With latest version, I sometimes see that a backport is missing parts of the commits.
This is dangerous and a drawback than before (where backport had to be done entirely manually in case of a conflict).
I think it is better to not use --strategy ours, but rather commit the merge conflict, so that we can solve it manually.

[rullzer]

@skjnldsv

[skjnldsv]

Let me quote myself

I mean, the backport warns you when there was conflicts, so I don't see where do we "risks the data" here? If you review your project and code properly, then there is no issues
It is a stupid bot, but still better than not having the pull request opened
It's like any automated script, if you follow it blindly, you'll have bad things
the rule also apply to the backport bot
So I kind of don't see why you're complaining about a script that warns you that it partially failed
It's not really a surprise

[tobiasKaminsky]

Can you please explain me why "--strategy ours" is used?
Given there is is a merge conflict while backporting and we silently use "ours" then this can never be 100% the right choice.
So the user has always check back and most of the time add missing lines / change something.

So why is this chosen instead of having a merge conflict commited to PR?
And then the user checks out the branch, resolves all conflicts and pushes it?

[skjnldsv]

Because it's the easier way I found to still continue a conflicting cherry pick without forcing the patch, by protecting the original code in place.
I'm fine with whatever best command we can find. I'm far from being a git expert

[tobiasKaminsky]

I tested it with the failing backport locally and for me it would be something like:
git cherry-pick $commit
git add .
git commit -m "Cherry picked $commit with conflicts"

[tobiasKaminsky]

This then has the merge conflict in it:

 
+<<<<<<< HEAD
         assertEquals("Nextcloud Text", directEditing.editors.get("text").name);
         assertEquals("text/plain", directEditing.editors.get("text").optionalMimetypes.get(0));
+=======
+        assertTrue(directEditing.editors.containsKey("text"));
+>>>>>>> 3b186b483... enhance tests

and then I could checkout the branch, resolve it and push back.

[skjnldsv]

Keep in mind that it will probably explode if you have multiple conflicting commits then

[tobiasKaminsky]

Hm.
If those are not exactly the same line (could this be at all?) then you would have multiple merge conflicts in different files or on different locations, or?

[skjnldsv]

No clue 🤷

[PVince81]

I had raised #341 until I noticed this older ticket here today.

[PVince81]

Hm. If those are not exactly the same line (could this be at all?) then you would have multiple merge conflicts in different files or on different locations, or?

maybe something to test if it was possible to have a beta of the bot on a test repo