[PATCH] Allow update hooks to update refs on their own

[PATCH] Allow update hooks to update refs on their own

[Steven Grimm]

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., 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 <koreth@midwinter.com>
---

	I did this to support a bridge between svn and git. The idea is
	that the people who use git can do "git push" to publish their
	changes, and if they've pushed to certain branches, the changes
	will get transparently committed to svn using git-svn.

	The git users therefore can operate in a totally git-centric
	environment.  (This is a step toward transitioning my company
	away from svn entirely: get people used to a native git environment
	with no direct svn interactions.)
	
	One problem is that git-svn wants to rewrite history to add its
	git-svn-id: lines, which means that when the update hook returns
	after doing the git-svn dcommit, HEAD is already pointed at the
	updated commit.  Without this change or something like it,
	receive-pack bombs out because HEAD has moved, and the push
	appears to fail. This patch addresses that particular problem.

	There are other issues with the history rewriting, but this
	change is hopefully useful on its own for cases like the ones
	listed in the commit message.

 Documentation/git-receive-pack.txt |    8 +++-
 receive-pack.c                     |   68 +++++++++++++++++++++++-------------
 2 files changed, 50 insertions(+), 26 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/receive-pack.c b/receive-pack.c
index 38e35c0..19162ec 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 (delete_ref(name, old_sha1)) {
-			error("failed to delete %s", name);
-			return "failed to delete";
+	switch (run_update_hook(cmd)) {
+	case 0:
+		if (is_null_sha1(new_sha1)) {
+			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));
 		}
-		fprintf(stderr, "%s: %s -> deleted\n", name,
-			sha1_to_hex(old_sha1));
-		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";
+		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 */
+			}
+			fprintf(stderr, "%s: %s -> %s\n", name,
+				sha1_to_hex(old_sha1), sha1_to_hex(new_sha1));
 		}
-		if (write_ref_sha1(lock, new_sha1, "push")) {
-			return "failed to write"; /* error() already called */
+		return NULL; /* good */
+
+	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";
 		}
 		fprintf(stderr, "%s: %s -> %s\n", name,
 			sha1_to_hex(old_sha1), sha1_to_hex(new_sha1));
 		return NULL; /* good */
+
+	default:
+		error("hook declined to update %s", name);
+		return "hook declined";
 	}
 }
 
-- 
1.5.3.6.862.g7acd00-dirty

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

[Jakub Narebski]

Steven Grimm wrote:

> +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.

Why 100, and not for example 127, 128 or -1?

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

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

[Steven Grimm]

On Nov 27, 2007, at 1:21 PM, Jakub Narebski wrote:
>> +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.
>
> Why 100, and not for example 127, 128 or -1?

Because those seemed much more likely to be used by existing hook  
scripts to denote an error condition, and I wanted to reduce the  
chance that this would break existing scripts.

-Steve

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

[Junio C Hamano]

How does this interact with the "pretend to have fetched back
immediately" supported by modern git-push?

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

[Steven Grimm]

On Nov 27, 2007, at 5:19 PM, Junio C Hamano wrote:

> How does this interact with the "pretend to have fetched back
> immediately" supported by modern git-push?


That continues to fire, but it updates the local tracking ref to point  
to the SHA1 that was pushed, which isn't the actual remote ref. So you  
have to do a real fetch to get the local tracking ref pointed to the  
right place. In other words, that feature doesn't do any good in this  
context, but it doesn't really hurt anything either.

It would of course be better if git-push could notice that it needs to  
do an actual fetch. I think it'd be sufficient to transmit the final  
remote ref SHA1 back to git-push, and if it doesn't match what was  
pushed, that's a sign that a fetch is needed. But that change wouldn't  
be mutually exclusive with this patch, I believe.

-Steve

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

[Daniel Barkalow]

On Tue, 27 Nov 2007, Steven Grimm wrote:

> On Nov 27, 2007, at 5:19 PM, Junio C Hamano wrote:
> 
> >How does this interact with the "pretend to have fetched back
> >immediately" supported by modern git-push?
> 
> 
> That continues to fire, but it updates the local tracking ref to point to the
> SHA1 that was pushed, which isn't the actual remote ref. So you have to do a
> real fetch to get the local tracking ref pointed to the right place. In other
> words, that feature doesn't do any good in this context, but it doesn't really
> hurt anything either.
> 
> It would of course be better if git-push could notice that it needs to do an
> actual fetch. I think it'd be sufficient to transmit the final remote ref SHA1
> back to git-push, and if it doesn't match what was pushed, that's a sign that
> a fetch is needed. But that change wouldn't be mutually exclusive with this
> patch, I believe.

Couldn't you do this with a status message? ("ok <refname> changed by 
hook" or something.)

I disagree that the feature doesn't do any good; it records that the state 
of the remote is at least as new as the local state, so you can tell 
without a network connection that you don't have any local changes you 
haven't sent off.

	-Daniel
*This .sig left intentionally blank*

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

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Tue, 27 Nov 2007, Steven Grimm wrote:
>
>> On Nov 27, 2007, at 5:19 PM, Junio C Hamano wrote:
>> 
>> >How does this interact with the "pretend to have fetched back
>> >immediately" supported by modern git-push?
>> 
>> 
>> That continues to fire, but it updates the local tracking ref to point to the
>> SHA1 that was pushed, which isn't the actual remote ref. So you have to do a
>> real fetch to get the local tracking ref pointed to the right place. In other
>> words, that feature doesn't do any good in this context, but it doesn't really
>> hurt anything either.
>> 
>> It would of course be better if git-push could notice that it needs to do an
>> actual fetch. I think it'd be sufficient to transmit the final remote ref SHA1
>> back to git-push, and if it doesn't match what was pushed, that's a sign that
>> a fetch is needed. But that change wouldn't be mutually exclusive with this
>> patch, I believe.
>
> Couldn't you do this with a status message? ("ok <refname> changed by 
> hook" or something.)
>
> I disagree that the feature doesn't do any good; it records that the state 
> of the remote is at least as new as the local state, so you can tell 
> without a network connection that you don't have any local changes you 
> haven't sent off.

