Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Elijah Newren]

On Mon, May 7, 2018 at 10:50 AM, SZEDER Gábor  wrote:
>> 2) Your completion commands for branch-diff will only complete one
>> revision range, not two.  e.g.
>> git branch-diff origin/master..my-topic@{2} origin/master..my-top
>> won't complete "my-topic" as I'd expect.
>
> It does complete two revision ranges, but if you want to look at
> reflogs, then you must escape the opening curly brace.  I'm not sure
> why, but apparently after the unescaped '{' Bash thinks that it's a
> new command, and doesn't even call our completion functions anymore.
> It's not specific to the completion of 'branch-diff', or even to our
> completion script.  I don't think we can do anything about it.

Ah, indeed.  Thanks for the pointer.

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[SZEDER Gábor]

> 2) Your completion commands for branch-diff will only complete one
> revision range, not two.  e.g.
> git branch-diff origin/master..my-topic@{2} origin/master..my-top
> won't complete "my-topic" as I'd expect.

It does complete two revision ranges, but if you want to look at
reflogs, then you must escape the opening curly brace.  I'm not sure
why, but apparently after the unescaped '{' Bash thinks that it's a
new command, and doesn't even call our completion functions anymore.
It's not specific to the completion of 'branch-diff', or even to our
completion script.  I don't think we can do anything about it.

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Elijah Newren]

Hi Dscho,

On Sat, May 5, 2018 at 1:03 PM, Johannes Schindelin
 wrote:
> Hi Elijah,
>
> On Fri, 4 May 2018, Elijah Newren wrote:
>

>> - tbdiff aligned output columns better when there were more than 9
>> patches (I'll comment more on patch 09/18)
>
> I added a new patch to align the patch numbers specifically. I considered
> squashing it into 9/18, but decided against it: it will make it easier to
> read through the rationale when calling `git annotate` on those lines.

Awesome, thanks.


>> Also, I don't have bash-completion for either tbdiff or branch-diff.
>> :-(  But I saw some discussion on the v1 patches about how this gets
>> handled...  :-)
>
> Oh? Does 18/18 not work for you?
> https://public-inbox.org/git/71698f11835311c103aae565a2a761d10f4676b9.1525448066.git.johannes.schinde...@gmx.de/


It looks like it does work, in part, there were just two issues:

1) I apparently wasn't using all the nice improvements from the
completion script in my locally built git, but was instead still using
the one associated with my system-installed (and much older) git.
(Oops, my bad.)


2) Your completion commands for branch-diff will only complete one
revision range, not two.  e.g.
git branch-diff origin/master..my-topic@{2} origin/master..my-top
won't complete "my-topic" as I'd expect.


Elijah

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Johannes Schindelin]

Hi Brian,

On Sun, 6 May 2018, brian m. carlson wrote:

> On Fri, May 04, 2018 at 05:34:27PM +0200, Johannes Schindelin wrote:
> > The incredibly useful `git-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 reimplement the whole shebang
> > as a builtin command.
> 
> I just want to say thanks for writing this.  I use tbdiff extensively at
> work and having this built-in and much faster will really help.
> 
> I did a once-over of v1 and I'll probably take a look at v2 or v3
> (whatever's the latest) later in the week.

Thank you so much!
Dscho

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[brian m. carlson]

On Fri, May 04, 2018 at 05:34:27PM +0200, Johannes Schindelin wrote:
> The incredibly useful `git-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
> reimplement the whole shebang as a builtin command.

I just want to say thanks for writing this.  I use tbdiff extensively at
work and having this built-in and much faster will really help.

I did a once-over of v1 and I'll probably take a look at v2 or v3
(whatever's the latest) later in the week.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Johannes Schindelin]

Hi Junio,

On Sun, 6 May 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Johannes Schindelin (17):
> >   Add a function to solve least-cost assignment problems
> >   Add a new builtin: branch-diff
> 
> Perhaps retitling these to
> 
> hungarian: a function to solve least-cost assignment problems
> branch-diff: a new builtin to compare iterations of a topic
> 
> may serve as good precedents to changes other people may later make
> to these files.  Especially the second one is already consistent
> with the several changes that are listed below ;-)

I like it! They are retitled locally, in preparation for whenever I send
out the next iteration.

Ciao,
Dscho

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Junio C Hamano]

