[PATCH] disable grafts during fetch/push/bundle

[PATCH] disable grafts during fetch/push/bundle

[Jeff King]

When we are creating a pack to send to a remote, we should
make sure that we are not respecting grafts or replace refs.
Otherwise, we may end up sending a broken pack to the other
side that does not contain all objects (either omitting them
entirely, or using objects that the other side does not have
as delta bases).

We already make an attempt to do the right thing in several
places by turning off read_replace_refs. However, we missed
at least one case (during bundle creation), and we do
nothing anywhere to handle grafts.

This patch introduces a new function to disable both grafts
and replace refs both for the current process and for its
children. We add a call anywhere that packs objects for
sending: "bundle create", upload-pack, and push.

This may seem like a rather heavy hammer, as we could just
teach pack-objects not to respect grafts. But this catches
more possible failures:

  1. We may actually feed pack-objects with the results of
     rev-list (e.g., bundle creation does this).

  2. We may be negotiating the HAVEs and WANTs with the
     other side, and our view should be consistent with what
     we feed pack-objects.

  3. It may be useful to let pack-objects use grafts in some
     instances, as evidenced by --keep-true-parents.

Signed-off-by: Jeff King <peff@peff.net>
---
This is motivated by a real-world case of somebody trying to push to
GitHub with a graft on their local end.

I suspect many other spots that use "read_replace_refs = 0" probably
want to disable grafts, too (e.g., fsck and index-pack) but I do not
know of any particular breakage offhand.

I am tempted to say that not using --keep-true-parents is insane, and it
should be the default, but perhaps there is some case I'm missing.

Note that disabling grafts here just turns off .git/info/grafts, which
explicitly leaves the shallow file enabled (even though it ends up in
the same graft list). I'm assuming that the shallow file is handled
properly where it's appropriate, and it would not want to be included in
any of this disabling (certainly fetch/push should be handling it
explicitly already).

 builtin/push.c                   |  1 +
 bundle.c                         |  2 +
 cache.h                          |  1 +
 commit.c                         |  5 +++
 commit.h                         |  1 +
 environment.c                    | 22 ++++++++++
 t/t6051-replace-grafts-remote.sh | 94 ++++++++++++++++++++++++++++++++++++++++
 upload-pack.c                    |  2 +-
 8 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100755 t/t6051-replace-grafts-remote.sh

diff --git a/builtin/push.c b/builtin/push.c
index 0e50ddb..448dcb5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -527,6 +527,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
+	disable_grafts_and_replace_refs();
 	packet_trace_identity("push");
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, options, push_usage, 0);
diff --git a/bundle.c b/bundle.c
index e99065c..37b00a6 100644
--- a/bundle.c
+++ b/bundle.c
@@ -248,6 +248,8 @@ int create_bundle(struct bundle_header *header, const char *path,
 	struct child_process rls;
 	FILE *rls_fout;
 
+	disable_grafts_and_replace_refs();
+
 	bundle_to_stdout = !strcmp(path, "-");
 	if (bundle_to_stdout)
 		bundle_fd = 1;
diff --git a/cache.h b/cache.h
index db223e8..f538cc2 100644
--- a/cache.h
+++ b/cache.h
@@ -410,6 +410,7 @@ extern const char *get_git_work_tree(void);
 extern const char *read_gitfile(const char *path);
 extern const char *resolve_gitdir(const char *suspect);
 extern void set_git_work_tree(const char *tree);
+extern void disable_grafts_and_replace_refs(void);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
diff --git a/commit.c b/commit.c
index 6bf4fe0..886dbfe 100644
--- a/commit.c
+++ b/commit.c
@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)
 static struct commit_graft **commit_graft;
 static int commit_graft_alloc, commit_graft_nr;
 
+int commit_grafts_loaded(void)
+{
+	return !!commit_graft_nr;
+}
+
 static int commit_graft_pos(const unsigned char *sha1)
 {
 	int lo, hi;
diff --git a/commit.h b/commit.h
index 16d9c43..dde0618 100644
--- a/commit.h
+++ b/commit.h
@@ -186,6 +186,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 struct commit_graft *read_graft_line(char *buf, int len);
 int register_commit_graft(struct commit_graft *, int);
 struct commit_graft *lookup_commit_graft(const unsigned char *sha1);
+int commit_grafts_loaded(void);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos, int cleanup);
diff --git a/environment.c b/environment.c
index 4a3437d..b960293 100644
--- a/environment.c
+++ b/environment.c
@@ -284,3 +284,25 @@ const char *get_commit_output_encoding(void)
 {
 	return git_commit_encoding ? git_commit_encoding : "UTF-8";
 }
