Re: [PATCH v3] Allow update hooks to update refs on their own

Steven Grimm [off-list ref] writes:

If any one of #1-5 wasn't true or was solvable in a different way,
then #6 wouldn't be needed. For example, if git-svn kept its mapping
of git revisions to svn revisions somewhere else it could leave the
commit messages untouched, meaning the SHA1s wouldn't change.

That does sound like an unfortunate design problem with how git-svn
keeps track of the correlation between two systems.

But after thinking about this issue a bit more, I think I agree that
allowing to munge what was pushed inside update hook may make sense even
outside of git-svn context (iow, if this were an ugly workaround for
git-svn deficiency then I would be unhappy but I think there are valid
use cases).  "You push A, I inspect it and may rewrite it to A' but only
if A' is reasonable -- otherwise I reject your pushing of A" could be a
valid thing to do in other contexts.  A stupid example would be an update
hook that tries to cleanse what you pushed for whitespace breakage and
commit a cleaned-up result only if the result passes the testsuite.

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., fixing whitespace errors, adding an annotation to
the commit message, noting the location of output from a profiling tool,
or committing to an svn repository using git-svn.

Signed-off-by: Steven Grimm <redacted>
---

	Since Junio's main objection to this seemed to be the protocol
	change to bypass the automatic update of the tracking ref in
	git-send-pack, that code is gone (thus reverting this to the
	same code change as the initial version!) and I added a section
	to the git-send-pack manual page describing the automatic
	tracking ref update behavior, which wasn't documented at all
	before. Someone please review my terminology there.

 Documentation/git-receive-pack.txt |    8 +++-
 Documentation/git-send-pack.txt    |   16 ++++++++
 receive-pack.c                     |   70 +++++++++++++++++++++++-------------
 3 files changed, 67 insertions(+), 27 deletions(-)

diff --git a/Documentation/git-receive-pack.txt b/Documentation/git-receive-pack.txt
index 2633d94..115ae97 100644
--- a/Documentation/git-receive-pack.txt
+++ b/Documentation/git-receive-pack.txt

@@ -74,8 +74,12 @@ Note that the hook is called before the refname is updated,
 so either sha1-old is 0\{40} (meaning there is no such ref yet),
 or it should match what is recorded in refname.
 
-The hook should exit with non-zero status if it wants to disallow
-updating the named ref.  Otherwise it should exit with zero.
+The hook may optionally choose to update the ref on its own, e.g.,
+if it needs to modify incoming revisions in some way. If it updates
+the ref, it should exit with a status of 100.  The hook should exit
+with a status between 1 and 99 if it wants to disallow updating the
+named ref.  Otherwise it should exit with zero, and the ref will be
+updated automatically.
 
 Successful execution (a zero exit status) of this hook does not
 ensure the ref will actually be updated, it is only a prerequisite.

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index a2d9cb6..db64a1b 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt

@@ -115,6 +115,22 @@ Optionally, a <ref> parameter can be prefixed with a plus '+' sign
 to disable the fast-forward check only on that ref.
 
 
+Remote Tracking Refs
+--------------------
+
+After successfully sending a pack to the remote, 'git-send-pack'
+updates the corresponding remote tracking ref in the local repository
+to point to the same commit as was just sent to the remote side. In
+most cases this eliminates the need to subsequently fetch from the
+remote repository since there would be nothing new to fetch.
+
+If the remote side's update hook modifies the incoming commit
+before applying it, the local repository's remote tracking ref will
+point at a different commit than the corresponding remote ref (since
+the local repository will not have a copy of the modified version).
+In that case an explicit fetch will be required.
+
+
 Author
 ------
 Written by Linus Torvalds <torvalds@osdl.org>

diff --git a/receive-pack.c b/receive-pack.c
index fba4cf8..ca906bf 100644
--- a/receive-pack.c
+++ b/receive-pack.c

@@ -18,6 +18,9 @@ static int report_status;
 static char capabilities[] = " report-status delete-refs ";
 static int capabilities_sent;
 
