Re: [PATCH] use refnames instead of "left"/"right" in dirdiffs

From: John Keeping <john@keeping.me.uk>
Date: Mar 27, 2013, 23:07 UTC
Message-ID: <20130327230715.GU2286@serenity.lan>
In-Reply-To: <1364422397.8091.1.camel@heisenberg.scientia.net>

On Wed, Mar 27, 2013 at 11:13:17PM +0100, Christoph Anton Mitterer wrote:

Show 40 quoted lines

> Currently, when a dir-diff is made with git-difftool the two revisions are
> stored in two temporary directories ".../left" and ".../right".
> Many difftools show these pathnames in ther UI and therefore it would be helpful
> for users, if actual reference names specified as progam arguments was used
> instead.
> 
> Reference names might contain slash / characters which are not allowed to be
> part of a file name. These must therefore be encoded.
> 
> Also, reference names that would could possibly "break out" of the temporary
> directory (e.g. "/foo", "foo/../bar" or "foo/././bar") must be sanitised.
> * Added a subroutine escape_reference_to_single_directory_name() which encodes a
>   reference name to a valid single directory name.
>   Any occurance of a slash / is replaced by two backslashes \\.
>   Having a backslash \ in a reference name should be forbidden, but just to be
>   save from collisions, any occurance of a backslash \ is replaced by a
>   backslash followed by an underscore \_ at first.
> 
> * Use this new function to construct the pathnames of the temporary directories
>   for the two revisions in dir-diffs.
> 
> Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
> 
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 12231fb..53e756d 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -83,6 +83,28 @@ sub exit_cleanup
>         exit($status | ($status >> 8));
>  }
>  
> +sub escape_reference_to_single_directory_name
> +{
> +       # Git allows reference names (see git-check-ref-format(1)) which cannot
> +       # be directly mapped to a single directory name.
> +       #
> +       # This subroutines replaces any occurance of a slash / by two
> +       # backslashes \\.
> +       # Thereby, break-out attempts like "/foo", "foo/../bar" or "foo/././bar"
> +       # are prevented, too.

That's not going to work well on Windows, is it? Anything with two dots in is already forbidden so we don't need to worry about that; I'm not sure we need to remove forward slashes at all, until we consider the "commit containing" syntax ':/fix nasty bug' or 'master^{/fix bug}'.

I'm more concerned with specifiers containing '^', '@', '{', ':' - see 'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of what's acceptable. At some point I think it may be better to fall back to the SHA1 of the relevant commit.

Show 29 quoted lines

> +       #
> +       # Having a backslash \ in a reference name should be forbidden, but just
> +       # to be save from collisions, any occurance of a backslash \ is replaced
> +       # by a backslash followed by an underscore \_ at first.
> +
> +       my ($commit_name)  = @_;
> +
> +       $commit_name =~ s/\\/\\_/g;
> +       $commit_name =~ s/\//\\\\/g;
> +
> +       return $commit_name;
> +}
> +
>  sub setup_dir_diff
>  {
>         my ($repo, $workdir, $symlinks) = @_;
> @@ -169,8 +191,13 @@ EOF
>  
>         # Setup temp directories
>         my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);
> -       my $ldir = "$tmpdir/left";
> -       my $rdir = "$tmpdir/right";
> +       my $ldir = "$tmpdir/" . escape_reference_to_single_directory_name($ARGV[0]);
> +       my $rdir = "$tmpdir/";
> +       if (@ARGV < 2) {
> +               $rdir .= 'HEAD';
> +       } else {
> +               $rdir .= escape_reference_to_single_directory_name($ARGV[1]);
> +       }

I don't think this approach is general enough, since git-difftool accepts the same range of arguments that git-diff does.

We need to walk over @ARGV here, ignore anything starting with '-', stop when we reach '--' and handle the arguments found in there including the three acceptable range forms:

    <commit> <commit>
    <commit>..<commit>
    <commit>...<commit>

replacing missing commits with HEAD.

>         mkpath($ldir) or exit_cleanup($tmpdir, 1);
>         mkpath($rdir) or exit_cleanup($tmpdir, 1);
>  
>