Re: tools for easily "uncommitting" parts of a patch I just commited?

[Jeff King]

On Sun, Oct 23, 2016 at 08:23:01AM +0700, Duy Nguyen wrote:

> I hit the same problem sometimes, but in my case sometimes I
> accidentally do "git add" after "git add -p" and a configuration in
> "git commit -a" won't help me. I'd prefer we could undo changes in
> index instead. Something like reflog but for index.

An index write always writes the whole file from scratch, so you really
just need to save a copy of the old file. Perhaps something like:

  rm -f $GIT_DIR/index.old
  ln $GIT_DIR/index.old $GIT_DIR/index
  ... and then open $GIT_DIR/index.tmp ...
  ... and then rename(index.tmp, index) ...

could do it cheaply. It's a little more complicated if you want to save
a sequence of versions, and eventually would take a lot of space, but
presumably a handful of saved indexes would be sufficient.

Another option would be an index format that journals, and you could
potentially walk back the journal to a point. That seems like a much
bigger change (and has weird layering, because deciding when to fold in
the journal is usually a performance thing, but obviously this would
have user-visible impact about how far back you could undo).

-Peff

Re: tools for easily "uncommitting" parts of a patch I just commited?

[Duy Nguyen]

On Sat, Oct 22, 2016 at 4:19 PM, Lukas Fleischer  wrote:
> On Thu, 20 Oct 2016 at 19:27:58, Jacob Keller wrote:
>> [...]
>> I still think we're misunderstanding. I want git commit to complain
>> *only* under the following circumstance:
>>
>> I run "git add -p" and put a partial change into the index in .
>> There are still other parts which were not added to the index yet.
>> Thus, the index version of the file and the actual file differ.
>>
>> Then, I (accidentally) run "git commit "
>> [...]
>
> This reminded me of something that bothered me for a while. It's not
> 100% on-topic but still quite related so I thought I'd bring it up.
>
> When working on a feature, I usually try to make atomic changes from the
> beginning and use `git commit -a` to commit them one after another. This
> works fine most of the time. Sometimes I notice only after making some
> changes that it might be better to split the working tree changes into
> several commits.
>
> In that case, I git-add the relevant hunks and then, unfortunately, I
> often run `git commit -a` instead of `git commit` (muscle memory bites
> me), so I need to do all the splitting work again.
>
> It's not much of an issue but would it be worthwhile to add an optional
> feature (configurable) that warns you when using --all with staged
> changes (which are not new files)? Are there others having the same
> issue? Do you think this should be implemented as part of an alias
> instead?

I hit the same problem sometimes, but in my case sometimes I
accidentally do "git add" after "git add -p" and a configuration in
"git commit -a" won't help me. I'd prefer we could undo changes in
index instead. Something like reflog but for index.
-- 
Duy

Re: tools for easily "uncommitting" parts of a patch I just commited?

[Jacob Keller]

On Sat, Oct 22, 2016 at 2:19 AM, Lukas Fleischer  wrote:
> On Thu, 20 Oct 2016 at 19:27:58, Jacob Keller wrote:
>> [...]
>> I still think we're misunderstanding. I want git commit to complain
>> *only* under the following circumstance:
>>
>> I run "git add -p" and put a partial change into the index in .
>> There are still other parts which were not added to the index yet.
>> Thus, the index version of the file and the actual file differ.
>>
>> Then, I (accidentally) run "git commit "
>> [...]
>
> This reminded me of something that bothered me for a while. It's not
> 100% on-topic but still quite related so I thought I'd bring it up.
>
> When working on a feature, I usually try to make atomic changes from the
> beginning and use `git commit -a` to commit them one after another. This
> works fine most of the time. Sometimes I notice only after making some
> changes that it might be better to split the working tree changes into
> several commits.
>
> In that case, I git-add the relevant hunks and then, unfortunately, I
> often run `git commit -a` instead of `git commit` (muscle memory bites
> me), so I need to do all the splitting work again.
>
> It's not much of an issue but would it be worthwhile to add an optional
> feature (configurable) that warns you when using --all with staged
> changes (which are not new files)? Are there others having the same
> issue? Do you think this should be implemented as part of an alias
> instead?
>
> Regards,
> Lukas

This is (essentially) what I am asking for above. It's the same
overall problem of "muscle memory bites me" and I want the tool to
change to help avoiding because I don't think I can win the fight
against muscle memory every time. Being configurable would ensure that
only those that want the behavior opt in.

Thanks,
Jake

[PATCH 24/36] attr.c: always pass check[] to collect_some_attrs()

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

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

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

[Stefan Beller]

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

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

[PATCH 33/36] pathspec: allow escaped query values

[Stefan Beller]

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

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

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

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

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

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

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

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

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

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

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

[PATCH 34/36] submodule update: add `--init-default-path` switch

[Stefan Beller]

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

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

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

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

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

[PATCH 32/36] pathspec: allow querying for attributes

[Stefan Beller]

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

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 Documentation/glossary-content.txt |  20 +
 dir.c  |  35 
 pathspec.c | 103 +-
 pathspec.h |  16 
 t/t6134-pathspec-with-labels.sh| 170 +
 5 files changed, 340 insertions(+), 4 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

diff --git a/Documentation/glossary-content.txt 
b/Documentation/glossary-content.txt
index 8ad29e6..f90bd45 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -384,6 +384,26 @@ full pathname may have special meaning:
 +
 Glob magic is incompatible with literal magic.
 
