From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
This patch series is based on mhagger/ref-transactions and expands on the
transaction API. It converts all external (outside of refs.c) callers to
use the transaction API for any writes.
This makes most of the ref updates to become atomic when there are failures
locking or writing to a ref.
Version 5:
- Reword commit messages for having _create/_delete/_update returning
success/failure. There are no conditions yet that return an error from
these failures but there will be in the future. So we still check the
return from these functions in the callers in preparation for this.
- Don't leak memory by just passing a strbuf_detach() pointer to functions.
Use <obj>.buf and explicitely strbuf_release the data afterwards.
- Remove the function update_ref_lock.
- Remove the function update_ref_write.
- Track transaction status and die(BUG:) if we call _create/_delete/_update/
_commit for a transaction that is not OPEN.
Version 4:
- Rename patch series from "Use ref transactions from most callers" to
"Use ref transactions for all ref updates".
- Convert all external ref writes to use transactions and make write_ref_sha1
and lock_ref_sha1 static functions.
- Change the ref commit and free handling so we no longer pass pointer to
pointer to _commit. _commit no longer frees the transaction. The caller
MUST call _free itself.
- Change _commit to take a strbuf pointer instead of a char* for error
reporting back to the caller.
- Re-add the walker patch after fixing it.
Version 3:
- Remove the walker patch for now. Walker needs more complex solution
so defer it until the basics are done.
- Remove the onerr argument to ref_transaction_commit(). All callers
that need to die() on error now have to do this explicitely.
- Pass an error string from ref_transaction_commit() back to the callers
so that they can craft a nice error message upon failures.
- Make ref_transaction_rollback() accept NULL as argument.
- Change ref_transaction_commit() to take a pointer to pointer argument for
the transaction and have it clear the callers pointer to NULL when
invoked. This allows for much nicer handling of transaction rollback on
failure.
Version 2:
- Add a patch to ref_transaction_commit to make it honor onerr even if the
error triggered in ref_Transaction_commit itself rather than in a call
to other functions (that already honor onerr).
- Add a patch to make the update_ref() helper function use transactions
internally.
- Change ref_transaction_update to die() instead of error() if we pass
if a NULL old_sha1 but have have_old == true.
- Change ref_transaction_create to die() instead of error() if new_sha1
is false but we pass it a null_sha1.
- Change ref_transaction_delete die() instead of error() if we pass
if a NULL old_sha1 but have have_old == true.
- Change several places to do if(!transaction || ref_transaction_update()
|| ref_Transaction_commit()) die(generic-message) instead of checking each
step separately and having a different message for each failure.
Most users are likely not interested in what step of the transaction
failed and only whether it failed or not.
- Change commit.c to only pass a pointer to ref_transaction_update
iff current_head is non-NULL.
The previous patch used to compute a garbage pointer for
current_head->object.sha1 and relied on the fact that ref_transaction_update
would not try to dereference this pointer if !!current_head was 0.
- Updated commit message for the walker_fetch change to try to justify why
the change in locking semantics should not be harmful.
Ronnie Sahlberg (30):
refs.c: constify the sha arguments for
ref_transaction_create|delete|update
refs.c: allow passing NULL to ref_transaction_free
refs.c: add a strbuf argument to ref_transaction_commit for error
logging
refs.c: make ref_update_reject_duplicates take a strbuf argument for
errors
update-ref.c: log transaction error from the update_ref
refs.c: make update_ref_write update a strbuf on failure
refs.c: remove the onerr argument to ref_transaction_commit
refs.c: change ref_transaction_update() to do error checking and
return status
refs.c: change ref_transaction_create to do error checking and return
status
refs.c: ref_transaction_delete to check for error and return status
tag.c: use ref transactions when doing updates
replace.c: use the ref transaction functions for updates
commit.c: use ref transactions for updates
sequencer.c: use ref transactions for all ref updates
fast-import.c: change update_branch to use ref transactions
branch.c: use ref transaction for all ref updates
refs.c: change update_ref to use a transaction
refs.c: free the transaction before returning when number of updates
is 0
refs.c: ref_transaction_commit should not free the transaction
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
receive-pack.c: use a reference transaction for updating the refs
fast-import.c: use a ref transaction when dumping tags
walker.c: use ref transaction for ref updates
refs.c: make write_ref_sha1 static
refs.c: make lock_ref_sha1 static
refs.c: add transaction.status and track OPEN/CLOSED/ERROR
refs.c: remove the update_ref_lock function
refs.c: remove the update_ref_write function
branch.c | 31 ++++----
builtin/commit.c | 24 +++---
builtin/fetch.c | 35 ++++-----
builtin/receive-pack.c | 20 ++---
builtin/replace.c | 15 ++--
builtin/tag.c | 15 ++--
builtin/update-ref.c | 29 +++++---
fast-import.c | 38 ++++++----
refs.c | 193 +++++++++++++++++++++++++++++--------------------
refs.h | 45 ++++++------
sequencer.c | 24 ++++--
walker.c | 51 ++++++-------
12 files changed, 297 insertions(+), 223 deletions(-)
--
1.9.1.532.gf8485a6
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 7 ++++---
refs.h | 7 ++++---
2 files changed, 8 insertions(+), 6 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
In this case transaction is reset to NULL IFF ref_transaction_commit() was
invoked and thus the rollback becomes ref_transaction_rollback(NULL) which
is safe. IF the conditional triggered prior to ref_transaction_commit()
then transaction is untouched and then ref_transaction_rollback(transaction)
will rollback the failed transaction.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Do basic error checking in ref_transaction_create() and make it return
status. Update all callers to check the result of ref_transaction_create()
There are currently no conditions in _update that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 4 +++-
refs.c | 17 +++++++++++------
refs.h | 8 ++++----
3 files changed, 18 insertions(+), 11 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
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. Statics live in .bss and
are all automatically initialized to 0.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/fetch.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change ref_transaction_commit so that it does not free the transaction.
Instead require that a caller will end a transaction by either calling
ref_transaction_rollback or ref_transaction_free.
By having the transaction object remaining valid after _commit returns allows
us to write much nicer code and still be able to call ref_transaction_rollback
safely. Instead of this horribleness
t = ref_transaction_begin();
if ((!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval)) ||
(ref_transaction_commit(t, action, &err) && !(t = NULL))) {
ref_transaction_rollback(t);
we can now just do the much nicer
t = ref_transaction_begin();
if (!t ||
ref_transaction_update(t, refname, sha1, oldval, flags,
!!oldval) ||
ref_transaction_commit(&t, action, &err)) {
ref_transaction_rollback(t);
... die/return ...
ref_transaction_free(transaction);
Signed-off-by: Ronnie Sahlberg <redacted>
---
branch.c | 1 +
builtin/commit.c | 1 +
builtin/replace.c | 1 +
builtin/tag.c | 1 +
builtin/update-ref.c | 1 +
fast-import.c | 8 ++++----
refs.c | 14 ++++++--------
refs.h | 14 +++++++++-----
sequencer.c | 8 ++++----
9 files changed, 28 insertions(+), 21 deletions(-)
@@ -3451,10 +3452,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,intn=transaction->nr;structref_update**updates=transaction->updates;-if(!n){-ref_transaction_free(transaction);+if(!n)return0;-}/* Allocate work space */delnames=xmalloc(sizeof(*delnames)*n);
@@ -267,13 +267,17 @@ int ref_transaction_delete(struct ref_transaction *transaction,/**Commitallofthechangesthathavebeenqueuedintransaction,as*atomicallyaspossible.Returnanonzerovalueifthereisa-*problem.Theref_transactionisfreedbythisfunction.-*Iferrisnon-NULLwewilladdanerrorstringtoittoexplainwhy-*thetransactionfailed.+*problem.Iferrisnon-NULLwewilladdanerrorstringtoittoexplain+*whythetransactionfailed.*/intref_transaction_commit(structref_transaction*transaction,constchar*msg,structstrbuf*err);+/*+*Freeanexistingtransaction.+*/+voidref_transaction_free(structref_transaction*transaction);+/** Lock a ref and then write its file */intupdate_ref(constchar*action,constchar*refname,constunsignedchar*sha1,constunsignedchar*oldval,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change the update_ref helper function to use a ref transaction internally.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
We have to free the transaction before returning in the early check for
'return early if number of updates == 0' or else the following code would
create a memory leak with the transaction never being freed :
t = ref_transaction_begin()
ref_transaction_commit(t)
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
@@ -3451,8 +3451,10 @@ int ref_transaction_commit(struct ref_transaction *transaction,intn=transaction->nr;structref_update**updates=transaction->updates;-if(!n)+if(!n){+ref_transaction_free(transaction);return0;+}/* Allocate work space */delnames=xmalloc(sizeof(*delnames)*n);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change create_branch to use a ref transaction when creating the new branch.
ref_transaction_create will check that the ref does not already exist and fail
otherwise meaning that we no longer need to keep a lock on the ref during the
setup_tracking. This simplifies the code since we can now do the transaction
in one single step.
If the forcing flag is false then use ref_transaction_create since this will
fail if the ref already exist. Otherwise use ref_transaction_update.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existsing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Signed-off-by: Ronnie Sahlberg <redacted>
---
branch.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
@@ -285,15 +284,6 @@ void create_branch(const char *head,die(_("Not a valid branch point: '%s'."),start_name);hashcpy(sha1,commit->object.sha1);-if(!dont_change_ref){-lock=lock_any_ref_for_update(ref.buf,NULL,0,NULL);-if(!lock)-die_errno(_("Failed to lock ref for update"));-}--if(reflog)-log_all_ref_updates=1;-if(forcing)snprintf(msg,sizeofmsg,"branch: Reset to %s",start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,snprintf(msg,sizeofmsg,"branch: Created from %s",start_name);+if(reflog)+log_all_ref_updates=1;++if(!dont_change_ref){+structref_transaction*transaction;+structstrbuferr=STRBUF_INIT;++transaction=ref_transaction_begin();+if(!transaction||+ref_transaction_update(transaction,ref.buf,sha1,+null_sha1,0,!forcing)||+ref_transaction_commit(transaction,msg,&err))+die_errno(_("%s: failed to write ref: %s"),+ref.buf,err.buf);+}+if(real_ref&&track)setup_tracking(ref.buf+11,real_ref,track,quiet);-if(!dont_change_ref)-if(write_ref_sha1(lock,sha1,msg)<0)-die_errno(_("Failed to write ref"));-strbuf_release(&ref);free(real_ref);}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 35 +++++++++--------------------------
1 file changed, 9 insertions(+), 26 deletions(-)
@@ -3235,25 +3235,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)returnretval;}-staticintupdate_ref_write(constchar*action,constchar*refname,-constunsignedchar*sha1,structref_lock*lock,-structstrbuf*err,enumaction_on_erronerr)-{-if(write_ref_sha1(lock,sha1,action)<0){-constchar*str="Cannot update the ref '%s'.";-if(err)-strbuf_addf(err,str,refname);--switch(onerr){-caseUPDATE_REFS_MSG_ON_ERR:error(str,refname);break;-caseUPDATE_REFS_DIE_ON_ERR:die(str,refname);break;-caseUPDATE_REFS_QUIET_ON_ERR:break;-}-return1;-}-return0;-}-/***Informationneededforasinglerefupdate.Setnew_sha1tothe*newvalueortozerotodeletetheref.Tochecktheoldvalue
@@ -3498,14 +3479,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,structref_update*update=updates[i];if(!is_null_sha1(update->new_sha1)){-ret=update_ref_write(msg,-update->refname,-update->new_sha1,-update->lock,err,-UPDATE_REFS_QUIET_ON_ERR);-update->lock=NULL;/* freed by update_ref_write */-if(ret)+ret=write_ref_sha1(update->lock,update->new_sha1,+msg);+update->lock=NULL;/* freed by write_ref_sha1 */+if(ret){+constchar*str="Cannot update the ref '%s'.";++if(err)+strbuf_addf(err,str,update->refname);gotocleanup;+}}}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
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 cod
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, assume 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 since 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 who
fetch with STORE_REF_ERROR_DF_CONFLICT
In the new code, since we defer committing the transaction until all refs
have 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 t
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 <redacted>
---
builtin/fetch.c | 34 ++++++++++++++++------------------
1 file changed, 16 insertions(+), 18 deletions(-)
@@ -676,6 +670,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,}}}+if(ref_transaction_commit(transaction,"fetch_ref transaction",NULL))+rc|=errno==ENOTDIR?STORE_REF_ERROR_DF_CONFLICT:+STORE_REF_ERROR_OTHER;+ref_transaction_free(transaction);if(rc&STORE_REF_ERROR_DF_CONFLICT)error(_("some local refs could not be updated; try running\n"
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
This changes the locking slightly for walker_fetch. Previously the code would
lock all refs before writing them but now we do not lock the refs until the
commit stage. There is thus a very short window where changes could be done
locally during the fetch which would be overwritten when the fetch completes
and commits its transaction. But this window should be reasonably short.
Even if this race does trigger, since both the old code and the new code
just overwrites the refs to the new values without checking or comparing
them with the previous value, this is not too dissimilar to a similar scenario
where you first do a ref change locally and then later do a fetch that
overwrites the local change. With this in mind I do not see the change in
locking semantics to be critical.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Signed-off-by: Ronnie Sahlberg <redacted>
---
walker.c | 51 ++++++++++++++++++++++++++-------------------------
1 file changed, 26 insertions(+), 25 deletions(-)
@@ -276,14 +270,14 @@ int walker_fetch(struct walker *walker, int targets, char **target,for(i=0;i<targets;i++){if(interpret_target(walker,target[i],&sha1[20*i])){error("Could not interpret response from server '%s' as something to pull",target[i]);-gotounlock_and_fail;+gotorollback_and_fail;}if(process(walker,lookup_unknown_object(&sha1[20*i])))-gotounlock_and_fail;+gotorollback_and_fail;}if(loop(walker))-gotounlock_and_fail;+gotorollback_and_fail;if(write_ref_log_details){msg=xmalloc(strlen(write_ref_log_details)+12);
@@ -294,19 +288,26 @@ int walker_fetch(struct walker *walker, int targets, char **target,for(i=0;i<targets;i++){if(!write_ref||!write_ref[i])continue;-ret=write_ref_sha1(lock[i],&sha1[20*i],msg?msg:"fetch (unknown)");-lock[i]=NULL;-if(ret)-gotounlock_and_fail;+sprintf(ref_name,"refs/%s",write_ref[i]);+if(ref_transaction_update(transaction,ref_name,+&sha1[20*i],NULL,+0,0))+gotorollback_and_fail;+}++if(ref_transaction_commit(transaction,msg?msg:"fetch (unknown)",+&err)){+error("%s",err.buf);+gotorollback_and_fail;}-free(msg);+free(msg);return0;-unlock_and_fail:-for(i=0;i<targets;i++)-if(lock[i])-unlock_ref(lock[i]);+rollback_and_fail:+free(msg);+strbuf_release(&err);+ref_transaction_free(transaction);return-1;}
@@ -150,9 +150,6 @@ extern int commit_ref(struct ref_lock *lock);/** Release any lock taken but not written. **/externvoidunlock_ref(structref_lock*lock);-/** Writes sha1 into the ref specified by the lock. **/-externintwrite_ref_sha1(structref_lock*lock,constunsignedchar*sha1,constchar*msg);-/** Setup reflog before using. **/intlog_ref_setup(constchar*refname,char*logfile,intbufsize);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 37 +++++++++++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 4 deletions(-)
@@ -3344,7 +3356,10 @@ int ref_transaction_update(struct ref_transaction *transaction,structref_update*update;if(have_old&&!old_sha1)-die("have_old is true but old_sha1 is NULL");+die("BUG: have_old is true but old_sha1 is NULL");++if(transaction->status!=REF_TRANSACTION_OPEN)+die("BUG: update on transaction that is not open");update=add_update(transaction,refname);hashcpy(update->new_sha1,new_sha1);
@@ -3363,7 +3378,10 @@ int ref_transaction_create(struct ref_transaction *transaction,structref_update*update;if(!new_sha1||is_null_sha1(new_sha1))-die("create ref with null new_sha1");+die("BUG: create ref with null new_sha1");++if(transaction->status!=REF_TRANSACTION_OPEN)+die("BUG: create on transaction that is not open");update=add_update(transaction,refname);
@@ -3382,7 +3400,10 @@ int ref_transaction_delete(struct ref_transaction *transaction,structref_update*update;if(have_old&&!old_sha1)-die("have_old is true but old_sha1 is NULL");+die("BUG: have_old is true but old_sha1 is NULL");++if(transaction->status!=REF_TRANSACTION_OPEN)+die("BUG: delete on transaction that is not open");update=add_update(transaction,refname);update->flags=flags;
@@ -3454,8 +3475,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,intn=transaction->nr;structref_update**updates=transaction->updates;-if(!n)+if(transaction->status!=REF_TRANSACTION_OPEN)+die("BUG: commit on transaction that is not open");++if(!n){+transaction->status=REF_TRANSACTION_CLOSED;return0;+}/* Allocate work space */delnames=xmalloc(sizeof(*delnames)*n);
@@ -3517,6 +3543,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,clear_loose_ref_cache(&ref_cache);cleanup:+transaction->status=ret?REF_TRANSACTION_ERROR+:REF_TRANSACTION_CLOSED;+for(i=0;i<n;i++)if(updates[i]->lock)unlock_ref(updates[i]->lock);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg <redacted>
---
sequencer.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
@@ -272,19 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)staticintfast_forward_to(constunsignedchar*to,constunsignedchar*from,intunborn,structreplay_opts*opts){-structref_lock*ref_lock;+structref_transaction*transaction;structstrbufsb=STRBUF_INIT;-intret;+structstrbuferr=STRBUF_INIT;read_cache();if(checkout_fast_forward(from,to,1))exit(1);/* the callee should have complained already */-ref_lock=lock_any_ref_for_update("HEAD",unborn?null_sha1:from,-0,NULL);+strbuf_addf(&sb,"%s: fast-forward",action_name(opts));-ret=write_ref_sha1(ref_lock,to,sb.buf);++transaction=ref_transaction_begin();+if((!transaction||+ref_transaction_update(transaction,"HEAD",to,from,+0,!unborn))||+(ref_transaction_commit(transaction,sb.buf,&err)&&+!(transaction=NULL))){+ref_transaction_rollback(transaction);+error(_("HEAD: Could not fast-forward: %s\n"),err.buf);+strbuf_release(&sb);+strbuf_release(&err);+return-1;+}+strbuf_release(&sb);-returnret;+return0;}staticintdo_recursive_merge(structcommit*base,structcommit*next,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Update ref_transaction_update() do some basic error checking and return
true on error. Update all callers to check ref_transaction_update() for error.
There are currently no conditions in _update that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 10 ++++++----
refs.c | 9 +++++++--
refs.h | 10 +++++-----
3 files changed, 18 insertions(+), 11 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++++++---------------
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
@@ -272,8 +272,7 @@ void ref_transaction_delete(struct ref_transaction *transaction,*thetransactionfailed.*/intref_transaction_commit(structref_transaction*transaction,-constchar*msg,structstrbuf*err,-enumaction_on_erronerr);+constchar*msg,structstrbuf*err);/** Lock a ref and then write its file */intupdate_ref(constchar*action,constchar*refname,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change ref_transaction_delete() to do basic error checking and return
status. Update all callers to check the return for ref_transaction_delete()
There are currently no conditions in _update that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 5 +++--
refs.c | 15 ++++++++++-----
refs.h | 8 ++++----
3 files changed, 17 insertions(+), 11 deletions(-)
@@ -3372,19 +3372,24 @@ int ref_transaction_create(struct ref_transaction *transaction,return0;}-voidref_transaction_delete(structref_transaction*transaction,-constchar*refname,-constunsignedchar*old_sha1,-intflags,inthave_old)+intref_transaction_delete(structref_transaction*transaction,+constchar*refname,+constunsignedchar*old_sha1,+intflags,inthave_old){-structref_update*update=add_update(transaction,refname);+structref_update*update;+if(have_old&&!old_sha1)+die("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);}+return0;}intupdate_ref(constchar*action,constchar*refname,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/fetch.c | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:55
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/commit.c | 23 ++++++++++-------------
1 file changed, 10 insertions(+), 13 deletions(-)
@@ -129,7 +129,8 @@ static int replace_object(const char *object_ref, const char *replace_ref,unsignedcharobject[20],prev[20],repl[20];enumobject_typeobj_type,repl_type;charref[PATH_MAX];-structref_lock*lock;+structref_transaction*transaction;+structstrbuferr=STRBUF_INIT;if(get_sha1(object_ref,object))die("Failed to resolve '%s' as a valid ref.",object_ref);
@@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref,elseif(!force)die("replace ref '%s' already exists",ref);-lock=lock_any_ref_for_update(ref,prev,0,NULL);-if(!lock)-die("%s: cannot lock the ref",ref);-if(write_ref_sha1(lock,repl,NULL)<0)-die("%s: cannot update the ref",ref);+transaction=ref_transaction_begin();+if(!transaction||+ref_transaction_update(transaction,ref,repl,prev,+0,!is_null_sha1(prev))||+ref_transaction_commit(transaction,NULL,&err))+die(_("%s: failed to replace ref: %s"),ref,err.buf);return0;}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:56
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)constchar*refname,*oldval,*msg=NULL;unsignedcharsha1[20],oldsha1[20];intdelete=0,no_deref=0,read_stdin=0,end_null=0,flags=0;+structstrbuferr=STRBUF_INIT;structoptionoptions[]={OPT_STRING('m',NULL,&msg,N_("reason"),N_("reason of the update")),OPT_BOOL('d',NULL,&delete,N_("delete the reference")),
@@ -359,17 +360,16 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)die("Refusing to perform update with empty message.");if(read_stdin){-intret;transaction=ref_transaction_begin();-if(delete||no_deref||argc>0)usage_with_options(git_update_ref_usage,options);if(end_null)line_termination='\0';update_refs_stdin();-ret=ref_transaction_commit(transaction,msg,NULL,-UPDATE_REFS_DIE_ON_ERR);-returnret;+if(ref_transaction_commit(transaction,msg,&err,+UPDATE_REFS_QUIET_ON_ERR))+die("%s",err.buf);+return0;}if(end_null)
@@ -1678,36 +1678,41 @@ found_entry:staticintupdate_branch(structbranch*b){staticconstchar*msg="fast-import";-structref_lock*lock;+structref_transaction*transaction;unsignedcharold_sha1[20];+structstrbuferr=STRBUF_INIT;if(is_null_sha1(b->sha1))return0;if(read_ref(b->name,old_sha1))hashclr(old_sha1);-lock=lock_any_ref_for_update(b->name,old_sha1,0,NULL);-if(!lock)-returnerror("Unable to lock %s",b->name);if(!force_update&&!is_null_sha1(old_sha1)){structcommit*old_cmit,*new_cmit;old_cmit=lookup_commit_reference_gently(old_sha1,0);new_cmit=lookup_commit_reference_gently(b->sha1,0);if(!old_cmit||!new_cmit){-unlock_ref(lock);returnerror("Branch %s is missing commits.",b->name);}if(!in_merge_bases(old_cmit,new_cmit)){-unlock_ref(lock);warning("Not updating %s"" (new tip %s does not contain %s)",b->name,sha1_to_hex(b->sha1),sha1_to_hex(old_sha1));return-1;}}-if(write_ref_sha1(lock,b->sha1,msg)<0)-returnerror("Unable to update %s",b->name);+transaction=ref_transaction_begin();+if((!transaction||+ref_transaction_update(transaction,b->name,b->sha1,old_sha1,+0,1))||+(ref_transaction_commit(transaction,msg,&err)&&+!(transaction=NULL))){+ref_transaction_rollback(transaction);+error("Unable to update branch %s: %s",b->name,err.buf);+strbuf_release(&err);+return-1;+}return0;}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:56
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 2 +-
refs.c | 6 +++++-
refs.h | 5 ++++-
3 files changed, 10 insertions(+), 3 deletions(-)
@@ -268,9 +268,12 @@ void ref_transaction_delete(struct ref_transaction *transaction,*Commitallofthechangesthathavebeenqueuedintransaction,as*atomicallyaspossible.Returnanonzerovalueifthereisa*problem.Theref_transactionisfreedbythisfunction.+*Iferrisnon-NULLwewilladdanerrorstringtoittoexplainwhy+*thetransactionfailed.*/intref_transaction_commit(structref_transaction*transaction,-constchar*msg,enumaction_on_erronerr);+constchar*msg,structstrbuf*err,+enumaction_on_erronerr);/** Lock a ref and then write its file */intupdate_ref(constchar*action,constchar*refname,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:56
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:56
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -3400,6 +3401,9 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,if(!strcmp(updates[i-1]->refname,updates[i]->refname)){constchar*str="Multiple updates for ref '%s' not allowed.";+if(err)+strbuf_addf(err,str,updates[i]->refname);+switch(onerr){caseUPDATE_REFS_MSG_ON_ERR:error(str,updates[i]->refname);break;
@@ -3430,7 +3434,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,/* Copy, sort, and reject duplicate refs */qsort(updates,n,sizeof(*updates),ref_update_compare);-ret=ref_update_reject_duplicates(updates,n,onerr);+ret=ref_update_reject_duplicates(updates,n,err,onerr);if(ret)gotocleanup;
@@ -132,9 +132,6 @@ extern int ref_exists(const char *);*/externintpeel_ref(constchar*refname,unsignedchar*sha1);-/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/-externstructref_lock*lock_ref_sha1(constchar*refname,constunsignedchar*old_sha1);-/** Locks any ref (for 'HEAD' type refs). */#define REF_NODEREF 0x01externstructref_lock*lock_any_ref_for_update(constchar*refname,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:56
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 30 ++++++------------------------
1 file changed, 6 insertions(+), 24 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:56
Wrap all the ref updates inside a transaction to make the update atomic.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/receive-pack.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
@@ -475,7 +477,6 @@ static const char *update(struct command *cmd, struct shallow_info *si)constchar*namespaced_name;unsignedchar*old_sha1=cmd->old_sha1;unsignedchar*new_sha1=cmd->new_sha1;-structref_lock*lock;/* only refs/... are allowed */if(!starts_with(name,"refs/")||check_refname_format(name+5,0)){
@@ -580,15 +581,9 @@ static const char *update(struct command *cmd, struct shallow_info *si)update_shallow_ref(cmd,si))return"shallow error";-lock=lock_any_ref_for_update(namespaced_name,old_sha1,-0,NULL);-if(!lock){-rp_error("failed to lock %s",name);-return"failed to lock";-}-if(write_ref_sha1(lock,new_sha1,"push")){-return"failed to write";/* error() already called */-}+if(ref_transaction_update(transaction,namespaced_name,+new_sha1,old_sha1,0,1))+return"failed to update";returnNULL;/* good */}}