Re: [PATCH 2/2] name-rev: learn --format=<pretty>

[Junio C Hamano <gitster@pobox.com>]

kristofferhaugsbakk@fastmail.com writes:

> diff --git a/Documentation/git-name-rev.adoc b/Documentation/git-name-rev.adoc
> index d4f1c4d5945..8f050cd4763 100644
> --- a/Documentation/git-name-rev.adoc
> +++ b/Documentation/git-name-rev.adoc
> @@ -9,7 +9,7 @@ git-name-rev - Find symbolic names for given revs
>  SYNOPSIS
>  --------
>  [verse]
> -'git name-rev' [--tags] [--refs=<pattern>]
> +'git name-rev' [--tags] [--refs=<pattern>] [--format=<pretty>]
>  	       ( --all | --annotate-stdin | <commit-ish>... )

We acquired a new option.  Do we need a matching change to
the contents of name_rev_usage[] array?

> +--format=<pretty>::
> +--no-format::
> +	Format revisions instead of outputting symbolic names. The
> +	default is `--no-format`.
> ++
> +Implies `--name-only`.

If it is implication, would 

    git name-rev --format=reference --no-name-only

do what is naturally expected?

> @@ -462,6 +472,25 @@ static const char *get_rev_name(const struct object *o, struct strbuf *buf)
>  	if (o->type != OBJ_COMMIT)
>  		return get_exact_ref_match(o);
>  	c = (const struct commit *) o;
> +
> +	if (format_ctx) {
> +		strbuf_reset(buf);
> +
> +		if (format_ctx->want.notes) {
> +			struct strbuf notebuf = STRBUF_INIT;
> +
> +			format_display_notes(&c->object.oid, &notebuf,
> +					     get_log_output_encoding(),
> +					     format_ctx->ctx.fmt == CMIT_FMT_USERFORMAT);
> +			format_ctx->ctx.notes_message = strbuf_detach(&notebuf, NULL);
> +		}
> +
> +		pretty_print_commit(&format_ctx->ctx, c, buf);
> +		free(format_ctx->ctx.notes_message);

Is free() the expected thing to do here, or FREE_AND_NULL()?  Unlike
callers like log-tree.c:show_log() where a context is prepared, used
once, and then discarded, format_pp is initialized in cmd_name_rev()
once and then repeatedly used by show_name() potentially multiple
times, so there may be a risk of getting confused by this leftover
non-NULL pointer that points at an already free'd piece of memory.

Or there may not be---I did not check, but you as the author must
have already checked, hence this question.

> +		return buf->buf;
> +	}
> +
>  	n = get_commit_rev_name(c);
>  	if (!n)
>  		return NULL;
> @@ -479,6 +508,7 @@ static const char *get_rev_name(const struct object *o, struct strbuf *buf)
>  
>  static void show_name(const struct object *obj,
>  		      const char *caller_name,
> +		      struct pretty_format *format_ctx,
>  		      int always, int allow_undefined, int name_only)
>  {
>  	const char *name;
> @@ -487,7 +517,7 @@ static void show_name(const struct object *obj,
>  
>  	if (!name_only)
>  		printf("%s ", caller_name ? caller_name : oid_to_hex(oid));
> -	name = get_rev_name(obj, &buf);
> +	name = get_rev_name(obj, format_ctx, &buf);
>  	if (name)
>  		printf("%s\n", name);
>  	else if (allow_undefined)
> @@ -507,7 +537,9 @@ static char const * const name_rev_usage[] = {
>  	NULL
>  };
>  
> -static void name_rev_line(char *p, struct name_ref_data *data)
> +static void name_rev_line(char *p,
> +			  struct name_ref_data *data,
> +			  struct pretty_format *format_ctx)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	int counter = 0;
> @@ -532,7 +564,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)
>  				struct object *o =
>  					lookup_object(the_repository, &oid);
>  				if (o)
> -					name = get_rev_name(o, &buf);
> +					name = get_rev_name(o, format_ctx, &buf);
>  			}
>  			*(p+1) = c;
>  
> @@ -567,6 +599,10 @@ int cmd_name_rev(int argc,
>  #endif
>  	int all = 0, annotate_stdin = 0, allow_undefined = 1, always = 0, peel_tag = 0;
>  	struct name_ref_data data = { 0, 0, STRING_LIST_INIT_NODUP, STRING_LIST_INIT_NODUP };
> +	const char *format = NULL;
> +	struct rev_info format_rev = REV_INFO_INIT;
> +	struct pretty_format *format_ctx = NULL;
> +	struct pretty_format format_pp = {0};

Hmph, would we want to use the full init_revisions() instead of
static REV_INFO_INIT that initialises a lot more members of the
struct properly, most importantly the "repo" member that points at
the repostiory to be used?

> +	if (format) {
> +		struct pretty_print_context ctx = {0};
> +		struct userformat_want want = {0};
> +
> +		get_commit_format(format, &format_rev);
> +		ctx.rev = &format_rev;
> +		ctx.fmt = format_rev.commit_format;
> +		ctx.abbrev = format_rev.abbrev;
> +		ctx.date_mode_explicit = format_rev.date_mode_explicit;
> +		ctx.date_mode = format_rev.date_mode;
> +		ctx.color = GIT_COLOR_AUTO;
> +		format_pp.ctx = ctx;

Why does this code initialize and assign to a on-stack ctx first and
then assign it to format_pp.ctx, instead of working on format_pp.ctx
directly?

> +		userformat_find_requirements(format, &want);
> +		if (want.notes)
> +			load_display_notes(NULL);
> +
> +		format_pp.want = want;
> +		format_ctx = &format_pp;
> +
> +		data.name_only = true;
> +	}
> +