* [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality
2010-02-01 7:55 [RFC/PATCH 0/6] add --ff option to cherry-pick Christian Couder
@ 2010-02-01 7:55 ` Christian Couder
2010-02-01 10:26 ` Johannes Schindelin
2010-02-03 16:40 ` Stephen Boyd
2010-02-01 7:55 ` [RFC/PATCH 2/6] reset: refactor updating heads into a static function Christian Couder
` (4 subsequent siblings)
5 siblings, 2 replies; 14+ messages in thread
From: Christian Couder @ 2010-02-01 7:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow
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 b/builtin-revert.c
index 8ac86f0..3cbeab7 100644
--- a/builtin-revert.c
+++ b/builtin-revert.c
@@ -1,18 +1,14 @@
#include "cache.h"
#include "builtin.h"
-#include "object.h"
#include "commit.h"
#include "tag.h"
-#include "wt-status.h"
-#include "run-command.h"
#include "exec_cmd.h"
#include "utf8.h"
#include "parse-options.h"
-#include "cache-tree.h"
#include "diff.h"
#include "revision.h"
#include "rerere.h"
-#include "merge-recursive.h"
+#include "pick.h"
/*
* This implements the builtins revert and cherry-pick.
@@ -35,26 +31,24 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-static int edit, no_replay, no_commit, mainline, signoff;
-static enum { REVERT, CHERRY_PICK } action;
+static int edit, no_commit, mainline, signoff;
+static int flags;
static struct commit *commit;
static int allow_rerere_auto;
-static const char *me;
-
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
static void parse_args(int argc, const char **argv)
{
const char * const * usage_str =
- action == REVERT ? revert_usage : cherry_pick_usage;
+ flags & PICK_REVERSE ? revert_usage : cherry_pick_usage;
unsigned char sha1[20];
const char *arg;
int noop;
struct option options[] = {
OPT_BOOLEAN('n', "no-commit", &no_commit, "don't automatically commit"),
OPT_BOOLEAN('e', "edit", &edit, "edit the commit message"),
- OPT_BOOLEAN('x', NULL, &no_replay, "append commit name when cherry-picking"),
+ 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_INTEGER('m', "mainline", &mainline, "parent number"),
@@ -79,42 +73,12 @@ static void parse_args(int argc, const char **argv)
die ("'%s' does not point to a commit", arg);
}
-static char *get_oneline(const char *message)
-{
- char *result;
- const char *p = message, *abbrev, *eol;
- int abbrev_len, oneline_len;
-
- if (!p)
- die ("Could not read commit message of %s",
- sha1_to_hex(commit->object.sha1));
- while (*p && (*p != '\n' || p[1] != '\n'))
- p++;
-
- if (*p) {
- p += 2;
- for (eol = p + 1; *eol && *eol != '\n'; eol++)
- ; /* do nothing */
- } else
- eol = p;
- abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
- abbrev_len = strlen(abbrev);
- oneline_len = eol - p;
- result = xmalloc(abbrev_len + 5 + oneline_len);
- memcpy(result, abbrev, abbrev_len);
- memcpy(result + abbrev_len, "... ", 4);
- memcpy(result + abbrev_len + 4, p, oneline_len);
- result[abbrev_len + 4 + oneline_len] = '\0';
- return result;
-}
-
static char *get_encoding(const char *message)
{
const char *p = message, *eol;
if (!p)
- die ("Could not read commit message of %s",
- sha1_to_hex(commit->object.sha1));
+ return NULL;
while (*p && *p != '\n') {
for (eol = p + 1; *eol && *eol != '\n'; eol++)
; /* do nothing */
@@ -130,30 +94,6 @@ static char *get_encoding(const char *message)
return NULL;
}
-static struct lock_file msg_file;
-static int msg_fd;
-
-static void add_to_msg(const char *string)
-{
- int len = strlen(string);
- if (write_in_full(msg_fd, string, len) < 0)
- die_errno ("Could not write to MERGE_MSG");
-}
-
-static void add_message_to_msg(const char *message)
-{
- const char *p = message;
- while (*p && (*p != '\n' || p[1] != '\n'))
- p++;
-
- if (!*p)
- add_to_msg(sha1_to_hex(commit->object.sha1));
-
- p += 2;
- add_to_msg(p);
- return;
-}
-
static void set_author_ident_env(const char *message)
{
const char *p = message;
@@ -216,7 +156,7 @@ static char *help_msg(const unsigned char *sha1)
"mark the corrected paths with 'git add <paths>' "
"or 'git rm <paths>' and commit the result.");
- if (action == CHERRY_PICK) {
+ if (!(flags & PICK_REVERSE)) {
sprintf(helpbuf + strlen(helpbuf),
"\nWhen commiting, use the option "
"'-c %s' to retain authorship and message.",
@@ -225,14 +165,17 @@ static char *help_msg(const unsigned char *sha1)
return helpbuf;
}
-static struct tree *empty_tree(void)
+static void write_message(struct strbuf *msgbuf, const char *filename)
{
- struct tree *tree = xcalloc(1, sizeof(struct tree));
-
- tree->object.parsed = 1;
- tree->object.type = OBJ_TREE;
- pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
- return tree;
+ struct lock_file msg_file;
+ int msg_fd;
+ msg_fd = hold_lock_file_for_update(&msg_file, filename,
+ LOCK_DIE_ON_ERROR);
+ if (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)
+ die_errno("Could not write to %s.", filename);
+ strbuf_release(msgbuf);
+ if (commit_lock_file(&msg_file) < 0)
+ die("Error wrapping up %s", filename);
}
static NORETURN void die_dirty_index(const char *me)
@@ -250,175 +193,53 @@ static NORETURN void die_dirty_index(const char *me)
static int revert_or_cherry_pick(int argc, const char **argv)
{
- unsigned char head[20];
- struct commit *base, *next, *parent;
- int i, index_fd, clean;
- char *oneline, *reencoded_message = NULL;
- const char *message, *encoding;
- char *defmsg = git_pathdup("MERGE_MSG");
- struct merge_options o;
- struct tree *result, *next_tree, *base_tree, *head_tree;
- static struct lock_file index_lock;
+ const char *me;
+ struct strbuf msgbuf;
+ char *reencoded_message = NULL;
+ const char *encoding;
+ int failed;
git_config(git_default_config, NULL);
- me = action == REVERT ? "revert" : "cherry-pick";
+ me = flags & PICK_REVERSE ? "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 (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
- * merge the differences in, so let's compute the tree
- * that represents the "current" state for merge-recursive
- * to work on.
- */
- if (write_cache_as_tree(head, 0, NULL))
- die ("Your index file is unmerged.");
- } else {
- if (get_sha1("HEAD", head))
- die ("You do not have a valid HEAD");
- if (index_differs_from("HEAD", 0))
- die_dirty_index(me);
- }
- discard_cache();
-
- index_fd = hold_locked_index(&index_lock, 1);
+ if (!no_commit && index_differs_from("HEAD", 0))
+ die_dirty_index(me);
- if (!commit->parents) {
- if (action == REVERT)
- die ("Cannot revert a root commit");
- parent = NULL;
- }
- else if (commit->parents->next) {
- /* Reverting or cherry-picking a merge commit */
- int cnt;
- struct commit_list *p;
-
- if (!mainline)
- die("Commit %s is a merge but no -m option was given.",
- sha1_to_hex(commit->object.sha1));
-
- for (cnt = 1, p = commit->parents;
- cnt != mainline && p;
- cnt++)
- p = p->next;
- if (cnt != mainline || !p)
- die("Commit %s does not have parent %d",
- sha1_to_hex(commit->object.sha1), mainline);
- parent = p->item;
- } else if (0 < mainline)
- die("Mainline was specified but commit %s is not a merge.",
- sha1_to_hex(commit->object.sha1));
- else
- parent = commit->parents->item;
-
- if (!(message = commit->buffer))
- die ("Cannot get commit message for %s",
+ if (!commit->buffer)
+ return error("Cannot get commit message for %s",
sha1_to_hex(commit->object.sha1));
-
- if (parent && parse_commit(parent) < 0)
- die("%s: cannot parse parent commit %s",
- me, sha1_to_hex(parent->object.sha1));
-
- /*
- * "commit" is an existing commit. We would want to apply
- * the difference it introduces since its first parent "prev"
- * on top of the current HEAD if we are cherry-pick. Or the
- * reverse of it if we are revert.
- */
-
- msg_fd = hold_lock_file_for_update(&msg_file, defmsg,
- LOCK_DIE_ON_ERROR);
-
- encoding = get_encoding(message);
+ encoding = get_encoding(commit->buffer);
if (!encoding)
encoding = "UTF-8";
if (!git_commit_encoding)
git_commit_encoding = "UTF-8";
- if ((reencoded_message = reencode_string(message,
+ if ((reencoded_message = reencode_string(commit->buffer,
git_commit_encoding, encoding)))
- message = reencoded_message;
-
- oneline = get_oneline(message);
-
- if (action == REVERT) {
- char *oneline_body = strchr(oneline, ' ');
-
- base = commit;
- next = parent;
- add_to_msg("Revert \"");
- add_to_msg(oneline_body + 1);
- add_to_msg("\"\n\nThis reverts commit ");
- add_to_msg(sha1_to_hex(commit->object.sha1));
-
- if (commit->parents->next) {
- add_to_msg(", reversing\nchanges made to ");
- add_to_msg(sha1_to_hex(parent->object.sha1));
- }
- add_to_msg(".\n");
- } else {
- base = parent;
- next = commit;
- set_author_ident_env(message);
- add_message_to_msg(message);
- if (no_replay) {
- add_to_msg("(cherry picked from commit ");
- add_to_msg(sha1_to_hex(commit->object.sha1));
- add_to_msg(")\n");
- }
- }
-
- read_cache();
- init_merge_options(&o);
- o.branch1 = "HEAD";
- o.branch2 = oneline;
+ commit->buffer = reencoded_message;
- head_tree = parse_tree_indirect(head);
- next_tree = next ? next->tree : empty_tree();
- base_tree = base ? base->tree : empty_tree();
-
- clean = merge_trees(&o,
- head_tree,
- next_tree, base_tree, &result);
-
- if (active_cache_changed &&
- (write_cache(index_fd, active_cache, active_nr) ||
- commit_locked_index(&index_lock)))
- die("%s: Unable to write new index file", me);
- rollback_lock_file(&index_lock);
-
- if (!clean) {
- add_to_msg("\nConflicts:\n\n");
- for (i = 0; i < active_nr;) {
- struct cache_entry *ce = active_cache[i++];
- if (ce_stage(ce)) {
- add_to_msg("\t");
- add_to_msg(ce->name);
- add_to_msg("\n");
- while (i < active_nr && !strcmp(ce->name,
- active_cache[i]->name))
- i++;
- }
- }
- if (commit_lock_file(&msg_file) < 0)
- die ("Error wrapping up %s", defmsg);
+ failed = pick_commit(commit, mainline, flags, &msgbuf);
+ if (failed < 0) {
+ exit(1);
+ } else if (failed > 0) {
fprintf(stderr, "Automatic %s failed.%s\n",
me, help_msg(commit->object.sha1));
+ write_message(&msgbuf, git_path("MERGE_MSG"));
rerere(allow_rerere_auto);
exit(1);
}
- if (commit_lock_file(&msg_file) < 0)
- die ("Error wrapping up %s", defmsg);
+ if (!(flags & PICK_REVERSE))
+ set_author_ident_env(commit->buffer);
+ free(reencoded_message);
+
fprintf(stderr, "Finished one %s.\n", me);
+ write_message(&msgbuf, git_path("MERGE_MSG"));
+
/*
- *
* If we are cherry-pick, and if the merge did not result in
* hand-editing, we will hit this commit and inherit the original
* author date and name.
@@ -436,14 +257,11 @@ static int revert_or_cherry_pick(int argc, const char **argv)
args[i++] = "-s";
if (!edit) {
args[i++] = "-F";
- args[i++] = defmsg;
+ args[i++] = git_path("MERGE_MSG");
}
args[i] = NULL;
return execv_git_cmd(args);
}
- free(reencoded_message);
- free(defmsg);
-
return 0;
}
@@ -451,14 +269,12 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
{
if (isatty(0))
edit = 1;
- no_replay = 1;
- action = REVERT;
+ flags = PICK_REVERSE | PICK_ADD_NOTE;
return revert_or_cherry_pick(argc, argv);
}
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
{
- no_replay = 0;
- action = CHERRY_PICK;
+ flags = 0;
return revert_or_cherry_pick(argc, argv);
}
diff --git a/pick.c b/pick.c
new file mode 100644
index 0000000..bb04c68
--- /dev/null
+++ b/pick.c
@@ -0,0 +1,218 @@
+#include "cache.h"
+#include "commit.h"
+#include "run-command.h"
+#include "cache-tree.h"
+#include "pick.h"
+#include "merge-recursive.h"
+
+static struct commit *commit;
+
+static char *get_oneline(const char *message)
+{
+ char *result;
+ const char *p = message, *abbrev, *eol;
+ int abbrev_len, oneline_len;
+
+ if (!p)
+ return NULL;
+ while (*p && (*p != '\n' || p[1] != '\n'))
+ p++;
+
+ if (*p) {
+ p += 2;
+ for (eol = p + 1; *eol && *eol != '\n'; eol++)
+ ; /* do nothing */
+ } else
+ eol = p;
+ abbrev = find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV);
+ abbrev_len = strlen(abbrev);
+ oneline_len = eol - p;
+ result = xmalloc(abbrev_len + 5 + oneline_len);
+ memcpy(result, abbrev, abbrev_len);
+ memcpy(result + abbrev_len, "... ", 4);
+ memcpy(result + abbrev_len + 4, p, oneline_len);
+ result[abbrev_len + 4 + oneline_len] = '\0';
+ return result;
+}
+
+static void add_message_to_msg(struct strbuf *msg, const char *message)
+{
+ const char *p = message;
+ while (*p && (*p != '\n' || p[1] != '\n'))
+ p++;
+
+ if (!*p)
+ strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
+
+ p += 2;
+ strbuf_addstr(msg, p);
+ return;
+}
+
+static struct tree *empty_tree(void)
+{
+ struct tree *tree = xcalloc(1, sizeof(struct tree));
+
+ tree->object.parsed = 1;
+ tree->object.type = OBJ_TREE;
+ pretend_sha1_file(NULL, 0, OBJ_TREE, tree->object.sha1);
+ return tree;
+}
+
+/*
+ * Pick changes introduced by "commit" argument into current working
+ * tree and index.
+ *
+ * It 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.
+ *
+ * You do not have to start from a commit, so you can replay many commits
+ * to the index in sequence without commiting in between to squash multiple
+ * steps if you wanted to.
+ *
+ * Return 0 on success.
+ * Return negative value on error before picking,
+ * and a positive value after picking,
+ * and return 1 if and only if a conflict occurs but no other error.
+ */
+int pick_commit(struct commit *pick_commit, int mainline, int flags,
+ struct strbuf *msg)
+{
+ unsigned char head[20];
+ struct commit *base, *next, *parent;
+ int i, index_fd, clean;
+ int ret = 0;
+ char *oneline;
+ const char *message;
+ struct merge_options o;
+ struct tree *result, *next_tree, *base_tree, *head_tree;
+ static struct lock_file index_lock;
+
+ strbuf_init(msg, 0);
+ commit = pick_commit;
+
+ /*
+ * Let's compute the tree that represents the "current" state
+ * for merge-recursive to work on.
+ */
+ if (write_cache_as_tree(head, 0, NULL))
+ return error("Your index file is unmerged.");
+ discard_cache();
+
+ index_fd = hold_locked_index(&index_lock, 0);
+ if (index_fd < 0)
+ return error("Unable to create locked index: %s",
+ strerror(errno));
+
+ if (!commit->parents) {
+ if (flags & PICK_REVERSE)
+ return error("Cannot revert a root commit");
+ parent = NULL;
+ }
+ else if (commit->parents->next) {
+ /* Reverting or cherry-picking a merge commit */
+ int cnt;
+ struct commit_list *p;
+
+ if (!mainline)
+ return error("Commit %s is a merge but no mainline was given.",
+ sha1_to_hex(commit->object.sha1));
+
+ for (cnt = 1, p = commit->parents;
+ cnt != mainline && p;
+ cnt++)
+ p = p->next;
+ if (cnt != mainline || !p)
+ return error("Commit %s does not have parent %d",
+ sha1_to_hex(commit->object.sha1),
+ mainline);
+ parent = p->item;
+ } else if (0 < mainline)
+ return error("Mainline was specified but commit %s is not a merge.",
+ sha1_to_hex(commit->object.sha1));
+ else
+ parent = commit->parents->item;
+
+ if (!(message = commit->buffer))
+ return error("Cannot get commit message for %s",
+ sha1_to_hex(commit->object.sha1));
+
+ if (parent && parse_commit(parent) < 0)
+ return error("Cannot parse parent commit %s",
+ sha1_to_hex(parent->object.sha1));
+
+ oneline = get_oneline(message);
+
+ if (flags & PICK_REVERSE) {
+ char *oneline_body = strchr(oneline, ' ');
+
+ base = commit;
+ next = parent;
+ strbuf_addstr(msg, "Revert \"");
+ strbuf_addstr(msg, oneline_body + 1);
+ strbuf_addstr(msg, "\"\n\nThis reverts commit ");
+ strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
+
+ if (commit->parents->next) {
+ strbuf_addstr(msg, ", reversing\nchanges made to ");
+ strbuf_addstr(msg, sha1_to_hex(parent->object.sha1));
+ }
+ strbuf_addstr(msg, ".\n");
+ } else {
+ base = parent;
+ next = commit;
+ add_message_to_msg(msg, message);
+ if (flags & PICK_ADD_NOTE) {
+ strbuf_addstr(msg, "(cherry picked from commit ");
+ strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
+ strbuf_addstr(msg, ")\n");
+ }
+ }
+
+ read_cache();
+ init_merge_options(&o);
+ o.branch1 = "HEAD";
+ o.branch2 = oneline;
+
+ head_tree = parse_tree_indirect(head);
+ next_tree = next ? next->tree : empty_tree();
+ base_tree = base ? base->tree : empty_tree();
+
+ clean = merge_trees(&o,
+ head_tree,
+ next_tree, base_tree, &result);
+
+ if (active_cache_changed &&
+ (write_cache(index_fd, active_cache, active_nr) ||
+ commit_locked_index(&index_lock))) {
+ error("Unable to write new index file");
+ return 2;
+ }
+ rollback_lock_file(&index_lock);
+
+ if (!clean) {
+ strbuf_addstr(msg, "\nConflicts:\n\n");
+ for (i = 0; i < active_nr;) {
+ struct cache_entry *ce = active_cache[i++];
+ if (ce_stage(ce)) {
+ strbuf_addstr(msg, "\t");
+ strbuf_addstr(msg, ce->name);
+ strbuf_addstr(msg, "\n");
+ while (i < active_nr && !strcmp(ce->name,
+ active_cache[i]->name))
+ i++;
+ }
+ }
+ ret = 1;
+ }
+ free(oneline);
+
+ discard_cache();
+ if (read_cache() < 0) {
+ error("Cannot read the index");
+ return 2;
+ }
+
+ return ret;
+}
diff --git a/pick.h b/pick.h
new file mode 100644
index 0000000..7a74ad8
--- /dev/null
+++ b/pick.h
@@ -0,0 +1,13 @@
+#ifndef PICK_H
+#define PICK_H
+
+#include "commit.h"
+
+/* Pick flags: */
+#define PICK_REVERSE 1 /* pick the reverse changes ("revert") */
+#define PICK_ADD_NOTE 2 /* add note about original commit (unless conflict) */
+/* We don't need a PICK_QUIET. This is done by
+ * setenv("GIT_MERGE_VERBOSITY", "0", 1); */
+extern int pick_commit(struct commit *commit, int mainline, int flags, struct strbuf *msg);
+
+#endif
--
1.6.6.1.557.g77031
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality
2010-02-01 7:55 ` [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality Christian Couder
@ 2010-02-01 10:26 ` Johannes Schindelin
2010-02-03 16:40 ` Stephen Boyd
1 sibling, 0 replies; 14+ messages in thread
From: Johannes Schindelin @ 2010-02-01 10:26 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Linus Torvalds, Stephan Beyer,
Daniel Barkalow
Hi,
On Mon, 1 Feb 2010, Christian Couder wrote:
> 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>
> ---
I guess that this patch would look nicer using --patience. In its current
form, it would put too big a dent in my Git time budget, sorry, I will not
review it.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality
2010-02-01 7:55 ` [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality Christian Couder
2010-02-01 10:26 ` Johannes Schindelin
@ 2010-02-03 16:40 ` Stephen Boyd
1 sibling, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2010-02-03 16:40 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Linus Torvalds, Johannes Schindelin,
Stephan Beyer, Daniel Barkalow
On 01/31/2010 11:55 PM, Christian Couder wrote:
> + if (flags& PICK_REVERSE) {
> + char *oneline_body = strchr(oneline, ' ');
> +
> + base = commit;
> + next = parent;
> + strbuf_addstr(msg, "Revert \"");
> + strbuf_addstr(msg, oneline_body + 1);
>
Why not do the oneline_body + 1 during the strchr()? Seems like
oneline_body is pointing to before the actual string we want.
> + for (i = 0; i< active_nr;) {
> + struct cache_entry *ce = active_cache[i++];
> + if (ce_stage(ce)) {
> + strbuf_addstr(msg, "\t");
> + strbuf_addstr(msg, ce->name);
> + strbuf_addstr(msg, "\n");
>
use strbuf_addch() for characters.
--->8----
diff --git a/pick.c b/pick.c
index bb04c68..1e1628a 100644
--- a/pick.c
+++ b/pick.c
@@ -145,12 +145,12 @@ int pick_commit(struct commit *pick_commit, int mainline,
oneline = get_oneline(message);
if (flags& PICK_REVERSE) {
- char *oneline_body = strchr(oneline, ' ');
+ char *oneline_body = strchr(oneline, ' ') + 1;
base = commit;
next = parent;
strbuf_addstr(msg, "Revert \"");
- strbuf_addstr(msg, oneline_body + 1);
+ strbuf_addstr(msg, oneline_body);
strbuf_addstr(msg, "\"\n\nThis reverts commit ");
strbuf_addstr(msg, sha1_to_hex(commit->object.sha1));
@@ -196,9 +196,9 @@ int pick_commit(struct commit *pick_commit, int mainline, in
for (i = 0; i< active_nr;) {
struct cache_entry *ce = active_cache[i++];
if (ce_stage(ce)) {
- strbuf_addstr(msg, "\t");
+ strbuf_addch(msg, '\t');
strbuf_addstr(msg, ce->name);
- strbuf_addstr(msg, "\n");
+ strbuf_addch(msg, '\n');
while (i< active_nr&& !strcmp(ce->name,
active_cache[i]->name))
i++;
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [RFC/PATCH 2/6] reset: refactor updating heads into a static function
2010-02-01 7:55 [RFC/PATCH 0/6] add --ff option to cherry-pick Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality Christian Couder
@ 2010-02-01 7:55 ` Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 3/6] reset: refactor reseting in its own function Christian Couder
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Christian Couder @ 2010-02-01 7:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow
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 ORIG_HEAD before. */
- 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));
- update_ref_status = update_ref(msg, "HEAD", sha1, orig, 0, MSG_ON_ERR);
+ update_ref_status = update_heads(sha1);
switch (reset_type) {
case HARD:
--
1.6.6.1.557.g77031
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC/PATCH 3/6] reset: refactor reseting in its own function
2010-02-01 7:55 [RFC/PATCH 0/6] add --ff option to cherry-pick Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 1/6] revert: libify cherry-pick and revert functionnality Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 2/6] reset: refactor updating heads into a static function Christian Couder
@ 2010-02-01 7:55 ` Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 4/6] reset: make reset function non static and declare it in "reset.h" Christian Couder
` (2 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Christian Couder @ 2010-02-01 7:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow
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 */
- } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) {
- rev = argv[i];
- i += 2;
- }
- /*
- * Otherwise, argv[i] could be either <rev> or <paths> and
- * has to be unambiguous.
- */
- else if (!get_sha1(argv[i], sha1)) {
- /*
- * Ok, argv[i] looks like a rev; it should not
- * be a filename.
- */
- verify_non_filename(prefix, argv[i]);
- rev = argv[i++];
- } else {
- /* Otherwise we treat this as a filename */
- verify_filename(prefix, argv[i]);
- }
- }
+ unsigned char sha1[20];
+ int update_ref_status;
if (get_sha1(rev, sha1))
die("Failed to resolve '%s' as a valid ref.", rev);
@@ -309,19 +254,19 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
if (patch_mode) {
if (reset_type != NONE)
die("--patch is incompatible with --{hard,mixed,soft}");
- return interactive_reset(rev, argv + i, prefix);
+ return interactive_reset(rev, argv, prefix);
}
/* git reset tree [--] paths... can be used to
* load chosen paths from the tree into the index without
* affecting the working tree nor HEAD. */
- if (i < argc) {
+ if (argc > 0) {
if (reset_type == MIXED)
warning("--mixed option is deprecated with paths.");
else if (reset_type != NONE)
die("Cannot do %s reset with paths.",
reset_type_names[reset_type]);
- return read_from_tree(prefix, argv + i, sha1,
+ return read_from_tree(prefix, argv, sha1,
quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN);
}
if (reset_type == NONE)
@@ -361,7 +306,72 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
remove_branch_state();
+ return update_ref_status;
+}
+
+int cmd_reset(int argc, const char **argv, const char *prefix)
+{
+ int i = 0, reset_type = NONE, quiet = 0;
+ int patch_mode = 0;
+ const char *rev = "HEAD";
+ unsigned char sha1[20];
+ 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);
free(reflog_action);
- return update_ref_status;
+ /*
+ * 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 */
+ } else if (i + 1 < argc && !strcmp(argv[i+1], "--")) {
+ rev = argv[i];
+ i += 2;
+ }
+ /*
+ * Otherwise, argv[i] could be either <rev> or <paths> and
+ * has to be unambiguous.
+ */
+ else if (!get_sha1(argv[i], sha1)) {
+ /*
+ * Ok, argv[i] looks like a rev; it should not
+ * be a filename.
+ */
+ verify_non_filename(prefix, argv[i]);
+ rev = argv[i++];
+ } else {
+ /* Otherwise we treat this as a filename */
+ verify_filename(prefix, argv[i]);
+ }
+ }
+
+ return reset(rev, prefix, reset_type, quiet, patch_mode,
+ argc - i, argv + i);
}
--
1.6.6.1.557.g77031
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC/PATCH 4/6] reset: make reset function non static and declare it in "reset.h"
2010-02-01 7:55 [RFC/PATCH 0/6] add --ff option to cherry-pick Christian Couder
` (2 preceding siblings ...)
2010-02-01 7:55 ` [RFC/PATCH 3/6] reset: refactor reseting in its own function Christian Couder
@ 2010-02-01 7:55 ` Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking Christian Couder
2010-02-01 7:55 ` [RFC/PATCH 6/6] rebase -i: use new --ff cherry-pick option Christian Couder
5 siblings, 0 replies; 14+ messages in thread
From: Christian Couder @ 2010-02-01 7:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
2010-02-01 7:55 [RFC/PATCH 0/6] add --ff option to cherry-pick Christian Couder
` (3 preceding siblings ...)
2010-02-01 7:55 ` [RFC/PATCH 4/6] reset: make reset function non static and declare it in "reset.h" Christian Couder
@ 2010-02-01 7:55 ` Christian Couder
2010-02-01 11:10 ` Paolo Bonzini
2010-02-01 7:55 ` [RFC/PATCH 6/6] rebase -i: use new --ff cherry-pick option Christian Couder
5 siblings, 1 reply; 14+ messages in thread
From: Christian Couder @ 2010-02-01 7:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
builtin-revert.c | 25 +++++++++++++++++++++----
1 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/builtin-revert.c b/builtin-revert.c
index 3cbeab7..3f92515 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,7 +32,7 @@ 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;
@@ -51,6 +52,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 +73,8 @@ static void parse_args(int argc, const char **argv)
}
if (commit->object.type != OBJ_COMMIT)
die ("'%s' does not point to a commit", arg);
+ if (ff_ok && no_commit)
+ die ("options --ff and --no-commit are incompatible");
}
static char *get_encoding(const char *message)
@@ -191,7 +195,7 @@ static NORETURN void die_dirty_index(const char *me)
}
}
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv, const char *prefix)
{
const char *me;
struct strbuf msgbuf;
@@ -209,6 +213,19 @@ static int revert_or_cherry_pick(int argc, const char **argv)
if (!no_commit && index_differs_from("HEAD", 0))
die_dirty_index(me);
+ if (!(flags & PICK_REVERSE) && ff_ok && commit->parents) {
+ unsigned char head_sha1[20];
+ if (get_sha1("HEAD", head_sha1))
+ die("You do not have a valid HEAD.");
+ if (!hashcmp(commit->parents->item->object.sha1, head_sha1)) {
+ char *hex = sha1_to_hex(commit->object.sha1);
+ int res = reset(hex, prefix, HARD, 0, 0, 0, NULL);
+ if (!res)
+ fprintf(stderr, "Fast-forward to %s\n", hex);
+ return res;
+ }
+ }
+
if (!commit->buffer)
return error("Cannot get commit message for %s",
sha1_to_hex(commit->object.sha1));
@@ -270,11 +287,11 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
if (isatty(0))
edit = 1;
flags = PICK_REVERSE | PICK_ADD_NOTE;
- return revert_or_cherry_pick(argc, argv);
+ return revert_or_cherry_pick(argc, argv, prefix);
}
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
{
flags = 0;
- return revert_or_cherry_pick(argc, argv);
+ return revert_or_cherry_pick(argc, argv, prefix);
}
--
1.6.6.1.557.g77031
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
2010-02-01 7:55 ` [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking Christian Couder
@ 2010-02-01 11:10 ` Paolo Bonzini
2010-02-01 12:43 ` Christian Couder
0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2010-02-01 11:10 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, git, Linus Torvalds, Johannes Schindelin,
Stephan Beyer, Daniel Barkalow
> + OPT_BOOLEAN(0, "ff",&ff_ok, "allow fast forward"),
Why should this not be the default? Instead, you'd add --no-ff. This
would simplify 6/6 further, like
eval sha1=\$$#
...
output git cherry-pick "$@"
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
2010-02-01 11:10 ` Paolo Bonzini
@ 2010-02-01 12:43 ` Christian Couder
2010-02-01 14:29 ` Paolo Bonzini
0 siblings, 1 reply; 14+ messages in thread
From: Christian Couder @ 2010-02-01 12:43 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Christian Couder, Junio C Hamano, git, Linus Torvalds,
Johannes Schindelin, Stephan Beyer, Daniel Barkalow
On Mon, Feb 1, 2010 at 12:10 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
>> + OPT_BOOLEAN(0, "ff",&ff_ok, "allow fast forward"),
>
> Why should this not be the default?
Maybe it could be the default, but in this case it should be made
compatible with -n option
(and perhaps other options) for backward compatibility, and this would
probably need more
involved changes.
Thanks,
Christian.
> Instead, you'd add --no-ff. This would
> simplify 6/6 further, like
>
> eval sha1=\$$#
> ...
> output git cherry-pick "$@"
>
> Paolo
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
2010-02-01 12:43 ` Christian Couder
@ 2010-02-01 14:29 ` Paolo Bonzini
2010-02-02 5:13 ` Christian Couder
0 siblings, 1 reply; 14+ messages in thread
From: Paolo Bonzini @ 2010-02-01 14:29 UTC (permalink / raw)
To: Christian Couder
Cc: Christian Couder, Junio C Hamano, git, Linus Torvalds,
Johannes Schindelin, Stephan Beyer, Daniel Barkalow
On 02/01/2010 01:43 PM, Christian Couder wrote:
> Maybe it could be the default, but in this case it should be made
> compatible with -n option (and perhaps other options) for backward
> compatibility, and this would probably need more involved changes.
A better objection is that GIT_COMMITTER_* is respected by |git
cherry-pick" but not by "git cherry-pick --ff", and that even without
setting the variables, "git cherry-pick" will pick a new commit date but
"git cherry-pick --ff" wouldn't. The latter, I think is the only
difference that is worth pondering further.
My impression is that a user would never have any problem with
fast-forwarding. For scripts probably the same is true (the typical
scenario for scripts is probably very similar to what "git rebase -i"
does), but it can still be a potential backwards-incompatibility in case
the script is *expecting* cherry-picking to generate a new SHA1. How
broken can we consider this expectation?
That said, to reply to your question, of course -n would disable it, and
so would -x, -s and possibly -e. But then, the same applies to --ff:
your patch forbids "-n --ff", but -x, -s and -e would need the same
treatment.
Note that "-e --ff" would error out; however if --ff would be the
default, "-e" would probably choose between fast-forward and
non-fast-forward depending on whether the commit message was edited.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
2010-02-01 14:29 ` Paolo Bonzini
@ 2010-02-02 5:13 ` Christian Couder
2010-02-02 7:56 ` Paolo Bonzini
0 siblings, 1 reply; 14+ messages in thread
From: Christian Couder @ 2010-02-02 5:13 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Christian Couder, Junio C Hamano, git, Linus Torvalds,
Johannes Schindelin, Stephan Beyer, Daniel Barkalow
On lundi 01 février 2010, Paolo Bonzini wrote:
> On 02/01/2010 01:43 PM, Christian Couder wrote:
> > Maybe it could be the default, but in this case it should be made
> > compatible with -n option (and perhaps other options) for backward
> > compatibility, and this would probably need more involved changes.
>
> A better objection is that GIT_COMMITTER_* is respected by |git
> cherry-pick" but not by "git cherry-pick --ff",
Yes, indeed! Good catch!
> and that even without
> setting the variables, "git cherry-pick" will pick a new commit date but
> "git cherry-pick --ff" wouldn't. The latter, I think is the only
> difference that is worth pondering further.
Because --no-ff could be used when the GIT_COMMITTER_* and GIT_AUTHOR_* env
variable should be respected? Or because we should check if one of these
env variable is set and, if that is the case, we should not fast forward?
As I think it would be a big backward incompatibility to force people to
update their scripts to add --no-ff, I think you probably suggest the
latter. This mean that we could have both --ff and --no-ff. --ff could
force fast forward even if some of the above env variables are set. --no-ff
would disable fast forward even if none of the above env variable is set.
> My impression is that a user would never have any problem with
> fast-forwarding. For scripts probably the same is true (the typical
> scenario for scripts is probably very similar to what "git rebase -i"
> does), but it can still be a potential backwards-incompatibility in case
> the script is *expecting* cherry-picking to generate a new SHA1. How
> broken can we consider this expectation?
I am not too worried by this expectation, but I think that, as it looks like
we will need --ff anyway, it is safer to start by implementing --ff like I
did and then later we can implement --no-ff and change the default (when
neither --ff nor --no-ff is used) to look at env variables (or config
variables) to decide if we will fast forward or not.
> That said, to reply to your question, of course -n would disable it, and
> so would -x, -s and possibly -e. But then, the same applies to --ff:
> your patch forbids "-n --ff", but -x, -s and -e would need the same
> treatment.
Yeah, I will add the same treatment for these options.
> Note that "-e --ff" would error out; however if --ff would be the
> default, "-e" would probably choose between fast-forward and
> non-fast-forward depending on whether the commit message was edited.
Yeah, but let's change the default later please.
Thanks,
Christian.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking
2010-02-02 5:13 ` Christian Couder
@ 2010-02-02 7:56 ` Paolo Bonzini
0 siblings, 0 replies; 14+ messages in thread
From: Paolo Bonzini @ 2010-02-02 7:56 UTC (permalink / raw)
To: Christian Couder
Cc: Christian Couder, Junio C Hamano, git, Linus Torvalds,
Johannes Schindelin, Stephan Beyer, Daniel Barkalow
>> and that even without
>> setting the variables, "git cherry-pick" will pick a new commit date but
>> "git cherry-pick --ff" wouldn't. The latter, I think is the only
>> difference that is worth pondering further.
>
> Because --no-ff could be used when the GIT_COMMITTER_* and GIT_AUTHOR_* env
> variable should be respected? Or because we should check if one of these
> env variable is set and, if that is the case, we should not fast forward?
I wish it could be the former, at least in the long run; after all git
merge fast-forwards by default, and it doesn't adjust its behavior if
GIT_COMMITTER_* is passed.
Anyway, your plan of starting with --ff and changing the default to
--no-ff later seems fine. Maybe you can add --no-ff now already, though
it would be a no-op.
>> Note that "-e --ff" would error out; however if --ff would be the
>> default, "-e" would probably choose between fast-forward and
>> non-fast-forward depending on whether the commit message was edited.
>
> Yeah, but let's change the default later please.
Sure.
Paolo
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC/PATCH 6/6] rebase -i: use new --ff cherry-pick option
2010-02-01 7:55 [RFC/PATCH 0/6] add --ff option to cherry-pick Christian Couder
` (4 preceding siblings ...)
2010-02-01 7:55 ` [RFC/PATCH 5/6] revert: add --ff option to allow fast forward when cherry-picking Christian Couder
@ 2010-02-01 7:55 ` Christian Couder
5 siblings, 0 replies; 14+ messages in thread
From: Christian Couder @ 2010-02-01 7:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Linus Torvalds, Johannes Schindelin, Stephan Beyer,
Daniel Barkalow
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
^ permalink raw reply related [flat|nested] 14+ messages in thread