[v2] rev-list-options.txt: start a list for `show-pulls`

Commit Message

Martin Ågren May 25, 2020, 5:06 p.m. UTC

On Mon, 18 May 2020 at 22:22, Junio C Hamano <[e-mail]> wrote:
>
> Martin Ågren <[e-mail]> writes:
>
> > I currently have the commit message below for my patch plus your fixup.
> > ...
>
> I've queued 1, 3, 4, 5, and 6 in the meantime.  Todd gave us a
> replacement for 2, which I also took.
>
> Thanks.

A long weekend offline passes and it's already a week later...

Here is my original patch 7/7 plus Stolee's fixup, with the rephrased
commit message from upthread. I've tried to test it from all angles I
can think of -- AsciiDoc/Asciidoctor, man/html, doc-diff, ... It should
be low-risk and does avoid beginning ~20 paragraphs with a literal "+"
in the rendered docs for this new option.

Martin

-- >8 --
The explanation of the `--show-pulls` option added in commit 8d049e182e
("revision: --show-pulls adds helpful merges", 2020-04-10) consists of
several paragraphs and we use "+" throughout to tie them together in one
long chain of list continuations. Only thing is, we're not in any kind
of list, so these pluses end up being rendered literally.

The preceding few paragraphs describe `--ancestry-path` and there we
*do* have a list, since we've started one with `--ancestry-path::`. In
fact, we have several such lists for all the various history-simplifying
options we're discussing earlier in this file.

Thus, we're missing a list both from a consistency point of view and
from a practical rendering standpoint.

Let's start a list for `--show-pulls` where we start actually discussing
the option, and keep the paragraphs preceding it out of that list. That
is, drop all those pluses before the new list we're adding here.

Helped-by: Derrick Stolee <[e-mail]>
Signed-off-by: Martin Ågren <[e-mail]>
---
 Documentation/rev-list-options.txt | 35 ++++++++++++++++--------------
 1 file changed, 19 insertions(+), 16 deletions(-)

Comments

Derrick Stolee May 26, 2020, 12:24 p.m. UTC | #1

On 5/25/2020 1:06 PM, Martin Ågren wrote:
> On Mon, 18 May 2020 at 22:22, Junio C Hamano <[e-mail]> wrote:
> @@ -671,10 +671,13 @@ important branch. Instead, the merge `N` was used to merge `R` and `X`
>  into the important branch. This commit may have information about why
>  the change `X` came to override the changes from `A` and `B` in its
>  commit message.
> +
> +--show-pulls::
> +	In addition to the commits shown in the default history, show
> +	each merge commit that is not TREESAME to its first parent but
> +	is TREESAME to a later parent.
>  +
> -The `--show-pulls` option helps with both of these issues by adding more

I like how you found a way to add the list item without needing to make a
huge shift in the surrounding prose. LGTM.

Thanks,
-Stolee

Junio C Hamano May 26, 2020, 3:16 p.m. UTC | #2

Martin Ågren <[e-mail]> writes:

> Let's start a list for `--show-pulls` where we start actually discussing
> the option, and keep the paragraphs preceding it out of that list. That
> is, drop all those pluses before the new list we're adding here.

The way the "History Simplification" section is organized is
somewhat peculiar in that it begins with a short list of what's
available, followed by mixture of detailed explanation in prose.  I
agree with you two that the result of this patch fits very well to
the surrounding text.

This is not a new issue introduced by this patch, but ...

> +--show-pulls::
> +	In addition to the commits shown in the default history, show
> +	each merge commit that is not TREESAME to its first parent but
> +	is TREESAME to a later parent.
>  +
> +When a merge commit is included by `--show-pulls`, the merge is
>  treated as if it "pulled" the change from another branch. When using
>  `--show-pulls` on this example (and no other options) the resulting
>  graph is:

... "is treated AS IF" somewhat made me go "huh?"; with or without
the option, the merge did pull the change from another branch,
didn't it?  The only effect the option has is to make that fact
stand out in the output.

But rewording it is another topic totally different from "we should
render this section correctly" fix we have here, and should be done
(if it needs to be done in the first place) separately after this
change lands.

Thanks, both.

Derrick Stolee May 26, 2020, 5:01 p.m. UTC | #3

