Re: [PATCH] cherry_pick_list: quit early if one side is empty

From

Jeff King <peff@peff.net>

Date

Feb 21, 2010, 06:50 UTC

Message-ID

<20100221065009.GB2840@coredump.intra.peff.net>

In-Reply-To

<a39e9a6e6f043121568de106f41f5b01eb38a200.1266665083.git.trast@student.ethz.ch>

On Sat, Feb 20, 2010 at 12:42:04PM +0100, Thomas Rast wrote:

> > 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?
> 
> I tend to agree, but I can't seem to find out what flags to flip :-(

diff --git a/diff.c b/diff.c
index 989dbc5..97ce40a 100644
--- a/diff.c
+++ b/diff.c
@@ -3381,6 +3381,14 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 
 		diff_fill_sha1_info(p->one);
 		diff_fill_sha1_info(p->two);
+
+		if (diff_filespec_is_binary(p->one) ||
+		    diff_filespec_is_binary(p->two)) {
+			/* TODO: write binary patch into buffer */
+			git_SHA1_Update(&ctx, buffer, len);
+			continue;
+		}
+
 		if (fill_mmfile(&mf1, p->one) < 0 ||
 				fill_mmfile(&mf2, p->two) < 0)
 			return error("unable to read files to diff");

However, thinking on it more, it is a bit more complicated than that.
The patch-id we generate for --cherry-pick is not purely internal. You
can also generate patch-id's by handing the patch text to git-patch-id,
which strips out whitespace and line numbers.  Which means that whatever
we generate should probably match the binary patch format output by the
regular diff.

On the other hand, what we do now for cherry-pick totally does not match
what "git log" would output, and nobody has complained, so perhaps it is
not a big deal.

And I guess they don't technically need to match. The --cherry-pick
thing is internal (i.e., as long as you use the same format for both
sides, you are fine). But I suspect there are other code paths that use
that patch-id code.

-Peff