+/* Update hook exit code: hook has updated ref on its own */
+#define EXIT_CODE_REF_UPDATED 100
+
 static int receive_pack_config(const char *var, const char *value)
 {
 	if (strcmp(var, "receive.denynonfastforwards") == 0) {

@@ -70,8 +73,11 @@ static struct command *commands;
 static const char pre_receive_hook[] = "hooks/pre-receive";
 static const char post_receive_hook[] = "hooks/post-receive";
 
-static int hook_status(int code, const char *hook_name)
+static int hook_status(int code, const char *hook_name, int ok_start)
 {
+	if (ok_start && -code >= ok_start)
+		return -code;
+
 	switch (code) {
 	case 0:
 		return 0;

@@ -121,7 +127,7 @@ static int run_hook(const char *hook_name)
 
 	code = start_command(&proc);
 	if (code)
-		return hook_status(code, hook_name);
+		return hook_status(code, hook_name, 0);
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string) {
 			size_t n = snprintf(buf, sizeof(buf), "%s %s %s\n",

@@ -132,7 +138,7 @@ static int run_hook(const char *hook_name)
 				break;
 		}
 	}
-	return hook_status(finish_command(&proc), hook_name);
+	return hook_status(finish_command(&proc), hook_name, 0);
 }
 
 static int run_update_hook(struct command *cmd)

@@ -155,7 +161,8 @@ static int run_update_hook(struct command *cmd)
 	proc.no_stdin = 1;
 	proc.stdout_to_stderr = 1;
 
-	return hook_status(run_command(&proc), update_hook);
+	return hook_status(run_command(&proc), update_hook,
+			   EXIT_CODE_REF_UPDATED);
 }
 
 static const char *update(struct command *cmd)

@@ -194,32 +201,45 @@ static const char *update(struct command *cmd)
 			return "non-fast forward";
 		}
 	}
