threads / patch / 50065

patchlog: add %S option (like --source) to log --format

Subject: [PATCH] log: add %S option (like --source) to log --format

## tl;dr

7 messages between Dec 19, 2018 and Dec 31, 2018. Diffs are folded; open one to read it.

replies: 6people: 4as markdown or json

issac.trotts@gmail.com· Dec 19, 2018, 08:33 UTC · lore
From: Issac Trotts <issac.trotts@gmail.com>
Make it possible to write for example
        git log --format="%H,%S"

where the %S at the end is a new placeholder that prints out the ref (tag/branch) for each commit.

Using %d might seem like an alternative but it only shows the ref for the last commit in the branch.

Signed-off-by: Issac Trotts <issactrotts@google.com>
---
This change is based on a question from Stack Overflow:
https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
---
 Documentation/pretty-formats.txt |  2 ++
 builtin/log.c                    |  2 +-
 log-tree.c                       |  1 +
 pretty.c                         | 15 ++++++++++
 pretty.h                         |  1 +
 t/t4205-log-pretty-formats.sh    | 50 ++++++++++++++++++++++++++++++++
 6 files changed, 70 insertions(+), 1 deletion(-)
Show changes to 6 files +70 −1

Documentation/pretty-formats.txt, builtin/log.c, log-tree.c, pretty.c, pretty.h, t/t4205-log-pretty-formats.sh

diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index 417b638cd..de6953108 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -134,6 +134,8 @@ The placeholders are:
 - '%cI': committer date, strict ISO 8601 format
 - '%d': ref names, like the --decorate option of linkgit:git-log[1]
 - '%D': ref names without the " (", ")" wrapping.
+- '%S': ref name given on the command line by which the commit was reached
+  (like `git log --source`), only works with `git log`
 - '%e': encoding
 - '%s': subject
 - '%f': sanitized subject line, suitable for a filename
diff --git a/builtin/log.c b/builtin/log.c
index e8e51068b..be3025657 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
 	    rev->diffopt.filter || rev->diffopt.flags.follow_renames)
 		rev->always_show_header = 0;
 
