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
threads / bug / 7720
[BUG] git-new-workdir doesn't understand packed refs
Subject: [BUG] git-new-workdir doesn't understand packed refs
Julian PhillipsRe: [BUG] git-new-workdir doesn't understand packed refs
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
On Tue, Apr 17, 2007 at 10:55:17PM +0100, Julian Phillips wrote:
Show 8 quoted lines
> 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
Julian PhillipsRe: [BUG] git-new-workdir doesn't understand packed refs
On Wed, 18 Apr 2007, Peter Baumann wrote:
Show 34 quoted lines
> 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
Junio C HamanoRe: [BUG] git-new-workdir doesn't understand packed refs
Peter Baumann <waste.manager@gmx.de> writes:
Show 5 quoted lines
> 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
On Wed, Apr 18, 2007 at 12:40:10AM -0700, Junio C Hamano wrote:
Show 35 quoted lines
> 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
Julian PhillipsRe: [BUG] git-new-workdir doesn't understand packed refs
On Wed, 18 Apr 2007, Peter Baumann wrote:
Show 29 quoted lines
> 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.
Junio C HamanoRe: [BUG] git-new-workdir doesn't understand packed refs
Julian Phillips <julian@quantumfyre.co.uk> writes:
Show 23 quoted lines
>>> (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
On Wed, Apr 18, 2007 at 09:23:14AM -0700, Junio C Hamano wrote:
Show 51 quoted lines
> 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
Junio C HamanoRe: [BUG] git-new-workdir doesn't understand packed refs
Peter Baumann <waste.manager@gmx.de> writes:
Show 10 quoted lines
> 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
On Wed, Apr 18, 2007 at 11:17:43AM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> 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
Junio C HamanoRe: [BUG] git-new-workdir doesn't understand packed refs
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>
Show 21 quoted lines
> 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 ALLOWEDYou 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
On Wed, Apr 18, 2007 at 11:42:24AM -0700, Junio C Hamano wrote:
Show 11 quoted lines
> 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 ...
Show 33 quoted lines
> > > 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 ..." ?
Show 7 quoted lines
> 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.
Show 5 quoted lines
> 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
Junio C HamanoRe: [BUG] git-new-workdir doesn't understand packed refs
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 refsMany 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; }
[PATCH] Add test for symlinked .git/packed-refs
Signed-off-by: Peter Baumann <waste.manager@gmx.de> --- On Wed, Apr 18, 2007 at 02:31:29PM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> > 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
Junio C HamanoRe: [PATCH] Add test for symlinked .git/packed-refs
Peter Baumann <waste.manager@gmx.de> writes:
Show 15 quoted lines
> 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
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:
Show 20 quoted lines
> 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
Junio C HamanoRe: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink
Peter Baumann <waste.manager@gmx.de> writes:
Show 8 quoted lines
> 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.
Julian PhillipsRe: [BUG] git-new-workdir doesn't understand packed refs
On Wed, 18 Apr 2007, Peter Baumann wrote:
Show 10 quoted lines
> 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
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
Linus TorvaldsRe: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink
On Wed, 18 Apr 2007, Peter Baumann wrote:
Show 6 quoted lines
> > 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
On Wed, Apr 18, 2007 at 09:09:13AM -0700, Linus Torvalds wrote:
Show 15 quoted lines
> > > 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