Re: [RFC] gitweb: add 'historyfollow' view that follows renames

[Jakub Narebski <jnareb@gmail.com>]

I'm sorry for the delay in reviewing this patch...

On Mon, 27 Oct 2008, Huy Blucher wrote:

[Please try do not lose attributions...]

> > > 
> > > What should we add to automatically get all file history?
> 
> > While the 'commitdiff' view would, in default gitweb configuration, 
> > contain information about file renames, currently 'history' view does 
> > not support '--follow' option to git-log.  It wouldn't be too hard to 
> > add it, but it just wasn't done (well, add to this the fact that 
> > --follow works only for simple cases).
> 
> We also ran up against this issue because renaming of files in our
> project is a useful bit of information while browsing file history.
> 
> I hacked gitweb to add a 'historyfollow' viewing option in addition to
> the 'history' option.  Yes, '--follow' is expensive so I didn't just
> make it the default 'history' behaviour.

I would prefer if instead of adding new 'historyfollow' action, which
goes against stated in TODO goal of uniquifying log-like views handling,
the patch added support for '--follow' as extra option to 'history'
view (i.e. a=history;opt=--follow)... on the other hand 'historyfollow'
(or simply 'follow') can be used in pure path_info gitweb URL... Hmm...

> 
> The only problem with doing it was that parse_commits in gitweb.perl
> uses git rev-list which doesn't support the '--follow' option like
> git-log does. A bit of hacking to use 'git log --pretty=raw -z' to make
> the output look close to that from rev-list means only minor
> shoe-horning is required (perhaps it would be better to make rev-list
> understand --follow though?).

Either that, or use --pretty=format:<sth> which mimics current use of
"git-rev-list <opts>" output in parse_commits exactly.

And either move parse_commits to use git-log, change git-rev-parse to
understand '--follow', or make parse_commits use git-log with --follow,
git-rev-list otherwise.

> 
> I wasn't originally prepared to publish the work here because I don't
> really think it's the best solution. But considering it's come up... I
> include a patch inline against gitweb.perl from v1.6.0.3 that implements
> a 'historyfollow' view.

RFC (usually marked [RFC/PATCH]) patches are good because they allow
others to test and comment on your solution: early review, better to
spot bugs etc. earlier.

> 
> Feel free to do with it what you will. It would need some substantial
> tidying up by someone who knows much more about perl than me :) 
> 
> We use it such that anywhere a 'history' link is provided a
> 'historyfollow' link is also provided next to it - This patch doesn't
> implement that bit though.
> 
> Hope it helps.
> 
> Cheers,
> Guy.

It would be nice though if such [RFC/PATCH] followed guidelines from
SubmittingPatches on commit messages...

> 
> ---

Diffstat?

