[PATCH v2 00/10] replace: add option to edit a Git object

[PATCH v2 00/10] 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

The first 4 patches (1/4, 2/4, 3/4 and 4/4) didn't change since v1.
I added 6 more small patches to add tests, documentation and a few
small improvements. 

Christian Couder (6):
  replace: make sure --edit results in a different object
  replace: refactor checking ref validity
  replace: die early if replace ref already exists
  replace: add tests for --edit
  replace: add --edit to usage string
  Documentation: replace: describe new --edit option

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

 Documentation/git-replace.txt |  15 ++-
 builtin/replace.c             | 225 +++++++++++++++++++++++++++++++++---------
 t/t6050-replace.sh            |  29 ++++++
 3 files changed, 223 insertions(+), 46 deletions(-)

-- 
1.9.rc0.17.g651113e

[PATCH v2 01/10] 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.rc0.17.g651113e

[PATCH v2 02/10] 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.rc0.17.g651113e

[PATCH v2 03/10] 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.rc0.17.g651113e

[PATCH v2 04/10] 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.rc0.17.g651113e

[PATCH v2 05/10] replace: make sure --edit results in a different object

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
 builtin/replace.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/builtin/replace.c b/builtin/replace.c
index 3da1bae..0751804 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);
 }
 
-- 
1.9.rc0.17.g651113e

Re: [PATCH v2 05/10] replace: make sure --edit results in a different object

[Jeff King]

On Sat, May 17, 2014 at 08:41:27AM +0200, Christian Couder wrote:

> 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.

I think that's reasonable.

Another option, which I think I mentioned earlier, would be to delete
any existing replacement ref, and return success. So you could use that
to "revert" a replaced object back to its original non-replaced form.

On a similar note, we might want to consider what happens when you
"--edit" an object which already has a replacement. Right now you end up
editing the _original_ object. I wonder if it would make sense to start
the editor with the contents of the replacement object (in which case
you might even "revert" without realizing it).

But those can easily come later if somebody feels like working on them.
Erroring out is not that bad an outcome, since the user can then "git
replace -d" themselves if they want to.

-Peff

[PATCH v2 06/10] replace: refactor checking ref validity

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
 builtin/replace.c | 31 ++++++++++++++++++++-----------
 1 file changed, 20 insertions(+), 11 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 0751804..3d6edaf 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)
-- 
1.9.rc0.17.g651113e

[PATCH v2 07/10] replace: die early if replace ref already exists

[Christian Couder]

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 <chriscool@tuxfamily.org>
---
 builtin/replace.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 3d6edaf..4ee3d92 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");
-- 
1.9.rc0.17.g651113e

Re: [PATCH v2 07/10] replace: die early if replace ref already exists

[Jeff King]

On Sat, May 17, 2014 at 08:41:29AM +0200, Christian Couder wrote:

> 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.

Definitely. Code looks fine to me.

-Peff

[PATCH v2 08/10] replace: add tests for --edit

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t6050-replace.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/t/t6050-replace.sh b/t/t6050-replace.sh
index 719a116..7609174 100755
--- a/t/t6050-replace.sh
+++ b/t/t6050-replace.sh
@@ -318,6 +318,35 @@ test_expect_success 'test --format long' '
 	test_cmp expected actual
 '
 
+test_expect_success 'setup a fake editor' '
+	cat >fakeeditor <<-\EOF &&
+		#!/bin/sh
+		sed -e "s/A U Thor/A fake Thor/" "$1" >"$1.new"
+		mv "$1.new" "$1"
+	EOF
+	chmod +x fakeeditor
+'
+
+test_expect_success '--edit with and without already replaced object' '
+	GIT_EDITOR=./fakeeditor test_must_fail git replace --edit "$PARA3" &&
+	GIT_EDITOR=./fakeeditor git replace --force --edit "$PARA3" &&
+	git replace -l | grep "$PARA3" &&
+	git cat-file commit "$PARA3" | grep "A fake Thor" &&
+	git replace -d "$PARA3" &&
+	GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" &&
+	git replace -l | grep "$PARA3" &&
+	git cat-file commit "$PARA3" | grep "A fake Thor"
+'
+
+test_expect_success '--edit and change nothing or command failed' '
+	git replace -d "$PARA3" &&
+	GIT_EDITOR=true test_must_fail git replace --edit "$PARA3" &&
+	GIT_EDITOR="./fakeeditor;false" test_must_fail git replace --edit "$PARA3" &&
+	GIT_EDITOR=./fakeeditor git replace --edit "$PARA3" &&
+	git replace -l | grep "$PARA3" &&
+	git cat-file commit "$PARA3" | grep "A fake Thor"
+'
+
 test_expect_success 'replace ref cleanup' '
 	test -n "$(git replace)" &&
 	git replace -d $(git replace) &&
-- 
1.9.rc0.17.g651113e

Re: [PATCH v2 08/10] replace: add tests for --edit

[Jeff King]

On Sat, May 17, 2014 at 08:41:30AM +0200, Christian Couder wrote:

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>  t/t6050-replace.sh | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

Yay, tests.

