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:

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

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

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"));

Along with a test for what we do in bare repos without a HEAD....?

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