+attr;;
+After `attr:` comes a space separated list of "attribute
+requirements", all of which must be met in order for the
+path to be considered a match; this is in addition to the
+usual non-magic pathspec pattern matching.
++
+Each of the attribute requirements for the path takes one of
+these forms:
+
+- "`ATTR`" requires that the attribute `ATTR` must be set.
+
+- "`-ATTR`" requires that the attribute `ATTR` must be unset.
+
+- "`ATTR=VALUE`" requires that the attribute `ATTR` must be
+  set to the string `VALUE`.
+
+- "`!ATTR`" requires that the attribute `ATTR` must be
+  unspecified.
++
+
 exclude;;
After a path matches any non-exclude pathspec, it will be run
through all exclude pathspec (magic signature: `!`). If it
diff --git a/dir.c b/dir.c
index 3bad1ad..3ec9117 100644
--- a/dir.c
+++ b/dir.c
@@ -9,6 +9,7 @@
  */
 #include "cache.h"
 #include "dir.h"
+#include "attr.h"
 #include "refs.h"
 #include "wildmatch.h"
 #include "pathspec.h"
@@ -207,6 +208,37 @@ int within_depth(const char *name, int namelen,
return 1;
 }
 
+static int match_attrs(const char *name, int namelen,
+  const struct pathspec_item *item)
+{
+   int i;
+   struct git_attr_result *res = git_attr_result_alloc(item->attr_check);
+
+   git_check_attr(name, item->attr_check, res);
+   for (i = 0; i < item->attr_match_nr; i++) {
+   const char *value;
+   int matched;
+   enum attr_match_mode match_mode;
+
+   value = res[i].value;
+   match_mode = item->attr_match[i].match_mode;
+
+   if (ATTR_TRUE(value))
+   matched = (match_mode == MATCH_SET);
+   else if (ATTR_FALSE(value))
+   matched = (match_mode == MATCH_UNSET);
+   else if (ATTR_UNSET(value))
+   matched = (match_mode == MATCH_UNSPECIFIED);
+   else
+   matched = (match_mode == MATCH_VALUE &&
+  !strcmp(item->attr_match[i].value, value));
+   if (!matched)
+   return 0;
+   }
+
+   return 1;
+}
+
 #define DO_MATCH_EXCLUDE   1
 #define DO_MATCH_DIRECTORY 2
 
@@ -262,6 +294,9 @@ static int match_pathspec_item(const struct pathspec_item 
*item, int prefix,
strncmp(item->match, name - prefix, item->prefix))
return 0;
 
+   if (item->attr_match_nr && !match_attrs(name, namelen, item))
+   return 0;
+
/* If the match was just the prefix, we matched */
if (!*match)
return MATCHED_RECURSIVELY;
diff --git a/pathspec.c b/pathspec.c
index d44f8e7..0eee177 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "dir.h"
 #include "pathspec.h"
+#include "attr.h"
 
 /*
  * Finds which of the given pathspecs match items in the index.
@@ -88,12 +89,78 @@ static void prefix_short_magic(struct strbuf *sb, int 
prefixlen,
strbuf_addf(sb, ",prefix:%d)", prefixlen);
 }
 
+static void parse_pathspec_attr_match(struct pathspec_item *item, const char 
*value)
+{
+   struct string_list_item *si;
+   struct string_list list = STRING_LIST_INIT_DUP;
+
+
+   if (!value || !strlen(value))
+   die(_("attr spec must not be empty"));
+
+   string_list_split(, value, ' ', -1);
+   string_list_remove_empty_items(, 0);
+
+   if (!item->attr_check)
+   git_attr_check_alloc(>attr_check);
+   else
+   die(_("Only one 'attr:' specification is allowed."));
+
+   ALLOC_GROW(item->attr_match, item->attr_match_nr + list.nr, 
item->attr_match_alloc);
+
+   for_each_string_list_item(si, ) {
+   size_t attr_len;
+
+   int j = item->attr_match_nr++;
+   const char *attr = si->string;
+   struct attr_match *am = >attr_match[j];
+
+   

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

[Stefan Beller]

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

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

[PATCH 25/36] attr.c: outline the future plans by heavily commenting

[Stefan Beller]

From: Junio C Hamano 

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

[PATCH 20/36] attr.c: pass struct git_attr_check down the callchain

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

[PATCH 23/36] attr.c: introduce empty_attr_check_elems()

[Stefan Beller]

From: Junio C Hamano 

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

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

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

[PATCH 22/36] attr.c: correct ugly hack for git_all_attrs()

[Stefan Beller]

From: Junio C Hamano 

The collect_some_attrs() function has an ugly hack since

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

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

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

[PATCH 18/36] attr: support quoting pathname patterns in C style

[Stefan Beller]

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

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

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

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

[PATCH 26/36] attr: make git_check_attr_counted static

[Stefan Beller]

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

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

diff --git a/attr.c b/attr.c
index 0f08ee6..881bdfa 100644
--- a/attr.c
+++ b/attr.c
@@ -892,8 +892,8 @@ void git_attr_set_direction(enum git_attr_direction new, 
struct index_state *ist
use_index = istate;
 }
 
-int git_check_attr_counted(const char *path, int pathlen,
-  struct git_attr_check *check)
+static int git_check_attr_counted(const char *path, int pathlen,
+ struct git_attr_check *check)
 {
check->finalized = 1;
return git_check_attrs(path, pathlen, check);
diff --git a/attr.h b/attr.h
index 40abc16..06ac93b 100644
--- a/attr.h
+++ b/attr.h
@@ -44,7 +44,6 @@ struct git_attr_check {
 
 extern struct git_attr_check *git_attr_check_initl(const char *, ...);
 extern int git_check_attr(const char *path, struct git_attr_check *);
-extern int git_check_attr_counted(const char *, int, struct git_attr_check *);
 
 extern struct git_attr_check *git_attr_check_alloc(void);
 extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check 
*, const struct git_attr *);
-- 
2.10.1.508.g6572022

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

[PATCH 31/36] pathspec: move prefix check out of the inner loop

[Stefan Beller]

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

[PATCH 29/36] Documentation: fix a typo

[Stefan Beller]

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

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

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

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

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

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

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

[PATCH 17/36] attr: expose validity check for attribute names

[Stefan Beller]

From: Junio C Hamano 

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

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

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

diff --git a/attr.c b/attr.c
index 90dbacd..659dc41 100644
--- a/attr.c
+++ b/attr.c
@@ -59,23 +59,38 @@ static unsigned hash_name(const char *name, int namelen)
return val;
 }
 
-static int invalid_attr_name(const char *name, int namelen)
+int attr_name_valid(const char *name, size_t namelen)
 {
/*
 * Attribute name cannot begin with '-' and must consist of
 * characters from [-A-Za-z0-9_.].
 */
