On Tue, 20 May 2025 at 11:27, Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> The support for assuming "push" when "-p" is given introduced in
> 9e140909f61 (stash: allow pathspecs in the no verb form, 2017-02-28) is
> very narrow, neither "git stash -m <message> -p <pathspec>" nor "git
> stash --patch <pathspec>" imply "push" and die instead. Relax this by
> passing PARSE_OPT_STOP_AT_NON_OPTION when push is being assumed and then
> setting "force_assume" if "--patch" was present. This means "git stash
> <pathspec> -p" still dies so that it does not assume the user meant
> "push" if they mistype a subcommand name but "git stash -m <message> -p
> <pathspec>" will now succeed. The test added in the last commit is
> adjusted to check that push is still assumed when "--patch" comes after
> other options on the command-line.
All makes sense to me.
> if (argc) {
> - force_assume = argc > 1 && !strcmp(argv[1], "-p");
This is where we drop the very specific approach of "let's look for -p".
> + int flags = PARSE_OPT_KEEP_DASHDASH;
This is the flag we've always been using.
> + if (push_assumed)
> + flags |= PARSE_OPT_STOP_AT_NON_OPTION;
Now we use this, too, if we've assumed "push". Makes sense even without
the specific context of this patch: we've assumed an implicit "push", so
let's be a bit less aggressive in parsing the remainder.
> argc = parse_options(argc, argv, prefix, options,
> push_assumed ? git_stash_usage :
> - git_stash_push_usage,
> - PARSE_OPT_KEEP_DASHDASH);
> + git_stash_push_usage, flags);
> + force_assume |= patch_mode;
Rather than looking for "-p" in a fixed place, we see if option parsing
spotted it. Makes perfect sense. Although, why `|=` here? We initialize
`force_assume` to 0 at the top and this is the only other time we write
to it. Why not just `force_assume = patch_mode`? Future-proofing?
> -test_expect_success 'stash -p <pathspec> stash and restores the file' '
> +test_expect_success 'stash --patch <pathspec> stash and restores the file' '
> cat file >expect-file &&
> echo changed-file >file &&
> echo changed-other-file >other-file &&
> - echo a | git stash -p file &&
> + echo a | git stash -m "stash bar" --patch file &&
> test_cmp expect-file file &&
> echo changed-other-file >expect &&
> test_cmp expect other-file &&
We lose the test of `-p` that we just added. Ok. We should be able to
trust our option parsing machinery to get this right. This s/-p/--patch/
demonstrates that your patch works, and as for running this as a
regression test in the future, we'll be using one of the equivalent ways
of spelling this option. Ok.
Martin
