stash: don't show irrelevant entry count in status

From: Miroma <[email protected]>

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

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

---
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(+)

index 1977e50df2..2bd4b7d753 100644
@@ -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);
 	}

base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0

gitgitgadget

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.

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"?

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

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

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.

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.

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

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

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

From: Miroma <[email protected]>

Currently, 'stash apply' internally calls 'status' to show a summary of what the command did. This also happens with 'stash pop', which roughly corresponds to 'apply' + 'drop'.

When the configuration 'status.showStash' is set, 'status' shows a count of stashed entries. This leads to the following, confusing, output:

```sh
$ # assuming there are two stashed entries
$ git stash pop
...
Your stash currently has 2 entries
Dropped refs/stash@{0} (abc123...)
$ # when actually...
$ git status --show-stash
...
Your stash currently has 1 entry
```

This patch changes the output format to the following, no matter the value of 'status.showStash':

```sh
$ # assuming there's a stashed entry
$ git stash pop
...
Dropped refs/stash@{0} (abc123...)
Your stash now has 0 entries
```

This new output is only shown if the stash count has changed, which can happen with the following subcommands:
'drop', 'pop', 'branch', 'store', 'push', 'save'.

---
stash: show correct entries count

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2067%2Fits-miroma%2Fstash-no-status-showStash-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2067/its-miroma/stash-no-status-showStash-v2
Pull-Request: https://github.com/git/git/pull/2067

Range-diff vs v1:

 1:  32a45cf3d2 < -:  ---------- stash: don't show irrelevant entry count in status
 -:  ---------- > 1:  1118bd68ac stash: show correct entries count

 builtin/stash.c  | 31 ++++++++++++++++++--
 t/t3903-stash.sh | 75 ++++++++++++++++++++++++++++--------------------
 wt-status.c      |  2 +-
 wt-status.h      |  1 +
 4 files changed, 75 insertions(+), 34 deletions(-)

index 1977e50df2..1bee391fb2 100644
@@ -32,6 +32,7 @@
 #include "add-interactive.h"
 #include "oid-array.h"
 #include "commit.h"
+#include "wt-status.h"
 
 #define INCLUDE_ALL_FILES 2
 

@@ -585,6 +586,20 @@ static void unstage_changes_unless_new(struct object_id *orig_tree)
 		die(_("could not write index"));
 }
 
+static int previous_stash_count = -1;
+
+static void print_stash_status(bool quiet)
+{
+	int stash_count;
+	stash_count = count_stash_entries();
+
+	if (quiet || stash_count == previous_stash_count || previous_stash_count == -1)
+		return;
+
+	printf_ln(Q_("Your stash now has %d entry",
+		"Your stash now has %d entries", stash_count), stash_count);
+}
+
 static int do_apply_stash(const char *prefix, struct stash_info *info,
 			  int index, int quiet)
 {

@@ -705,7 +720,7 @@ 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.args, "status");
+		strvec_pushl(&cp.args, "status", "--no-show-stash", NULL);
 		run_command(&cp);
 	}
 

@@ -756,6 +771,8 @@ static int reflog_is_empty(const char *refname)
 
 static int do_drop_stash(struct stash_info *info, int quiet)
 {
+	previous_stash_count = count_stash_entries();
+
 	if (!reflog_delete(info->revision.buf,
 			   EXPIRE_REFLOGS_REWRITE | EXPIRE_REFLOGS_UPDATE_REF,
 			   0)) {

@@ -805,6 +822,8 @@ static int drop_stash(int argc, const char **argv, const char *prefix,
 		goto cleanup;
 
 	ret = do_drop_stash(&info, quiet);
+	print_stash_status(quiet);
+
 cleanup:
 	free_stash_info(&info);
 	return ret;

@@ -836,6 +855,8 @@ static int pop_stash(int argc, const char **argv, const char *prefix,
 	else
 		ret = do_drop_stash(&info, quiet);
 
+	print_stash_status(quiet);
+
 cleanup:
 	free_stash_info(&info);
 	return ret;

@@ -875,6 +896,8 @@ static int branch_stash(int argc, const char **argv, const char *prefix,
 	if (!ret && info.is_stash_ref)
 		ret = do_drop_stash(&info, 0);
 
+	print_stash_status(0);
+
 cleanup:
 	free_stash_info(&info);
 	return ret;

@@ -1062,6 +1085,7 @@ static int do_store_stash(const struct object_id *w_commit, const char *stash_ms
 	struct stash_info info;
 	char revision[GIT_MAX_HEXSZ];
 
+	previous_stash_count = count_stash_entries();
 	oid_to_hex_r(revision, w_commit);
 	assert_stash_like(&info, revision);
 

@@ -1119,6 +1143,7 @@ static int store_stash(int argc, const char **argv, const char *prefix,
 	}
 
 	ret = do_store_stash(&obj, stash_msg, quiet);
+	print_stash_status(quiet);
 
 out:
 	object_context_release(&dummy);

@@ -1914,6 +1939,7 @@ static int push_stash(int argc, const char **argv, const char *prefix,
 
 	ret = do_push_stash(&ps, stash_msg, quiet, keep_index, patch_mode,
 			    &add_p_opt, include_untracked, only_staged);
+	print_stash_status(quiet);
 
 	clear_pathspec(&ps);
 	free(pathspec_from_file);

@@ -1982,6 +2008,7 @@ static int save_stash(int argc, const char **argv, const char *prefix,
 	ret = do_push_stash(&ps, stash_msg, quiet, keep_index,
 			    patch_mode, &add_p_opt, include_untracked,
 			    only_staged);
+	print_stash_status(quiet);
 
 	strbuf_release(&stash_msg_buf);
 	return ret;

@@ -2291,7 +2318,7 @@ static int do_export_stash(struct repository *r,
 			.r = r, .items = iter,
 		};
 		if (refs_for_each_reflog_ent_reverse(get_main_ref_store(r),
-						     "refs/stash",
+						     ref_stash,
 						     collect_stash_entries,
 						     &cb_data) && cb_data.count)
 			goto out;

index 454601afa1..6bdf71efee 100644
@@ -983,7 +983,7 @@ static int stash_count_refs(struct object_id *ooid UNUSED,
 	return 0;
 }
 
-static int count_stash_entries(void)
+int count_stash_entries(void)
 {
 	int n = 0;
 	refs_for_each_reflog_ent(get_main_ref_store(the_repository),

index 4e377ce62b..daccde0b6e 100644
@@ -151,6 +151,7 @@ size_t wt_status_locate_end(const char *s, size_t len);
 void wt_status_append_cut_line(struct strbuf *buf);
 void wt_status_add_cut_line(struct wt_status *s);
 void wt_status_prepare(struct repository *r, struct wt_status *s);
+int count_stash_entries(void);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
 /*

base-commit: c44beea485f0f2feaf460e2ac87fdd5608d63cf0

gitgitgadget

I have a few questions I'd appreciate an input on:

- Should 'stash clear' also print "Your stash now has 0 entries"?
- Why does 'do_push_stack' pass quiet=0 to 'do_store_stash' (line 1695)?
- Why do the following subcommands not support '--quiet'?

    'branch', 'import', 'export', 'create', 'clear'

- Is exporting 'count_stash_entries' in "wt-status" a good idea?