Add 3 blank lines between lock entries to minimize merge conflicts

[schlosna]

Before this PR

Automated upgrades for non-overlapping dependencies would generate merge conflicts due to overlapping git context

After this PR

==COMMIT_MSG==
Add 3 blank lines between lock entries to minimize merge conflicts
==COMMIT_MSG==

Possible downsides?

versions.lock contains extra visual bloat that may make manual reviews more difficult (e.g. checking that all modules for related dependencies are upgraded in sync).

This doesn't address direct conflicts when the number or dependency constraints hash conflict, those still need to be manually deconflicted (likely serializing the automation and rerunning --write-locks after merging versions.props)

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Add 3 lines of autogenerated comments to minimize merge conflicts

Check the box to generate changelog(s)

• Generate changelog entry

[ash211]

There are some excavators that read the versions.lock file -- see https://g.p.b/excavator-checks/excavator-gradle/search?q=versions.lock They might need to be updated to handle these extra comments, but they should have been handling the comment at the top of the file anyways so maybe nbd.

[iamdanfox]

Woah I think this is gonna cause a lot of noise... I would honestly prefer that we find ways of configuring git to not freak about about adjacent lines being modified (i.e. these are not a conflict)

[pkoenig10]

Do we need to add this text before every line? It would be much easier to read if the comment lines were just #.

As long as this message appears once in the file, that feels sufficient.

[schlosna]

I believe as long as the 3 lines of context used by git diff are static it shouldn't matter, so we can adjust this. I was using something similar to https://github.com/chromium/chromium/blob/main/DEPS#L311-L314

[schlosna]

updated to blank lines to avoid visual thrash

[ash211]

We have the same issues with versions.props as we do with versions.lock. Incrementing the version number of two adjacent lines in separate, overlapping PRs causes a git merge conflict in either file. Will we need a similar approach for versions.props too?

[schlosna]

I am a bit more hesitant to apply to versions.props since it is not autogenerated and has more entropy, but might be worth exploring. We don't currently enforce canonicalization of versions.props -- might be a bit orthogonal but if we visually spread the versions.props apart it would be nice to have everything lexicographically sorted, with specifically pinned versions in a separate section so that when reviewing one knows what to expect where.

[iamdanfox]

Before we go down this route, I've found that apparently git has some built in merge drivers (e.g. one called union) and also allows us to configure a custom one. Maybe it's worth pursuing this?? http://schacon.github.io/git/gitattributes.html

[carterkozak]

I'd like to try this.

In terms of rollout, do you think it would be reasonable to define a new gradle property which opts into this feature for --write-locks? This way we can excavate enablement in parallel with gcv upgrades without risking friction for the in-flight gradle8 improvements.

Regarding merge drivers, github doesn't allow us to specify custom merge drivers, so we'd need additional robots to handle merges for that to work out. I would much rather change our spacing as its a simpler, more elegant solution.

[ash211]

Once this progresses to the point of rolling out more broadly, I'd like to explore adding an explanation for the three blank lines here in the versions.locks file header

[schlosna]

updated header to # Run ./gradlew --write-locks to regenerate this file. Blank lines are to minimize merge conflicts.

[ash211]

Is there somewhere in the default git merge driver's code that makes 3 lines a special amount of context? I'm wondering where this comes from, and why not 1 or 2 or 4+

[schlosna]

I'm not super familiar with the git source tree, and I haven't been able to find the exact code for default merge driver, but https://github.com/git/git/blob/a9e066fa63149291a55f383cfa113d8bdbdaa6b3/t/t4055-diff-context.sh#L40 is test case test_expect_success 'the default number of context lines is 3'

Separately, gnu diff in diffutils also defaults to 3 lines for context per https://git.savannah.gnu.org/cgit/diffutils.git/tree/src/diff.c#n944

according to an unnamed 🤖 source:

By default, Git uses three lines of context when determining merge conflicts. This means that Git compares the changes made in the conflicting files with the version of the file in the common ancestor, as well as the two versions of the file being merged. These three versions of the file, along with the three lines of context around each change, are used to determine how to resolve the conflict. However, it is possible to specify a different number of lines of context using the -U or --unified option when running the git merge command.

