* [PATCH] submodule.c: use GIT_DIR_ENVIRONMENT consistently
From: Stefan Beller @ 2016-12-30 0:47 UTC (permalink / raw)
To: gitster, jrnieder; +Cc: git, Stefan Beller
In C code we have the luxury of having constants for all the important
things that are hard coded. This is the only place in C, that hard codes
the git directory environment variable, so fix it.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
Signed-off-by-the-format-patch-config ;)
This is the only occurrence for "GIT_DIR=" in C, but what about ".git"
git grep "\.git\"" *.c finds some places, which we may want to convert
to DEFAULT_GIT_DIR_ENVIRONMENT?
(mainly things that are newer if I can judge the places correctly
lots of submodules, worktrees and the no data in ".git" bug AFAICT)
Thanks,
Stefan
submodule.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/submodule.c b/submodule.c
index ece17315d6..973b9f3f96 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
argv_array_push(out, *var);
}
- argv_array_push(out, "GIT_DIR=.git");
+ argv_array_push(out, "%s=%s", GIT_DIR_ENVIRONMENT,
+ DEFAULT_GIT_DIR_ENVIRONMENT);
}
--
2.11.0.259.ga95e92af08.dirty
^ permalink raw reply related
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2016-12-30 0:48 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: git, Duy Nguyen, Junio C Hamano, Stefan Beller
In-Reply-To: <20161230001114.GB7883@aiede.mtv.corp.google.com>
On Thu, Dec 29, 2016 at 04:11:14PM -0800, Jonathan Nieder wrote:
> Thanks. Here's the patch again, now with commit messages and a test.
> Thanks for the analysis and sorry for the slow turnaround.
Thanks for following up. While working on a similar one recently, I had
the nagging feeling that there was a case we had found that was still to
be dealt with, and I think this was it. :)
The patch to the C code looks good. I have a few comments on the tests:
> diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
> index aeb3a63f7c..a86fca3e6c 100755
> --- a/t/t5550-http-fetch-dumb.sh
> +++ b/t/t5550-http-fetch-dumb.sh
> @@ -34,6 +34,21 @@ test_expect_success 'clone http repository' '
> test_cmp file clone/file
> '
>
> +test_expect_success 'list refs from outside any repository' '
> + cat >expect <<-EOF &&
> + $(git rev-parse master) HEAD
> + $(git rev-parse master) refs/heads/master
> + EOF
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$HTTPD_URL/dumb/repo.git" >../actual
> + ) &&
> + test_cmp expect actual
> +'
Since my recent de95302a4c (t5000: extract nongit function to
test-lib-functions.sh, 2016-12-15), this can be shortened to:
cat >expect <<-EOF &&
...
EOF
nongit git ls-remote "$HTTPD_URL/dumb/repo.git" >actual &&
test_cmp expect actual
I think that commit is in 'master' now.
Without my patch to die() in setup_git_env(), I think this would pass
with or without your patch, right? I think the current behavior _is_
buggy, but a setup to show off the improvement would be so arcane that I
don't think it is worth it. E.g., something like:
echo '[http]extraHeader = "Foo: Bar" >nongit/.git/config
would probably trigger the use of that config when it shouldn't. But
that's really stretching.
> diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
> index 6e5b9e42fb..7ba894f0f4 100755
> --- a/t/t5551-http-fetch-smart.sh
> +++ b/t/t5551-http-fetch-smart.sh
> @@ -163,6 +163,21 @@ test_expect_success 'redirects send auth to new location' '
> expect_askpass both user@host auth/smart/repo.git
> '
>
> +test_expect_success 'list refs from outside any repository' '
> + cat >expect <<-EOF &&
> + $(git rev-parse master) HEAD
> + $(git rev-parse master) refs/heads/master
> + EOF
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> + ) &&
> + test_cmp expect actual
> +'
Is this really testing anything that the dumb one isn't? The interesting
bit is in invoking git-remote-http, not what it does under the hood. I
don't mind too much being thorough, but if we are going to go that route
we should probably come up with a standard battery of tests for dumb
and smart HTTP, and then invoke them for each case without having to
write each one twice.
> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> index 225a022e8a..4573d98e6c 100755
> --- a/t/t5570-git-daemon.sh
> +++ b/t/t5570-git-daemon.sh
> @@ -35,6 +35,21 @@ test_expect_success 'clone git repository' '
> test_cmp file clone/file
> '
>
> +test_expect_success 'list refs from outside any repository' '
> + cat >expect <<-EOF &&
> + $(git rev-parse master) HEAD
> + $(git rev-parse master) refs/heads/master
> + EOF
> + mkdir lsremote-root &&
> + (
> + GIT_CEILING_DIRECTORIES=$(pwd) &&
> + export GIT_CEILING_DIRECTORIES &&
> + cd lsremote-root &&
> + git ls-remote "$GIT_DAEMON_URL/repo.git" >../actual
> + ) &&
> + test_cmp expect actual
> +'
This one isn't actually broken now, right? The test is just there to
catch future regressions?
If we are being thorough, then would we also care about file-local
repos, git-over-ssh, etc?
> diff --git a/t/t5802-connect-helper.sh b/t/t5802-connect-helper.sh
> index c6c2661878..7232032cd2 100755
> --- a/t/t5802-connect-helper.sh
> +++ b/t/t5802-connect-helper.sh
This one is quite a big addition. I know this falls under the "while
we're at it" line at the end of your commit message, but maybe it's
worth pulling the GIT_EXT_SERVICE bits out into their own patch (and
explaining briefly what's going on; I had to go look up what
GIT_EXT_SERVICE_NOPREFIX even was).
-Peff
^ permalink raw reply
* Re: [PATCH v2] remote helpers: avoid blind fall-back to ".git" when setting GIT_DIR
From: Jeff King @ 2016-12-30 0:49 UTC (permalink / raw)
To: Stefan Beller
Cc: Jonathan Nieder, git@vger.kernel.org, Duy Nguyen, Junio C Hamano
In-Reply-To: <CAGZ79kZiR7grGeMrMFRuQQ0kW6eQ4m=ZizGHDbT-s14iAj8hWQ@mail.gmail.com>
On Thu, Dec 29, 2016 at 04:37:30PM -0800, Stefan Beller wrote:
> > + mkdir lsremote-root &&
> > + (
> > + GIT_CEILING_DIRECTORIES=$(pwd) &&
> > + export GIT_CEILING_DIRECTORIES &&
> > + cd lsremote-root &&
> > + git ls-remote "$HTTPD_URL/smart/repo.git" >../actual
> > + ) &&
>
> We could avoid the subshell via
>
> GIT_CEILING_DIRECTORIES=$(pwd) \
> git -C lsremote-root lsremote ... >actual
>
> Not sure if it is worth to trade off a block of code (and an extra shell
> at run time) for an overly long line.
>
> The rest looks good to me.
I mentioned elsewhere that we now have a "nongit" function to do this as
a one-liner. It might be worth applying your optimization to that
function, so it would take effect in may places.
-Peff
^ permalink raw reply
* Re: [PATCH] submodule.c: use GIT_DIR_ENVIRONMENT consistently
From: René Scharfe @ 2016-12-30 1:36 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, jrnieder, git
In-Reply-To: <20161230004739.19144-1-sbeller@google.com>
Am 30.12.2016 um 01:47 schrieb Stefan Beller:
> diff --git a/submodule.c b/submodule.c
> index ece17315d6..973b9f3f96 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1333,5 +1333,6 @@ void prepare_submodule_repo_env(struct argv_array *out)
> if (strcmp(*var, CONFIG_DATA_ENVIRONMENT))
> argv_array_push(out, *var);
> }
> - argv_array_push(out, "GIT_DIR=.git");
> + argv_array_push(out, "%s=%s", GIT_DIR_ENVIRONMENT,
> + DEFAULT_GIT_DIR_ENVIRONMENT);
argv_array_pushf (with added "f") instead?
And indent continued lines to align them with the left parenthesis, like
this:
fn(arg1,
arg2);
> }
>
^ permalink raw reply
* Wanted: shallow submodule clones with --no-single-branch.
From: Tor Andersson @ 2016-12-30 10:50 UTC (permalink / raw)
To: git
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 without ending up in this situation. I've got a local
patch to hardwire the --no-single-branch argument in the
builtin/submodule--helper.c clone_submodule function, but I'm not sure
if this will have any other adverse effects?
Better yet would be for the shallow submodule clone to automatically
retrieve and graft the actual commit the submodule points to, but
that's probably wishing for too much.
-Tor
^ permalink raw reply
* [PATCH v3 01/23] files_rename_ref(): tidy up whitespace
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
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 @@ static int files_rename_ref(struct ref_store *ref_store,
if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
sha1, NULL) &&
delete_ref(newrefname, NULL, REF_NODEREF)) {
- if (errno==EISDIR) {
+ if (errno == EISDIR) {
struct strbuf path = STRBUF_INIT;
int result;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 02/23] t5505: use "for-each-ref" to test for the non-existence of references
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
t/t5505-remote.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh
index 8198d8e..65030fb 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -725,7 +725,7 @@ test_expect_success 'rename a remote' '
(
cd four &&
git remote rename origin upstream &&
- rmdir .git/refs/remotes/origin &&
+ test -z "$(git for-each-ref refs/remotes/origin)" &&
test "$(git symbolic-ref refs/remotes/upstream/HEAD)" = "refs/remotes/upstream/master" &&
test "$(git rev-parse upstream/master)" = "$(git rev-parse master)" &&
test "$(git config remote.upstream.fetch)" = "+refs/heads/*:refs/remotes/upstream/*" &&
--
2.9.3
^ permalink raw reply related
* [PATCH v3 00/23] Delete directories left empty after ref deletion
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
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 work. Sorry for the long delay before
getting back to it.
It turns out that this patch series is still relevant and didn't even
need all that must adjustment. While rebasing onto the current
`master`, I tidied up a bit, tightened up some code, and improved some
commit messages. But the spirit of the patch series and most of its
code are unchanged.
This patch series is also available from my GitHub account [3] as
branch delete-empty-refs-dirs.
Brief summary (see v1 [1] for more details):
Previously, we were pretty sloppy about leaving empty directories
behind (under both $GIT_DIR/refs and $GIT_DIR/logs) when deleting
references. Such directories could accumulate essentially forever.
It's true that `pack-refs` deletes directories that it empties, but if
a directory is *already* empty, then `pack-refs` doesn't remove it. It
is also true that if an empty directory gets in the way of the
creation of a *new* reference, then it is deleted. But otherwise there
is no systematic cleanup of empty directories.
A reason for the old behavior was that the code paths *creating* new
files in these hierarchies were not always robust against a race with
another process that was cleaning up empty directories. So most of
this patch series is dedicated to hardening up the creation code paths
(via a new function, `raceproof_create_file()`). The last several
patches teach `files_transaction_commit()` to delete empty directories
when deleting references and reflogs.
Michael
[1] http://public-inbox.org/git/cover.1455626201.git.mhagger@alum.mit.edu/T/#u
[2] http://public-inbox.org/git/cover.1456405698.git.mhagger@alum.mit.edu/T/#u
[3] http://github.com/mhagger/git
Michael Haggerty (23):
files_rename_ref(): tidy up whitespace
t5505: use "for-each-ref" to test for the non-existence of references
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(): inline constant
lock_ref_sha1_basic(): use raceproof_create_file()
rename_tmp_log(): use raceproof_create_file()
rename_tmp_log(): improve error reporting
log_ref_write(): inline function
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 argument
log_ref_setup(): manage the name of the reflog file internally
log_ref_write_1(): inline function
delete_ref_loose(): derive loose reference path from lock
delete_ref_loose(): 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
try_remove_empty_parents(): teach to remove parents of reflogs, too
files_transaction_commit(): clean up empty directories
cache.h | 48 ++++++-
refs/files-backend.c | 375 +++++++++++++++++++++++++-------------------------
refs/refs-internal.h | 11 +-
sha1_file.c | 76 +++++++++-
t/t1400-update-ref.sh | 27 ++++
t/t5505-remote.sh | 2 +-
6 files changed, 346 insertions(+), 193 deletions(-)
--
2.9.3
^ permalink raw reply
* [PATCH v3 03/23] safe_create_leading_directories_const(): preserve errno
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
sha1_file.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index 1173071..10395e7 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -166,10 +166,14 @@ enum scld_error safe_create_leading_directories(char *path)
enum scld_error safe_create_leading_directories_const(const char *path)
{
+ int save_errno;
/* path points to cache entries, so xstrdup before messing with it */
char *buf = xstrdup(path);
enum scld_error result = safe_create_leading_directories(buf);
+
+ save_errno = errno;
free(buf);
+ errno = save_errno;
return result;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v3 05/23] raceproof_create_file(): new function
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
cache.h | 43 ++++++++++++++++++++++++++++++++++++++
sha1_file.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+)
diff --git a/cache.h b/cache.h
index 8177c3a..7bc4ea6 100644
--- a/cache.h
+++ b/cache.h
@@ -1057,6 +1057,49 @@ enum scld_error {
enum scld_error safe_create_leading_directories(char *path);
enum scld_error safe_create_leading_directories_const(const char *path);
+/*
+ * Callback function for raceproof_create_file(). This function is
+ * expected to do something that makes dirname(path) permanent despite
+ * the fact that other processes might be cleaning up empty
+ * directories at the same time. Usually it will create a file named
+ * path, but alternatively it could create another file in that
+ * directory, or even chdir() into that directory. The function should
+ * return 0 if the action was completed successfully. On error, it
+ * should return a nonzero result and set errno.
+ * raceproof_create_file() treats two errno values specially:
+ *
+ * - ENOENT -- dirname(path) does not exist. In this case,
+ * raceproof_create_file() tries creating dirname(path)
+ * (and any parent directories, if necessary) and calls
+ * the function again.
+ *
+ * - EISDIR -- the file already exists and is a directory. In this
+ * case, raceproof_create_file() deletes the directory
+ * (recursively) if it is empty and calls the function
+ * again.
+ *
+ * Any other errno causes raceproof_create_file() to fail with the
+ * callback's return value and errno.
+ *
+ * Obviously, this function should be OK with being called again if it
+ * fails with ENOENT or EISDIR. In other scenarios it will not be
+ * called again.
+ */
+typedef int create_file_fn(const char *path, void *cb);
+
+/*
+ * Create a file in dirname(path) by calling fn, creating leading
+ * directories if necessary. Retry a few times in case we are racing
+ * with another process that is trying to clean up the directory that
+ * contains path. See the documentation for create_file_fn for more
+ * details.
+ *
+ * Return the value and set the errno that resulted from the most
+ * recent call of fn. fn is always called at least once, and will be
+ * called more than once if it returns ENOENT or EISDIR.
+ */
+int raceproof_create_file(const char *path, create_file_fn fn, void *cb);
+
int mkdir_in_gitdir(const char *path);
extern char *expand_user_path(const char *path);
const char *enter_repo(const char *path, int strict);
diff --git a/sha1_file.c b/sha1_file.c
index ae8f0b4..b08f54c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -179,6 +179,74 @@ enum scld_error safe_create_leading_directories_const(const char *path)
return result;
}
+int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
+{
+ /*
+ * The number of times we will try to remove empty directories
+ * in the way of path. This is only 1 because if another
+ * process is racily creating directories that conflict with
+ * us, we don't want to fight against them.
+ */
+ int remove_directories_remaining = 1;
+
+ /*
+ * The number of times that we will try to create the
+ * directories containing path. We are willing to attempt this
+ * more than once, because another process could be trying to
+ * clean up empty directories at the same time as we are
+ * trying to create them.
+ */
+ int create_directories_remaining = 3;
+
+ /* A scratch copy of path, filled lazily if we need it: */
+ struct strbuf path_copy = STRBUF_INIT;
+
+ int ret, save_errno;
+
+ /* Sanity check: */
+ assert(*path);
+
+retry_fn:
+ ret = fn(path, cb);
+ save_errno = errno;
+ if (!ret)
+ goto out;
+
+ if (errno == EISDIR && remove_directories_remaining-- > 0) {
+ /*
+ * A directory is in the way. Maybe it is empty; try
+ * to remove it:
+ */
+ if (!path_copy.len)
+ strbuf_addstr(&path_copy, path);
+
+ if (!remove_dir_recursively(&path_copy, REMOVE_DIR_EMPTY_ONLY))
+ goto retry_fn;
+ } else if (errno == ENOENT && create_directories_remaining-- > 0) {
+ /*
+ * Maybe the containing directory didn't exist, or
+ * maybe it was just deleted by a process that is
+ * racing with us to clean up empty directories. Try
+ * to create it:
+ */
+ enum scld_error scld_result;
+
+ if (!path_copy.len)
+ strbuf_addstr(&path_copy, path);
+
+ do {
+ scld_result = safe_create_leading_directories(path_copy.buf);
+ if (scld_result == SCLD_OK)
+ goto retry_fn;
+ } while (scld_result == SCLD_VANISHED && create_directories_remaining-- > 0);
+ }
+
+out:
+ strbuf_release(&path_copy);
+ errno = save_errno;
+ return ret;
+}
+
static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
{
int i;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 06/23] lock_ref_sha1_basic(): inline constant
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
`lflags` is set a single time then never changed, so just inline it.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
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
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2000,7 +2000,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
struct strbuf ref_file = STRBUF_INIT;
struct ref_lock *lock;
int last_errno = 0;
- int lflags = LOCK_NO_DEREF;
int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
int resolve_flags = RESOLVE_REF_NO_RECURSE;
int attempts_remaining = 3;
@@ -2083,7 +2082,7 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
goto error_return;
}
- if (hold_lock_file_for_update(lock->lk, ref_file.buf, lflags) < 0) {
+ if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {
last_errno = errno;
if (errno == ENOENT && --attempts_remaining > 0)
/*
--
2.9.3
^ permalink raw reply related
* [PATCH v3 07/23] lock_ref_sha1_basic(): use raceproof_create_file()
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 35 +++++++++--------------------------
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7d4658a..74de289 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1985,6 +1985,13 @@ static int remove_empty_directories(struct strbuf *path)
return remove_dir_recursively(path, REMOVE_DIR_EMPTY_ONLY);
}
+static int create_reflock(const char *path, void *cb)
+{
+ struct lock_file *lk = cb;
+
+ return hold_lock_file_for_update(lk, path, LOCK_NO_DEREF) < 0 ? -1 : 0;
+}
+
/*
* Locks a ref returning the lock on success and NULL on failure.
* On failure errno is set to something meaningful.
@@ -2002,7 +2009,6 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
int last_errno = 0;
int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
int resolve_flags = RESOLVE_REF_NO_RECURSE;
- int attempts_remaining = 3;
int resolved;
assert_main_repository(&refs->base, "lock_ref_sha1_basic");
@@ -2067,35 +2073,12 @@ static struct ref_lock *lock_ref_sha1_basic(struct files_ref_store *refs,
lock->ref_name = xstrdup(refname);
- retry:
- switch (safe_create_leading_directories_const(ref_file.buf)) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
+ if (raceproof_create_file(ref_file.buf, create_reflock, lock->lk)) {
last_errno = errno;
- strbuf_addf(err, "unable to create directory for '%s'",
- ref_file.buf);
+ unable_to_lock_message(ref_file.buf, errno, err);
goto error_return;
}
- if (hold_lock_file_for_update(lock->lk, ref_file.buf, LOCK_NO_DEREF) < 0) {
- last_errno = errno;
- if (errno == ENOENT && --attempts_remaining > 0)
- /*
- * Maybe somebody just deleted one of the
- * directories leading to ref_file. Try
- * again:
- */
- goto retry;
- else {
- unable_to_lock_message(ref_file.buf, errno, err);
- goto error_return;
- }
- }
if (verify_lock(lock, old_sha1, mustexist, err)) {
last_errno = errno;
goto error_return;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 13/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 345f7c3..7f26cf8 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2719,19 +2719,23 @@ static int open_or_create_logfile(const char *path, void *cb)
}
/*
- * Create a reflog for a ref. If force_create = 0, the reflog will
- * only be created for certain refs (those for which
- * should_autocreate_reflog returns non-zero. Otherwise, create it
- * regardless of the ref name. Fill in *err and return -1 on failure.
+ * Create a reflog for a ref. Store its path to *logfile. If
+ * force_create = 0, only create the reflog for certain refs (those
+ * for which should_autocreate_reflog returns non-zero). Otherwise,
+ * create it regardless of the reference name. If the logfile already
+ * existed or was created, return 0 and set *logfd to the file
+ * descriptor opened for appending to the file. If no logfile exists
+ * and we decided not to create one, return 0 and set *logfd to -1. On
+ * failure, fill in *err, set *logfd to -1, and return -1.
*/
-static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname,
+ struct strbuf *logfile, int *logfd,
+ struct strbuf *err, int force_create)
{
- int logfd;
-
strbuf_git_path(logfile, "logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
- if (raceproof_create_file(logfile->buf, open_or_create_logfile, &logfd)) {
+ if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) {
if (errno == ENOENT)
strbuf_addf(err, "unable to create directory for '%s': "
"%s", logfile->buf, strerror(errno));
@@ -2745,8 +2749,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
return -1;
}
} else {
- logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
- if (logfd < 0) {
+ *logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
+ if (*logfd < 0) {
if (errno == ENOENT || errno == EISDIR) {
/*
* The logfile doesn't already exist,
@@ -2762,10 +2766,8 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
}
}
- if (logfd >= 0) {
+ if (*logfd >= 0)
adjust_shared_perm(logfile->buf);
- close(logfd);
- }
return 0;
}
@@ -2776,11 +2778,14 @@ static int files_create_reflog(struct ref_store *ref_store,
{
int ret;
struct strbuf sb = STRBUF_INIT;
+ int fd;
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "create_reflog");
- ret = log_ref_setup(refname, &sb, err, force_create);
+ ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+ if (fd >= 0)
+ close(fd);
strbuf_release(&sb);
return ret;
}
@@ -2816,17 +2821,17 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
struct strbuf *logfile, int flags,
struct strbuf *err)
{
- int logfd, result, oflags = O_APPEND | O_WRONLY;
+ int logfd, result;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, logfile, err, flags & REF_FORCE_CREATE_REFLOG);
+ result = log_ref_setup(refname, logfile, &logfd, err,
+ flags & REF_FORCE_CREATE_REFLOG);
if (result)
return result;
- logfd = open(logfile->buf, oflags);
if (logfd < 0)
return 0;
result = log_ref_write_fd(logfd, old_sha1, new_sha1,
--
2.9.3
^ permalink raw reply related
* [PATCH v3 12/23] log_ref_setup(): improve robustness against races
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 41 ++++++++++++++++++-----------------------
1 file changed, 18 insertions(+), 23 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 47c7829..345f7c3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2710,6 +2710,14 @@ static int commit_ref(struct ref_lock *lock)
return 0;
}
+static int open_or_create_logfile(const char *path, void *cb)
+{
+ int *fd = cb;
+
+ *fd = open(path, O_APPEND | O_WRONLY | O_CREAT, 0666);
+ return (*fd < 0) ? -1 : 0;
+}
+
/*
* Create a reflog for a ref. If force_create = 0, the reflog will
* only be created for certain refs (those for which
@@ -2723,31 +2731,18 @@ static int log_ref_setup(const char *refname, struct strbuf *logfile, struct str
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) {
- /*
- * The directory that is in the way might be
- * empty. Try to remove it.
- */
- 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)) {
+ if (errno == ENOENT)
+ strbuf_addf(err, "unable to create directory for '%s': "
+ "%s", logfile->buf, strerror(errno));
+ else if (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 {
logfd = open(logfile->buf, O_APPEND | O_WRONLY, 0666);
--
2.9.3
^ permalink raw reply related
* [PATCH v3 11/23] log_ref_setup(): separate code for create vs non-create
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 58 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 38 insertions(+), 20 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fd8a751..47c7829 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2718,45 +2718,63 @@ static int commit_ref(struct ref_lock *lock)
*/
static int log_ref_setup(const char *refname, struct strbuf *logfile, struct strbuf *err, int force_create)
{
- int logfd, oflags = O_APPEND | O_WRONLY;
+ int logfd;
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;
}
- oflags |= O_CREAT;
- }
-
- logfd = open(logfile->buf, oflags, 0666);
- if (logfd < 0) {
- if (!(oflags & O_CREAT) && (errno == ENOENT || errno == EISDIR))
- return 0;
+ logfd = open(logfile->buf, O_APPEND | O_WRONLY | O_CREAT, 0666);
+ if (logfd < 0) {
+ if (errno == EISDIR) {
+ /*
+ * The directory that is in the way might be
+ * empty. Try to remove it.
+ */
+ 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);
}
-
+ } 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) {
+ /*
+ * The logfile doesn't already exist,
+ * but that is not an error; it only
+ * means that we won't write log
+ * entries to it.
+ */
+ } else {
+ strbuf_addf(err, "unable to append to '%s': %s",
+ logfile->buf, strerror(errno));
+ return -1;
+ }
}
}
- adjust_shared_perm(logfile->buf);
- close(logfd);
+ if (logfd >= 0) {
+ adjust_shared_perm(logfile->buf);
+ close(logfd);
+ }
+
return 0;
}
-
static int files_create_reflog(struct ref_store *ref_store,
const char *refname, int force_create,
struct strbuf *err)
--
2.9.3
^ permalink raw reply related
* [PATCH v3 15/23] log_ref_setup(): manage the name of the reflog file internally
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 69 ++++++++++++++++++++++++++--------------------------
1 file changed, 34 insertions(+), 35 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 5a96424..455652c 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2719,37 +2719,36 @@ static int open_or_create_logfile(const char *path, void *cb)
}
/*
- * Create a reflog for a ref. Store its path to *logfile. If
- * force_create = 0, only create the reflog for certain refs (those
- * for which should_autocreate_reflog returns non-zero). Otherwise,
- * create it regardless of the reference name. If the logfile already
- * existed or was created, return 0 and set *logfd to the file
- * descriptor opened for appending to the file. If no logfile exists
- * and we decided not to create one, return 0 and set *logfd to -1. On
- * failure, fill in *err, set *logfd to -1, and return -1.
+ * Create a reflog for a ref. If force_create = 0, only create the
+ * reflog for certain refs (those for which should_autocreate_reflog
+ * returns non-zero). Otherwise, create it regardless of the reference
+ * name. If the logfile already existed or was created, return 0 and
+ * set *logfd to the file descriptor opened for appending to the file.
+ * If no logfile exists and we decided not to create one, return 0 and
+ * set *logfd to -1. On failure, fill in *err, set *logfd to -1, and
+ * return -1.
*/
-static int log_ref_setup(const char *refname,
- struct strbuf *logfile, int *logfd,
- struct strbuf *err, int force_create)
+static int log_ref_setup(const char *refname, int force_create,
+ int *logfd, struct strbuf *err)
{
- strbuf_git_path(logfile, "logs/%s", refname);
+ char *logfile = git_pathdup("logs/%s", refname);
if (force_create || should_autocreate_reflog(refname)) {
- if (raceproof_create_file(logfile->buf, open_or_create_logfile, logfd)) {
+ if (raceproof_create_file(logfile, open_or_create_logfile, logfd)) {
if (errno == ENOENT)
strbuf_addf(err, "unable to create directory for '%s': "
- "%s", logfile->buf, strerror(errno));
+ "%s", logfile, strerror(errno));
else if (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;
+ goto error;
}
} 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) {
/*
@@ -2760,34 +2759,39 @@ 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));
+ goto error;
}
}
}
if (*logfd >= 0)
- adjust_shared_perm(logfile->buf);
+ adjust_shared_perm(logfile);
+ free(logfile);
return 0;
+
+error:
+ free(logfile);
+ return -1;
}
static int files_create_reflog(struct ref_store *ref_store,
const char *refname, int force_create,
struct strbuf *err)
{
- int ret;
- struct strbuf sb = STRBUF_INIT;
int fd;
/* Check validity (but we don't need the result): */
files_downcast(ref_store, 0, "create_reflog");
- ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+ if (log_ref_setup(refname, force_create, &fd, err))
+ return -1;
+
if (fd >= 0)
close(fd);
- strbuf_release(&sb);
- return ret;
+
+ return 0;
}
static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
@@ -2818,16 +2822,15 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg,
- struct strbuf *logfile, int flags,
- struct strbuf *err)
+ int flags, struct strbuf *err)
{
int logfd, result;
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
- result = log_ref_setup(refname, logfile, &logfd, err,
- flags & REF_FORCE_CREATE_REFLOG);
+ result = log_ref_setup(refname, flags & REF_FORCE_CREATE_REFLOG,
+ &logfd, err);
if (result)
return result;
@@ -2858,11 +2861,7 @@ int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
const unsigned char *new_sha1, const char *msg,
int flags, struct strbuf *err)
{
- struct strbuf sb = STRBUF_INIT;
- int ret = log_ref_write_1(refname, old_sha1, new_sha1, msg, &sb, flags,
- err);
- strbuf_release(&sb);
- return ret;
+ return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
}
/*
--
2.9.3
^ permalink raw reply related
* [PATCH v3 16/23] log_ref_write_1(): inline function
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 455652c..a4e0de5 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2820,9 +2820,9 @@ static int log_ref_write_fd(int fd, const unsigned char *old_sha1,
return 0;
}
-static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err)
+int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
+ const unsigned char *new_sha1, const char *msg,
+ int flags, struct strbuf *err)
{
int logfd, result;
@@ -2857,13 +2857,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
return 0;
}
-int files_log_ref_write(const char *refname, const unsigned char *old_sha1,
- const unsigned char *new_sha1, const char *msg,
- int flags, struct strbuf *err)
-{
- return log_ref_write_1(refname, old_sha1, new_sha1, msg, flags, err);
-}
-
/*
* Write sha1 into the open lockfile, then close the lockfile. On
* errors, rollback the lockfile, fill in *err and
--
2.9.3
^ permalink raw reply related
* [PATCH v3 19/23] try_remove_empty_parents(): rename parameter "name" -> "refname"
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
This is the standard nomenclature.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
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
+++ b/refs/files-backend.c
@@ -2282,13 +2282,13 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
/*
* Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *name.
+ * Note: munges *refname.
*/
-static void try_remove_empty_parents(char *name)
+static void try_remove_empty_parents(char *refname)
{
char *p, *q;
int i;
- p = name;
+ p = refname;
for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
while (*p && *p != '/')
p++;
@@ -2306,7 +2306,7 @@ static void try_remove_empty_parents(char *name)
if (q == p)
break;
*q = '\0';
- if (rmdir(git_path("%s", name)))
+ if (rmdir(git_path("%s", refname)))
break;
}
}
--
2.9.3
^ permalink raw reply related
* [PATCH v3 20/23] try_remove_empty_parents(): don't trash argument contents
From: Michael Haggerty @ 2016-12-31 3:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
It's bad manners and surprising and therefore error-prone.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
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 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2282,13 +2282,15 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
/*
* Remove empty parents, but spare refs/ and immediate subdirs.
- * Note: munges *refname.
*/
-static void try_remove_empty_parents(char *refname)
+static void try_remove_empty_parents(const char *refname)
{
+ struct strbuf buf = STRBUF_INIT;
char *p, *q;
int i;
- p = refname;
+
+ strbuf_addstr(&buf, refname);
+ p = buf.buf;
for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
while (*p && *p != '/')
p++;
@@ -2296,8 +2298,7 @@ static void try_remove_empty_parents(char *refname)
while (*p == '/')
p++;
}
- for (q = p; *q; q++)
- ;
+ q = buf.buf + buf.len;
while (1) {
while (q > p && *q != '/')
q--;
@@ -2305,10 +2306,11 @@ static void try_remove_empty_parents(char *refname)
q--;
if (q == p)
break;
- *q = '\0';
- if (rmdir(git_path("%s", refname)))
+ strbuf_setlen(&buf, q - buf.buf);
+ if (rmdir(git_path("%s", buf.buf)))
break;
}
+ strbuf_release(&buf);
}
/* make sure nobody touched the ref, and unlink */
--
2.9.3
^ permalink raw reply related
* [PATCH v3 22/23] try_remove_empty_parents(): teach to remove parents of reflogs, too
From: Michael Haggerty @ 2016-12-31 3:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 397488e..2155acf 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2280,10 +2280,18 @@ static int pack_if_possible_fn(struct ref_entry *entry, void *cb_data)
return 0;
}
+enum {
+ REMOVE_EMPTY_PARENTS_REF = 0x01,
+ REMOVE_EMPTY_PARENTS_REFLOG = 0x02
+};
+
/*
- * Remove empty parents, but spare refs/ and immediate subdirs.
+ * Remove empty parent directories associated with the specified
+ * reference and/or its reflog, but spare [logs/]refs/ and immediate
+ * subdirs. flags is a combination of REMOVE_EMPTY_PARENTS_REF and/or
+ * REMOVE_EMPTY_PARENTS_REFLOG.
*/
-static void try_remove_empty_parents(const char *refname)
+static void try_remove_empty_parents(const char *refname, unsigned int flags)
{
struct strbuf buf = STRBUF_INIT;
char *p, *q;
@@ -2298,7 +2306,7 @@ static void try_remove_empty_parents(const char *refname)
p++;
}
q = buf.buf + buf.len;
- while (1) {
+ while (flags & (REMOVE_EMPTY_PARENTS_REF | REMOVE_EMPTY_PARENTS_REFLOG)) {
while (q > p && *q != '/')
q--;
if (q > p && *(q-1) == '/')
@@ -2306,8 +2314,12 @@ static void try_remove_empty_parents(const char *refname)
if (q == p)
break;
strbuf_setlen(&buf, q - buf.buf);
- if (rmdir(git_path("%s", buf.buf)))
- break;
+ if ((flags & REMOVE_EMPTY_PARENTS_REF) &&
+ rmdir(git_path("%s", buf.buf)))
+ flags &= ~REMOVE_EMPTY_PARENTS_REF;
+ if ((flags & REMOVE_EMPTY_PARENTS_REFLOG) &&
+ rmdir(git_path("logs/%s", buf.buf)))
+ flags &= ~REMOVE_EMPTY_PARENTS_REFLOG;
}
strbuf_release(&buf);
}
@@ -2333,7 +2345,7 @@ static void prune_ref(struct ref_to_prune *r)
}
ref_transaction_free(transaction);
strbuf_release(&err);
- try_remove_empty_parents(r->name);
+ try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
}
static void prune_refs(struct ref_to_prune *r)
--
2.9.3
^ permalink raw reply related
* [PATCH v3 21/23] try_remove_empty_parents(): don't accommodate consecutive slashes
From: Michael Haggerty @ 2016-12-31 3:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
"refname" has already been checked by check_refname_format(), so it
cannot have consecutive slashes.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index af5a0e2..397488e 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2294,15 +2294,14 @@ static void try_remove_empty_parents(const char *refname)
for (i = 0; i < 2; i++) { /* refs/{heads,tags,...}/ */
while (*p && *p != '/')
p++;
- /* tolerate duplicate slashes; see check_refname_format() */
- while (*p == '/')
+ if (*p == '/')
p++;
}
q = buf.buf + buf.len;
while (1) {
while (q > p && *q != '/')
q--;
- while (q > p && *(q-1) == '/')
+ if (q > p && *(q-1) == '/')
q--;
if (q == p)
break;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 23/23] files_transaction_commit(): clean up empty directories
From: Michael Haggerty @ 2016-12-31 3:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 files_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.
This change would be unwise if the *creation* of these directories could
race with our deletion of them. But the earlier changes in this patch
series made the creation paths robust against races, so now it is safe
to tidy them up more aggressively.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 34 ++++++++++++++++++++++++++++------
refs/refs-internal.h | 11 +++++++++--
t/t1400-update-ref.sh | 27 +++++++++++++++++++++++++++
3 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 2155acf..26cfea3 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2345,7 +2345,6 @@ static void prune_ref(struct ref_to_prune *r)
}
ref_transaction_free(transaction);
strbuf_release(&err);
- try_remove_empty_parents(r->name, REMOVE_EMPTY_PARENTS_REF);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3792,6 +3791,7 @@ static int files_transaction_commit(struct ref_store *ref_store,
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
+ update->flags |= REF_DELETED_LOOSE;
}
if (!(update->flags & REF_ISPRUNING))
@@ -3804,16 +3804,38 @@ static int files_transaction_commit(struct ref_store *ref_store,
ret = TRANSACTION_GENERIC_ERROR;
goto cleanup;
}
- 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(refs);
cleanup:
transaction->state = REF_TRANSACTION_CLOSED;
- for (i = 0; i < transaction->nr; i++)
- if (transaction->updates[i]->backend_data)
- unlock_ref(transaction->updates[i]->backend_data);
+ for (i = 0; i < transaction->nr; i++) {
+ struct ref_update *update = transaction->updates[i];
+ struct ref_lock *lock = update->backend_data;
+
+ if (lock)
+ unlock_ref(lock);
+
+ if (update->flags & REF_DELETED_LOOSE) {
+ /*
+ * The loose reference was deleted. Delete any
+ * empty parent directories. (Note that this
+ * can only work because we have already
+ * removed the lockfile.)
+ */
+ try_remove_empty_parents(update->refname,
+ REMOVE_EMPTY_PARENTS_REF);
+ }
+ }
+
string_list_clear(&refs_to_delete, 0);
free(head_ref);
string_list_clear(&affected_refnames, 0);
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..de49362 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -56,6 +56,12 @@
#define REF_UPDATE_VIA_HEAD 0x100
/*
+ * Used as a flag in ref_update::flags when the loose reference has
+ * been deleted.
+ */
+#define REF_DELETED_LOOSE 0x200
+
+/*
* Return true iff refname is minimally safe. "Safe" here means that
* deleting a loose reference by this name will not do any damage, for
* example by causing a file that is not a reference to be deleted.
@@ -157,8 +163,9 @@ struct ref_update {
/*
* One or more of REF_HAVE_NEW, REF_HAVE_OLD, REF_NODEREF,
- * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY, and
- * REF_UPDATE_VIA_HEAD:
+ * REF_DELETING, REF_ISPRUNING, REF_LOG_ONLY,
+ * REF_UPDATE_VIA_HEAD, REF_NEEDS_COMMIT, and
+ * REF_DELETED_LOOSE:
*/
unsigned int flags;
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index d4fb977..97d8793 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -191,6 +191,33 @@ test_expect_success \
git update-ref HEAD'" $A &&
test $A"' = $(cat .git/'"$m"')'
+test_expect_success "empty directory removal" '
+ git branch d1/d2/r1 HEAD &&
+ git branch d1/r2 HEAD &&
+ test -f .git/refs/heads/d1/d2/r1 &&
+ test -f .git/logs/refs/heads/d1/d2/r1 &&
+ git branch -d d1/d2/r1 &&
+ ! test -e .git/refs/heads/d1/d2 &&
+ ! test -e .git/logs/refs/heads/d1/d2 &&
+ test -f .git/refs/heads/d1/r2 &&
+ test -f .git/logs/refs/heads/d1/r2
+'
+
+test_expect_success "symref empty directory removal" '
+ git branch e1/e2/r1 HEAD &&
+ git branch e1/r2 HEAD &&
+ git checkout e1/e2/r1 &&
+ test_when_finished "git checkout master" &&
+ test -f .git/refs/heads/e1/e2/r1 &&
+ test -f .git/logs/refs/heads/e1/e2/r1 &&
+ git update-ref -d HEAD &&
+ ! test -e .git/refs/heads/e1/e2 &&
+ ! test -e .git/logs/refs/heads/e1/e2 &&
+ test -f .git/refs/heads/e1/r2 &&
+ test -f .git/logs/refs/heads/e1/r2 &&
+ test -f .git/logs/HEAD
+'
+
cat >expect <<EOF
$Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000 Initial Creation
$A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000 Switch
--
2.9.3
^ permalink raw reply related
* [PATCH v3 17/23] delete_ref_loose(): derive loose reference path from lock
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a4e0de5..847af81 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2430,10 +2430,7 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
* loose. The loose file name is the same as the
* lockfile name, minus ".lock":
*/
- char *loose_filename = get_locked_file_path(lock->lk);
- int res = unlink_or_msg(loose_filename, err);
- free(loose_filename);
- if (res)
+ if (unlink_or_msg(git_path("%s", lock->ref_name), err))
return 1;
}
return 0;
--
2.9.3
^ permalink raw reply related
* [PATCH v3 18/23] delete_ref_loose(): inline function
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
It was hardly doing anything anymore, and had only one caller.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
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 847af81..e81c8a9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2421,21 +2421,6 @@ static int repack_without_refs(struct files_ref_store *refs,
return ret;
}
-static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err)
-{
- assert(err);
-
- if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
- /*
- * loose. The loose file name is the same as the
- * lockfile name, minus ".lock":
- */
- if (unlink_or_msg(git_path("%s", lock->ref_name), err))
- return 1;
- }
- return 0;
-}
-
static int files_delete_refs(struct ref_store *ref_store,
struct string_list *refnames, unsigned int flags)
{
@@ -3787,9 +3772,13 @@ static int files_transaction_commit(struct ref_store *ref_store,
if (update->flags & REF_DELETING &&
!(update->flags & REF_LOG_ONLY)) {
- if (delete_ref_loose(lock, update->type, err)) {
- ret = TRANSACTION_GENERIC_ERROR;
- goto cleanup;
+ if (!(update->type & REF_ISPACKED) ||
+ update->type & REF_ISSYMREF) {
+ /* It is a loose reference. */
+ if (unlink_or_msg(git_path("%s", lock->ref_name), err)) {
+ ret = TRANSACTION_GENERIC_ERROR;
+ goto cleanup;
+ }
}
if (!(update->flags & REF_ISPRUNING))
--
2.9.3
^ permalink raw reply related
* [PATCH v3 14/23] log_ref_write_1(): don't depend on logfile argument
From: Michael Haggerty @ 2016-12-31 3:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, David Turner, Michael Haggerty
In-Reply-To: <cover.1483153436.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 7f26cf8..5a96424 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2837,14 +2837,18 @@ 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));
+ int save_errno = errno;
+
+ strbuf_addf(err, "unable to append to '%s': %s",
+ git_path("logs/%s", refname), strerror(save_errno));
close(logfd);
return -1;
}
if (close(logfd)) {
- strbuf_addf(err, "unable to append to '%s': %s", logfile->buf,
- strerror(errno));
+ int save_errno = errno;
+
+ strbuf_addf(err, "unable to append to '%s': %s",
+ git_path("logs/%s", refname), strerror(save_errno));
return -1;
}
return 0;
--
2.9.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox