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'?
>
> 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@xxxxxxxxx>
For what is worth:
Acked-by: Jakub Narebski <jnareb@xxxxxxxxx>
> ---
> 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).
> + 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...
> }
> $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?
> + $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 hash
Nice 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) {
> + $input_params{'hash_parent_base'} ||= $parentrefname;
> + } else {
> + $input_params{'hash_parent'} ||= $parentrefname;
> + }
> + }
> }
> evaluate_path_info();
>
> --
> 1.5.6.5
>
>
--
Jakub Narebski
Poland
--
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
