Re: [PATCH] rebase-i-p: only list commits that require rewriting in todo

[Stephen Haberman <stephen@exigencecorp.com>]

> >> + cat "$TODO" | grep -v "${rev:0:7}" > "${TODO}2" ; mv "${TODO}2"
> >> "$TODO"
> >
> > Substring expansion (like ${rev:0:7}) is not portable. At least it
> > doesn't work on FreeBSD /bin/sh, and "it's not even in POSIX", I
> > believe.
>
> True.

Thanks--sorry about that. rev:0:7 was a cute/stupid optimization to
avoid an extra `git rev-parse` call.

> I do not remember the individual patches in the series, but I have to
> say that the script at the tip of the topic is, eh, less than ideal.

Agreed. The previous dropped commits check was nicer, with just one
pass, as todo already had all of the non-dropped of the commits in it.

Now that todo has to include all commits initially to do parent probing,
the dropped commits check requires two passes, dropping lines from todo,
etc., and is a good deal uglier.

> Here is a small untested patch to fix a few issues I spotted while reading
> it for two minutes.
> 
>  * Why filter output from "rev-list --left-right A...B" and look for the
>    ones that begin with ">"?  Wouldn't "rev-list A..B" give that?

Oh--right. I was being too careful about keeping the existing rev-list
call and only changing what I needed that I didn't step back realize
that.

>  * The abbreviated SHA-1 are made with "rev-list --abbrev=7" into $TODO in
>    an earlier invocation, and it can be more than 7 letters to avoid
>    ambiguity.  Not just that "${r:0:7} is not even in POSIX", but use of
>    it here is actively wrong.

Ah, didn't think of that.

>  * There is no point in catting a single file and piping it into grep.

Right, right, I've been trying to get out of that habit.

> diff --git i/git-rebase--interactive.sh w/git-rebase--interactive.sh
> index 848fbe7..a563dea 100755
> --- i/git-rebase--interactive.sh
> +++ w/git-rebase--interactive.sh
> @@ -635,8 +635,8 @@ first and then run 'git rebase --continue' again."
>  				sed -n "s/^>//p" > "$DOTEST"/not-cherry-picks
>  			# Now all commits and note which ones are missing in
>  			# not-cherry-picks and hence being dropped
> -			git rev-list $UPSTREAM...$HEAD --left-right | \
> -				sed -n "s/^>//p" | while read rev
> +			git rev-list $UPSTREAM..$HEAD |
> +			while read rev
>  			do
>  				if test -f "$REWRITTEN"/$rev -a "$(grep "$rev" "$DOTEST"/not-cherry-picks)" = ""
>  				then
> @@ -645,7 +645,8 @@ first and then run 'git rebase --continue' again."
>  					# just the history of its first-parent for others that will
>  					# be rebasing on top of it
>  					git rev-list --parents -1 $rev | cut -d' ' -f2 > "$DROPPED"/$rev
> -					cat "$TODO" | grep -v "${rev:0:7}" > "${TODO}2" ; mv "${TODO}2" "$TODO"
> +					short=$(git rev-list -1 --abbrev-commit --abbrev=7 $rev)
> +					grep -v "^[a-z][a-z]* $short" <"$TODO" > "${TODO}2" ; mv "${TODO}2" "$TODO"
>  					rm "$REWRITTEN"/$rev
>  				fi
>  			done

Looks good--I applied this locally and it passes t3404, t3410, and
t3411. Do I need to do anything else with this, e.g. resubmit/append on
top of the previous series, or do you have it taken care of?

Thanks for reviewing this, and Jeff too. I appreciate the feedback.

I will be bullish about the use cases I'd like to see work (these
preserve merges tweaks and the config setting for `git pull`), but
humble about my patches. This one especially is not elegant, it just
passes the tests. I've been re-reading it looking for a better way to
do it and nothing is jumping out at me. Let me know if you know of a
better approach or would like me to try something else.

Thanks,
Stephen