Re: [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs

From: Jerry Zhang <jerry@skydio.com>
Date: Oct 14, 2022, 22:34 UTC
Message-ID: <CAMKO5CvdDWEd6HPbkg7DP9bZMKNzcvmK+c1UPpuTk7vM1D8i9g@mail.gmail.com>
In-Reply-To: <xmqqmt9y6rem.fsf@gitster.g>

On Fri, Oct 14, 2022 at 2:12 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 17 quoted lines

>
> "Jerry Zhang via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Jerry Zhang <Jerry@skydio.com>
> >
> > "git patch-id" currently doesn't produce correct output if the
> > incoming diff has any binary files. Add logic to
> > get_one_patchid to handle the different possible styles of binary
> > diff. This attempts to keep resulting patch-ids identical to what
> > would be produced by the counterpart logic in diff.c, that is it
> > produces the id by hashing the a and b oids in succession.
>
> I thought I saw that a previous step touched diff.c to change how
> patch ID for a binary diff is computed to match what patch-id
> command computes?  Now we also have to change patch-id?  In the end
> output from both may match, but which one between diff and patch-id
> have we standardised on?

Er yeah let me see if I can simplify.

Before: Internal patch-id w/ unstable + binary was correct Internal patch-id w/ stable + binary was broken builtin patch-id w/ binary was broken

After: Internal patch-id w/ unstable + binary is correct Internal patch-id w/ stable + binary is now correct builtin patch-id w/ binary is now correct

So the "standard" actually came from the one working case from "before", which was the diff.c logic + unstable. I based all new logic on that because it seemed reasonable and correct. Since "internal w/unstable" is never exposed externally, it's perhaps true that i could have invented a totally new format and standardized on that. Hashing the oids in succession is pretty much representative of a binary patch though, so I don't think there's much to be improved on.

>
> Puzzled...