subject:"\[PATCH v2 20\/27\] update\-ref \-\-stdin\: Reimplement using reference transactions"

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

2014-04-03 Thread Junio C Hamano

Michael Haggerty mhag...@alum.mit.edu writes:

 I assumed that rolling back a non-consummated transaction in the case of
 early program death should be the responsibility of the library, not of
 the caller.  If I'm correct, the caller(s) won't have to be modified
 when the atexit facility is added, so I don't see a reason to add it
 before it is needed by a concrete backend.

 But you suggest that the caller should be involved.

I didn't say should.  If the library can automatically rollback
without being called upon die() anywhere in the system, that is
better.  The suggestion was because I didn't think you were shooting
for such a completeness in the library part, and a possible way out
is for the caller to help.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

2014-04-03 Thread Michael Haggerty

On 04/03/2014 05:57 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 I assumed that rolling back a non-consummated transaction in the case of
 early program death should be the responsibility of the library, not of
 the caller.  If I'm correct, the caller(s) won't have to be modified
 when the atexit facility is added, so I don't see a reason to add it
 before it is needed by a concrete backend.

 But you suggest that the caller should be involved.
 
 I didn't say should.  If the library can automatically rollback
 without being called upon die() anywhere in the system, that is
 better.  The suggestion was because I didn't think you were shooting
 for such a completeness in the library part, and a possible way out
 is for the caller to help.

I was assuming that any ref backends that required rollback-on-fail
would register an atexit handler and a signal handler, similar to how
lock_file rollbacks are done.

I admit that I haven't thought through all the details; for example, are
there restrictions on the things that a signal handler is allowed to do
that would preclude its being able to rollback the types of transactions
that back ends might want to implement?  (Though if so, what hope do we
have that the caller can do better?)

So, if somebody can think of a reason that we would need to involve the
caller in cleanup, please speak up.  Otherwise I think it would be less
error-prone to leave this responsibility with the individual back ends.
 (And if something unexpected comes up, we can make this change later.)

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

2014-04-01 Thread Junio C Hamano

Michael Haggerty mhag...@alum.mit.edu writes:

 This change is mostly clerical: the parse_cmd_*() functions need to
 use local variables rather than a struct ref_update to collect the
 arguments needed for each update, and then call ref_transaction_*() to
 queue the change rather than building up the list of changes at the
 caller side.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---

With the implementation of ref_transaction at this point in the
series it does not matter, but the updated code in this patch means
that it is perfectly acceptable to do this sequence:

ref_transaction_begin();
ref_transaction_update();
...
ref_transaction_update();
die();

without ever calling ref_transaction_rollback() API function.
Depending on the future backends, we may want to ensure rollback is
called, no?  And if that is the case, we would want to prepare
callers of the API with some at-exit facility to call rollback, no?

Other than that, the code looks straight-forward.

