Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Thu, Jan 12, 2017 at 10:43 PM, Johannes Sixt  wrote:
> Am 13.01.2017 um 01:59 schrieb Jacob Keller:
>>
>> I think that --exclude makes sense, but the current implementation
>> does not differentiate ordering, since both are merely accumulated
>> into string_lists and then matched together. I'm not sure how order
>> would impact things here? In the current implementation, if something
>> is excluded and matched, it will be excluded. That is, exclusion
>> patterns take precedence over match patterns. I think this makes the
>> most sense semantically.
>
>
> When you write
>
>   git log --exclude=wip/* --branches --remotes
>
> --exclude applies only to --branches, not to --remotes.
>
>  When you write
>
>   git log --branches --exclude=origin/* --remotes
>
> --exclude=origin/* applies only to --remotes, but not to --branches.
>
> -- Hannes
>

Well for describe I don't think the order matters.

Thanks,
Jake

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Sixt]

Am 13.01.2017 um 01:59 schrieb Jacob Keller:

I think that --exclude makes sense, but the current implementation
does not differentiate ordering, since both are merely accumulated
into string_lists and then matched together. I'm not sure how order
would impact things here? In the current implementation, if something
is excluded and matched, it will be excluded. That is, exclusion
patterns take precedence over match patterns. I think this makes the
most sense semantically.


When you write

  git log --exclude=wip/* --branches --remotes

--exclude applies only to --branches, not to --remotes.

 When you write

  git log --branches --exclude=origin/* --remotes

--exclude=origin/* applies only to --remotes, but not to --branches.

-- Hannes

Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path

[Eric Wong]

Pat Pannuto  wrote:
> You may still want the 1/2 patch in this series, just to make things
> internally consistent with "-w" vs "use warnings;" inside git's perl
> scripts.

No, that is a step back.  "-w" affects the entire process, so it
spots more potential problems.  The "warnings" pragma is scoped
to the enclosing block, so it won't span across files.

Existing instances of "use warnings" should remain, but I would
rather support adding "-w" to scripts which do not have it (and
fixing newly-found warnings along the way).

Thanks.

[RFC-PATCH] lib-submodule-update.sh: define tests for recursing into submodules

[Stefan Beller]

Currently lib-submodule-update.sh provides 2 functions
test_submodule_switch and test_submodule_forced_switch that are used by a
variety of tests to ensure that submodules behave as expected. The current
expected behavior is that submodules are not touched at all.
(See 42639d2317a for the exact setup)

In the future we want to teach all these commands to properly recurse
into submodules. To do that, we'll add two testing functions to
submodule-update-lib.sh test_submodule_switch_recursing and
test_submodule_forced_switch_recursing.

These two functions behave in analogy to the already existing functions
just with a different expectation on submodule behavior. The submodule
in the working tree is expected to be updated to the recorded submodule
version. The behavior is analogous to e.g. the behavior of files in a
nested directory in the working tree, where a change to the working tree
handles any arising directory/file conflicts just fine.

Signed-off-by: Stefan Beller 
---

So I have been looking into implementing "checkout --recurse-submodules",
for quite some time, but the correct approach is to not just make git-checkout
support submodules, but all the working tree operations at the same time.

Currently all the working tree operations have a test that submodules are
not touched even when the superproject is messing with the submodule,
so all submodule operations are tested via the submodule library, e.g.:

test_submodule_switch "git pull"

would see what happens for git-pull. Below I am proposing a test
that can be used for any operation that is aware of submodules, e.g.
eventually we'll have checks like:

test_submodule_switch_recursing "git checkout --recurse-submodules"
# as well as
test_submodule_switch_recursing "git pull --recurse-submodules"

This RFC email is asking if the behavior is sound and expected for submodule
operations in the working tree.

Thanks,
Stefan

 t/lib-submodule-update.sh | 465 +-
 1 file changed, 463 insertions(+), 2 deletions(-)

diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
index 79cdd34a54..bf33c30409 100755
--- a/t/lib-submodule-update.sh
+++ b/t/lib-submodule-update.sh
@@ -4,6 +4,7 @@
 # - New submodule (no_submodule => add_sub1)
 # - Removed submodule (add_sub1 => remove_sub1)
 # - Updated submodule (add_sub1 => modify_sub1)
+# - Updated submodule recursively (modify_sub1 => modify_sub1_recursively)
 # - Submodule updated to invalid commit (add_sub1 => invalid_sub1)
 # - Submodule updated from invalid commit (invalid_sub1 => valid_sub1)
 # - Submodule replaced by tracked files in directory (add_sub1 =>
@@ -20,8 +21,8 @@
 # /   replace_sub1_with_directory
 #/O
 #   / ^
-#  /  modify_sub1
-#  O--O---O
+#  /  modify_sub1  modify_sub1_recursive
+#  O--O---OO
 #  ^  ^\  ^
 #  |  | \ remove_sub1
 #  |  |  -O-O
@@ -67,6 +68,14 @@ create_lib_submodule_repo () {
git add sub1 &&
git commit -m "Modify sub1" &&
 
+   git checkout -b "modify_sub1_recursively" "modify_sub1" &&
+   git -C sub1 checkout -b "add_nested_sub" &&
+   git -C sub1 submodule add --branch no_submodule ./. sub2 &&
+   git -C sub1 commit -a -m "add a nested submodule" &&
+   git add sub1 &&
+   git commit -a -m "update submodule, that updates a nested 
submodule" &&
+   git -C sub1 submodule deinit -f --all &&
+
git checkout -b "replace_sub1_with_directory" "add_sub1" &&
git submodule update &&
(
@@ -133,6 +142,15 @@ test_git_directory_is_unchanged () {
)
 }
 
+test_git_directory_exists() {
+   test -e ".git/modules/$1" &&
+   (
+   cd ".git/modules/$1" &&
+   # does core.worktree point at the right place?
+   test "$(git config core.worktree)" = "../../../$1"
+   )
+}
+
 # Helper function to be executed at the start of every test below, it sets up
 # the submodule repo if it doesn't exist and configures the most problematic
 # settings for diff.ignoreSubmodules.
@@ -678,3 +696,446 @@ test_submodule_forced_switch () {
)
'
 }
+
+# Test that submodule contents are correctly updated when switching
+# between commits that change a submodule.
+# Test that the following transitions are correctly handled:
+# (These tests are also above in the case where we expect no change
+#  in the submodule)
+# - Updated submodule
+# - New submodule
+# - Removed submodule
+# - Directory containing tracked files replaced by submodule
+# - Submodule replaced by tracked files in directory
+# - Submodule replaced by tracked file with the same name
+# - tracked file replaced by submodule
+#
+# New test cases
+# - Removing a 

Re: Bug report: Documentation error in git-bisect man description

[Manuel Ullmann]

> On Fri, Jan 13, 2017 at 12:42 AM, Junio C Hamano  wrote:
>> Junio C Hamano  writes:
>>
>>> Manuel Ullmann  writes:
>>>
>>> Hmmm, I tend to agree, modulo a minor fix.
>>>
>>> If the description were in a context inside a paragraph like this:
>>>
>>>   When you want to tell 'git bisect' that a  belongs to
>>>   the newer half of the history, you say
>>>
>>>   git bisect (bad|new) []
>>>
>>>   On the other hand, when you want to tell 'git bisect' that a
>>>belongs to the older half of the history, you can say
>>>
>>>   git bisect (good|old) []
>>>
>>> then the pairing we see in the current text makes quite a lot of
>>> sense.
>>
>> Actually, the above is _exactly_ what was intended.  I misread the
>> current documentation when I made the comment, and I think that the
>> current one _IS_ correct.  The latter half of the above is not about
>> a single rev.  You can paint multiple commits with the "older half"
>> color, i.e.
>>
>> On the other hand, when you want to tell 'git bisect' that
>> one or more s  belong to the older half of the history,
>> you can say
>>
>> git bisect (good|old) [...]
>>
>> In contrast, you can mark only one  as newer (or "already
>> bad").  So pairing (bad|good) and (new|old) like you suggested
>> breaks the correctness of the command line description.
>
> Yeah, I agree.
>
>> If (bad|new) and (good|old) bothers you because they may mislead the
>> readers to think bad is an opposite of new (and good is an opposite
>> of old), the only solution I can think of to that problem is to
>> expand these two lines into four and list them like this:
>>
>> git bisect bad []
>> git bisect good [...]
>> git bisect new []
>> git bisect old [...]
>
> Maybe it would be more complete and a bit clearer if it was:
>
>git bisect (bad|new|) []
>git bisect (good|old|) [...]

That would clarify the intention quite a bit (at least for me).

Best regards,
Manuel

Re: Bug report: Documentation error in git-bisect man description

[Christian Couder]

On Fri, Jan 13, 2017 at 12:42 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Manuel Ullmann  writes:
>>
>> Hmmm, I tend to agree, modulo a minor fix.
>>
>> If the description were in a context inside a paragraph like this:
>>
>>   When you want to tell 'git bisect' that a  belongs to
>>   the newer half of the history, you say
>>
>>   git bisect (bad|new) []
>>
>>   On the other hand, when you want to tell 'git bisect' that a
>>belongs to the older half of the history, you can say
>>
>>   git bisect (good|old) []
>>
>> then the pairing we see in the current text makes quite a lot of
>> sense.
>
> Actually, the above is _exactly_ what was intended.  I misread the
> current documentation when I made the comment, and I think that the
> current one _IS_ correct.  The latter half of the above is not about
> a single rev.  You can paint multiple commits with the "older half"
> color, i.e.
>
> On the other hand, when you want to tell 'git bisect' that
> one or more s  belong to the older half of the history,
> you can say
>
> git bisect (good|old) [...]
>
> In contrast, you can mark only one  as newer (or "already
> bad").  So pairing (bad|good) and (new|old) like you suggested
> breaks the correctness of the command line description.

Yeah, I agree.

> If (bad|new) and (good|old) bothers you because they may mislead the
> readers to think bad is an opposite of new (and good is an opposite
> of old), the only solution I can think of to that problem is to
> expand these two lines into four and list them like this:
>
> git bisect bad []
> git bisect good [...]
> git bisect new []
> git bisect old [...]

Maybe it would be more complete and a bit clearer if it was:

   git bisect (bad|new|) []
   git bisect (good|old|) [...]

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Jacob Keller]

On Thu, Jan 12, 2017 at 5:45 AM, Johannes Sixt  wrote:
> Am 12.01.2017 um 01:17 schrieb Jacob Keller:
>>
>> From: Jacob Keller 
>>
>> Teach git-describe the `--discard` option which will allow specifying
>> a glob pattern of tags to ignore. This can be combined with the
>> `--match` patterns to enable more flexibility in determining which tags
>> to consider.
>>
>> For example, suppose you wish to find the first official release tag
>> that contains a certain commit. If we assume that official release tags
>> are of the form "v*" and pre-release candidates include "*rc*" in their
>> name, we can now find the first tag that introduces commit abcdef via:
>>
>>   git describe --contains --match="v*" --discard="*rc*"
>
>
> I have a few dozen topic branches, many of them are work in progress and
> named wip/something. To see the completed branches, I routinely say
>
> gitk --exclude=wip/* --branches
>
> these days.
>
> It would be great if you could provide the same user interface here. The
> example in the commit message would then look like this:
>
>git describe --contains --exclude="*rc*" --match="v*"
>
> (I'm not saying that you should add --branches, but that you should prefer
> --exclude over --discard. Also, the order of --exclude and --match would be
> important.)

I think that --exclude makes sense, but the current implementation
does not differentiate ordering, since both are merely accumulated
into string_lists and then matched together. I'm not sure how order
would impact things here? In the current implementation, if something
is excluded and matched, it will be excluded. That is, exclusion
patterns take precedence over match patterns. I think this makes the
most sense semantically.

Thanks,
Jake

>
> -- Hannes
>

Re: [PATCH 3/5] name-rev: add support to discard refs by pattern match

[Jacob Keller]

On Thu, Jan 12, 2017 at 1:57 AM, Johannes Schindelin
 wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> From: Jacob Keller 
>>
>> Extend name-rev further to support matching refs by adding `--discard`
>> patterns.
>
> Same comment applies as for 5/5: `--exclude-refs` may be a better name
> than `--discard`.
>

I agree, will change.

Thanks,
Jake

> Ciao,
> Dscho

Re: [PATCH 2/5] name-rev: extend --refs to accept multiple patterns

[Jacob Keller]

On Thu, Jan 12, 2017 at 1:56 AM, Johannes Schindelin
 wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
>> b/t/t6007-rev-list-cherry-pick-file.sh
>> index 1408b608eb03..d072ec43b016 100755
>> --- a/t/t6007-rev-list-cherry-pick-file.sh
>> +++ b/t/t6007-rev-list-cherry-pick-file.sh
>> @@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up 
>> empty (II)' '
>>   test_cmp actual.named expect
>>  '
>>
>> +test_expect_success 'name-rev multiple --refs combine inclusive' '
>> + git rev-list --left-right --cherry-pick F...E -- bar > actual &&
>
> Our current coding style seems to skip the space between `>` and `actual`
> (this applies to all redirections added in this patch).
>

Right that's easy to fix.

>> + git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
>> + < actual > actual.named &&
>> + test_cmp actual.named expect
>> +'
>> +
>> +cat >expect <> +> +$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
>> +EOF
>
> In the current revision of t6007, we seem to list the expected output
> explicitly, i.e. *not* generating it dynamically.
>
> If you *do* insist to generate the `expect` file dynamically, a better way
> would be to include that generation in the `test_expect_success` code so
> that errors in the call can be caught, too:
>

The main reason I don't like static expecations is that often other
tests inserted before or after my test now have to know what to put
here. Specifically, the problem is that we expect the output not to be
named, but actually to be sha1 hex output. This seems really brittle
for a test.

> test_expect_success 'name-rev --refs excludes non-matched patterns' '
> echo "expect &&
> git rev-list --left-right --right-only --cherry-pick F...E -- \
> bar >>expect &&
> [...]
>
> However, if I was asked for my preference, I would suggest to specify the
> `expect` contents explicitly, to document the expectation as of time of
> writing. The reason: I debugged my share of test breakages and these
> dynamically-generated `expect` files are the worst. When things break, you
> have to dig *real* deep to figure out what is going wrong, as sometimes
> the *generation of the `expect` file* regresses.
>

Do you have a better suggestion for how to check the expect vs actual
output ignoring the raw hex data that would be there otherwise? What I
want to avoid is a brittle test that depends precisely on actions of
prior tests in generating commits and tags. Specifically in this case,
we're going to end up with the sha1 hex ID of the commit in the
expected value.. hard-coding that feels wrong.

Thanks,
Jake

> Ciao,
> Dscho

Re: [PATCH 1/5] doc: add documentation for OPT_STRING_LIST

[Jacob Keller]

On Thu, Jan 12, 2017 at 1:47 AM, Johannes Schindelin
 wrote:
> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> diff --git a/Documentation/technical/api-parse-options.txt 
>> b/Documentation/technical/api-parse-options.txt
>> index 27bd701c0d68..15e876e4c804 100644
>> --- a/Documentation/technical/api-parse-options.txt
>> +++ b/Documentation/technical/api-parse-options.txt
>> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>>   Introduce an option with string argument.
>>   The string argument is put into `str_var`.
>>
>> +`OPT_STRING_LIST(short, long, , arg_str, description)`::
>> + Introduce an option with a string argument. Repeated invocations
>> + accumulate into a list of strings. Reset and clear the list with
>> + `--no-option`.
>
> One suggestions: as the list parameter is not type-safe (apart from
> checking that it can be cast to a `void *`), it would be good to mention
> in the documentation that `list` must be of type `struct string_list`.
>

Ok.

> I was about to suggest that `--no-option` may be misleading, as the
> command-line option is not really called `--option` in almost all cases,
> but I see that the rest of that document uses that convention to refer to
> the negated option already...

Also, I am merely documenting what already existed, since the original
commit failed to add documentation. I don't know if we could make a
better implementation, but it certainly would be a behavior change for
the current users.

Thanks,
Jake

>
> Ciao,
> Dscho

Re: Bug report: Documentation error in git-bisect man description

[Manuel Ullmann]

I see. Thanks for the clarification. The pairing not being pairs of
opposites was indeed, what confused me. So that description was not
meant in the sense, that you use these pairs when working with bisect, but
instead they are ordered according to the argument possibilities.

Sorry for spreading confusion. I think the second paragraph should be
sufficient for documentation.

Best regards
Manuel
> Junio C Hamano  writes:
>
>> Manuel Ullmann  writes:
>>
>> Hmmm, I tend to agree, modulo a minor fix.
>>
>> If the description were in a context inside a paragraph like this:
>>
>>  When you want to tell 'git bisect' that a  belongs to
>>  the newer half of the history, you say
>>
>>  git bisect (bad|new) []
>>
>>  On the other hand, when you want to tell 'git bisect' that a
>>   belongs to the older half of the history, you can say
>>
>>  git bisect (good|old) []
>>
>> then the pairing we see in the current text makes quite a lot of
>> sense.
>
> Actually, the above is _exactly_ what was intended.  I misread the
> current documentation when I made the comment, and I think that the
> current one _IS_ correct.  The latter half of the above is not about
> a single rev.  You can paint multiple commits with the "older half"
> color, i.e.
>
>   On the other hand, when you want to tell 'git bisect' that
>   one or more s  belong to the older half of the history,
>   you can say
>
>   git bisect (good|old) [...]
>
> In contrast, you can mark only one  as newer (or "already
> bad").  So pairing (bad|good) and (new|old) like you suggested
> breaks the correctness of the command line description.
>
> If (bad|new) and (good|old) bothers you because they may mislead the
> readers to think bad is an opposite of new (and good is an opposite
> of old), the only solution I can think of to that problem is to
> expand these two lines into four and list them like this:
>
> git bisect bad []
> git bisect good [...]
> git bisect new []
> git bisect old [...]

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

[Brandon Williams]

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: Brandon Williams 
---
 attr.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/attr.c b/attr.c
index 04d24334e..007f1a299 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.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index 6b55a57ef..9bdf87a6f 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.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

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: Brandon Williams 
---
 attr.c | 12 
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/attr.c b/attr.c
index f7cf7ae30..d180c7833 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.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index 007f1a299..6b55a57ef 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.11.0.390.gc69c2f50cf-goog

[PATCH 14/27] attr: rename function and struct related to checking attributes

[Brandon Williams]

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 attr_check_item" and rename
the function to "git_check_attrs()".

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Brandon Williams 
---
 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 01751e574..b76bd4691 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 attr_check_item *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 attr_check_item 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 50e5ee393..2f180d609 100644
--- a/attr.c
+++ b/attr.c
@@ -56,7 +56,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
 static int cannot_trust_maybe_real;
 
 /* NEEDSWORK: This will become per git_attr_check */
