From aabbd3f3c9dd17c414100d0e029f8a3d13673ab1 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:06:50 -0400
Subject: [PATCH 1/9] config: fix bogus fd check when setting up default config
Since 9830534 (config --global --edit: create a template
file if needed, 2014-07-25), an edit of the global config
file will try to open() it with O_EXCL, and wants to handle
three cases:
1. We succeeded; the user has no config file, and we
should fill in the default template.
2. We got EEXIST; they have a file already, proceed as usual.
3. We got another error; we should complain.
However, the check for case 1 does "if (fd)", which will
generally _always_ be true (except for the oddball case that
somehow our stdin got closed and opening really did give us
a new descriptor 0).
So in the EEXIST case, we tried to write the default config
anyway! Fortunately, this turns out to be a noop, since we
just end up writing to and closing "-1", which does nothing.
But in case 3, we would fail to notice any other errors, and
just silently continue (given that we don't actually notice
write errors for the template either, it's probably not that
big a deal; we're about to spawn the editor, so it would
notice any problems. But the code is clearly _trying_ to hit
cover this case and failing).
We can fix it easily by using "fd >= 0" for case 1.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/config.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/config.c b/builtin/config.c
index 1d7c6ef558b..a991a534185 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -604,7 +604,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
given_config_source.file : git_path("config"));
if (use_global_config) {
int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);
- if (fd) {
+ if (fd >= 0) {
char *content = default_user_config();
write_str_in_full(fd, content);
free(content);
From 1dad879a7b7b41381cf4ccf471dcab06993a131b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= <l.s.r@web.de>
Date: Fri, 8 Jul 2016 05:08:05 -0400
Subject: [PATCH 2/9] am: ignore return value of write_file()
write_file() either returns 0 or dies, so there is no point in checking
its return value. The callers of the wrappers write_state_text(),
write_state_count() and write_state_bool() consequently already ignore
their return values. Stop pretending we care and make them void.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/am.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 3dfe70b7a03..a9742238767 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -183,22 +183,22 @@ static inline const char *am_path(const struct am_state *state, const char *path
/**
* For convenience to call write_file()
*/
-static int write_state_text(const struct am_state *state,
- const char *name, const char *string)
+static void write_state_text(const struct am_state *state,
+ const char *name, const char *string)
{
- return write_file(am_path(state, name), "%s", string);
+ write_file(am_path(state, name), "%s", string);
}
-static int write_state_count(const struct am_state *state,
+static void write_state_count(const struct am_state *state,
+ const char *name, int value)
+{
+ write_file(am_path(state, name), "%d", value);
+}
+
+static void write_state_bool(const struct am_state *state,
const char *name, int value)
{
- return write_file(am_path(state, name), "%d", value);
-}
-
-static int write_state_bool(const struct am_state *state,
- const char *name, int value)
-{
- return write_state_text(state, name, value ? "t" : "f");
+ write_state_text(state, name, value ? "t" : "f");
}
/**
From 3d75bba28d8665ec43f1faffce38d5817c77ebe8 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:08:54 -0400
Subject: [PATCH 3/9] branch: use non-gentle write_file for branch description
We use write_file_gently() to do this job currently.
However, if we see an error, we simply complain via
error_errno() and then end up exiting with an error code.
By switching to the non-gentle form, the function will die
for us, with a better error. It is more specific about which
syscall caused the error, and that mentions the
actual filename we're trying to write.
Our exit code for the error case does switch from "1" to
"128", but that's OK; it wasn't a meaningful documented code
(and in fact it was odd that it was a different exit code
than most other error conditions).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/branch.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 2ecde53bf83..15232c4a428 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -618,10 +618,7 @@ static int edit_branch_description(const char *branch_name)
" %s\n"
"Lines starting with '%c' will be stripped.\n",
branch_name, comment_line_char);
- if (write_file_gently(git_path(edit_description), "%s", buf.buf)) {
- strbuf_release(&buf);
- return error_errno(_("could not write branch description template"));
- }
+ write_file(git_path(edit_description), "%s", buf.buf);
strbuf_reset(&buf);
if (launch_editor(git_path(edit_description), &buf, NULL)) {
strbuf_release(&buf);
From ef22318cff51244cd0047b11ee7accfded522782 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:09:34 -0400
Subject: [PATCH 4/9] write_file: drop "gently" form
There are no callers left of write_file_gently(). Let's drop
it, as it doesn't seem likely for new callers to be added
(since its inception, the only callers who wanted the gentle
form generally just died immediately themselves, and have
since been converted).
While we're there, let's also drop the "int" return from
write_file, as it is never meaningful (in the non-gentle
form, we always either die or return 0).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 3 +--
wrapper.c | 56 ++++++++++++-------------------------------------------
2 files changed, 13 insertions(+), 46 deletions(-)
diff --git a/cache.h b/cache.h
index 6049f867113..3f6ae242dfd 100644
--- a/cache.h
+++ b/cache.h
@@ -1734,8 +1734,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
return write_in_full(fd, str, strlen(str));
}
-extern int write_file(const char *path, const char *fmt, ...);
-extern int write_file_gently(const char *path, const char *fmt, ...);
+extern void write_file(const char *path, const char *fmt, ...);
/* pager.c */
extern void setup_pager(void);
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15aa9b..0349441bde5 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -640,56 +640,24 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
return len;
}
-static int write_file_v(const char *path, int fatal,
- const char *fmt, va_list params)
+void write_file(const char *path, const char *fmt, ...)
{
+ va_list params;
struct strbuf sb = STRBUF_INIT;
int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
- if (fd < 0) {
- if (fatal)
- die_errno(_("could not open %s for writing"), path);
- return -1;
- }
+ if (fd < 0)
+ die_errno(_("could not open %s for writing"), path);
+
+ va_start(params, fmt);
strbuf_vaddf(&sb, fmt, params);
+ va_end(params);
+
strbuf_complete_line(&sb);
- if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
- int err = errno;
- close(fd);
- strbuf_release(&sb);
- errno = err;
- if (fatal)
- die_errno(_("could not write to %s"), path);
- return -1;
- }
+ if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+ die_errno(_("could not write to %s"), path);
strbuf_release(&sb);
- if (close(fd)) {
- if (fatal)
- die_errno(_("could not close %s"), path);
- return -1;
- }
- return 0;
-}
-
-int write_file(const char *path, const char *fmt, ...)
-{
- int status;
- va_list params;
-
- va_start(params, fmt);
- status = write_file_v(path, 1, fmt, params);
- va_end(params);
- return status;
-}
-
-int write_file_gently(const char *path, const char *fmt, ...)
-{
- int status;
- va_list params;
-
- va_start(params, fmt);
- status = write_file_v(path, 0, fmt, params);
- va_end(params);
- return status;
+ if (close(fd))
+ die_errno(_("could not close %s"), path);
}
void sleep_millisec(int millisec)
From ee861e0f78367ff0e94feeb42f721ef83ceec251 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:10:08 -0400
Subject: [PATCH 5/9] write_file: use xopen
This simplifies the code a tiny bit, and provides consistent
error messages with other users of xopen().
While we're here, let's also switch to using O_WRONLY. We
know we're only going to open/write/close the file, so
there's no point in asking for O_RDWR.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
wrapper.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/wrapper.c b/wrapper.c
index 0349441bde5..7c126b87c50 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -644,9 +644,7 @@ void write_file(const char *path, const char *fmt, ...)
{
va_list params;
struct strbuf sb = STRBUF_INIT;
- int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
- if (fd < 0)
- die_errno(_("could not open %s for writing"), path);
+ int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
va_start(params, fmt);
strbuf_vaddf(&sb, fmt, params);
From 52563d7ecc8f3f38cb1c0521294c5f6a0a475637 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:12:22 -0400
Subject: [PATCH 6/9] write_file: add pointer+len variant
There are many callsites which could use write_file, but for
which it is a little awkward because they have a strbuf or
other pointer/len combo. Specifically:
1. write_file() takes a format string, so we have to use
"%s" or "%.*s", which are ugly.
2. Using any form of "%s" does not handle embedded NULs in
the output. That probably doesn't matter for our
call-sites, but it's nicer not to have to worry.
3. It's less efficient; we format into another strbuf
just to do the write. That's probably not measurably
slow for our uses, but it's simply inelegant.
We can fix this by providing a helper to write out the
formatted buffer, and just calling it from write_file().
Note that we don't do the usual "complete with a newline"
that write_file does. If the caller has their own buffer,
there's a reasonable chance they're doing something more
complicated than a single line, and they can call
strbuf_complete_line() themselves.
We could go even further and add strbuf_write_file(), but it
doesn't save much:
- write_file_buf(path, sb.buf, sb.len);
+ strbuf_write_file(&sb, path);
It would also be somewhat asymmetric with strbuf_read_file,
which actually returns errors rather than dying (and the
error handling is most of the benefit of write_file() in the
first place).
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 6 ++++++
wrapper.c | 16 +++++++++++-----
2 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/cache.h b/cache.h
index 3f6ae242dfd..206d594370f 100644
--- a/cache.h
+++ b/cache.h
@@ -1734,6 +1734,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
return write_in_full(fd, str, strlen(str));
}
+/**
+ * Open (and truncate) the file at path, write the contents of buf to it,
+ * and close it. Dies if any errors are encountered.
+ */
+extern void write_file_buf(const char *path, const char *buf, size_t len);
+
extern void write_file(const char *path, const char *fmt, ...);
/* pager.c */
diff --git a/wrapper.c b/wrapper.c
index 7c126b87c50..b827206a42f 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -640,22 +640,28 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
return len;
}
+void write_file_buf(const char *path, const char *buf, size_t len)
+{
+ int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+ if (write_in_full(fd, buf, len) != len)
+ die_errno(_("could not write to %s"), path);
+ if (close(fd))
+ die_errno(_("could not close %s"), path);
+}
+
void write_file(const char *path, const char *fmt, ...)
{
va_list params;
struct strbuf sb = STRBUF_INIT;
- int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
va_start(params, fmt);
strbuf_vaddf(&sb, fmt, params);
va_end(params);
strbuf_complete_line(&sb);
- if (write_in_full(fd, sb.buf, sb.len) != sb.len)
- die_errno(_("could not write to %s"), path);
+
+ write_file_buf(path, sb.buf, sb.len);
strbuf_release(&sb);
- if (close(fd))
- die_errno(_("could not close %s"), path);
}
void sleep_millisec(int millisec)
From e04d08a4b3373972717c805ae8e788219b873b2a Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:12:42 -0400
Subject: [PATCH 7/9] write_file: add format attribute
This gives us compile-time checking of our format strings,
which is a good thing.
I had also hoped it would help with confusing write_file()
and write_file_buf(), since the former's "..." can make it
match the signature of the latter. But given that the buffer
for write_file_buf() is generally not a string literal, the
compiler won't complain unless -Wformat-nonliteral is on,
and that creates a ton of false positives elsewhere in the
code base.
While we're there, let's also give the function a docstring,
which it never had.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
cache.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/cache.h b/cache.h
index 206d594370f..b2d24da73f6 100644
--- a/cache.h
+++ b/cache.h
@@ -1740,6 +1740,14 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
*/
extern void write_file_buf(const char *path, const char *buf, size_t len);
+/**
+ * Like write_file_buf(), but format the contents into a buffer first.
+ * Additionally, write_file() will append a newline if one is not already
+ * present, making it convenient to write text files:
+ *
+ * write_file(path, "counter: %d", ctr);
+ */
+__attribute__((format (printf, 2, 3)))
extern void write_file(const char *path, const char *fmt, ...);
/* pager.c */
From e78d5d49935373dabcc40c5e32aefe4956a70b97 Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:12:55 -0400
Subject: [PATCH 8/9] use write_file_buf where applicable
There are several places where we open a file, write some
content from a strbuf, and close it. These can be simplified
with write_file_buf(). As a bonus, many of these did not
catch write problems at close() time.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/am.c | 7 +------
builtin/merge.c | 45 +++++----------------------------------------
2 files changed, 6 insertions(+), 46 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index a9742238767..5fdf5b4c242 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -402,13 +402,8 @@ static int read_commit_msg(struct am_state *state)
*/
static void write_commit_msg(const struct am_state *state)
{
- int fd;
const char *filename = am_path(state, "final-commit");
-
- fd = xopen(filename, O_WRONLY | O_CREAT, 0666);
- if (write_in_full(fd, state->msg, state->msg_len) < 0)
- die_errno(_("could not write to %s"), filename);
- close(fd);
+ write_file_buf(filename, state->msg, state->msg_len);
}
/**
diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1bf9cd..2e782db7b46 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -336,15 +336,9 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
struct rev_info rev;
struct strbuf out = STRBUF_INIT;
struct commit_list *j;
- const char *filename;
- int fd;
struct pretty_print_context ctx = {0};
printf(_("Squash commit -- not updating HEAD\n"));
- filename = git_path_squash_msg();
- fd = open(filename, O_WRONLY | O_CREAT, 0666);
- if (fd < 0)
- die_errno(_("Could not write to '%s'"), filename);
init_revisions(&rev, NULL);
rev.ignore_merges = 1;
@@ -371,10 +365,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
oid_to_hex(&commit->object.oid));
pretty_print_commit(&ctx, commit, &out);
}
- if (write_in_full(fd, out.buf, out.len) != out.len)
- die_errno(_("Writing SQUASH_MSG"));
- if (close(fd))
- die_errno(_("Finishing SQUASH_MSG"));
+ write_file_buf(git_path_squash_msg(), out.buf, out.len);
strbuf_release(&out);
}
@@ -756,18 +747,6 @@ static void add_strategies(const char *string, unsigned attr)
}
-static void write_merge_msg(struct strbuf *msg)
-{
- const char *filename = git_path_merge_msg();
- int fd = open(filename, O_WRONLY | O_CREAT, 0666);
- if (fd < 0)
- die_errno(_("Could not open '%s' for writing"),
- filename);
- if (write_in_full(fd, msg->buf, msg->len) != msg->len)
- die_errno(_("Could not write to '%s'"), filename);
- close(fd);
-}
-
static void read_merge_msg(struct strbuf *msg)
{
const char *filename = git_path_merge_msg();
@@ -801,7 +780,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
strbuf_addch(&msg, '\n');
if (0 < option_edit)
strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
- write_merge_msg(&msg);
+ write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
git_path_merge_msg(), "merge", NULL))
abort_commit(remoteheads, NULL);
@@ -964,8 +943,6 @@ static int setup_with_upstream(const char ***argv)
static void write_merge_state(struct commit_list *remoteheads)
{
- const char *filename;
- int fd;
struct commit_list *j;
struct strbuf buf = STRBUF_INIT;
@@ -979,26 +956,14 @@ static void write_merge_state(struct commit_list *remoteheads)
}
strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
}
- filename = git_path_merge_head();
- fd = open(filename, O_WRONLY | O_CREAT, 0666);
- if (fd < 0)
- die_errno(_("Could not open '%s' for writing"), filename);
- if (write_in_full(fd, buf.buf, buf.len) != buf.len)
- die_errno(_("Could not write to '%s'"), filename);
- close(fd);
+ write_file_buf(git_path_merge_head(), buf.buf, buf.len);
strbuf_addch(&merge_msg, '\n');
- write_merge_msg(&merge_msg);
+ write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
- filename = git_path_merge_mode();
- fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
- if (fd < 0)
- die_errno(_("Could not open '%s' for writing"), filename);
strbuf_reset(&buf);
if (fast_forward == FF_NO)
strbuf_addf(&buf, "no-ff");
- if (write_in_full(fd, buf.buf, buf.len) != buf.len)
- die_errno(_("Could not write to '%s'"), filename);
- close(fd);
+ write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
}
static int default_edit_option(void)
From 7eb6e10c9d7f43913615667740d1b22055cfba1f Mon Sep 17 00:00:00 2001
From: Jeff King <peff@peff.net>
Date: Fri, 8 Jul 2016 05:16:53 -0400
Subject: [PATCH 9/9] branch: use write_file_buf instead of write_file
If we already have a strbuf, then using write_file_buf is a
little nicer to read (no wondering whether "%s" will eat
your NULs), and it's more efficient (no extra formatting
step).
We don't care about the newline magic of write_file(), as we
have our own multi-line content.
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/branch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 15232c4a428..1d41251a9a8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -618,7 +618,7 @@ static int edit_branch_description(const char *branch_name)
" %s\n"
"Lines starting with '%c' will be stripped.\n",
branch_name, comment_line_char);
- write_file(git_path(edit_description), "%s", buf.buf);
+ write_file_buf(git_path(edit_description), buf.buf, buf.len);
strbuf_reset(&buf);
if (launch_editor(git_path(edit_description), &buf, NULL)) {
strbuf_release(&buf);