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

["SZEDER Gábor" <szeder.dev@gmail.com>]

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.

> 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 &&