On 5/26/2020 11:16 AM, Junio C Hamano wrote:
> Martin Ågren <[e-mail]> writes:
> 
>> Let's start a list for `--show-pulls` where we start actually discussing
>> the option, and keep the paragraphs preceding it out of that list. That
>> is, drop all those pluses before the new list we're adding here.
> 
> The way the "History Simplification" section is organized is
> somewhat peculiar in that it begins with a short list of what's
> available, followed by mixture of detailed explanation in prose.  I
> agree with you two that the result of this patch fits very well to
> the surrounding text.
> 
> This is not a new issue introduced by this patch, but ...
> 
>> +--show-pulls::
>> +	In addition to the commits shown in the default history, show
>> +	each merge commit that is not TREESAME to its first parent but
>> +	is TREESAME to a later parent.
>>  +
>> +When a merge commit is included by `--show-pulls`, the merge is
>>  treated as if it "pulled" the change from another branch. When using
>>  `--show-pulls` on this example (and no other options) the resulting
>>  graph is:
> 
> ... "is treated AS IF" somewhat made me go "huh?"; with or without
> the option, the merge did pull the change from another branch,
> didn't it?  The only effect the option has is to make that fact
> stand out in the output.

I guess the 'as if it "pulled" the change from another branch' sentence
is literally talking about the "git pull" command, as opposed to the
"git merge" command, or creating the merge upon completion of a pull request
on a Git service (which is almost always using libgit2 to generate a merge
commit).

Perhaps there is no semantic difference between "pulling" and "merging"
and then this could be reworded to be less awkward.

Thanks,
-Stolee

Martin Ågren May 26, 2020, 7:18 p.m. UTC | #4

Hi Stolee,

On Tue, 26 May 2020 at 14:24, Derrick Stolee <[e-mail]> wrote:
>
> On 5/25/2020 1:06 PM, Martin Ågren wrote:
> > On Mon, 18 May 2020 at 22:22, Junio C Hamano <[e-mail]> wrote:
> > @@ -671,10 +671,13 @@ important branch. Instead, the merge `N` was used to merge `R` and `X`
> >  into the important branch. This commit may have information about why
> >  the change `X` came to override the changes from `A` and `B` in its
> >  commit message.
> > +
> > +--show-pulls::
> > +     In addition to the commits shown in the default history, show
> > +     each merge commit that is not TREESAME to its first parent but
> > +     is TREESAME to a later parent.
> >  +
> > -The `--show-pulls` option helps with both of these issues by adding more
>
> I like how you found a way to add the list item without needing to make a
> huge shift in the surrounding prose. LGTM.

Actually, I didn't see how to do it, but luckily, you did. [1] So I take
this to mean that even on re-reading your proposed text some time later,
you like it. ;-)

Martin

[1] https://lore.kernel.org/git/[e-mail]/

Martin Ågren May 26, 2020, 7:20 p.m. UTC | #5

On Tue, 26 May 2020 at 19:01, Derrick Stolee <[e-mail]> wrote:
>
> On 5/26/2020 11:16 AM, Junio C Hamano wrote:
> > Martin Ågren <[e-mail]> writes:
> >
> > This is not a new issue introduced by this patch, but ...
> >
> >> +--show-pulls::
> >> +    In addition to the commits shown in the default history, show
> >> +    each merge commit that is not TREESAME to its first parent but
> >> +    is TREESAME to a later parent.
> >>  +
> >> +When a merge commit is included by `--show-pulls`, the merge is
> >>  treated as if it "pulled" the change from another branch. When using
> >>  `--show-pulls` on this example (and no other options) the resulting
> >>  graph is:
> >
> > ... "is treated AS IF" somewhat made me go "huh?"; with or without
> > the option, the merge did pull the change from another branch,
> > didn't it?  The only effect the option has is to make that fact
> > stand out in the output.
>
> I guess the 'as if it "pulled" the change from another branch' sentence
> is literally talking about the "git pull" command, as opposed to the
> "git merge" command, or creating the merge upon completion of a pull request
> on a Git service (which is almost always using libgit2 to generate a merge
> commit).
>
> Perhaps there is no semantic difference between "pulling" and "merging"
> and then this could be reworded to be less awkward.

Agreed on the awkwardness as it stands (before or after this proposed
patch). I don't have any concrete thoughts to offer though.

Martin

Patch

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 04ad7dd36e..b01b2b6773 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -581,12 +581,12 @@  option does. Applied to the 'D..M' range, it results in:
 
 Before discussing another option, `--show-pulls`, we need to
 create a new example history.
