On Mon, Apr 08 2019, SZEDER Gábor wrote:
> 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.
I mean for context in the sense that the current error we display we
haven't always shown, and explicitly started doing in that commit (but
for another purpose...).
>> 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"));
Yeah, I missed that it's the same message as with no HEAD in a non-bare
repo. Makes sense.
> 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.
If you setup a repo to manually fetch refspecs you can end up without a
HEAD but still be able to 'blame' stuff. I don't think that happens with
non-bare, but does with bare. E.g. I set up some server-side "blame"
that supports the "master" branch of several repos like that in the
past, and just plain 'git blame' throws the old message.
So maybe since we're checking is_bare_repository() it's worth improving
the error while we're at it, or at least testing that bare & non-bare do
the same thing.
>>
>> >
>> > 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 &&
