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

> From: Miroma <its.miroma@proton.me>
>
> Currently, when status.showStash is set, 'stash pop' shows the
> following, confusing, output:
>
>     ...
>     Your stash currently has 1 entry
>     Dropped refs/stash@{0} (abc123...)
>

Right, so your proposal is to not print stash related status information
when already running a stash command. It would be nice to note that
here, along why you think so.

Personally, I think it is important to keep this as is, because it tells
the user the entries left in the stash post the stash operation.

> Signed-off-by: Miroma <its.miroma@proton.me>
> ---
>     stash: don't show irrelevant entry count in status
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2067%2Fits-miroma%2Fstash-no-status-showStash-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2067/its-miroma/stash-no-status-showStash-v1
> Pull-Request: https://github.com/git/git/pull/2067
>
>  builtin/stash.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/builtin/stash.c b/builtin/stash.c
> index 1977e50df2..2bd4b7d753 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -705,6 +705,9 @@ restore_untracked:
>  			     absolute_path(repo_get_work_tree(the_repository)));
>  		strvec_pushf(&cp.env, GIT_DIR_ENVIRONMENT"=%s",
>  			     absolute_path(repo_get_git_dir(the_repository)));
> +		strvec_push(&cp.env, "GIT_CONFIG_COUNT=1");
> +		strvec_push(&cp.env, "GIT_CONFIG_KEY_0=status.showStash");
> +		strvec_push(&cp.env, "GIT_CONFIG_VALUE_0=false");
>  		strvec_push(&cp.args, "status");
>  		run_command(&cp);
>

So this block is run to print the status, unless the '--quiet' option is
used. So it makes sense to do this here.

Small nit: Shouldn't we add a test to validate this change in behavior?

Karthik Nayak <karthik.188@gmail.com> writes:

> "Miroma via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: Miroma <its.miroma@proton.me>
>>
>> Currently, when status.showStash is set, 'stash pop' shows the
>> following, confusing, output:
>>
>>     ...
>>     Your stash currently has 1 entry
>>     Dropped refs/stash@{0} (abc123...)
>>
>
> Right, so your proposal is to not print stash related status information
> when already running a stash command. It would be nice to note that
> here, along why you think so.
>
> Personally, I think it is important to keep this as is, because it tells
> the user the entries left in the stash post the stash operation.

When pop sees a conflict, it keeps the stash entry, and whe it does
not, it discards the stash entry it used just now.  So I agree with
you that the number of remaining entries may be something the user
would want to see, but in do_apply_stash(), I think we are getting
the number before popping, as dropping the entry seems to be the
responsibility of the caller of do_apply_stash(), if I am reading
pop_stash() correctly.  So, I suspect the current output may need
adjustment if we want to keep the message and want it to be useful.

>> @@ -705,6 +705,9 @@ restore_untracked:
>>  			     absolute_path(repo_get_work_tree(the_repository)));
>>  		strvec_pushf(&cp.env, GIT_DIR_ENVIRONMENT"=%s",
>>  			     absolute_path(repo_get_git_dir(the_repository)));
>> +		strvec_push(&cp.env, "GIT_CONFIG_COUNT=1");
>> +		strvec_push(&cp.env, "GIT_CONFIG_KEY_0=status.showStash");
>> +		strvec_push(&cp.env, "GIT_CONFIG_VALUE_0=false");
>>  		strvec_push(&cp.args, "status");
>>  		run_command(&cp);
>>
>
> So this block is run to print the status, unless the '--quiet' option is
> used. So it makes sense to do this here.

Does it?  What happens in existing GIT_CONFIG_{COUNT,KEY,VALUE}*
environment variables passed by the script that invoked us?

If we are spawning "git status" here, can't we do a much simpler and
more obvious thing, i.e. "git -c status.showstash=false status"?

> Small nit: Shouldn't we add a test to validate this change in behavior?

Yes.  I am not yet sure what the right output should be, though.

"Karthik Nayak" <karthik.188@gmail.com> writes:

> "Miroma via GitGitGadget" gitgitgadget@gmail.com writes:
> 

> > From: Miroma its.miroma@proton.me
> > 

> > Currently, when status.showStash is set, 'stash pop' shows the
> > following, confusing, output:
> > 

> > ...
> > Your stash currently has 1 entry
> > Dropped refs/stash@{0} (abc123...)
> 

> 

> Right, so your proposal is to not print stash related status information
> when already running a stash command. It would be nice to note that
> here, along why you think so.

I'll update the commit message.  Briefly: I think the stash count shown by
'status' when called by 'stash' internally is confusing, especially in the
cited example of 'stash pop', because the count is immediately rendered
obsolete as the pop drops it by one:

```sh
$ git stash pop
...
Your stash currently has 1 entry
Dropped refs/stash@{0} (abc123...)
$
$ git status --show-stash
...
<no stashes>
```

> Personally, I think it is important to keep this as is, because it tells
> the user the entries left in the stash post the stash operation.

The issue is that the number shown is *not* post the stash operation, rather
before; I called the output in the original message "confusing" because it
claimed 1 entry, then immediately dropped it.  The real count after the stash
drop is 0.


"Junio C Hamano" gitster@pobox.com writes:

> When pop sees a conflict, it keeps the stash entry, and whe it does
> not, it discards the stash entry it used just now. So I agree with
> you that the number of remaining entries may be something the user
> would want to see, but in do_apply_stash(), I think we are getting
> the number before popping, as dropping the entry seems to be the
> responsibility of the caller of do_apply_stash(), if I am reading
> pop_stash() correctly. So, I suspect the current output may need
> adjustment if we want to keep the message and want it to be useful.

Yes, currently 'status' shows the stash count, then 'stash pop' calls it
before the stash is dropped.

A possible solution would be calling wt_longstatus_print_stash_summary()
from 'wt-status.c' here, once 'stash' is ready to show the count.

> > So this block is run to print the status, unless the '--quiet' option is
> > used. So it makes sense to do this here.
> 

> 

> Does it? What happens in existing GIT_CONFIG_{COUNT,KEY,VALUE}*
> environment variables passed by the script that invoked us?

Good point, I missed that...  But find a solution below

> If we are spawning "git status" here, can't we do a much simpler and
> more obvious thing, i.e. "git -c status.showstash=false status"?

I found that 'status' knows of the '--no-show-stash' option; I'll use that.

> > Small nit: Shouldn't we add a test to validate this change in behavior?
> 

> 

> Yes. I am not yet sure what the right output should be, though.

I suggest the last line of the output of 'stash' commands be the count:

```sh
# assuming there are two stashes
$ git stash pop
...
Dropped refs/stash@{0} (abc123...)
Your stash currently has 1 entry
```