From fef461ea5d53dd84c6d946f57a018ffc9f391a05 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 25 Apr 2018 11:54:04 +0200 Subject: commit: Let the callback of for_each_mergetag return on error This is yet another patch to be filed under the keyword "libification". There is one subtle change in behavior here, where a `git log` that has been asked to show the mergetags would now stop reporting the mergetags upon the first failure, whereas previously, it would have continued to the next mergetag, if any. In practice, that change should not matter, as it is 1) uncommon to perform octopus merges using multiple tags as merge heads, and 2) when the user asks to be shown those tags, they really should be there. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/replace.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'builtin/replace.c') diff --git a/builtin/replace.c b/builtin/replace.c index 935647be6b..245d3f4164 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -345,7 +345,7 @@ struct check_mergetag_data { const char **argv; }; -static void check_one_mergetag(struct commit *commit, +static int check_one_mergetag(struct commit *commit, struct commit_extra_header *extra, void *data) { @@ -368,20 +368,20 @@ static void check_one_mergetag(struct commit *commit, if (get_oid(mergetag_data->argv[i], &oid) < 0) die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]); if (!oidcmp(&tag->tagged->oid, &oid)) - return; /* found */ + return 0; /* found */ } die(_("original commit '%s' contains mergetag '%s' that is discarded; " "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid)); } -static void check_mergetags(struct commit *commit, int argc, const char **argv) +static int check_mergetags(struct commit *commit, int argc, const char **argv) { struct check_mergetag_data mergetag_data; mergetag_data.argc = argc; mergetag_data.argv = argv; - for_each_mergetag(check_one_mergetag, commit, &mergetag_data); + return for_each_mergetag(check_one_mergetag, commit, &mergetag_data); } static int create_graft(int argc, const char **argv, int force) -- cgit v1.2.3 From d398f2ea001ab4e639896c81fb616810cf14eb12 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 25 Apr 2018 11:54:06 +0200 Subject: replace: avoid using die() to indicate a bug We have the BUG() macro for that purpose. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/replace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'builtin/replace.c') diff --git a/builtin/replace.c b/builtin/replace.c index 245d3f4164..e345a5a0f1 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -501,6 +501,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix) return list_replace_refs(argv[0], format); default: - die("BUG: invalid cmdmode %d", (int)cmdmode); + BUG("invalid cmdmode %d", (int)cmdmode); } } -- cgit v1.2.3 From e24e871920ac04473a470645e4c4dc53b422dd75 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 29 Apr 2018 00:44:26 +0200 Subject: replace: "libify" create_graft() and callees File this away as yet another patch in the "libification" category. As with all useful functions, in the next commit we want to use create_graft() from a higher-level function where it would be inconvenient if the called function simply die()s: if there is a problem, we want to let the user know how to proceed, and the callee simply has no way of knowing what to say. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/replace.c | 169 ++++++++++++++++++++++++++++++++++++------------------ 1 file changed, 112 insertions(+), 57 deletions(-) (limited to 'builtin/replace.c') diff --git a/builtin/replace.c b/builtin/replace.c index e345a5a0f1..e57d3d187e 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -79,9 +79,9 @@ static int list_replace_refs(const char *pattern, const char *format) else if (!strcmp(format, "long")) data.format = REPLACE_FORMAT_LONG; else - die("invalid replace format '%s'\n" - "valid formats are 'short', 'medium' and 'long'\n", - format); + return error("invalid replace format '%s'\n" + "valid formats are 'short', 'medium' and 'long'\n", + format); for_each_replace_ref(show_reference, (void *)&data); @@ -134,7 +134,7 @@ static int delete_replace_ref(const char *name, const char *ref, return 0; } -static void check_ref_valid(struct object_id *object, +static int check_ref_valid(struct object_id *object, struct object_id *prev, struct strbuf *ref, int force) @@ -142,12 +142,13 @@ static void check_ref_valid(struct object_id *object, strbuf_reset(ref); strbuf_addf(ref, "%s%s", git_replace_ref_base, oid_to_hex(object)); if (check_refname_format(ref->buf, 0)) - die("'%s' is not a valid ref name.", ref->buf); + return error("'%s' is not a valid ref name.", ref->buf); if (read_ref(ref->buf, prev)) oidclr(prev); else if (!force) - die("replace ref '%s' already exists", ref->buf); + return error("replace ref '%s' already exists", ref->buf); + return 0; } static int replace_object_oid(const char *object_ref, @@ -161,28 +162,33 @@ static int replace_object_oid(const char *object_ref, struct strbuf ref = STRBUF_INIT; struct ref_transaction *transaction; struct strbuf err = STRBUF_INIT; + int res = 0; obj_type = oid_object_info(object, NULL); repl_type = oid_object_info(repl, NULL); if (!force && obj_type != repl_type) - die("Objects must be of the same type.\n" - "'%s' points to a replaced object of type '%s'\n" - "while '%s' points to a replacement object of type '%s'.", - object_ref, type_name(obj_type), - replace_ref, type_name(repl_type)); - - check_ref_valid(object, &prev, &ref, force); + return error("Objects must be of the same type.\n" + "'%s' points to a replaced object of type '%s'\n" + "while '%s' points to a replacement object of " + "type '%s'.", + object_ref, type_name(obj_type), + replace_ref, type_name(repl_type)); + + if (check_ref_valid(object, &prev, &ref, force)) { + strbuf_release(&ref); + return -1; + } transaction = ref_transaction_begin(&err); if (!transaction || ref_transaction_update(transaction, ref.buf, repl, &prev, 0, NULL, &err) || ref_transaction_commit(transaction, &err)) - die("%s", err.buf); + res = error("%s", err.buf); ref_transaction_free(transaction); strbuf_release(&ref); - return 0; + return res; } static int replace_object(const char *object_ref, const char *replace_ref, int force) @@ -190,9 +196,11 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f struct object_id object, repl; if (get_oid(object_ref, &object)) - die("Failed to resolve '%s' as a valid ref.", object_ref); + return error("Failed to resolve '%s' as a valid ref.", + object_ref); if (get_oid(replace_ref, &repl)) - die("Failed to resolve '%s' as a valid ref.", replace_ref); + return error("Failed to resolve '%s' as a valid ref.", + replace_ref); return replace_object_oid(object_ref, &object, replace_ref, &repl, force); } @@ -202,7 +210,7 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f * If "raw" is true, then the object's raw contents are printed according to * "type". Otherwise, we pretty-print the contents for human editing. */ -static void export_object(const struct object_id *oid, enum object_type type, +static int export_object(const struct object_id *oid, enum object_type type, int raw, const char *filename) { struct child_process cmd = CHILD_PROCESS_INIT; @@ -210,7 +218,7 @@ static void export_object(const struct object_id *oid, enum object_type type, fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (fd < 0) - die_errno("unable to open %s for writing", filename); + return error_errno("unable to open %s for writing", filename); argv_array_push(&cmd.args, "--no-replace-objects"); argv_array_push(&cmd.args, "cat-file"); @@ -223,7 +231,8 @@ static void export_object(const struct object_id *oid, enum object_type type, cmd.out = fd; if (run_command(&cmd)) - die("cat-file reported failure"); + return error("cat-file reported failure"); + return 0; } /* @@ -231,14 +240,14 @@ static void export_object(const struct object_id *oid, enum object_type type, * interpreting it as "type", and writing the result to the object database. * The sha1 of the written object is returned via sha1. */ -static void import_object(struct object_id *oid, enum object_type type, +static int import_object(struct object_id *oid, enum object_type type, int raw, const char *filename) { int fd; fd = open(filename, O_RDONLY); if (fd < 0) - die_errno("unable to open %s for reading", filename); + return error_errno("unable to open %s for reading", filename); if (!raw && type == OBJ_TREE) { const char *argv[] = { "mktree", NULL }; @@ -250,27 +259,40 @@ static void import_object(struct object_id *oid, enum object_type type, cmd.in = fd; cmd.out = -1; - if (start_command(&cmd)) - die("unable to spawn mktree"); + if (start_command(&cmd)) { + close(fd); + return error("unable to spawn mktree"); + } - if (strbuf_read(&result, cmd.out, 41) < 0) - die_errno("unable to read from mktree"); + if (strbuf_read(&result, cmd.out, 41) < 0) { + error_errno("unable to read from mktree"); + close(fd); + close(cmd.out); + return -1; + } close(cmd.out); - if (finish_command(&cmd)) - die("mktree reported failure"); - if (get_oid_hex(result.buf, oid) < 0) - die("mktree did not return an object name"); + if (finish_command(&cmd)) { + strbuf_release(&result); + return error("mktree reported failure"); + } + if (get_oid_hex(result.buf, oid) < 0) { + strbuf_release(&result); + return error("mktree did not return an object name"); + } strbuf_release(&result); } else { struct stat st; int flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT; - if (fstat(fd, &st) < 0) - die_errno("unable to fstat %s", filename); + if (fstat(fd, &st) < 0) { + error_errno("unable to fstat %s", filename); + close(fd); + return -1; + } if (index_fd(oid, fd, &st, type, NULL, flags) < 0) - die("unable to write object to database"); + return error("unable to write object to database"); /* index_fd close()s fd for us */ } @@ -278,30 +300,43 @@ static void import_object(struct object_id *oid, enum object_type type, * No need to close(fd) here; both run-command and index-fd * will have done it for us. */ + return 0; } static int edit_and_replace(const char *object_ref, int force, int raw) { - char *tmpfile = git_pathdup("REPLACE_EDITOBJ"); + char *tmpfile; enum object_type type; struct object_id old_oid, new_oid, prev; struct strbuf ref = STRBUF_INIT; if (get_oid(object_ref, &old_oid) < 0) - die("Not a valid object name: '%s'", object_ref); + return error("Not a valid object name: '%s'", object_ref); type = oid_object_info(&old_oid, NULL); if (type < 0) - die("unable to get object type for %s", oid_to_hex(&old_oid)); + return error("unable to get object type for %s", + oid_to_hex(&old_oid)); - check_ref_valid(&old_oid, &prev, &ref, force); + if (check_ref_valid(&old_oid, &prev, &ref, force)) { + strbuf_release(&ref); + return -1; + } strbuf_release(&ref); - export_object(&old_oid, type, raw, tmpfile); - if (launch_editor(tmpfile, NULL, NULL) < 0) - die("editing object file failed"); - import_object(&new_oid, type, raw, tmpfile); - + tmpfile = git_pathdup("REPLACE_EDITOBJ"); + if (export_object(&old_oid, type, raw, tmpfile)) { + free(tmpfile); + return -1; + } + if (launch_editor(tmpfile, NULL, NULL) < 0) { + free(tmpfile); + return error("editing object file failed"); + } + if (import_object(&new_oid, type, raw, tmpfile)) { + free(tmpfile); + return -1; + } free(tmpfile); if (!oidcmp(&old_oid, &new_oid)) @@ -310,7 +345,7 @@ static int edit_and_replace(const char *object_ref, int force, int raw) return replace_object_oid(object_ref, &old_oid, "replacement", &new_oid, force); } -static void replace_parents(struct strbuf *buf, int argc, const char **argv) +static int replace_parents(struct strbuf *buf, int argc, const char **argv) { struct strbuf new_parents = STRBUF_INIT; const char *parent_start, *parent_end; @@ -327,9 +362,15 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv) /* prepare new parents */ for (i = 0; i < argc; i++) { struct object_id oid; - if (get_oid(argv[i], &oid) < 0) - die(_("Not a valid object name: '%s'"), argv[i]); - lookup_commit_or_die(&oid, argv[i]); + if (get_oid(argv[i], &oid) < 0) { + strbuf_release(&new_parents); + return error(_("Not a valid object name: '%s'"), + argv[i]); + } + if (!lookup_commit_reference(&oid)) { + strbuf_release(&new_parents); + return error(_("could not parse %s"), argv[i]); + } strbuf_addf(&new_parents, "parent %s\n", oid_to_hex(&oid)); } @@ -338,6 +379,7 @@ static void replace_parents(struct strbuf *buf, int argc, const char **argv) new_parents.buf, new_parents.len); strbuf_release(&new_parents); + return 0; } struct check_mergetag_data { @@ -358,21 +400,23 @@ static int check_one_mergetag(struct commit *commit, hash_object_file(extra->value, extra->len, type_name(OBJ_TAG), &tag_oid); tag = lookup_tag(&tag_oid); if (!tag) - die(_("bad mergetag in commit '%s'"), ref); + return error(_("bad mergetag in commit '%s'"), ref); if (parse_tag_buffer(tag, extra->value, extra->len)) - die(_("malformed mergetag in commit '%s'"), ref); + return error(_("malformed mergetag in commit '%s'"), ref); /* iterate over new parents */ for (i = 1; i < mergetag_data->argc; i++) { struct object_id oid; if (get_oid(mergetag_data->argv[i], &oid) < 0) - die(_("Not a valid object name: '%s'"), mergetag_data->argv[i]); + return error(_("Not a valid object name: '%s'"), + mergetag_data->argv[i]); if (!oidcmp(&tag->tagged->oid, &oid)) return 0; /* found */ } - die(_("original commit '%s' contains mergetag '%s' that is discarded; " - "use --edit instead of --graft"), ref, oid_to_hex(&tag_oid)); + return error(_("original commit '%s' contains mergetag '%s' that is " + "discarded; use --edit instead of --graft"), ref, + oid_to_hex(&tag_oid)); } static int check_mergetags(struct commit *commit, int argc, const char **argv) @@ -394,24 +438,35 @@ static int create_graft(int argc, const char **argv, int force) unsigned long size; if (get_oid(old_ref, &old_oid) < 0) - die(_("Not a valid object name: '%s'"), old_ref); - commit = lookup_commit_or_die(&old_oid, old_ref); + return error(_("Not a valid object name: '%s'"), old_ref); + commit = lookup_commit_reference(&old_oid); + if (!commit) + return error(_("could not parse %s"), old_ref); buffer = get_commit_buffer(commit, &size); strbuf_add(&buf, buffer, size); unuse_commit_buffer(commit, buffer); - replace_parents(&buf, argc - 1, &argv[1]); + if (replace_parents(&buf, argc - 1, &argv[1]) < 0) { + strbuf_release(&buf); + return -1; + } if (remove_signature(&buf)) { warning(_("the original commit '%s' has a gpg signature."), old_ref); warning(_("the signature will be removed in the replacement commit!")); } - check_mergetags(commit, argc, argv); + if (check_mergetags(commit, argc, argv)) { + strbuf_release(&buf); + return -1; + } - if (write_object_file(buf.buf, buf.len, commit_type, &new_oid)) - die(_("could not write replacement commit for: '%s'"), old_ref); + if (write_object_file(buf.buf, buf.len, commit_type, &new_oid)) { + strbuf_release(&buf); + return error(_("could not write replacement commit for: '%s'"), + old_ref); + } strbuf_release(&buf); -- cgit v1.2.3 From 041c98e22d88d963a92f1bac6ab348a0d4c6d228 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 29 Apr 2018 00:44:32 +0200 Subject: replace: prepare create_graft() for converting graft files wholesale When converting all grafts in a graft file to replace refs, and one of them happens to leave the original commit's parents unchanged, we do not want to error out. Instead, we would like to issue a warning. Prepare the create_graft() function for such a use case by adding a `gentle` parameter. If set, we do not return an error when the replace ref is unchanged, but a mere warning. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- builtin/replace.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) (limited to 'builtin/replace.c') diff --git a/builtin/replace.c b/builtin/replace.c index e57d3d187e..64f5811270 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -428,7 +428,7 @@ static int check_mergetags(struct commit *commit, int argc, const char **argv) return for_each_mergetag(check_one_mergetag, commit, &mergetag_data); } -static int create_graft(int argc, const char **argv, int force) +static int create_graft(int argc, const char **argv, int force, int gentle) { struct object_id old_oid, new_oid; const char *old_ref = argv[0]; @@ -470,8 +470,13 @@ static int create_graft(int argc, const char **argv, int force) strbuf_release(&buf); - if (!oidcmp(&old_oid, &new_oid)) + if (!oidcmp(&old_oid, &new_oid)) { + if (gentle) { + warning("graft for '%s' unnecessary", oid_to_hex(&old_oid)); + return 0; + } return error("new commit is the same as the old one: '%s'", oid_to_hex(&old_oid)); + } return replace_object_oid(old_ref, &old_oid, "replacement", &new_oid, force); } @@ -547,7 +552,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix) if (argc < 1) usage_msg_opt("-g needs at least one argument", git_replace_usage, options); - return create_graft(argc, argv, force); + return create_graft(argc, argv, force, 0); case MODE_LIST: if (argc > 1) -- cgit v1.2.3 From fb40429109723c0d8ec77ba81421f508ac2532a0 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Sun, 29 Apr 2018 00:44:35 +0200 Subject: replace: introduce --convert-graft-file This option is intended to help with the transition away from the now-deprecated graft file. Signed-off-by: Johannes Schindelin Signed-off-by: Junio C Hamano --- Documentation/git-replace.txt | 11 ++++++++--- builtin/replace.c | 44 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 4 deletions(-) (limited to 'builtin/replace.c') diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt index e5c57ae6ef..246dc9943c 100644 --- a/Documentation/git-replace.txt +++ b/Documentation/git-replace.txt @@ -11,6 +11,7 @@ SYNOPSIS 'git replace' [-f] 'git replace' [-f] --edit 'git replace' [-f] --graft [...] +'git replace' [-f] --convert-graft-file 'git replace' -d ... 'git replace' [--format=] [-l []] @@ -87,9 +88,13 @@ OPTIONS content as except that its parents will be [...] instead of 's parents. A replacement ref is then created to replace with the newly created - commit. See contrib/convert-grafts-to-replace-refs.sh for an - example script based on this option that can convert grafts to - replace refs. + commit. Use `--convert-graft-file` to convert a + `$GIT_DIR/info/grafts` file and use replace refs instead. + +--convert-graft-file:: + Creates graft commits for all entries in `$GIT_DIR/info/grafts` + and deletes that file upon success. The purpose is to help users + with transitioning off of the now-deprecated graft file. -l :: --list :: diff --git a/builtin/replace.c b/builtin/replace.c index 64f5811270..a87fca045b 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -20,6 +20,7 @@ static const char * const git_replace_usage[] = { N_("git replace [-f] "), N_("git replace [-f] --edit "), N_("git replace [-f] --graft [...]"), + N_("git replace [-f] --convert-graft-file"), N_("git replace -d ..."), N_("git replace [--format=] [-l []]"), NULL @@ -481,6 +482,38 @@ static int create_graft(int argc, const char **argv, int force, int gentle) return replace_object_oid(old_ref, &old_oid, "replacement", &new_oid, force); } +static int convert_graft_file(int force) +{ + const char *graft_file = get_graft_file(); + FILE *fp = fopen_or_warn(graft_file, "r"); + struct strbuf buf = STRBUF_INIT, err = STRBUF_INIT; + struct argv_array args = ARGV_ARRAY_INIT; + + if (!fp) + return -1; + + while (strbuf_getline(&buf, fp) != EOF) { + if (*buf.buf == '#') + continue; + + argv_array_split(&args, buf.buf); + if (args.argc && create_graft(args.argc, args.argv, force, 1)) + strbuf_addf(&err, "\n\t%s", buf.buf); + argv_array_clear(&args); + } + fclose(fp); + + strbuf_release(&buf); + + if (!err.len) + return unlink_or_warn(graft_file); + + warning(_("could not convert the following graft(s):\n%s"), err.buf); + strbuf_release(&err); + + return -1; +} + int cmd_replace(int argc, const char **argv, const char *prefix) { int force = 0; @@ -492,6 +525,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix) MODE_DELETE, MODE_EDIT, MODE_GRAFT, + MODE_CONVERT_GRAFT_FILE, MODE_REPLACE } cmdmode = MODE_UNSPECIFIED; struct option options[] = { @@ -499,6 +533,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix) OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE), OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT), OPT_CMDMODE('g', "graft", &cmdmode, N_("change a commit's parents"), MODE_GRAFT), + OPT_CMDMODE(0, "convert-graft-file", &cmdmode, N_("convert existing graft file"), MODE_CONVERT_GRAFT_FILE), OPT_BOOL_F('f', "force", &force, N_("replace the ref if it exists"), PARSE_OPT_NOCOMPLETE), OPT_BOOL(0, "raw", &raw, N_("do not pretty-print contents for --edit")), @@ -521,7 +556,8 @@ int cmd_replace(int argc, const char **argv, const char *prefix) if (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT && - cmdmode != MODE_GRAFT) + cmdmode != MODE_GRAFT && + cmdmode != MODE_CONVERT_GRAFT_FILE) usage_msg_opt("-f only makes sense when writing a replacement", git_replace_usage, options); @@ -554,6 +590,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix) git_replace_usage, options); return create_graft(argc, argv, force, 0); + case MODE_CONVERT_GRAFT_FILE: + if (argc != 0) + usage_msg_opt("--convert-graft-file takes no argument", + git_replace_usage, options); + return !!convert_graft_file(force); + case MODE_LIST: if (argc > 1) usage_msg_opt("only one pattern can be given with -l", -- cgit v1.2.3