> 
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -478,6 +478,7 @@ my %actions = (
>         "forks" => \&git_forks,
>         "heads" => \&git_heads,
>         "history" => \&git_history,
> +       "historyfollow" => \&git_history_follow,
>         "log" => \&git_log,
>         "rss" => \&git_rss,
>         "atom" => \&git_atom,
> @@ -2311,25 +2312,39 @@ sub parse_commit {
>  }
>  
>  sub parse_commits {
> -       my ($commit_id, $maxcount, $skip, $filename, @args) = @_;
> +       my ($commit_id, $maxcount, $skip, $mode, $filename, @args) = @_;

Can't you simply pass '--follow' or 'follow' in @args instead of
changing the signature of parse_commits?

>         my @cos;
>  
>         $maxcount ||= 1;
>         $skip ||= 0;
>  
>         local $/ = "\0";
> +        # The '--max-count' argument is not available when doing a
> +        # '--follow' to 'git log'
> +        my $count_arg = ("--max-count=" . $maxcount) ;
> +        if (defined $mode && $mode eq "--follow") {
> +            $count_arg = "--follow" ;
> +        }

I don't understand. Do you mean that

  $ git log --max-count=<n> --follow <rev> -- <path>

doesn't work as expected? Hmmm... true, it doesn't work. 
Then it is certainly a _BUG_ in git!

>  
> -       open my $fd, "-|", git_cmd(), "rev-list",
> -               "--header",
> +
> +       open my $fd, "-|", git_cmd(), "log",
> +               "-z",
> +               "--pretty=raw",
>                 @args,
> -               ("--max-count=" . $maxcount),
> +                ($count_arg ? ($count_arg ) : ()),

Whitespace damage (here visible).

>                 ("--skip=" . $skip),
>                 @extra_options,
>                 $commit_id,
>                 "--",
>                 ($filename ? ($filename) : ())
> -               or die_error(500, "Open git-rev-list failed");
> +               or die_error(500, "Open git-log failed");
>         while (my $line = <$fd>) {
> +               # Need to put a delimiter on the end of output
> +                # 'git-log -z' doesn't put one before EOF like rev-list
> does
> +                $line = ($line . '\0');

Doesn't it work if you don't add the above line?

> +                # Need to strip the word commit from the start so it
> +                # looks like rev-list output
> +                $line =~ s/^commit // ;
>                 my %co = parse_commit_text($line);

Perhaps we should update parse_commit_text instead... or add an option
like parse_commit_text($line, -format=>'log'), or something like that.

>                 push @cos, \%co;
>         }
> @@ -5363,6 +5378,13 @@ sub git_commitdiff_plain {
>  }
>  
>  sub git_history {
> +        my $mode = shift || '';
> +        my $history_call = "history";
> +
> +       if ($mode eq "--follow") {
> +           $history_call .= "historyfollow" ;
> +       }
> +

I don't quite like this solution...

If '--follow' was passed through @extra_options ('opt') parameter, then
it should be re-used in links thanks to href(..., -replay=>1).

>         if (!defined $hash_base) {
>                 $hash_base = git_get_head_hash($project);
>         }
> @@ -5377,7 +5399,7 @@ sub git_history {
>         my $limit = sprintf("--max-count=%i", (100 * ($page+1)));
>  
>         my @commitlist = parse_commits($hash_base, 101, (100 * $page),
> -                                      $file_name, "--full-history")
> +                                      $mode, $file_name,
> "--full-history")

Word wrapped (not only here).

>             or die_error(404, "No such file or directory on given
> branch");
>  
>         if (!defined $hash && defined $file_name) {
> @@ -5398,7 +5420,7 @@ sub git_history {
>         my $paging_nav = '';
>         if ($page > 0) {
>                 $paging_nav .=
> -                       $cgi->a({-href => href(action=>"history",
> hash=>$hash, hash_base=>$hash_base,
> +                       $cgi->a({-href => href(action=>"$history_call",
> hash=>$hash, hash_base=>$hash_base,
>                                                file_name=>$file_name)},
>                                 "first");

I would add opts=>[@extra_options] instead of changing action=>"history"
to action=>$history_call (the quotes around variable are not needed).

>                 $paging_nav .= " &sdot; " .
> @@ -5429,6 +5451,11 @@ sub git_history {
>         git_footer_html();
>  }
>  
> +sub git_history_follow {
> +       git_history('--follow');
> +}
> +
> +
>  sub git_search {
>         gitweb_check_feature('search') or die_error(403, "Search is
> disabled");
>         if (!defined $searchtext) {
> @@ -5469,7 +5496,7 @@ sub git_search {
>                         $greptype = "--committer=";
>                 }
>                 $greptype .= $searchtext;
> -               my @commitlist = parse_commits($hash, 101, (100 *
> $page), undef,
> +               my @commitlist = parse_commits($hash, 101, (100 *
> $page), undef, undef,
>                                                $greptype,
> '--regexp-ignore-case',
>                                                $search_use_regexp ?
> '--extended-regexp' : '--fixed-strings');
>  
> @@ -5737,7 +5764,7 @@ sub git_feed {
>  
>         # log/feed of current (HEAD) branch, log of given branch,
> history of file/directory
>         my $head = $hash || 'HEAD';
> -       my @commitlist = parse_commits($head, 150, 0, $file_name);
> +       my @commitlist = parse_commits($head, 150, 0, undef,
> $file_name);
>  
>         my %latest_commit;
>         my %latest_date;
> ---

What I would like to see is the link in the bottom of action bar
(navbar) or just below it, which would list 'follow' as one of possible
'history' view formats (just like '--no-merges' or '--first-parent'
should be).

-- 
Jakub Narebski
Poland