if (namelen <= 0 || *name == '-')
-   return -1;
+   return 0;
while (namelen--) {
char ch = *name++;
if (! (ch == '-' || ch == '.' || ch == '_' ||
   ('0' <= ch && ch <= '9') ||
   ('a' <= ch && ch <= 'z') ||
   ('A' <= ch && ch <= 'Z')) )
-   return -1;
+   return 0;
}
-   return 0;
+   return 1;
+}
+
+void invalid_attr_name_message(struct strbuf *err, const char *name, int len)
+{
+   strbuf_addf(err, _("%.*s is not a valid attribute name"),
+   len, name);
+}
+
+static void report_invalid_attr(const char *name, size_t len,
+   const char *src, int lineno)
+{
+   struct strbuf err = STRBUF_INIT;
+   invalid_attr_name_message(, name, len);
+   fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno);
+   strbuf_release();
 }
 
 struct git_attr *git_attr_counted(const char *name, size_t len)
@@ -90,7 +105,7 @@ struct git_attr *git_attr_counted(const char *name, size_t 
len)
return a;
}
 
-   if (invalid_attr_name(name, len))
+   if (!attr_name_valid(name, len))
return NULL;
 
FLEX_ALLOC_MEM(a, name, name, len);
@@ -176,17 +191,15 @@ static const char *parse_attr(const char *src, int 
lineno, const char *cp,
cp++;
len--;
}
-   if (invalid_attr_name(cp, len)) {
-   fprintf(stderr,
-   "%.*s is not a valid attribute name: %s:%d\n",
-   len, cp, src, lineno);
+   if (!attr_name_valid(cp, len)) {
+   report_invalid_attr(cp, len, src, lineno);
return NULL;
}
} else {
/*
 * As this function is always called twice, once with
 * e == NULL in the first pass and then e != NULL in
-* the second pass, no need for invalid_attr_name()
+* the second pass, no need for attr_name_valid()
 * check here.
 */
if (*cp == '-' || *cp == '!') {
@@ -229,10 +242,8 @@ static struct match_attr *parse_attr_line(const char 
*line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
-   if (invalid_attr_name(name, namelen)) {
-   fprintf(stderr,
-   "%.*s is not a valid attribute name: %s:%d\n",
-   namelen, name, src, lineno);
+   if (!attr_name_valid(name, namelen)) {
+   report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
}
diff --git a/attr.h b/attr.h
index bcedf92..40abc16 100644
--- a/attr.h
+++ b/attr.h
@@ -13,6 +13,9 @@ extern struct git_attr *git_attr(const char *);
 /* The same, but with counted string */
 extern struct git_attr *git_attr_counted(const char *, size_t);
 
+extern int attr_name_valid(const char *name, size_t namelen);
+extern void invalid_attr_name_message(struct strbuf *, const char *, int);
+
 /* Internal use */
 extern const char git_attr__true[];
 extern const char git_attr__false[];
-- 
2.10.1.508.g6572022

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

[PATCH 16/36] attr: add counted string version of git_attr()

[Stefan Beller]

From: Junio C Hamano 

Often a potential caller has  pair that
represents the name it wants to create an attribute out of.

When name[namelen] is not NUL, the caller has to xmemdupz()
only to call git_attr().

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

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

diff --git a/attr.c b/attr.c
index 9bec243..90dbacd 100644
--- a/attr.c
+++ b/attr.c
@@ -78,7 +78,7 @@ static int invalid_attr_name(const char *name, int namelen)
return 0;
 }
 
-static struct git_attr *git_attr_internal(const char *name, int len)
+struct git_attr *git_attr_counted(const char *name, size_t len)
 {
unsigned hval = hash_name(name, len);
unsigned pos = hval % HASHSIZE;
@@ -109,7 +109,7 @@ static struct git_attr *git_attr_internal(const char *name, 
int len)
 
 struct git_attr *git_attr(const char *name)
 {
-   return git_attr_internal(name, strlen(name));
+   return git_attr_counted(name, strlen(name));
 }
 
 /* What does a matched pattern decide? */
@@ -199,7 +199,7 @@ static const char *parse_attr(const char *src, int lineno, 
const char *cp,
else {
e->setto = xmemdupz(equals + 1, ep - equals - 1);
}
-   e->attr = git_attr_internal(cp, len);
+   e->attr = git_attr_counted(cp, len);
}
return ep + strspn(ep, blank);
 }
@@ -254,7 +254,7 @@ static struct match_attr *parse_attr_line(const char *line, 
const char *src,
  sizeof(struct attr_state) * num_attr +
  (is_macro ? 0 : namelen + 1));
if (is_macro) {
-   res->u.attr = git_attr_internal(name, namelen);
+   res->u.attr = git_attr_counted(name, namelen);
res->u.attr->maybe_macro = 1;
} else {
char *p = (char *)&(res->state[num_attr]);
diff --git a/attr.h b/attr.h
index c84f164..bcedf92 100644
--- a/attr.h
+++ b/attr.h
@@ -8,7 +8,10 @@ struct git_attr;
  * Given a string, return the gitattribute object that
  * corresponds to it.
  */
-struct git_attr *git_attr(const char *);
+extern struct git_attr *git_attr(const char *);
+
+/* The same, but with counted string */
+extern struct git_attr *git_attr_counted(const char *, size_t);
 
 /* Internal use */
 extern const char git_attr__true[];
-- 
2.10.1.508.g6572022

[PATCH 19/36] attr.c: add push_stack() helper

[Stefan Beller]

From: Junio C Hamano 

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

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

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

[PATCH 28/36] attr: keep attr stack for each check

[Stefan Beller]

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



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

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

[PATCH 21/36] attr.c: rename a local variable check

[Stefan Beller]

From: Junio C Hamano 

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

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

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

[PATCH 30/36] pathspec: move long magic parsing out of prefix_pathspec

[Stefan Beller]

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

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

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

[PATCH 27/36] attr: convert to new threadsafe API

[Stefan Beller]

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

static struct git_attr_check *check;

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

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

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

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

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

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

The usage of the new API will be:

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

/*
 * Obtain a pointer to a correctly sized result
 * statically allocated on the stack; this macro:
 */
GIT_ATTR_RESULT_INIT_FOR(myresult, 1);

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

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

Signed-off-by: Stefan Beller 
---
 Documentation/technical/api-gitattributes.txt |  91 +---
 archive.c |  14 +--
 attr.c| 143 ++
 attr.h|  71 -
 builtin/check-attr.c  |  35 ---
 builtin/pack-objects.c|  16 +--
 convert.c |  39 +++
 ll-merge.c|  25 +++--
 userdiff.c|  16 +--
 ws.c  |  17 ++-
 10 files changed, 279 insertions(+), 188 deletions(-)

diff --git a/Documentation/technical/api-gitattributes.txt 
b/Documentation/technical/api-gitattributes.txt
index 92fc32a..f3fc7bd 100644
--- a/Documentation/technical/api-gitattributes.txt
+++ b/Documentation/technical/api-gitattributes.txt
@@ -16,15 +16,17 @@ Data Structure
of no interest to the calling programs.  The name of the
attribute can be retrieved by calling `git_attr_name()`.
 
-`struct git_attr_check_elem`::
-
-   This structure represents one attribute and its value.
-
 `struct git_attr_check`::
 
-   This structure represents a collection of `git_attr_check_elem`.
+   This structure represents a collection of `struct git_attrs`.
It is passed to `git_check_attr()` function, specifying the
-   attributes to check, and receives their values.
+   attributes to check, and receives their values into a corresponding
+   `struct git_attr_result`.
+
+`struct git_attr_result`::
+
+   This structure represents a collection of results to its
+   corresponding `struct git_attr_check`, that has the same order.
 
 
 Attribute Values
@@ -32,7 +34,7 @@ Attribute Values
 
 An attribute for a path can be in one of four states: Set, Unset,
 Unspecified or set to a string, and `.value` member of `struct
-git_attr_check` records it.  There are three macros to check these:
+git_attr_result` records it.  There are three macros to check these:
 
 `ATTR_TRUE()`::
 
@@ -53,19 +55,31 @@ value of the attribute for the path.
 Querying Specific Attributes
 
 
-* Prepare `struct git_attr_check` using git_attr_check_initl()
+* Prepare a `struct git_attr_check` using `git_attr_check_initl()`
   function, enumerating the names of attributes whose values you are
   interested in, terminated with a NULL pointer.  Alternatively, an
-  empty `struct git_attr_check` can be prepared by calling
-  

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

[Stefan Beller]

From: Junio C Hamano 

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

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

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

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

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

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

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

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

[Stefan Beller]

From: Junio C Hamano 

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

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

[PATCHv2 00/36] Revamping the attr subsystem!

[Stefan Beller]

previous discussion:
http://public-inbox.org/git/20161012224109.23410-1-sbel...@google.com
http://public-inbox.org/git/20161011002115.23312-1-sbel...@google.com/

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

* I rebase to origin master (no merge conflicts)
* I implemented the thread safe attr API in patch 27 (attr: convert to new 
threadsafe API)
* patch 28 (attr: keep attr stack for each check) makes it actually possible
  to run in a multithreaded environment.
* I added a test for the multithreaded when it is introduced in patch 32
  (pathspec: allow querying for attributes)

Thanks,
Stefan

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

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

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

 Documentation/config.txt  |   5 +
 Documentation/git-clone.txt   |  23 +-
 Documentation/git-submodule.txt   |  17 +-
 Documentation/gitattributes.txt   |  10 +-
 Documentation/glossary-content.txt|  20 +
 Documentation/technical/api-gitattributes.txt | 117 --
 archive.c |  26 +-
 attr.c| 530 ++
 attr.h|  74 +++-
 builtin/check-attr.c  |  65 ++--
 builtin/clone.c   |  36 +-
 builtin/pack-objects.c|  27 +-
 commit.c  |   3 +-
 contrib/completion/git-completion.bash|   1 +
 convert.c |  45 +--
 dir.c |  35 ++
 git-submodule.sh  |  21 +-
 hashmap.h |   2 +
 ll-merge.c|  36 +-
 pathspec.c| 225 +--
 pathspec.h|  16 +
 t/t0003-attributes.sh |  26 ++
 t/t6134-pathspec-with-labels.sh   | 180 +
 t/t7400-submodule-basic.sh| 134 +++
 userdiff.c|  21 +-
 ws.c  |  26 +-
 26 files changed, 1313 insertions(+), 408 deletions(-)
 create mode 100755 t/t6134-pathspec-with-labels.sh

-- 
2.10.1.508.g6572022

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

[Stefan Beller]

From: Junio C Hamano 

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

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

Stash pop/apply conflict and --theirs and --ours

[Sven Strickroth]

Hi,

I regularly experience that beginners have problems unterstanding that
--ours and --theirs are swapped when a conflict occurrs on git stash
apply or stash pop.

>From the HCI perspective this is really counter intuitive.

So, I'd like to propose that on git shash pop/apply theirs and ours
should be swapped in git index, so that git checkout --theirs and --ours
work as expected.

PS: I'm sorry if this was already discussed, I haven't found any discussion.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Stefan Beller]

On Sat, Oct 22, 2016 at 12:33 AM, Junio C Hamano  wrote:
> Johannes Sixt  writes:
>
>>> The logic to construct the relative urls is not smart enough to
>>> detect that the ending /. is referring to the directory itself
>>> but rather treats it like any other relative path, i.e.
>>>
>>> path/to/dir/. + ../relative/path/to/submodule
>>>
>>> would result in
>>>
>>> path/to/dir/relative/path/to/submodule
>>>
>>> and not omit the "dir" as you may expect.
>>>
>>> As in a later patch we'll normalize the remote url before the
>>> computation of relative urls takes place, we need to first get our
>>> test suite in a shape with normalized urls only, which is why we should
>>> refrain from cloning from '.'
>>
>> But you are removing a valid use case from the tests. Aren't you
>> sweeping something under the rug with this patch?
>
> I share the same reaction.

Oh I see. I agree.
I reverted the lines that replace . by "$(pwd)" and it still works, but it
still works because the code path regarding local paths was not broken
(both . as well as "$(pwd)" are working without the fix)

>
> If the primary problem being solved is that the combination of a
> relative URL ../sub and the base URL for the superproject which is
> set to /path/to/dir/. (due to "clone .") were incorrectly resolved
> as /path/to/dir/sub (because the buggy relative path logic did not
> know that removing "/." at the end does not take you to one level
> up), and a topic that fixes the bug would make that relative URL
> ../sub to be resolved as /path/to/sub, of course.  Otherwise, the
> topic did not fix the bug.
>
> Now if a test that wanted to have a clone of the superproject by
> "clone .", which results in the base URL of /path/to/dir/., actually
> wants to refer in its .gitmodules to /path/to/dir/sub (which after
> all was where the submodule the test created with or without the
> bugfix), I would think the right adjustment for the test that used
> to rely on the buggy behaviour would be to stop using ../sub and
> instead use ./sub as the relative URL, no?  After all, the bug forced
> the original test writer to write ../sub but the submodule in this
> case actually is at ./sub relative to its superproject, and that is
> what the original test writer would have written if the bug weren't
> there in the first place, no?

True.

I have looked into it again, and by now I think the bug is a feature,
actually.

Consider this:

git clone . super
git -C super submodule add ../submodule
# we thought the previous line is buggy
git clone super super-clone

Now in the super-clone the ../submodule is the correct
relative url, because the url where we cloned from doesn't
end in /.

If we were to fix the bug with the /. ending, then we would need the
following:

git clone . super
git -C super submodule add ./submodule
# correct relative URL above!

git clone super super-clone
cd superclone && sed s|url =./|url = ../|
# make sure we fix the relative URLs to account for a different remote.

And this doesn't seem right to me as it burdens the user of the super-clone.

>
> Another thing I do not quite understand is why this step comes
> before the fix.  If the "clone ." is adjusted to avoid triggering
> the buggy behaviour, i.e. making the base URL to /path/to/dir
> (instead of /path/to/dir/.), wouldn't the relative URL ../sub that
> was written to work around the bug that hasn't been fixed yet in
> this step need to be adjusted anyway?  It would end up referring to
> /path/to/sub and not /path/to/dir/sub until the fix is put in place.
>
> Is the removal of remote.origin.url a wrong workaround for that
> breakage, I wonder...  I do not understand that change at all, and I
> do not think it was explained in the log message.

I think it is wrong, because it is sidestepping the actual issue.
Continuing from above:

git clone super-clone super-clone2
# this works again, as the remote change doesn't matter.

mkdir test && git -C test clone ../ .
# submodule urls need to be "undone again to work:
cd test && sed s|url =../|url = ./|

So I think keeping the /. around as it currently is, is a pretty
useful bug.

>
> If we really wanted to update the test before fixing the bug, I
> would understand if this step changed ../sub (or whatever relative
> URL that has extra ../ only because the base URL has extra /. at the
> end to compensate for the buggy resolution) to ./sub in the tests
> and marked them to expect failure, and then a later step that fixes
> the bug turns them to expect success without make any other change.

I'll think about this further, but currently I am inclined to declare
it a nonbug
and as it eases testing a lot. Also if someone wants to clone a repository
with broken relative urls, they can work around that by e.g.

git clone /.

to fix one level of indentation, though it is not documented and seems
to be brittle.

Thanks,
Stefan

Re: [PATCH v5 00/27] Prepare the sequencer for the upcoming rebase -i patches

[Junio C Hamano]

Johannes Schindelin  writes:

> This patch series marks the '4' in the countdown to speed up rebase -i
> by implementing large parts in C (read: there will be three more patch
> series after that before the full benefit hits git.git: sequencer-i,
> rebase--helper and rebase-i-extra).
> ...
> It would be *really* nice if we could get this patch series at least into
> `next` soon, as it gets late and later for the rest of the patches to make
> it into `master` in time for v2.11 (and it is not for lack of trying on my
> end...).

This "countdown 4" step can affect cherry-pick and revert, even
though we were careful to review changes to the sequencer.c code.  I
prefer to cook it in 'next' sufficiently long to ensure that we hear
feedbacks from non-Windows users if there is any unexpected breakage.

There isn't enough time to include this topic in the upcoming
release within the current https://tinyurl.com/gitCal calendar,
however, which places the final on Nov 11th.

I am wondering if it makes sense to delay 2.11 by moving the final
by 4 weeks to Dec 9th.

Thoughts?

Speaking of what to and not to include in the upcoming release, we
do want to include Stefan's off-by-one fix to the submodule-helper,
but that is blocked on Windows end due to the test.  I think
everybody agreed that a longer time "right thing to do" fix is to
address the "when base is /path/to/dir/., where is ../sub relative
to it?" issue, but if we are to do so, it would need a longer
gestation period once it hits 'next', as it can affect the current
users and we may even need B/C notes in the release notes for the
change.  Giving ourselves a few more weeks of breathing room would
help us to make sure the fix to relative URL issue is sound, too.

As to "countdown 3" and below steps, I am guessing that some of them
can start cooking in 'next' before 2.11, but even with lengthened
schedule, it is likely that they need to cook there beyond the end
of this cycle, unless they are truly trivial changes that do not
even need any reviews.

Thanks.

Re: [PATCH] daemon: detect and reject too-long paths

[Junio C Hamano]

Jeff King  writes:

> When we are checking the path via path_ok(), we use some
> fixed PATH_MAX buffers. We write into them via snprintf(),
> so there's no possibility of overflow, but it does mean we
> may silently truncate the path, leading to potentially
> confusing errors when the partial path does not exist.
>
> We're better off to reject the path explicitly.
>
> Signed-off-by: Jeff King 
> ---

Sounds sensible.

> Another option would be to switch to strbufs here. That potentially
> introduces cases where a client can convince us to just keep allocating
> memory, but I don't think so in practice; the paths and interpolated
> data items all have to come in 64K pkt-lines, which places a hard
> limit. This is a much more minimal change, though, and I don't hear
> anybody complaining about the inability to use large paths.

The alternative version did not look bad, either; in fact, the end
result may even be conceptually simpler.

But I agree that this one with the same hard-limit we always had is
a much more minimal change and is sufficient.

Thanks.

>  daemon.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/daemon.c b/daemon.c
> index 425aad0507..ff0fa583b0 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct 
> hostinfo *hi)
>  {
>   static char rpath[PATH_MAX];
>   static char interp_path[PATH_MAX];
> + size_t rlen;
>   const char *path;
>   const char *dir;
>  
> @@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct 
> hostinfo *hi)
>   namlen = slash - dir;
>   restlen -= namlen;
>   loginfo("userpath <%s>, request <%s>, namlen %d, 
> restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
> - snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
> -  namlen, dir, user_path, restlen, slash);
> + rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
> + namlen, dir, user_path, restlen, slash);
> + if (rlen >= sizeof(rpath)) {
> + logerror("user-path too large: %s", rpath);
> + return NULL;
> + }
>   dir = rpath;
>   }
>   }
> @@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct 
> hostinfo *hi)
>  
>   strbuf_expand(_path, interpolated_path,
> expand_path, );
> - strlcpy(interp_path, expanded_path.buf, PATH_MAX);
> +
> + rlen = strlcpy(interp_path, expanded_path.buf,
> +sizeof(interp_path));
> + if (rlen >= sizeof(interp_path)) {
> + logerror("interpolated path too large: %s",
> +  interp_path);
> + return NULL;
> + }
> +
>   strbuf_release(_path);
>   loginfo("Interpolated dir '%s'", interp_path);
>  
> @@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct 
> hostinfo *hi)
>   logerror("'%s': Non-absolute path denied (base-path 
> active)", dir);
>   return NULL;
>   }
> - snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
> + rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
> + if (rlen >= sizeof(rpath)) {
> + logerror("base-path too large: %s", rpath);
> + return NULL;
> + }
>   dir = rpath;
>   }

Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically

[Junio C Hamano]

Jeff King  writes:

> On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
>
>> And this is the final one.
>> 
>> -- >8 --
>> From: Junio C Hamano 
>> Date: Fri, 21 Oct 2016 21:33:06 -0700
>> Subject: [PATCH] transport: compute summary-width dynamically
>> 
>> Now all that is left to do is to actually iterate over the refs
>> and measure the display width needed to show their abbreviation.
>
> I think we crossed emails. :) This is obviously correct, if we don't
> mind paying the find_unique_abbrev cost twice for each sha1.

Indeed we did.  I do not think the cost matters that much in the
codepath to produce the final summary output.

> This is a minor style nit, but I think it's better to avoid mixing
> unrelated bits between the initialization, condition, and iteration bits
> of a for loop.

Yeah, you're right.

Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer

[Junio C Hamano]

Christian Couder  writes:

> On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano  wrote:
>>
>> If I were guiding a topic that introduce this feature from scratch
>> today, I would probably suggest a pattern based approach, e.g.  a
>> built-in "[-A-Za-z0-9]+:" [*1*] may be the default prefix that is
>> used to recognize the beginning of a trailer, and a user or a
>> project that wants "Bug #538" would be allowed to add an additional
>> pattern, e.g. "Bug *#", that recognises a custom trailer line that
>> is used by the project.
>
> When we designed the separator mechanism, we had the following discussions:
>
> https://public-inbox.org/git/xmqqa9a1d6xn@gitster.dls.corp.google.com/
> https://public-inbox.org/git/xmqqmwcuzyqx@gitster.dls.corp.google.com/
>
> They made me think that you were against too much flexibility, so I
> removed functionality that allowed to put separators into the ".key"
> config options, and now you are saying that we botched the thing and
> that you would like more flexibility of this kind back.

Correct.  Pay attention to the fact that I said _we_ botched.

If an initial design made by a topic author is crappy, that may be
author's botch.  Once a topic goes through a review cycle by getting
reviewed, rerolled, re-reviewed, ... to the point that those
involved accept the result, and we later realize that it was not
good, the botch no longer is author's alone.  If it is shipped as
part of a release, then it is not just the authors and the reviewers
but everybody.  We collectively stopped at a place that was not
ideal and share the blame ;-).

> Anyway I think it is still possible to add back such kind of
> functionality in a backward compatible way for example by adding
> ".extendedKey" config options.

Yup, or with trailer.keyPattern that is multi-values, or with any
number of alternatives.

Contact Mr.Alex Morgan For Your payment (2.1M USD

[George Fred]

Attention,

Be informed that We have concluded to effect your payment (2.1M USD) Two 
Million One Hundred Thousand Dollars through Western Union Money Transfer. But 
the maximum amount you will be receiving daily starting from tomorrow onward is 
5,100.00USD until the funds is completely paid out to you.

Now, contact Western Union Agent:  Alex Morgan with your : Full Names , 
address, Telephone number. E-mail him at :( wire.file...@gmail.com  
)Tele:+2348163253958 Though, 5,100.00 USD has been sent in your name today so 
contact Mr.Alex Morgan and tell him to give you the MTCN, Sender Name, 
Question/Answer to pick the 5,100.00 USD. Please let me know as soon as you 
received all your funds (2.1M USD) Two Million One Hundred Thousand Dollars.

Thank you.
George Fred

Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer

[Christian Couder]

On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano  wrote:
> Jonathan Tan  writes:
>
>> That is true - I think we can take the allowed separators as an
>> argument (meaning that we can have different behavior for file parsing
>> and command line parsing), and since we already have that string, we
>> can use strcspn. I'll try this out in the next reroll.
>
> Sounds good.  Thanks.
>
>
> The following is a tangent that I think this topic should ignore,
> but we may want to revisit it sometime later.
>
> I think the design of the "separator" mechanism is one of the things
> we botched in the current system.  If I recall correctly, this was
> introduced to allow people write "Bug# 538" in the trailer section
> and get it recognised as a valid trailer.
>
> When I say that this was a botched design, I do not mean to say that
> we should have instead forced projects to adopt "Bug: 538" format.
> The design is botched because the users' wish to allow "Bug# 538" or
> "Bug #538" by setting separators to ":#" from the built-in ":" does
> not mean that they would want "Signed-off-by# me " to
> be accepted.
>
> If I were guiding a topic that introduce this feature from scratch
> today, I would probably suggest a pattern based approach, e.g.  a
> built-in "[-A-Za-z0-9]+:" [*1*] may be the default prefix that is
> used to recognize the beginning of a trailer, and a user or a
> project that wants "Bug #538" would be allowed to add an additional
> pattern, e.g. "Bug *#", that recognises a custom trailer line that
> is used by the project.

When we designed the separator mechanism, we had the following discussions:

https://public-inbox.org/git/xmqqa9a1d6xn@gitster.dls.corp.google.com/
https://public-inbox.org/git/xmqqmwcuzyqx@gitster.dls.corp.google.com/

They made me think that you were against too much flexibility, so I
removed functionality that allowed to put separators into the ".key"
config options, and now you are saying that we botched the thing and
that you would like more flexibility of this kind back.

Anyway I think it is still possible to add back such kind of
functionality in a backward compatible way for example by adding
".extendedKey" config options.

Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer

[Christian Couder]

On Fri, Oct 21, 2016 at 12:45 AM, Junio C Hamano  wrote:
> Jonathan Tan  writes:
>
>> If we do that, there is also the necessity of creating a string that
>> combines the separators and '=' (I guess '\n' is not necessary now,
>> since all the lines are null terminated). I'm OK either way.
>>
>> (We could cache that string, although I would think that if we did
>> that, we might as well write the loop manually, like in this patch.)
>
> I wonder if there is a legit reason to look for '=' in the first
> place.  "Signed-off-by= Jonathan Tan " does not look
> like a valid trailer line to me.
>
> Isn't that a remnant of lazy coding in the original that tried to
> share a single parser for contents and command line options or
> something?

I think the relevant discussion was this one:

https://public-inbox.org/git/20140915.080429.1739849931027469667.chrisc...@tuxfamily.org/

Re: tools for easily "uncommitting" parts of a patch I just commited?

[Lukas Fleischer]

On Thu, 20 Oct 2016 at 19:27:58, Jacob Keller wrote:
> [...]
> I still think we're misunderstanding. I want git commit to complain
> *only* under the following circumstance:
> 
> I run "git add -p" and put a partial change into the index in .
> There are still other parts which were not added to the index yet.
> Thus, the index version of the file and the actual file differ.
> 
> Then, I (accidentally) run "git commit "
> [...]

This reminded me of something that bothered me for a while. It's not
100% on-topic but still quite related so I thought I'd bring it up.

When working on a feature, I usually try to make atomic changes from the
beginning and use `git commit -a` to commit them one after another. This
works fine most of the time. Sometimes I notice only after making some
changes that it might be better to split the working tree changes into
several commits.

In that case, I git-add the relevant hunks and then, unfortunately, I
often run `git commit -a` instead of `git commit` (muscle memory bites
me), so I need to do all the splitting work again.

It's not much of an issue but would it be worthwhile to add an optional
feature (configurable) that warns you when using --all with staged
changes (which are not new files)? Are there others having the same
issue? Do you think this should be implemented as part of an alias
instead?

Regards,
Lukas

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Junio C Hamano]

Johannes Sixt  writes:

>> The logic to construct the relative urls is not smart enough to
>> detect that the ending /. is referring to the directory itself
>> but rather treats it like any other relative path, i.e.
>>
>> path/to/dir/. + ../relative/path/to/submodule
>>
>> would result in
>>
>> path/to/dir/relative/path/to/submodule
>>
>> and not omit the "dir" as you may expect.
>>
>> As in a later patch we'll normalize the remote url before the
>> computation of relative urls takes place, we need to first get our
>> test suite in a shape with normalized urls only, which is why we should
>> refrain from cloning from '.'
>
> But you are removing a valid use case from the tests. Aren't you
> sweeping something under the rug with this patch?

I share the same reaction.

If the primary problem being solved is that the combination of a
relative URL ../sub and the base URL for the superproject which is
set to /path/to/dir/. (due to "clone .") were incorrectly resolved
as /path/to/dir/sub (because the buggy relative path logic did not
know that removing "/." at the end does not take you to one level
up), and a topic that fixes the bug would make that relative URL
../sub to be resolved as /path/to/sub, of course.  Otherwise, the
topic did not fix the bug.  

Now if a test that wanted to have a clone of the superproject by
"clone .", which results in the base URL of /path/to/dir/., actually
wants to refer in its .gitmodules to /path/to/dir/sub (which after
all was where the submodule the test created with or without the
bugfix), I would think the right adjustment for the test that used
to rely on the buggy behaviour would be to stop using ../sub and
instead use ./sub as the relative URL, no?  After all, the bug forced
the original test writer to write ../sub but the submodule in this
case actually is at ./sub relative to its superproject, and that is
what the original test writer would have written if the bug weren't
there in the first place, no?

Another thing I do not quite understand is why this step comes
before the fix.  If the "clone ." is adjusted to avoid triggering
the buggy behaviour, i.e. making the base URL to /path/to/dir
(instead of /path/to/dir/.), wouldn't the relative URL ../sub that
was written to work around the bug that hasn't been fixed yet in
this step need to be adjusted anyway?  It would end up referring to
/path/to/sub and not /path/to/dir/sub until the fix is put in place.

Is the removal of remote.origin.url a wrong workaround for that
breakage, I wonder...  I do not understand that change at all, and I
do not think it was explained in the log message.

If we really wanted to update the test before fixing the bug, I
would understand if this step changed ../sub (or whatever relative
URL that has extra ../ only because the base URL has extra /. at the
end to compensate for the buggy resolution) to ./sub in the tests
and marked them to expect failure, and then a later step that fixes
the bug turns them to expect success without make any other change.

Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"

[Johannes Sixt]

Am 22.10.2016 um 01:59 schrieb Stefan Beller:

When adding a submodule via "git submodule add ",
the relative url applies to the superprojects remote. When the
superproject was cloned via "git clone . super", the remote url
is ending with '/.'.

The logic to construct the relative urls is not smart enough to
detect that the ending /. is referring to the directory itself
but rather treats it like any other relative path, i.e.

path/to/dir/. + ../relative/path/to/submodule

would result in

path/to/dir/relative/path/to/submodule

and not omit the "dir" as you may expect.

As in a later patch we'll normalize the remote url before the
computation of relative urls takes place, we need to first get our
test suite in a shape with normalized urls only, which is why we should
refrain from cloning from '.'


But you are removing a valid use case from the tests. Aren't you 
sweeping something under the rug with this patch?




Signed-off-by: Stefan Beller 
---
 t/t7064-wtstatus-pv2.sh  | 9 ++---
 t/t7403-submodule-sync.sh| 3 ++-
 t/t7406-submodule-update.sh  | 6 --
 t/t7407-submodule-foreach.sh | 3 ++-
 t/t7506-status-submodule.sh  | 3 ++-
 5 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..95514bb 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
 test_expect_success 'verify upstream fields in branch header' '
git checkout master &&
test_when_finished "rm -rf sub_repo" &&
-   git clone . sub_repo &&
+   git clone "$(pwd)" sub_repo &&
+   git -C sub_repo config --unset remote.origin.url &&


Why is it necessary to remove this configuration? Is it because when it 
is present, the submodule does not construct its own reference? And if 
so, should it not be sufficient to only remove the configuration 
(without changing 'git clone' command), but move this patch after the 
patch that fixes the /. treatment?



(
## Confirm local master tracks remote master.
cd sub_repo &&

...