# Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view 9 messages from 2006-10-30 to 2006-10-30. Participants: Jakub Narebski, Junio C Hamano, Luben Tuikov. Thread: https://gitlist.dev/t/43275 ## Luben Tuikov, 2006-10-30 01:59 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <92622.251.qm@web31812.mail.mud.yahoo.com> URL: https://gitlist.dev/e/92622.251.qm%40web31812.mail.mud.yahoo.com In-Reply-To: <200610300051.37896.jnareb@gmail.com> ``` --- Jakub Narebski wrote: > Replace "gitweb diff header" with its full sha1 of blobs and replace > it by "git diff" header and extended diff header. Change also somewhat > highlighting of diffs. > > Changes: > * "gitweb diff header" which looked for example like below: > file:__ -> file:__ > where 'file' is file type and '' is full sha1 of blob is > changed to > diff --git _a/_ _b/_ > In both cases links are visible and use default link style. If file > is added, a/ is not hyperlinked, if file is deleted, b/ > is not hyperlinked. "Everything clickable underlined" isn't the best way to represent things. Anyway, my 2 cents is that I don't like the overly explicit underlineing. I liked it the way it was in take 1. > * there is added "extended diff header", with and > hyperlinked (and shortened to 7 characters), and > explained: '' is extended to ' ()'. > * hyperlinking should work also when is originally > quoted. For now we present filename quoted. This needed changes to > parse_difftree_raw_line subroutine. And doesn't work: perhaps > unquote is broken. In which case we shouldn't commit this. IOW, let's commit things which we _know_ to work. Why not resubmit your original patch with the bugfixes as few comments as mentioned? > * from-file/to-file two-line header lines have slightly darker color > than removed/added lines. > * chunk header has now delicate line above for easier finding chunk > boundary, and top margin of 1px. Wouldn't this be confusing with the other fine lines? I personally don't like this chunk separation. Chunk separation already exists as is and we view it all the time elsewhere. If you'd like to separate chunks, why not darken the background of the section of line the chunk header is printed at? I.e. anything between the @@ including the @@. Luben ``` ## Jakub Narebski, 2006-10-30 08:05 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <200610300905.04454.jnareb@gmail.com> URL: https://gitlist.dev/e/200610300905.04454.jnareb%40gmail.com In-Reply-To: <92622.251.qm@web31812.mail.mud.yahoo.com> ``` Luben Tuikov wrote: > --- Jakub Narebski wrote: >> Replace "gitweb diff header" with its full sha1 of blobs and replace >> it by "git diff" header and extended diff header. Change also somewhat >> highlighting of diffs. >> >> Changes: >> * "gitweb diff header" which looked for example like below: >> file:__ -> file:__ >> where 'file' is file type and '' is full sha1 of blob is >> changed to >> diff --git _a/_ _b/_ >> In both cases links are visible and use default link style. If file >> is added, a/ is not hyperlinked, if file is deleted, b/ >> is not hyperlinked. > > "Everything clickable underlined" isn't the best way to represent things. > Anyway, my 2 cents is that I don't like the overly explicit underlineing. > I liked it the way it was in take 1. Thet is the only "obviously link" per patch. And I think there should be at least one non-hidden link. BTW. comments like this are the reason I've sent the patch as-is, without resolving the strange filenames problem (it would be nice if somebody was to send code; well Junio send patch to address core git filename quoting issue). >> * there is added "extended diff header", with and >> hyperlinked (and shortened to 7 characters), and >> explained: '' is extended to ' ()'. >> * hyperlinking should work also when is originally >> quoted. For now we present filename quoted. This needed changes to >> parse_difftree_raw_line subroutine. And doesn't work: perhaps >> unquote is broken. > > In which case we shouldn't commit this. IOW, let's commit things > which we _know_ to work. > > Why not resubmit your original patch with the bugfixes as few comments > as mentioned? I'll do that, but for now quoting/unquoting filename is broken, both in gitweb but also to lesser extent in git core (quoting perfectly valid UTF-8 characters). I'll try to adress that, but I wanted to send next RFC patch for review. >> * from-file/to-file two-line header lines have slightly darker color >> than removed/added lines. >> * chunk header has now delicate line above for easier finding chunk >> boundary, and top margin of 1px. > > Wouldn't this be confusing with the other fine lines? > I personally don't like this chunk separation. Chunk separation > already exists as is and we view it all the time elsewhere. But not always the program displaying diff can display such line separating chunks, for example on text terminal it can't. But if you think that the dotted 1px #ffbbff line is too intrusive, we can remove it (and perhaps increase vertical space a few pixels). I'd like to have more opinions first. BTW. you can easily override it in your CSS file. > If you'd like to separate chunks, why not darken the background > of the section of line the chunk header is printed at? I.e. > anything between the @@ including the @@. I'd rather have this one in a separate commit (this needs changes to format_diff_line, not only to git_patchset_body). -- Jakub Narebski ``` ## Junio C Hamano, 2006-10-30 08:21 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <7vy7qyw6w6.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vy7qyw6w6.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200610300905.04454.jnareb@gmail.com> ``` Jakub Narebski writes: > ..., without > resolving the strange filenames problem (it would be nice if somebody was > to send code; well Junio send patch to address core git filename quoting > issue). Having showed that patch, I do not think it is a good way to go. I think the UI layer like gitweb should have freedom to choose its own pathname handling, and should read from -z output. The git plumbing is agnostic to the character encoding issues, and does not care what character set pathnames are encoded, and what character set the file contents are encoded. These are very project specific and should be handled per project. One of the primary purposes, aside from being understandable by humans, of human readable "git diff" format is to convey machine readable information without corruption in text form transport (think "e-mail"), so quoting pathnames to avoid high-bits has clear advantage (pathname character set and file contents character set are often different, and when both are non-ascii we would need to sacrifice one if we spit diff out in a single text file --- we choose to quote pathnames so that it would not interfere with file contents, which would be more important to be inspectable by humans reading "diff" output). People in the past suggested converting everything to UTF-8 but that is not a usable robust solution in the real world. The HTML output generated by gitweb do not have the "machine readably robust" requirement, so it _can_ stress more on readability by humans, including converting both pathname and contents to UTF-8; for this, the project needs to tell gitweb what its pathname character encoding is, and blob contents encoding, at the worst case blob-by-blob, but more often path-by-path. ``` ## Junio C Hamano, 2006-10-30 08:51 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <7vlkmyw5hx.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlkmyw5hx.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <7vy7qyw6w6.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > Jakub Narebski writes: > >> ..., without >> resolving the strange filenames problem (it would be nice if somebody was >> to send code; well Junio send patch to address core git filename quoting >> issue). > > Having showed that patch, I do not think it is a good way to go. > > I think the UI layer like gitweb should have freedom to choose > its own pathname handling, and should read from -z output. > > The git plumbing is agnostic to the character encoding issues, > and does not care what character set pathnames are encoded, and > what character set the file contents are encoded. These are > very project specific and should be handled per project. Side note. There is one action item for shorter term I realized we would need. We need to make patch-id generation use unquoted filenames, so that no matter how differently we might reimplement quote_c_style() later, we would get the same patch-id from the diff between the same two trees. Currently I think it hashes the quoted filename that appear on ---/+++ lines (and "rename from"/"rename to" lines as well, but usually you do not use -M/-C when computing the patch-id, so these are not really an issue). I think longer term plan for git should include standardizing on how pathname encoding and content encoding are handled at the UI layer. And that plan _should_ eventually include the textual diff part. But textual diff has a machine readability requirements, so we need to proceed very carefully. I think the patch I sent out earlier is probably good enough if we only care about utf-8, but would not correctly handle other sane pathname encodings such as extended UNIX code (let alone broken but still widely used encodings such as MS-Kanji aka Shift_JIS). ``` ## Jakub Narebski, 2006-10-30 09:43 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <200610301043.22033.jnareb@gmail.com> URL: https://gitlist.dev/e/200610301043.22033.jnareb%40gmail.com In-Reply-To: <7vy7qyw6w6.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Jakub Narebski writes: > >> ..., without >> resolving the strange filenames problem (it would be nice if somebody was >> to send code; well Junio send patch to address core git filename quoting >> issue). > > Having showed that patch, I do not think it is a good way to go. > > I think the UI layer like gitweb should have freedom to choose > its own pathname handling, and should read from -z output. That's a very good idea. I'll send separate patch (if noone else will do this, that is) which would convert gitweb to always use -z output, both git-ls-tree and git-diff-tree... oh, well, there is no -z patch output, so in patch part we would have to replace git quoted part by gitweb quoted part. BTW gitweb has to do it's own pathname handling, at least LF, perhaps TAB, perhaps other control characters like ESC; quote quoting character and mark filename as quoted somewhat (it can be with CSS style, not necessary by surrounding with some kind of quotes; besides that can also be added via CSS in modern enough browsers using :before and :after pseudo-elements and 'content' property). -- Jakub Narebski ``` ## Jakub Narebski, 2006-10-30 13:58 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <200610301458.33634.jnareb@gmail.com> URL: https://gitlist.dev/e/200610301458.33634.jnareb%40gmail.com In-Reply-To: <200610301043.22033.jnareb@gmail.com> ``` Jakub Narebski wrote: > Junio C Hamano wrote: >> >> I think the UI layer like gitweb should have freedom to choose >> its own pathname handling, and should read from -z output. > > That's a very good idea. I'll send separate patch (if noone else will > do this, that is) which would convert gitweb to always use -z output, > both git-ls-tree and git-diff-tree... oh, well, there is no -z patch > output, so in patch part we would have to replace git quoted part by > gitweb quoted part. I have realized that it is not as easy as it sounds, at least for the git-diff-tree output. For the LF-terminated output (without '-z') you know that LF separates records, and you can split on LF ('\n'). It is not the case for '-z' '\0' delimited output: NUL ('\0') might also mean end of one of the filenames in the rename/copy case, and is used to separate filename(s) from the score (although here TAB would be enough). And that is probably the case that gitweb uses default git-diff-tree output, and _tries_ to unescape(...) filename. The solution would be perhaps to add '--zz' option to use '-z' output but to separate records by double NUL, i.e. '\0\0'... By the way, why diff-tree "raw" format for merge gives only one, final, filename? -- Jakub Narebski ``` ## Junio C Hamano, 2006-10-30 22:59 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <7v7iyhv29o.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7iyhv29o.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200610301458.33634.jnareb@gmail.com> ``` Jakub Narebski writes: > I have realized that it is not as easy as it sounds, at least for the > git-diff-tree output... I do not think so.a The output is designed to be machine parsable. See Documentation/diff-format.txt and also please refer to 65a9289d:diff-helper.c for a sample code to see how to parse the output. ``` ## Jakub Narebski, 2006-10-30 23:32 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <200610310032.09726.jnareb@gmail.com> URL: https://gitlist.dev/e/200610310032.09726.jnareb%40gmail.com In-Reply-To: <7v7iyhv29o.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Jakub Narebski writes: > >> I have realized that it is not as easy as it sounds, at least for the >> git-diff-tree output... > > I do not think so.a The output is designed to be machine > parsable. See Documentation/diff-format.txt and also please > refer to 65a9289d:diff-helper.c for a sample code to see how to > parse the output. What I wanted to say is that the output is not easy to use in "split first, parse later", with sometimes counting number of entries in between. With current format you rather have to pass in one go, for '-z' format reading filenames from the stream. By the way, git-ls-tree -z uses TAB as a start of filename, while git-diff-tree -z uses NUL as a start of (first) filename. Why? -- Jakub Narebski ``` ## Junio C Hamano, 2006-10-30 23:40 Subject: Re: [PATCH/RFC (take 2)] gitweb: New improved patchset view Message-ID: <7vd589tlso.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vd589tlso.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200610310032.09726.jnareb@gmail.com> ``` Jakub Narebski writes: > By the way, git-ls-tree -z uses TAB as a start of filename, while > git-diff-tree -z uses NUL as a start of (first) filename. Why? I do not know a good explanation other than hysterical raisins. ```