| Message ID | 20190407234327.25617-1-szeder.dev@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blame: default to HEAD in a bare repo when no start commit is given | expand |
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"); > + }
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 &&
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 &&
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 &&
Æ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.
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 &&
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(+)