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

[Jakub Narebski <jnareb@gmail.com>]

On Wed, Mar 21, 2012, W. Trevor King wrote:
> 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.

I am not subscribed to git mailing list, and I am reading it and
responding (to those emails that I'm not Cc-ed) via GMane's NNTP
(Usenet) interface:

  nntp://news.gmane.org/gmane.comp.version-control.git

And I think I am not the only one.  But I guess that even for those
subscribed, emails addressed directly to them take priority, which
is important considering volume of email on git mailing list.
 
[..]
> > 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.

There is a tool to create patches to send: git-format-patch.  Myself I
usually create a new directory for a patch series, e.g. mdir.if_mod.v3,
and use git-format-patch to populate it, e.g.

  $ git format-patch origin/master.. -o mdir.if_mod.3/ \
                     --cover-letter --subject-prefix="PATCH v4"

Note that you need to edit at least cover letter.

BTW. "git diff" has '--stat -p' and '--patch-with-stat' options :-)

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

I think it would be better to add initial tests with refactoring, and
snapshot specific tests with snapshot support, e.g.:

  1/2: gitweb: Refactor If-Modified-Since handling and add tests
  2/2: gitweb: Add If-Modified-Since support for snapshots

> > > @@ -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:

But wouldn't work in my clone... ;-(

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

Currently all of those work

    http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz
    http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz"
    http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz"

v1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree
(v1.7.6^{tree}).

The last URL would stop working after your change with 404
"Unknown commit object".

I'm not sure but I think that currently 'snapshot' link in the navbar
of the 'tree' view uses that kind of link, with 'h' parameter set to
SHA-1 of a tree.

-- 
Jakub Narebski
Poland