threads / patch / 40985

v2, 3 partsformat-patch: introduce option to suppress commit hashes

Subject: [PATCH v2 0/3] format-patch: introduce option to suppress commit hashes

## tl;dr

10 messages between Dec 13, 2015 and Dec 15, 2015. Diffs are folded; open one to read it.

replies: 9people: 4as markdown or json

brian m. carlson· Dec 13, 2015, 17:27 UTC · lore

git format-patch is often used to create patches that are then stored in version control or displayed with diff. Having the commit hash in the "From " line usually just creates diff noise in these cases, so this series introduces --zero-commit to set that to all zeros.

Changes from v1:
* Rename the option --zero-commit.
* Improve the tests to look for a 40-hex hash value in "From " header.
brian m. carlson (3):
  Introduce a null_oid constant.
  format-patch: add an option to suppress commit hash
  format-patch: check that header line has expected format
 Documentation/git-format-patch.txt |  4 ++++
 builtin/log.c                      |  5 +++++
 cache.h                            |  1 +
 log-tree.c                         |  3 ++-
 revision.h                         |  1 +
 sha1_file.c                        |  1 +
 t/t4014-format-patch.sh            | 12 ++++++++++++
 7 files changed, 26 insertions(+), 1 deletion(-)
brian m. carlson· Dec 13, 2015, 17:27 UTC · re: brian m. carlson · lore

[PATCH v2 1/3] Introduce a null_oid constant.

null_oid is the struct object_id equivalent to null_sha1.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h     | 1 +
 sha1_file.c | 1 +
 2 files changed, 2 insertions(+)
Show changes to 2 files +2 −0

cache.h, sha1_file.c

diff --git a/cache.h b/cache.h
index 5ab6cb50..c63fcc11 100644
--- a/cache.h
+++ b/cache.h
@@ -831,6 +831,7 @@ extern const char *find_unique_abbrev(const unsigned char *sha1, int len);
 extern int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len);
 
 extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