Yeah, and I am wondering why update hook needs to be changed for this.
Didn't we introduce post-receive exactly for this sort of thing?

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

[Steven Grimm]

On Nov 27, 2007, at 7:49 PM, Junio C Hamano wrote:
> Yeah, and I am wondering why update hook needs to be changed for this.
> Didn't we introduce post-receive exactly for this sort of thing?

I didn't think the post-receive hook could reject a revision. I need  
to be able to do that here, e.g., if the user's change fails to commit  
because it's rejected by an svn commit hook. (Hooks upon hooks upon  
hooks...) If I do the svn commit in post-receive it's not clear how  
the user would repush the change after fixing it -- their fix would  
show up as a delta on top of a revision that I can't commit on its own  
to svn, so I would somehow have to know to do a squash merge and there  
would no longer be a one-to-one correspondence between git and svn  
revisions. That's not a total showstopper (I'm rewriting history  
anyway) but it sure seems like it'll be confusing and error-prone.

Also, running in post-receive will subject me to race conditions that  
aren't present in the update hook case; if I make my update hook  
script do its own locking and update the refs on its own, I'm  
guaranteed no other push will come along and update my ref out from  
under me. In post-receive there is no such guarantee and I may end up  
sending two pushes' worth of commits to svn when I think I'm only  
sending one.

If I'm misunderstanding the flow of control, please feel free to  
correct me. It just seemed like update was the only good place to do  
what I needed to do.

-Steve

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

[Jeff King]

On Tue, Nov 27, 2007 at 10:25:32PM -0500, Daniel Barkalow wrote:

> > It would of course be better if git-push could notice that it needs
> > to do an actual fetch. I think it'd be sufficient to transmit the
> > final remote ref SHA1 back to git-push, and if it doesn't match what
> > was pushed, that's a sign that a fetch is needed. But that change
> > wouldn't be mutually exclusive with this patch, I believe.
> 
> Couldn't you do this with a status message? ("ok <refname> changed by 
> hook" or something.)

Having just touched this code, I believe the answer is yes. receive-pack
has always sent just "ok <refname>\n", so we could start interpreting
anything after the <refname> bit freely (I think "ok <refname>
changed-to <hash>" is even more informative, but perhaps not useful
given that the sender probably doesn't have that commit object).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> Couldn't you do this with a status message? ("ok <refname> changed by 
>> hook" or something.)
>
> Having just touched this code, I believe the answer is yes. receive-pack
> has always sent just "ok <refname>\n", so we could start interpreting
> anything after the <refname> bit freely...

More importantly, no send-pack choked on "ok <refname>" followed by a SP
(old ones just checked "ok" and nothing else, the current one NULs out
the SP and lets you use the rest as a message), so such a change to
receive-pack does not have to break older send-pack.

[PATCH] Allow update hooks to update refs on their own

[Steven Grimm]

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., 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.

recieve-pack will now send the post-update SHA1 back to send-pack. If
it's different than the one that was pushed, git-push won't do the
fake update of the local tracking ref and will instead inform the user
that the tracking ref needs to be fetched.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

	The protocol was much easier to tweak than I expected. It
	seems like a reasonable extension to always send back the
	resulting SHA1; won't stop other people from adding more
	information later.

	It would IMO be better still if git-push were to fetch
	automatically here rather than just printing a message, but
	I'm not sure if that would smack too much of automagic behavior
	to people. Comments welcome.

 Documentation/git-receive-pack.txt |    8 +++-
 receive-pack.c                     |   72 +++++++++++++++++++++++-------------
 remote.c                           |    2 +-
 remote.h                           |    2 +
 send-pack.c                        |   64 +++++++++++++++++++++++++++-----
 5 files changed, 109 insertions(+), 39 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/receive-pack.c b/receive-pack.c
index 38e35c0..d07dd6d 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 (delete_ref(name, old_sha1)) {
-			error("failed to delete %s", name);
-			return "failed to delete";
+	switch (run_update_hook(cmd)) {
+	case 0:
+		if (is_null_sha1(new_sha1)) {
+			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));
 		}
-		fprintf(stderr, "%s: %s -> deleted\n", name,
-			sha1_to_hex(old_sha1));
-		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";
+		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 */
+			}
+			fprintf(stderr, "%s: %s -> %s\n", name,
+				sha1_to_hex(old_sha1), sha1_to_hex(new_sha1));
 		}
-		if (write_ref_sha1(lock, new_sha1, "push")) {
-			return "failed to write"; /* error() already called */
+		return NULL; /* good */
+
+	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";
 		}
 		fprintf(stderr, "%s: %s -> %s\n", name,
 			sha1_to_hex(old_sha1), sha1_to_hex(new_sha1));
 		return NULL; /* good */
+
+	default:
+		error("hook declined to update %s", name);
+		return "hook declined";
 	}
 }
 
@@ -419,8 +439,8 @@ static void report(const char *unpack_status)
 		     unpack_status ? unpack_status : "ok");
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string)
-			packet_write(1, "ok %s\n",
-				     cmd->ref_name);
+			packet_write(1, "ok %s %s\n",
+				     cmd->ref_name, sha1_to_hex(cmd->new_sha1));
 		else
 			packet_write(1, "ng %s %s\n",
 				     cmd->ref_name, cmd->error_string);
