Merge branch 'jk/quarantine-received-objects'

gitster · gitster · commit 9f1384f711e4 · 2017-04-23T22:07:52.000-07:00
Add finishing touches to a recent topic.

* jk/quarantine-received-objects:
  refs: reject ref updates while GIT_QUARANTINE_PATH is set
  receive-pack: document user-visible quarantine effects
  receive-pack: drop tmp_objdir_env from run_update_hook
diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
@@ -114,6 +114,8 @@ will be performed, and the update, post-receive and post-update
 hooks will not be invoked either.  This can be useful to quickly
 bail out if the update is not to be supported.
 
+See the notes on the quarantine environment below.
+
 update Hook
 -----------
 Before each ref is updated, if $GIT_DIR/hooks/update file exists
@@ -214,6 +216,33 @@ if the repository is packed and is served via a dumb transport.
 	exec git update-server-info
 
 
+Quarantine Environment
+----------------------
+
+When `receive-pack` takes in objects, they are placed into a temporary
+"quarantine" directory within the `$GIT_DIR/objects` directory and
+migrated into the main object store only after the `pre-receive` hook
+has completed. If the push fails before then, the temporary directory is
+removed entirely.
+
+This has a few user-visible effects and caveats:
+
+  1. Pushes which fail due to problems with the incoming pack, missing
+     objects, or due to the `pre-receive` hook will not leave any
+     on-disk data. This is usually helpful to prevent repeated failed
+     pushes from filling up your disk, but can make debugging more
+     challenging.
+
+  2. Any objects created by the `pre-receive` hook will be created in
+     the quarantine directory (and migrated only if it succeeds).
+
+  3. The `pre-receive` hook MUST NOT update any refs to point to
+     quarantined objects. Other programs accessing the repository will
+     not be able to see the objects (and if the pre-receive hook fails,
+     those refs would become corrupted). For safety, any ref updates
+     from within `pre-receive` are automatically rejected.
+
+
 SEE ALSO
 --------
 linkgit:git-send-pack[1], linkgit:gitnamespaces[7]
diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
@@ -256,6 +256,9 @@ environment variables will not be set. If the client selects
 to use push options, but doesn't transmit any, the count variable
 will be set to zero, `GIT_PUSH_OPTION_COUNT=0`.
 
+See the section on "Quarantine Environment" in
+linkgit:git-receive-pack[1] for some caveats.
+
 [[update]]
 update
 ~~~~~~
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
@@ -772,7 +772,6 @@ static int run_update_hook(struct command *cmd)
 	proc.stdout_to_stderr = 1;
 	proc.err = use_sideband ? -1 : 0;
 	proc.argv = argv;
-	proc.env = tmp_objdir_env(tmp_objdir);
 
 	code = start_command(&proc);
 	if (code)
diff --git a/refs.c b/refs.c
@@ -1643,6 +1643,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
 {
 	struct ref_store *refs = transaction->ref_store;
 
+	if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {
+		strbuf_addstr(err,
+			      _("ref updates forbidden inside quarantine environment"));
+		return -1;
+	}
+
 	return refs->be->transaction_commit(refs, transaction, err);
 }
 
diff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh
@@ -58,4 +58,15 @@ test_expect_success 'push to repo path with path separator (colon)' '
 	git push "$(pwd)/xxx${pathsep}yyy.git" HEAD
 '
 
+test_expect_success 'updating a ref from quarantine is forbidden' '
+	git init --bare update.git &&
+	write_script update.git/hooks/pre-receive <<-\EOF &&
+	read old new refname
+	git update-ref refs/heads/unrelated $new
+	exit 1
+	EOF
+	test_must_fail git push update.git HEAD &&
+	git -C update.git fsck
+'
+
 test_done

Original file line number	Diff line number	Diff line change
`@@ -1643,6 +1643,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,`
`1643`	`1643`	`{`
`1644`	`1644`	`struct ref_store *refs = transaction->ref_store;`
`1645`	`1645`
	`1646`	`+ if (getenv(GIT_QUARANTINE_ENVIRONMENT)) {`
	`1647`	`+ strbuf_addstr(err,`
	`1648`	`+ _("ref updates forbidden inside quarantine environment"));`
	`1649`	`+ return -1;`
	`1650`	`+ }`
	`1651`	`+`
`1646`	`1652`	`return refs->be->transaction_commit(refs, transaction, err);`
`1647`	`1653`	`}`
`1648`	`1654`