Re: [PATCH] t6026: ensure that long-running script really is

[Johannes Sixt]

Am 11.11.2016 um 00:53 schrieb Junio C Hamano:
> Johannes Schindelin  writes:
> 
>> When making sure that background tasks are cleaned up in 5babb5b
>> (t6026-merge-attr: clean up background process at end of test case,
>> 2016-09-07), we considered to let the background task sleep longer, just
>> to be certain that it will still be running when we want to kill it
>> after the test.
>>
>> Sadly, the assumption appears not to hold true that the test case passes
>> quickly enough to kill the background task within a second.
>>
>> Simply increase it to an hour. No system can be possibly slow enough to
>> make above-mentioned assumption incorrect.
>>
>> Reported by Andreas Schwab.
>>
>> Signed-off-by: Johannes Schindelin 
>> ---
>> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1
>> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1
> 
> OK, I think this is a much better option.  Assuming 3600 is long
> enough for everybody (and if not, we have a bigger problem ;-),
> it will ensure that the stray process will be around when we run the
> 'git merge' test, and by not adding "|| :" after the kill, we check
> that the stray process is still there, i.e. we tested what we wanted
> to test.
> 
> Will revert the two patches that were queued previously and then
> queue this one.

Shouldn't we then move the 'kill' out of test_when_finished and make
it a proper condition of the test? Like this?

diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 348d78b205..6ad8bd098a 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' '
sleep 3600 &
echo $! >sleep.pid
EOF
-   test_when_finished "kill \$(cat sleep.pid)" &&
 
test_write_lines >.gitattributes \
"* merge=ours" "text merge=sleep-an-hour" &&
test_config merge.ours.driver true &&
test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
-   git merge master
+   git merge master &&
+
+   # We are testing that the custom merge driver does not block
+   # index.lock on Windows due to an inherited file handle.
+   # To ensure that this test checks this condition, the process
+   # must still be running at this point (and must have started
+   # in the first place), hence, this kill must not fail:
+   kill "$(cat sleep.pid)"
 '
 
 test_done

Did You Get My Message This Time?

[Friedrich Mayrhofer]

This is the second time i am sending you this mail.I, Friedrich Mayrhofer 
Donate $ 1,000,000.00 to You, Email  Me personally for more details.

Regards.
Friedrich Mayrhofer

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Jacob Keller]

On Thu, Nov 10, 2016 at 9:36 AM, Karthik Nayak  wrote:
> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller  wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>>> From: Karthik Nayak 
>>>
>>> Add support for %(objectname:short=) which would print the
>>> abbreviated unique objectname of given length. When no length is
>>> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
>>> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
>>> object name is unique.
>>>
>>
>> Ok this makes sense. It may be annoying that the length might go
>> beyond the size that we wanted, but I think it's better than printing
>> a non-unique short abbreviation.
>>
>> I have one suggested change, which is to drop O_LENGTH and have
>> O_SHORT store the length always, setting it to DEFAULT_ABBREV when no
>> length provided. This allows you to drop some code. I don't think it's
>> actually worth a re-roll by itself since the current code is correct.
>>
>> Thanks,
>> Jake
>>
>
> That does make sense, It would also not error out when we use
> %(objectname:short=) and
> not specify the length. Idk, if that's desirable or not. But it does
> make the code a little more
> confusing to read at the same time.
>

I am not sure that would be the case. If you see "objectname:short"
you trreat this as if they had passed "objectname:short=" but if you see "objectname:short=" you die, no?

> So since its a small change, I'd be okay going either ways with this.
>
> --
> Regards,
> Karthik Nayak

Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Jacob Keller]

On Thu, Nov 10, 2016 at 9:31 AM, Karthik Nayak  wrote:
> On Wed, Nov 9, 2016 at 4:52 AM, Jacob Keller  wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>>
>> Ok. How does this handle whitespace? The previous if implementation
>> treated whitespace as trimming to ignore. Does this require an exact
>> whitespace match? It appears by the code that strings must match
>> exactly. Would it make more sense to always trim the value of
>> whitespace first before comparison? Hmm.. I think we should avoid
>> doing that actually.
>>
>
> This does not trim whitespace what so ever and requires an exact match.
> I don't see the reason behind trimming whitespace though.
>

The comment was made in reference to the fact that %(if)%(then)
essentially trims whitespace, since if the  prints only
whitespace it does not match.

Thus, it might make sense to keep it all the same and have this trim
as well. However, I think there is no benefit or reasoning to do so in
this case.

Thanks,
Jake

Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Jacob Keller]

On Thu, Nov 10, 2016 at 3:26 PM, Junio C Hamano  wrote:
> Jacob Keller  writes:
>
>>> @@ -49,6 +51,10 @@ static struct used_atom {
>>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
>>> C_SUB } option;
>>> unsigned int nlines;
>>> } contents;
>>> +   struct {
>>> +   const char *if_equals,
>>> +   *not_equals;
>>
>>
>> Same here, why do we need both strings here stored separately? Could
>> we instead store which state to check and store the string once? I'm
>> not sure that really buys us any storage.
>
> I am not sure if storage is an issue, but I tend to agree that it
> would be semantically cleaner if this was done as a pair of  operation uses this string constant?, the string constant>, and the
> former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
>
> You could later enhance the comparison operator more easily with
> such an arrangement (e.g. if-equals-case-insensitively).

The main advantage I see is that it ensures in the API that there is
no way for it to be both "equal" and "not equal" at the same time,
where as two separate pointers while not actually done in practice
could both be set somehow, leading to potential questions.

Thanks,
Jake

Re: [PATCH] fetch: do not redundantly calculate tag refmap

[Jeff King]

On Thu, Nov 10, 2016 at 04:13:28PM -0800, Jonathan Tan wrote:

> builtin/fetch.c redundantly calculates refmaps for tags twice. Remove
> the first calculation.
> 
> This is only a code simplification and slight performance improvement -
> the result is unchanged, as the redundant refmaps are subsequently
> removed by the invocation to "ref_remove_duplicates" anyway.
> 
> This was introduced in commit c5a84e9 ("fetch --tags: fetch tags *in
> addition to* other stuff", 2013-10-29) when modifying the effect of the
> --tags parameter to "git fetch". The refmap-for-tag calculation was
> copied instead of moved.
> 
> Signed-off-by: Jonathan Tan 
> ---
> 
> (I noticed this when working on something in this file.)

Coincidentally I noticed this a few weeks ago, too, while working on
[1], but didn't follow it up. Mainly I was worried there was something
hidden or clever going on, but I think it really is just a case of the
code growing the two calls over time. So this looks good to me. Thanks
for digging it up.

-Peff

[1] 
http://public-inbox.org/git/20161024132932.i42rqn2vlpocq...@sigill.intra.peff.net/

Re: 2.11.0-rc1 will not be tagged for a few days

[Junio C Hamano]

Junio C Hamano  writes:

> Junio C Hamano  writes:
>
>> I'll report back an updated schedule when able.
>
> I pushed some updates out on 'master' today.  Between 'master' and
> 'pu' on the first-parent history there is a merge 52975d2b1f ("Merge
> branch 'ls/macos-update' into jch", 2016-11-10) and that matches
> what I expect to be in -rc1 tomorrow (modulo RelNotes and the actual
> version tag), unless there is a showstopper regresion reported, in
> which case we may want to first look into reverting the whole series
> that introduced the regression before considering to pile on fix-up
> patches.

Please make that 71d1bcb661 ("Merge branch 'ls/macos-update' into
jch", 2016-11-10); among the three extra topics that is not yet in
'master', as/merge-attr-sleep topic has been updated with a better
fix from Dscho.

Thanks.

[PATCH] fetch: do not redundantly calculate tag refmap

[Jonathan Tan]

builtin/fetch.c redundantly calculates refmaps for tags twice. Remove
the first calculation.

This is only a code simplification and slight performance improvement -
the result is unchanged, as the redundant refmaps are subsequently
removed by the invocation to "ref_remove_duplicates" anyway.

This was introduced in commit c5a84e9 ("fetch --tags: fetch tags *in
addition to* other stuff", 2013-10-29) when modifying the effect of the
--tags parameter to "git fetch". The refmap-for-tag calculation was
copied instead of moved.

Signed-off-by: Jonathan Tan 
---

(I noticed this when working on something in this file.)

 builtin/fetch.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b6a5597..1d77e58 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -359,9 +359,6 @@ static struct ref *get_ref_map(struct transport *transport,
 
for (i = 0; i < fetch_refspec_nr; i++)
get_fetch_map(ref_map, _refspec[i], _tail, 
1);
-
-   if (tags == TAGS_SET)
-   get_fetch_map(remote_refs, tag_refspec, , 0);
} else if (refmap_array) {
die("--refmap option is only meaningful with command-line 
refspec(s).");
} else {
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH] t6026: ensure that long-running script really is

[Junio C Hamano]

Johannes Schindelin  writes:

> When making sure that background tasks are cleaned up in 5babb5b
> (t6026-merge-attr: clean up background process at end of test case,
> 2016-09-07), we considered to let the background task sleep longer, just
> to be certain that it will still be running when we want to kill it
> after the test.
>
> Sadly, the assumption appears not to hold true that the test case passes
> quickly enough to kill the background task within a second.
>
> Simply increase it to an hour. No system can be possibly slow enough to
> make above-mentioned assumption incorrect.
>
> Reported by Andreas Schwab.
>
> Signed-off-by: Johannes Schindelin 
> ---
> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1

OK, I think this is a much better option.  Assuming 3600 is long
enough for everybody (and if not, we have a bigger problem ;-),
it will ensure that the stray process will be around when we run the
'git merge' test, and by not adding "|| :" after the kill, we check
that the stray process is still there, i.e. we tested what we wanted
to test.

Will revert the two patches that were queued previously and then
queue this one.

Thanks.

>
>  t/t6026-merge-attr.sh | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 7a6e33e..348d78b 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -183,16 +183,16 @@ test_expect_success 'up-to-date merge without common 
> ancestor' '
>  
>  test_expect_success 'custom merge does not lock index' '
>   git reset --hard anchor &&
> - write_script sleep-one-second.sh <<-\EOF &&
> - sleep 1 &
> + write_script sleep-an-hour.sh <<-\EOF &&
> + sleep 3600 &
>   echo $! >sleep.pid
>   EOF
>   test_when_finished "kill \$(cat sleep.pid)" &&
>  
>   test_write_lines >.gitattributes \
> - "* merge=ours" "text merge=sleep-one-second" &&
> + "* merge=ours" "text merge=sleep-an-hour" &&
>   test_config merge.ours.driver true &&
> - test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
> + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
>   git merge master
>  '
>  
>
> base-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Junio C Hamano]

Karthik Nayak  writes:

>   else if (!strcmp(arg, "short"))
> - atom->u.objectname = O_SHORT;
> - else
> + atom->u.objectname.option = O_SHORT;
> + else if (skip_prefix(arg, "short=", )) {
> + atom->u.objectname.option = O_LENGTH;
> + if (strtoul_ui(arg, 10, >u.objectname.length) ||
> + atom->u.objectname.length == 0)
> + die(_("positive value expected objectname:short=%s"), 
> arg);
> + if (atom->u.objectname.length < MINIMUM_ABBREV)
> + atom->u.objectname.length = MINIMUM_ABBREV;
> + } else
>   die(_("unrecognized %%(objectname) argument: %s"), arg);
>  }

Users who want to use the default-abbrev, i.e. the autoscaling one
introduced recently, must use "short", not "short=-1", with this
code (especially with the "must be at least MINIMUM_ABBREV" logic),
but I do not think it is a problem, so I think this is good.


> @@ -591,12 +601,15 @@ static int grab_objectname(const char *name, const 
> unsigned char *sha1,
>  struct atom_value *v, struct used_atom *atom)
>  {
>   if (starts_with(name, "objectname")) {
> - if (atom->u.objectname == O_SHORT) {
> + if (atom->u.objectname.option == O_SHORT) {
>   v->s = xstrdup(find_unique_abbrev(sha1, 
> DEFAULT_ABBREV));
>   return 1;
> - } else if (atom->u.objectname == O_FULL) {
> + } else if (atom->u.objectname.option == O_FULL) {
>   v->s = xstrdup(sha1_to_hex(sha1));
>   return 1;
> + } else if (atom->u.objectname.option == O_LENGTH) {
> + v->s = xstrdup(find_unique_abbrev(sha1, 
> atom->u.objectname.length));
> + return 1;

Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Junio C Hamano]

Jacob Keller  writes:

>> @@ -49,6 +51,10 @@ static struct used_atom {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
>> C_SUB } option;
>> unsigned int nlines;
>> } contents;
>> +   struct {
>> +   const char *if_equals,
>> +   *not_equals;
>
>
> Same here, why do we need both strings here stored separately? Could
> we instead store which state to check and store the string once? I'm
> not sure that really buys us any storage.

I am not sure if storage is an issue, but I tend to agree that it
would be semantically cleaner if this was done as a pair of , and the
former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.

You could later enhance the comparison operator more easily with
such an arrangement (e.g. if-equals-case-insensitively).

Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms

[Junio C Hamano]

Karthik Nayak  writes:

>> Minor nit. I'm not sure what standard we use here at Git, but
>> traditionally, I prefer to see { } blocks on all sections even if only
>> one of them needs it. (That is, only drop the braces when every
>> section is one line.) It also looks weird with a comment since it
>> appears as multiple lines to the reader. I think the braces improve
>> readability.
>>
>> I don't know whether that's Git's code base standard or not, however.
>> It's not really worth a re-roll unless something else would need to
>> change.
>>
> I believe this is the syntax followed in Git, xdiff/xmerge.c:173 and so on.

That is a bad example for two reasons, if you mean this part:

if ((size = file->recs[i]->size) &&
file->recs[i]->ptr[size - 1] == '\n')
/* Last line; ends in LF; Is it CR/LF? */
return size > 1 &&
file->recs[i]->ptr[size - 2] == '\r';
*   if (!i)
/* The only line has no eol */
return -1;
/* Determine eol from second-to-last line */


What Jacob prefers is this:

-
if (cond)
simple;
else if (cond)
simple;
else
simple;
-
if (cond) {
simple;
} else if (cond) {
no;
longer;
simple;
} else {
simple;
}
-

That is, as long as all arms of if/else if/.../else cascade is
simple, {} is used nowhere in the cascade, but once even one of them
requires {} then all others gain {}.

Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms

[Junio C Hamano]

Jacob Keller  writes:

> Ok, so I have only one minor nit, but otherwise this looks quite good
> to me. A few comments explaining my understanding, but only one
> suggested
> change which is really a minor nit and not worth re-rolling just for it.

As you didn't snip parts you didn't comment, I'll use this to add my
own for convenience ;-)

>> +if::
>> +   Used as %(if)...%(then)...(%end) or
>> +   %(if)...%(then)...%(else)...%(end).  If there is an atom with
>> +   value or string literal after the %(if) then everything after
>> +   the %(then) is printed, else if the %(else) atom is used, then
>> +   everything after %(else) is printed. We ignore space when
>> +   evaluating the string before %(then), this is useful when we
>> +   use the %(HEAD) atom which prints either "*" or " " and we
>> +   want to apply the 'if' condition only on the 'HEAD' ref.
>> +
>>  In addition to the above, for commit and tag objects, the header
>>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>>  be used to specify the value in the header field.

I see a few instances of (%end) that were meant to be %(end).

Aren't the following two paragraphs ...

>> +When a scripting language specific quoting is in effect (i.e. one of
>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>> +atoms, replacement from every %(atom) is quoted when and only when it
>> +appears at the top-level (that is, when it appears outside
>> +%($open)...%(end)).

>> +When a scripting language specific quoting is in effect, everything
>> +between a top-level opening atom and its matching %(end) is evaluated
>> +according to the semantics of the opening atom and its result is
>> +quoted.

... saying the same thing?


>> +   }
>> +   } else if (!if_then_else->condition_satisfied)
>
> Minor nit. I'm not sure what standard we use here at Git, but
> traditionally, I prefer to see { } blocks on all sections even if only
> one of them needs it. (That is, only drop the braces when every
> section is one line.) It also looks weird with a comment since it
> appears as multiple lines to the reader. I think the braces improve
> readability.
>
> I don't know whether that's Git's code base standard or not, however.
> It's not really worth a re-roll unless something else would need to
> change.
>

In principle, we mimick the kernel style of using {} block even on a
single-liner body in if/else if/else cascade when any one of them is
not a single-liner and requires {}.  But we often ignore that when a
truly trivial single liner follows if() even if its else clause is a
big block, e.g.

if (cond)
single;
else {
big;
block;
}

I agree with you that this case should just use {} for the following
paragraph, because it is technically a single-liner, but comes with
a big comment block and is very much easier to read with {} around
it.

>> +   /*
>> +* No %(else) atom: just drop the %(then) branch if the
>> +* condition is not satisfied.
>> +*/
>> +   strbuf_reset(>output);

Thanks.

Re: [PATCH] doc: fill in omitted word

[Jeff King]

On Thu, Nov 10, 2016 at 02:07:30PM -0800, Junio C Hamano wrote:

> Kristoffer Haugsbakk  writes:
> 
> > Signed-off-by: Kristoffer Haugsbakk 
> > ---
> >  Documentation/gitcore-tutorial.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/gitcore-tutorial.txt 
> > b/Documentation/gitcore-tutorial.txt
> > index 4546fa0..9860517 100644
> > --- a/Documentation/gitcore-tutorial.txt
> > +++ b/Documentation/gitcore-tutorial.txt
> > @@ -25,7 +25,7 @@ you want to understand Git's internals.
> >  The core Git is often called "plumbing", with the prettier user
> >  interfaces on top of it called "porcelain". You may not want to use the
> >  plumbing directly very often, but it can be good to know what the
> > -plumbing does for when the porcelain isn't flushing.
> > +plumbing does for you when the porcelain isn't flushing.
> 
> I need an English teacher here to help me out, but I think this
> changes the meaning of the sentence from what the original author
> intended..

It does. I'd take the original to mean "for the case when", which is
correct (though as you note, just dropping "for" says the same thing
more succinctly).

-Peff

Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Jeff King]

On Thu, Nov 10, 2016 at 02:55:06PM -0800, Junio C Hamano wrote:

> If we ensure that the process is still running, then such a check is
> a good belt-and-suspenders way to catch a breakage in the mechanism
> we choose to ensure it.  So probably we can require that the kill in
> the "when finished" part to actually send a signal to a process that
> is still running.
> 
> Is there an equivalent to pause(2) available to shell scripts?  I
> really hate a single "sleep 3600" or anything with a magic number.

I think it is usually spelled "read 

Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Junio C Hamano]

Jeff King  writes:

> I do think the test would be a lot more obvious if it confirmed at the
> end of the test that the process was still running, as opposed to
> relying on test_when_finished to check it.

I agree that "check that the process is still running" is a wrong
thing to do in the first place.  What would it mean if the process
is no longer running?  It is a timing-dependent bug in the test;
after all we failed to produce the condition that could trigger a
bug that we are guarding against.  And "let's do '|| :'" is sweeping
the bug (not in the code we are testing, but in the test that will
fail to notice a bug we are preparing against) under the rug.  So I
agree with Dscho that we should do that first.

If we ensure that the process is still running, then such a check is
a good belt-and-suspenders way to catch a breakage in the mechanism
we choose to ensure it.  So probably we can require that the kill in
the "when finished" part to actually send a signal to a process that
is still running.

Is there an equivalent to pause(2) available to shell scripts?  I
really hate a single "sleep 3600" or anything with a magic number.

Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Jeff King]

On Thu, Nov 10, 2016 at 02:30:36PM -0800, Junio C Hamano wrote:

> As everybody knows there is no appropriate timeout value that is
> good for everybody.  I wonder if we can replace the sleep 1 with
> something like
> 
>   ( while sleep 3600; do :; done ) &
> 
> so that leaked fd will be kept even in any heavily loaded
> environment instead?

I think you may have missed:

  
http://public-inbox.org/git/16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schinde...@gmx.de/

which does roughly that. It does not loop, but I suspect 3600 is plenty
in practice.

I do think the test would be a lot more obvious if it confirmed at the
end of the test that the process was still running, as opposed to
relying on test_when_finished to check it.

-Peff

Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Junio C Hamano]

Johannes Schindelin  writes:

>> OK.  sleep.pid is a reasonable easy-to-access side effect we can
>> observe to make sure that the sleep-one-second merge driver was
>> indeed invoked, which was missing from the earlier round.
>
> No, this is incorrect. The condition that we need to know applies is that
> the script is still running, and blocking if the bug reappears.

OK, I see what you are saying, and I see a few things wrong in here:

 * First, the test is titled in a misleading way.  In the context of
   a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child
   processes must not inherit index.lock handles", 2016-08-18), it
   might have been clear enough to say "does not lock index", but
   the sleeping is to make sure that we would notice if the fd to
   the index.lock leaked to the child process by mistake, and the
   way to do so is that the child arranges the leaked fd to be kept
   open after it exits (by spawning "sleep").  The test was never
   about "does not lock index" (the driver does not take any lock by
   itself in the first place).

 * There are three possible outcome from this test:

   - 'git merge' fails.

 This is expected to happen only on Windows and if the code gets
 broken and starts leaking the fd.

   - 'git merge' finishes correctly, the sleep is still running when
 test_when_finished goes to cull it.

 In this case, we KNOW there wasn't any fd leak IF we are on
 Windows where a leaked FD would not allow 'git merge' to
 succeed.  But on other platforms, fd leak that may cause
 trouble for Windows friends will not be caught.

   - 'git merge' finishes correctly, the sleep is no longer
 running because the machine was heavily loaded; a workaround is
 to tolerate failure of culling it.

 In this case, we cannot tell anything from the test.  Even if
 the fd was leaked, 'git merge' may have succeeded even on
 Windows.

As everybody knows there is no appropriate timeout value that is
good for everybody.  I wonder if we can replace the sleep 1 with
something like

( while sleep 3600; do :; done ) &

so that leaked fd will be kept even in any heavily loaded
environment instead?

Re: [PATCH] doc: fill in omitted word

[Junio C Hamano]

Kristoffer Haugsbakk  writes:

> Signed-off-by: Kristoffer Haugsbakk 
> ---
>  Documentation/gitcore-tutorial.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/gitcore-tutorial.txt 
> b/Documentation/gitcore-tutorial.txt
> index 4546fa0..9860517 100644
> --- a/Documentation/gitcore-tutorial.txt
> +++ b/Documentation/gitcore-tutorial.txt
> @@ -25,7 +25,7 @@ you want to understand Git's internals.
>  The core Git is often called "plumbing", with the prettier user
>  interfaces on top of it called "porcelain". You may not want to use the
>  plumbing directly very often, but it can be good to know what the
> -plumbing does for when the porcelain isn't flushing.
> +plumbing does for you when the porcelain isn't flushing.

I need an English teacher here to help me out, but I think this
changes the meaning of the sentence from what the original author
intended..

The way I've read this statement for the past ten years since it was
written originally at 8c7fa2478e ("Add first cut at a simple git
tutorial.", 2005-05-31) and then slightly reworded at f35ca9ed3e
("tutorial.txt: start describing how to copy repositories",
2005-06-01) is that 

 * there are unfortunate occasions in which the porcelain is not
   flushing, and 

 * the knowledge of what the plumbing does is a good thing to have
   for such occasions.  It will help you figure out what needs to be
   done.

The rewritten text means a very different thing, at least to me.

 * there are things plumbing does for you.

 * the knowledge of what they are helps when the porcelain isn't
   flushing.

Just dropping "for" instead of adding "you" may make it easier to
read, grammatically more correct and retain the original intent
better, I suspect.

Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Johannes Schindelin]

Hi all,

On Thu, 10 Nov 2016, Junio C Hamano wrote:

> Andreas Schwab  writes:
> 
> > Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end
> > of test case") added a kill command to clean up after the test, but this
> > can fail if the sleep command exits before the cleanup is executed.
> > Ignore the error from the kill command.
> >
> > Explicitly check for the existence of the pid file to test that the merge
> > driver was actually called.
> >
> > Signed-off-by: Andreas Schwab 
> > ---
> 
> OK.  sleep.pid is a reasonable easy-to-access side effect we can
> observe to make sure that the sleep-one-second merge driver was
> indeed invoked, which was missing from the earlier round.

No, this is incorrect. The condition that we need to know applies is that
the script is still running, and blocking if the bug reappears.

It is not enough to test that the script *started* running. If it exits
too early, the problematic condition is never tested, and the test is
useless.

Ciao,
Johannes

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Jeff King]

