Re: [PATCH v2 18/29] grep: catch a missing enum in switch statement

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> Add a die(...) to a default case for the switch statement selecting
> between grep pattern types under --recurse-submodules.
>
> Normally this would be caught by -Wswitch, but the grep_pattern_type
> type is converted to int by going through parse_options(). Changing
> the argument type passed to compile_submodule_options() won't work,
> the value will just get coerced. The -Wswitch-default warning will
> warn about it, but that produces a lot of noise across the codebase,
> this potential issue would be drowned in that noise.
>
> Thus catching this at runtime is the least worst option. This won't

least bad, I guess?

> ever trigger in practice, but if a new pattern type were to be added
> this catches an otherwise silent bug during development.

Good future-proofing.  When this and Peff's "BUG" thing both
graduates, we can turn this into a BUG, I think.

Thanks.

>
> See commit 0281e487fd ("grep: optionally recurse into submodules",
> 2016-12-16) for the initial addition of this code.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>  builtin/grep.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/builtin/grep.c b/builtin/grep.c
> index 3ffb5b4e81..a191e2976b 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -495,6 +495,8 @@ static void compile_submodule_options(const struct 
> grep_opt *opt,
>   break;
>   case GREP_PATTERN_TYPE_UNSPECIFIED:
>   break;
> + default:
> + die("BUG: Added a new grep pattern type without updating switch 
> statement");
>   }
>  
>   for (pattern = opt->pattern_list; pattern != NULL;

Re: [PATCH v2 04/29] log: add exhaustive tests for pattern style options & config

[Junio C Hamano]

Ævar Arnfjörð Bjarmason   writes:

> + echo 2e >expect &&
> + # In PCRE \d in [\d] is like saying "0-9", and matches the 2
> + # in 2e...
> + git -C num_commits log -1 --pretty="tformat:%s" -F -E --perl-regexp 
> --grep="[\d]" >actual &&
> + test_cmp expect actual &&
> + echo 1d >expect &&
> + # ...in POSIX basic & extended it is the same as [d],
> + # i.e. "d", which matches 1d, but not and does not match 2e.

s/not and//; I think.

> + git -C num_commits log -1 --pretty="tformat:%s" -F -E --grep="[\d]" 
> >actual &&
>   test_cmp expect actual
>  '
>  
> @@ -280,6 +301,77 @@ test_expect_success 'log with grep.patternType 
> configuration and command line' '
>   test_cmp expect actual
>  '
>  
> +test_expect_success 'log with various grep.patternType configurations & 
> command-lines' '
> + git init pattern-type &&
> + (
> + cd pattern-type &&
> + test_commit 1 file A &&
> +
> + # The tagname is overridden here because creating a
> + # tag called "(1|2)" as test_commit would otherwise
> + # implicitly do would fail on e.g. MINGW.

Thanks.

> + # POSIX extended needs to have | escaped to match it
> + # literally, whereas under basic this is the same as
> + # (|2), i.e. it would also match "1". This test checks
> + # for extended by asserting that it is not matching
> + # what basic would match.
> + git -c grep.patternType=extended log --pretty=tformat:%s \
> + --grep="\|2" >actual.extended &&

Makes sense.

> + if test_have_prereq PCRE
> + then
> + # Only PCRE would match [\d]\| with only
> + # "(1|2)" due to [\d]. POSIX basic would match
> + # both it and "1", and POSIX extended would
> + # match neither.

OK.  BRE would match because the other side of "\|" is empty to
match anything?

> + git -c grep.patternType=perl log --pretty=tformat:%s \
> + --grep="[\d]\|" >actual.perl &&
> + test_cmp expect.perl actual.perl
> + fi &&

Re: [PATCH] interpret-trailers: obey scissors lines

[Junio C Hamano]

Junio C Hamano  writes:

> This can be done by the same logic as the existing helper
> function, wt_status_truncate_message_at_cut_line(), uses,
> but it wants the caller to pass a strbuf to it.  Because the
> helper function ignore_non_trailer() used by the command
> takes a  pair, not a strbuf, steal the
> logic from wt_status_truncate_message_at_cut_line() to
> create a new wt_status_strip_scissors() helper function that
> takes  pair, and make ignore_non_trailer()
> call it to help "interpret-trailers".  Since there is only
> one caller of wt_status_truncate_message_at_cut_line() in
> cmd_commit(), rewrite it to call wt_status_strip_scissors()
> helper instead and remove the old helper that no longer has
> any caller.
>
> The last paragraph would have saved me from getting puzzled.

And re-reading the above, wt_status_strip_scissors() does not sound
right to me.  

Yes, I am bikeshedding at this point, but names matter.  I prefer to
keep the distinction between the two clear by not calling the
cut_line[] array "scissors", but in any case, the new function is
not about "finding" the cut-line or scissors, in other words,
wt_status_locate_scissors() is not a good name either (we do not say
"not found" when there is none).

We are locating the logical end of the commit log message.  It ends
at "---\n", if exists, in the output of "git format-patch", and it
ends at the cut-line, if exists, in "commit -v" editor buffer, and
if there aren't these funny End-Of-Message marks, then we return the
location of the last byte.  IOW, the helper function you added will
be the place to add more logic in the future if we ever found the
need to notice other kinds of "logical end" markers (which may be
ones we will invent in the future) while accepting a proposed commit
log message.

So how about calling it wt_status_locate_end() or something like
that, without limiting ourselves only to the "scissors"?

Re: [PATCH v7 04/10] convert: move packet_write_line() into pkt-line as packet_writel()

[Junio C Hamano]

Jeff King  writes:

> This isn't a new problem, but I noticed that this function should
> probably get annotated to describe its interface.
>
> Junio, can you pick up the patch below on top of Ben's series (or I'd be
> fine if it were squashed into this patch)?

Surely.  Thanks for a careful review.

>
> -- >8 --
> Subject: [PATCH] pkt-line: annotate packet_writel with LAST_ARG_MUST_BE_NULL
>
> packet_writel() takes a variable-sized list and reads to
> the first NULL. Let's let the compiler know so that it can
> help us catch mistakes in the callers.
>
> This should have been annotated similarly when it was a
> static function, but it's doubly important now that the
> function is available to the whole code-base.
>
> Signed-off-by: Jeff King 
> ---
>  pkt-line.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/pkt-line.h b/pkt-line.h
> index b2965869a..450183b64 100644
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -25,6 +25,7 @@ void packet_buf_flush(struct strbuf *buf);
>  void packet_buf_write(struct strbuf *buf, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
>  int packet_flush_gently(int fd);
>  int packet_write_fmt_gently(int fd, const char *fmt, ...) 
> __attribute__((format (printf, 2, 3)));
> +LAST_ARG_MUST_BE_NULL
>  int packet_writel(int fd, const char *line, ...);
>  int write_packetized_from_fd(int fd_in, int fd_out);
>  int write_packetized_from_buf(const char *src_in, size_t len, int fd_out);

Re: [PATCH v4] send-email: --batch-size to work around some SMTP server limit

[Junio C Hamano]

xiaoqiang zhao  writes:

> Some email servers (e.g. smtp.163.com) limit the number emails to be
> sent per session(connection) and this will lead to a faliure when
> sending many messages.
>
> Teach send-email to disconnect after sending a number of messages
> (configurable via the --batch-size= option), wait for a few
> seconds (configurable via the --relogin-delay= option) and
> reconnect, to work around such a limit.
>
> Also add this two configure option for git config command.

s/configure/configuration/; "for git config command" is better left
unsaid (too obvious).

> Note:
>Re-authentication will happen every $ messages, so it
> will be much more acceptable if you use some form of credential helper
> (e.g. the 'sendemail.smtppass' config option), otherwise you will have
> to retype password every time when asked.

I think this deserves to be in the end-user documentation (i.e. the
part of your patch that updates Documentation/git-send-email.txt).

Other than that, looking good ;-)

Thanks.

> Signed-off-by: xiaoqiang zhao 
> ---
>  contrib/completion/git-completion.bash |  2 ++
>  git-send-email.perl| 18 ++
>  2 files changed, 20 insertions(+)

Re: [PATCH] interpret-trailers: obey scissors lines

[Junio C Hamano]

Brian Malehorn  writes:

> If a commit message is being edited as "verbose", it will contain a
> scissors string ("-- >8 --") and a diff:
>
> my subject
>
> #  >8 
> # Do not touch the line above.
> # Everything below will be removed.
> diff --git a/foo.txt b/foo.txt
> index 5716ca5..7601807 100644
> --- a/foo.txt
> +++ b/foo.txt
> @@ -1 +1 @@
> -bar
> +baz
>
> interpret-trailers doesn't interpret the scissors and therefore places
> trailer information after the diff. A simple reproduction is:
>
> git config commit.verbose true
> GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
> git commit --amend
>
> This commit resolves the issue by teaching "git interpret-trailers" to
> obey scissors the same way "git commit" does.

I'd prefer to see s/obey scissors lines/honor the cut line/, and the
last paragraph rephrased somewhat, perhaps like:

Subject: interpret-trailers: honor the cut line

If a commit message is edited with the "verbose" option, the
buffer will have a cut line and diff after the log message,
like so:

 my subject

 #  >8 
 # Do not touch the line above.
 # Everything below will be removed.
 diff --git a/foo.txt b/foo.txt
 index 5716ca5..7601807 100644
 --- a/foo.txt
 +++ b/foo.txt
 @@ -1 +1 @@
 -bar
 +baz

"git interpret-trailers" is unaware of the cut line, and
assumes the trailer block would be at the end of the whole
thing.  This can easily be seen with:

 $ GIT_EDITOR='git interpret-trailers --in-place --trailer 
Acked-by:me' \
   git commit --amend -v

Teach "git interpret-trailers" to notice the cut-line and
ignore the remainder of the input when looking for a place
to add new trailer block.  This makes it consistent with how
"git commit -v -s" inserts a new Signed-off-by: line.

This can be done by the same logic as the existing helper
function, wt_status_truncate_message_at_cut_line(), uses,
but it wants the caller to pass a strbuf to it.  Because the
helper function ignore_non_trailer() used by the command
takes a  pair, not a strbuf, steal the
logic from wt_status_truncate_message_at_cut_line() to
create a new wt_status_strip_scissors() helper function that
takes  pair, and make ignore_non_trailer()
call it to help "interpret-trailers".  Since there is only
one caller of wt_status_truncate_message_at_cut_line() in
cmd_commit(), rewrite it to call wt_status_strip_scissors()
helper instead and remove the old helper that no longer has
any caller.

The last paragraph would have saved me from getting puzzled.

The mention of "'commit -v -s' works that way, too" was my attempt
to justify why it is OK to make this change unconditionally to
intepret-trailers, but I am still not 100% convinced with that
reasoning (or your original log message) that it is a safe thing to
do.  It is not like its sole purpose is to serve as GIT_EDITOR for
the "git commit" command.

The patch text itself looks sensible.  

We still need to see your patch with your sign-off, though.

Thanks.

Re: [PATCH v3] builtin/log: honor log.decorate

[Alex Henrie]

2017-05-14 20:35 GMT-06:00 Junio C Hamano :
> "brian m. carlson"  writes:
>
>> The recent change that introduced autodecorating of refs accidentally
>> broke the ability of users to set log.decorate = false to override it.
>> When the git_log_config was traversed a second time with an option other
>> than log.decorate, the decoration style would be set to the automatic
>> style, even if the user had already overridden it.  Instead of setting
>> the option in config parsing, set it in init_log_defaults instead.
>>
>> Add a test for this case.  The actual additional config option doesn't
>> matter, but it needs to be something not already set in the
>> configuration file.
>>
>> Signed-off-by: brian m. carlson 
>> ---
>> Changes from v2:
>> * Add a test.  I tested that the config parsing both works with
>>   additional options and also can be overridden from the command line.
>
> Thanks, all.
>
> Will queue with Acked-by by Alex and Reviewed-by by Jonathan.

That sounds great, thank you.

-Alex

Re: [PATCH] interpret-trailers: obey scissors lines

[Jeff King]

On Mon, May 15, 2017 at 12:32:24PM +0900, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:
> >
> >> >> diff --git a/builtin/commit.c b/builtin/commit.c
> >> >> index 2de5f6cc6..2ce9c339d 100644
> >> >> --- a/builtin/commit.c
> >> >> +++ b/builtin/commit.c
> >> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const 
> >> >> char *prefix)
> >> >>  
> >> >> if (verbose || /* Truncate the message just before the diff, if 
> >> >> any. */
> >> >> cleanup_mode == CLEANUP_SCISSORS)
> >> >> -   wt_status_truncate_message_at_cut_line();
> >> >> +   strbuf_setlen(,
> >> >> + wt_status_last_nonscissors_index(sb.buf, 
> >> >> sb.len));
> >> >
> >> > This hunk surprised me at first (that we would need to touch commit.c at
> >> > all), but the refactoring makes sense.
> >> 
> >> This still surprises me.  If the problem is in interpret-trailers,
> >> why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us
> >
> > The behavior of cmd_commit() shouldn't be changed by the patch. But to
> > make the interface suitable for the new callsite (which doesn't have a
> > strbuf, but a ptr/len buffer), it needs to return the length rather than
> > shortening the strbuf. We could leave in place:
> >
> >   void wt_status_truncate_message_at_cut_line(struct strbuf *sb)
> >   {
> > strbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));
> >   }
> >
> > but it would only have this one caller.
> >
> > If I were doing the patch series, I'd probably have split that
> > refactoring into its own patch and discussed the reason separately. I
> > waffled on whether or not to ask Brian to do so (and obviously didn't in
> > the end).
> 
> I suspect that you would have just explained "since there is only
> one caller, let's open-code it" in the log message, without making
> this a two-patch series, and that would also have been perfectly
> understandable (and in this case probably preferrable).
> 
> So the patch text would be OK; it was surprising to have the change
> without being explained, that's all.

