Re: [GSoC][RFC PATCH] git-merge.adoc: detail submodule merge

From: Elijah Newren <newren@gmail.com>
Date: Feb 18, 2025, 21:58 UTC
Message-ID: <CABPp-BHvutRL80QiTYx6gJbYiATOD32DrvXgQQtEy=u5oUZ9Zg@mail.gmail.com>
In-Reply-To: <EE88CF05-9120-4CAB-A097-84CC5EA2FACE@gmail.com>

On Tue, Feb 18, 2025 at 10:55 AM Lucas Oshiro <lucasseikioshiro@gmail.com> wrote:

Show 35 quoted lines

>
> > It may be worth referencing the commit(s) that introduced the behavior
> > for other reviewers: commit 68d03e4a6e44 (Implement automatic
> > fast-forward merge for submodules, 2010-07-07).
>
> Ok! I'll inspect the codebase and reference it in a future v2.
>
> > "referred by it" is hard for me to parse.  Maybe something like
> >
> > """
> > In the case where the path is a submodule, if the HEAD version of the
> > submodule is a descendant of the MERGE_HEAD version of the submodule,
> > or vice-versa, Git...
> > """
> > ?
>
> Perfect! Actually, I find submodules a little abstract to be explained
> using only words, but your sentence is very clear.
>
> > Also, the references to HEAD and MERGE_HEAD do tie this documentation
> > rather directly to `git merge`; the basic idea is applicable to all
> > callers of the merge machinery, but none of the other callers use
> > MERGE_HEAD (some use CHERRY_PICK_HEAD or REBASE_HEAD), and some do not
> > assume HEAD points to one of the parents either (e.g. merge-tree and
> > replay).  So, if we want to move this somewhere more general, we'd
> > need to reword it a bit.
>
> Given your previous suggestion, what about:
>
> """
> In the case where the path is a submodule, if one of the versions of
> submodule is descendant of another, Git...
> """
>
> ?

That seems like the right direction, but I think "descendant of another" is vague/confusing. Perhaps

""" In the case where the path is a submodule, if the submodule commit used on one side of the merge is a descendant of the submodule commit used on the other side of the merge, Git... """

Show 9 quoted lines

>
> > Oh, maybe we could put this information in
> > Documentation/merge-strategies.txt?  Hmm....
>
> Looks like a good place to put this. My only concerns are:
>
> 1. It would need to be documented in both `ort` and `recursive`. I don't
> think it would be a big deal as most of the first paragraph of both
> strategies are equal.

Yes, until `recursive` is deleted anyway. (At which point we'll just remap `recursive` to mean `ort` and not have to have separate documentation for the two.)

> 2. Maybe it would detail too much on this specific case, while not
> covering others (e.g. changing file permissions, symlinks, etc).

Yeah, but we don't have a way to resolve differences for those kinds of changes when neither side matches the base version (unless something like -Xours or -Xtheirs is passed, but even then that belongs under the -X documentation); submodules are somewhat special in that regard.

Show 6 quoted lines

> > Thanks for sending this in.  It's always helpful to get researched
> > documentation improvements, even if I can't help but nitpick and
> > complicate matters here and there....  ;-)
>
> Thank you! Given how deeply you understand the merge machinery any nitpick
> is immensely valuable!