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

From: Jeff King <peff@peff.net>
Date: Jun 4, 2015, 08:34 UTC
Message-ID: <20150604083430.GB5771@peff.net>
In-Reply-To: <xmqqpp5cyabx.fsf@gitster.dls.corp.google.com>

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

Show 11 quoted lines

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

Show 10 quoted lines

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

Show 7 quoted lines

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