Re: [PATCH v2] stash: implement '--staged' option for 'push' and 'save'

> Sergey Organov <sorganov@gmail.com> writes:
>
>> @@ -205,6 +205,16 @@ to learn how to operate the `--patch` mode.
>>  The `--patch` option implies `--keep-index`.  You can use
>>  `--no-keep-index` to override this.
>>  
>> +-S::
>> +--staged::
>> +	This option is only valid for `push` and `save` commands.
>> ++
>> +Stash only the changes that are currently staged. This is similar to
>> +basic `git commit` except the state is committed to the stash instead
>> +of current branch.
>> ++
>> +The `--patch` option has priority over this one.
>> +
>>  --pathspec-from-file=<file>::
>>  	This option is only valid for `push` command.
>>  +
>> @@ -341,6 +351,24 @@ $ edit/build/test remaining parts
>>  $ git commit foo -m 'Remaining parts'
>>  ----------------------------------------------------------------
>>  
>> +Saving unrelated changes for future use::
>> +
>> +When you are in the middle of massive changes and you find some
>> +unrelated issue that you don't want to forget to fix, you can do the
>> +change(s), stage them, and use `git stash push --staged` to stash them
>> +out for future use. This is similar to committing the staged changes,
>> +only the commit ends-up being in the stash and not on the current branch.
>> ++
>> +----------------------------------------------------------------
>> +# ... hack hack hack ...
>> +$ git add --patch foo           # add unrelated changes to the index
>> +$ git stash push --staged       # save these changes to the stash
>> +# ... hack hack hack, finish curent changes ...
>> +$ git commit -m 'Massive'       # commit fully tested changes
>> +$ git switch fixup-branch       # switch to another branch
>> +$ git stash pop                 # to finish work on the saved changes
>> +----------------------------------------------------------------
>> +
>
> The last step would more like "to start working on top of the saved
> changes", I would think, as the user did not want to bother thinking
> about them earlier while working on the other theme.  Otherwise, the
> user would have done
>
>     git checkout -b fixup-branch
>     git add -p
>     git commit -m '[WIP] mostly done' -e
>     git checkout -
>
> to remember that the fixup is not quite but mostly done and what was
> done so far.

>
> But I'd agree that the new mode would fit in such a workflow.
>
>
>> +static int stash_staged(struct stash_info *info, const struct pathspec *ps,
>> +		       struct strbuf *out_patch, int quiet)
>> +{
>> +	int ret = 0;
>> +	struct child_process cp_diff_tree = CHILD_PROCESS_INIT;
>> +	struct index_state istate = { NULL };
>> +
>> + if (write_index_as_tree(&info->w_tree, &istate,
>> the_repository->index_file,
>> +				0, NULL)) {
>> +		ret = -1;
>> +		goto done;
>> +	}
>
> OK.  So what is currently in the index becomes the w-tree.
>
>> +	cp_diff_tree.git_cmd = 1;
>> +	strvec_pushl(&cp_diff_tree.args, "diff-tree", "-p", "-U1", "HEAD",
>> +		     oid_to_hex(&info->w_tree), "--", NULL);
>> +	if (pipe_command(&cp_diff_tree, NULL, 0, out_patch, 0, NULL, 0)) {
>> +		ret = -1;
>> +		goto done;
>> +	}
>> +
>> +	if (!out_patch->len) {
>> +		if (!quiet)
>> +			fprintf_ln(stderr, _("No changes selected"));
>> +		ret = 1;
>> +	}
>
> This seems to have been taken from the "stash_patch()" flow,

> but
> unlike the "stash -p" that goes interactive to let the user pick
> hunks, in which context "oh, no, you did not SELECT anything" makes
> perfect sense as an error message, this message would be confusing
> to users who weren't offered a chance to select.

>
>> +done:
>> +	discard_index(&istate);
>> +	return ret;
>> +}
>> +
>
> Also, as stash_staged() and stash_patch() are _so_ close, I suspect
> that we might want to make a common helper out of the original
> stash_patch() and make stash_patch() a thin wrapper around it in the
> step #1 of the series, and then add stash_staged() as a second
> caller to the common helper.  That might result in a cleaner end
> result.  And optionally the "do we really need to spawn diff-tree"
> optimization can be done on top after the dust settles [*].

>>  static int do_create_stash(const struct pathspec *ps, struct strbuf
>> *stash_msg_buf,
>> -			   int include_untracked, int patch_mode,
>> + int include_untracked, int patch_mode, int only_staged,
>
> Let's not keep adding a parameter to represent just a bit (or less).
> Can't we at least treat this as an auxiliary bit in the "patch_mode"
> flag?  The traditional patch_mode may be variant #1 while the one
> that does the same thing but skips the interactive hunk selection
> (i.e. only-staged) becomes the variant #2, or something?

>
>> @@ -1321,6 +1353,16 @@ static int do_create_stash(const struct pathspec *ps, struct strbuf *stash_msg_b
>>  		} else if (ret > 0) {
>>  			goto done;
>>  		}
>> +	} else if (only_staged) {
>> +		ret = stash_staged(info, ps, patch, quiet);
>> +		if (ret < 0) {
>> +			if (!quiet)
>> +				fprintf_ln(stderr, _("Cannot save the current "
>> +						     "staged state"));
>> +			goto done;
>> +		} else if (ret > 0) {
>> +			goto done;
>> +		}
>
> ... which would reduce the need to add yet another else-if to this
> cascade.

>
>> @@ -1379,7 +1421,7 @@ static int create_stash(int argc, const char **argv, const char *prefix)
>>  	if (!check_changes_tracked_files(&ps))
>>  		return 0;
>>  
>> -	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, &info,
>> +	ret = do_create_stash(&ps, &stash_msg_buf, 0, 0, 0, &info,
>>  			      NULL, 0);
>
> and no need to touch this hunk.

>
>> @@ -1389,7 +1431,7 @@ static int create_stash(int argc, const char
>> **argv, const char *prefix)
>>  }
>>  
>>  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 keep_index, int patch_mode, int include_untracked, int
>> only_staged)
>>  {
>
> nor this one.