Johannes Schindelin  writes:

> Johannes Schindelin (17):
>   Add a function to solve least-cost assignment problems
>   Add a new builtin: branch-diff

Perhaps retitling these to

hungarian: a function to solve least-cost assignment problems
branch-diff: a new builtin to compare iterations of a topic

may serve as good precedents to changes other people may later make
to these files.  Especially the second one is already consistent
with the several changes that are listed below ;-)

>   branch-diff: first rudimentary implementation
>   branch-diff: improve the order of the shown commits
>   branch-diff: also show the diff between patches
>...

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Johannes Schindelin]

Hi Elijah,

On Fri, 4 May 2018, Elijah Newren wrote:

> On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
>  wrote:
> > The incredibly useful `git-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
> > reimplement the whole shebang as a builtin command.
> 
> tbdiff is awesome; thanks for bringing it in as a builtin to git.

You're welcome.

> I've run through a few cases, comparing output of tbdiff and
> branch-diff.  So far, what I've noted is that they produce largely the
> same output except that:
> 
> - tbdiff seems to shorten shas to 7 characters, branch-diff is using
> 10, in git.git at least.  (Probably a good change)

Yes, it is a good change ;-)

> - tbdiff aligned output columns better when there were more than 9
> patches (I'll comment more on patch 09/18)

I added a new patch to align the patch numbers specifically. I considered
squashing it into 9/18, but decided against it: it will make it easier to
read through the rationale when calling `git annotate` on those lines.

> - As noted elsewhere in the review of round 1, tbdiff uses difflib
> while branch-diff uses xdiff.  I found some cases where that mattered,
> and in all of them, I either felt like the difference was irrelevant
> or that difflib was suboptimal, so this is definitely an improvement
> for me.

Indeed. It is more or less ambiguities that get resolved differently.

> - branch-diff produces it's output faster, and it is automatically
> paged.  This is really cool.

:-)

It was actually the paging that made the most difference for me. The `git
tbdiff` command broke for me as soon as I switched on the pager, as tbdiff
got confused by the decoration (AEvar had put up a PR to fix that, but
that PR has not even so much as been answered in the meantime, so I
thought it'd be a good time to rewrite the entire shebang in C, also
because I could not use tbdiff *at all* on Windows due to its hefty
dependencies).

> Also, I don't have bash-completion for either tbdiff or branch-diff.
> :-(  But I saw some discussion on the v1 patches about how this gets
> handled...  :-)

Oh? Does 18/18 not work for you?
https://public-inbox.org/git/71698f11835311c103aae565a2a761d10f4676b9.1525448066.git.johannes.schinde...@gmx.de/

Ciao,
Dscho

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Elijah Newren]

Hi,

On Fri, May 4, 2018 at 9:21 AM, Elijah Newren  wrote:
> On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
>  wrote:
>> The incredibly useful `git-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
>> reimplement the whole shebang as a builtin command.
>
> tbdiff is awesome; thanks for bringing it in as a builtin to git.
>
> I've run through a few cases, comparing output of tbdiff and
> branch-diff.  So far, what I've noted is that they produce largely the
> same output except that:
>
> - tbdiff seems to shorten shas to 7 characters, branch-diff is using
> 10, in git.git at least.  (Probably a good change)

Sorry, a quick self-correction here:

tbdiff, when using an actual shortened sha, uses 10 characters.  But
when a patch doesn't have a match, tbdiff seems to use seven dashes on
one side in lieu of a shortened sha, whereas branch-diff will use 10
characters whether it has an actual shortened sha or is just putting a
bunch of dashes there.  So, this is definitely a good change.

> - tbdiff aligned output columns better when there were more than 9
> patches (I'll comment more on patch 09/18)
> - As noted elsewhere in the review of round 1, tbdiff uses difflib
> while branch-diff uses xdiff.  I found some cases where that mattered,
> and in all of them, I either felt like the difference was irrelevant
> or that difflib was suboptimal, so this is definitely an improvement
> for me.
> - branch-diff produces it's output faster, and it is automatically
> paged.  This is really cool.
>
> Also, I don't have bash-completion for either tbdiff or branch-diff.
> :-(  But I saw some discussion on the v1 patches about how this gets
> handled...  :-)

Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Elijah Newren]

On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
 wrote:
> The incredibly useful `git-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
> reimplement the whole shebang as a builtin command.

tbdiff is awesome; thanks for bringing it in as a builtin to git.

I've run through a few cases, comparing output of tbdiff and
branch-diff.  So far, what I've noted is that they produce largely the
same output except that:

- tbdiff seems to shorten shas to 7 characters, branch-diff is using
10, in git.git at least.  (Probably a good change)
- tbdiff aligned output columns better when there were more than 9
patches (I'll comment more on patch 09/18)
- As noted elsewhere in the review of round 1, tbdiff uses difflib
while branch-diff uses xdiff.  I found some cases where that mattered,
and in all of them, I either felt like the difference was irrelevant
or that difflib was suboptimal, so this is definitely an improvement
for me.
- branch-diff produces it's output faster, and it is automatically
paged.  This is really cool.

Also, I don't have bash-completion for either tbdiff or branch-diff.
:-(  But I saw some discussion on the v1 patches about how this gets
handled...  :-)


Elijah

[PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike

[Johannes Schindelin]

The incredibly useful `git-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
reimplement 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 branch-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 iteraion. 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 branch-diff --dual-color $pre$base..$pre $tag$base..$tag

The --dual-color mode will identify the many changes that are solely due to
different diff context lines (where otherwise uncolored lines start with a
background-colored -/+ marker), i.e. merge conflicts I had to resolve.

Changes since v1:

- Fixed the usage to *not* say "rebase--helper" (oops, now everybody knows that
  I copy-edited that code... ;-))

- Removed `branch-diff` from the `git help` output for now, by removing the
  `info` keyword from the respective line in command-list.txt.

- Removed the bogus `COLOR_DUAL_MODE` constant that was introduced in one
  patch, only to be removed in the next one.

- Fixed typo "emtpy".

- Fixed `make sparse` warnings.

- Changed the usage string to avoid the confusing lower-case bare `base`.

- Fixed tyop in commit message: "Pythong".

- Removed an awkward empty line before a `continue;` statement.

- Released the `line` strbuf after use.

- Fixed a typo and some "foreigner's English" in the commit message of
  "branch-diff: also show the diff between patches".

- Avoided introducing --no-patches too early and then replacing it by the
  version that uses the diff_options.

- Fixed the man page to continue with upper-case after a colon.

The branch-diff of v1 vs 2 is best viewed with --dual-color; This helps e.g.
with identifying changed *context* lines in the diff:

git branch-diff --dual-color origin/master branch-diff-v1 branch-diff-v2

(assuming that your `origin` points to git.git and that you fetched the tags
from https://github.com/dscho/git). For example, you will see that the only
change in patch #10 is a change in the diff context (due to the change of the
usage string in patch #2):

10:  9810869ced9 ! 10:  2695a6abc46 branch-diff: do not show "function names" 
in hunk headers
@@ -17,7 +17,7 @@
 +#include "userdiff.h"

  static const char * const builtin_branch_diff_usage[] = {
-   N_("git rebase--helper [] ( A..B C..D | A...B | base A B )"),
+ N_("git branch-diff [] .. 
.."),
 @@
return data;
  }


Johannes Schindelin (17):
  Add a function to solve least-cost assignment problems
  Add a new builtin: branch-diff
  branch-diff: first rudimentary implementation
  branch-diff: improve the order of the shown commits
  branch-diff: also show the diff between patches
  branch-diff: right-trim commit messages
  branch-diff: indent the diffs just like tbdiff
  branch-diff: suppress the diff headers
  branch-diff: adjust the output of the commit pairs
  branch-diff: do not show "function names" in hunk headers
  branch-diff: use color for the commit pairs
  color: provide inverted colors, too
  diff: add an internal option to dual-color diffs of diffs
  branch-diff: offer to dual-color the diffs
  branch-diff --dual-color: work around bogus white-space warning
  branch-diff: add a man page
  completion: support branch-diff

Thomas Rast (1):
  branch-diff: add tests

 .gitignore |   1 +
 Documentation/git-branch-diff.txt  | 239 ++
 Makefile   |   2 +
 builtin.h  |   1 +
 builtin/branch-diff.c  | 534 ++
 color.h|   6 +
 command-list.txt   |   1 +
 contrib/completion/git-completion.bash |  18 +
 diff.c |  76 +++-