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 <stefanbeller@xxxxxxxxxxxxxx>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> 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 <committer@xxxxxxxxxxx>" >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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html