* What's cooking in git.git (Mar 2012, #07; Mon, 19)
@ 2012-03-20 0:23 Junio C Hamano
2012-03-15 7:54 ` [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots W. Trevor King
2012-03-20 23:35 ` Incremental updates to What's cooking Junio C Hamano
0 siblings, 2 replies; 46+ messages in thread
From: Junio C Hamano @ 2012-03-20 0:23 UTC (permalink / raw)
To: git
What's cooking in git.git (Mar 2012, #07; Mon, 19)
--------------------------------------------------
Here are the topics that have been cooking. Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.
The "git push" preparation steps (there are two topics) have enough time
to be polished to graduate to 'master' soon after 1.7.10 final.
You can find the changes described here in the integration branches of the
repositories listed at
http://git-blame.blogspot.com/p/git-public-repositories.html
--------------------------------------------------
[Graduated to "master"]
* ab/perl-i18n (2012-03-10) 3 commits
+ perl/Makefile: install Git::I18N under NO_PERL_MAKEMAKER
+ Git::I18N: compatibility with perl <5.8.3
Attempts to help installations with ancient Perl and/or without
MakeMaker.
* jc/maint-verify-objects-remove-pessimism (2012-03-15) 1 commit
(merged to 'next' on 2012-03-15 at f824530)
+ fetch/receive: remove over-pessimistic connectivity check
The code to validate the history connectivity between old refs and new
refs used by fetch and receive-pack, introduced in 1.7.8, was grossly
inefficient and unnecessarily tried to re-validate integrity of individual
objects. This essentially reverts the patch that introduced the
performance regression.
* jn/maint-fast-import-empty-ls (2012-03-09) 2 commits
(merged to 'next' on 2012-03-15 at d531079)
+ fast-import: don't allow 'ls' of path with empty components
+ fast-import: leakfix for 'ls' of dirty trees
fast-import did not diagnose "ls ''" that asks an empty path
as an error.
* sl/customize-sane-tool-path (2012-03-09) 1 commit
(merged to 'next' on 2012-03-15 at a838844)
+ configure: allow user to prevent $PATH "sanitization" on Solaris
"configure" script learned to take "--sane-tool-path" from the command
line to record SANE_TOOL_PATH (used to avoid broken platform tools in
/usr/bin) in config.mak-autogen. This may be useful for people on Solaris
who have saner tools outside /usr/xpg[46]/bin.
* th/doc-diff-submodule-option (2012-03-14) 1 commit
(merged to 'next' on 2012-03-14 at 0e1d755)
+ Documentation/diff-options: reword description of --submodule option
Update "diff --submodule" documentation.
* th/git-diffall (2012-03-14) 5 commits
(merged to 'next' on 2012-03-14 at 38e1251)
+ contrib/diffall: fix cleanup trap on Windows
+ contrib/diffall: eliminate duplicate while loops
+ contrib/diffall: eliminate use of tar
+ contrib/diffall: create tmp dirs without mktemp
+ contrib/diffall: comment actual reason for 'cdup'
Update sample "diffall" script.
* th/mergetools-deltawalker (2012-03-15) 1 commit
(merged to 'next' on 2012-03-15 at 1a62faf)
+ Documentation/difftool: add deltawalker to list of valid diff tools
--------------------------------------------------
[New Topics]
* jc/maint-clean-nested-worktree-in-subdir (2012-03-15) 2 commits
- clean: preserve nested git worktree in subdirectories
- remove_dir_recursively(): Add flag for skipping removal of toplevel dir
(this branch is tangled with jh/notes-merge-in-git-dir-worktree.)
"git clean -d -f" (not "-d -f -f") is supposed to protect nested working
trees of independent git repositories that exist in the current project
working tree from getting removed, but the protection applied only to such
working trees that are at the top-level of the current project by mistake.
Not urgent.
* ct/advise-push-default (2012-03-19) 1 commit
- push: Provide situational hints for non-fast-forward errors
Breaks down the cases in which "git push" fails due to non-ff into three
categories, and gives separate advise messages. I think there should be
three advise confvars, not just two.
* nl/rebase-i-cheat-sheet (2012-03-16) 1 commit
- [Do not merge] rebase -i: remind that the lines are top-to-bottom
Not urgent.
* da/difftool-test (2012-03-19) 1 commit
- t7800: Test difftool passing arguments to diff
Makes sure "difftool" options can be given in any order.
* th/difftool-diffall (2012-03-19) 9 commits
- difftool: print list of valid tools with '--tool-help'
- difftool: teach dir-diff to copy modified files back to working tree
- difftool: teach difftool to handle directory diffs
- difftool: replace system call with Git::command_noisy
- difftool: eliminate setup_environment function
- difftool: stop appending '.exe' to git
- difftool: remove explicit change of PATH
- difftool: exit(0) when usage is printed
- difftool: parse options using Getopt::Long
Reworks the "diffall" contrib script into "difftool" framework. Use of
Getopt::Long obviously contradicts with da/difftool-test and breaks 'pu'.
--------------------------------------------------
[Stalled]
* nd/columns (2012-03-13) 12 commits
- column: support grouping entries
- column: support "denser" mode
- ls-files: support --column
- tag: add --column
- column: support piping stdout to external git-column process
- status: add --column
- branch: add --column
- help: reuse print_columns() for help -a
- column: add dense layout support
- column: add columnar layout
- Stop starting pager recursively
- Add column layout skeleton and git-column
Rerolled again. Modulo minor nits, looked nicer than the previous round.
* nd/threaded-index-pack (2012-03-11) 2 commits
- index-pack: support multithreaded delta resolving
- index-pack: split second pass obj handling into own function
Another reroll after a bugreport on pthread usage discovered by Ramsey,
but it seems the topic is cooking between Ramsay and Duy out of tree.
Waiting for resolution.
* dg/test-from-elsewhere (2012-03-04) 2 commits
- Support out-of-tree Valgrind tests
- Allow overriding GIT_BUILD_DIR
Better support for out-of-tree test scripts, but it appears that the
approach needs to be rethought. By repointing TEST_DIRECTORY to a
directory other than $(pwd)/.., an out of place test script can reach
test helpers and freshly built Git relative to it (GIT_BUILD_DIR is
a mere short-hand for $TEST_DIRECTORY/..).
* cn/apply-fix-ws-can-lengthen-lines (2012-03-11) 1 commit
. apply: reallocate the postimage buffer when needed
Attempts to address an ancient bug that dates back to the addition
of an oddball "tab-in-indent" whitespace breakage class that wants
to have longer lines than the original when fixing things up.
Needs more work; results in double-frees.
* hv/submodule-recurse-push (2012-02-13) 3 commits
- push: teach --recurse-submodules the on-demand option
- Refactor submodule push check to use string list instead of integer
- Teach revision walking machinery to walk multiple times sequencially
The bottom one was not clearly explained and needs a reroll.
* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
- fixup! 15eaaf4
- git-svn, perl/Git.pm: extend Git::prompt helper for querying users
- perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS
The bottom one has been replaced with a rewrite based on comments
from Ævar. The second one needs more work, both in perl/Git.pm and
prompt.c, to give precedence to tty over SSH_ASKPASS when terminal
is available.
* jc/split-blob (2012-03-16) 6 commits
- chunked-object: streaming checkout
- chunked-object: fallback checkout codepaths
- bulk-checkin: support chunked-object encoding
- bulk-checkin: allow the same data to be multiply hashed
- new representation types in the packstream
- varint-in-pack: refactor varint encoding/decoding
Not ready.
I finished the streaming checkout codepath, but as explained in
127b177 (bulk-checkin: support chunked-object encoding, 2011-11-30),
these are still early steps of a long and painful journey. At least
pack-objects and fsck need to learn the new encoding for the series
to be usable locally, and then index-pack/unpack-objects needs to
learn it to be used remotely.
Given that I heard a lot of noise that people want large files, and
that I was asked by somebody at GitTogether'11 privately for an
advice on how to pay developers (not me) to help adding necessary
support, I am somewhat dissapointed that the original patch series
that was sent almost two months ago still remains here without much
comments and updates from the developer community. I even made the
interface to the logic that decides where to split chunks easily
replaceable, and I deliberately made the logic in the original patch
extremely stupid to entice others, especially the "bup" fanboys, to
come up with a better logic, thinking that giving people an easy
target to shoot for, they may be encouraged to help out. The plan is
not working :-(.
--------------------------------------------------
[Cooking]
* jh/notes-merge-in-git-dir-worktree (2012-03-15) 4 commits
- notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd
- notes-merge: use opendir/readdir instead of using read_directory()
- t3310: illustrate failure to "notes merge --commit" inside $GIT_DIR/
- remove_dir_recursively(): Add flag for skipping removal of toplevel dir
(this branch is tangled with jc/maint-clean-nested-worktree-in-subdir.)
Running "notes merge --commit" failed to perform correctly when run
from any directory inside $GIT_DIR/. When "notes merge" stops with
conflicts, $GIT_DIR/NOTES_MERGE_WORKTREE is the place a user edits
to resolve it.
Not urgent.
* jn/diffstat-tests (2012-03-13) 7 commits
- diffstat summary line varies by locale: miscellany
- test: use numstat instead of diffstat in binary-diff test
- test: use --numstat instead of --stat in "git stash show" tests
- test: test cherry-pick functionality and output separately
- test: modernize funny-names test style
- test: use numstat instead of diffstat in funny-names test
- test: use test_i18ncmp when checking --stat output
Some tests checked the "diff --stat" output when they do not have to,
which unnecessarily made things harder to verify under GETTEXT_POISON.
Not urgent.
* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits
- diff: tweak a _copy_ of diff_options with word-diff
- diff: refactor the word-diff setup from builtin_diff_cmd
- t4034: diff.*.wordregex should not be "sticky" in --word-diff
The regexp configured with wordregex was incorrectly reused across
files.
Not urgent.
* zj/test-cred-helper-nicer-prove (2012-03-15) 2 commits
- t0303: resurrect commit message as test documentation
- t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER
Minor improvement to t0303.
Not urgent.
* jc/commit-hook-authorship (2012-03-11) 3 commits
(merged to 'next' on 2012-03-12 at 05ca7f8)
+ commit: pass author/committer info to hooks
+ t7503: does pre-commit-hook learn authorship?
+ ident.c: add split_ident_line() to parse formatted ident line
(this branch is tangled with jc/run-hook-env-1.)
"git commit --author=$name" did not tell the name that was being
recorded in the resulting commit to hooks, even though it does do so
when the end user overrode the authorship via the "GIT_AUTHOR_NAME"
environment variable. This is a simpler of the two approaches.
Will defer til 1.7.10.
* jc/run-hook-env-1 (2012-03-11) 3 commits
- run_hook(): enhance the interface to pass arbitrary environment
+ t7503: does pre-commit-hook learn authorship?
+ ident.c: add split_ident_line() to parse formatted ident line
(this branch is tangled with jc/commit-hook-authorship.)
Not urgent.
Updates run_hook() API to be much less specific to "commit". It would
only be useful if people start doing more interesting things with hooks.
* jc/diff-algo-cleanup (2012-02-19) 2 commits
(merged to 'next' on 2012-03-15 at cca0032)
+ xdiff: PATIENCE/HISTOGRAM are not independent option bits
+ xdiff: remove XDL_PATCH_* macros
(this branch is used by jc/diff-ignore-case.)
Resurrects the preparatory clean-up patches from another topic that was
discarded, as this would give a saner foundation to build on diff.algo
configuration option series.
Not urgent.
* jh/apply-free-patch (2012-03-07) 1 commit
- apply: do not leak patches and fragments
Will defer til 1.7.10.
* rs/unpack-trees-leakfix (2012-03-06) 1 commit
(merged to 'next' on 2012-03-07 at 69a69cd)
+ unpack-trees: plug minor memory leak
Will defer til 1.7.10.
* tb/maint-remove-irrelevant-i18n-test (2012-03-06) 1 commit
(merged to 'next' on 2012-03-07 at 23f2dd1)
+ t0204: remove a test that checks undefined behaviour
I tentatively parked this in 'next' but later reverted the merge.
Will discard.
* mm/push-default-switch-warning (2012-03-09) 1 commit
- push: start warning upcoming default change for push.default
Not urgent.
This resurrects an ancient patch I wrote during a discussion we had in the
1.6.3-1.6.4 era. This should probably come after ct/advise-push-default
topic and at that point the advise messages need to be rephrased, taking
the future default change into account.
* jc/fmt-merge-msg-people (2012-03-13) 1 commit
- fmt-merge-msg: show those involved in a merged series
The "fmt-merge-msg" command learns to list the primary contributors
involved in the side topic you are merging.
Will defer til 1.7.10.
* nl/http-proxy-more (2012-03-15) 5 commits
- http: rename HTTP_REAUTH to HTTP_AUTH_RETRY
- http: Avoid limit of retrying request only twice
- http: handle proxy authentication failure (error 407)
- http: handle proxy proactive authentication
- http: try http_proxy env var when http.proxy config option is not set
The code to talk to http proxies learn to use the same credential
API used to talk to the final http destinations.
Will defer til 1.7.10.
* nd/stream-more (2012-03-07) 7 commits
(merged to 'next' on 2012-03-07 at 7325922)
+ update-server-info: respect core.bigfilethreshold
+ fsck: use streaming API for writing lost-found blobs
+ show: use streaming API for showing blobs
+ parse_object: avoid putting whole blob in core
+ cat-file: use streaming API to print blobs
+ Add more large blob test cases
+ streaming: make streaming-write-entry to be more reusable
Use API to read blob data in smaller chunks in more places to
reduce the memory footprint. In general, looked fairly good.
Will defer til 1.7.10.
--------------------------------------------------
[Discarded]
* jc/diff-ignore-mode (2012-03-01) 1 commit
. diff --ignore-mode-change
* jc/diff-ignore-case (2012-02-28) 6 commits
. diff: -i is "--ignore-case" but means a bit more in "log"
. diff: --ignore-case
. xdiff: introduce XDF_IGNORE_CASE
. xdiff: introduce XDF_INEXACT_MATCH
- xdiff: PATIENCE/HISTOGRAM are not independent option bits
- xdiff: remove XDL_PATCH_* macros
* jh/trace-use-startup-info (2012-03-02) 1 commit
. Use startup_info->prefix rather than prefix.
I tend to agree with the doubt of the author of this patch expressed.
* nd/optim-connected (2012-03-15) 1 commit
. {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack
jc/maint-verify-objects-remove-pessimism replaces this.
* jc/advise-push-default (2011-12-18) 1 commit
. push: hint to use push.default=upstream when appropriate
A rework by Christopher Tiwald replaces this.
* th/mergetools-tool-help (2012-03-15) 1 commit
. difftool: print list of valid tools with '--tool-help'
Reworked as part of th/difftool-diffall topic.
^ permalink raw reply [flat|nested] 46+ messages in thread* [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. @ 2012-03-15 7:54 ` W. Trevor King 2012-03-20 1:48 ` W. Trevor King 2012-03-20 11:55 ` Jakub Narebski 0 siblings, 2 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-15 7:54 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 3587 bytes --] The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function modified_since. This makes the code easy to reuse for other actions where it is appropriate, and I've added the code to do that in git_snapshot. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 54 ++++++++++++++++++++++++++++++++++++--------------- 1 files changed, 38 insertions(+), 16 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a8b5fad..96a13e7 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,31 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub modified_since { + my ($content_type, $latest_epoch, $timezone) = @_; + our $cgi; + + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); + if (defined $if_modified) { + my $since; + if (eval { require HTTP::Date; 1; }) { + $since = HTTP::Date::str2time($if_modified); + } elsif (eval { require Time::ParseDate; 1; }) { + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); + } + if (defined $since && $latest_epoch <= $since) { + my %latest_date = parse_date($latest_epoch, $timezone); + print $cgi->header( + -type => $content_type, + -charset => 'utf-8', + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + return 0; + } + } + return 1; +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -7029,6 +7054,14 @@ 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"); + if (! modified_since($known_snapshot_formats{$format}{'type'}, + $co{'committer_epoch'}, + $co{'committer_tz'})) { + return; + } + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7038,9 +7071,11 @@ sub git_snapshot { } $filename =~ s/(["\\])/\\$1/g; + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); print $cgi->header( -type => $known_snapshot_formats{$format}{'type'}, -content_disposition => 'inline; filename="' . $filename . '"', + -last_modified => $latest_date{'rfc2822'}, -status => '200 OK'); open my $fd, "-|", $cmd @@ -7821,22 +7856,9 @@ sub git_feed { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_epoch'}; %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); - if (defined $if_modified) { - my $since; - if (eval { require HTTP::Date; 1; }) { - $since = HTTP::Date::str2time($if_modified); - } elsif (eval { require Time::ParseDate; 1; }) { - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); - } - if (defined $since && $latest_epoch <= $since) { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '304 Not Modified'); - return; - } + if (! modified_since($content_type, $latest_epoch, + $latest_commit{'comitter_tz'})) { + return; } print $cgi->header( -type => $content_type, -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. 2012-03-15 7:54 ` [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots W. Trevor King @ 2012-03-20 1:48 ` W. Trevor King 2012-03-20 23:07 ` Junio C Hamano 2012-03-20 11:55 ` Jakub Narebski 1 sibling, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-20 1:48 UTC (permalink / raw) To: git, Junio C Hamano [-- Attachment #1: Type: text/plain, Size: 664 bytes --] On Thu, Mar 15, 2012 at 03:54:07AM -0400, W. Trevor King wrote: > The current gitweb only generates Last-Modified and handles > If-Modified-Since headers for the git_feed action. This patch… On Mon, Mar 19, 2012 at 05:23:07PM -0700, Junio C Hamano wrote: > What's cooking in git.git (Mar 2012, #07; Mon, 19) > … My If-Modified-Since patch seems to have fallen through the cracks here. Is that because nobody is interested, or did I mess up something in the submission? 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. 2012-03-20 1:48 ` W. Trevor King @ 2012-03-20 23:07 ` Junio C Hamano 0 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2012-03-20 23:07 UTC (permalink / raw) To: W. Trevor King; +Cc: git "W. Trevor King" <wking@drexel.edu> writes: > My If-Modified-Since patch seems to have fallen through the cracks > here. Is that because nobody is interested, or did I mess up > something in the submission? Probably "just fell through the cracks"; if somebody found what it wants to do very interesting but the implementation or submission lacking, you would have heard loud noises. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. 2012-03-15 7:54 ` [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots W. Trevor King 2012-03-20 1:48 ` W. Trevor King @ 2012-03-20 11:55 ` Jakub Narebski 2012-03-20 16:40 ` [PATCH v2] " W. Trevor King 2012-03-26 17:36 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King 1 sibling, 2 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-20 11:55 UTC (permalink / raw) To: W. Trevor King; +Cc: git I'm sorry I am only now answering, but unfortunately this patch has fallen thought crack. "W. Trevor King" <wking@drexel.edu> writes: > The current gitweb only generates Last-Modified and handles > If-Modified-Since headers for the git_feed action. This patch breaks > the Last-Modified and If-Modified-Since handling code out from > git_feed into a new function modified_since. This makes the code easy > to reuse for other actions where it is appropriate, That is a very good idea. Of course adding support for If-Modified-Since has sense only if we can calculate modification time without doing all the work (and calculating modification time separately doesn;t add to work to be done), or at least we are to send large amount of data so even if we cannot save CPU time and I/O hit we can save network bandwidth. > and I've added the code to do that in git_snapshot. Nitpick about grammar: here you change from impersonal to personal form in commit message, unnecessarily I think. > Signed-off-by: W Trevor King <wking@drexel.edu> > > --- > gitweb/gitweb.perl | 54 ++++++++++++++++++++++++++++++++++++--------------- > 1 files changed, 38 insertions(+), 16 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index a8b5fad..96a13e7 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -7003,6 +7003,31 @@ sub snapshot_name { > return wantarray ? ($name, $name) : $name; > } > > +sub modified_since { > + my ($content_type, $latest_epoch, $timezone) = @_; Passing $content_type to function named modified_since() looks quite strange to me. > + our $cgi; > + > + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > + if (defined $if_modified) { > + my $since; > + if (eval { require HTTP::Date; 1; }) { > + $since = HTTP::Date::str2time($if_modified); > + } elsif (eval { require Time::ParseDate; 1; }) { > + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > + } > + if (defined $since && $latest_epoch <= $since) { > + my %latest_date = parse_date($latest_epoch, $timezone); Shouldn't we pass \%latest_date (or rather \%modification_date to be more generic)? Ah, I see that parse_date() subroutine does not store original epoch not original timezone. By the way, for $latest_date{'rfc2822'} we don't need $timezone argument, and I think we should not pass it to generic modified_since() function (or whatever it will be called). > + print $cgi->header( > + -type => $content_type, > + -charset => 'utf-8', Do we need -charset here? There is no HTTP body, and -charset => 'utf-8' is incorrect for snapshots (with e.g. 'application/x-zip' $content_type) > + -last_modified => $latest_date{'rfc2822'}, > + -status => '304 Not Modified'); > + return 0; > + } > + } > + return 1; > +} > + > sub git_snapshot { > my $format = $input_params{'snapshot_format'}; > if (!@snapshot_fmts) { > @@ -7029,6 +7054,14 @@ 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"); > + if (! modified_since($known_snapshot_formats{$format}{'type'}, > + $co{'committer_epoch'}, > + $co{'committer_tz'})) { > + return; > + } For a function named modified_since(), it does too many things. I think a better idea would be to make this function/subroutine (named e.g. if_modified_since() or something like that) to work more like die_error() -- it should simply end request instead of having caller to use complcated calling convention, and care about eraly termination by itself. This means turning modified_since() into subroutine (and renaming it), and ending it with goto DONE_GITWEB; or goto DONE_REQUEST; depending on the code base you are applying to. > + > my $cmd = quote_command( > git_cmd(), 'archive', > "--format=$known_snapshot_formats{$format}{'format'}", > @@ -7038,9 +7071,11 @@ sub git_snapshot { > } > > $filename =~ s/(["\\])/\\$1/g; > + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > print $cgi->header( > -type => $known_snapshot_formats{$format}{'type'}, > -content_disposition => 'inline; filename="' . $filename . '"', > + -last_modified => $latest_date{'rfc2822'}, > -status => '200 OK'); > Have you run gitweb tests? This simply has no way of working, as %co variable you use here is not defined anywhere in git_snapshot() subroutine. What if there is no commit, for example if we are rewuesting snapshot of a tree by its SHA-1? We need to be able to deal with sich situation, if only by not handling Last-Modified / If-Modified-Since. > open my $fd, "-|", $cmd > @@ -7821,22 +7856,9 @@ sub git_feed { > %latest_commit = %{$commitlist[0]}; > my $latest_epoch = $latest_commit{'committer_epoch'}; > %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); Do we use %latest_date beside handling If-Modified-Since? Is there a need for separate $latest_epoch and %;atest_date variables? > - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > - if (defined $if_modified) { > - my $since; > - if (eval { require HTTP::Date; 1; }) { > - $since = HTTP::Date::str2time($if_modified); > - } elsif (eval { require Time::ParseDate; 1; }) { > - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > - } > - if (defined $since && $latest_epoch <= $since) { > - print $cgi->header( > - -type => $content_type, > - -charset => 'utf-8', > - -last_modified => $latest_date{'rfc2822'}, > - -status => '304 Not Modified'); > - return; > - } > + if (! modified_since($content_type, $latest_epoch, > + $latest_commit{'comitter_tz'})) { > + return; > } > print $cgi->header( > -type => $content_type, > -- > 1.7.3.4 -- Jakub Narebski ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v2] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. 2012-03-20 11:55 ` Jakub Narebski @ 2012-03-20 16:40 ` W. Trevor King 2012-03-21 12:11 ` [PATCH v3] Isolate If-Modified-Since handling in gitweb W. Trevor King 2012-03-26 17:36 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King 1 sibling, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-20 16:40 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 5316 bytes --] The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function modified_since. This makes the code easy to reuse for other actions where it is appropriate and adds the code to do that to git_snapshot. Signed-off-by: W Trevor King <wking@drexel.edu> --- Thanks for the feedback, Jakub. I believe I've addressed your points in this revised patch. Changed since v1: - Fixed impersonal/personal commit message - Simplified modified_since() -> die_if_unmodified(). Now it only takes a $latest_epoch argument and we set fewer headers. It also jump straight to DONE_GITWEB if the page is unmodified. - Ran tests (which passed this time). Somehow I missed t/README earlier. I'd add tests for my patch, but I'd have to have commit timestamps from the test repository, and I don't understand the test framework well enough to know how to do that cleanly. Things that haven't changed: > > @@ -7038,9 +7071,11 @@ sub git_snapshot { > > } > > > > $filename =~ s/(["\\])/\\$1/g; > > + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > ... > This simply has no way of working, as %co variable you use here is > not defined anywhere in git_snapshot() subroutine. I defined it earlier: > > @@ -7029,6 +7054,14 @@ 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"); > > + if (! modified_since($known_snapshot_formats{$format}{'type'}, > > + $co{'committer_epoch'}, While modified_since() (now die_if_unmodified()) handles the 304 case, we also want to set Last-Modified for 200s. That's what the latter code is trying to do. > What if there is no commit, for example if we are rewuesting snapshot > of a tree by its SHA-1? We need to be able to deal with sich > situation, if only by not handling Last-Modified / If-Modified-Since. The parse_commit() call should handle these cases. diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a8b5fad..b944351 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,28 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub die_if_unmodified { + my ($latest_epoch) = @_; + our $cgi; + + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); + if (defined $if_modified) { + my $since; + if (eval { require HTTP::Date; 1; }) { + $since = HTTP::Date::str2time($if_modified); + } elsif (eval { require Time::ParseDate; 1; }) { + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); + } + if (defined $since && $latest_epoch <= $since) { + my %latest_date = parse_date($latest_epoch); + print $cgi->header( + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + goto DONE_GITWEB; + } + } +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -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'}); + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7038,9 +7064,11 @@ sub git_snapshot { } $filename =~ s/(["\\])/\\$1/g; + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); print $cgi->header( -type => $known_snapshot_formats{$format}{'type'}, -content_disposition => 'inline; filename="' . $filename . '"', + -last_modified => $latest_date{'rfc2822'}, -status => '200 OK'); open my $fd, "-|", $cmd @@ -7820,24 +7848,8 @@ sub git_feed { if (defined($commitlist[0])) { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_epoch'}; + die_if_unmodified($latest_epoch); %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); - if (defined $if_modified) { - my $since; - if (eval { require HTTP::Date; 1; }) { - $since = HTTP::Date::str2time($if_modified); - } elsif (eval { require Time::ParseDate; 1; }) { - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); - } - if (defined $since && $latest_epoch <= $since) { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '304 Not Modified'); - return; - } - } print $cgi->header( -type => $content_type, -charset => 'utf-8', -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-20 16:40 ` [PATCH v2] " W. Trevor King @ 2012-03-21 12:11 ` W. Trevor King 2012-03-21 13:19 ` Jakub Narebski 0 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-21 12:11 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 3478 bytes --] The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function die_if_unmodified. This makes the code easy to reuse for other actions where it is appropriate and adds the code to do that to git_snapshot. Signed-off-by: W Trevor King <wking@drexel.edu> --- Changed since v2: - Shorter title. - Fixed modified_since -> die_if_unmodified in commit message. diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a8b5fad..b944351 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,28 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub die_if_unmodified { + my ($latest_epoch) = @_; + our $cgi; + + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); + if (defined $if_modified) { + my $since; + if (eval { require HTTP::Date; 1; }) { + $since = HTTP::Date::str2time($if_modified); + } elsif (eval { require Time::ParseDate; 1; }) { + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); + } + if (defined $since && $latest_epoch <= $since) { + my %latest_date = parse_date($latest_epoch); + print $cgi->header( + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + goto DONE_GITWEB; + } + } +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -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'}); + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7038,9 +7064,11 @@ sub git_snapshot { } $filename =~ s/(["\\])/\\$1/g; + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); print $cgi->header( -type => $known_snapshot_formats{$format}{'type'}, -content_disposition => 'inline; filename="' . $filename . '"', + -last_modified => $latest_date{'rfc2822'}, -status => '200 OK'); open my $fd, "-|", $cmd @@ -7820,24 +7848,8 @@ sub git_feed { if (defined($commitlist[0])) { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_epoch'}; + die_if_unmodified($latest_epoch); %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); - if (defined $if_modified) { - my $since; - if (eval { require HTTP::Date; 1; }) { - $since = HTTP::Date::str2time($if_modified); - } elsif (eval { require Time::ParseDate; 1; }) { - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); - } - if (defined $since && $latest_epoch <= $since) { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '304 Not Modified'); - return; - } - } print $cgi->header( -type => $content_type, -charset => 'utf-8', -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 12:11 ` [PATCH v3] Isolate If-Modified-Since handling in gitweb W. Trevor King @ 2012-03-21 13:19 ` Jakub Narebski 2012-03-21 14:04 ` W. Trevor King 2012-03-21 17:21 ` Junio C Hamano 0 siblings, 2 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-21 13:19 UTC (permalink / raw) To: W. Trevor King; +Cc: git 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 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. > The current gitweb only generates Last-Modified and handles > If-Modified-Since headers for the git_feed action. This patch breaks > the Last-Modified and If-Modified-Since handling code out from > git_feed into a new function die_if_unmodified. This makes the code > easy to reuse for other actions O.K. > 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. > and adds the > code to do that to git_snapshot. This would read better as a separate paragraph. > Signed-off-by: W Trevor King <wking@drexel.edu> > --- > Changed since v2: > - Shorter title. > - Fixed modified_since -> die_if_unmodified in commit message. Nice. BTW. what happened to diffstat? 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). > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index a8b5fad..b944351 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -7003,6 +7003,28 @@ sub snapshot_name { > return wantarray ? ($name, $name) : $name; > } > > +sub die_if_unmodified { > + my ($latest_epoch) = @_; > + our $cgi; > + > + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > + if (defined $if_modified) { > + my $since; > + if (eval { require HTTP::Date; 1; }) { > + $since = HTTP::Date::str2time($if_modified); > + } elsif (eval { require Time::ParseDate; 1; }) { > + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > + } > + if (defined $since && $latest_epoch <= $since) { > + my %latest_date = parse_date($latest_epoch); > + print $cgi->header( > + -last_modified => $latest_date{'rfc2822'}, > + -status => '304 Not Modified'); > + goto DONE_GITWEB; > + } > + } > +} O.K. die_if_unmodified() is good enough name, though I wonder if we could come up with a better name.... > + > sub git_snapshot { > my $format = $input_params{'snapshot_format'}; > if (!@snapshot_fmts) { > @@ -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; + > @@ -7038,9 +7064,11 @@ sub git_snapshot { > } > > $filename =~ s/(["\\])/\\$1/g; > + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > print $cgi->header( > -type => $known_snapshot_formats{$format}{'type'}, > -content_disposition => 'inline; filename="' . $filename . '"', > + -last_modified => $latest_date{'rfc2822'}, > -status => '200 OK'); Of course this would need to be updated too. > open my $fd, "-|", $cmd > @@ -7820,24 +7848,8 @@ sub git_feed { > if (defined($commitlist[0])) { > %latest_commit = %{$commitlist[0]}; > my $latest_epoch = $latest_commit{'committer_epoch'}; > + die_if_unmodified($latest_epoch); > %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); > - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > - if (defined $if_modified) { > - my $since; > - if (eval { require HTTP::Date; 1; }) { > - $since = HTTP::Date::str2time($if_modified); > - } elsif (eval { require Time::ParseDate; 1; }) { > - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > - } > - if (defined $since && $latest_epoch <= $since) { > - print $cgi->header( > - -type => $content_type, > - -charset => 'utf-8', > - -last_modified => $latest_date{'rfc2822'}, > - -status => '304 Not Modified'); > - return; > - } > - } > print $cgi->header( > -type => $content_type, > -charset => 'utf-8', Nice. -- Jakub Narebski ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 13:19 ` Jakub Narebski @ 2012-03-21 14:04 ` W. Trevor King 2012-03-21 16:55 ` Jakub Narebski 2012-03-21 17:21 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-21 14:04 UTC (permalink / raw) To: git, Jakub Narebski [-- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 14:04 ` W. Trevor King @ 2012-03-21 16:55 ` Jakub Narebski 2012-03-21 17:38 ` W. Trevor King 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narebski @ 2012-03-21 16:55 UTC (permalink / raw) To: W. Trevor King; +Cc: git 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 16:55 ` Jakub Narebski @ 2012-03-21 17:38 ` W. Trevor King 2012-03-21 19:22 ` Junio C Hamano 2012-03-22 13:05 ` Jakub Narebski 0 siblings, 2 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-21 17:38 UTC (permalink / raw) To: Jakub Narebski; +Cc: git [-- Attachment #1: Type: text/plain, Size: 1551 bytes --] On Wed, Mar 21, 2012 at 05:55:06PM +0100, Jakub Narebski wrote: > 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. Ah, I like that. Then I can rebase away ;). > 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 But the new tests would be for the new functionality (i.e. snapshot support), so they wouldn't belong in the general refactoring commit. > 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". Indeed it does. I'll add you're suggested skipping for these cases. There should be a way to determine the oldest commit refering to a tree, which could be used for timestamping tree-ishes, but that can be a project for another day ;). -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 17:38 ` W. Trevor King @ 2012-03-21 19:22 ` Junio C Hamano 2012-03-21 19:55 ` W. Trevor King 2012-03-22 13:05 ` Jakub Narebski 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2012-03-21 19:22 UTC (permalink / raw) To: W. Trevor King; +Cc: Jakub Narebski, git "W. Trevor King" <wking@drexel.edu> writes: >> 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 > > But the new tests would be for the new functionality (i.e. snapshot > support), so they wouldn't belong in the general refactoring commit. Then you are planning to split it in a wrong way. As I said, I do not think it matters that much for a small patch like this, but if the plan is to make the part to create i-m-s helper function as a standalone "refactoring" patch, then what Jakub outlined is the right way to go about it. In the first patch, you create i-m-s helper and update the existing code that can use the helper, without changing anything else. Do not touch snapshot code in this patch, if the current code does not support i-m-s in snapshot. And in the same patch, add tests for codepaths that use i-m-s to make sure your refactoring did not break them. In other words, if you remove the change to gitweb/ from the first patch and apply only the changes to the tests, the resulting new tests should pass with the current code that has i-m-s support inline without the i-m-s helper. And if you add back your change to gitweb/ for the refactoring, the test should still pass. And then in the second patch, you update the snapshot code and whatever else that can use i-m-s helper to support i-m-s. You can add tests to protect the new feature from future breakages in this patch. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 19:22 ` Junio C Hamano @ 2012-03-21 19:55 ` W. Trevor King 2012-03-21 20:04 ` Jakub Narebski 0 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-21 19:55 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jakub Narebski, git [-- Attachment #1: Type: text/plain, Size: 1300 bytes --] On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote: > "W. Trevor King" <wking@drexel.edu> writes: > >> 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 > > > > But the new tests would be for the new functionality (i.e. snapshot > > support), so they wouldn't belong in the general refactoring commit. > > Then you are planning to split it in a wrong way. > > ... add tests for codepaths that use i-m-s to make sure your > refactoring did not break them... Ah, I was assuming that some current tests might be checking the current behavior, and that my new tests would be testing my new snapshot behavior. If the old i-m-s handling also needs tests, that should happen before any of my previously proposed patches: 1: tests for i-m-s and git_feed 2: refactor i-m-s handling 3: tests for i-m-s and git_snapshot (which fail until 4) 4: add i-m-s to git_snapshot How does that sound? -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 19:55 ` W. Trevor King @ 2012-03-21 20:04 ` Jakub Narebski 2012-03-21 20:09 ` Junio C Hamano 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narebski @ 2012-03-21 20:04 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git W. Trevor King wrote: > On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote: > > "W. Trevor King" <wking@drexel.edu> writes: > > > > 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 > > > > > > But the new tests would be for the new functionality (i.e. snapshot > > > support), so they wouldn't belong in the general refactoring commit. > > > > Then you are planning to split it in a wrong way. > > > > ... add tests for codepaths that use i-m-s to make sure your > > refactoring did not break them... > > Ah, I was assuming that some current tests might be checking the > current behavior, and that my new tests would be testing my new > snapshot behavior. If the old i-m-s handling also needs tests, that > should happen before any of my previously proposed patches: > > 1: tests for i-m-s and git_feed > 2: refactor i-m-s handling Those two can be in single commit. Tests added need only test i-m-s using git_feed ('atom' or 'rss' action), as it is the only user, and you touch only i-m-s handling. > 3: tests for i-m-s and git_snapshot (which fail until 4) > 4: add i-m-s to git_snapshot We usually put tests together with feature. Tests before feature means that you would need to mark them as test_expect_failure, as they would not pass before feature is added, isn't it? -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 20:04 ` Jakub Narebski @ 2012-03-21 20:09 ` Junio C Hamano 2012-03-21 20:34 ` W. Trevor King 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2012-03-21 20:09 UTC (permalink / raw) To: Jakub Narebski; +Cc: W. Trevor King, git Jakub Narebski <jnareb@gmail.com> writes: >> 1: tests for i-m-s and git_feed >> 2: refactor i-m-s handling > > Those two can be in single commit. Tests added need only test i-m-s > using git_feed ('atom' or 'rss' action), as it is the only user, > and you touch only i-m-s handling. Correct. >> 3: tests for i-m-s and git_snapshot (which fail until 4) >> 4: add i-m-s to git_snapshot > > We usually put tests together with feature. Tests before feature means > that you would need to mark them as test_expect_failure, as they would > not pass before feature is added, isn't it? Correct. In a more elaborate series, you would first add a lot of tests to nail the behaviour of the existing code down, then refactor or reimplement them in a different way, add tests that expect failure until the next phase, and then add features and turn expect_failure in tests to expect_success. But I do not htink this single patch deserves that complexity. Does it even have to split into three? ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 20:09 ` Junio C Hamano @ 2012-03-21 20:34 ` W. Trevor King 0 siblings, 0 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-21 20:34 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jakub Narebski, git [-- Attachment #1: Type: text/plain, Size: 788 bytes --] On Wed, Mar 21, 2012 at 01:09:24PM -0700, Junio C Hamano wrote: > In a more elaborate series, you would first add a lot of tests to nail the > behaviour of the existing code down, then refactor or reimplement them in > a different way, add tests that expect failure until the next phase, and > then add features and turn expect_failure in tests to expect_success. > > But I do not htink this single patch deserves that complexity. Does it > even have to split into three? Sorry, testing makes me think of test-driven development ;). Two patches it is (refactor + git_feed tests, git_snapshot + git_snapshot tests). -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 17:38 ` W. Trevor King 2012-03-21 19:22 ` Junio C Hamano @ 2012-03-22 13:05 ` Jakub Narebski 1 sibling, 0 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-22 13:05 UTC (permalink / raw) To: W. Trevor King; +Cc: git On Wed, 21 Mar 2012, W. Trevor King wrote: > On Wed, Mar 21, 2012 at 05:55:06PM +0100, Jakub Narebski wrote: > > 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. > > Ah, I like that. Then I can rebase away ;). Yep. Or use some patch / mailqueue management interface like StGit. > > 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 > > But the new tests would be for the new functionality (i.e. snapshot > support), so they wouldn't belong in the general refactoring commit. I was thinking that "and add tests" would be about 'feed' action test and i-m-s. > > 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". > > Indeed it does. I'll add you're suggested skipping for these cases. If by "skipping" you mean here turning off handling of If-Modified-Since and Last-Modified headers, then I agree. > There should be a way to determine the oldest commit refering to a > tree, which could be used for timestamping tree-ishes, but that can be > a project for another day ;). That also can be ambiguous. -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 13:19 ` Jakub Narebski 2012-03-21 14:04 ` W. Trevor King @ 2012-03-21 17:21 ` Junio C Hamano 2012-03-22 12:46 ` Jakub Narebski 1 sibling, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2012-03-21 17:21 UTC (permalink / raw) To: Jakub Narebski; +Cc: W. Trevor King, git 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. > 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. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-21 17:21 ` Junio C Hamano @ 2012-03-22 12:46 ` Jakub Narebski 2012-03-22 17:00 ` Junio C Hamano 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King 0 siblings, 2 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-22 12:46 UTC (permalink / raw) To: Junio C Hamano; +Cc: W. Trevor King, git 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 ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb 2012-03-22 12:46 ` Jakub Narebski @ 2012-03-22 17:00 ` Junio C Hamano 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2012-03-22 17:00 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, W. Trevor King, git Jakub Narebski <jnareb@gmail.com> writes: > tl;dr. Two commits. OK, thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb 2012-03-22 12:46 ` Jakub Narebski 2012-03-22 17:00 ` Junio C Hamano @ 2012-03-26 11:09 ` W. Trevor King 2012-03-26 11:11 ` [PATCH v4 1/3] gitweb: add `status` headers to git_feed() responses W. Trevor King ` (3 more replies) 1 sibling, 4 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-26 11:09 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 1344 bytes --] Sorry for the delay since my last message, it's been a busy week ;). On Thu, Mar 22, 2012 at 01:46:34PM +0100, Jakub Narebski wrote: > ...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. I'll be sending along three patches. The second and third are the ones you discuss above. The first is a teensy patch to add `Status` output to non-304 calls to git_feed(). Without it you'd have to get a bit more creative in the test suite. If the status lines were left out intentionally, let me know, and I'll come up with another condition for those tests. The second patch refactors and tests git_feed(), and the third patch adds i-m-s support to git_snapshot() with associated tests. Changes since v3: * Patch 1/3 is completely new. * Split previous patch into 2/3 and 3/3. * Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}. * Added testing to both 2/3 and 3/3. * Reworked commit messages. 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 1/3] gitweb: add `status` headers to git_feed() responses. 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King @ 2012-03-26 11:11 ` W. Trevor King 2012-03-26 11:12 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-26 11:11 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 971 bytes --] The git_feed() method was not setting a `Status` header unless it was responding to an If-Modified-Since request with `304 Not Modified`. Now, when it is serving successful responses, it sets status to `200 OK`. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a8b5fad..041da17 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7841,11 +7841,13 @@ sub git_feed { print $cgi->header( -type => $content_type, -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}); + -last_modified => $latest_date{'rfc2822'}, + -status => '200 OK'); } else { print $cgi->header( -type => $content_type, - -charset => 'utf-8'); + -charset => 'utf-8', + -status => '200 OK'); } # Optimization: skip generating the body if client asks only -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King 2012-03-26 11:11 ` [PATCH v4 1/3] gitweb: add `status` headers to git_feed() responses W. Trevor King @ 2012-03-26 11:12 ` W. Trevor King 2012-03-26 17:12 ` Junio C Hamano 2012-03-26 11:13 ` [PATCH v4 3/3] gitweb: add If-Modified-Since handling to git_snapshot() W. Trevor King 2012-03-27 19:24 ` [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb Jakub Narebski 3 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-26 11:12 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 4546 bytes --] The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function die_if_unmodified. This makes the code easy to reuse for other actions. Only gitweb actions which can easily calculate a modification time should use die_if_unmodified, as the goal is to balance local processing time vs. upload bandwidth. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 40 +++++++++++++++++------------ t/t9501-gitweb-standalone-http-status.sh | 27 +++++++++++++++++++- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 041da17..229f3da 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,28 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub die_if_unmodified { + my ($latest_epoch) = @_; + our $cgi; + + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); + if (defined $if_modified) { + my $since; + if (eval { require HTTP::Date; 1; }) { + $since = HTTP::Date::str2time($if_modified); + } elsif (eval { require Time::ParseDate; 1; }) { + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); + } + if (defined $since && $latest_epoch <= $since) { + my %latest_date = parse_date($latest_epoch); + print $cgi->header( + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + goto DONE_GITWEB; + } + } +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -7820,24 +7842,8 @@ sub git_feed { if (defined($commitlist[0])) { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_epoch'}; + die_if_unmodified($latest_epoch); %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); - if (defined $if_modified) { - my $since; - if (eval { require HTTP::Date; 1; }) { - $since = HTTP::Date::str2time($if_modified); - } elsif (eval { require Time::ParseDate; 1; }) { - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); - } - if (defined $since && $latest_epoch <= $since) { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '304 Not Modified'); - return; - } - } print $cgi->header( -type => $content_type, -charset => 'utf-8', diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh index 31076ed..0e49f29 100755 --- a/t/t9501-gitweb-standalone-http-status.sh +++ b/t/t9501-gitweb-standalone-http-status.sh @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output' test_expect_success 'snapshots: bad tree-ish id (tagged object)' ' echo object > tag-object && git add tag-object && - git commit -m "Object to be tagged" && + test_tick && git commit -m "Object to be tagged" && git tag tagged-object `git hash-object tag-object` && gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" && grep "400 - Object is not a tree-ish" gitweb.output @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' ' ' test_debug 'cat gitweb.output' +# ---------------------------------------------------------------------- +# modification times (Last-Modified and If-Modified-Since) + +test_expect_success 'modification: feed last-modified' ' + gitweb_run "p=.git;a=atom;h=master" && + grep "Status: 200 OK" gitweb.output + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: feed if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=atom;h=master" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 200 OK" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: feed if-modified-since (unmodified)' ' + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=atom;h=master" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 304 Not Modified" gitweb.output +' +test_debug 'cat gitweb.headers' # ---------------------------------------------------------------------- # load checking -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling 2012-03-26 11:12 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King @ 2012-03-26 17:12 ` Junio C Hamano 0 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2012-03-26 17:12 UTC (permalink / raw) To: W. Trevor King; +Cc: Jakub Narebski, git "W. Trevor King" <wking@drexel.edu> writes: > +sub die_if_unmodified { > + my ($latest_epoch) = @_; > + our $cgi; > + > + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > + if (defined $if_modified) { > + my $since; > + if (eval { require HTTP::Date; 1; }) { > + $since = HTTP::Date::str2time($if_modified); > + } elsif (eval { require Time::ParseDate; 1; }) { > + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > + } > + if (defined $since && $latest_epoch <= $since) { > + my %latest_date = parse_date($latest_epoch); > + print $cgi->header( > + -last_modified => $latest_date{'rfc2822'}, > + -status => '304 Not Modified'); > + goto DONE_GITWEB; > + } > + } > +} > sub git_snapshot { > my $format = $input_params{'snapshot_format'}; > if (!@snapshot_fmts) { > @@ -7820,24 +7842,8 @@ sub git_feed { > if (defined($commitlist[0])) { > %latest_commit = %{$commitlist[0]}; > my $latest_epoch = $latest_commit{'committer_epoch'}; > + die_if_unmodified($latest_epoch); > %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); It might make more sense to call the helper *after* stuffing %latest_date and pass the $latest_date{'rfc2822'} as parameter to the new helper function, so that the helper function do not have to call parse_date(). The verb "die" everywhere else in the codebase of Git not just means an immediate exit, but means an imediate _error_ exit. The new helper function is not about failure, but merely is an early return. Perhaps rename it to exit_if_unmodified_since() or something? > diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh > index 31076ed..0e49f29 100755 > --- a/t/t9501-gitweb-standalone-http-status.sh > +++ b/t/t9501-gitweb-standalone-http-status.sh > @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output' > test_expect_success 'snapshots: bad tree-ish id (tagged object)' ' > echo object > tag-object && > git add tag-object && > - git commit -m "Object to be tagged" && > + test_tick && git commit -m "Object to be tagged" && > git tag tagged-object `git hash-object tag-object` && > gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" && > grep "400 - Object is not a tree-ish" gitweb.output > @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' ' > ' > test_debug 'cat gitweb.output' > > +# ---------------------------------------------------------------------- > +# modification times (Last-Modified and If-Modified-Since) > + > +test_expect_success 'modification: feed last-modified' ' > + gitweb_run "p=.git;a=atom;h=master" && > + grep "Status: 200 OK" gitweb.output Missing " &&" at the end (same in 3/3). > + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output > +' > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: feed if-modified-since (modified)' ' > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > + gitweb_run "p=.git;a=atom;h=master" && > + unset HTTP_IF_MODIFIED_SINCE && > + grep "Status: 200 OK" gitweb.output > +' > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: feed if-modified-since (unmodified)' ' > + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && > + gitweb_run "p=.git;a=atom;h=master" && > + unset HTTP_IF_MODIFIED_SINCE && > + grep "Status: 304 Not Modified" gitweb.output > +' > +test_debug 'cat gitweb.headers' Other than that, the patch looks cleanly done. Thanks. ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v4 3/3] gitweb: add If-Modified-Since handling to git_snapshot(). 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King 2012-03-26 11:11 ` [PATCH v4 1/3] gitweb: add `status` headers to git_feed() responses W. Trevor King 2012-03-26 11:12 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King @ 2012-03-26 11:13 ` W. Trevor King 2012-03-26 19:14 ` [PATCH v5 " W. Trevor King 2012-03-27 19:24 ` [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb Jakub Narebski 3 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-26 11:13 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 3712 bytes --] Because snapshots can be large, you can save some bandwidth by supporting caching via If-Modified-Since. This patch adds support for the i-m-s request to git_snapshot() if the requested hash is a commit. Requests for snapshots of tree-ishes, which lack well defined timestamps, are still handled as they were before. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 21 +++++++++++++++--- t/t9501-gitweb-standalone-http-status.sh | 33 ++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 229f3da..be9ad5d 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7051,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); + die_if_unmodified($co{'committer_epoch'}) if %co; + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7060,10 +7064,19 @@ sub git_snapshot { } $filename =~ s/(["\\])/\\$1/g; - print $cgi->header( - -type => $known_snapshot_formats{$format}{'type'}, - -content_disposition => 'inline; filename="' . $filename . '"', - -status => '200 OK'); + if (%co) { + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); + print $cgi->header( + -type => $known_snapshot_formats{$format}{'type'}, + -content_disposition => 'inline; filename="' . $filename . '"', + -last_modified => $latest_date{'rfc2822'}, + -status => '200 OK'); + } else { + print $cgi->header( + -type => $known_snapshot_formats{$format}{'type'}, + -content_disposition => 'inline; filename="' . $filename . '"', + -status => '200 OK'); + } open my $fd, "-|", $cmd or die_error(500, "Execute git-archive failed"); diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh index 0e49f29..38e90bd 100755 --- a/t/t9501-gitweb-standalone-http-status.sh +++ b/t/t9501-gitweb-standalone-http-status.sh @@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' ' ' test_debug 'cat gitweb.headers' +test_expect_success 'modification: snapshot last-modified' ' + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + grep "Status: 200 OK" gitweb.output + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: snapshot if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 200 OK" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: snapshot if-modified-since (unmodified)' ' + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 304 Not Modified" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: tree-ish snapshot' ' + ID=`git rev-parse --verify HEAD^{tree}` && + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 200 OK" gitweb.output && + ! grep "Last-Modified" gitweb.output +' +test_debug 'cat gitweb.headers' + # ---------------------------------------------------------------------- # load checking -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot(). 2012-03-26 11:13 ` [PATCH v4 3/3] gitweb: add If-Modified-Since handling to git_snapshot() W. Trevor King @ 2012-03-26 19:14 ` W. Trevor King 2012-03-27 22:31 ` Jakub Narebski 0 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-26 19:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jakub Narebski, git [-- Attachment #1: Type: text/plain, Size: 3722 bytes --] Because snapshots can be large, you can save some bandwidth by supporting caching via If-Modified-Since. This patch adds support for the i-m-s request to git_snapshot() if the requested hash is a commit. Requests for snapshots of tree-ishes, which lack well defined timestamps, are still handled as they were before. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 21 +++++++++++++++--- t/t9501-gitweb-standalone-http-status.sh | 33 ++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 229f3da..be9ad5d 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7051,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); + exit_if_unmodified_since($co{'committer_epoch'}) if %co; + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7060,10 +7064,19 @@ sub git_snapshot { } $filename =~ s/(["\\])/\\$1/g; - print $cgi->header( - -type => $known_snapshot_formats{$format}{'type'}, - -content_disposition => 'inline; filename="' . $filename . '"', - -status => '200 OK'); + if (%co) { + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); + print $cgi->header( + -type => $known_snapshot_formats{$format}{'type'}, + -content_disposition => 'inline; filename="' . $filename . '"', + -last_modified => $latest_date{'rfc2822'}, + -status => '200 OK'); + } else { + print $cgi->header( + -type => $known_snapshot_formats{$format}{'type'}, + -content_disposition => 'inline; filename="' . $filename . '"', + -status => '200 OK'); + } open my $fd, "-|", $cmd or die_error(500, "Execute git-archive failed"); diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh index 0e49f29..38e90bd 100755 --- a/t/t9501-gitweb-standalone-http-status.sh +++ b/t/t9501-gitweb-standalone-http-status.sh @@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' ' ' test_debug 'cat gitweb.headers' +test_expect_success 'modification: snapshot last-modified' ' + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + grep "Status: 200 OK" gitweb.output && + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: snapshot if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 200 OK" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: snapshot if-modified-since (unmodified)' ' + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 304 Not Modified" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: tree-ish snapshot' ' + ID=`git rev-parse --verify HEAD^{tree}` && + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 200 OK" gitweb.output && + ! grep "Last-Modified" gitweb.output +' +test_debug 'cat gitweb.headers' + # ---------------------------------------------------------------------- # load checking -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot(). 2012-03-26 19:14 ` [PATCH v5 " W. Trevor King @ 2012-03-27 22:31 ` Jakub Narebski 2012-03-28 13:58 ` W. Trevor King 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narebski @ 2012-03-27 22:31 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git On Mon, 26 Mar 2012, W. Trevor King wrote: > Because snapshots can be large, you can save some bandwidth by > supporting caching via If-Modified-Since. This patch adds support for > the i-m-s request to git_snapshot() if the requested hash is a commit. "if the requested hash is a commit" means here "if we request snapshot of a commit", isn't it? > Requests for snapshots of tree-ishes, which lack well defined > timestamps, are still handled as they were before. s/tree-ishes/trees/. Commit is tree-ish but not a tree; it is tree that lacks timestamp that commit has. > > Signed-off-by: W Trevor King <wking@drexel.edu> > --- > gitweb/gitweb.perl | 21 +++++++++++++++--- > t/t9501-gitweb-standalone-http-status.sh | 33 ++++++++++++++++++++++++++++++ > 2 files changed, 50 insertions(+), 4 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 229f3da..be9ad5d 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -7051,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); > + exit_if_unmodified_since($co{'committer_epoch'}) if %co; > + O.K. Nice. > my $cmd = quote_command( > git_cmd(), 'archive', > "--format=$known_snapshot_formats{$format}{'format'}", > @@ -7060,10 +7064,19 @@ sub git_snapshot { > } > > $filename =~ s/(["\\])/\\$1/g; > - print $cgi->header( > - -type => $known_snapshot_formats{$format}{'type'}, > - -content_disposition => 'inline; filename="' . $filename . '"', > - -status => '200 OK'); > + if (%co) { > + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > + print $cgi->header( > + -type => $known_snapshot_formats{$format}{'type'}, > + -content_disposition => 'inline; filename="' . $filename . '"', > + -last_modified => $latest_date{'rfc2822'}, > + -status => '200 OK'); > + } else { > + print $cgi->header( > + -type => $known_snapshot_formats{$format}{'type'}, > + -content_disposition => 'inline; filename="' . $filename . '"', > + -status => '200 OK'); > + } This can be written shorter, and with less code repetition using ?: conditional operator: > + print $cgi->header( > + -type => $known_snapshot_formats{$format}{'type'}, > + -content_disposition => 'inline; filename="' . $filename . '"', > + (%co ? -last_modified => $latest_date{'rfc2822'} : ()), > + -status => '200 OK'); ...or something like that. > > open my $fd, "-|", $cmd > or die_error(500, "Execute git-archive failed"); > diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh > index 0e49f29..38e90bd 100755 > --- a/t/t9501-gitweb-standalone-http-status.sh > +++ b/t/t9501-gitweb-standalone-http-status.sh Same comments as for 2/3. -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot(). 2012-03-27 22:31 ` Jakub Narebski @ 2012-03-28 13:58 ` W. Trevor King 0 siblings, 0 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-28 13:58 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 1996 bytes --] On Tue, Mar 27, 2012 at 11:31:37PM +0100, Jakub Narebski wrote: > On Mon, 26 Mar 2012, W. Trevor King wrote: > > Because snapshots can be large, you can save some bandwidth by > > supporting caching via If-Modified-Since. This patch adds support for > > the i-m-s request to git_snapshot() if the requested hash is a commit. > > "if the requested hash is a commit" means here "if we request snapshot > of a commit", isn't it? > > > Requests for snapshots of tree-ishes, which lack well defined > > timestamps, are still handled as they were before. > > s/tree-ishes/trees/. Commit is tree-ish but not a tree; it is tree > that lacks timestamp that commit has. Right. I'll fix those in v6 ;). > > + if (%co) { > > + my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > > + print $cgi->header( > > + -type => $known_snapshot_formats{$format}{'type'}, > > + -content_disposition => 'inline; filename="' . $filename . '"', > > + -last_modified => $latest_date{'rfc2822'}, > > + -status => '200 OK'); > > + } else { > > + print $cgi->header( > > + -type => $known_snapshot_formats{$format}{'type'}, > > + -content_disposition => 'inline; filename="' . $filename . '"', > > + -status => '200 OK'); > > + } > > This can be written shorter, and with less code repetition using ?: > conditional operator: > > > + print $cgi->header( > > + -type => $known_snapshot_formats{$format}{'type'}, > > + -content_disposition => 'inline; filename="' . $filename . '"', > > + (%co ? -last_modified => $latest_date{'rfc2822'} : ()), > > + -status => '200 OK'); > > ...or something like that. Ah, that's much better. I'm not particulary familar with Perl, sorry ;). I'll wait until I here back on Last-Modified timestamp generation, and then submit v6. -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King ` (2 preceding siblings ...) 2012-03-26 11:13 ` [PATCH v4 3/3] gitweb: add If-Modified-Since handling to git_snapshot() W. Trevor King @ 2012-03-27 19:24 ` Jakub Narebski 2012-03-27 19:49 ` W. Trevor King 2012-03-27 19:55 ` W. Trevor King 3 siblings, 2 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-27 19:24 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git On Mon, 26 Mar 2012, W. Trevor King wrote: > Sorry for the delay since my last message, it's been a busy week ;). Thanks for working on this topic. BTW. we are in feature freeze currently, before release of v1.7.10. > I'll be sending along three patches. [...] > [...] The first is a teensy patch to add `Status` > output to non-304 calls to git_feed(). Without it you'd have to get a > bit more creative in the test suite. If the status lines were left > out intentionally, let me know, and I'll come up with another > condition for those tests. No, the status line was omitted unintentionally, thanks for fixing this. I guess that web server (at least Apache) adds it if it is missing. Though I am not sure how lack of "Status:" header is different from incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'... > The second patch refactors and tests git_feed(), and the third patch > adds i-m-s support to git_snapshot() with associated tests. > > Changes since v3: > * Patch 1/3 is completely new. > * Split previous patch into 2/3 and 3/3. > * Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}. > * Added testing to both 2/3 and 3/3. > * Reworked commit messages. Good. -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb 2012-03-27 19:24 ` [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb Jakub Narebski @ 2012-03-27 19:49 ` W. Trevor King 2012-03-27 19:57 ` Jakub Narebski 2012-03-27 19:55 ` W. Trevor King 1 sibling, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-27 19:49 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 711 bytes --] On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote: > BTW. we are in feature freeze currently, before release of v1.7.10. I'm not in a big hurry or anything, just trying to save some bandwidth… > Though I am not sure how lack of "Status:" header is different from > incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'... It's not for the 304 tests. I thought it would be a good idea to grep for a 200 status before looking for Last-Modified times, to ensure you actually get to the successful response stage. -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb 2012-03-27 19:49 ` W. Trevor King @ 2012-03-27 19:57 ` Jakub Narebski 0 siblings, 0 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-27 19:57 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git W. Trevor King wrote: > On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote: > > Though I am not sure how lack of "Status:" header is different from > > incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'... > > It's not for the 304 tests. I thought it would be a good idea to grep > for a 200 status before looking for Last-Modified times, to ensure you > actually get to the successful response stage. Right. Testing both sides of a new feature (i.e. modified and unmodified here) is a good testing practice. Thanks. -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb 2012-03-27 19:24 ` [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb Jakub Narebski 2012-03-27 19:49 ` W. Trevor King @ 2012-03-27 19:55 ` W. Trevor King 2012-03-27 20:18 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-27 19:55 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 666 bytes --] On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote: > No, the status line was omitted unintentionally, thanks for fixing this. > I guess that web server (at least Apache) adds it if it is missing. From the CGI 1.1 specs [1], section 9.2: If the script does not return a Status header, then "200 OK" should be assumed. So, perhaps it is better to skip the Status header on success (less for the server to parse). [1]: http://tools.ietf.org/html/draft-robinson-www-interface-00 -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb 2012-03-27 19:55 ` W. Trevor King @ 2012-03-27 20:18 ` Junio C Hamano 0 siblings, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2012-03-27 20:18 UTC (permalink / raw) To: W. Trevor King; +Cc: Jakub Narebski, git "W. Trevor King" <wking@drexel.edu> writes: > On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote: >> No, the status line was omitted unintentionally, thanks for fixing this. >> I guess that web server (at least Apache) adds it if it is missing. > > From the CGI 1.1 specs [1], section 9.2: > > If the script does not return a Status header, then "200 OK" should > be assumed. > > So, perhaps it is better to skip the Status header on success (less > for the server to parse). I do not think it matters either way. It is just a single header line, no? The code is certainly more explicit with your patch. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling 2012-03-20 11:55 ` Jakub Narebski 2012-03-20 16:40 ` [PATCH v2] " W. Trevor King @ 2012-03-26 17:36 ` W. Trevor King 2012-03-26 18:39 ` Junio C Hamano 1 sibling, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-26 17:36 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jakub Narebski, git [-- Attachment #1: Type: text/plain, Size: 2144 bytes --] On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote: > It might make more sense to call the helper *after* stuffing %latest_date > and pass the $latest_date{'rfc2822'} as parameter to the new helper > function, so that the helper function do not have to call parse_date(). I did something closer to your suggestion in my v1 patch, but On Tue, Mar 20, 2012 at 04:55:45AM -0700, Jakub Narebski wrote: > Shouldn't we pass \%latest_date (or rather \%modification_date to be > more generic)? Ah, I see that parse_date() subroutine does not store > original epoch not original timezone. > > By the way, for $latest_date{'rfc2822'} we don't need $timezone > argument, and I think we should not pass it to generic > modified_since() function (or whatever it will be called). > > … > > I think a better idea would be to make this function/subroutine (named > e.g. if_modified_since() or something like that) to work more like > die_error() -- it should simply end request instead of having caller > to use complcated calling convention, and care about eraly termination > by itself. Only passing in the epoch. We could reduce computation at the expense of complication by passing in both the epoch and a formatted time string, but after Jakub's suggestions, I felt like a simpler interface was the better approach. I don't feel particularly committed to either way, so just tell me which you'd like best ;). > The verb "die" everywhere else in the codebase of Git not just means an > immediate exit, but means an imediate _error_ exit. The new helper > function is not about failure, but merely is an early return. Perhaps > rename it to exit_if_unmodified_since() or something? Will fix in v5. > Missing " &&" at the end (same in 3/3). Oops. I wonder why my tests still passed :p. Will fix in v5. > Other than that, the patch looks cleanly done. > > Thanks. You're welcome. Thanks for shepherding me through it ;). -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling 2012-03-26 17:36 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King @ 2012-03-26 18:39 ` Junio C Hamano 2012-03-26 19:12 ` [PATCH v5 " W. Trevor King 0 siblings, 1 reply; 46+ messages in thread From: Junio C Hamano @ 2012-03-26 18:39 UTC (permalink / raw) To: W. Trevor King; +Cc: Jakub Narebski, git "W. Trevor King" <wking@drexel.edu> writes: > On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote: > ... > Only passing in the epoch. We could reduce computation at the expense > of complication by passing in both the epoch and a formatted time > string, but after Jakub's suggestions, I felt like a simpler interface > was the better approach. I don't feel particularly committed to > either way, so just tell me which you'd like best ;). The existing codepath already had the call to parse_date() nearby and that was the only reason I made the suggestion. The caller in the snapshot codepath would need to call parse_date() much earlier and it would have to be conditional to the availability of %co, so the simpler interface would probably be overall win. >> Missing " &&" at the end (same in 3/3). > > Oops. I wonder why my tests still passed :p. Will fix in v5. The test will pass if there is no breakage. The offence of missing " &&" after your "grep 200" is that it will let the test pass even if you did not have "200" in the output, failing to catch future breakage. ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling 2012-03-26 18:39 ` Junio C Hamano @ 2012-03-26 19:12 ` W. Trevor King 2012-03-27 22:24 ` Jakub Narebski 0 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-26 19:12 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jakub Narebski, git [-- Attachment #1: Type: text/plain, Size: 4844 bytes --] The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function exit_if_unmodified_since. This makes the code easy to reuse for other actions. Only gitweb actions which can easily calculate a modification time should use exit_if_unmodified_since, as the goal is to balance local processing time vs. upload bandwidth. Signed-off-by: W Trevor King <wking@drexel.edu> --- Patch v4 1/3 is unchanged. Should I mail it back in with a [PATCH v5 1/3] tag? Changes since v4: * die_if_unmodified() -> exit_if_unmodified_since() * Added missing `&&` to tests for feed-last-modified (patch 2/3) and snapshot-last-modified (patch 3/3). gitweb/gitweb.perl | 40 +++++++++++++++++------------ t/t9501-gitweb-standalone-http-status.sh | 27 +++++++++++++++++++- 2 files changed, 49 insertions(+), 18 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 041da17..229f3da 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,28 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub exit_if_unmodified_since { + my ($latest_epoch) = @_; + our $cgi; + + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); + if (defined $if_modified) { + my $since; + if (eval { require HTTP::Date; 1; }) { + $since = HTTP::Date::str2time($if_modified); + } elsif (eval { require Time::ParseDate; 1; }) { + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); + } + if (defined $since && $latest_epoch <= $since) { + my %latest_date = parse_date($latest_epoch); + print $cgi->header( + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + goto DONE_GITWEB; + } + } +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -7820,24 +7842,8 @@ sub git_feed { if (defined($commitlist[0])) { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_epoch'}; + exit_if_unmodified_since($latest_epoch); %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); - if (defined $if_modified) { - my $since; - if (eval { require HTTP::Date; 1; }) { - $since = HTTP::Date::str2time($if_modified); - } elsif (eval { require Time::ParseDate; 1; }) { - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); - } - if (defined $since && $latest_epoch <= $since) { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '304 Not Modified'); - return; - } - } print $cgi->header( -type => $content_type, -charset => 'utf-8', diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh index 31076ed..0e49f29 100755 --- a/t/t9501-gitweb-standalone-http-status.sh +++ b/t/t9501-gitweb-standalone-http-status.sh @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output' test_expect_success 'snapshots: bad tree-ish id (tagged object)' ' echo object > tag-object && git add tag-object && - git commit -m "Object to be tagged" && + test_tick && git commit -m "Object to be tagged" && git tag tagged-object `git hash-object tag-object` && gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" && grep "400 - Object is not a tree-ish" gitweb.output @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' ' ' test_debug 'cat gitweb.output' +# ---------------------------------------------------------------------- +# modification times (Last-Modified and If-Modified-Since) + +test_expect_success 'modification: feed last-modified' ' + gitweb_run "p=.git;a=atom;h=master" && + grep "Status: 200 OK" gitweb.output && + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: feed if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=atom;h=master" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 200 OK" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: feed if-modified-since (unmodified)' ' + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && + gitweb_run "p=.git;a=atom;h=master" && + unset HTTP_IF_MODIFIED_SINCE && + grep "Status: 304 Not Modified" gitweb.output +' +test_debug 'cat gitweb.headers' # ---------------------------------------------------------------------- # load checking -- 1.7.3.4 [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 490 bytes --] ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling 2012-03-26 19:12 ` [PATCH v5 " W. Trevor King @ 2012-03-27 22:24 ` Jakub Narebski 2012-03-28 13:51 ` W. Trevor King 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narebski @ 2012-03-27 22:24 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git On Mon, 26 Mar 2012, W. Trevor King wrote: > The current gitweb only generates Last-Modified and handles > If-Modified-Since headers for the git_feed action. This patch breaks > the Last-Modified and If-Modified-Since handling code out from > git_feed into a new function exit_if_unmodified_since. This makes the > code easy to reuse for other actions. Nice description, and I think quite good name for a subroutine. Well, good enough; any more we would drown in bikeshed-ding. > > Only gitweb actions which can easily calculate a modification time > should use exit_if_unmodified_since, as the goal is to balance local > processing time vs. upload bandwidth. Good. > > Signed-off-by: W Trevor King <wking@drexel.edu> > --- > Patch v4 1/3 is unchanged. Should I mail it back in with a > [PATCH v5 1/3] tag? > > Changes since v4: > * die_if_unmodified() -> exit_if_unmodified_since() > * Added missing `&&` to tests for feed-last-modified (patch 2/3) and > snapshot-last-modified (patch 3/3). > > gitweb/gitweb.perl | 40 +++++++++++++++++------------ > t/t9501-gitweb-standalone-http-status.sh | 27 +++++++++++++++++++- > 2 files changed, 49 insertions(+), 18 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index 041da17..229f3da 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -7003,6 +7003,28 @@ sub snapshot_name { > return wantarray ? ($name, $name) : $name; > } > > +sub exit_if_unmodified_since { > + my ($latest_epoch) = @_; We could have made \%latest_date an optional parameter... + my ($latest_epoch, $latest_date) = @_; > + our $cgi; > + > + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > + if (defined $if_modified) { > + my $since; > + if (eval { require HTTP::Date; 1; }) { > + $since = HTTP::Date::str2time($if_modified); > + } elsif (eval { require Time::ParseDate; 1; }) { > + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > + } > + if (defined $since && $latest_epoch <= $since) { > + my %latest_date = parse_date($latest_epoch); ...and use it if provided + my %latest_date = ref($latest_date) eq 'HASH' + ? %$latest_date : parse_date($latest_epoch); but something is to be said for simplicity. We need epoch for comparison anyway. > + print $cgi->header( > + -last_modified => $latest_date{'rfc2822'}, > + -status => '304 Not Modified'); O.K. we lose some unnecessary headers; never mind that. What is important is here. > + goto DONE_GITWEB; > + } > + } > +} > + > sub git_snapshot { > my $format = $input_params{'snapshot_format'}; > if (!@snapshot_fmts) { > @@ -7820,24 +7842,8 @@ sub git_feed { > if (defined($commitlist[0])) { > %latest_commit = %{$commitlist[0]}; > my $latest_epoch = $latest_commit{'committer_epoch'}; > + exit_if_unmodified_since($latest_epoch); > %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); > - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); > - if (defined $if_modified) { > - my $since; > - if (eval { require HTTP::Date; 1; }) { > - $since = HTTP::Date::str2time($if_modified); > - } elsif (eval { require Time::ParseDate; 1; }) { > - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); > - } > - if (defined $since && $latest_epoch <= $since) { > - print $cgi->header( > - -type => $content_type, > - -charset => 'utf-8', > - -last_modified => $latest_date{'rfc2822'}, > - -status => '304 Not Modified'); > - return; > - } > - } > print $cgi->header( > -type => $content_type, > -charset => 'utf-8', > diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh > index 31076ed..0e49f29 100755 > --- a/t/t9501-gitweb-standalone-http-status.sh > +++ b/t/t9501-gitweb-standalone-http-status.sh > @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output' > test_expect_success 'snapshots: bad tree-ish id (tagged object)' ' > echo object > tag-object && > git add tag-object && > - git commit -m "Object to be tagged" && > + test_tick && git commit -m "Object to be tagged" && > git tag tagged-object `git hash-object tag-object` && > gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" && > grep "400 - Object is not a tree-ish" gitweb.output > @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' ' > ' > test_debug 'cat gitweb.output' > > +# ---------------------------------------------------------------------- > +# modification times (Last-Modified and If-Modified-Since) > + > +test_expect_success 'modification: feed last-modified' ' > + gitweb_run "p=.git;a=atom;h=master" && > + grep "Status: 200 OK" gitweb.output && > + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output > +' All right. What's that date from? Wouldn't it be better to read it from commit object with `git show -s --pretty=%cD HEAD` or postprocessed from '%ct' timestamp? > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: feed if-modified-since (modified)' ' > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > + gitweb_run "p=.git;a=atom;h=master" && > + unset HTTP_IF_MODIFIED_SINCE && > + grep "Status: 200 OK" gitweb.output > +' I think it *might* be better solution to use test_when_finished: +test_expect_success 'modification: feed if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && + gitweb_run "p=.git;a=atom;h=master" && + grep "Status: 200 OK" gitweb.output +' I don't think we need sane_unset here. > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: feed if-modified-since (unmodified)' ' > + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && > + gitweb_run "p=.git;a=atom;h=master" && > + unset HTTP_IF_MODIFIED_SINCE && > + grep "Status: 304 Not Modified" gitweb.output > +' Same here. > +test_debug 'cat gitweb.headers' > > # ---------------------------------------------------------------------- > # load checking > -- > 1.7.3.4 > -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling 2012-03-27 22:24 ` Jakub Narebski @ 2012-03-28 13:51 ` W. Trevor King 2012-03-28 14:13 ` Jakub Narebski 0 siblings, 1 reply; 46+ messages in thread From: W. Trevor King @ 2012-03-28 13:51 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 1950 bytes --] On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote: > On Mon, 26 Mar 2012, W. Trevor King wrote: > > +# ---------------------------------------------------------------------- > > +# modification times (Last-Modified and If-Modified-Since) > > + > > +test_expect_success 'modification: feed last-modified' ' > > + gitweb_run "p=.git;a=atom;h=master" && > > + grep "Status: 200 OK" gitweb.output && > > + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output > > +' > > All right. > > What's that date from? Wouldn't it be better to read it from commit > object with `git show -s --pretty=%cD HEAD` or postprocessed from > '%ct' timestamp? That's the date set by the first `test_tick`, which is hardcoded in `test-lib-functions.sh`. Extracting the date dynamically seems unnecessary, since I can't imagine anyone changing the `test_tick` date. It's easy enough to do if you think it is appropriate though… > > +test_debug 'cat gitweb.headers' > > + > > +test_expect_success 'modification: feed if-modified-since (modified)' ' > > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > > + gitweb_run "p=.git;a=atom;h=master" && > > + unset HTTP_IF_MODIFIED_SINCE && > > + grep "Status: 200 OK" gitweb.output > > +' > > I think it *might* be better solution to use test_when_finished: > > +test_expect_success 'modification: feed if-modified-since (modified)' ' > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && > + gitweb_run "p=.git;a=atom;h=master" && > + grep "Status: 200 OK" gitweb.output > +' > > I don't think we need sane_unset here. Good point. Sloppy me not reading `t/README` thoroughly enough ;). -- 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling 2012-03-28 13:51 ` W. Trevor King @ 2012-03-28 14:13 ` Jakub Narebski 2012-03-28 15:46 ` [PATCH v6 0/3] " wking 0 siblings, 1 reply; 46+ messages in thread From: Jakub Narebski @ 2012-03-28 14:13 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git On Wed, 28 Mar 2012, W. Trevor King wrote: > On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote: > > On Mon, 26 Mar 2012, W. Trevor King wrote: > > > +# ---------------------------------------------------------------------- > > > +# modification times (Last-Modified and If-Modified-Since) > > > + > > > +test_expect_success 'modification: feed last-modified' ' > > > + gitweb_run "p=.git;a=atom;h=master" && > > > + grep "Status: 200 OK" gitweb.output && > > > + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output > > > +' > > > > All right. > > > > What's that date from? Wouldn't it be better to read it from commit > > object with `git show -s --pretty=%cD HEAD` or postprocessed from > > '%ct' timestamp? > > That's the date set by the first `test_tick`, which is hardcoded in > `test-lib-functions.sh`. Extracting the date dynamically seems > unnecessary, since I can't imagine anyone changing the `test_tick` > date. Ah, it's all right then. I should have checked the test_tick function. That of course assuming that nobody would add test_tick earlier, but if he/she does, he/she can deal with fallout... > It's easy enough to do if you think it is appropriate though… No, it is not needed. > > > +test_debug 'cat gitweb.headers' > > > + > > > +test_expect_success 'modification: feed if-modified-since (modified)' ' > > > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > > > + gitweb_run "p=.git;a=atom;h=master" && > > > + unset HTTP_IF_MODIFIED_SINCE && > > > + grep "Status: 200 OK" gitweb.output > > > +' > > > > I think it *might* be better solution to use test_when_finished: > > > > +test_expect_success 'modification: feed if-modified-since (modified)' ' > > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > > + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && > > + gitweb_run "p=.git;a=atom;h=master" && > > + grep "Status: 200 OK" gitweb.output > > +' > > > > I don't think we need sane_unset here. > > Good point. Sloppy me not reading `t/README` thoroughly enough ;). -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v6 0/3] gitweb: refactor If-Modified-Since handling 2012-03-28 14:13 ` Jakub Narebski @ 2012-03-28 15:46 ` wking 2012-03-28 15:46 ` [PATCH v6 1/3] gitweb: add `status` headers to git_feed() responses wking ` (3 more replies) 0 siblings, 4 replies; 46+ messages in thread From: wking @ 2012-03-28 15:46 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git, W. Trevor King Changes since v5: * Use `test_when_finished` to unset HTTP_IF_MODIFIED_SINCE. * Actually use the tree ID in the `modification: tree snapshot` test. * Conditional last_modified argument for `$cgi->header` git_snapshot and git_feed. * tree-ish -> trees in git_snapshot commit message and test name. Unchanged since v5: * Patch 1/3, but I'm resending it anyway with `git send-email`. I could skip resending it with: git send-email … HEAD^^ but that would mess up the patch numbering. W. Trevor King (3): gitweb: add `status` headers to git_feed() responses. gitweb: refactor If-Modified-Since handling gitweb: add If-Modified-Since handling to git_snapshot(). gitweb/gitweb.perl | 65 ++++++++++++++++++------------ t/t9501-gitweb-standalone-http-status.sh | 60 +++++++++++++++++++++++++++- 2 files changed, 98 insertions(+), 27 deletions(-) -- 1.7.3.4 ^ permalink raw reply [flat|nested] 46+ messages in thread
* [PATCH v6 1/3] gitweb: add `status` headers to git_feed() responses. 2012-03-28 15:46 ` [PATCH v6 0/3] " wking @ 2012-03-28 15:46 ` wking 2012-03-28 15:47 ` [PATCH v6 2/3] gitweb: refactor If-Modified-Since handling wking ` (2 subsequent siblings) 3 siblings, 0 replies; 46+ messages in thread From: wking @ 2012-03-28 15:46 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git, W. Trevor King The git_feed() method was not setting a `Status` header unless it was responding to an If-Modified-Since request with `304 Not Modified`. Now, when it is serving successful responses, it sets status to `200 OK`. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 6 ++++-- 1 files changed, 4 insertions(+), 2 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index a8b5fad..041da17 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7841,11 +7841,13 @@ sub git_feed { print $cgi->header( -type => $content_type, -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}); + -last_modified => $latest_date{'rfc2822'}, + -status => '200 OK'); } else { print $cgi->header( -type => $content_type, - -charset => 'utf-8'); + -charset => 'utf-8', + -status => '200 OK'); } # Optimization: skip generating the body if client asks only -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v6 2/3] gitweb: refactor If-Modified-Since handling 2012-03-28 15:46 ` [PATCH v6 0/3] " wking 2012-03-28 15:46 ` [PATCH v6 1/3] gitweb: add `status` headers to git_feed() responses wking @ 2012-03-28 15:47 ` wking 2012-03-28 15:47 ` [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot() wking 2012-03-28 15:56 ` [PATCH v6 0/3] gitweb: refactor If-Modified-Since handling W. Trevor King 3 siblings, 0 replies; 46+ messages in thread From: wking @ 2012-03-28 15:47 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git, W. Trevor King The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function exit_if_unmodified_since. This makes the code easy to reuse for other actions. Only gitweb actions which can easily calculate a modification time should use exit_if_unmodified_since, as the goal is to balance local processing time vs. upload bandwidth. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 57 +++++++++++++++-------------- t/t9501-gitweb-standalone-http-status.sh | 27 +++++++++++++- 2 files changed, 55 insertions(+), 29 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index 041da17..ba106c1 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7003,6 +7003,28 @@ sub snapshot_name { return wantarray ? ($name, $name) : $name; } +sub exit_if_unmodified_since { + my ($latest_epoch) = @_; + our $cgi; + + my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); + if (defined $if_modified) { + my $since; + if (eval { require HTTP::Date; 1; }) { + $since = HTTP::Date::str2time($if_modified); + } elsif (eval { require Time::ParseDate; 1; }) { + $since = Time::ParseDate::parsedate($if_modified, GMT => 1); + } + if (defined $since && $latest_epoch <= $since) { + my %latest_date = parse_date($latest_epoch); + print $cgi->header( + -last_modified => $latest_date{'rfc2822'}, + -status => '304 Not Modified'); + goto DONE_GITWEB; + } + } +} + sub git_snapshot { my $format = $input_params{'snapshot_format'}; if (!@snapshot_fmts) { @@ -7820,35 +7842,14 @@ sub git_feed { if (defined($commitlist[0])) { %latest_commit = %{$commitlist[0]}; my $latest_epoch = $latest_commit{'committer_epoch'}; - %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); - my $if_modified = $cgi->http('IF_MODIFIED_SINCE'); - if (defined $if_modified) { - my $since; - if (eval { require HTTP::Date; 1; }) { - $since = HTTP::Date::str2time($if_modified); - } elsif (eval { require Time::ParseDate; 1; }) { - $since = Time::ParseDate::parsedate($if_modified, GMT => 1); - } - if (defined $since && $latest_epoch <= $since) { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '304 Not Modified'); - return; - } - } - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -last_modified => $latest_date{'rfc2822'}, - -status => '200 OK'); - } else { - print $cgi->header( - -type => $content_type, - -charset => 'utf-8', - -status => '200 OK'); + exit_if_unmodified_since($latest_epoch); + %latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'}); } + print $cgi->header( + -type => $content_type, + -charset => 'utf-8', + -last_modified => (%latest_date ? $latest_date{'rfc2822'} : undef), + -status => '200 OK'); # Optimization: skip generating the body if client asks only # for Last-Modified date. diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh index 31076ed..afa6bd4 100755 --- a/t/t9501-gitweb-standalone-http-status.sh +++ b/t/t9501-gitweb-standalone-http-status.sh @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output' test_expect_success 'snapshots: bad tree-ish id (tagged object)' ' echo object > tag-object && git add tag-object && - git commit -m "Object to be tagged" && + test_tick && git commit -m "Object to be tagged" && git tag tagged-object `git hash-object tag-object` && gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" && grep "400 - Object is not a tree-ish" gitweb.output @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' ' ' test_debug 'cat gitweb.output' +# ---------------------------------------------------------------------- +# modification times (Last-Modified and If-Modified-Since) + +test_expect_success 'modification: feed last-modified' ' + gitweb_run "p=.git;a=atom;h=master" && + grep "Status: 200 OK" gitweb.output && + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: feed if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && + gitweb_run "p=.git;a=atom;h=master" && + grep "Status: 200 OK" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: feed if-modified-since (unmodified)' ' + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && + gitweb_run "p=.git;a=atom;h=master" && + grep "Status: 304 Not Modified" gitweb.output +' +test_debug 'cat gitweb.headers' # ---------------------------------------------------------------------- # load checking -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot(). 2012-03-28 15:46 ` [PATCH v6 0/3] " wking 2012-03-28 15:46 ` [PATCH v6 1/3] gitweb: add `status` headers to git_feed() responses wking 2012-03-28 15:47 ` [PATCH v6 2/3] gitweb: refactor If-Modified-Since handling wking @ 2012-03-28 15:47 ` wking 2012-03-28 16:08 ` Jakub Narebski 2012-03-28 15:56 ` [PATCH v6 0/3] gitweb: refactor If-Modified-Since handling W. Trevor King 3 siblings, 1 reply; 46+ messages in thread From: wking @ 2012-03-28 15:47 UTC (permalink / raw) To: Jakub Narebski; +Cc: Junio C Hamano, git, W. Trevor King Because snapshots can be large, you can save some bandwidth by supporting caching via If-Modified-Since. This patch adds support for the i-m-s request to git_snapshot() if the request is a commit. Requests for snapshots of trees, which lack well defined timestamps, are still handled as they were before. Signed-off-by: W Trevor King <wking@drexel.edu> --- gitweb/gitweb.perl | 10 +++++++++ t/t9501-gitweb-standalone-http-status.sh | 33 ++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 0 deletions(-) diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl index ba106c1..591c793 100755 --- a/gitweb/gitweb.perl +++ b/gitweb/gitweb.perl @@ -7051,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); + exit_if_unmodified_since($co{'committer_epoch'}) if %co; + my $cmd = quote_command( git_cmd(), 'archive', "--format=$known_snapshot_formats{$format}{'format'}", @@ -7060,9 +7064,15 @@ sub git_snapshot { } $filename =~ s/(["\\])/\\$1/g; + my %latest_date; + if (%co) { + %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); + } + print $cgi->header( -type => $known_snapshot_formats{$format}{'type'}, -content_disposition => 'inline; filename="' . $filename . '"', + -last_modified => (%co ? $latest_date{'rfc2822'} : undef), -status => '200 OK'); open my $fd, "-|", $cmd diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh index afa6bd4..1487820 100755 --- a/t/t9501-gitweb-standalone-http-status.sh +++ b/t/t9501-gitweb-standalone-http-status.sh @@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' ' ' test_debug 'cat gitweb.headers' +test_expect_success 'modification: snapshot last-modified' ' + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + grep "Status: 200 OK" gitweb.output && + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: snapshot if-modified-since (modified)' ' + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + grep "Status: 200 OK" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: snapshot if-modified-since (unmodified)' ' + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && + grep "Status: 304 Not Modified" gitweb.output +' +test_debug 'cat gitweb.headers' + +test_expect_success 'modification: tree snapshot' ' + ID=`git rev-parse --verify HEAD^{tree}` && + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && + gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" && + grep "Status: 200 OK" gitweb.output && + ! grep "Last-Modified" gitweb.output +' +test_debug 'cat gitweb.headers' + # ---------------------------------------------------------------------- # load checking -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 46+ messages in thread
* Re: [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot(). 2012-03-28 15:47 ` [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot() wking @ 2012-03-28 16:08 ` Jakub Narebski 0 siblings, 0 replies; 46+ messages in thread From: Jakub Narebski @ 2012-03-28 16:08 UTC (permalink / raw) To: W. Trevor King; +Cc: Junio C Hamano, git On Wed, 28 Mar 2012, wking@tremily.us wrote: > Because snapshots can be large, you can save some bandwidth by > supporting caching via If-Modified-Since. This patch adds support for > the i-m-s request to git_snapshot() if the request is a commit. > Requests for snapshots of trees, which lack well defined timestamps, > are still handled as they were before. > > Signed-off-by: W Trevor King <wking@drexel.edu> > --- > gitweb/gitweb.perl | 10 +++++++++ > t/t9501-gitweb-standalone-http-status.sh | 33 ++++++++++++++++++++++++++++++ > 2 files changed, 43 insertions(+), 0 deletions(-) > > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl > index ba106c1..591c793 100755 > --- a/gitweb/gitweb.perl > +++ b/gitweb/gitweb.perl > @@ -7051,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); > + exit_if_unmodified_since($co{'committer_epoch'}) if %co; > + Nice. > my $cmd = quote_command( > git_cmd(), 'archive', > "--format=$known_snapshot_formats{$format}{'format'}", > @@ -7060,9 +7064,15 @@ sub git_snapshot { > } > > $filename =~ s/(["\\])/\\$1/g; > + my %latest_date; > + if (%co) { > + %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}); > + } > + That is probably more readable than + %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'}) + if %co; > print $cgi->header( > -type => $known_snapshot_formats{$format}{'type'}, > -content_disposition => 'inline; filename="' . $filename . '"', > + -last_modified => (%co ? $latest_date{'rfc2822'} : undef), > -status => '200 OK'); > Same issue as with previous patch: print $cgi->header( -type => $known_snapshot_formats{$format}{'type'}, -content_disposition => 'inline; filename="' . $filename . '"', + %co ? (-last_modified => $latest_date{'rfc2822'}) : (), -status => '200 OK'); > open my $fd, "-|", $cmd > diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh > index afa6bd4..1487820 100755 > --- a/t/t9501-gitweb-standalone-http-status.sh > +++ b/t/t9501-gitweb-standalone-http-status.sh > @@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' ' > ' > test_debug 'cat gitweb.headers' > > +test_expect_success 'modification: snapshot last-modified' ' > + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && > + grep "Status: 200 OK" gitweb.output && > + grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output > +' > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: snapshot if-modified-since (modified)' ' > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && > + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && > + grep "Status: 200 OK" gitweb.output > +' > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: snapshot if-modified-since (unmodified)' ' > + export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" && > + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && > + gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" && > + grep "Status: 304 Not Modified" gitweb.output > +' > +test_debug 'cat gitweb.headers' > + > +test_expect_success 'modification: tree snapshot' ' > + ID=`git rev-parse --verify HEAD^{tree}` && > + export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" && > + test_when_finished "unset HTTP_IF_MODIFIED_SINCE" && > + gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" && > + grep "Status: 200 OK" gitweb.output && > + ! grep "Last-Modified" gitweb.output > +' > +test_debug 'cat gitweb.headers' Good! -- Jakub Narebski Poland ^ permalink raw reply [flat|nested] 46+ messages in thread
* Re: [PATCH v6 0/3] gitweb: refactor If-Modified-Since handling 2012-03-28 15:46 ` [PATCH v6 0/3] " wking ` (2 preceding siblings ...) 2012-03-28 15:47 ` [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot() wking @ 2012-03-28 15:56 ` W. Trevor King 3 siblings, 0 replies; 46+ messages in thread From: W. Trevor King @ 2012-03-28 15:56 UTC (permalink / raw) Cc: Jakub Narebski, Junio C Hamano, git [-- Attachment #1: Type: text/plain, Size: 482 bytes --] On Wed, Mar 28, 2012 at 11:46:58AM -0400, wking@tremily.us wrote: > ... Oops, I set `sendemail.from`, but it doesn't look like it stuck, and I forgot to check before OKing the mails. Please consider the v6 patches to be from `W. Trevor King <wking@drexel.edu>`, and adjust any replies accordingly. Sorry, 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 --] ^ permalink raw reply [flat|nested] 46+ messages in thread
* Incremental updates to What's cooking 2012-03-20 0:23 What's cooking in git.git (Mar 2012, #07; Mon, 19) Junio C Hamano 2012-03-15 7:54 ` [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots W. Trevor King @ 2012-03-20 23:35 ` Junio C Hamano 1 sibling, 0 replies; 46+ messages in thread From: Junio C Hamano @ 2012-03-20 23:35 UTC (permalink / raw) To: git Again, just the highlights of the notable topics. Not much has happened on the 'master' front. [New Topics] * am/completion-zsh-fix (2012-03-20) 1 commit - contrib/completion: "local var=()" is misinterpreted as function decl by zsh Needs a bit better explanation. * lp/maint-diff-three-dash-with-graph (2012-03-20) 3 commits - t4202: add test for "log --graph --stat -p" separator lines - log --graph: fix break in graph lines - log --graph --stat: three-dash separator should come after graph lines The combination of two options "log --graph --stat" was an obscure corner case nobody cared about, and did not correctly show the ancestry graph lines. I've split the original patch into three pieces, one for fixes to two different issues and a test. Also the test is adjusted so that the series can be back-merged to older codebase that did not have 7f81463 (Use correct grammar in diffstat summary line, 2012-02-01) that first appeared in v1.7.9.2 * wk/gitweb-snapshot-use-if-modified-since (2012-03-20) 1 commit - Pull gitweb If-Modified-Since handling out into its own function and use for snapshots. Unreviewed; the title looks way too long and does not sit well in the shortlog output. * jc/maint-merge-autoedit (2012-03-20) 1 commit - merge: backport GIT_MERGE_AUTOEDIT support In 1.7.10, we added GIT_MERGE_AUTOEDIT=no environment variable to help older scripts to let them refuse giving users a chance to explain the merge, but forgot that 1.7.9 automatically opens an editor when merging an annotated tag, and there is no equivalent escape hatch. A merge of this topic to 1.7.10 track becomes a no-op, but we may want to apply this to the 1.7.9.x series. [Cooking] * ct/advise-push-default (2012-03-19) 1 commit - push: Provide situational hints for non-fast-forward error Breaks down the cases in which "git push" fails due to non-ff into three categories, and gives separate advise messages. This should be a good change regardless of mm/push-default-switch-warning topic. * tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits (merged to 'next' on 2012-03-20 at b3f67cd) + diff: tweak a _copy_ of diff_options with word-diff + diff: refactor the word-diff setup from builtin_diff_cmd + t4034: diff.*.wordregex should not be "sticky" in --word-diff It would be nice to have this fix in 1.7.10 but perhaps we can wait until 1.7.10.1. * nl/http-proxy-more (2012-03-15) 5 commits (merged to 'next' on 2012-03-20 at c004001) + http: rename HTTP_REAUTH to HTTP_AUTH_RETRY + http: Avoid limit of retrying request only twice + http: handle proxy authentication failure (error 407) + http: handle proxy proactive authentication + http: try http_proxy env var when http.proxy config option is not set The code to talk to http proxies learn to use the same credential API used to talk to the final http destinations. [Stalled] * th/difftool-diffall (2012-03-19) 9 commits . difftool: print list of valid tools with '--tool-help' . difftool: teach dir-diff to copy modified files back to working tree . difftool: teach difftool to handle directory diffs . difftool: replace system call with Git::command_noisy . difftool: eliminate setup_environment function . difftool: stop appending '.exe' to git . difftool: remove explicit change of PATH . difftool: exit(0) when usage is printed . difftool: parse options using Getopt::Long Reworks the "diffall" contrib script into "difftool" framework. Breaks tests when merged to 'pu'. * jh/apply-free-patch (2012-03-20) 2 commits - apply: free patch->result - apply: do not leak patches and fragments Valgrind reports quite a lot of discarded memory inside apply. I have a slight suspicion that we should first clarify the ownership rules of pieces of memory in this standalone program that was designed to be "run once and let exit take care of the memory" before proceeding further. * dg/test-from-elsewhere (2012-03-04) 2 commits - Support out-of-tree Valgrind tests - Allow overriding GIT_BUILD_DIR Better support for out-of-tree test scripts, but it appears that the approach needs to be rethought. By repointing TEST_DIRECTORY to a directory other than $(pwd)/.., an out of place test script can reach test helpers and freshly built Git relative to it (GIT_BUILD_DIR is a mere short-hand for $TEST_DIRECTORY/..). Discussion stalled. ^ permalink raw reply [flat|nested] 46+ messages in thread
end of thread, other threads:[~2012-03-28 16:09 UTC | newest] Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-03-20 0:23 What's cooking in git.git (Mar 2012, #07; Mon, 19) Junio C Hamano 2012-03-15 7:54 ` [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots W. Trevor King 2012-03-20 1:48 ` W. Trevor King 2012-03-20 23:07 ` Junio C Hamano 2012-03-20 11:55 ` Jakub Narebski 2012-03-20 16:40 ` [PATCH v2] " W. Trevor King 2012-03-21 12:11 ` [PATCH v3] Isolate If-Modified-Since handling in gitweb W. Trevor King 2012-03-21 13:19 ` Jakub Narebski 2012-03-21 14:04 ` W. Trevor King 2012-03-21 16:55 ` Jakub Narebski 2012-03-21 17:38 ` W. Trevor King 2012-03-21 19:22 ` Junio C Hamano 2012-03-21 19:55 ` W. Trevor King 2012-03-21 20:04 ` Jakub Narebski 2012-03-21 20:09 ` Junio C Hamano 2012-03-21 20:34 ` W. Trevor King 2012-03-22 13:05 ` Jakub Narebski 2012-03-21 17:21 ` Junio C Hamano 2012-03-22 12:46 ` Jakub Narebski 2012-03-22 17:00 ` Junio C Hamano 2012-03-26 11:09 ` [PATCH v4 0/3] " W. Trevor King 2012-03-26 11:11 ` [PATCH v4 1/3] gitweb: add `status` headers to git_feed() responses W. Trevor King 2012-03-26 11:12 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King 2012-03-26 17:12 ` Junio C Hamano 2012-03-26 11:13 ` [PATCH v4 3/3] gitweb: add If-Modified-Since handling to git_snapshot() W. Trevor King 2012-03-26 19:14 ` [PATCH v5 " W. Trevor King 2012-03-27 22:31 ` Jakub Narebski 2012-03-28 13:58 ` W. Trevor King 2012-03-27 19:24 ` [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb Jakub Narebski 2012-03-27 19:49 ` W. Trevor King 2012-03-27 19:57 ` Jakub Narebski 2012-03-27 19:55 ` W. Trevor King 2012-03-27 20:18 ` Junio C Hamano 2012-03-26 17:36 ` [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King 2012-03-26 18:39 ` Junio C Hamano 2012-03-26 19:12 ` [PATCH v5 " W. Trevor King 2012-03-27 22:24 ` Jakub Narebski 2012-03-28 13:51 ` W. Trevor King 2012-03-28 14:13 ` Jakub Narebski 2012-03-28 15:46 ` [PATCH v6 0/3] " wking 2012-03-28 15:46 ` [PATCH v6 1/3] gitweb: add `status` headers to git_feed() responses wking 2012-03-28 15:47 ` [PATCH v6 2/3] gitweb: refactor If-Modified-Since handling wking 2012-03-28 15:47 ` [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot() wking 2012-03-28 16:08 ` Jakub Narebski 2012-03-28 15:56 ` [PATCH v6 0/3] gitweb: refactor If-Modified-Since handling W. Trevor King 2012-03-20 23:35 ` Incremental updates to What's cooking Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.