Re: [PATCH v2 1/6] doc: add new gitmergeconflicts man page

["Julia Evans" <julia@jvns.ca>]

Thanks for the review, I'm especially excited about the idea to remove zdiff3. 

>> Co-Authored-By: Marie Claire LeBlanc Flanagan <hello@marieflanagan.com>
>> Reviewed-by: D. Ben Knoble <ben.knoble+github@gmail.com>
>> Reviewed-by: Patrick Steinhardt <ps@pks.im>
>> Signed-off-by: Julia Evans <julia@jvns.ca>
>
> We want a sign-off by the coauthor, too.

Will do.

>> diff --git a/Documentation/gitmergeconflicts.adoc b/Documentation/gitmergeconflicts.adoc
>> new file mode 100644
>> index 0000000000..5b0ba1a1de
>> --- /dev/null
>> +++ b/Documentation/gitmergeconflicts.adoc
>> @@ -0,0 +1,333 @@
>> +gitmergeconflicts(7)
>> +====================
>> +
>> +NAME
>> +----
>> +gitmergeconflicts - Guide to handling merge conflicts
>> +
>> +DESCRIPTION
>> +-----------
>> +
>> +Merge conflicts can happen during a `git merge`, `git rebase`, `git
>> +cherry-pick`, `git pull`, or `git revert`. All of those commands use
>> +the same merge algorithm, and the process for resolving a merge conflict
>> +is always very similar.
>
> Is it deliberate to omit 'am -3' and 'checkout -m', perhaps in order
> to limit ourselves to most common ways to help new people by keeping
> the description to the absolute minimum?

It's deliberate, we talked about that a bit in the discussion of the v1.
Can add a note in the commit message.

> In any case, the first paragraph clearly stating that conflicts
> happen with operations other than 'merge' is a very welcome change.
> On this list, we often say "mergy operations can cause conflicts",
> with the understanding that readers know what mergy operations are
> and "conflicts" alone can convey the state you call "merge
> conflicts" in this document.  But in a document for end-users, using
> a longer term "merge conflicts" instead of "conflicts" and avoiding
> "mergy operations" like you did above may be a better direction to
> go.
>
>> +The most common ways to handle a merge conflict are:
>
> A natural paraphrase of the above is "A merge conflict is typically
> handled by these ways", but I thought the current readers are
> puzzled by "typically your side" that does not say when is typical.
>
>> +* Resolve the conflict. (see <<resolve,HOW TO RESOLVE A MERGE CONFLICT>>
>> +  below for details)
>> +* Or stop the operation and return your branch to its original state
>> +  with the appropriate `--abort` command, for example `git merge --abort`
>> +  or `git rebase --abort`. See <<git_status,EXAMPLE: GIT STATUS OUTPUT>> below
>> +  for how to find the command to run.
>
> Both are good options and I do not think of a middle way.  Perhaps
> we do not have to say that these are "the most common" and instead
> say "You handle a merge conflict by doing either of these two"?

I agree the "the most common" is kind of weaselly and I'd like to be more clear.
The reason I wrote "typically" is that during a rebase, there's an extra
"skip" option, so it's not strictly true to say that there are just two options.
Not sure if there's another option I'm not thinking of other than the
"skip" in rebase.

> Saying "return your branch to" is a bit misleading for two reasons.
> Conflicts presented to the users are primarily visible in their
> working tree files and the index.  A single commit operations like
> 'merge', 'pull', and 'revert' does not touch your branch if they hit
> a conflict, and 'rebase' works on a detached HEAD, and stops without
> touching your branch when it sees a conflict.
>
> If I were writing this, with the goal of avoiding the issues the
> current text has you listed in the proposed log message, I would
> probably say something like this:
>
>  * Or give up and return to the original state with ...

That's reasonable, I think "return to the original state" would be fine.
Will look at this.

