Gitweb snapshot selective - #206

FYI : the 6-patch set was submitted by e-mail using hints from #137
http://git.661346.n2.nabble.com/PATCH-1-6-gitweb-perl-added-ability-to-quot-git-archive-quot-just-certain-paths-files-or-subdirs-note-tt7650409.html#none

I see that https://public-inbox.org/git/[email protected]/#t never saw any replies.

My best guess is that this is because the commit messages look more like Subversion-style oneliners than like the commits you usually find in the Git history.

See for example https://public-inbox.org/git/[email protected]/T/#m47da28d57215cc937e202da0d81a6206b185d771. As you will readily recognize the commit message there makes kind of a case why this is a desirable change. It uses only one paragraph to do so, but there are plenty of examples in Git's commit history of commit messages that give quite a bit more background and motivation to the patches.

You can also see that this patch series of two patches was introduced with a cover letter that explains the broader structure, and I would suggest that also for your patches.

Also, you probably noticed that all patches have a sign off, please do the same.

It may have been a busy time back then, too, maybe you want to wait until v2.22.0 is released (according to https://tinyurl.com/gitCal it is expected this Friday) and send a new iteration?

@jimklimov v2.22.0 was released three weeks ago. Maybe now would be a good time?

Hello, sorry this sort of fell out of the loop in the busy mailbox :)
I'll try to update the PR formalities soon.

For now, rebased over current master and updated with Signed-off-by tags.

I'll have to go over the commits and re-push later to make sense of what and why I wrote years ago ;)

Reviewed (and rearranged a bit) the commit contents and updated their descriptions, although rather following the reasonable Problem/Solution pattern promoted in several ecosystems I'm in nowadays.

Hope it makes sense here too :)

Unit-tests for these changes passed locally with ./configure && make && make -C t/ gitweb-test however I see some failures like on CI for the make test... I assume the upstream state I had rebased against was a bit broken (some tests are even commented as known breakage).

Had a few iterations above for documentation - asciidoc not markdown, no syntax-highlight tool in build env, etc.

I see some failures like on CI for the make test...

I looked at the logs, and it seems to have been a (hopefully intermittent) timeout while getting the Homebrew cask. I restarted the build.

In Travis I see this for one scenario:

Test Summary Report
-------------------
t9502-gitweb-standalone-parse-output.sh          (Wstat: 256 Tests: 13 Failed: 1)
  Failed test:  13
  Non-zero exit status: 1
  Parse errors: No plan found in TAP output
t7405-submodule-merge.sh                         (Wstat: 0 Tests: 18 Failed: 0)
  TODO passed:   17
Files=872, Tests=20633, 526 wallclock secs ( 9.49 usr  1.89 sys + 686.64 cusr 299.85 csys = 997.87 CPU)
Result: FAIL
Makefile:52: recipe for target 'prove' failed

with Linux / Xcode: xcode10.1 C / jobname=GIT_TEST_GETTEXT_POISON

but have little idea what that means so far.

Seems I can reproduce locally however, with

~/git/t$ GIT_TEST_GETTEXT_POISON=YesPlease make clean t9502-gitweb-standalone-parse-output.sh

so will try to figure it out.

Ok, so it seems the new tests which expect ...bad path - empty output and/or HTTP-404 send requests that instead return a Status: 500 Internal Server Error when GIT_TEST_GETTEXT_POISON=YesPlease. Without the latter, server does return 404 as expected.

Looking at how this is my fault... per https://github.com/git/git/blob/master/git-sh-i18n.sh#L66

Yes... so it 500 - Failed to transmit output from git-archive&lt;br/&gt;&lt;pre&gt;fatal: # GETTEXT POISON #<span class="cntrl">\n</span>&lt;/pre&gt;&lt;br/&gt; probably due to comparison at https://github.com/git/git/pull/206/files#diff-00703a794a540acf45e225abd6aeda3bR7512

