Re: [PATCH 19/19] gitweb: Remove creating directory for temporary files

[Junio C Hamano <junkio@cox.net>]

Jakub Narebski <jnareb@gmail.com> 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.