Weird problem with long $PATH and alternates (bisected)

Weird problem with long $PATH and alternates (bisected)

[Mikael Magnusson]

% mkdir 1; cd 1
% echo > a; git add a; git commit -m a
% cd ..
% git clone -s 1 2
% git push . master:master
fatal: Could not switch to
'/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
fatal: The remote end hung up unexpectedly
% PATH=/bin:/usr/bin git push . master:master
Everything up-to-date

The same thing happens if i try to push from 1 to 2:
% cd ../1
% git push ../2
fatal: Could not switch to
'/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
fatal: The remote end hung up unexpectedly

% echo $#PATH
196
% echo $PATH
/home/mikaelh/bin:/opt/bin:/usr/local/bin:/usr/bin:/bin:/usr/local/sbin:/usr/sbin:/sbin:/usr/i686-pc-linux-gnu/gcc-bin/4.2.4:/usr/kde/3.5/bin:/usr/qt/3/bin:/usr/games/bin:/usr/local/ipod-chain/bin

d79796bcf05b89774671a75b3285000c43129823 is first bad commit
commit d79796bcf05b89774671a75b3285000c43129823
Author: Junio C Hamano <gitster@pobox.com>
Date:   Tue Sep 9 01:27:10 2008 -0700

    push: receiver end advertises refs from alternate repositories

    Earlier, when pushing into a repository that borrows from alternate object
    stores, we followed the longstanding design decision not to trust refs in
    the alternate repository that houses the object store we are borrowing
    from.  If your public repository is borrowing from Linus's public
    repository, you pushed into it long time ago, and now when you try to push
    your updated history that is in sync with more recent history from Linus,
    you will end up sending not just your own development, but also the
    changes you acquired through Linus's tree, even though the objects needed
    for the latter already exists at the receiving end.  This is because the
    receiving end does not advertise that the objects only reachable from the
    borrowed repository (i.e. Linus's) are already available there.

    This solves the issue by making the receiving end advertise refs from
    borrowed repositories.  They are not sent with their true names but with a
    phoney name ".have" to make sure that the old senders will safely ignore
    them (otherwise, the old senders will misbehave, trying to push matching
    refs, and mirror push that deletes refs that only exist at the receiving
    end).

    Signed-off-by: Junio C Hamano <gitster@pobox.com>

:100644 100644 6d6027ead17b86519d69c3dfc9b98c01e916d277
45e3cd90fd476cdb0a32e5de27739b18e060e031 Mbuiltin-receive-pack.c
:100644 100644 98a742122dbacbb39fa104cdfe909a9a884ed7b6
99af83a0479ef472078afcd288b1f6ba6283a2f0 Mcache.h
:100644 100644 ae550e89c0942bb9e34b252b653c40e869a074a4
12be17b5dace07a2ca71613e4e9093cdb77492ac Msha1_file.c


-- 
Mikael Magnusson

Re: Weird problem with long $PATH and alternates (bisected)

[Junio C Hamano]

"Mikael Magnusson" <mikachu@gmail.com> writes:

> % mkdir 1; cd 1
> % echo > a; git add a; git commit -m a
> % cd ..
> % git clone -s 1 2
> % git push . master:master
> fatal: Could not switch to
> '/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
> fatal: The remote end hung up unexpectedly

I think I see a bug in foreach_alt_odb() to add_refs_from_alternate()
callchain, but I cannot explain why the contents of $PATH leaks to the
error message.

Can you try this patch?

 sha1_file.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git i/sha1_file.c w/sha1_file.c
index ab2b520..98e7d7a 100644
--- i/sha1_file.c
+++ w/sha1_file.c
@@ -402,9 +402,11 @@ void foreach_alt_odb(alt_odb_fn fn, void *cb)
 	struct alternate_object_database *ent;
 
 	prepare_alt_odb();
-	for (ent = alt_odb_list; ent; ent = ent->next)
+	for (ent = alt_odb_list; ent; ent = ent->next) {
+		*ent->name = '\0';
 		if (fn(ent, cb))
 			return;
+	}
 }
 
 void prepare_alt_odb(void)

Re: Weird problem with long $PATH and alternates (bisected)

[Mikael Magnusson]

2008/10/26 Junio C Hamano <gitster@pobox.com>:
> "Mikael Magnusson" <mikachu@gmail.com> writes:
>
>> % mkdir 1; cd 1
>> % echo > a; git add a; git commit -m a
>> % cd ..
>> % git clone -s 1 2
>> % git push . master:master
>> fatal: Could not switch to
>> '/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
>> fatal: The remote end hung up unexpectedly
>
> I think I see a bug in foreach_alt_odb() to add_refs_from_alternate()
> callchain, but I cannot explain why the contents of $PATH leaks to the
> error message.
>
> Can you try this patch?

