Thanks for the review. On Mon, Jun 27, 2011 at 11:46:52AM -0700, Junio C Hamano wrote: > Greg Price <price@xxxxxxx> writes: > > > ... > > + 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. > > @@ -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. > > + 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html