-+
+
 A common problem users face when looking at simplified history is that a
 commit they know changed a file somehow does not appear in the file's
 simplified history. Let's demonstrate a new example and show how options
 such as `--full-history` and `--simplify-merges` works in that case:
-+
+
 -----------------------------------------------------------------------
 	  .-A---M-----C--N---O---P
 	 /     / \  \  \/   /   /
@@ -595,7 +595,7 @@  such as `--full-history` and `--simplify-merges` works in that case:
 	  \ /      /\        /
 	   `---X--'  `---Y--'
 -----------------------------------------------------------------------
-+
+
 For this example, suppose `I` created `file.txt` which was modified by
 `A`, `B`, and `X` in different ways. The single-parent commits `C`, `Z`,
 and `Y` do not change `file.txt`. The merge commit `M` was created by
@@ -607,19 +607,19 @@  the contents of `file.txt` at `X`. Hence, `R` is TREESAME to `X` but not
 contents of `file.txt` at `R`, so `N` is TREESAME to `R` but not `C`.
 The merge commits `O` and `P` are TREESAME to their first parents, but
 not to their second parents, `Z` and `Y` respectively.
-+
+
 When using the default mode, `N` and `R` both have a TREESAME parent, so
 those edges are walked and the others are ignored. The resulting history
 graph is:
-+
+
 -----------------------------------------------------------------------
 	I---X
 -----------------------------------------------------------------------
-+
+
 When using `--full-history`, Git walks every edge. This will discover
 the commits `A` and `B` and the merge `M`, but also will reveal the
 merge commits `O` and `P`. With parent rewriting, the resulting graph is:
-+
+
 -----------------------------------------------------------------------
 	  .-A---M--------N---O---P
 	 /     / \  \  \/   /   /
@@ -628,21 +628,21 @@  merge commits `O` and `P`. With parent rewriting, the resulting graph is:
 	  \ /      /\        /
 	   `---X--'  `------'
 -----------------------------------------------------------------------
-+
+
 Here, the merge commits `O` and `P` contribute extra noise, as they did
 not actually contribute a change to `file.txt`. They only merged a topic
 that was based on an older version of `file.txt`. This is a common
 issue in repositories using a workflow where many contributors work in
 parallel and merge their topic branches along a single trunk: manu
 unrelated merges appear in the `--full-history` results.
-+
+
 When using the `--simplify-merges` option, the commits `O` and `P`
 disappear from the results. This is because the rewritten second parents
 of `O` and `P` are reachable from their first parents. Those edges are
 removed and then the commits look like single-parent commits that are
 TREESAME to their parent. This also happens to the commit `N`, resulting
 in a history view as follows:
-+
+
 -----------------------------------------------------------------------
 	  .-A---M--.
 	 /     /    \
@@ -651,18 +651,18 @@  in a history view as follows:
 	  \ /      /
 	   `---X--'
 -----------------------------------------------------------------------
-+
+
 In this view, we see all of the important single-parent changes from
 `A`, `B`, and `X`. We also see the carefully-resolved merge `M` and the
 not-so-carefully-resolved merge `R`. This is usually enough information
 to determine why the commits `A` and `B` "disappeared" from history in
 the default view. However, there are a few issues with this approach.
-+
+
 The first issue is performance. Unlike any previous option, the
 `--simplify-merges` option requires walking the entire commit history
 before returning a single result. This can make the option difficult to
 use for very large repositories.
-+
+
 The second issue is one of auditing. When many contributors are working
 on the same repository, it is important which merge commits introduced
 a change into an important branch. The problematic merge `R` above is
@@ -671,10 +671,13 @@  important branch. Instead, the merge `N` was used to merge `R` and `X`
 into the important branch. This commit may have information about why
 the change `X` came to override the changes from `A` and `B` in its
 commit message.
+
+--show-pulls::
+	In addition to the commits shown in the default history, show
+	each merge commit that is not TREESAME to its first parent but
+	is TREESAME to a later parent.
 +
-The `--show-pulls` option helps with both of these issues by adding more
-merge commits to the history results. If a merge is not TREESAME to its
-first parent but is TREESAME to a later parent, then that merge is
+When a merge commit is included by `--show-pulls`, the merge is
 treated as if it "pulled" the change from another branch. When using
 `--show-pulls` on this example (and no other options) the resulting
 graph is: