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

[Johannes Schindelin]

Hi Junio,

On Tue, 22 May 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> >> In the picture, the three pre-context lines that are all indented by
> >> a HT are prefixed by a SP, and that is prefixed by a '+' sign of the
> >> outer diff.
> >
> > Yep, that's exactly it.
> >
> > The way tbdiff did it was to parse the diff and re-roll the coloring on
> > its own. I am not really keen on doing that in `branch --diff`, too.
> 
> Are you saying that these are "whitespace errors" getting painted?

Indentation errors, to be precise. Yes.

> It is somewhat odd because my whitespace errors are configured to be
> painted in "reverse blue".  Perhaps you are forcing the internal
> diff not to pay attention to the end-user configuration---which
> actually does make sense, as reusing of "git diff" to take "diff of
> diff" is a mere implementation detail.

It may have been the case before I introduced that call to
git_diff_ui_config(), but that happened after -v2, and I did not
contribute -v3 yet.

> In any case, the "whitespace errors" in "diff of diff" are mostly
> distracting.

Precisely. That's why I tried to suppress them in --dual-color mode.

I did not try to suppress them in --color (--no-dual-color) mode, as I
find that mode pretty useless.

> > I was wondering from the get-go whether it would make sense to make
> > --dual-color the default.
> >
> > But now I wonder whether there is actually *any* use in `--color` without
> > `--dual-color`.
> >
> > What do you think? Should I make the dual color mode the *only* color
> > mode?
> 
> Sorry but you are asking a good question to a wrong person.
> 
> I normally do not seek much useful information in colored output, so
> my reaction would not be very useful.  Non dual-color mode irritates
> me due to the false whitespace errors, and dual-color mode irritates
> me because it looks sufficiently different from tbdiff output that I
> am used to see.

Do you use --dual-color normally?

I derive *a ton* of information from the colored diff. It really helps me
navigate the output of range-diff very quickly.

I ask whether you use --dual-color because in that case I would consider
scrapping the way I handle color right now and try to imitate tbdiff's
way. But that would lose whitespace error coloring *altogether*. So I, for
one, would be unable to see where a subsequent patch series iteration
fixes whitespace errors of a previous iteration.

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin  writes:

>> In the picture, the three pre-context lines that are all indented by
>> a HT are prefixed by a SP, and that is prefixed by a '+' sign of the
>> outer diff.
>
> Yep, that's exactly it.
>
> The way tbdiff did it was to parse the diff and re-roll the coloring on
> its own. I am not really keen on doing that in `branch --diff`, too.

Are you saying that these are "whitespace errors" getting painted?
It is somewhat odd because my whitespace errors are configured to be
painted in "reverse blue".  Perhaps you are forcing the internal
diff not to pay attention to the end-user configuration---which
actually does make sense, as reusing of "git diff" to take "diff of
diff" is a mere implementation detail.

In any case, the "whitespace errors" in "diff of diff" are mostly
distracting.

> I was wondering from the get-go whether it would make sense to make
> --dual-color the default.
>
> But now I wonder whether there is actually *any* use in `--color` without
> `--dual-color`.
>
> What do you think? Should I make the dual color mode the *only* color
> mode?

Sorry but you are asking a good question to a wrong person.

I normally do not seek much useful information in colored output, so
my reaction would not be very useful.  Non dual-color mode irritates
me due to the false whitespace errors, and dual-color mode irritates
me because it looks sufficiently different from tbdiff output that I
am used to see.

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

[Johannes Schindelin]

Hi Junio,

On Mon, 21 May 2018, Junio C Hamano wrote:

> I've been using both branch-diff and tbdiff while comparing rerolled
> topics before accepting them.  One obvious difference between the two is
> that the speed to compute pairing is quite different but that is
> expected ;-)

Yep.

It is also expected that the branch --diff actually works without you
having to make sure that the Python modules numpy and hungarian are built
correctly (which you Linux folks couldn't care less about because it is
done for ya).

I spent such a lot on time trying to get it to work on Windows that it
probably only now reaches parity with the time I spent on implementing
branch --diff.

> Another difference that is somewhat irritating is that a SP that
> leads a context line in a diff hunk that is introduced in the newer
> iteration is often painted in reverse, and I do not know what aspect
> of that single SP on these lines the branch-diff wants to pull my
> attention to.

Right. This is due to the fact that the diff does not realize that it
looks at pre/post images being diffs. And hence it does not understand
that the single space indicates a context line and is therefore not a
whitespace error before the tab.

> https://pasteboard.co/Hm9ujI7F.png
> 
> In the picture, the three pre-context lines that are all indented by
> a HT are prefixed by a SP, and that is prefixed by a '+' sign of the
> outer diff.

