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

Jakub Narebski <jnareb@gmail.com> writes:

Before answering 3 questions, I think we need to ask a bigger question. What is the primary target audience of gitweb?

If it is for git-uninitiated, then staying as close to what GNU diff would give would be a better idea. I would say we at least can assume that the user has some familiarity with SCM, and knows what kind of information is kept track of and is shown as differences between versions, and what files, directories and symlinks are, but not how git represents these. On the other hand, if the user uses git to track a project (not necessarily the project the user is looking at with gitweb) and is familiar with the way git-diff presents these information, deviating from git-diff output is distractiing.

At least to me /-rw-rw-... part made me feel uneasy for that reason.

WIth that in mind, I'll think aloud what I would like if I were not familiar with git:

 * "diff --git a/file b/file" would not use /dev/null but
   ---/+++ does.  If the former is shown as link, it should be
   visible which side is a link and which side is not for
   creation or deletion diff; otherwise you would need a second
   to realize it is not a bug that clicking on a/file on the
   "diff --git" line did not show anything for a creation diff.

 * I think showing object names in "index xxx..yyy mode" line is
   not very useful to humans (they are added for tools).  I do
   agree that we would want some clickable handle in combined
   diff output, but people not familiar with git would not know
   that "index xxx,yyy..zzz" is where you would find the
   parents, so that line needs to be munged anyway.

   Side note: Even though some git people (Luben, for example)
   claim they recognize some abbreviated object names, I suspect
   that are mostly recent commits and not blobs.

 * Mode on the "index" line may be useful, but as you say 100644
   is probably too git specific; however if our audience is
   git-uninitiated, I doubt -rw-r--r-- is much better (it is
   UNIXism, which I personally do not mind).  Spelling them out
   like "regular file", "executable file", or "symbolic link"
   might be more readable.  And spelling them out is more
   technically correct; when git says 100644, it does _not_ mean
   "-rw-r--r--", but it means "regular file, not executable"
   (IOW, we borrowed the octal bits from POSIX but it means
   slightly a different thing).

 * So the above two together suggests that I probably would want
   to see something like:

	diff --git a/file b/file (regular file)

   with a/file and b/file as visible links for one parent diff.
   For combined,

        diff --cc a/file b/file (regular file)

   with perhaps a/file be a pop-up window that has a list of
   links to parent blobs (b/file is a link to the merge result
   blob), or maybe 

        diff --cc a/file (parent1 parent2) b/file (regular file)

   where parent#N is a link to the blob in the parent (a/file is
   not clickable, b/file is the link to the result blob).

 * I think redundant links to blobs in extended headers (rename
   from ... to ...) and ---/+++ lines are Ok, but as other
   redundant links they probably are better kept half-hidden
   (i.e. react to mouse-over like commit titles) to avoid visual
   distraction.

 * I think the filename quoting is better left as-is, since it
   is a way to indicate something funny is going on. 

   One thing I noticed while browsing gitweb/ part is that we
   quote byte values above 128; we might want to change
   quote_c_style_counted() to either use unsigned char to avoid
   this, or explicitly use signed char to keep this across
   platforms (char could be unsigned char).

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

I agree.

gitweb's primary audience is the developers who type "git ..." at shell prompt all the time.

It made me feel uneasy to see it where it was because it didn't belong there either way.

I'd argue otherwise: trying to click on /dev/null and failing but succeeding on b/file has already taught something to the uinitiated user.

OTOH, if one is trying to "click" on /dev/null in gitweb commitdiff view -- they have other problems to resolve first...

I like to see it because I might need to know the sha in order to go back to shell prompt and do something with the object. Instead of having to git-ls-tree -r .... in order to find it from the sha of the commitdiff.

I think gitweb should first and foremost cater to git users.

Yes, commit-8 for the day, blob-8 for less than a minute which allows me to move the mouse pointer from to the xterm.

It would be more readable, but then it would be more readable. Ideally I'd like it to be less readable, i.e. less to read.

I agree.

   Luben