Re: [PATCH] hash binary sha1 into patch id

From: Clemens Buchacher <drizzd@aon.at>
Date: Aug 13, 2010, 21:23 UTC
Message-ID: <20100813212331.GA24127@localhost>
In-Reply-To: <20100813200031.GD2003@burratino>

Hi Jonathan,

On Fri, Aug 13, 2010 at 03:00:31PM -0500, Jonathan Nieder wrote:

Show 10 quoted lines

> Clemens Buchacher wrote:
> 
> > Since commit 2f82f760 (Take binary diffs into account for "git rebase"), binary
> > files are included in patch ID computation. Binary files are diffed using the
> > text diff algorithm, however
> [...]
> > Instead of hashing the diff of binary files, use the post-image sha1, which is
> > just as unique. As a result, performance is much improved.
> 
> Maybe it should use both the pre- and post-image?

That would make the patch ID more correct in that it will identify a particular change. But ultimately, we want to know whether or not a change has been applied already. If the contents of a binary file are the same in both commits, this is almost certainly true, regardless of whether or not the pre-images match.

So I think we get better behavior if we ignore the pre-image. Although the difference is probably minuscule.

Show 17 quoted lines

> 
> > diff --git a/diff.c b/diff.c
> > index 17873f3..20fc6db 100644
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -3758,6 +3758,12 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
> >  					len2, p->two->path);
> >  		git_SHA1_Update(&ctx, buffer, len1);
> >  
> > +		if (diff_filespec_is_binary(p->two)) {
> > +			len1 = sprintf(buffer, "%s", sha1_to_hex(p->two->sha1));
> > +			git_SHA1_Update(&ctx, buffer, len1);
> 
> 
> i.e., maybe also
> 
> 			git_SHA1_Update(&ctx, sha1_to_hex(p->one->sha1), 40);

Thanks.