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

[PATCH 0/2] 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 --no-hash to set that to all zeros.

The idea for this series came from Dominic Hargreaves in Debian bug
#635663.

This series is based off of next so as not to conflict with the
conversion of struct object to struct object_id.

Careful readers will note that this option is referred to by both
"no_hash" and "zero_commit".  I felt that the latter made more sense as
an option in struct rev_info, as it's strictly more accurate, while the
former is less accurate but more friendly to humans.  Of course, this
can be changed if reviewers feel it to be undesirable.

brian m. carlson (2):
  Introduce a null_oid constant.
  format-patch: add an option to suppress commit hash

 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            | 6 ++++++
 7 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.6.3.658.g85d7f57

[PATCH 1/2] 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
-- 
2.6.3.658.g85d7f57

[PATCH 2/2] 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 --no-hash 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..1266f135 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.
 
+--no-hash::
+  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..774ec1bf 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 no_hash = 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, "no-hash", &no_hash,
+			 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 = no_hash;
+
 	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..ce8313c7 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 --no-hash' '
+	git format-patch --no-hash --stdout v2..v1 >patch2 &&
+	cnt=$(egrep "^From 0+ Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
+	test $cnt = 3
+'
+
 test_done
-- 
2.6.3.658.g85d7f57

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

[Junio C Hamano]

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

> 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 --no-hash to set that to all zeros.

I am somewhat negative on this change that deliberately loses
information in a way that seems too specific to a single workflow.

I understand that in that particular workflow, the patch stored as
payload in a history needs only the diff part and the commit that
the patch was taken from is deemed irrelevant.

But the reason why that and only that piece of information is
expendable, while author, subject and log message text are not is
because...?  The answer to that question would very much be
project's workflow dependant.  From that point of view, I'd say the
users are much better off without the addition of this feature, but
have a custom script in their workflow to remove parts that their
project and workflow deems unnecessary.  Your project may deem the
source commit object name unnecessary.  Another one may think the
author date and name are, too.  Patch e-mail signature (i.e. what
comes after a line with "-- ") by default depends on the version of
Git that happened to have been used to prepare the patch, which may
not be something you would want.

Stepping back a bit, why is the history from which the patches are
taken from irrelevant in the first place?  Perhaps because you
replayed these patches on top of the same base but did not preserve
their timestamps?  If this user, i.e. the part of the workflow that
commits generated patches to version control, finds the "irrelevant"
change irritating, isn't it fair to expect other users, i.e. other
parts of the same workflow, also find that unnecessary and
irrelevant rebasing irritating?  It feels like I am seeing an
entrance to an X-Y problem whose real solution is to stop doing the
pointless rebases in the first place.

And if that rebase is not pointless, then I am not sure if it is a
good thing to discard the information that records which incarnation
of that constantly rebased source tree the patches were taken from.

So...

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

[brian m. carlson]

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

On Sun, Dec 06, 2015 at 06:49:14PM -0800, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > 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 --no-hash to set that to all zeros.
> 
> I am somewhat negative on this change that deliberately loses
> information in a way that seems too specific to a single workflow.
> 
> I understand that in that particular workflow, the patch stored as
> payload in a history needs only the diff part and the commit that
> the patch was taken from is deemed irrelevant.

Not exactly.  At $DAYJOB, we store pristine-tar files, the contents of
the tarball, and patches against the source in Git.  We do it because
RPM likes patches and doesn't know how to speak to Git, but many Debian
source packages are stored similarly.  Every source change results in a
rebase and reformat of all the patches.

> But the reason why that and only that piece of information is
> expendable, while author, subject and log message text are not is
> because...?  The answer to that question would very much be
> project's workflow dependant.  From that point of view, I'd say the
> users are much better off without the addition of this feature, but
> have a custom script in their workflow to remove parts that their
> project and workflow deems unnecessary.  Your project may deem the
> source commit object name unnecessary.  Another one may think the
> author date and name are, too.  Patch e-mail signature (i.e. what
> comes after a line with "-- ") by default depends on the version of
> Git that happened to have been used to prepare the patch, which may
> not be something you would want.

We do want to know who wrote the patch and the reason behind the patch,
and so pretty much all the information except for the actual hash of the
commit is useful to us.  The hash is not useful because we've just
rebased the commit on a temporary branch.  We do, of course, want
--no-signature, which already exists, as you pointed out.

The hash of the source file isn't generally as much of a problem,
because the patch tends to change, even incidentally (line numbers and
such), when the hash of the file changes.  It's also something that we
have in our history, whereas the temporary branch we rebased in is not.

> Stepping back a bit, why is the history from which the patches are
> taken from irrelevant in the first place?  Perhaps because you
> replayed these patches on top of the same base but did not preserve
> their timestamps?  If this user, i.e. the part of the workflow that
> commits generated patches to version control, finds the "irrelevant"
> change irritating, isn't it fair to expect other users, i.e. other
> parts of the same workflow, also find that unnecessary and
> irrelevant rebasing irritating?  It feels like I am seeing an
> entrance to an X-Y problem whose real solution is to stop doing the
> pointless rebases in the first place.

The history is irrelevant because it happens on a temporary branch which
is not pushed anywhere.  The patches are the canonical form.  As I said,
this workflow is not specific to us; it's also used by certain Debian
maintainers to handle their source packages.

The rebase is required because we're not sure if the patches all apply
to the updated source (e.g. we had Git 2.6.2 and are rebasing them on
2.6.3).  If patch 1 needed changes, but patch 2 did not, then patch 2
shows up in the diff even though we don't want it to.

> And if that rebase is not pointless, then I am not sure if it is a
> good thing to discard the information that records which incarnation
> of that constantly rebased source tree the patches were taken from.

I was looking through the Debian BTS for Git and I saw this feature
request and thought, "Gee, I've always wanted this functionality, but I
never thought to ask for it."  This frequent, throwaway rebasing is very
common in patch-based workflows.
-- 
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: 835 bytes --]

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

