Re: [PATCHv4] gitweb: parse parent..current syntax from pathinfo

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Oct 4, 2008, 07:24 UTC
Message-ID: <cb7bb73a0810040024q4dfad117uf719f0aaa17ae95b@mail.gmail.com>
In-Reply-To: <200810040331.27605.jnareb@gmail.com>

On Sat, Oct 4, 2008 at 3:31 AM, Jakub Narebski <jnareb@gmail.com> wrote:

Show 12 quoted lines

> On Thu, 2 Oct 2008, Giuseppe Bilotta wrote:
>
>> This 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.
>>
>
> In short, it allows to have link to '*diff' views using path_info URL,
> or in general to pass $hash_[parent_]base and $file_parent using
> path_info.

Yes, that's probably a better form for the commit message.

Show 22 quoted lines

>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
>> ---
>>  gitweb/gitweb.perl |   26 ++++++++++++++++++++++++--
>>  1 files changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>> index 3e5b2b7..89e360f 100755
>> --- a/gitweb/gitweb.perl
>> +++ b/gitweb/gitweb.perl
>> @@ -534,7 +534,9 @@ if ($path_info && !defined $action) {
>>
>>  # we can now parse ref and pathnames in PATH_INFO
>>  if ($path_info) {
>> -     my ($refname, $pathname) = split(/:/, $path_info, 2);
>> +     $path_info =~ /^((.+?)(:(.+))?\.\.)?(.+?)(:(.+))?$/;
>> +     my ($parentrefname, $parentpathname, $refname, $pathname) = (
>> +             $2, $4, $5, $7);
>
> Style: I would use (but that is perhaps matter of taste)
>
> +       my ($parentrefname, $parentpathname, $refname, $pathname) =
> +               ($2, $4, $5, $7);

Right, I'm not sure why I put the ( on the previous line.

> Also it would be I think simpler to use instead non-catching grouping,
> i.e. (?: xxx ) extended pattern (see perlre(1)), and use
> ($1, $2, $3, $4), or even simpler  'list = (string =~ regexp)'  form.

Good idea, I'll rework it in that sense.

Show 8 quoted lines

> I also think that the situation is more complicated than that, if we
> want to be more correct.
>
> The following path_info layouts with '..' make sense:
>
>  hpb:fp..hb:f
>  hpb..hb:f     == hpb:f..hb:f
>  hp..h

And these are matched by the above regexp

> And the layout below can be though to make sense, but it is just
> plain weird.
>
>  hpb:fp..f     == hpb:fp..HEAD:f

I'm afraid I'm not going to support that, although it's probably easy to support hpb:fp..:f (i.e. accept a missing refname but on condition of having a : in front of the file spec).

>> +             if (defined $input_params{'file_parent'}) {
>> +                     $input_params{'hash_parent'} ||= git_get_hash_by_path($input_params{'hash_parent_base'}, $input_params{'file_parent'});
>
> This line is bit long, and I think it should be wrapped..

By the way, on the first revision of the path_info patchset, you had me discard

$hash      ||= git_get_hash_by_path($hash_base, $file_name);

in the simple case on the basis that it was an extra call to external git.

I actually forgot to remove it from this part of the patchset too at the time, so this gets me wondering about this: should I put it back in place in the simple case, or remove it from here too?

-- 
Giuseppe "Oblomov" Bilotta