[RFC/PATCH 0/4] implement "git cherry-pick A..B"

[RFC/PATCH 0/4] implement "git cherry-pick A..B"

[Christian Couder]

This an RFC patch series to implement "git cherry-pick A..B" and
"git revert A..B".

There is no documentation yet, and there is no way to continue or
abort the process when cherry-picking fails.

Christian Couder (4):
  revert: use run_command_v_opt() instead of execv_git_cmd()
  revert: refactor code into a do_pick_commit() function
  revert: allow cherry-picking a range of commits
  revert: add tests to check cherry-picking a range of commits

 builtin/revert.c             |   97 +++++++++++++++++++++++++++++-------------
 t/t3508-cherry-pick-range.sh |   65 ++++++++++++++++++++++++++++
 2 files changed, 132 insertions(+), 30 deletions(-)
 create mode 100755 t/t3508-cherry-pick-range.sh

[RFC/PATCH 1/4] revert: use run_command_v_opt() instead of execv_git_cmd()

[Christian Couder]

This is needed by the following commits, because we are going
to cherry pick many commits instead of just one.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 7976b5a..9737ad5 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -534,7 +534,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 			args[i++] = defmsg;
 		}
 		args[i] = NULL;
-		return execv_git_cmd(args);
+		return run_command_v_opt(args, RUN_GIT_CMD);
 	}
 	free_message(&msg);
 	free(defmsg);
-- 
1.7.1.346.g7c1d7.dirty

[RFC/PATCH 2/4] revert: refactor code into a do_pick_commit() function

[Christian Couder]

