[RFC] More diff possibilities in gitweb

[RFC] More diff possibilities in gitweb

[Martin Koegler]

Compared to eg. viewcvs, gitweb offers fewer possibilies to do a diff.
With viewcvs, you can compare arbitrary version of a file; gitweb only
offers diff to previous (or sometimes current) version.

As git offers an infinite number of possible (and maybe useful) diffs,
I added to each each compareable object two links:
* "base": Select this object as parent
* "diff": Diff against the last select parent

The selected parent object is stored as a cookie, so it is possible to
do a diff between objects shown on different pages.

The whole is implemented in JavaScript on the client side (tested with
IE 6 and Mozilla). On the server side, only the JavaScript file is
included in the output (see below). The link generation is done
completely in the browser.

The patch offers the following possibilies:
* Compare any commit with an other commit
  Passes the hashes of the two commits as h and hp to commitdiff
  function. The output includes the message of one commit, which can
  be confusing.
* Compare any blob with an other blob
  This feature is based on the blobdiff function. There are two types
  of blob:
  - blobs in the tree view can be compared with any blob in the tree view.
  - blobs in the history view can only be compared with any blob with
    the same file name in the history view.
* Compare trees
  gitweb currently has no function for comparing trees. I currently
  misused blobdiff for this. The content of the diff is OK, only the
  filenames are wrong/destroyed. Additionally gitweb issus some "Use
  of uninitialized value" message to the error log.

  A proper implementation would need new function in gitweb.cgi,
  basically git_commitdiff removing the requirement, that the hashes
  must be a commit.

Is there an interest these function?

mfg Martin Kögler

Patch for gitweb.cgi (in function git_footer_html):
---------------------------------------------------
 
+        print '<script type="text/javascript" src="gitweb.js"></script>';
         print "</body>\n" .
               "</html>";

File gitweb.js:
---------------
function getCookie(name)
{
  var name=name+"=";
  var c=document.cookie;
  var p=c.indexOf(name);
  if(p==-1)
    return null;
  c=c.substr(p+name.length,c.length);
  p=c.indexOf(";");
  if(p==-1)
    return c;
  else
    return c.substr(0, p);
}

function insertAfter(elem, node)
{
  if(node.nextSibling)
    node.parentNode.insertBefore(elem, node.nextSibling);
  else
    node.parentNode.appendChild(elem);
}

function createLink(href, linktext)
{
  var l=document.createElement("a");
  l.appendChild(document.createTextNode(linktext));
  l.href=href;
  return l;
}

function createLinkGroup(href1, basetxt, href2, difftxt)
{
  var l=document.createElement("span");
  l.appendChild(document.createTextNode(" ("));
  l.appendChild(createLink(href1, basetxt));
  l.appendChild(document.createTextNode(" | "));
  l.appendChild(createLink(href2, difftxt));
  l.appendChild(document.createTextNode(") "));
  return l;
}

function GitRef()
{
    this.t=null;
    this.h=null;
    this.hb=null;
    this.f=null;
    this.ToRef=ToRef;
}

function ToRef()
{
  var parts=new Array();
  if(this.f)
    parts.push("f="+this.f);
  if(this.h)
    parts.push("h="+this.h);
  if(this.hb)
    parts.push("hb="+this.hb);
  if(this.t)
    parts.push("t="+this.t);
  return parts.join("@");
}

function splitGitRef(ref)
{
  var parts=ref.split("@");
  var res=new GitRef();
  var i;
  for(i=0;i<parts.length;i++)
  {
      var p=parts[i].split("=");
      res[p[0]]=p[1];
  }
  return res;
}

function GitURL(base)
{
  this.base=base;
  this.p=null;
  this.a=null;
  this.f=null;
  this.fp=null;
  this.h=null;
  this.hp=null;
  this.hb=null;
  this.hpb=null;
  this.pg=null;
  this.o=null;
  this.s=null;
  this.st=null;
  this.ToURL=ToURL;
  this.ToRef=UrlToRef;
  this.ToDUrl=ToDUrl;
}

function ToURL()
{
  var parts=new Array();
  if(this.p)
    parts.push("p="+this.p);
  if(this.a)
    parts.push("a="+this.a);
  if(this.f)
    parts.push("f="+this.f);
  if(this.fp)
    parts.push("fp="+this.fp);
  if(this.h)
    parts.push("h="+this.h);
  if(this.hp)
    parts.push("hp="+this.hp);
  if(this.hb)
    parts.push("hb="+this.hb);
  if(this.hpb)
    parts.push("hpb="+this.hpb);
  if(this.o)
    parts.push("o="+this.o);
  if(this.s)
    parts.push("s="+this.s);
  if(this.st)
    parts.push("st="+this.st);
  return this.base+"?"+parts.join(";");
}

