{"thread":{"id":"43456","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","startedAt":"2006-10-29T18:49:45Z","lastAt":"2006-10-30T01:43:43Z","messageCount":2,"participants":["Luben Tuikov","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"296915","messageId":"7v3b972bzq.fsf@assigned-by-dhcp.cox.net","threadId":"43456","inReplyTo":"200610291850.46321.jnareb@gmail.com","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-29T18:49:45Z","receivedAt":"2006-10-29T18:49:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> A couple of questions regarding new patchset/diff look for gitweb.\n> Currently patch starts with \"git diff\" header\n>\n>   diff --git a/file1 b/file2\n>\n> then we have a couple of extended header lines\n>\n>   old|new|deleted file|new file mode <mode>\n>   copy|rename from|to <path>\n>   (dis)?similarity index <percent>\n>   index <hash>..<hash> [<mode>]\n>\n> then we have two-line from-file/to-file header\n>\n>   --- a/file1\n>   +++ b/file2\n>\n> then patch itself.\n\nBefore answering 3 questions, I think we need to ask a bigger\nquestion.  What is the primary target audience of gitweb?\n\nIf it is for git-uninitiated, then staying as close to what GNU\ndiff would give would be a better idea.  I would say we at least\ncan assume that the user has some familiarity with SCM, and\nknows what kind of information is kept track of and is shown as\ndifferences between versions, and what files, directories and\nsymlinks are, but not how git represents these.  On the other\nhand, if the user uses git to track a project (not necessarily\nthe project the user is looking at with gitweb) and is familiar\nwith the way git-diff presents these information, deviating from\ngit-diff output is distractiing.\n\nAt least to me /-rw-rw-... part made me feel uneasy for that\nreason.\n\nWIth that in mind, I'll think aloud what I would like if I were\nnot familiar with git:\n\n * \"diff --git a/file b/file\" would not use /dev/null but\n   ---/+++ does.  If the former is shown as link, it should be\n   visible which side is a link and which side is not for\n   creation or deletion diff; otherwise you would need a second\n   to realize it is not a bug that clicking on a/file on the\n   \"diff --git\" line did not show anything for a creation diff.\n\n * I think showing object names in \"index xxx..yyy mode\" line is\n   not very useful to humans (they are added for tools).  I do\n   agree that we would want some clickable handle in combined\n   diff output, but people not familiar with git would not know\n   that \"index xxx,yyy..zzz\" is where you would find the\n   parents, so that line needs to be munged anyway.\n\n   Side note: Even though some git people (Luben, for example)\n   claim they recognize some abbreviated object names, I suspect\n   that are mostly recent commits and not blobs.\n\n * Mode on the \"index\" line may be useful, but as you say 100644\n   is probably too git specific; however if our audience is\n   git-uninitiated, I doubt -rw-r--r-- is much better (it is\n   UNIXism, which I personally do not mind).  Spelling them out\n   like \"regular file\", \"executable file\", or \"symbolic link\"\n   might be more readable.  And spelling them out is more\n   technically correct; when git says 100644, it does _not_ mean\n   \"-rw-r--r--\", but it means \"regular file, not executable\"\n   (IOW, we borrowed the octal bits from POSIX but it means\n   slightly a different thing).\n\n * So the above two together suggests that I probably would want\n   to see something like:\n\n\tdiff --git a/file b/file (regular file)\n\n   with a/file and b/file as visible links for one parent diff.\n   For combined,\n\n        diff --cc a/file b/file (regular file)\n\n   with perhaps a/file be a pop-up window that has a list of\n   links to parent blobs (b/file is a link to the merge result\n   blob), or maybe \n\n        diff --cc a/file (parent1 parent2) b/file (regular file)\n\n   where parent#N is a link to the blob in the parent (a/file is\n   not clickable, b/file is the link to the result blob).\n\n * I think redundant links to blobs in extended headers (rename\n   from ... to ...) and ---/+++ lines are Ok, but as other\n   redundant links they probably are better kept half-hidden\n   (i.e. react to mouse-over like commit titles) to avoid visual\n   distraction.\n\n * I think the filename quoting is better left as-is, since it\n   is a way to indicate something funny is going on. \n\n   One thing I noticed while browsing gitweb/ part is that we\n   quote byte values above 128; we might want to change\n   quote_c_style_counted() to either use unsigned char to avoid\n   this, or explicitly use signed char to keep this across\n   platforms (char could be unsigned char).\n\n\n"},{"id":"294869","messageId":"811156.54327.qm@web31808.mail.mud.yahoo.com","threadId":"43456","inReplyTo":"7v3b972bzq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] gitweb: New improved patchset view","fromName":"Luben Tuikov","fromEmail":"ltuikov@yahoo.com","sentAt":"2006-10-30T01:43:43Z","receivedAt":"2006-10-30T01:43:43Z","isPatch":true,"sender":{"key":"ltuikov@yahoo.com","avatar":null},"body":"--- Junio C Hamano <junkio@cox.net> wrote:\n> Before answering 3 questions, I think we need to ask a bigger\n> question.  What is the primary target audience of gitweb?\n> \n> If it is for git-uninitiated, then staying as close to what GNU\n> diff would give would be a better idea.  I would say we at least\n> can assume that the user has some familiarity with SCM, and\n> knows what kind of information is kept track of and is shown as\n> differences between versions, and what files, directories and\n> symlinks are, but not how git represents these.  On the other\n> hand, if the user uses git to track a project (not necessarily\n> the project the user is looking at with gitweb) and is familiar\n> with the way git-diff presents these information, deviating from\n> git-diff output is distractiing.\n\nI agree.\n\ngitweb's primary audience is the developers who type \"git ...\"\nat shell prompt all the time.\n\n> At least to me /-rw-rw-... part made me feel uneasy for that\n> reason.\n\nIt made me feel uneasy to see it where it was because it\ndidn't belong there either way.\n\n> WIth that in mind, I'll think aloud what I would like if I were\n> not familiar with git:\n> \n>  * \"diff --git a/file b/file\" would not use /dev/null but\n>    ---/+++ does.  If the former is shown as link, it should be\n>    visible which side is a link and which side is not for\n>    creation or deletion diff; otherwise you would need a second\n\nI'd argue otherwise: trying to click on /dev/null and failing\nbut succeeding on b/file has already taught something to the\nuinitiated user.\n\nOTOH, if one is trying to \"click\" on /dev/null in gitweb commitdiff\nview -- they have other problems to resolve first...\n\n>    to realize it is not a bug that clicking on a/file on the\n>    \"diff --git\" line did not show anything for a creation diff.\n> \n>  * I think showing object names in \"index xxx..yyy mode\" line is\n>    not very useful to humans (they are added for tools).  I do\n\nI like to see it because I might need to know the sha in order to\ngo back to shell prompt and do something with the object.  Instead\nof having to git-ls-tree -r .... in order to find it from the sha\nof the commitdiff.\n\n>    agree that we would want some clickable handle in combined\n>    diff output, but people not familiar with git would not know\n>    that \"index xxx,yyy..zzz\" is where you would find the\n>    parents, so that line needs to be munged anyway.\n\nI think gitweb should first and foremost cater to git users.\n\n>    Side note: Even though some git people (Luben, for example)\n>    claim they recognize some abbreviated object names, I suspect\n>    that are mostly recent commits and not blobs.\n\nYes, commit-8 for the day, blob-8 for less than a minute\nwhich allows me to move the mouse pointer from to the xterm.\n\n>  * Mode on the \"index\" line may be useful, but as you say 100644\n>    is probably too git specific; however if our audience is\n>    git-uninitiated, I doubt -rw-r--r-- is much better (it is\n>    UNIXism, which I personally do not mind).  Spelling them out\n>    like \"regular file\", \"executable file\", or \"symbolic link\"\n>    might be more readable.\n\nIt would be more readable, but then it would be more readable.\nIdeally I'd like it to be less readable, i.e. less to read.\n\n>  * I think the filename quoting is better left as-is, since it\n>    is a way to indicate something funny is going on. \n\nI agree.\n\n   Luben\n"}]}