[Junio C Hamano]

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

> The hash of the source file isn't generally as much of a problem,
> because the patch tends to change, even incidentally (line numbers and
> such), when the hash of the file changes.  It's also something that we
> have in our history, whereas the temporary branch we rebased in is not.

That is exactly the kind of workflow specific reasoning that tells
you "object name of the commit that the patch was taken from is the
only thing that is undesired" that makes me wonder if the feature is
too workflow specific.  You do something on a temporary branch without
worrying about producing unnecessary object name churn, and end up
wanting not to see object names.

But I can buy that a step in the workflow to rebuild the history on
a temporary branch before going to the next step is a common thing
to have, so let's decide to accept the goal as a good thing to have,
and see how well the patched code implements, documents and tests
the advertised new feature.

Thanks.

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

[Junio C Hamano]

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

> 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 --no-hash 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..1266f135 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.
>  
> +--no-hash::
> +  Output an all-zero hash in each patch's From header instead
> +  of the hash of the commit.
> +

Two (big) problems with the option name.

 - "--no-something" would mislead people to think you are removing
   something, not replacing it with something else.  This option
   does the latter (i.e. the first line of your output still has
   40-hex; it's just it no longer has a useful 40-hex).

 - There are many places we use hexadecimal strings in format-patch
   output and you are not removing or replacing all of them, only
   the commit object name on the fake "From " line.  Saying "hash"
   would mislead readers.

> +test_expect_success 'format-patch --no-hash' '
> +	git format-patch --no-hash --stdout v2..v1 >patch2 &&
> +	cnt=$(egrep "^From 0+ Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&

Don't test "any number of '0'"; test 40 '0's.  This is because the
line format was designed to be usable by things like /etc/magic to
detect format-patch output, and we want to notice if/when we break
that aspect of our output format.

> +	test $cnt = 3
> +'
> +
>  test_done

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> +--no-hash::
>> +  Output an all-zero hash in each patch's From header instead
>> +  of the hash of the commit.
>> +
>
> Two (big) problems with the option name.
>
>  - "--no-something" would mislead people to think you are removing
>    something, not replacing it with something else.  This option
>    does the latter (i.e. the first line of your output still has
>    40-hex; it's just it no longer has a useful 40-hex).
>
>  - There are many places we use hexadecimal strings in format-patch
>    output and you are not removing or replacing all of them, only
>    the commit object name on the fake "From " line.  Saying "hash"
>    would mislead readers.

I am not good at bikeshedding, but you used 'zero_commit' elsewhere
in the code.  I think that would be a much better name--perhaps use
that consistently throughout, as the local variable in options[]
array and end-user facing option name?

>
>> +test_expect_success 'format-patch --no-hash' '
>> +	git format-patch --no-hash --stdout v2..v1 >patch2 &&
>> +	cnt=$(egrep "^From 0+ Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
>
> Don't test "any number of '0'"; test 40 '0's.  This is because the
> line format was designed to be usable by things like /etc/magic to
> detect format-patch output, and we want to notice if/when we break
> that aspect of our output format.
>
>> +	test $cnt = 3
>> +'
>> +
>>  test_done
>
> Thanks.

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

[brian m. carlson]

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

On Mon, Dec 07, 2015 at 11:34:59AM -0800, Junio C Hamano wrote:
> Two (big) problems with the option name.
> 
>  - "--no-something" would mislead people to think you are removing
>    something, not replacing it with something else.  This option
>    does the latter (i.e. the first line of your output still has
>    40-hex; it's just it no longer has a useful 40-hex).

Right.  I originally considered removing that line altogether, but other
parts of Git rely on that header to process patches correctly.

>  - There are many places we use hexadecimal strings in format-patch
>    output and you are not removing or replacing all of them, only
>    the commit object name on the fake "From " line.  Saying "hash"
>    would mislead readers.

I'll reroll with the name --zero-commit, unless somebody comes up with a
better suggestion.

> > +test_expect_success 'format-patch --no-hash' '
> > +	git format-patch --no-hash --stdout v2..v1 >patch2 &&
> > +	cnt=$(egrep "^From 0+ Mon Sep 17 00:00:00 2001" patch2 | wc -l) &&
> 
> Don't test "any number of '0'"; test 40 '0's.  This is because the
> line format was designed to be usable by things like /etc/magic to
> detect format-patch output, and we want to notice if/when we break
> that aspect of our output format.

My idea was to future-proof it against changes in the hash function,
although that's in the distant future.  I'll reroll with the change you
suggested.  I might drop in another patch to improve the existing tests
to cover the case without this option, as I think we just look for
"From " currently.
-- 
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: 835 bytes --]

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

[Junio C Hamano]

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

>> Don't test "any number of '0'"; test 40 '0's.  This is because the
>> line format was designed to be usable by things like /etc/magic to
>> detect format-patch output, and we want to notice if/when we break
>> that aspect of our output format.
>
> My idea was to future-proof it against changes in the hash function,
> although that's in the distant future.

Making sure that the test fails when somebody accidentally lengthens
(or fails to truncate to 40-hex, when you start using a longer hash)
the displayed name there is a good future-proofing.

Thanks.