# [PATCH 0/2] Breaking the ref-transactions-reflog series in smaller parts 14 messages from 2014-11-19 to 2014-11-20. Participants: Stefan Beller, Jonathan Nieder, Michael Haggerty, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/38012 ## Stefan Beller, 2014-11-19 21:40 Subject: [PATCH 0/2] Breaking the ref-transactions-reflog series in smaller parts Message-ID: <1416433224-29763-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1416433224-29763-1-git-send-email-sbeller%40google.com ``` Hi, so I think the patch series ref-transactions-reflog[1] sent previously to the list is still too large for easy review and I want to break it up more. So in this series we'll digest only 2 small patches, which do not seem to be controversial (yet;) and seem to be useful no matter how the discussion on the other series continues. ref_transaction_create as well as ref_transaction_delete are just special cases of ref_transaction_update, so let's reduce some redundant code. While this doesn't change code in pathes, which are supposed to be run, we do have slight changes in error messages starting with "BUG: ". So in the best case, the user will never see these messages at all. Thanks, Stefan [1] http://thread.gmane.org/gmane.comp.version-control.git/259712 Ronnie Sahlberg (2): refs.c: make ref_transaction_create a wrapper for ref_transaction_update refs.c: make ref_transaction_delete a wrapper for ref_transaction_update refs.c | 49 ++++--------------------------------------------- refs.h | 2 +- 2 files changed, 5 insertions(+), 46 deletions(-) -- 2.2.0.rc2.13.g0786cdb ``` ## Stefan Beller, 2014-11-19 21:40 Subject: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <1416433224-29763-2-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1416433224-29763-2-git-send-email-sbeller%40google.com In-Reply-To: <1416433224-29763-1-git-send-email-sbeller@google.com> ``` From: Ronnie Sahlberg The ref_transaction_update function can already be used to create refs by passing null_sha1 as the old_sha1 parameter. Simplify by replacing transaction_create with a thin wrapper. Signed-off-by: Ronnie Sahlberg Signed-off-by: Jonathan Nieder Signed-off-by: Stefan Beller --- The same as sent 2 days before as part of the ref-transactions-reflog series http://thread.gmane.org/gmane.comp.version-control.git/259712 no changes since then. refs.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/refs.c b/refs.c index 5ff457e..005eb18 100644 --- a/refs.c +++ b/refs.c @@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction *transaction, int flags, const char *msg, struct strbuf *err) { - struct ref_update *update; - - assert(err); - - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: create called for transaction that is not open"); - - if (!new_sha1 || is_null_sha1(new_sha1)) - die("BUG: create ref with null new_sha1"); - - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { - strbuf_addf(err, "refusing to create ref with bad name %s", - refname); - return -1; - } - - update = add_update(transaction, refname); - - hashcpy(update->new_sha1, new_sha1); - hashclr(update->old_sha1); - update->flags = flags; - update->have_old = 1; - if (msg) - update->msg = xstrdup(msg); - return 0; + return ref_transaction_update(transaction, refname, new_sha1, + null_sha1, flags, 1, msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, -- 2.2.0.rc2.13.g0786cdb ``` ## Stefan Beller, 2014-11-19 21:40 Subject: [PATCH 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update Message-ID: <1416433224-29763-3-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1416433224-29763-3-git-send-email-sbeller%40google.com In-Reply-To: <1416433224-29763-1-git-send-email-sbeller@google.com> ``` From: Ronnie Sahlberg Signed-off-by: Ronnie Sahlberg Signed-off-by: Jonathan Nieder Signed-off-by: Stefan Beller --- The same as sent 2 days before as part of the ref-transactions-reflog series http://thread.gmane.org/gmane.comp.version-control.git/259712 no changes since then. refs.c | 22 ++-------------------- refs.h | 2 +- 2 files changed, 3 insertions(+), 21 deletions(-) diff --git a/refs.c b/refs.c index 005eb18..05cb299 100644 --- a/refs.c +++ b/refs.c @@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction *transaction, int flags, int have_old, const char *msg, struct strbuf *err) { - struct ref_update *update; - - assert(err); - - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: delete called for transaction that is not open"); - - if (have_old && !old_sha1) - die("BUG: have_old is true but old_sha1 is NULL"); - - update = add_update(transaction, refname); - update->flags = flags; - update->have_old = have_old; - if (have_old) { - assert(!is_null_sha1(old_sha1)); - hashcpy(update->old_sha1, old_sha1); - } - if (msg) - update->msg = xstrdup(msg); - return 0; + return ref_transaction_update(transaction, refname, null_sha1, + old_sha1, flags, have_old, msg, err); } int update_ref(const char *action, const char *refname, diff --git a/refs.h b/refs.h index 2bc3556..7d675b7 100644 --- a/refs.h +++ b/refs.h @@ -283,7 +283,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); /* * Add a reference update to transaction. new_sha1 is the value that - * the reference should have after the update, or zeros if it should + * the reference should have after the update, or null_sha1 if it should * be deleted. If have_old is true, then old_sha1 holds the value * that the reference should have had before the update, or zeros if * it must not have existed beforehand. -- 2.2.0.rc2.13.g0786cdb ``` ## Jonathan Nieder, 2014-11-20 01:10 Subject: Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <20141120011038.GB6527@google.com> URL: https://gitlist.dev/e/20141120011038.GB6527%40google.com In-Reply-To: <1416433224-29763-2-git-send-email-sbeller@google.com> ``` Stefan Beller wrote: > From: Ronnie Sahlberg > > The ref_transaction_update function can already be used to create refs by > passing null_sha1 as the old_sha1 parameter. Simplify by replacing > transaction_create with a thin wrapper. > > Signed-off-by: Ronnie Sahlberg > Signed-off-by: Jonathan Nieder > Signed-off-by: Stefan Beller > --- > refs.c | 27 ++------------------------- > 1 file changed, 2 insertions(+), 25 deletions(-) I feel a bit ashamed to have my sign-off peppering all these patches that I didn't have anything to do with except preparing to send them to the list once or twice. I'd be happier if my sign-off weren't there. Except for that, Reviewed-by: Jonathan Nieder ``` ## Jonathan Nieder, 2014-11-20 01:11 Subject: Re: [PATCH 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update Message-ID: <20141120011136.GC6527@google.com> URL: https://gitlist.dev/e/20141120011136.GC6527%40google.com In-Reply-To: <1416433224-29763-3-git-send-email-sbeller@google.com> ``` Stefan Beller wrote: > refs.c | 22 ++-------------------- > refs.h | 2 +- > 2 files changed, 3 insertions(+), 21 deletions(-) Likewise: Reviewed-by: Jonathan Nieder but this shouldn't have had my sign-off. ``` ## Stefan Beller, 2014-11-20 01:12 Subject: Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: URL: https://gitlist.dev/e/CAGZ79kaW3xGjepcXFZYYUTuDqbEixVA4rH-FMcG0hyEoCVqizQ%40mail.gmail.com In-Reply-To: <20141120011038.GB6527@google.com> ``` I am sorry for not having asked before. As I picked up the patches, you had sign offs pretty much at any patch. I'll remove them from future patches when sending to the list. On Wed, Nov 19, 2014 at 5:10 PM, Jonathan Nieder wrote: > Stefan Beller wrote: > >> From: Ronnie Sahlberg >> >> The ref_transaction_update function can already be used to create refs by >> passing null_sha1 as the old_sha1 parameter. Simplify by replacing >> transaction_create with a thin wrapper. >> >> Signed-off-by: Ronnie Sahlberg >> Signed-off-by: Jonathan Nieder >> Signed-off-by: Stefan Beller >> --- >> refs.c | 27 ++------------------------- >> 1 file changed, 2 insertions(+), 25 deletions(-) > > I feel a bit ashamed to have my sign-off peppering all these patches > that I didn't have anything to do with except preparing to send them > to the list once or twice. I'd be happier if my sign-off weren't > there. > > Except for that, > Reviewed-by: Jonathan Nieder ``` ## Jonathan Nieder, 2014-11-20 01:37 Subject: Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <20141120013755.GD6527@google.com> URL: https://gitlist.dev/e/20141120013755.GD6527%40google.com In-Reply-To: ``` (administrivia: please don't top-post) Stefan Beller wrote: > On Wed, Nov 19, 2014 at 5:10 PM, Jonathan Nieder wrote: >> I feel a bit ashamed to have my sign-off peppering all these patches >> that I didn't have anything to do with except preparing to send them >> to the list once or twice. I'd be happier if my sign-off weren't >> there. > > I am sorry for not having asked before. Don't worry too much about it --- you were starting from commits that already had my name there. I was just mentioning my preference for the future (since there are a lot of these patches). ``` ## Michael Haggerty, 2014-11-20 11:03 Subject: Re: [PATCH 0/2] Breaking the ref-transactions-reflog series in smaller parts Message-ID: <546DCA85.8060806@alum.mit.edu> URL: https://gitlist.dev/e/546DCA85.8060806%40alum.mit.edu In-Reply-To: <1416433224-29763-1-git-send-email-sbeller@google.com> ``` On 11/19/2014 10:40 PM, Stefan Beller wrote: > so I think the patch series ref-transactions-reflog[1] sent previously to the list > is still too large for easy review and I want to break it up more. > So in this series we'll digest only 2 small patches, which do not seem to be > controversial (yet;) and seem to be useful no matter how the discussion on > the other series continues. > > ref_transaction_create as well as ref_transaction_delete are just special cases > of ref_transaction_update, so let's reduce some redundant code. While this doesn't > change code in pathes, which are supposed to be run, we do have slight changes > in error messages starting with "BUG: ". So in the best case, the user will never > see these messages at all. Users should *never* see error messages starting with "BUG: ". These are basically what the Git project uses instead of assert(). As long as we Git developers would understand a bug report submitted by a user who sees such an error message, their text can be changed without worries. Both patches are Reviewed-by: Michael Haggerty And I love that you are proceeding in smaller pieces. Michael -- Michael Haggerty mhagger@alum.mit.edu ``` ## Jeff King, 2014-11-20 16:00 Subject: Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <20141120160000.GA31738@peff.net> URL: https://gitlist.dev/e/20141120160000.GA31738%40peff.net In-Reply-To: <1416433224-29763-2-git-send-email-sbeller@google.com> ``` On Wed, Nov 19, 2014 at 01:40:23PM -0800, Stefan Beller wrote: > { > - struct ref_update *update; > - > - assert(err); > - > - if (transaction->state != REF_TRANSACTION_OPEN) > - die("BUG: create called for transaction that is not open"); > - > - if (!new_sha1 || is_null_sha1(new_sha1)) > - die("BUG: create ref with null new_sha1"); > - > - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { > - strbuf_addf(err, "refusing to create ref with bad name %s", > - refname); > - return -1; > - } You claimed in the cover letter that only BUG messages were changed. But I think this third one is a real user-visible message. That being said, I think the sum total of the change to the message is s/create/update/, and it's probably fine. -Peff ``` ## Stefan Beller, 2014-11-20 17:38 Subject: [PATCH v2 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <1416505090-1839-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1416505090-1839-1-git-send-email-sbeller%40google.com In-Reply-To: <20141120160000.GA31738@peff.net> ``` From: Ronnie Sahlberg The ref_transaction_update function can already be used to create refs by passing null_sha1 as the old_sha1 parameter. Simplify by replacing transaction_create with a thin wrapper. Signed-off-by: Ronnie Sahlberg Signed-off-by: Stefan Beller Reviewed-by: Michael Haggerty Reviewed-by: Jonathan Nieder --- no changes in code, just removing Jonathans sign off, adding reviewed-by by Michael and Jonathan refs.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/refs.c b/refs.c index 5ff457e..005eb18 100644 --- a/refs.c +++ b/refs.c @@ -3623,31 +3623,8 @@ int ref_transaction_create(struct ref_transaction *transaction, int flags, const char *msg, struct strbuf *err) { - struct ref_update *update; - - assert(err); - - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: create called for transaction that is not open"); - - if (!new_sha1 || is_null_sha1(new_sha1)) - die("BUG: create ref with null new_sha1"); - - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { - strbuf_addf(err, "refusing to create ref with bad name %s", - refname); - return -1; - } - - update = add_update(transaction, refname); - - hashcpy(update->new_sha1, new_sha1); - hashclr(update->old_sha1); - update->flags = flags; - update->have_old = 1; - if (msg) - update->msg = xstrdup(msg); - return 0; + return ref_transaction_update(transaction, refname, new_sha1, + null_sha1, flags, 1, msg, err); } int ref_transaction_delete(struct ref_transaction *transaction, -- 2.2.0.rc2.23.gca0107e ``` ## Stefan Beller, 2014-11-20 17:39 Subject: [PATCH v2 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update Message-ID: <1416505150-1913-1-git-send-email-sbeller@google.com> URL: https://gitlist.dev/e/1416505150-1913-1-git-send-email-sbeller%40google.com In-Reply-To: <20141120011136.GC6527@google.com> ``` From: Ronnie Sahlberg Signed-off-by: Ronnie Sahlberg Signed-off-by: Stefan Beller Reviewed-by: Michael Haggerty Reviewed-by: Jonathan Nieder --- no changes in code, just removing Jonathans sign off, adding reviewed-by by Michael and Jonathan refs.c | 22 ++-------------------- refs.h | 2 +- 2 files changed, 3 insertions(+), 21 deletions(-) diff --git a/refs.c b/refs.c index 005eb18..05cb299 100644 --- a/refs.c +++ b/refs.c @@ -3633,26 +3633,8 @@ int ref_transaction_delete(struct ref_transaction *transaction, int flags, int have_old, const char *msg, struct strbuf *err) { - struct ref_update *update; - - assert(err); - - if (transaction->state != REF_TRANSACTION_OPEN) - die("BUG: delete called for transaction that is not open"); - - if (have_old && !old_sha1) - die("BUG: have_old is true but old_sha1 is NULL"); - - update = add_update(transaction, refname); - update->flags = flags; - update->have_old = have_old; - if (have_old) { - assert(!is_null_sha1(old_sha1)); - hashcpy(update->old_sha1, old_sha1); - } - if (msg) - update->msg = xstrdup(msg); - return 0; + return ref_transaction_update(transaction, refname, null_sha1, + old_sha1, flags, have_old, msg, err); } int update_ref(const char *action, const char *refname, diff --git a/refs.h b/refs.h index 2bc3556..7d675b7 100644 --- a/refs.h +++ b/refs.h @@ -283,7 +283,7 @@ struct ref_transaction *ref_transaction_begin(struct strbuf *err); /* * Add a reference update to transaction. new_sha1 is the value that - * the reference should have after the update, or zeros if it should + * the reference should have after the update, or null_sha1 if it should * be deleted. If have_old is true, then old_sha1 holds the value * that the reference should have had before the update, or zeros if * it must not have existed beforehand. -- 2.2.0.rc2.23.gca0107e ``` ## Jonathan Nieder, 2014-11-20 18:03 Subject: Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <20141120180315.GA15945@google.com> URL: https://gitlist.dev/e/20141120180315.GA15945%40google.com In-Reply-To: <20141120160000.GA31738@peff.net> ``` Jeff King wrote: > On Wed, Nov 19, 2014 at 01:40:23PM -0800, Stefan Beller wrote: >> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { >> - strbuf_addf(err, "refusing to create ref with bad name %s", >> - refname); >> - return -1; >> - } > > You claimed in the cover letter that only BUG messages were changed. But > I think this third one is a real user-visible message. > > That being said, I think the sum total of the change to the message is > s/create/update/, and it's probably fine. Yeah, it we want to get the 'create' back, we could handle it by checking if old_sha1 is null_sha1 in transaction_update (that would take of other callers, too, like 'git update-ref ""'). But I haven't convinced myself that's worth the complication --- in any event it could be a separate patch to avoid blocking this one. Thanks, Jonathan ``` ## Junio C Hamano, 2014-11-20 18:05 Subject: Re: [PATCH v2 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update Message-ID: URL: https://gitlist.dev/e/xmqq1tox4ujw.fsf%40gitster.dls.corp.google.com In-Reply-To: <1416505150-1913-1-git-send-email-sbeller@google.com> ``` Thanks; I think these match what I locally amended just now ;-) ``` ## Jeff King, 2014-11-20 18:18 Subject: Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update Message-ID: <20141120181831.GC2559@peff.net> URL: https://gitlist.dev/e/20141120181831.GC2559%40peff.net In-Reply-To: <20141120180315.GA15945@google.com> ``` On Thu, Nov 20, 2014 at 10:03:15AM -0800, Jonathan Nieder wrote: > Jeff King wrote: > > On Wed, Nov 19, 2014 at 01:40:23PM -0800, Stefan Beller wrote: > > >> - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) { > >> - strbuf_addf(err, "refusing to create ref with bad name %s", > >> - refname); > >> - return -1; > >> - } > > > > You claimed in the cover letter that only BUG messages were changed. But > > I think this third one is a real user-visible message. > > > > That being said, I think the sum total of the change to the message is > > s/create/update/, and it's probably fine. > > Yeah, it we want to get the 'create' back, we could handle it by > checking if old_sha1 is null_sha1 in transaction_update (that would > take of other callers, too, like 'git update-ref ""'). > But I haven't convinced myself that's worth the complication --- in > any event it could be a separate patch to avoid blocking this one. Agreed on all of that. -Peff ```