[PATCH v1 0/4] replace: add option to edit a Git object

[PATCH v1 0/4] replace: add option to edit a Git object

[Christian Couder]

This patch series comes from what Peff sent in the following thread:

http://thread.gmane.org/gmane.comp.version-control.git/243361/focus=243528

I added the following fixes:

- add "strbuf_release(&result);" in import_object(); this was suggested
  by Eric Sunshine
- use MODE_LIST instead of MODE_DELETE if no arguments are passed; this
  makes the test suite pass
- add "--no-replace-objects" when calling "git cat-file" in export_object();
  so that we edit the original object if an object is already replaced

I am not happy with the fact that if the user doesn't modify the object when
editing it, then a replace ref can still be created that points to the
original object. I think something should be done to avoid that.

Once that is fixed, I plan to add some tests and documentation, but I wanted
first to let you know that I am looking at this.

Jeff King (4):
  replace: refactor command-mode determination
  replace: use OPT_CMDMODE to handle modes
  replace: factor object resolution out of replace_object
  replace: add --edit option

 builtin/replace.c | 189 ++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 154 insertions(+), 35 deletions(-)

-- 
1.9.1.636.g20d5f34

[PATCH v1 1/4] replace: refactor command-mode determination

[Christian Couder]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index b62420a..28db96f 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);
 }
-- 
1.9.1.636.g20d5f34

[PATCH v1 2/4] replace: use OPT_CMDMODE to handle modes

[Christian Couder]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 49 +++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 28db96f..29cf699 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);
+	}
 }
-- 
1.9.1.636.g20d5f34

[PATCH v1 3/4] replace: factor object resolution out of replace_object

[Christian Couder]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 29cf699..af40129 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;
-- 
1.9.1.636.g20d5f34

[PATCH v1 4/4] replace: add --edit option

[Christian Couder]

From: Jeff King <peff@peff.net>

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 <peff@peff.net>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 112 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index af40129..3da1bae 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] <object> <replacement>"),
@@ -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",
-- 
1.9.1.636.g20d5f34

Re: [PATCH v1 0/4] replace: add option to edit a Git object

[Jeff King]

On Sat, Apr 26, 2014 at 10:00:53PM +0200, Christian Couder wrote:

> This patch series comes from what Peff sent in the following thread:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/243361/focus=243528

Thanks. As I recall, these were in pretty good shape, and I just read
over them again and didn't see anything wrong.

> I added the following fixes:
> 
> - add "strbuf_release(&result);" in import_object(); this was suggested
>   by Eric Sunshine
> - use MODE_LIST instead of MODE_DELETE if no arguments are passed; this
>   makes the test suite pass
> - add "--no-replace-objects" when calling "git cat-file" in export_object();
>   so that we edit the original object if an object is already replaced

All sensible, I think.

> I am not happy with the fact that if the user doesn't modify the object when
> editing it, then a replace ref can still be created that points to the
> original object. I think something should be done to avoid that.

Yeah, it should be easy to just hashcmp the sha1s after calling
import_object. In fact, I think we can just erase any existing replace
ref in that case (the user might have started with a replace ref and
converted it _back_ to the original object, for example).

> Once that is fixed, I plan to add some tests and documentation, but I wanted
> first to let you know that I am looking at this.

Great. Thanks for working on this.

-Peff

Re: [PATCH v1 0/4] replace: add option to edit a Git object

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sat, Apr 26, 2014 at 10:00:53PM +0200, Christian Couder wrote:
>
>> This patch series comes from what Peff sent in the following thread:
>> 
>> http://thread.gmane.org/gmane.comp.version-control.git/243361/focus=243528
>
> Thanks. As I recall, these were in pretty good shape, and I just read
> over them again and didn't see anything wrong.
>
>> I added the following fixes:
>> 
>> - add "strbuf_release(&result);" in import_object(); this was suggested
>>   by Eric Sunshine
>> - use MODE_LIST instead of MODE_DELETE if no arguments are passed; this
>>   makes the test suite pass
>> - add "--no-replace-objects" when calling "git cat-file" in export_object();
>>   so that we edit the original object if an object is already replaced
>
> All sensible, I think.
>
>> I am not happy with the fact that if the user doesn't modify the object when
>> editing it, then a replace ref can still be created that points to the
>> original object. I think something should be done to avoid that.
>
> Yeah, it should be easy to just hashcmp the sha1s after calling
> import_object. In fact, I think we can just erase any existing replace
> ref in that case (the user might have started with a replace ref and
> converted it _back_ to the original object, for example).
>
>> Once that is fixed, I plan to add some tests and documentation, but I wanted
>> first to let you know that I am looking at this.
>
> Great. Thanks for working on this.
>
> -Peff

Thanks.  In the meantime, I'll queue these as-is and push the result
out.