{"thread":{"id":"33311","subject":"[PATCH] use refnames instead of \"left\"/\"right\" in dirdiffs","startedAt":"2013-03-27T22:13:17Z","lastAt":"2013-03-28T14:38:30Z","messageCount":5,"participants":["Christoph Anton Mitterer","John Keeping"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"212466","messageId":"1364422397.8091.1.camel@heisenberg.scientia.net","threadId":"33311","inReplyTo":null,"subject":"[PATCH] use refnames instead of \"left\"/\"right\" in dirdiffs","fromName":"Christoph Anton Mitterer","fromEmail":"mail@christoph.anton.mitterer.name","sentAt":"2013-03-27T22:13:17Z","receivedAt":"2013-03-27T22:13:17Z","isPatch":true,"sender":{"key":"mail@christoph.anton.mitterer.name","avatar":null},"body":"Currently, when a dir-diff is made with git-difftool the two revisions are\nstored in two temporary directories \".../left\" and \".../right\".\nMany difftools show these pathnames in ther UI and therefore it would be helpful\nfor users, if actual reference names specified as progam arguments was used\ninstead.\n\nReference names might contain slash / characters which are not allowed to be\npart of a file name. These must therefore be encoded.\n\nAlso, reference names that would could possibly \"break out\" of the temporary\ndirectory (e.g. \"/foo\", \"foo/../bar\" or \"foo/././bar\") must be sanitised.\n* Added a subroutine escape_reference_to_single_directory_name() which encodes a\n  reference name to a valid single directory name.\n  Any occurance of a slash / is replaced by two backslashes \\\\.\n  Having a backslash \\ in a reference name should be forbidden, but just to be\n  save from collisions, any occurance of a backslash \\ is replaced by a\n  backslash followed by an underscore \\_ at first.\n\n* Use this new function to construct the pathnames of the temporary directories\n  for the two revisions in dir-diffs.\n\nSigned-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 12231fb..53e756d 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -83,6 +83,28 @@ sub exit_cleanup\n        exit($status | ($status >> 8));\n }\n \n+sub escape_reference_to_single_directory_name\n+{\n+       # Git allows reference names (see git-check-ref-format(1)) which cannot\n+       # be directly mapped to a single directory name.\n+       #\n+       # This subroutines replaces any occurance of a slash / by two\n+       # backslashes \\\\.\n+       # Thereby, break-out attempts like \"/foo\", \"foo/../bar\" or \"foo/././bar\"\n+       # are prevented, too.\n+       #\n+       # Having a backslash \\ in a reference name should be forbidden, but just\n+       # to be save from collisions, any occurance of a backslash \\ is replaced\n+       # by a backslash followed by an underscore \\_ at first.\n+\n+       my ($commit_name)  = @_;\n+\n+       $commit_name =~ s/\\\\/\\\\_/g;\n+       $commit_name =~ s/\\//\\\\\\\\/g;\n+\n+       return $commit_name;\n+}\n+\n sub setup_dir_diff\n {\n        my ($repo, $workdir, $symlinks) = @_;\n@@ -169,8 +191,13 @@ EOF\n \n        # Setup temp directories\n        my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n-       my $ldir = \"$tmpdir/left\";\n-       my $rdir = \"$tmpdir/right\";\n+       my $ldir = \"$tmpdir/\" . escape_reference_to_single_directory_name($ARGV[0]);\n+       my $rdir = \"$tmpdir/\";\n+       if (@ARGV < 2) {\n+               $rdir .= 'HEAD';\n+       } else {\n+               $rdir .= escape_reference_to_single_directory_name($ARGV[1]);\n+       }\n        mkpath($ldir) or exit_cleanup($tmpdir, 1);\n        mkpath($rdir) or exit_cleanup($tmpdir, 1);\n \n\n"},{"id":"212479","messageId":"20130327230715.GU2286@serenity.lan","threadId":"33311","inReplyTo":"1364422397.8091.1.camel@heisenberg.scientia.net","subject":"Re: [PATCH] use refnames instead of \"left\"/\"right\" in dirdiffs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-27T23:07:16Z","receivedAt":"2013-03-27T23:07:16Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Wed, Mar 27, 2013 at 11:13:17PM +0100, Christoph Anton Mitterer wrote:\n> Currently, when a dir-diff is made with git-difftool the two revisions are\n> stored in two temporary directories \".../left\" and \".../right\".\n> Many difftools show these pathnames in ther UI and therefore it would be helpful\n> for users, if actual reference names specified as progam arguments was used\n> instead.\n> \n> Reference names might contain slash / characters which are not allowed to be\n> part of a file name. These must therefore be encoded.\n> \n> Also, reference names that would could possibly \"break out\" of the temporary\n> directory (e.g. \"/foo\", \"foo/../bar\" or \"foo/././bar\") must be sanitised.\n> * Added a subroutine escape_reference_to_single_directory_name() which encodes a\n>   reference name to a valid single directory name.\n>   Any occurance of a slash / is replaced by two backslashes \\\\.\n>   Having a backslash \\ in a reference name should be forbidden, but just to be\n>   save from collisions, any occurance of a backslash \\ is replaced by a\n>   backslash followed by an underscore \\_ at first.\n> \n> * Use this new function to construct the pathnames of the temporary directories\n>   for the two revisions in dir-diffs.\n> \n> Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>\n> \n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 12231fb..53e756d 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -83,6 +83,28 @@ sub exit_cleanup\n>         exit($status | ($status >> 8));\n>  }\n>  \n> +sub escape_reference_to_single_directory_name\n> +{\n> +       # Git allows reference names (see git-check-ref-format(1)) which cannot\n> +       # be directly mapped to a single directory name.\n> +       #\n> +       # This subroutines replaces any occurance of a slash / by two\n> +       # backslashes \\\\.\n> +       # Thereby, break-out attempts like \"/foo\", \"foo/../bar\" or \"foo/././bar\"\n> +       # are prevented, too.\n\nThat's not going to work well on Windows, is it?  Anything with two dots\nin is already forbidden so we don't need to worry about that; I'm not\nsure we need to remove forward slashes at all, until we consider the\n\"commit containing\" syntax ':/fix nasty bug' or 'master^{/fix bug}'.\n\nI'm more concerned with specifiers containing '^', '@', '{', ':' - see\n'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of\nwhat's acceptable.  At some point I think it may be better to fall back\nto the SHA1 of the relevant commit.\n\n> +       #\n> +       # Having a backslash \\ in a reference name should be forbidden, but just\n> +       # to be save from collisions, any occurance of a backslash \\ is replaced\n> +       # by a backslash followed by an underscore \\_ at first.\n> +\n> +       my ($commit_name)  = @_;\n> +\n> +       $commit_name =~ s/\\\\/\\\\_/g;\n> +       $commit_name =~ s/\\//\\\\\\\\/g;\n> +\n> +       return $commit_name;\n> +}\n> +\n>  sub setup_dir_diff\n>  {\n>         my ($repo, $workdir, $symlinks) = @_;\n> @@ -169,8 +191,13 @@ EOF\n>  \n>         # Setup temp directories\n>         my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 0, TMPDIR => 1);\n> -       my $ldir = \"$tmpdir/left\";\n> -       my $rdir = \"$tmpdir/right\";\n> +       my $ldir = \"$tmpdir/\" . escape_reference_to_single_directory_name($ARGV[0]);\n> +       my $rdir = \"$tmpdir/\";\n> +       if (@ARGV < 2) {\n> +               $rdir .= 'HEAD';\n> +       } else {\n> +               $rdir .= escape_reference_to_single_directory_name($ARGV[1]);\n> +       }\n\nI don't think this approach is general enough, since git-difftool\naccepts the same range of arguments that git-diff does.\n\nWe need to walk over @ARGV here, ignore anything starting with '-', stop\nwhen we reach '--' and handle the arguments found in there including the\nthree acceptable range forms:\n\n    <commit> <commit>\n    <commit>..<commit>\n    <commit>...<commit>\n\nreplacing missing commits with HEAD.\n\n>         mkpath($ldir) or exit_cleanup($tmpdir, 1);\n>         mkpath($rdir) or exit_cleanup($tmpdir, 1);\n>  \n> \n"},{"id":"212504","messageId":"1364474776.11891.9.camel@heisenberg.scientia.net","threadId":"33311","inReplyTo":"20130327230715.GU2286@serenity.lan","subject":"Re: [PATCH] use refnames instead of \"left\"/\"right\" in dirdiffs","fromName":"Christoph Anton Mitterer","fromEmail":"mail@christoph.anton.mitterer.name","sentAt":"2013-03-28T12:46:16Z","receivedAt":"2013-03-28T12:46:16Z","isPatch":true,"sender":{"key":"mail@christoph.anton.mitterer.name","avatar":null},"body":"Hi John.\n\n\nOn Wed, 2013-03-27 at 23:07 +0000, John Keeping wrote: \n> That's not going to work well on Windows, is it?\nUhm Winwhat? No seriously... doesn't dir-diff fail ther anway? The mkdir\nright now also uses mkpath with \"/\"... and I could read in it's\ndocumentation that it would automatically translate this, does it?\n\n>  Anything with two dots\n> in is already forbidden so we don't need to worry about that\nSure about this? I mean they're forbidden as refnames, but is this\nreally checked within git-difftool before?\n\n> ; I'm not\n> sure we need to remove forward slashes at all\nMhh could be... seems that the cleanup happens via completely removing\nthe $tmpdir path.\nAnd for the actual diff it shouldn't matter when there's a partentdir\nmore.\n\n\n>  until we consider the\n> \"commit containing\" syntax ':/fix nasty bug' or 'master^{/fix bug}'.\nPhew... don't ask me... I'm not that into the git source code... from\nthe filename side, these shouldn't bother, only / an NUL is forbidden\n(in POSIX,... again I haven't checked win/mac which might not adhere to\nthe standards).\n\n> I'm more concerned with specifiers containing '^', '@', '{', ':' - see\n> 'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of\n> what's acceptable.\nMhh there are likely more problems... I just noted that $ldir/$rdir are\nused in a call to system() so... that needs to be shell escaped to\nprevent any bad stuff\nAnd if perl (me not a perl guy) interprets any such characters specially\nin strings, it must be handled either.\n\n> At some point I think it may be better to fall back\n> to the SHA1 of the relevant commit.\nThink that would be quite a drawback...\n\nCheers,\nChris.\n"},{"id":"212519","messageId":"20130328141944.GW2286@serenity.lan","threadId":"33311","inReplyTo":"1364474776.11891.9.camel@heisenberg.scientia.net","subject":"Re: [PATCH] use refnames instead of \"left\"/\"right\" in dirdiffs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-28T14:19:45Z","receivedAt":"2013-03-28T14:19:45Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Mar 28, 2013 at 01:46:16PM +0100, Christoph Anton Mitterer wrote:\n> On Wed, 2013-03-27 at 23:07 +0000, John Keeping wrote: \n> > That's not going to work well on Windows, is it?\n>\n> Uhm Winwhat? No seriously... doesn't dir-diff fail ther anway? The mkdir\n> right now also uses mkpath with \"/\"... and I could read in it's\n> documentation that it would automatically translate this, does it?\n\nI believe Windows generally accepts '/' as a directory separator in\nplace of '\\'.  In a recent thread Johannes Sixt reported a difftool test\nfailure on Windows, so it does work (and we do have code to implicitly\nset --no-symlinks when running on Windows).\n\n> >  Anything with two dots\n> > in is already forbidden so we don't need to worry about that\n>\n> Sure about this? I mean they're forbidden as refnames, but is this\n> really checked within git-difftool before?\n\nWe've already run git-diff by this point, and that should have\ncomplained if the refs are invalid, causing difftool to die.\n\n> > ; I'm not\n> > sure we need to remove forward slashes at all\n>\n> Mhh could be... seems that the cleanup happens via completely removing\n> the $tmpdir path.\n> And for the actual diff it shouldn't matter when there's a partentdir\n> more.\n> \n> >  until we consider the\n> > \"commit containing\" syntax ':/fix nasty bug' or 'master^{/fix bug}'.\n>\n> Phew... don't ask me... I'm not that into the git source code... from\n> the filename side, these shouldn't bother, only / an NUL is forbidden\n> (in POSIX,... again I haven't checked win/mac which might not adhere to\n> the standards).\n\nFilenames on Windows cannot contain any of the following[1]:\n\n    \\ / : * ? \" < > |\n\nbut it's also potentially annoying that '^' and '{' have special meaning\nin some shells and would need escaping (although I suppose we don't\nreally expect users to by typing these directory names in themselves).\n\n> > I'm more concerned with specifiers containing '^', '@', '{', ':' - see\n> > 'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of\n> > what's acceptable.\n>\n> Mhh there are likely more problems... I just noted that $ldir/$rdir are\n> used in a call to system() so... that needs to be shell escaped to\n> prevent any bad stuff\n\nAre there really non-list calls to system?  Providing the only calls\nprovide each of the arguments separately (instead of in a single string)\nI think we're OK, but I'm also not a Perl expert.\n\n> And if perl (me not a perl guy) interprets any such characters specially\n> in strings, it must be handled either.\n> \n> > At some point I think it may be better to fall back\n> > to the SHA1 of the relevant commit.\n> Think that would be quite a drawback...\n\nIt depends where that happens.  I suspect most people use relatively\nsimple ref specifiers which wouldn't get to this stage, but do you\nreally want to turn the following into a directory name?\n\n    origin/master@{3 weeks ago}^{/Merge branch 'jk/}^2\n\nI admit it's something of a contrived example but I hope it illustrates\nmy point that sometimes naming the directory after the ref specifier may\nbe less useful than just using \"left\" or the SHA1.\n\n\n[1] http://support.microsoft.com/kb/177506\n\n\nJohn\n"},{"id":"212520","messageId":"20130328143830.GX2286@serenity.lan","threadId":"33311","inReplyTo":"20130328141944.GW2286@serenity.lan","subject":"Re: [PATCH] use refnames instead of \"left\"/\"right\" in dirdiffs","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-03-28T14:38:30Z","receivedAt":"2013-03-28T14:38:30Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Mar 28, 2013 at 02:19:44PM +0000, John Keeping wrote:\n> On Thu, Mar 28, 2013 at 01:46:16PM +0100, Christoph Anton Mitterer wrote:\n> > On Wed, 2013-03-27 at 23:07 +0000, John Keeping wrote: \n> > > That's not going to work well on Windows, is it?\n> >\n> > Uhm Winwhat? No seriously... doesn't dir-diff fail ther anway? The mkdir\n> > right now also uses mkpath with \"/\"... and I could read in it's\n> > documentation that it would automatically translate this, does it?\n> \n> I believe Windows generally accepts '/' as a directory separator in\n> place of '\\'.  In a recent thread Johannes Sixt reported a difftool test\n> failure on Windows, so it does work (and we do have code to implicitly\n> set --no-symlinks when running on Windows).\n> \n> > >  Anything with two dots\n> > > in is already forbidden so we don't need to worry about that\n> >\n> > Sure about this? I mean they're forbidden as refnames, but is this\n> > really checked within git-difftool before?\n> \n> We've already run git-diff by this point, and that should have\n> complained if the refs are invalid, causing difftool to die.\n> \n> > > ; I'm not\n> > > sure we need to remove forward slashes at all\n> >\n> > Mhh could be... seems that the cleanup happens via completely removing\n> > the $tmpdir path.\n> > And for the actual diff it shouldn't matter when there's a partentdir\n> > more.\n> > \n> > >  until we consider the\n> > > \"commit containing\" syntax ':/fix nasty bug' or 'master^{/fix bug}'.\n> >\n> > Phew... don't ask me... I'm not that into the git source code... from\n> > the filename side, these shouldn't bother, only / an NUL is forbidden\n> > (in POSIX,... again I haven't checked win/mac which might not adhere to\n> > the standards).\n> \n> Filenames on Windows cannot contain any of the following[1]:\n> \n>     \\ / : * ? \" < > |\n> \n> but it's also potentially annoying that '^' and '{' have special meaning\n> in some shells and would need escaping (although I suppose we don't\n> really expect users to by typing these directory names in themselves).\n> \n> > > I'm more concerned with specifiers containing '^', '@', '{', ':' - see\n> > > 'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of\n> > > what's acceptable.\n> >\n> > Mhh there are likely more problems... I just noted that $ldir/$rdir are\n> > used in a call to system() so... that needs to be shell escaped to\n> > prevent any bad stuff\n> \n> Are there really non-list calls to system?  Providing the only calls\n> provide each of the arguments separately (instead of in a single string)\n> I think we're OK, but I'm also not a Perl expert.\n> \n> > And if perl (me not a perl guy) interprets any such characters specially\n> > in strings, it must be handled either.\n> > \n> > > At some point I think it may be better to fall back\n> > > to the SHA1 of the relevant commit.\n> > Think that would be quite a drawback...\n> \n> It depends where that happens.  I suspect most people use relatively\n> simple ref specifiers which wouldn't get to this stage, but do you\n> really want to turn the following into a directory name?\n> \n>     origin/master@{3 weeks ago}^{/Merge branch 'jk/}^2\n> \n> I admit it's something of a contrived example but I hope it illustrates\n> my point that sometimes naming the directory after the ref specifier may\n> be less useful than just using \"left\" or the SHA1.\n\nActually, I think I was wrong here it's probably easier to just do\nsomething you already do, but with a whitelist, like this:\n\n\tmy $leftname, $rightname;\n\t// figure out what refs these point at...\n\t$leftname =~ s/[^a-zA-Z0-9]/_/g;\n\t$rightname =~ s/[^a-zA-Z0-9]/_/g;\n\nWe probably want to whitelist some more characters there, but that seems\nlike the simplest way to tackle it.  And if someone puts in a long\nrevision then they get a long directory name (or we truncate it at some\npoint).\n\n> [1] http://support.microsoft.com/kb/177506\n"}]}