From 25b763ba7a544d05fc46ad601f3f9c50a4cea8b0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 24 Aug 2015 09:12:53 -0700 Subject: [PATCH 1/5] builtin/am: introduce write_state_*() helper functions There are many calls to write_file() that repeat the same pattern in the implementation of the builtin version of "am". They all share the same traits, i.e they - produce a text file with a single string in it; - have enough information to produce the entire contents of that file; - generate the pathname of the file by making a call to am_path(); and - they ask write_file() to die() upon failure. The slight differences among the call sites throw them into roughly three categories: - many write either "t" or "f" based on a boolean value to a file; - some write the integer value in decimal text; - some others write more general string, e.g. an object name in hex, an empty string (i.e. the presense of the file itself serves as a flag), etc. Introduce three helpers, write_state_bool(), write_state_count() and write_state_text(), to reduce direct calls to write_file(). This is a preparatory step for the next step to ensure that no "state" file this command leaves in $GIT_DIR is with an incomplete line at the end. Suggested-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/am.c | 68 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 634f7a7aa7..4d34dc5f74 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -193,6 +193,27 @@ static inline const char *am_path(const struct am_state *state, const char *path return mkpath("%s/%s", state->dir, path); } +/** + * For convenience to call write_file() + */ +static int write_state_text(const struct am_state *state, + const char *name, const char *string) +{ + return write_file(am_path(state, name), 1, "%s", string); +} + +static int write_state_count(const struct am_state *state, + const char *name, int value) +{ + return write_file(am_path(state, name), 1, "%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"); +} + /** * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline * at the end. @@ -362,7 +383,7 @@ static void write_author_script(const struct am_state *state) sq_quote_buf(&sb, state->author_date); strbuf_addch(&sb, '\n'); - write_file(am_path(state, "author-script"), 1, "%s", sb.buf); + write_state_text(state, "author-script", sb.buf); strbuf_release(&sb); } @@ -1000,13 +1021,10 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, if (state->rebasing) state->threeway = 1; - write_file(am_path(state, "threeway"), 1, state->threeway ? "t" : "f"); - - write_file(am_path(state, "quiet"), 1, state->quiet ? "t" : "f"); - - write_file(am_path(state, "sign"), 1, state->signoff ? "t" : "f"); - - write_file(am_path(state, "utf8"), 1, state->utf8 ? "t" : "f"); + write_state_bool(state, "threeway", state->threeway); + write_state_bool(state, "quiet", state->quiet); + write_state_bool(state, "sign", state->signoff); + write_state_bool(state, "utf8", state->utf8); switch (state->keep) { case KEEP_FALSE: @@ -1022,9 +1040,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, die("BUG: invalid value for state->keep"); } - write_file(am_path(state, "keep"), 1, "%s", str); - - write_file(am_path(state, "messageid"), 1, state->message_id ? "t" : "f"); + write_state_text(state, "keep", str); + write_state_bool(state, "messageid", state->message_id); switch (state->scissors) { case SCISSORS_UNSET: @@ -1039,24 +1056,23 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, default: die("BUG: invalid value for state->scissors"); } - - write_file(am_path(state, "scissors"), 1, "%s", str); + write_state_text(state, "scissors", str); sq_quote_argv(&sb, state->git_apply_opts.argv, 0); - write_file(am_path(state, "apply-opt"), 1, "%s", sb.buf); + write_state_text(state, "apply-opt", sb.buf); if (state->rebasing) - write_file(am_path(state, "rebasing"), 1, "%s", ""); + write_state_text(state, "rebasing", ""); else - write_file(am_path(state, "applying"), 1, "%s", ""); + write_state_text(state, "applying", ""); if (!get_sha1("HEAD", curr_head)) { - write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(curr_head)); + write_state_text(state, "abort-safety", sha1_to_hex(curr_head)); if (!state->rebasing) update_ref("am", "ORIG_HEAD", curr_head, NULL, 0, UPDATE_REFS_DIE_ON_ERR); } else { - write_file(am_path(state, "abort-safety"), 1, "%s", ""); + write_state_text(state, "abort-safety", ""); if (!state->rebasing) delete_ref("ORIG_HEAD", NULL, 0); } @@ -1066,9 +1082,8 @@ static void am_setup(struct am_state *state, enum patch_format patch_format, * session is in progress, they should be written last. */ - write_file(am_path(state, "next"), 1, "%d", state->cur); - - write_file(am_path(state, "last"), 1, "%d", state->last); + write_state_count(state, "next", state->cur); + write_state_count(state, "last", state->last); strbuf_release(&sb); } @@ -1101,12 +1116,12 @@ static void am_next(struct am_state *state) unlink(am_path(state, "original-commit")); if (!get_sha1("HEAD", head)) - write_file(am_path(state, "abort-safety"), 1, "%s", sha1_to_hex(head)); + write_state_text(state, "abort-safety", sha1_to_hex(head)); else - write_file(am_path(state, "abort-safety"), 1, "%s", ""); + write_state_text(state, "abort-safety", ""); state->cur++; - write_file(am_path(state, "next"), 1, "%d", state->cur); + write_state_count(state, "next", state->cur); } /** @@ -1479,8 +1494,7 @@ static int parse_mail_rebase(struct am_state *state, const char *mail) write_commit_patch(state, commit); hashcpy(state->orig_commit, commit_sha1); - write_file(am_path(state, "original-commit"), 1, "%s", - sha1_to_hex(commit_sha1)); + write_state_text(state, "original-commit", sha1_to_hex(commit_sha1)); return 0; } @@ -1782,7 +1796,7 @@ static void am_run(struct am_state *state, int resume) refresh_and_write_cache(); if (index_has_changes(&sb)) { - write_file(am_path(state, "dirtyindex"), 1, "t"); + write_state_bool(state, "dirtyindex", 1); die(_("Dirty index: cannot apply patches (dirty: %s)"), sb.buf); } From 57c867efe4e005e40cfdee8a64550d7a95bbb9a0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 24 Aug 2015 09:43:41 -0700 Subject: [PATCH 2/5] builtin/am: make sure state files are text We forgot to terminate the payload given to write_file() with LF, resulting in files that end with an incomplete line. Teach the wrappers builtin/am uses to make sure it adds LF at the end as necessary. Signed-off-by: Junio C Hamano --- builtin/am.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 4d34dc5f74..f0a046bdc0 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -199,13 +199,19 @@ static inline const char *am_path(const struct am_state *state, const char *path static int write_state_text(const struct am_state *state, const char *name, const char *string) { - return write_file(am_path(state, name), 1, "%s", string); + const char *fmt; + + if (*string && string[strlen(string) - 1] != '\n') + fmt = "%s\n"; + else + fmt = "%s"; + return write_file(am_path(state, name), 1, fmt, string); } static int write_state_count(const struct am_state *state, const char *name, int value) { - return write_file(am_path(state, name), 1, "%d", value); + return write_file(am_path(state, name), 1, "%d\n", value); } static int write_state_bool(const struct am_state *state, From 12d6ce1dba504dfc5279b8d24da3edb4865c2820 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 24 Aug 2015 13:03:07 -0700 Subject: [PATCH 3/5] write_file(): drop "fatal" parameter All callers except three passed 1 for the "fatal" parameter to ask this function to die upon error, but to a casual reader of the code, it was not all obvious what that 1 meant. Instead, split the function into two based on a common write_file_v() that takes the flag, introduce write_file_gently() as a new way to attempt creating a file without dying on error, and make three callers to call it. Signed-off-by: Junio C Hamano --- builtin/am.c | 4 ++-- builtin/branch.c | 2 +- builtin/init-db.c | 2 +- builtin/worktree.c | 10 +++++----- cache.h | 5 +++-- daemon.c | 2 +- setup.c | 2 +- submodule.c | 2 +- transport.c | 2 +- wrapper.c | 28 ++++++++++++++++++++++++---- 10 files changed, 40 insertions(+), 19 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index f0a046bdc0..9c576779c3 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -205,13 +205,13 @@ static int write_state_text(const struct am_state *state, fmt = "%s\n"; else fmt = "%s"; - return write_file(am_path(state, name), 1, fmt, string); + return write_file(am_path(state, name), fmt, string); } static int write_state_count(const struct am_state *state, const char *name, int value) { - return write_file(am_path(state, name), 1, "%d\n", value); + return write_file(am_path(state, name), "%d\n", value); } static int write_state_bool(const struct am_state *state, diff --git a/builtin/branch.c b/builtin/branch.c index 58aa84f1e8..ff05869949 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -776,7 +776,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(git_path(edit_description), 0, "%s", buf.buf)) { + if (write_file_gently(git_path(edit_description), "%s", buf.buf)) { strbuf_release(&buf); return error(_("could not write branch description template: %s"), strerror(errno)); diff --git a/builtin/init-db.c b/builtin/init-db.c index 49df78d262..bfe1d08234 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir) die_errno(_("unable to move %s to %s"), src, git_dir); } - write_file(git_link, 1, "gitdir: %s\n", git_dir); + write_file(git_link, "gitdir: %s\n", git_dir); } int init_db(const char *template_dir, unsigned int flags) diff --git a/builtin/worktree.c b/builtin/worktree.c index 6a264ee749..368502d837 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv) * after the preparation is over. */ strbuf_addf(&sb, "%s/locked", sb_repo.buf); - write_file(sb.buf, 1, "initializing\n"); + write_file(sb.buf, "initializing\n"); strbuf_addf(&sb_git, "%s/.git", path); if (safe_create_leading_directories_const(sb_git.buf)) @@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv) strbuf_reset(&sb); strbuf_addf(&sb, "%s/gitdir", sb_repo.buf); - write_file(sb.buf, 1, "%s\n", real_path(sb_git.buf)); - write_file(sb_git.buf, 1, "gitdir: %s/worktrees/%s\n", + write_file(sb.buf, "%s\n", real_path(sb_git.buf)); + write_file(sb_git.buf, "gitdir: %s/worktrees/%s\n", real_path(get_git_common_dir()), name); /* * This is to keep resolve_ref() happy. We need a valid HEAD @@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv) die(_("unable to resolve HEAD")); strbuf_reset(&sb); strbuf_addf(&sb, "%s/HEAD", sb_repo.buf); - write_file(sb.buf, 1, "%s\n", sha1_to_hex(rev)); + write_file(sb.buf, "%s\n", sha1_to_hex(rev)); strbuf_reset(&sb); strbuf_addf(&sb, "%s/commondir", sb_repo.buf); - write_file(sb.buf, 1, "../..\n"); + write_file(sb.buf, "../..\n"); fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name); diff --git a/cache.h b/cache.h index 6bb7119032..3f79e6b616 100644 --- a/cache.h +++ b/cache.h @@ -1539,8 +1539,9 @@ static inline ssize_t write_str_in_full(int fd, const char *str) { return write_in_full(fd, str, strlen(str)); } -__attribute__((format (printf, 3, 4))) -extern int write_file(const char *path, int fatal, const char *fmt, ...); + +extern int write_file(const char *path, const char *fmt, ...); +extern int write_file_gently(const char *path, const char *fmt, ...); /* pager.c */ extern void setup_pager(void); diff --git a/daemon.c b/daemon.c index d3d3e433e3..91545090a7 100644 --- a/daemon.c +++ b/daemon.c @@ -1376,7 +1376,7 @@ int main(int argc, char **argv) sanitize_stdfds(); if (pid_file) - write_file(pid_file, 1, "%"PRIuMAX"\n", (uintmax_t) getpid()); + write_file(pid_file, "%"PRIuMAX"\n", (uintmax_t) getpid()); /* prepare argv for serving-processes */ cld_argv = xmalloc(sizeof (char *) * (argc + 2)); diff --git a/setup.c b/setup.c index 5f9f07dcdb..feb85651ef 100644 --- a/setup.c +++ b/setup.c @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir) strbuf_addf(&path, "%s/gitfile", gitdir); if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL)) - write_file(path.buf, 0, "%s\n", gitfile); + write_file_gently(path.buf, "%s\n", gitfile); strbuf_release(&path); } diff --git a/submodule.c b/submodule.c index 700bbf4fcb..5519f11fdb 100644 --- a/submodule.c +++ b/submodule.c @@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) /* Update gitfile */ strbuf_addf(&file_name, "%s/.git", work_tree); - write_file(file_name.buf, 1, "gitdir: %s\n", + write_file(file_name.buf, "gitdir: %s\n", relative_path(git_dir, real_work_tree, &rel_path)); /* Update core.worktree setting */ diff --git a/transport.c b/transport.c index 40692f8ae8..0254394356 100644 --- a/transport.c +++ b/transport.c @@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid, strbuf_addstr(buf, name); if (safe_create_leading_directories(buf->buf) || - write_file(buf->buf, 0, "%s\n", oid_to_hex(oid))) + write_file_gently(buf->buf, "%s\n", oid_to_hex(oid))) return error("problems writing temporary file %s: %s", buf->buf, strerror(errno)); strbuf_setlen(buf, len); diff --git a/wrapper.c b/wrapper.c index e451463431..8c8925b72a 100644 --- a/wrapper.c +++ b/wrapper.c @@ -621,19 +621,17 @@ char *xgetcwd(void) return strbuf_detach(&sb, NULL); } -int write_file(const char *path, int fatal, const char *fmt, ...) +static int write_file_v(const char *path, int fatal, + const char *fmt, va_list params) { struct strbuf sb = STRBUF_INIT; - va_list params; 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; } - va_start(params, fmt); strbuf_vaddf(&sb, fmt, params); - va_end(params); if (write_in_full(fd, sb.buf, sb.len) != sb.len) { int err = errno; close(fd); @@ -652,6 +650,28 @@ int write_file(const char *path, int fatal, const char *fmt, ...) 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; +} + void sleep_millisec(int millisec) { poll(NULL, 0, millisec); From e7ffa38c6e726e8014b76297b06f78e008deb2d0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 24 Aug 2015 09:39:48 -0700 Subject: [PATCH 4/5] write_file_v(): do not leave incomplete line at the end All existing callers to this function use it to produce a text file or an empty file, and a new callsite that mimick them must end their payload with a LF. If they forget to do so, the resulting file will end with an incomplete line. Teach write_file_v() to complete the incomplete line, if exists, so that the callers do not have to. With this, the caller-side fix in builtin/am.c becomes unnecessary. Signed-off-by: Junio C Hamano --- builtin/am.c | 10 ++-------- wrapper.c | 1 + 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/builtin/am.c b/builtin/am.c index 9c576779c3..486ff594d7 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -199,19 +199,13 @@ static inline const char *am_path(const struct am_state *state, const char *path static int write_state_text(const struct am_state *state, const char *name, const char *string) { - const char *fmt; - - if (*string && string[strlen(string) - 1] != '\n') - fmt = "%s\n"; - else - fmt = "%s"; - return write_file(am_path(state, name), fmt, string); + return write_file(am_path(state, name), "%s", string); } static int write_state_count(const struct am_state *state, const char *name, int value) { - return write_file(am_path(state, name), "%d\n", value); + return write_file(am_path(state, name), "%d", value); } static int write_state_bool(const struct am_state *state, diff --git a/wrapper.c b/wrapper.c index 8c8925b72a..0e22d43814 100644 --- a/wrapper.c +++ b/wrapper.c @@ -632,6 +632,7 @@ static int write_file_v(const char *path, int fatal, return -1; } strbuf_vaddf(&sb, fmt, params); + strbuf_complete_line(&sb); if (write_in_full(fd, sb.buf, sb.len) != sb.len) { int err = errno; close(fd); From 1f76a10b2d72245332ac41bd79249cb82f3946f0 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Mon, 24 Aug 2015 13:20:39 -0700 Subject: [PATCH 5/5] write_file(): drop caller-supplied LF from calls to create a one-liner file All of the callsites covered by this change call write_file() or write_file_gently() to create a one-liner file. Drop the caller supplied LF and let these callees to append it as necessary. Signed-off-by: Junio C Hamano --- builtin/init-db.c | 2 +- builtin/worktree.c | 10 +++++----- daemon.c | 2 +- setup.c | 2 +- submodule.c | 2 +- transport.c | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/builtin/init-db.c b/builtin/init-db.c index bfe1d08234..69323e186c 100644 --- a/builtin/init-db.c +++ b/builtin/init-db.c @@ -378,7 +378,7 @@ static void separate_git_dir(const char *git_dir) die_errno(_("unable to move %s to %s"), src, git_dir); } - write_file(git_link, "gitdir: %s\n", git_dir); + write_file(git_link, "gitdir: %s", git_dir); } int init_db(const char *template_dir, unsigned int flags) diff --git a/builtin/worktree.c b/builtin/worktree.c index 368502d837..bbb169a5d0 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -213,7 +213,7 @@ static int add_worktree(const char *path, const char **child_argv) * after the preparation is over. */ strbuf_addf(&sb, "%s/locked", sb_repo.buf); - write_file(sb.buf, "initializing\n"); + write_file(sb.buf, "initializing"); strbuf_addf(&sb_git, "%s/.git", path); if (safe_create_leading_directories_const(sb_git.buf)) @@ -223,8 +223,8 @@ static int add_worktree(const char *path, const char **child_argv) strbuf_reset(&sb); strbuf_addf(&sb, "%s/gitdir", sb_repo.buf); - write_file(sb.buf, "%s\n", real_path(sb_git.buf)); - write_file(sb_git.buf, "gitdir: %s/worktrees/%s\n", + write_file(sb.buf, "%s", real_path(sb_git.buf)); + write_file(sb_git.buf, "gitdir: %s/worktrees/%s", real_path(get_git_common_dir()), name); /* * This is to keep resolve_ref() happy. We need a valid HEAD @@ -241,10 +241,10 @@ static int add_worktree(const char *path, const char **child_argv) die(_("unable to resolve HEAD")); strbuf_reset(&sb); strbuf_addf(&sb, "%s/HEAD", sb_repo.buf); - write_file(sb.buf, "%s\n", sha1_to_hex(rev)); + write_file(sb.buf, "%s", sha1_to_hex(rev)); strbuf_reset(&sb); strbuf_addf(&sb, "%s/commondir", sb_repo.buf); - write_file(sb.buf, "../..\n"); + write_file(sb.buf, "../.."); fprintf_ln(stderr, _("Enter %s (identifier %s)"), path, name); diff --git a/daemon.c b/daemon.c index 91545090a7..f9eb296888 100644 --- a/daemon.c +++ b/daemon.c @@ -1376,7 +1376,7 @@ int main(int argc, char **argv) sanitize_stdfds(); if (pid_file) - write_file(pid_file, "%"PRIuMAX"\n", (uintmax_t) getpid()); + write_file(pid_file, "%"PRIuMAX, (uintmax_t) getpid()); /* prepare argv for serving-processes */ cld_argv = xmalloc(sizeof (char *) * (argc + 2)); diff --git a/setup.c b/setup.c index feb85651ef..a206781d58 100644 --- a/setup.c +++ b/setup.c @@ -404,7 +404,7 @@ static void update_linked_gitdir(const char *gitfile, const char *gitdir) strbuf_addf(&path, "%s/gitfile", gitdir); if (stat(path.buf, &st) || st.st_mtime + 24 * 3600 < time(NULL)) - write_file_gently(path.buf, "%s\n", gitfile); + write_file_gently(path.buf, "%s", gitfile); strbuf_release(&path); } diff --git a/submodule.c b/submodule.c index 5519f11fdb..4549c1baf4 100644 --- a/submodule.c +++ b/submodule.c @@ -1103,7 +1103,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) /* Update gitfile */ strbuf_addf(&file_name, "%s/.git", work_tree); - write_file(file_name.buf, "gitdir: %s\n", + write_file(file_name.buf, "gitdir: %s", relative_path(git_dir, real_work_tree, &rel_path)); /* Update core.worktree setting */ diff --git a/transport.c b/transport.c index 0254394356..788cf20585 100644 --- a/transport.c +++ b/transport.c @@ -291,7 +291,7 @@ static int write_one_ref(const char *name, const struct object_id *oid, strbuf_addstr(buf, name); if (safe_create_leading_directories(buf->buf) || - write_file_gently(buf->buf, "%s\n", oid_to_hex(oid))) + write_file_gently(buf->buf, "%s", oid_to_hex(oid))) return error("problems writing temporary file %s: %s", buf->buf, strerror(errno)); strbuf_setlen(buf, len);