Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Apr 26, 2010, 03:11 UTC
- Message-ID
- <20100426031012.GA29953@progeny.tock>
- In-Reply-To
- <1272232579-18895-3-git-send-email-wmpalmer@gmail.com>
Will Palmer wrote:
Show 15 quoted lines
> Here we make "git log --pretty=%H --abbrev-commit" synonymous with > "git log --pretty=%h", and make %h/abbreviated-%H respect the length > specified for --abbrev. > > The same is applied to other commit-placeholders %P and %p, and > --abbrev is respected for %t, though %T is not changed. > > Signed-off-by: Will Palmer <wmpalmer@gmail.com> > --- > builtin/rev-list.c | 1 + > builtin/shortlog.c | 2 ++ > commit.h | 1 + > log-tree.c | 2 ++ > pretty.c | 30 +++++++++++++++++++----------- > 5 files changed, 25 insertions(+), 11 deletions(-)
I agree that this is the right to do, since this is how the built-in formats work (the ‘commit ...’ line follows the semantics of your %H, and ‘Merge: ...’ line your %p, for example).
Documentation and tests?
Show 12 quoted lines
> diff --git a/builtin/rev-list.c b/builtin/rev-list.c
> index 5a53862..1d1e59c 100644
> --- a/builtin/rev-list.c
> +++ b/builtin/rev-list.c
> @@ -98,6 +98,7 @@ static void show_commit(struct commit *commit, void *data)
> struct strbuf buf = STRBUF_INIT;
> struct pretty_print_context ctx = {0};
> ctx.abbrev = revs->abbrev;
> + ctx.abbrev_commit = revs->abbrev_commit;
> ctx.date_mode = revs->date_mode;
> ctx.use_color = DIFF_OPT_TST(&revs->diffopt, COLOR_DIFF);
> pretty_print_commit(revs->commit_format, commit, &buf, &ctx);Makes sense.
Show 13 quoted lines
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 7aee491..5c0721c 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -143,6 +143,8 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit, struct rev
> struct strbuf ufbuf = STRBUF_INIT;
> struct pretty_print_context ctx = {0};
>
> + ctx.abbrev = rev->abbrev;
> + ctx.abbrev_commit = rev->abbrev_commit;
> ctx.use_color = DIFF_OPT_TST(&rev->diffopt, COLOR_DIFF);
> pretty_print_commit(CMIT_FMT_RAW, commit, &buf, &ctx);
> buffer = buf.buf;Shortlog doesn’t print commit hashes, does it?
Show 12 quoted lines
> diff --git a/commit.h b/commit.h > index b6caf91..7a476a0 100644 > --- a/commit.h > +++ b/commit.h > @@ -72,6 +72,7 @@ struct pretty_print_context > int need_8bit_cte; > int show_notes; > int use_color; > + int abbrev_commit; > struct reflog_walk_info *reflog_info; > }; >
nitpick: I’d stick this up with abbrev and maybe add a comment to explain their distinct uses.
Show 13 quoted lines
> diff --git a/log-tree.c b/log-tree.c
> index 6bb4748..0a2309c 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -282,6 +282,8 @@ void show_log(struct rev_info *opt)
> int abbrev_commit = opt->abbrev_commit ? opt->abbrev : 40;
> const char *extra_headers = opt->extra_headers;
> struct pretty_print_context ctx = {0};
> + ctx.abbrev = opt->abbrev;
> + ctx.abbrev_commit = opt->abbrev_commit;
> ctx.use_color = DIFF_OPT_TST(&opt->diffopt, COLOR_DIFF);
>
> opt->loginfo = NULL;There is a
ctx.abbrev = opt->diffopt.abbrev;
later in the same function; how do these interact?
> @@ -741,24 +744,29 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
[...]
> strbuf_addstr(sb, find_unique_abbrev( > - p->item->object.sha1, DEFAULT_ABBREV)); > + p->item->object.sha1, > + c->pretty_ctx->abbrev));
nitpick: the new indentation makes these look like parameters to strbuf_addstr.
Here’s an alternative implementation of the more controversial half of your patch, for your amusement. The big downside is that it requires one to specify --abbrev-commit before the --format option.
Thanks for the pleasant read.
Jonathan
diff --git a/pretty.c b/pretty.c index 7cb3a2a..1008a41 100644 --- a/pretty.c +++ b/pretty.c @@ -12,10 +12,31 @@ static char *user_format; +static void abbreviate_commit_hashes(char *fmt) +{ + char *p; + for (p = fmt; p != NULL; p = strchr(p + 1, '%')) { + p++; + switch (*p) { + case 'H': + *p = 'h'; + break; + case 'P': + *p = 'p'; + break; + case 'T': + default: + break; + } + } +} + static void save_user_format(struct rev_info *rev, const char *cp, int is_tformat) { free(user_format); user_format = xstrdup(cp); + if (rev->abbrev_commit) + abbreviate_commit_hashes(user_format); if (is_tformat) rev->use_terminator = 1; rev->commit_format = CMIT_FMT_USERFORMAT;