What's cooking in git.git (Mar 2012, #07; Mon, 19)

What's cooking in git.git (Mar 2012, #07; Mon, 19)
--------------------------------------------------

Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

The "git push" preparation steps (there are two topics) have enough time to be polished to graduate to 'master' soon after 1.7.10 final.

You can find the changes described here in the integration branches of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--------------------------------------------------
[Graduated to "master"]

* ab/perl-i18n (2012-03-10) 3 commits
+ perl/Makefile: install Git::I18N under NO_PERL_MAKEMAKER
+ Git::I18N: compatibility with perl <5.8.3

Attempts to help installations with ancient Perl and/or without MakeMaker.

* jc/maint-verify-objects-remove-pessimism (2012-03-15) 1 commit (merged to 'next' on 2012-03-15 at f824530)
+ fetch/receive: remove over-pessimistic connectivity check

The code to validate the history connectivity between old refs and new refs used by fetch and receive-pack, introduced in 1.7.8, was grossly inefficient and unnecessarily tried to re-validate integrity of individual objects. This essentially reverts the patch that introduced the performance regression.

* jn/maint-fast-import-empty-ls (2012-03-09) 2 commits (merged to 'next' on 2012-03-15 at d531079)
+ fast-import: don't allow 'ls' of path with empty components
+ fast-import: leakfix for 'ls' of dirty trees

fast-import did not diagnose "ls ''" that asks an empty path as an error.

* sl/customize-sane-tool-path (2012-03-09) 1 commit (merged to 'next' on 2012-03-15 at a838844)
+ configure: allow user to prevent $PATH "sanitization" on Solaris

"configure" script learned to take "--sane-tool-path" from the command line to record SANE_TOOL_PATH (used to avoid broken platform tools in /usr/bin) in config.mak-autogen. This may be useful for people on Solaris who have saner tools outside /usr/xpg[46]/bin.

* th/doc-diff-submodule-option (2012-03-14) 1 commit
(merged to 'next' on 2012-03-14 at 0e1d755)
+ Documentation/diff-options: reword description of --submodule option

Update "diff --submodule" documentation.

* th/git-diffall (2012-03-14) 5 commits (merged to 'next' on 2012-03-14 at 38e1251)
+ contrib/diffall: fix cleanup trap on Windows
+ contrib/diffall: eliminate duplicate while loops
+ contrib/diffall: eliminate use of tar
+ contrib/diffall: create tmp dirs without mktemp
+ contrib/diffall: comment actual reason for 'cdup'

Update sample "diffall" script.

* th/mergetools-deltawalker (2012-03-15) 1 commit
(merged to 'next' on 2012-03-15 at 1a62faf)
+ Documentation/difftool: add deltawalker to list of valid diff tools

--------------------------------------------------
[New Topics]

* jc/maint-clean-nested-worktree-in-subdir (2012-03-15) 2 commits
- clean: preserve nested git worktree in subdirectories
- remove_dir_recursively(): Add flag for skipping removal of toplevel dir (this branch is tangled with jh/notes-merge-in-git-dir-worktree.)

"git clean -d -f" (not "-d -f -f") is supposed to protect nested working trees of independent git repositories that exist in the current project working tree from getting removed, but the protection applied only to such working trees that are at the top-level of the current project by mistake.

Not urgent.

* ct/advise-push-default (2012-03-19) 1 commit
- push: Provide situational hints for non-fast-forward errors

Breaks down the cases in which "git push" fails due to non-ff into three categories, and gives separate advise messages. I think there should be three advise confvars, not just two.

* nl/rebase-i-cheat-sheet (2012-03-16) 1 commit
- [Do not merge] rebase -i: remind that the lines are top-to-bottom

Not urgent.

* da/difftool-test (2012-03-19) 1 commit
- t7800: Test difftool passing arguments to diff

Makes sure "difftool" options can be given in any order.

* th/difftool-diffall (2012-03-19) 9 commits
- difftool: print list of valid tools with '--tool-help'
- difftool: teach dir-diff to copy modified files back to working tree
- difftool: teach difftool to handle directory diffs
- difftool: replace system call with Git::command_noisy
- difftool: eliminate setup_environment function
- difftool: stop appending '.exe' to git
- difftool: remove explicit change of PATH
- difftool: exit(0) when usage is printed
- difftool: parse options using Getopt::Long

Reworks the "diffall" contrib script into "difftool" framework. Use of Getopt::Long obviously contradicts with da/difftool-test and breaks 'pu'.

--------------------------------------------------
[Stalled]

* nd/columns (2012-03-13) 12 commits
- column: support grouping entries
- column: support "denser" mode
- ls-files: support --column
- tag: add --column
- column: support piping stdout to external git-column process
- status: add --column
- branch: add --column
- help: reuse print_columns() for help -a
- column: add dense layout support
- column: add columnar layout
- Stop starting pager recursively
- Add column layout skeleton and git-column

Rerolled again. Modulo minor nits, looked nicer than the previous round.

* nd/threaded-index-pack (2012-03-11) 2 commits
- index-pack: support multithreaded delta resolving
- index-pack: split second pass obj handling into own function

Another reroll after a bugreport on pthread usage discovered by Ramsey, but it seems the topic is cooking between Ramsay and Duy out of tree. Waiting for resolution.

* dg/test-from-elsewhere (2012-03-04) 2 commits
- Support out-of-tree Valgrind tests
- Allow overriding GIT_BUILD_DIR

Better support for out-of-tree test scripts, but it appears that the approach needs to be rethought. By repointing TEST_DIRECTORY to a directory other than $(pwd)/.., an out of place test script can reach test helpers and freshly built Git relative to it (GIT_BUILD_DIR is a mere short-hand for $TEST_DIRECTORY/..).

