threads / rfc / 43258

patchRe: [PATCH/RFC] gitweb: New improved patchset view

Subject: Re: [PATCH/RFC] gitweb: New improved patchset view

## tl;dr

5 messages between Oct 29, 2006 and Oct 29, 2006. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Junio C Hamano· Oct 29, 2006, 11:47 UTC · lore

Jakub Narebski <jnareb@gmail.com> writes:

Show 9 quoted lines

> Changes:
> * "gitweb diff header" which looked for example like below:
>     file:_<sha1 before>_ -> file:_<sha1 after>_
>   where 'file' is file type and '<sha1>' is full sha1 of blob, is link
>   and uses default link style is changed to
>     diff --git a/<file before> b/<file after>
>   where <file> is hidden link (i.e. underline on hover, only)
>   to appropriate version of file. If file is added, a/<file> is not
>   hyperlinked, if file is deleted, b/<file> is not hyperlinked.

I do not have time to look at the code right now, but here are quick comments on the output.

I personally do not mind "hidden" but it might be more obvious to make them normal links -- the filenames in the header are not part of long text people need to "read". On the other hand, it feels a bit wasteful to have these hidden links both on "diff --git" line *and* ---/+++ lines (these three are very close to each other).

> * there is added "extended diff header", with <path> and <hash>
>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.

It somehow feels that deviating from what "git diff" gives makes it very distracting; I would feel better if "/-rw-r--r-- (file)" were not there.

Also I think arguing over 7 or 8 hexdigits is pointless; if you are reading this from "git diff", it is probably the easiest to match what "git diff" gave you. One thing we _might_ want to do in the future is to change "git diff" to use DEFAULT_ABBREV hexdigits at the minimum but more if needed to disambiguate; I think it currently does not do the "more if needed" part.

> * <file> hyperlinking should work also when <file> is originally
>   quoted. For now we present filename quoted. This needed changes to
>   parse_difftree_raw_line subroutine.

This feels Ok.

> * from-file/to-file two-line header lines have slightly darker color
>   than removed/added lines.

This visually feels right.

> * chunk header has now delicate line above for easier finding chunk
>   boundary, and top margin of 1px.

This visually feels right.

Jakub Narebski· Oct 29, 2006, 12:24 UTC · re: Junio C Hamano · lore

Junio C Hamano wrote:

Show 14 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Changes:
>> * "gitweb diff header" which looked for example like below:
>>     file:_<sha1 before>_ -> file:_<sha1 after>_
>>   where 'file' is file type and '<sha1>' is full sha1 of blob, is link
>>   and uses default link style is changed to
>>     diff --git a/<file before> b/<file after>
>>   where <file> is hidden link (i.e. underline on hover, only)
>>   to appropriate version of file. If file is added, a/<file> is not
>>   hyperlinked, if file is deleted, b/<file> is not hyperlinked.
> 
> I do not have time to look at the code right now, but here are 
> quick comments on the output.

Code is a bit horrible I think in trying to be smart and avoid code repetition.

Show 6 quoted lines

> I personally do not mind "hidden" but it might be more obvious
> to make them normal links -- the filenames in the header are not
> part of long text people need to "read".  On the other hand,
> it feels a bit wasteful to have these hidden links both on "diff
> --git" line *and* ---/+++ lines (these three are very close to
> each other).

Good idea. We have lost visible links with the change. And easy to implement: just remove style override for .diff.header a.list from CSS (or what would be better but harder to code remove "list" class from <a> element in "git header").

Originally there was visible link with full sha1 of blob as text, and hidden link with filename as text; perhaps dropping the latter ---/+++ line link is better idea (here hyperlink should I think be hidden, if there is any, to match git-diff and GNU diff -u output). And this would simplify code somewhat...

The hidden (but here I think we can change to not-hidden links) links in the "index" extended header line are to stay I think: this is the only place where we can have links to all the versions of file in the future combined commitdiff format.

Show 7 quoted lines

