git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Issac Trotts <issac.trotts@gmail.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: git@vger.kernel.org, Noemi Mercado <noemi@sourcegraph.com>,
	Issac Trotts <issactrotts@google.com>
Subject: Re: [PATCH] log: add %S option (like --source) to log --format
Date: Mon, 31 Dec 2018 15:35:02 +1100	[thread overview]
Message-ID: <CANdyxMz8RsCUmumyNP=0Nxh8TpoC2z5TGRW8Hpf_Z_MH7wAj4Q@mail.gmail.com> (raw)
In-Reply-To: <5dfd92d1-2e87-3006-1630-a33794b6066b@gmail.com>

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:
>
> 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.

> >   - '%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
>

  parent reply	other threads:[~2018-12-31  4:35 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-19  8:33 [PATCH] log: add %S option (like --source) to log --format issac.trotts
2018-12-21  5:24 ` Issac Trotts
2018-12-22 22:22   ` Jeff King
2018-12-25  2:12     ` Issac Trotts
2018-12-27 13:20 ` Derrick Stolee
2018-12-28 18:14   ` Junio C Hamano
2018-12-31  4:35   ` Issac Trotts [this message]
  -- strict thread matches above, loose matches on Subject: below --
2019-01-11  6:30 issac.trotts
2019-01-08 13:20 issac.trotts
2019-01-08 22:21 ` Junio C Hamano
2019-01-11  6:28   ` Issac Trotts
2019-01-11 17:37     ` Junio C Hamano
2019-01-11 17:47     ` Junio C Hamano
2018-12-31 21:48 issac.trotts
2018-12-31  4:53 issac.trotts
2019-01-02 19:33 ` Junio C Hamano
2019-01-08 13:14   ` Issac Trotts
2018-12-17  6:25 Issac Trotts
2018-12-17  9:31 ` Junio C Hamano
2018-12-18  3:35   ` Issac Trotts
2018-12-17 15:59 ` Jeff King
2018-12-18 17:14   ` Issac Trotts
2018-12-19  3:47     ` Issac Trotts
2018-12-19  8:07       ` Issac Trotts
2018-12-19 17:09         ` Issac Trotts

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CANdyxMz8RsCUmumyNP=0Nxh8TpoC2z5TGRW8Hpf_Z_MH7wAj4Q@mail.gmail.com' \
    --to=issac.trotts@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=issactrotts@google.com \
    --cc=noemi@sourcegraph.com \
    --cc=stolee@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).