From: Martin Ågren <martin.agren@gmail.com>
Date: Fri, 06 Jun 2025 11:32:58 GMT
Subject: Re: [PATCH v2 2/2] stash: allow "git stash [<options>] --patch <pathspec>" to assume push
Message-ID: <CAN0heSq56q5nQnrd0YBOWEvj7uXEFkWG3DH6Ms6JVkFNnuUmBA@mail.gmail.com>
In-Reply-To: <98ad3de977090a793408b25ca880b65f058ea44e.1747733203.git.phillip.wood@dunelm.org.uk>

On Tue, 20 May 2025 at 11:27, Phillip Wood <phillip.wood123@gmail.com> 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