Re: [RFC/PATCH] reset --hard: make use of the pretty machinery

[Jeff King <peff@peff.net>]

On Thu, Feb 01, 2018 at 08:57:21PM +0000, Thomas Gummerer wrote:

> It is a slight change of the output if the second line of the commit
> message is not a blank line, i.e. if the commit message is
> 
>     foo
>     bar
> 
> previously we would print "HEAD is now at 000000 foo", while after
> this change we print "HEAD is now at 000000 foo bar", same as 'git log
> --oneline' shows "000000 foo bar".
> 
> So this does make the output more consistent with other commands, and
> 'reset' is a porcelain command, so nobody should be parsing the output
> in scripts.

Yeah, I'd say it's definitely fine to change the (already translated!)
stderr output of git-reset. I agree that it's an improvement to be
consistent with other commands here.

> The current behaviour dates back to 0e5a7faa3a ("Make "git reset" a
> builtin.", 2007-09-11), so I assume (without digging into the old
> codebase too much) that the logic was implemented because there was
> no convenience function such as 'pp_commit_easy' that would do this
> already.

Yes, there used to be quite a lot of ad hoc parsing of commit objects,
but these days we have safer and more readable alternatives.  Even
without the visible behavior change, I think it would be worth doing
this patch just as a code cleanup.

> +	struct strbuf buf = STRBUF_INIT;
> +
> +	printf(_("HEAD is now at %s"),
> +		find_unique_abbrev(commit->object.oid.hash, DEFAULT_ABBREV));
> +
> +	pp_commit_easy(CMIT_FMT_ONELINE, commit, &buf);
> +	if (buf.len > 0)
> +		printf(" %s", buf.buf);
> +	putchar('\n');
> +	strbuf_release(&buf);

I was disappointed you still had to call find_unique_abbrev(). It seems
like we ought to be able to do this in a single formatting call. But
CMIT_FMT_ONELINE is just about the subject, and doesn't include the hash
at all. There's no equivalent without turning to the user-format code.

You can do:

  struct pretty_print_context ctx = {0, DEFAULT_ABBREV};
  format_commit_message(commit, _("HEAD is now at %H %s"), &buf, &ctx);
  puts(buf.buf);

but I was annoyed at having to say "DEFAULT_ABBREV". It seems like that
ought to be the default.

Anyway, I'm fine with your patch as-is. I just needed a little
formatting code-golf in my day.

-Peff