login
Search
Changes since the previous series are the following: - added some tests in a new patch - added some documentation in a new patch - added a patch to use the new "reset" function in builtin-merge.c - made --ff incompatible with --edit -x and --signoff too, thanks to Paolo - improved the first patch with suggestions from Stephen Boyd, thanks - made a "struct lock_file" static to initialize it in the first patch - improved commit messages Christian Couder (8): reset: refactor updating heads into a static function reset: refactor reseting in its own function reset: make reset function non static and declare it in "reset.h" revert: add --ff option to allow fast forward when cherry-picking cherry-pick: add tests for new --ff option Documentation: describe new cherry-pick --ff option rebase -i: use new --ff cherry-pick option merge: use new "reset" function instead of running "git read-tree" Stephan Beyer (1): revert: libify cherry-pick and revert functionnality Documentation/git-cherry-pick.txt | 6 +- Makefile | 2 + builtin-merge.c | 31 ++--- builtin-reset.c | 175 ++++++++++++---------- builtin-revert.c | 303 +++++++++---------------------------- git-rebase--interactive.sh | 15 +-- pick.c | 218 ++++++++++++++++++++++++++ pick.h | 13 ++ reset.h | 10 ++ t/t3506-cherry-pick-ff.sh | 38 +++++ 10 files changed, 471 insertions(+), 340 deletions(-) create mode 100644 pick.c create mode 100644 pick.h create mode 100644 reset.h create mode 100755 t/t3506-cherry-pick-ff.sh --
Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- t/t3506-cherry-pick-ff.sh | 38 ++++++++++++++++++++++++++++++++++++++ 1 files changed, 38 insertions(+), 0 deletions(-) create mode 100755 t/t3506-cherry-pick-ff.sh diff --git a/t/t3506-cherry-pick-ff.sh b/t/t3506-cherry-pick-ff.sh new file mode 100755 index 0000000..21c0729 --- /dev/null +++ b/t/t3506-cherry-pick-ff.sh @@ -0,0 +1,38 @@ +#!/bin/sh + +test_description='test cherry-picking with --ff option' + +. ./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 && + echo second >> file1 && + git add file1 && + test_tick && + git commit -m "second" && + git tag second +' + +test_expect_success 'cherry-pick using --ff fast forwards' ' + git checkout master && + git reset --hard first && + test_tick && + git cherry-pick --ff second && + test "$(git rev-parse --verify HEAD)" = "$(git rev-parse --verify second)" +' + +test_expect_success 'cherry-pick not using --ff does not fast forwards' ' + git checkout master && + git reset --hard first && + test_tick && + git cherry-pick second && + test "$(git rev-parse --verify HEAD)" != "$(git rev-parse --verify second)" +' + +test_done -- 1.6.6.1.557.g77031 --
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-reset.c | 8 ++++----
reset.h | 10 ++++++++++
2 files changed, 14 insertions(+), 4 deletions(-)
create mode 100644 reset.h
diff --git a/builtin-reset.c b/builtin-reset.c
index bf97931..b004f9a 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -20,6 +20,7 @@
#include "parse-options.h"
#include "unpack-trees.h"
#include "cache-tree.h"
+#include "reset.h"
static const char * const git_reset_usage[] = {
"git reset [--mixed | --soft | --hard | --merge] [-q] [<commit>]",
@@ -27,7 +28,6 @@ static const char * const git_reset_usage[] = {
NULL
};
-enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
static const char *reset_type_names[] = { "mixed", "soft", "hard", "merge", NULL };
static char *args_to_str(const char **argv)
@@ -235,9 +235,9 @@ static int update_heads(unsigned char *sha1)
return update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
}
-static int reset(const char *rev, const char *prefix,
- int reset_type, int quiet, int patch_mode,
- int argc, const char **argv)
+int reset(const char *rev, const char *prefix,
+ int reset_type, int quiet, int patch_mode,
+ int argc, const char **argv)
{
struct commit *commit;
unsigned char sha1[20];
diff --git a/reset.h b/reset.h
new file mode 100644
index 0000000..c8497e4
--- /dev/null
+++ b/reset.h
@@ -0,0 +1,10 @@
+#ifndef RESET_H
+#define RESET_H
+
+enum reset_type { MIXED, SOFT, HARD, MERGE, NONE };
+
+int reset(const char *rev, const char *prefix,
+ int reset_type, int quiet, int patch_mode,
+ int argc, const char **argv);
+
+#endif
--
1.6.6.1.557.g77031
--
As "git merge" fast forwards if possible, it seems sensible to
have such a feature for "git cherry-pick" too, especially as it
could be used in git-rebase--interactive.sh.
Maybe this option could be made the default in the long run, with
another --no-ff option to disable this default behavior, but that
could make some scripts backward incompatible and/or that would
require testing if some GIT_AUTHOR_* environment variables are
set. So we don't do that for now.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-revert.c | 35 +++++++++++++++++++++++++++++++----
1 files changed, 31 insertions(+), 4 deletions(-)
diff --git a/builtin-revert.c b/builtin-revert.c
index 7488d4c..b78926f 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -9,6 +9,7 @@
#include "revision.h"
#include "rerere.h"
#include "pick.h"
+#include "reset.h"
/*
* This implements the builtins revert and cherry-pick.
@@ -31,13 +32,19 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-static int edit, no_commit, mainline, signoff;
+static int edit, no_commit, mainline, signoff, ff_ok;
static int flags;
static struct commit *commit;
static int allow_rerere_auto;
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
+static void die_if_ff_incompatible(int opt_set, const char *opt_name)
+{
+ if (opt_set)
+ die ("options --ff and %s are incompatible", opt_name);
+}
+
static void parse_args(int argc, const char **argv)
{
const char * const * usage_str =
@@ -51,6 +58,7 @@ static void parse_args(int argc, const char **argv)
OPT_BIT('x', NULL, &flags, "append commit name when cherry-picking", PICK_ADD_NOTE),
OPT_BOOLEAN('r', NULL, &noop, "no-op (backward compatibility)"),
OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
+ OPT_BOOLEAN(0, "ff", &ff_ok, "allow fast forward"),
OPT_INTEGER('m', "mainline", &mainline, "parent number"),
OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
OPT_END(),
@@ -71,6 +79,12 @@ static void ...With this check, you are saying:
If we are cherry-picking commit $C, and if the current HEAD is
the first parent of $C, then just reset to $C instead of running a
cherry-pick.
I didn't check if you have access to commit->parents->item->object.sha1 at
this point in the codepath, though (e.g. have you parsed "commit" yet?).
If the goal is to make untouched 'pick' in rebase-i to fast forward
without actually running cherry-picking, perhaps it is much cleaner to do
this kind of comparison in the caller of cherry-pick (i.e. rebase-i and
anything that runs cherry-pick)?
The whole series is titled as if "cherry-pick --ff" is the primary goal,
but I am puzzled why earlier patches in the series were necessary for this
change.
--
Is there a need to check commit->parents->next? Should this code work the same way if the commit being cherry-picked is a merge? Should "-m <parent-num>" option affect this codepath in any way? --
At least Daniel Barkalow and Paolo Bonzini have stated before that they would find natural that git cherry-pick itself can fast forward. The argument in the commit message is that as "git merge" can and does fast forward by default it seems strange that "git cherry-pick" cannot fast forward. It's not just because rebase-i tries to fast forward, as I think it Earlier patches are just refactoring and making the needed functions extern to Yeah I had not even checked what happens with merge commits and/or "-m <parent-num>". The next version of the patch series takes care of that. Thanks, Christian. --
I would still like to have a no-op --no-ff so that scripts that do rely on that can be future proofed (or also, scripts that do "git cherry-pick $blah -e COMMIT" could use --no-ff to avoid errors in case $blah contains --ff). Paolo --
Ok, I will add a --no-ff option but I think it should be incompatible with --ff rather than overide it. Thanks, Christian. --
Whatever it does it should be the same as git merge. Paolo --
Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- Documentation/git-cherry-pick.txt | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt index 78f4714..d71607a 100644 --- a/Documentation/git-cherry-pick.txt +++ b/Documentation/git-cherry-pick.txt @@ -7,7 +7,7 @@ git-cherry-pick - Apply the change introduced by an existing commit SYNOPSIS -------- -'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] <commit> +'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit> DESCRIPTION ----------- @@ -70,6 +70,10 @@ effect to your index in a row. --signoff:: Add Signed-off-by line at the end of the commit message. +--ff:: + If the current HEAD is the same as the parent of the + cherry-pick'ed commit, then a fast forward to this commit will + be performed. Author ------ -- 1.6.6.1.557.g77031 --
From: Stephan Beyer <s-beyer@gmx.net> The goal of this commit is to abstract out "git cherry-pick" and "git revert" functionnality into a new pick_commit() function made of code from "builtin-revert.c". The new pick_commit() function is in a new "pick.c" file with an associated "pick.h". This function starts from the current index (not HEAD), and allow the effect of one commit replayed (either forward or backward) to that state, leaving the result in the index. So it makes it possible to replay many commits to the index in sequence without commiting in between. This commit is made of code from the sequencer GSoC project: git://repo.or.cz/git/sbeyer.git (at commit 5a78908b70ceb5a4ea9fd4b82f07ceba1f019079) And it contains some changes and comments suggested by Junio. The original commit in the sequencer project that introduced this change is: 94a568a78d243d7a6c13778bc6b7ac1eb46e48cc Mentored-by: Daniel Barkalow <barkalow@iabervon.org> Mentored-by: Christian Couder <chriscool@tuxfamily.org> Signed-off-by: Stephan Beyer <s-beyer@gmx.net> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- Makefile | 2 + builtin-revert.c | 272 +++++++++--------------------------------------------- pick.c | 218 +++++++++++++++++++++++++++++++++++++++++++ pick.h | 13 +++ 4 files changed, 277 insertions(+), 228 deletions(-) create mode 100644 pick.c create mode 100644 pick.h diff --git a/Makefile b/Makefile index d624530..71901c8 100644 --- a/Makefile +++ b/Makefile @@ -458,6 +458,7 @@ LIB_H += pack-refs.h LIB_H += pack-revindex.h LIB_H += parse-options.h LIB_H += patch-ids.h +LIB_H += pick.h LIB_H += pkt-line.h LIB_H += progress.h LIB_H += quote.h @@ -550,6 +551,7 @@ LIB_OBJS += parse-options.o LIB_OBJS += patch-delta.o LIB_OBJS += patch-ids.o LIB_OBJS += path.o +LIB_OBJS += pick.o LIB_OBJS += pkt-line.o LIB_OBJS += preload-index.o LIB_OBJS += pretty.o diff --git a/builtin-revert.c ...
This simplifies rebase -i a little bit.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
git-rebase--interactive.sh | 15 +++------------
1 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index c2f6089..6b224a6 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -222,8 +222,8 @@ do_with_author () {
}
pick_one () {
- no_ff=
- case "$1" in -n) sha1=$2; no_ff=t ;; *) sha1=$1 ;; esac
+ ff=--ff
+ case "$1" in -n) sha1=$2; ff= ;; *) sha1=$1 ;; esac
output git rev-parse --verify $sha1 || die "Invalid commit name: $sha1"
test -d "$REWRITTEN" &&
pick_one_preserving_merges "$@" && return
@@ -232,16 +232,7 @@ pick_one () {
output git cherry-pick "$@"
return
fi
- parent_sha1=$(git rev-parse --verify $sha1^) ||
- die "Could not get the parent of $sha1"
- current_sha1=$(git rev-parse --verify HEAD)
- if test -z "$no_ff" && test "$current_sha1" = "$parent_sha1"
- then
- output git reset --hard $sha1
- output warn Fast-forward to $(git rev-parse --short $sha1)
- else
- output git cherry-pick "$@"
- fi
+ output git cherry-pick $ff "$@"
}
pick_one_preserving_merges () {
--
1.6.6.1.557.g77031
--
This simplifies "git merge" code and make it more efficient in some
cases.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-merge.c | 31 +++++++++++--------------------
1 files changed, 11 insertions(+), 20 deletions(-)
diff --git a/builtin-merge.c b/builtin-merge.c
index 3aaec7b..de2343a 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -25,6 +25,7 @@
#include "help.h"
#include "merge-recursive.h"
#include "resolve-undo.h"
+#include "reset.h"
#define DEFAULT_TWOHEAD (1<<0)
#define DEFAULT_OCTOPUS (1<<1)
@@ -231,24 +232,14 @@ static void save_state(void)
die("not a valid object: %s", buffer.buf);
}
-static void reset_hard(unsigned const char *sha1, int verbose)
+static void reset_hard(const char *prefix, unsigned const char *sha1, int verbose)
{
- int i = 0;
- const char *args[6];
-
- args[i++] = "read-tree";
- if (verbose)
- args[i++] = "-v";
- args[i++] = "--reset";
- args[i++] = "-u";
- args[i++] = sha1_to_hex(sha1);
- args[i] = NULL;
-
- if (run_command_v_opt(args, RUN_GIT_CMD))
- die("read-tree failed");
+ int res = reset(sha1_to_hex(sha1), prefix, HARD, !verbose, 0, 0, NULL);
+ if (res)
+ die("hard reset failed");
}
-static void restore_state(void)
+static void restore_state(const char *prefix)
{
struct strbuf sb = STRBUF_INIT;
const char *args[] = { "stash", "apply", NULL, NULL };
@@ -256,7 +247,7 @@ static void restore_state(void)
if (is_null_sha1(stash))
return;
- reset_hard(head, 1);
+ reset_hard(prefix, head, 1);
args[2] = sha1_to_hex(stash);
@@ -970,7 +961,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die("%s - not something we can merge", argv[0]);
update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
DIE_ON_ERR);
- reset_hard(remote_head->sha1, 0);
+ reset_hard(prefix, remote_head->sha1, 0);
return 0;
} else {
struct strbuf msg = STRBUF_INIT;
@@ -1167,7 +1158,7 @@ int cmd_merge(int argc, const char **argv, ...I vaguely recall somebody (perhaps it was you) tried to do something like this before to drive unpack_trees() inside the main process, broke the program rather badly, and then we ended up keeping read-tree invocation external to the process. Am I misremembering things? If not, could you describe how is this round different from the old one? --
I don't think it was me and I don't recall that, but I don't follow all the threads on the mailing list. I will search the archive, but any pointer would be very appreciated. Thanks anyway, --
This splits the reseting logic away from the argument parsing.
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-reset.c | 138 +++++++++++++++++++++++++++++-------------------------
1 files changed, 74 insertions(+), 64 deletions(-)
diff --git a/builtin-reset.c b/builtin-reset.c
index 3569695..bf97931 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -235,68 +235,13 @@ static int update_heads(unsigned char *sha1)
return update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
}
-int cmd_reset(int argc, const char **argv, const char *prefix)
+static int reset(const char *rev, const char *prefix,
+ int reset_type, int quiet, int patch_mode,
+ int argc, const char **argv)
{
- int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
- int patch_mode = 0;
- const char *rev = "HEAD";
- unsigned char sha1[20];
struct commit *commit;
- char *reflog_action;
- const struct option options[] = {
- OPT__QUIET(&quiet),
- OPT_SET_INT(0, "mixed", &reset_type,
- "reset HEAD and index", MIXED),
- OPT_SET_INT(0, "soft", &reset_type, "reset only HEAD", SOFT),
- OPT_SET_INT(0, "hard", &reset_type,
- "reset HEAD, index and working tree", HARD),
- OPT_SET_INT(0, "merge", &reset_type,
- "reset HEAD, index and working tree", MERGE),
- OPT_BOOLEAN('p', "patch", &patch_mode, "select hunks interactively"),
- OPT_END()
- };
-
- git_config(git_default_config, NULL);
-
- argc = parse_options(argc, argv, prefix, options, git_reset_usage,
- PARSE_OPT_KEEP_DASHDASH);
- reflog_action = args_to_str(argv);
- setenv("GIT_REFLOG_ACTION", reflog_action, 0);
-
- /*
- * Possible arguments are:
- *
- * git reset [-opts] <rev> <paths>...
- * git reset [-opts] <rev> -- <paths>...
- * git reset [-opts] -- <paths>...
- * git reset [-opts] <paths>...
- *
- * At this point, argv[i] points immediately after [-opts].
- */
-
- if (i < argc) {
- if (!strcmp(argv[i], "--")) {
- i++; /* reset to HEAD, possibly with paths ...Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-reset.c | 43 +++++++++++++++++++++++++++----------------
1 files changed, 27 insertions(+), 16 deletions(-)
diff --git a/builtin-reset.c b/builtin-reset.c
index 0f5022e..3569695 100644
--- a/builtin-reset.c
+++ b/builtin-reset.c
@@ -211,15 +211,38 @@ static void prepend_reflog_action(const char *action, char *buf, size_t size)
warning("Reflog action message too long: %.*s...", 50, buf);
}
+/*
+ * Any resets update HEAD to the head being switched to,
+ * saving the previous head in ORIG_HEAD before.
+ */
+static int update_heads(unsigned char *sha1)
+{
+ unsigned char sha1_orig[20], *orig = NULL,
+ sha1_old_orig[20], *old_orig = NULL;
+ char msg[1024];
+
+ if (!get_sha1("ORIG_HEAD", sha1_old_orig))
+ old_orig = sha1_old_orig;
+ if (!get_sha1("HEAD", sha1_orig)) {
+ orig = sha1_orig;
+ prepend_reflog_action("updating ORIG_HEAD", msg, sizeof(msg));
+ update_ref(msg, "ORIG_HEAD", orig, old_orig, 0, MSG_ON_ERR);
+ }
+ else if (old_orig)
+ delete_ref("ORIG_HEAD", old_orig, 0);
+ prepend_reflog_action("updating HEAD", msg, sizeof(msg));
+
+ return update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+}
+
int cmd_reset(int argc, const char **argv, const char *prefix)
{
int i = 0, reset_type = NONE, update_ref_status = 0, quiet = 0;
int patch_mode = 0;
const char *rev = "HEAD";
- unsigned char sha1[20], *orig = NULL, sha1_orig[20],
- *old_orig = NULL, sha1_old_orig[20];
+ unsigned char sha1[20];
struct commit *commit;
- char *reflog_action, msg[1024];
+ char *reflog_action;
const struct option options[] = {
OPT__QUIET(&quiet),
OPT_SET_INT(0, "mixed", &reset_type,
@@ -321,19 +344,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
else if (reset_index_file(sha1, reset_type, quiet))
die("Could not reset index file to revision '%s'.", rev);
- /* Any resets update HEAD to the head being switched to,
- * saving the previous head in ...Could you please briefly describe what good does "adding -ff option to cherry-pick" do in the first place? IOW, help reviewers to convince themselves that it is worth allocating a block of time to read through a 9 patch series. That is what the cover letter is for. Thanks. --
Ok I will do that. Thanks, Christian. --
