* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Jeff King @ 2016-11-10 22:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Andreas Schwab, Johannes Sixt, git
In-Reply-To: <xmqq60nv55o3.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 02:30:36PM -0800, Junio C Hamano wrote:
> As everybody knows there is no appropriate timeout value that is
> good for everybody. I wonder if we can replace the sleep 1 with
> something like
>
> ( while sleep 3600; do :; done ) &
>
> so that leaked fd will be kept even in any heavily loaded
> environment instead?
I think you may have missed:
http://public-inbox.org/git/16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de/
which does roughly that. It does not loop, but I suspect 3600 is plenty
in practice.
I do think the test would be a lot more obvious if it confirmed at the
end of the test that the process was still running, as opposed to
relying on test_when_finished to check it.
-Peff
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Junio C Hamano @ 2016-11-10 22:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Andreas Schwab, Jeff King, Johannes Sixt, git
In-Reply-To: <alpine.DEB.2.20.1611102254340.24684@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> OK. sleep.pid is a reasonable easy-to-access side effect we can
>> observe to make sure that the sleep-one-second merge driver was
>> indeed invoked, which was missing from the earlier round.
>
> No, this is incorrect. The condition that we need to know applies is that
> the script is still running, and blocking if the bug reappears.
OK, I see what you are saying, and I see a few things wrong in here:
* First, the test is titled in a misleading way. In the context of
a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child
processes must not inherit index.lock handles", 2016-08-18), it
might have been clear enough to say "does not lock index", but
the sleeping is to make sure that we would notice if the fd to
the index.lock leaked to the child process by mistake, and the
way to do so is that the child arranges the leaked fd to be kept
open after it exits (by spawning "sleep"). The test was never
about "does not lock index" (the driver does not take any lock by
itself in the first place).
* There are three possible outcome from this test:
- 'git merge' fails.
This is expected to happen only on Windows and if the code gets
broken and starts leaking the fd.
- 'git merge' finishes correctly, the sleep is still running when
test_when_finished goes to cull it.
In this case, we KNOW there wasn't any fd leak IF we are on
Windows where a leaked FD would not allow 'git merge' to
succeed. But on other platforms, fd leak that may cause
trouble for Windows friends will not be caught.
- 'git merge' finishes correctly, the sleep is no longer
running because the machine was heavily loaded; a workaround is
to tolerate failure of culling it.
In this case, we cannot tell anything from the test. Even if
the fd was leaked, 'git merge' may have succeeded even on
Windows.
As everybody knows there is no appropriate timeout value that is
good for everybody. I wonder if we can replace the sleep 1 with
something like
( while sleep 3600; do :; done ) &
so that leaked fd will be kept even in any heavily loaded
environment instead?
^ permalink raw reply
* Re: [PATCH] doc: fill in omitted word
From: Junio C Hamano @ 2016-11-10 22:07 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <20161104214357.32477-1-kristoffer.haugsbakk@gmail.com>
Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com> writes:
> Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
> ---
> Documentation/gitcore-tutorial.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
> index 4546fa0..9860517 100644
> --- a/Documentation/gitcore-tutorial.txt
> +++ b/Documentation/gitcore-tutorial.txt
> @@ -25,7 +25,7 @@ you want to understand Git's internals.
> The core Git is often called "plumbing", with the prettier user
> interfaces on top of it called "porcelain". You may not want to use the
> plumbing directly very often, but it can be good to know what the
> -plumbing does for when the porcelain isn't flushing.
> +plumbing does for you when the porcelain isn't flushing.
I need an English teacher here to help me out, but I think this
changes the meaning of the sentence from what the original author
intended..
The way I've read this statement for the past ten years since it was
written originally at 8c7fa2478e ("Add first cut at a simple git
tutorial.", 2005-05-31) and then slightly reworded at f35ca9ed3e
("tutorial.txt: start describing how to copy repositories",
2005-06-01) is that
* there are unfortunate occasions in which the porcelain is not
flushing, and
* the knowledge of what the plumbing does is a good thing to have
for such occasions. It will help you figure out what needs to be
done.
The rewritten text means a very different thing, at least to me.
* there are things plumbing does for you.
* the knowledge of what they are helps when the porcelain isn't
flushing.
Just dropping "for" instead of adding "you" may make it easier to
read, grammatically more correct and retain the original intent
better, I suspect.
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Johannes Schindelin @ 2016-11-10 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Andreas Schwab, Jeff King, Johannes Sixt, git
In-Reply-To: <xmqqbmxn6t11.fsf@gitster.mtv.corp.google.com>
Hi all,
On Thu, 10 Nov 2016, Junio C Hamano wrote:
> Andreas Schwab <schwab@suse.de> writes:
>
> > Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end
> > of test case") added a kill command to clean up after the test, but this
> > can fail if the sleep command exits before the cleanup is executed.
> > Ignore the error from the kill command.
> >
> > Explicitly check for the existence of the pid file to test that the merge
> > driver was actually called.
> >
> > Signed-off-by: Andreas Schwab <schwab@suse.de>
> > ---
>
> OK. sleep.pid is a reasonable easy-to-access side effect we can
> observe to make sure that the sleep-one-second merge driver was
> indeed invoked, which was missing from the earlier round.
No, this is incorrect. The condition that we need to know applies is that
the script is still running, and blocking if the bug reappears.
It is not enough to test that the script *started* running. If it exits
too early, the problematic condition is never tested, and the test is
useless.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Jeff King @ 2016-11-10 21:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <xmqqk2cb57jz.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 01:49:52PM -0800, Junio C Hamano wrote:
> Yes, I recall the IIS one raised and discussed at least twice on the
> list in the past, and it sounded that we want some solution to that.
The patches had some issues. I suspect the population of people who want
to run a git server on IIS is relatively small. I am content to wait for
somebody who has such a setup to produce a working patch.
> > 3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
> > a bundle file (Apache gives you a 404, lighttpd serves the bundle).
>
> That's a bad one. Do we want a client-side "I am connecting to a
> site that knows how to talk smart-http" option or something to work
> it around?
It doesn't matter because we don't actually support fetching HTTP
bundles via "git fetch" yet.
But I ran it across the issue and did make such a fix when I was
implementing that feature long ago. See the discussion of "surprise" in
[1] and [2].
Wow, that series is exactly 5 years old today. Have I really been
procrastinating on re-rolling it that long? Yikes.
-Peff
[1] http://public-inbox.org/git/20111110075052.GI27950@sigill.intra.peff.net/
[2] http://public-inbox.org/git/20111110075052.GI27950@sigill.intra.peff.net/
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Matt McCutchen @ 2016-11-10 21:53 UTC (permalink / raw)
To: git
In-Reply-To: <1477523244.2764.114.camel@mattmccutchen.net>
On Wed, 2016-10-26 at 19:07 -0400, Matt McCutchen wrote:
> Maybe we would never hit any of these problems in practice, but they
> give me a bad enough feeling that I'm planning to write my own tool
> that tracks the upstream commit ID in a file (like a submodule) and
> doesn't generate any extra commits. Without generating extra commits,
> the only place to store the upstream content in the superproject would
> be in another subtree, which would take up disk space in every working
> tree unless developers manually set skip-worktree. I think I prefer to
> not store the upstream content and just have the tool fetch it from a
> local subproject repository each time it's needed.
>
> I'll of course post the tool on the web and would be happy to see it
> integrated into "git subtree" if that makes sense, but I don't know how
> much time I'd be willing to put into making that happen.
I have named my tool "git subtree-lite" and posted it here:
https://mattmccutchen.net/utils/git-subtree-lite.git/
For now, please email any bug reports, enhancement requests, or
proposed patches to me. I have philosophical concerns about hosting my
own projects on services I don't control and practical concerns about a
few "forge" apps that I looked into installing on my own web site, but
if people are seriously interested in collaborating on this, I'll work
something out.
Matt
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Junio C Hamano @ 2016-11-10 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <20161110214345.cau5i4eybqdv74k3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I however do not know what the universally available simplest dummy
>> HTTP server would be. There probably are better alternative than
>> Apache with distro-customized ways of configuration that we have to
>> adjust.
>>
>> A solution around HTTP::Server::Simple sounds attractive but is it
>> a realistic alternative or too much effort required? I dunno.
>
> I'm less concerned about the amount of effort (though I agree it may be
> a blocker) than about the fact that it may not behave similarly to real
> servers.
I was wondering if it takes too much effort to make it behave
similarly to real servers, so I guess we are on the same page.
Yes, I recall the IIS one raised and discussed at least twice on the
list in the past, and it sounded that we want some solution to that.
> 3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
> a bundle file (Apache gives you a 404, lighttpd serves the bundle).
That's a bad one. Do we want a client-side "I am connecting to a
site that knows how to talk smart-http" option or something to work
it around?
^ permalink raw reply
* Re: [git-for-windows] [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Johannes Schindelin @ 2016-11-10 21:39 UTC (permalink / raw)
To: Lars Schneider; +Cc: git-for-windows, Git Mailing List, me
In-Reply-To: <B2BEB5B4-5CF0-4CD7-A8E2-50D51E00D2FF@gmail.com>
Hi Lars,
On Wed, 9 Nov 2016, Lars Schneider wrote:
> On 05 Nov 2016, at 10:50, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>
> > I finally got around to rebase the Windows-specific patches (which seem to
> > not make it upstream as fast as we get new ones) on top of upstream Git
> > v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
> >
> > https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
>
>
> I tested a new feature in 2.11 on Windows today and it failed. After some
> confusion I realized that the feature is not on your 2.11 branch.
Oops. That must have been a major snafu on my side, very sorry for that.
I just tagged v2.11.0-rc0.windows.2 in https://github.com/dscho/git and
will make a new prerelease tomorrow.
My apologies!
Dscho
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-10 21:43 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqk2cgc95m.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I'll report back an updated schedule when able.
I pushed some updates out on 'master' today. Between 'master' and
'pu' on the first-parent history there is a merge 52975d2b1f ("Merge
branch 'ls/macos-update' into jch", 2016-11-10) and that matches
what I expect to be in -rc1 tomorrow (modulo RelNotes and the actual
version tag), unless there is a showstopper regresion reported, in
which case we may want to first look into reverting the whole series
that introduced the regression before considering to pile on fix-up
patches.
http://tinyurl.com/gitCal has been redrawn, with -rc1 scheduled for
tomorrow on 11th, -rc2 on the 17th, and final on the 23rd.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Jeff King @ 2016-11-10 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <xmqq1syj6mvq.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 01:33:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > IMHO, the value in the http tests is not testing the server side, but
> > the client side. Without being able to set up a dummy HTTP server, we do
> > not have any way to exercise the client side of git-over-http at all.
> > And people on macOS _do_ use that. :)
>
> Amen to that.
>
> I however do not know what the universally available simplest dummy
> HTTP server would be. There probably are better alternative than
> Apache with distro-customized ways of configuration that we have to
> adjust.
>
> A solution around HTTP::Server::Simple sounds attractive but is it
> a realistic alternative or too much effort required? I dunno.
I'm less concerned about the amount of effort (though I agree it may be
a blocker) than about the fact that it may not behave similarly to real
servers. We have had real bugs and surprises with the way that various
web servers implement things. A few that come to mind are:
1. Buffering/deadlock issues between the webserver and the CGI (this
was an issue with Apache, but I don't know about other servers).
2. The handling of CONTENT_LENGTH with chunked-encoding (this is still
an issue with IIS).
3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
a bundle file (Apache gives you a 404, lighttpd serves the bundle).
Ideally we'd test against a lot of different webservers, but that's
expensive (in CPU, but also in developer time). But I'd guess that
Apache is at least more representative than HTTP::Server::Simple of real
servers in the wild.
So if we had a simple fallback in addition to Apache, I'd be OK with
that. But we still have a problem that (say) people on MacOS would never
actually test against Apache, because it's not supported there.
-Peff
^ permalink raw reply
* Re: [PATCH v1 0/2] Fix default macOS build locally and on Travis CI
From: Junio C Hamano @ 2016-11-10 21:34 UTC (permalink / raw)
To: Lars Schneider; +Cc: git, tboegi
In-Reply-To: <584C7DBA-71FE-4E66-85DC-EA22A6D2BB80@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> I've followed what was available at the public-inbox archive, but it
>> is unclear what the conclusion was.
>>
>> For the first one your "how about" non-patch, to which Peff said
>> "that's simple and good", looked good to me as well, but is it
>> available as a final patch that I can just take and apply (otherwise
>> I think I can do the munging myself, but I'd rather be spoon-fed
>> when able ;-).
>
> Sure! Here you go:
> http://public-inbox.org/git/20161110111348.61580-1-larsxschneider@gmail.com/
>
>
>> I do not have a strong opinion on the second one. For an interim
>> solution, disabling webserver tests certainly is expedite and safe,
>> so I am fine taking it as-is, but I may have missed strong
>> objections.
>
> I haven't seen strong objections either. Just for reference, here is the patch:
> http://public-inbox.org/git/20161017002550.88782-3-larsxschneider@gmail.com/
Thanks. Picked up both of them.
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Junio C Hamano @ 2016-11-10 21:33 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <20161110161012.jube4bwbww2wa2ew@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> IMHO, the value in the http tests is not testing the server side, but
> the client side. Without being able to set up a dummy HTTP server, we do
> not have any way to exercise the client side of git-over-http at all.
> And people on macOS _do_ use that. :)
Amen to that.
I however do not know what the universally available simplest dummy
HTTP server would be. There probably are better alternative than
Apache with distro-customized ways of configuration that we have to
adjust.
A solution around HTTP::Server::Simple sounds attractive but is it
a realistic alternative or too much effort required? I dunno.
^ permalink raw reply
* [PATCH 04/35] attr.c: explain the lack of attr-name syntax check in parse_attr()
From: Stefan Beller @ 2016-11-10 20:33 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@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: Junio C Hamano <gitster@pobox.com>
---
attr.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/attr.c b/attr.c
index 4ae7801..05db667 100644
--- a/attr.c
+++ b/attr.c
@@ -183,6 +183,12 @@ static const char *parse_attr(const char *src, int lineno, const char *cp,
return NULL;
}
} else {
+ /*
+ * As this function is always called twice, once with
+ * e == NULL in the first pass and then e != NULL in
+ * the second pass, no need for invalid_attr_name()
+ * check here.
+ */
if (*cp == '-' || *cp == '!') {
e->setto = (*cp == '-') ? ATTR__FALSE : ATTR__UNSET;
cp++;
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 07/35] attr.c: simplify macroexpand_one()
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@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: Junio C Hamano <gitster@pobox.com>
---
attr.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/attr.c b/attr.c
index 95416d3..7bfeef3 100644
--- a/attr.c
+++ b/attr.c
@@ -701,24 +701,21 @@ static int fill(const char *path, int pathlen, int basename_offset,
static int macroexpand_one(int nr, int rem)
{
struct attr_stack *stk;
- struct match_attr *a = NULL;
int i;
if (check_all_attr[nr].value != ATTR__TRUE ||
!check_all_attr[nr].attr->maybe_macro)
return rem;
- for (stk = attr_stack; !a && stk; stk = stk->prev)
- for (i = stk->num_matches - 1; !a && 0 <= i; i--) {
+ for (stk = attr_stack; stk; stk = stk->prev) {
+ for (i = stk->num_matches - 1; 0 <= i; i--) {
struct match_attr *ma = stk->attrs[i];
if (!ma->is_macro)
continue;
if (ma->u.attr->attr_nr == nr)
- a = ma;
+ return fill_one("expand", ma, rem);
}
-
- if (a)
- rem = fill_one("expand", a, rem);
+ }
return rem;
}
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 03/35] attr.c: update a stale comment on "struct match_attr"
From: Stefan Beller @ 2016-11-10 20:33 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@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: Junio C Hamano <gitster@pobox.com>
---
attr.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 45aec1b..4ae7801 100644
--- a/attr.c
+++ b/attr.c
@@ -131,9 +131,8 @@ struct pattern {
* If is_macro is true, then u.attr is a pointer to the git_attr being
* defined.
*
- * If is_macro is false, then u.pattern points at the filename pattern
- * to which the rule applies. (The memory pointed to is part of the
- * memory block allocated for the match_attr instance.)
+ * If is_macro is false, then u.pat is the filename pattern to which the
+ * rule applies.
*
* In either case, num_attr is the number of attributes affected by
* this rule, and state is an array listing them. The attributes are
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 08/35] attr.c: tighten constness around "git_attr" structure
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
attr.c | 2 +-
attr.h | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 7bfeef3..5c35d42 100644
--- a/attr.c
+++ b/attr.c
@@ -43,7 +43,7 @@ static int cannot_trust_maybe_real;
static struct git_attr_check *check_all_attr;
static struct git_attr *(git_attr_hash[HASHSIZE]);
-char *git_attr_name(struct git_attr *attr)
+const char *git_attr_name(const struct git_attr *attr)
{
return attr->name;
}
diff --git a/attr.h b/attr.h
index 8b08d33..00d7a66 100644
--- a/attr.h
+++ b/attr.h
@@ -25,7 +25,7 @@ extern const char git_attr__false[];
* Unset one is returned as NULL.
*/
struct git_attr_check {
- struct git_attr *attr;
+ const struct git_attr *attr;
const char *value;
};
@@ -34,7 +34,7 @@ struct git_attr_check {
* return value is a pointer to a null-delimited string that is part
* of the internal data structure; it should not be modified or freed.
*/
-char *git_attr_name(struct git_attr *);
+extern const char *git_attr_name(const struct git_attr *);
int git_check_attr(const char *path, int, struct git_attr_check *);
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 15/35] attr: add counted string version of git_check_attr()
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
Often a potential caller has <path, pathlen> 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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
attr.c | 23 ++++++++++++++---------
attr.h | 1 +
2 files changed, 15 insertions(+), 9 deletions(-)
diff --git a/attr.c b/attr.c
index d427798..9bec243 100644
--- a/attr.c
+++ b/attr.c
@@ -734,20 +734,19 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)
* check_all_attr. If num is non-zero, only attributes in check[] are
* collected. Otherwise all attributes are collected.
*/
-static void collect_some_attrs(const char *path, int num,
+static void collect_some_attrs(const char *path, int pathlen, int num,
struct git_attr_check_elem *check)
{
struct attr_stack *stk;
- int i, pathlen, rem, dirlen;
+ int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
- for (cp = path; *cp; cp++) {
+ for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
last_slash = cp;
}
- pathlen = cp - path;
if (last_slash) {
basename_offset = last_slash + 1 - path;
dirlen = last_slash - path;
@@ -778,12 +777,12 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-static int git_check_attrs(const char *path, int num,
+static int git_check_attrs(const char *path, int pathlen, int num,
struct git_attr_check_elem *check)
{
int i;
- collect_some_attrs(path, num, check);
+ collect_some_attrs(path, pathlen, num, check);
for (i = 0; i < num; i++) {
const char *value = check_all_attr[check[i].attr->attr_nr].value;
@@ -800,7 +799,7 @@ void git_all_attrs(const char *path, struct git_attr_check *check)
int i;
git_attr_check_clear(check);
- collect_some_attrs(path, 0, NULL);
+ collect_some_attrs(path, strlen(path), 0, NULL);
for (i = 0; i < attr_nr; i++) {
const char *name = check_all_attr[i].attr->name;
@@ -825,10 +824,16 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-int git_check_attr(const char *path, struct git_attr_check *check)
+int git_check_attr_counted(const char *path, int pathlen,
+ struct git_attr_check *check)
{
check->finalized = 1;
- return git_check_attrs(path, check->check_nr, check->check);
+ return git_check_attrs(path, pathlen, check->check_nr, check->check);
+}
+
+int git_check_attr(const char *path, struct git_attr_check *check)
+{
+ return git_check_attr_counted(path, strlen(path), check);
}
struct git_attr_check *git_attr_check_initl(const char *one, ...)
diff --git a/attr.h b/attr.h
index 506db0c..c84f164 100644
--- a/attr.h
+++ b/attr.h
@@ -38,6 +38,7 @@ struct git_attr_check {
extern struct git_attr_check *git_attr_check_initl(const char *, ...);
extern int git_check_attr(const char *path, struct git_attr_check *);
+extern int git_check_attr_counted(const char *, int, struct git_attr_check *);
extern struct git_attr_check *git_attr_check_alloc(void);
extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 01/35] commit.c: use strchrnul() to scan for one line
From: Stefan Beller @ 2016-11-10 20:33 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@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: Junio C Hamano <gitster@pobox.com>
---
commit.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/commit.c b/commit.c
index 856fd4a..41b2fdd 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
p++;
if (*p) {
p = skip_blank_lines(p + 2);
- for (eol = p; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
+ eol = strchrnul(p, '\n');
} else
eol = p;
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 14/35] attr: retire git_check_attrs() API
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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 attribute described by a
- `git_attr_check` object can be retrieved via
- `git_attr_name(check[i].attr)`. (Please note that no items will be
- returned for unset attributes, so `ATTR_UNSET()` will return false
- for all returned `git_array_check` objects.)
+* Iterate over the `git_attr_check.check[]` array to examine
+ the attribute names and values. The name of the attribute
+ described by a `git_attr_check.check[]` object can be retrieved via
+ `git_attr_name(check->check[i].attr)`. (Please note that no items
+ will be returned for unset attributes, so `ATTR_UNSET()` will return
+ false for all returned `git_array_check` objects.)
-* Free the `git_array_check` array.
+* Free the `git_array_check` by calling `git_attr_check_free()`.
diff --git a/attr.c b/attr.c
index 76f0d6b..d427798 100644
--- a/attr.c
+++ b/attr.c
@@ -778,7 +778,8 @@ static void collect_some_attrs(const char *path, int num,
rem = fill(path, pathlen, basename_offset, stk, rem);
}
-int git_check_attrs(const char *path, int num, struct git_attr_check_elem *check)
+static int git_check_attrs(const char *path, int num,
+ struct git_attr_check_elem *check)
{
int i;
diff --git a/attr.h b/attr.h
index 0d94077..506db0c 100644
--- a/attr.h
+++ b/attr.h
@@ -52,8 +52,6 @@ extern void git_attr_check_free(struct git_attr_check *);
*/
extern const char *git_attr_name(const struct git_attr *);
-int git_check_attrs(const char *path, int, struct git_attr_check_elem *);
-
/*
* Retrieve all attributes that apply to the specified path.
* check holds the attributes and their values.
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 21/35] attr.c: correct ugly hack for git_all_attrs()
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
attr.c | 24 ++++++++----------------
1 file changed, 8 insertions(+), 16 deletions(-)
diff --git a/attr.c b/attr.c
index e18098c..48460d2 100644
--- a/attr.c
+++ b/attr.c
@@ -748,8 +748,8 @@ static int attr_check_is_dynamic(const struct git_attr_check *check)
/*
* Collect attributes for path into the array pointed to by
- * check_all_attr. If num is non-zero, only attributes in check[] are
- * collected. Otherwise all attributes are collected.
+ * check_all_attr. If check is not NULL, only attributes in
+ * check[] are collected. Otherwise all attributes are collected.
*/
static void collect_some_attrs(const char *path, int pathlen,
struct git_attr_check *check)
@@ -759,17 +759,6 @@ static void collect_some_attrs(const char *path, int pathlen,
int i, rem, dirlen;
const char *cp, *last_slash = NULL;
int basename_offset;
- int num;
- struct git_attr_check_elem *celem;
-
- if (!check) {
- /* Yuck - ugly git_all_attrs() hack! */
- celem = NULL;
- num = 0;
- } else {
- celem = check->check;
- num = check->check_nr;
- }
for (cp = path; cp < path + pathlen; cp++) {
if (*cp == '/' && cp[1])
@@ -786,9 +775,12 @@ static void collect_some_attrs(const char *path, int pathlen,
prepare_attr_stack(path, dirlen);
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- if (num && !cannot_trust_maybe_real) {
+
+ if (check && !cannot_trust_maybe_real) {
+ struct git_attr_check_elem *celem = check->check;
+
rem = 0;
- for (i = 0; i < num; i++) {
+ for (i = 0; i < check->check_nr; i++) {
if (!celem[i].attr->maybe_real) {
struct git_attr_check_elem *c;
c = check_all_attr + celem[i].attr->attr_nr;
@@ -796,7 +788,7 @@ static void collect_some_attrs(const char *path, int pathlen,
rem++;
}
}
- if (rem == num)
+ if (rem == check->check_nr)
return;
}
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 24/35] attr.c: outline the future plans by heavily commenting
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@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: Junio C Hamano <gitster@pobox.com>
---
attr.c | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/attr.c b/attr.c
index 246a5d0..60d7eec 100644
--- a/attr.c
+++ b/attr.c
@@ -30,6 +30,11 @@ static const char git_attr__unknown[] = "(builtin)unknown";
#define DEBUG_ATTR 0
#endif
+/*
+ * NEEDSWORK: the global dictionary of the interned attributes
+ * must stay a singleton even after we become thread-ready.
+ * Access to these must be surrounded with mutex when it happens.
+ */
struct git_attr {
struct git_attr *next;
unsigned h;
@@ -39,10 +44,19 @@ struct git_attr {
char name[FLEX_ARRAY];
};
static int attr_nr;
+static struct git_attr *(git_attr_hash[HASHSIZE]);
+
+/*
+ * NEEDSWORK: maybe-real, maybe-macro are not property of
+ * an attribute, as it depends on what .gitattributes are
+ * read. Once we introduce per git_attr_check attr_stack
+ * and check_all_attr, the optimization based on them will
+ * become unnecessary and can go away. So is this variable.
+ */
static int cannot_trust_maybe_real;
+/* NEEDSWORK: This will become per git_attr_check */
static struct git_attr_check_elem *check_all_attr;
-static struct git_attr *(git_attr_hash[HASHSIZE]);
const char *git_attr_name(const struct git_attr *attr)
{
@@ -117,6 +131,11 @@ static struct git_attr *git_attr_internal(const char *name, int len)
a->maybe_real = 0;
git_attr_hash[pos] = a;
+ /*
+ * NEEDSWORK: per git_attr_check check_all_attr
+ * will be initialized a lot more lazily, not
+ * like this, and not here.
+ */
REALLOC_ARRAY(check_all_attr, attr_nr);
check_all_attr[a->attr_nr].attr = a;
check_all_attr[a->attr_nr].value = ATTR__UNKNOWN;
@@ -329,6 +348,7 @@ static struct match_attr *parse_attr_line(const char *line, const char *src,
* .gitignore file and info/excludes file as a fallback.
*/
+/* NEEDSWORK: This will become per git_attr_check */
static struct attr_stack {
struct attr_stack *prev;
char *origin;
@@ -393,6 +413,24 @@ static struct attr_stack *read_attr_from_array(const char **list)
return res;
}
+/*
+ * NEEDSWORK: these two are tricky. The callers assume there is a
+ * single, system-wide global state "where we read attributes from?"
+ * and when the state is flipped by calling git_attr_set_direction(),
+ * attr_stack is discarded so that subsequent attr_check will lazily
+ * read from the right place. And they do not know or care who called
+ * by them uses the attribute subsystem, hence have no knowledge of
+ * existing git_attr_check instances or future ones that will be
+ * created).
+ *
+ * Probably we need a thread_local that holds these two variables,
+ * and a list of git_attr_check instances (which need to be maintained
+ * by hooking into git_attr_check_alloc(), git_attr_check_initl(), and
+ * git_attr_check_clear(). Then git_attr_set_direction() updates the
+ * fields in that thread_local for these two variables, iterate over
+ * all the active git_attr_check instances and discard the attr_stack
+ * they hold. Yuck, but it sounds doable.
+ */
static enum git_attr_direction direction;
static struct index_state *use_index;
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 25/35] attr: make git_check_attr_counted static
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
It's not used outside the attr code, so let's keep it private.
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
attr.c | 4 ++--
attr.h | 1 -
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/attr.c b/attr.c
index 60d7eec..19a3651 100644
--- a/attr.c
+++ b/attr.c
@@ -892,8 +892,8 @@ void git_attr_set_direction(enum git_attr_direction new, struct index_state *ist
use_index = istate;
}
-int git_check_attr_counted(const char *path, int pathlen,
- struct git_attr_check *check)
+static int git_check_attr_counted(const char *path, int pathlen,
+ struct git_attr_check *check)
{
check->finalized = 1;
return git_check_attrs(path, pathlen, check);
diff --git a/attr.h b/attr.h
index 1c6a6d8..7416b14 100644
--- a/attr.h
+++ b/attr.h
@@ -48,7 +48,6 @@ struct git_attr_check {
extern struct git_attr_check *git_attr_check_initl(const char *, ...);
extern int git_check_attr(const char *path, struct git_attr_check *);
-extern int git_check_attr_counted(const char *, int, struct git_attr_check *);
extern struct git_attr_check *git_attr_check_alloc(void);
extern struct git_attr_check_elem *git_attr_check_append(struct git_attr_check *, const struct git_attr *);
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 23/35] attr.c: always pass check[] to collect_some_attrs()
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
From: Junio C Hamano <gitster@pobox.com>
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 <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
attr.c | 37 +++++++++++++++++++------------------
1 file changed, 19 insertions(+), 18 deletions(-)
diff --git a/attr.c b/attr.c
index 5bb925d..246a5d0 100644
--- a/attr.c
+++ b/attr.c
@@ -756,11 +756,12 @@ static void empty_attr_check_elems(struct git_attr_check *check)
/*
* Collect attributes for path into the array pointed to by
- * check_all_attr. If check is not NULL, only attributes in
- * check[] are collected. Otherwise all attributes are collected.
+ * check_all_attr. If collect_all is zero, only attributes in
+ * check[] are collected. Otherwise, check[] is cleared and
+ * any and all attributes that are visible are collected in it.
*/
static void collect_some_attrs(const char *path, int pathlen,
- struct git_attr_check *check)
+ struct git_attr_check *check, int collect_all)
{
struct attr_stack *stk;
@@ -781,10 +782,11 @@ static void collect_some_attrs(const char *path, int pathlen,
}
prepare_attr_stack(path, dirlen);
+
for (i = 0; i < attr_nr; i++)
check_all_attr[i].value = ATTR__UNKNOWN;
- if (check && !cannot_trust_maybe_real) {
+ if (!collect_all && !cannot_trust_maybe_real) {
struct git_attr_check_elem *celem = check->check;
rem = 0;
@@ -803,6 +805,17 @@ static void collect_some_attrs(const char *path, int pathlen,
rem = attr_nr;
for (stk = attr_stack; 0 < rem && stk; stk = stk->prev)
rem = fill(path, pathlen, basename_offset, stk, rem);
+
+ if (collect_all) {
+ empty_attr_check_elems(check);
+ for (i = 0; i < attr_nr; i++) {
+ const struct git_attr *attr = check_all_attr[i].attr;
+ const char *value = check_all_attr[i].value;
+ if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
+ continue;
+ git_attr_check_append(check, attr)->value = value;
+ }
+ }
}
static int git_check_attrs(const char *path, int pathlen,
@@ -811,7 +824,7 @@ static int git_check_attrs(const char *path, int pathlen,
int i;
struct git_attr_check_elem *celem = check->check;
- collect_some_attrs(path, pathlen, check);
+ collect_some_attrs(path, pathlen, check, 0);
for (i = 0; i < check->check_nr; i++) {
const char *value = check_all_attr[celem[i].attr->attr_nr].value;
@@ -825,19 +838,7 @@ static int git_check_attrs(const char *path, int pathlen,
void git_all_attrs(const char *path, struct git_attr_check *check)
{
- int i;
-
- git_attr_check_clear(check);
- collect_some_attrs(path, strlen(path), NULL);
-
- for (i = 0; i < attr_nr; i++) {
- const char *name = check_all_attr[i].attr->name;
- const char *value = check_all_attr[i].value;
- if (value == ATTR__UNSET || value == ATTR__UNKNOWN)
- continue;
- git_attr_check_append(check, git_attr(name));
- check->check[check->check_nr - 1].value = value;
- }
+ collect_some_attrs(path, strlen(path), check, 1);
}
void git_attr_set_direction(enum git_attr_direction new, struct index_state *istate)
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 29/35] pathspec: move long magic parsing out of prefix_pathspec
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
`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 <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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,
+ &endptr, 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,
- &endptr, 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++;
+ eat_long_magic(item, elt, &magic, &pathspec_prefix, ©from, &long_magic_end);
} else {
/* shorthand */
for (copyfrom = elt + 1;
--
2.10.1.469.g00a8914
^ permalink raw reply related
* [PATCH 30/35] pathspec: move prefix check out of the inner loop
From: Stefan Beller @ 2016-11-10 20:34 UTC (permalink / raw)
To: gitster, bmwill, pclouds; +Cc: git, Stefan Beller
In-Reply-To: <20161110203428.30512-1-sbeller@google.com>
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 <sbeller@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
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,
+ &endptr, 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,
- &endptr, 10);
- if (endptr - copyfrom != len)
- die(_("invalid parameter for pathspec magic 'prefix'"));
- /* "i" would be wrong, but it does not matter */
- break;
- }
}
if (ARRAY_SIZE(pathspec_magic) <= i)
die(_("Invalid pathspec magic '%.*s' in '%s'"),
--
2.10.1.469.g00a8914
^ 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