Re: [PATCH] disable grafts during fetch/push/bundle

Jeff King [off-list ref] writes:

On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:

quoted

... the plan, at least in my mind, has always been exactly that: grafts
were a nice little attempt but is broken---if you really wanted to
muck with the history without rewriting (which is still discouraged,
by the way), do not use "graft", but use "replace".

I certainly had in the back of my mind that grafts were a lesser form of
"replace", and that eventually we could get rid of the former. Perhaps
my question should have been: "why haven't we deprecated grafts yet?".

Given that we discourage "grafts" strongly and "replace" less so
(but still discourage it), telling the users that biting the bullet
and rewriting the history is _the_ permanent solution, I think it is
understandable why nobody has bothered to.

On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:

quoted

... the plan, at least in my mind, has always been exactly that: grafts
were a nice little attempt but is broken---if you really wanted to
muck with the history without rewriting (which is still discouraged,
by the way), do not use "graft", but use "replace".

I certainly had in the back of my mind that grafts were a lesser form of
"replace", and that eventually we could get rid of the former. Perhaps
my question should have been: "why haven't we deprecated grafts yet?".

Given that we discourage "grafts" strongly and "replace" less so
(but still discourage it), telling the users that biting the bullet
and rewriting the history is _the_ permanent solution, I think it is
understandable why nobody has bothered to.

Perhaps the patch below would help discourage grafts more?

The notable place in the documentation where grafts are still used is
git-filter-branch.txt.  But since the example there is about cementing
rewritten history, it might be OK to leave.

I used "outdated" below. We could also up the ante to "deprecated".

-- >8 --
Subject: [PATCH] docs: mark info/grafts as outdated

We should be encouraging people to use git-replace instead.

Signed-off-by: Jeff King <redacted>
---
 Documentation/gitrepository-layout.txt | 4 ++++
 Documentation/glossary-content.txt     | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt
index aa03882..17d2ea6 100644
--- a/Documentation/gitrepository-layout.txt
+++ b/Documentation/gitrepository-layout.txt

@@ -176,6 +176,10 @@ info/grafts::
 	per line describes a commit and its fake parents by
 	listing their 40-byte hexadecimal object names separated
 	by a space and terminated by a newline.
++
+Note that the grafts mechanism is outdated and can lead to problems
+transferring objects between repositories; see linkgit:git-replace[1]
+for a more flexible and robust system to do the same thing.
 
 info/exclude::
 	This file, by convention among Porcelains, stores the

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 378306f..be0858c 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt

@@ -176,6 +176,10 @@ current branch integrates with) obviously do not work, as there is no
 	you can make Git pretend the set of <<def_parent,parents>> a <<def_commit,commit>> has
 	is different from what was recorded when the commit was
 	created. Configured via the `.git/info/grafts` file.
++
+Note that the grafts mechanism is outdated and can lead to problems
+transferring objects between repositories; see linkgit:git-replace[1]
+for a more flexible and robust system to do the same thing.
 
 [[def_hash]]hash::
 	In Git's context, synonym for <<def_object_name,object name>>.

-- 
1.8.5.2.500.g8060133

On 03/05/2014 08:18 PM, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:

quoted

... the plan, at least in my mind, has always been exactly that: grafts
were a nice little attempt but is broken---if you really wanted to
muck with the history without rewriting (which is still discouraged,
by the way), do not use "graft", but use "replace".

I certainly had in the back of my mind that grafts were a lesser form of
"replace", and that eventually we could get rid of the former. Perhaps
my question should have been: "why haven't we deprecated grafts yet?".

Given that we discourage "grafts" strongly and "replace" less so
(but still discourage it), telling the users that biting the bullet
and rewriting the history is _the_ permanent solution, I think it is
understandable why nobody has bothered to.

Replace objects are better than grafts in *almost* every dimension.  The
exception is that it is dead simple to create grafts, whereas I always
have to break open the man pages to remember how to create a replace
object that does the same thing.

So I think a helpful step towards deprecating grafts would be to offer a
couple of convenience features to help people kick the "grafts" habit:

* A tool that converts grafts (i.e., the grafts read from
$GIT_DIR/info/grafts) into the equivalent replacements.

* A tool that creates a new replacement object that is the equivalent of
a graft.  I.e., it should do, using replace references, the equivalent
of the following command:

      echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts

These features could be added to "git replace" or could be built into a
new "git grafts" command.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Thu, Mar 6, 2014 at 9:42 AM, Michael Haggerty [off-list ref] wrote:

On 03/05/2014 08:18 PM, Junio C Hamano wrote:

quoted

Jeff King [off-list ref] writes:

quoted

On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:

quoted

... the plan, at least in my mind, has always been exactly that: grafts
were a nice little attempt but is broken---if you really wanted to
muck with the history without rewriting (which is still discouraged,
by the way), do not use "graft", but use "replace".

I certainly had in the back of my mind that grafts were a lesser form of
"replace", and that eventually we could get rid of the former. Perhaps
my question should have been: "why haven't we deprecated grafts yet?".

Given that we discourage "grafts" strongly and "replace" less so
(but still discourage it), telling the users that biting the bullet
and rewriting the history is _the_ permanent solution, I think it is
understandable why nobody has bothered to.

Replace objects are better than grafts in *almost* every dimension.  The
exception is that it is dead simple to create grafts, whereas I always
have to break open the man pages to remember how to create a replace
object that does the same thing.

So I think a helpful step towards deprecating grafts would be to offer a
couple of convenience features to help people kick the "grafts" habit:

* A tool that converts grafts (i.e., the grafts read from
$GIT_DIR/info/grafts) into the equivalent replacements.

Yeah, I sent a kind of rough draft of a script to do that last year to
the mailing list, but I didn't take the time to convert it to a real
script or command.

* A tool that creates a new replacement object that is the equivalent of
a graft.  I.e., it should do, using replace references, the equivalent
of the following command:

      echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts

Yeah, maybe it can be a "git create-replace-ref command" and it could
have a --convert-graft-file option to convert an existing graft file.

There have been discussions about such a command already some time ago.

These features could be added to "git replace" or could be built into a
new "git grafts" command.

I think Junio previously said that it was better if such features were
not part of "git replace". But maybe I misunderstood his subtle
saying.

And I don't think "git grafts" is a good name. It looks too much like
we are encouraging people to use grafts.

Thanks,
Christian.

On Thu, Mar 06, 2014 at 09:42:46AM +0100, Michael Haggerty wrote:

Replace objects are better than grafts in *almost* every dimension.  The
exception is that it is dead simple to create grafts, whereas I always
have to break open the man pages to remember how to create a replace
object that does the same thing.

So I think a helpful step towards deprecating grafts would be to offer a
couple of convenience features to help people kick the "grafts" habit:

I agree that better tool support would make "git replace" more pleasant
to use.

* A tool that converts grafts (i.e., the grafts read from
$GIT_DIR/info/grafts) into the equivalent replacements.

I don't know if this is strictly necessary, if we make your command
below pleasant to use. I.e., it should just be:

  while read sha1 parents; do
    git replace --graft $sha1 $parents
  done <.git/info/grafts

We can wrap that in "git replace --convert-grafts", but I do not think
grafts are so common that there would be a big demand for it.

* A tool that creates a new replacement object that is the equivalent of
a graft.  I.e., it should do, using replace references, the equivalent
of the following command:

      echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts

These features could be added to "git replace" or could be built into a
new "git grafts" command.

I think it would be nice to have a set of "mode" options for
"git-replace" to do basic editing of a sha1 and install the result
(technically you could split the editing into a separate command, but I
do not see the point in editing a sha1 and then _not_ replacing it).

Perhaps:

  # pretty-print sha1 based on type, start $EDITOR, create a
  # type-appropriate object from the result (e.g., using hash-object,
  # mktree, or mktag), and then set up the object as a replacement for
  # SHA1
  git replace --edit SHA1

  # ditto, but replace the $EDITOR step with the parent list
  git replace --graft SHA1 PARENT1 PARENT2

  # ...or remove entries from a tree
  git replace --remove-entry SHA1 foo bar

-Peff

On 03/06/2014 04:56 PM, Jeff King wrote:

On Thu, Mar 06, 2014 at 09:42:46AM +0100, Michael Haggerty wrote:

quoted

Replace objects are better than grafts in *almost* every dimension.  The
exception is that it is dead simple to create grafts, whereas I always
have to break open the man pages to remember how to create a replace
object that does the same thing.

So I think a helpful step towards deprecating grafts would be to offer a
couple of convenience features to help people kick the "grafts" habit:

I agree that better tool support would make "git replace" more pleasant
to use.

quoted

* A tool that converts grafts (i.e., the grafts read from
$GIT_DIR/info/grafts) into the equivalent replacements.

I don't know if this is strictly necessary, if we make your command
below pleasant to use. I.e., it should just be:

  while read sha1 parents; do
    git replace --graft $sha1 $parents
  done <.git/info/grafts

We can wrap that in "git replace --convert-grafts", but I do not think
grafts are so common that there would be a big demand for it.

It's probably easier to wrap it than to explain to Windows users what
they have to do.

quoted

* A tool that creates a new replacement object that is the equivalent of
a graft.  I.e., it should do, using replace references, the equivalent
of the following command:

      echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts

These features could be added to "git replace" or could be built into a
new "git grafts" command.

I think it would be nice to have a set of "mode" options for
"git-replace" to do basic editing of a sha1 and install the result
(technically you could split the editing into a separate command, but I
do not see the point in editing a sha1 and then _not_ replacing it).

If modifying without replacing is needed, it would be pretty easy to add
an option --stdout that writes the SHA1 of the modified object to stdout
instead of creating a replace reference.  That way what you want 95% of
the time is the default but there is still an escape hatch.

Perhaps:

  # pretty-print sha1 based on type, start $EDITOR, create a
  # type-appropriate object from the result (e.g., using hash-object,
  # mktree, or mktag), and then set up the object as a replacement for
  # SHA1
  git replace --edit SHA1

  # ditto, but replace the $EDITOR step with the parent list
  git replace --graft SHA1 PARENT1 PARENT2

  # ...or remove entries from a tree
  git replace --remove-entry SHA1 foo bar

I like this idea a lot, especially the pretty-printer round-tripping.

"git replace" could support some of the options that "git filter-branch"
can take, like --env-filter, --msg-filter, etc. (at least if the target
is a commit object).

All of this would make it possible to build up the changes that you want
to integrate via "filter-branch" piecemeal instead of having to have a
single monster filter-branch invocation.  For example,

    for c in $(git rev-list --all --before=2007-01-01
--author=root@localhost)
    do
        git replace --env-filter 'export AUTHOR_EMAIL=john@example.com' $c
    done
    # Make some more changes to other commits...
    # And when everything is done and checked:
    git filter-branch --all --tag-name-filter=cat

To me this is easier to construct than the equivalent filter-branch
invocation, and can be faster because its processing can be more easily
limited to the commits that need it.  Of course to really gain speed,
there should be a C program that bakes in replace references by
traversing the object tree rather than processing each commit
separately, like filter-branch.  I predict that this approach would have
most of the speed of BFG.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:

quoted

We can wrap that in "git replace --convert-grafts", but I do not think
grafts are so common that there would be a big demand for it.

It's probably easier to wrap it than to explain to Windows users what
they have to do.

How would Windows users get a graft file in the first-place? There's no
GUI for it! ;)

It should be easy to do "--convert-grafts", though, and I think it fits
into the scheme we're discussing below.

quoted

I think it would be nice to have a set of "mode" options for
"git-replace" to do basic editing of a sha1 and install the result
(technically you could split the editing into a separate command, but I
do not see the point in editing a sha1 and then _not_ replacing it).

If modifying without replacing is needed, it would be pretty easy to add
an option --stdout that writes the SHA1 of the modified object to stdout
instead of creating a replace reference.  That way what you want 95% of
the time is the default but there is still an escape hatch.

Agreed. I had originally though that perhaps something like this should
be part of "hash-object", and that "replace" should farm out the work.
But thinking on it more, it doesn't really make sense as part of
"hash-object".

quoted

Perhaps:

  # pretty-print sha1 based on type, start $EDITOR, create a
  # type-appropriate object from the result (e.g., using hash-object,
  # mktree, or mktag), and then set up the object as a replacement for
  # SHA1
  git replace --edit SHA1

Here's a rough series that gets us this far:

  [1/4]: replace: refactor command-mode determination
  [2/4]: replace: use OPT_CMDMODE to handle modes
  [3/4]: replace: factor object resolution out of replace_object
  [4/4]: replace: add --edit option

It shouldn't be too hard to do "--graft" or "--convert-grafts" on top.

I also noticed that doing:

    git replace foo foo

is less than friendly (we notice the cycle, but just barf). It's
especially easy to do with "git replace --edit", if you just exit the
editor without making changes.  Or if you make changes to an
already-replaced object to revert it back, in which case we would want
to notice and delete the replacement.

So I think we want to have "git replace foo foo" silently converted into
"git replace -d foo" (but without an error if there is no existing
replacement), and then "--edit" will just do the right thing, as it's
built on top.

I also noticed that the diff engine does not play well with replacements
of blobs. When we are diffing the trees, we see that the sha1 for path
"foo" is the same on either side, and do not look further, even though
feeding those sha1s to builtin_diff would fetch the replacements.  I
think compare_tree_entry would have to learn lookup_replace_object (and
I suspect it would make tree diffs noticeably slower when you have even
one replace ref).

"git replace" could support some of the options that "git filter-branch"
can take, like --env-filter, --msg-filter, etc. (at least if the target
is a commit object).

All of this would make it possible to build up the changes that you want
to integrate via "filter-branch" piecemeal instead of having to have a
single monster filter-branch invocation.  For example,

Right. I was tempted to suggest that, too, but I think it can get rather
tricky, as you need to replace in a loop, and sometimes the exact
objects you need aren't obvious.  For example, a common use of
"--index-filter" is to remove a single file. But to remove
"foo/bar/baz", you would need to loop over each commit, find the tree
for "foo/bar", and then remove the "baz" entry in 

Still, I really like the workflow of having decent "replace" tools,
followed by "cementing" the changes into place with a "filter-branch"
run (which, btw, does not yet know how to cement trees and blobs into
place). It lets you work on the filtering incrementally, and even share
or work collaboratively on it by pushing refs/replace).

And as you mention, it could be a heck of a lot faster than what we have
now.

-Peff

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 <redacted>
---
 builtin/replace.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 2336325..6a0e8bd 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.8.5.2.500.g8060133

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 <redacted>
---
 builtin/replace.c | 49 +++++++++++++++++++++++++------------------------
 1 file changed, 25 insertions(+), 24 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 6a0e8bd..0b5cb17 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_DELETE;
 
-	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.8.5.2.500.g8060133

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 <redacted>
---
 builtin/replace.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index 0b5cb17..a090302 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.8.5.2.500.g8060133

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 <redacted>
---
Besides missing docs and tests, we might find that we want to factor the
code a little differently when we start adding other helpers (like
"--graft"). I will probably push this forward at some point, but I'm not
planning on working on it for the rest of the day, so if you want to
pick it up as a base in the meantime and try "--graft", "--env-filter",
or anything else clever on top, please go ahead.

 builtin/replace.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 1 deletion(-)

diff --git a/builtin/replace.c b/builtin/replace.c
index a090302..3ed5f75 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,105 @@ 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[] = { "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[2] = 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");
+	} 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 +284,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 +307,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 +324,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.8.5.2.500.g8060133

From: "Jeff King" <redacted>

On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:

quoted

quoted

We can wrap that in "git replace --convert-grafts", but I do not 
think
grafts are so common that there would be a big demand for it.

It's probably easier to wrap it than to explain to Windows users what
they have to do.

How would Windows users get a graft file in the first-place? There's 
no
GUI for it! ;)

Now, now... It's dead easy using the git-gui and Notepad++, you can see 
and confirm the sha1's, copy and paste, and the graft file is a very 
easy format, so even wimps (windows, icons, menus, pointers aka mouse) 
folks can do it. (It worked for me when I needed it ;-)

The main point is that grafts are very easy to create [1], in that there 
is no object manipulation, while the replace mechanism does need a fresh 
object to be created that will 'replace' the old object. This 
manipulation can be perceived at least an awkward step. The replace 
mechanism needs to be at least as easy as the graft.

Something as simple as the 'git replace --graft $sha1 $parents' idea 
would make it very easy to deprecate the older graft process with this 
conceptually almost identical syntax. There are a few other 
documentation places that should also be updated when its sorted [2].

It should be easy to do "--convert-grafts", though, and I think it 
fits
into the scheme we're discussing below.

quoted

quoted

I think it would be nice to have a set of "mode" options for
"git-replace" to do basic editing of a sha1 and install the result
(technically you could split the editing into a separate command, 
but I
do not see the point in editing a sha1 and then _not_ replacing 
it).

If modifying without replacing is needed, it would be pretty easy to 
add
an option --stdout that writes the SHA1 of the modified object to 
stdout
instead of creating a replace reference.  That way what you want 95% 
of
the time is the default but there is still an escape hatch.

Agreed. I had originally though that perhaps something like this 
should
be part of "hash-object", and that "replace" should farm out the work.
But thinking on it more, it doesn't really make sense as part of
"hash-object".

quoted

quoted

Perhaps:

  # pretty-print sha1 based on type, start $EDITOR, create a
  # type-appropriate object from the result (e.g., using 
hash-object,
  # mktree, or mktag), and then set up the object as a replacement 
for
  # SHA1
  git replace --edit SHA1

Here's a rough series that gets us this far:

 [1/4]: replace: refactor command-mode determination
 [2/4]: replace: use OPT_CMDMODE to handle modes
 [3/4]: replace: factor object resolution out of replace_object
 [4/4]: replace: add --edit option

It shouldn't be too hard to do "--graft" or "--convert-grafts" on top.

I also noticed that doing:

   git replace foo foo