>
>> +WHAT IS A MERGE CONFLICT?
>> +-------------------------
>> +
>> +When Git merges two commits together, it looks at the changes that
>> +each side has made and combines those changes. For example, if one side
>> +edited lines 1-5 of `hello.py` and the other side edited lines 20-25 of
>> +the same file, then it can easily combine them since there's no overlap.
>
> Many of the operations, even "git merge", is not about merging "two
> commits" together, but I do not think of a good way to explain it,
> so I accept that phrasing as a helpful white lie.  I mention this
> because somebody else may be able to come up with a better phrasing
> that I (or authors of this iteration) couldn't think of.
>
>> +But if both sides edited overlapping lines of the same file (for example
>> +one side edited lines 1-5 and the other edited lines 3-6), Git will
>> +not try to guess how to combine those changes. This is called a "merge
>> +conflict".
>> +
>> +When this happens, Git shows you both sides' edits and asks you to pick
>> +how to resolve them. It:
>> +
>> +* Stages all of the files which were successfully merged
>> +* For the files with conflicts, it marks them as conflicted, puts both
>> +  sides' edits in the file, and leaves <<markers, merge conflict markers>>
>> +  that you need to resolve.
>
> By the way, I think we should briefly mention what sematnic merge
> conflicts are, and that Git does not detect them and that this
> manual page does not tell readers how to deal with them.
>    
>     Note that non-overlapping changes from two sides may leave the
>     result in an inconsistent state.  With the edit to lines 1-5,
>     one side may have changed the name of a function, while with the
>     edit to lines 20-25, the other side may have added a new call to
>     the function by its original name.  This type of inconsistencies
>     are called semantic conflicts, Git has no way knowing that a
>     merge introduced semantic conflicts, and ends up producing a
>     broken result without merge conflicts.  This document does not
>     cover what to do with semantic conflicts.
>
> That is overly long, but perhaps you can condense it down to the
> essense and shrink down to 1/3 of the size.

I was thinking about that too. Maybe we can briefly mention that git's
merges are not guaranteed to produce working code even when they
succeed and point to an example further down the page.
Added to my list of things to work on.

>> +[[markers]]
>> +MERGE CONFLICT MARKERS
>> +----------------------
>> +
>> +When there's a merge conflict, Git will update the conflicted file
>> +to include merge conflict markers `<<<<<<<`, `=======`, and `>>>>>>>`.
>
> I notice that when you introduce `diff3` below, you silently add
> `|||||||` to the mix without explaining what it is.
>
>     `|||||||` may also be used as merge conflict markers (explained
>     later).
>
> or something along the line here may help.  Or explain what it is in
> `diff3` section.  Either would work.  Adding without explanation
> would not.

I think explaining it in the diff3 section makes sense, will do.

>> +For example, here's a merge conflict where both sides edited a list of
>> +fruits in different ways:
>> +
>> +----
>> +FRUITS = [
>> +    "apple",
>> +<<<<<<< HEAD
>> +    "cherry",
>> +=======
>> +    "banana",
>> +>>>>>>> add-fruit
>> +    "mango",
>> +    "orange",
>> +]
>> +----
>> +
>> +The code from one side of the merge conflict is between `<<<<<<<` and
>> +`=======`, and the code for the other side is between `=======` and
>> +`>>>>>>>`. See <<ours,"OURS" AND "THEIRS">> below for a full explanation
>> +of which side is which.
>
> If we said "one side wanted to have 'apple, cherry, mango, orange',
> while the other side wanted 'apply, banana, mango, orange', in the
> FRUITS array", would it help the understanding?  Or is it too
> obvious?

I think it could make sense to add something here yes.
Added to my list.

>> +[[example]]
>> +EXAMPLE OF RESOLVING A MERGE CONFLICT
>> +-------------------------------------
>> +
>> +If you see this in your code during a merge conflict:
>> +
>> +----
>> +FRUITS = [
>> +    "apple",
>> +<<<<<<< HEAD
>> +    "cherry",
>> +    "mango",
>> +=======
>> +    "banana",
>> +    "mango",
>> +>>>>>>> add-fruit
>> +    "orange",
>> +]
>> +----
>
> It would make your readers puzzled why the example is subtly
> different from the earlier one that showed "mango" as not touched by
> either side.  I see this lays the groundwork for later demonstration
> of `diff3`, so having both sides explicitly want "mango" is a good
> example.  Perhaps update the first example to be the same as this
> one, which would reduce the mental burden by readers?

I very much agree it's important for the examples to match,
will work on that.  It's a bit tricky with diff3 like you say.

>> +[[tools]]
>> +TOOLS FOR HANDLING MERGE CONFLICTS
>> +----------------------------------
>> +
>> +Here are some ways to get extra context while handling a merge conflict:
>> +
>> +* There are many graphical "merge tools" for Git, which will normally
>> +  show you the different versions of the code side by side.
>> +  If you have a mergetool configured, `git mergetool` will launch it.
>> +  See also `merge.tool` in linkgit:git-config[1] for a list of
>> +  the mergetools Git supports.
>> +
>> +* You can set the configuration option `merge.conflictstyle=diff3`.
>> +  See <<diff3,DIFF3 AND ZDIFF3>> below for more.
>
> These are called 'configuration variables' throughout the manual
> pages.  Be consistent and replace "configuration option" with
> "configuration variable", perhaps?