+
+/*
+ * In theory we could just set the environment variables to disable these
+ * features, but we may have lazily read them already. So we set the
+ * environment to cover further reads of them by ourselves or child processes,
+ * and also make sure to clear any state set by already reading them.
+ *
+ * Note that this covers the time between program start and when we first read
+ * a variable. Once you have actually loaded the grafts themselves into memory,
+ * it is too late (they are intermingled with shallow information). This should
+ * be OK, but let's die() as a fallback.
+ */
+void disable_grafts_and_replace_refs(void)
+{
+	setenv(NO_REPLACE_OBJECTS_ENVIRONMENT, "1", 1);
+	read_replace_refs = 0;
+	setenv(GRAFT_ENVIRONMENT, "/dev/null", 1);
+	git_graft_file = "/dev/null";
+
+	if (commit_grafts_loaded())
+		die("BUG: tried to disable grafts too late");
+}
diff --git a/t/t6051-replace-grafts-remote.sh b/t/t6051-replace-grafts-remote.sh
new file mode 100755
index 0000000..67f122d
--- /dev/null
+++ b/t/t6051-replace-grafts-remote.sh
@@ -0,0 +1,94 @@
+#!/bin/sh
+
+test_description='test interaction replace and grafts with fetch/push/bundles
+
+We have a simple divergent history like this:
+
+  A--B master
+   \
+    C--D  other
+
+In our remote, we have A and B, but not C or D.
+
+In our main repository, we create a graft or replace ref to look like this:
+
+  A--C--B
+
+The remote will claim to have B, but we must be sure to continue to send C; it
+is an ancestor of B only in our graft/replace view, not in reality.
+'
+. ./test-lib.sh
+
+make_repo () {
+	rm -rf "$1" &&
+	git init --bare "$1" &&
+	git push remote "$2"
+}
+
+replace () {
+	test_when_finished git replace -d "$1" &&
+	git replace "$1" "$2"
+}
+
+graft () {
+	test_when_finished "rm -f .git/info/grafts" &&
+	echo "$1 $2" >.git/info/grafts
+}
+
+test_expect_success 'create commits' '
+	test_commit A &&
+	test_commit B &&
+	git checkout -b other A &&
+	test_commit C &&
+	test_commit D &&
+	A=$(git rev-parse --verify A) &&
+	B=$(git rev-parse --verify B) &&
+	C=$(git rev-parse --verify C) &&
+	D=$(git rev-parse --verify D)
+'
+
+test_expect_success 'create replace object' '
+	git cat-file commit B >commit.in &&
+	sed "s/$A/$C/" <commit.in >commit.out &&
+	replace=$(git hash-object -w -t commit commit.out)
+'
+
+test_expect_success 'push with replace object' '
+	make_repo remote B &&
+	replace "$B" "$replace" &&
+	git push remote D
+'
+
+test_expect_success 'fetch with replace object' '
+	make_repo remote B &&
+	replace "$B" "$replace" &&
+	(cd remote && git fetch .. D)
+'
+
+test_expect_success 'bundle with replace object' '
+	make_repo remote B &&
+	replace "$B" "$replace" &&
+	git bundle create foo.bundle ^B D &&
+	(cd remote && git fetch ../foo.bundle D)
+'
+
+test_expect_success 'push with graft' '
+	make_repo remote B &&
+	graft "$B" "$C" &&
+	git push remote D
+'
+
+test_expect_success 'fetch with graft' '
+	make_repo remote B &&
+	graft "$B" "$C" &&
+	(cd remote && git fetch .. D)
+'
+
+test_expect_success 'bundle with graft' '
+	make_repo remote B &&
+	graft "$B" "$C" &&
+	git bundle create foo.bundle ^B D &&
+	(cd remote && git fetch ../foo.bundle D)
+'
+
+test_done
diff --git a/upload-pack.c b/upload-pack.c
index 0c44f6b..a72e4fa 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -796,7 +796,7 @@ int main(int argc, char **argv)
 
 	packet_trace_identity("upload-pack");
 	git_extract_argv0_path(argv[0]);
