On Mon, Apr 08, 2019 at 02:44:59PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Apr 08 2019, SZEDER Gábor wrote:
>
> > When 'git blame' is invoked without specifying the commit to start
> > blaming from, it starts from the given file's state in the work tree.
> > However, when invoked in a bare repository without a start commit,
> > then there is no work tree state to start from, and it dies with the
> > following error message:
> >
> > $ git rev-parse --is-bare-repository
> > true
> > $ git blame file.c
> > fatal: this operation must be run in a work tree
> >
> > This is misleading, because it implies that 'git blame' doesn't work
> > in bare repositories at all, but it does, in fact, work just fine when
> > it is given a commit to start from.
> >
> > We could improve the error message, of course, but let's just default
> > to HEAD in a bare repository instead, as most likely that is what the
> > user wanted anyway (if they wanted to start from an other commit, then
> > they would have specified that in the first place).
> >
> > 'git annotate' is just a thin wrapper around 'git blame', so in the
> > same situation it printed the same misleading error message, and this
> > patch fixes it, too.
> >
> > Signed-off-by: SZEDER Gábor <szeder.dev@xxxxxxxxx>
>
> There was the explicit decision not to fall back to HEAD in 1cfe77333f
> ("git-blame: no rev means start from the working tree file.",
> 2007-01-30). This change makes sense to me, but perhaps some discussion
> or reference to the previous commit is warranted?
I don't think so, because that doesn't apply here, since it doesn't
really make much sense to talk about a working tree file in a bare
repo.
> Although from skimming the thread from back then it seems to be "not
> HEAD but working tree file", not "let's not use HEAD in bare repos".
>
> > builtin/blame.c | 13 +++++++++++++
> > t/annotate-tests.sh | 8 ++++++++
> > 2 files changed, 21 insertions(+)
> >
> > diff --git a/builtin/blame.c b/builtin/blame.c
> > index 177c1022a0..21cde57e71 100644
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -27,6 +27,7 @@
> > #include "object-store.h"
> > #include "blame.h"
> > #include "string-list.h"
> > +#include "refs.h"
> >
> > static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
> >
> > @@ -993,6 +994,18 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> >
> > revs.disable_stdin = 1;
> > setup_revisions(argc, argv, &revs, NULL);
> > + if (!revs.pending.nr && is_bare_repository()) {
> > + struct commit *head_commit;
> > + struct object_id head_oid;
> > +
> > + if (!resolve_ref_unsafe("HEAD", RESOLVE_REF_READING,
> > + &head_oid, NULL) ||
> > + !(head_commit = lookup_commit_reference_gently(revs.repo,
> > + &head_oid, 1)))
> > + die("no such ref: HEAD");
> > +
> > + add_pending_object(&revs, &head_commit->object, "HEAD");
> > + }
>
> With this patch, if I have a bare repo without a HEAD I now get:
>
> fatal: no such ref: HEAD
This is the same error message as in a regular repo without HEAD.
That's good: it's consistent, and it tells what the actual problem is.
Though perhaps too terse and the error message from 'git log' would
apply just as well and be more friendly:
fatal: your current branch 'master' does not have any commits yet
> Instead of:
>
> fatal: this operation must be run in a work tree
>
> Both are bad & misleading, perhaps we can instead say something like:
>
> die(_("in a bare repository you must specify a ref to blame from, we tried and failed to implicitly use HEAD"));
The point of this patch is that you don't necessarily have to specify
the starting ref in a bare repo anymore.
> Along with a test for what we do in bare repos without a HEAD....?
How can a repo have no HEAD? Maybe I'm missing something, but I only
see the following cases:
- An empty repo: there is nothing to blame there at all in the first
place.
- An orphan branch in a non-bare repo: there is nothing to blame
there.
- The user is looking for trouble, and ran 'git update-ref -d HEAD',
as you mentioned below, or something else with similar results.
"If it hurts, don't it" applies.
- Some sort of repo corruption that left the refs in a sorry state.
The user has more serious problems than the error message from
'git blame'.
So I doubt that any of these cases are worth dealing with and testing
specifically in a bare repo.
>
> >
> > init_scoreboard(&sb);
> > sb.revs = &revs;
> > diff --git a/t/annotate-tests.sh b/t/annotate-tests.sh
> > index 6da48a2e0a..d933af5714 100644
> > --- a/t/annotate-tests.sh
> > +++ b/t/annotate-tests.sh
> > @@ -68,6 +68,14 @@ test_expect_success 'blame 1 author' '
> > check_count A 2
> > '
> >
> > +test_expect_success 'blame in a bare repo without starting commit' '
> > + git clone --bare . bare.git &&
> > + (
> > + cd bare.git &&
> > + check_count A 2
> > + )
>
> ....just 'git update-ref -d HEAD` after this and a test for 'git blame
> <file>' here would test bare without HEAD.
>
> > test_expect_success 'blame by tag objects' '
> > git tag -m "test tag" testTag &&
> > git tag -m "test tag #2" testTag2 testTag &&
