{"thread":{"id":"30001","subject":"What's cooking in git.git (Mar 2012, #07; Mon, 19)","startedAt":"2012-03-20T00:23:07Z","lastAt":"2012-03-28T16:08:55Z","messageCount":43,"participants":["Junio C Hamano","W. Trevor King","Jakub Narebski","wking@tremily.us"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"187295","messageId":"7vmx7c86j8.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":null,"subject":"What's cooking in git.git (Mar 2012, #07; Mon, 19)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T00:23:07Z","receivedAt":"2012-03-20T00:23:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"What's cooking in git.git (Mar 2012, #07; Mon, 19)\n--------------------------------------------------\n\nHere are the topics that have been cooking.  Commits prefixed with '-' are\nonly in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.\n\nThe \"git push\" preparation steps (there are two topics) have enough time\nto be polished to graduate to 'master' soon after 1.7.10 final.\n\nYou can find the changes described here in the integration branches of the\nrepositories listed at\n\n    http://git-blame.blogspot.com/p/git-public-repositories.html\n\n--------------------------------------------------\n[Graduated to \"master\"]\n\n* ab/perl-i18n (2012-03-10) 3 commits\n + perl/Makefile: install Git::I18N under NO_PERL_MAKEMAKER\n + Git::I18N: compatibility with perl <5.8.3\n\nAttempts to help installations with ancient Perl and/or without\nMakeMaker.\n\n* jc/maint-verify-objects-remove-pessimism (2012-03-15) 1 commit\n  (merged to 'next' on 2012-03-15 at f824530)\n + fetch/receive: remove over-pessimistic connectivity check\n\nThe code to validate the history connectivity between old refs and new\nrefs used by fetch and receive-pack, introduced in 1.7.8, was grossly\ninefficient and unnecessarily tried to re-validate integrity of individual\nobjects. This essentially reverts the patch that introduced the\nperformance regression.\n\n* jn/maint-fast-import-empty-ls (2012-03-09) 2 commits\n  (merged to 'next' on 2012-03-15 at d531079)\n + fast-import: don't allow 'ls' of path with empty components\n + fast-import: leakfix for 'ls' of dirty trees\n\nfast-import did not diagnose \"ls ''\" that asks an empty path\nas an error.\n\n* sl/customize-sane-tool-path (2012-03-09) 1 commit\n  (merged to 'next' on 2012-03-15 at a838844)\n + configure: allow user to prevent $PATH \"sanitization\" on Solaris\n\n\"configure\" script learned to take \"--sane-tool-path\" from the command\nline to record SANE_TOOL_PATH (used to avoid broken platform tools in\n/usr/bin) in config.mak-autogen.  This may be useful for people on Solaris\nwho have saner tools outside /usr/xpg[46]/bin.\n\n* th/doc-diff-submodule-option (2012-03-14) 1 commit\n  (merged to 'next' on 2012-03-14 at 0e1d755)\n + Documentation/diff-options: reword description of --submodule option\n\nUpdate \"diff --submodule\" documentation.\n\n* th/git-diffall (2012-03-14) 5 commits\n  (merged to 'next' on 2012-03-14 at 38e1251)\n + contrib/diffall: fix cleanup trap on Windows\n + contrib/diffall: eliminate duplicate while loops\n + contrib/diffall: eliminate use of tar\n + contrib/diffall: create tmp dirs without mktemp\n + contrib/diffall: comment actual reason for 'cdup'\n\nUpdate sample \"diffall\" script.\n\n* th/mergetools-deltawalker (2012-03-15) 1 commit\n  (merged to 'next' on 2012-03-15 at 1a62faf)\n + Documentation/difftool: add deltawalker to list of valid diff tools\n\n--------------------------------------------------\n[New Topics]\n\n* jc/maint-clean-nested-worktree-in-subdir (2012-03-15) 2 commits\n - clean: preserve nested git worktree in subdirectories\n - remove_dir_recursively(): Add flag for skipping removal of toplevel dir\n (this branch is tangled with jh/notes-merge-in-git-dir-worktree.)\n\n\"git clean -d -f\" (not \"-d -f -f\") is supposed to protect nested working\ntrees of independent git repositories that exist in the current project\nworking tree from getting removed, but the protection applied only to such\nworking trees that are at the top-level of the current project by mistake.\n\nNot urgent.\n\n* ct/advise-push-default (2012-03-19) 1 commit\n - push: Provide situational hints for non-fast-forward errors\n\nBreaks down the cases in which \"git push\" fails due to non-ff into three\ncategories, and gives separate advise messages.  I think there should be\nthree advise confvars, not just two.\n\n* nl/rebase-i-cheat-sheet (2012-03-16) 1 commit\n - [Do not merge] rebase -i: remind that the lines are top-to-bottom\n\nNot urgent.\n\n* da/difftool-test (2012-03-19) 1 commit\n - t7800: Test difftool passing arguments to diff\n\nMakes sure \"difftool\" options can be given in any order.\n\n* th/difftool-diffall (2012-03-19) 9 commits\n - difftool: print list of valid tools with '--tool-help'\n - difftool: teach dir-diff to copy modified files back to working tree\n - difftool: teach difftool to handle directory diffs\n - difftool: replace system call with Git::command_noisy\n - difftool: eliminate setup_environment function\n - difftool: stop appending '.exe' to git\n - difftool: remove explicit change of PATH\n - difftool: exit(0) when usage is printed\n - difftool: parse options using Getopt::Long\n\nReworks the \"diffall\" contrib script into \"difftool\" framework.  Use of\nGetopt::Long obviously contradicts with da/difftool-test and breaks 'pu'.\n\n--------------------------------------------------\n[Stalled]\n\n* nd/columns (2012-03-13) 12 commits\n - column: support grouping entries\n - column: support \"denser\" mode\n - ls-files: support --column\n - tag: add --column\n - column: support piping stdout to external git-column process\n - status: add --column\n - branch: add --column\n - help: reuse print_columns() for help -a\n - column: add dense layout support\n - column: add columnar layout\n - Stop starting pager recursively\n - Add column layout skeleton and git-column\n\nRerolled again.  Modulo minor nits, looked nicer than the previous round.\n\n* nd/threaded-index-pack (2012-03-11) 2 commits\n - index-pack: support multithreaded delta resolving\n - index-pack: split second pass obj handling into own function\n\nAnother reroll after a bugreport on pthread usage discovered by Ramsey,\nbut it seems the topic is cooking between Ramsay and Duy out of tree.\nWaiting for resolution.\n\n* dg/test-from-elsewhere (2012-03-04) 2 commits\n - Support out-of-tree Valgrind tests\n - Allow overriding GIT_BUILD_DIR\n\nBetter support for out-of-tree test scripts, but it appears that the\napproach needs to be rethought.  By repointing TEST_DIRECTORY to a\ndirectory other than $(pwd)/.., an out of place test script can reach\ntest helpers and freshly built Git relative to it (GIT_BUILD_DIR is\na mere short-hand for $TEST_DIRECTORY/..).\n\n* cn/apply-fix-ws-can-lengthen-lines (2012-03-11) 1 commit\n . apply: reallocate the postimage buffer when needed\n\nAttempts to address an ancient bug that dates back to the addition\nof an oddball \"tab-in-indent\" whitespace breakage class that wants\nto have longer lines than the original when fixing things up.\n\nNeeds more work; results in double-frees.\n\n* hv/submodule-recurse-push (2012-02-13) 3 commits\n - push: teach --recurse-submodules the on-demand option\n - Refactor submodule push check to use string list instead of integer\n - Teach revision walking machinery to walk multiple times sequencially\n\nThe bottom one was not clearly explained and needs a reroll.\n\n* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits\n - fixup! 15eaaf4\n - git-svn, perl/Git.pm: extend Git::prompt helper for querying users\n - perl/Git.pm: \"prompt\" helper to honor GIT_ASKPASS and SSH_ASKPASS\n\nThe bottom one has been replaced with a rewrite based on comments\nfrom Ævar. The second one needs more work, both in perl/Git.pm and\nprompt.c, to give precedence to tty over SSH_ASKPASS when terminal\nis available.\n\n* jc/split-blob (2012-03-16) 6 commits\n - chunked-object: streaming checkout\n - chunked-object: fallback checkout codepaths\n - bulk-checkin: support chunked-object encoding\n - bulk-checkin: allow the same data to be multiply hashed\n - new representation types in the packstream\n - varint-in-pack: refactor varint encoding/decoding\n\nNot ready.\n\nI finished the streaming checkout codepath, but as explained in\n127b177 (bulk-checkin: support chunked-object encoding, 2011-11-30),\nthese are still early steps of a long and painful journey. At least\npack-objects and fsck need to learn the new encoding for the series\nto be usable locally, and then index-pack/unpack-objects needs to\nlearn it to be used remotely.\n\nGiven that I heard a lot of noise that people want large files, and\nthat I was asked by somebody at GitTogether'11 privately for an\nadvice on how to pay developers (not me) to help adding necessary\nsupport, I am somewhat dissapointed that the original patch series\nthat was sent almost two months ago still remains here without much\ncomments and updates from the developer community. I even made the\ninterface to the logic that decides where to split chunks easily\nreplaceable, and I deliberately made the logic in the original patch\nextremely stupid to entice others, especially the \"bup\" fanboys, to\ncome up with a better logic, thinking that giving people an easy\ntarget to shoot for, they may be encouraged to help out. The plan is\nnot working :-(.\n\n--------------------------------------------------\n[Cooking]\n\n* jh/notes-merge-in-git-dir-worktree (2012-03-15) 4 commits\n - notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd\n - notes-merge: use opendir/readdir instead of using read_directory()\n - t3310: illustrate failure to \"notes merge --commit\" inside $GIT_DIR/\n - remove_dir_recursively(): Add flag for skipping removal of toplevel dir\n (this branch is tangled with jc/maint-clean-nested-worktree-in-subdir.)\n\nRunning \"notes merge --commit\" failed to perform correctly when run\nfrom any directory inside $GIT_DIR/.  When \"notes merge\" stops with\nconflicts, $GIT_DIR/NOTES_MERGE_WORKTREE is the place a user edits\nto resolve it.\n\nNot urgent.\n\n* jn/diffstat-tests (2012-03-13) 7 commits\n - diffstat summary line varies by locale: miscellany\n - test: use numstat instead of diffstat in binary-diff test\n - test: use --numstat instead of --stat in \"git stash show\" tests\n - test: test cherry-pick functionality and output separately\n - test: modernize funny-names test style\n - test: use numstat instead of diffstat in funny-names test\n - test: use test_i18ncmp when checking --stat output\n\nSome tests checked the \"diff --stat\" output when they do not have to,\nwhich unnecessarily made things harder to verify under GETTEXT_POISON.\n\nNot urgent.\n\n* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits\n - diff: tweak a _copy_ of diff_options with word-diff\n - diff: refactor the word-diff setup from builtin_diff_cmd\n - t4034: diff.*.wordregex should not be \"sticky\" in --word-diff\n\nThe regexp configured with wordregex was incorrectly reused across\nfiles.\nNot urgent.\n\n* zj/test-cred-helper-nicer-prove (2012-03-15) 2 commits\n - t0303: resurrect commit message as test documentation\n - t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER\n\nMinor improvement to t0303.\nNot urgent.\n\n* jc/commit-hook-authorship (2012-03-11) 3 commits\n  (merged to 'next' on 2012-03-12 at 05ca7f8)\n + commit: pass author/committer info to hooks\n + t7503: does pre-commit-hook learn authorship?\n + ident.c: add split_ident_line() to parse formatted ident line\n (this branch is tangled with jc/run-hook-env-1.)\n\n\"git commit --author=$name\" did not tell the name that was being\nrecorded in the resulting commit to hooks, even though it does do so\nwhen the end user overrode the authorship via the \"GIT_AUTHOR_NAME\"\nenvironment variable.  This is a simpler of the two approaches.\n\nWill defer til 1.7.10.\n\n* jc/run-hook-env-1 (2012-03-11) 3 commits\n - run_hook(): enhance the interface to pass arbitrary environment\n + t7503: does pre-commit-hook learn authorship?\n + ident.c: add split_ident_line() to parse formatted ident line\n (this branch is tangled with jc/commit-hook-authorship.)\n\nNot urgent.\n\nUpdates run_hook() API to be much less specific to \"commit\".  It would\nonly be useful if people start doing more interesting things with hooks.\n\n* jc/diff-algo-cleanup (2012-02-19) 2 commits\n  (merged to 'next' on 2012-03-15 at cca0032)\n + xdiff: PATIENCE/HISTOGRAM are not independent option bits\n + xdiff: remove XDL_PATCH_* macros\n (this branch is used by jc/diff-ignore-case.)\n\nResurrects the preparatory clean-up patches from another topic that was\ndiscarded, as this would give a saner foundation to build on diff.algo\nconfiguration option series.\n\nNot urgent.\n\n* jh/apply-free-patch (2012-03-07) 1 commit\n - apply: do not leak patches and fragments\n\nWill defer til 1.7.10.\n\n* rs/unpack-trees-leakfix (2012-03-06) 1 commit\n  (merged to 'next' on 2012-03-07 at 69a69cd)\n + unpack-trees: plug minor memory leak\n\nWill defer til 1.7.10.\n\n* tb/maint-remove-irrelevant-i18n-test (2012-03-06) 1 commit\n  (merged to 'next' on 2012-03-07 at 23f2dd1)\n + t0204: remove a test that checks undefined behaviour\n\nI tentatively parked this in 'next' but later reverted the merge.\nWill discard.\n\n* mm/push-default-switch-warning (2012-03-09) 1 commit\n - push: start warning upcoming default change for push.default\n\nNot urgent.\n\nThis resurrects an ancient patch I wrote during a discussion we had in the\n1.6.3-1.6.4 era.  This should probably come after ct/advise-push-default\ntopic and at that point the advise messages need to be rephrased, taking\nthe future default change into account.\n\n* jc/fmt-merge-msg-people (2012-03-13) 1 commit\n - fmt-merge-msg: show those involved in a merged series\n\nThe \"fmt-merge-msg\" command learns to list the primary contributors\ninvolved in the side topic you are merging.\n\nWill defer til 1.7.10.\n\n* nl/http-proxy-more (2012-03-15) 5 commits\n - http: rename HTTP_REAUTH to HTTP_AUTH_RETRY\n - http: Avoid limit of retrying request only twice\n - http: handle proxy authentication failure (error 407)\n - http: handle proxy proactive authentication\n - http: try http_proxy env var when http.proxy config option is not set\n\nThe code to talk to http proxies learn to use the same credential\nAPI used to talk to the final http destinations.\n\nWill defer til 1.7.10.\n\n* nd/stream-more (2012-03-07) 7 commits\n  (merged to 'next' on 2012-03-07 at 7325922)\n + update-server-info: respect core.bigfilethreshold\n + fsck: use streaming API for writing lost-found blobs\n + show: use streaming API for showing blobs\n + parse_object: avoid putting whole blob in core\n + cat-file: use streaming API to print blobs\n + Add more large blob test cases\n + streaming: make streaming-write-entry to be more reusable\n\nUse API to read blob data in smaller chunks in more places to\nreduce the memory footprint.  In general, looked fairly good.\n\nWill defer til 1.7.10.\n\n--------------------------------------------------\n[Discarded]\n\n* jc/diff-ignore-mode (2012-03-01) 1 commit\n . diff --ignore-mode-change\n\n * jc/diff-ignore-case (2012-02-28) 6 commits\n . diff: -i is \"--ignore-case\" but means a bit more in \"log\"\n . diff: --ignore-case\n . xdiff: introduce XDF_IGNORE_CASE\n . xdiff: introduce XDF_INEXACT_MATCH\n - xdiff: PATIENCE/HISTOGRAM are not independent option bits\n - xdiff: remove XDL_PATCH_* macros\n\n* jh/trace-use-startup-info (2012-03-02) 1 commit\n . Use startup_info->prefix rather than prefix.\n\nI tend to agree with the doubt of the author of this patch expressed.\n\n* nd/optim-connected (2012-03-15) 1 commit\n . {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack\n\njc/maint-verify-objects-remove-pessimism replaces this.\n\n* jc/advise-push-default (2011-12-18) 1 commit\n . push: hint to use push.default=upstream when appropriate\n\nA rework by Christopher Tiwald replaces this.\n\n* th/mergetools-tool-help (2012-03-15) 1 commit\n . difftool: print list of valid tools with '--tool-help'\n\nReworked as part of th/difftool-diffall topic.\n"},{"id":"187300","messageId":"20120320014853.GA21052@odin.tremily.us","threadId":"30001","inReplyTo":"7vmx7c86j8.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-20T01:48:53Z","receivedAt":"2012-03-20T01:48:53Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Thu, Mar 15, 2012 at 03:54:07AM -0400, W. Trevor King wrote:\n> The current gitweb only generates Last-Modified and handles\n> If-Modified-Since headers for the git_feed action.  This patch…\n\nOn Mon, Mar 19, 2012 at 05:23:07PM -0700, Junio C Hamano wrote:\n> What's cooking in git.git (Mar 2012, #07; Mon, 19)\n> …\n\nMy If-Modified-Since patch seems to have fallen through the cracks\nhere.  Is that because nobody is interested, or did I mess up\nsomething in the submission?\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187365","messageId":"7vvcly50tg.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"20120320014853.GA21052@odin.tremily.us","subject":"Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T23:07:07Z","receivedAt":"2012-03-20T23:07:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@drexel.edu> writes:\n\n> My If-Modified-Since patch seems to have fallen through the cracks\n> here.  Is that because nobody is interested, or did I mess up\n> something in the submission?\n\nProbably \"just fell through the cracks\"; if somebody found what it wants\nto do very interesting but the implementation or submission lacking, you\nwould have heard loud noises.\n"},{"id":"187369","messageId":"7v62dy4zhf.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"7vmx7c86j8.fsf@alter.siamese.dyndns.org","subject":"Incremental updates to What's cooking","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-20T23:35:56Z","receivedAt":"2012-03-20T23:35:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Again, just the highlights of the notable topics.\n\nNot much has happened on the 'master' front.\n\n[New Topics]\n\n* am/completion-zsh-fix (2012-03-20) 1 commit\n - contrib/completion: \"local var=()\" is misinterpreted as function decl by zsh\n\nNeeds a bit better explanation.\n\n* lp/maint-diff-three-dash-with-graph (2012-03-20) 3 commits\n - t4202: add test for \"log --graph --stat -p\" separator lines\n - log --graph: fix break in graph lines\n - log --graph --stat: three-dash separator should come after graph lines\n\nThe combination of two options \"log --graph --stat\" was an obscure corner\ncase nobody cared about, and did not correctly show the ancestry graph\nlines.\n\nI've split the original patch into three pieces, one for fixes to two\ndifferent issues and a test.  Also the test is adjusted so that the series\ncan be back-merged to older codebase that did not have 7f81463 (Use\ncorrect grammar in diffstat summary line, 2012-02-01) that first appeared\nin v1.7.9.2\n\n* wk/gitweb-snapshot-use-if-modified-since (2012-03-20) 1 commit\n - Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.\n\nUnreviewed; the title looks way too long and does not sit well in the\nshortlog output.\n\n* jc/maint-merge-autoedit (2012-03-20) 1 commit\n - merge: backport GIT_MERGE_AUTOEDIT support\n\nIn 1.7.10, we added GIT_MERGE_AUTOEDIT=no environment variable to help\nolder scripts to let them refuse giving users a chance to explain the\nmerge, but forgot that 1.7.9 automatically opens an editor when merging an\nannotated tag, and there is no equivalent escape hatch.  A merge of this\ntopic to 1.7.10 track becomes a no-op, but we may want to apply this to\nthe 1.7.9.x series.\n\n[Cooking]\n\n* ct/advise-push-default (2012-03-19) 1 commit\n - push: Provide situational hints for non-fast-forward error\n\nBreaks down the cases in which \"git push\" fails due to non-ff into three\ncategories, and gives separate advise messages.  This should be a good\nchange regardless of mm/push-default-switch-warning topic.\n\n* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits\n  (merged to 'next' on 2012-03-20 at b3f67cd)\n + diff: tweak a _copy_ of diff_options with word-diff\n + diff: refactor the word-diff setup from builtin_diff_cmd\n + t4034: diff.*.wordregex should not be \"sticky\" in --word-diff\n\nIt would be nice to have this fix in 1.7.10 but perhaps we can wait until\n1.7.10.1.\n\n* nl/http-proxy-more (2012-03-15) 5 commits\n  (merged to 'next' on 2012-03-20 at c004001)\n + http: rename HTTP_REAUTH to HTTP_AUTH_RETRY\n + http: Avoid limit of retrying request only twice\n + http: handle proxy authentication failure (error 407)\n + http: handle proxy proactive authentication\n + http: try http_proxy env var when http.proxy config option is not set\n\nThe code to talk to http proxies learn to use the same credential\nAPI used to talk to the final http destinations.\n\n[Stalled]\n\n* th/difftool-diffall (2012-03-19) 9 commits\n . difftool: print list of valid tools with '--tool-help'\n . difftool: teach dir-diff to copy modified files back to working tree\n . difftool: teach difftool to handle directory diffs\n . difftool: replace system call with Git::command_noisy\n . difftool: eliminate setup_environment function\n . difftool: stop appending '.exe' to git\n . difftool: remove explicit change of PATH\n . difftool: exit(0) when usage is printed\n . difftool: parse options using Getopt::Long\n\nReworks the \"diffall\" contrib script into \"difftool\" framework.\nBreaks tests when merged to 'pu'.\n\n* jh/apply-free-patch (2012-03-20) 2 commits\n - apply: free patch->result\n - apply: do not leak patches and fragments\n\nValgrind reports quite a lot of discarded memory inside apply. I have a\nslight suspicion that we should first clarify the ownership rules of\npieces of memory in this standalone program that was designed to be \"run\nonce and let exit take care of the memory\" before proceeding further.\n\n* dg/test-from-elsewhere (2012-03-04) 2 commits\n - Support out-of-tree Valgrind tests\n - Allow overriding GIT_BUILD_DIR\n\nBetter support for out-of-tree test scripts, but it appears that the\napproach needs to be rethought.  By repointing TEST_DIRECTORY to a\ndirectory other than $(pwd)/.., an out of place test script can reach\ntest helpers and freshly built Git relative to it (GIT_BUILD_DIR is\na mere short-hand for $TEST_DIRECTORY/..).\n\nDiscussion stalled.\n"},{"id":"187390","messageId":"20120321121126.GA27660@odin.tremily.us","threadId":"30001","inReplyTo":"7v62dy4zhf.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-21T12:11:27Z","receivedAt":"2012-03-21T12:11:27Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The current gitweb only generates Last-Modified and handles\nIf-Modified-Since headers for the git_feed action.  This patch breaks\nthe Last-Modified and If-Modified-Since handling code out from\ngit_feed into a new function die_if_unmodified.  This makes the code\neasy to reuse for other actions where it is appropriate and adds the\ncode to do that to git_snapshot.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>                                 \n---\nChanged since v2:\n  - Shorter title.\n  - Fixed modified_since -> die_if_unmodified in commit message.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..b944351 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7003,6 +7003,28 @@ sub snapshot_name {\n \treturn wantarray ? ($name, $name) : $name;\n }\n \n+sub die_if_unmodified {\n+\tmy ($latest_epoch) = @_;\n+\tour $cgi;\n+\n+\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n+\tif (defined $if_modified) {\n+\t\tmy $since;\n+\t\tif (eval { require HTTP::Date; 1; }) {\n+\t\t\t$since = HTTP::Date::str2time($if_modified);\n+\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n+\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n+\t\t}\n+\t\tif (defined $since && $latest_epoch <= $since) {\n+\t\t\tmy %latest_date = parse_date($latest_epoch);\n+\t\t\tprint $cgi->header(\n+\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t\t-status => '304 Not Modified');\n+\t\t\tgoto DONE_GITWEB;\n+\t\t}\n+\t}\n+}\n+\n sub git_snapshot {\n \tmy $format = $input_params{'snapshot_format'};\n \tif (!@snapshot_fmts) {\n@@ -7029,6 +7051,10 @@ sub git_snapshot {\n \n \tmy ($name, $prefix) = snapshot_name($project, $hash);\n \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n+\n+\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n+\tdie_if_unmodified($co{'committer_epoch'});\n+\n \tmy $cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n@@ -7038,9 +7064,11 @@ sub git_snapshot {\n \t}\n \n \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n+\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \tprint $cgi->header(\n \t\t-type => $known_snapshot_formats{$format}{'type'},\n \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t-last_modified => $latest_date{'rfc2822'},\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n@@ -7820,24 +7848,8 @@ sub git_feed {\n \tif (defined($commitlist[0])) {\n \t\t%latest_commit = %{$commitlist[0]};\n \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n+\t\tdie_if_unmodified($latest_epoch);\n \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n-\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n-\t\tif (defined $if_modified) {\n-\t\t\tmy $since;\n-\t\t\tif (eval { require HTTP::Date; 1; }) {\n-\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n-\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n-\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n-\t\t\t}\n-\t\t\tif (defined $since && $latest_epoch <= $since) {\n-\t\t\t\tprint $cgi->header(\n-\t\t\t\t\t-type => $content_type,\n-\t\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t\t\t-status => '304 Not Modified');\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n \t\t\t-charset => 'utf-8',\n--                                                                              \n1.7.3.4                                                                         \n"},{"id":"187394","messageId":"m3sjh2ay6j.fsf@localhost.localdomain","threadId":"30001","inReplyTo":"20120321121126.GA27660@odin.tremily.us","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-21T13:19:51Z","receivedAt":"2012-03-21T13:19:51Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"By the way, it is custom on this mailing list to usually Cc (send a\ncopy) to all people participating in discussion, and not only to git\nmailing list.\n\n\"W. Trevor King\" <wking@drexel.edu> writes:\n\n> Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb\n\nPerhaps a better title would be:\n\n  gitweb: Refactor If-Modified-Since handling, support in snapshot\n\nto mention all that thispatch does.  Though trouble with coming up\nwith a short but fairly complete one-line summary might mean that this\npatch would be better split in two: refactoring and adding support for\nIf-Modified-Since to snapshots.\n\n> The current gitweb only generates Last-Modified and handles\n> If-Modified-Since headers for the git_feed action.  This patch breaks\n> the Last-Modified and If-Modified-Since handling code out from\n> git_feed into a new function die_if_unmodified.  This makes the code\n> easy to reuse for other actions\n\nO.K.\n\n>                                  where it is appropriate\n                                   ^^^^^^^^^^^^^^^^^^^^^^^\n\nThis doesn't add any information.  I think it the commit message would\nbe better if you either remove this, or expand (in a separate\nparagraph) where support for If-Modified-Since might make sense, and\nwhere it could not.\n\n>                                                         and adds the\n> code to do that to git_snapshot.\n\nThis would read better as a separate paragraph.\n \n> Signed-off-by: W Trevor King <wking@drexel.edu>                                 \n> ---\n> Changed since v2:\n>   - Shorter title.\n>   - Fixed modified_since -> die_if_unmodified in commit message.\n\nNice.\n\nBTW. what happened to diffstat?\n\nTests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?\nWe could use test_tick() and $test_tick for that (or extract formatted\ncommitter date from a commit).\n \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a8b5fad..b944351 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -7003,6 +7003,28 @@ sub snapshot_name {\n>  \treturn wantarray ? ($name, $name) : $name;\n>  }\n>  \n> +sub die_if_unmodified {\n> +\tmy ($latest_epoch) = @_;\n> +\tour $cgi;\n> +\n> +\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> +\tif (defined $if_modified) {\n> +\t\tmy $since;\n> +\t\tif (eval { require HTTP::Date; 1; }) {\n> +\t\t\t$since = HTTP::Date::str2time($if_modified);\n> +\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> +\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> +\t\t}\n> +\t\tif (defined $since && $latest_epoch <= $since) {\n> +\t\t\tmy %latest_date = parse_date($latest_epoch);\n> +\t\t\tprint $cgi->header(\n> +\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> +\t\t\t\t-status => '304 Not Modified');\n> +\t\t\tgoto DONE_GITWEB;\n> +\t\t}\n> +\t}\n> +}\n\nO.K.\n\ndie_if_unmodified() is good enough name, though I wonder if we could\ncome up with a better name....\n\n> +\n>  sub git_snapshot {\n>  \tmy $format = $input_params{'snapshot_format'};\n>  \tif (!@snapshot_fmts) {\n> @@ -7029,6 +7051,10 @@ sub git_snapshot {\n>  \n>  \tmy ($name, $prefix) = snapshot_name($project, $hash);\n>  \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> +\n> +\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n> +\tdie_if_unmodified($co{'committer_epoch'});\n> +\n\nThat unexplainably changes behavior of 'snapshot' action.  Before we\nwould accept snapshot of a tree given by its SHA-1, now we do not.\n\nThis might be a good idea from a security point of view wrt. leaking\ninformation (c.f. \"git archive --remote\" behavior), but it at least\nneeds to be explained in commit message, and perhaps even in a comment\nabove this line.\n\nAlternative solution would be to skip If-Modified-Since handling if we\nrequest snapshot by tree id:\n\n  +\n  +\tmy %co = parse_commit($hash);\n  +\tdie_if_unmodified($co{'committer_epoch'}) if %co;\n  +\n\n> @@ -7038,9 +7064,11 @@ sub git_snapshot {\n>  \t}\n>  \n>  \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n> +\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n>  \tprint $cgi->header(\n>  \t\t-type => $known_snapshot_formats{$format}{'type'},\n>  \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> +\t\t-last_modified => $latest_date{'rfc2822'},\n>  \t\t-status => '200 OK');\n\nOf course this would need to be updated too.\n  \n>  \topen my $fd, \"-|\", $cmd\n> @@ -7820,24 +7848,8 @@ sub git_feed {\n>  \tif (defined($commitlist[0])) {\n>  \t\t%latest_commit = %{$commitlist[0]};\n>  \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n> +\t\tdie_if_unmodified($latest_epoch);\n>  \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n> -\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> -\t\tif (defined $if_modified) {\n> -\t\t\tmy $since;\n> -\t\t\tif (eval { require HTTP::Date; 1; }) {\n> -\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n> -\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> -\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> -\t\t\t}\n> -\t\t\tif (defined $since && $latest_epoch <= $since) {\n> -\t\t\t\tprint $cgi->header(\n> -\t\t\t\t\t-type => $content_type,\n> -\t\t\t\t\t-charset => 'utf-8',\n> -\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> -\t\t\t\t\t-status => '304 Not Modified');\n> -\t\t\t\treturn;\n> -\t\t\t}\n> -\t\t}\n>  \t\tprint $cgi->header(\n>  \t\t\t-type => $content_type,\n>  \t\t\t-charset => 'utf-8',\n\nNice.\n\n-- \nJakub Narebski\n"},{"id":"187399","messageId":"20120321140429.GA28721@odin.tremily.us","threadId":"30001","inReplyTo":"m3sjh2ay6j.fsf@localhost.localdomain","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-21T14:04:30Z","receivedAt":"2012-03-21T14:04:30Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Wed, Mar 21, 2012 at 06:19:51AM -0700, Jakub Narebski wrote:\n> By the way, it is custom on this mailing list to usually Cc (send a\n> copy) to all people participating in discussion, and not only to git\n> mailing list.\n\nAh, sorry.  I figured that if you got the original email to the list,\nI'd just be doubling up in your inbox by Cc-ing you directly.\n\n> Though trouble with coming up with a short but fairly complete\n> one-line summary might mean that this patch would be better split in\n> two: refactoring and adding support for If-Modified-Since to\n> snapshots.\n\nAgreed.\n\n> >                                  where it is appropriate\n>                                    ^^^^^^^^^^^^^^^^^^^^^^^\n> This doesn't add any information.  I think it the commit message would\n> be better if you either remove this, or expand (in a separate\n> paragraph) where support for If-Modified-Since might make sense, and\n> where it could not.\n\nI'll expand it in the refactoring patch.\n\n> BTW. what happened to diffstat?\n\nMy local branch has sequential commits for each patch version (e.g.,\ncommits for v1, v2, ...).  When it's time to email the list, I'm\nsupposed to send logical patches against the trunk (e.g., patches for\nrefactoring, git_snapshot, ...).  For v2 I just used `git diff\norigin/master` to generate the patch, and it doesn't include the\ndiffstat.  Now that I'm splitting into two patches, I'll probably use\n`git rebase -i origin/master` and just keep track of the changes by\nhand ;).  If there's a better way that I'm overlooking, feel free to\npoint me in the right direction.\n\n> Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?\n> We could use test_tick() and $test_tick for that (or extract formatted\n> committer date from a commit).\n\nI'll try to set that up.  Should it be bundled into the git_snapshot\npatch, or should there be three patches:\n\n1: gitweb: Refactor If-Modified-Since handling\n2: gitweb: Add If-Modified-Since tests for git_snapshot\n3: gitweb: Add If-Modified-Since support for git_snapshot\n\n> > @@ -7029,6 +7051,10 @@ sub git_snapshot {\n> >  \n> >  \tmy ($name, $prefix) = snapshot_name($project, $hash);\n> >  \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> > +\n> > +\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n> > +\tdie_if_unmodified($co{'committer_epoch'});\n> > +\n> \n> That unexplainably changes behavior of 'snapshot' action.  Before we\n> would accept snapshot of a tree given by its SHA-1, now we do not.\n> \n> This might be a good idea from a security point of view wrt. leaking\n> information (c.f. \"git archive --remote\" behavior), but it at least\n> needs to be explained in commit message, and perhaps even in a comment\n> above this line.\n> \n> Alternative solution would be to skip If-Modified-Since handling if we\n> request snapshot by tree id:\n> \n>   +\n>   +\tmy %co = parse_commit($hash);\n>   +\tdie_if_unmodified($co{'committer_epoch'}) if %co;\n>   +\n\nI'm still not understanding the problem here.  The following all work\nin my testing:\n\n  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d545cab4a8dc894fae2c2634a74993ea62b054d;sf=tgz\n  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d5;sf=tgz\n  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=HEAD;sf=tgz\n\nCan you give me an example of a hash that you expect to not work?\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187404","messageId":"201203211755.07121.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120321140429.GA28721@odin.tremily.us","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-21T16:55:06Z","receivedAt":"2012-03-21T16:55:06Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, Mar 21, 2012, W. Trevor King wrote:\n> On Wed, Mar 21, 2012 at 06:19:51AM -0700, Jakub Narebski wrote:\n\n> > By the way, it is custom on this mailing list to usually Cc (send a\n> > copy) to all people participating in discussion, and not only to git\n> > mailing list.\n> \n> Ah, sorry.  I figured that if you got the original email to the list,\n> I'd just be doubling up in your inbox by Cc-ing you directly.\n\nI am not subscribed to git mailing list, and I am reading it and\nresponding (to those emails that I'm not Cc-ed) via GMane's NNTP\n(Usenet) interface:\n\n  nntp://news.gmane.org/gmane.comp.version-control.git\n\nAnd I think I am not the only one.  But I guess that even for those\nsubscribed, emails addressed directly to them take priority, which\nis important considering volume of email on git mailing list.\n \n[..]\n> > BTW. what happened to diffstat?\n> \n> My local branch has sequential commits for each patch version (e.g.,\n> commits for v1, v2, ...).  When it's time to email the list, I'm\n> supposed to send logical patches against the trunk (e.g., patches for\n> refactoring, git_snapshot, ...).  For v2 I just used `git diff\n> origin/master` to generate the patch, and it doesn't include the\n> diffstat.  Now that I'm splitting into two patches, I'll probably use\n> `git rebase -i origin/master` and just keep track of the changes by\n> hand ;).  If there's a better way that I'm overlooking, feel free to\n> point me in the right direction.\n\nThere is a tool to create patches to send: git-format-patch.  Myself I\nusually create a new directory for a patch series, e.g. mdir.if_mod.v3,\nand use git-format-patch to populate it, e.g.\n\n  $ git format-patch origin/master.. -o mdir.if_mod.3/ \\\n                     --cover-letter --subject-prefix=\"PATCH v4\"\n\nNote that you need to edit at least cover letter.\n\nBTW. \"git diff\" has '--stat -p' and '--patch-with-stat' options :-)\n\n> > Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?\n> > We could use test_tick() and $test_tick for that (or extract formatted\n> > committer date from a commit).\n> \n> I'll try to set that up.  Should it be bundled into the git_snapshot\n> patch, or should there be three patches:\n> \n> 1: gitweb: Refactor If-Modified-Since handling\n> 2: gitweb: Add If-Modified-Since tests for git_snapshot\n> 3: gitweb: Add If-Modified-Since support for git_snapshot\n\nI think it would be better to add initial tests with refactoring, and\nsnapshot specific tests with snapshot support, e.g.:\n\n  1/2: gitweb: Refactor If-Modified-Since handling and add tests\n  2/2: gitweb: Add If-Modified-Since support for snapshots\n\n> > > @@ -7029,6 +7051,10 @@ sub git_snapshot {\n> > >  \n> > >  \tmy ($name, $prefix) = snapshot_name($project, $hash);\n> > >  \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> > > +\n> > > +\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n> > > +\tdie_if_unmodified($co{'committer_epoch'});\n> > > +\n> > \n> > That unexplainably changes behavior of 'snapshot' action.  Before we\n> > would accept snapshot of a tree given by its SHA-1, now we do not.\n> > \n> > This might be a good idea from a security point of view wrt. leaking\n> > information (c.f. \"git archive --remote\" behavior), but it at least\n> > needs to be explained in commit message, and perhaps even in a comment\n> > above this line.\n> > \n> > Alternative solution would be to skip If-Modified-Since handling if we\n> > request snapshot by tree id:\n> > \n> >   +\n> >   +\tmy %co = parse_commit($hash);\n> >   +\tdie_if_unmodified($co{'committer_epoch'}) if %co;\n> >   +\n> \n> I'm still not understanding the problem here.  The following all work\n> in my testing:\n\nBut wouldn't work in my clone... ;-(\n\n>   http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d545cab4a8dc894fae2c2634a74993ea62b054d;sf=tgz\n>   http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d5;sf=tgz\n>   http://.../gitweb.cgi?p=a/.git;a=snapshot;h=HEAD;sf=tgz\n> \n> Can you give me an example of a hash that you expect to not work?\n\nCurrently all of those work\n\n    http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz\n    http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz\"\n    http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz\"\n\nv1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree\n(v1.7.6^{tree}).\n\nThe last URL would stop working after your change with 404\n\"Unknown commit object\".\n\nI'm not sure but I think that currently 'snapshot' link in the navbar\nof the 'tree' view uses that kind of link, with 'h' parameter set to\nSHA-1 of a tree.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187408","messageId":"7v1uol3m5m.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"m3sjh2ay6j.fsf@localhost.localdomain","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-21T17:21:25Z","receivedAt":"2012-03-21T17:21:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> By the way, it is custom on this mailing list to usually Cc (send a\n> copy) to all people participating in discussion, and not only to git\n> mailing list.\n>\n> \"W. Trevor King\" <wking@drexel.edu> writes:\n>\n>> Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb\n>\n> Perhaps a better title would be:\n>\n>   gitweb: Refactor If-Modified-Since handling, support in snapshot\n\nWith \"gitweb: \" prefix to denote what area it affects, that is certainly\nbetter.  Given the primary objective and effect is that the snapshot\nfeature starts honoring i-m-s,\n\n\tgitweb: honor If-Modified-Since request header in snapshot\n\nwould be sufficient.\n\n> to mention all that thispatch does.  Though trouble with coming up\n> with a short but fairly complete one-line summary might mean that this\n> patch would be better split in two: refactoring and adding support for\n> If-Modified-Since to snapshots.\n\nIf many existing callsites had duplicated code to handle i-m-s, we may\nwant two patch series, the first of which consolidates them into a single\nhelper function without changing anything else (most importantly, without\nregression) and the second that uses the helper to add support in the\nsnapshot feature.  But if that is not the case, I think we can go either\nway.\n"},{"id":"187414","messageId":"20120321173824.GA31490@odin.tremily.us","threadId":"30001","inReplyTo":"201203211755.07121.jnareb@gmail.com","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-21T17:38:24Z","receivedAt":"2012-03-21T17:38:24Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Wed, Mar 21, 2012 at 05:55:06PM +0100, Jakub Narebski wrote:\n> There is a tool to create patches to send: git-format-patch.  Myself I\n> usually create a new directory for a patch series, e.g. mdir.if_mod.v3,\n> and use git-format-patch to populate it, e.g.\n\nAh, I like that.  Then I can rebase away ;).\n\n> I think it would be better to add initial tests with refactoring, and\n> snapshot specific tests with snapshot support, e.g.:\n> \n>   1/2: gitweb: Refactor If-Modified-Since handling and add tests\n>   2/2: gitweb: Add If-Modified-Since support for snapshots\n\nBut the new tests would be for the new functionality (i.e. snapshot\nsupport), so they wouldn't belong in the general refactoring commit.\n\n> Currently all of those work\n> \n>     http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz\n>     http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz\"\n>     http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz\"\n> \n> v1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree\n> (v1.7.6^{tree}).\n> \n> The last URL would stop working after your change with 404\n> \"Unknown commit object\".\n\nIndeed it does.  I'll add you're suggested skipping for these cases.\n\nThere should be a way to determine the oldest commit refering to a\ntree, which could be used for timestamping tree-ishes, but that can be\na project for another day ;).\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187417","messageId":"7vmx7921yz.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"20120321173824.GA31490@odin.tremily.us","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-21T19:22:44Z","receivedAt":"2012-03-21T19:22:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@drexel.edu> writes:\n\n>> I think it would be better to add initial tests with refactoring, and\n>> snapshot specific tests with snapshot support, e.g.:\n>> \n>>   1/2: gitweb: Refactor If-Modified-Since handling and add tests\n>>   2/2: gitweb: Add If-Modified-Since support for snapshots\n>\n> But the new tests would be for the new functionality (i.e. snapshot\n> support), so they wouldn't belong in the general refactoring commit.\n\nThen you are planning to split it in a wrong way.\n\nAs I said, I do not think it matters that much for a small patch like\nthis, but if the plan is to make the part to create i-m-s helper function\nas a standalone \"refactoring\" patch, then what Jakub outlined is the right\nway to go about it.\n\nIn the first patch, you create i-m-s helper and update the existing code\nthat can use the helper, without changing anything else. Do not touch\nsnapshot code in this patch, if the current code does not support i-m-s in\nsnapshot.  And in the same patch, add tests for codepaths that use i-m-s\nto make sure your refactoring did not break them.  In other words, if you\nremove the change to gitweb/ from the first patch and apply only the\nchanges to the tests, the resulting new tests should pass with the current\ncode that has i-m-s support inline without the i-m-s helper.  And if you\nadd back your change to gitweb/ for the refactoring, the test should still\npass.\n\nAnd then in the second patch, you update the snapshot code and whatever\nelse that can use i-m-s helper to support i-m-s.  You can add tests to\nprotect the new feature from future breakages in this patch.\n"},{"id":"187433","messageId":"20120321195520.GB32535@odin.tremily.us","threadId":"30001","inReplyTo":"7vmx7921yz.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-21T19:55:20Z","receivedAt":"2012-03-21T19:55:20Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote:\n> \"W. Trevor King\" <wking@drexel.edu> writes:\n> >> I think it would be better to add initial tests with refactoring, and\n> >> snapshot specific tests with snapshot support, e.g.:\n> >> \n> >>   1/2: gitweb: Refactor If-Modified-Since handling and add tests\n> >>   2/2: gitweb: Add If-Modified-Since support for snapshots\n> >\n> > But the new tests would be for the new functionality (i.e. snapshot\n> > support), so they wouldn't belong in the general refactoring commit.\n> \n> Then you are planning to split it in a wrong way.\n> \n> ... add tests for codepaths that use i-m-s to make sure your\n> refactoring did not break them...\n\nAh, I was assuming that some current tests might be checking the\ncurrent behavior, and that my new tests would be testing my new\nsnapshot behavior.  If the old i-m-s handling also needs tests, that\nshould happen before any of my previously proposed patches:\n\n1: tests for i-m-s and git_feed\n2: refactor i-m-s handling\n3: tests for i-m-s and git_snapshot (which fail until 4)\n4: add i-m-s to git_snapshot\n\nHow does that sound?\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187426","messageId":"201203212104.23192.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120321195520.GB32535@odin.tremily.us","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-21T20:04:22Z","receivedAt":"2012-03-21T20:04:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W. Trevor King wrote:\n> On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote:\n> > \"W. Trevor King\" <wking@drexel.edu> writes:\n\n> > > > I think it would be better to add initial tests with refactoring, and\n> > > > snapshot specific tests with snapshot support, e.g.:\n> > > > \n> > > >   1/2: gitweb: Refactor If-Modified-Since handling and add tests\n> > > >   2/2: gitweb: Add If-Modified-Since support for snapshots\n> > >\n> > > But the new tests would be for the new functionality (i.e. snapshot\n> > > support), so they wouldn't belong in the general refactoring commit.\n> > \n> > Then you are planning to split it in a wrong way.\n> > \n> > ... add tests for codepaths that use i-m-s to make sure your\n> > refactoring did not break them...\n> \n> Ah, I was assuming that some current tests might be checking the\n> current behavior, and that my new tests would be testing my new\n> snapshot behavior.  If the old i-m-s handling also needs tests, that\n> should happen before any of my previously proposed patches:\n> \n> 1: tests for i-m-s and git_feed\n> 2: refactor i-m-s handling\n\nThose two can be in single commit.  Tests added need only test i-m-s\nusing git_feed ('atom' or 'rss' action), as it is the only user,\nand you touch only i-m-s handling.\n\n> 3: tests for i-m-s and git_snapshot (which fail until 4)\n> 4: add i-m-s to git_snapshot\n\nWe usually put tests together with feature.  Tests before feature means\nthat you would need to mark them as test_expect_failure, as they would\nnot pass before feature is added, isn't it?\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187427","messageId":"7viphx1zt7.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"201203212104.23192.jnareb@gmail.com","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-21T20:09:24Z","receivedAt":"2012-03-21T20:09:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> 1: tests for i-m-s and git_feed\n>> 2: refactor i-m-s handling\n>\n> Those two can be in single commit.  Tests added need only test i-m-s\n> using git_feed ('atom' or 'rss' action), as it is the only user,\n> and you touch only i-m-s handling.\n\nCorrect.\n\n>> 3: tests for i-m-s and git_snapshot (which fail until 4)\n>> 4: add i-m-s to git_snapshot\n>\n> We usually put tests together with feature.  Tests before feature means\n> that you would need to mark them as test_expect_failure, as they would\n> not pass before feature is added, isn't it?\n\nCorrect.\n\nIn a more elaborate series, you would first add a lot of tests to nail the\nbehaviour of the existing code down, then refactor or reimplement them in\na different way, add tests that expect failure until the next phase, and\nthen add features and turn expect_failure in tests to expect_success.\n\nBut I do not htink this single patch deserves that complexity.  Does it\neven have to split into three?\n"},{"id":"187431","messageId":"20120321203401.GA660@odin.tremily.us","threadId":"30001","inReplyTo":"7viphx1zt7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-21T20:34:01Z","receivedAt":"2012-03-21T20:34:01Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Wed, Mar 21, 2012 at 01:09:24PM -0700, Junio C Hamano wrote:\n> In a more elaborate series, you would first add a lot of tests to nail the\n> behaviour of the existing code down, then refactor or reimplement them in\n> a different way, add tests that expect failure until the next phase, and\n> then add features and turn expect_failure in tests to expect_success.\n> \n> But I do not htink this single patch deserves that complexity.  Does it\n> even have to split into three?\n\nSorry, testing makes me think of test-driven development ;).  Two\npatches it is (refactor + git_feed tests, git_snapshot + git_snapshot\ntests).\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187460","messageId":"201203221346.35295.jnareb@gmail.com","threadId":"30001","inReplyTo":"7v1uol3m5m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-22T12:46:34Z","receivedAt":"2012-03-22T12:46:34Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 21 Mar 2012, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > By the way, it is custom on this mailing list to usually Cc (send a\n> > copy) to all people participating in discussion, and not only to git\n> > mailing list.\n> >\n> > \"W. Trevor King\" <wking@drexel.edu> writes:\n> >\n> > > Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb\n> >\n> > Perhaps a better title would be:\n> >\n> >   gitweb: Refactor If-Modified-Since handling, support in snapshot\n> \n> With \"gitweb: \" prefix to denote what area it affects, that is certainly\n> better.  Given the primary objective and effect is that the snapshot\n> feature starts honoring i-m-s,\n> \n> \tgitweb: honor If-Modified-Since request header in snapshot\n> \n> would be sufficient.\n\nThat is a very good title... and if all changes were to be put in single\ncommit (see below), that is what we should concentrate on.  Refactoring\nwould be just a detail.\n \n> > to mention all that thispatch does.  Though trouble with coming up\n> > with a short but fairly complete one-line summary might mean that this\n> > patch would be better split in two: refactoring and adding support for\n> > If-Modified-Since to snapshots.\n> \n> If many existing callsites had duplicated code to handle i-m-s, we may\n> want two patch series, the first of which consolidates them into a single\n> helper function without changing anything else (most importantly, without\n> regression) and the second that uses the helper to add support in the\n> snapshot feature.  But if that is not the case, I think we can go either\n> way.\n\nThere is only one callsite, so theoretically we could do it in single\ncommit, refactoring to add support in new callsite...\n\n...if not for the fact that control flow changes from using conditional\nand early return to [longjump] \"exception\" based one.  That is why\nI think it would be better to put tests and refactoring in a commit\nseparate from adding If-Modified-Since handling to 'snapshot' action.\n\ntl;dr.  Two commits.\n-- \nJakub Narebski\nPoland\n"},{"id":"187465","messageId":"201203221405.13193.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120321173824.GA31490@odin.tremily.us","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-22T13:05:12Z","receivedAt":"2012-03-22T13:05:12Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 21 Mar 2012, W. Trevor King wrote:\n> On Wed, Mar 21, 2012 at 05:55:06PM +0100, Jakub Narebski wrote:\n\n> > There is a tool to create patches to send: git-format-patch.  Myself I\n> > usually create a new directory for a patch series, e.g. mdir.if_mod.v3,\n> > and use git-format-patch to populate it, e.g.\n> \n> Ah, I like that.  Then I can rebase away ;).\n\nYep.  Or use some patch / mailqueue management interface like StGit.\n \n> > I think it would be better to add initial tests with refactoring, and\n> > snapshot specific tests with snapshot support, e.g.:\n> > \n> >   1/2: gitweb: Refactor If-Modified-Since handling and add tests\n> >   2/2: gitweb: Add If-Modified-Since support for snapshots\n> \n> But the new tests would be for the new functionality (i.e. snapshot\n> support), so they wouldn't belong in the general refactoring commit.\n\nI was thinking that \"and add tests\" would be about 'feed' action test\nand i-m-s.\n\n> > Currently all of those work\n> > \n> >     http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz\n> >     http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz\"\n> >     http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz\"\n> > \n> > v1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree\n> > (v1.7.6^{tree}).\n> > \n> > The last URL would stop working after your change with 404\n> > \"Unknown commit object\".\n> \n> Indeed it does.  I'll add you're suggested skipping for these cases.\n\nIf by \"skipping\" you mean here turning off handling of If-Modified-Since\nand Last-Modified headers, then I agree.\n \n> There should be a way to determine the oldest commit refering to a\n> tree, which could be used for timestamping tree-ishes, but that can be\n> a project for another day ;).\n\nThat also can be ambiguous.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187478","messageId":"7vy5qsy3jj.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"201203221346.35295.jnareb@gmail.com","subject":"Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-22T17:00:00Z","receivedAt":"2012-03-22T17:00:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> tl;dr.  Two commits.\n\nOK, thanks.\n"},{"id":"187711","messageId":"20120326110943.GA2951@odin.tremily.us","threadId":"30001","inReplyTo":"201203221346.35295.jnareb@gmail.com","subject":"Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T11:09:46Z","receivedAt":"2012-03-26T11:09:46Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"Sorry for the delay since my last message, it's been a busy week ;).\n\nOn Thu, Mar 22, 2012 at 01:46:34PM +0100, Jakub Narebski wrote:\n> ...if not for the fact that control flow changes from using conditional\n> and early return to [longjump] \"exception\" based one.  That is why\n> I think it would be better to put tests and refactoring in a commit\n> separate from adding If-Modified-Since handling to 'snapshot' action.\n\nI'll be sending along three patches.  The second and third are the\nones you discuss above.  The first is a teensy patch to add `Status`\noutput to non-304 calls to git_feed().  Without it you'd have to get a\nbit more creative in the test suite.  If the status lines were left\nout intentionally, let me know, and I'll come up with another\ncondition for those tests.\n\nThe second patch refactors and tests git_feed(), and the third patch\nadds i-m-s support to git_snapshot() with associated tests.\n\nChanges since v3:\n* Patch 1/3 is completely new.\n* Split previous patch into 2/3 and 3/3.\n* Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}.\n* Added testing to both 2/3 and 3/3.\n* Reworked commit messages.\n\nCheers,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187712","messageId":"20120326111110.GB2951@odin.tremily.us","threadId":"30001","inReplyTo":"20120326110943.GA2951@odin.tremily.us","subject":"[PATCH v4 1/3] gitweb: add `status` headers to git_feed() responses.","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T11:11:10Z","receivedAt":"2012-03-26T11:11:10Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The git_feed() method was not setting a `Status` header unless it was\nresponding to an If-Modified-Since request with `304 Not Modified`.\nNow, when it is serving successful responses, it sets status to `200\nOK`.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..041da17 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7841,11 +7841,13 @@ sub git_feed {\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n \t\t\t-charset => 'utf-8',\n-\t\t\t-last_modified => $latest_date{'rfc2822'});\n+\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t-status => '200 OK');\n \t} else {\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n-\t\t\t-charset => 'utf-8');\n+\t\t\t-charset => 'utf-8',\n+\t\t\t-status => '200 OK');\n \t}\n \n \t# Optimization: skip generating the body if client asks only\n-- \n1.7.3.4\n"},{"id":"187713","messageId":"20120326111203.GC2951@odin.tremily.us","threadId":"30001","inReplyTo":"20120326110943.GA2951@odin.tremily.us","subject":"[PATCH v4 2/3] gitweb: refactor If-Modified-Since handling","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T11:12:03Z","receivedAt":"2012-03-26T11:12:03Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The current gitweb only generates Last-Modified and handles\nIf-Modified-Since headers for the git_feed action.  This patch breaks\nthe Last-Modified and If-Modified-Since handling code out from\ngit_feed into a new function die_if_unmodified.  This makes the code\neasy to reuse for other actions.\n\nOnly gitweb actions which can easily calculate a modification time\nshould use die_if_unmodified, as the goal is to balance local\nprocessing time vs. upload bandwidth.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl                       |   40 +++++++++++++++++------------\n t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++-\n 2 files changed, 49 insertions(+), 18 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 041da17..229f3da 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7003,6 +7003,28 @@ sub snapshot_name {\n \treturn wantarray ? ($name, $name) : $name;\n }\n \n+sub die_if_unmodified {\n+\tmy ($latest_epoch) = @_;\n+\tour $cgi;\n+\n+\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n+\tif (defined $if_modified) {\n+\t\tmy $since;\n+\t\tif (eval { require HTTP::Date; 1; }) {\n+\t\t\t$since = HTTP::Date::str2time($if_modified);\n+\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n+\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n+\t\t}\n+\t\tif (defined $since && $latest_epoch <= $since) {\n+\t\t\tmy %latest_date = parse_date($latest_epoch);\n+\t\t\tprint $cgi->header(\n+\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t\t-status => '304 Not Modified');\n+\t\t\tgoto DONE_GITWEB;\n+\t\t}\n+\t}\n+}\n+\n sub git_snapshot {\n \tmy $format = $input_params{'snapshot_format'};\n \tif (!@snapshot_fmts) {\n@@ -7820,24 +7842,8 @@ sub git_feed {\n \tif (defined($commitlist[0])) {\n \t\t%latest_commit = %{$commitlist[0]};\n \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n+\t\tdie_if_unmodified($latest_epoch);\n \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n-\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n-\t\tif (defined $if_modified) {\n-\t\t\tmy $since;\n-\t\t\tif (eval { require HTTP::Date; 1; }) {\n-\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n-\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n-\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n-\t\t\t}\n-\t\t\tif (defined $since && $latest_epoch <= $since) {\n-\t\t\t\tprint $cgi->header(\n-\t\t\t\t\t-type => $content_type,\n-\t\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t\t\t-status => '304 Not Modified');\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n \t\t\t-charset => 'utf-8',\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 31076ed..0e49f29 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'\n test_expect_success 'snapshots: bad tree-ish id (tagged object)' '\n \techo object > tag-object &&\n \tgit add tag-object &&\n-\tgit commit -m \"Object to be tagged\" &&\n+\ttest_tick && git commit -m \"Object to be tagged\" &&\n \tgit tag tagged-object `git hash-object tag-object` &&\n \tgitweb_run \"p=.git;a=snapshot;h=tagged-object;sf=tgz\" &&\n \tgrep \"400 - Object is not a tree-ish\" gitweb.output\n@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '\n '\n test_debug 'cat gitweb.output'\n \n+# ----------------------------------------------------------------------\n+# modification times (Last-Modified and If-Modified-Since)\n+\n+test_expect_success 'modification: feed last-modified' '\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: feed if-modified-since (modified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: feed if-modified-since (unmodified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 304 Not Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n \n # ----------------------------------------------------------------------\n # load checking\n-- \n1.7.3.4\n"},{"id":"187717","messageId":"20120326111300.GD2951@odin.tremily.us","threadId":"30001","inReplyTo":"20120326110943.GA2951@odin.tremily.us","subject":"[PATCH v4 3/3] gitweb: add If-Modified-Since handling to git_snapshot().","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T11:13:00Z","receivedAt":"2012-03-26T11:13:00Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"Because snapshots can be large, you can save some bandwidth by\nsupporting caching via If-Modified-Since.  This patch adds support for\nthe i-m-s request to git_snapshot() if the requested hash is a commit.\nRequests for snapshots of tree-ishes, which lack well defined\ntimestamps, are still handled as they were before.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl                       |   21 +++++++++++++++---\n t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 229f3da..be9ad5d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7051,6 +7051,10 @@ sub git_snapshot {\n \n \tmy ($name, $prefix) = snapshot_name($project, $hash);\n \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n+\n+\tmy %co = parse_commit($hash);\n+\tdie_if_unmodified($co{'committer_epoch'}) if %co;\n+\n \tmy $cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n@@ -7060,10 +7064,19 @@ sub git_snapshot {\n \t}\n \n \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n-\tprint $cgi->header(\n-\t\t-type => $known_snapshot_formats{$format}{'type'},\n-\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n-\t\t-status => '200 OK');\n+\tif (%co) {\n+\t\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n+\t\tprint $cgi->header(\n+\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n+\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t-status => '200 OK');\n+\t} else {\n+\t\tprint $cgi->header(\n+\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n+\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t\t-status => '200 OK');\n+\t}\n \n \topen my $fd, \"-|\", $cmd\n \t\tor die_error(500, \"Execute git-archive failed\");\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 0e49f29..38e90bd 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '\n '\n test_debug 'cat gitweb.headers'\n \n+test_expect_success 'modification: snapshot last-modified' '\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: snapshot if-modified-since (modified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 304 Not Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: tree-ish snapshot' '\n+\tID=`git rev-parse --verify HEAD^{tree}` &&\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\t! grep \"Last-Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n # ----------------------------------------------------------------------\n # load checking\n \n-- \n1.7.3.4\n"},{"id":"187739","messageId":"7v8vinqoaj.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"20120326111203.GC2951@odin.tremily.us","subject":"Re: [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-26T17:12:36Z","receivedAt":"2012-03-26T17:12:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@drexel.edu> writes:\n\n> +sub die_if_unmodified {\n> +\tmy ($latest_epoch) = @_;\n> +\tour $cgi;\n> +\n> +\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> +\tif (defined $if_modified) {\n> +\t\tmy $since;\n> +\t\tif (eval { require HTTP::Date; 1; }) {\n> +\t\t\t$since = HTTP::Date::str2time($if_modified);\n> +\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> +\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> +\t\t}\n> +\t\tif (defined $since && $latest_epoch <= $since) {\n> +\t\t\tmy %latest_date = parse_date($latest_epoch);\n> +\t\t\tprint $cgi->header(\n> +\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> +\t\t\t\t-status => '304 Not Modified');\n> +\t\t\tgoto DONE_GITWEB;\n> +\t\t}\n> +\t}\n> +}\n\n>  sub git_snapshot {\n>  \tmy $format = $input_params{'snapshot_format'};\n>  \tif (!@snapshot_fmts) {\n> @@ -7820,24 +7842,8 @@ sub git_feed {\n>  \tif (defined($commitlist[0])) {\n>  \t\t%latest_commit = %{$commitlist[0]};\n>  \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n> +\t\tdie_if_unmodified($latest_epoch);\n>  \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n\nIt might make more sense to call the helper *after* stuffing %latest_date\nand pass the $latest_date{'rfc2822'} as parameter to the new helper\nfunction, so that the helper function do not have to call parse_date().\n\nThe verb \"die\" everywhere else in the codebase of Git not just means an\nimmediate exit, but means an imediate _error_ exit.  The new helper\nfunction is not about failure, but merely is an early return.  Perhaps\nrename it to exit_if_unmodified_since() or something?\n\n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index 31076ed..0e49f29 100755\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n> @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'\n>  test_expect_success 'snapshots: bad tree-ish id (tagged object)' '\n>  \techo object > tag-object &&\n>  \tgit add tag-object &&\n> -\tgit commit -m \"Object to be tagged\" &&\n> +\ttest_tick && git commit -m \"Object to be tagged\" &&\n>  \tgit tag tagged-object `git hash-object tag-object` &&\n>  \tgitweb_run \"p=.git;a=snapshot;h=tagged-object;sf=tgz\" &&\n>  \tgrep \"400 - Object is not a tree-ish\" gitweb.output\n> @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '\n>  '\n>  test_debug 'cat gitweb.output'\n>  \n> +# ----------------------------------------------------------------------\n> +# modification times (Last-Modified and If-Modified-Since)\n> +\n> +test_expect_success 'modification: feed last-modified' '\n> +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output\n\nMissing \" &&\" at the end (same in 3/3).\n\n> +\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: feed if-modified-since (modified)' '\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> +\tunset HTTP_IF_MODIFIED_SINCE &&\n> +\tgrep \"Status: 200 OK\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: feed if-modified-since (unmodified)' '\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n> +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> +\tunset HTTP_IF_MODIFIED_SINCE &&\n> +\tgrep \"Status: 304 Not Modified\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n\nOther than that, the patch looks cleanly done.\n\nThanks.\n"},{"id":"187747","messageId":"20120326173646.GA6524@odin.tremily.us","threadId":"30001","inReplyTo":"7v8vinqoaj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T17:36:46Z","receivedAt":"2012-03-26T17:36:46Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote:\n> It might make more sense to call the helper *after* stuffing %latest_date\n> and pass the $latest_date{'rfc2822'} as parameter to the new helper\n> function, so that the helper function do not have to call parse_date().\n\nI did something closer to your suggestion in my v1 patch, but\n\nOn Tue, Mar 20, 2012 at 04:55:45AM -0700, Jakub Narebski wrote:\n> Shouldn't we pass \\%latest_date (or rather \\%modification_date to be\n> more generic)?  Ah, I see that parse_date() subroutine does not store\n> original epoch not original timezone.\n> \n> By the way, for $latest_date{'rfc2822'} we don't need $timezone\n> argument, and I think we should not pass it to generic\n> modified_since() function (or whatever it will be called).\n> \n> …\n> \n> I think a better idea would be to make this function/subroutine (named\n> e.g. if_modified_since() or something like that) to work more like\n> die_error() -- it should simply end request instead of having caller\n> to use complcated calling convention, and care about eraly termination\n> by itself.\n\nOnly passing in the epoch.  We could reduce computation at the expense\nof complication by passing in both the epoch and a formatted time\nstring, but after Jakub's suggestions, I felt like a simpler interface\nwas the better approach.  I don't feel particularly committed to\neither way, so just tell me which you'd like best ;).\n\n> The verb \"die\" everywhere else in the codebase of Git not just means an\n> immediate exit, but means an imediate _error_ exit.  The new helper\n> function is not about failure, but merely is an early return.  Perhaps\n> rename it to exit_if_unmodified_since() or something?\n\nWill fix in v5.\n\n> Missing \" &&\" at the end (same in 3/3).\n\nOops.  I wonder why my tests still passed :p.  Will fix in v5.\n\n> Other than that, the patch looks cleanly done.\n> \n> Thanks.\n\nYou're welcome.  Thanks for shepherding me through it ;).\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187762","messageId":"7vsjgvnr4x.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"20120326173646.GA6524@odin.tremily.us","subject":"Re: [PATCH v4 2/3] gitweb: refactor If-Modified-Since handling","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-26T18:39:26Z","receivedAt":"2012-03-26T18:39:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@drexel.edu> writes:\n\n> On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote:\n> ...\n> Only passing in the epoch.  We could reduce computation at the expense\n> of complication by passing in both the epoch and a formatted time\n> string, but after Jakub's suggestions, I felt like a simpler interface\n> was the better approach.  I don't feel particularly committed to\n> either way, so just tell me which you'd like best ;).\n\nThe existing codepath already had the call to parse_date() nearby and that\nwas the only reason I made the suggestion.  The caller in the snapshot\ncodepath would need to call parse_date() much earlier and it would have to\nbe conditional to the availability of %co, so the simpler interface would\nprobably be overall win.\n\n>> Missing \" &&\" at the end (same in 3/3).\n>\n> Oops.  I wonder why my tests still passed :p.  Will fix in v5.\n\nThe test will pass if there is no breakage.  The offence of missing \" &&\"\nafter your \"grep 200\" is that it will let the test pass even if you did\nnot have \"200\" in the output, failing to catch future breakage.\n"},{"id":"187767","messageId":"20120326191242.GA9041@odin.tremily.us","threadId":"30001","inReplyTo":"7vsjgvnr4x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T19:12:42Z","receivedAt":"2012-03-26T19:12:42Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The current gitweb only generates Last-Modified and handles\nIf-Modified-Since headers for the git_feed action.  This patch breaks\nthe Last-Modified and If-Modified-Since handling code out from\ngit_feed into a new function exit_if_unmodified_since.  This makes the\ncode easy to reuse for other actions.\n\nOnly gitweb actions which can easily calculate a modification time\nshould use exit_if_unmodified_since, as the goal is to balance local\nprocessing time vs. upload bandwidth.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\nPatch v4 1/3 is unchanged.  Should I mail it back in with a [PATCH v5\n1/3] tag?\n\nChanges since v4:\n* die_if_unmodified() -> exit_if_unmodified_since()\n* Added missing `&&` to tests for feed-last-modified (patch 2/3) and\n  snapshot-last-modified (patch 3/3).\n\n gitweb/gitweb.perl                       |   40 +++++++++++++++++------------\n t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++-\n 2 files changed, 49 insertions(+), 18 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 041da17..229f3da 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7003,6 +7003,28 @@ sub snapshot_name {\n \treturn wantarray ? ($name, $name) : $name;\n }\n \n+sub exit_if_unmodified_since {\n+\tmy ($latest_epoch) = @_;\n+\tour $cgi;\n+\n+\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n+\tif (defined $if_modified) {\n+\t\tmy $since;\n+\t\tif (eval { require HTTP::Date; 1; }) {\n+\t\t\t$since = HTTP::Date::str2time($if_modified);\n+\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n+\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n+\t\t}\n+\t\tif (defined $since && $latest_epoch <= $since) {\n+\t\t\tmy %latest_date = parse_date($latest_epoch);\n+\t\t\tprint $cgi->header(\n+\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t\t-status => '304 Not Modified');\n+\t\t\tgoto DONE_GITWEB;\n+\t\t}\n+\t}\n+}\n+\n sub git_snapshot {\n \tmy $format = $input_params{'snapshot_format'};\n \tif (!@snapshot_fmts) {\n@@ -7820,24 +7842,8 @@ sub git_feed {\n \tif (defined($commitlist[0])) {\n \t\t%latest_commit = %{$commitlist[0]};\n \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n+\t\texit_if_unmodified_since($latest_epoch);\n \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n-\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n-\t\tif (defined $if_modified) {\n-\t\t\tmy $since;\n-\t\t\tif (eval { require HTTP::Date; 1; }) {\n-\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n-\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n-\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n-\t\t\t}\n-\t\t\tif (defined $since && $latest_epoch <= $since) {\n-\t\t\t\tprint $cgi->header(\n-\t\t\t\t\t-type => $content_type,\n-\t\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t\t\t-status => '304 Not Modified');\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n \t\t\t-charset => 'utf-8',\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 31076ed..0e49f29 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'\n test_expect_success 'snapshots: bad tree-ish id (tagged object)' '\n \techo object > tag-object &&\n \tgit add tag-object &&\n-\tgit commit -m \"Object to be tagged\" &&\n+\ttest_tick && git commit -m \"Object to be tagged\" &&\n \tgit tag tagged-object `git hash-object tag-object` &&\n \tgitweb_run \"p=.git;a=snapshot;h=tagged-object;sf=tgz\" &&\n \tgrep \"400 - Object is not a tree-ish\" gitweb.output\n@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '\n '\n test_debug 'cat gitweb.output'\n \n+# ----------------------------------------------------------------------\n+# modification times (Last-Modified and If-Modified-Since)\n+\n+test_expect_success 'modification: feed last-modified' '\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: feed if-modified-since (modified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: feed if-modified-since (unmodified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 304 Not Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n \n # ----------------------------------------------------------------------\n # load checking\n-- \n1.7.3.4\n"},{"id":"187768","messageId":"20120326191418.GB9041@odin.tremily.us","threadId":"30001","inReplyTo":"7vsjgvnr4x.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot().","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-26T19:14:19Z","receivedAt":"2012-03-26T19:14:19Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"Because snapshots can be large, you can save some bandwidth by\nsupporting caching via If-Modified-Since.  This patch adds support for\nthe i-m-s request to git_snapshot() if the requested hash is a commit.\nRequests for snapshots of tree-ishes, which lack well defined\ntimestamps, are still handled as they were before.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl                       |   21 +++++++++++++++---\n t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++\n 2 files changed, 50 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 229f3da..be9ad5d 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7051,6 +7051,10 @@ sub git_snapshot {\n \n \tmy ($name, $prefix) = snapshot_name($project, $hash);\n \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n+\n+\tmy %co = parse_commit($hash);\n+\texit_if_unmodified_since($co{'committer_epoch'}) if %co;\n+\n \tmy $cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n@@ -7060,10 +7064,19 @@ sub git_snapshot {\n \t}\n \n \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n-\tprint $cgi->header(\n-\t\t-type => $known_snapshot_formats{$format}{'type'},\n-\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n-\t\t-status => '200 OK');\n+\tif (%co) {\n+\t\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n+\t\tprint $cgi->header(\n+\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n+\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t-status => '200 OK');\n+\t} else {\n+\t\tprint $cgi->header(\n+\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n+\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t\t-status => '200 OK');\n+\t}\n \n \topen my $fd, \"-|\", $cmd\n \t\tor die_error(500, \"Execute git-archive failed\");\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 0e49f29..38e90bd 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '\n '\n test_debug 'cat gitweb.headers'\n \n+test_expect_success 'modification: snapshot last-modified' '\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: snapshot if-modified-since (modified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 304 Not Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: tree-ish snapshot' '\n+\tID=`git rev-parse --verify HEAD^{tree}` &&\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tunset HTTP_IF_MODIFIED_SINCE &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\t! grep \"Last-Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n # ----------------------------------------------------------------------\n # load checking\n \n-- \n1.7.3.4\n"},{"id":"187886","messageId":"201203272124.23145.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120326110943.GA2951@odin.tremily.us","subject":"Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-27T19:24:22Z","receivedAt":"2012-03-27T19:24:22Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 26 Mar 2012, W. Trevor King wrote:\n\n> Sorry for the delay since my last message, it's been a busy week ;).\n\nThanks for working on this topic.  \n\nBTW. we are in feature freeze currently, before release of v1.7.10.\n \n> I'll be sending along three patches.  [...]\n> [...] The first is a teensy patch to add `Status` \n> output to non-304 calls to git_feed().  Without it you'd have to get a\n> bit more creative in the test suite.  If the status lines were left\n> out intentionally, let me know, and I'll come up with another\n> condition for those tests.\n\nNo, the status line was omitted unintentionally, thanks for fixing this.\nI guess that web server (at least Apache) adds it if it is missing.\n\nThough I am not sure how lack of \"Status:\" header is different from\nincorrect \"Status:\" header for 'grep \"Status: 304\" gitweb.headers'...\n \n> The second patch refactors and tests git_feed(), and the third patch\n> adds i-m-s support to git_snapshot() with associated tests.\n> \n> Changes since v3:\n> * Patch 1/3 is completely new.\n> * Split previous patch into 2/3 and 3/3.\n> * Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}.\n> * Added testing to both 2/3 and 3/3.\n> * Reworked commit messages.\n\nGood.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187887","messageId":"20120327194933.GA32529@odin.tremily.us","threadId":"30001","inReplyTo":"201203272124.23145.jnareb@gmail.com","subject":"Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-27T19:49:33Z","receivedAt":"2012-03-27T19:49:33Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:\n> BTW. we are in feature freeze currently, before release of v1.7.10.\n\nI'm not in a big hurry or anything, just trying to save some\nbandwidth…\n\n> Though I am not sure how lack of \"Status:\" header is different from\n> incorrect \"Status:\" header for 'grep \"Status: 304\" gitweb.headers'...\n\nIt's not for the 304 tests.  I thought it would be a good idea to grep\nfor a 200 status before looking for Last-Modified times, to ensure you\nactually get to the successful response stage.\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187888","messageId":"20120327195558.GA32699@odin.tremily.us","threadId":"30001","inReplyTo":"201203272124.23145.jnareb@gmail.com","subject":"Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-27T19:55:58Z","receivedAt":"2012-03-27T19:55:58Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:\n> No, the status line was omitted unintentionally, thanks for fixing this.\n> I guess that web server (at least Apache) adds it if it is missing.\n\nFrom the CGI 1.1 specs [1], section 9.2:\n\n  If the script does not return a Status header, then \"200 OK\" should\n  be assumed.\n\nSo, perhaps it is better to skip the Status header on success (less\nfor the server to parse).\n\n[1]: http://tools.ietf.org/html/draft-robinson-www-interface-00\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187889","messageId":"201203272157.31870.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120327194933.GA32529@odin.tremily.us","subject":"Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-27T19:57:31Z","receivedAt":"2012-03-27T19:57:31Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W. Trevor King wrote:\n> On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:\n\n> > Though I am not sure how lack of \"Status:\" header is different from\n> > incorrect \"Status:\" header for 'grep \"Status: 304\" gitweb.headers'...\n> \n> It's not for the 304 tests.  I thought it would be a good idea to grep\n> for a 200 status before looking for Last-Modified times, to ensure you\n> actually get to the successful response stage.\n\nRight.\n\nTesting both sides of a new feature (i.e. modified and unmodified here)\nis a good testing practice.  Thanks.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187890","messageId":"7vzkb1hk5r.fsf@alter.siamese.dyndns.org","threadId":"30001","inReplyTo":"20120327195558.GA32699@odin.tremily.us","subject":"Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-27T20:18:56Z","receivedAt":"2012-03-27T20:18:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"W. Trevor King\" <wking@drexel.edu> writes:\n\n> On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:\n>> No, the status line was omitted unintentionally, thanks for fixing this.\n>> I guess that web server (at least Apache) adds it if it is missing.\n>\n> From the CGI 1.1 specs [1], section 9.2:\n>\n>   If the script does not return a Status header, then \"200 OK\" should\n>   be assumed.\n>\n> So, perhaps it is better to skip the Status header on success (less\n> for the server to parse).\n\nI do not think it matters either way.  It is just a single header line, no?\n\nThe code is certainly more explicit with your patch.\n"},{"id":"187897","messageId":"201203280024.20805.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120326191242.GA9041@odin.tremily.us","subject":"Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-27T22:24:20Z","receivedAt":"2012-03-27T22:24:20Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 26 Mar 2012, W. Trevor King wrote:\n\n> The current gitweb only generates Last-Modified and handles\n> If-Modified-Since headers for the git_feed action.  This patch breaks\n> the Last-Modified and If-Modified-Since handling code out from\n> git_feed into a new function exit_if_unmodified_since.  This makes the\n> code easy to reuse for other actions.\n\nNice description, and I think quite good name for a subroutine.  Well,\ngood enough; any more we would drown in bikeshed-ding.\n> \n> Only gitweb actions which can easily calculate a modification time\n> should use exit_if_unmodified_since, as the goal is to balance local\n> processing time vs. upload bandwidth.\n\nGood.\n>\n> Signed-off-by: W Trevor King <wking@drexel.edu>\n> ---\n> Patch v4 1/3 is unchanged.  Should I mail it back in with a\n> [PATCH v5 1/3] tag?\n> \n> Changes since v4:\n> * die_if_unmodified() -> exit_if_unmodified_since()\n> * Added missing `&&` to tests for feed-last-modified (patch 2/3) and\n>   snapshot-last-modified (patch 3/3).\n> \n>  gitweb/gitweb.perl                       |   40 +++++++++++++++++------------\n>  t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++-\n>  2 files changed, 49 insertions(+), 18 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 041da17..229f3da 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -7003,6 +7003,28 @@ sub snapshot_name {\n>  \treturn wantarray ? ($name, $name) : $name;\n>  }\n>  \n> +sub exit_if_unmodified_since {\n> +\tmy ($latest_epoch) = @_;\n\nWe could have made \\%latest_date an optional parameter...\n\n  +\tmy ($latest_epoch, $latest_date) = @_;\n\n> +\tour $cgi;\n> +\n> +\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> +\tif (defined $if_modified) {\n> +\t\tmy $since;\n> +\t\tif (eval { require HTTP::Date; 1; }) {\n> +\t\t\t$since = HTTP::Date::str2time($if_modified);\n> +\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> +\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> +\t\t}\n> +\t\tif (defined $since && $latest_epoch <= $since) {\n> +\t\t\tmy %latest_date = parse_date($latest_epoch);\n\n...and use it if provided\n\n  +\t\t\tmy %latest_date = ref($latest_date) eq 'HASH' \n  +\t\t\t\t? %$latest_date : parse_date($latest_epoch);\n\nbut something is to be said for simplicity.  We need epoch for comparison\nanyway.\n\n> +\t\t\tprint $cgi->header(\n> +\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> +\t\t\t\t-status => '304 Not Modified');\n\nO.K. we lose some unnecessary headers; never mind that.  What is\nimportant is here.\n\n> +\t\t\tgoto DONE_GITWEB;\n> +\t\t}\n> +\t}\n> +}\n> +\n>  sub git_snapshot {\n>  \tmy $format = $input_params{'snapshot_format'};\n>  \tif (!@snapshot_fmts) {\n> @@ -7820,24 +7842,8 @@ sub git_feed {\n>  \tif (defined($commitlist[0])) {\n>  \t\t%latest_commit = %{$commitlist[0]};\n>  \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n> +\t\texit_if_unmodified_since($latest_epoch);\n>  \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n> -\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> -\t\tif (defined $if_modified) {\n> -\t\t\tmy $since;\n> -\t\t\tif (eval { require HTTP::Date; 1; }) {\n> -\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n> -\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> -\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> -\t\t\t}\n> -\t\t\tif (defined $since && $latest_epoch <= $since) {\n> -\t\t\t\tprint $cgi->header(\n> -\t\t\t\t\t-type => $content_type,\n> -\t\t\t\t\t-charset => 'utf-8',\n> -\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> -\t\t\t\t\t-status => '304 Not Modified');\n> -\t\t\t\treturn;\n> -\t\t\t}\n> -\t\t}\n>  \t\tprint $cgi->header(\n>  \t\t\t-type => $content_type,\n>  \t\t\t-charset => 'utf-8',\n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index 31076ed..0e49f29 100755\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n> @@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'\n>  test_expect_success 'snapshots: bad tree-ish id (tagged object)' '\n>  \techo object > tag-object &&\n>  \tgit add tag-object &&\n> -\tgit commit -m \"Object to be tagged\" &&\n> +\ttest_tick && git commit -m \"Object to be tagged\" &&\n>  \tgit tag tagged-object `git hash-object tag-object` &&\n>  \tgitweb_run \"p=.git;a=snapshot;h=tagged-object;sf=tgz\" &&\n>  \tgrep \"400 - Object is not a tree-ish\" gitweb.output\n> @@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '\n>  '\n>  test_debug 'cat gitweb.output'\n>  \n> +# ----------------------------------------------------------------------\n> +# modification times (Last-Modified and If-Modified-Since)\n> +\n> +test_expect_success 'modification: feed last-modified' '\n> +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output &&\n> +\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n> +'\n\nAll right.\n\nWhat's that date from?  Wouldn't it be better to read it from commit\nobject with `git show -s --pretty=%cD HEAD` or postprocessed from\n'%ct' timestamp?\n\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: feed if-modified-since (modified)' '\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> +\tunset HTTP_IF_MODIFIED_SINCE &&\n> +\tgrep \"Status: 200 OK\" gitweb.output\n> +'\n\nI think it *might* be better solution to use test_when_finished:\n\n  +test_expect_success 'modification: feed if-modified-since (modified)' '\n  +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n  +\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n  +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n  +\tgrep \"Status: 200 OK\" gitweb.output\n  +'\n\nI don't think we need sane_unset here.\n\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: feed if-modified-since (unmodified)' '\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n> +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> +\tunset HTTP_IF_MODIFIED_SINCE &&\n> +\tgrep \"Status: 304 Not Modified\" gitweb.output\n> +'\n\nSame here.\n\n> +test_debug 'cat gitweb.headers'\n>  \n>  # ----------------------------------------------------------------------\n>  # load checking\n> -- \n> 1.7.3.4\n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"187898","messageId":"201203280031.38011.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120326191418.GB9041@odin.tremily.us","subject":"Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot().","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-27T22:31:37Z","receivedAt":"2012-03-27T22:31:37Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 26 Mar 2012, W. Trevor King wrote:\n\n> Because snapshots can be large, you can save some bandwidth by\n> supporting caching via If-Modified-Since.  This patch adds support for\n> the i-m-s request to git_snapshot() if the requested hash is a commit.\n\n\"if the requested hash is a commit\" means here \"if we request snapshot\nof a commit\", isn't it?\n\n> Requests for snapshots of tree-ishes, which lack well defined\n> timestamps, are still handled as they were before.\n\ns/tree-ishes/trees/.  Commit is tree-ish but not a tree; it is tree\nthat lacks timestamp that commit has.\n\n> \n> Signed-off-by: W Trevor King <wking@drexel.edu>\n> ---\n>  gitweb/gitweb.perl                       |   21 +++++++++++++++---\n>  t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++\n>  2 files changed, 50 insertions(+), 4 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 229f3da..be9ad5d 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -7051,6 +7051,10 @@ sub git_snapshot {\n>  \n>  \tmy ($name, $prefix) = snapshot_name($project, $hash);\n>  \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> +\n> +\tmy %co = parse_commit($hash);\n> +\texit_if_unmodified_since($co{'committer_epoch'}) if %co;\n> +\n\nO.K.  Nice.\n\n>  \tmy $cmd = quote_command(\n>  \t\tgit_cmd(), 'archive',\n>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n> @@ -7060,10 +7064,19 @@ sub git_snapshot {\n>  \t}\n>  \n>  \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n> -\tprint $cgi->header(\n> -\t\t-type => $known_snapshot_formats{$format}{'type'},\n> -\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> -\t\t-status => '200 OK');\n> +\tif (%co) {\n> +\t\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n> +\t\tprint $cgi->header(\n> +\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n> +\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> +\t\t\t-last_modified => $latest_date{'rfc2822'},\n> +\t\t\t-status => '200 OK');\n> +\t} else {\n> +\t\tprint $cgi->header(\n> +\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n> +\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> +\t\t\t-status => '200 OK');\n> +\t}\n\nThis can be written shorter, and with less code repetition using ?: \nconditional operator:\n\n> +\tprint $cgi->header(\n> +\t\t-type => $known_snapshot_formats{$format}{'type'},\n> +\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> +\t\t (%co ? -last_modified => $latest_date{'rfc2822'} : ()),\n> +\t\t-status => '200 OK');\n\n...or something like that.\n\n>  \n>  \topen my $fd, \"-|\", $cmd\n>  \t\tor die_error(500, \"Execute git-archive failed\");\n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index 0e49f29..38e90bd 100755\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n\nSame comments as for 2/3.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187928","messageId":"20120328135146.GA3525@odin.tremily.us","threadId":"30001","inReplyTo":"201203280024.20805.jnareb@gmail.com","subject":"Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-28T13:51:46Z","receivedAt":"2012-03-28T13:51:46Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote:\n> On Mon, 26 Mar 2012, W. Trevor King wrote:\n> > +# ----------------------------------------------------------------------\n> > +# modification times (Last-Modified and If-Modified-Since)\n> > +\n> > +test_expect_success 'modification: feed last-modified' '\n> > +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> > +\tgrep \"Status: 200 OK\" gitweb.output &&\n> > +\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n> > +'\n> \n> All right.\n> \n> What's that date from?  Wouldn't it be better to read it from commit\n> object with `git show -s --pretty=%cD HEAD` or postprocessed from\n> '%ct' timestamp?\n\nThat's the date set by the first `test_tick`, which is hardcoded in\n`test-lib-functions.sh`.  Extracting the date dynamically seems\nunnecessary, since I can't imagine anyone changing the `test_tick`\ndate.  It's easy enough to do if you think it is appropriate though…\n\n> > +test_debug 'cat gitweb.headers'\n> > +\n> > +test_expect_success 'modification: feed if-modified-since (modified)' '\n> > +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> > +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> > +\tunset HTTP_IF_MODIFIED_SINCE &&\n> > +\tgrep \"Status: 200 OK\" gitweb.output\n> > +'\n> \n> I think it *might* be better solution to use test_when_finished:\n> \n>   +test_expect_success 'modification: feed if-modified-since (modified)' '\n>   +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n>   +\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n>   +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n>   +\tgrep \"Status: 200 OK\" gitweb.output\n>   +'\n> \n> I don't think we need sane_unset here.\n\nGood point.  Sloppy me not reading `t/README` thoroughly enough ;).\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187932","messageId":"20120328135847.GB3525@odin.tremily.us","threadId":"30001","inReplyTo":"201203280031.38011.jnareb@gmail.com","subject":"Re: [PATCH v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot().","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-28T13:58:47Z","receivedAt":"2012-03-28T13:58:47Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Tue, Mar 27, 2012 at 11:31:37PM +0100, Jakub Narebski wrote:\n> On Mon, 26 Mar 2012, W. Trevor King wrote:\n> > Because snapshots can be large, you can save some bandwidth by\n> > supporting caching via If-Modified-Since.  This patch adds support for\n> > the i-m-s request to git_snapshot() if the requested hash is a commit.\n> \n> \"if the requested hash is a commit\" means here \"if we request snapshot\n> of a commit\", isn't it?\n> \n> > Requests for snapshots of tree-ishes, which lack well defined\n> > timestamps, are still handled as they were before.\n> \n> s/tree-ishes/trees/.  Commit is tree-ish but not a tree; it is tree\n> that lacks timestamp that commit has.\n\nRight.  I'll fix those in v6 ;).\n\n> > +\tif (%co) {\n> > +\t\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n> > +\t\tprint $cgi->header(\n> > +\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n> > +\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> > +\t\t\t-last_modified => $latest_date{'rfc2822'},\n> > +\t\t\t-status => '200 OK');\n> > +\t} else {\n> > +\t\tprint $cgi->header(\n> > +\t\t\t-type => $known_snapshot_formats{$format}{'type'},\n> > +\t\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> > +\t\t\t-status => '200 OK');\n> > +\t}\n> \n> This can be written shorter, and with less code repetition using ?: \n> conditional operator:\n> \n> > +\tprint $cgi->header(\n> > +\t\t-type => $known_snapshot_formats{$format}{'type'},\n> > +\t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> > +\t\t (%co ? -last_modified => $latest_date{'rfc2822'} : ()),\n> > +\t\t-status => '200 OK');\n> \n> ...or something like that.\n\nAh, that's much better.  I'm not particulary familar with Perl, sorry ;).\n\nI'll wait until I here back on Last-Modified timestamp generation, and\nthen submit v6.\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187930","messageId":"201203281613.52624.jnareb@gmail.com","threadId":"30001","inReplyTo":"20120328135146.GA3525@odin.tremily.us","subject":"Re: [PATCH v5 2/3] gitweb: refactor If-Modified-Since handling","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-28T14:13:52Z","receivedAt":"2012-03-28T14:13:52Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 28 Mar 2012, W. Trevor King wrote:\n> On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote:\n> > On Mon, 26 Mar 2012, W. Trevor King wrote:\n> > > +# ----------------------------------------------------------------------\n> > > +# modification times (Last-Modified and If-Modified-Since)\n> > > +\n> > > +test_expect_success 'modification: feed last-modified' '\n> > > +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> > > +\tgrep \"Status: 200 OK\" gitweb.output &&\n> > > +\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n> > > +'\n> > \n> > All right.\n> > \n> > What's that date from?  Wouldn't it be better to read it from commit\n> > object with `git show -s --pretty=%cD HEAD` or postprocessed from\n> > '%ct' timestamp?\n> \n> That's the date set by the first `test_tick`, which is hardcoded in\n> `test-lib-functions.sh`.  Extracting the date dynamically seems\n> unnecessary, since I can't imagine anyone changing the `test_tick`\n> date.  \n\nAh, it's all right then.  I should have checked the test_tick function.\n\nThat of course assuming that nobody would add test_tick earlier, but\nif he/she does, he/she can deal with fallout...\n\n> It's easy enough to do if you think it is appropriate though… \n\nNo, it is not needed.\n\n> > > +test_debug 'cat gitweb.headers'\n> > > +\n> > > +test_expect_success 'modification: feed if-modified-since (modified)' '\n> > > +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> > > +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> > > +\tunset HTTP_IF_MODIFIED_SINCE &&\n> > > +\tgrep \"Status: 200 OK\" gitweb.output\n> > > +'\n> > \n> > I think it *might* be better solution to use test_when_finished:\n> > \n> >   +test_expect_success 'modification: feed if-modified-since (modified)' '\n> >   +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> >   +\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n> >   +\tgitweb_run \"p=.git;a=atom;h=master\" &&\n> >   +\tgrep \"Status: 200 OK\" gitweb.output\n> >   +'\n> > \n> > I don't think we need sane_unset here.\n> \n> Good point.  Sloppy me not reading `t/README` thoroughly enough ;).\n\n-- \nJakub Narebski\nPoland\n"},{"id":"187941","messageId":"cover.1332949186.git.wking@drexel.edu","threadId":"30001","inReplyTo":"201203281613.52624.jnareb@gmail.com","subject":"[PATCH v6 0/3] gitweb: refactor If-Modified-Since handling","fromName":"","fromEmail":"wking@tremily.us","sentAt":"2012-03-28T15:46:58Z","receivedAt":"2012-03-28T15:46:58Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"Changes since v5:\n* Use `test_when_finished` to unset HTTP_IF_MODIFIED_SINCE.\n* Actually use the tree ID in the `modification: tree snapshot` test.\n* Conditional last_modified argument for `$cgi->header` git_snapshot\n  and git_feed.\n* tree-ish -> trees in git_snapshot commit message and test name.\n\nUnchanged since v5:\n* Patch 1/3, but I'm resending it anyway with `git send-email`.  I\n  could skip resending it with:\n\n    git send-email … HEAD^^\n\n  but that would mess up the patch numbering.\n\n\nW. Trevor King (3):\n  gitweb: add `status` headers to git_feed() responses.\n  gitweb: refactor If-Modified-Since handling\n  gitweb: add If-Modified-Since handling to git_snapshot().\n\n gitweb/gitweb.perl                       |   65 ++++++++++++++++++------------\n t/t9501-gitweb-standalone-http-status.sh |   60 +++++++++++++++++++++++++++-\n 2 files changed, 98 insertions(+), 27 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"187943","messageId":"4958e06536924de10105bb453d88e8f4c28bb8c1.1332949186.git.wking@drexel.edu","threadId":"30001","inReplyTo":"201203281613.52624.jnareb@gmail.com","subject":"[PATCH v6 1/3] gitweb: add `status` headers to git_feed() responses.","fromName":"","fromEmail":"wking@tremily.us","sentAt":"2012-03-28T15:46:59Z","receivedAt":"2012-03-28T15:46:59Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The git_feed() method was not setting a `Status` header unless it was\nresponding to an If-Modified-Since request with `304 Not Modified`.\nNow, when it is serving successful responses, it sets status to `200\nOK`.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl |    6 ++++--\n 1 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..041da17 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7841,11 +7841,13 @@ sub git_feed {\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n \t\t\t-charset => 'utf-8',\n-\t\t\t-last_modified => $latest_date{'rfc2822'});\n+\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t-status => '200 OK');\n \t} else {\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n-\t\t\t-charset => 'utf-8');\n+\t\t\t-charset => 'utf-8',\n+\t\t\t-status => '200 OK');\n \t}\n \n \t# Optimization: skip generating the body if client asks only\n-- \n1.7.3.4\n"},{"id":"187942","messageId":"fc2823f5cbf90c9b8e5d026eddcd7c8dbda58cf4.1332949186.git.wking@drexel.edu","threadId":"30001","inReplyTo":"201203281613.52624.jnareb@gmail.com","subject":"[PATCH v6 2/3] gitweb: refactor If-Modified-Since handling","fromName":"","fromEmail":"wking@tremily.us","sentAt":"2012-03-28T15:47:00Z","receivedAt":"2012-03-28T15:47:00Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The current gitweb only generates Last-Modified and handles\nIf-Modified-Since headers for the git_feed action.  This patch breaks\nthe Last-Modified and If-Modified-Since handling code out from\ngit_feed into a new function exit_if_unmodified_since.  This makes the\ncode easy to reuse for other actions.\n\nOnly gitweb actions which can easily calculate a modification time\nshould use exit_if_unmodified_since, as the goal is to balance local\nprocessing time vs. upload bandwidth.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl                       |   57 +++++++++++++++--------------\n t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++-\n 2 files changed, 55 insertions(+), 29 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 041da17..ba106c1 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7003,6 +7003,28 @@ sub snapshot_name {\n \treturn wantarray ? ($name, $name) : $name;\n }\n \n+sub exit_if_unmodified_since {\n+\tmy ($latest_epoch) = @_;\n+\tour $cgi;\n+\n+\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n+\tif (defined $if_modified) {\n+\t\tmy $since;\n+\t\tif (eval { require HTTP::Date; 1; }) {\n+\t\t\t$since = HTTP::Date::str2time($if_modified);\n+\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n+\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n+\t\t}\n+\t\tif (defined $since && $latest_epoch <= $since) {\n+\t\t\tmy %latest_date = parse_date($latest_epoch);\n+\t\t\tprint $cgi->header(\n+\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t\t-status => '304 Not Modified');\n+\t\t\tgoto DONE_GITWEB;\n+\t\t}\n+\t}\n+}\n+\n sub git_snapshot {\n \tmy $format = $input_params{'snapshot_format'};\n \tif (!@snapshot_fmts) {\n@@ -7820,35 +7842,14 @@ sub git_feed {\n \tif (defined($commitlist[0])) {\n \t\t%latest_commit = %{$commitlist[0]};\n \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n-\t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n-\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n-\t\tif (defined $if_modified) {\n-\t\t\tmy $since;\n-\t\t\tif (eval { require HTTP::Date; 1; }) {\n-\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n-\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n-\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n-\t\t\t}\n-\t\t\tif (defined $since && $latest_epoch <= $since) {\n-\t\t\t\tprint $cgi->header(\n-\t\t\t\t\t-type => $content_type,\n-\t\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t\t\t-status => '304 Not Modified');\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n-\t\tprint $cgi->header(\n-\t\t\t-type => $content_type,\n-\t\t\t-charset => 'utf-8',\n-\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t-status => '200 OK');\n-\t} else {\n-\t\tprint $cgi->header(\n-\t\t\t-type => $content_type,\n-\t\t\t-charset => 'utf-8',\n-\t\t\t-status => '200 OK');\n+\t\texit_if_unmodified_since($latest_epoch);\n+\t\t%latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n \t}\n+\tprint $cgi->header(\n+\t\t-type => $content_type,\n+\t\t-charset => 'utf-8',\n+\t\t-last_modified => (%latest_date ? $latest_date{'rfc2822'} : undef),\n+\t\t-status => '200 OK');\n \n \t# Optimization: skip generating the body if client asks only\n \t# for Last-Modified date.\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex 31076ed..afa6bd4 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'\n test_expect_success 'snapshots: bad tree-ish id (tagged object)' '\n \techo object > tag-object &&\n \tgit add tag-object &&\n-\tgit commit -m \"Object to be tagged\" &&\n+\ttest_tick && git commit -m \"Object to be tagged\" &&\n \tgit tag tagged-object `git hash-object tag-object` &&\n \tgitweb_run \"p=.git;a=snapshot;h=tagged-object;sf=tgz\" &&\n \tgrep \"400 - Object is not a tree-ish\" gitweb.output\n@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '\n '\n test_debug 'cat gitweb.output'\n \n+# ----------------------------------------------------------------------\n+# modification times (Last-Modified and If-Modified-Since)\n+\n+test_expect_success 'modification: feed last-modified' '\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: feed if-modified-since (modified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: feed if-modified-since (unmodified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n+\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n+\tgitweb_run \"p=.git;a=atom;h=master\" &&\n+\tgrep \"Status: 304 Not Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n \n # ----------------------------------------------------------------------\n # load checking\n-- \n1.7.3.4\n"},{"id":"187944","messageId":"8129a671c793613ad2a0b00e1239e6a7ded7491a.1332949186.git.wking@drexel.edu","threadId":"30001","inReplyTo":"201203281613.52624.jnareb@gmail.com","subject":"[PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot().","fromName":"","fromEmail":"wking@tremily.us","sentAt":"2012-03-28T15:47:01Z","receivedAt":"2012-03-28T15:47:01Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"Because snapshots can be large, you can save some bandwidth by\nsupporting caching via If-Modified-Since.  This patch adds support for\nthe i-m-s request to git_snapshot() if the request is a commit.\nRequests for snapshots of trees, which lack well defined timestamps,\nare still handled as they were before.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n---\n gitweb/gitweb.perl                       |   10 +++++++++\n t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 0 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex ba106c1..591c793 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7051,6 +7051,10 @@ sub git_snapshot {\n \n \tmy ($name, $prefix) = snapshot_name($project, $hash);\n \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n+\n+\tmy %co = parse_commit($hash);\n+\texit_if_unmodified_since($co{'committer_epoch'}) if %co;\n+\n \tmy $cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n@@ -7060,9 +7064,15 @@ sub git_snapshot {\n \t}\n \n \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n+\tmy %latest_date;\n+\tif (%co) {\n+\t\t%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n+\t}\n+\n \tprint $cgi->header(\n \t\t-type => $known_snapshot_formats{$format}{'type'},\n \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t-last_modified => (%co ? $latest_date{'rfc2822'} : undef),\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\ndiff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\nindex afa6bd4..1487820 100755\n--- a/t/t9501-gitweb-standalone-http-status.sh\n+++ b/t/t9501-gitweb-standalone-http-status.sh\n@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '\n '\n test_debug 'cat gitweb.headers'\n \n+test_expect_success 'modification: snapshot last-modified' '\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: snapshot if-modified-since (modified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '\n+\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n+\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n+\tgrep \"Status: 304 Not Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n+test_expect_success 'modification: tree snapshot' '\n+\tID=`git rev-parse --verify HEAD^{tree}` &&\n+\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n+\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n+\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n+\tgrep \"Status: 200 OK\" gitweb.output &&\n+\t! grep \"Last-Modified\" gitweb.output\n+'\n+test_debug 'cat gitweb.headers'\n+\n # ----------------------------------------------------------------------\n # load checking\n \n-- \n1.7.3.4\n"},{"id":"187946","messageId":"20120328155624.GA4135@odin.tremily.us","threadId":"30001","inReplyTo":"cover.1332949186.git.wking@drexel.edu","subject":"Re: [PATCH v6 0/3] gitweb: refactor If-Modified-Since handling","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-28T15:56:24Z","receivedAt":"2012-03-28T15:56:24Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"On Wed, Mar 28, 2012 at 11:46:58AM -0400, wking@tremily.us wrote:\n> ...\n\nOops, I set `sendemail.from`, but it doesn't look like it stuck, and I\nforgot to check before OKing the mails.  Please consider the v6\npatches to be from `W. Trevor King <wking@drexel.edu>`, and adjust any\nreplies accordingly.\n\nSorry,\nTrevor\n\n-- \nThis email may be signed or encrypted with GnuPG (http://www.gnupg.org).\nFor more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy\n"},{"id":"187949","messageId":"201203281808.56432.jnareb@gmail.com","threadId":"30001","inReplyTo":"8129a671c793613ad2a0b00e1239e6a7ded7491a.1332949186.git.wking@drexel.edu","subject":"Re: [PATCH v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot().","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-28T16:08:55Z","receivedAt":"2012-03-28T16:08:55Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 28 Mar 2012, wking@tremily.us wrote:\n\n> Because snapshots can be large, you can save some bandwidth by\n> supporting caching via If-Modified-Since.  This patch adds support for\n> the i-m-s request to git_snapshot() if the request is a commit.\n> Requests for snapshots of trees, which lack well defined timestamps,\n> are still handled as they were before.\n> \n> Signed-off-by: W Trevor King <wking@drexel.edu>\n> ---\n>  gitweb/gitweb.perl                       |   10 +++++++++\n>  t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++\n>  2 files changed, 43 insertions(+), 0 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index ba106c1..591c793 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -7051,6 +7051,10 @@ sub git_snapshot {\n>  \n>  \tmy ($name, $prefix) = snapshot_name($project, $hash);\n>  \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> +\n> +\tmy %co = parse_commit($hash);\n> +\texit_if_unmodified_since($co{'committer_epoch'}) if %co;\n> +\n\nNice.\n\n>  \tmy $cmd = quote_command(\n>  \t\tgit_cmd(), 'archive',\n>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n> @@ -7060,9 +7064,15 @@ sub git_snapshot {\n>  \t}\n>  \n>  \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n> +\tmy %latest_date;\n> +\tif (%co) {\n> +\t\t%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n> +\t}\n> +\n\nThat is probably more readable than\n\n  +\t%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'})\n  +\t\tif %co;\n\n>  \tprint $cgi->header(\n>  \t\t-type => $known_snapshot_formats{$format}{'type'},\n>  \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> +\t\t-last_modified => (%co ? $latest_date{'rfc2822'} : undef),\n>  \t\t-status => '200 OK');\n>  \n\nSame issue as with previous patch:\n\n   \tprint $cgi->header(\n   \t\t-type => $known_snapshot_formats{$format}{'type'},\n   \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n  +\t\t%co ? (-last_modified => $latest_date{'rfc2822'}) : (),\n   \t\t-status => '200 OK');\n\n\n>  \topen my $fd, \"-|\", $cmd\n> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh\n> index afa6bd4..1487820 100755\n> --- a/t/t9501-gitweb-standalone-http-status.sh\n> +++ b/t/t9501-gitweb-standalone-http-status.sh\n> @@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '\n>  '\n>  test_debug 'cat gitweb.headers'\n>  \n> +test_expect_success 'modification: snapshot last-modified' '\n> +\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output &&\n> +\tgrep \"Last-modified: Thu, 7 Apr 2005 22:14:13 +0000\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: snapshot if-modified-since (modified)' '\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> +\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n> +\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: snapshot if-modified-since (unmodified)' '\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Thu, 7 Apr 2005 22:14:13 +0000\" &&\n> +\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n> +\tgitweb_run \"p=.git;a=snapshot;h=master;sf=tgz\" &&\n> +\tgrep \"Status: 304 Not Modified\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n> +\n> +test_expect_success 'modification: tree snapshot' '\n> +\tID=`git rev-parse --verify HEAD^{tree}` &&\n> +\texport HTTP_IF_MODIFIED_SINCE=\"Wed, 6 Apr 2005 22:14:13 +0000\" &&\n> +\ttest_when_finished \"unset HTTP_IF_MODIFIED_SINCE\" &&\n> +\tgitweb_run \"p=.git;a=snapshot;h=$ID;sf=tgz\" &&\n> +\tgrep \"Status: 200 OK\" gitweb.output &&\n> +\t! grep \"Last-Modified\" gitweb.output\n> +'\n> +test_debug 'cat gitweb.headers'\n\nGood!\n\n-- \nJakub Narebski\nPoland\n"}]}