-	read_replace_refs = 0;
+	disable_grafts_and_replace_refs();
 
 	for (i = 1; i < argc; i++) {
 		char *arg = argv[i];
-- 
1.8.5.2.500.g8060133

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> When we are creating a pack to send to a remote, we should
> make sure that we are not respecting grafts or replace refs.
> Otherwise, we may end up sending a broken pack to the other
> side that does not contain all objects (either omitting them
> entirely, or using objects that the other side does not have
> as delta bases).
>
> We already make an attempt to do the right thing in several
> places by turning off read_replace_refs. However, we missed
> at least one case (during bundle creation), and we do
> nothing anywhere to handle grafts.

"Doing nothing for grafts" has been pretty much a deliberate
omission.  Because we have no way to transfer how histories are
grafted together, people cloning from a repository that grafts away
a commit that records a mistakenly committed sekrit will end up with
a disjoint history, instead of exposing the sekrit to them, and are
expected to join the history by recreating grafts (perhaps a README
of such a project instructs them to do so).  That was deemed far
better than exposing the hidden history, I think.

And "replace tries to do the right thing" was an attempt to rectify
that misfeature of grafts in that we now do have a way to transfer
how the history is grafted together, so that project README does not
have to instruct the fetcher of doing anything special.

It _might_ be a misfeature, however, for the object connectivity
layer to expose a part of the history replaced away to the party
that fetches from such a repository.  Ideally, the "right thing"
ought to be to include history that would be omitted if we did not
have the replacement (i.e. adding parents the underlying commit does
not record), while not following the history that replacement wants
to hide (i.e. excluding the commits replacement commits overlay).

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

[Eric Sunshine]

On Tue, Mar 4, 2014 at 12:48 PM, Jeff King <peff@peff.net> wrote:
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..886dbfe 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)
>  static struct commit_graft **commit_graft;
>  static int commit_graft_alloc, commit_graft_nr;
>
> +int commit_grafts_loaded(void)
> +{
> +       return !!commit_graft_nr;
> +}

Did you mean !!commit_graft ?

>  static int commit_graft_pos(const unsigned char *sha1)
>  {
>         int lo, hi;
> --
> 1.8.5.2.500.g8060133

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

[Jeff King]

On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:

> On Tue, Mar 4, 2014 at 12:48 PM, Jeff King <peff@peff.net> wrote:
> > diff --git a/commit.c b/commit.c
> > index 6bf4fe0..886dbfe 100644
> > --- a/commit.c
> > +++ b/commit.c
> > @@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)
> >  static struct commit_graft **commit_graft;
> >  static int commit_graft_alloc, commit_graft_nr;
> >
> > +int commit_grafts_loaded(void)
> > +{
> > +       return !!commit_graft_nr;
> > +}
> 
> Did you mean !!commit_graft ?

Shouldn't they produce the same results?

-Peff

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

[Jeff King]

On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote:

> > We already make an attempt to do the right thing in several
> > places by turning off read_replace_refs. However, we missed
> > at least one case (during bundle creation), and we do
> > nothing anywhere to handle grafts.
> 
> "Doing nothing for grafts" has been pretty much a deliberate
> omission.  Because we have no way to transfer how histories are
> grafted together, people cloning from a repository that grafts away
> a commit that records a mistakenly committed sekrit will end up with
> a disjoint history, instead of exposing the sekrit to them, and are
> expected to join the history by recreating grafts (perhaps a README
> of such a project instructs them to do so).  That was deemed far
> better than exposing the hidden history, I think.

I see your point, but I would be tempted to say that the person trying
to hide a secret with grafting is simply wrong to do so. You need to
cement that history with a rewrite if you want to share with people.

I do not recall any past discussion on this topic, and searching the
archive only shows people echoing what I said above. Is this something
we've promised to work in the past?

I'm certainly sympathetic to systems failing to a secure default rather
than doing something that the user does not expect. But at the same
time, if using grafts for security isn't something people reasonably
expect, then failing only hurts the non-security cases.

> And "replace tries to do the right thing" was an attempt to rectify
> that misfeature of grafts in that we now do have a way to transfer
> how the history is grafted together, so that project README does not
> have to instruct the fetcher of doing anything special.

Perhaps the right response is "grafts are broken, use git-replace
instead". But then should we think about deprecating grafts? Again, this
patch was spurred by a real user with a graft trying to push and getting
a confusing error message.

