Re: [PATCH v8 00/44] Use ref transactions for all ref updates

[Jonathan Nieder <jrnieder@gmail.com>]

Ronnie Sahlberg wrote:

> This patch series can also be found at
> https://github.com/rsahlberg/git/tree/ref-transactions

Thoughts on 65a1cb7b (2014-05-22 12:08):

 04/40 add a strbuf argument to ref_transaction_commit for error logging

 Ideally this would come after the functions it calls so the comment
 "If error is non-NULL we will add an error string to it to explain
 why the transaction failed" would be true already.  But rearranging
 the series for that would be more fuss than it's worth IMHO.

 The sanity check

	int ref_transaction_commit(...)
	{
		int ret = ref_transaction_commit_real(...);
		if (ret && err && !err->len)
			die("BUG: ref_transaction_commit forgot to write an error message");
		return ret;
	}

 shows no instances of forgotten error messages in cases exercised by
 tests, at least.  And it's a step in the right direction.

 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

 05/40 add an err argument to repack_without_refs
 unable_to_lock_strbuf could be called unable_to_lock_message (which
 would make its behavior more obvious imho).

 This makes errno meaningful when commit_packed_refs returns an error.
 Probably its API documentation should say so to make it obvious to
 people modifying it in the future to preserve that property or audit
 callers.

 Via the new call to unable_to_lock_..., repack_without_refs cares
 about errno after a failed call to lock_packed_refs.  lock_packed_refs
 can only fail in hold_lock_file_for_update.  hold_lock_file_for_update
 is a thin wrapper around lockfile.c::lock_file.  lock_file can error
 out because

	strlen(path) >= max_path_len
	adjust_shared_perm failed [calls error(), clobbers errno]
	open failed

 So lock_file needs a similar kind of fix, and it's probably worth
 updating API documentation for these calls to make it clear that
 their errno is used (though that's not a new problem since
 repack_without_refs already called unable_to_lock_error).  Could be
 a separate, earlier patch since it's fixing an existing bug.

 06/40 make ref_update_reject_duplicates take a strbuf argument for errors
 still Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

 07/40 add an err argument to delete_ref_loose
 The new unlink_or_err has an odd contract when the err argument is passed.
 On error:

  * if errno != ENOENT, it will append a message to err and return -1 (good)
  * if errno == ENOENT, it will not append a message to err but will
    still return -1.

 Perhaps it should return 0 when errno == ENOENT.  After all, in that
 case the file does not exist any more, which is all we wanted.  And it
 would save the caller from having to inspect errno.

 On failure we seem to add our own message to err, too, so the resulting
 message would be something like

	fatal: unable to unlink .git/refs/heads/master: \
	Permission deniedfailed to delete loose ref '.git/refs/heads/master.lock'

 The second strbuf_addf is probably not needed.

 08/40 make update_ref_write update a strbuf on failure
 still Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
 
 09/40 log transaction error from the update_ref
 No actual functional change intended, right?  I'd say something like
 "update-ref: use err argument to get error from ref_transaction_commit"
 or something similar to make it clearer that this is just about
 changing APIs.  Or if there's an intended functional change, then the
 commit message could say something about that.

 10/40 remove the onerr argument to ref_transaction_commit
 still Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>