Re: [BUG] git-new-workdir doesn't understand packed refs

From: Junio C Hamano <junkio@cox.net>
Date: Apr 18, 2007, 21:31 UTC
Message-ID: <7v4pndfjym.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070418210819.GG5913@xp.machine.xx>

Peter Baumann <waste.manager@gmx.de> writes:

Show 19 quoted lines

> On Wed, Apr 18, 2007 at 11:42:24AM -0700, Junio C Hamano wrote:
>> Peter Baumann <waste.manager@gmx.de> writes:
>> 
>> <ot>
>> 
>> Getting more and more annoyed by your stupid Mail-Followup-To...
>> I do *not* want to bother Julian with a message that points out
>> a flaw (in my opinion) in YOUR reasoning but you are forcing me
>> to send my message that way, which I have to waste time
>> correcting every time.  Grumble.
>> 
>> </ot>
>
> Hm. Sorry. I don't understand. I'm just pressing 'g' for group reply in
> mutt which should do the right thing; even your mail has a CC to Julian
> set so I _really_ don't understand the problem. I addressed him in the
> begining because he was the author of git-new-workdir. But please
> forgive me if I'm breaking some netiquette rules but I just started to
> hang out activly on mailinglists ...

Because you had Mail-Followup-To: set to point at me and Julian, when I say "followup", by default I get this in my MUA:

    To: Julian Phillips <julian@quantumfyre.co.uk>
    Cc: git@vger.kernel.org
    Subject: Re: [BUG] git-new-workdir doesn't understand packed refs

Many people prioritize their e-mails depending on where in the header their name appears (ones that have you on Cc: typically gets lower priority than the ones addressed specifically to you by having you on To: line), and if Julian is doing that, sending my message in which I want to talk to YOU that way would steal from Julian's time. So as a general netiquette, I end up hand fixing it, putting you on To: and demoting Julian to Cc:.

I know why you (or some version of mutt) do so. It saves you from filtering incoming duplicates (one addressed to you, another addressed to the mailing list you subscribe to), but it is a misfeature.

Anyhow...

Show 11 quoted lines

>> Also, how is the above different from this?
>> 
>> 	git init a
>>         cd a ; git gc ; cd ..	# allowed
>> 	git new-workdir a b
>> 	cd b ; git gc ; cd ..	# NOT ALLOWED
>> 
>
> Sorry, you lost me here. Your above sequence _is_ allowed and that was
> just the point of the patch. I lightly tested it that it does the right
> thing, so perhaps I'm missing something?

What I was getting at was that if you do not allow new-workdir to be done off of a symlinked one, that was like not allowing gc in a symlinked one. Both are limitations we _could_ lift. But I'd like to take that back, because...

Show 9 quoted lines

> This is even dissallowed by the code in git-new-workdir (Sorry, I just
> saw it now; otherwise I wouldn't spend so much time in arguing this)):
>
> # don't link to a workdir
> if test -L "$orig_git/.git/config"
> then
>         die "\"$orig_git\" is a working directory only, please specify" \
>                 "a complete repository."
> fi

... I missed this one. People cannot make a symlinked one off of another by using new-workdir script, which means perhaps something like this on top of your patch would be safe enough. Sorry for the confusion.

> But with my patch it just works! I really tested it again. The link
> in b/.git/packed-refs -> a/.git/packed-refs (using the example from above)
> isn't broken up and in the new generated packed-refs are stored inside
> the repo a (as they should).

Oh, I never questioned that you made that basic case work. I was worried about not making sure the symlink we are looking at really is the case we are willing to handle, and not erroring out if that is not the case, perhaps like the attached patch on top of yours.

An additional test or two in t/t3210 would be nice to accompany this change.

diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index afa9b5a..1ce4f55 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -123,6 +123,9 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 			die("readlink failed\n");
 		}
 		buf[st.st_size] = '\0';
+		if (!lstat(buf, &st) && S_ISLNK(st.st_mode))
+			die("cannot have doubly symlinked packed-refs file: %s",
+			    ref_file_name);
 		ref_file_name = buf;
 	}