Jakub Narebski [off-list ref] writes:
Please remove this chunk from patch!. It makes gitweb "tree" view
empty. I have forgot that git-ls-tree _requires_ <tree-ish> so there
is no way to mistake pathspec with <tree-ish>.
To be honest, I dislike these */n series where the the end is
unknown. It just confuses me what's still surviving, what's
already shot down, and what's being rerolled.
Let's step back a bit and see if we share the same view as to
the status of each one:
[PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p...
Marked preliminary, perhaps need some discussion and rerolling
but I haven't looked at it.
[PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
Discussed; we agreed that showing byte values in different
colors is preferable. Waiting for re-roll.
[PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena...
I looked at it although haven't said anything yet. Probably a
safe and good change but I wonder how LF at the end of the line
matches /...(.+)$/s pattern; iow, if we do not use -z does it
still do the right thing? Otherwise I suspect you would perhaps
need to chomp?
[PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same...
Good fix and even improves readability; will apply after
dropping -- from ls-tree args.
[PATCH 5/n] [take 3] gitweb: New improved patchset view
[PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_...
[PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view
[PATCH 8/n] gitweb: Fix two issues with quoted filenames in git_patc...
Haven't looked at them and I do not think people have had enough
time to comment on them yet.
Junio C Hamano wrote:
To be honest, I dislike these */n series where the the end is
unknown. It just confuses me what's still surviving, what's
already shot down, and what's being rerolled.
Well, it looks like this patch series is closing to final patch.
The "New improved patchset view" is done.
Let's step back a bit and see if we share the same view as to
the status of each one:
[PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p...
Marked preliminary, perhaps need some discussion and rerolling
but I haven't looked at it.
I'm not sure if without this patch (well, the unquote part) gitweb
can work with filenames which git quotes using escape sequences,
like ", \, LF, TAB. Former version didn't unquote fully, and it
passed partially unquoted filename to git.
[PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
Discussed; we agreed that showing byte values in different
colors is preferable. Waiting for re-roll.
The problem with using text color or background color is that
the filenames tends to be shown with different color and background
color: "tree" view, parts of difftree, parts of diff header, etc.
Perhaps text-decoration: overline;? Just kidding...
[PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena...
I looked at it although haven't said anything yet. Probably a
safe and good change but I wonder how LF at the end of the line
matches /...(.+)$/s pattern; iow, if we do not use -z does it
still do the right thing? Otherwise I suspect you would perhaps
need to chomp?
We always pass chomped lines. First chunk is unnecessary (we care only
for type), without second "tree" view look strange for files with
embedded newline in filename.
[PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same...
Good fix and even improves readability; will apply after
dropping -- from ls-tree args.
As I said, noticed while testing gitweb with strange filenames
in 'gitweb/test' branch.
[PATCH 5/n] [take 3] gitweb: New improved patchset view
[PATCH 6/n] gitweb: Remove redundant "blob" links from git_difftree_...
[PATCH 7/n] gitweb: Output also empty patches in "commitdiff" view
[PATCH 8/n] gitweb: Fix two issues with quoted filenames in git_patc...
Haven't looked at them and I do not think people have had enough
time to comment on them yet.
Well, patch 5 and 8 could be collapsed.
--
Jakub Narebski
Jakub Narebski [off-list ref] writes:
quoted
[PATCH/RFC 1/n] gitweb: Better git-unquoting and gitweb-quoting of p...
Marked preliminary, perhaps need some discussion and rerolling
but I haven't looked at it.
I'm not sure if without this patch (well, the unquote part) gitweb
can work with filenames which git quotes using escape sequences,
I am reasonably sure it wouldn't, and it sounded like you wanted
to fix it better than the preliminary one, so I think we are in
agreement.
quoted
[PATCH 2/n] gitweb: Use '&iquot;' instead of '?' in esc_path
Discussed; we agreed that showing byte values in different
colors is preferable. Waiting for re-roll.
The problem with using text color or background color is that
the filenames tends to be shown with different color and background
color: "tree" view, parts of difftree, parts of diff header, etc.
Perhaps text-decoration: overline;? Just kidding...
Use of overstrike may actually not be a bad thing. It _is_
unusual situation after all.
quoted
[PATCH 3/n] gitweb: Use 's' regexp modifier to secure against filena...
I looked at it although haven't said anything yet. Probably a
safe and good change but I wonder how LF at the end of the line
matches /...(.+)$/s pattern; iow, if we do not use -z does it
still do the right thing? Otherwise I suspect you would perhaps
need to chomp?
We always pass chomped lines. First chunk is unnecessary (we care only
for type), without second "tree" view look strange for files with
embedded newline in filename.
The codepath affected by the first chunk does not chomp, which
was what I was referring to. So in the meantime will apply only
the second hunk.
quoted
[PATCH 4/n] gitweb: Secure against commit-ish/tree-ish with the same...
Good fix and even improves readability; will apply after
dropping -- from ls-tree args.
I just applied this. I'll be pushing out a "master" update
sometime today, and do not expect to be able to get to your "n
turned out to be ten" series, so it might be worthwhile to
reroll the remaining bits that you still care about on top of
what I push out tonight to make sure we are on the same page.
Preferably:
- you should avoid making a series out of more-or-less
unrelated things;
- if you are doing related things in one series, do not send
half-baked early parts out until you are finished and are
confident with it. If you do not know how many patches you
need to complete that logically single topic yet, that is a
sure sign that you are not done. Instead, finish writing and
testing it, and if your test finds an earlier mistake,
especially a trivial one, go back and fix it in the earlier
patch in the series. Everybody makes mistakes so fixing up
before submission is a norm, and other people do not have to
be forced to see your "oops" in the development history.
Thanks.
Junio C Hamano wrote:
I'll be pushing out a "master" update
sometime today, and do not expect to be able to get to your "n
turned out to be ten" series, so it might be worthwhile to
reroll the remaining bits that you still care about on top of
what I push out tonight to make sure we are on the same page.
I'll wait a while if there are any comments (for example on formatting
used), and resend cleaned-up series.
Preferably:
- you should avoid making a series out of more-or-less
unrelated things;
Well, truly unrelated were adding "--" to secure against ref with
the same name as path in the repository (but it was discovered
during testing the series) and replacing ' ' with ' ' in blob
and diff body for non-CSS aware browsers.
Better quoting and unquoting was needed for better commitdiff view.
Ah, well, perhaps it is unrelated.
Securing against filenames with LF for example has sense only if
there can be filenames with LF, and earlier gitweb unquoted it halfway
leaving '\n' instead of LF.
- if you are doing related things in one series, do not send
half-baked early parts out until you are finished and are
confident with it.
I've send series early to get some comments, but I see while I got
some comments on "take 1" and "take 2" on _single_ "new commitdiff"
RFC patch, I got comments only about half-baked '&iquot;' idea.
--
Jakub Narebski