They seem to be both used interchangeably already:

```
$ grep 'configuration variable' *.adoc | wc -l
     256
$ grep 'configuration option' *.adoc | wc -l
      46
```

`git-config.adoc` uses the term "configuration option" 2 times
and "configuration variable" once. Is there supposed to be
some difference between these terms? As far as I can tell
from brief history spelunking Git has used those terms
interchangeably for a long time. AFAIK "configuration option"
is the term more often used outside Git.

>> +* `git log --merge -p <filename>`  will list all commits which
>> +  caused the merge conflict for `<filename>`, and the diff
>> +  of how they changed the file.
>
> Maybe worth mentioning that `--left-right` often helps when you are
> not super familiar with the histories being merged.

I don't understand what this does or what it would be useful for so
it's not possible for me to explain it  :). From my perspective
"ours" and "theirs" are already confusing enough and introducing
"left" and "right" seems like a lot. Is "left" the same as "ours"?

>> +* Look at the original files.  `git show :1:filename` shows the
>> +  common ancestor, `git show :2:filename` shows the "ours"
>> +  version, and `git show :3:filename` shows the "theirs"
>> +  version.
>
> Maybe it will help to say we will explain "ours" and "theirs" later
> in this document.

Plausible, added to my todo list to look at, thanks.

>> +* Use `git diff AUTO_MERGE` to show what changes you've made so far to
>> +  resolve the conflicts.
>
> Does a "See below" here help readers who haven't learned what
> AUTO_MERGE is?  If you can describe what AUTO_MERGE records (in
> other words, what you are comparing your progress against) in a
> sentence of two here, that would alleviate the need to assure them
> that we have more in-depth coverage on this topic elsewhere.

I think this is okay the way it is.

>> +[[git_status]]
>> +EXAMPLE: GIT STATUS OUTPUT
>> +--------------------------
>> +
>> +When you're in a merge conflict, you can find out what commands to run
>> +to handle the conflict by running `git status`.
>> +
>> +For example, this `git status` output tells you that:
>> +
>> +* `git rebase --abort` will safely bring your branch back to its
>> +  original state
>> +* you should run `git rebase --continue` when you're done resolving all
>> +  the conflicts
>> +* there's one file left with conflicts in it: `fruits.py`
>
> There may be users, after seeing the last point, left puzzled why
> fruits.py is still listed after they edited the file like instructed
> in an earlier example but haven't marked the resolution.
>
>     `fruits.py` is not marked as its conflicts resolved yet.
>
> or something?

Thanks, agreed that "there's one file left with conflicts in it: `fruits.py`" isn't
precise enough. Will make it more accurate.

>> +----
>> +$ git status
>> +You are currently rebasing branch 'main' on '58a9fcc'.
>> +  (fix conflicts and then run "git rebase --continue")
>> +  (use "git rebase --skip" to skip this patch)
>> +  (use "git rebase --abort" to check out the original branch)
>> +
>> +Unmerged paths:
>> +  (use "git restore --staged <file>..." to unstage)
>> +  (use "git add <file>..." to mark resolution)
>> +        both modified:   fruits.py
>> +----
>
> The approach to give explanations first and then an example the
> explanation explains next is refreshing to me.  As long as the
> explanations are short enough, this may work better than the usual
> order to say "you'd see something like this. let us explain ...".

Glad to hear it!

>> +[[diff3]]
>> +DIFF3 AND ZDIFF3
>> +----------------
>> +
>> +By default, Git doesn't include the original code when formatting
>> +a merge conflict. To include the original code, you can set the
>> +configuration option `merge.conflictstyle` to `diff3` or `zdiff3`.
>> +This extra context can make it much easier to understand what's
>> +happening in a merge conflict.
>
> I think most on the list considers `zdiff3` a failed experiment that
> reduces usefulness of `diff3`.  Do we want to recommend it?

I'd be extremely happy to remove this if the list doesn't think zdiff3
is useful. When I was writing this I was confused by zdiff3 and thought
diff3 made a lot more sense.

Maybe we could add a note like this somewhere?

   NOTE: zdiff3 was an experimental alternative to diff3 that makes
   the merge conflict shorter by introducing more ambiguity.
   It's still there for backwards compatibility but we don't recommend it.


