Re: [PATCH] Add 'preserve' subcommand to 'git stash'

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 16, 2023, 16:42 UTC
Message-ID: <xmqqjzw3qry6.fsf@gitster.g>
In-Reply-To: <pull.1528.git.git.1686913210137.gitgitgadget@gmail.com>

"Nadav Goldstein via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 6 quoted lines

> From: Nadav Goldstein <nadav.goldstein@blazepod.co>
>
> In this patch, we introduce a new subcommand preserve to
> git stash. The purpose of this subcommand is to save the
> current changes into the stash and then immediately re-apply
> those changes to the working directory.

Why a new subcommand, not a new option to "push"? Adding a new subcommand would mean it would be another unfamiliar thing users need to learn, as opposed to a slight variation of what they are already familiar with.

Show 5 quoted lines

> Implementation-wise, this is achieved by adding a new branch
> to the conditional in the cmd_stash function, where we check
> if argv[0] is "preserve". If it is, we push_stash with the
> new argument that we added to it preserve=1.
> In all other cases we call push_stack/do_push_stack preserve=0

The proposed log message is lacking "why". What problem does it want to solve? What are the things you cannot do without the feature, and what does the new feature allow you to do that you couldn't do before? On the other hand, the implementation detail, unless it is tricky to read from the patch, does not have to be repeated here---please aim to make the patch and new code clear enough not to require such commentary.

More on this in "Describe your changes well" section in the Documentation/SubmittingPatches document.

Show 13 quoted lines

> Signed-off-by: Nadav Goldstein <nadav.goldstein96@gmail.com>
> ---
>     Add 'preserve' subcommand to 'git stash'
>     
>     In this patch, we introduce a new subcommand preserve to git stash. The
>     purpose of this subcommand is to save the current changes into the stash
>     and then immediately re-apply those changes to the working directory.
>     
>     Implementation-wise, this is achieved by adding a new branch to the
>     conditional in the cmd_stash function, where we check if argv[0] is
>     "preserve". If it is, we push_stash with the new argument that we added
>     to it preserve=1. In all other cases we call push_stack/do_push_stack
>     preserve=0

Please do not repeat what you already said in your log message here. This is a place to give additional message that would help only during the review.

>     If the community will approve, I will modify the patch to include help
>     messages for the new subcommand

Please don't think this way. If the new feature is not worth completing to document and tests for your own use, it is not worth community's time to review or "approve" it. Instead, we try to send a patch that is already perfected, with tests and docs, in order to improve the chance reviewers will understand the new feature and its motivation better when they review the patch.

Show 15 quoted lines

>  static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int quiet,
> -			 int keep_index, int patch_mode, int include_untracked, int only_staged)
> +			 int keep_index, int patch_mode, int include_untracked, int only_staged, int preserve)
>  {
>  	int ret = 0;
>  	struct stash_info info = STASH_INFO_INIT;
> @@ -1643,7 +1643,7 @@ static int do_push_stash(const struct pathspec *ps, const char *stash_msg, int q
>  				ret = -1;
>  				goto done;
>  			}
> -		} else {
> +		} else if (!preserve) {
>  			struct child_process cp = CHILD_PROCESS_INIT;
>  			cp.git_cmd = 1;
>  			/* BUG: this nukes untracked files in the way */

While this does skip the "reset --hard" step,

 * With --include-untracked and without a pathspec, we have run "clean
   --force --quiet" already, removing the untracked files that have
   been stored in the stash.  Wasn't the objective of the change to
   ensure that the working tree files are unaffected?

 * When run with a pathspec, this change has no effect, no?  We've
   added updated files with "add -u" to the index, compared the
   contents between HEAD and what was added with "add -u" and
   applied the reverse of it to both the index and the working tree,
   which amounts to reverting the changes to the paths that match
   the pathspec.

 * When run with --keep-index, there is another block after this
   part that uses "checkout --no-overlay" to ensure that the
   contents of the working tree matches what is in the index.

 * When the outermost "if (!(patch_mode || only_staged))" block is
   not entered, we'd run "apply -R" to revert the changes in the
   patch, whose point is to update the working tree contents.

What the patch does seem to fall far short of "preserving" in many cases. In some modes, I suspect that it might not make sense to use "preserve" (for example, it could be that "--keep-index" may be totally incompatible with "--preserve"), but that is hard to guess without knowing _why_ you wanted to this new feature in the first place. Adding a separate subcommand that gives users no access to these modes would be an easy way to avoid having to think about these issues, but may close too many doors, including ones those that do not have to be closed if this feature is done as a new option to existing "push" command. I dunno.

Thanks.