Jeff King <peff@xxxxxxxx> 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@xxxxxxxx>
> ---
> 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");
+
/* Any arguments at this point are not recognized */
if (argc > 1)
die(_("unrecognized argument: %s"), argv[1]);
@@ -404,6 +407,14 @@ static int git_log_config(const char *var, const char *value, void *cb)
return git_diff_ui_config(var, value, cb);
}
+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";
+}
+
int cmd_whatchanged(int argc, const char **argv, const char *prefix)
{
struct rev_info rev;
@@ -416,7 +427,7 @@ int cmd_whatchanged(int argc, const char **argv, const char *prefix)
rev.diff = 1;
rev.simplify_history = 0;
memset(&opt, 0, sizeof(opt));
- opt.def = "HEAD";
+ default_to_head_if_exists(&opt);
opt.revarg_opt = REVARG_COMMITTISH;
cmd_log_init(argc, argv, prefix, &rev, &opt);
if (!rev.diffopt.output_format)
@@ -530,7 +541,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
rev.diffopt.stat_width = -1; /* Scale to real terminal size */
memset(&opt, 0, sizeof(opt));
- opt.def = "HEAD";
+ default_to_head_if_exists(&opt);
opt.tweak = show_rev_tweak_rev;
cmd_log_init(argc, argv, prefix, &rev, &opt);
@@ -607,7 +618,7 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)
init_reflog_walk(&rev.reflog_info);
rev.verbose_header = 1;
memset(&opt, 0, sizeof(opt));
- opt.def = "HEAD";
+ default_to_head_if_exists(&opt);
cmd_log_init_defaults(&rev);
rev.abbrev_commit = 1;
rev.commit_format = CMIT_FMT_ONELINE;
@@ -629,7 +640,7 @@ int cmd_log(int argc, const char **argv, const char *prefix)
init_revisions(&rev, prefix);
rev.always_show_header = 1;
memset(&opt, 0, sizeof(opt));
- opt.def = "HEAD";
+ default_to_head_if_exists(&opt);
opt.revarg_opt = REVARG_COMMITTISH;
cmd_log_init(argc, argv, prefix, &rev, &opt);
return cmd_log_walk(&rev);
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html