git-blame: handling of revisions and filenames

git-blame: handling of revisions and filenames

[Alex Riesen]

I noticed that there is no safe way to give a revision to git-blame:
it can always be interpreted as an existing file:
 - "git blame rev -- file.txt" can fail if "rev" is a file
 - "git blame rev^0 -- file.txt" can file there is a "rev^0" file
   (happens if you type too fast with a tool not designed for keyboard)
 - "git blame file.txt rev" is ambiguos too, for the same reasons.

I did the simple patch (below) to resolve at least the very first one,
just because that is how git-rev-list does it.
But if all forms of git-blame command line are expected to live, a
more serious surgery of the argv[] handling code needed.
And I afraid the patch has a small chance of crashing: I don't check
if there is enough space in argv (don't actually even know how to),
so Junio, please do not apply it (it passes blame tests, though).
I also suspect git-blame is not the only command using revision
machinery which has the same problem, so this message is more like a
discussion starter.

diff --git a/builtin-blame.c b/builtin-blame.c
index 066dee7..83c8905 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -1787,6 +1787,7 @@ int cmd_blame(int argc, const char **arg
        /* Now we got rev and path.  We do not want the path pruning
         * but we may want "bottom" processing.
         */
+       argv[unk++] = "--";
        argv[unk] = NULL;
 
        init_revisions(&revs, NULL);

Re: git-blame: handling of revisions and filenames

[Junio C Hamano]

fork0@t-online.de (Alex Riesen) writes:

> And I afraid the patch has a small chance of crashing: I don't check
> if there is enough space in argv (don't actually even know how to),

Actually this is a safe thing to do because we already know (and
removed) the "path" argument from the command line at that
point.  If the original command line did not have "path" we have
already rejected it.  So unk at that point never exceeds argc.

> diff --git a/builtin-blame.c b/builtin-blame.c
> index 066dee7..83c8905 100644
> --- a/builtin-blame.c
> +++ b/builtin-blame.c
> @@ -1787,6 +1787,7 @@ int cmd_blame(int argc, const char **arg
>         /* Now we got rev and path.  We do not want the path pruning
>          * but we may want "bottom" processing.
>          */
> +       argv[unk++] = "--";
>         argv[unk] = NULL;
>  
>         init_revisions(&revs, NULL);

Thanks.