function UrlToRef(type)
{
    var res=new GitRef;
    res.f=this.f;
    res.h=this.h;
    res.hb=this.hb;
    res.t=type;
    return res.ToRef();
}

function ToDUrl(type)
{
    var res=new GitURL(this.base);
    res.f=this.f;
    res.h=this.h;
    res.hb=this.hb;
    res.p=this.p;
    res.a=type;
    return res.ToURL();
}

function splitGitURL(url)
{
  var Urls=url.split("?");
  var res=new GitURL(Urls[0]);
  if(Urls.length>1)
    {
      var parts=Urls[1].split(";");
      var i;
      for(i=0;i<parts.length;i++)
	{
	  var p=parts[i].split("=");
	  res[p[0]]=p[1];
	}
    }
  return res;
}

function GitAddLinks()
{
  var links=document.getElementsByTagName("a");
  var i;
  for(i=0;i<links.length;i++)
    {
      var link=links[i];
      if(link.innerHTML=='commit')
	{
	  var url=splitGitURL(link.href);
	  if(!url.h)
	    continue;
	  var l=createLinkGroup("javascript:base('"+url.ToRef('commit')+"')","base",
				"javascript:diff('"+url.ToDUrl('commit')+"')","diff");
	  insertAfter(l, link);
	}
      if(link.innerHTML=='blob')
	{
	  var url=splitGitURL(link.href);
	  if(!url.h&&!(url.hb&&url.f))
	    continue;
	  var l=createLinkGroup("javascript:base('"+url.ToRef('blob')+"')","base",
				"javascript:diff('"+url.ToDUrl('blob')+"')","diff");
	  insertAfter(l, link);
	}
      if(link.innerHTML=='tree')
	{
	  var url=splitGitURL(link.href);
	  if(!url.h)
	    continue;
	  var l=createLinkGroup("javascript:base('"+url.ToRef('tree')+"')","base",
				"javascript:diff('"+url.ToDUrl('tree')+"')","diff");
	  insertAfter(l, link);
	}
    }
}

function base(ref)
{
  document.cookie="basename="+ref;
}

function diff(url)
{
  var c=getCookie("basename");
  if (!c)
  {
      alert("no diff base selected");
      return;
  }
  c=splitGitRef(c);
  url=splitGitURL(url);

  if(c.t=='commit'&&url.a=='commit')
  {
      url.a='commitdiff';
      if(!c.h||!url.h)
      {
	  alert("commit diff not possible");
	  return;
      }
      url.hb=null;
      url.f=null;
      url.hp=c.h;
      document.location.href=url.ToURL();
      return;
  }
  if(c.t=='blob'&&url.a=='blob')
  {
      url.a='blobdiff';
      if(c.h&&url.h)
      {
	  url.hb=null;
	  url.hp=c.h;
	  url.fp=c.f;
      } 
      else if (c.hb&&url.hb&&url.f&&c.f)
      {
	  if (url.f!=c.f)
	  {
	      alert("file name do not match");
	      return;
	  }
	  url.h=null;
	  url.hpb=c.hb;
	  url.fp=c.f;
      }
      else
      {
	  alert("blob diff not possible");
	  return;
      }
      document.location.href=url.ToURL();
      return;
  }
  if(c.t=='tree'&&url.a=='tree')
  {
      url.a='blobdiff';
      if(c.h&&url.h)
      {
	  url.hb=null;
	  url.hp=c.h;
	  url.fp=c.f;
	  document.location.href=url.ToURL();
	  return;
      } 
  }
  alert("diff not possible");
}

GitAddLinks();

Re: [RFC] More diff possibilities in gitweb

[Johannes Schindelin]

Hi,

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).

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?

Ciao,
Dscho

Re: [RFC] More diff possibilities in gitweb

[Jakub Narebski]

[Cc: Martin Koegler <mkoegler@auto.tuwien.ac.at>, git@vger.kernel.org]

Martin Koegler wrote:

> The whole is implemented in JavaScript on the client side (tested with
> IE 6 and Mozilla). On the server side, only the JavaScript file is
> included in the output (see below). 

I'd rather not use JavaScript for that, but perhaps add in the history
view (shortlog, history) selectbox to select commits (for shortlog, and
history of a directory) or versions of a blob/file (history of a file)
like in an "info" view e.g. in MoinMoin wiki:
  http://git.or.cz/gitwiki/GitSurvey?action=info