-static struct git_attr_check *check_all_attr;
+static struct attr_check_item *check_all_attr;
 
 const char *git_attr_name(const struct git_attr *attr)
 {
@@ -713,7 +713,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 attr_check_item *check = check_all_attr;
int i;
 
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
@@ -778,7 +778,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 attr_check_item *check)
 
 {
struct attr_stack *stk;
@@ -806,7 +806,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 attr_check_item *c;
c = check_all_attr + check[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
@@ -821,7 +821,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 attr_check_item *check)
 {
int i;
 
@@ -837,7 +837,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 attr_check_item **check)
 {
int i, count, j;
 
diff --git a/attr.h b/attr.h
index 00d7a662c..efc7bb3b3 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 attr_check_item {
  

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

[Brandon Williams]

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: Brandon Williams 
---
 attr.c | 11 ---
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/attr.c b/attr.c
index 17297fffe..e42f931b3 100644
--- a/attr.c
+++ b/attr.c
@@ -705,24 +705,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.11.0.390.gc69c2f50cf-goog

[PATCH 10/27] attr: support quoting pathname patterns in C style

[Brandon Williams]

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: Brandon Williams 
---
 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 e0b66c122..3173dee7e 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 d180c7833..e1c630f79 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";
@@ -212,12 +213,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) {
@@ -239,7 +249,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 */
@@ -282,9 +291,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 f0fbb4255..f19ae4f8c 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.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index 9bdf87a6f..17297fffe 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.11.0.390.gc69c2f50cf-goog

[PATCH 13/27] attr.c: outline the future plans by heavily commenting

[Brandon Williams]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index 8026d68bd..50e5ee393 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 *check_all_attr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 const char *git_attr_name(const struct git_attr *attr)
 {
@@ -102,6 +116,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;
@@ -318,6 +337,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;
@@ -382,6 +402,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.11.0.390.gc69c2f50cf-goog

[PATCH 11/27] attr.c: add push_stack() helper

[Brandon Williams]

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: Brandon Williams 
---
 attr.c | 71 +++---
 1 file changed, 33 insertions(+), 38 deletions(-)

diff --git a/attr.c b/attr.c
index e1c630f79..8026d68bd 100644
--- a/attr.c
+++ b/attr.c
@@ -510,6 +510,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;
@@ -517,37 +529,23 @@ 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);
}
 
@@ -558,15 +556,12 @@ static void bootstrap_attr_stack(void)
 
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;
 
/*
@@ -626,20 +621,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);
}
 
@@ -649,8 +645,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
/*
 * Finally push the "info" one at the top of the stack.
   

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

[Brandon Williams]

From: Junio C Hamano 

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

diff --git a/commit.c b/commit.c
index 2cf85158b..0c4ee3de4 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.11.0.390.gc69c2f50cf-goog

[PATCH 00/27] Revamp the attribute system; another round

[Brandon Williams]

This series has been bounced around a bit (from Junio to Stefan) and finally
landed in my lap.  The end result of Stefan's attempt at the series still had a
couple of things that needed more tweaking.  It also has a few patches on top
which added functionality to pathspecs to be able to query into the attribute
system, which I've dropped from this series due to this series' length.

As a reminder the intent of this series is to revamp the attribute system so
that it can be thread-safe as well as a couple of other quality of life
changes.  This entailed removing dependencies on writing global data structures
during the attribute collection process.  Major changes are as follows:

 * The global array used to collect attributes needed to be made local and as a
   result was pushed out to the attr_check structure the caller prepares before
   querying the attribute system.

 * As it turns out the attribute stack ends up being used as a read-only
   structure during the collection process and as such parts of the attribute
   stack can be shared between different threads calling into the system.  To
   enable this sharing the attribute stack frames are stored in a hashmap and
   can be read out (or created and stored in the hashmap) based on the
   directory name of the path being queried.  This is possible because if a
   particular stack frame is included in the overall stack for a particular
   query, all of the frames underneath it will be the same for all queries that
   use this frame (only exception is the info frame which is handled special
   case, see the patch for details).

I took many of the first patches of this series as is from the series Stefan
prepared as as such may only need a cursory glace.  I did modify and change
some of the later patches authored by Junio to address a couple of naming
changes and to redistribute some code between patches so those patches would
need a closer look.

Thanks again to all the work Junio and Stefan put into this before I got a hold
of it.

Any comments are appreciated!

Thanks,
Brandon Williams

Brandon Williams (8):
  attr: pass struct attr_check to collect_some_attrs
  attr: use hashmap for attribute dictionary
  attr: eliminate global check_all_attr array
  attr: remove maybe-real, maybe-macro from git_attr
  attr: tighten const correctness with git_attr and match_attr
  attr: store attribute stacks in hashmap
  attr: push the bare repo check into read_attr()
  attr: reformat git_attr_set_direction() function

Junio C Hamano (17):
  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.c: add push_stack() helper
  attr.c: outline the future plans by heavily commenting
  attr: rename function and struct related to checking attributes
  attr: (re)introduce git_check_attr() and struct attr_check
  attr: convert git_all_attrs() to use "struct attr_check"
  attr: convert git_check_attrs() callers to use the new API
  attr: retire git_check_attrs() API
  attr: change validity check for attribute names to use positive logic

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

Stefan Beller (1):
  Documentation: fix a typo

 Documentation/gitattributes.txt   |  10 +-
 Documentation/technical/api-gitattributes.txt |  86 ++-
 archive.c |  24 +-
 attr.c| 932 +-
 attr.h|  50 +-
 builtin/check-attr.c  |  66 +-
 builtin/pack-objects.c|  19 +-
 commit.c  |   3 +-
 common-main.c |   3 +
 convert.c |  25 +-
 ll-merge.c|  33 +-
 t/t0003-attributes.sh |  26 +
 userdiff.c|  19 +-
 ws.c  |  19 +-
 14 files changed, 834 insertions(+), 481 deletions(-)

-- 
2.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

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: Brandon Williams 
---
 attr.c | 2 +-
 attr.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/attr.c b/attr.c
index e42f931b3..f7cf7ae30 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 8b08d33af..00d7a662c 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.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

From: Junio C Hamano 

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

diff --git a/attr.c b/attr.c
index 1fcf042b8..04d24334e 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.11.0.390.gc69c2f50cf-goog

[PATCH 12/27] Documentation: fix a typo

[Brandon Williams]

From: Stefan Beller 

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

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 3173dee7e..a53d093ca 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.11.0.390.gc69c2f50cf-goog

[PATCH 21/27] attr: use hashmap for attribute dictionary

[Brandon Williams]

The current implementation of the attribute dictionary uses a custom
hashtable.  This modernizes the dictionary by converting it to the builtin
'hashmap' structure.

Also, in order to enable a threaded API in the future add an
accompanying mutex which must be acquired prior to accessing the
dictionary of interned attributes.

Signed-off-by: Brandon Williams 
---
 attr.c| 171 ++
 attr.h|   2 +
 common-main.c |   3 ++
 3 files changed, 131 insertions(+), 45 deletions(-)

diff --git a/attr.c b/attr.c
index 5399e1cb3..8cf2ea901 100644
--- a/attr.c
+++ b/attr.c
@@ -14,6 +14,7 @@
 #include "dir.h"
 #include "utf8.h"
 #include "quote.h"
+#include "thread-utils.h"
 
 const char git_attr__true[] = "(builtin)true";
 const char git_attr__false[] = "\0(builtin)false";
@@ -23,28 +24,17 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 #define ATTR__UNSET NULL
 #define ATTR__UNKNOWN git_attr__unknown
 
-/* This is a randomly chosen prime. */
-#define HASHSIZE 257
-
 #ifndef DEBUG_ATTR
 #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;
-   int attr_nr;
+   int attr_nr; /* unique attribute number */
int maybe_macro;
int maybe_real;
-   char name[FLEX_ARRAY];
+   char name[FLEX_ARRAY]; /* attribute name */
 };
 static int attr_nr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 /*
  * NEEDSWORK: maybe-real, maybe-macro are not property of
@@ -63,15 +53,94 @@ const char *git_attr_name(const struct git_attr *attr)
return attr->name;
 }
 
-static unsigned hash_name(const char *name, int namelen)
+struct attr_hashmap {
+   struct hashmap map;
+#ifndef NO_PTHREADS
+   pthread_mutex_t mutex;
+#endif
+};
+
+static inline void hashmap_lock(struct attr_hashmap *map)
 {
-   unsigned val = 0, c;
+#ifndef NO_PTHREADS
+   pthread_mutex_lock(>mutex);
+#endif
+}
 
-   while (namelen--) {
-   c = *name++;
-   val = ((val << 7) | (val >> 22)) ^ c;
-   }
-   return val;
+static inline void hashmap_unlock(struct attr_hashmap *map)
+{
+#ifndef NO_PTHREADS
+   pthread_mutex_unlock(>mutex);
+#endif
+}
+
+/*
+ * The global dictionary of all interned attributes.  This
+ * is a singleton object which is shared between threads.
+ * Access to this dictionary must be surrounded with a mutex.
+ */
+static struct attr_hashmap g_attr_hashmap;
+
+/* The container for objects stored in "struct attr_hashmap" */
+struct attr_hash_entry {
+   struct hashmap_entry ent; /* must be the first member! */
+   const char *key; /* the key; memory should be owned by value */
+   size_t keylen; /* length of the key */
+   void *value; /* the stored value */
+};
+
+/* attr_hashmap comparison function */
+static int attr_hash_entry_cmp(const struct attr_hash_entry *a,
+  const struct attr_hash_entry *b,
+  void *unused)
+{
+   return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
+}
+
+/* Initialize an 'attr_hashmap' object */
+void attr_hashmap_init(struct attr_hashmap *map)
+{
+   hashmap_init(>map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
+}
+
+/*
+ * Retrieve the 'value' stored in a hashmap given the provided 'key'.
+ * If there is no matching entry, return NULL.
+ */
+static void *attr_hashmap_get(struct attr_hashmap *map,
+ const char *key, size_t keylen)
+{
+   struct attr_hash_entry k;
+   struct attr_hash_entry *e;
+
+   if (!map->map.tablesize)
+   attr_hashmap_init(map);
+
+   hashmap_entry_init(, memhash(key, keylen));
+   k.key = key;
+   k.keylen = keylen;
+   e = hashmap_get(>map, , NULL);
+
+   return e ? e->value : NULL;
+}
+
+/* Add 'value' to a hashmap based on the provided 'key'. */
+static void attr_hashmap_add(struct attr_hashmap *map,
+const char *key, size_t keylen,
+void *value)
+{
+   struct attr_hash_entry *e;
+
+   if (!map->map.tablesize)
+   attr_hashmap_init(map);
+
+   e = xmalloc(sizeof(struct attr_hash_entry));
+   hashmap_entry_init(e, memhash(key, keylen));
+   e->key = key;
+   e->keylen = keylen;
+   e->value = value;
+
+   hashmap_add(>map, e);
 }
 
 static int attr_name_valid(const char *name, size_t namelen)
@@ -103,37 +172,44 @@ static void report_invalid_attr(const char *name, size_t 
len,
strbuf_release();
 }
 
-static struct git_attr *git_attr_internal(const char *name, int len)
+/*
+ * Given a 'name', lookup and return the corresponding attribute in the global
+ * dictionary.  If no entry is found, 

[PATCH 22/27] attr: eliminate global check_all_attr array

[Brandon Williams]

Currently there is a reliance on 'check_all_attr' which is a global
array of 'attr_check_item' items which is used to store the value of
each attribute during the collection process.

This patch eliminates this global and instead creates an array per
'attr_check' instance which is then used in the attribute collection
process.  This brings the attribute system one step closer to being
thread-safe.

Signed-off-by: Brandon Williams 
---
 attr.c | 114 +++--
 attr.h |   2 ++
 2 files changed, 78 insertions(+), 38 deletions(-)

diff --git a/attr.c b/attr.c
index 8cf2ea901..38b0d4347 100644
--- a/attr.c
+++ b/attr.c
@@ -34,7 +34,6 @@ struct git_attr {
int maybe_real;
char name[FLEX_ARRAY]; /* attribute name */
 };
-static int attr_nr;
 
 /*
  * NEEDSWORK: maybe-real, maybe-macro are not property of
@@ -45,9 +44,6 @@ static int attr_nr;
  */
 static int cannot_trust_maybe_real;
 
-/* NEEDSWORK: This will become per git_attr_check */
-static struct attr_check_item *check_all_attr;
-
 const char *git_attr_name(const struct git_attr *attr)
 {
return attr->name;
@@ -143,6 +139,52 @@ static void attr_hashmap_add(struct attr_hashmap *map,
hashmap_add(>map, e);
 }
 
+/*
+ * Reallocate and reinitialize the array of all attributes (which is used in
+ * the attribute collection process) in 'check' based on the global dictionary
+ * of attributes.
+ */
+static void all_attrs_init(struct attr_hashmap *map, struct attr_check *check)
+{
+   int i;
+
+   hashmap_lock(map);
+
+   if (map->map.size < check->all_attrs_nr)
+   die("BUG: interned attributes shouldn't be deleted");
+
+   /*
+* If the number of attributes in the global dictionary has increased
+* (or this attr_check instance doesn't have an initialized all_attrs
+* field), reallocate the provided attr_check instance's all_attrs
+* field and fill each entry with its corresponding git_attr.
+*/
+   if (map->map.size != check->all_attrs_nr) {
+   struct attr_hash_entry *e;
+   struct hashmap_iter iter;
+   hashmap_iter_init(>map, );
+
+   REALLOC_ARRAY(check->all_attrs, map->map.size);
+   check->all_attrs_nr = map->map.size;
+
+   while ((e = hashmap_iter_next())) {
+   const struct git_attr *a = e->value;
+   check->all_attrs[a->attr_nr].attr = a;
+   }
+   }
+
+   hashmap_unlock(map);
+
+   /*
+* Re-initialize every entry in check->all_attrs.
+* This re-initialization can live outside of the locked region since
+* the attribute dictionary is no longer being accessed.
+*/
+   for (i = 0; i < check->all_attrs_nr; i++) {
+   check->all_attrs[i].value = ATTR__UNKNOWN;
+   }
+}
+
 static int attr_name_valid(const char *name, size_t namelen)
 {
/*
@@ -196,16 +238,6 @@ static struct git_attr *git_attr_internal(const char 
*name, int namelen)
 
attr_hashmap_add(_attr_hashmap, a->name, namelen, a);
assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
-
-   /*
-* 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;
-   assert(a->attr_nr == (attr_nr - 1));
}
 
hashmap_unlock(_attr_hashmap);
@@ -791,16 +823,16 @@ static int path_matches(const char *pathname, int pathlen,
  pattern, prefix, pat->patternlen, pat->flags);
 }
 
-static int macroexpand_one(int attr_nr, int rem);
+static int macroexpand_one(struct attr_check_item *all_attrs, int nr, int rem);
 
-static int fill_one(const char *what, struct match_attr *a, int rem)
+static int fill_one(const char *what, struct attr_check_item *all_attrs,
+   struct match_attr *a, int rem)
 {
-   struct attr_check_item *check = check_all_attr;
int i;
 
-   for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
+   for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) {
struct git_attr *attr = a->state[i].attr;
-   const char **n = &(check[attr->attr_nr].value);
+   const char **n = &(all_attrs[attr->attr_nr].value);
const char *v = a->state[i].setto;
 
if (*n == ATTR__UNKNOWN) {
@@ -809,14 +841,15 @@ static int fill_one(const char *what, struct match_attr 
*a, int rem)
  attr, v);
*n = v;
rem--;
-   rem = macroexpand_one(attr->attr_nr, rem);
+   

[PATCH 26/27] attr: push the bare repo check into read_attr()

[Brandon Williams]

Push the bare repository check into the 'read_attr()' function.  This
avoids needing to have extra logic which creates an empty stack frame
when inside a bare repo as a similar bit of logic already exists in the
'read_attr()' function.

Signed-off-by: Brandon Williams 
---
 attr.c | 40 
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/attr.c b/attr.c
index 78562592b..cbb07d25d 100644
--- a/attr.c
+++ b/attr.c
@@ -591,25 +591,29 @@ static struct attr_stack *read_attr_from_index(const char 
*path, int macro_ok)
 
 static struct attr_stack *read_attr(const char *path, int macro_ok)
 {
-   struct attr_stack *res;
+   struct attr_stack *res = NULL;
 
-   if (direction == GIT_ATTR_CHECKOUT) {
+   if (direction == GIT_ATTR_INDEX) {
res = read_attr_from_index(path, macro_ok);
-   if (!res)
-   res = read_attr_from_file(path, macro_ok);
-   }
-   else if (direction == GIT_ATTR_CHECKIN) {
-   res = read_attr_from_file(path, macro_ok);
-   if (!res)
-   /*
-* There is no checked out .gitattributes file there, 
but
-* we might have it in the index.  We allow operation 
in a
-* sparsely checked out work tree, so read from it.
-*/
+   } else if (!is_bare_repository()) {
+   if (direction == GIT_ATTR_CHECKOUT) {
res = read_attr_from_index(path, macro_ok);
+   if (!res)
+   res = read_attr_from_file(path, macro_ok);
+   }
+   else if (direction == GIT_ATTR_CHECKIN) {
+   res = read_attr_from_file(path, macro_ok);
+   if (!res)
+   /*
+* There is no checked out .gitattributes file
+* there, but we might have it in the index.
+* We allow operation in a sparsely checked out
+* work tree, so read from it.
+*/
+   res = read_attr_from_index(path, macro_ok);
+   }
}
-   else
-   res = read_attr_from_index(path, macro_ok);
+
if (!res)
res = xcalloc(1, sizeof(*res));
return res;
@@ -796,11 +800,7 @@ static const struct attr_stack *core_attr_stack(void)
}
 
/* root directory */
-   if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
-   e = read_attr(GITATTRIBUTES_FILE, 1);
-   } else {
-   e = xcalloc(1, sizeof(struct attr_stack));
-   }
+   e = read_attr(GITATTRIBUTES_FILE, 1);
key = "";
push_stack(, e, key, strlen(key));
}
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH 24/27] attr: tighten const correctness with git_attr and match_attr

