Jonathan Nieder <[email protected]> writes: >> https://code-review.googlesource.com/#/q/topic:ref-transaction-1 > > Outcome of the experiment: patches gained some minor changes and then > > 1-12 > Reviewed-by: Jonathan Nieder <[email protected]> > > 13 > Reviewed-by: Michael Haggerty <[email protected]> > Reviewed-by: Jonathan Nieder <[email protected]> > > 14 > Reviewed-by: Jonathan Nieder <[email protected]> > > 15-16 > Reviewed-by: Michael Haggerty <[email protected]> > Reviewed-by: Jonathan Nieder <[email protected]> > > 17 > Reviewed-by: Michael Haggerty <[email protected]> > > 18 > Reviewed-by: Michael Haggerty <[email protected]> > Reviewed-by: Jonathan Nieder <[email protected]> > > 19 > Reviewed-by: Jonathan Nieder <[email protected]> > > 20 > Reviewed-by: Michael Haggerty <[email protected]> > Reviewed-by: Jonathan Nieder <[email protected]> > > I found the web UI helpful in seeing how each patch evolved since I > last looked at it. Interdiff relative to the version in pu is below.
Thanks for the interdiff, it really helps to be able to take a glance without having to click around. It seems that I can hold the topic in 'pu' a bit longer and expect a reroll from this effort before merging it to 'next'? > The next series from Ronnie's collection is available at > https://code-review.googlesource.com/#/q/topic:ref-transaction in case > someone wants a fresh series to look at. Thanks. > diff --git a/builtin/commit.c b/builtin/commit.c > index 668fa6a..9bf1003 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1765,8 +1765,8 @@ int cmd_commit(int argc, const char **argv, const char > *prefix) > transaction = ref_transaction_begin(&err); > if (!transaction || > ref_transaction_update(transaction, "HEAD", sha1, > - current_head ? > - current_head->object.sha1 : NULL, > + current_head > + ? current_head->object.sha1 : NULL, > 0, !!current_head, &err) || > ref_transaction_commit(transaction, sb.buf, &err)) { > rollback_index_files(); Perhaps this is nicer, but probably most people would not care. > diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c > index 91099ad..224fadc 100644 > --- a/builtin/receive-pack.c > +++ b/builtin/receive-pack.c > @@ -580,18 +580,18 @@ static char *update(struct command *cmd, struct > shallow_info *si) > > if (shallow_update && si->shallow_ref[cmd->index] && > update_shallow_ref(cmd, si)) > - return xstrdup("shallow error"); > + return "shallow error"; > > transaction = ref_transaction_begin(&err); > if (!transaction || > ref_transaction_update(transaction, namespaced_name, > new_sha1, old_sha1, 0, 1, &err) || > ref_transaction_commit(transaction, "push", &err)) { > - char *str = strbuf_detach(&err, NULL); > ref_transaction_free(transaction); > > - rp_error("%s", str); > - return str; > + rp_error("%s", err.buf); > + strbuf_release(&err); > + return "failed to update ref"; > } I am guessing that the purpose of this change is to make sure that cmd->error_string is taken from a fixed set of strings, not an arbitrary collection of errors from the transaction subsystem, but what is the significance of doing so? Do the other side i18n while running print_ref_status() or something? > diff --git a/refs.h b/refs.h > index aad846c..69ef28c 100644 > --- a/refs.h > +++ b/refs.h > @@ -180,8 +180,7 @@ extern int peel_ref(const char *refname, unsigned char > *sha1); > */ > #define REF_NODEREF 0x01 > /* > - * Locks any ref (for 'HEAD' type refs) and sets errno to something > - * meaningful on failure. > + * This function sets errno to something meaningful on failure. > */ > extern struct ref_lock *lock_any_ref_for_update(const char *refname, > const unsigned char *old_sha1, To contrast this function with lock_ref_sha1() that only allowed refs under refs/ hierarchy, the comment may have made sense, but now that other function is gone, losing the comment makes sense. I removed from the above the interdiff hunks I did not have any comments or questions on. Thanks again. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