[newren]

Not sure if this helps, but since I was pinged, I'll try to answer...

Is there somewhere in the default git merge driver's code that makes 3 lines a special amount of context? I'm wondering where this comes from, and why not 1 or 2 or 4+

So, the merging of three textual files is done by the xdiff library in Git (which...has a history. It's an internal copy of libxdiff, which was modified internally. Other projects have also copied and modified this library. The upstream project is dead, the code is somewhat inscrutable at times, someone tried to split xdiff back out of Git so it could be shared at https://github.com/libgit2/xdiff but Git still uses their internal copy). I've rarely touched the xdiff code, so I'm slightly outside my wheelhouse. However, I have extensively modified the parts of the Git merge machinery that call xdiff, so I know at least a little.

The file of interest is https://git.kernel.org/pub/scm/git/git.git/tree/xdiff/xmerge.c, and you can start with the function xdl_merge().

Digging around a bit, I think the constant of 3 is only relevant in terms of the number of lines of content being printed when creating a diff. For example, from the Git sources:

builtin_diff() ->
  sets xecfg.ctxlen
  xdi_diff_outf() [passing xecfg]->
    xdi_diff() [passing xecfg] ->
      xdl_diff() [passing xefcg] ->
        xdl_do_diff() [NOT passing xefcg] ->
          xdl_recs_cmp()
        emit_func called with xefcg passed

whereas for merges:

merge_3way() ->
  ll_merge() ->
    xdl_merge() ->
      xdl_do_diff() ->
        xdl_recs_cmp()    

but importantly, nowhere in the latter call chain sets up or uses a xdemitconf_t (the type of xecfg), and nothing calls an emit function. I think the extended conflict range you see likely has more to do with the xdl_change_compact() calls trying to merge nearby groups of changed & unchanged code.

Further a git grep '\b3\b' xdiff and investigating all of them, and doing a few other searches, doesn't seem to suggest there's any variable or constant controlling "context" lines, just the number of changed lines (chg[012] is a useful search there).

Basically, I think the behavior of nearby lines being sucked into the conflict region is pretty hard-coded into the algorithm currently in use.

I'm not super familiar with the git source tree, and I haven't been able to find the exact code for default merge driver, but https://github.com/git/git/blob/a9e066fa63149291a55f383cfa113d8bdbdaa6b3/t/t4055-diff-context.sh#L40 is test case test_expect_success 'the default number of context lines is 3'

Yes, for diff generation, 3 lines of context is the default, but this doesn't affect merge operation. (It would affect git-apply, but apply is not merge). Note also that the 3 lines of context default for diffs is documented in the git-config(1) manpage (under diff.context) and can be found where diff_context_default is defined near the top of https://git.kernel.org/pub/scm/git/git.git/tree/diff.c.

By default, Git uses three lines of context when determining merge conflicts. This means that Git compares the changes made in the conflicting files with the version of the file in the common ancestor, as well as the two versions of the file being merged. These three versions of the file, along with the three lines of context around each change, are used to determine how to resolve the conflict. However, it is possible to specify a different number of lines of context using the -U or --unified option when running the git merge command.

In the past, not understanding xdiff, I probably would have thought that everything before the final sentence was correct. From digging into the sources today, I'm not so sure there is even a "number of context lines" idea involved in the algorithm at all.

The last sentence is simply incorrect; try running git merge -U8 $BRANCH or git merge --unified 8 $BRANCH.
It'll throw an error (unknown switch 'U' or unknown option 'unified')

[CRogers]

Note from internal discussion about this. I think the idea is good but the tricky bit is the rollout, especially as we have external users of this plugin since it's open source and there are consumers who read the lock file, who may not skip empty lines.

I think the only real way to do this is to have a feature flag in gradle.properties (or somewhere else) to write out the new file with the extra blank lines. Still support the old version. Then we can have an excavator internally which applies this to every repo one by one. We can find and fix consumers as it rolls out. External people can opt in but are not affected.

Maybe some time in the future make a major version bump that changes the default - but only maybe.