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

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

[brian m. carlson]

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(-)

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

[brian m. carlson]

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(+)

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

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

[brian m. carlson]

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(-)

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

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

[brian m. carlson]

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
+'
+
 test_done

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

[Torsten Bögershausen]

On 13.12.15 18:27, brian m. carlson wrote:
> 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

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> 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

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

[Jeff King]

On Sun, Dec 13, 2015 at 05:27:15PM +0000, brian m. carlson wrote:

> 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

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

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> +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?

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

[Junio C Hamano]

"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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On Mon, Dec 14, 2015 at 04:32:39PM -0500, Jeff King wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]