Re: [PATCH v2 2/3] pretty: make %H/%h dependent on --abbrev[-commit]

[Jonathan Nieder <jrnieder@gmail.com>]

Will Palmer wrote:

> 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?

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

> 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?

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

> 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;