On Tue, Jun 02, 2015 at 11:48:30PM -0700, Junio C Hamano wrote:
> I am kind of surprised after reading these two threads that my
> take on this issue has changed over time, as my knee-jerk
> reaction before reading them was the opposite, something
> along the lines of "This is only immediately after 'git init'; why
> bother?". Or depending on my mood, that "How stupid do you
> have to be..." sounds exactly like a message I may advocate
> for us to send. Perhaps I grew more bitter with age.
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?".
> Jokes aside, I wonder why we did not pursue that "default to
> HEAD only when HEAD points at a branch that exists"
> approach, with the definition of "exists" being "either a file
> exists under the $GIT_DIR/refs/heads directory with any
> (including corrupt) contents, or an entry in the packed refs
> file exists with any (including corrupt) contents". With or
> without the error exit and warning, that sounds like a very
> sensible way to set the default, at least to me.
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.
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 <p...@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. 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. ;)
revision.c | 19 ++++++++++++++++++-
t/t4202-log.sh | 11 +++++++++++
2 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/revision.c b/revision.c
index 7ddbaa0..775df8b 100644
--- a/revision.c
+++ b/revision.c
@@ -2170,6 +2170,23 @@ static int handle_revision_pseudo_opt(const char
*submodule,
return 1;
}
+static void NORETURN diagnose_missing_default(const char *def)
+{
+ unsigned char sha1[20];
+ int flags;
+ const char *refname;
+
+ refname = resolve_ref_unsafe(def, 0, sha1, &flags);
+ if (!(flags & REF_ISSYMREF))
+ die(_("bad default revision '%s'"), def);
+ if (flags & REF_ISBROKEN)
+ die(_("default revision '%s' points to a broken branch"), def);
+
+ skip_prefix(refname, "refs/heads/", &refname);
+ die(_("default revision '%s' points to an unborn branch '%s'"),
+ def, refname);
+}
+
/*
* Parse revision information, filling in the "rev_info" structure,
* and removing the used arguments from the argument list.
@@ -2299,7 +2316,7 @@ int setup_revisions(int argc, const char **argv, struct
rev_info *revs, struct s
struct object *object;
struct object_context oc;
if (get_sha1_with_context(revs->def, 0, sha1, &oc))
- die("bad default revision '%s'", revs->def);
+ diagnose_missing_default(revs->def);
object = get_reference(revs, revs->def, sha1, 0);
add_pending_object_with_mode(revs, object, revs->def, oc.mode);
}
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 1b2e981..71d8326 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -871,4 +871,15 @@ test_expect_success 'log --graph --no-walk is forbidden' '
test_must_fail git log --graph --no-walk
'
+test_expect_success 'log diagnoses bogus HEAD' '
+ git init empty &&
+ test_must_fail git -C empty log 2>stderr &&
+ test_i18ngrep unborn stderr &&
+ echo 1234abcd >empty/.git/refs/heads/master &&
+ test_must_fail git -C empty log 2>stderr &&
+ test_i18ngrep broken stderr &&
+ test_must_fail git -C empty log --default totally-bogus 2>stderr &&
+ test_i18ngrep bad.default.revision stderr
+'
+
test_done
--
2.4.2.745.g0aa058d
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
