* Re: [PATCH 0/2] config includes, take 2
From: Junio C Hamano @ 2012-02-06 7:41 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> ... But the point of
> duplicate suppression was that individual config files wouldn't have to
> know or care what was being included elsewhere.
I think you wanted to say "the point of inclusion mechanism" is that
individual configuration files would not have to know, and I think I
agree.
> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.
Yeah, I stand corrected. It was a bad suggestion.
Thanks.
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Jeff King @ 2012-02-06 7:45 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120206071302.GA10447@sigill.intra.peff.net>
On Mon, Feb 06, 2012 at 02:13:02AM -0500, Jeff King wrote:
> > BTW, writing that helped me notice two bugs in your patch:
> >
> > 1. You read up to 47 bytes into the buffer without ever checking
> > whether size >= 47.
> >
> > 2. You never check whether the object you read from read_sha1_file is
> > actually a tag.
>
> Hmm, the "filter->lines" code for "git tag -n" makes a similar error. It
> should probably print nothing for objects that are not tags.
Ugh, this part of builtin/tag.c is riddled with small bugs. I'm
preparing a series that will fix them, and hopefully it should make
building your points-at patch on top much more pleasant.
-Peff
^ permalink raw reply
* Re: git-svn: t9155 fails against subversion 1.7.0
From: Frans Klaver @ 2012-02-06 8:02 UTC (permalink / raw)
To: Robin H. Johnson; +Cc: Git Mailing List, Erik Wong, Jonathan Nieder, Ben Walton
In-Reply-To: <robbat2-20120205T212444-523294742Z@orbis-terrarum.net>
Hi Robin,
On Sun, Feb 5, 2012 at 10:25 PM, Robin H. Johnson <robbat2@gentoo.org> wrote:
> On Thu, Nov 10, 2011 at 07:02:13AM +0100, Frans Klaver wrote:
>> I missed $gmane/184644 in my search for this issue.
> Did you make any progress in fixing this?
I haven't invested time in this, and the tests still seem to fail on
subversion 1.7.x. Maybe one of the people involved in $gmane/184644
knows more?
Cheers,
Frans
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Jeff King @ 2012-02-06 8:11 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120206074558.GA24535@sigill.intra.peff.net>
On Mon, Feb 06, 2012 at 02:45:58AM -0500, Jeff King wrote:
> > Hmm, the "filter->lines" code for "git tag -n" makes a similar error. It
> > should probably print nothing for objects that are not tags.
>
> Ugh, this part of builtin/tag.c is riddled with small bugs. I'm
> preparing a series that will fix them, and hopefully it should make
> building your points-at patch on top much more pleasant.
So here's what I ended up with:
[1/3]: tag: fix output of "tag -n" when errors occur
[2/3]: tag: die when listing missing or corrupt objects
[3/3]: tag: don't show non-tag contents with "-n"
I had hoped to have a 4th patch teach "tag -n" to use parse_object
instead of read_sha1_file directly. That way we could avoid reading tag
objects multiple times when things like "--contains" or "--points-at"
are used. But we don't actually cache the body of an annotated tag,
only its headers. So the "tag -n" code has to read the object fresh.
I do still think it's worth using the parse_object interface for the
"--points-at" feature.
-Peff
^ permalink raw reply
* [PATCH 1/3] tag: fix output of "tag -n" when errors occur
From: Jeff King @ 2012-02-06 8:13 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, Junio C Hamano, jasampler
In-Reply-To: <20120206081119.GA3939@sigill.intra.peff.net>
When "git tag" is instructed to print lines from annotated
tags via "-n", it first prints the tag name, then attempts
to parse and print the lines of the tag object, and then
finally adds a trailing newline.
If an error occurs, we return early from the function and
never print the newline, screwing up the output for the next
tag. Let's factor the line-printing into its own function so
we can manage the early returns better, and make sure that
we always terminate the line.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/tag.c | 66 +++++++++++++++++++++++++++++---------------------------
1 files changed, 34 insertions(+), 32 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..2250915 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -83,18 +83,45 @@ static int contains(struct commit *candidate, const struct commit_list *want)
return contains_recurse(candidate, want);
}
+static void show_tag_lines(const unsigned char *sha1, int lines)
+{
+ int i;
+ unsigned long size;
+ enum object_type type;
+ char *buf, *sp, *eol;
+ size_t len;
+
+ buf = read_sha1_file(sha1, &type, &size);
+ if (!buf || !size)
+ return;
+
+ /* skip header */
+ sp = strstr(buf, "\n\n");
+ if (!sp) {
+ free(buf);
+ return;
+ }
+ /* only take up to "lines" lines, and strip the signature */
+ size = parse_signature(buf, size);
+ for (i = 0, sp += 2; i < lines && sp < buf + size; i++) {
+ if (i)
+ printf("\n ");
+ eol = memchr(sp, '\n', size - (sp - buf));
+ len = eol ? eol - sp : size - (sp - buf);
+ fwrite(sp, len, 1, stdout);
+ if (!eol)
+ break;
+ sp = eol + 1;
+ }
+ free(buf);
+}
+
static int show_reference(const char *refname, const unsigned char *sha1,
int flag, void *cb_data)
{
struct tag_filter *filter = cb_data;
if (match_pattern(filter->patterns, refname)) {
- int i;
- unsigned long size;
- enum object_type type;
- char *buf, *sp, *eol;
- size_t len;
-
if (filter->with_commit) {
struct commit *commit;
@@ -110,33 +137,8 @@ static int show_reference(const char *refname, const unsigned char *sha1,
return 0;
}
printf("%-15s ", refname);
-
- buf = read_sha1_file(sha1, &type, &size);
- if (!buf || !size)
- return 0;
-
- /* skip header */
- sp = strstr(buf, "\n\n");
- if (!sp) {
- free(buf);
- return 0;
- }
- /* only take up to "lines" lines, and strip the signature */
- size = parse_signature(buf, size);
- for (i = 0, sp += 2;
- i < filter->lines && sp < buf + size;
- i++) {
- if (i)
- printf("\n ");
- eol = memchr(sp, '\n', size - (sp - buf));
- len = eol ? eol - sp : size - (sp - buf);
- fwrite(sp, len, 1, stdout);
- if (!eol)
- break;
- sp = eol + 1;
- }
+ show_tag_lines(sha1, filter->lines);
putchar('\n');
- free(buf);
}
return 0;
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Jeff King @ 2012-02-06 8:13 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, Junio C Hamano, jasampler
In-Reply-To: <20120206081119.GA3939@sigill.intra.peff.net>
We don't usually bother looking at tagged objects at all
when listing. However, if "-n" is specified, we open the
objects to read the annotations of the tags. If we fail to
read an object, or if the object has zero length, we simply
silently return.
The first case is an indication of a broken or corrupt repo,
and we should notify the user of the error.
The second case is OK to silently ignore; however, the
existing code leaked the buffer returned by read_sha1_file.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/tag.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 2250915..1bb42a4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -92,8 +92,12 @@ static void show_tag_lines(const unsigned char *sha1, int lines)
size_t len;
buf = read_sha1_file(sha1, &type, &size);
- if (!buf || !size)
+ if (!buf)
+ die_errno("unable to read object %s", sha1_to_hex(sha1));
+ if (!size) {
+ free(buf);
return;
+ }
/* skip header */
sp = strstr(buf, "\n\n");
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* [PATCH 3/3] tag: don't show non-tag contents with "-n"
From: Jeff King @ 2012-02-06 8:14 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, Junio C Hamano, jasampler
In-Reply-To: <20120206081119.GA3939@sigill.intra.peff.net>
When given "-n", tag will show one or more lines of the body
of an annotated tag. However, we never actually checked to
see that what we got was a tag; we might end up showing
random lines from a lightweight-tagged blob or commit.
With this patch, we'll show lines only from tag objects (but
still include non-tag objects in the listing). It might make
more sense to omit lightweight tags from the listing
entirely when "-n" is in effect. I stuck with this behavior
because it is slightly more compatible with the original
behavior.
This might be seen as a regression for people with
lightweight tags to commit, who would previously get the
subject line of the commit. The code seems to indicate that
is not expected (since it does things like parsing off
gpg signatures), but it's possible somebody has been relying
on it.
Signed-off-by: Jeff King <peff@peff.net>
---
The regression comment above makes me a little nervous. Still, if we
want to handle commits, we should do so explicitly and not munge them
with parse_signature. So I think it's a step in the right direction, and
we should let it cook for a bit and see if anybody complains.
builtin/tag.c | 2 +-
t/t7004-tag.sh | 13 +++++++++++++
2 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 1bb42a4..0a7c174 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -94,7 +94,7 @@ static void show_tag_lines(const unsigned char *sha1, int lines)
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
die_errno("unable to read object %s", sha1_to_hex(sha1));
- if (!size) {
+ if (!size || type != OBJ_TAG) {
free(buf);
return;
}
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index e93ac73..0db0f6a 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -586,6 +586,19 @@ test_expect_success \
test_cmp expect actual
'
+test_expect_success 'annotations for non-tags are empty' '
+ blob=$(git hash-object -w --stdin <<-\EOF
+ Blob paragraph 1.
+
+ Blob paragraph 2.
+ EOF
+ ) &&
+ git tag tag-blob $blob &&
+ echo "tag-blob " >expect &&
+ git tag -n1 -l tag-blob >actual &&
+ test_cmp expect actual
+'
+
# trying to verify annotated non-signed tags:
test_expect_success GPG \
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Junio C Hamano @ 2012-02-06 8:32 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <20120206081342.GB3966@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The first case is an indication of a broken or corrupt repo,
> and we should notify the user of the error.
>
> The second case is OK to silently ignore; however, the
> existing code leaked the buffer returned by read_sha1_file.
> ...
> buf = read_sha1_file(sha1, &type, &size);
> - if (!buf || !size)
> + if (!buf)
> + die_errno("unable to read object %s", sha1_to_hex(sha1));
> + if (!size) {
> + free(buf);
> return;
> + }
>
> /* skip header */
> sp = strstr(buf, "\n\n");
Hmm, a pedant in me says a tag object cannot have zero length, so the
second case is also an indication of a corrupt repository, unless the tag
happens to be a lightweight one that refers directly to a blob object that
is empty.
For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
because the definition of "first N lines" is compatible between tag and
commit for the purpose of the -n option.
For example, in the kernel repository, what would this do, I have to
wonder:
$ git tag c2.6.12 v2.6.12^{commit}
$ git tag t2.6.12 v2.6.12^{tree}
$ git tag -l -n 12 c2.6.12 t2.6.12
^ permalink raw reply
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Jeff King @ 2012-02-06 8:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <7vk4408ir6.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 12:32:13AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The first case is an indication of a broken or corrupt repo,
> > and we should notify the user of the error.
> >
> > The second case is OK to silently ignore; however, the
> > existing code leaked the buffer returned by read_sha1_file.
> > ...
> > buf = read_sha1_file(sha1, &type, &size);
> > - if (!buf || !size)
> > + if (!buf)
> > + die_errno("unable to read object %s", sha1_to_hex(sha1));
> > + if (!size) {
> > + free(buf);
> > return;
> > + }
> >
> > /* skip header */
> > sp = strstr(buf, "\n\n");
>
> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.
Yes. Or alternatively, it should just be caught in the strstr() case
below (which would silently ignore it).
> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> because the definition of "first N lines" is compatible between tag and
> commit for the purpose of the -n option.
Yup. See patch 3. :)
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Junio C Hamano @ 2012-02-06 8:36 UTC (permalink / raw)
To: Jeff King; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <7vk4408ir6.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Hmm, a pedant in me says a tag object cannot have zero length, so the
> second case is also an indication of a corrupt repository, unless the tag
> happens to be a lightweight one that refers directly to a blob object that
> is empty.
>
> For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> because the definition of "first N lines" is compatible between tag and
> commit for the purpose of the -n option.
Ahh, Ok, your 3/3 addresses this exact issue.
I do not object to silently return when the object is not OBJ_TAG (even
though I slightly prefer showing the first N lines of commit log contents
for OBJ_COMMIT lightweight tag), but I still think it should be warned
just like a corruption when we see (type == OBJ_TAG && !size).
^ permalink raw reply
* Re: [PATCH 1/6] read-cache: use sha1file for sha1 calculation
From: Nguyen Thai Ngoc Duy @ 2012-02-06 8:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vsjio8leo.fsf@alter.siamese.dyndns.org>
2012/2/6 Junio C Hamano <gitster@pobox.com>:
> This is open source, and I wouldn't stop you from spending time on anything
> that interests you.
Well fun stuff is more interesting to do.. but I guess that's about
it. More time cut down requires bigger changes that does not fit in a
few hours of work.
> But having said that, if you have extra Git time, I would still rather see
> you spend it first on tying up loose ends of your topics in flight and
> on helping others that touch parts that are related to areas that you have
> already thought about, namely:
>
> (1) nd/commit-ignore-i-t-a, which I think should be marketted as fixing
> an earlier UI mistake and presented with a clean migration path to
> make the updated behaviour the default in the future; and
Yeah, I was avoiding the deprecation procedure (plus providing a
convincing argument to push it forward). Need to look up old emails..
> (2) the negative pathspec thing that resurfaced in disguise as Albert
> Yale's "grep --exclude" series.
This is pure headache. Can't avoid it forever, I guess.
> *1* A possible approach might be to stuff unmodified trees in the index
> without exploding them into its components, and as entries are modified,
> lazily expand these "tree" entries, while ensuring the "unmodified" parts
> remain unmodified by turning the files in the working tree read-only and
> requiring the user to say "git edit" or "git open" or something before
> starting to edit. But as I said, I consider this not an ultra-urgent
> issue, so I haven't thought things through yet.
A sparse index is something that may be achieved with narrow clone (or
narrow checkout in full clone) because by nature we can't have full
index in narrow clone. That may be the right way to go.
--
Duy
^ permalink raw reply
* Re: [PATCH 2/3] tag: die when listing missing or corrupt objects
From: Jeff King @ 2012-02-06 8:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Tom Grennan, git, jasampler
In-Reply-To: <7vfweo8ikq.fsf@alter.siamese.dyndns.org>
On Mon, Feb 06, 2012 at 12:36:05AM -0800, Junio C Hamano wrote:
> > For that matter, shouldn't we make sure that the type is OBJ_TAG? It might
> > make sense to allow OBJ_COMMIT (i.e. lightweight tag to a commit) as well,
> > because the definition of "first N lines" is compatible between tag and
> > commit for the purpose of the -n option.
>
> Ahh, Ok, your 3/3 addresses this exact issue.
>
> I do not object to silently return when the object is not OBJ_TAG (even
> though I slightly prefer showing the first N lines of commit log contents
> for OBJ_COMMIT lightweight tag), but I still think it should be warned
> just like a corruption when we see (type == OBJ_TAG && !size).
OK, that's easy enough to do. Should we show lightweight tags to commits
for backwards compatibility (and just drop the parse_signature junk in
that case)? The showing of blobs or trees is the really bad thing, I
think.
-Peff
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Michael Haggerty @ 2012-02-06 8:51 UTC (permalink / raw)
To: Jeff King; +Cc: Andrew Ardill, Junio C Hamano, git
In-Reply-To: <20120206050637.GA4263@sigill.intra.peff.net>
On 02/06/2012 06:06 AM, Jeff King wrote:
> I kind of agree that we shouldn't be unnecessarily restrictive. On the
> other hand, I am stretching to find the plausible reason that one would
> want to throw away the normal convention. Code aside, it simply
> introduces a slight communication barrier when talking with other git
> users, and for that reason should be something you don't do lightly. I
> don't recall seeing anybody complain seriously about it in the past six
> years of git's existence.
In the real-world situation when I noticed this bug, I wasn't trying to
use a nonstandard name for "master". What I was doing is importing a
snapshot of some code from another non-git project onto a "vendor
branch", which I knew I would later want to merge into my own work
(which I planned to do on master).
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 6/6] Automatically switch to crc32 checksum for index when it's too large
From: Nguyen Thai Ngoc Duy @ 2012-02-06 8:54 UTC (permalink / raw)
To: Dave Zarzycki; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <E799595D-61B3-4978-BCE1-BA6A33034B55@apple.com>
2012/2/6 Dave Zarzycki <zarzycki@apple.com>:
> Which crc32 polynomial is being used? crc32c (a.k.a. Castagnoli)? It would be great if this were the same polynomial that Intel implements in hardware via SSE4.2.
It's zlib's crc32.
> On Feb 5, 2012, at 9:48 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>
>> An experiment with -O3 is done on Intel D510@1.66GHz. At around 250k
>> entries, index reading time exceeds 0.5s. Switching to crc32 brings it
>> back lower than 0.2s.
>>
>> On 4M files index, reading time with SHA-1 takes ~8.4, crc32 2.8s.
--
Duy
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Jeff King @ 2012-02-06 8:57 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Andrew Ardill, Junio C Hamano, git
In-Reply-To: <4F2F94AC.6010800@alum.mit.edu>
On Mon, Feb 06, 2012 at 09:51:56AM +0100, Michael Haggerty wrote:
> On 02/06/2012 06:06 AM, Jeff King wrote:
> >I don't recall seeing anybody complain seriously about it in the past
> >six years of git's existence.
>
> In the real-world situation when I noticed this bug, I wasn't trying to
> use a nonstandard name for "master". What I was doing is importing a
> snapshot of some code from another non-git project onto a "vendor
> branch", which I knew I would later want to merge into my own work
> (which I planned to do on master).
Thanks, that sounds like a very reasonable use case. I stand corrected.
(For some reason I thought you ran across it accidentally while mucking
with "--edit-description", since were talking about that an unborn
branches in a nearby thread).
-Peff
PS I probably would have done it as:
git init vendor
cd vendor
import import import
cd ..
git init project
cd project
git fetch ../vendor master:vendor
but I don't think there's anything wrong with your approach (in fact,
it's slightly more efficient).
^ permalink raw reply
* Re: Bug: "git checkout -b" should be allowed in empty repo
From: Michael Haggerty @ 2012-02-06 8:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <7vk440a5qw.fsf@alter.siamese.dyndns.org>
On 02/06/2012 06:30 AM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> Sure, that's one way to do it. But I don't see any point in not allowing
>> "git checkout -b" to be another way of doing it. Is there some other use
>> case for "git checkout -b" from an unborn branch? Or is there some
>> harmful outcome that can come from doing so that we need to be
>> protecting against? Am I missing something?
>
> Mostly because it is wrong at the conceptual level to do so.
>
> git checkout -b foo
>
> is a short-hand for
>
> git checkout -b foo HEAD
>
> which is a short-hand for
>
> git branch foo HEAD &&
> git checkout foo
>
> But the last one has no chance of working if you think about it, because
> "git branch foo $start" is a way to start a branch at $start and you need
> to have something to point at with refs/heads/foo.
>
> So we are breaking the equivalence between these three only when HEAD
> points at an unborn branch.
You are thinking too much like a developer and not like a user. For a user,
git checkout -b foo
is a short-hand for
"create and check out a branch at my current state"
and the interpretation of what that means when I am on an unborn branch
seems unambiguous.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* Re: [PATCH 6/6] Automatically switch to crc32 checksum for index when it's too large
From: Dave Zarzycki @ 2012-02-06 9:07 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <CACsJy8Dv9fUzL3COZKVw_KR6aF20kHaw8M4CdBXJDA9H3fbxLw@mail.gmail.com>
On Feb 6, 2012, at 12:54 AM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> 2012/2/6 Dave Zarzycki <zarzycki@apple.com>:
>> Which crc32 polynomial is being used? crc32c (a.k.a. Castagnoli)? It would be great if this were the same polynomial that Intel implements in hardware via SSE4.2.
>
> It's zlib's crc32.
That's too bad. Zlib uses crc32, not crc32c. The Intel instruction is crc32c and is 2-3 times faster than the best software based implementation.
http://www.strchr.com/crc32_popcnt
>
>> On Feb 5, 2012, at 9:48 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
>>
>>> An experiment with -O3 is done on Intel D510@1.66GHz. At around 250k
>>> entries, index reading time exceeds 0.5s. Switching to crc32 brings it
>>> back lower than 0.2s.
>>>
>>> On 4M files index, reading time with SHA-1 takes ~8.4, crc32 2.8s.
> --
> Duy
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 6/6] Automatically switch to crc32 checksum for index when it's too large
From: Dave Zarzycki @ 2012-02-06 8:50 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <1328507319-24687-6-git-send-email-pclouds@gmail.com>
Which crc32 polynomial is being used? crc32c (a.k.a. Castagnoli)? It would be great if this were the same polynomial that Intel implements in hardware via SSE4.2.
On Feb 5, 2012, at 9:48 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:
> An experiment with -O3 is done on Intel D510@1.66GHz. At around 250k
> entries, index reading time exceeds 0.5s. Switching to crc32 brings it
> back lower than 0.2s.
>
> On 4M files index, reading time with SHA-1 takes ~8.4, crc32 2.8s.
^ permalink raw reply
* [PATCH 0/2] config includes 3: revenge of the killer includes
From: Jeff King @ 2012-02-06 9:53 UTC (permalink / raw)
To: git
So here's a third version of the config include patches that I think are
sane. For those of you who missed episode 2, our heroes decided to drop
the include-from-ref bits from the series, but otherwise incorporated
all suggestions. There was a surprise cliffhanger ending in which it was
discovered that suppressing duplicate include files was actually the
villain!
This version keeps a simple depth counter to stop cycles from causing
infinite loops, but otherwise allows multiple inclusion of files.
And now, the exciting conclusion...
[1/2]: docs: add a basic description of the config API
[2/2]: config: add include directive
-Peff
^ permalink raw reply
* [PATCH 1/2] docs: add a basic description of the config API
From: Jeff King @ 2012-02-06 9:53 UTC (permalink / raw)
To: git
In-Reply-To: <20120206095306.GA2404@sigill.intra.peff.net>
This wasn't documented at all; this is pretty bare-bones,
but it should at least give new git hackers a basic idea of
how the reading side works.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/technical/api-config.txt | 101 ++++++++++++++++++++++++++++++++
1 files changed, 101 insertions(+), 0 deletions(-)
create mode 100644 Documentation/technical/api-config.txt
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
new file mode 100644
index 0000000..f428c5c
--- /dev/null
+++ b/Documentation/technical/api-config.txt
@@ -0,0 +1,101 @@
+config API
+==========
+
+The config API gives callers a way to access git configuration files
+(and files which have the same syntax). See linkgit:git-config[1] for a
+discussion of the config file syntax.
+
+General Usage
+-------------
+
+Config files are parsed linearly, and each variable found is passed to a
+caller-provided callback function. The callback function is responsible
+for any actions to be taken on the config option, and is free to ignore
+some options (it is not uncommon for the configuration to be parsed
+several times during the run of a git program, with different callbacks
+picking out different variables useful to themselves).
+
+A config callback function takes three parameters:
+
+- the name of the parsed variable. This is in canonical "flat" form: the
+ section, subsection, and variable segments will be separated by dots,
+ and the section and variable segments will be all lowercase. E.g.,
+ `core.ignorecase`, `diff.SomeType.textconv`.
+
+- the value of the found variable, as a string. If the variable had no
+ value specified, the value will be NULL (typically this means it
+ should be interpreted as boolean true).
+
+- a void pointer passed in by the caller of the config API; this can
+ contain callback-specific data
+
+A config callback should return 0 for success, or -1 if the variable
+could not be parsed properly.
+
+Basic Config Querying
+---------------------
+
+Most programs will simply want to look up variables in all config files
+that git knows about, using the normal precedence rules. To do this,
+call `git_config` with a callback function and void data pointer.
+
+`git_config` will read all config sources in order of increasing
+priority. Thus a callback should typically overwrite previously-seen
+entries with new ones (e.g., if both the user-wide `~/.gitconfig` and
+repo-specific `.git/config` contain `color.ui`, the config machinery
+will first feed the user-wide one to the callback, and then the
+repo-specific one; by overwriting, the higher-priority repo-specific
+value is left at the end).
+
+There is a special version of `git_config` called `git_config_early`
+that takes an additional parameter to specify the repository config.
+This should be used early in a git program when the repository location
+has not yet been determined (and calling the usual lazy-evaluation
+lookup rules would yield an incorrect location).
+
+Reading Specific Files
+----------------------
+
+To read a specific file in git-config format, use
+`git_config_from_file`. This takes the same callback and data parameters
+as `git_config`.
+
+Value Parsing Helpers
+---------------------
+
+To aid in parsing string values, the config API provides callbacks with
+a number of helper functions, including:
+
+`git_config_int`::
+Parse the string to an integer, including unit factors. Dies on error;
+otherwise, returns the parsed result.
+
+`git_config_ulong`::
+Identical to `git_config_int`, but for unsigned longs.
+
+`git_config_bool`::
+Parse a string into a boolean value, respecting keywords like "true" and
+"false". Integer values are converted into true/false values (when they
+are non-zero or zero, respectively). Other values cause a die(). If
+parsing is successful, the return value is the result.
+
+`git_config_bool_or_int`::
+Same as `git_config_bool`, except that integers are returned as-is, and
+an `is_bool` flag is unset.
+
+`git_config_maybe_bool`::
+Same as `git_config_bool`, except that it returns -1 on error rather
+than dying.
+
+`git_config_string`::
+Allocates and copies the value string into the `dest` parameter; if no
+string is given, prints an error message and returns -1.
+
+`git_config_pathname`::
+Similar to `git_config_string`, but expands `~` or `~user` into the
+user's home directory when found at the beginning of the path.
+
+Writing Config Files
+--------------------
+
+TODO
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* Re: [PATCH 0/2] config includes, take 2
From: Michael Haggerty @ 2012-02-06 9:53 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20120206062713.GA9699@sigill.intra.peff.net>
On 02/06/2012 07:27 AM, Jeff King wrote:
> 3. perform cycle and duplicate detection on included files
> [...]
>
> We first read the global config, which sets the value to "global", then
> includes foo, which overwrites it to "foo". Then we read the repo
> config, which sets the value to "repo", and then does _not_ actually
> read foo. Because git config is read linearly and later values tend to
> overwrite earlier ones, we would want to suppress the _first_ instance
> of a file, not the second (or really, the final if it is included many
> times). But that is impossible to do without generating a complete graph
> of includes and then pruning the early ones.
ISTM that the main goal was to prevent infinite recursion, not avoid a
little bit of redundant reading. If that is the goal, all you have to
record is the "include stack" context that caused the
currently-being-included file to be read and make sure that the
currently-being-included file didn't appear earlier on the stack. The
fact that the same file was included earlier (but not as part of the
same include chain) needn't be considered an error.
> [...]
> So I'm actually thinking I should drop the duplicate suppression and
> just do some sort of sanity check on include-depth to break cycles
> (i.e., just die because it's a crazy thing to do, and we are really just
> trying to tell the user their config is broken rather than go into an
> infinite loop). As a bonus, it makes the code much simpler, too.
I agree that it is more sensible to error out than to ignore a recursive
include.
It might nevertheless be useful to have an "include call stack"
available for generating output for errors that occur when parsing
config files; something like:
Error when parsing /home/me/bogusconfigfile line 75
which was included from /home/me/okconfigfile line 13
which was included from /home/me/.gitconfig line 22
or
Error: /home/me/bogusconfigfile included recursively by
/home/me/bogusfile2 line 75
which was included from /home/me/bogusconfigfile line 13
which was included from /home/me/.gitconfig line 22
OTOH such error stack dumps could be generated when unwinding the C call
stack without the need for a separate "include call stack".
However, one could even imagine a command like
$ git config --where-defined user.email
user.email defined by /home/me/myfile2 line 75
which was included from /home/me/myfile1 line 13
which was included from /home/me/.gitconfig line 22
user.email redefined by /home/me/project/.git/companyconfig line 8
which was included from /home/me/project/.git/config line 20
This usage could not be implemented using the C stack, because the C
stack cannot be unwound multiple times.
But these are all just wild ideas. I doubt that people's config files
will become so complicated that this much infrastructure is needed.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply
* [PATCH 2/2] config: add include directive
From: Jeff King @ 2012-02-06 9:54 UTC (permalink / raw)
To: git
In-Reply-To: <20120206095306.GA2404@sigill.intra.peff.net>
It can be useful to split your ~/.gitconfig across multiple
files. For example, you might have a "main" file which is
used on many machines, but a small set of per-machine
tweaks. Or you may want to make some of your config public
(e.g., clever aliases) while keeping other data back (e.g.,
your name or other identifying information). Or you may want
to include a number of config options in some subset of your
repos without copying and pasting (e.g., you want to
reference them from the .git/config of participating repos).
This patch introduces an include directive for config files.
It looks like:
[include]
path = /path/to/file
This is syntactically backwards-compatible with existing git
config parsers (i.e., they will see it as another config
entry and ignore it unless you are looking up include.path).
The implementation provides a "git_config_include" callback
which wraps regular config callbacks. Callers can pass it to
git_config_from_file, and it will transparently follow any
include directives, passing all of the discovered options to
the real callback.
Include directives are turned on automatically for "regular"
git config parsing. This includes calls to git_config, as
well as calls to the "git config" program that do not
specify a single file (e.g., using "-f", "--global", etc).
They are not turned on in other cases, including:
1. Parsing of other config-like files, like .gitmodules.
There isn't a real need, and I'd rather be conservative
and avoid unnecessary incompatibility or confusion.
2. Reading single files via "git config". This is for two
reasons:
a. backwards compatibility with scripts looking at
config-like files.
b. inspection of a specific file probably means you
care about just what's in that file, not a general
lookup for "do we have this value anywhere at
all". If that is not the case, the caller can
always specify "--includes".
3. Writing files via "git config"; we want to treat
include.* variables as literal items to be copied (or
modified), and not expand them. So "git config
--unset-all foo.bar" would operate _only_ on
.git/config, not any of its included files (just as it
also does not operate on ~/.gitconfig).
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 15 ++++
Documentation/git-config.txt | 5 +
Documentation/technical/api-config.txt | 22 ++++++
builtin/config.c | 31 ++++++--
cache.h | 8 ++
config.c | 69 +++++++++++++++++-
t/t1305-config-include.sh | 126 ++++++++++++++++++++++++++++++++
7 files changed, 268 insertions(+), 8 deletions(-)
create mode 100755 t/t1305-config-include.sh
diff --git a/Documentation/config.txt b/Documentation/config.txt
index abeb82b..e55dae1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -84,6 +84,17 @@ customary UNIX fashion.
Some variables may require a special value format.
+Includes
+~~~~~~~~
+
+You can include one config file from another by setting the special
+`include.path` variable to the name of the file to be included. The
+included file is expanded immediately, as if its contents had been
+found at the location of the include directive. If the value of the
+`include.path` variable is a relative path, the path is considered to be
+relative to the configuration file in which the include directive was
+found. See below for examples.
+
Example
~~~~~~~
@@ -106,6 +117,10 @@ Example
gitProxy="ssh" for "kernel.org"
gitProxy=default-proxy ; for the rest
+ [include]
+ path = /path/to/foo.inc ; include by absolute path
+ path = foo ; expand "foo" relative to the current file
+
Variables
~~~~~~~~~
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index e7ecf5d..aa8303b 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -178,6 +178,11 @@ See also <<FILES>>.
Opens an editor to modify the specified config file; either
'--system', '--global', or repository (default).
+--includes::
+--no-includes::
+ Respect `include.*` directives in config files when looking up
+ values. Defaults to on.
+
[[FILES]]
FILES
-----
diff --git a/Documentation/technical/api-config.txt b/Documentation/technical/api-config.txt
index f428c5c..c60b6b3 100644
--- a/Documentation/technical/api-config.txt
+++ b/Documentation/technical/api-config.txt
@@ -95,6 +95,28 @@ string is given, prints an error message and returns -1.
Similar to `git_config_string`, but expands `~` or `~user` into the
user's home directory when found at the beginning of the path.
+Include Directives
+------------------
+
+By default, the config parser does not respect include directives.
+However, a caller can use the special `git_config_include` wrapper
+callback to support them. To do so, you simply wrap your "real" callback
+function and data pointer in a `struct config_include_data`, and pass
+the wrapper to the regular config-reading functions. For example:
+
+-------------------------------------------
+int read_file_with_include(const char *file, config_fn_t fn, void *data)
+{
+ struct config_include_data inc = CONFIG_INCLUDE_INIT;
+ inc.fn = fn;
+ inc.data = data;
+ return git_config_from_file(git_config_include, file, &inc);
+}
+-------------------------------------------
+
+`git_config` respects includes automatically. The lower-level
+`git_config_from_file` does not.
+
Writing Config Files
--------------------
diff --git a/builtin/config.c b/builtin/config.c
index d35c06a..09bf778 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -25,6 +25,7 @@ static const char *given_config_file;
static int actions, types;
static const char *get_color_slot, *get_colorbool_slot;
static int end_null;
+static int respect_includes = -1;
#define ACTION_GET (1<<0)
#define ACTION_GET_ALL (1<<1)
@@ -74,6 +75,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, "path", &types, "value is a path (file or directory name)", TYPE_PATH),
OPT_GROUP("Other"),
OPT_BOOLEAN('z', "null", &end_null, "terminate values with NUL byte"),
+ OPT_BOOL(0, "includes", &respect_includes, "respect include directives on lookup"),
OPT_END(),
};
@@ -161,6 +163,9 @@ static int get_value(const char *key_, const char *regex_)
int ret = -1;
char *global = NULL, *repo_config = NULL;
const char *system_wide = NULL, *local;
+ struct config_include_data inc = CONFIG_INCLUDE_INIT;
+ config_fn_t fn;
+ void *data;
local = config_exclusive_filename;
if (!local) {
@@ -213,19 +218,28 @@ static int get_value(const char *key_, const char *regex_)
}
}
+ fn = show_config;
+ data = NULL;
+ if (respect_includes) {
+ inc.fn = fn;
+ inc.data = data;
+ fn = git_config_include;
+ data = &inc;
+ }
+
if (do_all && system_wide)
- git_config_from_file(show_config, system_wide, NULL);
+ git_config_from_file(fn, system_wide, data);
if (do_all && global)
- git_config_from_file(show_config, global, NULL);
+ git_config_from_file(fn, global, data);
if (do_all)
- git_config_from_file(show_config, local, NULL);
- git_config_from_parameters(show_config, NULL);
+ git_config_from_file(fn, local, data);
+ git_config_from_parameters(fn, data);
if (!do_all && !seen)
- git_config_from_file(show_config, local, NULL);
+ git_config_from_file(fn, local, data);
if (!do_all && !seen && global)
- git_config_from_file(show_config, global, NULL);
+ git_config_from_file(fn, global, data);
if (!do_all && !seen && system_wide)
- git_config_from_file(show_config, system_wide, NULL);
+ git_config_from_file(fn, system_wide, data);
free(key);
if (regexp) {
@@ -384,6 +398,9 @@ int cmd_config(int argc, const char **argv, const char *prefix)
config_exclusive_filename = given_config_file;
}
+ if (respect_includes == -1)
+ respect_includes = !config_exclusive_filename;
+
if (end_null) {
term = '\0';
delim = '\n';
diff --git a/cache.h b/cache.h
index 9bd8c2d..7cf6dc1 100644
--- a/cache.h
+++ b/cache.h
@@ -1139,6 +1139,14 @@ extern const char *get_commit_output_encoding(void);
extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);
+struct config_include_data {
+ int depth;
+ config_fn_t fn;
+ void *data;
+};
+#define CONFIG_INCLUDE_INIT { 0 }
+extern int git_config_include(const char *name, const char *value, void *data);
+
extern const char *config_exclusive_filename;
#define MAX_GITNAME (1000)
diff --git a/config.c b/config.c
index 40f9c6d..e3fcf75 100644
--- a/config.c
+++ b/config.c
@@ -28,6 +28,69 @@ static int zlib_compression_seen;
const char *config_exclusive_filename = NULL;
+#define MAX_INCLUDE_DEPTH 10
+static const char include_depth_advice[] =
+"exceeded maximum include depth (%d) while including\n"
+" %s\n"
+"from\n"
+" %s\n"
+"Do you have circular includes?";
+static int handle_path_include(const char *path, struct config_include_data *inc)
+{
+ int ret = 0;
+ struct strbuf buf = STRBUF_INIT;
+
+ /*
+ * Use an absolute path as-is, but interpret relative paths
+ * based on the including config file.
+ */
+ if (!is_absolute_path(path)) {
+ char *slash;
+
+ if (!cf || !cf->name)
+ return error("relative config includes must come from files");
+
+ slash = find_last_dir_sep(cf->name);
+ if (slash)
+ strbuf_add(&buf, cf->name, slash - cf->name + 1);
+ strbuf_addstr(&buf, path);
+ path = buf.buf;
+ }
+
+ if (!access(path, R_OK)) {
+ if (++inc->depth > MAX_INCLUDE_DEPTH)
+ die(include_depth_advice, MAX_INCLUDE_DEPTH, path,
+ cf && cf->name ? cf->name : "the command line");
+ ret = git_config_from_file(git_config_include, path, inc);
+ inc->depth--;
+ }
+ strbuf_release(&buf);
+ return ret;
+}
+
+int git_config_include(const char *var, const char *value, void *data)
+{
+ struct config_include_data *inc = data;
+ const char *type;
+ int ret;
+
+ /*
+ * Pass along all values, including "include" directives; this makes it
+ * possible to query information on the includes themselves.
+ */
+ ret = inc->fn(var, value, inc->data);
+ if (ret < 0)
+ return ret;
+
+ type = skip_prefix(var, "include.");
+ if (!type)
+ return ret;
+
+ if (!strcmp(type, "path"))
+ ret = handle_path_include(value, inc);
+ return ret;
+}
+
static void lowercase(char *p)
{
for (; *p; p++)
@@ -921,9 +984,13 @@ int git_config(config_fn_t fn, void *data)
{
char *repo_config = NULL;
int ret;
+ struct config_include_data inc = CONFIG_INCLUDE_INIT;
+
+ inc.fn = fn;
+ inc.data = data;
repo_config = git_pathdup("config");
- ret = git_config_early(fn, data, repo_config);
+ ret = git_config_early(git_config_include, &inc, repo_config);
if (repo_config)
free(repo_config);
return ret;
diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
new file mode 100755
index 0000000..0a27ec4
--- /dev/null
+++ b/t/t1305-config-include.sh
@@ -0,0 +1,126 @@
+#!/bin/sh
+
+test_description='test config file include directives'
+. ./test-lib.sh
+
+test_expect_success 'include file by absolute path' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = \"$PWD/one\"" >.gitconfig &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'include file by relative path' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo 1 >expect &&
+ git config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'chained relative paths' '
+ mkdir subdir &&
+ echo "[test]three = 3" >subdir/three &&
+ echo "[include]path = three" >subdir/two &&
+ echo "[include]path = subdir/two" >.gitconfig &&
+ echo 3 >expect &&
+ git config test.three >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'include options can still be examined' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ echo one >expect &&
+ git config include.path >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'listing includes option and expansion' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ cat >expect <<-\EOF &&
+ include.path=one
+ test.one=1
+ EOF
+ git config --list >actual.full &&
+ grep -v ^core actual.full >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'single file lookup does not expand includes by default' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ test_must_fail git config -f .gitconfig test.one &&
+ test_must_fail git config --global test.one &&
+ echo 1 >expect &&
+ git config --includes -f .gitconfig test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'writing config file does not expand includes' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ git config test.two 2 &&
+ echo 2 >expect &&
+ git config --no-includes test.two >actual &&
+ test_cmp expect actual &&
+ test_must_fail git config --no-includes test.one
+'
+
+test_expect_success 'config modification does not affect includes' '
+ echo "[test]one = 1" >one &&
+ echo "[include]path = one" >.gitconfig &&
+ git config test.one 2 &&
+ echo 1 >expect &&
+ git config -f one test.one >actual &&
+ test_cmp expect actual &&
+ cat >expect <<-\EOF &&
+ 1
+ 2
+ EOF
+ git config --get-all test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'missing include files are ignored' '
+ cat >.gitconfig <<-\EOF &&
+ [include]path = foo
+ [test]value = yes
+ EOF
+ echo yes >expect &&
+ git config test.value >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'absolute includes from command line work' '
+ echo "[test]one = 1" >one &&
+ echo 1 >expect &&
+ git -c include.path="$PWD/one" config test.one >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'relative includes from command line fail' '
+ echo "[test]one = 1" >one &&
+ test_must_fail git -c include.path=one config test.one
+'
+
+test_expect_success 'include cycles are detected' '
+ cat >.gitconfig <<-\EOF &&
+ [test]value = gitconfig
+ [include]path = cycle
+ EOF
+ cat >cycle <<-\EOF &&
+ [test]value = cycle
+ [include]path = .gitconfig
+ EOF
+ cat >expect <<-\EOF &&
+ gitconfig
+ cycle
+ EOF
+ test_must_fail git config --get-all test.value 2>stderr &&
+ grep "exceeded maximum include depth" stderr
+'
+
+test_done
--
1.7.9.rc1.29.g43677
^ permalink raw reply related
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-06 10:06 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <4F2FA330.7020803@alum.mit.edu>
On Mon, Feb 06, 2012 at 10:53:52AM +0100, Michael Haggerty wrote:
> ISTM that the main goal was to prevent infinite recursion, not avoid a
> little bit of redundant reading.
It was both, actually. There was a sense that we should not end up with
duplicate entries by reading the same file twice. However, entries which
actually create lists (and could therefore create duplicates) are by far
the minority compared to entries which overwrite. And it is the
overwrite-style entries which are harmed by suppressing duplicates.
> If that is the goal, all you have to record is the "include stack"
> context that caused the currently-being-included file to be read and
> make sure that the currently-being-included file didn't appear earlier
> on the stack. The fact that the same file was included earlier (but
> not as part of the same include chain) needn't be considered an error.
I considered this, but decided the complexity wasn't worth it,
especially because getting it accurate means cooperation from
git_config_from_file, which otherwise doesn't know or care about this
mechanism. Instead I keep a simple depth counter and barf at a
reasonable maximum, printing the "from" and "to" files. Yes, it's not
nearly as elegant as keeping the whole stack, but I really don't expect
people to have complex super-deep includes, nor for it to be difficult
to hunt down the cause of a cycle.
Maybe that is short-sighted or lazy of me, but I'd just really be
surprised if people ever went more than 1 or 2 layers deep, or if they
actually ended up with a cycle at all.
There is a stack kept already by git_config_from_file, but it doesn't
respect the call-chain (so if I start a new git_config() call from
inside a callback, it is still part of the same stack).
We could possibly put a marker in the stack for that situation, and then
it would be usable for include cycle-detection.
> However, one could even imagine a command like
>
> $ git config --where-defined user.email
> user.email defined by /home/me/myfile2 line 75
> which was included from /home/me/myfile1 line 13
> which was included from /home/me/.gitconfig line 22
> user.email redefined by /home/me/project/.git/companyconfig line 8
> which was included from /home/me/project/.git/config line 20
You can already implement this with the existing stack by providing a
suitable callback (since its your callback, you'd know that there was no
recursion of git_config, and therefore the stack refers only to a single
set of includes).
-Peff
^ permalink raw reply
* Re: [PATCH 0/2] config includes, take 2
From: Jeff King @ 2012-02-06 10:16 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <20120206100622.GC4300@sigill.intra.peff.net>
On Mon, Feb 06, 2012 at 05:06:22AM -0500, Jeff King wrote:
> > If that is the goal, all you have to record is the "include stack"
> > context that caused the currently-being-included file to be read and
> > make sure that the currently-being-included file didn't appear earlier
> > on the stack. The fact that the same file was included earlier (but
> > not as part of the same include chain) needn't be considered an error.
>
> I considered this, but decided the complexity wasn't worth it,
> especially because getting it accurate means cooperation from
> git_config_from_file, which otherwise doesn't know or care about this
> mechanism. Instead I keep a simple depth counter and barf at a
> reasonable maximum, printing the "from" and "to" files. Yes, it's not
> nearly as elegant as keeping the whole stack, but I really don't expect
> people to have complex super-deep includes, nor for it to be difficult
> to hunt down the cause of a cycle.
Oh, one other thing: to detect cycles, you have to use a canonical
version of the filename for comparisons. That makes the existing stack
that git_config_from_file keeps useless. Using real_path is hard,
because it will die() if some path components don't exist. Using
absolute_path is a reasonable compromise, but it can actually miss some
cycles if they involve symbolic links.
Not insurmountable if you can accept teaching git_config_from_file to
real_path() each file it reads. But again, it ends up getting complex
for IMHO not much gain. A stupid depth counter can't show you a stack
trace (and it may have false positives), but it's dirt simple and
probably good enough.
-Peff
^ permalink raw reply
* [PATCH 0/4] Deprecate "not allow as-is commit with i-t-a entries"
From: Nguyễn Thái Ngọc Duy @ 2012-02-06 10:57 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jonathan Nieder,
Nguyễn Thái Ngọc Duy
By the end of this series "git commit -m foo" will proceed regardless
intent-to-add (aka "git add -N") entries. commit.ignoreIntentToAdd is
used as transitiono config key.
The plan is in 2/4. I set 1.8.0 as the first deprecation date, but of
course it's just a suggestion. The second deprecation date might be
1.8.5, long enough for users to adapt to the new behavior.
..or we switch back half way because we find current behavior does
make more sense.
Nguyễn Thái Ngọc Duy (4):
cache-tree: update API to take abitrary flags
commit: introduce a config key to allow as-is commit with i-t-a
entries
commit: turn commit.ignoreIntentToAdd to true by default
commit: remove commit.ignoreIntentToAdd, assume it's always true
Documentation/git-add.txt | 12 ++++++++++--
builtin/commit.c | 9 ++++++---
cache-tree.c | 35 +++++++++++++++++------------------
cache-tree.h | 5 ++++-
merge-recursive.c | 2 +-
t/t2203-add-intent.sh | 21 ++++++++++++++++++++-
test-dump-cache-tree.c | 2 +-
7 files changed, 59 insertions(+), 27 deletions(-)
--
1.7.8.36.g69ee2
^ permalink raw reply
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