* cn/apply-fix-ws-can-lengthen-lines (2012-03-11) 1 commit . apply: reallocate the postimage buffer when needed

Attempts to address an ancient bug that dates back to the addition of an oddball "tab-in-indent" whitespace breakage class that wants to have longer lines than the original when fixing things up.

Needs more work; results in double-frees.

* hv/submodule-recurse-push (2012-02-13) 3 commits
- push: teach --recurse-submodules the on-demand option
- Refactor submodule push check to use string list instead of integer
- Teach revision walking machinery to walk multiple times sequencially

The bottom one was not clearly explained and needs a reroll.

* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
- fixup! 15eaaf4
- git-svn, perl/Git.pm: extend Git::prompt helper for querying users
- perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS

The bottom one has been replaced with a rewrite based on comments from Ævar. The second one needs more work, both in perl/Git.pm and prompt.c, to give precedence to tty over SSH_ASKPASS when terminal is available.

* jc/split-blob (2012-03-16) 6 commits
- chunked-object: streaming checkout
- chunked-object: fallback checkout codepaths
- bulk-checkin: support chunked-object encoding
- bulk-checkin: allow the same data to be multiply hashed
- new representation types in the packstream
- varint-in-pack: refactor varint encoding/decoding

Not ready.

I finished the streaming checkout codepath, but as explained in 127b177 (bulk-checkin: support chunked-object encoding, 2011-11-30), these are still early steps of a long and painful journey. At least pack-objects and fsck need to learn the new encoding for the series to be usable locally, and then index-pack/unpack-objects needs to learn it to be used remotely.

Given that I heard a lot of noise that people want large files, and that I was asked by somebody at GitTogether'11 privately for an advice on how to pay developers (not me) to help adding necessary support, I am somewhat dissapointed that the original patch series that was sent almost two months ago still remains here without much comments and updates from the developer community. I even made the interface to the logic that decides where to split chunks easily replaceable, and I deliberately made the logic in the original patch extremely stupid to entice others, especially the "bup" fanboys, to come up with a better logic, thinking that giving people an easy target to shoot for, they may be encouraged to help out. The plan is not working :-(.

--------------------------------------------------
[Cooking]

* jh/notes-merge-in-git-dir-worktree (2012-03-15) 4 commits
- notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd
- notes-merge: use opendir/readdir instead of using read_directory()
- t3310: illustrate failure to "notes merge --commit" inside $GIT_DIR/
- remove_dir_recursively(): Add flag for skipping removal of toplevel dir (this branch is tangled with jc/maint-clean-nested-worktree-in-subdir.)

Running "notes merge --commit" failed to perform correctly when run from any directory inside $GIT_DIR/. When "notes merge" stops with conflicts, $GIT_DIR/NOTES_MERGE_WORKTREE is the place a user edits to resolve it.

Not urgent.

* jn/diffstat-tests (2012-03-13) 7 commits
- diffstat summary line varies by locale: miscellany
- test: use numstat instead of diffstat in binary-diff test
- test: use --numstat instead of --stat in "git stash show" tests
- test: test cherry-pick functionality and output separately
- test: modernize funny-names test style
- test: use numstat instead of diffstat in funny-names test
- test: use test_i18ncmp when checking --stat output

Some tests checked the "diff --stat" output when they do not have to, which unnecessarily made things harder to verify under GETTEXT_POISON.

Not urgent.

* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits
- diff: tweak a _copy_ of diff_options with word-diff
- diff: refactor the word-diff setup from builtin_diff_cmd
- t4034: diff.*.wordregex should not be "sticky" in --word-diff

The regexp configured with wordregex was incorrectly reused across files.
Not urgent.

* zj/test-cred-helper-nicer-prove (2012-03-15) 2 commits
- t0303: resurrect commit message as test documentation
- t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

Minor improvement to t0303.
Not urgent.

* jc/commit-hook-authorship (2012-03-11) 3 commits (merged to 'next' on 2012-03-12 at 05ca7f8)
+ commit: pass author/committer info to hooks
+ t7503: does pre-commit-hook learn authorship?
+ ident.c: add split_ident_line() to parse formatted ident line (this branch is tangled with jc/run-hook-env-1.)

"git commit --author=$name" did not tell the name that was being recorded in the resulting commit to hooks, even though it does do so when the end user overrode the authorship via the "GIT_AUTHOR_NAME" environment variable. This is a simpler of the two approaches.

Will defer til 1.7.10.

* jc/run-hook-env-1 (2012-03-11) 3 commits
- run_hook(): enhance the interface to pass arbitrary environment
+ t7503: does pre-commit-hook learn authorship?
+ ident.c: add split_ident_line() to parse formatted ident line (this branch is tangled with jc/commit-hook-authorship.)

Not urgent.

Updates run_hook() API to be much less specific to "commit". It would only be useful if people start doing more interesting things with hooks.

* jc/diff-algo-cleanup (2012-02-19) 2 commits (merged to 'next' on 2012-03-15 at cca0032)
+ xdiff: PATIENCE/HISTOGRAM are not independent option bits
+ xdiff: remove XDL_PATCH_* macros
(this branch is used by jc/diff-ignore-case.)

Resurrects the preparatory clean-up patches from another topic that was discarded, as this would give a saner foundation to build on diff.algo configuration option series.

Not urgent.

* jh/apply-free-patch (2012-03-07) 1 commit
- apply: do not leak patches and fragments

Will defer til 1.7.10.

* rs/unpack-trees-leakfix (2012-03-06) 1 commit
(merged to 'next' on 2012-03-07 at 69a69cd)
+ unpack-trees: plug minor memory leak

