From: Jerry Zhang <jerry@skydio.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jerry Zhang via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs
Date: Fri, 14 Oct 2022 15:33:40 -0700 [thread overview]
Message-ID: <CAMKO5CtKorXv+SO821V6s4COp4MZxHbNCCY_-MR=YUjC_vgH4g@mail.gmail.com> (raw)
In-Reply-To: <xmqqa65y9vm0.fsf@gitster.g>
On Fri, Oct 14, 2022 at 10:13 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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.
>
> It is sad that we have two separate implementations in the first
> place. Do you see if it is feasible to unify the implementation
> by reusing one from the other (answering this is not a requirement
> for this patch to be looked at)?
Yeah I wondered this myself, it's tricky because they are actually doing
opposite things: the diff.c logic is adding diff metadata before doing the
patch-id, while the patch-id logic is parsing out the diff metadata. We could
refactor it, but would have to be careful not to accidentally change the output
semantics.
Another possible path to "unifying" the logic would be to add a
"--patch-id" mode
to "git diff' that produces the patch-id of what would be the diff,
rather than the diff
itself. For the usecases that involve piping "git diff" into "git
patch-id", this would
require not needing the separate patch-id tool at all. Of course
people also like
to run "patch-id" on the output of "format-patch" after the fact so
this isn't a perfect
solution either.
Speaking of which, do you have some context as to why we promise that
"git patch-id"
output will remain the same across git versions? Were there cases in
the past where
people actually made persistent databases of patch-ids, or complained
about the output
changing? I ask because this requirement makes it difficult to make
big changes, and
there aren't any tests to verify consistent output between git
versions. Also git itself
is already a persistent database of patches, so I'm not sure why
someone would choose
to implement a new system for this.
>
next prev parent reply other threads:[~2022-10-14 22:33 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-20 5:58 [PATCH 0/2] update internal patch-id to use "stable" algorithm Jerry Zhang via GitGitGadget
2022-09-20 5:58 ` [PATCH 1/2] patch-id: fix stable patch id for binary / header-only Jerry Zhang via GitGitGadget
2022-09-20 5:58 ` [PATCH 2/2] patch-id: use stable patch-id for rebases Jerry Zhang via GitGitGadget
2022-09-20 6:20 ` [PATCH v2 0/2] update internal patch-id to use "stable" algorithm Jerry Zhang via GitGitGadget
2022-09-20 6:20 ` [PATCH v2 1/2] patch-id: fix stable patch id for binary / header-only Jerry Zhang via GitGitGadget
2022-09-20 6:20 ` [PATCH v2 2/2] patch-id: use stable patch-id for rebases Jerry Zhang via GitGitGadget
2022-10-14 8:56 ` [PATCH v3 0/7] patch-id fixes and improvements Jerry Zhang via GitGitGadget
2022-10-14 8:56 ` [PATCH v3 1/7] patch-id: fix stable patch id for binary / header-only Jerry Zhang via GitGitGadget
2022-10-14 8:56 ` [PATCH v3 2/7] patch-id: use stable patch-id for rebases Jerry Zhang via GitGitGadget
2022-10-14 8:56 ` [PATCH v3 3/7] builtin: patch-id: fix patch-id with binary diffs Jerry Zhang via GitGitGadget
2022-10-14 17:13 ` Junio C Hamano
2022-10-14 22:33 ` Jerry Zhang [this message]
2022-10-14 21:12 ` Junio C Hamano
2022-10-14 22:34 ` Jerry Zhang
2022-10-17 15:23 ` Junio C Hamano
2022-10-14 8:56 ` [PATCH v3 4/7] patch-id: fix patch-id for mode changes Jerry Zhang via GitGitGadget
2022-10-14 21:17 ` Junio C Hamano
2022-10-14 8:56 ` [PATCH v3 5/7] builtin: patch-id: add --include-whitespace as a command mode Jerry Zhang via GitGitGadget
2022-10-14 21:24 ` Junio C Hamano
2022-10-14 22:55 ` Jerry Zhang
2022-10-17 15:38 ` Junio C Hamano
2022-10-18 22:12 ` Jerry Zhang
2022-10-14 8:56 ` [PATCH v3 6/7] builtin: patch-id: remove unused diff-tree prefix Jerry Zhang via GitGitGadget
2022-10-14 22:03 ` Junio C Hamano
2022-10-14 8:56 ` [PATCH v3 7/7] documentation: format-patch: clarify requirements for patch-ids to match Jerry Zhang via GitGitGadget
2022-10-17 15:18 ` Junio C Hamano
2022-10-18 21:57 ` Jerry Zhang
2022-10-19 16:19 ` Junio C Hamano
2022-10-20 23:16 ` [PATCH v4 0/6] patch-id fixes and improvements Jerry Zhang via GitGitGadget
2022-10-20 23:16 ` [PATCH v4 1/6] patch-id: fix stable patch id for binary / header-only Jerry Zhang via GitGitGadget
2022-10-20 23:16 ` [PATCH v4 2/6] patch-id: use stable patch-id for rebases Jerry Zhang via GitGitGadget
2022-10-20 23:16 ` [PATCH v4 3/6] builtin: patch-id: fix patch-id with binary diffs Jerry Zhang via GitGitGadget
2022-10-20 23:16 ` [PATCH v4 4/6] patch-id: fix patch-id for mode changes Jerry Zhang via GitGitGadget
2022-10-20 23:16 ` [PATCH v4 5/6] builtin: patch-id: add --verbatim as a command mode Jerry Zhang via GitGitGadget
2022-10-20 23:16 ` [PATCH v4 6/6] builtin: patch-id: remove unused diff-tree prefix Jerry Zhang via GitGitGadget
2022-10-21 9:33 ` Junio C Hamano
2022-10-24 20:07 ` [PATCH v5 0/6] patch-id fixes and improvements Jerry Zhang via GitGitGadget
2022-10-24 20:07 ` [PATCH v5 1/6] patch-id: fix stable patch id for binary / header-only Jerry Zhang via GitGitGadget
2022-10-24 20:07 ` [PATCH v5 2/6] patch-id: use stable patch-id for rebases Jerry Zhang via GitGitGadget
2022-10-24 20:07 ` [PATCH v5 3/6] builtin: patch-id: fix patch-id with binary diffs Jerry Zhang via GitGitGadget
2022-10-24 20:07 ` [PATCH v5 4/6] patch-id: fix patch-id for mode changes Jerry Zhang via GitGitGadget
2022-10-24 20:07 ` [PATCH v5 5/6] builtin: patch-id: add --verbatim as a command mode Jerry Zhang via GitGitGadget
2022-10-24 20:07 ` [PATCH v5 6/6] builtin: patch-id: remove unused diff-tree prefix Jerry Zhang via GitGitGadget
2022-10-24 22:55 ` [PATCH v5 0/6] patch-id fixes and improvements Junio C Hamano
2022-09-21 19:16 ` [PATCH 0/2] update internal patch-id to use "stable" algorithm Junio C Hamano
2022-09-21 20:59 ` Jerry Zhang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMKO5CtKorXv+SO821V6s4COp4MZxHbNCCY_-MR=YUjC_vgH4g@mail.gmail.com' \
--to=jerry@skydio.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).