This is needed because we are going to make it possible
to cherry-pick many commits instead of just one in the following
commit. And we will be able to do that by just calling
do_pick_commit() one for each commit to cherry-pick.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   52 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 9737ad5..70372dc 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -365,7 +365,7 @@ static void do_recursive_merge(struct commit *base, struct commit *next,
 	fprintf(stderr, "Finished one %s.\n", me);
 }
 
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int do_pick_commit()
 {
 	unsigned char head[20];
 	struct commit *base, *next, *parent;
@@ -374,28 +374,6 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	char *defmsg = NULL;
 	struct strbuf msgbuf = STRBUF_INIT;
 
-	git_config(git_default_config, NULL);
-	me = action == REVERT ? "revert" : "cherry-pick";
-	setenv(GIT_REFLOG_ACTION, me, 0);
-	parse_args(argc, argv);
-
-	/* this is copied from the shell script, but it's never triggered... */
-	if (action == REVERT && !no_replay)
-		die("revert is incompatible with replay");
-
-	if (allow_ff) {
-		if (signoff)
-			die("cherry-pick --ff cannot be used with --signoff");
-		if (no_commit)
-			die("cherry-pick --ff cannot be used with --no-commit");
-		if (no_replay)
-			die("cherry-pick --ff cannot be used with -x");
-		if (edit)
-			die("cherry-pick --ff cannot be used with --edit");
-	}
-
-	if (read_cache() < 0)
-		die("git %s: failed to read the index", me);
 	if (no_commit) {
 		/*
 		 * We do not intend to commit immediately.  We just want to
@@ -542,6 +520,34 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	return 0;
 }
 
+static int revert_or_cherry_pick(int argc, const char **argv)
+{
+	git_config(git_default_config, NULL);
+	me = action == REVERT ? "revert" : "cherry-pick";
+	setenv(GIT_REFLOG_ACTION, me, 0);
+	parse_args(argc, argv);
+
+	/* this is copied from the shell script, but it's never triggered... */
+	if (action == REVERT && !no_replay)
+		die("revert is incompatible with replay");
+
+	if (allow_ff) {
+		if (signoff)
+			die("cherry-pick --ff cannot be used with --signoff");
+		if (no_commit)
+			die("cherry-pick --ff cannot be used with --no-commit");
+		if (no_replay)
+			die("cherry-pick --ff cannot be used with -x");
+		if (edit)
+			die("cherry-pick --ff cannot be used with --edit");
+	}
+
+	if (read_cache() < 0)
+		die("git %s: failed to read the index", me);
+
+	return do_pick_commit();
+}
+
 int cmd_revert(int argc, const char **argv, const char *prefix)
 {
 	if (isatty(0))
-- 
1.7.1.346.g7c1d7.dirty

Re: [RFC/PATCH 2/4] revert: refactor code into a do_pick_commit() function

[Ramkumar Ramachandra]

Hi Christian,

Excellent feature to work on- I've wanted to cherry pick a range several times.

Christian Couder wrote:
> -static int revert_or_cherry_pick(int argc, const char **argv)
> +static int do_pick_commit()
>  {
>        unsigned char head[20];
>        struct commit *base, *next, *parent;

Is there a better way to do this instead of allocating memory for each
commit? When you cherry pick a lot of commits, it might make sense to
use a shared memory pool.

> +       /* this is copied from the shell script, but it's never triggered... */
> +       if (action == REVERT && !no_replay)
> +               die("revert is incompatible with replay");

If it's here for historical reasons, why don't you remove it now?

-- Ram

Re: [RFC/PATCH 2/4] revert: refactor code into a do_pick_commit() function

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Christian Couder wrote:

>> -static int revert_or_cherry_pick(int argc, const char **argv)
>> +static int do_pick_commit()
>>  {
>>        unsigned char head[20];
>>        struct commit *base, *next, *parent;
>
> Is there a better way to do this instead of allocating memory for each
> commit? When you cherry pick a lot of commits, it might make sense to
> use a shared memory pool.

In fact they do belong to a pool, as far as I can tell.

  lookup_commit_reference ->
   lookup_commit_reference_gently ->
    parse_object ->
     parse_object_buffer ->
      lookup_commit ->
       lookup_object (or create_object, alloc_commit_node)

alloc_commit_node is defined in alloc.c to draw from a growing array
of fixed-size, never-freed commit objects.

Hope that helps.
Jonathan

Re: [RFC/PATCH 2/4] revert: refactor code into a do_pick_commit() function

[Christian Couder]

Hi Ram,

On Sunday 30 May 2010 13:19:39 Ramkumar Ramachandra wrote:
> Hi Christian,
> 
> Excellent feature to work on- I've wanted to cherry pick a range several
>  times.

Thanks!

> Christian Couder wrote:
> > -static int revert_or_cherry_pick(int argc, const char **argv)
> > +static int do_pick_commit()
> >  {
> >        unsigned char head[20];
> >        struct commit *base, *next, *parent;
> 
> Is there a better way to do this instead of allocating memory for each
> commit? When you cherry pick a lot of commits, it might make sense to
> use a shared memory pool.

I think that most of the time I will cherry pick less than 20 commits so I 
think the memory usage is not so important. And in any case it could be 
improved later if there is a real need.

> > +       /* this is copied from the shell script, but it's never
> > triggered... */ +       if (action == REVERT && !no_replay)
> > +               die("revert is incompatible with replay");
> 
> If it's here for historical reasons, why don't you remove it now?

Right, I will have a look at removing that.

Thanks,
Christian.

[RFC/PATCH 3/4] revert: allow cherry-picking a range of commits

[Christian Couder]

This makes it possible to pass a range of commits like A..B
to "git cherry-pick" and to "git revert" to process many
commits instead of just one.

But there is currently no way to continue cherry-picking or
reverting if there is a problem with one commit. It's also
not possible to abort the whole process. Some future work
should provide the --continue and --abort options to do
just that.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/revert.c |   43 +++++++++++++++++++++++++++++++++++++------
 1 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 70372dc..c281a80 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -53,7 +53,6 @@ static void parse_args(int argc, const char **argv)
 {
 	const char * const * usage_str =
 		action == REVERT ?  revert_usage : cherry_pick_usage;
-	unsigned char sha1[20];
 	int noop;
 	struct option options[] = {
 		OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
@@ -82,11 +81,6 @@ static void parse_args(int argc, const char **argv)
 		usage_with_options(usage_str, options);
 
 	commit_name = argv[0];
-	if (get_sha1(commit_name, sha1))
-		die ("Cannot find '%s'", commit_name);
-	commit = lookup_commit_reference(sha1);
-	if (!commit)
-		exit(1);
 }
 
 struct commit_message {
@@ -522,6 +516,9 @@ static int do_pick_commit()
 
 static int revert_or_cherry_pick(int argc, const char **argv)
 {
+	const char *dotdot;
+	unsigned char sha1[20];
+
 	git_config(git_default_config, NULL);
 	me = action == REVERT ? "revert" : "cherry-pick";
 	setenv(GIT_REFLOG_ACTION, me, 0);
@@ -545,6 +542,40 @@ static int revert_or_cherry_pick(int argc, const char **argv)
 	if (read_cache() < 0)
 		die("git %s: failed to read the index", me);
 
+	dotdot = strstr(commit_name, "..");
+	if (dotdot) {
+		struct rev_info revs;
+		const char *argv[4];
+		int argc = 0;
+
+		argv[argc++] = NULL;
+		if (action != REVERT)
+			argv[argc++] = "--reverse";
+		argv[argc++] = commit_name;
+		argv[argc++] = NULL;
+
+		init_revisions(&revs, NULL);
+		setup_revisions(argc - 1, argv, &revs, NULL);
+		if (prepare_revision_walk(&revs))
+			die("revision walk setup failed");
+
+		if (!revs.commits)
+			die("empty range passed");
+
+		while ( (commit = get_revision(&revs)) ) {
+			int res = do_pick_commit();
+			if (res)
+				return res;
+		}
+		return 0;
+	}
+
+	if (get_sha1(commit_name, sha1))
+		die ("Cannot find '%s'", commit_name);
+	commit = lookup_commit_reference(sha1);
+	if (!commit)
+		exit(1);
+
 	return do_pick_commit();
 }
 
-- 
1.7.1.346.g7c1d7.dirty

Re: [RFC/PATCH 3/4] revert: allow cherry-picking a range of commits

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> This makes it possible to pass a range of commits like A..B
> to "git cherry-pick" and to "git revert" to process many
> commits instead of just one.

> @@ -545,6 +542,40 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  	if (read_cache() < 0)
>  		die("git %s: failed to read the index", me);
>  
> +	dotdot = strstr(commit_name, "..");
> +	if (dotdot) {
> +		struct rev_info revs;
> +		const char *argv[4];
> +		int argc = 0;
> +
> +		argv[argc++] = NULL;
> +		if (action != REVERT)
> +			argv[argc++] = "--reverse";
> +		argv[argc++] = commit_name;
> +		argv[argc++] = NULL;
> +
> +		init_revisions(&revs, NULL);

The goal of the series is a worthy one, but I would imagine people would
want to run these while on "maint":

    git cherry-pick master~2..master
    git cherry-pick master^ master

or even

    git cherry-pick -2 master

How about enumerating the commits with an equivalent of

    git rev-list --no-walk "$@"

as an alternative implementation?

The current behaviour would fall out just as a natural special case
because

    git rev-list --no-walk $commit == $commit

Hmm?

Re: [RFC/PATCH 3/4] revert: allow cherry-picking a range of commits

[Christian Couder]

On Saturday 29 May 2010 17:27:01 Junio C Hamano wrote:
> Christian Couder <chriscool@tuxfamily.org> writes:
> > This makes it possible to pass a range of commits like A..B
> > to "git cherry-pick" and to "git revert" to process many
> > commits instead of just one.
> >
> > @@ -545,6 +542,40 @@ static int revert_or_cherry_pick(int argc, const
> > char **argv) if (read_cache() < 0)
> >  		die("git %s: failed to read the index", me);
> >
> > +	dotdot = strstr(commit_name, "..");
> > +	if (dotdot) {
> > +		struct rev_info revs;
> > +		const char *argv[4];
> > +		int argc = 0;
> > +
> > +		argv[argc++] = NULL;
> > +		if (action != REVERT)
> > +			argv[argc++] = "--reverse";
> > +		argv[argc++] = commit_name;
> > +		argv[argc++] = NULL;
> > +
> > +		init_revisions(&revs, NULL);
> 
> The goal of the series is a worthy one, but I would imagine people would
> want to run these while on "maint":
> 
>     git cherry-pick master~2..master
>     git cherry-pick master^ master
> 
> or even
> 
>     git cherry-pick -2 master
> 
> How about enumerating the commits with an equivalent of
> 
>     git rev-list --no-walk "$@"
> 
> as an alternative implementation?

I agree that it would be nice, but I am not sure it would allow using "-2 
master" as arguments because "--no-walk" seems to take over "-2": 

$ git rev-list --no-walk -2 master
81fa024cd8e336ba257f13fe7724b95baacfa3ad
$

> The current behaviour would fall out just as a natural special case
> because
> 
>     git rev-list --no-walk $commit == $commit
> 
> Hmm?

Yes, I will provide an updated patch series using the equivalent of '--no-walk 
"$@"' but arguments like "-2 master" will not work.

Thanks,
Christian.


 

Re: [RFC/PATCH 3/4] revert: allow cherry-picking a range of commits

[Johannes Schindelin]

Hi,

On Sat, 29 May 2010, Christian Couder wrote:

> diff --git a/builtin/revert.c b/builtin/revert.c
> index 70372dc..c281a80 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -545,6 +542,40 @@ static int revert_or_cherry_pick(int argc, const char **argv)
>  	if (read_cache() < 0)
>  		die("git %s: failed to read the index", me);
>  
> +	dotdot = strstr(commit_name, "..");
> +	if (dotdot) {
> +		struct rev_info revs;
> +		const char *argv[4];
> +		int argc = 0;
> +
> +		argv[argc++] = NULL;
> +		if (action != REVERT)
> +			argv[argc++] = "--reverse";
> +		argv[argc++] = commit_name;
> +		argv[argc++] = NULL;

Maybe "--no-merges"?

> +		init_revisions(&revs, NULL);
> +		setup_revisions(argc - 1, argv, &revs, NULL);
> +		if (prepare_revision_walk(&revs))
> +			die("revision walk setup failed");
> +
> +		if (!revs.commits)
> +			die("empty range passed");
> +
> +		while ( (commit = get_revision(&revs)) ) {

The style more in linet with the rest of the source code would be:

		while ((commit = get_revision(&revs))) {

The rest of the patch series looks very, very good to me. I totally agree 
that we do not have to implement the --abort and --continue for now, as 
well as the HEAD-detaching business we're used to from rebase and am.

Thanks,
Dscho

Re: [RFC/PATCH 3/4] revert: allow cherry-picking a range of commits

[Christian Couder]

On Saturday 29 May 2010 17:47:10 Johannes Schindelin wrote:
Hi Dscho,

> Hi,
> 
> On Sat, 29 May 2010, Christian Couder wrote:
> > diff --git a/builtin/revert.c b/builtin/revert.c
> > index 70372dc..c281a80 100644
> > --- a/builtin/revert.c
> > +++ b/builtin/revert.c
> > @@ -545,6 +542,40 @@ static int revert_or_cherry_pick(int argc, const
> > char **argv) if (read_cache() < 0)
> >  		die("git %s: failed to read the index", me);
> >
> > +	dotdot = strstr(commit_name, "..");
> > +	if (dotdot) {
> > +		struct rev_info revs;
> > +		const char *argv[4];
> > +		int argc = 0;
> > +
> > +		argv[argc++] = NULL;
> > +		if (action != REVERT)
> > +			argv[argc++] = "--reverse";
> > +		argv[argc++] = commit_name;
> > +		argv[argc++] = NULL;
> 
> Maybe "--no-merges"?

I will have a look at it. Thanks!
 
> > +		init_revisions(&revs, NULL);
> > +		setup_revisions(argc - 1, argv, &revs, NULL);
> > +		if (prepare_revision_walk(&revs))
> > +			die("revision walk setup failed");
> > +
> > +		if (!revs.commits)
> > +			die("empty range passed");
> > +
> > +		while ( (commit = get_revision(&revs)) ) {
> 
> The style more in linet with the rest of the source code would be:
> 
> 		while ((commit = get_revision(&revs))) {

Ok.

> The rest of the patch series looks very, very good to me. I totally agree
> that we do not have to implement the --abort and --continue for now, as
> well as the HEAD-detaching business we're used to from rebase and am.

Thanks for the kind words,
Christian.

Re: [RFC/PATCH 3/4] revert: allow cherry-picking a range of commits

[Christian Couder]

On Sunday 30 May 2010 08:45:52 Christian Couder wrote:
> On Saturday 29 May 2010 17:47:10 Johannes Schindelin wrote:
Hi Dscho,

> Hi Dscho,
> 
> > Hi,
> >
> > On Sat, 29 May 2010, Christian Couder wrote:
> > > diff --git a/builtin/revert.c b/builtin/revert.c
> > > index 70372dc..c281a80 100644
> > > --- a/builtin/revert.c
> > > +++ b/builtin/revert.c
> > > @@ -545,6 +542,40 @@ static int revert_or_cherry_pick(int argc, const
> > > char **argv) if (read_cache() < 0)
> > >  		die("git %s: failed to read the index", me);
> > >
> > > +	dotdot = strstr(commit_name, "..");
> > > +	if (dotdot) {
> > > +		struct rev_info revs;
> > > +		const char *argv[4];
> > > +		int argc = 0;
> > > +
> > > +		argv[argc++] = NULL;
> > > +		if (action != REVERT)
> > > +			argv[argc++] = "--reverse";
> > > +		argv[argc++] = commit_name;
> > > +		argv[argc++] = NULL;
> >
> > Maybe "--no-merges"?
> 
> I will have a look at it. Thanks!

I decided against using it as it would be incompatible with the -m option to 
revert or cherry-pick a merge.

Thanks,
Christian.

[RFC/PATCH 4/4] revert: add tests to check cherry-picking a range of commits

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t3508-cherry-pick-range.sh |   65 ++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 65 insertions(+), 0 deletions(-)
 create mode 100755 t/t3508-cherry-pick-range.sh

diff --git a/t/t3508-cherry-pick-range.sh b/t/t3508-cherry-pick-range.sh
new file mode 100755
index 0000000..d83b74b
--- /dev/null
+++ b/t/t3508-cherry-pick-range.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+
+test_description='test cherry-picking a range of commits'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	echo first > file1 &&
+	git add file1 &&
+	test_tick &&
+	git commit -m "first" &&
+	git tag first &&
+
+	git checkout -b other &&
+	for val in second third fourth
+	do
+		echo $val >> file1 &&
+		git add file1 &&
+		test_tick &&
+		git commit -m "$val" &&
+		git tag $val
+	done
+'
+
+test_expect_success 'cherry-pick first..fourth works' '
+	git checkout master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick first..fourth &&
+	git diff --quiet other &&
+	git diff --quiet HEAD other &&
+	test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify fourth)"
+'
+
+test_expect_success 'cherry-pick --ff first..fourth works' '
+	git checkout master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick --ff first..fourth &&
+	git diff --quiet other &&
+	git diff --quiet HEAD other &&
+	test "$(git rev-parse --verify HEAD)" = "$(git rev-parse --verify fourth)"
+'
+
+test_expect_success 'cherry-pick -n first..fourth works' '
+	git checkout master &&
+	git reset --hard first &&
+	test_tick &&
+	git cherry-pick -n first..fourth &&
+	git diff --quiet other &&
+	git diff --cached --quiet other &&
+	git diff --quiet HEAD first
+'
+
+test_expect_success 'revert first..fourth works' '
+	git checkout master &&
+	git reset --hard fourth &&
+	test_tick &&
+	git revert first..fourth &&
+	git diff --quiet first &&
+	git diff --cached --quiet first &&
+	git diff --quiet HEAD first
+'
+
+test_done
-- 
1.7.1.346.g7c1d7.dirty

Re: [RFC/PATCH 0/4] implement "git cherry-pick A..B"

[Sverre Rabbelier]

Heya,

On Sat, May 29, 2010 at 06:40, Christian Couder <chriscool@tuxfamily.org> wrote:
> This an RFC patch series to implement "git cherry-pick A..B" and
> "git revert A..B".

Awesome! Please keep working on this, I would love to see this implemented :).

-- 
Cheers,

Sverre Rabbelier

Re: [RFC/PATCH 0/4] implement "git cherry-pick A..B"

[Christian Couder]

On Saturday 29 May 2010 15:12:23 Sverre Rabbelier wrote:
> Heya,
> 
> On Sat, May 29, 2010 at 06:40, Christian Couder <chriscool@tuxfamily.org> 
wrote:
> > This an RFC patch series to implement "git cherry-pick A..B" and
> > "git revert A..B".
> 
> Awesome! Please keep working on this, I would love to see this implemented
>  :).

Yeah, I will continue working on this.

Thanks for the kind words,
Christian.