-	if (source) {
+	if (source || (rev->pretty_given && (rev->commit_format == CMIT_FMT_USERFORMAT) && w.source)) {
 		init_revision_sources(&revision_sources);
 		rev->sources = &revision_sources;
 	}
diff --git a/log-tree.c b/log-tree.c
index 10680c139..3cb14256e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -700,6 +700,7 @@ void show_log(struct rev_info *opt)
 	ctx.color = opt->diffopt.use_color;
 	ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
 	ctx.output_encoding = get_log_output_encoding();
+	ctx.rev = opt;
 	if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
 		ctx.from_ident = &opt->from_ident;
 	if (opt->graph)
diff --git a/pretty.c b/pretty.c
index b83a3ecd2..258e86e9c 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1084,6 +1084,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 	struct commit_list *p;
 	const char *arg;
 	int ch;
+	char **slot;
 
 	/* these are independent of the commit */
 	switch (placeholder[0]) {
@@ -1194,6 +1195,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
 		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
 		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
 		return 1;
+	case 'S':		/* tag/branch like --source */
+		if (c->pretty_ctx->rev->sources == NULL) {
+			return 0;
+		}
+		slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
+		if (slot && *slot) {
+			strbuf_addstr(sb, *slot);
+			return 1;
+		} else {
+			die(_("failed to get info for %%S"));
+		}
 	case 'g':		/* reflog info */
 		switch(placeholder[1]) {
 		case 'd':	/* reflog selector */
@@ -1498,6 +1510,9 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
 	case 'N':
 		w->notes = 1;
 		break;
+	case 'S':
+		w->source = 1;
+		break;
 	}
 	return 0;
 }
diff --git a/pretty.h b/pretty.h
index 7359d318a..87ca5dfcb 100644
--- a/pretty.h
+++ b/pretty.h
@@ -60,6 +60,7 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
 
 struct userformat_want {
 	unsigned notes:1;
+	unsigned source:1;
 };
 
 /* Set the flag "w->notes" if there is placeholder %N in "fmt". */
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 978a8a66f..7df8c3d4e 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -621,4 +621,54 @@ test_expect_success 'trailer parsing not fooled by --- line' '
 	test_cmp expect actual
 '
 
+test_expect_success 'set up %S tests' '
+	git checkout --orphan source-a &&
+	test_commit one &&
+	test_commit two &&
+	git checkout -b source-b HEAD^ &&
+	test_commit three
+'
+
+test_expect_success 'log --format=%S paints branch names' '
+	cat >expect <<-\EOF &&
+	source-b
+	source-a
+	source-b
+	EOF
+	git log --format=%S source-a source-b >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --format=%S paints tag names' '
+	git tag -m tagged source-tag &&
+	cat >expect <<-\EOF &&
+	source-tag
+	source-a
+	source-tag
+	EOF
+	git log --format=%S source-tag source-a >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --format=%S paints symmetric ranges' '
+	cat >expect <<-\EOF &&
+	source-b
+	source-a
+	EOF
+	git log --format=%S source-a...source-b >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '%S in git log --format works with other placeholders (part 1)' '
+	git log --format="source-b %h" source-b >expect &&
+	git log --format="%S %h" source-b >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '%S in git log --format works with other placeholders (part 2)' '
+	git log --format="%h source-b" source-b >expect &&
+	git log --format="%h %S" source-b >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.19.1
Issac Trotts· Dec 21, 2018, 05:24 UTC · re: issac.trotts@gmail.com · lore

Re: [PATCH] log: add %S option (like --source) to log --format

Hi all, friendly ping. Is there still interest in merging this patch?

Thanks, Issac

On Wed, Dec 19, 2018 at 12:33 AM <issac.trotts@gmail.com> wrote:
Show 180 quoted lines
>
> From: Issac Trotts <issac.trotts@gmail.com>
>
> Make it possible to write for example
>
>         git log --format="%H,%S"
>
> where the %S at the end is a new placeholder that prints out the ref
> (tag/branch) for each commit.
>
> Using %d might seem like an alternative but it only shows the ref for the last
> commit in the branch.
>
> Signed-off-by: Issac Trotts <issactrotts@google.com>
>
> ---
>
> This change is based on a question from Stack Overflow:
> https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
> ---
>  Documentation/pretty-formats.txt |  2 ++
>  builtin/log.c                    |  2 +-
>  log-tree.c                       |  1 +
>  pretty.c                         | 15 ++++++++++
>  pretty.h                         |  1 +
>  t/t4205-log-pretty-formats.sh    | 50 ++++++++++++++++++++++++++++++++
>  6 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 417b638cd..de6953108 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -134,6 +134,8 @@ The placeholders are:
>  - '%cI': committer date, strict ISO 8601 format
>  - '%d': ref names, like the --decorate option of linkgit:git-log[1]
>  - '%D': ref names without the " (", ")" wrapping.
> +- '%S': ref name given on the command line by which the commit was reached
> +  (like `git log --source`), only works with `git log`
>  - '%e': encoding
>  - '%s': subject
>  - '%f': sanitized subject line, suitable for a filename
> diff --git a/builtin/log.c b/builtin/log.c
> index e8e51068b..be3025657 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>             rev->diffopt.filter || rev->diffopt.flags.follow_renames)
>                 rev->always_show_header = 0;
>
> -       if (source) {
> +       if (source || (rev->pretty_given && (rev->commit_format == CMIT_FMT_USERFORMAT) && w.source)) {
>                 init_revision_sources(&revision_sources);
>                 rev->sources = &revision_sources;
>         }
> diff --git a/log-tree.c b/log-tree.c
> index 10680c139..3cb14256e 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -700,6 +700,7 @@ void show_log(struct rev_info *opt)
>         ctx.color = opt->diffopt.use_color;
>         ctx.expand_tabs_in_log = opt->expand_tabs_in_log;
>         ctx.output_encoding = get_log_output_encoding();
> +       ctx.rev = opt;
>         if (opt->from_ident.mail_begin && opt->from_ident.name_begin)
>                 ctx.from_ident = &opt->from_ident;
>         if (opt->graph)
> diff --git a/pretty.c b/pretty.c
> index b83a3ecd2..258e86e9c 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1084,6 +1084,7 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>         struct commit_list *p;
>         const char *arg;
>         int ch;
> +       char **slot;
>
>         /* these are independent of the commit */
>         switch (placeholder[0]) {
> @@ -1194,6 +1195,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>                 load_ref_decorations(NULL, DECORATE_SHORT_REFS);
>                 format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
>                 return 1;
> +       case 'S':               /* tag/branch like --source */
> +               if (c->pretty_ctx->rev->sources == NULL) {
> +                       return 0;
> +               }
> +               slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
> +               if (slot && *slot) {
> +                       strbuf_addstr(sb, *slot);
> +                       return 1;
> +               } else {
> +                       die(_("failed to get info for %%S"));
> +               }
>         case 'g':               /* reflog info */
>                 switch(placeholder[1]) {
>                 case 'd':       /* reflog selector */
> @@ -1498,6 +1510,9 @@ static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
>         case 'N':
>                 w->notes = 1;
>                 break;
> +       case 'S':
> +               w->source = 1;
> +               break;
>         }
>         return 0;
>  }
> diff --git a/pretty.h b/pretty.h
> index 7359d318a..87ca5dfcb 100644
> --- a/pretty.h
> +++ b/pretty.h
> @@ -60,6 +60,7 @@ static inline int cmit_fmt_is_mail(enum cmit_fmt fmt)
>
>  struct userformat_want {
>         unsigned notes:1;
> +       unsigned source:1;
>  };
>
>  /* Set the flag "w->notes" if there is placeholder %N in "fmt". */
> diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
> index 978a8a66f..7df8c3d4e 100755
> --- a/t/t4205-log-pretty-formats.sh
> +++ b/t/t4205-log-pretty-formats.sh
> @@ -621,4 +621,54 @@ test_expect_success 'trailer parsing not fooled by --- line' '
>         test_cmp expect actual
>  '
>
> +test_expect_success 'set up %S tests' '
> +       git checkout --orphan source-a &&
> +       test_commit one &&
> +       test_commit two &&
> +       git checkout -b source-b HEAD^ &&
> +       test_commit three
> +'
> +
> +test_expect_success 'log --format=%S paints branch names' '
> +       cat >expect <<-\EOF &&
> +       source-b
> +       source-a
> +       source-b
> +       EOF
> +       git log --format=%S source-a source-b >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'log --format=%S paints tag names' '
> +       git tag -m tagged source-tag &&
> +       cat >expect <<-\EOF &&
> +       source-tag
> +       source-a
> +       source-tag
> +       EOF
> +       git log --format=%S source-tag source-a >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success 'log --format=%S paints symmetric ranges' '
> +       cat >expect <<-\EOF &&
> +       source-b
> +       source-a
> +       EOF
> +       git log --format=%S source-a...source-b >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '%S in git log --format works with other placeholders (part 1)' '
> +       git log --format="source-b %h" source-b >expect &&
> +       git log --format="%S %h" source-b >actual &&
> +       test_cmp expect actual
> +'
> +
> +test_expect_success '%S in git log --format works with other placeholders (part 2)' '
> +       git log --format="%h source-b" source-b >expect &&
> +       git log --format="%h %S" source-b >actual &&
> +       test_cmp expect actual
> +'
> +
>  test_done
> --
> 2.19.1
>
Jeff King· Dec 22, 2018, 22:22 UTC · re: Issac Trotts · lore

Re: [PATCH] log: add %S option (like --source) to log --format

On Thu, Dec 20, 2018 at 09:24:10PM -0800, Issac Trotts wrote:
> Hi all, friendly ping. Is there still interest in merging this patch?

Yes, but I think people are largely absent for holidays at this point. If there hasn't been any movement on it, I'll try to review after Jan 5th (it also wouldn't hurt to re-post around then).

-Peff
Issac Trotts· Dec 25, 2018, 02:12 UTC · re: Jeff King · lore

Re: [PATCH] log: add %S option (like --source) to log --format

Got it, sounds good.
On Sat, Dec 22, 2018 at 2:22 PM Jeff King <peff@peff.net> wrote:
Show 10 quoted lines
>
> On Thu, Dec 20, 2018 at 09:24:10PM -0800, Issac Trotts wrote:
>
> > Hi all, friendly ping. Is there still interest in merging this patch?
>
> Yes, but I think people are largely absent for holidays at this point.
> If there hasn't been any movement on it, I'll try to review after Jan
> 5th (it also wouldn't hurt to re-post around then).
>
> -Peff
Derrick Stolee· Dec 27, 2018, 13:20 UTC · re: issac.trotts@gmail.com · lore

Re: [PATCH] log: add %S option (like --source) to log --format

On 12/19/2018 3:33 AM, issac.trotts@gmail.com wrote:
Show 37 quoted lines
> From: Issac Trotts <issac.trotts@gmail.com>
>
> Make it possible to write for example
>
>          git log --format="%H,%S"
>
> where the %S at the end is a new placeholder that prints out the ref
> (tag/branch) for each commit.
>
> Using %d might seem like an alternative but it only shows the ref for the last
> commit in the branch.
>
> Signed-off-by: Issac Trotts <issactrotts@google.com>
>
> ---
>
> This change is based on a question from Stack Overflow:
> https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
> ---
>   Documentation/pretty-formats.txt |  2 ++
>   builtin/log.c                    |  2 +-
>   log-tree.c                       |  1 +
>   pretty.c                         | 15 ++++++++++
>   pretty.h                         |  1 +
>   t/t4205-log-pretty-formats.sh    | 50 ++++++++++++++++++++++++++++++++
>   6 files changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> index 417b638cd..de6953108 100644
> --- a/Documentation/pretty-formats.txt
> +++ b/Documentation/pretty-formats.txt
> @@ -134,6 +134,8 @@ The placeholders are:
>   - '%cI': committer date, strict ISO 8601 format
>   - '%d': ref names, like the --decorate option of linkgit:git-log[1]
>   - '%D': ref names without the " (", ")" wrapping.
> +- '%S': ref name given on the command line by which the commit was reached
> +  (like `git log --source`), only works with `git log`

This "only works with `git log`" made me think about what would happen with `git rev-list --pretty=format:"%h %S"` and the answer (on my machine) was a segfault.

Show 16 quoted lines
>   - '%e': encoding
>   - '%s': subject
>   - '%f': sanitized subject line, suitable for a filename
> diff --git a/builtin/log.c b/builtin/log.c
> index e8e51068b..be3025657 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>   	    rev->diffopt.filter || rev->diffopt.flags.follow_renames)
>   		rev->always_show_header = 0;
>   
> -	if (source) {
> +	if (source || (rev->pretty_given && (rev->commit_format == CMIT_FMT_USERFORMAT) && w.source)) {
>   		init_revision_sources(&revision_sources);
>   		rev->sources = &revision_sources;
>   	}

Likely, you'll want to duplicate this initialization in the revision machinery. Keep this one in builtin/log.c as it was before, but add something like this initialization to setup_revisions(). Add a test for rev-list.

[snip]
Show 6 quoted lines
> @@ -1194,6 +1195,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
>   		load_ref_decorations(NULL, DECORATE_SHORT_REFS);
>   		format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
>   		return 1;
> +	case 'S':		/* tag/branch like --source */
> +		if (c->pretty_ctx->rev->sources == NULL) {
Use "if (!c->pretty_ctx->rev->sources".
> +			return 0;
> +		}
> +		slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
> +		if (slot && *slot) {

I'm not sure this check for 'slot' being non-null is necessary, as we would already get a failure in the commit-slab code (for revision_sources_at()) if the slab is not initialized.

> +			strbuf_addstr(sb, *slot);
> +			return 1;
> +		} else {
> +			die(_("failed to get info for %%S"));

Here, you die() when you fail to get a slot but above you return 0 when the sources are not initialized.

I don't see another use of die() in this method. Is that the right way to handle failure here? (I'm legitimately asking because I have over-used 'die()' in the past and am still unclear on when it is appropriate.)

Show 53 quoted lines
>   '
>   
> +test_expect_success 'set up %S tests' '
> +	git checkout --orphan source-a &&
> +	test_commit one &&
> +	test_commit two &&
> +	git checkout -b source-b HEAD^ &&
> +	test_commit three
> +'
> +
> +test_expect_success 'log --format=%S paints branch names' '
> +	cat >expect <<-\EOF &&
> +	source-b
> +	source-a
> +	source-b
> +	EOF
> +	git log --format=%S source-a source-b >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --format=%S paints tag names' '
> +	git tag -m tagged source-tag &&
> +	cat >expect <<-\EOF &&
> +	source-tag
> +	source-a
> +	source-tag
> +	EOF
> +	git log --format=%S source-tag source-a >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --format=%S paints symmetric ranges' '
> +	cat >expect <<-\EOF &&
> +	source-b
> +	source-a
> +	EOF
> +	git log --format=%S source-a...source-b >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '%S in git log --format works with other placeholders (part 1)' '
> +	git log --format="source-b %h" source-b >expect &&
> +	git log --format="%S %h" source-b >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success '%S in git log --format works with other placeholders (part 2)' '
> +	git log --format="%h source-b" source-b >expect &&
> +	git log --format="%h %S" source-b >actual &&
> +	test_cmp expect actual
> +'
> +
>   test_done
Please also add a simple test to t6006-rev-list-format.sh.
Thanks,
-Stolee
Junio C Hamano· Dec 28, 2018, 18:14 UTC · re: Derrick Stolee · lore

Re: [PATCH] log: add %S option (like --source) to log --format

Derrick Stolee <stolee@gmail.com> writes:
Show 11 quoted lines
>> +++ b/Documentation/pretty-formats.txt
>> @@ -134,6 +134,8 @@ The placeholders are:
>>   - '%cI': committer date, strict ISO 8601 format
>>   - '%d': ref names, like the --decorate option of linkgit:git-log[1]
>>   - '%D': ref names without the " (", ")" wrapping.
>> +- '%S': ref name given on the command line by which the commit was reached
>> +  (like `git log --source`), only works with `git log`
>
> This "only works with `git log`" made me think about what would happen
> with `git rev-list --pretty=format:"%h %S"` and the answer (on my
> machine) was a segfault.
That's a bad one X-<.
Show 17 quoted lines
>> +		slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
>> +		if (slot && *slot) {
> I'm not sure this check for 'slot' being non-null is necessary, as we
> would already get a failure in the commit-slab code (for
> revision_sources_at()) if the slab is not initialized.
>> +			strbuf_addstr(sb, *slot);
>> +			return 1;
>> +		} else {
>> +			die(_("failed to get info for %%S"));
>
> Here, you die() when you fail to get a slot but above you return 0
> when the sources are not initialized.
>
> I don't see another use of die() in this method. Is that the right way
> to handle failure here? (I'm legitimately asking because I have
> over-used 'die()' in the past and am still unclear on when it is
> appropriate.)

This is definitely a bad one, too. If '%d' cannot find decoration, it would not "die".

Thanks.
Issac Trotts· Dec 31, 2018, 04:35 UTC · re: Derrick Stolee · lore

Re: [PATCH] log: add %S option (like --source) to log --format

Hi Derrick, thanks for your feedback.
I'll send out a new patch with the changes.
On Fri, Dec 28, 2018 at 12:20 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 43 quoted lines
>
> On 12/19/2018 3:33 AM, issac.trotts@gmail.com wrote:
> > From: Issac Trotts <issac.trotts@gmail.com>
> >
> > Make it possible to write for example
> >
> >          git log --format="%H,%S"
> >
> > where the %S at the end is a new placeholder that prints out the ref
> > (tag/branch) for each commit.
> >
> > Using %d might seem like an alternative but it only shows the ref for the last
> > commit in the branch.
> >
> > Signed-off-by: Issac Trotts <issactrotts@google.com>
> >
> > ---
> >
> > This change is based on a question from Stack Overflow:
> > https://stackoverflow.com/questions/12712775/git-get-source-information-in-format
> > ---
> >   Documentation/pretty-formats.txt |  2 ++
> >   builtin/log.c                    |  2 +-
> >   log-tree.c                       |  1 +
> >   pretty.c                         | 15 ++++++++++
> >   pretty.h                         |  1 +
> >   t/t4205-log-pretty-formats.sh    | 50 ++++++++++++++++++++++++++++++++
> >   6 files changed, 70 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
> > index 417b638cd..de6953108 100644
> > --- a/Documentation/pretty-formats.txt
> > +++ b/Documentation/pretty-formats.txt
> > @@ -134,6 +134,8 @@ The placeholders are:
> >   - '%cI': committer date, strict ISO 8601 format
> >   - '%d': ref names, like the --decorate option of linkgit:git-log[1]
> >   - '%D': ref names without the " (", ")" wrapping.
> > +- '%S': ref name given on the command line by which the commit was reached
> > +  (like `git log --source`), only works with `git log`
>
> This "only works with `git log`" made me think about what would happen
> with `git rev-list --pretty=format:"%h %S"` and the answer (on my
> machine) was a segfault.
Good find. Checking for c->pretty_ctx->rev == NULL prevents the seg fault.
Show 21 quoted lines
> >   - '%e': encoding
> >   - '%s': subject
> >   - '%f': sanitized subject line, suitable for a filename
> > diff --git a/builtin/log.c b/builtin/log.c
> > index e8e51068b..be3025657 100644
> > --- a/builtin/log.c
> > +++ b/builtin/log.c
> > @@ -203,7 +203,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
> >           rev->diffopt.filter || rev->diffopt.flags.follow_renames)
> >               rev->always_show_header = 0;
> >
> > -     if (source) {
> > +     if (source || (rev->pretty_given && (rev->commit_format == CMIT_FMT_USERFORMAT) && w.source)) {
> >               init_revision_sources(&revision_sources);
> >               rev->sources = &revision_sources;
> >       }
>
> Likely, you'll want to duplicate this initialization in the revision
> machinery. Keep this one in builtin/log.c as it was before, but add
> something like this initialization to setup_revisions(). Add a test for
> rev-list.

Okay, I added a test to check that rev-list leaves %S alone and implicitly that it doesn't segfault.

I'd like to limit the scope of this change to what's already here, with the check preventing the segfault, if that's okay. Could support for %S in rev-list be in a later patch?

Show 29 quoted lines
>
> [snip]
>
> > @@ -1194,6 +1195,17 @@ static size_t format_commit_one(struct strbuf *sb, /* in UTF-8 */
> >               load_ref_decorations(NULL, DECORATE_SHORT_REFS);
> >               format_decorations_extended(sb, commit, c->auto_color, "", ", ", "");
> >               return 1;
> > +     case 'S':               /* tag/branch like --source */
> > +             if (c->pretty_ctx->rev->sources == NULL) {
> Use "if (!c->pretty_ctx->rev->sources".
> > +                     return 0;
> > +             }
> > +             slot = revision_sources_at(c->pretty_ctx->rev->sources, commit);
> > +             if (slot && *slot) {
> I'm not sure this check for 'slot' being non-null is necessary, as we
> would already get a failure in the commit-slab code (for
> revision_sources_at()) if the slab is not initialized.
> > +                     strbuf_addstr(sb, *slot);
> > +                     return 1;
> > +             } else {
> > +                     die(_("failed to get info for %%S"));
>
> Here, you die() when you fail to get a slot but above you return 0 when
> the sources are not initialized.
>
> I don't see another use of die() in this method. Is that the right way
> to handle failure here? (I'm legitimately asking because I have
> over-used 'die()' in the past and am still unclear on when it is
> appropriate.)
Fixed.
Show 56 quoted lines
>
> >   '
> >
> > +test_expect_success 'set up %S tests' '
> > +     git checkout --orphan source-a &&
> > +     test_commit one &&
> > +     test_commit two &&
> > +     git checkout -b source-b HEAD^ &&
> > +     test_commit three
> > +'
> > +
> > +test_expect_success 'log --format=%S paints branch names' '
> > +     cat >expect <<-\EOF &&
> > +     source-b
> > +     source-a
> > +     source-b
> > +     EOF
> > +     git log --format=%S source-a source-b >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'log --format=%S paints tag names' '
> > +     git tag -m tagged source-tag &&
> > +     cat >expect <<-\EOF &&
> > +     source-tag
> > +     source-a
> > +     source-tag
> > +     EOF
> > +     git log --format=%S source-tag source-a >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success 'log --format=%S paints symmetric ranges' '
> > +     cat >expect <<-\EOF &&
> > +     source-b
> > +     source-a
> > +     EOF
> > +     git log --format=%S source-a...source-b >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success '%S in git log --format works with other placeholders (part 1)' '
> > +     git log --format="source-b %h" source-b >expect &&
> > +     git log --format="%S %h" source-b >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> > +test_expect_success '%S in git log --format works with other placeholders (part 2)' '
> > +     git log --format="%h source-b" source-b >expect &&
> > +     git log --format="%h %S" source-b >actual &&
> > +     test_cmp expect actual
> > +'
> > +
> >   test_done
>
> Please also add a simple test to t6006-rev-list-format.sh.
Done.
Show 5 quoted lines
>
> Thanks,
>
> -Stolee
>

← back to recent threads