[Brandon Williams]

Signed-off-by: Brandon Williams 
---
 attr.c   | 14 +++---
 attr.h   |  2 +-
 builtin/check-attr.c |  3 ++-
 3 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/attr.c b/attr.c
index 633a12cc3..90f576044 100644
--- a/attr.c
+++ b/attr.c
@@ -209,7 +209,7 @@ static void report_invalid_attr(const char *name, size_t 
len,
  * dictionary.  If no entry is found, create a new attribute and store it in
  * the dictionary.
  */
-static struct git_attr *git_attr_internal(const char *name, int namelen)
+static const struct git_attr *git_attr_internal(const char *name, int namelen)
 {
struct git_attr *a;
 
@@ -233,14 +233,14 @@ static struct git_attr *git_attr_internal(const char 
*name, int namelen)
return a;
 }
 
-struct git_attr *git_attr(const char *name)
+const struct git_attr *git_attr(const char *name)
 {
return git_attr_internal(name, strlen(name));
 }
 
 /* What does a matched pattern decide? */
 struct attr_state {
-   struct git_attr *attr;
+   const struct git_attr *attr;
const char *setto;
 };
 
@@ -267,7 +267,7 @@ struct pattern {
 struct match_attr {
union {
struct pattern pat;
-   struct git_attr *attr;
+   const struct git_attr *attr;
} u;
char is_macro;
unsigned num_attr;
@@ -814,7 +814,7 @@ static int fill_one(const char *what, struct 
attr_check_item *all_attrs,
int i;
 
for (i = a->num_attr - 1; rem > 0 && i >= 0; i--) {
-   struct git_attr *attr = a->state[i].attr;
+   const struct git_attr *attr = a->state[i].attr;
const char **n = &(all_attrs[attr->attr_nr].value);
const char *v = a->state[i].setto;
 
@@ -838,7 +838,7 @@ static int fill(const char *path, int pathlen, int 
basename_offset,
const char *base = stk->origin ? stk->origin : "";
 
for (i = stk->num_matches - 1; 0 < rem && 0 <= i; i--) {
-   struct match_attr *a = stk->attrs[i];
+   const struct match_attr *a = stk->attrs[i];
if (a->is_macro)
continue;
if (path_matches(path, pathlen, basename_offset,
@@ -988,7 +988,7 @@ struct attr_check *attr_check_initl(const char *one, ...)
check->check[0].attr = git_attr(one);
va_start(params, one);
for (cnt = 1; cnt < check->check_nr; cnt++) {
-   struct git_attr *attr;
+   const struct git_attr *attr;
param = va_arg(params, const char *);
if (!param)
die("BUG: counted %d != ended at %d",
diff --git a/attr.h b/attr.h
index f40524875..9b4dc07d8 100644
--- a/attr.h
+++ b/attr.h
@@ -8,7 +8,7 @@ struct git_attr;
  * Given a string, return the gitattribute object that
  * corresponds to it.
  */
-struct git_attr *git_attr(const char *);
+const struct git_attr *git_attr(const char *);
 
 /* Internal use */
 extern const char git_attr__true[];
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 3d4704be5..cc6caf7ac 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -166,7 +166,8 @@ int cmd_check_attr(int argc, const char **argv, const char 
*prefix)
check = attr_check_alloc();
if (!all_attrs) {
for (i = 0; i < cnt; i++) {
-   struct git_attr *a = git_attr(argv[i]);
+   const struct git_attr *a = git_attr(argv[i]);
+
if (!a)
return error("%s: not a valid attribute name",
 argv[i]);
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH 27/27] attr: reformat git_attr_set_direction() function

[Brandon Williams]

Move the 'git_attr_set_direction()' up to be closer to the variables
that it modifies as well as a small formatting by renaming the variable
'new' to 'new_direction' so that it is more descriptive.

Update the comment about how 'direction' is used to read the state of
the world.  It should be noted that callers of
'git_attr_set_direction()' should ensure that other threads are not
making calls into the attribute system until after the call to
'git_attr_set_direction()' completes.  This function essentially acts as
reset button for the attribute system and should be handled with care.

Signed-off-by: Brandon Williams 
---
 attr.c | 49 -
 attr.h |  3 ++-
 2 files changed, 22 insertions(+), 30 deletions(-)

diff --git a/attr.c b/attr.c
index cbb07d25d..f5cc68b67 100644
--- a/attr.c
+++ b/attr.c
@@ -521,26 +521,30 @@ static struct attr_stack *read_attr_from_array(const char 
**list)
 }
 
 /*
- * 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.
+ * Callers into the attribute system assume there is a single, system-wide
+ * global state where attributes are read from and when the state is flipped by
+ * calling git_attr_set_direction(), the stack frames that have been
+ * constructed need to be discarded so so that subsequent calls into the
+ * attribute system will lazily read from the right place.  Since changing
+ * direction causes a global paradigm shift, it should not ever be called while
+ * another thread could potentially be calling into the attribute system.
  */
 static enum git_attr_direction direction;
 static struct index_state *use_index;
 
+void git_attr_set_direction(enum git_attr_direction new_direction,
+   struct index_state *istate)
+{
+   if (is_bare_repository() && new_direction != GIT_ATTR_INDEX)
+   die("BUG: non-INDEX attr direction in a bare repo");
+
+   if (new_direction != direction)
+   drop_attr_stack();
+
+   direction = new_direction;
+   use_index = istate;
+}
+
 static struct attr_stack *read_attr_from_file(const char *path, int macro_ok)
 {
FILE *fp = fopen(path, "r");
@@ -1130,19 +1134,6 @@ void attr_check_free(struct attr_check *check)
free(check);
 }
 
-void git_attr_set_direction(enum git_attr_direction new, struct index_state 
*istate)
-{
-   enum git_attr_direction old = direction;
-
-   if (is_bare_repository() && new != GIT_ATTR_INDEX)
-   die("BUG: non-INDEX attr direction in a bare repo");
-
-   direction = new;
-   if (new != old)
-   drop_attr_stack();
-   use_index = istate;
-}
-
 void attr_start(void)
 {
pthread_mutex_init(_attr_hashmap.mutex, NULL);
diff --git a/attr.h b/attr.h
index 9b4dc07d8..b8be37c91 100644
--- a/attr.h
+++ b/attr.h
@@ -73,7 +73,8 @@ enum git_attr_direction {
GIT_ATTR_CHECKOUT,
GIT_ATTR_INDEX
 };
-void git_attr_set_direction(enum git_attr_direction, struct index_state *);
+void git_attr_set_direction(enum git_attr_direction new_direction,
+   struct index_state *istate);
 
 extern void attr_start(void);
 
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH 25/27] attr: store attribute stacks in hashmap

[Brandon Williams]

The last big hurdle towards a thread-safe API for the attribute system
is the reliance on a global attribute stack that is modified during each
call into the attribute system.

This patch removes this global stack and instead a stack is retrieved or
constructed locally.  Since each of these stacks is only used as a
read-only structure once constructed, they can be stored in a hashmap
and shared between threads.  The key into the hashmap of attribute
stacks is, in the general case, the directory that corresponds to the
attribute stack frame.  For the core stack frames (builtin, system,
home, and info) a key of ".git/-attr" is used to prevent potential
collisions since a directory or file named ".git" is disallowed.

One caveat with storing and sharing the stack frames like this is that
the info stack needs to be treated separately from the rest of the
attribute stack.  This is because each stack frame holds a pointer to
the stack that comes before it and if it was placed on top of the rest
of the attribute stack then this pointer would be different for each
attribute stack and wouldn't be able to be shared between threads.  In
order to allow for sharing the info stack frame it needs to be its own
isolated frame and can simply be processed first to have the same affect
of being at the top of the stack.

Signed-off-by: Brandon Williams 
---
 attr.c | 375 +
 1 file changed, 235 insertions(+), 140 deletions(-)

diff --git a/attr.c b/attr.c
index 90f576044..78562592b 100644
--- a/attr.c
+++ b/attr.c
@@ -434,17 +434,19 @@ 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;
+struct attr_stack {
+   const struct attr_stack *prev;
char *origin;
size_t originlen;
unsigned num_matches;
unsigned alloc;
struct match_attr **attrs;
-} *attr_stack;
+};
+
+/* Dictionary of stack frames; access should be surrounded by mutex */
+static struct attr_hashmap g_stack_hashmap;
 
-static void free_attr_elem(struct attr_stack *e)
+static void attr_stack_free(struct attr_stack *e)
 {
int i;
free(e->origin);
@@ -467,6 +469,25 @@ static void free_attr_elem(struct attr_stack *e)
free(e);
 }
 
+static void drop_attr_stack(void)
+{
+   struct hashmap_iter iter;
+   struct attr_hash_entry *e;
+
+   hashmap_lock(_stack_hashmap);
+
+   hashmap_iter_init(_stack_hashmap.map, );
+   while ((e = hashmap_iter_next())) {
+   struct attr_stack *stack = e->value;
+   attr_stack_free(stack);
+   free(e);
+   }
+
+   hashmap_free(_stack_hashmap.map, 0);
+
+   hashmap_unlock(_stack_hashmap);
+}
+
 static const char *builtin_attr[] = {
"[attr]binary -diff -merge -text",
NULL,
@@ -621,15 +642,6 @@ static void debug_set(const char *what, const char *match, 
struct git_attr *attr
 #define debug_set(a,b,c,d) do { ; } while (0)
 #endif /* DEBUG_ATTR */
 
-static void drop_attr_stack(void)
-{
-   while (attr_stack) {
-   struct attr_stack *elem = attr_stack;
-   attr_stack = elem->prev;
-   free_attr_elem(elem);
-   }
-}
-
 static const char *git_etc_gitattributes(void)
 {
static const char *system_wide;
@@ -638,6 +650,14 @@ static const char *git_etc_gitattributes(void)
return system_wide;
 }
 
+static const char *get_home_gitattributes(void)
+{
+   if (!git_attributes_file)
+   git_attributes_file = xdg_config_home("attributes");
+
+   return git_attributes_file;
+}
+
 static int git_attr_system(void)
 {
return !git_env_bool("GIT_ATTR_NOSYSTEM", 0);
@@ -645,142 +665,208 @@ 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)
+/*
+ * This funciton should only be called from 'get_attr_stack()' or
+ * 'get_info_stack()', which already needs to acquire the lock to the stack
+ * hashmap, so there is no need to also acquire the lock in this function.
+ */
+static void push_stack(const struct attr_stack **attr_stack_p,
+  struct attr_stack *elem,
+  const char *origin, size_t originlen)
 {
if (elem) {
-   elem->origin = origin;
-   if (origin)
-   elem->originlen = originlen;
+   elem->origin = xmemdupz(origin, originlen);
+   elem->originlen = originlen;
elem->prev = *attr_stack_p;
*attr_stack_p = elem;
+   attr_hashmap_add(_stack_hashmap, elem->origin,
+

[PATCH 20/27] attr: change validity check for attribute names to use positive logic

[Brandon Williams]

From: Junio C Hamano 

Convert 'invalid_attr_name()' to 'attr_name_valid()' and use positive
logic for the return value.  In addition create a helper function that
prints out an error message when an invalid attribute name is used.

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: Brandon Williams 
---
 attr.c | 34 --
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/attr.c b/attr.c
index e58fa340c..5399e1cb3 100644
--- a/attr.c
+++ b/attr.c
@@ -74,23 +74,33 @@ static unsigned hash_name(const char *name, int namelen)
return val;
 }
 
-static int invalid_attr_name(const char *name, int namelen)
+static 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;
+}
+
+static void report_invalid_attr(const char *name, size_t len,
+   const char *src, int lineno)
+{
+   struct strbuf err = STRBUF_INIT;
+   strbuf_addf(, _("%.*s is not a valid attribute name"),
+   (int) len, name);
+   fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno);
+   strbuf_release();
 }
 
 static struct git_attr *git_attr_internal(const char *name, int len)
@@ -105,7 +115,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);
@@ -196,17 +206,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 == '!') {
@@ -258,10 +266,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;
}
}
-- 
2.11.0.390.gc69c2f50cf-goog

[PATCH 16/27] attr: convert git_all_attrs() to use "struct attr_check"

[Brandon Williams]

From: Junio C Hamano 

This updates the other two ways the attribute check is done via an
array of "struct attr_check_item" 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 attr_check_initl() to prepare the
   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 attr_check_alloc() to allocate an empty
attr_check, and then call attr_check_append() to add attribute names
one by one.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Brandon Williams 
---
 attr.c   | 38 -
 attr.h   |  9 +++-
 builtin/check-attr.c | 60 ++--
 3 files changed, 47 insertions(+), 60 deletions(-)

diff --git a/attr.c b/attr.c
index be9e398e9..d2eaa0410 100644
--- a/attr.c
+++ b/attr.c
@@ -837,42 +837,32 @@ int git_check_attrs(const char *path, int num, struct 
attr_check_item *check)
return 0;
 }
 
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check)
+void git_all_attrs(const char *path, struct attr_check *check)
 {
-   int i, count, j;
+   int i;
 
-   collect_some_attrs(path, 0, NULL);
+   attr_check_reset(check);
+   collect_some_attrs(path, check->check_nr, check->check);
 
-   /* 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;
-   }
+   struct attr_check_item *item;
+   if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+   continue;
+   item = attr_check_append(check, git_attr(name));
+   item->value = value;
}
-
-   return 0;
 }
 
-struct attr_check *attr_check_alloc(void)
+int git_check_attr(const char *path, struct attr_check *check)
 {
-   return xcalloc(1, sizeof(struct attr_check));
+   return git_check_attrs(path, check->check_nr, check->check);
 }
 
-int git_check_attr(const char *path, struct attr_check *check)
+struct attr_check *attr_check_alloc(void)
 {
-   return git_check_attrs(path, check->check_nr, check->check);
+   return xcalloc(1, sizeof(struct attr_check));
 }
 
 struct attr_check *attr_check_initl(const char *one, ...)
diff --git a/attr.h b/attr.h
index 459347f4b..971bb9a38 100644
--- a/attr.h
+++ b/attr.h
@@ -56,13 +56,10 @@ int git_check_attrs(const char *path, int, struct 
attr_check_item *);
 extern int git_check_attr(const char *path, struct attr_check *check);
 
 /*
- * Retrieve all attributes that apply to the specified path.  *num
- * will be set to the number of attributes on the path; **check will
- * be set to point at a newly-allocated array of git_attr_check
- * objects describing the attributes and their values.  *check must be
- * free()ed by the caller.
+ * Retrieve all attributes that apply to the specified path.
+ * check holds the attributes and their values.
  */
-int git_all_attrs(const char *path, int *num, struct attr_check_item **check);
+void git_all_attrs(const char *path, struct attr_check *check);
 
 enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 889264a5b..3d4704be5 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,12 +24,13 @@ static const struct option check_attr_options[] = {
OPT_END()
 };
 
-static void output_attr(int cnt, struct attr_check_item *check,
-   const char *file)
+static void output_attr(struct attr_check *check, const char *file)
 {
int j;
+   int cnt = check->check_nr;
+
for (j = 0; j < cnt; j++) {
-   const char *value = check[j].value;
+   const char *value = check->check[j].value;
 
if (ATTR_TRUE(value))
value = "set";
@@ -42,36 +43,38 @@ static void output_attr(int cnt, struct attr_check_item 
*check,
printf("%s%c" /* path */
   "%s%c" /* attrname */
   "%s%c" /* attrvalue */,
-  

[PATCH 23/27] attr: remove maybe-real, maybe-macro from git_attr

[Brandon Williams]

Whether or not a git attribute is real or a macro isn't a property of
the attribute but rather it depends on the attribute stack (which
.gitattribute files were read).

This patch removes the 'maybe_real' and 'maybe_macro' fields in a
git_attr and instead adds the 'macro' field to a attr_check_item.  The
'macro' indicates (if non-NULL) that a particular attribute is a macro
for the given attribute stack.  It's populated, through a quick scan of
the attribute stack, with the match_attr that corresponds to the macro's
definition.  This way the attribute stack only needs to be scanned a
single time prior to attribute collection instead of each time a macro
needs to be expanded.

Signed-off-by: Brandon Williams 
---
 attr.c | 69 ++
 attr.h |  6 ++
 2 files changed, 37 insertions(+), 38 deletions(-)

diff --git a/attr.c b/attr.c
index 38b0d4347..633a12cc3 100644
--- a/attr.c
+++ b/attr.c
@@ -30,20 +30,9 @@ static const char git_attr__unknown[] = "(builtin)unknown";
 
 struct git_attr {
int attr_nr; /* unique attribute number */
-   int maybe_macro;
-   int maybe_real;
char name[FLEX_ARRAY]; /* attribute name */
 };
 
-/*
- * 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;
-
 const char *git_attr_name(const struct git_attr *attr)
 {
return attr->name;
@@ -182,6 +171,7 @@ static void all_attrs_init(struct attr_hashmap *map, struct 
attr_check *check)
 */
for (i = 0; i < check->all_attrs_nr; i++) {
check->all_attrs[i].value = ATTR__UNKNOWN;
+   check->all_attrs[i].macro = NULL;
}
 }
 
@@ -233,8 +223,6 @@ static struct git_attr *git_attr_internal(const char *name, 
int namelen)
if (!a) {
FLEX_ALLOC_MEM(a, name, name, namelen);
a->attr_nr = g_attr_hashmap.map.size;
-   a->maybe_real = 0;
-   a->maybe_macro = 0;
 
attr_hashmap_add(_attr_hashmap, a->name, namelen, a);
assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
@@ -397,7 +385,6 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
  (is_macro ? 0 : namelen + 1));
if (is_macro) {
res->u.attr = git_attr_internal(name, namelen);
-   res->u.attr->maybe_macro = 1;
} else {
char *p = (char *)&(res->state[num_attr]);
memcpy(p, name, namelen);
@@ -418,10 +405,6 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
/* Second pass to fill the attr_states */
for (cp = states, i = 0; *cp; i++) {
cp = parse_attr(src, lineno, cp, &(res->state[i]));
-   if (!is_macro)
-   res->state[i].attr->maybe_real = 1;
-   if (res->state[i].attr->maybe_macro)
-   cannot_trust_maybe_real = 1;
}
 
strbuf_release();
@@ -826,7 +809,7 @@ static int path_matches(const char *pathname, int pathlen,
 static int macroexpand_one(struct attr_check_item *all_attrs, int nr, int rem);
 
 static int fill_one(const char *what, struct attr_check_item *all_attrs,
-   struct match_attr *a, int rem)
+   const struct match_attr *a, int rem)
 {
int i;
 
@@ -867,24 +850,34 @@ static int fill(const char *path, int pathlen, int 
basename_offset,
 
 static int macroexpand_one(struct attr_check_item *all_attrs, int nr, int rem)
 {
-   struct attr_stack *stk;
-   int i;
+   const struct attr_check_item *item = _attrs[nr];
 
-   if (all_attrs[nr].value != ATTR__TRUE ||
-   !all_attrs[nr].attr->maybe_macro)
+   if (item->macro && item->value == ATTR__TRUE)
+   return fill_one("expand", all_attrs, item->macro, rem);
+   else
return rem;
+}
 
-   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)
-   return fill_one("expand", all_attrs, ma, rem);
+/*
+ * Marks the attributes which are macros based on the attribute stack.
+ * This prevents having to search through the attribute stack each time
+ * a macro needs to be expanded during the fill stage.
+ */
+static void determine_macros(struct attr_check_item *all_attrs,
+const struct attr_stack *stack)
+{
+   for (; stack; stack = stack->prev) {
+  

[PATCH 18/27] attr: retire git_check_attrs() API

[Brandon Williams]

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: Brandon Williams 
---
 Documentation/technical/api-gitattributes.txt | 86 +--
 attr.c|  3 +-
 attr.h|  1 -
 3 files changed, 58 insertions(+), 32 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 260266867..82f5130e7 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`::
+`struct attr_check_item`::
 
-   This structure represents a set of attributes to check in a call
-   to `git_check_attr()` function, and receives the results.
+   This structure represents one attribute and its value.
+
+`struct attr_check`::
+
+   This structure represents a collection of `attr_check_item`.
+   It is passed to `git_check_attr()` function, specifying the
+   attributes to check, and receives their values.
 
 
 Attribute Values
@@ -27,7 +32,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:
+attr_check_item` records it.  There are three macros to check these:
 
 `ATTR_TRUE()`::
 
@@ -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 attr_check` using attr_check_initl()
+  function, enumerating the names of attributes whose values you are
+  interested in, terminated with a NULL pointer.  Alternatively, an
+  empty `struct attr_check` can be prepared by calling
+  `attr_check_alloc()` function and then attributes you want to
+  ask about can be added to it with `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 `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 attr_check` with two elements (because
+  we are checking two attributes):
 
 
-static struct git_attr_check check[2];
+static struct 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 = 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 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 attr_check *check;
+static void setup_check(const char **argv)
+{
+   check = attr_check_alloc();
+   while (*argv) {
+   struct git_attr *attr = git_attr(*argv);
+   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 

[PATCH 19/27] attr: pass struct attr_check to collect_some_attrs

[Brandon Williams]

The old callchain used to take an array of attr_check_item items.
Instead pass the 'attr_check' container object to 'collect_some_attrs()'
and access the fields in the data structure directly.

Signed-off-by: Brandon Williams 
---
 attr.c | 33 +
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/attr.c b/attr.c
index da727e3fd..e58fa340c 100644
--- a/attr.c
+++ b/attr.c
@@ -777,9 +777,7 @@ static int macroexpand_one(int nr, int rem)
  * 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,
-  struct attr_check_item *check)
-
+static void collect_some_attrs(const char *path, struct attr_check *check)
 {
struct attr_stack *stk;
int i, pathlen, rem, dirlen;
@@ -802,17 +800,18 @@ static void collect_some_attrs(const char *path, int num,
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->check_nr && !cannot_trust_maybe_real) {
rem = 0;
-   for (i = 0; i < num; i++) {
-   if (!check[i].attr->maybe_real) {
+   for (i = 0; i < check->check_nr; i++) {
+   const struct git_attr *a = check->check[i].attr;
+   if (!a->maybe_real) {
struct attr_check_item *c;
-   c = check_all_attr + check[i].attr->attr_nr;
+   c = check_all_attr + a->attr_nr;
c->value = ATTR__UNSET;
rem++;
}
}
-   if (rem == num)
+   if (rem == check->check_nr)
return;
}
 
@@ -821,18 +820,17 @@ 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,
-  struct attr_check_item *check)
+int git_check_attr(const char *path, struct attr_check *check)
 {
int i;
 
-   collect_some_attrs(path, num, check);
+   collect_some_attrs(path, 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[check->check[i].attr->attr_nr].value;
if (value == ATTR__UNKNOWN)
value = ATTR__UNSET;
-   check[i].value = value;
+   check->check[i].value = value;
}
 
return 0;
@@ -843,7 +841,7 @@ void git_all_attrs(const char *path, struct attr_check 
*check)
int i;
 
attr_check_reset(check);
-   collect_some_attrs(path, check->check_nr, check->check);
+   collect_some_attrs(path, check);
 
for (i = 0; i < attr_nr; i++) {
const char *name = check_all_attr[i].attr->name;
@@ -856,11 +854,6 @@ void git_all_attrs(const char *path, struct attr_check 
*check)
}
 }
 
-int git_check_attr(const char *path, struct attr_check *check)
-{
-   return git_check_attrs(path, check->check_nr, check->check);
-}
-
 struct attr_check *attr_check_alloc(void)
 {
return xcalloc(1, sizeof(struct attr_check));
-- 
2.11.0.390.gc69c2f50cf-goog

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

[Brandon Williams]

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 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: Brandon Williams 
---
 archive.c  | 24 ++--
 builtin/pack-objects.c | 19 +--
 convert.c  | 17 ++---
 ll-merge.c | 33 ++---
 userdiff.c | 19 ---
 ws.c   | 19 ++-
 6 files changed, 45 insertions(+), 86 deletions(-)

diff --git a/archive.c b/archive.c
index b76bd4691..3591f7d55 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 attr_check_item *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;
@@ -120,10 +107,10 @@ static int write_archive_entry(const unsigned char *sha1, 
const char *base,
void *context)
 {
static struct strbuf path = STRBUF_INIT;
+   static struct attr_check *check;
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
-   struct attr_check_item check[2];
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 = 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/builtin/pack-objects.c b/builtin/pack-objects.c
index 8b8fbd814..ff8b3c12d 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -894,24 +894,15 @@ static void write_pack_file(void)
written, nr_result);
 }
 
-static void setup_delta_attr_check(struct attr_check_item *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 attr_check_item check[1];
+   static struct attr_check *check;
 
-   setup_delta_attr_check(check);
-   if (git_check_attrs(path, ARRAY_SIZE(check), check))
+   if (!check)
+   check = 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 1b9829279..affd8ce9b 100644
--- a/convert.c
+++ b/convert.c
@@ -1085,24 +1085,19 @@ 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 attr_check_item ccheck[NUM_CONV_ATTRS];
+   static struct 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 = 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 attr_check_item *ccheck = check->check;
   

[PATCH 15/27] attr: (re)introduce git_check_attr() and struct attr_check

[Brandon Williams]

From: Junio C Hamano 

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

 (1) prepare an array A of N attr_check_item 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 attr_check" that contains N, the number of
attributes being sought, and A, the array that holds N
attr_check_item 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.

Signed-off-by: Junio C Hamano 
Signed-off-by: Stefan Beller 
Signed-off-by: Brandon Williams 
---
 attr.c | 74 ++
 attr.h | 17 +++
 2 files changed, 91 insertions(+)

diff --git a/attr.c b/attr.c
index 2f180d609..be9e398e9 100644
--- a/attr.c
+++ b/attr.c
@@ -865,6 +865,80 @@ int git_all_attrs(const char *path, int *num, struct 
attr_check_item **check)
return 0;
 }
 
+struct attr_check *attr_check_alloc(void)
+{
+   return xcalloc(1, sizeof(struct attr_check));
+}
+
+int git_check_attr(const char *path, struct attr_check *check)
+{
+   return git_check_attrs(path, check->check_nr, check->check);
+}
+
+struct attr_check *attr_check_initl(const char *one, ...)
+{
+   struct 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 = attr_check_alloc();
+   check->check_nr = cnt;
+   check->check_alloc = cnt;
+   check->check = xcalloc(cnt, sizeof(struct attr_check_item));
+
+   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);
+   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 attr_check_item *attr_check_append(struct attr_check *check,
+ const struct git_attr *attr)
+{
+   struct attr_check_item *item;
+
+   ALLOC_GROW(check->check, check->check_nr + 1, check->check_alloc);
+   item = >check[check->check_nr++];
+   item->attr = attr;
+   return item;
+}
+
+void attr_check_reset(struct attr_check *check)
+{
+   check->check_nr = 0;
+}
+
+void attr_check_clear(struct attr_check *check)
+{
+   free(check->check);
+   check->check = NULL;
+   check->check_alloc = 0;
+   check->check_nr = 0;
+}
+
+void attr_check_free(struct attr_check *check)
+{
+   attr_check_clear(check);
+   free(check);
+}
+
 void git_attr_set_direction(enum git_attr_direction new, struct index_state 
*istate)
 {
enum git_attr_direction old = direction;
diff --git a/attr.h b/attr.h
index efc7bb3b3..459347f4b 100644
--- a/attr.h
+++ b/attr.h
@@ -29,6 +29,22 @@ struct attr_check_item {
const char *value;
 };
 
+struct attr_check {
+   int check_nr;
+   int check_alloc;
+   struct attr_check_item *check;
+};
+
+extern struct attr_check *attr_check_alloc(void);
+extern struct attr_check *attr_check_initl(const char *, ...);
+
+extern struct attr_check_item 

Re: Bug report: Documentation error in git-bisect man description

[Junio C Hamano]

Junio C Hamano  writes:

> Manuel Ullmann  writes:
>
> Hmmm, I tend to agree, modulo a minor fix.
>
> If the description were in a context inside a paragraph like this:
>
>   When you want to tell 'git bisect' that a  belongs to
>   the newer half of the history, you say
>
>   git bisect (bad|new) []
>
>   On the other hand, when you want to tell 'git bisect' that a
>belongs to the older half of the history, you can say
>
>   git bisect (good|old) []
>
> then the pairing we see in the current text makes quite a lot of
> sense.

Actually, the above is _exactly_ what was intended.  I misread the
current documentation when I made the comment, and I think that the
current one _IS_ correct.  The latter half of the above is not about
a single rev.  You can paint multiple commits with the "older half"
color, i.e.

On the other hand, when you want to tell 'git bisect' that
one or more s  belong to the older half of the history,
you can say

git bisect (good|old) [...]

In contrast, you can mark only one  as newer (or "already
bad").  So pairing (bad|good) and (new|old) like you suggested
breaks the correctness of the command line description.

If (bad|new) and (good|old) bothers you because they may mislead the
readers to think bad is an opposite of new (and good is an opposite
of old), the only solution I can think of to that problem is to
expand these two lines into four and list them like this:

git bisect bad []
git bisect good [...]
git bisect new []
git bisect old [...]

Re: Bug report: Documentation error in git-bisect man description

[Junio C Hamano]

Manuel Ullmann  writes:

> Hi,
>
> there is a mistake in the git-bisect description.
> The second paragraph of it says ‘the terms "old" and "new" can be used
> in place of "good" and "bad"’. So from a logical point of view the
> description part stating the usage syntax should be:
>
> git bisect (bad|good) []
> git bisect (old|new) [...]
>
> instead of
>
> git bisect (bad|new) []
> git bisect (good|old) [...]
>
> Checked man page version of 2.11.0, but it is in my local 2.10.2 git as well.

Hmmm, I tend to agree, modulo a minor fix.

If the description were in a context inside a paragraph like this:

When you want to tell 'git bisect' that a  belongs to
the newer half of the history, you say

git bisect (bad|new) []

On the other hand, when you want to tell 'git bisect' that a
 belongs to the older half of the history, you can say

git bisect (good|old) []

then the pairing we see in the current text makes quite a lot of
sense.

But in the early part of the description section, listing the
information that logically belongs to the synopsis section, I think
the current one is misleading.  You are painting commits with two
colors, and if you are from the "older vs newer" school, you say
either 'old' or 'new' as the names of these two colors, and do not
use 'bad' or 'good'.  A line with "git bisect (old|new) []" in
the list would make more sense.

Similarly, if you are from the "still good vs already bad" school,
you would either say 'good' or 'bad' so you would want to see a line
with "git bisect (good|bad) []" in the list (not "bad|good" in
that order, but opposite).

Christian, am I talking nonsense?

[PATCH] worktree: fix an 'using plain integer as NULL pointer' warning

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Duy,

If you need to re-roll your 'nd/worktree-move' branch, could you
please squash this into the relevant patch. (commit 62985f75c1
"worktree move: refuse to move worktrees with submodules", 08-01-2017).

[BTW, this is a sparse warning, just in case you were wondering!]

Thanks!

ATB,
Ramsay Jones

 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index 339c622e2..a1c91f154 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -528,7 +528,7 @@ static int unlock_worktree(int ac, const char **av, const 
char *prefix)
 
 static void validate_no_submodules(const struct worktree *wt)
 {
-   struct index_state istate = {0};
+   struct index_state istate = { NULL };
int i, found_submodules = 0;
 
if (read_index_from(, worktree_git_path(wt, "index")) > 0) {
-- 
2.11.0

Bug report: Documentation error in git-bisect man description

[Manuel Ullmann]

Hi,

there is a mistake in the git-bisect description.
The second paragraph of it says ‘the terms "old" and "new" can be used
in place of "good" and "bad"’. So from a logical point of view the
description part stating the usage syntax should be:

git bisect (bad|good) []
git bisect (old|new) [...]

instead of

git bisect (bad|new) []
git bisect (good|old) [...]

Checked man page version of 2.11.0, but it is in my local 2.10.2 git as well.

Best regards,
Manuel

Re: [PATCH/RFC 5/4] Redefine core.bare in multiple working tree setting

[Junio C Hamano]

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

> With per-worktree configuration in place, core.bare is moved to main
> worktree's private config file. But it does not really make sense
> because this is about _repository_. Instead we could leave core.bare in
> the per-repo config and change/extend its definition from:
>
>If true this repository is assumed to be 'bare' and has no working
>directory associated with it.
>
> to
>
>If true this repository is assumed to be 'bare' and has no _main_
>working directory associated with it.
>
> In other words, linked worktrees are not covered by core.bare. This
> definition is the same as before when it comes to single worktree setup.

Up to this point, I think it is not _wrong_ per-se, but it does not
say anything about secondary worktrees.  Some may have their own
working tree, others may be bare, and there is no way for programs
to discover if a particular secondary worktree has or lacks its own
working tree.

Granted, "git worktree" porcelain may be incapable of creating a
secondary worktree without a working tree, but I think the
underlying repository layout still is capable of expressing such a
secondary worktree.

So there still is something else necessary, I suspect, to make the
definition complete.  Perhaps core.bare should be set in
per-worktree configuration for all worktrees including the primary
one, and made the definition/explanation of core.bare to be
"definition of this variable, if done, must be done in per-worktree
config file.  If set to true, the worktree is 'bare' and has no
working directory associated with it"?  That makes things even more
equal, as there is truly no "special one" at that point.

I dunno.

Re: [PATCH 1/3] doc: gitk: remove gitview reference

[Stefan Beller]

On Thu, Jan 12, 2017 at 1:32 PM, Philip Oakley  wrote:
> contrib/gitview has been removed. Remove the reference.

Thanks for this cleanup.

Re: [PATCHv2 4/4] unpack-trees: support super-prefix option

[Stefan Beller]

On Thu, Jan 12, 2017 at 1:40 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> This is only patchv4 that is rerolled, patches 1-3 remain as is.
>
> Good timing, as I was about to send a reminder to suggest rerolling
> 4/4 alone ;-)
>
>> +static const char *super_prefixed(const char *path)
>> +{
>
> There used to be a comment that explains why we keep two static
> buffers around here.  Even though it is in the log message, the
> in-code comment would save people trouble of having to go through
> "git blame" output.
>
> I'd say something like
>
> /*
>  * It is necessary and sufficient to have two static buffers
>  * as the return value of this function is fed to error()
>  * using the unpack_*_errors[] templates we can see above.
>  */
>
> perhaps.

If you think it helps, I can reroll with such a comment.
At the time of fixing up for v4 I felt like it is overly verbose.
Such a comment is only useful in understanding the choice of 2.
I thought it was sufficient in the log as once you're interested in
that function you'd read the output of blame anyway.

On the other hand having statically allocated arrays of fixed size is
dangerous in terms of maintainability, i.e. down the road someone
thinks this is a good function to reuse and then they may run into
subtle bugs as they will not be aware of the internally static buffer
that is overwritten after a certain time.

>
>> + static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
>> + static int super_prefix_len = -1;
>> + static unsigned idx = 0;
>> +
>
> If we initialize this to 1 (or even better, "ARRAY_SIZE(buf) - 1"),
> then we would use buf[0] first and then buf[1], which feels more
> natural to me.

Yes I agree, though I overcomplicating things just so that they feel
more natural seems wrong as well.

At the time I assumed that having a static variable initialized to 0
was slightly cheaper, as the init code just memsets all of .bss to 0
unlike the .data segment that has to be crafted manually.
But to get that variable into the .bss section reliably we'd have
to omit the "=0". (My compiler recognized that it can be put into
.bss because it is 0)

>
> Other than that, this looks OK.  Will queue.
>
> Thanks.

Thanks,
Stefan

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Jake,
>
> On Wed, 11 Jan 2017, Jacob Keller wrote:
>
>> From: Jacob Keller 
>> 
>> Teach git-describe the `--discard` option which will allow specifying
>> a glob pattern of tags to ignore.
>
> IMHO "discard" is the wrong word, it almost sounds as if the matching tags
> would be *deleted*.
>
> Maybe `--exclude` or `--unmatch` instead?

Yeah, as j6t mentions, I think --exclude would be a good choice.

Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path

[Junio C Hamano]

Pat Pannuto  writes:

> You may still want the 1/2 patch in this series, just to make things
> internally consistent with "-w" vs "use warnings;" inside git's perl
> scripts.

I do not think so.  1/2 is justified as a prerequisite of 2/2 and
needs a different log message, so cannot be used as is.

I vaguely recall hearing arguments for and against the choice
between "#!path-to-perl -w" vs "use warnings;" but do not recall the
details to have a strong opinion either way, so we might even end up
wanting to be "internally consistent" by going the other way.  Please
prepare a standalone patch with an update message to convince people
why "use warnings;" is more preferable than "#!path-to-perl -w" and
explaining that we are making things consistently use the more
preferable form, if you want to go in that direction.

Thanks.

Re: [PATCHv2 4/4] unpack-trees: support super-prefix option

[Junio C Hamano]

Stefan Beller  writes:

> This is only patchv4 that is rerolled, patches 1-3 remain as is.

Good timing, as I was about to send a reminder to suggest rerolling
4/4 alone ;-)

> +static const char *super_prefixed(const char *path)
> +{

There used to be a comment that explains why we keep two static
buffers around here.  Even though it is in the log message, the
in-code comment would save people trouble of having to go through
"git blame" output.

I'd say something like

/*
 * It is necessary and sufficient to have two static buffers
 * as the return value of this function is fed to error()
 * using the unpack_*_errors[] templates we can see above.
 */

perhaps.

> + static struct strbuf buf[2] = {STRBUF_INIT, STRBUF_INIT};
> + static int super_prefix_len = -1;
> + static unsigned idx = 0;
> +

If we initialize this to 1 (or even better, "ARRAY_SIZE(buf) - 1"),
then we would use buf[0] first and then buf[1], which feels more
natural to me.

Other than that, this looks OK.  Will queue.

Thanks.

> + if (super_prefix_len < 0) {
> + if (!get_super_prefix())
> + super_prefix_len = 0;
> + else {
> + int i;
> +
> + super_prefix_len = strlen(get_super_prefix());
> + for (i = 0; i < ARRAY_SIZE(buf); i++)
> + strbuf_addstr([i], get_super_prefix());
> + }
> + }
> +
> + if (!super_prefix_len)
> + return path;
> +
> + if (++idx >= ARRAY_SIZE(buf))
> + idx = 0;
> +
> + strbuf_setlen([idx], super_prefix_len);
> + strbuf_addstr([idx], path);
> +
> + return buf[idx].buf;
> +}
> +
>  void setup_unpack_trees_porcelain(struct unpack_trees_options *opts,
> const char *cmd)
>  {
> @@ -172,7 +202,7 @@ static int add_rejected_path(struct unpack_trees_options 
> *o,
>const char *path)
>  {
>   if (!o->show_all_errors)
> - return error(ERRORMSG(o, e), path);
> + return error(ERRORMSG(o, e), super_prefixed(path));
>  
>   /*
>* Otherwise, insert in a list for future display by
> @@ -196,7 +226,7 @@ static void display_error_msgs(struct 
> unpack_trees_options *o)
>   something_displayed = 1;
>   for (i = 0; i < rejects->nr; i++)
>   strbuf_addf(, "\t%s\n", 
> rejects->items[i].string);
> - error(ERRORMSG(o, e), path.buf);
> + error(ERRORMSG(o, e), super_prefixed(path.buf));
>   strbuf_release();
>   }
>   string_list_clear(rejects, 0);
> @@ -1918,7 +1948,9 @@ int bind_merge(const struct cache_entry * const *src,
>o->merge_size);
>   if (a && old)
>   return o->gently ? -1 :
> - error(ERRORMSG(o, ERROR_BIND_OVERLAP), a->name, 
> old->name);
> + error(ERRORMSG(o, ERROR_BIND_OVERLAP),
> +   super_prefixed(a->name),
> +   super_prefixed(old->name));
>   if (!a)
>   return keep_entry(old, o);
>   else

[PATCH 1/3] doc: gitk: remove gitview reference

[Philip Oakley]

contrib/gitview has been removed. Remove the reference.

Signed-off-by: Philip Oakley 
---
cc: Paul Mackerras 
---
 Documentation/gitk.txt | 4 
 1 file changed, 4 deletions(-)

diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index e382dd9..73c02b9 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -187,10 +187,6 @@ SEE ALSO
 'qgit(1)'::
A repository browser written in C++ using Qt.
 
-'gitview(1)'::
-   A repository browser written in Python using Gtk. It's based on
-   'bzrk(1)' and distributed in the contrib area of the Git repository.
-
 'tig(1)'::
A minimal repository browser and Git tool output highlighter written
in C using Ncurses.
-- 
2.9.0.windows.1.323.g0305acf

[PATCH 3/3] doc: git-gui browser does not default to HEAD

[Philip Oakley]

37cd4f7 ("Document git-gui, git-citool as mainporcelain manual pages",
2007-06-21) documented the default, but was shortly followed by c52c945
("git-gui: Allow blame/browser subcommands on bare repositories",
2007-07-17) which, it would appear, as a side effect, removed that default.

Finally document that change.

Signed-off-by: Philip Oakley 
---
cc: Shawn O. Pearce 
cc: Pat Thoyts 
---
 Documentation/git-gui.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git-gui.txt b/Documentation/git-gui.txt
index c1a3e8b..5f93f80 100644
--- a/Documentation/git-gui.txt
+++ b/Documentation/git-gui.txt
@@ -35,7 +35,7 @@ blame::
 
 browser::
Start a tree browser showing all files in the specified
-   commit (or `HEAD` by default).  Files selected through the
+   commit.  Files selected through the
browser are opened in the blame viewer.
 
 citool::
-- 
2.9.0.windows.1.323.g0305acf

[PATCH 2/3] doc: gitk: add the upstream repo location

[Philip Oakley]

Match the 'git gui' information regarding the graphical browser
and its upstream location.

Signed-off-by: Philip Oakley 
---
cc: Paul Mackerras 
---
 Documentation/gitk.txt | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/gitk.txt b/Documentation/gitk.txt
index 73c02b9..9244379 100644
--- a/Documentation/gitk.txt
+++ b/Documentation/gitk.txt
@@ -178,9 +178,15 @@ used by default. If '$XDG_CONFIG_HOME' is not set it 
defaults to
 History
 ---
 Gitk was the first graphical repository browser. It's written in
-tcl/tk and started off in a separate repository but was later merged
-into the main Git repository.
+tcl/tk.
 
+'gitk' is actually maintained as an independent project, but stable
+versions are distributed as part of the Git suite for the convenience
+of end users.
+
+gitk-git/ comes from Paul Mackerras's gitk project:
+
+git://ozlabs.org/~paulus/gitk
 
 SEE ALSO
 
-- 
2.9.0.windows.1.323.g0305acf

[PATCH 0/3] updates to gitk & git-gui doc now gitview has gone

[Philip Oakley]

gitview was removed recently.

> Sent: Tuesday, January 10, 2017 11:48 PM
> Subject: What's cooking in git.git (Jan 2017, #01; Tue, 10)

> * sb/remove-gitview (2017-01-07) 1 commit
>   (merged to 'next' on 2017-01-10 at dcb3abd146)
>  + contrib: remove gitview

> Will merge to 'master'.


Lets remove the reference in the gitk man page, and do some other
fixups while in the area.

Philip Oakley (3):
  doc: gitk: remove gitview reference
  doc: gitk: add the upstream repo location
  doc: git-gui browser does not default to HEAD

 Documentation/git-gui.txt |  2 +-
 Documentation/gitk.txt| 14 --
 2 files changed, 9 insertions(+), 7 deletions(-)

-- 
2.9.0.windows.1.323.g0305acf

Re: Bug report: Git pull hang occasionally

[Kai Zhang]

> On Jan 12, 2017, at 1:12 PM, Junio C Hamano  wrote:
> 
> Kai Zhang  writes:
> 
>>> On Dec 21, 2016, at 1:32 PM, Junio C Hamano  wrote:
>>> 
>>> Junio C Hamano  writes:
>>> ...
>>> 
>>> I wonder if the latter is solved by recent patch 296b847c0d
>>> ("remote-curl: don't hang when a server dies before any output",
>>> 2016-11-18) on the client side.
>>> ...
> 
>> After apply this patch, hanging did not happen again. 
> 
> Thanks for confirming.
> 
>> Would this patch go to release in near future?
> 
> I see 296b847c0d in the message you are responding to (by the way,
> do not top-post to this list).  
> 
> Let's check it together.
> 
>   $ git log master..296b847c0d
>   $ git merge-base master 296b847c0d
>296b847c0d6de63353e236cfbf94163d24155529
> 
> Yup, it already is in master.  
> 
> Using a third-party script "when-merged" [*1*], we can easily find
> that it was merged a few days ago to 'master', after cooking in
> 'next' for a handful of weeks:
> 
>   $ git when-merged 296b847c0d next
>   refs/heads/next 3ea70d01afc6305b88d33b8585f1fc41c486a182
>   $ git when-merged 296b847c0d master
>   refs/heads/master d984592043aec3c9f5b1955560a133896ca115b5
>   $ git show -s --format='%cI' 3ea70d01af d984592043 
>2016-12-05T11:38:03-08:00
>2017-01-10T15:24:25-08:00
> 
> Unless people find regressions caused by this change (in which case
> we may have to revert it), this will be included in the release at
> the end of this cycle.  http://tinyurl.com/gitCal tells us that the
> current cycle is expected to complete early February.
> 
> 
> [Footnote]
> 
> *1* git://github.com/mhagger/git-when-merged

Thank you for your help!

Re: Bug report: Git pull hang occasionally

[Junio C Hamano]

Kai Zhang  writes:

>> On Dec 21, 2016, at 1:32 PM, Junio C Hamano  wrote:
>> 
>> Junio C Hamano  writes:
>>  ...
>> 
>> I wonder if the latter is solved by recent patch 296b847c0d
>> ("remote-curl: don't hang when a server dies before any output",
>> 2016-11-18) on the client side.
>> ...

> After apply this patch, hanging did not happen again. 

Thanks for confirming.

> Would this patch go to release in near future?

I see 296b847c0d in the message you are responding to (by the way,
do not top-post to this list).  

Let's check it together.

$ git log master..296b847c0d
$ git merge-base master 296b847c0d
296b847c0d6de63353e236cfbf94163d24155529

Yup, it already is in master.  

Using a third-party script "when-merged" [*1*], we can easily find
that it was merged a few days ago to 'master', after cooking in
'next' for a handful of weeks:

$ git when-merged 296b847c0d next
refs/heads/next 3ea70d01afc6305b88d33b8585f1fc41c486a182
$ git when-merged 296b847c0d master
refs/heads/master d984592043aec3c9f5b1955560a133896ca115b5
$ git show -s --format='%cI' 3ea70d01af d984592043 
2016-12-05T11:38:03-08:00
2017-01-10T15:24:25-08:00

Unless people find regressions caused by this change (in which case
we may have to revert it), this will be included in the release at
the end of this cycle.  http://tinyurl.com/gitCal tells us that the
current cycle is expected to complete early February.


[Footnote]

*1* git://github.com/mhagger/git-when-merged

Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path

[Pat Pannuto]

I'm happy to let this drop.

I've filed the missing perl library as a homebrew issue [1], so
hopefully this won't be an issue for folks in the future.

You may still want the 1/2 patch in this series, just to make things
internally consistent with "-w" vs "use warnings;" inside git's perl
scripts.

-Pat

[1] https://github.com/Homebrew/homebrew-core/issues/8783

On Thu, Jan 12, 2017 at 3:40 PM, Junio C Hamano  wrote:
> Johannes Schindelin  writes:
>
>> So if this patch would make it into upstream Git, I would *have* to revert
>> it in Git for Windows, adding to my already considerable maintenance burden.
>
> I do not think we want "#!/usr/bin/env $language" in the upstream,
> either.
>

Re: [PATCH v2] diff: add interhunk context config option

[Junio C Hamano]

Vegard Nossum  writes:

> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 58f4bd6..d8cd854 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -60,6 +60,12 @@ diff.context::
>   Generate diffs with  lines of context instead of the default
>   of 3. This value is overridden by the -U option.
>  
> +diff.interHunkContext::
> + Show the context between diff hunks, up to  lines, thereby
> + fusing the hunks that are close to each other. The default is 0,
> + meaning no fusing will occur. This value is overridden by the
> + --inter-hunk-context option.

This is good if it were a description for
"--inter-hunk-context=", but the text needs to be adjusted if it
were to be used for "diff.interHunkContext".  It is unclear how the
'' the description refers to comes from.

I suspect that it would be sufficient to just revert the first
sentence to exactly match the way --inter-hunk-context= is
described.

> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index e6215c3..a219aa2 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -511,6 +511,8 @@ endif::git-format-patch[]
>  --inter-hunk-context=::
>   Show the context between diff hunks, up to the specified number
>   of lines, thereby fusing hunks that are close to each other.
> + Defaults to `diff.interHunkContext` or 0 if the config option
> + is unset.

This one is good, but then "The default is 0, meaning no fusing will
occur." in "diff.interHunkContext" is misleading and unnecessary.
When "diff.interHunkContext" is not set, it simply is not set (as
opposed to having a default value of 0).

> diff --git a/t/t4032-diff-inter-hunk-context.sh 
> b/t/t4032-diff-inter-hunk-context.sh
> index e4e3e28..d9ac9d1 100755
> --- a/t/t4032-diff-inter-hunk-context.sh
> +++ b/t/t4032-diff-inter-hunk-context.sh
> @@ -16,11 +16,15 @@ f() {
>  }
>  
>  t() {
> + use_config=""

It is more customary to just say

use_config=

All of the above are micronits that I can locally touch-up.  For
now, I'll queue the following.

-- >8 --
From: Vegard Nossum 
Date: Thu, 12 Jan 2017 13:21:11 +0100
Subject: [PATCH] diff: add interhunk context config option

The --inter-hunk-context= option was added in commit 6d0e674a5754
("diff: add option to show context between close hunks"). This patch
allows configuring a default for this option.

Signed-off-by: Vegard Nossum 
Signed-off-by: Junio C Hamano 
---
 Documentation/diff-config.txt  |  6 ++
 Documentation/diff-options.txt |  2 ++
 diff.c |  8 
 t/t4032-diff-inter-hunk-context.sh | 27 ++-
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index d8570f2a75..15521f5191 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -60,6 +60,12 @@ diff.context::
Generate diffs with  lines of context instead of the default
of 3. This value is overridden by the -U option.
 
+diff.interHunkContext::
+   Show the context between diff hunks, up to the specified number
+   of lines, thereby fusing the hunks that are close to each other.
+   This value serves as the default for the `--inter-hunk-context`
+   command line option.
+
 diff.external::
If this config variable is set, diff generation is not
performed using the internal diff machinery, but using the
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c372c..a219aa2907 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -511,6 +511,8 @@ endif::git-format-patch[]
 --inter-hunk-context=::
Show the context between diff hunks, up to the specified number
of lines, thereby fusing hunks that are close to each other.
+   Defaults to `diff.interHunkContext` or 0 if the config option
+   is unset.
 
 -W::
 --function-context::
diff --git a/diff.c b/diff.c
index e2eb6d66a9..f08cd8e033 100644
--- a/diff.c
+++ b/diff.c
@@ -32,6 +32,7 @@ static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
 static int diff_context_default = 3;
+static int diff_interhunk_context_default;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 static const char *diff_order_file_cfg;
@@ -239,6 +240,12 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
return -1;
return 0;
}
+   if (!strcmp(var, "diff.interhunkcontext")) {
+   diff_interhunk_context_default = git_config_int(var, value);
+   if (diff_interhunk_context_default < 0)
+   

Re: git clone failing when used through bundler on Docker for Windows with a shared volume

[Philip Oakley]

From: "Omar Qureshi" 

Hi there, I'm not sure this is the best place for this, but, this
seems to be an issue with Git when used through Docker on Windows when
there is a shared volume.

The issue is documented at
https://github.com/bundler/bundler/issues/5322 and I've provided a git


I think this was
7814fbe ("normalize_path_copy(): fix pushing to //server/share/dir on 
Windows", 2016-12-14)


I've added a longer comment to the github issue (didn't have email at the 
time)



repository that allows you to simulate the issue, for this the
requirements are Docker for Windows with the Docker client installed
on WSL as well as docker-compose installed via pip.

Docker for Windows will need to be configured to have a shared drive

Also, it makes no difference if a tag is provided or not


--
Philip 

Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path

[Junio C Hamano]

Johannes Schindelin  writes:

> So if this patch would make it into upstream Git, I would *have* to revert
> it in Git for Windows, adding to my already considerable maintenance burden.

I do not think we want "#!/usr/bin/env $language" in the upstream,
either.

Re: [PATCH 2/2] mailinfo: Understand forwarded patches

[Junio C Hamano]

Matthew Wilcox  writes:

> From: Matthew Wilcox 
>
> Extend the --scissors mechanism to strip off the preamble created by
> forwarding a patch.  There are a couple of extra headers ("Sent" and
> "To") added by forwarding, but other than that, the --scissors option
> will now remove patches forwarded from Microsoft Outlook to a Linux
> email account.
>
> Signed-off-by: Matthew Wilcox 
> ---
>  mailinfo.c | 9 -
>  1 file changed, 8 insertions(+), 1 deletion(-)

To be quite honest, I am not enthused to even having to think about
this kind of change.  There seems to be no standard way MUAs produce
"forwarded" messages, and this adds support for only one specific
MUA, that happens to say "Original Message".  Why should such a thing
hardcoded in this codepath?

I think I am OK with a patch that lets users customize
is_scissors_line(), perhaps accepting a regexp that ought to match a
line to consider a scissors line.  When such a regexp is given,
check for a match before doing the "do we have a line filled with
'-' with the scissors marker and is the mark long enough?" check.

Then you can do

mailinfo --scissors='^-{5}Original Message-{5}$'

or something like that.  Perhaps allow multiple regexps, or even
allow them to come from a multi-valued configuration variable.

> diff --git a/mailinfo.c b/mailinfo.c
> index 2059704a8..fc1275532 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi, struct 
> strbuf *subject)
>  
>  #define MAX_HDR_PARSED 10
>  static const char *header[MAX_HDR_PARSED] = {
> - "From","Subject","Date",
> + "From","Subject","Date","Sent","To",
>  };

This array lists fields whose value we _care_ about.  Do not put
random garbage whose value we do not use in it.

Even though I am not enthused to see support for messages forwarded
by Outlook bolted onto this codepath, I think it may make sense to
allow random garbage that looks like an e-mail header to appear
immediately after a scissors line (and ignore them).  For that,
perhaps you would instead want to extend is_inbody_header() so that
after it decides that the given line is *NOT* one of the header
field we care about, return a value that is not 0 or 1.  Its caller
currently expects to see 1 if it is a relevant in-body header line,
or 0 if the line signals the end of the in-body header block.  You'd
be adding another class of lines that are not a header line with a
meaning but do not terminate the in-body header block.


>  static inline int cmp_header(const struct strbuf *line, const char *hdr)
> @@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)
>   c++;
>   continue;
>   }
> + if (!memcmp(c, "Original Message", 16)) {
> + in_perforation = 1;
> + perforation += 16;
> + scissors += 16;
> + c += 15;
> + continue;
> + }
>   in_perforation = 0;
>   }

RE: [PATCH 2/2] mailinfo: Understand forwarded patches

[Matthew Wilcox]

From: Jonathan Tan [mailto:jonathanta...@google.com]
> On 01/12/2017 01:20 AM, Matthew Wilcox wrote:
> > From: Matthew Wilcox 
> >
> > Extend the --scissors mechanism to strip off the preamble created by
> > forwarding a patch.  There are a couple of extra headers ("Sent" and
> > "To") added by forwarding, but other than that, the --scissors option
> > will now remove patches forwarded from Microsoft Outlook to a Linux
> > email account.
> >
> > Signed-off-by: Matthew Wilcox 
> 
> Also add a test showing the kind of message that the current code
> doesn't handle, and that this commit addresses.

OK.  For the sake of discussion, here's what the base64-decoded message looks 
like:

--- 8< ---

-Original Message-
From: Rehas Sachdeva [mailto:aquan...@gmail.com]
Sent: Wednesday, January 4, 2017 11:55 AM
To: Matthew Wilcox ; r...@surriel.com
Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v

Make the output of radix tree test suite less verbose by default and add
-v and -vv command line options for increasing level of verbosity.

--- >8 ---

> > ---
> >  mailinfo.c | 9 -
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/mailinfo.c b/mailinfo.c
> > index 2059704a8..fc1275532 100644
> > --- a/mailinfo.c
> > +++ b/mailinfo.c
> > @@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi,
> struct strbuf *subject)
> >
> >  #define MAX_HDR_PARSED 10
> >  static const char *header[MAX_HDR_PARSED] = {
> > -   "From","Subject","Date",
> > +   "From","Subject","Date","Sent","To",
> 
> Are these extra headers used in both the "real" e-mail headers and the
> in-body headers, or only one of them? (If the latter, they should
> probably be handled only in the relevant function - my previous patches
> to this file were in that direction too, if I remember correctly.) Also,
> I suspect that these will have to be handled differently to the other 3,
> but that will be clearer when you add the test with an example message.

Without this part of the patch, using --scissors means it stops parsing headers 
when it hits the 'Sent' line.  So all I'm looking for is to have 
'is_inbody_header()' return true.  If there's a better way to achieve that, I'm 
all for it.  Without this hunk of the patch, the commit looks like this:

Without any of this patch, the commit looks like this:

Author: Matthew Wilcox 
Date:   Sat Jan 7 18:33:43 2017 +

FW: [PATCH v3] radix tree test suite: Dial down verbosity with -v

-Original Message-
From: Rehas Sachdeva [mailto:aquan...@gmail.com]
Sent: Wednesday, January 4, 2017 11:55 AM
To: Matthew Wilcox ; r...@surriel.com
Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v

Make the output of radix tree test suite less verbose by default and add
-v and -vv command line options for increasing level of verbosity.

Without this hunk of the patch, the commit looks like this:

Author: Rehas Sachdeva <[mailto:aquan...@gmail.com]>
Date:   Sat Jan 7 18:33:43 2017 +

FW: [PATCH v3] radix tree test suite: Dial down verbosity with -v

Sent: Wednesday, January 4, 2017 11:55 AM
To: Matthew Wilcox ; r...@surriel.com
Subject: [PATCH v3] radix tree test suite: Dial down verbosity with -v

Make the output of radix tree test suite less verbose by default and add
-v and -vv command line options for increasing level of verbosity.

And with this hunk, it turns into this:

Author: Rehas Sachdeva <[mailto:aquan...@gmail.com]>
Date:   Sat Jan 7 18:33:43 2017 +

radix tree test suite: Dial down verbosity with -v

Make the output of radix tree test suite less verbose by default and add
-v and -vv command line options for increasing level of verbosity.


There's more work to do here, turning that silly <[mailto:]> into a proper 
email address, for example, and parsing Sent: like we parse Date:, but I 
thought it worth sending out patches 1 & 2 before writing patch 3.

> > @@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)
> > c++;
> > continue;
> > }
> > +   if (!memcmp(c, "Original Message", 16)) {
> 
> 1) You can use starts_with or skip_prefix.

I was following the existing logic in this function that looks for 8< or >8 or 
...

> 2) This seems vulnerable to false positives. If "Original Message"
> always follows a certain kind of line, it might be better to check for
> that. (Again, it will be clearer when we have an example message.)

I don't think it's terribly vulnerable to false positives.  The logic at the 
end of the function tries to make sure that the scissor marks make up a 
substantial component of the line.

We could do this differently; I'm not sure if there's a regex library built 
into git, but we could even custom-write a 

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Junio C Hamano]

Johannes Schindelin  writes:

>> And I tend to agree that the silliness you observed (like a t-o-c
>> for a one-section "chapter") is not quite welcome.
>> 
>> For now I queued only 2/2 which looked good.  I won't object if
>> somebody else rerolls 1/2 to appease AsciiDoctor, but let's take an
>> obviously good bit first.
>
> For fun, I just reverted the article->book patch and I was greeted with
> this:
> ...
> It still builds, funnily enough, but the result is definitely worse on the
> eyes. The page is *really* long, and structuring it into individual parts
> does help the readability.
> ...
> P.S.: I also tried to use [glossary] and [appendix] as appropriate, but it
> seems that AsciiDoc *insists* on level-2 sections in an appendix, while
> AsciiDoctor *insists* on level-3 sections.

So in short, what you are saying is that the support for articles in
AsciiDoctor is borked and totally unusable on an article that needs
to be taken correctly by AsciiDoc, and your conclusion is that the
only way to move forward (other than giving up using AsciiDoctor) is
to avoid writing documents as articles, and existing articles need
to be adjusted to read as books.

If that is the case, then I agree with the conclusion.  As I already
said, I won't object to a reroll of 1/2 to make the document format
well with AsciiDoctor without breaking rendering by AsciiDoc too
badly, and your "for fun" experiment illustrated that such a reroll
still needs to avoid using article style.  Perhaps 1/2 posted as-is
is the best we could do within that constraint.

Let's queue it on 'pu' and see if somebody else comes up with an
update that is more visually pleasing with both backends.

Thanks.

RE: [PATCH 1/2] mailinfo: Add support for keep_cr

[Matthew Wilcox]

From: Jonathan Tan [mailto:jonathanta...@google.com]
> On 01/12/2017 01:20 AM, Matthew Wilcox wrote:
> A test exercising the new functionality would be nice.

Roger.

> Also, maybe a more descriptive title like "mailinfo: also respect
> keep_cr after base64 decode" (50 characters) is better.

No problem.

> > @@ -143,6 +144,7 @@ static void am_state_init(struct am_state *state,
> const char *dir)
> >
> > memset(state, 0, sizeof(*state));
> >
> > +   state->keep_cr = -1;
> 
> Maybe query the git config here (instead of later) so that we never have
> to worry about state->keep_cr being neither 0 nor 1? This function
> already queries the git config anyway.

I wondered why the existing code didn't do that, but I wanted to make a minimal 
change rather than clean up an older mistake.  I'm happy to do it that way.

> > diff --git a/mailinfo.h b/mailinfo.h
> > index 04a25351d..9fddcf684 100644
> > --- a/mailinfo.h
> > +++ b/mailinfo.h
> > @@ -12,6 +12,7 @@ struct mailinfo {
> > struct strbuf email;
> > int keep_subject;
> > int keep_non_patch_brackets_in_subject;
> > +   int keep_cr;
> > int add_message_id;
> > int use_scissors;
> > int use_inbody_headers;
> 
> I personally would write "unsigned keep_cr : 1" to further emphasize
> that this can only be 0 or 1, but I don't know if it's better to keep
> with the style existing in the file (that is, using int).

Probably best to stick to the existing file ... someone can always do a cleanup 
patch later, and having this match the others will make that easier, not harder.

Thanks for the review.

Re: [PATCH v3 06/38] sequencer (rebase -i): implement the 'edit' command

[Junio C Hamano]

Johannes Schindelin  writes:

> +static int make_patch(struct commit *commit, struct replay_opts *opts)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + struct rev_info log_tree_opt;
> + const char *subject, *p;
> + int res = 0;
> +
> + p = short_commit_name(commit);
> + if (write_message(p, strlen(p), rebase_path_stopped_sha(), 1) < 0)
> + return -1;
> +
> + strbuf_addf(, "%s/patch", get_dir(opts));
> + memset(_tree_opt, 0, sizeof(log_tree_opt));
> + init_revisions(_tree_opt, NULL);
> + log_tree_opt.abbrev = 0;
> + log_tree_opt.diff = 1;
> + log_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;
> + log_tree_opt.disable_stdin = 1;
> + log_tree_opt.no_commit_id = 1;
> + log_tree_opt.diffopt.file = fopen(buf.buf, "w");
> + log_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;
> + if (!log_tree_opt.diffopt.file)
> + res |= error_errno(_("could not open '%s'"), buf.buf);
> + else {
> + res |= log_tree_commit(_tree_opt, commit);
> + fclose(log_tree_opt.diffopt.file);
> + }
> + strbuf_reset();
> +
> + strbuf_addf(, "%s/message", get_dir(opts));
> + if (!file_exists(buf.buf)) {
> + const char *commit_buffer = get_commit_buffer(commit, NULL);
> + find_commit_subject(commit_buffer, );
> + res |= write_message(subject, strlen(subject), buf.buf, 1);
> + unuse_commit_buffer(commit, commit_buffer);
> + }
> + strbuf_release();
> +
> + return res;
> +}

Unlike the scripted version, where a merge is shown with "diff --cc"
and a root commit is shown as "Root commit", this only deals with a
single-parent commit.  Is this because this helper, at least in its
current form, will not be used by "rebase -m" and not with "--root"?

If that is the case, that is perfectly fine, perhaps that deserves a
mention in the log message and in-code comment before the function.

Re: [PATCH v3 02/38] sequencer: move "else" keyword onto the same line as preceding brace

[Junio C Hamano]

Johannes Schindelin  writes:

> It is the current coding style of the Git project to write
>
>   if (...) {
>   ...
>   } else {
>   ...
>   }
>
> instead of putting the closing brace and the "else" keyword on separate
> lines.
>
> Pointed out by Junio Hamano.

For a small thing like this, the attribution is mostly irrelevant,
but for the record, the credit should go to checkpatch.pl from the
kernel project ;-).

I'll try to squash the part that was touched by 01/38 into this
patch locally; even though I haven't finished going through the
individual patches, I expect that my conclusion would be what I
said in , i.e. the
changes in the series are mostly good and improved relative to the
previous round except for the parts reading and writing of
author-script (from the maintainability's point of view).  

As I do not think we want to see another reroll of three-dozen
patches, I am leaning towards merging v3 (with minimum fixes like
squashing 01/ and 02/ into one) to 'next' and cook it there and
fix remaining issues incrementally while the series is in 'next'.

I may have to change my mind after I read through the remaining
patches (I've done with the first dozen or so at this moment), but I
do not expect that, judging from what I saw in the interdiff.

Thanks.

> Signed-off-by: Johannes Schindelin 
> ---
>  sequencer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 23793db08b..3eededcb98 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1070,8 +1070,7 @@ static int create_seq_dir(void)
>   error(_("a cherry-pick or revert is already in progress"));
>   advise(_("try \"git cherry-pick (--continue | --quit | 
> --abort)\""));
>   return -1;
> - }
> - else if (mkdir(git_path_seq_dir(), 0777) < 0)
> + } else if (mkdir(git_path_seq_dir(), 0777) < 0)
>   return error_errno(_("could not create sequencer directory 
> '%s'"),
>  git_path_seq_dir());
>   return 0;

Re: [PATCH v3 03/38] sequencer: use a helper to find the commit message

[Junio C Hamano]

Johannes Schindelin  writes:

> It is actually not safe to look for a commit message by looking for the
> first empty line and skipping it.
>
> The find_commit_subject() function looks more carefully, so let's use
> it. Since we are interested in the entire commit message, we re-compute
> the string length after verifying that the commit subject is not empty
> (in which case the entire commit message would be empty, something that
> should not happen but that we want to handle gracefully).
>
> Signed-off-by: Johannes Schindelin 
> ---

Very sensible.

>  sequencer.c | 11 +++
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 3eededcb98..720857beda 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -703,14 +703,9 @@ static int do_pick_commit(enum todo_command command, 
> struct commit *commit,
>   next = commit;
>   next_label = msg.label;
>  
> - /*
> -  * Append the commit log message to msgbuf; it starts
> -  * after the tree, parent, author, committer
> -  * information followed by "\n\n".
> -  */
> - p = strstr(msg.message, "\n\n");
> - if (p)
> - strbuf_addstr(, skip_blank_lines(p + 2));
> + /* Append the commit log message to msgbuf. */
> + if (find_commit_subject(msg.message, ))
> + strbuf_addstr(, p);
>  
>   if (opts->record_origin) {
>   if (!has_conforming_footer(, NULL, 0))

Re: [PATCH v3 01/38] sequencer: avoid unnecessary curly braces

[Junio C Hamano]

Johannes Schindelin  writes:

>  
> - if (!commit->parents) {
> + if (!commit->parents)
>   parent = NULL;
> - }
>   else if (commit->parents->next) {
>   /* Reverting or cherry-picking a merge commit */
>   int cnt;

The result becomes

if (...)
single statement;
else if (...) {
multiple;
statements;
}

which is not quite an improvement.  

The preferred style is for all arms in if/elseif/else cascade to
either use or not use brace pairs, so I think a fix toward that goal
would be more like:

if (!commit->parents) {
parent = NULL;
-   }
-   else if (commit->parents->next) {
+   } else if (commit->parents->next) {
/* Reverting ...

Re: Bug report: Git pull hang occasionally

[Kai Zhang]

Hi Junio,

After apply this patch, hanging did not happen again. Would this patch go to 
release in near future?

Thanks.

Regards,
Kai 
> On Dec 21, 2016, at 1:32 PM, Junio C Hamano  wrote:
> 
> Junio C Hamano  writes:
> 
>> And the unexpected discrepancy is reported by find_symref() as
>> fatal.  The server side dies, and somehow that fact is lost between
>> the upload-pack process and the client and somebody in the middle
>> (e.g. fastcgi interface or nginx webserver on the server side, or
>> the remote-curl helper on the client side) keeps the "git fetch"
>> process waiting.
>> 
>> So there seem to be two issues.  
>> 
>> - Because of the unlocked read, find_symref() can observe an
>>   inconsistent state.  Perhaps it should be updated not to die but
>>   to retry, expecting that transient inconsistency will go away.
>> 
>> - A fatal error in upload-pack is not reported back to the client
>>   to cause it exit is an obvious one, and even if we find a way to
>>   make this fatal error in find_symref() not to trigger, fatal
>>   errors in other places in the code can trigger the same symptom.
> 
> I wonder if the latter is solved by recent patch 296b847c0d
> ("remote-curl: don't hang when a server dies before any output",
> 2016-11-18) on the client side.
> 
> -- >8 --
> From: David Turner 
> Date: Fri, 18 Nov 2016 15:30:49 -0500
> Subject: [PATCH] remote-curl: don't hang when a server dies before any output
> 
> In the event that a HTTP server closes the connection after giving a
> 200 but before giving any packets, we don't want to hang forever
> waiting for a response that will never come.  Instead, we should die
> immediately.
> 
> One case where this happens is when attempting to fetch a dangling
> object by its object name.  In this case, the server dies before
> sending any data.  Prior to this patch, fetch-pack would wait for
> data from the server, and remote-curl would wait for fetch-pack,
> causing a deadlock.
> 
> Despite this patch, there is other possible malformed input that could
> cause the same deadlock (e.g. a half-finished pktline, or a pktline but
> no trailing flush).  There are a few possible solutions to this:
> 
> 1. Allowing remote-curl to tell fetch-pack about the EOF (so that
> fetch-pack could know that no more data is coming until it says
> something else).  This is tricky because an out-of-band signal would
> be required, or the http response would have to be re-framed inside
> another layer of pkt-line or something.
> 
> 2. Make remote-curl understand some of the protocol.  It turns out
> that in addition to understanding pkt-line, it would need to watch for
> ack/nak.  This is somewhat fragile, as information about the protocol
> would end up in two places.  Also, pkt-lines which are already at the
> length limit would need special handling.
> 
> Both of these solutions would require a fair amount of work, whereas
> this hack is easy and solves at least some of the problem.
> 
> Still to do: it would be good to give a better error message
> than "fatal: The remote end hung up unexpectedly".
> 
> Signed-off-by: David Turner 
> Signed-off-by: Junio C Hamano 
> ---
> remote-curl.c   |  8 
> t/t5551-http-fetch-smart.sh | 30 ++
> 2 files changed, 38 insertions(+)
> 
> diff --git a/remote-curl.c b/remote-curl.c
> index f14c41f4c0..ee4423659f 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -400,6 +400,7 @@ struct rpc_state {
>   size_t pos;
>   int in;
>   int out;
> + int any_written;
>   struct strbuf result;
>   unsigned gzip_request : 1;
>   unsigned initial_buffer : 1;
> @@ -456,6 +457,8 @@ static size_t rpc_in(char *ptr, size_t eltsize,
> {
>   size_t size = eltsize * nmemb;
>   struct rpc_state *rpc = buffer_;
> + if (size)
> + rpc->any_written = 1;
>   write_or_die(rpc->in, ptr, size);
>   return size;
> }
> @@ -659,6 +662,8 @@ static int post_rpc(struct rpc_state *rpc)
>   curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
>   curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
> 
> +
> + rpc->any_written = 0;
>   err = run_slot(slot, NULL);
>   if (err == HTTP_REAUTH && !large_request) {
>   credential_fill(_auth);
> @@ -667,6 +672,9 @@ static int post_rpc(struct rpc_state *rpc)
>   if (err != HTTP_OK)
>   err = -1;
> 
> + if (!rpc->any_written)
> + err = -1;
> +
>   curl_slist_free_all(headers);
>   free(gzip_body);
>   return err;
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 1ec5b2747a..43665ab4a8 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -276,6 +276,36 @@ test_expect_success 'large fetch-pack requests can be 
> split across POSTs' '
>   test_line_count = 2 posts
> '
> 
> 

Re: [PATCH 2/2] mailinfo: Understand forwarded patches

[Jonathan Tan]

On 01/12/2017 01:20 AM, Matthew Wilcox wrote:

From: Matthew Wilcox 

Extend the --scissors mechanism to strip off the preamble created by
forwarding a patch.  There are a couple of extra headers ("Sent" and
"To") added by forwarding, but other than that, the --scissors option
will now remove patches forwarded from Microsoft Outlook to a Linux
email account.

Signed-off-by: Matthew Wilcox 


Also add a test showing the kind of message that the current code 
doesn't handle, and that this commit addresses.



---
 mailinfo.c | 9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/mailinfo.c b/mailinfo.c
index 2059704a8..fc1275532 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -332,7 +332,7 @@ static void cleanup_subject(struct mailinfo *mi, struct 
strbuf *subject)

 #define MAX_HDR_PARSED 10
 static const char *header[MAX_HDR_PARSED] = {
-   "From","Subject","Date",
+   "From","Subject","Date","Sent","To",


Are these extra headers used in both the "real" e-mail headers and the 
in-body headers, or only one of them? (If the latter, they should 
probably be handled only in the relevant function - my previous patches 
to this file were in that direction too, if I remember correctly.) Also, 
I suspect that these will have to be handled differently to the other 3, 
but that will be clearer when you add the test with an example message.



 };

 static inline int cmp_header(const struct strbuf *line, const char *hdr)
@@ -685,6 +685,13 @@ static int is_scissors_line(const char *line)
c++;
continue;
}
+   if (!memcmp(c, "Original Message", 16)) {


1) You can use starts_with or skip_prefix.
2) This seems vulnerable to false positives. If "Original Message" 
always follows a certain kind of line, it might be better to check for 
that. (Again, it will be clearer when we have an example message.)



+   in_perforation = 1;
+   perforation += 16;
+   scissors += 16;
+   c += 15;


Why 15? Also, can skip_prefix avoid these magic numbers?


+   continue;
+   }
in_perforation = 0;
}

Re: [PATCH 1/2] mailinfo: Add support for keep_cr

[Jonathan Tan]

On 01/12/2017 01:20 AM, Matthew Wilcox wrote:

From: Matthew Wilcox 

If you have a base-64 encoded patch with CRLF endings (as produced
by forwarding a patch from Outlook to a Linux machine, for example),
the keep_cr setting is not honoured because keep_cr is only passed
to mailsplit, which does not look through the encoding.  The keep_cr
logic needs to be applied after the base64 decode.  I copied that
logic to handle_filter(), and rather than add a new keep_cr parameter
to handle_filter, I opted to add keep_cr to struct mailinfo; it seemed
appropriate given use_scissors was already there.

Then I needed to initialise keep_cr in the struct mailinfo passed from
git-am, and rather than thread a 'keep_cr' parameter all the way through
to parse_mail(), I decided to add keep_cr to struct am_state, which let
it be removed as a parameter from five other functions.

Signed-off-by: Matthew Wilcox 


A test exercising the new functionality would be nice.

Also, maybe a more descriptive title like "mailinfo: also respect 
keep_cr after base64 decode" (50 characters) is better.



@@ -143,6 +144,7 @@ static void am_state_init(struct am_state *state, const 
char *dir)

memset(state, 0, sizeof(*state));

+   state->keep_cr = -1;


Maybe query the git config here (instead of later) so that we never have 
to worry about state->keep_cr being neither 0 nor 1? This function 
already queries the git config anyway.



diff --git a/mailinfo.h b/mailinfo.h
index 04a25351d..9fddcf684 100644
--- a/mailinfo.h
+++ b/mailinfo.h
@@ -12,6 +12,7 @@ struct mailinfo {
struct strbuf email;
int keep_subject;
int keep_non_patch_brackets_in_subject;
+   int keep_cr;
int add_message_id;
int use_scissors;
int use_inbody_headers;


I personally would write "unsigned keep_cr : 1" to further emphasize 
that this can only be 0 or 1, but I don't know if it's better to keep 
with the style existing in the file (that is, using int).

Re: [PATCH] imap-send.c: Avoid deprecated openssl 1.1.0 API

[Jack Bates]

You might need the following, to still build with LibreSSL.
That was my experience anyway, when I recently prepared similar fixes 
for OpenSSL 1.1 and Apache Traffic Server.


#if OPENSSL_VERSION_NUMBER < 0x1010L || defined(LIBRESSL_VERSION_NUMBER)

On 12/01/17 03:42 AM, eroen wrote:

Library initialization functions are deprecated in openssl 1.1.0 API, as
initialization is handled by openssl internally.

Symbols for deprecated functions are not exported if openssl is built with
`--api=1.1 disable-deprecated`, so their use will cause a build failure.

Reported-by: Lars Wendler (Polynomial-C)

X-Gentoo-Bug: 592466
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=592466
---
 imap-send.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/imap-send.c b/imap-send.c
index 5c7e27a89..98774360e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -284,8 +284,10 @@ static int ssl_socket_connect(struct imap_socket *sock, 
int use_tls_only, int ve
int ret;
X509 *cert;

+#if OPENSSL_VERSION_NUMBER < 0x1010L
SSL_library_init();
SSL_load_error_strings();
+#endif

meth = SSLv23_method();
if (!meth) {

Re: [PATCH] imap-send.c: Avoid deprecated openssl 1.1.0 API

[Jack Bates]

You might need the following, to still build with LibreSSL.
That was my experience anyway, when I recently prepared similar fixes 
for OpenSSL 1.1 and Apache Traffic Server.


#if OPENSSL_VERSION_NUMBER < 0x1010L || defined(LIBRESSL_VERSION_NUMBER)

On 12/01/17 03:42 AM, eroen wrote:

Library initialization functions are deprecated in openssl 1.1.0 API, as
initialization is handled by openssl internally.

Symbols for deprecated functions are not exported if openssl is built with
`--api=1.1 disable-deprecated`, so their use will cause a build failure.

Reported-by: Lars Wendler (Polynomial-C)

X-Gentoo-Bug: 592466
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=592466
---
 imap-send.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/imap-send.c b/imap-send.c
index 5c7e27a89..98774360e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -284,8 +284,10 @@ static int ssl_socket_connect(struct imap_socket *sock, 
int use_tls_only, int ve
int ret;
X509 *cert;

+#if OPENSSL_VERSION_NUMBER < 0x1010L
SSL_library_init();
SSL_load_error_strings();
+#endif

meth = SSLv23_method();
if (!meth) {

problem with insider build for windows and git

[Michael Gooch]

when running commands like pull and clone I get the following message:

Cygwin WARNING:
  Couldn't compute FAST_CWD pointer.  This typically occurs if you're using
  an older Cygwin version on a newer Windows.  Please update to the latest
  available Cygwin version from https://cygwin.com/.  If the problem 
persists,

  please see https://cygwin.com/problems.html

Windows build is version 1607, OS BUILD 15002.1001

I assume they broke something that cygwin was depending on.

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Sixt]

Am 12.01.2017 um 01:17 schrieb Jacob Keller:

From: Jacob Keller 

Teach git-describe the `--discard` option which will allow specifying
a glob pattern of tags to ignore. This can be combined with the
`--match` patterns to enable more flexibility in determining which tags
to consider.

For example, suppose you wish to find the first official release tag
that contains a certain commit. If we assume that official release tags
are of the form "v*" and pre-release candidates include "*rc*" in their
name, we can now find the first tag that introduces commit abcdef via:

  git describe --contains --match="v*" --discard="*rc*"


I have a few dozen topic branches, many of them are work in progress and 
named wip/something. To see the completed branches, I routinely say


gitk --exclude=wip/* --branches

these days.

It would be great if you could provide the same user interface here. The 
example in the commit message would then look like this:


   git describe --contains --exclude="*rc*" --match="v*"

(I'm not saying that you should add --branches, but that you should 
prefer --exclude over --discard. Also, the order of --exclude and 
--match would be important.)


-- Hannes

[PATCH v2] diff: add interhunk context config option

[Vegard Nossum]

The --inter-hunk-context= option was added in commit 6d0e674a5754
("diff: add option to show context between close hunks"). This patch
allows configuring a default for this option.

Cc: René Scharfe 
Cc: Pranit Bauva 
Signed-off-by: Vegard Nossum 

---
v2:
 - Update Documentation/diff-config.txt, suggested by Pranit Bauva.
 - Add tests, suggested by Pranit Bauva.
 - Don't initialize BSS variable to 0, suggested by Junio Hamano.
 - Junio: if git_config_int() fails, you will get something like:
   "fatal: bad config variable 'diff.interhunkcontext' in file 
'/home/vegard/.gitconfig' at line 5"
---
 Documentation/diff-config.txt  |  6 ++
 Documentation/diff-options.txt |  2 ++
 diff.c |  8 
 t/t4032-diff-inter-hunk-context.sh | 27 ++-
 4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index 58f4bd6..d8cd854 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -60,6 +60,12 @@ diff.context::
Generate diffs with  lines of context instead of the default
of 3. This value is overridden by the -U option.
 
+diff.interHunkContext::
+   Show the context between diff hunks, up to  lines, thereby
+   fusing the hunks that are close to each other. The default is 0,
+   meaning no fusing will occur. This value is overridden by the
+   --inter-hunk-context option.
+
 diff.external::
If this config variable is set, diff generation is not
performed using the internal diff machinery, but using the
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e6215c3..a219aa2 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -511,6 +511,8 @@ endif::git-format-patch[]
 --inter-hunk-context=::
Show the context between diff hunks, up to the specified number
of lines, thereby fusing hunks that are close to each other.
+   Defaults to `diff.interHunkContext` or 0 if the config option
+   is unset.
 
 -W::
 --function-context::
diff --git a/diff.c b/diff.c
index 84dba60..a92080c 100644
--- a/diff.c
+++ b/diff.c
@@ -33,6 +33,7 @@ static int diff_rename_limit_default = 400;
 static int diff_suppress_blank_empty;
 static int diff_use_color_default = -1;
 static int diff_context_default = 3;
+static int diff_interhunk_context_default;
 static const char *diff_word_regex_cfg;
 static const char *external_diff_cmd_cfg;
 static const char *diff_order_file_cfg;
@@ -248,6 +249,12 @@ int git_diff_ui_config(const char *var, const char *value, 
void *cb)
return -1;
return 0;
}
+   if (!strcmp(var, "diff.interhunkcontext")) {
+   diff_interhunk_context_default = git_config_int(var, value);
+   if (diff_interhunk_context_default < 0)
+   return -1;
+   return 0;
+   }
if (!strcmp(var, "diff.renames")) {
diff_detect_rename_default = git_config_rename(var, value);
return 0;
@@ -3371,6 +3378,7 @@ void diff_setup(struct diff_options *options)
options->rename_limit = -1;
options->dirstat_permille = diff_dirstat_permille_default;
options->context = diff_context_default;
+   options->interhunkcontext = diff_interhunk_context_default;
options->ws_error_highlight = ws_error_highlight_default;
DIFF_OPT_SET(options, RENAME_EMPTY);
 
diff --git a/t/t4032-diff-inter-hunk-context.sh 
b/t/t4032-diff-inter-hunk-context.sh
index e4e3e28..d9ac9d1 100755
--- a/t/t4032-diff-inter-hunk-context.sh
+++ b/t/t4032-diff-inter-hunk-context.sh
@@ -16,11 +16,15 @@ f() {
 }
 
 t() {
+   use_config=""
+   git config --unset diff.interHunkContext
+
case $# in
4) hunks=$4; cmd="diff -U$3";;
5) hunks=$5; cmd="diff -U$3 --inter-hunk-context=$4";;
+   6) hunks=$5; cmd="diff -U$3"; git config diff.interHunkContext $4; 
use_config="(diff.interHunkContext=$4) ";;
esac
-   label="$cmd, $1 common $2"
+   label="$use_config$cmd, $1 common $2"
file=f$1
expected=expected.$file.$3.$hunks
 
@@ -89,4 +93,25 @@ t 9 lines3   2
 t 9 lines  3   2   2
 t 9 lines  3   3   1
 
+#  use diff.interHunkContext?
+t 1 line   0   0   2   config
+t 1 line   0   1   1   config
+t 1 line   0   2   1   config
+t 9 lines  3   3   1   config
+t 2 lines  0   0   2   config
+t 2 lines  0   1   2   config
+t 2 lines  0   2   1   config
+t 3 lines  1   0   2   config
+t 3 lines  1   1   1   config
+t 3 lines  1   2   1   config
+t 9 lines  3   2   2 

Re: [PATCH v4] log --graph: customize the graph lines with config log.graphColors

[Duy Nguyen]

Just FYI. The broken internet cables in Vietnam seem to hit my ISP
really hard. It's nearly impossible to make a TCP connection. So I'm
basically off the grid, hopefully not longer than two weeks.

On 1/10/17, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> +end = string + strlen(string);
>> +while (start < end) {
>> +const char *comma = strchrnul(start, ',');
>> +char color[COLOR_MAXLEN];
>> +
>> +while (start < comma && isspace(*start))
>> +start++;
>> +if (start == comma) {
>> +start = comma + 1;
>> +continue;
>> +}
>> +
>> +if (!color_parse_mem(start, comma - start, color))
>
> So you skip the leading blanks but let color_parse_mem() trim the
> trailing blanks?  It would work once the control reaches the loop,
> but wouldn't that miss
>
>   git -c log.graphColors=' reset , blue, red' log --graph
>
> as "reset" is not

git clone failing when used through bundler on Docker for Windows with a shared volume

[Omar Qureshi]

Hi there, I'm not sure this is the best place for this, but, this
seems to be an issue with Git when used through Docker on Windows when
there is a shared volume.

The issue is documented at
https://github.com/bundler/bundler/issues/5322 and I've provided a git
repository that allows you to simulate the issue, for this the
requirements are Docker for Windows with the Docker client installed
on WSL as well as docker-compose installed via pip.

Docker for Windows will need to be configured to have a shared drive

Also, it makes no difference if a tag is provided or not

Re: [PATCH 1/2] asciidoctor: fix user-manual to be built by `asciidoctor`

[Johannes Schindelin]

Hi Junio,

On Tue, 10 Jan 2017, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > On Sat, Jan 07, 2017 at 02:03:30PM -0800, Junio C Hamano wrote:
> >
> >> Is that a longer way to say that the claim "... is designed as a
> >> book" is false?
> >> 
> >> > So I dunno. I really do think "article" is conceptually the most
> >> > appropriate style, but I agree that there are some book-like things
> >> > (like appendices).
> >> 
> >> ... Yeah, I should have read forward first before starting to insert
> >> my comments.
> >
> > To be honest, I'm not sure whether "book" versus "article" was really
> > considered in the original writing. I think we can call it whichever
> > produces the output we find most pleasing. I was mostly just pointing at
> > there are some tradeoffs in the end result in flipping the type.
> 
> I understand.  
> 
> And I tend to agree that the silliness you observed (like a t-o-c
> for a one-section "chapter") is not quite welcome.
> 
> For now I queued only 2/2 which looked good.  I won't object if
> somebody else rerolls 1/2 to appease AsciiDoctor, but let's take an
> obviously good bit first.

For fun, I just reverted the article->book patch and I was greeted with
this:

-- snip --
ASCIIDOC user-manual.xml
asciidoctor: ERROR: user-manual.txt: line 44: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 477: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 477: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 477: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1003: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1003: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1003: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1003: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1720: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1720: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1720: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1720: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 1720: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2462: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2462: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2462: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2462: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2814: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2814: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2814: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2958: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2958: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 2958: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 3514: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 3514: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 3514: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 3771: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 3771: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 3771: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4147: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4147: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4147: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4395: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4395: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4395: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4401: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4401: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4636: only book doctypes can
contain level 0 sections
asciidoctor: ERROR: user-manual.txt: line 4636: only book doctypes can
contain level 0 

[PATCH] imap-send.c: Avoid deprecated openssl 1.1.0 API

[eroen]

Library initialization functions are deprecated in openssl 1.1.0 API, as
initialization is handled by openssl internally.

Symbols for deprecated functions are not exported if openssl is built with
`--api=1.1 disable-deprecated`, so their use will cause a build failure.

Reported-by: Lars Wendler (Polynomial-C)

X-Gentoo-Bug: 592466
X-Gentoo-Bug-URL: https://bugs.gentoo.org/show_bug.cgi?id=592466
---
 imap-send.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/imap-send.c b/imap-send.c
index 5c7e27a89..98774360e 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -284,8 +284,10 @@ static int ssl_socket_connect(struct imap_socket *sock, 
int use_tls_only, int ve
int ret;
X509 *cert;
 
+#if OPENSSL_VERSION_NUMBER < 0x1010L
SSL_library_init();
SSL_load_error_strings();
+#endif
 
meth = SSLv23_method();
if (!meth) {
-- 
2.11.0

Re: [PATCH 2/2] Use 'env' to find perl instead of fixed path

[Johannes Schindelin]

Hi Pat,

On Thu, 12 Jan 2017, Pat Pannuto wrote:

> I'm not at all attached to changing all of them, just figured it made
> sense while I was here.
> 
> Would a patch that changes only:
> 
>  git-add--interactive.perl | 2 +-
>  git-archimport.perl   | 2 +-
>  git-cvsexportcommit.perl  | 2 +-
>  git-cvsimport.perl| 2 +-
>  git-cvsserver.perl| 2 +-
>  git-difftool.perl | 2 +-
>  git-relink.perl   | 2 +-
>  git-send-email.perl   | 2 +-
>  git-svn.perl  | 2 +-
> 
> be more acceptable?

Unfortunately not. Take git-svn.perl for example, in particular in
conjunction with Git for Windows. git-svn requires the Subversion bindings
for Perl, and they are a *major* pain to build correctly (this is
frequently underestimated by users who complain about git-svn problems).

Allowing users to override the Perl path (even if it were possible, which
it is not, as Hannes Sixt pointed out in the mail you replied to) would
open Git for Windows for a metric ton of users complaining that their
git-svn does not work (when it is their fault, really, by overriding Perl
with their own preferred version that comes without Subversion bindings,
but how would they know).

So if this patch would make it into upstream Git, I would *have* to revert
it in Git for Windows, adding to my already considerable maintenance burden.

Ciao,
Johannes

Re: [PATCH 0/5] extend git-describe pattern matching

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> From: Jacob Keller 
> 
> Teach git describe and git name-rev the ability to match multiple
> patterns inclusively. Additionally, teach these commands to also accept
> negative patterns to discard any refs which match.

I like the idea, and I think this patch series is already in a pretty good
shape, offering a couple of comments as individual replies to the
respective patches.

Ciao,
Dscho

Re: [PATCH 3/5] name-rev: add support to discard refs by pattern match

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> From: Jacob Keller 
> 
> Extend name-rev further to support matching refs by adding `--discard`
> patterns.

Same comment applies as for 5/5: `--exclude-refs` may be a better name
than `--discard`.

Ciao,
Dscho

Re: [PATCH 2/5] name-rev: extend --refs to accept multiple patterns

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> diff --git a/t/t6007-rev-list-cherry-pick-file.sh 
> b/t/t6007-rev-list-cherry-pick-file.sh
> index 1408b608eb03..d072ec43b016 100755
> --- a/t/t6007-rev-list-cherry-pick-file.sh
> +++ b/t/t6007-rev-list-cherry-pick-file.sh
> @@ -99,6 +99,36 @@ test_expect_success '--cherry-pick bar does not come up 
> empty (II)' '
>   test_cmp actual.named expect
>  '
>  
> +test_expect_success 'name-rev multiple --refs combine inclusive' '
> + git rev-list --left-right --cherry-pick F...E -- bar > actual &&

Our current coding style seems to skip the space between `>` and `actual`
(this applies to all redirections added in this patch).

> + git name-rev --stdin --name-only --refs="*tags/F" --refs="*tags/E" \
> + < actual > actual.named &&
> + test_cmp actual.named expect
> +'
> +
> +cat >expect < + +$(git rev-list --left-right --right-only --cherry-pick F...E -- bar)
> +EOF

In the current revision of t6007, we seem to list the expected output
explicitly, i.e. *not* generating it dynamically.

If you *do* insist to generate the `expect` file dynamically, a better way
would be to include that generation in the `test_expect_success` code so
that errors in the call can be caught, too:

test_expect_success 'name-rev --refs excludes non-matched patterns' '
echo "expect &&
git rev-list --left-right --right-only --cherry-pick F...E -- \
bar >>expect &&
[...]

However, if I was asked for my preference, I would suggest to specify the
`expect` contents explicitly, to document the expectation as of time of
writing. The reason: I debugged my share of test breakages and these
dynamically-generated `expect` files are the worst. When things break, you
have to dig *real* deep to figure out what is going wrong, as sometimes
the *generation of the `expect` file* regresses.

Ciao,
Dscho

Re: [PATCH 1/5] doc: add documentation for OPT_STRING_LIST

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> diff --git a/Documentation/technical/api-parse-options.txt 
> b/Documentation/technical/api-parse-options.txt
> index 27bd701c0d68..15e876e4c804 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -168,6 +168,11 @@ There are some macros to easily define options:
>   Introduce an option with string argument.
>   The string argument is put into `str_var`.
>  
> +`OPT_STRING_LIST(short, long, , arg_str, description)`::
> + Introduce an option with a string argument. Repeated invocations
> + accumulate into a list of strings. Reset and clear the list with
> + `--no-option`.

One suggestions: as the list parameter is not type-safe (apart from
checking that it can be cast to a `void *`), it would be good to mention
in the documentation that `list` must be of type `struct string_list`.

I was about to suggest that `--no-option` may be misleading, as the
command-line option is not really called `--option` in almost all cases,
but I see that the rest of that document uses that convention to refer to
the negated option already...

Ciao,
Dscho

Re: [PATCH 5/5] describe: teach describe negative pattern matches

[Johannes Schindelin]

Hi Jake,

On Wed, 11 Jan 2017, Jacob Keller wrote:

> From: Jacob Keller 
> 
> Teach git-describe the `--discard` option which will allow specifying
> a glob pattern of tags to ignore.

IMHO "discard" is the wrong word, it almost sounds as if the matching tags
would be *deleted*.

Maybe `--exclude` or `--unmatch` instead?

Ciao,
Dscho

git fast-import crashing on big imports

[Ulrich Spörlein]

Hey,

the FreeBSD svn2git conversion is crashing somewhat
non-deterministically during its long conversion process. From memory,
this was not as bad is it is with more recent versions of git (but I
can't be sure, really).

I have a dump file that you can grab at
http://scan.spoerlein.net/pub/freebsd-base.dump.xz (19G, 55G uncompressed)
that shows this problem after a couple of minutes of runtime. The caveat is
that for another member of the team on a different machine the crashes are on
different revisions.

Googling around I found two previous threads that were discussing
problems just like this (memory corruption, bad caching, etc)

https://www.spinics.net/lists/git/msg93598.html  from 2009
and
http://git.661346.n2.nabble.com/long-fast-import-errors-out-quot-failed-to-apply-delta-quot-td6557884.html
from 2011

% git fast-import --stats < ../freebsd-base.dump
...
progress SVN r49318 branch master = :49869
progress SVN r49319 branch stable/3 = :49870
progress SVN r49320 branch master = :49871
error: failed to apply delta
error: bad offset for revindex
error: bad offset for revindex
error: bad offset for revindex
error: bad offset for revindex
error: bad offset for revindex
fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e
fast-import: dumping crash report to fast_import_crash_29613


fast-import crash report:
fast-import process: 29613
parent process : 29612
at 2017-01-11 19:33:37 +

fatal: Can't load tree b35ae4e9c2a41677e84a3f14bed09f584c3ff25e


git fsck shows a somewhat incomplete pack file (I guess that's expected if the
process dies mid-stream?)

% git fsck
Checking object directories: 100% (256/256), done.
error: failed to apply delta6/614500)
error: cannot unpack d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f from 
./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 
122654805
error: failed to apply delta
error: failed to read delta base object 
d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from 
./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
error: cannot unpack 8523bde63ef34bef725347994fdaec996d756510 from 
./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack at offset 
122671596
error: failed to apply delta0/614500)
error: failed to read delta base object 
d1d7ee1f81c6767c5e0f75d14d400d7512a85a0f at offset 122654805 from 
./objects/pack/pack-e28fcea43fc221d2ebe92857b484da58bb888237.pack
...


Any comments on whether the original problems from 2009 and 2011 were ever
fixed and committed?

Some more facts:
- git version 2.11.0
- I don't recall these sorts of crashes with a git from 2-3 years ago
- adding more checkpoints does help, but not fix the problem, it merely shifts
  the crashes around to different revisions
- incremental runs of the conversion *will* complete most of the time, but
  depending on how often checkpoints are used, I've seen it croak on specific
  commits and not being able to progress further :(

Thanks for any pointers or things to try!
Cheers
Uli

Re: [PATCH 0/2] Use env for all perl invocations

[Junio C Hamano]

Pat Pannuto  writes:

> It feels weird to me that the perl path is fixed at
> compile/install-time as opposed to run-time discovery -- this means
> users can't change their perl install without breaking git?

Among the software packages that use interpreters like perl, python,
ruby, etc., there are ones that seem to consider that it is a good
idea to let "#!/usr/bin/env $language" pick whatever variant
[*1*] of the $language that happens to be on end-user's $PATH.

Git does not subscribe to that thought, and it is done for very good
reasons.

When you take a popular $language used by many software packages, it
is more than likely that one particular end user uses more than one
such package written in the same $language.  If one assumes that
there is one variant of $language such software packages can all
use, then $PATH can be tweaked so that the common variant and no
other variants of $language can be found and you are done.

However, that is too simplistic in real life.  If you are using Git
(which wants Perl 5.8 or later with certain libs) and some other
software package that needs a much older Perl, there is no such
single variant that can be placed on end-user's $PATH.  Only when
all the other software packages write their she-bang line without
"env" and instead point at the exact variant they need, Git can use
the "env" to rely on $PATH, but at that point, Git is being overly
arrogant.  It insists to be special among packages that use the same
$language and wants the variant it needs to be on $PATH.

Git knows that the real-world is not simplistic.  Git is not
arrogant, either.  And that is why it does not use "#!/usr/bin/env".


[Footnote]

*1* Here a "variant" of a $language refers to a binary of one
particular version of the $language, together with libs and
extensions it uses that are installed on the system.  You
apparently have two variants, one installed as /usr/bin/perl,
the other as /usr/local/bin/perl.