On Thu, Nov 10, 2016 at 01:49:52PM -0800, Junio C Hamano wrote:

> Yes, I recall the IIS one raised and discussed at least twice on the
> list in the past, and it sounded that we want some solution to that.

The patches had some issues. I suspect the population of people who want
to run a git server on IIS is relatively small. I am content to wait for
somebody who has such a setup to produce a working patch.

> >   3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
> >  a bundle file (Apache gives you a 404, lighttpd serves the bundle).
> 
> That's a bad one.  Do we want a client-side "I am connecting to a
> site that knows how to talk smart-http" option or something to work
> it around?

It doesn't matter because we don't actually support fetching HTTP
bundles via "git fetch" yet.

But I ran it across the issue and did make such a fix when I was
implementing that feature long ago. See the discussion of "surprise" in
[1] and [2].

Wow, that series is exactly 5 years old today. Have I really been
procrastinating on re-rolling it that long? Yikes.

-Peff

[1] http://public-inbox.org/git/2010075052.gi27...@sigill.intra.peff.net/

[2] http://public-inbox.org/git/2010075052.gi27...@sigill.intra.peff.net/

Re: "git subtree --squash" interacts poorly with revert, merge, and rebase

[Matt McCutchen]

On Wed, 2016-10-26 at 19:07 -0400, Matt McCutchen wrote:
> Maybe we would never hit any of these problems in practice, but they
> give me a bad enough feeling that I'm planning to write my own tool
> that tracks the upstream commit ID in a file (like a submodule) and
> doesn't generate any extra commits.  Without generating extra commits,
> the only place to store the upstream content in the superproject would
> be in another subtree, which would take up disk space in every working
> tree unless developers manually set skip-worktree.  I think I prefer to
> not store the upstream content and just have the tool fetch it from a
> local subproject repository each time it's needed.
> 
> I'll of course post the tool on the web and would be happy to see it
> integrated into "git subtree" if that makes sense, but I don't know how
> much time I'd be willing to put into making that happen.

I have named my tool "git subtree-lite" and posted it here:

https://mattmccutchen.net/utils/git-subtree-lite.git/

For now, please email any bug reports, enhancement requests, or
proposed patches to me.  I have philosophical concerns about hosting my
own projects on services I don't control and practical concerns about a
 few "forge" apps that I looked into installing on my own web site, but
if people are seriously interested in collaborating on this, I'll work
something out.

Matt

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Junio C Hamano]

Jeff King  writes:

>> I however do not know what the universally available simplest dummy
>> HTTP server would be.  There probably are better alternative than
>> Apache with distro-customized ways of configuration that we have to
>> adjust.  
>> 
>> A solution around HTTP::Server::Simple sounds attractive but is it
>> a realistic alternative or too much effort required?  I dunno.
>
> I'm less concerned about the amount of effort (though I agree it may be
> a blocker) than about the fact that it may not behave similarly to real
> servers.

I was wondering if it takes too much effort to make it behave
similarly to real servers, so I guess we are on the same page.  

Yes, I recall the IIS one raised and discussed at least twice on the
list in the past, and it sounded that we want some solution to that.

>   3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
>  a bundle file (Apache gives you a 404, lighttpd serves the bundle).

That's a bad one.  Do we want a client-side "I am connecting to a
site that knows how to talk smart-http" option or something to work
it around?

Re: [git-for-windows] [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0

[Johannes Schindelin]

Hi Lars,

On Wed, 9 Nov 2016, Lars Schneider wrote:

> On 05 Nov 2016, at 10:50, Johannes Schindelin  
> wrote:
> 
> > I finally got around to rebase the Windows-specific patches (which seem to
> > not make it upstream as fast as we get new ones) on top of upstream Git
> > v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
> > 
> > https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
> 
> 
> I tested a new feature in 2.11 on Windows today and it failed. After some 
> confusion I realized that the feature is not on your 2.11 branch.

Oops. That must have been a major snafu on my side, very sorry for that.

I just tagged v2.11.0-rc0.windows.2 in https://github.com/dscho/git and
will make a new prerelease tomorrow.

My apologies!
Dscho

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Jeff King]

On Thu, Nov 10, 2016 at 01:33:29PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > IMHO, the value in the http tests is not testing the server side, but
> > the client side. Without being able to set up a dummy HTTP server, we do
> > not have any way to exercise the client side of git-over-http at all.
> > And people on macOS _do_ use that. :)
> 
> Amen to that.
> 
> I however do not know what the universally available simplest dummy
> HTTP server would be.  There probably are better alternative than
> Apache with distro-customized ways of configuration that we have to
> adjust.  
> 
> A solution around HTTP::Server::Simple sounds attractive but is it
> a realistic alternative or too much effort required?  I dunno.

I'm less concerned about the amount of effort (though I agree it may be
a blocker) than about the fact that it may not behave similarly to real
servers. We have had real bugs and surprises with the way that various
web servers implement things. A few that come to mind are:

  1. Buffering/deadlock issues between the webserver and the CGI (this
 was an issue with Apache, but I don't know about other servers).

  2. The handling of CONTENT_LENGTH with chunked-encoding (this is still
 an issue with IIS).

  3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
 a bundle file (Apache gives you a 404, lighttpd serves the bundle).

Ideally we'd test against a lot of different webservers, but that's
expensive (in CPU, but also in developer time). But I'd guess that
Apache is at least more representative than HTTP::Server::Simple of real
servers in the wild.

So if we had a simple fallback in addition to Apache, I'd be OK with
that. But we still have a problem that (say) people on MacOS would never
actually test against Apache, because it's not supported there.

-Peff

Re: 2.11.0-rc1 will not be tagged for a few days

[Junio C Hamano]

Junio C Hamano  writes:

> I'll report back an updated schedule when able.

I pushed some updates out on 'master' today.  Between 'master' and
'pu' on the first-parent history there is a merge 52975d2b1f ("Merge
branch 'ls/macos-update' into jch", 2016-11-10) and that matches
what I expect to be in -rc1 tomorrow (modulo RelNotes and the actual
version tag), unless there is a showstopper regresion reported, in
which case we may want to first look into reverting the whole series
that introduced the regression before considering to pile on fix-up
patches.

http://tinyurl.com/gitCal has been redrawn, with -rc1 scheduled for
tomorrow on 11th, -rc2 on the 17th, and final on the 23rd.

Thanks.

Re: [PATCH v1 0/2] Fix default macOS build locally and on Travis CI

[Junio C Hamano]

Lars Schneider  writes:

>> I've followed what was available at the public-inbox archive, but it
>> is unclear what the conclusion was.  
>> 
>> For the first one your "how about" non-patch, to which Peff said
>> "that's simple and good", looked good to me as well, but is it
>> available as a final patch that I can just take and apply (otherwise
>> I think I can do the munging myself, but I'd rather be spoon-fed
>> when able ;-).
>
> Sure! Here you go:
> http://public-inbox.org/git/20161110111348.61580-1-larsxschnei...@gmail.com/
>
>
>> I do not have a strong opinion on the second one.  For an interim
>> solution, disabling webserver tests certainly is expedite and safe,
>> so I am fine taking it as-is, but I may have missed strong
>> objections.
>
> I haven't seen strong objections either. Just for reference, here is the 
> patch:
> http://public-inbox.org/git/20161017002550.88782-3-larsxschnei...@gmail.com/

Thanks.  Picked up both of them.

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Junio C Hamano]

Jeff King  writes:

> IMHO, the value in the http tests is not testing the server side, but
> the client side. Without being able to set up a dummy HTTP server, we do
> not have any way to exercise the client side of git-over-http at all.
> And people on macOS _do_ use that. :)

Amen to that.

I however do not know what the universally available simplest dummy
HTTP server would be.  There probably are better alternative than
Apache with distro-customized ways of configuration that we have to
adjust.  

A solution around HTTP::Server::Simple sounds attractive but is it
a realistic alternative or too much effort required?  I dunno.

[PATCH 17/35] attr: support quoting pathname patterns in C style

[Stefan Beller]

From: Nguyễn Thái Ngọc Duy 

Full pattern must be quoted. So 'pat"t"ern attr' will give exactly
'pat"t"ern', not 'pattern'. Also clarify that leading whitespaces are
not part of the pattern and document comment syntax.

Signed-off-by: Nguyễn Thái Ngọc Duy 
Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/gitattributes.txt |  8 +---
 attr.c  | 15 +--
 t/t0003-attributes.sh   | 26 ++
 3 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 7aff940..8a061af 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -21,9 +21,11 @@ Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
 
 That is, a pattern followed by an attributes list,
-separated by whitespaces.  When the pattern matches the
-path in question, the attributes listed on the line are given to
-the path.
+separated by whitespaces. Leading and trailing whitespaces are
+ignored. Lines that begin with '#' are ignored. Patterns
+that begin with a double quote are quoted in C style.
+When the pattern matches the path in question, the attributes
+listed on the line are given to the path.
 
 Each attribute can be in one of these states for a given path:
 
diff --git a/attr.c b/attr.c
index 14bbfd4..88fcb7c 100644
--- a/attr.c
+++ b/attr.c
@@ -13,6 +13,7 @@
 #include "attr.h"
 #include "dir.h"
 #include "utf8.h"
+#include "quote.h"
 
 const char git_attr__true[] = "(builtin)true";
 const char git_attr__false[] = "\0(builtin)false";
@@ -225,12 +226,21 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
const char *cp, *name, *states;
struct match_attr *res = NULL;
int is_macro;
+   struct strbuf pattern = STRBUF_INIT;
 
cp = line + strspn(line, blank);
if (!*cp || *cp == '#')
return NULL;
name = cp;
-   namelen = strcspn(name, blank);
+
+   if (*cp == '"' && !unquote_c_style(, name, )) {
+   name = pattern.buf;
+   namelen = pattern.len;
+   } else {
+   namelen = strcspn(name, blank);
+   states = name + namelen;
+   }
+
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
if (!macro_ok) {
@@ -250,7 +260,6 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
else
is_macro = 0;
 
-   states = name + namelen;
states += strspn(states, blank);
 
/* First pass to count the attr_states */
@@ -293,9 +302,11 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
cannot_trust_maybe_real = 1;
}
 
+   strbuf_release();
return res;
 
 fail_return:
+   strbuf_release();
free(res);
return NULL;
 }
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb42..f19ae4f 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -13,10 +13,31 @@ attr_check () {
test_line_count = 0 err
 }
 
+attr_check_quote () {
+
+   path="$1"
+   quoted_path="$2"
+   expect="$3"
+
+   git check-attr test -- "$path" >actual &&
+   echo "\"$quoted_path\": test: $expect" >expect &&
+   test_cmp expect actual
+
+}
+
+test_expect_success 'open-quoted pathname' '
+   echo "\"a test=a" >.gitattributes &&
+   test_must_fail attr_check a a
+'
+
+
 test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
echo "[attr]notest !test"
+   echo "\" d \"   test=d"
+   echo " etest=e"
+   echo " e\"  test=e"
echo "f test=f"
echo "a/i test=a/i"
echo "onoff test -test"
@@ -69,6 +90,11 @@ test_expect_success 'command line checks' '
 '
 
 test_expect_success 'attribute test' '
+
+   attr_check " d " d &&
+   attr_check e e &&
+   attr_check_quote e\" e\\\" e &&
+
attr_check f f &&
attr_check a/f f &&
attr_check a/c/f f &&
-- 
2.10.1.469.g00a8914

[PATCH 03/35] attr.c: update a stale comment on "struct match_attr"

[Stefan Beller]

From: Junio C Hamano 

When 82dce998 (attr: more matching optimizations from .gitignore,
2012-10-15) changed a pointer to a string "*pattern" into an
embedded "struct pattern" in struct match_attr, it forgot to update
the comment that describes the structure.

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

diff --git a/attr.c b/attr.c
index 45aec1b..4ae7801 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
  * If is_macro is true, then u.attr is a pointer to the git_attr being
  * defined.
  *