> It _might_ be a misfeature, however, for the object connectivity
> layer to expose a part of the history replaced away to the party
> that fetches from such a repository.  Ideally, the "right thing"
> ought to be to include history that would be omitted if we did not
> have the replacement (i.e. adding parents the underlying commit does
> not record), while not following the history that replacement wants
> to hide (i.e. excluding the commits replacement commits overlay).

I don't really think it's worth the complexity. It's fairly common
knowledge (or at least I think so) that replace refs are a _view_ onto
the history. When you share the history graph, you share the true
objects. You can _also_ share your views in replace/refs, but it is up
to the client to fetch them. If you want to hide things, then you need
to rewrite the true objects, end of story.

I dunno. Maybe there are people who have different expectations.

-Peff

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

[Eric Sunshine]

On Tue, Mar 4, 2014 at 7:37 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:
>
>> On Tue, Mar 4, 2014 at 12:48 PM, Jeff King <peff@peff.net> wrote:
>> > diff --git a/commit.c b/commit.c
>> > index 6bf4fe0..886dbfe 100644
>> > --- a/commit.c
>> > +++ b/commit.c
>> > @@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)
>> >  static struct commit_graft **commit_graft;
>> >  static int commit_graft_alloc, commit_graft_nr;
>> >
>> > +int commit_grafts_loaded(void)
>> > +{
>> > +       return !!commit_graft_nr;
>> > +}
>>
>> Did you mean !!commit_graft ?
>
> Shouldn't they produce the same results?

Yes they should, but the use of !! seemed to imply that you wanted to
apply it to the pointer value. (If you indeed intended to use
commit_graft_nr, then 'return commit_graft_nr', without !!, would have
been sufficient and idiomatic C.)

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

[Jeff King]

On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:

> >> > +int commit_grafts_loaded(void)
> >> > +{
> >> > +       return !!commit_graft_nr;
> >> > +}
> >>
> >> Did you mean !!commit_graft ?
> >
> > Shouldn't they produce the same results?
> 
> Yes they should, but the use of !! seemed to imply that you wanted to
> apply it to the pointer value. (If you indeed intended to use
> commit_graft_nr, then 'return commit_graft_nr', without !!, would have
> been sufficient and idiomatic C.)

I just wanted to normalize the return value to a boolean 0/1. Even when
the input is an int, it eliminates surprises when you try to assign the
result to a bitfield or other smaller-width type.

-Peff

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

[Eric Sunshine]

On Tue, Mar 4, 2014 at 8:05 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:
>
>> >> > +int commit_grafts_loaded(void)
>> >> > +{
>> >> > +       return !!commit_graft_nr;
>> >> > +}
>> >>
>> >> Did you mean !!commit_graft ?
>> >
>> > Shouldn't they produce the same results?
>>
>> Yes they should, but the use of !! seemed to imply that you wanted to
>> apply it to the pointer value. (If you indeed intended to use
>> commit_graft_nr, then 'return commit_graft_nr', without !!, would have
>> been sufficient and idiomatic C.)
>
> I just wanted to normalize the return value to a boolean 0/1. Even when
> the input is an int, it eliminates surprises when you try to assign the
> result to a bitfield or other smaller-width type.

Thanks for the explanation.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote:
>
> I do not recall any past discussion on this topic, and searching the
> archive only shows people echoing what I said above. Is this something
> we've promised to work in the past?

The history lesson is coming solely from my recollection of a
discussion I and Linus had on the list back when we were doing the
original "graft" and thinking about the interaction between it and
the object/history transfer; especially the "only add new ones, but
hide the ones that the user wants to be hidden" part is something
suggested by Linus but we couldn't implement back then, IIRC.

> Perhaps the right response is "grafts are broken, use git-replace
> instead". But then should we think about deprecating grafts?

I am sort of surprised to hear that question, actually ;-)

I didn't say that in the message you are responding to because I
somehow thought that everybody has been in agreement with these two
lines for a long while.  Ever since I suggested the "replace" as an
alternative "grafts done right" and outlined how it should work to
Christian while sitting next to him in one of the early GitTogether,
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".

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

[Jeff King]

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

