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

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

[Stefan Beller]

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

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

[Stefan Beller]

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(-)

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

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

[Stefan Beller]

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(-)

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

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

[Jonathan Nieder]

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>

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

[Jonathan Nieder]

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.

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

[Stefan Beller]

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:
> 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>

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

[Jonathan Nieder]

(administrivia: please don't top-post)
Stefan Beller wrote:
> On Wed, Nov 19, 2014 at 5:10 PM, Jonathan Nieder <jrnieder@gmail.com> 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).

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

[Michael Haggerty]

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 <mhagger@alum.mit.edu>

And I love that you are proceeding in smaller pieces.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

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

[Jeff King]

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

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

[Stefan Beller]

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(-)

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

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

[Stefan Beller]

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(-)

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

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

[Jonathan Nieder]

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.

Thanks,
Jonathan

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

[Junio C Hamano]

Thanks; I think these match what I locally amended just now ;-)

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

[Jeff King]

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 <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