JavaScript can be used to beautify this like e.g. MediaWiki engine
does:
  http://en.wikipedia.org/w/index.php?title=Darcs&action=history 
(to allow selection of diff only in forward direction).

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [RFC] More diff possibilities in gitweb

[Martin Koegler]

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).

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

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.

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

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.

mfg Martin Kögler

--- old/gitweb.js   2007-03-17 15:18:23.284317140 +0100
+++ gitweb.js        2007-03-17 22:25:18.254190096 +0100
@@ -259,7 +259,7 @@
   }
   if(c.t=='tree'&&url.a=='tree')
   {
-      url.a='blobdiff';
+      url.a='treediff';
       if(c.h&&url.h)
       {
          url.hb=null;
--- ../mirror/git.git/gitweb/gitweb.perl	2007-03-12 22:06:44.000000000 +0100
+++ gitweb.cgi	2007-03-17 18:41:50.000000000 +0100
@@ -446,6 +446,8 @@
 	"tag" => \&git_tag,
 	"tags" => \&git_tags,
 	"tree" => \&git_tree,
+	"treediff" => \&git_treediff,
+	"treediff_plain" => \&git_treediff_plain,
 	"snapshot" => \&git_snapshot,
 	"object" => \&git_object,
 	# those below don't need $project
@@ -1835,6 +1837,7 @@
 		close $fd;
 	}
 
+	print '<script type="text/javascript" src="gitweb.js"></script>';
 	print "</body>\n" .
 	      "</html>";
 }
@@ -4191,6 +4194,106 @@
 	git_commitdiff('plain');
 }
 
+sub git_treediff {
+	my $format = shift || 'html';
+	$hash ||= $hash_base;
+	$hash_parent ||= $hash_parent_base;
+	if (!defined $hash_parent || !defined $hash)
+	{
+	    die_error(undef, "Missing one of the tree diff parameters");
+	}
+
+	# we need to prepare $formats_nav before any parameter munging
+	my $formats_nav;
+	if ($format eq 'html') {
+		$formats_nav =
+			$cgi->a({-href => href(action=>"treediff_plain",
+			                       hash=>$hash, hash_parent=>$hash_parent)},
+			        "raw");
+		$formats_nav .= ' | ';
+		$formats_nav .=
+			$cgi->a({-href => href(action=>"tree",
+			                       hash=>$hash)},
+			        "tree");
+		$formats_nav .= ' &lt;-&gt; ';
+		$formats_nav .=
+			$cgi->a({-href => href(action=>"tree",
+			                       hash=>$hash)},
+			        "tree");
+	}
+
+	# read treediff
+	my $fd;
+	my @difftree;
+	if ($format eq 'html') {
+		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			"--no-commit-id", "--patch-with-raw", "--full-index",
+			$hash_parent, $hash, "--"
+			or die_error(undef, "Open git-diff-tree failed");
+
+		while (my $line = <$fd>) {
+			chomp $line;
+			# empty line ends raw part of diff-tree output
+			last unless $line;
+			push @difftree, $line;
+		}
+
+	} elsif ($format eq 'plain') {
+		open $fd, "-|", git_cmd(), "diff-tree", '-r', @diff_opts,
+			'-p', $hash_parent, $hash, "--"
+			or die_error(undef, "Open git-diff-tree failed");
+
+	} else {
+		die_error(undef, "Unknown treediff format");
+	}
+
+	# non-textual hash id's can be cached
+	my $expires;
+	if ($hash =~ m/^[0-9a-fA-F]{40}$/) {
+		$expires = "+1d";
+	}
+
+	# write diff message
+	if ($format eq 'html') {
+		git_header_html(undef, $expires);
+
+		print "<div class=\"page_nav\"><br/>$formats_nav<br/></div>\n";
+		print "<div class=\"title\">$hash vs $hash_parent</div>\n";
+		print "<div class=\"page_body\">\n";
+
+	} elsif ($format eq 'plain') {
+		my $filename = basename($project) . "-$hash-$hash_parent.patch";
+
+		print $cgi->header(
+			-type => 'text/plain',
+			-charset => 'utf-8',
+			-expires => $expires,
+			-content_disposition => 'inline; filename="' . "$filename" . '"');
+		print "X-Git-Url: " . $cgi->self_url() . "\n\n";
+	}
+
+	# write patch
+	if ($format eq 'html') {
+		git_difftree_body(\@difftree, $hash, $hash_parent);
+		print "<br/>\n";
+
+		git_patchset_body($fd, \@difftree, $hash, $hash_parent);
+		close $fd;
+		print "</div>\n"; # class="page_body"
+		git_footer_html();
+
+	} elsif ($format eq 'plain') {
+		local $/ = undef;
+		print <$fd>;
+		close $fd
+			or print "Reading git-diff-tree failed\n";
+	}
+}
+
+sub git_treediff_plain {
+	git_treediff('plain');
+}
+
 sub git_history {
 	if (!defined $hash_base) {
 		$hash_base = git_get_head_hash($project);

Re: [RFC] More diff possibilities in gitweb

[Martin Koegler]

On Sat, Mar 17, 2007 at 10:51:51PM +0100, Jakub Narebski wrote:
> Martin Koegler wrote:
> 
> > The whole is implemented in JavaScript on the client side (tested with
> > IE 6 and Mozilla). On the server side, only the JavaScript file is
> > included in the output (see below). 
> 
> I'd rather not use JavaScript for that, but perhaps add in the history
> view (shortlog, history) selectbox to select commits (for shortlog, and
> history of a directory) or versions of a blob/file (history of a file)
> like in an "info" view e.g. in MoinMoin wiki:
>   http://git.or.cz/gitwiki/GitSurvey?action=info
> JavaScript can be used to beautify this like e.g. MediaWiki engine
> does:
>   http://en.wikipedia.org/w/index.php?title=Darcs&action=history 
> (to allow selection of diff only in forward direction).

Let's focus on the UI first:

Using a select box, as shown above, exposes only a small part of all
possible combinations, eg. how to compare commits on different
branches.

In my solution, I add to each link to a compareable object (commit,
tree, blob) two new link, eg: link "commit" becomes "commit (base |
diff)". "base" stores the ID of the object in a cookie. "diff"
retrieves the cookie and compares it with it's associated object.

Currently, only few combinations are implemented. Future version could
integrate tags (as an tag links to one commit). Additionally,
comparing tags/commits with a tree could be enabled, as a tag/commit
link to one tree.

Storing the base object in a cookie enables the user to use multiple
windows/tabs showing, eg. different branches. He can select in on
window the base object, switch to an other window, where he selects
the diff option. He can also go back in the browser history, without
losing his selection.

Any sugesstions, how the UI can be improved or designed better?

To the implementation:

The whole thing can be implemented without any JavaScript on the
server side. For an out of tree implementation, my prototype is
simpler to maintain, as it needs only a few small changes to gitweb.

If there is an interesst in this feature (and nobody opposes including
it), I can reimplement it in perl.

I would do it in the following way:
1) introduce git_treediff
2) Add the generation of a base and diff link to each object
3) If a base link is selected, the server sends a Set-Cookie header
and redirects to the page, where it came from. So maybe the broser can
fetch the page out of its cash in some situation.
4) If a diff link is selected, based on the base object in the cookie,
the url to do the diff and redirects the browser to it. This should
simplify caching.

