Re: [PATCH 1/6] rebase -i: Add the "ref" command

Thanks for the review.

On Mon, Jun 27, 2011 at 11:46:52AM -0700, Junio C Hamano wrote:

Greg Price [off-list ref] writes:

quoted

...
+		if ! grep -Fq " $refname" "$state_dir"/oldrefs 2>/dev/null
+		then
+			echo "$sha1 $refname" >> "$state_dir"/oldrefs

(Style) Extra SP between ">>" and "$state_dir/oldrefs"

Hmm -- it looks like the prevalent style in the codebase is actually
to include the space:

greg@gouda:~/w/git$ git grep -c '>>[^ ]' v1.7.6 git-*.sh
v1.7.6:git-bisect.sh:3
v1.7.6:git-instaweb.sh:2
v1.7.6:git-rebase--interactive.sh:2
v1.7.6:git-stash.sh:1
greg@gouda:~/w/git$ git grep -c '>> ' v1.7.6 git-*.sh
v1.7.6:git-am.sh:1
v1.7.6:git-filter-branch.sh:1
v1.7.6:git-instaweb.sh:8
v1.7.6:git-rebase--interactive.sh:10
v1.7.6:git-rebase--merge.sh:1

and in particular in git-rebase--interactive.sh.  But I could do it
either way.

quoted

@@ -332,6 +334,15 @@ skip)
 abort)
 	git rerere clear
 	read_basic_state
+	[ -n "$oldrefs" ] && echo "$oldrefs" | while read sha1 ref

(Style) I think almost everybody else spells out "test".  Also please
break line before the while, like this:

	test -n "$oldrefs" &&
	echo "$oldrefs" |
	while read sha1 ref
        do
        	...

Sure, done.

quoted

+	do
+		if test "(null)" = $sha1

Who is giving you "(null)"???

I am, myself -- it's what the 'ref' implementation in
git-rebase--interactive.sh uses to indicate that a ref had not existed
and should be deleted on abort.

+       ref)
+               mark_action_done
+               refname=$sha1
+               sha1=$(git rev-parse --quiet --verify "$refname" \
+                       || echo "(null)")
+               if ! grep -Fq " $refname" "$state_dir"/oldrefs 2>/dev/null
+               then
+                       echo "$sha1 $refname" >> "$state_dir"/oldrefs
+               fi

I could change it to something like "-".  It needs to be something
that the 'read' builtin, as used at the top of the loop, treats as a
word.

Greg