Will defer til 1.7.10.

* tb/maint-remove-irrelevant-i18n-test (2012-03-06) 1 commit (merged to 'next' on 2012-03-07 at 23f2dd1)
+ t0204: remove a test that checks undefined behaviour

I tentatively parked this in 'next' but later reverted the merge. Will discard.

* mm/push-default-switch-warning (2012-03-09) 1 commit
- push: start warning upcoming default change for push.default

Not urgent.

This resurrects an ancient patch I wrote during a discussion we had in the 1.6.3-1.6.4 era. This should probably come after ct/advise-push-default topic and at that point the advise messages need to be rephrased, taking the future default change into account.

* jc/fmt-merge-msg-people (2012-03-13) 1 commit
- fmt-merge-msg: show those involved in a merged series

The "fmt-merge-msg" command learns to list the primary contributors involved in the side topic you are merging.

Will defer til 1.7.10.

* nl/http-proxy-more (2012-03-15) 5 commits
- http: rename HTTP_REAUTH to HTTP_AUTH_RETRY
- http: Avoid limit of retrying request only twice
- http: handle proxy authentication failure (error 407)
- http: handle proxy proactive authentication
- http: try http_proxy env var when http.proxy config option is not set

The code to talk to http proxies learn to use the same credential API used to talk to the final http destinations.

Will defer til 1.7.10.

* nd/stream-more (2012-03-07) 7 commits
(merged to 'next' on 2012-03-07 at 7325922)
+ update-server-info: respect core.bigfilethreshold
+ fsck: use streaming API for writing lost-found blobs
+ show: use streaming API for showing blobs
+ parse_object: avoid putting whole blob in core
+ cat-file: use streaming API to print blobs
+ Add more large blob test cases
+ streaming: make streaming-write-entry to be more reusable

Use API to read blob data in smaller chunks in more places to reduce the memory footprint. In general, looked fairly good.

Will defer til 1.7.10.

--------------------------------------------------
[Discarded]

* jc/diff-ignore-mode (2012-03-01) 1 commit
. diff --ignore-mode-change

* jc/diff-ignore-case (2012-02-28) 6 commits
. diff: -i is "--ignore-case" but means a bit more in "log" . diff: --ignore-case
. xdiff: introduce XDF_IGNORE_CASE
. xdiff: introduce XDF_INEXACT_MATCH
- xdiff: PATIENCE/HISTOGRAM are not independent option bits
- xdiff: remove XDL_PATCH_* macros

* jh/trace-use-startup-info (2012-03-02) 1 commit
. Use startup_info->prefix rather than prefix.

I tend to agree with the doubt of the author of this patch expressed.

