Re: [PATCH v5 00/12] In-core git merge-tree ("Server side merges")

[Elijah Newren <newren@gmail.com>]

On Sun, Feb 20, 2022 at 4:35 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
> On Sun, Feb 20 2022, Elijah Newren via GitGitGadget wrote:
>
> > == Updates Log ==
> >
> > Many thanks to the many reviewers who provided good feedback on the most
> > recent round -- Junio, Ævar, Josh, Emily, and perhaps some others I've
> > forgotten from review club.
> >
> > Updates since v4:
> >
> >  * Fixed double "is" in documentation.
> >  * Fixed a few small items with testcases
> >
> > Updates since v3 (or v5, if you include the rounds at
> > https://lore.kernel.org/git/pull.1114.v2.git.git.1641403655.gitgitgadget@gmail.com/):
> >
> >  * Dropped previous patches 5, 6, and 8 of the old series; they weren't
> >    being used and opened a can of worms[1]
> >  * [Patch 3] Restructured argument checking, including using an enum
> >  * [Patch 4] Restored the extended paragraph about the deprecated form of
> >    git-merge-tree, mentioned write-tree in plumbing commands, and a few
> >    other small fixups to the documentation
> >  * [Patch 4] Also provide an example of a clean merge rather than just a
> >    conflicted one
> >  * [Patch 6] Fix the incompatible arguments check and add some tests for it
> >  * [Patch 6] Introduce an anonymize_hash() shell function to make tests
> >    easier to read (less repeated sed)
> >  * [Patch 9] Rename --exclude-modes-oids-stages to --name-only; no short
> >    option for now
> >  * [Patch 10] When -z passed, the tree in the first section should have a
> >    trailing NUL rather than trailing newline [1]
> >    https://lore.kernel.org/git/CABPp-BEKuXHELVx4=5JJTj5HVOKZ=Y-4G4BK47BCZYYRSrkFsQ@mail.gmail.com/
> >
> > Stuff NOT included that reviewers brought up in earlier rounds:
> >
> >  * Very generic (mode, oid, stage, filename) printing formatting[2]
> >  * Always printing 3 stages for each filename with conflicts[3]
> >  * Attempting to group conflict stages by logical conflict rather than by
> >    affected target filepath[4]
> >  * Providing similar functionality for doing cherry-picks/rebases/reverts,
> >    i.e. a scheme for three-way merges with a specified merge-base[5]. That's
> >    being deferred to a future series. [2]
> >    https://lore.kernel.org/git/CABPp-BGnOes7J_piDyBUeuLVm274w4-9G3k0vR-0it3z7TPn_w@mail.gmail.com/
> >    [3]
> >    https://lore.kernel.org/git/CABPp-BG2rMEYBLuBW=0wtpJe4aUFGCFa8D0NTSKz9Sm+CkXPxw@mail.gmail.com/
> >    [4]
> >    https://lore.kernel.org/git/CABPp-BGCL0onSmpgKuO1k2spYCkx=v27ed9TSSxFib=OdDcLbw@mail.gmail.com/
> >    [5]
> >    https://lore.kernel.org/git/CABPp-BEaemkGGm0cSofP0gau7YN-y6HFoi0yJbHA8+iGjxsYSA@mail.gmail.com/
>
> I've looked through this, I think it all looks good overall & that the
> things that needed to be addressed (as opposed to my --format rambling)
> have been.
>
> I think all the code should be ready for "next".
>
> I suggested (I think around getopts discussion) in an earlier that the
> code would have been easier with a new built-in, but if we're
> deprecating the existing "mode" I think using the name is probably
> better in the end.
>
> I find the resulting documentation to be really hard to grok though
> because we're effectively describing two different commands. The current
> docs are small: https://git-scm.com/docs/git-merge-tree
>
> I built the tip of this series and read the manpage, and found myself
> needing to carefully squint to see what referred to what mode in the
> docs.
>
> E.g. by the time it's discussing "-z" and other options the reader needs
> to be astute really aware of the context, and infer from the lack of
> "<options>" on the "--trivial-merge" that these options refer to the
> "--write-tree" only.
>
> The same goes for the rest of "--trivial-merge". I.e. I found myself
> needing to read the whole docs word-by-word (no skimming!) to see if
> OUTPUT etc. was going to describe its output, or just the "new" mode.
>
> It is my NSHO that man pages should be structured for the impatient
> reader :)
>
> Then when you say "git-merge-tree was written to be[...]" I thought "ah
> ha! surely this will discuss the since-2005 implemented mode", but "was
> written to be" is referring to code new in this series.
>
> The below patch-on-top addresses all those concerns. Basically I just
> added a line to the top of the DESCRIPTION saying that you should read a
> "DEPRECATED DESCRIPTION" section at the end for "--trivial-merge", and
> that all of the rest is talking about the "--write-tree" mode.
>
> I then edited various prose to do away with the now-unnecessary "the
> first form" etc.
>
> This diff is better against "master" in that you'll see that the current
> merge-tree DESCRIPTION section isn't touched at all (it's now just under
> a new heading), but this diff is against the tip of your series.
>
> There's various other small fixes while I was it it here, e.g. all your
> cross-section links were using some pseudo-not-quite-ASCIIDOC syntax
> that doesn't work. Now it uses the right syntax. Ditto link-ifying
> references to "mktag" etc.
>
> I don't know if you'd consider this for a v6, or if I should just submit
> this on top myself, but in any case here it is. I'll leave it to you how
> you'd like to proceed with it:

Overall the diff below looks good; thanks!  I'll include it in a v6.
Two exceptions, listed below:

> diff --git a/Documentation/git-merge-tree.txt b/Documentation/git-merge-tree.txt
> index 306149fa0e2..723b1995426 100644
> --- a/Documentation/git-merge-tree.txt
> +++ b/Documentation/git-merge-tree.txt
> @@ -9,17 +9,24 @@ git-merge-tree - Perform merge without touching index or working tree
>  SYNOPSIS
>  --------
>  [verse]
> -'git merge-tree' [--write-tree] [<options>] <branch1> <branch2>
> -'git merge-tree' [--trivial-merge] <base-tree> <branch1> <branch2> (deprecated)
> +'git merge-tree' --write-tree [<options>] <branch1> <branch2>
> +'git merge-tree' --trivial-merge <base-tree> <branch1> <branch2>

I dislike all three changes to the synopsis; I'll discard these.

> +[[NEWMERGE]]
>  DESCRIPTION
>  -----------
>
> -Performs a merge, but does not make any new commits and does not read
> -from or write to either the working tree or index.
> +This command has a modern `--write-tree` mode and a deprecated
> +`--trivial-merge` mode. The rest of this documentation describes
> +modern `--write-tree` mode unless otherwise specified. see
> +<<DEPMERGE,DEPRECATED DESCRIPTION>> below for a summary of the
> +`--trivial-merge` mode.
>
> -The first form will merge the two branches, doing a real merge.  A real
> -merge is distinguished from a trivial merge in that it includes:
> +Performs a "real" merge, but does not make any new commits and does
> +not read from or write to either the working tree or index.
> +
> +The performed merge will use the same feature as the "real"
> +linkgit:git-merge[1], including:
>
>    * three way content merges of individual files
>    * rename detection
> @@ -28,24 +35,8 @@ merge is distinguished from a trivial merge in that it includes:
>      merge base, creating a virtual merge base by merging the merge bases)
>    * etc.
>
> -After the merge completes, the first form will create a new toplevel
> -tree object.  See `OUTPUT` below for details.
> -
> -The second form is deprecated; it is kept for backward compatibility
> -reasons but may be deleted in the future.  Other than the optional
> -`--trivial-merge`, it accepts no options.  It can only do a trivial
> -merge.  It reads three tree-ish, and outputs trivial merge results and
> -conflicting stages to the standard output in a semi-diff format.
> -Since this was designed for higher level scripts to consume and merge
> -the results back into the index, it omits entries that match
> -<branch1>.  The result of this second form is similar to what
> -three-way 'git read-tree -m' does, but instead of storing the results
> -in the index, the command outputs the entries to the standard output.
> -This form not only has limited applicability, the output format is
> -also difficult to work with, and it will generally be less performant
> -than the first form even on successful merges (especially if working
> -in large repositories).  The remainder of this manual will only
> -discuss the first form.
> +After the merge completes, a newtoplevel tree object is created.  See
> +`OUTPUT` below for details.
>
>  OPTIONS
>  -------
> @@ -54,7 +45,7 @@ OPTIONS
>         Do not quote filenames in the <Conflicted file info> section,
>         and end each filename with a NUL character rather than
>         newline.  Also begin the messages section with a NUL character
> -       instead of a newline.  See OUTPUT below for more information.
> +       instead of a newline.  See <<OUTPUT>> below for more information.
>
>  --name-only::
>         In the Conflicted file info section, instead of writing a list
> @@ -74,11 +65,12 @@ OPTIONS
>         share no common history.  This flag can be given to override that
>         check and make the merge proceed anyway.
>
> +[[OUTPUT]]
>  OUTPUT
>  ------
>
> -By default, for a successful merge, the output from git-merge-tree is
> -simply one line:
> +For a successful merge, the output from git-merge-tree is simply one
> +line:
>
>         <OID of toplevel tree>
>
> @@ -90,6 +82,7 @@ Whereas for a conflicted merge, the output is by default of the form:
>
>  These are discussed individually below.
>
> +[[OIDTLT]]
>  OID of toplevel tree
>  ~~~~~~~~~~~~~~~~~~~~
>
> @@ -98,6 +91,7 @@ working tree at the end of `git merge`.  If there were conflicts, then
>  files within this tree may have embedded conflict markers.  This section
>  is always followed by a newline (or NUL if `-z` is passed).
>
> +[[CFI]]
>  Conflicted file info
>  ~~~~~~~~~~~~~~~~~~~~
>
> @@ -111,6 +105,7 @@ the `--name-only` option is passed, the mode, object, and stage will
>  be omitted.  If `-z` is passed, the "lines" are terminated by a NUL
>  character instead of a newline character.
>
> +[[IM]]
>  Informational messages
>  ~~~~~~~~~~~~~~~~~~~~~~
>
> @@ -138,72 +133,94 @@ something other than 0 or 1 (and the output is unspecified).
>  USAGE NOTES
>  -----------
>
> -git-merge-tree was written to be low-level plumbing, similar to
> -hash-object, mktree, commit-tree, write-tree, update-ref, and mktag.
> -Thus, it could be used as a part of a series of steps such as
> +This command is intended as low-level plumbing, similar to
> +linkgit:git-hash-object[1], linkgit:git-mktree[1],
> +linkgit:git-commit-tree[1], linkgit:git-write-tree[1],
> +linkgit:git-update-ref[1], and linkgit:git-mktag[1].  Thus, it can be
> +used as a part of a series of steps such as:
>
>         NEWTREE=$(git merge-tree --write-tree $BRANCH1 $BRANCH2)
>         test $? -eq 0 || die "There were conflicts..."
>         NEWCOMMIT=$(git commit-tree $NEWTREE -p $BRANCH1 -p $BRANCH2)
>         git update-ref $BRANCH1 $NEWCOMMIT
>
> -Note that when the exit status is non-zero, NEWTREE in this sequence
> +Note that when the exit status is non-zero, `NEWTREE` in this sequence
>  will contain a lot more output than just a tree.
>
> -git-merge-tree was written to provide users with the same information
> -that they'd have access to if using `git merge`:
> -  * what would be written to the working tree (the <OID of toplevel tree>)
> +The output will include the same information that you'd get with
> +linkgit:git-merge[1]:
> +
> +  * what would be written to the working tree (the <<OIDTLT,OID of toplevel tree>>)
>    * the higher order stages that would be written to the index (the
> -    <Conflicted file info>)
> -  * any messages that would have been printed to stdout (the <Informational
> -    messages>)
> +    <<CFI,Conflicted file info>>)
> +  * any messages that would have been printed to stdout (the <<IM,Informational
> +    messages>>)
>
>  MISTAKES TO AVOID
>  -----------------
>
>  Do NOT look through the resulting toplevel tree to try to find which
> -files conflict; parse the <Conflicted file info> section instead.  Not
> +files conflict; parse the <<CFI,Conflicted file info>> section instead.  Not
>  only would parsing an entire tree be horrendously slow in large
>  repositories, there are numerous types of conflicts not representable by
>  conflict markers (modify/delete, mode conflict, binary file changed on
>  both sides, file/directory conflicts, various rename conflict
>  permutations, etc.)
>
> -Do NOT interpret an empty <Conflicted file info> list as a clean merge;
> +Do NOT interpret an empty <<CFI,Conflicted file info>> list as a clean merge;
>  check the exit status.  A merge can have conflicts without having
>  individual files conflict (there are a few types of directory rename
>  conflicts that fall into this category, and others might also be added
>  in the future).
>
>  Do NOT attempt to guess or make the user guess the conflict types from
> -the <Conflicted file info> list.  The information there is insufficient
> +the <<CFI,Conflicted file info>> list.  The information there is insufficient
>  to do so.  For example: Rename/rename(1to2) conflicts (both sides
>  renamed the same file differently) will result in three different file
>  having higher order stages (but each only has one higher order stage),
> -with no way (short of the <Informational messages> section) to determine
> +with no way (short of the <<IM,Informational messages>> section) to determine
>  which three files are related.  File/directory conflicts also result in
>  a file with exactly one higher order stage.
>  Possibly-involved-in-directory-rename conflicts (when
>  "merge.directoryRenames" is unset or set to "conflicts") also result in
>  a file with exactly one higher order stage.  In all cases, the
> -<Informational messages> section has the necessary info, though it is
> +<<IM,Informational messages>> section has the necessary info, though it is
>  not designed to be machine parseable.
>
> -Do NOT assume all filenames listed in the <Informational messages>
> +Do NOT assume all filenames listed in the <<IM,Informational messages>>
>  section had conflicts.  Messages can be included for files that have no
>  conflicts, such as "Auto-merging <file>".
>
> -AVOID taking the OIDS from the <Conflicted file info> and re-merging
> +AVOID taking the OIDS from the <<CFI,Conflicted file info>> and re-merging
>  them to present the conflicts to the user.  This will lose information.
> -Instead, look up the version of the file found within the <OID of
> -toplevel tree> and show that instead.  In particular, the latter will
> +Instead, look up the version of the file found within the <<OIDTLT,OID of
> +toplevel tree>> and show that instead.  In particular, the latter will
>  have conflict markers annotated with the original branch/commit being
>  merged and, if renames were involved, the original filename.  While you
>  could include the original branch/commit in the conflict marker
>  annotations when re-merging, the original filename is not available from
> -the <Conflicted file info> and thus you would be losing information that
> +the <<CFI,Conflicted file info>> and thus you would be losing information that
>  might help the user resolve the conflict.
>
> +[[DEPMERGE]]
> +DEPRECATED DESCRIPTION
> +----------------------
> +
> +Per the <<NEWMERGE,DESCRIPTION>> and unlike the rest of this
> +documentation this section describes describes the deprecated
> +`--trivial-merge` mode.
> +
> +Reads three tree-ish, and output trivial merge results and
> +conflicting stages to the standard output.  This is similar to
> +what three-way 'git read-tree -m' does, but instead of storing the
> +results in the index, the command outputs the entries to the
> +standard output.
> +
> +This is meant to be used by higher level scripts to compute
> +merge results outside of the index, and stuff the results back into the
> +index.  For this reason, the output from the command omits
> +entries that match the <branch1> tree.

What you have in the new section is fine, but what you've deleted
should probably be included here.  In particular, the brief
explanation about why this mode is deprecated.


> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
>