Re: [PATCH 5/6] Documentation/git-merge.txt: improve short description in DESCRIPTION

From: Jakub Narębski <jnareb@gmail.com>
Date: Oct 5, 2016, 16:58 UTC
Message-ID: <e03a84ad-9aa6-8ada-5828-7b28f76baaaf@gmail.com>
In-Reply-To: <e74ae8afc1bfc4cd9161ccaa56d926a89439551e.1475678515.git.sorganov@gmail.com>

W dniu 05.10.2016 o 16:46, sorganov@gmail.com pisze:

Show 12 quoted lines

> From: Sergey Organov <sorganov@gmail.com>
> 
> Old description had a few problems:
> 
> - sounded as if commits have changes
> 
> - stated that changes are taken since some "divergence point"
>   that was not defined.
> 
> New description rather uses "common ancestor" and "merge base",
> definitions of which are easily discoverable in the rest of GIT
> documentation.

This is a step in a good direction, but it has a few issues.

Show 25 quoted lines

> 
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  Documentation/git-merge.txt | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/git-merge.txt b/Documentation/git-merge.txt
> index cc0329d..351b8fc 100644
> --- a/Documentation/git-merge.txt
> +++ b/Documentation/git-merge.txt
> @@ -16,11 +16,16 @@ SYNOPSIS
>  
>  DESCRIPTION
>  -----------
> -Incorporates changes from the named commits (since the time their
> -histories diverged from the current branch) into the current
> -branch.  This command is used by 'git pull' to incorporate changes
> -from another repository and can be used by hand to merge changes
> -from one branch into another.
> +
> +Incorporates changes that lead to the named commits into the current
> +branch, and joins corresponding histories. The best common ancestor of
> +named commits and the current branch, called "merge base", is
> +calculated, and then net changes taken from the merge base to
> +the named commits are applied.

The first sentence is all right; it reads better than the original without the introduced part in parentheses. The only minor issue is with "joins corresponding histories" - it is a good description, but may imply that the branch we are merging vanishes: it doesn't. But all in all, it is a good change.

Second sentence has some problems. First, while it is a good idea to use well defined term "merge base", I think writing "since the time their histories diverged" or "(which is the point where histories diverged)" would be a good plain language description; it was removed entirely in the proposal.

Second, while "common ancestor" and "least common ancestor" are well defined in mathematics of graphs, "best common ancestor" isn't... but this is what git-merge-base(1) documentation uses.

Also, the "best common ancestor" doesn't need to be only one. There might be many such ancestors... though Git would generate then a virtual best common ancestor thanks to recursive merge strategy. And usually there is only one "best common ancestor", that is a single merge base. So this may need clarification, but it is not much of a problem.

Third, and most important, is that "net changes taken from the merge base to the named commits are applied" is simply not true. The `git merge` command does not reapply changes - that is what rebase and cherry-pick do. The merge operation uses 3-way merge strategy (diff3) between merge-base, current branch, and merged commit. That is, it finds differences between differences, and "applies" that.

See "A Formal Investigation of Diff3" paper by Sanjeev Khanna,
Keshav Kunal, and Benjamin C. Pierce:
  http://www.cis.upenn.edu/~bcpierce/papers/diff3-short.pdf

I'm not sure how to explain it succintly.  Perhaps

  net changes between merge base to the current (merged into)
  branch and named commits are integrated

There is description of trivial 3-way merge somewhere in Git docs, though in very unobvious place; we can link it.

> +
> +This command is used by 'git pull' to incorporate changes from another
> +repository, and can be used by hand to merge changes from one branch
> +into another.

Rather "can be used by 'git pull'", or "is used by 'git pull' (unless configured otherwise)"...

Separating this information makes a very good sense.  Thanks.

Show 17 quoted lines

>  
>  Assume the following history exists and the current branch is
>  "`master`":
> @@ -31,11 +36,11 @@ Assume the following history exists and the current branch is
>      D---E---F---G master
>  ------------
>  
> -Then "`git merge topic`" will replay the changes made on the
> -`topic` branch since it diverged from `master` (i.e., `E`) until
> -its current commit (`C`) on top of `master`, and record the result
> -in a new commit along with the names of the two parent commits and
> -a log message from the user describing the changes.
> +Then "`git merge topic`" will replay the changes made on the `topic`
> +branch since it diverged from `master` (i.e., `E`) until its current
> +commit (`C`) on top of `master`, and record the result in a new commit
> +along with references to the two parent commits and a log message from
> +the user describing the changes.

What the happened here!?! Please do not rewrap documentation, especially not without changes!

>  
>  ------------
>  	  A---B---C topic
>

-- 
Jakub Narębski