[RFC PATCH 0/2] pretty: add %I formatting for patch-id

[RFC PATCH 0/2] pretty: add %I formatting for patch-id

[Michael McClimon]

I'll say up front that this patch doesn't actually work, but I am stuck
on it and interested in continuing, and am hopeful someone can point me
in something closer to the right direction. (Also, the proposed log
messages are not good, and I would not actually submit the patch for
merge consideration with these messages.)

I would like to have a single-command way to get the patch id for a
commit: the thing you'd see in a pipeline like
    git diff-tree --patch-with-raw HEAD | git patch-id

My initial thought was to add a --patch-id flag to git diff-tree, but
then I thought that maybe better would be to add a pretty specifier to
do so, so that (for instance) you could generate patch-ids for
everything in a branch by saying something like
    git log --pretty='%I %H' start..

I have taken a pass at doing so here, but it doesn't _work_, and I'm not
sure why. It seemed like the thing to do was to use commit_patch_id from
patch-ids.c, but I think that either I'm not holding it correctly, or
that it's not fit for purpose. It was added (or rather, made public) in
ded2c097 (patch-ids: make commit_patch_id() a public helper function,
2016-04-26), which was in service of fa2ab86d (format-patch: add
'--base' option to record base tree info, 2016-04-26). It seems like at
the very least, it probably won't work as expected on merge commits.

The thing that is perplexing to me is that it _does_ appear to work on
some commits, for example (where 8b3aa36f is a recent-ish commit from
master, chosen at random):

$ ./git --no-pager show -s --format='%I %H' 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08
ec66952bdef82a1fc6d31c1057195af31c86da48 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08

$ git diff-tree --patch-with-raw 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08 | git patch-id
ec66952bdef82a1fc6d31c1057195af31c86da48 8b3aa36f5a7a0c923bc4a28ff19caae78644ae08

But for other commits, like the one in the test here, it does not. I
have done a bit of investigation, but I would not really call myself a C
programmer and I'm not super familiar with the codebase, so I'm a bit
stuck. I thought maybe at first I wasn't initializing the diff_options
correctly, but I suspect the problem is actually more fundamental than
that.

Anyway: I would be happy to hear any response at all, whether that's 
a) a pointer to some other way of implementing a %I pretty format,
b) an easier-to-implement possible solution (maybe diff-tree --patch-id
would actually be better), or c) saying the thing I want isn't feasible
and I should stick with the pipeline. Thanks!


Michael McClimon (2):
  patch-ids: add const modifier to commit
  pretty: add 'I' placeholder for patch-id

 patch-ids.c                   |  4 ++--
 patch-ids.h                   |  2 +-
 pretty.c                      | 11 +++++++++++
 t/t4205-log-pretty-formats.sh |  7 +++++++
 4 files changed, 21 insertions(+), 3 deletions(-)

-- 
2.42.0.424.gceadf0f3

[RFC PATCH 1/2] patch-ids: add const modifier to commit

[Michael McClimon]

These aren't modified by these functions, and I want to use them
somewhere where the commit is in fact a const.

Signed-off-by: Michael McClimon <michael@mcclimon.org>
---
 patch-ids.c | 4 ++--
 patch-ids.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index c3e1a0dd..ecfd7ba0 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -6,13 +6,13 @@
 #include "hex.h"
 #include "patch-ids.h"
 
-static int patch_id_defined(struct commit *commit)
+static int patch_id_defined(const struct commit *commit)
 {
 	/* must be 0 or 1 parents */
 	return !commit->parents || !commit->parents->next;
 }
 
