From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions
This patch series is based on current master 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 20:
- Whitespace and style changes suggested by Jun.
Ronnie Sahlberg (48):
refs.c: remove ref_transaction_rollback
refs.c: ref_transaction_commit should not free the transaction
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
lockfile.c: add a new public function unable_to_lock_message
lockfile.c: make lock_file return a meaningful errno on failurei
refs.c: add an err argument to repack_without_refs
refs.c: make sure log_ref_setup returns a meaningful errno
refs.c: verify_lock should set errno to something meaningful
refs.c: make remove_empty_directories always set errno to something
sane
refs.c: commit_packed_refs to return a meaningful errno on failure
refs.c: make resolve_ref_unsafe set errno to something meaningful on
error
refs.c: log_ref_write should try to return meaningful errno
refs.c: make ref_update_reject_duplicates take a strbuf argument for
errors
refs.c: make update_ref_write update a strbuf on failure
update-ref: use err argument to get error from ref_transaction_commit
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: update ref_transaction_delete to check for error and return
status
refs.c: make ref_transaction_begin take an err argument
refs.c: add transaction.status and track OPEN/CLOSED/ERROR
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
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 lock_ref_sha1 static
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: add an err argument to delete_ref_loose
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: move the check for valid refname to lock_ref_sha1_basic
refs.c: call lock_ref_sha1_basic directly from commit
refs.c: pass a skip list to name_conflict_fn
refs.c: propagate any errno==ENOTDIR from _commit back to the callers
fetch.c: change s_update_ref to use a ref transaction
refs.c: make write_ref_sha1 static
branch.c | 30 +--
builtin/commit.c | 24 ++-
builtin/fetch.c | 36 ++--
builtin/receive-pack.c | 97 ++++++----
builtin/remote.c | 5 +-
builtin/replace.c | 15 +-
builtin/tag.c | 15 +-
builtin/update-ref.c | 34 ++--
cache.h | 4 +-
fast-import.c | 53 ++++--
lockfile.c | 39 ++--
refs.c | 506 ++++++++++++++++++++++++++++++++-----------------
refs.h | 131 +++++++++----
sequencer.c | 24 ++-
walker.c | 58 +++---
wrapper.c | 14 +-
16 files changed, 710 insertions(+), 375 deletions(-)
--
2.0.0.420.g181e020.dirty
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
Making errno when returning from lock_file() meaningful, which should
fix
* an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries
* an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point
Signed-off-by: Ronnie Sahlberg <redacted>
---
lockfile.c | 17 ++++++++++++-----
refs.c | 1 +
refs.h | 1 +
3 files changed, 14 insertions(+), 5 deletions(-)
@@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)returnp;}-+/* Make sure errno contains a meaningful value on error */staticintlock_file(structlock_file*lk,constchar*path,intflags){/*
@@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)*/staticconstsize_tmax_path_len=sizeof(lk->filename)-5;-if(strlen(path)>=max_path_len)+if(strlen(path)>=max_path_len){+errno=ENAMETOOLONG;return-1;+}strcpy(lk->filename,path);if(!(flags&LOCK_NODEREF))resolve_symlink(lk->filename,max_path_len);
@@ -148,9 +150,13 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)lock_file_list=lk;lk->on_list=1;}-if(adjust_shared_perm(lk->filename))-returnerror("cannot fix permission bits on %s",-lk->filename);+if(adjust_shared_perm(lk->filename)){+intsave_errno=errno;+error("cannot fix permission bits on %s",+lk->filename);+errno=save_errno;+return-1;+}}elselk->filename[0]=0;
@@ -188,6 +194,7 @@ NORETURN void unable_to_lock_index_die(const char *path, int err)die("%s",buf.buf);}+/* This should return a meaningful errno on failure */inthold_lock_file_for_update(structlock_file*lk,constchar*path,intflags){intfd=lock_file(lk,path,flags);
@@ -2212,6 +2212,7 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)return0;}+/* This should return a meaningful errno on failure */intlock_packed_refs(intflags){structpacked_ref_cache*packed_ref_cache;
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free
easier to use and more similar to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
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.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 2 +-
refs.c | 6 +++++-
refs.h | 5 ++++-
3 files changed, 10 insertions(+), 3 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.
Signed-off-by: Ronnie Sahlberg <redacted>
---
cache.h | 2 ++
lockfile.c | 22 ++++++++++++----------
2 files changed, 14 insertions(+), 10 deletions(-)
@@ -157,33 +157,35 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)returnlk->fd;}-staticchar*unable_to_lock_message(constchar*path,interr)+voidunable_to_lock_message(constchar*path,interr,structstrbuf*buf){-structstrbufbuf=STRBUF_INIT;-if(err==EEXIST){-strbuf_addf(&buf,"Unable to create '%s.lock': %s.\n\n"+strbuf_addf(buf,"Unable to create '%s.lock': %s.\n\n""If no other git process is currently running, this probably means a\n""git process crashed in this repository earlier. Make sure no other git\n""process is running and remove the file manually to continue.",absolute_path(path),strerror(err));}else-strbuf_addf(&buf,"Unable to create '%s.lock': %s",+strbuf_addf(buf,"Unable to create '%s.lock': %s",absolute_path(path),strerror(err));-returnstrbuf_detach(&buf,NULL);}intunable_to_lock_error(constchar*path,interr){-char*msg=unable_to_lock_message(path,err);-error("%s",msg);-free(msg);+structstrbufbuf=STRBUF_INIT;++unable_to_lock_message(path,err,&buf);+error("%s",buf.buf);+strbuf_release(&buf);return-1;}NORETURNvoidunable_to_lock_index_die(constchar*path,interr){-die("%s",unable_to_lock_message(path,err));+structstrbufbuf=STRBUF_INIT;++unable_to_lock_message(path,err,&buf);+die("%s",buf.buf);}inthold_lock_file_for_update(structlock_file*lk,constchar*path,intflags)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
Making errno when returning from commit_packed_refs() meaningful,
which should fix
* a bug in "git clone" where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
* the same kind of bug in "git pack-refs"
and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 10 +++++++++-
refs.h | 1 +
2 files changed, 10 insertions(+), 1 deletion(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to this function.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/remote.c | 5 +++--
refs.c | 19 ++++++++++++++-----
refs.h | 3 ++-
3 files changed, 19 insertions(+), 8 deletions(-)
@@ -2456,12 +2456,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)return0;}-intrepack_without_refs(constchar**refnames,intn)+intrepack_without_refs(constchar**refnames,intn,structstrbuf*err){structref_dir*packed;structstring_listrefs_to_delete=STRING_LIST_INIT_DUP;structstring_list_item*ref_to_delete;-inti,removed=0;+inti,ret,removed=0;/* Look for a packed ref */for(i=0;i<n;i++)
@@ -2473,6 +2473,11 @@ int repack_without_refs(const char **refnames, int n)return0;/* no refname exists in packed refs */if(lock_packed_refs(0)){+if(err){+unable_to_lock_message(git_path("packed-refs"),errno,+err);+return-1;+}unable_to_lock_error(git_path("packed-refs"),errno);returnerror("cannot delete '%s' from packed refs",refnames[i]);}
@@ -2499,12 +2504,16 @@ int repack_without_refs(const char **refnames, int n)}/* Write what remains */-returncommit_packed_refs();+ret=commit_packed_refs();+if(ret&&err)+strbuf_addf(err,"unable to overwrite old ref-pack file: %s",+strerror(errno));+returnret;}staticintrepack_without_ref(constchar*refname){-returnrepack_without_refs(&refname,1);+returnrepack_without_refs(&refname,1,NULL);}staticintdelete_ref_loose(structref_lock*lock,intflag)
@@ -3508,7 +3517,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,}}-ret|=repack_without_refs(delnames,delnum);+ret|=repack_without_refs(delnames,delnum,err);for(i=0;i<delnum;i++)unlink_or_warn(git_path("logs/%s",delnames[i]));clear_loose_ref_cache(&ref_cache);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
@@ -3495,6 +3496,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;
@@ -3525,7 +3529,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:01:42
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.
Reviewed-by: Jonathan Nieder <redacted>
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:01:42
We do not yet need both a rollback and a free function for transactions.
Remove ref_transaction_rollback and use ref_transaction_free instead.
At a later stage we may reintroduce a rollback function if we want to start
adding reusable transactions and similar.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 7 +------
refs.h | 16 +++++++---------
2 files changed, 8 insertions(+), 15 deletions(-)
@@ -280,6 +273,11 @@ void ref_transaction_delete(struct ref_transaction *transaction,intref_transaction_commit(structref_transaction*transaction,constchar*msg,enumaction_on_erronerr);+/*+*Freeanexistingtransactionandallassociateddata.+*/+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:01:42
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Simplify warn_if_unremovable.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 33 ++++++++++++++++++++++++++++-----
wrapper.c | 14 ++++++--------
2 files changed, 34 insertions(+), 13 deletions(-)
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)staticintwarn_if_unremovable(constchar*op,constchar*file,intrc){-if(rc<0){-interr=errno;-if(ENOENT!=err){-warning("unable to %s %s: %s",-op,file,strerror(errno));-errno=err;-}-}+interr;+if(rc>=0||errno==ENOENT)+returnrc;+err=errno;+warning("unable to %s %s: %s",op,file,strerror(errno));+errno=err;returnrc;}
@@ -2665,6 +2665,9 @@ static int rename_tmp_log(const char *newrefname)return0;}+staticintwrite_ref_sha1(structref_lock*lock,constunsignedchar*sha1,+constchar*logmsg);+intrename_ref(constchar*oldrefname,constchar*newrefname,constchar*logmsg){unsignedcharsha1[20],orig_sha1[20];
@@ -2914,8 +2917,11 @@ static int is_branch(const char *refname)return!strcmp(refname,"HEAD")||starts_with(refname,"refs/heads/");}-/* This function must return a meaningful errno */-intwrite_ref_sha1(structref_lock*lock,+/*+*Writessha1intotherefspecifiedbythelock.Makessurethaterrno+*issaneonerror.+*/+staticintwrite_ref_sha1(structref_lock*lock,constunsignedchar*sha1,constchar*logmsg){staticcharterm='\n';
@@ -203,9 +203,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);-/**Setupreflogbeforeusing.Seterrnotosomethingmeaningfulonfailure.*/
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
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. This flag is private to
refs.c and not exposed to external callers.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 27 ++++++++++++++++++++-------
refs.h | 14 ++++++++++++--
2 files changed, 32 insertions(+), 9 deletions(-)
@@ -177,9 +177,19 @@ extern int ref_exists(const char *);*/externintpeel_ref(constchar*refname,unsignedchar*sha1);-/** Locks any ref (for 'HEAD' type refs). */+/*+*Flagscontrollinglock_any_ref_for_update(),ref_transaction_update(),+*ref_transaction_create(),etc.+*REF_NODEREF:actontherefdirectly,insteadofdereferencing+*symbolicreferences.+*+*Flags>=0x100arereservedforinternaluse.+*/#define REF_NODEREF 0x01-/* errno is set to something meaningful on failure */+/*+*Locksanyref(for'HEAD'typerefs)andsetserrnotosomething+*meaningfulonfailure.+*/externstructref_lock*lock_any_ref_for_update(constchar*refname,constunsignedchar*old_sha1,intflags,int*type_p);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
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.
Change delete_ref to return 0 on success and 1 on failure instead of the
previous 0 on success either 1 or -1 on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
@@ -2544,11 +2544,6 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)returnret;}-staticintrepack_without_ref(constchar*refname)-{-returnrepack_without_refs(&refname,1,NULL);-}-staticintdelete_ref_loose(structref_lock*lock,intflag){if(!(flag&REF_ISPACKED)||flag&REF_ISSYMREF){
@@ -2566,24 +2561,21 @@ 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;+structstrbuferr=STRBUF_INIT;-lock=lock_ref_sha1_basic(refname,sha1,delopt,&flag);-if(!lock)+transaction=ref_transaction_begin(&err);+if(!transaction||+ref_transaction_delete(transaction,refname,sha1,delopt,+sha1&&!is_null_sha1(sha1),&err)||+ref_transaction_commit(transaction,NULL,&err)){+error("%s",err.buf);+ref_transaction_free(transaction);+strbuf_release(&err);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:01:42
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.
Reviewed-by: Jonathan Nieder <redacted>
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:01:42
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/fetch.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:42
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.
If lock_ref_sha1_basic fails the check_refname_format test, set errno to
EINVAL before returning NULL. This to guarantee that we will not return an
error without updating errno.
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.
This changes semantics for lock_ref_sha1_basic slightly. With this change
it is no longer possible to open a ref that has a badly name which breaks
any codepaths that tries to open and repair badly named refs. The normal refs
API should not allow neither creating nor accessing refs with invalid names.
If we need such recovery code we could add it as an option to git fsck and have
git fsck be the only sanctioned way of bypassing the normal API and checks.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Reviewed-by: Jonathan Nieder <redacted>
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:01:43
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when
we lstat the new refname and it returns ENOTDIR or if the name checking
function reports that the same type of conflict happened. In both cases it
means that we can not create the new ref due to a name conflict.
For these cases, save the errno value and abort and make sure that the caller
can see errno==ENOTDIR.
Also start defining specific return codes for _commit, assign -1 as a generic
error and -2 as the error that refers to a name conflict. Callers can (and
should) use that return value inspecting errno directly.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 22 +++++++++++++++-------
refs.h | 6 ++++++
2 files changed, 21 insertions(+), 7 deletions(-)
@@ -3582,7 +3582,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n,intref_transaction_commit(structref_transaction*transaction,structstrbuf*err){-intret=0,delnum=0,i;+intret=0,delnum=0,i,df_conflict=0;constchar**delnames;intn=transaction->nr;structref_update**updates=transaction->updates;
@@ -3600,9 +3600,10 @@ 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,err);-if(ret)+if(ref_update_reject_duplicates(updates,n,err)){+ret=-1;gotocleanup;+}/* Acquire all locks while verifying old values */for(i=0;i<n;i++){
@@ -3616,10 +3617,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,&update->type,delnames,delnum);if(!update->lock){+if(errno==ENOTDIR)+df_conflict=1;if(err)strbuf_addf(err,"Cannot lock the ref '%s'.",update->refname);-ret=1;+ret=-1;gotocleanup;}}
@@ -3637,6 +3640,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,if(err)strbuf_addf(err,str,update->refname);+ret=-1;gotocleanup;}}
@@ -3647,14 +3651,16 @@ int ref_transaction_commit(struct ref_transaction *transaction,structref_update*update=updates[i];if(update->lock){-ret|=delete_ref_loose(update->lock,update->type,-err);+if(delete_ref_loose(update->lock,update->type,err))+ret=-1;+if(!(update->flags&REF_ISPRUNING))delnames[delnum++]=update->lock->ref_name;}}-ret|=repack_without_refs(delnames,delnum,err);+if(repack_without_refs(delnames,delnum,err))+ret=-1;for(i=0;i<delnum;i++)unlink_or_warn(git_path("logs/%s",delnames[i]));clear_loose_ref_cache(&ref_cache);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Allow passing a list of refs to skip 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 and 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 | 41 ++++++++++++++++++++++++++---------------
1 file changed, 26 insertions(+), 15 deletions(-)
@@ -2077,7 +2083,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)/* This function should make sure errno is meaningful on error */staticstructref_lock*lock_ref_sha1_basic(constchar*refname,constunsignedchar*old_sha1,-intflags,int*type_p)+intflags,int*type_p,+constchar**skip,intskipnum){char*ref_file;constchar*orig_refname=refname;
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c | 3 +--
builtin/receive-pack.c | 5 +++--
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 | 5 ++---
12 files changed, 52 insertions(+), 44 deletions(-)
@@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,}}}-if(rc&STORE_REF_ERROR_DF_CONFLICT)error(_("some local refs could not be updated; try running\n"-" 'git remote prune %s' to remove any old, conflicting "+"'git remote prune %s' to remove any old, conflicting ""branches"),remote_name);abort:
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Change create_branch to use a ref transaction when creating the new branch.
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 existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Reviewed-by: Jonathan Nieder <redacted>
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(&err);+if(!transaction||+ref_transaction_update(transaction,ref.buf,sha1,+null_sha1,0,!forcing,&err)||+ref_transaction_commit(transaction,msg,&err))+die("%s",err.buf);+ref_transaction_free(transaction);+}+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:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
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:01:43
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. This changes the return status for _commit from
1 to -1 on failures when writing to the ref. Eventually we will want
_commit to start returning more detailed error conditions than the current
simple success/failure. For example if the commit failed due to name
conflicts etc.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 35 +++++++++--------------------------
1 file changed, 9 insertions(+), 26 deletions(-)
@@ -3333,25 +3333,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
@@ -3605,14 +3586,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:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/commit.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Change the update_ref helper function to use a ref transaction internally.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
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:01:43
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.
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.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 24 deletions(-)
@@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref)intwalker_fetch(structwalker*walker,inttargets,char**target,constchar**write_ref,constchar*write_ref_log_details){-structref_lock**lock=xcalloc(targets,sizeof(structref_lock*));+structstrbufref_name=STRBUF_INIT;+structstrbuferr=STRBUF_INIT;+structref_transaction*transaction=NULL;unsignedchar*sha1=xmalloc(targets*20);-char*msg;-intret;+char*msg=NULL;inti;save_commit_buffer=0;-for(i=0;i<targets;i++){-if(!write_ref||!write_ref[i])-continue;--lock[i]=lock_ref_sha1(write_ref[i],NULL);-if(!lock[i]){-error("Can't lock ref %s",write_ref[i]);-gotounlock_and_fail;+if(write_ref){+transaction=ref_transaction_begin(&err);+if(!transaction){+error("%s",err.buf);+gotorollback_and_fail;}}-if(!walker->get_recover)for_each_ref(mark_complete,NULL);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 +291,33 @@ 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;+strbuf_reset(&ref_name);+strbuf_addf(&ref_name,"refs/%s",write_ref[i]);+if(ref_transaction_update(transaction,ref_name.buf,+&sha1[20*i],NULL,0,0,+&err)){+error("%s",err.buf);+gotorollback_and_fail;+}+}+if(write_ref){+if(ref_transaction_commit(transaction,+msg?msg:"fetch (unknown)",+&err)){+error("%s",err.buf);+gotorollback_and_fail;+}+ref_transaction_free(transaction);}-free(msg);+free(msg);return0;-unlock_and_fail:-for(i=0;i<targets;i++)-if(lock[i])-unlock_ref(lock[i]);+rollback_and_fail:+ref_transaction_free(transaction);+free(msg);+strbuf_release(&err);+strbuf_release(&ref_name);return-1;}
@@ -468,19 +468,18 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)return0;}-staticconstchar*update(structcommand*cmd,structshallow_info*si)+staticchar*update(structcommand*cmd,structshallow_info*si){constchar*name=cmd->ref_name;structstrbufnamespaced_name_buf=STRBUF_INIT;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)){rp_error("refusing to create funny ref '%s' remotely",name);-return"funny refname";+returnxstrdup("funny refname");}strbuf_addf(&namespaced_name_buf,"%s%s",get_git_namespace(),name);
@@ -498,20 +497,20 @@ static const char *update(struct command *cmd, struct shallow_info *si)rp_error("refusing to update checked out branch: %s",name);if(deny_current_branch==DENY_UNCONFIGURED)refuse_unconfigured_deny();-return"branch is currently checked out";+returnxstrdup("branch is currently checked out");}}if(!is_null_sha1(new_sha1)&&!has_sha1_file(new_sha1)){error("unpack should have generated %s, ""but I can't find it!",sha1_to_hex(new_sha1));-return"bad pack";+returnxstrdup("bad pack");}if(!is_null_sha1(old_sha1)&&is_null_sha1(new_sha1)){if(deny_deletes&&starts_with(name,"refs/heads/")){rp_error("denying ref deletion for %s",name);-return"deletion prohibited";+returnxstrdup("deletion prohibited");}if(!strcmp(namespaced_name,head_name)){
@@ -526,7 +525,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)if(deny_delete_current==DENY_UNCONFIGURED)refuse_unconfigured_deny_delete_current();rp_error("refusing to delete the current branch: %s",name);-return"deletion of the current branch prohibited";+returnxstrdup("deletion of the current branch prohibited");}}}
@@ -571,24 +570,32 @@ static const char *update(struct command *cmd, struct shallow_info *si)}if(delete_ref(namespaced_name,old_sha1,0)){rp_error("failed to delete %s",name);-return"failed to delete";+returnxstrdup("failed to delete");}returnNULL;/* good */}else{+structstrbuferr=STRBUF_INIT;+structref_transaction*transaction;+if(shallow_update&&si->shallow_ref[cmd->index]&&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 */+returnxstrdup("shallow error");++transaction=ref_transaction_begin(&err);+if(!transaction||+ref_transaction_update(transaction,namespaced_name,+new_sha1,old_sha1,0,1,&err)||+ref_transaction_commit(transaction,"push",&err)){+char*str=strbuf_detach(&err,NULL);+ref_transaction_free(transaction);++rp_error("%s",str);+returnstr;}++ref_transaction_free(transaction);+strbuf_release(&err);returnNULL;/* good */}}
@@ -647,6 +654,9 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)charcmd_oldh[41],cmd_newh[41],dst_oldh[41],dst_newh[41];intflag;+if(cmd->error_string)+die("BUG: check_alised_update called with failed cmd");+strbuf_addf(&buf,"%s%s",get_git_namespace(),cmd->ref_name);dst_name=resolve_ref_unsafe(buf.buf,sha1,0,&flag);strbuf_release(&buf);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
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 | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
@@ -3437,6 +3458,9 @@ int ref_transaction_update(struct ref_transaction *transaction,{structref_update*update;+if(transaction->state!=REF_TRANSACTION_OPEN)+die("BUG: update called for transaction that is not open");+if(have_old&&!old_sha1)die("BUG: have_old is true but old_sha1 is NULL");
@@ -3457,6 +3481,9 @@ int ref_transaction_create(struct ref_transaction *transaction,{structref_update*update;+if(transaction->state!=REF_TRANSACTION_OPEN)+die("BUG: create called for transaction that is not open");+if(!new_sha1||is_null_sha1(new_sha1))die("BUG: create ref with null new_sha1");
@@ -3477,6 +3504,9 @@ int ref_transaction_delete(struct ref_transaction *transaction,{structref_update*update;+if(transaction->state!=REF_TRANSACTION_OPEN)+die("BUG: delete called for transaction that is not open");+if(have_old&&!old_sha1)die("BUG: have_old is true but old_sha1 is NULL");
@@ -3532,8 +3562,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,intn=transaction->nr;structref_update**updates=transaction->updates;-if(!n)+if(transaction->state!=REF_TRANSACTION_OPEN)+die("BUG: commit called for transaction that is not open");++if(!n){+transaction->state=REF_TRANSACTION_CLOSED;return0;+}/* Allocate work space */delnames=xmalloc(sizeof(*delnames)*n);
@@ -3595,6 +3630,9 @@ int ref_transaction_commit(struct ref_transaction *transaction,clear_loose_ref_cache(&ref_cache);cleanup:+transaction->state=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:01:43
Update ref_transaction_update() do some basic error checking and return
non-zero 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. Add an err argument that will be updated on
failure. In future patches we will start doing both locking and checking
for name conflicts in _update instead of _commit at which time this function
will start returning errors for these conditions.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 12 +++++++-----
refs.c | 18 ++++++++++++------
refs.h | 14 +++++++++-----
3 files changed, 28 insertions(+), 16 deletions(-)
@@ -342,7 +345,6 @@ 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")),
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. 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. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 5 +++--
refs.c | 16 +++++++++++-----
refs.h | 12 ++++++++----
3 files changed, 22 insertions(+), 11 deletions(-)
@@ -3469,19 +3469,25 @@ 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,+structstrbuf*err){-structref_update*update=add_update(transaction,refname);+structref_update*update;+if(have_old&&!old_sha1)+die("BUG: have_old is true but old_sha1 is NULL");++update=add_update(transaction,refname);update->flags=flags;update->have_old=have_old;if(have_old){assert(!is_null_sha1(old_sha1));hashcpy(update->old_sha1,old_sha1);}+return0;}intupdate_ref(constchar*action,constchar*refname,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Making errno when returning from remove_empty_directories() more
obviously meaningful, which should provide some peace of mind for
people auditing lock_ref_sha1_basic.
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
@@ -1960,14 +1960,16 @@ static int remove_empty_directories(const char *file)*onlyemptydirectories),removethem.*/structstrbufpath;-intresult;+intresult,save_errno;strbuf_init(&path,20);strbuf_addstr(&path,file);result=remove_dir_recursively(&path,REMOVE_DIR_EMPTY_ONLY);+save_errno=errno;strbuf_release(&path);+errno=save_errno;returnresult;}
@@ -2056,6 +2058,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)returnlogs_found;}+/* This function should make sure errno is meaningful on error */staticstructref_lock*lock_ref_sha1_basic(constchar*refname,constunsignedchar*old_sha1,intflags,int*type_p)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
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:01:43
Change tag.c to use ref transactions for all ref updates.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/tag.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Making errno from write_ref_sha1() meaningful, which should fix
* a bug in "git checkout -b" where it prints strerror(errno)
despite errno possibly being zero or clobbered
* a bug in "git fetch"'s s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
@@ -2859,8 +2859,19 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,len+=copy_msg(logrec+len-1,msg)-1;written=len<=maxlen?write_in_full(logfd,logrec,len):-1;free(logrec);-if(close(logfd)!=0||written!=len)-returnerror("Unable to append to %s",log_file);+if(written!=len){+intsave_errno=errno;+close(logfd);+error("Unable to append to %s",log_file);+errno=save_errno;+return-1;+}+if(close(logfd)){+intsave_errno=errno;+error("Unable to append to %s",log_file);+errno=save_errno;+return-1;+}return0;}
@@ -2869,14 +2880,17 @@ static int is_branch(const char *refname)return!strcmp(refname,"HEAD")||starts_with(refname,"refs/heads/");}+/* This function must return a meaningful errno */intwrite_ref_sha1(structref_lock*lock,constunsignedchar*sha1,constchar*logmsg){staticcharterm='\n';structobject*o;-if(!lock)+if(!lock){+errno=EINVAL;return-1;+}if(!lock->force_write&&!hashcmp(lock->old_sha1,sha1)){unlock_ref(lock);return0;
@@ -2886,19 +2900,23 @@ int write_ref_sha1(struct ref_lock *lock,error("Trying to write ref %s with nonexistent object %s",lock->ref_name,sha1_to_hex(sha1));unlock_ref(lock);+errno=EINVAL;return-1;}if(o->type!=OBJ_COMMIT&&is_branch(lock->ref_name)){error("Trying to write non-commit object %s to branch %s",sha1_to_hex(sha1),lock->ref_name);unlock_ref(lock);+errno=EINVAL;return-1;}if(write_in_full(lock->lock_fd,sha1_to_hex(sha1),40)!=40||-write_in_full(lock->lock_fd,&term,1)!=1-||close_ref(lock)<0){+write_in_full(lock->lock_fd,&term,1)!=1||+close_ref(lock)<0){+intsave_errno=errno;error("Couldn't write %s",lock->lk->filename);unlock_ref(lock);+errno=save_errno;return-1;}clear_loose_ref_cache(&ref_cache);
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
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,18 +360,17 @@ 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);+if(ref_transaction_commit(transaction,msg,&err,+UPDATE_REFS_QUIET_ON_ERR))+die("%s",err.buf);ref_transaction_free(transaction);-returnret;+return0;}if(end_null)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix
* a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way
Signed-off-by: Ronnie Sahlberg <redacted>
---
cache.h | 2 +-
refs.c | 19 +++++++++++++++----
2 files changed, 16 insertions(+), 5 deletions(-)
@@ -1334,6 +1334,7 @@ static const char *handle_missing_loose_ref(const char *refname,}}+/* This function needs to return a meaningful errno on failure */constchar*resolve_ref_unsafe(constchar*refname,unsignedchar*sha1,intreading,int*flag){intdepth=MAXDEPTH;
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _being with
"Can not connect to MySQL server. No route to host".
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix
* a bug in "git fetch"'s s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts
ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create. Should "git fetch" also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 4 ++++
refs.h | 6 +++++-
2 files changed, 9 insertions(+), 1 deletion(-)
@@ -1932,18 +1932,22 @@ int refname_match(const char *abbrev_name, const char *full_name)return0;}+/* This function should make sure errno is meaningful on error */staticstructref_lock*verify_lock(structref_lock*lock,constunsignedchar*old_sha1,intmustexist){if(read_ref_full(lock->ref_name,lock->old_sha1,mustexist,NULL)){+intsave_errno=errno;error("Can't verify ref %s",lock->ref_name);unlock_ref(lock);+errno=save_errno;returnNULL;}if(hashcmp(lock->old_sha1,old_sha1)){error("Ref %s is at %s but expected %s",lock->ref_name,sha1_to_hex(lock->old_sha1),sha1_to_hex(old_sha1));unlock_ref(lock);+errno=EBUSY;returnNULL;}returnlock;
@@ -137,11 +137,15 @@ 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. **/+/*+*Locksa"refs/"refreturningthelockonsuccessandNULLonfailure.+*Onfailureerrnoissettosomethingmeaningful.+*/externstructref_lock*lock_ref_sha1(constchar*refname,constunsignedchar*old_sha1);/** Locks any ref (for 'HEAD' type refs). */#define REF_NODEREF 0x01+/* errno is set to something meaningful on failure */externstructref_lock*lock_any_ref_for_update(constchar*refname,constunsignedchar*old_sha1,intflags,int*type_p);
@@ -177,12 +177,6 @@ extern int ref_exists(const char *);*/externintpeel_ref(constchar*refname,unsignedchar*sha1);-/*-*Locksa"refs/"refreturningthelockonsuccessandNULLonfailure.-*Onfailureerrnoissettosomethingmeaningful.-*/-externstructref_lock*lock_ref_sha1(constchar*refname,constunsignedchar*old_sha1);-/** Locks any ref (for 'HEAD' type refs). */#define REF_NODEREF 0x01/* errno is set to something meaningful on failure */
@@ -2751,6 +2751,7 @@ static int copy_msg(char *buf, const char *msg)returncp-buf;}+/* This function must set a meaningful errno on failure */intlog_ref_setup(constchar*refname,char*logfile,intbufsize){intlogfd,oflags=O_APPEND|O_WRONLY;
@@ -2761,9 +2762,12 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)starts_with(refname,"refs/remotes/")||starts_with(refname,"refs/notes/")||!strcmp(refname,"HEAD"))){-if(safe_create_leading_directories(logfile)<0)-returnerror("unable to create directory for %s",-logfile);+if(safe_create_leading_directories(logfile)<0){+intsave_errno=errno;+error("unable to create directory for %s",logfile);+errno=save_errno;+return-1;+}oflags|=O_CREAT;}
@@ -2774,15 +2778,22 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)if((oflags&O_CREAT)&&errno==EISDIR){if(remove_empty_directories(logfile)){-returnerror("There are still logs under '%s'",-logfile);+intsave_errno=errno;+error("There are still logs under '%s'",+logfile);+errno=save_errno;+return-1;}logfd=open(logfile,oflags,0666);}-if(logfd<0)-returnerror("Unable to append to %s: %s",-logfile,strerror(errno));+if(logfd<0){+intsave_errno=errno;+error("Unable to append to %s: %s",logfile,+strerror(errno));+errno=save_errno;+return-1;+}}adjust_shared_perm(logfile);
@@ -158,7 +158,9 @@ extern void unlock_ref(struct ref_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. **/+/*+*Setupreflogbeforeusing.Seterrnotosomethingmeaningfulonfailure.+*/intlog_ref_setup(constchar*refname,char*logfile,intbufsize);/** Reads log for the value of ref during at_time. **/
@@ -1689,29 +1690,32 @@ static int update_branch(struct branch *b)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);+if(!old_cmit||!new_cmit)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(&err);+if(!transaction||+ref_transaction_update(transaction,b->name,b->sha1,old_sha1,+0,1,&err)||+ref_transaction_commit(transaction,msg,&err)){+ref_transaction_free(transaction);+error("%s",err.buf);+strbuf_release(&err);+return-1;+}+ref_transaction_free(transaction);return0;}
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. 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. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 4 +++-
refs.c | 18 +++++++++++------
refs.h | 55 +++++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 63 insertions(+), 14 deletions(-)
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
Change to use ref transactions for all updates to refs.
Reviewed-by: Jonathan Nieder <redacted>
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(&err);+if(!transaction||+ref_transaction_update(transaction,"HEAD",to,from,+0,!unborn,&err)||+ref_transaction_commit(transaction,sb.buf,&err)){+ref_transaction_free(transaction);+error("%s",err.buf);+strbuf_release(&sb);+strbuf_release(&err);+return-1;+}strbuf_release(&sb);-returnret;+ref_transaction_free(transaction);+return0;}staticintdo_recursive_merge(structcommit*base,structcommit*next,
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:43
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.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++++++---------------
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote:
Making errno when returning from lock_file() meaningful, which should
fix
* an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries
* an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point
[...]
Typo in subject line:
s/failurei/failure/
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _being with
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
I'm in my next attempt to get through your patch series. Sorry for the
long hiatus.
Patches 1-19 look OK aside from a minor typo that I just reported.
See below for a comment on this patch.
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted hunk
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. 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. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 4 +++-
refs.c | 18 +++++++++++------
refs.h | 55 +++++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 63 insertions(+), 14 deletions(-)
I don't have a problem with the API, but I think the idiom suggested in
the comment above is a bit silly. Surely one would do the following
instead:
if (ref_transaction_update(..., &err)) {
ret = error("Error while doing foo-bar: %s", err.buf);
goto cleanup;
}
I think it would also be helpful to document whether the error string
that is appended to the strbuf is terminated with a LF.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
Track the status of a transaction in a new status field. Check the field for
The status field is not set or used anywhere. The field that you use is
"state".
quoted hunk
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 | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
@@ -3437,6 +3458,9 @@ int ref_transaction_update(struct ref_transaction *transaction, { struct ref_update *update;+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: update called for transaction that is not open");+ if (have_old && !old_sha1) die("BUG: have_old is true but old_sha1 is NULL");
@@ -3457,6 +3481,9 @@ int ref_transaction_create(struct ref_transaction *transaction, { struct ref_update *update;+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: create called for transaction that is not open");+ if (!new_sha1 || is_null_sha1(new_sha1)) die("BUG: create ref with null new_sha1");
@@ -3477,6 +3504,9 @@ int ref_transaction_delete(struct ref_transaction *transaction, { struct ref_update *update;+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: delete called for transaction that is not open");+ if (have_old && !old_sha1) die("BUG: have_old is true but old_sha1 is NULL");
@@ -3532,8 +3562,13 @@ int ref_transaction_commit(struct ref_transaction *transaction, int n = transaction->nr; struct ref_update **updates = transaction->updates;- if (!n)+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: commit called for transaction that is not open");++ if (!n) {+ transaction->state = REF_TRANSACTION_CLOSED; return 0;+ } /* Allocate work space */ delnames = xmalloc(sizeof(*delnames) * n);
@@ -3595,6 +3630,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, clear_loose_ref_cache(&ref_cache); cleanup:+ transaction->state = ret ? REF_TRANSACTION_ERROR+ : REF_TRANSACTION_CLOSED;+ for (i = 0; i < n; i++) if (updates[i]->lock) unlock_ref(updates[i]->lock);
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted hunk
Change to use ref transactions for all updates to refs.
Reviewed-by: Jonathan Nieder <redacted>
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"));
I think you've changed the semantics when unborn is set. Please note
that lock_any_ref_for_update() behaves differently if old_sha1 is NULL
(when no check is done) vs. when it is null_sha1 (when it verifies that
the reference didn't previously exist). So when unborn is true, the old
code verifies that the reference previously didn't exist...
strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", to, from,
+ 0, !unborn, &err) ||
...whereas when unborn is true, the new code does no check at all. I
think you want
ref_transaction_update(transaction, "HEAD",
to, unborn ? null_sha1 : from,
0, 1, &err) ||
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted hunk
Change tag.c to use ref transactions for all ref updates.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/tag.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)constchar*msgfile=NULL,*keyid=NULL;structmsg_argmsg={0,STRBUF_INIT};structcommit_list*with_commit=NULL;+structref_transaction*transaction;+structstrbuferr=STRBUF_INIT;structoptionoptions[]={OPT_CMDMODE('l',"list",&cmdmode,N_("list tag names"),'l'),{OPTION_INTEGER,'n',NULL,&lines,N_("n"),
@@ -701,11 +702,13 @@ 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(&err);+if(!transaction||+ref_transaction_update(transaction,ref.buf,object,prev,+0,!is_null_sha1(prev),&err)||
Similar to the error in sequencer.c a few patches later (explained in
more detail in my comment on that patch), here you only do a check if
!is_null_sha1(prev), whereas the old code always did the check. I think
you want
ref_transaction_update(transaction, ref.buf, object, prev,
0, 1, &err) ||
Please check whether you have made the same mistake in other patches.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted hunk
Change the update_ref helper function to use a ref transaction internally.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
@@ -3524,11 +3524,31 @@ int update_ref(const char *action, const char *refname,constunsignedchar*sha1,constunsignedchar*oldval,intflags,enumaction_on_erronerr){-structref_lock*lock;-lock=update_ref_lock(refname,oldval,flags,NULL,onerr);-if(!lock)+structref_transaction*t;+structstrbuferr=STRBUF_INIT;++t=ref_transaction_begin(&err);+if(!t||+ref_transaction_update(t,refname,sha1,oldval,flags,+!!oldval,&err)||+ref_transaction_commit(t,action,&err)){+constchar*str="update_ref failed for ref '%s': %s";++ref_transaction_free(t);+switch(onerr){+caseUPDATE_REFS_MSG_ON_ERR:+error(str,refname,err.buf);+break;+caseUPDATE_REFS_DIE_ON_ERR:+die(str,refname,err.buf);+break;+caseUPDATE_REFS_QUIET_ON_ERR:+break;+}+strbuf_release(&err);return1;-returnupdate_ref_write(action,refname,sha1,lock,NULL,onerr);+}+return0;}
Should this function be scheduled for the "take strbuf *err argument"
treatment instead of continuing to use an action_on_err parameter?
(Maybe you've changed this later in the patch series?)
I'm not saying this change has to be part of the current patch series,
but let's consider it for the future.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
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.
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
s/collissions/collisions/
protect against and cause the fetch to fail for to be even more rare.
Grammatico: s/to be/are/ ?
quoted hunk
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 24 deletions(-)
Is there some reason why the transaction cannot be built up during a
single iteration over targets, thereby also avoiding the need for the
sha1[20*i] stuff? This seems like exactly the kind of situation where
transactions should *save* code. But perhaps I've overlooked a
dependency between the two loops.
quoted hunk
if (!walker->get_recover)
for_each_ref(mark_complete, NULL);
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]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(&sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +291,33 @@ 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)- goto unlock_and_fail;+ strbuf_reset(&ref_name);+ strbuf_addf(&ref_name, "refs/%s", write_ref[i]);+ if (ref_transaction_update(transaction, ref_name.buf,+ &sha1[20 * i], NULL, 0, 0,+ &err)) {+ error("%s", err.buf);+ goto rollback_and_fail;+ }+ }+ if (write_ref) {+ if (ref_transaction_commit(transaction,+ msg ? msg : "fetch (unknown)",+ &err)) {+ error("%s", err.buf);+ goto rollback_and_fail;+ }+ ref_transaction_free(transaction); }- free(msg);+ free(msg); return 0;-unlock_and_fail:- for (i = 0; i < targets; i++)- if (lock[i])- unlock_ref(lock[i]);+rollback_and_fail:+ ref_transaction_free(transaction);+ free(msg);+ strbuf_release(&err);+ strbuf_release(&ref_name); return -1; }
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
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.
[...]
I'm especially glad to see this ugly function disappear!
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted hunk
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.
Change delete_ref to return 0 on success and 1 on failure instead of the
previous 0 on success either 1 or -1 on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
@@ -2544,11 +2544,6 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)returnret;}-staticintrepack_without_ref(constchar*refname)-{-returnrepack_without_refs(&refname,1,NULL);-}-staticintdelete_ref_loose(structref_lock*lock,intflag){if(!(flag&REF_ISPACKED)||flag&REF_ISSYMREF){
@@ -2566,24 +2561,21 @@ 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;+structstrbuferr=STRBUF_INIT;-lock=lock_ref_sha1_basic(refname,sha1,delopt,&flag);
The old code checked that the old value of refname was sha1, regardless
of whether sha1 was null_sha1. Presumably callers never set sha1 to
null_sha1...
...But the new code explicitly skips the check if sha1 is null_sha1.
This shouldn't make a practical difference, because presumably callers
never set sha1 to null_sha1. But given that the new policy elsewhere
for "delete" updates is that it is an error for old_sha1 to equal
null_sha1, it seems to me that this extra check shouldn't be here. So I
think this should be changed to
ref_transaction_delete(transaction, refname, sha1, delopt,
!!sha1, &err) ||
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Simplify warn_if_unremovable.
The change to warn_if_unremovable() is orthogonal to the rest of the
commit and should be a separate commit.
@@ -2544,16 +2544,38 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)returnret;}-staticintdelete_ref_loose(structref_lock*lock,intflag)+staticintadd_err_if_unremovable(constchar*op,constchar*file,+structstrbuf*e,intrc)
This function is only used once. Given also that its purpose is not
that obvious from its signature, it seems to me that the code would be
easier to read if it were inlined.
The name of this function is misleading; it sounds like it will try to
unlink the file and if not possible call error(). Maybe a name like
"unlink_or_report" would be less prejudicial.
It might also make sense to move this function to wrapper.c and
implement unlink_or_warn() in terms of it rather than vice versa.
quoted hunk
+{
+ if (err)
+ return add_err_if_unremovable("unlink", file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int res, i = strlen(lock->lk->filename) - 5; /* .lock */
lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
+ res = unlink_or_err(lock->lk->filename, err);
lock->lk->filename[i] = '.';
- if (err && errno != ENOENT)
+ if (res)
return 1;
}
return 0;
@@ -3603,7 +3625,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct ref_update *update = updates[i]; if (update->lock) {- ret |= delete_ref_loose(update->lock, update->type);+ ret |= delete_ref_loose(update->lock, update->type,+ err); if (!(update->flags & REF_ISPRUNING)) delnames[delnum++] = update->lock->ref_name; }
@@ -429,14 +429,12 @@ int xmkstemp_mode(char *template, int mode)staticintwarn_if_unremovable(constchar*op,constchar*file,intrc){-if(rc<0){-interr=errno;-if(ENOENT!=err){-warning("unable to %s %s: %s",-op,file,strerror(errno));-errno=err;-}-}+interr;+if(rc>=0||errno==ENOENT)+returnrc;+err=errno;+warning("unable to %s %s: %s",op,file,strerror(errno));+errno=err;returnrc;}
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted hunk
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.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
[...]
Would you please document the msg parameter in the block comment that
precedes these three declarations? Especially important is the fact
that the functions make internal copies of msg, so the caller retains
ownership of its copy. You might also mention what happens if msg is
NULL (which, as far as I can see, is that a reflog entry is created
anyway (except in the case of a delete) but that the entry doesn't
contain an explanation).
quoted hunk
@@ -297,7 +297,7 @@ int ref_transaction_update(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1, const unsigned char *old_sha1,- int flags, int have_old,+ int flags, int have_old, const char *msg, struct strbuf *err); /*
@@ -312,7 +312,7 @@ int ref_transaction_update(struct ref_transaction *transaction, int ref_transaction_create(struct ref_transaction *transaction, const char *refname, const unsigned char *new_sha1,- int flags,+ int flags, const char *msg, struct strbuf *err);
It is noteworthy that ref_transaction_delete() accepts a msg parameter,
even though we currently delete a reference's entire reflog when the
reference is deleted. I prefer to think of this as a shortcoming of the
current reference backend, from which future backends hopefully will not
suffer. So I like this design choice.
However, I think it is worth noting this dichotomy in the commit message
and perhaps also in the function documentation.
quoted hunk
/*
@@ -326,7 +326,7 @@ int ref_transaction_create(struct ref_transaction *transaction, int ref_transaction_delete(struct ref_transaction *transaction, const char *refname, const unsigned char *old_sha1,- int flags, int have_old,+ int flags, int have_old, const char *msg, struct strbuf *err); /*
@@ -335,7 +335,7 @@ int ref_transaction_delete(struct ref_transaction *transaction, * problem. */ int ref_transaction_commit(struct ref_transaction *transaction,- const char *msg, struct strbuf *err);+ struct strbuf *err); /* * Free an existing transaction and all associated data.
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
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.
If lock_ref_sha1_basic fails the check_refname_format test, set errno to
EINVAL before returning NULL. This to guarantee that we will not return an
error without updating errno.
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.
This changes semantics for lock_ref_sha1_basic slightly. With this change
it is no longer possible to open a ref that has a badly name which breaks
s/badly name/bad name,/
any codepaths that tries to open and repair badly named refs. The normal refs
s/tries/try/
API should not allow neither creating nor accessing refs with invalid names.
s/not allow neither/allow neither/
If we need such recovery code we could add it as an option to git fsck and have
git fsck be the only sanctioned way of bypassing the normal API and checks.
I like the sentiment, but in the real world I'm not sure we can take
such a step based only on good intentions. Which callers would be
affected? Where is this "git fsck" code that would be needed to help
people rescue their repos?
I can also imagine that we will tighten up the check_refname_format
checks in the future; for example, I think it would be a good idea to
prohibit reference names that start with '-' because it is almost
impossible to work with them (their names look like command-line
options). If we ever make a change like that, we will need some amount
of tolerance in git versions around the transition.
So...I like the idea of enforcing refname checks at the lowest level
possible, but I think that the change you propose is too abrupt. I
think it needs either more careful analysis showing that it won't hurt
anybody, or some kind of tooling or non-strict mode that people can use
to fix their repositories.
Michael
From: Michael Haggerty <hidden> Date: 2016-06-15 23:01:50
On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote:
This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions
This patch series is based on current master 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 20:
- Whitespace and style changes suggested by Jun.
I spent most of the day on reviewing this patch series, but now I'm out
of time again. Here is a summary from my point of view:
Patches 01-19 -- ACK mhagger
Patches 20-42 -- I sent various comments, small to large, concerning
these patches
Patch 43 -- Needs more justification if it is to be acceptable
Patch 44 -- Depends on 43
Patches 45-48 -- I didn't quite get to these, but...
Perhaps it would be more appropriate for the rules about reference name
conflicts to be enforced by the backend, since it is the limitations of
the current backend that impose the restrictions. Would that make sense?
On the other hand, removing the restrictions isn't simply a matter of
picking a different backend, because all Git repositories have to be
able to interact with each other.
So, I don't yet have a considered opinion on the matter.
I think it would be good to try to merge the first part of this patch
series to lock in some progress while we continue iterating on the
remainder. I'm satisfied that it is all going in the right direction
and I am thankful to Ronnie for pushing it forward. But handling
48-patch series is very daunting and I would welcome a split.
I'm not sure whether patches 01-19 are necessarily the right split
between merge-now/iterate-more; it is more or less an accident that I
stopped after patch 19 on an earlier review. Maybe Ronnie could propose
a logical subset of the commits as being ready to be merged to next in
the nearish term?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:55
On Tue, Jul 8, 2014 at 9:29 AM, Michael Haggerty [off-list ref] wrote:
On 06/20/2014 04:42 PM, Ronnie Sahlberg wrote:
quoted
This patch series can also be found at
https://github.com/rsahlberg/git/tree/ref-transactions
This patch series is based on current master 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 20:
- Whitespace and style changes suggested by Jun.
I spent most of the day on reviewing this patch series,
Thanks!
but now I'm out
of time again. Here is a summary from my point of view:
Patches 01-19 -- ACK mhagger
Patches 20-42 -- I sent various comments, small to large, concerning
these patches
Patch 43 -- Needs more justification if it is to be acceptable
Patch 44 -- Depends on 43
Patches 45-48 -- I didn't quite get to these, but...
Perhaps it would be more appropriate for the rules about reference name
conflicts to be enforced by the backend, since it is the limitations of
the current backend that impose the restrictions. Would that make sense?
On the other hand, removing the restrictions isn't simply a matter of
picking a different backend, because all Git repositories have to be
able to interact with each other.
So, I don't yet have a considered opinion on the matter.
I think for compatibility I would prefer to keep the same rules for
name conflicts as for the current files implementation.
But we could have a configuration option to disable these checks, with
the caveat that this might mean that some users will
no longer be able to access pull all the branches anymore.
I think it would be good to try to merge the first part of this patch
series to lock in some progress while we continue iterating on the
remainder. I'm satisfied that it is all going in the right direction
and I am thankful to Ronnie for pushing it forward. But handling
48-patch series is very daunting and I would welcome a split.
Will do.
I'm not sure whether patches 01-19 are necessarily the right split
between merge-now/iterate-more; it is more or less an accident that I
stopped after patch 19 on an earlier review. Maybe Ronnie could propose
a logical subset of the commits as being ready to be merged to next in
the nearish term?
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:55
I have changed the comment. Thanks.
On Tue, Jul 8, 2014 at 4:48 AM, Michael Haggerty [off-list ref] wrote:
I'm in my next attempt to get through your patch series. Sorry for the
long hiatus.
Patches 1-19 look OK aside from a minor typo that I just reported.
See below for a comment on this patch.
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. 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. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
builtin/update-ref.c | 4 +++-
refs.c | 18 +++++++++++------
refs.h | 55 +++++++++++++++++++++++++++++++++++++++++++++-------
3 files changed, 63 insertions(+), 14 deletions(-)
I don't have a problem with the API, but I think the idiom suggested in
the comment above is a bit silly. Surely one would do the following
instead:
if (ref_transaction_update(..., &err)) {
ret = error("Error while doing foo-bar: %s", err.buf);
goto cleanup;
}
I think it would also be helpful to document whether the error string
that is appended to the strbuf is terminated with a LF.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:55
Thanks. Fixed.
On Tue, Jul 8, 2014 at 4:53 AM, Michael Haggerty [off-list ref] wrote:
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _being with
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:55
I updated the comments.
Status is used in a later series to track certain errno settings. This
used to be done here but was moved to a later series.
I removed the status field for now and will re add it later when we
start using it.
Thanks!
On Tue, Jul 8, 2014 at 5:00 AM, Michael Haggerty [off-list ref] wrote:
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted
Track the status of a transaction in a new status field. Check the field for
The status field is not set or used anywhere. The field that you use is
"state".
quoted
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 | 40 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 39 insertions(+), 1 deletion(-)
@@ -3437,6 +3458,9 @@ int ref_transaction_update(struct ref_transaction *transaction, { struct ref_update *update;+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: update called for transaction that is not open");+ if (have_old && !old_sha1) die("BUG: have_old is true but old_sha1 is NULL");
@@ -3457,6 +3481,9 @@ int ref_transaction_create(struct ref_transaction *transaction, { struct ref_update *update;+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: create called for transaction that is not open");+ if (!new_sha1 || is_null_sha1(new_sha1)) die("BUG: create ref with null new_sha1");
@@ -3477,6 +3504,9 @@ int ref_transaction_delete(struct ref_transaction *transaction, { struct ref_update *update;+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: delete called for transaction that is not open");+ if (have_old && !old_sha1) die("BUG: have_old is true but old_sha1 is NULL");
@@ -3532,8 +3562,13 @@ int ref_transaction_commit(struct ref_transaction *transaction, int n = transaction->nr; struct ref_update **updates = transaction->updates;- if (!n)+ if (transaction->state != REF_TRANSACTION_OPEN)+ die("BUG: commit called for transaction that is not open");++ if (!n) {+ transaction->state = REF_TRANSACTION_CLOSED; return 0;+ } /* Allocate work space */ delnames = xmalloc(sizeof(*delnames) * n);
@@ -3595,6 +3630,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, clear_loose_ref_cache(&ref_cache); cleanup:+ transaction->state = 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:01:55
On Tue, Jul 8, 2014 at 6:33 AM, Michael Haggerty [off-list ref] wrote:
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted
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.
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
s/collissions/collisions/
quoted
protect against and cause the fetch to fail for to be even more rare.
Grammatico: s/to be/are/ ?
Thanks. Fixed.
quoted
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
walker.c | 59 +++++++++++++++++++++++++++++++++++------------------------
1 file changed, 35 insertions(+), 24 deletions(-)
Is there some reason why the transaction cannot be built up during a
single iteration over targets, thereby also avoiding the need for the
sha1[20*i] stuff? This seems like exactly the kind of situation where
transactions should *save* code. But perhaps I've overlooked a
dependency between the two loops.
I did it this way to keep the changes minimal. But you are right that
with this we can do a larger refactoring and start saving some code.
I can add changes to a later series to do that change but I want to
keep this change as small as possible for now.
regards
ronnie sahlberg
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:55
On Tue, Jul 8, 2014 at 5:54 AM, Michael Haggerty [off-list ref] wrote:
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted
Change the update_ref helper function to use a ref transaction internally.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 28 ++++++++++++++++++++++++----
1 file changed, 24 insertions(+), 4 deletions(-)
@@ -3524,11 +3524,31 @@ int update_ref(const char *action, const char *refname,constunsignedchar*sha1,constunsignedchar*oldval,intflags,enumaction_on_erronerr){-structref_lock*lock;-lock=update_ref_lock(refname,oldval,flags,NULL,onerr);-if(!lock)+structref_transaction*t;+structstrbuferr=STRBUF_INIT;++t=ref_transaction_begin(&err);+if(!t||+ref_transaction_update(t,refname,sha1,oldval,flags,+!!oldval,&err)||+ref_transaction_commit(t,action,&err)){+constchar*str="update_ref failed for ref '%s': %s";++ref_transaction_free(t);+switch(onerr){+caseUPDATE_REFS_MSG_ON_ERR:+error(str,refname,err.buf);+break;+caseUPDATE_REFS_DIE_ON_ERR:+die(str,refname,err.buf);+break;+caseUPDATE_REFS_QUIET_ON_ERR:+break;+}+strbuf_release(&err);return1;-returnupdate_ref_write(action,refname,sha1,lock,NULL,onerr);+}+return0;}
Should this function be scheduled for the "take strbuf *err argument"
treatment instead of continuing to use an action_on_err parameter?
(Maybe you've changed this later in the patch series?)
I'm not saying this change has to be part of the current patch series,
but let's consider it for the future.
There is a patch that does that in a later series. At that stage we
get rid of all action_on_err arguments.
From: Ronnie Sahlberg <hidden> Date: 2016-06-15 23:01:55
On Tue, Jul 8, 2014 at 6:52 AM, Michael Haggerty [off-list ref] wrote:
On 06/20/2014 04:43 PM, Ronnie Sahlberg wrote:
quoted
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.
Change delete_ref to return 0 on success and 1 on failure instead of the
previous 0 on success either 1 or -1 on failure.
Reviewed-by: Jonathan Nieder <redacted>
Signed-off-by: Ronnie Sahlberg <redacted>
---
refs.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
@@ -2544,11 +2544,6 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)returnret;}-staticintrepack_without_ref(constchar*refname)-{-returnrepack_without_refs(&refname,1,NULL);-}-staticintdelete_ref_loose(structref_lock*lock,intflag){if(!(flag&REF_ISPACKED)||flag&REF_ISSYMREF){
@@ -2566,24 +2561,21 @@ 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;+structstrbuferr=STRBUF_INIT;-lock=lock_ref_sha1_basic(refname,sha1,delopt,&flag);
The old code checked that the old value of refname was sha1, regardless
of whether sha1 was null_sha1. Presumably callers never set sha1 to
null_sha1...
...But the new code explicitly skips the check if sha1 is null_sha1.
This shouldn't make a practical difference, because presumably callers
never set sha1 to null_sha1.
There are actually a few cases where callers do call delete_ref() with
sha1 == null_sha1.
For example fast-import.c:update_branch() will do this is the ref can
not be resolved.
It can also happen in builtin/update-ref.c where we are passing user
supplied data into the call to delete_ref.
So I think the current behaviour should be ok.
There are a few options we could do:
We could change the semantics for ref_transaction_update|delete and
start allowing
have_old==1
old_sha1==null_sha1
and have this behave the same way as
have_old==0
but I think that would be horrible I think.
We could also change all callers to delete_ref() to be careful to only
specify a sha1 IFF it is not null_sha1
but that would just mean we require all callers to do this type of check.
But that would also be fragile since if/when we get new callers to
delete_ref we risk breaking delete_ref if we are not careful.
I think the least bad option is to just have this check in
delete_ref() as now and have the semantics for delete_ref be that if
sha1 is either NULL or null_sha1 then it means we don't care what the
old value is.
regards
ronnie sahlberg