* Re: [DEMO][PATCH v2 6/5] compat: add a qsort_s() implementation based on GNU's qsort_r(1)
From: Junio C Hamano @ 2017-01-23 19:07 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Jeff King, Johannes Schindelin
In-Reply-To: <9f8b564d-ec9f-abc9-77f6-aa84c6e78b7a@web.de>
René Scharfe <l.s.r@web.de> writes:
> Implement qsort_s() as a wrapper to the GNU version of qsort_r(1) and
> use it on Linux. Performance increases slightly:
>
> Test HEAD^ HEAD
> --------------------------------------------------------------------
> 0071.2: sort(1) 0.10(0.20+0.02) 0.10(0.21+0.01) +0.0%
> 0071.3: string_list_sort() 0.17(0.15+0.01) 0.16(0.15+0.01) -5.9%
>
> Additionally the unstripped size of compat/qsort_s.o falls from 24576
> to 16544 bytes in my build.
>
> IMHO these savings aren't worth the increased complexity of having to
> support two implementations.
I do worry about having to support more implementations in the
future that have different function signature for the comparison
callbacks, which will make things ugly, but this addition alone
doesn't look too bad to me.
Thanks.
> diff --git a/compat/qsort_s.c b/compat/qsort_s.c
> index 52d1f0a73d..763ee1faae 100644
> --- a/compat/qsort_s.c
> +++ b/compat/qsort_s.c
> @@ -1,5 +1,21 @@
> #include "../git-compat-util.h"
>
> +#if defined HAVE_GNU_QSORT_R
> +
> +int git_qsort_s(void *b, size_t n, size_t s,
> + int (*cmp)(const void *, const void *, void *), void *ctx)
> +{
> + if (!n)
> + return 0;
> + if (!b || !cmp)
> + return -1;
> +
> + qsort_r(b, n, s, cmp, ctx);
> + return 0;
> +}
> +
> +#else
> +
> /*
> * A merge sort implementation, simplified from the qsort implementation
> * by Mike Haertel, which is a part of the GNU C Library.
> @@ -67,3 +83,5 @@ int git_qsort_s(void *b, size_t n, size_t s,
> }
> return 0;
> }
> +
> +#endif
> diff --git a/config.mak.uname b/config.mak.uname
> index 447f36ac2e..a1858f54ff 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -37,6 +37,7 @@ ifeq ($(uname_S),Linux)
> NEEDS_LIBRT = YesPlease
> HAVE_GETDELIM = YesPlease
> SANE_TEXT_GREP=-a
> + HAVE_GNU_QSORT_R = YesPlease
> endif
> ifeq ($(uname_S),GNU/kFreeBSD)
> HAVE_ALLOCA_H = YesPlease
^ permalink raw reply
* Re: [RFC] Case insensitive Git attributes
From: Junio C Hamano @ 2017-01-23 19:25 UTC (permalink / raw)
To: Lars Schneider
Cc: Dakota Hawkins, Duy Nguyen, Johannes Schindelin, Stefan Beller,
git
In-Reply-To: <C265204B-BCCF-4085-9933-F28EB459CFB9@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> Problem:
> Git attributes for path names are generally case sensitive. However, on
> a case insensitive file system (e.g. macOS/Windows) they appear to be
> case insensitive (`*.bar` would match `foo.bar` and `foo.BAR`). That
> works great until a Git users joins the party with a case sensitive file
> system. For this Git user the attributes pattern only matches files with
> the exact case (*.bar` would match only `foo.bar`).
>
> Question/Proposal:
> Could we introduce some flag to signal that certain attribute patterns
> are always case insensitive?
>
> Thread:
> http://public-inbox.org/git/C83BE22D-EAC8-49E2-AEE3-22D4A99AE205@gmail.com/#t
Thanks for a pointer.
So you are worried about the case where somebody on a case
insensitive but case preserving system would do
$ edit file.txt
$ edit .gitattributes
$ git add file.txt .gitattributes
and adds "*.TXT someattr=true" to the attributes file, which
would set someattr to true on his system for file.txt, but when the
result is checked out on a case sensitive system, it would behave
differently because "*.TXT" does not match "file.txt"?
How do other systems address it? Your Java, Ruby, etc. sources may
refer to another file with "import" and the derivation of the file
names from class names or package names would have the same issue,
isn't it? Do they have an option that lets you say
Even though the import statements may say "import a.b.C", we
know that the source tarball was prepared on a case insensitive
system, and I want you to look for a/b/C.java and a/b/c.java and
use what was found.
or something like that? Same for anything that records other
filenames in the content to refer to them, like "Makefile".
My knee jerk reaction to that is that .gitattributes and .gitignore
should not be instructed to go case insensitive on case sensitive
systems. If the system is case insensitive but case preserving,
it probably would make sense not to do case insensitive matching,
which would prevent the issue from happening in the first place.
^ permalink raw reply
* Re: [RFC] Case insensitive Git attributes
From: Junio C Hamano @ 2017-01-23 19:38 UTC (permalink / raw)
To: Lars Schneider
Cc: Dakota Hawkins, Duy Nguyen, Johannes Schindelin, Stefan Beller,
git
In-Reply-To: <xmqq1svtshnr.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> So you are worried about the case where somebody on a case
> insensitive but case preserving system would do
>
> $ edit file.txt
> $ edit .gitattributes
> $ git add file.txt .gitattributes
>
> and adds "*.TXT someattr=true" to the attributes file, which
> would set someattr to true on his system for file.txt, but when the
> result is checked out on a case sensitive system, it would behave
> differently because "*.TXT" does not match "file.txt"?
>
> How do other systems address it? Your Java, Ruby, etc. sources may
> refer to another file with "import" and the derivation of the file
> names from class names or package names would have the same issue,
> isn't it? Do they have an option that lets you say
>
> Even though the import statements may say "import a.b.C", we
> know that the source tarball was prepared on a case insensitive
> system, and I want you to look for a/b/C.java and a/b/c.java and
> use what was found.
>
> or something like that? Same for anything that records other
> filenames in the content to refer to them, like "Makefile".
>
> My knee jerk reaction to that is that .gitattributes and .gitignore
> should not be instructed to go case insensitive on case sensitive
> systems. If the system is case insensitive but case preserving,
> it probably would make sense not to do case insensitive matching,
> which would prevent the issue from happening in the first place.
Sorry, but there is a slight leap in the above that makes it hard to
track my thought, so let me clarify a bit.
In the above, I am guessing the answer to the "How do other systems
address it?" question to be "nothing". And that leads to the
conclusion that it is better to do "nothing on case sensitive
systems, and probably become evem more strict on case insensitive
but case preserving systems", because that will give us a chance to
expose the problem earlier, hopefully even on the originating
system.
^ permalink raw reply
* Re: [PATCH v1 0/2] urlmatch: allow regexp-based matches
From: Junio C Hamano @ 2017-01-23 19:53 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt
In-Reply-To: <20170123130635.29577-1-patrick.steinhardt@elego.de>
Patrick Steinhardt <patrick.steinhardt@elego.de> writes:
> This patch is mostly a request for comments. The use case is to
> be able to configure an HTTP proxy for all subdomains of a
> certain domain where there are hundreds of subdomains. The most
> flexible way I could imagine was by using regular expressions for
> the matching, which is how I implemented it for now. So users can
> now create a configuration key like
> `http.?http://.*\\.example\\.com.*` to apply settings for all
> subdomains of `example.com`.
While reading 2/2, I got an impression that this is "too" flexible
and possibly operates at a wrong level. I would have expected that
the wildcarding to be limited to the host part only and hook into
match_urls(), allowing the users of the new feature to still take
advantage of the existing support of "http://me@example.com" that
limits the match to the case that the connection is authenticated
for a user, for example, by newly allowing "http://me@*.example.com"
or something like that.
Because you cannot have a literal '*' in your hostname, I would
imagine that supporting a match pattern "http://me@*.example.com"
would be already backward compatible without requiring a leading
question-mark.
I also personally would prefer these textual matching to be done
with glob not with regexp, by the way, as the above description of
mine shows.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var
From: Junio C Hamano @ 2017-01-23 19:59 UTC (permalink / raw)
To: Christian Couder
Cc: Duy Nguyen, git, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <CAP8UFD2aQJ92KjzTQTGLyYeEuVk9TK51mn05OSWCZu5c4c6WuQ@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
>>> Perhaps we should declare that config will be the one and only way
>>> in the future and start deprecating the command line option way.
>>> That will remove the need for two to interact with each other.
>
> That would be my preferred solution as I think it is the simplest in
> the end for users.
> Also, as Duy wrote above, one can always use something like "git -c
> core.splitIndex= ...", which by the way can work for any command, not
> just "update-index".
>
> Anyway we would have to introduce core.splitIndex first, and it
> wouldn't make sense to have a different behavior between
> --[no-]split-index and --[no-]untracked-cache in the meantime before
> they are deprecated and eventually removed.
>
> So let's just go with the implementation in this series, which is
> similar to --[no-]untracked-cache, and let's plan to deprecate
> --[no-]split-index and --[no-]untracked-cache in a later patch series.
Sounds like we have a plan. Thanks.
^ permalink raw reply
* Re: [PATCH] rebase: pass --signoff option to git am
From: Giuseppe Bilotta @ 2017-01-23 20:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqh94ptzke.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 23, 2017 at 7:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Should we plan to extend this to the interactive backend that is
> shared between rebase -i and rebase -m, too? Or is this patch
> already sufficient to cover them?
AFAIK this is sufficient for both, in the sense that I've used it with
git rebase -i and it works.
^ permalink raw reply
* Re: [PATCH v3 0/5] show-ref: Allow -d, --head to work with --verify
From: Junio C Hamano @ 2017-01-23 20:03 UTC (permalink / raw)
To: Vladimir Panteleev; +Cc: git
In-Reply-To: <20170123180059.4288-1-git@thecybershadow.net>
Vladimir Panteleev <git@thecybershadow.net> writes:
> Third iteration, according to Junio's comments. This time we keep
> show_ref and show_one separate, accept HEAD with --verify even without
> --head, and add tests for dangling ref validation with --verify.
I am no longer a neutral judge but to me the resulting code looks a
lot more naturally structured than the original.
Thanks. Will queue.
^ permalink raw reply
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
From: Giuseppe Bilotta @ 2017-01-23 20:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqd1fdtzgv.fsf@gitster.mtv.corp.google.com>
On Mon, Jan 23, 2017 at 7:15 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> By the way, I noticed going over the code that the -allow options are
>> not stored, so that in case of interruption they will be reset, is
>> this intentional or a bug?
>
> I do not know offhand, but given the history of the two commands, I
> would guess it was a bug simply overlooked when people bolted "a
> series of commits" mode onto these commands that originally worked
> only on a single commit.
It seems like a bug to me. I'll prepare a new series that also fixes
this. I still don't see how to force a complete reread of the index
after running a git reset (which I need for the --skip command), but
maybe I'll add a WIP of what I have for --skip so far and people can
comment on that.
--
Giuseppe "Oblomov" Bilotta
^ permalink raw reply
* Re: [RFC PATCH] Option to allow cherry-pick to skip empty commits
From: Junio C Hamano @ 2017-01-23 20:10 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: Git List
In-Reply-To: <CAOxFTcxRjWS-=wyGBVOtg-tfCHrqqAr9rVOcvkyxXwJHonN_Tg@mail.gmail.com>
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> ... I still don't see how to force a complete reread of the index
> after running a git reset (which I need for the --skip command), ...
Do you mean discard_index() or discard_cache() followed by
read_index() or read_cache(), or do you mean something more
elaborate?
^ permalink raw reply
* Re: [PATCH] rebase: pass --signoff option to git am
From: Junio C Hamano @ 2017-01-23 20:16 UTC (permalink / raw)
To: Giuseppe Bilotta; +Cc: Git List
In-Reply-To: <CAOxFTcyuLkvgPOxQuzaDUVuDRu_KJg=JrYtU84pQyjLstChbLg@mail.gmail.com>
Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
> On Mon, Jan 23, 2017 at 7:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Should we plan to extend this to the interactive backend that is
>> shared between rebase -i and rebase -m, too? Or is this patch
>> already sufficient to cover them?
>
> AFAIK this is sufficient for both, in the sense that I've used it with
> git rebase -i and it works.
That is a good news and at the same time a bit awkard one ;-)
The mention of "passed to 'git am'" twice in the documentation and
help text would lead people to think "rebase -i" would not be
affected and (1) would need more work to do so, or (2) the user does
not want "rebase -i" to be unaffected for whatever reason, and gets
surprised to see that it actually does get affected.
In any case, will queue as-is so that we won't lose the patch while
waiting for people to raise their opinions.
Thanks.
^ permalink raw reply
* [PATCH v2 00/27] Revamp the attribute system; another round
From: Brandon Williams @ 2017-01-23 20:34 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170112235354.153403-1-bmwill@google.com>
Changes in v2:
* surround the mutex initializer calls by #ifdef
* mark file-local symbol static
* handling of attribute stacks. Instead of storing each stack frame in a
hashmap, there is a stack per attr_check instance. This will allow for
easier optimizing of the stack in future patches as well as eliminates the
potential for memory to grow unbounded. This is also more inline with the
original vision of the attribute system refactor.
Brandon Williams (8):
attr: pass struct attr_check to collect_some_attrs
attr: use hashmap for attribute dictionary
attr: eliminate global check_all_attr array
attr: remove maybe-real, maybe-macro from git_attr
attr: tighten const correctness with git_attr and match_attr
attr: store attribute stack in attr_check structure
attr: push the bare repo check into read_attr()
attr: reformat git_attr_set_direction() function
Junio C Hamano (17):
commit.c: use strchrnul() to scan for one line
attr.c: use strchrnul() to scan for one line
attr.c: update a stale comment on "struct match_attr"
attr.c: explain the lack of attr-name syntax check in parse_attr()
attr.c: complete a sentence in a comment
attr.c: mark where #if DEBUG ends more clearly
attr.c: simplify macroexpand_one()
attr.c: tighten constness around "git_attr" structure
attr.c: plug small leak in parse_attr_line()
attr.c: add push_stack() helper
attr.c: outline the future plans by heavily commenting
attr: rename function and struct related to checking attributes
attr: (re)introduce git_check_attr() and struct attr_check
attr: convert git_all_attrs() to use "struct attr_check"
attr: convert git_check_attrs() callers to use the new API
attr: retire git_check_attrs() API
attr: change validity check for attribute names to use positive logic
Nguyễn Thái Ngọc Duy (1):
attr: support quoting pathname patterns in C style
Stefan Beller (1):
Documentation: fix a typo
Documentation/gitattributes.txt | 10 +-
Documentation/technical/api-gitattributes.txt | 86 ++-
archive.c | 24 +-
attr.c | 854 ++++++++++++++++++--------
attr.h | 53 +-
builtin/check-attr.c | 66 +-
builtin/pack-objects.c | 19 +-
commit.c | 3 +-
common-main.c | 3 +
convert.c | 25 +-
ll-merge.c | 33 +-
t/t0003-attributes.sh | 26 +
userdiff.c | 19 +-
ws.c | 19 +-
14 files changed, 800 insertions(+), 440 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH v2 01/27] commit.c: use strchrnul() to scan for one line
From: Brandon Williams @ 2017-01-23 20:34 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
commit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 2cf85158b..0c4ee3de4 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
p++;
if (*p) {
p = skip_blank_lines(p + 2);
- for (eol = p; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
+ eol = strchrnul(p, '\n');
} else
eol = p;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 02/27] attr.c: use strchrnul() to scan for one line
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/attr.c b/attr.c
index 1fcf042b8..04d24334e 100644
--- a/attr.c
+++ b/attr.c
@@ -402,8 +402,8 @@ static struct attr_stack *read_attr_from_index(const char *path, int macro_ok)
for (sp = buf; *sp; ) {
char *ep;
int more;
- for (ep = sp; *ep && *ep != '\n'; ep++)
- ;
+
+ ep = strchrnul(sp, '\n');
more = (*ep == '\n');
*ep = '\0';
handle_attr_line(res, sp, path, ++lineno, macro_ok);
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 03/27] attr.c: update a stale comment on "struct match_attr"
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 04d24334e..007f1a299 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
* If is_macro is true, then u.attr is a pointer to the git_attr being
* defined.
*
- * If is_macro is false, then u.pattern points at the filename pattern
- * to which the rule applies. (The memory pointed to is part of the
- * memory block allocated for the match_attr instance.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
*
* In either case, num_attr is the number of attributes affected by
* this rule, and state is an array listing them. The attributes are
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 04/27] attr.c: explain the lack of attr-name syntax check in parse_attr()
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/attr.c b/attr.c
index 007f1a299..6b55a57ef 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
return NULL;
}
} else {
+ /*
+ * As this function is always called twice, once with
+ * e == NULL in the first pass and then e != NULL in
+ * the second pass, no need for invalid_attr_name()
+ * check here.
+ */
if (*cp == '-' || *cp == '!') {
e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
cp++;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 05/27] attr.c: complete a sentence in a comment
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 6b55a57ef..9bdf87a6f 100644
--- a/attr.c
+++ b/attr.c
@@ -300,7 +300,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* directory (again, reading the file from top to bottom) down to the
* current directory, and then scan the list backwards to find the first match.
* This is exactly the same as what is_excluded() does in dir.c to deal with
- * .gitignore
+ * .gitignore file and info/excludes file as a fallback.
*/
static struct attr_stack {
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 07/27] attr.c: simplify macroexpand_one()
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 17297fffe..e42f931b3 100644
--- a/attr.c
+++ b/attr.c
@@ -705,24 +705,21 @@ static int fill(const char *path, int pathlen, int basename_offset,
static int macroexpand_one(int nr, int rem)
{
struct attr_stack *stk;
- struct match_attr *a = NULL;
int i;
if (check_all_attr[nr].value != ATTR__TRUE ||
!check_all_attr[nr].attr->maybe_macro)
return rem;
- for (stk = attr_stack; !a && stk; stk = stk->prev)
- for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+ for (stk = attr_stack; stk; stk = stk->prev) {
+ for (i = stk->num_matches - 1; 0 <= i; i--) {
struct match_attr *ma = stk->attrs[i];
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
- a = ma;
+ return fill_one("expand", ma, rem);
}
-
- if (a)
- rem = fill_one("expand", a, rem);
+ }
return rem;
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 10/27] attr: support quoting pathname patterns in C style
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git
Cc: Nguyễn Thái Ngọc Duy, sbeller, gitster,
Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
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 <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/gitattributes.txt | 8 +++++---
attr.c | 15 +++++++++++++--
t/t0003-attributes.sh | 26 ++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 5 deletions(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e0b66c122..3173dee7e 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -21,9 +21,11 @@ Each line in `gitattributes` file is of form:
pattern attr1 attr2 ...
That is, a pattern followed by an attributes list,
-separated by whitespaces. When the pattern matches the
-path in question, the attributes listed on the line are given to
-the path.
+separated by whitespaces. Leading and trailing whitespaces are
+ignored. Lines that begin with '#' are ignored. Patterns
+that begin with a double quote are quoted in C style.
+When the pattern matches the path in question, the attributes
+listed on the line are given to the path.
Each attribute can be in one of these states for a given path:
diff --git a/attr.c b/attr.c
index d180c7833..e1c630f79 100644
--- a/attr.c
+++ b/attr.c
@@ -13,6 +13,7 @@
#include "attr.h"
#include "dir.h"
#include "utf8.h"
+#include "quote.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -212,12 +213,21 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
const char *cp, *name, *states;
struct match_attr *res = NULL;
int is_macro;
+ struct strbuf pattern = STRBUF_INIT;
cp = line + strspn(line, blank);
if (!*cp || *cp == '#')
return NULL;
name = cp;
- namelen = strcspn(name, blank);
+
+ if (*cp == '"' && !unquote_c_style(&pattern, name, &states)) {
+ name = pattern.buf;
+ namelen = pattern.len;
+ } else {
+ namelen = strcspn(name, blank);
+ states = name + namelen;
+ }
+
if (strlen(ATTRIBUTE_MACRO_PREFIX) < namelen &&
starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
if (!macro_ok) {
@@ -239,7 +249,6 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
else
is_macro = 0;
- states = name + namelen;
states += strspn(states, blank);
/* First pass to count the attr_states */
@@ -282,9 +291,11 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
cannot_trust_maybe_real = 1;
}
+ strbuf_release(&pattern);
return res;
fail_return:
+ strbuf_release(&pattern);
free(res);
return NULL;
}
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb4255..f19ae4f8c 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -13,10 +13,31 @@ attr_check () {
test_line_count = 0 err
}
+attr_check_quote () {
+
+ path="$1"
+ quoted_path="$2"
+ expect="$3"
+
+ git check-attr test -- "$path" >actual &&
+ echo "\"$quoted_path\": test: $expect" >expect &&
+ test_cmp expect actual
+
+}
+
+test_expect_success 'open-quoted pathname' '
+ echo "\"a test=a" >.gitattributes &&
+ test_must_fail attr_check a a
+'
+
+
test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
echo "[attr]notest !test"
+ echo "\" d \" test=d"
+ echo " e test=e"
+ echo " e\" test=e"
echo "f test=f"
echo "a/i test=a/i"
echo "onoff test -test"
@@ -69,6 +90,11 @@ test_expect_success 'command line checks' '
'
test_expect_success 'attribute test' '
+
+ attr_check " d " d &&
+ attr_check e e &&
+ attr_check_quote e\" e\\\" e &&
+
attr_check f f &&
attr_check a/f f &&
attr_check a/c/f f &&
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 12/27] Documentation: fix a typo
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Stefan Beller, gitster, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Stefan Beller <sbeller@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
Documentation/gitattributes.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 3173dee7e..a53d093ca 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -88,7 +88,7 @@ is either not set or empty, $HOME/.config/git/attributes is used instead.
Attributes for all users on a system should be placed in the
`$(prefix)/etc/gitattributes` file.
-Sometimes you would need to override an setting of an attribute
+Sometimes you would need to override a setting of an attribute
for a path to `Unspecified` state. This can be done by listing
the name of the attribute prefixed with an exclamation point `!`.
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 13/27] attr.c: outline the future plans by heavily commenting
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 8026d68bd..50e5ee393 100644
--- a/attr.c
+++ b/attr.c
@@ -30,6 +30,11 @@ static const char git_attr__unknown[] = "(builtin)unknown";
#define DEBUG_ATTR 0
#endif
+/*
+ * NEEDSWORK: the global dictionary of the interned attributes
+ * must stay a singleton even after we become thread-ready.
+ * Access to these must be surrounded with mutex when it happens.
+ */
struct git_attr {
struct git_attr *next;
unsigned h;
@@ -39,10 +44,19 @@ struct git_attr {
char name[FLEX_ARRAY];
};
static int attr_nr;
+static struct git_attr *(git_attr_hash[HASHSIZE]);
+
+/*
+ * NEEDSWORK: maybe-real, maybe-macro are not property of
+ * an attribute, as it depends on what .gitattributes are
+ * read. Once we introduce per git_attr_check attr_stack
+ * and check_all_attr, the optimization based on them will
+ * become unnecessary and can go away. So is this variable.
+ */
static int cannot_trust_maybe_real;
+/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check *check_all_attr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
const char *git_attr_name(const struct git_attr *attr)
{
@@ -102,6 +116,11 @@ static struct git_attr *git_attr_internal(const char *name, int len)
a->maybe_real = 0;
git_attr_hash[pos] = a;
+ /*
+ * NEEDSWORK: per git_attr_check check_all_attr
+ * will be initialized a lot more lazily, not
+ * like this, and not here.
+ */
REALLOC_ARRAY(check_all_attr, attr_nr);
check_all_attr[a->attr_nr].attr = a;
check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
@@ -318,6 +337,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
+/* NEEDSWORK: This will become per git_attr_check */
static struct attr_stack {
struct attr_stack *prev;
char *origin;
@@ -382,6 +402,24 @@ static struct attr_stack *read_attr_from_array(const char **list)
return res;
}
+/*
+ * NEEDSWORK: these two are tricky. The callers assume there is a
+ * single, system-wide global state "where we read attributes from?"
+ * and when the state is flipped by calling git_attr_set_direction(),
+ * attr_stack is discarded so that subsequent attr_check will lazily
+ * read from the right place. And they do not know or care who called
+ * by them uses the attribute subsystem, hence have no knowledge of
+ * existing git_attr_check instances or future ones that will be
+ * created).
+ *
+ * Probably we need a thread_local that holds these two variables,
+ * and a list of git_attr_check instances (which need to be maintained
+ * by hooking into git_attr_check_alloc(), git_attr_check_initl(), and
+ * git_attr_check_clear(). Then git_attr_set_direction() updates the
+ * fields in that thread_local for these two variables, iterate over
+ * all the active git_attr_check instances and discard the attr_stack
+ * they hold. Yuck, but it sounds doable.
+ */
static enum git_attr_direction direction;
static struct index_state *use_index;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 09/27] attr.c: plug small leak in parse_attr_line()
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/attr.c b/attr.c
index f7cf7ae30..d180c7833 100644
--- a/attr.c
+++ b/attr.c
@@ -223,7 +223,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (!macro_ok) {
fprintf(stderr, "%s not allowed: %s:%d\n",
name, src, lineno);
- return NULL;
+ goto fail_return;
}
is_macro = 1;
name += strlen(ATTRIBUTE_MACRO_PREFIX);
@@ -233,7 +233,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
fprintf(stderr,
"%.*s is not a valid attribute name: %s:%d\n",
namelen, name, src, lineno);
- return NULL;
+ goto fail_return;
}
}
else
@@ -246,7 +246,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
for (cp = states, num_attr = 0; *cp; num_attr++) {
cp = parse_attr(src, lineno, cp, NULL);
if (!cp)
- return NULL;
+ goto fail_return;
}
res = xcalloc(1,
@@ -267,7 +267,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
if (res->u.pat.flags & EXC_FLAG_NEGATIVE) {
warning(_("Negative patterns are ignored in git attributes\n"
"Use '\\!' for literal leading exclamation."));
- return NULL;
+ goto fail_return;
}
}
res->is_macro = is_macro;
@@ -283,6 +283,10 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
}
return res;
+
+fail_return:
+ free(res);
+ return NULL;
}
/*
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 11/27] attr.c: add push_stack() helper
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 71 +++++++++++++++++++++++++++++++-----------------------------------
1 file changed, 33 insertions(+), 38 deletions(-)
diff --git a/attr.c b/attr.c
index e1c630f79..8026d68bd 100644
--- a/attr.c
+++ b/attr.c
@@ -510,6 +510,18 @@ static int git_attr_system(void)
static GIT_PATH_FUNC(git_path_info_attributes, INFOATTRIBUTES_FILE)
+static void push_stack(struct attr_stack **attr_stack_p,
+ struct attr_stack *elem, char *origin, size_t originlen)
+{
+ if (elem) {
+ elem->origin = origin;
+ if (origin)
+ elem->originlen = originlen;
+ elem->prev = *attr_stack_p;
+ *attr_stack_p = elem;
+ }
+}
+
static void bootstrap_attr_stack(void)
{
struct attr_stack *elem;
@@ -517,37 +529,23 @@ static void bootstrap_attr_stack(void)
if (attr_stack)
return;
- elem = read_attr_from_array(builtin_attr);
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
-
- if (git_attr_system()) {
- elem = read_attr_from_file(git_etc_gitattributes(), 1);
- if (elem) {
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
- }
- }
+ push_stack(&attr_stack, read_attr_from_array(builtin_attr), NULL, 0);
+
+ if (git_attr_system())
+ 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) {
- 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(&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);
- elem->origin = xstrdup("");
- elem->originlen = 0;
- elem->prev = attr_stack;
- attr_stack = elem;
+ push_stack(&attr_stack, elem, xstrdup(""), 0);
debug_push(elem);
}
@@ -558,15 +556,12 @@ static void bootstrap_attr_stack(void)
if (!elem)
elem = xcalloc(1, sizeof(*elem));
- elem->origin = NULL;
- elem->prev = attr_stack;
- attr_stack = elem;
+ push_stack(&attr_stack, elem, NULL, 0);
}
static void prepare_attr_stack(const char *path, int dirlen)
{
struct attr_stack *elem, *info;
- int len;
const char *cp;
/*
@@ -626,20 +621,21 @@ static void prepare_attr_stack(const char *path, int dirlen)
assert(attr_stack->origin);
while (1) {
- len = strlen(attr_stack->origin);
+ size_t len = strlen(attr_stack->origin);
+ char *origin;
+
if (dirlen <= len)
break;
cp = memchr(path + len + 1, '/', dirlen - len - 1);
if (!cp)
cp = path + dirlen;
- strbuf_add(&pathbuf, path, cp - path);
- strbuf_addch(&pathbuf, '/');
- strbuf_addstr(&pathbuf, GITATTRIBUTES_FILE);
+ strbuf_addf(&pathbuf,
+ "%.*s/%s", (int)(cp - path), path,
+ GITATTRIBUTES_FILE);
elem = read_attr(pathbuf.buf, 0);
strbuf_setlen(&pathbuf, cp - path);
- elem->origin = strbuf_detach(&pathbuf, &elem->originlen);
- elem->prev = attr_stack;
- attr_stack = elem;
+ origin = strbuf_detach(&pathbuf, &len);
+ push_stack(&attr_stack, elem, origin, len);
debug_push(elem);
}
@@ -649,8 +645,7 @@ static void prepare_attr_stack(const char *path, int dirlen)
/*
* Finally push the "info" one at the top of the stack.
*/
- info->prev = attr_stack;
- attr_stack = info;
+ push_stack(&attr_stack, info, NULL, 0);
}
static int path_matches(const char *pathname, int pathlen,
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 14/27] attr: rename function and struct related to checking attributes
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
The traditional API to check attributes is to prepare an N-element
array of "struct git_attr_check" and pass N and the array to the
function "git_check_attr()" as arguments.
In preparation to revamp the API to pass a single structure, in
which these N elements are held, rename the type used for these
individual array elements to "struct attr_check_item" and rename
the function to "git_check_attrs()".
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
archive.c | 6 +++---
attr.c | 12 ++++++------
attr.h | 8 ++++----
builtin/check-attr.c | 19 ++++++++++---------
builtin/pack-objects.c | 6 +++---
convert.c | 12 ++++++------
ll-merge.c | 10 +++++-----
userdiff.c | 4 ++--
ws.c | 6 +++---
9 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/archive.c b/archive.c
index 01751e574..b76bd4691 100644
--- a/archive.c
+++ b/archive.c
@@ -87,7 +87,7 @@ void *sha1_file_to_archive(const struct archiver_args *args,
return buffer;
}
-static void setup_archive_check(struct git_attr_check *check)
+static void setup_archive_check(struct attr_check_item *check)
{
static struct git_attr *attr_export_ignore;
static struct git_attr *attr_export_subst;
@@ -123,7 +123,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
struct archiver_context *c = context;
struct archiver_args *args = c->args;
write_archive_entry_fn_t write_entry = c->write_entry;
- struct git_attr_check check[2];
+ struct attr_check_item check[2];
const char *path_without_prefix;
int err;
@@ -138,7 +138,7 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
path_without_prefix = path.buf + args->baselen;
setup_archive_check(check);
- if (!git_check_attr(path_without_prefix, ARRAY_SIZE(check), check)) {
+ if (!git_check_attrs(path_without_prefix, ARRAY_SIZE(check), check)) {
if (ATTR_TRUE(check[0].value))
return 0;
args->convert = ATTR_TRUE(check[1].value);
diff --git a/attr.c b/attr.c
index 50e5ee393..2f180d609 100644
--- a/attr.c
+++ b/attr.c
@@ -56,7 +56,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
static int cannot_trust_maybe_real;
/* NEEDSWORK: This will become per git_attr_check */
-static struct git_attr_check *check_all_attr;
+static struct attr_check_item *check_all_attr;
const char *git_attr_name(const struct git_attr *attr)
{
@@ -713,7 +713,7 @@ static int macroexpand_one(int attr_nr, int rem);
static int fill_one(const char *what, struct match_attr *a, int rem)
{
- struct git_attr_check *check = check_all_attr;
+ struct attr_check_item *check = check_all_attr;
int i;
for (i = a->num_attr - 1; 0 < rem && 0 <= i; i--) {
@@ -778,7 +778,7 @@ static int macroexpand_one(int nr, int rem)
* collected. Otherwise all attributes are collected.
*/
static void collect_some_attrs(const char *path, int num,
- struct git_attr_check *check)
+ struct attr_check_item *check)
{
struct attr_stack *stk;
@@ -806,7 +806,7 @@ static void collect_some_attrs(const char *path, int num,
rem = 0;
for (i = 0; i < num; i++) {
if (!check[i].attr->maybe_real) {
- struct git_attr_check *c;
+ struct attr_check_item *c;
c = check_all_attr + check[i].attr->attr_nr;
c->value = ATTR__UNSET;
rem++;
@@ -821,7 +821,7 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-int git_check_attr(const char *path, int num, struct git_attr_check *check)
+int git_check_attrs(const char *path, int num, struct attr_check_item *check)
{
int i;
@@ -837,7 +837,7 @@ int git_check_attr(const char *path, int num, struct git_attr_check *check)
return 0;
}
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check)
+int git_all_attrs(const char *path, int *num, struct attr_check_item **check)
{
int i, count, j;
diff --git a/attr.h b/attr.h
index 00d7a662c..efc7bb3b3 100644
--- a/attr.h
+++ b/attr.h
@@ -20,11 +20,11 @@ extern const char git_attr__false[];
#define ATTR_UNSET(v) ((v) == NULL)
/*
- * Send one or more git_attr_check to git_check_attr(), and
+ * Send one or more git_attr_check to git_check_attrs(), and
* each 'value' member tells what its value is.
* Unset one is returned as NULL.
*/
-struct git_attr_check {
+struct attr_check_item {
const struct git_attr *attr;
const char *value;
};
@@ -36,7 +36,7 @@ struct git_attr_check {
*/
extern const char *git_attr_name(const struct git_attr *);
-int git_check_attr(const char *path, int, struct git_attr_check *);
+int git_check_attrs(const char *path, int, struct attr_check_item *);
/*
* Retrieve all attributes that apply to the specified path. *num
@@ -45,7 +45,7 @@ int git_check_attr(const char *path, int, struct git_attr_check *);
* objects describing the attributes and their values. *check must be
* free()ed by the caller.
*/
-int git_all_attrs(const char *path, int *num, struct git_attr_check **check);
+int git_all_attrs(const char *path, int *num, struct attr_check_item **check);
enum git_attr_direction {
GIT_ATTR_CHECKIN,
diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 53a5a18c1..889264a5b 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -24,8 +24,8 @@ static const struct option check_attr_options[] = {
OPT_END()
};
-static void output_attr(int cnt, struct git_attr_check *check,
- const char *file)
+static void output_attr(int cnt, struct attr_check_item *check,
+ const char *file)
{
int j;
for (j = 0; j < cnt; j++) {
@@ -51,14 +51,15 @@ static void output_attr(int cnt, struct git_attr_check *check,
}
}
-static void check_attr(const char *prefix, int cnt,
- struct git_attr_check *check, const char *file)
+static void check_attr(const char *prefix,
+ int cnt, struct attr_check_item *check,
+ const char *file)
{
char *full_path =
prefix_path(prefix, prefix ? strlen(prefix) : 0, file);
if (check != NULL) {
- if (git_check_attr(full_path, cnt, check))
- die("git_check_attr died");
+ if (git_check_attrs(full_path, cnt, check))
+ die("git_check_attrs died");
output_attr(cnt, check, file);
} else {
if (git_all_attrs(full_path, &cnt, &check))
@@ -69,8 +70,8 @@ static void check_attr(const char *prefix, int cnt,
free(full_path);
}
-static void check_attr_stdin_paths(const char *prefix, int cnt,
- struct git_attr_check *check)
+static void check_attr_stdin_paths(const char *prefix,
+ int cnt, struct attr_check_item *check)
{
struct strbuf buf = STRBUF_INIT;
struct strbuf unquoted = STRBUF_INIT;
@@ -99,7 +100,7 @@ static NORETURN void error_with_usage(const char *msg)
int cmd_check_attr(int argc, const char **argv, const char *prefix)
{
- struct git_attr_check *check;
+ struct attr_check_item *check;
int cnt, i, doubledash, filei;
if (!is_bare_repository())
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 8841f8b36..8b8fbd814 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -894,7 +894,7 @@ static void write_pack_file(void)
written, nr_result);
}
-static void setup_delta_attr_check(struct git_attr_check *check)
+static void setup_delta_attr_check(struct attr_check_item *check)
{
static struct git_attr *attr_delta;
@@ -906,10 +906,10 @@ static void setup_delta_attr_check(struct git_attr_check *check)
static int no_try_delta(const char *path)
{
- struct git_attr_check check[1];
+ struct attr_check_item check[1];
setup_delta_attr_check(check);
- if (git_check_attr(path, ARRAY_SIZE(check), check))
+ if (git_check_attrs(path, ARRAY_SIZE(check), check))
return 0;
if (ATTR_FALSE(check->value))
return 1;
diff --git a/convert.c b/convert.c
index 4e17e45ed..1b9829279 100644
--- a/convert.c
+++ b/convert.c
@@ -1028,7 +1028,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
return 1;
}
-static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
+static enum crlf_action git_path_check_crlf(struct attr_check_item *check)
{
const char *value = check->value;
@@ -1045,7 +1045,7 @@ static enum crlf_action git_path_check_crlf(struct git_attr_check *check)
return CRLF_UNDEFINED;
}
-static enum eol git_path_check_eol(struct git_attr_check *check)
+static enum eol git_path_check_eol(struct attr_check_item *check)
{
const char *value = check->value;
@@ -1058,7 +1058,7 @@ static enum eol git_path_check_eol(struct git_attr_check *check)
return EOL_UNSET;
}
-static struct convert_driver *git_path_check_convert(struct git_attr_check *check)
+static struct convert_driver *git_path_check_convert(struct attr_check_item *check)
{
const char *value = check->value;
struct convert_driver *drv;
@@ -1071,7 +1071,7 @@ static struct convert_driver *git_path_check_convert(struct git_attr_check *chec
return NULL;
}
-static int git_path_check_ident(struct git_attr_check *check)
+static int git_path_check_ident(struct attr_check_item *check)
{
const char *value = check->value;
@@ -1093,7 +1093,7 @@ static const char *conv_attr_name[] = {
static void convert_attrs(struct conv_attrs *ca, const char *path)
{
int i;
- static struct git_attr_check ccheck[NUM_CONV_ATTRS];
+ static struct attr_check_item ccheck[NUM_CONV_ATTRS];
if (!ccheck[0].attr) {
for (i = 0; i < NUM_CONV_ATTRS; i++)
@@ -1102,7 +1102,7 @@ static void convert_attrs(struct conv_attrs *ca, const char *path)
git_config(read_convert_config, NULL);
}
- if (!git_check_attr(path, NUM_CONV_ATTRS, ccheck)) {
+ if (!git_check_attrs(path, NUM_CONV_ATTRS, ccheck)) {
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 ad8be42f9..198f07aca 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -336,13 +336,13 @@ static const struct ll_merge_driver *find_ll_merge_driver(const char *merge_attr
return &ll_merge_drv[LL_TEXT_MERGE];
}
-static int git_path_check_merge(const char *path, struct git_attr_check check[2])
+static int git_path_check_merge(const char *path, struct attr_check_item check[2])
{
if (!check[0].attr) {
check[0].attr = git_attr("merge");
check[1].attr = git_attr("conflict-marker-size");
}
- return git_check_attr(path, 2, check);
+ return git_check_attrs(path, 2, check);
}
static void normalize_file(mmfile_t *mm, const char *path)
@@ -362,7 +362,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 check[2];
+ static struct attr_check_item check[2];
static const struct ll_merge_options default_opts;
const char *ll_driver_name = NULL;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
@@ -398,12 +398,12 @@ int ll_merge(mmbuffer_t *result_buf,
int ll_merge_marker_size(const char *path)
{
- static struct git_attr_check check;
+ static struct attr_check_item check;
int marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
if (!check.attr)
check.attr = git_attr("conflict-marker-size");
- if (!git_check_attr(path, 1, &check) && check.value) {
+ if (!git_check_attrs(path, 1, &check) && check.value) {
marker_size = atoi(check.value);
if (marker_size <= 0)
marker_size = DEFAULT_CONFLICT_MARKER_SIZE;
diff --git a/userdiff.c b/userdiff.c
index 2125d6da2..b0b44467a 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -263,7 +263,7 @@ struct userdiff_driver *userdiff_find_by_name(const char *name) {
struct userdiff_driver *userdiff_find_by_path(const char *path)
{
static struct git_attr *attr;
- struct git_attr_check check;
+ struct attr_check_item check;
if (!attr)
attr = git_attr("diff");
@@ -271,7 +271,7 @@ struct userdiff_driver *userdiff_find_by_path(const char *path)
if (!path)
return NULL;
- if (git_check_attr(path, 1, &check))
+ if (git_check_attrs(path, 1, &check))
return NULL;
if (ATTR_TRUE(check.value))
diff --git a/ws.c b/ws.c
index ea4b2b1df..fbd876e84 100644
--- a/ws.c
+++ b/ws.c
@@ -71,7 +71,7 @@ unsigned parse_whitespace_rule(const char *string)
return rule;
}
-static void setup_whitespace_attr_check(struct git_attr_check *check)
+static void setup_whitespace_attr_check(struct attr_check_item *check)
{
static struct git_attr *attr_whitespace;
@@ -82,10 +82,10 @@ static void setup_whitespace_attr_check(struct git_attr_check *check)
unsigned whitespace_rule(const char *pathname)
{
- struct git_attr_check attr_whitespace_rule;
+ struct attr_check_item attr_whitespace_rule;
setup_whitespace_attr_check(&attr_whitespace_rule);
- if (!git_check_attr(pathname, 1, &attr_whitespace_rule)) {
+ if (!git_check_attrs(pathname, 1, &attr_whitespace_rule)) {
const char *value;
value = attr_whitespace_rule.value;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 20/27] attr: change validity check for attribute names to use positive logic
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, sbeller, pclouds, Brandon Williams
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
From: Junio C Hamano <gitster@pobox.com>
Convert 'invalid_attr_name()' to 'attr_name_valid()' and use positive
logic for the return value. In addition create a helper function that
prints out an error message when an invalid attribute name is used.
We could later update the message to exactly spell out what the
rules for a good attribute name are, etc.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 34 ++++++++++++++++++++--------------
1 file changed, 20 insertions(+), 14 deletions(-)
diff --git a/attr.c b/attr.c
index e58fa340c..5399e1cb3 100644
--- a/attr.c
+++ b/attr.c
@@ -74,23 +74,33 @@ static unsigned hash_name(const char *name, int namelen)
return val;
}
-static int invalid_attr_name(const char *name, int namelen)
+static int attr_name_valid(const char *name, size_t namelen)
{
/*
* Attribute name cannot begin with '-' and must consist of
* characters from [-A-Za-z0-9_.].
*/
if (namelen <= 0 || *name == '-')
- return -1;
+ return 0;
while (namelen--) {
char ch = *name++;
if (! (ch == '-' || ch == '.' || ch == '_' ||
('0' <= ch && ch <= '9') ||
('a' <= ch && ch <= 'z') ||
('A' <= ch && ch <= 'Z')) )
- return -1;
+ return 0;
}
- return 0;
+ return 1;
+}
+
+static void report_invalid_attr(const char *name, size_t len,
+ const char *src, int lineno)
+{
+ struct strbuf err = STRBUF_INIT;
+ strbuf_addf(&err, _("%.*s is not a valid attribute name"),
+ (int) len, name);
+ fprintf(stderr, "%s: %s:%d\n", err.buf, src, lineno);
+ strbuf_release(&err);
}
static struct git_attr *git_attr_internal(const char *name, int len)
@@ -105,7 +115,7 @@ static struct git_attr *git_attr_internal(const char *name, int len)
return a;
}
- if (invalid_attr_name(name, len))
+ if (!attr_name_valid(name, len))
return NULL;
FLEX_ALLOC_MEM(a, name, name, len);
@@ -196,17 +206,15 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
cp++;
len--;
}
- if (invalid_attr_name(cp, len)) {
- fprintf(stderr,
- "%.*s is not a valid attribute name: %s:%d\n",
- len, cp, src, lineno);
+ if (!attr_name_valid(cp, len)) {
+ report_invalid_attr(cp, len, src, lineno);
return NULL;
}
} else {
/*
* As this function is always called twice, once with
* e == NULL in the first pass and then e != NULL in
- * the second pass, no need for invalid_attr_name()
+ * the second pass, no need for attr_name_valid()
* check here.
*/
if (*cp == '-' || *cp == '!') {
@@ -258,10 +266,8 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
name += strlen(ATTRIBUTE_MACRO_PREFIX);
name += strspn(name, blank);
namelen = strcspn(name, blank);
- if (invalid_attr_name(name, namelen)) {
- fprintf(stderr,
- "%.*s is not a valid attribute name: %s:%d\n",
- namelen, name, src, lineno);
+ if (!attr_name_valid(name, namelen)) {
+ report_invalid_attr(name, namelen, src, lineno);
goto fail_return;
}
}
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH v2 21/27] attr: use hashmap for attribute dictionary
From: Brandon Williams @ 2017-01-23 20:35 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, gitster, pclouds
In-Reply-To: <20170123203525.185058-1-bmwill@google.com>
The current implementation of the attribute dictionary uses a custom
hashtable. This modernizes the dictionary by converting it to the builtin
'hashmap' structure.
Also, in order to enable a threaded API in the future add an
accompanying mutex which must be acquired prior to accessing the
dictionary of interned attributes.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
attr.c | 173 +++++++++++++++++++++++++++++++++++++++++++---------------
attr.h | 2 +
common-main.c | 3 +
3 files changed, 133 insertions(+), 45 deletions(-)
diff --git a/attr.c b/attr.c
index 5399e1cb3..d2ece4eba 100644
--- a/attr.c
+++ b/attr.c
@@ -14,6 +14,7 @@
#include "dir.h"
#include "utf8.h"
#include "quote.h"
+#include "thread-utils.h"
const char git_attr__true[] = "(builtin)true";
const char git_attr__false[] = "\0(builtin)false";
@@ -23,28 +24,17 @@ static const char git_attr__unknown[] = "(builtin)unknown";
#define ATTR__UNSET NULL
#define ATTR__UNKNOWN git_attr__unknown
-/* This is a randomly chosen prime. */
-#define HASHSIZE 257
-
#ifndef DEBUG_ATTR
#define DEBUG_ATTR 0
#endif
-/*
- * NEEDSWORK: the global dictionary of the interned attributes
- * must stay a singleton even after we become thread-ready.
- * Access to these must be surrounded with mutex when it happens.
- */
struct git_attr {
- struct git_attr *next;
- unsigned h;
- int attr_nr;
+ int attr_nr; /* unique attribute number */
int maybe_macro;
int maybe_real;
- char name[FLEX_ARRAY];
+ char name[FLEX_ARRAY]; /* attribute name */
};
static int attr_nr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
/*
* NEEDSWORK: maybe-real, maybe-macro are not property of
@@ -63,15 +53,94 @@ const char *git_attr_name(const struct git_attr *attr)
return attr->name;
}
-static unsigned hash_name(const char *name, int namelen)
+struct attr_hashmap {
+ struct hashmap map;
+#ifndef NO_PTHREADS
+ pthread_mutex_t mutex;
+#endif
+};
+
+static inline void hashmap_lock(struct attr_hashmap *map)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_lock(&map->mutex);
+#endif
+}
+
+static inline void hashmap_unlock(struct attr_hashmap *map)
{
- unsigned val = 0, c;
+#ifndef NO_PTHREADS
+ pthread_mutex_unlock(&map->mutex);
+#endif
+}
- while (namelen--) {
- c = *name++;
- val = ((val << 7) | (val >> 22)) ^ c;
- }
- return val;
+/*
+ * The global dictionary of all interned attributes. This
+ * is a singleton object which is shared between threads.
+ * Access to this dictionary must be surrounded with a mutex.
+ */
+static struct attr_hashmap g_attr_hashmap;
+
+/* The container for objects stored in "struct attr_hashmap" */
+struct attr_hash_entry {
+ struct hashmap_entry ent; /* must be the first member! */
+ const char *key; /* the key; memory should be owned by value */
+ size_t keylen; /* length of the key */
+ void *value; /* the stored value */
+};
+
+/* attr_hashmap comparison function */
+static int attr_hash_entry_cmp(const struct attr_hash_entry *a,
+ const struct attr_hash_entry *b,
+ void *unused)
+{
+ return (a->keylen != b->keylen) || strncmp(a->key, b->key, a->keylen);
+}
+
+/* Initialize an 'attr_hashmap' object */
+static void attr_hashmap_init(struct attr_hashmap *map)
+{
+ hashmap_init(&map->map, (hashmap_cmp_fn) attr_hash_entry_cmp, 0);
+}
+
+/*
+ * Retrieve the 'value' stored in a hashmap given the provided 'key'.
+ * If there is no matching entry, return NULL.
+ */
+static void *attr_hashmap_get(struct attr_hashmap *map,
+ const char *key, size_t keylen)
+{
+ struct attr_hash_entry k;
+ struct attr_hash_entry *e;
+
+ if (!map->map.tablesize)
+ attr_hashmap_init(map);
+
+ hashmap_entry_init(&k, memhash(key, keylen));
+ k.key = key;
+ k.keylen = keylen;
+ e = hashmap_get(&map->map, &k, NULL);
+
+ return e ? e->value : NULL;
+}
+
+/* Add 'value' to a hashmap based on the provided 'key'. */
+static void attr_hashmap_add(struct attr_hashmap *map,
+ const char *key, size_t keylen,
+ void *value)
+{
+ struct attr_hash_entry *e;
+
+ if (!map->map.tablesize)
+ attr_hashmap_init(map);
+
+ e = xmalloc(sizeof(struct attr_hash_entry));
+ hashmap_entry_init(e, memhash(key, keylen));
+ e->key = key;
+ e->keylen = keylen;
+ e->value = value;
+
+ hashmap_add(&map->map, e);
}
static int attr_name_valid(const char *name, size_t namelen)
@@ -103,37 +172,44 @@ static void report_invalid_attr(const char *name, size_t len,
strbuf_release(&err);
}
-static struct git_attr *git_attr_internal(const char *name, int len)
+/*
+ * Given a 'name', lookup and return the corresponding attribute in the global
+ * dictionary. If no entry is found, create a new attribute and store it in
+ * the dictionary.
+ */
+static struct git_attr *git_attr_internal(const char *name, int namelen)
{
- unsigned hval = hash_name(name, len);
- unsigned pos = hval % HASHSIZE;
struct git_attr *a;
- for (a = git_attr_hash[pos]; a; a = a->next) {
- if (a->h == hval &&
- !memcmp(a->name, name, len) && !a->name[len])
- return a;
- }
-
- if (!attr_name_valid(name, len))
+ if (!attr_name_valid(name, namelen))
return NULL;
- FLEX_ALLOC_MEM(a, name, name, len);
- a->h = hval;
- a->next = git_attr_hash[pos];
- a->attr_nr = attr_nr++;
- a->maybe_macro = 0;
- a->maybe_real = 0;
- git_attr_hash[pos] = a;
+ hashmap_lock(&g_attr_hashmap);
+
+ a = attr_hashmap_get(&g_attr_hashmap, name, namelen);
+
+ if (!a) {
+ FLEX_ALLOC_MEM(a, name, name, namelen);
+ a->attr_nr = g_attr_hashmap.map.size;
+ a->maybe_real = 0;
+ a->maybe_macro = 0;
+
+ attr_hashmap_add(&g_attr_hashmap, a->name, namelen, a);
+ assert(a->attr_nr == (g_attr_hashmap.map.size - 1));
+
+ /*
+ * NEEDSWORK: per git_attr_check check_all_attr
+ * will be initialized a lot more lazily, not
+ * like this, and not here.
+ */
+ REALLOC_ARRAY(check_all_attr, ++attr_nr);
+ check_all_attr[a->attr_nr].attr = a;
+ check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
+ assert(a->attr_nr == (attr_nr - 1));
+ }
+
+ hashmap_unlock(&g_attr_hashmap);
- /*
- * 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;
return a;
}
@@ -941,3 +1017,10 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
drop_attr_stack();
use_index = istate;
}
+
+void attr_start(void)
+{
+#ifndef NO_PTHREADS
+ pthread_mutex_init(&g_attr_hashmap.mutex, NULL);
+#endif
+}
diff --git a/attr.h b/attr.h
index 3db9893ef..8505bca79 100644
--- a/attr.h
+++ b/attr.h
@@ -67,4 +67,6 @@ enum git_attr_direction {
};
void git_attr_set_direction(enum git_attr_direction, struct index_state *);
+extern void attr_start(void);
+
#endif /* ATTR_H */
diff --git a/common-main.c b/common-main.c
index c654f9555..6a689007e 100644
--- a/common-main.c
+++ b/common-main.c
@@ -1,5 +1,6 @@
#include "cache.h"
#include "exec_cmd.h"
+#include "attr.h"
/*
* Many parts of Git have subprograms communicate via pipe, expect the
@@ -33,6 +34,8 @@ int main(int argc, const char **argv)
git_setup_gettext();
+ attr_start();
+
git_extract_argv0_path(argv[0]);
restore_sigpipe_to_default();
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox