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

> Jeff King <peff@peff.net> writes:
> 
>> My concern there would be risk of regression. I.e., that we would take
>> some case which used to error out and turn it into a silent noop. So I'd
>> prefer to keep the behavior the same, and just modify the error code
>> path. The end result is pretty similar to the user, because we obviously
>> cannot produce any actual output either way.
> 
> Okay.
> 
>> Something like:
>>
>> -- >8 --
>> Subject: log: diagnose empty HEAD more clearly
>>
>> If you init or clone an empty repository, the initial
>> message from running "git log" is not very friendly:
>>
>>   $ git init
>>   Initialized empty Git repository in /home/peff/foo/.git/
>>   $ git log
>>   fatal: bad default revision 'HEAD'
>>
>> Let's detect this situation and write a more friendly
>> message:
>>
>>   $ git log
>>   fatal: default revision 'HEAD' points to an unborn branch 'master'
>>
>> We also detect the case that 'HEAD' points to a broken ref;
>> this should be even less common, but is easy to see. Note
>> that we do not diagnose all possible cases. We rely on
>> resolve_ref, which means we do not get information about
>> complex cases. E.g., "--default master" would use dwim_ref
>> to find "refs/heads/master", but we notice only that
>> "master" does not exist. Similarly, a complex sha1
>> expression like "--default HEAD^2" will not resolve as a
>> ref.
>>
>> But that's OK. We fall back to the existing error message in
>> those cases, and they are unlikely to be used anyway.
>> Catching an empty or broken "HEAD" improves the common case,
>> and the other cases are not regressed.
>>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>> I'm not a big fan of the new error message, either, just because the
>> term "unborn" is also likely to be confusing to new users.
>>
>> We can also probably get rid of mentioning "HEAD" completely. It is
>> always the default unless the user has overridden it explicitly.
> 
> I think that still mentioning "HEAD" goes directly against the
> reasoning you made in an earlier part of your message:
> 
>> I think it is one of those cases where the message makes sense when you
>> know how git works. But a new user who does not even know what "HEAD" or
>> a ref actually is may find it a bit confusing. Saying "we can't run
>> git-log, your repository empty!" may seem redundantly obvious even to
>> such a new user. But saying "bad revision 'HEAD'" may leave them saying
>> "eh, what is HEAD and why is it bad?".
> 
> and I agree with that reasoning: the user didn't say anything about
> "HEAD", so why complain about it?
> 
> 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.
> 
>> So we
>> could go with something like:
>>
>>   fatal: your default branch 'master' does not contain any commits
>>
>> or something. I dunno. Bikeshed colors welcome. Adding:
>>
>>   advise("try running 'git commit'");
>>
>> is probably too pedantic. ;)
> 
> ;-)
> 
> I am wondering if the attached is better, if only because it is of
> less impact.  I have die() there to avoid the behaviour change, but
> if we are going to have another future version where we are willing
> to take incompatiblity for better end-user experience like we did at
> 2.0, I suspect turning it to warning() or even removing it might be
> a candidate for such a version.  If you have one commit and ask
> "log", you get one commit back. If you have no commit and ask "log",
> I think it is OK to say that you should get nothing back without
> fuss.
> 
>  builtin/log.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> 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");