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

[Jakub Narebski <jnareb@gmail.com>]

Giuseppe Bilotta wrote:
> On Sat, Oct 4, 2008 at 3:31 AM, Jakub Narebski <jnareb@gmail.com> wrote:
>> 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.

I have thought about this rather as supplement (addition) to the
current commit message (which states explicitly new form of supported
path_info URL), than replacing it.
 
>> 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).

No, not supporting this form is just fine.
 
>>> +             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?

I think you should remove it here too.  IMHO if needed, it should be
dealt with (and I think is dealt with) in appropriate action subroutine.

-- 
Jakub Narebski
Poland