* nd/optim-connected (2012-03-15) 1 commit
. {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack

jc/maint-verify-objects-remove-pessimism replaces this.

* jc/advise-push-default (2011-12-18) 1 commit
. push: hint to use push.default=upstream when appropriate

A rework by Christopher Tiwald replaces this.

* th/mergetools-tool-help (2012-03-15) 1 commit . difftool: print list of valid tools with '--tool-help'

Reworked as part of th/difftool-diffall topic.

My If-Modified-Since patch seems to have fallen through the cracks here. Is that because nobody is interested, or did I mess up something in the submission?

Cheers,
Trevor

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Probably "just fell through the cracks"; if somebody found what it wants to do very interesting but the implementation or submission lacking, you would have heard loud noises.

Again, just the highlights of the notable topics.

Not much has happened on the 'master' front.

[New Topics]

* am/completion-zsh-fix (2012-03-20) 1 commit
- contrib/completion: "local var=()" is misinterpreted as function decl by zsh

Needs a bit better explanation.

* lp/maint-diff-three-dash-with-graph (2012-03-20) 3 commits
- t4202: add test for "log --graph --stat -p" separator lines
- log --graph: fix break in graph lines
- log --graph --stat: three-dash separator should come after graph lines

The combination of two options "log --graph --stat" was an obscure corner case nobody cared about, and did not correctly show the ancestry graph lines.

I've split the original patch into three pieces, one for fixes to two different issues and a test. Also the test is adjusted so that the series can be back-merged to older codebase that did not have 7f81463 (Use correct grammar in diffstat summary line, 2012-02-01) that first appeared in v1.7.9.2

* wk/gitweb-snapshot-use-if-modified-since (2012-03-20) 1 commit
- Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

Unreviewed; the title looks way too long and does not sit well in the shortlog output.

* jc/maint-merge-autoedit (2012-03-20) 1 commit
- merge: backport GIT_MERGE_AUTOEDIT support

In 1.7.10, we added GIT_MERGE_AUTOEDIT=no environment variable to help older scripts to let them refuse giving users a chance to explain the merge, but forgot that 1.7.9 automatically opens an editor when merging an annotated tag, and there is no equivalent escape hatch. A merge of this topic to 1.7.10 track becomes a no-op, but we may want to apply this to the 1.7.9.x series.

[Cooking]

* ct/advise-push-default (2012-03-19) 1 commit
- push: Provide situational hints for non-fast-forward error

Breaks down the cases in which "git push" fails due to non-ff into three categories, and gives separate advise messages. This should be a good change regardless of mm/push-default-switch-warning topic.

* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits (merged to 'next' on 2012-03-20 at b3f67cd)
+ diff: tweak a _copy_ of diff_options with word-diff
+ diff: refactor the word-diff setup from builtin_diff_cmd
+ t4034: diff.*.wordregex should not be "sticky" in --word-diff

It would be nice to have this fix in 1.7.10 but perhaps we can wait until 1.7.10.1.

* nl/http-proxy-more (2012-03-15) 5 commits
(merged to 'next' on 2012-03-20 at c004001)
+ http: rename HTTP_REAUTH to HTTP_AUTH_RETRY
+ http: Avoid limit of retrying request only twice
+ http: handle proxy authentication failure (error 407)
+ http: handle proxy proactive authentication
+ http: try http_proxy env var when http.proxy config option is not set

The code to talk to http proxies learn to use the same credential API used to talk to the final http destinations.

[Stalled]

* th/difftool-diffall (2012-03-19) 9 commits
. difftool: print list of valid tools with '--tool-help'
. difftool: teach dir-diff to copy modified files back to working tree . difftool: teach difftool to handle directory diffs
. difftool: replace system call with Git::command_noisy
. difftool: eliminate setup_environment function
. difftool: stop appending '.exe' to git
. difftool: remove explicit change of PATH
. difftool: exit(0) when usage is printed
. difftool: parse options using Getopt::Long

Reworks the "diffall" contrib script into "difftool" framework. Breaks tests when merged to 'pu'.

* jh/apply-free-patch (2012-03-20) 2 commits
- apply: free patch->result
- apply: do not leak patches and fragments

Valgrind reports quite a lot of discarded memory inside apply. I have a slight suspicion that we should first clarify the ownership rules of pieces of memory in this standalone program that was designed to be "run once and let exit take care of the memory" before proceeding further.

* dg/test-from-elsewhere (2012-03-04) 2 commits
- Support out-of-tree Valgrind tests
- Allow overriding GIT_BUILD_DIR

Better support for out-of-tree test scripts, but it appears that the approach needs to be rethought. By repointing TEST_DIRECTORY to a directory other than $(pwd)/.., an out of place test script can reach test helpers and freshly built Git relative to it (GIT_BUILD_DIR is a mere short-hand for $TEST_DIRECTORY/..).

Discussion stalled.

The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function die_if_unmodified. This makes the code easy to reuse for other actions where it is appropriate and adds the code to do that to git_snapshot.

---
Changed since v2:
- Shorter title.
- Fixed modified_since -> die_if_unmodified in commit message.

index a8b5fad..b944351 100755
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub die_if_unmodified {
+	my ($latest_epoch) = @_;
+	our $cgi;
+
+	my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+	if (defined $if_modified) {
+		my $since;
+		if (eval { require HTTP::Date; 1; }) {
+			$since = HTTP::Date::str2time($if_modified);
+		} elsif (eval { require Time::ParseDate; 1; }) {
+			$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+		}
+		if (defined $since && $latest_epoch <= $since) {
+			my %latest_date = parse_date($latest_epoch);
+			print $cgi->header(
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			goto DONE_GITWEB;
+		}
+	}
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {

@@ -7029,6 +7051,10 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash) or die_error(404, "Unknown commit object");
+	die_if_unmodified($co{'committer_epoch'});
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",

@@ -7038,9 +7064,11 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
+	my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
 	print $cgi->header(
 		-type => $known_snapshot_formats{$format}{'type'},
 		-content_disposition => 'inline; filename="' . $filename . '"',
+		-last_modified => $latest_date{'rfc2822'},
 		-status => '200 OK');
 
 	open my $fd, "-|", $cmd

@@ -7820,24 +7848,8 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
+		die_if_unmodified($latest_epoch);
 		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
-		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
-		if (defined $if_modified) {
-			my $since;
-			if (eval { require HTTP::Date; 1; }) {
-				$since = HTTP::Date::str2time($if_modified);
-			} elsif (eval { require Time::ParseDate; 1; }) {
-				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
-			}
-			if (defined $since && $latest_epoch <= $since) {
-				print $cgi->header(
-					-type => $content_type,
-					-charset => 'utf-8',
-					-last_modified => $latest_date{'rfc2822'},
-					-status => '304 Not Modified');
-				return;
-			}
-		}
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',

1.7.3.4                                                                         

By the way, it is custom on this mailing list to usually Cc (send a copy) to all people participating in discussion, and not only to git mailing list.

Perhaps a better title would be:

gitweb: Refactor If-Modified-Since handling, support in snapshot

to mention all that thispatch does. Though trouble with coming up with a short but fairly complete one-line summary might mean that this patch would be better split in two: refactoring and adding support for If-Modified-Since to snapshots.

O.K.

                                   ^^^^^^^^^^^^^^^^^^^^^^^

This doesn't add any information. I think it the commit message would be better if you either remove this, or expand (in a separate paragraph) where support for If-Modified-Since might make sense, and where it could not.

This would read better as a separate paragraph.

Nice.

BTW. what happened to diffstat?

Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)? We could use test_tick() and $test_tick for that (or extract formatted committer date from a commit).

  +
  +	my %co = parse_commit($hash);
  +	die_if_unmodified($co{'committer_epoch'}) if %co;
  +

Jakub Narebski

Ah, sorry. I figured that if you got the original email to the list, I'd just be doubling up in your inbox by Cc-ing you directly.

Agreed.

I'll expand it in the refactoring patch.

My local branch has sequential commits for each patch version (e.g., commits for v1, v2, ...). When it's time to email the list, I'm supposed to send logical patches against the trunk (e.g., patches for refactoring, git_snapshot, ...). For v2 I just used git diff origin/master to generate the patch, and it doesn't include the diffstat. Now that I'm splitting into two patches, I'll probably use git rebase -i origin/master and just keep track of the changes by hand ;). If there's a better way that I'm overlooking, feel free to point me in the right direction.

I'll try to set that up. Should it be bundled into the git_snapshot patch, or should there be three patches:

1: gitweb: Refactor If-Modified-Since handling 2: gitweb: Add If-Modified-Since tests for git_snapshot 3: gitweb: Add If-Modified-Since support for git_snapshot

I'm still not understanding the problem here. The following all work in my testing:

http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d545cab4a8dc894fae2c2634a74993ea62b054d;sf=tgz
http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d5;sf=tgz
http://.../gitweb.cgi?p=a/.git;a=snapshot;h=HEAD;sf=tgz

Can you give me an example of a hash that you expect to not work?

Cheers,
Trevor

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

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

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

And I think I am not the only one. But I guess that even for those subscribed, emails addressed directly to them take priority, which is important considering volume of email on git mailing list.

[..]

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

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

Note that you need to edit at least cover letter.

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

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

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

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

Currently all of those work

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

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

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

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

Jakub Narebski
Poland

Ah, I like that. Then I can rebase away ;).

But the new tests would be for the new functionality (i.e. snapshot support), so they wouldn't belong in the general refactoring commit.

Indeed it does. I'll add you're suggested skipping for these cases.

There should be a way to determine the oldest commit refering to a tree, which could be used for timestamping tree-ishes, but that can be a project for another day ;).

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Yep. Or use some patch / mailqueue management interface like StGit.

