[PATCH 0/3] Use ref transactions for fetch

[PATCH 0/3] Use ref transactions for fetch

[Ronnie Sahlberg]

This change is based on the previous ref transaction patches.
This is sent as a separate patch series since it implements a lot more
non-trivial changes to the behaviour than the previous patches and thus can
use more detailed review.

Update fetch.c to use ref transactions when performing updates. Use a single
ref transaction for all updates and only commit the transaction if all other
checks and oeprations have been successful. This makes the ref updates during
a fetch (mostly) atomic.

Ronnie Sahlberg (3):
  fetch.c: clear errno before calling functions that might set it
  fetch.c: change s_update_ref to use a ref transaction
  fetch.c: use a single ref transaction for all ref updates

 builtin/fetch.c | 39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

-- 
1.9.1.518.g16976cb.dirty

[PATCH 1/3] fetch.c: clear errno before calling functions that might set it

[Ronnie Sahlberg]

In s_update_ref there are two calls that when they fail we return an error
based on the errno value. In particular we want to return a specific error
if ENOTDIR happened. Both these functions do have failure modes where they
may return an error without updating errno, in which case a previous and
unrelated ENOTDIT may cause us to return the wrong error. Clear errno before
calling any functions if we check errno afterwards.

Also skip initializing a static variable to 0. Sstatics live in .bss and
are all automatically initialized to 0.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..a93c893 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,7 +44,7 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
-static int shown_url = 0;
+static int shown_url;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
 	if (!rla)
 		rla = default_rla.buf;
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
+
+	errno = 0;
 	lock = lock_any_ref_for_update(ref->name,
 				       check_old ? ref->old_sha1 : NULL,
 				       0, NULL);
-- 
1.9.1.518.g16976cb.dirty

[PATCH 2/3] fetch.c: change s_update_ref to use a ref transaction

[Ronnie Sahlberg]

Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index a93c893..5c15584 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
 {
 	char msg[1024];
 	char *rla = getenv("GIT_REFLOG_ACTION");
-	static struct ref_lock *lock;
+	struct ref_transaction *transaction;
 
 	if (dry_run)
 		return 0;
@@ -384,15 +384,14 @@ static int s_update_ref(const char *action,
 	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
 	errno = 0;
-	lock = lock_any_ref_for_update(ref->name,
-				       check_old ? ref->old_sha1 : NULL,
-				       0, NULL);
-	if (!lock)
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
-	if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
+	transaction = ref_transaction_begin();
+	if (!transaction ||
+	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
+				   ref->old_sha1, 0, check_old) ||
+	    ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))
 		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
 					  STORE_REF_ERROR_OTHER;
+
 	return 0;
 }
 
-- 
1.9.1.518.g16976cb.dirty

[PATCH 3/3] fetch.c: use a single ref transaction for all ref updates

[Ronnie Sahlberg]

Change store_updated_refs to use a single ref transaction for all refs that
are updated during the fetch. This makes the fetch more atomic when update
failures occur.

Since ref update failures will now no longer occur in the code path for
updating a single ref in s_update_ref, we no longer have as detailed error
message logging the exact reference and the ref log action as in the old code.
Instead since we fail the entire transaction we log a much more generic
message. But since we commit the transaction using MSG_ON_ERR we will log
an error containing the ref name if either locking of writing the ref would
so the regression in the log message is minor.

This will also change the order in which errors are checked for and logged
which may alter which error will be logged if there are multiple errors
occuring during a fetch.

