Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

["W. Trevor King" <wking@drexel.edu>]

[-- Attachment #1: Type: text/plain, Size: 3571 bytes --]

On Wed, Mar 21, 2012 at 06:19:51AM -0700, Jakub Narebski wrote:
> By the way, it is custom on this mailing list to usually Cc (send a
> copy) to all people participating in discussion, and not only to git
> mailing list.

Ah, sorry.  I figured that if you got the original email to the list,
I'd just be doubling up in your inbox by Cc-ing you directly.

> Though trouble with coming up with a short but fairly complete
> one-line summary might mean that this patch would be better split in
> two: refactoring and adding support for If-Modified-Since to
> snapshots.

Agreed.

> >                                  where it is appropriate
>                                    ^^^^^^^^^^^^^^^^^^^^^^^
> This doesn't add any information.  I think it the commit message would
> be better if you either remove this, or expand (in a separate
> paragraph) where support for If-Modified-Since might make sense, and
> where it could not.

I'll expand it in the refactoring patch.

> BTW. what happened to diffstat?

My local branch has sequential commits for each patch version (e.g.,
commits for v1, v2, ...).  When it's time to email the list, I'm
supposed to send logical patches against the trunk (e.g., patches for
refactoring, git_snapshot, ...).  For v2 I just used `git diff
origin/master` to generate the patch, and it doesn't include the
diffstat.  Now that I'm splitting into two patches, I'll probably use
`git rebase -i origin/master` and just keep track of the changes by
hand ;).  If there's a better way that I'm overlooking, feel free to
point me in the right direction.

> Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?
> We could use test_tick() and $test_tick for that (or extract formatted
> committer date from a commit).

I'll try to set that up.  Should it be bundled into the git_snapshot
patch, or should there be three patches:

1: gitweb: Refactor If-Modified-Since handling
2: gitweb: Add If-Modified-Since tests for git_snapshot
3: gitweb: Add If-Modified-Since support for git_snapshot

> > @@ -7029,6 +7051,10 @@ sub git_snapshot {
> >  
> >  	my ($name, $prefix) = snapshot_name($project, $hash);
> >  	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
> > +
> > +	my %co = parse_commit($hash) or die_error(404, "Unknown commit object");
> > +	die_if_unmodified($co{'committer_epoch'});
> > +
> 
> That unexplainably changes behavior of 'snapshot' action.  Before we
> would accept snapshot of a tree given by its SHA-1, now we do not.
> 
> This might be a good idea from a security point of view wrt. leaking
> information (c.f. "git archive --remote" behavior), but it at least
> needs to be explained in commit message, and perhaps even in a comment
> above this line.
> 
> Alternative solution would be to skip If-Modified-Since handling if we
> request snapshot by tree id:
> 
>   +
>   +	my %co = parse_commit($hash);
>   +	die_if_unmodified($co{'committer_epoch'}) if %co;
>   +

I'm still not understanding the problem here.  The following all work
in my testing:

  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d545cab4a8dc894fae2c2634a74993ea62b054d;sf=tgz
  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d5;sf=tgz
  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=HEAD;sf=tgz

Can you give me an example of a hash that you expect to not work?

Cheers,
Trevor

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]