Re: [PATCH] rebase -i: avoid --cherry-pick when rebasing to a direct ancestor

From: Jeff King <peff@peff.net>
Date: Feb 20, 2010, 07:27 UTC
Message-ID: <20100220072728.GA12168@coredump.intra.peff.net>
In-Reply-To: <201002200102.15777.trast@student.ethz.ch>

On Sat, Feb 20, 2010 at 01:02:15AM +0100, Thomas Rast wrote:

Show 18 quoted lines

> > the --cherry-pick is useless since there are no commits to compare to.
> [...]
> Or maybe it's as simple as this?
> 
> diff --git i/revision.c w/revision.c
> index 438cc87..29721ec 100644
> --- i/revision.c
> +++ w/revision.c
> @@ -547,6 +547,9 @@ static void cherry_pick_list(struct commit_list *list, struct rev_info *revs)
>  			right_count++;
>  	}
>  
> +	if (!left_count || !right_count)
> +		return;
> +
>  	left_first = left_count < right_count;
>  	init_patch_ids(&ids);
>  	if (revs->diffopt.nr_paths) {

>From my reading of the code, that is right, but this is the first time I

ever looked at it, so who knows? :)

Also, I was curious whether we correctly respected gitattributes when calculating patch ids (specifically, do we actually treat this thing as binary). But it looks like patch-id's don't make any exceptions for binary files, and they get fed to xdiff. I think this is not buggy (the xdiff consumer seems to get the diff content with a length, so we are not treating the potentially binary data as a C-string anywhere that I see). But it is probably the source of the slowness to xdiff that gigantic files.

Does it really make sense to treat binary files as anything other than a blob for generating patch id? That is, should we simply turn it into:

  binary diff
  $from_sha1
  $to_sha1

and hash that for the patch id?

-Peff