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

[Jakub Narebski <jnareb@gmail.com>]

On Wed, 21 Mar 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > 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.
> >
> > "W. Trevor King" <wking@drexel.edu> writes:
> >
> > > Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb
> >
> > Perhaps a better title would be:
> >
> >   gitweb: Refactor If-Modified-Since handling, support in snapshot
> 
> With "gitweb: " prefix to denote what area it affects, that is certainly
> better.  Given the primary objective and effect is that the snapshot
> feature starts honoring i-m-s,
> 
> 	gitweb: honor If-Modified-Since request header in snapshot
> 
> would be sufficient.

That is a very good title... and if all changes were to be put in single
commit (see below), that is what we should concentrate on.  Refactoring
would be just a detail.
 
> > to mention all that thispatch does.  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.
> 
> If many existing callsites had duplicated code to handle i-m-s, we may
> want two patch series, the first of which consolidates them into a single
> helper function without changing anything else (most importantly, without
> regression) and the second that uses the helper to add support in the
> snapshot feature.  But if that is not the case, I think we can go either
> way.

There is only one callsite, so theoretically we could do it in single
commit, refactoring to add support in new callsite...

...if not for the fact that control flow changes from using conditional
and early return to [longjump] "exception" based one.  That is why
I think it would be better to put tests and refactoring in a commit
separate from adding If-Modified-Since handling to 'snapshot' action.

tl;dr.  Two commits.
-- 
Jakub Narebski
Poland