On Sun, Feb 20, 2022 at 4:35 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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@xxxxxxxxx/): > > > > * 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@xxxxxxxxxxxxxx/ > > > > 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@xxxxxxxxxxxxxx/ > > [3] > > https://lore.kernel.org/git/CABPp-BG2rMEYBLuBW=0wtpJe4aUFGCFa8D0NTSKz9Sm+CkXPxw@xxxxxxxxxxxxxx/ > > [4] > > https://lore.kernel.org/git/CABPp-BGCL0onSmpgKuO1k2spYCkx=v27ed9TSSxFib=OdDcLbw@xxxxxxxxxxxxxx/ > > [5] > > https://lore.kernel.org/git/CABPp-BEaemkGGm0cSofP0gau7YN-y6HFoi0yJbHA8+iGjxsYSA@xxxxxxxxxxxxxx/ > > 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 >
