Re: [PATCH v3 17/20] range-diff: add a man page

[Stefan Beller <sbeller@google.com>]

On Mon, Jul 9, 2018 at 1:00 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Stefan,
>
> On Mon, 9 Jul 2018, Stefan Beller wrote:
>
> > On Tue, Jul 3, 2018 at 4:26 AM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >
> > > +'git range-diff' [--color=[<when>]] [--no-color] [<diff-options>]
> > > +       [--dual-color] [--creation-factor=<factor>]
> > > +       ( <range1> <range2> | <rev1>...<rev2> | <base> <rev1> <rev2> )
> > > +
> > > +DESCRIPTION
> > > +-----------
> > > +
> > > +This command shows the differences between two versions of a patch
> > > +series, or more generally, two commit ranges (ignoring merges).
> >
> > Does it completely ignore merges or does it die("not supported"), how is
> > the user expected to cope with the accidental merge in the given range?
>
> It ignores merges. It does not reject them. It simply ignores them and
> won't talk about them as a consequence.
>
> Could you suggest an improved way to say that?

Well that is what the patch said already; I was just dense in reading.
I just tested it, and the commit with more than one parent itself is
ignored (not showing up in the output), but the commits that are merged
in are still considered. So giving range1 as
f7761a5a065..0a5677f6f68 with

  0a5677f6f68 (Merge branch 'js/branch-diff' into pu, 2018-07-06)
  f7761a5a065 (Merge branch 'jk/fsck-gitmodules-gently' into jch, 2018-07-06)

still produces cool output and with --word-diff it is even more amazing, as it
just tells me a large part was s/branch-diff/range-diff/ :-)

12:  cbc752c57ce ! 12:  7273cc64797 branch-diff: use color for the commit pairs
    @@ -1,31 +1,28 @@
    Author: Johannes Schindelin <johannes.schindelin@gmx.de>

        [-branch-diff:-]{+range-diff:+} use color for the commit pairs

        Arguably the most important part of [-branch-diff's-]{+`git
range-diff`'s+} output is the
        list of commits in the two branches, together with their relationships.

        For that reason, tbdiff introduced color-coding that is pretty
        intuitive, especially for unchanged patches (all dim yellow, like the
        first line in `git show`'s output) vs modified patches (old commit is
        red, new commit is green). Let's imitate that color scheme.

    [-    While at it, also copy tbdiff's change of the fragment color
to magenta.-]

        Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
    [-    Signed-off-by: Junio C Hamano <gitster@pobox.com>-]

Sorry for being offtopic here; I do not have a better suggestion than what
is already said.


> > I presume this is a boolean option, and can be turned off with
> > --no-dual-color, but not with --dual-color=no. Would it be worth to
> > give the --no-option here as well.
> > The more pressing question I had when reading this, is whether this
> > is the default.
>
> In the final patch (which I mulled about adding or not for a couple of
> weeks), the `--dual-color` mode is the default, and the man page talks
> about `--no-dual-color`.
>
> Do you want me to change this intermediate commit, even if that change
> will be reverted anyway?

No, I just wasn't aware of that part, yet, as I have seen some patch series
that add the man page/documentation as their final patch. This looked
so similar that I assumed this is the final man page. My bad!


> The coloring gives a strong hint of "pre" vs "post", i.e. old vs new: the
> changes that are only in the "old" patches are marked with a minus with a
> red background color, which only really makes sense if you think about
> these changes as "dropped" or "removed" from the "new" changes.
>
> So yes, it is really considered an older version, in my mind.
>
> Again, if you have suggestions how to improve my patch (giving rise to a
> "new" patch :-)), let's hear them.

I will send patches as I get more used to this new tool.

>
> > I think this comes from the notion of e.g. patch 4 ("range-diff: improve the
> > order of the shown commits "), that assume the user wants the range-diff
> > to be expressed with range2 as its "base range".
>
> No, it is motivated by the fact that we use -/+ markers to indicate
> differences between the "old" and the "new" patches.

And at this point in time we do not want to question the use of -/+ markers
for the next layer of abstraction. While +/- are well understood on
the patch level
we could argue for a different set of characters for diffs of diffs,
as that helps
to differentiate between the layers ("is it added in the diff or the
diff of the diff
due to e.g. different context?"), but then it would not recurse. (I am
not sure I
want to read diff of diffs of diffs)

Okay, for now I accept the terms of old and new patches, as I have no
better idea.

>
> > > +Algorithm
> > > +---------
> > > +
> > > +The general idea is this: we generate a cost matrix between the commits
> > > +in both commit ranges, then solve the least-cost assignment.
> >
> > Can you say more about the generation of the cost matrix?
> > I assume that it counts the number of lines added/deleted to make
> > one patch into the other patch.
>
> I think that is correct.
>
> *reading the patch*
>
> Actually, no, I was wrong. For the cost matrix, the *length* of the diff
> *of the diffs* is computed. Think of it as
>
>         git diff --no-index <(git diff A^!) <(git diff B^!) | wc -l

So the matching is based only on diffs, but the output still takes
the commit messages into account. So when diffing my series to the
series that Junio applies, (merely adding his sign off,) would be a
"cost of 0" in this context, but I still have output.

>
> > Another spot to look at is further metadata, such as author and
> > author-date, which are kept the same in a rebase workflow.
>
> I encourage you to offer that as an add-on patch series. Because what you
> suggest is not necessary for my use cases, so I'd rather not spend time on
> it.

Makes sense. When I stumble about this yet theoretical problem to materialize
in practice I will send a patch. In my mind this is not another use
case, but just
an improved matching, with the matching that this series provides being
good enough for now.

Thanks,
Stefan