| Message ID | 20230602102533.876905-13-christian.couder@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce new `git replay` command | expand |
Christian Couder <christian.couder@gmail.com> writes: > + /* > + * Reject any pathspec. (They are allowed and eaten by > + * setup_revisions() above.) In the future we might accept > + * them, after adding related tests and doc though. > + */ > + if (revs.prune_data.nr) { > + error(_("invalid pathspec: %s"), revs.prune_data.items[0].match); This made me waste a few minutes wondering if and how I misspelt my pathspec elements. If we mean "no pathspec is allowed", we should say so instead. > + usage_with_options(replay_usage, replay_options); > + } > + > /* requirements/overrides for revs */ > revs.reverse = 1; > revs.sort_order = REV_SORT_IN_GRAPH_ORDER; > diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh > index a1da4f9ef9..de6e40950e 100755 > --- a/t/t3650-replay-basics.sh > +++ b/t/t3650-replay-basics.sh > @@ -80,4 +80,20 @@ test_expect_success 'using replay on bare repo to rebase with a conflict' ' > test_expect_code 1 git -C bare replay --onto topic1 B..conflict > ' > > +test_expect_success 'using replay with (for now) disallowed revision specific option --not' ' > + test_must_fail git replay --onto main topic2 --not topic1 > +' > + > +test_expect_success 'using replay on bare repo with (for now) disallowed revision specific option --first-parent' ' > + test_must_fail git -C bare replay --onto main --first-parent topic1..topic2 > +' > + > +test_expect_success 'using replay with disallowed pathspec' ' > + test_must_fail git replay --onto main topic1..topic2 A.t > +' > + > +test_expect_success 'using replay on bare repo with disallowed pathspec' ' > + test_must_fail git -C bare replay --onto main topic1..topic2 -- A.t > +' > + > test_done
On Tue, Jul 25, 2023 at 11:16 PM Junio C Hamano <gitster@pobox.com> wrote: > > Christian Couder <christian.couder@gmail.com> writes: > > > + /* > > + * Reject any pathspec. (They are allowed and eaten by > > + * setup_revisions() above.) In the future we might accept > > + * them, after adding related tests and doc though. > > + */ > > + if (revs.prune_data.nr) { > > + error(_("invalid pathspec: %s"), revs.prune_data.items[0].match); > > This made me waste a few minutes wondering if and how I misspelt my > pathspec elements. If we mean "no pathspec is allowed", we should > say so instead. Yeah, right. I have changed this to: error(_("no pathspec is allowed: '%s'"), revs.prune_data.items[0].match); > > + usage_with_options(replay_usage, replay_options); > > + }
diff --git a/builtin/replay.c b/builtin/replay.c index c1bd72c0e5..cffbf34290 100644 --- a/builtin/replay.c +++ b/builtin/replay.c @@ -121,7 +121,7 @@ int cmd_replay(int argc, const char **argv, const char *prefix) struct commit *commit; struct merge_options merge_opt; struct merge_result result; - int ret = 0; + int ret = 0, i; const char * const replay_usage[] = { N_("git replay --onto <newbase> <revision-range>..."), @@ -137,6 +137,20 @@ int cmd_replay(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, replay_options, replay_usage, PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN_OPT); + /* + * TODO: For now, we reject any unknown or invalid option, + * including revision related ones, like --not, + * --first-parent, etc that would be allowed and eaten by + * setup_revisions() below. In the future we should definitely + * accept those that make sense and add related tests and doc + * though. + */ + for (i = 0; i < argc; i++) + if (argv[i][0] == '-') { + error(_("invalid option: %s"), argv[i]); + usage_with_options(replay_usage, replay_options); + } + if (!onto_name) { error(_("option --onto is mandatory")); usage_with_options(replay_usage, replay_options); @@ -152,6 +166,16 @@ int cmd_replay(int argc, const char **argv, const char *prefix) goto cleanup; } + /* + * Reject any pathspec. (They are allowed and eaten by + * setup_revisions() above.) In the future we might accept + * them, after adding related tests and doc though. + */ + if (revs.prune_data.nr) { + error(_("invalid pathspec: %s"), revs.prune_data.items[0].match); + usage_with_options(replay_usage, replay_options); + } + /* requirements/overrides for revs */ revs.reverse = 1; revs.sort_order = REV_SORT_IN_GRAPH_ORDER; diff --git a/t/t3650-replay-basics.sh b/t/t3650-replay-basics.sh index a1da4f9ef9..de6e40950e 100755 --- a/t/t3650-replay-basics.sh +++ b/t/t3650-replay-basics.sh @@ -80,4 +80,20 @@ test_expect_success 'using replay on bare repo to rebase with a conflict' ' test_expect_code 1 git -C bare replay --onto topic1 B..conflict ' +test_expect_success 'using replay with (for now) disallowed revision specific option --not' ' + test_must_fail git replay --onto main topic2 --not topic1 +' + +test_expect_success 'using replay on bare repo with (for now) disallowed revision specific option --first-parent' ' + test_must_fail git -C bare replay --onto main --first-parent topic1..topic2 +' + +test_expect_success 'using replay with disallowed pathspec' ' + test_must_fail git replay --onto main topic1..topic2 A.t +' + +test_expect_success 'using replay on bare repo with disallowed pathspec' ' + test_must_fail git -C bare replay --onto main topic1..topic2 -- A.t +' + test_done
A previous commit changed `git replay` to make it accept standard revision ranges using the setup_revisions() function. While this is a good thing to make this command more standard and more flexible, it has the downside of enabling many revision related options accepted and eaten by setup_revisions(). Some of these options might make sense, but others, like those generating non-contiguous history, might not. Anyway those we might want to allow should probably be tested and perhaps documented a bit, which could be done in future work. For now it is just simpler and safer to just disallow all of them, so let's do that. Other commands, like `git fast-export`, currently allow all these revision specific options even though some of them might not make sense, as these commands also use setup_revisions() but do not check the options that might be passed to this function. So a way to fix those commands as well as git replay could be to improve or refactor the setup_revisions() mechanism to let callers allow and disallow options in a relevant way for them. Such improvements are outside the scope of this work though. Pathspecs, which are also accepted and eaten by setup_revisions(), are likely to result in disconnected history. That could perhaps be useful, but that would need tests and documentation, which can be added in future work. So, while at it, let's disallow them too. Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- builtin/replay.c | 26 +++++++++++++++++++++++++- t/t3650-replay-basics.sh | 16 ++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-)