{"thread":{"id":"36053","subject":"[PATCH] disable grafts during fetch/push/bundle","startedAt":"2014-03-04T17:48:07Z","lastAt":"2014-03-21T00:49:06Z","messageCount":35,"participants":["Jeff King","Junio C Hamano","Eric Sunshine","Michael Haggerty","Christian Couder","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"236011","messageId":"20140304174806.GA11561@sigill.intra.peff.net","threadId":"36053","inReplyTo":null,"subject":"[PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-04T17:48:07Z","receivedAt":"2014-03-04T17:48:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are creating a pack to send to a remote, we should\nmake sure that we are not respecting grafts or replace refs.\nOtherwise, we may end up sending a broken pack to the other\nside that does not contain all objects (either omitting them\nentirely, or using objects that the other side does not have\nas delta bases).\n\nWe already make an attempt to do the right thing in several\nplaces by turning off read_replace_refs. However, we missed\nat least one case (during bundle creation), and we do\nnothing anywhere to handle grafts.\n\nThis patch introduces a new function to disable both grafts\nand replace refs both for the current process and for its\nchildren. We add a call anywhere that packs objects for\nsending: \"bundle create\", upload-pack, and push.\n\nThis may seem like a rather heavy hammer, as we could just\nteach pack-objects not to respect grafts. But this catches\nmore possible failures:\n\n 1. We may actually feed pack-objects with the results of\n rev-list (e.g., bundle creation does this).\n\n 2. We may be negotiating the HAVEs and WANTs with the\n other side, and our view should be consistent with what\n we feed pack-objects.\n\n 3. It may be useful to let pack-objects use grafts in some\n instances, as evidenced by --keep-true-parents.\n\nSigned-off-by: Jeff King \n---\nThis is motivated by a real-world case of somebody trying to push to\nGitHub with a graft on their local end.\n\nI suspect many other spots that use \"read_replace_refs = 0\" probably\nwant to disable grafts, too (e.g., fsck and index-pack) but I do not\nknow of any particular breakage offhand.\n\nI am tempted to say that not using --keep-true-parents is insane, and it\nshould be the default, but perhaps there is some case I'm missing.\n\nNote that disabling grafts here just turns off .git/info/grafts, which\nexplicitly leaves the shallow file enabled (even though it ends up in\nthe same graft list). I'm assuming that the shallow file is handled\nproperly where it's appropriate, and it would not want to be included in\nany of this disabling (certainly fetch/push should be handling it\nexplicitly already).\n\n builtin/push.c | 1 +\n bundle.c | 2 +\n cache.h | 1 +\n commit.c | 5 +++\n commit.h | 1 +\n environment.c | 22 ++++++++++\n t/t6051-replace-grafts-remote.sh | 94 ++++++++++++++++++++++++++++++++++++++++\n upload-pack.c | 2 +-\n 8 files changed, 127 insertions(+), 1 deletion(-)\n create mode 100755 t/t6051-replace-grafts-remote.sh\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 0e50ddb..448dcb5 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -527,6 +527,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\tOPT_END()\n \t};\n \n+\tdisable_grafts_and_replace_refs();\n \tpacket_trace_identity(\"push\");\n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, options, push_usage, 0);\ndiff --git a/bundle.c b/bundle.c\nindex e99065c..37b00a6 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -248,6 +248,8 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tstruct child_process rls;\n \tFILE *rls_fout;\n \n+\tdisable_grafts_and_replace_refs();\n+\n \tbundle_to_stdout = !strcmp(path, \"-\");\n \tif (bundle_to_stdout)\n \t\tbundle_fd = 1;\ndiff --git a/cache.h b/cache.h\nindex db223e8..f538cc2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -410,6 +410,7 @@ extern const char *get_git_work_tree(void);\n extern const char *read_gitfile(const char *path);\n extern const char *resolve_gitdir(const char *suspect);\n extern void set_git_work_tree(const char *tree);\n+extern void disable_grafts_and_replace_refs(void);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \ndiff --git a/commit.c b/commit.c\nindex 6bf4fe0..886dbfe 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)\n static struct commit_graft **commit_graft;\n static int commit_graft_alloc, commit_graft_nr;\n \n+int commit_grafts_loaded(void)\n+{\n+\treturn !!commit_graft_nr;\n+}\n+\n static int commit_graft_pos(const unsigned char *sha1)\n {\n \tint lo, hi;\ndiff --git a/commit.h b/commit.h\nindex 16d9c43..dde0618 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -186,6 +186,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);\n struct commit_graft *read_graft_line(char *buf, int len);\n int register_commit_graft(struct commit_graft *, int);\n struct commit_graft *lookup_commit_graft(const unsigned char *sha1);\n+int commit_grafts_loaded(void);\n \n extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);\n extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos, int cleanup);\ndiff --git a/environment.c b/environment.c\nindex 4a3437d..b960293 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -284,3 +284,25 @@ const char *get_commit_output_encoding(void)\n {\n \treturn git_commit_encoding ? git_commit_encoding : \"UTF-8\";\n }\n+\n+/*\n+ * In theory we could just set the environment variables to disable these\n+ * features, but we may have lazily read them already. So we set the\n+ * environment to cover further reads of them by ourselves or child processes,\n+ * and also make sure to clear any state set by already reading them.\n+ *\n+ * Note that this covers the time between program start and when we first read\n+ * a variable. Once you have actually loaded the grafts themselves into memory,\n+ * it is too late (they are intermingled with shallow information). This should\n+ * be OK, but let's die() as a fallback.\n+ */\n+void disable_grafts_and_replace_refs(void)\n+{\n+\tsetenv(NO_REPLACE_OBJECTS_ENVIRONMENT, \"1\", 1);\n+\tread_replace_refs = 0;\n+\tsetenv(GRAFT_ENVIRONMENT, \"/dev/null\", 1);\n+\tgit_graft_file = \"/dev/null\";\n+\n+\tif (commit_grafts_loaded())\n+\t\tdie(\"BUG: tried to disable grafts too late\");\n+}\ndiff --git a/t/t6051-replace-grafts-remote.sh b/t/t6051-replace-grafts-remote.sh\nnew file mode 100755\nindex 0000000..67f122d\n--- /dev/null\n+++ b/t/t6051-replace-grafts-remote.sh\n@@ -0,0 +1,94 @@\n+#!/bin/sh\n+\n+test_description='test interaction replace and grafts with fetch/push/bundles\n+\n+We have a simple divergent history like this:\n+\n+ A--B master\n+ \\\n+ C--D other\n+\n+In our remote, we have A and B, but not C or D.\n+\n+In our main repository, we create a graft or replace ref to look like this:\n+\n+ A--C--B\n+\n+The remote will claim to have B, but we must be sure to continue to send C; it\n+is an ancestor of B only in our graft/replace view, not in reality.\n+'\n+. ./test-lib.sh\n+\n+make_repo () {\n+\trm -rf \"$1\" &&\n+\tgit init --bare \"$1\" &&\n+\tgit push remote \"$2\"\n+}\n+\n+replace () {\n+\ttest_when_finished git replace -d \"$1\" &&\n+\tgit replace \"$1\" \"$2\"\n+}\n+\n+graft () {\n+\ttest_when_finished \"rm -f .git/info/grafts\" &&\n+\techo \"$1 $2\" >.git/info/grafts\n+}\n+\n+test_expect_success 'create commits' '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tgit checkout -b other A &&\n+\ttest_commit C &&\n+\ttest_commit D &&\n+\tA=$(git rev-parse --verify A) &&\n+\tB=$(git rev-parse --verify B) &&\n+\tC=$(git rev-parse --verify C) &&\n+\tD=$(git rev-parse --verify D)\n+'\n+\n+test_expect_success 'create replace object' '\n+\tgit cat-file commit B >commit.in &&\n+\tsed \"s/$A/$C/\" commit.out &&\n+\treplace=$(git hash-object -w -t commit commit.out)\n+'\n+\n+test_expect_success 'push with replace object' '\n+\tmake_repo remote B &&\n+\treplace \"$B\" \"$replace\" &&\n+\tgit push remote D\n+'\n+\n+test_expect_success 'fetch with replace object' '\n+\tmake_repo remote B &&\n+\treplace \"$B\" \"$replace\" &&\n+\t(cd remote && git fetch .. D)\n+'\n+\n+test_expect_success 'bundle with replace object' '\n+\tmake_repo remote B &&\n+\treplace \"$B\" \"$replace\" &&\n+\tgit bundle create foo.bundle ^B D &&\n+\t(cd remote && git fetch ../foo.bundle D)\n+'\n+\n+test_expect_success 'push with graft' '\n+\tmake_repo remote B &&\n+\tgraft \"$B\" \"$C\" &&\n+\tgit push remote D\n+'\n+\n+test_expect_success 'fetch with graft' '\n+\tmake_repo remote B &&\n+\tgraft \"$B\" \"$C\" &&\n+\t(cd remote && git fetch .. D)\n+'\n+\n+test_expect_success 'bundle with graft' '\n+\tmake_repo remote B &&\n+\tgraft \"$B\" \"$C\" &&\n+\tgit bundle create foo.bundle ^B D &&\n+\t(cd remote && git fetch ../foo.bundle D)\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 0c44f6b..a72e4fa 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -796,7 +796,7 @@ int main(int argc, char **argv)\n \n \tpacket_trace_identity(\"upload-pack\");\n \tgit_extract_argv0_path(argv[0]);\n-\tread_replace_refs = 0;\n+\tdisable_grafts_and_replace_refs();\n \n \tfor (i = 1; i < argc; i++) {\n \t\tchar *arg = argv[i];\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236039","messageId":"xmqqd2i1k7p9.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"20140304174806.GA11561@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T20:52:18Z","receivedAt":"2014-03-04T20:52:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> When we are creating a pack to send to a remote, we should\n> make sure that we are not respecting grafts or replace refs.\n> Otherwise, we may end up sending a broken pack to the other\n> side that does not contain all objects (either omitting them\n> entirely, or using objects that the other side does not have\n> as delta bases).\n>\n> We already make an attempt to do the right thing in several\n> places by turning off read_replace_refs. However, we missed\n> at least one case (during bundle creation), and we do\n> nothing anywhere to handle grafts.\n\n\"Doing nothing for grafts\" has been pretty much a deliberate\nomission. Because we have no way to transfer how histories are\ngrafted together, people cloning from a repository that grafts away\na commit that records a mistakenly committed sekrit will end up with\na disjoint history, instead of exposing the sekrit to them, and are\nexpected to join the history by recreating grafts (perhaps a README\nof such a project instructs them to do so). That was deemed far\nbetter than exposing the hidden history, I think.\n\nAnd \"replace tries to do the right thing\" was an attempt to rectify\nthat misfeature of grafts in that we now do have a way to transfer\nhow the history is grafted together, so that project README does not\nhave to instruct the fetcher of doing anything special.\n\nIt _might_ be a misfeature, however, for the object connectivity\nlayer to expose a part of the history replaced away to the party\nthat fetches from such a repository. Ideally, the \"right thing\"\nought to be to include history that would be omitted if we did not\nhave the replacement (i.e. adding parents the underlying commit does\nnot record), while not following the history that replacement wants\nto hide (i.e. excluding the commits replacement commits overlay).\n"},{"id":"236055","messageId":"CAPig+cTzOxn5t0JOwxQyVrU___QPAQvwHR-zqFccaEQw+mYUCA@mail.gmail.com","threadId":"36053","inReplyTo":"20140304174806.GA11561@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-04T23:36:07Z","receivedAt":"2014-03-04T23:36:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 4, 2014 at 12:48 PM, Jeff King wrote:\n> diff --git a/commit.c b/commit.c\n> index 6bf4fe0..886dbfe 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)\n> static struct commit_graft **commit_graft;\n> static int commit_graft_alloc, commit_graft_nr;\n>\n> +int commit_grafts_loaded(void)\n> +{\n> + return !!commit_graft_nr;\n> +}\n\nDid you mean !!commit_graft ?\n\n> static int commit_graft_pos(const unsigned char *sha1)\n> {\n> int lo, hi;\n> --\n> 1.8.5.2.500.g8060133\n"},{"id":"236059","messageId":"20140305003721.GA11509@sigill.intra.peff.net","threadId":"36053","inReplyTo":"CAPig+cTzOxn5t0JOwxQyVrU___QPAQvwHR-zqFccaEQw+mYUCA@mail.gmail.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T00:37:22Z","receivedAt":"2014-03-05T00:37:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:\n\n> On Tue, Mar 4, 2014 at 12:48 PM, Jeff King wrote:\n> > diff --git a/commit.c b/commit.c\n> > index 6bf4fe0..886dbfe 100644\n> > --- a/commit.c\n> > +++ b/commit.c\n> > @@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)\n> > static struct commit_graft **commit_graft;\n> > static int commit_graft_alloc, commit_graft_nr;\n> >\n> > +int commit_grafts_loaded(void)\n> > +{\n> > + return !!commit_graft_nr;\n> > +}\n> \n> Did you mean !!commit_graft ?\n\nShouldn't they produce the same results?\n\n-Peff\n"},{"id":"236060","messageId":"20140305005649.GB11509@sigill.intra.peff.net","threadId":"36053","inReplyTo":"xmqqd2i1k7p9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T00:56:49Z","receivedAt":"2014-03-05T00:56:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote:\n\n> > We already make an attempt to do the right thing in several\n> > places by turning off read_replace_refs. However, we missed\n> > at least one case (during bundle creation), and we do\n> > nothing anywhere to handle grafts.\n> \n> \"Doing nothing for grafts\" has been pretty much a deliberate\n> omission. Because we have no way to transfer how histories are\n> grafted together, people cloning from a repository that grafts away\n> a commit that records a mistakenly committed sekrit will end up with\n> a disjoint history, instead of exposing the sekrit to them, and are\n> expected to join the history by recreating grafts (perhaps a README\n> of such a project instructs them to do so). That was deemed far\n> better than exposing the hidden history, I think.\n\nI see your point, but I would be tempted to say that the person trying\nto hide a secret with grafting is simply wrong to do so. You need to\ncement that history with a rewrite if you want to share with people.\n\nI do not recall any past discussion on this topic, and searching the\narchive only shows people echoing what I said above. Is this something\nwe've promised to work in the past?\n\nI'm certainly sympathetic to systems failing to a secure default rather\nthan doing something that the user does not expect. But at the same\ntime, if using grafts for security isn't something people reasonably\nexpect, then failing only hurts the non-security cases.\n\n> And \"replace tries to do the right thing\" was an attempt to rectify\n> that misfeature of grafts in that we now do have a way to transfer\n> how the history is grafted together, so that project README does not\n> have to instruct the fetcher of doing anything special.\n\nPerhaps the right response is \"grafts are broken, use git-replace\ninstead\". But then should we think about deprecating grafts? Again, this\npatch was spurred by a real user with a graft trying to push and getting\na confusing error message.\n\n> It _might_ be a misfeature, however, for the object connectivity\n> layer to expose a part of the history replaced away to the party\n> that fetches from such a repository. Ideally, the \"right thing\"\n> ought to be to include history that would be omitted if we did not\n> have the replacement (i.e. adding parents the underlying commit does\n> not record), while not following the history that replacement wants\n> to hide (i.e. excluding the commits replacement commits overlay).\n\nI don't really think it's worth the complexity. It's fairly common\nknowledge (or at least I think so) that replace refs are a _view_ onto\nthe history. When you share the history graph, you share the true\nobjects. You can _also_ share your views in replace/refs, but it is up\nto the client to fetch them. If you want to hide things, then you need\nto rewrite the true objects, end of story.\n\nI dunno. Maybe there are people who have different expectations.\n\n-Peff\n"},{"id":"236061","messageId":"CAPig+cRqyG_JXSO-DsdP0mAjfmgS+FUCxLz+0+rHeUXrjmcjJg@mail.gmail.com","threadId":"36053","inReplyTo":"20140305003721.GA11509@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-05T01:00:44Z","receivedAt":"2014-03-05T01:00:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 4, 2014 at 7:37 PM, Jeff King wrote:\n> On Tue, Mar 04, 2014 at 06:36:07PM -0500, Eric Sunshine wrote:\n>\n>> On Tue, Mar 4, 2014 at 12:48 PM, Jeff King wrote:\n>> > diff --git a/commit.c b/commit.c\n>> > index 6bf4fe0..886dbfe 100644\n>> > --- a/commit.c\n>> > +++ b/commit.c\n>> > @@ -114,6 +114,11 @@ static unsigned long parse_commit_date(const char *buf, const char *tail)\n>> > static struct commit_graft **commit_graft;\n>> > static int commit_graft_alloc, commit_graft_nr;\n>> >\n>> > +int commit_grafts_loaded(void)\n>> > +{\n>> > + return !!commit_graft_nr;\n>> > +}\n>>\n>> Did you mean !!commit_graft ?\n>\n> Shouldn't they produce the same results?\n\nYes they should, but the use of !! seemed to imply that you wanted to\napply it to the pointer value. (If you indeed intended to use\ncommit_graft_nr, then 'return commit_graft_nr', without !!, would have\nbeen sufficient and idiomatic C.)\n"},{"id":"236062","messageId":"20140305010529.GA11923@sigill.intra.peff.net","threadId":"36053","inReplyTo":"CAPig+cRqyG_JXSO-DsdP0mAjfmgS+FUCxLz+0+rHeUXrjmcjJg@mail.gmail.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T01:05:29Z","receivedAt":"2014-03-05T01:05:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:\n\n> >> > +int commit_grafts_loaded(void)\n> >> > +{\n> >> > + return !!commit_graft_nr;\n> >> > +}\n> >>\n> >> Did you mean !!commit_graft ?\n> >\n> > Shouldn't they produce the same results?\n> \n> Yes they should, but the use of !! seemed to imply that you wanted to\n> apply it to the pointer value. (If you indeed intended to use\n> commit_graft_nr, then 'return commit_graft_nr', without !!, would have\n> been sufficient and idiomatic C.)\n\nI just wanted to normalize the return value to a boolean 0/1. Even when\nthe input is an int, it eliminates surprises when you try to assign the\nresult to a bitfield or other smaller-width type.\n\n-Peff\n"},{"id":"236063","messageId":"CAPig+cTDbEHM0Ko-h=08T683kgsmjR07WaejZZc4Q_UfEp_UVA@mail.gmail.com","threadId":"36053","inReplyTo":"20140305010529.GA11923@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-05T01:07:56Z","receivedAt":"2014-03-05T01:07:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 4, 2014 at 8:05 PM, Jeff King wrote:\n> On Tue, Mar 04, 2014 at 08:00:44PM -0500, Eric Sunshine wrote:\n>\n>> >> > +int commit_grafts_loaded(void)\n>> >> > +{\n>> >> > + return !!commit_graft_nr;\n>> >> > +}\n>> >>\n>> >> Did you mean !!commit_graft ?\n>> >\n>> > Shouldn't they produce the same results?\n>>\n>> Yes they should, but the use of !! seemed to imply that you wanted to\n>> apply it to the pointer value. (If you indeed intended to use\n>> commit_graft_nr, then 'return commit_graft_nr', without !!, would have\n>> been sufficient and idiomatic C.)\n>\n> I just wanted to normalize the return value to a boolean 0/1. Even when\n> the input is an int, it eliminates surprises when you try to assign the\n> result to a bitfield or other smaller-width type.\n\nThanks for the explanation.\n"},{"id":"236109","messageId":"xmqqy50oh45n.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"20140305005649.GB11509@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T18:49:24Z","receivedAt":"2014-03-05T18:49:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Mar 04, 2014 at 12:52:18PM -0800, Junio C Hamano wrote:\n>\n> I do not recall any past discussion on this topic, and searching the\n> archive only shows people echoing what I said above. Is this something\n> we've promised to work in the past?\n\nThe history lesson is coming solely from my recollection of a\ndiscussion I and Linus had on the list back when we were doing the\noriginal \"graft\" and thinking about the interaction between it and\nthe object/history transfer; especially the \"only add new ones, but\nhide the ones that the user wants to be hidden\" part is something\nsuggested by Linus but we couldn't implement back then, IIRC.\n\n> Perhaps the right response is \"grafts are broken, use git-replace\n> instead\". But then should we think about deprecating grafts?\n\nI am sort of surprised to hear that question, actually ;-)\n\nI didn't say that in the message you are responding to because I\nsomehow thought that everybody has been in agreement with these two\nlines for a long while. Ever since I suggested the \"replace\" as an\nalternative \"grafts done right\" and outlined how it should work to\nChristian while sitting next to him in one of the early GitTogether,\nthe plan, at least in my mind, has always been exactly that: grafts\nwere a nice little attempt but is broken---if you really wanted to\nmuck with the history without rewriting (which is still discouraged,\nby the way), do not use \"graft\", but use \"replace\".\n"},{"id":"236110","messageId":"20140305185212.GA23907@sigill.intra.peff.net","threadId":"36053","inReplyTo":"xmqqy50oh45n.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T18:52:12Z","receivedAt":"2014-03-05T18:52:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:\n\n> > Perhaps the right response is \"grafts are broken, use git-replace\n> > instead\". But then should we think about deprecating grafts?\n> \n> I am sort of surprised to hear that question, actually ;-)\n> \n> I didn't say that in the message you are responding to because I\n> somehow thought that everybody has been in agreement with these two\n> lines for a long while. Ever since I suggested the \"replace\" as an\n> alternative \"grafts done right\" and outlined how it should work to\n> Christian while sitting next to him in one of the early GitTogether,\n> the plan, at least in my mind, has always been exactly that: grafts\n> were a nice little attempt but is broken---if you really wanted to\n> muck with the history without rewriting (which is still discouraged,\n> by the way), do not use \"graft\", but use \"replace\".\n\nI certainly had in the back of my mind that grafts were a lesser form of\n\"replace\", and that eventually we could get rid of the former. Perhaps\nmy question should have been: \"why haven't we deprecated grafts yet?\".\n\n-Peff\n"},{"id":"236117","messageId":"xmqqppm0h2ti.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"20140305185212.GA23907@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T19:18:17Z","receivedAt":"2014-03-05T19:18:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:\n>\n>> ... the plan, at least in my mind, has always been exactly that: grafts\n>> were a nice little attempt but is broken---if you really wanted to\n>> muck with the history without rewriting (which is still discouraged,\n>> by the way), do not use \"graft\", but use \"replace\".\n>\n> I certainly had in the back of my mind that grafts were a lesser form of\n> \"replace\", and that eventually we could get rid of the former. Perhaps\n> my question should have been: \"why haven't we deprecated grafts yet?\".\n\nGiven that we discourage \"grafts\" strongly and \"replace\" less so\n(but still discourage it), telling the users that biting the bullet\nand rewriting the history is _the_ permanent solution, I think it is\nunderstandable why nobody has bothered to.\n"},{"id":"236118","messageId":"20140305192837.GA11304@sigill.intra.peff.net","threadId":"36053","inReplyTo":"xmqqppm0h2ti.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-05T19:28:38Z","receivedAt":"2014-03-05T19:28:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:\n> >\n> >> ... the plan, at least in my mind, has always been exactly that: grafts\n> >> were a nice little attempt but is broken---if you really wanted to\n> >> muck with the history without rewriting (which is still discouraged,\n> >> by the way), do not use \"graft\", but use \"replace\".\n> >\n> > I certainly had in the back of my mind that grafts were a lesser form of\n> > \"replace\", and that eventually we could get rid of the former. Perhaps\n> > my question should have been: \"why haven't we deprecated grafts yet?\".\n> \n> Given that we discourage \"grafts\" strongly and \"replace\" less so\n> (but still discourage it), telling the users that biting the bullet\n> and rewriting the history is _the_ permanent solution, I think it is\n> understandable why nobody has bothered to.\n\nPerhaps the patch below would help discourage grafts more?\n\nThe notable place in the documentation where grafts are still used is\ngit-filter-branch.txt. But since the example there is about cementing\nrewritten history, it might be OK to leave.\n\nI used \"outdated\" below. We could also up the ante to \"deprecated\".\n\n-- >8 --\nSubject: [PATCH] docs: mark info/grafts as outdated\n\nWe should be encouraging people to use git-replace instead.\n\nSigned-off-by: Jeff King \n---\n Documentation/gitrepository-layout.txt | 4 ++++\n Documentation/glossary-content.txt | 4 ++++\n 2 files changed, 8 insertions(+)\n\ndiff --git a/Documentation/gitrepository-layout.txt b/Documentation/gitrepository-layout.txt\nindex aa03882..17d2ea6 100644\n--- a/Documentation/gitrepository-layout.txt\n+++ b/Documentation/gitrepository-layout.txt\n@@ -176,6 +176,10 @@ info/grafts::\n \tper line describes a commit and its fake parents by\n \tlisting their 40-byte hexadecimal object names separated\n \tby a space and terminated by a newline.\n++\n+Note that the grafts mechanism is outdated and can lead to problems\n+transferring objects between repositories; see linkgit:git-replace[1]\n+for a more flexible and robust system to do the same thing.\n \n info/exclude::\n \tThis file, by convention among Porcelains, stores the\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex 378306f..be0858c 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -176,6 +176,10 @@ current branch integrates with) obviously do not work, as there is no\n \tyou can make Git pretend the set of <> a <> has\n \tis different from what was recorded when the commit was\n \tcreated. Configured via the `.git/info/grafts` file.\n++\n+Note that the grafts mechanism is outdated and can lead to problems\n+transferring objects between repositories; see linkgit:git-replace[1]\n+for a more flexible and robust system to do the same thing.\n \n [[def_hash]]hash::\n \tIn Git's context, synonym for <>.\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236124","messageId":"xmqq4n3cgzrc.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"20140305192837.GA11304@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-05T20:24:23Z","receivedAt":"2014-03-05T20:24:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, Mar 05, 2014 at 11:18:17AM -0800, Junio C Hamano wrote:\n>\n>> Given that we discourage \"grafts\" strongly and \"replace\" less so\n>> (but still discourage it), telling the users that biting the bullet\n>> and rewriting the history is _the_ permanent solution, I think it is\n>> understandable why nobody has bothered to.\n>\n> Perhaps the patch below would help discourage grafts more?\n>\n> The notable place in the documentation where grafts are still used is\n> git-filter-branch.txt. But since the example there is about cementing\n> rewritten history, it might be OK to leave.\n\nThanks; I agree with the reasoning.\n"},{"id":"236152","messageId":"53183506.5080002@alum.mit.edu","threadId":"36053","inReplyTo":"xmqqppm0h2ti.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-06T08:42:46Z","receivedAt":"2014-03-06T08:42:46Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/05/2014 08:18 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:\n>>\n>>> ... the plan, at least in my mind, has always been exactly that: grafts\n>>> were a nice little attempt but is broken---if you really wanted to\n>>> muck with the history without rewriting (which is still discouraged,\n>>> by the way), do not use \"graft\", but use \"replace\".\n>>\n>> I certainly had in the back of my mind that grafts were a lesser form of\n>> \"replace\", and that eventually we could get rid of the former. Perhaps\n>> my question should have been: \"why haven't we deprecated grafts yet?\".\n> \n> Given that we discourage \"grafts\" strongly and \"replace\" less so\n> (but still discourage it), telling the users that biting the bullet\n> and rewriting the history is _the_ permanent solution, I think it is\n> understandable why nobody has bothered to.\n\nReplace objects are better than grafts in *almost* every dimension. The\nexception is that it is dead simple to create grafts, whereas I always\nhave to break open the man pages to remember how to create a replace\nobject that does the same thing.\n\nSo I think a helpful step towards deprecating grafts would be to offer a\ncouple of convenience features to help people kick the \"grafts\" habit:\n\n* A tool that converts grafts (i.e., the grafts read from\n$GIT_DIR/info/grafts) into the equivalent replacements.\n\n* A tool that creates a new replacement object that is the equivalent of\na graft. I.e., it should do, using replace references, the equivalent\nof the following command:\n\n echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts\n\nThese features could be added to \"git replace\" or could be built into a\nnew \"git grafts\" command.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236154","messageId":"CAP8UFD0JFWG3g2L1DNp+omE4unajjDW36qse-aE-MgbJFj3CDg@mail.gmail.com","threadId":"36053","inReplyTo":"53183506.5080002@alum.mit.edu","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-03-06T09:17:50Z","receivedAt":"2014-03-06T09:17:50Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Mar 6, 2014 at 9:42 AM, Michael Haggerty wrote:\n> On 03/05/2014 08:18 PM, Junio C Hamano wrote:\n>> Jeff King writes:\n>>\n>>> On Wed, Mar 05, 2014 at 10:49:24AM -0800, Junio C Hamano wrote:\n>>>\n>>>> ... the plan, at least in my mind, has always been exactly that: grafts\n>>>> were a nice little attempt but is broken---if you really wanted to\n>>>> muck with the history without rewriting (which is still discouraged,\n>>>> by the way), do not use \"graft\", but use \"replace\".\n>>>\n>>> I certainly had in the back of my mind that grafts were a lesser form of\n>>> \"replace\", and that eventually we could get rid of the former. Perhaps\n>>> my question should have been: \"why haven't we deprecated grafts yet?\".\n>>\n>> Given that we discourage \"grafts\" strongly and \"replace\" less so\n>> (but still discourage it), telling the users that biting the bullet\n>> and rewriting the history is _the_ permanent solution, I think it is\n>> understandable why nobody has bothered to.\n>\n> Replace objects are better than grafts in *almost* every dimension. The\n> exception is that it is dead simple to create grafts, whereas I always\n> have to break open the man pages to remember how to create a replace\n> object that does the same thing.\n>\n> So I think a helpful step towards deprecating grafts would be to offer a\n> couple of convenience features to help people kick the \"grafts\" habit:\n>\n> * A tool that converts grafts (i.e., the grafts read from\n> $GIT_DIR/info/grafts) into the equivalent replacements.\n\nYeah, I sent a kind of rough draft of a script to do that last year to\nthe mailing list, but I didn't take the time to convert it to a real\nscript or command.\n\n> * A tool that creates a new replacement object that is the equivalent of\n> a graft. I.e., it should do, using replace references, the equivalent\n> of the following command:\n>\n> echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts\n\nYeah, maybe it can be a \"git create-replace-ref command\" and it could\nhave a --convert-graft-file option to convert an existing graft file.\n\nThere have been discussions about such a command already some time ago.\n\n> These features could be added to \"git replace\" or could be built into a\n> new \"git grafts\" command.\n\nI think Junio previously said that it was better if such features were\nnot part of \"git replace\". But maybe I misunderstood his subtle\nsaying.\n\nAnd I don't think \"git grafts\" is a good name. It looks too much like\nwe are encouraging people to use grafts.\n\nThanks,\nChristian.\n"},{"id":"236167","messageId":"20140306155626.GB18519@sigill.intra.peff.net","threadId":"36053","inReplyTo":"53183506.5080002@alum.mit.edu","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T15:56:26Z","receivedAt":"2014-03-06T15:56:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2014 at 09:42:46AM +0100, Michael Haggerty wrote:\n\n> Replace objects are better than grafts in *almost* every dimension. The\n> exception is that it is dead simple to create grafts, whereas I always\n> have to break open the man pages to remember how to create a replace\n> object that does the same thing.\n> \n> So I think a helpful step towards deprecating grafts would be to offer a\n> couple of convenience features to help people kick the \"grafts\" habit:\n\nI agree that better tool support would make \"git replace\" more pleasant\nto use.\n\n> * A tool that converts grafts (i.e., the grafts read from\n> $GIT_DIR/info/grafts) into the equivalent replacements.\n\nI don't know if this is strictly necessary, if we make your command\nbelow pleasant to use. I.e., it should just be:\n\n while read sha1 parents; do\n git replace --graft $sha1 $parents\n done <.git/info/grafts\n\nWe can wrap that in \"git replace --convert-grafts\", but I do not think\ngrafts are so common that there would be a big demand for it.\n\n> * A tool that creates a new replacement object that is the equivalent of\n> a graft. I.e., it should do, using replace references, the equivalent\n> of the following command:\n> \n> echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts\n> \n> These features could be added to \"git replace\" or could be built into a\n> new \"git grafts\" command.\n\nI think it would be nice to have a set of \"mode\" options for\n\"git-replace\" to do basic editing of a sha1 and install the result\n(technically you could split the editing into a separate command, but I\ndo not see the point in editing a sha1 and then _not_ replacing it).\n\nPerhaps:\n\n # pretty-print sha1 based on type, start $EDITOR, create a\n # type-appropriate object from the result (e.g., using hash-object,\n # mktree, or mktag), and then set up the object as a replacement for\n # SHA1\n git replace --edit SHA1\n\n # ditto, but replace the $EDITOR step with the parent list\n git replace --graft SHA1 PARENT1 PARENT2\n\n # ...or remove entries from a tree\n git replace --remove-entry SHA1 foo bar\n\n-Peff\n"},{"id":"236168","messageId":"5318A537.4010400@alum.mit.edu","threadId":"36053","inReplyTo":"20140306155626.GB18519@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-06T16:41:27Z","receivedAt":"2014-03-06T16:41:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/06/2014 04:56 PM, Jeff King wrote:\n> On Thu, Mar 06, 2014 at 09:42:46AM +0100, Michael Haggerty wrote:\n> \n>> Replace objects are better than grafts in *almost* every dimension. The\n>> exception is that it is dead simple to create grafts, whereas I always\n>> have to break open the man pages to remember how to create a replace\n>> object that does the same thing.\n>>\n>> So I think a helpful step towards deprecating grafts would be to offer a\n>> couple of convenience features to help people kick the \"grafts\" habit:\n> \n> I agree that better tool support would make \"git replace\" more pleasant\n> to use.\n> \n>> * A tool that converts grafts (i.e., the grafts read from\n>> $GIT_DIR/info/grafts) into the equivalent replacements.\n> \n> I don't know if this is strictly necessary, if we make your command\n> below pleasant to use. I.e., it should just be:\n> \n> while read sha1 parents; do\n> git replace --graft $sha1 $parents\n> done <.git/info/grafts\n> \n> We can wrap that in \"git replace --convert-grafts\", but I do not think\n> grafts are so common that there would be a big demand for it.\n\nIt's probably easier to wrap it than to explain to Windows users what\nthey have to do.\n\n>> * A tool that creates a new replacement object that is the equivalent of\n>> a graft. I.e., it should do, using replace references, the equivalent\n>> of the following command:\n>>\n>> echo SHA1 [PARENT1...] >>$GIT_DIR/info/grafts\n>>\n>> These features could be added to \"git replace\" or could be built into a\n>> new \"git grafts\" command.\n> \n> I think it would be nice to have a set of \"mode\" options for\n> \"git-replace\" to do basic editing of a sha1 and install the result\n> (technically you could split the editing into a separate command, but I\n> do not see the point in editing a sha1 and then _not_ replacing it).\n\nIf modifying without replacing is needed, it would be pretty easy to add\nan option --stdout that writes the SHA1 of the modified object to stdout\ninstead of creating a replace reference. That way what you want 95% of\nthe time is the default but there is still an escape hatch.\n\n> Perhaps:\n> \n> # pretty-print sha1 based on type, start $EDITOR, create a\n> # type-appropriate object from the result (e.g., using hash-object,\n> # mktree, or mktag), and then set up the object as a replacement for\n> # SHA1\n> git replace --edit SHA1\n> \n> # ditto, but replace the $EDITOR step with the parent list\n> git replace --graft SHA1 PARENT1 PARENT2\n> \n> # ...or remove entries from a tree\n> git replace --remove-entry SHA1 foo bar\n\nI like this idea a lot, especially the pretty-printer round-tripping.\n\n\"git replace\" could support some of the options that \"git filter-branch\"\ncan take, like --env-filter, --msg-filter, etc. (at least if the target\nis a commit object).\n\nAll of this would make it possible to build up the changes that you want\nto integrate via \"filter-branch\" piecemeal instead of having to have a\nsingle monster filter-branch invocation. For example,\n\n for c in $(git rev-list --all --before=2007-01-01\n--author=root@localhost)\n do\n git replace --env-filter 'export AUTHOR_EMAIL=john@example.com' $c\n done\n # Make some more changes to other commits...\n # And when everything is done and checked:\n git filter-branch --all --tag-name-filter=cat\n\nTo me this is easier to construct than the equivalent filter-branch\ninvocation, and can be faster because its processing can be more easily\nlimited to the commits that need it. Of course to really gain speed,\nthere should be a C program that bakes in replace references by\ntraversing the object tree rather than processing each commit\nseparately, like filter-branch. I predict that this approach would have\nmost of the speed of BFG.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236171","messageId":"20140306174803.GA30486@sigill.intra.peff.net","threadId":"36053","inReplyTo":"5318A537.4010400@alum.mit.edu","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T17:48:03Z","receivedAt":"2014-03-06T17:48:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:\n\n> > We can wrap that in \"git replace --convert-grafts\", but I do not think\n> > grafts are so common that there would be a big demand for it.\n> \n> It's probably easier to wrap it than to explain to Windows users what\n> they have to do.\n\nHow would Windows users get a graft file in the first-place? There's no\nGUI for it! ;)\n\nIt should be easy to do \"--convert-grafts\", though, and I think it fits\ninto the scheme we're discussing below.\n\n> > I think it would be nice to have a set of \"mode\" options for\n> > \"git-replace\" to do basic editing of a sha1 and install the result\n> > (technically you could split the editing into a separate command, but I\n> > do not see the point in editing a sha1 and then _not_ replacing it).\n> \n> If modifying without replacing is needed, it would be pretty easy to add\n> an option --stdout that writes the SHA1 of the modified object to stdout\n> instead of creating a replace reference. That way what you want 95% of\n> the time is the default but there is still an escape hatch.\n\nAgreed. I had originally though that perhaps something like this should\nbe part of \"hash-object\", and that \"replace\" should farm out the work.\nBut thinking on it more, it doesn't really make sense as part of\n\"hash-object\".\n\n> > Perhaps:\n> > \n> > # pretty-print sha1 based on type, start $EDITOR, create a\n> > # type-appropriate object from the result (e.g., using hash-object,\n> > # mktree, or mktag), and then set up the object as a replacement for\n> > # SHA1\n> > git replace --edit SHA1\n\nHere's a rough series that gets us this far:\n\n [1/4]: replace: refactor command-mode determination\n [2/4]: replace: use OPT_CMDMODE to handle modes\n [3/4]: replace: factor object resolution out of replace_object\n [4/4]: replace: add --edit option\n\nIt shouldn't be too hard to do \"--graft\" or \"--convert-grafts\" on top.\n\nI also noticed that doing:\n\n git replace foo foo\n\nis less than friendly (we notice the cycle, but just barf). It's\nespecially easy to do with \"git replace --edit\", if you just exit the\neditor without making changes. Or if you make changes to an\nalready-replaced object to revert it back, in which case we would want\nto notice and delete the replacement.\n\nSo I think we want to have \"git replace foo foo\" silently converted into\n\"git replace -d foo\" (but without an error if there is no existing\nreplacement), and then \"--edit\" will just do the right thing, as it's\nbuilt on top.\n\nI also noticed that the diff engine does not play well with replacements\nof blobs. When we are diffing the trees, we see that the sha1 for path\n\"foo\" is the same on either side, and do not look further, even though\nfeeding those sha1s to builtin_diff would fetch the replacements. I\nthink compare_tree_entry would have to learn lookup_replace_object (and\nI suspect it would make tree diffs noticeably slower when you have even\none replace ref).\n\n> \"git replace\" could support some of the options that \"git filter-branch\"\n> can take, like --env-filter, --msg-filter, etc. (at least if the target\n> is a commit object).\n> \n> All of this would make it possible to build up the changes that you want\n> to integrate via \"filter-branch\" piecemeal instead of having to have a\n> single monster filter-branch invocation. For example,\n\nRight. I was tempted to suggest that, too, but I think it can get rather\ntricky, as you need to replace in a loop, and sometimes the exact\nobjects you need aren't obvious. For example, a common use of\n\"--index-filter\" is to remove a single file. But to remove\n\"foo/bar/baz\", you would need to loop over each commit, find the tree\nfor \"foo/bar\", and then remove the \"baz\" entry in \n\nStill, I really like the workflow of having decent \"replace\" tools,\nfollowed by \"cementing\" the changes into place with a \"filter-branch\"\nrun (which, btw, does not yet know how to cement trees and blobs into\nplace). It lets you work on the filtering incrementally, and even share\nor work collaboratively on it by pushing refs/replace).\n\nAnd as you mention, it could be a heck of a lot faster than what we have\nnow.\n\n-Peff\n"},{"id":"236172","messageId":"20140306174904.GA30691@sigill.intra.peff.net","threadId":"36053","inReplyTo":"20140306174803.GA30486@sigill.intra.peff.net","subject":"[RFC/PATCH 1/4] replace: refactor command-mode determination","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T17:49:05Z","receivedAt":"2014-03-06T17:49:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The git-replace command has three modes: listing, deleting,\nand replacing. The first two are selected explicitly. If\nnone is selected, we fallback to listing when there are no\narguments, and replacing otherwise.\n\nLet's figure out up front which operation we are going to\ndo, before getting into the application logic. That lets us\nsimplify our option checks (e.g., we currently have to check\nwhether a useless \"--force\" is given both along with an\nexplicit list, as well as with an implicit one).\n\nThis saves some lines, makes the logic easier to follow, and\nwill facilitate further cleanups.\n\nSigned-off-by: Jeff King \n---\n builtin/replace.c | 14 ++++++--------\n 1 file changed, 6 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 2336325..6a0e8bd 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -182,12 +182,16 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);\n \n+\tif (!list && !delete)\n+\t\tif (!argc)\n+\t\t\tlist = 1;\n+\n \tif (list && delete)\n \t\tusage_msg_opt(\"-l and -d cannot be used together\",\n \t\t\t git_replace_usage, options);\n \n-\tif (format && delete)\n-\t\tusage_msg_opt(\"--format and -d cannot be used together\",\n+\tif (format && !list)\n+\t\tusage_msg_opt(\"--format cannot be used when not listing\",\n \t\t\t git_replace_usage, options);\n \n \tif (force && (list || delete))\n@@ -207,9 +211,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tif (argc != 2)\n \t\t\tusage_msg_opt(\"bad number of arguments\",\n \t\t\t\t git_replace_usage, options);\n-\t\tif (format)\n-\t\t\tusage_msg_opt(\"--format cannot be used when not listing\",\n-\t\t\t\t git_replace_usage, options);\n \t\treturn replace_object(argv[0], argv[1], force);\n \t}\n \n@@ -217,9 +218,6 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \tif (argc > 1)\n \t\tusage_msg_opt(\"only one pattern can be given with -l\",\n \t\t\t git_replace_usage, options);\n-\tif (force)\n-\t\tusage_msg_opt(\"-f needs some arguments\",\n-\t\t\t git_replace_usage, options);\n \n \treturn list_replace_refs(argv[0], format);\n }\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236173","messageId":"20140306174921.GB30691@sigill.intra.peff.net","threadId":"36053","inReplyTo":"20140306174803.GA30486@sigill.intra.peff.net","subject":"[RFC/PATCH 2/4] replace: use OPT_CMDMODE to handle modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T17:49:22Z","receivedAt":"2014-03-06T17:49:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"By using OPT_CMDMODE, the mutual exclusion between modes is\ntaken care of for us. It also makes it easy for us to\nmaintain a single variable with the mode, which makes its\nintent more clear. We can use a single switch() to make sure\nwe have covered all of the modes.\n\nThis ends up breaking even in code size, but the win will be\nmuch bigger when we start adding more modes.\n\nSigned-off-by: Jeff King \n---\n builtin/replace.c | 49 +++++++++++++++++++++++++------------------------\n 1 file changed, 25 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 6a0e8bd..0b5cb17 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -168,11 +168,17 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \n int cmd_replace(int argc, const char **argv, const char *prefix)\n {\n-\tint list = 0, delete = 0, force = 0;\n+\tint force = 0;\n \tconst char *format = NULL;\n+\tenum {\n+\t\tMODE_UNSPECIFIED = 0,\n+\t\tMODE_LIST,\n+\t\tMODE_DELETE,\n+\t\tMODE_REPLACE\n+\t} cmdmode = MODE_UNSPECIFIED;\n \tstruct option options[] = {\n-\t\tOPT_BOOL('l', \"list\", &list, N_(\"list replace refs\")),\n-\t\tOPT_BOOL('d', \"delete\", &delete, N_(\"delete replace refs\")),\n+\t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n+\t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n \t\tOPT_BOOL('f', \"force\", &force, N_(\"replace the ref if it exists\")),\n \t\tOPT_STRING(0, \"format\", &format, N_(\"format\"), N_(\"use this format\")),\n \t\tOPT_END()\n@@ -182,42 +188,37 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, options, git_replace_usage, 0);\n \n-\tif (!list && !delete)\n-\t\tif (!argc)\n-\t\t\tlist = 1;\n+\tif (!cmdmode)\n+\t\tcmdmode = argc ? MODE_REPLACE : MODE_DELETE;\n \n-\tif (list && delete)\n-\t\tusage_msg_opt(\"-l and -d cannot be used together\",\n-\t\t\t git_replace_usage, options);\n-\n-\tif (format && !list)\n+\tif (format && cmdmode != MODE_LIST)\n \t\tusage_msg_opt(\"--format cannot be used when not listing\",\n \t\t\t git_replace_usage, options);\n \n-\tif (force && (list || delete))\n-\t\tusage_msg_opt(\"-f cannot be used with -d or -l\",\n+\tif (force && cmdmode != MODE_REPLACE)\n+\t\tusage_msg_opt(\"-f only makes sense when writing a replacement\",\n \t\t\t git_replace_usage, options);\n \n-\t/* Delete refs */\n-\tif (delete) {\n+\tswitch (cmdmode) {\n+\tcase MODE_DELETE:\n \t\tif (argc < 1)\n \t\t\tusage_msg_opt(\"-d needs at least one argument\",\n \t\t\t\t git_replace_usage, options);\n \t\treturn for_each_replace_name(argv, delete_replace_ref);\n-\t}\n \n-\t/* Replace object */\n-\tif (!list && argc) {\n+\tcase MODE_REPLACE:\n \t\tif (argc != 2)\n \t\t\tusage_msg_opt(\"bad number of arguments\",\n \t\t\t\t git_replace_usage, options);\n \t\treturn replace_object(argv[0], argv[1], force);\n-\t}\n \n-\t/* List refs, even if \"list\" is not set */\n-\tif (argc > 1)\n-\t\tusage_msg_opt(\"only one pattern can be given with -l\",\n-\t\t\t git_replace_usage, options);\n+\tcase MODE_LIST:\n+\t\tif (argc > 1)\n+\t\t\tusage_msg_opt(\"only one pattern can be given with -l\",\n+\t\t\t\t git_replace_usage, options);\n+\t\treturn list_replace_refs(argv[0], format);\n \n-\treturn list_replace_refs(argv[0], format);\n+\tdefault:\n+\t\tdie(\"BUG: invalid cmdmode %d\", (int)cmdmode);\n+\t}\n }\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236174","messageId":"20140306174929.GC30691@sigill.intra.peff.net","threadId":"36053","inReplyTo":"20140306174803.GA30486@sigill.intra.peff.net","subject":"[RFC/PATCH 3/4] replace: factor object resolution out of replace_object","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T17:49:29Z","receivedAt":"2014-03-06T17:49:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As we add new options that operate on objects before\nreplacing them, we'll want to be able to feed raw sha1s\nstraight into replace_object. Split replace_object into the\nobject-resolution part and the actual replacement.\n\nSigned-off-by: Jeff King \n---\n builtin/replace.c | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 0b5cb17..a090302 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -123,19 +123,17 @@ static int delete_replace_ref(const char *name, const char *ref,\n \treturn 0;\n }\n \n-static int replace_object(const char *object_ref, const char *replace_ref,\n-\t\t\t int force)\n+static int replace_object_sha1(const char *object_ref,\n+\t\t\t unsigned char object[20],\n+\t\t\t const char *replace_ref,\n+\t\t\t unsigned char repl[20],\n+\t\t\t int force)\n {\n-\tunsigned char object[20], prev[20], repl[20];\n+\tunsigned char prev[20];\n \tenum object_type obj_type, repl_type;\n \tchar ref[PATH_MAX];\n \tstruct ref_lock *lock;\n \n-\tif (get_sha1(object_ref, object))\n-\t\tdie(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n-\tif (get_sha1(replace_ref, repl))\n-\t\tdie(\"Failed to resolve '%s' as a valid ref.\", replace_ref);\n-\n \tif (snprintf(ref, sizeof(ref),\n \t\t \"refs/replace/%s\",\n \t\t sha1_to_hex(object)) > sizeof(ref) - 1)\n@@ -166,6 +164,18 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \treturn 0;\n }\n \n+static int replace_object(const char *object_ref, const char *replace_ref, int force)\n+{\n+\tunsigned char object[20], repl[20];\n+\n+\tif (get_sha1(object_ref, object))\n+\t\tdie(\"Failed to resolve '%s' as a valid ref.\", object_ref);\n+\tif (get_sha1(replace_ref, repl))\n+\t\tdie(\"Failed to resolve '%s' as a valid ref.\", replace_ref);\n+\n+\treturn replace_object_sha1(object_ref, object, replace_ref, repl, force);\n+}\n+\n int cmd_replace(int argc, const char **argv, const char *prefix)\n {\n \tint force = 0;\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236175","messageId":"20140306175117.GD30691@sigill.intra.peff.net","threadId":"36053","inReplyTo":"20140306174803.GA30486@sigill.intra.peff.net","subject":"[RFC/PATCH 4/4] replace: add --edit option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T17:51:17Z","receivedAt":"2014-03-06T17:51:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This allows you to run:\n\n git replace --edit SHA1\n\nto get dumped in an editor with the contents of the object\nfor SHA1. The result is then read back in and used as a\n\"replace\" object for SHA1. The writing/reading is\ntype-aware, so you get to edit \"ls-tree\" output rather than\nthe binary tree format.\n\nMissing documentation and tests.\n\nSigned-off-by: Jeff King \n---\nBesides missing docs and tests, we might find that we want to factor the\ncode a little differently when we start adding other helpers (like\n\"--graft\"). I will probably push this forward at some point, but I'm not\nplanning on working on it for the rest of the day, so if you want to\npick it up as a base in the meantime and try \"--graft\", \"--env-filter\",\nor anything else clever on top, please go ahead.\n\n builtin/replace.c | 110 +++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 109 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex a090302..3ed5f75 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -12,6 +12,7 @@\n #include \"builtin.h\"\n #include \"refs.h\"\n #include \"parse-options.h\"\n+#include \"run-command.h\"\n \n static const char * const git_replace_usage[] = {\n \tN_(\"git replace [-f] \"),\n@@ -176,6 +177,105 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f\n \treturn replace_object_sha1(object_ref, object, replace_ref, repl, force);\n }\n \n+/*\n+ * Write the contents of the object named by \"sha1\" to the file \"filename\",\n+ * pretty-printed for human editing based on its type.\n+ */\n+static void export_object(const unsigned char *sha1, const char *filename)\n+{\n+\tconst char *argv[] = { \"cat-file\", \"-p\", NULL, NULL };\n+\tstruct child_process cmd = { argv };\n+\tint fd;\n+\n+\tfd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\tif (fd < 0)\n+\t\tdie_errno(\"unable to open %s for writing\", filename);\n+\n+\targv[2] = sha1_to_hex(sha1);\n+\tcmd.git_cmd = 1;\n+\tcmd.out = fd;\n+\n+\tif (run_command(&cmd))\n+\t\tdie(\"cat-file reported failure\");\n+\n+\tclose(fd);\n+}\n+\n+/*\n+ * Read a previously-exported (and possibly edited) object back from \"filename\",\n+ * interpreting it as \"type\", and writing the result to the object database.\n+ * The sha1 of the written object is returned via sha1.\n+ */\n+static void import_object(unsigned char *sha1, enum object_type type,\n+\t\t\t const char *filename)\n+{\n+\tint fd;\n+\n+\tfd = open(filename, O_RDONLY);\n+\tif (fd < 0)\n+\t\tdie_errno(\"unable to open %s for reading\", filename);\n+\n+\tif (type == OBJ_TREE) {\n+\t\tconst char *argv[] = { \"mktree\", NULL };\n+\t\tstruct child_process cmd = { argv };\n+\t\tstruct strbuf result = STRBUF_INIT;\n+\n+\t\tcmd.argv = argv;\n+\t\tcmd.git_cmd = 1;\n+\t\tcmd.in = fd;\n+\t\tcmd.out = -1;\n+\n+\t\tif (start_command(&cmd))\n+\t\t\tdie(\"unable to spawn mktree\");\n+\n+\t\tif (strbuf_read(&result, cmd.out, 41) < 0)\n+\t\t\tdie_errno(\"unable to read from mktree\");\n+\t\tclose(cmd.out);\n+\n+\t\tif (finish_command(&cmd))\n+\t\t\tdie(\"mktree reported failure\");\n+\t\tif (get_sha1_hex(result.buf, sha1) < 0)\n+\t\t\tdie(\"mktree did not return an object name\");\n+\t} else {\n+\t\tstruct stat st;\n+\t\tint flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT;\n+\n+\t\tif (fstat(fd, &st) < 0)\n+\t\t\tdie_errno(\"unable to fstat %s\", filename);\n+\t\tif (index_fd(sha1, fd, &st, type, NULL, flags) < 0)\n+\t\t\tdie(\"unable to write object to database\");\n+\t\t/* index_fd close()s fd for us */\n+\t}\n+\n+\t/*\n+\t * No need to close(fd) here; both run-command and index-fd\n+\t * will have done it for us.\n+\t */\n+}\n+\n+static int edit_and_replace(const char *object_ref, int force)\n+{\n+\tchar *tmpfile = git_pathdup(\"REPLACE_EDITOBJ\");\n+\tenum object_type type;\n+\tunsigned char old[20], new[20];\n+\n+\tif (get_sha1(object_ref, old) < 0)\n+\t\tdie(\"Not a valid object name: '%s'\", object_ref);\n+\n+\ttype = sha1_object_info(old, NULL);\n+\tif (type < 0)\n+\t\tdie(\"unable to get object type for %s\", sha1_to_hex(old));\n+\n+\texport_object(old, tmpfile);\n+\tif (launch_editor(tmpfile, NULL, NULL) < 0)\n+\t\tdie(\"editing object file failed\");\n+\timport_object(new, type, tmpfile);\n+\n+\tfree(tmpfile);\n+\n+\treturn replace_object_sha1(object_ref, old, \"replacement\", new, force);\n+}\n+\n int cmd_replace(int argc, const char **argv, const char *prefix)\n {\n \tint force = 0;\n@@ -184,11 +284,13 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tMODE_UNSPECIFIED = 0,\n \t\tMODE_LIST,\n \t\tMODE_DELETE,\n+\t\tMODE_EDIT,\n \t\tMODE_REPLACE\n \t} cmdmode = MODE_UNSPECIFIED;\n \tstruct option options[] = {\n \t\tOPT_CMDMODE('l', \"list\", &cmdmode, N_(\"list replace refs\"), MODE_LIST),\n \t\tOPT_CMDMODE('d', \"delete\", &cmdmode, N_(\"delete replace refs\"), MODE_DELETE),\n+\t\tOPT_CMDMODE('e', \"edit\", &cmdmode, N_(\"edit existing object\"), MODE_EDIT),\n \t\tOPT_BOOL('f', \"force\", &force, N_(\"replace the ref if it exists\")),\n \t\tOPT_STRING(0, \"format\", &format, N_(\"format\"), N_(\"use this format\")),\n \t\tOPT_END()\n@@ -205,7 +307,7 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\tusage_msg_opt(\"--format cannot be used when not listing\",\n \t\t\t git_replace_usage, options);\n \n-\tif (force && cmdmode != MODE_REPLACE)\n+\tif (force && cmdmode != MODE_REPLACE && cmdmode != MODE_EDIT)\n \t\tusage_msg_opt(\"-f only makes sense when writing a replacement\",\n \t\t\t git_replace_usage, options);\n \n@@ -222,6 +324,12 @@ int cmd_replace(int argc, const char **argv, const char *prefix)\n \t\t\t\t git_replace_usage, options);\n \t\treturn replace_object(argv[0], argv[1], force);\n \n+\tcase MODE_EDIT:\n+\t\tif (argc != 1)\n+\t\t\tusage_msg_opt(\"-e needs exactly one argument\",\n+\t\t\t\t git_replace_usage, options);\n+\t\treturn edit_and_replace(argv[0], force);\n+\n \tcase MODE_LIST:\n \t\tif (argc > 1)\n \t\t\tusage_msg_opt(\"only one pattern can be given with -l\",\n-- \n1.8.5.2.500.g8060133\n"},{"id":"236178","messageId":"20140306184801.GA30890@sigill.intra.peff.net","threadId":"36053","inReplyTo":"CAP8UFD2c0UKT8Uyw4j9SzKGx2oLn=o7N-dtvQHPaaBtLT6ggcw@mail.gmail.com","subject":"Re: [RFC/PATCH 2/4] replace: use OPT_CMDMODE to handle modes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T18:48:01Z","receivedAt":"2014-03-06T18:48:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2014 at 07:35:19PM +0100, Christian Couder wrote:\n\n> > + if (!cmdmode)\n> > + cmdmode = argc ? MODE_REPLACE : MODE_DELETE;\n> \n> Shouldn't it be MODE_LIST instead of MODE_DELETE?\n\nArgh, yes, thank you for catching. My original iteration used chars like\n'd' and 'l' (similar to other uses of OPT_CMDMODE). I switched it at the\nlast minute to an enum, and somehow thinko'd that completely (and\nobviously did not run the tests again afterwards).\n\n-Peff\n"},{"id":"236179","messageId":"xmqqbnxjywxz.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"20140306174803.GA30486@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T19:00:08Z","receivedAt":"2014-03-06T19:00:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I also noticed that the diff engine does not play well with replacements\n> of blobs. When we are diffing the trees, we see that the sha1 for path\n> \"foo\" is the same on either side, and do not look further, even though\n> feeding those sha1s to builtin_diff would fetch the replacements.\n\nSorry, I do not quite understand.\n\nIn \"git diff A B -- path\", if the object name recorded for A:path\nand B:path are the same, but the replacement mechanism maps the\nobject name for that blob object to some other blob object, wouldn't\nthe result be empty because both sides replace to the same thing\nanyway?\n"},{"id":"236180","messageId":"20140306190757.GA27583@sigill.intra.peff.net","threadId":"36053","inReplyTo":"xmqqbnxjywxz.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-06T19:07:57Z","receivedAt":"2014-03-06T19:07:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2014 at 11:00:08AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I also noticed that the diff engine does not play well with replacements\n> > of blobs. When we are diffing the trees, we see that the sha1 for path\n> > \"foo\" is the same on either side, and do not look further, even though\n> > feeding those sha1s to builtin_diff would fetch the replacements.\n> \n> Sorry, I do not quite understand.\n> \n> In \"git diff A B -- path\", if the object name recorded for A:path\n> and B:path are the same, but the replacement mechanism maps the\n> object name for that blob object to some other blob object, wouldn't\n> the result be empty because both sides replace to the same thing\n> anyway?\n\nOh, right, I was being stupid. I did:\n\n git replace --edit HEAD:some-file\n\nand expected \"git show\" to find the diff. But that doesn't make sense.\nOn top of that, I need to do:\n\n git replace --edit HEAD^{tree}\n\nto replace the sha1 of the entry in the tree. In which case diff would\nfind it just fine.\n\n-Peff\n"},{"id":"236214","messageId":"08A515BA063C44E5A9EFC754793B2AD8@PhilipOakley","threadId":"36053","inReplyTo":"20140306174803.GA30486@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-03-06T22:20:28Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Jeff King\" \n> On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:\n>\n>> > We can wrap that in \"git replace --convert-grafts\", but I do not \n>> > think\n>> > grafts are so common that there would be a big demand for it.\n>>\n>> It's probably easier to wrap it than to explain to Windows users what\n>> they have to do.\n>\n> How would Windows users get a graft file in the first-place? There's \n> no\n> GUI for it! ;)\n\nNow, now... It's dead easy using the git-gui and Notepad++, you can see \nand confirm the sha1's, copy and paste, and the graft file is a very \neasy format, so even wimps (windows, icons, menus, pointers aka mouse) \nfolks can do it. (It worked for me when I needed it ;-)\n\nThe main point is that grafts are very easy to create [1], in that there \nis no object manipulation, while the replace mechanism does need a fresh \nobject to be created that will 'replace' the old object. This \nmanipulation can be perceived at least an awkward step. The replace \nmechanism needs to be at least as easy as the graft.\n\nSomething as simple as the 'git replace --graft $sha1 $parents' idea \nwould make it very easy to deprecate the older graft process with this \nconceptually almost identical syntax. There are a few other \ndocumentation places that should also be updated when its sorted [2].\n\n>\n> It should be easy to do \"--convert-grafts\", though, and I think it \n> fits\n> into the scheme we're discussing below.\n>\n>> > I think it would be nice to have a set of \"mode\" options for\n>> > \"git-replace\" to do basic editing of a sha1 and install the result\n>> > (technically you could split the editing into a separate command, \n>> > but I\n>> > do not see the point in editing a sha1 and then _not_ replacing \n>> > it).\n>>\n>> If modifying without replacing is needed, it would be pretty easy to \n>> add\n>> an option --stdout that writes the SHA1 of the modified object to \n>> stdout\n>> instead of creating a replace reference. That way what you want 95% \n>> of\n>> the time is the default but there is still an escape hatch.\n>\n> Agreed. I had originally though that perhaps something like this \n> should\n> be part of \"hash-object\", and that \"replace\" should farm out the work.\n> But thinking on it more, it doesn't really make sense as part of\n> \"hash-object\".\n>\n>> > Perhaps:\n>> >\n>> > # pretty-print sha1 based on type, start $EDITOR, create a\n>> > # type-appropriate object from the result (e.g., using \n>> > hash-object,\n>> > # mktree, or mktag), and then set up the object as a replacement \n>> > for\n>> > # SHA1\n>> > git replace --edit SHA1\n>\n> Here's a rough series that gets us this far:\n>\n> [1/4]: replace: refactor command-mode determination\n> [2/4]: replace: use OPT_CMDMODE to handle modes\n> [3/4]: replace: factor object resolution out of replace_object\n> [4/4]: replace: add --edit option\n>\n> It shouldn't be too hard to do \"--graft\" or \"--convert-grafts\" on top.\n>\n> I also noticed that doing:\n>\n> git replace foo foo\n>\n> is less than friendly (we notice the cycle, but just barf). It's\n> especially easy to do with \"git replace --edit\", if you just exit the\n> editor without making changes. Or if you make changes to an\n> already-replaced object to revert it back, in which case we would want\n> to notice and delete the replacement.\n>\n> So I think we want to have \"git replace foo foo\" silently converted \n> into\n> \"git replace -d foo\" (but without an error if there is no existing\n> replacement), and then \"--edit\" will just do the right thing, as it's\n> built on top.\n>\n> I also noticed that the diff engine does not play well with \n> replacements\n> of blobs. When we are diffing the trees, we see that the sha1 for path\n> \"foo\" is the same on either side, and do not look further, even though\n> feeding those sha1s to builtin_diff would fetch the replacements. I\n> think compare_tree_entry would have to learn lookup_replace_object \n> (and\n> I suspect it would make tree diffs noticeably slower when you have \n> even\n> one replace ref).\n>\n>> \"git replace\" could support some of the options that \"git \n>> filter-branch\"\n>> can take, like --env-filter, --msg-filter, etc. (at least if the \n>> target\n>> is a commit object).\n>>\n>> All of this would make it possible to build up the changes that you \n>> want\n>> to integrate via \"filter-branch\" piecemeal instead of having to have \n>> a\n>> single monster filter-branch invocation. For example,\n>\n> Right. I was tempted to suggest that, too, but I think it can get \n> rather\n> tricky, as you need to replace in a loop, and sometimes the exact\n> objects you need aren't obvious. For example, a common use of\n> \"--index-filter\" is to remove a single file. But to remove\n> \"foo/bar/baz\", you would need to loop over each commit, find the tree\n> for \"foo/bar\", and then remove the \"baz\" entry in\n>\n> Still, I really like the workflow of having decent \"replace\" tools,\n> followed by \"cementing\" the changes into place with a \"filter-branch\"\n> run (which, btw, does not yet know how to cement trees and blobs into\n> place). It lets you work on the filtering incrementally, and even \n> share\n> or work collaboratively on it by pushing refs/replace).\n>\n> And as you mention, it could be a heck of a lot faster than what we \n> have\n> now.\n>\n> -Peff\n> --\n\nPhilip\n[1] https://git.wiki.kernel.org/index.php/GraftPoint\n[2] http://stackoverflow.com/q/6800692/717355 \n"},{"id":"236221","messageId":"531904E1.6010606@alum.mit.edu","threadId":"36053","inReplyTo":"08A515BA063C44E5A9EFC754793B2AD8@PhilipOakley","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-06T23:29:37Z","receivedAt":"2014-03-06T23:29:37Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/07/2014 12:01 AM, Philip Oakley wrote:\n> From: \"Jeff King\" \n>> On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:\n>>\n>>> > We can wrap that in \"git replace --convert-grafts\", but I do not >\n>>> think\n>>> > grafts are so common that there would be a big demand for it.\n>>>\n>>> It's probably easier to wrap it than to explain to Windows users what\n>>> they have to do.\n>>\n>> How would Windows users get a graft file in the first-place? There's no\n>> GUI for it! ;)\n> \n> Now, now... It's dead easy using the git-gui and Notepad++, you can see\n> and confirm the sha1's, copy and paste, and the graft file is a very\n> easy format, so even wimps (windows, icons, menus, pointers aka mouse)\n> folks can do it. (It worked for me when I needed it ;-)\n\nI didn't mean to insult all Windows users in general. I was only\nreferring to the fact that since the default Windows command line is not\na POSIX shell, even an experienced Windows user might have trouble\nfiguring out how to execute a shell loop. Putting this functionality in\na git command or script, by contrast, would make it work universally, no\nfuss, no muss.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236223","messageId":"xmqqob1ivqv4.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"531904E1.6010606@alum.mit.edu","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-06T23:39:43Z","receivedAt":"2014-03-06T23:39:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> I didn't mean to insult all Windows users in general. I was only\n> referring to the fact that since the default Windows command line is not\n> a POSIX shell, even an experienced Windows user might have trouble\n> figuring out how to execute a shell loop. Putting this functionality in\n> a git command or script, by contrast, would make it work universally, no\n> fuss, no muss.\n\n;-)\n\nBe it graft or replace, I do not think we want to invite people to\nuse these mechansims too lightly to locally rewrite their history\nwilly-nilly without fixing their mistakes at the object layer with\n\"commit --amend\", \"rebase\", \"bfg\", etc. in the longer term. So in\nthat sense, adding a command to make it easy is not something I am\nenthusiastic about.\n\nOn the other hand, if the user does need to use graft or replace\n(perhaps to prepare for casting the fixed history in stone with\nfilter-branch), it would be good to help them avoid making mistakes\nwhile doing so and tool support may be a way to do so.\n\nSo, ... I am of two minds.\n"},{"id":"236224","messageId":"06EAF31C54FB49E5951781E637A79F04@PhilipOakley","threadId":"36053","inReplyTo":"531904E1.6010606@alum.mit.edu","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":null,"receivedAt":"2014-03-06T23:39:43Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Michael Haggerty\" \n> On 03/07/2014 12:01 AM, Philip Oakley wrote:\n>> From: \"Jeff King\" \n>>> On Thu, Mar 06, 2014 at 05:41:27PM +0100, Michael Haggerty wrote:\n>>>\n>>>> > We can wrap that in \"git replace --convert-grafts\", but I do not \n>>>> > >\n>>>> think\n>>>> > grafts are so common that there would be a big demand for it.\n>>>>\n>>>> It's probably easier to wrap it than to explain to Windows users \n>>>> what\n>>>> they have to do.\n>>>\n>>> How would Windows users get a graft file in the first-place? There's \n>>> no\n>>> GUI for it! ;)\n>>\n>> Now, now... It's dead easy using the git-gui and Notepad++, you can \n>> see\n>> and confirm the sha1's, copy and paste, and the graft file is a very\n>> easy format, so even wimps (windows, icons, menus, pointers aka \n>> mouse)\n>> folks can do it. (It worked for me when I needed it ;-)\n>\n> I didn't mean to insult all Windows users in general. I was only\n> referring to the fact that since the default Windows command line is \n> not\n> a POSIX shell, even an experienced Windows user might have trouble\n> figuring out how to execute a shell loop.\n\nI'd missed that aspect about the shell loop. I was mainly pointing out \ncurrent awkwardness of creating the replace object, relative to grafts - \nThere was an initial attempt by Christian, but it became quite hard to \nmake it robust to sha1's embedded in commit messages.\n\n> Putting this functionality in\n> a git command or script, by contrast, would make it work universally, \n> no\n> fuss, no muss.\n>\n> Michael\n>\n> -- \n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n> \n"},{"id":"236231","messageId":"CAPig+cQi+7md4P2OXJDDcs7VMkMEsUHaEz5wsaDOyu4ppk2uAg@mail.gmail.com","threadId":"36053","inReplyTo":"20140306175117.GD30691@sigill.intra.peff.net","subject":"Re: [RFC/PATCH 4/4] replace: add --edit option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-07T01:57:58Z","receivedAt":"2014-03-07T01:57:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 6, 2014 at 12:51 PM, Jeff King wrote:\n> This allows you to run:\n>\n> git replace --edit SHA1\n>\n> to get dumped in an editor with the contents of the object\n> for SHA1. The result is then read back in and used as a\n> \"replace\" object for SHA1. The writing/reading is\n> type-aware, so you get to edit \"ls-tree\" output rather than\n> the binary tree format.\n>\n> Signed-off-by: Jeff King \n> ---\n> diff --git a/builtin/replace.c b/builtin/replace.c\n> index a090302..3ed5f75 100644\n> --- a/builtin/replace.c\n> +++ b/builtin/replace.c\n> @@ -176,6 +177,105 @@ static int replace_object(const char *object_ref, const char *replace_ref, int f\n> return replace_object_sha1(object_ref, object, replace_ref, repl, force);\n> }\n>\n> +/*\n> + * Read a previously-exported (and possibly edited) object back from \"filename\",\n> + * interpreting it as \"type\", and writing the result to the object database.\n> + * The sha1 of the written object is returned via sha1.\n> + */\n> +static void import_object(unsigned char *sha1, enum object_type type,\n> + const char *filename)\n> +{\n> + int fd;\n> +\n> + fd = open(filename, O_RDONLY);\n> + if (fd < 0)\n> + die_errno(\"unable to open %s for reading\", filename);\n> +\n> + if (type == OBJ_TREE) {\n> + const char *argv[] = { \"mktree\", NULL };\n> + struct child_process cmd = { argv };\n> + struct strbuf result = STRBUF_INIT;\n> +\n> + cmd.argv = argv;\n> + cmd.git_cmd = 1;\n> + cmd.in = fd;\n> + cmd.out = -1;\n> +\n> + if (start_command(&cmd))\n> + die(\"unable to spawn mktree\");\n> +\n> + if (strbuf_read(&result, cmd.out, 41) < 0)\n> + die_errno(\"unable to read from mktree\");\n> + close(cmd.out);\n> +\n> + if (finish_command(&cmd))\n> + die(\"mktree reported failure\");\n> + if (get_sha1_hex(result.buf, sha1) < 0)\n> + die(\"mktree did not return an object name\");\n\nstrbuf_release(&result);\n\n> + } else {\n> + struct stat st;\n> + int flags = HASH_FORMAT_CHECK | HASH_WRITE_OBJECT;\n> +\n> + if (fstat(fd, &st) < 0)\n> + die_errno(\"unable to fstat %s\", filename);\n> + if (index_fd(sha1, fd, &st, type, NULL, flags) < 0)\n> + die(\"unable to write object to database\");\n> + /* index_fd close()s fd for us */\n> + }\n> +\n> + /*\n> + * No need to close(fd) here; both run-command and index-fd\n> + * will have done it for us.\n> + */\n> +}\n> --\n> 1.8.5.2.500.g8060133\n"},{"id":"236249","messageId":"CAP8UFD0UnUGZb9hWyLS1vPJ6fh3QR-g_p5HNQk79Gqhs9YWi0A@mail.gmail.com","threadId":"36053","inReplyTo":"xmqqob1ivqv4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2014-03-07T07:08:37Z","receivedAt":"2014-03-07T07:08:37Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Mar 7, 2014 at 12:39 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>\n>> I didn't mean to insult all Windows users in general. I was only\n>> referring to the fact that since the default Windows command line is not\n>> a POSIX shell, even an experienced Windows user might have trouble\n>> figuring out how to execute a shell loop. Putting this functionality in\n>> a git command or script, by contrast, would make it work universally, no\n>> fuss, no muss.\n>\n> ;-)\n>\n> Be it graft or replace, I do not think we want to invite people to\n> use these mechansims too lightly to locally rewrite their history\n> willy-nilly without fixing their mistakes at the object layer with\n> \"commit --amend\", \"rebase\", \"bfg\", etc. in the longer term. So in\n> that sense, adding a command to make it easy is not something I am\n> enthusiastic about.\n>\n> On the other hand, if the user does need to use graft or replace\n> (perhaps to prepare for casting the fixed history in stone with\n> filter-branch), it would be good to help them avoid making mistakes\n> while doing so and tool support may be a way to do so.\n>\n> So, ... I am of two minds.\n>\n\nMaybe if we add a new command (or maybe a script) with a name long and\ncryptic-looking enough like \"git create-replacement-object\" it will\nscare casual users from touching it, while power users will be happy\nto benefit from it.\n"},{"id":"236264","messageId":"20140307171714.GB23587@sigill.intra.peff.net","threadId":"36053","inReplyTo":"CAPig+cQi+7md4P2OXJDDcs7VMkMEsUHaEz5wsaDOyu4ppk2uAg@mail.gmail.com","subject":"Re: [RFC/PATCH 4/4] replace: add --edit option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-07T17:17:14Z","receivedAt":"2014-03-07T17:17:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 06, 2014 at 08:57:58PM -0500, Eric Sunshine wrote:\n\n> > + if (strbuf_read(&result, cmd.out, 41) < 0)\n> > + die_errno(\"unable to read from mktree\");\n> > + close(cmd.out);\n> > +\n> > + if (finish_command(&cmd))\n> > + die(\"mktree reported failure\");\n> > + if (get_sha1_hex(result.buf, sha1) < 0)\n> > + die(\"mktree did not return an object name\");\n> \n> strbuf_release(&result);\n\nThanks for catching. I'll include it in any re-roll.\n\n-Peff\n"},{"id":"236265","messageId":"20140307171936.GC23587@sigill.intra.peff.net","threadId":"36053","inReplyTo":"CAP8UFD0UnUGZb9hWyLS1vPJ6fh3QR-g_p5HNQk79Gqhs9YWi0A@mail.gmail.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-07T17:19:36Z","receivedAt":"2014-03-07T17:19:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote:\n\n> > Be it graft or replace, I do not think we want to invite people to\n> > use these mechansims too lightly to locally rewrite their history\n> > willy-nilly without fixing their mistakes at the object layer with\n> > \"commit --amend\", \"rebase\", \"bfg\", etc. in the longer term. So in\n> > that sense, adding a command to make it easy is not something I am\n> > enthusiastic about.\n> >\n> > On the other hand, if the user does need to use graft or replace\n> > (perhaps to prepare for casting the fixed history in stone with\n> > filter-branch), it would be good to help them avoid making mistakes\n> > while doing so and tool support may be a way to do so.\n> >\n> > So, ... I am of two minds.\n> \n> Maybe if we add a new command (or maybe a script) with a name long and\n> cryptic-looking enough like \"git create-replacement-object\" it will\n> scare casual users from touching it, while power users will be happy\n> to benefit from it.\n\nI do not think the features we are talking about are significantly more\ndangerous than \"git replace\" is in the first place. If we want to make\npeople aware of the dangers, perhaps git-replace.1 is the right place to\ndo it.\n\n-Peff\n"},{"id":"237129","messageId":"xmqq8us524pd.fsf@gitster.dls.corp.google.com","threadId":"36053","inReplyTo":"20140307171936.GC23587@sigill.intra.peff.net","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-19T22:39:42Z","receivedAt":"2014-03-19T22:39:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote:\n>\n>> > Be it graft or replace, I do not think we want to invite people to\n>> > use these mechansims too lightly to locally rewrite their history\n>> > willy-nilly without fixing their mistakes at the object layer with\n>> > \"commit --amend\", \"rebase\", \"bfg\", etc. in the longer term. So in\n>> > that sense, adding a command to make it easy is not something I am\n>> > enthusiastic about.\n>> >\n>> > On the other hand, if the user does need to use graft or replace\n>> > (perhaps to prepare for casting the fixed history in stone with\n>> > filter-branch), it would be good to help them avoid making mistakes\n>> > while doing so and tool support may be a way to do so.\n>> >\n>> > So, ... I am of two minds.\n> ...\n> I do not think the features we are talking about are significantly more\n> dangerous than \"git replace\" is in the first place. If we want to make\n> people aware of the dangers, perhaps git-replace.1 is the right place to\n> do it.\n\nSure.\n\nSo should we take the four-patch series for \"git replace --edit\"?\n"},{"id":"237248","messageId":"20140321004906.GE7774@sigill.intra.peff.net","threadId":"36053","inReplyTo":"xmqq8us524pd.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] disable grafts during fetch/push/bundle","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-03-21T00:49:06Z","receivedAt":"2014-03-21T00:49:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 19, 2014 at 03:39:42PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Mar 07, 2014 at 08:08:37AM +0100, Christian Couder wrote:\n> >\n> >> > Be it graft or replace, I do not think we want to invite people to\n> >> > use these mechansims too lightly to locally rewrite their history\n> >> > willy-nilly without fixing their mistakes at the object layer with\n> >> > \"commit --amend\", \"rebase\", \"bfg\", etc. in the longer term. So in\n> >> > that sense, adding a command to make it easy is not something I am\n> >> > enthusiastic about.\n> >> >\n> >> > On the other hand, if the user does need to use graft or replace\n> >> > (perhaps to prepare for casting the fixed history in stone with\n> >> > filter-branch), it would be good to help them avoid making mistakes\n> >> > while doing so and tool support may be a way to do so.\n> >> >\n> >> > So, ... I am of two minds.\n> > ...\n> > I do not think the features we are talking about are significantly more\n> > dangerous than \"git replace\" is in the first place. If we want to make\n> > people aware of the dangers, perhaps git-replace.1 is the right place to\n> > do it.\n> \n> Sure.\n> \n> So should we take the four-patch series for \"git replace --edit\"?\n\nI think that is certainly going in the right direction, but it is\nmissing documentation and tests still. Aside from a one-liner bug (which\nChristian pointed out on the list), I do not think it will _hurt_\nanybody. But it probably should be \"finished\" before seeing the light of\nday. I'd be happy if you wanted to pick it up for \"pu\" or even \"next\"\nwaiting and do that finishing in-tree.\n\nOtherwise, I may eventually get to it and re-roll the whole completed\nseries.\n\n-Peff\n"}]}