Re: [PATCH 09/20] log_ref_setup(): pass the open file descriptor back to the caller

2016-02-18 Thread Junio C Hamano
Michael Haggerty  writes:

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

Makes sense.

>   } else {
>   adjust_shared_perm(logfile->buf);
> - close(logfd);
>   }

I briefly wondered if permission twiddling on an open fd may have
negative effect on certain platforms, but the original was doing so
already (even before step 08/20), so that shouldn't be an issue with
this change, either.

Nicely done.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 09/20] log_ref_setup(): pass the open file descriptor back to the caller

2016-02-16 Thread Michael Haggerty
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 
---
 refs/files-backend.c | 37 +
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0cfe1ce..78a6213 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2591,19 +2591,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 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) < 0) {
+   if (raceproof_create_file(logfile->buf, open_or_create_logfile, 
logfd) < 0) {
if (errno == ENOENT) {
strbuf_addf(err, "unable to create directory 
for %s: "
"%s", logfile->buf, 
strerror(errno));
@@ -2617,11 +2621,10 @@ static int log_ref_setup(const char *refname, struct 
strbuf *logfile, struct str
return -1;
} else {
adjust_shared_perm(logfile->buf);
-   close(logfd);
}
} 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,
@@ -2636,7 +2639,6 @@ static int log_ref_setup(const char *refname, struct 
strbuf *logfile, struct str
}
} else {
adjust_shared_perm(logfile->buf);
-   close(logfd);
}
}
 
@@ -2647,8 +2649,11 @@ int safe_create_reflog(const char *refname, int 
force_create, struct strbuf *err
 {
int ret;
struct strbuf sb = STRBUF_INIT;
+   int fd;
 
-   ret = log_ref_setup(refname, &sb, err, force_create);
+   ret = log_ref_setup(refname, &sb, &fd, err, force_create);
+   if (!ret && fd >= 0)
+   close(fd);
strbuf_release(&sb);
return ret;
 }
@@ -2684,17 +2689,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.7.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html