Multiple simultaneously locked ref updates

fatal: Unable to create '....lock': File exists. If no other git process is currently running, this
probably means a git process crashed in this repository earlier. Make sure no other git process is running and remove the file manually to continue.

Quoted 6 lines · Brad King

fatal: Unable to create '....lock': File exists. If no other git process is currently running, this
probably means a git process crashed in this repository earlier. Make sure no other git process is running and remove the file manually to continue.

I don't believe git currently tries to do any form of stale lock recovery since it is racy and unreliable (both single server or on a multi-server shared repo),

Nor should it in this case. I was saying that the front-end needs to reject duplicate ref names from the stdin lines before trying to lock the ref twice to avoid this message.

Quoted 2 lines · Brad King

needs to reject duplicate ref names from the stdin lines before trying to lock the ref twice to avoid this message.

How about trying not to feed duplicates?

Quoted 5 lines · Junio C Hamano

Quoted 2 lines · Brad King

needs to reject duplicate ref names from the stdin lines before trying to lock the ref twice to avoid this message.

How about trying not to feed duplicates?

Sure, perhaps it is simplest to push the responsibility on the user to avoid duplicates.

The lock file may exist because:
- another running git process already has the lock, or
- this process already has the lock because it was asked to update the same file multiple times simultaneously, or
- a stale lock is left from a git process that crashed earlier. In the last case, make sure no other git process is running and remove the file manually to continue.

--------------------------------------------------------------------

IIUC the message cannot say anything about a 'ref' because it is used for other file type lock failures too.

Comments?
-Brad

I didn't mean to force the caller of new "update-ref --stdin"; the new code you wrote for it is what feeds the input to update_refs() function, and that is one place you can make sure you do not lock yourself out.

Besides, if you get two updates to the same ref from --stdin, you would need to verify these are identical (i.e. updating to the same new object name from the same old object name; otherwise the requests are conflicting and do not make sense), so the code to collect the requests from stdin needs to match potential duplicates anyway.

One clean way to do so may be to put an update request (old and new sha1) in a structure, and use a string_list to hold list of refs, with the util field pointing at the update request data.

Quoted 2 lines

- this process already has the lock because it was asked to update the same file multiple times simultaneously, or

The second case is like left hand not knowing what right hand is doing, no? It should not happen if we code it right.

Get it out of the way for a future refs.h function.

Signed-off-by Brad King <[email protected]>

---
 builtin/reset.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

builtin/reset.c +2 -2

index afa6e02..789ee48 100644
@@ -219,7 +219,7 @@ static const char **parse_args(const char **argv, const char *prefix, const char
 	return argv[0] ? get_pathspec(prefix, argv) : NULL;
 }
 
