Jakub Narebski <jnareb@xxxxxxxxx> writes:
> A couple of questions regarding new patchset/diff look for gitweb.
> Currently patch starts with "git diff" header
>
> diff --git a/file1 b/file2
>
> then we have a couple of extended header lines
>
> old|new|deleted file|new file mode <mode>
> copy|rename from|to <path>
> (dis)?similarity index <percent>
> index <hash>..<hash> [<mode>]
>
> then we have two-line from-file/to-file header
>
> --- a/file1
> +++ b/file2
>
> then patch itself.
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).
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html