Yeah, I'd agree it could use a better explanation in the commit message.
I was surprised to see it, too. Since both of us were surprised, that's
probably a good indicator. :)

-Peff

Re: [PATCH] interpret-trailers: obey scissors lines

[Junio C Hamano]

Jeff King  writes:

> On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:
>
>> >> diff --git a/builtin/commit.c b/builtin/commit.c
>> >> index 2de5f6cc6..2ce9c339d 100644
>> >> --- a/builtin/commit.c
>> >> +++ b/builtin/commit.c
>> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const 
>> >> char *prefix)
>> >>  
>> >>   if (verbose || /* Truncate the message just before the diff, if any. */
>> >>   cleanup_mode == CLEANUP_SCISSORS)
>> >> - wt_status_truncate_message_at_cut_line();
>> >> + strbuf_setlen(,
>> >> +   wt_status_last_nonscissors_index(sb.buf, sb.len));
>> >
>> > This hunk surprised me at first (that we would need to touch commit.c at
>> > all), but the refactoring makes sense.
>> 
>> This still surprises me.  If the problem is in interpret-trailers,
>> why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us
>
> The behavior of cmd_commit() shouldn't be changed by the patch. But to
> make the interface suitable for the new callsite (which doesn't have a
> strbuf, but a ptr/len buffer), it needs to return the length rather than
> shortening the strbuf. We could leave in place:
>
>   void wt_status_truncate_message_at_cut_line(struct strbuf *sb)
>   {
>   strbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));
>   }
>
> but it would only have this one caller.
>
> If I were doing the patch series, I'd probably have split that
> refactoring into its own patch and discussed the reason separately. I
> waffled on whether or not to ask Brian to do so (and obviously didn't in
> the end).

I suspect that you would have just explained "since there is only
one caller, let's open-code it" in the log message, without making
this a two-patch series, and that would also have been perfectly
understandable (and in this case probably preferrable).

So the patch text would be OK; it was surprising to have the change
without being explained, that's all.

Thanks.

Re: [PATCH] config: complain about --local outside of a git repo

[Jeff King]

On Sun, May 14, 2017 at 08:31:54PM -0400, Josh Hagins wrote:

> For context, the "user.name" bit was purely notional; I just wanted to
> give a sample reproduction. Where I've actually been running into this
> in real life is with oh-my-git, a GitHub-themed Powerline bash prompt:
> https://github.com/arialdomartini/oh-my-git/blob/42c11f08540949b75bd513e6880a4ce6824a2bcc/base.sh#L52.
> 
> Since this command is invoked every time the prompt is build, I see
> the error message after every single command I run outside of a Git
> repository. While this is more indicative of a code smell in
> oh-my-git, I figured that, as you said, BUG assertions should never
> get hit in the wild. Thanks for the patch!

Ah, thanks for the context. That makes a lot more sense. I do think
oh-my-git should just drop the "--local", which will cause git-config to
follow the usual precedence rules (and it will skip repo-level config
automatically when we are not in a repo).

Since the oh-my-git code only looks for "false", nobody would ever need
to default the value to true in their ~/.gitconfig. But respecting the
usual config rules would mean you could set it to "false" in
~/.gitconfig to turn it off everywhere, but then opt back into it for
specific repos by setting it to "true" there. Probably not a use case
anybody cares that much about, but it's how "git config" was meant to be
used. :)

Anyway, thanks again for reporting the BUG assertion. I'm glad to be
weeding out more cases.

-Peff

Re: [PATCH] interpret-trailers: obey scissors lines

[Jeff King]

On Sun, May 14, 2017 at 01:33:48AM -0700, Brian Malehorn wrote:

> > And two, we usually indent the contents to the same level as the outer
> > cat/EOF pair
> 
> Fixed.
> 
> I was indenting the same as the other tests in that file. But if the way
> you described is the preferred way, then sure.

Doh. I didn't notice that. That file _is_ unlike the rest of the code
base, but it's generally OK to match the style of surrounding code.

-Peff

Re: [PATCH] interpret-trailers: obey scissors lines

[Jeff King]

On Mon, May 15, 2017 at 11:12:03AM +0900, Junio C Hamano wrote:

> >> diff --git a/builtin/commit.c b/builtin/commit.c
> >> index 2de5f6cc6..2ce9c339d 100644
> >> --- a/builtin/commit.c
> >> +++ b/builtin/commit.c
> >> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const 
> >> char *prefix)
> >>  
> >>if (verbose || /* Truncate the message just before the diff, if any. */
> >>cleanup_mode == CLEANUP_SCISSORS)
> >> -  wt_status_truncate_message_at_cut_line();
> >> +  strbuf_setlen(,
> >> +wt_status_last_nonscissors_index(sb.buf, sb.len));
> >
> > This hunk surprised me at first (that we would need to touch commit.c at
> > all), but the refactoring makes sense.
> 
> This still surprises me.  If the problem is in interpret-trailers,
> why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us

The behavior of cmd_commit() shouldn't be changed by the patch. But to
make the interface suitable for the new callsite (which doesn't have a
strbuf, but a ptr/len buffer), it needs to return the length rather than
shortening the strbuf. We could leave in place:

  void wt_status_truncate_message_at_cut_line(struct strbuf *sb)
  {
strbuf_setlen(sb, wt_status_last_nonscissors_index(sb->buf, sb->len));
  }

but it would only have this one caller.

If I were doing the patch series, I'd probably have split that
refactoring into its own patch and discussed the reason separately. I
waffled on whether or not to ask Brian to do so (and obviously didn't in
the end).

> The proposed log message calls the cut-line "scissors", but that is
> probably a source of this confusion.  The cut-line and scissors do
> not have much in commmon.  For one thing, scissors is a mechanism to
> discard everything _ABOVE_ it.  The cut-line we see in this example,
> on the other hand, is about discarding everything _BELOW_ it.

I suspect the confusion comes from calling it CLEANUP_SCISSORS in the
quoted context above, then.  Certainly we use scissors for "snip
everything above this line" in mailinfo, but here it is "snip everything
after this line". I don't think it's wrong to refer to it as a scissors
line, even though the operation is different.

