* Re: Regression: Ctrl-c from the pager in an alias exits it
From: Jeff King @ 2017-01-06 19:41 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Trygve Aaberge, Nguyễn Thái Ngọc Duy, git
In-Reply-To: <3d433abf-71a2-4702-f62b-e254520dc32c@kdbg.org>
On Fri, Jan 06, 2017 at 03:39:59PM +0100, Johannes Sixt wrote:
> > diff --git a/run-command.c b/run-command.c
> > index ca905a9e80..db47c429b7 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -29,6 +29,8 @@ static int installed_child_cleanup_handler;
> >
> > static void cleanup_children(int sig, int in_signal)
> > {
> > + struct child_to_clean *children_to_wait_for = NULL;
> > +
> > while (children_to_clean) {
> > struct child_to_clean *p = children_to_clean;
> > children_to_clean = p->next;
> > @@ -45,6 +47,17 @@ static void cleanup_children(int sig, int in_signal)
> > }
> >
> > kill(p->pid, sig);
> > + p->next = children_to_wait_for;
> > + children_to_wait_for = p;
> > + }
> > +
> > + while (children_to_wait_for) {
> > + struct child_to_clean *p = children_to_wait_for;
> > + children_to_wait_for = p->next;
> > +
> > + while (waitpid(p->pid, NULL, 0) < 0 && errno == EINTR)
> > + ; /* spin waiting for process exit or error */
> > +
> > if (!in_signal)
> > free(p);
> > }
> >
>
> This looks like the minimal change necessary. I wonder, though, whether the
> new local variable is really required. Wouldn't it be sufficient to walk the
> children_to_clean chain twice?
Yeah, I considered that. The fact that we disassemble the list in the
first loop has two side effects:
1. It lets us free the list as we go (for the !in_signal case).
2. If we were to get another signal, it makes us sort-of reentrant. We
will only kill and wait for each pid once.
Obviously (1) moves down to the lower loop, but I was trying to preserve
(2). I'm not sure if it is worth bothering, though. The way we pull
items off of the list is certainly not atomic (it does shorten the race
to a few instructions, though, versus potentially waiting on waitpid()
to return).
My bigger concern with the whole thing is whether we could hit some sort
of deadlock if the child doesn't die when we send it a signal. E.g.,
imagine we have a pipe open to the child and somebody sends SIGTERM to
us. We propagate SIGTERM to the child, and then waitpid() for it. The
child decides to ignore our SIGTERM for some reason and keep reading
until EOF on the pipe. It won't ever get it, and the two processes will
hang forever.
You can argue perhaps that the child is broken in that case. And I doubt
this could trigger when running a git sub-command. But we may add more
children in the future. Right now we use it for the new multi-file
clean/smudge filters. They use the hook feature to close the
descriptors, but note that that won't run in the in_signal case.
So I dunno. Maybe this waiting should be restricted only to certain
cases like executing git sub-commands.
-Peff
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Ralf Thielow @ 2017-01-06 19:31 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jake Lambert, git@vger.kernel.org
In-Reply-To: <CAGZ79kaz_hS9P7vqV9EhZazM_g0OUdRfMtFPJ7gFu-h-ku=NKw@mail.gmail.com>
2017-01-06 18:55 GMT+01:00 Stefan Beller <sbeller@google.com>:
> On Fri, Jan 6, 2017 at 9:25 AM, Jake Lambert <jake@onahill.co> wrote:
>> Hello,
>>
>> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
>
> That seems strange. Is it possible that your config is not writable?
> (.git/config, ~/gitconfig, you'd need to find out where the <branch>
> is configured already via git config --global/--system/--local --list)
>
Have you actually tried to reproduce this issue? I'm on current next
and can reproduce the problem.
It seems Git is not happy with unsetting non-existent configs. And at
least the following piece of code
seems to be there since 10bea152a (Add functions git_config_set() and
git_config_set_multivar(), 2005-11-17).
config.c:2212
/* if nothing to unset, or too many matches, error out */
if ((store.seen == 0 && value == NULL) ||
I wonder why it's an error to remove something that doesn't exist
except the case we 'know' it
exists because of further actions. I don't think this is a thing, though.
Just for the record: `git config --unset branch.$branch.description`
does return with code "5" here as well.
I think removing things that don't exist should behave the same like
removing things that do exist, with
a success.
Ralf
^ permalink raw reply
* Rebasing a branch with merges
From: Robert Dailey @ 2017-01-06 19:12 UTC (permalink / raw)
To: Git
Here's the scenario:
I create a topic branch so one other developer and myself can work on
a feature that takes 2 weeks to complete. During that 2 week period,
changes are occurring on master that I need in my topic branch. Since
I have a collaborator on the branch, I opt for merges instead of
rebase.
Each day I merge from master to the topic branch, which changes code
I'm actively working in and requires semantic changes (functions
renamed, moved, etc).
Once I'm ready to merge the topic branch back into master, I have two
options (bearing in mind the goal is to keep history as clean as
possible. Furthermore this implies that the constant merging into
topic from master has made the topic branch look unwieldy and
difficult to audit):
1. Do a squash merge, which keeps history clean but we lose context
for the important bits (the commits representing units of work that
contribute to the topic itself).
2. Do a final rebase prior to merging.
#2 doesn't seem to be possible due to patch ordering. For example, if
I have real commits after merge commits that depend on those changes
from master being present as a base at that point in time, the rebase
will cause the patch before it to no longer include those changes from
master.
Is there a mechanism to rebase in this situation to both achieve a
clean, linear history for the topic branch and allow fast forward
merging if desired, while still not causing superfluous conflicts due
to the merges being omitted during the rebase?
Thanks in advance for any advice in this scenario.
^ permalink raw reply
* Re: git branch --editdescription fatal error
From: Stefan Beller @ 2017-01-06 17:55 UTC (permalink / raw)
To: Jake Lambert; +Cc: git@vger.kernel.org
In-Reply-To: <MWHPR19MB11357994E6C43DBCC0931CD7BC630@MWHPR19MB1135.namprd19.prod.outlook.com>
On Fri, Jan 6, 2017 at 9:25 AM, Jake Lambert <jake@onahill.co> wrote:
> Hello,
>
> When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place.
That seems strange. Is it possible that your config is not writable?
(.git/config, ~/gitconfig, you'd need to find out where the <branch>
is configured already via git config --global/--system/--local --list)
> I am not too familiar with the inner workings of git, but if you accept pull requests, I'm happy to give it a shot if you can give me directions as to how to submit one to you.
We rather do email based workflow, see
https://github.com/git/git/blob/master/Documentation/SubmittingPatches
(We use github only as a host, you could also obtain it from one of
https://git-blame.blogspot.com/p/git-public-repositories.html)
>
> While I have you, I would also like to make a feature request for branch descriptions to (optionally) be included in the output of git branch <noargs> (or something like git branch --descriptions). This is particularly helpful where branch names match JIRA tickets numbers or are simply so old I don't remember what they are but don't want to delete them just in case.
Apparently the branch descriptions are only used for the request-pull
subcommand,
https://github.com/git/git/commit/c0168147831fce00975949213eef3471b7a2b76b
and the merge message
https://github.com/git/git/commit/898eacd8ada2d012f977948350ed60845e238037
If you want to work on both of these issues, have a look at
edit_branch_description
as well as print_ref_list in builtin/branch.c
> Thank you for maintaining this absolutely wonderful program, and have a happy New Year!
Happy new year!
Stefan
^ permalink raw reply
* git branch --editdescription fatal error
From: Jake Lambert @ 2017-01-06 17:25 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
When executing "git branch <branch> --edit-description" on a branch with no description set, I get "fatal: could not unset 'branch.<branch>.description". It would seem that the unsetting piece should occur only after checking if it was set in the first place. I am not too familiar with the inner workings of git, but if you accept pull requests, I'm happy to give it a shot if you can give me directions as to how to submit one to you.
While I have you, I would also like to make a feature request for branch descriptions to (optionally) be included in the output of git branch <noargs> (or something like git branch --descriptions). This is particularly helpful where branch names match JIRA tickets numbers or are simply so old I don't remember what they are but don't want to delete them just in case.
Thank you for maintaining this absolutely wonderful program, and have a happy New Year!
Best,
Jake
Jake Lambert
Full Stack Developer
The Company On A Hill
www.onahill.co
^ permalink raw reply
* [PATCH v4 14/23] log_ref_setup(): pass the open file descriptor back to the caller
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 27d4fd3..f723834 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,
@@ -2763,10 +2767,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;
}
@@ -2777,11 +2779,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;
}
@@ -2817,17 +2822,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 v4 17/23] log_ref_write_1(): inline function
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 846380f..39d6f5b 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2821,9 +2821,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;
@@ -2858,13 +2858,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 v4 15/23] log_ref_write_1(): don't depend on logfile argument
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 f723834..9c5e804 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2838,14 +2838,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
* [PATCH v4 12/23] log_ref_setup(): separate code for create vs non-create
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 | 59 ++++++++++++++++++++++++++++++++++------------------
1 file changed, 39 insertions(+), 20 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index fd8a751..c8f6d82 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2718,45 +2718,64 @@ 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 v4 16/23] log_ref_setup(): manage the name of the reflog file internally
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 9c5e804..846380f 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) {
/*
@@ -2761,34 +2760,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,
@@ -2819,16 +2823,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;
@@ -2859,11 +2862,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 v4 20/23] try_remove_empty_parents(): rename parameter "name" -> "refname"
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 9abd7c3..92a9d99 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 v4 21/23] try_remove_empty_parents(): don't trash argument contents
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 92a9d99..88f8c7a 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 v4 22/23] try_remove_empty_parents(): teach to remove parents of reflogs, too
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 88f8c7a..bce0022 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;
@@ -2299,7 +2307,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--;
while (q > p && *(q-1) == '/')
@@ -2307,8 +2315,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);
}
@@ -2334,7 +2346,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 v4 19/23] delete_ref_loose(): inline function
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 4d55364..9abd7c3 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)
{
@@ -3788,9 +3773,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 v4 18/23] delete_ref_loose(): derive loose reference path from lock
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 39d6f5b..4d55364 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 v4 23/23] files_transaction_commit(): clean up empty directories
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 bce0022..c426575 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2346,7 +2346,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)
@@ -3794,6 +3793,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))
@@ -3806,16 +3806,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 dc81acc..15d5a1e 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.
@@ -158,8 +164,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 v4 11/23] log_ref_write(): inline function
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
refs/files-backend.c | 24 ++++++++++--------------
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 49a119c..fd8a751 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2832,14 +2832,6 @@ static int log_ref_write_1(const char *refname, const unsigned char *old_sha1,
return 0;
}
-static int 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 files_log_ref_write(refname, old_sha1, new_sha1, msg, flags,
- 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)
@@ -2903,7 +2895,8 @@ static int commit_ref_update(struct files_ref_store *refs,
assert_main_repository(&refs->base, "commit_ref_update");
clear_loose_ref_cache(refs);
- if (log_ref_write(lock->ref_name, lock->old_oid.hash, sha1, logmsg, 0, err)) {
+ if (files_log_ref_write(lock->ref_name, lock->old_oid.hash, sha1,
+ logmsg, 0, err)) {
char *old_msg = strbuf_detach(err, NULL);
strbuf_addf(err, "cannot update the ref '%s': %s",
lock->ref_name, old_msg);
@@ -2934,7 +2927,7 @@ static int commit_ref_update(struct files_ref_store *refs,
if (head_ref && (head_flag & REF_ISSYMREF) &&
!strcmp(head_ref, lock->ref_name)) {
struct strbuf log_err = STRBUF_INIT;
- if (log_ref_write("HEAD", lock->old_oid.hash, sha1,
+ if (files_log_ref_write("HEAD", lock->old_oid.hash, sha1,
logmsg, 0, &log_err)) {
error("%s", log_err.buf);
strbuf_release(&log_err);
@@ -2973,7 +2966,8 @@ static void update_symref_reflog(struct ref_lock *lock, const char *refname,
struct strbuf err = STRBUF_INIT;
unsigned char new_sha1[20];
if (logmsg && !read_ref(target, new_sha1) &&
- log_ref_write(refname, lock->old_oid.hash, new_sha1, logmsg, 0, &err)) {
+ files_log_ref_write(refname, lock->old_oid.hash, new_sha1,
+ logmsg, 0, &err)) {
error("%s", err.buf);
strbuf_release(&err);
}
@@ -3748,9 +3742,11 @@ static int files_transaction_commit(struct ref_store *ref_store,
if (update->flags & REF_NEEDS_COMMIT ||
update->flags & REF_LOG_ONLY) {
- if (log_ref_write(lock->ref_name, lock->old_oid.hash,
- update->new_sha1,
- update->msg, update->flags, err)) {
+ if (files_log_ref_write(lock->ref_name,
+ lock->old_oid.hash,
+ update->new_sha1,
+ update->msg, update->flags,
+ err)) {
char *old_msg = strbuf_detach(err, NULL);
strbuf_addf(err, "cannot update the ref '%s': %s",
--
2.9.3
^ permalink raw reply related
* [PATCH v4 08/23] lock_ref_sha1_basic(): use raceproof_create_file()
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 v4 13/23] log_ref_setup(): improve robustness against races
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 c8f6d82..27d4fd3 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 v4 07/23] lock_ref_sha1_basic(): inline constant
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 v4 04/23] safe_create_leading_directories_const(): preserve errno
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.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 v4 09/23] rename_tmp_log(): use raceproof_create_file()
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
Besides shortening the code, this saves an unnecessary call to
safe_create_leading_directories_const() in almost all cases.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/files-backend.c | 73 +++++++++++++++++++++-------------------------------
1 file changed, 30 insertions(+), 43 deletions(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 74de289..3f18a01 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2489,55 +2489,42 @@ static int files_delete_refs(struct ref_store *ref_store,
*/
#define TMP_RENAMED_LOG "logs/refs/.tmp-renamed-log"
-static int rename_tmp_log(const char *newrefname)
+static int rename_tmp_log_callback(const char *path, void *cb)
{
- int attempts_remaining = 4;
- struct strbuf path = STRBUF_INIT;
- int ret = -1;
+ int *true_errno = cb;
- retry:
- strbuf_reset(&path);
- strbuf_git_path(&path, "logs/%s", newrefname);
- switch (safe_create_leading_directories_const(path.buf)) {
- case SCLD_OK:
- break; /* success */
- case SCLD_VANISHED:
- if (--attempts_remaining > 0)
- goto retry;
- /* fall through */
- default:
- error("unable to create directory for %s", newrefname);
- goto out;
+ if (rename(git_path(TMP_RENAMED_LOG), path)) {
+ /*
+ * rename(a, b) when b is an existing directory ought
+ * to result in ISDIR, but Solaris 5.8 gives ENOTDIR.
+ * Sheesh. Record the true errno for error reporting,
+ * but report EISDIR to raceproof_create_file() so
+ * that it knows to retry.
+ */
+ *true_errno = errno;
+ if (errno == ENOTDIR)
+ errno = EISDIR;
+ return -1;
+ } else {
+ return 0;
}
+}
- if (rename(git_path(TMP_RENAMED_LOG), path.buf)) {
- if ((errno==EISDIR || errno==ENOTDIR) && --attempts_remaining > 0) {
- /*
- * rename(a, b) when b is an existing
- * directory ought to result in ISDIR, but
- * Solaris 5.8 gives ENOTDIR. Sheesh.
- */
- if (remove_empty_directories(&path)) {
- error("Directory not empty: logs/%s", newrefname);
- goto out;
- }
- goto retry;
- } else if (errno == ENOENT && --attempts_remaining > 0) {
- /*
- * Maybe another process just deleted one of
- * the directories in the path to newrefname.
- * Try again from the beginning.
- */
- goto retry;
- } else {
+static int rename_tmp_log(const char *newrefname)
+{
+ char *path = git_pathdup("logs/%s", newrefname);
+ int ret, true_errno;
+
+ ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
+ if (ret) {
+ if (errno == EISDIR)
+ error("Directory not empty: %s", path);
+ else
error("unable to move logfile "TMP_RENAMED_LOG" to logs/%s: %s",
- newrefname, strerror(errno));
- goto out;
- }
+ newrefname, strerror(true_errno));
}
- ret = 0;
-out:
- strbuf_release(&path);
+
+ free(path);
return ret;
}
--
2.9.3
^ permalink raw reply related
* [PATCH v4 10/23] rename_tmp_log(): improve error reporting
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
* Don't capitalize error strings
* Report true paths of affected files
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
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 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2518,10 +2518,11 @@ static int rename_tmp_log(const char *newrefname)
ret = raceproof_create_file(path, rename_tmp_log_callback, &true_errno);
if (ret) {
if (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);
--
2.9.3
^ permalink raw reply related
* [PATCH v4 05/23] safe_create_leading_directories(): set errno on SCLD_EXISTS
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
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 <mhagger@alum.mit.edu>
---
cache.h | 5 +++--
sha1_file.c | 4 +++-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/cache.h b/cache.h
index a50a61a..8177c3a 100644
--- a/cache.h
+++ b/cache.h
@@ -1031,8 +1031,9 @@ int adjust_shared_perm(const char *path);
/*
* Create the directory containing the named path, using care to be
- * somewhat safe against races. Return one of the scld_error values
- * to indicate success/failure.
+ * somewhat safe against races. Return one of the scld_error values to
+ * indicate success/failure. On error, set errno to describe the
+ * problem.
*
* SCLD_VANISHED indicates that one of the ancestor directories of the
* path existed at one point during the function call and then
diff --git a/sha1_file.c b/sha1_file.c
index 10395e7..ae8f0b4 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -137,8 +137,10 @@ enum scld_error safe_create_leading_directories(char *path)
*slash = '\0';
if (!stat(path, &st)) {
/* path exists */
- if (!S_ISDIR(st.st_mode))
+ if (!S_ISDIR(st.st_mode)) {
+ errno = ENOTDIR;
ret = SCLD_EXISTS;
+ }
} else if (mkdir(path, 0777)) {
if (errno == EEXIST &&
!stat(path, &st) && S_ISDIR(st.st_mode))
--
2.9.3
^ permalink raw reply related
* [PATCH v4 02/23] refname_is_safe(): correct docstring
From: Michael Haggerty @ 2017-01-06 16:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, David Turner, Jacob Keller, Philip Oakley,
Michael Haggerty
In-Reply-To: <cover.1483719289.git.mhagger@alum.mit.edu>
The behavior of refname_is_safe() was changed in
e40f355 "refname_is_safe(): insist that the refname already be normalized", 2016-04-27
without a corresponding update to its docstring. The function is in fact
stricter than documented, because it now insists that the result of
normalizing the part of a refname following "refs/" is identical to that
part of the original refname. Fix the docstring.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
refs/refs-internal.h | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/refs/refs-internal.h b/refs/refs-internal.h
index 708b260..dc81acc 100644
--- a/refs/refs-internal.h
+++ b/refs/refs-internal.h
@@ -62,11 +62,12 @@
* This function does not check that the reference name is legal; for
* that, use check_refname_format().
*
- * We consider a refname that starts with "refs/" to be safe as long
- * as any ".." components that it might contain do not escape "refs/".
- * Names that do not start with "refs/" are considered safe iff they
- * consist entirely of upper case characters and '_' (like "HEAD" and
- * "MERGE_HEAD" but not "config" or "FOO/BAR").
+ * A refname that starts with "refs/" is considered safe iff it
+ * doesn't contain any "." or ".." components or consecutive '/'
+ * characters, end with '/', or (on Windows) contain any '\'
+ * characters. Names that do not start with "refs/" are considered
+ * safe iff they consist entirely of upper case characters and '_'
+ * (like "HEAD" and "MERGE_HEAD" but not "config" or "FOO/BAR").
*/
int refname_is_safe(const char *refname);
--
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