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

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

[Peter Baumann]

running git-gc or git-gc --prune isn't save because e.g. all the tags
are packed and .git/packed-refs isn't shared on the several workdirs.

This has caused me to lose some tags, but being lucky, I could find those in
the backup.

Greetings,
  Peter

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

[Julian Phillips]

On Tue, 17 Apr 2007, Peter Baumann wrote:

> running git-gc or git-gc --prune isn't save because e.g. all the tags
> are packed and .git/packed-refs isn't shared on the several workdirs.

Do you mean that the link wasn't created?  Or that the link was removed 
and replaced with a file when you ran gc from a workdir?

-- 
Julian

  ---
My mother is a fish.
- William Faulkner

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

[Peter Baumann]

On Tue, Apr 17, 2007 at 10:55:17PM +0100, Julian Phillips wrote:
>  On Tue, 17 Apr 2007, Peter Baumann wrote:
> 
> > running git-gc or git-gc --prune isn't save because e.g. all the tags
> > are packed and .git/packed-refs isn't shared on the several workdirs.
> 
>  Do you mean that the link wasn't created?  Or that the link was removed and
>  replaced with a file when you ran gc from a workdir?
> 

The problem is, when I created the new workdir, I don't have a file
.git/packed-refs, so a new workdir was created with a dangling symlink,
e.g.  workdir/.git/packed-refs -> repo/.git/packed-refs (but the last one
doesn't exist). As it seems, git gc removes the dangling symlink and
replaces it with a file.

Steps to reproduce (written in this mail; after /usr/bin/script gave me an
output whith color coded text *GRR* in ASCII squences):

	mkdir a && cd a && git init
	echo 1 > file.txt
	git add file.txt
	git commit -m "file added"
	git tag v0
	cd ..

	git-new-workdir a b
	cd b && git-gc


Oh. Wait. Just forget that theorie about dangling symlink. git-gc replaces
the symlink in a new workdir with a file. Just confirmed that.

So it isn't save to run git-gc in a workdir.

-Peter

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

[Julian Phillips]

On Wed, 18 Apr 2007, Peter Baumann wrote:

> On Tue, Apr 17, 2007 at 10:55:17PM +0100, Julian Phillips wrote:
>>  On Tue, 17 Apr 2007, Peter Baumann wrote:
>>
>>> running git-gc or git-gc --prune isn't save because e.g. all the tags
>>> are packed and .git/packed-refs isn't shared on the several workdirs.
>>
>>  Do you mean that the link wasn't created?  Or that the link was removed and
>>  replaced with a file when you ran gc from a workdir?
>>
>
> The problem is, when I created the new workdir, I don't have a file
> .git/packed-refs, so a new workdir was created with a dangling symlink,
> e.g.  workdir/.git/packed-refs -> repo/.git/packed-refs (but the last one
> doesn't exist). As it seems, git gc removes the dangling symlink and
> replaces it with a file.
>
> Steps to reproduce (written in this mail; after /usr/bin/script gave me an
> output whith color coded text *GRR* in ASCII squences):
>
> 	mkdir a && cd a && git init
> 	echo 1 > file.txt
> 	git add file.txt
> 	git commit -m "file added"
> 	git tag v0
> 	cd ..
>
> 	git-new-workdir a b
> 	cd b && git-gc
>
>
> Oh. Wait. Just forget that theorie about dangling symlink. git-gc replaces
> the symlink in a new workdir with a file. Just confirmed that.
>
> So it isn't save to run git-gc in a workdir.

True.  I don't think that it would be a good idea to run any purely 
repository type commands in a workdir.

-- 
Julian

  ---
You know you're using the computer too much when:
you call a doctor a "virus scanner"
 	-- Lews_Therin

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

[Junio C Hamano]

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

> The problem is, when I created the new workdir, I don't have a file
> .git/packed-refs, so a new workdir was created with a dangling symlink,
> e.g.  workdir/.git/packed-refs -> repo/.git/packed-refs (but the last one
> doesn't exist). As it seems, git gc removes the dangling symlink and
> replaces it with a file.

Yes, packed-refs file is creat-to-temp-and-then-rename, and we
will lose the sharing if it is run in the symlink-shared work
tree.

We can do one of two things.  I am not sure which one is better.

 (0) The effect of 'git gc' by definition in the symlink-shared
     work tree should be the same as in the original repository
     as the former is to share all the refspace and object
     database.  So we _could_ declare that running 'git gc' in
     symlink-shared work tree is insane and educate people to
     run that in the original repository.  This is _not_ doing
     anything.

 (1) We could by convention declare a worktree whose .git/refs
     is a symlink, and have git-gc and friends check for it, and
     either refuse to run or automatically chdir and run there.

     If we were to do this, we probably should check more than
     just .git/refs but some other symlinks under .git/ as well.

 (2) We could dereference .git/packed-refs, when it is a
     symlink, by hand, just like we dereference a symlink HEAD
     by hand (see resolve_ref() in refs.c), and run the
     creat-to-temp-and-then-rename sequence to update the real
     file that is pointed at by it.


     

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

[Peter Baumann]

On Wed, Apr 18, 2007 at 12:40:10AM -0700, Junio C Hamano wrote:
> Peter Baumann <waste.manager@gmx.de> writes:
> 
> > The problem is, when I created the new workdir, I don't have a file
> > .git/packed-refs, so a new workdir was created with a dangling symlink,
> > e.g.  workdir/.git/packed-refs -> repo/.git/packed-refs (but the last one
> > doesn't exist). As it seems, git gc removes the dangling symlink and
> > replaces it with a file.
> 
> Yes, packed-refs file is creat-to-temp-and-then-rename, and we
> will lose the sharing if it is run in the symlink-shared work
> tree.
> 
> We can do one of two things.  I am not sure which one is better.
> 
>  (0) The effect of 'git gc' by definition in the symlink-shared
>      work tree should be the same as in the original repository
>      as the former is to share all the refspace and object
>      database.  So we _could_ declare that running 'git gc' in
>      symlink-shared work tree is insane and educate people to
>      run that in the original repository.  This is _not_ doing
>      anything.
> 
>  (1) We could by convention declare a worktree whose .git/refs
>      is a symlink, and have git-gc and friends check for it, and
>      either refuse to run or automatically chdir and run there.
> 
>      If we were to do this, we probably should check more than
>      just .git/refs but some other symlinks under .git/ as well.
> 
>  (2) We could dereference .git/packed-refs, when it is a
>      symlink, by hand, just like we dereference a symlink HEAD
>      by hand (see resolve_ref() in refs.c), and run the
>      creat-to-temp-and-then-rename sequence to update the real
>      file that is pointed at by it.
>

Its not all the clear which one is the best, but (2) sounds as the most
promosing aproach. Hopefully, I'll have time to cook up a patch this
evening.

-Peter

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

[Julian Phillips]

On Wed, 18 Apr 2007, Peter Baumann wrote:

> On Wed, Apr 18, 2007 at 12:40:10AM -0700, Junio C Hamano wrote:
>>
>> We can do one of two things.  I am not sure which one is better.
>>
>>  (0) The effect of 'git gc' by definition in the symlink-shared
>>      work tree should be the same as in the original repository
>>      as the former is to share all the refspace and object
>>      database.  So we _could_ declare that running 'git gc' in
>>      symlink-shared work tree is insane and educate people to
>>      run that in the original repository.  This is _not_ doing
>>      anything.
>>
>>  (1) We could by convention declare a worktree whose .git/refs
>>      is a symlink, and have git-gc and friends check for it, and
>>      either refuse to run or automatically chdir and run there.
>>
>>      If we were to do this, we probably should check more than
>>      just .git/refs but some other symlinks under .git/ as well.
>>
>>  (2) We could dereference .git/packed-refs, when it is a
>>      symlink, by hand, just like we dereference a symlink HEAD
>>      by hand (see resolve_ref() in refs.c), and run the
>>      creat-to-temp-and-then-rename sequence to update the real
>>      file that is pointed at by it.
>>
>
> Its not all the clear which one is the best, but (2) sounds as the most
> promosing aproach. Hopefully, I'll have time to cook up a patch this
> evening.

Personally I think (1) might be slightly better, in the refuse to run 
form.  gc is a repository operation, not a working directory one - and by 
refusing to run in a workdir this is made clear.  You could print out a 
message that includes the location of the actual repo to be more friendly 
though.

But whatever solution you go for, you can't use _any_ workdir that points 
at a repo that is having gc run on, either directly or indirectly, without 
risky odd behaviour.

-- 
Julian

  ---
Q:	How many supply-siders does it take to change a light bulb?
A:	None.  The darkness will cause the light bulb to change by itself.

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

[Junio C Hamano]

Julian Phillips <julian@quantumfyre.co.uk> writes:

>>>  (1) We could by convention declare a worktree whose .git/refs
>>>      is a symlink, and have git-gc and friends check for it, and
>>>      either refuse to run or automatically chdir and run there.
>>>
>>>      If we were to do this, we probably should check more than
>>>      just .git/refs but some other symlinks under .git/ as well.
>>>
>>>  (2) We could dereference .git/packed-refs, when it is a
>>>      symlink, by hand, just like we dereference a symlink HEAD
>>>      by hand (see resolve_ref() in refs.c), and run the
>>>      creat-to-temp-and-then-rename sequence to update the real
>>>      file that is pointed at by it.
>>>
>>
>> Its not all the clear which one is the best, but (2) sounds as the most
>> promosing aproach. Hopefully, I'll have time to cook up a patch this
>> evening.
>
> Personally I think (1) might be slightly better, in the refuse to run
> form.  gc is a repository operation, not a working directory one - and
> by refusing to run in a workdir this is made clear.  You could print
> out a message that includes the location of the actual repo to be more
> friendly though.

I've seen Peter's patch that attempts to do (2), and I think
that probably is a right direction.  A worktree that borrows a
repository from another worktree is trying to allow you to do
as many things you would normally do in the original worktree,
with a caveat: certain things are less safe and/or confusing and
you must know what you are doing if you use such a setting.

> But whatever solution you go for, you can't use _any_ workdir that
> points at a repo that is having gc run on, either directly or
> indirectly, without risky odd behaviour.

And I think the above is just one of certain things that are
less safe (one "confusing" is that working on the same branch
would result in gremlin updates).  

There still is an issue of what to do if the .git/packed-refs is
a symlink to a symlink.  Peter's patch does a wrong thing, by
creat-then-rename overwriting the symlinked target; at least we
should detect that case and error out, I think.

Recursively dereferencing the symbolic link by hand to a limit
to avoid infinite recursion (error out when we reach the limit)
would be a more elaborate solution that probably is the right
thing to do.

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

[Peter Baumann]

On Wed, Apr 18, 2007 at 09:23:14AM -0700, Junio C Hamano wrote:
> Julian Phillips <julian@quantumfyre.co.uk> writes:
> 
> >>>  (1) We could by convention declare a worktree whose .git/refs
> >>>      is a symlink, and have git-gc and friends check for it, and
> >>>      either refuse to run or automatically chdir and run there.
> >>>
> >>>      If we were to do this, we probably should check more than
> >>>      just .git/refs but some other symlinks under .git/ as well.
> >>>
> >>>  (2) We could dereference .git/packed-refs, when it is a
> >>>      symlink, by hand, just like we dereference a symlink HEAD
> >>>      by hand (see resolve_ref() in refs.c), and run the
> >>>      creat-to-temp-and-then-rename sequence to update the real
> >>>      file that is pointed at by it.
> >>>
> >>
> >> Its not all the clear which one is the best, but (2) sounds as the most
> >> promosing aproach. Hopefully, I'll have time to cook up a patch this
> >> evening.
> >
> > Personally I think (1) might be slightly better, in the refuse to run
> > form.  gc is a repository operation, not a working directory one - and
> > by refusing to run in a workdir this is made clear.  You could print
> > out a message that includes the location of the actual repo to be more
> > friendly though.
> 
> I've seen Peter's patch that attempts to do (2), and I think
> that probably is a right direction.  A worktree that borrows a
> repository from another worktree is trying to allow you to do
> as many things you would normally do in the original worktree,
> with a caveat: certain things are less safe and/or confusing and
> you must know what you are doing if you use such a setting.
> 
> > But whatever solution you go for, you can't use _any_ workdir that
> > points at a repo that is having gc run on, either directly or
> > indirectly, without risky odd behaviour.
> 
> And I think the above is just one of certain things that are
> less safe (one "confusing" is that working on the same branch
> would result in gremlin updates).  
> 
> There still is an issue of what to do if the .git/packed-refs is
> a symlink to a symlink.  Peter's patch does a wrong thing, by
> creat-then-rename overwriting the symlinked target; at least we
> should detect that case and error out, I think.
> 
> Recursively dereferencing the symbolic link by hand to a limit
> to avoid infinite recursion (error out when we reach the limit)
> would be a more elaborate solution that probably is the right
> thing to do.
>
I thought about the case where packed-refs is a symlink to another symlink
and then decided that it's not worth to implement this because a workdir
should be linked to a _repo_ and not another workdir.

-Peter

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

[Junio C Hamano]

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

> On Wed, Apr 18, 2007 at 09:23:14AM -0700, Junio C Hamano wrote:
>> 
>> Recursively dereferencing the symbolic link by hand to a limit
>> to avoid infinite recursion (error out when we reach the limit)
>> would be a more elaborate solution that probably is the right
>> thing to do.
>>
> I thought about the case where packed-refs is a symlink to another symlink
> and then decided that it's not worth to implement this because a workdir
> should be linked to a _repo_ and not another workdir.

That's incredibly weak, as the initial motivation of this patch
is that you did not want to say "you should run gc only in the
_repo_ not in workdir".

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

[Peter Baumann]

On Wed, Apr 18, 2007 at 11:17:43AM -0700, Junio C Hamano wrote:
> Peter Baumann <waste.manager@gmx.de> writes:
> 
> > On Wed, Apr 18, 2007 at 09:23:14AM -0700, Junio C Hamano wrote:
> >> 
> >> Recursively dereferencing the symbolic link by hand to a limit
> >> to avoid infinite recursion (error out when we reach the limit)
> >> would be a more elaborate solution that probably is the right
> >> thing to do.
> >>
> > I thought about the case where packed-refs is a symlink to another symlink
> > and then decided that it's not worth to implement this because a workdir
> > should be linked to a _repo_ and not another workdir.
> 
> That's incredibly weak, as the initial motivation of this patch
> is that you did not want to say "you should run gc only in the
> _repo_ not in workdir".
> 

Yes. That's my motivation and it works right now

	git init a
	<hack, hack, hack,>
	git commit -a

	git-new-workdir a b 	# allowed
	git-new-workdir a c	# allowed

	git-new-workdir b d	# NOT ALLOWED

The user should only create new work dirs which refere to the repo and not
to another workdir.

But *iff* thats the only point for keeping my patch out I'll fix it, but
not tonight. (Leaving now ...)

-Peter

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

[Junio C Hamano]

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>

> On Wed, Apr 18, 2007 at 11:17:43AM -0700, Junio C Hamano wrote:
>> Peter Baumann <waste.manager@gmx.de> writes:
>> ...
>> > I thought about the case where packed-refs is a symlink to another symlink
>> > and then decided that it's not worth to implement this because a workdir
>> > should be linked to a _repo_ and not another workdir.
>> 
>> That's incredibly weak, as the initial motivation of this patch
>> is that you did not want to say "you should run gc only in the
>> _repo_ not in workdir".
>
> Yes. That's my motivation and it works right now
>
> 	git init a
> 	<hack, hack, hack,>
> 	git commit -a
>
> 	git-new-workdir a b 	# allowed
> 	git-new-workdir a c	# allowed
>
> 	git-new-workdir b d	# NOT ALLOWED

But I do not think you are disallowing it; instead you are
making the same problem appear without telling the user.

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

You are saying "you should run workdir only in the _repo_ not in
workdir".

As I already said, certain things work differently between a
proper repository and a worktree that borrows .git/refs from a
proper repository, and you always have to know what you are
doing when you use such a setup.  If your goal is to minimize
the difference, I do not think it makes much sense to allow gc
and not allow new-workdir.

On the other hand, if we admit that things work differently, I
think erroring out gc or pack-refs when we see .git/packed-refs
is a symbolic link is much simpler, less error prone and easier
to explain.

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

[Peter Baumann]

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 ...

> 
> > On Wed, Apr 18, 2007 at 11:17:43AM -0700, Junio C Hamano wrote:
> >> Peter Baumann <waste.manager@gmx.de> writes:
> >> ...
> >> > I thought about the case where packed-refs is a symlink to another symlink
> >> > and then decided that it's not worth to implement this because a workdir
> >> > should be linked to a _repo_ and not another workdir.
> >> 
> >> That's incredibly weak, as the initial motivation of this patch
> >> is that you did not want to say "you should run gc only in the
> >> _repo_ not in workdir".
> >
> > Yes. That's my motivation and it works right now
> >
> > 	git init a
> > 	<hack, hack, hack,>
> > 	git commit -a
> >
> > 	git-new-workdir a b 	# allowed
> > 	git-new-workdir a c	# allowed
> >
> > 	git-new-workdir b d	# NOT ALLOWED
> 
> But I do not think you are disallowing it; instead you are
> making the same problem appear without telling the user.
> 
> 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 isn't allowed is the following:

	mkdir a; cd a; git-init; cd ..
	git new-workdir a b
	cd b; git gc ; cd .. # IS ALLOWED
	git new-workdir b c
	cd b; git gc ; cd .. # NOT ALLOWED

Because now you created a new workdir c which doesn't point to a repo,
but only to another _workdir_ b. And only in this case you get a symlink
chain like this:

c/.git/packed-refs -> b/.git/packed-refs -> a/.git/packed-refs

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

> You are saying "you should run workdir only in the _repo_ not in
> workdir".
> 

This sentence doesn't make any sense to me. Did you mean "you should run
gc only ..." ?

> As I already said, certain things work differently between a
> proper repository and a worktree that borrows .git/refs from a
> proper repository, and you always have to know what you are
> doing when you use such a setup.  If your goal is to minimize
> the difference, I do not think it makes much sense to allow gc
> and not allow new-workdir.
> 

I think you missunderstud me. Hopefully the above explanation clears this
missunderstanding. The case I feared (symlink chain of workdirs) is not
allowed in git-new-workdir from the very begining of this script, so
there shouldn't be any problem with the symlink handling in my patch.

> On the other hand, if we admit that things work differently, I
> think erroring out gc or pack-refs when we see .git/packed-refs
> is a symbolic link is much simpler, less error prone and easier
> to explain.
> 

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).

-Peter

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

[Junio C Hamano]

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

> 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...

>> 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...

> 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;
 	}
 

[PATCH] Add test for symlinked .git/packed-refs

[Peter Baumann]

Signed-off-by: Peter Baumann <waste.manager@gmx.de>
---
On Wed, Apr 18, 2007 at 02:31:29PM -0700, Junio C Hamano wrote:
> 
> 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.
> 

Something like this?

 t/t3210-pack-refs.sh |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index f0c7e22..bf63954 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -105,4 +105,12 @@ test_expect_success 'pack, prune and repack' '
 	diff all-of-them again
 '
 
+test_expect_success \
+	'derefence symlinks for packed-refs' \
+	'mv -f .git/packed-refs .git/real_packed-refs &&
+	ln -s real_packed-refs .git/packed-refs &&
+	git-tag z &&
+	git-pack-refs --all --prune &&
+	diff .git/real_packed-refs .git/packed-refs'
+
 test_done
-- 
1.5.1

Re: [PATCH] Add test for symlinked .git/packed-refs

[Junio C Hamano]

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

> Signed-off-by: Peter Baumann <waste.manager@gmx.de>
> ---
> On Wed, Apr 18, 2007 at 02:31:29PM -0700, Junio C Hamano wrote:
>> 
>> 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.
>> 
>
> Something like this?

That's a good start, but I expected to see at least tests for
two cases: a case in which .git/packed-refs symlink points at an
actual file (i.e. the original repository has run pack-refs) and
another case in which .git/packed-refs symlink is dangling
(i.e. the original repository hasn't run pack-refs).  I
understand that the borrower "worktree" can have .git/packed-refs
symlink pointing at the repositories .git/packed-refs yet to be
born.

[PATCH] pack-refs: dereference .git/packed-refs if it is a symlink

[Peter Baumann]

git-new-workdir creates a new working directory where everything
necessary, including .git/packed-refs, is symlinked to your master repo.
But git-pack-refs breaks the symlink, so you could accidentally loose some
refs.

This fixes git-pack-refs to first dereference .git/packed-refs if it is a
symlink. While we are it, add some tests to prevent this from happening
again.

Signed-off-by: Peter Baumann <waste.manager@gmx.de>
---
On Wed, Apr 18, 2007 at 11:06:45PM -0700, Junio C Hamano wrote:
> Peter Baumann <waste.manager@gmx.de> writes:
> 
> > Signed-off-by: Peter Baumann <waste.manager@gmx.de>
> > ---
> > On Wed, Apr 18, 2007 at 02:31:29PM -0700, Junio C Hamano wrote:
> >> An additional test or two in t/t3210 would be nice to accompany
> >> this change.
> >> 
> >
> > Something like this?
> 
> That's a good start, but I expected to see at least tests for
> two cases: a case in which .git/packed-refs symlink points at an
> actual file (i.e. the original repository has run pack-refs) and
> another case in which .git/packed-refs symlink is dangling
> (i.e. the original repository hasn't run pack-refs).  I
> understand that the borrower "worktree" can have .git/packed-refs
> symlink pointing at the repositories .git/packed-refs yet to be
> born.
> builtin-pack-refs.c  |   18 +++++++++++++++++-

As I couldn't find anything related to this in your repo, I added a test
for a danling symklink and integrated your little fix to check for
doubly symlinked files for easier handling and to not mess up the
history with all does tiny "fixes"

Greetings,
  Peter

 t/t3210-pack-refs.sh |   15 +++++++++++++++
 2 files changed, 32 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index d080e30..1ce4f55 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -89,6 +89,8 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	int fd, i;
 	struct pack_refs_cb_data cbdata;
+	struct stat st;
+	char *ref_file_name;
 
 	memset(&cbdata, 0, sizeof(cbdata));
 
@@ -113,7 +115,21 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	if (i != argc)
 		usage(builtin_pack_refs_usage);
 
-	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"), 1);
+	ref_file_name = git_path("packed-refs");
+	if (!lstat(ref_file_name, &st) && S_ISLNK(st.st_mode)) {
+		char *buf = xmalloc(st.st_size + 1);
+		if (readlink(ref_file_name, buf, st.st_size + 1) != st.st_size) {
+			free(buf);
+			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;
+	}
+
+	fd = hold_lock_file_for_update(&packed, ref_file_name, 1);
 	cbdata.refs_file = fdopen(fd, "w");
 	if (!cbdata.refs_file)
 		die("unable to create ref-pack file structure (%s)",
diff --git a/t/t3210-pack-refs.sh b/t/t3210-pack-refs.sh
index f0c7e22..5756304 100755
--- a/t/t3210-pack-refs.sh
+++ b/t/t3210-pack-refs.sh
@@ -105,4 +105,19 @@ test_expect_success 'pack, prune and repack' '
 	diff all-of-them again
 '
 
+test_expect_success \
+	'derefence symlinks for packed-refs' \
+	'mv -f .git/packed-refs .git/real_packed-refs &&
+	ln -s `pwd`/.git/real_packed-refs .git/packed-refs &&
+	git-tag z &&
+	git-pack-refs --prune &&
+	diff .git/real_packed-refs .git/packed-refs'
+
+test_expect_success \
+	'derefence dangling symlinks for packed-refs' \
+	'git branch dangling_symlink &&
+	rm .git/real_packed-refs
+	git-pack-refs --all --prune &&
+	diff .git/real_packed-refs .git/packed-refs'
+
 test_done
-- 
1.5.1

Re: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink

[Junio C Hamano]

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

> git-new-workdir creates a new working directory where everything
> necessary, including .git/packed-refs, is symlinked to your master repo.
> But git-pack-refs breaks the symlink, so you could accidentally loose some
> refs.
>
> This fixes git-pack-refs to first dereference .git/packed-refs if it is a
> symlink. While we are it, add some tests to prevent this from happening
> again.

Because you are only fixing the case where the worktree is
borrowing the packed-refs file from a real repository with a
symlink trick, and we do not know if somebody had his
packed-refs as a symlink to some random place for reasons other
than creating a lightweight worktree (maybe there was a
mistake), I am wondering if it makes sense to be more strict
about the value we read from readlink().

For example, if it does not end with "/packed-refs", doesn't it
suggest that the reason because the symlink is there is
different from the case you are handling (i.e. it is not a
packed-refs symlink in a lightweight worktree that points at the
corresponding real repository)?  I wonder if in such a case we
would want to signal an error, instead of overwriting whatever
real file the symlink points at.  Or is it too strict and
paranoid?  I dunno.

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

[Julian Phillips]

On Wed, 18 Apr 2007, Peter Baumann wrote:

> Yes. That's my motivation and it works right now
>
> 	git init a
> 	<hack, hack, hack,>
> 	git commit -a
>
> 	git-new-workdir a b 	# allowed
> 	git-new-workdir a c	# allowed
>
> 	git-new-workdir b d	# NOT ALLOWED

btw, if you copy git-new-workdir to $GIT_EXEC_PATH then you can do

 	git new-workdir a b

(and the bash completion script works too. :D)

It's cunning stuff this git program ...

-- 
Julian

  ---
All the world's a stage and most of us are desperately unrehearsed.
 		-- Sean O'Casey

[PATCH] pack-refs: dereference .git/packed-refs if it is a symlink

[Peter Baumann]

git-new-workdir creates a new working directory where everything
necessary, including .git/packed-refs, is symlinked to your master repo.
But git-pack-refs breaks the symlink, so you could accidentally loose some
refs. This fixes it to first dereference .git/packed-refs if it is a
symlink.

Signed-off-by: Peter Baumann <waste.manager@gmx.de>
---
 builtin-pack-refs.c |   15 ++++++++++++++-
 1 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
index d080e30..afa9b5a 100644
--- a/builtin-pack-refs.c
+++ b/builtin-pack-refs.c
@@ -89,6 +89,8 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 {
 	int fd, i;
 	struct pack_refs_cb_data cbdata;
+	struct stat st;
+	char *ref_file_name;
 
 	memset(&cbdata, 0, sizeof(cbdata));
 
@@ -113,7 +115,18 @@ int cmd_pack_refs(int argc, const char **argv, const char *prefix)
 	if (i != argc)
 		usage(builtin_pack_refs_usage);
 
-	fd = hold_lock_file_for_update(&packed, git_path("packed-refs"), 1);
+	ref_file_name = git_path("packed-refs");
+	if (!lstat(ref_file_name, &st) && S_ISLNK(st.st_mode)) {
+		char *buf = xmalloc(st.st_size + 1);
+		if (readlink(ref_file_name, buf, st.st_size + 1) != st.st_size) {
+			free(buf);
+			die("readlink failed\n");
+		}
+		buf[st.st_size] = '\0';
+		ref_file_name = buf;
+	}
+
+	fd = hold_lock_file_for_update(&packed, ref_file_name, 1);
 	cbdata.refs_file = fdopen(fd, "w");
 	if (!cbdata.refs_file)
 		die("unable to create ref-pack file structure (%s)",
-- 
1.5.1

Re: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink

[Linus Torvalds]

On Wed, 18 Apr 2007, Peter Baumann wrote:
>
> git-new-workdir creates a new working directory where everything
> necessary, including .git/packed-refs, is symlinked to your master repo.
> But git-pack-refs breaks the symlink, so you could accidentally loose some
> refs. This fixes it to first dereference .git/packed-refs if it is a
> symlink.

Wouldn't it be nicer to instead make "git gc" *notice* the fact that we're 
in a workdir, and just "cd" to the main git repository instead?

		Linus

Re: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink

[Peter Baumann]

On Wed, Apr 18, 2007 at 09:09:13AM -0700, Linus Torvalds wrote:
> 
> 
> On Wed, 18 Apr 2007, Peter Baumann wrote:
> >
> > git-new-workdir creates a new working directory where everything
> > necessary, including .git/packed-refs, is symlinked to your master repo.
> > But git-pack-refs breaks the symlink, so you could accidentally loose some
> > refs. This fixes it to first dereference .git/packed-refs if it is a
> > symlink.
> 
> Wouldn't it be nicer to instead make "git gc" *notice* the fact that we're 
> in a workdir, and just "cd" to the main git repository instead?
> 
> 		Linus
> 

Don't think so. Because then all the low level tools aren't aware of this.
And restricting a WorkDir to use only porcelanish commands isn't what I
want. And teaching every tool about symklinked workdirs doesn't sound right
to me.

-Peter