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

[John Keeping <john@keeping.me.uk>]

On Thu, Mar 28, 2013 at 02:19:44PM +0000, John Keeping wrote:
> On Thu, Mar 28, 2013 at 01:46:16PM +0100, Christoph Anton Mitterer wrote:
> > On Wed, 2013-03-27 at 23:07 +0000, John Keeping wrote: 
> > > That's not going to work well on Windows, is it?
> >
> > Uhm Winwhat? No seriously... doesn't dir-diff fail ther anway? The mkdir
> > right now also uses mkpath with "/"... and I could read in it's
> > documentation that it would automatically translate this, does it?
> 
> I believe Windows generally accepts '/' as a directory separator in
> place of '\'.  In a recent thread Johannes Sixt reported a difftool test
> failure on Windows, so it does work (and we do have code to implicitly
> set --no-symlinks when running on Windows).
> 
> > >  Anything with two dots
> > > in is already forbidden so we don't need to worry about that
> >
> > Sure about this? I mean they're forbidden as refnames, but is this
> > really checked within git-difftool before?
> 
> We've already run git-diff by this point, and that should have
> complained if the refs are invalid, causing difftool to die.
> 
> > > ; I'm not
> > > sure we need to remove forward slashes at all
> >
> > Mhh could be... seems that the cleanup happens via completely removing
> > the $tmpdir path.
> > And for the actual diff it shouldn't matter when there's a partentdir
> > more.
> > 
> > >  until we consider the
> > > "commit containing" syntax ':/fix nasty bug' or 'master^{/fix bug}'.
> >
> > Phew... don't ask me... I'm not that into the git source code... from
> > the filename side, these shouldn't bother, only / an NUL is forbidden
> > (in POSIX,... again I haven't checked win/mac which might not adhere to
> > the standards).
> 
> Filenames on Windows cannot contain any of the following[1]:
> 
>     \ / : * ? " < > |
> 
> but it's also potentially annoying that '^' and '{' have special meaning
> in some shells and would need escaping (although I suppose we don't
> really expect users to by typing these directory names in themselves).
> 
> > > I'm more concerned with specifiers containing '^', '@', '{', ':' - see
> > > 'SPECIFYING REVISIONS' in git-rev-parse(1) for the full details of
> > > what's acceptable.
> >
> > Mhh there are likely more problems... I just noted that $ldir/$rdir are
> > used in a call to system() so... that needs to be shell escaped to
> > prevent any bad stuff
> 
> Are there really non-list calls to system?  Providing the only calls
> provide each of the arguments separately (instead of in a single string)
> I think we're OK, but I'm also not a Perl expert.
> 
> > And if perl (me not a perl guy) interprets any such characters specially
> > in strings, it must be handled either.
> > 
> > > At some point I think it may be better to fall back
> > > to the SHA1 of the relevant commit.
> > Think that would be quite a drawback...
> 
> It depends where that happens.  I suspect most people use relatively
> simple ref specifiers which wouldn't get to this stage, but do you
> really want to turn the following into a directory name?
> 
>     origin/master@{3 weeks ago}^{/Merge branch 'jk/}^2
> 
> I admit it's something of a contrived example but I hope it illustrates
> my point that sometimes naming the directory after the ref specifier may
> be less useful than just using "left" or the SHA1.

Actually, I think I was wrong here it's probably easier to just do
something you already do, but with a whitelist, like this:

	my $leftname, $rightname;
	// figure out what refs these point at...
	$leftname =~ s/[^a-zA-Z0-9]/_/g;
	$rightname =~ s/[^a-zA-Z0-9]/_/g;

We probably want to whitelist some more characters there, but that seems
like the simplest way to tackle it.  And if someone puts in a long
revision then they get a long directory name (or we truncate it at some
point).

> [1] http://support.microsoft.com/kb/177506