Re: [PATCH v3 1/4] replace: add --graft option
Christian Couder christian.cou...@gmail.com writes: Now, after having read the recent thread about git verify-commit, I understand that you also want me to drop the signature of a tag that was merged, because such signatures are added to the commit message. Huh? I am not sure if I follow. Perhaps we are talking about different things? When you are preparing a replacement for an existing commit that merges a signed tag, there are two cases: - The replacement commit still merges the same signed tag; or - The replacement commit does not merge that signed tag (it may become a single-parent commit, or it may stay to be a merge but merges a different commit on the side branch). In the former, it would be sensible to keep the mergetag and propagate it to the replacement; it is a signature over the tip of the side branch being merged by the original (and the replacement) merge, and the replacement will not affect the validity of the signature at all. In the latter, we do want to drop the mergetag for the parent you are losing in the replacement, because by definition it will be irrelevant. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Mon, Jun 30, 2014 at 8:37 AM, Junio C Hamano gits...@pobox.com wrote: Christian Couder christian.cou...@gmail.com writes: Now, after having read the recent thread about git verify-commit, I understand that you also want me to drop the signature of a tag that was merged, because such signatures are added to the commit message. Huh? I am not sure if I follow. Perhaps we are talking about different things? I think we are talking about the same thing but I might not have been clear. When you are preparing a replacement for an existing commit that merges a signed tag, there are two cases: - The replacement commit still merges the same signed tag; or - The replacement commit does not merge that signed tag (it may become a single-parent commit, or it may stay to be a merge but merges a different commit on the side branch). In the former, it would be sensible to keep the mergetag and propagate it to the replacement; it is a signature over the tip of the side branch being merged by the original (and the replacement) merge, and the replacement will not affect the validity of the signature at all. Ok, this is what is done right now by the patch series. In the latter, we do want to drop the mergetag for the parent you are losing in the replacement, because by definition it will be irrelevant. Yeah, it might be a good idea to drop the mergetag, but note that anyway such a commit probably has a title like Merge tag 'tag' and we won't automatically change this title and this title will be wrong (because we are not merging anymore this tag). So anyway in this case, --graft will do something that is not good. So it might be better in this case to just error out and say that it would be better to use --edit instead of --graft. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Sun, Jun 8, 2014 at 10:18 AM, Junio C Hamano gits...@pobox.com wrote: On Sat, Jun 7, 2014 at 11:49 PM, Christian Couder christian.cou...@gmail.com wrote: On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder christian.cou...@gmail.com wrote: /* find existing parents */ strbuf_addstr(buf, commit-buffer); Unfortunately, it looks like the above will not work if the commit-buffer contains an embedded NUL. I wonder if it is a real problem or not. Yes, it is a real problem (there was another thread on this regarding the code path that verifies GPG signature on the commit itself), which incidentally reminds us to another thing to think about in your patch as well. I *think* you shoud drop the GPG signature on the commit itself, and you also should drop the merge-tag of a parent you are not going to keep, but should keep the merge-tag of a parent you are keeping. In the v5 of the patch series, I now drop the GPG signature on the commit itself. Now, after having read the recent thread about git verify-commit, I understand that you also want me to drop the signature of a tag that was merged, because such signatures are added to the commit message. But I wonder how far should we go in this path. For example merge commits have a title like Merge branch 'dev' or Merge tag 'stuff', but this does not make sense any more if the replacement commit drops the parent corresponding to 'dev' or 'stuff'. And I don't think we should change the commit title. Thanks, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
Jeff King p...@peff.net writes: I think it would make sense to actually take this one step further, though, and move commit-buffer into the slab, as well. That has two advantages: 1. It further decreases the size of struct commit for callers who do not use save_commit_buffer. 2. It ensures that no new callers crop up who set commit-buffer but to not save the size in the slab (you can see in the patch below that I had to modify builtin/blame.c, which (ab)uses commit-buffer). It would be more disruptive to existing callers, but I think the end result would be pretty clean. The API would be something like: /* attach buffer to commit */ set_commit_buffer(struct commit *, void *buf, unsigned long size); /* get buffer, either from slab cache or by calling read_sha1_file */ void *get_commit_buffer(struct commit *, unsigned long *size); /* free() an allocated buffer from above, noop for cached buffer */ void unused_commit_buffer(struct commit *, void *buf); /* drop saved commit buffer to free memory */ void free_commit_buffer(struct commit *); The get function would serve the existing callers in pretty.c, as well as the one I'm adding elsewhere in show_signature. And it should work as a drop-in read_sha1_file/free replacement for you here. This solution has the kind of niceness to make everybody (certainly including me) who has been involved in the code path should say why didn't I think of that! ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder christian.cou...@gmail.com wrote: /* find existing parents */ strbuf_addstr(buf, commit-buffer); Unfortunately, it looks like the above will not work if the commit-buffer contains an embedded NUL. I wonder if it is a real problem or not. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Sun, Jun 08, 2014 at 08:49:45AM +0200, Christian Couder wrote: On Fri, Jun 6, 2014 at 5:44 PM, Christian Couder christian.cou...@gmail.com wrote: /* find existing parents */ strbuf_addstr(buf, commit-buffer); Unfortunately, it looks like the above will not work if the commit-buffer contains an embedded NUL. I wonder if it is a real problem or not. I ran into a similar problem recently[1] and have been pondering solutions to know the size of commit-buffer. What I've been come up with is: 1. Look up the object size via sha1_object_info. Besides being inefficient (which probably does not matter for you here, but might for using commit-buffer in a traversal), it strikes me as inelegant; is it possible for commit-buffer to ever disagree in size with the results of sha1_object_info, and if so, what happens? 2. Add an extra member len to struct commit. This is simple, but bloats struct commit, which may have a performance impact for things like rev-list, where the field will be unused. 3. Store the length of objects as a size_t, exactly sizeof(size_t) bytes before the object buffer. Provide a macro: #define OBJECT_SIZE(buf) (((size_t *)(buf))[-1]) to access it. Most callers can just use the buffer as-is, but anybody who calls free() would need to be adjusted to use a special object_free. 4. Keep a static commit_slab that points to the length for each parsed commit. We pay the same memory cost as (2), but as it's not part of the struct, the cache effects are minimized. -Peff [1] http://article.gmane.org/gmane.comp.version-control.git/250480 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Sun, Jun 08, 2014 at 07:23:33AM -0400, Jeff King wrote: 4. Keep a static commit_slab that points to the length for each parsed commit. We pay the same memory cost as (2), but as it's not part of the struct, the cache effects are minimized. I think I favor this solution, which would look something like this: -- 8 -- Subject: [PATCH] commit: add slab for commit buffer size We store the commit object buffer for later reuse as commit-buffer. However, since we store only a pointer, we must treat the result as a NUL-terminated string. This is generally OK for pretty-printing, but could be a problem for other uses. Adding a len member to struct commit would solve this, but at the cost of bloating the struct even for callers who do not care about the size or buffer (e.g., traversals like rev-list or merge-base). Instead, let's use a commit_slab so that the memory is used only when save_commit_buffer is in effect (and even then, it should have less cache impact on most uses of struct commit). Signed-off-by: Jeff King p...@peff.net --- I think it would make sense to actually take this one step further, though, and move commit-buffer into the slab, as well. That has two advantages: 1. It further decreases the size of struct commit for callers who do not use save_commit_buffer. 2. It ensures that no new callers crop up who set commit-buffer but to not save the size in the slab (you can see in the patch below that I had to modify builtin/blame.c, which (ab)uses commit-buffer). It would be more disruptive to existing callers, but I think the end result would be pretty clean. The API would be something like: /* attach buffer to commit */ set_commit_buffer(struct commit *, void *buf, unsigned long size); /* get buffer, either from slab cache or by calling read_sha1_file */ void *get_commit_buffer(struct commit *, unsigned long *size); /* free() an allocated buffer from above, noop for cached buffer */ void unused_commit_buffer(struct commit *, void *buf); /* drop saved commit buffer to free memory */ void free_commit_buffer(struct commit *); The get function would serve the existing callers in pretty.c, as well as the one I'm adding elsewhere in show_signature. And it should work as a drop-in read_sha1_file/free replacement for you here. builtin/blame.c | 2 +- commit.c| 13 - commit.h| 1 + object.c| 2 +- 4 files changed, 15 insertions(+), 3 deletions(-) diff --git a/builtin/blame.c b/builtin/blame.c index a52a279..1945ea4 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, ident, ident, path, (!contents_from ? path : (!strcmp(contents_from, -) ? standard input : contents_from))); - commit-buffer = strbuf_detach(msg, NULL); + set_commit_buffer(commit, strbuf_detach(msg, NULL), msg.len); if (!contents_from || strcmp(-, contents_from)) { struct stat st; diff --git a/commit.c b/commit.c index f479331..71143ed 100644 --- a/commit.c +++ b/commit.c @@ -302,6 +302,17 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s return 0; } +define_commit_slab(commit_size_slab, unsigned long); +static struct commit_size_slab commit_size; + +void set_commit_buffer(struct commit *item, void *buffer, unsigned long size) +{ + if (!commit_size.stride) + init_commit_size_slab(commit_size); + *commit_size_slab_at(commit_size, item) = size; + item-buffer = buffer; +} + int parse_commit(struct commit *item) { enum object_type type; @@ -324,7 +335,7 @@ int parse_commit(struct commit *item) } ret = parse_commit_buffer(item, buffer, size); if (save_commit_buffer !ret) { - item-buffer = buffer; + set_commit_buffer(item, buffer, size); return 0; } free(buffer); diff --git a/commit.h b/commit.h index a9f177b..7704ab2 100644 --- a/commit.h +++ b/commit.h @@ -48,6 +48,7 @@ struct commit *lookup_commit_reference_by_name(const char *name); struct commit *lookup_commit_or_die(const unsigned char *sha1, const char *ref_name); int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long size); +void set_commit_buffer(struct commit *item, void *buffer, unsigned long size); int parse_commit(struct commit *item); void parse_commit_or_die(struct commit *item); diff --git a/object.c b/object.c index 57a0890..c1c6a24 100644 --- a/object.c +++ b/object.c @@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t if (parse_commit_buffer(commit, buffer, size)) return NULL; if (!commit-buffer) { - commit-buffer =
Re: [PATCH v3 1/4] replace: add --graft option
On Sun, Jun 08, 2014 at 08:04:39AM -0400, Jeff King wrote: diff --git a/builtin/blame.c b/builtin/blame.c index a52a279..1945ea4 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt, ident, ident, path, (!contents_from ? path : (!strcmp(contents_from, -) ? standard input : contents_from))); - commit-buffer = strbuf_detach(msg, NULL); + set_commit_buffer(commit, strbuf_detach(msg, NULL), msg.len); Side note: this is wrong, as the fake commit created by blame here does not have its index field set. This is a bug waiting to happen the first time somebody uses a slab in builtin/blame.c. It looks like merge-recursive does the same thing in make_virtual_commit. We probably want to provide a function to allocate a commit, including the index, and use it consistently. I'll try to work up a series doing that, and the fuller slab API I mentioned in the previous message, but probably not until tomorrow. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano gits...@pobox.com wrote: Christian Couder chrisc...@tuxfamily.org writes: +static int create_graft(int argc, const char **argv, int force) +{ + unsigned char old[20], new[20]; + const char *old_ref = argv[0]; + struct commit *commit; + struct strbuf buf = STRBUF_INIT; + struct strbuf new_parents = STRBUF_INIT; + const char *parent_start, *parent_end; + int i; + + if (get_sha1(old_ref, old) 0) + die(_(Not a valid object name: '%s'), old_ref); + commit = lookup_commit_or_die(old, old_ref); Not a problem with this patch, but the above sequence somehow makes me wonder if lookup-commit-or-die is a good API for this sort of thing. Wouldn't it be more natural if it took not unsigned char old[20] but anything that would be understood by get_sha1()? It could be that this particular caller is peculiar and all the existing callers are happy, though. I didn't git grep to spot patterns in existing callers. lookup_commit_or_die() looked like a good API to me because I saw that it checked a lot of things and die in case of problems, which could make the patch shorter. + if (read_sha1_commit(old, buf)) + die(_(Invalid commit: '%s'), old_ref); + /* make sure the commit is not corrupt */ + if (parse_commit_buffer(commit, buf.buf, buf.len)) + die(_(Could not parse commit: '%s'), old_ref); It is unclear to me what you are trying to achieve with these two. If the earlier lookup-commit has returned a commit object that has already been parsed, parse_commit_buffer() would not check anything, would it? Yeah, you are right. I missed the fact that lookup_commit_or_die() calls parse_object() which itself calls read_sha1_file() and then parse_object_buffer() which calls parse_commit_buffer(). Here is a backtrace that shows this: #0 parse_commit_buffer (item=0x8597b0, buffer=0x851730, size=228) at commit.c:251 #1 0x004fa215 in parse_object_buffer (sha1=0x7fffdbf0 \tA\247\235J\213\376u\212\226\311^[\371\343^\330\234, type=OBJ_COMMIT, size=228, buffer=0x851730, eaten_p=0x7fffdacc) at object.c:198 #2 0x004fa50a in parse_object (sha1=0x7fffdbf0 \tA\247\235J\213\376u\212\226\311^[\371\343^\330\234) at object.c:264 #3 0x004a89ef in lookup_commit_reference_gently (sha1=0x7fffdbf0 \tA\247\235J\213\376u\212\226\311^[\371\343^\330\234, quiet=0) at commit.c:38 #4 0x004a8a48 in lookup_commit_reference (sha1=0x7fffdbf0 \tA\247\235J\213\376u\212\226\311^[\371\343^\330\234) at commit.c:47 #5 0x004a8a67 in lookup_commit_or_die (sha1=0x7fffdbf0 \tA\247\235J\213\376u\212\226\311^[\371\343^\330\234, ref_name=0x7fffe465 093e41a79d4a8bfe3c758a96c95e5bf9e35ed89c) at commit.c:52 #6 0x0047f89a in create_graft (argc=1, argv=0x7fffe130, refdir=0x0, force=0) at builtin/replace.c:353 #7 0x0047ff71 in cmd_replace (argc=1, argv=0x7fffe130, prefix=0x0) at builtin/replace.c:461 #8 0x00405441 in run_builtin (p=0x7eee90, argc=3, argv=0x7fffe130) at git.c:314 #9 0x0040563a in handle_builtin (argc=3, argv=0x7fffe130) at git.c:487 #10 0x00405754 in run_argv (argcp=0x7fffe01c, argv=0x7fffe020) at git.c:533 #11 0x004058f9 in main (argc=3, av=0x7fffe128) at git.c:616 A typical sequence would look more like this: commit = lookup_commit(...); if (parse_commit(commit)) oops there is an error; /* otherwise */ use(commit-buffer); without reading a commit object using low-level read-sha1-file interface yourself, no? Yeah, or I could just rely on the fact that lookup_commit_or_die() already parses the commit, with something like this: if (get_sha1(old_ref, old) 0) die(_(Not a valid object name: '%s'), old_ref); /* parse the commit buffer to make sure the commit is not corrupt */ commit = lookup_commit_or_die(old, old_ref); /* find existing parents */ parent_start = buf.buf; parent_start += 46; /* tree + hex sha1 + \n */ parent_end = parent_start; ... Thanks, Christian. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
On Fri, Jun 6, 2014 at 5:29 PM, Christian Couder christian.cou...@gmail.com wrote: Yeah, or I could just rely on the fact that lookup_commit_or_die() already parses the commit, with something like this: if (get_sha1(old_ref, old) 0) die(_(Not a valid object name: '%s'), old_ref); /* parse the commit buffer to make sure the commit is not corrupt */ commit = lookup_commit_or_die(old, old_ref); /* find existing parents */ parent_start = buf.buf; parent_start += 46; /* tree + hex sha1 + \n */ parent_end = parent_start; This last part should be: /* find existing parents */ strbuf_addstr(buf, commit-buffer); parent_start = buf.buf; parent_start += 46; /* tree + hex sha1 + \n */ parent_end = parent_start; ... I will send an updated patch series soon. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
Christian Couder christian.cou...@gmail.com writes: On Thu, Jun 5, 2014 at 11:49 PM, Junio C Hamano gits...@pobox.com wrote: Christian Couder chrisc...@tuxfamily.org writes: +static int create_graft(int argc, const char **argv, int force) +{ + unsigned char old[20], new[20]; + const char *old_ref = argv[0]; + struct commit *commit; + struct strbuf buf = STRBUF_INIT; + struct strbuf new_parents = STRBUF_INIT; + const char *parent_start, *parent_end; + int i; + + if (get_sha1(old_ref, old) 0) + die(_(Not a valid object name: '%s'), old_ref); + commit = lookup_commit_or_die(old, old_ref); Not a problem with this patch, but the above sequence somehow makes me wonder if lookup-commit-or-die is a good API for this sort of thing. Wouldn't it be more natural if it took not unsigned char old[20] but anything that would be understood by get_sha1()? It could be that this particular caller is peculiar and all the existing callers are happy, though. I didn't git grep to spot patterns in existing callers. lookup_commit_or_die() looked like a good API to me because I saw that it checked a lot of things and die in case of problems, which could make the patch shorter. Perhaps I was vague. find the commit object and die if that object is not properly formed is a good thing. I was referring to the fact that you - first had to do get-sha1 yourself to turn the extended sha1 you got from the user into a binary object name, and die with your own error message if the user input was malformed. - and then had to call lookup-commit-or-die to do the checking and let it die. It would have been simpler for *this* particular codepath if we had another helper function you can use like so: commit = lookup_commit_with_extended_sha1_or_die(old_ref); which did the two-call sequence you handrolled above, and I was wondering if other existing callers to lookup-commit-or-die wished the same thing. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/4] replace: add --graft option
Christian Couder chrisc...@tuxfamily.org writes: +static int create_graft(int argc, const char **argv, int force) +{ + unsigned char old[20], new[20]; + const char *old_ref = argv[0]; + struct commit *commit; + struct strbuf buf = STRBUF_INIT; + struct strbuf new_parents = STRBUF_INIT; + const char *parent_start, *parent_end; + int i; + + if (get_sha1(old_ref, old) 0) + die(_(Not a valid object name: '%s'), old_ref); + commit = lookup_commit_or_die(old, old_ref); Not a problem with this patch, but the above sequence somehow makes me wonder if lookup-commit-or-die is a good API for this sort of thing. Wouldn't it be more natural if it took not unsigned char old[20] but anything that would be understood by get_sha1()? It could be that this particular caller is peculiar and all the existing callers are happy, though. I didn't git grep to spot patterns in existing callers. + if (read_sha1_commit(old, buf)) + die(_(Invalid commit: '%s'), old_ref); + /* make sure the commit is not corrupt */ + if (parse_commit_buffer(commit, buf.buf, buf.len)) + die(_(Could not parse commit: '%s'), old_ref); It is unclear to me what you are trying to achieve with these two. If the earlier lookup-commit has returned a commit object that has already been parsed, parse_commit_buffer() would not check anything, would it? A typical sequence would look more like this: commit = lookup_commit(...); if (parse_commit(commit)) oops there is an error; /* otherwise */ use(commit-buffer); without reading a commit object using low-level read-sha1-file interface yourself, no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 1/4] replace: add --graft option
The usage string for this option is: git replace [-f] --graft commit [parent...] First we create a new commit that is the same as commit except that its parents are [parents...] Then we create a replace ref that replace commit with the commit we just created. With this new option, it should be straightforward to convert grafts to replace refs. Signed-off-by: Christian Couder chrisc...@tuxfamily.org --- builtin/replace.c | 84 ++- 1 file changed, 83 insertions(+), 1 deletion(-) diff --git a/builtin/replace.c b/builtin/replace.c index 1bb491d..9d1e82f 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -17,6 +17,7 @@ static const char * const git_replace_usage[] = { N_(git replace [-f] object replacement), N_(git replace [-f] --edit object), + N_(git replace [-f] --graft commit [parent...]), N_(git replace -d object...), N_(git replace [--format=format] [-l [pattern]]), NULL @@ -294,6 +295,76 @@ static int edit_and_replace(const char *object_ref, int force) return replace_object_sha1(object_ref, old, replacement, new, force); } +static int read_sha1_commit(const unsigned char *sha1, struct strbuf *dst) +{ + void *buf; + enum object_type type; + unsigned long size; + + buf = read_sha1_file(sha1, type, size); + if (!buf) + return error(_(cannot read object %s), sha1_to_hex(sha1)); + if (type != OBJ_COMMIT) { + free(buf); + return error(_(object %s is not a commit), sha1_to_hex(sha1)); + } + strbuf_attach(dst, buf, size, size + 1); + return 0; +} + +static int create_graft(int argc, const char **argv, int force) +{ + unsigned char old[20], new[20]; + const char *old_ref = argv[0]; + struct commit *commit; + struct strbuf buf = STRBUF_INIT; + struct strbuf new_parents = STRBUF_INIT; + const char *parent_start, *parent_end; + int i; + + if (get_sha1(old_ref, old) 0) + die(_(Not a valid object name: '%s'), old_ref); + commit = lookup_commit_or_die(old, old_ref); + if (read_sha1_commit(old, buf)) + die(_(Invalid commit: '%s'), old_ref); + + /* make sure the commit is not corrupt */ + if (parse_commit_buffer(commit, buf.buf, buf.len)) + die(_(Could not parse commit: '%s'), old_ref); + + /* find existing parents */ + parent_start = buf.buf; + parent_start += 46; /* tree + hex sha1 + \n */ + parent_end = parent_start; + + while (starts_with(parent_end, parent )) + parent_end += 48; /* parent + hex sha1 + \n */ + + /* prepare new parents */ + for (i = 1; i argc; i++) { + unsigned char sha1[20]; + if (get_sha1(argv[i], sha1) 0) + die(_(Not a valid object name: '%s'), argv[i]); + lookup_commit_or_die(sha1, argv[i]); + strbuf_addf(new_parents, parent %s\n, sha1_to_hex(sha1)); + } + + /* replace existing parents with new ones */ + strbuf_splice(buf, parent_start - buf.buf, parent_end - parent_start, + new_parents.buf, new_parents.len); + + if (write_sha1_file(buf.buf, buf.len, commit_type, new)) + die(_(could not write replacement commit for: '%s'), old_ref); + + strbuf_release(new_parents); + strbuf_release(buf); + + if (!hashcmp(old, new)) + return error(new commit is the same as the old one: '%s', sha1_to_hex(old)); + + return replace_object_sha1(old_ref, old, replacement, new, force); +} + int cmd_replace(int argc, const char **argv, const char *prefix) { int force = 0; @@ -303,12 +374,14 @@ int cmd_replace(int argc, const char **argv, const char *prefix) MODE_LIST, MODE_DELETE, MODE_EDIT, + MODE_GRAFT, 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_CMDMODE('g', graft, cmdmode, N_(change a commit's parents), MODE_GRAFT), 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() @@ -325,7 +398,10 @@ 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 cmdmode != MODE_EDIT) + if (force + cmdmode != MODE_REPLACE +