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

From: John Keeping <john@keeping.me.uk>
Date: Mar 28, 2013, 14:19 UTC
Message-ID: <20130328141944.GW2286@serenity.lan>
In-Reply-To: <1364474776.11891.9.camel@heisenberg.scientia.net>

On Thu, Mar 28, 2013 at 01:46:16PM +0100, Christoph Anton Mitterer wrote:

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 15 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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

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

John