Re: [git] Re: Bug in git-stash(.sh) ?

On Thu, May 03, 2012 at 02:44:01PM -0400, Eli Barzilay wrote:

quoted

Once that is fixed, then we can consider whether something more should
happen for stash (though I am inclined to say that is enough; it is a
feature that you can do "git stash list --date=relative" to see the
stash timestamps).

Since the general problem is bigger, how about just the quick patch of
adding --date=default in the list_stash function as a stopgap?  That
seems to be close enough to how it should work anyway.

It is bigger in scope, but the fix is still pretty small. I was trying
to trick^W gently prod you into making a patch, but that does not seem
to have worked. :) So here is a series that fixes it, and we don't have
to worry about a stopgap.

  [1/4]: t1411: add more selector index/date tests
  [2/4]: log: respect date_mode_explicit --format:%gd
  [3/4]: reflog-walk: clean up "flag" field of commit_reflog struct
  [4/4]: reflog-walk: always make HEAD@{0} show indexed selectors

The first two fix and test the bug I mentioned, and as a result solve
the stash problem. The second two fix and test the bug that Junio
mentioned. This doesn't affect stash, but it's the right thing for "git
log" to do.

quoted

quoted

  * Some new %gi uses the index number: stash@{1}, and %gI produces
    refs/stash@{1}, unrelated to any date setting
  * git-stash.sh uses %gi so the output has the numbers
  * Some new option for "stash list" for the format string, so it's
    possible to show the dates if you want to with something like
    git stash list --format:"%gi: %gs (%gd)"

I don't have a huge problem with that. But what issue is it really
solving? Are people using "git stash list --date=iso" and then
getting confused by the output? Or is it simply a matter of
mistakenly applying the config when it should not be? The latter
needs fixed in either case.

It's basically an attempt to have a %gi that is disconnected from date
options (config or flags), which solves the config problem in a
trivial way (no date options are used)...

I don't have a problem at all with %gi; I think it would be a good
addition. I just think that stash shouldn't use, as the "--date" thing
is a feature that there is no reason to deny to stash users (it just
needs to be less buggy :) ).

-Peff

When we prepare to walk a reflog, we parse the specification
and pull some information from it, such as which reflog to
look in (e.g., HEAD), and where to start (e.g., HEAD@{10} or
HEAD@{yesterday}). The resulting struct has a "recno" field
to show where in the reflog we are starting. It also has a
"flag" field; if true, it means the recno field came from
parsing a date like HEAD@{yesterday}.

There are two problems with this:

  1. "flag" is an absolutely terrible name, as it conveys
     nothing about the meaning

  2. you can tell "HEAD" from "HEAD@{yesterday}", but you
     can't differentiate "HEAD" from "HEAD{0}"

This patch converts the flag into a tri-state (and gives it
a better name!).

Signed-off-by: Jeff King <redacted>
---
 reflog-walk.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 86d1884..3549318 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c

@@ -126,7 +126,12 @@ static void add_commit_info(struct commit *commit, void *util,
 }
 
 struct commit_reflog {
-	int flag, recno;
+	int recno;
+	enum selector_type {
+		SELECTOR_NONE,
+		SELECTOR_INDEX,
+		SELECTOR_DATE
+	} selector;
 	struct complete_reflogs *reflogs;
 };
 

@@ -150,6 +155,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 	struct complete_reflogs *reflogs;
 	char *branch, *at = strchr(name, '@');
 	struct commit_reflog *commit_reflog;
+	enum selector_type selector = SELECTOR_NONE;
 
 	if (commit->object.flags & UNINTERESTING)
 		die ("Cannot walk reflogs for %s", name);

@@ -162,7 +168,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		if (*ep != '}') {
 			recno = -1;
 			timestamp = approxidate(at + 2);
+			selector = SELECTOR_DATE;
 		}
+		else
+			selector = SELECTOR_INDEX;
 	} else
 		recno = 0;
 

@@ -200,7 +209,6 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 
 	commit_reflog = xcalloc(sizeof(struct commit_reflog), 1);
 	if (recno < 0) {
-		commit_reflog->flag = 1;
 		commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
 		if (commit_reflog->recno < 0) {
 			free(branch);

@@ -209,6 +217,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		}
 	} else
 		commit_reflog->recno = reflogs->nr - recno - 1;
+	commit_reflog->selector = selector;
 	commit_reflog->reflogs = reflogs;
 
 	add_commit_info(commit, commit_reflog, &info->reflogs);

@@ -267,7 +276,7 @@ void get_reflog_selector(struct strbuf *sb,
 	}
 
 	strbuf_addf(sb, "%s@{", printed_ref);
-	if (commit_reflog->flag || dmode) {
+	if (commit_reflog->selector == SELECTOR_DATE || dmode) {
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
 	} else {

-- 
1.7.10.1.10.ge534bc3