[PATCH 0/2] git stash drop stash@{2.days.ago}

[PATCH 0/2] git stash drop stash@{2.days.ago}

[Junio C Hamano]

Because 'stash' is implemented in terms of the reflog, it can accept
not only a small integer index (such as 'stash@{4}') but also a
time-based reference.  This is not a good thing.

 - 'git stash pop stash@{2.days.ago}' picks the first stash entry
   that is no younger than the specified time and uses it to modify
   the working tree and the index, but then removes all stash
   entries that are no younger than that specified time.

 - 'git stash drop stash@{2.days.ago}' does the same, except that
   no entry is used to affect the working tree and the index.

These two patches forbid passing time-based stash references to the
'git stash drop' and 'git stash pop' commands as minor safety
improvements.

 1/2: stash: record positional index in 'struct stash_info'
 2/2: stash: reject time-based selectors in drop and pop

 Documentation/git-stash.adoc |  8 ++++++++
 builtin/stash.c              | 18 ++++++++++++++++++
 t/t3903-stash.sh             | 13 +++++++++++++
 3 files changed, 39 insertions(+)

-- 
2.55.0-597-ge6126a35d6

[PATCH 1/2] stash: record positional index in 'struct stash_info'

[Junio C Hamano]

get_stash_info() resolves revision arguments (such as 'stash@{0}'
or '2') and checks whether they refer to 'refs/stash', but it
does not allow callers to determine the 0-based positional
reflog index.

Record '.stash_idx' in 'struct stash_info'.  Populate it in
get_stash_info(), setting it to 0 when omitted (defaulting
to the latest stash), to 'n' when a valid positional index
'@{n}' is specified, or to -1 when the index specification
is invalid or non-positional (such as a time-based reference).

Subcommands that manipulate reflog entries by index can use
'.stash_idx' directly, instead of parsing the revision arguments
themselves.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/stash.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/builtin/stash.c b/builtin/stash.c
index c4809f299a..5041a9ba81 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -175,6 +175,7 @@ struct stash_info {
 	struct strbuf revision;
 	int is_stash_ref;
 	int has_u;
+	int stash_idx;
 };
 
 #define STASH_INFO_INIT { \
@@ -248,6 +249,7 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	char *expanded_ref;
 	const char *revision;
 	const char *commit = NULL;
+	const char *at;
 	struct object_id dummy;
 	struct strbuf symbolic = STRBUF_INIT;
 
@@ -300,6 +302,19 @@ static int get_stash_info(struct stash_info *info, int argc, const char **argv)
 	}
 
 	free(expanded_ref);
+
+	at = strstr(revision, "@{");
+	if (at) {
+		char *ep;
+		unsigned long u = strtoul(at + 2, &ep, 10);
+		if (ep > at + 2 && *ep == '}' && u < 100000000)
+			info->stash_idx = (int)u;
+		else
+			info->stash_idx = -1;
+	} else {
+		info->stash_idx = 0;
+	}
+
 	return !(ret == 0 || ret == 1);
 }
 
-- 
2.55.0-597-ge6126a35d6

[PATCH 2/2] stash: reject time-based selectors in drop and pop

[Junio C Hamano]

get_stash_info_assert() verifies that a revision is a stash
reference ('.is_stash_ref'), but it does not verify whether the
reference is a positional reflog index as opposed to a time-based
selector (such as 'stash@{2.days}').

When subcommands such as 'git stash drop' or 'git stash pop' pass
time-based selectors to reflog_delete(), reflog_delete() treats
non-integer selectors as expiration cutoff timestamps.
Consequently,

 - 'git stash drop stash@{2.days.ago}' deletes all stash entries
   older than two days instead of dropping a single entry, and

 - 'git stash pop stash@{2.days.ago}' applies a single stash entry
   at that timestamp and then deletes all stash entries older than
   two days.

While the former might be remotely useful, the latter is certainly
not.  In get_stash_info_assert(), reject references where
'.stash_idx' is negative (i.e., a time-based reference was used),
ensuring that 'git stash drop' and 'git stash pop' fail early on
invalid or date-based stash references.

Document that 'git reflog expire --expire=<time> refs/stash' should
be used to prune stashes by age, and add unit tests covering
rejection of time-based selectors for 'drop' and 'pop'.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-stash.adoc |  8 ++++++++
 builtin/stash.c              |  3 +++
 t/t3903-stash.sh             | 13 +++++++++++++
 3 files changed, 24 insertions(+)

diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
index 50bb89f483..6711157421 100644
--- a/Documentation/git-stash.adoc
+++ b/Documentation/git-stash.adoc
@@ -106,6 +106,10 @@ command to control what is shown and how. See linkgit:git-log[1].
 	operation of `git stash push`. The working directory must
 	match the index.
 +
+When _<stash>_ is specified, it must be a positional stash index
+of the form `stash@{<n>}` or `<n>`. Time-based reflog selectors
+(e.g. `stash@{2.days.ago}`) are not accepted.
++
 Applying the state can fail with conflicts; in this case, it is not
 removed from the stash list. You need to resolve the conflicts by hand
 and call `git stash drop` manually afterwards.
@@ -137,6 +141,10 @@ with no conflicts.
 
 `drop [-q | --quiet] [<stash>]`::
 	Remove a single stash entry from the list of stash entries.
+	When _<stash>_ is specified, it must be a positional stash index
+	of the form `stash@{<n>}` or `<n>`. Time-based reflog selectors
+	(e.g. `stash@{2.days.ago}`) are not accepted. To prune stashes older
+	than a given timestamp, use `git reflog expire --expire=<time> refs/stash`.
 
 `create`::
 	Create a stash entry (which is a regular commit object) and
diff --git a/builtin/stash.c b/builtin/stash.c
index 5041a9ba81..6f9561ee3a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -865,6 +865,9 @@ static int get_stash_info_assert(struct stash_info *info, int argc,
 	if (!info->is_stash_ref)
 		return error(_("'%s' is not a stash reference"), info->revision.buf);
 
+	if (info->stash_idx < 0)
+		return error(_("'%s' is not a valid stash index"), info->revision.buf);
+
 	return 0;
 }
 
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index da27a6599a..01d59c8ef4 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -808,6 +808,19 @@ test_expect_success 'pop: fail early if specified stash is not a stash ref' '
 	git reset --hard HEAD
 '
 
+test_expect_success 'drop and pop reject time-based reflog selectors' '
+	git stash clear &&
+	test_when_finished "git reset --hard HEAD && git stash clear" &&
+	git reset --hard &&
+	echo foo >file &&
+	git stash &&
+	test_must_fail git stash drop stash@{2.days.ago} 2>err &&
+	test_grep "is not a valid stash index" err &&
+	test_must_fail git stash pop stash@{2.days.ago} 2>err &&
+	test_grep "is not a valid stash index" err &&
+	git stash drop
+'
+
 test_expect_success 'ref with non-existent reflog' '
 	git stash clear &&
 	echo bar5 >file &&
-- 
2.55.0-597-ge6126a35d6