diff --git a/remote.c b/remote.c
index bec2ba1..07558c1 100644
--- a/remote.c
+++ b/remote.c
@@ -684,7 +684,7 @@ static int match_explicit_refs(struct ref *src, struct ref *dst,
 	return -errs;
 }
 
-static struct ref *find_ref_by_name(struct ref *list, const char *name)
+struct ref *find_ref_by_name(struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)
 		if (!strcmp(list->name, name))
diff --git a/remote.h b/remote.h
index 878b4ec..e822f3c 100644
--- a/remote.h
+++ b/remote.h
@@ -56,6 +56,8 @@ void ref_remove_duplicates(struct ref *ref_map);
 
 struct refspec *parse_ref_spec(int nr_refspec, const char **refspec);
 
+struct ref *find_ref_by_name(struct ref *list, const char *name);
+
 int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,
 	       int nr_refspec, char **refspec, int all);
 
diff --git a/send-pack.c b/send-pack.c
index b74fd45..bf564ae 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -147,9 +147,31 @@ static void get_local_heads(void)
 	for_each_ref(one_local_ref, NULL);
 }
 
-static int receive_status(int in)
+/* Verifies that a remote ref matches the revision we pushed. */
+static void verify_tracking_ref(const char *refname, const char *newsha1_hex, struct ref *remote_refs)
+{
+	struct ref *ref;
+	unsigned char newsha1[20];
+	if (get_sha1_hex(newsha1_hex, newsha1)) {
+		fprintf(stderr, "protocol error: bad sha1 %s\n", newsha1_hex);
+		return;
+	}
+	ref = find_ref_by_name(remote_refs, refname);
+	if (ref != NULL) {
+		if (hashcmp(ref->new_sha1, newsha1)) {
+			hashcpy(ref->new_sha1, newsha1);
+			ref->force = 1;
+		}
+		else {
+			ref->force = 0;
+		}
+	}
+}
+
+static int receive_status(int in, struct ref *remote_refs)
 {
 	char line[1000];
+	char *sha1;
 	int ret = 0;
 	int len = packet_read_line(in, line, sizeof(line));
 	if (len < 10 || memcmp(line, "unpack ", 7)) {
@@ -170,8 +192,22 @@ static int receive_status(int in)
 			ret = -1;
 			break;
 		}
-		if (!memcmp(line, "ok", 2))
+		if (!memcmp(line, "ok", 2)) {
+			/* If the result looks like "ok refname sha1", we can
+			 * compare the actual SHA1 for the remote ref with the
+			 * one we pushed; if they differ then the remote
+			 * may have rewritten our commits and we will need to
+			 * do a real fetch rather than just updating the
+			 * tracking refs.
+			 */
+			if (line[2] == ' ' &&
+			    (sha1 = strchr(line+3, ' '))) {
+				/* null-terminate refname */
+				*sha1++ = '\0';
+				verify_tracking_ref(line+3, sha1, remote_refs);
+			}
 			continue;
+		}
 		fputs(line, stderr);
 		ret = -1;
 	}
@@ -196,13 +232,21 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 		return;
 
 	if (!remote_find_tracking(remote, &rs)) {
-		fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (is_null_sha1(ref->peer_ref->new_sha1)) {
-			if (delete_ref(rs.dst, NULL))
-				error("Failed to delete");
-		} else
-			update_ref("update by push", rs.dst,
-					ref->new_sha1, NULL, 0, 0);
+		if (ref->force) {
+			fprintf(stderr,
+				"local tracking ref '%s' needs to be fetched\n",
+				rs.dst);
+		}
+		else {
+			fprintf(stderr, "updating local tracking ref '%s'\n",
+				rs.dst);
+			if (is_null_sha1(ref->peer_ref->new_sha1)) {
+				if (delete_ref(rs.dst, NULL))
+					error("Failed to delete");
+			} else
+				update_ref("update by push", rs.dst,
+						ref->new_sha1, NULL, 0, 0);
+		}
 		free(rs.dst);
 	}
 }
@@ -343,7 +387,7 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 	close(out);
 
 	if (expect_status_report) {
-		if (receive_status(in))
+		if (receive_status(in, remote_refs))
 			ret = -4;
 	}
 
-- 
1.5.3.6.862.gb50ba-dirty

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

[Jeff King]

On Wed, Nov 28, 2007 at 11:41:59AM -0800, Steven Grimm wrote:

> recieve-pack will now send the post-update SHA1 back to send-pack. If
> it's different than the one that was pushed, git-push won't do the
> fake update of the local tracking ref and will instead inform the user
> that the tracking ref needs to be fetched.

Hrm, this is going to have nasty conflicts with 'next', which already
does the remote ref matching. I think the best way to implement this
would probably be on top of the jk/send-pack topic in next, and add a
new REF_STATUS_REMOTE_CHANGED status type.

-Peff

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

[Steven Grimm]

On Nov 28, 2007, at 11:49 AM, Jeff King wrote:

> Hrm, this is going to have nasty conflicts with 'next', which already
> does the remote ref matching. I think the best way to implement this
> would probably be on top of the jk/send-pack topic in next, and add a
> new REF_STATUS_REMOTE_CHANGED status type.

Ah, yes, I see what you mean. Okay, please ignore my latest patch -- I  
will redo it on top of "next" later today/tonight.

Well, actually, I would still like opinions on one thing: What do  
people think of having git-push do a fetch if the remote side changes  
a ref to point to a revision that doesn't exist locally? Is there a  
situation where you'd ever want to *not* do that?

