| Message ID | fd547aab491b24a47f683d717c6b77255ac16901.1577185374.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge: learn --autostash | expand |
Denton Liu <liu.denton@gmail.com> writes: > -static int read_oneliner(struct strbuf *buf, > - const char *path, int skip_if_empty) > +static int read_oneliner(struct strbuf *buf, const char *path, > + int skip_if_empty, int warn_nonexistence) I would have to say that this results in a rather poor API in the end, and also forces an awkward API evolution, and we need to be careful especially if we plan to make this an external API in a later step in the series. There is a topic in flight that introduces additional callsites of read_oneliner(). It is a selfish move that breaks codebase unnecessarily to unilaterally change the function signature. Two possible ways to improve are: * keep read_oneliner() with existing function signature working for other topics in flight, by renaming your extended variant and making read_oneliner() a thin wrapper that calls yours with the extended feature disabled (i.e. warn_nonexistence==0); or * update skip_if_empty that wastes an int parameter to pass only a single bit into an unsigned that is a collection of bits, after vetting that new callsites contemporary topics add always pass either 0 or 1 as the parameter. Then #define READ_ONELINER_SKIP_IF_EMPTY 01 #define READ_ONELINER_WARN_MISSING 02 static int read_oneliner(struct strbuf *buf, const char *path, unsigned flags) would transparently work well for those other topics. The latter probably is more preferrable, as the end result would be a cleaner API than the former or the one presented in this series. Thanks. > { > int ret = 0; > struct strbuf file_buf = STRBUF_INIT; > > - if (!file_exists(path)) > + if (!warn_nonexistence && !file_exists(path)) > return 0; > > if (strbuf_read_file(&file_buf, path, 0) < 0) { > @@ -2558,10 +2558,10 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) > static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) > { > strbuf_reset(buf); > - if (!read_oneliner(buf, rebase_path_strategy(), 0)) > + if (!read_oneliner(buf, rebase_path_strategy(), 0, 0)) > return; > opts->strategy = strbuf_detach(buf, NULL); > - if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) > + if (!read_oneliner(buf, rebase_path_strategy_opts(), 0, 0)) > return; > > parse_strategy_opts(opts, buf->buf); > @@ -2572,7 +2572,7 @@ static int read_populate_opts(struct replay_opts *opts) > if (is_rebase_i(opts)) { > struct strbuf buf = STRBUF_INIT; > > - if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) { > + if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1, 0)) { > if (!starts_with(buf.buf, "-S")) > strbuf_reset(&buf); > else { > @@ -2582,7 +2582,7 @@ static int read_populate_opts(struct replay_opts *opts) > strbuf_reset(&buf); > } > > - if (read_oneliner(&buf, rebase_path_allow_rerere_autoupdate(), 1)) { > + if (read_oneliner(&buf, rebase_path_allow_rerere_autoupdate(), 1, 0)) { > if (!strcmp(buf.buf, "--rerere-autoupdate")) > opts->allow_rerere_auto = RERERE_AUTOUPDATE; > else if (!strcmp(buf.buf, "--no-rerere-autoupdate")) > @@ -2618,7 +2618,7 @@ static int read_populate_opts(struct replay_opts *opts) > strbuf_release(&buf); > > if (read_oneliner(&opts->current_fixups, > - rebase_path_current_fixups(), 1)) { > + rebase_path_current_fixups(), 1, 0)) { > const char *p = opts->current_fixups.buf; > opts->current_fixup_count = 1; > while ((p = strchr(p, '\n'))) { > @@ -2627,7 +2627,7 @@ static int read_populate_opts(struct replay_opts *opts) > } > } > > - if (read_oneliner(&buf, rebase_path_squash_onto(), 0)) { > + if (read_oneliner(&buf, rebase_path_squash_onto(), 0, 0)) { > if (get_oid_hex(buf.buf, &opts->squash_onto) < 0) > return error(_("unusable squash-onto")); > opts->have_squash_onto = 1; > @@ -3759,7 +3759,7 @@ static int apply_autostash(struct replay_opts *opts) > struct child_process child = CHILD_PROCESS_INIT; > int ret = 0; > > - if (!read_oneliner(&stash_sha1, rebase_path_autostash(), 1)) { > + if (!read_oneliner(&stash_sha1, rebase_path_autostash(), 1, 0)) { > strbuf_release(&stash_sha1); > return 0; > } > @@ -4093,7 +4093,7 @@ static int pick_commits(struct repository *r, > if (todo_list->current < todo_list->nr) > return 0; > > - if (read_oneliner(&head_ref, rebase_path_head_name(), 0) && > + if (read_oneliner(&head_ref, rebase_path_head_name(), 0, 0) && > starts_with(head_ref.buf, "refs/")) { > const char *msg; > struct object_id head, orig; > @@ -4106,13 +4106,13 @@ static int pick_commits(struct repository *r, > strbuf_release(&buf); > return res; > } > - if (!read_oneliner(&buf, rebase_path_orig_head(), 0) || > + if (!read_oneliner(&buf, rebase_path_orig_head(), 0, 0) || > get_oid_hex(buf.buf, &orig)) { > res = error(_("could not read orig-head")); > goto cleanup_head_ref; > } > strbuf_reset(&buf); > - if (!read_oneliner(&buf, rebase_path_onto(), 0)) { > + if (!read_oneliner(&buf, rebase_path_onto(), 0, 0)) { > res = error(_("could not read 'onto'")); > goto cleanup_head_ref; > } > @@ -4145,7 +4145,7 @@ static int pick_commits(struct repository *r, > DIFF_FORMAT_DIFFSTAT; > log_tree_opt.disable_stdin = 1; > > - if (read_oneliner(&buf, rebase_path_orig_head(), 0) && > + if (read_oneliner(&buf, rebase_path_orig_head(), 0, 0) && > !get_oid(buf.buf, &orig) && > !get_oid("HEAD", &head)) { > diff_tree_oid(&orig, &head, "", > @@ -4230,7 +4230,7 @@ static int commit_staged_changes(struct repository *r, > > if (get_oid("HEAD", &head)) > return error(_("cannot amend non-existing commit")); > - if (!read_oneliner(&rev, rebase_path_amend(), 0)) > + if (!read_oneliner(&rev, rebase_path_amend(), 0, 0)) > return error(_("invalid file: '%s'"), rebase_path_amend()); > if (get_oid_hex(rev.buf, &to_amend)) > return error(_("invalid contents: '%s'"), > @@ -4391,7 +4391,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) > struct strbuf buf = STRBUF_INIT; > struct object_id oid; > > - if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) && > + if (read_oneliner(&buf, rebase_path_stopped_sha(), 1, 0) && > !get_oid_committish(buf.buf, &oid)) > record_in_rewritten(&oid, peek_command(&todo_list, 0)); > strbuf_release(&buf);
diff --git a/sequencer.c b/sequencer.c index 5ba8b4383f..103cea1460 100644 --- a/sequencer.c +++ b/sequencer.c @@ -426,13 +426,13 @@ static int write_message(const void *buf, size_t len, const char *filename, * * Returns 1 if the file was read, 0 if it could not be read or does not exist. */ -static int read_oneliner(struct strbuf *buf, - const char *path, int skip_if_empty) +static int read_oneliner(struct strbuf *buf, const char *path, + int skip_if_empty, int warn_nonexistence) { int ret = 0; struct strbuf file_buf = STRBUF_INIT; - if (!file_exists(path)) + if (!warn_nonexistence && !file_exists(path)) return 0; if (strbuf_read_file(&file_buf, path, 0) < 0) { @@ -2558,10 +2558,10 @@ void parse_strategy_opts(struct replay_opts *opts, char *raw_opts) static void read_strategy_opts(struct replay_opts *opts, struct strbuf *buf) { strbuf_reset(buf); - if (!read_oneliner(buf, rebase_path_strategy(), 0)) + if (!read_oneliner(buf, rebase_path_strategy(), 0, 0)) return; opts->strategy = strbuf_detach(buf, NULL); - if (!read_oneliner(buf, rebase_path_strategy_opts(), 0)) + if (!read_oneliner(buf, rebase_path_strategy_opts(), 0, 0)) return; parse_strategy_opts(opts, buf->buf); @@ -2572,7 +2572,7 @@ static int read_populate_opts(struct replay_opts *opts) if (is_rebase_i(opts)) { struct strbuf buf = STRBUF_INIT; - if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1)) { + if (read_oneliner(&buf, rebase_path_gpg_sign_opt(), 1, 0)) { if (!starts_with(buf.buf, "-S")) strbuf_reset(&buf); else { @@ -2582,7 +2582,7 @@ static int read_populate_opts(struct replay_opts *opts) strbuf_reset(&buf); } - if (read_oneliner(&buf, rebase_path_allow_rerere_autoupdate(), 1)) { + if (read_oneliner(&buf, rebase_path_allow_rerere_autoupdate(), 1, 0)) { if (!strcmp(buf.buf, "--rerere-autoupdate")) opts->allow_rerere_auto = RERERE_AUTOUPDATE; else if (!strcmp(buf.buf, "--no-rerere-autoupdate")) @@ -2618,7 +2618,7 @@ static int read_populate_opts(struct replay_opts *opts) strbuf_release(&buf); if (read_oneliner(&opts->current_fixups, - rebase_path_current_fixups(), 1)) { + rebase_path_current_fixups(), 1, 0)) { const char *p = opts->current_fixups.buf; opts->current_fixup_count = 1; while ((p = strchr(p, '\n'))) { @@ -2627,7 +2627,7 @@ static int read_populate_opts(struct replay_opts *opts) } } - if (read_oneliner(&buf, rebase_path_squash_onto(), 0)) { + if (read_oneliner(&buf, rebase_path_squash_onto(), 0, 0)) { if (get_oid_hex(buf.buf, &opts->squash_onto) < 0) return error(_("unusable squash-onto")); opts->have_squash_onto = 1; @@ -3759,7 +3759,7 @@ static int apply_autostash(struct replay_opts *opts) struct child_process child = CHILD_PROCESS_INIT; int ret = 0; - if (!read_oneliner(&stash_sha1, rebase_path_autostash(), 1)) { + if (!read_oneliner(&stash_sha1, rebase_path_autostash(), 1, 0)) { strbuf_release(&stash_sha1); return 0; } @@ -4093,7 +4093,7 @@ static int pick_commits(struct repository *r, if (todo_list->current < todo_list->nr) return 0; - if (read_oneliner(&head_ref, rebase_path_head_name(), 0) && + if (read_oneliner(&head_ref, rebase_path_head_name(), 0, 0) && starts_with(head_ref.buf, "refs/")) { const char *msg; struct object_id head, orig; @@ -4106,13 +4106,13 @@ static int pick_commits(struct repository *r, strbuf_release(&buf); return res; } - if (!read_oneliner(&buf, rebase_path_orig_head(), 0) || + if (!read_oneliner(&buf, rebase_path_orig_head(), 0, 0) || get_oid_hex(buf.buf, &orig)) { res = error(_("could not read orig-head")); goto cleanup_head_ref; } strbuf_reset(&buf); - if (!read_oneliner(&buf, rebase_path_onto(), 0)) { + if (!read_oneliner(&buf, rebase_path_onto(), 0, 0)) { res = error(_("could not read 'onto'")); goto cleanup_head_ref; } @@ -4145,7 +4145,7 @@ static int pick_commits(struct repository *r, DIFF_FORMAT_DIFFSTAT; log_tree_opt.disable_stdin = 1; - if (read_oneliner(&buf, rebase_path_orig_head(), 0) && + if (read_oneliner(&buf, rebase_path_orig_head(), 0, 0) && !get_oid(buf.buf, &orig) && !get_oid("HEAD", &head)) { diff_tree_oid(&orig, &head, "", @@ -4230,7 +4230,7 @@ static int commit_staged_changes(struct repository *r, if (get_oid("HEAD", &head)) return error(_("cannot amend non-existing commit")); - if (!read_oneliner(&rev, rebase_path_amend(), 0)) + if (!read_oneliner(&rev, rebase_path_amend(), 0, 0)) return error(_("invalid file: '%s'"), rebase_path_amend()); if (get_oid_hex(rev.buf, &to_amend)) return error(_("invalid contents: '%s'"), @@ -4391,7 +4391,7 @@ int sequencer_continue(struct repository *r, struct replay_opts *opts) struct strbuf buf = STRBUF_INIT; struct object_id oid; - if (read_oneliner(&buf, rebase_path_stopped_sha(), 1) && + if (read_oneliner(&buf, rebase_path_stopped_sha(), 1, 0) && !get_oid_committish(buf.buf, &oid)) record_in_rewritten(&oid, peek_command(&todo_list, 0)); strbuf_release(&buf);
In the future, we plan on externing read_oneliner(). Future users of read_oneliner() will want the ability to output warnings in the event that the `path` doesn't exist. Introduce the `warn_nonexistence` parameter which, if true, would issue a warning when a file doesn't exist by skipping the `!file_exists()` check and letting `strbuf_read_file()` handle that case. Mechanically replace uses of read_oneliner() in sequencer.c with the following spatch so that current behaviour is maintained: @@ expression a, b, c; @@ read_oneliner(a, b, c + , 0 ) Signed-off-by: Denton Liu <liu.denton@gmail.com> --- sequencer.c | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)