The command line parser of "git blame" is prepared to take an ancient odd argument order "blame <path> <rev>" in addition to the usual "blame [<rev>] <path>". It has at least two negative ramifications:
- In order to tell these two apart, it checks if the last command line argument names a path in the working tree, using file_exists(). However, "blame <rev> <path>" is a request to explalin each and every line in the contents of <path> stored in revision <rev> and does not need to have a working tree version of the file. A check with file_exists() is simply wrong. - To coerce that mistaken file_exists() check to work, the code calls setup_work_tree() before doing so, because the path it has is relative to the top-level of the project tree. However, "blame <rev> <path>" MUST be usable even in a bare repository, and there is no reason for letting setup_work_tree() to complain and die with "This operation must be run in a work tree". To correct the former, switch to check if the last token is a revision (and if so, parse arguments using "blame <path> <rev>" rule). Correct the latter by getting rid of setup_work_tree() and file_exists() check--the only case the call to this function matters is when we are running "blame <path>" (i.e. no starting revision and asking to blame the working tree file at <path>, digging through the HEAD revision), but there is a call in setup_scoreboard() just before it calls fake_working_tree_commit(). Also attempt to give a bit more sensible error message when "blame XYZ" is given and XYZ cannot be a path. side note: I am not happy with the "only one arg, which is a rev, given in a bare repository" condition to give the new error message, but this should be a good starting point. Signed-off-by: Junio C Hamano <[email protected]> --- * Somebody noticed that this transcript looked funny. $ cd .git $ git blame HEAD fatal: This operation must be run in a work tree If it were a request for blaming Makefile instead, this error message may make some sense, though. By the way, I am not happy with is_a_rev() either. There should be a handy helper function (or two) we already have that I am forgetting. builtin/blame.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index 005f55aaa2..9dcb367b90 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -649,6 +649,15 @@ static int blame_move_callback(const struct option *option, const char *arg, int return 0; } +static int is_a_rev(const char *name) +{ + struct object_id oid; + + if (get_oid(name, &oid)) + return 0; + return OBJ_NONE < sha1_object_info(oid.hash, NULL); +} + int cmd_blame(int argc, const char **argv, const char *prefix) { struct rev_info revs; @@ -845,16 +854,15 @@ int cmd_blame(int argc, const char **argv, const char *prefix) } else { if (argc < 2) usage_with_options(blame_opt_usage, options); - path = add_prefix(prefix, argv[argc - 1]); - if (argc == 3 && !file_exists(path)) { /* (2b) */ + if (argc == 3 && is_a_rev(argv[argc - 1])) { /* (2b) */ path = add_prefix(prefix, argv[1]); argv[1] = argv[2]; + } else { /* (2a) */ + if (argc == 2 && is_a_rev(argv[1]) && !get_git_work_tree()) + die("missing <path> to blame"); + path = add_prefix(prefix, argv[argc - 1]); } argv[argc - 1] = "--"; - - setup_work_tree(); - if (!file_exists(path)) - die_errno("cannot stat path '%s'", path); } revs.disable_stdin = 1; -- 2.16.1-72-g5be1f00a9a
