Re: [PATCHv6 4/5] gitweb: parse parent..current syntax from PATH_INFO
- From
Jakub Narebski <jnareb@gmail.com>
- Date
- Oct 20, 2008, 09:18 UTC
- Message-ID
- <200810201118.44654.jnareb@gmail.com>
- In-Reply-To
- <1224188831-17767-5-git-send-email-giuseppe.bilotta@gmail.com>
On Thu, 16 Oct 2008, Giuseppe Bilotta wrote:
Show 5 quoted lines
> 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'?
> > 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'.
> > Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
For what is worth:
Acked-by: Jakub Narebski <jnareb@gmail.com>
Show 15 quoted lines
> ---
> gitweb/gitweb.perl | 36 ++++++++++++++++++++++++++++++++++--
> 1 files changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 49730f3..1a7b0b9 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -548,7 +548,12 @@ sub evaluate_path_info {
> 'history',
> );
>
> - my ($refname, $pathname) = split(/:/, $path_info, 2);
> + # horrible regexp to catch
> + # [$hash_parent_base[:$file_parent]..]$hash_parent[:$file_name]This comment is really nice here, to explain what we try to catch. Is it really "horrible"... let others decide.
Note: this (as also previous code) makes use of the fact that refname
cannot contain ':' as per git-check-ref-format(1), and the fact that
gitweb limits $hash* parameters to simple revision syntax, allowing
only SHA1 and refnames, and not allowing (see validate_refname() used
in $hash* validation) extended SHA1 syntax like <commit-ish>:<path>
for $hash ('h') param for 'blob' view: gitweb uses $hash_base and
$file_name for that.(But I guess it is too long explanation to put it in comment)
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).
Show 19 quoted lines
> + my ($parentrefname, $parentpathname, $refname, $pathname) =
> + ($path_info =~ /^(?:(.+?)(?::(.+))?\.\.)?(.+?)(?::(.+))?$/);
> +
> + # first, analyze the 'current' part
> if (defined $pathname) {
> # we got "branch:filename" or "branch:dir/"
> # we could use git_get_type(branch:pathname), but it needs $git_dir
> @@ -557,7 +562,13 @@ sub evaluate_path_info {
> $input_params{'action'} ||= "tree";
> $pathname =~ s,/$,,;
> } else {
> - $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...
Show 15 quoted lines
> }
> $input_params{'hash_base'} ||= $refname;
> $input_params{'file_name'} ||= $pathname;
> @@ -577,6 +588,27 @@ sub evaluate_path_info {
> $input_params{'hash'} ||= $refname;
> }
> }
> +
> + # 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?
Show 6 quoted lines
> + $input_params{'file_parent'} ||= $parentpathname;
> + } else {
> + $input_params{'file_parent'} ||= $input_params{'file_name'};
> + }
> + # we assume that hash_parent_base is wanted if a path was specified,
> + # or if the action wants hash_base instead of hashNice comment.
> + if (defined $input_params{'file_parent'} ||
> + grep {$input_params{'action'} eq $_} @wants_base) {My preference of style would be to use here:
+ grep { $input_params{'action'} eq $_ } @wants_base) {Show 12 quoted lines
> + $input_params{'hash_parent_base'} ||= $parentrefname;
> + } else {
> + $input_params{'hash_parent'} ||= $parentrefname;
> + }
> + }
> }
> evaluate_path_info();
>
> --
> 1.5.6.5
>
> -- Jakub Narebski Poland