git-new-workdir doesn't understand packed refs
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
Quoted 2 lines · 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.
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?
Signature · 5 lines
Julian --- My mother is a fish. - William Faulkner
Quoted 8 lines · Julian Phillips
Quoted 2 lines · 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.
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
Quoted 5 lines · Peter Baumann
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.
Quoted 26 lines · Junio C Hamano
Quoted 5 lines · Peter Baumann
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.
Quoted 9 lines
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()inrefs.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
Quoted 26 lines · Peter Baumann
Quoted 15 lines · Junio C Hamano
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.
Quoted 9 lines
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()inrefs.c), and run the creat-to-temp-and-then-rename sequence to update the real file that is pointed at by it.
Quoted 3 lines
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.
Signature · 5 lines
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.
Quoted 18 lines · Julian Phillips
Quoted 14 lines
Quoted 4 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.
Quoted 9 lines
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()inrefs.c), and run the creat-to-temp-and-then-rename sequence to update the real file that is pointed at by it.Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 46 lines · Junio C Hamano
Quoted 18 lines · Julian Phillips
Quoted 14 lines
Quoted 4 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.
Quoted 9 lines
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()inrefs.c), and run the creat-to-temp-and-then-rename sequence to update the real file that is pointed at by it.Quoted 3 lines
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.
Quoted 5 lines
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.
Quoted 3 lines
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.
Quoted 5 lines
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
Quoted 10 lines · Peter Baumann
Quoted 6 lines · Junio C Hamano
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".
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Peter Baumann
Quoted 6 lines · Junio C Hamano
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.
Quoted 8 lines
Quoted 3 lines
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
Peter Baumann <[email protected]> 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>
Quoted 13 lines · Junio C Hamano
Quoted 9 lines
Peter Baumann <[email protected]> writes:
...Quoted 3 lines
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
Quoted 8 lines
git init a
<hack, hack, hack,>
git commit -agit-new-workdir a b # allowed
git-new-workdir a c # allowedgit-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.
Quoted 11 lines · Junio C Hamano
Peter Baumann <[email protected]> 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 ...
Quoted 26 lines
Quoted 14 lines · Junio C Hamano
Quoted 9 lines
Peter Baumann <[email protected]> writes:
...Quoted 3 lines
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
Quoted 8 lines
git init a
<hack, hack, hack,>
git commit -agit-new-workdir a b # allowed
git-new-workdir a c # allowedgit-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.
Quoted 7 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 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
Quoted 3 lines
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 ..." ?
Quoted 7 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.
Quoted 5 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
Quoted 11 lines · Peter Baumann
Quoted 10 lines · Junio C Hamano
Peter Baumann <[email protected]> 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.
Quoted 8 lines
Quoted 1 line
</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 <[email protected]>
Cc: [email protected]
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...
Quoted 11 lines
Quoted 7 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 ALLOWEDSorry, 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...
Quoted 9 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.
Quoted 4 lines
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.
builtin-pack-refs.c +3 -0
@@ -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 1/1 Add test for symlinked .git/packed-refs
---
Quoted 10 lines · Junio C Hamano
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(-)
t/t3210-pack-refs.sh +8 -0
@@ -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
Signature · 1 line
1.5.1
Re: [PATCH] Add test for symlinked .git/packed-refs
Quoted 10 lines · Peter Baumann
Signed-off-by Peter Baumann <[email protected]>---
Quoted 7 lines · Junio C Hamano
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.
Quoted 5 lines
Quoted 3 lines
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 1/1 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.
---
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Peter Baumann
Signed-off-by Peter Baumann <[email protected]>---
Quoted 3 lines · Junio C Hamano
An additional test or two in t/t3210 would be nice to accompany this change.
Something like this?
Quoted 9 lines
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(-)
builtin-pack-refs.c +17 -1
@@ -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)",
t/t3210-pack-refs.sh +15 -0
@@ -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
Signature · 1 line
1.5.1
Re: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink
Quoted 8 lines · 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.
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.
Quoted 10 lines · Peter Baumann
Yes. That's my motivation and it works right now
git init a
<hack, hack, hack,>
git commit -agit-new-workdir a b # allowed
git-new-workdir a c # allowedgit-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 ...
Signature · 5 lines
Julian --- All the world's a stage and most of us are desperately unrehearsed. -- Sean O'Casey
PATCH 1/1 pack-refs: dereference .git/packed-refs if it is a symlink
In reply to Junio C Hamanogit-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.
--- builtin-pack-refs.c | 15 ++++++++++++++- 1 files changed, 14 insertions(+), 1 deletions(-)
builtin-pack-refs.c +14 -1
@@ -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)",
Signature · 1 line
1.5.1
Re: [PATCH] pack-refs: dereference .git/packed-refs if it is a symlink
Quoted 6 lines · 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.
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
Quoted 10 lines · Linus Torvalds
Quoted 6 lines · 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.
Quoted 5 lines
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
Quoted 26 lines · Peter Baumann
Quoted 8 lines · Julian Phillips
Quoted 2 lines · 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.
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/scriptgave 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 ..
Quoted 8 lines
git-new-workdir a b
cd b && git-gcOh. 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.
Signature · 6 lines
Julian --- You know you're using the computer too much when: you call a doctor a "virus scanner" -- Lews_Therin