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

From: Stephen Haberman <stephen@exigencecorp.com>
Date: Oct 22, 2008, 05:01 UTC
Message-ID: <20081022000113.bacc1923.stephen@exigencecorp.com>
In-Reply-To: <7vej2a3kl5.fsf@gitster.siamese.dyndns.org>

Show 8 quoted lines

> >> + 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.

Show 5 quoted lines

> 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.

Show 25 quoted lines

> 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