If depending on cookies is a problem for some out there, the base
object can be passed in the URLs as fallback. It would require, that
the select base object is included in every link, which gitweb
generates.

mfg Martin Kögler

Re: [RFC] More diff possibilities in gitweb

[Jakub Narebski]

On Sun, Mar 18, 2007, Martin Koegler wrote:
> On Sat, Mar 17, 2007 at 10:51:51PM +0100, Jakub Narebski wrote:
>> Martin Koegler wrote:
>> 
>>> The whole is implemented in JavaScript on the client side (tested with
>>> IE 6 and Mozilla). On the server side, only the JavaScript file is
>>> included in the output (see below). 
>> 
>> I'd rather not use JavaScript for that, but perhaps add in the history
>> view (shortlog, history) selectbox to select commits (for shortlog, and
>> history of a directory) or versions of a blob/file (history of a file)
>> like in an "info" view e.g. in MoinMoin wiki:
>>   http://git.or.cz/gitwiki/GitSurvey?action=info
>> JavaScript can be used to beautify this like e.g. MediaWiki engine
>> does:
>>   http://en.wikipedia.org/w/index.php?title=Darcs&action=history 
>> (to allow selection of diff only in forward direction).
> 
> Let's focus on the UI first:
> 
> Using a select box, as shown above, exposes only a small part of all
> possible combinations, eg. how to compare commits on different
> branches.

I assume you meant "e.g. it doesn't provide a way to compare commits
on different branches".

We can easily add selectbox not only for shortlog and history views,
but also for heads view (which would allow to compare tips of different
branches) and tags view (which would allow to compare [commits pointed
to by] different tags).

Although there is yet another limitation, namely that shortlog and
history views are divided into pages...

> In my solution, I add to each link to a compareable object (commit,
> tree, blob) two new link, eg: link "commit" becomes "commit (base |
> diff)". "base" stores the ID of the object in a cookie. "diff"
> retrieves the cookie and compares it with it's associated object.

