{"thread":{"id":"6147","subject":"Re: [BUG] git-fetch -k is broken","startedAt":"2006-11-30T20:11:54Z","lastAt":"2007-01-01T20:42:57Z","messageCount":5,"participants":["Nicolas Pitre","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"296933","messageId":"Pine.LNX.4.64.0611301441440.9647@xanadu.home","threadId":"6147","inReplyTo":null,"subject":"[BUG] git-fetch -k is broken","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-11-30T20:11:54Z","receivedAt":"2006-11-30T20:11:54Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Actually, the .keep file is simply not removed as it should.\n\nBut first it appears that commit f64d7fd2 added an && on line 431 of \ngit-fetch.sh and that cannot be right.  There is simply no condition for \nnot removing the lock file.  It must be removed regardless if the \nprevious command succeeded or not.  Junio?\n\nNow for the actual problem.  I instrumented git-fetch.sh to understand \nwhat's going on as follows:\n\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex 8365785..042040e 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -397,10 +397,12 @@ fetch_main () {\n \t\t  continue ;;\n \t  keep)\n \t\t  pack_lockfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n+echo \"a $pack_lockfile\"\n \t\t  continue ;;\n \t  esac\n \t  found=\n \t  single_force=\n+echo \"b $pack_lockfile\"\n \t  for ref in $refs\n \t  do\n \t      case \"$ref\" in\n@@ -425,10 +427,13 @@ fetch_main () {\n \t      esac\n \t  done\n \t  local_name=$(expr \"z$found\" : 'z[^:]*:\\(.*\\)')\n+echo \"c $pack_lockfile\"\n \t  append_fetch_head \"$sha1\" \"$remote\" \\\n \t\t  \"$remote_name\" \"$remote_nick\" \"$local_name\" \\\n \t\t  \"$not_for_merge\" || exit\n-      done &&\n+echo \"d $pack_lockfile\"\n+      done\n+echo \"e $pack_lockfile\"\n       if [ \"$pack_lockfile\" ]; then rm -f \"$pack_lockfile\"; fi\n     ) || exit ;;\n   esac\n\nAnd performing a fetch -k produced the following output:\n\na .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\n* refs/heads/next: fast forward to branch 'next' of \ngit://git.kernel.org/pub/scm/git/git\n  old..new: ede546b..c41921b\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nb .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nc .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\nd .git/objects/pack/pack-da39a3ee5e6b4b0d3255bfef95601890afd80709.keep\ne\n\nSo....... How the heck could pack_lockfile become empty on the line with \nthe \"e\" mark?\n\n$ /bin/sh --version\nGNU bash, version 3.1.17(1)-release (i686-redhat-linux-gnu)\n\n\n"},{"id":"297721","messageId":"7vd574iqa0.fsf@assigned-by-dhcp.cox.net","threadId":"6147","inReplyTo":"Pine.LNX.4.64.0611301441440.9647@xanadu.home","subject":"Re: [BUG] git-fetch -k is broken","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-30T21:21:59Z","receivedAt":"2006-11-30T21:21:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> Actually, the .keep file is simply not removed as it should.\n>\n> But first it appears that commit f64d7fd2 added an && on line 431 of \n> git-fetch.sh and that cannot be right.  There is simply no condition for \n> not removing the lock file.  It must be removed regardless if the \n> previous command succeeded or not.  Junio?\n\nTrue, but your \"echo\" patch breaks things even more -- when fast\nforward check fails, it should cause the entire command should\nreport that with the exit status.\n\nThat suggests that we need to come up with a way to clean up\nthese .keep files some other way than just being one of the\ncommand near the end.  As to the mysterious \"echo e <empty>\"\nI will not have chance to look at it myself until later today\n(I'm at work now and it is not my git day today).\n"},{"id":"30454","messageId":"Pine.LNX.4.64.0612282031390.18171@xanadu.home","threadId":"6147","inReplyTo":"7vd574iqa0.fsf@assigned-by-dhcp.cox.net","subject":"Re: [BUG] git-fetch -k is broken","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-12-29T01:41:53Z","receivedAt":"2006-12-29T01:41:53Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"\n[ resuming an old thread ]\n\nOn Thu, 30 Nov 2006, Junio C Hamano wrote:\n\n> Nicolas Pitre <nico@cam.org> writes:\n> \n> > Actually, the .keep file is simply not removed as it should.\n> >\n> > But first it appears that commit f64d7fd2 added an && on line 431 of \n> > git-fetch.sh and that cannot be right.  There is simply no condition for \n> > not removing the lock file.  It must be removed regardless if the \n> > previous command succeeded or not.  Junio?\n> \n> True, but your \"echo\" patch breaks things even more -- when fast\n> forward check fails, it should cause the entire command should\n> report that with the exit status.\n\nThis \"echo\" patch was not a fix.  It was only an expeditive hack to \ndemonstrate the problem.  Please consider this stripped down test case \ninstead:\n\n-------- >8\n#!/bin/sh\n#\n\nLF='\n'\nIFS=\"$LF\"\n\n    ( : subshell because we muck with IFS\n      pack_lockfile=\n      IFS=\" \t$LF\"\n      (\n\t  echo \"keep\t123456789abcdef0123456789abcdef012345678\"\n      ) |\n      while read sha1 remote_name\n      do\n\t  case \"$sha1\" in\n\t  # special line coming from index-pack with the pack name\n\t  keep)\n\t\t  pack_lockfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n\t\t  echo \"pack_lockfile set to $pack_lockfile\"\n\t\t  continue ;;\n\t  esac\n      done &&\n      if [ \"$pack_lockfile\" ]; then echo \"rm -f $pack_lockfile\"; fi\n      echo \"pack_lockfile=$pack_lockfile\"\n    )\n-------- >8\n\nThe output I get is:\n\npack_lockfile set to /pack/pack-123456789abcdef0123456789abcdef012345678.keep\npack_lockfile=\n\nIn other words the line with the echo \"rm -f ...\" never shows up and I \ndon't know why.\n\n> That suggests that we need to come up with a way to clean up\n> these .keep files some other way than just being one of the\n> command near the end.  As to the mysterious \"echo e <empty>\"\n> I will not have chance to look at it myself until later today\n> (I'm at work now and it is not my git day today).\n\nI hope you (or anyone else) will be able to have a look at this.  My \nshell programming skills are simply not up to it.\n\n\nNicolas\n"},{"id":"30456","messageId":"7vlkkrsbjx.fsf@assigned-by-dhcp.cox.net","threadId":"6147","inReplyTo":"Pine.LNX.4.64.0612282031390.18171@xanadu.home","subject":"Re: [BUG] git-fetch -k is broken","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-12-29T02:03:46Z","receivedAt":"2006-12-29T02:03:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> [ resuming an old thread ]\n>\n> On Thu, 30 Nov 2006, Junio C Hamano wrote:\n>\n>> Nicolas Pitre <nico@cam.org> writes:\n>> \n>> > Actually, the .keep file is simply not removed as it should.\n>> >\n>> > But first it appears that commit f64d7fd2 added an && on line 431 of \n>> > git-fetch.sh and that cannot be right.  There is simply no condition for \n>> > not removing the lock file.  It must be removed regardless if the \n>> > previous command succeeded or not.  Junio?\n>> \n>> True, but your \"echo\" patch breaks things even more -- when fast\n>> forward check fails, it should cause the entire command should\n>> report that with the exit status.\n>\n> This \"echo\" patch was not a fix.  It was only an expeditive hack to \n> demonstrate the problem.  Please consider this stripped down test case \n> instead:\n>\n> -------- >8\n> #!/bin/sh\n> #\n>\n> LF='\n> '\n> IFS=\"$LF\"\n>\n>     ( : subshell because we muck with IFS\n>       pack_lockfile=\n>       IFS=\" \t$LF\"\n>       (\n> \t  echo \"keep\t123456789abcdef0123456789abcdef012345678\"\n>       ) |\n>       while read sha1 remote_name\n>       do\n> \t  case \"$sha1\" in\n> \t  # special line coming from index-pack with the pack name\n> \t  keep)\n> \t\t  pack_lockfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n> \t\t  echo \"pack_lockfile set to $pack_lockfile\"\n> \t\t  continue ;;\n> \t  esac\n>       done &&\n>       if [ \"$pack_lockfile\" ]; then echo \"rm -f $pack_lockfile\"; fi\n>       echo \"pack_lockfile=$pack_lockfile\"\n>     )\n> -------- >8\n>\n> The output I get is:\n>\n> pack_lockfile set to /pack/pack-123456789abcdef0123456789abcdef012345678.keep\n> pack_lockfile=\n>\n> In other words the line with the echo \"rm -f ...\" never shows up and I \n> don't know why.\n\nThe whole while loop is run in a subshell and the process runs\nthe last echo \"pack_lockfile=$pack_lockfile\" is a process\ndifferent from the one that did the other echo in the while\nloop.\n"},{"id":"30622","messageId":"7vr6ue7a26.fsf_-_@assigned-by-dhcp.cox.net","threadId":"6147","inReplyTo":"Pine.LNX.4.64.0612282031390.18171@xanadu.home","subject":"[PATCH] git-fetch: remove .keep file even on other errors.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-01-01T20:42:57Z","receivedAt":"2007-01-01T20:42:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Actually removal of them is needed especially on errors.  The\noriginal code had the removal outside of the process which sets\nthe flag to tell the later step what to remove, but it runs as a\ndownstream of a pipeline and its effect was lost.\n\nSigned-off-by: Junio C Hamano <junkio@cox.net>\n---\n  Nicolas Pitre <nico@cam.org> writes:\n\n  > [ resuming an old thread ]\n\n  Likewise...  This should fix it, I think.\n\n git-fetch.sh |   23 ++++++++++++++++-------\n 1 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex 8bd11f8..466fe59 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -382,13 +382,22 @@ fetch_main () {\n       ;; # we are already done.\n   *)\n     ( : subshell because we muck with IFS\n-      pack_lockfile=\n       IFS=\" \t$LF\"\n       (\n-\t  git-fetch-pack --thin $exec $keep $shallow_depth \"$remote\" $rref || echo failed \"$remote\"\n+\t  git-fetch-pack --thin $exec $keep $shallow_depth \"$remote\" $rref ||\n+\t  echo failed \"$remote\"\n       ) |\n-      while read sha1 remote_name\n-      do\n+      (\n+\ttrap '\n+\t\tif test -n \"$keepfile\" && test -f \"$keepfile\"\n+\t\tthen\n+\t\t\trm -f \"$keepfile\"\n+\t\tfi\n+\t' 0\n+\n+        keepfile=\n+\twhile read sha1 remote_name\n+\tdo\n \t  case \"$sha1\" in\n \t  failed)\n \t\t  echo >&2 \"Fetch failure: $remote\"\n@@ -397,7 +406,7 @@ fetch_main () {\n \t  pack)\n \t\t  continue ;;\n \t  keep)\n-\t\t  pack_lockfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n+\t\t  keepfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n \t\t  continue ;;\n \t  esac\n \t  found=\n@@ -429,8 +438,8 @@ fetch_main () {\n \t  append_fetch_head \"$sha1\" \"$remote\" \\\n \t\t  \"$remote_name\" \"$remote_nick\" \"$local_name\" \\\n \t\t  \"$not_for_merge\" || exit\n-      done &&\n-      if [ \"$pack_lockfile\" ]; then rm -f \"$pack_lockfile\"; fi\n+        done\n+      )\n     ) || exit ;;\n   esac\n \n-- \n1.5.0.rc0.ga105\n"}]}