From 3f495f67bc4ec744ac60f6e7bec0924022670998 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 26 Apr 2014 22:00:54 +0200 Subject: replace: refactor command-mode determination The git-replace command has three modes: listing, deleting, and replacing. The first two are selected explicitly. If none is selected, we fallback to listing when there are no arguments, and replacing otherwise. Let's figure out up front which operation we are going to do, before getting into the application logic. That lets us simplify our option checks (e.g., we currently have to check whether a useless "--force" is given both along with an explicit list, as well as with an implicit one). This saves some lines, makes the logic easier to follow, and will facilitate further cleanups. Signed-off-by: Jeff King Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index b62420a01a..28db96fcce 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -182,12 +182,16 @@ int cmd_replace(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0); + if (!list && !delete) + if (!argc) + list = 1; + if (list && delete) usage_msg_opt("-l and -d cannot be used together", git_replace_usage, options); - if (format && delete) - usage_msg_opt("--format and -d cannot be used together", + if (format && !list) + usage_msg_opt("--format cannot be used when not listing", git_replace_usage, options); if (force && (list || delete)) @@ -207,9 +211,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix) if (argc != 2) usage_msg_opt("bad number of arguments", git_replace_usage, options); - if (format) - usage_msg_opt("--format cannot be used when not listing", - git_replace_usage, options); return replace_object(argv[0], argv[1], force); } @@ -217,9 +218,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix) if (argc > 1) usage_msg_opt("only one pattern can be given with -l", git_replace_usage, options); - if (force) - usage_msg_opt("-f needs some arguments", - git_replace_usage, options); return list_replace_refs(argv[0], format); } -- cgit v1.2.1 From 70c7bd6dafc83eee7bf76d33d12027224d80f20d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 26 Apr 2014 22:00:55 +0200 Subject: replace: use OPT_CMDMODE to handle modes By using OPT_CMDMODE, the mutual exclusion between modes is taken care of for us. It also makes it easy for us to maintain a single variable with the mode, which makes its intent more clear. We can use a single switch() to make sure we have covered all of the modes. This ends up breaking even in code size, but the win will be much bigger when we start adding more modes. Signed-off-by: Jeff King Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 49 +++++++++++++++++++++++++------------------------ 1 file changed, 25 insertions(+), 24 deletions(-) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index 28db96fcce..29cf699659 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -168,11 +168,17 @@ static int replace_object(const char *object_ref, const char *replace_ref, int cmd_replace(int argc, const char **argv, const char *prefix) { - int list = 0, delete = 0, force = 0; + int force = 0; const char *format = NULL; + enum { + MODE_UNSPECIFIED = 0, + MODE_LIST, + MODE_DELETE, + MODE_REPLACE + } cmdmode = MODE_UNSPECIFIED; struct option options[] = { - OPT_BOOL('l', "list", &list, N_("list replace refs")), - OPT_BOOL('d', "delete", &delete, N_("delete replace refs")), + OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST), + OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE), OPT_BOOL('f', "force", &force, N_("replace the ref if it exists")), OPT_STRING(0, "format", &format, N_("format"), N_("use this format")), OPT_END() @@ -182,42 +188,37 @@ int cmd_replace(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, git_replace_usage, 0); - if (!list && !delete) - if (!argc) - list = 1; + if (!cmdmode) + cmdmode = argc ? MODE_REPLACE : MODE_LIST; - if (list && delete) - usage_msg_opt("-l and -d cannot be used together", - git_replace_usage, options); - - if (format && !list) + if (format && cmdmode != MODE_LIST) usage_msg_opt("--format cannot be used when not listing", git_replace_usage, options); - if (force && (list || delete)) - usage_msg_opt("-f cannot be used with -d or -l", + if (force && cmdmode != MODE_REPLACE) + usage_msg_opt("-f only makes sense when writing a replacement", git_replace_usage, options); - /* Delete refs */ - if (delete) { + switch (cmdmode) { + case MODE_DELETE: if (argc < 1) usage_msg_opt("-d needs at least one argument", git_replace_usage, options); return for_each_replace_name(argv, delete_replace_ref); - } - /* Replace object */ - if (!list && argc) { + case MODE_REPLACE: if (argc != 2) usage_msg_opt("bad number of arguments", git_replace_usage, options); return replace_object(argv[0], argv[1], force); - } - /* List refs, even if "list" is not set */ - if (argc > 1) - usage_msg_opt("only one pattern can be given with -l", - git_replace_usage, options); + case MODE_LIST: + if (argc > 1) + usage_msg_opt("only one pattern can be given with -l", + git_replace_usage, options); + return list_replace_refs(argv[0], format); - return list_replace_refs(argv[0], format); + default: + die("BUG: invalid cmdmode %d", (int)cmdmode); + } } -- cgit v1.2.1 From 479bd75751d81b6f3ffc29fcece56f9a200493a1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 26 Apr 2014 22:00:56 +0200 Subject: replace: factor object resolution out of replace_object As we add new options that operate on objects before replacing them, we'll want to be able to feed raw sha1s straight into replace_object. Split replace_object into the object-resolution part and the actual replacement. Signed-off-by: Jeff King Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index 29cf699659..af40129c8f 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -123,19 +123,17 @@ static int delete_replace_ref(const char *name, const char *ref, return 0; } -static int replace_object(const char *object_ref, const char *replace_ref, - int force) +static int replace_object_sha1(const char *object_ref, + unsigned char object[20], + const char *replace_ref, + unsigned char repl[20], + int force) { - unsigned char object[20], prev[20], repl[20]; + unsigned char prev[20]; enum object_type obj_type, repl_type; char ref[PATH_MAX]; struct ref_lock *lock; - if (get_sha1(object_ref, object)) - die("Failed to resolve '%s' as a valid ref.", object_ref); - if (get_sha1(replace_ref, repl)) - die("Failed to resolve '%s' as a valid ref.", replace_ref); - if (snprintf(ref, sizeof(ref), "refs/replace/%s", sha1_to_hex(object)) > sizeof(ref) - 1) @@ -166,6 +164,18 @@ static int replace_object(const char *object_ref, const char *replace_ref, return 0; } +static int replace_object(const char *object_ref, const char *replace_ref, int force) +{ + unsigned char object[20], repl[20]; + + if (get_sha1(object_ref, object)) + die("Failed to resolve '%s' as a valid ref.", object_ref); + if (get_sha1(replace_ref, repl)) + die("Failed to resolve '%s' as a valid ref.", replace_ref); + + return replace_object_sha1(object_ref, object, replace_ref, repl, force); +} + int cmd_replace(int argc, const char **argv, const char *prefix) { int force = 0; -- cgit v1.2.1 From b892bb45eacb484be281a992bef66ea723210717 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 26 Apr 2014 22:00:57 +0200 Subject: replace: add --edit option This allows you to run: git replace --edit SHA1 to get dumped in an editor with the contents of the object for SHA1. The result is then read back in and used as a "replace" object for SHA1. The writing/reading is type-aware, so you get to edit "ls-tree" output rather than the binary tree format. Missing documentation and tests. Signed-off-by: Jeff King Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 111 insertions(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index af40129c8f..3da1bae0a6 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -12,6 +12,7 @@ #include "builtin.h" #include "refs.h" #include "parse-options.h" +#include "run-command.h" static const char * const git_replace_usage[] = { N_("git replace [-f] "), @@ -176,6 +177,107 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f return replace_object_sha1(object_ref, object, replace_ref, repl, force); } +/* + * Write the contents of the object named by "sha1" to the file "filename", + * pretty-printed for human editing based on its type. + */ +static void export_object(const unsigned char *sha1, const char *filename) +{ + const char *argv[] = { "--no-replace-objects", "cat-file", "-p", NULL, NULL }; + struct child_process cmd = { argv }; + int fd; + + fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666); + if (fd < 0) + die_errno("unable to open %s for writing", filename); + + argv[3] = sha1_to_hex(sha1); + cmd.git_cmd = 1; + cmd.out = fd; + + if (run_command(&cmd)) + die("cat-file reported failure"); + + close(fd); +} + +/* + * Read a previously-exported (and possibly edited) object back from "filename", + * 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(unsigned char *sha1, enum object_type type, + const char *filename) +{ + int fd; + + fd = open(filename, O_RDONLY); + if (fd < 0) + die_errno("unable to open %s for reading", filename); + + if (type == OBJ_TREE) { + const char *argv[] = { "mktree", NULL }; + struct child_process cmd = { argv }; + struct strbuf result = STRBUF_INIT; + + cmd.argv = argv; + cmd.git_cmd = 1; + cmd.in = fd; + cmd.out = -1; + + if (start_command(&cmd)) + die("unable to spawn mktree"); + + if (strbuf_read(&result, cmd.out, 41) < 0) + die_errno("unable to read from mktree"); + close(cmd.out); + + if (finish_command(&cmd)) + die("mktree reported failure"); + if (get_sha1_hex(result.buf, sha1) < 0) + die("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 (index_fd(sha1, fd, &st, type, NULL, flags) < 0) + die("unable to write object to database"); + /* index_fd close()s fd for us */ + } + + /* + * No need to close(fd) here; both run-command and index-fd + * will have done it for us. + */ +} + +static int edit_and_replace(const char *object_ref, int force) +{ + char *tmpfile = git_pathdup("REPLACE_EDITOBJ"); + enum object_type type; + unsigned char old[20], new[20]; + + if (get_sha1(object_ref, old) < 0) + die("Not a valid object name: '%s'", object_ref); + + type = sha1_object_info(old, NULL); + if (type < 0) + die("unable to get object type for %s", sha1_to_hex(old)); + + export_object(old, tmpfile); + if (launch_editor(tmpfile, NULL, NULL) < 0) + die("editing object file failed"); + import_object(new, type, tmpfile); + + free(tmpfile); + + return replace_object_sha1(object_ref, old, "replacement", new, force); +} + int cmd_replace(int argc, const char **argv, const char *prefix) { int force = 0; @@ -184,11 +286,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix) MODE_UNSPECIFIED = 0, MODE_LIST, MODE_DELETE, + MODE_EDIT, MODE_REPLACE } cmdmode = MODE_UNSPECIFIED; struct option options[] = { OPT_CMDMODE('l', "list", &cmdmode, N_("list replace refs"), MODE_LIST), OPT_CMDMODE('d', "delete", &cmdmode, N_("delete replace refs"), MODE_DELETE), + OPT_CMDMODE('e', "edit", &cmdmode, N_("edit existing object"), MODE_EDIT), OPT_BOOL('f', "force", &force, N_("replace the ref if it exists")), OPT_STRING(0, "format", &format, N_("format"), N_("use this format")), OPT_END() @@ -205,7 +309,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix) usage_msg_opt("--format cannot be used when not listing", git_replace_usage, options); - if (force && cmdmode != MODE_REPLACE) + if (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT) usage_msg_opt("-f only makes sense when writing a replacement", git_replace_usage, options); @@ -222,6 +326,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix) git_replace_usage, options); return replace_object(argv[0], argv[1], force); + case MODE_EDIT: + if (argc != 1) + usage_msg_opt("-e needs exactly one argument", + git_replace_usage, options); + return edit_and_replace(argv[0], force); + case MODE_LIST: if (argc > 1) usage_msg_opt("only one pattern can be given with -l", -- cgit v1.2.1 From f22166b5fee7dc3deaf44dda31d1b5d7ac1fdfd8 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 17 May 2014 14:16:34 +0200 Subject: replace: make sure --edit results in a different object It's a bad idea to create a replace ref for an object that points to the original object itself. That's why we have to check if the result from editing the original object is a different object and error out if it isn't. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index 3da1bae0a6..0751804039 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -275,6 +275,9 @@ static int edit_and_replace(const char *object_ref, int force) free(tmpfile); + if (!hashcmp(old, new)) + return error("new object is the same as the old one: '%s'", sha1_to_hex(old)); + return replace_object_sha1(object_ref, old, "replacement", new, force); } -- cgit v1.2.1 From b6e38840921ac4a0fe07e10e632f66736745da10 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 17 May 2014 14:16:35 +0200 Subject: replace: refactor checking ref validity This will be useful in a following commit when we will want to check if the ref already exists before we let the user edit an object. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index 0751804039..3d6edaf7c7 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -124,6 +124,25 @@ static int delete_replace_ref(const char *name, const char *ref, return 0; } +static void check_ref_valid(unsigned char object[20], + unsigned char prev[20], + char *ref, + int ref_size, + int force) +{ + if (snprintf(ref, ref_size, + "refs/replace/%s", + sha1_to_hex(object)) > ref_size - 1) + die("replace ref name too long: %.*s...", 50, ref); + if (check_refname_format(ref, 0)) + die("'%s' is not a valid ref name.", ref); + + if (read_ref(ref, prev)) + hashclr(prev); + else if (!force) + die("replace ref '%s' already exists", ref); +} + static int replace_object_sha1(const char *object_ref, unsigned char object[20], const char *replace_ref, @@ -135,13 +154,6 @@ static int replace_object_sha1(const char *object_ref, char ref[PATH_MAX]; struct ref_lock *lock; - if (snprintf(ref, sizeof(ref), - "refs/replace/%s", - sha1_to_hex(object)) > sizeof(ref) - 1) - die("replace ref name too long: %.*s...", 50, ref); - if (check_refname_format(ref, 0)) - die("'%s' is not a valid ref name.", ref); - obj_type = sha1_object_info(object, NULL); repl_type = sha1_object_info(repl, NULL); if (!force && obj_type != repl_type) @@ -151,10 +163,7 @@ static int replace_object_sha1(const char *object_ref, object_ref, typename(obj_type), replace_ref, typename(repl_type)); - if (read_ref(ref, prev)) - hashclr(prev); - else if (!force) - die("replace ref '%s' already exists", ref); + check_ref_valid(object, prev, ref, sizeof(ref), force); lock = lock_any_ref_for_update(ref, prev, 0, NULL); if (!lock) -- cgit v1.2.1 From 24790835738dc098fa6becedc44aac0341b7d5af Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 17 May 2014 14:16:36 +0200 Subject: replace: die early if replace ref already exists If a replace ref already exists for an object, it is much better for the user if we error out before we let the user edit the object, rather than after. Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index 3d6edaf7c7..4ee3d929fa 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -268,7 +268,8 @@ static int edit_and_replace(const char *object_ref, int force) { char *tmpfile = git_pathdup("REPLACE_EDITOBJ"); enum object_type type; - unsigned char old[20], new[20]; + unsigned char old[20], new[20], prev[20]; + char ref[PATH_MAX]; if (get_sha1(object_ref, old) < 0) die("Not a valid object name: '%s'", object_ref); @@ -277,6 +278,8 @@ static int edit_and_replace(const char *object_ref, int force) if (type < 0) die("unable to get object type for %s", sha1_to_hex(old)); + check_ref_valid(old, prev, ref, sizeof(ref), force); + export_object(old, tmpfile); if (launch_editor(tmpfile, NULL, NULL) < 0) die("editing object file failed"); -- cgit v1.2.1 From ab77c309b633ab3afc16e91cbff307c39599d048 Mon Sep 17 00:00:00 2001 From: Christian Couder Date: Sat, 17 May 2014 14:16:38 +0200 Subject: replace: add --edit to usage string Signed-off-by: Christian Couder Signed-off-by: Junio C Hamano --- builtin/replace.c | 1 + 1 file changed, 1 insertion(+) (limited to 'builtin') diff --git a/builtin/replace.c b/builtin/replace.c index 4ee3d929fa..1bb491d3c4 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -16,6 +16,7 @@ static const char * const git_replace_usage[] = { N_("git replace [-f] "), + N_("git replace [-f] --edit "), N_("git replace -d ..."), N_("git replace [--format=] [-l []]"), NULL -- cgit v1.2.1