- * If is_macro is false, then u.pattern points at the filename pattern
- * to which the rule applies.  (The memory pointed to is part of the
- * memory block allocated for the match_attr instance.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
  *
  * In either case, num_attr is the number of attributes affected by
  * this rule, and state is an array listing them.  The attributes are
-- 
2.10.1.469.g00a8914

[PATCH 30/35] pathspec: move prefix check out of the inner loop

[Stefan Beller]

The prefix check is not related the check of pathspec magic; also there
is no code that is relevant after we'd break the loop on a match for
"prefix:". So move the check before the loop and shortcircuit the outer
loop.

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

diff --git a/pathspec.c b/pathspec.c
index 67678fc..d44f8e7 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -107,21 +107,22 @@ static void eat_long_magic(struct pathspec_item *item, 
const char *elt,
nextat = copyfrom + len;
if (!len)
continue;
+
+   if (starts_with(copyfrom, "prefix:")) {
+   char *endptr;
+   *pathspec_prefix = strtol(copyfrom + 7,
+ , 10);
+   if (endptr - copyfrom != len)
+   die(_("invalid parameter for pathspec magic 
'prefix'"));
+   continue;
+   }
+
for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
if (strlen(pathspec_magic[i].name) == len &&
!strncmp(pathspec_magic[i].name, copyfrom, len)) {
*magic |= pathspec_magic[i].bit;
break;
}
-   if (starts_with(copyfrom, "prefix:")) {
-   char *endptr;
-   *pathspec_prefix = strtol(copyfrom + 7,
- , 10);
-   if (endptr - copyfrom != len)
-   die(_("invalid parameter for pathspec 
magic 'prefix'"));
-   /* "i" would be wrong, but it does not matter */
-   break;
-   }
}
if (ARRAY_SIZE(pathspec_magic) <= i)
die(_("Invalid pathspec magic '%.*s' in '%s'"),
-- 
2.10.1.469.g00a8914

[PATCH 04/35] attr.c: explain the lack of attr-name syntax check in parse_attr()

[Stefan Beller]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index 4ae7801..05db667 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, 
const char *cp,
return NULL;
}
} else {
+   /*
+* As this function is always called twice, once with
+* e == NULL in the first pass and then e != NULL in
+* the second pass, no need for invalid_attr_name()
+* check here.
+*/
if (*cp == '-' || *cp == '!') {
e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
cp++;
-- 
2.10.1.469.g00a8914

[PATCH 28/35] Documentation: fix a typo

[Stefan Beller]

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/gitattributes.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 8a061af..5b31797 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -88,7 +88,7 @@ is either not set or empty, $HOME/.config/git/attributes is 
used instead.
 Attributes for all users on a system should be placed in the
 `$(prefix)/etc/gitattributes` file.
 
-Sometimes you would need to override an setting of an attribute
+Sometimes you would need to override a setting of an attribute
 for a path to `Unspecified` state.  This can be done by listing
 the name of the attribute prefixed with an exclamation point `!`.
 
-- 
2.10.1.469.g00a8914

[PATCH 35/35] completion: clone can initialize specific submodules

[Stefan Beller]

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 contrib/completion/git-completion.bash | 1 +
 1 file changed, 1 insertion(+)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 21016bf..90eb772 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1138,6 +1138,7 @@ _git_clone ()
--single-branch
--branch
--recurse-submodules
+   --init-submodule
"
return
;;
-- 
2.10.1.469.g00a8914

[PATCH 29/35] pathspec: move long magic parsing out of prefix_pathspec

[Stefan Beller]

`prefix_pathspec` is quite a lengthy function and we plan on adding more.
Split it up for better readability. As we want to add code into the
inner loop of the long magic parsing, we also benefit from lower
indentation.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 pathspec.c | 84 +++---
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 86f2b44..67678fc 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -88,6 +88,52 @@ static void prefix_short_magic(struct strbuf *sb, int 
prefixlen,
strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static void eat_long_magic(struct pathspec_item *item, const char *elt,
+   unsigned *magic, int *pathspec_prefix,
+   const char **copyfrom_, const char **long_magic_end)
+{
+   int i;
+   const char *copyfrom = *copyfrom_;
+   /* longhand */
+   const char *nextat;
+   for (copyfrom = elt + 2;
+*copyfrom && *copyfrom != ')';
+copyfrom = nextat) {
+   size_t len = strcspn(copyfrom, ",)");
+   if (copyfrom[len] == ',')
+   nextat = copyfrom + len + 1;
+   else
+   /* handle ')' and '\0' */
+   nextat = copyfrom + len;
+   if (!len)
+   continue;
+   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
+   if (strlen(pathspec_magic[i].name) == len &&
+   !strncmp(pathspec_magic[i].name, copyfrom, len)) {
+   *magic |= pathspec_magic[i].bit;
+   break;
+   }
+   if (starts_with(copyfrom, "prefix:")) {
+   char *endptr;
+   *pathspec_prefix = strtol(copyfrom + 7,
+ , 10);
+   if (endptr - copyfrom != len)
+   die(_("invalid parameter for pathspec 
magic 'prefix'"));
+   /* "i" would be wrong, but it does not matter */
+   break;
+   }
+   }
+   if (ARRAY_SIZE(pathspec_magic) <= i)
+   die(_("Invalid pathspec magic '%.*s' in '%s'"),
+   (int) len, copyfrom, elt);
+   }
+   if (*copyfrom != ')')
+   die(_("Missing ')' at the end of pathspec magic in '%s'"), elt);
+   *long_magic_end = copyfrom;
+   copyfrom++;
+   *copyfrom_ = copyfrom;
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -150,43 +196,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item,
(flags & PATHSPEC_LITERAL_PATH)) {
; /* nothing to do */
} else if (elt[1] == '(') {
-   /* longhand */
-   const char *nextat;
-   for (copyfrom = elt + 2;
-*copyfrom && *copyfrom != ')';
-copyfrom = nextat) {
-   size_t len = strcspn(copyfrom, ",)");
-   if (copyfrom[len] == ',')
-   nextat = copyfrom + len + 1;
-   else
-   /* handle ')' and '\0' */
-   nextat = copyfrom + len;
-   if (!len)
-   continue;
-   for (i = 0; i < ARRAY_SIZE(pathspec_magic); i++) {
-   if (strlen(pathspec_magic[i].name) == len &&
-   !strncmp(pathspec_magic[i].name, copyfrom, 
len)) {
-   magic |= pathspec_magic[i].bit;
-   break;
-   }
-   if (starts_with(copyfrom, "prefix:")) {
-   char *endptr;
-   pathspec_prefix = strtol(copyfrom + 7,
-, 10);
-   if (endptr - copyfrom != len)
-   die(_("invalid parameter for 
pathspec magic 'prefix'"));
-   /* "i" would be wrong, but it does not 
matter */
-   break;
-   }
-   }
-   if (ARRAY_SIZE(pathspec_magic) <= i)
-   die(_("Invalid pathspec magic '%.*s' in '%s'"),
-   (int) len, copyfrom, elt);
-   }
-   if (*copyfrom != ')')
- 

[PATCH 34/35] clone: add --init-submodule= switch

[Stefan Beller]

The new switch passes the pathspec to `git submodule update --init`
which is called after the actual clone is done.

Additionally this configures the submodule.defaultUpdatePath to
be the given pathspec, such that any future invocation of
`git submodule update --init-default-paths` will keep up
with the pathspec.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt|  2 +-
 Documentation/git-clone.txt | 23 +
 builtin/clone.c | 36 ++--
 t/t7400-submodule-basic.sh  | 81 +
 4 files changed, 132 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 290de3b..917904e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2887,7 +2887,7 @@ submodule.alternateErrorStrategy
 
 submodule.defaultUpdatePath::
Specifies a set of submodules to initialize when calling
-   `git submodule --init-default-group` by using the pathspec
+   `git submodule --init-default-path` by using the pathspec
syntax.
 
 tag.forceSignAnnotated::
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 35cc34b..1089f38 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -15,7 +15,8 @@ SYNOPSIS
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
  [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
- [--jobs ] [--]  []
+ [--init-submodule ] [--jobs ] [--]
+  []
 
 DESCRIPTION
 ---
@@ -217,12 +218,20 @@ objects from the source repository into a pack in the 
cloned repository.
 
 --recursive::
 --recurse-submodules::
-   After the clone is created, initialize all submodules within,
-   using their default settings. This is equivalent to running
-   `git submodule update --init --recursive` immediately after
-   the clone is finished. This option is ignored if the cloned
-   repository does not have a worktree/checkout (i.e. if any of
-   `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+   After the clone is created, initialize and clone all submodules
+   within, using their default settings. This is equivalent to
+   running `git submodule update --recursive --init `
+   immediately after the clone is finished. This option is ignored
+   if the cloned repository does not have a worktree/checkout (i.e.
+   if any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
+
+--init-submodule::
+   After the clone is created, initialize and clone specified
+   submodules within, using their default settings. It is possible
+   to give multiple specifications by giving this argument multiple
+   times. This is equivalent to configure `submodule.defaultUpdateGroup`
+   and then running `git submodule update --init-default-path`
+   immediately after the clone is finished.
 
 --[no-]shallow-submodules::
All submodules which are cloned will be shallow with a depth of 1.
diff --git a/builtin/clone.c b/builtin/clone.c
index 6c80690..ef924c6 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -56,6 +56,16 @@ static struct string_list option_required_reference = 
STRING_LIST_INIT_NODUP;
 static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
 static int option_dissociate;
 static int max_jobs = -1;
+static struct string_list init_submodules;
+
+static int init_submodules_cb(const struct option *opt, const char *arg, int 
unset)
+{
+   if (unset)
+   return -1;
+
+   string_list_append((struct string_list *)opt->value, arg);
+   return 0;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -112,6 +122,9 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "init-submodule", _submodules, N_(""),
+   N_("clone specific submodules. Pass multiple times for 
complex pathspecs"),
+   init_submodules_cb),
OPT_END()
 };
 
@@ -730,13 +743,21 @@ static int checkout(int submodule_progress)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive) {
+   if (!err && (option_recursive || init_submodules.nr > 0)) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(, "submodule", "update", NULL);
+
+   if (init_submodules.nr > 0)
+   argv_array_pushf(, "--init-default-path");
+   else
+   

[PATCH 33/35] submodule update: add `--init-default-path` switch

[Stefan Beller]

The new switch `--init-default-path` initializes the submodules which are
configured in `submodule.defaultUpdatePath` instead of those given as
command line arguments before updating. In the first implementation this
is made incompatible with further command line arguments as it is
unclear what the user means by

git submodule update --init --init-default-path 

This new switch allows to record more complex patterns as it saves
retyping them whenever you invoke update.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/config.txt|  5 
 Documentation/git-submodule.txt | 17 +
 git-submodule.sh| 21 +---
 t/t7400-submodule-basic.sh  | 53 +
 4 files changed, 89 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a179474..290de3b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2885,6 +2885,11 @@ submodule.alternateErrorStrategy
as computed via `submodule.alternateLocation`. Possible values are
`ignore`, `info`, `die`. Default is `die`.
 
+submodule.defaultUpdatePath::
+   Specifies a set of submodules to initialize when calling
+   `git submodule --init-default-group` by using the pathspec
+   syntax.
+
 tag.forceSignAnnotated::
A boolean to specify whether annotated tags created should be GPG 
signed.
If `--annotate` is specified on the command line, it takes
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bf3bb37..503fec8 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -14,10 +14,10 @@ SYNOPSIS
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] (--all|[--] ...)
-'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [--[no-]recommend-shallow] [-f|--force] [--rebase|--merge]
- [--reference ] [--depth ] [--recursive]
- [--jobs ] [--] [...]
+'git submodule' [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch]
+ [--[no-]recommend-shallow]
+ [-f|--force] [--rebase|--merge] [--reference ]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -194,6 +194,10 @@ If the submodule is not yet initialized, and you just want 
to use the
 setting as stored in .gitmodules, you can automatically initialize the
 submodule with the `--init` option.
 
+You can configure a set of submodules using pathspec syntax in
+submodule.defaultUpdatePath you can use `--init-default-path` to initialize
+those before updating.
+
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 --
@@ -361,6 +365,11 @@ the submodule itself.
Initialize all submodules for which "git submodule init" has not been
called so far before updating.
 
+--init-default-path::
+   This option is only valid for the update command.
+   Initialize all submodules configured in "`submodule.defaultUpdatePath`"
+   that have not been updated before.
+
 --name::
This option is only valid for the add command. It sets the submodule's
name to the given string instead of defaulting to its path. The name
diff --git a/git-submodule.sh b/git-submodule.sh
index a024a13..334cecc 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] (--all| [--] ...)
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--checkout|--merge|--rebase] [--[no-]recommend-shallow] 
[--reference ] [--recursive] [--] [...]
+   or: $dashless [--quiet] update [--init[-default-path]] [--remote] 
[-N|--no-fetch] [-f|--force] [--checkout|--merge|--rebase] 
[--[no-]recommend-shallow] [--reference ] [--recursive] [--] 
[...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -503,7 +503,12 @@ cmd_update()
progress="--progress"
;;
-i|--init)
-   init=1
+   test -z $init || test $init = by_args || die "$(gettext 
"Only one of --init or --init-default-path may be used.")"
+   init=by_args
+   ;;
+   --init-default-path)
+   test -z $init || test 

[PATCH 32/35] pathspec: allow escaped query values

[Stefan Beller]

In our own .gitattributes file we have attributes such as:

*.[ch] whitespace=indent,trail,space

When querying for attributes we want to be able to ask for the exact
value, i.e.

git ls-files :(attr:whitespace=indent,trail,space)

should work, but the commas are used in the attr magic to introduce
the next attr, such that this query currently fails with

fatal: Invalid pathspec magic 'trail' in ':(attr:whitespace=indent,trail,space)'

This change allows escaping characters by a backslash, such that the query

git ls-files :(attr:whitespace=indent\,trail\,space)

will match all path that have the value "indent,trail,space" for the
whitespace attribute. To accomplish this, we need to modify two places.
First `eat_long_magic` needs to not stop early upon seeing a comma or
closing paren that is escaped. As a second step we need to remove any
escaping from the attr value.

Helped-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 pathspec.c  | 53 +
 t/t6134-pathspec-with-labels.sh | 10 
 2 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 4aa7ea5..d702803 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -90,13 +90,57 @@ static void prefix_short_magic(struct strbuf *sb, int 
prefixlen,
strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static size_t strcspn_escaped(const char *s, const char *stop)
+{
+   const char *i;
+
+   for (i = s; *i; i++) {
+   /* skip the escaped character */
+   if (i[0] == '\\' && i[1]) {
+   i++;
+   continue;
+   }
+
+   if (strchr(stop, *i))
+   break;
+   }
+   return i - s;
+}
+
+static inline int invalid_value_char(const char ch)
+{
+   if (isalnum(ch) || strchr(",-_", ch))
+   return 0;
+   return -1;
+}
+
+static char *attr_value_unescape(const char *value)
+{
+   const char *src;
+   char *dst, *ret;
+
+   ret = xmallocz(strlen(value));
+   for (src = value, dst = ret; *src; src++, dst++) {
+   if (*src == '\\') {
+   if (!src[1])
+   die(_("Escape character '\\' not allowed as "
+ "last character in attr value"));
+   src++;
+   }
+   if (invalid_value_char(*src))
+   die("cannot use '%c' for value matching", *src);
+   *dst = *src;
+   }
+   *dst = '\0';
+   return ret;
+}
+
 static void parse_pathspec_attr_match(struct pathspec_item *item, const char 
*value)
 {
struct string_list_item *si;
struct string_list list = STRING_LIST_INIT_DUP;
struct argv_array attrs = ARGV_ARRAY_INIT;
 
-
if (!value || !strlen(value))
die(_("attr spec must not be empty"));
 
@@ -128,10 +172,9 @@ static void parse_pathspec_attr_match(struct pathspec_item 
*item, const char *va
if (attr[attr_len] != '=')
am->match_mode = MATCH_SET;
else {
+   const char *v = [attr_len + 1];
am->match_mode = MATCH_VALUE;
-   am->value = xstrdup([attr_len + 1]);
-   if (strchr(am->value, '\\'))
-   die(_("attr spec values must not 
contain backslashes"));
+   am->value = attr_value_unescape(v);
}
break;
}
@@ -168,7 +211,7 @@ static void eat_long_magic(struct pathspec_item *item, 
const char *elt,
for (copyfrom = elt + 2;
 *copyfrom && *copyfrom != ')';
 copyfrom = nextat) {
-   size_t len = strcspn(copyfrom, ",)");
+   size_t len = strcspn_escaped(copyfrom, ",)");
if (copyfrom[len] == ',')
nextat = copyfrom + len + 1;
else
diff --git a/t/t6134-pathspec-with-labels.sh b/t/t6134-pathspec-with-labels.sh
index e276811..5aafc0c 100755
--- a/t/t6134-pathspec-with-labels.sh
+++ b/t/t6134-pathspec-with-labels.sh
@@ -188,4 +188,14 @@ test_expect_success 'abort on asking for wrong magic' '
test_must_fail git ls-files . ":(attr:!label=foo)"
 '
 
+test_expect_success 'check attribute list' '
+   cat <<-EOF >>.gitattributes &&
+   * whitespace=indent,trail,space
+   EOF
+   cat .gitattributes &&
+   git ls-files ":(attr:whitespace=indent\,trail\,space)" >actual &&
+   git ls-files >expect &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.10.1.469.g00a8914

[PATCH 15/35] attr: add counted string version of git_check_attr()

[Stefan Beller]

From: Junio C Hamano 

Often a potential caller has  pair that
represents the path it wants to ask attributes for; when
path[pathlen] is not NUL, the caller has to xmemdupz()
only to call git_check_attr().

Add git_check_attr_counted() that takes such a counted
string instead of "const char *path".

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 23 ++-
 attr.h |  1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/attr.c b/attr.c
index d427798..9bec243 100644
--- a/attr.c
+++ b/attr.c
@@ -734,20 +734,19 @@ static int attr_check_is_dynamic(const struct 
git_attr_check *check)
  * check_all_attr. If num is non-zero, only attributes in check[] are
  * collected. Otherwise all attributes are collected.
  */
-static void collect_some_attrs(const char *path, int num,
+static void collect_some_attrs(const char *path, int pathlen, int num,
   struct git_attr_check_elem *check)
 
 {
struct attr_stack *stk;
-   int i, pathlen, rem, dirlen;
+   int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
 
-   for (cp = path; *cp; cp++) {
+   for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
last_slash = cp;
}
-   pathlen = cp - path;
if (last_slash) {
basename_offset = last_slash + 1 - path;
dirlen = last_slash - path;
@@ -778,12 +777,12 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
 }
 
-static int git_check_attrs(const char *path, int num,
+static int git_check_attrs(const char *path, int pathlen, int num,
   struct git_attr_check_elem *check)
 {
int i;
 
-   collect_some_attrs(path, num, check);
+   collect_some_attrs(path, pathlen, num, check);
 
for (i = 0; i < num; i++) {
const char *value = 
check_all_attr[check[i].attr->attr_nr].value;
@@ -800,7 +799,7 @@ void git_all_attrs(const char *path, struct git_attr_check 
*check)
int i;
 
git_attr_check_clear(check);
-   collect_some_attrs(path, 0, NULL);
+   collect_some_attrs(path, strlen(path), 0, NULL);
 
for (i = 0; i < attr_nr; i++) {
const char *name = check_all_attr[i].attr->name;
@@ -825,10 +824,16 @@ void git_attr_set_direction(enum git_attr_direction new, 
struct index_state *ist
use_index = istate;
 }
 
-int git_check_attr(const char *path, struct git_attr_check *check)
+int git_check_attr_counted(const char *path, int pathlen,
+  struct git_attr_check *check)
 {
check->finalized = 1;
-   return git_check_attrs(path, check->check_nr, check->check);
+   return git_check_attrs(path, pathlen, check->check_nr, check->check);
+}
+
+int git_check_attr(const char *path, struct git_attr_check *check)
+{
+   return git_check_attr_counted(path, strlen(path), check);
 }
 
 struct git_attr_check *git_attr_check_initl(const char *one, ...)
diff --git a/attr.h b/attr.h
index 506db0c..c84f164 100644
--- a/attr.h
+++ b/attr.h
@@ -38,6 +38,7 @@ struct git_attr_check {
 
 extern struct git_attr_check *git_attr_check_initl(const char *, ...);
 extern int git_check_attr(const char *path, struct git_attr_check *);
+extern int git_check_attr_counted(const char *, int, struct git_attr_check *);
 
 extern struct git_attr_check *git_attr_check_alloc(void);
 extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check 
*, const struct git_attr *);
-- 
2.10.1.469.g00a8914

[PATCH 25/35] attr: make git_check_attr_counted static

[Stefan Beller]

It's not used outside the attr code, so let's keep it private.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 4 ++--
 attr.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/attr.c b/attr.c
index 60d7eec..19a3651 100644
--- a/attr.c
+++ b/attr.c
@@ -892,8 +892,8 @@ void git_attr_set_direction(enum git_attr_direction new, 
struct index_state *ist
use_index = istate;
 }
 
-int git_check_attr_counted(const char *path, int pathlen,
-  struct git_attr_check *check)
+static int git_check_attr_counted(const char *path, int pathlen,
+ struct git_attr_check *check)
 {
check->finalized = 1;
return git_check_attrs(path, pathlen, check);
diff --git a/attr.h b/attr.h
index 1c6a6d8..7416b14 100644
--- a/attr.h
+++ b/attr.h
@@ -48,7 +48,6 @@ struct git_attr_check {
 
 extern struct git_attr_check *git_attr_check_initl(const char *, ...);
 extern int git_check_attr(const char *path, struct git_attr_check *);
-extern int git_check_attr_counted(const char *, int, struct git_attr_check *);
 
 extern struct git_attr_check *git_attr_check_alloc(void);
 extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check 
*, const struct git_attr *);
-- 
2.10.1.469.g00a8914

[PATCH 31/35] pathspec: allow querying for attributes

[Stefan Beller]

The pathspec mechanism is extended via the new
":(attr:eol=input)pattern/to/match" syntax to filter paths so that it
requires paths to not just match the given pattern but also have the
specified attrs attached for them to be chosen.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/glossary-content.txt |  20 
 dir.c  |  41 +++-
 pathspec.c | 106 +++-
 pathspec.h |  19 +++-
 t/t6134-pathspec-with-labels.sh| 191 +
 5 files changed, 370 insertions(+), 7 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index 8ad29e6..f90bd45 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -384,6 +384,26 @@ full pathname may have special meaning:
 +
 Glob magic is incompatible with literal magic.
 
+attr;;
+After `attr:` comes a space separated list of "attribute
+requirements", all of which must be met in order for the
+path to be considered a match; this is in addition to the
+usual non-magic pathspec pattern matching.
++
+Each of the attribute requirements for the path takes one of
+these forms:
+
+- "`ATTR`" requires that the attribute `ATTR` must be set.
+
+- "`-ATTR`" requires that the attribute `ATTR` must be unset.
+
+- "`ATTR=VALUE`" requires that the attribute `ATTR` must be
+  set to the string `VALUE`.
+
+- "`!ATTR`" requires that the attribute `ATTR` must be
+  unspecified.
++
+
 exclude;;
After a path matches any non-exclude pathspec, it will be run
through all exclude pathspec (magic signature: `!`). If it
diff --git a/dir.c b/dir.c
index 3bad1ad..c56d2ee 100644
--- a/dir.c
+++ b/dir.c
@@ -9,6 +9,7 @@
  */
 #include "cache.h"
 #include "dir.h"
+#include "attr.h"
 #include "refs.h"
 #include "wildmatch.h"
 #include "pathspec.h"
@@ -139,7 +140,8 @@ static size_t common_prefix_len(const struct pathspec 
*pathspec)
   PATHSPEC_LITERAL |
   PATHSPEC_GLOB |
   PATHSPEC_ICASE |
-  PATHSPEC_EXCLUDE);
+  PATHSPEC_EXCLUDE |
+  PATHSPEC_ATTR);
 
for (n = 0; n < pathspec->nr; n++) {
size_t i = 0, len = 0, item_len;
@@ -207,6 +209,37 @@ int within_depth(const char *name, int namelen,
return 1;
 }
 
+static int match_attrs(const char *name, int namelen,
+  const struct pathspec_item *item)
+{
+   int i;
+   struct git_attr_result *res = git_attr_result_alloc(item->attr_check);
+
+   git_check_attr(name, item->attr_check, res);
+   for (i = 0; i < item->attr_match_nr; i++) {
+   const char *value;
+   int matched;
+   enum attr_match_mode match_mode;
+
+   value = res[i].value;
+   match_mode = item->attr_match[i].match_mode;
+
+   if (ATTR_TRUE(value))
+   matched = (match_mode == MATCH_SET);
+   else if (ATTR_FALSE(value))
+   matched = (match_mode == MATCH_UNSET);
+   else if (ATTR_UNSET(value))
+   matched = (match_mode == MATCH_UNSPECIFIED);
+   else
+   matched = (match_mode == MATCH_VALUE &&
+  !strcmp(item->attr_match[i].value, value));
+   if (!matched)
+   return 0;
+   }
+
+   return 1;
+}
+
 #define DO_MATCH_EXCLUDE   1
 #define DO_MATCH_DIRECTORY 2
 
@@ -262,6 +295,9 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
strncmp(item->match, name - prefix, item->prefix))
return 0;
 
+   if (item->attr_match_nr && !match_attrs(name, namelen, item))
+   return 0;
+
/* If the match was just the prefix, we matched */
if (!*match)
return MATCHED_RECURSIVELY;
@@ -314,7 +350,8 @@ static int do_match_pathspec(const struct pathspec *ps,
   PATHSPEC_LITERAL |
   PATHSPEC_GLOB |
   PATHSPEC_ICASE |
-  PATHSPEC_EXCLUDE);
+  PATHSPEC_EXCLUDE |
+  PATHSPEC_ATTR);
 
if (!ps->nr) {
if (!ps->recursive ||
diff --git a/pathspec.c b/pathspec.c
index d44f8e7..4aa7ea5 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,6 +1,8 @@
 #include "cache.h"
 #include "dir.h"
 #include "pathspec.h"
+#include "attr.h"
+#include "argv-array.h"
 
 /*
  * Finds which of the given pathspecs match items in the index.
@@ -88,12 +90,79 @@ static void prefix_short_magic(struct strbuf *sb, int 
prefixlen,
strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static void parse_pathspec_attr_match(struct 

[PATCH 27/35] attr: keep attr stack for each check

[Stefan Beller]

Instead of having a global attr stack, attach the stack to each check.
This allows to use the attr in a multithreaded way.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c| 101 +++---
 attr.h|   4 ++-
 hashmap.h |   2 ++
 3 files changed, 69 insertions(+), 38 deletions(-)

diff --git a/attr.c b/attr.c
index 32ec453..979bd3c 100644
--- a/attr.c
+++ b/attr.c
@@ -375,15 +375,17 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
  * .gitignore file and info/excludes file as a fallback.
  */
 
-/* NEEDSWORK: This will become per git_attr_check */
-static struct attr_stack {
+struct attr_stack {
struct attr_stack *prev;
char *origin;
size_t originlen;
unsigned num_matches;
unsigned alloc;
struct match_attr **attrs;
-} *attr_stack;
+};
+
+static struct hashmap all_attr_stacks;
+static int all_attr_stacks_init;
 
 static void free_attr_elem(struct attr_stack *e)
 {
@@ -564,11 +566,23 @@ static void debug_set(const char *what, const char 
*match, struct git_attr *attr
 
 static void drop_attr_stack(void)
 {
-   while (attr_stack) {
-   struct attr_stack *elem = attr_stack;
-   attr_stack = elem->prev;
-   free_attr_elem(elem);
+   struct hashmap_iter iter;
+   struct git_attr_check *check;
+
+   attr_lock();
+   if (!all_attr_stacks_init) {
+   attr_unlock();
+   return;
}
+   hashmap_iter_init(_attr_stacks, );
+   while ((check = hashmap_iter_next())) {
+   while (check->attr_stack) {
+   struct attr_stack *elem = check->attr_stack;
+   check->attr_stack = elem->prev;
+   free_attr_elem(elem);
+   }
+   }
+   attr_unlock();
 }
 
 static const char *git_etc_gitattributes(void)
@@ -598,40 +612,42 @@ static void push_stack(struct attr_stack **attr_stack_p,
}
 }
 
-static void bootstrap_attr_stack(void)
+static void bootstrap_attr_stack(struct git_attr_check *check)
 {
struct attr_stack *elem;
 
-   if (attr_stack)
+   if (check->attr_stack)
return;
 
-   push_stack(_stack, read_attr_from_array(builtin_attr), NULL, 0);
+   push_stack(>attr_stack,
+  read_attr_from_array(builtin_attr), NULL, 0);
 
if (git_attr_system())
-   push_stack(_stack,
+   push_stack(>attr_stack,
   read_attr_from_file(git_etc_gitattributes(), 1),
   NULL, 0);
 
if (!git_attributes_file)
git_attributes_file = xdg_config_home("attributes");
if (git_attributes_file)
-   push_stack(_stack,
+   push_stack(>attr_stack,
   read_attr_from_file(git_attributes_file, 1),
   NULL, 0);
 
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
elem = read_attr(GITATTRIBUTES_FILE, 1);
-   push_stack(_stack, elem, xstrdup(""), 0);
+   push_stack(>attr_stack, elem, xstrdup(""), 0);
debug_push(elem);
}
 
elem = read_attr_from_file(git_path_info_attributes(), 1);
if (!elem)
elem = xcalloc(1, sizeof(*elem));
-   push_stack(_stack, elem, NULL, 0);
+   push_stack(>attr_stack, elem, NULL, 0);
 }
 
-static void prepare_attr_stack(const char *path, int dirlen)
+static void prepare_attr_stack(const char *path, int dirlen,
+  struct git_attr_check *check)
 {
struct attr_stack *elem, *info;
const char *cp;
@@ -651,13 +667,13 @@ static void prepare_attr_stack(const char *path, int 
dirlen)
 * .gitattributes in deeper directories to shallower ones,
 * and finally use the built-in set as the default.
 */
-   bootstrap_attr_stack();
+   bootstrap_attr_stack(check);
 
/*
 * Pop the "info" one that is always at the top of the stack.
 */
-   info = attr_stack;
-   attr_stack = info->prev;
+   info = check->attr_stack;
+   check->attr_stack = info->prev;
 
/*
 * Pop the ones from directories that are not the prefix of
@@ -665,17 +681,17 @@ static void prepare_attr_stack(const char *path, int 
dirlen)
 * the root one (whose origin is an empty string "") or the builtin
 * one (whose origin is NULL) without popping it.
 */
-   while (attr_stack->origin) {
-   int namelen = strlen(attr_stack->origin);
+   while (check->attr_stack->origin) {
+   int namelen = strlen(check->attr_stack->origin);
 
-   elem = attr_stack;
+   elem = check->attr_stack;
if (namelen <= dirlen &&
!strncmp(elem->origin, 

[PATCH 14/35] attr: retire git_check_attrs() API

[Stefan Beller]

From: Junio C Hamano 

Since nobody uses the old API, make it file-scope static, and update
the documentation to describe the new API.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/technical/api-gitattributes.txt | 82 ++-
 attr.c|  3 +-
 attr.h|  2 -
 3 files changed, 56 insertions(+), 31 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 2602668..92fc32a 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,10 +16,15 @@ Data Structure
of no interest to the calling programs.  The name of the
attribute can be retrieved by calling `git_attr_name()`.
 
+`struct git_attr_check_elem`::
+
+   This structure represents one attribute and its value.
+
 `struct git_attr_check`::
 
-   This structure represents a set of attributes to check in a call
-   to `git_check_attr()` function, and receives the results.
+   This structure represents a collection of `git_attr_check_elem`.
+   It is passed to `git_check_attr()` function, specifying the
+   attributes to check, and receives their values.
 
 
 Attribute Values
@@ -48,49 +53,51 @@ value of the attribute for the path.
 Querying Specific Attributes
 
 
-* Prepare an array of `struct git_attr_check` to define the list of
-  attributes you would want to check.  To populate this array, you would
-  need to define necessary attributes by calling `git_attr()` function.
+* Prepare `struct git_attr_check` using git_attr_check_initl()
+  function, enumerating the names of attributes whose values you are
+  interested in, terminated with a NULL pointer.  Alternatively, an
+  empty `struct git_attr_check` can be prepared by calling
+  `git_attr_check_alloc()` function and then attributes you want to
+  ask about can be added to it with `git_attr_check_append()`
+  function.
 
 * Call `git_check_attr()` to check the attributes for the path.
 
-* Inspect `git_attr_check` structure to see how each of the attribute in
-  the array is defined for the path.
+* Inspect `git_attr_check` structure to see how each of the
+  attribute in the array is defined for the path.
 
 
 Example
 ---
 
-To see how attributes "crlf" and "indent" are set for different paths.
+To see how attributes "crlf" and "ident" are set for different paths.
 
-. Prepare an array of `struct git_attr_check` with two elements (because
-  we are checking two attributes).  Initialize their `attr` member with
-  pointers to `struct git_attr` obtained by calling `git_attr()`:
+. Prepare a `struct git_attr_check` with two elements (because
+  we are checking two attributes):
 
 
-static struct git_attr_check check[2];
+static struct git_attr_check *check;
 static void setup_check(void)
 {
-   if (check[0].attr)
+   if (check)
return; /* already done */
-   check[0].attr = git_attr("crlf");
-   check[1].attr = git_attr("ident");
+   check = git_attr_check_initl("crlf", "ident", NULL);
 }
 
 
-. Call `git_check_attr()` with the prepared array of `struct git_attr_check`:
+. Call `git_check_attr()` with the prepared `struct git_attr_check`:
 
 
const char *path;
 
setup_check();
-   git_check_attr(path, ARRAY_SIZE(check), check);
+   git_check_attr(path, check);
 
 
-. Act on `.value` member of the result, left in `check[]`:
+. Act on `.value` member of the result, left in `check->check[]`:
 
 
-   const char *value = check[0].value;
+   const char *value = check->check[0].value;
 
if (ATTR_TRUE(value)) {
The attribute is Set, by listing only the name of the
@@ -109,20 +116,39 @@ static void setup_check(void)
}
 
 
+To see how attributes in argv[] are set for different paths, only
+the first step in the above would be different.
+
+
+static struct git_attr_check *check;
+static void setup_check(const char **argv)
+{
+   check = git_attr_check_alloc();
+   while (*argv) {
+   struct git_attr *attr = git_attr(*argv);
+   git_attr_check_append(check, attr);
+   argv++;
+   }
+}
+
+
 
 Querying All Attributes
 ---
 
 To get the values of all attributes associated with a file:
 
-* Call `git_all_attrs()`, which returns an array of `git_attr_check`
-  structures.
+* Prepare an empty `git_attr_check` structure by calling
+  `git_attr_check_alloc()`.
+
+* Call `git_all_attrs()`, which populates the `git_attr_check`
+  with the attributes attached to the path.
 
-* Iterate over the `git_attr_check` array to examine the 

[PATCH 08/35] attr.c: tighten constness around "git_attr" structure

[Stefan Beller]

From: Junio C Hamano 

It holds an interned string, and git_attr_name() is a way to peek
into it.  Make sure the involved pointer types are pointer-to-const.

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

diff --git a/attr.c b/attr.c
index 7bfeef3..5c35d42 100644
--- a/attr.c
+++ b/attr.c
@@ -43,7 +43,7 @@ static int cannot_trust_maybe_real;
 static struct git_attr_check *check_all_attr;
 static struct git_attr *(git_attr_hash[HASHSIZE]);
 
-char *git_attr_name(struct git_attr *attr)
+const char *git_attr_name(const struct git_attr *attr)
 {
return attr->name;
 }
diff --git a/attr.h b/attr.h
index 8b08d33..00d7a66 100644
--- a/attr.h
+++ b/attr.h
@@ -25,7 +25,7 @@ extern const char git_attr__false[];
  * Unset one is returned as NULL.
  */
 struct git_attr_check {
-   struct git_attr *attr;
+   const struct git_attr *attr;
const char *value;
 };
 
@@ -34,7 +34,7 @@ struct git_attr_check {
  * return value is a pointer to a null-delimited string that is part
  * of the internal data structure; it should not be modified or freed.
  */
-char *git_attr_name(struct git_attr *);
+extern const char *git_attr_name(const struct git_attr *);
 
 int git_check_attr(const char *path, int, struct git_attr_check *);
 
-- 
2.10.1.469.g00a8914

[PATCH 16/35] attr: expose validity check for attribute names

[Stefan Beller]

From: Junio C Hamano 

Export attr_name_valid() function, and a helper function that
returns the message to be given when a given  pair
is not a good name for an attribute.

We could later update the message to exactly spell out what the
rules for a good attribute name are, etc.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 39 +--
 attr.h | 10 ++
 2 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/attr.c b/attr.c
index 9bec243..14bbfd4 100644
--- a/attr.c
+++ b/attr.c
@@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen)
return val;
 }
 
-static int invalid_attr_name(const char *name, int namelen)
+int attr_name_valid(const char *name, size_t namelen)
 {
/*
 * Attribute name cannot begin with '-' and must consist of
 * characters from [-A-Za-z0-9_.].
 */
if (namelen <= 0 || *name == '-')
-   return -1;
+   return 0;
while (namelen--) {
char ch = *name++;
if (! (ch == '-' || ch == '.' || ch == '_' ||
   ('0' <= ch && ch <= '9') ||
   ('a' <= ch && ch <= 'z') ||
   ('A' <= ch && ch <= 'Z')) )
-   return -1;
+   return 0;
}
-   return 0;
+   return 1;
+}
+
+void invalid_attr_name_message(struct strbuf *err, const char *name, int len)
+{
+   strbuf_addf(err, _("%.*s is not a valid attribute name"),
+   len, name);
+}
+
+static void report_invalid_attr(const char *name, size_t len,
+   const char *src, int lineno)
+{
+   struct strbuf err = STRBUF_INIT;
+   invalid_attr_name_message(, name, len);
+   fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno);
+   strbuf_release();
 }
 
 static struct git_attr *git_attr_internal(const char *name, int len)
@@ -90,7 +105,7 @@ static struct git_attr *git_attr_internal(const char *name, 
int len)
return a;
}
 
-   if (invalid_attr_name(name, len))
+   if (!attr_name_valid(name, len))
return NULL;
 
FLEX_ALLOC_MEM(a, name, name, len);
@@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int 
lineno, const char *cp,
cp++;
len--;
}
-   if (invalid_attr_name(cp, len)) {
-   fprintf(stderr,
-   "%.*s is not a valid attribute name: %s:%d\n",
-   len, cp, src, lineno);
+   if (!attr_name_valid(cp, len)) {
+   report_invalid_attr(cp, len, src, lineno);
return NULL;
}
} else {
/*
 * As this function is always called twice, once with
 * e == NULL in the first pass and then e != NULL in
-* the second pass, no need for invalid_attr_name()
+* the second pass, no need for attr_name_valid()
 * check here.
 */
if (*cp == '-' || *cp == '!') {
@@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
-   if (invalid_attr_name(name, namelen)) {
-   fprintf(stderr,
-   "%.*s is not a valid attribute name: %s:%d\n",
-   namelen, name, src, lineno);
+   if (!attr_name_valid(name, namelen)) {
+   report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
}
diff --git a/attr.h b/attr.h
index c84f164..1c6a6d8 100644
--- a/attr.h
+++ b/attr.h
@@ -10,6 +10,16 @@ struct git_attr;
  */
 struct git_attr *git_attr(const char *);
 
+/*
+ * Return the name of the attribute represented by the argument.  The
+ * return value is a pointer to a null-delimited string that is part
+ * of the internal data structure; it should not be modified or freed.
+ */
+extern const char *git_attr_name(const struct git_attr *);
+
+extern int attr_name_valid(const char *name, size_t namelen);
+extern void invalid_attr_name_message(struct strbuf *, const char *, int);
+
 /* Internal use */
 extern const char git_attr__true[];
 extern const char git_attr__false[];
-- 
2.10.1.469.g00a8914

[PATCH 26/35] attr: convert to new threadsafe API

[Stefan Beller]

This revamps the API of the attr subsystem to be thread safe.
Before we had the question and its results in one struct type.
The typical usage of the API was

static struct git_attr_check *check;

if (!check)
check = git_attr_check_initl("text", NULL);

git_check_attr(path, check);
act_on(check->value[0]);

This has a couple of issues when it comes to thread safety:

* the initialization is racy in this implementation. To make it
  thread safe, we need to acquire a mutex, such that only one
  thread is executing the code in git_attr_check_initl.
  As we do not want to introduce a mutex at each call site,
  this is best done in the attr code. However to do so, we need
  to have access to the `check` variable, i.e. the API has to
  look like
git_attr_check_initl(struct git_attr_check**, ...);
  Then one of the threads calling git_attr_check_initl will
  acquire the mutex and init the `check`, while all other threads
  will wait on the mutex just to realize they're late to the
  party and they'll return with no work done.

* While the check for attributes to be questioned only need to
  be initalized once as that part will be read only after its
  initialisation, the answer may be different for each path.
  Because of that we need to decouple the check and the answer,
  such that each thread can obtain an answer for the path it
  is currently processing.

This commit changes the API and adds locking in
git_attr_check_initl that provides the thread safety for constructing
`struct git_attr_check`.

The usage of the new API will be:

/*
 * The initl call will thread-safely check whether the
 * struct git_attr_check has been initialized. We only
 * want to do the initialization work once, hence we do
 * that work inside a thread safe environment.
 */
static struct git_attr_check *check;
git_attr_check_initl(, "text", NULL);

/* We're just asking for one attribute "text". */
git_attr_result myresult[1];

/* Perform the check and act on it: */
git_check_attr(path, check, myresult);
act_on(myresult[0].value);

/*
 * No need to free the check as it is static, hence doesn't leak
 * memory. The result is also static, so no need to free there either.
 */

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 

# Conflicts:
#   attr.h

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-gitattributes.txt | 136 +++-
 archive.c |  11 +-
 attr.c| 172 +-
 attr.h|  72 ++-
 builtin/check-attr.c  |  50 
 builtin/pack-objects.c|  16 +--
 common-main.c |   2 +
 convert.c |  40 +++---
 ll-merge.c|  24 ++--
 userdiff.c|  16 +--
 ws.c  |   8 +-
 11 files changed, 323 insertions(+), 224 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 92fc32a..7dd6616 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -10,21 +10,23 @@ Data Structure
 
 `struct git_attr`::
 
-   An attribute is an opaque object that is identified by its name.
-   Pass the name to `git_attr()` function to obtain the object of
-   this type.  The internal representation of this structure is
+   The internal representation of this structure is
of no interest to the calling programs.  The name of the
attribute can be retrieved by calling `git_attr_name()`.
 
-`struct git_attr_check_elem`::
-
-   This structure represents one attribute and its value.
-
 `struct git_attr_check`::
 
-   This structure represents a collection of `git_attr_check_elem`.
+   This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
-   attributes to check, and receives their values.
+   attributes to check, and receives their values into a corresponding
+   `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+   This structure represents one results for a check, such that an
+   array of `struct git_attr_results` corresponds to a
+   `struct git_attr_check`. The answers given in that array are in
+   the the same order as the check struct.
 
 
 Attribute Values
@@ -32,7 +34,7 @@ Attribute Values
 
 An attribute for a path can be in one of four states: Set, Unset,
 Unspecified or set to a string, and `.value` member of `struct
-git_attr_check` records it.  There are three macros to check these:
+git_attr_result` records it.  There are three 

[PATCH 07/35] attr.c: simplify macroexpand_one()

[Stefan Beller]

From: Junio C Hamano 

The double-loop wants to do an early return immediately when one
matching macro is found.  Eliminate the extra variable 'a' used for
that purpose and rewrite the "assign the found item to 'a' to make
it non-NULL and force the loop(s) to terminate" with a direct return
from there.

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

diff --git a/attr.c b/attr.c
index 95416d3..7bfeef3 100644
--- a/attr.c
+++ b/attr.c
@@ -701,24 +701,21 @@ static int fill(const char *path, int pathlen, int 
basename_offset,
 static int macroexpand_one(int nr, int rem)
 {
struct attr_stack *stk;
-   struct match_attr *a = NULL;
int i;
 
if (check_all_attr[nr].value != ATTR__TRUE ||
!check_all_attr[nr].attr->maybe_macro)
return rem;
 
-   for (stk = attr_stack; !a && stk; stk = stk->prev)
-   for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+   for (stk = attr_stack; stk; stk = stk->prev) {
+   for (i = stk->num_matches - 1; 0 <= i; i--) {
struct match_attr *ma = stk->attrs[i];
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
-   a = ma;
+   return fill_one("expand", ma, rem);
}
-
-   if (a)
-   rem = fill_one("expand", a, rem);
+   }
 
return rem;
 }
-- 
2.10.1.469.g00a8914

[PATCH 01/35] commit.c: use strchrnul() to scan for one line

[Stefan Beller]

From: Junio C Hamano 

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/commit.c b/commit.c
index 856fd4a..41b2fdd 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const 
char **subject)
p++;
if (*p) {
p = skip_blank_lines(p + 2);
-   for (eol = p; *eol && *eol != '\n'; eol++)
-   ; /* do nothing */
+   eol = strchrnul(p, '\n');
} else
eol = p;
 
-- 
2.10.1.469.g00a8914

[PATCH 21/35] attr.c: correct ugly hack for git_all_attrs()

[Stefan Beller]

From: Junio C Hamano 

The collect_some_attrs() function has an ugly hack since

06a604e6 (attr: avoid heavy work when we know the specified attr is
not defined, 2014-12-28) added an optimization that relies on the
fact that the caller knows what attributes it is interested in, so
that we can leave once we know the final answer for all the
attributes the caller asked.

git_all_attrs() that asks "what attributes are on this path?"
however does not know what attributes it is interested in, other
than the vague "we are interested in all of them", which is not a
very useful thing to say.  As a way to disable this optimization
for this caller, the said commit added a code to skip it when
the caller passes a NULL for the check structure.

However, it skipped the optimization not when check is NULL, but
when the number of attributes being checked is 0, which is
unnecessarily pessimistic.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 24 
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/attr.c b/attr.c
index e18098c..48460d2 100644
--- a/attr.c
+++ b/attr.c
@@ -748,8 +748,8 @@ static int attr_check_is_dynamic(const struct 
git_attr_check *check)
 
 /*
  * Collect attributes for path into the array pointed to by
- * check_all_attr. If num is non-zero, only attributes in check[] are
- * collected. Otherwise all attributes are collected.
+ * check_all_attr.  If check is not NULL, only attributes in
+ * check[] are collected. Otherwise all attributes are collected.
  */
 static void collect_some_attrs(const char *path, int pathlen,
   struct git_attr_check *check)
@@ -759,17 +759,6 @@ static void collect_some_attrs(const char *path, int 
pathlen,
int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
-   int num;
-   struct git_attr_check_elem *celem;
-
-   if (!check) {
-   /* Yuck - ugly git_all_attrs() hack! */
-   celem = NULL;
-   num = 0;
-   } else {
-   celem = check->check;
-   num = check->check_nr;
-   }
 
for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
@@ -786,9 +775,12 @@ static void collect_some_attrs(const char *path, int 
pathlen,
prepare_attr_stack(path, dirlen);
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
-   if (num && !cannot_trust_maybe_real) {
+
+   if (check && !cannot_trust_maybe_real) {
+   struct git_attr_check_elem *celem = check->check;
+
rem = 0;
-   for (i = 0; i < num; i++) {
+   for (i = 0; i < check->check_nr; i++) {
if (!celem[i].attr->maybe_real) {
struct git_attr_check_elem *c;
c = check_all_attr + celem[i].attr->attr_nr;
@@ -796,7 +788,7 @@ static void collect_some_attrs(const char *path, int 
pathlen,
rem++;
}
}
-   if (rem == num)
+   if (rem == check->check_nr)
return;
}
 
-- 
2.10.1.469.g00a8914

[PATCH 23/35] attr.c: always pass check[] to collect_some_attrs()

[Stefan Beller]

From: Junio C Hamano 

This function used to be called with check=NULL to signal it to
collect all attributes in the global check_all_attr[] array.

Because the longer term plan is to allocate check_all_attr[] and
attr_stack data structures per git_attr_check instance (i.e. "check"
here) to make the attr subsystem thread-safe, it is unacceptable.

Pass "Are we grabbing all attributes defined in the system?" bit as
a separate argument and pass it from the callers.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 37 +++--
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/attr.c b/attr.c
index 5bb925d..246a5d0 100644
--- a/attr.c
+++ b/attr.c
@@ -756,11 +756,12 @@ static void empty_attr_check_elems(struct git_attr_check 
*check)
 
 /*
  * Collect attributes for path into the array pointed to by
- * check_all_attr.  If check is not NULL, only attributes in
- * check[] are collected. Otherwise all attributes are collected.
+ * check_all_attr.  If collect_all is zero, only attributes in
+ * check[] are collected.  Otherwise, check[] is cleared and
+ * any and all attributes that are visible are collected in it.
  */
 static void collect_some_attrs(const char *path, int pathlen,
-  struct git_attr_check *check)
+  struct git_attr_check *check, int collect_all)
 
 {
struct attr_stack *stk;
@@ -781,10 +782,11 @@ static void collect_some_attrs(const char *path, int 
pathlen,
}
 
prepare_attr_stack(path, dirlen);
+
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
 
-   if (check && !cannot_trust_maybe_real) {
+   if (!collect_all && !cannot_trust_maybe_real) {
struct git_attr_check_elem *celem = check->check;
 
rem = 0;
@@ -803,6 +805,17 @@ static void collect_some_attrs(const char *path, int 
pathlen,
rem = attr_nr;
for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
rem = fill(path, pathlen, basename_offset, stk, rem);
+
+   if (collect_all) {
+   empty_attr_check_elems(check);
+   for (i = 0; i < attr_nr; i++) {
+   const struct git_attr *attr = check_all_attr[i].attr;
+   const char *value = check_all_attr[i].value;
+   if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+   continue;
+   git_attr_check_append(check, attr)->value = value;
+   }
+   }
 }
 
 static int git_check_attrs(const char *path, int pathlen,
@@ -811,7 +824,7 @@ static int git_check_attrs(const char *path, int pathlen,
int i;
struct git_attr_check_elem *celem = check->check;
 
-   collect_some_attrs(path, pathlen, check);
+   collect_some_attrs(path, pathlen, check, 0);
 
for (i = 0; i < check->check_nr; i++) {
const char *value = 
check_all_attr[celem[i].attr->attr_nr].value;
@@ -825,19 +838,7 @@ static int git_check_attrs(const char *path, int pathlen,
 
 void git_all_attrs(const char *path, struct git_attr_check *check)
 {
-   int i;
-
-   git_attr_check_clear(check);
-   collect_some_attrs(path, strlen(path), NULL);
-
-   for (i = 0; i < attr_nr; i++) {
-   const char *name = check_all_attr[i].attr->name;
-   const char *value = check_all_attr[i].value;
-   if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
-   continue;
-   git_attr_check_append(check, git_attr(name));
-   check->check[check->check_nr - 1].value = value;
-   }
+   collect_some_attrs(path, strlen(path), check, 1);
 }
 
 void git_attr_set_direction(enum git_attr_direction new, struct index_state 
*istate)
-- 
2.10.1.469.g00a8914

[PATCH 24/35] attr.c: outline the future plans by heavily commenting

[Stefan Beller]

From: Junio C Hamano 

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 40 +++-
 1 file changed, 39 insertions(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index 246a5d0..60d7eec 100644
--- a/attr.c
+++ b/attr.c
@@ -30,6 +30,11 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 #define DEBUG_ATTR 0
 #endif
 
+/*
+ * NEEDSWORK: the global dictionary of the interned attributes
+ * must stay a singleton even after we become thread-ready.
+ * Access to these must be surrounded with mutex when it happens.
+ */
 struct git_attr {
struct git_attr *next;
unsigned h;
@@ -39,10 +44,19 @@ struct git_attr {
char name[FLEX_ARRAY];
 };
 static int attr_nr;
+static struct git_attr *(git_attr_hash[HASHSIZE]);
+
+/*
+ * NEEDSWORK: maybe-real, maybe-macro are not property of
+ * an attribute, as it depends on what .gitattributes are
+ * read.  Once we introduce per git_attr_check attr_stack
+ * and check_all_attr, the optimization based on them will
+ * become unnecessary and can go away.  So is this variable.
+ */
 static int cannot_trust_maybe_real;
 
+/* NEEDSWORK: This will become per git_attr_check */
 static struct git_attr_check_elem *check_all_attr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 const char *git_attr_name(const struct git_attr *attr)
 {
@@ -117,6 +131,11 @@ static struct git_attr *git_attr_internal(const char 
*name, int len)
a->maybe_real = 0;
git_attr_hash[pos] = a;
 
+   /*
+* NEEDSWORK: per git_attr_check check_all_attr
+* will be initialized a lot more lazily, not
+* like this, and not here.
+*/
REALLOC_ARRAY(check_all_attr, attr_nr);
check_all_attr[a->attr_nr].attr = a;
check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
@@ -329,6 +348,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
  * .gitignore file and info/excludes file as a fallback.
  */
 
+/* NEEDSWORK: This will become per git_attr_check */
 static struct attr_stack {
struct attr_stack *prev;
char *origin;
@@ -393,6 +413,24 @@ static struct attr_stack *read_attr_from_array(const char 
**list)
return res;
 }
 
+/*
+ * NEEDSWORK: these two are tricky.  The callers assume there is a
+ * single, system-wide global state "where we read attributes from?"
+ * and when the state is flipped by calling git_attr_set_direction(),
+ * attr_stack is discarded so that subsequent attr_check will lazily
+ * read from the right place.  And they do not know or care who called
+ * by them uses the attribute subsystem, hence have no knowledge of
+ * existing git_attr_check instances or future ones that will be
+ * created).
+ *
+ * Probably we need a thread_local that holds these two variables,
+ * and a list of git_attr_check instances (which need to be maintained
+ * by hooking into git_attr_check_alloc(), git_attr_check_initl(), and
+ * git_attr_check_clear().  Then git_attr_set_direction() updates the
+ * fields in that thread_local for these two variables, iterate over
+ * all the active git_attr_check instances and discard the attr_stack
+ * they hold.  Yuck, but it sounds doable.
+ */
 static enum git_attr_direction direction;
 static struct index_state *use_index;
 
-- 
2.10.1.469.g00a8914

[PATCH 19/35] attr.c: pass struct git_attr_check down the callchain

[Stefan Beller]

From: Junio C Hamano 

The callchain that starts from git_check_attrs() down to
collect_some_attrs() used to take an array of git_attr_check_elem
as their parameters.  Pass the enclosing git_attr_check instance
instead, so that they will have access to new fields we will add to
the data structure.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 36 
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/attr.c b/attr.c
index a504cb3..219ab11 100644
--- a/attr.c
+++ b/attr.c
@@ -751,14 +751,25 @@ static int attr_check_is_dynamic(const struct 
git_attr_check *check)
  * check_all_attr. If num is non-zero, only attributes in check[] are
  * collected. Otherwise all attributes are collected.
  */
-static void collect_some_attrs(const char *path, int pathlen, int num,
-  struct git_attr_check_elem *check)
+static void collect_some_attrs(const char *path, int pathlen,
+  struct git_attr_check *check)
 
 {
struct attr_stack *stk;
int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
+   int num;
+   struct git_attr_check_elem *celem;
+
+   if (!check) {
+   /* Yuck - ugly git_all_attrs() hack! */
+   celem = NULL;
+   num = 0;
+   } else {
+   celem = check->check;
+   num = check->check_nr;
+   }
 
for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
@@ -778,9 +789,9 @@ static void collect_some_attrs(const char *path, int 
pathlen, int num,
if (num && !cannot_trust_maybe_real) {
rem = 0;
for (i = 0; i < num; i++) {
-   if (!check[i].attr->maybe_real) {
+   if (!celem[i].attr->maybe_real) {
struct git_attr_check_elem *c;
-   c = check_all_attr + check[i].attr->attr_nr;
+   c = check_all_attr + celem[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
@@ -794,18 +805,19 @@ static void collect_some_attrs(const char *path, int 
pathlen, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
 }
 
-static int git_check_attrs(const char *path, int pathlen, int num,
-  struct git_attr_check_elem *check)
+static int git_check_attrs(const char *path, int pathlen,
+  struct git_attr_check *check)
 {
int i;
+   struct git_attr_check_elem *celem = check->check;
 
-   collect_some_attrs(path, pathlen, num, check);
+   collect_some_attrs(path, pathlen, check);
 
-   for (i = 0; i < num; i++) {
-   const char *value = 
check_all_attr[check[i].attr->attr_nr].value;
+   for (i = 0; i < check->check_nr; i++) {
+   const char *value = 
check_all_attr[celem[i].attr->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
-   check[i].value = value;
+   celem[i].value = value;
}
 
return 0;
@@ -816,7 +828,7 @@ void git_all_attrs(const char *path, struct git_attr_check 
*check)
int i;
 
git_attr_check_clear(check);
-   collect_some_attrs(path, strlen(path), 0, NULL);
+   collect_some_attrs(path, strlen(path), NULL);
 
for (i = 0; i < attr_nr; i++) {
const char *name = check_all_attr[i].attr->name;
@@ -845,7 +857,7 @@ int git_check_attr_counted(const char *path, int pathlen,
   struct git_attr_check *check)
 {
check->finalized = 1;
-   return git_check_attrs(path, pathlen, check->check_nr, check->check);
+   return git_check_attrs(path, pathlen, check);
 }
 
 int git_check_attr(const char *path, struct git_attr_check *check)
-- 
2.10.1.469.g00a8914

[PATCH 13/35] attr: convert git_check_attrs() callers to use the new API

[Stefan Beller]

From: Junio C Hamano 

The remaining callers are all simple "I have N attributes I am
interested in.  I'll ask about them with various paths one by one".

After this step, no caller to git_check_attrs() remains.  After
removing it, we can extend "struct git_attr_check" struct with data
that can be used in optimizing the query for the specific N
attributes it contains.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/pack-objects.c | 19 +--
 convert.c  | 18 +++---
 ll-merge.c | 33 ++---
 userdiff.c | 19 ---
 ws.c   | 19 ++-
 5 files changed, 40 insertions(+), 68 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3cb38ed..3918c07 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -896,24 +896,15 @@ static void write_pack_file(void)
written, nr_result);
 }
 
-static void setup_delta_attr_check(struct git_attr_check_elem *check)
-{
-   static struct git_attr *attr_delta;
-
-   if (!attr_delta)
-   attr_delta = git_attr("delta");
-
-   check[0].attr = attr_delta;
-}
-
 static int no_try_delta(const char *path)
 {
-   struct git_attr_check_elem check[1];
+   static struct git_attr_check *check;
 
-   setup_delta_attr_check(check);
-   if (git_check_attrs(path, ARRAY_SIZE(check), check))
+   if (!check)
+   check = git_attr_check_initl("delta", NULL);
+   if (git_check_attr(path, check))
return 0;
-   if (ATTR_FALSE(check->value))
+   if (ATTR_FALSE(check->check[0].value))
return 1;
return 0;
 }
diff --git a/convert.c b/convert.c
index c95ae71..bb2435a 100644
--- a/convert.c
+++ b/convert.c
@@ -775,24 +775,20 @@ struct conv_attrs {
int ident;
 };
 
-static const char *conv_attr_name[] = {
-   "crlf", "ident", "filter", "eol", "text",
-};
-#define NUM_CONV_ATTRS ARRAY_SIZE(conv_attr_name)
-
 static void convert_attrs(struct conv_attrs *ca, const char *path)
 {
-   int i;
-   static struct git_attr_check_elem ccheck[NUM_CONV_ATTRS];
+   static struct git_attr_check *check;
 
-   if (!ccheck[0].attr) {
-   for (i = 0; i < NUM_CONV_ATTRS; i++)
-   ccheck[i].attr = git_attr(conv_attr_name[i]);
+   if (!check) {
+   check = git_attr_check_initl("crlf", "ident",
+"filter", "eol", "text",
+NULL);
user_convert_tail = _convert;
git_config(read_convert_config, NULL);
}
 
-   if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
+   if (!git_check_attr(path, check)) {
+   struct git_attr_check_elem *ccheck = check->check;
ca->crlf_action = git_path_check_crlf(ccheck + 4);
if (ca->crlf_action == CRLF_UNDEFINED)
ca->crlf_action = git_path_check_crlf(ccheck + 0);
diff --git a/ll-merge.c b/ll-merge.c
index eb2c37e..bc6479c 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -336,15 +336,6 @@ static const struct ll_merge_driver 
*find_ll_merge_driver(const char *merge_attr
return _merge_drv[LL_TEXT_MERGE];
 }
 
-static int git_path_check_merge(const char *path, struct git_attr_check_elem 
check[2])
-{
-   if (!check[0].attr) {
-   check[0].attr = git_attr("merge");
-   check[1].attr = git_attr("conflict-marker-size");
-   }
-   return git_check_attrs(path, 2, check);
-}
-
 static void normalize_file(mmfile_t *mm, const char *path)
 {
struct strbuf strbuf = STRBUF_INIT;
@@ -362,7 +353,7 @@ int ll_merge(mmbuffer_t *result_buf,
 mmfile_t *theirs, const char *their_label,
 const struct ll_merge_options *opts)
 {
-   static struct git_attr_check_elem check[2];
+   static struct git_attr_check *check;
static const struct ll_merge_options default_opts;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -376,10 +367,14 @@ int ll_merge(mmbuffer_t *result_buf,
normalize_file(ours, path);
normalize_file(theirs, path);
}
-   if (!git_path_check_merge(path, check)) {
-   ll_driver_name = check[0].value;
-   if (check[1].value) {
-   marker_size = atoi(check[1].value);
+
+   if (!check)
+   check = git_attr_check_initl("merge", "conflict-marker-size", 
NULL);
+
+   if (!git_check_attr(path, check)) {
+   ll_driver_name = check->check[0].value;
+   if (check->check[1].value) {
+   marker_size = atoi(check->check[1].value);
 

[PATCH 22/35] attr.c: introduce empty_attr_check_elems()

[Stefan Beller]

From: Junio C Hamano 

One codepath needs to just empty the git_attr_check_elem array in
the git_attr_check structure, without releasing the entire resource.
Introduce a helper to do so and rewrite git_attr_check_clear() using
it.

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

diff --git a/attr.c b/attr.c
index 48460d2..5bb925d 100644
--- a/attr.c
+++ b/attr.c
@@ -746,6 +746,14 @@ static int attr_check_is_dynamic(const struct 
git_attr_check *check)
return (void *)(check->check) != (void *)(check + 1);
 }
 
+static void empty_attr_check_elems(struct git_attr_check *check)
+{
+   if (!attr_check_is_dynamic(check))
+   die("BUG: emptying a statically initialized git_attr_check");
+   check->check_nr = 0;
+   check->finalized = 0;
+}
+
 /*
  * Collect attributes for path into the array pointed to by
  * check_all_attr.  If check is not NULL, only attributes in
@@ -912,12 +920,11 @@ struct git_attr_check_elem *git_attr_check_append(struct 
git_attr_check *check,
 
 void git_attr_check_clear(struct git_attr_check *check)
 {
+   empty_attr_check_elems(check);
if (!attr_check_is_dynamic(check))
die("BUG: clearing a statically initialized git_attr_check");
free(check->check);
-   check->check_nr = 0;
check->check_alloc = 0;
-   check->finalized = 0;
 }
 
 void git_attr_check_free(struct git_attr_check *check)
-- 
2.10.1.469.g00a8914

[PATCH 12/35] attr: convert git_all_attrs() to use "struct git_attr_check"

[Stefan Beller]

From: Junio C Hamano 

This updates the other two ways the attribute check is done via an
array of "struct git_attr_check_elem" elements.  These two niches
appear only in "git check-attr".

 * The caller does not know offhand what attributes it wants to ask
   about and cannot use git_attr_check_initl() to prepare the
   git_attr_check structure.

 * The caller may not know what attributes it wants to ask at all,
   and instead wants to learn everything that the given path has.

Such a caller can call git_attr_check_alloc() to allocate an empty
git_attr_check, and then call git_attr_check_append() to add
attribute names one by one.  A new attribute can be appended until
git_attr_check structure is "finalized", which happens when it is
used to ask for attributes for any path by calling git_check_attr()
or git_all_attrs().  A git_attr_check structure that is initialized
by git_attr_check_initl() is already finalized when it is returned.

I am not at all happy with the way git_all_attrs() API turned out to
be, but it is only to support one niche caller ("check-attr --all"),
so I'll stop here for now.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c   | 75 ++--
 attr.h   | 16 ++-
 builtin/check-attr.c | 51 ++-
 3 files changed, 90 insertions(+), 52 deletions(-)

diff --git a/attr.c b/attr.c
index 861e1a2..76f0d6b 100644
--- a/attr.c
+++ b/attr.c
@@ -724,6 +724,11 @@ static int macroexpand_one(int nr, int rem)
return rem;
 }
 
+static int attr_check_is_dynamic(const struct git_attr_check *check)
+{
+   return (void *)(check->check) != (void *)(check + 1);
+}
+
 /*
  * Collect attributes for path into the array pointed to by
  * check_all_attr. If num is non-zero, only attributes in check[] are
@@ -789,32 +794,21 @@ int git_check_attrs(const char *path, int num, struct 
git_attr_check_elem *check
return 0;
 }
 
-int git_all_attrs(const char *path, int *num, struct git_attr_check_elem 
**check)
+void git_all_attrs(const char *path, struct git_attr_check *check)
 {
-   int i, count, j;
+   int i;
 
+   git_attr_check_clear(check);
collect_some_attrs(path, 0, NULL);
 
-   /* Count the number of attributes that are set. */
-   count = 0;
-   for (i = 0; i < attr_nr; i++) {
-   const char *value = check_all_attr[i].value;
-   if (value != ATTR__UNSET && value != ATTR__UNKNOWN)
-   ++count;
-   }
-   *num = count;
-   ALLOC_ARRAY(*check, count);
-   j = 0;
for (i = 0; i < attr_nr; i++) {
+   const char *name = check_all_attr[i].attr->name;
const char *value = check_all_attr[i].value;
-   if (value != ATTR__UNSET && value != ATTR__UNKNOWN) {
-   (*check)[j].attr = check_all_attr[i].attr;
-   (*check)[j].value = value;
-   ++j;
-   }
+   if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+   continue;
+   git_attr_check_append(check, git_attr(name));
+   check->check[check->check_nr - 1].value = value;
}
-
-   return 0;
 }
 
 void git_attr_set_direction(enum git_attr_direction new, struct index_state 
*istate)
@@ -832,6 +826,7 @@ void git_attr_set_direction(enum git_attr_direction new, 
struct index_state *ist
 
 int git_check_attr(const char *path, struct git_attr_check *check)
 {
+   check->finalized = 1;
return git_check_attrs(path, check->check_nr, check->check);
 }
 
@@ -849,17 +844,57 @@ struct git_attr_check *git_attr_check_initl(const char 
*one, ...)
check = xcalloc(1,
sizeof(*check) + cnt * sizeof(*(check->check)));
check->check_nr = cnt;
+   check->finalized = 1;
check->check = (struct git_attr_check_elem *)(check + 1);
 
check->check[0].attr = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
+   struct git_attr *attr;
param = va_arg(params, const char *);
if (!param)
die("BUG: counted %d != ended at %d",
check->check_nr, cnt);
-   check->check[cnt].attr = git_attr(param);
+   attr = git_attr(param);
+   if (!attr)
+   die("BUG: %s: not a valid attribute name", param);
+   check->check[cnt].attr = attr;
}
va_end(params);
return check;
 }
+
+struct git_attr_check *git_attr_check_alloc(void)
+{
+   return xcalloc(1, sizeof(struct git_attr_check));
+}
+
+struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *check,
+  

[PATCH 06/35] attr.c: mark where #if DEBUG ends more clearly

[Stefan Beller]

From: Junio C Hamano 

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index a7f2c3f..95416d3 100644
--- a/attr.c
+++ b/attr.c
@@ -469,7 +469,7 @@ static void debug_set(const char *what, const char *match, 
struct git_attr *attr
 #define debug_push(a) do { ; } while (0)
 #define debug_pop(a) do { ; } while (0)
 #define debug_set(a,b,c,d) do { ; } while (0)
-#endif
+#endif /* DEBUG_ATTR */
 
 static void drop_attr_stack(void)
 {
-- 
2.10.1.469.g00a8914

[PATCH 05/35] attr.c: complete a sentence in a comment

[Stefan Beller]

From: Junio C Hamano 

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/attr.c b/attr.c
index 05db667..a7f2c3f 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
  * directory (again, reading the file from top to bottom) down to the
  * current directory, and then scan the list backwards to find the first match.
  * This is exactly the same as what is_excluded() does in dir.c to deal with
- * .gitignore
+ * .gitignore file and info/excludes file as a fallback.
  */
 
 static struct attr_stack {
-- 
2.10.1.469.g00a8914

[PATCH 18/35] attr.c: add push_stack() helper

[Stefan Beller]

From: Junio C Hamano 

There are too many repetitious "I have this new attr_stack element;
push it at the top of the stack" sequence.  The new helper function
push_stack() gives us a way to express what is going on at these
places, and as a side effect, halves the number of times we mention
the attr_stack global variable.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 attr.c | 71 +++---
 1 file changed, 33 insertions(+), 38 deletions(-)

diff --git a/attr.c b/attr.c
index 88fcb7c..a504cb3 100644
--- a/attr.c
+++ b/attr.c
@@ -521,6 +521,18 @@ static int git_attr_system(void)
 
 static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
 
+static void push_stack(struct attr_stack **attr_stack_p,
+  struct attr_stack *elem, char *origin, size_t originlen)
+{
+   if (elem) {
+   elem->origin = origin;
+   if (origin)
+   elem->originlen = originlen;
+   elem->prev = *attr_stack_p;
+   *attr_stack_p = elem;
+   }
+}
+
 static void bootstrap_attr_stack(void)
 {
struct attr_stack *elem;
@@ -528,52 +540,35 @@ static void bootstrap_attr_stack(void)
if (attr_stack)
return;
 
-   elem = read_attr_from_array(builtin_attr);
-   elem->origin = NULL;
-   elem->prev = attr_stack;
-   attr_stack = elem;
-
-   if (git_attr_system()) {
-   elem = read_attr_from_file(git_etc_gitattributes(), 1);
-   if (elem) {
-   elem->origin = NULL;
-   elem->prev = attr_stack;
-   attr_stack = elem;
-   }
-   }
+   push_stack(_stack, read_attr_from_array(builtin_attr), NULL, 0);
+
+   if (git_attr_system())
+   push_stack(_stack,
+  read_attr_from_file(git_etc_gitattributes(), 1),
+  NULL, 0);
 
if (!git_attributes_file)
git_attributes_file = xdg_config_home("attributes");
-   if (git_attributes_file) {
-   elem = read_attr_from_file(git_attributes_file, 1);
-   if (elem) {
-   elem->origin = NULL;
-   elem->prev = attr_stack;
-   attr_stack = elem;
-   }
-   }
+   if (git_attributes_file)
+   push_stack(_stack,
+  read_attr_from_file(git_attributes_file, 1),
+  NULL, 0);
 
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
elem = read_attr(GITATTRIBUTES_FILE, 1);
-   elem->origin = xstrdup("");
-   elem->originlen = 0;
-   elem->prev = attr_stack;
-   attr_stack = elem;
+   push_stack(_stack, elem, xstrdup(""), 0);
debug_push(elem);
}
 
elem = read_attr_from_file(git_path_info_attributes(), 1);
if (!elem)
elem = xcalloc(1, sizeof(*elem));
-   elem->origin = NULL;
-   elem->prev = attr_stack;
-   attr_stack = elem;
+   push_stack(_stack, elem, NULL, 0);
 }
 
 static void prepare_attr_stack(const char *path, int dirlen)
 {
struct attr_stack *elem, *info;
-   int len;
const char *cp;
 
/*
@@ -633,20 +628,21 @@ static void prepare_attr_stack(const char *path, int 
dirlen)
 
assert(attr_stack->origin);
while (1) {
-   len = strlen(attr_stack->origin);
+   size_t len = strlen(attr_stack->origin);
+   char *origin;
+
if (dirlen <= len)
break;
cp = memchr(path + len + 1, '/', dirlen - len - 1);
if (!cp)
cp = path + dirlen;
-   strbuf_add(, path, cp - path);
-   strbuf_addch(, '/');
-   strbuf_addstr(, GITATTRIBUTES_FILE);
+   strbuf_addf(,
+   "%.*s/%s", (int)(cp - path), path,
+   GITATTRIBUTES_FILE);
elem = read_attr(pathbuf.buf, 0);
strbuf_setlen(, cp - path);
-   elem->origin = strbuf_detach(, 
>originlen);
-   elem->prev = attr_stack;
-   attr_stack = elem;
+   origin = strbuf_detach(, );
+   push_stack(_stack, elem, origin, len);
debug_push(elem);
}
 
@@ -656,8 +652,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
/*
 * Finally push the "info" one at the top of the stack.
 

[PATCH 11/35] attr: (re)introduce git_check_attr() and struct git_attr_check

[Stefan Beller]

From: Junio C Hamano 

A common pattern to check N attributes for many paths is to

 (1) prepare an array A of N git_attr_check_elem items;
 (2) call git_attr() to intern the N attribute names and fill A;
 (3) repeatedly call git_check_attrs() for path with N and A;

A look-up for these N attributes for a single path P scans the
entire attr_stack, starting from the .git/info/attributes file and
then .gitattributes file in the directory the path P is in, going
upwards to find .gitattributes file found in parent directories.

An earlier commit 06a604e6 (attr: avoid heavy work when we know the
specified attr is not defined, 2014-12-28) tried to optimize out
this scanning for one trivial special case: when the attribute being
sought is known not to exist, we do not have to scan for it.  While
this may be a cheap and effective heuristic, it would not work well
when N is (much) more than 1.

What we would want is a more customized way to skip irrelevant
entries in the attribute stack, and the definition of irrelevance
is tied to the set of attributes passed to git_check_attrs() call,
i.e. the set of attributes being sought.  The data necessary for
this optimization needs to live alongside the set of attributes, but
a simple array of git_attr_check_elem simply does not have any place
for that.

Introduce "struct git_attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
git_attr_check_elem items, and a function git_check_attr() that
takes a path P and this structure as its parameters.  This structure
can later be extended to hold extra data necessary for optimization.

Also, to make it easier to write the first two steps in common
cases, introduce git_attr_check_initl() helper function, which takes
a NULL-terminated list of attribute names and initialize this
structure.

As an illustration of this new API, convert archive.c that asks for
export-subst and export-ignore attributes for each paths.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 archive.c | 24 ++--
 attr.c| 34 ++
 attr.h|  9 +
 3 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/archive.c b/archive.c
index 2dc8d6c..11e3951 100644
--- a/archive.c
+++ b/archive.c
@@ -87,19 +87,6 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
 }
 
-static void setup_archive_check(struct git_attr_check_elem *check)
-{
-   static struct git_attr *attr_export_ignore;
-   static struct git_attr *attr_export_subst;
-
-   if (!attr_export_ignore) {
-   attr_export_ignore = git_attr("export-ignore");
-   attr_export_subst = git_attr("export-subst");
-   }
-   check[0].attr = attr_export_ignore;
-   check[1].attr = attr_export_subst;
-}
-
 struct directory {
struct directory *up;
struct object_id oid;
@@ -123,7 +110,7 @@ static int write_archive_entry(const unsigned char *sha1, 
const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
-   struct git_attr_check_elem check[2];
+   static struct git_attr_check *check;
const char *path_without_prefix;
int err;
 
@@ -137,11 +124,12 @@ static int write_archive_entry(const unsigned char *sha1, 
const char *base,
strbuf_addch(, '/');
path_without_prefix = path.buf + args->baselen;
 
-   setup_archive_check(check);
-   if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
-   if (ATTR_TRUE(check[0].value))
+   if (!check)
+   check = git_attr_check_initl("export-ignore", "export-subst", 
NULL);
+   if (!git_check_attr(path_without_prefix, check)) {
+   if (ATTR_TRUE(check->check[0].value))
return 0;
-   args->convert = ATTR_TRUE(check[1].value);
+   args->convert = ATTR_TRUE(check->check[1].value);
}
 
if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
diff --git a/attr.c b/attr.c
index c99e23a..861e1a2 100644
--- a/attr.c
+++ b/attr.c
@@ -829,3 +829,37 @@ void git_attr_set_direction(enum git_attr_direction new, 
struct index_state *ist
drop_attr_stack();
use_index = istate;
 }
+
+int git_check_attr(const char *path, struct git_attr_check *check)
+{
+   return git_check_attrs(path, check->check_nr, check->check);
+}
+
+struct git_attr_check *git_attr_check_initl(const char *one, ...)
+{
+   struct git_attr_check *check;
+   int cnt;
+   va_list params;
+   const char *param;
+
+   va_start(params, one);
+   for (cnt = 1; (param = va_arg(params, const char *)) != NULL; cnt++)
+   ;
+   va_end(params);
+   check = xcalloc(1,
+   

[PATCH 20/35] attr.c: rename a local variable check

[Stefan Beller]

From: Junio C Hamano 

Throughout this series, we are trying to use "check" to name an
instance of "git_attr_check" structure; let's rename a "check" that
refers to an array whose elements are git_attr_check_elem to avoid
confusion.

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

diff --git a/attr.c b/attr.c
index 219ab11..e18098c 100644
--- a/attr.c
+++ b/attr.c
@@ -682,12 +682,12 @@ static int macroexpand_one(int attr_nr, int rem);
 
 static int fill_one(const char *what, struct match_attr *a, int rem)
 {
-   struct git_attr_check_elem *check = check_all_attr;
+   struct git_attr_check_elem *celem = check_all_attr;
int i;
 
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
struct git_attr *attr = a->state[i].attr;
-   const char **n = &(check[attr->attr_nr].value);
+   const char **n = &(celem[attr->attr_nr].value);
const char *v = a->state[i].setto;
 
if (*n == ATTR__UNKNOWN) {
-- 
2.10.1.469.g00a8914

[PATCH 02/35] attr.c: use strchrnul() to scan for one line

[Stefan Beller]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index eec5d7d..45aec1b 100644
--- a/attr.c
+++ b/attr.c
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char 
*path, int macro_ok)
for (sp = buf; *sp; ) {
char *ep;
int more;
-   for (ep = sp; *ep && *ep != '\n'; ep++)
-   ;
+
+   ep = strchrnul(sp, '\n');
more = (*ep == '\n');
*ep = '\0';
handle_attr_line(res, sp, path, ++lineno, macro_ok);
-- 
2.10.1.469.g00a8914

[PATCHv3 00/35] Revamp the attr subsystem!

[Stefan Beller]

previous discussion: 
https://public-inbox.org/git/20161028185502.8789-1-sbel...@google.com/

Changes since v2:
* dropped one patch (that exposes git_attr_counted, nobody uses
  it throughout the series )
* added pathspec guarding and added a test with git-add to see if it works.
* squashed "SQUASH" commits.
* changed test for multithreaded tests as git-status makes use of pathspec
  code that doesn't support this new magic yet. 

interdiff to currently queued at the bottom of this email.

v2 was:
previous discussion at 
https://public-inbox.org/git/20161022233225.8883-1-sbel...@google.com

This implements the discarded series':
jc/attr
jc/attr-more
sb/pathspec-label
sb/submodule-default-paths

This includes
* The fixes for windows
* Junios latest suggestion to use git_attr_check_initv instead of
  alloc/append.

* I implemented the thread safe attr API in patch 27 (attr: convert to new 
threadsafe API)
* patch 28 (attr: keep attr stack for each check) makes it actually possible
  to run in a multithreaded environment.
* I added a test for the multithreaded when it is introduced in patch 32
  (pathspec: allow querying for attributes) as well as a test to disallow
  multiple "attr"s in a pathspec.

Thanks,
Stefan


Junio C Hamano (23):
  commit.c: use strchrnul() to scan for one line
  attr.c: use strchrnul() to scan for one line
  attr.c: update a stale comment on "struct match_attr"
  attr.c: explain the lack of attr-name syntax check in parse_attr()
  attr.c: complete a sentence in a comment
  attr.c: mark where #if DEBUG ends more clearly
  attr.c: simplify macroexpand_one()
  attr.c: tighten constness around "git_attr" structure
  attr.c: plug small leak in parse_attr_line()
  attr: rename function and struct related to checking attributes
  attr: (re)introduce git_check_attr() and struct git_attr_check
  attr: convert git_all_attrs() to use "struct git_attr_check"
  attr: convert git_check_attrs() callers to use the new API
  attr: retire git_check_attrs() API
  attr: add counted string version of git_check_attr()
  attr: expose validity check for attribute names
  attr.c: add push_stack() helper
  attr.c: pass struct git_attr_check down the callchain
  attr.c: rename a local variable check
  attr.c: correct ugly hack for git_all_attrs()
  attr.c: introduce empty_attr_check_elems()
  attr.c: always pass check[] to collect_some_attrs()
  attr.c: outline the future plans by heavily commenting

Nguyễn Thái Ngọc Duy (1):
  attr: support quoting pathname patterns in C style

Stefan Beller (11):
  attr: make git_check_attr_counted static
  attr: convert to new threadsafe API
  attr: keep attr stack for each check
  Documentation: fix a typo
  pathspec: move long magic parsing out of prefix_pathspec
  pathspec: move prefix check out of the inner loop
  pathspec: allow querying for attributes
  pathspec: allow escaped query values
  submodule update: add `--init-default-path` switch
  clone: add --init-submodule= switch
  completion: clone can initialize specific submodules

 Documentation/config.txt  |   5 +
 Documentation/git-clone.txt   |  23 +-
 Documentation/git-submodule.txt   |  17 +-
 Documentation/gitattributes.txt   |  10 +-
 Documentation/glossary-content.txt|  20 +
 Documentation/technical/api-gitattributes.txt | 140 ---
 archive.c |  25 +-
 attr.c| 517 ++
 attr.h|  71 ++--
 builtin/check-attr.c  |  74 ++--
 builtin/clone.c   |  36 +-
 builtin/pack-objects.c|  27 +-
 commit.c  |   3 +-
 common-main.c |   2 +
 contrib/completion/git-completion.bash|   1 +
 convert.c |  46 ++-
 dir.c |  41 +-
 git-submodule.sh  |  21 +-
 hashmap.h |   2 +
 ll-merge.c|  35 +-
 pathspec.c| 228 ++--
 pathspec.h|  19 +-
 t/t0003-attributes.sh |  26 ++
 t/t6134-pathspec-with-labels.sh   | 201 ++
 t/t7400-submodule-basic.sh| 134 +++
 userdiff.c|  21 +-
 ws.c  |  19 +-
 27 files changed, 1354 insertions(+), 410 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

-- interdiff to current origin/sb/attr:

diff --git a/attr.c b/attr.c
index cf15e76..979bd3c 100644
--- a/attr.c
+++ b/attr.c
@@ -135,7 +135,7 @@ static void report_invalid_attr(const char *name, size_t 
len,
strbuf_release();
 }
 
-struct git_attr *git_attr_counted(const char *name, 

[PATCH 10/35] attr: rename function and struct related to checking attributes

[Stefan Beller]

From: Junio C Hamano 

The traditional API to check attributes is to prepare an N-element
array of "struct git_attr_check" and pass N and the array to the
function "git_check_attr()" as arguments.

In preparation to revamp the API to pass a single structure, in
which these N elements are held, rename the type used for these
individual array elements to "struct git_attr_check_elem" and rename
the function to "git_check_attrs()".

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 archive.c  |  6 +++---
 attr.c | 12 ++--
 attr.h |  8 
 builtin/check-attr.c   | 19 ++-
 builtin/pack-objects.c |  6 +++---
 convert.c  | 12 ++--
 ll-merge.c | 10 +-
 userdiff.c |  4 ++--
 ws.c   |  6 +++---
 9 files changed, 42 insertions(+), 41 deletions(-)

diff --git a/archive.c b/archive.c
index dde1ab4..2dc8d6c 100644
--- a/archive.c
+++ b/archive.c
@@ -87,7 +87,7 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
 }
 
-static void setup_archive_check(struct git_attr_check *check)
+static void setup_archive_check(struct git_attr_check_elem *check)
 {
static struct git_attr *attr_export_ignore;
static struct git_attr *attr_export_subst;
@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1, 
const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
-   struct git_attr_check check[2];
+   struct git_attr_check_elem check[2];
const char *path_without_prefix;
int err;
 
@@ -138,7 +138,7 @@ static int write_archive_entry(const unsigned char *sha1, 
const char *base,
path_without_prefix = path.buf + args->baselen;
 
setup_archive_check(check);
-   if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+   if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
if (ATTR_TRUE(check[0].value))
return 0;
args->convert = ATTR_TRUE(check[1].value);
diff --git a/attr.c b/attr.c
index 1877f7a..c99e23a 100644
--- a/attr.c
+++ b/attr.c
@@ -40,7 +40,7 @@ struct git_attr {
 static int attr_nr;
 static int cannot_trust_maybe_real;
 
-static struct git_attr_check *check_all_attr;
+static struct git_attr_check_elem *check_all_attr;
 static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 const char *git_attr_name(const struct git_attr *attr)
@@ -665,7 +665,7 @@ static int macroexpand_one(int attr_nr, int rem);
 
 static int fill_one(const char *what, struct match_attr *a, int rem)
 {
-   struct git_attr_check *check = check_all_attr;
+   struct git_attr_check_elem *check = check_all_attr;
int i;
 
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
@@ -730,7 +730,7 @@ static int macroexpand_one(int nr, int rem)
  * collected. Otherwise all attributes are collected.
  */
 static void collect_some_attrs(const char *path, int num,
-  struct git_attr_check *check)
+  struct git_attr_check_elem *check)
 
 {
struct attr_stack *stk;
@@ -758,7 +758,7 @@ static void collect_some_attrs(const char *path, int num,
rem = 0;
for (i = 0; i < num; i++) {
if (!check[i].attr->maybe_real) {
-   struct git_attr_check *c;
+   struct git_attr_check_elem *c;
c = check_all_attr + check[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
@@ -773,7 +773,7 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
 }
 
-int git_check_attr(const char *path, int num, struct git_attr_check *check)
+int git_check_attrs(const char *path, int num, struct git_attr_check_elem 
*check)
 {
int i;
 
@@ -789,7 +789,7 @@ int git_check_attr(const char *path, int num, struct 
git_attr_check *check)
return 0;
 }
 
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
+int git_all_attrs(const char *path, int *num, struct git_attr_check_elem 
**check)
 {
int i, count, j;
 
diff --git a/attr.h b/attr.h
index 00d7a66..dd3c4a3 100644
--- a/attr.h
+++ b/attr.h
@@ -20,11 +20,11 @@ extern const char git_attr__false[];
 #define ATTR_UNSET(v) ((v) == NULL)
 
 /*
- * Send one or more git_attr_check to git_check_attr(), and
+ * Send one or more git_attr_check to git_check_attrs(), and
  * each 'value' member tells what its value is.
  * Unset one is returned as NULL.
  */
-struct git_attr_check {
+struct 

[PATCH 09/35] attr.c: plug small leak in parse_attr_line()

[Stefan Beller]

From: Junio C Hamano 

If any error is noticed after the match_attr structure is allocated,
we shouldn't just return NULL from this function.

Add a fail_return label that frees the allocated structure and
returns NULL, and consistently jump there when we want to return
NULL after cleaning up.

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

diff --git a/attr.c b/attr.c
index 5c35d42..1877f7a 100644
--- a/attr.c
+++ b/attr.c
@@ -223,7 +223,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
if (!macro_ok) {
fprintf(stderr, "%s not allowed: %s:%d\n",
name, src, lineno);
-   return NULL;
+   goto fail_return;
}
is_macro = 1;
name += strlen(ATTRIBUTE_MACRO_PREFIX);
@@ -233,7 +233,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
fprintf(stderr,
"%.*s is not a valid attribute name: %s:%d\n",
namelen, name, src, lineno);
-   return NULL;
+   goto fail_return;
}
}
else
@@ -246,7 +246,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
for (cp = states, num_attr = 0; *cp; num_attr++) {
cp = parse_attr(src, lineno, cp, NULL);
if (!cp)
-   return NULL;
+   goto fail_return;
}
 
res = xcalloc(1,
@@ -267,7 +267,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
warning(_("Negative patterns are ignored in git 
attributes\n"
  "Use '\\!' for literal leading 
exclamation."));
-   return NULL;
+   goto fail_return;
}
}
res->is_macro = is_macro;
@@ -283,6 +283,10 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
}
 
return res;
+
+fail_return:
+   free(res);
+   return NULL;
 }
 
 /*
-- 
2.10.1.469.g00a8914

Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Junio C Hamano]

Andreas Schwab  writes:

> Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end
> of test case") added a kill command to clean up after the test, but this
> can fail if the sleep command exits before the cleanup is executed.
> Ignore the error from the kill command.
>
> Explicitly check for the existence of the pid file to test that the merge
> driver was actually called.
>
> Signed-off-by: Andreas Schwab 
> ---

OK.  sleep.pid is a reasonable easy-to-access side effect we can
observe to make sure that the sleep-one-second merge driver was
indeed invoked, which was missing from the earlier round.

Thanks, will apply.

>  t/t6026-merge-attr.sh | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 7a6e33e673..03d13d00b5 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -187,13 +187,14 @@ test_expect_success 'custom merge does not lock index' '
>   sleep 1 &
>   echo $! >sleep.pid
>   EOF
> - test_when_finished "kill \$(cat sleep.pid)" &&
> + test_when_finished "kill \$(cat sleep.pid) || :" &&
>  
>   test_write_lines >.gitattributes \
>   "* merge=ours" "text merge=sleep-one-second" &&
>   test_config merge.ours.driver true &&
>   test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
> - git merge master
> + git merge master &&
> + test -f sleep.pid
>  '
>  
>  test_done
> -- 
> 2.10.2

Re: [PATCH v2] rebase: add --forget to cleanup rebase, leave everything else untouched

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Nov 10, 2016 at 6:09 PM, Duy Nguyen  wrote:
>> On Thu, Nov 10, 2016 at 3:12 AM, Junio C Hamano  wrote:
>>> Nguyễn Thái Ngọc Duy   writes:
>>>
 ---
  v2 changes just the subject line
>>>
>>> That's not sufficient, is it?  What you did in the documentation
>>> would raise the same "Hmph, is this only about HEAD?" and unlike the
>>> commit subject, it will carve it in stone for end-users.
>>
>> Oops. I forgot about git-rebase.txt. How about this?
>>
>> --forget::
>> Abort the rebase operation but leave HEAD, the index and
>> working tree untouched.
>
> Or, since --abort describes it as "reset HEAD to the original branch",
> we could write "Abort the rebase operation. Unlike --abort, HEAD is
> not restored back to the original branch". Index and worktree are
> implied by "not restored". Not sure if it's too subtle.

I think you are moving in the right direction, even though "Abort
it; unlike --abort, does this instead" is a bit hard to follow.

Re: [PATCH v7 05/17] ref-filter: move get_head_description() from branch.c

[Karthik Nayak]

On Wed, Nov 9, 2016 at 5:01 AM, Jacob Keller  wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>
>>
>> -   if (starts_with(name, "refname"))
>> +   if (starts_with(name, "refname")) {
>> refname = ref->refname;
>> -   else if (starts_with(name, "symref"))
>> +   if (ref->kind & FILTER_REFS_DETACHED_HEAD)
>> +   refname = get_head_description();
>
> Since this (I think?) changes behavior of refname would it make sense
> to add a test for this?
>

At the moment there is no way to check this, since this option is not used by
git for-each-ref or git tag (both of which completely use ref-filter
ATM). This is
however tested as eventually git branch uses ref-filter.

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=) and %(if:notequals=)

[Karthik Nayak]

On Wed, Nov 9, 2016 at 4:52 AM, Jacob Keller  wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>
> Ok. How does this handle whitespace? The previous if implementation
> treated whitespace as trimming to ignore. Does this require an exact
> whitespace match? It appears by the code that strings must match
> exactly. Would it make more sense to always trim the value of
> whitespace first before comparison? Hmm.. I think we should avoid
> doing that actually.
>

This does not trim whitespace what so ever and requires an exact match.
I don't see the reason behind trimming whitespace though.

>>  In addition to the above, for commit and tag objects, the header
>>  field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> diff --git a/ref-filter.c b/ref-filter.c
>> index 8392303..44481c3 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -22,6 +22,8 @@ struct align {
>>  };
>>
>>  struct if_then_else {
>> +   const char *if_equals,
>> +   *not_equals;
>
> Ok so we add both if_equals and not_equals values. Could we re-use the
> same string?

If we use an union then we'll be saving space, but that comes at the
disadvantage that
we'd need an indicator to show which subatom we're using (i.e
if_equals or not_equals)
for the checks made in the code below.

At the expense of keeping two pointers we avoid the extra indicator
while keeping the code
elegant. Either ways not too keen on this, up for discussion :)

>
>> unsigned int then_atom_seen : 1,
>> else_atom_seen : 1,
>> condition_satisfied : 1;
>> @@ -49,6 +51,10 @@ static struct used_atom {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, 
>> C_SUB } option;
>> unsigned int nlines;
>> } contents;
>> +   struct {
>> +   const char *if_equals,
>> +   *not_equals;
>
>
> Same here, why do we need both strings here stored separately? Could
> we instead store which state to check and store the string once? I'm
> not sure that really buys us any storage.
>

same as above, we utilize the fact that its easier to check the strings like

if (if_equals)
...
else (not_equals)
...

else we'd have to have an indicator to know which to check.

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length

[Karthik Nayak]

On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller  wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>> From: Karthik Nayak 
>>
>> Add support for %(objectname:short=) which would print the
>> abbreviated unique objectname of given length. When no length is
>> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
>> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
>> object name is unique.
>>
>
> Ok this makes sense. It may be annoying that the length might go
> beyond the size that we wanted, but I think it's better than printing
> a non-unique short abbreviation.
>
> I have one suggested change, which is to drop O_LENGTH and have
> O_SHORT store the length always, setting it to DEFAULT_ABBREV when no
> length provided. This allows you to drop some code. I don't think it's
> actually worth a re-roll by itself since the current code is correct.
>
> Thanks,
> Jake
>

That does make sense, It would also not error out when we use
%(objectname:short=) and
not specify the length. Idk, if that's desirable or not. But it does
make the code a little more
confusing to read at the same time.

So since its a small change, I'd be okay going either ways with this.

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 02/17] ref-filter: include reference to 'used_atom' within 'atom_value'

[Karthik Nayak]

>>
>>  /*
>> @@ -370,7 +368,7 @@ static void align_atom_handler(struct atom_value *atomv, 
>> struct ref_formatting_s
>> push_stack_element(>stack);
>> new = state->stack;
>> new->at_end = end_align_handler;
>> -   new->at_end_data = >u.align;
>> +   new->at_end_data = >atom->u.align;
>
> At first, this confused me. I was like "we dropped the union, why are
> we still referencing it. But I realized that the "used_atom" struct
> actually contains the same union and we were copying it.
>
> Ok, so this looks good.
>

It is confusing if one only looks at the patch without actually going
through ref-filter.c. I'm sure your comment will help
anyone going through these patches.

-- 
Regards,
Karthik Nayak

Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms

[Karthik Nayak]

Hey,

On Wed, Nov 9, 2016 at 4:43 AM, Jacob Keller  wrote:
> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak  wrote:
>> From: Karthik Nayak 
>>
>> +Some atoms like %(align) and %(if) always require a matching %(end).
>> +We call them "opening atoms" and sometimes denote them as %($open).
>> +
>> +When a scripting language specific quoting is in effect (i.e. one of
>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>> +atoms, replacement from every %(atom) is quoted when and only when it
>> +appears at the top-level (that is, when it appears outside
>> +%($open)...%(end)).
>> +
>> +When a scripting language specific quoting is in effect, everything
>> +between a top-level opening atom and its matching %(end) is evaluated
>> +according to the semantics of the opening atom and its result is
>> +quoted.
>> +
>>
>
> Nice, I like the explanation above.
>

All thanks to Eric, Junio, Christian, Matthieu and everyone else who
helped me phrase
these.

>>
>> +   }
>> +   } else if (!if_then_else->condition_satisfied)
>
> Minor nit. I'm not sure what standard we use here at Git, but
> traditionally, I prefer to see { } blocks on all sections even if only
> one of them needs it. (That is, only drop the braces when every
> section is one line.) It also looks weird with a comment since it
> appears as multiple lines to the reader. I think the braces improve
> readability.
>
> I don't know whether that's Git's code base standard or not, however.
> It's not really worth a re-roll unless something else would need to
> change.
>

I believe this is the syntax followed in Git, xdiff/xmerge.c:173 and so on.

You're comments are right on though.

-- 
Regards,
Karthik Nayak

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Jeff King]

On Thu, Nov 10, 2016 at 12:07:14PM +0100, Lars Schneider wrote:

> > Using Apache in the tests has been the source of frequent portability
> > problems and configuration headaches. I do wonder if we'd be better off
> > using some small special-purpose web server (even a short perl script
> > written around HTTP::Server::Simple or something).
> > 
> > On the other hand, testing against Apache approximates a more real-world
> > case, which has value. It might be nice if our tests supported multiple
> > web servers, but that would mean duplicating the config for each
> > manually.
> 
> I agree that the real-world Apache test is more valuable and I really want
> to keep the Linux Apache test running. However, I don't think many people
> use macOS as Git web server and therefore I thought it is not worth the
> effort to investigate this problem further.

IMHO, the value in the http tests is not testing the server side, but
the client side. Without being able to set up a dummy HTTP server, we do
not have any way to exercise the client side of git-over-http at all.
And people on macOS _do_ use that. :)

-Peff

Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter

[Jeff King]

On Thu, Nov 10, 2016 at 01:10:17PM +0100, Matthieu Moy wrote:

> Lars Schneider  writes:
> 
> > I haven't looked at an implemenation approach at all. I wonder if this could
> > be OK from a conceptional point of view or if there are obvious security 
> > problems that I am missing.
> 
> Did you consider just running "git credential" from the filter? It may
> not be the perfect solution, but it should work. I already used it to
> get credential from a remote-helper (git-remote-mediawiki). When
> prompting credentials interactively, it grabs the terminal directly, so
> it work even if stdin/stdout are used for the protocol.

Yeah, that is the solution I was going to suggest. The credentials are
totally orthogonal to the filters, and I would rather not shove them
into the protocol. It's an extra process, but with the new multi-use
smudge filter, it's one per git invocation, not one per file.

-Peff

Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter

[Matthieu Moy]

Lars Schneider  writes:

> I haven't looked at an implemenation approach at all. I wonder if this could
> be OK from a conceptional point of view or if there are obvious security 
> problems that I am missing.

Did you consider just running "git credential" from the filter? It may
not be the perfect solution, but it should work. I already used it to
get credential from a remote-helper (git-remote-mediawiki). When
prompting credentials interactively, it grabs the terminal directly, so
it work even if stdin/stdout are used for the protocol.

Asking the main git process to get the credentials probably has added
value like the ability to prompt once and use the same for several
filter processes.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

[RFC] Add way to make Git credentials accessible from clean/smudge filter

[Lars Schneider]

Hi,

we just implemented the first "real-world" user of the new clean/smudge 
"filter protocol" interface (see "convert: add filter..process option"
edcc858 for details) and the results are fantastic. Filtering 12,000 files in
my artificial test repo is more than 60x faster (depending on the platform). 
On Windows that means the clean/smudge operations runs in 57 seconds instead 
of 55 minutes [1]!

I have a number of ideas to improve the protocol even further and I am seeking
early feedback on the following - possibly most controversial - idea:

Some filters might want to perform additional network interactions and these
filters would like to use the Git credentials to perform these actions. If
such a filter is configured with "offerCredentials = true" then the filter 
could request the current Git credentials via the "filter-protocol".

A configuration could look like this:

[filter "myfilter"]
process = my-filter-process
required = true
offerCredentials = true


The default would, of course, be "offerCredentials = false".

I haven't looked at an implemenation approach at all. I wonder if this could
be OK from a conceptional point of view or if there are obvious security 
problems that I am missing.

Thanks,
Lars


[1] https://github.com/github/git-lfs/pull/1617#issuecomment-259411850

Re: [PATCH v2] rebase: add --forget to cleanup rebase, leave everything else untouched

[Duy Nguyen]

On Thu, Nov 10, 2016 at 6:09 PM, Duy Nguyen  wrote:
> On Thu, Nov 10, 2016 at 3:12 AM, Junio C Hamano  wrote:
>> Nguyễn Thái Ngọc Duy   writes:
>>
>>> ---
>>>  v2 changes just the subject line
>>
>> That's not sufficient, is it?  What you did in the documentation
>> would raise the same "Hmph, is this only about HEAD?" and unlike the
>> commit subject, it will carve it in stone for end-users.
>
> Oops. I forgot about git-rebase.txt. How about this?
>
> --forget::
> Abort the rebase operation but leave HEAD, the index and
> working tree untouched.

Or, since --abort describes it as "reset HEAD to the original branch",
we could write "Abort the rebase operation. Unlike --abort, HEAD is
not restored back to the original branch". Index and worktree are
implied by "not restored". Not sure if it's too subtle.
-- 
Duy

Re: [PATCH v1 0/2] Fix default macOS build locally and on Travis CI

[Lars Schneider]

> On 10 Nov 2016, at 00:39, Junio C Hamano  wrote:
> 
> larsxschnei...@gmail.com writes:
> 
>> From: Lars Schneider 
>> 
>> Apple removed the OpenSSL header files in macOS and therefore Git does
>> not build out of the box on macOS anymore. See previous discussion with
>> Torsten here: http://public-inbox.org/git/565b3036.8000...@web.de/
>> 
>> This mini series makes Git build out of the box on macOS, again, and
>> disables the HTTPD tests on macOS TravisCI as they don't work anymore
>> with the new macOS TravisCI default image:
>> https://blog.travis-ci.com/2016-10-04-osx-73-default-image-live/
>> 
>> Thanks,
>> Lars
>> 
>> 
>> Lars Schneider (2):
>>  config.mak.in: set NO_OPENSSL and APPLE_COMMON_CRYPTO for macOS >10.11
>>  travis-ci: disable GIT_TEST_HTTPD for macOS
>> 
>> .travis.yml  | 3 ++-
>> config.mak.uname | 6 ++
>> 2 files changed, 8 insertions(+), 1 deletion(-)
> 
> I've followed what was available at the public-inbox archive, but it
> is unclear what the conclusion was.  
> 
> For the first one your "how about" non-patch, to which Peff said
> "that's simple and good", looked good to me as well, but is it
> available as a final patch that I can just take and apply (otherwise
> I think I can do the munging myself, but I'd rather be spoon-fed
> when able ;-).

Sure! Here you go:
http://public-inbox.org/git/20161110111348.61580-1-larsxschnei...@gmail.com/


> I do not have a strong opinion on the second one.  For an interim
> solution, disabling webserver tests certainly is expedite and safe,
> so I am fine taking it as-is, but I may have missed strong
> objections.

I haven't seen strong objections either. Just for reference, here is the patch:
http://public-inbox.org/git/20161017002550.88782-3-larsxschnei...@gmail.com/


I hope you're well, again!!

- Lars

[PATCH v2] Makefile: set NO_OPENSSL on macOS by default

[larsxschneider]

From: Lars Schneider 

Apple removed the OpenSSL header files in macOS 10.11 and above. OpenSSL
was deprecated since macOS 10.7.

Set `NO_OPENSSL` and `APPLE_COMMON_CRYPTO` to `YesPlease` as default for
macOS. It is possible to override this and use OpenSSL by defining
`NO_APPLE_COMMON_CRYPTO`.

Signed-off-by: Lars Schneider 
---
 Makefile | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Makefile b/Makefile
index 9d6c245..f53fcc9 100644
--- a/Makefile
+++ b/Makefile
@@ -1047,6 +1047,7 @@ ifeq ($(uname_S),Darwin)
endif
endif
ifndef NO_APPLE_COMMON_CRYPTO
+   NO_OPENSSL = YesPlease
APPLE_COMMON_CRYPTO = YesPlease
COMPAT_CFLAGS += -DAPPLE_COMMON_CRYPTO
endif
-- 
2.10.2

Re: [PATCH v2] rebase: add --forget to cleanup rebase, leave everything else untouched

[Duy Nguyen]

On Thu, Nov 10, 2016 at 3:12 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> ---
>>  v2 changes just the subject line
>
> That's not sufficient, is it?  What you did in the documentation
> would raise the same "Hmph, is this only about HEAD?" and unlike the
> commit subject, it will carve it in stone for end-users.

Oops. I forgot about git-rebase.txt. How about this?

--forget::
Abort the rebase operation but leave HEAD, the index and
working tree untouched.

And think that's all the things rebase touches.
-- 
Duy

Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS

[Lars Schneider]

> On 07 Nov 2016, at 22:20, Jeff King  wrote:
> 
> On Sun, Nov 06, 2016 at 10:42:36PM +0100, Lars Schneider wrote:
> 
>>> From: Lars Schneider 
>>> 
>>> TravisCI changed their default macOS image from 10.10 to 10.11 [1].
>>> Unfortunately the HTTPD tests do not run out of the box using the
>>> pre-installed Apache web server anymore. Therefore we enable these
>>> tests only for Linux and disable them for macOS.
>> [...]
>> Hi Junio,
>> 
>> the patch above is one of two patches to make TravisCI pass, again.
>> Could you queue it?
> 
> I don't really mind disabling tests if they don't run on a platform. But
> the more interesting question to me is: why don't they run any more? Is
> there some config tweak needed, or is it an insurmountable problem?

I can't really remember what the problem was. I think some apache config
required some module that was not present and I wasn't able to get this
working quickly.


> Using Apache in the tests has been the source of frequent portability
> problems and configuration headaches. I do wonder if we'd be better off
> using some small special-purpose web server (even a short perl script
> written around HTTP::Server::Simple or something).
> 
> On the other hand, testing against Apache approximates a more real-world
> case, which has value. It might be nice if our tests supported multiple
> web servers, but that would mean duplicating the config for each
> manually.

I agree that the real-world Apache test is more valuable and I really want
to keep the Linux Apache test running. However, I don't think many people
use macOS as Git web server and therefore I thought it is not worth the
effort to investigate this problem further.

- Lars

Re: [PATCH 4/5] attr: do not respect symlinks for in-tree .gitattributes

[Duy Nguyen]

On Thu, Nov 10, 2016 at 7:23 AM, Jeff King  wrote:
> On Wed, Nov 09, 2016 at 04:18:29PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > On Wed, Nov 09, 2016 at 02:58:37PM -0800, Junio C Hamano wrote:
>> >
>> > I'm slightly confused. Did you mean "supporting any in-tree symlink to
>> > an out-of-tree destination" in your first sentence?
>>
>> I was trying to say that these "control files used solely by git"
>> have no business being a symbolic link pointing at anywhere, even
>> inside the same tree; actually, especially if it is inside the same
>> tree.
>
> OK. That is what my patch does (modulo .gitmodules, which I did not
> think of). But I think that is the opposite of Duy's opinion, as his
> review seemed to object to that.

I only objected the rationale (to be consistent with reading index).
If you sold it as malicious symlinks, or even put it like Junio "no,
the design makes more sense to be this way", I would be ok.

On the implementation side, we should print something friendlier than
strerror(ELOOP) if we decide that "symlinks on .git* files are wrong".
The standard ELOOP message does not communicate our design decision
well to the users. But this is a minor thing and can be ignored.

> As you know my ulterior motive is dealing with malicious out-of-tree
> symlinks, and I would be happy to deal with that directly. That still
> leaves symlinked ".gitmodules" etc in a funny state (they work in the
> filesystem but not in the index), but since nobody is _complaining_,
> it's a bug we could leave for another day.

The discussion trailed off a bit to symlinks in general in worktree
too, I think. But it's not my itch, we can leave it for another day.
-- 
Duy

Hello,

[pr-team4]

We currently have openings to fund early startups at the preferential interest 
rate of two percent per year.

Regards,
Larry Koetze

Re: Cleaning ignored files

[John Szakmeister]

On Wed, Nov 9, 2016 at 1:23 PM, Roman Terekhov  wrote:
> Hi,
>
> I want to ask about git clean -dXf command behaviour.
>
> I do the following:
>
> $ mkdir gitignore_test
> $ cd gitignore_test/
> $ git init
> Initialized empty Git repository in ~/gitignore_test/.git/
>
> $ echo *.sln > .gitignore
> $ git add .gitignore
> $ git commit -m "add gitignore"
> [master (root-commit) ef78a3c] add gitignore
>  1 file changed, 1 insertion(+)
>  create mode 100644 .gitignore
>
> $ mkdir src
> $ touch test.sln
> $ touch src/test.sln
> $ tree
> .
> ├── src
> │   └── test.sln
> └── test.sln
>
> 1 directory, 2 files
>
> $ git clean -dXf
> Removing test.sln
>
> $ tree
> .
> └── src
> └── test.sln
>
> 1 directory, 1 file
>
>
> Why git clean -dXf does not remove all my test.sln files, but just one of 
> them?

src/ is not under version control, and currently git does not descend
into unknown folders to remove ignored files.  If you had a tracked or
staged file in src/, then git would descend into src/ and remove
test.sln as expected.  In your example, try doing:

$ touch src/foo.c
$ git add src/foo.c
$ git clean -dXf
Removing src/test.sln
Removing test.sln

Hope that helps!

-John

[PATCH v2] t6026-merge-attr: don't fail if sleep exits early

[Andreas Schwab]

Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end
of test case") added a kill command to clean up after the test, but this
can fail if the sleep command exits before the cleanup is executed.
Ignore the error from the kill command.

Explicitly check for the existence of the pid file to test that the merge
driver was actually called.

Signed-off-by: Andreas Schwab 
---
 t/t6026-merge-attr.sh | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 7a6e33e673..03d13d00b5 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -187,13 +187,14 @@ test_expect_success 'custom merge does not lock index' '
sleep 1 &
echo $! >sleep.pid
EOF
-   test_when_finished "kill \$(cat sleep.pid)" &&
+   test_when_finished "kill \$(cat sleep.pid) || :" &&
 
test_write_lines >.gitattributes \
"* merge=ours" "text merge=sleep-one-second" &&
test_config merge.ours.driver true &&
test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
-   git merge master
+   git merge master &&
+   test -f sleep.pid
 '
 
 test_done
-- 
2.10.2


-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0

[stefan.naewe]

Am 05.11.2016 um 10:50 schrieb Johannes Schindelin:
> Dear Git users,
> 
> I finally got around to rebase the Windows-specific patches (which seem to
> not make it upstream as fast as we get new ones) on top of upstream Git
> v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
> 
> https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
> 
> It would be really nice if those of you who have access to Windows [*2*]
> could try it out and report bugs before v2.11.0 final.

I tried that version on 64bit Win7.
Somehow the PATH env. variable does no longer get set correctly.

On git-for-windows 2.10.2 my PATH (inside git bash) contains ~75 Entries.
With the above installer I only get ~17 !
(Inside cmd.exe I get 61 entries. I set some more entries with by ~/.bashrc)

It seems that all entries that contain a path with whitespace (e.g. 'c:\program 
files (x86)' )
are no longer there.

Any ideas ?

Thanks,
  Stefan
-- 

/dev/random says: Oxymoron: Stuck in traffic.
python -c "print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF