# Re: [BUG] git-fetch -k is broken 5 messages from 2006-11-30 to 2007-01-01. Participants: Nicolas Pitre, Junio C Hamano. Thread: https://gitlist.dev/t/6147 ## Nicolas Pitre, 2006-11-30 20:11 Subject: [BUG] git-fetch -k is broken Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0611301441440.9647%40xanadu.home ``` Actually, the .keep file is simply not removed as it should. But first it appears that commit f64d7fd2 added an && on line 431 of git-fetch.sh and that cannot be right. There is simply no condition for not removing the lock file. It must be removed regardless if the previous command succeeded or not. Junio? Now for the actual problem. I instrumented git-fetch.sh to understand what's going on as follows: diff --git a/git-fetch.sh b/git-fetch.sh index 8365785..042040e 100755 --- a/git-fetch.sh +++ b/git-fetch.sh @@ -397,10 +397,12 @@ fetch_main () { continue ;; keep) pack_lockfile="$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep" +echo "a $pack_lockfile" continue ;; esac found= single_force= +echo "b $pack_lockfile" for ref in $refs do case "$ref" in @@ -425,10 +427,13 @@ fetch_main () { esac done local_name=$(expr "z$found" : 'z[^:]*:\(.*\)') +echo "c $pack_lockfile" append_fetch_head "$sha1" "$remote" \ "$remote_name" "$remote_nick" "$local_name" \ "$not_for_merge" || exit - done && +echo "d $pack_lockfile" + done +echo "e $pack_lockfile" if [ "$pack_lockfile" ]; then rm -f "$pack_lockfile"; fi ) || exit ;; esac And performing a fetch -k produced the following output: a .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep * refs/heads/next: fast forward to branch 'next' of git://git.kernel.org/pub/scm/git/git old..new: ede546b..c41921b d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep b .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep c .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep d .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep e So....... How the heck could pack_lockfile become empty on the line with the "e" mark? $ /bin/sh --version GNU bash, version 3.1.17(1)-release (i686-redhat-linux-gnu) ``` ## Junio C Hamano, 2006-11-30 21:21 Subject: Re: [BUG] git-fetch -k is broken Message-ID: <7vd574iqa0.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd574iqa0.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Nicolas Pitre writes: > Actually, the .keep file is simply not removed as it should. > > But first it appears that commit f64d7fd2 added an && on line 431 of > git-fetch.sh and that cannot be right. There is simply no condition for > not removing the lock file. It must be removed regardless if the > previous command succeeded or not. Junio? True, but your "echo" patch breaks things even more -- when fast forward check fails, it should cause the entire command should report that with the exit status. That suggests that we need to come up with a way to clean up these .keep files some other way than just being one of the command near the end. As to the mysterious "echo e " I will not have chance to look at it myself until later today (I'm at work now and it is not my git day today). ``` ## Nicolas Pitre, 2006-12-29 01:41 Subject: Re: [BUG] git-fetch -k is broken Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0612282031390.18171%40xanadu.home In-Reply-To: <7vd574iqa0.fsf@assigned-by-dhcp.cox.net> ``` [ resuming an old thread ] On Thu, 30 Nov 2006, Junio C Hamano wrote: > Nicolas Pitre writes: > > > Actually, the .keep file is simply not removed as it should. > > > > But first it appears that commit f64d7fd2 added an && on line 431 of > > git-fetch.sh and that cannot be right. There is simply no condition for > > not removing the lock file. It must be removed regardless if the > > previous command succeeded or not. Junio? > > True, but your "echo" patch breaks things even more -- when fast > forward check fails, it should cause the entire command should > report that with the exit status. This "echo" patch was not a fix. It was only an expeditive hack to demonstrate the problem. Please consider this stripped down test case instead: -------- >8 #!/bin/sh # LF=' ' IFS="$LF" ( : subshell because we muck with IFS pack_lockfile= IFS=" $LF" ( echo "keep 123456789abcdef0123456789abcdef012345678" ) | while read sha1 remote_name do case "$sha1" in # special line coming from index-pack with the pack name keep) pack_lockfile="$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep" echo "pack_lockfile set to $pack_lockfile" continue ;; esac done && if [ "$pack_lockfile" ]; then echo "rm -f $pack_lockfile"; fi echo "pack_lockfile=$pack_lockfile" ) -------- >8 The output I get is: pack_lockfile set to /pack/pack-123456789abcdef0123456789abcdef012345678.keep pack_lockfile= In other words the line with the echo "rm -f ..." never shows up and I don't know why. > That suggests that we need to come up with a way to clean up > these .keep files some other way than just being one of the > command near the end. As to the mysterious "echo e " > I will not have chance to look at it myself until later today > (I'm at work now and it is not my git day today). I hope you (or anyone else) will be able to have a look at this. My shell programming skills are simply not up to it. Nicolas ``` ## Junio C Hamano, 2006-12-29 02:03 Subject: Re: [BUG] git-fetch -k is broken Message-ID: <7vlkkrsbjx.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlkkrsbjx.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Nicolas Pitre writes: > [ resuming an old thread ] > > On Thu, 30 Nov 2006, Junio C Hamano wrote: > >> Nicolas Pitre writes: >> >> > Actually, the .keep file is simply not removed as it should. >> > >> > But first it appears that commit f64d7fd2 added an && on line 431 of >> > git-fetch.sh and that cannot be right. There is simply no condition for >> > not removing the lock file. It must be removed regardless if the >> > previous command succeeded or not. Junio? >> >> True, but your "echo" patch breaks things even more -- when fast >> forward check fails, it should cause the entire command should >> report that with the exit status. > > This "echo" patch was not a fix. It was only an expeditive hack to > demonstrate the problem. Please consider this stripped down test case > instead: > > -------- >8 > #!/bin/sh > # > > LF=' > ' > IFS="$LF" > > ( : subshell because we muck with IFS > pack_lockfile= > IFS=" $LF" > ( > echo "keep 123456789abcdef0123456789abcdef012345678" > ) | > while read sha1 remote_name > do > case "$sha1" in > # special line coming from index-pack with the pack name > keep) > pack_lockfile="$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep" > echo "pack_lockfile set to $pack_lockfile" > continue ;; > esac > done && > if [ "$pack_lockfile" ]; then echo "rm -f $pack_lockfile"; fi > echo "pack_lockfile=$pack_lockfile" > ) > -------- >8 > > The output I get is: > > pack_lockfile set to /pack/pack-123456789abcdef0123456789abcdef012345678.keep > pack_lockfile= > > In other words the line with the echo "rm -f ..." never shows up and I > don't know why. The whole while loop is run in a subshell and the process runs the last echo "pack_lockfile=$pack_lockfile" is a process different from the one that did the other echo in the while loop. ``` ## Junio C Hamano, 2007-01-01 20:42 Subject: [PATCH] git-fetch: remove .keep file even on other errors. Message-ID: <7vr6ue7a26.fsf_-_@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vr6ue7a26.fsf_-_%40assigned-by-dhcp.cox.net In-Reply-To: ``` Actually removal of them is needed especially on errors. The original code had the removal outside of the process which sets the flag to tell the later step what to remove, but it runs as a downstream of a pipeline and its effect was lost. Signed-off-by: Junio C Hamano --- Nicolas Pitre writes: > [ resuming an old thread ] Likewise... This should fix it, I think. git-fetch.sh | 23 ++++++++++++++++------- 1 files changed, 16 insertions(+), 7 deletions(-) diff --git a/git-fetch.sh b/git-fetch.sh index 8bd11f8..466fe59 100755 --- a/git-fetch.sh +++ b/git-fetch.sh @@ -382,13 +382,22 @@ fetch_main () { ;; # we are already done. *) ( : subshell because we muck with IFS - pack_lockfile= IFS=" $LF" ( - git-fetch-pack --thin $exec $keep $shallow_depth "$remote" $rref || echo failed "$remote" + git-fetch-pack --thin $exec $keep $shallow_depth "$remote" $rref || + echo failed "$remote" ) | - while read sha1 remote_name - do + ( + trap ' + if test -n "$keepfile" && test -f "$keepfile" + then + rm -f "$keepfile" + fi + ' 0 + + keepfile= + while read sha1 remote_name + do case "$sha1" in failed) echo >&2 "Fetch failure: $remote" @@ -397,7 +406,7 @@ fetch_main () { pack) continue ;; keep) - pack_lockfile="$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep" + keepfile="$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep" continue ;; esac found= @@ -429,8 +438,8 @@ fetch_main () { append_fetch_head "$sha1" "$remote" \ "$remote_name" "$remote_nick" "$local_name" \ "$not_for_merge" || exit - done && - if [ "$pack_lockfile" ]; then rm -f "$pack_lockfile"; fi + done + ) ) || exit ;; esac -- 1.5.0.rc0.ga105 ```