Skip to content

Commit

Permalink
revert: --reference should apply only to 'revert', not 'cherry-pick'
Browse files Browse the repository at this point in the history
As 'revert' and 'cherry-pick' share a lot of code, it is easy to
modify the behaviour of one command and inadvertently affect the
other.  An earlier change to teach the '--reference' option and the
'revert.reference' configuration variable to the former was not
careful enough and 'cherry-pick --reference' wasn't rejected as an
error.

It is possible to think 'cherry-pick -x' might benefit from the
'--reference' option, but it is fundamentally different from
'revert' in at least two ways to make it questionable:

 - 'revert' names a commit that is ancestor of the resulting commit,
   so an abbreviated object name with human readable title is
   sufficient to identify the named commit uniquely without using
   the full object name.  On the other hand, 'cherry-pick'
   usually [*] picks a commit that is not an ancestor.  It might be
   even picking a private commit that never becomes part of the
   public history.

 - The whole commit message of 'cherry-pick' is a copy of the
   original commit, and there is nothing gained to repeat only the
   title part on 'cherry-picked from' message.

[*] well, you could revert and then you can pick the original that
    was reverted to get back to where you were, but then you can
    revert the revert to do the same thing.

Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
gitster committed May 31, 2022
1 parent 43966ab commit 191faaf
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 3 deletions.
9 changes: 7 additions & 2 deletions builtin/revert.c
Expand Up @@ -116,8 +116,6 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
N_("option for merge strategy"), option_parse_x),
{ OPTION_STRING, 'S', "gpg-sign", &opts->gpg_sign, N_("key-id"),
N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
OPT_BOOL(0, "reference", &opts->commit_use_reference,
N_("use the 'reference' format to refer to commits")),
OPT_END()
};
struct option *options = base_options;
Expand All @@ -132,6 +130,13 @@ static int run_sequencer(int argc, const char **argv, struct replay_opts *opts)
OPT_END(),
};
options = parse_options_concat(options, cp_extra);
} else if (opts->action == REPLAY_REVERT) {
struct option cp_extra[] = {
OPT_BOOL(0, "reference", &opts->commit_use_reference,
N_("use the 'reference' format to refer to commits")),
OPT_END(),
};
options = parse_options_concat(options, cp_extra);
}

argc = parse_options(argc, argv, NULL, options, usage_str,
Expand Down
2 changes: 1 addition & 1 deletion sequencer.c
Expand Up @@ -221,7 +221,7 @@ static int git_sequencer_config(const char *k, const char *v, void *cb)
return ret;
}

if (!strcmp(k, "revert.reference"))
if (opts->action == REPLAY_REVERT && !strcmp(k, "revert.reference"))
opts->commit_use_reference = git_config_bool(k, v);

status = git_gpg_config(k, v, NULL);
Expand Down
6 changes: 6 additions & 0 deletions t/t3501-revert-cherry-pick.sh
Expand Up @@ -205,4 +205,10 @@ test_expect_success 'identification of reverted commit (revert.reference)' '
test_cmp expect actual
'

test_expect_success 'cherry-pick is unaware of --reference (for now)' '
test_when_finished "git reset --hard" &&
test_must_fail git cherry-pick --reference HEAD 2>actual &&
grep "^usage: git cherry-pick" actual
'

test_done

0 comments on commit 191faaf

Please sign in to comment.