-Peff

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> Yes I could do that, no reason not to, and as you point out it would
>> reduce duplication.
>>
>> I wrote it like this trying to preserve the indentation with/without
>> the macro being true, thinking someone would have an issue with it
>> otherwise.
>>
>> I also thought just now that perhaps if it were changed the code like
>> that it would warn under -Wmisleading-indentation, but at least on gcc
>> that's not the case, it knows not to warn in the presence of macros.
>>
>> Unless someone feel strongly otherwise / can think of a good reason
>> for why not, I'll change it as you suggest in the next version.
>>
>> Thanks for the review!
>
> ...and if I do change it do others think this is something that
> warrants a comment & some whitespace padding? I.e.:
>
> @@ -378,8 +392,17 @@ static int pcre1match(struct grep_pat *p, const
> char *line, const char *eol,
> if (eflags & REG_NOTBOL)
> flags |= PCRE_NOTBOL;
>
> +#ifdef PCRE_CONFIG_JIT
> +   if (p->pcre1_jit_on)
> +   ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, 
> line,
> +   eol - line, 0, flags, ovector,
> +   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
> +   else
> +#endif
> +   /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
> ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - 
> line,
> 0, flags, ovector, ARRAY_SIZE(ovector));
> +
> if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
> die("pcre_exec failed with error code %d", ret);
> if (ret > 0) {

If we MUST have this #ifdef/#endif in-line in this function, then
tolerating funny indentation in the else clause I think is an
accepted common practice that may not need an extra comment.

But I wonder if the resulting code of this function may become
simpler to follow if we remove #ifdef/#endif from it, and instead
have this function call a helper (which may itself have #ifdef, or
maybe #ifdef/#else/#endif may have two implementations)?

Re: [PATCH v3] builtin/log: honor log.decorate

[Junio C Hamano]

"brian m. carlson"  writes:

> The recent change that introduced autodecorating of refs accidentally
> broke the ability of users to set log.decorate = false to override it.
> When the git_log_config was traversed a second time with an option other
> than log.decorate, the decoration style would be set to the automatic
> style, even if the user had already overridden it.  Instead of setting
> the option in config parsing, set it in init_log_defaults instead.
>
> Add a test for this case.  The actual additional config option doesn't
> matter, but it needs to be something not already set in the
> configuration file.
>
> Signed-off-by: brian m. carlson 
> ---
> Changes from v2:
> * Add a test.  I tested that the config parsing both works with
>   additional options and also can be overridden from the command line.

Thanks, all.

Will queue with Acked-by by Alex and Reviewed-by by Jonathan.


>  builtin/log.c  |  4 ++--
>  t/t4202-log.sh | 12 
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index b3b10cc1e..ec3258368 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -110,6 +110,8 @@ static void init_log_defaults(void)
>  {
>   init_grep_defaults();
>   init_diff_ui_defaults();
> +
> + decoration_style = auto_decoration_style();
>  }
>  
>  static void cmd_log_init_defaults(struct rev_info *rev)
> @@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char 
> *value, void *cb)
>   if (decoration_style < 0)
>   decoration_style = 0; /* maybe warn? */
>   return 0;
> - } else {
> - decoration_style = auto_decoration_style();
>   }
>   if (!strcmp(var, "log.showroot")) {
>   default_show_root = git_config_bool(var, value);
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index f57799071..1c7d6729c 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -577,6 +577,18 @@ test_expect_success 'log.decorate configuration' '
>  
>  '
>  
> +test_expect_success 'log.decorate config parsing' '
> + git log --oneline --decorate=full >expect.full &&
> + git log --oneline --decorate=short >expect.short &&
> +
> + test_config log.decorate full &&
> + test_config log.mailmap true &&
> + git log --oneline >actual &&
> + test_cmp expect.full actual &&
> + git log --oneline --decorate=short >actual &&
> + test_cmp expect.short actual
> +'
> +
>  test_expect_success TTY 'log output on a TTY' '
>   git log --oneline --decorate >expect.short &&
>  

Re: [PATCH 1/3] usage.c: add BUG() function

[Junio C Hamano]

Jeff King  writes:

> On Fri, May 12, 2017 at 11:28:50PM -0400, Jeff King wrote:
>
>> +static NORETURN void BUG_vfl(const char *file, int line, const char *fmt, 
>> va_list params)
>> +{
>> +char prefix[256];
>> +
>> +/* truncation via snprintf is OK here */
>> +if (file)
>> +snprintf(prefix, sizeof(prefix), "BUG: %s:%d: ", file, line);
>> +else
>> +snprintf(prefix, sizeof(prefix), "BUG: ");
>> +
>> +vreportf(prefix, fmt, params);
>> +abort();
>> +}
>
> I used vreportf() here to match die(). But the two things that function
> does are:
>
>   1. Respect error_handle. But after bw/forking-and-threading is merged,
>  nobody will ever set error_handle (and I just sent a patch to drop
>  it entirely).
>
>   2. Quotes non-printable characters. I don't know how useful this is.
>  Most of the assertion messages are pretty vanilla (because anything
>  that relies on user input probably should be a regular die, not an
>  assertion failure). But a few of them do actually print arbitrary
>  strings in a reasonable way (e.g., a BUG() which is handling user
>  string which was supposed to be vetted by an earlier function is
>  still a reasonable assertion, but it's useful to show the string).
>
> So an alternative would be just:
>
>   fprintf(stderr, "BUG: ");
>   if (file)
>   fprintf(stderr, "%s:%d ", file, line);
>   vfprintf(stderr, fmt, params);
>   fputc('\n', stderr);
>
> which is perhaps a bit simpler (not much in lines of code, but there's
> no extra buffer to reason about). But given the discussion in (2) above,
> it's probably worth continuing to use vreportf.

Yeah, that does sound sensible.  Thanks.

Re: [PATCH v4 1/3] usability: don't ask questions if no reply is required

[Junio C Hamano]

Johannes Sixt  writes:

> Am 13.05.2017 um 00:36 schrieb Junio C Hamano:
>> Thanks, all three patches look good.  Will queue.
>>
>> Let's merge them to 'next' soonish and eventually down to 'master'
>> and 'maint'.
>
> The patches change translated strings. You should probably wait for an
> update of their translations before you release a maintenance version
> with these changes.

Yup, thanks.

Re: [PATCH] fixup! log: add exhaustive tests for pattern style options & config

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Thanks for the hotfix. I'll fix this in my v2, but do it differently
> in such a way that I can still run these tests on windows.
>
> I.e. the actual test here just needs these odd characters in the
> commit message. It's just an unintended implementation detail of
> test_commit that a tag is being created.

My knee-jerk reaction matched Dscho's, but grep is about contents,
and we should be able to test this if we used a sensible tagnames or
didn't use any.  Glad to see somebody can step back and think ;-)

Re: [PATCH] interpret-trailers: obey scissors lines

[Junio C Hamano]

Jeff King  writes:

>> If a commit message is being editted as "verbose", it will contain a
>> scissors string ("-- >8 --") and a diff:
>> 
>> my subject
>> 
>> #  >8 
>> # Do not touch the line above.
>> # Everything below will be removed.
>> diff --git a/foo.txt b/foo.txt
>> index 5716ca5..7601807 100644
>> --- a/foo.txt
>> +++ b/foo.txt
>> @@ -1 +1 @@
>> -bar
>> +baz
>> 
>> interpret-trailers doesn't interpret the scissors and therefore places
>> trailer information after the diff. A simple reproduction is:
>> 
>> git config commit.verbose true
>> GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
>> git commit --amend

So, "git commit --amend --verbose" prepares something like the above
example and passes it to the GIT_EDITOR.  The problem description
indicates that because interpret-trailers (which happens to be used
as the GIT_EDITOR) treats the cut-line used by "git commit" just
like any other random line, it tries to find the trailer block way
below, in the patch text.  I agree that it is a problem.  But...

>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 2de5f6cc6..2ce9c339d 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char 
>> *prefix)
>>  
>>  if (verbose || /* Truncate the message just before the diff, if any. */
>>  cleanup_mode == CLEANUP_SCISSORS)
>> -wt_status_truncate_message_at_cut_line();
>> +strbuf_setlen(,
>> +  wt_status_last_nonscissors_index(sb.buf, sb.len));
>
> This hunk surprised me at first (that we would need to touch commit.c at
> all), but the refactoring makes sense.

This still surprises me.  If the problem is in interpret-trailers,
why do we even need to touch cmd_commit()?  If GIT_EDITOR returns us

 my subject
 
 Acked-by: me
 #  >8 
 # Do not touch the line above.
 # Everything below will be removed.
 diff --git a/foo.txt b/foo.txt
 index 5716ca5..7601807 100644
 --- a/foo.txt
 +++ b/foo.txt
 @@ -1 +1 @@
 -bar
 +baz

after this patch fixes interpret-trailers, wouldn't the cmd_commit()
function work as expected without any change?

Puzzled.

The proposed log message calls the cut-line "scissors", but that is
probably a source of this confusion.  The cut-line and scissors do
not have much in commmon.  For one thing, scissors is a mechanism to
discard everything _ABOVE_ it.  The cut-line we see in this example,
on the other hand, is about discarding everything _BELOW_ it.

Re: [PATCH v2] send-email: support validate hook

[Junio C Hamano]

Jonathan Tan  writes:

> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
> index 706091a56..b2514f4d4 100644
> --- a/Documentation/githooks.txt
> +++ b/Documentation/githooks.txt
> @@ -447,6 +447,14 @@ rebase::
>  The commits are guaranteed to be listed in the order that they were
>  processed by rebase.
>  
> +sendemail-validate
> +~~
> +
> +This hook is invoked by 'git send-email'.  It takes a single parameter,
> +the name of the file that holds the e-mail to be sent.  Exiting with a
> +non-zero status causes 'git send-email' to abort before sending any
> +e-mails.
> +

I briefly wondered if an interface that allows only the name of the
file will close the door to future enhancements, but in the end
decided it is OK.  E.g. users may want "here is the contents, is it
appropriate to be sent to this and that address?"---but this hook is
meant to enhances/extends the existing "make sure the contents do
not syntactically bust the technical limit of underlying transport",
and sits at a place best to do so in the codeflow, i.e. before we
even start to decide who the recipients of the patch are.  So those
who want "given the contents of the patch and list of the recipients,
decide if it is OK to send the patch to all of them" would be better
served by a separate hook, not this one.

> + write_script .git/hooks/sendemail-validate <<-\EOF &&
> + # test that we have the correct environment variable, pwd, and
> + # argument
> + case "$GIT_DIR" in
> + *.git)
> + true
> + ;;
> + *)
> + false
> + ;;
> + esac &&
> + test -e 0001-add-master.patch &&
> + grep "add master" "$1"
> + EOF

I'd squash in cosmetic changes to de-dent the contents of
write_script (our standard style is that the body of the script is
written as if the column at which write_script..EOF starts is the
left-edge of the page; I think this file already has a few style
violations that may want to be updated with a separate clean-up
patch when the file is quiet), and then de-dent the case arm (case
arms are indented at the same depth as case/esac).  Also I think we
care that 0001-add-master.patch not just exists but is a file, so
I'd do s/test -e/test -f/ while at it.

Thanks.

Re: [PATCH] compat/regex: fix compilation on Windows

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

> Let's drop this current gawk import series.
>
> After talking to the gawk author it turns out it's better to use the
> version from gnulib, this includes the equivalent of your patch.

OK.  That may make things simpler ;-)

Thanks.

Re: [PATCH 11/29] grep: add a test helper function for less verbose -f \0 tests

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  writes:

>> It somehow was unintuitive that 0 expected failure and 1 expected
>> success, but it probably was just me.
>
> Except this. The wildmatch uses the same idiom, and I think it makes
> sense. 1 = true, 0 = false, ... when all we wanted was
> "does this match".

OK.  Makes perfect sense (it would have been even easier to
understand if the variable was not $status but $matches or
something).

Re: [PATCH v6] fetch-pack: always allow fetching of literal SHA1s

[Junio C Hamano]

Jeff King  writes:

> On Fri, May 12, 2017 at 01:46:48PM -0700, Jonathan Tan wrote:
>
>> Change from v5: used "ensure_tip_oids_initialized" function instead.
>> This removes some of the muddiness (e.g. with newlist being modified
>> after the function).
>
> I don't think it really improves the muddiness. You are still calling
> the ensure function each time through the loop with a potentially
> modified list, but it doesn't actually look at the list after the first
> time. So the muddy part is still there.
>
> I think rather than changing the code, you'd do better with a comment
> like:
>
>   /*
>* Note that this only looks at the ref lists the first time
>* it's called. This works out in filter_refs() because even
>* though it may add to "newlist" between calls, the additions
>* will always be for oids that are already in the set.
>*/
>
> At which point the original all-in-one function is probably fine (as it
> avoids the "return 1 just to join the &&-chain" bit).

