Re: [PATCH v2] stash: expose untracked modes in create

[Junio C Hamano <gitster@pobox.com>]

Kazumasa Shigeta <kazumasa.shigeta@kanamei.com> writes:

> `git stash create` always passes zero for the include_untracked parameter
> of do_create_stash(), even though that helper already supports untracked
> and ignored files and stash push/save expose those modes as
> -u/--include-untracked and -a/--all.

There may be no lies in what the above says, but we would prefer to
hear what the user visible implication of "passing 0" is more than
what mechanically is happening inside a program.  For example:

    "git stash create", "git stash push", and "git stash save" are
    commands that create a new stash entry.  The latter two are also
    responsible for storing the resulting stash entry to the reflog
    of the "refs/stash" ref, but have options to control what is
    included in the stash entry.  Among these options, "create" only
    supports the equivalent of "-m <message." to record in the stash
    entry.  Most notably, "-u" and "-a" options are missing.

> Teach create to accept the same options and pass the existing mode
> through. Unlike push/save, create continues to only create objects: it
> does not update refs/stash, reset the index, or clean the working tree.

Sure.  It is a very concise and good description of what we want to
do.

> Use parse_options() for the new options and stop parsing at the first
> non-option message word. This keeps option-like tokens after the message
> as message text, while leading option-like arguments now follow Git's
> normal option parsing. In particular, unknown or malformed leading
> options are rejected instead of silently becoming a message, short
> options may be combined, and `--` can be used when a message itself
> begins with a dash.

Why do we need to go into such a detail in the log message?  What is
the above paragraph designed to convey to the reader?  Again, it may
not be telling any lies, but it misses the point by being inconsiderate
to your readers.  What you need to tell them is _WHY_ you chose to
use parse_options() in such a way.  What were you trying to achieve?

I am guessing that something along this line ...

    "git stash create" traditionally treated the rest of the command
    line as a message.  For example, 

	$ git stash create adding -u option

    has always been a request to create a stash entry with the
    string "adding -u option" as its message.  We should not make it
    trigger the "-u" (include untracked) behavior for backward
    compatibility, by using parse_options() with stop-at-the-non-option
    mode to forbid it from reordering the command line arguments.

... was what you wanted to say, but I am not sure.

How much of all these verbiage was written by AI by the way?  You'd
need to spend effort to make it readable to humans.

> Keep create's existing no-change behavior: detect the usual no-change
> case before do_create_stash() refreshes and writes the index, and return
> success without printing an object name. If do_create_stash() still
> reports its internal "nothing to create" result, map that to create's
> public success status.

You already said that with "does not update, reset, or clean".

> This follows the stash subcommand exit-status convention established by
> 786fc390465f (stash: reserve exit status 1 for conflicts, 2026-09-03):
> subcommands return 0 on success, negative values on failure, and status 1
> when applying a stash results in conflicts. cmd_stash() maps negative
> subcommand failures to 128.

Again, there may not be lies in here, but if you did not make a
breaking change to the established convention, is it worth saying?

> 9ca6326dff29 (stash: refactor stash_create, 2017-02-19) added the
> internal include-untracked path while intentionally leaving the user
> interface for "git stash create" unchanged. Reuse that machinery and
> the existing INCLUDE_ALL_FILES mode rather than adding a separate stash
> creation path.
>
> Add coverage for short and long aliases, combined short options, the
> untracked/ignored boundary including an ignored-only worktree, option
> parsing and dash-leading messages, no-change behavior, and preservation
> of refs/stash, the index state, and the working tree.

Again, adding tests for comprehensive coverage is not something to
boast about.  Is it worth saying?

Aren't -p/-S/-k/-q and pathspec support all about the creating half
of "git stash push" that are not available to "git stash create",
not just "-u" and "-a"?  Why are we singling out only these two?  It
may be more worthwhile to explain the rationale behind such a design
decision.