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