Thanks.

  builtin/update-ref.c | 142 
 +++
  1 file changed, 75 insertions(+), 67 deletions(-)

 diff --git a/builtin/update-ref.c b/builtin/update-ref.c
 index bbc04b2..2c8678b 100644
 --- a/builtin/update-ref.c
 +++ b/builtin/update-ref.c
 @@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
   NULL
  };
  
 -static int updates_alloc;
 -static int updates_count;
 -static struct ref_update **updates;
 +static struct ref_transaction *transaction;
  
  static char line_termination = '\n';
  static int update_flags;
  
 -static struct ref_update *update_alloc(void)
 -{
 - struct ref_update *update;
 -
 - /* Allocate and zero-init a struct ref_update */
 - update = xcalloc(1, sizeof(*update));
 - ALLOC_GROW(updates, updates_count + 1, updates_alloc);
 - updates[updates_count++] = update;
 -
 - /* Store and reset accumulated options */
 - update-flags = update_flags;
 - update_flags = 0;
 -
 - return update;
 -}
 -
  /*
   * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
   * and append the result to arg.  Return a pointer to the terminator.
 @@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const 
 char **next,
  
  static const char *parse_cmd_update(struct strbuf *input, const char *next)
  {
 - struct ref_update *update;
 -
 - update = update_alloc();
 + char *refname;
 + unsigned char new_sha1[20];
 + unsigned char old_sha1[20];
 + int have_old;
  
 - update-ref_name = parse_refname(input, next);
 - if (!update-ref_name)
 + refname = parse_refname(input, next);
 + if (!refname)
   die(update: missing ref);
  
 - if (parse_next_sha1(input, next, update-new_sha1,
 - update, update-ref_name,
 + if (parse_next_sha1(input, next, new_sha1, update, refname,
   PARSE_SHA1_ALLOW_EMPTY))
 - die(update %s: missing newvalue, update-ref_name);
 + die(update %s: missing newvalue, refname);
  
 - update-have_old = !parse_next_sha1(input, next, update-old_sha1,
 - update, update-ref_name,
 - PARSE_SHA1_OLD);
 + have_old = !parse_next_sha1(input, next, old_sha1, update, refname,
 + PARSE_SHA1_OLD);
  
   if (*next != line_termination)
 - die(update %s: extra input: %s, update-ref_name, next);
 + die(update %s: extra input: %s, refname, next);
 +
 + ref_transaction_update(transaction, refname, new_sha1, old_sha1,
 +update_flags, have_old);
 +
 + update_flags = 0;
 + free(refname);
  
   return next;
  }
  
  static const char *parse_cmd_create(struct strbuf *input, const char *next)
  {
 - struct ref_update *update;
 -
 - update = update_alloc();
 + char *refname;
 + unsigned char new_sha1[20];
  
 - update-ref_name = parse_refname(input, next);
 - if (!update-ref_name)
 + refname = parse_refname(input, next);
 + if (!refname)
   die(create: missing ref);
  
 - if (parse_next_sha1(input, next, update-new_sha1,
 - create, update-ref_name, 0))
 - die(create %s: missing newvalue, update-ref_name);
 + if (parse_next_sha1(input, next, new_sha1, create, refname, 0))
 + die(create %s: missing newvalue, refname);
  
 - if (is_null_sha1(update-new_sha1))
 - die(create %s: zero newvalue, update-ref_name);
 + if (is_null_sha1(new_sha1))
 + die(create %s: zero newvalue, refname);
  
   if (*next != line_termination)
 - die(create %s: extra input: %s, update-ref_name, next);
 + die(create %s: extra input: %s, refname, next);
 +
 + ref_transaction_create(transaction, refname, new_sha1, update_flags);
 +
 +

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

2014-04-01 Thread Michael Haggerty

On 04/01/2014 09:46 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 This change is mostly clerical: the parse_cmd_*() functions need to
 use local variables rather than a struct ref_update to collect the
 arguments needed for each update, and then call ref_transaction_*() to
 queue the change rather than building up the list of changes at the
 caller side.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
 
 With the implementation of ref_transaction at this point in the
 series it does not matter, but the updated code in this patch means
 that it is perfectly acceptable to do this sequence:
 
 ref_transaction_begin();
 ref_transaction_update();
 ...
 ref_transaction_update();
 die();
 
 without ever calling ref_transaction_rollback() API function.
 Depending on the future backends, we may want to ensure rollback is
 called, no?  And if that is the case, we would want to prepare
 callers of the API with some at-exit facility to call rollback, no?

I assumed that rolling back a non-consummated transaction in the case of
early program death should be the responsibility of the library, not of
the caller.  If I'm correct, the caller(s) won't have to be modified
when the atexit facility is added, so I don't see a reason to add it
before it is needed by a concrete backend.

But you suggest that the caller should be involved.  Do you have an idea
for something that a caller might want to do besides roll back the
transaction?

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

2014-03-24 Thread Michael Haggerty

This change is mostly clerical: the parse_cmd_*() functions need to
use local variables rather than a struct ref_update to collect the
arguments needed for each update, and then call ref_transaction_*() to
queue the change rather than building up the list of changes at the
caller side.

Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
 builtin/update-ref.c | 142 +++
 1 file changed, 75 insertions(+), 67 deletions(-)

diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index bbc04b2..2c8678b 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -12,29 +12,11 @@ static const char * const git_update_ref_usage[] = {
NULL
 };
 
-static int updates_alloc;
-static int updates_count;
-static struct ref_update **updates;
+static struct ref_transaction *transaction;
 
 static char line_termination = '\n';
 static int update_flags;
 
-static struct ref_update *update_alloc(void)
-{
-   struct ref_update *update;
-
-   /* Allocate and zero-init a struct ref_update */
-   update = xcalloc(1, sizeof(*update));
-   ALLOC_GROW(updates, updates_count + 1, updates_alloc);
-   updates[updates_count++] = update;
-
-   /* Store and reset accumulated options */
-   update-flags = update_flags;
-   update_flags = 0;
-
-   return update;
-}
-
 /*
  * Parse one whitespace- or NUL-terminated, possibly C-quoted argument
  * and append the result to arg.  Return a pointer to the terminator.
@@ -196,97 +178,119 @@ static int parse_next_sha1(struct strbuf *input, const 
char **next,
 
 static const char *parse_cmd_update(struct strbuf *input, const char *next)
 {
-   struct ref_update *update;
-
-   update = update_alloc();
+   char *refname;
+   unsigned char new_sha1[20];
+   unsigned char old_sha1[20];
+   int have_old;
 
-   update-ref_name = parse_refname(input, next);
-   if (!update-ref_name)
+   refname = parse_refname(input, next);
+   if (!refname)
die(update: missing ref);
 
-   if (parse_next_sha1(input, next, update-new_sha1,
-   update, update-ref_name,
+   if (parse_next_sha1(input, next, new_sha1, update, refname,
PARSE_SHA1_ALLOW_EMPTY))
-   die(update %s: missing newvalue, update-ref_name);
+   die(update %s: missing newvalue, refname);
 
-   update-have_old = !parse_next_sha1(input, next, update-old_sha1,
-   update, update-ref_name,
-   PARSE_SHA1_OLD);
+   have_old = !parse_next_sha1(input, next, old_sha1, update, refname,
+   PARSE_SHA1_OLD);
 
if (*next != line_termination)
-   die(update %s: extra input: %s, update-ref_name, next);
+   die(update %s: extra input: %s, refname, next);
+
+   ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+  update_flags, have_old);
+
+   update_flags = 0;
+   free(refname);
 
return next;
 }
 
 static const char *parse_cmd_create(struct strbuf *input, const char *next)
 {
-   struct ref_update *update;
-
-   update = update_alloc();
+   char *refname;
+   unsigned char new_sha1[20];
 
-   update-ref_name = parse_refname(input, next);
-   if (!update-ref_name)
+   refname = parse_refname(input, next);
+   if (!refname)
die(create: missing ref);
 
-   if (parse_next_sha1(input, next, update-new_sha1,
-   create, update-ref_name, 0))
-   die(create %s: missing newvalue, update-ref_name);
+   if (parse_next_sha1(input, next, new_sha1, create, refname, 0))
+   die(create %s: missing newvalue, refname);
 
-   if (is_null_sha1(update-new_sha1))
-   die(create %s: zero newvalue, update-ref_name);
+   if (is_null_sha1(new_sha1))
+   die(create %s: zero newvalue, refname);
 
if (*next != line_termination)
-   die(create %s: extra input: %s, update-ref_name, next);
+   die(create %s: extra input: %s, refname, next);
+
+   ref_transaction_create(transaction, refname, new_sha1, update_flags);
+
+   update_flags = 0;
+   free(refname);
 
return next;
 }
 
 static const char *parse_cmd_delete(struct strbuf *input, const char *next)
 {
-   struct ref_update *update;
+   char *refname;
+   unsigned char old_sha1[20];
+   int have_old;
 
-   update = update_alloc();
-
-   update-ref_name = parse_refname(input, next);
-   if (!update-ref_name)
+   refname = parse_refname(input, next);
+   if (!refname)
die(delete: missing ref);
 
-   if (parse_next_sha1(input, next, update-old_sha1,
-   delete, update-ref_name, PARSE_SHA1_OLD)) {
-   update-have_old = 0;
+

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

Re: [PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

[PATCH v2 20/27] update-ref --stdin: Reimplement using reference transactions

5 matches

Site Navigation

Mail list logo

Footer information