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

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

[SZEDER Gábor]

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>
---
 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");
+	}
 
 	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
+	)
+'
+
 test_expect_success 'blame by tag objects' '
 	git tag -m "test tag" testTag &&
 	git tag -m "test tag #2" testTag2 testTag &&
-- 
2.21.0.539.g07239c3a71.dirty

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

[Eric Sunshine]

On Sun, Apr 7, 2019 at 7:43 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> [...]
> 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).
> [...]
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -993,6 +994,18 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
> +       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");

This source file is already mostly internationalized, so perhaps:

    die(_("no such ref: %s"), "HEAD");

> +               add_pending_object(&revs, &head_commit->object, "HEAD");
> +       }

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

[Ævar Arnfjörð Bjarmason]

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

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

[SZEDER Gábor]

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

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

[Ævar Arnfjörð Bjarmason]

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

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

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

Yes.  That is a good suggestion.  I do not think the original meant
to say that no rev should error out in a bare repository because no
rev must mean 'start from te working tree' and there is no way to
satisify it in a bare repository.

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

Sounds like an easy-to-understand message, albeit way too looong.

> Along with a test for what we do in bare repos without a HEAD....?
> ...
> ....just 'git update-ref -d HEAD` after this and a test for 'git blame
> <file>' here would test bare without HEAD.

That's a cute way to bring us on an unborn branch, but let's not
promote it too much.  Doing so while on detached HEAD will render
your repository corrupt.