-Steve

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

[Jeff King]

On Wed, Nov 28, 2007 at 12:16:27PM -0800, Steven Grimm wrote:

> Well, actually, I would still like opinions on one thing: What do people 
> think of having git-push do a fetch if the remote side changes a ref to 
> point to a revision that doesn't exist locally? Is there a situation where 
> you'd ever want to *not* do that?

It can be slow, since you have to make another connection to the server,
so clearly it should only be done when you detect an update (which I
think is what you're proposing).

A raw "git-fetch" might pull a lot of extra cruft that you didn't want
to get right now. So if you did do it, I think it would make sense to
construct a set of refspecs that match only the ones which need pulling
(i.e., in update_tracking_ref, rather than doing the update, construct a
refspec of "local:tracking", and then hand all such refspecs to
git-fetch).

-Peff

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

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> Well, actually, I would still like opinions on one thing: What do
> people think of having git-push do a fetch if the remote side changes
> a ref to point to a revision that doesn't exist locally?

I do not like it at all.  git-push is about pushing what you did to the
other end; you may or may not want to refetch what other people
immediately on top of what you did, and this includes what the hook
script did in the repository you have pushed into.  If you want an
immediate refetch, you can script it so that you call push and then
fetch; the latter needs to be a separate connection to a different
service anyway.

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

[Steven Grimm]

This is useful in cases where a hook needs to modify an incoming commit
in some way, e.g., 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.

recieve-pack will now send the post-update SHA1 back to send-pack. If
it's different than the one that was pushed, git-push won't do the
fake update of the local tracking ref and will instead inform the user
that the tracking ref needs to be fetched.

Signed-off-by: Steven Grimm <koreth@midwinter.com>
---

	This is on top of "next". It doesn't do any automatic fetching,
	just prints a status message.

 Documentation/git-receive-pack.txt |    8 +++-
 builtin-send-pack.c                |   65 +++++++++++++++++++++++++++-------
 cache.h                            |    1 +
 receive-pack.c                     |   68 +++++++++++++++++++++++-------------
 4 files changed, 103 insertions(+), 39 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/builtin-send-pack.c b/builtin-send-pack.c
index 25ae1fe..4ba716a 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -164,7 +164,9 @@ static int receive_status(int in, struct ref *refs)
 	hint = NULL;
 	while (1) {
 		char *refname;
+		char *newsha1_hex;
 		char *msg;
+		unsigned char newsha1[20];
 		len = packet_read_line(in, line, sizeof(line));
 		if (!len)
 			break;
@@ -177,7 +179,16 @@ static int receive_status(int in, struct ref *refs)
 
 		line[strlen(line)-1] = '\0';
 		refname = line + 3;
-		msg = strchr(refname, ' ');
+		newsha1_hex = strchr(refname, ' ');
+		if (newsha1_hex) {
+			*newsha1_hex++ = '\0';
+			if (get_sha1_hex(newsha1_hex, newsha1)) {
+				fprintf(stderr, "protocol error: bad sha1 %s\n",
+					newsha1_hex);
+				newsha1_hex = NULL;
+			}
+		}
+		msg = strchr(newsha1_hex, ' ');
 		if (msg)
 			*msg++ = '\0';
 
@@ -197,8 +208,16 @@ static int receive_status(int in, struct ref *refs)
 			continue;
 		}
 
-		if (line[0] == 'o' && line[1] == 'k')
-			hint->status = REF_STATUS_OK;
+		if (line[0] == 'o' && line[1] == 'k') {
+			if (newsha1_hex != NULL &&
+			    hashcmp(hint->new_sha1, newsha1)) {
+				hint->status = REF_STATUS_REMOTE_CHANGED;
+				hashcpy(hint->new_sha1, newsha1);
+			}
+			else {
+				hint->status = REF_STATUS_OK;
+			}
+		}
 		else {
 			hint->status = REF_STATUS_REMOTE_REJECT;
 			ret = -1;
@@ -215,21 +234,34 @@ static void update_tracking_ref(struct remote *remote, struct ref *ref)
 {
 	struct refspec rs;
 
-	if (ref->status != REF_STATUS_OK)
+	if (ref->status != REF_STATUS_OK &&
+	    ref->status != REF_STATUS_REMOTE_CHANGED)
 		return;
 
 	rs.src = ref->name;
 	rs.dst = NULL;
 
 	if (!remote_find_tracking(remote, &rs)) {
-		if (args.verbose)
-			fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
-		if (ref->deletion) {
-			if (delete_ref(rs.dst, NULL))
-				error("Failed to delete");
-		} else
-			update_ref("update by push", rs.dst,
-					ref->new_sha1, NULL, 0, 0);
+		if (ref->status == REF_STATUS_REMOTE_CHANGED) {
+			if (args.verbose) {
+				fprintf(stderr, "local tracking ref '%s' "
+					"needs to be fetched\n", rs.dst);
+			}
+		}
+		else {
+			if (args.verbose) {
+				fprintf(stderr, "updating local tracking "
+					"ref '%s'\n", rs.dst);
+			}
+			if (ref->deletion) {
+				if (delete_ref(rs.dst, NULL))
+					error("Failed to delete");
+			}
+			else {
+				update_ref("update by push", rs.dst,
+						ref->new_sha1, NULL, 0, 0);
+			}
+		}
 		free(rs.dst);
 	}
 }
@@ -329,6 +361,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count)
 				ref->deletion ? NULL : ref->peer_ref,
 				"remote failed to report status");
 		break;