I was thinking that "and add tests" would be about 'feed' action test and i-m-s.

If by "skipping" you mean here turning off handling of If-Modified-Since and Last-Modified headers, then I agree.

That also can be ambiguous.

Jakub Narebski
Poland

Then you are planning to split it in a wrong way.

As I said, I do not think it matters that much for a small patch like this, but if the plan is to make the part to create i-m-s helper function as a standalone "refactoring" patch, then what Jakub outlined is the right way to go about it.

In the first patch, you create i-m-s helper and update the existing code that can use the helper, without changing anything else. Do not touch snapshot code in this patch, if the current code does not support i-m-s in snapshot. And in the same patch, add tests for codepaths that use i-m-s to make sure your refactoring did not break them. In other words, if you remove the change to gitweb/ from the first patch and apply only the changes to the tests, the resulting new tests should pass with the current code that has i-m-s support inline without the i-m-s helper. And if you add back your change to gitweb/ for the refactoring, the test should still pass.

And then in the second patch, you update the snapshot code and whatever else that can use i-m-s helper to support i-m-s. You can add tests to protect the new feature from future breakages in this patch.

Ah, I was assuming that some current tests might be checking the current behavior, and that my new tests would be testing my new snapshot behavior. If the old i-m-s handling also needs tests, that should happen before any of my previously proposed patches:

1: tests for i-m-s and git_feed
2: refactor i-m-s handling
3: tests for i-m-s and git_snapshot (which fail until 4) 4: add i-m-s to git_snapshot

How does that sound?

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Those two can be in single commit. Tests added need only test i-m-s using git_feed ('atom' or 'rss' action), as it is the only user, and you touch only i-m-s handling.

We usually put tests together with feature. Tests before feature means that you would need to mark them as test_expect_failure, as they would not pass before feature is added, isn't it?

Jakub Narebski
Poland

Correct.

Correct.

In a more elaborate series, you would first add a lot of tests to nail the behaviour of the existing code down, then refactor or reimplement them in a different way, add tests that expect failure until the next phase, and then add features and turn expect_failure in tests to expect_success.

But I do not htink this single patch deserves that complexity. Does it even have to split into three?

Sorry, testing makes me think of test-driven development ;). Two patches it is (refactor + git_feed tests, git_snapshot + git_snapshot tests).

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

With "gitweb: " prefix to denote what area it affects, that is certainly better. Given the primary objective and effect is that the snapshot feature starts honoring i-m-s,

gitweb: honor If-Modified-Since request header in snapshot

would be sufficient.

If many existing callsites had duplicated code to handle i-m-s, we may want two patch series, the first of which consolidates them into a single helper function without changing anything else (most importantly, without regression) and the second that uses the helper to add support in the snapshot feature. But if that is not the case, I think we can go either way.

That is a very good title... and if all changes were to be put in single commit (see below), that is what we should concentrate on. Refactoring would be just a detail.

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

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

tl;dr. Two commits.

Jakub Narebski
Poland

OK, thanks.

Sorry for the delay since my last message, it's been a busy week ;).

I'll be sending along three patches. The second and third are the ones you discuss above. The first is a teensy patch to add Status output to non-304 calls to git_feed(). Without it you'd have to get a bit more creative in the test suite. If the status lines were left out intentionally, let me know, and I'll come up with another condition for those tests.

The second patch refactors and tests git_feed(), and the third patch adds i-m-s support to git_snapshot() with associated tests.

Changes since v3:
* Patch 1/3 is completely new.
* Split previous patch into 2/3 and 3/3.
* Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}.
* Added testing to both 2/3 and 3/3.
* Reworked commit messages.

Cheers,
Trevor

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

The git_feed() method was not setting a Status header unless it was responding to an If-Modified-Since request with 304 Not Modified. Now, when it is serving successful responses, it sets status to 200 OK.

---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index a8b5fad..041da17 100755
@@ -7841,11 +7841,13 @@ sub git_feed {
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'});
+			-last_modified => $latest_date{'rfc2822'},
+			-status => '200 OK');
 	} else {
 		print $cgi->header(
 			-type => $content_type,
-			-charset => 'utf-8');
+			-charset => 'utf-8',
+			-status => '200 OK');
 	}
 
 	# Optimization: skip generating the body if client asks only

1.7.3.4

The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function die_if_unmodified. This makes the code easy to reuse for other actions.

Only gitweb actions which can easily calculate a modification time should use die_if_unmodified, as the goal is to balance local processing time vs. upload bandwidth.

---
 gitweb/gitweb.perl                       |   40 +++++++++++++++++------------
 t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++-
 2 files changed, 49 insertions(+), 18 deletions(-)

