Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller

On 12/31/2016 07:32 AM, Jeff King wrote: > On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote: > >> 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

Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create

On 12/31/2016 07:26 AM, Jeff King wrote: > On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote: >> [...] >> -adjust_shared_perm(logfile->buf); >> -close(logfd); >> +if (logfd >= 0) { >> +adjust_shared_perm(logfile->buf); >> +close(logfd); >> +}

Re: [PATCH v3 05/23] raceproof_create_file(): new function

On 12/31/2016 07:11 AM, Jeff King wrote: > On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote: >> [...] >> +/* >> + * Callback function for raceproof_create_file(). This function is >> + * expected to do something that makes dirname(path) permanent despite >> + * the fact that other

Re: [PATCH v3 00/23] Delete directories left empty after ref deletion

On Sat, Dec 31, 2016 at 04:12:40AM +0100, Michael Haggerty wrote: > This is a re-roll of an old patch series. v1 [1] got some feedback, > which I think was all addressed in v2 [2]. But it seems that v2 fell > on the floor, and I didn't bother following up because it was in the > same area of code

Re: [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents

On Sat, Dec 31, 2016 at 04:13:00AM +0100, Michael Haggerty wrote: > It's bad manners and surprising and therefore error-prone. Agreed. I wondered while reading your earlier patch whether the safe_create_leading_directories() function had the same problem, but it carefully replaces the NUL it

Re: [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument

On Sat, Dec 31, 2016 at 04:12:54AM +0100, Michael Haggerty wrote: > 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

Re: [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller

On Sat, Dec 31, 2016 at 04:12:53AM +0100, Michael Haggerty wrote: > 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

Re: [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create

On Sat, Dec 31, 2016 at 04:12:51AM +0100, Michael Haggerty wrote: > + } else { > + logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666); > if (logfd < 0) { > - strbuf_addf(err, "unable to append to '%s': %s", > -

Re: [PATCH v3 05/23] raceproof_create_file(): new function

On Sat, Dec 31, 2016 at 04:12:45AM +0100, Michael Haggerty wrote: > 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

[PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument

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 ---

[PATCH v3 08/23] rename_tmp_log(): use raceproof_create_file()

Besides shortening the code, this saves an unnecessary call to safe_create_leading_directories_const() in almost all cases. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 73 +--- 1 file changed, 30 insertions(+),

[PATCH v3 09/23] rename_tmp_log(): improve error reporting

* Don't capitalize error strings * Report true paths of affected files Signed-off-by: Michael Haggerty --- refs/files-backend.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 3f18a01..49a119c

[PATCH v3 04/23] safe_create_leading_directories(): set errno on SCLD_EXISTS

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. Signed-off-by: Michael Haggerty ---

[PATCH v3 10/23] log_ref_write(): inline function

This function doesn't do anything beyond call files_log_ref_write(), so replace it with the latter at its call sites. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 24 ++-- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git

[PATCH v3 01/23] files_rename_ref(): tidy up whitespace

Signed-off-by: Michael Haggerty --- refs/files-backend.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index f902393..be4ffdc 100644 --- a/refs/files-backend.c +++ b/refs/files-backend.c @@ -2631,7 +2631,7 @@

[PATCH v3 05/23] raceproof_create_file(): new function

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

[PATCH v3 00/23] Delete directories left empty after ref deletion

This is a re-roll of an old patch series. v1 [1] got some feedback, which I think was all addressed in v2 [2]. But it seems that v2 fell on the floor, and I didn't bother following up because it was in the same area of code that was undergoing heavy changes due to the pluggable reference backend

[PATCH v3 02/23] t5505: use "for-each-ref" to test for the non-existence of references

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 --- t/t5505-remote.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff

[PATCH v3 03/23] safe_create_leading_directories_const(): preserve errno

Some implementations of free() change errno (even thought they shouldn't): https://sourceware.org/bugzilla/show_bug.cgi?id=17924 So preserve the errno from safe_create_leading_directories() across the call to free(). Signed-off-by: Michael Haggerty --- sha1_file.c | 4

[PATCH v3 06/23] lock_ref_sha1_basic(): inline constant

`lflags` is set a single time then never changed, so just inline it. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index be4ffdc..7d4658a 100644 ---

[PATCH v3 16/23] log_ref_write_1(): inline function

Now files_log_ref_write() doesn't do anything beyond call log_ref_write_1(), so inline the latter into the former. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 13 +++-- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git

[PATCH v3 18/23] delete_ref_loose(): inline function

It was hardly doing anything anymore, and had only one caller. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 25 +++-- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index

[PATCH v3 07/23] lock_ref_sha1_basic(): use raceproof_create_file()

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 --- refs/files-backend.c | 35 +-- 1 file changed, 9 insertions(+),

[PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create

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

[PATCH v3 19/23] try_remove_empty_parents(): rename parameter "name" -> "refname"

This is the standard nomenclature. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index e81c8a9..0275c8d 100644 --- a/refs/files-backend.c +++

[PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller

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.

[PATCH v3 12/23] log_ref_setup(): improve robustness against races

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

[PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents

It's bad manners and surprising and therefore error-prone. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/refs/files-backend.c b/refs/files-backend.c index 0275c8d..af5a0e2

[PATCH v3 17/23] delete_ref_loose(): derive loose reference path from lock

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 --- refs/files-backend.c | 5 + 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/refs/files-backend.c

[PATCH v3 15/23] log_ref_setup(): manage the name of the reflog file internally

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

[PATCH v3 23/23] files_transaction_commit(): clean up empty directories

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

[PATCH v3 22/23] try_remove_empty_parents(): teach to remove parents of reflogs, too

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 --- refs/files-backend.c | 24

[PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes

"refname" has already been checked by check_refname_format(), so it cannot have consecutive slashes. Signed-off-by: Michael Haggerty --- refs/files-backend.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/refs/files-backend.c

Wanted: shallow submodule clones with --no-single-branch.

Hi, When adding submodules with --depth=1 only the master branch is cloned. This often leaves the submodule pointing to a non-existing commit. It would be useful if I could pass the --no-single-branch argument to the submodule clone process, since then a submodule can point to any tag or branch