Re: [PATCH v6 00/21] Add range-diff, a tbdiff lookalike
On 08/13, Johannes Schindelin wrote:
> Hi,
>
> On Mon, 13 Aug 2018, 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.
> >
> > 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:
> >
> > 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 v5:
>
> - Fixed the bug (introduced in v5) where a dashdash would not be handled
> appropriately.
Thanks! I've read through all the patches (and the range-diff :))
again and played around a bit with the newest version, and I think
this is ready for 'next'.
While playing around with it I did find one error message that reads
slightly odd, but it's still understandable, so I'm not sure it's
worth worrying about now (we can always improve it on top):
$ ./git range-diff -- js/range-diff-v4...HEADt
fatal: ambiguous argument 'HEADt..js/range-diff-v4': unknown revision or
path not in the working tree.
Use '--' to separate paths from revisions, like this:
'git [...] -- [...]'
error: could not parse log for 'HEADt..js/range-diff-v4'
> [...]
Re: [PATCH v6 00/21] Add range-diff, a tbdiff lookalike
Hi,
On Mon, 13 Aug 2018, 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.
>
> 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:
>
> 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 v5:
- Fixed the bug (introduced in v5) where a dashdash would not be handled
appropriately.
> Changes since v4:
>
> * Fixed a typo in the commit message of "range-diff: add tests" that was
>introduced in v4.
> * White-space fixes.
> * Fixed the length of the first header underline in the man page.
> * Changed the preprocessor guard in linear-assignment.h to reflect the new
>name (instead of the old name, which was hungarian.h).
> * Likewise, changed the preprocessor guards in range-diff.h to hide the
>history of the thrice-renamed command.
> * Fixed indentation in the completion.
> * Instead of trying to paper over white-space error handling that does not
>apply to "diffs of diffs", dual color mode now simply disables all
>white-space warnings.
> * When showing the "single arg must be symmetric range" error message, git
>range-diff now also shows the usage.
> * Adjusted the commit message of "range-diff: adjust the output of the
>commit pairs" to avoid the surprise of the reviewer when onelines are
>printed all of a sudden, too.
> * "range-diff: adjust the output of the commit pairs" is now using a
>simpler way to print onelines.
> * We are now sandwiching the diff_opt_parse() loop between two
>parse_options(), to make sure that we caught all options, and that the --
>separator is handled.
> * Adjusted the lookup_commit_reference() call to the newest master (it now
>takes a the_repository parameter).
>
> 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
[PATCH v6 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:
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 v4:
* Fixed a typo in the commit message of "range-diff: add tests" that was
introduced in v4.
* White-space fixes.
* Fixed the length of the first header underline in the man page.
* Changed the preprocessor guard in linear-assignment.h to reflect the new
name (instead of the old name, which was hungarian.h).
* Likewise, changed the preprocessor guards in range-diff.h to hide the
history of the thrice-renamed command.
* Fixed indentation in the completion.
* Instead of trying to paper over white-space error handling that does not
apply to "diffs of diffs", dual color mode now simply disables all
white-space warnings.
* When showing the "single arg must be symmetric range" error message, git
range-diff now also shows the usage.
* Adjusted the commit message of "range-diff: adjust the output of the
commit pairs" to avoid the surprise of the reviewer when onelines are
printed all of a sudden, too.
* "range-diff: adjust the output of the commit pairs" is now using a
simpler way to print onelines.
* We are now sandwiching the diff_opt_parse() loop between two
parse_options(), to make sure that we caught all options, and that the --
separator is handled.
* Adjusted the lookup_commit_reference() call to the newest master (it now
takes a the_repository parameter).
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