index 041da17..229f3da 100755
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub die_if_unmodified {
+	my ($latest_epoch) = @_;
+	our $cgi;
+
+	my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+	if (defined $if_modified) {
+		my $since;
+		if (eval { require HTTP::Date; 1; }) {
+			$since = HTTP::Date::str2time($if_modified);
+		} elsif (eval { require Time::ParseDate; 1; }) {
+			$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+		}
+		if (defined $since && $latest_epoch <= $since) {
+			my %latest_date = parse_date($latest_epoch);
+			print $cgi->header(
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			goto DONE_GITWEB;
+		}
+	}
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {

@@ -7820,24 +7842,8 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
+		die_if_unmodified($latest_epoch);
 		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
-		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
-		if (defined $if_modified) {
-			my $since;
-			if (eval { require HTTP::Date; 1; }) {
-				$since = HTTP::Date::str2time($if_modified);
-			} elsif (eval { require Time::ParseDate; 1; }) {
-				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
-			}
-			if (defined $since && $latest_epoch <= $since) {
-				print $cgi->header(
-					-type => $content_type,
-					-charset => 'utf-8',
-					-last_modified => $latest_date{'rfc2822'},
-					-status => '304 Not Modified');
-				return;
-			}
-		}
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',

index 31076ed..0e49f29 100755
@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'
 test_expect_success 'snapshots: bad tree-ish id (tagged object)' '
 	echo object > tag-object &&
 	git add tag-object &&
-	git commit -m "Object to be tagged" &&
+	test_tick && git commit -m "Object to be tagged" &&
 	git tag tagged-object `git hash-object tag-object` &&
 	gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" &&
 	grep "400 - Object is not a tree-ish" gitweb.output

@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '
 '
 test_debug 'cat gitweb.output'
 
+# ----------------------------------------------------------------------
+# modification times (Last-Modified and If-Modified-Since)
+
+test_expect_success 'modification: feed last-modified' '
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.output
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking

1.7.3.4

I did something closer to your suggestion in my v1 patch, but

Only passing in the epoch. We could reduce computation at the expense of complication by passing in both the epoch and a formatted time string, but after Jakub's suggestions, I felt like a simpler interface was the better approach. I don't feel particularly committed to either way, so just tell me which you'd like best ;).

Will fix in v5.

Oops. I wonder why my tests still passed :p. Will fix in v5.

You're welcome. Thanks for shepherding me through it ;).

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

The existing codepath already had the call to parse_date() nearby and that was the only reason I made the suggestion. The caller in the snapshot codepath would need to call parse_date() much earlier and it would have to be conditional to the availability of %co, so the simpler interface would probably be overall win.

The test will pass if there is no breakage. The offence of missing " &&" after your "grep 200" is that it will let the test pass even if you did not have "200" in the output, failing to catch future breakage.

The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function exit_if_unmodified_since. This makes the code easy to reuse for other actions.

Only gitweb actions which can easily calculate a modification time should use exit_if_unmodified_since, as the goal is to balance local processing time vs. upload bandwidth.

---
Patch v4 1/3 is unchanged. Should I mail it back in with a [PATCH v5 1/3] tag?

Changes since v4:
* die_if_unmodified() -> exit_if_unmodified_since()
* Added missing && to tests for feed-last-modified (patch 2/3) and snapshot-last-modified (patch 3/3).

 gitweb/gitweb.perl                       |   40 +++++++++++++++++------------
 t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++-
 2 files changed, 49 insertions(+), 18 deletions(-)

index 041da17..229f3da 100755
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub exit_if_unmodified_since {
+	my ($latest_epoch) = @_;
+	our $cgi;
+
+	my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+	if (defined $if_modified) {
+		my $since;
+		if (eval { require HTTP::Date; 1; }) {
+			$since = HTTP::Date::str2time($if_modified);
+		} elsif (eval { require Time::ParseDate; 1; }) {
+			$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+		}
+		if (defined $since && $latest_epoch <= $since) {
+			my %latest_date = parse_date($latest_epoch);
+			print $cgi->header(
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			goto DONE_GITWEB;
+		}
+	}
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {

@@ -7820,24 +7842,8 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
+		exit_if_unmodified_since($latest_epoch);
 		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
-		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
-		if (defined $if_modified) {
-			my $since;
-			if (eval { require HTTP::Date; 1; }) {
-				$since = HTTP::Date::str2time($if_modified);
-			} elsif (eval { require Time::ParseDate; 1; }) {
-				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
-			}
-			if (defined $since && $latest_epoch <= $since) {
-				print $cgi->header(
-					-type => $content_type,
-					-charset => 'utf-8',
-					-last_modified => $latest_date{'rfc2822'},
-					-status => '304 Not Modified');
-				return;
-			}
-		}
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',

index 31076ed..0e49f29 100755
@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'
 test_expect_success 'snapshots: bad tree-ish id (tagged object)' '
 	echo object > tag-object &&
 	git add tag-object &&
-	git commit -m "Object to be tagged" &&
+	test_tick && git commit -m "Object to be tagged" &&
 	git tag tagged-object `git hash-object tag-object` &&
 	gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" &&
 	grep "400 - Object is not a tree-ish" gitweb.output

@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '
 '
 test_debug 'cat gitweb.output'
 
+# ----------------------------------------------------------------------
+# modification times (Last-Modified and If-Modified-Since)
+
+test_expect_success 'modification: feed last-modified' '
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.output &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking

1.7.3.4

Nice description, and I think quite good name for a subroutine. Well, good enough; any more we would drown in bikeshed-ding.

Good.

  +	my ($latest_epoch, $latest_date) = @_;

  +			my %latest_date = ref($latest_date) eq 'HASH' 
  +				? %$latest_date : parse_date($latest_epoch);

Jakub Narebski
Poland

That's the date set by the first test_tick, which is hardcoded in test-lib-functions.sh. Extracting the date dynamically seems unnecessary, since I can't imagine anyone changing the test_tick date. It's easy enough to do if you think it is appropriate though…

Good point. Sloppy me not reading t/README thoroughly enough ;).

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Ah, it's all right then. I should have checked the test_tick function.

That of course assuming that nobody would add test_tick earlier, but if he/she does, he/she can deal with fallout...

No, it is not needed.

Jakub Narebski
Poland

The git_feed() method was not setting a Status header unless it was responding to an If-Modified-Since request with 304 Not Modified. Now, when it is serving successful responses, it sets status to 200 OK.

---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index a8b5fad..041da17 100755
@@ -7841,11 +7841,13 @@ sub git_feed {
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'});
+			-last_modified => $latest_date{'rfc2822'},
+			-status => '200 OK');
 	} else {
 		print $cgi->header(
 			-type => $content_type,
-			-charset => 'utf-8');
+			-charset => 'utf-8',
+			-status => '200 OK');
 	}
 
 	# Optimization: skip generating the body if client asks only

1.7.3.4

The current gitweb only generates Last-Modified and handles If-Modified-Since headers for the git_feed action. This patch breaks the Last-Modified and If-Modified-Since handling code out from git_feed into a new function exit_if_unmodified_since. This makes the code easy to reuse for other actions.

Only gitweb actions which can easily calculate a modification time should use exit_if_unmodified_since, as the goal is to balance local processing time vs. upload bandwidth.

---
 gitweb/gitweb.perl                       |   57 +++++++++++++++--------------
 t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++-
 2 files changed, 55 insertions(+), 29 deletions(-)

index 041da17..ba106c1 100755
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub exit_if_unmodified_since {
+	my ($latest_epoch) = @_;
+	our $cgi;
+
+	my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+	if (defined $if_modified) {
+		my $since;
+		if (eval { require HTTP::Date; 1; }) {
+			$since = HTTP::Date::str2time($if_modified);
+		} elsif (eval { require Time::ParseDate; 1; }) {
+			$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+		}
+		if (defined $since && $latest_epoch <= $since) {
+			my %latest_date = parse_date($latest_epoch);
+			print $cgi->header(
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			goto DONE_GITWEB;
+		}
+	}
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {

@@ -7820,35 +7842,14 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
-		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
-		if (defined $if_modified) {
-			my $since;
-			if (eval { require HTTP::Date; 1; }) {
-				$since = HTTP::Date::str2time($if_modified);
-			} elsif (eval { require Time::ParseDate; 1; }) {
-				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
-			}
-			if (defined $since && $latest_epoch <= $since) {
-				print $cgi->header(
-					-type => $content_type,
-					-charset => 'utf-8',
-					-last_modified => $latest_date{'rfc2822'},
-					-status => '304 Not Modified');
-				return;
-			}
-		}
-		print $cgi->header(
-			-type => $content_type,
-			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'},
-			-status => '200 OK');
-	} else {
-		print $cgi->header(
-			-type => $content_type,
-			-charset => 'utf-8',
-			-status => '200 OK');
+		exit_if_unmodified_since($latest_epoch);
+		%latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
 	}
+	print $cgi->header(
+		-type => $content_type,
+		-charset => 'utf-8',
+		-last_modified => (%latest_date ? $latest_date{'rfc2822'} : undef),
+		-status => '200 OK');
 
 	# Optimization: skip generating the body if client asks only
 	# for Last-Modified date.

index 31076ed..afa6bd4 100755
@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'
 test_expect_success 'snapshots: bad tree-ish id (tagged object)' '
 	echo object > tag-object &&
 	git add tag-object &&
-	git commit -m "Object to be tagged" &&
+	test_tick && git commit -m "Object to be tagged" &&
 	git tag tagged-object `git hash-object tag-object` &&
 	gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" &&
 	grep "400 - Object is not a tree-ish" gitweb.output

@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '
 '
 test_debug 'cat gitweb.output'
 
+# ----------------------------------------------------------------------
+# modification times (Last-Modified and If-Modified-Since)
+
+test_expect_success 'modification: feed last-modified' '
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.output &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking

1.7.3.4

Changes since v5:
* Use test_when_finished to unset HTTP_IF_MODIFIED_SINCE.
* Actually use the tree ID in the modification: tree snapshot test.
* Conditional last_modified argument for $cgi->header git_snapshot and git_feed.
* tree-ish -> trees in git_snapshot commit message and test name.

Unchanged since v5:
* Patch 1/3, but I'm resending it anyway with git send-email. I could skip resending it with:

git send-email … HEAD^^

but that would mess up the patch numbering.

W. Trevor King (3):
gitweb: add status headers to git_feed() responses.
gitweb: refactor If-Modified-Since handling
gitweb: add If-Modified-Since handling to git_snapshot().

 gitweb/gitweb.perl                       |   65 ++++++++++++++++++------------
 t/t9501-gitweb-standalone-http-status.sh |   60 +++++++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 27 deletions(-)

1.7.3.4

Oops, I set sendemail.from, but it doesn't look like it stuck, and I forgot to check before OKing the mails. Please consider the v6 patches to be from W. Trevor King <[email protected]>, and adjust any replies accordingly.

Sorry,
Trevor

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Because snapshots can be large, you can save some bandwidth by supporting caching via If-Modified-Since. This patch adds support for the i-m-s request to git_snapshot() if the request is a commit. Requests for snapshots of trees, which lack well defined timestamps, are still handled as they were before.

---
 gitweb/gitweb.perl                       |   10 +++++++++
 t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

index ba106c1..591c793 100755
@@ -7051,6 +7051,10 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash);
+	exit_if_unmodified_since($co{'committer_epoch'}) if %co;
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",

@@ -7060,9 +7064,15 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
+	my %latest_date;
+	if (%co) {
+		%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+	}
+
 	print $cgi->header(
 		-type => $known_snapshot_formats{$format}{'type'},
 		-content_disposition => 'inline; filename="' . $filename . '"',
+		-last_modified => (%co ? $latest_date{'rfc2822'} : undef),
 		-status => '200 OK');
 
 	open my $fd, "-|", $cmd

index afa6bd4..1487820 100755
@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '
 '
 test_debug 'cat gitweb.headers'
 
+test_expect_success 'modification: snapshot last-modified' '
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output &&
+	! grep "Last-Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 

1.7.3.4

Because snapshots can be large, you can save some bandwidth by supporting caching via If-Modified-Since. This patch adds support for the i-m-s request to git_snapshot() if the requested hash is a commit. Requests for snapshots of tree-ishes, which lack well defined timestamps, are still handled as they were before.

---
 gitweb/gitweb.perl                       |   21 +++++++++++++++---
 t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

index 229f3da..be9ad5d 100755
@@ -7051,6 +7051,10 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash);
+	exit_if_unmodified_since($co{'committer_epoch'}) if %co;
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",

@@ -7060,10 +7064,19 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
-	print $cgi->header(
-		-type => $known_snapshot_formats{$format}{'type'},
-		-content_disposition => 'inline; filename="' . $filename . '"',
-		-status => '200 OK');
+	if (%co) {
+		my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+		print $cgi->header(
+			-type => $known_snapshot_formats{$format}{'type'},
+			-content_disposition => 'inline; filename="' . $filename . '"',
+			-last_modified => $latest_date{'rfc2822'},
+			-status => '200 OK');
+	} else {
+		print $cgi->header(
+			-type => $known_snapshot_formats{$format}{'type'},
+			-content_disposition => 'inline; filename="' . $filename . '"',
+			-status => '200 OK');
+	}
 
 	open my $fd, "-|", $cmd
 		or die_error(500, "Execute git-archive failed");

index 0e49f29..38e90bd 100755
@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '
 '
 test_debug 'cat gitweb.headers'
 
+test_expect_success 'modification: snapshot last-modified' '
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree-ish snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output &&
+	! grep "Last-Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 

1.7.3.4

"if the requested hash is a commit" means here "if we request snapshot of a commit", isn't it?

s/tree-ishes/trees/. Commit is tree-ish but not a tree; it is tree that lacks timestamp that commit has.

Jakub Narebski
Poland

Right. I'll fix those in v6 ;).

Ah, that's much better. I'm not particulary familar with Perl, sorry ;).

I'll wait until I here back on Last-Modified timestamp generation, and then submit v6.

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Because snapshots can be large, you can save some bandwidth by supporting caching via If-Modified-Since. This patch adds support for the i-m-s request to git_snapshot() if the requested hash is a commit. Requests for snapshots of tree-ishes, which lack well defined timestamps, are still handled as they were before.

---
 gitweb/gitweb.perl                       |   21 +++++++++++++++---
 t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 4 deletions(-)

index 229f3da..be9ad5d 100755
@@ -7051,6 +7051,10 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash);
+	die_if_unmodified($co{'committer_epoch'}) if %co;
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",

@@ -7060,10 +7064,19 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
-	print $cgi->header(
-		-type => $known_snapshot_formats{$format}{'type'},
-		-content_disposition => 'inline; filename="' . $filename . '"',
-		-status => '200 OK');
+	if (%co) {
+		my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+		print $cgi->header(
+			-type => $known_snapshot_formats{$format}{'type'},
+			-content_disposition => 'inline; filename="' . $filename . '"',
+			-last_modified => $latest_date{'rfc2822'},
+			-status => '200 OK');
+	} else {
+		print $cgi->header(
+			-type => $known_snapshot_formats{$format}{'type'},
+			-content_disposition => 'inline; filename="' . $filename . '"',
+			-status => '200 OK');
+	}
 
 	open my $fd, "-|", $cmd
 		or die_error(500, "Execute git-archive failed");

index 0e49f29..38e90bd 100755
@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '
 '
 test_debug 'cat gitweb.headers'
 
+test_expect_success 'modification: snapshot last-modified' '
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree-ish snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output &&
+	! grep "Last-Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 

1.7.3.4

Thanks for working on this topic.

BTW. we are in feature freeze currently, before release of v1.7.10.

No, the status line was omitted unintentionally, thanks for fixing this. I guess that web server (at least Apache) adds it if it is missing.

Though I am not sure how lack of "Status:" header is different from incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'...

Good.

Jakub Narebski
Poland

I'm not in a big hurry or anything, just trying to save some bandwidth…

It's not for the 304 tests. I thought it would be a good idea to grep for a 200 status before looking for Last-Modified times, to ensure you actually get to the successful response stage.

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

Right.

Testing both sides of a new feature (i.e. modified and unmodified here) is a good testing practice. Thanks.

Jakub Narebski
Poland

From the CGI 1.1 specs [1], section 9.2:

If the script does not return a Status header, then "200 OK" should be assumed.

So, perhaps it is better to skip the Status header on success (less for the server to parse).

[1]: http://tools.ietf.org/html/draft-robinson-www-interface-00

This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

I do not think it matters either way. It is just a single header line, no?

The code is certainly more explicit with your patch.