>> * there is added "extended diff header", with <path> and <hash>
>>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.
> 
> It somehow feels that deviating from what "git diff" gives makes
> it very distracting; I would feel better if "/-rw-r--r-- (file)"
> were not there.

Well, the old version had "(<file type>)" in gitweb diff header. Not all git/gitweb users are familiar with POSIX numeric modes; perhaps changing the style (color for example) of added stuff to mark it as added would be enough?

Show 6 quoted lines

> Also I think arguing over 7 or 8 hexdigits is pointless; if you
> are reading this from "git diff", it is probably the easiest to
> match what "git diff" gave you.  One thing we _might_ want to do
> in the future is to change "git diff" to use DEFAULT_ABBREV
> hexdigits at the minimum but more if needed to disambiguate; I
> think it currently does not do the "more if needed" part.

I used 7 hexdigits because git-diff uses 7 by default.

Show 5 quoted lines

>> * <file> hyperlinking should work also when <file> is originally
>>   quoted. For now we present filename quoted. This needed changes to
>>   parse_difftree_raw_line subroutine.
> 
> This feels Ok.

I'm not sure if we shouldn't unquote filename always, or at least remove double quotes surrounding filename, or [ab]/filename. The decision to leave visible filename quoted was based on the troubles to code it correctly in "git diff" header where one of the filenames might not be hyperlinked: if file was added or deleted.

>> * from-file/to-file two-line header lines have slightly darker color
>>   than removed/added lines.
> 
> This visually feels right.

What is left is fine-tuning the color.

>> * chunk header has now delicate line above for easier finding chunk
>>   boundary, and top margin of 1px.
> 
> This visually feels right.

What is left is fine-tuning the line and vertical space.

-- 
Jakub Narebski

Jakub Narebski· Oct 29, 2006, 12:48 UTC · re: Jakub Narebski · lore

Jakub Narebski wrote:

Show 15 quoted lines

> Junio C Hamano wrote:
>> Jakub Narebski <jnareb@gmail.com> writes:
>>>
>>> * there is added "extended diff header", with <path> and <hash>
>>>   hyperlinked (and <hash> shortened to 7 characters), and <mode>
>>>   explained: '<mode>' is extnded to '<mode>/<symbolic mode> (<file type>)'.
>> 
>> It somehow feels that deviating from what "git diff" gives makes
>> it very distracting; I would feel better if "/-rw-r--r-- (file)"
>> were not there.
> 
> Well, the old version had "(<file type>)" in gitweb diff header.
> Not all git/gitweb users are familiar with POSIX numeric modes;
> perhaps changing the style (color for example) of added stuff
> to mark it as added would be enough?

Or we could use the following code:

  <abbr title="100755/-rwxr-xr-x (file)">100755</abbr>

which gives explanation on mouse-over.

-- 
Jakub Narebski

Jakub Narebski· Oct 29, 2006, 15:35 UTC · re: Junio C Hamano · lore

Junio C Hamano wrote:

> Jakub Narebski <jnareb@gmail.com> writes:
> 
>> Changes:

[...]

Show 5 quoted lines

>> * <file> hyperlinking should work also when <file> is originally
>>   quoted. For now we present filename quoted. This needed changes to
>>   parse_difftree_raw_line subroutine.
> 
> This feels Ok.

Emphasisis on "should". Check
  http://roke(.)dyndns(.)info/cgi-bin/gitweb/gitweb.cgi?p=git.git;a=commitdiff;h=gitweb/test

By the way, should we use quoted or unquoted filename?

-- 
Jakub Narebski

Luben Tuikov· Oct 29, 2006, 19:33 UTC · re: Junio C Hamano · lore

--- Junio C Hamano <junkio@cox.net> wrote:

> It somehow feels that deviating from what "git diff" gives makes
> it very distracting; I would feel better if "/-rw-r--r-- (file)"
> were not there.

Agree.

> Also I think arguing over 7 or 8 hexdigits is pointless; if you
> are reading this from "git diff", it is probably the easiest to
> match what "git diff" gave you.  One thing we _might_ want to do

Agree.

    Luben