From aabbd3f3c9dd17c414100d0e029f8a3d13673ab1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 8 Jul 2016 05:06:50 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/config.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/config.c b/builtin/config.c index 1d7c6ef55..a991a5341 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); -- cgit v1.2.1 From 1dad879a7b7b41381cf4ccf471dcab06993a131b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Fri, 8 Jul 2016 05:08:05 -0400 Subject: 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 Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/am.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) (limited to 'builtin') diff --git a/builtin/am.c b/builtin/am.c index 3dfe70b7a..a97422387 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, - const char *name, int value) +static void write_state_count(const struct am_state *state, + const char *name, int value) { - return write_file(am_path(state, name), "%d", value); + write_file(am_path(state, name), "%d", value); } -static int write_state_bool(const struct am_state *state, - const char *name, int value) +static void 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"); } /** -- cgit v1.2.1 From 3d75bba28d8665ec43f1faffce38d5817c77ebe8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 8 Jul 2016 05:08:54 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/branch.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index 2ecde53bf..15232c4a4 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); -- cgit v1.2.1 From e78d5d49935373dabcc40c5e32aefe4956a70b97 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 8 Jul 2016 05:12:55 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/am.c | 7 +------ builtin/merge.c | 45 +++++---------------------------------------- 2 files changed, 6 insertions(+), 46 deletions(-) (limited to 'builtin') diff --git a/builtin/am.c b/builtin/am.c index a97422387..5fdf5b4c2 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 b555a1bf9..2e782db7b 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) -- cgit v1.2.1 From 7eb6e10c9d7f43913615667740d1b22055cfba1f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 8 Jul 2016 05:16:53 -0400 Subject: 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 Signed-off-by: Junio C Hamano --- builtin/branch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/branch.c b/builtin/branch.c index 15232c4a4..1d41251a9 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); -- cgit v1.2.1