[BUG] git-fetch -k is broken

[BUG] git-fetch -k is broken

[Nicolas Pitre]

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)

Re: [BUG] git-fetch -k is broken

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> 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 <empty>"
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).

Re: [BUG] git-fetch -k is broken

[Nicolas Pitre]

[ resuming an old thread ]

On Thu, 30 Nov 2006, Junio C Hamano wrote:

> Nicolas Pitre <nico@cam.org> 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 <empty>"
> 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

Re: [BUG] git-fetch -k is broken

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> [ resuming an old thread ]
>
> On Thu, 30 Nov 2006, Junio C Hamano wrote:
>
>> Nicolas Pitre <nico@cam.org> 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.

[PATCH] git-fetch: remove .keep file even on other errors.

[Junio C Hamano]

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 <junkio@cox.net>
---
  Nicolas Pitre <nico@cam.org> 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