-static int update_refs(const char *rev, const unsigned char *sha1)
+static int reset_refs(const char *rev, const unsigned char *sha1)
 {
 	int update_ref_status;
 	struct strbuf msg = STRBUF_INIT;

@@ -350,7 +350,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	if (!pathspec && !unborn) {
 		/* Any resets without paths update HEAD to the head being
 		 * switched to, saving the previous head in ORIG_HEAD before. */
-		update_ref_status = update_refs(rev, sha1);
+		update_ref_status = reset_refs(rev, sha1);
 
 		if (reset_type == HARD && !update_ref_status && !quiet)
 			print_new_head_line(lookup_commit_reference(sha1));

Quoted 1 line · Brad King

Get it out of the way for a future refs.h function.

Readers do not know if "update_refs()" is a good name for that future refs.h function at this point, so "evict squatter" is not a very good justification by itself. I do agree that this static function is resetting a ref, not doing an arbitrary update, and the new name is a better match than the old one for what it does, though.

Expose lock_ref_sha1_basic's type_p argument to callers of lock_any_ref_for_update. Update all call sites to ignore it; we will use it later.
...

branch.c +0 -0

index c5c6984..c244483 100644

If you are passing an NULL as a new parameter, please spell it "NULL", not "0".

Factor loose ref deletion into helper function delete_ref_loose to allow later use elsewhere. While at it, rename local names 'flag => type' and 'delopt => flags' for consistency with callers and called functions.

Signed-off-by Brad King <[email protected]>

---
 refs.c |   24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

refs.c +1 -1

index 2e755b4..5908648 100644
@@ -2450,15 +2450,10 @@ static int repack_without_ref(const char *refname)
 	return commit_packed_refs();
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+static int delete_ref_loose(struct ref_lock *lock, int type)

refs.c +13 -0

 		/* loose */
 		i = strlen(lock->lk->filename) - 5; /* .lock */
 		lock->lk->filename[i] = 0;

@@ -2468,6 +2463,19 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 
 		lock->lk->filename[i] = '.';
 	}
+	return ret;
+}
+
+int delete_ref(const char *refname, const unsigned char *sha1, int flags)
+{
+	struct ref_lock *lock;
+	int ret = 0, type = 0;
+
+	lock = lock_ref_sha1_basic(refname, sha1, flags, &type);
+	if (!lock)
+		return 1;
+	ret |= delete_ref_loose(lock, type);
+
 	/* removing the loose one could have resurrected an earlier
 	 * packed one.  Also, if it was not loose we need to repack
 	 * without it.

refs.c static int repack_without_ref(const char *refname) · line 2456

Quoted 2 lines · Brad King

refs.c +1 -1

-	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
+	if (!(type & REF_ISPACKED) || type & REF_ISSYMREF) {

Hits from "git grep REF_IS" tell me that all users of REF_IS* symbol that check if a bit is on in a word does so against "flag" (either a variable called "flag", "flags", or a structure member ".flag").

This change is making things less consistent, not more, I am afraid.

Generalize repack_without_ref as repack_without_refs to support a list of refs and implement the former in terms of the latter.

Signed-off-by Brad King <[email protected]>

---
 refs.c |   29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

refs.c +2 -1

index 5908648..5a6c14e 100644
@@ -2414,25 +2414,35 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
 	return 0;
 }
 
-static int repack_without_ref(const char *refname)
+static int repack_without_refs(const char **refnames, int n)
 {
 	struct ref_dir *packed;
 	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
 	struct string_list_item *ref_to_delete;
+	int i, removed = 0;

refs.c +3 -3

+		return 0; /* no refname exists in packed refs */
 
 	if (lock_packed_refs(0)) {
 		unable_to_lock_error(git_path("packed-refs"), errno);
-		return error("cannot delete '%s' from packed refs", refname);
+		return error("cannot delete '%s' from packed refs", refnames[i]);
 	}
 	packed = get_packed_refs(&ref_cache);
 
-	/* Remove refname from the cache: */
-	if (remove_entry(packed, refname) == -1) {
+	/* Remove refnames from the cache: */

refs.c +5 -0

@@ -2450,6 +2460,11 @@ static int repack_without_ref(const char *refname)
 	return commit_packed_refs();
 }
 
+static int repack_without_ref(const char *refname)
+{
+	return repack_without_refs(&refname, 1);
+}
+
 static int delete_ref_loose(struct ref_lock *lock, int type)
 {
 	int err, i, ret = 0;

refs.c static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data) · line 2430

Quoted 1 line · Brad King

refs.c +1 -0

+	if(i == n)

Style:

	if (i == n)

Add 'struct ref_update' to encode the information needed to update or delete a ref (name, new sha1, optional old sha1, no-deref flag). Add function 'update_refs' accepting an array of updates to perform. First acquire locks on all refs with verified old values. Then update or delete all refs accordingly. Fail if any one lock cannot be obtained or any one old value does not match.

Though the refs themeselves cannot be modified together in a single atomic transaction, this function does enable some useful semantics. For example, a caller may create a new branch starting from the head of another branch and rewind the original branch at the same time. This transfers ownership of commits between branches without risk of losing commits added to the original branch by a concurrent process, or risk of a concurrent process creating the new branch first.

Signed-off-by Brad King <[email protected]>

---
 refs.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h |   11 +++++++++++
 2 files changed, 77 insertions(+)

refs.c +8 -0

index 5a6c14e..0a0c19e 100644
@@ -3238,6 +3238,72 @@ int update_ref(const char *action, const char *refname,
 	return update_ref_write(action, refname, sha1, lock, onerr);
 }
 
+int update_refs(const char *action, struct ref_update *updates,
+		int n, enum action_on_err onerr)
+{
+	int ret = 0, delnum = 0, i;
+	int *types;
+	struct ref_lock **locks;
+	const char **delnames;
+

refs.c +40 -0

+	/* Abort if we did not get all locks: */
+	if (i < n) {
+		while (--i >= 0)
+			unlock_ref(locks[i]);
+		free(types);
+		free(locks);
+		free(delnames);
+		return 1;
+	}
+
+	/* Perform updates first so live commits remain referenced: */
+	for (i=0; i < n; ++i)
+		if (!is_null_sha1(updates[i].new_sha1)) {
+			ret |= update_ref_write(action,
+						updates[i].ref_name,
+						updates[i].new_sha1,
+						locks[i], onerr);
+			locks[i] = 0; /* freed by update_ref_write */
+		}
+
+	/* Perform deletes now that updates are safely completed: */
+	for (i=0; i < n; ++i)
+		if (locks[i]) {
+			delnames[delnum++] = locks[i]->ref_name;
+			ret |= delete_ref_loose(locks[i], types[i]);
+		}
+	ret |= repack_without_refs(delnames, delnum);
+	for (i=0; i < delnum; ++i)
+		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	clear_loose_ref_cache(&ref_cache);
+	for (i=0; i < n; ++i)
+		if (locks[i])
+			unlock_ref(locks[i]);
+
+	free(types);
+	free(locks);
+	free(delnames);
+	return ret;
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)

refs.h +0 -0

index 2cd307a..5763f3a 100644

refs.h +7 -0

+	int flags;
+};
+
+/** lock all refs and then write all of them */
+int update_refs(const char *action, struct ref_update *updates,
+		int n, enum action_on_err onerr);
+
 extern int parse_hide_refs_config(const char *var, const char *value, const char *);
 extern int ref_is_hidden(const char *);

