From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
This re-roll addresses a few minor points that were brought up about v1 [1]:
* "safe_create_leading_directories(): set errno on SCLD_EXISTS":
* Set errno to ENOTDIR rather than EEXIST.
* "raceproof_create_file(): new function":
* Improve comments.
* "rename_tmp_log(): use raceproof_create_file()":
* Fix whitespace.
* "rename_tmp_log(): improve error reporting":
* Fix whitespace.
This patch series is also available from my GitHub account [2] as
branch delete-empty-refs-dirs.
Thanks to Junio and Peff for their feedback about v1.
Michael
[1] http://thread.gmane.org/gmane.comp.version-control.git/286370
[2] http://github.com/mhagger/git
Michael Haggerty (20):
safe_create_leading_directories_const(): preserve errno
safe_create_leading_directories(): set errno on SCLD_EXISTS
raceproof_create_file(): new function
lock_ref_sha1_basic(): use raceproof_create_file()
rename_tmp_log(): use raceproof_create_file()
rename_tmp_log(): improve error reporting
log_ref_setup(): separate code for create vs non-create
log_ref_setup(): improve robustness against races
log_ref_setup(): pass the open file descriptor back to the caller
log_ref_write_1(): don't depend on logfile
log_ref_setup(): manage the name of the reflog file internally
log_ref_write_1(): inline function
try_remove_empty_parents(): rename parameter "name" -> "refname"
try_remove_empty_parents(): don't trash argument contents
try_remove_empty_parents(): don't accommodate consecutive slashes
t5505: use "for-each-ref" to test for the non-existence of references
delete_ref_loose(): derive loose reference path from lock
delete_ref_loose(): inline function
try_remove_empty_parents(): teach to remove parents of reflogs, too
ref_transaction_commit(): clean up empty directories
cache.h | 52 ++++++-
refs/files-backend.c | 370 ++++++++++++++++++++++++++------------------------
refs/refs-internal.h | 9 +-
sha1_file.c | 77 ++++++++++-
t/t1400-update-ref.sh | 27 ++++
t/t5505-remote.sh | 2 +-
6 files changed, 351 insertions(+), 186 deletions(-)
--
2.7.0
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Instead of coding the retry loop inline, use raceproof_create_file() to
make lock acquisition safe against directory creation/deletion races.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 47 +++++++++++++++++++----------------------------
1 file changed, 19 insertions(+), 28 deletions(-)
@@ -1980,35 +1992,14 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,lock->orig_ref_name=xstrdup(orig_refname);strbuf_git_path(&ref_file,"%s",refname);-retry:-switch(safe_create_leading_directories_const(ref_file.buf)){-caseSCLD_OK:-break;/* success */-caseSCLD_VANISHED:-if(--attempts_remaining>0)-gotoretry;-/* fall through */-default:+create_reflock_data.lk=lock->lk;++if(raceproof_create_file(ref_file.buf,create_reflock,&create_reflock_data)){last_errno=errno;-strbuf_addf(err,"unable to create directory for %s",-ref_file.buf);+unable_to_lock_message(ref_file.buf,errno,err);gotoerror_return;}-if(hold_lock_file_for_update(lock->lk,ref_file.buf,lflags)<0){-last_errno=errno;-if(errno==ENOENT&&--attempts_remaining>0)-/*-*Maybesomebodyjustdeletedoneofthe-*directoriesleadingtoref_file.Try-*again:-*/-gotoretry;-else{-unable_to_lock_message(ref_file.buf,errno,err);-gotoerror_return;-}-}if(verify_lock(lock,old_sha1,mustexist,err)){last_errno=errno;gotoerror_return;
@@ -2430,10 +2430,11 @@ static int rename_tmp_log(const char *newrefname)ret=raceproof_create_file(path,rename_tmp_log_callback,&true_errno);if(ret){if(true_errno==EISDIR)-error("Directory not empty: %s",path);+error("directory not empty: %s",path);else-error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",-newrefname,strerror(true_errno));+error("unable to move logfile %s to %s: %s",+git_path(TMP_RENAMED_LOG),path,+strerror(true_errno));}free(path);
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Besides shortening the code, this saves an unnecessary call to
safe_create_leading_directories_const() in almost all cases.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 76 ++++++++++++++++++++++------------------------------
1 file changed, 32 insertions(+), 44 deletions(-)
@@ -2400,55 +2400,43 @@ out:*/#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"+staticintrename_tmp_log_callback(constchar*path,void*cb)+{+int*true_errno=cb;++if(rename(git_path(TMP_RENAMED_LOG),path)){+/*+*rename(a,b)whenbisanexistingdirectoryought+*toresultinISDIR,butSolaris5.8givesENOTDIR.+*Sheesh.Recordthetrueerrnoforerrorreporting,+*butreportEISDIRtoraceproof_create_file()so+*thatitknowstoretry.+*/+*true_errno=errno;+if(errno==ENOTDIR)+errno=EISDIR;+return-1;+}else{+return0;+}+}+staticintrename_tmp_log(constchar*newrefname){-intattempts_remaining=4;-structstrbufpath=STRBUF_INIT;-intret=-1;+char*path=git_pathdup("logs/%s",newrefname);+inttrue_errno;+intret;-retry:-strbuf_reset(&path);-strbuf_git_path(&path,"logs/%s",newrefname);-switch(safe_create_leading_directories_const(path.buf)){-caseSCLD_OK:-break;/* success */-caseSCLD_VANISHED:-if(--attempts_remaining>0)-gotoretry;-/* fall through */-default:-error("unable to create directory for %s",newrefname);-gotoout;-}--if(rename(git_path(TMP_RENAMED_LOG),path.buf)){-if((errno==EISDIR||errno==ENOTDIR)&&--attempts_remaining>0){-/*-*rename(a,b)whenbisanexisting-*directoryoughttoresultinISDIR,but-*Solaris5.8givesENOTDIR.Sheesh.-*/-if(remove_empty_directories(&path)){-error("Directory not empty: logs/%s",newrefname);-gotoout;-}-gotoretry;-}elseif(errno==ENOENT&&--attempts_remaining>0){-/*-*Maybeanotherprocessjustdeletedoneof-*thedirectoriesinthepathtonewrefname.-*Tryagainfromthebeginning.-*/-gotoretry;-}else{+ret=raceproof_create_file(path,rename_tmp_log_callback,&true_errno);+if(ret){+if(true_errno==EISDIR)+error("Directory not empty: %s",path);+elseerror("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",-newrefname,strerror(errno));-gotoout;-}+newrefname,strerror(true_errno));}-ret=0;-out:-strbuf_release(&path);++free(path);returnret;}
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Instead of writing the name of the reflog file into a strbuf that is
supplied by the caller but not needed there, write it into a local
temporary buffer and remove the strbuf parameter entirely.
And while we're adjusting the function signature, reorder the arguments
to move the input parameters before the output parameters.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 66 +++++++++++++++++++++++-----------------------------
1 file changed, 29 insertions(+), 37 deletions(-)
@@ -2591,39 +2591,38 @@ static int open_or_create_logfile(const char *path, void *cb)}/*-*Createareflogforaref.Storeitspathto*logfile.If-*force_create=0,onlycreatethereflogforcertainrefs(those-*forwhichshould_autocreate_reflogreturnsnon-zero).Otherwise,-*createitregardlessofthereferencename.Ifthelogfilealready-*existedorwascreated,return0andset*logfdtothefile-*descriptoropenedforappendingtothefile.Ifnologfileexists-*andwedecidednottocreateone,return0andset*logfdto-1.On-*failure,fillin*errandreturn-1.+*Createareflogforaref.Ifforce_create=0,onlycreatethe+*reflogforcertainrefs(thoseforwhichshould_autocreate_reflog+*returnsnon-zero).Otherwise,createitregardlessofthereference+*name.Ifthelogfilealreadyexistedorwascreated,return0and+*set*logfdtothefiledescriptoropenedforappendingtothefile.+*Ifnologfileexistsandwedecidednottocreateone,return0and+*set*logfdto-1.Onfailure,fillin*errandreturn-1.*/-staticintlog_ref_setup(constchar*refname,-structstrbuf*logfile,int*logfd,-structstrbuf*err,intforce_create)+staticintlog_ref_setup(constchar*refname,intforce_create,+int*logfd,structstrbuf*err){-strbuf_git_path(logfile,"logs/%s",refname);+char*logfile=git_pathdup("logs/%s",refname);+intret=0;if(force_create||should_autocreate_reflog(refname)){-if(raceproof_create_file(logfile->buf,open_or_create_logfile,logfd)<0){+if(raceproof_create_file(logfile,open_or_create_logfile,logfd)<0){if(errno==ENOENT){strbuf_addf(err,"unable to create directory for %s: "-"%s",logfile->buf,strerror(errno));+"%s",logfile,strerror(errno));}elseif(errno==EISDIR){strbuf_addf(err,"there are still logs under %s",-logfile->buf);+logfile);}else{strbuf_addf(err,"unable to append to %s: %s",-logfile->buf,strerror(errno));+logfile,strerror(errno));}-return-1;+ret=-1;}else{-adjust_shared_perm(logfile->buf);+adjust_shared_perm(logfile);}}else{-*logfd=open(logfile->buf,O_APPEND|O_WRONLY,0666);+*logfd=open(logfile,O_APPEND|O_WRONLY,0666);if(*logfd<0){if(errno==ENOENT||errno==EISDIR){/*
@@ -2634,27 +2633,25 @@ static int log_ref_setup(const char *refname,*/}else{strbuf_addf(err,"unable to append to %s: %s",-logfile->buf,strerror(errno));-return-1;+logfile,strerror(errno));+ret=-1;}}else{-adjust_shared_perm(logfile->buf);+adjust_shared_perm(logfile);}}-return0;+free(logfile);+returnret;}intsafe_create_reflog(constchar*refname,intforce_create,structstrbuf*err){-intret;-structstrbufsb=STRBUF_INIT;-intfd;+intret,fd;-ret=log_ref_setup(refname,&sb,&fd,err,force_create);+ret=log_ref_setup(refname,force_create,&fd,err);if(!ret&&fd>=0)close(fd);-strbuf_release(&sb);returnret;}
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Now log_ref_write_1() doesn't do anything, so inline it.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 29 +++++++++++------------------
1 file changed, 11 insertions(+), 18 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
This is the standard nomenclature.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Instead of looking on the filesystem inside ".git/refs/remotes/origin",
use "git for-each-ref" to check for leftover references under the
remote's old name.
Signed-off-by: Michael Haggerty <redacted>
---
t/t5505-remote.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
It is simpler to derive the path to the file that must be deleted from
"lock->ref_name" than from the lock_file object.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
When deleting/pruning references, remove any directories that are made
empty by the deletion of loose references or of reflogs. Otherwise such
empty directories can survive forever and accumulate over time. (Even
'pack-refs', which is smart enough to remove the parent directories of
loose references that it prunes, leaves directories that were already
empty.)
And now that ref_transaction_commit() takes care of deleting the parent
directories of loose references that it prunes, we don't have to do that
in prune_ref() anymore.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 37 +++++++++++++++++++++++++++++++------
refs/refs-internal.h | 9 ++++++++-
t/t1400-update-ref.sh | 27 +++++++++++++++++++++++++++
3 files changed, 66 insertions(+), 7 deletions(-)
@@ -3261,6 +3260,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}+update->flags|=REF_DELETED_LOOSE;}if(!(update->flags&REF_ISPRUNING))
@@ -3273,16 +3273,41 @@ int ref_transaction_commit(struct ref_transaction *transaction,ret=TRANSACTION_GENERIC_ERROR;gotocleanup;}-for_each_string_list_item(ref_to_delete,&refs_to_delete)-unlink_or_warn(git_path("logs/%s",ref_to_delete->string));++/* Delete the reflogs of any references that were deleted: */+for_each_string_list_item(ref_to_delete,&refs_to_delete){+if(!unlink_or_warn(git_path("logs/%s",ref_to_delete->string)))+try_remove_empty_parents(ref_to_delete->string,+REMOVE_EMPTY_PARENTS_REFLOG);+}+clear_loose_ref_cache(&ref_cache);cleanup:transaction->state=REF_TRANSACTION_CLOSED;-for(i=0;i<n;i++)-if(updates[i]->lock)-unlock_ref(updates[i]->lock);+for(i=0;i<n;i++){+structref_update*update=updates[i];++if(!update->lock)+continue;++if(update->flags&REF_DELETED_LOOSE){+/*+*Theloosereferencewasdeleted.Wewantto+*deleteanyemptyparentdirectories,but+*thatcanonlyworkafterwehaveremoved+*thelockfile:+*/+char*path=xstrdup(update->lock->ref_name);+unlock_ref(update->lock);+try_remove_empty_parents(path,REMOVE_EMPTY_PARENTS_REF);+free(path);+}else{+unlock_ref(update->lock);+}+}+string_list_clear(&refs_to_delete,0);string_list_clear(&affected_refnames,0);returnret;
@@ -3257,9 +3242,13 @@ int ref_transaction_commit(struct ref_transaction *transaction,structref_update*update=updates[i];if(update->flags&REF_DELETING){-if(delete_ref_loose(update->lock,update->type,err)){-ret=TRANSACTION_GENERIC_ERROR;-gotocleanup;+if(!(update->type&REF_ISPACKED)||+update->type&REF_ISSYMREF){+/* It is a loose reference. */+if(unlink_or_msg(git_path("%s",update->lock->ref_name),err)){+ret=TRANSACTION_GENERIC_ERROR;+gotocleanup;+}}if(!(update->flags&REF_ISPRUNING))
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Add a new "flags" parameter that tells the function whether to remove
empty parent directories of the loose reference file, of the reflog
file, or both. The new functionality is not yet used.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
"refname" has already been checked by check_refname_format(), so it
cannot have consecutive slashes.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
The exit path for SCLD_EXISTS wasn't setting errno, which some callers
use to generate error messages for the user. Fix the problem and
document that the function sets errno correctly to help avoid similar
regressions in the future.
While we're at it, document the difference between
safe_create_leading_directories() and
safe_create_leading_directories_const().
Signed-off-by: Michael Haggerty <redacted>
---
cache.h | 10 ++++++++--
sha1_file.c | 4 +++-
2 files changed, 11 insertions(+), 3 deletions(-)
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Add a function that tries to create a file and any containing
directories in a way that is robust against races with other processes
that might be cleaning up empty directories at the same time.
The actual file creation is done by a callback function, which, if it
fails, should set errno to EISDIR or ENOENT according to the convention
of open(). raceproof_create_file() detects such failures, and
respectively either tries to delete empty directories that might be in
the way of the file or tries to create the containing directories. Then
it retries the callback function.
This function is not yet used.
Signed-off-by: Michael Haggerty <redacted>
---
cache.h | 42 +++++++++++++++++++++++++++++++++++++
sha1_file.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+)
@@ -177,6 +177,75 @@ enum scld_error safe_create_leading_directories_const(const char *path)returnresult;}+intraceproof_create_file(constchar*path,create_file_fnfn,void*cb)+{+/*+*Thenumberoftimeswewilltrytoremoveemptydirectories+*inthewayofpath.Thisisonly1becauseifanother+*processisracilycreatingdirectoriesthatconflictwith+*us,wedon'twanttofightagainstthem.+*/+intremove_directories_remaining=1;++/*+*Thenumberoftimesthatwewilltrytocreatethe+*directoriescontainingpath.Wearewillingtoattemptthis+*morethanonce,becauseanotherprocesscouldbetryingto+*cleanupemptydirectoriesatthesametimeasweare+*tryingtocreatethem.+*/+intcreate_directories_remaining=3;++/* A scratch copy of path, filled lazily if we need it: */+structstrbufpath_copy=STRBUF_INIT;++intsave_errno;+intret;++retry_fn:+ret=fn(path,cb);+save_errno=errno;+if(!ret)+gotoout;++if(errno==EISDIR&&remove_directories_remaining>0){+/*+*Adirectoryisintheway.Maybeitisempty;try+*toremoveit:+*/+if(!path_copy.len)+strbuf_addstr(&path_copy,path);++if(!remove_dir_recursively(&path_copy,REMOVE_DIR_EMPTY_ONLY)){+remove_directories_remaining--;+gotoretry_fn;+}+}elseif(errno==ENOENT&&create_directories_remaining>0){+/*+*Maybethecontainingdirectorydidn'texist,or+*maybeitwasjustdeletedbyaprocessthatis+*racingwithustocleanupemptydirectories.Try+*tocreateit:+*/+enumscld_errorscld_result;++if(!path_copy.len)+strbuf_addstr(&path_copy,path);++do{+create_directories_remaining--;+scld_result=safe_create_leading_directories(path_copy.buf);+if(scld_result==SCLD_OK)+gotoretry_fn;+}while(scld_result==SCLD_VANISHED&&create_directories_remaining>0);+}++out:+strbuf_release(&path_copy);+errno=save_errno;+returnret;+}+staticvoidfill_sha1_path(char*pathbuf,constunsignedchar*sha1){inti;
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
The behavior of this function (especially how it handles errors) is
quite different depending on whether we are willing to create the reflog
vs. whether we are only trying to open an existing reflog. So separate
the code paths.
This also simplifies the next steps.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 56 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 37 insertions(+), 19 deletions(-)
@@ -2590,7 +2590,7 @@ static int commit_ref(struct ref_lock *lock)*/staticintlog_ref_setup(constchar*refname,structstrbuf*logfile,structstrbuf*err,intforce_create){-intlogfd,oflags=O_APPEND|O_WRONLY;+intlogfd;strbuf_git_path(logfile,"logs/%s",refname);if(force_create||should_autocreate_reflog(refname)){
@@ -2599,36 +2599,54 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str"%s",logfile->buf,strerror(errno));return-1;}-oflags|=O_CREAT;-}--logfd=open(logfile->buf,oflags,0666);-if(logfd<0){-if(!(oflags&O_CREAT)&&(errno==ENOENT||errno==EISDIR))-return0;+logfd=open(logfile->buf,O_APPEND|O_WRONLY|O_CREAT,0666);+if(logfd<0){+if(errno==EISDIR){+/*+*Thedirectorythatisinthewaymightbe+*empty.Trytoremoveit.+*/+if(remove_empty_directories(logfile)){+strbuf_addf(err,"There are still logs under "+"'%s'",logfile->buf);+return-1;+}+logfd=open(logfile->buf,O_APPEND|O_WRONLY|O_CREAT,0666);+}-if(errno==EISDIR){-if(remove_empty_directories(logfile)){-strbuf_addf(err,"There are still logs under "-"'%s'",logfile->buf);+if(logfd<0){+strbuf_addf(err,"unable to append to %s: %s",+logfile->buf,strerror(errno));return-1;}-logfd=open(logfile->buf,oflags,0666);}+adjust_shared_perm(logfile->buf);+close(logfd);+}else{+logfd=open(logfile->buf,O_APPEND|O_WRONLY,0666);if(logfd<0){-strbuf_addf(err,"unable to append to %s: %s",-logfile->buf,strerror(errno));-return-1;+if(errno==ENOENT||errno==EISDIR){+/*+*Thelogfiledoesn'talreadyexist,+*butthatisnotanerror;itonly+*meansthatwewon'twritelog+*entriestoit.+*/+}else{+strbuf_addf(err,"unable to append to %s: %s",+logfile->buf,strerror(errno));+return-1;+}+}else{+adjust_shared_perm(logfile->buf);+close(logfd);}}-adjust_shared_perm(logfile->buf);-close(logfd);return0;}-intsafe_create_reflog(constchar*refname,intforce_create,structstrbuf*err){intret;
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
It's unnecessary to pass a strbuf holding the reflog path up and down
the call stack now that it is hardly needed by the callers. Remove the
places where log_ref_write_1() uses it, in preparation for making it
internal to log_ref_setup().
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -2705,14 +2705,14 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,result=log_ref_write_fd(logfd,old_sha1,new_sha1,git_committer_info(0),msg);if(result){-strbuf_addf(err,"unable to append to %s: %s",logfile->buf,-strerror(errno));+strbuf_addf(err,"unable to append to %s: %s",+git_path("logs/%s",refname),strerror(errno));close(logfd);return-1;}if(close(logfd)){-strbuf_addf(err,"unable to append to %s: %s",logfile->buf,-strerror(errno));+strbuf_addf(err,"unable to append to %s: %s",+git_path("logs/%s",refname),strerror(errno));return-1;}return0;
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Change log_ref_setup() to use raceproof_create_file() to create the new
logfile. This makes it more robust against a race against another
process that might be trying to clean up empty directories while we are
trying to create a new logfile.
This also means that it will only call create_leading_directories() if
open() fails, which should be a net win. Even in the cases where we are
willing to create a new logfile, it will usually be the case that the
logfile already exists, or if not then that the directory containing the
logfile already exists. In such cases, we will save some work that was
previously done unconditionally.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 46 +++++++++++++++++++++-------------------------
1 file changed, 21 insertions(+), 25 deletions(-)
@@ -2582,6 +2582,14 @@ static int commit_ref(struct ref_lock *lock)return0;}+staticintopen_or_create_logfile(constchar*path,void*cb)+{+int*fd=cb;++*fd=open(path,O_APPEND|O_WRONLY|O_CREAT,0666);+return(*fd<0)?-1:0;+}+/**Createareflogforaref.Ifforce_create=0,thereflogwill*onlybecreatedforcertainrefs(thoseforwhich
@@ -2593,36 +2601,24 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strintlogfd;strbuf_git_path(logfile,"logs/%s",refname);+if(force_create||should_autocreate_reflog(refname)){-if(safe_create_leading_directories(logfile->buf)<0){-strbuf_addf(err,"unable to create directory for %s: "-"%s",logfile->buf,strerror(errno));-return-1;-}-logfd=open(logfile->buf,O_APPEND|O_WRONLY|O_CREAT,0666);-if(logfd<0){-if(errno==EISDIR){-/*-*Thedirectorythatisinthewaymightbe-*empty.Trytoremoveit.-*/-if(remove_empty_directories(logfile)){-strbuf_addf(err,"There are still logs under "-"'%s'",logfile->buf);-return-1;-}-logfd=open(logfile->buf,O_APPEND|O_WRONLY|O_CREAT,0666);-}--if(logfd<0){+if(raceproof_create_file(logfile->buf,open_or_create_logfile,&logfd)<0){+if(errno==ENOENT){+strbuf_addf(err,"unable to create directory for %s: "+"%s",logfile->buf,strerror(errno));+}elseif(errno==EISDIR){+strbuf_addf(err,"there are still logs under %s",+logfile->buf);+}else{strbuf_addf(err,"unable to append to %s: %s",logfile->buf,strerror(errno));-return-1;}+return-1;+}else{+adjust_shared_perm(logfile->buf);+close(logfd);}--adjust_shared_perm(logfile->buf);-close(logfd);}else{logfd=open(logfile->buf,O_APPEND|O_WRONLY,0666);if(logfd<0){
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
Theoretically, free() is allowed to change errno. So preserve the errno
from safe_create_leading_directories() across the call to free().
Signed-off-by: Michael Haggerty <redacted>
---
sha1_file.c | 4 ++++
1 file changed, 4 insertions(+)
@@ -164,10 +164,14 @@ enum scld_error safe_create_leading_directories(char *path)enumscld_errorsafe_create_leading_directories_const(constchar*path){+intsave_errno;/* path points to cache entries, so xstrdup before messing with it */char*buf=xstrdup(path);enumscld_errorresult=safe_create_leading_directories(buf);++save_errno=errno;free(buf);+errno=save_errno;returnresult;}
From: Michael Haggerty <hidden> Date: 2016-06-15 23:08:27
This function will most often be called by log_ref_write_1(), which
wants to append to the reflog file. In that case, it is silly to close
the file only for the caller to reopen it immediately. So, in the case
that the file was opened, pass the open file descriptor back to the
caller.
Signed-off-by: Michael Haggerty <redacted>
---
refs/files-backend.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)