Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Hi Junio, On Fri, 10 Aug 2018, Junio C Hamano wrote: > Johannes Schindelin writes: > > > I'll just try to get that option parsing change in that Eric suggested, > > force-push, then wait for macOS and Linux builds to pass (trusting that > > Windows will follow suite) and hit /submit. > > OK. Obviously receiving, applying and inspecting that result will > not be done in time for today's integration cycle, but having a > version that limits its cope and is suitable for 'next' is a good > thing to have at this point during the cycle. Correct whitespace > error colouring, etc., can be done on top after the basics settle, > and the basics were good in a few versions ago already IIRC. No, a couple of issues were identified, still, that merited several dozen rebases on my side. Ciao, Dscho
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Johannes Schindelin writes: > I'll just try to get that option parsing change in that Eric suggested, > force-push, then wait for macOS and Linux builds to pass (trusting that > Windows will follow suite) and hit /submit. OK. Obviously receiving, applying and inspecting that result will not be done in time for today's integration cycle, but having a version that limits its cope and is suitable for 'next' is a good thing to have at this point during the cycle. Correct whitespace error colouring, etc., can be done on top after the basics settle, and the basics were good in a few versions ago already IIRC.
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Hi Stefan,
On Wed, 8 Aug 2018, Stefan Beller wrote:
> On Wed, Aug 8, 2018 at 6:05 AM Johannes Schindelin
> wrote:
>
> > > [...]
> > > > diff --git a/Makefile b/Makefile
> > > > --- a/Makefile
> > > > +++ b/Makefile
> > >
> > > The line starting with --- is red (default removed color) and the line
> > > with +++ is green (default add color).
> > >
> > > Ideally these two lines and the third line above starting with "diff
> > > --git"
> > > would render in GIT_COLOR_BOLD ("METAINFO").
> >
> > I agree that is not the best coloring here, but as you remarked elsewhere,
> > it would require content-aware dual coloring, and I am loathe to try to
> > implement that for two reasons: 1) it would take most likely a long time
> > to design and implement that, and 2) I don't have that time.
> >
> > So I would like to declare that good enough is good enough in this case.
>
> I anticipated this answer, so I wrote some patches myself, starting at
> https://public-inbox.org/git/20180804015317.182683-1-sbel...@google.com/
> specifically
> https://public-inbox.org/git/20180804015317.182683-5-sbel...@google.com/
>
> I plan on resending these on top of your resend (if any) at a later convenient
> time for both you and Junio, as noted in
> https://public-inbox.org/git/cagz79kznvesvpicnu7lxkrchurqgvesfvg3dl5o_2kpvyrw...@mail.gmail.com/
Thank you!
> > > > 3: 076e1192d ! 3: 4e3fb47a1 range-diff: first rudimentary
> > > > implementation
> > > > @@ -4,7 +4,7 @@
> > > >
> > > > At this stage, `git range-diff` can determine corresponding
> > > > commits
> > > > of two related commit ranges. This makes use of the recently
> > > > introduced
> > > > -implementation of the Hungarian algorithm.
> > > > +implementation of the linear assignment algorithm.
> > > >
> > > > The core of this patch is a straight port of the ideas of
> > > > tbdiff, the
> > > > apparently dormant project at https://github.com/trast/tbdiff.
> > > > @@ -51,19 +51,17 @@
> > > > + int res = 0;
> > > > + struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
> > > >
> > > > -- argc = parse_options(argc, argv, NULL, options,
> > > > -- builtin_range_diff_usage, 0);
> > > > -+ argc = parse_options(argc, argv, NULL, options,
> > > > builtin_range_diff_usage,
> > > > -+ 0);
> > > > + argc = parse_options(argc, argv, NULL, options,
> > > > + builtin_range_diff_usage, 0);
> > >
> > > This is really nice in colors when viewed locally.
> > >
> > > > 16: dfa7b1e71 < -: - range-diff --dual-color: work around
> > > > bogus white-space warning
> > > > -: - > 16: f4252f2b2 range-diff --dual-color: fix bogus
> > > > white-space warning
> > >
> > > Ah; here my initial assumption of only reviewing the range-diff breaks
> > > down now.
> > > I'll dig into patch 16 separately.
> >
> > Right. This was an almost complete rewrite, and then next iteration will
> > hopefully bring another complete rewrite: disabling whitespace warnings in
> > dual color mode.
> >
> > > Maybe it is worth having an option to expand all "new" patches.
> >
> > Sure.
> >
> > And I also have a use case for --left-only/--right-only.
> >
> > And I also have a strong use case (and so does Junio, it seems, or for
> > that matter, anybody contributing to Git due to Junio's insistence on
> > signing off on each patch, rather than on the merge commit) for something
> > like --ignore-lines=.
> >
> > And you probably guess what I will say next: these features will make for
> > really fantastic patch series *on top* of mine. There really is no good
> > reason to delay the current patch series just to cram more features into
> > it that had not been planned in the first place.
>
> Yes, I agree.
I am happy to hear that.
> I am unsure about the current state of your series, though;
>
> Junio thinks (expects?) a resend, whereas you seem to call it good enough
> but also said (some time back) that you want to resend due to Thomas
> feedback.
Sorry about being so unclear. When time gets scarce, sometimes I get too
stressed (and too short on time) to communicate properly.
Yes, I want to limit the new features put into this patch series, in the
interest of getting things into `next` (and maybe still into `master`
before v2.19, but I am not allowing myself to hope for that too much).
And yes, I want to send another iteration, as there have been too many
changes that I do not want to ask Junio to touch up, in particular because
I am a little bit of a detail-oriented person and want my fixes just so.
> I do have 2 series on top of the current range-diff.
> * The first (queued by Junio as origin/sb/range-diff-colors)
>adds a basic test for colors and improves diff.c readability
> * The second (linked above) changes colors for some lines.
>
> I do not want to build more on top as long
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
On Wed, Aug 8, 2018 at 6:05 AM Johannes Schindelin
wrote:
> > [...]
> > > diff --git a/Makefile b/Makefile
> > > --- a/Makefile
> > > +++ b/Makefile
> >
> > The line starting with --- is red (default removed color) and the line
> > with +++ is green (default add color).
> >
> > Ideally these two lines and the third line above starting with "diff --git"
> > would render in GIT_COLOR_BOLD ("METAINFO").
>
> I agree that is not the best coloring here, but as you remarked elsewhere,
> it would require content-aware dual coloring, and I am loathe to try to
> implement that for two reasons: 1) it would take most likely a long time
> to design and implement that, and 2) I don't have that time.
>
> So I would like to declare that good enough is good enough in this case.
I anticipated this answer, so I wrote some patches myself, starting at
https://public-inbox.org/git/20180804015317.182683-1-sbel...@google.com/
specifically
https://public-inbox.org/git/20180804015317.182683-5-sbel...@google.com/
I plan on resending these on top of your resend (if any) at a later convenient
time for both you and Junio, as noted in
https://public-inbox.org/git/cagz79kznvesvpicnu7lxkrchurqgvesfvg3dl5o_2kpvyrw...@mail.gmail.com/
>
> > > 3: 076e1192d ! 3: 4e3fb47a1 range-diff: first rudimentary
> > > implementation
> > > @@ -4,7 +4,7 @@
> > >
> > > At this stage, `git range-diff` can determine corresponding
> > > commits
> > > of two related commit ranges. This makes use of the recently
> > > introduced
> > > -implementation of the Hungarian algorithm.
> > > +implementation of the linear assignment algorithm.
> > >
> > > The core of this patch is a straight port of the ideas of
> > > tbdiff, the
> > > apparently dormant project at https://github.com/trast/tbdiff.
> > > @@ -51,19 +51,17 @@
> > > + int res = 0;
> > > + struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
> > >
> > > -- argc = parse_options(argc, argv, NULL, options,
> > > -- builtin_range_diff_usage, 0);
> > > -+ argc = parse_options(argc, argv, NULL, options,
> > > builtin_range_diff_usage,
> > > -+ 0);
> > > + argc = parse_options(argc, argv, NULL, options,
> > > + builtin_range_diff_usage, 0);
> >
> > This is really nice in colors when viewed locally.
> >
> > > 16: dfa7b1e71 < -: - range-diff --dual-color: work around
> > > bogus white-space warning
> > > -: - > 16: f4252f2b2 range-diff --dual-color: fix bogus
> > > white-space warning
> >
> > Ah; here my initial assumption of only reviewing the range-diff breaks down
> > now.
> > I'll dig into patch 16 separately.
>
> Right. This was an almost complete rewrite, and then next iteration will
> hopefully bring another complete rewrite: disabling whitespace warnings in
> dual color mode.
>
> > Maybe it is worth having an option to expand all "new" patches.
>
> Sure.
>
> And I also have a use case for --left-only/--right-only.
>
> And I also have a strong use case (and so does Junio, it seems, or for
> that matter, anybody contributing to Git due to Junio's insistence on
> signing off on each patch, rather than on the merge commit) for something
> like --ignore-lines=.
>
> And you probably guess what I will say next: these features will make for
> really fantastic patch series *on top* of mine. There really is no good
> reason to delay the current patch series just to cram more features into
> it that had not been planned in the first place.
Yes, I agree. I am unsure about the current state of your series, though;
Junio thinks (expects?) a resend, whereas you seem to call it good enough
but also said (some time back) that you want to resend due to Thomas
feedback.
I do have 2 series on top of the current range-diff.
* The first (queued by Junio as origin/sb/range-diff-colors)
adds a basic test for colors and improves diff.c readability
* The second (linked above) changes colors for some lines.
I do not want to build more on top as long as I do not know if
you resend (and how much it'll change)
Thanks,
Stefan
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Hi Stefan,
On Mon, 23 Jul 2018, Stefan Beller wrote:
> On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget
> wrote:
>
> > 1: 39272eefc ! 1: f7e70689e linear-assignment: a function to solve
> > least-cost assignment problems
> > @@ -223,9 +223,7 @@
> > + BUG("negative j: %d", j);
> > + i = pred[j];
> > + column2row[j] = i;
> > -+ k = j;
> > -+ j = row2column[i];
> > -+ row2column[i] = k;
> > ++ SWAP(j, row2column[i]);
>
> The dual color option (as a default) really helps here. Thanks for that!
> Does it have to be renamed though? (It's more than two colors; originally
> it was inverting the beginning signs)
>
> Maybe --color=emphasize-later
> assuming there will be other modes for coloring, such as "diff-only",
> which would correspond with --no-dual-color, or "none" that will correspond
> would be --no-color. I imagine there could be more fancy things, hence I would
> propose a mode rather than a switch.
> (Feel free to push back if discussing naming here feels like bike shedding)
I do feel free to push back on that.
> 2: 7f15b26d4ea ! 82: 88134121d2a Introduce `range-diff` to compare
> iterations of a topic branch
> [...]
> > diff --git a/Makefile b/Makefile
> > --- a/Makefile
> > +++ b/Makefile
>
> The line starting with --- is red (default removed color) and the line
> with +++ is green (default add color).
>
> Ideally these two lines and the third line above starting with "diff --git"
> would render in GIT_COLOR_BOLD ("METAINFO").
I agree that is not the best coloring here, but as you remarked elsewhere,
it would require content-aware dual coloring, and I am loathe to try to
implement that for two reasons: 1) it would take most likely a long time
to design and implement that, and 2) I don't have that time.
So I would like to declare that good enough is good enough in this case.
> > 3: 076e1192d ! 3: 4e3fb47a1 range-diff: first rudimentary
> > implementation
> > @@ -4,7 +4,7 @@
> >
> > At this stage, `git range-diff` can determine corresponding
> > commits
> > of two related commit ranges. This makes use of the recently
> > introduced
> > -implementation of the Hungarian algorithm.
> > +implementation of the linear assignment algorithm.
> >
> > The core of this patch is a straight port of the ideas of tbdiff,
> > the
> > apparently dormant project at https://github.com/trast/tbdiff.
> > @@ -51,19 +51,17 @@
> > + int res = 0;
> > + struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
> >
> > -- argc = parse_options(argc, argv, NULL, options,
> > -- builtin_range_diff_usage, 0);
> > -+ argc = parse_options(argc, argv, NULL, options,
> > builtin_range_diff_usage,
> > -+ 0);
> > + argc = parse_options(argc, argv, NULL, options,
> > + builtin_range_diff_usage, 0);
>
> This is really nice in colors when viewed locally.
>
> > 16: dfa7b1e71 < -: - range-diff --dual-color: work around bogus
> > white-space warning
> > -: - > 16: f4252f2b2 range-diff --dual-color: fix bogus
> > white-space warning
>
> Ah; here my initial assumption of only reviewing the range-diff breaks down
> now.
> I'll dig into patch 16 separately.
Right. This was an almost complete rewrite, and then next iteration will
hopefully bring another complete rewrite: disabling whitespace warnings in
dual color mode.
> Maybe it is worth having an option to expand all "new" patches.
Sure.
And I also have a use case for --left-only/--right-only.
And I also have a strong use case (and so does Junio, it seems, or for
that matter, anybody contributing to Git due to Junio's insistence on
signing off on each patch, rather than on the merge commit) for something
like --ignore-lines=.
And you probably guess what I will say next: these features will make for
really fantastic patch series *on top* of mine. There really is no good
reason to delay the current patch series just to cram more features into
it that had not been planned in the first place.
> (Given that the range-diff
> pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4 told me you used a
> different base, this is a hard problem, as I certainly would want to
> skip over all new base commits, but this one is interesting to look at.
> An easy way out: Maybe an option to expand any new commits/patches after
> the first expansion? Asking for opinions rather than implementing it)
>
> > 19: 144363006 < -: - range-diff: left-pad patch numbers
> > -: - > 19: 07ec215e8 range-diff: left-pad patch numbers
>
> > -: - > 21: d8498fb32 range-diff: use dim/bold cues to improve
> > dual color mode
>
> Those are interesting, I'll look
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
On 07/21, Johannes Schindelin via GitGitGadget wrote:
> The incredibly useful [`git-tbdiff`](https://github.com/trast/tbdiff) tool to
> compare patch series (say, to see what changed between two iterations sent to
> the Git mailing list) is slightly less useful for this developer due to the
> fact that it requires the `hungarian` and `numpy` Python packages which are
> for some reason really hard to build in MSYS2. So hard that I even had to
> give up, because it was simply easier to re-implement the whole shebang as a
> builtin command.
Thanks for your work making this a built-in! I finally found some
time to go through the series, and overall it makes sense to me. I
left a few comments here and there, and fwiw I didn't think there's
anything major that needs to be addressed.
Hope it helps!
> The project at https://github.com/trast/tbdiff seems to be dormant, anyway.
> Funny (and true) story: I looked at the open Pull Requests to see how active
> that project is, only to find to my surprise that I had submitted one in
> August 2015, and that it was still unanswered let alone merged.
>
> While at it, I forward-ported AEvar's patch to force `--decorate=no` because
> `git -p tbdiff` would fail otherwise.
>
> Side note: I work on implementing range-diff not only to make life easier for
> reviewers who have to suffer through v2, v3, ... of my patch series, but also
> to verify my changes before submitting a new iteration. And also, maybe even
> more importantly, I plan to use it to verify my merging-rebases of Git for
> Windows (for which I previously used to redirect the pre-rebase/post-rebase
> diffs vs upstream and then compare them using `git diff --no-index`). And of
> course any interested person can see what changes were necessary e.g. in the
> merging-rebase of Git for Windows onto v2.17.0 by running a command like:
>
> ```sh
> base=^{/Start.the.merging-rebase}
> tag=v2.17.0.windows.1
> pre=$tag$base^2
> git range-diff $pre$base..$pre $tag$base..$tag
> ```
>
> The command uses what it calls the "dual color mode" (can be disabled via
> `--no-dual-color`) which helps identifying what *actually* changed: it
> prefixes lines with a `-` (and red background) that correspond to the first
> commit range, and with a `+` (and green background) that correspond to the
> second range. The rest of the lines will be colored according to the original
> diffs.
>
> Changes since v3:
>
> - The cover letter was adjusted to reflect the new reality (the command is
> called `range-diff` now, not `branch-diff`, and `--dual-color` is the
> default).
> - The documentation was adjusted a bit more in the patch that makes
> `--dual-color` the default.
> - Clarified the calculation of the cost matrix, as per Stefan Beller's
> request.
> - The man page now spells out that merge *commits* are ignored in the commit
> ranges (not merges per se).
> - The code in `linear-assignment.c` was adjusted to use the `SWAP()` macro.
> - The commit message of the patch introducing the first rudimentary
> implementation no longer talks about the "Hungarian" algorithm, but about the
> "linear assignment algorithm" instead.
> - A bogus indentation change was backed out from the patch introducing the
> first rudimentary implementation.
> - Instead of merely warning about missing `..` in the 2-parameter invocation,
> we now exit with the error message.
> - The `diff_opt_parse()` function is allowed to return a value larger than 1,
> indicating that more than just one command-line parameter was parsed. We now
> advance by the indicated value instead of always advancing exactly 1 (which
> is still correct much of the time).
> - A lengthy `if...else if...else if...else` was simplified (from a logical
> point of view) by reordering it.
> - The unnecessarily `static` variable `dashes` was turned into a local
> variable of the caller.
> - The commit message talking about the new man page still referred to `git
> branch --diff`, which has been fixed.
> - A forgotten t7910 reference was changed to t3206.
> - An unbalanced double-tick was fixed in the man page.
> - Fixed grammar both of the commit message and the description of the
> `--no-dual-color` option.
> - To fix the build, a blank man page is now introduced together with the new
> `range-diff` command, even if it is populated for real only at a later patch
> (i.e. at the same time as before).
> - The headaches Junio fears would be incurred by that simple workaround to
> avoid bogus white-space error reporting are fended off: a more complex patch
> is now in place that adds (and uses) a new white-space flag. Sadly, as is all
> too common when Junio "encourages" me to replace a simple workaround by
> something "proper", it caused all kinds of headaches to get this right, so I
> am rather less certain that the "proper" fix will cause us less headaches
> than the simple workaround would have done. But whatever.
> - The
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Hi Stefan,
On Mon, 23 Jul 2018, Stefan Beller wrote:
> On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget
> wrote:
>
> > Range-diff vs v3:
> >
> > 1: 39272eefc ! 1: f7e70689e linear-assignment: a function to solve
> > least-cost assignment problems
> > @@ -223,9 +223,7 @@
> > + BUG("negative j: %d", j);
> > + i = pred[j];
> > + column2row[j] = i;
> > -+ k = j;
> > -+ j = row2column[i];
> > -+ row2column[i] = k;
> > ++ SWAP(j, row2column[i]);
>
> The dual color option (as a default) really helps here. Thanks for that!
> Does it have to be renamed though? (It's more than two colors; originally
> it was inverting the beginning signs)
I understand (and understood) the "dual" to mean that there are two
separate coloring methods, the coloring of the inner, and the coloring of
the outer diff. (And in my mind, the dimming is not so much an "inner"
diff things as an "outer" diff thing.)
> Maybe --color=emphasize-later assuming there will be other modes for
> coloring, such as "diff-only", which would correspond with
> --no-dual-color, or "none" that will correspond would be --no-color. I
> imagine there could be more fancy things, hence I would propose a mode
> rather than a switch. (Feel free to push back if discussing naming here
> feels like bike shedding)
Your suggestion does not really feel like bike-shedding to me, here, I can
see the merit of it.
It's just that 1) overloading `--color` here would be cumbersome, as
`--color` is *already* a diff option that we actually use, and 2) I am not
all that certain that new fancy things will crop up anytime soon. It was
hard enough to think of the dimming feature, and then implementing it.
So while I think your idea has merit, I still think that we can do that
later. The --no-dual-color option can easily be deprecated in favor of,
say, --color-mode=, when (and if) new modes crop up.
> 2: 7f15b26d4ea ! 82: 88134121d2a Introduce `range-diff` to compare
> iterations of a topic branch
> [...]
> > diff --git a/Makefile b/Makefile
> > --- a/Makefile
> > +++ b/Makefile
>
> The line starting with --- is red (default removed color) and the line
> with +++ is green (default add color).
>
> Ideally these two lines and the third line above starting with "diff --git"
> would render in GIT_COLOR_BOLD ("METAINFO").
I see where you are coming from, but given how complicated it seems to me
to implement this (dual color mode gets away with working line-based for
the moment, and what you ask for would require state, and would not even
be fool-proof, as the `diff --git` line might not even be part of the
context.
Seeing how long this patch series has already simmered, I would want to
invoke that old adage "the perfect is the enemy of the good", and rather
see a version of range-diff enter Git's source code, if need be marked as
"EXPERIMENTAL" so that the maintainer can claim that it is okay to be
buggy, and then invite contributions from other sides than from me.
> > 16: dfa7b1e71 < -: - range-diff --dual-color: work around bogus
> > white-space warning
> > -: - > 16: f4252f2b2 range-diff --dual-color: fix bogus
> > white-space warning
>
> Ah; here my initial assumption of only reviewing the range-diff breaks down
> now.
> I'll dig into patch 16 separately.
Right. In this case, it is a total rewrite, anyway (and I'll have to ask
you to overlook my frustration with how complex and hard it is to work on
ws.c without breaking anything). For the sake of review, you should ignore
the old patch. Unless you find that the new version is so complex and
prone to introduce bugs (with which I would agree) that we should go back
to the original workaround, which is so easy to understand that there are
no obvious bugs lurking inside it.
> Maybe it is worth having an option to expand all "new" patches.
Sure. And I would love to have this in a separate patch series, as it is
well beyond the original purpose of this patch series to simply make
tbdiff a builtin.
I should have known better, of course, but I was really not keen on
improving range-diff *before* it enters the code base, to the point of
introducing new features that might very well introduce new regressions in
unrelated commands, too.
Essentially, I am declaring a feature-freeze on this patch series until it
enters `next`.
> (Given that the range-diff
> pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4 told me you used a
> different base, this is a hard problem, as I certainly would want to
> skip over all new base commits, but this one is interesting to look at.
> An easy way out: Maybe an option to expand any new commits/patches after
> the first expansion? Asking for opinions rather than implementing it)
Any fulfilled wish is immediately welcomed with offspring, it seems.
Again,
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
On Mon, Jul 23, 2018 at 2:49 PM Junio C Hamano wrote: > > Stefan Beller writes: > > > On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget > > wrote: > > > >> Side note: I work on implementing range-diff not only to make life easier > >> for reviewers who have to suffer through v2, v3, ... of my patch series, > >> but also to verify my changes before submitting a new iteration. And also, > >> maybe even more importantly, I plan to use it to verify my merging-rebases > >> of Git for > >> Windows (for which I previously used to redirect the > >> pre-rebase/post-rebase diffs vs upstream and then compare them using `git > >> diff --no-index`). And of course any interested person can see what > >> changes were necessary e.g. in the merging-rebase of Git for Windows onto > >> v2.17.0 by running a command like: > > > > Thanks for making tools that makes the life of a Git developer easier! > > (Just filed https://github.com/gitgitgadget/gitgitgadget/issues/26 > > which asks to break lines for this cover letter) > > Thanks. These cover letters are unreadable without W Q > (gnus-article-fill-long-lines) While I had some comments on how I dislike some aspects of the implementation, I think it proves its usefulness by my usage, so I would suggest to merge it down to next as-is (and as soon as possible); deferring the issues in the implementation to later. I found running the range-diff on origin/pu to be a pleasant experience, although that still highlights the issues of in-exact coloring (the colors are chosen by the first two characters of the diff, which leads to mis-coloring of diff headers of the inner diff in the outer diff. But despite the imperfection, I strongly urge to consider the series as-is as good enough for inclusion. Thanks, Stefan Thanks, Stefan
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
Stefan Beller writes: > On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget > wrote: > >> Side note: I work on implementing range-diff not only to make life easier >> for reviewers who have to suffer through v2, v3, ... of my patch series, but >> also to verify my changes before submitting a new iteration. And also, maybe >> even more importantly, I plan to use it to verify my merging-rebases of Git >> for >> Windows (for which I previously used to redirect the pre-rebase/post-rebase >> diffs vs upstream and then compare them using `git diff --no-index`). And of >> course any interested person can see what changes were necessary e.g. in the >> merging-rebase of Git for Windows onto v2.17.0 by running a command like: > > Thanks for making tools that makes the life of a Git developer easier! > (Just filed https://github.com/gitgitgadget/gitgitgadget/issues/26 > which asks to break lines for this cover letter) Thanks. These cover letters are unreadable without W Q (gnus-article-fill-long-lines)
Re: [PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
On Sat, Jul 21, 2018 at 3:04 PM Johannes Schindelin via GitGitGadget
wrote:
> Side note: I work on implementing range-diff not only to make life easier for
> reviewers who have to suffer through v2, v3, ... of my patch series, but also
> to verify my changes before submitting a new iteration. And also, maybe even
> more importantly, I plan to use it to verify my merging-rebases of Git for
> Windows (for which I previously used to redirect the pre-rebase/post-rebase
> diffs vs upstream and then compare them using `git diff --no-index`). And of
> course any interested person can see what changes were necessary e.g. in the
> merging-rebase of Git for Windows onto v2.17.0 by running a command like:
Thanks for making tools that makes the life of a Git developer easier!
(Just filed https://github.com/gitgitgadget/gitgitgadget/issues/26
which asks to break lines for this cover letter)
> base-commit: b7bd9486b055c3f967a870311e704e3bb0654e4f
> Published-As:
> https://github.com/gitgitgadget/git/releases/tags/pr-1%2Fdscho%2Fbranch-diff-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
> pr-1/dscho/branch-diff-v4
> Pull-Request: https://github.com/gitgitgadget/git/pull/1
I just realized that if I had ideal memory of the previous review,
I could contain my whole review answer to this email only.
>
> Range-diff vs v3:
>
> 1: 39272eefc ! 1: f7e70689e linear-assignment: a function to solve
> least-cost assignment problems
> @@ -223,9 +223,7 @@
> + BUG("negative j: %d", j);
> + i = pred[j];
> + column2row[j] = i;
> -+ k = j;
> -+ j = row2column[i];
> -+ row2column[i] = k;
> ++ SWAP(j, row2column[i]);
The dual color option (as a default) really helps here. Thanks for that!
Does it have to be renamed though? (It's more than two colors; originally
it was inverting the beginning signs)
Maybe --color=emphasize-later
assuming there will be other modes for coloring, such as "diff-only",
which would correspond with --no-dual-color, or "none" that will correspond
would be --no-color. I imagine there could be more fancy things, hence I would
propose a mode rather than a switch.
(Feel free to push back if discussing naming here feels like bike shedding)
2: 7f15b26d4ea ! 82: 88134121d2a Introduce `range-diff` to compare
iterations of a topic branch
[...]
> diff --git a/Makefile b/Makefile
> --- a/Makefile
> +++ b/Makefile
The line starting with --- is red (default removed color) and the line
with +++ is green (default add color).
Ideally these two lines and the third line above starting with "diff --git"
would render in GIT_COLOR_BOLD ("METAINFO").
> 3: 076e1192d ! 3: 4e3fb47a1 range-diff: first rudimentary implementation
> @@ -4,7 +4,7 @@
>
> At this stage, `git range-diff` can determine corresponding commits
> of two related commit ranges. This makes use of the recently
> introduced
> -implementation of the Hungarian algorithm.
> +implementation of the linear assignment algorithm.
>
> The core of this patch is a straight port of the ideas of tbdiff,
> the
> apparently dormant project at https://github.com/trast/tbdiff.
> @@ -51,19 +51,17 @@
> + int res = 0;
> + struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
>
> -- argc = parse_options(argc, argv, NULL, options,
> -- builtin_range_diff_usage, 0);
> -+ argc = parse_options(argc, argv, NULL, options,
> builtin_range_diff_usage,
> -+ 0);
> + argc = parse_options(argc, argv, NULL, options,
> + builtin_range_diff_usage, 0);
This is really nice in colors when viewed locally.
> 16: dfa7b1e71 < -: - range-diff --dual-color: work around bogus
> white-space warning
> -: - > 16: f4252f2b2 range-diff --dual-color: fix bogus
> white-space warning
Ah; here my initial assumption of only reviewing the range-diff breaks down now.
I'll dig into patch 16 separately.
Maybe it is worth having an option to expand all "new" patches.
(Given that the range-diff pr-1/dscho/branch-diff-v3...pr-1/dscho/branch-diff-v4
told me you used a different base, this is a hard problem, as I certainly
would want to skip over all new base commits, but this one is interesting
to look at. An easy way out: Maybe an option to expand any new
commits/patches after the first expansion? Asking for opinions rather
than implementing it)
> 19: 144363006 < -: - range-diff: left-pad patch numbers
> -: - > 19: 07ec215e8 range-diff: left-pad patch numbers
> -: - > 21: d8498fb32 range-diff: use dim/bold cues to improve
> dual color mode
Those are interesting, I'll look at them separately, too.
Thanks,
Stefan
[PATCH v4 00/21] Add `range-diff`, a `tbdiff` lookalike
The incredibly useful [`git-tbdiff`](https://github.com/trast/tbdiff) tool to
compare patch series (say, to see what changed between two iterations sent to
the Git mailing list) is slightly less useful for this developer due to the
fact that it requires the `hungarian` and `numpy` Python packages which are for
some reason really hard to build in MSYS2. So hard that I even had to give up,
because it was simply easier to re-implement the whole shebang as a builtin
command.
The project at https://github.com/trast/tbdiff seems to be dormant, anyway.
Funny (and true) story: I looked at the open Pull Requests to see how active
that project is, only to find to my surprise that I had submitted one in August
2015, and that it was still unanswered let alone merged.
While at it, I forward-ported AEvar's patch to force `--decorate=no` because
`git -p tbdiff` would fail otherwise.
Side note: I work on implementing range-diff not only to make life easier for
reviewers who have to suffer through v2, v3, ... of my patch series, but also
to verify my changes before submitting a new iteration. And also, maybe even
more importantly, I plan to use it to verify my merging-rebases of Git for
Windows (for which I previously used to redirect the pre-rebase/post-rebase
diffs vs upstream and then compare them using `git diff --no-index`). And of
course any interested person can see what changes were necessary e.g. in the
merging-rebase of Git for Windows onto v2.17.0 by running a command like:
```sh
base=^{/Start.the.merging-rebase}
tag=v2.17.0.windows.1
pre=$tag$base^2
git range-diff $pre$base..$pre $tag$base..$tag
```
The command uses what it calls the "dual color mode" (can be disabled via
`--no-dual-color`) which helps identifying what *actually* changed: it prefixes
lines with a `-` (and red background) that correspond to the first commit
range, and with a `+` (and green background) that correspond to the second
range. The rest of the lines will be colored according to the original diffs.
Changes since v3:
- The cover letter was adjusted to reflect the new reality (the command is
called `range-diff` now, not `branch-diff`, and `--dual-color` is the default).
- The documentation was adjusted a bit more in the patch that makes
`--dual-color` the default.
- Clarified the calculation of the cost matrix, as per Stefan Beller's request.
- The man page now spells out that merge *commits* are ignored in the commit
ranges (not merges per se).
- The code in `linear-assignment.c` was adjusted to use the `SWAP()` macro.
- The commit message of the patch introducing the first rudimentary
implementation no longer talks about the "Hungarian" algorithm, but about the
"linear assignment algorithm" instead.
- A bogus indentation change was backed out from the patch introducing the
first rudimentary implementation.
- Instead of merely warning about missing `..` in the 2-parameter invocation,
we now exit with the error message.
- The `diff_opt_parse()` function is allowed to return a value larger than 1,
indicating that more than just one command-line parameter was parsed. We now
advance by the indicated value instead of always advancing exactly 1 (which is
still correct much of the time).
- A lengthy `if...else if...else if...else` was simplified (from a logical
point of view) by reordering it.
- The unnecessarily `static` variable `dashes` was turned into a local variable
of the caller.
- The commit message talking about the new man page still referred to `git
branch --diff`, which has been fixed.
- A forgotten t7910 reference was changed to t3206.
- An unbalanced double-tick was fixed in the man page.
- Fixed grammar both of the commit message and the description of the
`--no-dual-color` option.
- To fix the build, a blank man page is now introduced together with the new
`range-diff` command, even if it is populated for real only at a later patch
(i.e. at the same time as before).
- The headaches Junio fears would be incurred by that simple workaround to
avoid bogus white-space error reporting are fended off: a more complex patch is
now in place that adds (and uses) a new white-space flag. Sadly, as is all too
common when Junio "encourages" me to replace a simple workaround by something
"proper", it caused all kinds of headaches to get this right, so I am rather
less certain that the "proper" fix will cause us less headaches than the simple
workaround would have done. But whatever.
- The dual color mode now also dims the changes that are exclusively in the
first specified commit range, and uses bold face on the changes exclusively in
the second one. This matches the intuition when using `range-diff` to compare
an older iteration of a patch series to a newer one: the changes from the
previous iteration that were replaced by new ones "fade", while the changes
that replace them are "shiny new".
Changes since v2:
- Right-aligned the patch numbers in the commit