Well, I haven't though of it being a problem, because _I_ can have
always handcraft an URL to get requested diff from gitweb.

But after some thinking I guess that your idea has some merit
_provided_ that "(base | diff)" (by the way, what would happen
if someone would click "diff" without setting "base" first? is
"diff" inactive, or is some "base" default?) is added _only_ when
web browser supports it. It means that it should appear only when
JavaScript is turned on (easy if links are added by JavaScript)
and when cookies are turned on (I'm not sure if you are checking
this).

I guess that we could (under control of gitweb configuration, %feature
hash or something like that) add links which would lead to server
setting a cookie, or adding unused CGI parameters ('hp', 'hpb' and
'fp' are used only by 'diff' views).

> Currently, only few combinations are implemented. Future version could
> integrate tags (as an tag links to one commit). Additionally,
> comparing tags/commits with a tree could be enabled, as a tag/commit
> link to one tree.

Diffs makes sense only between two commit-ish (commit-ish being commit
or a tag; head also denotes a commit) for which we need 'h' and 'hp'
params, between two tree-ish (tree-ish being tree, commit or a tag)
for which we need 'h'/'hb' and 'hp'/'hpb' and usually 'f', sometimes
'fp', and finnally between two blobs, for which we need 'h'/'hb' and
'hp'/'hpb' and usually 'f', sometimes 'fp'.

There makes no sense to compare tree-ish with blob, for example.

By the way, I'm not that sure if gitweb handle correctly request for
diff of two _different_ tree-ish, or two arbitrary different files.

[...]
> 1) introduce git_treediff

NOTE: git_treediff is in some parts very similar (I think) to
git_commitdiff with the exception of not being there commit message,
nor links to parents, and I'd like very much to avoid code repetition
(duplication) betweem git_treediff and git_commitdiff if possible.
On the other hand git_treediff is similar to git_blob in handling
its parameters ('h', 'hp' but also 'hb', 'hpb' and 'f', 'fp').

[...]
> If depending on cookies is a problem for some out there, the base
> object can be passed in the URLs as fallback. It would require, that
> the select base object is included in every link, which gitweb
> generates.

Not much of a problem. Generation of links is consolidated in href()
subroutine, and it would be fairly easy to add passing through base
parameters, if they are needed or not. It means that if 'shortlog' view
was passed 'hp' parameter then "diff" links (or "diff <sha1>" links,
or "diff sel"/"diff selected" links) would lead to diff with given base.


P.S. I find it strange that you don't send patches from git. If you want
to test gitweb, you should have git installed. Why not fetch git.git
repository, and send patches from it?

-- 
Jakub Narebski
Poland

Re: [RFC] More diff possibilities in gitweb

[Jakub Narebski]

Some of discussion is repeated in another subthread

On Sun, Mar 18, 2007, Martin Koegler wrote:
> 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 ;-)

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

> @@ -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.

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

Re: [RFC] More diff possibilities in gitweb

[Martin Koegler]

I tried to merge the two thread.

On Sun, Mar 18, 2007 at 10:55:59PM +0100, Jakub Narebski wrote:
> On Sun, Mar 18, 2007, Martin Koegler wrote:
> > On Sat, Mar 17, 2007 at 10:51:51PM +0100, Jakub Narebski wrote:
> > Let's focus on the UI first:
> > 
> > Using a select box, as shown above, exposes only a small part of all
> > possible combinations, eg. how to compare commits on different
> > branches.
> 
> I assume you meant "e.g. it doesn't provide a way to compare commits
> on different branches".

Yes

> We can easily add selectbox not only for shortlog and history views,
> but also for heads view (which would allow to compare tips of different
> branches) and tags view (which would allow to compare [commits pointed
> to by] different tags).
> 
> Although there is yet another limitation, namely that shortlog and
> history views are divided into pages...

To avoid all those problems, I tried the generic approach with "(base | diff)"
 
> > In my solution, I add to each link to a compareable object (commit,
> > tree, blob) two new link, eg: link "commit" becomes "commit (base |
> > diff)". "base" stores the ID of the object in a cookie. "diff"
> > retrieves the cookie and compares it with it's associated object.
> 
> Well, I haven't though of it being a problem, because _I_ can have
> always handcraft an URL to get requested diff from gitweb.

I want a comfortable solution. Additionally, this can only be done by
a user, who knows about the internals of gitweb.

> But after some thinking I guess that your idea has some merit
> _provided_ that "(base | diff)" (by the way, what would happen
> if someone would click "diff" without setting "base" first? is
> "diff" inactive, or is some "base" default?) is added _only_ when

