From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
This patch series is based on next and expands on the transaction API. It
converts all ref updates, inside refs.c as well as external, to use the
transaction API for updates. This makes most of the ref updates to become
atomic when there are failures locking or writing to a ref.
This version completes the work to convert all ref updates to use transactions.
Now that all updates are through transactions I will start working on
cleaning up the reading of refs and to create an api for managing reflogs but
all that will go in a different patch series.
Version 6:
- Convert all updates in refs.c to use transactions too.
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 (42):
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
refs.c: remove lock_ref_sha1
refs.c: make prune_ref use a transaction to delete the ref
refs.c: make delete_ref use a transaction
refs.c: pass the ref log message to _create/delete/update instead of
_commit
refs.c: pass NULL as *flags to read_ref_full
refs.c: pack all refs before we start to rename a ref
refs.c: move the check for valid refname to lock_ref_sha1_basic
refs.c: call lock_ref_sha1_basic directly from commit
refs.c: add a new flag for transaction delete for refs we know are
packed only
refs.c: pass a skip list to name_conflict_fn
refs.c: make rename_ref use a transaction
refs.c: remove forward declaraion of write_ref_sha1
branch.c | 31 ++--
builtin/commit.c | 24 ++-
builtin/fetch.c | 29 ++--
builtin/receive-pack.c | 20 +--
builtin/replace.c | 15 +-
builtin/tag.c | 15 +-
builtin/update-ref.c | 32 ++--
fast-import.c | 39 +++--
refs.c | 404 +++++++++++++++++++++++++++----------------------
refs.h | 49 +++---
sequencer.c | 24 ++-
t/t3200-branch.sh | 2 +-
walker.c | 51 ++++---
13 files changed, 414 insertions(+), 321 deletions(-)
--
2.0.0.rc1.351.g4d2c8e4
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
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:57
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:57
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:57
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:57
Allow passing a list of refs to ckip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock anc create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
@@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsintlog=!lstat(git_path("logs/%s",oldrefname),&loginfo);constchar*symref=NULL;+if(!strcmp(oldrefname,newrefname))+return0;+if(log&&S_ISLNK(loginfo.st_mode))returnerror("reflog for %s is a symlink",oldrefname);
@@ -2586,10 +2606,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsif(!symref)returnerror("refname %s not found",oldrefname);-if(!is_refname_available(newrefname,oldrefname,get_packed_refs(&ref_cache)))+if(!is_refname_available(newrefname,oldrefname,+get_packed_refs(&ref_cache),NULL,0))return1;-if(!is_refname_available(newrefname,oldrefname,get_loose_refs(&ref_cache)))+if(!is_refname_available(newrefname,oldrefname,+get_loose_refs(&ref_cache),NULL,0))return1;if(log&&rename(git_path("logs/%s",oldrefname),git_path(TMP_RENAMED_LOG)))
@@ -2622,7 +2644,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmslogmoved=log;-lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL);+lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL,NULL,0);if(!lock){error("unable to lock %s for update",newrefname);gotorollback;
@@ -2637,7 +2659,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsreturn0;rollback:-lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL);+lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL,NULL,0);if(!lock){error("unable to lock %s for rollback",oldrefname);gotorollbacklog;
@@ -3483,7 +3505,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,update->old_sha1:NULL),update->flags,-&update->type);+&update->type,+delnames,delnum);if(!update->lock){if(err)strbuf_addf(err,"Cannot lock the ref '%s'.",
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
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:57
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 */}}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
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:57
Change rename_ref to use a single transaction to perform the ref rename.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 73 ++++++++++++++++++------------------------------------------------
1 file changed, 20 insertions(+), 53 deletions(-)
@@ -2586,9 +2586,10 @@ static int rename_tmp_log(const char *newrefname)intrename_ref(constchar*oldrefname,constchar*newrefname,constchar*logmsg){-unsignedcharsha1[20],orig_sha1[20];-intflag=0,logmoved=0;-structref_lock*lock;+unsignedcharsha1[20];+intflag=0;+structref_transaction*transaction;+structstrbuferr=STRBUF_INIT;structstatloginfo;intlog=!lstat(git_path("logs/%s",oldrefname),&loginfo);constchar*symref=NULL;
@@ -2599,7 +2600,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsif(log&&S_ISLNK(loginfo.st_mode))returnerror("reflog for %s is a symlink",oldrefname);-symref=resolve_ref_unsafe(oldrefname,orig_sha1,1,&flag);+symref=resolve_ref_unsafe(oldrefname,sha1,1,&flag);if(flag&REF_ISSYMREF)returnerror("refname %s is a symbolic ref, renaming it is not supported",oldrefname);
@@ -2621,62 +2622,28 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsif(pack_refs(PACK_REFS_ALL|PACK_REFS_PRUNE))returnerror("unable to pack refs");-if(delete_ref(oldrefname,orig_sha1,REF_NODEREF)){-error("unable to delete old %s",oldrefname);-gotorollback;-}--if(!read_ref_full(newrefname,sha1,1,NULL)&&-delete_ref(newrefname,sha1,REF_NODEREF)){-if(errno==EISDIR){-if(remove_empty_directories(git_path("%s",newrefname))){-error("Directory not empty: %s",newrefname);-gotorollback;-}-}else{-error("unable to delete existing %s",newrefname);-gotorollback;-}+transaction=ref_transaction_begin();+if(!transaction||+ref_transaction_delete(transaction,oldrefname,sha1,+REF_NODEREF|REF_ISPACKONLY,+1,NULL)||+ref_transaction_update(transaction,newrefname,sha1,+NULL,0,0,logmsg)||+ref_transaction_commit(transaction,&err)){+ref_transaction_rollback(transaction);+error("rename_ref failed: %s",err.buf);+strbuf_release(&err);+gotorollbacklog;}+ref_transaction_free(transaction);if(log&&rename_tmp_log(newrefname))-gotorollback;--logmoved=log;--lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL,NULL,0);-if(!lock){-error("unable to lock %s for update",newrefname);-gotorollback;-}-lock->force_write=1;-hashcpy(lock->old_sha1,orig_sha1);-if(write_ref_sha1(lock,orig_sha1,logmsg)){-error("unable to write current sha1 into %s",newrefname);-gotorollback;-}--return0;--rollback:-lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL,NULL,0);-if(!lock){-error("unable to lock %s for rollback",oldrefname);gotorollbacklog;-}-lock->force_write=1;-flag=log_all_ref_updates;-log_all_ref_updates=0;-if(write_ref_sha1(lock,orig_sha1,NULL))-error("unable to write current sha1 into %s",oldrefname);-log_all_ref_updates=flag;+return0;rollbacklog:-if(logmoved&&rename(git_path("logs/%s",newrefname),git_path("logs/%s",oldrefname)))-error("unable to restore logfile %s from %s: %s",-oldrefname,newrefname,strerror(errno));-if(!logmoved&&log&&+if(log&&rename(git_path(TMP_RENAMED_LOG),git_path("logs/%s",oldrefname)))error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",oldrefname,strerror(errno));
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
Add a new flag REF_ISPACKONLY that we can use in ref_transaction_delete.
This flag indicates that the ref does not exist as a loose ref andf only as
a packed ref. If this is the case we then change the commit code so that
we skip taking out a lock file and we skip calling delete_ref_loose.
Check for this flag and die(BUG:...) if used with _update or _create.
At the start of the transaction, before we even start locking any refs,
we add all such REF_ISPACKONLY refs to delnames so that we have a list of
all pack only refs that we will be deleting during this transaction.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 19 +++++++++++++++++++
refs.h | 2 ++
2 files changed, 21 insertions(+)
@@ -3321,6 +3321,9 @@ int ref_transaction_update(struct ref_transaction *transaction,if(transaction->status!=REF_TRANSACTION_OPEN)die("BUG: update on transaction that is not open");+if(flags&REF_ISPACKONLY)+die("BUG: REF_ISPACKONLY can not be used with updates");+update=add_update(transaction,refname);hashcpy(update->new_sha1,new_sha1);update->flags=flags;
@@ -3345,6 +3348,9 @@ int ref_transaction_create(struct ref_transaction *transaction,if(transaction->status!=REF_TRANSACTION_OPEN)die("BUG: create on transaction that is not open");+if(flags&REF_ISPACKONLY)+die("BUG: REF_ISPACKONLY can not be used with creates");+update=add_update(transaction,refname);hashcpy(update->new_sha1,new_sha1);
@@ -3458,10 +3464,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,if(ret)gotocleanup;+for(i=0;i<n;i++){+structref_update*update=updates[i];++if(update->flags&REF_ISPACKONLY)+delnames[delnum++]=update->refname;+}+/* Acquire all locks while verifying old values */for(i=0;i<n;i++){structref_update*update=updates[i];+if(update->flags&REF_ISPACKONLY)+continue;+update->lock=lock_ref_sha1_basic(update->refname,(update->have_old?update->old_sha1:
@@ -3499,6 +3515,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,for(i=0;i<n;i++){structref_update*update=updates[i];+if(update->flags&REF_ISPACKONLY)+continue;+if(update->lock){ret|=delete_ref_loose(update->lock,update->type);if(!(update->flags&REF_ISPRUNING))
@@ -136,6 +136,8 @@ extern int peel_ref(const char *refname, unsigned char *sha1);#define REF_NODEREF 0x01/** Deleting a loose ref during prune */#define REF_ISPRUNING 0x02+/** Deletion of a ref that only exists as a packed ref */+#define REF_ISPACKONLY 0x04externstructref_lock*lock_any_ref_for_update(constchar*refname,constunsignedchar*old_sha1,intflags,int*type_p);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 3 +++
t/t3200-branch.sh | 2 +-
2 files changed, 4 insertions(+), 1 deletion(-)
@@ -2595,6 +2595,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsreturnerror("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",oldrefname,strerror(errno));+if(pack_refs(PACK_REFS_ALL|PACK_REFS_PRUNE))+returnerror("unable to pack refs");+if(delete_ref(oldrefname,orig_sha1,REF_NODEREF)){error("unable to delete old %s",oldrefname);gotorollback;
@@ -289,7 +289,7 @@ test_expect_success 'renaming a symref is not allowed' 'gitsymbolic-refrefs/heads/master2refs/heads/master&&test_must_failgitbranch-mmaster2master3&&gitsymbolic-refrefs/heads/master2&&-test_path_is_file.git/refs/heads/master&&+test_path_is_missing.git/refs/heads/master&&test_path_is_missing.git/refs/heads/master3'
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Signed-off-by: Ronnie Sahlberg <redacted>
---
branch.c | 6 +++---
builtin/commit.c | 4 ++--
builtin/fetch.c | 10 ++++++++--
builtin/receive-pack.c | 4 ++--
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++++++------
fast-import.c | 8 ++++----
refs.c | 34 +++++++++++++++++++++-------------
refs.h | 8 ++++----
sequencer.c | 4 ++--
walker.c | 6 +++---
12 files changed, 60 insertions(+), 45 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 22 +++++++++++++++-------
refs.h | 2 ++
2 files changed, 17 insertions(+), 7 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:57
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:57
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);
@@ -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:58
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;}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
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;+}}}
@@ -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:58
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 31 ++++++++++---------------------
1 file changed, 10 insertions(+), 21 deletions(-)
@@ -2481,11 +2481,6 @@ static int repack_without_refs(const char **refnames, int n)returncommit_packed_refs();}-staticintrepack_without_ref(constchar*refname)-{-returnrepack_without_refs(&refname,1);-}-staticintdelete_ref_loose(structref_lock*lock,intflag){if(!(flag&REF_ISPACKED)||flag&REF_ISSYMREF){
@@ -2503,24 +2498,18 @@ static int delete_ref_loose(struct ref_lock *lock, int flag)intdelete_ref(constchar*refname,constunsignedchar*sha1,intdelopt){-structref_lock*lock;-intret=0,flag=0;+structref_transaction*transaction;-lock=lock_ref_sha1_basic(refname,sha1,delopt,&flag);-if(!lock)+transaction=ref_transaction_begin();+if(!transaction||+ref_transaction_delete(transaction,refname,sha1,delopt,+sha1&&!is_null_sha1(sha1))||+ref_transaction_commit(transaction,NULL,NULL)){+ref_transaction_rollback(transaction);return1;-ret|=delete_ref_loose(lock,flag);--/* removing the loose one could have resurrected an earlier-*packedone.Also,ifitwasnotlooseweneedtorepack-*withoutit.-*/-ret|=repack_without_ref(lock->ref_name);--unlink_or_warn(git_path("logs/%s",lock->ref_name));-clear_loose_ref_cache(&ref_cache);-unlock_ref(lock);-returnret;+}+ref_transaction_free(transaction);+return0;}/*
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial "refs/"
part of the ref path name, the initial "refs/" that this caller had already
stripped off before calling lock_ref_sha1.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
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 _delete 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:58
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:58
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:58
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:58
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:58
Change to use ref transactions for all updates to refs.
Signed-off-by: Ronnie Sahlberg <redacted>
---
sequencer.c | 24 ++++++++++++++++--------
1 file changed, 16 insertions(+), 8 deletions(-)
@@ -272,23 +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);-if(!ref_lock)-returnerror(_("Failed to lock HEAD during fast_forward_to"));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,
@@ -1679,39 +1679,45 @@ found_entry:staticintupdate_branch(structbranch*b){staticconstchar*msg="fast-import";-structref_lock*lock;+structref_transaction*transaction;unsignedcharold_sha1[20];+structstrbuferr=STRBUF_INIT;if(read_ref(b->name,old_sha1))hashclr(old_sha1);+if(is_null_sha1(b->sha1)){if(b->delete)delete_ref(b->name,old_sha1,0);return0;}-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:58
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:58
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:58
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 _create 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:58
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)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
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;
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
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:58
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: Eric Sunshine <hidden> Date: 2016-06-15 23:00:58
On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg [off-list ref] 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
s/ENOTDIT/ENOTDIR/
quoted hunk
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: Eric Sunshine <hidden> Date: 2016-06-15 23:00:58
On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg [off-list ref] wrote:
Allow passing a list of refs to ckip checking to name_conflict_fn.
s/ckip/skip/
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock anc create m/m since we know that the
s/anc/and/
quoted hunk
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
@@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsintlog=!lstat(git_path("logs/%s",oldrefname),&loginfo);constchar*symref=NULL;+if(!strcmp(oldrefname,newrefname))+return0;+if(log&&S_ISLNK(loginfo.st_mode))returnerror("reflog for %s is a symlink",oldrefname);
@@ -2586,10 +2606,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsif(!symref)returnerror("refname %s not found",oldrefname);-if(!is_refname_available(newrefname,oldrefname,get_packed_refs(&ref_cache)))+if(!is_refname_available(newrefname,oldrefname,+get_packed_refs(&ref_cache),NULL,0))return1;-if(!is_refname_available(newrefname,oldrefname,get_loose_refs(&ref_cache)))+if(!is_refname_available(newrefname,oldrefname,+get_loose_refs(&ref_cache),NULL,0))return1;if(log&&rename(git_path("logs/%s",oldrefname),git_path(TMP_RENAMED_LOG)))
@@ -2622,7 +2644,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmslogmoved=log;-lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL);+lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL,NULL,0);if(!lock){error("unable to lock %s for update",newrefname);gotorollback;
@@ -2637,7 +2659,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsreturn0;rollback:-lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL);+lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL,NULL,0);if(!lock){error("unable to lock %s for rollback",oldrefname);gotorollbacklog;
@@ -3483,7 +3505,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,update->old_sha1:NULL),update->flags,-&update->type);+&update->type,+delnames,delnum);if(!update->lock){if(err)strbuf_addf(err,"Cannot lock the ref '%s'.",--
2.0.0.rc1.351.g4d2c8e4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:00:58
Fixed. Thanks.
On Thu, May 1, 2014 at 9:11 PM, Eric Sunshine [off-list ref] wrote:
On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg [off-list ref] wrote:
quoted
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
s/ENOTDIT/ENOTDIR/
quoted
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:58
Fixed. Thanks.
On Thu, May 1, 2014 at 9:22 PM, Eric Sunshine [off-list ref] wrote:
On Thu, May 1, 2014 at 4:37 PM, Ronnie Sahlberg [off-list ref] wrote:
quoted
Allow passing a list of refs to ckip checking to name_conflict_fn.
s/ckip/skip/
quoted
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock anc create m/m since we know that the
s/anc/and/
quoted
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 43 +++++++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 10 deletions(-)
@@ -2576,6 +2593,9 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsintlog=!lstat(git_path("logs/%s",oldrefname),&loginfo);constchar*symref=NULL;+if(!strcmp(oldrefname,newrefname))+return0;+if(log&&S_ISLNK(loginfo.st_mode))returnerror("reflog for %s is a symlink",oldrefname);
@@ -2586,10 +2606,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsif(!symref)returnerror("refname %s not found",oldrefname);-if(!is_refname_available(newrefname,oldrefname,get_packed_refs(&ref_cache)))+if(!is_refname_available(newrefname,oldrefname,+get_packed_refs(&ref_cache),NULL,0))return1;-if(!is_refname_available(newrefname,oldrefname,get_loose_refs(&ref_cache)))+if(!is_refname_available(newrefname,oldrefname,+get_loose_refs(&ref_cache),NULL,0))return1;if(log&&rename(git_path("logs/%s",oldrefname),git_path(TMP_RENAMED_LOG)))
@@ -2622,7 +2644,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmslogmoved=log;-lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL);+lock=lock_ref_sha1_basic(newrefname,NULL,0,NULL,NULL,0);if(!lock){error("unable to lock %s for update",newrefname);gotorollback;
@@ -2637,7 +2659,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsreturn0;rollback:-lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL);+lock=lock_ref_sha1_basic(oldrefname,NULL,0,NULL,NULL,0);if(!lock){error("unable to lock %s for rollback",oldrefname);gotorollbacklog;
@@ -3483,7 +3505,8 @@ int ref_transaction_commit(struct ref_transaction *transaction,update->old_sha1:NULL),update->flags,-&update->type);+&update->type,+delnames,delnum);if(!update->lock){if(err)strbuf_addf(err,"Cannot lock the ref '%s'.",--
2.0.0.rc1.351.g4d2c8e4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:00
On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
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.
I would change s/true/non-zero/, because error return values are not
just boolean values; the error values sometimes encode the type of error
that occurred.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:01
On Mon, May 5, 2014 at 5:57 AM, Michael Haggerty [off-list ref] wrote:
On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
quoted
This patch series is based on next and expands on the transaction API. [...]
Meta-comment:
Ronnie,
It seems like successive versions of this patch series are growing not
only in maturity but also in breadth. That makes it harder to review them.
I, for one, would prefer that a patch series cover a roughly fixed set
of changes [1], so that all of the patches in a version of the series
are at roughly the same level of maturity. That way, the whole series
can progress from "is this a good idea?" to "is the implementation
correct?" to "are all the details right?" at roughly the same time, and
then Junio can merge the branch, locking in that bit of progress. While
this is happening, other series can be making their way through other
stages of the pipeline.
When new patches are added to an old series, then they delay the merge
of the older patches, even if those are ripe. Plus, it makes it harder
for reviewers to keep track of the maturity level of each patch and to
read off how the older patches have changed. It makes the patch series
a moving target.
There's no need to re-split this patch series, but please take this wish
into account in the future.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:01
Thanks!
On Mon, May 5, 2014 at 6:08 AM, Michael Haggerty [off-list ref] wrote:
On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
quoted
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.
I would change s/true/non-zero/, because error return values are not
just boolean values; the error values sometimes encode the type of error
that occurred.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:01
On 05/01/2014 10:37 PM, Ronnie Sahlberg wrote:
This patch series is based on next and expands on the transaction API. [...]
Meta-comment:
Ronnie,
It seems like successive versions of this patch series are growing not
only in maturity but also in breadth. That makes it harder to review them.
I, for one, would prefer that a patch series cover a roughly fixed set
of changes [1], so that all of the patches in a version of the series
are at roughly the same level of maturity. That way, the whole series
can progress from "is this a good idea?" to "is the implementation
correct?" to "are all the details right?" at roughly the same time, and
then Junio can merge the branch, locking in that bit of progress. While
this is happening, other series can be making their way through other
stages of the pipeline.
When new patches are added to an old series, then they delay the merge
of the older patches, even if those are ripe. Plus, it makes it harder
for reviewers to keep track of the maturity level of each patch and to
read off how the older patches have changed. It makes the patch series
a moving target.
There's no need to re-split this patch series, but please take this wish
into account in the future.
Thanks,
Michael
[1] Of course, if a patch series has to grow to make the *existing*
changes correct, then that's perfectly OK.
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:07
Hi,
Ronnie Sahlberg wrote:
This patch series is based on next and expands on the transaction API.
Sorry to take so long to get to this.
For the future, it's easier to review patches based on some particular
branch that got merged into next, since next is a moving target
(series come and go from there depending on what seems to need testing
at a given moment). Is mh/ref-transaction the relevant branch to
build on?
Trying to apply the series to mh/ref-transaction, I get conflicts in
patch 13 due to absence of rs/ref-update-check-errors-early.
Trying to apply the series to a merge of mh/ref-transaction and
rs/ref-update-check-errors-early, I get a minor conflict in patch 15
but it is easy to resolve and the rest goes smoothly.
Looking forward to reading the rest. Thanks.
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:08
Ronnie Sahlberg wrote:
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
In extension = as a result?
Makes sense. It lets someone do the usual
struct ref_transaction *transaction;
int ret = 0;
if (something_fails()) {
ret = -1;
goto cleanup;
}
...
cleanup:
ref_transaction_free(transaction);
return ret;
just like you can already do with free().
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
Somewhere in the whitespace and parentheses I'm lost.
Is the idea that when ref_transaction_commit fails it will have
freed the transaction so we need not to roll back to prevent a
double free? I think it would be simpler for the caller to
unconditionally set transaction to NULL after calling
ref_transaction_commit in such a case to avoid use-after-free.
Even better if it is the caller's responsibility to free
the transaction. At any rate, it doesn't seem related to this
patch.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:08
On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
In extension = as a result?
Makes sense. It lets someone do the usual
struct ref_transaction *transaction;
int ret = 0;
if (something_fails()) {
ret = -1;
goto cleanup;
}
...
cleanup:
ref_transaction_free(transaction);
return ret;
just like you can already do with free().
quoted
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
Somewhere in the whitespace and parentheses I'm lost.
Is the idea that when ref_transaction_commit fails it will have
freed the transaction so we need not to roll back to prevent a
double free?
Yes. But also, this horribleness is also to illustrate a weak point in the API
in that ref_transaction_commit actually frees the transaction if
successful, so the
&& !(transaction = NULL) kludge is to avoid a double free in the
ref_transaction_rollback.
This is horrible, but all this is going away later in the patch
series when _commit is fixed so that
it does not free the transaction anymore.
When that patch comes in later in this series, this horribleness will go away.
I think it would be simpler for the caller to
unconditionally set transaction to NULL after calling
ref_transaction_commit in such a case to avoid use-after-free.
Yes. Later patches does that by having ref_transaction_commit not free
the transaction
and instead requiring the caller to explicitely free it by calling
ref_transaction_free.
Maybe see this as this is how ugly rollback is by the current _commit
semantics. Then see how beautiful it
all gets once _commit is repaired and the && !(transaction = NULL)
kludge is removed. :-)
Even better if it is the caller's responsibility to free
the transaction. At any rate, it doesn't seem related to this
patch.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:08
Ronnie Sahlberg wrote:
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.
Very nice.
[...]
quoted hunk
+++ b/refs.c
[...]
quoted hunk
@@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, update->flags, &update->type, onerr); if (!update->lock) {+ if (err)+ strbuf_addf(err ,"Cannot lock the ref '%s'.",+ update->refname);
Probably worth mentioning the error string doesn't end with a newline
so the caller knows how to use it.
With the whitespace fix and with or without the comment tweak,
Reviewed-by: Jonathan Nieder <redacted>
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:08
Ronnie Sahlberg wrote:
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument.
Sensible. The caller-visible effect would be that now
ref_transaction_commit() can pass back a helpful error message through
its "err" argument when asked to make multiple updates for the same
ref.
Reviewed-by: Jonathan Nieder <redacted>
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:08
Thanks.
I changed the commit message.
On Tue, May 13, 2014 at 3:44 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
Allow ref_transaction_free to be called with NULL and in extension allow
ref_transaction_rollback to be called for a NULL transaction.
In extension = as a result?
Makes sense. It lets someone do the usual
struct ref_transaction *transaction;
int ret = 0;
if (something_fails()) {
ret = -1;
goto cleanup;
}
...
cleanup:
ref_transaction_free(transaction);
return ret;
just like you can already do with free().
quoted
This allows us to write code that will
if ( (!transaction ||
ref_transaction_update(...)) ||
(ref_transaction_commit(...) && !(transaction = NULL)) {
ref_transaction_rollback(transaction);
...
}
Somewhere in the whitespace and parentheses I'm lost.
Is the idea that when ref_transaction_commit fails it will have
freed the transaction so we need not to roll back to prevent a
double free? I think it would be simpler for the caller to
unconditionally set transaction to NULL after calling
ref_transaction_commit in such a case to avoid use-after-free.
Even better if it is the caller's responsibility to free
the transaction. At any rate, it doesn't seem related to this
patch.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:08
Thanks.
Comment added and whitespace fixed.
On Tue, May 13, 2014 at 4:10 PM, Jonathan Nieder [off-list ref] wrote:
quoted hunk
Ronnie Sahlberg wrote:
quoted
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.
Very nice.
[...]
quoted
+++ b/refs.c
[...]
quoted
@@ -3443,6 +3444,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, update->flags, &update->type, onerr); if (!update->lock) {+ if (err)+ strbuf_addf(err ,"Cannot lock the ref '%s'.",+ update->refname);
Probably worth mentioning the error string doesn't end with a newline
so the caller knows how to use it.
With the whitespace fix and with or without the comment tweak,
Reviewed-by: Jonathan Nieder <redacted>
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:08
Thanks.
I updated the commit message to highlight that this means the error
string can now be passed all the way back to the caller.
On Tue, May 13, 2014 at 5:04 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument.
Sensible. The caller-visible effect would be that now
ref_transaction_commit() can pass back a helpful error message through
its "err" argument when asked to make multiple updates for the same
ref.
Reviewed-by: Jonathan Nieder <redacted>
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);
- return ret;
+ if (ref_transaction_commit(transaction, msg, &err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die("%s", err.buf);
Nice. I like this much more than passing a flag to each function to
tell it how to handle errors. :)
ref_transaction_commit didn't have any stray codepaths that return
some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
so this should be safe as far as the exit code is concerned.
The only danger would be that some codepath leaves 'err' alone and
forgets to write a messages, so we die with
"fatal: "
Alas, it looks like this patch can do that.
i. The call to update_ref_write can error out without updating the
error string.
ii. delete_ref_loose can print a message and then fail without updating
the error string so the output looks like
warning: unable to unlink .git/refs/heads/master.lock: Permission denied
fatal:
$
iii. repack_without_refs can similarly return an error
error: Unable to create '/home/jrn/test/.git/packed-refs.lock: Permission denied
error: cannot delete 'refs/heads/master' from packed refs
fatal:
$
iv. commit_lock_file in commit_packed_refs is silent on error.
repack_without_refs probably intends to write a message in that
case but doesn't :(
I wish there were some way to automatically detect missed spots or
make them stand out (like with the current "return error()" idiom a
bare "return -1" stands out).
(i) is fixed by a later patch. It would be better to put that before
this one for bisectability.
I don't see fixes to (ii), (iii), and (iv) in the series yet from a
quick glance.
Thanks,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
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.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
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.
Nice, and obviously correct.
Reviewed-by: Jonathan Nieder <redacted>
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
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(-)
Revisiting comments from [1]:
* When I call ref_transaction_update, what does it mean that I get
a nonzero return value? Does it mean the _update failed and had
no effect? What will I want to do next: should I try again or
print an error and exit?
Ideally I should be able to answer these questions by reading
the signature of ref_transaction_update and the comment documenting
it. The comment doesn't say anything about what errors
mean here.
* the error message change for the have_old && !old_sha1 case (to add
"BUG:" so users know the impossible has happened and translators
know not to bother with it) seems to have snuck ahead into patch 28
(refs.c: add transaction.status and track OPEN/CLOSED/ERROR).
* It would be easier to make sense of the error path (does the error
message have enough information? Will the user be bewildered?)
if there were an example of how ref_transaction_update can fail.
There still doesn't seem to be one by the end of the series.
The general idea still seems sensible.
Thanks,
Jonathan
[1] http://thread.gmane.org/gmane.comp.version-control.git/246437/focus=247115
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
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 _create that will return error but there
will be in the future.
If it were ever triggered, the message
error: some bad thing
fatal: failed transaction create for refs/heads/master
looks overly verbose and unclear. Something like
fatal: cannot create ref refs/heads/master: some bad thing
might work better. It's hard to tell without an example in mind.
[...]
- assert(!is_null_sha1(new_sha1));
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die("create ref with null new_sha1");
One less 'assert' is nice. :)
As with _update, the message should start with "BUG:" to make it clear
to users and translators that this should never happen, even with
malformed user input. That gets corrected in patch 28 but it's
clearer to include it from the start.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
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 _delete that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg <redacted>
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
quoted hunk
--- a/builtin/tag.c+++ b/builtin/tag.c
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)if(annotate)create_tag(object,tag,&buf,&opt,prev,object);-lock=lock_any_ref_for_update(ref.buf,prev,0,NULL);-if(!lock)-die(_("%s: cannot lock the ref"),ref.buf);-if(write_ref_sha1(lock,object,NULL)<0)-die(_("%s: cannot update the ref"),ref.buf);+transaction=ref_transaction_begin();+if(!transaction||+ref_transaction_update(transaction,ref.buf,object,prev,+0,!is_null_sha1(prev))||+ref_transaction_commit(transaction,NULL,&err))+die(_("%s: cannot update the ref: %s"),ref.buf,err.buf);
Makes sense for the _update and _commit case. (BTW, why is have_old
a separate boolean instead of a bit in flags?)
For the _begin() case, can ref_transaction_begin() ever fail? xcalloc
die()s on allocation failure. So I think it's fine to assume
transaction is non-null (i.e., drop the !transaction condition), or if
you want to be defensive, then label it as a bug --- e.g.:
if (!transaction)
die("BUG: ref_transaction_begin() returned NULL?");
Otherwise if ref_transaction_begin regresses in the future and this
case is tripped then the message would be
fatal: refs/tags/v1.0: cannot update the ref:
which is not as obvious an indicator that the user should contact
the mailing list.
Thanks,
Jonathan
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:09
Ronnie Sahlberg wrote:
[...]
quoted hunk
+++ b/builtin/replace.c
[...]
quoted hunk
@@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, else if (!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);
Same question about the !transaction case.
This makes the message translated, which is a nice change but not
mentioned in the commit message. (Generally speaking, I don't mind
either way about adding or not adding _() to new messages in files
that have not already undergone a pass of marking everything for
translation.)
Same question about !transaction (it also applies to later patches but I
won't mention it any more).
The error message changed from
fatal: cannot lock HEAD ref
to
fatal: HEAD: cannot update ref: Cannot lock the ref 'HEAD'.
Does the message from ref_transaction_commit always say what ref
was being updated when it failed? If so, it's tempting to just use
the message as-is:
fatal: Cannot lock the ref 'HEAD'
If the caller should add to the message, it could say something about
the context --- e.g.,
fatal: cannot update HEAD with new commit: cannot lock the ref 'HEAD'
Looking at that,
die("%s", err.buf)
seems simplest since even if "git commit" was being called in a loop,
it's already clear that git was trying to lock HEAD to advance it.
Thanks,
Jonathan
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);
- return ret;
+ if (ref_transaction_commit(transaction, msg, &err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die("%s", err.buf);
Nice. I like this much more than passing a flag to each function to
tell it how to handle errors. :)
ref_transaction_commit didn't have any stray codepaths that return
some other exit code instead of die()-ing with UPDATE_REFS_DIE_ON_ERR,
so this should be safe as far as the exit code is concerned.
The only danger would be that some codepath leaves 'err' alone and
forgets to write a messages, so we die with
"fatal: "
Alas, it looks like this patch can do that.
i. The call to update_ref_write can error out without updating the
error string.
Fixed.
I reordered the patches so the change to update_ref_write to take an
err argument will come before the change to update-ref.c as you
suggested.
ii. delete_ref_loose can print a message and then fail without updating
the error string so the output looks like
warning: unable to unlink .git/refs/heads/master.lock: Permission denied
fatal:
$
Fixed.
I have added a new patch before the change to update-ref.c to add err
to delete_ref_loose.
iii. repack_without_refs can similarly return an error
error: Unable to create '/home/jrn/test/.git/packed-refs.lock: Permission denied
error: cannot delete 'refs/heads/master' from packed refs
fatal:
$
iv. commit_lock_file in commit_packed_refs is silent on error.
repack_without_refs probably intends to write a message in that
case but doesn't :(
Fixed.
I added a patch to take an err argument to repack_without_refs and
update it for both
conditions iii and iv.
I wish there were some way to automatically detect missed spots or
make them stand out (like with the current "return error()" idiom a
bare "return -1" stands out).
(i) is fixed by a later patch. It would be better to put that before
this one for bisectability.
I don't see fixes to (ii), (iii), and (iv) in the series yet from a
quick glance.
Fixed in the next version of the patch series I will send out.
Thanks.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:10
On Wed, May 14, 2014 at 4:40 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
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(-)
Revisiting comments from [1]:
* When I call ref_transaction_update, what does it mean that I get
a nonzero return value? Does it mean the _update failed and had
no effect? What will I want to do next: should I try again or
print an error and exit?
It means the transaction will no longer work and must be rolled back.
See below for the updated text I added to refs.h
Ideally I should be able to answer these questions by reading
the signature of ref_transaction_update and the comment documenting
it. The comment doesn't say anything about what errors
mean here.
I have updated the description to include :
* Function returns 0 on success and non-zero on failure. A failure to update
* means that the transaction as a whole has failed and will need to be
* rolled back.
* the error message change for the have_old && !old_sha1 case (to add
"BUG:" so users know the impossible has happened and translators
know not to bother with it) seems to have snuck ahead into patch 28
(refs.c: add transaction.status and track OPEN/CLOSED/ERROR).
Done.
* It would be easier to make sense of the error path (does the error
message have enough information? Will the user be bewildered?)
if there were an example of how ref_transaction_update can fail.
There still doesn't seem to be one by the end of the series.
This patch series got a lot longer than I initially thought so I did
not get to the point where we it would make sense
to start returning !0. :-(
The next patchseries I sent out for review does add things to _update
that will cause it to return failures.
For example, locking the ref there happens in _update instead of
_commit and then it starts make sense to
return failures back to the caller for things such as "Multiple
updates for ref '%s' not allowed."
Unfortunate but since this patch series reached >40 patches I did not
want to continue expanding on it.
This means that actually starting to use "let _update return error"
did not actually start becomming used until the second
patch series, which now is well over 30 patches in size :-(
I just felt I had to stop growing this series or it would never be finished.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:10
On Wed, May 14, 2014 at 5:04 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
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 _create that will return error but there
will be in the future.
If it were ever triggered, the message
error: some bad thing
fatal: failed transaction create for refs/heads/master
looks overly verbose and unclear. Something like
fatal: cannot create ref refs/heads/master: some bad thing
I changed it to :
die("cannot create ref '%s'", refname);
But it would still mean you would have
error: some bad thing
fatal: cannot create 'refs/heads/master'
To make it better we have to wait until the end of the second patch
series, ref-transactions-next
where we will have an err argument to _update/_create/_delete too and
thus we can do this from update-ref.c :
if (transaction_create_sha1(transaction, refname, new_sha1,
update_flags, msg, &err))
die("%s", err.buf);
might work better. It's hard to tell without an example in mind.
[...]
- assert(!is_null_sha1(new_sha1));
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die("create ref with null new_sha1");
One less 'assert' is nice. :)
As with _update, the message should start with "BUG:" to make it clear
to users and translators that this should never happen, even with
malformed user input. That gets corrected in patch 28 but it's
clearer to include it from the start.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:10
On Wed, May 14, 2014 at 5:19 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
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 _delete that will return error but there
will be in the future.
Signed-off-by: Ronnie Sahlberg <redacted>
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:10
On Wed, May 14, 2014 at 5:27 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
quoted
--- a/builtin/tag.c+++ b/builtin/tag.c
@@ -701,11 +702,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)if(annotate)create_tag(object,tag,&buf,&opt,prev,object);-lock=lock_any_ref_for_update(ref.buf,prev,0,NULL);-if(!lock)-die(_("%s: cannot lock the ref"),ref.buf);-if(write_ref_sha1(lock,object,NULL)<0)-die(_("%s: cannot update the ref"),ref.buf);+transaction=ref_transaction_begin();+if(!transaction||+ref_transaction_update(transaction,ref.buf,object,prev,+0,!is_null_sha1(prev))||+ref_transaction_commit(transaction,NULL,&err))+die(_("%s: cannot update the ref: %s"),ref.buf,err.buf);
Makes sense for the _update and _commit case. (BTW, why is have_old
a separate boolean instead of a bit in flags?)
For the _begin() case, can ref_transaction_begin() ever fail? xcalloc
die()s on allocation failure. So I think it's fine to assume
transaction is non-null (i.e., drop the !transaction condition), or if
you want to be defensive, then label it as a bug --- e.g.:
if (!transaction)
die("BUG: ref_transaction_begin() returned NULL?");
Otherwise if ref_transaction_begin regresses in the future and this
case is tripped then the message would be
fatal: refs/tags/v1.0: cannot update the ref:
which is not as obvious an indicator that the user should contact
the mailing list.
For the current refs implementation, _begin can never return NULL
since the only failure mode would be OOM in which case we die().
And then for that case we could remove the !transaction check since transaction
can never be NULL here.
(I am not a big fan of die() in general)
However, if we implement a different datastore for refs in the future
it is likely that
the ref_transaction_begin equivalent for that backend could well start returning
failures for a lot other reasons than just OOM.
I could imagine that tdb_transaction_start() could fail for a
corrupted database.
An SQL based backend could fail due to the client library failing to
open a socket to the db,
etc.
But you bring a good point about the error message.
Instead of the suggestions above, would you accept an alternative
approach where I would
add an err argument to ref_transaction_begin() instead?
For a hypothetical mysql backend, this could then do something like :
Which could then result in output like
fatal: refs/heads/master: cannot update the ref: failed to connect to
mysql database ...
So I suggest that instead of doing these changes I will add an err
argument to ref_transaction_begin.
Does that sound ok with you?
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:10
On Wed, May 14, 2014 at 5:30 PM, Jonathan Nieder [off-list ref] wrote:
Ronnie Sahlberg wrote:
[...]
quoted
+++ b/builtin/replace.c
[...]
quoted
@@ -157,11 +158,12 @@ static int replace_object(const char *object_ref, const char *replace_ref, else if (!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);
Same question about the !transaction case.
This makes the message translated, which is a nice change but not
mentioned in the commit message. (Generally speaking, I don't mind
either way about adding or not adding _() to new messages in files
that have not already undergone a pass of marking everything for
translation.)
Removed the _. This series is long enough as is so lets not start
worrying about translations too.
Same opinion about the ref_transaction_begin() case as before. I think
it will be better to just add err to it
since it is likely this will be useful for future non-file based ref backends.
From: Jonathan Nieder <hidden> Date: 2016-06-15 23:01:10
Ronnie Sahlberg wrote:
Instead of the suggestions above, would you accept an alternative
approach where I would
add an err argument to ref_transaction_begin() instead?
For a hypothetical mysql backend, this could then do something like :
[...]
fatal: refs/heads/master: cannot update the ref: failed to connect to mysql database ...
Yes, sounds like a good thing to do.
Thanks,
Jonathan