I wonder how else (better) such parsing can be handled... would it help (certainly would be "more correct" than what I PR'ed now) to export locale settings into $cmd at https://github.com/git/git/pull/206/files#diff-00703a794a540acf45e225abd6aeda3bR7451 ?

Forcing LANG=C LC_ALL=C LANGUAGE=C into it in the web-server might not hurt, but did not help against poisoning...

It's curious how the Azure Pipeline passes, though. Any idea why?

Not sure. Might run a different set of tests? Per test script, poisoned gettext is quite OS-dependent.

I now lean towards turning a blind eye and accepting both HTTP 404 and 500 as valid outcomes for test (maybe depending on the envvar for poisoned mode), lacking a way to determine git error type or details other than by parsing error texts.

Not sure.

Well, you could have a look ;-) From the output of https://travis-ci.org/git/git/jobs/565980864 (you have to click open the "ci/print-test-failures.sh" part, then search for "not ok"):

------------------------------------------------------------------------
test-results/t9502-gitweb-standalone-parse-output.out...
------------------------------------------------------------------------
[...]
+rm -f gitweb.body file_list
+BRANCH=master
+gitweb_run p=.git;a=snapshot;h=master;sf=tar;f=third
+GATEWAY_INTERFACE=CGI/1.1
+HTTP_ACCEPT=*/*
+REQUEST_METHOD=GET
+QUERY_STRING=p=.git;a=snapshot;h=master;sf=tar;f=third
+PATH_INFO=
+export GATEWAY_INTERFACE HTTP_ACCEPT REQUEST_METHOD QUERY_STRING PATH_INFO
+pwd
+GITWEB_CONFIG=/home/travis/build/git/git/t/trash directory.t9502-gitweb-standalone-parse-output/gitweb_config.perl
+export GITWEB_CONFIG
+rm -f gitweb.log
+perl -- /home/travis/build/git/git/t/../gitweb/gitweb.perl
+command /usr/bin/perl -- /home/travis/build/git/git/t/../gitweb/gitweb.perl
+perl -w -e 
		open O, ">gitweb.headers";
	while (<>) {
			print O;
			last if (/^\r$/ || /^$/);
		}
		open O, ">gitweb.body";
		while (<>) {
			print O;
		}
		close O;
	 gitweb.output
+command /usr/bin/perl -w -e 
		open O, ">gitweb.headers";
		while (<>) {
			print O;
			last if (/^\r$/ || /^$/);
		}
		open O, ">gitweb.body";
		while (<>) {
			print O;
		}
		close O;
	 gitweb.output
+grep ^[[] gitweb.log
+true
+egrep ^Status: 404 
+head -1 gitweb.headers
+[ ! -s gitweb.body -o -n  ]
error: last command exited with $?=1
not ok 13 - snapshot certain objects: have expected content in master branch - subdir name is required in requested nested path (bad path - empty output and/or HTTP-404)
#	
#		rm -f gitweb.body file_list &&
#		BRANCH=master &&
#		gitweb_run "p=.git;a=snapshot;h=$BRANCH;sf=tar;f=third" &&
#		[ ! -s gitweb.body -o -n "`head -1 gitweb.headers | egrep "^Status: 404 "`" ]
#	

As indicated by the end of the log:

------------------------------------------------------------------------
Trash directories embedded in this log can be extracted by running:

  curl https://api.travis-ci.org/v3/job/565980864/log.txt |./ci/util/extract-trash-dirs.sh

If you do that, you will find that there is a gitweb.headers file, and its contents read like this:

Status: 500 Internal Server Error
Content-Type: text/html; charset=utf-8

That's no good. We clearly expect a 404 ("Not Found"), and instead get the totally unhelpful 500 ("Internal Server Error").

So what to do about this?

In the least, a bit more investigation is in order. For example, gitweb.body clearly contains this indicative line:

500 - Failed to transmit output from git-archive&lt;br/&gt;&lt;pre&gt;fatal: # GETTEXT POISON #<span class="cntrl">\n</span>&lt;/pre&gt;&lt;br/&gt;

While it is not enough to say what is going wrong, it points a clear way forward how to debug this: put in debug print statements (print STDERR should print into that gitweb.log file, if the existing line is any indication), and force-push to trigger the CI build. You should probably delete azure-pipelines.yml and delete the parts in .travis.yml that do not concern themselves with macOS nor GETTEXT_POISON, and you probably want to restrict testing to the respective script by replacing the line T = $(sort $(wildcard t[0-9][0-9][0-9][0-9]-*.sh)) in t/Makefile by T = t9502-gitweb-standalone-parse-output.sh, so that you have a quicker turnaround between editing the file locally and seeing the result of the Travis build.

I could imagine, for example, that $readSize is 0 in https://github.com/git/git/pull/206/files#diff-00703a794a540acf45e225abd6aeda3bR7509, and it could also be that $cmd is strange or something. In any case, it would be interesting to see $retError verbatim (which the die() call probably feeds through the gettext machinery, so you cannot see it in the log).

And yes, I do think that this needs to be fixed. We cannot simply show a 500 when a 404 is the right status to report.

I wonder also if we could try git ls <pattern from request> to check if the answer is empty but result is(? Hopefully) a success by exit code.

And I see I haven't pushed up my latest iterations made before vacation, it seems ;)

So, a few new cans of worms - but at least the selftest passed for me locally :)

• For fallback checking if there really was no hit to some of the requested patterns, there is a git ls-remote which does not help with local workspaces, a git ls-tree that does not support shell globs (we need them because git archive does), and a git ls-files that does not really support tree-ish - only the current workspace, checked. The frequently recommended workaround for git ls-tree is to list everything and then grep what you want (not too easy for shell globs, especially with the subset git supports, but doable) and for git ls-files we can trick it with a temporary index, but enjoy "natively" the same shell-globbing mechanism as used elsewhere in git C code so this seems more future-proof.
• Using perl IPC::Run (or any other module I've found that would capture both stdout and stderr and exit-code) introduces a dependency on a module that is not commonly in perl core, nor even in default packaging footprint (at least not on both linux and macos environments for Travis CI).
  ** With this one I'm at a crossroads - can just introduce and document the dependency (and amend CI scripts), but I assume it would add some headache for distro packagers with a next release, or I can add a copy of the module to install along with gitweb.perl script - there seems to be precedent for that approach in the perl subdirectory (which I think per se is a source of Git.pm and I am not sure whether it should house the module for gitweb or not).

I am tempted to squash much of the change set into a handful of commits that lose the iterations and hassle invested, but indeed may be more subjected for review. Sort of like the original 4 IIRC commits were. Half a decade ago... :D

@jimklimov I know it has been a while, is this PR still on your radar?

Oh, fell through the cracks I guess, across several workplace and FOSS project switches. And years.

I guess the feature still makes sense (if nobody merged an equivalent yet), so I hope to take another look to tie up the loose ends.

Did any git project side requirements change significantly during this time?

IIRC one big issue was that this feature needed to differentiate causes of git program failure, but with exit codes equal and stdout/stderr forbidden for parsing (CI checked with spewer of bogus strings as logs), there was basically no good way to tackle that reliably. If these constraints are still the case, I'd welcome portable suggestions (e.g. can new exit-codes be defined?) :)

TBH I haven't paid attention to the gitweb side of things...

Well, as far as this issue is concerned, gitweb is a consumer of git CLI program which happens to require to know how exactly its call to git succeeded or failed.

Earlier this was torpedoed by lack of discerned exit code meanings and intentionally unreliable logged messages. I don't yet know if something changed during the years the PR was collecting dust - whether about these two ways (e.g. not so constraining anymore), or some others I don't know of (e.g. machine-parsable reliable output/log style?), or if some reasonable new wheel must be invented?..

Insights welcome to speed up the dive back :)