[v3,09/10] pretty: implement 'reference' format
diff mbox series

Message ID 470a2b0f4fd450af1d9c9d27ec0f0c91ea59117f.1573764280.git.liu.denton@gmail.com
State New
Headers show
Series
  • learn the "reference" pretty format
Related show

Commit Message

Denton Liu Nov. 14, 2019, 8:47 p.m. UTC
The standard format for referencing other commits within some projects
(such as git.git) is the reference format. This is described in
Documentation/SubmittingPatches as

	If you want to reference a previous commit in the history of a stable
	branch, use the format "abbreviated hash (subject, date)", like this:

	....
		Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30)
		noticed that ...
	....

Since this format is so commonly used, standardize it as a pretty
format.

The tests that are implemented essentially show that the format-string
does not change in response to various log options. This is useful
because, for future developers, it shows that we've considered the
limitations of the "canned format-string" approach and we are fine with
them.

Based-on-a-patch-by: SZEDER Gábor <szeder.dev@gmail.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/pretty-formats.txt       | 10 +++++++
 Documentation/pretty-options.txt       |  2 +-
 Documentation/rev-list-options.txt     |  4 ++-
 contrib/completion/git-completion.bash |  2 +-
 pretty.c                               |  4 ++-
 t/t4205-log-pretty-formats.sh          | 36 ++++++++++++++++++++++++++
 6 files changed, 54 insertions(+), 4 deletions(-)

Comments