> +test_expect_success 'setup a fake editor' '
> +	cat >fakeeditor <<-\EOF &&
> +		#!/bin/sh
> +		sed -e "s/A U Thor/A fake Thor/" "$1" >"$1.new"
> +		mv "$1.new" "$1"
> +	EOF
> +	chmod +x fakeeditor
> +'

Please use write_script, like:

  test_expect_success 'setup a fake editor' '
	write_script fakeeditor <<-\EOF
		sed -e "s/A U Thor/A fake Thor/" "$1" >"$1.new"
		mv "$1.new" "$1"
	EOF
  '

which will use the $(SHELL_PATH) shebang. It doesn't matter for such a
simple script here (which even a broken #!/bin/sh could manage), but in
general, I think we're trying to set it consistently.

You could also &&-chain the commands in your fakeeditor script, though I
find it unlikely that sed will fail.

> +test_expect_success '--edit with and without already replaced object' '
> +	GIT_EDITOR=./fakeeditor test_must_fail git replace --edit "$PARA3" &&

Unfortunately it's not portable to do a one-shot environment variable on
a shell function like this; some shells leave the variable set after the
function returns. We've been using:

  test_must_fail env GIT_EDITOR=./fakeeditor git ...

The regular

  GIT_EDITOR=./fakeeditor git ...

ones are OK.

-Peff

[PATCH v2 09/10] replace: add --edit to usage string

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/replace.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/replace.c b/builtin/replace.c
index 4ee3d92..1bb491d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -16,6 +16,7 @@
 
 static const char * const git_replace_usage[] = {
 	N_("git replace [-f] <object> <replacement>"),
+	N_("git replace [-f] --edit <object>"),
 	N_("git replace -d <object>..."),
 	N_("git replace [--format=<format>] [-l [<pattern>]]"),
 	NULL
-- 
1.9.rc0.17.g651113e

[PATCH v2 10/10] Documentation: replace: describe new --edit option

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-replace.txt | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-replace.txt b/Documentation/git-replace.txt
index 0a02f70..37d872d 100644
--- a/Documentation/git-replace.txt
+++ b/Documentation/git-replace.txt
@@ -9,6 +9,7 @@ SYNOPSIS
 --------
 [verse]
 'git replace' [-f] <object> <replacement>
+'git replace' [-f] --edit <object>
 'git replace' -d <object>...
 'git replace' [--format=<format>] [-l [<pattern>]]
 
@@ -63,6 +64,14 @@ OPTIONS
 --delete::
 	Delete existing replace refs for the given objects.
 
+--edit <object>::
+	Launch an editor to let the user change the content of
+	<object>, then create a new object of the same type with the
+	changed content, and create a replace ref to replace <object>
+	with the new object. See linkgit:git-commit[1] and
+	linkgit:git-var[1] for details about how the editor will be
+	chosen.
+
 -l <pattern>::
 --list <pattern>::
 	List replace refs for objects that match the given pattern (or
@@ -92,7 +101,9 @@ CREATING REPLACEMENT OBJECTS
 
 linkgit:git-filter-branch[1], linkgit:git-hash-object[1] and
 linkgit:git-rebase[1], among other git commands, can be used to create
-replacement objects from existing objects.
+replacement objects from existing objects. The `--edit` option can
+also be used with 'git replace' to create a replacement object by
+editing an existing object.
 
 If you want to replace many blobs, trees or commits that are part of a
 string of commits, you may just want to create a replacement string of
@@ -117,6 +128,8 @@ linkgit:git-filter-branch[1]
 linkgit:git-rebase[1]
 linkgit:git-tag[1]
 linkgit:git-branch[1]
+linkgit:git-commit[1]
+linkgit:git-var[1]
 linkgit:git[1]
 
 GIT
-- 
1.9.rc0.17.g651113e

Re: [PATCH v2 10/10] Documentation: replace: describe new --edit option

[Jeff King]

On Sat, May 17, 2014 at 08:41:32AM +0200, Christian Couder wrote:

> @@ -63,6 +64,14 @@ OPTIONS
>  --delete::
>  	Delete existing replace refs for the given objects.
>  
> +--edit <object>::
> +	Launch an editor to let the user change the content of
> +	<object>, then create a new object of the same type with the
> +	changed content, and create a replace ref to replace <object>
> +	with the new object. See linkgit:git-commit[1] and
> +	linkgit:git-var[1] for details about how the editor will be
> +	chosen.

I found the first sentence a little hard to parse, and there are a few
more details that might be worth mentioning:

  --edit <object>::
    Edit an object's content interactively. The existing content for
    <object> is pretty-printed into a temporary file, an editor is
    launched on the file, and the result is parsed to create a new
    object of the same type as <object>. A replacement ref is then
    created to replace <object> with the newly created object.

I do not know if it is worth mentioning git-commit and git-var here. But
if we want to, I think git-var is sufficient (git-commit seems to mostly
just points at git-var these days).

-Peff

Re: [PATCH v2 00/10] replace: add option to edit a Git object

[Jeff King]

On Sat, May 17, 2014 at 08:41:22AM +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
> 
> The first 4 patches (1/4, 2/4, 3/4 and 4/4) didn't change since v1.
> I added 6 more small patches to add tests, documentation and a few
> small improvements. 

Thanks, I think these look pretty good. I made a few small comments in
reply to the individual patches.

-Peff