Yep, that's exactly it.

The way tbdiff did it was to parse the diff and re-roll the coloring on
its own. I am not really keen on doing that in `branch --diff`, too.

> We can use --dual-color mode to unsee these but it is somewhat
> frustrating not to know what the branch-diff program wants to tell
> me by highlighting the single SPs on these lines.

I was wondering from the get-go whether it would make sense to make
--dual-color the default.

But now I wonder whether there is actually *any* use in `--color` without
`--dual-color`.

What do you think? Should I make the dual color mode the *only* color
mode?

Ciao,
Dscho

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

[Junio C Hamano]

I've been using both branch-diff and tbdiff while comparing rerolled
topics before accepting them.  One obvious difference between the
two is that the speed to compute pairing is quite different but that
is expected ;-)

Another difference that is somewhat irritating is that a SP that
leads a context line in a diff hunk that is introduced in the newer
iteration is often painted in reverse, and I do not know what aspect
of that single SP on these lines the branch-diff wants to pull my
attention to.

https://pasteboard.co/Hm9ujI7F.png

In the picture, the three pre-context lines that are all indented by
a HT are prefixed by a SP, and that is prefixed by a '+' sign of the
outer diff.

We can use --dual-color mode to unsee these but it is somewhat
frustrating not to know what the branch-diff program wants to tell
me by highlighting the single SPs on these lines.

Thanks.

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

[Johannes Schindelin]

Hi Junio,

On Fri, 4 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
> >   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
> 
> Lovely.  
> 
> I often have to criticize a series whose later half consists of many
> follow-up patches with "don't do 'oops, the previous was wrong'",
> but the follow-up patches in this series are not such corrections.
> The organization of the series to outline the basic and core idea
> first in the minimum form and then to build on it to improve an
> aspect of the command one step at a time is very helpful to guide
> the readers where the author of the series wants them to go.

Thanks! *beams*

Ciao,
Dscho

Re: [PATCH 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
>   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

Lovely.  

I often have to criticize a series whose later half consists of many
follow-up patches with "don't do 'oops, the previous was wrong'",
but the follow-up patches in this series are not such corrections.
The organization of the series to outline the basic and core idea
first in the minimum form and then to build on it to improve an
aspect of the command one step at a time is very helpful to guide
the readers where the author of the series wants them to go.

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

[Jacob Keller]

On Thu, May 3, 2018 at 11:05 AM, Ævar Arnfjörð Bjarmason
 wrote:
>
> On Thu, May 03 2018, 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.
>>
>> 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.
>
> I've been using branch-diff and haven't found issues with it yet, it
> works like tbdiff but better. Faster, uses the same diff as git
> (better), and spews to the pager by default.

I'm hoping to take a look at this as well, I remember looking into
tbdiff in the past, but also had trouble getting it to work. I've
tried a variety of similar things, including 4-way parent diffs, but
nothing quite gave the results I expected.

Thanks!

Regards,
Jake

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

[Johannes Schindelin]

Hi Ævar,

On Thu, 3 May 2018, Ævar Arnfjörð Bjarmason wrote:

> On Thu, May 03 2018, 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.
> >
> > 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.
> 
> I've been using branch-diff and haven't found issues with it yet, it
> works like tbdiff but better. Faster, uses the same diff as git
> (better), and spews to the pager by default.

Thanks for your enthusiasm!
Dscho

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

[Ævar Arnfjörð Bjarmason]

On Thu, May 03 2018, 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.
>
> 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.

I've been using branch-diff and haven't found issues with it yet, it
works like tbdiff but better. Faster, uses the same diff as git
(better), and spews to the pager by default.

[PATCH 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.


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  | 531 ++
 color.h|   6 +
 command-list.txt   |   1 +
 contrib/completion/git-completion.bash |  18 +
 diff.c |  76 +++-
 diff.h |   6 +-
 git.c  |   1 +
 hungarian.c| 205 +
 hungarian.h|  19 +
 t/.gitattributes   |   1 +
 t/t7910-branch-diff.sh | 144 ++
 t/t7910/history.export | 604 +
 16 files changed, 1843 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/git-branch-diff.txt
 create mode 100644 builtin/branch-diff.c
 create mode 100644 hungarian.c
 create mode 100644 hungarian.h
 create mode 100755 t/t7910-branch-diff.sh
 create mode 100644 t/t7910/history.export


base-commit: 1f1cddd558b54bb0ce19c8ace353fd07b758510d
Published-As: https://github.com/dscho/git/releases/tag/branch-diff-v1
Fetch-It-Via: git fetch https://github.com/dscho/git branch-diff-v1
-- 
2.17.0.395.g6a618d6010f.dirty