[PATCH] Function for updating refs.

[PATCH] Function for updating refs.

[Carlos Rica]

A function intended to be called from builtins updating refs
by locking them before write, specially those that came from
scripts using "git update-ref".

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 builtin-fetch--tool.c |   21 ++++++++-------------
 builtin-update-ref.c  |    8 ++------
 refs.c                |   28 ++++++++++++++++++++++++++++
 refs.h                |    6 ++++++
 send-pack.c           |   11 +++--------
 5 files changed, 47 insertions(+), 27 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index e2f8ede..a192fd7 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -31,24 +31,19 @@ static void show_new(enum object_type type, unsigned char *sha1_new)
 		find_unique_abbrev(sha1_new, DEFAULT_ABBREV));
 }

-static int update_ref(const char *action,
+static int update_ref_env(const char *action,
 		      const char *refname,
 		      unsigned char *sha1,
 		      unsigned char *oldval)
 {
 	char msg[1024];
 	char *rla = getenv("GIT_REFLOG_ACTION");
-	static struct ref_lock *lock;

 	if (!rla)
 		rla = "(reflog update)";
-	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
-	lock = lock_any_ref_for_update(refname, oldval, 0);
-	if (!lock)
-		return 1;
-	if (write_ref_sha1(lock, sha1, msg) < 0)
-		return 1;
-	return 0;
+	if (snprintf(msg, sizeof(msg), "%s: %s", rla, action) >= sizeof(msg))
+		error("reflog message too long: %.*s...", 50, msg);
+	return update_ref(msg, refname, sha1, oldval, 0, QUIET_ON_ERR);
 }

 static int update_local_ref(const char *name,
@@ -88,7 +83,7 @@ static int update_local_ref(const char *name,
 		fprintf(stderr, "* %s: storing %s\n",
 			name, note);
 		show_new(type, sha1_new);
-		return update_ref(msg, name, sha1_new, NULL);
+		return update_ref_env(msg, name, sha1_new, NULL);
 	}

 	if (!hashcmp(sha1_old, sha1_new)) {
@@ -102,7 +97,7 @@ static int update_local_ref(const char *name,
 	if (!strncmp(name, "refs/tags/", 10)) {
 		fprintf(stderr, "* %s: updating with %s\n", name, note);
 		show_new(type, sha1_new);
-		return update_ref("updating tag", name, sha1_new, NULL);
+		return update_ref_env("updating tag", name, sha1_new, NULL);
 	}

 	current = lookup_commit_reference(sha1_old);
@@ -117,7 +112,7 @@ static int update_local_ref(const char *name,
 		fprintf(stderr, "* %s: fast forward to %s\n",
 			name, note);
 		fprintf(stderr, "  old..new: %s..%s\n", oldh, newh);
-		return update_ref("fast forward", name, sha1_new, sha1_old);
+		return update_ref_env("fast forward", name, sha1_new, sha1_old);
 	}
 	if (!force) {
 		fprintf(stderr,
@@ -131,7 +126,7 @@ static int update_local_ref(const char *name,
 		"* %s: forcing update to non-fast forward %s\n",
 		name, note);
 	fprintf(stderr, "  old...new: %s...%s\n", oldh, newh);
-	return update_ref("forced-update", name, sha1_new, sha1_old);
+	return update_ref_env("forced-update", name, sha1_new, sha1_old);
 }

 static int append_fetch_head(FILE *fp,
diff --git a/builtin-update-ref.c b/builtin-update-ref.c
index 8339cf1..2ed98e8 100644
--- a/builtin-update-ref.c
+++ b/builtin-update-ref.c
@@ -8,7 +8,6 @@ static const char git_update_ref_usage[] =
 int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname=NULL, *value=NULL, *oldval=NULL, *msg=NULL;
-	struct ref_lock *lock;
 	unsigned char sha1[20], oldsha1[20];
 	int i, delete, ref_flags;

@@ -62,10 +61,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (oldval && *oldval && get_sha1(oldval, oldsha1))
 		die("%s: not a valid old SHA1", oldval);

-	lock = lock_any_ref_for_update(refname, oldval ? oldsha1 : NULL, ref_flags);
-	if (!lock)
-		die("%s: cannot lock the ref", refname);
-	if (write_ref_sha1(lock, sha1, msg) < 0)
-		die("%s: cannot update the ref", refname);
+	update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
+						ref_flags, DIE_ON_ERR);
 	return 0;
 }
diff --git a/refs.c b/refs.c
index 09a2c87..1343f0d 100644
--- a/refs.c
+++ b/refs.c
@@ -1455,3 +1455,31 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 {
 	return do_for_each_reflog("", fn, cb_data);
 }
+
+int update_ref(const char *action, const char *refname,
+		const unsigned char *sha1, const unsigned char *oldval,
+		int flags, enum action_on_err onerr)
+{
+	static struct ref_lock *lock;
+	lock = lock_any_ref_for_update(refname, oldval, flags);
+	if (!lock) {
+		const char *str = "Cannot lock the ref '%s'.";
+		switch (onerr) {
+		case MSG_ON_ERR: error(str, refname); break;
+		case DIE_ON_ERR: die(str, refname); break;
+		case QUIET_ON_ERR: break;
+		}
+		return 1;
+	}
+	if (write_ref_sha1(lock, sha1, action) < 0) {
+		const char *str = "Cannot update the ref '%s'.";
+		switch (onerr) {
+		case MSG_ON_ERR: error(str, refname); break;
+		case DIE_ON_ERR: die(str, refname); break;
+		case QUIET_ON_ERR: break;
+		}
+		return 1;
+	}
+	return 0;
+}
+
diff --git a/refs.h b/refs.h
index f234eb7..6eb98a4 100644
--- a/refs.h
+++ b/refs.h
@@ -64,4 +64,10 @@ extern int rename_ref(const char *oldref, const char *newref, const char *logmsg
 /** resolve ref in nested "gitlink" repository */
 extern int resolve_gitlink_ref(const char *name, const char *refname, unsigned char *result);

+/** lock a ref and then write its file */
+enum action_on_err { MSG_ON_ERR, DIE_ON_ERR, QUIET_ON_ERR };
+int update_ref(const char *action, const char *refname,
+		const unsigned char *sha1, const unsigned char *oldval,
+		int flags, enum action_on_err onerr);
+
 #endif /* REFS_H */
diff --git a/send-pack.c b/send-pack.c
index 9fc8a81..c59eea4 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -313,14 +313,9 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
 					if (delete_ref(rs.dst, NULL)) {
 						error("Failed to delete");
 					}
-				} else {
-					lock = lock_any_ref_for_update(rs.dst, NULL, 0);
-					if (!lock)
-						error("Failed to lock");
-					else
-						write_ref_sha1(lock, ref->new_sha1,
-							       "update by push");
-				}
+				} else
+					update_ref("update by push", rs.dst,
+						ref->new_sha1, NULL, 0, 0);
 				free(rs.dst);
 			}
 		}
-- 
1.5.0

Re: [PATCH] Function for updating refs.

[Junio C Hamano]

Carlos Rica <jasampler@gmail.com> writes:

> A function intended to be called from builtins updating refs
> by locking them before write, specially those that came from
> scripts using "git update-ref".
>
> Signed-off-by: Carlos Rica <jasampler@gmail.com>

Thanks.  Very nice.

I have two comments but I think they are very minor details I
can and should fix in my inbox and apply, instead of asking you
to update and resend.

> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> index e2f8ede..a192fd7 100644
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -31,24 +31,19 @@ static void show_new(enum object_type type, unsigned char *sha1_new)
>  		find_unique_abbrev(sha1_new, DEFAULT_ABBREV));
>  }
>
> -static int update_ref(const char *action,
> +static int update_ref_env(const char *action,
>  		      const char *refname,
>  		      unsigned char *sha1,
>  		      unsigned char *oldval)
>  {
>  	char msg[1024];
>  	char *rla = getenv("GIT_REFLOG_ACTION");
> -	static struct ref_lock *lock;
>
>  	if (!rla)
>  		rla = "(reflog update)";
> -	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> -	lock = lock_any_ref_for_update(refname, oldval, 0);
> -	if (!lock)
> -		return 1;
> -	if (write_ref_sha1(lock, sha1, msg) < 0)
> -		return 1;
> -	return 0;
> +	if (snprintf(msg, sizeof(msg), "%s: %s", rla, action) >= sizeof(msg))
> +		error("reflog message too long: %.*s...", 50, msg);

The original I did was sloppy and did not detect this situation;
thanks for fixing it.  You do not refuse the primary operation,
which is to update the ref, so this should be a warning instead
of an error, I think.

> diff --git a/send-pack.c b/send-pack.c
> index 9fc8a81..c59eea4 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -313,14 +313,9 @@ static int send_pack(int in, int out, struct remote *remote, int nr_refspec, cha
>  					if (delete_ref(rs.dst, NULL)) {
>  						error("Failed to delete");
>  					}
> -				} else {
> -					lock = lock_any_ref_for_update(rs.dst, NULL, 0);
> -					if (!lock)
> -						error("Failed to lock");
> -					else
> -						write_ref_sha1(lock, ref->new_sha1,
> -							       "update by push");
> -				}

This removal makes "struct ref_lock *lock" (not shown in the
context) unused.  I will remove the declaration.

Re: [PATCH] Function for updating refs.

[Carlos Rica]

2007/9/5, Junio C Hamano <gitster@pobox.com>:
> Carlos Rica <jasampler@gmail.com> writes:
> > -     snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> > -     lock = lock_any_ref_for_update(refname, oldval, 0);
> > -     if (!lock)
> > -             return 1;
> > -     if (write_ref_sha1(lock, sha1, msg) < 0)
> > -             return 1;
> > -     return 0;
> > +     if (snprintf(msg, sizeof(msg), "%s: %s", rla, action) >= sizeof(msg))
> > +             error("reflog message too long: %.*s...", 50, msg);
>
> The original I did was sloppy and did not detect this situation;
> thanks for fixing it.  You do not refuse the primary operation,
> which is to update the ref, so this should be a warning instead
> of an error, I think.

Yes, it is. Also, I tested what would happen when the lock fails. I tried to
lock an already locked ref, and it died printing the message
die("unable to create '%s.lock': %s", path, strerror(errno));  from
lockfile.c. I think this is interesting. There are other failing
reasons that could
make the update_ref function to print its error, but I haven't tested them.

> > diff --git a/send-pack.c b/send-pack.c
> > ...
> This removal makes "struct ref_lock *lock" (not shown in the
> context) unused.  I will remove the declaration.

Thank you. Also in builtin-update-ref.c the main function could return
directly that value returned from the call to update_ref().