In my JavaScript solution, the user gets a message box in this case:
"no base object selected".

> web browser supports it. It means that it should appear only when
> JavaScript is turned on (easy if links are added by JavaScript)
> and when cookies are turned on (I'm not sure if you are checking
> this).

No, there is no check at the moment. Buts this is basically checking
the navigator.cookieEnabled property. A user could only reject some
cookies, but catching this would probably be too complicated.

> I guess that we could (under control of gitweb configuration, %feature
> hash or something like that) add links which would lead to server
> setting a cookie, or adding unused CGI parameters ('hp', 'hpb' and
> 'fp' are used only by 'diff' views).

Yes, but I would you a new set of parameters for it (eg bf, bh, bhb), so that a
possible (future) fallback version without cookies can reuse them.
 
> > Currently, only few combinations are implemented. Future version could
> > integrate tags (as an tag links to one commit). Additionally,
> > comparing tags/commits with a tree could be enabled, as a tag/commit
> > link to one tree.
> 
> Diffs makes sense only between two commit-ish (commit-ish being commit
> or a tag; head also denotes a commit) for which we need 'h' and 'hp'
> params,  between two tree-ish (tree-ish being tree, commit or a tag)
> for which we need 'h'/'hb' and 'hp'/'hpb' and usually 'f', sometimes

For tree diffs, I currently only support h/hp. Using hb/hbp and f/fp
would require, that both trees share the same file name.

> 'fp', and finnally between two blobs, for which we need 'h'/'hb' and
> 'hp'/'hpb' and usually 'f', sometimes 'fp'.

For blob diffs, I prefer to use only h/bp (and f/fp for showing nice
file names). I fall back to hp/hb and f for the blobs in the history
views (where h is not available), which has the drawback, that both
blobs must have the same name.

> There makes no sense to compare tree-ish with blob, for example.

Yes.

> By the way, I'm not that sure if gitweb handle correctly request for
> diff of two _different_ tree-ish, or two arbitrary different files.

If you use not hb/hbp, this works.

> > 1) introduce git_treediff
> 
> NOTE: git_treediff is in some parts very similar (I think) to
> git_commitdiff with the exception of not being there commit message,
> nor links to parents, and I'd like very much to avoid code repetition
> (duplication) betweem git_treediff and git_commitdiff if possible.
> On the other hand git_treediff is similar to git_blob in handling
> its parameters ('h', 'hp' but also 'hb', 'hpb' and 'f', 'fp').

I created git_treediff out of git_commitdiff.

> > If depending on cookies is a problem for some out there, the base
> > object can be passed in the URLs as fallback. It would require, that
> > the select base object is included in every link, which gitweb
> > generates.
> 
> Not much of a problem. Generation of links is consolidated in href()
> subroutine, and it would be fairly easy to add passing through base
> parameters, if they are needed or not. It means that if 'shortlog' view
> was passed 'hp' parameter then "diff" links (or "diff <sha1>" links,
> or "diff sel"/"diff selected" links) would lead to diff with given base.

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

This has on complication: href only generats the url. The link is
generated in the caller. We would need a new wrapper for this.

[...]

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

I'll try to implement a solution for this.

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

gitweb.cgi is generated out of gitweb.perl. Then I need to manually
change some parameters in it and put it on a webserver. For
developing, it's easier for me to edit the cgi-script on the
server. In a diff of gitweb.cgi, I have to remove the unrelated
changes, for eg. setting paths parameters.

I would be interessted, how git can help me in this workflow?

> > 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 

I'll try to produce better patches in the future.

> > @@ -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.

For my prototyp, it was easier put the script at the end. Putting it
in the header would require, that I call the JavaScript load function
in the body onload event. In the end version, I'll change that.

> > @@ -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...

Compaing this (if git-gui.git history is part of git.git), already
works.  It's currently simple, as I ignore hb. The diff would be
relativ (that means, that git-gui/ in git.git would be a/ and / in
git-gui.git would be b/). 

Do git-diff-tree offers an option to compare in two trees two
different file names? If not, using hb would require, that both trees
have the same name. In this case, as all tree links seems to have h, I
see no use for hb/hpb.

mfg Martin Kögler

Re: [RFC] More diff possibilities in gitweb

[Jakub Narebski]

