Page MenuHomePhabricator

Configure Gerrit to use conflictStyle diff3
Closed, ResolvedPublicFeature

Description

When Gerrit creates a cherry-pick with conflicts, it saves the change with conflict markers and asks the user to resolve the conflicts. By default, it uses the standard Git conflict style (“merge”), which only shows “ours” and “theirs”. This often makes it difficult to understand the conflict and, by extension, the correct resolution.

Since Gerrit 3.5, the conflictStyle configuration can apparently be used to select the “diff3” style instead (upstream commit; corresponds to merge.conflictStyle git config), which also shows the “base” version. In my experience, this makes it much easier to understand the conflict, as you can see which changes “ours” and “theirs” made relative to the “base”. I suggest we change the configuration to use “diff3” style. (Unfortunately, it sounds like this is only a system-wide setting, not one that can be changed by individual users.)

Details

Event Timeline

Suggestion motivated by this Mastodon interaction with Julia Evans.

I don’t know if anyone would object to this or potentially have their workflow broken… should we ask around on wikitech-l? Run a poll somewhere? WDYT? :)

hashar subscribed.

The commit solely adds a conflictStyle configuration setting but there is no implementation and that is only in jgit, a counterpart needs to be added to Gerrit to invoke the jgit function which formats the conflict as diff3 and I guess some Gerrit setting for it.

Turns out that is Upstream change 382894 - Add diff3 formatter for merge conflicts.. It introduces a change.diff3ConflictView configuration setting to Gerrit. It defaults to false, and we should definitely turn it on once we upgrade to Gerrit (Gerrit 3.9)

Thank you for the detailed report!

I see, then I misunderstood the Gerrit 3.5 release notes. Thanks for investigating!

https://www.gerritcodereview.com/3.5.html#jgit-changes lists any change that happened in JGit and it lists:

  • e58bf0870: Add git config for conflict style merge/diff3.

Beside that change above has no code for formatting with diff3, JGit is a library and Gerrit has to be adjusted to call the new code :]

Change #1037065 had a related patch set uploaded (by Hashar; author: Hashar):

[operations/puppet@production] gerrit: enable change.diff3ConflictView

https://gerrit.wikimedia.org/r/1037065

I don’t know if anyone would object to this or potentially have their workflow broken… should we ask around on wikitech-l? Run a poll somewhere? WDYT? :)

I guess the wikitech-l announcement of the Gerrit 3.9 deployment (which links to this task) is enough advance notice to see if anyone objects ^^

Change #1037065 merged by Jelto:

[operations/puppet@production] gerrit: enable change.diff3ConflictView

https://gerrit.wikimedia.org/r/1037065

hashar claimed this task.

We have upgraded to Gerrit 3.9.x series and the new conflict style should show up now.

And here is a screenshot for posterity which shows the diff3 markers (<<<<<<<, ====== and >>>>>>>):

gerrit-diff3.png (131×765 px, 28 KB)

Thank you @Lucas_Werkmeister_WMDE to have noticed it and for the verification.

Hang on, I think I got confused there. Isn’t that just a normal merge conflict display, and with diff3 style we’d also expect a ||||||| marker? 🤔

At least that’s what I get when I do a quick nonsense merge conflict locally:

<<<<<<< .merge_file_aBXbpe
A
b
||||||| .merge_file_acSBfm
a
b
=======
a
B
>>>>>>> .merge_file_qsGToA
c

Grmblblblb The upstream patch is https://gerrit-review.googlesource.com/c/gerrit/+/382894 which was releases in v3.9.0 (and we run a snapshot of 3.9.6).
My guess is that it is only applied for merge commits and the automerge view?

After a night of sleep, I have dig into the issue. 1046598/1 has been cherry-picked and Gerrit inserts a comment showing the conflicting files:

The following files contain Git conflicts:

src/MediaWiki/Hooks/LoadExtensionSchemaUpdatesHookHandler.php
tests/phpunit/unit/MediaWiki/Hooks/LoadExtensionSchemaUpdatesHookHandlerTest.php

The method is mergeWithConflicts() and is in java/com/google/gerrit/server/git/MergeUtil.java. The upstream patch introduced a new variant of the method which accepts a boolean to set the diff3 view and update some code paths but did not update all callers.

In the file I find:

public CodeReviewCommit createCherryPickFromCommit(
...
          mergeWithConflicts(
              rw, inserter, dc, "HEAD", mergeTip, "CHANGE", originalCommit, mergeResults);
      logger.atFine().log(
          "AutoMerge treeId=%s (with conflicts, inserter: %s)", tree.name(), inserter);
    }

The mergeWithConflicts() is not passed the boolean to format with diff3 and the variant of that method sets it to false. I haven't looked at other calls though, I imagine only a single code path got updated.

I added support for diff3 to rebasing/cherry-picking in the UI. https://gerrit-review.googlesource.com/c/gerrit/+/431417

Screenshot:

Screenshot 2024-06-25 at 14.52.22.png (848×3 px, 197 KB)

I have upgraded our Gerrit to 3.10.2 which does include 431417 - Add support for using diff3 for rebasing and cherry-pick with conflicts which was backported by @Paladox.

Our Gerrit already has change.diff3ConflictView enabled.

I had reopen the task ( T359821#9905941 ) because the diff3 markers were not showing on diff3. That was caught on the cherry-picked change: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EntitySchema/+/1046598/1/src/MediaWiki/Hooks/LoadExtensionSchemaUpdatesHookHandler.php

gerrit-diff3.png (131×765 px, 28 KB)

That is still happening cause the diff is stored in a cache and I would like to avoid flushing the whole cache of diffs (but maybe I should).

In theory, a future cherry pick would show a diff3, that is to be confirmed.

Mentioned in SAL (#wikimedia-releng) [2025-01-06T09:03:56Z] <hashar> gerrit: flushed diff_intraline, diff_summary, gerrit_file_diff and git_file_diff caches after having turned on diff3 style # T359821

I ended up flushing various diff caches. The link in my previous comment ( https://gerrit.wikimedia.org/r/c/mediawiki/extensions/EntitySchema/+/1046598/1/src/MediaWiki/Hooks/LoadExtensionSchemaUpdatesHookHandler.php ) still shows the same marker which I assume are the diff3 markers and thus it always worked? What is the expected output? Isn't >>>>>> CHANGE (xxx Whatever) the diff3 marker?

Rereading, the documentation: This setting takes effect when generating the automerge, which happens on upload. and then when I look at Gerrit source code:

java/com/google/gerrit/server/patch/AutoMerger.java
* <p>All created auto-merge commits are stored in the repository of their merge commit as {@code
* refs/cache-automerge/} branches. These branches serve:
*
* <ul>
*   <li>as a cache so that the each auto-merge gets computed only once
*   <li>as base for merge commits on which users can comment
* </ul>
*
* <p>The second point means that these commits are referenced from NoteDb. The consequence of this
* is that these refs should never be deleted.

So they are stored on disk and flushing the diff caches was useless.

Yes, it would be pretty terrible if it changed the contents of old patch sets… hopefully we’ll see the effect next time someone backports / cherry-picks with a merge conflict (and doesn’t abort).

For posterity:

gerrit_diff3_conflict_style.png (366×891 px, 92 KB)

Thank you to have remembered about it!!

antoine-approve