[PATCH 1/2] grep.c: extract show_line_header()

[Taylor Blau]

Teach 'git-grep(1)' how to print a line header multiple times from
show_line() in preparation for it learning '--only-matching'.

show_line_header() comprises of the code in show_line() executed in
order to produce a line header. It is a one-for-one extraction of the
existing implementation.

For now, show_line_header() provides no benefit over the change before
this patch. The following patch will call conditionally call
show_line_header() multiple times per invocation to show_line(), which
is the desired benefit of this change.

Signed-off-by: Taylor Blau 
---
 grep.c | 44 +---
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/grep.c b/grep.c
index 37bb39a4a8..89dd719e4d 100644
--- a/grep.c
+++ b/grep.c
@@ -1369,26 +1369,9 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
return hit;
 }
 
-static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, unsigned cno, char sign)
+static void show_line_header(struct grep_opt *opt, const char *name,
+ unsigned lno, unsigned cno, char sign)
 {
-   int rest = eol - bol;
-   const char *match_color, *line_color = NULL;
-
-   if (opt->file_break && opt->last_shown == 0) {
-   if (opt->show_hunk_mark)
-   opt->output(opt, "\n", 1);
-   } else if (opt->pre_context || opt->post_context || opt->funcbody) {
-   if (opt->last_shown == 0) {
-   if (opt->show_hunk_mark) {
-   output_color(opt, "--", 2, opt->color_sep);
-   opt->output(opt, "\n", 1);
-   }
-   } else if (lno > opt->last_shown + 1) {
-   output_color(opt, "--", 2, opt->color_sep);
-   opt->output(opt, "\n", 1);
-   }
-   }
if (opt->heading && opt->last_shown == 0) {
output_color(opt, name, strlen(name), opt->color_filename);
opt->output(opt, "\n", 1);
@@ -1416,6 +1399,29 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_columnno);
output_sep(opt, sign);
}
+}
+
+static void show_line(struct grep_opt *opt, char *bol, char *eol,
+ const char *name, unsigned lno, unsigned cno, char sign)
+{
+   int rest = eol - bol;
+   const char *match_color, *line_color = NULL;
+
+   if (opt->file_break && opt->last_shown == 0) {
+   if (opt->show_hunk_mark)
+   opt->output(opt, "\n", 1);
+   } else if (opt->pre_context || opt->post_context || opt->funcbody) {
+   if (opt->last_shown == 0) {
+   if (opt->show_hunk_mark) {
+   output_color(opt, "--", 2, opt->color_sep);
+   opt->output(opt, "\n", 1);
+   }
+   } else if (lno > opt->last_shown + 1) {
+   output_color(opt, "--", 2, opt->color_sep);
+   opt->output(opt, "\n", 1);
+   }
+   }
+   show_line_header(opt, name, lno, cno, sign);
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
-- 
2.17.0

[PATCH 0/2] builtin/grep.c: teach '-o', '--only-matching'

[Taylor Blau]

Hi,

Attached is a series to teach 'git-grep(1)' how to respond to
'--only-matching' (a-la GNU grep(1)'s --only-matching, including an '-o'
synonym) to only print the matching component(s) of a line. It is based
on v4 of tb/grep-colno, which was sent in [1].

This was suggested to me by Ævar in [2].

This change was fairly straightforward, as Ævar suggests in [3], with
the only complication being that we must print a line header multiple
times when there are >1 matches per-line. This requirement pushes the
implementation towards the extraction of a show_line_header() function,
and some minor changes in show_line() to reflect.

Thank you in advance for your review.


Thanks,
Taylor

[1]: https://public-inbox.org/git/cover.1525488108.git...@ttaylorr.com
[2]: https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com
[3]: https://public-inbox.org/git/87in9ucsbb@evledraar.gmail.com

Taylor Blau (2):
  grep.c: extract show_line_header()
  builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

 Documentation/git-grep.txt |  6 +++-
 builtin/grep.c |  1 +
 grep.c | 67 +-
 grep.h |  1 +
 t/t7810-grep.sh| 33 +++
 5 files changed, 85 insertions(+), 23 deletions(-)

--
2.17.0

[PATCH 2/2] builtin/grep.c: teach '-o', '--only-matching' to 'git-grep'

[Taylor Blau]

Teach GNU grep(1)'s '-o' ('--only-matching') to 'git-grep'. This option
prints only the matching components of each line. It writes multiple
lines if more than one match exists on a given line.

For example:

  $ git grep -on --column --heading git -- README.md | head -3
  README.md
  15:56:git
  18:20:git

By using show_line_header(), 'git grep --only-matching' correctly
respects the '--header' option:

  $ git grep -on --column --heading git -- README.md | head -4
  README.md
  15:56:git
  18:20:git
  19:16:git

Signed-off-by: Taylor Blau 
---
 Documentation/git-grep.txt |  6 +-
 builtin/grep.c |  1 +
 grep.c | 23 ---
 grep.h |  1 +
 t/t7810-grep.sh| 33 +
 5 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index d451cd8883..9754923041 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -20,7 +20,7 @@ SYNOPSIS
   [-c | --count] [--all-match] [-q | --quiet]
   [--max-depth ]
   [--color[=] | --no-color]
-  [--break] [--heading] [-p | --show-function]
+  [--break] [--heading] [-o | --only-matching] [-p | --show-function]
   [-A ] [-B ] [-C ]
   [-W | --function-context]
   [--threads ]
@@ -221,6 +221,10 @@ providing this option will cause it to die.
Show the filename above the matches in that file instead of
at the start of each shown line.
 
+--o::
+--only-matching::
+   Show only the matching part of the lines.
+
 -p::
 --show-function::
Show the preceding line that contains the function name of
diff --git a/builtin/grep.c b/builtin/grep.c
index 5c83f17759..5028bf96cf 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -851,6 +851,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
N_("print empty line between matches from different 
files")),
OPT_BOOL(0, "heading", ,
N_("show filename only once above matches from same 
file")),
+   OPT_BOOL('o', "only-matching", _matching, N_("show 
only matches")),
OPT_GROUP(""),
OPT_CALLBACK('C', "context", , N_("n"),
N_("show  context lines before and after matches"),
diff --git a/grep.c b/grep.c
index 89dd719e4d..da3f8e6266 100644
--- a/grep.c
+++ b/grep.c
@@ -1422,11 +1422,13 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
}
}
show_line_header(opt, name, lno, cno, sign);
-   if (opt->color) {
+   if (opt->color || opt->only_matching) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
int ch = *eol;
int eflags = 0;
+   int first = 1;
+   int offset = 1;
 
if (sign == ':')
match_color = opt->color_match_selected;
@@ -1443,16 +1445,31 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
if (match.rm_so == match.rm_eo)
break;
 
-   output_color(opt, bol, match.rm_so, line_color);
+   if (!opt->only_matching)
+   output_color(opt, bol, match.rm_so, line_color);
+   else if (!first) {
+   /*
+* We are given --only-matching, and this is not
+* the first match on a line. Reprint the
+* newline and header before showing another
+* match.
+*/
+   opt->output(opt, "\n", 1);
+   show_line_header(opt, name, lno,
+   offset+match.rm_so, sign);
+   }
output_color(opt, bol + match.rm_so,
 match.rm_eo - match.rm_so, match_color);
+   offset += match.rm_eo;
bol += match.rm_eo;
rest -= match.rm_eo;
eflags = REG_NOTBOL;
+   first = 0;
}
*eol = ch;
}
-   output_color(opt, bol, rest, line_color);
+   if (!opt->only_matching)
+   output_color(opt, bol, rest, line_color);
opt->output(opt, "\n", 1);
 }
 
diff --git a/grep.h b/grep.h
index 08a0b391c5..24c1460100 100644
--- a/grep.h
+++ b/grep.h
@@ -126,6 +126,7 @@ struct grep_opt {
const char *prefix;
int prefix_length;
regex_t regexp;
+   int only_matching;
int linenum;
int columnnum;
int invert;
diff --git a/t/t7810-grep.sh 

[PATCH v4 2/7] grep.c: expose matched column in match_line()

[Taylor Blau]

When calling match_line(), callers presently cannot determine the
relative offset of the match because match_line() discards the
'regmatch_t' that contains this information.

Instead, teach match_line() to take in a 'regmatch_t *' so that callers
can inspect the match's starting and ending offset from the beginning of
the line. This additional argument has no effect when opt->extended is
non-zero.

We will later pass the starting offset from 'regmatch_t *' to
show_line() in order to display the column number of the first match.

Signed-off-by: Taylor Blau 
---
 grep.c | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 65b90c10a3..1c25782355 100644
--- a/grep.c
+++ b/grep.c
@@ -1299,17 +1299,17 @@ static int match_expr(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static int match_line(struct grep_opt *opt, char *bol, char *eol,
- enum grep_context ctx, int collect_hits)
+ regmatch_t *match, enum grep_context ctx,
+ int collect_hits)
 {
struct grep_pat *p;
-   regmatch_t match;
 
if (opt->extended)
return match_expr(opt, bol, eol, ctx, collect_hits);
 
/* we do not call with collect_hits without being extended */
for (p = opt->pattern_list; p; p = p->next) {
-   if (match_one_pattern(p, bol, eol, ctx, , 0))
+   if (match_one_pattern(p, bol, eol, ctx, match, 0))
return 1;
}
return 0;
@@ -1699,6 +1699,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
int try_lookahead = 0;
int show_function = 0;
struct userdiff_driver *textconv = NULL;
+   regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_HEAD;
xdemitconf_t xecfg;
 
@@ -1788,7 +1789,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
if ((ctx == GREP_CONTEXT_HEAD) && (eol == bol))
ctx = GREP_CONTEXT_BODY;
 
-   hit = match_line(opt, bol, eol, ctx, collect_hits);
+   hit = match_line(opt, bol, eol, , ctx, collect_hits);
*eol = ch;
 
if (collect_hits)
-- 
2.17.0

[PATCH v4 3/7] grep.[ch]: extend grep_opt to allow showing matched column

[Taylor Blau]

To support showing the matched column when calling 'git-grep(1)', teach
'grep_opt' the normal set of options to configure the default behavior
and colorization of this feature.

Signed-off-by: Taylor Blau 
---
 grep.c | 3 +++
 grep.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/grep.c b/grep.c
index 1c25782355..fb0fa23231 100644
--- a/grep.c
+++ b/grep.c
@@ -46,6 +46,7 @@ void init_grep_defaults(void)
color_set(opt->color_filename, "");
color_set(opt->color_function, "");
color_set(opt->color_lineno, "");
+   color_set(opt->color_columnno, "");
color_set(opt->color_match_context, GIT_COLOR_BOLD_RED);
color_set(opt->color_match_selected, GIT_COLOR_BOLD_RED);
color_set(opt->color_selected, "");
@@ -155,6 +156,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->extended_regexp_option = def->extended_regexp_option;
opt->pattern_type_option = def->pattern_type_option;
opt->linenum = def->linenum;
+   opt->columnnum = def->columnnum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
opt->relative = def->relative;
@@ -164,6 +166,7 @@ void grep_init(struct grep_opt *opt, const char *prefix)
color_set(opt->color_filename, def->color_filename);
color_set(opt->color_function, def->color_function);
color_set(opt->color_lineno, def->color_lineno);
+   color_set(opt->color_columnno, def->color_columnno);
color_set(opt->color_match_context, def->color_match_context);
color_set(opt->color_match_selected, def->color_match_selected);
color_set(opt->color_selected, def->color_selected);
diff --git a/grep.h b/grep.h
index 399381c908..08a0b391c5 100644
--- a/grep.h
+++ b/grep.h
@@ -127,6 +127,7 @@ struct grep_opt {
int prefix_length;
regex_t regexp;
int linenum;
+   int columnnum;
int invert;
int ignore_case;
int status_only;
@@ -159,6 +160,7 @@ struct grep_opt {
char color_filename[COLOR_MAXLEN];
char color_function[COLOR_MAXLEN];
char color_lineno[COLOR_MAXLEN];
+   char color_columnno[COLOR_MAXLEN];
char color_match_context[COLOR_MAXLEN];
char color_match_selected[COLOR_MAXLEN];
char color_selected[COLOR_MAXLEN];
-- 
2.17.0

[PATCH v4 7/7] contrib/git-jump/git-jump: jump to match column in addition to line

[Taylor Blau]

Take advantage of 'git-grep(1)''s new option, '--column' in order to
teach Peff's 'git-jump' script how to jump to the correct column for any
given match.

'git-grep(1)''s output is in the correct format for Vim's jump list, so
no additional cleanup is necessary.

Signed-off-by: Taylor Blau 
---
 contrib/git-jump/README   | 6 +++---
 contrib/git-jump/git-jump | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -35,7 +35,7 @@ Git-jump can generate four types of interesting lists:
 
   2. The beginning of any merge conflict markers.
 
-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a line.
 
   4. Any whitespace errors detected by `git diff --check`.
 
@@ -65,7 +65,7 @@ git jump grep foo_bar
 git jump grep -i foo_bar
 
 # use the silver searcher for git jump grep
-git config jump.grepCmd "ag --column"
+git config jump.grepCmd "ag"
 --
 
 
@@ -82,7 +82,7 @@ which does something similar to `git jump grep`. However, it 
is limited
 to positioning the cursor to the correct line in only the first file,
 leaving you to locate subsequent hits in that file or other files using
 the editor or pager. By contrast, git-jump provides the editor with a
-complete list of files and line numbers for each match.
+complete list of files, lines, and a column number for each match.
 
 
 Limitations
diff --git a/contrib/git-jump/git-jump b/contrib/git-jump/git-jump
index 80ab0590bc..931b0fe3a9 100755
--- a/contrib/git-jump/git-jump
+++ b/contrib/git-jump/git-jump
@@ -52,7 +52,7 @@ mode_merge() {
 # editor shows them to us in the status bar.
 mode_grep() {
cmd=$(git config jump.grepCmd)
-   test -n "$cmd" || cmd="git grep -n"
+   test -n "$cmd" || cmd="git grep -n --column"
$cmd "$@" |
perl -pe '
s/[ \t]+/ /g;
-- 
2.17.0

[PATCH v4 6/7] grep.c: add configuration variables to show matched option

[Taylor Blau]

To support git-grep(1)'s new option, '--column', document and teach
grep.c how to interpret relevant configuration options, similar to those
associated with '--line-number'.

Signed-off-by: Taylor Blau 
---
 Documentation/config.txt   | 5 +
 Documentation/git-grep.txt | 3 +++
 grep.c | 6 ++
 3 files changed, 14 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6e8d969f52..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,6 +1159,8 @@ color.grep.::
function name lines (when using `-p`)
 `lineNumber`;;
line number prefix (when using `-n`)
+`column`;;
+   column number prefix (when using `--column`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1708,6 +1710,9 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 5409a24399..d451cd8883 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -44,6 +44,9 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.
 
+grep.column::
+   If set to true, enable the `--column` option by default.
+
 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
'fixed', or 'perl' will enable the `--basic-regexp`, 
`--extended-regexp`,
diff --git a/grep.c b/grep.c
index f3fe416791..37bb39a4a8 100644
--- a/grep.c
+++ b/grep.c
@@ -96,6 +96,10 @@ int grep_config(const char *var, const char *value, void *cb)
opt->linenum = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "grep.column")) {
+   opt->columnnum = git_config_bool(var, value);
+   return 0;
+   }
 
if (!strcmp(var, "grep.fullname")) {
opt->relative = !git_config_bool(var, value);
@@ -112,6 +116,8 @@ int grep_config(const char *var, const char *value, void 
*cb)
color = opt->color_function;
else if (!strcmp(var, "color.grep.linenumber"))
color = opt->color_lineno;
+   else if (!strcmp(var, "color.grep.column"))
+   color = opt->color_columnno;
else if (!strcmp(var, "color.grep.matchcontext"))
color = opt->color_match_context;
else if (!strcmp(var, "color.grep.matchselected"))
-- 
2.17.0

[PATCH v4 5/7] builtin/grep.c: add '--column' option to 'git-grep(1)'

[Taylor Blau]

Teach 'git-grep(1)' a new option, '--column', to show the column
number of the first match on a non-context line.

For example:

  $ git grep -n --column foo | head -n3
  .clang-format:51:14:# myFunction(foo, bar, baz);
  .clang-format:64:7:# int foo();
  .clang-format:75:8:# void foo()

Signed-off-by: Taylor Blau 
---
 Documentation/git-grep.txt |  5 -
 builtin/grep.c |  1 +
 t/t7810-grep.sh| 22 ++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index 18b494731f..5409a24399 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -169,6 +169,9 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.
 
+--column::
+   Prefix the 1-indexed column number of the first match on non-context 
lines.
+
 -l::
 --files-with-matches::
 --name-only::
diff --git a/builtin/grep.c b/builtin/grep.c
index 5f32d2ce84..5c83f17759 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,6 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
+   OPT_BOOL(0, "column", , N_("show column number of 
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh
index 1797f632a3..a03c3416e7 100755
--- a/t/t7810-grep.sh
+++ b/t/t7810-grep.sh
@@ -99,6 +99,28 @@ do
test_cmp expected actual
'
 
+   test_expect_success "grep -w $L (with --column)" '
+   {
+   echo ${HC}file:5:foo mmap bar
+   echo ${HC}file:14:foo_mmap bar mmap
+   echo ${HC}file:5:foo mmap bar_mmap
+   echo ${HC}file:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
+   test_expect_success "grep -w $L (with --{line,column}-number)" '
+   {
+   echo ${HC}file:1:5:foo mmap bar
+   echo ${HC}file:3:14:foo_mmap bar mmap
+   echo ${HC}file:4:5:foo mmap bar_mmap
+   echo ${HC}file:5:14:foo_mmap bar mmap baz
+   } >expected &&
+   git grep -n --column -w -e mmap $H >actual &&
+   test_cmp expected actual
+   '
+
test_expect_success "grep -w $L" '
{
echo ${HC}file:1:foo mmap bar
-- 
2.17.0

[PATCH v4 4/7] grep.c: display column number of first match

[Taylor Blau]

To prepare for 'git grep' learning '--column', teach grep.c's
show_line() how to show the column of the first match on non-context
line.

Signed-off-by: Taylor Blau 
---
 grep.c | 21 -
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/grep.c b/grep.c
index fb0fa23231..f3fe416791 100644
--- a/grep.c
+++ b/grep.c
@@ -1364,7 +1364,7 @@ static int next_match(struct grep_opt *opt, char *bol, 
char *eol,
 }
 
 static void show_line(struct grep_opt *opt, char *bol, char *eol,
- const char *name, unsigned lno, char sign)
+ const char *name, unsigned lno, unsigned cno, char sign)
 {
int rest = eol - bol;
const char *match_color, *line_color = NULL;
@@ -1399,6 +1399,17 @@ static void show_line(struct grep_opt *opt, char *bol, 
char *eol,
output_color(opt, buf, strlen(buf), opt->color_lineno);
output_sep(opt, sign);
}
+   /*
+* Treat 'cno' as the 1-indexed offset from the start of a non-context
+* line to its first match. Otherwise, 'cno' is 0 indicating that we are
+* being called with a context line.
+*/
+   if (opt->columnnum && cno) {
+   char buf[32];
+   xsnprintf(buf, sizeof(buf), "%d", cno);
+   output_color(opt, buf, strlen(buf), opt->color_columnno);
+   output_sep(opt, sign);
+   }
if (opt->color) {
regmatch_t match;
enum grep_context ctx = GREP_CONTEXT_BODY;
@@ -1504,7 +1515,7 @@ static void show_funcname_line(struct grep_opt *opt, 
struct grep_source *gs,
break;
 
if (match_funcname(opt, gs, bol, eol)) {
-   show_line(opt, bol, eol, gs->name, lno, '=');
+   show_line(opt, bol, eol, gs->name, lno, 0, '=');
break;
}
}
@@ -1569,7 +1580,7 @@ static void show_pre_context(struct grep_opt *opt, struct 
grep_source *gs,
 
while (*eol != '\n')
eol++;
-   show_line(opt, bol, eol, gs->name, cur, sign);
+   show_line(opt, bol, eol, gs->name, cur, 0, sign);
bol = eol + 1;
cur++;
}
@@ -1833,7 +1844,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
show_pre_context(opt, gs, bol, eol, lno);
else if (opt->funcname)
show_funcname_line(opt, gs, bol, lno);
-   show_line(opt, bol, eol, gs->name, lno, ':');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
':');
last_hit = lno;
if (opt->funcbody)
show_function = 1;
@@ -1862,7 +1873,7 @@ static int grep_source_1(struct grep_opt *opt, struct 
grep_source *gs, int colle
/* If the last hit is within the post context,
 * we need to show this line.
 */
-   show_line(opt, bol, eol, gs->name, lno, '-');
+   show_line(opt, bol, eol, gs->name, lno, match.rm_so+1, 
'-');
}
 
next_line:
-- 
2.17.0

[PATCH v4 1/7] Documentation/config.txt: camel-case lineNumber for consistency

[Taylor Blau]

lineNumber has casing that is inconsistent with surrounding options,
like color.grep.matchContext, and color.grep.matchSelected. Re-case this
documentation in order to be consistent with the text around it, and to
ensure that new entries are consistent, too.

Signed-off-by: Taylor Blau 
---
 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 2659153cb3..6e8d969f52 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1157,7 +1157,7 @@ color.grep.::
filename prefix (when not using `-h`)
 `function`;;
function name lines (when using `-p`)
-`linenumber`;;
+`lineNumber`;;
line number prefix (when using `-n`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
-- 
2.17.0

[PATCH v4 0/7] Teach '--column' to 'git-grep(1)'

[Taylor Blau]

Hi,

Attached is my fourth--and what I anticipate to be the final--re-roll of
my series to add teach 'git-grep(1)' a new '--column' flag.

Since last time, I have changed the following:

  * Respond to Ævar's review suggesting that I (1) change git-jump's
README, (2) use --column over --column-number, and (3) use /*\n, not
/**\n. [1].

This change comprises the majority of the inter-diff between v3..v4,
which is added below for conveniency. I have chosen to additionally
rename the configuration variables from columnNumber to column, to be
consistent with the new flag name.

Thanks in advance for your review. I am going to send out my next patch
(which Ævar suggested) to add '--only-matching' to 'git-grep(1)'.


Thanks,
Taylor

[1]: https://public-inbox.org/git/874lk2e4he@evledraar.gmail.com

Taylor Blau (7):
  Documentation/config.txt: camel-case lineNumber for consistency
  grep.c: expose matched column in match_line()
  grep.[ch]: extend grep_opt to allow showing matched column
  grep.c: display column number of first match
  builtin/grep.c: add '--column' option to 'git-grep(1)'
  grep.c: add configuration variables to show matched option
  contrib/git-jump/git-jump: jump to match column in addition to line

 Documentation/config.txt   |  7 ++-
 Documentation/git-grep.txt |  8 +++-
 builtin/grep.c |  1 +
 contrib/git-jump/README|  6 +++---
 contrib/git-jump/git-jump  |  2 +-
 grep.c | 39 +-
 grep.h |  2 ++
 t/t7810-grep.sh| 22 +
 8 files changed, 72 insertions(+), 15 deletions(-)

Inter-diff (since v3):

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8a2893d1e1..b3c861c5c3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1159,8 +1159,8 @@ color.grep.::
function name lines (when using `-p`)
 `lineNumber`;;
line number prefix (when using `-n`)
-`columnNumber`;;
-   column number prefix (when using `--column-number`)
+`column`;;
+   column number prefix (when using `--column`)
 `match`;;
matching text (same as setting `matchContext` and `matchSelected`)
 `matchContext`;;
@@ -1710,8 +1710,8 @@ gitweb.snapshot::
 grep.lineNumber::
If set to true, enable `-n` option by default.

-grep.columnNumber::
-   If set to true, enable the `--column-number` option by default.
+grep.column::
+   If set to true, enable the `--column` option by default.

 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
diff --git a/Documentation/git-grep.txt b/Documentation/git-grep.txt
index c5c4d712e6..d451cd8883 100644
--- a/Documentation/git-grep.txt
+++ b/Documentation/git-grep.txt
@@ -13,7 +13,7 @@ SYNOPSIS
   [-v | --invert-match] [-h|-H] [--full-name]
   [-E | --extended-regexp] [-G | --basic-regexp]
   [-P | --perl-regexp]
-  [-F | --fixed-strings] [-n | --line-number] [--column-number]
+  [-F | --fixed-strings] [-n | --line-number] [--column]
   [-l | --files-with-matches] [-L | --files-without-match]
   [(-O | --open-files-in-pager) []]
   [-z | --null]
@@ -44,8 +44,8 @@ CONFIGURATION
 grep.lineNumber::
If set to true, enable `-n` option by default.

-grep.columnNumber::
-   If set to true, enable the `--column-number` option by default.
+grep.column::
+   If set to true, enable the `--column` option by default.

 grep.patternType::
Set the default matching behavior. Using a value of 'basic', 'extended',
@@ -172,7 +172,7 @@ providing this option will cause it to die.
 --line-number::
Prefix the line number to matching lines.

---column-number::
+--column::
Prefix the 1-indexed column number of the first match on non-context 
lines.

 -l::
diff --git a/builtin/grep.c b/builtin/grep.c
index 512f60c591..5c83f17759 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -829,7 +829,7 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
GREP_PATTERN_TYPE_PCRE),
OPT_GROUP(""),
OPT_BOOL('n', "line-number", , N_("show line 
numbers")),
-   OPT_BOOL(0, "column-number", , N_("show column 
number of first match")),
+   OPT_BOOL(0, "column", , N_("show column number of 
first match")),
OPT_NEGBIT('h', NULL, , N_("don't show 
filenames"), 1),
OPT_BIT('H', NULL, , N_("show filenames"), 1),
OPT_NEGBIT(0, "full-name", ,
diff --git a/contrib/git-jump/README b/contrib/git-jump/README
index 4484bda410..7630e16854 100644
--- a/contrib/git-jump/README
+++ b/contrib/git-jump/README
@@ -35,7 +35,7 @@ Git-jump can generate four types of interesting lists:

   2. The beginning of any merge conflict markers.

-  3. Any grep matches.
+  3. Any grep matches, including the column of the first match on a line.

   4. Any 

Re: [GSoC] A blog about 'git stash' project

[Paul-Sebastian Ungureanu]

Hello Dscho,

On 04.05.2018 01:10, Johannes Schindelin wrote:

Hi Paul,

On Fri, 4 May 2018, Paul-Sebastian Ungureanu wrote:


The community bonding period started. It is well known that for a
greater rate of success, it is recommended to send weekly reports
regarding project status.  But, what would be a good form for such a
report? I, for one, think that starting a blog is one of the best
options because all the content will be stored in one place. Without
further ado, I would like you to present my blog [1].

Any feedback is greatly appreciated! Thank you!

[1]
https://ungps.github.io/


Looks great!

Maybe also mention that you hang out on IRC occasionally, in case anybody
wants to tell you just how awesome a contributor you are?

Ciao,
Dscho



Thanks for the kind words and for mentoring me. It really means a lot to 
me seeing that my work is appreciated by professionals like you. It is a 
great confidence boost. I will definitely add a paragraph about IRC.


Best,
Paul Ungureanu

Re: cover letter cc's [was: [PATCH 60/67] hw/s390x: add include directory headers]

[Michael S. Tsirkin]

On Fri, May 04, 2018 at 08:07:53AM -0500, Eric Blake wrote:
> [adding a cross-post to the git mailing list]
> 
> On 05/04/2018 02:10 AM, Cornelia Huck wrote:
> > On Thu, 3 May 2018 22:51:40 +0300
> > "Michael S. Tsirkin"  wrote:
> > 
> > > This way they are easier to find using standard rules.
> > > 
> > > Signed-off-by: Michael S. Tsirkin 
> > > ---
> ...
> 
> > [Goes to find cover letter to figure out what this is all about.
> > *Please*, cc: people on the cover letter so they can see immediately
> > what this is trying to do!]
> 
> Is there an EASY way to make 'git format-patch --cover-letter $commitid'
> (and git send-email, by extension) automatically search for all cc's any any
> of the N/M patches, and auto-cc ALL of those recipients on the 0/N cover
> letter?  And if that is not something easily built into git format-patch
> directly, is it something that can easily be added to sendemail.cccmd?  This
> is not the first time that someone has complained that automatic cc's are
> not sending the cover letter context to a particular maintainer interested
> (and auto-cc'd) in only a subset of an overall series.
> 
> On the other hand, cc'ing all recipients for a largely mechanical patch
> series that was split into 67 parts, in part because it touches so many
> different maintainers' areas, may make the cover letter have so many
> recipients that various mail gateways start rejecting it as potential spam.

I do this sometimes (pipe to this script):

grep -e ^Signed-off-by -e ^Acked -e ^Reported -e ^Tested -e ^Cc | sed 
's/.*.*//'|sort | uniq | sed 's/^/Cc: /'


> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.   +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org

Re: [PATCH 1/5] submodule foreach: correct '$path' in nested submodules from a subdirectory

[Jonathan Tan]

On Thu, 3 May 2018 11:12:27 -0700
Stefan Beller  wrote:

> >> There are three different possible solutions that have more value:
> >> (a) The path value is documented as the path from the toplevel of the
> >> superproject to the mount point of the submodule. If 'the' refers to
> >> the superproject holding this submodule ('sub' holding 'nested'),
> >> the path would be expected to be path='nested'.
> >
> > What is "the", and why is it quoted?
> 
> because it is unclear what is emphasizes.
> It could be the intermediate (direct) superproject, or it
> could be the topmost superproject (where the command was run from).
> 
> Just having "the", makes it unclear which of both it refers to.

Ah, I see - so s/'the'/'the superproject'/

> >> (b) In case 'the' superproject is referring to the toplevel, which
> >> is the superproject in which the original command was invoked,
> >> then path is expected to be path='sub/nested'.

And here, s/'the' superproject/'the superproject'/

> > Same comment about "the", and I think s/toplevel, which is the
> > superproject in which the original command was invoked/outermost
> > superproject/.
> 
> The outermost superproject may not be the one you invoke the
> command in.

Good point. Maybe "the superproject the original command was run from",
but I'm open to a better name. So I would write the beginning as
follows:

  

  Also, in git-submodule.txt, $path is documented to be the "name of the
  submodule directory relative to the superproject", but "the
  superproject" is ambiguous.

  To resolve both these issues, we could:
  (a) Change "the superproject" to "its immediate superproject", so
  $path would be "nested" instead of "../nested".
  (b) Change "the superproject" to "the superproject the original
  command was run from", so $path would be "sub/nested" instead of
  "../nested".
  (c) Change "the superproject" to "the directory the original command
  was run from", so $path would be "../sub/nested" instead of
  "../nested".

Going back to the original patch:

> The behavior for (c) was introduced in 091a6eb0fe (submodule: drop the
> top-level requirement, 2013-06-16) the intent for $path seemed to be
> relative to $cwd to the submodule worktree, but that did not work for
> nested submodules, as the intermittent submodules were not included in
> the path.

The (c) behavior was never really introduced, right? 091a6eb0fe
attempted to introduce (c), but it didn't work when --recursive was
specified.

Re: [RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.

[Ævar Arnfjörð Bjarmason]

On Fri, May 04 2018, Jakub Narebski wrote:

(Just off-the cuff here and I'm surely about to be corrected by
Derrick...)

> * What to do about merge commits, and octopus merges in particular?
>   Should Bloom filter be stored for each of the parents?  How to ensure
>   fast access then (fixed-width records) - use large edge list?

You could still store it fixed with, you'd just say that if you
encounter a merge with N parents the filter wouldn't store files changed
in that commit, but rather whether any of the N (including the merge)
had changes to files as of the their common merge-base.

Then if they did you'd need to walk all sides of the merge where each
commit would also have the filter to figure out where the change(s)
was/were, but if they didn't you could skip straight to the merge base
and keep walking.

Which, on the topic of what else a commit graph could store: A mapping
from merge commits of N parents to the merge-base of those commits.

You could also store nothing for merges (or only files the merge itself
changed v.s. its parents). Derrick talked about how the bloom filter
implementation has a value that's "Didn't compute (for whatever reason),
look at it manually".

> * Then there is problem of rename and copying detection - I think we can
>   simply ignore it: unless someone has an idea about how to handle it?
>
>   Though this means that "git log --follow " wouldn't get any
>   speedup, and neither the half of "git gui blame" that runs "git blame
>   --incremental -C -C -w" -- the one that allows code copying and
>   movement detection.

Couldn't the bloom filter also speed up --follow if you did two passes
through the history? The first to figure out all files that ever changed
names, and then say you did `--follow sha1-name.c` on git.git. The
filter would have had all the bits for both sha1_name.c and sha1-name.c
set on all commits that touched either for all of the history.

Of course this would only work with a given default value of -M, but
on the assumption that most users left it at the default, and
furthermore that renames weren't so common as to make the filter useless
with too many false-positives as a result, it might be worth it. If you

Re: [RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.

[Ævar Arnfjörð Bjarmason]

On Fri, May 04 2018, Jakub Narebski wrote:

> With early parts of commit-graph feature (ds/commit-graph and
> ds/lazy-load-trees) close to being merged into "master", see
> https://public-inbox.org/git/xmqq4ljtz87g@gitster-ct.c.googlers.com/
> I think it would be good idea to think what other data could be added
> there to make Git even faster.

Thanks for the write-up.

> 3. Third, it needs to be reasonably fast to create, and fast to update.
> This means time to create the chunk to be proportional to number of
> commits, or sum of number of commits and edges (which for commit graph
> and other sparse graphs is proprtional to the number of nodes / commits
> anyway).  In my opinion time proportional to n*lug(n), where 'n' is the
> number of commits, is also acceptable.  Times proportional to n^2 or n^3
> are not acceptable.

I don't think this requirement makes sense...

>   DS> If we add commit-graph file downloads to the protocol, then the
>   DS> server could do this computation and send the data to all
>   DS> clients. But that would be "secondary" information that maybe
>   DS> clients want to verify, which is as difficult as computing it
>   DS> themselves.

... when combined with this. If hypothetically there was some data that
significantly sped up Git and the algorithm to generate it was
ridiculously expensive, that would be fine when combined with the
ability to fetch it pre-generated from the server. There could always be
an option where you trust the server and optionally don't verify the
data, or where it's much cheaper to verify than compute.

Re: [PATCH v2 0/6] Let the sequencer handle `git rebase -i --root`

[Stefan Beller]

>
> Branch-diff vs v1:
>  1: 42db734a980 ! 1: 73398da7119 sequencer: learn about the special "fake 
> root commit" handling
>  @@ -54,40 +54,50 @@
> return NULL;
>}
>
>  ++/* Read author-script and return an ident line (author  
> timestamp) */
>   +static const char *read_author_ident(struct strbuf *buf)


I like the new way of read_author_ident. Thanks for writing it!


>   +
>static const char staged_changes_advice[] =
>  @@ -159,7 +169,17 @@
>   +/* Does this command create a (non-merge) commit? */
>   +static int is_pick_or_similar(enum todo_command command)
>   +{
>  -+ return command <= TODO_SQUASH;
>  ++ switch (command) {
>  ++ case TODO_PICK:
>  ++ case TODO_REVERT:
>  ++ case TODO_EDIT:
>  ++ case TODO_REWORD:
>  ++ case TODO_FIXUP:
>  ++ case TODO_SQUASH:
>  ++ return 1;
>  ++ default:
>  ++ return 0;
>  ++ }

The switch case is not as bad as I thought following the discussion on of v1.

This series is
Reviewed-by: Stefan Beller 

Thanks!


During a lunch discussion I wondered if the branch diff format could lead to
another form of machine readable communication, i.e. if we want to add the
ability to read the branch diff format and apply the changes. Note how applying
this diff-diff would not create new commits, but rather amend existing commits.

[RFC] Other chunks for commit-graph, part 1 - Bloom filters, topo order, etc.

[Jakub Narebski]

Hello,

With early parts of commit-graph feature (ds/commit-graph and
ds/lazy-load-trees) close to being merged into "master", see
https://public-inbox.org/git/xmqq4ljtz87g@gitster-ct.c.googlers.com/
I think it would be good idea to think what other data could be added
there to make Git even faster.


Commit-graph format
===

A quick reminder: in its current incarnation the commit graph file
includes the following data [1]:

1. Some commit data that is just enough for many Git operations:
   - commit parents
   - commit tree (root tree OID)
   - committer date

2. The generation number of the commit. Commits with no parents have
   generation number 1; commits with parents have generation number one
   more than the maximum generation number of its parents.

The commit-graph file is split into chunks, which theoretically allows
extending the format wihout need for a version bump... though there is
currently no specified policy about unknown chunks (and understandably
so, as currently there are not any).

I think it would be good idea to talk about what more could be added to
be stored in the commit-graph file.

[1]: 
https://github.com/git/git/blob/pu/Documentation/technical/commit-graph-format.txt


Requirements and recommendations about possible new chunks
==

Because the main goal of commit-graph feature is better performance in
large repositories, any proposed new chunks (or, at higher level, every
proposed piece of new data) needs to have the following properties.


1. First, it needs to have bounded and sensible size.  This means that
the size of new proposed chunk should be constant, proportional to the
number of commits, or at worst proportional to the number of edges.

>From the existing chunks, OIDF (OID Fanout) has constant size, both OIDL
(OID Lookup) and CGET/CDAT (Commit Data) has size proportional to the
number of commits, while not always present EDGE (Large Edge List) has
size proportional to the number of "excess" edges in "huge vertices"
(octopus merges).


2. Second, we want fast access, in most cases fast random access.  This
means using fixed-size records.  All currently existing chunks have
records (columns) of fixed and aligned size.

This restriction means that idexes where we have variable amount of data
per vertex (per commit) are discouraged.


3. Third, it needs to be reasonably fast to create, and fast to update.
This means time to create the chunk to be proportional to number of
commits, or sum of number of commits and edges (which for commit graph
and other sparse graphs is proprtional to the number of nodes / commits
anyway).  In my opinion time proportional to n*lug(n), where 'n' is the
number of commits, is also acceptable.  Times proportional to n^2 or n^3
are not acceptable.

It is also strongly preferred that time to update the chunk is
proportional to the number of new commits, so that incremental
(append-only) update is possible.  The generation numbers index has this
property.


Generic new chunks
==

There are a few ideas for new chunks, new pieces of data to be added to
the commit-graph file, that are not connected with some labeling scheme
for directed acyclic graphs like GRAIL, FERRARI, FELINE or TF-Label.
Let's list them here.

If you have an idea of another bit of information that could be added to
the commit-graph file, please tell us.


Bloom filter for changed paths
--

The goal of this chunk is to speed up checking if the file or directory
was changed in given commit, for queries such as "git log -- " or
"git blame ".  This is something that according to "Git Merge
contributor summit notes" [2] is already present in VSTS (Visual Studio
Team Services - the server counterpart of GVFS: Git Virtual File System)
at Microsoft:

AV> - VSTS adds bloom filters to know which paths have changed on the commit
AV> - tree-same check in the bloom filter is fast; speeds up file history checks
AV> - might be useful in the client as well, since limited-traversal is common
AV> - if the file history is _very_ sparse, then bloom filter is useful
AV> - but needs pre-compute, so useful to do once
AV> - first make the client do it, then think about how to serve it centrally

[2]: 
https://public-inbox.org/git/alpine.DEB.2.20.1803091557510.23109@alexmv-linux/

I think it was what Derrick Stolee was talking about at the end of his
part of "Making Git for Windows" presentation at Git Merge 2018:
https://youtu.be/oOMzi983Qmw?t=1835

This was also mentioned in subthread of "Re: [PATCH v2 0/4] Lazy-load
trees when reading commit-graph", starting from [3]
[3]: https://public-inbox.org/git/86y3hyeu6c@gmail.com/

A quick reminder: a Bloom filter is a space-efficient probabilistic data
structure that is used to test whether an element is a member of a set.
False negatives are not possible: if a Bloom filter says that the
element is not in set, then it certainly 

Re: [PATCH v3 0/7] Some doc-fixes

[Martin Ågren]

On 3 May 2018 at 20:48, Andreas Heiduk  wrote:
> Changes since the last reroll:
>
> - Better commit comment for "doc: align 'diff --no-index' in text and 
> synopsis"
>   This includes Martin's `s/with/and/` comment.
> - Eric's typo fix in "doc: add note about shell quoting to revision.txt"
> - Added new patch for git-diff.txt with s/--options/options/.
>   This addresses Eric's and Martin's comments.

FWIW, this version looks good to me. I was a tiny bit surprised that
patch 7/7 was not patch 1/7. Could be just a matter of opinion,
probably nothing to reroll for. Thanks for getting back to this.

Martin

Re: git update-ref fails to create reference. (bug)

[Martin Ågren]

Hi Rafael,

On 4 May 2018 at 18:28, Rafael Ascensão  wrote:
> While trying to create a pseudo reference named REF pointing to the
> empty tree iff it doesn't exist, I stumbled on the following:
>
> I assume both are valid ways to create such reference:
> a) $ echo -e option no-deref\\nupdate REF $(git hash-object -t
> tree /dev/null)  | git
> update-ref --stdin
> b) $ git update-ref --no-deref REF $(git hash-object -t tree
> /dev/null) 
>
> While a) works, b) will throw:
> fatal: could not read ref 'REF'


I can reproduce this and I agree with your understanding of what should
happen here. The patch below makes this work according to my and your
expectations, at least in my command-line testing.

The die("... already exists") could instead be a no-op, trusting that
the backend discovers the problem. "die" could also be strbuf_addf(...),
I'm just following 2c3aed138 here.

Anyway, that's not where I'm stuck... Regardless of how I try to write
tests (in t1400), they just pass beautifully even before this patch. I
might be able to look into that more on the weekend. If anyone has
ideas, I am all ears. Or if someone feels like picking this up and
running with it, feel free.

Martin

diff --git a/refs.c b/refs.c
index 8b7a77fe5e..cdb0a5ab29 100644
--- a/refs.c
+++ b/refs.c
@@ -666,9 +666,12 @@ static int write_pseudoref(const char *pseudoref, const 
struct object_id *oid,
if (old_oid) {
struct object_id actual_old_oid;
 
-   if (read_ref(pseudoref, _old_oid))
-   die("could not read ref '%s'", pseudoref);
-   if (oidcmp(_old_oid, old_oid)) {
+   if (read_ref(pseudoref, _old_oid)) {
+   if (!is_null_oid(old_oid))
+   die("could not read ref '%s'", pseudoref);
+   } else if (is_null_oid(old_oid)) {
+   die("reference '%s' already exists", pseudoref);
+   } else if (oidcmp(_old_oid, old_oid)) {
strbuf_addf(err, "unexpected sha1 when writing '%s'", 
pseudoref);
rollback_lock_file();
goto done;
-- 
2.17.0.392.g7fa371e468

Re: What's cooking in git.git (Apr 2018, #04; Mon, 30)

[Elijah Newren]

Hi Junio,

On Sun, Apr 29, 2018 at 8:25 PM, Junio C Hamano  wrote:
> * en/rename-directory-detection-reboot (2018-04-25) 36 commits

>
>  Reboot of an attempt to detect wholesale directory renames and use
>  it while merging.
>
>  Will merge to 'next'.

Usually you have a mini-release-note in your "What's cooking" emails
next to the series, so I'm assuming from the text here that you might
just be intending to re-use the release note you used with the
original series.  For the rebooted series, it is probably worth adding
something to the release notes about how it also fixes the
unnecessary-update issue reported by Linus
(https://public-inbox.org/git/CA+55aFzLZ3UkG5svqZwSnhNk75=fxjrkvu1m_rhbg54nooa...@mail.gmail.com/),
a bug that's been with us for several years.

Elijah

Re: [PATCH v2] checkout & worktree: introduce checkout.implicitRemote

[Ævar Arnfjörð Bjarmason]

On Fri, May 04 2018, Duy Nguyen wrote:

> On Fri, May 4, 2018 at 9:54 AM, Ævar Arnfjörð Bjarmason
>  wrote:
>> Realistically the way we do hooks now would make the UI of this suck,
>> i.e. you couldn't configure it globally or system-wide easily. Something
>> like what I noted in
>> https://public-inbox.org/git/871sf3el01@evledraar.gmail.com/ would
>> make it suck less, but that's a much bigger task.
>
> I thought you would bring this up :) I've given some more thoughts on
> this topic and am willing to implement something like below, in a week
> or two. Would that help change your mind?
>
> I proposed hooks.* config space in that same thread. Here is the
> extension to make it cover both of your points.
>
> hooks.* can have multiple values. So you can specify
> hooks.post-checkout multiple times and all those scripts will run (in
> the same order you specified). Since we already have a search path for
> config (/etc/gitconfig -> $HOME/.gitconfig -> $REPO/config) this helps
> hooks management as well. Execution order is still the same: if you
> specify hooks.post-checkout in both /etc/gitconfig and .git/config,
> then the one in /etc/gitconfig is executed first, the one in .git
> second.
>
> And here's something extra to make it possible to override the search
> order: if you specify hooks.post-checkout = reset (reset is a random
> keyword) then _all_ post-checkout hooks before this point are ignored.
> So you can put this in .git/config
>
> [hooks]
> post-checkout = reset
> post-checkout = ~/some-hook
>
> and can be sure that post-checkout specified in $HOME and /etc will
> not affect you, only ~/some-hook will run. If you drop the second line
> then you have no post-checkout hooks. This is a workaround for a
> bigger problem (not being able to unset a config entry) but I think
> it's sufficient for this use case.

A few things:

 1) I don't see a reason to hold back this feature in particular waiting
for some way to do it via config / hooks. If we grow some compatible
way to do it via hooks in the future, great, then we can just make
this (and numerous other config values) historical aliases for that
facility.

 2) Let's not have some per-config type way to reset earlier config. I
suggested a way to do it generally in
https://public-inbox.org/git/874lkq11ug@evledraar.gmail.com/ I'm
not saying we should go for that suggestion in particular, but that
we should have something general.

 3) Doing #2 will take a lot longer to implement than what you're
suggesting.

 4) I think such a facility should also be able to replace something
like https://docs.gitlab.com/ee/administration/custom_hooks.html
which requires not just supporting hooks from the config, but
executing some hooks on the FS in glob() order.

 5) What you're describing above is just 1/2 of what we need to have a
viable way to replace something like this checkout.implicitRemote
with a hook while providing the same functionality to the end
user.

If something ships as a config value like checkout.implicitRemote
users can just turn it on in their ~/.gitconfig without any further
config, or you can tell users in docs via one-liner to enable it.

We also need the other half which some method of shipping "standard"
hooks with git, i.e. with your proposal something like (along with
the general config reset):

[config]
reject = hooks.post-checkout
[hooks]
# Reads config from hooks.post-checkout-implicit-remote.*
# (e.g. hooks.post-checkout-implicit-remote.remote = origin)
post-checkout = git-hooks://post-checkout-implicit-remote-config

   Only then will this be as easy to enable as `git config --global..`
   (although you'll need two invocations of that, which is fine...)

 6) The feature being discussed here would not be a post-checkout hook,
but would need to be fairly integrated with the internals of
checkout, see `dwim_ok` and the big comment in
parse_branchname_arg().

I.e. the thing that makes this work is the Nth step in some fairly
intricate fallback logic. It's not clear to me in the general case
how we'd turn this into a hook without having N number of
checkout-what-step-N-hooks, or requiring every checkout-what hook to
implement a huge part of what checkout.c is doing now just to tweak
some tiny aspect of it, such as this tweak of
unique_tracking_name().

Re: Hello My Dear Friend!!!!

[SISTER DOROTHY KENT]

Hello Dear

I am Sister Dorothy Kent I really need your assistance to help me discuss a 
project .

Thanks,
Sister Dorothy Kent

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Elijah Newren]

On Thu, May 3, 2018 at 11:40 PM, Johannes Schindelin
 wrote:
> I actually have a hacky script to fixup commits in a patch series. It lets
> me stage part of the current changes, then figures out which of the
> commits' changes overlap with the staged changed. If there is only one
> commit, it automatically commits with --fixup, otherwise it lets me choose
> which one I want to fixup (giving me the list of candidates).

Ooh, interesting.  Are you willing to share said hacky script by chance?

(And as a total aside, I found your apply-from-public-inbox.sh script
and really like it.  Thanks for making it public.)

Re: [PATCH] alloc.c: replace alloc by mempool

[Duy Nguyen]

On Fri, May 4, 2018 at 12:18 AM, Stefan Beller  wrote:
> I just measured on git.git and linux.git (both of which are not *huge* by
> any standard, but should give a good indication. linux has  6M objects,
> and when allocating 1024 at a time, we run into the new block allocation
> ~6000 times).
>
> I could not measure any meaningful difference.
>
> linux.git $ git count-objects -v
> count: 0
> size: 0
> in-pack: 6036543
> packs: 2
> size-pack: 2492985
> prune-packable: 0
> garbage: 0
> size-garbage: 0
>
> (with this patch)
>  Performance counter stats for '/u/git/git count-objects -v' (30 runs):
>
>   2.123683  task-clock:u (msec)   #0.831 CPUs utilized
> ( +-  2.32% )
>  0  context-switches:u#0.000 K/sec
>  0  cpu-migrations:u  #0.000 K/sec
>126  page-faults:u #0.059 M/sec
> ( +-  0.22% )
>895,900  cycles:u  #0.422 GHz  
> ( +-  1.40% )
>976,596  instructions:u#1.09  insn per cycle   
> ( +-  0.01% )
>218,256  branches:u#  102.772 M/sec
> ( +-  0.01% )
>  8,331  branch-misses:u   #3.82% of all branches  
> ( +-  0.61% )
>
>0.002556203 seconds time elapsed   
>( +-  2.20% )
>
>   Performance counter stats for 'git count-objects -v' (30 runs):
>
>   2.410352  task-clock:u (msec)   #0.801 CPUs utilized
> ( +-  2.79% )
>  0  context-switches:u#0.000 K/sec
>  0  cpu-migrations:u  #0.000 K/sec
>131  page-faults:u #0.054 M/sec
> ( +-  0.16% )
>993,301  cycles:u  #0.412 GHz  
> ( +-  1.99% )
>  1,087,428  instructions:u#1.09  insn per cycle   
> ( +-  0.02% )
>244,292  branches:u#  101.351 M/sec
> ( +-  0.02% )
>  9,264  branch-misses:u   #3.79% of all branches  
> ( +-  0.57% )
>
>0.003010854 seconds time elapsed   
>( +-  2.54% )
>
> So I think we could just replace it for now and optimize again later, if it
> turns out to be a problem. I think the easiest optimisation is to increase
> the allocation size of having a lot more objects per mp_block.

Yeah. I also tested this from a different angle: memory overhead. For
2M objects with one mp_block containing 1024 objects (same setting as
alloc.c), the overhead (not counting malloc() internal overhead) is
46KB and we don't have any extra overhead due to padding between
objects. This is true for all struct blob, commit, tree and tag. This
is really good. alloc.c has zero overhead when measured this way but
46KB is practically zero to me.
-- 
Duy

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

[Elijah Newren]

Hi,

On Fri, May 4, 2018 at 9:21 AM, Elijah Newren  wrote:
> On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
>  wrote:
>> The incredibly useful `git-tbdiff` tool to compare patch series (say, to see
>> what changed between two iterations sent to the Git mailing list) is slightly
>> less useful for this developer due to the fact that it requires the 
>> `hungarian`
>> and `numpy` Python packages which are for some reason really hard to build in
>> MSYS2. So hard that I even had to give up, because it was simply easier to
>> reimplement the whole shebang as a builtin command.
>
> tbdiff is awesome; thanks for bringing it in as a builtin to git.
>
> I've run through a few cases, comparing output of tbdiff and
> branch-diff.  So far, what I've noted is that they produce largely the
> same output except that:
>
> - tbdiff seems to shorten shas to 7 characters, branch-diff is using
> 10, in git.git at least.  (Probably a good change)

Sorry, a quick self-correction here:

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

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

git update-ref fails to create reference. (bug)

[Rafael Ascensão]

While trying to create a pseudo reference named REF pointing to the
empty tree iff it doesn't exist, I stumbled on the following:

I assume both are valid ways to create such reference:
a) $ echo -e option no-deref\\nupdate REF $(git hash-object -t tree 
/dev/null)  | git update-ref --stdin
b) $ git update-ref --no-deref REF $(git hash-object -t tree /dev/null) 


While a) works, b) will throw:
fatal: could not read ref 'REF'

Bisect seems to point to:
2c3aed138 (pseudoref: check return values from read_ref(), 2015-07-15)

Thanks,
Rafael Ascensão

Re: [PATCH v2 09/18] branch-diff: adjust the output of the commit pairs

[Elijah Newren]

Hi Dscho,

On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
 wrote:
> This change brings branch-diff yet another step closer to feature parity
> with tbdiff: it now shows the oneline, too, and indicates with `=` when
> the commits have identical diffs.
>

> @@ -270,9 +272,57 @@ static int get_correspondences(struct string_list *a, 
> struct string_list *b,
> return res;
>  }
>
> -static const char *short_oid(struct patch_util *util)
> +static void output_pair_header(struct strbuf *buf,
> +  int i, struct patch_util *a_util,
> +  int j, struct patch_util *b_util)
>  {
> -   return find_unique_abbrev(>oid, DEFAULT_ABBREV);
> +   static char *dashes;
> +   struct object_id *oid = a_util ? _util->oid : _util->oid;
> +   struct commit *commit;
> +
> +   if (!dashes) {
> +   char *p;
> +
> +   dashes = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));
> +   for (p = dashes; *p; p++)
> +   *p = '-';
> +   }
> +
> +   strbuf_reset(buf);
> +   if (i < 0)
> +   strbuf_addf(buf, "-:  %s ", dashes);
> +   else
> +   strbuf_addf(buf, "%d:  %s ", i + 1,


One nice thing tbdiff did was to right align patch numbers (which also
helped align other columns in the output).  So, for example when there
are more than 9 patches I would see output like:

...
 8: a980de43fd =  8: 362ab315ac directory rename detection: testcases
exploring possibly suboptimal merges
 9: 3633e79ed9 =  9: 792e1371d9 directory rename detection:
miscellaneous testcases to complete coverage
10: e10d07ef40 = 10: a0b0a15103 directory rename detection: tests for
handling overwriting untracked files
11: f6d84b503e = 11: a7a436042a directory rename detection: tests for
handling overwriting dirty files
...

whereas branch-diff here is instead giving output of the form

...
8:  a980de43fd = 8:  362ab315ac directory rename detection: testcases
exploring possibly suboptimal merges
9:  3633e79ed9 = 9:  792e1371d9 directory rename detection:
miscellaneous testcases to complete coverage
10:  e10d07ef40 = 10:  a0b0a15103 directory rename detection: tests
for handling overwriting untracked files
11:  f6d84b503e = 11:  a7a436042a directory rename detection: tests
for handling overwriting dirty files
...


Not a critical difference, but it'd be nice to match tbdiff here all the same.

> +   find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
> +
> +   if (i < 0)
> +   strbuf_addch(buf, '>');
> +   else if (j < 0)
> +   strbuf_addch(buf, '<');
> +   else if (strcmp(a_util->patch, b_util->patch))
> +   strbuf_addch(buf, '!');
> +   else
> +   strbuf_addch(buf, '=');
> +
> +   if (j < 0)
> +   strbuf_addf(buf, " -:  %s", dashes);
> +   else
> +   strbuf_addf(buf, " %d:  %s", j + 1,

Same comment on these last two strbuf_addf's about alignment.



Elijah

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

[Elijah Newren]

On Fri, May 4, 2018 at 8:34 AM, Johannes Schindelin
 wrote:
> The incredibly useful `git-tbdiff` tool to compare patch series (say, to see
> what changed between two iterations sent to the Git mailing list) is slightly
> less useful for this developer due to the fact that it requires the 
> `hungarian`
> and `numpy` Python packages which are for some reason really hard to build in
> MSYS2. So hard that I even had to give up, because it was simply easier to
> reimplement the whole shebang as a builtin command.

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

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

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

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


Elijah

Respond for details

[Mr. Allen]

I can help you secure a private loan , should you be interested please respond 
for more details . 


Thanks 

Allen 

Re: [PATCH v2 1/3] upload-pack: fix error message typo

[Jonathan Tan]

On Fri, 04 May 2018 11:24:39 +0900
Junio C Hamano  wrote:

> Hmm, when somebody breaks "git server serve", we probably would not
> want to see the binary spewed to the output while debugging it.  So
> I'd probably keep the redirection---it may be an improvement to use
> ">out" and then checking it is empty after the expected failure.

That's a good point - I've readded the redirection in my local copy.
I'll send out the new version if needed.

I checked the other patches and patch 3 has a similar situation but
still has the >/dev/null because I forgot to remove it when I removed it
in patch 1, so nothing needs to be changed in patch 3.

Re: Fetching tags overwrites existing tags

[Jacob Keller]

On Fri, Apr 27, 2018 at 12:13 PM, Bryan Turner  wrote:
> On Fri, Apr 27, 2018 at 12:08 PM, Wink Saville  wrote:
>>
>> The other change was rather than using 
>> ""+refs/tags/*:refs/remote-tags/$name/*"
>> I've changed it to "+refs/tags/*:refs/remote/tags/$name/*" which seems 
>> cleaner.
>> Again, if remote-tags is preferred I'll change it back.
>
>
> From looking at the code, it looks like you mean
> "+refs/tags/*:refs/remotes/tags/$name/*".
>
> The issue with that approach is that it collides with a remote named
> "tags". "refs/remote-tags", on the other hand, represents a new-to-Git
> path, one that won't already be in use by any other standard
> functionality. That seems like a better approach than hoping no one
> out there will call one of their remotes "tags".
>
> Bryan

Note that my suggestion was very specific "remote" not pluralized,
which is obviously a bit confusing, since there's remote and
"remotes".

The goal being that you put "remote//" followed by the full
remote ref minus the refs prefix.

It specifically is attempting to avoid the problem of expanding
"remotes". Unfortunately, I don't have a better alternative format,
and i very much want to avoid having to do "remote-tags",
"remote-notes", "remote-replaces", "remote-meta" etc...

In that spirit, I'm working to hopefully propose something today.

Thanks,
Jake

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Ramsay Jones]

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

[PATCH v2 16/18] branch-diff --dual-color: work around bogus white-space warning

[Johannes Schindelin]

When displaying a diff of diffs, it is possible that there is an outer
`+` before a context line. That happens when the context changed between
old and new commit. When that context line starts with a tab (after the
space that marks it as context line), our diff machinery spits out a
white-space error (space before tab), but in this case, that is
incorrect.

Work around this by detecting that situation and simply *not* printing
the space in that case.

This is slightly improper a fix because it is conceivable that an
output_prefix might be configured with *just* the right length to let
that tab jump to a different tab stop depending whether we emit that
space or not.

However, the proper fix would be relatively ugly and intrusive because
it would have to *weaken* the WS_SPACE_BEFORE_TAB option in ws.c.
Besides, we do not expose the --dual-color option in cases other than
the `branch-diff` command, which only uses a hard-coded output_prefix of
four spaces (which misses the problem by one column ;-)).

Signed-off-by: Johannes Schindelin 
---
 diff.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/diff.c b/diff.c
index 98a41e88620..b98a18fe014 100644
--- a/diff.c
+++ b/diff.c
@@ -1098,6 +1098,12 @@ static void emit_diff_symbol_from_struct(struct 
diff_options *o,
set = diff_get_color_opt(o, DIFF_FILE_OLD);
else if (c != '+')
set = diff_get_color_opt(o, DIFF_CONTEXT);
+   /* Avoid space-before-tab warning */
+   if (c == ' ' && (len < 2 || line[1] == '\t' ||
+line[1] == '\r' || line[1] == '\n')) {
+   line++;
+   len--;
+   }
}
emit_line_ws_markup(o, set, reset, line, len, set_sign, '+',
flags & DIFF_SYMBOL_CONTENT_WS_MASK,
-- 
2.17.0.409.g71698f11835

[PATCH v2 04/18] branch-diff: improve the order of the shown commits

[Johannes Schindelin]

This patch lets branch-diff use the same order as tbdiff.

The idea is simple: for left-to-right readers, it is natural to assume
that the branch-diff is performed between an older vs a newer version of
the branch. As such, the user is probably more interested in the
question "where did this come from?" rather than "where did that one
go?".

To that end, we list the commits in the order of the second commit range
("the newer version"), inserting the unmatched commits of the first
commit range as soon as all their predecessors have been shown.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 59 +--
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index c462681067c..92302b1c339 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -31,7 +31,7 @@ struct patch_util {
struct hashmap_entry e;
const char *diff, *patch;
 
-   int i;
+   int i, shown;
int diffsize;
size_t diff_offset;
/* the index of the matching item in the other branch, or -1 */
@@ -274,28 +274,49 @@ static const char *short_oid(struct patch_util *util)
 
 static void output(struct string_list *a, struct string_list *b)
 {
-   int i;
-
-   for (i = 0; i < b->nr; i++) {
-   struct patch_util *util = b->items[i].util, *prev;
+   int i = 0, j = 0;
+
+   /*
+* We assume the user is really more interested in the second argument
+* ("newer" version). To that end, we print the output in the order of
+* the RHS (the `b` parameter). To put the LHS (the `a` parameter)
+* commits that are no longer in the RHS into a good place, we place
+* them once we have shown all of their predecessors in the LHS.
+*/
+
+   while (i < a->nr || j < b->nr) {
+   struct patch_util *a_util, *b_util;
+   a_util = i < a->nr ? a->items[i].util : NULL;
+   b_util = j < b->nr ? b->items[j].util : NULL;
+
+   /* Skip all the already-shown commits from the LHS. */
+   while (i < a->nr && a_util->shown)
+   a_util = ++i < a->nr ? a->items[i].util : NULL;
+
+   /* Show unmatched LHS commit whose predecessors were shown. */
+   if (i < a->nr && a_util->matching < 0) {
+   printf("%d: %s < -: \n",
+  i + 1, short_oid(a_util));
+   i++;
+   continue;
+   }
 
-   if (util->matching < 0)
+   /* Show unmatched RHS commits. */
+   while (j < b->nr && b_util->matching < 0) {
printf("-:  > %d: %s\n",
-   i + 1, short_oid(util));
-   else {
-   prev = a->items[util->matching].util;
-   printf("%d: %s ! %d: %s\n",
-  util->matching + 1, short_oid(prev),
-  i + 1, short_oid(util));
+  j + 1, short_oid(b_util));
+   b_util = ++j < b->nr ? b->items[j].util : NULL;
}
-   }
-
-   for (i = 0; i < a->nr; i++) {
-   struct patch_util *util = a->items[i].util;
 
-   if (util->matching < 0)
-   printf("%d: %s < -: \n",
-  i + 1, short_oid(util));
+   /* Show matching LHS/RHS pair. */
+   if (j < b->nr) {
+   a_util = a->items[b_util->matching].util;
+   printf("%d: %s ! %d: %s\n",
+  b_util->matching + 1, short_oid(a_util),
+  j + 1, short_oid(b_util));
+   a_util->shown = 1;
+   j++;
+   }
}
 }
 
-- 
2.17.0.409.g71698f11835

[PATCH v2 14/18] diff: add an internal option to dual-color diffs of diffs

[Johannes Schindelin]

When diffing diffs, it can be quite daunting to figure out what the heck
is going on, as there are nested +/- signs.

Let's make this easier by adding a flag in diff_options that allows
color-coding the outer diff sign with inverted colors, so that the
preimage and postimage is colored like the diff it is.

Of course, this really only makes sense when the preimage and postimage
*are* diffs. So let's not expose this flag via a command-line option for
now.

This is a feature that was invented by git-tbdiff, and it will be used
in `branch-diff` in the next commit.

Signed-off-by: Johannes Schindelin 
---
 diff.c | 65 +-
 diff.h |  5 -
 2 files changed, 59 insertions(+), 11 deletions(-)

diff --git a/diff.c b/diff.c
index f1bda0db3f5..98a41e88620 100644
--- a/diff.c
+++ b/diff.c
@@ -67,6 +67,8 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_BOLD_YELLOW,  /* NEW_MOVED ALTERNATIVE */
GIT_COLOR_FAINT,/* NEW_MOVED_DIM */
GIT_COLOR_FAINT_ITALIC, /* NEW_MOVED_ALTERNATIVE_DIM */
+   GIT_COLOR_INV_RED,  /* OLD_INV */
+   GIT_COLOR_INV_GREEN,/* NEW_INV */
 };
 
 static NORETURN void die_want_option(const char *option_name)
@@ -108,6 +110,10 @@ static int parse_diff_color_slot(const char *var)
return DIFF_FILE_NEW_MOVED_DIM;
if (!strcasecmp(var, "newmovedalternativedimmed"))
return DIFF_FILE_NEW_MOVED_ALT_DIM;
+   if (!strcasecmp(var, "oldinv"))
+   return DIFF_FILE_OLD_INV;
+   if (!strcasecmp(var, "newinv"))
+   return DIFF_FILE_NEW_INV;
return -1;
 }
 
@@ -577,7 +583,10 @@ static void emit_line_0(struct diff_options *o, const char 
*set, const char *res
int nofirst;
FILE *file = o->file;
 
-   fputs(diff_line_prefix(o), file);
+   if (first)
+   fputs(diff_line_prefix(o), file);
+   else if (!len)
+   return;
 
if (len == 0) {
has_trailing_newline = (first == '\n');
@@ -596,7 +605,7 @@ static void emit_line_0(struct diff_options *o, const char 
*set, const char *res
 
if (len || !nofirst) {
fputs(set, file);
-   if (!nofirst)
+   if (first && !nofirst)
fputc(first, file);
fwrite(line, len, 1, file);
fputs(reset, file);
@@ -970,7 +979,8 @@ static void dim_moved_lines(struct diff_options *o)
 
 static void emit_line_ws_markup(struct diff_options *o,
const char *set, const char *reset,
-   const char *line, int len, char sign,
+   const char *line, int len,
+   const char *set_sign, char sign,
unsigned ws_rule, int blank_at_eof)
 {
const char *ws = NULL;
@@ -981,14 +991,18 @@ static void emit_line_ws_markup(struct diff_options *o,
ws = NULL;
}
 
-   if (!ws)
+   if (!ws && set_sign == set)
emit_line_0(o, set, reset, sign, line, len);
-   else if (blank_at_eof)
+   else if (!ws) {
+   /* Emit just the prefix, then the rest. */
+   emit_line_0(o, set_sign, reset, sign, "", 0);
+   emit_line_0(o, set, reset, 0, line, len);
+   } else if (blank_at_eof)
/* Blank line at EOF - paint '+' as well */
emit_line_0(o, ws, reset, sign, line, len);
else {
/* Emit just the prefix, then the rest. */
-   emit_line_0(o, set, reset, sign, "", 0);
+   emit_line_0(o, set_sign, reset, sign, "", 0);
ws_check_emit(line, len, ws_rule,
  o->file, set, reset, ws);
}
@@ -998,7 +1012,7 @@ static void emit_diff_symbol_from_struct(struct 
diff_options *o,
 struct emitted_diff_symbol *eds)
 {
static const char *nneof = " No newline at end of file\n";
-   const char *context, *reset, *set, *meta, *fraginfo;
+   const char *context, *reset, *set, *set_sign, *meta, *fraginfo;
struct strbuf sb = STRBUF_INIT;
 
enum diff_symbol s = eds->s;
@@ -1038,7 +1052,16 @@ static void emit_diff_symbol_from_struct(struct 
diff_options *o,
case DIFF_SYMBOL_CONTEXT:
set = diff_get_color_opt(o, DIFF_CONTEXT);
reset = diff_get_color_opt(o, DIFF_RESET);
-   emit_line_ws_markup(o, set, reset, line, len, ' ',
+   set_sign = set;
+   if (o->flags.dual_color_diffed_diffs) {
+   char c = !len ? 0 : line[0];
+
+   if (c == '+')
+   set = diff_get_color_opt(o, DIFF_FILE_NEW);
+   else if (c == '-')
+   set = diff_get_color_opt(o, 

[PATCH v2 12/18] branch-diff: use color for the commit pairs

[Johannes Schindelin]

Arguably the most important part of branch-diff's output is the list of
commits in the two branches, together with their relationships.

For that reason, tbdiff introduced color-coding that is pretty
intuitive, especially for unchanged patches (all dim yellow, like the
first line in `git show`'s output) vs modified patches (old commit is
red, new commit is green). Let's imitate that color scheme.

While at it, also copy tbdiff's change of the fragment color to magenta.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 49 +++
 1 file changed, 36 insertions(+), 13 deletions(-)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index 89d75c93115..04efd30f0f6 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -273,13 +273,19 @@ static int get_correspondences(struct string_list *a, 
struct string_list *b,
return res;
 }
 
-static void output_pair_header(struct strbuf *buf,
+static void output_pair_header(struct diff_options *diffopt, struct strbuf 
*buf,
   int i, struct patch_util *a_util,
   int j, struct patch_util *b_util)
 {
static char *dashes;
struct object_id *oid = a_util ? _util->oid : _util->oid;
struct commit *commit;
+   char status;
+   const char *color_reset = diff_get_color_opt(diffopt, DIFF_RESET);
+   const char *color_old = diff_get_color_opt(diffopt, DIFF_FILE_OLD);
+   const char *color_new = diff_get_color_opt(diffopt, DIFF_FILE_NEW);
+   const char *color_commit = diff_get_color_opt(diffopt, DIFF_COMMIT);
+   const char *color;
 
if (!dashes) {
char *p;
@@ -289,21 +295,33 @@ static void output_pair_header(struct strbuf *buf,
*p = '-';
}
 
+   if (j < 0) {
+   color = color_old;
+   status = '<';
+   } else if (i < 0) {
+   color = color_new;
+   status = '>';
+   } else if (strcmp(a_util->patch, b_util->patch)) {
+   color = color_commit;
+   status = '!';
+   } else {
+   color = color_commit;
+   status = '=';
+   }
+
strbuf_reset(buf);
+   strbuf_addstr(buf, status == '!' ? color_old : color);
if (i < 0)
strbuf_addf(buf, "-:  %s ", dashes);
else
strbuf_addf(buf, "%d:  %s ", i + 1,
find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
 
-   if (i < 0)
-   strbuf_addch(buf, '>');
-   else if (j < 0)
-   strbuf_addch(buf, '<');
-   else if (strcmp(a_util->patch, b_util->patch))
-   strbuf_addch(buf, '!');
-   else
-   strbuf_addch(buf, '=');
+   if (status == '!')
+   strbuf_addf(buf, "%s%s", color_reset, color);
+   strbuf_addch(buf, status);
+   if (status == '!')
+   strbuf_addf(buf, "%s%s", color_reset, color_new);
 
if (j < 0)
strbuf_addf(buf, " -:  %s", dashes);
@@ -316,12 +334,15 @@ static void output_pair_header(struct strbuf *buf,
const char *commit_buffer = get_commit_buffer(commit, NULL);
const char *subject;
 
+   if (status == '!')
+   strbuf_addf(buf, "%s%s", color_reset, color);
+
find_commit_subject(commit_buffer, );
strbuf_addch(buf, ' ');
format_subject(buf, subject, " ");
unuse_commit_buffer(commit, commit_buffer);
}
-   strbuf_addch(buf, '\n');
+   strbuf_addf(buf, "%s\n", color_reset);
 
fwrite(buf->buf, buf->len, 1, stdout);
 }
@@ -384,21 +405,21 @@ static void output(struct string_list *a, struct 
string_list *b,
 
/* Show unmatched LHS commit whose predecessors were shown. */
if (i < a->nr && a_util->matching < 0) {
-   output_pair_header(, i, a_util, -1, NULL);
+   output_pair_header(diffopt, , i, a_util, -1, NULL);
i++;
continue;
}
 
/* Show unmatched RHS commits. */
while (j < b->nr && b_util->matching < 0) {
-   output_pair_header(, -1, NULL, j, b_util);
+   output_pair_header(diffopt, , -1, NULL, j, b_util);
b_util = ++j < b->nr ? b->items[j].util : NULL;
}
 
/* Show matching LHS/RHS pair. */
if (j < b->nr) {
a_util = a->items[b_util->matching].util;
-   output_pair_header(,
+   output_pair_header(diffopt, ,
   b_util->matching, a_util, j, b_util);
if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))

[PATCH v2 03/18] branch-diff: first rudimentary implementation

[Johannes Schindelin]

At this stage, `git branch-diff` can determine corresponding commits of
two related commit ranges. This makes use of the recently introduced
implementation of the Hungarian algorithm.

The core of this patch is a straight port of the ideas of tbdiff, the
seemingly dormant project at https://github.com/trast/tbdiff.

The output does not at all match `tbdiff`'s output yet, as this patch
really concentrates on getting the patch matching part right.

Note: due to differences in the diff algorithm (`tbdiff` uses the
Python 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).

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 335 +-
 1 file changed, 334 insertions(+), 1 deletion(-)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index 60a4b4fbe30..c462681067c 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -1,6 +1,12 @@
 #include "cache.h"
 #include "builtin.h"
 #include "parse-options.h"
+#include "string-list.h"
+#include "run-command.h"
+#include "argv-array.h"
+#include "hashmap.h"
+#include "xdiff-interface.h"
+#include "hungarian.h"
 
 static const char * const builtin_branch_diff_usage[] = {
 N_("git branch-diff [] .. .."),
@@ -20,6 +26,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(, "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())
+   return error_errno(_("could not start `log`"));
+   in = fdopen(cp.out, "r");
+   if (!in) {
+   error_errno(_("could not read `log` output"));
+   finish_command();
+   return -1;
+   }
+
+   while (strbuf_getline(, in) != EOF) {
+   const char *p;
+
+   if (skip_prefix(line.buf, "commit ", )) {
+   if (util) {
+   string_list_append(list, buf.buf)->util = util;
+   strbuf_reset();
+   }
+   util = xcalloc(sizeof(*util), 1);
+   if (get_oid(p, >oid)) {
+   error(_("could not parse commit '%s'"), p);
+   free(util);
+   string_list_clear(list, 1);
+   strbuf_release();
+   strbuf_release();
+   fclose(in);
+   finish_command();
+   return -1;
+   }
+   util->matching = -1;
+   in_header = 1;
+   continue;
+   }
+
+   if (starts_with(line.buf, "diff --git")) {
+   in_header = 0;
+   strbuf_addch(, '\n');
+   if (!util->diff_offset)
+   util->diff_offset = buf.len;
+   strbuf_addbuf(, );
+   } else if (in_header) {
+   if (starts_with(line.buf, "Author: ")) {
+   strbuf_addbuf(, );
+   strbuf_addstr(, "\n\n");
+   } else if (starts_with(line.buf, "")) {
+   strbuf_addbuf(, );
+   strbuf_addch(, '\n');
+   }
+   continue;
+   } else if (starts_with(line.buf, "@@ "))
+   strbuf_addstr(, "@@");
+   else if (line.buf[0] && !starts_with(line.buf, "index "))
+   /*
+* A completely blank (not ' \n', which is context)
+   

[PATCH v2 18/18] completion: support branch-diff

[Johannes Schindelin]

Tab completion of `branch-diff` is very convenient, especially given
that the revision arguments that need to be passed to `git branch-diff`
are typically more complex than, say, your grandfather's `git log`
arguments.

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 braorimas` would expand to
`git branch origin/master`, as we now no longer automatically append a
space after completing `git branch`: this is now ambiguous.

Signed-off-by: Johannes Schindelin 
---
 contrib/completion/git-completion.bash | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 01dd9ff07a2..45addd525ac 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1496,6 +1496,24 @@ _git_format_patch ()
__git_complete_revlist
 }
 
+__git_branch_diff_options="
+   --no-patches --creation-weight= --dual-color
+"
+
+_git_branch_diff ()
+{
+   case "$cur" in
+   --*)
+   __gitcomp "
+   $__git_branch_diff_options
+   $__git_diff_common_options
+   "
+   return
+   ;;
+   esac
+   __git_complete_revlist
+}
+
 _git_fsck ()
 {
case "$cur" in
-- 
2.17.0.409.g71698f11835

[PATCH v2 15/18] branch-diff: offer to dual-color the diffs

[Johannes Schindelin]

When showing what changed between old and new commits, we show a diff of
the patches. This diff is a diff between diffs, therefore there are
nested +/- signs, and it can be relatively hard to understand what is
going on.

With the --dual-color option, the preimage and the postimage are colored
like the diffs they are, and the *outer* +/- sign is inverted for
clarity.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index 04efd30f0f6..8a16352e3a1 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -435,8 +435,11 @@ int cmd_branch_diff(int argc, const char **argv, const 
char *prefix)
 {
struct diff_options diffopt = { NULL };
struct strbuf four_spaces = STRBUF_INIT;
+   int dual_color = 0;
double creation_weight = 0.6;
struct option options[] = {
+   OPT_BOOL(0, "dual-color", _color,
+   N_("color both diff and diff-between-diffs")),
OPT_SET_INT(0, "no-patches", _format,
N_("short format (no diffs)"),
DIFF_FORMAT_NO_OUTPUT),
@@ -472,6 +475,11 @@ int cmd_branch_diff(int argc, const char **argv, const 
char *prefix)
argc = j;
diff_setup_done();
 
+   if (dual_color) {
+   diffopt.use_color = 1;
+   diffopt.flags.dual_color_diffed_diffs = 1;
+   }
+
if (argc == 2) {
if (!strstr(argv[0], ".."))
warning(_("no .. in range: '%s'"), argv[0]);
-- 
2.17.0.409.g71698f11835

[PATCH v2 13/18] color: provide inverted colors, too

[Johannes Schindelin]

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.

Signed-off-by: Johannes Schindelin 
---
 color.h | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/color.h b/color.h
index cd0bcedd084..f0984b09583 100644
--- a/color.h
+++ b/color.h
@@ -36,6 +36,12 @@ struct strbuf;
 #define GIT_COLOR_BOLD_BLUE"\033[1;34m"
 #define GIT_COLOR_BOLD_MAGENTA "\033[1;35m"
 #define GIT_COLOR_BOLD_CYAN"\033[1;36m"
+#define GIT_COLOR_INV_RED  "\033[7;31m"
+#define GIT_COLOR_INV_GREEN"\033[7;32m"
+#define GIT_COLOR_INV_YELLOW   "\033[7;33m"
+#define GIT_COLOR_INV_BLUE "\033[7;34m"
+#define GIT_COLOR_INV_MAGENTA  "\033[7;35m"
+#define GIT_COLOR_INV_CYAN "\033[7;36m"
 #define GIT_COLOR_BG_RED   "\033[41m"
 #define GIT_COLOR_BG_GREEN "\033[42m"
 #define GIT_COLOR_BG_YELLOW"\033[43m"
-- 
2.17.0.409.g71698f11835

[PATCH v2 06/18] branch-diff: right-trim commit messages

[Johannes Schindelin]

When comparing commit messages, we need to keep in mind that they are
indented by four spaces. That is, empty lines are no longer empty, but
have "trailing whitespace". When displaying them in color, that results
in those nagging red lines.

Let's just right-trim the lines in the commit message, it's not like
trailing white-space in the commit messages are important enough to care
about in branch-diff.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index b23d66a3b1c..e2337b905b1 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -105,6 +105,7 @@ static int read_patches(const char *range, struct 
string_list *list)
strbuf_addbuf(, );
strbuf_addstr(, "\n\n");
} else if (starts_with(line.buf, "")) {
+   strbuf_rtrim();
strbuf_addbuf(, );
strbuf_addch(, '\n');
}
-- 
2.17.0.409.g71698f11835

[PATCH v2 07/18] branch-diff: indent the diffs just like tbdiff

[Johannes Schindelin]

The main information in the branch-diff view comes from the list of
matching and non-matching commits, the diffs are additional information.
Indenting them helps with the reading flow.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index e2337b905b1..4fc9fd74531 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -275,6 +275,11 @@ static const char *short_oid(struct patch_util *util)
return find_unique_abbrev(>oid, DEFAULT_ABBREV);
 }
 
+static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
+{
+   return data;
+}
+
 static struct diff_filespec *get_filespec(const char *name, const char *p)
 {
struct diff_filespec *spec = alloc_filespec(name);
@@ -353,6 +358,7 @@ static void output(struct string_list *a, struct 
string_list *b,
 int cmd_branch_diff(int argc, const char **argv, const char *prefix)
 {
struct diff_options diffopt = { NULL };
+   struct strbuf four_spaces = STRBUF_INIT;
double creation_weight = 0.6;
struct option options[] = {
OPT_SET_INT(0, "no-patches", _format,
@@ -371,6 +377,9 @@ int cmd_branch_diff(int argc, const char **argv, const char 
*prefix)
 
diff_setup();
diffopt.output_format = DIFF_FORMAT_PATCH;
+   diffopt.output_prefix = output_prefix_cb;
+   strbuf_addstr(_spaces, "");
+   diffopt.output_prefix_data = _spaces;
 
argc = parse_options(argc, argv, NULL, options,
builtin_branch_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
-- 
2.17.0.409.g71698f11835

[PATCH v2 08/18] branch-diff: suppress the diff headers

[Johannes Schindelin]

When showing the diff between corresponding patches of the two branch
versions, we have to make up a fake filename to run the diff machinery.

That filename does not carry any meaningful information, hence tbdiff
suppresses it. So we should, too.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 1 +
 diff.c| 5 -
 diff.h| 1 +
 3 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index 4fc9fd74531..ed520d6229d 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -377,6 +377,7 @@ int cmd_branch_diff(int argc, const char **argv, const char 
*prefix)
 
diff_setup();
diffopt.output_format = DIFF_FORMAT_PATCH;
+   diffopt.flags.suppress_diff_headers = 1;
diffopt.output_prefix = output_prefix_cb;
strbuf_addstr(_spaces, "");
diffopt.output_prefix_data = _spaces;
diff --git a/diff.c b/diff.c
index 1289df4b1f9..f1bda0db3f5 100644
--- a/diff.c
+++ b/diff.c
@@ -3197,13 +3197,16 @@ static void builtin_diff(const char *name_a,
memset(, 0, sizeof(xpp));
memset(, 0, sizeof(xecfg));
memset(, 0, sizeof(ecbdata));
+   if (o->flags.suppress_diff_headers)
+   lbl[0] = NULL;
ecbdata.label_path = lbl;
ecbdata.color_diff = want_color(o->use_color);
ecbdata.ws_rule = whitespace_rule(name_b);
if (ecbdata.ws_rule & WS_BLANK_AT_EOF)
check_blank_at_eof(, , );
ecbdata.opt = o;
-   ecbdata.header = header.len ?  : NULL;
+   if (header.len && !o->flags.suppress_diff_headers)
+   ecbdata.header = 
xpp.flags = o->xdl_opts;
xpp.anchors = o->anchors;
xpp.anchors_nr = o->anchors_nr;
diff --git a/diff.h b/diff.h
index d29560f822c..0dd6a71af60 100644
--- a/diff.h
+++ b/diff.h
@@ -94,6 +94,7 @@ struct diff_flags {
unsigned funccontext:1;
unsigned default_follow_renames:1;
unsigned stat_with_summary:1;
+   unsigned suppress_diff_headers:1;
 };
 
 static inline void diff_flags_or(struct diff_flags *a,
-- 
2.17.0.409.g71698f11835

[PATCH v2 09/18] branch-diff: adjust the output of the commit pairs

[Johannes Schindelin]

This change brings branch-diff yet another step closer to feature parity
with tbdiff: it now shows the oneline, too, and indicates with `=` when
the commits have identical diffs.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 67 +--
 1 file changed, 58 insertions(+), 9 deletions(-)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index ed520d6229d..5b187890bdf 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -9,6 +9,8 @@
 #include "hungarian.h"
 #include "diff.h"
 #include "diffcore.h"
+#include "commit.h"
+#include "pretty.h"
 
 static const char * const builtin_branch_diff_usage[] = {
 N_("git branch-diff [] .. .."),
@@ -270,9 +272,57 @@ static int get_correspondences(struct string_list *a, 
struct string_list *b,
return res;
 }
 
-static const char *short_oid(struct patch_util *util)
+static void output_pair_header(struct strbuf *buf,
+  int i, struct patch_util *a_util,
+  int j, struct patch_util *b_util)
 {
-   return find_unique_abbrev(>oid, DEFAULT_ABBREV);
+   static char *dashes;
+   struct object_id *oid = a_util ? _util->oid : _util->oid;
+   struct commit *commit;
+
+   if (!dashes) {
+   char *p;
+
+   dashes = xstrdup(find_unique_abbrev(oid, DEFAULT_ABBREV));
+   for (p = dashes; *p; p++)
+   *p = '-';
+   }
+
+   strbuf_reset(buf);
+   if (i < 0)
+   strbuf_addf(buf, "-:  %s ", dashes);
+   else
+   strbuf_addf(buf, "%d:  %s ", i + 1,
+   find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
+
+   if (i < 0)
+   strbuf_addch(buf, '>');
+   else if (j < 0)
+   strbuf_addch(buf, '<');
+   else if (strcmp(a_util->patch, b_util->patch))
+   strbuf_addch(buf, '!');
+   else
+   strbuf_addch(buf, '=');
+
+   if (j < 0)
+   strbuf_addf(buf, " -:  %s", dashes);
+   else
+   strbuf_addf(buf, " %d:  %s", j + 1,
+   find_unique_abbrev(_util->oid, DEFAULT_ABBREV));
+
+   commit = lookup_commit_reference(oid);
+   if (commit) {
+   const char *commit_buffer = get_commit_buffer(commit, NULL);
+   const char *subject;
+
+   find_commit_subject(commit_buffer, );
+   strbuf_addch(buf, ' ');
+   format_subject(buf, subject, " ");
+   unuse_commit_buffer(commit, commit_buffer);
+   }
+   strbuf_addch(buf, '\n');
+
+   fwrite(buf->buf, buf->len, 1, stdout);
 }
 
 static struct strbuf *output_prefix_cb(struct diff_options *opt, void *data)
@@ -306,6 +356,7 @@ static void patch_diff(const char *a, const char *b,
 static void output(struct string_list *a, struct string_list *b,
   struct diff_options *diffopt)
 {
+   struct strbuf buf = STRBUF_INIT;
int i = 0, j = 0;
 
/*
@@ -327,25 +378,22 @@ static void output(struct string_list *a, struct 
string_list *b,
 
/* Show unmatched LHS commit whose predecessors were shown. */
if (i < a->nr && a_util->matching < 0) {
-   printf("%d: %s < -: \n",
-  i + 1, short_oid(a_util));
+   output_pair_header(, i, a_util, -1, NULL);
i++;
continue;
}
 
/* Show unmatched RHS commits. */
while (j < b->nr && b_util->matching < 0) {
-   printf("-:  > %d: %s\n",
-  j + 1, short_oid(b_util));
+   output_pair_header(, -1, NULL, j, b_util);
b_util = ++j < b->nr ? b->items[j].util : NULL;
}
 
/* Show matching LHS/RHS pair. */
if (j < b->nr) {
a_util = a->items[b_util->matching].util;
-   printf("%d: %s ! %d: %s\n",
-  b_util->matching + 1, short_oid(a_util),
-  j + 1, short_oid(b_util));
+   output_pair_header(,
+  b_util->matching, a_util, j, b_util);
if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
patch_diff(a->items[b_util->matching].string,
   b->items[j].string, diffopt);
@@ -353,6 +401,7 @@ static void output(struct string_list *a, struct 
string_list *b,
j++;
}
}
+   strbuf_release();
 }
 
 int cmd_branch_diff(int argc, const char **argv, const char *prefix)
-- 
2.17.0.409.g71698f11835

[PATCH v2 11/18] branch-diff: add tests

[Johannes Schindelin]

From: Thomas Rast 

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 empty line. And apparently xdiff picks a different
option here than Python's difflib.

Signed-off-by: Johannes Schindelin 
---
 t/.gitattributes   |   1 +
 t/t7910-branch-diff.sh | 144 ++
 t/t7910/history.export | 604 +
 3 files changed, 749 insertions(+)
 create mode 100755 t/t7910-branch-diff.sh
 create mode 100644 t/t7910/history.export

diff --git a/t/.gitattributes b/t/.gitattributes
index 3bd959ae523..af15d5aeedd 100644
--- a/t/.gitattributes
+++ b/t/.gitattributes
@@ -18,5 +18,6 @@ t[0-9][0-9][0-9][0-9]/* -whitespace
 /t5515/* eol=lf
 /t556x_common eol=lf
 /t7500/* eol=lf
+/t7910/* eol=lf
 /t8005/*.txt eol=lf
 /t9*/*.dump eol=lf
diff --git a/t/t7910-branch-diff.sh b/t/t7910-branch-diff.sh
new file mode 100755
index 000..a7fece88045
--- /dev/null
+++ b/t/t7910-branch-diff.sh
@@ -0,0 +1,144 @@
+#!/bin/sh
+
+test_description='branch-diff tests'
+
+. ./test-lib.sh
+
+# Note that because of git-branch-diff's heuristics, test_commit does more
+# harm than good.  We need some real history.
+
+test_expect_success 'setup' '
+   git fast-import < "$TEST_DIRECTORY"/t7910/history.export
+'
+
+test_expect_success 'simple A..B A..C (unmodified)' '
+   git branch-diff --no-color master..topic master..unmodified >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  35b9b25 s/5/A/
+   2:  fccce22 = 2:  de345ab s/4/A/
+   3:  147e64e = 3:  9af6654 s/11/B/
+   4:  a63e992 = 4:  2901f77 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'simple B...C (unmodified)' '
+   git branch-diff --no-color topic...unmodified >actual &&
+   # same "expected" as above
+   test_cmp expected actual
+'
+
+test_expect_success 'simple A B C (unmodified)' '
+   git branch-diff --no-color master topic unmodified >actual &&
+   # same "expected" as above
+   test_cmp expected actual
+'
+
+test_expect_success 'trivial reordering' '
+   git branch-diff --no-color master topic reordered >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  aca177a s/5/A/
+   3:  147e64e = 2:  14ad629 s/11/B/
+   4:  a63e992 = 3:  ee58208 s/12/B/
+   2:  fccce22 = 4:  307b27a s/4/A/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'removed a commit' '
+   git branch-diff --no-color master topic removed >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  7657159 s/5/A/
+   2:  fccce22 < -:  --- s/4/A/
+   3:  147e64e = 2:  43d84d3 s/11/B/
+   4:  a63e992 = 3:  a740396 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'added a commit' '
+   git branch-diff --no-color master topic added >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  2716022 s/5/A/
+   2:  fccce22 = 2:  b62accd s/4/A/
+   -:  --- > 3:  df46cfa s/6/A/
+   3:  147e64e = 4:  3e64548 s/11/B/
+   4:  a63e992 = 5:  12b4063 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'new base, A B C' '
+   git branch-diff --no-color master topic rebased >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  cc9c443 s/5/A/
+   2:  fccce22 = 2:  c5d9641 s/4/A/
+   3:  147e64e = 3:  28cc2b6 s/11/B/
+   4:  a63e992 = 4:  5628ab7 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'new base, B...C' '
+   # this syntax includes the commits from master!
+   git branch-diff --no-color topic...rebased >actual &&
+   cat >expected <<-EOF &&
+   -:  --- > 1:  a31b12e unrelated
+   1:  4de457d = 2:  cc9c443 s/5/A/
+   2:  fccce22 = 3:  c5d9641 s/4/A/
+   3:  147e64e = 4:  28cc2b6 s/11/B/
+   4:  a63e992 = 5:  5628ab7 s/12/B/
+   EOF
+   test_cmp expected actual
+'
+
+test_expect_success 'changed commit' '
+   git branch-diff --no-color topic...changed >actual &&
+   cat >expected <<-EOF &&
+   1:  4de457d = 1:  a4b s/5/A/
+   2:  fccce22 = 2:  f51d370 s/4/A/
+   3:  147e64e ! 3:  0559556 s/11/B/
+   @@ -10,7 +10,7 @@
+ 9
+ 10
+-11
+   -+B
+   ++BB
+ 12
+ 13
+ 14
+   4:  a63e992 ! 4:  d966c5c s/12/B/
+   @@ -8,7 +8,7 @@
+@@
+ 9
+   

[PATCH v2 17/18] branch-diff: add a man page

[Johannes Schindelin]

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 
---
 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 000..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=[]] [--no-color] []
+   [--dual-color] [--no-patches] [--creation-weight=]
+   (   | ... |)
+
+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=::
+   Set the creation/deletion cost fudge factor to ``.
+   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.
+
+ ::
+   Compare the commits specified by the two ranges, where
+   `` is considered an older version of ``.
+
+...::
+   Equivalent to passing `..` and `..`.
+
+  ::
+   Equivalent to passing `..` and `..`.
+   Note that `` 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=[]` 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 
+
+-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 

[PATCH v2 10/18] branch-diff: do not show "function names" in hunk headers

[Johannes Schindelin]

We are comparing complete, formatted commit messages with patches. There
are no function names here, so stop looking for them.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index 5b187890bdf..89d75c93115 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -11,6 +11,7 @@
 #include "diffcore.h"
 #include "commit.h"
 #include "pretty.h"
+#include "userdiff.h"
 
 static const char * const builtin_branch_diff_usage[] = {
 N_("git branch-diff [] .. .."),
@@ -330,6 +331,10 @@ static struct strbuf *output_prefix_cb(struct diff_options 
*opt, void *data)
return data;
 }
 
+static struct userdiff_driver no_func_name = {
+   .funcname = { "$^", 0 }
+};
+
 static struct diff_filespec *get_filespec(const char *name, const char *p)
 {
struct diff_filespec *spec = alloc_filespec(name);
@@ -339,6 +344,7 @@ static struct diff_filespec *get_filespec(const char *name, 
const char *p)
spec->size = strlen(p);
spec->should_munmap = 0;
spec->is_stdin = 1;
+   spec->driver = _func_name;
 
return spec;
 }
-- 
2.17.0.409.g71698f11835

[PATCH v2 01/18] Add a function to solve least-cost assignment problems

[Johannes Schindelin]

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 
---
 Makefile|   1 +
 hungarian.c | 205 
 hungarian.h |  19 +
 3 files changed, 225 insertions(+)
 create mode 100644 hungarian.c
 create mode 100644 hungarian.h

diff --git a/Makefile b/Makefile
index 50da82b0169..96f2e76a904 100644
--- a/Makefile
+++ b/Makefile
@@ -829,6 +829,7 @@ LIB_OBJS += gpg-interface.o
 LIB_OBJS += graph.o
 LIB_OBJS += grep.o
 LIB_OBJS += hashmap.o
+LIB_OBJS += hungarian.o
 LIB_OBJS += help.o
 LIB_OBJS += hex.o
 LIB_OBJS += ident.o
diff --git a/hungarian.c b/hungarian.c
new file mode 100644
index 000..346299a97d9
--- /dev/null
+++ b/hungarian.c
@@ -0,0 +1,205 @@
+/*
+ * Based on: Jonker, R., & Volgenant, A. (1987). A shortest augmenting path
+ * algorithm for dense and sparse linear assignment problems. Computing,
+ * 38(4), 325-340.
+ */
+#include "cache.h"
+#include "hungarian.h"
+#include 
+
+#define COST(column, row) cost[(column) + column_count * (row)]
+
+/*
+ * The parameter `cost` is the cost matrix: the cost to assign column j to row
+ * i is `cost[j + column_count * i].
+ */
+int compute_assignment(int column_count, int row_count, double *cost,
+  int *column2row, int *row2column)
+{
+   double *v = xmalloc(sizeof(double) * column_count), *d;
+   int *free_row, free_count = 0, saved_free_count, *pred, *col;
+   int i, j, phase;
+
+   memset(column2row, -1, sizeof(int) * column_count);
+   memset(row2column, -1, sizeof(int) * row_count);
+
+   /* column reduction */
+   for (j = column_count - 1; j >= 0; j--) {
+   int i1 = 0;
+
+   for (i = 1; i < row_count; i++)
+   if (COST(j, i1) > COST(j, i))
+   i1 = i;
+   v[j] = COST(j, i1);
+   if (row2column[i1] == -1) {
+   /* row i1 unassigned */
+   row2column[i1] = j;
+   column2row[j] = i1;
+   } else {
+   if (row2column[i1] >= 0)
+   row2column[i1] = -2 - row2column[i1];
+   column2row[j] = -1;
+   }
+   }
+
+   /* reduction transfer */
+   free_row = xmalloc(sizeof(int) * row_count);
+   for (int i = 0; i < row_count; i++) {
+   int j1 = row2column[i];
+   if (j1 == -1)
+   free_row[free_count++] = i;
+   else if (j1 < -1)
+   row2column[i] = -2 - j1;
+   else {
+   double min = COST(!j1, i) - v[!j1];
+   for (j = 1; j < column_count; j++)
+   if (j != j1 && min > COST(j, i) - v[j])
+   min = COST(j, i) - v[j];
+   v[j1] -= min;
+   }
+   }
+
+   if (free_count ==
+   (column_count < row_count ? row_count - column_count : 0)) {
+   free(v);
+   free(free_row);
+   return 0;
+   }
+
+   /* augmenting row reduction */
+   for (phase = 0; phase < 2; phase++) {
+   int k = 0;
+
+   saved_free_count = free_count;
+   free_count = 0;
+   while (k < saved_free_count) {
+   double u1, u2;
+   int j1 = 0, j2, i0;
+
+   i = free_row[k++];
+   u1 = COST(j1, i) - v[j1];
+   j2 = -1;
+   u2 = DBL_MAX;
+   for (j = 1; j < column_count; j++) {
+   double c = COST(j, i) - v[j];
+   if (u2 > c) {
+   if (u1 < c) {
+   u2 = c;
+   j2 = j;
+   } else {
+   u2 = u1;
+   u1 = c;
+   j2 = j1;
+   j1 = j;
+   }
+   }
+   }
+   if (j2 < 0) {
+   j2 = j1;
+   u2 = u1;
+   }
+
+   i0 = column2row[j1];
+   if (u1 < u2)
+   v[j1] -= u2 - u1;
+   else if (i0 >= 0) {
+   j1 = j2;
+   i0 = column2row[j1];
+   

[PATCH v2 05/18] branch-diff: also show the diff between patches

[Johannes Schindelin]

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`).

This brings branch-diff closer to feature parity with regard to 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.
This is left for a later commit.

Signed-off-by: Johannes Schindelin 
---
 builtin/branch-diff.c | 53 +++
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
index 92302b1c339..b23d66a3b1c 100644
--- a/builtin/branch-diff.c
+++ b/builtin/branch-diff.c
@@ -7,6 +7,8 @@
 #include "hashmap.h"
 #include "xdiff-interface.h"
 #include "hungarian.h"
+#include "diff.h"
+#include "diffcore.h"
 
 static const char * const builtin_branch_diff_usage[] = {
 N_("git branch-diff [] .. .."),
@@ -272,7 +274,31 @@ static const char *short_oid(struct patch_util *util)
return find_unique_abbrev(>oid, DEFAULT_ABBREV);
 }
 
-static void output(struct string_list *a, struct string_list *b)
+static struct diff_filespec *get_filespec(const char *name, const char *p)
+{
+   struct diff_filespec *spec = alloc_filespec(name);
+
+   fill_filespec(spec, _oid, 0, 0644);
+   spec->data = (char *)p;
+   spec->size = strlen(p);
+   spec->should_munmap = 0;
+   spec->is_stdin = 1;
+
+   return spec;
+}
+
+static void patch_diff(const char *a, const char *b,
+ struct diff_options *diffopt)
+{
+   diff_queue(_queued_diff,
+  get_filespec("a", a), get_filespec("b", b));
+
+   diffcore_std(diffopt);
+   diff_flush(diffopt);
+}
+
+static void output(struct string_list *a, struct string_list *b,
+  struct diff_options *diffopt)
 {
int i = 0, j = 0;
 
@@ -314,6 +340,9 @@ static void output(struct string_list *a, struct 
string_list *b)
printf("%d: %s ! %d: %s\n",
   b_util->matching + 1, short_oid(a_util),
   j + 1, short_oid(b_util));
+   if (!(diffopt->output_format & DIFF_FORMAT_NO_OUTPUT))
+   patch_diff(a->items[b_util->matching].string,
+  b->items[j].string, diffopt);
a_util->shown = 1;
j++;
}
@@ -322,21 +351,37 @@ static void output(struct string_list *a, struct 
string_list *b)
 
 int cmd_branch_diff(int argc, const char **argv, const char *prefix)
 {
+   struct diff_options diffopt = { NULL };
double creation_weight = 0.6;
struct option options[] = {
+   OPT_SET_INT(0, "no-patches", _format,
+   N_("short format (no diffs)"),
+   DIFF_FORMAT_NO_OUTPUT),
{ OPTION_CALLBACK,
0, "creation-weight", _weight, N_("factor"),
N_("Fudge factor by which creation is weighted [0.6]"),
0, parse_creation_weight },
OPT_END()
};
-   int res = 0;
+   int i, j, res = 0;
struct strbuf range1 = STRBUF_INIT, range2 = STRBUF_INIT;
struct string_list branch1 = STRING_LIST_INIT_DUP;
struct string_list branch2 = STRING_LIST_INIT_DUP;
 
+   diff_setup();
+   diffopt.output_format = DIFF_FORMAT_PATCH;
+
argc = parse_options(argc, argv, NULL, options,
-   builtin_branch_diff_usage, 0);
+   builtin_branch_diff_usage, PARSE_OPT_KEEP_UNKNOWN);
+
+   for (i = j = 0; i < argc; i++) {
+   int c = diff_opt_parse(, argv + i, argc - i, prefix);
+
+   if (!c)
+   argv[j++] = argv[i];
+   }
+   argc = j;
+   diff_setup_done();
 
if (argc == 2) {
if (!strstr(argv[0], ".."))
@@ -380,7 +425,7 @@ int cmd_branch_diff(int argc, const char **argv, const char 
*prefix)
find_exact_matches(, );
res = get_correspondences(, , creation_weight);
if (!res)
-   output(, );
+   output(, , );
}
 
strbuf_release();
-- 
2.17.0.409.g71698f11835

[PATCH v2 02/18] Add a new builtin: branch-diff

[Johannes Schindelin]

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`.

At this point, we ignore tbdiff's color options as well as the
--no-patches option, as they will all be implemented later using
diff_options.

Signed-off-by: Johannes Schindelin 
---
 .gitignore|  1 +
 Makefile  |  1 +
 builtin.h |  1 +
 builtin/branch-diff.c | 38 ++
 command-list.txt  |  1 +
 git.c |  1 +
 6 files changed, 43 insertions(+)
 create mode 100644 builtin/branch-diff.c

diff --git a/.gitignore b/.gitignore
index 833ef3b0b78..1346a64492f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -20,6 +20,7 @@
 /git-bisect--helper
 /git-blame
 /git-branch
+/git-branch-diff
 /git-bundle
 /git-cat-file
 /git-check-attr
diff --git a/Makefile b/Makefile
index 96f2e76a904..9b1984776d8 100644
--- a/Makefile
+++ b/Makefile
@@ -953,6 +953,7 @@ BUILTIN_OBJS += builtin/archive.o
 BUILTIN_OBJS += builtin/bisect--helper.o
 BUILTIN_OBJS += builtin/blame.o
 BUILTIN_OBJS += builtin/branch.o
+BUILTIN_OBJS += builtin/branch-diff.o
 BUILTIN_OBJS += builtin/bundle.o
 BUILTIN_OBJS += builtin/cat-file.o
 BUILTIN_OBJS += builtin/check-attr.o
diff --git a/builtin.h b/builtin.h
index 42378f3aa47..e1c4d2a529a 100644
--- a/builtin.h
+++ b/builtin.h
@@ -135,6 +135,7 @@ extern int cmd_archive(int argc, const char **argv, const 
char *prefix);
 extern int cmd_bisect__helper(int argc, const char **argv, const char *prefix);
 extern int cmd_blame(int argc, const char **argv, const char *prefix);
 extern int cmd_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_branch_diff(int argc, const char **argv, const char *prefix);
 extern int cmd_bundle(int argc, const char **argv, const char *prefix);
 extern int cmd_cat_file(int argc, const char **argv, const char *prefix);
 extern int cmd_checkout(int argc, const char **argv, const char *prefix);
diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
new file mode 100644
index 000..60a4b4fbe30
--- /dev/null
+++ b/builtin/branch-diff.c
@@ -0,0 +1,38 @@
+#include "cache.h"
+#include "builtin.h"
+#include "parse-options.h"
+
+static const char * const builtin_branch_diff_usage[] = {
+N_("git branch-diff [] .. .."),
+N_("git branch-diff [] ..."),
+N_("git branch-diff []   "),
+NULL
+};
+
+static int parse_creation_weight(const struct option *opt, const char *arg,
+int unset)
+{
+   double *d = opt->value;
+   if (unset)
+   *d = 0.6;
+   else
+   *d = atof(arg);
+   return 0;
+}
+
+int cmd_branch_diff(int argc, const char **argv, const char *prefix)
+{
+   double creation_weight = 0.6;
+   struct option options[] = {
+   { OPTION_CALLBACK,
+   0, "creation-weight", _weight, N_("factor"),
+   N_("Fudge factor by which creation is weighted [0.6]"),
+   0, parse_creation_weight },
+   OPT_END()
+   };
+
+   argc = parse_options(argc, argv, NULL, options,
+   builtin_branch_diff_usage, 0);
+
+   return 0;
+}
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd82..d01b9063e81 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -19,6 +19,7 @@ git-archive mainporcelain
 git-bisect  mainporcelain   info
 git-blame   ancillaryinterrogators
 git-branch  mainporcelain   history
+git-branch-diff mainporcelain
 git-bundle  mainporcelain
 git-cat-fileplumbinginterrogators
 git-check-attr  purehelpers
diff --git a/git.c b/git.c
index f598fae7b7a..d2794fb6f5d 100644
--- a/git.c
+++ b/git.c
@@ -377,6 +377,7 @@ static struct cmd_struct commands[] = {
{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
{ "blame", cmd_blame, RUN_SETUP },
{ "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
+   { "branch-diff", cmd_branch_diff, RUN_SETUP | USE_PAGER },
{ "bundle", cmd_bundle, RUN_SETUP_GENTLY | NO_PARSEOPT },
{ "cat-file", cmd_cat_file, RUN_SETUP },
{ "check-attr", cmd_check_attr, RUN_SETUP },
-- 
2.17.0.409.g71698f11835

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

[Johannes Schindelin]

The incredibly useful `git-tbdiff` tool to compare patch series (say, to see
what changed between two iterations sent to the Git mailing list) is slightly
less useful for this developer due to the fact that it requires the `hungarian`
and `numpy` Python packages which are for some reason really hard to build in
MSYS2. So hard that I even had to give up, because it was simply easier to
reimplement the whole shebang as a builtin command.

The project at https://github.com/trast/tbdiff seems to be dormant, anyway.
Funny (and true) story: I looked at the open Pull Requests to see how active
that project is, only to find to my surprise that I had submitted one in August
2015, and that it was still unanswered let alone merged.

While at it, I forward-ported AEvar's patch to force `--decorate=no` because
`git -p tbdiff` would fail otherwise.

Side note: I work on implementing branch-diff not only to make life easier for
reviewers who have to suffer through v2, v3, ... of my patch series, but also
to verify my changes before submitting a new iteraion. And also, maybe even
more importantly, I plan to use it to verify my merging-rebases of Git for
Windows (for which I previously used to redirect the pre-rebase/post-rebase
diffs vs upstream and then compare them using `git diff --no-index`). And of
course any interested person can see what changes were necessary e.g. in the
merging-rebase of Git for Windows onto v2.17.0 by running a command like:

base=^{/Start.the.merging-rebase}
tag=v2.17.0.windows.1
pre=$tag$base^2
git branch-diff --dual-color $pre$base..$pre $tag$base..$tag

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

Changes since v1:

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

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

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

- Fixed typo "emtpy".

- Fixed `make sparse` warnings.

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

- Fixed tyop in commit message: "Pythong".

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

- Released the `line` strbuf after use.

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

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

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

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

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

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

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

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


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

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

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

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Duy Nguyen]

On Fri, May 4, 2018 at 5:23 PM, Johannes Schindelin
 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

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Johannes Schindelin]

Hi Duy,

On Fri, 4 May 2018, Duy Nguyen wrote:

> On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
>  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

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Duy Nguyen]

On Fri, May 04, 2018 at 04:44:49PM +0200, Duy Nguyen wrote:
> On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
>  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

Re: [PATCH v3] wt-status: use settings from git_diff_ui_config

[Elijah Newren]

On Fri, May 4, 2018 at 4:12 AM, Eckhard S. Maaß
 wrote:
> If you do something like
>
> - git add .
> - git status
> - git commit
> - git show (or git diff HEAD)
>
> one would expect to have analogous output from git status and git show
> (or similar diff-related programs). This is generally not the case, as
> git status has hard coded values for diff related options.
>

> ---
>
> Hello,
>
> Hopefully I have addressed all issues that have come up so far.

Looks good to me, thanks.

Elijah

Re: [PATCH v2] checkout & worktree: introduce checkout.implicitRemote

[Duy Nguyen]

On Fri, May 4, 2018 at 9:54 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Realistically the way we do hooks now would make the UI of this suck,
> i.e. you couldn't configure it globally or system-wide easily. Something
> like what I noted in
> https://public-inbox.org/git/871sf3el01@evledraar.gmail.com/ would
> make it suck less, but that's a much bigger task.

I thought you would bring this up :) I've given some more thoughts on
this topic and am willing to implement something like below, in a week
or two. Would that help change your mind?

I proposed hooks.* config space in that same thread. Here is the
extension to make it cover both of your points.

hooks.* can have multiple values. So you can specify
hooks.post-checkout multiple times and all those scripts will run (in
the same order you specified). Since we already have a search path for
config (/etc/gitconfig -> $HOME/.gitconfig -> $REPO/config) this helps
hooks management as well. Execution order is still the same: if you
specify hooks.post-checkout in both /etc/gitconfig and .git/config,
then the one in /etc/gitconfig is executed first, the one in .git
second.

And here's something extra to make it possible to override the search
order: if you specify hooks.post-checkout = reset (reset is a random
keyword) then _all_ post-checkout hooks before this point are ignored.
So you can put this in .git/config

[hooks]
post-checkout = reset
post-checkout = ~/some-hook

and can be sure that post-checkout specified in $HOME and /etc will
not affect you, only ~/some-hook will run. If you drop the second line
then you have no post-checkout hooks. This is a workaround for a
bigger problem (not being able to unset a config entry) but I think
it's sufficient for this use case.
-- 
Duy

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Duy Nguyen]

On Fri, May 4, 2018 at 9:23 AM, Johannes Schindelin
 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

Re: git merge banch w/ different submodule revision

[Elijah Newren]

On Fri, May 4, 2018 at 3:18 AM, Heiko Voigt  wrote:
> Hi,
>
> On Fri, May 04, 2018 at 08:29:32AM +, Middelschulte, Leif wrote:
>> Am Donnerstag, den 03.05.2018, 18:42 +0200 schrieb Heiko Voigt:

>> > It seems to me that you do not want to mix integration testing and
>> > testing of the feature itself.
>> That's on point. That's why it would be nice if git *at least* warned
>> about the different revisions wrt submodules.

There's a good point here...

> Well a submodule version is pinned down as long a you do not change it
> and commit it. The same as files and the goal is to make submodules
> behave as close to normal files as possible. And git "warns" about
> changed submodules by displaying them in the diff.

Actually, submodules do behave differently than normal files in an
important way, which we may be able to fix and may help Leif here:

When merging two regular files that have been modified on both sides
of history, git always prints a message, "Auto-merging $FILE".  We
could omit that and depend on the user to check the diffstat or run
diff afterwards or something, but we don't just rely on that; we also
warn them with a simple message that we are doing something to resolve
this both-sides-changed-this-path (namely employing the well known
three-way-file-merge algorithm to come up with something).

Inside merge_submodule(), the equivalent would be printing a message
whenever we decide that one branch is a fast-forward of the other
("Case #1", as it's called in the code), yet currently it prints
nothing.  Perhaps it should.


Leif, would you like to try your hand at creating a patch for this?

Re: cover letter cc's [was: [PATCH 60/67] hw/s390x: add include directory headers]

[Peter Maydell]

On 4 May 2018 at 14:26, Cornelia Huck  wrote:
> On Fri, 4 May 2018 08:07:53 -0500
> Eric Blake  wrote:
>> On the other hand, cc'ing all recipients for a largely mechanical patch
>> series that was split into 67 parts, in part because it touches so many
>> different maintainers' areas, may make the cover letter have so many
>> recipients that various mail gateways start rejecting it as potential spam.
>
> Yes, large cross-subsystem patch series make this painful.

My solution is to either (a) avoid them or (b) not bother cc'ing
people (except people I think might be interested in reviewing
cross-subsystem patchsets like Eric ;-))...

thanks
-- PMM

Re: cover letter cc's [was: [PATCH 60/67] hw/s390x: add include directory headers]

[Cornelia Huck]

On Fri, 4 May 2018 08:07:53 -0500
Eric Blake  wrote:

> [adding a cross-post to the git mailing list]
> 
> On 05/04/2018 02:10 AM, Cornelia Huck wrote:
> > On Thu, 3 May 2018 22:51:40 +0300
> > "Michael S. Tsirkin"  wrote:
> >   
> >> This way they are easier to find using standard rules.
> >>
> >> Signed-off-by: Michael S. Tsirkin 
> >> ---  
> ...
> 
> > [Goes to find cover letter to figure out what this is all about.
> > *Please*, cc: people on the cover letter so they can see immediately
> > what this is trying to do!]  
> 
> Is there an EASY way to make 'git format-patch --cover-letter $commitid' 
> (and git send-email, by extension) automatically search for all cc's any 
> any of the N/M patches, and auto-cc ALL of those recipients on the 0/N 
> cover letter?  And if that is not something easily built into git 
> format-patch directly, is it something that can easily be added to 
> sendemail.cccmd?  This is not the first time that someone has complained 
> that automatic cc's are not sending the cover letter context to a 
> particular maintainer interested (and auto-cc'd) in only a subset of an 
> overall series.

I think for most cases where I've been cc:ed on the cover letter and
only some of the patches, people actually added cc: lines to the cover
letter manually.

> 
> On the other hand, cc'ing all recipients for a largely mechanical patch 
> series that was split into 67 parts, in part because it touches so many 
> different maintainers' areas, may make the cover letter have so many 
> recipients that various mail gateways start rejecting it as potential spam.

Yes, large cross-subsystem patch series make this painful.

If I get some patches like "subsystem: frobnicate foo" and it's clear
that it's simply frobnicating foo for various subsystems, I can see
what this is about without reading the cover letter, no need to cc: me.
In this case, however, the patch did not make any sense at all without
looking at the explanation in the cover letter.

So I think we don't want to do this automatically, although some way to
collect potential candidates for cc:ing on the cover letter might be
helpful.

[PATCHv2] git-send-email: allow re-editing of message

[Drew DeVault]

When shown the email summary, an opportunity is presented for the user
to edit the email as if they had specified --annotate. This also permits
them to edit it multiple times.

Signed-off-by: Drew DeVault 
Reviewed-by: Simon Ser 

---
Thanks for the review Eric, updated to address your feedback.

 git-send-email.perl | 38 +++---
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/git-send-email.perl b/git-send-email.perl
index 2fa7818ca..b45953733 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -1330,9 +1330,14 @@ sub file_name_is_absolute {
return File::Spec::Functions::file_name_is_absolute($path);
 }
 
-# Returns 1 if the message was sent, and 0 otherwise.
-# In actuality, the whole program dies when there
-# is an error sending a message.
+# Prepares the email, then asks the user what to do.
+#
+# If the user chooses to send the email, it's sent and 1 is returned.
+# If the user chooses not to send the email, 0 is returned.
+# If the user decides they want to make further edits, -1 is returned and the
+# caller is expected to call send_message again after the edits are performed.
+#
+# If an error occurs sending the email, this just dies.
 
 sub send_message {
my @recipients = unique_email_list(@to);
@@ -1404,15 +1409,17 @@ Message-Id: $message_id
 
 EOF
}
-   # TRANSLATORS: Make sure to include [y] [n] [q] [a] in your
+   # TRANSLATORS: Make sure to include [y] [n] [e] [q] [a] in your
# translation. The program will only accept English input
# at this point.
-   $_ = ask(__("Send this email? ([y]es|[n]o|[q]uit|[a]ll): "),
-valid_re => qr/^(?:yes|y|no|n|quit|q|all|a)/i,
+   $_ = ask(__("Send this email? ([y]es|[n]o|[e]dit|[q]uit|[a]ll): 
"),
+valid_re => qr/^(?:yes|y|no|n|edit|e|quit|q|all|a)/i,
 default => $ask_default);
die __("Send this email reply required") unless defined $_;
if (/^n/i) {
return 0;
+   } elsif (/^e/i) {
+   return -1;
} elsif (/^q/i) {
cleanup_compose_files();
exit(0);
@@ -1552,7 +1559,12 @@ $references = $initial_in_reply_to || '';
 $subject = $initial_subject;
 $message_num = 0;
 
-foreach my $t (@files) {
+# Prepares the email, prompts the user, sends it out
+# Returns 0 if an edit was done and the function should be called again, or 1
+# otherwise.
+sub process_file {
+   my ($t) = @_;
+
open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
 
my $author = undef;
@@ -1755,6 +1767,10 @@ foreach my $t (@files) {
}
 
my $message_was_sent = send_message();
+   if ($message_was_sent == -1) {
+   do_edit($t);
+   return 0;
+   }
 
# set up for the next message
if ($thread && $message_was_sent &&
@@ -1776,6 +1792,14 @@ foreach my $t (@files) {
undef $auth;
sleep($relogin_delay) if defined $relogin_delay;
}
+
+   return 1;
+}
+
+foreach my $t (@files) {
+   while (!process_file($t)) {
+   # user edited the file
+   }
 }
 
 # Execute a command (e.g. $to_cmd) to get a list of email addresses
-- 
2.17.0

cover letter cc's [was: [PATCH 60/67] hw/s390x: add include directory headers]

[Eric Blake]

[adding a cross-post to the git mailing list]

On 05/04/2018 02:10 AM, Cornelia Huck wrote:

On Thu, 3 May 2018 22:51:40 +0300
"Michael S. Tsirkin"  wrote:


This way they are easier to find using standard rules.

Signed-off-by: Michael S. Tsirkin 
---

...


[Goes to find cover letter to figure out what this is all about.
*Please*, cc: people on the cover letter so they can see immediately
what this is trying to do!]


Is there an EASY way to make 'git format-patch --cover-letter $commitid' 
(and git send-email, by extension) automatically search for all cc's any 
any of the N/M patches, and auto-cc ALL of those recipients on the 0/N 
cover letter?  And if that is not something easily built into git 
format-patch directly, is it something that can easily be added to 
sendemail.cccmd?  This is not the first time that someone has complained 
that automatic cc's are not sending the cover letter context to a 
particular maintainer interested (and auto-cc'd) in only a subset of an 
overall series.


On the other hand, cc'ing all recipients for a largely mechanical patch 
series that was split into 67 parts, in part because it touches so many 
different maintainers' areas, may make the cover letter have so many 
recipients that various mail gateways start rejecting it as potential spam.


--
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[PATCH v2] perf/bisect_run_script: disable codespeed

[Christian Couder]

When bisecting a performance regression using a config file,
`./bisect_regression --config my_perf.conf` for example, the
config file can contain Codespeed configuration which would
instruct the 'aggregate.perl' script called by the 'run'
script to output results in the Codespeed format and maybe
to try to send this output to a Codespeed server.

This is unfortunate because the 'bisect_run_script' relies
on the regular output from 'aggregate.perl' to mesure
performance, so let's disable Codespeed output and sending
results to a Codespeed server.

Signed-off-by: Christian Couder 
---
Previous version was:

https://public-inbox.org/git/20180427135135.22931-1-chrisc...@tuxfamily.org/

The only change compared to this previous version is a typo
fix in the commit message:

s/This in unfortunate/This is unfortunate/

 t/perf/bisect_run_script | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/t/perf/bisect_run_script b/t/perf/bisect_run_script
index 038255df4b..3ebaf15521 100755
--- a/t/perf/bisect_run_script
+++ b/t/perf/bisect_run_script
@@ -24,6 +24,12 @@ 
result_file="$info_dir/perf_${script_number}_${bisect_head}_results.txt"
 GIT_PERF_DIRS_OR_REVS="$bisect_head"
 export GIT_PERF_DIRS_OR_REVS
 
+# Don't use codespeed
+GIT_PERF_CODESPEED_OUTPUT=
+GIT_PERF_SEND_TO_CODESPEED=
+export GIT_PERF_CODESPEED_OUTPUT
+export GIT_PERF_SEND_TO_CODESPEED
+
 ./run "$script" >"$result_file" 2>&1 || die "Failed to run perf test '$script'"
 
 rtime=$(sed -n 
"s/^$script_number\.$test_number:.*\([0-9]\+\.[0-9]\+\)(.*).*\$/\1/p" 
"$result_file")
-- 
2.17.0.400.gd2f70daabc

[PATCH v3] wt-status: use settings from git_diff_ui_config

[Eckhard S. Maaß]

If you do something like

- git add .
- git status
- git commit
- git show (or git diff HEAD)

one would expect to have analogous output from git status and git show
(or similar diff-related programs). This is generally not the case, as
git status has hard coded values for diff related options.

With this commit the hard coded settings are dropped from the status
command in favour for values provided by git_diff_ui_config.

What follows are some remarks on the concrete options which were hard
coded in git status:

diffopt.detect_rename

Since the very beginning of git status in a3e870f2e2 ("Add "commit"
helper script", 2005-05-30), git status always used rename detection,
whereas with commands like show and log one had to activate it with a
command line option. After 5404c116aa ("diff: activate diff.renames by
default", 2016-02-25) the default behaves the same by coincidence, but
changing diff.renames to other values can break the consistency between
git status and other commands again. With this commit one control the
same default behaviour with diff.renames.

diffopt.rename_limit

Similarly one has the option diff.renamelimit to adjust this limit for
all commands but git status. With this commit git status will also honor
those.

diffopt.break_opt

Unlike the other two options this cannot be configured by a
configuration option yet. This commit will also change the default
behaviour to not use break rewrites. But as rename detection is most
likely on, this is dangerous to be activated anyway as one can see
here:

https://public-inbox.org/git/xmqqegqaahnh@gitster.dls.corp.google.com/

Signed-off-by: Eckhard S. Maaß 
Reviewed-by: Elijah Newren 
---

Hello,

Hopefully I have addressed all issues that have come up so far.

Greetings,
Eckhard

 builtin/commit.c   |  2 +-
 t/t4001-diff-rename.sh | 12 
 wt-status.c|  4 
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 5571d4a3e2..5240f11225 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -161,9 +161,9 @@ static void determine_whence(struct wt_status *s)
 static void status_init_config(struct wt_status *s, config_fn_t fn)
 {
wt_status_prepare(s);
+   init_diff_ui_defaults();
git_config(fn, s);
determine_whence(s);
-   init_diff_ui_defaults();
s->hints = advice_status_hints; /* must come after git_config() */
 }
 
diff --git a/t/t4001-diff-rename.sh b/t/t4001-diff-rename.sh
index a07816d560..bf4030371a 100755
--- a/t/t4001-diff-rename.sh
+++ b/t/t4001-diff-rename.sh
@@ -138,6 +138,18 @@ test_expect_success 'favour same basenames over different 
ones' '
test_i18ngrep "renamed: .*path1 -> subdir/path1" out
 '
 
+test_expect_success 'test diff.renames=true for git status' '
+   git -c diff.renames=true status >out &&
+   test_i18ngrep "renamed: .*path1 -> subdir/path1" out
+'
+
+test_expect_success 'test diff.renames=false for git status' '
+   git -c diff.renames=false status >out &&
+   test_i18ngrep ! "renamed: .*path1 -> subdir/path1" out &&
+   test_i18ngrep "new file: .*subdir/path1" out &&
+   test_i18ngrep "deleted: .*[^/]path1" out
+'
+
 test_expect_success 'favour same basenames even with minor differences' '
git show HEAD:path1 | sed "s/15/16/" > subdir/path1 &&
git status >out &&
diff --git a/wt-status.c b/wt-status.c
index 50815e5faf..32f3bcaebd 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -625,9 +625,6 @@ static void wt_status_collect_changes_index(struct 
wt_status *s)
rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
rev.diffopt.format_callback = wt_status_collect_updated_cb;
rev.diffopt.format_callback_data = s;
-   rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
-   rev.diffopt.rename_limit = 200;
-   rev.diffopt.break_opt = 0;
copy_pathspec(_data, >pathspec);
run_diff_index(, 1);
 }
@@ -985,7 +982,6 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
setup_revisions(0, NULL, , );
 
rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
-   rev.diffopt.detect_rename = DIFF_DETECT_RENAME;
rev.diffopt.file = s->fp;
rev.diffopt.close_file = 0;
/*
-- 
2.17.0.252.gfe0a9eaf31

Re: git merge banch w/ different submodule revision

[Heiko Voigt]

Hi,

On Fri, May 04, 2018 at 08:29:32AM +, Middelschulte, Leif wrote:
> Am Donnerstag, den 03.05.2018, 18:42 +0200 schrieb Heiko Voigt:
> > I still do not understand how the current behaviour is mismatching with
> > users expectations. Let's assume that you directly tracked the files of
> > L in your product repository P, without any submodule boundary. How
> > would the behavior be different? Would it be? If D started on an older
> > revision and gets merged into a newer revision, there can always be
> > regressions even without submodules.
> > 
> > Why would the core developer need to be informed about mismatching
> > revisions if he himself advanced the submodule?
> In that case you'd be right. I should have picked my example more wisely.
> Assume right here that not a core developer, but another developer advanced
> the submodule (also via feature branch + merge).
> > 
> > It seems to me that you do not want to mix integration testing and
> > testing of the feature itself. 
> That's on point. That's why it would be nice if git *at least* warned
> about the different revisions wrt submodules.
> 
> But, I guess, I learned something about submodules:
> I used to think of submodules as means to pin down a specific revision like: 
> `ver == x`.
> Now I'm learning that submodules are treated as `ver >= x` during a merge.

Well a submodule version is pinned down as long a you do not change it
and commit it. The same as files and the goal is to make submodules
behave as close to normal files as possible. And git "warns" about
changed submodules by displaying them in the diff.

Actually the use case you are describing is not even involving a real
merge for submodules. It is just changing the pointer to another
revision.

> > How about just testing/reviewing on the
> > branch then? You would still get the submodule revision D was working on
> > and then in a later stage check if integration with everything else
> > works.
> Sure. But if the behavior deviates after a merge the merging developer is 
> currently not
> aware that it *might* have to do with different submodule revisions used, not 
> the "actual" code merged.
> 
> Like not even "beware: the (feature) branch you've merged used an 'older' 
> revision of X"

The submodule is part of the "actual" code and should be reviewed the
same. Maybe you want to set the diff.submodule option to 'diff' ? Then
git shows the actual diff of the changed contents in the submodule and
it would be more obvious how the code changed.

At the moment it seems to me that you want submodules to behave
differently than we handle normal files/directories which is the
opposite direction we have been trying to get git into. My feeling
though is that this should be covered by the review process instead of a
failing merge. Another option would be that you could write a hook that
warns reviewers that they are merging a submodule update.

Cheers Heiko

Re: [PATCH v2] checkout & worktree: introduce checkout.implicitRemote

[Eric Sunshine]

On Thu, May 3, 2018 at 9:18 AM, Ævar Arnfjörð Bjarmason
 wrote:
> Introduce a checkout.implicitRemote setting which can be used to
> designate a remote to prefer (via checkout.implicitRemote=origin) when
> running e.g. "git checkout master" to mean origin/master, even though
> there's other remotes that have the "master" branch.
> [...]
> The new checkout.implicitRemote config allows me to say that whenever
> that ambiguity comes up I'd like to prefer "origin", and it'll still
> work as though the only remote I had was "origin".
> [...]
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> @@ -1084,6 +1084,23 @@ browser..path::
> +checkout.implicitRemote::
> +   When you run 'git checkout ' and only have one
> +   remote, it may implicitly fall back on checking out and
> +   tracking e.g. 'origin/'. This stops working as soon
> +   as you have more than one remote with a ''
> +   reference. This setting allows for setting the name of a
> +   special remote that should always win when it comes to

"special" is overly broad. "preferred" may better convey the intended
meaning. Simply dropping "special" also works.

Subjective; not worth a re-roll.

> +   disambiguation. The typical use-case is to set this to
> +   `origin`.
> ++
> +Currently this is used by linkgit:git-checkout[1] when 'git checkout
> +' will checkout the '' branch on another remote,
> +and by linkgit:git-worktree[1] when 'git worktree add' when referring

"when ... when"?

> +to a remote branch.  This setting might be used for other
> +checkout-like commands or functionality in the future when
> +appropriate.

Not sure the final sentence adds value as user-facing documentation
(versus the commit message in which it may).

> diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
> @@ -60,6 +60,11 @@ with a matching name, treat as equivalent to:
>  $ git worktree add --track -b   /
> +It's also possible to use the `checkout.implicitRemote` setting to
> +designate a special remote this rule should be applied to, even if the

Again, you could drop "special".

> +branch isn't unique across all remotes. See `checkout.implicitRemote`
> +in linkgit:git-config[1].

I have a hard time digesting this paragraph. Perhaps it wants to say:

Option `checkout.implicitRemote` can be configured to designate a
 to use when  isn't unique across all remotes.
See ...

> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> @@ -450,6 +450,24 @@ test_expect_success 'git worktree --no-guess-remote 
> option overrides config' '
> +test_expect_success '"add"   dwims with 
> checkout.implicitRemote' '
> +   test_when_finished rm -rf repo_upstream repo_dwim foo &&
> +   setup_remote_repo repo_upstream repo_dwim &&
> +   git init repo_dwim &&

Maybe replace "dwim" here and in the test title with something else
since checkout.implicitRemote is no longer about DWIM'ing?

> +   (
> +   cd repo_dwim &&
> +   git remote add repo_upstream2 ../repo_upstream &&
> +   git fetch repo_upstream2 &&
> +   test_must_fail git worktree add ../foo foo &&
> +   git -c checkout.implicitRemote=repo_upstream worktree add 
> ../foo foo
> +   ) &&
> +   (
> +   cd foo &&
> +   test_branch_upstream foo repo_upstream foo &&
> +   test_cmp_rev refs/remotes/repo_upstream/foo refs/heads/foo
> +   )
> +'

Re: [PATCH v3 09/12] get_short_oid / peel_onion: ^{tree} should be tree, not treeish

[Ævar Arnfjörð Bjarmason]

On Fri, May 04 2018, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  writes:
>
>> The reason I'm doing this is because I found it confusing that I can't
>> do:
>>
>> for t in tag commit tree blob; do ./git --exec-path=$PWD rev-parse 
>> 7452^{$t}; done
>>
>> And get, respectively, only the SHAs that match the respective type, but
>> currently (with released git) you can do:
>>
>> for t in tag commit committish treeish tree blob; do git -c 
>> core.disambiguate=$t rev-parse 7452; done
>
> Exactly.  The former asks "I (think I) know 7452 can be used to name
> an object of type $t, with peeling if necessary--give me the underlying
> object of type $t".

Right, and I'm with you so far, this makes sense to me for all existing
uses of the peel syntax, otherwise v2.17.0^{tree} wouldn't be the same
as rev-parse v2.17.0^{tree}^{tree}...

> In short, the fact that you can write "$X^{$t}"
> says that $X is a $t-ish (otherwise $X cannot be used as a stand-in
> for an object of type $t) and that you fully expect that $X can merely
> be of type $t-ish and not exactly $t (otherwise you wouldn't be
> making sure to coerce $X into $t with ^{$t} notation).
>
> In *THAT* context, disambiguation help that lists objects whose name
> begins with "7452" you gave, hoping that it is a unique enough
> prefix when it wasn't in reality, *MUST* give $t-ish; restricting it
> to $t makes the help mostly useless.
>
>> 1) Am I missing some subtlety or am I correct that there was no way to
>> get git to return more than one SHA-1 for ^{commit} or ^{tree} before
>> this disambiguation feature was added?
>
> There is no such feature either before or after the disambiguation
> help.  I am not saying there shouldn't exist such a feature.  I am
> saying that breaking the existing feature and making it useless is
> not the way to add such a feature.

I still don't get how what you're proposing is going to be consistent,
but let's fully enumerate the output of 7452 with my patch to take that
case-by-case[1]:

^{tag}:
7452b4b5786778d5d87f5c90a94fab8936502e20
^{commit}:
hint:   74521eee4c commit 2007-12-01 - git-gui: install-sh from automake 
does not like -m755
hint:   745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for 
check_refname_component
^{tree}:
hint:   7452336aa3 tree
hint:   74524f384d tree
hint:   7452813bcd tree
hint:   7452b1a701 tree
hint:   7452b73c42 tree
hint:   7452ca1557 tree
^{blob}:
hint:   7452001351 blob
hint:   745254665d blob
hint:   7452a572c1 blob
hint:   7452b9fd21 blob
hint:   7452db13c8 blob
hint:   7452fce0da blob

And[2]:

core.disambiguate=tag:
[same as ^{tag]
core.disambiguate=commit:
[same as ^{commit}]
core.disambiguate=committish:
hint:   7452b4b578 tag v2.1.0
hint:   74521eee4c commit 2007-12-01 - git-gui: install-sh from automake 
does not like -m755
hint:   745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for 
check_refname_component
core.disambiguate=tree:
[same as ^{tree}]
core.disambiguate=treeish (same as $sha1:)
hint:   7452b4b578 tag v2.1.0
hint:   74521eee4c commit 2007-12-01 - git-gui: install-sh from automake 
does not like -m755
hint:   745224e04a commit 2014-06-18 - refs.c: SSE2 optimizations for 
check_refname_component
hint:   7452336aa3 tree
hint:   74524f384d tree
hint:   7452813bcd tree
hint:   7452b1a701 tree
hint:   7452b73c42 tree
hint:   7452ca1557 tree
core.disambiguate=blob:
[same as ^{blob}]

So from my understanding of what you're saying you'd like to list tag,
commits and trees given $sha1^{tree}, because they're all types that can
be used to reach a tree.

I don't think that's very useful, yes it would "break" existing
disambiguations, but this is such an obscure (and purely manual)
use-case than I think that's fine.

Because I think to the extent anyone's going to use this it's because
they know they have e.g. a short blob, commit etc. SHA-1 they're not
going to use it because they have some short $SHA they know is a tree,
and then want all SHA-1s of that *and* random tag & commit objects that
happen to have the same object prefix just because tags and commits can
also point to trees.

How does that make any sense? The entire reason for using the normal
peel syntax is because you e.g. have v2.17.0 and want to get to the
^{tree} or the ^{commit} tht v2.17.0 directly points to. That's entirely
orthogonal to what the disambiguation is doing. There with your proposed
semantics you're peeling 7452 as 7452^{tree} because (IMO) you're
looking for trees, just to get some entirely unrelated commits and tags.

But *leaving that aside*, i.e. I don't see why the use-case would make
sense. What I *don't* get is why, if you think that, you only want to
apply that rule to ^{tree}. I.e. wouldn't it then be consistent to say:

# a)
^{tag}= tag
^{commit} = tag, commit

Re: git merge banch w/ different submodule revision

[Middelschulte, Leif]

Hi,
Am Donnerstag, den 03.05.2018, 18:42 +0200 schrieb Heiko Voigt:
> Hi,
> 
> On Wed, May 02, 2018 at 07:30:25AM +, Middelschulte, Leif wrote:
> > Am Montag, den 30.04.2018, 19:02 +0200 schrieb Heiko Voigt:
> > > On Thu, Apr 26, 2018 at 03:19:36PM -0700, Stefan Beller wrote:
> > > > Stefan wrote:
> > > > > See 
> > > > > https://github.com/git/git/commit/68d03e4a6e448aa557f52adef92595ac4d6cd4bd
> > > > > (68d03e4a6e (Implement automatic fast-forward merge for submodules, 
> > > > > 2010-07-07)
> > > > > to explain the situation you encounter. (specifically merge_submodule
> > > > > at the end of the diff)
> > > > 
> > > > +cc Heiko, author of that commit.
> > > 
> > > In that commit we tried to be very careful about. I do not understand
> > > the situation in which the current strategy would be wrong by default.
> > > 
> > > We only merge if the following applies:
> > > 
> > >  * The changes in the superproject on both sides point forward in the
> > >submodule.
> > > 
> > >  * One side is contained in the other. Contained from the submodule
> > >perspective. Sides from the superproject merge perspective.
> > > 
> > > So in case of the mentioned rewind of a submodule: Only one side of the
> > > 3-way merge would point forward and the merge would fail.
> > > 
> > > I can imagine, that in case of a temporary revert of a commit in the
> > > submodule that you would not want that merged into some other branch.
> > > But that would be the same without submodules. If you merge a temporary
> > > revert from another branch you will not get any conflict.
> > > 
> > > So maybe someone can explain the use case in which one would get the
> > > results that seem wrong?
> > 
> > In an ideal world, where there are no regressions between revisions, a
> > fast-forward is appropriate. However, we might have regressions within
> > submodules.
> > 
> > So the usecase is the following:
> > 
> > Environment:
> > - We have a base library L that is developed by some team (Team B).
> > - Another team (Team A) developes a product P based on those libraries 
> > using git-flow.
> > 
> > Case:
> > The problem occurs, when a developer (D) of Team A tries to have a feature
> > that he developed on a branch accepted by a core developer of P:
> > If a core developer of P advanced the reference of L within P (linear 
> > history), he might
> > deem the work D insufficient. Not because of the actual work by D, but 
> > regressions
> > that snuck into L. The core developer will not be informed about the 
> > missmatching
> > revisions of L.
> > 
> > So it would be nice if there was some kind of switch or at least some 
> > trigger.
> 
> I still do not understand how the current behaviour is mismatching with
> users expectations. Let's assume that you directly tracked the files of
> L in your product repository P, without any submodule boundary. How
> would the behavior be different? Would it be? If D started on an older
> revision and gets merged into a newer revision, there can always be
> regressions even without submodules.
> 
> Why would the core developer need to be informed about mismatching
> revisions if he himself advanced the submodule?
In that case you'd be right. I should have picked my example more wisely.
Assume right here that not a core developer, but another developer advanced
the submodule (also via feature branch + merge).
> 
> It seems to me that you do not want to mix integration testing and
> testing of the feature itself. 
That's on point. That's why it would be nice if git *at least* warned about the 
different revisions wrt submodules.

But, I guess, I learned something about submodules:
I used to think of submodules as means to pin down a specific revision like: 
`ver == x`.
Now I'm learning that submodules are treated as `ver >= x` during a merge.

> How about just testing/reviewing on the
> branch then? You would still get the submodule revision D was working on
> and then in a later stage check if integration with everything else
> works.
Sure. But if the behavior deviates after a merge the merging developer is 
currently not
aware that it *might* have to do with different submodule revisions used, not 
the "actual" code merged.

Like not even "beware: the (feature) branch you've merged used an 'older' 
revision of X"

> 
> Cheers Heiko

Cheers,

Leif

Re: [PATCH] git-send-email: allow re-editing of message

[Eric Wong]

Drew DeVault  wrote:
> When shown the email summary, an opportunity is presented for the user
> to edit the email as if they had specified --annotate. This also permits
> them to edit it multiple times.

Thanks, this seems like a good idea for the cover letter, especially.
I prefer to get the commit messages right in the git history, first,
but I more often screw up the cover letter.

I haven't looked at the code to send-email in a while,
so some thinking out loud below :>

> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1330,9 +1330,14 @@ sub file_name_is_absolute {
>   return File::Spec::Functions::file_name_is_absolute($path);
>  }
>  
> -# Returns 1 if the message was sent, and 0 otherwise.
> -# In actuality, the whole program dies when there
> -# is an error sending a message.
> +# Prepares the email, then asks the user what to do.
> +#
> +# If the user chooses to send the email, it's sent and 1 is returned.
> +# If the user chooses not to send the email, 0 is returned.
> +# If the user decides they want to make further edits, -1 is returned and the
> +# caller is expected to call send_message again after the edits are 
> performed.

OK, -1 is the new return value.  Thanks for documenting this.
The rest of the prompt implementation looks fine and I won't quote it.

> @@ -1552,7 +1559,9 @@ $references = $initial_in_reply_to || '';
>  $subject = $initial_subject;
>  $message_num = 0;
>  
> -foreach my $t (@files) {
> +sub process_file {
> + my ($t) = @_;
> +
>   open my $fh, "<", $t or die sprintf(__("can't open file %s"), $t);
>  
>   my $author = undef;

OK, process_file is a new function...

> @@ -1755,6 +1764,10 @@ foreach my $t (@files) {
>   }
>  
>   my $message_was_sent = send_message();
> + if ($message_was_sent == -1) {
> + do_edit($t);
> + return 0;
> + }

And the previously documented -1 return value is used here.
Mental note: process_file returns 0 to indicate an edit was done.

>   # set up for the next message
>   if ($thread && $message_was_sent &&
> @@ -1776,6 +1789,14 @@ foreach my $t (@files) {
>   undef $auth;
>   sleep($relogin_delay) if defined $relogin_delay;
>   }
> +
> + return 1;

Mental note: process_file normally returns 1

> +}
> +
> +foreach my $t (@files) {
> + while (!process_file($t)) {
> + # This space deliberately left blank

Cute, but that comment could say something useful, instead:

# user is still editing the file


Otherwise, I think the patch is great.  Thanks!

Re: [PATCH v2] checkout & worktree: introduce checkout.implicitRemote

[Ævar Arnfjörð Bjarmason]

On Thu, May 03 2018, Duy Nguyen wrote:

> On Thu, May 3, 2018 at 3:18 PM, Ævar Arnfjörð Bjarmason
>  wrote:
>> Introduce a checkout.implicitRemote setting which can be used to
>> designate a remote to prefer (via checkout.implicitRemote=origin) when
>> running e.g. "git checkout master" to mean origin/master, even though
>> there's other remotes that have the "master" branch.
>>
>> I want this because it's very handy to use this workflow to checkout a
>> repository and create a topic branch, then get back to a "master" as
>> retrieved from upstream:
>>
>> (
>> rm -rf /tmp/tbdiff &&
>> git clone g...@github.com:trast/tbdiff.git &&
>> cd tbdiff &&
>> git branch -m topic &&
>> git checkout master
>> )
>>
>> That will output:
>>
>> Branch 'master' set up to track remote branch 'master' from 'origin'.
>> Switched to a new branch 'master'
>>
>> But as soon as a new remote is added (e.g. just to inspect something
>> from someone else) the DWIMery goes away:
>>
>> (
>> rm -rf /tmp/tbdiff &&
>> git clone g...@github.com:trast/tbdiff.git &&
>> cd tbdiff &&
>> git branch -m topic &&
>> git remote add avar g...@github.com:avar/tbdiff.git &&
>> git fetch avar &&
>> git checkout master
>> )
>>
>> Will output:
>>
>> error: pathspec 'master' did not match any file(s) known to git.
>>
>> The new checkout.implicitRemote config allows me to say that whenever
>> that ambiguity comes up I'd like to prefer "origin", and it'll still
>> work as though the only remote I had was "origin".
>>
>> I considered splitting this into checkout.implicitRemote and
>> worktree.implicitRemote, but it's probably less confusing to break our
>> own rules that anything shared between config should live in core.*
>> than have two config settings, and I couldn't come up with a short
>> name under core.* that made sense (core.implicitRemoteForCheckout?).
>
> I wonder if it's difficult to add a dwim hook that allows us to
> replace the entire dwim logic with a hook? Doing this "preferring
> origin" in a shell script should be easy and it lets us play more with
> tweaking dwim logic. (Yeah sorry I'm getting off topic again)

Realistically the way we do hooks now would make the UI of this suck,
i.e. you couldn't configure it globally or system-wide easily. Something
like what I noted in
https://public-inbox.org/git/871sf3el01@evledraar.gmail.com/ would
make it suck less, but that's a much bigger task.

I think for now this is a common enough case users run into that it
makes sense to add it to the code, and if we ever do have more
extensible hook features in the future we can just go and replace
various options like these to use them.

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Eric Sunshine]

On Fri, May 4, 2018 at 2:52 AM, Johannes Schindelin
 wrote:
> On Thu, 3 May 2018, Eric Sunshine wrote:
>> On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
>>  wrote:
>> > +static const char * const builtin_branch_diff_usage[] = {
>> > +   N_("git rebase--helper [] ( A..B C..D | A...B | base A B 
>> > )"),
>>
>> The formatting of "" vs. "base" confused me into thinking
>> that the latter was a literal keyword, but I see from reading patch
>> 3/18 that it is not a literal at all, thus probably ought to be
>> specified as "".
>
> Good point. Or maybe BASE?

Indeed, that's probably more consistent with 'A', 'B', etc. than .

> Or I should just use the same convention as in the man page. Or not, as
> the usage should be conciser.
>
> This is what I have currently:
>
> static const char * const builtin_branch_diff_usage[] = {
> N_("git branch-diff [] .. .."),
> N_("git branch-diff [] ..."),
> N_("git branch-diff []   "),
> NULL
> };

I can live with this. It's more verbose but more self-explanatory,
thus likely a good choice.

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

[Johannes Schindelin]

Hi Junio,

On Fri, 4 May 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Johannes Schindelin (17):
> >   Add a function to solve least-cost assignment problems
> >   Add a new builtin: branch-diff
> >   branch-diff: first rudimentary implementation
> >   branch-diff: improve the order of the shown commits
> >   branch-diff: also show the diff between patches
> >   branch-diff: right-trim commit messages
> >   branch-diff: indent the diffs just like tbdiff
> >   branch-diff: suppress the diff headers
> >   branch-diff: adjust the output of the commit pairs
> >   branch-diff: do not show "function names" in hunk headers
> >   branch-diff: use color for the commit pairs
> >   color: provide inverted colors, too
> >   diff: add an internal option to dual-color diffs of diffs
> >   branch-diff: offer to dual-color the diffs
> >   branch-diff --dual-color: work around bogus white-space warning
> >   branch-diff: add a man page
> >   completion: support branch-diff
> >
> > Thomas Rast (1):
> >   branch-diff: add tests
> 
> Lovely.  
> 
> I often have to criticize a series whose later half consists of many
> follow-up patches with "don't do 'oops, the previous was wrong'",
> but the follow-up patches in this series are not such corrections.
> The organization of the series to outline the basic and core idea
> first in the minimum form and then to build on it to improve an
> aspect of the command one step at a time is very helpful to guide
> the readers where the author of the series wants them to go.

Thanks! *beams*

Ciao,
Dscho

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Johannes Schindelin]

Hi Duy,

On Fri, 4 May 2018, Duy Nguyen wrote:

> On Thu, May 3, 2018 at 10:32 PM, Johannes Schindelin
>  wrote:
> >
> > On Thu, 3 May 2018, Johannes Schindelin wrote:
> >
> >> On Thu, 3 May 2018, Duy Nguyen wrote:
> >>
> >> > On Thu, May 3, 2018 at 5:30 PM, Johannes Schindelin
> >> >  wrote:
> >> > > diff --git a/command-list.txt b/command-list.txt
> >> > > index a1fad28fd82..c89ac8f417f 100644
> >> > > --- a/command-list.txt
> >> > > +++ b/command-list.txt
> >> > > @@ -19,6 +19,7 @@ git-archive mainporcelain
> >> > >  git-bisect  mainporcelain   info
> >> > >  git-blame   ancillaryinterrogators
> >> > >  git-branch  mainporcelain   
> >> > > history
> >> > > +git-branch-diff mainporcelain   info
> >> >
> >> > Making it part of "git help" with the info keywords at this stage may
> >> > be premature. "git help" is about _common_ commands and we don't know
> >> > (yet) how popular this will be.
> >>
> >> Makes sense. I removed the `mainporcelain` keyword locally.
> >
> > On second thought, I *think* you meant to imply that I should remove that
> > line altogether. Will do that now.
> 
> Actually I only suggested to remove the last word "info". That was
> what made this command "common". Classifying all commands in this file
> is definitely a good thing, and I think mainporcelain is the right
> choice.

Oh, okay. It was not at all clear to me what the exact format and role of
these lines are... 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.

I re-added the line, this time without `info` and verified that
`branch-diff` does not show up in `git help`'s output.

Ciao,
Dscho

Re: [PATCH] checkout & worktree: introduce a core.DWIMRemote setting

[Eric Sunshine]

On Fri, May 4, 2018 at 3:14 AM, Eric Sunshine  wrote:
> The setting up of a remote-tracking branch is DWIM'd as of 4e85333197
> ("worktree: make add   dwim", 2017-11-26); it doesn't
> require an explicit option to enable it (though tracking can be
> disabled via --no-track). The "guess-remote" feature does something
> entirely different; it was added to avoid backward compatibility
> problems.
>
> In long-form:
>
> git worktree add  
>
> adds a new worktree at  and checks out . As originally
> implemented, shortened:
>
> git worktree add 
>
> does one type of DWIM, as a convenience, and pretends that the user
> actually typed:
>
> branch=$(basename )
> git branch $branch HEAD
> git workree add  $branch

This explanation isn't wrong, but it conflates two separate features,
thus is confusing. The $(basename ) DWIM'ing from the short
form of the command, "git worktree add ", doesn't actually have
anything to do with the DWIM'ing added by 4e85333197 ("worktree: make
add   dwim", 2017-11-26), which merely tries to DWIM a
remote-tracking branch of similar name to $branch (whether $branch
came from long or short form of the command), so I confused the issue
unnecessarily by talking about the short form.

Re: [PATCH 03/18] branch-diff: first rudimentary implementation

[Johannes Schindelin]

Hi Junio,

On Fri, 4 May 2018, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > Note: due to differences in the diff algorithm (`tbdiff` uses the
> > Pythong module `difflib`, Git uses its xdiff fork), the cost matrix
> 
> Pythong???

Yes, that's the French pronunciation.

Ciao,
Dscho

Re: [PATCH 17/18] branch-diff: add a man page

[Johannes Schindelin]

Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

> On Thu, May 3, 2018 at 11:31 AM, Johannes Schindelin
>  wrote:
> > 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 
> > ---
> > diff --git a/Documentation/git-branch-diff.txt 
> > b/Documentation/git-branch-diff.txt
> > @@ -0,0 +1,239 @@
> > +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
> 
> s/let/Let/

Okay. I am always a little bit fuzzy on the question whether to continue
lower-case or upper-case after a colon or semicolon.

Ciao,
Dscho

Re: [PATCH 05/18] branch-diff: also show the diff between patches

[Johannes Schindelin]

Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

> On Thu, May 3, 2018 at 11:30 AM, 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
> > beginnger.
> 
> s/beginnger/beginner/
> 
> > This brings branch-diff closer to be feature-complete with regard to
> 
> s/be feature-complete/feature parity/

Yes.

> > 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", _patches,
> > -N_("short format (no diffs)")),
> 
> This was added in 2/18 but never used...
> 
> > +   OPT_SET_INT(0, "no-patches", _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?

Sure. I actually started out by parsing even the --color option, but had
stripped that out in a rebase -i run, in favor of using diff_parse_opt()
later. I should really do the same here, too.

Thanks,
Dscho

Re: [PATCH] checkout & worktree: introduce a core.DWIMRemote setting

[Eric Sunshine]

On Wed, May 2, 2018 at 2:25 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Wed, May 02 2018, Eric Sunshine wrote:
>> A few observations:
>>
>> 1. DWIM has broad meaning; while this certainly falls within DWIM,
>> it's also just a _default_, so should this instead be named
>> "defaultRemote"?
>>
>> 2. Building on #1: How well is the term "DWIM" understood by non-power
>> users? A term, such as "default" is more well known.
>
> I've got no love for the DWIM term. And I think I should change it in
> v2, I just want some way to enable this functionality since this
> behavior has annoyed me for a long time.
>
> I wonder though if something like "core.defaultRemote" might not bring
> up connotations about whether this would e.g. affect "push" in the minds
> of users (not that my initial suggestion is better).

A reasonable concern.

> So maybe something like checkout.implicitRemote would be better? And we
> could just break the rule that only git-checkout would use it, since
> git-worktree could be said to be doing something checkout-like, or just
> also add a worktree.implicitRemote.

Considering that git-worktree runs the post-checkout hook, it seems
pretty safe to say that it does something checkout-like.

Personally, I find "defaultRemote" easier to understand than
"implicitRemote", but I suppose I can see your reasoning for choosing
"implicit". Whereas "default" is something to "fall back upon" when
something is missing, "implicit" suggests what to choose when a
something has not been specified explicitly.

>> 3. git-worktree learned --guess-remote and worktree.guessRemote in [1]
>> and [2], which, on the surface, sound confusingly similar to
>> "DWIMRemote". Even though I was well involved in the discussion and
>> repeatedly reviewed the patch series which introduced those, it still
>> took me an embarrassingly long time, and repeated re-reads of all
>> commit messages involved, to grasp (or re-grasp) how "guess remote"
>> and "DWIM remote" differ. If it was that difficult for me, as a person
>> involved in the patch series, to figure out "guess remote" vs. "DWIM
>> remote", then I worry that it may be too confusing in general. If the
>> new config option had been named "defaultRemote", I don't think I'd
>> have been confused at all.
>
> I hadn't looked at this at all before today when I wrote the patch, and
> I've never used git-worktree (but maybe I should...), but my
> understanding of this --[no-]guess-remote functionality is that it
> effectively splits up the functionality that the "git checkout" does,
> and we'll unconditionally check out the branch, but the option controls
> whether or not we'd set up the equivalent of remote tracking for
> git-worktree.
>
> But maybe I've completely misunderstood it.

Yes, you misunderstood it.

The setting up of a remote-tracking branch is DWIM'd as of 4e85333197
("worktree: make add   dwim", 2017-11-26); it doesn't
require an explicit option to enable it (though tracking can be
disabled via --no-track). The "guess-remote" feature does something
entirely different; it was added to avoid backward compatibility
problems.

In long-form:

git worktree add  

adds a new worktree at  and checks out . As originally
implemented, shortened:

git worktree add 

does one type of DWIM, as a convenience, and pretends that the user
actually typed:

branch=$(basename )
git branch $branch HEAD
git workree add  $branch

which creates a new branch and then checks it out in the new worktree
as usual. The "guess remote" feature which Thomas added augments that
by adding a DWIM which checks if $(basename ) names a
remote-tracking branch, in which case, it becomes shorthand for
(something like):

branch=$(basename )
if remote-tracking branch named $branch exists:
git branch --track $branch $guessedRemote/$branch
else:
git branch $branch HEAD
fi
git worktree add  $branch

In retrospect, this DWIM-checking for a like-named remote-tracking
branch should have been implemented from the start in git-worktree
since it mirrors how "git checkout " will DWIM remote-tracking
/. However, such DWIM'ing was overlooked and
git-worktree existed long enough without it that, due to backward
compatibility concerns, the new DWIM'ing got hidden behind a switch,
hence --guess-remote ("worktree.guessRemote") to enable it.

Unrelated: Thomas added another DWIM, which we just finalized a few
days ago, which extends the shorthand "git worktree add " to
first check if a local branch named $(basename ) already exists
and merely check that out into the new worktree rather than trying to
create a new branch of that name (which is another obvious DWIM missed
during initial implementation). In other words:

branch=$(basename )
if local branch named $branch does _not_ exist:
if remote-tracking branch named $branch exists:
git branch --track $branch $guessedRemote/$branch
else:
git branch $branch HEAD
  

Re: [PATCH 03/18] branch-diff: first rudimentary implementation

[Johannes Schindelin]

Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

> On Thu, May 3, 2018 at 11:30 AM, Johannes Schindelin
>  wrote:
> > At this stage, `git branch-diff` can determine corresponding commits of
> > two related commit ranges. This makes use of the recently introduced
> > implementation of the Hungarian algorithm.
> >
> > The core of this patch is a straight port of the ideas of tbdiff, the
> > seemingly dormant project at https://github.com/trast/tbdiff.
> >
> > The output does not at all match `tbdiff`'s output yet, as this patch
> > really concentrates on getting the patch matching part right.
> >
> > Note: due to differences in the diff algorithm (`tbdiff` uses the
> > Pythong module `difflib`, Git uses its xdiff fork), the cost matrix
> 
> s/Pythong/Python/

Yep!

> > 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).
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> > diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> > @@ -19,6 +23,279 @@ static int parse_creation_weight(const struct option 
> > *opt, const char *arg,
> > +static int read_patches(const char *range, struct string_list *list)
> > +{
> > +   [...]
> > +   struct strbuf buf = STRBUF_INIT, line = STRBUF_INIT;
> > +   [...]
> > +   } else if (starts_with(line.buf, "")) {
> > +   strbuf_addbuf(, );
> > +   strbuf_addch(, '\n');
> > +   }
> > +
> > +   continue;
> 
> Unnecessary blank line above 'continue'?

Sure.

> > +   } else if (starts_with(line.buf, "@@ "))
> > +   strbuf_addstr(, "@@");
> > +   [...]
> > +   }
> > +   fclose(in);
> > +
> > +   if (util)
> > +   string_list_append(list, buf.buf)->util = util;
> > +   strbuf_release();
> 
> strbuf_release();

Yes!

> > +   if (finish_command())
> > +   return -1;
> > +
> > +   return 0;
> > +}
> > @@ -32,9 +309,63 @@ int cmd_branch_diff(int argc, const char **argv, const 
> > char *prefix)
> > +   if (argc == 2) {
> > +   if (!strstr(argv[0], ".."))
> > +   warning(_("no .. in range: '%s'"), argv[0]);
> > +   strbuf_addstr(, argv[0]);
> > +
> > +   if (!strstr(argv[1], ".."))
> > +   warning(_("no .. in range: '%s'"), argv[1]);
> > +   strbuf_addstr(, argv[1]);
> > +   } else if (argc == 1) {
> > +   if (!b)
> > +   die(_("single arg format requires a symmetric 
> > range"));
> > +   } else {
> > +   error("Need two commit ranges");
> 
> Other warning/error messages emitted by this function are not
> capitalized: s/Need/need/

Right. And it is also not translated. Fixed both.

Thank you for helping me make this patch series better!

Ciao,
Dscho

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Johannes Schindelin]

Hi Eric,

On Thu, 3 May 2018, Eric Sunshine wrote:

> On Thu, May 3, 2018 at 11:30 AM, 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`.
> >
> > At this point, we ignore tbdiff's color options, as they will all be
> > implemented later and require some patches to the diff machinery.
> >
> > Signed-off-by: Johannes Schindelin 
> > ---
> > diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> > @@ -0,0 +1,40 @@
> > +static const char * const builtin_branch_diff_usage[] = {
> > +   N_("git rebase--helper [] ( A..B C..D | A...B | base A B 
> > )"),
> > +   NULL
> > +};
> 
> The formatting of "" vs. "base" confused me into thinking
> that the latter was a literal keyword, but I see from reading patch
> 3/18 that it is not a literal at all, thus probably ought to be
> specified as "".

Good point. Or maybe BASE?

Or I should just use the same convention as in the man page. Or not, as
the usage should be conciser.

This is what I have currently:

static const char * const builtin_branch_diff_usage[] = {
N_("git branch-diff [] .. .."),
N_("git branch-diff [] ..."),
N_("git branch-diff []   "),
NULL
};

Thanks,
Dscho

Re: [PATCH 11/18] branch-diff: add tests

[Johannes Schindelin]

Hi Philip,

On Fri, 4 May 2018, Philip Oakley wrote:

> From: "Johannes Schindelin" 
> > From: Thomas Rast 
> > 
> > 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
> 
> s/emtpy/empty/

Yep, that has been pointed out by others, too... ;-)

It is good that this thing gets a really good review *grins*

Ciao,
Dscho

Re: [PATCH 02/18] Add a new builtin: branch-diff

[Johannes Schindelin]

Hi Ramsay,

On Fri, 4 May 2018, Ramsay Jones wrote:

> On 03/05/18 21:25, Johannes Schindelin wrote:
> 
> > On Thu, 3 May 2018, Ramsay Jones wrote:
> 
> >> On 03/05/18 16:30, Johannes Schindelin wrote:
> [snip]
> 
> >>> diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
> >>> new file mode 100644
> >>> index 000..97266cd326d
> >>> --- /dev/null
> >>> +++ b/builtin/branch-diff.c
> >>> @@ -0,0 +1,40 @@
> >>> +#include "cache.h"
> >>> +#include "parse-options.h"
> >>> +
> >>> +static const char * const builtin_branch_diff_usage[] = {
> >>> + N_("git rebase--helper [] ( A..B C..D | A...B | base A B )"),
> >>
> >> s/rebase--helper/branch-diff/
> > 
> > Whoops!
> > 
> > BTW funny side note: when I saw that you replied, I instinctively thought
> > "oh no, I forgot to mark a function as `static`!" ;-)
> 
> Heh, but I hadn't got around to applying the patches and building
> git yet! ;-)

;-)

> Sparse has two complaints:
> 
>   > SP builtin/branch-diff.c
>   > builtin/branch-diff.c:433:41: warning: Using plain integer as NULL pointer
>   > builtin/branch-diff.c:431:5: warning: symbol 'cmd_branch_diff' was not 
> declared. Should it be static?
> 
> I suppressed those warnings with the following patch (on top
> of these patches):
> 
>   $ git diff
>   diff --git a/builtin/branch-diff.c b/builtin/branch-diff.c
>   index edf80ecb7..1373c22f4 100644
>   --- a/builtin/branch-diff.c
>   +++ b/builtin/branch-diff.c
>   @@ -1,4 +1,5 @@
>#include "cache.h"
>   +#include "builtin.h"
>#include "parse-options.h"
>#include "string-list.h"
>#include "run-command.h"
>   @@ -430,7 +431,7 @@ static void output(struct string_list *a, struct 
> string_list *b,
>  
>int cmd_branch_diff(int argc, const char **argv, const char *prefix)
>{
>   -   struct diff_options diffopt = { 0 };
>   +   struct diff_options diffopt = { NULL };
>   struct strbuf four_spaces = STRBUF_INIT;
>   int dual_color = 0;
>   double creation_weight = 0.6;
>   $ 

Thanks!

> The first hunk applies to patch 02/18 (ie this very patch) and
> the second hunk should be applied to patch 05/18 (ie, "branch-diff:
> also show the diff between patches").

I actually have a hacky script to fixup commits in a patch series. It lets
me stage part of the current changes, then figures out which of the
commits' changes overlap with the staged changed. If there is only one
commit, it automatically commits with --fixup, otherwise it lets me choose
which one I want to fixup (giving me the list of candidates).

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?

Ciao,
Dscho