Re: [PATCH] cherry-pick: do not segfault without arguments.

2013-10-10 Thread Jeff King 
On Fri, Oct 04, 2013 at 04:09:12PM +0200, Stefan Beller wrote:

 Commit 182d7dc46b (2013-09-05, cherry-pick: allow - as abbreviation of
 '@{-1}') accesses the first argument without checking whether it exists.

I think this is an obviously correct fix for the immediate segfault,
but...

 diff --git a/builtin/revert.c b/builtin/revert.c
 index 52c35e7..f81a48c 100644
 --- a/builtin/revert.c
 +++ b/builtin/revert.c
 @@ -202,7 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const 
 char *prefix)
   memset(opts, 0, sizeof(opts));
   opts.action = REPLAY_PICK;
   git_config(git_default_config, NULL);
 - if (!strcmp(argv[1], -))
 + if (argc  1  !strcmp(argv[1], -))
   argv[1] = @{-1};
   parse_args(argc, argv, opts);
   res = sequencer_pick_revisions(opts);

Why are we looking at argv/argc at all before calling parse_args? We
would want to allow options to come before the -, no?

In other words, I think the right fix is more like this:

-- 8 --
Subject: [PATCH] cherry-pick: handle - after parsing options

Currently, we only try converting argv[1] from - into
@{-1}.  This means we do not notice - when used together
with an option. We can fix this by doing the substitution
after we know any remaining options must be revisions.

Since we now also come after the check that there is
something in argv to cherry-pick, this has the added bonus
of avoiding a segfault when git cherry-pick is run with no
options.

Noticed-by: Stefan Beller stefanbel...@googlemail.com
Signed-off-by: Jeff King p...@peff.net
---
I still am unsure if we should be more robust about finding - in other
options. For example, I think you can cherry-pick multiple commits these
days. Should we allow something like:

  git cherry-pick foo~2 - bar~5

Certainly the convenience of - over @{-1} is not as big a deal if
you are cherry-picking more than one item, but it seems like we should
treat it consistently.

 builtin/revert.c  |  4 ++--
 t/t3501-revert-cherry-pick.sh | 12 
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 52c35e7..87659c9 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -168,6 +168,8 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
opts-revs-no_walk = REVISION_WALK_NO_WALK_UNSORTED;
if (argc  2)
usage_with_options(usage_str, options);
+   if (!strcmp(argv[1], -))
+   argv[1] = @{-1};
memset(s_r_opt, 0, sizeof(s_r_opt));
s_r_opt.assume_dashdash = 1;
argc = setup_revisions(argc, argv, opts-revs, s_r_opt);
@@ -202,8 +204,6 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
memset(opts, 0, sizeof(opts));
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
-   if (!strcmp(argv[1], -))
-   argv[1] = @{-1};
parse_args(argc, argv, opts);
res = sequencer_pick_revisions(opts);
if (res  0)
diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index bff6ffe..51f3bbb 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -129,4 +129,16 @@ test_expect_success 'cherry-pick - is meaningless 
without checkout' '
)
 '
 
+test_expect_success 'cherry-pick - works with arguments' '
+   git checkout -b side-branch 
+   test_commit change actual change 
+   git checkout master 
+   git cherry-pick -s - 
+   echo Signed-off-by: C O Mitter commit...@example.com expect 
+   git cat-file commit HEAD | grep ^Signed-off-by: signoff 
+   test_cmp expect signoff 
+   echo change expect 
+   test_cmp expect actual
+'
+
 test_done
-- 
1.8.4.1.4.gf327177

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cherry-pick: do not segfault without arguments.

2013-10-10 Thread Stefan Beller 
On 10/10/2013 06:41 PM, Jeff King wrote:
 On Fri, Oct 04, 2013 at 04:09:12PM +0200, Stefan Beller wrote:
 
 Commit 182d7dc46b (2013-09-05, cherry-pick: allow - as abbreviation of
 '@{-1}') accesses the first argument without checking whether it exists.
 
 I think this is an obviously correct fix for the immediate segfault,
 but...

