I'm sorry for the delay in reviewing this patch...
On Mon, 27 Oct 2008, Huy Blucher wrote:
[Please try do not lose attributions...]
> > >

quoted text
> > > 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...
>

quoted text
> 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: which mimics current use of

"git-rev-list " 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.
>

quoted text
> 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.
>

quoted text
> 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...
>

quoted text
> ---

Diffstat?
>

quoted text
> --- 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;

quoted text
>

>         $maxcount ||= 1;

>         $skip ||= 0;

>

>         local $/ = "__PLACEHOLDER__6_";

> +        # 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= --follow  -- 
doesn't work as expected? Hmmm... true, it doesn't work.

Then it is certainly a _BUG_ in git!
>

quoted text
> -       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),

quoted text
>                 @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 . '__PLACEHOLDER__8_');

Doesn't it work if you don't add the above line?
> +                # Need to strip the word commit from the start so it

quoted text
> +                # 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;

quoted text
>         }

> @@ -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) {

quoted text
>                 $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

quoted text
> 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 .= " ⋅ " .

quoted text
> @@ -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

--

To unsubscribe from this list: send the line "unsubscribe git" in

the body of a message to majordomo@vger.kernel.org

More majordomo info at  http://vger.kernel.org/majordomo-info.html