Re: Supporting more gitweb diff possibities

[Jakub Narebski <jnareb@gmail.com>]

On Sun, Mar 25, 2007, Martin Koegler wrote:

> I have done a first series of 6 patches, which improves blobdiff and
> adds treediff.

Just a few nits.

First, I think it would be better to have individual patches to be
replies to this cover letter, either directly (all patch mails
are replies to cover letter) or indirectly (i.e. chained, the later
patch is reply to earlier patch, first patch ir reply to cover letter).

Second, it is a good idea to _number_ patches in series, i.e. have
  "[PATCH 0/6] Supporting more gitweb diff possibities"
as cover letter subject, and number patches respectively. The option
`-n' of git-format-patch (together with --start-number if you make
patches one by one) takes care of that.

> [PATCH] gitweb: show no difference message

This should be I think: 
  "[PATCH 1/6] gitweb: show "no difference" message for empty diff"

> This patch shows an "no difference" message instead of nothing for
> equal objects.

This is a good patch, worth doing even without rest of the patches.

Currently we have only one place (I think) where gitweb can generate
link to "blobdiff", namely "diff to parent" link in "history" view
for plain file, when e.g. some change was (explicitely or accidentally)
reverted.

The few comments about style (variable naming, CSS style for 
"no differences" message) are in the reply to the patch.

> [PATCH] gitweb: Support comparing blobs with different names
> 
> This patch adds support for comparing objects with different file
> names using hb/hbp.

Good idea; I have replied with an alternate solution, involving adding
'fp' to git-diff-tree path limit instead of git-diff with hpb:fp.

I'm sorry for the confusing advice wrt. git_blobdiff and rename diffs.

> [PATCH] gitweb: link base commit (hpb) to blobdiff output
> 
> Add link the parent commit, as there is currently no such link.

I'm rather ambivalent about this patch. Perhaps there is currently no such
link, but you can always click on one of the links to parent blob, and
from blob view to commit, commitdiff or tree.

By the way, the "(from: _commitdiff_)" link in "commitdiff" view
(and similar one in "commit" view) is part of the "next link" family
of "(parent: _commitdiff_)" and "(merge: _commitdiff_ _commitdiff_)"
which allow to go down the line of parents without need to change
views (or go to the 'parent' header in the case of "commit" view),
just like you would press PageDown during "git log" or "git log -p"
invocation from the command line. "(from: _commitdiff_)" was added
only for completeness (and because it was easy to do).

> [PATCH] gitweb: support filename prefix in git_patchset_body
> [PATCH] gitweb: support filename prefix in git_difftree_body

I'm not sure if those patches should or should not be concatenated
together in one commit.

See further comments on style (variable naming) and alternate solution
in the comments to first patch of those two: they refer to both patches.

> [PATCH] gitweb: Add treediff

Shouldn't it be:
  "[PATCH 6/6] gitweb: Add "treediff" and "treediff_plain" views"

> These 3 patches add the treediff method. Its a complete reworked
> verion. As git-diff-tree outputs relative patches (discards part of the
> compared tree objects), the first two patches are necessary to produce
> correct links in the treediff output.

See comments for that patch.

> I do not see many possibilties for code sharing with git_commitdiff:
> The only large portion of common code is calling git-diff-tree. I
> don't think that this would justify the more complex code.

I was not thinking about using git_commitdiff to generate "treediff"
view, but rather about extracting the common code into separate subroutine.
But it might (or might not) be not worth the hassle. And it can always
be done in separate "refactoring" patch.

-- 
Jakub Narebski
Poland