Re: [PATCH 2/2] receive-pack: support push-to-checkout hook

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Junio,

On Mon, 1 Dec 2014, Junio C Hamano wrote:

> When receive.denyCurrentBranch is set to updateInstead, this hook
> can be used to override the built-in "push-to-deploy" logic, which
> insists that the working tree and the index must be unchanged
> relative to HEAD.  The hook receives the commit with which the
> tip of the current is going to be updated, and is responsible to
> make any necessary changes to the working tree and to the index to
> bring them to the desired state when the tip of the current branch
> is updated to the new commit.
> 
> For example, the hook can simply run "git read-tree -u -m HEAD $1"
> to the workflow to emulate "'git fetch' going in the reverse
> direction with 'git push'" better than the push-to-deploy logic, as
> the two-tree form of "read-tree -u -m" is essentially the same as
> "git checkout" that switches branches while keeping the local
> changes in the working tree that do not interfere with the
> difference between the branches.

I like it.

The only sad part is that the already huge test suite is enlarged by yet
another extensive set of test cases (and those tests might not really
need to be that extensive because they essentially only need to make sure
that the hook is run successfully *instead* of trying to update the
working directory, i.e. a simple 'touch yep' hook would have been enough).
It starts to be painful to run the complete test suite, not only on
Windows (where this has been a multi-hour endeavor for me for ages
already). BuildHive (CloudBees' very kind offer of Jenkins CI for Open
Source, integrated conveniently with GitHub) already takes over an hour to
run the Git test suite – and BuildHive runs on Linux, not Windows!

That means that everytime I push into a GitHub Pull Request, I have to
wait for a full hour to know whether everything is groovy.

Worse: when Git for Windows contributors (yes! they exist!) push into
their Pull Requests, I have to wait for a full hour before I can merge,
unless I want to merge something that the test suite did not validate!

So maybe it is time to start thinking about conciser tests that verify the
bare minimum, especially for rarely exercised functionality? I mean,
testing is always a balance between too much and too little. And at this
point, I am afraid that several well-intended, but overly extensive tests
increase the overall runtime of "make test" to a point where developers
*avoid* running it, costing more time in the long run than necessary.

In this particular case, I think that we really, really *just* need to
verify that the presence of the hook switches off the default behavior of
updateInstead. *Nothing* else is needed to verify that this particular
functionality hasn't regressed. I.e. something like:

+test_expect_success 'updateInstead with push-to-checkout hook' '
+	rm -fr testrepo &&
+	git clone . testrepo &&
+	(
+		cd testrepo &&
+		echo unclean > path1 &&
+		git config receive.denyCurrentBranch updateInstead &&
+		echo 'touch yep' | write_script .git/hooks/push-to-checkout
+	) &&
+	git push testrepo HEAD^:refs/heads/master &&
+	test unclean = $(cat testrepo/path1) &&
+	test -f testrepo/yep
+'

would be more appropriate (although it probably has one or three bugs,
given that I wrote this in the mailer).

Ciao,
Johannes

[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]