[-- Attachment #1: Type: text/plain, Size: 8930 bytes --]

On Mon, Mar 19, 2007, Martin Koegler wrote:
> I tried to merge the two threads.
> 
> On Sun, Mar 18, 2007 at 10:55:59PM +0100, Jakub Narebski wrote:
>> On Sun, Mar 18, 2007, Martin Koegler wrote:
>>> On Sat, Mar 17, 2007 at 10:51:51PM +0100, Jakub Narebski wrote:
>>>
>>> Let's focus on the UI first:
>>> 
>>> Using a select box, as shown above, exposes only a small part of all
>>> possible combinations, eg. how to compare commits on different
>>> branches.
>> 
>> I assume you meant "e.g. it doesn't provide a way to compare commits
>> on different branches".
> 
> Yes
[...]
>> But after some thinking I guess that your idea has some merit
>> _provided_ that "(base | diff)" (by the way, what would happen
>> if someone would click "diff" without setting "base" first? is
>> "diff" inactive, or is some "base" default?) is added _only_ when
> 
> In my JavaScript solution, the user gets a message box in this case:
> "no base object selected".

This is a bad idea from UI point of view. Users don't like message boxes,
unless really, really necessary.

We can either do like MediaWiki engine does, i.e. have default base, be
it current version (tip of shortlog, history) or the previous (parent,
or rather first simplified parent) version. Or we can make "diff" part
of "(base | diff)" inactive and grayed out till base is selected, which
should be fairly easy in JavaScript.
 
>> web browser supports it. It means that it should appear only when
>> JavaScript is turned on (easy if links are added by JavaScript)
>> and when cookies are turned on (I'm not sure if you are checking
>> this).
> 
> No, there is no check at the moment. Buts this is basically checking
> the navigator.cookieEnabled property. A user could only reject some
> cookies, but catching this would probably be too complicated.

Please make it so.

>> I guess that we could (under control of gitweb configuration, %feature
>> hash or something like that) add links which would lead to server
>> setting a cookie, or adding unused CGI parameters ('hp', 'hpb' and
>> 'fp' are used only by 'diff' views).
[...]

>>> If depending on cookies is a problem for some out there, the base
>>> object can be passed in the URLs as fallback. It would require, that
>>> the select base object is included in every link, which gitweb
>>> generates.
>> 
>> Not much of a problem. Generation of links is consolidated in href()
>> subroutine, and it would be fairly easy to add passing through base
>> parameters, if they are needed or not. It means that if 'shortlog' view
>> was passed 'hp' parameter then "diff" links (or "diff <sha1>" links,
>> or "diff sel"/"diff selected" links) would lead to diff with given base.
> 
>> 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. 
> 
> This has on complication: href only generats the url. The link is
> generated in the caller. We would need a new wrapper for this.
> 
> [...]

I was thinking about the following solution: by default "base" part
of server-side plain HTML "(base | diff)" would lead to current URL
but with 'hp' and if necessary also 'hpb', 'fp' CGI parameters added,
(e.g. if current URL is
  p=project.git;a=shortlog;h=next
then "base" link somewhere deeper in the "shortlog" would be:
  p=project.git;a=shortlog;h=next;hp=<sha1 of a commit>
i.e. with appropriate 'hb' param added). This of course would mean
that "base" link would mean additional request for the server. And
newly served page would have modified "diff" part of "(base | diff)"
to have just set base as base.

If non-diff view has diff-related ('hp', 'hpb', 'fp') parameters set,
they are passed to "diff" part of "(base | diff)" links; if they
are not set, "diff" part is inactive (unless there is some default).

Of course when we finally view requested diff, the selected base is
lost... unless we save it in a cookie, server-side.

>>> Currently, only few combinations are implemented. Future version could
>>> integrate tags (as an tag links to one commit). Additionally,
>>> comparing tags/commits with a tree could be enabled, as a tag/commit
>>> link to one tree.
>> 
>> Diffs makes sense only between two commit-ish (commit-ish being commit
>> or a tag; head also denotes a commit) for which we need 'h' and 'hp'
>> params,  between two tree-ish (tree-ish being tree, commit or a tag)
>> for which we need 'h'/'hb' and 'hp'/'hpb' and usually 'f', sometimes
> 
> For tree diffs, I currently only support h/hp. Using hb/hbp and f/fp
> would require, that both trees share the same file name.

Not true. You can use extended sha1 syntax to specify trees by
revision (commit/base hash) and directory name (path): see 
"SPECIFYING REVISIONS" section in git-rev-parse(1):

 * A suffix : followed by a path; this names the blob or tree at the given
   path in the tree-ish object named by the part before the colon.

So if 'h=<tree_hash>' is given, we use it's value; if only 
'hb=<commitish_hash>' and 'f=<filename>' are given, use
<commitish_hash>:<filename> to specify tree. Not hard.

>> 'fp', and finnally between two blobs, for which we need 'h'/'hb' and
>> 'hp'/'hpb' and usually 'f', sometimes 'fp'.
> 
> For blob diffs, I prefer to use only h/hp (and f/fp for showing nice
> file names). I fall back to hb/hpb and f for the blobs in the history
> views (where h is not available), which has the drawback, that both
> blobs must have the same name.

I think it works. If I remember correctly in commitdiff and commit views,
ahen file modification is detected as rename the blobdiff link is
generated with both 'h' (for blob hash) and 'hb' (for commit hash),
and 'hp' and 'hbp', and with 'f' and 'fp'; and it just works.

>> By the way, I'm not that sure if gitweb handle correctly request for
>> diff of two _different_ tree-ish, or two arbitrary different files.
> 
> If you use not hb/hbp, this works.

See above.

>>> 1) introduce git_treediff
>> 
>> NOTE: git_treediff is in some parts very similar (I think) to
>> git_commitdiff with the exception of not being there commit message,
>> nor links to parents, and I'd like very much to avoid code repetition
>> (duplication) betweem git_treediff and git_commitdiff if possible.
>> On the other hand git_treediff is similar to git_blob in handling
>> its parameters ('h', 'hp' but also 'hb', 'hpb' and 'f', 'fp').
> 
> I created git_treediff out of git_commitdiff.

