Re: [PATCH v2 02/13] refs.c: use a single exit path from transaction commit and handle onerr

Ronnie Sahlberg [off-list ref] writes:

I think this is a response to Michael's earlier review "do different
callers want to give different error messages more suitable for the
situation?".  I suspect that the ideal endgame may end up all
callers passing QUIET_ON_ERR and doing the error message themselves,
e.g. branch.c::craete_branch() may end something like this:

    ...
    if (!dont_change_ref)
        if (ref_transaction_commit(..., UPDATE_REFS_QUIET_ON_ERR))
                die_errno(_("Failed to write branch '%s'"),
                          skip_prefix(ref.buf, "refs/heads/));

And in the meantime until the callers are converted, we may end up
showing the fallback "Cannot commit transaction." but that would be
OK during the development and polishing of this series.

On Tue, Apr 22, 2014 at 12:11 PM, Junio C Hamano [off-list ref] wrote:

Ronnie Sahlberg [off-list ref] writes:

quoted

diff --git a/refs.c b/refs.c
index 138ab70..9daf89e 100644
--- a/refs.c
+++ b/refs.c

@@ -3414,12 +3414,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,
                         const char *msg, enum action_on_err onerr)

...
+     if (ret) {
+             const char *str = "Cannot commit transaction.";
+             switch (onerr) {
+             case UPDATE_REFS_MSG_ON_ERR: error(str); break;
+             case UPDATE_REFS_DIE_ON_ERR: die(str); break;
+             case UPDATE_REFS_QUIET_ON_ERR: break;
+             }
+     }
      return ret;
 }

I think this is a response to Michael's earlier review "do different
callers want to give different error messages more suitable for the
situation?".  I suspect that the ideal endgame may end up all
callers passing QUIET_ON_ERR and doing the error message themselves,
e.g. branch.c::craete_branch() may end something like this:

    ...
    if (!dont_change_ref)
        if (ref_transaction_commit(..., UPDATE_REFS_QUIET_ON_ERR))
                die_errno(_("Failed to write branch '%s'"),
                          skip_prefix(ref.buf, "refs/heads/));

And in the meantime until the callers are converted, we may end up
showing the fallback "Cannot commit transaction." but that would be
OK during the development and polishing of this series.

That is a good idea.
I will try to address that in the next respin.