*[PATCH v3 17/20] range-diff: add a man page
2018-07-03 11:26 ` [PATCH v3 00/20] Add `range-diff`, " Johannes Schindelin via GitGitGadget
` (13 preceding siblings ...)
2018-05-03 1:11 ` [PATCH v3 16/20] range-diff --dual-color: work around bogus white-space warning Johannes Schindelin via GitGitGadget
@ 2018-05-03 13:50 ` Johannes Schindelin via GitGitGadget
2018-07-09 18:20 ` Stefan Beller
2018-07-16 8:01 ` Eric Sunshine
2018-05-03 14:44 ` [PATCH v3 18/20] completion: support `git range-diff` Johannes Schindelin via GitGitGadget
` (5 subsequent siblings)20 siblings, 2 replies; 387+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2018-05-03 13:50 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The bulk of this patch consists of a heavily butchered version of
tbdiff's README written by Thomas Rast and Thomas Gummerer, lifted from
https://github.com/trast/tbdiff.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-range-diff.txt | 235 +++++++++++++++++++++++++++++++
1 file changed, 235 insertions(+)
create mode 100644 Documentation/git-range-diff.txt
diff --git a/Documentation/git-range-diff.txt b/Documentation/git-range-diff.txt
new file mode 100644
index 000000000..189236cc6
--- /dev/null
+++ b/Documentation/git-range-diff.txt
@@ -0,0 +1,235 @@+git-range-diff(1)
+==================
+
+NAME
+----
+git-range-diff - Compare two commit ranges (e.g. two versions of a branch)
+
+SYNOPSIS
+--------
+[verse]
+'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).
+
+To that end, it first finds pairs of commits from both commit ranges
+that correspond with each other. Two commits are said to correspond when
+the diff between their patches (i.e. the author information, the commit
+message and the commit diff) is reasonably small compared to the
+patches' size. See ``Algorithm` below for details.
+
+Finally, the list of matching commits is shown in the order of the
+second commit range, with unmatched commits being inserted just after
+all of their ancestors have been shown.
+
+
+OPTIONS
+-------
+--dual-color::
+ When the commit diffs differ, recreate the original diffs'
+ coloring, and add outer -/+ diff markers with the *background*
+ being red/green to make it easier to see e.g. when there was a
+ change in what exact lines were added.
+
+--creation-factor=<percent>::
+ Set the creation/deletion cost fudge factor to `<percent>`.
+ Defaults to 60. Try a larger value if `git range-diff` erroneously
+ considers a large change a total rewrite (deletion of one commit
+ and addition of another), and a smaller one in the reverse case.
+ See the ``Algorithm`` section below for an explanation why this is
+ needed.
+
+<range1> <range2>::
+ Compare the commits specified by the two ranges, where
+ `<range1>` is considered an older version of `<range2>`.
+
+<rev1>...<rev2>::
+ Equivalent to passing `<rev2>..<rev1>` and `<rev1>..<rev2>`.
+
+<base> <rev1> <rev2>::
+ Equivalent to passing `<base>..<rev1>` and `<base>..<rev2>`.
+ Note that `<base>` does not need to be the exact branch point
+ of the branches. Example: after rebasing a branch `my-topic`,
+ `git range-diff my-topic@{u} my-topic@{1} my-topic` would
+ show the differences introduced by the rebase.
+
+`git range-diff` also accepts the regular diff options (see
+linkgit:git-diff[1]), most notably the `--color=[<when>]` and
+`--no-color` options. These options are used when generating the "diff
+between patches", i.e. to compare the author, commit message and diff of
+corresponding old/new commits. There is currently no means to tweak the
+diff options passed to `git log` when generating those patches.
+
+
+CONFIGURATION
+-------------
+This command uses the `diff.color.*` and `pager.range-diff` settings
+(the latter is on by default).
+See linkgit:git-config[1].
+
+
+EXAMPLES
+--------
+
+When a rebase required merge conflicts to be resolved, compare the changes
+introduced by the rebase directly afterwards using:
+
+------------
+$ git range-diff @{u} @{1} @
+------------
+
+
+A typical output of `git range-diff` would look like this:
+
+------------
+-: ------- > 1: 0ddba11 Prepare for the inevitable!
+1: c0debee = 2: cab005e Add a helpful message at the start
+2: f00dbal ! 3: decafe1 Describe a bug
+ @@ -1,3 +1,3 @@
+ Author: A U Thor <author@example.com>
+
+ -TODO: Describe a bug
+ +Describe a bug
+ @@ -324,5 +324,6
+ This is expected.
+
+ -+What is unexpected is that it will also crash.
+ ++Unexpectedly, it also crashes. This is a bug, and the jury is
+ ++still out there how to fix it best. See ticket #314 for details.
+
+ Contact
+3: bedead < -: ------- TO-UNDO
+------------
+
+In this example, there are 3 old and 3 new commits, where the developer
+removed the 3rd, added a new one before the first two, and modified the
+commit message of the 2nd commit as well its diff.
+
+When the output goes to a terminal, it is color-coded by default, just
+like regular `git diff`'s output. In addition, the first line (adding a
+commit) is green, the last line (deleting a commit) is red, the second
+line (with a perfect match) is yellow like the commit header of `git
+show`'s output, and the third line colors the old commit red, the new
+one green and the rest like `git show`'s commit header.
+
+The color-coded diff is actually a bit hard to read, though, as it
+colors the entire lines red or green. The line that added "What is
+unexpected" in the old commit, for example, is completely red, even if
+the intent of the old commit was to add something.
+
+To help with that, use the `--dual-color` mode. In this mode, the diff
+of diffs will retain the original diff colors, and prefix the lines with
+-/+ markers that have their *background* red or green, to make it more
+obvious that they describe how the diff itself changed.
+
+
+Algorithm
+---------
+
+The general idea is this: we generate a cost matrix between the commits
+in both commit ranges, then solve the least-cost assignment.
+
+To avoid false positives (e.g. when a patch has been removed, and an
+unrelated patch has been added between two iterations of the same patch
+series), the cost matrix is extended to allow for that, by adding
+fixed-cost entries for wholesale deletes/adds.
+
+Example: Let commits `1--2` be the first iteration of a patch series and
+`A--C` the second iteration. Let's assume that `A` is a cherry-pick of
+`2,` and `C` is a cherry-pick of `1` but with a small modification (say,
+a fixed typo). Visualize the commits as a bipartite graph:
+
+------------
+ 1 A
+
+ 2 B
+
+ C
+------------
+
+We are looking for a "best" explanation of the new series in terms of
+the old one. We can represent an "explanation" as an edge in the graph:
+
+
+------------
+ 1 A
+ /
+ 2 --------' B
+
+ C
+------------
+
+This explanation comes for "free" because there was no change. Similarly
+`C` could be explained using `1`, but that comes at some cost c>0
+because of the modification:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ |
+ `----- C
+ c>0
+------------
+
+In mathematical terms, what we are looking for is some sort of a minimum
+cost bipartite matching; `1` is matched to `C` at some cost, etc. The
+underlying graph is in fact a complete bipartite graph; the cost we
+associate with every edge is the size of the diff between the two
+commits' patches. To explain also new commits, we introduce dummy nodes
+on both sides:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ |
+ o `----- C
+ c>0
+ o o
+
+ o o
+------------
+
+The cost of an edge `o--C` is the size of `C`'s diff, modified by a
+fudge factor that should be smaller than 100%. The cost of an edge
+`o--o` is free. The fudge factor is necessary because even if `1` and
+`C` have nothing in common, they may still share a few empty lines and
+such, possibly making the assignment `1--C`, `o--o` slightly cheaper
+than `1--o`, `o--C` even if `1` and `C` have nothing in common. With the
+fudge factor we require a much larger common part to consider patches as
+corresponding.
+
+The overall time needed to compute this algorithm is the time needed to
+compute n+m commit diffs and then n*m diffs of patches, plus the time
+needed to compute the least-cost assigment between n and m diffs. Git
+uses an implementation of the Jonker-Volgenant algorithm to solve the
+assignment problem, which has cubic runtime complexity. The matching
+found in this case will look like this:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ .--+-----'
+ o -' `----- C
+ c>0
+ o ---------- o
+
+ o ---------- o
+------------
+
+
+SEE ALSO
+--------
+linkgit:git-log[1]
+
+GIT
+---
+Part of the linkgit:git[1] suite
--
gitgitgadget
^permalinkrawreply [flat|nested] 387+ messages in thread

*[PATCH 17/18] branch-diff: add a man page
2018-05-03 15:30 [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike Johannes Schindelin
` (15 preceding siblings ...)
2018-05-03 15:31 ` [PATCH 16/18] branch-diff --dual-color: work around bogus white-space warning Johannes Schindelin
@ 2018-05-03 15:31 ` Johannes Schindelin
2018-05-04 3:27 ` Eric Sunshine
2018-05-03 15:31 ` [PATCH 18/18] completion: support branch-diff Johannes Schindelin
` (4 subsequent siblings)21 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 15:31 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason
This is a heavily butchered version of the README written by Thomas
Rast and Thomas Gummerer, lifted from https://github.com/trast/tbdiff.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-branch-diff.txt | 239 ++++++++++++++++++++++++++++++
1 file changed, 239 insertions(+)
create mode 100644 Documentation/git-branch-diff.txt
diff --git a/Documentation/git-branch-diff.txt b/Documentation/git-branch-diff.txt
new file mode 100644
index 00000000000..b841586735c
--- /dev/null
+++ b/Documentation/git-branch-diff.txt
@@ -0,0 +1,239 @@+git-branch-diff(1)
+==================
+
+NAME
+----
+git-branch-diff - Compare two versions of a branch
+
+SYNOPSIS
+--------
+[verse]
+'git branch-diff' [--color=[<when>]] [--no-color] [<diff-options>]
+ [--dual-color] [--no-patches] [--creation-weight=<weight>]
+ ( <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).
+
+To that end, it first finds pairs of commits from both commit ranges
+that correspond with each other. Two commits are said to correspond when
+the diff between their patches (i.e. the author information, the commit
+message and the commit diff) is reasonably small compared to the
+patches' size. See ``Algorithm` below for details.
+
+Finally, the list of matching commits is shown in the order of the
+second commit range, with unmatched commits being inserted just after
+all of their ancestors have been shown.
+
+
+OPTIONS
+-------
+--no-patches::
+ Suppress the diffs between commit pairs that were deemed to
+ correspond; only show the pairings.
+
+--dual-color::
+ When the commit diffs differ, recreate the original diffs'
+ coloring, and add outer -/+ diff markers with the *background*
+ being red/green to make it easier to see e.g. when there was a
+ change in what exact lines were added.
+
+--creation-weight=<factor>::
+ Set the creation/deletion cost fudge factor to `<factor>`.
+ Defaults to 0.6. Try a larger value if `git branch-diff`
+ erroneously considers a large change a total rewrite (deletion
+ of one commit and addition of another), and a smaller one in
+ the reverse case. See the ``Algorithm`` section below for an
+ explanation why this is needed.
+
+<range1> <range2>::
+ Compare the commits specified by the two ranges, where
+ `<range1>` is considered an older version of `<range2>`.
+
+<rev1>...<rev2>::
+ Equivalent to passing `<rev2>..<rev1>` and `<rev1>..<rev2>`.
+
+<base> <rev1> <rev2>::
+ Equivalent to passing `<base>..<rev1>` and `<base>..<rev2>`.
+ Note that `<base>` does not need to be the exact branch point
+ of the branches. Example: after rebasing a branch `my-topic`,
+ `git branch-diff my-topic@{u} my-topic@{1} my-topic` would
+ show the differences introduced by the rebase.
+
+`git branch-diff` also accepts the regular diff options (see
+linkgit:git-diff[1]), most notably the `--color=[<when>]` and
+`--no-color` options. These options are used when generating the "diff
+between patches", i.e. to compare the author, commit message and diff of
+corresponding old/new commits. There is currently no means to tweak the
+diff options passed to `git log` when generating those patches.
+
+
+CONFIGURATION
+-------------
+This command uses the `diff.color.*` and `pager.branch-diff` settings
+(the latter is on by default).
+See linkgit:git-config[1].
+
+
+Examples
+--------
+
+When a rebase required merge conflicts to be resolved, compare the changes
+introduced by the rebase directly afterwards using:
+
+------------
+$ git branch-diff @{u} @{1} @
+------------
+
+
+A typical output of `git branch-diff` would look like this:
+
+------------
+-: ------- > 1: 0ddba11 Prepare for the inevitable!
+1: c0debee = 2: cab005e Add a helpful message at the start
+2: f00dbal ! 3: decafe1 Describe a bug
+ @@ -1,3 +1,3 @@
+ Author: A U Thor <author@example.com>
+
+ -TODO: Describe a bug
+ +Describe a bug
+ @@ -324,5 +324,6
+ This is expected.
+
+ -+What is unexpected is that it will also crash.
+ ++Unexpectedly, it also crashes. This is a bug, and the jury is
+ ++still out there how to fix it best. See ticket #314 for details.
+
+ Contact
+3: bedead < -: ------- TO-UNDO
+------------
+
+In this example, there are 3 old and 3 new commits, where the developer
+removed the 3rd, added a new one before the first two, and modified the
+commit message of the 2nd commit as well its diff.
+
+When the output goes to a terminal, it is color-coded by default, just
+like regular `git diff`'s output. In addition, the first line (adding a
+commit) is green, the last line (deleting a commit) is red, the second
+line (with a perfect match) is yellow like the commit header of `git
+show`'s output, and the third line colors the old commit red, the new
+one green and the rest like `git show`'s commit header.
+
+The color-coded diff is actually a bit hard to read, though, as it
+colors the entire lines red or green. The line that added "What is
+unexpected" in the old commit, for example, is completely red, even if
+the intent of the old commit was to add something.
+
+To help with that, use the `--dual-color` mode. In this mode, the diff
+of diffs will retain the original diff colors, and prefix the lines with
+-/+ markers that have their *background* red or green, to make it more
+obvious that they describe how the diff itself changed.
+
+
+Algorithm
+---------
+
+The general idea is this: we generate a cost matrix between the commits
+in both commit ranges, then solve the least-cost assignment.
+
+To avoid false positives (e.g. when a patch has been removed, and an
+unrelated patch has been added between two iterations of the same patch
+series), the cost matrix is extended to allow for that, by adding
+fixed-cost entries for wholesale deletes/adds.
+
+Example: let commits `1--2` be the first iteration of a patch series and
+`A--C` the second iteration. Let's assume that `A` is a cherry-pick of
+`2,` and `C` is a cherry-pick of `1` but with a small modification (say,
+a fixed typo). Visualize the commits as a bipartite graph:
+
+------------
+ 1 A
+
+ 2 B
+
+ C
+------------
+
+We are looking for a "best" explanation of the new series in terms of
+the old one. We can represent an "explanation" as an edge in the graph:
+
+
+------------
+ 1 A
+ /
+ 2 --------' B
+
+ C
+------------
+
+This explanation comes for "free" because there was no change. Similarly
+`C` could be explained using `1`, but that comes at some cost c>0
+because of the modification:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ |
+ `----- C
+ c>0
+------------
+
+In mathematical terms, what we are looking for is some sort of a minimum
+cost bipartite matching; `1` is matched to `C` at some cost, etc. The
+underlying graph is in fact a complete bipartite graph; the cost we
+associate with every edge is the size of the diff between the two
+commits' patches. To explain also new commits, we introduce dummy nodes
+on both sides:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ |
+ o `----- C
+ c>0
+ o o
+
+ o o
+------------
+
+The cost of an edge `o--C` is the size of `C`'s diff, modified by a
+fudge factor that should be smaller than 1.0. The cost of an edge `o--o`
+is free. The fudge factor is necessary because even if `1` and `C` have
+nothing in common, they may still share a few empty lines and such,
+possibly making the assignment `1--C`, `o--o` slightly cheaper than
+`1--o`, `o--C` even if `1` and `C` have nothing in common. With the
+fudge factor we require a much larger common part to consider patches as
+corresponding.
+
+The overall time needed to compute this algorithm is the time needed to
+compute n+m commit diffs and then n*m diffs of patches, plus the time
+needed to compute the least-cost assigment between n and m diffs. Git
+uses an implementation of the Jonker-Volgenant algorithm to solve the
+assignment problem, which has cubic runtime complexity. The matching
+found in this case will look like this:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ .--+-----'
+ o -' `----- C
+ c>0
+ o ---------- o
+
+ o ---------- o
+------------
+
+
+SEE ALSO
+--------
+linkgit:git-log[1]
+
+GIT
+---
+Part of the linkgit:git[1] suite
--
2.17.0.395.g6a618d6010f.dirty
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-03 15:30 ` [PATCH 02/18] Add a new builtin: branch-diff Johannes Schindelin
2018-05-03 16:10 ` Ramsay Jones
2018-05-03 16:41 ` Duy Nguyen@ 2018-05-03 16:43 ` Stefan Beller
2018-05-03 20:42 ` Johannes Schindelin
2018-05-04 2:35 ` Eric Sunshine3 siblings, 1 reply; 387+ messages in thread
From: Stefan Beller @ 2018-05-03 16:43 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Johannes,
On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> This builtin does not do a whole lot so far, apart from showing a usage
> that is oddly similar to that of `git tbdiff`. And for a good reason:
> the next commits will turn `branch-diff` into a full-blown replacement
> for `tbdiff`.
While I appreciate the 1:1 re-implementation, I'll comment as if this
was a newly invented tool, questioning design choices. They are probably
chosen pretty well, and fudge facotrs as below are at tweaked to a reasonable
factor, but I'll try to look with fresh eyes.
>
> At this point, we ignore tbdiff's color options, as they will all be
> implemented later and require some patches to the diff machinery.
Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
cyan for "uninteresting" parts to deliver a consistent color scheme for
Git. Eventually he dreams of having 2 layers of indirection IIUC, with
"uninteresting" -> cyan
"repeated lines in blame" -> uninteresting
Maybe we can fit the coloring of this tool in this scheme, too?
> + double creation_weight = 0.6;
I wondered if we use doubles in Gits code base at all,
and I found
khash.h:59:static const double __ac_HASH_UPPER = 0.77;
pack-bitmap-write.c:248: static const double
REUSE_BITMAP_THRESHOLD = 0.2;
pack-bitmap.c:751: static const double REUSE_PERCENT = 0.9;
all other occurrences of `git grep double` are mentioning it in other
contexts (e.g. "double linked list" or comments).
When implementing diff heuristics in 433860f3d0b (diff: improve
positioning of add/delete blocks in diffs, 2016-09-05), Michael broke
it down to fixed integers instead of floating point.
Do we need to dynamic of a floating point, or would a rather small range
suffice here? (Also see rename detection settings, that take percents as
integers)
Thanks,
Stefan
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-03 16:43 ` Stefan Beller@ 2018-05-03 20:42 ` Johannes Schindelin
2018-05-03 21:12 ` Stefan Beller0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 20:42 UTC (permalink / raw)
To: Stefan Beller
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Stefan,
On Thu, 3 May 2018, Stefan Beller wrote:
> On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > This builtin does not do a whole lot so far, apart from showing a
> > usage that is oddly similar to that of `git tbdiff`. And for a good
> > reason: the next commits will turn `branch-diff` into a full-blown
> > replacement for `tbdiff`.
>
> While I appreciate the 1:1 re-implementation, I'll comment as if this
> was a newly invented tool, questioning design choices. They are probably
> chosen pretty well, and fudge facotrs as below are at tweaked to a
> reasonable factor, but I'll try to look with fresh eyes.
Absolutely. While tbdiff got some testing over time, it has definitely not
gotten as much exposure as branch-diff hopefully will.
BTW I chose a different command name on purpose, so that we are free to
change the design and not harm existing tbdiff users.
> > At this point, we ignore tbdiff's color options, as they will all be
> > implemented later and require some patches to the diff machinery.
>
> Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
> cyan for "uninteresting" parts to deliver a consistent color scheme for
> Git. Eventually he dreams of having 2 layers of indirection IIUC, with
> "uninteresting" -> cyan
> "repeated lines in blame" -> uninteresting
>
> Maybe we can fit the coloring of this tool in this scheme, too?
Sure. So you mean I should use cyan for... what part of the colored
output? ;-)
> > + double creation_weight = 0.6;
>
> I wondered if we use doubles in Gits code base at all,
> and I found
>
> khash.h:59:static const double __ac_HASH_UPPER = 0.77;
> pack-bitmap-write.c:248: static const double
> REUSE_BITMAP_THRESHOLD = 0.2;
> pack-bitmap.c:751: static const double REUSE_PERCENT = 0.9;
>
> all other occurrences of `git grep double` are mentioning it in other
> contexts (e.g. "double linked list" or comments).
>
> When implementing diff heuristics in 433860f3d0b (diff: improve
> positioning of add/delete blocks in diffs, 2016-09-05), Michael broke
> it down to fixed integers instead of floating point.
>
> Do we need to dynamic of a floating point, or would a rather small range
> suffice here? (Also see rename detection settings, that take percents as
> integers)
I guess you are right, and we do not need floats. It was just very, very
convenient to do that instead of using integers because
- I already had the Jonker-Volgenant implementation "lying around" from my
previous life as an image processing expert, using doubles (but it was
in Java, not in C, so I quickly converted it for branch-diff).
- I was actually not paying attention whether divisions are a thing in the
algorithm. From a cursory glance, it would appear that we are never
dividing in hungarian.c, so theoretically integers should be fine.
- using doubles neatly side-steps the overflow problem. If I use integers
instead, I always will have to worry what to do if, say, adding
`INT_MAX` to `INT_MAX`.
I am particularly worried about that last thing: it could easily lead to
incorrect results if we blindly, say, pretend that `INT_MAX + INT_MAX ==
INT_MAX` for the purpose of avoiding overflows.
If, however, I misunderstood and you are only concerned about using
*double-precision* floating point numbers, and would suggest using `float`
typed variables instead, that would be totally cool with me.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 03/18] branch-diff: first rudimentary implementation
2018-05-03 17:06 ` Stefan Beller@ 2018-05-03 21:01 ` Johannes Schindelin
2018-05-03 21:19 ` Stefan Beller0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 21:01 UTC (permalink / raw)
To: Stefan Beller
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Stefan,
On Thu, 3 May 2018, Stefan Beller wrote:
> On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>
> > Note: due to differences in the diff algorithm (`tbdiff` uses the
> > Pythong module `difflib`, Git uses its xdiff fork), the cost matrix
> > calculated by `branch-diff` is different (but very similar) to the one
> > calculated by `tbdiff`. Therefore, it is possible that they find
> > different matching commits in corner cases (e.g. when a patch was
> > split into two patches of roughly equal length).
>
> Does that mean, we may want to tweak the underlying diff parameters for
> this special use case eventually?
I don't think that will be necessary. Generating diffs is an ambiguous
business, after all, and we just have to live with the consequence that it
might even be possible that the cost is non-symmetric, i.e. that the
length (i.e. line count) of the diff is different depending whether we
compare old patch to new patch vs new patch to old patch.
If the result changes due to these vagaries, it means that there is no
single one good answer to the question which old/new commits form a pair.
I would expect that only to happen if a commit with a lengthy diff is cut
into two commits whose diffs have roughly equal lengths (so that the
difference of the commit message won't matter that much).
> > -#define COLOR_DUAL_MODE 2
> > -
>
> Leave this out in the first patch?
Yep, as Ramsay said.
> > @@ -19,6 +23,279 @@ static int parse_creation_weight(const struct option *opt, const char *arg,
> > return 0;
> > }
> >
> > +struct patch_util {
> > + /* For the search for an exact match */
> > + struct hashmap_entry e;
> > + const char *diff, *patch;
> > +
> > + int i;
> > + int diffsize;
> > + size_t diff_offset;
> > + /* the index of the matching item in the other branch, or -1 */
> > + int matching;
> > + struct object_id oid;
> > +};
> > +
> > +/*
> > + * Reads the patches into a string list, with the `util` field being populated
> > + * as struct object_id (will need to be free()d).
> > + */
> > +static int read_patches(const char *range, struct string_list *list)
> > +{
> > + struct child_process cp = CHILD_PROCESS_INIT;
> > + FILE *in;
> > + struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
> > + struct patch_util *util = NULL;
> > + int in_header = 1;
> > +
> > + argv_array_pushl(&cp.args, "log", "--no-color", "-p", "--no-merges",
> > + "--reverse", "--date-order", "--decorate=no",
> > + "--no-abbrev-commit", range,
> > + NULL);
> > + cp.out = -1;
> > + cp.no_stdin = 1;
> > + cp.git_cmd = 1;
> > +
> > + if (start_command(&cp))
> > + return error_errno(_("could not start `log`"));
> > + in = fdopen(cp.out, "r");
> > + if (!in) {
> > + error_errno(_("could not read `log` output"));
> > + finish_command(&cp);
> > + return -1;
> > + }
>
> With the implementation of --color-moved, there is an option
> to buffer all diff output in memory e6e045f8031 (diff.c: buffer
> all output if asked to, 2017-06-29), so I posit that running this
> diff in-core may be "not too hard". Famous last words.
True. I *did* stumble over emitted_symbols and thought that there might be
a way to leverage it, but I did not find any existing knob, and did not
want to interfere with any other user case of that feature (which we may
want to allow combining with branch-diff one day, afer all).
To be honest, the main reason I spawn here is that I did not want to be
bothered with resetting the commit flags after traversing the first commit
range. But I guess I was just too cheap and should really do it?
OTOH spawning here is a lot easier than not spawning, so maybe it would be
premature optimization?
> In addition to that patch, we'd have to buffer commit messages
> and buffer multiple commits, as that only buffers a diff of a single
> commit.
... and make sure that the moved-code logic (which is currently the only
user of emitted_symbols, correct?) would never be called at the same time
as we generate the diff.
> The benefit would be no invocation of new processes, letting us
> do more in core. This would allow for tweaking revision walking
> internally, e.g. passing of options to this command such as rename
> detection factors, can be passed through easily without the need
> of translating it back to the command line.
On the other hand, we can simply copy those options to the command-line
for `log`. Which might even be better, as e.g. `--format` changes global
state :-(
> > +
> > + if (starts_with(line.buf, "diff --git")) {
>
> When using the internal buffers, you would not need to
> string compare, but could just check for the
> DIFF_SYMBOL_HEADER.
True. Not sure whether the current way is *that* terrible, though, as the
`diff` line is meant for parsing by other commands, anyway.
> > + } else if (starts_with(line.buf, "@@ "))
> > + strbuf_addstr(&buf, "@@");
>
> So we omit line information for hunks. Makes sense,
> though then we could also skip the "index ..." lines?
And we do, in the next line:
> > + else if (line.buf[0] && !starts_with(line.buf, "index "))
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 11/18] branch-diff: add tests
2018-05-03 17:11 ` Stefan Beller@ 2018-05-03 21:05 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 21:05 UTC (permalink / raw)
To: Stefan Beller
Cc: git, Thomas Rast, Junio C Hamano, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Stefan,
On Thu, 3 May 2018, Stefan Beller wrote:
> On Thu, May 3, 2018 at 8:30 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
> > From: Thomas Rast <tr@thomasrast.ch>
> >
> > These are essentially lifted from https://github.com/trast/tbdiff, with
> > light touch-ups to account for the new command name.
> >
> > Apart from renaming `tbdiff` to `branch-diff`, only one test case needed
> > to be adjusted: 11 - 'changed message'.
> >
> > The underlying reason it had to be adjusted is that diff generation is
> > sometimes ambiguous. In this case, a comment line and an empty line are
> > added, but it is ambiguous whether they were added after the existing
> > empty line, or whether an empty line and the comment line are added
> > *before* the existing emtpy line. And apparently xdiff picks a different
> > option here than Python's difflib.
>
> I think that is the fallout of the diff heuristics. If you are keen on
> a 1:1 port, you can disable the diff sliding heuristics and it should
> produce the same diff with trailing new lines.
I am not keen on a 1:1 port. I am fine with having xdiff generate
different diffs than Python's difflib. That's par for the course when
relying on a not-quite-well-defined metric.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-03 18:05 ` [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike Ævar Arnfjörð Bjarmason
@ 2018-05-03 21:07 ` Johannes Schindelin
2018-05-03 21:50 ` Jacob Keller1 sibling, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 21:07 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer
[-- Attachment #1: Type: text/plain, Size: 1148 bytes --]
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
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-03 20:42 ` Johannes Schindelin@ 2018-05-03 21:12 ` Stefan Beller
2018-05-03 21:49 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Stefan Beller @ 2018-05-03 21:12 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On Thu, May 3, 2018 at 1:42 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
>> cyan for "uninteresting" parts to deliver a consistent color scheme for
>> Git. Eventually he dreams of having 2 layers of indirection IIUC, with
>> "uninteresting" -> cyan
>> "repeated lines in blame" -> uninteresting
>>
>> Maybe we can fit the coloring of this tool in this scheme, too?
>
> Sure. So you mean I should use cyan for... what part of the colored
> output? ;-)
>
It is just a FYI heads up, not an actionable bikeshed painting plan. ;)
>> Do we need to dynamic of a floating point, or would a rather small range
>> suffice here? (Also see rename detection settings, that take percents as
>> integers)
>
> I guess you are right, and we do not need floats. It was just very, very
> convenient to do that instead of using integers because
>
> - I already had the Jonker-Volgenant implementation "lying around" from my
> previous life as an image processing expert, using doubles (but it was
> in Java, not in C, so I quickly converted it for branch-diff).
>
> - I was actually not paying attention whether divisions are a thing in the
> algorithm. From a cursory glance, it would appear that we are never
> dividing in hungarian.c, so theoretically integers should be fine.
>
> - using doubles neatly side-steps the overflow problem. If I use integers
> instead, I always will have to worry what to do if, say, adding
> `INT_MAX` to `INT_MAX`.
>
> I am particularly worried about that last thing: it could easily lead to
> incorrect results if we blindly, say, pretend that `INT_MAX + INT_MAX ==
> INT_MAX` for the purpose of avoiding overflows.
>
> If, however, I misunderstood and you are only concerned about using
> *double-precision* floating point numbers, and would suggest using `float`
> typed variables instead, that would be totally cool with me.
So by being worried about INT_MAX occurring, you are implying that
we have to worry about a large range of values, so maybe floating points
are the best choice here.
Looking through that algorithm the costs seem to be integers only
measuring number of lines, so I would not be too worried about running
into INT_MAX problems except for the costs that are assigned INT_MAX
explicitly.
I was more asking, if floating point is the right tool for the job.
Stefan
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 03/18] branch-diff: first rudimentary implementation
2018-05-03 21:01 ` Johannes Schindelin@ 2018-05-03 21:19 ` Stefan Beller
2018-05-03 22:00 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Stefan Beller @ 2018-05-03 21:19 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
> To be honest, the main reason I spawn here is that I did not want to be
> bothered with resetting the commit flags after traversing the first commit
> range. But I guess I was just too cheap and should really do it?
Oh right, you'd have to do multiple revision walks.
> OTOH spawning here is a lot easier than not spawning, so maybe it would be
> premature optimization?
Most likely.
>
>> In addition to that patch, we'd have to buffer commit messages
>> and buffer multiple commits, as that only buffers a diff of a single
>> commit.
>
> ... and make sure that the moved-code logic (which is currently the only
> user of emitted_symbols, correct?) would never be called at the same time
> as we generate the diff.
The moved detection is all part of the flags of an emitted symbol.
By design the emitted symbol has easy access to the raw line of the output,
which made it easy for the move detection to work on the lines. AFAICT this
is also desired here as lines are put into a hashmap for comparisons.
(and having it colored differently would make finding the same line
complex using hashmaps)
I just entertain the thought of having move detection active in a
branch-diff. That would be really cool actually.
>
>> The benefit would be no invocation of new processes, letting us
>> do more in core. This would allow for tweaking revision walking
>> internally, e.g. passing of options to this command such as rename
>> detection factors, can be passed through easily without the need
>> of translating it back to the command line.
>
> On the other hand, we can simply copy those options to the command-line
> for `log`. Which might even be better, as e.g. `--format` changes global
> state :-(
ok.
Thanks for your patience,
Stefan
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-03 21:12 ` Stefan Beller@ 2018-05-03 21:49 ` Johannes Schindelin
2018-05-04 3:23 ` Junio C Hamano0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 21:49 UTC (permalink / raw)
To: Stefan Beller
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Stefan,
On Thu, 3 May 2018, Stefan Beller wrote:
> On Thu, May 3, 2018 at 1:42 PM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> >> Speaking of colors, for origin/sb/blame-color Junio hinted at re-using
> >> cyan for "uninteresting" parts to deliver a consistent color scheme for
> >> Git. Eventually he dreams of having 2 layers of indirection IIUC, with
> >> "uninteresting" -> cyan
> >> "repeated lines in blame" -> uninteresting
> >>
> >> Maybe we can fit the coloring of this tool in this scheme, too?
> >
> > Sure. So you mean I should use cyan for... what part of the colored
> > output? ;-)
>
> It is just a FYI heads up, not an actionable bikeshed painting plan. ;)
Oh, I did not understand it as bike-shedding at all. I had hoped that you
ran `git branch-diff --dual-color` on something interesting and found e.g.
the yellow color inherited from DIFF_COMMIT to be the wrong color for
unchanged commit pairs.
So please: as soon as you have a concrete suggestion where to use cyan
(and preferably even a DIFF_* constant to feed to diff_get_color_opt()), I
will be more than interested.
> >> Do we need to dynamic of a floating point, or would a rather small range
> >> suffice here? (Also see rename detection settings, that take percents as
> >> integers)
> >
> > I guess you are right, and we do not need floats. It was just very, very
> > convenient to do that instead of using integers because
> >
> > - I already had the Jonker-Volgenant implementation "lying around" from my
> > previous life as an image processing expert, using doubles (but it was
> > in Java, not in C, so I quickly converted it for branch-diff).
> >
> > - I was actually not paying attention whether divisions are a thing in the
> > algorithm. From a cursory glance, it would appear that we are never
> > dividing in hungarian.c, so theoretically integers should be fine.
> >
> > - using doubles neatly side-steps the overflow problem. If I use integers
> > instead, I always will have to worry what to do if, say, adding
> > `INT_MAX` to `INT_MAX`.
> >
> > I am particularly worried about that last thing: it could easily lead to
> > incorrect results if we blindly, say, pretend that `INT_MAX + INT_MAX ==
> > INT_MAX` for the purpose of avoiding overflows.
> >
> > If, however, I misunderstood and you are only concerned about using
> > *double-precision* floating point numbers, and would suggest using `float`
> > typed variables instead, that would be totally cool with me.
>
> So by being worried about INT_MAX occurring, you are implying that
> we have to worry about a large range of values, so maybe floating points
> are the best choice here.
I am not really worried about a large range of values, I am worried about
a use case where we use the maximal value as an "impossible, must avoid at
all cost" value. See this line in hungarian.c:
u2 = DBL_MAX;
It does not seem as if any arithmetic is done on u2 after that
(theoretically, it should not survive the loop that comes after it and
tries to replace u2 with any smaller value it finds, but what if that loop
does not even run because column_count == 1? Sure, it is a pathological
case, but even those should have correct results).
But actually, yes, there *is* arithmetic performed on u2:
if (u1 < u2)
v[j1] -= u2 - u1;
So in the pathological case where we try to find the best assignment of a
single column to an arbitrary number of rows (where simply the row with
a smallest cost should be picked), we try to subtract from v[j1] an
insanely large value. As a consequence, v[j1] will be close to INT_MIN if
we were to switch to integers, and who is to say that the next time we get
to this part, j1 will be different? If it is the same, and we hit the same
u2, then we might end up subtracting something close to INT_MAX from
INT_MIN, which will definitely overflow and the computation will be
incorrect.
*That* is what I am worried about: overflowing integer arithmetic. IIRC if
you subtract DBL_MAX from -DBL_MAX, you still end up with -DBL_MAX. So in
that respect, using floating point numbers here is safer.
> Looking through that algorithm the costs seem to be integers only
> measuring number of lines, so I would not be too worried about running
> into INT_MAX problems except for the costs that are assigned INT_MAX
> explicitly.
>
> I was more asking, if floating point is the right tool for the job.
I think I would have to spend some real quality time with the code in
hungarian.c to turn it into using integer costs instead of floating point
numbers, to ensure that the arithmetic is done in a way that is consistent
with the algorithm, even if we cannot represent the arithmetic faithfully
with limited-range integers.
I'll think about the best way forward.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-03 18:05 ` [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike Ævar Arnfjörð Bjarmason
2018-05-03 21:07 ` Johannes Schindelin@ 2018-05-03 21:50 ` Jacob Keller1 sibling, 0 replies; 387+ messages in thread
From: Jacob Keller @ 2018-05-03 21:50 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Johannes Schindelin, Git mailing list, Junio C Hamano,
Thomas Rast, Thomas Gummerer
On Thu, May 3, 2018 at 11:05 AM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> 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
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 03/18] branch-diff: first rudimentary implementation
2018-05-03 21:19 ` Stefan Beller@ 2018-05-03 22:00 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-03 22:00 UTC (permalink / raw)
To: Stefan Beller
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
[-- Attachment #1: Type: text/plain, Size: 2637 bytes --]
Hi Stefan,
On Thu, 3 May 2018, Stefan Beller wrote:
> >> In addition to that patch, we'd have to buffer commit messages and
> >> buffer multiple commits, as that only buffers a diff of a single
> >> commit.
> >
> > ... and make sure that the moved-code logic (which is currently the
> > only user of emitted_symbols, correct?) would never be called at the
> > same time as we generate the diff.
>
> The moved detection is all part of the flags of an emitted symbol.
>
> By design the emitted symbol has easy access to the raw line of the output,
> which made it easy for the move detection to work on the lines. AFAICT this
> is also desired here as lines are put into a hashmap for comparisons.
> (and having it colored differently would make finding the same line
> complex using hashmaps)
>
> I just entertain the thought of having move detection active in a
> branch-diff. That would be really cool actually.
There are two separate times when we generate a diff in branch-diff: the
first time in that `git log -p <range>` call for both ranges, and later,
when displaying the changes of old/new commits.
(There is actually a third time, when the cost is calculated, but those
diffs are not shown, only their line count is used.)
It would be relatively easy to use move detection in the diff between
old/new commits. But it would be harder to do that with the `git log -p`
diffs, as we only color-code them later, not at the time they are
generated.
In fact, Ævar mentioned that he was pretty happy about the fact that `git
branch-diff` accepts all kinds of diff options, when tbdiff emulated only
two of them. Ævar mentioned specifically the use of `--color-words`...
> >> The benefit would be no invocation of new processes, letting us do
> >> more in core. This would allow for tweaking revision walking
> >> internally, e.g. passing of options to this command such as rename
> >> detection factors, can be passed through easily without the need of
> >> translating it back to the command line.
> >
> > On the other hand, we can simply copy those options to the
> > command-line for `log`. Which might even be better, as e.g. `--format`
> > changes global state :-(
>
> ok.
I really appreciate the sanity check. It would benefit me on Windows if I
could avoid spawning... But I think in this case, it would save me 2*50ms,
which is not really worth doing the work for, so far.
> Thanks for your patience,
And thank you for yours! It *is* important to challenge beliefs during
code review, so that the choices are made for the right reasons.
Thanks,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 05/18] branch-diff: also show the diff between patches
2018-05-03 15:30 ` [PATCH 05/18] branch-diff: also show the diff between patches Johannes Schindelin
@ 2018-05-04 2:51 ` Eric Sunshine
2018-05-04 3:15 ` Eric Sunshine
2018-05-04 7:15 ` Johannes Schindelin0 siblings, 2 replies; 387+ messages in thread
From: Eric Sunshine @ 2018-05-04 2:51 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Just like tbdiff, we now show the diff between matching patches. This is
> a "diff of two diffs", so it can be a bit daunting to read for the
> beginnger.
s/beginnger/beginner/
> This brings branch-diff closer to be feature-complete with regard to
s/be feature-complete/feature parity/
> tbdiff.
>
> An alternative would be to display an interdiff, i.e. the hypothetical
> diff which is the result of first reverting the old diff and then
> applying the new diff.
>
> Especially when rebasing often, an interdiff is often not feasible,
> though: if the old diff cannot be applied in reverse (due to a moving
> upstream), an interdiff can simply not be inferred.
>
> Note: while we now parse diff options such as --color, the effect is not
> yet the same as in tbdiff, where also the commit pairs would be colored.
"... tbdiff, in which the commit pairs would also be colored."
However, I don't see the --color option being parsed by this patch, so
perhaps this "Note" can be dropped?
> This is left for a later commit.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> @@ -319,24 +348,37 @@ static void output(struct string_list *a, struct string_list *b)
> int cmd_branch_diff(int argc, const char **argv, const char *prefix)
> {
> - int no_patches = 0;
> + struct diff_options diffopt = { 0 };
> double creation_weight = 0.6;
> struct option options[] = {
> - OPT_BOOL(0, "no-patches", &no_patches,
> - N_("short format (no diffs)")),
This was added in 2/18 but never used...
> + OPT_SET_INT(0, "no-patches", &diffopt.output_format,
> + N_("short format (no diffs)"),
> + DIFF_FORMAT_NO_OUTPUT),
... and is then replaced in its entirety by this. Perhaps just drop
the original --no-patches from 2/18 and let it be introduced for the
first time here?
> { OPTION_CALLBACK,
> 0, "creation-weight", &creation_weight, N_("factor"),
> N_("Fudge factor by which creation is weighted [0.6]"),
> 0, parse_creation_weight },
> OPT_END()
> };
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-03 21:49 ` Johannes Schindelin@ 2018-05-04 3:23 ` Junio C Hamano0 siblings, 0 replies; 387+ messages in thread
From: Junio C Hamano @ 2018-05-04 3:23 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Stefan Beller, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> So please: as soon as you have a concrete suggestion where to use cyan
> (and preferably even a DIFF_* constant to feed to diff_get_color_opt()), I
> will be more than interested.
I do not think Stefan's comment was that he was keen to use 'cyan'.
It was a color I suggested in a review of his change where he added
new colors to the color.[ch] palette, and I found that reusing an
existing color would have achieved the same distinction between
lines of output from his code, and it would be beneficial to make
the outcome consistent to consider why these existing colors are
used in existing places and trying to align the rationale for new
uses. "cyan" was cited as an example to illustrate that last point,
i.e. we use it to dim out relatively uninteresting part.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-04 7:23 ` Johannes Schindelin@ 2018-05-04 14:44 ` Duy Nguyen
2018-05-04 15:17 ` Duy Nguyen
2018-05-04 15:23 ` Johannes Schindelin0 siblings, 2 replies; 387+ messages in thread
From: Duy Nguyen @ 2018-05-04 14:44 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> Oh, okay. It was not at all clear to me what the exact format and role of
> these lines are...
Noted. I'm making more updates in this file in another topic and will
add some explanation so the next guy will be less confused.
> So that's what `info` does: it influences whether/where
> the command is listed in `git help`'s output... Interesting. I thought the
> lines here were trying to automate parts of the tab completion or
> something.
Oh it does many things. The completion part is coming (so yeah you
don't need to update git-completion.bash at all, as long as you have a
line here and use parse_options() ;-), but I think it's mainly for
"git help" and command listing in "git help git" (this command for
example should show up under the "Main porcelain commands" in that man
page when you put a line here)
--
Duy
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-04 14:44 ` Duy Nguyen@ 2018-05-04 15:17 ` Duy Nguyen
2018-05-04 15:23 ` Johannes Schindelin1 sibling, 0 replies; 387+ messages in thread
From: Duy Nguyen @ 2018-05-04 15:17 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On Fri, May 04, 2018 at 04:44:49PM +0200, Duy Nguyen wrote:
> On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Oh, okay. It was not at all clear to me what the exact format and role of
> > these lines are...
>
> Noted. I'm making more updates in this file in another topic and will
> add some explanation so the next guy will be less confused.
This is what I will include (but in a separate topic). I will not CC
you there to keep your inbox slightly less full. I hope this helps
understand what command-list.txt is for.
diff --git a/command-list.txt b/command-list.txt
index 40776b9587..929d8f0ea0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -1,3+1,47 @@+# Command classification list
+# ---------------------------
+# All supported commands, builtin or external, must be described in
+# here. This info is used to list commands in various places. Each
+# command is on one line followed by one or more attributes.
+#
+# The first attribute group is mandatory and indicates the command
+# type. This group includes:
+#
+# mainporcelain
+# ancillarymanipulators
+# ancillaryinterrogators
+# foreignscminterface
+# plumbingmanipulators
+# plumbinginterrogators
+# synchingrepositories
+# synchelpers
+# purehelpers
+#
+# the type names are self explanatory. But if you want to see what
+# command belongs to what group to get a better idea, have a look at
+# "git" man page, "GIT COMMANDS" section.
+#
+# Commands of type mainporcelain can also optionally have one of these
+# attributes:
+#
+# init
+# worktree
+# info
+# history
+# remote
+#
+# These commands are considered "common" and will show up in "git
+# help" output in groups. Uncommon porcelain commands must not
+# specify any of these attributes.
+#
+# "complete" attribute is used to mark that the command should be
+# completable by git-completion.bash. Note that by default,
+# mainporcelain commands are completable and you don't need this
+# attribute.
+#
+# While not true commands, guides are also specified here, which can
+# only have "guide" attribute and nothing else.
+#
### command list (do not change this line, also do not change alignment)
# command name category [category] [category]
git-add mainporcelain worktree
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-04 14:44 ` Duy Nguyen
2018-05-04 15:17 ` Duy Nguyen@ 2018-05-04 15:23 ` Johannes Schindelin
2018-05-04 15:29 ` Duy Nguyen1 sibling, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-04 15:23 UTC (permalink / raw)
To: Duy Nguyen
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Duy,
On Fri, 4 May 2018, Duy Nguyen wrote:
> On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > Oh, okay. It was not at all clear to me what the exact format and role of
> > these lines are...
>
> Noted. I'm making more updates in this file in another topic and will
> add some explanation so the next guy will be less confused.
Thank you!
> > So that's what `info` does: it influences whether/where
> > the command is listed in `git help`'s output... Interesting. I thought the
> > lines here were trying to automate parts of the tab completion or
> > something.
>
> Oh it does many things. The completion part is coming (so yeah you
> don't need to update git-completion.bash at all, as long as you have a
> line here and use parse_options() ;-), but I think it's mainly for
> "git help" and command listing in "git help git" (this command for
> example should show up under the "Main porcelain commands" in that man
> page when you put a line here)
I have a hard time believing that anything automated can infer from the
source code that branch-diff can accept the non-options in three different
formats, and what those formats look like...
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-04 15:23 ` Johannes Schindelin@ 2018-05-04 15:29 ` Duy Nguyen0 siblings, 0 replies; 387+ messages in thread
From: Duy Nguyen @ 2018-05-04 15:29 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On Fri, May 4, 2018 at 5:23 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>> > So that's what `info` does: it influences whether/where
>> > the command is listed in `git help`'s output... Interesting. I thought the
>> > lines here were trying to automate parts of the tab completion or
>> > something.
>>
>> Oh it does many things. The completion part is coming (so yeah you
>> don't need to update git-completion.bash at all, as long as you have a
>> line here and use parse_options() ;-), but I think it's mainly for
>> "git help" and command listing in "git help git" (this command for
>> example should show up under the "Main porcelain commands" in that man
>> page when you put a line here)
>
> I have a hard time believing that anything automated can infer from the
> source code that branch-diff can accept the non-options in three different
> formats, and what those formats look like...
Yeah. For now it can only do options which is machine readable. But I
have a big plan, so who knows :D
--
Duy
^permalinkrawreply [flat|nested] 387+ messages in thread

*[PATCH v2 17/18] branch-diff: add a man page
2018-05-04 15:34 ` [PATCH v2 " Johannes Schindelin
` (15 preceding siblings ...)
2018-05-04 15:35 ` [PATCH v2 16/18] branch-diff --dual-color: work around bogus white-space warning Johannes Schindelin
@ 2018-05-04 15:35 ` Johannes Schindelin
2018-05-04 15:35 ` [PATCH v2 18/18] completion: support branch-diff Johannes Schindelin
` (4 subsequent siblings)21 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-04 15:35 UTC (permalink / raw)
To: git
Cc: Johannes Schindelin, Junio C Hamano, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Stefan Beller, Jacob Keller, Eric Sunshine
This is a heavily butchered version of the README written by Thomas
Rast and Thomas Gummerer, lifted from https://github.com/trast/tbdiff.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
Documentation/git-branch-diff.txt | 239 ++++++++++++++++++++++++++++++
1 file changed, 239 insertions(+)
create mode 100644 Documentation/git-branch-diff.txt
diff --git a/Documentation/git-branch-diff.txt b/Documentation/git-branch-diff.txt
new file mode 100644
index 00000000000..f9e23eaf721
--- /dev/null
+++ b/Documentation/git-branch-diff.txt
@@ -0,0 +1,239 @@+git-branch-diff(1)
+==================
+
+NAME
+----
+git-branch-diff - Compare two versions of a branch
+
+SYNOPSIS
+--------
+[verse]
+'git branch-diff' [--color=[<when>]] [--no-color] [<diff-options>]
+ [--dual-color] [--no-patches] [--creation-weight=<weight>]
+ ( <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).
+
+To that end, it first finds pairs of commits from both commit ranges
+that correspond with each other. Two commits are said to correspond when
+the diff between their patches (i.e. the author information, the commit
+message and the commit diff) is reasonably small compared to the
+patches' size. See ``Algorithm` below for details.
+
+Finally, the list of matching commits is shown in the order of the
+second commit range, with unmatched commits being inserted just after
+all of their ancestors have been shown.
+
+
+OPTIONS
+-------
+--no-patches::
+ Suppress the diffs between commit pairs that were deemed to
+ correspond; only show the pairings.
+
+--dual-color::
+ When the commit diffs differ, recreate the original diffs'
+ coloring, and add outer -/+ diff markers with the *background*
+ being red/green to make it easier to see e.g. when there was a
+ change in what exact lines were added.
+
+--creation-weight=<factor>::
+ Set the creation/deletion cost fudge factor to `<factor>`.
+ Defaults to 0.6. Try a larger value if `git branch-diff`
+ erroneously considers a large change a total rewrite (deletion
+ of one commit and addition of another), and a smaller one in
+ the reverse case. See the ``Algorithm`` section below for an
+ explanation why this is needed.
+
+<range1> <range2>::
+ Compare the commits specified by the two ranges, where
+ `<range1>` is considered an older version of `<range2>`.
+
+<rev1>...<rev2>::
+ Equivalent to passing `<rev2>..<rev1>` and `<rev1>..<rev2>`.
+
+<base> <rev1> <rev2>::
+ Equivalent to passing `<base>..<rev1>` and `<base>..<rev2>`.
+ Note that `<base>` does not need to be the exact branch point
+ of the branches. Example: after rebasing a branch `my-topic`,
+ `git branch-diff my-topic@{u} my-topic@{1} my-topic` would
+ show the differences introduced by the rebase.
+
+`git branch-diff` also accepts the regular diff options (see
+linkgit:git-diff[1]), most notably the `--color=[<when>]` and
+`--no-color` options. These options are used when generating the "diff
+between patches", i.e. to compare the author, commit message and diff of
+corresponding old/new commits. There is currently no means to tweak the
+diff options passed to `git log` when generating those patches.
+
+
+CONFIGURATION
+-------------
+This command uses the `diff.color.*` and `pager.branch-diff` settings
+(the latter is on by default).
+See linkgit:git-config[1].
+
+
+Examples
+--------
+
+When a rebase required merge conflicts to be resolved, compare the changes
+introduced by the rebase directly afterwards using:
+
+------------
+$ git branch-diff @{u} @{1} @
+------------
+
+
+A typical output of `git branch-diff` would look like this:
+
+------------
+-: ------- > 1: 0ddba11 Prepare for the inevitable!
+1: c0debee = 2: cab005e Add a helpful message at the start
+2: f00dbal ! 3: decafe1 Describe a bug
+ @@ -1,3 +1,3 @@
+ Author: A U Thor <author@example.com>
+
+ -TODO: Describe a bug
+ +Describe a bug
+ @@ -324,5 +324,6
+ This is expected.
+
+ -+What is unexpected is that it will also crash.
+ ++Unexpectedly, it also crashes. This is a bug, and the jury is
+ ++still out there how to fix it best. See ticket #314 for details.
+
+ Contact
+3: bedead < -: ------- TO-UNDO
+------------
+
+In this example, there are 3 old and 3 new commits, where the developer
+removed the 3rd, added a new one before the first two, and modified the
+commit message of the 2nd commit as well its diff.
+
+When the output goes to a terminal, it is color-coded by default, just
+like regular `git diff`'s output. In addition, the first line (adding a
+commit) is green, the last line (deleting a commit) is red, the second
+line (with a perfect match) is yellow like the commit header of `git
+show`'s output, and the third line colors the old commit red, the new
+one green and the rest like `git show`'s commit header.
+
+The color-coded diff is actually a bit hard to read, though, as it
+colors the entire lines red or green. The line that added "What is
+unexpected" in the old commit, for example, is completely red, even if
+the intent of the old commit was to add something.
+
+To help with that, use the `--dual-color` mode. In this mode, the diff
+of diffs will retain the original diff colors, and prefix the lines with
+-/+ markers that have their *background* red or green, to make it more
+obvious that they describe how the diff itself changed.
+
+
+Algorithm
+---------
+
+The general idea is this: we generate a cost matrix between the commits
+in both commit ranges, then solve the least-cost assignment.
+
+To avoid false positives (e.g. when a patch has been removed, and an
+unrelated patch has been added between two iterations of the same patch
+series), the cost matrix is extended to allow for that, by adding
+fixed-cost entries for wholesale deletes/adds.
+
+Example: Let commits `1--2` be the first iteration of a patch series and
+`A--C` the second iteration. Let's assume that `A` is a cherry-pick of
+`2,` and `C` is a cherry-pick of `1` but with a small modification (say,
+a fixed typo). Visualize the commits as a bipartite graph:
+
+------------
+ 1 A
+
+ 2 B
+
+ C
+------------
+
+We are looking for a "best" explanation of the new series in terms of
+the old one. We can represent an "explanation" as an edge in the graph:
+
+
+------------
+ 1 A
+ /
+ 2 --------' B
+
+ C
+------------
+
+This explanation comes for "free" because there was no change. Similarly
+`C` could be explained using `1`, but that comes at some cost c>0
+because of the modification:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ |
+ `----- C
+ c>0
+------------
+
+In mathematical terms, what we are looking for is some sort of a minimum
+cost bipartite matching; `1` is matched to `C` at some cost, etc. The
+underlying graph is in fact a complete bipartite graph; the cost we
+associate with every edge is the size of the diff between the two
+commits' patches. To explain also new commits, we introduce dummy nodes
+on both sides:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ |
+ o `----- C
+ c>0
+ o o
+
+ o o
+------------
+
+The cost of an edge `o--C` is the size of `C`'s diff, modified by a
+fudge factor that should be smaller than 1.0. The cost of an edge `o--o`
+is free. The fudge factor is necessary because even if `1` and `C` have
+nothing in common, they may still share a few empty lines and such,
+possibly making the assignment `1--C`, `o--o` slightly cheaper than
+`1--o`, `o--C` even if `1` and `C` have nothing in common. With the
+fudge factor we require a much larger common part to consider patches as
+corresponding.
+
+The overall time needed to compute this algorithm is the time needed to
+compute n+m commit diffs and then n*m diffs of patches, plus the time
+needed to compute the least-cost assigment between n and m diffs. Git
+uses an implementation of the Jonker-Volgenant algorithm to solve the
+assignment problem, which has cubic runtime complexity. The matching
+found in this case will look like this:
+
+------------
+ 1 ----. A
+ | /
+ 2 ----+---' B
+ .--+-----'
+ o -' `----- C
+ c>0
+ o ---------- o
+
+ o ---------- o
+------------
+
+
+SEE ALSO
+--------
+linkgit:git-log[1]
+
+GIT
+---
+Part of the linkgit:git[1] suite
--
2.17.0.409.g71698f11835
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-04 6:40 ` Johannes Schindelin@ 2018-05-04 15:37 ` Ramsay Jones
2018-05-05 19:41 ` Johannes Schindelin
2018-05-04 16:34 ` Elijah Newren1 sibling, 1 reply; 387+ messages in thread
From: Ramsay Jones @ 2018-05-04 15:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On 04/05/18 07:40, Johannes Schindelin wrote:
[snip]
> BTW I ran `make sparse` for the first time, and it spits out tons of
> stuff. And I notice that they are all non-fatal warnings, but so were the
> ones you pointed out above. This is a bit sad, as I would *love* to
> install a VSTS build job to run `make sparse` automatically. Examples of
> warnings *after* applying your patch:
>
> connect.c:481:40: warning: incorrect type in argument 2 (invalid types)
> connect.c:481:40: expected union __CONST_SOCKADDR_ARG [usertype] __addr
> connect.c:481:40: got struct sockaddr *ai_addr
>
> or
>
> pack-revindex.c:65:23: warning: memset with byte count of 262144
>
> What gives?
My stock answer, until recently, was that you are using a very
old version of sparse. Which is probably still true here - but
I recently noticed that more up-to-date platforms/gcc versions
also have many problems. (The main sparse contributors tend to
stick with conservative distros and/or don't use sparse on any
software that uses system headers - thus they tend not to notice
the problems caused by new gcc/glibc versions! ;-) )
Since I am on Linux Mint 18.3 (based on the last Ubuntu LTS) and
build sparse from source, the current 'master', 'next' and 'pu'
branches are all 'sparse-clean' for me. (On cygwin, which is
built with NO_REGEX, I have a single sparse warning).
I was just about to say that, unusually for me, I was using a
sparse built from a release tag, but then remembered that I have
some additional patches which fixes a problem on fedora 27!
Using sparse on fedora 27 is otherwise useless. (There are still
many warnings spewed on f27 - but they are caused by incorrect
system headers :( ).
The current release of sparse is v0.5.2, which probably hasn't
been included in any distro yet (I think the previous release
v0.5.1, which should also work for you, is in Debian unstable).
If you wanted to try building a more up-to-date sparse, the repo
is at: git://git.kernel.org/pub/scm/devel/sparse/sparse.git.
Linux Mint 19, which will be released in about a month, will be
using the Ubuntu 18.04 LTS as a base, so I guess it is possible
that I will need to debug sparse again ...
BTW, I spent some time last night playing with 'git-branch-diff'.
First of all - Good Job! This tool will be very useful (thanks
also go to Thomas, of course).
I noticed that there seemed to be an occasional 'whitespace error'
indicator (red background) directly after an +/- change character
which I suspect is an error (I haven't actually checked). However,
this indicator disappears if you add the --dual-color option.
Thanks!
ATB,
Ramsay Jones
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-04 15:34 ` [PATCH v2 " Johannes Schindelin
` (17 preceding siblings ...)
2018-05-04 15:35 ` [PATCH v2 18/18] completion: support branch-diff Johannes Schindelin
@ 2018-05-04 16:21 ` Elijah Newren
2018-05-04 16:30 ` Elijah Newren
2018-05-05 20:03 ` Johannes Schindelin
2018-05-06 5:22 ` Junio C Hamano
` (2 subsequent siblings)21 siblings, 2 replies; 387+ messages in thread
From: Elijah Newren @ 2018-05-04 16:21 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> 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
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-04 16:21 ` [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike Elijah Newren
@ 2018-05-04 16:30 ` Elijah Newren
2018-05-05 20:03 ` Johannes Schindelin1 sibling, 0 replies; 387+ messages in thread
From: Elijah Newren @ 2018-05-04 16:30 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi,
On Fri, May 4, 2018 at 9:21 AM, Elijah Newren <newren@gmail.com> wrote:
> On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> 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... :-)
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-04 15:34 ` [PATCH v2 02/18] Add a new builtin: branch-diff Johannes Schindelin
@ 2018-05-05 18:26 ` Jeff King
2018-05-05 21:57 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Jeff King @ 2018-05-05 18:26 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote:
> This builtin does not do a whole lot so far, apart from showing a usage
> that is oddly similar to that of `git tbdiff`. And for a good reason:
> the next commits will turn `branch-diff` into a full-blown replacement
> for `tbdiff`.
One minor point about the name: will it become annoying as a tab
completion conflict with git-branch?
It feels really petty complaining about the name, but I just want to
raise the point, since it will never be easier to change than right now.
(And no, I don't really have another name in mind; I'm just wondering if
"subset" names like this might be a mild annoyance in the long run).
-Peff
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-04 15:37 ` Ramsay Jones@ 2018-05-05 19:41 ` Johannes Schindelin
2018-05-09 16:24 ` Ramsay Jones0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-05 19:41 UTC (permalink / raw)
To: Ramsay Jones
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Ramsay,
On Fri, 4 May 2018, Ramsay Jones wrote:
> On 04/05/18 07:40, Johannes Schindelin wrote:
> [snip]
> > BTW I ran `make sparse` for the first time, and it spits out tons of
> > stuff. And I notice that they are all non-fatal warnings, but so were the
> > ones you pointed out above. This is a bit sad, as I would *love* to
> > install a VSTS build job to run `make sparse` automatically. Examples of
> > warnings *after* applying your patch:
> >
> > connect.c:481:40: warning: incorrect type in argument 2 (invalid types)
> > connect.c:481:40: expected union __CONST_SOCKADDR_ARG [usertype] __addr
> > connect.c:481:40: got struct sockaddr *ai_addr
> >
> > or
> >
> > pack-revindex.c:65:23: warning: memset with byte count of 262144
> >
> > What gives?
>
> My stock answer, until recently, was that you are using a very
> old version of sparse.
Sure. I did this in an Ubuntu 16.04 LTS VM, via `sudo apt-get install
sparse`.
> Which is probably still true here - but I recently noticed that more
> up-to-date platforms/gcc versions also have many problems. (The main
> sparse contributors tend to stick with conservative distros and/or don't
> use sparse on any software that uses system headers - thus they tend not
> to notice the problems caused by new gcc/glibc versions! ;-) )
>
> Since I am on Linux Mint 18.3 (based on the last Ubuntu LTS) and build
> sparse from source, the current 'master', 'next' and 'pu' branches are
> all 'sparse-clean' for me. (On cygwin, which is built with NO_REGEX, I
> have a single sparse warning).
>
> I was just about to say that, unusually for me, I was using a sparse
> built from a release tag, but then remembered that I have some
> additional patches which fixes a problem on fedora 27! Using sparse on
> fedora 27 is otherwise useless. (There are still many warnings spewed on
> f27 - but they are caused by incorrect system headers :( ).
>
> The current release of sparse is v0.5.2, which probably hasn't been
> included in any distro yet (I think the previous release v0.5.1, which
> should also work for you, is in Debian unstable). If you wanted to try
> building a more up-to-date sparse, the repo is at:
> git://git.kernel.org/pub/scm/devel/sparse/sparse.git.
Well, what I would want to do is let the cloud work for me. By adding an
automated build to my Visual Studio Team Services (VSTS) account, of
course, as I have "cloud privilege" (i.e. I work in the organization
providing the service, so I get to play with all of it for free).
So I really don't want to build sparse every time a new revision needs to
be tested (whether that be from one of my branches, an internal PR for
pre-review of patches to be sent to the mailing list, or maybe even `pu`
or the personalized branches on https://github.com/gitster/git).
I really would need a ready-to-install sparse, preferably as light-weight
as possible (by not requiring any dependencies outside what is available
in VSTS' hosted Linux build agents.
Maybe there is a specific apt source for sparse?
> Linux Mint 19, which will be released in about a month, will be
> using the Ubuntu 18.04 LTS as a base, so I guess it is possible
> that I will need to debug sparse again ...
:-)
> BTW, I spent some time last night playing with 'git-branch-diff'.
Great!
> First of all - Good Job! This tool will be very useful (thanks
> also go to Thomas, of course).
Both Thomases. Thomas Rast and Thomas Gummerer.
> I noticed that there seemed to be an occasional 'whitespace error'
> indicator (red background) directly after an +/- change character
> which I suspect is an error (I haven't actually checked). However,
> this indicator disappears if you add the --dual-color option.
Indeed. This is a quirk of the whitespace error paired with diffing diffs:
the whitespace error correctly treats the leading space as marker for a
context line, but if you diff diffs, the next character may still be a
marker for a context line (but this time the "inner" diff). And our
whitespace error detection mechanism cannot guess that it looks at a diff
of diffs.
However, in dual-color mode, we know that we will almost certainly look at
diffs of diffs (except if the change is in the commit message, in which
case I don't care about whitespace errors at all, anyway).
So I have this hack in 16/18:
https://public-inbox.org/git/b99ab186c4f11239a10950b9902d9c87d0e60513.1525448066.git.johannes.schindelin@gmx.de/T/#u
Essentially, I extend the dual-color mode to detect where such a bogus
whitespace error would be detected, and simply *skip the space*! I can get
away with that because dual-color is meant for human consumption, and if a
horizontal tab follows, it would not matter whether there was a space: it
would find the same tab stop. Likewise, if the space comes before a CR or
LF, or even just before the final NUL, the space can be safely omitted
from the output, too.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-04 16:21 ` [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike Elijah Newren
2018-05-04 16:30 ` Elijah Newren@ 2018-05-05 20:03 ` Johannes Schindelin
2018-05-07 17:07 ` Elijah Newren1 sibling, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-05 20:03 UTC (permalink / raw)
To: Elijah Newren
Cc: Git Mailing List, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Elijah,
On Fri, 4 May 2018, Elijah Newren wrote:
> On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> 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.schindelin@gmx.de/
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-05 18:26 ` Jeff King@ 2018-05-05 21:57 ` Johannes Schindelin
2018-05-06 0:25 ` Todd Zullinger
` (3 more replies)0 siblings, 4 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-05 21:57 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Peff,
On Sat, 5 May 2018, Jeff King wrote:
> On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote:
>
> > This builtin does not do a whole lot so far, apart from showing a usage
> > that is oddly similar to that of `git tbdiff`. And for a good reason:
> > the next commits will turn `branch-diff` into a full-blown replacement
> > for `tbdiff`.
>
> One minor point about the name: will it become annoying as a tab
> completion conflict with git-branch?
I did mention this in the commit message of 18/18:
Without this patch, we would only complete the `branch-diff` part but
not the options and other arguments.
This of itself may already be slightly disruptive for well-trained
fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
`git branch origin/master`, as we now no longer automatically append a
space after completing `git branch`: this is now ambiguous.
> It feels really petty complaining about the name, but I just want to
> raise the point, since it will never be easier to change than right now.
I do hear you. Especially since I hate `git cherry` every single time that
I try to tab-complete `git cherry-pick`.
> (And no, I don't really have another name in mind; I'm just wondering if
> "subset" names like this might be a mild annoyance in the long run).
They totally are, and if you can come up with a better name, I am really
interested in changing it before this hits `next`, even.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 13/18] color: provide inverted colors, too
2018-05-05 18:29 ` Jeff King@ 2018-05-05 22:03 ` Johannes Schindelin
2018-05-06 6:35 ` Jeff King0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-05 22:03 UTC (permalink / raw)
To: Jeff King
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Peff,
On Sat, 5 May 2018, Jeff King wrote:
> On Fri, May 04, 2018 at 05:34:58PM +0200, Johannes Schindelin wrote:
>
> > For every regular color, there exists the inverted equivalent where
> > background and foreground colors are exchanged.
> >
> > We will use this in the next commit to allow inverting *just* the +/-
> > signs in a diff.
>
> There's a "reverse" attribute (which we already parse and support) that
> can do this without having to repeat the colors. AFAIK it's well
> supported everywhere, but I could be wrong.
How would I use that here, though? I need to get the thing via
diff_get_color_opt() which takes a parameter of type `enum color_diff`.
There is no way I can specify `reverse` here, can I?
> I wonder if that would make configuring this slightly more pleasant,
> since it saves the user having to define "oldinv" whenever they change
> "old".
I am all for making the configuration more pleasant. So I hope I can make
use of the `reverse` thing here, without having to introduce a new enum
value.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
2018-05-04 15:34 ` [PATCH v2 12/18] branch-diff: use color for the commit pairs Johannes Schindelin
@ 2018-05-05 23:48 ` Todd Zullinger
2018-05-07 1:52 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Todd Zullinger @ 2018-05-05 23:48 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Johannes,
As many others have already said, thanks for this series!
I've used tbdiff a bit over the years, but having a builtin
will make it much more convenient (and the speed boost from
a C implementation will be a very nice bonus).
Johannes Schindelin wrote:
> @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const char *prefix)
> struct string_list branch1 = STRING_LIST_INIT_DUP;
> struct string_list branch2 = STRING_LIST_INIT_DUP;
>
> + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> +
> diff_setup(&diffopt);
> diffopt.output_format = DIFF_FORMAT_PATCH;
> diffopt.flags.suppress_diff_headers = 1;
Should this also (or only) check color.diff.frag? I thought
that color.diff.* was preferred over diff.color.*, though
that doesn't seem to be entirely true in all parts of the
current codebase.
In testing this series it seems that setting color.diff
options to change the various colors read earlier in this
patch via diff_get_color_opt, as well as the 'frag' slot,
are ignored. Setting them via diff.color.<slot> does work.
The later patch adding a man page documents branch-diff as
using `diff.color.*` and points to git-config(1), but the
config docs only list color.diff.
Is this a bug in the diff_get_color{,_opt}() tooling?
It's certainly not anything you've introduced here, of
course. I just noticed that some custom color.diff settings
I've used weren't picked up by branch-diff, despite your
clear intention to respect colors from the config.
--
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Abandon the search for Truth; settle for a good fantasy.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-05 21:57 ` Johannes Schindelin@ 2018-05-06 0:25 ` Todd Zullinger
2018-05-06 0:38 ` Todd Zullinger
2018-05-06 1:05 ` Igor Djordjevic
` (2 subsequent siblings)3 siblings, 1 reply; 387+ messages in thread
From: Todd Zullinger @ 2018-05-06 0:25 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Johannes,
Johannes Schindelin wrote:
> On Sat, 5 May 2018, Jeff King wrote:
>> One minor point about the name: will it become annoying as a tab
>> completion conflict with git-branch?
>
> I did mention this in the commit message of 18/18:
>
> Without this patch, we would only complete the `branch-diff` part but
> not the options and other arguments.
>
> This of itself may already be slightly disruptive for well-trained
> fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
> `git branch origin/master`, as we now no longer automatically append a
> space after completing `git branch`: this is now ambiguous.
>
>> It feels really petty complaining about the name, but I just want to
>> raise the point, since it will never be easier to change than right now.
>
> I do hear you. Especially since I hate `git cherry` every single time that
> I try to tab-complete `git cherry-pick`.
>
>> (And no, I don't really have another name in mind; I'm just wondering if
>> "subset" names like this might be a mild annoyance in the long run).
>
> They totally are, and if you can come up with a better name, I am really
> interested in changing it before this hits `next`, even.
Would it be possible and reasonable to teach 'git branch' to
call this as a subcommand, i.e. as 'git branch diff'? Then
the completion wouldn't offer git branch-diff.
Users could still call it directly if they wanted, though
I'd tend to think that should be discouraged and have it
treated as an implementation detail that it's a separate
binary.
We have a number of commands which take subcommands this way
(bundle, bisect, notes, submodule, and stash come to mind).
I don't know if any are used with and without a subcommand,
but it doesn't seem too strange from a UI point of view, to
me.
(I don't know if it's coincidental that of the existing
commands I noted above, 3 of the 5 are currently implemented
as shell scripts. But they've all seen at least some work
toward converting them to C, I believe).
The idea might be gross and/or unreasonable from an
implementation or UI view. I'm not sure, but I thought I
would toss the idea out.
This wouldn't work for git cherry{,-pick} where you wouldn't
consider 'git cherry pick' as related to 'git cherry'
though.
We also have this with git show{,-branch} and some others.
It's a mild annoyance, but muscle memory adapts eventually.
--
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
A budget is just a method of worrying before you spend money, as well
as afterward.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-05 21:57 ` Johannes Schindelin
2018-05-06 0:25 ` Todd Zullinger@ 2018-05-06 1:05 ` Igor Djordjevic
2018-05-06 4:53 ` Jacob Keller
2018-05-06 12:10 ` Johannes Schindelin
2018-05-06 2:33 ` Junio C Hamano
2018-05-07 7:50 ` Jeff King3 siblings, 2 replies; 387+ messages in thread
From: Igor Djordjevic @ 2018-05-06 1:05 UTC (permalink / raw)
To: Johannes Schindelin, Jeff King
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Dscho,
On 05/05/2018 23:57, Johannes Schindelin wrote:
>
> > > This builtin does not do a whole lot so far, apart from showing a
> > > usage that is oddly similar to that of `git tbdiff`. And for a
> > > good reason: the next commits will turn `branch-diff` into a
> > > full-blown replacement for `tbdiff`.
> >
> > One minor point about the name: will it become annoying as a tab
> > completion conflict with git-branch?
>
> I did mention this in the commit message of 18/18:
>
> Without this patch, we would only complete the `branch-diff` part but
> not the options and other arguments.
>
> This of itself may already be slightly disruptive for well-trained
> fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
> `git branch origin/master`, as we now no longer automatically append a
> space after completing `git branch`: this is now ambiguous.
>
> > It feels really petty complaining about the name, but I just want
> > to raise the point, since it will never be easier to change than
> > right now.
>
> I do hear you. Especially since I hate `git cherry` every single
> time that I try to tab-complete `git cherry-pick`.
>
> > (And no, I don't really have another name in mind; I'm just
> > wondering if "subset" names like this might be a mild annoyance in
> > the long run).
>
> They totally are, and if you can come up with a better name, I am
> really interested in changing it before this hits `next`, even.
I gave this just a quick glance so might be I`m missing something
obvious or otherwise well-known here, bur why not `diff-branch` instead?
From user interface perspective, I would (personally) rather expect a
command that does "diff of branches" to belong to "diff family" of
commands (just operating on branches, instead of "branch" command
knowing to "diff itself"), and I see we already have `diff-files`,
`diff-index` and `diff-tree`, for what that`s worth.
Heck, I might even expect something like `git diff --branch ...` to work,
but I guess that is yet a different matter :)
Thanks, Buga
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 1:05 ` Igor Djordjevic@ 2018-05-06 4:53 ` Jacob Keller
2018-05-06 8:32 ` Duy Nguyen
2018-05-06 12:10 ` Johannes Schindelin1 sibling, 1 reply; 387+ messages in thread
From: Jacob Keller @ 2018-05-06 4:53 UTC (permalink / raw)
To: Igor Djordjevic
Cc: Johannes Schindelin, Jeff King, Git mailing list, Junio C Hamano,
Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Eric Sunshine
On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic
<igor.d.djordjevic@gmail.com> wrote:
> Hi Dscho,
>
> On 05/05/2018 23:57, Johannes Schindelin wrote:
>>
>> > > This builtin does not do a whole lot so far, apart from showing a
>> > > usage that is oddly similar to that of `git tbdiff`. And for a
>> > > good reason: the next commits will turn `branch-diff` into a
>> > > full-blown replacement for `tbdiff`.
>> >
>> > One minor point about the name: will it become annoying as a tab
>> > completion conflict with git-branch?
>>
>> I did mention this in the commit message of 18/18:
>>
>> Without this patch, we would only complete the `branch-diff` part but
>> not the options and other arguments.
>>
>> This of itself may already be slightly disruptive for well-trained
>> fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
>> `git branch origin/master`, as we now no longer automatically append a
>> space after completing `git branch`: this is now ambiguous.
>>
>> > It feels really petty complaining about the name, but I just want
>> > to raise the point, since it will never be easier to change than
>> > right now.
>>
>> I do hear you. Especially since I hate `git cherry` every single
>> time that I try to tab-complete `git cherry-pick`.
>>
>> > (And no, I don't really have another name in mind; I'm just
>> > wondering if "subset" names like this might be a mild annoyance in
>> > the long run).
>>
>> They totally are, and if you can come up with a better name, I am
>> really interested in changing it before this hits `next`, even.
>
> I gave this just a quick glance so might be I`m missing something
> obvious or otherwise well-known here, bur why not `diff-branch` instead?
>
> From user interface perspective, I would (personally) rather expect a
> command that does "diff of branches" to belong to "diff family" of
> commands (just operating on branches, instead of "branch" command
> knowing to "diff itself"), and I see we already have `diff-files`,
> `diff-index` and `diff-tree`, for what that`s worth.
>
> Heck, I might even expect something like `git diff --branch ...` to work,
> but I guess that is yet a different matter :)
>
> Thanks, Buga
I like diff-branch, though I suppose that also conflicts with diff too.
Thanks,
Jake
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 13/18] color: provide inverted colors, too
2018-05-05 22:03 ` Johannes Schindelin@ 2018-05-06 6:35 ` Jeff King
2018-05-06 6:41 ` Jeff King0 siblings, 1 reply; 387+ messages in thread
From: Jeff King @ 2018-05-06 6:35 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
On Sun, May 06, 2018 at 12:03:50AM +0200, Johannes Schindelin wrote:
> > There's a "reverse" attribute (which we already parse and support) that
> > can do this without having to repeat the colors. AFAIK it's well
> > supported everywhere, but I could be wrong.
>
> How would I use that here, though? I need to get the thing via
> diff_get_color_opt() which takes a parameter of type `enum color_diff`.
> There is no way I can specify `reverse` here, can I?
My thinking was that the code would know that coloring the initial "+"
should combine color.diff.new, along with a new tbdiff-specific config
option. So the C equivalent of something like this:
new=$(git config --get-color color.diff.new green)
tbdiff=$(git config --get-color color.tbdiff.new reverse)
reset=$(git config --get-color color.diff.reset reset)
echo "${new}${tbdiff}+${reset}${new}+actual diff content${reset}"
Then if you set color.diff.new to blue, you'll get a reverse-blue "+"
without having to configure anything else.
You can still override the tbdiff coloring with a totally unrelated
color, since it comes after ${new} (so you could set it to purple or
something if you wanted, though obviously a background or attribute from
${new} can still leak through if you have one set). The only downside in
such a case is that the color sequence is slightly longer ("green, no
blue!").
You could also have tbdiff.new and tbdiff.old to allow setting them
independently (but they'd both default to "reverse").
> > I wonder if that would make configuring this slightly more pleasant,
> > since it saves the user having to define "oldinv" whenever they change
> > "old".
>
> I am all for making the configuration more pleasant. So I hope I can make
> use of the `reverse` thing here, without having to introduce a new enum
> value.
I think the new enum (and matching config) has some value in case people
want to override it. But if you don't want to, diff_get_color() is
really just checking want_color() as a convenience. You could do that,
too:
const char *reverse = want_color(opt->use_color) ? GIT_COLOR_REVERSE : "";
You'd have to introduce GIT_COLOR_REVERSE. I don't think we have a
constant for it yet, but it's \x[7m.
-Peff
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 4:53 ` Jacob Keller@ 2018-05-06 8:32 ` Duy Nguyen
2018-05-06 12:08 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Duy Nguyen @ 2018-05-06 8:32 UTC (permalink / raw)
To: Jacob Keller
Cc: Igor Djordjevic, Johannes Schindelin, Jeff King,
Git mailing list, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Eric Sunshine
On Sun, May 6, 2018 at 6:53 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic
> <igor.d.djordjevic@gmail.com> wrote:
>> Hi Dscho,
>>
>> On 05/05/2018 23:57, Johannes Schindelin wrote:
>>>
>>> > > This builtin does not do a whole lot so far, apart from showing a
>>> > > usage that is oddly similar to that of `git tbdiff`. And for a
>>> > > good reason: the next commits will turn `branch-diff` into a
>>> > > full-blown replacement for `tbdiff`.
>>> >
>>> > One minor point about the name: will it become annoying as a tab
>>> > completion conflict with git-branch?
>>>
>>> I did mention this in the commit message of 18/18:
>>>
>>> Without this patch, we would only complete the `branch-diff` part but
>>> not the options and other arguments.
>>>
>>> This of itself may already be slightly disruptive for well-trained
>>> fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
>>> `git branch origin/master`, as we now no longer automatically append a
>>> space after completing `git branch`: this is now ambiguous.
>>>
>>> > It feels really petty complaining about the name, but I just want
>>> > to raise the point, since it will never be easier to change than
>>> > right now.
>>>
>>> I do hear you. Especially since I hate `git cherry` every single
>>> time that I try to tab-complete `git cherry-pick`.
>>>
>>> > (And no, I don't really have another name in mind; I'm just
>>> > wondering if "subset" names like this might be a mild annoyance in
>>> > the long run).
>>>
>>> They totally are, and if you can come up with a better name, I am
>>> really interested in changing it before this hits `next`, even.
>>
>> I gave this just a quick glance so might be I`m missing something
>> obvious or otherwise well-known here, bur why not `diff-branch` instead?
>>
>> From user interface perspective, I would (personally) rather expect a
>> command that does "diff of branches" to belong to "diff family" of
>> commands (just operating on branches, instead of "branch" command
>> knowing to "diff itself"), and I see we already have `diff-files`,
>> `diff-index` and `diff-tree`, for what that`s worth.
>>
>> Heck, I might even expect something like `git diff --branch ...` to work,
>> but I guess that is yet a different matter :)
>>
>> Thanks, Buga
>
> I like diff-branch, though I suppose that also conflicts with diff too.
How about interdiff?
--
Duy
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 0:38 ` Todd Zullinger@ 2018-05-06 12:04 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-06 12:04 UTC (permalink / raw)
To: Todd Zullinger
Cc: Jeff King, git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Todd,
On Sat, 5 May 2018, Todd Zullinger wrote:
> I wrote:
> > Would it be possible and reasonable to teach 'git branch' to
> > call this as a subcommand, i.e. as 'git branch diff'? Then
> > the completion wouldn't offer git branch-diff.
>
> Of course right after I sent this, it occurred to me that
> 'git branch diff' would make mask the ability to create a
> branch named diff. Using 'git branch --diff ...' wouldn't
> suffer that problem.
Yep, I immediately thought of --diff instead of diff when I read your
previous mail on that matter. And I like this idea!
Of course, it will complicate the code to set up the pager a bit (for
`branch-diff`, I could default to "on" all the time). But IIRC we recently
changed the --list cmdmode to set the pager to "auto", so I'll just copy
that.
> It does add a bit more overhead to the 'git branch' command,
> in terms of documentation and usage. I'm not sure it's too
> much though. The git-branch summary wouldn't change much:
>
> -git-branch - List, create, or delete branches
> +git-branch - List, create, delete, or diff branches
Indeed.
Unless I hear objections, I will work on moving to `git branch --diff` (it
might take a while, though, I will be traveling for work this week).
Ciao,
Johannes
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 8:32 ` Duy Nguyen@ 2018-05-06 12:08 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-06 12:08 UTC (permalink / raw)
To: Duy Nguyen
Cc: Jacob Keller, Igor Djordjevic, Jeff King, Git mailing list,
Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Eric Sunshine
Hi Duy,
On Sun, 6 May 2018, Duy Nguyen wrote:
> On Sun, May 6, 2018 at 6:53 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> > On Sat, May 5, 2018 at 6:05 PM, Igor Djordjevic
> > <igor.d.djordjevic@gmail.com> wrote:
> >>
> >> On 05/05/2018 23:57, Johannes Schindelin wrote:
> >>>
> >>> > > This builtin does not do a whole lot so far, apart from showing a
> >>> > > usage that is oddly similar to that of `git tbdiff`. And for a
> >>> > > good reason: the next commits will turn `branch-diff` into a
> >>> > > full-blown replacement for `tbdiff`.
> >>> >
> >>> > One minor point about the name: will it become annoying as a tab
> >>> > completion conflict with git-branch?
> >>>
> >>> I did mention this in the commit message of 18/18:
> >>>
> >>> Without this patch, we would only complete the `branch-diff` part but
> >>> not the options and other arguments.
> >>>
> >>> This of itself may already be slightly disruptive for well-trained
> >>> fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
> >>> `git branch origin/master`, as we now no longer automatically append a
> >>> space after completing `git branch`: this is now ambiguous.
> >>>
> >>> > It feels really petty complaining about the name, but I just want
> >>> > to raise the point, since it will never be easier to change than
> >>> > right now.
> >>>
> >>> I do hear you. Especially since I hate `git cherry` every single
> >>> time that I try to tab-complete `git cherry-pick`.
> >>>
> >>> > (And no, I don't really have another name in mind; I'm just
> >>> > wondering if "subset" names like this might be a mild annoyance in
> >>> > the long run).
> >>>
> >>> They totally are, and if you can come up with a better name, I am
> >>> really interested in changing it before this hits `next`, even.
> >>
> >> I gave this just a quick glance so might be I`m missing something
> >> obvious or otherwise well-known here, bur why not `diff-branch` instead?
> >>
> >> From user interface perspective, I would (personally) rather expect a
> >> command that does "diff of branches" to belong to "diff family" of
> >> commands (just operating on branches, instead of "branch" command
> >> knowing to "diff itself"), and I see we already have `diff-files`,
> >> `diff-index` and `diff-tree`, for what that`s worth.
> >>
> >> Heck, I might even expect something like `git diff --branch ...` to work,
> >> but I guess that is yet a different matter :)
> >>
> >> Thanks, Buga
> >
> > I like diff-branch, though I suppose that also conflicts with diff too.
>
> How about interdiff?
No. An interdiff is well defined as the diff you would get by first
applying the first of two patches in reverse and then the second patch
forward. In other words, it turns two revisions of a patch into the diff
between the result of applying both revisions.
I tried very hard to avoid using that term in my patch series (tbdiff used
the term incorrectly: what it called an interdiff is a diff of two
patches, where a patch is an author line followed by the commit message
followed by the commit diff).
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 1:05 ` Igor Djordjevic
2018-05-06 4:53 ` Jacob Keller@ 2018-05-06 12:10 ` Johannes Schindelin
2018-05-06 13:37 ` Igor Djordjevic1 sibling, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-06 12:10 UTC (permalink / raw)
To: Igor Djordjevic
Cc: Jeff King, git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Buga,
On Sun, 6 May 2018, Igor Djordjevic wrote:
> On 05/05/2018 23:57, Johannes Schindelin wrote:
> >
> > > > This builtin does not do a whole lot so far, apart from showing a
> > > > usage that is oddly similar to that of `git tbdiff`. And for a
> > > > good reason: the next commits will turn `branch-diff` into a
> > > > full-blown replacement for `tbdiff`.
> > >
> > > One minor point about the name: will it become annoying as a tab
> > > completion conflict with git-branch?
> >
> > I did mention this in the commit message of 18/18:
> >
> > Without this patch, we would only complete the `branch-diff` part but
> > not the options and other arguments.
> >
> > This of itself may already be slightly disruptive for well-trained
> > fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
> > `git branch origin/master`, as we now no longer automatically append a
> > space after completing `git branch`: this is now ambiguous.
> >
> > > It feels really petty complaining about the name, but I just want
> > > to raise the point, since it will never be easier to change than
> > > right now.
> >
> > I do hear you. Especially since I hate `git cherry` every single
> > time that I try to tab-complete `git cherry-pick`.
> >
> > > (And no, I don't really have another name in mind; I'm just
> > > wondering if "subset" names like this might be a mild annoyance in
> > > the long run).
> >
> > They totally are, and if you can come up with a better name, I am
> > really interested in changing it before this hits `next`, even.
>
> I gave this just a quick glance so might be I`m missing something
> obvious or otherwise well-known here, bur why not `diff-branch` instead?
I think that is just turning the problem from `branch` to `diff`.
Of course, we have precedent with diff-index and diff-files. Except that
they don't auto-complete (because they are low-level commands) and I
*would* like the subcommand discussed in this here patch series to
auto-complete.
I think Todd's idea to shift it from a full-blown builtin to a cmdmode
of `branch` makes tons of sense.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 05/18] branch-diff: also show the diff between patches
2018-05-06 1:14 ` Igor Djordjevic@ 2018-05-06 12:18 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-06 12:18 UTC (permalink / raw)
To: Igor Djordjevic
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Buga,
On Sun, 6 May 2018, Igor Djordjevic wrote:
> On 04/05/2018 17:34, Johannes Schindelin wrote:
> > Just like tbdiff, we now show the diff between matching patches. This is
> > a "diff of two diffs", so it can be a bit daunting to read for the
> > beginner.
> >
> > And just like tbdiff, we now also accept the `--no-patches` option
> > (which is actually equivalent to the diff option `-s`).
>
> A quick nit - would `--no-patch` (singular form) option name be more
> aligned with diff `-s` option it resembles?
The reason I used `--no-patches` is that tbdiff called it that way.
But you're right, the functionality is already available via -s, and we
*do* make this a distinct thing from tbdiff. So I'll simply drop support
for --no-patches.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 2:33 ` Junio C Hamano@ 2018-05-06 12:21 ` Johannes Schindelin
2018-05-06 20:51 ` Eric Sunshine
2018-05-07 1:45 ` Junio C Hamano0 siblings, 2 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-06 12:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Junio,
On Sun, 6 May 2018, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > On Sat, 5 May 2018, Jeff King wrote:
> >
> >> On Fri, May 04, 2018 at 05:34:32PM +0200, Johannes Schindelin wrote:
> >>
> >> > This builtin does not do a whole lot so far, apart from showing a usage
> >> > that is oddly similar to that of `git tbdiff`. And for a good reason:
> >> > the next commits will turn `branch-diff` into a full-blown replacement
> >> > for `tbdiff`.
> >>
> >> One minor point about the name: will it become annoying as a tab
> >> completion conflict with git-branch?
>
> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-)
> but I think the 't' in there stands for "topic", not "Thomas's".
>
> How about "git topic-diff"?
Or `git topic-branch-diff`?
But then, we do not really use the term `topic branch` a lot in Git, *and*
the operation in question is not really about showing differences between
topic branches, but between revisions of topic branches.
So far, the solution I like best is to use `git branch --diff <...>`,
which also neatly side-steps the problem of cluttering the top-level
command list (because tab completion).
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 12:10 ` Johannes Schindelin@ 2018-05-06 13:37 ` Igor Djordjevic
2018-05-07 1:34 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Igor Djordjevic @ 2018-05-06 13:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Dscho,
On 06/05/2018 14:10, Johannes Schindelin wrote:
>
> > > > > This builtin does not do a whole lot so far, apart from showing a
> > > > > usage that is oddly similar to that of `git tbdiff`. And for a
> > > > > good reason: the next commits will turn `branch-diff` into a
> > > > > full-blown replacement for `tbdiff`.
> > > >
> > > > One minor point about the name: will it become annoying as a tab
> > > > completion conflict with git-branch?
> > >
> > > I did mention this in the commit message of 18/18:
> > >
> > > Without this patch, we would only complete the `branch-diff` part but
> > > not the options and other arguments.
> > >
> > > This of itself may already be slightly disruptive for well-trained
> > > fingers that assume that `git bra<TAB>ori<TAB>mas<TAB>` would expand to
> > > `git branch origin/master`, as we now no longer automatically append a
> > > space after completing `git branch`: this is now ambiguous.
> > >
> > > > It feels really petty complaining about the name, but I just want
> > > > to raise the point, since it will never be easier to change than
> > > > right now.
> > >
> > > I do hear you. Especially since I hate `git cherry` every single
> > > time that I try to tab-complete `git cherry-pick`.
> > >
> > > > (And no, I don't really have another name in mind; I'm just
> > > > wondering if "subset" names like this might be a mild annoyance in
> > > > the long run).
> > >
> > > They totally are, and if you can come up with a better name, I am
> > > really interested in changing it before this hits `next`, even.
> >
> > I gave this just a quick glance so might be I`m missing something
> > obvious or otherwise well-known here, bur why not `diff-branch` instead?
>
> I think that is just turning the problem from `branch` to `diff`.
>
> Of course, we have precedent with diff-index and diff-files. Except that
> they don't auto-complete (because they are low-level commands) and I
> *would* like the subcommand discussed in this here patch series to
> auto-complete.
Yeah, I did suspect it might be something like this (those other ones
not auto-completing, where we do want it here), thanks for elaborating.
> I think Todd's idea to shift it from a full-blown builtin to a cmdmode
> of `branch` makes tons of sense.
I don`t know, I still find it a bit strange that in order to "diff
something", you go to "something" and tell it to "diff itself" - not
because it`s a weird concept (OOP, anyone? :]), but because we
already have "diff" command that can accept different things, thus
just teaching it to accept additional "something" (branch, in this
case), seems more natural (to me) - "branch diff" being just another
"diff" mode of operation.
What about that side thought you left out from my original message,
making it `git diff --branch` instead?
But if "branch diff" is considered to be too special-cased mode of
"diff" so that supporting it from `diff` itself would make it feel
awkward in both usage and maintenance (in terms of many other regular
`diff` specific options being unsupported), I guess I would understand
having it outside `diff` altogether (and implemented as proposed `git
branch --diff`, or something)... for the time being, at least :)
Regards, Buga
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 13:37 ` Igor Djordjevic@ 2018-05-07 1:34 ` Johannes Schindelin
2018-05-07 22:05 ` Igor Djordjevic0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-07 1:34 UTC (permalink / raw)
To: Igor Djordjevic
Cc: Jeff King, git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Buga,
On Sun, 6 May 2018, Igor Djordjevic wrote:
> On 06/05/2018 14:10, Johannes Schindelin wrote:
>
> > I think Todd's idea to shift it from a full-blown builtin to a cmdmode
> > of `branch` makes tons of sense.
>
> I don`t know, I still find it a bit strange that in order to "diff
> something", you go to "something" and tell it to "diff itself" - not
> because it`s a weird concept (OOP, anyone? :]), but because we already
> have "diff" command that can accept different things, thus just teaching
> it to accept additional "something" (branch, in this case), seems more
> natural (to me) - "branch diff" being just another "diff" mode of
> operation.
You also have to call `git branch` to list branches. And to rename
branches. And to delete them. So why not also compare them at the same
time?
> What about that side thought you left out from my original message,
> making it `git diff --branch` instead?
I really did not like this, as all of the `git diff` options really are
about comparing two revisions, not two *sets* of revisions.
Further, if I put my unsuspecting user hat on, I would ask myself how you
can compare branches with one another? That is what I would expect `git
diff --branch` to do, not to compare two versions of *the same* branch.
So `git diff --branch` does not at all convey the same to me as `git
branch --diff`, and I find that the latter does match better what this
patch series tries to achieve.
I briefly considered `git branch --compare` instead, but then rejected it:
it would again sound more like I try to compare two separate (and likely
unrelated) branches with one another, and that simply does not make much
sense, and tbdiff would not help with that, anyway.
> But if "branch diff" is considered to be too special-cased mode of
> "diff" so that supporting it from `diff` itself would make it feel
> awkward in both usage and maintenance (in terms of many other regular
> `diff` specific options being unsupported), I guess I would understand
> having it outside `diff` altogether (and implemented as proposed `git
> branch --diff`, or something)... for the time being, at least :)
The branch diff is not even a special-cased mode of diff. It is *way* more
complicated than that. It tries to find 1:1 correspondences between *sets*
of commits, and then only outputs a "sort" of a diff between the commits
that correspond with each other. I say "sort" of a diff because that diff
does not look like `git diff <commit1> <commit2>` at all!
So I think it would just be confusing to add that mode to `git diff`.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 13/18] color: provide inverted colors, too
2018-05-06 6:41 ` Jeff King
2018-05-07 1:20 ` Johannes Schindelin@ 2018-05-07 1:35 ` Junio C Hamano
2018-05-07 5:38 ` Johannes Schindelin
2018-05-07 7:40 ` Jeff King1 sibling, 2 replies; 387+ messages in thread
From: Junio C Hamano @ 2018-05-07 1:35 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Jeff King <peff@peff.net> writes:
> On Sun, May 06, 2018 at 02:35:44AM -0400, Jeff King wrote:
>
>> You'd have to introduce GIT_COLOR_REVERSE. I don't think we have a
>> constant for it yet, but it's \x[7m.
>
> Heh, of course you knew that already, as I just noticed your patch is
> using the reverse attribute internally (I had thought at first glance
> you were just specifying the background independently).
I somehow suspected as such, but I also thought so and reacted "what
about us whose terminal is black-on-white unlike most others?",
before looking up what 7 meant ;-)
> So really, I guess all I am arguing for is having GIT_COLOR_INV (or
> REVERSE) as a constant, and then teaching the code to combine it with
> the existing "new" color. It's perfectly OK to have:
>
> \x1b[7m\x1b[36m
>
> instead of:
>
> \x1b[7;36m
>
> It's two extra bytes, but I doubt anybody cares.
I do not think two extra bytes will be missed, but it was not
immediately obvious to me how much flexibility or simplicity weu are
gaining by combining values from multiple configuration variables.
With a "letters on a new line is painted with ${new}, in addition,
the leading plus is further annotated with ${tbdiffNew}" (similarly
to "old") scheme, the user can take advantage of the fact that there
is no ${reset} between ${new} and ${tbdiffNew} and set tbdiffNew and
tbdiffOld to a same value (that does not change the color but
changes some other aspect of the appearance, like "reverse" or
"underline"). Since only pre-designed combination can be used (your
example works only because you chose to allow combination by
annotating the leading "+" with ${new}${tbdiffNew}), we'd need to
(1) establish a convention to paint things with similar meanings in
the same color, modifyable by individual command (e.g. you could say
anything new is by default green with "color.new=green", and then
"color.frotz.new=blink" "color.status.new=" "color.diff.new=blue"
would make frotz, status and diff subcommands to show new things in
blinking green, normal green, and blue), and (2) push the codebase
to adopt such color combination as a preferred design pattern if we
want the resulting system to be useful.
I guess you are getting simpler configuration, which is a big plus,
but to make a truly useful combining convention, we'd need to
rethink and find a way to transition existing configurations to the
new world, which may not be feasible.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 12:21 ` Johannes Schindelin
2018-05-06 20:51 ` Eric Sunshine@ 2018-05-07 1:45 ` Junio C Hamano
2018-05-07 5:39 ` Johannes Schindelin1 sibling, 1 reply; 387+ messages in thread
From: Junio C Hamano @ 2018-05-07 1:45 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-)
>> but I think the 't' in there stands for "topic", not "Thomas's".
>>
>> How about "git topic-diff"?
>
> Or `git topic-branch-diff`?
Yeah something along that line, which is about comparing each step
in two iterations of a single topic. It would be wonderful if it
also supported a short-hand
$ git tbdiff --reflog 1.day.ago js/branch-diff
that turned into:
$ git tbdiff js/branch-diff..js/branch-diff@{1.day.ago} \
js/branch-diff@{1.day.ago}..js/branch-diff
That compares "what was on the topic a day ago" with "what is new on
the topic since that time", which is exactly what an individual
contributor wants when reviewing how the topic was polished, I would
say.
[Footnote]
A variant I often use when accepting a rerolled series is
$ git checkout js/branch-diff
$ git checkout master...
$ git am ./+js-branch-diff-v2
$ git tbdiff ..@{-1} @{-1}..
so this is not only for individual contributors but also helps
integrators.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 12/18] branch-diff: use color for the commit pairs
2018-05-05 23:48 ` Todd Zullinger@ 2018-05-07 1:52 ` Johannes Schindelin
2018-05-08 2:10 ` Todd Zullinger0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-07 1:52 UTC (permalink / raw)
To: Todd Zullinger
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Todd,
On Sat, 5 May 2018, Todd Zullinger wrote:
> > @@ -430,6 +451,8 @@ int cmd_branch_diff(int argc, const char **argv, const char *prefix)
> > struct string_list branch1 = STRING_LIST_INIT_DUP;
> > struct string_list branch2 = STRING_LIST_INIT_DUP;
> >
> > + git_diff_basic_config("diff.color.frag", "magenta", NULL);
> > +
> > diff_setup(&diffopt);
> > diffopt.output_format = DIFF_FORMAT_PATCH;
> > diffopt.flags.suppress_diff_headers = 1;
>
> Should this also (or only) check color.diff.frag?
This code is not querying diff.color.frag, it is setting it. Without
any way to override it.
Having thought about it longer, and triggered by Peff's suggestion to
decouple the "reverse" part from the actual color, I fixed this by
- *not* setting .frag to magenta,
- using the reverse method also to mark outer *hunk headers* (not only the
outer -/+ markers).
- actually calling git_diff_ui_config()...
> I thought that color.diff.* was preferred over diff.color.*, though
> that doesn't seem to be entirely true in all parts of the current
> codebase.
>
> In testing this series it seems that setting color.diff
> options to change the various colors read earlier in this
> patch via diff_get_color_opt, as well as the 'frag' slot,
> are ignored. Setting them via diff.color.<slot> does work.
In my tests, it did not even work via diff.color.<slot>. But I think I
fixed this (at least my local testing confirms this) by calling
git_diff_ui_config().
> The later patch adding a man page documents branch-diff as
> using `diff.color.*` and points to git-config(1), but the
> config docs only list color.diff.
In the current form (`git branch --diff`), I refrained from going into
*so* much detail ;-) But the gist still holds, and now the code should
support it, too.
The current work in progress can be pulled as `branch-diff` from
https://github.com/dscho/git, if I could ask you to test?
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-06 20:51 ` Eric Sunshine@ 2018-05-07 2:04 ` Johannes Schindelin
2018-05-07 7:48 ` Jeff King0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-07 2:04 UTC (permalink / raw)
To: Eric Sunshine
Cc: Junio C Hamano, Jeff King, Git List, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Stefan Beller, Jacob Keller
Hi Eric,
On Sun, 6 May 2018, Eric Sunshine wrote:
> On Sun, May 6, 2018 at 8:21 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> > On Sun, 6 May 2018, Junio C Hamano wrote:
> >> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> >> > On Sat, 5 May 2018, Jeff King wrote:
> >> >> One minor point about the name: will it become annoying as a tab
> >> >> completion conflict with git-branch?
> >>
> >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-)
> >> but I think the 't' in there stands for "topic", not "Thomas's".
> >> How about "git topic-diff"?
> >
> > Or `git topic-branch-diff`?
> >
> > But then, we do not really use the term `topic branch` a lot in Git, *and*
> > the operation in question is not really about showing differences between
> > topic branches, but between revisions of topic branches.
> >
> > So far, the solution I like best is to use `git branch --diff <...>`,
> > which also neatly side-steps the problem of cluttering the top-level
> > command list (because tab completion).
>
> Let's, please, not fall into the trap of polluting git-branch with
> utterly unrelated functionality, as has happened a few times with
> other Git commands. Let's especially not do so merely for the sake of
> tab-completion. git-branch is for branch management; it's not for
> diff'ing.
I totally disagree. `git branch` is *the* command to work with branches.
Yes, you can manage branches. But you can also list them. And now you can
also compare them.
> Of the suggestions thus far, Junio's git-topic-diff seems the least
> worse, and doesn't suffer from tab-completion problems.
Except that this is too limited a view.
Have you seen one of the more important tidbits in the cover letter, the
one about Git for Windows' *branch thicket*? In this case, it is not *one*
topic branch that we are talking about.
And even worse: what this patch series introduces is not at all a feature
to compare topic branches!
Instead, it is a way to compare iterations of patch series, versions of
topic branches, changes introduced into a topic branch by rebasing it,
etc. And `git topic-diff` simply does not say this. It says something
different, something that my patches cannot fulfill.
> Building on Duy's suggestion: git-interdiff could be a superset of the
> current git-branch-diff:
>
> # standard interdiff
> git interdiff womp-v1 womp-v2
> # 'tbdiff'-like output
> git interdiff --topic womp-v1 womp-v2
No, no, and no. An interdiff is an interdiff is an interdiff. See e.g.
https://www.tutorialspoint.com/unix_commands/interdiff.htm for details.
The operation introduced by this patch series, or for that matter tbdiff,
*never ever* produced an interdiff. Get this "interdiff" label out of your
mind immediately when you think about this here operation.
One of my commit messages even talks about this, and says *why* we do not
generate interdiffs: they are in general not even well-defined.
Take my --rebase-merges patch series, for example. It is so long-running
that at some stages, all I did was to resolve merge conflicts incurred
from rebasing to `master`. That was literally all. Now, if you tried to
produce an interdiff, you would *already fail in the first step*, as the
previous overall diff does not apply in reverse on current `master`.
Out of all the options so far, the one that I liked was `git branch
--diff`. Seriously. I do not understand why you think that this is abusing
the `git branch` command. It is no less abusing it than `git branch
--edit-description`! And that is a *very good* command, and it is *very
good* that it is an option to `git branch`. It makes a total lot of sense,
I have never had to think "wait, in which Git command is this implemented
already?" And I would expect the exact same thing to happen with `git
branch --diff`.
Ciao,
Johannes
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-06 22:56 ` brian m. carlson@ 2018-05-07 2:05 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-07 2:05 UTC (permalink / raw)
To: brian m. carlson
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
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
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 13/18] color: provide inverted colors, too
2018-05-07 1:35 ` Junio C Hamano@ 2018-05-07 5:38 ` Johannes Schindelin
2018-05-07 7:40 ` Jeff King1 sibling, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-07 5:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Junio,
On Mon, 7 May 2018, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > So really, I guess all I am arguing for is having GIT_COLOR_INV (or
> > REVERSE) as a constant, and then teaching the code to combine it with
> > the existing "new" color. It's perfectly OK to have:
> >
> > \x1b[7m\x1b[36m
> >
> > instead of:
> >
> > \x1b[7;36m
> >
> > It's two extra bytes, but I doubt anybody cares.
>
> I do not think two extra bytes will be missed, but it was not
> immediately obvious to me how much flexibility or simplicity weu are
> gaining by combining values from multiple configuration variables.
> With a "letters on a new line is painted with ${new}, in addition,
> the leading plus is further annotated with ${tbdiffNew}" (similarly
> to "old") scheme, the user can take advantage of the fact that there
> is no ${reset} between ${new} and ${tbdiffNew} and set tbdiffNew and
> tbdiffOld to a same value (that does not change the color but
> changes some other aspect of the appearance, like "reverse" or
> "underline"). Since only pre-designed combination can be used (your
> example works only because you chose to allow combination by
> annotating the leading "+" with ${new}${tbdiffNew}), we'd need to
> (1) establish a convention to paint things with similar meanings in
> the same color, modifyable by individual command (e.g. you could say
> anything new is by default green with "color.new=green", and then
> "color.frotz.new=blink" "color.status.new=" "color.diff.new=blue"
> would make frotz, status and diff subcommands to show new things in
> blinking green, normal green, and blue), and (2) push the codebase
> to adopt such color combination as a preferred design pattern if we
> want the resulting system to be useful.
>
> I guess you are getting simpler configuration, which is a big plus,
> but to make a truly useful combining convention, we'd need to
> rethink and find a way to transition existing configurations to the
> new world, which may not be feasible.
I really do not like the sound of that much complexity. It strikes me as
yet another instance of Yer Ain't Gonna Need It. In *particular* because
nested diffs are a special thing: you *already* get overwhelmed with
too much information, and adding colors to the fray won't help.
What does help is to keep the colors, so that they can mean the same thing
in inner vs outer diffs, but reverse foreground and background to make the
outer diff "stick out more".
Should my assessment be wrong, I think it'll still be relatively easy to
add support for config settings, *then*, not before we know it is needed.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 1:45 ` Junio C Hamano@ 2018-05-07 5:39 ` Johannes Schindelin
2018-05-07 15:12 ` Junio C Hamano0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-07 5:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Junio,
On Mon, 7 May 2018, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> If tbdiff were "Thomas's branch diff", I would call this jbdiff ;-)
> >> but I think the 't' in there stands for "topic", not "Thomas's".
> >>
> >> How about "git topic-diff"?
> >
> > Or `git topic-branch-diff`?
>
> Yeah something along that line, which is about comparing each step
> in two iterations of a single topic. It would be wonderful if it
> also supported a short-hand
>
> $ git tbdiff --reflog 1.day.ago js/branch-diff
>
> that turned into:
>
> $ git tbdiff js/branch-diff..js/branch-diff@{1.day.ago} \
> js/branch-diff@{1.day.ago}..js/branch-diff
Or even easier: `git tbdiff js/branch-diff@{1.day.ago}...js/branch-diff`.
> That compares "what was on the topic a day ago" with "what is new on
> the topic since that time", which is exactly what an individual
> contributor wants when reviewing how the topic was polished, I would
> say.
It would be easy to introduce, but I am wary about its usefulness.
Unless you re-generate the branch from patches (which I guess you do a
lot, but I don't), you are likely to compare incomplete patch series: say,
when you call `git rebase -i` to reword 05/18's commit message, your
command will only compare 05--18 of the patch series.
Worse, if js/branch-diff needs to be uprooted (e.g. because it now depends
on some different patch, or because it already depended on a separate
patch series that was now updated), your `git branch --diff` call will
compare more than just my patches: it will assume that those dependencies
are part of the patch series, because they changed, too.
> [Footnote]
>
> A variant I often use when accepting a rerolled series is
>
> $ git checkout js/branch-diff
> $ git checkout master...
> $ git am ./+js-branch-diff-v2
> $ git tbdiff ..@{-1} @{-1}..
>
> so this is not only for individual contributors but also helps
> integrators.
Yes, and I also pointed out (twice) that it will help interested parties
follow what I do with my merging-rebases in Git for Windows.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 13/18] color: provide inverted colors, too
2018-05-07 1:20 ` Johannes Schindelin@ 2018-05-07 7:37 ` Jeff King0 siblings, 0 replies; 387+ messages in thread
From: Jeff King @ 2018-05-07 7:37 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
On Sun, May 06, 2018 at 09:20:46PM -0400, Johannes Schindelin wrote:
> > Heh, of course you knew that already, as I just noticed your patch is
> > using the reverse attribute internally (I had thought at first glance
> > you were just specifying the background independently).
> >
> > So really, I guess all I am arguing for is having GIT_COLOR_INV (or
> > REVERSE) as a constant, and then teaching the code to combine it with
> > the existing "new" color. It's perfectly OK to have:
> >
> > \x1b[7m\x1b[36m
> >
> > instead of:
> >
> > \x1b[7;36m
> >
> > It's two extra bytes, but I doubt anybody cares.
>
> Yep, I agree that it is a small price to pay for the benefit of simply
> using the reverse of diff.color.old (and .new).
>
> While at it, I also changed the hunk header colors: they are *also* simply
> the same ones, with the outer one having background and foreground
> reversed.
That sound sane.
If we ever did want to care about the number of bytes we output, I
suspect we could "compress" our ANSI terminal outputs by collapsing
adjacent colors into a single one. But IMHO it's not even worth worrying
about that optimization at this point.
-Peff
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 13/18] color: provide inverted colors, too
2018-05-07 1:35 ` Junio C Hamano
2018-05-07 5:38 ` Johannes Schindelin@ 2018-05-07 7:40 ` Jeff King1 sibling, 0 replies; 387+ messages in thread
From: Jeff King @ 2018-05-07 7:40 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
On Mon, May 07, 2018 at 10:35:53AM +0900, Junio C Hamano wrote:
> > So really, I guess all I am arguing for is having GIT_COLOR_INV (or
> > REVERSE) as a constant, and then teaching the code to combine it with
> > the existing "new" color. It's perfectly OK to have:
> >
> > \x1b[7m\x1b[36m
> >
> > instead of:
> >
> > \x1b[7;36m
> >
> > It's two extra bytes, but I doubt anybody cares.
>
> I do not think two extra bytes will be missed, but it was not
> immediately obvious to me how much flexibility or simplicity weu are
> gaining by combining values from multiple configuration variables.
My goal was just to let you set color.diff.new to something besides
green without having to also manually set color.tbdiff.new (or whatever
it's called) to match.
> With a "letters on a new line is painted with ${new}, in addition,
> the leading plus is further annotated with ${tbdiffNew}" (similarly
> to "old") scheme, the user can take advantage of the fact that there
> is no ${reset} between ${new} and ${tbdiffNew} and set tbdiffNew and
> tbdiffOld to a same value (that does not change the color but
> changes some other aspect of the appearance, like "reverse" or
> "underline"). Since only pre-designed combination can be used (your
> example works only because you chose to allow combination by
> annotating the leading "+" with ${new}${tbdiffNew}), we'd need to
> (1) establish a convention to paint things with similar meanings in
> the same color, modifyable by individual command (e.g. you could say
> anything new is by default green with "color.new=green", and then
> "color.frotz.new=blink" "color.status.new=" "color.diff.new=blue"
> would make frotz, status and diff subcommands to show new things in
> blinking green, normal green, and blue), and (2) push the codebase
> to adopt such color combination as a preferred design pattern if we
> want the resulting system to be useful.
Right, this is basically making that "new" piggy-backing explicit, but
only for this one case.
> I guess you are getting simpler configuration, which is a big plus,
> but to make a truly useful combining convention, we'd need to
> rethink and find a way to transition existing configurations to the
> new world, which may not be feasible.
Yes, one could probably develop a whole theming system for Git. We've
resisted it so far. :)
-Peff
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 2:04 ` Johannes Schindelin@ 2018-05-07 7:48 ` Jeff King
2018-05-07 21:33 ` Igor Djordjevic
2018-05-08 0:30 ` Junio C Hamano0 siblings, 2 replies; 387+ messages in thread
From: Jeff King @ 2018-05-07 7:48 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Eric Sunshine, Junio C Hamano, Git List, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Stefan Beller, Jacob Keller
On Sun, May 06, 2018 at 10:04:31PM -0400, Johannes Schindelin wrote:
> > Let's, please, not fall into the trap of polluting git-branch with
> > utterly unrelated functionality, as has happened a few times with
> > other Git commands. Let's especially not do so merely for the sake of
> > tab-completion. git-branch is for branch management; it's not for
> > diff'ing.
>
> I totally disagree. `git branch` is *the* command to work with branches.
> Yes, you can manage branches. But you can also list them. And now you can
> also compare them.
One of the things I don't like about "git branch --diff" is that this
feature is not _just_ about branches at all. E.g., I could do:
git tbdiff HEAD~10 HEAD~5 foo
Or even:
git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0
Those arguments really are just commitishes, not necessarily branches.
One of the current interface rules for "git branch" is that the branch
names we hand it are interpreted _exactly_ as branch names. You cannot
"git branch -m v2.16.0", and there is no ambiguity in "git branch -d
foo" if "foo" is both a tag and a branch.
But this new mode does not fit the pattern at all.
If we were to attach this to an existing command, I think it has more to
do with "diff" than "branch". But I'm not sure we want to overload
"diff" either (which has traditionally been about two endpoints, and
does not really traverse at all, though arguably "foo...bar" is a bit of
a cheat :) ).
> > Of the suggestions thus far, Junio's git-topic-diff seems the least
> > worse, and doesn't suffer from tab-completion problems.
>
> Except that this is too limited a view.
Right, I agree with you. Topic branches are the intended use, but that's
not what it _does_, and obviously it can be applied in other cases. So
since "branch" is too specific, I think "topic branch" is even more so.
It's really "diff-history" or something, I think. That's not very
catchy, but I think the best name would imply that it was diffing a set
of commits (so even "diff-commit" would not be right, because that again
sounds like endpoints).
-Peff
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 5:39 ` Johannes Schindelin@ 2018-05-07 15:12 ` Junio C Hamano
2018-05-21 10:41 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Junio C Hamano @ 2018-05-07 15:12 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> It would be easy to introduce, but I am wary about its usefulness.
> Unless you re-generate the branch from patches (which I guess you do a
> lot, but I don't), you are likely to compare incomplete patch series: say,
> when you call `git rebase -i` to reword 05/18's commit message, your
> command will only compare 05--18 of the patch series.
Well that is exactly the point of that "..@{1} @{1}..", which turned
out to be very useful in practice at least for me when I am updating
a topic with "rebase -i", and then reviewing what I did with tbdiff.
I do not want 01-04 in the above case as I already know I did not
touch them.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 7:48 ` Jeff King@ 2018-05-07 21:33 ` Igor Djordjevic
2018-05-21 10:33 ` Johannes Schindelin
2018-05-08 0:30 ` Junio C Hamano1 sibling, 1 reply; 387+ messages in thread
From: Igor Djordjevic @ 2018-05-07 21:33 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin
Cc: Eric Sunshine, Junio C Hamano, Git List, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Stefan Beller, Jacob Keller
On 07/05/2018 09:48, Jeff King wrote:
>
> > > Let's, please, not fall into the trap of polluting git-branch with
> > > utterly unrelated functionality, as has happened a few times with
> > > other Git commands. Let's especially not do so merely for the sake of
> > > tab-completion. git-branch is for branch management; it's not for
> > > diff'ing.
> >
> > I totally disagree. `git branch` is *the* command to work with branches.
> > Yes, you can manage branches. But you can also list them. And now you can
> > also compare them.
>
> One of the things I don't like about "git branch --diff" is that this
> feature is not _just_ about branches at all. E.g., I could do:
>
> git tbdiff HEAD~10 HEAD~5 foo
>
> Or even:
>
> git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0
>
> Those arguments really are just commitishes, not necessarily branches.
> One of the current interface rules for "git branch" is that the branch
> names we hand it are interpreted _exactly_ as branch names. You cannot
> "git branch -m v2.16.0", and there is no ambiguity in "git branch -d
> foo" if "foo" is both a tag and a branch.
>
> But this new mode does not fit the pattern at all.
>
> If we were to attach this to an existing command, I think it has more to
> do with "diff" than "branch". But I'm not sure we want to overload
> "diff" either (which has traditionally been about two endpoints, and
> does not really traverse at all, though arguably "foo...bar" is a bit of
> a cheat :) ).
>
> > > Of the suggestions thus far, Junio's git-topic-diff seems the least
> > > worse, and doesn't suffer from tab-completion problems.
> >
> > Except that this is too limited a view.
>
> Right, I agree with you. Topic branches are the intended use, but that's
> not what it _does_, and obviously it can be applied in other cases. So
> since "branch" is too specific, I think "topic branch" is even more so.
>
> It's really "diff-history" or something, I think. That's not very
> catchy, but I think the best name would imply that it was diffing a set
> of commits (so even "diff-commit" would not be right, because that again
> sounds like endpoints).
This is exactly what I feel as well, thanks for concise and
to-the-point spelling out.
From user interface perspective, I would expect something like this
to be possible (and natural):
(1) git diff topic-v1...topic-v2
(2) git diff --branch topic-v1...topic-v2
(1) is what we are all familiar with, providing a diff between two
revisions with focus on file changes, where (2) shifts focus to
history changes.
It`s all still a comparison between two revisions (pointed to by
"topic-v1" and "topic-v2" branch heads in this specific example), but
it differs in what we are comparing - (1) set of files contained in
endpoints, or (2) set of revisions contained in (or "leading to")
endpoints.
Hmm... what about `git diff --history`? :/ It does seem more "true"
to what it does, though I still like `git diff --branch` more
(catchier, indeed).
Regards, Buga
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 1:34 ` Johannes Schindelin@ 2018-05-07 22:05 ` Igor Djordjevic
2018-05-07 22:24 ` Stefan Beller0 siblings, 1 reply; 387+ messages in thread
From: Igor Djordjevic @ 2018-05-07 22:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Dscho,
On 07/05/2018 03:34, Johannes Schindelin wrote:
>
> > > I think Todd's idea to shift it from a full-blown builtin to a cmdmode
> > > of `branch` makes tons of sense.
> >
> > I don`t know, I still find it a bit strange that in order to "diff
> > something", you go to "something" and tell it to "diff itself" - not
> > because it`s a weird concept (OOP, anyone? :]), but because we already
> > have "diff" command that can accept different things, thus just teaching
> > it to accept additional "something" (branch, in this case), seems more
> > natural (to me) - "branch diff" being just another "diff" mode of
> > operation.
>
> You also have to call `git branch` to list branches. And to rename
> branches. And to delete them. So why not also compare them at the same
> time?
Maybe because we already have a command that specifically does
comparison? :)
List, rename, delete -- all these seem more as basic CRUD operations,
where comparison is a more complex one. And not to get me wrong - I
could see "branch diff" being part of "branch", but not really when
"diff" already exists as a separate thing, already doing quite some
(but still diff related, and configurable) stuff.
> > What about that side thought you left out from my original message,
> > making it `git diff --branch` instead?
>
> I really did not like this, as all of the `git diff` options really are
> about comparing two revisions, not two *sets* of revisions.
I see what you mean, but I would argue this being a deliberate user
choice here, like picking a diff "strategy" - I`d say it still utterly
does compare two revisions (branch tips, in this case), just putting
focus on comparing revisions that lead to them (branch history),
instead of just files found in them (branch files).
> Further, if I put my unsuspecting user hat on, I would ask myself how you
> can compare branches with one another? That is what I would expect `git
> diff --branch` to do, not to compare two versions of *the same* branch.
I totally agree with you here, and thus I have a question - what
determines "two versions of *the same* branch"? :) Do you still
explicitly provide both "old" and "new" version branch tips?
I see "multiple versions of the same branch" more as a conceptual
model, and not something Git is aware of (I think?) - BUT, even if it
was, I don`t see why this should be a(n artificial) restriction?
Basically, what you (conceptually) call "two versions of the same
branch", I simply call "two branches" (from usage standpoint).
And you may have a branch that got split, or more of them that got
unified, so defining "previous branch version" may not be that
straightforward - it`s really just "two commit ranges" (as man page
defines it in general), with "two versions of a patch series" only
being the most common/expected use case of the former.
Finally, if user picks two totally unrelated "branches" to compare,
he won`t get a really useful diff - but it`s the same as if he would
compare two totally unrelated commits (where tree state massively
changed in between, or having unrelated histories, even).
Besides, while I might still not be much into the matter, but isn`t
"branch" in Git just a pointer to revision? Being so, there is really
no such thing as "branch" in terms of being a specific (sub)set of
revisions (commits), other then "everything from branch head/pointer
to root commit" (in general).
Yes, we do perceive "a branch" being a specific set of topic related
commits, but which *exact* commits we are interested in ("branch" lower
bounds) may differ in regards to what we aim for - how far do we consider
one branch to reach in the past depends solely on the use case.
> So `git diff --branch` does not at all convey the same to me as `git
> branch --diff`, and I find that the latter does match better what this
> patch series tries to achieve.
I agree with the first part, but it seems to me your finding is
biased due to your (expected) use case.
> > But if "branch diff" is considered to be too special-cased mode of
> > "diff" so that supporting it from `diff` itself would make it feel
> > awkward in both usage and maintenance (in terms of many other regular
> > `diff` specific options being unsupported), I guess I would understand
> > having it outside `diff` altogether (and implemented as proposed `git
> > branch --diff`, or something)... for the time being, at least :)
>
> The branch diff is not even a special-cased mode of diff. It is *way* more
> complicated than that. It tries to find 1:1 correspondences between *sets*
> of commits, and then only outputs a "sort" of a diff between the commits
> that correspond with each other. I say "sort" of a diff because that diff
> does not look like `git diff <commit1> <commit2>` at all!
But there is not only one `git diff <commit1> <commit2>` looks, it
depends on other options (like --name-status, for example), which is
my point exactly :)
With something like `git diff --branch <commit1>...<commit2>` you
would get yet another "diff look", useful for use case in question
here.
Regards, Buga
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 22:05 ` Igor Djordjevic@ 2018-05-07 22:24 ` Stefan Beller
2018-05-07 23:39 ` Igor Djordjevic
2018-05-08 3:44 ` Jeff King0 siblings, 2 replies; 387+ messages in thread
From: Stefan Beller @ 2018-05-07 22:24 UTC (permalink / raw)
To: Igor Djordjevic
Cc: Johannes Schindelin, Jeff King, git, Junio C Hamano, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Jacob Keller, Eric Sunshine
On Mon, May 7, 2018 at 3:05 PM, Igor Djordjevic
<igor.d.djordjevic@gmail.com> wrote:
> List, rename, delete -- all these seem more as basic CRUD operations,
> where comparison is a more complex one. And not to get me wrong - I
> could see "branch diff" being part of "branch", but not really when
> "diff" already exists as a separate thing, already doing quite some
> (but still diff related, and configurable) stuff.
If we go with "branch --diff", because it has the CRUD operations already
there for branches, I might ask for "remote --diff" to diff two remotes. ;)
(That command "remote --diff" would not make any sense, would it?)
> Basically, what you (conceptually) call "two versions of the same
> branch", I simply call "two branches" (from usage standpoint).
If I diff 2 (topic) branches, which are based on a different version
from upstream, then I see changes from commits that I don't care
about, but this tool explicitly excludes them. Instead it includes
the ordering of the commits as well as its commit messages to
the diff.
So I would not say this tool "diffs two branches", as that is understood
as "diffing the trees, where each of the two branches points two",
whereas this tool diffs a patch series, or if you give Git-ranges,
then it would produce such a patch series in memory.
> And you may have a branch that got split, or more of them that got
> unified, so defining "previous branch version" may not be that
> straightforward - it`s really just "two commit ranges" (as man page
> defines it in general), with "two versions of a patch series" only
> being the most common/expected use case of the former.
>
> Finally, if user picks two totally unrelated "branches" to compare,
> he won`t get a really useful diff - but it`s the same as if he would
> compare two totally unrelated commits (where tree state massively
> changed in between, or having unrelated histories, even).
I used just that, but narrowed down the comparison to one file
instead of the whole tree.
> With something like `git diff --branch <commit1>...<commit2>` you
> would get yet another "diff look", useful for use case in question
> here.
Personally I think this patch series should neither extend git-diff
nor git-branch.
It should not extend git-diff, because currently git-diff can diff
tree-ishs (and does that very well) and comparing to
worktree/index.
It should also not extend git-branch, as that command is for
CRUD operations that you hinted at earlier (Earlier I proposed
git-remote --diff for diffing two remote, which makes no sense,
another one might be git-worktree, which also just does CRUD
for worktrees. It would be a bad idea to have "git worktree --diff")
Hence I propose "git range-diff", similar to topic-diff, that
was proposed earlier.
* it "diffs ranges" of commits.
* it can also deal with out-of-git things like patch series,
but that is a mere by product and may not be desired.
Just like git-diff can also compare two files outside a git
repo, that would not be a good use case.
Keep the name Git-centric!
* it autocompletes well.
Stefan
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 22:24 ` Stefan Beller@ 2018-05-07 23:39 ` Igor Djordjevic
2018-05-08 3:44 ` Jeff King1 sibling, 0 replies; 387+ messages in thread
From: Igor Djordjevic @ 2018-05-07 23:39 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, Jeff King, git, Junio C Hamano, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Jacob Keller, Eric Sunshine
Hi Stefan,
On 08/05/2018 00:24, Stefan Beller wrote:
>
> > List, rename, delete -- all these seem more as basic CRUD operations,
> > where comparison is a more complex one. And not to get me wrong - I
> > could see "branch diff" being part of "branch", but not really when
> > "diff" already exists as a separate thing, already doing quite some
> > (but still diff related, and configurable) stuff.
>
> If we go with "branch --diff", because it has the CRUD operations already
> there for branches, I might ask for "remote --diff" to diff two remotes. ;)
> (That command "remote --diff" would not make any sense, would it?)
I`m not sure if this is a reply to me or in general, and whether you
support what I sad, or argue against it...? Because what you`re
saying was (or at least should have been) my exact point there :)
> > Basically, what you (conceptually) call "two versions of the same
> > branch", I simply call "two branches" (from usage standpoint).
>
> If I diff 2 (topic) branches, which are based on a different version
> from upstream, then I see changes from commits that I don't care
> about, but this tool explicitly excludes them. Instead it includes
> the ordering of the commits as well as its commit messages to
> the diff.
Here, I was merely pointing out that you still need to provide two
branch heads - which might be expected to resemble "two versions of
the same topic", but they are still (just) "two branches" in Git world.
> > And you may have a branch that got split, or more of them that got
> > unified, so defining "previous branch version" may not be that
> > straightforward - it`s really just "two commit ranges" (as man page
> > defines it in general), with "two versions of a patch series" only
> > being the most common/expected use case of the former.
> >
> > Finally, if user picks two totally unrelated "branches" to compare,
> > he won`t get a really useful diff - but it`s the same as if he would
> > compare two totally unrelated commits (where tree state massively
> > changed in between, or having unrelated histories, even).
>
> I used just that, but narrowed down the comparison to one file
> instead of the whole tree.
Again, not sure if this should support the argument, or argue against
it? :) My point was that there might be other use cases (as you seem
to have supported now), and as "diff" is pretty forgiving, might be
"diff branch" should be as well.
> > With something like `git diff --branch <commit1>...<commit2>` you
> > would get yet another "diff look", useful for use case in question
> > here.
>
> Personally I think this patch series should neither extend git-diff
> nor git-branch.
>
> It should not extend git-diff, because currently git-diff can diff
> tree-ishs (and does that very well) and comparing to
> worktree/index.
Hmm, are you saying that `git diff` actually has a too generic name
for its (more specific) purpose?
> It should also not extend git-branch, as that command is for
> CRUD operations that you hinted at earlier (Earlier I proposed
> git-remote --diff for diffing two remote, which makes no sense,
> another one might be git-worktree, which also just does CRUD
> for worktrees. It would be a bad idea to have "git worktree --diff")
Agreed here.
> Hence I propose "git range-diff", similar to topic-diff, that
> was proposed earlier.
I find it strange that we already have both "diff" and "diff-something"
commands, and yet you still propose "something-diff" naming pattern
instead (but I guess it`s mainly because of auto-complete concerns).
Please forgive my lack of code base familiarity, but from what I`ve
seen so far, and at least from end-user perspective, I may rather expect
`git diff-range` as low level implementation, and possibly exposed
through `git diff --range` (with a nice single letter abbreviation?).
> * it "diffs ranges" of commits.
Thus "diff-range", as your description says itself :) ("range-diff"
might sound like it "ranges diffs"...?)
> * it can also deal with out-of-git things like patch series,
> but that is a mere by product and may not be desired.
> Just like git-diff can also compare two files outside a git
> repo, that would not be a good use case.
Hmm, so still follows `git diff` in general... `git diff --range`? :D
> * it autocompletes well.
Only here I`m not sure if something like `git diff --range` (with
accompanying single letter option) would be considered "auto-complete
friendly", or not?
Regards, Buga
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 7:48 ` Jeff King
2018-05-07 21:33 ` Igor Djordjevic@ 2018-05-08 0:30 ` Junio C Hamano1 sibling, 0 replies; 387+ messages in thread
From: Junio C Hamano @ 2018-05-08 0:30 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Eric Sunshine, Git List, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Stefan Beller, Jacob Keller
Jeff King <peff@peff.net> writes:
> One of the things I don't like about "git branch --diff" is that this
> feature is not _just_ about branches at all.
I actually wouldn't be that much against the word "branch" in
"branch-diff" on the ground that we are typically not feeding
branches to the command (we are feeding two ranges, and one endpoint
of each range typically gets expressed using branch name), as we
have a precedent in "show-branch", for example, that often takes
branches but does not have to.
> It's really "diff-history" or something, I think. That's not very
> catchy, but I think the best name would imply that it was diffing a set
> of commits (so even "diff-commit" would not be right, because that again
> sounds like endpoints).
Sure. This should't be a submode "--diff" of "git branch" just like
it shouldn't be a submode of "git commit" only because it is about
comparing two sets of commits. "diff" is about comparing two
endpoints, and not about comparing two sets. "log" is the closest
thing, if we really want to coerce it into an existing set of
commands, as it is about a set of commits, but it does not do
multiple sets, let alone comparing them.
"branch-diff" was just a good as "diff-history", except that both of
them may irritate command line completion users. I do not think I
care too much about which exact command name it gets, but I think it
is a bad idea to tacked it to an existing command as a submode that
does unrelated thing to what the main command does. So from that
point of view, "branch-diff" and "diff-history" are equally good
being a distinct command, and equally bad sharing prefix with common
existing command.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-08 3:44 ` Jeff King@ 2018-05-08 3:48 ` Jeff King
2018-05-22 11:38 ` Ævar Arnfjörð Bjarmason1 sibling, 0 replies; 387+ messages in thread
From: Jeff King @ 2018-05-08 3:48 UTC (permalink / raw)
To: Stefan Beller
Cc: Igor Djordjevic, Johannes Schindelin, git, Junio C Hamano,
Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Jacob Keller, Eric Sunshine
On Mon, May 07, 2018 at 11:44:29PM -0400, Jeff King wrote:
> On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote:
>
> > Hence I propose "git range-diff", similar to topic-diff, that
> > was proposed earlier.
> >
> > * it "diffs ranges" of commits.
> > * it can also deal with out-of-git things like patch series,
> > but that is a mere by product and may not be desired.
> > Just like git-diff can also compare two files outside a git
> > repo, that would not be a good use case.
> > Keep the name Git-centric!
> > * it autocompletes well.
>
> FWIW, I like this by far of all of the suggested names.
I hit "send" before I had a chance to expound. ;)
The thing that I really like about it is that it names the _concept_.
If I were writing a manual page describing what this output is, I would
call it a "range diff". And naturally, the command to generate range
diffs is "git range-diff".
I think "git diff --range" would also be OK, but IMHO it's useful to
keep the "git diff" family as always comparing end-points.
-Peff
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-05 19:41 ` Johannes Schindelin@ 2018-05-09 16:24 ` Ramsay Jones
2018-06-01 8:23 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Ramsay Jones @ 2018-05-09 16:24 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
On 05/05/18 20:41, Johannes Schindelin wrote:
[snip]
[Sorry for the late reply - still catching up after (long weekend)
UK public holiday]
> Well, what I would want to do is let the cloud work for me. By adding an
> automated build to my Visual Studio Team Services (VSTS) account, of
> course, as I have "cloud privilege" (i.e. I work in the organization
> providing the service, so I get to play with all of it for free).
>
> So I really don't want to build sparse every time a new revision needs to
> be tested (whether that be from one of my branches, an internal PR for
> pre-review of patches to be sent to the mailing list, or maybe even `pu`
> or the personalized branches on https://github.com/gitster/git).
>
> I really would need a ready-to-install sparse, preferably as light-weight
> as possible (by not requiring any dependencies outside what is available
> in VSTS' hosted Linux build agents.
>
> Maybe there is a specific apt source for sparse?
Not that I'm aware of, sorry! :(
[release _source_ tar-balls are available, but that's not
what you are after, right?]
I don't know what is involved in setting up a 'ppa repo' for
Ubuntu, which I suspect is the kind of thing you want, but it
would have helped me several times in the past (so that I could
have something to point people to) ... ;-)
ATB,
Ramsay Jones
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-03 15:30 [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike Johannes Schindelin
` (20 preceding siblings ...)
2018-05-04 15:34 ` [PATCH v2 " Johannes Schindelin
@ 2018-05-21 4:48 ` Junio C Hamano
2018-05-21 9:51 ` Johannes Schindelin21 siblings, 1 reply; 387+ messages in thread
From: Junio C Hamano @ 2018-05-21 4:48 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
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.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-21 4:48 ` [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike Junio C Hamano
@ 2018-05-21 9:51 ` Johannes Schindelin
2018-05-22 1:42 ` Junio C Hamano0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-21 9:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
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
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 21:33 ` Igor Djordjevic@ 2018-05-21 10:33 ` Johannes Schindelin
2018-05-21 17:56 ` Stefan Beller0 siblings, 1 reply; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-21 10:33 UTC (permalink / raw)
To: Igor Djordjevic
Cc: Jeff King, Eric Sunshine, Junio C Hamano, Git List, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Stefan Beller, Jacob Keller
Hi Buga,
On Mon, 7 May 2018, Igor Djordjevic wrote:
> On 07/05/2018 09:48, Jeff King wrote:
> >
> > > > Let's, please, not fall into the trap of polluting git-branch with
> > > > utterly unrelated functionality, as has happened a few times with
> > > > other Git commands. Let's especially not do so merely for the sake of
> > > > tab-completion. git-branch is for branch management; it's not for
> > > > diff'ing.
> > >
> > > I totally disagree. `git branch` is *the* command to work with branches.
> > > Yes, you can manage branches. But you can also list them. And now you can
> > > also compare them.
> >
> > One of the things I don't like about "git branch --diff" is that this
> > feature is not _just_ about branches at all. E.g., I could do:
> >
> > git tbdiff HEAD~10 HEAD~5 foo
> >
> > Or even:
> >
> > git tbdiff v2.16.0 v2.17.0 my-rewritten-v2.17.0
> >
> > Those arguments really are just commitishes, not necessarily branches.
> > One of the current interface rules for "git branch" is that the branch
> > names we hand it are interpreted _exactly_ as branch names. You cannot
> > "git branch -m v2.16.0", and there is no ambiguity in "git branch -d
> > foo" if "foo" is both a tag and a branch.
> >
> > But this new mode does not fit the pattern at all.
> >
> > If we were to attach this to an existing command, I think it has more to
> > do with "diff" than "branch". But I'm not sure we want to overload
> > "diff" either (which has traditionally been about two endpoints, and
> > does not really traverse at all, though arguably "foo...bar" is a bit of
> > a cheat :) ).
> >
> > > > Of the suggestions thus far, Junio's git-topic-diff seems the least
> > > > worse, and doesn't suffer from tab-completion problems.
> > >
> > > Except that this is too limited a view.
> >
> > Right, I agree with you. Topic branches are the intended use, but that's
> > not what it _does_, and obviously it can be applied in other cases. So
> > since "branch" is too specific, I think "topic branch" is even more so.
> >
> > It's really "diff-history" or something, I think. That's not very
> > catchy, but I think the best name would imply that it was diffing a set
> > of commits (so even "diff-commit" would not be right, because that again
> > sounds like endpoints).
>
> This is exactly what I feel as well, thanks for concise and
> to-the-point spelling out.
>
> From user interface perspective, I would expect something like this
> to be possible (and natural):
>
> (1) git diff topic-v1...topic-v2
No, we cannot. The `git diff topic-v1...topic-v2` invocation has worked
for a looooong time, and does something very different.
We should not even allow ourselves to think of such a breakage.
> (2) git diff --branch topic-v1...topic-v2
From my point of view, `git diff --branch` indicates that I diff
*branches*. Which is not really something that makes sense, and definitely
not what this command is about.
We are not comparing branches.
We are comparing versions of the same branch.
> (1) is what we are all familiar with, providing a diff between two
> revisions with focus on file changes, where (2) shifts focus to
> history changes.
>
> It`s all still a comparison between two revisions (pointed to by
> "topic-v1" and "topic-v2" branch heads in this specific example), but
> it differs in what we are comparing - (1) set of files contained in
> endpoints, or (2) set of revisions contained in (or "leading to")
> endpoints.
It is very much not about comparing *two* revisions. It is very much about
comparing two *ranges of* revisions, and not just any ranges, no. Those
ranges need to be so related as to contain mostly identical changes.
Otherwise, `git branch --diff` will spend a ton of time, just to come back
with a series of `-` lines followed by a series of `+` lines
(figuratively, not literally). Which would be stupid, to spend that much
time on something that `git rev-list --left-right topic1...topic2` would
have computed a lot faster.
> Hmm... what about `git diff --history`? :/ It does seem more "true"
> to what it does, though I still like `git diff --branch` more
> (catchier, indeed).
It certainly is catchier. But also a ton more puzzling.
I do not want to compare histories, after all. That would be like saying:
okay, topic1 and topic2 ended up at the same stage, but *how* did they
get there?
What I *want* to ask via the command implemented by this patch series is
the question: there was a set of patches previously, and now I have a set
of revised patches, what changed?
Most fellow German software engineers (who seem to have a knack for
idiotically long variable/function names) would now probably suggest:
git compare-patch-series-with-revised-patch-series
I hope you agree that that is better *and* worse than your suggestions,
depending from what angle you look at it: it is better because it
describes what the command is *actually* doing. But it is much worse at
the same time because it is too long.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-07 15:12 ` Junio C Hamano@ 2018-05-21 10:41 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-21 10:41 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Stefan Beller, Jacob Keller, Eric Sunshine
Hi Junio,
On Tue, 8 May 2018, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > It would be easy to introduce, but I am wary about its usefulness.
> > Unless you re-generate the branch from patches (which I guess you do a
> > lot, but I don't), you are likely to compare incomplete patch series: say,
> > when you call `git rebase -i` to reword 05/18's commit message, your
> > command will only compare 05--18 of the patch series.
>
> Well that is exactly the point of that "..@{1} @{1}..", which turned
> out to be very useful in practice at least for me when I am updating
> a topic with "rebase -i", and then reviewing what I did with tbdiff.
>
> I do not want 01-04 in the above case as I already know I did not
> touch them.
And you are a seasoned veteran maintainer.
To the occasional contributor, this information is not obvious, and it is
not stored in their brain. It needs to be made explicit, which is why this
here command outputs those `abcdef = 012345` lines: it lists all the
commits, stating which ones are unchanged. In your 01-04 example, those
lines would be of the form `abcdef = abcdef`, of course.
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-21 10:33 ` Johannes Schindelin@ 2018-05-21 17:56 ` Stefan Beller
2018-05-21 20:24 ` Jeff King0 siblings, 1 reply; 387+ messages in thread
From: Stefan Beller @ 2018-05-21 17:56 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Igor Djordjevic, Jeff King, Eric Sunshine, Junio C Hamano,
Git List, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Jacob Keller
Hi Johannes,
>> (2) git diff --branch topic-v1...topic-v2
>
> From my point of view, `git diff --branch` indicates that I diff
> *branches*. Which is not really something that makes sense, and definitely
> not what this command is about.
>
> We are not comparing branches.
>
> We are comparing versions of the same branch.
I happen to have a messier workflow than you have, as I
develop a "resend" of a topic in a new branch (or I have to
restore the old sent topic from the reflog).
Now that I have the tool I also compare two branches,
namely, the branch that Junio queued
(origin/base..origin/sb/intelligent-name) vs the resend
that I had locally (origin/base..foo).
Next time I might compare Junios queued topic to the
local format-patch'es that I already annotated.
So in a way this diffs different versions of a topic, "diff-topic-versions".
>> It`s all still a comparison between two revisions (pointed to by
>> "topic-v1" and "topic-v2" branch heads in this specific example), but
>> it differs in what we are comparing - (1) set of files contained in
>> endpoints, or (2) set of revisions contained in (or "leading to")
>> endpoints.
>
> It is very much not about comparing *two* revisions.
I wonder if we can make the tool more intelligent to take two revisions
and it figures out the range by finding the base branch itself.
Probably as a follow up.
> It is very much about
> comparing two *ranges of* revisions, and not just any ranges, no. Those
> ranges need to be so related as to contain mostly identical changes.
range-diff, eh?
> Most fellow German software engineers (who seem to have a knack for
> idiotically long variable/function names) would now probably suggest:
>
> git compare-patch-series-with-revised-patch-series
or short:
revision-compare
compare-revs
com-revs
revised-diff
revise-diff
revised-compare
diff-revise
> I hope you agree that that is better *and* worse than your suggestions,
> depending from what angle you look at it: it is better because it
> describes what the command is *actually* doing. But it is much worse at
> the same time because it is too long.
btw, you think very much in terms of *patch series*, but there are workflows
without patches (pull requests at Github et Al., changes in Gerrit),
and I would think the output of the tool under discussion would still be
useful.
In [1] Junio gives his use case, it is "before accepting them", which could
be comparing an mbox or patch files against a branch, or first building
up a local history on a detached head (and then wondering if to reset
the branch to the new history), which would be all in Git.
That use case still has 'patches' involved, but these are not the main
selling point for the tool, as you could turn patches into commits before
using this tool.
[1] https://public-inbox.org/git/xmqqvabh1ung.fsf@gitster-ct.c.googlers.com/
Thanks,
Stefan
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-21 17:56 ` Stefan Beller@ 2018-05-21 20:24 ` Jeff King
2018-05-21 21:40 ` Brandon Williams0 siblings, 1 reply; 387+ messages in thread
From: Jeff King @ 2018-05-21 20:24 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, Igor Djordjevic, Eric Sunshine,
Junio C Hamano, Git List, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Jacob Keller
On Mon, May 21, 2018 at 10:56:47AM -0700, Stefan Beller wrote:
> > It is very much about
> > comparing two *ranges of* revisions, and not just any ranges, no. Those
> > ranges need to be so related as to contain mostly identical changes.
>
> range-diff, eh?
>
> > Most fellow German software engineers (who seem to have a knack for
> > idiotically long variable/function names) would now probably suggest:
> >
> > git compare-patch-series-with-revised-patch-series
>
> or short:
>
> revision-compare
> compare-revs
> com-revs
>
> revised-diff
> revise-diff
> revised-compare
>
> diff-revise
I still like "range diff", but I think something around "revise" is a
good line of thought, too. Because it implies that we expect the two
ranges to be composed of almost-the-same commits.
That leads to another use case where I think focusing on topic branches
(or even branches at all) would be a misnomer. Imagine I cherry-pick a
bunch of commits with:
git cherry-pick -10 $old_commit
I might then want to see how the result differs with something like:
git range-diff $old_commit~10..$old_commit HEAD~10..HEAD
I wouldn't think of this as a topic-branch operation, but just as
comparing two sequences of commits. I guess "revise" isn't strictly
accurate here either, as I'm not revising. But I do assume the two
ranges share some kind of mapping of patches.
-Peff
PS I wish there were a nicer syntax to do that. Perhaps
"git range-diff -10 $old_commit HEAD" could work, though occasionally
the two ranges are not the same length (e.g., if you ended up
skipping one of the cherry-picked commits). Anyway, those kind of
niceties can easily come later on top. :)
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-21 9:51 ` Johannes Schindelin@ 2018-05-22 1:42 ` Junio C Hamano
2018-06-01 8:28 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: Junio C Hamano @ 2018-05-22 1:42 UTC (permalink / raw)
To: Johannes Schindelin
Cc: git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Johannes Schindelin <Johannes.Schindelin@gmx.de> 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.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-21 21:52 ` Jeff King@ 2018-05-22 2:08 ` Junio C Hamano0 siblings, 0 replies; 387+ messages in thread
From: Junio C Hamano @ 2018-05-22 2:08 UTC (permalink / raw)
To: Jeff King
Cc: Brandon Williams, Stefan Beller, Johannes Schindelin,
Igor Djordjevic, Eric Sunshine, Git List, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Jacob Keller
Jeff King <peff@peff.net> writes:
>> I haven't really been following all of the discussion but from what I
>> can tell the point of this command is to generate a diff based on two
>> different versions of a series, so why not call it 'series-diff'? :)
>
> That's OK with me, though I prefer "range" as I think we use that term
> elsewhere ("series" is usually part of "patch series", but many people
> do not use a workflow with that term).
FWIW, I am OK with either, with a bit of preference to "range" over
"series". As long as this stays to be an independent command (as
opposed to be made into a new mode to existing command) and the
command name is not overly hard to type, I am OK with anything ;-)
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-08 3:44 ` Jeff King
2018-05-08 3:48 ` Jeff King@ 2018-05-22 11:38 ` Ævar Arnfjörð Bjarmason
2018-05-25 22:06 ` Stefan Beller1 sibling, 1 reply; 387+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2018-05-22 11:38 UTC (permalink / raw)
To: Jeff King
Cc: Stefan Beller, Igor Djordjevic, Johannes Schindelin, git,
Junio C Hamano, Thomas Rast, Thomas Gummerer, Ramsay Jones,
Jacob Keller, Eric Sunshine
On Tue, May 08 2018, Jeff King wrote:
> On Mon, May 07, 2018 at 03:24:59PM -0700, Stefan Beller wrote:
>
>> Hence I propose "git range-diff", similar to topic-diff, that
>> was proposed earlier.
>>
>> * it "diffs ranges" of commits.
>> * it can also deal with out-of-git things like patch series,
>> but that is a mere by product and may not be desired.
>> Just like git-diff can also compare two files outside a git
>> repo, that would not be a good use case.
>> Keep the name Git-centric!
>> * it autocompletes well.
>
> FWIW, I like this by far of all of the suggested names.
I agree, "range-diff" is the best one mentioned so far.
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 02/18] Add a new builtin: branch-diff
2018-05-22 11:38 ` Ævar Arnfjörð Bjarmason@ 2018-05-25 22:06 ` Stefan Beller
[not found] ` <CAA8fPEkNjy+ETz4Mx+C2kUfLjLzR9uuOmO3GfN48ZH1SwyfE1A@mail.gmail.com>
0 siblings, 1 reply; 387+ messages in thread
From: Stefan Beller @ 2018-05-25 22:06 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Jeff King, Igor Djordjevic,
Ævar Arnfjörð Bjarmason, git, Junio C Hamano,
Thomas Rast, Thomas Gummerer, Ramsay Jones, Jacob Keller,
Eric Sunshine
Johannes,
On IRC you wrote:
<dscho> And BTW this is not bike-shedding to me. Discussing the name
of a variable, or indentation, or line wrapping, is. But improving the
user experience is important. We *suck* on that, historically, and I
do want to break with that habit.
...
<dscho> avar, _ikke_: so a colleague of mine whose opinion on naming I
respect more than all Git developers combined *also* came up with the
term `range-diff`, independently.
...
<dscho> Yes, you are looking at two ranges. But not *any* two ranges.
*That* is my point.
So I sat back and want to try again;
IIUC your dislike for "range-diff" boils down to:
(A) it doesn't diff any arbitrary range, as the output would become
very cumbersome and hard to understand,
(B) it is not a good intuitive name for users, as they would not think
of range-diff when they'd want to have this feature.
Regarding (A), I think the same can be said about input to the diff
machinery, e.g. 'git diff v2.0.0 v2.17.0' is just very much text, and
it is hardly useful (except as a patch fed to the machine).
Over time there were added tons of options that make the diff output
easier to digest, e.g. additional pathspecs to restrict to a sub tree or
ignoring certain things (white spaces mostly), such that
'git diff -w v2.0.0 v2.17.0 -- refs.h' is easier for a human to grok.
Regarding (B), I agree, but blame it on the nature of an open
source project that provides a toolbox. So the way a user is
going to discover this feature is via stackoverflow or via
asking a coworker or finding the example output somewhere.
I think that last point could be part of the feedback:
git-diff has prominently hints at its name via "diff --git ..."
in the first line of its output, so maybe the output of this feature
also wants to name itself?
Other thoughts:
We could go down the route and trying to find a best possible
technical name, for which I could offer:
revision-walk-difference
revwalk-diff
As that literally describes the output: two rev walks are
performed and then those outputs of the rev walks is diffed.
Based off these technicals we could get more creative:
redo-rev-walk-spot-the-difference
re-walk-spot
retravel-spot
spot-diff
But I think all these do not address the feedback (B).
"What would a user find intuitive?"; I personally thought
a bit about how I discovered cherry-pick. I just took it as
a given name, without much thought, as I discovered it
by tell tale, not looking for it in the docs. It sort of made
sense as a command that I learned earlier about,
"interactive rebase", also has had the "pick" command,
such that "picking" made sense. I think I retroactively
made sense of the "cherry" part. Now I tried to find it
in the mailing list archive and actually learn about its origin,
but no good stories are found there.
For what the user might find most useful, I just looked
at other tools in Gerrits landscape and there the expectation
seems that you upload your code first and do the diff of the different
patches serverside. I think the same holds for Github or other
branch based reviewing systems. You can force push the
branch that is pull requested and the web UI somehow makes
sense of it.
That leads me to the (weak) conclusion of branch-diff or tbdiff
to be useful most for patch based / mailing list based workflows
as there is no magic server helping you out.
Searching for "kernel +tbdiff" to find the kernel devs using tbdiff
gave me no results, so I may be mistaken there.
Trying to find "interdiffs" (for the lack of a better name) between
patches on the kernel mailing list also is not obvious to the uninitiated.
So for the various workflows, I could come up with
change-diff
pullrequest-diff
patch-series-diff
but we do not look at diffs, rather we only use this tool to work on
incremental things, so maybe instead:
change-history
pullrequest-history
patch-series-evolution
Note how these are 3 suggestions, one for each major workflow,
and I'd *REALLY* would want to have a tool that is agnostic to the
workflow on top (whether you use pull requests or Gerrit changes),
but now I would like to step back and remind us that this tool
is only mostly used for viewing the evolution of your new thing,
but it can also be very useful to inspect non-new things.
(backported patches to maint, or some -stable branch)
Or rather: We do not know the major use case yet. Sure
I will use it in my cover letter and that is on my mind now,
but I think there are other use cases that are not explored
yet, so we should rather make the naming decision based
off of technicals rather than anticipated use case and user
discovery methods.
I hope this is actually useful feedback on the naming discovery.
Thanks,
Stefan
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 01/18] Add a function to solve least-cost assignment problems
2018-05-30 13:55 ` SZEDER Gábor@ 2018-05-30 16:14 ` Stefan Beller
2018-05-30 23:28 ` brian m. carlson0 siblings, 1 reply; 387+ messages in thread
From: Stefan Beller @ 2018-05-30 16:14 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Johannes Schindelin, git, Junio C Hamano, Thomas Rast,
Thomas Gummerer, Ævar Arnfjörð Bjarmason,
Ramsay Jones, Jacob Keller, Eric Sunshine
On Wed, May 30, 2018 at 6:55 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:
>> The Jonker-Volgenant algorithm was implemented to answer questions such
>> as: given two different versions of a topic branch (or iterations of a
>> patch series), what is the best pairing of commits/patches between the
>> different versions?
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> Makefile | 1 +
>> hungarian.c | 205 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> hungarian.h | 19 +++++
>
> (Nit: I personally don't really like these filenames, I know they will
> surprise and distract me every time I notice them for years to come... :)
Good point. I remember my initial reaction to the file names was expecting
some hungarian notation, which totally didn't make sense, so I refrained from
commenting. Searching the web for the algorithm, maybe 'lapjv.c' is adequate?
(short for "Linear Assignment Problem Jonker Volgenant") Matlab has a function
named lapjv solving the same problem, so it would fall in line with the outside
world.
Out of interest, why is it called hungarian in the first place? (I presume that
comes from some background of DScho in image processing or such, so the
the answer will be interesting for sure:)
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 01/18] Add a function to solve least-cost assignment problems
2018-05-30 16:14 ` Stefan Beller@ 2018-05-30 23:28 ` brian m. carlson
2018-05-31 12:19 ` Johannes Schindelin0 siblings, 1 reply; 387+ messages in thread
From: brian m. carlson @ 2018-05-30 23:28 UTC (permalink / raw)
To: Stefan Beller
Cc: SZEDER Gábor, Johannes Schindelin, git, Junio C Hamano,
Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Jacob Keller, Eric Sunshine
[-- Attachment #1: Type: text/plain, Size: 1142 bytes --]
On Wed, May 30, 2018 at 09:14:06AM -0700, Stefan Beller wrote:
> Good point. I remember my initial reaction to the file names was expecting
> some hungarian notation, which totally didn't make sense, so I refrained from
> commenting. Searching the web for the algorithm, maybe 'lapjv.c' is adequate?
> (short for "Linear Assignment Problem Jonker Volgenant") Matlab has a function
> named lapjv solving the same problem, so it would fall in line with the outside
> world.
>
> Out of interest, why is it called hungarian in the first place? (I presume that
> comes from some background of DScho in image processing or such, so the
> the answer will be interesting for sure:)
I think it's because tbdiff uses the hungarian Python module, which
implements the Hungarian method, also known as the Kuhn-Munkres
algorithm, for solving the linear assignment problem. This is the
Jonker-Volgenant algorithm, which solves the same problem. It's faster,
but less tolerant.
At least this is what I just learned after about ten minutes of
searching.
--
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH v2 01/18] Add a function to solve least-cost assignment problems
2018-05-30 23:28 ` brian m. carlson@ 2018-05-31 12:19 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-05-31 12:19 UTC (permalink / raw)
To: brian m. carlson
Cc: Stefan Beller, SZEDER Gábor, git, Junio C Hamano,
Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason, Ramsay Jones,
Jacob Keller, Eric Sunshine
Hi Brian,
On Wed, 30 May 2018, brian m. carlson wrote:
> On Wed, May 30, 2018 at 09:14:06AM -0700, Stefan Beller wrote:
> > Good point. I remember my initial reaction to the file names was
> > expecting some hungarian notation, which totally didn't make sense, so
> > I refrained from commenting. Searching the web for the algorithm,
> > maybe 'lapjv.c' is adequate? (short for "Linear Assignment Problem
> > Jonker Volgenant") Matlab has a function named lapjv solving the same
> > problem, so it would fall in line with the outside world.
> >
> > Out of interest, why is it called hungarian in the first place? (I
> > presume that comes from some background of DScho in image processing
> > or such, so the the answer will be interesting for sure:)
>
> I think it's because tbdiff uses the hungarian Python module, which
> implements the Hungarian method, also known as the Kuhn-Munkres
> algorithm, for solving the linear assignment problem. This is the
> Jonker-Volgenant algorithm, which solves the same problem. It's faster,
> but less tolerant.
>
> At least this is what I just learned after about ten minutes of
> searching.
You learned well.
The Assignment Problem (or "Linear Assignment Problem") is generally
solved by the Hungarian algorithm. I forgot why it is called that way.
Kuhn-Munkres came up with a simplification of the algorithm IIRC but it
still is O(N^4). Then Jonker-Volgenant took a very different approach that
somehow results in O(N^3). It's been *years* since I studied both
implementations, so I cannot really explain what they do, and how the
latter achieves its order-of-magnitude better performance.
And after reading these mails, I agree that the name "hungarian" might be
confusing.
I also think that "lapjv" is confusing. In general, I try to let Matlab
conventions inform on my naming as little as possible, and I find my
naming fu a lot better for it.
So in this case, how about `linear-assignment.c`?
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 02/18] Add a new builtin: branch-diff
2018-05-09 16:24 ` Ramsay Jones@ 2018-06-01 8:23 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-06-01 8:23 UTC (permalink / raw)
To: Ramsay Jones
Cc: git, Junio C Hamano, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Ramsay,
On Wed, 9 May 2018, Ramsay Jones wrote:
> On 05/05/18 20:41, Johannes Schindelin wrote:
> [snip]
>
> [Sorry for the late reply - still catching up after (long weekend)
> UK public holiday]
>
> > Well, what I would want to do is let the cloud work for me. By adding an
> > automated build to my Visual Studio Team Services (VSTS) account, of
> > course, as I have "cloud privilege" (i.e. I work in the organization
> > providing the service, so I get to play with all of it for free).
> >
> > So I really don't want to build sparse every time a new revision needs to
> > be tested (whether that be from one of my branches, an internal PR for
> > pre-review of patches to be sent to the mailing list, or maybe even `pu`
> > or the personalized branches on https://github.com/gitster/git).
> >
> > I really would need a ready-to-install sparse, preferably as light-weight
> > as possible (by not requiring any dependencies outside what is available
> > in VSTS' hosted Linux build agents.
> >
> > Maybe there is a specific apt source for sparse?
>
> Not that I'm aware of, sorry! :(
>
> [release _source_ tar-balls are available, but that's not
> what you are after, right?]
No, that's not what I am after, because my goal is not to build sparse
every time somebody pushes a new commit.
I want to use the Hosted Agents of Visual Studio Team Services (because I
have cloud privilege, as part of the team working on VSTS, I can use them
for free, as much as I want, within reason of course). And yes, I want to
use the Hosted Linux Agents for the sparse job.
So I cannot compile sparse and then install it into an agent. Those agents
are recycled after every build, so that every new build starts from a
clean slate.
If you have anything in the way of providing some easily-consumable
package, that would do the job. I guess I could build sparse.deb via
checkinstall in one VSTS build, offer it as artifact, and consume that
from the VSTS job that uses it on the Git branches.
Could you point me to a robus, yet current revision of sparse (and ideally
provide me with precise instructions how to build it so that I do not have
to hunt for that information)?
Ciao,
Dscho
^permalinkrawreply [flat|nested] 387+ messages in thread

*Re: [PATCH 00/18] Add `branch-diff`, a `tbdiff` lookalike
2018-05-22 1:42 ` Junio C Hamano@ 2018-06-01 8:28 ` Johannes Schindelin0 siblings, 0 replies; 387+ messages in thread
From: Johannes Schindelin @ 2018-06-01 8:28 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Thomas Rast, Thomas Gummerer,
Ævar Arnfjörð Bjarmason
Hi Junio,
On Tue, 22 May 2018, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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
^permalinkrawreply [flat|nested] 387+ messages in thread