On Fri, May 30, 2014 at 5:22 PM, Jonathan Nieder <jrnie...@gmail.com> wrote:
> Hi,
>
> Ronnie Sahlberg wrote:
>
>> For these cases, save the errno value and abort and make sure that the caller
>> can see errno==ENOTDIR.
>>
>> Also start defining specific return codes for _commit, assign -1 as a generic
>> error and -2 as the error that refers to a name conflict. Callers can (and
>> should) use that return value inspecting errno directly.
>
> Heh.  Here's a patch on top to hopefully finish that part of the job.

Thanks. Updated.

>
> Unfortunately, the value of errno after calling lock_ref_sha1_basic is
> not reliable.

I have changed the patch for lock_ref_sha1_basic so it now does :
  if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
    errno = EINVAL;
    return NULL;
  }

so this function should no longer fail without changing errno to a,
hopefully, sane value.


> http://thread.gmane.org/gmane.comp.version-control.git/249159/focus=249407
> lists the error paths that are broken (marked with "[!]" in that
> message).
>
> diff --git i/builtin/fetch.c w/builtin/fetch.c
> index b13e8f9..0a01cda 100644
> --- i/builtin/fetch.c
> +++ w/builtin/fetch.c
> @@ -377,6 +377,7 @@ static int s_update_ref(const char *action,
>         char *rla = getenv("GIT_REFLOG_ACTION");
>         struct ref_transaction *transaction;
>         struct strbuf err = STRBUF_INIT;
> +       int ret, df_conflict = 0;
>
>         if (dry_run)
>                 return 0;
> @@ -387,16 +388,22 @@ static int s_update_ref(const char *action,
>         transaction = ref_transaction_begin(&err);
>         if (!transaction ||
>             ref_transaction_update(transaction, ref->name, ref->new_sha1,
> -                                  ref->old_sha1, 0, check_old, msg, &err) ||
> -           ref_transaction_commit(transaction, &err)) {
> -               ref_transaction_free(transaction);
> -               error("%s", err.buf);
> -               strbuf_release(&err);
> -               return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> -                                         STORE_REF_ERROR_OTHER;
> -       }
> +                                  ref->old_sha1, 0, check_old, msg, &err))
> +               goto fail;
> +
> +       ret = ref_transaction_commit(transaction, &err);
> +       if (ret == UPDATE_REFS_NAME_CONFLICT)
> +               df_conflict = 1;
> +       if (ret)
> +               goto fail;
>         ref_transaction_free(transaction);
>         return 0;
> +fail:
> +       ref_transaction_free(transaction);
> +       error("%s", err.buf);
> +       strbuf_release(&err);
> +       return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
> +                          : STORE_REF_ERROR_OTHER;
>  }
>
>  #define REFCOL_WIDTH  10
> diff --git i/refs.c w/refs.c
> index dbaabba..b22b99b 100644
> --- i/refs.c
> +++ w/refs.c
> @@ -3499,7 +3499,7 @@ static int ref_update_reject_duplicates(struct 
> ref_update **updates, int n,
>  int ref_transaction_commit(struct ref_transaction *transaction,
>                            struct strbuf *err)
>  {
> -       int ret = 0, delnum = 0, i, save_errno = 0;
> +       int ret = 0, delnum = 0, i, df_conflict = 0;
>         const char **delnames;
>         int n = transaction->nr;
>         struct ref_update **updates = transaction->updates;
> @@ -3535,7 +3535,7 @@ int ref_transaction_commit(struct ref_transaction 
> *transaction,
>                                                    delnames, delnum);
>                 if (!update->lock) {
>                         if (errno == ENOTDIR)
> -                               save_errno = errno;
> +                               df_conflict = 1;
>                         if (err)
>                                 strbuf_addf(err, "Cannot lock the ref '%s'.",
>                                             update->refname);
> @@ -3590,8 +3590,7 @@ cleanup:
>                 if (updates[i]->lock)
>                         unlock_ref(updates[i]->lock);
>         free(delnames);
> -       errno = save_errno;
> -       if (save_errno == ENOTDIR)
> +       if (df_conflict)
>                 ret = -2;
>         return ret;
>  }
> diff --git i/refs.h w/refs.h
> index 88732a1..1583097 100644
> --- i/refs.h
> +++ w/refs.h
> @@ -325,9 +325,11 @@ int ref_transaction_delete(struct ref_transaction 
> *transaction,
>   * problem.
>   * If the transaction is already in failed state this function will return
>   * an error.
> - * Function returns 0 on success, -1 for generic failures and -2 if the
> - * failure was due to a name collision (ENOTDIR).
> + * Function returns 0 on success, -1 for generic failures and
> + * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
> + * collision (ENOTDIR).
>   */
> +#define UPDATE_REFS_NAME_CONFLICT -2
>  int ref_transaction_commit(struct ref_transaction *transaction,
>                            struct strbuf *err);
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to