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

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

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

> 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.

> 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.

> 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.

>  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.