threads / bug / 6147

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

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

## tl;dr

5 messages between Nov 30, 2006 and Jan 1, 2007.

replies: 4people: 2as markdown or json

Nicolas Pitre· Nov 30, 2006, 20:11 UTC · lore

[BUG] git-fetch -k is broken

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· Nov 30, 2006, 21:21 UTC · re: Nicolas Pitre · lore
Nicolas Pitre <nico@cam.org> writes:
Show 6 quoted lines
> 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).

Nicolas Pitre· Dec 29, 2006, 01:41 UTC · re: Junio C Hamano · lore
[ resuming an old thread ]
On Thu, 30 Nov 2006, Junio C Hamano wrote:
Show 12 quoted lines
> 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.

Show 5 quoted lines
> 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
Junio C Hamano· Dec 29, 2006, 02:03 UTC · re: Nicolas Pitre · lore
Nicolas Pitre <nico@cam.org> writes:
Show 57 quoted lines
> [ 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.

Junio C Hamano· Jan 1, 2007, 20:42 UTC · re: Nicolas Pitre · lore

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

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

← back to recent threads