On Tue, 20 May 2025 at 11:27, Phillip Wood [off-list ref] 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