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

> > 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.

> > 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?