For example, assuming we have a fetch for two refs that both would fail.
Where the first ref would fail with ENOTDIR due to a directory in the ref
path not existing, and the second ref in the fetch would fail due to
the check in update_logical_ref():
	if (current_branch &&
	    !strcmp(ref->name, current_branch->name) &&
	    !(update_head_ok || is_bare_repository()) &&
	    !is_null_sha1(ref->old_sha1)) {
		/*
		 * If this is the head, and it's not okay to update
		 * the head, and the old value of the head isn't empty...
		 */

In the old code sicne we would update the refs one ref at a time we would
first fail the ENOTDIR and then fail the second update of HEAD as well.
But since the first ref failed with ENOTDIR we would eventually fail the whole
fetch with STORE_REF_ERROR_DF_CONFLICT

In the new code, since we defer committing the transaction until all refs
has been processed, we would now detect that the second ref was bad and
rollback the transaction before we would even try start writing the update to
disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.

I think this new behaviour is more correct, since if there was a problem
we would not even try to commit the transaction but need to highlight this
change in how/what errors are reported.
This change in what error is returned only occurs if there are multiple
refs that fail to update and only some, but not all, of them fail due to
ENOTDIR.

Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
---
 builtin/fetch.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 5c15584..97c6b9a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,7 @@ static struct transport *gsecondary;
 static const char *submodule_prefix = "";
 static const char *recurse_submodules_default;
 static int shown_url;
+struct ref_transaction *transaction;
 
 static int option_parse_recurse_submodules(const struct option *opt,
 				   const char *arg, int unset)
@@ -373,24 +374,12 @@ static int s_update_ref(const char *action,
 			struct ref *ref,
 			int check_old)
 {
-	char msg[1024];
-	char *rla = getenv("GIT_REFLOG_ACTION");
-	struct ref_transaction *transaction;
-
 	if (dry_run)
 		return 0;
-	if (!rla)
-		rla = default_rla.buf;
-	snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 
-	errno = 0;
-	transaction = ref_transaction_begin();
-	if (!transaction ||
-	    ref_transaction_update(transaction, ref->name, ref->new_sha1,
-				   ref->old_sha1, 0, check_old) ||
-	    ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))
-		return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
-					  STORE_REF_ERROR_OTHER;
+	if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
+			       ref->old_sha1, 0, check_old))
+		return STORE_REF_ERROR_OTHER;
 
 	return 0;
 }
@@ -563,6 +552,13 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		goto abort;
 	}
 
+	errno = 0;
+	transaction = ref_transaction_begin();
+	if (!transaction) {
+		rc = error(_("cannot start ref transaction\n"));
+		goto abort;
+	}
+
 	/*
 	 * We do a pass for each fetch_head_status type in their enum order, so
 	 * merged entries are written before not-for-merge. That lets readers
@@ -674,6 +670,14 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 			}
 		}
 	}
+	if (rc)
+		ref_transaction_rollback(transaction);
+	else {
+		if (ref_transaction_commit(transaction, "fetch_ref transaction",
+					   UPDATE_REFS_MSG_ON_ERR))
+			rc |= errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
+					  STORE_REF_ERROR_OTHER;
+	}
 
 	if (rc & STORE_REF_ERROR_DF_CONFLICT)
 		error(_("some local refs could not be updated; try running\n"
-- 
1.9.1.518.g16976cb.dirty

Re: [PATCH 1/3] fetch.c: clear errno before calling functions that might set it

[Eric Sunshine]

On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> In s_update_ref there are two calls that when they fail we return an error
> based on the errno value. In particular we want to return a specific error
> if ENOTDIR happened. Both these functions do have failure modes where they
> may return an error without updating errno, in which case a previous and
> unrelated ENOTDIT may cause us to return the wrong error. Clear errno before
> calling any functions if we check errno afterwards.
>
> Also skip initializing a static variable to 0. Sstatics live in .bss and

s/Sstatics/Statics/

> are all automatically initialized to 0.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
> ---
>  builtin/fetch.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 55f457c..a93c893 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>  static struct transport *gsecondary;
>  static const char *submodule_prefix = "";
>  static const char *recurse_submodules_default;
> -static int shown_url = 0;
> +static int shown_url;
>
>  static int option_parse_recurse_submodules(const struct option *opt,
>                                    const char *arg, int unset)
> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>         if (!rla)
>                 rla = default_rla.buf;
>         snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> +
> +       errno = 0;
>         lock = lock_any_ref_for_update(ref->name,
>                                        check_old ? ref->old_sha1 : NULL,
>                                        0, NULL);
> --
> 1.9.1.518.g16976cb.dirty

Re: [PATCH 2/3] fetch.c: change s_update_ref to use a ref transaction

[Eric Sunshine]

On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> Change s_update_ref to use a ref transaction for the ref update.
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>

Doubled sign-off.

> ---
>  builtin/fetch.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index a93c893..5c15584 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
>  {
>         char msg[1024];
>         char *rla = getenv("GIT_REFLOG_ACTION");
> -       static struct ref_lock *lock;
> +       struct ref_transaction *transaction;
>
>         if (dry_run)
>                 return 0;
> @@ -384,15 +384,14 @@ static int s_update_ref(const char *action,
>         snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>
>         errno = 0;
> -       lock = lock_any_ref_for_update(ref->name,
> -                                      check_old ? ref->old_sha1 : NULL,
> -                                      0, NULL);
> -       if (!lock)
> -               return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> -                                         STORE_REF_ERROR_OTHER;
> -       if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
> +       transaction = ref_transaction_begin();
> +       if (!transaction ||
> +           ref_transaction_update(transaction, ref->name, ref->new_sha1,
> +                                  ref->old_sha1, 0, check_old) ||
> +           ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))
>                 return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
>                                           STORE_REF_ERROR_OTHER;
> +
>         return 0;
>  }
>
> --
> 1.9.1.518.g16976cb.dirty

Re: [PATCH 3/3] fetch.c: use a single ref transaction for all ref updates

[Eric Sunshine]

On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
> Change store_updated_refs to use a single ref transaction for all refs that
> are updated during the fetch. This makes the fetch more atomic when update
> failures occur.
>
> Since ref update failures will now no longer occur in the code path for
> updating a single ref in s_update_ref, we no longer have as detailed error
> message logging the exact reference and the ref log action as in the old code.
> Instead since we fail the entire transaction we log a much more generic
> message. But since we commit the transaction using MSG_ON_ERR we will log
> an error containing the ref name if either locking of writing the ref would
> so the regression in the log message is minor.
>
> This will also change the order in which errors are checked for and logged
> which may alter which error will be logged if there are multiple errors
> occuring during a fetch.
>
> For example, assuming we have a fetch for two refs that both would fail.

s/assuming/assume/ perhaps?

> Where the first ref would fail with ENOTDIR due to a directory in the ref
> path not existing, and the second ref in the fetch would fail due to
> the check in update_logical_ref():
>         if (current_branch &&
>             !strcmp(ref->name, current_branch->name) &&
>             !(update_head_ok || is_bare_repository()) &&
>             !is_null_sha1(ref->old_sha1)) {
>                 /*
>                  * If this is the head, and it's not okay to update
>                  * the head, and the old value of the head isn't empty...
>                  */
>
> In the old code sicne we would update the refs one ref at a time we would