Yes.  I agree that the in-code comment is the best approach to the
"muddy combination of 'grabbing it for the first time' and 'the
thing that is grabbed mutates in the loop'" confusion.

Re: [PATCH 1/7] compat/regex: add a README with a maintenance guide

[Junio C Hamano]

arn...@skeeve.com writes:

> Junio C Hamano  wrote:
>
>> Johannes Schindelin  writes:
>>
>> > - rather than scraping the files from the CGit website (which does not
>> >   guarantee that the first scraped file will be from the same revision as
>> >   the last scraped file), I would very strongly prefer the files to be
>> >   copied from a clone of gawk.git, and the gawk.git revision from which
>> >   they were copied should be recorded in git.git's commit adding them.
>>
>> Wow, I didn't even notice that was how the "original" came about.
>> No question that we should be taking from a known-stable snapshot,
>> not from a moving target.
>
> Gawk's regex has been fairly stable of late.  But marking the revision
> from which you take the regex is a good idea in any case.

I do not mind taking a snapshot from an untagged commit (instead of
sticking to the last tagged commit, being suspicious about newer
developments).  What I was reacting to was a loop like this:

for f in $(find . -name '*.[ch]' -printf "%f\n")
do wget http://git.savannah.gnu.org/cgit/gawk.git/plain/support/$f -O $f
done

i.e. allowing wget to grab paths out of possibly different commits.

Thanks.

Re: [PATCH] config: complain about --local outside of a git repo

[Josh Hagins]

Hi Jeff,

For context, the "user.name" bit was purely notional; I just wanted to
give a sample reproduction. Where I've actually been running into this
in real life is with oh-my-git, a GitHub-themed Powerline bash prompt:
https://github.com/arialdomartini/oh-my-git/blob/42c11f08540949b75bd513e6880a4ce6824a2bcc/base.sh#L52.

Since this command is invoked every time the prompt is build, I see
the error message after every single command I run outside of a Git
repository. While this is more indicative of a code smell in
oh-my-git, I figured that, as you said, BUG assertions should never
get hit in the wild. Thanks for the patch!

Cheers,
Josh

On Fri, May 12, 2017 at 4:34 PM, Jeff King  wrote:
> On Fri, May 12, 2017 at 10:19:59AM -0400, Josh Hagins wrote:
>
>> Since upgrading to Git 2.13.0 I'm seeing this error message whenever
>> `git config --local ` is called outside a Git repository.
>> For example, note the difference in behavior between Git 2.13 and
>> Apple Git:
>>
>> $ pwd
>> /Users/jhagins
>> $ /usr/bin/git --version
>> git version 2.11.0 (Apple Git-81)
>> $ /usr/bin/git config --local --get user.name
>> $ /usr/local/bin/git --version
>> git version 2.13.0
>> $ /usr/local/bin/git config --local --get user.name
>> fatal: BUG: setup_git_env called without repository
>>
>> Apple Git outputs nothing, as expected. The summarized release notes
>> published by GitHub specifically mentioned that instances of this
>> error message should be reported, so here you go!
>
> Thanks for reporting. All the developers have been running with this
> change for months, but I knew as soon as it was released into the wild
> that somebody would find a new corner case. :)
>
> I think dying is the right thing here; you are asking for "--local" but
> there is no local repository. But we should never hit a BUG assertion.
> Patch is below.
>
> I'm not sure exactly what you wanted to accomplish with --local. If you
> just want to know if user.name is set anywhere (and you may or may not
> be in a git repo), then just "git config --get user.name" would work. If
> you want to know if you're in a local repo and if so whether the
> variable is set, you'd need to use two commands, like:
>
>   git rev-parse --git-dir >/dev/null 2>&1 &&
>   git config --local --get user.name
>
> -- >8 --
> Subject: [PATCH] config: complain about --local outside of a git repo
>
> The "--local" option instructs git-config to read or modify
> the repository-level config. This doesn't make any sense if
> you're not actually in a repository.
>
> Older versions of Git would blindly try to read or write
> ".git/config". For reading, this would result in a quiet
> failure, since there was no config to read (and thus no
> matching config value). Writing would generally fail
> noisily, since ".git" was unlikely to exist. But since
> b1ef400ee (setup_git_env: avoid blind fall-back to ".git",
> 2016-10-20), we catch this in the call to git_pathdup() and
> die("BUG").
>
> Dying is the right thing to do, but we should catch the
> problem early and give a more human-friendly error message.
>
> Note that even without --local, git-config will sometimes
> default to using local repository config. These cases are
> already protected by a similar check.
>
> Signed-off-by: Jeff King 
> ---
>  builtin/config.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/config.c b/builtin/config.c
> index 3a554ad50..ad7c6a19c 100644
> --- a/builtin/config.c
> +++ b/builtin/config.c
> @@ -496,6 +496,9 @@ int cmd_config(int argc, const char **argv, const char 
> *prefix)
> usage_with_options(builtin_config_usage, 
> builtin_config_options);
> }
>
> +   if (use_local_config && nongit)
> +   die(_("--local only be used inside a git repository"));
> +
> if (given_config_source.file &&
> !strcmp(given_config_source.file, "-")) {
> given_config_source.file = NULL;
> --
> 2.13.0.452.g0afc8e12b
>



-- 
Josh Hagins

Re: [Git 2.13.0] BUG: setup_git_env called without repository

[Josh Hagins]

Hi Johannes,

Here's the full text of the gdb session, including backtrace. Hope it helps!

$ pwd
/Users/jhagins/dev/github
$ gdb -args ./git/git config --local --get user.name
GNU gdb (GDB) 7.12.1
Copyright (C) 2017 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later 
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-apple-darwin16.4.0".
Type "show configuration" for configuration details.
For bug reporting instructions, please see:
.
Find the GDB manual and other documentation resources online at:
.
For help, type "help".
Type "apropos word" to search for commands related to "word"...
Reading symbols from ./git/git...done.
(gdb) b die_builtin
Breakpoint 1 at 0x100210757: file usage.c, line 33.
(gdb) r
Starting program: /Volumes/git/github/git/git config --local --get user.name
[New Thread 0x1403 of process 64101]
warning: unhandled dyld version (15)

Thread 2 hit Breakpoint 1, die_builtin (err=0x100270d95 "BUG:
setup_git_env called without repository", params=0x7fff5fbfdbe0) at
usage.c:33
33 vreportf("fatal: ", err, params);
(gdb) bt
#0  die_builtin (err=0x100270d95 "BUG: setup_git_env called without
repository", params=0x7fff5fbfdbe0) at usage.c:33
#1  0x00010020fc80 in die (err=0x100270d95 "BUG: setup_git_env
called without repository") at usage.c:120
#2  0x00010012cb2b in setup_git_env () at environment.c:172
#3  0x00010012cab7 in get_git_dir () at environment.c:214
#4  0x000100214e9c in get_worktree_git_dir (wt=0x0) at worktree.c:215
#5  0x00010017968c in do_git_path (wt=0x0, buf=0x7fff5fbfde50,
fmt=0x10024c24d "config", args=0x7fff5fbfde70) at path.c:395
#6  0x000100179c78 in git_pathdup (fmt=0x10024c24d "config") at path.c:437
#7  0x000100030449 in cmd_config (argc=1, argv=0x7fff5fbfe180,
prefix=0x0) at builtin/config.c:527
#8  0x00011ac5 in run_builtin (p=0x10028c3a8, argc=4,
argv=0x7fff5fbfe180) at git.c:371
#9  0x00010cc6 in handle_builtin (argc=4, argv=0x7fff5fbfe180)
at git.c:572
#10 0x00011883 in run_argv (argcp=0x7fff5fbfe12c,
argv=0x7fff5fbfe120) at git.c:624
#11 0x00010a4f in cmd_main (argc=4, argv=0x7fff5fbfe180) at git.c:701
#12 0x0001000c3772 in main (argc=5, argv=0x7fff5fbfe178) at common-main.c:43

Cheers,
Josh

On Fri, May 12, 2017 at 10:48 AM, Johannes Schindelin
 wrote:
> Hi Josh,
>
> On Fri, 12 May 2017, Josh Hagins wrote:
>
>> Since upgrading to Git 2.13.0 I'm seeing this error message whenever
>> `git config --local ` is called outside a Git repository.
>> For example, note the difference in behavior between Git 2.13 and
>> Apple Git:
>>
>> $ pwd
>> /Users/jhagins
>> $ /usr/bin/git --version
>> git version 2.11.0 (Apple Git-81)
>> $ /usr/bin/git config --local --get user.name
>> $ /usr/local/bin/git --version
>> git version 2.13.0
>> $ /usr/local/bin/git config --local --get user.name
>> fatal: BUG: setup_git_env called without repository
>>
>> Apple Git outputs nothing, as expected. The summarized release notes
>> published by GitHub specifically mentioned that instances of this
>> error message should be reported, so here you go!
>>
>> Please let me know if I can provide any more information that would be
>> helpful.
>
> Since this is in /usr/local/bin/, there are two possibilities:
>
> 1) you built and installed it yourself (but then it would be more likely
>in your $HOME/bin), or
>
> 2) you installed it via HomeBrew.
>
> I guess it is the latter.
>
> In both cases, however, you have XCode installed, so you can dig further.
> Yay.
>
> The thing I would do in your case would be to clone Git:
>
> git clone https://github.com/git/git
>
> then check out v2.13.0:
>
> git checkout v2.13.0
>
> then edit the Makefile to remove the -O2 from the CFLAGS (the next step is
> to use the GNU debugger, and in my hands the -O2 optimization made that
> pretty useless), and then build with
>
> make
>
> After that, you should be able to start the command in your local GNU
> debugger:
>
> gdb -args ./git config --local --get user.name
>
> You will then want to set a breakpoint on the die_builtin() function:
>
> b die_builtin
>
> Now run it with the `r` command, and it should stop in the die_builtin
> routine, in which case a backtrace would be most helpful to figure out
> what is going wrong:
>
> bt
>
> If the output is not enlightening on its own, it would be nice to paste it
> into a reply to this mail so that the entire Git developer community can
> have a look.
>
> Ciao,
> Johannes



-- 
Josh Hagins

Cant clone/pull/fetch because of "Unable to create temporary file '$HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_'

[Thomas Schweikle]

$ git clone
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux-stable.git
linux-stable
Cloning into 'linux-stable'...
remote: Counting objects: 5932092, done.
remote: Compressing objects: 100% (154131/154131), done.
fatal: Unable to create temporary file
'$HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX': Permission
denied
fatal: index-pack failed

Since no file/directory created by git I cant tell why git isn't
able to create
"$HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX".

If I try to create this file and directory I can create it:
$ mkdir -p $HOME/Dev/linux-stable/.git/objects/pack
$ touch $HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX
$ ll $HOME/Dev/linux-stable/.git/objects/pack/tmp_pack_XX
-rw-rw-r-x+ 1 tps tps 0 May 15 00:18
/home/tps/Dev/linux-stable/.git/objects/pack/tmp_pack_XX
$

$ git --version
git version 2.11.0

-- 
Thomas

Re: [PATCH 1/7] compat/regex: add a README with a maintenance guide

[arnold]

Junio C Hamano  wrote:

> Johannes Schindelin  writes:
>
> > - rather than scraping the files from the CGit website (which does not
> >   guarantee that the first scraped file will be from the same revision as
> >   the last scraped file), I would very strongly prefer the files to be
> >   copied from a clone of gawk.git, and the gawk.git revision from which
> >   they were copied should be recorded in git.git's commit adding them.
>
> Wow, I didn't even notice that was how the "original" came about.
> No question that we should be taking from a known-stable snapshot,
> not from a moving target.

Gawk's regex has been fairly stable of late.  But marking the revision
from which you take the regex is a good idea in any case.

With respect to bug fixes that may have happened downstream, please do
let me know of any.  But I do request it as a bug report to bug-g...@gnu.org
and not just a pull request with no commentary.

Thanks,

Arnold

[PATCH v3] builtin/log: honor log.decorate

[brian m. carlson]

The recent change that introduced autodecorating of refs accidentally
broke the ability of users to set log.decorate = false to override it.
When the git_log_config was traversed a second time with an option other
than log.decorate, the decoration style would be set to the automatic
style, even if the user had already overridden it.  Instead of setting
the option in config parsing, set it in init_log_defaults instead.

Add a test for this case.  The actual additional config option doesn't
matter, but it needs to be something not already set in the
configuration file.

Signed-off-by: brian m. carlson 
---
Changes from v2:
* Add a test.  I tested that the config parsing both works with
  additional options and also can be overridden from the command line.

 builtin/log.c  |  4 ++--
 t/t4202-log.sh | 12 
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index b3b10cc1e..ec3258368 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -110,6 +110,8 @@ static void init_log_defaults(void)
 {
init_grep_defaults();
init_diff_ui_defaults();
+
+   decoration_style = auto_decoration_style();
 }
 
 static void cmd_log_init_defaults(struct rev_info *rev)
