* [PATCH 15/18] revert: Remove sequencer state when no commits are pending
From: Ramkumar Ramachandra @ 2011-08-01 18:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
When cherry-pick or revert is called on a list of commits, and a
conflict encountered somewhere in the middle, the data in
".git/sequencer" is required to continue the operation. However, when
a conflict is encountered in the very last commit, the user will have
to "continue" after resolving the conflict and committing just so that
the sequencer state is removed. This is how the current "rebase -i"
script works as well.
$ git cherry-pick foo..bar
... conflict encountered while picking "bar" ...
$ echo "resolved" >problematicfile
$ git add problematicfile
$ git commit
$ git cherry-pick --continue # This would be a no-op
Change this so that the sequencer state is cleared when a conflict is
encountered in the last commit. Incidentally, this patch makes sure
that some existing tests don't break when features like "--reset" and
"--continue" are implemented later in the series.
A better way to implement this feature is to get the last "git commit"
to remove the sequencer state. However, that requires tighter
coupling between "git commit" and the sequencer, a goal that can be
pursued once the sequencer is made more general.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 12 +++++++++++-
t/t3510-cherry-pick-sequence.sh | 24 ++++++++++++++++++++++++
2 files changed, 35 insertions(+), 1 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 2ddd7e7..ce57301 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -765,8 +765,18 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
res = do_pick_commit(cur->item, opts);
- if (res)
+ if (res) {
+ if (!cur->next)
+ /*
+ * An error was encountered while
+ * picking the last commit; the
+ * sequencer state is useless now --
+ * the user simply needs to resolve
+ * the conflict and commit
+ */
+ remove_sequencer_state(0);
return res;
+ }
}
/*
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index a3427a5..0b68397 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -82,4 +82,28 @@ test_expect_success '--reset cleans up sequencer state' '
test_path_is_missing .git/sequencer
'
+test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..picked &&
+ test_path_is_missing .git/sequencer &&
+ echo "resolved" >foo &&
+ git add foo &&
+ git commit &&
+ {
+ git rev-list HEAD |
+ git diff-tree --root --stdin |
+ sed "s/$_x40/OBJID/g"
+ } >actual &&
+ cat >expect <<-\EOF &&
+ OBJID
+ :100644 100644 OBJID OBJID M foo
+ OBJID
+ :100644 100644 OBJID OBJID M unrelated
+ OBJID
+ :000000 100644 OBJID OBJID A foo
+ :000000 100644 OBJID OBJID A unrelated
+ EOF
+ test_cmp expect actual
+'
+
test_done
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* Re: git-svn questions
From: Jeff King @ 2011-08-01 18:12 UTC (permalink / raw)
To: John M. Dlugosz; +Cc: git
In-Reply-To: <29920-1312210883-194720@sneakemail.com>
On Mon, Aug 01, 2011 at 10:01:17AM -0500, John M. Dlugosz wrote:
> I want to introduce git to a project that's currently using svn.
> I used svn git with no problems. But someone else trying the same
> steps runs into authentication issues.
> How did mine "just know" what my username for SVN is? I'm using
> Tortoise-svn and it remembers (I only ever signed in once, when I set
> it up), but so is he.
>
> We are on windows, using msysgit.
I believe SVN will cache your username (and optionally password, in
cleartext!) forever in ~/.subversion/auth. I have no idea if Tortoise
writes to the same area or not. Are you sure you never ran "svn" from
the command-line, in addition to running Tortoise?
Just a guess. I know very little about how svn works in this respect. :)
-Peff
^ permalink raw reply
* [PATCH 14/18] reset: Make reset remove the sequencer state
From: Ramkumar Ramachandra @ 2011-08-01 18:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Years of muscle memory have trained users to use "git reset --hard" to
remove the branch state after any sort operation. Make it also remove
the sequencer state to facilitate this established workflow:
$ git cherry-pick foo..bar
... conflict encountered ...
$ git reset --hard # Oops, I didn't mean that
$ git cherry-pick quux..bar
... cherry-pick succeeded ...
Guard against accidental removal of the sequencer state by providing
one level of "undo". In the first "reset" invocation,
".git/sequencer" is moved to ".git/sequencer-old"; it is completely
removed only in the second invocation.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
branch.c | 2 ++
t/7106-reset-sequence.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 0 deletions(-)
create mode 100755 t/7106-reset-sequence.sh
diff --git a/branch.c b/branch.c
index c0c865a..d06aec4 100644
--- a/branch.c
+++ b/branch.c
@@ -3,6 +3,7 @@
#include "refs.h"
#include "remote.h"
#include "commit.h"
+#include "sequencer.h"
struct tracking {
struct refspec spec;
@@ -228,4 +229,5 @@ void remove_branch_state(void)
unlink(git_path("MERGE_MSG"));
unlink(git_path("MERGE_MODE"));
unlink(git_path("SQUASH_MSG"));
+ remove_sequencer_state(0);
}
diff --git a/t/7106-reset-sequence.sh b/t/7106-reset-sequence.sh
new file mode 100755
index 0000000..4956caa
--- /dev/null
+++ b/t/7106-reset-sequence.sh
@@ -0,0 +1,44 @@
+#!/bin/sh
+
+test_description='Test interaction of reset --hard with sequencer
+
+ + anotherpick: rewrites foo to d
+ + picked: rewrites foo to c
+ + unrelatedpick: rewrites unrelated to reallyunrelated
+ + base: rewrites foo to b
+ + initial: writes foo as a, unrelated as unrelated
+'
+
+. ./test-lib.sh
+
+pristine_detach () {
+ git cherry-pick --reset &&
+ git checkout -f "$1^0" &&
+ git read-tree -u --reset HEAD &&
+ git clean -d -f -f -q -x
+}
+
+test_expect_success setup '
+ echo unrelated >unrelated &&
+ git add unrelated &&
+ test_commit initial foo a &&
+ test_commit base foo b &&
+ test_commit unrelatedpick unrelated reallyunrelated &&
+ test_commit picked foo c &&
+ test_commit anotherpick foo d &&
+ git config advice.detachedhead false
+
+'
+
+test_expect_success 'reset --hard cleans up sequencer state, providing one-level undo' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ test_path_is_dir .git/sequencer &&
+ git reset --hard &&
+ test_path_is_missing .git/sequencer &&
+ test_path_is_dir .git/sequencer-old &&
+ git reset --hard &&
+ test_path_is_missing .git/sequencer-old
+'
+
+test_done
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 13/18] revert: Introduce --reset to remove sequencer state
From: Ramkumar Ramachandra @ 2011-08-01 18:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
To explicitly remove the sequencer state for a fresh cherry-pick or
revert invocation, introduce a new subcommand called "--reset" to
remove the sequencer state.
Take the opportunity to publicly expose the sequencer paths, and a
generic function called "remove_sequencer_state" that various git
programs can use to remove the sequencer state in a uniform manner;
"git reset" uses it later in this series. Introducing this public API
is also in line with our long-term goal of eventually factoring out
functions from revert.c into a generic commit sequencer.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Documentation/git-cherry-pick.txt | 5 +++
Documentation/git-revert.txt | 5 +++
Documentation/sequencer.txt | 4 ++
Makefile | 2 +
builtin/revert.c | 62 +++++++++++++++++++++++++-----------
sequencer.c | 19 +++++++++++
sequencer.h | 20 ++++++++++++
t/t3510-cherry-pick-sequence.sh | 15 ++++++++-
8 files changed, 111 insertions(+), 21 deletions(-)
create mode 100644 Documentation/sequencer.txt
create mode 100644 sequencer.c
create mode 100644 sequencer.h
diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
index 6c9c2cb..12a85ba 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -9,6 +9,7 @@ SYNOPSIS
--------
[verse]
'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff] <commit>...
+'git cherry-pick' --reset
DESCRIPTION
-----------
@@ -110,6 +111,10 @@ effect to your index in a row.
Pass the merge strategy-specific option through to the
merge strategy. See linkgit:git-merge[1] for details.
+SEQUENCER SUBCOMMANDS
+---------------------
+include::sequencer.txt[]
+
EXAMPLES
--------
git cherry-pick master::
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 3d0a7d1..31c85b4 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -9,6 +9,7 @@ SYNOPSIS
--------
[verse]
'git revert' [--edit | --no-edit] [-n] [-m parent-number] [-s] <commit>...
+'git revert' --reset
DESCRIPTION
-----------
@@ -91,6 +92,10 @@ effect to your index in a row.
Pass the merge strategy-specific option through to the
merge strategy. See linkgit:git-merge[1] for details.
+SEQUENCER SUBCOMMANDS
+---------------------
+include::sequencer.txt[]
+
EXAMPLES
--------
git revert HEAD~3::
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
new file mode 100644
index 0000000..16ce88c
--- /dev/null
+++ b/Documentation/sequencer.txt
@@ -0,0 +1,4 @@
+--reset::
+ Forget about the current operation in progress. Can be used
+ to clear the sequencer state after a failed cherry-pick or
+ revert.
diff --git a/Makefile b/Makefile
index 4ed7996..afd7673 100644
--- a/Makefile
+++ b/Makefile
@@ -554,6 +554,7 @@ LIB_H += rerere.h
LIB_H += resolve-undo.h
LIB_H += revision.h
LIB_H += run-command.h
+LIB_H += sequencer.h
LIB_H += sha1-array.h
LIB_H += sha1-lookup.h
LIB_H += sideband.h
@@ -658,6 +659,7 @@ LIB_OBJS += revision.o
LIB_OBJS += run-command.o
LIB_OBJS += server-info.o
LIB_OBJS += setup.o
+LIB_OBJS += sequencer.o
LIB_OBJS += sha1-array.o
LIB_OBJS += sha1-lookup.o
LIB_OBJS += sha1_file.o
diff --git a/builtin/revert.c b/builtin/revert.c
index cc130e0..2ddd7e7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -14,6 +14,7 @@
#include "merge-recursive.h"
#include "refs.h"
#include "dir.h"
+#include "sequencer.h"
/*
* This implements the builtins revert and cherry-pick.
@@ -28,18 +29,22 @@
static const char * const revert_usage[] = {
"git revert [options] <commit-ish>",
+ "git revert <subcommand>",
NULL
};
static const char * const cherry_pick_usage[] = {
"git cherry-pick [options] <commit-ish>",
+ "git cherry-pick <subcommand>",
NULL
};
enum replay_action { REVERT, CHERRY_PICK };
+enum replay_subcommand { REPLAY_NONE, REPLAY_RESET };
struct replay_opts {
enum replay_action action;
+ enum replay_subcommand subcommand;
/* Boolean options */
int edit;
@@ -61,11 +66,6 @@ struct replay_opts {
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
-#define SEQ_DIR "sequencer"
-#define SEQ_HEAD_FILE "sequencer/head"
-#define SEQ_TODO_FILE "sequencer/todo"
-#define SEQ_OPTS_FILE "sequencer/opts"
-
static const char *action_name(const struct replay_opts *opts)
{
return opts->action == REVERT ? "revert" : "cherry-pick";
@@ -115,7 +115,9 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
const char *me = action_name(opts);
int noop;
+ int reset = 0;
struct option options[] = {
+ OPT_BOOLEAN(0, "reset", &reset, "forget the current operation"),
OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
@@ -144,7 +146,27 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
PARSE_OPT_KEEP_ARGV0 |
PARSE_OPT_KEEP_UNKNOWN);
- if (opts->commit_argc < 2)
+
+ /* Set the subcommand */
+ if (reset)
+ opts->subcommand = REPLAY_RESET;
+ else
+ opts->subcommand = REPLAY_NONE;
+
+ /* Check for incompatible command line arguments */
+ if (opts->subcommand == REPLAY_RESET) {
+ verify_opt_compatible(me, "--reset",
+ "--no-commit", opts->no_commit,
+ "--signoff", opts->signoff,
+ "--mainline", opts->mainline,
+ "--strategy", opts->strategy ? 1 : 0,
+ "--strategy-option", opts->xopts ? 1 : 0,
+ "-x", opts->record_origin,
+ "--ff", opts->allow_ff,
+ NULL);
+ }
+
+ else if (opts->commit_argc < 2)
usage_with_options(usage_str, options);
if (opts->allow_ff)
@@ -731,7 +753,6 @@ static void save_opts(struct replay_opts *opts)
static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
{
- struct strbuf buf = STRBUF_INIT;
struct commit_list *cur;
int res;
@@ -752,9 +773,7 @@ static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
* Sequence of picks finished successfully; cleanup by
* removing the .git/sequencer directory
*/
- strbuf_addf(&buf, "%s", git_path(SEQ_DIR));
- remove_dir_recursively(&buf, 0);
- strbuf_release(&buf);
+ remove_sequencer_state(1);
return 0;
}
@@ -770,16 +789,21 @@ static int pick_revisions(struct replay_opts *opts)
* cherry-pick should be handled differently from an existing
* one that is being continued
*/
- walk_revs_populate_todo(&todo_list, opts);
- create_seq_dir();
- if (get_sha1("HEAD", sha1)) {
- if (opts->action == REVERT)
- die(_("Can't revert as initial commit"));
- die(_("Can't cherry-pick into empty head"));
+ if (opts->subcommand == REPLAY_RESET) {
+ remove_sequencer_state(1);
+ return 0;
+ } else {
+ /* Start a new cherry-pick/ revert sequence */
+ walk_revs_populate_todo(&todo_list, opts);
+ create_seq_dir();
+ if (get_sha1("HEAD", sha1)) {
+ if (opts->action == REVERT)
+ die(_("Can't revert as initial commit"));
+ die(_("Can't cherry-pick into empty head"));
+ }
+ save_head(sha1_to_hex(sha1));
+ save_opts(opts);
}
- save_head(sha1_to_hex(sha1));
- save_opts(opts);
-
return pick_commits(todo_list, opts);
}
diff --git a/sequencer.c b/sequencer.c
new file mode 100644
index 0000000..bc2c046
--- /dev/null
+++ b/sequencer.c
@@ -0,0 +1,19 @@
+#include "cache.h"
+#include "sequencer.h"
+#include "strbuf.h"
+#include "dir.h"
+
+void remove_sequencer_state(int aggressive)
+{
+ struct strbuf seq_dir = STRBUF_INIT;
+ struct strbuf seq_old_dir = STRBUF_INIT;
+
+ strbuf_addf(&seq_dir, "%s", git_path(SEQ_DIR));
+ strbuf_addf(&seq_old_dir, "%s", git_path(SEQ_OLD_DIR));
+ remove_dir_recursively(&seq_old_dir, 0);
+ rename(git_path(SEQ_DIR), git_path(SEQ_OLD_DIR));
+ if (aggressive)
+ remove_dir_recursively(&seq_old_dir, 0);
+ strbuf_release(&seq_dir);
+ strbuf_release(&seq_old_dir);
+}
diff --git a/sequencer.h b/sequencer.h
new file mode 100644
index 0000000..905d295
--- /dev/null
+++ b/sequencer.h
@@ -0,0 +1,20 @@
+#ifndef SEQUENCER_H
+#define SEQUENCER_H
+
+#define SEQ_DIR "sequencer"
+#define SEQ_OLD_DIR "sequencer-old"
+#define SEQ_HEAD_FILE "sequencer/head"
+#define SEQ_TODO_FILE "sequencer/todo"
+#define SEQ_OPTS_FILE "sequencer/opts"
+
+/*
+ * Removes SEQ_OLD_DIR and renames SEQ_DIR to SEQ_OLD_DIR, ignoring
+ * any errors. Intended to be used by 'git reset'.
+ *
+ * With the aggressive flag, it additionally removes SEQ_OLD_DIR,
+ * ignoring any errors. Inteded to be used by the sequencer's
+ * '--reset' subcommand.
+ */
+void remove_sequencer_state(int aggressive);
+
+#endif
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 6038419..a3427a5 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -13,7 +13,7 @@ test_description='Test cherry-pick continuation features
. ./test-lib.sh
pristine_detach () {
- rm -rf .git/sequencer &&
+ git cherry-pick --reset &&
git checkout -f "$1^0" &&
git read-tree -u --reset HEAD &&
git clean -d -f -f -q -x
@@ -41,7 +41,6 @@ test_expect_success 'cherry-pick persists data on failure' '
'
test_expect_success 'cherry-pick persists opts correctly' '
- rm -rf .git/sequencer &&
pristine_detach initial &&
test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
test_path_is_dir .git/sequencer &&
@@ -71,4 +70,16 @@ test_expect_success 'cherry-pick cleans up sequencer state upon success' '
test_path_is_missing .git/sequencer
'
+test_expect_success '--reset does not complain when no cherry-pick is in progress' '
+ pristine_detach initial &&
+ git cherry-pick --reset
+'
+
+test_expect_success '--reset cleans up sequencer state' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..picked &&
+ git cherry-pick --reset &&
+ test_path_is_missing .git/sequencer
+'
+
test_done
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 12/18] revert: Make pick_commits functionally act on a commit list
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Apart from its central objective of calling into the picking
mechanism, pick_commits creates a sequencer directory, prepares a todo
list, and even acts upon the "--reset" subcommand. This makes for a
bad API since the central worry of callers is to figure out whether or
not any conflicts were encountered during the cherry picking. The
current API is like:
if (pick_commits(opts) < 0)
print "Something failed, we're not sure what"
So, change pick_commits so that it's only responsible for picking
commits in a loop and reporting any errors, leaving the rest to a new
function called pick_revisions. Consequently, the API of pick_commits
becomes much clearer:
act_on_subcommand(opts->subcommand);
todo_list = prepare_todo_list();
if (pick_commits(todo_list, opts) < 0)
print "Error encountered while picking commits"
Now, callers can easily call-in to the cherry-picking machinery by
constructing an arbitrary todo list along with some options.
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 43 ++++++++++++++++++++++++++++---------------
1 files changed, 28 insertions(+), 15 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 7aa0aaf..cc130e0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -729,11 +729,9 @@ static void save_opts(struct replay_opts *opts)
}
}
-static int pick_commits(struct replay_opts *opts)
+static int pick_commits(struct commit_list *todo_list, struct replay_opts *opts)
{
- struct commit_list *todo_list = NULL;
struct strbuf buf = STRBUF_INIT;
- unsigned char sha1[20];
struct commit_list *cur;
int res;
@@ -743,16 +741,6 @@ static int pick_commits(struct replay_opts *opts)
opts->record_origin || opts->edit));
read_and_refresh_cache(opts);
- walk_revs_populate_todo(&todo_list, opts);
- create_seq_dir();
- if (get_sha1("HEAD", sha1)) {
- if (opts->action == REVERT)
- die(_("Can't revert as initial commit"));
- die(_("Can't cherry-pick into empty head"));
- }
- save_head(sha1_to_hex(sha1));
- save_opts(opts);
-
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
res = do_pick_commit(cur->item, opts);
@@ -770,6 +758,31 @@ static int pick_commits(struct replay_opts *opts)
return 0;
}
+static int pick_revisions(struct replay_opts *opts)
+{
+ struct commit_list *todo_list = NULL;
+ unsigned char sha1[20];
+
+ read_and_refresh_cache(opts);
+
+ /*
+ * Decide what to do depending on the arguments; a fresh
+ * cherry-pick should be handled differently from an existing
+ * one that is being continued
+ */
+ walk_revs_populate_todo(&todo_list, opts);
+ create_seq_dir();
+ if (get_sha1("HEAD", sha1)) {
+ if (opts->action == REVERT)
+ die(_("Can't revert as initial commit"));
+ die(_("Can't cherry-pick into empty head"));
+ }
+ save_head(sha1_to_hex(sha1));
+ save_opts(opts);
+
+ return pick_commits(todo_list, opts);
+}
+
int cmd_revert(int argc, const char **argv, const char *prefix)
{
struct replay_opts opts;
@@ -780,7 +793,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
opts.action = REVERT;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
- return pick_commits(&opts);
+ return pick_revisions(&opts);
}
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
@@ -791,5 +804,5 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
opts.action = CHERRY_PICK;
git_config(git_default_config, NULL);
parse_args(argc, argv, &opts);
- return pick_commits(&opts);
+ return pick_revisions(&opts);
}
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 11/18] revert: Save command-line options for continuing operation
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
In the same spirit as ".git/sequencer/head" and ".git/sequencer/todo",
introduce ".git/sequencer/opts" to persist the replay_opts structure
for continuing after a conflict resolution. Use the gitconfig format
for this file so that it looks like:
[options]
signoff = true
record-origin = true
mainline = 1
strategy = recursive
strategy-option = patience
strategy-option = ours
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 33 +++++++++++++++++++++++++++++++++
t/t3510-cherry-pick-sequence.sh | 30 ++++++++++++++++++++++++++++--
2 files changed, 61 insertions(+), 2 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 1d5a242..7aa0aaf 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -64,6 +64,7 @@ struct replay_opts {
#define SEQ_DIR "sequencer"
#define SEQ_HEAD_FILE "sequencer/head"
#define SEQ_TODO_FILE "sequencer/todo"
+#define SEQ_OPTS_FILE "sequencer/opts"
static const char *action_name(const struct replay_opts *opts)
{
@@ -697,6 +698,37 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
strbuf_release(&buf);
}
+static void save_opts(struct replay_opts *opts)
+{
+ const char *opts_file = git_path(SEQ_OPTS_FILE);
+
+ if (opts->no_commit)
+ git_config_set_in_file(opts_file, "options.no-commit", "true");
+ if (opts->edit)
+ git_config_set_in_file(opts_file, "options.edit", "true");
+ if (opts->signoff)
+ git_config_set_in_file(opts_file, "options.signoff", "true");
+ if (opts->record_origin)
+ git_config_set_in_file(opts_file, "options.record-origin", "true");
+ if (opts->allow_ff)
+ git_config_set_in_file(opts_file, "options.allow-ff", "true");
+ if (opts->mainline) {
+ struct strbuf buf = STRBUF_INIT;
+ strbuf_addf(&buf, "%d", opts->mainline);
+ git_config_set_in_file(opts_file, "options.mainline", buf.buf);
+ strbuf_release(&buf);
+ }
+ if (opts->strategy)
+ git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
+ if (opts->xopts) {
+ int i;
+ for (i = 0; i < opts->xopts_nr; i++)
+ git_config_set_multivar_in_file(opts_file,
+ "options.strategy-option",
+ opts->xopts[i], "^$", 0);
+ }
+}
+
static int pick_commits(struct replay_opts *opts)
{
struct commit_list *todo_list = NULL;
@@ -719,6 +751,7 @@ static int pick_commits(struct replay_opts *opts)
die(_("Can't cherry-pick into empty head"));
}
save_head(sha1_to_hex(sha1));
+ save_opts(opts);
for (cur = todo_list; cur; cur = cur->next) {
save_todo(cur, opts);
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index a2c70ad..6038419 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -33,10 +33,36 @@ test_expect_success setup '
test_expect_success 'cherry-pick persists data on failure' '
pristine_detach initial &&
- test_must_fail git cherry-pick base..anotherpick &&
+ test_must_fail git cherry-pick -s base..anotherpick &&
test_path_is_dir .git/sequencer &&
test_path_is_file .git/sequencer/head &&
- test_path_is_file .git/sequencer/todo
+ test_path_is_file .git/sequencer/todo &&
+ test_path_is_file .git/sequencer/opts
+'
+
+test_expect_success 'cherry-pick persists opts correctly' '
+ rm -rf .git/sequencer &&
+ pristine_detach initial &&
+ test_must_fail git cherry-pick -s -m 1 --strategy=recursive -X patience -X ours base..anotherpick &&
+ test_path_is_dir .git/sequencer &&
+ test_path_is_file .git/sequencer/head &&
+ test_path_is_file .git/sequencer/todo &&
+ test_path_is_file .git/sequencer/opts &&
+ echo "true" >expect
+ git config --file=.git/sequencer/opts --get-all options.signoff >actual &&
+ test_cmp expect actual &&
+ echo "1" >expect
+ git config --file=.git/sequencer/opts --get-all options.mainline >actual &&
+ test_cmp expect actual &&
+ echo "recursive" >expect
+ git config --file=.git/sequencer/opts --get-all options.strategy >actual &&
+ test_cmp expect actual &&
+ cat >expect <<-\EOF
+ patience
+ ours
+ EOF
+ git config --file=.git/sequencer/opts --get-all options.strategy-option >actual &&
+ test_cmp expect actual
'
test_expect_success 'cherry-pick cleans up sequencer state upon success' '
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 10/18] revert: Save data for continuing after conflict resolution
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than
one commit, 2010-06-02), a single invocation of "git cherry-pick" or
"git revert" can perform picks of several individual commits. To
implement features like "--continue" to continue the whole operation,
we will need to store some information about the state and the plan at
the beginning. Introduce a ".git/sequencer/head" file to store this
state, and ".git/sequencer/todo" file to store the plan. The head
file contains the SHA-1 of the HEAD before the start of the operation,
and the todo file contains an instruction sheet whose format is
inspired by the format of the "rebase -i" instruction sheet. As a
result, a typical todo file looks like:
pick 8537f0e submodule add: test failure when url is not configured
pick 4d68932 submodule add: allow relative repository path
pick f22a17e submodule add: clean up duplicated code
pick 59a5775 make copy_ref globally available
Since SHA-1 hex is abbreviated using an find_unique_abbrev(), it is
unambiguous. This does not guarantee that there will be no ambiguity
when more objects are added to the repository.
These two files alone are not enough to implement a "--continue" that
remembers the command-line options specified; later patches in the
series save them too.
These new files are unrelated to the existing .git/CHERRY_PICK_HEAD,
which will still be useful while committing after a conflict
resolution.
Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 134 +++++++++++++++++++++++++++++++++++++-
t/t3510-cherry-pick-sequence.sh | 48 ++++++++++++++
2 files changed, 178 insertions(+), 4 deletions(-)
create mode 100755 t/t3510-cherry-pick-sequence.sh
diff --git a/builtin/revert.c b/builtin/revert.c
index 0196046..1d5a242 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -13,6 +13,7 @@
#include "rerere.h"
#include "merge-recursive.h"
#include "refs.h"
+#include "dir.h"
/*
* This implements the builtins revert and cherry-pick.
@@ -60,6 +61,10 @@ struct replay_opts {
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
+#define SEQ_DIR "sequencer"
+#define SEQ_HEAD_FILE "sequencer/head"
+#define SEQ_TODO_FILE "sequencer/todo"
+
static const char *action_name(const struct replay_opts *opts)
{
return opts->action == REVERT ? "revert" : "cherry-pick";
@@ -589,10 +594,116 @@ static void read_and_refresh_cache(struct replay_opts *opts)
rollback_lock_file(&index_lock);
}
-static int pick_commits(struct replay_opts *opts)
+/*
+ * Append a commit to the end of the commit_list.
+ *
+ * next starts by pointing to the variable that holds the head of an
+ * empty commit_list, and is updated to point to the "next" field of
+ * the last item on the list as new commits are appended.
+ *
+ * Usage example:
+ *
+ * struct commit_list *list;
+ * struct commit_list **next = &list;
+ *
+ * next = commit_list_append(c1, next);
+ * next = commit_list_append(c2, next);
+ * assert(commit_list_count(list) == 2);
+ * return list;
+ */
+struct commit_list **commit_list_append(struct commit *commit,
+ struct commit_list **next)
+{
+ struct commit_list *new = xmalloc(sizeof(struct commit_list));
+ new->item = commit;
+ *next = new;
+ new->next = NULL;
+ return &new->next;
+}
+
+static int format_todo(struct strbuf *buf, struct commit_list *todo_list,
+ struct replay_opts *opts)
+{
+ struct commit_list *cur = NULL;
+ struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+ const char *sha1_abbrev = NULL;
+ const char *action_str = opts->action == REVERT ? "revert" : "pick";
+
+ for (cur = todo_list; cur; cur = cur->next) {
+ sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
+ if (get_message(cur->item, &msg))
+ return error(_("Cannot get commit message for %s"), sha1_abbrev);
+ strbuf_addf(buf, "%s %s %s\n", action_str, sha1_abbrev, msg.subject);
+ }
+ return 0;
+}
+
+static void walk_revs_populate_todo(struct commit_list **todo_list,
+ struct replay_opts *opts)
{
struct rev_info revs;
struct commit *commit;
+ struct commit_list **next;
+
+ prepare_revs(&revs, opts);
+
+ next = todo_list;
+ while ((commit = get_revision(&revs)))
+ next = commit_list_append(commit, next);
+}
+
+static void create_seq_dir(void)
+{
+ const char *seq_dir = git_path(SEQ_DIR);
+
+ if (!(file_exists(seq_dir) && is_directory(seq_dir))
+ && mkdir(seq_dir, 0777) < 0)
+ die_errno(_("Could not create sequencer directory '%s'."), seq_dir);
+}
+
+static void save_head(const char *head)
+{
+ const char *head_file = git_path(SEQ_HEAD_FILE);
+ static struct lock_file head_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ fd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);
+ strbuf_addf(&buf, "%s\n", head);
+ if (write_in_full(fd, buf.buf, buf.len) < 0)
+ die_errno(_("Could not write to %s."), head_file);
+ if (commit_lock_file(&head_lock) < 0)
+ die(_("Error wrapping up %s."), head_file);
+}
+
+static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
+{
+ const char *todo_file = git_path(SEQ_TODO_FILE);
+ static struct lock_file todo_lock;
+ struct strbuf buf = STRBUF_INIT;
+ int fd;
+
+ fd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);
+ if (format_todo(&buf, todo_list, opts) < 0)
+ die(_("Could not format %s."), todo_file);
+ if (write_in_full(fd, buf.buf, buf.len) < 0) {
+ strbuf_release(&buf);
+ die_errno(_("Could not write to %s."), todo_file);
+ }
+ if (commit_lock_file(&todo_lock) < 0) {
+ strbuf_release(&buf);
+ die(_("Error wrapping up %s."), todo_file);
+ }
+ strbuf_release(&buf);
+}
+
+static int pick_commits(struct replay_opts *opts)
+{
+ struct commit_list *todo_list = NULL;
+ struct strbuf buf = STRBUF_INIT;
+ unsigned char sha1[20];
+ struct commit_list *cur;
+ int res;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
if (opts->allow_ff)
@@ -600,14 +711,29 @@ static int pick_commits(struct replay_opts *opts)
opts->record_origin || opts->edit));
read_and_refresh_cache(opts);
- prepare_revs(&revs, opts);
+ walk_revs_populate_todo(&todo_list, opts);
+ create_seq_dir();
+ if (get_sha1("HEAD", sha1)) {
+ if (opts->action == REVERT)
+ die(_("Can't revert as initial commit"));
+ die(_("Can't cherry-pick into empty head"));
+ }
+ save_head(sha1_to_hex(sha1));
- while ((commit = get_revision(&revs))) {
- int res = do_pick_commit(commit, opts);
+ for (cur = todo_list; cur; cur = cur->next) {
+ save_todo(cur, opts);
+ res = do_pick_commit(cur->item, opts);
if (res)
return res;
}
+ /*
+ * Sequence of picks finished successfully; cleanup by
+ * removing the .git/sequencer directory
+ */
+ strbuf_addf(&buf, "%s", git_path(SEQ_DIR));
+ remove_dir_recursively(&buf, 0);
+ strbuf_release(&buf);
return 0;
}
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
new file mode 100755
index 0000000..a2c70ad
--- /dev/null
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -0,0 +1,48 @@
+#!/bin/sh
+
+test_description='Test cherry-pick continuation features
+
+ + anotherpick: rewrites foo to d
+ + picked: rewrites foo to c
+ + unrelatedpick: rewrites unrelated to reallyunrelated
+ + base: rewrites foo to b
+ + initial: writes foo as a, unrelated as unrelated
+
+'
+
+. ./test-lib.sh
+
+pristine_detach () {
+ rm -rf .git/sequencer &&
+ git checkout -f "$1^0" &&
+ git read-tree -u --reset HEAD &&
+ git clean -d -f -f -q -x
+}
+
+test_expect_success setup '
+ echo unrelated >unrelated &&
+ git add unrelated &&
+ test_commit initial foo a &&
+ test_commit base foo b &&
+ test_commit unrelatedpick unrelated reallyunrelated &&
+ test_commit picked foo c &&
+ test_commit anotherpick foo d &&
+ git config advice.detachedhead false
+
+'
+
+test_expect_success 'cherry-pick persists data on failure' '
+ pristine_detach initial &&
+ test_must_fail git cherry-pick base..anotherpick &&
+ test_path_is_dir .git/sequencer &&
+ test_path_is_file .git/sequencer/head &&
+ test_path_is_file .git/sequencer/todo
+'
+
+test_expect_success 'cherry-pick cleans up sequencer state upon success' '
+ pristine_detach initial &&
+ git cherry-pick initial..picked &&
+ test_path_is_missing .git/sequencer
+'
+
+test_done
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 09/18] revert: Don't create invalid replay_opts in parse_args
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
The "--ff" command-line option cannot be used with some other
command-line options. However, parse_args still parses these
incompatible options into a replay_opts structure for use by the rest
of the program. Although pick_commits, the current gatekeeper to the
cherry-pick machinery, checks the validity of the replay_opts
structure before before starting its operation, there will be multiple
entry points to the cherry-pick machinery in future. To futureproof
the code and catch these errors in one place, make sure that an
invalid replay_opts structure is not created by parse_args in the
first place. We still check the replay_opts structure for validity in
pick_commits, but this is an assert() now to emphasize that it's the
caller's responsibility to get it right.
Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 40 +++++++++++++++++++++++++++++-----------
1 files changed, 29 insertions(+), 11 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 90fe2ef..0196046 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -86,9 +86,28 @@ static int option_parse_x(const struct option *opt,
return 0;
}
+static void verify_opt_compatible(const char *me, const char *base_opt, ...)
+{
+ const char *this_opt;
+ va_list ap;
+ int set;
+
+ va_start(ap, base_opt);
+ while ((this_opt = va_arg(ap, const char *))) {
+ set = va_arg(ap, int);
+ if (set) {
+ va_end(ap);
+ die(_("%s: %s cannot be used with %s"),
+ me, this_opt, base_opt);
+ }
+ }
+ va_end(ap);
+}
+
static void parse_args(int argc, const char **argv, struct replay_opts *opts)
{
const char * const * usage_str = revert_or_cherry_pick_usage(opts);
+ const char *me = action_name(opts);
int noop;
struct option options[] = {
OPT_BOOLEAN('n', "no-commit", &opts->no_commit, "don't automatically commit"),
@@ -122,6 +141,13 @@ static void parse_args(int argc, const char **argv, struct replay_opts *opts)
if (opts->commit_argc < 2)
usage_with_options(usage_str, options);
+ if (opts->allow_ff)
+ verify_opt_compatible(me, "--ff",
+ "--signoff", opts->signoff,
+ "--no-commit", opts->no_commit,
+ "-x", opts->record_origin,
+ "--edit", opts->edit,
+ NULL);
opts->commit_argv = argv;
}
@@ -569,17 +595,9 @@ static int pick_commits(struct replay_opts *opts)
struct commit *commit;
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
- if (opts->allow_ff) {
- if (opts->signoff)
- die(_("cherry-pick --ff cannot be used with --signoff"));
- if (opts->no_commit)
- die(_("cherry-pick --ff cannot be used with --no-commit"));
- if (opts->record_origin)
- die(_("cherry-pick --ff cannot be used with -x"));
- if (opts->edit)
- die(_("cherry-pick --ff cannot be used with --edit"));
- }
-
+ if (opts->allow_ff)
+ assert(!(opts->signoff || opts->no_commit ||
+ opts->record_origin || opts->edit));
read_and_refresh_cache(opts);
prepare_revs(&revs, opts);
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 08/18] revert: Separate cmdline parsing from functional code
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Currently, revert_or_cherry_pick sets up a default git config, parses
command-line arguments, before preparing to pick commits. This makes
for a bad API as the central worry of callers is to assert whether or
not a conflict occured while cherry picking. The current API is like:
if (revert_or_cherry_pick(argc, argv, opts) < 0)
print "Something failed, we're not sure what"
Simplify and rename revert_or_cherry_pick to pick_commits so that it
only has the responsibility of setting up the revision walker and
picking commits in a loop. Transfer the remaining work to its
callers. Now, the API is simplified as:
if (parse_args(argc, argv, opts) < 0)
print "Can't parse arguments"
if (pick_commits(opts) < 0)
print "Error encountered in picking machinery"
Later in the series, pick_commits will also serve as the starting
point for continuing a cherry-pick or revert.
Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 50f72de..90fe2ef 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -563,16 +563,12 @@ static void read_and_refresh_cache(struct replay_opts *opts)
rollback_lock_file(&index_lock);
}
-static int revert_or_cherry_pick(int argc, const char **argv,
- struct replay_opts *opts)
+static int pick_commits(struct replay_opts *opts)
{
struct rev_info revs;
struct commit *commit;
- git_config(git_default_config, NULL);
setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
- parse_args(argc, argv, opts);
-
if (opts->allow_ff) {
if (opts->signoff)
die(_("cherry-pick --ff cannot be used with --signoff"));
@@ -605,7 +601,9 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
if (isatty(0))
opts.edit = 1;
opts.action = REVERT;
- return revert_or_cherry_pick(argc, argv, &opts);
+ git_config(git_default_config, NULL);
+ parse_args(argc, argv, &opts);
+ return pick_commits(&opts);
}
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
@@ -614,5 +612,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
memset(&opts, 0, sizeof(opts));
opts.action = CHERRY_PICK;
- return revert_or_cherry_pick(argc, argv, &opts);
+ git_config(git_default_config, NULL);
+ parse_args(argc, argv, &opts);
+ return pick_commits(&opts);
}
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 07/18] revert: Introduce struct to keep command-line options
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
The current code uses a set of file-scope static variables to tell the
cherry-pick/ revert machinery how to replay the changes, and
initializes them by parsing the command-line arguments. In later
steps in this series, we would like to introduce an API function that
calls into this machinery directly and have a way to tell it what to
do. Hence, introduce a structure to group these variables, so that
the API can take them as a single replay_options parameter. The only
exception is the variable "me" -- remove it since it not an
independent option, and can be inferred from the action.
Unfortunately, this patch introduces a minor regression. Parsing
strategy-option violates a C89 rule: Initializers cannot refer to
variables whose address is not known at compile time. Currently, this
rule is violated by some other parts of Git as well, and it is
possible to get GCC to report these instances using the "-std=c89
-pedantic" option.
Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 201 ++++++++++++++++++++++++++++++------------------------
1 files changed, 113 insertions(+), 88 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index d6c9f1d..50f72de 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,76 +35,94 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
-static enum { REVERT, CHERRY_PICK } action;
-static int commit_argc;
-static const char **commit_argv;
-static int allow_rerere_auto;
-
-static const char *me;
-
-/* Merge strategy. */
-static const char *strategy;
-static const char **xopts;
-static size_t xopts_nr, xopts_alloc;
+enum replay_action { REVERT, CHERRY_PICK };
+
+struct replay_opts {
+ enum replay_action action;
+
+ /* Boolean options */
+ int edit;
+ int record_origin;
+ int no_commit;
+ int signoff;
+ int allow_ff;
+ int allow_rerere_auto;
+
+ int mainline;
+ int commit_argc;
+ const char **commit_argv;
+
+ /* Merge strategy */
+ const char *strategy;
+ const char **xopts;
+ size_t xopts_nr, xopts_alloc;
+};
#define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
+static const char *action_name(const struct replay_opts *opts)
+{
+ return opts->action == REVERT ? "revert" : "cherry-pick";
+}
+
static char *get_encoding(const char *message);
-static const char * const *revert_or_cherry_pick_usage(void)
+static const char * const *revert_or_cherry_pick_usage(struct replay_opts *opts)
{
- return action == REVERT ? revert_usage : cherry_pick_usage;
+ return opts->action == REVERT ? revert_usage : cherry_pick_usage;
}
static int option_parse_x(const struct option *opt,
const char *arg, int unset)
{
+ struct replay_opts **opts_ptr = opt->value;
+ struct replay_opts *opts = *opts_ptr;
+
if (unset)
return 0;
- ALLOC_GROW(xopts, xopts_nr + 1, xopts_alloc);
- xopts[xopts_nr++] = xstrdup(arg);
+ ALLOC_GROW(opts->xopts, opts->xopts_nr + 1, opts->xopts_alloc);
+ opts->xopts[opts->xopts_nr++] = xstrdup(arg);
return 0;
}
-static void parse_args(int argc, const char **argv)
+static void parse_args(int argc, const char **argv, struct replay_opts *opts)
{
- const char * const * usage_str = revert_or_cherry_pick_usage();
+ const char * const * usage_str = revert_or_cherry_pick_usage(opts);
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('n', "no-commit", &opts->no_commit, "don't automatically commit"),
+ OPT_BOOLEAN('e', "edit", &opts->edit, "edit the commit message"),
{ OPTION_BOOLEAN, 'r', NULL, &noop, NULL, "no-op (backward compatibility)",
PARSE_OPT_NOARG | PARSE_OPT_HIDDEN, NULL, 0 },
- OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by:"),
- OPT_INTEGER('m', "mainline", &mainline, "parent number"),
- OPT_RERERE_AUTOUPDATE(&allow_rerere_auto),
- OPT_STRING(0, "strategy", &strategy, "strategy", "merge strategy"),
- OPT_CALLBACK('X', "strategy-option", &xopts, "option",
+ OPT_BOOLEAN('s', "signoff", &opts->signoff, "add Signed-off-by:"),
+ OPT_INTEGER('m', "mainline", &opts->mainline, "parent number"),
+ OPT_RERERE_AUTOUPDATE(&opts->allow_rerere_auto),
+ OPT_STRING(0, "strategy", &opts->strategy, "strategy", "merge strategy"),
+ OPT_CALLBACK('X', "strategy-option", &opts, "option",
"option for merge strategy", option_parse_x),
OPT_END(),
OPT_END(),
OPT_END(),
};
- if (action == CHERRY_PICK) {
+ if (opts->action == CHERRY_PICK) {
struct option cp_extra[] = {
- OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
- OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
+ OPT_BOOLEAN('x', NULL, &opts->record_origin, "append commit name"),
+ OPT_BOOLEAN(0, "ff", &opts->allow_ff, "allow fast-forward"),
OPT_END(),
};
if (parse_options_concat(options, ARRAY_SIZE(options), cp_extra))
die(_("program error"));
}
- commit_argc = parse_options(argc, argv, NULL, options, usage_str,
- PARSE_OPT_KEEP_ARGV0 |
- PARSE_OPT_KEEP_UNKNOWN);
- if (commit_argc < 2)
+ opts->commit_argc = parse_options(argc, argv, NULL, options, usage_str,
+ PARSE_OPT_KEEP_ARGV0 |
+ PARSE_OPT_KEEP_UNKNOWN);
+ if (opts->commit_argc < 2)
usage_with_options(usage_str, options);
- commit_argv = argv;
+ opts->commit_argv = argv;
}
struct commit_message {
@@ -240,20 +258,20 @@ static struct tree *empty_tree(void)
return tree;
}
-static NORETURN void die_dirty_index(const char *me)
+static NORETURN void die_dirty_index(struct replay_opts *opts)
{
if (read_cache_unmerged()) {
- die_resolve_conflict(me);
+ die_resolve_conflict(action_name(opts));
} else {
if (advice_commit_before_merge) {
- if (action == REVERT)
+ if (opts->action == REVERT)
die(_("Your local changes would be overwritten by revert.\n"
"Please, commit your changes or stash them to proceed."));
else
die(_("Your local changes would be overwritten by cherry-pick.\n"
"Please, commit your changes or stash them to proceed."));
} else {
- if (action == REVERT)
+ if (opts->action == REVERT)
die(_("Your local changes would be overwritten by revert.\n"));
else
die(_("Your local changes would be overwritten by cherry-pick.\n"));
@@ -274,7 +292,8 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from)
static int do_recursive_merge(struct commit *base, struct commit *next,
const char *base_label, const char *next_label,
- unsigned char *head, struct strbuf *msgbuf)
+ unsigned char *head, struct strbuf *msgbuf,
+ struct replay_opts *opts)
{
struct merge_options o;
struct tree *result, *next_tree, *base_tree, *head_tree;
@@ -295,7 +314,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
next_tree = next ? next->tree : empty_tree();
base_tree = base ? base->tree : empty_tree();
- for (xopt = xopts; xopt != xopts + xopts_nr; xopt++)
+ for (xopt = opts->xopts; xopt != opts->xopts + opts->xopts_nr; xopt++)
parse_merge_opt(&o, *xopt);
clean = merge_trees(&o,
@@ -306,7 +325,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
(write_cache(index_fd, active_cache, active_nr) ||
commit_locked_index(&index_lock)))
/* TRANSLATORS: %s will be "revert" or "cherry-pick" */
- die(_("%s: Unable to write new index file"), me);
+ die(_("%s: Unable to write new index file"), action_name(opts));
rollback_lock_file(&index_lock);
if (!clean) {
@@ -335,7 +354,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
* If we are revert, or if our cherry-pick results in a hand merge,
* we had better say that the current user is responsible for that.
*/
-static int run_git_commit(const char *defmsg)
+static int run_git_commit(const char *defmsg, struct replay_opts *opts)
{
/* 6 is max possible length of our args array including NULL */
const char *args[6];
@@ -343,9 +362,9 @@ static int run_git_commit(const char *defmsg)
args[i++] = "commit";
args[i++] = "-n";
- if (signoff)
+ if (opts->signoff)
args[i++] = "-s";
- if (!edit) {
+ if (!opts->edit) {
args[i++] = "-F";
args[i++] = defmsg;
}
@@ -354,7 +373,7 @@ static int run_git_commit(const char *defmsg)
return run_command_v_opt(args, RUN_GIT_CMD);
}
-static int do_pick_commit(struct commit *commit)
+static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -364,7 +383,7 @@ static int do_pick_commit(struct commit *commit)
struct strbuf msgbuf = STRBUF_INIT;
int res;
- if (no_commit) {
+ if (opts->no_commit) {
/*
* We do not intend to commit immediately. We just want to
* merge the differences in, so let's compute the tree
@@ -377,7 +396,7 @@ static int do_pick_commit(struct commit *commit)
if (get_sha1("HEAD", head))
die (_("You do not have a valid HEAD"));
if (index_differs_from("HEAD", 0))
- die_dirty_index(me);
+ die_dirty_index(opts);
}
discard_cache();
@@ -389,32 +408,32 @@ static int do_pick_commit(struct commit *commit)
int cnt;
struct commit_list *p;
- if (!mainline)
+ if (!opts->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 != opts->mainline && p;
cnt++)
p = p->next;
- if (cnt != mainline || !p)
+ if (cnt != opts->mainline || !p)
die(_("Commit %s does not have parent %d"),
- sha1_to_hex(commit->object.sha1), mainline);
+ sha1_to_hex(commit->object.sha1), opts->mainline);
parent = p->item;
- } else if (0 < mainline)
+ } else if (0 < opts->mainline)
die(_("Mainline was specified but commit %s is not a merge."),
sha1_to_hex(commit->object.sha1));
else
parent = commit->parents->item;
- if (allow_ff && parent && !hashcmp(parent->object.sha1, head))
+ if (opts->allow_ff && parent && !hashcmp(parent->object.sha1, head))
return fast_forward_to(commit->object.sha1, head);
if (parent && parse_commit(parent) < 0)
/* TRANSLATORS: The first %s will be "revert" or
"cherry-pick", the second %s a SHA1 */
die(_("%s: cannot parse parent commit %s"),
- me, sha1_to_hex(parent->object.sha1));
+ action_name(opts), sha1_to_hex(parent->object.sha1));
if (get_message(commit, &msg) != 0)
die(_("Cannot get commit message for %s"),
@@ -429,7 +448,7 @@ static int do_pick_commit(struct commit *commit)
defmsg = git_pathdup("MERGE_MSG");
- if (action == REVERT) {
+ if (opts->action == REVERT) {
base = commit;
base_label = msg.label;
next = parent;
@@ -463,18 +482,18 @@ static int do_pick_commit(struct commit *commit)
strbuf_addstr(&msgbuf, p);
}
- if (record_origin) {
+ if (opts->record_origin) {
strbuf_addstr(&msgbuf, "(cherry picked from commit ");
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
strbuf_addstr(&msgbuf, ")\n");
}
- if (!no_commit)
+ if (!opts->no_commit)
write_cherry_pick_head(commit);
}
- if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
+ if (!opts->strategy || !strcmp(opts->strategy, "recursive") || opts->action == REVERT) {
res = do_recursive_merge(base, next, base_label, next_label,
- head, &msgbuf);
+ head, &msgbuf, opts);
write_message(&msgbuf, defmsg);
} else {
struct commit_list *common = NULL;
@@ -484,23 +503,23 @@ static int do_pick_commit(struct commit *commit)
commit_list_insert(base, &common);
commit_list_insert(next, &remotes);
- res = try_merge_command(strategy, xopts_nr, xopts, common,
- sha1_to_hex(head), remotes);
+ res = try_merge_command(opts->strategy, opts->xopts_nr, opts->xopts,
+ common, sha1_to_hex(head), remotes);
free_commit_list(common);
free_commit_list(remotes);
}
if (res) {
- error(action == REVERT
+ error(opts->action == REVERT
? _("could not revert %s... %s")
: _("could not apply %s... %s"),
find_unique_abbrev(commit->object.sha1, DEFAULT_ABBREV),
msg.subject);
print_advice();
- rerere(allow_rerere_auto);
+ rerere(opts->allow_rerere_auto);
} else {
- if (!no_commit)
- res = run_git_commit(defmsg);
+ if (!opts->no_commit)
+ res = run_git_commit(defmsg, opts);
}
free_message(&msg);
@@ -509,18 +528,18 @@ static int do_pick_commit(struct commit *commit)
return res;
}
-static void prepare_revs(struct rev_info *revs)
+static void prepare_revs(struct rev_info *revs, struct replay_opts *opts)
{
int argc;
init_revisions(revs, NULL);
revs->no_walk = 1;
- if (action != REVERT)
+ if (opts->action != REVERT)
revs->reverse = 1;
- argc = setup_revisions(commit_argc, commit_argv, revs, NULL);
+ argc = setup_revisions(opts->commit_argc, opts->commit_argv, revs, NULL);
if (argc > 1)
- usage(*revert_or_cherry_pick_usage());
+ usage(*revert_or_cherry_pick_usage(opts));
if (prepare_revision_walk(revs))
die(_("revision walk setup failed"));
@@ -529,48 +548,48 @@ static void prepare_revs(struct rev_info *revs)
die(_("empty commit set passed"));
}
-static void read_and_refresh_cache(const char *me)
+static void read_and_refresh_cache(struct replay_opts *opts)
{
static struct lock_file index_lock;
int index_fd = hold_locked_index(&index_lock, 0);
if (read_index_preload(&the_index, NULL) < 0)
- die(_("git %s: failed to read the index"), me);
+ die(_("git %s: failed to read the index"), action_name(opts));
refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);
if (the_index.cache_changed) {
if (write_index(&the_index, index_fd) ||
commit_locked_index(&index_lock))
- die(_("git %s: failed to refresh the index"), me);
+ die(_("git %s: failed to refresh the index"), action_name(opts));
}
rollback_lock_file(&index_lock);
}
-static int revert_or_cherry_pick(int argc, const char **argv)
+static int revert_or_cherry_pick(int argc, const char **argv,
+ struct replay_opts *opts)
{
struct rev_info revs;
struct commit *commit;
git_config(git_default_config, NULL);
- me = action == REVERT ? "revert" : "cherry-pick";
- setenv(GIT_REFLOG_ACTION, me, 0);
- parse_args(argc, argv);
+ setenv(GIT_REFLOG_ACTION, action_name(opts), 0);
+ parse_args(argc, argv, opts);
- if (allow_ff) {
- if (signoff)
+ if (opts->allow_ff) {
+ if (opts->signoff)
die(_("cherry-pick --ff cannot be used with --signoff"));
- if (no_commit)
+ if (opts->no_commit)
die(_("cherry-pick --ff cannot be used with --no-commit"));
- if (record_origin)
+ if (opts->record_origin)
die(_("cherry-pick --ff cannot be used with -x"));
- if (edit)
+ if (opts->edit)
die(_("cherry-pick --ff cannot be used with --edit"));
}
- read_and_refresh_cache(me);
+ read_and_refresh_cache(opts);
- prepare_revs(&revs);
+ prepare_revs(&revs, opts);
while ((commit = get_revision(&revs))) {
- int res = do_pick_commit(commit);
+ int res = do_pick_commit(commit, opts);
if (res)
return res;
}
@@ -580,14 +599,20 @@ static int revert_or_cherry_pick(int argc, const char **argv)
int cmd_revert(int argc, const char **argv, const char *prefix)
{
+ struct replay_opts opts;
+
+ memset(&opts, 0, sizeof(opts));
if (isatty(0))
- edit = 1;
- action = REVERT;
- return revert_or_cherry_pick(argc, argv);
+ opts.edit = 1;
+ opts.action = REVERT;
+ return revert_or_cherry_pick(argc, argv, &opts);
}
int cmd_cherry_pick(int argc, const char **argv, const char *prefix)
{
- action = CHERRY_PICK;
- return revert_or_cherry_pick(argc, argv);
+ struct replay_opts opts;
+
+ memset(&opts, 0, sizeof(opts));
+ opts.action = CHERRY_PICK;
+ return revert_or_cherry_pick(argc, argv, &opts);
}
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 06/18] revert: Eliminate global "commit" variable
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Functions which act on commits currently rely on a file-scope static
variable to be set before they're called. Consequently, the API and
corresponding callsites are ugly and unclear. Remove this variable
and change their API to accept the commit to act on as additional
argument so that the callsites change from looking like
commit = prepare_a_commit();
act_on_commit();
to looking like
commit = prepare_a_commit();
act_on_commit(commit);
This change is also in line with our long-term goal of exposing some
of these functions through a public API.
Inspired-by: Christian Couder <chriscool@tuxfamily.org>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/revert.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 794c050..d6c9f1d 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -37,7 +37,6 @@ static const char * const cherry_pick_usage[] = {
static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
static enum { REVERT, CHERRY_PICK } action;
-static struct commit *commit;
static int commit_argc;
static const char **commit_argv;
static int allow_rerere_auto;
@@ -116,25 +115,25 @@ struct commit_message {
const char *message;
};
-static int get_message(const char *raw_message, struct commit_message *out)
+static int get_message(struct commit *commit, struct commit_message *out)
{
const char *encoding;
const char *abbrev, *subject;
int abbrev_len, subject_len;
char *q;
- if (!raw_message)
+ if (!commit->buffer)
return -1;
- encoding = get_encoding(raw_message);
+ encoding = get_encoding(commit->buffer);
if (!encoding)
encoding = "UTF-8";
if (!git_commit_encoding)
git_commit_encoding = "UTF-8";
out->reencoded_message = NULL;
- out->message = raw_message;
+ out->message = commit->buffer;
if (strcmp(encoding, git_commit_encoding))
- out->reencoded_message = reencode_string(raw_message,
+ out->reencoded_message = reencode_string(commit->buffer,
git_commit_encoding, encoding);
if (out->reencoded_message)
out->message = out->reencoded_message;
@@ -182,7 +181,7 @@ static char *get_encoding(const char *message)
return NULL;
}
-static void write_cherry_pick_head(void)
+static void write_cherry_pick_head(struct commit *commit)
{
int fd;
struct strbuf buf = STRBUF_INIT;
@@ -355,7 +354,7 @@ static int run_git_commit(const char *defmsg)
return run_command_v_opt(args, RUN_GIT_CMD);
}
-static int do_pick_commit(void)
+static int do_pick_commit(struct commit *commit)
{
unsigned char head[20];
struct commit *base, *next, *parent;
@@ -417,7 +416,7 @@ static int do_pick_commit(void)
die(_("%s: cannot parse parent commit %s"),
me, sha1_to_hex(parent->object.sha1));
- if (get_message(commit->buffer, &msg) != 0)
+ if (get_message(commit, &msg) != 0)
die(_("Cannot get commit message for %s"),
sha1_to_hex(commit->object.sha1));
@@ -470,7 +469,7 @@ static int do_pick_commit(void)
strbuf_addstr(&msgbuf, ")\n");
}
if (!no_commit)
- write_cherry_pick_head();
+ write_cherry_pick_head(commit);
}
if (!strategy || !strcmp(strategy, "recursive") || action == REVERT) {
@@ -548,6 +547,7 @@ static void read_and_refresh_cache(const char *me)
static int revert_or_cherry_pick(int argc, const char **argv)
{
struct rev_info revs;
+ struct commit *commit;
git_config(git_default_config, NULL);
me = action == REVERT ? "revert" : "cherry-pick";
@@ -570,7 +570,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
prepare_revs(&revs);
while ((commit = get_revision(&revs))) {
- int res = do_pick_commit();
+ int res = do_pick_commit(commit);
if (res)
return res;
}
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 05/18] revert: Rename no_replay to record_origin
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
The "-x" command-line option is used to record the name of the
original commits being picked in the commit message. The variable
corresponding to this option is named "no_replay" for historical
reasons; the name is especially confusing because the term "replay" is
used to describe what cherry-pick does (for example, in the
documentation of the "--mainline" option). So, give the variable
corresponding to the "-x" command-line option a better name:
"record_origin".
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 30b39c0..794c050 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -35,7 +35,7 @@ static const char * const cherry_pick_usage[] = {
NULL
};
-static int edit, no_replay, no_commit, mainline, signoff, allow_ff;
+static int edit, record_origin, no_commit, mainline, signoff, allow_ff;
static enum { REVERT, CHERRY_PICK } action;
static struct commit *commit;
static int commit_argc;
@@ -91,7 +91,7 @@ static void parse_args(int argc, const char **argv)
if (action == CHERRY_PICK) {
struct option cp_extra[] = {
- OPT_BOOLEAN('x', NULL, &no_replay, "append commit name"),
+ OPT_BOOLEAN('x', NULL, &record_origin, "append commit name"),
OPT_BOOLEAN(0, "ff", &allow_ff, "allow fast-forward"),
OPT_END(),
};
@@ -464,7 +464,7 @@ static int do_pick_commit(void)
strbuf_addstr(&msgbuf, p);
}
- if (no_replay) {
+ if (record_origin) {
strbuf_addstr(&msgbuf, "(cherry picked from commit ");
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
strbuf_addstr(&msgbuf, ")\n");
@@ -559,7 +559,7 @@ static int revert_or_cherry_pick(int argc, const char **argv)
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)
+ if (record_origin)
die(_("cherry-pick --ff cannot be used with -x"));
if (edit)
die(_("cherry-pick --ff cannot be used with --edit"));
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 04/18] revert: Don't check lone argument in get_encoding
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
The only place get_encoding uses the global "commit" variable is when
writing an error message explaining that its lone argument was NULL.
Since the function's only caller ensures that a NULL argument isn't
passed, we can remove this check with two beneficial consequences:
1. Since the function doesn't use the global "commit" variable any
more, it won't need to change when we eliminate the global variable
later in the series.
2. Translators no longer need to localize an error message that will
never be shown.
Suggested-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 7dfe295..30b39c0 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -167,9 +167,6 @@ 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));
while (*p && *p != '\n') {
for (eol = p + 1; *eol && *eol != '\n'; eol++)
; /* do nothing */
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 03/18] revert: Simplify and inline add_message_to_msg
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
The add_message_to_msg function has some dead code, an unclear API,
only one callsite. While it originally intended fill up an empty
commit message with the commit object name while picking, it really
doesn't do this -- a bug introduced in v1.5.1-rc1~65^2~2 (Make
git-revert & git-cherry-pick a builtin, 2007-03-01). Today, tests in
t3505-cherry-pick-empty.sh indicate that not filling up an empty
commit message is the desired behavior. Re-implement and inline the
function accordingly, with a beneficial side-effect: don't dereference
a NULL pointer when the commit doesn't have a delimeter after the
header.
Helped-by: Junio C Hamano <gitster@pobox.com>
Mentored-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
builtin/revert.c | 28 ++++++++++++++--------------
1 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/builtin/revert.c b/builtin/revert.c
index 2df3f3b..7dfe295 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -185,19 +185,6 @@ static char *get_encoding(const char *message)
return NULL;
}
-static void add_message_to_msg(struct strbuf *msgbuf, const char *message)
-{
- const char *p = message;
- while (*p && (*p != '\n' || p[1] != '\n'))
- p++;
-
- if (!*p)
- strbuf_addstr(msgbuf, sha1_to_hex(commit->object.sha1));
-
- p += 2;
- strbuf_addstr(msgbuf, p);
-}
-
static void write_cherry_pick_head(void)
{
int fd;
@@ -462,11 +449,24 @@ static int do_pick_commit(void)
}
strbuf_addstr(&msgbuf, ".\n");
} else {
+ const char *p;
+
base = parent;
base_label = msg.parent_label;
next = commit;
next_label = msg.label;
- add_message_to_msg(&msgbuf, msg.message);
+
+ /*
+ * Append the commit log message to msgbuf; it starts
+ * after the tree, parent, author, committer
+ * information followed by "\n\n".
+ */
+ p = strstr(msg.message, "\n\n");
+ if (p) {
+ p += 2;
+ strbuf_addstr(&msgbuf, p);
+ }
+
if (no_replay) {
strbuf_addstr(&msgbuf, "(cherry picked from commit ");
strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 01/18] advice: Introduce error_resolve_conflict
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Enable future callers to report a conflict and not die immediately by
introducing a new function called error_resolve_conflict.
Re-implement die_resolve_conflict as a call to error_resolve_conflict
followed by a call to die. Consequently, the message printed by
die_resolve_conflict changes from
fatal: 'commit' is not possible because you have unmerged files.
Please, fix them up in the work tree ...
...
to
error: 'commit' is not possible because you have unmerged files.
hint: Fix them up in the work tree ...
hint: ...
fatal: Exiting because of an unresolved conflict.
Hints are printed using the same advise function introduced in
v1.7.3-rc0~26^2~3 (Introduce advise() to print hints, 2010-08-11).
Inspired-by: Christian Couder <chistian.couder@gmail.com>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
advice.c | 31 ++++++++++++++++++++++++-------
advice.h | 3 ++-
builtin/revert.c | 9 ---------
3 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/advice.c b/advice.c
index 0be4b5f..e02e632 100644
--- a/advice.c
+++ b/advice.c
@@ -19,6 +19,15 @@ static struct {
{ "detachedhead", &advice_detached_head },
};
+void advise(const char *advice, ...)
+{
+ va_list params;
+
+ va_start(params, advice);
+ vreportf("hint: ", advice, params);
+ va_end(params);
+}
+
int git_default_advice_config(const char *var, const char *value)
{
const char *k = skip_prefix(var, "advice.");
@@ -34,16 +43,24 @@ int git_default_advice_config(const char *var, const char *value)
return 0;
}
-void NORETURN die_resolve_conflict(const char *me)
+int error_resolve_conflict(const char *me)
{
- if (advice_resolve_conflict)
+ error("'%s' is not possible because you have unmerged files.", me);
+ if (advice_resolve_conflict) {
/*
* Message used both when 'git commit' fails and when
* other commands doing a merge do.
*/
- die("'%s' is not possible because you have unmerged files.\n"
- "Please, fix them up in the work tree, and then use 'git add/rm <file>' as\n"
- "appropriate to mark resolution and make a commit, or use 'git commit -a'.", me);
- else
- die("'%s' is not possible because you have unmerged files.", me);
+ advise("Fix them up in the work tree,");
+ advise("and then use 'git add/rm <file>' as");
+ advise("appropriate to mark resolution and make a commit,");
+ advise("or use 'git commit -a'.");
+ }
+ return -1;
+}
+
+void NORETURN die_resolve_conflict(const char *me)
+{
+ error_resolve_conflict(me);
+ die("Exiting because of an unresolved conflict.");
}
diff --git a/advice.h b/advice.h
index 3244ebb..e5d0af7 100644
--- a/advice.h
+++ b/advice.h
@@ -11,7 +11,8 @@ extern int advice_implicit_identity;
extern int advice_detached_head;
int git_default_advice_config(const char *var, const char *value);
-
+void advise(const char *advice, ...);
+int error_resolve_conflict(const char *me);
extern void NORETURN die_resolve_conflict(const char *me);
#endif /* ADVICE_H */
diff --git a/builtin/revert.c b/builtin/revert.c
index 1f27c63..2df3f3b 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -214,15 +214,6 @@ static void write_cherry_pick_head(void)
strbuf_release(&buf);
}
-static void advise(const char *advice, ...)
-{
- va_list params;
-
- va_start(params, advice);
- vreportf("hint: ", advice, params);
- va_end(params);
-}
-
static void print_advice(void)
{
char *msg = getenv("GIT_CHERRY_PICK_HELP");
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH 02/18] config: Introduce functions to write non-standard file
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
In-Reply-To: <1312222025-28453-1-git-send-email-artagnon@gmail.com>
Introduce two new functions corresponding to "git_config_set" and
"git_config_set_multivar" to write a non-standard configuration file.
Expose these new functions in cache.h for other git programs to use.
Helped-by: Jeff King <peff@peff.net>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
cache.h | 2 ++
config.c | 36 +++++++++++++++++++++++++++---------
2 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/cache.h b/cache.h
index 9e12d55..33d3428 100644
--- a/cache.h
+++ b/cache.h
@@ -1069,9 +1069,11 @@ extern int git_config_bool(const char *, const char *);
extern int git_config_maybe_bool(const char *, const char *);
extern int git_config_string(const char **, const char *, const char *);
extern int git_config_pathname(const char **, const char *, const char *);
+extern int git_config_set_in_file(const char *, const char *, const char *);
extern int git_config_set(const char *, const char *);
extern int git_config_parse_key(const char *, char **, int *);
extern int git_config_set_multivar(const char *, const char *, const char *, int);
+extern int git_config_set_multivar_in_file(const char *, const char *, const char *, const char *, int);
extern int git_config_rename_section(const char *, const char *);
extern const char *git_etc_gitconfig(void);
extern int check_repository_format_version(const char *var, const char *value, void *cb);
diff --git a/config.c b/config.c
index e42c59b..9e8580b 100644
--- a/config.c
+++ b/config.c
@@ -1092,6 +1092,12 @@ contline:
return offset;
}
+int git_config_set_in_file(const char *config_filename,
+ const char *key, const char *value)
+{
+ return git_config_set_multivar_in_file(config_filename, key, value, NULL, 0);
+}
+
int git_config_set(const char *key, const char *value)
{
return git_config_set_multivar(key, value, NULL, 0);
@@ -1189,19 +1195,14 @@ out_free_ret_1:
* - the config file is removed and the lock file rename()d to it.
*
*/
-int git_config_set_multivar(const char *key, const char *value,
- const char *value_regex, int multi_replace)
+int git_config_set_multivar_in_file(const char *config_filename,
+ const char *key, const char *value,
+ const char *value_regex, int multi_replace)
{
int fd = -1, in_fd;
int ret;
- char *config_filename;
struct lock_file *lock = NULL;
- if (config_exclusive_filename)
- config_filename = xstrdup(config_exclusive_filename);
- else
- config_filename = git_pathdup("config");
-
/* parse-key returns negative; flip the sign to feed exit(3) */
ret = 0 - git_config_parse_key(key, &store.key, &store.baselen);
if (ret)
@@ -1378,7 +1379,6 @@ int git_config_set_multivar(const char *key, const char *value,
out_free:
if (lock)
rollback_lock_file(lock);
- free(config_filename);
return ret;
write_err_out:
@@ -1387,6 +1387,24 @@ write_err_out:
}
+int git_config_set_multivar(const char *key, const char *value,
+ const char *value_regex, int multi_replace)
+{
+ const char *config_filename;
+ char *buf = NULL;
+ int ret;
+
+ if (config_exclusive_filename)
+ config_filename = config_exclusive_filename;
+ else
+ config_filename = buf = git_pathdup("config");
+
+ ret = git_config_set_multivar_in_file(config_filename, key, value,
+ value_regex, multi_replace);
+ free(buf);
+ return ret;
+}
+
static int section_name_match (const char *buf, const char *name)
{
int i = 0, j = 0, dot = 0;
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply related
* [PATCH v5 00/18] Sequencer for inclusion
From: Ramkumar Ramachandra @ 2011-08-01 18:06 UTC (permalink / raw)
To: Junio C Hamano
Cc: Git List, Jonathan Nieder, Christian Couder, Daniel Barkalow,
Jeff King
Hi,
This is yet another iteration, with few improvements since last time.
Some minor nits pointed out by Jonathan and Christian are now fixed.
One additional detail: while playing with my generalized sequencer, I
noticed a couple of embarrassing bugs in the tests -- "malformed
instruction sheet 1" and "malformed instruction sheet 2" were picking
"base..picked" instead of "base..anotherpick". Fixed now.
Thanks.
-- Ram
Ramkumar Ramachandra (18):
advice: Introduce error_resolve_conflict
config: Introduce functions to write non-standard file
revert: Simplify and inline add_message_to_msg
revert: Don't check lone argument in get_encoding
revert: Rename no_replay to record_origin
revert: Eliminate global "commit" variable
revert: Introduce struct to keep command-line options
revert: Separate cmdline parsing from functional code
revert: Don't create invalid replay_opts in parse_args
revert: Save data for continuing after conflict resolution
revert: Save command-line options for continuing operation
revert: Make pick_commits functionally act on a commit list
revert: Introduce --reset to remove sequencer state
reset: Make reset remove the sequencer state
revert: Remove sequencer state when no commits are pending
revert: Don't implicitly stomp pending sequencer operation
revert: Introduce --continue to continue the operation
revert: Propagate errors upwards from do_pick_commit
Documentation/git-cherry-pick.txt | 6 +
Documentation/git-revert.txt | 6 +
Documentation/sequencer.txt | 9 +
Makefile | 2 +
advice.c | 31 ++-
advice.h | 3 +-
branch.c | 2 +
builtin/revert.c | 735 +++++++++++++++++++++++++++++--------
cache.h | 2 +
config.c | 36 ++-
sequencer.c | 19 +
sequencer.h | 20 +
t/7106-reset-sequence.sh | 44 +++
t/t3510-cherry-pick-sequence.sh | 214 +++++++++++
14 files changed, 958 insertions(+), 171 deletions(-)
create mode 100644 Documentation/sequencer.txt
create mode 100644 sequencer.c
create mode 100644 sequencer.h
create mode 100755 t/7106-reset-sequence.sh
create mode 100755 t/t3510-cherry-pick-sequence.sh
--
1.7.4.rc1.7.g2cf08.dirty
^ permalink raw reply
* Re: [PATCH] For Real - Fixed pluralization in diff reports
From: Jeff King @ 2011-08-01 18:06 UTC (permalink / raw)
To: Jon Forrest; +Cc: git, gitster
In-Reply-To: <4E36B8E4.5080900@gmail.com>
On Mon, Aug 01, 2011 at 07:32:04AM -0700, Jon Forrest wrote:
> On 8/1/2011 2:58 AM, Sverre Rabbelier wrote:
> >Heya,
> >
> >On Mon, Aug 1, 2011 at 06:46, Jon Forrest<nobozo@gmail.com> wrote:
> >> fprintf(options->file, "%s", line_prefix);
> >> fprintf(options->file,
> >>- " %d files changed, %d insertions(+), %d deletions(-)\n",
> >>- total_files, adds, dels);
> >>+ " %d file%s changed, %d insertion%s(+), %d deletion%s(-)\n",
> >>+ total_files, total_files == 1 ? "" : "s", adds, adds == 1 ?
> >>"" : "s", dels,
> >>+ dels == 1 ? "" : "s");
> >> }
> >
> >Also, this is rather detrimental to the i18n effort methinks?
>
> If the goal if the i18n effort is also to produce grammatically
> correct output in all the supported languages then the
> tests that my patch would break would have to be rewritten
> anyway.
I think he means that auto-pluralization like this cannot be done in an
i18n world, as many languages do not simply add "s". Your patch would
have to use ngettext, something like this (totally untested and just
copying a similar spot in suggest_reattach, as I have never done any
i18n myself):
fprintf(options->file,
Q_(" %d file changed,",
" %d files changed",
total_files),
Q_(" %d insertion(+)",
" %d insertions(+)",
adds),
Q_(" %d deletion(-)",
" %d deletions(-)",
dels),
"\n",
total_files, adds, dels);
And that gives translators a chance to specify the entire singular and
plural versions separately.
-Peff
^ permalink raw reply
* [PATCH] test ls-files with relative paths
From: Clemens Buchacher @ 2011-08-01 18:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael J Gruber, git, rrt, john
In-Reply-To: <7vipqiknjh.fsf@alter.siamese.dyndns.org>
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Sun, Jul 31, 2011 at 05:01:22PM -0700, Junio C Hamano wrote:
>
> Sounds sane; tests?
Certainly. I'm not testing partial commit since it uses the same
codepath as ls-files --error-unmatch anyways.
t/t3005-ls-files-relative.sh | 74 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 74 insertions(+), 0 deletions(-)
create mode 100755 t/t3005-ls-files-relative.sh
diff --git a/t/t3005-ls-files-relative.sh b/t/t3005-ls-files-relative.sh
new file mode 100755
index 0000000..e9a2f75
--- /dev/null
+++ b/t/t3005-ls-files-relative.sh
@@ -0,0 +1,74 @@
+#!/bin/sh
+
+test_description='ls-files tests with relative paths
+
+This test runs git ls-files with various relative path arguments.
+'
+
+. ./test-lib.sh
+
+new_line='
+'
+sq=\'
+
+test_expect_success 'prepare' '
+ : >never-mind-me &&
+ git add never-mind-me &&
+ mkdir top &&
+ (
+ cd top &&
+ mkdir sub &&
+ x="x xa xbc xdef xghij xklmno" &&
+ y=$(echo "$x" | tr x y) &&
+ touch $x &&
+ touch $y &&
+ cd sub &&
+ git add ../x*
+ )
+'
+
+test_expect_success 'ls-files with mixed levels' '
+ (
+ cd top/sub &&
+ f=$(cat <<-EOF
+ ../../never-mind-me
+ ../x
+ EOF
+ ) &&
+ echo "$f" >expect &&
+ git ls-files $f >actual &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'ls-files -c' '
+ (
+ cd top/sub &&
+ for f in ../y*
+ do
+ echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+ done >expect &&
+ echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+ set ../x* &&
+ IFS="$new_line" && echo "$*" >>expect && unset IFS &&
+ (git ls-files -c --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+ test_cmp expect actual
+ )
+'
+
+test_expect_success 'ls-files -o' '
+ (
+ cd top/sub &&
+ for f in ../x*
+ do
+ echo "error: pathspec ${sq}${f}${sq} did not match any file(s) known to git."
+ done >expect &&
+ echo "Did you forget to ${sq}git add${sq}?" >>expect &&
+ set ../y* &&
+ IFS="$new_line" && echo "$*" >>expect && unset IFS &&
+ (git ls-files -o --error-unmatch ../[xy]* || true) >actual 2>&1 &&
+ test_cmp expect actual
+ )
+'
+
+test_done
--
1.7.3.1.105.g84915
^ permalink raw reply related
* Re: [PATCH v2 4/4] upload-archive: use start_command instead of fork
From: Erik Faye-Lund @ 2011-08-01 18:02 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, j6t
In-Reply-To: <20110801174603.GB10302@sigill.intra.peff.net>
On Mon, Aug 1, 2011 at 7:46 PM, Jeff King <peff@peff.net> wrote:
> On Mon, Aug 01, 2011 at 04:45:03PM +0200, Erik Faye-Lund wrote:
>
>> > And that patch would look something like the one below.
>>
>> This is the most straight forward way of doing this, thanks. I'll post
>> a new version with this squashed in soon.
>>
>> What's the proper way of attributing you for the work?
>
> If you're squashing, just make a note in the text, or add a Helped-by
> header at the end.
>
OK, that's basically what I did already ;)
>> > You can also now
>> > drop the "remote" parameter to write_archive entirely, but I didn't do
>> > so here.
>>
>> I'm not entirely sure how you propose we do this... do you mean by
>> hoisting the relevant logic from write_archive up to cmd_archive or
>> something?
>
> Sorry, I wrote that comment, then totally changed how my patch was
> implemented and failed to update the comment. :)
>
> The arguments to git-archive actually get parsed in two places:
>
> 1. cmd_archive passes them to decide on meta stuff like remote vs
> local
>
> 2. write_archive has a parse_archive_args function to actually parse
> the arguments that both upload-archive and archive share
>
> My patch puts the new argument in (1). We could put it in (2) and do
> away with passing the flag all the way down the call stack. But it seems
> conceptually cleaner to me to have it in (1) (and the diff is much
> shorter, too).
>
Makes sense. Thanks for the explanation, I agree.
>> $ make t5000-tar-tree
>> *** t5000-tar-tree.sh ***
>> ok 1 - populate workdir
>> <snip>
>> ok 53 - infer tgz from .tar.gz filename
>> not ok - 54 extract tgz file
>> #
>> # $GUNZIP -c <j.tgz >j.tar &&
>> # test_cmp b.tar j.tar
>> #
>> not ok - 55 remote tar.gz is allowed by default
>> #
>> # git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&
>> # test_cmp j.tgz remote.tar.gz
>> #
>> ok 56 - remote tar.gz can be disabled
>> # failed 2 among 56 test(s)
>> 1..56
>> make: *** [t5000-tar-tree.sh] Error 1
>>
>> It seems "git archive --format=tgz HEAD" is broken on Windows:
>> $ git archive --format=tgz HEAD > j.tgz
>> $ gunzip -c j.tgz > j.tar && echo ok
>>
>> gzip: j.tgz: invalid compressed data--format violated
>>
>> But if I perform the compression manually, the archive is fine:
>> $ git archive --format=tar HEAD | gzip -cn > j.tgz
>> $ gunzip -c j.tgz > j.tar && echo ok
>> ok
>
> Weird. What does j.tgz end up looking like? Is it empty, or does it have
> bogus data in it? Does gzip actually get invoked at all? Try running
> with GIT_TRACE=1. I don't suppose you guys have something like strace,
> which might be helpful.
>
It does have data, and gzip does get invoked:
$ gunzip -c j.tgz | wc -c
gzip: j.tgz: invalid compressed data--format violated
131072
So it seems there are around 130k of valid data before it barfs.
Hmm, but when I try the same after re-running the test, I get a
different amount of valid data (491520 bytes this time)... Is this a
timing-related issue?
Yeah, strace would have been useful, but we don't have that on
Windows. There are probably other alternatives, but I'm not aware...
perhaps procmon can be of help?
>> So, the big question is, are all tar-filters broken on Windows? It
>> seems not; the tests for the foo-filter works fine... any clues? Could
>> it somehow be newline-related, perhaps?
>
> I'm not sure that newlines would make a difference. We use straight
> write() everywhere in the archive code, which should be binary safe.
>
OK, so that hunch was a bad one ;)
^ permalink raw reply
* Re: Possibly solved invalid free() in git-remote-http from Git 1.7.2.1
From: Jeff King @ 2011-08-01 18:00 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: Git Mailing List, Tay Ray Chuan
In-Reply-To: <CACBZZX5N0DaSrU6rxW=PTMQ8b6c_sxMFJQHMaZy1L138eFFo6w@mail.gmail.com>
On Mon, Aug 01, 2011 at 05:42:54PM +0200, Ævar Arnfjörð Bjarmason wrote:
> On Mon, Aug 1, 2011 at 17:39, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> > On Mon, Aug 1, 2011 at 17:22, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >> In case anyone would like to look into this I had this undiagnosed
> >> free() error from git-remote-http in git version 1.7.2.1:
> >
> > Actually this is still an issue in master, CC-ing Tay who introduced
> > this error.
> >
> > This evil hack would get around it most of the time, but would
> > introduce another race condition:
>
> *brainfart*. This would be better:
>
> diff --git a/http.c b/http.c
> index a1ea3db..c5da23a 100644
> --- a/http.c
> +++ b/http.c
> @@ -1298,7 +1298,7 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
> if (ftruncate(freq->localfile, 0) < 0) {
> error("Couldn't truncate temporary
> file %s: %s",
> freq->tmpfile, strerror(errno));
> - goto abort;
> + goto abort_url;
> }
> }
> }
> @@ -1328,9 +1328,10 @@ struct http_object_request
> *new_http_object_request(const char *base_url,
>
> return freq;
>
> +abort_url:
> + free(freq->url);
> abort:
> free(filename);
> - free(freq->url);
> free(freq);
> return NULL;
Hmm. The memory management in the function seems weird to me.
Why do we free filename only on abort? Are we leaking it otherwise? Even
weirder, AFAICT, filename can only ever point to the static buffer
returned by sha1_file_name. So it should _never_ be freed. And I'm
surprised glibc's free() checker isn't complaining about that. Am I
reading it wrong?
With respect to freq->url, from your patch I expected that some code
paths would allocate it on the heap, and some would not. But it looks
like there is only one spot, and the problem is that freq is filled with
uninitialized cruft. Wouldn't it be simpler to just s/xmalloc/xcalloc/,
and then you know all of the pointers are initialized properly to NULL?
You can also get rid of the memset-to-zero later in the function.
-Peff
^ permalink raw reply
* [PATCH] notice error exit from pager
From: Clemens Buchacher @ 2011-08-01 17:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20110726210401.GA25207@toss.lan>
If the pager fails to run, git produces no output, e.g.:
$ GIT_PAGER=not-a-command git log
The error reporting fails for two reasons:
(1) start_command: There is a mechanism that detects errors during
execvp introduced in 2b541bf8 (start_command: detect execvp
failures early). The child writes one byte to a pipe only if
execvp fails. The parent waits for either EOF, when the
successful execvp automatically closes the pipe (see
FD_CLOEXEC in fcntl(1)), or it reads a single byte, in which
case it knows that the execvp failed. This mechanism is
incompatible with the workaround introduced in 35ce8622
(pager: Work around window resizing bug in 'less'), which
waits for input from the parent before the exec. Since both
the parent and the child are waiting for input from each
other, that would result in a deadlock. In order to avoid
that, the mechanism is disabled by closing the child_notifier
file descriptor.
(2) finish_command: The parent correctly detects the 127 exit
status from the child, but the error output goes nowhere,
since by that time it is already being redirected to the
child.
No simple solution for (1) comes to mind.
Number (2) can be solved by not sending error output to the pager.
Not redirecting error output to the pager can result in the pager
overwriting error output with standard output, however.
Since there is no reliable way to handle error reporting in the
parent, produce the output in the child instead.
Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
Nothing new compared to the RFC, just a slightly trimmed commit
message.
run-command.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/run-command.c b/run-command.c
index 5c91f37..a2796c4 100644
--- a/run-command.c
+++ b/run-command.c
@@ -125,9 +125,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
if (code == 127) {
code = -1;
failed_errno = ENOENT;
- if (!silent_exec_failure)
- error("cannot run %s: %s", argv0,
- strerror(ENOENT));
}
} else {
error("waitpid is confused (%s)", argv0);
@@ -282,14 +279,14 @@ fail_pipe:
} else {
execvp(cmd->argv[0], (char *const*) cmd->argv);
}
- /*
- * Do not check for cmd->silent_exec_failure; the parent
- * process will check it when it sees this exit code.
- */
- if (errno == ENOENT)
+ if (errno == ENOENT) {
+ if (!cmd->silent_exec_failure)
+ error("cannot run %s: %s", cmd->argv[0],
+ strerror(ENOENT));
exit(127);
- else
+ } else {
die_errno("cannot exec '%s'", cmd->argv[0]);
+ }
}
if (cmd->pid < 0)
error("cannot fork() for %s: %s", cmd->argv[0],
--
1.7.3.1.105.g84915
^ permalink raw reply related
* Re: [PATCH 11/18] revert: Save command-line options for continuing operation
From: Ramkumar Ramachandra @ 2011-08-01 17:55 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, Git List, Jonathan Nieder, Daniel Barkalow,
Jeff King
In-Reply-To: <201107311342.12547.chriscool@tuxfamily.org>
Hi,
Christian Couder writes:
> On Thursday 28 July 2011 18:52:24 Ramkumar Ramachandra wrote:
>> + if (opts->mainline) {
>> + strbuf_reset(&buf);
>
> It is not necessary to reset &buf here.
>
>> + strbuf_addf(&buf, "%d", opts->mainline);
>> + git_config_set_in_file(opts_file, "options.mainline", buf.buf);
>> + }
>
> And perhaps it would be clearer if it was:
>
> + if (opts->mainline) {
> + struct strbuf buf = STRBUF_INIT;
> + strbuf_addf(&buf, "%d", opts->mainline);
> + git_config_set_in_file(opts_file, "options.mainline", buf.buf);
> + strbuf_release(&buf);
> + }
Much clearer. Fixed.
>> + if (opts->strategy)
>> + git_config_set_in_file(opts_file, "options.strategy", opts->strategy);
>> + if (opts->xopts) {
>
> Other nit: maybe you could put "int i" here, instead of at the beginning of
> the function.
Fixed. Thanks.
-- Ram
^ permalink raw reply
* Re: [PATCH 13/18] revert: Introduce --reset to remove sequencer state
From: Ramkumar Ramachandra @ 2011-08-01 17:52 UTC (permalink / raw)
To: Christian Couder
Cc: Junio C Hamano, Git List, Jonathan Nieder, Daniel Barkalow,
Jeff King
In-Reply-To: <201107311601.43472.chriscool@tuxfamily.org>
Hi Christian,
Christian Couder writes:
> On Thursday 28 July 2011 18:52:26 Ramkumar Ramachandra wrote:
>> @@ -765,17 +784,21 @@ static int pick_revisions(struct replay_opts *opts)
>>
>> read_and_refresh_cache(opts);
>>
>> - walk_revs_populate_todo(&todo_list, opts);
>> - create_seq_dir();
>> - if (get_sha1("HEAD", sha1)) {
>> - if (opts->action == REVERT)
>> - die(_("Can't revert as initial commit"));
>> - die(_("Can't cherry-pick into empty head"));
>> - } else
>> + if (opts->subcommand == REPLAY_RESET) {
>> + remove_sequencer_state(1);
>> + return 0;
>> + } else {
>
> Maybe you could remove this "else" so that the indentation level is reduced.
This was intentional, mainly for stylistic reasons. See [1].
>> + /* Start a new cherry-pick/ revert sequence */
>> + walk_revs_populate_todo(&todo_list, opts);
>> + create_seq_dir();
>> + if (get_sha1("HEAD", sha1)) {
>> + if (opts->action == REVERT)
>> + die(_("Can't revert as initial commit"));
>> + die(_("Can't cherry-pick into empty head"));
>> + }
>> save_head(sha1_to_hex(sha1));
>> - save_opts(opts);
>> - save_todo(todo_list, opts);
>
> It looks like this save_todo() was not necessary in the first place because
> pick_commits() does a save_todo() before processing each commit.
Fixed in the earlier patch. Thanks!
[1]: http://thread.gmane.org/gmane.comp.version-control.git/177960
Thanks.
-- Ram
^ permalink raw reply
* BUG?: git diff with 3 args consults work tree when it shouldn't
From: Johan Herland @ 2011-08-01 17:35 UTC (permalink / raw)
To: git
Hi,
I'm viewing the combined diff of a merge commit where the correct
resolution involved the removal of a conflicting file. In short, the
(simplified) scenario is:
A
/ \
O M
\ /
B
where commit O does not contain the file, branches A and B add
conflicting versions of the file, and merge M removes the file (see
below for a full command session illustrating the scenario).
When I view the combined diff of the merge (using "git diff M A B" or
"git show M" ), it seems the resulting diff depends on the
existence/contents of the file in my *current work tree*. I.e. if I
checkout O or M (where the file is missing) before producing the diff,
the result seems correct, but if I put a garbage version of the file in
my work tree (without adding or committing it at all), the garbage shows
up in the diff, as follows:
$ git diff M A B
diff --cc file
index 34b6a0c,732c85a..0000000
--- a/file
+++ b/file
@@@ -1,1 -1,1 +1,1 @@@
- contents contributed by A
-contents contributed by B
++garbage contents from my work tree
AFAICS, when producing combined diffs, the diff code should not look up
the file in the current work tree, since the diff arguments refer to
existing commits, and do not indicate that the work tree should be
consulted at all.
...Johan
Example session illustrating the problem:
$ git init repo
Initialized empty Git repository in ./repo/.git/
$ cd repo
$ echo foo > foo
$ git add foo
$ git commit -m foo
[master (root-commit) c6d62c2] foo
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 foo
$ git checkout -b a
Switched to a new branch 'a'
$ echo spam > file
$ git add file
$ git commit -m spam
[a d2d3128] spam
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 file
$ git checkout -b b master
Switched to a new branch 'b'
$ echo eggs > file
$ git add file
$ git commit -m eggs
[b 72e8252] eggs
1 files changed, 1 insertions(+), 0 deletions(-)
create mode 100644 file
$ git checkout -b m
Switched to a new branch 'm'
$ git merge a
Auto-merging file
CONFLICT (add/add): Merge conflict in file
Automatic merge failed; fix conflicts and then commit the result.
$ git rm file
file: needs merge
rm 'file'
$ git commit -m merge
[m c096356] merge
$ git diff m a b
diff --cc file
index 34b6a0c,732c85a..0000000
deleted file mode 100644,100644
--- a/file
+++ /dev/null
$ ### HERE IS WHERE IT GOES WRONG:
$ echo xyzzy > file
$ git diff m a b
diff --cc file
index 34b6a0c,732c85a..0000000
--- a/file
+++ b/file
@@@ -1,1 -1,1 +1,1 @@@
- spam
-eggs
++xyzzy
--
Johan Herland, <johan@herland.net>
www.herland.net
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox