On Mon, Oct 20, 2008 at 11:18 AM, Jakub Narebski <jnareb@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
