Re: [PATCH] log: diagnose empty HEAD more clearly

[Jeff King <peff@peff.net>]

On Wed, Jun 03, 2015 at 10:24:02AM -0700, Junio C Hamano wrote:

> Which is what led me to say "Why are we defaulting to HEAD before
> checking if it even exists?  Isn't that the root cause of this
> confusion?  What happens if we stopped doing it?"
> 
> And I think the "diagnose after finding that pending is empty and we
> were given 'def'" approach almost gives us the equivalent, which is
> why I said "Okay" above.
> 
> If we can make it possible to tell if that "def" was given by the
> user (i.e. --default parameter) or by the machinery (e.g. "git log"
> and friends), then we can remove "almost" from the above sentence.

My thought was that we would not stop dying (for compatibility), and
that if we keep dying, it is effectively the same as what I proposed
(perhaps slightly worse, in the sense that we do not diagnose "--default
foo" as thoroughly, but effectively the same in that basically nobody
uses "--default" in the first place).

But if you are OK to eventually stop dying, I think this line of
reasoning is OK.

> diff --git a/builtin/log.c b/builtin/log.c
> index 4c4e6be..3b568a1 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -148,6 +148,9 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,
>  		rev->diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT;
>  	argc = setup_revisions(argc, argv, rev, opt);
>  
> +	if (!rev->pending.nr && !opt->def)
> +		die("you do not have a commit yet on your branch");

Do we want to mention the name of the branch here? I guess it does not
really matter. Perhaps "the current branch" would be better than "your
branch", though. Maybe:

  fatal: you do not have any commits yet on the current branch

This message hopefully goes away in the long run, but we'll have it for
a while.

> +static void default_to_head_if_exists(struct setup_revision_opt *opt)
> +{
> +	unsigned char unused[20];
> +
> +	if (resolve_ref_unsafe("HEAD", RESOLVE_REF_READING, unused, NULL))
> +		opt->def = "HEAD";
> +}

This approach looks sane to me. Want to wrap it up with a commit
message and a test?

-Peff