@@ -410,8 +412,6 @@ static int git_log_config(const char *var, const char 
*value, void *cb)
if (decoration_style < 0)
decoration_style = 0; /* maybe warn? */
return 0;
-   } else {
-   decoration_style = auto_decoration_style();
}
if (!strcmp(var, "log.showroot")) {
default_show_root = git_config_bool(var, value);
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index f57799071..1c7d6729c 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -577,6 +577,18 @@ test_expect_success 'log.decorate configuration' '
 
 '
 
+test_expect_success 'log.decorate config parsing' '
+   git log --oneline --decorate=full >expect.full &&
+   git log --oneline --decorate=short >expect.short &&
+
+   test_config log.decorate full &&
+   test_config log.mailmap true &&
+   git log --oneline >actual &&
+   test_cmp expect.full actual &&
+   git log --oneline --decorate=short >actual &&
+   test_cmp expect.short actual
+'
+
 test_expect_success TTY 'log output on a TTY' '
git log --oneline --decorate >expect.short &&
 

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

[Ævar Arnfjörð Bjarmason]

On Sun, May 14, 2017 at 5:10 PM, Ævar Arnfjörð Bjarmason
 wrote:
> On Sun, May 14, 2017 at 4:43 PM, Simon Ruderich  wrote:
>> On Sat, May 13, 2017 at 11:45:32PM +, Ęvar Arnfjörš Bjarmason wrote:
>>> [snip]
>>>
>>> +#ifdef PCRE_CONFIG_JIT
>>> + if (p->pcre1_jit_on)
>>> + ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, 
>>> line,
>>> + eol - line, 0, flags, ovector,
>>> + ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>>> + else
>>> + ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>>> + eol - line, 0, flags, ovector,
>>> + ARRAY_SIZE(ovector));
>>> +#else
>>>   ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - 
>>> line,
>>>   0, flags, ovector, ARRAY_SIZE(ovector));
>>> +#endif
>>
>> Wouldn't it be simpler to remove the duplication and
>> unconditionally use the old pcre_exec() call? Something like
>> this:
>>
>> +#ifdef PCRE_CONFIG_JIT
>> +   if (p->pcre1_jit_on)
>> +   ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, 
>> line,
>> +   eol - line, 0, flags, ovector,
>> +   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>> +   else
>> +#endif
>> ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - 
>> line,
>> 0, flags, ovector, ARRAY_SIZE(ovector));
>>
>>>   if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>>>   die("pcre_exec failed with error code %d", ret);
>>>   if (ret > 0) {
>>> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char 
>>> *line, const char *eol,
>>>  static void free_pcre1_regexp(struct grep_pat *p)
>>>  {
>>>   pcre_free(p->pcre1_regexp);
>>> +#ifdef PCRE_CONFIG_JIT
>>> + if (p->pcre1_jit_on) {
>>> + pcre_free_study(p->pcre1_extra_info);
>>> + pcre_jit_stack_free(p->pcre1_jit_stack);
>>> + } else {
>>> + pcre_free(p->pcre1_extra_info);
>>> + }
>>> +#else
>>>   pcre_free(p->pcre1_extra_info);
>>> +#endif
>>
>> Same here. The pcre_free() is the same with and without the
>> ifdef.
>
> Yes I could do that, no reason not to, and as you point out it would
> reduce duplication.
>
> I wrote it like this trying to preserve the indentation with/without
> the macro being true, thinking someone would have an issue with it
> otherwise.
>
> I also thought just now that perhaps if it were changed the code like
> that it would warn under -Wmisleading-indentation, but at least on gcc
> that's not the case, it knows not to warn in the presence of macros.
>
> Unless someone feel strongly otherwise / can think of a good reason
> for why not, I'll change it as you suggest in the next version.
>
> Thanks for the review!

...and if I do change it do others think this is something that
warrants a comment & some whitespace padding? I.e.:

@@ -378,8 +392,17 @@ static int pcre1match(struct grep_pat *p, const
char *line, const char *eol,
if (eflags & REG_NOTBOL)
flags |= PCRE_NOTBOL;

+#ifdef PCRE_CONFIG_JIT
+   if (p->pcre1_jit_on)
+   ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
+   eol - line, 0, flags, ovector,
+   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
+   else
+#endif
+   /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
0, flags, ovector, ARRAY_SIZE(ovector));
+
if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
die("pcre_exec failed with error code %d", ret);
if (ret > 0) {

and:

@@ -394,7 +417,16 @@ static int pcre1match(struct grep_pat *p, const
char *line, const char *eol,
 static void free_pcre1_regexp(struct grep_pat *p)
 {
pcre_free(p->pcre1_regexp);
+
+#ifdef PCRE_CONFIG_JIT
+   if (p->pcre1_jit_on) {
+   pcre_free_study(p->pcre1_extra_info);
+   pcre_jit_stack_free(p->pcre1_jit_stack);
+   } else
+#endif
+   /* PCRE_CONFIG_JIT !p->pcre1_jit_on else branch */
pcre_free(p->pcre1_extra_info);
+
pcre_free((void *)p->pcre1_tables);
 }
 #else /* !USE_LIBPCRE1 */

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

[Ævar Arnfjörð Bjarmason]

On Sun, May 14, 2017 at 4:43 PM, Simon Ruderich  wrote:
> On Sat, May 13, 2017 at 11:45:32PM +, Ęvar Arnfjörš Bjarmason wrote:
>> [snip]
>>
>> +#ifdef PCRE_CONFIG_JIT
>> + if (p->pcre1_jit_on)
>> + ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>> + eol - line, 0, flags, ovector,
>> + ARRAY_SIZE(ovector), p->pcre1_jit_stack);
>> + else
>> + ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
>> + eol - line, 0, flags, ovector,
>> + ARRAY_SIZE(ovector));
>> +#else
>>   ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>>   0, flags, ovector, ARRAY_SIZE(ovector));
>> +#endif
>
> Wouldn't it be simpler to remove the duplication and
> unconditionally use the old pcre_exec() call? Something like
> this:
>
> +#ifdef PCRE_CONFIG_JIT
> +   if (p->pcre1_jit_on)
> +   ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, 
> line,
> +   eol - line, 0, flags, ovector,
> +   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
> +   else
> +#endif
> ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - 
> line,
> 0, flags, ovector, ARRAY_SIZE(ovector));
>
>>   if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>>   die("pcre_exec failed with error code %d", ret);
>>   if (ret > 0) {
>> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char 
>> *line, const char *eol,
>>  static void free_pcre1_regexp(struct grep_pat *p)
>>  {
>>   pcre_free(p->pcre1_regexp);
>> +#ifdef PCRE_CONFIG_JIT
>> + if (p->pcre1_jit_on) {
>> + pcre_free_study(p->pcre1_extra_info);
>> + pcre_jit_stack_free(p->pcre1_jit_stack);
>> + } else {
>> + pcre_free(p->pcre1_extra_info);
>> + }
>> +#else
>>   pcre_free(p->pcre1_extra_info);
>> +#endif
>
> Same here. The pcre_free() is the same with and without the
> ifdef.

Yes I could do that, no reason not to, and as you point out it would
reduce duplication.

I wrote it like this trying to preserve the indentation with/without
the macro being true, thinking someone would have an issue with it
otherwise.

I also thought just now that perhaps if it were changed the code like
that it would warn under -Wmisleading-indentation, but at least on gcc
that's not the case, it knows not to warn in the presence of macros.

Unless someone feel strongly otherwise / can think of a good reason
for why not, I'll change it as you suggest in the next version.

Thanks for the review!

Re: [PATCH v2 4/7] grep: add support for the PCRE v1 JIT API

[Simon Ruderich]

On Sat, May 13, 2017 at 11:45:32PM +, Ævar Arnfjörð Bjarmason wrote:
> [snip]
>
> +#ifdef PCRE_CONFIG_JIT
> + if (p->pcre1_jit_on)
> + ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> + eol - line, 0, flags, ovector,
> + ARRAY_SIZE(ovector), p->pcre1_jit_stack);
> + else
> + ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
> + eol - line, 0, flags, ovector,
> + ARRAY_SIZE(ovector));
> +#else
>   ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
>   0, flags, ovector, ARRAY_SIZE(ovector));
> +#endif

Wouldn't it be simpler to remove the duplication and
unconditionally use the old pcre_exec() call? Something like
this:

+#ifdef PCRE_CONFIG_JIT
+   if (p->pcre1_jit_on)
+   ret = pcre_jit_exec(p->pcre1_regexp, p->pcre1_extra_info, line,
+   eol - line, 0, flags, ovector,
+   ARRAY_SIZE(ovector), p->pcre1_jit_stack);
+   else
+#endif
ret = pcre_exec(p->pcre1_regexp, p->pcre1_extra_info, line, eol - line,
0, flags, ovector, ARRAY_SIZE(ovector));

