On 05/16/2014 07:37 PM, Ronnie Sahlberg wrote:
> Change store_updated_refs to use a single ref transaction for all refs that
> are updated during the fetch. This makes the fetch more atomic when update
> failures occur.
>
> Since ref update failures will now no longer occur in the code path for
> updating a single ref in s_update_ref, we no longer have as detailed error
> message logging the exact reference and the ref log action as in the old cod
s/cod/code/ ?
> Instead since we fail the entire transaction we log a much more generic
> message. But since we commit the transaction using MSG_ON_ERR we will log
> an error containing the ref name if either locking of writing the ref would
s/of/or/ ?
s/would/would fail,/ ?
> so the regression in the log message is minor.
>
> This will also change the order in which errors are checked for and logged
> which may alter which error will be logged if there are multiple errors
> occuring during a fetch.
s/occuring/occurring/
>
> For example, assume we have a fetch for two refs that both would fail.
> Where the first ref would fail with ENOTDIR due to a directory in the ref
> path not existing, and the second ref in the fetch would fail due to
> the check in update_logical_ref():
> if (current_branch &&
> !strcmp(ref->name, current_branch->name) &&
> !(update_head_ok || is_bare_repository()) &&
> !is_null_sha1(ref->old_sha1)) {
> /*
> * If this is the head, and it's not okay to update
> * the head, and the old value of the head isn't empty...
> */
>
> In the old code since we would update the refs one ref at a time we would
> first fail the ENOTDIR and then fail the second update of HEAD as well.
> But since the first ref failed with ENOTDIR we would eventually fail the who
s/who/whole/
> fetch with STORE_REF_ERROR_DF_CONFLICT
>
> In the new code, since we defer committing the transaction until all refs
> have been processed, we would now detect that the second ref was bad and
> rollback the transaction before we would even try start writing the update t
s/try/try to/
s/t$/to/
> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
>
> I think this new behaviour is more correct, since if there was a problem
> we would not even try to commit the transaction but need to highlight this
> change in how/what errors are reported.
> This change in what error is returned only occurs if there are multiple
> refs that fail to update and only some, but not all, of them fail due to
> ENOTDIR.
Thanks for the detailed explanation. The change in behavior seems
reasonable to me.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@xxxxxxxxxx>
> ---
> builtin/fetch.c | 34 ++++++++++++++++------------------
> 1 file changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8cf70cd..5b0cc31 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -45,6 +45,7 @@ static struct transport *gsecondary;
> static const char *submodule_prefix = "";
> static const char *recurse_submodules_default;
> static int shown_url;
> +static struct ref_transaction *transaction;
>
> static int option_parse_recurse_submodules(const struct option *opt,
> const char *arg, int unset)
> @@ -373,27 +374,13 @@ static int s_update_ref(const char *action,
> struct ref *ref,
> int check_old)
> {
> - char msg[1024];
> - char *rla = getenv("GIT_REFLOG_ACTION");
> - struct ref_transaction *transaction;
> -
> if (dry_run)
> return 0;
> - if (!rla)
> - rla = default_rla.buf;
> - snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>
> - errno = 0;
> - transaction = ref_transaction_begin();
> - if (!transaction ||
> - ref_transaction_update(transaction, ref->name, ref->new_sha1,
> - ref->old_sha1, 0, check_old, NULL) ||
> - ref_transaction_commit(transaction, msg, NULL)) {
> - ref_transaction_free(transaction);
> - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> - STORE_REF_ERROR_OTHER;
> - }
> - ref_transaction_free(transaction);
> + if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
> + ref->old_sha1, 0, check_old, NULL))
> + return STORE_REF_ERROR_OTHER;
> +
> return 0;
> }
>
> @@ -565,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> goto abort;
> }
>
> + errno = 0;
> + transaction = ref_transaction_begin();
> + if (!transaction) {
> + rc = error(_("cannot start ref transaction\n"));
> + goto abort;
> + }
> +
> /*
> * We do a pass for each fetch_head_status type in their enum order, so
> * merged entries are written before not-for-merge. That lets readers
> @@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> }
> }
> }
> + if (ref_transaction_commit(transaction, "fetch_ref transaction", NULL))
> + rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> + STORE_REF_ERROR_OTHER;
> + ref_transaction_free(transaction);
>
> if (rc & STORE_REF_ERROR_DF_CONFLICT)
> error(_("some local refs could not be updated; try running\n"
>
--
Michael Haggerty
mhagger@xxxxxxxxxxxx
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html