-	if (run_update_hook(cmd)) {
-		error("hook declined to update %s", name);
-		return "hook declined";
-	}
-
-	if (is_null_sha1(new_sha1)) {
-		if (!parse_object(old_sha1)) {
-			warning ("Allowing deletion of corrupt ref.");
-			old_sha1 = NULL;
+	switch (run_update_hook(cmd)) {
+	case 0:
+		if (is_null_sha1(new_sha1)) {
+			if (!parse_object(old_sha1)) {
+				warning ("Allowing deletion of corrupt ref.");
+				old_sha1 = NULL;
+			}
+			if (delete_ref(name, old_sha1)) {
+				error("failed to delete %s", name);
+				return "failed to delete";
+			}
+			fprintf(stderr, "%s: %s -> deleted\n", name,
+				sha1_to_hex(old_sha1));
 		}
-		if (delete_ref(name, old_sha1)) {
-			error("failed to delete %s", name);
-			return "failed to delete";
+		else {
+			lock = lock_any_ref_for_update(name, old_sha1, 0);
+			if (!lock) {
+				error("failed to lock %s", name);
+				return "failed to lock";
+			}
+			if (write_ref_sha1(lock, new_sha1, "push")) {
+				return "failed to write"; /* error() already called */
+			}
 		}
 		return NULL; /* good */
-	}
-	else {
-		lock = lock_any_ref_for_update(name, old_sha1, 0);
-		if (!lock) {
-			error("failed to lock %s", name);
-			return "failed to lock";
-		}
-		if (write_ref_sha1(lock, new_sha1, "push")) {
-			return "failed to write"; /* error() already called */
+
+	case EXIT_CODE_REF_UPDATED:
+		/* hook has taken care of updating ref, which means it
+		   might be a different revision than we think. */
+		if (! resolve_ref(name, new_sha1, 1, NULL)) {
+			error("can't resolve ref %s after hook updated it",
+				name);
+			return "ref not resolvable";
 		}
 		return NULL; /* good */
+
+	default:
+		error("hook declined to update %s", name);
+		return "hook declined";
 	}
 }
 

-- 
1.5.3.6.2040.g97735-dirty

On Sun, Dec 02, 2007 at 01:22:24PM -0800, Steven Grimm wrote:

	Since Junio's main objection to this seemed to be the protocol
	change to bypass the automatic update of the tracking ref in
	git-send-pack, that code is gone (thus reverting this to the
	same code change as the initial version!) and I added a section
	to the git-send-pack manual page describing the automatic
	tracking ref update behavior, which wasn't documented at all
	before. Someone please review my terminology there.

I am dubious of the usefulness of passing back the new commit id, but an
"ok, but btw I changed your commit" status from receive-pack seems like
it would be useful, for two reasons:

  - it can be displayed differently, so the user is reminded to do a
    fetch afterwards
  - we can avoid updating the tracking ref, which makes it less likely
    to result in a non-fast forward fetch next time.  For example,
    consider:

      1. The remote master and my origin/master are at A.
      2. I make a commit B on top of A.
      3. I push B to remote, who rewrites it to B' on top of A. At the
         same time, I move my origin/master to B.
      4. I fetch, and get non-ff going from B to B'.

    If I had never written anything to my origin/master, it would be a
    fast forward. And obviously git handles it just fine, but it is more
    useful to the user during the next fetch to see A..B rather than
    B'...B.

-Peff

Steven Grimm [off-list ref] wrote:

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., fixing whitespace errors, adding an annotation to
the commit message, noting the location of output from a profiling tool,
or committing to an svn repository using git-svn.

...

+/* Update hook exit code: hook has updated ref on its own */
+#define EXIT_CODE_REF_UPDATED 100

Hmm.  I would actually rather move the ref locking to before we run
the update hook, so the ref is locked *while* the hook executes.
If we ran a hook and it exited 0 then re-read the ref to see if the
value differs from old_sha1; if it does then we can assume the update
hook took care of the update.  If the value is still old_sha1 then we
know the hook didn't do the update and we need to do it for the hook.

This probably requires exporting the name of the ref we currently
have locked in an environment variable (and teach lockfile.c it)
so we can effectively do recursive locking.  That way the update
hook can still use git-update-ref to change the ref safely.

The advantage of this approach is the hook programmer doesn't need
to implement their own locking scheme and we also don't make a
special case exit code where there wasn't one before.

-- 
Shawn.

Hi,

On Sun, 2 Dec 2007, Shawn O. Pearce wrote:

Steven Grimm [off-list ref] wrote:

quoted

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., fixing whitespace errors, adding an annotation to
the commit message, noting the location of output from a profiling tool,
or committing to an svn repository using git-svn.

...

quoted

+/* Update hook exit code: hook has updated ref on its own */
+#define EXIT_CODE_REF_UPDATED 100

Hmm.  I would actually rather move the ref locking to before we run
the update hook, so the ref is locked *while* the hook executes.

Would that not mean that you cannot use update-ref to update the ref, 
since that wants to use the same lock?

Ciao,
Dscho

Johannes Schindelin [off-list ref] wrote:

On Sun, 2 Dec 2007, Shawn O. Pearce wrote:

quoted

Steven Grimm [off-list ref] wrote:

quoted

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., fixing whitespace errors, adding an annotation to
the commit message, noting the location of output from a profiling tool,
or committing to an svn repository using git-svn.

...

quoted

+/* Update hook exit code: hook has updated ref on its own */
+#define EXIT_CODE_REF_UPDATED 100

Hmm.  I would actually rather move the ref locking to before we run
the update hook, so the ref is locked *while* the hook executes.

Would that not mean that you cannot use update-ref to update the ref, 
since that wants to use the same lock?

You failed to quote the part of my email where I talked about how
we set an evironment variable to pass a hint to lockfile.c running
within the git-update-ref subprocess to instruct it to perform a
different style of locking, one that would work as a "recursive"
lock.

Such a recursive lock could be useful for a whole lot more than just
the update hook.  But it would at least allow the update hook to
use git-update-ref to safely change the ref, without receive-pack
losing its own lock on the ref.

-- 
Shawn.

Hi,

On Mon, 3 Dec 2007, Shawn O. Pearce wrote:

Johannes Schindelin [off-list ref] wrote:

quoted

On Sun, 2 Dec 2007, Shawn O. Pearce wrote:

quoted

Steven Grimm [off-list ref] wrote:

quoted

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., fixing whitespace errors, adding an annotation to
the commit message, noting the location of output from a profiling tool,
or committing to an svn repository using git-svn.

...

quoted

+/* Update hook exit code: hook has updated ref on its own */
+#define EXIT_CODE_REF_UPDATED 100

Hmm.  I would actually rather move the ref locking to before we run
the update hook, so the ref is locked *while* the hook executes.

Would that not mean that you cannot use update-ref to update the ref, 
since that wants to use the same lock?

You failed to quote the part of my email where I talked about how
we set an evironment variable to pass a hint to lockfile.c running
within the git-update-ref subprocess to instruct it to perform a
different style of locking, one that would work as a "recursive"
lock.

Such a recursive lock could be useful for a whole lot more than just
the update hook.  But it would at least allow the update hook to
use git-update-ref to safely change the ref, without receive-pack
losing its own lock on the ref.

Indeed, I even failed to read it fully ;-)

What do you propose, though?  <filename>.lock.<n>?

Ciao,
Dscho

Johannes Schindelin [off-list ref] wrote:

On Mon, 3 Dec 2007, Shawn O. Pearce wrote:

quoted

You failed to quote the part of my email where I talked about how
we set an evironment variable to pass a hint to lockfile.c running
within the git-update-ref subprocess to instruct it to perform a
different style of locking, one that would work as a "recursive"
lock.

Such a recursive lock could be useful for a whole lot more than just
the update hook.  But it would at least allow the update hook to
use git-update-ref to safely change the ref, without receive-pack
losing its own lock on the ref.

Indeed, I even failed to read it fully ;-)

What do you propose, though?  <filename>.lock.<n>?

Sure.  :-)