> > Perhaps the right response is "grafts are broken, use git-replace
> > instead". But then should we think about deprecating grafts?
> 
> I am sort of surprised to hear that question, actually ;-)
> 
> I didn't say that in the message you are responding to because I
> somehow thought that everybody has been in agreement with these two
> lines for a long while.  Ever since I suggested the "replace" as an
> alternative "grafts done right" and outlined how it should work to
> Christian while sitting next to him in one of the early GitTogether,
> 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?".

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:
>
>> ... 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.

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

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:
> >
> >> ... 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 <peff@peff.net>
---
 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote:
>
>> 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.

Thanks; I agree with the reasoning.

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

[Michael Haggerty]

On 03/05/2014 08:18 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:
>>
>>> ... 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/

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

[Christian Couder]

On Thu, Mar 6, 2014 at 9:42 AM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
> On 03/05/2014 08:18 PM, Junio C Hamano wrote:
>> Jeff King <peff@peff.net> writes:
>>
>>> On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:
>>>
>>>> ... 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.

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

[Jeff King]

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

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

[Michael Haggerty]

On 03/06/2014 04:56 PM, Jeff King wrote:
> 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.

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

>> * 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/

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

[Jeff King]

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

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

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

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

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

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

[Jeff King]

On Thu, Mar 06, 2014 at 07:35:19PM +0100, Christian Couder wrote:

> > +       if (!cmdmode)
> > +               cmdmode = argc ? MODE_REPLACE : MODE_DELETE;
> 
> Shouldn't it be MODE_LIST instead of MODE_DELETE?

Argh, yes, thank you for catching. My original iteration used chars like
'd' and 'l' (similar to other uses of OPT_CMDMODE). I switched it at the
last minute to an enum, and somehow thinko'd that completely (and
obviously did not run the tests again afterwards).

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

Sorry, I do not quite understand.

In "git diff A B -- path", if the object name recorded for A:path
and B:path are the same, but the replacement mechanism maps the
object name for that blob object to some other blob object, wouldn't
the result be empty because both sides replace to the same thing
anyway?

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

[Jeff King]

On Thu, Mar 06, 2014 at 11:00:08AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> Sorry, I do not quite understand.
> 
> In "git diff A B -- path", if the object name recorded for A:path
> and B:path are the same, but the replacement mechanism maps the
> object name for that blob object to some other blob object, wouldn't
> the result be empty because both sides replace to the same thing
> anyway?

Oh, right, I was being stupid. I did:

  git replace --edit HEAD:some-file

and expected "git show" to find the diff. But that doesn't make sense.
On top of that, I need to do:

  git replace --edit HEAD^{tree}

to replace the sha1 of the entry in the tree. In which case diff would
find it just fine.

-Peff

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

[Philip Oakley]

From: "Jeff King" <peff@peff.net>
> On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:
>
>> > 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.
>
>> > 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".
>
>> > 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
> --

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

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

[Michael Haggerty]

On 03/07/2014 12:01 AM, Philip Oakley wrote:
> From: "Jeff King" <peff@peff.net>
>> On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:
>>
>>> > 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 ;-)

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/

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

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

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

;-)

Be it graft or replace, I do not think we want to invite people to
use these mechansims too lightly to locally rewrite their history
willy-nilly without fixing their mistakes at the object layer with
"commit --amend", "rebase", "bfg", etc. in the longer term.  So in
that sense, adding a command to make it easy is not something I am
enthusiastic about.

On the other hand, if the user does need to use graft or replace
(perhaps to prepare for casting the fixed history in stone with
filter-branch), it would be good to help them avoid making mistakes
while doing so and tool support may be a way to do so.

So, ... I am of two minds.

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

[Philip Oakley]

From: "Michael Haggerty" <mhagger@alum.mit.edu>
> On 03/07/2014 12:01 AM, Philip Oakley wrote:
>> From: "Jeff King" <peff@peff.net>
>>> On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:
>>>
>>>> > 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 ;-)
>
> 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/
> 

Re: [RFC/PATCH 4/4] replace: add --edit option

[Eric Sunshine]

On Thu, Mar 6, 2014 at 12:51 PM, Jeff King <peff@peff.net> wrote:
> 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.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/replace.c b/builtin/replace.c
> index a090302..3ed5f75 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -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);
>  }
>
> +/*
> + * 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.
> +        */
> +}
> --
> 1.8.5.2.500.g8060133

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

[Christian Couder]

