* 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: 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: 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: [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: 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 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: [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: 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 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: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
* [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
* [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 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
* 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
* 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 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: [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: [PATCH 1/6] read-cache: use sha1file for sha1 calculation
From: Junio C Hamano @ 2012-02-06 7:34 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, Thomas Rast, Joshua Redstone
In-Reply-To: <1328507319-24687-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Having no explanation on any of the patch in the series without cover
letter makes it hard to comment on anything, and not having any numbers
makes it even harder after guessing that this is about some performance
tweaks for 2M entry index cases.
This is open source, and I wouldn't stop you from spending time on anything
that interests you.
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
(2) the negative pathspec thing that resurfaced in disguise as Albert
Yale's "grep --exclude" series.
than playing with the approach of this series. The two reasons I suspect
that spending your time on this series will give us much less value than
the above two topics out of you are:
(1) While I think 2M-entry index is an interesting issue, it does not
affect most of the people; and more importantly
(2) I think the proper way to handle 2M-entry index case is to avoid
having to write and read the whole 2M-entry as a flat table in the
first place, not by weakening how its integrity is assured in order
to micro-tweak the read/write efficiency without re-examining the
flatness of the current in-core index [*1*].
The first patch that reuses the existing csum-file API to older code that
was written before csum-file was invented is probably a good thing to do,
though, independent of the 2M-entry issue.
Thanks.
[Footnote]
*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.
^ permalink raw reply
* Re: Git performance results on a large repository
From: David Mohs @ 2012-02-06 7:10 UTC (permalink / raw)
To: git
In-Reply-To: <243C23AF01622E49BEA3F28617DBF0AD5912CA85@SC-MBX02-5.TheFacebook.com>
Joshua Redstone <joshua.redstone <at> fb.com> writes:
> To get a bit abstract for a moment, in an ideal world, it doesn't seem like
> performance constraints of a source-control-system should dictate how we
> choose to structure our code. Ideally, seems like we should be able to choose
> to structure our code in whatever way we feel maximizes developer
> productivity. If development and code/release management seem easier in a
> single repo, than why not make an SCM that can handle it? This is one reason
> I've been leaning towards figuring out an SCM approach that can work well with
> our current practices rather than changing them as a prerequisite for good SCM
> performance.
I certainly agree with this perspective---that our tools should support our
use cases and not the other way around. However, I'd like you to consider that
the size of this hypothetical repository might be giving you some useful
information on the health of the code it contains. You might consider creating
separate repositories simply to promote good modularization. It would involve
some up-front effort and certainly some pain, but this work itself might be
beneficial to your codebase without even considering the improved performance
of the version control system.
My concern here is that it may be extremely difficult to make a single piece
of software scale for a project that can grow arbitrarily large. You may add
some great performance improvements to git to then find that your bottleneck
is the filesystem. That would enlarge the scope of your work and would likely
make the project more difficult to manage.
If you are able to prove me wrong, the entire software community will benefit
from this work. However, before you embark upon a technical solution to your
problem, I would urge you to consider the possible benefits of a non-technical
solution, specifically restructuring your code and/or teams into more
independent modules. You might find benefits from this approach that extend
beyond source code control, which could make it the solution with the least
amount of overall risk.
Thanks for starting this valuable discussion.
-David
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Jeff King @ 2012-02-06 7:13 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, jasampler
In-Reply-To: <20120206070424.GC9931@sigill.intra.peff.net>
On Mon, Feb 06, 2012 at 02:04:24AM -0500, Jeff King wrote:
> > >Before your patch, a tag whose sha1 could not be read would get its name
> > >printed, and then we would later return without printing anything more.
> > >Now it won't get even the first bit printed.
> > >
> > >However, I'm not sure the old behavior wasn't buggy; it would print part
> > >of the line, but never actually print the newline.
> >
> > If you prefer, I can restore the old behavior just moving the
> > condition/return back below the refname print; then add "buf" qualifier
> > to the following fragment and at each intermediate free.
>
> Thinking on it more, your behavior is at least as good as the old. And
> it only comes up in a broken repo, anyway, so trying to come up with
> some kind of useful outcome is pointless.
Sorry to reverse myself, but I just peeked at the show_reference
function one more time. Unconditionally moving the buffer-reading up
above the "if (!filter->lines)" conditional is not a good idea.
If I do "git tag -l", right now git doesn't have to actually read and
parse each object that has been tagged (lightweight or not). If I use
"git tag -n10", then obviously we do need to read it (and we do). And if
we use your new "--points-at", we also do. But if neither of those
options are in use, it would be nice to avoid the object lookup (it may
not seem like much, but if you have a repo with an insane number of
tags, it can add up).
> 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.
-Peff
^ permalink raw reply
* Re: [PATCH 3/6] Stop producing index version 2
From: Junio C Hamano @ 2012-02-06 7:10 UTC (permalink / raw)
To: spearce
Cc: Nguyễn Thái Ngọc Duy, git, Thomas Rast,
Joshua Redstone
In-Reply-To: <1328507319-24687-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> read-cache.c learned to produce version 2 or 3 depending on whether
> extended cache entries exist in 06aaaa0 (Extend index to save more flags
> - 2008-10-01), first released in 1.6.1. The purpose is to keep
> compatibility with older git. It's been more than three years since
> then and git has reached 1.7.9. Drop support for older git.
Cc'ing this, as I suspect this would surely raise eyebrows of some people
who wanted to get rid of the version 3 format.
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Jeff King @ 2012-02-06 7:04 UTC (permalink / raw)
To: Tom Grennan; +Cc: git, gitster, krh, jasampler
In-Reply-To: <20120206063213.GC10489@tgrennan-laptop>
On Sun, Feb 05, 2012 at 10:32:13PM -0800, Tom Grennan wrote:
> >> +--points-at <object>::
> >> + Only list annotated or signed tags of the given object.
> >> +
> >
> >It is unclear to me from this documentation if we will only peel a
> >single level, or if we will peel indefinitely. E.g., what will this
> >show:
> >
> > $ git tag one v1.0
> > $ git tag two one
> > $ git tag --points-at=v1.0
> >
> >It will clearly show "one", but will it also show "two" (from reading
> >the code, I think the answer is "no")? If not, should it?
>
> Actually, neither one nor two would be listed as these are lightweight
> tags. In the modified example,
>
> $ git tag -a -m One one v1.0
> $ git tag -a -m Two two one
> $ git tag --points-at v1.0
> one
> $ git tag --points-at one
> two
Hmm. Yeah, I see that now. And re-reading your description, I see that
it is explicit only to read from tag objects. Somehow the name
"points-at" seems a bit misleading to me, then, as it implies being more
inclusive of all pointing, including lightweight tags.
I know that has nothing to do with your use-case though; I just wonder
if there could be a better name. I can't think of one, though, and I'm
not sure we will ever want a more inclusive --points-at, so maybe it is
not worth caring about.
> >Before your patch, a tag whose sha1 could not be read would get its name
> >printed, and then we would later return without printing anything more.
> >Now it won't get even the first bit printed.
> >
> >However, I'm not sure the old behavior wasn't buggy; it would print part
> >of the line, but never actually print the newline.
>
> If you prefer, I can restore the old behavior just moving the
> condition/return back below the refname print; then add "buf" qualifier
> to the following fragment and at each intermediate free.
Thinking on it more, your behavior is at least as good as the old. And
it only comes up in a broken repo, anyway, so trying to come up with
some kind of useful outcome is pointless.
> >> + if (filter->points_at) {
> >> + unsigned char tagged_sha1[20];
> >> + if (memcmp("object ", buf, 7) \
> >> + || buf[47] != '\n' \
> >> + || get_sha1_hex(buf + 7, tagged_sha1) \
> >> + || memcmp(filter->points_at, tagged_sha1, 20)) {
> >> + free(buf);
> >> + return 0;
> >> + }
> >> + }
> >
> >Hmm, I would have expected to use parse_tag_buffer instead of doing it
> >by hand. This is probably a tiny bit more efficient, but I wonder if the
> >code complexity is worth it.
>
> I didn't see how to get the object sha out of parse_tag_buffer() to
> compare with "point_at". The inline conditions seem simple enough.
I think it would be:
struct tag *t = lookup_tag(sha1);
if (parse_tag_buffer(t, buf, size) < 0)
return 0; /* error, possibly should die() */
if (!hashcmp(filter->points_at, t->tagged.sha1))
/* matches */
That might bear a little bit of explanation. Git keeps a struct in
memory for each object, each of which contains a "struct object" at the
beginning. By calling lookup_tag, we either find an existing reference
to the tag with this sha1, or create a new "struct tag". And then we
parse it using the data we've read, storing it in the "struct tag" (for
our use, or for later use). The "tagged" member points to the tagged
object. Which again is a struct object; it may or may not have been
read and parsed, but we definitely know its sha1.
If this seems a little cumbersome, it is because the usual usage is more
like:
struct object *obj = parse_object(sha1);
if (!obj)
die("unable to read %s", sha1_to_hex(sha1));
if (obj->type == OBJ_TAG) {
struct tag *t = (struct tag *)obj;
if (!hashcmp(filter->points_at, t->tagged.sha1))
/* matched */
And then you don't have to bother with calling read_sha1_file at all.
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.
So your patch would read random heap memory on something like:
blob=`echo foo | git hash-object --stdin -w`
git tag foo $blob
-Peff
^ permalink raw reply
* Re: [RFD] Rewriting safety - warn before/when rewriting published history
From: Johan Herland @ 2012-02-06 6:53 UTC (permalink / raw)
To: Steven Michalske; +Cc: Jakub Narebski, git
In-Reply-To: <EAF9D593-4E0C-4C95-A048-3F6AC8ADD866@gmail.com>
On Mon, Feb 6, 2012 at 01:57, Steven Michalske <smichalske@gmail.com> wrote:
> On Feb 4, 2012, at 11:45 AM, Jakub Narebski wrote:
>> In Mercurial 2.1 there are three available phases: 'public' for
>> published commits, 'draft' for local un-published commits and
>> 'secret' for local un-published commits which are not meant to
>> be published.
>>
>> The phase of a changeset is always equal to or higher than the phase
>> of it's descendants, according to the following order:
>>
>> public < draft < secret
>
> Let's not limit ourselves to just three levels. They are a great start but I propose the following.
>
> published - The commits that are on a public repository that if are rewritten will invoke uprisings.
> general rule here would be to revert or patch, no rewrites.
> based - The commits that the core developers have work based upon. (not just the commits in their repo.)
> general rule is notify your fellow developers before a rewrite.
> shared - The commits that are known to your fellow core developers.
> These commits are known, but have not had work based off of them. Minimal risk to rewrite.
> local - The commits that are local only, no one else has a copy.
> Commits your willing to share, but have not been yet shared, either from actions of you, or a fetch from others.
> restricted or private - The commits that you do not want shared.
> Manually added, think of a branch tip marked as restricted automatically promotes commits to the branch as restricted.
>
> Maybe make these like nice levels, but as two components, publicity 0-100 and rewritability 0-100
> Published is publicity 100 and rewritability 0
> Restricted is publicity 0 and rewritability 100
> Based publicity 75 and rewritability 25
> Shared publicity 50 and rewritability 50
> Local publicity 25 and rewritability 75
> Restricted publicity 0 and rewritability 100
>
> [...]
With all due respect, I believe this is crazy. You're adding an entire
layer of complexity on top of commits that every user has to know
about, and has little or no value to most of them. IMHO, most users
only want Git to help them avoid doing something stupid (rewriting
'public' commits or publishing 'secret' commits), and to do so with
the minimal amount of manual user interaction. The above idea is more
suitable for armchair dictators that want to micromanage their commits
along two arbitrary axes of evil^H^H^H^Hpointlessness. On both axes,
you'll need threshold values where Git starts refusing to
publish/rewrite your commit. Hence, the only thing that matters is
whether the 'publicity'/'rewritability' value is above/below that
threshold, at which point you could save yourself a lot of trouble by
making them simple boolean flags instead.
Having said that, you can use 'git notes' along with existing and
proposed hooks (as described elsewhere in this thread) to implement
whatever crazy commit publishing/rewriting scheme you desire. To
misquote someone famous: I disapprove of what you want to do with Git,
but I will defend to the death your right to make Git do what you want
(in the privacy of your own repos). ;)
Have fun! :)
...Johan
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Tom Grennan @ 2012-02-06 6:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, jasampler
In-Reply-To: <7v8vkga370.fsf@alter.siamese.dyndns.org>
On Sun, Feb 05, 2012 at 10:25:23PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>>>I wonder if defaulting to HEAD even makes sense for --points-at. When you
>>>are chasing a bug and checked out an old version that originally had
>>>problem, "git tag --contains" that defaults to HEAD does have a value. It
>>>tells us what releases are potentially contaminated with the buggy commit.
>>>
>>>But does a similar use case support points-at that defaults to HEAD?
>>
>> Yes, the usage, "--points-at <object>..." implies that there is no
>> default. So, I suppose that NULL more appropriate than "HEAD".
>
>That's a circular logic.
>
>The usage could very well say "--points-at <object>" and forbid missing
><object>. I think that would make a lot _more_ sense, because I did not
>think of offhand any good reason that --points-at should default to HEAD
>to support some common usage, and you also seem to be unable to.
Sorry for the miss-communication. I agreed with you - at least I thought I did.
So, "--points-at <object>" should forbid a missing <object>.
I think I can do so by using defval = (intptr_t)NULL instead of "HEAD",
right?
--
TomG
^ permalink raw reply
* Re: [RFC/PATCH] tag: add --points-at list option
From: Tom Grennan @ 2012-02-06 6:32 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, krh, jasampler
In-Reply-To: <20120206000420.GC28735@sigill.intra.peff.net>
On Sun, Feb 05, 2012 at 07:04:21PM -0500, Jeff King wrote:
>On Sun, Feb 05, 2012 at 02:28:07PM -0800, Tom Grennan wrote:
>
>> This filters the list for annotated|signed tags of the given object.
>> Example,
>>
>> john$ git tag -s v1.0-john v1.0
>> john$ git tag -l --points-at v1.0
>> v1.0-john
>
>I really like this approach. One big question, and a few small comments:
>
>> +--points-at <object>::
>> + Only list annotated or signed tags of the given object.
>> +
>
>It is unclear to me from this documentation if we will only peel a
>single level, or if we will peel indefinitely. E.g., what will this
>show:
>
> $ git tag one v1.0
> $ git tag two one
> $ git tag --points-at=v1.0
>
>It will clearly show "one", but will it also show "two" (from reading
>the code, I think the answer is "no")? If not, should it?
Actually, neither one nor two would be listed as these are lightweight
tags. In the modified example,
$ git tag -a -m One one v1.0
$ git tag -a -m Two two one
$ git tag --points-at v1.0
one
$ git tag --points-at one
two
one's object is v1.0 whereas two's object is one
>> + buf = read_sha1_file(sha1, &type, &size);
>> + if (!buf || !size)
>> + return 0;
>
>Before your patch, a tag whose sha1 could not be read would get its name
>printed, and then we would later return without printing anything more.
>Now it won't get even the first bit printed.
>
>However, I'm not sure the old behavior wasn't buggy; it would print part
>of the line, but never actually print the newline.
If you prefer, I can restore the old behavior just moving the
condition/return back below the refname print; then add "buf" qualifier
to the following fragment and at each intermediate free.
>> + if (filter->points_at) {
>> + unsigned char tagged_sha1[20];
>> + if (memcmp("object ", buf, 7) \
>> + || buf[47] != '\n' \
>> + || get_sha1_hex(buf + 7, tagged_sha1) \
>> + || memcmp(filter->points_at, tagged_sha1, 20)) {
>> + free(buf);
>> + return 0;
>> + }
>> + }
>
>Hmm, I would have expected to use parse_tag_buffer instead of doing it
>by hand. This is probably a tiny bit more efficient, but I wonder if the
>code complexity is worth it.
I didn't see how to get the object sha out of parse_tag_buffer() to
compare with "point_at". The inline conditions seem simple enough.
>
>> static int list_tags(const char **patterns, int lines,
>> - struct commit_list *with_commit)
>> + struct commit_list *with_commit,
>> + unsigned char *points_at)
>
>Like Junio, I was surprised this did not allow a list.
I agree and will change it.
Thanks,
TomG
^ 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