+	case REF_STATUS_REMOTE_CHANGED:
+		print_ref_status('+', "[changed]", ref, ref->peer_ref,
+				"remote made changes to revisions");
+		break;
 	case REF_STATUS_OK:
 		print_ok_ref_status(ref);
 		break;
@@ -349,12 +385,14 @@ static void print_push_status(const char *dest, struct ref *refs)
 	}
 
 	for (ref = refs; ref; ref = ref->next)
-		if (ref->status == REF_STATUS_OK)
+		if (ref->status == REF_STATUS_OK ||
+		    ref->status == REF_STATUS_REMOTE_CHANGED)
 			n += print_one_push_status(ref, dest, n);
 
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
+		    ref->status != REF_STATUS_REMOTE_CHANGED &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n);
 	}
@@ -522,6 +560,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		switch (ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
+		case REF_STATUS_REMOTE_CHANGED:
 		case REF_STATUS_OK:
 			break;
 		default:
diff --git a/cache.h b/cache.h
index 4e59646..ae2427d 100644
--- a/cache.h
+++ b/cache.h
@@ -516,6 +516,7 @@ struct ref {
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT,
+		REF_STATUS_REMOTE_CHANGED,
 	} status;
 	char *remote_status;
 	struct ref *peer_ref; /* when renaming */
diff --git a/receive-pack.c b/receive-pack.c
index ed44b89..1101e18 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,28 +201,41 @@ 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 (delete_ref(name, old_sha1)) {
-			error("failed to delete %s", name);
-			return "failed to delete";
+	switch (run_update_hook(cmd)) {
+	case 0:
+		if (is_null_sha1(new_sha1)) {
+			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));
 		}
-		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";
+		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 */
+			}
 		}
-		if (write_ref_sha1(lock, new_sha1, "push")) {
-			return "failed to write"; /* error() already called */
+		return NULL; /* good */
+
+	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";
 	}
 }
 