Yes my intention was to fix the obvious, as I assumed the discussion
about the exact spec for the '-' has been finished.
Your patch probably makes more sense however.

 
 diff --git a/builtin/revert.c b/builtin/revert.c
 index 52c35e7..f81a48c 100644
 --- a/builtin/revert.c
 +++ b/builtin/revert.c
 @@ -202,7 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const 
 char *prefix)
  memset(opts, 0, sizeof(opts));
  opts.action = REPLAY_PICK;
  git_config(git_default_config, NULL);
 -if (!strcmp(argv[1], -))
 +if (argc  1  !strcmp(argv[1], -))
  argv[1] = @{-1};
  parse_args(argc, argv, opts);
  res = sequencer_pick_revisions(opts);
 
 Why are we looking at argv/argc at all before calling parse_args? We
 would want to allow options to come before the -, no?
 
 In other words, I think the right fix is more like this:
 
 -- 8 --
 Subject: [PATCH] cherry-pick: handle - after parsing options
 
 Currently, we only try converting argv[1] from - into
 @{-1}.  This means we do not notice - when used together
 with an option. We can fix this by doing the substitution
 after we know any remaining options must be revisions.
 
 Since we now also come after the check that there is
 something in argv to cherry-pick, this has the added bonus
 of avoiding a segfault when git cherry-pick is run with no
 options.
 
 Noticed-by: Stefan Beller stefanbel...@googlemail.com
 Signed-off-by: Jeff King p...@peff.net
 ---
 I still am unsure if we should be more robust about finding - in other
 options. 

There was a discusssion about it recently. - would generally mean the
last thing (kind of an undo), so git checkout - would switch to the last
branch. It's similar to 'cd -'.
So I am not sure if this is generally @{-1}, or if we need to have other
references for other commands.

For example, I think you can cherry-pick multiple commits these
 days. Should we allow something like:
 
   git cherry-pick foo~2 - bar~5



 
 Certainly the convenience of - over @{-1} is not as big a deal if
 you are cherry-picking more than one item, but it seems like we should
 treat it consistently.
 
  builtin/revert.c  |  4 ++--
  t/t3501-revert-cherry-pick.sh | 12 
  2 files changed, 14 insertions(+), 2 deletions(-)
 
 diff --git a/builtin/revert.c b/builtin/revert.c
 index 52c35e7..87659c9 100644
 --- a/builtin/revert.c
 +++ b/builtin/revert.c
 @@ -168,6 +168,8 @@ static void parse_args(int argc, const char **argv, 
 struct replay_opts *opts)
   opts-revs-no_walk = REVISION_WALK_NO_WALK_UNSORTED;
   if (argc  2)
   usage_with_options(usage_str, options);
 + if (!strcmp(argv[1], -))
 + argv[1] = @{-1};
   memset(s_r_opt, 0, sizeof(s_r_opt));
   s_r_opt.assume_dashdash = 1;
   argc = setup_revisions(argc, argv, opts-revs, s_r_opt);
 @@ -202,8 +204,6 @@ int cmd_cherry_pick(int argc, const char **argv, const 
 char *prefix)
   memset(opts, 0, sizeof(opts));
   opts.action = REPLAY_PICK;
   git_config(git_default_config, NULL);
 - if (!strcmp(argv[1], -))
 - argv[1] = @{-1};
   parse_args(argc, argv, opts);
   res = sequencer_pick_revisions(opts);
   if (res  0)
 diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
 index bff6ffe..51f3bbb 100755
 --- a/t/t3501-revert-cherry-pick.sh
 +++ b/t/t3501-revert-cherry-pick.sh
 @@ -129,4 +129,16 @@ test_expect_success 'cherry-pick - is meaningless 
 without checkout' '
   )
  '
  
 +test_expect_success 'cherry-pick - works with arguments' '
 + git checkout -b side-branch 
 + test_commit change actual change 
 + git checkout master 
 + git cherry-pick -s - 
 + echo Signed-off-by: C O Mitter commit...@example.com expect 
 + git cat-file commit HEAD | grep ^Signed-off-by: signoff 
 + test_cmp expect signoff 
 + echo change expect 
 + test_cmp expect actual
 +'
 +
  test_done
 

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] cherry-pick: do not segfault without arguments.

2013-10-04 Thread Stefan Beller 
Commit 182d7dc46b (2013-09-05, cherry-pick: allow - as abbreviation of
'@{-1}') accesses the first argument without checking whether it exists.

Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
 builtin/revert.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/revert.c b/builtin/revert.c
index 52c35e7..f81a48c 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -202,7 +202,7 @@ int cmd_cherry_pick(int argc, const char **argv, const char 
*prefix)
memset(opts, 0, sizeof(opts));
opts.action = REPLAY_PICK;
git_config(git_default_config, NULL);
-   if (!strcmp(argv[1], -))
+   if (argc  1  !strcmp(argv[1], -))
argv[1] = @{-1};
parse_args(argc, argv, opts);
res = sequencer_pick_revisions(opts);
-- 
1.8.4.1.469.gb38b9db

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html