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

From: Jeff King <peff@peff.net>
Date: Feb 2, 2018, 07:15 UTC
Message-ID: <20180202071518.GB16533@sigill.intra.peff.net>
In-Reply-To: <20180201205721.30087-1-t.gummerer@gmail.com>

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

Show 13 quoted lines

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

Show 5 quoted lines

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

Show 10 quoted lines

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