Perhaps some code could be shared, i.e. separated into common subroutine,
or git_commitdiff could be generalized to take also trees?

>>> 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.
> 
> I'll try to implement a solution for this.

Thanks in advance.

>> 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.
> 
> gitweb.cgi is generated out of gitweb.perl. Then I need to manually
> change some parameters in it and put it on a webserver. For
> developing, it's easier for me to edit the cgi-script on the
> server. In a diff of gitweb.cgi, I have to remove the unrelated
> changes, for eg. setting paths parameters.
> 
> I would be interessted, how git can help me in this workflow?

There are two solutions. First is to use script to generate gitweb.cgi
out of (just modified) gitweb.perl, setting the parameters while at
it, and install it. See attached gitweb-update.sh which I use for this.
Another would be to use config file to set appropriate parameters;
see attached gitweb_config.perl which I use for similar purposes.

>>> @@ -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.
> 
> For my prototype, it was easier put the script at the end. Putting it
> in the header would require, that I call the JavaScript load function
> in the body onload event. In the end version, I'll change that.

I can understand this, but I think patches _for the inclusion_ should
have scripts included in the header.

[...]
> Do git-diff-tree offers an option to compare in two trees two
> different file names? If not, using hb would require, that both trees
> have the same name. In this case, as all tree links seems to have h, I
> see no use for hb/hpb.

See above. You can use hb:f and hpb:fp (or hpb:f if fp is not set,
meaning fp=f).

-- 
Jakub Narebski
Poland

[-- Attachment #2: gitweb-update.sh --]
[-- Type: text/plain, Size: 519 bytes --]

#!/bin/bash

#BASEDIR="/home/jnareb/"

#BINDIR="/usr/bin"
BINDIR="/home/local/git"

function make_gitweb()
{
	pushd "/home/jnareb/git/"

	make GITWEB_PROJECTROOT="/home/local/scm" \
	     GITWEB_CSS="/gitweb/gitweb.css" \
	     GITWEB_LOGO="/gitweb/git-logo.png" \
	     GITWEB_FAVICON="/gitweb/git-favicon.png" \
	     bindir=$BINDIR \
	     gitweb/gitweb.cgi

	popd
}

function copy_gitweb()
{
	cp -fv /home/jnareb/git/gitweb/gitweb.{cgi,css} /home/local/gitweb/
}

make_gitweb
copy_gitweb

# end of gitweb-update.sh

[-- Attachment #3: gitweb_config.perl --]
[-- Type: text/plain, Size: 621 bytes --]

#!/usr/bin/perl

# gitweb configuration

our $version = "current";
#our $GIT = "/usr/bin/git";
our $GIT = "/home/local/git/git";
our $projectroot = "/home/local/scm";
our $home_link_str = "projects";
our $site_name = "[localhost]";
our $site_header = "";
our $site_footer = "";
our $home_text = "indextext.html";
our @stylesheets = ("file:///home/jnareb/git/gitweb/gitweb.css");
our $logo = "file:///home/jnareb/git/gitweb/git-logo.png";
our $favicon = "file:///home/jnareb/git/gitweb/git-favicon.png";
our $projects_list = "";
our $export_ok = "";
our $strict_export = "";
our @git_base_url_list = ("/home/jnareb/git");