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@xxxxxxxxx> wrote: > > On 12/19/2018 3:33 AM, issac.trotts@xxxxxxxxx wrote: > > From: Issac Trotts <issac.trotts@xxxxxxxxx> > > > > 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@xxxxxxxxxx> > > > > --- > > > > 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. > > - '%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? > > [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. > > > ' > > > > +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. > > Thanks, > > -Stolee >