-int commit_patch_id(struct commit *commit, struct diff_options *options,
+int commit_patch_id(const struct commit *commit, struct diff_options *options,
 		    struct object_id *oid, int diff_header_only)
 {
 	if (!patch_id_defined(commit))
diff --git a/patch-ids.h b/patch-ids.h
index 490d7393..3f61d88a 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -19,7 +19,7 @@ struct patch_ids {
 	struct diff_options diffopts;
 };
 
-int commit_patch_id(struct commit *commit, struct diff_options *options,
+int commit_patch_id(const struct commit *commit, struct diff_options *options,
 		    struct object_id *oid, int);
 int init_patch_ids(struct repository *, struct patch_ids *);
 int free_patch_ids(struct patch_ids *);
-- 
2.42.0.424.gceadf0f3

[RFC PATCH 2/2] pretty: add 'I' placeholder for patch-id

[Michael McClimon]

This doesn't actually work yet, and the test is probably in the wrong
place, but I think it's sort of close enough to send an RFC patch to ask
some questions.

Signed-off-by: Michael McClimon <michael@mcclimon.org>
---
 pretty.c                      | 11 +++++++++++
 t/t4205-log-pretty-formats.sh |  7 +++++++
 2 files changed, 18 insertions(+)

diff --git a/pretty.c b/pretty.c
index cf964b06..47e2e6e9 100644
--- a/pretty.c
+++ b/pretty.c
@@ -19,6 +19,7 @@
 #include "trailer.h"
 #include "run-command.h"
 #include "object-name.h"
+#include "patch-ids.h"
 
 /*
  * The limit for formatting directives, which enable the caller to append
@@ -1571,6 +1572,16 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 						 c->pretty_ctx->abbrev);
 		}
 		return 1;
+	case 'I':
+		{
+			struct diff_options diffopt;
+			struct object_id patch_id;
+			repo_diff_setup(the_repository, &diffopt);
+			if (commit_patch_id(commit, &diffopt, &patch_id, 0))
+				die(_("cannot get patch id"));
+			strbuf_addstr(sb, oid_to_hex(&patch_id));
+			return 1;
+		}
 	case 'm':		/* left/right/bottom */
 		strbuf_addstr(sb, get_revision_mark(NULL, commit));
 		return 1;
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index e3d655e6..1e9fdcfe 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -38,6 +38,13 @@ test_expect_success 'set up basic repos' '
 	git config --unset i18n.commitEncoding
 '
 
+# %I placeholder
+test_expect_success '%I placeholder is a patch-id' '
+	git diff-tree --patch-with-raw HEAD | git patch-id >expected &&
+	git show -s --pretty="%I %H" >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'alias builtin format' '
 	git log --pretty=oneline >expected &&
 	git config pretty.test-alias oneline &&
-- 
2.42.0.424.gceadf0f3

Re: [RFC PATCH 0/2] pretty: add %I formatting for patch-id

[Jeff King]

On Sat, Oct 21, 2023 at 10:27:58PM -0400, Michael McClimon wrote:

> I would like to have a single-command way to get the patch id for a
> commit: the thing you'd see in a pipeline like
>     git diff-tree --patch-with-raw HEAD | git patch-id

So I actually don't think this is the worst thing in the world. You are
using two plumbing commands as intended. And if you want to get N
patch-ids, you can do it with a constant number of processes like:

  git rev-list HEAD | git diff-tree --stdin -p | git patch-id

Alternatively, I wonder if patch-id could generate the ids internally
(which we certainly have code for; that's how git-cherry, etc, work).
Would it make sense to teach patch-id:

  1. To accept just commit id's on stdin to produce the patches for
     those commit id's against their parents, without receiving the
     patch text itself on stdin. I guess --commits-only or something.

  2. To accept commit-ishes on the command line, overriding the need to
     read from stdin.

Which would let your single-patch case become just:

  git patch-id --commits-only HEAD

But let's read on...

> My initial thought was to add a --patch-id flag to git diff-tree, but
> then I thought that maybe better would be to add a pretty specifier to
> do so, so that (for instance) you could generate patch-ids for
> everything in a branch by saying something like
>     git log --pretty='%I %H' start..

I can see how "%I" would be convenient, especially from a scripting
standpoint, since you can control how it appears in the output. But it
does feel a little funny to me as a commit pretty-printing item.
Computing it requires calculating the diff, which we'll of course want
to do separately for the diff portion of many invocations. E.g., I
didn't check, but I'd guess that:

  git log --pretty="%I %H" -p

would compute each diff twice. Would it make more sense as a
diff-format alongside --raw, --name-status, and so on?

I have a more subtle reason to think this might be a good idea, which
I'll discuss below.

> The thing that is perplexing to me is that it _does_ appear to work on
> some commits, for example (where 8b3aa36f is a recent-ish commit from
> master, chosen at random):
> [...]
> But for other commits, like the one in the test here, it does not.

Hmm. When I run the failing test in a debugger, I see that going into
commit_patch_id(), there is already an entry in diff_queued_diff (which
is a global that stores the result of the last diff). So I think there
is already a diff there from the call to diff_tree_oid() in
log_tree_diff(). And then you diff _again_ and end up with duplicates
when computing the patch-id.

(This causes another bug, too; at the end of diff_flush_patch_id we
clear the queue, so the actual diff is not showed by "git show" anymore;
the use of "-s" in the test obscures that).

The thing that most surprises me is that it ever works at all. ;) But I
think what happens is this:

  - in the code in your patch, you fail to set up the diff_options
    correctly. In particular, you do not set the "recursive" flag.

  - so in the case of 8b3aa36f, which touches only a file in
    Documentation/, the call from log_tree_diff() queues the correct
    file modification entry. The second call from commit_patch_id()
    mistakenly queues only the parent tree "Documentation"

  - a patch-id is defined on the textual patch changes, so it ignores
    non-file changes that are queued (and in Git you wouldn't normally
    see these anyway unless you use a non-recursive diff or a special
    option like "-t").

And hence any patch which consists of changes to files only in subtrees
will "work", and other ones fail. To "fix" your patch, you could set
diffopt.flags.recursive, as we do in format-patch's prepare_bases()
call. But of course because of the duplicate diff, that just means we'll
consistently get the wrong answer now. ;)

So I'd expect something like this to work:

diff --git a/pretty.c b/pretty.c
index 47e2e6e99a..5a473c7941 100644
--- a/pretty.c
+++ b/pretty.c
@@ -20,6 +20,7 @@
 #include "run-command.h"
 #include "object-name.h"
 #include "patch-ids.h"
+#include "diffcore.h"
 
 /*
  * The limit for formatting directives, which enable the caller to append
@@ -1576,10 +1577,28 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		{
 			struct diff_options diffopt;
 			struct object_id patch_id;
+
+			/*
+			 * save the existing diff queue, as it may be
+			 * populated, e.g. by log_tree_diff(). Of course
+			 * it would be nice to use its results, but:
+			 *
+			 *   1. We can't count on it (we might be formatting a
+			 *      via some other code path).
+			 *   2. We have no clue which diff options were used.
+			 */
+			struct diff_queue_struct old_queue = diff_queued_diff;
+			DIFF_QUEUE_CLEAR(&diff_queued_diff);
+
 			repo_diff_setup(the_repository, &diffopt);
+			diffopt.flags.recursive = 1;
+			diff_setup_done(&diffopt);
+
 			if (commit_patch_id(commit, &diffopt, &patch_id, 0))
 				die(_("cannot get patch id"));
 			strbuf_addstr(sb, oid_to_hex(&patch_id));
+
+			diff_queued_diff = old_queue;
 			return 1;
 		}
 	case 'm':		/* left/right/bottom */

but it doesn't quite fix your test. There I think the issue is that
the patch-id code is a bit wonky for the special case of the creation of
an empty file (even before your patch). The internal patch-id generator
adds fake:

  --- /dev/null
  +++ b/whatever

lines that would normally appear in the patch for a newly created file.
But for an empty file, the "--patch" format does not emit those lines at
all! So we need this hackery on top:

diff --git a/diff.c b/diff.c
index 2c602df10a..9370e585f1 100644
--- a/diff.c
+++ b/diff.c
@@ -6431,15 +6431,15 @@ static int diff_get_patch_id(struct diff_options *options, struct object_id *oid
 			the_hash_algo->update_fn(&ctx, oid_to_hex(&p->two->oid),
 					the_hash_algo->hexsz);
 		} else {
-			if (p->one->mode == 0) {
+			if (p->one->mode == 0 && p->two->size) {
 				patch_id_add_string(&ctx, "---/dev/null");
 				patch_id_add_string(&ctx, "+++b/");
 				the_hash_algo->update_fn(&ctx, p->two->path, len2);
-			} else if (p->two->mode == 0) {
+			} else if (p->two->mode == 0 && p->one->size) {
 				patch_id_add_string(&ctx, "---a/");
 				the_hash_algo->update_fn(&ctx, p->one->path, len1);
 				patch_id_add_string(&ctx, "+++/dev/null");
-			} else {
+			} else if (p->one->size || p->two->size) {
 				patch_id_add_string(&ctx, "---a/");
 				the_hash_algo->update_fn(&ctx, p->one->path, len1);
 				patch_id_add_string(&ctx, "+++b/");

And with that, your new test succeeds.

> have done a bit of investigation, but I would not really call myself a C
> programmer and I'm not super familiar with the codebase, so I'm a bit
> stuck. I thought maybe at first I wasn't initializing the diff_options
> correctly, but I suspect the problem is actually more fundamental than
> that.

So as you can see, the diff options do matter. And that is a more
fundamental issue.  In many cases patch-id will be pretty stable and
independent of options, but I suspect it would get confused by things
like rename detection.

If we run "git log --format=%I", which diff options should be used for
the patch-id? A stable set (like we do for "format-patch --base")? Or
ones that match the diff we are otherwise generating? I could see
arguments both ways.

One nice thing about "diff-tree | patch-id" is that you are in control
of the options that diff-tree uses. But I'd think in most cases you just
want a stable patch-id, so the "git patch-id HEAD" I suggested earlier
would presumably use some very basic set of diff options.

I dunno. I do not use patch-id much (aside from internal use via things
like "git cherry", etc). So there may be use cases or corner cases I'm
not thinking about. But I do think there's a bit of complexity to think
about here.

-Peff