Jakub Narebski <jnareb@xxxxxxxxx> writes:
> Remove $git_temp variable which held location for temporary files
> needed by git_diff_print, and removed creating $git_temp directory.
Very good. Not writing into the filesystem even in the
temporary location is a very good thing.
Other things I noticed in this 19 series (note that I've applied
them more or less intact already, expecting that any issues will
be fixed in-tree):
* Overall
Your MUA configuration seems to have improved and stabilized
quite a bit. I did not see any corruption this time around.
* 01/19 gitweb: Use git-diff-tree patch output for commitdiff
This was a bit large for me to review during work-day, and
seemed a bit scary. It initially did not give me a good
feeling to see that a large series that followed depended on
something that was marked RFC, but it turned out mostly Ok.
* 02/19 gitweb: Replace git_commitdiff_plain by anonymous subroutine
03/19 gitweb: Show information about incomplete lines in commitdiff
Revert "gitweb: Replace git_commitdiff_plain by anonymous
subroutine"
I've commented on these.
* 06/19 gitweb: Add git_get_{following,preceding}_references functions
07/19 gitweb: Return on first ref found when
git_get_preceding_references is called in scalar context
This looks *VERY* expensive. Does git_get_references()
cache and reuse its result? How many times during a single
invocation are these subs called?
Also I am not sure about the correctness of "get-following".
B------D------F
/ base
--A------C------E
hash
You read from "rev-list $base", stop when you see $hash, and
grab all the refs that point at the rev you have seen before
stopping as "following". But in the above picture, you will
follow from F down to the very initial commit without
stopping and there actually is _no_ rev that follows E so
your result would contain B D A (if they are tagged) but none
of them follows E. There is something wrong here.
At least you should read from "rev-list $hash..$base"; then
traversal would go F D B and stop at A; you probably would
want --boundary to force showing of A as well, but even then
I am not sure how well the result would work.
"get-preceding" also wants to go down to the initial commit.
"get-following" is inherently a very expensive operation, so
I would suggest not doing this. It seems that nobody uses
these two subs yet, so probably it is better to yank them
before they cause damages.
* 08/19 gitweb: Add git_get_rev_name_tags function
09/19 gitweb: Use git_get_name_rev_tags for commitdiff_plain
X-Git-Tag: header
I suspect these make the generation of the header extremely
expensive. I'd suggest reverting them to the original.
* 13/19 gitweb: Add invisible hyperlink to from-file/to-file diff header
You seem to have forgotten esc_html() on the patch-line
before sending it to the browser. Careful.
* 14/19 gitweb: Always display link to blobdiff_plain in git_blobdiff
Need justification why this change is needed (or why previous
logic to avoid showing it in certain cases is wrong).
* 16/19 gitweb: Use git-diff-tree or git-diff patch output for blobdiff
Is git_to_hash sub always called with object names and
nothing else? "git rev-parse no-such" would die with an
error message, and "git rev-parse Makefile" in populated
working tree would say "Makefile" without complaints.
Perhaps you want --revs-only --no-flags here.
I think it is a bad style to return [] or $ in scalar context
depending on the number of results. It forces the caller to
do a conditional depending on the type of the stuff returned.
I would suggest just removing if (wantarray) and always
return @hashes. A caller who is interested in a single
element can say "($it) = your_sub(...)", a caller who wants
the number of elements can say "$cnt = your_sub(...)", and a
caller who wants to know all can say "(@them) = your_sub(...)".
I think that is the usual thing to do in Perl. Unless there
is a compelling reason that is so important that it is worth
to deviate from that norm and confusing the programmer, that
is.
I think "# try to find filename from $hash" part would
misbehave if $hash returned by git_to_hash($hash) becomes
undef.
You seem to spell out '-M', '-C' everywhere. I suspect
fixing them all to just '-C' (or perhaps '-B', '-C') would be
tedious but probably is a good idea.
* 17/19 gitweb: git_blobdiff_plain is git_blobdiff('plain')
Needs justification why commitdiff and blobdiff plain needs
to behave differently.
-
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