>   if (ret < 0 && ret != PCRE_ERROR_NOMATCH)
>   die("pcre_exec failed with error code %d", ret);
>   if (ret > 0) {
> @@ -394,7 +420,16 @@ static int pcre1match(struct grep_pat *p, const char 
> *line, const char *eol,
>  static void free_pcre1_regexp(struct grep_pat *p)
>  {
>   pcre_free(p->pcre1_regexp);
> +#ifdef PCRE_CONFIG_JIT
> + if (p->pcre1_jit_on) {
> + pcre_free_study(p->pcre1_extra_info);
> + pcre_jit_stack_free(p->pcre1_jit_stack);
> + } else {
> + pcre_free(p->pcre1_extra_info);
> + }
> +#else
>   pcre_free(p->pcre1_extra_info);
> +#endif

Same here. The pcre_free() is the same with and without the
ifdef.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Ich brauche Ihre Unterstützung zu investieren !!

[Ruth Williams]

Sehr geehrte Damen und Herren,

Ich brauche Ihre Unterstützung in Ihrem Land zu verlagern und zu investieren.
Ich bitte Sie um Hilfe, weil ich nicht das Wissen überGeschäft und die Regeln, 
die Ihr Land für eine sichere Investition führen.

Werden Sie versprechen, mit mir aufrichtig zu sein?

Bitte kontaktieren Sie mich für weitere Details!

Mit freundlichen Grüßen,
Fräulein Ruth Williams

[PATCH] interpret-trailers: obey scissors lines

[Brian Malehorn]

If a commit message is being edited as "verbose", it will contain a
scissors string ("-- >8 --") and a diff:

my subject

#  >8 
# Do not touch the line above.
# Everything below will be removed.
diff --git a/foo.txt b/foo.txt
index 5716ca5..7601807 100644
--- a/foo.txt
+++ b/foo.txt
@@ -1 +1 @@
-bar
+baz

interpret-trailers doesn't interpret the scissors and therefore places
trailer information after the diff. A simple reproduction is:

git config commit.verbose true
GIT_EDITOR='git interpret-trailers --in-place --trailer Acked-by:me' \
git commit --amend

This commit resolves the issue by teaching "git interpret-trailers" to
obey scissors the same way "git commit" does.
---
 builtin/commit.c  |  3 ++-
 commit.c  | 13 +++--
 t/t7513-interpret-trailers.sh | 17 +
 wt-status.c   | 11 ++-
 wt-status.h   |  2 +-
 5 files changed, 33 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 2de5f6cc6..d223cf3ea 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1735,7 +1735,8 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
 
if (verbose || /* Truncate the message just before the diff, if any. */
cleanup_mode == CLEANUP_SCISSORS)
-   wt_status_truncate_message_at_cut_line();
+   strbuf_setlen(,
+ wt_status_strip_scissors(sb.buf, sb.len));
 
if (cleanup_mode != CLEANUP_NONE)
strbuf_stripspace(, cleanup_mode == CLEANUP_ALL);
diff --git a/commit.c b/commit.c
index fab826973..6c26ac9b9 100644
--- a/commit.c
+++ b/commit.c
@@ -11,6 +11,7 @@
 #include "commit-slab.h"
 #include "prio-queue.h"
 #include "sha1-lookup.h"
+#include "wt-status.h"
 
 static struct commit_extra_header *read_commit_extra_header_lines(const char 
*buf, size_t len, const char **);
 
@@ -1649,10 +1650,9 @@ const char *find_commit_header(const char *msg, const 
char *key, size_t *out_len
 /*
  * Inspect the given string and determine the true "end" of the log message, in
  * order to find where to put a new Signed-off-by: line.  Ignored are
- * trailing comment lines and blank lines, and also the traditional
- * "Conflicts:" block that is not commented out, so that we can use
- * "git commit -s --amend" on an existing commit that forgot to remove
- * it.
+ * trailing comment lines and blank lines.  To support "git commit -s
+ * --amend" on an existing commit, we also ignore "Conflicts:".  To
+ * support "git commit -v", we truncate at scissor lines.
  *
  * Returns the number of bytes from the tail to ignore, to be fed as
  * the second parameter to append_signoff().
@@ -1662,8 +1662,9 @@ int ignore_non_trailer(const char *buf, size_t len)
int boc = 0;
int bol = 0;
int in_old_conflicts_block = 0;
+   size_t cutoff = wt_status_strip_scissors(buf, len);
 
-   while (bol < len) {
+   while (bol < cutoff) {
const char *next_line = memchr(buf + bol, '\n', len - bol);
 
if (!next_line)
@@ -1689,5 +1690,5 @@ int ignore_non_trailer(const char *buf, size_t len)
}
bol = next_line - buf;
}
-   return boc ? len - boc : 0;
+   return boc ? len - boc : len - cutoff;
 }
diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 4dd1d7c52..b16d8c5d9 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -1258,4 +1258,21 @@ test_expect_success 'with no command and no key' '
test_cmp expected actual
 '
 
+test_expect_success 'with scissors' '
+   cat >expected <<-\EOF &&
+   my subject
+
+   review: Brian
+   sign: A U Thor 
+   #  >8 
+   ignore this
+   EOF
+   git interpret-trailers --trailer review:Brian >actual <<-\EOF &&
+   my subject
+   #  >8 
+   ignore this
+   EOF
+   test_cmp expected actual
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index 4bb46781c..2a6bba205 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -883,17 +883,18 @@ static void wt_longstatus_print_other(struct wt_status *s,
status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 }
 
-void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
+size_t wt_status_strip_scissors(const char *s, size_t len)
 {
const char *p;
struct strbuf pattern = STRBUF_INIT;
 
strbuf_addf(, "\n%c %s", comment_line_char, cut_line);
-   if (starts_with(buf->buf, pattern.buf + 1))
-   strbuf_setlen(buf, 0);
-   else if ((p = strstr(buf->buf, pattern.buf)))
-   strbuf_setlen(buf, p - buf->buf + 1);
+   if (starts_with(s, pattern.buf + 1))
+   len 

Re: [PATCH] interpret-trailers: obey scissors lines

[Brian Malehorn]

> One, we'd usually use "\EOF" here unless you
> really do want to interpolate inside the here document.

Fixed, and I learned something new.

> And two, we usually indent the contents to the same level as the outer
> cat/EOF pair

Fixed.

I was indenting the same as the other tests in that file. But if the way
you described is the preferred way, then sure.

> Another way to think of it is still as a truncation. Our strip_suffix()
> helper behaves quite similarly to this (not actually writing into the
> buffer, but returning the new length). Perhaps something like
> "wt_status_strip_scissors" would work.

I agree the name was pretty awkward. I was trying to avoid using the
word "truncate" or "strip", since it doesn't make any change to the
buffer. But if strip_suffix() is already around it shouldn't be to
surprising.

I've now renamed it to "wt_status_strip_scissors".

Re: [RFC PATCH v2 04/22] blame: move origin and entry structures to header

[Junio C Hamano]

Jeff Smith  writes:

> The origin and blame_entry structures are core to the blame interface
> and reference each other. Since origin will be more exposed, rename it
> to blame_origin to clarify what it is a part of.
>
> Signed-off-by: Jeff Smith 
> ---
>  blame.h |  86 ++
>  builtin/blame.c | 185 
> +---
>  2 files changed, 140 insertions(+), 131 deletions(-)
>  create mode 100644 blame.h
>
> diff --git a/blame.h b/blame.h
> new file mode 100644
> index 000..f52d0fc
> --- /dev/null
> +++ b/blame.h
> @@ -0,0 +1,86 @@
> +#ifndef BLAME_H
> +#define BLAME_H
> +
> +#include "cache.h"
> +#include "commit.h"
> +#include "xdiff-interface.h"
> +
> +/*
> + * One blob in a commit that is being suspected
> + */
> +struct blame_origin {
> + int refcnt;
> +...
>   ...
> -/*
> - * One blob in a commit that is being suspected
> - */
> -struct origin {
> - int refcnt;

I hate to say this AFTER you sent your second attempt, but could you
have another preparetory step before this patch, that does many
renames of symbols (e.g. s/origin/blame_origin/) and nothing else,
while the lines are still in builtin/blame.c?

To review a step like yoru 04/22 to make sure nothing else other
than moving the lines is going on, an easy way to do so is to run
"blame -C" on the resulting blame.h file---that lets the reviewers'
eyes coast over the lines that came from contiguous region of the
original builtin/blame.c and concentrate on things like what used to
be static to the file now getting extern.

If you rename symbols while moving lines across files, that becomes
impossible because many lines are now different (i.e. a line that
talked about "origin" in builtin/blame.c in the preimage may now
talk about "blame_origin" in blame.h, hence attributed to the
new commit's blame.h).

It may even be OK to do the "rename the symbols" step in a single
patch (e.g. your 04/22 and 05/22 are separate patches and the former
renames "origin" while the latter does "scoreboard"---I am saying
that the preparatory step that renames the symbols without
introducing the new "blame.h" may not have to have these two as
separate patches).

If rename of symbols are done first while the lines are still in the
original file, mechanical moving of lines gets much easier to
review.  Of course, a patch that only renames symbols is also much
easier to review, even if it is a large one.

Thanks.

Re: checkout -b remotes/origin/ should not work

[Junio C Hamano]

Jeff King  writes:

> I think this problem extends beyond "remotes/". The worst is:
>
>   git checkout -b HEAD
>
> but there are many confusing variants:
>
>   git checkout -b refs/heads/foo
>   git checkout -b tags/v1.0
>
> etc. Those are all perfectly legal names, but almost certainly not what
> the user intended. I think the plumbing should continue to allow them,
> but I wouldn't object to some common-sense think-o protections in the
> "checkout -b" plumbing (especially if it could be disabled for power
> users).

Yup.  I suspect that the last one has uses (for those who may want
to build on v1.0 tag it is conceivable that a local branch they use
for it is named like so), but I agree that anything that begins with
refs/* is not something any sane person would want to use.

sanity.branchname configuration or something that tells "checkout"
and "branch" Porcelain commands to barf on an attempt to create such
refnames does not sound too bad, and making it on by default may not
even be a bad idea.  But that leads me to say it may not even need
to be a configurable thing (people who DO want funny names can
already and still use the plumbing).

In any case, no command after such a change should forbid checking
out such a funny-named branch if it already exists.  We should
complain only on (an attempted) creation.

Re: [PATCH] interpret-trailers: obey scissors lines

[Ævar Arnfjörð Bjarmason]

On Sun, May 14, 2017 at 5:39 AM, Brian Malehorn  wrote:
> If a commit message is being editted as "verbose", it will contain a

Typo, should be "edited": https://en.wiktionary.org/wiki/editted