threads / patch / 38012

patch, 2 partsBreaking the ref-transactions-reflog series in smaller parts

Subject: [PATCH 0/2] Breaking the ref-transactions-reflog series in smaller parts

## tl;dr

14 messages between Nov 19, 2014 and Nov 20, 2014. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Stefan Beller· Nov 19, 2014, 21:40 UTC · lore
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· Nov 19, 2014, 21:40 UTC · re: Stefan Beller · lore

[PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

From: Ronnie Sahlberg <sahlberg@google.com>

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 <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)
Show changes to refs.c +2 −25
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
Jonathan Nieder· Nov 20, 2014, 01:10 UTC · re: Stefan Beller · lore

Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

Stefan Beller wrote:
Show 12 quoted lines
> From: Ronnie Sahlberg <sahlberg@google.com>
>
> 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 <sahlberg@google.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  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 <jrnieder@gmail.com>
Stefan Beller· Nov 20, 2014, 01:12 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

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 <jrnieder@gmail.com> wrote:
Show 22 quoted lines
> Stefan Beller wrote:
>
>> From: Ronnie Sahlberg <sahlberg@google.com>
>>
>> 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 <sahlberg@google.com>
>> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>  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 <jrnieder@gmail.com>
Jonathan Nieder· Nov 20, 2014, 01:37 UTC · re: Stefan Beller · lore

Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

(administrivia: please don't top-post) Stefan Beller wrote:

> On Wed, Nov 19, 2014 at 5:10 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
>> 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).

Jeff King· Nov 20, 2014, 16:00 UTC · re: Stefan Beller · lore

Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

On Wed, Nov 19, 2014 at 01:40:23PM -0800, Stefan Beller wrote:
Show 16 quoted lines
>  {
> -	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· Nov 20, 2014, 17:38 UTC · re: Jeff King · lore

[PATCH v2 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

From: Ronnie Sahlberg <sahlberg@google.com>

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 <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

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(-)
Show changes to refs.c +2 −25
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
Jonathan Nieder· Nov 20, 2014, 18:03 UTC · re: Jeff King · lore

Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

Jeff King wrote:
> On Wed, Nov 19, 2014 at 01:40:23PM -0800, Stefan Beller wrote:
Show 11 quoted lines
>> -	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 <ref> <commit> ""'). 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

Jeff King· Nov 20, 2014, 18:18 UTC · re: Jonathan Nieder · lore

Re: [PATCH 1/2] refs.c: make ref_transaction_create a wrapper for ref_transaction_update

On Thu, Nov 20, 2014 at 10:03:15AM -0800, Jonathan Nieder wrote:
Show 20 quoted lines
> 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 <ref> <commit> ""').
> 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
Stefan Beller· Nov 19, 2014, 21:40 UTC · re: Stefan Beller · lore

[PATCH 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update

From: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
 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(-)
Show changes to 2 files +3 −21

refs.c, refs.h

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· Nov 20, 2014, 01:11 UTC · re: Stefan Beller · lore

Re: [PATCH 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update

Stefan Beller wrote:
>  refs.c | 22 ++--------------------
>  refs.h |  2 +-
>  2 files changed, 3 insertions(+), 21 deletions(-)
Likewise:
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
but this shouldn't have had my sign-off.
Stefan Beller· Nov 20, 2014, 17:39 UTC · re: Jonathan Nieder · lore

[PATCH v2 2/2] refs.c: make ref_transaction_delete a wrapper for ref_transaction_update

From: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---

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(-)
Show changes to 2 files +3 −21

refs.c, refs.h

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
Michael Haggerty· Nov 20, 2014, 11:03 UTC · re: Stefan Beller · lore

Re: [PATCH 0/2] Breaking the ref-transactions-reflog series in smaller parts

On 11/19/2014 10:40 PM, Stefan Beller wrote:
Show 11 quoted lines
> 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 <mhagger@alum.mit.edu>
And I love that you are proceeding in smaller pieces.
Michael
-- 
Michael Haggerty
mhagger@alum.mit.edu

← back to recent threads