git-blame: handling of revisions and filenames

Alex Riesen· Nov 15, 2006, 21:52 UTC · lore

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);

Junio C Hamano· Nov 29, 2006, 18:29 UTC · re: Alex Riesen · lore

Re: git-blame: handling of revisions and filenames

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.

Show 12 quoted lines

> 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.