I need you help pls
HELP ME PLEASE My name is Miss Victoria Jenny looking for a trustworthy, sincere and honest someone like you who can help me in this time of needs. My father is Rufus Okana, during the civil and political crisis in our country, my parents together with my three sisters was poisoned by heartless elements that called themselves his brother and wicked relatives. Fortunately for me, I was in the school when this tragedy took place to my family. I was in coma for almost two weeks. But I thank the almighty God because I never knew that I could support the shock of losing almost my whole family. That is by the way. Right now I am still here in Cote d'Ivoire with but very unsafe for me. I'm living in great fear and bondage. I intend leaving this country as soon as possible but only one thing kept me back. My late father has deposited with one of the prime institution in Europe the sum of money, $5.5Million USD, for onward transfer to any bank abroad But unfortunately he did not complete the transaction before he died. I have all the documents concerns this money in the security company in Holland and receipt of deposit with which my late father made the deposit, I have mapped out 25% out of the total money for your help and assistances because it looks stupid for me trying to confide in a total stranger I never met before . By instinct I am convinced you are an honest person and you have the capacity to handle this transaction with me. As soon as it is done, I will come over to meet you and spend the rest of my live in your country. I wish to invest the money into estate business and other good business you may propose. I promise to greatly compensate you for any assistance you may offer me. I do not know how you may feel about this but I want you to take this very serious and confidential. Down here, I am living in fear because enemies, uncles and wicked relatives of my parents are hunting for us. Please let me know your mind concerning my proposal to you, please contact me with this email With her loving arms Miss Victoria
[no subject]
Apply for a loan at 3% reply to this Email for more Info
Warning suggestion for git stash drop
Hi all, 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. Thank you for your work on this amazing little software, All the best, Laurent
Re: [GSoC][PATCH 1/6 v2] submodule--helper: introduce for_each_submodule_list
>> This series of patches is based on the 'next' branch. > > The reason not to base on 'master' is...? > The reason it wasn't based on 'master' was that it depended on the commit: dir: create function count_slashes(), which was merged to 'next' a few days back. > The thing is that a topic built on 'next' cannot be merged down to > 'master' until _all_ other topics in 'next' graduate to 'master', > which may never happen. If you are depending on one or more topics, > please make sure to name them. Then we can > > (1) create a branch from the tip of 'master'; > (2) merge these topics you depend on into that branch; and then > (3) apply these patches. > I'll soon be updating this patch series and will create the new patch series in accordance with the above routine. > The topic still needs to wait until these other topis graduate, but > at least you would not be blocked by unrelated topics that way. > > You _might_ be building on 'next' because you want to make sure that > your topic works not just with master but also want to make sure > that there won't be any unexpected breakage when used with topics in > 'next', even though your topic does not depend on anything in 'next' > in particular. It is a good development discipline to pay attention > to other topics in flight and I applaud you for it if that is why > you based it on 'next'. But the right way to do it would be to > build your topic on 'master', and then in addition to testing the > topic by itself, also make a trial merge of your topic into 'next' > and test the result as well. > Thanks for making me aware about this as well. And will be following this before sending out the updated patch-series. Thanks, Prathamesh Chavan
Re: [PATCH 2/2] hashmap: migrate documentation from Documentation/technical into header
On 6/28/2017 9:13 PM, Stefan Beller wrote: 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 > ... This looks very nice. Thank you! Jeff
[PATCH 1/2] commit-template: remove outdated notice about explicit paths
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
> 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
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
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
> 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
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
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
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
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
İ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
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
+cc Pat who maintains git-gui On Thu, Jun 29, 2017 at 6:13 PM, Samuel Leslie wrote: > 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 >
Bug with automated processing of git status results
Hi! I am trying to make an automated processing of "git status" results. I execute the command git status -z -uno I expect that it has stable output format. However, it still can print warnings like warning: CRLF will be replaced by LF in somefile.xml I understand that we can turn off core.safecrlf, but it's inconvinient. It would be better if "git status" command had an optional parameter that disables any other output besides changed files. Thanks! Sergey Shestakov Playrix