Todd Zullinger Nov. 15, 2019, 3:33 a.m. UTC | #1
Denton Liu wrote:
> index 90ff9e2bea..91edeaf6d5 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -269,7 +269,7 @@ list.
>  	exclude (that is, '{caret}commit', 'commit1..commit2',
>  	and 'commit1\...commit2' notations cannot be used).
>  +
> -With `--pretty` format other than `oneline` (for obvious reasons),
> +With `--pretty` format other than `oneline` and `reference` (for obvious reasons),
>  this causes the output to have two extra lines of information
>  taken from the reflog.  The reflog designator in the output may be shown
>  as `ref@{Nth}` (where `Nth` is the reverse-chronological index in the
> @@ -293,6 +293,8 @@ Under `--pretty=oneline`, the commit message is
>  prefixed with this information on the same line.
>  This option cannot be combined with `--reverse`.
>  See also linkgit:git-reflog[1].
> ++
> +Under `--pretty=summary`, this information will not be shown at all.

I think the line above wants s/summary/reference.
Junio C Hamano Nov. 15, 2019, 6:07 a.m. UTC | #2
Denton Liu <liu.denton@gmail.com> writes:

> +* 'reference'
> +
> +	  <abbrev hash> (<title line>, <short author date>)

s/title line/title/ as you definitely do *not* want a line with a
title on it (and nothing else) in this context.

> +This format is useful for referring to other commits when writing a new
> +commit message. It uses the following canned user format:
> +`%C(auto)%h (%s, %as)`. This means it will not respect options that
> +change the output format such as `--date` `--no-abbrev-commit`, and
> +`--walk-reflogs`.

Ignoring of the '--date' may want to be eventually fixed, but for an
initial cut, using %as in the canned format is a good approach for
expediency.

I thought that %h can be told to give arbitrary long hash with
--abbrev=<n>, up to n==40 (or the number of bytes the hash of your
choice has), so I am not sure if `--no-abbrev-commit` is worth
mentioning.  I do not think I understand what you wanted to do by
using `--walk-reflogs` with the format at all, either.

Thanks.
Eric Sunshine Nov. 15, 2019, 8:07 a.m. UTC | #3
On Thu, Nov 14, 2019 at 3:47 PM Denton Liu <liu.denton@gmail.com> wrote:
> [...]
> Since this format is so commonly used, standardize it as a pretty
> format.
> [...]
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> @@ -63,6 +63,16 @@ This is designed to be as compact as possible.
> +This format is useful for referring to other commits when writing a new
> +commit message. It uses the following canned user format:
> +`%C(auto)%h (%s, %as)`. This means it will not respect options that
> +change the output format such as `--date` `--no-abbrev-commit`, and
> +`--walk-reflogs`.

Missing comma between `--date` and `--no-abbrev-commit`.
SZEDER Gábor Nov. 15, 2019, 1:18 p.m. UTC | #4
On Fri, Nov 15, 2019 at 03:07:59PM +0900, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > +* 'reference'
> > +
> > +	  <abbrev hash> (<title line>, <short author date>)
> 
> s/title line/title/ as you definitely do *not* want a line with a
> title on it (and nothing else) in this context.

Well, we just followed suit of the descriptions of other pretty
formats, and they all say "<title line>".

On a related note, the description of the '%s' format specifier in the
same document is "subject", not "title".  Perhaps they should be made
consistent, but I'm not sure.  I like that '%s' means "subject",
because the first letter matches, but some man pages (commit,
format-patch, am) make difference between a subject and a title.
Junio C Hamano Nov. 16, 2019, 1:46 a.m. UTC | #5
SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Fri, Nov 15, 2019 at 03:07:59PM +0900, Junio C Hamano wrote:
>> Denton Liu <liu.denton@gmail.com> writes:
>> 
>> > +* 'reference'
>> > +
>> > +	  <abbrev hash> (<title line>, <short author date>)
>> 
>> s/title line/title/ as you definitely do *not* want a line with a
>> title on it (and nothing else) in this context.
>
> Well, we just followed suit of the descriptions of other pretty
> formats, and they all say "<title line>".
>
> On a related note, the description of the '%s' format specifier in the
> same document is "subject", not "title".  Perhaps they should be made
> consistent, but I'm not sure.  I like that '%s' means "subject",
> because the first letter matches, but some man pages (commit,
> format-patch, am) make difference between a subject and a title.

Yeah, I think these are used more or less interchangeably, and we
may want to clarify the distinction.  Between the two, <title> is a
more appropriate "context neutral" name for the thing, i.e. one line
(technically, one paragraph) summary of what the commit is about.
Where <subject> becomes more relevant is when a commit is expressed
as a piece of patch e-mail, where <title> is used on the "Subject: "
header.

Thanks.
Junio C Hamano Nov. 18, 2019, 1:42 a.m. UTC | #6
Junio C Hamano <gitster@pobox.com> writes:

> Denton Liu <liu.denton@gmail.com> writes:
>
>> +* 'reference'
>> +
>> +	  <abbrev hash> (<title line>, <short author date>)
>
> s/title line/title/ as you definitely do *not* want a line with a
> title on it (and nothing else) in this context.
>
>> +This format is useful for referring to other commits when writing a new
>> +commit message. It uses the following canned user format:
>> +`%C(auto)%h (%s, %as)`. This means it will not respect options that
>> +change the output format such as `--date` `--no-abbrev-commit`, and
>> +`--walk-reflogs`.
>
> Ignoring of the '--date' may want to be eventually fixed, but for an
> initial cut, using %as in the canned format is a good approach for
> expediency.
> ...
> ...  I do not think I understand what you wanted to do by
> using `--walk-reflogs` with the format at all, either.

OK, I re-read the patch text and I understand what you wanted to
say.  The mention of --walk-reflogs in the new description is
grossly misleading.

   $ git log --pretty=ref --walk-reflogs @{now}

would work perfectly fine.  It is an instruction that tells "git
log" not to follow the commit ancestry from the commit that sits at
the current branch right now, but instead follow the history of what
commits used to sit at the tip of the current branch.

"--pretty=format:<anything>" overrides only the side effect of
"--walk-reflogs" that modifies traditional built-in formats.  But
the above makes it sound as if use of "--pretty=ref" somehow makes
our commands to ignore the entire effect of any option that has side
effect of changing the output format, which is not true.

The `--date` and `--decorate` options would be a good example of
options "that change the output format".  If you want to mention the
effect of this option on the `--walk-reflogs` option correctly, the
explanation must make sure that only the output aspect is affected.

Perhaps like this

	This format is used to refer another commit in a commit
	message as "<short hash> (<title>, <author date>)" and is
	the same as `--pretty=format:%C(auto)%h (%s, %as)`.  As with
	any `format:` with format placeholders, its output is not
	affected by other options like `--decorate` and
	`--walk-reflogs`.

I guess?

We would want to fix it in such a way that it uses %ad instead of
%as, but internally tweak the default date format to short unless
the --date option is given.

Thanks.
Junio C Hamano Nov. 18, 2019, 1:44 a.m. UTC | #7
Junio C Hamano <gitster@pobox.com> writes:

> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> On Fri, Nov 15, 2019 at 03:07:59PM +0900, Junio C Hamano wrote:
>>> Denton Liu <liu.denton@gmail.com> writes:
>>> 
>>> > +* 'reference'
>>> > +
>>> > +	  <abbrev hash> (<title line>, <short author date>)
>>> 
>>> s/title line/title/ as you definitely do *not* want a line with a
>>> title on it (and nothing else) in this context.
>>
>> Well, we just followed suit of the descriptions of other pretty
>> formats, and they all say "<title line>".

Sorry I failed to respond to this point.  I think it is OK to leave
it <title line> in this series, then.  Updating them need to be done
for all at once together with other formats.

Thanks.

Patch
diff mbox series

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 11979301ff..ee1a945bae 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -63,6 +63,16 @@  This is designed to be as compact as possible.
 
 	       <full commit message>
 
+* 'reference'
+
+	  <abbrev hash> (<title line>, <short author date>)
++
+This format is useful for referring to other commits when writing a new
+commit message. It uses the following canned user format:
+`%C(auto)%h (%s, %as)`. This means it will not respect options that
+change the output format such as `--date` `--no-abbrev-commit`, and
+`--walk-reflogs`.
+
 * 'email'
 
 	  From <hash> <date>
diff --git a/Documentation/pretty-options.txt b/Documentation/pretty-options.txt
index e44fc8f738..a59426eefd 100644
--- a/Documentation/pretty-options.txt
+++ b/Documentation/pretty-options.txt
@@ -3,7 +3,7 @@ 
 
 	Pretty-print the contents of the commit logs in a given format,
 	where '<format>' can be one of 'oneline', 'short', 'medium',
-	'full', 'fuller', 'email', 'raw', 'format:<string>'
+	'full', 'fuller', 'reference', 'email', 'raw', 'format:<string>'
 	and 'tformat:<string>'.  When '<format>' is none of the above,
 	and has '%placeholder' in it, it acts as if
 	'--pretty=tformat:<format>' were given.
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 90ff9e2bea..91edeaf6d5 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -269,7 +269,7 @@  list.
 	exclude (that is, '{caret}commit', 'commit1..commit2',
 	and 'commit1\...commit2' notations cannot be used).
 +
-With `--pretty` format other than `oneline` (for obvious reasons),
+With `--pretty` format other than `oneline` and `reference` (for obvious reasons),
 this causes the output to have two extra lines of information
 taken from the reflog.  The reflog designator in the output may be shown
 as `ref@{Nth}` (where `Nth` is the reverse-chronological index in the
@@ -293,6 +293,8 @@  Under `--pretty=oneline`, the commit message is
 prefixed with this information on the same line.
 This option cannot be combined with `--reverse`.
 See also linkgit:git-reflog[1].
++
+Under `--pretty=summary`, this information will not be shown at all.
 
 --merge::
 	After a failed merge, show refs that touch files having a
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 557d0373c3..007e6a06d6 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1749,7 +1749,7 @@  __git_log_shortlog_options="
 	--all-match --invert-grep
 "
 
-__git_log_pretty_formats="oneline short medium full fuller email raw format: tformat: mboxrd"
+__git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
 __git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default raw unix format:"
 
 _git_log ()
diff --git a/pretty.c b/pretty.c
index 4d7f5e9aab..88a3bc621d 100644
--- a/pretty.c
+++ b/pretty.c
@@ -97,7 +97,9 @@  static void setup_commit_formats(void)
 		{ "mboxrd",	CMIT_FMT_MBOXRD,	0,	0 },
 		{ "fuller",	CMIT_FMT_FULLER,	0,	8 },
 		{ "full",	CMIT_FMT_FULL,		0,	8 },
-		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 }
+		{ "oneline",	CMIT_FMT_ONELINE,	1,	0 },
+		{ "reference",	CMIT_FMT_USERFORMAT,	1,	0,
+			0, "%C(auto)%h (%s, %as)" },
 		/*
 		 * Please update $__git_log_pretty_formats in
 		 * git-completion.bash when you add new formats.
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index da9cacffea..9a9a18f104 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -824,4 +824,40 @@  test_expect_success '%S in git log --format works with other placeholders (part
 	test_cmp expect actual
 '
 
+test_expect_success 'log --pretty=reference' '
+	git log --pretty="tformat:%h (%s, %as)" >expect &&
+	git log --pretty=reference >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --pretty=reference always uses short date' '
+	git log --pretty="tformat:%h (%s, %as)" >expect &&
+	git log --date=rfc --pretty=reference >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --pretty=reference is never unabbreviated' '
+	git log --pretty="tformat:%h (%s, %as)" >expect &&
+	git log --no-abbrev-commit --pretty=reference >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --pretty=reference is never decorated' '
+	git log --pretty="tformat:%h (%s, %as)" >expect &&
+	git log --decorate=short --pretty=reference >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --pretty=reference does not output reflog info' '
+	git log --walk-reflogs --pretty="tformat:%h (%s, %as)" >expect &&
+	git log --walk-reflogs --pretty=reference >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --pretty=reference is colored appropriately' '
+	git log --color=always --pretty="tformat:%C(auto)%h (%s, %as)" >expect &&
+	git log --color=always --pretty=reference >actual &&
+	test_cmp expect actual
+'
+
 test_done