@@ -415,8 +435,8 @@ static void report(const char *unpack_status)
 		     unpack_status ? unpack_status : "ok");
 	for (cmd = commands; cmd; cmd = cmd->next) {
 		if (!cmd->error_string)
-			packet_write(1, "ok %s\n",
-				     cmd->ref_name);
+			packet_write(1, "ok %s %s\n",
+				     cmd->ref_name, sha1_to_hex(cmd->new_sha1));
 		else
 			packet_write(1, "ng %s %s\n",
 				     cmd->ref_name, cmd->error_string);
-- 
1.5.3.6.2040.g97735-dirty

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

[Jeff King]

On Wed, Nov 28, 2007 at 02:14:03PM -0800, Steven Grimm wrote:

> @@ -177,7 +179,16 @@ static int receive_status(int in, struct ref *refs)
>  
>  		line[strlen(line)-1] = '\0';
>  		refname = line + 3;
> -		msg = strchr(refname, ' ');
> +		newsha1_hex = strchr(refname, ' ');
> +		if (newsha1_hex) {
> +			*newsha1_hex++ = '\0';
> +			if (get_sha1_hex(newsha1_hex, newsha1)) {
> +				fprintf(stderr, "protocol error: bad sha1 %s\n",
> +					newsha1_hex);
> +				newsha1_hex = NULL;
> +			}
> +		}
> +		msg = strchr(newsha1_hex, ' ');
>  		if (msg)
>  			*msg++ = '\0';

Doesn't this always put the first "word" of a response into newsha1_hex?
We want to do this only for 'ok' responses; 'ng' responses are already
using that space as part of the error message.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Nov 28, 2007 at 02:14:03PM -0800, Steven Grimm wrote:
>
>> @@ -177,7 +179,16 @@ static int receive_status(int in, struct ref *refs)
>>  
>>  		line[strlen(line)-1] = '\0';
>>  		refname = line + 3;
>> -		msg = strchr(refname, ' ');
>> +		newsha1_hex = strchr(refname, ' ');
>> +		if (newsha1_hex) {
>> +			*newsha1_hex++ = '\0';
>> +			if (get_sha1_hex(newsha1_hex, newsha1)) {
>> +				fprintf(stderr, "protocol error: bad sha1 %s\n",
>> +					newsha1_hex);
>> +				newsha1_hex = NULL;
>> +			}
>> +		}
>> +		msg = strchr(newsha1_hex, ' ');
>>  		if (msg)
>>  			*msg++ = '\0';
>
> Doesn't this always put the first "word" of a response into newsha1_hex?
> We want to do this only for 'ok' responses; 'ng' responses are already
> using that space as part of the error message.

I do not think reporting back the rewritten object name makes much sense
nor adds any value; it won't be useful information until you fetch the
object.

I do not think reporting back _anything_ other than "ok" adds much value
at all.  Sure, if the update hook did something funky you would get such
a report, but the situation is not any different if some warm body is
sitting on the other end and building on top of what you pushed
immediately he sees any push into the repository, and in such a case
your git-push would not get any such reporting anyway.

We do not even have to worry about this reporting at all if we do not
allow munging the refs in the update hook.  In a sense, this patch is
creating a problem that does not need to be solved.  Perhaps modifying
update hook to allow so makes it possible to munge refs while holding a
lock, but is it really worth this hassle?  Isn't there a better way, I
wonder?

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

[Steven Grimm]

On Nov 28, 2007, at 3:42 PM, Junio C Hamano wrote:
> I do not think reporting back the rewritten object name makes much  
> sense
> nor adds any value; it won't be useful information until you fetch the
> object.

Right, this was mostly in anticipation of doing an automatic fetch, so  
that I would avoid fetching anything but the rewritten revisions; if I  
just fetched the remote ref as normal, then I'd potentially pick up  
unrelated changes that happened to hit just after my pack was  
accepted, which wouldn't maintain the "update the tracking ref to  
point to what I just pushed" semantics.

Since it sounds like that's a nonstarter, I agree this part of the  
patch isn't useful.

> I do not think reporting back _anything_ other than "ok" adds much  
> value
> at all.  Sure, if the update hook did something funky you would get  
> such
> a report, but the situation is not any different if some warm body is
> sitting on the other end and building on top of what you pushed
> immediately he sees any push into the repository, and in such a case
> your git-push would not get any such reporting anyway.

I disagree that it's the same. In this case the updated ref happens as  
a component of the push operation (which of course includes running  
update hooks and at the very least looking at their exit codes to see  
if a change should be rejected), not as a result of some other process  
that happens to occur at nearly the same time. Reporting back the new  
ref, at the very least, tells you that it's not useful to update the  
tracking ref since it's 100% guaranteed to be wrong by the time the  
push finishes.

> We do not even have to worry about this reporting at all if we do not
> allow munging the refs in the update hook.  In a sense, this patch is
> creating a problem that does not need to be solved.  Perhaps modifying
> update hook to allow so makes it possible to munge refs while  
> holding a
> lock, but is it really worth this hassle?  Isn't there a better way, I
> wonder?

If there is, I'm happy to hear it; for me this patch is a means, not  
an end. What I actually want is to be able to have a particular set of  
branches in a particular git repository be as-transparent-as-possible  
conduits to corresponding branches in an svn repository.

I arrived at this approach by following this train of thought:

1. The update hook is the only hook that allows me to reject the push,  
which I need to do if svn refuses to accept the change.
2. To tell whether svn accepts a change, I need to run git-svn  
dcommit; thanks to #1, I need to do that from inside the update hook.
3. When I commit, git-svn needs to track that the git revision now  
corresponds to an svn revision. It does that by modifying the commit  
message to add its git-svn-id: line.
4. Modifying the commit comment causes the revision's SHA1 to change.
5. Out-of-the-box push thinks a push has failed if the ref's SHA1  
changes in the update hook.
6. Therefore push needs to be modified to not do that.

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.

-Steve

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

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> 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.

[PATCH v4] Allow update hooks to update refs on their own.

[Steven Grimm]

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 <koreth@midwinter.com>
---

	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

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

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> +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.

This makes one wonder what happens if it returns 101, iow if there is
difference between returning 99 and 101, and if so why.

> +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.

Micronit.  The above is all "if exists".  Not everybody pushes to
somewhere he uses tracking with.

> @@ -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;
> +

I've always been puzzled by this "ok_start" parameter from the very
beginning edition of your patch.  It is not like "if this is true, then
it is ok to run the hook but otherwise do not run".  In layman's terms,
what does the parameter mean?

Maybe the variable is misnamed and not expressing the concept well
enough.  Maybe the concept itself is muddy and hard to understand.  I
cannot tell.

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ..., 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.

Sensible argument.  I stand corrected.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> ..., 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.
>
> Sensible argument.  I stand corrected.

Having said that, I think the workflow that this "letting update hook
munge" patch supports has a bit more implications for the people who
interact with it.  The pusher will need to force fetch the result, but
after that he needs to discard his own commit and replace it with
whatever the hook did.  The question is how much to discard, and how to
reconstruct the changes since the last push that was made on top of the
commit that was pushed.

Maybe the push pushed out a string of five pearls and the hook may have
rewritten only the tip, or all of them.  You may have built a few
commits on top since then.  If what you pushed out contained merges and
the hook rewrote it, you would need to potentially replay such a merge
that was rewritten by the hook.

This is all the same with a workflow that deals with a branch that is
advertised to constantly rewound and rebased, and the common approach is
to use "fetch + rebase" (see recent discussion between Nico and Bruce).
So this patch is not creating a new problem (iow, I am not mentioning
this as the reason to reject this patch), but I thought I should bring
it up so that people know what they are doing.

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

[Steven Grimm]

On Dec 2, 2007, at 6:16 PM, Junio C Hamano wrote:
>> ..., but an
>> "ok, but btw I changed your commit" status from receive-pack seems  
>> like
>> it would be useful, for two reasons:
> Sensible argument.  I stand corrected.

If we want that status in principle, I'd argue that sending down the  
updated commit SHA1 is actually the right way to indicate it, because  
it gives the client all the information it needs to make an  
intelligent choice about what to do next. If you don't transmit the  
modified SHA1, the client will have to do another fetch to find out  
what rewriting was done by the server, and if another push happened in  
the meantime, the client will have to basically guess about which  
commits correspond to the ones it pushed.

I'm going to have to modify the "ok" line for this either way, and  
it's not like the extra 39 bytes (for sending a hex SHA1 instead of a  
one-character status indicator) is going to hurt in any but the  
narrowest corner cases.

But if people really object to that, I will add a simple flag to the  
"ok" line.

-Steve

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

[Junio C Hamano]

Steven Grimm <koreth@midwinter.com> writes:

> On Dec 2, 2007, at 6:16 PM, Junio C Hamano wrote:
>>> ..., but an
>>> "ok, but btw I changed your commit" status from receive-pack seems
>>> like
>>> it would be useful, for two reasons:
>> Sensible argument.  I stand corrected.
>
> If we want that status in principle, I'd argue that sending down the
> updated commit SHA1 is actually the right way to indicate it, because
> it gives the client all the information it needs to make an
> intelligent choice about what to do next. If you don't transmit the
> modified SHA1, the client will have to do another fetch to find out
> what rewriting was done by the server, and if another push happened in
> the meantime, the client will have to basically guess about which
> commits correspond to the ones it pushed.