On Fri, Mar 7, 2014 at 12:39 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> 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.
>
> ;-)
>
> Be it graft or replace, I do not think we want to invite people to
> use these mechansims too lightly to locally rewrite their history
> willy-nilly without fixing their mistakes at the object layer with
> "commit --amend", "rebase", "bfg", etc. in the longer term.  So in
> that sense, adding a command to make it easy is not something I am
> enthusiastic about.
>
> On the other hand, if the user does need to use graft or replace
> (perhaps to prepare for casting the fixed history in stone with
> filter-branch), it would be good to help them avoid making mistakes
> while doing so and tool support may be a way to do so.
>
> So, ... I am of two minds.
>

Maybe if we add a new command (or maybe a script) with a name long and
cryptic-looking enough like "git create-replacement-object" it will
scare casual users from touching it, while power users will be happy
to benefit from it.

Re: [RFC/PATCH 4/4] replace: add --edit option

[Jeff King]

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

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

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

[Jeff King]

On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote:

> > Be it graft or replace, I do not think we want to invite people to
> > use these mechansims too lightly to locally rewrite their history
> > willy-nilly without fixing their mistakes at the object layer with
> > "commit --amend", "rebase", "bfg", etc. in the longer term.  So in
> > that sense, adding a command to make it easy is not something I am
> > enthusiastic about.
> >
> > On the other hand, if the user does need to use graft or replace
> > (perhaps to prepare for casting the fixed history in stone with
> > filter-branch), it would be good to help them avoid making mistakes
> > while doing so and tool support may be a way to do so.
> >
> > So, ... I am of two minds.
> 
> Maybe if we add a new command (or maybe a script) with a name long and
> cryptic-looking enough like "git create-replacement-object" it will
> scare casual users from touching it, while power users will be happy
> to benefit from it.

I do not think the features we are talking about are significantly more
dangerous than "git replace" is in the first place. If we want to make
people aware of the dangers, perhaps git-replace.1 is the right place to
do it.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote:
>
>> > Be it graft or replace, I do not think we want to invite people to
>> > use these mechansims too lightly to locally rewrite their history
>> > willy-nilly without fixing their mistakes at the object layer with
>> > "commit --amend", "rebase", "bfg", etc. in the longer term.  So in
>> > that sense, adding a command to make it easy is not something I am
>> > enthusiastic about.
>> >
>> > On the other hand, if the user does need to use graft or replace
>> > (perhaps to prepare for casting the fixed history in stone with
>> > filter-branch), it would be good to help them avoid making mistakes
>> > while doing so and tool support may be a way to do so.
>> >
>> > So, ... I am of two minds.
> ...
> I do not think the features we are talking about are significantly more
> dangerous than "git replace" is in the first place. If we want to make
> people aware of the dangers, perhaps git-replace.1 is the right place to
> do it.

Sure.

So should we take the four-patch series for "git replace --edit"?

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

[Jeff King]

On Wed, Mar 19, 2014 at 03:39:42PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote:
> >
> >> > Be it graft or replace, I do not think we want to invite people to
> >> > use these mechansims too lightly to locally rewrite their history
> >> > willy-nilly without fixing their mistakes at the object layer with
> >> > "commit --amend", "rebase", "bfg", etc. in the longer term.  So in
> >> > that sense, adding a command to make it easy is not something I am
> >> > enthusiastic about.
> >> >
> >> > On the other hand, if the user does need to use graft or replace
> >> > (perhaps to prepare for casting the fixed history in stone with
> >> > filter-branch), it would be good to help them avoid making mistakes
> >> > while doing so and tool support may be a way to do so.
> >> >
> >> > So, ... I am of two minds.
> > ...
> > I do not think the features we are talking about are significantly more
> > dangerous than "git replace" is in the first place. If we want to make
> > people aware of the dangers, perhaps git-replace.1 is the right place to
> > do it.
> 
> Sure.
> 
> So should we take the four-patch series for "git replace --edit"?

I think that is certainly going in the right direction, but it is
missing documentation and tests still. Aside from a one-liner bug (which
Christian pointed out on the list), I do not think it will _hurt_
anybody. But it probably should be "finished" before seeing the light of
day. I'd be happy if you wanted to pick it up for "pu" or even "next"
waiting and do that finishing in-tree.

Otherwise, I may eventually get to it and re-roll the whole completed
series.

-Peff