+extern const struct object_id null_oid;
 
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 {
diff --git a/sha1_file.c b/sha1_file.c
index 27ce7b70..a54deb05 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -36,6 +36,7 @@
 static inline uintmax_t sz_fmt(size_t s) { return s; }
 
 const unsigned char null_sha1[20];
+const struct object_id null_oid;
 
 /*
  * This is meant to hold a *small* number of objects that you would
brian m. carlson· Dec 13, 2015, 17:27 UTC · re: brian m. carlson · lore

[PATCH v2 2/3] format-patch: add an option to suppress commit hash

Oftentimes, patches created by git format-patch will be stored in version control or compared with diff. In these cases, two otherwise identical patches can have different commit hashes, leading to diff noise. Teach git format-patch a --zero-commit option that instead produces an all-zero hash to avoid this diff noise.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-format-patch.txt | 4 ++++
 builtin/log.c                      | 5 +++++
 log-tree.c                         | 3 ++-
 revision.h                         | 1 +
 t/t4014-format-patch.sh            | 6 ++++++
 5 files changed, 18 insertions(+), 1 deletion(-)
Show changes to 5 files +18 −1

Documentation/git-format-patch.txt, builtin/log.c, log-tree.c, revision.h, t/t4014-format-patch.sh

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 40356491..e3cdaeb9 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -256,6 +256,10 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
 	using this option cannot be applied properly, but they are
 	still useful for code review.
 
+--zero-commit::
+  Output an all-zero hash in each patch's From header instead
+  of the hash of the commit.
+
 --root::
 	Treat the revision argument as a <revision range>, even if it
 	is just a single commit (that would normally be treated as a
diff --git a/builtin/log.c b/builtin/log.c
index 069bd3a9..e00cea75 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1196,6 +1196,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int cover_letter = -1;
 	int boundary_count = 0;
 	int no_binary_diff = 0;
+	int zero_commit = 0;
 	struct commit *origin = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
@@ -1236,6 +1237,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_NOARG | PARSE_OPT_NONEG, keep_callback },
 		OPT_BOOL(0, "no-binary", &no_binary_diff,
 			 N_("don't output binary diffs")),
+		OPT_BOOL(0, "zero-commit", &zero_commit,
+			 N_("output all-zero hash in From header")),
 		OPT_BOOL(0, "ignore-if-in-upstream", &ignore_if_in_upstream,
 			 N_("don't include a patch matching a commit upstream")),
 		{ OPTION_SET_INT, 'p', "no-stat", &use_patch_format, NULL,
@@ -1380,6 +1383,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	/* Always generate a patch */
 	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
 
+	rev.zero_commit = zero_commit;
+
 	if (!DIFF_OPT_TST(&rev.diffopt, TEXT) && !no_binary_diff)
 		DIFF_OPT_SET(&rev.diffopt, BINARY);
 
diff --git a/log-tree.c b/log-tree.c
index 35e78017..f70a30e1 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -342,7 +342,8 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 {
 	const char *subject = NULL;
 	const char *extra_headers = opt->extra_headers;
-	const char *name = oid_to_hex(&commit->object.oid);
+	const char *name = oid_to_hex(opt->zero_commit ?
+				      &null_oid : &commit->object.oid);
 
 	*need_8bit_cte_p = 0; /* unknown */
 	if (opt->total > 0) {
diff --git a/revision.h b/revision.h
index 5bc96868..23857c0e 100644
--- a/revision.h
+++ b/revision.h
@@ -135,6 +135,7 @@ struct rev_info {
 			pretty_given:1,
 			abbrev_commit:1,
 			abbrev_commit_given:1,
+			zero_commit:1,
 			use_terminator:1,
 			missing_newline:1,
 			date_mode_explicit:1,
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 890db117..b740e3da 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1431,4 +1431,10 @@ test_expect_success 'cover letter auto user override' '
 	test_line_count = 2 list
 '
 
+test_expect_success 'format-patch --zero-commit' '
+	git format-patch --zero-commit --stdout v2..v1 >patch2 &&
+	cnt=$(egrep "^From 0{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
+	test $cnt = 3
+'
+
 test_done
Junio C Hamano· Dec 14, 2015, 21:43 UTC · re: brian m. carlson · lore

Re: [PATCH v2 2/3] format-patch: add an option to suppress commit hash

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 5 quoted lines
> +test_expect_success 'format-patch --zero-commit' '
> +	git format-patch --zero-commit --stdout v2..v1 >patch2 &&
> +	cnt=$(egrep "^From 0{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
> +	test $cnt = 3
> +'

This test is not wrong per-se, but it makes the test as a whole somewhat brittle. People cannot add new commits in the history being tested, which would add to the number of patches, without adjusting this test, even though all this test cares about is that all the mbox "From " lines record the zeroed commit object name.

    git format-patch --zero-commit --stdout v2..v1 |
    grep "^From " | sort | uniq >actual &&
    echo "From $_z40 Mon Sep 17 00:00:00 2001" >expect &&
    test_cmp expect actual
or something like that instead?
brian m. carlson· Dec 13, 2015, 17:27 UTC · re: brian m. carlson · lore

[PATCH v2 3/3] format-patch: check that header line has expected format

The format of the "From " header line is very specific to allow utilities to detect Git-style patches. Add a test that the patches created are in the expected format.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t4014-format-patch.sh | 6 ++++++
 1 file changed, 6 insertions(+)
Show changes to t/t4014-format-patch.sh +6 −0
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index b740e3da..362bc228 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1437,4 +1437,10 @@ test_expect_success 'format-patch --zero-commit' '
 	test $cnt = 3
 '
 
+test_expect_success 'From line has expected format' '
+	git format-patch --stdout v2..v1 >patch2 &&
+	cnt=$(egrep "^From [0-9a-f]{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
+	test $cnt = 3
+'
+
 test_done
Torsten Bögershausen· Dec 14, 2015, 07:16 UTC · re: brian m. carlson · lore

Re: [PATCH v2 3/3] format-patch: check that header line has expected format

On 13.12.15 18:27, brian m. carlson wrote:
Show 21 quoted lines
> The format of the "From " header line is very specific to allow
> utilities to detect Git-style patches.  Add a test that the patches
> created are in the expected format.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  t/t4014-format-patch.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index b740e3da..362bc228 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1437,4 +1437,10 @@ test_expect_success 'format-patch --zero-commit' '
>  	test $cnt = 3
>  '
>  
> +test_expect_success 'From line has expected format' '
> +	git format-patch --stdout v2..v1 >patch2 &&
> +	cnt=$(egrep "^From [0-9a-f]{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
> +	test $cnt = 3

For these kind of things: test_line_count() is your friend

Junio C Hamano· Dec 14, 2015, 19:11 UTC · re: brian m. carlson · lore

Re: [PATCH v2 3/3] format-patch: check that header line has expected format

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 20 quoted lines
> The format of the "From " header line is very specific to allow
> utilities to detect Git-style patches.  Add a test that the patches
> created are in the expected format.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  t/t4014-format-patch.sh | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index b740e3da..362bc228 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1437,4 +1437,10 @@ test_expect_success 'format-patch --zero-commit' '
>  	test $cnt = 3
>  '
>  
> +test_expect_success 'From line has expected format' '
> +	git format-patch --stdout v2..v1 >patch2 &&
> +	cnt=$(egrep "^From [0-9a-f]{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
Don't you want to anchor the pattern to the right as well?
> +	test $cnt = 3
> +'
> +
>  test_done
Junio C Hamano· Dec 14, 2015, 21:45 UTC · re: brian m. carlson · lore

Re: [PATCH v2 3/3] format-patch: check that header line has expected format

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
> +test_expect_success 'From line has expected format' '
> +	git format-patch --stdout v2..v1 >patch2 &&
> +	cnt=$(egrep "^From [0-9a-f]{40} Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
Also, with $_x40, you do not need egrep.
> +	test $cnt = 3
> +'
> +
>  test_done
Jeff King· Dec 14, 2015, 21:32 UTC · re: brian m. carlson · lore

Re: [PATCH v2 0/3] format-patch: introduce option to suppress commit hashes

On Sun, Dec 13, 2015 at 05:27:15PM +0000, brian m. carlson wrote:
Show 13 quoted lines
> git format-patch is often used to create patches that are then stored in
> version control or displayed with diff.  Having the commit hash in the
> "From " line usually just creates diff noise in these cases, so this
> series introduces --zero-commit to set that to all zeros.
> 
> Changes from v1:
> * Rename the option --zero-commit.
> * Improve the tests to look for a 40-hex hash value in "From " header.
> 
> brian m. carlson (3):
>   Introduce a null_oid constant.
>   format-patch: add an option to suppress commit hash
>   format-patch: check that header line has expected format

The intent here makes sense to me, and with the exception of the test_line_count thing that Torsten mentioned, the code looks good.

I briefly wondered if the option should simply be "--diffable" or something like that, and trigger this new behavior as well as implying --no-signature. Along with any other relevant options (if any; I don't recall if --stat-width is terminal-dependent for format-patch, for example).

But that is probably overkill. People can flip those switches individually if they want to (and even if somebody did want "--diffable", it may make sense to build it on top, so they can flip the zero-commit thing individually if they want).

-Peff
brian m. carlson· Dec 15, 2015, 00:35 UTC · re: Jeff King · lore

Re: [PATCH v2 0/3] format-patch: introduce option to suppress commit hashes

On Mon, Dec 14, 2015 at 04:32:39PM -0500, Jeff King wrote:
Show 13 quoted lines
> The intent here makes sense to me, and with the exception of the
> test_line_count thing that Torsten mentioned, the code looks good.
> 
> I briefly wondered if the option should simply be "--diffable" or
> something like that, and trigger this new behavior as well as implying
> --no-signature. Along with any other relevant options (if any; I don't
> recall if --stat-width is terminal-dependent for format-patch, for
> example).
> 
> But that is probably overkill. People can flip those switches
> individually if they want to (and even if somebody did want
> "--diffable", it may make sense to build it on top, so they can flip the
> zero-commit thing individually if they want).

That does sound like a potentially worthwhile thing to build on top at some point.

I'll reroll with the other suggested changes and a slight tweak to make the tests less dependent on the history in both cases.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187

← back to recent threads