{"thread":{"id":"47764","subject":"[RFH/PATCH] blame: tighten command line parser","startedAt":"2018-02-05T23:37:39Z","lastAt":"2018-02-06T04:42:59Z","messageCount":2,"participants":["Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"338264","messageId":"xmqqmv0n80ue.fsf@gitster-ct.c.googlers.com","threadId":"47764","inReplyTo":null,"subject":"[RFH/PATCH] blame: tighten command line parser","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-05T23:37:29Z","receivedAt":"2018-02-05T23:37:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The command line parser of \"git blame\" is prepared to take an\nancient odd argument order \"blame <path> <rev>\" in addition to the\nusual \"blame [<rev>] <path>\".  It has at least two negative\nramifications:\n\n - In order to tell these two apart, it checks if the last command\n   line argument names a path in the working tree, using\n   file_exists().  However, \"blame <rev> <path>\" is a request to\n   explalin each and every line in the contents of <path> stored in\n   revision <rev> and does not need to have a working tree version\n   of the file.  A check with file_exists() is simply wrong.\n\n - To coerce that mistaken file_exists() check to work, the code\n   calls setup_work_tree() before doing so, because the path it has\n   is relative to the top-level of the project tree.  However,\n   \"blame <rev> <path>\" MUST be usable even in a bare repository,\n   and there is no reason for letting setup_work_tree() to complain\n   and die with \"This operation must be run in a work tree\".\n\nTo correct the former, switch to check if the last token is a\nrevision (and if so, parse arguments using \"blame <path> <rev>\"\nrule).  Correct the latter by getting rid of setup_work_tree() and\nfile_exists() check--the only case the call to this function matters\nis when we are running \"blame <path>\" (i.e. no starting revision and\nasking to blame the working tree file at <path>, digging through the\nHEAD revision), but there is a call in setup_scoreboard() just\nbefore it calls fake_working_tree_commit().\n\nAlso attempt to give a bit more sensible error message when \"blame\nXYZ\" is given and XYZ cannot be a path.\n\n   side note: I am not happy with the \"only one arg, which is a rev,\n   given in a bare repository\" condition to give the new error\n   message, but this should be a good starting point.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n * Somebody noticed that this transcript looked funny.\n\n   $ cd .git\n   $ git blame HEAD\n   fatal: This operation must be run in a work tree\n   \n   If it were a request for blaming Makefile instead, this error\n   message may make some sense, though.\n\n   By the way, I am not happy with is_a_rev() either.  There should\n   be a handy helper function (or two) we already have that I am\n   forgetting.\n\n builtin/blame.c | 20 ++++++++++++++------\n 1 file changed, 14 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 005f55aaa2..9dcb367b90 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -649,6 +649,15 @@ static int blame_move_callback(const struct option *option, const char *arg, int\n \treturn 0;\n }\n \n+static int is_a_rev(const char *name)\n+{\n+\tstruct object_id oid;\n+\n+\tif (get_oid(name, &oid))\n+\t\treturn 0;\n+\treturn OBJ_NONE < sha1_object_info(oid.hash, NULL);\n+}\n+\n int cmd_blame(int argc, const char **argv, const char *prefix)\n {\n \tstruct rev_info revs;\n@@ -845,16 +854,15 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t} else {\n \t\tif (argc < 2)\n \t\t\tusage_with_options(blame_opt_usage, options);\n-\t\tpath = add_prefix(prefix, argv[argc - 1]);\n-\t\tif (argc == 3 && !file_exists(path)) { /* (2b) */\n+\t\tif (argc == 3 && is_a_rev(argv[argc - 1])) { /* (2b) */\n \t\t\tpath = add_prefix(prefix, argv[1]);\n \t\t\targv[1] = argv[2];\n+\t\t} else {\t/* (2a) */\n+\t\t\tif (argc == 2 && is_a_rev(argv[1]) && !get_git_work_tree())\n+\t\t\t\tdie(\"missing <path> to blame\");\n+\t\t\tpath = add_prefix(prefix, argv[argc - 1]);\n \t\t}\n \t\targv[argc - 1] = \"--\";\n-\n-\t\tsetup_work_tree();\n-\t\tif (!file_exists(path))\n-\t\t\tdie_errno(\"cannot stat path '%s'\", path);\n \t}\n \n \trevs.disable_stdin = 1;\n-- \n2.16.1-72-g5be1f00a9a\n\n"},{"id":"338478","messageId":"CAPig+cTFSWJX3FyXA6JJPhd1TT9eS0QOtm_R2qB6SX8W60ZWtA@mail.gmail.com","threadId":"47764","inReplyTo":"xmqqmv0n80ue.fsf@gitster-ct.c.googlers.com","subject":"Re: [RFH/PATCH] blame: tighten command line parser","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-02-06T04:42:53Z","receivedAt":"2018-02-06T04:42:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Feb 5, 2018 at 6:37 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> The command line parser of \"git blame\" is prepared to take an\n> ancient odd argument order \"blame <path> <rev>\" in addition to the\n> usual \"blame [<rev>] <path>\".  It has at least two negative\n> ramifications:\n>\n>  - In order to tell these two apart, it checks if the last command\n>    line argument names a path in the working tree, using\n>    file_exists().  However, \"blame <rev> <path>\" is a request to\n>    explalin each and every line in the contents of <path> stored in\n\ns/explalin/explain/\n\n>    revision <rev> and does not need to have a working tree version\n>    of the file.  A check with file_exists() is simply wrong.\n>\n>  - To coerce that mistaken file_exists() check to work, the code\n>    calls setup_work_tree() before doing so, because the path it has\n>    is relative to the top-level of the project tree.  However,\n>    \"blame <rev> <path>\" MUST be usable even in a bare repository,\n>    and there is no reason for letting setup_work_tree() to complain\n\ns/to complain/complain/\n\n>    and die with \"This operation must be run in a work tree\".\n>\n> To correct the former, switch to check if the last token is a\n> revision (and if so, parse arguments using \"blame <path> <rev>\"\n> rule).  Correct the latter by getting rid of setup_work_tree() and\n> file_exists() check--the only case the call to this function matters\n> is when we are running \"blame <path>\" (i.e. no starting revision and\n> asking to blame the working tree file at <path>, digging through the\n> HEAD revision), but there is a call in setup_scoreboard() just\n> before it calls fake_working_tree_commit().\n>\n> Also attempt to give a bit more sensible error message when \"blame\n> XYZ\" is given and XYZ cannot be a path.\n>\n>    side note: I am not happy with the \"only one arg, which is a rev,\n>    given in a bare repository\" condition to give the new error\n>    message, but this should be a good starting point.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"}]}