* [RFH/PATCH] blame: tighten command line parser
@ 2018-02-05 23:37 Junio C Hamano
2018-02-06 4:42 ` Eric Sunshine
0 siblings, 1 reply; 2+ messages in thread
From: Junio C Hamano @ 2018-02-05 23:37 UTC (permalink / raw)
To: git
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 <gitster@pobox.com>
---
* 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
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [RFH/PATCH] blame: tighten command line parser
2018-02-05 23:37 [RFH/PATCH] blame: tighten command line parser Junio C Hamano
@ 2018-02-06 4:42 ` Eric Sunshine
0 siblings, 0 replies; 2+ messages in thread
From: Eric Sunshine @ 2018-02-06 4:42 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
On Mon, Feb 5, 2018 at 6:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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
s/explalin/explain/
> 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
s/to complain/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 <gitster@pobox.com>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-02-06 4:42 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-05 23:37 [RFH/PATCH] blame: tighten command line parser Junio C Hamano
2018-02-06 4:42 ` Eric Sunshine
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).