s/sicne/since/

> first fail the ENOTDIR and then fail the second update of HEAD as well.
> But since the first ref failed with ENOTDIR we would eventually fail the whole
> fetch with STORE_REF_ERROR_DF_CONFLICT
>
> In the new code, since we defer committing the transaction until all refs
> has been processed, we would now detect that the second ref was bad and

s/has/have/

> rollback the transaction before we would even try start writing the update to
> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
>
> I think this new behaviour is more correct, since if there was a problem
> we would not even try to commit the transaction but need to highlight this
> change in how/what errors are reported.
> This change in what error is returned only occurs if there are multiple
> refs that fail to update and only some, but not all, of them fail due to
> ENOTDIR.
>
> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>

Re: [PATCH 1/3] fetch.c: clear errno before calling functions that might set it

[Ronnie Sahlberg]

Fixed. Thanks.

On Wed, Apr 23, 2014 at 1:12 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> In s_update_ref there are two calls that when they fail we return an error
>> based on the errno value. In particular we want to return a specific error
>> if ENOTDIR happened. Both these functions do have failure modes where they
>> may return an error without updating errno, in which case a previous and
>> unrelated ENOTDIT may cause us to return the wrong error. Clear errno before
>> calling any functions if we check errno afterwards.
>>
>> Also skip initializing a static variable to 0. Sstatics live in .bss and
>
> s/Sstatics/Statics/
>
>> are all automatically initialized to 0.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>> ---
>>  builtin/fetch.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 55f457c..a93c893 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -44,7 +44,7 @@ static struct transport *gtransport;
>>  static struct transport *gsecondary;
>>  static const char *submodule_prefix = "";
>>  static const char *recurse_submodules_default;
>> -static int shown_url = 0;
>> +static int shown_url;
>>
>>  static int option_parse_recurse_submodules(const struct option *opt,
>>                                    const char *arg, int unset)
>> @@ -382,6 +382,8 @@ static int s_update_ref(const char *action,
>>         if (!rla)
>>                 rla = default_rla.buf;
>>         snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>> +
>> +       errno = 0;
>>         lock = lock_any_ref_for_update(ref->name,
>>                                        check_old ? ref->old_sha1 : NULL,
>>                                        0, NULL);
>> --
>> 1.9.1.518.g16976cb.dirty

Re: [PATCH 2/3] fetch.c: change s_update_ref to use a ref transaction

[Ronnie Sahlberg]

Fixed. Thanks.

On Wed, Apr 23, 2014 at 1:12 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> Change s_update_ref to use a ref transaction for the ref update.
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>
> Doubled sign-off.
>
>> ---
>>  builtin/fetch.c | 15 +++++++--------
>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index a93c893..5c15584 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -375,7 +375,7 @@ static int s_update_ref(const char *action,
>>  {
>>         char msg[1024];
>>         char *rla = getenv("GIT_REFLOG_ACTION");
>> -       static struct ref_lock *lock;
>> +       struct ref_transaction *transaction;
>>
>>         if (dry_run)
>>                 return 0;
>> @@ -384,15 +384,14 @@ static int s_update_ref(const char *action,
>>         snprintf(msg, sizeof(msg), "%s: %s", rla, action);
>>
>>         errno = 0;
>> -       lock = lock_any_ref_for_update(ref->name,
>> -                                      check_old ? ref->old_sha1 : NULL,
>> -                                      0, NULL);
>> -       if (!lock)
>> -               return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
>> -                                         STORE_REF_ERROR_OTHER;
>> -       if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
>> +       transaction = ref_transaction_begin();
>> +       if (!transaction ||
>> +           ref_transaction_update(transaction, ref->name, ref->new_sha1,
>> +                                  ref->old_sha1, 0, check_old) ||
>> +           ref_transaction_commit(transaction, msg, UPDATE_REFS_QUIET_ON_ERR))
>>                 return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
>>                                           STORE_REF_ERROR_OTHER;
>> +
>>         return 0;
>>  }
>>
>> --
>> 1.9.1.518.g16976cb.dirty

Re: [PATCH 3/3] fetch.c: use a single ref transaction for all ref updates

[Ronnie Sahlberg]

Fixed, thanks!

On Wed, Apr 23, 2014 at 1:17 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Apr 22, 2014 at 2:45 PM, Ronnie Sahlberg <sahlberg@google.com> wrote:
>> Change store_updated_refs to use a single ref transaction for all refs that
>> are updated during the fetch. This makes the fetch more atomic when update
>> failures occur.
>>
>> Since ref update failures will now no longer occur in the code path for
>> updating a single ref in s_update_ref, we no longer have as detailed error
>> message logging the exact reference and the ref log action as in the old code.
>> Instead since we fail the entire transaction we log a much more generic
>> message. But since we commit the transaction using MSG_ON_ERR we will log
>> an error containing the ref name if either locking of writing the ref would
>> so the regression in the log message is minor.
>>
>> This will also change the order in which errors are checked for and logged
>> which may alter which error will be logged if there are multiple errors
>> occuring during a fetch.
>>
>> For example, assuming we have a fetch for two refs that both would fail.
>
> s/assuming/assume/ perhaps?
>
>> Where the first ref would fail with ENOTDIR due to a directory in the ref
>> path not existing, and the second ref in the fetch would fail due to
>> the check in update_logical_ref():
>>         if (current_branch &&
>>             !strcmp(ref->name, current_branch->name) &&
>>             !(update_head_ok || is_bare_repository()) &&
>>             !is_null_sha1(ref->old_sha1)) {
>>                 /*
>>                  * If this is the head, and it's not okay to update
>>                  * the head, and the old value of the head isn't empty...
>>                  */
>>
>> In the old code sicne we would update the refs one ref at a time we would
>
> s/sicne/since/
>
>> first fail the ENOTDIR and then fail the second update of HEAD as well.
>> But since the first ref failed with ENOTDIR we would eventually fail the whole
>> fetch with STORE_REF_ERROR_DF_CONFLICT
>>
>> In the new code, since we defer committing the transaction until all refs
>> has been processed, we would now detect that the second ref was bad and
>
> s/has/have/
>
>> rollback the transaction before we would even try start writing the update to
>> disk and thus we would not return STORE_REF_ERROR_DF_CONFLICT for this case.
>>
>> I think this new behaviour is more correct, since if there was a problem
>> we would not even try to commit the transaction but need to highlight this
>> change in how/what errors are reported.
>> This change in what error is returned only occurs if there are multiple
>> refs that fail to update and only some, but not all, of them fail due to
>> ENOTDIR.
>>
>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>

Re: [PATCH 0/3] Use ref transactions for fetch

[Michael Haggerty]

On 04/22/2014 08:45 PM, Ronnie Sahlberg wrote:
> This change is based on the previous ref transaction patches.
> This is sent as a separate patch series since it implements a lot more
> non-trivial changes to the behaviour than the previous patches and thus can
> use more detailed review.
> 
> Update fetch.c to use ref transactions when performing updates. Use a single
> ref transaction for all updates and only commit the transaction if all other
> checks and oeprations have been successful. This makes the ref updates during
> a fetch (mostly) atomic.

Is this always an improvement?  What kind of checks are there that might
fail?

It would be pretty annoying to spend a lot of time fetching a big pack,
only to have the fetch fail because one reference out of many couldn't
be updated.  This would force the user to download the entire pack
again, whereas if the successful reference updates had been allowed,
then probably most or all of the second download would have been avoidable.

On the other hand, if a reference was renamed on the remote side,
allowing a partial reference update could cause history to be discarded
locally if the old name's delete was accepted but the new name's
addition was rejected.  This wouldn't be the end of the world, because
the history is presumably still available remotely to fetch again, but
it's not ideal either.

I'm not sure myself what I would prefer, but I wanted to point out that
it is IMO not obvious that atomicity here is an improvement.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 0/3] Use ref transactions for fetch

[Ronnie Sahlberg]

On Mon, May 5, 2014 at 4:22 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 04/22/2014 08:45 PM, Ronnie Sahlberg wrote:
>> This change is based on the previous ref transaction patches.
>> This is sent as a separate patch series since it implements a lot more
>> non-trivial changes to the behaviour than the previous patches and thus can
>> use more detailed review.
>>
>> Update fetch.c to use ref transactions when performing updates. Use a single
>> ref transaction for all updates and only commit the transaction if all other
>> checks and oeprations have been successful. This makes the ref updates during
>> a fetch (mostly) atomic.
>
> Is this always an improvement?  What kind of checks are there that might
> fail?
>
> It would be pretty annoying to spend a lot of time fetching a big pack,
> only to have the fetch fail because one reference out of many couldn't
> be updated.  This would force the user to download the entire pack
> again, whereas if the successful reference updates had been allowed,
> then probably most or all of the second download would have been avoidable.
>
> On the other hand, if a reference was renamed on the remote side,
> allowing a partial reference update could cause history to be discarded
> locally if the old name's delete was accepted but the new name's
> addition was rejected.  This wouldn't be the end of the world, because
> the history is presumably still available remotely to fetch again, but
> it's not ideal either.
>
> I'm not sure myself what I would prefer, but I wanted to point out that
> it is IMO not obvious that atomicity here is an improvement.
>

We could make it a .git/config option ?

Re: [PATCH 0/3] Use ref transactions for fetch

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It would be pretty annoying to spend a lot of time fetching a big pack,
> only to have the fetch fail because one reference out of many couldn't
> be updated.  This would force the user to download the entire pack
> again,...

Is that really true?  Doesn't quickfetch optimization kick in for
the second fetch?

Re: [PATCH 0/3] Use ref transactions for fetch

[Michael Haggerty]

On 05/06/2014 08:40 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> It would be pretty annoying to spend a lot of time fetching a big pack,
>> only to have the fetch fail because one reference out of many couldn't
>> be updated.  This would force the user to download the entire pack
>> again,...
> 
> Is that really true?  Doesn't quickfetch optimization kick in for
> the second fetch?

Yes, I guess it would.  I wasn't aware of that optimization.  Thanks for
the pointer.

I withdraw my objection to using atomic reference updates for fetch.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/