is less than friendly (we notice the cycle, but just barf). It's
especially easy to do with "git replace --edit", if you just exit the
editor without making changes.  Or if you make changes to an
already-replaced object to revert it back, in which case we would want
to notice and delete the replacement.

So I think we want to have "git replace foo foo" silently converted 
into
"git replace -d foo" (but without an error if there is no existing
replacement), and then "--edit" will just do the right thing, as it's
built on top.

I also noticed that the diff engine does not play well with 
replacements
of blobs. When we are diffing the trees, we see that the sha1 for path
"foo" is the same on either side, and do not look further, even though
feeding those sha1s to builtin_diff would fetch the replacements.  I
think compare_tree_entry would have to learn lookup_replace_object 
(and
I suspect it would make tree diffs noticeably slower when you have 
even
one replace ref).

quoted

"git replace" could support some of the options that "git 
filter-branch"
can take, like --env-filter, --msg-filter, etc. (at least if the 
target
is a commit object).

All of this would make it possible to build up the changes that you 
want
to integrate via "filter-branch" piecemeal instead of having to have 
a
single monster filter-branch invocation.  For example,

Right. I was tempted to suggest that, too, but I think it can get 
rather
tricky, as you need to replace in a loop, and sometimes the exact
objects you need aren't obvious.  For example, a common use of
"--index-filter" is to remove a single file. But to remove
"foo/bar/baz", you would need to loop over each commit, find the tree
for "foo/bar", and then remove the "baz" entry in

Still, I really like the workflow of having decent "replace" tools,
followed by "cementing" the changes into place with a "filter-branch"
run (which, btw, does not yet know how to cement trees and blobs into
place). It lets you work on the filtering incrementally, and even 
share
or work collaboratively on it by pushing refs/replace).

And as you mention, it could be a heck of a lot faster than what we 
have
now.

-Peff
--

Philip
[1] https://git.wiki.kernel.org/index.php/GraftPoint
[2] http://stackoverflow.com/q/6800692/717355 

On 03/07/2014 12:01 AM, Philip Oakley wrote:

From: "Jeff King" <redacted>

quoted

On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:

quoted

quoted

We can wrap that in "git replace --convert-grafts", but I do not >

think

quoted

grafts are so common that there would be a big demand for it.

It's probably easier to wrap it than to explain to Windows users what
they have to do.

How would Windows users get a graft file in the first-place? There's no
GUI for it! ;)

Now, now... It's dead easy using the git-gui and Notepad++, you can see
and confirm the sha1's, copy and paste, and the graft file is a very
easy format, so even wimps (windows, icons, menus, pointers aka mouse)
folks can do it. (It worked for me when I needed it ;-)

I didn't mean to insult all Windows users in general.  I was only
referring to the fact that since the default Windows command line is not
a POSIX shell, even an experienced Windows user might have trouble
figuring out how to execute a shell loop.  Putting this functionality in
a git command or script, by contrast, would make it work universally, no
fuss, no muss.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

From: "Michael Haggerty" <redacted>

On 03/07/2014 12:01 AM, Philip Oakley wrote:

quoted

From: "Jeff King" <redacted>

quoted

On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:

quoted

quoted

We can wrap that in "git replace --convert-grafts", but I do not 
 >

think

quoted

grafts are so common that there would be a big demand for it.

It's probably easier to wrap it than to explain to Windows users 
what
they have to do.

How would Windows users get a graft file in the first-place? There's 
no
GUI for it! ;)

Now, now... It's dead easy using the git-gui and Notepad++, you can 
see
and confirm the sha1's, copy and paste, and the graft file is a very
easy format, so even wimps (windows, icons, menus, pointers aka 
mouse)
folks can do it. (It worked for me when I needed it ;-)

I didn't mean to insult all Windows users in general.  I was only
referring to the fact that since the default Windows command line is 
not
a POSIX shell, even an experienced Windows user might have trouble
figuring out how to execute a shell loop.

I'd missed that aspect about the shell loop. I was mainly pointing out 
current awkwardness of creating the replace object, relative to grafts - 
There was an initial attempt by Christian, but it became quite hard to 
make it robust to sha1's embedded in commit messages.

     Putting this functionality in
a git command or script, by contrast, would make it work universally, 
no
fuss, no muss.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Thu, Mar 6, 2014 at 12:51 PM, Jeff King [off-list ref] wrote:

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.
+        */
+}
--
1.8.5.2.500.g8060133

On Thu, Mar 06, 2014 at 08:57:58PM -0500, Eric Sunshine wrote:

quoted

+               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);

Thanks for catching. I'll include it in any re-roll.

-Peff