refs.c int update_ref(const char *action, const char *refname, · line 3258

Quoted 1 line · Brad King

refs.c +1 -0

+	for (i=0; i < n; ++i) {

Style:

	for (i = 0; i < n; i++) {

Is it asking for AB-BA deadlock? If so, is the caller responsible for avoiding it?

refs.h int update_ref(const char *action, const char *refname, · line 220

Quoted 2 lines

refs.h +2 -0

+	unsigned char new_sha1[20];
+	unsigned char *old_sha1;

This asymmetry is rather curious and will later become problematic when we have more users of this API. Maybe your callers happen have static storage to keep old_sha1[] outside this structure but do not have it for new_sha1[] for some unknown reason, which may not apply to later callers we may want to add.

Quoted 6 lines · Junio C Hamano

refs.c int update_ref(const char *action, const char *refname, · line 3258

Quoted 1 line · Brad King

refs.c +1 -0

+	for (i=0; i < n; ++i) {

Style:

	for (i = 0; i < n; i++) {

Fixed.

Quoted 2 lines

Is it asking for AB-BA deadlock? If so, is the caller responsible for avoiding it?

Since we don't actually block waiting for locks we won't really deadlock.

For Git's internal API I think we can document this in a comment so that update_refs does not have to sort. Then we can add a new ref_update_sort function to sort an array of struct ref_update. The user-facing "update-ref --stdin" can then use ref_update_sort.

But it may not be a bad idea to keep the callers dumb and have this function always sort, dedup, *and* fail inconsistent request.

I wasn't happy with the asymmetry either but forgot to raise it in the cover letter. We need a way to represent "old value not given" as different from "old value is_null_sha1".

Another approach is to add a dedicated member to struct ref_update.

builtin/update-ref.c line 22

Quoted 1 line · Brad King

builtin/update-ref.c +1 -0

+	const char *c, *s, *oldvalue, *value[2] = {0,0};

This patch has many style issues of the same kind, lack of a SP at places where there should be between operators and after comma.

Factor the lock and write steps and error handling into helper functions update_ref_lock and update_ref_write to allow later use elsewhere. Expose lock_any_ref_for_update's type_p to update_ref_lock callers.

Signed-off-by Brad King <[email protected]>

---
 refs.c |   28 +++++++++++++++++++++++-----
 1 file changed, 23 insertions(+), 5 deletions(-)

refs.c +0 -0

index c69fd68..2e755b4 100644
@@ -3170,12 +3170,13 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
 	return retval;
 }

refs.c +19 -2

-	lock = lock_any_ref_for_update(refname, oldval, flags, NULL);
+	lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
 	if (!lock) {
 		const char *str = "Cannot lock the ref '%s'.";
 		switch (onerr) {

@@ -3183,8 +3184,14 @@ int update_ref(const char *action, const char *refname,
 		case DIE_ON_ERR: die(str, refname); break;
 		case QUIET_ON_ERR: break;
 		}
-		return 1;
 	}
+	return lock;
+}
+
+static int update_ref_write(const char *action, const char *refname,
+			    const unsigned char *sha1, struct ref_lock *lock,
+			    enum action_on_err onerr)
+{
 	if (write_ref_sha1(lock, sha1, action) < 0) {
 		const char *str = "Cannot update the ref '%s'.";
 		switch (onerr) {

@@ -3197,6 +3204,17 @@ int update_ref(const char *action, const char *refname,
 	return 0;
 }
 
+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 = update_ref_lock(refname, oldval, flags, 0, onerr);
+	if (!lock)
+		return 1;
+	return update_ref_write(action, refname, sha1, lock, onerr);
+}
+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)

Quoted 1 line · Brad King

static struct ref_lock *lock;

Not the fault of this patch, as the original update_ref() had it this way, but it is not necessary to keep the value of this variable across invocations. Let's drop "static" from here, and also the corresponding variable in the new update_ref().

Factor loose ref deletion into helper function delete_ref_loose to allow later use elsewhere.

Signed-off-by Brad King <[email protected]>

---
 refs.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

refs.c +2 -7

index 2e755b4..5dd86ee 100644
@@ -2450,14 +2450,9 @@ static int repack_without_ref(const char *refname)
 	return commit_packed_refs();
 }
 
-int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+static int delete_ref_loose(struct ref_lock *lock, int flag)
 {
-	struct ref_lock *lock;
-	int err, i = 0, ret = 0, flag = 0;
-
-	lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
-	if (!lock)
-		return 1;
+	int err, i, ret = 0;
 	if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
 		/* loose */
 		i = strlen(lock->lk->filename) - 5; /* .lock */

refs.c +3 -0

+int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
+{
+	struct ref_lock *lock;

Given that ret is only returned, you could restore the filename before the if statement and replace the ret variable with an immediate return statement:

Add 'struct ref_update' to encode the information needed to update or delete a ref (name, new sha1, optional old sha1, no-deref flag). Add function 'update_refs' accepting an array of updates to perform. First sort the input array to order locks consistently everywhere and reject multiple updates to the same ref. Then acquire locks on all refs with verified old values. Then update or delete all refs accordingly. Fail if any one lock cannot be obtained or any one old value does not match.

Though the refs themeselves cannot be modified together in a single

atomic transaction, this function does enable some useful semantics. For example, a caller may create a new branch starting from the head of another branch and rewind the original branch at the same time. This transfers ownership of commits between branches without risk of losing commits added to the original branch by a concurrent process, or risk of a concurrent process creating the new branch first.

Signed-off-by Brad King <[email protected]>

---
 refs.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h |   14 ++++++++
 2 files changed, 135 insertions(+)

refs.c +0 -0

index 3bcd26e..901a38a 100644

refs.c +4 -0

+	ret = strcmp(u1->ref_name, u2->ref_name);
+	if (ret)
+		return ret;
+	ret = hashcmp(u1->new_sha1, u2->new_sha1);

refs.c +16 -0

+	if (i < n) {
+		const char *str = "Multiple updates for ref '%s' not allowed.";
+		switch (onerr) {
+		case MSG_ON_ERR: error(str, updates[i].ref_name); break;
+		case DIE_ON_ERR: die(str, updates[i].ref_name); break;
+		case QUIET_ON_ERR: break;
+		}
+		return 1;
+	}
+	return 0;
+}
+
+int update_refs(const char *action, const struct ref_update *updates_orig,
+		int n, enum action_on_err onerr)
+{
+	int ret = 0, delnum = 0, i;

refs.c +7 -0

+	if (ref_update_reject_duplicates(updates, n, onerr)) {
+		free(updates);
+		free(types);
+		free(locks);
+		free(delnames);
+		return 1;
+	}

refs.c +13 -0

+	}
+
+	/* Abort if we did not get all locks: */
+	if (i < n) {
+		while (--i >= 0)
+			unlock_ref(locks[i]);
+		free(updates);
+		free(types);
+		free(locks);
+		free(delnames);
+		return 1;
+	}
+

refs.c +10 -0

+	/* Perform deletes now that updates are safely completed: */
+	for (i = 0; i < n; ++i)
+		if (locks[i]) {
+			delnames[delnum++] = locks[i]->ref_name;
+			ret |= delete_ref_loose(locks[i], types[i]);
+		}
+	ret |= repack_without_refs(delnames, delnum);
+	for (i = 0; i < delnum; ++i)
+		unlink_or_warn(git_path("logs/%s", delnames[i]));
+	clear_loose_ref_cache(&ref_cache);

refs.c +1 -0

+
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
 {
 	for ( ; list; list = list->next)

refs.h +0 -0

index 2cd307a..a8a7cc6 100644

s/themeselves/themselves/

refs.c int update_ref(const char *action, const char *refname, · line 3244

Quoted 2 lines

refs.c +2 -0

+	struct ref_update *u1 = (struct ref_update *)(r1);
+	struct ref_update *u2 = (struct ref_update *)(r2);

If you declare u1 and u2 to be "const struct ref_update *" (i.e., add "const"), then you have const correctness and don't need the explicit casts. (And the parentheses around r1 and r2 are superfluous in any case.)

refs.c int update_ref(const char *action, const char *refname, · line 3246

Quoted 1 line

refs.c +1 -0

+	ret = strcmp(u1->ref_name, u2->ref_name);

Is there a need to compare more than ref_name? If two entries are found with the same name, then ref_update_reject_duplicates() will error out

Review

Quoted 2 lines

(quoted diff) +2 -0

+		if (!strcmp(updates[i - 1].ref_name, updates[i].ref_name))
+			break;

The error handling code could be right here instead of the "break" statement, removing the need for the "if" conditional.

refs.c int update_ref(const char *action, const char *refname, · line 3293

Quoted 2 lines

refs.c +2 -0

+	/* Allocate work space: */
+	updates = xmalloc(sizeof(struct ref_update) * n);

It seems preferred here to write

	updates = xmalloc(sizeof(*updates) * n);

as this will continue to work if the type of updates is ever changed.

Similarly for the next lines.

refs.c int update_ref(const char *action, const char *refname, · line 3296

Quoted 3 lines

refs.c +3 -0

+	types = xmalloc(sizeof(int) * n);
+	locks = xmalloc(sizeof(struct ref_lock *) * n);
+	delnames = xmalloc(sizeof(const char *) * n);

An alternative to managing separate arrays to hold types and locks would be to include the scratch space in struct ref_update and document it "for internal use only; need not be initialized by caller". On the one hand it's ugly to cruft up the "interface" with internal implementation details; on the other hand there is precedent for this sort of thing (e.g., ref_lock::force_write or lock_file::on_list) and it would simplify the code.

refs.c int update_ref(const char *action, const char *refname, · line 3300

Quoted 3 lines

refs.c +3 -0

+	/* Copy, sort, and reject duplicate refs: */
+	memcpy(updates, updates_orig, sizeof(struct ref_update) * n);
+	qsort(updates, n, sizeof(struct ref_update), ref_update_compare);

You could save some space and memory shuffling (during memcpy() and qsort()) if you would declare "updates" to be an array of pointers to "struct ref_update" rather than an array of structs. Sorting could then be done by moving pointers around instead of moving the structs. This would also make it easier for update_refs() to pass information about the references back to its caller, should that ever be needed.

refs.c int update_ref(const char *action, const char *refname, · line 3340

Quoted 7 lines

refs.c +7 -0

+			ret |= update_ref_write(action,
+						updates[i].ref_name,
+						updates[i].new_sha1,
+						locks[i], onerr);
+			locks[i] = 0; /* freed by update_ref_write */
+		}
+

Hmmm, if one of the calls to update_ref_write() fails, would it be safer to abort the rest of the work (especially the reference deletions)?

refs.c int update_ref(const char *action, const char *refname, · line 3360

Quoted 6 lines

refs.c +6 -0

+	free(updates);
+	free(types);
+	free(locks);
+	free(delnames);
+	return ret;
+}

There's a lot of duplicated cleanup code in the function. If you put a label before the final for loop, and if you initialize the locks array to zeros (e.g., by using xcalloc()), then the three exits could all share the same code "ret = 1; goto cleanup;".

Quoted 1 line

+struct ref_update {

Please document this structure, especially the relationship between have_old and old_sha1.

Add 'struct ref_update' to encode the information needed to update or delete a ref (name, new sha1, optional old sha1, no-deref flag). Add function 'update_refs' accepting an array of updates to perform. First sort the input array to order locks consistently everywhere and reject multiple updates to the same ref. Then acquire locks on all refs with verified old values. Then update or delete all refs accordingly. Fail if any one lock cannot be obtained or any one old value does not match.

Though the refs themeselves cannot be modified together in a single

refs.c +4 -0

+	ret = strcmp(u1->ref_name, u2->ref_name);
+	if (ret)
+		return ret;
+	ret = hashcmp(u1->new_sha1, u2->new_sha1);

Quoted 1 line

Though the refs themeselves cannot be modified together in a single

"themselves".

I notice that we are using an array of structures and letting qsort swap 50~64 bytes of data

Optionally we could silently dedup multiple identical updates and not fail it in ref-update-reject-duplicates. But that does not have to be done until we find people's script would benefit from such a nicety.

By the way, unless there is a strong reason not to do so, post-increment "i++" (and pre-decrement "--i", if you use it) is the norm around here.

refs.c int update_ref(const char *action, const char *refname, · line 3338

Quoted 1 line

refs.c +1 -0

+			locks[i] = 0; /* freed by update_ref_write */

I think what is assigned here is a NULL pointer.

Quoted 3 lines · Junio C Hamano

Quoted 1 line

Though the refs themeselves cannot be modified together in a single

"themselves".

Fixed.

Quoted 2 lines

I notice that we are using an array of structures and letting qsort swap 50~64 bytes of data

Michael suggested this too, so fixed.

Hmmm, I see that you changed the signature of update_refs() to take an array of pointers. My suggestion was unclear, but I didn't mean that the function signature had to be changed.

However, your approach is also fine.

Add a --stdin signature to read update instructions from standard input and apply multiple ref updates together. Use an input format that supports any update that could be specified via the command-line, including object names like 'branch:path with space'.

Signed-off-by Brad King <[email protected]>

---
 Documentation/git-update-ref.txt |   21 ++++++-
 builtin/update-ref.c             |  121 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 2 deletions(-)

Documentation/git-update-ref.txt +7 -1

index 0df13ff..295d0bb 100644
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 --------
 [verse]
-'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>])
+'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>] | --stdin)
 
 DESCRIPTION
 -----------

@@ -58,6 +58,25 @@ archive by creating a symlink tree).
 With `-d` flag, it deletes the named <ref> after verifying it
 still contains <oldvalue>.
 
+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Empty lines are ignored.
+Each non-empty line is parsed as whitespace-separated arguments.
+Use single-quotes to enclose whitespace and backslashes and an
+unquoted backslash to escape a single quote.  Specify updates with
+lines of the form:

Documentation/git-update-ref.txt +3 -0

+modifications are performed.  Note that while each individual
+<ref> is updated or deleted atomically, a concurrent reader may
+still see a subset of the modifications.
 
 Logging Updates
 ---------------

builtin/update-ref.c +17 -0

index 51d2684..eb8db85 100644
@@ -6,19 +6,129 @@
 static const char * const git_update_ref_usage[] = {
 	N_("git update-ref [options] -d <refname> [<oldval>]"),
 	N_("git update-ref [options]    <refname> <newval> [<oldval>]"),
+	N_("git update-ref [options] --stdin"),
 	NULL
 };
 
+static const char blank[] = " \t\r\n";
+
+static int updates_size;
+static int updates_count;
+static struct ref_update *updates;
+
+static const char* update_refs_stdin_next_arg(const char* next,
+					      struct strbuf *arg)
+{
+	/* Skip leading whitespace: */
+	while (isspace(*next))
+		++next;
+
+	/* Return NULL when no argument is found: */
+	if (!*next)
+		return NULL;

builtin/update-ref.c +45 -0

+			if (!next[len])
+				die("unterminated single-quote: '%s", next);
+			strbuf_add(arg, next, len);
+			next += len + 1;
+			continue;
+		}
+		if (c == '\\') {
+			if (*next == '\'')
+				c = *next++;
+			else
+				die("unquoted backslash not escaping "
+				    "single-quote: \\%s", next);
+		}
+		strbuf_addch(arg, c);
+	}
+	return next;
+}
+
+static void update_refs_stdin(const char *line)
+{
+	int options = 1, flags = 0, argc = 0;
+	char *argv[3] = {0, 0, 0};
+	struct strbuf arg = STRBUF_INIT;
+	struct ref_update *update;
+	const char *next = line;
+
+	/* Skip blank lines: */
+	if (!line[0])
+		return;
+
+	/* Parse arguments on this line: */
+	while ((next = update_refs_stdin_next_arg(next, &arg)) != NULL) {
+		if (options && arg.buf[0] == '-')
+			if (!strcmp(arg.buf, "--no-deref"))
+				flags |= REF_NODEREF;
+			else if (!strcmp(arg.buf, "--"))
+				options = 0;
+			else
+				die("unknown option %s", arg.buf);
+		else if (argc >= 3)
+			die("too many arguments on line: %s", line);
+		else {
+			argv[argc++] = xstrdup(arg.buf);
+			options = 0;
+		}

builtin/update-ref.c +38 -1

+	update = &updates[updates_count++];
+	update->flags = flags;
+
+	/* Set the update ref_name: */
+	if (!argv[0])
+		die("no ref on line: %s", line);
+	if (check_refname_format(argv[0], REFNAME_ALLOW_ONELEVEL))
+		die("invalid ref format on line: %s", line);
+	update->ref_name = argv[0];
+	argv[0] = 0;
+
+	/* Set the update new_sha1 and, if specified, old_sha1: */
+	if (!argv[1])
+		die("missing new value on line: %s", line);
+	if (*argv[1] && get_sha1(argv[1], update->new_sha1))
+		die("invalid new value on line: %s", line);
+	if (argv[2]) {
+		update->have_old = 1;
+		if (*argv[2] && get_sha1(argv[2], update->old_sha1))
+			die("invalid old value on line: %s", line);
+	}
+
+	while (argc > 0)
+		free(argv[--argc]);
+}
+
 int cmd_update_ref(int argc, const char **argv, const char *prefix)
 {
 	const char *refname, *oldval, *msg = NULL;
 	unsigned char sha1[20], oldsha1[20];
-	int delete = 0, no_deref = 0, flags = 0;
+	int delete = 0, no_deref = 0, read_stdin = 0, flags = 0;
+	struct strbuf line = STRBUF_INIT;
 	struct option options[] = {
 		OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the update")),
 		OPT_BOOLEAN('d', NULL, &delete, N_("delete the reference")),
 		OPT_BOOLEAN( 0 , "no-deref", &no_deref,
 					N_("update <refname> not the one it points to")),
+		OPT_BOOLEAN( 0 , "stdin", &read_stdin, N_("read updates from stdin")),
 		OPT_END(),
 	};
 

@@ -28,6 +138,15 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
 	if (msg && !*msg)
 		die("Refusing to perform update with empty message.");
 
+	if (read_stdin) {
+		if (delete || no_deref || argc > 0)
+			usage_with_options(git_update_ref_usage, options);
+		while (strbuf_getline(&line, stdin, '\n') != EOF)
+			update_refs_stdin(line.buf);
+		strbuf_release(&line);
+		return update_refs(msg, updates, updates_count, DIE_ON_ERR);
+	}
+
 	if (delete) {
 		if (argc < 1 || argc > 2)
 			usage_with_options(git_update_ref_usage, options);

Documentation/git-update-ref.txt archive by creating a symlink tree). · line 76

Quoted 2 lines · Brad King

Documentation/git-update-ref.txt +2 -0

+If all <ref>s can be locked with matching <oldvalue>s
+simultaneously all modifications are performed.  Otherwise, no

Comma after "simultaneously".

I agree with Junio that your quoting rules are peculiar.

builtin/update-ref.c line 87

Quoted 1 line

builtin/update-ref.c +1 -0

+	/* Allocate and zero-init a struct ref_update: */

Here you can use ARRAY_GROW(). See

Documentation/technical/api-allocation-growing.txt

Add a --stdin signature to read update instructions from standard input and apply multiple ref updates together. Use an input format that supports any update that could be specified via the command-line, including object names like 'branch:path with space'.

Signed-off-by Brad King <[email protected]>

---
 Documentation/git-update-ref.txt |   21 ++++++-
 builtin/update-ref.c             |  121 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 140 insertions(+), 2 deletions(-)

Documentation/git-update-ref.txt +1 -1

index 0df13ff..295d0bb 100644
@@ -8,7 +8,7 @@ git-update-ref - Update the object name stored in a ref safely
 SYNOPSIS
 --------
 [verse]
-'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>])
+'git update-ref' [-m <reason>] (-d <ref> [<oldvalue>] | [--no-deref] <ref> <newvalue> [<oldvalue>] | --stdin)
 
 DESCRIPTION
 -----------

Documentation/git-update-ref.txt archive by creating a symlink tree).

Quoted 5 lines · Brad King

Documentation/git-update-ref.txt +5 -0

+With `--stdin`, update-ref reads instructions from standard input and
+performs all modifications together.  Empty lines are ignored.
+Each non-empty line is parsed as whitespace-separated arguments.
+Use single-quotes to enclose whitespace and backslashes and an
+unquoted backslash to escape a single quote.

That is somewhat unusual.

When we need to deal with arbitrary strings (like pathnames), other parts of the system usually give the user two interfaces, --stdin with and without -z, and the strings are C-quoted when run without the -z option, and terminated with NUL when run with the -z option.

Documentation/git-update-ref.txt archive by creating a symlink tree). · line 68

Quoted 4 lines

Documentation/git-update-ref.txt +4 -0

+Specify updates with
+lines of the form:
+
+	[--no-deref] [--] <ref> <newvalue> [<oldvalue>]

What is -- doing here? refs given to update-ref begin with refs/ (otherwise it is HEAD), no?

Here is the second revision of a series to support locking multiple refs at the same time to update all of them consistently. The first series can be found at $gmane/233260. This revision is ready to consider for integration.

Extend t/t1400-update-ref.sh to cover cases using the --stdin option.

Signed-off-by Brad King <[email protected]>

---
 t/t1400-update-ref.sh |  206 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 206 insertions(+)

t/t1400-update-ref.sh +0 -0

index e415ee0..9fd03fc 100755
@@ -302,4 +302,210 @@ test_expect_success \
        'git cat-file blob master@{2005-05-26 23:42}:F (expect OTHER)' \
        'test OTHER = $(git cat-file blob "master@{2005-05-26 23:42}:F")'

stdin &&

+ git update-ref --stdin < stdin &&

t/t1400-update-ref.sh +22 -0

+       git rev-parse --verify -q $m
+'
+
+test_expect_success 'stdin fails with bad line lines' '
+       echo " " > stdin &&
+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: no ref on line:  " err &&
+       echo "--" > stdin &&
+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: no ref on line: --" err &&
+       echo "--bad-option" > stdin &&
+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: unknown option --bad-option" err &&
+       echo "-\'"'"' $a $m" > stdin &&
+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: unknown option -'"'"'" err &&
+       echo "~a $m" > stdin &&
+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: invalid ref format on line: ~a $m" err &&
+       echo "$a '"'"'master" > stdin &&
+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: unterminated single-quote: '"'"'master" err &&

t/t1400-update-ref.sh +3 -0

+       test_must_fail git update-ref --stdin < stdin 2> err &&
+       grep "fatal: Multiple updates for ref '"'"'$a'"'"' not allowed." err
+'

t/t1400-update-ref.sh test_expect_success \ · line 313

Quoted 2 lines

t/t1400-update-ref.sh +2 -0

+       rm -f stdin &&
+       touch stdin &&

Unless the timestamp of 'stdin' has particular significance, modern git tests avoid 'touch' in favor of creating the empty file like this

Quoted 1 line

stdin &&

Quoted 1 line

+ git update-ref --stdin < stdin &&

Style: Git test scripts omit whitespace following <, >, <<, and >>.

Quoted 1 line

+test_expect_success 'stdin fails with bad line lines' '

Despite the semantic relationship between all these cases, if there is a regression in one case, the person reading the verbose output has to study it carefully to determine the offending case. If you decompose this monolith so that each case is in its own test_expect_success, then the regressed case becomes immediately obvious.

multi-line preparations of 'stdin' might be more readable with a heredoc:

    cat >stdin <<-EOF &&
    $a $m
    $b $m
    $a $m
    EOF