Thanks for your review. Please find fixes in reply to this email. In
order to better show individual changes I have not squashed them into
one commit. For upstream, you will probably want to squash patches 3-6
into patch 2. Patch 2 is the same as the one once queued as part of
cb/daemon-permission-errors.
[PATCH 1/6] t5550: repack everything into one file
[PATCH 2/6] daemon: add tests
[PATCH 3/6] avoid use of pkill
[PATCH 4/6] explain expected exit code
[PATCH 5/6] t5570: everything into one file
[PATCH 6/6] chmod: use lower-case x
On Tue, Jan 03, 2012 at 11:34:08AM -0800, Junio C Hamano wrote:
> >> +
> >> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}
>
> In lib-httpd.sh, LIB_HTTPD_PORT is defined in a similar way, but that is
> always overridden by the users and the convention there is to use the test
> numbers (cf. "git grep LIB_HTTPD_PORT t/"), which should be followed here
> as well.
This you already fixed in the version previously queued and is contained
in [PATCH 2/6] daemon: add tests.
> I am not very keen on the "lib-daemon.sh", GIT_TEST_DAEMON, etc. naming to
> pretend as if "git daemon" will forever be the only daemon we will ever
> ship, by the way. We might one day want to add an inotify daemon, a
> daemon for the git-pubsub protocol or somesuch.
Are you saying that the name "daemon" is too general, and it should
instead be "lib-git-daemon.sh" and GIT_TEST_GIT_DAEMON? Or do you
mean that it is not general enough and it should be called
lib-networking.sh and "GIT_TEST_NETWORKING"?
Either way, I have no preference here. Feel free to change any way you
like.
> >> + # kill git-daemon child of git
> >> + say >&3 "Stopping git daemon ..."
> >> + pkill -P "$DAEMON_PID"
>
> How portable is this one (I usually do not trust use of pkill anywhere)?
I read that it is supposed to be more portable than skill or killall.
But I have no way to research this. I have implemented a workaround
using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.
> >> + wait "$DAEMON_PID"
> >> + ret=$?
> # Please comment what 143 is on this line.
> >> + if test $ret -ne 143
Fixed in [PATCH 4/6] explain expected exit code.
> >> + git --bare repack &&
>
> As the later tests assume there will be only one pack, don't you want at
> least "-a" and possibly "-a -d" here?
Fixed in
[PATCH 1/6] t5550: repack everything into one file,
[PATCH 5/6] t5570: repack everything into one file.
> I find the use of cap X here dubious; it makes your intention unclear.
>
> Are you interested in the current status of 'x' bits on that directory, or
> are you more interested in dropping the executable/searchable bits from
> the directory no matter what its current status is (rhetorical: I fully
> expect that the answer is the latter)?
For directories, upper-case X does not have that meaning. The status is
always overwritten, irrespective of the current status. I wanted to
emphasize the fact that I am changing 'searchable' bits. But since that
does not seem to have the desired effect, I changed it to lower-case in
[PATCH 6/6] chmod: use lower-case x.
Clemens
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html