Re: [PATCH] blame: default to HEAD in a bare repo when no start commit is given

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

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@gmail.com>
>>
>> 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 &&