* Re: [regression?] trailing slash required in .gitattributes
From: Junio C Hamano @ 2013-03-19 18:10 UTC (permalink / raw)
To: Jeff King; +Cc: git, Jean-Noël AVILA
In-Reply-To: <20130319175756.GA13760@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Prior to v1.8.1.1, if I did this:
>
> git init
> echo content >foo &&
> mkdir subdir &&
> echo content >subdir/bar &&
> echo "subdir export-ignore" >.gitattributes
> git add . &&
> git commit -m one &&
> git archive HEAD | tar tf -
>
> my archive would contain only "foo" and ".gitattributes", not subdir. As
> of v1.8.1.1, the attribute on subdir is ignored unless it is written
> with a trailing slash, like:
>
> subdir/ export-ignore
>
> The issue bisects to 94bc671 (Add directory pattern matching to
> attributes, 2012-12-08). That commit actually tests not only that
> "subdir/" matches, but also that just "subdir" does not match.
>
> The commit message there is vague about the reasoning, but my
> understanding is that it was meant to harmonize gitignore and
> gitattributes, the former of which can take "dir/". I don't have a
> problem with offering "dir/" to match only directories, but what is the
> point in disallowing just "dir" to match a directory?
>
> It seems like a pointless regression to me, but I'm not clear whether it
> was intentional or not (and if it was intentional, I think we would need
> to handle it with a proper transition period, not in a maint release).
I agree that is a pointless regression.
^ permalink raw reply
* Re: [PATCH] push: Alias pushurl from push rewrites
From: Junio C Hamano @ 2013-03-19 18:08 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Rob Hoelz, git, josh
In-Reply-To: <20130319015541.GH5062@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>>> Test nits:
>>> ...
>>> Hope that helps,
>>>
>>> Jonathan Nieder (3):
>>> push test: use test_config where appropriate
>>> push test: simplify check of push result
>>> push test: rely on &&-chaining instead of 'if bad; then echo Oops; fi'
>>
>> Are these supposed to be follow-up patches? Preparatory steps that
>> Rob can reroll on top? Something else?
>
> Preparatory steps.
OK, thanks. I'll queue these first then.
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Junio C Hamano @ 2013-03-19 17:58 UTC (permalink / raw)
To: Thomas Rast
Cc: Stefan Zager, git, Jeff King,
Nguyễn Thái Ngọc Duy
In-Reply-To: <87obef2wut.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Actually scratch that again. It *is* a stack overflow, except that this
> is a thread stack, for which the OS X defaults are 512kB apparently, as
> opposed to 2MB on linux.
> ...
> And indeed the following patch fixes it. Sounds like the delta
> unpacking needs a rewrite to support stackless operation. Sigh.
Yikes. Thanks for digging it to the bottom.
I am not sure if I want to carry this patch in its current form,
though. As this episode demonstrates, no default is good enough for
everybody, and I am not sure if a configuration variable is a good
way to go, either.
>
> diff --git i/builtin/index-pack.c w/builtin/index-pack.c
> index 6be99e2..f73291f 100644
> --- i/builtin/index-pack.c
> +++ w/builtin/index-pack.c
> @@ -1075,13 +1075,17 @@ static void resolve_deltas(void)
> nr_dispatched = 0;
> if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
> init_thread();
> + pthread_attr_t attr;
> + pthread_attr_init(&attr);
> + pthread_attr_setstacksize(&attr, 2*1024*1024);
> for (i = 0; i < nr_threads; i++) {
> - int ret = pthread_create(&thread_data[i].thread, NULL,
> + int ret = pthread_create(&thread_data[i].thread, &attr,
> threaded_second_pass, thread_data + i);
> if (ret)
> die(_("unable to create thread: %s"),
> strerror(ret));
> }
> + pthread_attr_destroy(&attr);
> for (i = 0; i < nr_threads; i++)
> pthread_join(thread_data[i].thread, NULL);
> cleanup_thread();
^ permalink raw reply
* [regression?] trailing slash required in .gitattributes
From: Jeff King @ 2013-03-19 17:57 UTC (permalink / raw)
To: git; +Cc: Jean-Noël AVILA
Prior to v1.8.1.1, if I did this:
git init
echo content >foo &&
mkdir subdir &&
echo content >subdir/bar &&
echo "subdir export-ignore" >.gitattributes
git add . &&
git commit -m one &&
git archive HEAD | tar tf -
my archive would contain only "foo" and ".gitattributes", not subdir. As
of v1.8.1.1, the attribute on subdir is ignored unless it is written
with a trailing slash, like:
subdir/ export-ignore
The issue bisects to 94bc671 (Add directory pattern matching to
attributes, 2012-12-08). That commit actually tests not only that
"subdir/" matches, but also that just "subdir" does not match.
The commit message there is vague about the reasoning, but my
understanding is that it was meant to harmonize gitignore and
gitattributes, the former of which can take "dir/". I don't have a
problem with offering "dir/" to match only directories, but what is the
point in disallowing just "dir" to match a directory?
It seems like a pointless regression to me, but I'm not clear whether it
was intentional or not (and if it was intentional, I think we would need
to handle it with a proper transition period, not in a maint release).
-Peff
^ permalink raw reply
* Re* git merge <tag> behavior
From: Junio C Hamano @ 2013-03-19 17:54 UTC (permalink / raw)
To: Yann Droneaud; +Cc: Git
In-Reply-To: <7vfvzrjrad.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> 2) git merge <tag> VS git merge <object-id>
>> ...
>> It would be great to have Git using the tag message and check the
>> signature.
>
> Perhaps, but if you feed the $(git rev-parse v1.12.2) to merge, your
> subject will not be able to say "Merge tag 'v1.12.2'" in the first
> place, so I do not think you would want to encourage such usage in
> the first place.
A patch to do so may look like this. You would probably want to
also do read_sha1_file(desc->obj->sha1) here and parse out the
header line "tag " to replace the "remote" to recover the symbolic
tag name, but I didn't bother in this illustration.
diff --git a/builtin/merge.c b/builtin/merge.c
index 0ec8f0d..990e90c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -516,6 +516,19 @@ static void merge_name(const char *remote, struct strbuf *msg)
strbuf_release(&line);
goto cleanup;
}
+
+ if (remote_head->util) {
+ struct merge_remote_desc *desc;
+ desc = merge_remote_util(remote_head);
+ if (desc && desc->obj && desc->obj->type == OBJ_TAG) {
+ strbuf_addf(msg, "%s\t\t%s '%s'\n",
+ sha1_to_hex(desc->obj->sha1),
+ typename(desc->obj->type),
+ remote);
+ goto cleanup;
+ }
+ }
+
strbuf_addf(msg, "%s\t\tcommit '%s'\n",
sha1_to_hex(remote_head->object.sha1), remote);
cleanup:
^ permalink raw reply related
* Re: [PATCH v2] index-pack: always zero-initialize object_entry list
From: Junio C Hamano @ 2013-03-19 17:13 UTC (permalink / raw)
To: Thomas Rast
Cc: Jeff King, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <87ehfb2w4q.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> It apparently relates to the use of .idx.offset to compute the "next"
> offset, cf. append_obj_to_pack():
>
> struct object_entry *obj = &objects[nr_objects++];
> ...
> obj[1].idx.offset = obj[0].idx.offset + n;
> obj[1].idx.offset += write_compressed(f, buf, size);
>
> So you trashed the offset of the first object after all the objects that
> are actually *in* the patch.
>
> And with that: ACK.
Ahh, I also was scratching my head about that +1 thing. After all,
the +1 in the argument to xrealloc() was already a clue.
Thanks both for digging to the bottom of this one.
^ permalink raw reply
* Re: [PATCH v2] index-pack: always zero-initialize object_entry list
From: Thomas Rast @ 2013-03-19 16:27 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319161722.GA17445@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Mar 19, 2013 at 11:52:44AM -0400, Jeff King wrote:
>
>> > > > Commit 38a4556 (index-pack: start learning to emulate
>> > > > "verify-pack -v", 2011-06-03) added a "delta_depth" counter
>> > > > to each "struct object_entry". Initially, all object entries
>> > > > have their depth set to 0; in resolve_delta, we then set the
>> > > > depth of each delta to "base + 1". Base entries never have
>> > > > their depth touched, and remain at 0.
>> > >
>> > > This patch causes index-pack to fail on the pack that triggered the
>> > > whole discussion. More in a minute in another side thread, but
>> > > meanwhile: NAK until we understand what is really going on here.
>> >
>> > Odd; that's what I was testing with, and it worked fine.
>>
>> Ah, interesting. I built the fix on top of d1a0ed1, the first commit
>> that shows the problem. And it works fine there. But when it is
>> forward-ported to the current master, it breaks as you saw.
>>
>> More bisection fun.
>
> So after bisecting, I realize that it is indeed broken on top of
> d1a0ed1. I have no idea why I didn't notice that before; I'm guessing it
> was because I was running it under valgrind and paying attention only to
> valgrind errors.
>
> Anyway, the problem is simple and stupid. The original object array is
> not nr_objects item long; it is (nr_objects + 1) long, though I'm not
> clear why (1-indexing?).
It apparently relates to the use of .idx.offset to compute the "next"
offset, cf. append_obj_to_pack():
struct object_entry *obj = &objects[nr_objects++];
...
obj[1].idx.offset = obj[0].idx.offset + n;
obj[1].idx.offset += write_compressed(f, buf, size);
So you trashed the offset of the first object after all the objects that
are actually *in* the patch.
And with that: ACK.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: git merge <tag> behavior
From: Junio C Hamano @ 2013-03-19 16:19 UTC (permalink / raw)
To: Yann Droneaud; +Cc: Git
In-Reply-To: <1363704914.6289.39.camel@test.quest-ce.net>
Yann Droneaud <ydroneaud@opteya.com> writes:
> 1) there's no mention of the git merge <tag> behavior in git-merge.1
>
> When asking Git to merge a tag (such as a signed tag or annotated tag),
> it will always create a merge commit even if fast-forward was possible.
> It's like having --no-ff present on the command line.
>
> It's a difference from the default behavior described in git-merge.1[4].
> It should be documented as an exception of "FAST-FORWARD MERGE" section
> and "--ff" option description.
Yes; we welcome documentation patches.
> 2) git merge <tag> VS git merge <object-id>
>
> If <tag> is an object (not a lightweight/reference tag), git merge <tag>
> ...
> But, if you use the tag object-id instead of its name, for example using
> git merge `git show-ref <tag>`,
"git show-ref <tag>" gives you something like
572a535454612a046e7dd7404dcca94d6243c788 refs/tags/v1.8.2
which is an invalid thing to merge with. Perhaps you meant
git merge $(git rev-parse v1.12.2)
> signature is not checked. Git still create a merge commit, but doesn't
> prepare a commit message with the tag message and the signature:
>
> It would be great to have Git using the tag message and check the
> signature.
Perhaps, but if you feed the $(git rev-parse v1.12.2) to merge, your
subject will not be able to say "Merge tag 'v1.12.2'" in the first
place, so I do not think you would want to encourage such usage in
the first place.
^ permalink raw reply
* [PATCH v2] index-pack: always zero-initialize object_entry list
From: Jeff King @ 2013-03-19 16:17 UTC (permalink / raw)
To: Thomas Rast
Cc: Junio C Hamano, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319155244.GA16532@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 11:52:44AM -0400, Jeff King wrote:
> > > > Commit 38a4556 (index-pack: start learning to emulate
> > > > "verify-pack -v", 2011-06-03) added a "delta_depth" counter
> > > > to each "struct object_entry". Initially, all object entries
> > > > have their depth set to 0; in resolve_delta, we then set the
> > > > depth of each delta to "base + 1". Base entries never have
> > > > their depth touched, and remain at 0.
> > >
> > > This patch causes index-pack to fail on the pack that triggered the
> > > whole discussion. More in a minute in another side thread, but
> > > meanwhile: NAK until we understand what is really going on here.
> >
> > Odd; that's what I was testing with, and it worked fine.
>
> Ah, interesting. I built the fix on top of d1a0ed1, the first commit
> that shows the problem. And it works fine there. But when it is
> forward-ported to the current master, it breaks as you saw.
>
> More bisection fun.
So after bisecting, I realize that it is indeed broken on top of
d1a0ed1. I have no idea why I didn't notice that before; I'm guessing it
was because I was running it under valgrind and paying attention only to
valgrind errors.
Anyway, the problem is simple and stupid. The original object array is
not nr_objects item long; it is (nr_objects + 1) long, though I'm not
clear why (1-indexing?). So my previous patch was zeroing the final
entry, which was supposed to contain actual data. Oops.
Here's the corrected patch.
-- >8 --
Subject: [PATCH] index-pack: always zero-initialize object_entry list
Commit 38a4556 (index-pack: start learning to emulate
"verify-pack -v", 2011-06-03) added a "delta_depth" counter
to each "struct object_entry". Initially, all object entries
have their depth set to 0; in resolve_delta, we then set the
depth of each delta to "base + 1". Base entries never have
their depth touched, and remain at 0.
To ensure that all depths start at 0, that commit changed
calls to xmalloc the object_entry list into calls to
xcalloc. However, it forgot that we grow the list with
xrealloc later. These extra entries are used when we add an
object from elsewhere pack to complete a thin pack. If we
add a non-delta object, its depth value will just be
uninitialized heap data.
This patch fixes it by zero-initializing entries we add to
the objects list via the xrealloc.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/index-pack.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..5860085 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1107,6 +1107,8 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
objects = xrealloc(objects,
(nr_objects + nr_unresolved + 1)
* sizeof(*objects));
+ memset(objects + nr_objects + 1, 0,
+ nr_unresolved * sizeof(*objects));
f = sha1fd(output_fd, curr_pack);
fix_unresolved_deltas(f, nr_unresolved);
sprintf(msg, _("completed with %d local objects"),
--
1.8.2.22.g4863f63
^ permalink raw reply related
* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 16:11 UTC (permalink / raw)
To: Thomas Rast
Cc: Stefan Zager, git, Jeff King,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <87620n4clo.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> Thomas Rast <trast@student.ethz.ch> writes:
>
>> (gdb) r index-pack --keep --stdin -v --pack_header=2,50757 <borked
>> Starting program: /Users/trast/.local/bin/git index-pack --keep
>> --stdin -v --pack_header=2,50757 <borked
>> Reading symbols for shared libraries +++........................ done
>> Receiving objects: 100% (50757/50757), 24.52 MiB | 13.06 MiB/s, done.
>> Resolving deltas: 25% (10568/42272)
>> Program received signal EXC_BAD_ACCESS, Could not access memory.
>> Reason: KERN_PROTECTION_FAILURE at address: 0x000000014484dfe8
>> [Switching to process 96573 thread 0x10f]
>> 0x000000010017ee20 in use_pack (p=0x100500f30, w_cursor=0x14484e1a0,
>> offset=69638148, left=0x0) at sha1_file.c:866
>> 866 if (!win || !in_window(win, offset)) {
>>
>> This seems to be a SIGBUS triggered by stack overflow, largely based on
>> the observation that
>>
>> (gdb) p &p
>> $6 = (struct packed_git **) 0x144748058
>
> Actually, scratch that; the stack depth is the same no matter what
> ulimits I put (up to 64MB).
Actually scratch that again. It *is* a stack overflow, except that this
is a thread stack, for which the OS X defaults are 512kB apparently, as
opposed to 2MB on linux.
To wit:
(gdb) p &p
$11 = (struct packed_git **) 0x14484e058
(gdb) bt -5
#4093 0x0000000100054947 in find_unresolved_deltas (base=0x144e00000) at index-pack.c:930
#4094 0x0000000100054a79 in resolve_base (obj=0x1011b08c0) at index-pack.c:961
#4095 0x0000000100054ba5 in threaded_second_pass (data=0x100537dd0) at index-pack.c:984
#4096 0x00007fff8ec8b8bf in _pthread_start ()
#4097 0x00007fff8ec8eb75 in thread_start ()
(gdb) f 4094
#4094 0x0000000100054a79 in resolve_base (obj=0x1011b08c0) at index-pack.c:961
961 find_unresolved_deltas(base_obj);
(gdb) p &obj
$12 = (struct object_entry **) 0x1448cdec8
(gdb) p 0x14484e058-0x1448cdec8
$13 = -523888
(gdb) p 512*1024
$14 = 524288
And indeed the following patch fixes it. Sounds like the delta
unpacking needs a rewrite to support stackless operation. Sigh.
diff --git i/builtin/index-pack.c w/builtin/index-pack.c
index 6be99e2..f73291f 100644
--- i/builtin/index-pack.c
+++ w/builtin/index-pack.c
@@ -1075,13 +1075,17 @@ static void resolve_deltas(void)
nr_dispatched = 0;
if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
init_thread();
+ pthread_attr_t attr;
+ pthread_attr_init(&attr);
+ pthread_attr_setstacksize(&attr, 2*1024*1024);
for (i = 0; i < nr_threads; i++) {
- int ret = pthread_create(&thread_data[i].thread, NULL,
+ int ret = pthread_create(&thread_data[i].thread, &attr,
threaded_second_pass, thread_data + i);
if (ret)
die(_("unable to create thread: %s"),
strerror(ret));
}
+ pthread_attr_destroy(&attr);
for (i = 0; i < nr_threads; i++)
pthread_join(thread_data[i].thread, NULL);
cleanup_thread();
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply related
* Re: [PATCH] t7811 (grep-open): remove test_config() redefinition
From: Junio C Hamano @ 2013-03-19 16:09 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List
In-Reply-To: <1363689794-25974-1-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra <artagnon@gmail.com> writes:
> test_config() is already a well-defined function in
> test-lib-functions.sh. Don't duplicate it unnecessarily.
There are other instances of it in t7810 and t4018, with slightly
different styles.
t/t4018-diff-funcname.sh:test_config () {
t/t7810-grep.sh:test_config() {
t/t7811-grep-open.sh:test_config() {
t/test-lib-functions.sh:test_config () {
Incidentally, this is why a consistent coding style helps. It
should be possible to catch all ofhtme with a single
git grep 'test_config () {' t/
but the ones in t78xx wouldn't have been found.
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
> ---
> Found this while randomly grep'ping around.
>
> t/t7811-grep-open.sh | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/t/t7811-grep-open.sh b/t/t7811-grep-open.sh
> index a895778..e1951a5 100755
> --- a/t/t7811-grep-open.sh
> +++ b/t/t7811-grep-open.sh
> @@ -125,11 +125,6 @@ test_expect_success 'modified file' '
> test_cmp empty out
> '
>
> -test_config() {
> - git config "$1" "$2" &&
> - test_when_finished "git config --unset $1"
> -}
> -
> test_expect_success 'copes with color settings' '
> rm -f actual &&
> echo grep.h >expect &&
^ permalink raw reply
* Re: Memory corruption when rebasing with git version 1.8.1.5 on arch
From: Junio C Hamano @ 2013-03-19 16:02 UTC (permalink / raw)
To: Jeff King; +Cc: Bernhard Posselt, git
In-Reply-To: <20130319134452.GA6752@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> and so on. I haven't quite figured out what is going on. It looks like
> we call update_pre_post_images with postlen==0, which causes it to just
> write the fixed postimage into the existing buffer. But of course the
> fixed version is bigger, because we are expanding the tabs into 8
> spaces (but it _doesn't_ break if each line starts with only one tab,
> which confuses me).
I used to be intimately familiar with the update_pre_post_images()
function, but the version after 86c91f91794c (git apply: option to
ignore whitespace differences, 2009-08-04), I won't vouch for it
doing anything sensible. We recently had to do 5de7166d46d2
(apply.c:update_pre_post_images(): the preimage can be truncated,
2012-10-12) to fix one of its corner cases but I would not be
surprised if there are other cases the function gets it all wrong.
I'd come back to the topic after I finish other tasks on my plate,
so if somebody is inclined please go ahead digging this a bit
further; I won't have much head start to begin with in this code
X-<.
^ permalink raw reply
* Re: [PATCH 0/8] Improve git-status --ignored
From: Duy Nguyen @ 2013-03-19 15:58 UTC (permalink / raw)
To: Junio C Hamano
Cc: Karsten Blees, Git List, Erik Faye-Lund, Ramkumar Ramachandra,
Robert Zeh, Antoine Pelisse, Adam Spiers
In-Reply-To: <7vehfbla1y.fsf@alter.siamese.dyndns.org>
On Tue, Mar 19, 2013 at 9:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Would we we better off kicking nd/read-directory-recursive-optim
> back to 'pu' (and eventually ejecting it) and replacing it with a
> reroll of Karsten's series when it comes, perhaps?
I have no problem with that. Whatever better should get in.
--
Duy
^ permalink raw reply
* Re: Make GIT_USE_LOOKUP default?
From: Jeff King @ 2013-03-19 15:55 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Junio C Hamano, Ingo Molnar, Jonathan Nieder, Git Mailing List
In-Reply-To: <CACsJy8BxbJU9-7Q-Ef3cG2VV2cW8YbBGcjNT9wjT+JywDOxyNg@mail.gmail.com>
On Tue, Mar 19, 2013 at 10:43:40PM +0700, Nguyen Thai Ngoc Duy wrote:
> > I could not replicate his benchmarks at all. In fact, my measurements
> > showed a slight slowdown with 1a812f3 (hashcmp(): inline memcmp() by
> > hand to optimize, 2011-04-28).
> [...]
>
> What gcc and glibc versions are you using? With gcc 4.5.3 I got "repz
> cmpsb" after reverting the patch, just like what Ingo described
> (although interestingly it ran a bit faster than current master, glibc
> 2.11.2 on Atom D510 32 bit). gcc 4.6.3 -O2 (on another machine, 64
> bit) produced a call to libc's memcmp instead of "repz cmpsb". I guess
> if "repz cmpsb" is what we are against, then we could pass
> -fno-builtin-memcmp (potential impact to other parts of git though).
I have glibc 2.13. And looking at the changelog, there were a ton of
ssse-optimized memcmp implementations that went into 2.13 [1], so I suspect
that is what is going on.
-Peff
PS As a side note, you may recall a year or two ago when these went in,
because the initial versions walked backwards, meaning memcpys of
overlapping regions started to fail (as they are allowed to, but many
programs do not properly use memmove).
^ permalink raw reply
* Re: [PATCH v2] index-pack: guard nr_resolved_deltas reads by lock
From: Junio C Hamano @ 2013-03-19 15:53 UTC (permalink / raw)
To: Thomas Rast
Cc: Nguyễn Thái Ngọc Duy, git, Jeff King,
Stefan Zager
In-Reply-To: <8ddf4db38f33034b5ebf504a18948bccf841ab72.1363702423.git.trast@student.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> The threaded parts of index-pack increment the number of resolved
> deltas in nr_resolved_deltas guarded by counter_mutex. However, the
> per-thread outer loop accessed nr_resolved_deltas without any locks.
>
> This is not wrong as such, since it doesn't matter all that much
> whether we get an outdated value. However, unless someone proves that
> this one lock makes all the performance difference, it would be much
> cleaner to guard _all_ accesses to the variable with the lock.
>
> The only such use is display_progress() in the threaded section (all
> others are in the conclude_pack() callchain outside the threaded
> part). To make it obvious that it cannot deadlock, move it out of
> work_mutex.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
> ---
>
>> The only thing I don't
>> like here is the double locking (work_lock then counter_lock) is an
>> invitation for potential deadlocks (not now, but who now what can
>> change later). I think you could move work_lock(); down after
>> counter_unlock() so we hold one lock at a time.
>
> Good point.
Thanks guys for fixing my mess with these two patches.
^ permalink raw reply
* Re: [PATCH] index-pack: always zero-initialize object_entry list
From: Jeff King @ 2013-03-19 15:52 UTC (permalink / raw)
To: Thomas Rast
Cc: Junio C Hamano, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319154353.GA10010@sigill.intra.peff.net>
On Tue, Mar 19, 2013 at 11:43:53AM -0400, Jeff King wrote:
> On Tue, Mar 19, 2013 at 04:33:42PM +0100, Thomas Rast wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> > > Commit 38a4556 (index-pack: start learning to emulate
> > > "verify-pack -v", 2011-06-03) added a "delta_depth" counter
> > > to each "struct object_entry". Initially, all object entries
> > > have their depth set to 0; in resolve_delta, we then set the
> > > depth of each delta to "base + 1". Base entries never have
> > > their depth touched, and remain at 0.
> >
> > This patch causes index-pack to fail on the pack that triggered the
> > whole discussion. More in a minute in another side thread, but
> > meanwhile: NAK until we understand what is really going on here.
>
> Odd; that's what I was testing with, and it worked fine.
Ah, interesting. I built the fix on top of d1a0ed1, the first commit
that shows the problem. And it works fine there. But when it is
forward-ported to the current master, it breaks as you saw.
More bisection fun.
-Peff
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 15:45 UTC (permalink / raw)
To: Thomas Rast
Cc: Stefan Zager, git, Jeff King,
Nguyễn Thái Ngọc Duy, Junio C Hamano
In-Reply-To: <87hak74cse.fsf@pctrast.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> (gdb) r index-pack --keep --stdin -v --pack_header=2,50757 <borked
> Starting program: /Users/trast/.local/bin/git index-pack --keep
> --stdin -v --pack_header=2,50757 <borked
> Reading symbols for shared libraries +++........................ done
> Receiving objects: 100% (50757/50757), 24.52 MiB | 13.06 MiB/s, done.
> Resolving deltas: 25% (10568/42272)
> Program received signal EXC_BAD_ACCESS, Could not access memory.
> Reason: KERN_PROTECTION_FAILURE at address: 0x000000014484dfe8
> [Switching to process 96573 thread 0x10f]
> 0x000000010017ee20 in use_pack (p=0x100500f30, w_cursor=0x14484e1a0,
> offset=69638148, left=0x0) at sha1_file.c:866
> 866 if (!win || !in_window(win, offset)) {
>
> This seems to be a SIGBUS triggered by stack overflow, largely based on
> the observation that
>
> (gdb) p &p
> $6 = (struct packed_git **) 0x144748058
Actually, scratch that; the stack depth is the same no matter what
ulimits I put (up to 64MB). Roughly speaking
(gdb) bt 10
#0 0x000000010017ee20 in use_pack (p=0x100500f30, w_cursor=0x14484e1a0, offset=69638148, left=0x0) at sha1_file.c:866
#1 0x000000010018180c in get_delta_base (p=0x100500f30, w_curs=0x14484e1a0, curpos=0x14484e138, type=OBJ_OFS_DELTA, delta_obj_offset=69638146) at sha1_file.c:1609
#2 0x00000001001819e6 in packed_delta_info (p=0x100500f30, w_curs=0x14484e1a0, curpos=69638148, type=OBJ_OFS_DELTA, obj_offset=69638146, sizep=0x0) at sha1_file.c:1655
#3 0x0000000100181c97 in packed_object_info (p=0x100500f30, obj_offset=69638146, sizep=0x0, rtype=0x0) at sha1_file.c:1727
#4 0x0000000100181a25 in packed_delta_info (p=0x100500f30, w_curs=0x14484e2a0, curpos=69638193, type=OBJ_OFS_DELTA, obj_offset=69638190, sizep=0x0) at sha1_file.c:1658
#5 0x0000000100181c97 in packed_object_info (p=0x100500f30, obj_offset=69638190, sizep=0x0, rtype=0x0) at sha1_file.c:1727
#6 0x0000000100181a25 in packed_delta_info (p=0x100500f30, w_curs=0x14484e3a0, curpos=69638240, type=OBJ_OFS_DELTA, obj_offset=69638237, sizep=0x0) at sha1_file.c:1658
#7 0x0000000100181c97 in packed_object_info (p=0x100500f30, obj_offset=69638237, sizep=0x0, rtype=0x0) at sha1_file.c:1727
#8 0x0000000100181a25 in packed_delta_info (p=0x100500f30, w_curs=0x14484e4a0, curpos=69638285, type=OBJ_OFS_DELTA, obj_offset=69638282, sizep=0x0) at sha1_file.c:1658
#9 0x0000000100181c97 in packed_object_info (p=0x100500f30, obj_offset=69638282, sizep=0x0, rtype=0x0) at sha1_file.c:1727
(More stack frames follow...)
(gdb) bt -10
#4088 0x00000001001835f9 in sha1_object_info_extended (sha1=0x1011b0900 "D=L\022eO����}�r\fW\036F�Q\\Q;t�8", oi=0x1448cdc50) at sha1_file.c:2264
#4089 0x00000001001836eb in sha1_object_info (sha1=0x1011b0900 "D=L\022eO����}�r\fW\036F�Q\\Q;t�8", sizep=0x1448cdd28) at sha1_file.c:2286
#4090 0x0000000100053c44 in sha1_object (data=0x146002400, obj_entry=0x0, size=1992, type=OBJ_TREE, sha1=0x1011b0900 "D=L\022eO����}�r\fW\036F�Q\\Q;t�8") at index-pack.c:722
#4091 0x000000010005457f in resolve_delta (delta_obj=0x1011b0900, base=0x144e00000, result=0x144e00040) at index-pack.c:866
#4092 0x00000001000548b6 in find_unresolved_deltas_1 (base=0x144e00000, prev_base=0x0) at index-pack.c:914
#4093 0x0000000100054947 in find_unresolved_deltas (base=0x144e00000) at index-pack.c:930
#4094 0x0000000100054a79 in resolve_base (obj=0x1011b08c0) at index-pack.c:961
#4095 0x0000000100054ba5 in threaded_second_pass (data=0x100537dd0) at index-pack.c:984
#4096 0x00007fff8ec8b8bf in _pthread_start ()
#4097 0x00007fff8ec8eb75 in thread_start ()
That leaves me stumped as to the cause of that SIGBUS, however.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: Make GIT_USE_LOOKUP default?
From: Duy Nguyen @ 2013-03-19 15:43 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ingo Molnar, Jonathan Nieder, Git Mailing List
In-Reply-To: <20130318073229.GA5551@sigill.intra.peff.net>
On Mon, Mar 18, 2013 at 2:32 PM, Jeff King <peff@peff.net> wrote:
> By the way, looking at that made me think for a few minutes about
> hashcmp, and I was surprised to find that we use an open-coded
> comparison loop. That dates back to this thread by Ingo:
>
> http://article.gmane.org/gmane.comp.version-control.git/172286
>
> I could not replicate his benchmarks at all. In fact, my measurements
> showed a slight slowdown with 1a812f3 (hashcmp(): inline memcmp() by
> hand to optimize, 2011-04-28).
>
> Here are my best-of-five numbers for running "git rev-list --objects
> --all >/dev/null" on linux-2.6.git:
>
> [current master, compiled with -O2]
> real 0m45.612s
> user 0m45.140s
> sys 0m0.300s
>
> [current master, compiled with -O3 for comparison]
> real 0m45.588s
> user 0m45.088s
> sys 0m0.312s
>
> [revert 1a812f3 (i.e., go back to memcmp), -O2]
> real 0m44.358s
> user 0m43.876s
> sys 0m0.316s
>
> [open-code first byte, fall back to memcmp, -O2]
> real 0m43.963s
> user 0m43.568s
> sys 0m0.284s
>
> I wonder why we get such different numbers. Ingo said his tests are on a
> Nehalem CPU, as are mine (mine is an i7-840QM). I wonder if we should be
> wrapping the optimization in an #ifdef, but I'm not sure which flag we
> should be checking.
What gcc and glibc versions are you using? With gcc 4.5.3 I got "repz
cmpsb" after reverting the patch, just like what Ingo described
(although interestingly it ran a bit faster than current master, glibc
2.11.2 on Atom D510 32 bit). gcc 4.6.3 -O2 (on another machine, 64
bit) produced a call to libc's memcmp instead of "repz cmpsb". I guess
if "repz cmpsb" is what we are against, then we could pass
-fno-builtin-memcmp (potential impact to other parts of git though).
--
Duy
^ permalink raw reply
* Re: [PATCH] index-pack: always zero-initialize object_entry list
From: Jeff King @ 2013-03-19 15:43 UTC (permalink / raw)
To: Thomas Rast
Cc: Junio C Hamano, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <8738vr5rqh.fsf@pctrast.inf.ethz.ch>
On Tue, Mar 19, 2013 at 04:33:42PM +0100, Thomas Rast wrote:
> Jeff King <peff@peff.net> writes:
>
> > Commit 38a4556 (index-pack: start learning to emulate
> > "verify-pack -v", 2011-06-03) added a "delta_depth" counter
> > to each "struct object_entry". Initially, all object entries
> > have their depth set to 0; in resolve_delta, we then set the
> > depth of each delta to "base + 1". Base entries never have
> > their depth touched, and remain at 0.
>
> This patch causes index-pack to fail on the pack that triggered the
> whole discussion. More in a minute in another side thread, but
> meanwhile: NAK until we understand what is really going on here.
Odd; that's what I was testing with, and it worked fine.
Let me double-check that I didn't screw up my tests. I initially did
something more like:
if (is_delta_type(base->obj->type))
delta_obj->delta_depth = base->obj->delta_depth + 1;
else
delta_obj->delta_depth = 1;
and I'm wondering if I screwed up testing the revised version.
-Peff
^ permalink raw reply
* Re: [ITCH] Specify refspec without remote
From: Junio C Hamano @ 2013-03-19 15:43 UTC (permalink / raw)
To: Holger Hellmuth (IKS)
Cc: Duy Nguyen, Jeff King, Ramkumar Ramachandra, Git List
In-Reply-To: <514852D7.9080607@ira.uka.de>
"Holger Hellmuth (IKS)" <hellmuth@ira.uka.de> writes:
> Am 19.03.2013 12:33, schrieb Duy Nguyen:
>> git push . foo:bar
>
> '.' has more like a "here" semantic, '..' might be a more fitting
> mnemonic here.
Heh, why not say "origin"? Or rename it to "o" if you like in your
own repository ;-)
^ permalink raw reply
* Re: [ITCH] Specify refspec without remote
From: Junio C Hamano @ 2013-03-19 15:43 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Jeff King, Ramkumar Ramachandra, Git List
In-Reply-To: <CACsJy8Ad7rKtMd-6BoBtbVa70F0AaJ+OUjEykNh344tPw7F7Vg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> Assume that we agree on what remote is implied, we could simplify
> parsing by specifying the remote with "." (or something short and
> unambiguous). So the above command would become
>
> git push . foo:bar
That is an established idiom, a handy way to update your own bar
branch to commit pointed by foo branch when and only when the update
fast-forwards.
Please don't.
^ permalink raw reply
* Re: regression in multi-threaded git-pack-index
From: Thomas Rast @ 2013-03-19 15:41 UTC (permalink / raw)
To: Stefan Zager
Cc: git, Jeff King, Nguyễn Thái Ngọc Duy,
Junio C Hamano
In-Reply-To: <20130315224240.50AA340839@wince.sfo.corp.google.com>
szager@google.com (Stefan Zager) writes:
> We have uncovered a regression in this commit:
>
> b8a2486f1524947f232f657e9f2ebf44e3e7a243
>
> The symptom is that 'git fetch' dies with:
>
> error: index-pack died of signal 10
> fatal: index-pack failed
So after that fun detour into threading issues, I have actually managed
to reproduce this problem on OS X even with the three in-flight patches
already applied.
I reduced the issue to this file:
http://thomasrast.ch/download/broken-pack
on which you can run this command in the repo that Stefan provided:
git index-pack --keep --stdin -v --pack_header=2,50757 <broken-pack
I got the file by patching fetch-pack.c to pipe the pack to 'dd
of=broken-pack' instead of index-pack, as I couldn't find any other way
of getting at the data stream before index-pack ruins it.
The funny thing about it is that I get this on OS X:
$ git index-pack --keep --stdin -v --pack_header=2,50757 <borked
Receiving objects: 100% (50757/50757), 24.52 MiB | 23.91 MiB/s, done.
Bus error: 10tas: 24% (10194/42272)
(notice the error) and also
$ gdb --args $(which git)
GNU gdb 6.3.50-20050815 (Apple version gdb-1705) (Fri Jul 1 10:50:06 UTC 2011)
Copyright 2004 Free Software Foundation, Inc.
GDB is free software, covered by the GNU General Public License, and you are
welcome to change it and/or distribute copies of it under certain conditions.
Type "show copying" to see the conditions.
There is absolutely no warranty for GDB. Type "show warranty" for details.
This GDB was configured as "x86_64-apple-darwin"...Reading symbols for shared libraries .... done
(gdb) r index-pack --keep --stdin -v --pack_header=2,50757 <borked
Starting program: /Users/trast/.local/bin/git index-pack --keep --stdin -v --pack_header=2,50757 <borked
Reading symbols for shared libraries +++........................ done
Receiving objects: 100% (50757/50757), 24.52 MiB | 13.06 MiB/s, done.
Resolving deltas: 25% (10568/42272)
Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x000000014484dfe8
[Switching to process 96573 thread 0x10f]
0x000000010017ee20 in use_pack (p=0x100500f30, w_cursor=0x14484e1a0, offset=69638148, left=0x0) at sha1_file.c:866
866 if (!win || !in_window(win, offset)) {
This seems to be a SIGBUS triggered by stack overflow, largely based on
the observation that
(gdb) p &p
$6 = (struct packed_git **) 0x144748058
I can't see anything wrong with the values as such, but if you have good
ideas what I should ask of that debugger, I'm keeping the session
around.
Furthermore, if I run the same command on linux in the provided repo, I
get this instead:
$ git index-pack --fix-thin --keep --stdin -v --pack_header=2,50757 <../broken-pack
Receiving objects: 100% (50757/50757), 24.52 MiB | 18.84 MiB/s, done.
Resolving deltas: 100% (42272/42272), completed with 8264 local objects.
pack 1cd9880470ea812835edde58e8d7752818dc1ead
But when I do it with Peff's patch applied, I get:
$ git index-pack --fix-thin --keep --stdin -v --pack_header=2,50757 <../broken-pack
Empfange Objekte: 100% (50757/50757), 24.52 MiB | 17.92 MiB/s, done.
git: builtin/index-pack.c:897: find_unresolved_deltas_1: Assertion `child->real_type == OBJ_OFS_DELTA' failed.
Aborted
I think the patch is probably still good as it stands, but there's some
underlying breakage going on that hides the problem if we don't clear
that memory...
I'm still looking, but I wanted to get this -- and in particular the
pack -- out for you to play with ;-)
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH] index-pack: always zero-initialize object_entry list
From: Thomas Rast @ 2013-03-19 15:33 UTC (permalink / raw)
To: Jeff King
Cc: Junio C Hamano, Stefan Zager, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319105852.GA15182@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Commit 38a4556 (index-pack: start learning to emulate
> "verify-pack -v", 2011-06-03) added a "delta_depth" counter
> to each "struct object_entry". Initially, all object entries
> have their depth set to 0; in resolve_delta, we then set the
> depth of each delta to "base + 1". Base entries never have
> their depth touched, and remain at 0.
This patch causes index-pack to fail on the pack that triggered the
whole discussion. More in a minute in another side thread, but
meanwhile: NAK until we understand what is really going on here.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Junio C Hamano @ 2013-03-19 15:06 UTC (permalink / raw)
To: Matthieu Moy
Cc: Jonathan Nieder, Jeff King, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <vpq1ubb3o5g.fsf@grenoble-inp.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> No time to review the code now. I thought about implementing something
> like that, but did not do it because I didn't want the change in the
> code to be too big. At some point, we'll have to remove the warning and
> it's easier with my version than with yours. But the "damage" to the
> code do not seem too big, so that's probably OK and will actually reduce
> the pain for some users.
Getting these warnings is a *good* thing.
You may happen to have no changed path outside the current directory
with this particular invocation of "git add -u", or you may do, or
you may not *even* remember if you touched the paths outside.
Training your fingers to type "git add -u ." without having to even
think, is primarily to help the last case.
Squelching of the warning at the top-level is much less problematic
as it is much harder to forget if you are at the top level of the
working tree than forget if you touched paths outside the current
directory. "I know I am at the top, so I type 'git add -u' without
dot---why do you punish me with the warning?" is a much more valid
complaint.
^ permalink raw reply
* Re: [PATCH 3/4] add -u: only show pathless 'add -u' warning when changes exist outside cwd
From: Junio C Hamano @ 2013-03-19 14:57 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Jeff King, Matthieu Moy, git,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20130319052805.GO5062@elie.Belkin>
Jonathan Nieder <jrnieder@gmail.com> writes:
> Yes, that can work, for example like this (replacing the patch you're
> replying to).
I think that would be a better approach if we were to do this. I
still have the same reservation that "this is fundamentally not
worse but still hurts the users more".
> + /*
> + * Check if "git add -A" or "git add -u" was run from a
> + * subdirectory with a modified file outside that directory,
> + * and warn if so.
> + *
> + * "git add -u" will behave like "git add -u :/" instead of
> + * "git add -u ." in the future. This warning prepares for
> + * that change.
> + */
> + if (implicit_dot &&
> + !match_pathspec(implicit_dot, path, strlen(path), 0, NULL)) {
This one really should *not* use match_pathspec(), I think.
It is a special case where we were asked to limit to our directory
but decided to grab everything instead and filtering the outcome
outselves. We should have a "path to the starting directory" aka
"prefix" in implicit_dot and check if path is covered by the prefix
instead.
> + warn_pathless_add();
> + continue;
> + }
> switch (fix_unmerged_status(p, data)) {
> default:
> die(_("unexpected diff status %c"), p->status);
^ 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