Re: [PATCH 2/6] gitweb: Simplify git_diff_print

Jakub Narebski [off-list ref] writes:

Copy to temporaty file more directly, not using temporary variable @file.
Use list form of open for diff invocation (we cannot use git-diff because
first it doesn't support -L/--label option, and we cannot generate diff
between /dev/null and blob given by it's sha1 identifier). 

Use "local $/ = undef;" for (temporary) slurp mode.

Signed-off-by: Jakub Narebski <redacted>

I think this part of the gitweb code predates the git-diff
support by the core level.  If I recall correctly, back then we
did not even have "git-diff-{tree,files,index}" when this part
was written.

I think you could use "git-diff $from $to" instead of using
temporary files and running /usr/bin/diff on it with today's
git.  Which is a big win from both security and ease of
administration point of view.  Not having to write anything into
temporary file from gitweb.cgi means you do not have to worry
about stray gitweb process leaving temporary files dangling
behind, for example.

You obviously would need to emulate the -L$from_name -L$to_name
part by hand in the loop that reads from the diff output.

One worrysome pathological case is what happens if files being
tracked happen to have names that consist of 40-byte hexdigits.
I think "git-diff $from $to --" should do the right thing for
gitweb use ($from and $to should not mistaken taken as pathnames
with the explicit -- disambiguator) and "git-diff -- $from $to"
also should do the right thing to take $from and $to as pathnames
even if there are revisions with the same object names, but
somebody may want to verify that (and send patches to fix if it
is not the case).

Junio C Hamano wrote:

Jakub Narebski [off-list ref] writes:

quoted

Copy to temporaty file more directly, not using temporary variable @file.
Use list form of open for diff invocation (we cannot use git-diff because
first it doesn't support -L/--label option, and we cannot generate diff
between /dev/null and blob given by it's sha1 identifier). 

Use "local $/ = undef;" for (temporary) slurp mode.

Signed-off-by: Jakub Narebski <redacted>

I think this part of the gitweb code predates the git-diff
support by the core level.  If I recall correctly, back then we
did not even have "git-diff-{tree,files,index}" when this part
was written.

I think you could use "git-diff $from $to" instead of using
temporary files and running /usr/bin/diff on it with today's
git.  Which is a big win from both security and ease of
administration point of view.  Not having to write anything into
temporary file from gitweb.cgi means you do not have to worry
about stray gitweb process leaving temporary files dangling
behind, for example.

If I remember gitweb code correctly, diff to /dev/null is called
_only_ in commitdiff, where we can use git-diff -p to generate patch
directly, instead of generating difftree and calling diff on entries.

Still it would be nice if git-diff could diff between blob and arbitrary
file on filesystem (including /dev/null), and acquired -L/--label option.

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git