Re: [PATCH v3 1/4] replace: add --graft option

2014-06-30 Thread Junio C Hamano 
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

2014-06-30 Thread Christian Couder 
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

2014-06-29 Thread Christian Couder 
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

2014-06-09 Thread Junio C Hamano 
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

2014-06-08 Thread Christian Couder 
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

2014-06-08 Thread Jeff King 
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

2014-06-08 Thread Jeff King 
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

2014-06-08 Thread Jeff King 
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

2014-06-06 Thread Christian Couder 
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

2014-06-06 Thread Christian Couder 
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

2014-06-06 Thread Junio C Hamano 
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

2014-06-05 Thread Junio C Hamano 
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

2014-06-04 Thread Christian Couder 
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 
+