Yeah this fixes the issue for me. If I add this
static int add_refs_from_alternate(struct alternate_object_database
*e, void *unused)
{
+	fprintf(stderr, "add: %s\n", e->base);
	char *other = xstrdup(make_absolute_path(e->base));
	size_t len = strlen(other);

and

	prepare_alt_odb();
	for (ent = alt_odb_list; ent; ent = ent->next) {
		fprintf(stderr, "for: %s\n", ent->name);
		if (fn(ent, cb))
			return;

then i get
for: in//usr/qt/3/bin:/usr/games/bin:/usr/loca
add: /tmp/a/1/.git/objects/in//usr/qt/3/bin:/usr/games/bin:/usr/loca
fatal: Could not switch to
'/tmp/a/1/.git/objects/in//usr/qt/3/bin:/usr/games/bin:/usr'
fatal: The remote end hung up unexpectedly

-- 
Mikael Magnusson

Re: Weird problem with long $PATH and alternates (bisected)

[René Scharfe]

Junio C Hamano schrieb:
> "Mikael Magnusson" <mikachu@gmail.com> writes:
> 
>> % mkdir 1; cd 1
>> % echo > a; git add a; git commit -m a
>> % cd ..
>> % git clone -s 1 2
>> % git push . master:master
>> fatal: Could not switch to
>> '/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
>> fatal: The remote end hung up unexpectedly
> 
> I think I see a bug in foreach_alt_odb() to add_refs_from_alternate()
> callchain, but I cannot explain why the contents of $PATH leaks to the
> error message.

With the following patch, I can no longer reproduce the problem.  Does it
work fo you, too?

Thanks,
René


diff --git a/sha1_file.c b/sha1_file.c
index ab2b520..8044e9c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -269,7 +269,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
 		entlen += base_len;
 		pfxlen += base_len;
 	}
-	ent = xmalloc(sizeof(*ent) + entlen);
+	ent = xcalloc(1, sizeof(*ent) + entlen);
 
 	if (!is_absolute_path(entry) && relative_base) {
 		memcpy(ent->base, relative_base, base_len - 1);

Re: Weird problem with long $PATH and alternates (bisected)

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Junio C Hamano schrieb:
>> "Mikael Magnusson" <mikachu@gmail.com> writes:
>> 
>>> % mkdir 1; cd 1
>>> % echo > a; git add a; git commit -m a
>>> % cd ..
>>> % git clone -s 1 2
>>> % git push . master:master
>>> fatal: Could not switch to
>>> '/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
>>> fatal: The remote end hung up unexpectedly
>> 
>> I think I see a bug in foreach_alt_odb() to add_refs_from_alternate()
>> callchain, but I cannot explain why the contents of $PATH leaks to the
>> error message.
>
> With the following patch, I can no longer reproduce the problem.  Does it
> work fo you, too?
>
> Thanks,
> René
>
> diff --git a/sha1_file.c b/sha1_file.c
> index ab2b520..8044e9c 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -269,7 +269,7 @@ static int link_alt_odb_entry(const char * entry, int len, const char * relative
>  		entlen += base_len;
>  		pfxlen += base_len;
>  	}
> -	ent = xmalloc(sizeof(*ent) + entlen);
> +	ent = xcalloc(1, sizeof(*ent) + entlen);

Ah, that would explain the "filled with garbage from $PATH" issue, but I
don't think it fixes the fundamental issue.

In the alternate_object_database structure, ent->base[] is a buffer the
users can use to form pathnames to loose objects, and ent->name is a
pointer into that buffer (it points at one beyond ".git/objects/").  If
you get a call to add_refs_from_alternate() after somebody used the entry
(has_loose_object() has been called, for example), *ent->name would not be
NUL, and ent->base[] won't be the path to the object store.

This caller is expecting to read the path to the object store in ent->base[];
it needs to NUL terminate the buffer if it wants to.

I think the previous patch to sha1_file.c, while it may fix the issue, is
not quite nice.  Here is a replacement that should work.

 builtin-receive-pack.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git c/builtin-receive-pack.c w/builtin-receive-pack.c
index 45e3cd9..9f60f31 100644
--- c/builtin-receive-pack.c
+++ w/builtin-receive-pack.c
@@ -466,12 +466,17 @@ static int delete_only(struct command *cmd)
 
 static int add_refs_from_alternate(struct alternate_object_database *e, void *unused)
 {
-	char *other = xstrdup(make_absolute_path(e->base));
-	size_t len = strlen(other);
+	char *other;
+	size_t len;
 	struct remote *remote;
 	struct transport *transport;
 	const struct ref *extra;
 
+	e->name[-1] = '\0';
+	other = xstrdup(make_absolute_path(e->base));
+	e->name[-1] = '/';
+	len = strlen(other);
+
 	while (other[len-1] == '/')
 		other[--len] = '\0';
 	if (len < 8 || memcmp(other + len - 8, "/objects", 8))

Re: Weird problem with long $PATH and alternates (bisected)

[Mikael Magnusson]

2008/10/26 Junio C Hamano <gitster@pobox.com>:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> Junio C Hamano schrieb:
>>> "Mikael Magnusson" <mikachu@gmail.com> writes:
>>>
>>>> % mkdir 1; cd 1
>>>> % echo > a; git add a; git commit -m a
>>>> % cd ..
>>>> % git clone -s 1 2
>>>> % git push . master:master
>>>> fatal: Could not switch to
>>>> '/tmp/a/1/.git/objects/n:/usr/games/bin:/usr/local/ipod-chain'
>>>> fatal: The remote end hung up unexpectedly
>>>
>>> I think I see a bug in foreach_alt_odb() to add_refs_from_alternate()
>>> callchain, but I cannot explain why the contents of $PATH leaks to the
>>> error message.
>
> I think the previous patch to sha1_file.c, while it may fix the issue, is
> not quite nice.  Here is a replacement that should work.

It does.

-- 
Mikael Magnusson

Re: Weird problem with long $PATH and alternates (bisected)

[Junio C Hamano]

"Mikael Magnusson" <mikachu@gmail.com> writes:

> 2008/10/26 Junio C Hamano <gitster@pobox.com>:
> ...
>> I think the previous patch to sha1_file.c, while it may fix the issue, is
>> not quite nice.  Here is a replacement that should work.
>
> It does.

Thanks.