date:20170629

The notice that "git commit " default to "git commit
--only " was there since 756e3ee0 ("Merge branch
'jc/commit'", 2006-02-14).  Back then, existing users of Git
expected the command doing "git commit --include ", and
after we changed the behaviour of the command to align with
other people's "$scm commit ", we added the text to help
them transition their expectations.  Remove the message that now
has outlived its usefulness.
---
 builtin/commit.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8d1cac062..64701c8f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -139,7 +139,6 @@ static enum commit_whence whence;
 static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
-static const char *only_include_assumed;
 static struct strbuf message = STRBUF_INIT;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
@@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
  "with '%c' will be kept; you may remove them"
  " yourself if you want to.\n"
  "An empty message aborts the commit.\n"), 
comment_line_char);
-   if (only_include_assumed)
-   status_printf_ln(s, GIT_COLOR_NORMAL,
-   "%s", only_include_assumed);
 
/*
 * These should never fail because they come from our own
@@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const 
char *argv[],
die(_("Only one of --include/--only/--all/--interactive/--patch 
can be used."));
if (argc == 0 && (also || (only && !amend && !allow_empty)))
die(_("No paths with --include/--only does not make sense."));
-   if (argc > 0 && !also && !only)
-   only_include_assumed = _("Explicit paths specified without -i 
or -o; assuming --only paths...");
if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
else if (!strcmp(cleanup_arg, "verbatim"))
-- 
2.11.0

[PATCH 2/2] commit-template: add new line before status information

The commit template adds the optional parts without
a new line to distinguish them. This results in
difficulty in interpreting it's content, specifically
for inexperienced users.

Add new lines to separate the distinct parts of the
template.
---
 I tried writing tests to ensure that the new line is added
 but as it seems to require checking multi-line, special 
 options of grep were required to check. I tried the following,

   test_expect_success 'new line found before status message' '
! (GIT_EDITOR="cat >editor-input" git commit) &&
grep -Pz "#\n# On branch" editor-input
   '

 It worked well locally but seems to make the build with 
 GETTEXT_POISON=YesPlease to fail. So, I removed it.
 Not sure how to write a good test for this change, sorry :(

 builtin/commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 64701c8f4..22d17e6f2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -873,8 +873,7 @@ static int prepare_to_commit(const char *index_file, const 
char *prefix,
(int)(ci.name_end - ci.name_begin), 
ci.name_begin,
(int)(ci.mail_end - ci.mail_begin), 
ci.mail_begin);
 
-   if (ident_shown)
-   status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
+   status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new 
line for clarity */
 
saved_color_setting = s->use_color;
s->use_color = 0;
-- 
2.11.0

Re: [PATCH 2/5] grep: remove redundant grep pattern type assignment

On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
 wrote:
> Remove a redundant assignment to extended_regexp_option to make it
> zero if grep.extendedRegexp is not set. This is always called right
> after init_grep_defaults() which memsets the entire structure to 0.
>
> This is a logical follow-up to my commit to remove redundant regflags
> assignments[1]. This logic was originally introduced in [2], but as
> explained in the former commit it's working around a pattern in our
> code that no longer exists, and is now confusing as it leads the
> reader to think that this needs to be flipped back & forth.
>
> 1. e0b9f8ae09 ("grep: remove redundant regflags assignments",
>2017-05-25)
> 2. b22520a37c ("grep: allow -E and -n to be turned on by default via
>configuration", 2011-03-30)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  grep.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 29439886e7..6614042fdc 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void 
> *cb)
> if (!strcmp(var, "grep.extendedregexp")) {
> if (git_config_bool(var, value))
> opt->extended_regexp_option = 1;
> -   else
> -   opt->extended_regexp_option = 0;
> return 0;

Instead of having a condition here, have you considered to remove the
condition alltogether?

if (!strcmp(var, "grep.extendedregexp")) {
opt->extended_regexp_option = git_config_bool(var, value);
return 0;
}

This does not have the effect of not assigning the value in case of 0,
but it may be easier to reason about when reading the code.

This would also conform to the code below in that function, that parses
grep.linenumber or grep.fullname

Thanks,
Stefan

Re: [PATCH 3/5] grep: remove redundant "fixed" field re-assignment to 0

On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
 wrote:
> Remove the redundant re-assignment of the fixed field to zero right
> after the entire struct has been set to zero via memset(...).
>
> Unlike some nearby commits this pattern doesn't date back to the
> pattern described in e0b9f8ae09 ("grep: remove redundant regflags
> assignments", 2017-05-25), instead it was apparently cargo-culted in
> 9eceddeec6 ("Use kwset in grep", 2011-08-21).
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  grep.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/grep.c b/grep.c
> index 6614042fdc..7cd8a6512f 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -627,8 +627,6 @@ static void compile_regexp(struct grep_pat *p, struct 
> grep_opt *opt)
> has_null(p->pattern, p->patternlen) ||
> is_fixed(p->pattern, p->patternlen))
> p->fixed = !icase || ascii_only;
> -   else
> -   p->fixed = 0;
>

I was about to propose a similar action as in 2/5,
but getting the condition right is not as easy:

p->fixed = (opt->fixed ||
   has_null(p->pattern, p->patternlen) ||
   is_fixed(p->pattern, p->patternlen)) &&
   (!icase || ascii_only);

does not look as convincing here.

Thanks for mentioning 9eceddeec6 as in that commit
I would have been easy with just proposing to have

p->fixed = opt->fixed || is_fixed(p->pattern, p->patternlen);

Thanks,
Stefan

Re: [PATCH 5/5] grep: remove regflags from the public grep_opt API

On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
 wrote:
> Refactor calls to the grep machinery to always pass opt.ignore_case &
> opt.extended_regexp_option instead of setting the equivalent regflags
> bits.
>
> The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
> make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
> really just plastering over the code smell which this change fixes.
>
> See my "Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with
> --perl-regexp"[1] for the discussion leading up to this.
>
> The reason for adding the extensive commentary here is that I
> discovered some subtle complexity in implementing this that really
> should be called out explicitly to future readers.
>
> Before this change we'd rely on the difference between
> `extended_regexp_option` and `regflags` to serve as a membrane between
> our preliminary parsing of grep.extendedRegexp and grep.patternType,
> and what we decided to do internally.
>
> Now that those two are the same thing, it's necessary to unset
> `extended_regexp_option` just before we commit in cases where both of
> those config variables are set. See 84befcd0a4 ("grep: add a
> grep.patternType configuration setting", 2012-08-03) for the code and
> documentation related to that.
>
> The explanation of why the if/else branches in
> grep_commit_pattern_type() are ordered the way they are exists in that
> commit message, but I think it's worth calling this subtlety out
> explicitly with a comment for future readers.

Up to here the commit message is inspiring confidence.

>
> Unrelated to that: I could have factored out the default REG_NEWLINE
> flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but
> since it's just used in two places I didn't think it was worth the
> effort.
>
> As an aside we're really lacking test coverage regflags being
> initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's
> removed from compile_regexp(), but not if it's removed from
> compile_fixed_regexp(). I have not dug to see if it's actually needed
> in the latter case or if the test coverage is lacking.

This sounds as if extra careful review is needed.


>
> 1. 
>
> (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1k...@mail.gmail.com/)
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  builtin/grep.c |  2 --
>  grep.c | 43 ++-
>  grep.h |  1 -
>  revision.c |  2 --
>  4 files changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index f61a9d938b..b682966439 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char 
> *prefix)
>
> if (!opt.pattern_list)
> die(_("no pattern given."));
> -   if (!opt.fixed && opt.ignore_case)
> -   opt.regflags |= REG_ICASE;
>
> /*
>  * We have to find "--" in a separate pass, because its presence
> diff --git a/grep.c b/grep.c
> index 736e1e00d6..51aaad9f03 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -35,7 +35,6 @@ void init_grep_defaults(void)
> memset(opt, 0, sizeof(*opt));
> opt->relative = 1;
> opt->pathname = 1;
> -   opt->regflags = REG_NEWLINE;
> opt->max_depth = -1;
> opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
> color_set(opt->color_context, "");
> @@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
> opt->linenum = def->linenum;
> opt->max_depth = def->max_depth;
> opt->pathname = def->pathname;
> -   opt->regflags = def->regflags;
> opt->relative = def->relative;
> opt->output = def->output;
>
> @@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>
>  static void grep_set_pattern_type_option(enum grep_pattern_type 
> pattern_type, struct grep_opt *opt)
>  {
> +   /*
> +* When committing to the pattern type by setting the relevant
> +* fields in grep_opt it's generally not necessary to zero out
> +* the fields we're not choosing, since they won't have been
> +* set by anything. The extended_regexp_option field is the
> +* only exception to this.
> +*
> +* This is because in the process of parsing grep.patternType
> +* & grep.extendedRegexp we set opt->pattern_type_option and
> +* opt->extended_regexp_option, respectively. We then
> +* internally use opt->extended_regexp_option to see if we're
> +* compiling an ERE. It must be unset if that's not actually
> +* the case.
> +*/
> +   if (pattern_type != GREP_PATTERN_TYPE_ERE &&
> +   opt->extended_regexp_option)
> +   opt->extended_regexp_option = 0;
> +
> switch (pattern_type) {
> case GREP_PATTERN_TYPE_UNSPECIFIED:
> /* fall through */
>

Re: [PATCH 2/5] grep: remove redundant grep pattern type assignment


On Thu, Jun 29 2017, Stefan Beller jotted:

> On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
>  wrote:
>> Remove a redundant assignment to extended_regexp_option to make it
>> zero if grep.extendedRegexp is not set. This is always called right
>> after init_grep_defaults() which memsets the entire structure to 0.
>>
>> This is a logical follow-up to my commit to remove redundant regflags
>> assignments[1]. This logic was originally introduced in [2], but as
>> explained in the former commit it's working around a pattern in our
>> code that no longer exists, and is now confusing as it leads the
>> reader to think that this needs to be flipped back & forth.
>>
>> 1. e0b9f8ae09 ("grep: remove redundant regflags assignments",
>>2017-05-25)
>> 2. b22520a37c ("grep: allow -E and -n to be turned on by default via
>>configuration", 2011-03-30)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>>  grep.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/grep.c b/grep.c
>> index 29439886e7..6614042fdc 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -80,8 +80,6 @@ int grep_config(const char *var, const char *value, void 
>> *cb)
>> if (!strcmp(var, "grep.extendedregexp")) {
>> if (git_config_bool(var, value))
>> opt->extended_regexp_option = 1;
>> -   else
>> -   opt->extended_regexp_option = 0;
>> return 0;
>
> Instead of having a condition here, have you considered to remove the
> condition alltogether?
>
> if (!strcmp(var, "grep.extendedregexp")) {
> opt->extended_regexp_option = git_config_bool(var, value);
> return 0;
> }
>
> This does not have the effect of not assigning the value in case of 0,
> but it may be easier to reason about when reading the code.
>
> This would also conform to the code below in that function, that parses
> grep.linenumber or grep.fullname

I didn't think about that. Good point. I'll do that instead in v2.

Re: [PATCH 2/2] commit-template: add new line before status information

Kaartic Sivaraam  writes:

> The commit template adds the optional parts without
> a new line to distinguish them. This results in
> difficulty in interpreting it's content, specifically
> for inexperienced users.
>
> Add new lines to separate the distinct parts of the
> template.
> ---
>  I tried writing tests to ensure that the new line is added
>  but as it seems to require checking multi-line, special 
>  options of grep were required to check. I tried the following,
>
>test_expect_success 'new line found before status message' '
> ! (GIT_EDITOR="cat >editor-input" git commit) &&
> grep -Pz "#\n# On branch" editor-input
>'
>
>  It worked well locally but seems to make the build with 
>  GETTEXT_POISON=YesPlease to fail. So, I removed it.
>  Not sure how to write a good test for this change, sorry :(

The above is a good way to capture the input to the editor, but the
test with "grep -P" which is not portable would not work well.  You
however should be able to prepare an expected output with

cat >expect <<\-EOF &&
... expected contents to editor-input here ...
EOF

and do "test_i18ncmp expect editor-input" instead of "grep -P".

Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths

Kaartic Sivaraam  writes:

> The notice that "git commit " default to "git commit
> --only " was there since 756e3ee0 ("Merge branch
> 'jc/commit'", 2006-02-14).  Back then, existing users of Git
> expected the command doing "git commit --include ", and
> after we changed the behaviour of the command to align with
> other people's "$scm commit ", we added the text to help
> them transition their expectations.  Remove the message that now
> has outlived its usefulness.
> ---
>  builtin/commit.c | 6 --
>  1 file changed, 6 deletions(-)

When I said "I would have ... if I were doing this", I merely meant
exactly that---as I weren't doing it, I left it up to you.  But you
did it the way anyways, which is very nice of you ;-).

Looks good.  Should we consider this signed-off by you?

Thanks.

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 8d1cac062..64701c8f4 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -139,7 +139,6 @@ static enum commit_whence whence;
>  static int sequencer_in_use;
>  static int use_editor = 1, include_status = 1;
>  static int show_ignored_in_status, have_option_m;
> -static const char *only_include_assumed;
>  static struct strbuf message = STRBUF_INIT;
>  
>  static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
> @@ -841,9 +840,6 @@ static int prepare_to_commit(const char *index_file, 
> const char *prefix,
> "with '%c' will be kept; you may remove them"
> " yourself if you want to.\n"
> "An empty message aborts the commit.\n"), 
> comment_line_char);
> - if (only_include_assumed)
> - status_printf_ln(s, GIT_COLOR_NORMAL,
> - "%s", only_include_assumed);
>  
>   /*
>* These should never fail because they come from our own
> @@ -1208,8 +1204,6 @@ static int parse_and_validate_options(int argc, const 
> char *argv[],
>   die(_("Only one of --include/--only/--all/--interactive/--patch 
> can be used."));
>   if (argc == 0 && (also || (only && !amend && !allow_empty)))
>   die(_("No paths with --include/--only does not make sense."));
> - if (argc > 0 && !also && !only)
> - only_include_assumed = _("Explicit paths specified without -i 
> or -o; assuming --only paths...");
>   if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
>   cleanup_mode = use_editor ? CLEANUP_ALL : CLEANUP_SPACE;
>   else if (!strcmp(cleanup_arg, "verbatim"))

Re: [PATCH 1/2] hashmap.h: compare function has access to a data field

Stefan Beller  writes:

> When using the hashmap a common need is to have access to arbitrary data
> in the compare function. A couple of times we abuse the keydata field
> to pass in the data needed. This happens for example in patch-ids.c.

It is not "arbitrary data"; it is very important to streess that we
are not just passing random crud, but adding a mechansim to
tailor/curry the function in a way that is fixed throughout the
lifetime of a hashmap.

I haven't looked at the use of keydata in patch-ids.c and friends to
decide if that "abuse" claim is correct; if it were the case, should
we expect that a follow-up patch to clean up the existing mess by
using the new mechanism?  Or does fixing the "abuse" take another
mechanism that is different from this one?

> While at it improve the naming of the fields of all compare functions used
> by hashmaps by ensuring unused parameters are prefixed with 'unused_' and
> naming the parameters what they are (instead of 'unused' make it
> 'unused_keydata').
>
> Signed-off-by: Stefan Beller 
> ---

> diff --git a/hashmap.h b/hashmap.h
> index de6022a3a9..1c26bbad5b 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -33,11 +33,12 @@ struct hashmap_entry {
>  };
>  
>  typedef int (*hashmap_cmp_fn)(const void *entry, const void *entry_or_key,
> - const void *keydata);
> + const void *keydata, const void *cbdata);

As I view the new "data" thing the C's (read: poor-man's) way to
cutomize the function, I would have tweaked the function signature
by giving the customization data at the front, i.e.

fn(fndata, entry, entry_or_key, keydata);

as I think the way we wish to be able to express it in
a better language would have been something like:

(partial_apply(fn, fndata))(entry, entry_or_key, keydata)

But what you did is OK, too.

> +extern void hashmap_init(struct hashmap *map,
> +  hashmap_cmp_fn equals_function,
> +  const void *data,
> +  size_t initial_size);

And this does match my expectation ;-).

Re: [PATCH 1/2] hashmap.h: compare function has access to a data field

Junio C Hamano  writes:

> I haven't looked at the use of keydata in patch-ids.c and friends to
> decide if that "abuse" claim is correct; if it were the case, should
> we expect that a follow-up patch to clean up the existing mess by
> using the new mechanism?  Or does fixing the "abuse" take another
> mechanism that is different from this one?

I see that you corrected patch-ids.c "while at it".  That may make
it harder to revert only that "while at it", I suspect.

Thanks.

Re: [PATCH 5/5] grep: remove regflags from the public grep_opt API


On Thu, Jun 29 2017, Stefan Beller jotted:

> On Wed, Jun 28, 2017 at 2:58 PM, Ævar Arnfjörð Bjarmason
>  wrote:
>> Refactor calls to the grep machinery to always pass opt.ignore_case &
>> opt.extended_regexp_option instead of setting the equivalent regflags
>> bits.
>>
>> The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
>> make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
>> really just plastering over the code smell which this change fixes.
>>
>> See my "Re: [PATCH v3 05/30] log: make --regexp-ignore-case work with
>> --perl-regexp"[1] for the discussion leading up to this.
>>
>> The reason for adding the extensive commentary here is that I
>> discovered some subtle complexity in implementing this that really
>> should be called out explicitly to future readers.
>>
>> Before this change we'd rely on the difference between
>> `extended_regexp_option` and `regflags` to serve as a membrane between
>> our preliminary parsing of grep.extendedRegexp and grep.patternType,
>> and what we decided to do internally.
>>
>> Now that those two are the same thing, it's necessary to unset
>> `extended_regexp_option` just before we commit in cases where both of
>> those config variables are set. See 84befcd0a4 ("grep: add a
>> grep.patternType configuration setting", 2012-08-03) for the code and
>> documentation related to that.
>>
>> The explanation of why the if/else branches in
>> grep_commit_pattern_type() are ordered the way they are exists in that
>> commit message, but I think it's worth calling this subtlety out
>> explicitly with a comment for future readers.
>
> Up to here the commit message is inspiring confidence.

Thanks.

>>
>> Unrelated to that: I could have factored out the default REG_NEWLINE
>> flag into some custom GIT_GREP_H_DEFAULT_REGFLAGS or something, but
>> since it's just used in two places I didn't think it was worth the
>> effort.
>>
>> As an aside we're really lacking test coverage regflags being
>> initiated as 0 instead of as REG_NEWLINE. Tests will fail if it's
>> removed from compile_regexp(), but not if it's removed from
>> compile_fixed_regexp(). I have not dug to see if it's actually needed
>> in the latter case or if the test coverage is lacking.
>
> This sounds as if extra careful review is needed.

Note though (since I didn't say this explicitly) nothing about this
commit changes the semanics of what we pass to regcomp, I'm just noting
this caveat with REG_NEWLINE as an aside since I'm moving it around.

>>
>> 1. 
>>
>> (https://public-inbox.org/git/CACBZZX6Hp4Q4TOj_X1fbdCA4twoXF5JemZ5ZbEn7wmkA=1k...@mail.gmail.com/)
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason 
>> ---
>>  builtin/grep.c |  2 --
>>  grep.c | 43 ++-
>>  grep.h |  1 -
>>  revision.c |  2 --
>>  4 files changed, 34 insertions(+), 14 deletions(-)
>>
>> diff --git a/builtin/grep.c b/builtin/grep.c
>> index f61a9d938b..b682966439 100644
>> --- a/builtin/grep.c
>> +++ b/builtin/grep.c
>> @@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char 
>> *prefix)
>>
>> if (!opt.pattern_list)
>> die(_("no pattern given."));
>> -   if (!opt.fixed && opt.ignore_case)
>> -   opt.regflags |= REG_ICASE;
>>
>> /*
>>  * We have to find "--" in a separate pass, because its presence
>> diff --git a/grep.c b/grep.c
>> index 736e1e00d6..51aaad9f03 100644
>> --- a/grep.c
>> +++ b/grep.c
>> @@ -35,7 +35,6 @@ void init_grep_defaults(void)
>> memset(opt, 0, sizeof(*opt));
>> opt->relative = 1;
>> opt->pathname = 1;
>> -   opt->regflags = REG_NEWLINE;
>> opt->max_depth = -1;
>> opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
>> color_set(opt->color_context, "");
>> @@ -154,7 +153,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>> opt->linenum = def->linenum;
>> opt->max_depth = def->max_depth;
>> opt->pathname = def->pathname;
>> -   opt->regflags = def->regflags;
>> opt->relative = def->relative;
>> opt->output = def->output;
>>
>> @@ -170,6 +168,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
>>
>>  static void grep_set_pattern_type_option(enum grep_pattern_type 
>> pattern_type, struct grep_opt *opt)
>>  {
>> +   /*
>> +* When committing to the pattern type by setting the relevant
>> +* fields in grep_opt it's generally not necessary to zero out
>> +* the fields we're not choosing, since they won't have been
>> +* set by anything. The extended_regexp_option field is the
>> +* only exception to this.
>> +*
>> +* This is because in the process of parsing grep.patternType
>> +* & grep.extendedRegexp we set opt->pattern_type_option and
>> +* opt->extended_regexp_option, respectively. We then
>> +* internally use opt->extended_regexp_option to see if we're
>>

Re: [PATCH 2/2] commit-template: add new line before status information

Kaartic Sivaraam  writes:

> The commit template adds the optional parts without
> a new line to distinguish them. This results in
> difficulty in interpreting it's content, specifically
> for inexperienced users.
>
> Add new lines to separate the distinct parts of the
> template.

The rationale of this has changed in this final version, hasn't it,
especially with the removal of the "include/only warning" bit?

We used to add a blank line to separate the "we are committing for
somebody else", which is an optional part, from the status output,
only when we have the optional part.  Now with your change, we
unconditionally have a blank before the part that would have been
shown by "git status" to separate the two parts out.



> ---
>  I tried writing tests to ensure that the new line is added
>  but as it seems to require checking multi-line, special 
>  options of grep were required to check. I tried the following,
>
>test_expect_success 'new line found before status message' '
> ! (GIT_EDITOR="cat >editor-input" git commit) &&
> grep -Pz "#\n# On branch" editor-input
>'
>
>  It worked well locally but seems to make the build with 
>  GETTEXT_POISON=YesPlease to fail. So, I removed it.
>  Not sure how to write a good test for this change, sorry :(
>
>  builtin/commit.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 64701c8f4..22d17e6f2 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -873,8 +873,7 @@ static int prepare_to_commit(const char *index_file, 
> const char *prefix,
>   (int)(ci.name_end - ci.name_begin), 
> ci.name_begin,
>   (int)(ci.mail_end - ci.mail_begin), 
> ci.mail_begin);
>  
> - if (ident_shown)
> - status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
> + status_printf_ln(s, GIT_COLOR_NORMAL, "%s", ""); /* Add new 
> line for clarity */
>  
>   saved_color_setting = s->use_color;
>   s->use_color = 0;

Re: [PATCH 1/2] hashmap.h: compare function has access to a data field

On Thu, Jun 29, 2017 at 11:11 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> I haven't looked at the use of keydata in patch-ids.c and friends to
>> decide if that "abuse" claim is correct; if it were the case, should
>> we expect that a follow-up patch to clean up the existing mess by
>> using the new mechanism?  Or does fixing the "abuse" take another
>> mechanism that is different from this one?
>
> I see that you corrected patch-ids.c "while at it".  That may make
> it harder to revert only that "while at it", I suspect.
>
> Thanks.

Yes it was a last minute squash before sending it out, as the fix was only
two lines whereas the conversion is a lot. If it were separated I could have
claimed the introduction to be a rather mechanical patch, but I did not
make use of coccinelle or such, so the likelihood for errors is just as high.

So I decided to squash them.

Thanks,
Stefan

Re: [PATCH v8 6/6] convert: add "status=delayed" to filter process protocol

You seem to have squashed an unrelated "table-driven" clean-up into
this step.  While I think the end result looks good, I would have
liked to see it as a separate step, either as a preparatory "now we
are going to add the third one, let's make it table-driven before
doing so" step, or a follow-up "now we have done everything we
wanted to do, let's make this clean-up at the end" step.

Thanks.

Re: Warning suggestion for git stash drop

Laurent Humblet  writes:

> Would it be possible to add an optional Yes/No when doing a 'git stash
> drop'?  Something similar as what happens when pushing on a remotely
> checked out branch (with a config setting to turn the warning on/off).
>
> I know that you can always get your dropped stash back using git
> reflog but a small warning might be a useful feature to avoid unwanted
> stash drops on a regular basis.

I sympathize with this, but the same principle also would apply many
destructive commands like "git reset --hard", "git rm ", etc.
and also "/bin/rm -f" ;-)

I however do not think a good general way to do this without
breaking people's scripts.  When they do 'stash drop' in their
scripts, they know they want to get rid of the dropped stash
entries, and they expect that the user may not necessarily be
sitting in front of the shell to give the command a Yes (they
probably wouldn't even give the user a message "the next step
may ask you to say Yes; please do so").

On the other hand, just like "git reset --hard" and "git clean -f"
does not have such safety (i.e. the user is aware that the command
is destructive by giving "--hard" and "-f"), "drop" may be a sign
that the user knowingly doing something destructive.

So I dunno.

Re: [PATCH v8 6/6] convert: add "status=delayed" to filter process protocol

2017-06-29 Thread Lars Schneider


> On 29 Jun 2017, at 20:34, Junio C Hamano  wrote:
> 
> You seem to have squashed an unrelated "table-driven" clean-up into
> this step.  While I think the end result looks good, I would have
> liked to see it as a separate step, either as a preparatory "now we
> are going to add the third one, let's make it table-driven before
> doing so" step, or a follow-up "now we have done everything we
> wanted to do, let's make this clean-up at the end" step.

I am sorry. That was a misunderstanding - I thought you want to have 
this change squashed. 

The "preparatory" patch sounds good! 
Would you be OK if I send a v9 with this change?

Thanks,
Lars

RFC: Missing blob hook might be invoked infinitely recursively

2017-06-29 Thread Jonathan Tan

As some of you may know, I'm currently working on support for partial
clones/fetches in Git (where blobs above a user-specified size threshold
are not downloaded - only their names and sizes are downloaded). To do
this, the client repository needs to be able to download blobs at will
whenever it needs a missing one (for example, upon checkout).

So I have done this by adding support for a hook in Git [1], and
updating the object-reading code in Git to, by default, automatically
invoke this hook whenever necessary. (This means that existing
subsystems will all work by default, in theory at least.) My current
design is for the hook to have maximum flexibility - when invoked with a
list of SHA-1s, it must merely ensure that those objects are in the
local repo, whether packed or loose.

I am also working on a command (fetch-blob) to be bundled with Git to be
used as a default hook, and here is where the problem lies.

Suppose you have missing blob AB12 and CD34 that you now need, so
fetch-blob is invoked. It sends the literals AB12 and CD34 to a new
server endpoint and obtains a packfile, which it then pipes through "git
index-pack". The issue is that "git index-pack" wants to try to access
AB12 and CD34 in the local repo in order to do a SHA-1 collision check,
and therefore fetch-blob is invoked once again, creating infinite
recursion.

This is straightforwardly fixed by making "git index-pack" understand
about missing blobs, but this might be a symptom of this approach being
error-prone (custom hooks that invoke any Git command must be extra
careful).

So I have thought of a few solutions, each with its pros and cons:

1. Require the hook to instead output a packfile to stdout. This means
that that hook no longer needs to access the local repo, and thus has
less dependence on Git commands. But this reduces the flexibility in
that its output must be packed, not loose. (This is fine for the use
cases I'm thinking of, but probably not so for others.)

2. Add support for an environment variable to Git that suppresses access
to the missing blob manifest, in effect, suppressing invocation of the
hook. This allows anyone (the person configuring Git or the hook writer)
to suppress this access, although they might need in-depth knowledge to
know whether the hook is meant to be run with such access suppressed or
required.

3. Like the above, except for a command-line argument to Git.

What do you think? Any solutions that I am missing?

[1] Work in progress, but you can see an earlier version here:
https://public-inbox.org/git/b917a463f0ad4ce0ab115203b3f24894961a2e75.1497558851.git.jonathanta...@google.com/

Re: Warning suggestion for git stash drop

On Thu, Jun 29, 2017 at 11:44 AM, Junio C Hamano  wrote:
> Laurent Humblet  writes:
>
>> Would it be possible to add an optional Yes/No when doing a 'git stash
>> drop'?  Something similar as what happens when pushing on a remotely
>> checked out branch (with a config setting to turn the warning on/off).
>>
>> I know that you can always get your dropped stash back using git
>> reflog but a small warning might be a useful feature to avoid unwanted
>> stash drops on a regular basis.
>
> I sympathize with this, but the same principle also would apply many
> destructive commands like "git reset --hard", "git rm ", etc.
> and also "/bin/rm -f" ;-)
>
> I however do not think a good general way to do this without
> breaking people's scripts.  When they do 'stash drop' in their
> scripts, they know they want to get rid of the dropped stash
> entries, and they expect that the user may not necessarily be
> sitting in front of the shell to give the command a Yes (they
> probably wouldn't even give the user a message "the next step
> may ask you to say Yes; please do so").
>
> On the other hand, just like "git reset --hard" and "git clean -f"
> does not have such safety (i.e. the user is aware that the command
> is destructive by giving "--hard" and "-f"), "drop" may be a sign
> that the user knowingly doing something destructive.
>
> So I dunno.
>

I was about to propose to have an easy undo operation, such as the
reflog. But then I checked the man page for git-stash and it already says:

   older stashes are found in the reflog of this reference and can be named
   using the usual reflog syntax (e.g. stash@{0} is the most recently
   created stash, stash@{1} is the one before it, stash@{2.hours.ago} is
   also possible). Stashes may also be referenced by specifying
just the stash
   index (e.g. the integer n is equivalent to stash@{n})

Maybe that needs to be polished a bit more?

I myself used the stash back then (I don't use it any more), and I assumed
the stash was a stack of changes, but the data structure seems to imply it
is only one thing that can be stashed and the reflog enables the stacking
part, such that a dropped stash is gone and is not recorded in the reflog.
Dropping a stash seems to just erase the topmost line from the stash reflog,
so it really is as destructive an "/bin/rm -f"

So getting back a dropped stash via the reflog is not via asking for a stash
reflog but rather for HEADs or a branchs reflog, which may be complicated.

Re: RFC: Missing blob hook might be invoked infinitely recursively

2017-06-29 Thread Jonathan Nieder

Hi,

Jonathan Tan wrote:

> Suppose you have missing blob AB12 and CD34 that you now need, so
> fetch-blob is invoked. It sends the literals AB12 and CD34 to a new
> server endpoint and obtains a packfile, which it then pipes through "git
> index-pack". The issue is that "git index-pack" wants to try to access
> AB12 and CD34 in the local repo in order to do a SHA-1 collision check,
> and therefore fetch-blob is invoked once again, creating infinite
> recursion.
[...]
> 2. Add support for an environment variable to Git that suppresses access
> to the missing blob manifest, in effect, suppressing invocation of the
> hook. This allows anyone (the person configuring Git or the hook writer)
> to suppress this access, although they might need in-depth knowledge to
> know whether the hook is meant to be run with such access suppressed or
> required.

Small tweak: what if Git itself sets that environment variable when
invoking the hook?  A fetch-blob hook author can then explicitly unset
the environment variable to request recursion if they need it.

I should credit Shawn Pearce for saying this a little more clearly
offline a moment ago.

Thanks,
Jonathan

Re: [PATCH] completion: optionally disable checkout DWIM

2017-06-29 Thread Jason Karns

> We could discern between more than just empty vs. non-empty state of
> the environment variable, e.g.:
>
>  - if empty/unset, then include "DWIM" suggestions.
>  - if set to 'config', then query the 'completion.checkoutNoGuess'
>configuration variable, and omit "DWIM" suggestions if its true.
>  - if set to something else, then omit "DWIM" suggestions.

> Then users can themselves decide, whether the per-repo configurability
> is worth the overhead of running 'git config'.

I would _definitely_ appreciate this feature. Firstly, thank you for
the addition of the environment variable. It is indeed much better
than the --no-guess flag.

However, I'm in a situation where I very much prefer the DWIM behavior
for nearly all of my repos. However, a very few repos have LOTS of
branches. And I only wish to disable DWIM in those few repos.

I attempted to create an alias (`git config alias.co 'checkout
--no-guess'`) in those specific repos. However, that turned out to be
foolish since I believe the alias parsing doesn't occur until _after_
the shell completion script runs (thus the --no-guess is not actually
present in the command parsed by the completion script).

So I'm back to very much wanting the ability to disable DWIM
repo-specific via git-config; and am willing to pay the git-config tax
as necessary.

Re: Bug: Cannot kill Nodejs process using ctrl + c

2017-06-29 Thread Johannes Schindelin

Hi,

On Wed, 28 Jun 2017, Johannes Schindelin wrote:

> On Mon, 26 Jun 2017, Gyandeep Singh wrote:
> 
> [... a bug report ...]

This bug report was reposted as

https://github.com/git-for-windows/git/issues/1219

In my opinion, it is too Windows-specific for the discussion to continue
anywhere but in that ticket.

Ciao,
Johannes

Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes

On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Looking at the implementation of get_ws_cleaned_string() that is the
>> workhorse of emitted_symbol_cmp_no_ws(), it seems to be doing wrong
>> things for various "ignore whitespace" options (i.e. there is only
>> one implementation, while "git diff" family takes things like
>> "ignore space change", "ignore all whitespace", etc.), though.
>
> This probably deserves a bit more illustration of how I envision the
> code should evolve.
>
> In the longer term, I would prefer to see emitted_symbol_cmp_no_ws()
> to go and instead emitted_symbol_cmp() to take the diff options so
> that it can change the behaviour of the comparison function based on
> the -w/-b/--ignore-space-at-eol/etc. settings.  And compare two strings
> in place.
>
> For that, you may need a helper function that takes a pointer to a
> character pointer, picks the next byte that matters while advancing
> the pointer, and returns that byte.  The emitted_symbol_cmp(a, b)
> which is not used for real comparison (i.e. ordering to see if a
> sorts earlier than b) but for equivalence (i.e. considering various
> whitespace-ignoring settings, does a and b matfch?) may become
> something like:
>
> int
> emitted_symbol_eqv(struct emitted_diff_symbol *a,
>struct emitted_diff_symbol *b,
>const void *keydata) {
> struct diff_options *diffopt = keydata;
> const char *ap = a->line;
> const char *bp = b->line;
>
> while (1) {
> int ca, cb;
> ca = next_byte(&ap, diffopt);
> cb = next_byte(&bp, diffopt);
> if (ca != cb)
> return 1; /* differs */
> if (!ca)
> return 0;
> };
> }
>
> where next_byte() may look like:
>
> static int
> next_byte(const char **cp, struct diff_options *diffopt)
> {
> int retval;
>
> again:
> retval = **cp;
> if (!retval)
> return retval; /* end of string */
> if (!isspace(retval)) {
> (*cp)++; /* advance */
> return retval;
> }
>
> switch (ignore_space_type(diffopt)) {
> case NOT_IGNORING:
> (*cp)++; /* advance */
> return retval;
> case IGNORE_SPACE_CHANGE:
> while (**cp && isspace(**cp))
> (*cp)++; /* squash consecutive spaces */
> return ' '; /* normalize spaces with a SP */
> case IGNORE_ALL_SPACE:
> (*cp)++; /* advance */
> goto again;
> ... other cases here ...
> }
> }
>
>

I just rebased the diff series on top of the hashmap series and now
I want to implement the compare function based on the new data
feature. So I thought I might start off this proposal, as you seem to
have good taste for how to approach problems.

It turns out that the code here is rather a very loose proposal,
not to be copied literally, because of these constraints:
* When dealing with user content, we do not have C-strings,
  but memory + length, such that next_byte also needs to be
  aware of the end pointer.
* The ignore_space_type() as well as these constants do not exist
  as-is, I think the cleanest for now is to parse diffopt->xdl_opts via
  DIFF_XDL_TST

Thanks,
Stefan

Re: [PATCH 2/2] hashmap: migrate documentation from Documentation/technical into header

2017-06-29 Thread Jonathan Nieder

Stefan Beller wrote:

> Subject: hashmap: migrate documentation from Documentation/technical into 
> header
>
> While at it, clarify the use of `key`, `keydata`, `entry_or_key` as well
> as documenting the new data pointer for the compare function.
>
> Signed-off-by: Stefan Beller 
> ---
>  Documentation/technical/api-hashmap.txt | 309 
> 
>  hashmap.h   | 249 +++--
>  2 files changed, 231 insertions(+), 327 deletions(-)
>  delete mode 100644 Documentation/technical/api-hashmap.txt

Yay, I love this.

[...]
> --- a/Documentation/technical/api-hashmap.txt
> +++ /dev/null
> @@ -1,309 +0,0 @@

Verified that these docs have all been migrated to the header, except
where noted below.

[...]
> -`int (*hashmap_cmp_fn)(const void *entry, const void *entry_or_key, const 
> void *keydata)`::
> -
> - User-supplied function to test two hashmap entries for equality. Shall
> - return 0 if the entries are equal.
> -+
> -This function is always called with non-NULL `entry` / `entry_or_key`
> -parameters that have the same hash code. When looking up an entry, the `key`
> -and `keydata` parameters to hashmap_get and hashmap_remove are always passed
> -as second and third argument, respectively. Otherwise, `keydata` is NULL.

This was really heard to read in the preimage.  Thanks for cleaning it up.

[...]
> -Usage example
> --
> -
> -Here's a simple usage example that maps long keys to double values.

What happened to this?  I think it would be useful to include in a
long comment at the top of the header.  A worked example like this
makes it easier to get one's bearings and know which functions to look
at.

[...]
> -Using variable-sized keys
> --

Same question: should this discussion go in the hashmap_get()
docstring, with a pointer from hashmap_remove()?  Or should it go in
test-hashmap.c, with a pointer in the docstrings of both?

[...]
> +++ b/hashmap.h
> @@ -3,11 +3,18 @@
>  
>  /*
>   * Generic implementation of hash-based key-value mappings.
> - * See Documentation/technical/api-hashmap.txt.
> + *
>   */

nit: why the blank line?

>  
> -/* FNV-1 functions */
> -
> +/*
> + * Ready-to-use hash functions for strings, using the FNV-1 algorithm (see
> + * http://www.isthe.com/chongo/tech/comp/fnv).
> + * `strhash` and `strihash` take 0-terminated strings, while `memhash` and
> + * `memihash` operate on arbitrary-length memory.
> + * `strihash` and `memihash` are case insensitive versions.
> + * `memihash_cont` is a variant of `memihash` that allows a computation to be
> + * continued with another chunk of data.
> + */
>  extern unsigned int strhash(const char *buf);
>  extern unsigned int strihash(const char *buf);
>  extern unsigned int memhash(const void *buf, size_t len);
> @@ -16,6 +23,15 @@ extern unsigned int memihash_cont(unsigned int hash_seed, 
> const void *buf, size_
>  
>  static inline unsigned int sha1hash(const unsigned char *sha1)
>  {
> + /*
> +  * Converts a cryptographic hash (e.g. SHA-1) into an int-sized hash 
> code
> +  * for use in hash tables. Cryptographic hashes are supposed to have
> +  * uniform distribution, so in contrast to `memhash()`, this just copies
> +  * the first `sizeof(int)` bytes without shuffling any bits. Note that
> +  * the results will be different on big-endian and little-endian
> +  * platforms, so they should not be stored or transferred over the net.
> +  */

This comment should go in front of the function instead of in its body, like 
this:

/*
 * Converts a crypto [...]
 */
static inline unsigned int sha1hash(...

because it is describing the purpose and usage of the function and not
its implementation.

[...]
> +/*
> + * User-supplied function to test two hashmap entries for equality. Shall
> + * return 0 if the entries are equal.
> + *
> + * This function is always called with non-NULL `entry` and `entry_or_key`
> + * parameters that have the same hash code.
> + *
> + * When looking up an entry, the `key` and `keydata` parameters to 
> hashmap_get
> + * and hashmap_remove are always passed
> + * as second `entry_or_key` and third argument `keydata`, respectively.
> + * Otherwise, `keydata` is NULL.

strange wrapping

> + *
> + * There are two modes for this function to be used:
> + * (a) When looking for an exact match of a given `entry`, then `keydata`
> + * ought to be NULL, and this function should cast `entry` and
> + * `entry_or_key` to the user entries and check for equality.

What does it mean that `keydata` ought to be NULL?  Are you saying it
will be NULL, or that someone has to ensure it's NULL, or something
else?

> + * (b) When it is too expensive to allocate such a user entry (either because
> + * it is large or varialbe sized, such that it is not on the stack),
> + * Then the relevant data to check for equality should be passed via
> + * ` keydata`.

[PATCH] merge-recursive: use DIFF_XDL_SET macro

Instead of implementing this on our own, just use a convenience macro.

Signed-off-by: Stefan Beller 
---
 merge-recursive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 59e5ee41a8..1494ffdb82 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2210,11 +2210,11 @@ int parse_merge_opt(struct merge_options *o, const char 
*s)
o->xdl_opts |= value;
}
else if (!strcmp(s, "ignore-space-change"))
-   o->xdl_opts |= XDF_IGNORE_WHITESPACE_CHANGE;
+   DIFF_XDL_SET(o, IGNORE_WHITESPACE_CHANGE);
else if (!strcmp(s, "ignore-all-space"))
-   o->xdl_opts |= XDF_IGNORE_WHITESPACE;
+   DIFF_XDL_SET(o, IGNORE_WHITESPACE);
else if (!strcmp(s, "ignore-space-at-eol"))
-   o->xdl_opts |= XDF_IGNORE_WHITESPACE_AT_EOL;
+   DIFF_XDL_SET(o, IGNORE_WHITESPACE_AT_EOL);
else if (!strcmp(s, "renormalize"))
o->renormalize = 1;
else if (!strcmp(s, "no-renormalize"))
-- 
2.13.0.31.g9b732c453e

[PATCH v2 0/6] grep: remove redundant code & reflags from API

Addresses comments from Stefan Beller (thanks!). I looked into it and
the REG_NEWLINE flag was redundant in 1/2 cases, see 6/6 for the
removal of that.

I looked into refactoring 5/6 as noted in 87zicqirrg@gmail.com,
but for the reasons now explained in the last paragraph of 5/6 decided
not to and to keep it as it was.

Ævar Arnfjörð Bjarmason (6):
  grep: remove redundant double assignment to 0
  grep: adjust a redundant grep pattern type assignment
  grep: remove redundant "fixed" field re-assignment to 0
  grep: remove redundant and verbose re-assignments to 0
  grep: remove regflags from the public grep_opt API
  grep: remove redundant REG_NEWLINE when compiling fixed regex

 builtin/grep.c |  2 --
 grep.c | 62 +-
 grep.h |  1 -
 revision.c |  2 --
 4 files changed, 35 insertions(+), 32 deletions(-)

-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 3/6] grep: remove redundant "fixed" field re-assignment to 0

Remove the redundant re-assignment of the fixed field to zero right
after the entire struct has been set to zero via memset(...).

Unlike some nearby commits this pattern doesn't date back to the
pattern described in e0b9f8ae09 ("grep: remove redundant regflags
assignments", 2017-05-25), instead it was apparently cargo-culted in
9eceddeec6 ("Use kwset in grep", 2011-08-21).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 grep.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/grep.c b/grep.c
index 817270d081..86dc9b696f 100644
--- a/grep.c
+++ b/grep.c
@@ -626,8 +626,6 @@ static void compile_regexp(struct grep_pat *p, struct 
grep_opt *opt)
has_null(p->pattern, p->patternlen) ||
is_fixed(p->pattern, p->patternlen))
p->fixed = !icase || ascii_only;
-   else
-   p->fixed = 0;
 
if (p->fixed) {
p->kws = kwsalloc(icase ? tolower_trans_tbl : NULL);
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 2/6] grep: adjust a redundant grep pattern type assignment

Adjust a now-redundant assignment to extended_regexp_option to make it
zero if grep.extendedRegexp is not set. This is always called right
after init_grep_defaults() which memsets the entire structure to 0, so
there's no need to set it again to zero.

However the reason for the if/else pattern is a holdover from[1] where
this was adjusted from a bitfield assignment to a boolean. Rather than
getting rid of the assignment to 0 in all cases, let's just use the
value returned by git_config_bool(), which is more idiomatic and in
sync with the rest of the boolean handling in this function.

This is a logical follow-up to my commit to remove redundant regflags
assignments[2]. This logic was originally introduced in [3], but as
explained in the former commit it's working around a pattern in our
code that no longer exists, and is now confusing as it leads the
reader to think that this needs to be flipped back & forth.

1. 84befcd0a4 ("grep: add a grep.patternType configuration setting",
   2012-08-03)
2. e0b9f8ae09 ("grep: remove redundant regflags assignments",
   2017-05-25)
3. b22520a37c ("grep: allow -E and -n to be turned on by default via
   configuration", 2011-03-30)

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 grep.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/grep.c b/grep.c
index 29439886e7..817270d081 100644
--- a/grep.c
+++ b/grep.c
@@ -78,10 +78,7 @@ int grep_config(const char *var, const char *value, void *cb)
return -1;
 
if (!strcmp(var, "grep.extendedregexp")) {
-   if (git_config_bool(var, value))
-   opt->extended_regexp_option = 1;
-   else
-   opt->extended_regexp_option = 0;
+   opt->extended_regexp_option = git_config_bool(var, value);
return 0;
}
 
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 1/6] grep: remove redundant double assignment to 0

Stop assigning 0 to the extended_regexp_option field right after we've
zeroed out the entire struct with memset() just a few lines earlier.

Unlike some of the code being refactored in subsequent commits, this
was always completely redundant. See the original code introduced in
84befcd0a4 ("grep: add a grep.patternType configuration setting",
2012-08-03).

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 grep.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/grep.c b/grep.c
index 98733db623..29439886e7 100644
--- a/grep.c
+++ b/grep.c
@@ -38,7 +38,6 @@ void init_grep_defaults(void)
opt->regflags = REG_NEWLINE;
opt->max_depth = -1;
opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
-   opt->extended_regexp_option = 0;
color_set(opt->color_context, "");
color_set(opt->color_filename, "");
color_set(opt->color_function, "");
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 4/6] grep: remove redundant and verbose re-assignments to 0

Remove the redundant re-assignments of the fixed/pcre1/pcre2 fields to
zero right after the entire struct has been set to zero via
memset(...).

See an earlier related cleanup commit e0b9f8ae09 ("grep: remove
redundant regflags assignments", 2017-05-25) for an explanation of why
the code was structured like this to begin with.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 grep.c | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/grep.c b/grep.c
index 86dc9b696f..7fcdaa0753 100644
--- a/grep.c
+++ b/grep.c
@@ -174,28 +174,18 @@ static void grep_set_pattern_type_option(enum 
grep_pattern_type pattern_type, st
/* fall through */
 
case GREP_PATTERN_TYPE_BRE:
-   opt->fixed = 0;
-   opt->pcre1 = 0;
-   opt->pcre2 = 0;
break;
 
case GREP_PATTERN_TYPE_ERE:
-   opt->fixed = 0;
-   opt->pcre1 = 0;
-   opt->pcre2 = 0;
opt->regflags |= REG_EXTENDED;
break;
 
case GREP_PATTERN_TYPE_FIXED:
opt->fixed = 1;
-   opt->pcre1 = 0;
-   opt->pcre2 = 0;
break;
 
case GREP_PATTERN_TYPE_PCRE:
-   opt->fixed = 0;
 #ifdef USE_LIBPCRE2
-   opt->pcre1 = 0;
opt->pcre2 = 1;
 #else
/*
@@ -205,7 +195,6 @@ static void grep_set_pattern_type_option(enum 
grep_pattern_type pattern_type, st
 * "cannot use Perl-compatible regexes[...]".
 */
opt->pcre1 = 1;
-   opt->pcre2 = 0;
 #endif
break;
}
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 6/6] grep: remove redundant REG_NEWLINE when compiling fixed regex

Remove the redundant REG_NEWLINE regcomp() flag from the code that
compiles a fixed-string regular-expression.

The REG_NEWLINE causes metacharacters such as "." to match a newline,
since the basic_regex_quote_buf() function being called here escapes
all metacharacters using REG_NEWLINE is confusing and redundant.

The use of this flag was introduced as an unintended emergent property
of 793dc676e0 ("grep/icase: avoid kwsset when -F is specified",
2016-06-25).

That change amended the existing regflags, which were initialized to
REG_NEWLINE in init_grep_defaults() assuming a subsequent non-fixed
regcomp().

Manual testing reveals that this was always redundant, since no flags
of any use were inherited from opt->regflags even back
then. 793dc676e0 passes all tests with this on top:

diff --git a/grep.c b/grep.c
index 627ae3e3e8..89e84ed7fd 100644
--- a/grep.c
+++ b/grep.c
@@ -407,3 +407,3 @@ static void compile_fixed_regexp(struct grep_pat *p, 
struct grep_opt *opt)
basic_regex_quote_buf(&sb, p->pattern);
-   regflags = opt->regflags & ~REG_EXTENDED;
+   regflags = 0;
if (opt->ignore_case)

Since this isn't used for anything and never was, remove it to reduce
confusion when reading this code.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grep.c b/grep.c
index 11a86548d6..2efec0e182 100644
--- a/grep.c
+++ b/grep.c
@@ -593,7 +593,7 @@ static void compile_fixed_regexp(struct grep_pat *p, struct 
grep_opt *opt)
 {
struct strbuf sb = STRBUF_INIT;
int err;
-   int regflags = REG_NEWLINE;
+   int regflags = 0;
 
basic_regex_quote_buf(&sb, p->pattern);
if (opt->ignore_case)
-- 
2.13.1.611.g7e3b11ae1

[PATCH v2 5/6] grep: remove regflags from the public grep_opt API

Refactor calls to the grep machinery to always pass opt.ignore_case &
opt.extended_regexp_option instead of setting the equivalent regflags
bits.

The bug fixed when making -i work with -P in commit 9e3cbc59d5 ("log:
make --regexp-ignore-case work with --perl-regexp", 2017-05-20) was
really just plastering over the code smell which this change fixes.

The reason for adding the extensive commentary here is that I
discovered some subtle complexity in implementing this that really
should be called out explicitly to future readers.

Before this change we'd rely on the difference between
`extended_regexp_option` and `regflags` to serve as a membrane between
our preliminary parsing of grep.extendedRegexp and grep.patternType,
and what we decided to do internally.

Now that those two are the same thing, it's necessary to unset
`extended_regexp_option` just before we commit in cases where both of
those config variables are set. See 84befcd0a4 ("grep: add a
grep.patternType configuration setting", 2012-08-03) for the code and
documentation related to that.

The explanation of why the if/else branches in
grep_commit_pattern_type() are ordered the way they are exists in that
commit message, but I think it's worth calling this subtlety out
explicitly with a comment for future readers.

Even though grep_commit_pattern_type() is the only caller of
grep_set_pattern_type_option() it's simpler to reset the
extended_regexp_option flag in the latter, since 2/3 branches in the
former would otherwise need to reset it, this way we can do it in one
place.

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
 builtin/grep.c |  2 --
 grep.c | 43 ++-
 grep.h |  1 -
 revision.c |  2 --
 4 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index f61a9d938b..b682966439 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -1169,8 +1169,6 @@ int cmd_grep(int argc, const char **argv, const char 
*prefix)
 
if (!opt.pattern_list)
die(_("no pattern given."));
-   if (!opt.fixed && opt.ignore_case)
-   opt.regflags |= REG_ICASE;
 
/*
 * We have to find "--" in a separate pass, because its presence
diff --git a/grep.c b/grep.c
index 7fcdaa0753..11a86548d6 100644
--- a/grep.c
+++ b/grep.c
@@ -35,7 +35,6 @@ void init_grep_defaults(void)
memset(opt, 0, sizeof(*opt));
opt->relative = 1;
opt->pathname = 1;
-   opt->regflags = REG_NEWLINE;
opt->max_depth = -1;
opt->pattern_type_option = GREP_PATTERN_TYPE_UNSPECIFIED;
color_set(opt->color_context, "");
@@ -153,7 +152,6 @@ void grep_init(struct grep_opt *opt, const char *prefix)
opt->linenum = def->linenum;
opt->max_depth = def->max_depth;
opt->pathname = def->pathname;
-   opt->regflags = def->regflags;
opt->relative = def->relative;
opt->output = def->output;
 
@@ -169,6 +167,24 @@ void grep_init(struct grep_opt *opt, const char *prefix)
 
 static void grep_set_pattern_type_option(enum grep_pattern_type pattern_type, 
struct grep_opt *opt)
 {
+   /*
+* When committing to the pattern type by setting the relevant
+* fields in grep_opt it's generally not necessary to zero out
+* the fields we're not choosing, since they won't have been
+* set by anything. The extended_regexp_option field is the
+* only exception to this.
+*
+* This is because in the process of parsing grep.patternType
+* & grep.extendedRegexp we set opt->pattern_type_option and
+* opt->extended_regexp_option, respectively. We then
+* internally use opt->extended_regexp_option to see if we're
+* compiling an ERE. It must be unset if that's not actually
+* the case.
+*/
+   if (pattern_type != GREP_PATTERN_TYPE_ERE &&
+   opt->extended_regexp_option)
+   opt->extended_regexp_option = 0;
+
switch (pattern_type) {
case GREP_PATTERN_TYPE_UNSPECIFIED:
/* fall through */
@@ -177,7 +193,7 @@ static void grep_set_pattern_type_option(enum 
grep_pattern_type pattern_type, st
break;
 
case GREP_PATTERN_TYPE_ERE:
-   opt->regflags |= REG_EXTENDED;
+   opt->extended_regexp_option = 1;
break;
 
case GREP_PATTERN_TYPE_FIXED:
@@ -207,6 +223,11 @@ void grep_commit_pattern_type(enum grep_pattern_type 
pattern_type, struct grep_o
else if (opt->pattern_type_option != GREP_PATTERN_TYPE_UNSPECIFIED)
grep_set_pattern_type_option(opt->pattern_type_option, opt);
else if (opt->extended_regexp_option)
+   /*
+* This branch *must* happen after setting from the
+* opt->pattern_type_option above, we don't want
+* grep.extendedRegexp to override grep.patternType!
+*/
gr

Re: [PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

2017-06-29 Thread miguel torroja

On Thu, Jun 29, 2017 at 8:59 AM, Luke Diamand  wrote:
> On 28 June 2017 at 14:14, miguel torroja  wrote:
>> Thanks Luke,
>>
>> regarding the error in t9800 (not ok 18 - unresolvable host in P4PORT
>> should display error), for me it's very weird too as it doesn't seem
>> to be related to this particular change, as the patch changes are not
>> exercised with that test.
>
> I had a look at this. The problem is that the old code uses
> p4_read_pipe_lines() which calls sys.exit() if the subprocess fails.
>
> But the new code calls p4CmdList() which has does error handling by
> setting "p4ExitCode" to a non-zero value in the returned dictionary.
>
> I think if you just check for that case, the test will then pass

Thank you for debugging this,  I did as you suggested and it passed that test!

>>
>> The test 21 in t9807 was precisely the new test added to test the
>> change (it was passing with local setup), the test log is truncated
>> before the output of test 21 in t9807 but I'm afraid I'm not very
>> familiar with Travis, so maybe I'm missing something. Is there a way
>> to have the full logs or they are always truncated after some number
>> of lines?
>
> For me, t9807 is working fine.
>
>>
>> I think you get an error with git diff --check because I added spaces
>> after a tab, but those spaces are intentional, the tabs are for the
>> "<<-EOF" and spaces are for the "p4 triggers" specificiation.
>
> OK.
>

In the end, ,the reason t9807 was not passing was precisely the tabs
and spaces of the patch. the original patch had:
., as I explained, the tabs were supposed to be
ignored by "<<-EOF" and the spaces were supposed to be sent to stdin
of p4 triggers, but when the patch was applied to upstream the
 were substituted by tabs what led to a malformed  "p4
trigger" description. I just collapsed the description in one single
line and now it's passing
>
> Luke


I'm sending a new patch with the two changes I just mentioned.

Thanks,

[PATCH] git-p4: parse marshal output "p4 -G" in p4 changes

2017-06-29 Thread Miguel Torroja

The option -G of p4 (python marshal output) gives more context about the
data being output. That's useful when using the command "change -o" as
we can distinguish between warning/error line and real change description.

Some p4 triggers in the server side generate some warnings when
executed. Unfortunately those messages are mixed with the output of
"p4 change -o". Those extra warning lines are reported as {'code':'info'}
in python marshal output (-G). The real change output is reported as
{'code':'stat'}

A new test has been created to t9807-git-p4-submit.sh adding a p4 trigger
that outputs extra lines with "p4 change -o" and "p4 changes"

Signed-off-by: Miguel Torroja 
Signed-off-by: Junio C Hamano 
---
 git-p4.py| 85 
 t/t9807-git-p4-submit.sh | 27 +++
 2 files changed, 84 insertions(+), 28 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 8d151da91..61dc045f3 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -879,8 +879,12 @@ def p4ChangesForPaths(depotPaths, changeRange, 
requestedBlockSize):
 cmd += ["%s...@%s" % (p, revisionRange)]
 
 # Insert changes in chronological order
-for line in reversed(p4_read_pipe_lines(cmd)):
-changes.add(int(line.split(" ")[1]))
+for entry in reversed(p4CmdList(cmd)):
+if entry.has_key('p4ExitCode'):
+die('Error retrieving changes descriptions 
({})'.format(entry['p4ExitCode']))
+if not entry.has_key('change'):
+continue
+changes.add(int(entry['change']))
 
 if not block_size:
 break
@@ -1526,37 +1530,62 @@ class P4Submit(Command, P4UserMap):
 
 [upstream, settings] = findUpstreamBranchPoint()
 
-template = ""
+template = """\
+# A Perforce Change Specification.
+#
+#  Change:  The change number. 'new' on a new changelist.
+#  Date:The date this specification was last modified.
+#  Client:  The client on which the changelist was created.  Read-only.
+#  User:The user who created the changelist.
+#  Status:  Either 'pending' or 'submitted'. Read-only.
+#  Type:Either 'public' or 'restricted'. Default is 'public'.
+#  Description: Comments about the changelist.  Required.
+#  Jobs:What opened jobs are to be closed by this changelist.
+#   You may delete jobs from this list.  (New changelists only.)
+#  Files:   What opened files from the default changelist are to be added
+#   to this changelist.  You may delete files from this list.
+#   (New changelists only.)
+"""
+files_list = []
 inFilesSection = False
+change_entry = None
 args = ['change', '-o']
 if changelist:
 args.append(str(changelist))
-
-for line in p4_read_pipe_lines(args):
-if line.endswith("\r\n"):
-line = line[:-2] + "\n"
-if inFilesSection:
-if line.startswith("\t"):
-# path starts and ends with a tab
-path = line[1:]
-lastTab = path.rfind("\t")
-if lastTab != -1:
-path = path[:lastTab]
-if settings.has_key('depot-paths'):
-if not [p for p in settings['depot-paths']
-if p4PathStartsWith(path, p)]:
-continue
-else:
-if not p4PathStartsWith(path, self.depotPath):
-continue
+for entry in p4CmdList(args):
+if not entry.has_key('code'):
+continue
+if entry['code'] == 'stat':
+change_entry = entry
+break
+if not change_entry:
+die('Failed to decode output of p4 change -o')
+for key, value in change_entry.iteritems():
+if key.startswith('File'):
+if settings.has_key('depot-paths'):
+if not [p for p in settings['depot-paths']
+if p4PathStartsWith(value, p)]:
+continue
 else:
-inFilesSection = False
-else:
-if line.startswith("Files:"):
-inFilesSection = True
-
-template += line
-
+if not p4PathStartsWith(value, self.depotPath):
+continue
+files_list.append(value)
+continue
+# Output in the order expected by prepareLogMessage
+for key in ['Change','Client','User','Status','Description','Jobs']:
+if not change_entry.has_key(key):
+continue
+template += '\n'
+template += key + ':'
+if key == 'Description':
+template += '\

Fatura n.6760

2017-06-29 Thread Murat Başaran

İyi Günler
Tamamlanmış işler için rapor ve faturanız ekte gönderilmiştir. Lütfen ödemenizi 
geciktirmeyiniz.

En iyi dileklerimizle

Murat Başaran, HAVELSAN A.Ş.
TEL : 0312 219 57 97 (35 hat Pbx)
FAKS : 0312 219 57 87





pdf_fatura.rar
Description: Binary data

Re: [PATCH 2/2] hashmap: migrate documentation from Documentation/technical into header

Took all suggestions so far, but the last one:

>
> Probably worth mentioning this is a convenience wrapper for iter_init
> + iter_next, like the Documentation/technical/ text did.

No. The code to observe that this is a convenience wrapper
IS RIGHT THERE (it's a header file), and I find this header a
bit bloated already. So I'll keep it as a one liner.

Re: [PATCH 6/6] diff.c: detect blocks despite whitespace changes

On Thu, Jun 29, 2017 at 2:01 PM, Stefan Beller  wrote:
> On Tue, Jun 27, 2017 at 10:06 PM, Junio C Hamano  wrote:
>> Junio C Hamano  writes:
>> This probably deserves a bit more illustration of how I envision the
>> code should evolve.
>> ...
> It turns out that the code here is rather a very loose proposal,
> not to be copied literally,

Yeah, whenever I say "illustration", do not take it as anything more than
scribbling on the back of an envelope, whose primary purpose is to show
some idea (in this case, the central idea is that a helper function that lets
you ask "what's the next byte that matters? tell me also when you run out"
is all you need to do an in-place comparison with various "ignore" modes;
by the way, that applies also to ignore-case comparison if there is a need
to support it).

The implementation detail to support that central idea is left
to the readers. ;-)

Thanks.

[PATCHv2 2/2] hashmap: migrate documentation from Documentation/technical into header

While at it, clarify the use of `key`, `keydata`, `entry_or_key` as well
as documenting the new data pointer for the compare function.

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-hashmap.txt | 309 
 hashmap.h   | 351 +---
 2 files changed, 318 insertions(+), 342 deletions(-)
 delete mode 100644 Documentation/technical/api-hashmap.txt

diff --git a/Documentation/technical/api-hashmap.txt 
b/Documentation/technical/api-hashmap.txt
deleted file mode 100644
index ccc634bbd7..00
--- a/Documentation/technical/api-hashmap.txt
+++ /dev/null
@@ -1,309 +0,0 @@
-hashmap API
-===
-
-The hashmap API is a generic implementation of hash-based key-value mappings.
-
-Data Structures

-
-`struct hashmap`::
-
-   The hash table structure. Members can be used as follows, but should
-   not be modified directly:
-+
-The `size` member keeps track of the total number of entries (0 means the
-hashmap is empty).
-+
-`tablesize` is the allocated size of the hash table. A non-0 value indicates
-that the hashmap is initialized. It may also be useful for statistical purposes
-(i.e. `size / tablesize` is the current load factor).
-+
-`cmpfn` stores the comparison function specified in `hashmap_init()`. In
-advanced scenarios, it may be useful to change this, e.g. to switch between
-case-sensitive and case-insensitive lookup.
-+
-When `disallow_rehash` is set, automatic rehashes are prevented during inserts
-and deletes.
-
-`struct hashmap_entry`::
-
-   An opaque structure representing an entry in the hash table, which must
-   be used as first member of user data structures. Ideally it should be
-   followed by an int-sized member to prevent unused memory on 64-bit
-   systems due to alignment.
-+
-The `hash` member is the entry's hash code and the `next` member points to the
-next entry in case of collisions (i.e. if multiple entries map to the same
-bucket).
-
-`struct hashmap_iter`::
-
-   An iterator structure, to be used with hashmap_iter_* functions.
-
-Types
--
-
-`int (*hashmap_cmp_fn)(const void *entry, const void *entry_or_key, const void 
*keydata)`::
-
-   User-supplied function to test two hashmap entries for equality. Shall
-   return 0 if the entries are equal.
-+
-This function is always called with non-NULL `entry` / `entry_or_key`
-parameters that have the same hash code. When looking up an entry, the `key`
-and `keydata` parameters to hashmap_get and hashmap_remove are always passed
-as second and third argument, respectively. Otherwise, `keydata` is NULL.
-
-Functions
--
-
-`unsigned int strhash(const char *buf)`::
-`unsigned int strihash(const char *buf)`::
-`unsigned int memhash(const void *buf, size_t len)`::
-`unsigned int memihash(const void *buf, size_t len)`::
-`unsigned int memihash_cont(unsigned int hash_seed, const void *buf, size_t 
len)`::
-
-   Ready-to-use hash functions for strings, using the FNV-1 algorithm (see
-   http://www.isthe.com/chongo/tech/comp/fnv).
-+
-`strhash` and `strihash` take 0-terminated strings, while `memhash` and
-`memihash` operate on arbitrary-length memory.
-+
-`strihash` and `memihash` are case insensitive versions.
-+
-`memihash_cont` is a variant of `memihash` that allows a computation to be
-continued with another chunk of data.
-
-`unsigned int sha1hash(const unsigned char *sha1)`::
-
-   Converts a cryptographic hash (e.g. SHA-1) into an int-sized hash code
-   for use in hash tables. Cryptographic hashes are supposed to have
-   uniform distribution, so in contrast to `memhash()`, this just copies
-   the first `sizeof(int)` bytes without shuffling any bits. Note that
-   the results will be different on big-endian and little-endian
-   platforms, so they should not be stored or transferred over the net.
-
-`void hashmap_init(struct hashmap *map, hashmap_cmp_fn equals_function, size_t 
initial_size)`::
-
-   Initializes a hashmap structure.
-+
-`map` is the hashmap to initialize.
-+
-The `equals_function` can be specified to compare two entries for equality.
-If NULL, entries are considered equal if their hash codes are equal.
-+
-If the total number of entries is known in advance, the `initial_size`
-parameter may be used to preallocate a sufficiently large table and thus
-prevent expensive resizing. If 0, the table is dynamically resized.
-
-`void hashmap_free(struct hashmap *map, int free_entries)`::
-
-   Frees a hashmap structure and allocated memory.
-+
-`map` is the hashmap to free.
-+
-If `free_entries` is true, each hashmap_entry in the map is freed as well
-(using stdlib's free()).
-
-`void hashmap_entry_init(void *entry, unsigned int hash)`::
-
-   Initializes a hashmap_entry structure.
-+
-`entry` points to the entry to initialize.
-+
-`hash` is the hash code of the entry.
-+
-The hashmap_entry structure does not hold references to e

[PATCHv2 1/2] hashmap.h: compare function has access to a data field

When using the hashmap a common need is to have access to arbitrary data
in the compare function. A couple of times we abuse the keydata field
to pass in the data needed. This happens for example in patch-ids.c.

This patch changes the function signature of the compare function
to have one more void pointer available. The pointer given for each
invocation of the compare function must be defined in the init function
of the hashmap and is just passed through.

Documentation of this new feature is deferred to a later patch.

While at it improve the naming of the fields of all compare functions used
by hashmaps by ensuring unused parameters are prefixed with 'unused_' and
naming the parameters what they are (instead of 'unused' make it
'unused_keydata').

Signed-off-by: Stefan Beller 
---
 attr.c  |  4 ++--
 builtin/describe.c  |  6 --
 builtin/difftool.c  | 20 
 builtin/fast-export.c   |  5 +++--
 config.c|  7 +--
 convert.c   |  3 ++-
 diffcore-rename.c   |  2 +-
 hashmap.c   | 17 -
 hashmap.h   |  9 ++---
 name-hash.c | 12 
 oidset.c|  5 +++--
 patch-ids.c |  6 --
 refs.c  |  4 ++--
 remote.c|  7 +--
 sha1_file.c |  5 +++--
 sub-process.c   |  5 +++--
 sub-process.h   |  6 --
 submodule-config.c  | 10 ++
 t/helper/test-hashmap.c | 15 ++-
 19 files changed, 95 insertions(+), 53 deletions(-)

diff --git a/attr.c b/attr.c
index 37454999d2..2f51417675 100644
--- a/attr.c
+++ b/attr.c
@@ -78,7 +78,7 @@ struct attr_hash_entry {
 /* attr_hashmap comparison function */
 static int attr_hash_entry_cmp(const struct attr_hash_entry *a,
   const struct attr_hash_entry *b,
-  void *unused)
+  void *unused_keydata, void *unused_data)
 {
return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
 }
@@ -86,7 +86,7 @@ static int attr_hash_entry_cmp(const struct attr_hash_entry 
*a,
 /* Initialize an 'attr_hashmap' object */
 static void attr_hashmap_init(struct attr_hashmap *map)
 {
-   hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
+   hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, NULL, 0);
 }
 
 /*
diff --git a/builtin/describe.c b/builtin/describe.c
index 70eb144608..a6c5a969a0 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -55,7 +55,9 @@ static const char *prio_names[] = {
 };
 
 static int commit_name_cmp(const struct commit_name *cn1,
-   const struct commit_name *cn2, const void *peeled)
+  const struct commit_name *cn2,
+  const void *peeled,
+  const void *unused_data)
 {
return oidcmp(&cn1->peeled, peeled ? peeled : &cn2->peeled);
 }
@@ -501,7 +503,7 @@ int cmd_describe(int argc, const char **argv, const char 
*prefix)
return cmd_name_rev(args.argc, args.argv, prefix);
}
 
-   hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, 0);
+   hashmap_init(&names, (hashmap_cmp_fn) commit_name_cmp, NULL, 0);
for_each_rawref(get_name, NULL);
if (!names.size && !always)
die(_("No names found, cannot describe anything."));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 9199227f6e..80786a95ab 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -131,7 +131,9 @@ struct working_tree_entry {
 };
 
 static int working_tree_entry_cmp(struct working_tree_entry *a,
- struct working_tree_entry *b, void *keydata)
+ struct working_tree_entry *b,
+ void *unused_keydata,
+ void *unused_data)
 {
return strcmp(a->path, b->path);
 }
@@ -146,7 +148,8 @@ struct pair_entry {
const char path[FLEX_ARRAY];
 };
 
-static int pair_cmp(struct pair_entry *a, struct pair_entry *b, void *keydata)
+static int pair_cmp(struct pair_entry *a, struct pair_entry *b,
+   void *unused_keydata, void *unused_data)
 {
return strcmp(a->path, b->path);
 }
@@ -174,7 +177,8 @@ struct path_entry {
char path[FLEX_ARRAY];
 };
 
-static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void 
*key)
+static int path_entry_cmp(struct path_entry *a, struct path_entry *b,
+ void *key, void *unused_data)
 {
return strcmp(a->path, key ? key : b->path);
 }
@@ -367,9 +371,9 @@ static int run_dir_diff(const char *extcmd, int symlinks, 
const char *prefix,
wtdir_len = wtdir.len;
 
hashmap_init(&working_tree_dups,
-(hashmap_cmp_fn)working_tree_entry_cmp, 0);
-   hashmap_init(&submodules, (hashmap_cmp_fn)pair_cmp, 0);
-

[PATCHv2 0/2] Introduce data field in hashmap and migrate docs to header

v2:

addressed all but the last point of Jonathan Nieder.

Thanks,
Stefan

v1:

https://public-inbox.org/git/xmqqpodnvmmw@gitster.mtv.corp.google.com/
for context why we need a new data field.  Implement that.

Once upon a time we had a long discussion where to put documentation best.
The answer was header files as there documentation has less chance
to become stale and be out of date.  Improve the docs by
* migrating them to the header
* clarifying how the compare function is to be used
* how the arguments to hashmap_get/remove should be used.

Thanks,
Stefan

Stefan Beller (2):
  hashmap.h: compare function has access to a data field
  hashmap: migrate documentation from Documentation/technical into
header

 Documentation/technical/api-hashmap.txt | 309 ---
 attr.c  |   4 +-
 builtin/describe.c  |   6 +-
 builtin/difftool.c  |  20 +-
 builtin/fast-export.c   |   5 +-
 config.c|   7 +-
 convert.c   |   3 +-
 diffcore-rename.c   |   2 +-
 hashmap.c   |  17 +-
 hashmap.h   | 358 
 name-hash.c |  12 +-
 oidset.c|   5 +-
 patch-ids.c |   6 +-
 refs.c  |   4 +-
 remote.c|   7 +-
 sha1_file.c |   5 +-
 sub-process.c   |   5 +-
 sub-process.h   |   6 +-
 submodule-config.c  |  10 +-
 t/helper/test-hashmap.c |  15 +-
 20 files changed, 412 insertions(+), 394 deletions(-)
 delete mode 100644 Documentation/technical/api-hashmap.txt

-- 
2.13.0.31.g9b732c453e

[PATCH 06/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_FRAGINFO

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 75b996c4cf..6e48a129ed 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,7 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_CONTEXT_FRAGINFO,
DIFF_SYMBOL_CONTEXT_MARKER,
DIFF_SYMBOL_SEPARATOR
 };
@@ -570,6 +571,9 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
 {
const char *context, *reset;
switch (s) {
+   case DIFF_SYMBOL_CONTEXT_FRAGINFO:
+   emit_line(o, "", "", line, len);
+   break;
case DIFF_SYMBOL_CONTEXT_MARKER:
context = diff_get_color_opt(o, DIFF_CONTEXT);
reset = diff_get_color_opt(o, DIFF_RESET);
@@ -705,8 +709,8 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
 
strbuf_add(&msgbuf, line + len, org_len - len);
strbuf_complete_line(&msgbuf);
-
-   emit_line(ecbdata->opt, "", "", msgbuf.buf, msgbuf.len);
+   emit_diff_symbol(ecbdata->opt,
+DIFF_SYMBOL_CONTEXT_FRAGINFO, msgbuf.buf, msgbuf.len);
strbuf_release(&msgbuf);
 }
 
-- 
2.13.0.31.g9b732c453e

[PATCH 12/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_HEADER

The header is constructed lazily including line breaks, so just emit
the raw string as is.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 28 
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 49b45fef29..78f7c6f82f 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,7 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_HEADER,
DIFF_SYMBOL_FILEPAIR_PLUS,
DIFF_SYMBOL_FILEPAIR_MINUS,
DIFF_SYMBOL_WORDS_PORCELAIN,
@@ -689,6 +690,9 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
line, reset,
strchr(line, ' ') ? "\t" : "");
break;
+   case DIFF_SYMBOL_HEADER:
+   fprintf(o->file, "%s", line);
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -1385,7 +1389,8 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
o->found_changes = 1;
 
if (ecbdata->header) {
-   fprintf(o->file, "%s", ecbdata->header->buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
+ecbdata->header->buf, ecbdata->header->len, 0);
strbuf_reset(ecbdata->header);
ecbdata->header = NULL;
}
@@ -2519,7 +2524,8 @@ static void builtin_diff(const char *name_a,
if (complete_rewrite &&
(textconv_one || !diff_filespec_is_binary(one)) &&
(textconv_two || !diff_filespec_is_binary(two))) {
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
+header.buf, header.len, 0);
strbuf_reset(&header);
emit_rewrite_diff(name_a, name_b, one, two,
textconv_one, textconv_two, o);
@@ -2529,7 +2535,8 @@ static void builtin_diff(const char *name_a,
}
 
if (o->irreversible_delete && lbl[1][0] == '/') {
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER, header.buf,
+header.len, 0);
strbuf_reset(&header);
goto free_ab_and_return;
} else if (!DIFF_OPT_TST(o, TEXT) &&
@@ -2540,10 +2547,13 @@ static void builtin_diff(const char *name_a,
!DIFF_OPT_TST(o, BINARY)) {
if (!oidcmp(&one->oid, &two->oid)) {
if (must_show_header)
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
+header.buf, header.len,
+0);
goto free_ab_and_return;
}
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
+header.buf, header.len, 0);
fprintf(o->file, "%sBinary files %s and %s differ\n",
line_prefix, lbl[0], lbl[1]);
goto free_ab_and_return;
@@ -2554,10 +2564,11 @@ static void builtin_diff(const char *name_a,
if (mf1.size == mf2.size &&
!memcmp(mf1.ptr, mf2.ptr, mf1.size)) {
if (must_show_header)
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
+header.buf, header.len, 0);
goto free_ab_and_return;
}
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER, header.buf, header.len, 
0);
strbuf_reset(&header);
if (DIFF_OPT_TST(o, BINARY))
emit_binary_diff(o->file, &mf1, &mf2, line_prefix);
@@ -2575,7 +2586,8 @@ static void builtin_diff(const char *name_a,
const struct userdiff_funcname *pe;
 
if (must_show_header) {
-   fprintf(o->file, "%s", header.buf);
+   emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
+header.buf, header.len, 0);
strbuf_reset(&header);
}
 
-- 
2.13.0.31.g9b732c453e

[PATCH 05/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_MARKER

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 4637368d59..75b996c4cf 100644
--- a/diff.c
+++ b/diff.c
@@ -561,13 +561,20 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_CONTEXT_MARKER,
DIFF_SYMBOL_SEPARATOR
 };
 
 static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
 const char *line, int len)
 {
+   const char *context, *reset;
switch (s) {
+   case DIFF_SYMBOL_CONTEXT_MARKER:
+   context = diff_get_color_opt(o, DIFF_CONTEXT);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, context, reset, line, len);
+   break;
case DIFF_SYMBOL_SEPARATOR:
fprintf(o->file, "%s%c",
diff_line_prefix(o),
@@ -662,7 +669,8 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
if (len < 10 ||
memcmp(line, atat, 2) ||
!(ep = memmem(line + 2, len - 2, atat, 2))) {
-   emit_line(ecbdata->opt, context, reset, line, len);
+   emit_diff_symbol(ecbdata->opt,
+DIFF_SYMBOL_CONTEXT_MARKER, line, len);
return;
}
ep += 2; /* skip over @@ */
-- 
2.13.0.31.g9b732c453e

[PATCH 04/25] diff.c: introduce emit_diff_symbol

In a later patch we want to buffer all output before emitting it as a
new feature ("markup moved lines") conceptually cannot be implemented
in a single pass over the output.

There are different approaches to buffer all output such as:
* Buffering on the char level, i.e. we'd have a char[] which would
  grow at approximately 80 characters a line. This would keep the
  output completely unstructured, but might be very easy to implement,
  such as redirecting all output to a temporary file and working off
  that. The later passes over the buffer are quite complicated though,
  because we have to parse back any output and then decide if it should
  be modified.

* Buffer on a line level. As the output is mostly line oriented already,
  this would make sense, but it still is a bit awkward as we'd have to
  make sense of it again by looking at the first characters of a line
  to decide what part of a diff a line is.

* Buffer semantically. Imagine there is a formal grammar for the diff
  output and we'd keep the symbols of this grammar around. This keeps
  the highest level of structure in the buffered data, such that the
  actual memory requirements are less than say the first option. Instead
  of buffering the characters of the line, we'll buffer what we intend
  to do plus additional information for the specifics. An output of

diff --git a/new.txt b/new.txt
index fa69b07..412428c 100644
Binary files a/new.txt and b/new.txt differ

  could be buffered as
 DIFF_SYMBOL_DIFF_START + new.txt
 DIFF_SYMBOL_INDEX_MODE + fa69b07 412428c "non-executable" flag
 DIFF_SYMBOL_BINARY_FILES + new.txt

This and the following patches introduce the third option of buffering
by first moving any output to emit_diff_symbol, and then introducing the
buffering in this function.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 22 +++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 77ef56a6e4..4637368d59 100644
--- a/diff.c
+++ b/diff.c
@@ -560,6 +560,24 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
emit_line_0(o, set, reset, line[0], line+1, len-1);
 }
 
+enum diff_symbol {
+   DIFF_SYMBOL_SEPARATOR
+};
+
+static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
+const char *line, int len)
+{
+   switch (s) {
+   case DIFF_SYMBOL_SEPARATOR:
+   fprintf(o->file, "%s%c",
+   diff_line_prefix(o),
+   o->line_termination);
+   break;
+   default:
+   die("BUG: unknown diff symbol");
+   }
+}
+
 static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char 
*line, int len)
 {
if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
@@ -4828,9 +4846,7 @@ void diff_flush(struct diff_options *options)
 
if (output_format & DIFF_FORMAT_PATCH) {
if (separator) {
-   fprintf(options->file, "%s%c",
-   diff_line_prefix(options),
-   options->line_termination);
+   emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 
0);
if (options->stat_sep) {
/* attach patch instead of inline */
fputs(options->stat_sep, options->file);
-- 
2.13.0.31.g9b732c453e

[PATCH 01/25] diff.c: readability fix

We already have dereferenced 'p->two' into a local variable 'two'.
Use that.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 00b4c86698..2874dfc6fc 100644
--- a/diff.c
+++ b/diff.c
@@ -3282,8 +3282,8 @@ static void run_diff(struct diff_filepair *p, struct 
diff_options *o)
const char *other;
const char *attr_path;
 
-   name  = p->one->path;
-   other = (strcmp(name, p->two->path) ? p->two->path : NULL);
+   name  = one->path;
+   other = (strcmp(name, two->path) ? two->path : NULL);
attr_path = name;
if (o->prefix_length)
strip_prefix(o->prefix_length, &name, &other);
-- 
2.13.0.31.g9b732c453e

[PATCH 03/25] diff.c: factor out diff_flush_patch_all_file_pairs

In a later patch we want to do more things before and after all filepairs
are flushed. So factor flushing out all file pairs into its own function
that the new code can be plugged in easily.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 94fdb57927..77ef56a6e4 100644
--- a/diff.c
+++ b/diff.c
@@ -4734,6 +4734,17 @@ void diff_warn_rename_limit(const char *varname, int 
needed, int degraded_cc)
warning(_(rename_limit_advice), varname, needed);
 }
 
+static void diff_flush_patch_all_file_pairs(struct diff_options *o)
+{
+   int i;
+   struct diff_queue_struct *q = &diff_queued_diff;
+   for (i = 0; i < q->nr; i++) {
+   struct diff_filepair *p = q->queue[i];
+   if (check_pair_status(p))
+   diff_flush_patch(p, o);
+   }
+}
+
 void diff_flush(struct diff_options *options)
 {
struct diff_queue_struct *q = &diff_queued_diff;
@@ -4826,11 +4837,7 @@ void diff_flush(struct diff_options *options)
}
}
 
-   for (i = 0; i < q->nr; i++) {
-   struct diff_filepair *p = q->queue[i];
-   if (check_pair_status(p))
-   diff_flush_patch(p, options);
-   }
+   diff_flush_patch_all_file_pairs(options);
}
 
if (output_format & DIFF_FORMAT_CALLBACK)
-- 
2.13.0.31.g9b732c453e

[PATCH 02/25] diff.c: move line ending check into emit_hunk_header

The emit_hunk_header() function is responsible for assembling a
hunk header and calling emit_line() to send the hunk header
to the output file.  Its only caller fn_out_consume() needs
to prepare for a case where the function emits an incomplete
line and add the terminating LF.

Instead make sure emit_hunk_header() to always send a
completed line to emit_line().

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 2874dfc6fc..94fdb57927 100644
--- a/diff.c
+++ b/diff.c
@@ -678,6 +678,8 @@ static void emit_hunk_header(struct emit_callback *ecbdata,
}
 
strbuf_add(&msgbuf, line + len, org_len - len);
+   strbuf_complete_line(&msgbuf);
+
emit_line(ecbdata->opt, "", "", msgbuf.buf, msgbuf.len);
strbuf_release(&msgbuf);
 }
@@ -1315,8 +1317,6 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
len = sane_truncate_line(ecbdata, line, len);
find_lno(line, ecbdata);
emit_hunk_header(ecbdata, line, len);
-   if (line[len-1] != '\n')
-   putc('\n', o->file);
return;
}
 
-- 
2.13.0.31.g9b732c453e

[PATCH 24/25] diff.c: add dimming to moved line detection

Any lines inside a moved block of code are not interesting. Boundaries
of blocks are only interesting if they are next to another block of moved
code.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 color.h|   2 +
 diff.c | 132 +
 diff.h |   9 +++-
 t/t4015-diff-whitespace.sh | 124 ++
 4 files changed, 254 insertions(+), 13 deletions(-)

diff --git a/color.h b/color.h
index 90627650fc..fd2b688dfb 100644
--- a/color.h
+++ b/color.h
@@ -42,6 +42,8 @@ struct strbuf;
 #define GIT_COLOR_BG_BLUE  "\033[44m"
 #define GIT_COLOR_BG_MAGENTA   "\033[45m"
 #define GIT_COLOR_BG_CYAN  "\033[46m"
+#define GIT_COLOR_FAINT"\033[2m"
+#define GIT_COLOR_FAINT_ITALIC "\033[2;3m"
 
 /* A special value meaning "no color selected" */
 #define GIT_COLOR_NIL "NIL"
diff --git a/diff.c b/diff.c
index 41ee81d1d8..6b79e6a54f 100644
--- a/diff.c
+++ b/diff.c
@@ -58,10 +58,14 @@ static char diff_colors[][COLOR_MAXLEN] = {
GIT_COLOR_YELLOW,   /* COMMIT */
GIT_COLOR_BG_RED,   /* WHITESPACE */
GIT_COLOR_NORMAL,   /* FUNCINFO */
-   GIT_COLOR_MAGENTA,  /* OLD_MOVED */
-   GIT_COLOR_BLUE, /* OLD_MOVED ALTERNATIVE */
-   GIT_COLOR_CYAN, /* NEW_MOVED */
-   GIT_COLOR_YELLOW,   /* NEW_MOVED ALTERNATIVE */
+   GIT_COLOR_BOLD_MAGENTA, /* OLD_MOVED */
+   GIT_COLOR_BOLD_BLUE,/* OLD_MOVED ALTERNATIVE */
+   GIT_COLOR_FAINT,/* OLD_MOVED_DIM */
+   GIT_COLOR_FAINT_ITALIC, /* OLD_MOVED_ALTERNATIVE_DIM */
+   GIT_COLOR_BOLD_CYAN,/* NEW_MOVED */
+   GIT_COLOR_BOLD_YELLOW,  /* NEW_MOVED ALTERNATIVE */
+   GIT_COLOR_FAINT,/* NEW_MOVED_DIM */
+   GIT_COLOR_FAINT_ITALIC, /* NEW_MOVED_ALTERNATIVE_DIM */
 };
 
 static NORETURN void die_want_option(const char *option_name)
@@ -91,10 +95,18 @@ static int parse_diff_color_slot(const char *var)
return DIFF_FILE_OLD_MOVED;
if (!strcasecmp(var, "oldmovedalternative"))
return DIFF_FILE_OLD_MOVED_ALT;
+   if (!strcasecmp(var, "oldmoveddimmed"))
+   return DIFF_FILE_OLD_MOVED_DIM;
+   if (!strcasecmp(var, "oldmovedalternativedimmed"))
+   return DIFF_FILE_OLD_MOVED_ALT_DIM;
if (!strcasecmp(var, "newmoved"))
return DIFF_FILE_NEW_MOVED;
if (!strcasecmp(var, "newmovedalternative"))
return DIFF_FILE_NEW_MOVED_ALT;
+   if (!strcasecmp(var, "newmoveddimmed"))
+   return DIFF_FILE_NEW_MOVED_DIM;
+   if (!strcasecmp(var, "newmovedalternativedimmed"))
+   return DIFF_FILE_NEW_MOVED_ALT_DIM;
return -1;
 }
 
@@ -262,8 +274,10 @@ static int parse_color_moved(const char *arg)
return COLOR_MOVED_ZEBRA;
else if (!strcmp(arg, "default"))
return COLOR_MOVED_DEFAULT;
+   else if (!strcmp(arg, "dimmed_zebra"))
+   return COLOR_MOVED_ZEBRA_DIM;
else
-   return error(_("color moved setting must be one of 'no', 
'default', 'zebra', 'plain'"));
+   return error(_("color moved setting must be one of 'no', 
'default', 'zebra', 'dimmed_zebra', 'plain'"));
 }
 
 int git_diff_ui_config(const char *var, const char *value, void *cb)
@@ -649,6 +663,7 @@ enum diff_symbol {
 #define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<16)
 #define DIFF_SYMBOL_MOVED_LINE (1<<17)
 #define DIFF_SYMBOL_MOVED_LINE_ALT (1<<18)
+#define DIFF_SYMBOL_MOVED_LINE_UNINTERESTING   (1<<19)
 #define DIFF_SYMBOL_CONTENT_WS_MASK (WSEH_NEW | WSEH_OLD | WSEH_CONTEXT | 
WS_RULE_MASK)
 
 /*
@@ -930,6 +945,67 @@ static void mark_color_as_moved(struct diff_options *o,
free(pmb);
 }
 
+#define DIFF_SYMBOL_MOVED_LINE_ZEBRA_MASK \
+  (DIFF_SYMBOL_MOVED_LINE | DIFF_SYMBOL_MOVED_LINE_ALT)
+static void dim_moved_lines(struct diff_options *o)
+{
+   int n;
+   for (n = 0; n < o->emitted_symbols->nr; n++) {
+   struct emitted_diff_symbol *prev = (n != 0) ?
+   &o->emitted_symbols->buf[n - 1] : NULL;
+   struct emitted_diff_symbol *l = &o->emitted_symbols->buf[n];
+   struct emitted_diff_symbol *next =
+   (n < o->emitted_symbols->nr - 1) ?
+   &o->emitted_symbols->buf[n + 1] : NULL;
+
+   /* Not a plus or minus line? */
+   if (l->s != DIFF_SYMBOL_PLUS && l->s != DIFF_SYMBOL_MINUS)
+   continue;
+
+   /* Not a moved line? */
+   if (!(l->flags & DIFF_SYMBOL_MOVED_LINE))
+   continue;
+
+   /*
+* If prev or next are not a plus or minus line,
+* pretend they don't exist
+*/
+   if (prev && prev->s != DIFF_SYMBOL_PLUS &&

[PATCH 19/25] diff.c: emit_diff_symbol learns about DIFF_SYMBOL_STAT_SEP

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index e0d39d04da..5a9c55736d 100644
--- a/diff.c
+++ b/diff.c
@@ -571,6 +571,7 @@ enum diff_symbol {
DIFF_SYMBOL_STATS_SUMMARY_INSERTS_DELETES,
DIFF_SYMBOL_STATS_LINE,
DIFF_SYMBOL_WORD_DIFF,
+   DIFF_SYMBOL_STAT_SEP,
DIFF_SYMBOL_SUBMODULE_ADD,
DIFF_SYMBOL_SUBMODULE_DEL,
DIFF_SYMBOL_SUBMODULE_UNTRACKED,
@@ -766,6 +767,9 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_WORD_DIFF:
fprintf(o->file, "%.*s", len, line);
break;
+   case DIFF_SYMBOL_STAT_SEP:
+   fputs(o->stat_sep, o->file);
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -5077,10 +5081,10 @@ void diff_flush(struct diff_options *options)
if (output_format & DIFF_FORMAT_PATCH) {
if (separator) {
emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 
0, 0);
-   if (options->stat_sep) {
+   if (options->stat_sep)
/* attach patch instead of inline */
-   fputs(options->stat_sep, options->file);
-   }
+   emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP,
+NULL, 0, 0);
}
 
diff_flush_patch_all_file_pairs(options);
-- 
2.13.0.31.g9b732c453e

[PATCH 18/25] diff.c: convert word diffing to use emit_diff_symbol

The word diffing is not line oriented and would need some serious
effort to be transformed into a line oriented approach, so
just go with a symbol DIFF_SYMBOL_WORD_DIFF that is a partial line.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 79 ++
 1 file changed, 46 insertions(+), 33 deletions(-)

diff --git a/diff.c b/diff.c
index 76d4b8ebf9..e0d39d04da 100644
--- a/diff.c
+++ b/diff.c
@@ -570,6 +570,7 @@ enum diff_symbol {
DIFF_SYMBOL_STATS_SUMMARY_ABBREV,
DIFF_SYMBOL_STATS_SUMMARY_INSERTS_DELETES,
DIFF_SYMBOL_STATS_LINE,
+   DIFF_SYMBOL_WORD_DIFF,
DIFF_SYMBOL_SUBMODULE_ADD,
DIFF_SYMBOL_SUBMODULE_DEL,
DIFF_SYMBOL_SUBMODULE_UNTRACKED,
@@ -762,6 +763,9 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_STATS_SUMMARY_ABBREV:
emit_line(o, "", "", " ...\n", strlen(" ...\n"));
break;
+   case DIFF_SYMBOL_WORD_DIFF:
+   fprintf(o->file, "%.*s", len, line);
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -1092,37 +1096,49 @@ struct diff_words_data {
struct diff_words_style *style;
 };
 
-static int fn_out_diff_words_write_helper(FILE *fp,
+static int fn_out_diff_words_write_helper(struct diff_options *o,
  struct diff_words_style_elem *st_el,
  const char *newline,
- size_t count, const char *buf,
- const char *line_prefix)
+ size_t count, const char *buf)
 {
int print = 0;
+   struct strbuf sb = STRBUF_INIT;
 
while (count) {
char *p = memchr(buf, '\n', count);
if (print)
-   fputs(line_prefix, fp);
+   strbuf_addstr(&sb, diff_line_prefix(o));
+
if (p != buf) {
-   if (st_el->color && fputs(st_el->color, fp) < 0)
-   return -1;
-   if (fputs(st_el->prefix, fp) < 0 ||
-   fwrite(buf, p ? p - buf : count, 1, fp) != 1 ||
-   fputs(st_el->suffix, fp) < 0)
-   return -1;
-   if (st_el->color && *st_el->color
-   && fputs(GIT_COLOR_RESET, fp) < 0)
-   return -1;
+   const char *reset = st_el->color && *st_el->color ?
+   GIT_COLOR_RESET : NULL;
+   if (st_el->color && *st_el->color)
+   strbuf_addstr(&sb, st_el->color);
+   strbuf_addstr(&sb, st_el->prefix);
+   strbuf_add(&sb, buf, p ? p - buf : count);
+   strbuf_addstr(&sb, st_el->suffix);
+   if (reset)
+   strbuf_addstr(&sb, reset);
}
if (!p)
-   return 0;
-   if (fputs(newline, fp) < 0)
-   return -1;
+   goto out;
+
+   strbuf_addstr(&sb, newline);
count -= p + 1 - buf;
buf = p + 1;
print = 1;
+   if (count) {
+   emit_diff_symbol(o, DIFF_SYMBOL_WORD_DIFF,
+sb.buf, sb.len, 0);
+   strbuf_reset(&sb);
+   }
}
+
+out:
+   if (sb.len)
+   emit_diff_symbol(o, DIFF_SYMBOL_WORD_DIFF,
+sb.buf, sb.len, 0);
+   strbuf_release(&sb);
return 0;
 }
 
@@ -1204,24 +1220,20 @@ static void fn_out_diff_words_aux(void *priv, char 
*line, unsigned long len)
fputs(line_prefix, diff_words->opt->file);
}
if (diff_words->current_plus != plus_begin) {
-   fn_out_diff_words_write_helper(diff_words->opt->file,
+   fn_out_diff_words_write_helper(diff_words->opt,
&style->ctx, style->newline,
plus_begin - diff_words->current_plus,
-   diff_words->current_plus, line_prefix);
-   if (*(plus_begin - 1) == '\n')
-   fputs(line_prefix, diff_words->opt->file);
+   diff_words->current_plus);
}
if (minus_begin != minus_end) {
-   fn_out_diff_words_write_helper(diff_words->opt->file,
+   fn_out_diff_words_write_helper(diff_words->opt,
&style->old, style->newline,
-   minus_end - minus_begin, minus_begin,
-   line_pr

[PATCH 10/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_INCOMPLETE

The context marker use the exact same output pattern, so reuse it.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 85bfd9310d..b2b2a19fcf 100644
--- a/diff.c
+++ b/diff.c
@@ -564,6 +564,7 @@ enum diff_symbol {
DIFF_SYMBOL_WORDS_PORCELAIN,
DIFF_SYMBOL_WORDS,
DIFF_SYMBOL_CONTEXT,
+   DIFF_SYMBOL_CONTEXT_INCOMPLETE,
DIFF_SYMBOL_PLUS,
DIFF_SYMBOL_MINUS,
DIFF_SYMBOL_NO_LF_EOF,
@@ -622,6 +623,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_CONTEXT_FRAGINFO:
emit_line(o, "", "", line, len);
break;
+   case DIFF_SYMBOL_CONTEXT_INCOMPLETE:
case DIFF_SYMBOL_CONTEXT_MARKER:
context = diff_get_color_opt(o, DIFF_CONTEXT);
reset = diff_get_color_opt(o, DIFF_RESET);
@@ -1448,8 +1450,8 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
default:
/* incomplete line at the end */
ecbdata->lno_in_preimage++;
-   emit_line(o, diff_get_color(ecbdata->color_diff, DIFF_CONTEXT),
- reset, line, len);
+   emit_diff_symbol(o, DIFF_SYMBOL_CONTEXT_INCOMPLETE,
+line, len, 0);
break;
}
 }
-- 
2.13.0.31.g9b732c453e

[PATCH 23/25] diff.c: color moved lines differently, plain mode

Add the 'plain' mode for move detection of code. This omits the checking
for adjacent blocks, so it is not as useful. If you have a lot of the
same blocks moved in the same patch, the 'Zebra' would end up slow as it
is O(n^2) (n is number of same blocks). So this may be useful there and
is generally easy to add. Instead be very literal at the move detection,
do not skip over short blocks here.

Signed-off-by: Stefan Beller 
---
 diff.c | 10 +++--
 diff.h |  1 +
 t/t4015-diff-whitespace.sh | 51 +-
 3 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 322a2bfd72..41ee81d1d8 100644
--- a/diff.c
+++ b/diff.c
@@ -256,12 +256,14 @@ static int parse_color_moved(const char *arg)
 
if (!strcmp(arg, "no"))
return COLOR_MOVED_NO;
+   else if (!strcmp(arg, "plain"))
+   return COLOR_MOVED_PLAIN;
else if (!strcmp(arg, "zebra"))
return COLOR_MOVED_ZEBRA;
else if (!strcmp(arg, "default"))
return COLOR_MOVED_DEFAULT;
else
-   return error(_("color moved setting must be one of 'no', 
'default', 'zebra'"));
+   return error(_("color moved setting must be one of 'no', 
'default', 'zebra', 'plain'"));
 }
 
 int git_diff_ui_config(const char *var, const char *value, void *cb)
@@ -876,7 +878,8 @@ static void mark_color_as_moved(struct diff_options *o,
}
 
if (!match) {
-   if (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH) {
+   if (block_length < COLOR_MOVED_MIN_BLOCK_LENGTH &&
+   o->color_moved != COLOR_MOVED_PLAIN) {
for (i = 0; i < block_length + 1; i++) {
l = &o->emitted_symbols->buf[n - i];
l->flags &= ~DIFF_SYMBOL_MOVED_LINE;
@@ -890,6 +893,9 @@ static void mark_color_as_moved(struct diff_options *o,
l->flags |= DIFF_SYMBOL_MOVED_LINE;
block_length++;
 
+   if (o->color_moved == COLOR_MOVED_PLAIN)
+   continue;
+
/* Check any potential block runs, advance each or nullify */
for (i = 0; i < pmb_nr; i++) {
struct moved_entry *p = pmb[i];
diff --git a/diff.h b/diff.h
index 3196802673..4cfd609c54 100644
--- a/diff.h
+++ b/diff.h
@@ -190,6 +190,7 @@ struct diff_options {
struct emitted_diff_symbols *emitted_symbols;
enum {
COLOR_MOVED_NO = 0,
+   COLOR_MOVED_PLAIN = 1,
COLOR_MOVED_ZEBRA = 2,
} color_moved;
#define COLOR_MOVED_DEFAULT COLOR_MOVED_ZEBRA
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 29704ae14e..d1d7b0 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -986,7 +986,7 @@ test_expect_success 'detect moved code, complete file' '
git mv test.c main.c &&
test_config color.diff.oldMoved "normal red" &&
test_config color.diff.newMoved "normal green" &&
-   git diff HEAD --color-moved --no-renames | test_decode_color >actual &&
+   git diff HEAD --color-moved=zebra --no-renames | test_decode_color 
>actual &&
cat >expected <<-\EOF &&
diff --git a/main.c b/main.c
new file mode 100644
@@ -1130,6 +1130,55 @@ test_expect_success 'detect malicious moved code, inside 
file' '
test_cmp expected actual
 '
 
+test_expect_success 'plain moved code, inside file' '
+   test_config color.diff.oldMoved "normal red" &&
+   test_config color.diff.newMoved "normal green" &&
+   test_config color.diff.oldMovedAlternative "blue" &&
+   test_config color.diff.newMovedAlternative "yellow" &&
+   # needs previous test as setup
+   git diff HEAD --no-renames --color-moved=plain| test_decode_color 
>actual &&
+   cat <<-\EOF >expected &&
+   diff --git a/main.c b/main.c
+   index 27a619c..7cf9336 100644
+   --- a/main.c
+   +++ b/main.c
+   @@ -5,13 +5,6 @@ printf("Hello ");
+printf("World\n");
+}
+
+   -int secure_foo(struct user *u)
+   -{
+   -if (!u->is_allowed_foo)
+   -return;
+   -foo(u);
+   -}
+   -
+int main()
+{
+foo();
+   diff --git a/test.c b/test.c
+   index 1dc1d85..2bedec9 100644
+   --- a/test.c
+   +++ b/test.c
+   @@ -4,6 +4,13 @@ int bar()
+printf("Hello World, but different\n");
+}
+
+   +int secure_foo(struct user *u)
+   +{
+   +foo(u);
+   +if (!u->is_allowed_foo)
+   +return;
+   +}
+   +
+int another_function()
+{
+bar();
+   EOF
+
+   test_cmp expected actual
+'
+
 test_expect_success 'no effect from --color-moved with --word-diff' '
c

[PATCH 14/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_REWRITE_DIFF

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 35 +--
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/diff.c b/diff.c
index 1d0f8b509f..5c428e02b6 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,7 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_REWRITE_DIFF,
DIFF_SYMBOL_BINARY_FILES,
DIFF_SYMBOL_HEADER,
DIFF_SYMBOL_FILEPAIR_PLUS,
@@ -615,7 +616,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
 const char *line, int len, unsigned flags)
 {
static const char *nneof = " No newline at end of file\n";
-   const char *context, *reset, *set, *meta;
+   const char *context, *reset, *set, *meta, *fraginfo;
switch (s) {
case DIFF_SYMBOL_NO_LF_EOF:
context = diff_get_color_opt(o, DIFF_CONTEXT);
@@ -695,6 +696,11 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_HEADER:
fprintf(o->file, "%s", line);
break;
+   case DIFF_SYMBOL_REWRITE_DIFF:
+   fraginfo = diff_get_color(o->use_color, DIFF_FRAGINFO);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, fraginfo, reset, line, len);
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -817,17 +823,17 @@ static void remove_tempfile(void)
}
 }
 
-static void print_line_count(FILE *file, int count)
+static void add_line_count(struct strbuf *out, int count)
 {
switch (count) {
case 0:
-   fprintf(file, "0,0");
+   strbuf_addstr(out, "0,0");
break;
case 1:
-   fprintf(file, "1");
+   strbuf_addstr(out, "1");
break;
default:
-   fprintf(file, "1,%d", count);
+   strbuf_addf(out, "1,%d", count);
break;
}
 }
@@ -866,14 +872,12 @@ static void emit_rewrite_diff(const char *name_a,
  struct diff_options *o)
 {
int lc_a, lc_b;
-   const char *fraginfo = diff_get_color(o->use_color, DIFF_FRAGINFO);
-   const char *reset = diff_get_color(o->use_color, DIFF_RESET);
static struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;
const char *a_prefix, *b_prefix;
char *data_one, *data_two;
size_t size_one, size_two;
struct emit_callback ecbdata;
-   const char *line_prefix = diff_line_prefix(o);
+   struct strbuf out = STRBUF_INIT;
 
if (diff_mnemonic_prefix && DIFF_OPT_TST(o, REVERSE_DIFF)) {
a_prefix = o->b_prefix;
@@ -917,14 +921,17 @@ static void emit_rewrite_diff(const char *name_a,
emit_diff_symbol(o, DIFF_SYMBOL_FILEPAIR_PLUS,
 b_name.buf, b_name.len, 0);
 
-   fprintf(o->file, "%s%s@@ -", line_prefix, fraginfo);
+   strbuf_addstr(&out, "@@ -");
if (!o->irreversible_delete)
-   print_line_count(o->file, lc_a);
+   add_line_count(&out, lc_a);
else
-   fprintf(o->file, "?,?");
-   fprintf(o->file, " +");
-   print_line_count(o->file, lc_b);
-   fprintf(o->file, " @@%s\n", reset);
+   strbuf_addstr(&out, "?,?");
+   strbuf_addstr(&out, " +");
+   add_line_count(&out, lc_b);
+   strbuf_addstr(&out, " @@\n");
+   emit_diff_symbol(o, DIFF_SYMBOL_REWRITE_DIFF, out.buf, out.len, 0);
+   strbuf_release(&out);
+
if (lc_a && !o->irreversible_delete)
emit_rewrite_lines(&ecbdata, '-', data_one, size_one);
if (lc_b)
-- 
2.13.0.31.g9b732c453e

[PATCH 15/25] submodule.c: migrate diff output to use emit_diff_symbol

As the submodule process is no longer attached to the same file pointer
'o->file' as the superprojects process, there is a different result in
color.c::check_auto_color. That is why we need to pass coloring explicitly,
such that the submodule coloring decision will be made by the child process
processing the submodule. Only DIFF_SYMBOL_SUBMODULE_PIPETHROUGH contains
color, the other symbols are for embedding the submodule output into the
superprojects output.

Remove the colors from the function signatures, as all the coloring
decisions will be made either inside the child process or the final
emit_diff_symbol, but not in the functions driving the submodule diff.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c  | 83 +++-
 diff.h  |  9 +++
 submodule.c | 84 +++--
 submodule.h | 13 +++---
 4 files changed, 121 insertions(+), 68 deletions(-)

diff --git a/diff.c b/diff.c
index 5c428e02b6..48f719fb07 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,13 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_SUBMODULE_ADD,
+   DIFF_SYMBOL_SUBMODULE_DEL,
+   DIFF_SYMBOL_SUBMODULE_UNTRACKED,
+   DIFF_SYMBOL_SUBMODULE_MODIFIED,
+   DIFF_SYMBOL_SUBMODULE_HEADER,
+   DIFF_SYMBOL_SUBMODULE_ERROR,
+   DIFF_SYMBOL_SUBMODULE_PIPETHROUGH,
DIFF_SYMBOL_REWRITE_DIFF,
DIFF_SYMBOL_BINARY_FILES,
DIFF_SYMBOL_HEADER,
@@ -625,6 +632,9 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
emit_line_0(o, context, reset, '\\',
nneof, strlen(nneof));
break;
+   case DIFF_SYMBOL_SUBMODULE_HEADER:
+   case DIFF_SYMBOL_SUBMODULE_ERROR:
+   case DIFF_SYMBOL_SUBMODULE_PIPETHROUGH:
case DIFF_SYMBOL_CONTEXT_FRAGINFO:
emit_line(o, "", "", line, len);
break;
@@ -701,11 +711,68 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
reset = diff_get_color_opt(o, DIFF_RESET);
emit_line(o, fraginfo, reset, line, len);
break;
+   case DIFF_SYMBOL_SUBMODULE_ADD:
+   set = diff_get_color_opt(o, DIFF_FILE_NEW);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, set, reset, line, len);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_DEL:
+   set = diff_get_color_opt(o, DIFF_FILE_OLD);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, set, reset, line, len);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_UNTRACKED:
+   fprintf(o->file, "%sSubmodule %s contains untracked content\n",
+   diff_line_prefix(o), line);
+   break;
+   case DIFF_SYMBOL_SUBMODULE_MODIFIED:
+   fprintf(o->file, "%sSubmodule %s contains modified content\n",
+   diff_line_prefix(o), line);
+   break;
default:
die("BUG: unknown diff symbol");
}
 }
 
+void diff_emit_submodule_del(struct diff_options *o, const char *line)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_DEL, line, strlen(line), 0);
+}
+
+void diff_emit_submodule_add(struct diff_options *o, const char *line)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_ADD, line, strlen(line), 0);
+}
+
+void diff_emit_submodule_untracked(struct diff_options *o, const char *path)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_UNTRACKED,
+path, strlen(path), 0);
+}
+
+void diff_emit_submodule_modified(struct diff_options *o, const char *path)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_MODIFIED,
+path, strlen(path), 0);
+}
+
+void diff_emit_submodule_header(struct diff_options *o, const char *header)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_HEADER,
+header, strlen(header), 0);
+}
+
+void diff_emit_submodule_error(struct diff_options *o, const char *err)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_ERROR, err, strlen(err), 0);
+}
+
+void diff_emit_submodule_pipethrough(struct diff_options *o,
+const char *line, int len)
+{
+   emit_diff_symbol(o, DIFF_SYMBOL_SUBMODULE_PIPETHROUGH, line, len, 0);
+}
+
 static int new_blank_line_at_eof(struct emit_callback *ecbdata, const char 
*line, int len)
 {
if (!((ecbdata->ws_rule & WS_BLANK_AT_EOF) &&
@@ -2467,24 +2534,16 @@ static void builtin_diff(const char *name_a,
if (o->submodule_format == DIFF_SUBMODULE_LOG &&
(!one->mode || S_ISGITLINK(one->mode)) &&
(!two->mode || S_ISGITLINK(two->mode))) {
-   const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
-   const

[PATCH 16/25] diff.c: convert emit_binary_diff_body to use emit_diff_symbol

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 63 ++-
 1 file changed, 46 insertions(+), 17 deletions(-)

diff --git a/diff.c b/diff.c
index 48f719fb07..f5a14359ae 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,11 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_BINARY_DIFF_HEADER,
+   DIFF_SYMBOL_BINARY_DIFF_HEADER_DELTA,
+   DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,
+   DIFF_SYMBOL_BINARY_DIFF_BODY,
+   DIFF_SYMBOL_BINARY_DIFF_FOOTER,
DIFF_SYMBOL_SUBMODULE_ADD,
DIFF_SYMBOL_SUBMODULE_DEL,
DIFF_SYMBOL_SUBMODULE_UNTRACKED,
@@ -635,6 +640,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_SUBMODULE_HEADER:
case DIFF_SYMBOL_SUBMODULE_ERROR:
case DIFF_SYMBOL_SUBMODULE_PIPETHROUGH:
+   case DIFF_SYMBOL_BINARY_DIFF_BODY:
case DIFF_SYMBOL_CONTEXT_FRAGINFO:
emit_line(o, "", "", line, len);
break;
@@ -706,6 +712,19 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_HEADER:
fprintf(o->file, "%s", line);
break;
+   case DIFF_SYMBOL_BINARY_DIFF_HEADER:
+   fprintf(o->file, "%sGIT binary patch\n", diff_line_prefix(o));
+   break;
+   case DIFF_SYMBOL_BINARY_DIFF_HEADER_DELTA:
+   fprintf(o->file, "%sdelta %s\n", diff_line_prefix(o), line);
+   break;
+   case DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL:
+   fprintf(o->file, "%sliteral %s\n", diff_line_prefix(o), line);
+   break;
+   case DIFF_SYMBOL_BINARY_DIFF_FOOTER:
+   fputs(diff_line_prefix(o), o->file);
+   fputc('\n', o->file);
+   break;
case DIFF_SYMBOL_REWRITE_DIFF:
fraginfo = diff_get_color(o->use_color, DIFF_FRAGINFO);
reset = diff_get_color_opt(o, DIFF_RESET);
@@ -2390,8 +2409,8 @@ static unsigned char *deflate_it(char *data,
return deflated;
 }
 
-static void emit_binary_diff_body(FILE *file, mmfile_t *one, mmfile_t *two,
- const char *prefix)
+static void emit_binary_diff_body(struct diff_options *o,
+ mmfile_t *one, mmfile_t *two)
 {
void *cp;
void *delta;
@@ -2420,13 +2439,18 @@ static void emit_binary_diff_body(FILE *file, mmfile_t 
*one, mmfile_t *two,
}
 
if (delta && delta_size < deflate_size) {
-   fprintf(file, "%sdelta %lu\n", prefix, orig_size);
+   char *s = xstrfmt("%lu", orig_size);
+   emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_DELTA,
+s, strlen(s), 0);
+   free(s);
free(deflated);
data = delta;
data_size = delta_size;
-   }
-   else {
-   fprintf(file, "%sliteral %lu\n", prefix, two->size);
+   } else {
+   char *s = xstrfmt("%lu", two->size);
+   emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,
+s, strlen(s), 0);
+   free(s);
free(delta);
data = deflated;
data_size = deflate_size;
@@ -2435,8 +2459,9 @@ static void emit_binary_diff_body(FILE *file, mmfile_t 
*one, mmfile_t *two,
/* emit data encoded in base85 */
cp = data;
while (data_size) {
+   int len;
int bytes = (52 < data_size) ? 52 : data_size;
-   char line[70];
+   char line[71];
data_size -= bytes;
if (bytes <= 26)
line[0] = bytes + 'A' - 1;
@@ -2444,20 +2469,24 @@ static void emit_binary_diff_body(FILE *file, mmfile_t 
*one, mmfile_t *two,
line[0] = bytes - 26 + 'a' - 1;
encode_85(line + 1, cp, bytes);
cp = (char *) cp + bytes;
-   fprintf(file, "%s", prefix);
-   fputs(line, file);
-   fputc('\n', file);
+
+   len = strlen(line);
+   line[len++] = '\n';
+   line[len] = '\0';
+
+   emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_BODY,
+line, len, 0);
}
-   fprintf(file, "%s\n", prefix);
+   emit_diff_symbol(o, DIFF_SYMBOL_BINARY_DIFF_FOOTER, NULL, 0, 0);
free(data);
 }
 
-static void emit_binary_diff(FILE *file, mmfile_t *one, mmfile_t *two,
-const char *prefix)
+static void emit_binary_diff(struct diff_options *o,
+mmfile_t *one, mmfile_t *two)
 {
-   fprintf(file, "%sGIT binary patch\n", prefix);
-   emit_binary_diff_body(file, one, two, prefix);
-   emit_

[PATCH 11/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_FILEPAIR_{PLUS, MINUS}

We have to use fprintf instead of emit_line, because we want to emit the
tab after the color. This is important for ancient versions of gnu patch
AFAICT, although we probably do not want to feed colored output to the
patch utility, such that it would not matter if the trailing tab is
colored. Keep the corner case as-is though.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 51 ++-
 1 file changed, 30 insertions(+), 21 deletions(-)

diff --git a/diff.c b/diff.c
index b2b2a19fcf..49b45fef29 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,8 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_FILEPAIR_PLUS,
+   DIFF_SYMBOL_FILEPAIR_MINUS,
DIFF_SYMBOL_WORDS_PORCELAIN,
DIFF_SYMBOL_WORDS,
DIFF_SYMBOL_CONTEXT,
@@ -611,7 +613,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
 const char *line, int len, unsigned flags)
 {
static const char *nneof = " No newline at end of file\n";
-   const char *context, *reset, *set;
+   const char *context, *reset, *set, *meta;
switch (s) {
case DIFF_SYMBOL_NO_LF_EOF:
context = diff_get_color_opt(o, DIFF_CONTEXT);
@@ -673,6 +675,20 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
}
emit_line(o, context, reset, line, len);
break;
+   case DIFF_SYMBOL_FILEPAIR_PLUS:
+   meta = diff_get_color_opt(o, DIFF_METAINFO);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   fprintf(o->file, "%s%s+++ %s%s%s\n", diff_line_prefix(o), meta,
+   line, reset,
+   strchr(line, ' ') ? "\t" : "");
+   break;
+   case DIFF_SYMBOL_FILEPAIR_MINUS:
+   meta = diff_get_color_opt(o, DIFF_METAINFO);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   fprintf(o->file, "%s%s--- %s%s%s\n", diff_line_prefix(o), meta,
+   line, reset,
+   strchr(line, ' ') ? "\t" : "");
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -844,8 +860,6 @@ static void emit_rewrite_diff(const char *name_a,
  struct diff_options *o)
 {
int lc_a, lc_b;
-   const char *name_a_tab, *name_b_tab;
-   const char *metainfo = diff_get_color(o->use_color, DIFF_METAINFO);
const char *fraginfo = diff_get_color(o->use_color, DIFF_FRAGINFO);
const char *reset = diff_get_color(o->use_color, DIFF_RESET);
static struct strbuf a_name = STRBUF_INIT, b_name = STRBUF_INIT;
@@ -865,8 +879,6 @@ static void emit_rewrite_diff(const char *name_a,
 
name_a += (*name_a == '/');
name_b += (*name_b == '/');
-   name_a_tab = strchr(name_a, ' ') ? "\t" : "";
-   name_b_tab = strchr(name_b, ' ') ? "\t" : "";
 
strbuf_reset(&a_name);
strbuf_reset(&b_name);
@@ -893,11 +905,13 @@ static void emit_rewrite_diff(const char *name_a,
 
lc_a = count_lines(data_one, size_one);
lc_b = count_lines(data_two, size_two);
-   fprintf(o->file,
-   "%s%s--- %s%s%s\n%s%s+++ %s%s%s\n%s%s@@ -",
-   line_prefix, metainfo, a_name.buf, name_a_tab, reset,
-   line_prefix, metainfo, b_name.buf, name_b_tab, reset,
-   line_prefix, fraginfo);
+
+   emit_diff_symbol(o, DIFF_SYMBOL_FILEPAIR_MINUS,
+a_name.buf, a_name.len, 0);
+   emit_diff_symbol(o, DIFF_SYMBOL_FILEPAIR_PLUS,
+b_name.buf, b_name.len, 0);
+
+   fprintf(o->file, "%s%s@@ -", line_prefix, fraginfo);
if (!o->irreversible_delete)
print_line_count(o->file, lc_a);
else
@@ -1365,10 +1379,8 @@ static void find_lno(const char *line, struct 
emit_callback *ecbdata)
 static void fn_out_consume(void *priv, char *line, unsigned long len)
 {
struct emit_callback *ecbdata = priv;
-   const char *meta = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
struct diff_options *o = ecbdata->opt;
-   const char *line_prefix = diff_line_prefix(o);
 
o->found_changes = 1;
 
@@ -1379,15 +1391,12 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
}
 
if (ecbdata->label_path[0]) {
-   const char *name_a_tab, *name_b_tab;
-
-   name_a_tab = strchr(ecbdata->label_path[0], ' ') ? "\t" : "";
-   name_b_tab = strchr(ecbdata->label_path[1], ' ') ? "\t" : "";
-
-   fprintf(o->file, "%s%s--- %s%s%s\n",
-   line_prefix, meta, ecbdata->label_path[0], reset, 
name_a_tab);
-   fprintf(o->file, "%s%s+++ %s%s

[PATCH 17/25] diff.c: convert show_stats to use emit_diff_symbol

We call print_stat_summary from builtin/apply, so we still
need the version with a file pointer, so introduce
print_stat_summary_0 that uses emit_string machinery and
keep print_stat_summary with the same arguments around.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 114 +
 diff.h |   4 +--
 2 files changed, 74 insertions(+), 44 deletions(-)

diff --git a/diff.c b/diff.c
index f5a14359ae..76d4b8ebf9 100644
--- a/diff.c
+++ b/diff.c
@@ -566,6 +566,10 @@ enum diff_symbol {
DIFF_SYMBOL_BINARY_DIFF_HEADER_LITERAL,
DIFF_SYMBOL_BINARY_DIFF_BODY,
DIFF_SYMBOL_BINARY_DIFF_FOOTER,
+   DIFF_SYMBOL_STATS_SUMMARY_NO_FILES,
+   DIFF_SYMBOL_STATS_SUMMARY_ABBREV,
+   DIFF_SYMBOL_STATS_SUMMARY_INSERTS_DELETES,
+   DIFF_SYMBOL_STATS_LINE,
DIFF_SYMBOL_SUBMODULE_ADD,
DIFF_SYMBOL_SUBMODULE_DEL,
DIFF_SYMBOL_SUBMODULE_UNTRACKED,
@@ -629,6 +633,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
 {
static const char *nneof = " No newline at end of file\n";
const char *context, *reset, *set, *meta, *fraginfo;
+   struct strbuf sb = STRBUF_INIT;
switch (s) {
case DIFF_SYMBOL_NO_LF_EOF:
context = diff_get_color_opt(o, DIFF_CONTEXT);
@@ -640,6 +645,8 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_SUBMODULE_HEADER:
case DIFF_SYMBOL_SUBMODULE_ERROR:
case DIFF_SYMBOL_SUBMODULE_PIPETHROUGH:
+   case DIFF_SYMBOL_STATS_SUMMARY_INSERTS_DELETES:
+   case DIFF_SYMBOL_STATS_LINE:
case DIFF_SYMBOL_BINARY_DIFF_BODY:
case DIFF_SYMBOL_CONTEXT_FRAGINFO:
emit_line(o, "", "", line, len);
@@ -748,9 +755,17 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
fprintf(o->file, "%sSubmodule %s contains modified content\n",
diff_line_prefix(o), line);
break;
+   case DIFF_SYMBOL_STATS_SUMMARY_NO_FILES:
+   emit_line(o, "", "", " 0 files changed\n",
+ strlen(" 0 files changed\n"));
+   break;
+   case DIFF_SYMBOL_STATS_SUMMARY_ABBREV:
+   emit_line(o, "", "", " ...\n", strlen(" ...\n"));
+   break;
default:
die("BUG: unknown diff symbol");
}
+   strbuf_release(&sb);
 }
 
 void diff_emit_submodule_del(struct diff_options *o, const char *line)
@@ -1705,20 +1720,14 @@ static int scale_linear(int it, int width, int 
max_change)
return 1 + (it * (width - 1) / max_change);
 }
 
-static void show_name(FILE *file,
- const char *prefix, const char *name, int len)
-{
-   fprintf(file, " %s%-*s |", prefix, len, name);
-}
-
-static void show_graph(FILE *file, char ch, int cnt, const char *set, const 
char *reset)
+static void show_graph(struct strbuf *out, char ch, int cnt,
+  const char *set, const char *reset)
 {
if (cnt <= 0)
return;
-   fprintf(file, "%s", set);
-   while (cnt--)
-   putc(ch, file);
-   fprintf(file, "%s", reset);
+   strbuf_addstr(out, set);
+   strbuf_addchars(out, ch, cnt);
+   strbuf_addstr(out, reset);
 }
 
 static void fill_print_name(struct diffstat_file *file)
@@ -1742,14 +1751,16 @@ static void fill_print_name(struct diffstat_file *file)
file->print_name = pname;
 }
 
-int print_stat_summary(FILE *fp, int files, int insertions, int deletions)
+static void print_stat_summary_inserts_deletes(struct diff_options *options,
+   int files, int insertions, int deletions)
 {
struct strbuf sb = STRBUF_INIT;
-   int ret;
 
if (!files) {
assert(insertions == 0 && deletions == 0);
-   return fprintf(fp, "%s\n", " 0 files changed");
+   emit_diff_symbol(options, DIFF_SYMBOL_STATS_SUMMARY_NO_FILES,
+NULL, 0, 0);
+   return;
}
 
strbuf_addf(&sb,
@@ -1776,9 +1787,19 @@ int print_stat_summary(FILE *fp, int files, int 
insertions, int deletions)
deletions);
}
strbuf_addch(&sb, '\n');
-   ret = fputs(sb.buf, fp);
+   emit_diff_symbol(options, DIFF_SYMBOL_STATS_SUMMARY_INSERTS_DELETES,
+sb.buf, sb.len, 0);
strbuf_release(&sb);
-   return ret;
+}
+
+void print_stat_summary(FILE *fp, int files,
+   int insertions, int deletions)
+{
+   struct diff_options o;
+   memset(&o, 0, sizeof(o));
+   o.file = fp;
+
+   print_stat_summary_inserts_deletes(&o, files, insertions, deletions);
 }
 
 static void show_stats(struct diffstat_t *data, struct diff_options *options)
@@ -1788,13 +1809,13 @@ static void show_stats(struct diffstat_t *data, struct 
diff_options *opt

[PATCH 13/25] diff.c: emit_diff_symbol learns about DIFF_SYMBOL_BINARY_FILES

we could save a little bit of memory when buffering in a later mode
by just passing the inner part ("%s and %s", file1, file 2), but
those a just a few bytes, so instead let's reuse the implementation from
DIFF_SYMBOL_HEADER and keep the whole line around.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 20 +++-
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/diff.c b/diff.c
index 78f7c6f82f..1d0f8b509f 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,7 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_BINARY_FILES,
DIFF_SYMBOL_HEADER,
DIFF_SYMBOL_FILEPAIR_PLUS,
DIFF_SYMBOL_FILEPAIR_MINUS,
@@ -690,6 +691,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
line, reset,
strchr(line, ' ') ? "\t" : "");
break;
+   case DIFF_SYMBOL_BINARY_FILES:
case DIFF_SYMBOL_HEADER:
fprintf(o->file, "%s", line);
break;
@@ -2542,6 +2544,7 @@ static void builtin_diff(const char *name_a,
} else if (!DIFF_OPT_TST(o, TEXT) &&
( (!textconv_one && diff_filespec_is_binary(one)) ||
  (!textconv_two && diff_filespec_is_binary(two)) )) {
+   struct strbuf sb = STRBUF_INIT;
if (!one->data && !two->data &&
S_ISREG(one->mode) && S_ISREG(two->mode) &&
!DIFF_OPT_TST(o, BINARY)) {
@@ -2554,8 +2557,11 @@ static void builtin_diff(const char *name_a,
}
emit_diff_symbol(o, DIFF_SYMBOL_HEADER,
 header.buf, header.len, 0);
-   fprintf(o->file, "%sBinary files %s and %s differ\n",
-   line_prefix, lbl[0], lbl[1]);
+   strbuf_addf(&sb, "%sBinary files %s and %s differ\n",
+   diff_line_prefix(o), lbl[0], lbl[1]);
+   emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES,
+sb.buf, sb.len, 0);
+   strbuf_release(&sb);
goto free_ab_and_return;
}
if (fill_mmfile(&mf1, one) < 0 || fill_mmfile(&mf2, two) < 0)
@@ -2572,9 +2578,13 @@ static void builtin_diff(const char *name_a,
strbuf_reset(&header);
if (DIFF_OPT_TST(o, BINARY))
emit_binary_diff(o->file, &mf1, &mf2, line_prefix);
-   else
-   fprintf(o->file, "%sBinary files %s and %s differ\n",
-   line_prefix, lbl[0], lbl[1]);
+   else {
+   strbuf_addf(&sb, "%sBinary files %s and %s differ\n",
+   diff_line_prefix(o), lbl[0], lbl[1]);
+   emit_diff_symbol(o, DIFF_SYMBOL_BINARY_FILES,
+sb.buf, sb.len, 0);
+   strbuf_release(&sb);
+   }
o->found_changes = 1;
} else {
/* Crazy xdl interfaces.. */
-- 
2.13.0.31.g9b732c453e

[PATCH 25/25] diff: document the new --color-moved setting

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt   | 15 +--
 Documentation/diff-options.txt | 34 ++
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 06898a7498..74382e5ff5 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1077,14 +1077,25 @@ This does not affect linkgit:git-format-patch[1] or the
 'git-diff-{asterisk}' plumbing commands.  Can be overridden on the
 command line with the `--color[=]` option.
 
+diff.colorMoved::
+   If set to either a valid `` or a true value, moved lines
+   in a diff are colored differently, for details of valid modes
+   see '--color-moved' in linkgit:git-diff[1]. If simply set to
+   true the default color mode will be used. When set to false,
+   moved lines are not colored.
+
 color.diff.::
Use customized color for diff colorization.  `` specifies
which part of the patch to use the specified color, and is one
of `context` (context text - `plain` is a historical synonym),
`meta` (metainformation), `frag`
(hunk header), 'func' (function in hunk header), `old` (removed lines),
-   `new` (added lines), `commit` (commit headers), or `whitespace`
-   (highlighting whitespace errors).
+   `new` (added lines), `commit` (commit headers), `whitespace`
+   (highlighting whitespace errors), `oldMoved` (deleted lines),
+   `newMoved` (added lines), `oldMovedDimmed`, `oldMovedAlternative`,
+   `oldMovedAlternativeDimmed`, `newMovedDimmed`, `newMovedAlternative`
+   and `newMovedAlternativeDimmed` (See the ''
+   setting of '--color-moved' in linkgit:git-diff[1] for details).
 
 color.decorate.::
Use customized color for 'git log --decorate' output.  `` is one
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 89cc0f48de..bb257a83b2 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -231,6 +231,40 @@ ifdef::git-diff[]
 endif::git-diff[]
It is the same as `--color=never`.
 
+--color-moved[=]::
+   Moved lines of code are colored differently.
+ifdef::git-diff[]
+   It can be changed by the `diff.colorMoved` configuration setting.
+endif::git-diff[]
+   The  defaults to 'no' if the option is not given
+   and to 'zebra' if the option with no mode is given.
+   The mode must be one of:
++
+--
+no::
+   Moved lines are not highlighted.
+default::
+   Is a synonym for `zebra`. This may change to a more sensible mode
+   in the future.
+plain::
+   Any line that is added in one location and was removed
+   in another location will be colored with 'color.diff.newMoved'.
+   Similarly 'color.diff.oldMoved' will be used for removed lines
+   that are added somewhere else in the diff. This mode picks up any
+   moved line, but it is not very useful in a review to determine
+   if a block of code was moved without permutation.
+zebra::
+   Blocks of moved code are detected greedily. The detected blocks are
+   painted using either the 'color.diff.{old,new}Moved' color or
+   'color.diff.{old,new}MovedAlternative'. The change between
+   the two colors indicates that a new block was detected.
+   Small blocks of 3 moved lines or fewer are skipped.
+dimmed_zebra::
+   Similar to 'zebra', but additional dimming of uninteresting parts
+   of moved code is performed. The bordering lines of two adjacent
+   blocks are considered interesting, the rest is uninteresting.
+--
+
 --word-diff[=]::
Show a word diff, using the  to delimit changed words.
By default, words are delimited by whitespace; see
-- 
2.13.0.31.g9b732c453e

[PATCH 20/25] diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 71 ++
 1 file changed, 41 insertions(+), 30 deletions(-)

diff --git a/diff.c b/diff.c
index 5a9c55736d..2db0d7c0f5 100644
--- a/diff.c
+++ b/diff.c
@@ -572,6 +572,7 @@ enum diff_symbol {
DIFF_SYMBOL_STATS_LINE,
DIFF_SYMBOL_WORD_DIFF,
DIFF_SYMBOL_STAT_SEP,
+   DIFF_SYMBOL_SUMMARY,
DIFF_SYMBOL_SUBMODULE_ADD,
DIFF_SYMBOL_SUBMODULE_DEL,
DIFF_SYMBOL_SUBMODULE_UNTRACKED,
@@ -648,6 +649,7 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
case DIFF_SYMBOL_SUBMODULE_ERROR:
case DIFF_SYMBOL_SUBMODULE_PIPETHROUGH:
case DIFF_SYMBOL_STATS_SUMMARY_INSERTS_DELETES:
+   case DIFF_SYMBOL_SUMMARY:
case DIFF_SYMBOL_STATS_LINE:
case DIFF_SYMBOL_BINARY_DIFF_BODY:
case DIFF_SYMBOL_CONTEXT_FRAGINFO:
@@ -4717,67 +4719,76 @@ static void flush_one_pair(struct diff_filepair *p, 
struct diff_options *opt)
}
 }
 
-static void show_file_mode_name(FILE *file, const char *newdelete, struct 
diff_filespec *fs)
+static void show_file_mode_name(struct diff_options *opt, const char 
*newdelete, struct diff_filespec *fs)
 {
+   struct strbuf sb = STRBUF_INIT;
if (fs->mode)
-   fprintf(file, " %s mode %06o ", newdelete, fs->mode);
+   strbuf_addf(&sb, " %s mode %06o ", newdelete, fs->mode);
else
-   fprintf(file, " %s ", newdelete);
-   write_name_quoted(fs->path, file, '\n');
-}
+   strbuf_addf(&sb, " %s ", newdelete);
 
+   quote_c_style(fs->path, &sb, NULL, 0);
+   strbuf_addch(&sb, '\n');
+   emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
+sb.buf, sb.len, 0);
+   strbuf_release(&sb);
+}
 
-static void show_mode_change(FILE *file, struct diff_filepair *p, int 
show_name,
-   const char *line_prefix)
+static void show_mode_change(struct diff_options *opt, struct diff_filepair *p,
+   int show_name)
 {
if (p->one->mode && p->two->mode && p->one->mode != p->two->mode) {
-   fprintf(file, "%s mode change %06o => %06o%c", line_prefix, 
p->one->mode,
-   p->two->mode, show_name ? ' ' : '\n');
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addf(&sb, " mode change %06o => %06o",
+   p->one->mode, p->two->mode);
if (show_name) {
-   write_name_quoted(p->two->path, file, '\n');
+   strbuf_addch(&sb, ' ');
+   quote_c_style(p->two->path, &sb, NULL, 0);
}
+   emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
+sb.buf, sb.len, 0);
+   strbuf_release(&sb);
}
 }
 
-static void show_rename_copy(FILE *file, const char *renamecopy, struct 
diff_filepair *p,
-   const char *line_prefix)
+static void show_rename_copy(struct diff_options *opt, const char *renamecopy,
+   struct diff_filepair *p)
 {
+   struct strbuf sb = STRBUF_INIT;
char *names = pprint_rename(p->one->path, p->two->path);
-
-   fprintf(file, " %s %s (%d%%)\n", renamecopy, names, 
similarity_index(p));
+   strbuf_addf(&sb, " %s %s (%d%%)\n",
+   renamecopy, names, similarity_index(p));
free(names);
-   show_mode_change(file, p, 0, line_prefix);
+   emit_diff_symbol(opt, DIFF_SYMBOL_SUMMARY,
+sb.buf, sb.len, 0);
+   show_mode_change(opt, p, 0);
 }
 
 static void diff_summary(struct diff_options *opt, struct diff_filepair *p)
 {
-   FILE *file = opt->file;
-   const char *line_prefix = diff_line_prefix(opt);
-
switch(p->status) {
case DIFF_STATUS_DELETED:
-   fputs(line_prefix, file);
-   show_file_mode_name(file, "delete", p->one);
+   show_file_mode_name(opt, "delete", p->one);
break;
case DIFF_STATUS_ADDED:
-   fputs(line_prefix, file);
-   show_file_mode_name(file, "create", p->two);
+   show_file_mode_name(opt, "create", p->two);
break;
case DIFF_STATUS_COPIED:
-   fputs(line_prefix, file);
-   show_rename_copy(file, "copy", p, line_prefix);
+   show_rename_copy(opt, "copy", p);
break;
case DIFF_STATUS_RENAMED:
-   fputs(line_prefix, file);
-   show_rename_copy(file, "rename", p, line_prefix);
+   show_rename_copy(opt, "rename", p);
break;
default:
if (p->score) {
-   fprintf(file, "%s rewrite ", line_prefix);
-   write_name_quoted(p->two->path, file, ' ');
-   fprintf(file, "(%d%%)\n", similarity_inde

[PATCH 22/25] diff.c: color moved lines differently

When a patch consists mostly of moving blocks of code around, it can
be quite tedious to ensure that the blocks are moved verbatim, and not
undesirably modified in the move. To that end, color blocks that are
moved within the same patch differently. For example (OM, del, add,
and NM are different colors):

[OM]  -void sensitive_stuff(void)
[OM]  -{
[OM]  -if (!is_authorized_user())
[OM]  -die("unauthorized");
[OM]  -sensitive_stuff(spanning,
[OM]  -multiple,
[OM]  -lines);
[OM]  -}

   void another_function()
   {
[del] -printf("foo");
[add] +printf("bar");
   }

[NM]  +void sensitive_stuff(void)
[NM]  +{
[NM]  +if (!is_authorized_user())
[NM]  +die("unauthorized");
[NM]  +sensitive_stuff(spanning,
[NM]  +multiple,
[NM]  +lines);
[NM]  +}

However adjacent blocks may be problematic. For example, in this
potentially malicious patch, the swapping of blocks can be spotted:

[OM]  -void sensitive_stuff(void)
[OM]  -{
[OMA] -if (!is_authorized_user())
[OMA] -die("unauthorized");
[OM]  -sensitive_stuff(spanning,
[OM]  -multiple,
[OM]  -lines);
[OMA] -}

   void another_function()
   {
[del] -printf("foo");
[add] +printf("bar");
   }

[NM]  +void sensitive_stuff(void)
[NM]  +{
[NMA] +sensitive_stuff(spanning,
[NMA] +multiple,
[NMA] +lines);
[NM]  +if (!is_authorized_user())
[NM]  +die("unauthorized");
[NMA] +}

If the moved code is larger, it is easier to hide some permutation in the
code, which is why some alternative coloring is needed.

This patch implements the first mode:
* basic alternating 'Zebra' mode
  This conveys all information needed to the user.  Defer customization to
  later patches.

First I implemented an alternative design, which would try to fingerprint
a line by its neighbors to detect if we are in a block or at the boundary.
This idea iss error prone as it inspected each line and its neighboring
lines to determine if the line was (a) moved and (b) if was deep inside
a hunk by having matching neighboring lines. This is unreliable as the
we can construct hunks which have equal neighbors that just exceed the
number of lines inspected. (Think of 'AXYZBXYZCXYZD..' with each letter
as a line, that is permutated to AXYZCXYZBXYZD..').

Instead this provides a dynamic programming greedy algorithm that finds
the largest moved hunk and then has several modes on highlighting bounds.

A note on the options '--submodule=diff' and '--color-words/--word-diff':
In the conversion to use emit_line in the prior patches both submodules
as well as word diff output carefully chose to call emit_line with sign=0.
All output with sign=0 is ignored for move detection purposes in this
patch, such that no weird looking output will be generated for these
cases. This leads to another thought: We could pass on '--color-moved' to
submodules such that they color up moved lines for themselves. If we'd do
so only line moves within a repository boundary are marked up.

It is useful to have moved lines colored, but there are annoying corner
cases, such as a single line moved, that is very common. For example
in a typical patch of C code, we have closing braces that end statement
blocks or functions.

While it is technically true that these lines are moved as they show up
elsewhere, it is harmful for the review as the reviewers attention is
drawn to such a minor side annoyance.

For now let's have a simple solution of hardcoding the number of
moved lines to be at least 3 before coloring them. Note, that the
length is applied across all blocks to find the 'lonely' blocks
that pollute new code, but do not interfere with a permutated
block where each permutation has less lines than 3.

Helped-by: Jonathan Tan 
Signed-off-by: Stefan Beller 
---
 diff.c | 341 +++--
 diff.h |  12 +-
 t/t4015-diff-whitespace.sh | 261 ++
 3 files changed, 599 insertions(+), 15 deletions(-)

diff --git a/diff.c b/diff.c
index 31720abf8f..322a2bfd72 100644
--- a/diff.c
+++ b/diff.c
@@ -16,6 +16,7 @@
 #include "userdiff.h"
 #include "submodule-config.h"
 #include "submodule.h"
+#include "hashmap.h"
 #include "ll-merge.h"
 #include "string-list.h"
 #include "argv-array.h"
@@ -32,6 +33,7 @@ static int diff_indent_heuristic = 1;
 static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
+static int diff_color_moved_default;
 static int diff_context_def

[PATCH 00/25] Reroll of sb/diff-color-moved.

Reroll of sb/diff-color-moved, applies on top of the hashmap patches[1].

* Supports different whitespace modes correctly now (and has a test for trivial
  white space changes)
* The fixups[2] have been squashed.

Thanks,
Stefan

[1] https://public-inbox.org/git/20170629235336.28460-1-sbel...@google.com/
[2] https://public-inbox.org/git/20170628005651.8110-1-sbel...@google.com/


Stefan Beller (25):
  diff.c: readability fix
  diff.c: move line ending check into emit_hunk_header
  diff.c: factor out diff_flush_patch_all_file_pairs
  diff.c: introduce emit_diff_symbol
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_MARKER
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_FRAGINFO
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_NO_LF_EOF
  diff.c: migrate emit_line_checked to use emit_diff_symbol
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_WORDS[_PORCELAIN]
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_CONTEXT_INCOMPLETE
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_FILEPAIR_{PLUS, MINUS}
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_HEADER
  diff.c: emit_diff_symbol learns about DIFF_SYMBOL_BINARY_FILES
  diff.c: emit_diff_symbol learns DIFF_SYMBOL_REWRITE_DIFF
  submodule.c: migrate diff output to use emit_diff_symbol
  diff.c: convert emit_binary_diff_body to use emit_diff_symbol
  diff.c: convert show_stats to use emit_diff_symbol
  diff.c: convert word diffing to use emit_diff_symbol
  diff.c: emit_diff_symbol learns about DIFF_SYMBOL_STAT_SEP
  diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY
  diff.c: buffer all output if asked to
  diff.c: color moved lines differently
  diff.c: color moved lines differently, plain mode
  diff.c: add dimming to moved line detection
  diff: document the new --color-moved setting

 Documentation/config.txt   |   15 +-
 Documentation/diff-options.txt |   34 ++
 cache.h|2 +
 color.h|2 +
 diff.c | 1306 
 diff.h |   39 +-
 submodule.c|   84 ++-
 submodule.h|   13 +-
 t/t4015-diff-whitespace.sh |  434 +
 9 files changed, 1613 insertions(+), 316 deletions(-)

-- 
2.13.0.31.g9b732c453e

[PATCH 09/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_WORDS[_PORCELAIN]

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 42 ++
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/diff.c b/diff.c
index e5430d56da..85bfd9310d 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,8 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_WORDS_PORCELAIN,
+   DIFF_SYMBOL_WORDS,
DIFF_SYMBOL_CONTEXT,
DIFF_SYMBOL_PLUS,
DIFF_SYMBOL_MINUS,
@@ -649,6 +651,26 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
emit_line_ws_markup(o, set, reset, line, len, '-',
flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
break;
+   case DIFF_SYMBOL_WORDS_PORCELAIN:
+   context = diff_get_color_opt(o, DIFF_CONTEXT);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line(o, context, reset, line, len);
+   fputs("~\n", o->file);
+   break;
+   case DIFF_SYMBOL_WORDS:
+   context = diff_get_color_opt(o, DIFF_CONTEXT);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   /*
+* Skip the prefix character, if any.  With
+* diff_suppress_blank_empty, there may be
+* none.
+*/
+   if (line[0] != '\n') {
+   line++;
+   len--;
+   }
+   emit_line(o, context, reset, line, len);
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -1342,7 +1364,6 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
 {
struct emit_callback *ecbdata = priv;
const char *meta = diff_get_color(ecbdata->color_diff, DIFF_METAINFO);
-   const char *context = diff_get_color(ecbdata->color_diff, DIFF_CONTEXT);
const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
struct diff_options *o = ecbdata->opt;
const char *line_prefix = diff_line_prefix(o);
@@ -1384,6 +1405,9 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
}
 
if (ecbdata->diff_words) {
+   enum diff_symbol s =
+   ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN ?
+   DIFF_SYMBOL_WORDS_PORCELAIN : DIFF_SYMBOL_WORDS;
if (line[0] == '-') {
diff_words_append(line, len,
  &ecbdata->diff_words->minus);
@@ -1403,21 +1427,7 @@ static void fn_out_consume(void *priv, char *line, 
unsigned long len)
return;
}
diff_words_flush(ecbdata);
-   if (ecbdata->diff_words->type == DIFF_WORDS_PORCELAIN) {
-   emit_line(o, context, reset, line, len);
-   fputs("~\n", o->file);
-   } else {
-   /*
-* Skip the prefix character, if any.  With
-* diff_suppress_blank_empty, there may be
-* none.
-*/
-   if (line[0] != '\n') {
- line++;
- len--;
-   }
-   emit_line(o, context, reset, line, len);
-   }
+   emit_diff_symbol(o, s, line, len, 0);
return;
}
 
-- 
2.13.0.31.g9b732c453e

[PATCH 07/25] diff.c: emit_diff_symbol learns DIFF_SYMBOL_NO_LF_EOF

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index 6e48a129ed..488096b757 100644
--- a/diff.c
+++ b/diff.c
@@ -561,6 +561,7 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_NO_LF_EOF,
DIFF_SYMBOL_CONTEXT_FRAGINFO,
DIFF_SYMBOL_CONTEXT_MARKER,
DIFF_SYMBOL_SEPARATOR
@@ -569,8 +570,16 @@ enum diff_symbol {
 static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
 const char *line, int len)
 {
+   static const char *nneof = " No newline at end of file\n";
const char *context, *reset;
switch (s) {
+   case DIFF_SYMBOL_NO_LF_EOF:
+   context = diff_get_color_opt(o, DIFF_CONTEXT);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   putc('\n', o->file);
+   emit_line_0(o, context, reset, '\\',
+   nneof, strlen(nneof));
+   break;
case DIFF_SYMBOL_CONTEXT_FRAGINFO:
emit_line(o, "", "", line, len);
break;
@@ -751,7 +760,6 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
   int prefix, const char *data, int size)
 {
const char *endp = NULL;
-   static const char *nneof = " No newline at end of file\n";
const char *reset = diff_get_color(ecb->color_diff, DIFF_RESET);
 
while (0 < size) {
@@ -769,13 +777,8 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
size -= len;
data += len;
}
-   if (!endp) {
-   const char *context = diff_get_color(ecb->color_diff,
-DIFF_CONTEXT);
-   putc('\n', ecb->opt->file);
-   emit_line_0(ecb->opt, context, reset, '\\',
-   nneof, strlen(nneof));
-   }
+   if (!endp)
+   emit_diff_symbol(ecb->opt, DIFF_SYMBOL_NO_LF_EOF, NULL, 0);
 }
 
 static void emit_rewrite_diff(const char *name_a,
-- 
2.13.0.31.g9b732c453e

[PATCH 21/25] diff.c: buffer all output if asked to

Introduce a new option 'emitted_symbols' in the struct diff_options which
controls whether all output is buffered up until all output is available.
It is set internally in diff.c when necessary.

We'll have a new struct 'emitted_string' in diff.c which will be used to
buffer each line.  The emitted_string will duplicate the memory of the
line to buffer as that is easiest to reason about for now. In a future
patch we may want to decrease the memory usage by not duplicating all
output for buffering but rather we may want to store offsets into the
file or in case of hunk descriptions such as the similarity score, we
could just store the relevant number and reproduce the text later on.

This approach was chosen as a first step because it is quite simple
compared to the alternative with less memory footprint.

emit_diff_symbol factors out the emission part and depending on the
diff_options->emitted_symbols the emission will be performed directly
when calling emit_diff_symbol or after the whole process is done, i.e.
by buffering we have add the possibility for a second pass over the
whole output before doing the actual output.

In 6440d34 (2012-03-14, diff: tweak a _copy_ of diff_options with
word-diff) we introduced a duplicate diff options struct for word
emissions as we may have different regex settings in there.
When buffering the output, we need to operate on just one buffer,
so we have to copy back the emissions of the word buffer into the
main buffer.

Unconditionally enable output via buffer in this patch as it yields
a great opportunity for testing, i.e. all the diff tests from the
test suite pass without having reordering issues (i.e. only parts
of the output got buffered, and we forgot to buffer other parts).
The test suite passes, which gives confidence that we converted all
functions to use emit_string for output.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 diff.c | 109 +++--
 diff.h |   2 ++
 2 files changed, 109 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index 2db0d7c0f5..31720abf8f 100644
--- a/diff.c
+++ b/diff.c
@@ -605,6 +605,47 @@ enum diff_symbol {
 #define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<16)
 #define DIFF_SYMBOL_CONTENT_WS_MASK (WSEH_NEW | WSEH_OLD | WSEH_CONTEXT | 
WS_RULE_MASK)
 
+/*
+ * This struct is used when we need to buffer the output of the diff output.
+ *
+ * NEEDSWORK: Instead of storing a copy of the line, add an offset pointer
+ * into the pre/post image file. This pointer could be a union with the
+ * line pointer. By storing an offset into the file instead of the literal 
line,
+ * we can decrease the memory footprint for the buffered output. At first we
+ * may want to only have indirection for the content lines, but we could also
+ * enhance the state for emitting prefabricated lines, e.g. the similarity
+ * score line or hunk/file headers would only need to store a number or path
+ * and then the output can be constructed later on depending on state.
+ */
+struct emitted_diff_symbol {
+   const char *line;
+   int len;
+   int flags;
+   enum diff_symbol s;
+};
+#define EMITTED_DIFF_SYMBOL_INIT {NULL}
+
+struct emitted_diff_symbols {
+   struct emitted_diff_symbol *buf;
+   int nr, alloc;
+};
+#define EMITTED_DIFF_SYMBOLS_INIT {NULL, 0, 0}
+
+static void append_emitted_diff_symbol(struct diff_options *o,
+  struct emitted_diff_symbol *e)
+{
+   struct emitted_diff_symbol *f;
+
+   ALLOC_GROW(o->emitted_symbols->buf,
+  o->emitted_symbols->nr + 1,
+  o->emitted_symbols->alloc);
+   f = &o->emitted_symbols->buf[o->emitted_symbols->nr++];
+
+   memcpy(f, e, sizeof(struct emitted_diff_symbol));
+   f->line = e->line ? xmemdupz(e->line, e->len) : NULL;
+}
+
+
 static void emit_line_ws_markup(struct diff_options *o,
const char *set, const char *reset,
const char *line, int len, char sign,
@@ -631,12 +672,18 @@ static void emit_line_ws_markup(struct diff_options *o,
}
 }
 
-static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
-const char *line, int len, unsigned flags)
+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;
struct strbuf sb = STRBUF_INIT;
+
+   enum diff_symbol s = eds->s;
+   const char *line = eds->line;
+   int len = eds->len;
+   unsigned flags = eds->flags;
+
switch (s) {
case DIFF_SYMBOL_NO_LF_EOF:
context = diff_get_color_opt(o, DIFF_CONTEXT);
@@ -778,6 +825,17 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
strbuf_relea

[PATCH 08/25] diff.c: migrate emit_line_checked to use emit_diff_symbol

Add a new flags field to emit_diff_symbol, that will be used by
context lines for:
* white space rules that are applicable (The first 12 bits)
  Take a note in cahe.c as well, when this ws rules are extended we have
  to fix the bits in the flags field.
* how the rules are evaluated (actually this double encodes the sign
  of the line, but the code is easier to keep this way, bits 13,14,15)
* if the line a blank line at EOF (bit 16)

The check if new lines need to be marked up as extra lines at the end of
file, is now done unconditionally. That should be ok, as
'new_blank_line_at_eof' has a quick early return.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 cache.h |   2 ++
 diff.c  | 116 +---
 diff.h  |   6 ++--
 3 files changed, 80 insertions(+), 44 deletions(-)

diff --git a/cache.h b/cache.h
index 96055c2229..18aabacba5 100644
--- a/cache.h
+++ b/cache.h
@@ -1996,6 +1996,8 @@ void shift_tree_by(const struct object_id *, const struct 
object_id *, struct ob
 #define WS_TRAILING_SPACE  (WS_BLANK_AT_EOL|WS_BLANK_AT_EOF)
 #define WS_DEFAULT_RULE (WS_TRAILING_SPACE|WS_SPACE_BEFORE_TAB|8)
 #define WS_TAB_WIDTH_MASK077
+/* All WS_* -- when extended, adapt diff.c emit_symbol */
+#define WS_RULE_MASK   0
 extern unsigned whitespace_rule_cfg;
 extern unsigned whitespace_rule(const char *);
 extern unsigned parse_whitespace_rule(const char *);
diff --git a/diff.c b/diff.c
index 488096b757..e5430d56da 100644
--- a/diff.c
+++ b/diff.c
@@ -561,17 +561,54 @@ static void emit_line(struct diff_options *o, const char 
*set, const char *reset
 }
 
 enum diff_symbol {
+   DIFF_SYMBOL_CONTEXT,
+   DIFF_SYMBOL_PLUS,
+   DIFF_SYMBOL_MINUS,
DIFF_SYMBOL_NO_LF_EOF,
DIFF_SYMBOL_CONTEXT_FRAGINFO,
DIFF_SYMBOL_CONTEXT_MARKER,
DIFF_SYMBOL_SEPARATOR
 };
+/*
+ * Flags for content lines:
+ * 0..12 are whitespace rules
+ * 13-15 are WSEH_NEW | WSEH_OLD | WSEH_CONTEXT
+ * 16 is marking if the line is blank at EOF
+ */
+#define DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF (1<<16)
+#define DIFF_SYMBOL_CONTENT_WS_MASK (WSEH_NEW | WSEH_OLD | WSEH_CONTEXT | 
WS_RULE_MASK)
+
+static void emit_line_ws_markup(struct diff_options *o,
+   const char *set, const char *reset,
+   const char *line, int len, char sign,
+   unsigned ws_rule, int blank_at_eof)
+{
+   const char *ws = NULL;
+
+   if (o->ws_error_highlight & ws_rule) {
+   ws = diff_get_color_opt(o, DIFF_WHITESPACE);
+   if (!*ws)
+   ws = NULL;
+   }
+
+   if (!ws)
+   emit_line_0(o, set, reset, sign, 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);
+   ws_check_emit(line, len, ws_rule,
+ o->file, set, reset, ws);
+   }
+}
 
 static void emit_diff_symbol(struct diff_options *o, enum diff_symbol s,
-const char *line, int len)
+const char *line, int len, unsigned flags)
 {
static const char *nneof = " No newline at end of file\n";
-   const char *context, *reset;
+   const char *context, *reset, *set;
switch (s) {
case DIFF_SYMBOL_NO_LF_EOF:
context = diff_get_color_opt(o, DIFF_CONTEXT);
@@ -593,6 +630,25 @@ static void emit_diff_symbol(struct diff_options *o, enum 
diff_symbol s,
diff_line_prefix(o),
o->line_termination);
break;
+   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, ' ',
+   flags & (DIFF_SYMBOL_CONTENT_WS_MASK), 0);
+   break;
+   case DIFF_SYMBOL_PLUS:
+   set = diff_get_color_opt(o, DIFF_FILE_NEW);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line_ws_markup(o, set, reset, line, len, '+',
+   flags & DIFF_SYMBOL_CONTENT_WS_MASK,
+   flags & DIFF_SYMBOL_CONTENT_BLANK_LINE_EOF);
+   break;
+   case DIFF_SYMBOL_MINUS:
+   set = diff_get_color_opt(o, DIFF_FILE_OLD);
+   reset = diff_get_color_opt(o, DIFF_RESET);
+   emit_line_ws_markup(o, set, reset, line, len, '-',
+   flags & DIFF_SYMBOL_CONTENT_WS_MASK, 0);
+   break;
default:
die("BUG: unknown diff symbol");
}
@@ -609,57 +665,31 @@ static int new_

[PATCH] recursive submodules: detach HEAD from new state, keeping branches sane

When a submodule is on a branch and in its superproject you run a recursive
checkout, the branch of the submodule is updated to what the superproject
checks out. This is very unexpected. Instead detach the HEAD when updating
it.

Signed-off-by: Stefan Beller 
---
 submodule.c   |  3 ++-
 t/lib-submodule-update.sh | 17 +
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index da0b805493..719e8bd7a2 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1604,7 +1604,8 @@ int submodule_move_head(const char *path,
cp.dir = path;
 
prepare_submodule_repo_env(&cp.env_array);
-   argv_array_pushl(&cp.args, "update-ref", "HEAD", new, 
NULL);
+   argv_array_pushl(&cp.args, "update-ref", "HEAD",
+"--no-deref", new, NULL);
 
if (run_command(&cp)) {
ret = -1;
diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 2d26f86800..fc406b95d7 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -848,6 +848,23 @@ test_submodule_switch_recursing_with_args () {
test_submodule_content sub1 origin/add_sub1
)
'
+   test_expect_success "$command: submodule branch is not changed, detach 
HEAD instead" '
+   prolog &&
+   reset_work_tree_to_interested add_sub1 &&
+   (
+   cd submodule_update &&
+   git -C sub1 checkout -b keep_branch &&
+   git -C sub1 rev-parse HEAD >expect &&
+   git branch -t check-keep origin/modify_sub1 &&
+   $command check-keep &&
+   test_superproject_content origin/modify_sub1 &&
+   test_submodule_content sub1 origin/modify_sub1 &&
+   git -C sub1 rev-parse keep_branch >actual &&
+   test_cmp expect actual &&
+   test_must_fail git -C sub1 symbolic-ref HEAD
+   )
+   '
+
# Replacing a tracked file with a submodule produces a checked out 
submodule
test_expect_success "$command: replace tracked file with submodule 
checks out submodule" '
prolog &&
-- 
2.13.0.31.g9b732c453e

git-gui: Error on restoring defaults

2017-06-29 Thread Samuel Leslie

Hi there,

When selecting “Restore Defaults” in the Options dialogue via the Edit -> 
Options menu the following application error is received:

window name "!paving" already exists in parent
    while executing
"ttk::frame $w.!paving"
    (procedure "pave_toplevel" line 4)
    invoked from within
"pave_toplevel ."
    (procedure "apply_config" line 37)
    invoked from within
"apply_config"
    (procedure "do_restore_defaults" line 14)
    invoked from within
"do_restore_defaults"
    invoked from within
".options_editor.buttons.restore invoke "
    invoked from within
".options_editor.buttons.restore instate !disabled { 
.options_editor.buttons.restore invoke } "
    invoked from within
".options_editor.buttons.restore instate pressed { 
.options_editor.buttons.restore state !pressed; .options_editor.buttons.restore 
instate !disabled { ..."
    (command bound to event)

Tested using Git Gui 0.21 as included with Git 2.13.2 x64 on Windows x64. Also 
tested on a Windows VM with a clean install of Git.


Kind regards,
Samuel Leslie

Re: [PATCH 1/2] commit-template: remove outdated notice about explicit paths

On Thu, 2017-06-29 at 10:56 -0700, Junio C Hamano wrote:
> When I said "I would have ... if I were doing this", I merely meant
> exactly that---as I weren't doing it, I left it up to you.  But you
> did it the way anyways, which is very nice of you ;-).
> 
It made a *lot* of sense, that's why. :)

> Looks good.  Should we consider this signed-off by you?
> 
Yeah, forgot that. Will follow-up with a patch.

Re: [PATCH 2/2] commit-template: add new line before status information

On Thu, 2017-06-29 at 11:17 -0700, Junio C Hamano wrote:
> The rationale of this has changed in this final version, hasn't it,
> especially with the removal of the "include/only warning" bit?
> 
> We used to add a blank line to separate the "we are committing for
> somebody else", which is an optional part, from the status output,
> only when we have the optional part.  Now with your change, we
> unconditionally have a blank before the part that would have been
> shown by "git status" to separate the two parts out.
> 
Yes, modified the commit message accordingly hoping that's what you
meant.

Re: git-gui: Error on restoring defaults