On Mon, Feb 21 2022, Elijah Newren wrote: > 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: Thanks. >> 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. I don't know why I changed --write-tree to [--write-tree], I think that's just incorrect. But what I was going for was removing "(deprecated)" there. Maybe it's confusing to noone else, but it is ambiguous in the way we usually use parens in SYNOPSIS. I.e. per that syntax it means that the string "deprecated" is a mandotory paramater after <branch2>. Maybe just having a "DEPRECATED SYNOPSIS" section after "SYNOPSIS" would be better? That would also allow for more lines of examples without re-wording everything that comes after with "the first form" etc. >> +[[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. Ah, yes. That was overzelous. Sorry. I skimmed that and IIRC thought it was a re-flowing of the old DESCRIPTION section.
