Re: [PATCHv6 4/5] gitweb: parse parent..current syntax from PATH_INFO

["Giuseppe Bilotta" <giuseppe.bilotta@gmail.com>]

On Mon, Oct 20, 2008 at 11:18 AM, Jakub Narebski <jnareb@gmail.com> wrote:
> On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:
>
>> This patch makes it possible to use an URL such as
>> $project/somebranch..otherbranch:/filename to get a diff between
>> different version of a file. Paths like
>> $project/$action/somebranch:/somefile..otherbranch:/otherfile are parsed
>> as well.
>
> Just a nitpick: why '$project' and '$action', but 'somebranch',
> 'otherbranch' and 'filename'?

Good question ... I think I got distracted along the way.

>> All '*diff' actions and in general actions that use $hash_parent[_base]
>> and $file_parent can now get all of their parameters from PATH_INFO
>
> Which currently mean 'shortlog', and I guess in the future would mean
> also all other log-like views: 'log', 'history', 'search' (the commit
> message kind, not the pickaxe kind), and perhaps also 'rss'/'atom'.

I'm not sure rss/atom makes sense, but the others were already in my
todo list after the shortlog patch, so I'll try to get them ready as
soon as I have the time to refactor them as we discussed on IRC.

> Side note: the regexp below allow for $parentpathname to contain
> '..', but we don't want to rely on such minute detail of implementation
> detail (because it depends on whether we use greedy or non-greedy
> matching there).
>
>> +     my ($parentrefname, $parentpathname, $refname, $pathname) =
>> +             ($path_info =~ /^(?:(.+?)(?::(.+))?\.\.)?(.+?)(?::(.+))?$/);

Hm, actually it might be a better idea to make the first pathname
match non-greedy.

>> -                     $input_params{'action'} ||= "blob_plain";
>> +                     # the default action depends on whether we had parent info
>> +                     # or not
>> +                     if ($parentrefname) {
>> +                             $input_params{'action'} ||= "blobdiff_plain";
>> +                     } else {
>> +                             $input_params{'action'} ||= "blob_plain";
>> +                     }
>
> Nice.
>
> I was wondering about 'project/hash_parent..hash' syntax, but then I have
> realized that it doesn't change action (as in 'blob_plain' -> 'blobdiff_plain'),
> but is always 'shortlog'.
>
> By the way, I wonder if it should be 'blobdiff_plain' or just 'blobdiff'.
> the 'blob_plain' was here to use gitweb as a kind of versioned web
> server; there is no such equivalent for 'p/hbp..hb:f' syntax. On the
> other hand it is consistent behavior, always using *_plain...

Moreover, it allows sending shorter URLs for patches, which are the
ones you usually write by hand.

>> +
>> +     # next, handle the 'parent' part, if present
>> +     if (defined $parentrefname) {
>> +             # a missing pathspec defaults to the 'current' filename, allowing e.g.
>> +             # someproject/blobdiff/oldrev..newrev:/filename
>> +             if ($parentpathname) {
>> +                     $parentpathname =~ s,^/+,,;
>> +                     $parentpathname =~ s,/$,,;
>
> Hmmm... should we strip trailing '/' here?

I must confess I don't remember why I decided that was needed.

-- 
Giuseppe "Oblomov" Bilotta