I was also hand-waving.  Hoping someone else would fill in the
magic details.

Actually <n> wouldn't be so bad.  We could do something like:

	GIT_INHERITED_LOCKS="<ref> <depth> <ref> <depth> ..."

where <ref> is a ref name (which cannot contain spaces, even though
some people seem to forget that rule) and <depth> is the number
of times it has been locked already.  <depth> of 0 is the current
".lock" file.  So the first lock taken out by receive-pack would
be setting:

	GIT_INHERITED_LOCKS="refs/heads/master 0"

and another lock on the same ref by a subprocess would then update
it to:

	GIT_INHERITED_LOCKS="refs/heads/master 1"

etc...

</hand-waving>

-- 
Shawn.

Hi,

On Mon, 3 Dec 2007, Shawn O. Pearce wrote:

Johannes Schindelin [off-list ref] wrote:

quoted

On Mon, 3 Dec 2007, Shawn O. Pearce wrote:

quoted

You failed to quote the part of my email where I talked about how
we set an evironment variable to pass a hint to lockfile.c running
within the git-update-ref subprocess to instruct it to perform a
different style of locking, one that would work as a "recursive"
lock.

Such a recursive lock could be useful for a whole lot more than just
the update hook.  But it would at least allow the update hook to
use git-update-ref to safely change the ref, without receive-pack
losing its own lock on the ref.

Indeed, I even failed to read it fully ;-)

What do you propose, though?  <filename>.lock.<n>?

Sure.  :-)

I was also hand-waving.  Hoping someone else would fill in the
magic details.

Actually <n> wouldn't be so bad.  We could do something like:

	GIT_INHERITED_LOCKS="<ref> <depth> <ref> <depth> ..."

I am somewhat wary of using environment variables in that context, since 
the variables could leak to subprocesses, or (even worse), they could be 
set inadvertently by the user or other scripts.

Ciao,
Dscho

On Dec 3, 2007, at 6:25 PM, Johannes Schindelin wrote:

I am somewhat wary of using environment variables in that context,  
since
the variables could leak to subprocesses, or (even worse), they  
could be
set inadvertently by the user or other scripts.

Agreed on the inadvertent setting, but isn't leaking to subprocesses  
the whole point of the exercise here?

-Steve

Johannes Schindelin [off-list ref] wrote:

On Mon, 3 Dec 2007, Shawn O. Pearce wrote:

quoted

Actually <n> wouldn't be so bad.  We could do something like:

	GIT_INHERITED_LOCKS="<ref> <depth> <ref> <depth> ..."

I am somewhat wary of using environment variables in that context, since 
the variables could leak to subprocesses, or (even worse), they could be 
set inadvertently by the user or other scripts.

Sure.  But as bad as it is, its still more secure than the
"repository of record" that my day-job uses for its source code
tree (no, it doesn't use Git, and I wish it was as good as Visual
Source Suck).  </bad-joke>

I'd suggest also using something like getppid() to check the pid
against a pid in the env, and *gasp* maybe do a SHA-1 hash in there
or something to make it challening enough to fake that the average
user won't set it unless they really understand what they are doing.

-- 
Shawn.