> By the way, is it just me who finds those "Here's", "there's"
> contractions disturbing in an official manual?  I've seen many of
> them while reviewing this to be annoyed enough and had to blurt it
> out X-<.

I find "here is" and "there is" to be distracting and overly formal,
different people are different I guess :)

>> +"Ours" and "theirs" are both commits: "ours" is the current
>> +`HEAD` commit, and "theirs" is the other side being merged.
>
> Now, "commits" is again a white lie.  The story becomes more
> complicated when we talk about cherry-pick and revert, but if we
> primarily stick to what happens in 'merge' (which is what I've seen
> so far in this document), then it shouldn't add any extra difficulty
> to understand by saying "ours and theirs are history of changes
> leading to the two commits since they diverged from the original" to
> add clarity.
>
>> +The first part of a merge conflict (between `<<<<<<<` and `=======`) is
>> +from the "ours" side, and the second part (between `=======` and
>> +`>>>>>>>`) is from the "theirs" side.
>> +
>> +----
>> +FRUITS = [
>> +    "apple",
>> +<<<<<<< HEAD
>> +    "cherry",                      <- ours
>> +=======
>> +    "banana",                      <- theirs
>> +>>>>>>> add-fruit
>> +    "mango",
>> +    "orange",
>> +]
>> +----
>> +
>> +During a rebase, it can seem "upside down" because the "ours" commit is
>> +from the branch you're rebasing on (for instance `main` in `git rebase
>> +main`).
>> +
>> +These terms in Git all mean the same thing when dealing with a merge
>> +conflict:
>> +
>> +* "common ancestor" and "base". The files from this commit are "in stage 1".
>> +* "ours", "us", and `HEAD`. The files from this commit are "in stage 2".
>> +* "theirs", "them". The files from this commit are "in stage 3".
>> +
>> +If you're confused about what something like "deleted by us" means, it's
>> +often easiest to use some of the tools from
>> +<<tools,TOOLS FOR HANDLING MERGE CONFLICTS>> above to get more context.
>> +Finding the commit that deleted the file and seeing why is usually more
>> +helpful than trying to abstractly reason through what "us" means.
>> +
>> +[[automerge]]
>> +EXAMPLE OF USING `AUTO_MERGE`
>> +-----------------------------
>> +
>> +`git diff AUTO_MERGE` will show what changes you've made so far to
>> +resolve conflicts. `AUTO_MERGE` is a reference that Git creates during a
>> +merge. It contains the result of running the merge algorithm.
>
> The first sentence gave me "Huh?  You haven't explained what
> AUTO_MERGE is yet".  It may be just me, but I would have expected
> presentation order to be more like:
>
>     When merge conflicts happen, the result of merge algorithm,
>     together with conflict markers, is recorded in AUTO_MERGE.  As
>     you resolve conflicts, you can compare your working tree files
>     against it with `git diff AUTO_MERGE` to see your progress.

Can take a look but I don't think it makes a big difference.

>> +For example, if we resolved the conflict by adding both "banana" and
>> +"cherry" in order, the diff would look like this:
>> +
>> +----
>> + FRUITS = [
>> +     "apple",
>> +-<<<<<<< HEAD
>> +-    "cherry",
>> +-=======
>> +     "banana",
>> +->>>>>>> add-fruit
>> ++    "cherry",
>> +     "mango",
>> +     "orange",
>> + ]
>> +----
>
> Thanks.
>
>
> I was puzzled by this
>
>> - In "The only clean-ups you need are to reset the index file to the
>>   `HEAD` commit to reverse 2. and to clean up working tree changes made
>>   by 2. and 3.", it's not clear to users what "2" and "3" are supposed
>>   to mean
>
> and did some digging.
>
> The text comes from ffb1a4bed5 (Documentation: Describe merge
> operation a bit better., 2005-11-28) that had "When there are
> conflicts, these things happen. 1. HEAD does not move, 2. Cleanly
> merged paths are updated in the index 3. Conflicts are recorded in
> higher stage index entries and working tree files show conflict
> markers, 4. No other changes are done" well before the mysterious
> reference to 2. and 3.
>
> When ebef7e5049 (Documentation: simplify How Merge Works,
> 2010-01-23) tried to simplify the description, the list of "these
> things happen" were removed/rewritten, and yet instructions on how
> to reset are left behind, still referring to 2. and 3.
>
> We probably want a separate patch for Documentation/git-merge.adoc
> to rectify this 16 year old mistake.

Thanks for investigating!