Ok, but the output from fetch is meant to be human readable and we do
not promise parsability, so if we go this route (which I think you made
a sensible argument for) we would need a hook on the pushing end to act
on this (perhaps record the correspondence of pushed and rewritten sha1
somewhere for the hook's own use).

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ok, but the output from fetch is meant to be human readable and we do
> not promise parsability, so if we go this route (which I think you made

s/parsability/machine &/;

> a sensible argument for) we would need a hook on the pushing end to act
> on this (perhaps record the correspondence of pushed and rewritten sha1
> somewhere for the hook's own use).

s/on this/& information/;
s/own use/& in machine readable way/;

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

[Jeff King]

On Wed, Dec 05, 2007 at 02:19:58PM -0800, Junio C Hamano wrote:

> > what rewriting was done by the server, and if another push happened in
> > the meantime, the client will have to basically guess about which
> > commits correspond to the ones it pushed.
> 
> Ok, but the output from fetch is meant to be human readable and we do
> not promise parsability, so if we go this route (which I think you made
> a sensible argument for) we would need a hook on the pushing end to act
> on this (perhaps record the correspondence of pushed and rewritten sha1
> somewhere for the hook's own use).

I am not clear on what you mean. Are you saying that the send-pack code
should _not_ recognize the "ok, but I rewrote your commit" status?
Because that is how we will avoid updating the tracking ref, which I
think is a good goal.

Or are you saying "it's ok to understand the 'ok, but...' response and
not update the tracking ref, but pulling the new hash from the message
is up to a hook on the pushing side"? Which I think it reasonable.

Or alternatively, "there should be a hook on the pushing side which is
allowed to set the ref status to 'ok, but don't bother updating the
tracking ref' or 'ok, but here is the actual thing to put in the
tracking ref'"? Which is also fine by me.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Dec 05, 2007 at 02:19:58PM -0800, Junio C Hamano wrote:
>
>> > what rewriting was done by the server, and if another push happened in
>> > the meantime, the client will have to basically guess about which
>> > commits correspond to the ones it pushed.
>> 
>> Ok, but the output from fetch is meant to be human readable and we do
>> not promise parsability, so if we go this route (which I think you made
>> a sensible argument for) we would need a hook on the pushing end to act
>> on this (perhaps record the correspondence of pushed and rewritten sha1
>> somewhere for the hook's own use).
>
> I am not clear on what you mean. Are you saying that the send-pack code
> should _not_ recognize the "ok, but I rewrote your commit" status?
> Because that is how we will avoid updating the tracking ref, which I
> think is a good goal.
>
> Or are you saying "it's ok to understand the 'ok, but...' response and
> not update the tracking ref, but pulling the new hash from the message
> is up to a hook on the pushing side"? Which I think it reasonable.
>
> Or alternatively, "there should be a hook on the pushing side which is
> allowed to set the ref status to 'ok, but don't bother updating the
> tracking ref' or 'ok, but here is the actual thing to put in the
> tracking ref'"? Which is also fine by me.

What I meant in response to what I thought Steven was talking about was
this.

 * With Steven's patch, the sending side needs to expect what it pushes
   to be rewritten.  If it starts with this history:

    ---o---o---o---Y---o---o---X

   where Y is what its remote tracking branch points at for the
   corresponding branch, three commits were built locally since the last
   fetch, and the sender pushes X.

 * Then the receiving end rewrites the history, making the history into
   this:

		     o'--o'--X'
                    /
    ---o---o---o---Y---o---o---X

 * Before the next fetch, the sending side can continue building on top
   of X, leading to this:

		     o'--o'--X'
                    /
    ---o---o---o---Y---o---o---X---o---Z

 * Similarly other people push into the same remote, get their commits
   rewritten and remote side's history becomes like this (but the
   original sender does not know about the upper history at all yet).

		     o'--o'--X'--o'--o'--W'
                    /
    ---o---o---o---Y---o---o---X---o---Z

 * Then the original sender fetches from the remote, now the tracking
   branch points at W' (it previously pointed at Y).  You would want to
   rebase your work since the last push on top of that tracking branch.

The rebase would be "rebase --onto W' X Z", so it is not strictly
necessary to keep the fact that X corresponds to X', but somehow I
thought it was necessary, and Steven's message was hinting about that:

  > If we want that status in principle, I'd argue that sending down the
  > updated commit SHA1 is actually the right way to indicate it, because
  > it gives the client all the information it needs to make an
  > intelligent choice about what to do next. If you don't transmit the
  > modified SHA1, the client will have to do another fetch to find out
  > what rewriting was done by the server, and if another push happened in
  > the meantime, the client will have to basically guess about which
  > commits correspond to the ones it pushed.

(notice the last part).

So if we want to transmit minimum amount of information, we can just
send a bit ("the ref was rewritten") back to send-pack without telling
it what X' is (but it would not hurt to send it back either).  With that
one bit of information, send-pack can refrain from updating tracking ref
from Y to X.

In the above scenario I illustrated, it turns out that getting
correspondence between X and X' is not strictly necessary to perform a
rebase later, but maybe there is some other scenario that keeping track
of that information would be helpful.  In such a case, a hook on the
send-pack end (which currently we do not have) can be called with X and
X' as parameters (and perhaps the name of the ref and the corresponding
tracking ref) to do whatever it wants to do with that information.

Even if we do not send X' back but just one bit, having that hook would
probably be needed so that sender can record "I've pushed up to X" and
perhaps "now I cannot push out Z until I rebase" after receiving "push
was accepted but rewritten" bit.

This is all handwaving --- I suspect for this to really work, send-pack
might need a pre-send-pack hook that pays attention to such "now I
cannot push out Z until I rebase" information the previous round of push
may have left and declines to push.  Of course, the receiving end would
would probably refuse such a push because it is not a fast-forward.

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

[Jeff King]

On Wed, Dec 05, 2007 at 10:30:45PM -0800, Junio C Hamano wrote:

> The rebase would be "rebase --onto W' X Z", so it is not strictly
> necessary to keep the fact that X corresponds to X', but somehow I
> thought it was necessary, and Steven's message was hinting about that:
> 
>   > If we want that status in principle, I'd argue that sending down the
>   > updated commit SHA1 is actually the right way to indicate it, because
>   > it gives the client all the information it needs to make an
>   > intelligent choice about what to do next. If you don't transmit the
>   > modified SHA1, the client will have to do another fetch to find out
>   > what rewriting was done by the server, and if another push happened in
>   > the meantime, the client will have to basically guess about which
>   > commits correspond to the ones it pushed.
> 
> (notice the last part).
> 
> So if we want to transmit minimum amount of information, we can just
> send a bit ("the ref was rewritten") back to send-pack without telling
> it what X' is (but it would not hurt to send it back either).  With that
> one bit of information, send-pack can refrain from updating tracking ref
> from Y to X.

Ah, I thought his argument was "we have to send back a bit, so why not
just send the hash we made for informational purposes? It doesn't hurt,
and maybe we can make use of it later."

I was assuming that we were interested _only_ in fixing the send-pack
issues at this time, and that the rebasing or merging part of the
workflow would be figured out later. But it is probably sensible to
consider the whole workflow to see what is necessary at each step.

-Peff

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

[Steven Grimm]

On Dec 5, 2007, at 10:36 PM, Jeff King wrote:
> Ah, I thought his argument was "we have to send back a bit, so why not
> just send the hash we made for informational purposes? It doesn't  
> hurt,
> and maybe we can make use of it later."

Yeah, that was more or less my thinking. Keep it simple for now, but  
it seems like that information is bound to be useful at some point. In  
particular, if you don't send it down, it's really difficult to  
unambiguously get back after the fact (given that a fetch might  
contain subsequent revisions unrelated to yours.)

My v3 patch (which I will combine with a modified form of the  
documentation update now that it sounds like transmitting the SHA1  
isn't objectionable) actually sent it down twice: once in the protocol  
message and once in the human-readable push status report.

-Steve

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

[Shawn O. Pearce]

Steven Grimm <koreth@midwinter.com> 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.

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

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> 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.

Heh, I like that, although I suspect getting this right would mean the
topic should be post 1.5.4 (which I do not mind).  

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

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > 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.
> 
> Heh, I like that, although I suspect getting this right would mean the
> topic should be post 1.5.4 (which I do not mind).  

Yea, most likely.

Also I won't have any time in the near future to work on this
implementation myself.  I threw the idea out there in case someone
else can find the time.  I'm willing to do the work myself as I think
its the right approach to use here for this update hook change, but
I just don't see myself getting to it anytime before say Christmas...

Its slightly more involved then what Steven originally proposed,
but I think it solves the problem better and gives us more room
for future improvements where we may want/need something such as
a recursive ref locking.

-- 
Shawn.

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

[Johannes Schindelin]

Hi,

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

> Steven Grimm <koreth@midwinter.com> 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.

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

Ciao,
Dscho

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

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Sun, 2 Dec 2007, Shawn O. Pearce wrote:
> 
> > Steven Grimm <koreth@midwinter.com> 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.
> 
> 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.

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

[Johannes Schindelin]

Hi,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Sun, 2 Dec 2007, Shawn O. Pearce wrote:
> > 
> > > Steven Grimm <koreth@midwinter.com> 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.
> > 
> > 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

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

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Mon, 3 Dec 2007, Shawn O. Pearce wrote:
> > 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.

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

[Johannes Schindelin]

Hi,

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

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > On Mon, 3 Dec 2007, Shawn O. Pearce wrote:
> > > 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

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

[Steven Grimm]

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

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

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Mon, 3 Dec 2007, Shawn O. Pearce wrote:
> > 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.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hrm, this is going to have nasty conflicts with 'next', which already
> does the remote ref matching. I think the best way to implement this
> would probably be on top of the jk/send-pack topic in next, and add a
> new REF_STATUS_REMOTE_CHANGED status type.

I think Jeff is referring to sp/refspec-match (605b4978).

I still have doubts about having this in the update hook, as the hook is
about accepting or refusing and has never been about rewriting.

If the implementation of the svn hook were to check if you can rebase
cleanly in the update hook without actually rewriting the refs, and then
to perform the real update of the refs in post-receive or post-update
hook, that would feel much cleaner.  But the end result would be the
same as you rewrote the refs inside the update hook like your patch
does, so maybe I am worrying about conceptual cleanliness too much,
needlessly.

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

[Jeff King]

On Wed, Nov 28, 2007 at 01:49:42PM -0800, Junio C Hamano wrote:

> > Hrm, this is going to have nasty conflicts with 'next', which already
> > does the remote ref matching. I think the best way to implement this
> > would probably be on top of the jk/send-pack topic in next, and add a
> > new REF_STATUS_REMOTE_CHANGED status type.
> 
> I think Jeff is referring to sp/refspec-match (605b4978).

No, I was actually referring to the jk/send-pack topic. I had thought it
graduated to master, but since Steven's work was based on a much older
version (e.g., with send-pack.c rather than builtin-send-pack.c), I
assumed it had not graduated and didn't confirm.

So let me amend my comment to "base this on a more recent master...".

-Peff