Re: [RFC] More diff possibilities in gitweb

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 18, 2007, 23:20 UTC
Message-ID: <200703190020.55806.jnareb@gmail.com>
In-Reply-To: <20070318141451.GA9607@auto.tuwien.ac.at>

Some of discussion is repeated in another subthread

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

Show 22 quoted lines

> On Sat, Mar 17, 2007 at 07:04:54PM +0100, Johannes Schindelin wrote:
>> On Sat, 17 Mar 2007, Martin Koegler wrote:
>> 
>>> The whole is implemented in JavaScript on the client side (tested with 
>>> IE 6 and Mozilla).
>> 
>> This is not acceptable for me. There are too many people blindly running 
>> JavaScript everywhere, and we should not enforce such bad behaviour.
>>
>> At least, you need a way to do it with plain old HTML (which is easy 
>> enough).
> 
> If the browser supports no JavaScript, the user can use all current
> features, except the new diff capabilities.
>
> My first thoughts where also about implementing it inside
> gitweb.cgi. This would mean a large change to the code, as all code
> which generate links needs to be adapted.
>
> Additionally, selecting a base object would mean, that you must submit
> a request to the server and probably return something (probable the
> same page, on which you selected the base object).

First, generating gitweb links in gitweb _all_ goes through href() subroutine, so it would be fairly easy to implement it server side, i.e. directly in gitweb.perl. One solution would be to set cookies for parent (base) of diff server side. Another would be to pass through diff parent (diff base) options (CGI parameters) in non-diff views. It means that for example shortlog, or history, or summary view would get 'hp', 'hpb', 'fp' parameters and pass it through to "diff to selected base" links. But it is a fact that it would mean additional request to the server to set the base object.

The correct solution I think which would satisfy everyone would be to add plain HTML, i.e. server-side solution to "diff with selected base" if this option is turned on. Then JavaScript would replace it by JavaScript solution (or would add JavaScript solution if server-side solution is turned off), again subject to gitweb configuration. JavaScript would use cookies if possible, and changing location if not (that is for discussion).

> For me, this would be no problem, but for high load servers (eg. gitweb at
> kernel.org), it would incread the system load.

I think use JavaScript if possible, and server-side implementation
if not would be good compromise.

> With JavaScript, this step needs no server interaction. Infact, a
> client could add the diff feature to any gitweb server with
> eg. greasemonkey, by injecting the JavaScript code.

Well, we could always put this in contrib/ as Greasmonkey script ;-)

Show 9 quoted lines

>> Also I'd like to know: is there any reason you did not send a proper diff, 
>> given that you are so interested in diffs?
> 
> I'm not interessted in generating patchs via gitweb. I want to examine
> and review differences between various branches/tags/commits/... .
> 
> The "pseudo" diff for gitweb.cgi was to illustrate, how to integrate
> gitweb.js. The whole thing is under development, so I have not created
> a clean version.

I'm just wondering why did you not used git for development. You have
to have git installed to test gitweb, and it would be best to have
git.git repository fetched to base your work on newest 'master' version
(or rather 'origin/master' or 'origin' version). So it would be natural
to use git to work on gitweb, and to send git patches.

> As I wrote in my last mail, I used blobdiff to generation tree diffs,
> which results in wrong/missing file names. In the mean time, I've
> create a first version of a treediff function for gitweb.

Which is IMVHO long way from ready.

> --- old/gitweb.js   2007-03-17 15:18:23.284317140 +0100
> +++ gitweb.js        2007-03-17 22:25:18.254190096 +0100

First, if you don't use git to generate diffs, could you _please_ use equal depth paths patches, i.e. either

 --- gitweb.js.orig   2007-03-17 15:18:23.284317140 +0100
 +++ gitweb.js        2007-03-17 22:25:18.254190096 +0100

or

 --- old/gitweb.js   2007-03-17 15:18:23.284317140 +0100
 +++ new/gitweb.js   2007-03-17 22:25:18.254190096 +0100

> --- ../mirror/git.git/gitweb/gitweb.perl	2007-03-12 22:06:44.000000000 +0100
> +++ gitweb.cgi	2007-03-17 18:41:50.000000000 +0100

The same as above (or even worse)

Show 8 quoted lines

> @@ -1835,6 +1837,7 @@
>  		close $fd;
>  	}
>  
> +	print '<script type="text/javascript" src="gitweb.js"></script>';
>  	print "</body>\n" .
>  	      "</html>";
>  }

I'd rather have end of line after closing '</script>', and have scripts in the head section, not in body.

Show 6 quoted lines

> @@ -4191,6 +4194,106 @@
>  	git_commitdiff('plain');
>  }
>  
> +sub git_treediff {
> +	my $format = shift || 'html';

[...]

You base git_treediff on git_commitdiff. On one hand it is right, as treediff is like commmitdiff, but without commit message, nor link to commit parents.

On the other hand you should be able to handle 'hb', 'hpb', 'f', 'fp' parameters like git_blobdiff does. And making git_difftree work between two different trees from two different commits (e.g. git-gui/ in git.git mainline and / in git-gui.git mainline) is not that easy...

-- 
Jakub Narebski
Poland