git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Bug in git-stash(.sh) ?
@ 2012-04-27 22:57 Eli Barzilay
  2012-04-27 23:02 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Eli Barzilay @ 2012-04-27 22:57 UTC (permalink / raw)
  To: git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

[Note: cross-posted to the magit list to see if anyone else has this
problem.]

For a while now I had a problem when I try to do stash operations via
magit -- for example, it shows this in the process buffer:

  $ git --no-pager stash apply stash@{2012-04-27 08:53:30 -0400}
  Too many revisions specified: stash@{2012-04-27 08:53:30 -0400}

I tracked this down to this part of the script:

	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
	...
	set -- $REV

where $REV has one symbolic name but the name has spaces in it.  (This
was introduced two years ago, in ef76312.)

Removing the --symbolic flag could solve this but it looks like it's
needed for error reporting.  Instead, I tweaked IFS so it's split
correctly and added some quotations later in the script where $1 and
$REV are used without quotes.  (I also moved the "REV=..." line next
to the "set -- $REV", since the chunk of code between them isn't using
$REV.)

The following is the diff -- if it looks right I can send a properly
formatted patch.


-------------------------------------------------------------------------------
diff --git a/git-stash.sh b/git-stash.sh
index 4e2c7f8..10a264b 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -33,6 +33,8 @@ else
        reset_color=
 fi
 
+NEWLINE="
+"
 no_changes () {
 	git diff-index --quiet --cached HEAD --ignore-submodules -- &&
 	git diff-files --quiet --ignore-submodules &&
@@ -327,8 +329,6 @@ parse_flags_and_rev()
 	i_tree=
 	u_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
-
 	FLAGS=
 	for opt
 	do
@@ -345,7 +345,9 @@ parse_flags_and_rev()
 		esac
 	done
 
-	set -- $REV
+	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
+
+	OIFS="$IFS"; IFS="$NEWLINE"; set -- $REV; IFS="$OIFS"
 
 	case $# in
 		0)
@@ -360,13 +362,13 @@ parse_flags_and_rev()
 		;;
 	esac
 
-	REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
+	REV=$(git rev-parse --quiet --symbolic --verify "$1" 2>/dev/null) || {
 		reference="$1"
 		die "$(eval_gettext "\$reference is not valid reference")"
 	}
 
-	i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
-	set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
+	i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+	set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) &&
 	s=$1 &&
 	w_commit=$1 &&
 	b_commit=$2 &&
@@ -377,8 +379,8 @@ parse_flags_and_rev()
 	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
 	IS_STASH_REF=t
 
-	u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&
-	u_tree=$(git rev-parse $REV^3: 2>/dev/null)
+	u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+	u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
 }
 
 is_stash_like()
-------------------------------------------------------------------------------

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-27 22:57 Bug in git-stash(.sh) ? Eli Barzilay
@ 2012-04-27 23:02 ` Junio C Hamano
  2012-04-28  0:16   ` Eli Barzilay
  2012-04-28  7:47 ` Andreas Schwab
  2012-04-28 20:23 ` Yann Hodique
  2 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2012-04-27 23:02 UTC (permalink / raw)
  To: Eli Barzilay; +Cc: git

Eli Barzilay <eli-oSK4jVRJLyZg9hUCZPvPmw@public.gmane.org> writes:

> For a while now I had a problem when I try to do stash operations via
> magit -- for example, it shows this in the process buffer:
>
>   $ git --no-pager stash apply stash@{2012-04-27 08:53:30 -0400}
>   Too many revisions specified: stash@{2012-04-27 08:53:30 -0400}

Not surprised; as far as I understand, ever since the original design,
the stash entries are meant to be _counted_, i.e. stash@{0}, stash@{1},
stash@{2}, ... and never timed.

I do not mind a fix, but I would prefer a solution that does *not*
involve $IFS hack that would not work with a string with LF in it.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-27 23:02 ` Junio C Hamano
@ 2012-04-28  0:16   ` Eli Barzilay
       [not found]     ` <CALO-gut4csy5wef4iGPGD5jVPc1f0iFBfS3MUWrOwc2yczdviw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Eli Barzilay @ 2012-04-28  0:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Eli Barzilay, git

On Fri, Apr 27, 2012 at 19:02, Junio C Hamano <gitster@pobox.com> wrote:
>
> Not surprised; as far as I understand, ever since the original design,
> the stash entries are meant to be _counted_, i.e. stash@{0},
> stash@{1}, stash@{2}, ... and never timed.

Ah, that's an issue for magit, but I'd rather have it working anyway.


> I do not mind a fix, but I would prefer a solution that does *not*
> involve $IFS hack that would not work with a string with LF in it.

I didn't like that either, and I think that it's possible to avoid it by
dropping the --symbolic for that test, and re-parse if needed for an
error messag.  I'll try that and send a patch.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                  http://www.barzilay.org/                 Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-27 22:57 Bug in git-stash(.sh) ? Eli Barzilay
  2012-04-27 23:02 ` Junio C Hamano
@ 2012-04-28  7:47 ` Andreas Schwab
  2012-04-28 20:23 ` Yann Hodique
  2 siblings, 0 replies; 26+ messages in thread
From: Andreas Schwab @ 2012-04-28  7:47 UTC (permalink / raw)
  To: Eli Barzilay; +Cc: git

Eli Barzilay <eli-oSK4jVRJLyZg9hUCZPvPmw@public.gmane.org> writes:

> For a while now I had a problem when I try to do stash operations via
> magit -- for example, it shows this in the process buffer:
>
>   $ git --no-pager stash apply stash@{2012-04-27 08:53:30 -0400}
>   Too many revisions specified: stash@{2012-04-27 08:53:30 -0400}

FWIW, replacing the spaces by dots will avoid the bug.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-27 22:57 Bug in git-stash(.sh) ? Eli Barzilay
  2012-04-27 23:02 ` Junio C Hamano
  2012-04-28  7:47 ` Andreas Schwab
@ 2012-04-28 20:23 ` Yann Hodique
  2 siblings, 0 replies; 26+ messages in thread
From: Yann Hodique @ 2012-04-28 20:23 UTC (permalink / raw)
  To: git; +Cc: magit

>>>>> "Eli" == Eli Barzilay writes:

> [Note: cross-posted to the magit list to see if anyone else has
> this problem.]

> For a while now I had a problem when I try to do stash operations via
> magit -- for example, it shows this in the process buffer:

>   $ git --no-pager stash apply stash@{2012-04-27 08:53:30 -0400}
>   Too many revisions specified: stash@{2012-04-27 08:53:30 -0400}

How exactly do you make magit generate these calls?
AFAICT, Magit should operate on whatever "git stash list" outputs,
meaning stash@{N}. So I guess I'm missing something.

Yann.

-- 
The strictest limits are self-imposed.

  -- FRIEDRE GINAZ, Philosophy of the Swordmaster

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
       [not found]         ` <87wr4za9mr.fsf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2012-04-28 23:59           ` Eli Barzilay
  2012-04-29 22:01             ` Jeff King
  2012-04-29 22:07             ` Bug in git-stash(.sh) ? Junio C Hamano
  0 siblings, 2 replies; 26+ messages in thread
From: Eli Barzilay @ 2012-04-28 23:59 UTC (permalink / raw)
  To: Yann Hodique, Andreas Schwab
  Cc: Junio C Hamano, Eli Barzilay, git-u79uwXL29TY76Z2rM5mHXA,
	Eli Barzilay, magit-/JYPxA39Uh5TLH3MbocFFw

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 2404 bytes --]

Earlier today, Andreas Schwab wrote:
> 
> FWIW, replacing the spaces by dots will avoid the bug.

(Yeah, but I don't see a quick way to do that replacement without
resorting to bashisms.)


Yesterday, Eli Barzilay wrote:
> I didn't like that either, and I think that it's possible to avoid
> it by dropping the --symbolic for that test, and re-parse if needed
> for an error messag.  I'll try that and send a patch.

I'm attaching a patch rather than including it inline since it's
bigger than I thought, and since I'm not sure that it should be used.
It makes the script use $REV for sha1 revisions, and $SREV for the
--symbolic versions that were used previously.  With this, date refs
work for "stash apply" but they *don't* work for "stash drop" -- looks
like a number is required for that.

However, I thought that a much better solution is to not show the
dates to begin with, since that would make things work as expected...
The fact that things seem to be working fine for the whole world
except for me made me look into my config file, and ...

Three hours ago, Yann Hodique wrote:
> 
> How exactly do you make magit generate these calls?  AFAICT, Magit
> should operate on whatever "git stash list" outputs, meaning
> stash@{N}. So I guess I'm missing something.

... right: the offending configuration I had was log.date = iso.  This
calls for a simple chane for git-stash.sh to use `--date default':

	git log --date default --format="%gd: %gs" -g "$@" $ref_stash --

which follows.  This is independent of the other patch.  In any case,
it is also questionable -- reading the documentation for %gd:

           ·    %gD: reflog selector, e.g., refs/stash@{1}
           ·    %gd: shortened reflog selector, e.g., stash@{1}

makes it look like the problem is there -- in get_reflog_selector() --
which has explicit code for showing the dates.  (This was done in
8f8f5476.)

Another point is being able to see these dates, eg, make "stash list"
show the stash{N} and also show the dates.  It looks to me like the
date code in get_reflog_selector() should be *removed* since it can be
printed with "%cd" or "%ad" in the log line.  And it might be nicer to
add the date to the "stash list" output, something like:

	git log --date default --format="%gd: %gs (%cd)" -g "$@" $ref_stash --


This is the patch mentioned in the beginning:


[-- Attachment #2: .signature --]
[-- Type: text/plain, Size: 151 bytes --]


-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

[-- Attachment #3: srev-patch --]
[-- Type: application/octet-stream, Size: 3347 bytes --]

diff --git a/git-stash.sh b/git-stash.sh
old mode 100755
new mode 100644
index 4e2c7f8..fd59fa6
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -285,9 +285,10 @@ show_stash () {
 # stash records the work tree, and is a merge between the
 # base commit (first parent) and the index tree (second parent).
 #
-#   REV is set to the symbolic version of the specified stash-like commit
-#   IS_STASH_LIKE is non-blank if ${REV} looks like a stash
-#   IS_STASH_REF is non-blank if the ${REV} looks like a stash ref
+#   REV is set to the sha1 of the specified stash-like commit
+#   SREV is set to the its symbolic version
+#   IS_STASH_LIKE is non-blank if ${SREV} looks like a stash
+#   IS_STASH_REF is non-blank if the ${SREV} looks like a stash ref
 #   s is set to the SHA1 of the stash commit
 #   w_commit is set to the commit containing the working tree
 #   b_commit is set to the base commit
@@ -327,8 +328,6 @@ parse_flags_and_rev()
 	i_tree=
 	u_tree=
 
-	REV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
-
 	FLAGS=
 	for opt
 	do
@@ -345,6 +344,9 @@ parse_flags_and_rev()
 		esac
 	done
 
+	REV=$(git rev-parse --no-flags "$@") || exit 1
+	SREV=$(git rev-parse --no-flags --symbolic "$@") || exit 1
+
 	set -- $REV
 
 	case $# in
@@ -356,17 +358,17 @@ parse_flags_and_rev()
 			:
 		;;
 		*)
-			die "$(eval_gettext "Too many revisions specified: \$REV")"
+			die "$(eval_gettext "Too many revisions specified: \$SREV")"
 		;;
 	esac
 
-	REV=$(git rev-parse --quiet --symbolic --verify $1 2>/dev/null) || {
-		reference="$1"
+	reference=$SREV
+	SREV=$(git rev-parse --quiet --symbolic --verify "$SREV" 2>/dev/null) || {
 		die "$(eval_gettext "\$reference is not valid reference")"
 	}
 
-	i_commit=$(git rev-parse --quiet --verify $REV^2 2>/dev/null) &&
-	set -- $(git rev-parse $REV $REV^1 $REV: $REV^1: $REV^2: 2>/dev/null) &&
+	i_commit=$(git rev-parse --quiet --verify "$REV^2" 2>/dev/null) &&
+	set -- $(git rev-parse "$REV" "$REV^1" "$REV:" "$REV^1:" "$REV^2:" 2>/dev/null) &&
 	s=$1 &&
 	w_commit=$1 &&
 	b_commit=$2 &&
@@ -374,11 +376,11 @@ parse_flags_and_rev()
 	b_tree=$4 &&
 	i_tree=$5 &&
 	IS_STASH_LIKE=t &&
-	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${REV%@*}")" &&
+	test "$ref_stash" = "$(git rev-parse --symbolic-full-name "${SREV%@*}")" &&
 	IS_STASH_REF=t
 
-	u_commit=$(git rev-parse --quiet --verify $REV^3 2>/dev/null) &&
-	u_tree=$(git rev-parse $REV^3: 2>/dev/null)
+	u_commit=$(git rev-parse --quiet --verify "$REV^3" 2>/dev/null) &&
+	u_tree=$(git rev-parse "$REV^3:" 2>/dev/null)
 }
 
 is_stash_like()
@@ -487,9 +489,9 @@ pop_stash() {
 drop_stash () {
 	assert_stash_ref "$@"
 
-	git reflog delete --updateref --rewrite "${REV}" &&
-		say "$(eval_gettext "Dropped \${REV} (\$s)")" ||
-		die "$(eval_gettext "\${REV}: Could not drop stash entry")"
+	git reflog delete --updateref --rewrite "${SREV}" &&
+		say "$(eval_gettext "Dropped \${SREV} (\$s)")" ||
+		die "$(eval_gettext "\${SREV}: Could not drop stash entry")"
 
 	# clear_stash if we just dropped the last stash entry
 	git rev-parse --verify "$ref_stash@{0}" >/dev/null 2>&1 || clear_stash
@@ -503,7 +505,7 @@ apply_to_branch () {
 	set -- --index "$@"
 	assert_stash_like "$@"
 
-	git checkout -b $branch $REV^ &&
+	git checkout -b "$branch" "$REV^" &&
 	apply_stash "$@" && {
 		test -z "$IS_STASH_REF" || drop_stash "$@"
 	}

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-28 23:59           ` Eli Barzilay
@ 2012-04-29 22:01             ` Jeff King
       [not found]               ` <20120429220132.GB4491-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  2012-04-29 22:07             ` Bug in git-stash(.sh) ? Junio C Hamano
  1 sibling, 1 reply; 26+ messages in thread
From: Jeff King @ 2012-04-29 22:01 UTC (permalink / raw)
  To: Eli Barzilay
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano, Eli Barzilay, git,
	Eli Barzilay, magit

On Sat, Apr 28, 2012 at 07:59:37PM -0400, Eli Barzilay wrote:

> > How exactly do you make magit generate these calls?  AFAICT, Magit
> > should operate on whatever "git stash list" outputs, meaning
> > stash@{N}. So I guess I'm missing something.
> 
> ... right: the offending configuration I had was log.date = iso.  This
> calls for a simple chane for git-stash.sh to use `--date default':
> 
> 	git log --date default --format="%gd: %gs" -g "$@" $ref_stash --

I seem to remember dealing with this once a long time ago. And while
"--date=default" works, it is papering over the symptom of a larger
problem, which is that "log" should not use a non-commandline date to
make the stash selector decision. Searching turned up this discussion:

  http://thread.gmane.org/gmane.comp.version-control.git/128569

which led to f4ea32f (improve reflog date/number heuristic, 2009-09-24).
That fixed the case of:

  git config log.date iso
  git log -g --oneline

But later, 8f8f547 (Introduce new pretty formats %g[sdD] for reflog
information, 2009-10-19) added another way to show selectors, and it did
not respect the date_mode_explicit flag from f4ea32f. Which I think is a
bug.

So the right solution is to pass the date_mode_explicit flag through to
the pretty-print --format code, and then pass it along to the reflog
code.

> Another point is being able to see these dates, eg, make "stash list"
> show the stash{N} and also show the dates.

You can do so with:

  git stash list --date=iso

but there is no way to do it automatically via config (and indeed, you
can see that it creates problems for scripts when you do so. :) ).

> It looks to me like the date code in get_reflog_selector() should be
> *removed* since it can be printed with "%cd" or "%ad" in the log line.

No, all three are distinct dates. For example, from my git.git reflog:

  $ git log -g --format='%gd / %cd / %ad' --date=short
  HEAD@{2012-04-29} / 2009-09-29 / 2009-09-24

That's a commit (which happens to be f4ea32f) that was written on
2009-09-24 (author date), sent as a patch to the list and applied
upstream on 2009-09-29 (committer date), and reached my HEAD reflog via
"git checkout f4ea32f" three years later.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-28 23:59           ` Eli Barzilay
  2012-04-29 22:01             ` Jeff King
@ 2012-04-29 22:07             ` Junio C Hamano
       [not found]               ` <7vlilexkcq.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
  1 sibling, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2012-04-29 22:07 UTC (permalink / raw)
  To: Eli Barzilay, Thomas Rast
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano, Eli Barzilay,
	git-u79uwXL29TY76Z2rM5mHXA, Eli Barzilay,
	magit-/JYPxA39Uh5TLH3MbocFFw

Eli Barzilay <eli-oSK4jVRJLyZg9hUCZPvPmw@public.gmane.org> writes:

> ...  In any case,
> it is also questionable -- reading the documentation for %gd:
>
>            ·    %gD: reflog selector, e.g., refs/stash@{1}
>            ·    %gd: shortened reflog selector, e.g., stash@{1}
>
> makes it look like the problem is there -- in get_reflog_selector() --
> which has explicit code for showing the dates.  (This was done in
> 8f8f5476.)

I think the root cause of the bug is that there are three cases:

 - If we ask for "log -g ref@{0}", we should show them counted no matter what.

 - If we ask for "log -g ref@{now}", we should show them timed no matter what.

 - If we ask for "log -g ref" without specifier, we show them counted by
   default, but we try to be nice and show them timed when we can infer
   from other context that the user wanted to see them timed.

An ancient 4e244cb (log --reflog: honour --relative-date, 2007-02-08) was
what introduced the "explicit code for showing the dates", but it was done
somewhat poorly---it does not differentiate the first and third case.

Once we fix *that* bug, to disable the "timed" codepath altogether when
the caller gives "ref@{0}" to explicitly ask for counted output, we can
fix it a lot easily.

And the patch to do so should look like this; I'll leave it to the readers
to add whatever tests that are appropriate.

 git-stash.sh  |    2 +-
 reflog-walk.c |   16 ++++++++++++----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index fe4ab28..590c1f3 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -265,7 +265,7 @@ have_stash () {
 
 list_stash () {
 	have_stash || return 0
-	git log --format="%gd: %gs" -g "$@" $ref_stash --
+	git log --format="%gd: %gs" -g "$@" "$ref_stash@{0}" --
 }
 
 show_stash () {
diff --git a/reflog-walk.c b/reflog-walk.c
index 86d1884..6fe60a8 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -126,7 +126,10 @@ static void add_commit_info(struct commit *commit, void *util,
 }
 
 struct commit_reflog {
-	int flag, recno;
+	int recno;
+#define REFLOG_COUNTED 01
+#define REFLOG_TIMED   02
+	unsigned flags;
 	struct complete_reflogs *reflogs;
 };
 
@@ -150,6 +153,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;
+	unsigned flags = 0;
 
 	if (commit->object.flags & UNINTERESTING)
 		die ("Cannot walk reflogs for %s", name);
@@ -162,6 +166,9 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 		if (*ep != '}') {
 			recno = -1;
 			timestamp = approxidate(at + 2);
+			flags = REFLOG_TIMED;
+		} else {
+			flags = REFLOG_COUNTED;
 		}
 	} else
 		recno = 0;
@@ -199,8 +206,8 @@ 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->flags = flags;
+	if (flags & REFLOG_TIMED) {
 		commit_reflog->recno = get_reflog_recno_by_time(reflogs, timestamp);
 		if (commit_reflog->recno < 0) {
 			free(branch);
@@ -267,7 +274,8 @@ void get_reflog_selector(struct strbuf *sb,
 	}
 
 	strbuf_addf(sb, "%s@{", printed_ref);
-	if (commit_reflog->flag || dmode) {
+	if ((! (commit_reflog->flags && (REFLOG_COUNTED | REFLOG_TIMED)) && dmode) ||
+	    (commit_reflog->flags & REFLOG_TIMED)) {
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
 	} else {

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
       [not found]               ` <20120429220132.GB4491-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-04-29 22:26                 ` Eli Barzilay
  2012-05-01 13:42                   ` Jeff King
  0 siblings, 1 reply; 26+ messages in thread
From: Eli Barzilay @ 2012-04-29 22:26 UTC (permalink / raw)
  To: Jeff King
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano, Eli Barzilay,
	git-u79uwXL29TY76Z2rM5mHXA, Eli Barzilay,
	magit-/JYPxA39Uh5TLH3MbocFFw

A few minutes ago, Jeff King wrote:
> On Sat, Apr 28, 2012 at 07:59:37PM -0400, Eli Barzilay wrote:
> 
> > > How exactly do you make magit generate these calls?  AFAICT, Magit
> > > should operate on whatever "git stash list" outputs, meaning
> > > stash@{N}. So I guess I'm missing something.
> > 
> > ... right: the offending configuration I had was log.date = iso.  This
> > calls for a simple chane for git-stash.sh to use `--date default':
> > 
> > 	git log --date default --format="%gd: %gs" -g "$@" $ref_stash --
> 
> I seem to remember dealing with this once a long time ago. And while
> "--date=default" works, it is papering over the symptom of a larger
> problem, which is that "log" should not use a non-commandline date
> to make the stash selector decision. Searching turned up this
> discussion:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/128569

Ah, that looks like almost exactly the problem I started with...


> which led to f4ea32f (improve reflog date/number heuristic,
> 2009-09-24).  That fixed the case of:
> 
>   git config log.date iso
>   git log -g --oneline
> 
> But later, 8f8f547 (Introduce new pretty formats %g[sdD] for reflog
> information, 2009-10-19) added another way to show selectors, and it
> did not respect the date_mode_explicit flag from f4ea32f. Which I
> think is a bug.
> 
> So the right solution is to pass the date_mode_explicit flag through
> to the pretty-print --format code, and then pass it along to the
> reflog code.

Assuming that I followed all of that correctly, it still seems bogus
to do that, given that %gd and %gD are described as producing reflog
selector, and given that Junio's note that stash operations are really
intended to be used only with these selectos.  What looks more
sensible to me given the necessity of %gd (and the fact that it's
different from %cd/%ad) is to change things as follows:

  * %gd produces only the date, with the "default" having the same
    meaning as elsewhere (so it doesn't show the index numbers)
  * %gD is useless
  * 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)"

With this the output has the number independent of log.date setting,
and I get a --format if I want to see something else, which makes more
sense than --date being explicit or not.  IOW, I'd expect this:

>   git stash list --date=iso

to not have any effect.

This is not a backwards compatible change, but my guess is that
existing uses of %g[dD] are suffering from a similar problem anyway.
(So another option maybe making %gd use the number and something else
for the date version.)

(But my opinion is of course limited to my short encounter with all of
this...)

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
       [not found]               ` <7vlilexkcq.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
@ 2012-04-29 22:37                 ` Eli Barzilay
  2012-05-01 15:02                 ` Jeff King
  1 sibling, 0 replies; 26+ messages in thread
From: Eli Barzilay @ 2012-04-29 22:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Thomas Rast, Yann Hodique, Andreas Schwab,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

30 minutes ago, Junio C Hamano wrote:
> 
> I think the root cause of the bug is that there are three cases:
> 
>  - If we ask for "log -g ref@{0}", we should show them counted no
>    matter what.
> 
>  - If we ask for "log -g ref@{now}", we should show them timed no
>    matter what.
> 
>  - If we ask for "log -g ref" without specifier, we show them
>    counted by default, but we try to be nice and show them timed
>    when we can infer from other context that the user wanted to see
>    them timed.

Ah, I was unaware (unsurprisingly) that *that's* how an explict date
format is (supposed to?) requested -- but then what happens with

  git log --date iso -g "ref@{0}"

?

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
  2012-04-29 22:26                 ` Eli Barzilay
@ 2012-05-01 13:42                   ` Jeff King
       [not found]                     ` <20120501134254.GA11900-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2012-05-01 13:42 UTC (permalink / raw)
  To: Eli Barzilay
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano, Eli Barzilay, git,
	Eli Barzilay, magit

On Sun, Apr 29, 2012 at 06:26:36PM -0400, Eli Barzilay wrote:

> > which led to f4ea32f (improve reflog date/number heuristic,
> > 2009-09-24).  That fixed the case of:
> > 
> >   git config log.date iso
> >   git log -g --oneline
> > 
> > But later, 8f8f547 (Introduce new pretty formats %g[sdD] for reflog
> > information, 2009-10-19) added another way to show selectors, and it
> > did not respect the date_mode_explicit flag from f4ea32f. Which I
> > think is a bug.
> > 
> > So the right solution is to pass the date_mode_explicit flag through
> > to the pretty-print --format code, and then pass it along to the
> > reflog code.
> 
> Assuming that I followed all of that correctly, it still seems bogus
> to do that, given that %gd and %gD are described as producing reflog
> selector, and given that Junio's note that stash operations are really
> intended to be used only with these selectos.

Keep in mind this bug is not about stash at all; it is about showing
reflog selectors. Those are a more general mechanism, and are used for
more than just stash. The fact that user config affects the format of
"%gd" is a bug; it should follow the same rules as the regular reflog
pretty-printing (and the behavior of neither should be affected by user
config, as scripts rely on the output being consistent).

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).

> What looks more sensible to me given the necessity of %gd (and the
> fact that it's different from %cd/%ad) is to change things as follows:
> 
>   * %gd produces only the date, with the "default" having the same
>     meaning as elsewhere (so it doesn't show the index numbers)

%gd is part of the public interface and will not change its semantics
(or at least not without a long deprecation period).  It's a shame that
"d" is taken for the selector, when it would be better to mean "date" as
it does for author and committer. But I don't know if it's worth
changing at this point.

We could add new placeholders with different semantics, though. When I
added reflog identity placeholders a few months ago, there was a brief
discussion on adding a date placeholder:

  http://article.gmane.org/gmane.comp.version-control.git/185043

but the related work hasn't progressed.

>   * 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.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: Bug in git-stash(.sh) ?
       [not found]               ` <7vlilexkcq.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
  2012-04-29 22:37                 ` Eli Barzilay
@ 2012-05-01 15:02                 ` Jeff King
  1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2012-05-01 15:02 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eli Barzilay, Thomas Rast, Yann Hodique, Andreas Schwab,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

On Sun, Apr 29, 2012 at 03:07:49PM -0700, Junio C Hamano wrote:

> Eli Barzilay <eli-oSK4jVRJLyZg9hUCZPvPmw@public.gmane.org> writes:
> 
> > ...  In any case,
> > it is also questionable -- reading the documentation for %gd:
> >
> >            ·    %gD: reflog selector, e.g., refs/stash@{1}
> >            ·    %gd: shortened reflog selector, e.g., stash@{1}
> >
> > makes it look like the problem is there -- in get_reflog_selector() --
> > which has explicit code for showing the dates.  (This was done in
> > 8f8f5476.)
> 
> I think the root cause of the bug is that there are three cases:
> 
>  - If we ask for "log -g ref@{0}", we should show them counted no matter what.
> 
>  - If we ask for "log -g ref@{now}", we should show them timed no matter what.
> 
>  - If we ask for "log -g ref" without specifier, we show them counted by
>    default, but we try to be nice and show them timed when we can infer
>    from other context that the user wanted to see them timed.

Right. My argument is that the context in your third point was always
intended to be about command-line options. Respecting the log.date
config there is a bug (and not just in breaking intent; it also breaks
scriptability). It was fixed for the regular pretty-print code path, but
was broken again when the "%gd" code path was added.

> An ancient 4e244cb (log --reflog: honour --relative-date, 2007-02-08) was
> what introduced the "explicit code for showing the dates", but it was done
> somewhat poorly---it does not differentiate the first and third case.

If that is the case (and I haven't checked either way, but it does not
surprise me at all), then I believe that is a separate bug. And we
should fix that, too.

> Once we fix *that* bug, to disable the "timed" codepath altogether when
> the caller gives "ref@{0}" to explicitly ask for counted output, we can
> fix it a lot easily.
> [...]
> -	git log --format="%gd: %gs" -g "$@" $ref_stash --
> +	git log --format="%gd: %gs" -g "$@" "$ref_stash@{0}" --

That will solve the problem for stash, but the config bug would remain
for every _other_ user of "git log -g --format=%gd". So that needs fixed
either way.

However, I really wonder if this is the right thing. If I do:

  git stash list --date=relative

isn't it a feature that I get to see the date at which each stash was
made? Why are we taking it away? I can see if it were the only way to
fix the problem with log.date, but that has another solution. Are people
really calling "stash list" with a date on the command line and getting
confused by the output? My understanding was that the observed problem
was purely a bad interaction with log.date, which should not be
respected at all.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [git] Re: Bug in git-stash(.sh) ?
       [not found]                     ` <20120501134254.GA11900-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-03 18:44                       ` Eli Barzilay
       [not found]                         ` <20386.53745.200846.115335-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Eli Barzilay @ 2012-05-03 18:44 UTC (permalink / raw)
  To: Jeff King
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano, Eli Barzilay,
	git-u79uwXL29TY76Z2rM5mHXA, Eli Barzilay,
	magit-/JYPxA39Uh5TLH3MbocFFw

Two days ago, Jeff King wrote:
> On Sun, Apr 29, 2012 at 06:26:36PM -0400, Eli Barzilay wrote:
> 
> > > which led to f4ea32f (improve reflog date/number heuristic,
> > > 2009-09-24).  That fixed the case of:
> > > 
> > >   git config log.date iso
> > >   git log -g --oneline
> > > 
> > > But later, 8f8f547 (Introduce new pretty formats %g[sdD] for reflog
> > > information, 2009-10-19) added another way to show selectors, and it
> > > did not respect the date_mode_explicit flag from f4ea32f. Which I
> > > think is a bug.
> > > 
> > > So the right solution is to pass the date_mode_explicit flag through
> > > to the pretty-print --format code, and then pass it along to the
> > > reflog code.
> > 
> > Assuming that I followed all of that correctly, it still seems bogus
> > to do that, given that %gd and %gD are described as producing reflog
> > selector, and given that Junio's note that stash operations are really
> > intended to be used only with these selectos.
> 
> Keep in mind this bug is not about stash at all; it is about showing
> reflog selectors. Those are a more general mechanism, and are used for
> more than just stash. The fact that user config affects the format of
> "%gd" is a bug; it should follow the same rules as the regular reflog
> pretty-printing (and the behavior of neither should be affected by user
> config, as scripts rely on the output being consistent).
> 
> 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.


> > What looks more sensible to me given the necessity of %gd (and the
> > fact that it's different from %cd/%ad) is to change things as
> > follows:
> > 
> >   * %gd produces only the date, with the "default" having the same
> >     meaning as elsewhere (so it doesn't show the index numbers)
> 
> %gd is part of the public interface and will not change its semantics
> (or at least not without a long deprecation period).  It's a shame
> that "d" is taken for the selector, when it would be better to mean
> "date" as it does for author and committer. But I don't know if it's
> worth changing at this point.

(Yeah, I can see that.)


> >   * 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)...

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [git] Re: Bug in git-stash(.sh) ?
       [not found]                         ` <20386.53745.200846.115335-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
@ 2012-05-04  5:21                           ` Jeff King
       [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  2012-05-04  5:26                             ` [PATCH 3/4] reflog-walk: clean up "flag" field of commit_reflog struct Jeff King
  0 siblings, 2 replies; 26+ messages in thread
From: Jeff King @ 2012-05-04  5:21 UTC (permalink / raw)
  To: Eli Barzilay
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

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

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

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

^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH 1/4] t1411: add more selector index/date tests
       [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-04  5:23                               ` Jeff King
  2012-05-04  5:25                               ` [PATCH 2/4] log: respect date_mode_explicit with --format:%gd Jeff King
                                                 ` (2 subsequent siblings)
  3 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2012-05-04  5:23 UTC (permalink / raw)
  To: Eli Barzilay
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

We already check that @{now} and "--date" cause the
displayed selector to use the date for both the multiline
and oneline formats. However, we miss several cases:

  1. The --format=%gd selector is not tested at all.

  2. We do not check how the log.date config interacts with the
     "--date" magic (according to f4ea32f, it should not
     impact the output).

Doing so reveals that the combination of both (log.date
combined with the %gd format) does not behave as expected.

Signed-off-by: Jeff King <peff-AdEPDUrAXsQ@public.gmane.org>
---
This takes us up to 9 tests (3 cases by 3 formats). It's almost enough
to make me want to write loops, but I think the boilerplate would end up
just making it more confusing to read.

 t/t1411-reflog-show.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index caa687b..4706f4c 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -65,6 +65,14 @@ test_expect_success 'using @{now} syntax shows reflog date (oneline)' '
 '
 
 cat >expect <<'EOF'
+HEAD@{Thu Apr 7 15:13:13 2005 -0700}
+EOF
+test_expect_success 'using @{now} syntax shows reflog date (format=%gd)' '
+	git log -g -1 --format=%gd HEAD@{now} >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
 Reflog: HEAD@{1112911993 -0700} (C O Mitter <committer-hcDgGtZH8xNBDgjK7y7TUQ@public.gmane.org>)
 Reflog message: commit (initial): one
 EOF
@@ -82,6 +90,43 @@ test_expect_success 'using --date= shows reflog date (oneline)' '
 	test_cmp expect actual
 '
 
+cat >expect <<'EOF'
+HEAD@{1112911993 -0700}
+EOF
+test_expect_success 'using --date= shows reflog date (format=%gd)' '
+	git log -g -1 --format=%gd --date=raw >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+Reflog: HEAD@{0} (C O Mitter <committer-hcDgGtZH8xNBDgjK7y7TUQ@public.gmane.org>)
+Reflog message: commit (initial): one
+EOF
+test_expect_success 'log.date does not invoke "--date" magic (multiline)' '
+	test_config log.date raw &&
+	git log -g -1 >tmp &&
+	grep ^Reflog <tmp >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+e46513e HEAD@{0}: commit (initial): one
+EOF
+test_expect_success 'log.date does not invoke "--date" magic (oneline)' '
+	test_config log.date raw &&
+	git log -g -1 --oneline >actual &&
+	test_cmp expect actual
+'
+
+cat >expect <<'EOF'
+HEAD@{0}
+EOF
+test_expect_failure 'log.date does not invoke "--date" magic (format=%gd)' '
+	test_config log.date raw &&
+	git log -g -1 --format=%gd >actual &&
+	test_cmp expect actual
+'
+
 : >expect
 test_expect_success 'empty reflog file' '
 	git branch empty &&
-- 
1.7.10.1.10.ge534bc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 2/4] log: respect date_mode_explicit with --format:%gd
       [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  2012-05-04  5:23                               ` [PATCH 1/4] t1411: add more selector index/date tests Jeff King
@ 2012-05-04  5:25                               ` Jeff King
  2012-05-04  5:27                               ` [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors Jeff King
  2012-05-04 18:57                               ` [git] Re: Bug in git-stash(.sh) ? Eli Barzilay
  3 siblings, 0 replies; 26+ messages in thread
From: Jeff King @ 2012-05-04  5:25 UTC (permalink / raw)
  To: Eli Barzilay
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

When we show a reflog selector (e.g., via "git log -g"), we
perform some DWIM magic: while we normally show the entry's
index (e.g., HEAD@{1}), if the user has given us a date
with "--date", then we show a date-based select (e.g.,
HEAD@{yesterday}).

However, we don't want to trigger this magic if the
alternate date format we got was from the "log.date"
configuration; that is not sufficiently strong context for
us to invoke this particular magic. To fix this, commit
f4ea32f (improve reflog date/number heuristic, 2009-09-24)
introduced a "date_mode_explicit" flag in rev_info. This
flag is set only when we see a "--date" option on the
command line, and we a vanilla date to the reflog code if
the date was not explicit.

Later, commit 8f8f547 (Introduce new pretty formats %g[sdD]
for reflog information, 2009-10-19) added another way to
show selectors, and it did not respect the date_mode_explicit
flag from f4ea32f.

This patch propagates the date_mode_explicit flag to the
pretty-print code, which can then use it to pass the
appropriate date field to the reflog code. This brings the
behavior of "%gd" in line with the other formats, and means
that its output is independent of any user configuration.

Signed-off-by: Jeff King <peff-AdEPDUrAXsQ@public.gmane.org>
---
I'm not happy that users of pretty_print_context have to manually
remember to copy in the date_mode_explicit flag; it would be nice if it
just came with the date_mode field for free. But that would mean
changing the type of date_mode to hold the extra bit, and would disrupt
callers all over the code base.

 builtin/rev-list.c     | 1 +
 commit.h               | 1 +
 log-tree.c             | 1 +
 pretty.c               | 4 +++-
 t/t1411-reflog-show.sh | 2 +-
 5 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 4c4d404..ff5a383 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -109,6 +109,7 @@ static void show_commit(struct commit *commit, void *data)
 		struct pretty_print_context ctx = {0};
 		ctx.abbrev = revs->abbrev;
 		ctx.date_mode = revs->date_mode;
+		ctx.date_mode_explicit = revs->date_mode_explicit;
 		ctx.fmt = revs->commit_format;
 		pretty_print_commit(&ctx, commit, &buf);
 		if (revs->graph) {
diff --git a/commit.h b/commit.h
index ccaa20b..d617fa3 100644
--- a/commit.h
+++ b/commit.h
@@ -84,6 +84,7 @@ struct pretty_print_context {
 	const char *after_subject;
 	int preserve_subject;
 	enum date_mode date_mode;
+	unsigned date_mode_explicit:1;
 	int need_8bit_cte;
 	int show_notes;
 	struct reflog_walk_info *reflog_info;
diff --git a/log-tree.c b/log-tree.c
index 34c49e7..634f142 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -652,6 +652,7 @@ void show_log(struct rev_info *opt)
 	if (ctx.need_8bit_cte >= 0)
 		ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
 	ctx.date_mode = opt->date_mode;
+	ctx.date_mode_explicit = opt->date_mode_explicit;
 	ctx.abbrev = opt->diffopt.abbrev;
 	ctx.after_subject = extra_headers;
 	ctx.preserve_subject = opt->preserve_subject;
diff --git a/pretty.c b/pretty.c
index f2dee30..2bc64b3 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1009,7 +1009,9 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 			if (c->pretty_ctx->reflog_info)
 				get_reflog_selector(sb,
 						    c->pretty_ctx->reflog_info,
-						    c->pretty_ctx->date_mode,
+						    c->pretty_ctx->date_mode_explicit ?
+						      c->pretty_ctx->date_mode :
+						      DATE_NORMAL,
 						    (placeholder[1] == 'd'));
 			return 2;
 		case 's':	/* reflog message */
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 4706f4c..88247f8 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -121,7 +121,7 @@ test_expect_success 'log.date does not invoke "--date" magic (oneline)' '
 cat >expect <<'EOF'
 HEAD@{0}
 EOF
-test_expect_failure 'log.date does not invoke "--date" magic (format=%gd)' '
+test_expect_success 'log.date does not invoke "--date" magic (format=%gd)' '
 	test_config log.date raw &&
 	git log -g -1 --format=%gd >actual &&
 	test_cmp expect actual
-- 
1.7.10.1.10.ge534bc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 3/4] reflog-walk: clean up "flag" field of commit_reflog struct
  2012-05-04  5:21                           ` Jeff King
       [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-04  5:26                             ` Jeff King
  1 sibling, 0 replies; 26+ messages in thread
From: Jeff King @ 2012-05-04  5:26 UTC (permalink / raw)
  To: Eli Barzilay; +Cc: Yann Hodique, Andreas Schwab, Junio C Hamano, git, magit

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 <peff@peff.net>
---
 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

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
       [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  2012-05-04  5:23                               ` [PATCH 1/4] t1411: add more selector index/date tests Jeff King
  2012-05-04  5:25                               ` [PATCH 2/4] log: respect date_mode_explicit with --format:%gd Jeff King
@ 2012-05-04  5:27                               ` Jeff King
       [not found]                                 ` <20120504052725.GD16107-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  2012-05-04 18:57                               ` [git] Re: Bug in git-stash(.sh) ? Eli Barzilay
  3 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2012-05-04  5:27 UTC (permalink / raw)
  To: Eli Barzilay
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

When we are showing reflog selectors during a walk, we infer
from context whether the user wanted to see the index in
each selector, or the reflog date. The current rules are:

  1. if the user asked for an explicit date format in the
     output, show the date

  2. if the user asked for ref@{now}, show the date

  3. if neither is true, show the index

However,  if we see "ref@{0}", that should be a strong clue
that the user wants to see the counted version. In fact, it
should be much stronger than the date format in (1). The
user may have been setting the date format to use in another
part of the output (e.g., in --format="%gd (%ad)", they may
have wanted to influence the author date).

This patch flips the rules to:

  1. if the user asked for ref@{0}, always show the index

  2. if the user asked for ref@{now}, always show the date

  3. otherwise, we have just "ref"; show them counted by
     default, but respect the presence of "--date" as a clue
     that the user wanted them date-based

Signed-off-by: Jeff King <peff-AdEPDUrAXsQ@public.gmane.org>
---
 reflog-walk.c          | 3 ++-
 t/t1411-reflog-show.sh | 8 ++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index 3549318..b974258 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -276,7 +276,8 @@ void get_reflog_selector(struct strbuf *sb,
 	}
 
 	strbuf_addf(sb, "%s@{", printed_ref);
-	if (commit_reflog->selector == SELECTOR_DATE || dmode) {
+	if (commit_reflog->selector == SELECTOR_DATE ||
+	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
 	} else {
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 88247f8..7d9b5e3 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -127,6 +127,14 @@ test_expect_success 'log.date does not invoke "--date" magic (format=%gd)' '
 	test_cmp expect actual
 '
 
+cat >expect <<'EOF'
+HEAD@{0}
+EOF
+test_expect_success '--date magic does not override explicit @{0} syntax' '
+	git log -g -1 --format=%gd --date=raw HEAD@{0} >actual &&
+	test_cmp expect actual
+'
+
 : >expect
 test_expect_success 'empty reflog file' '
 	git branch empty &&
-- 
1.7.10.1.10.ge534bc3

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
       [not found]                                 ` <20120504052725.GD16107-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-04 17:02                                   ` Junio C Hamano
       [not found]                                     ` <7v7gwrc212.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2012-05-04 17:02 UTC (permalink / raw)
  To: Jeff King
  Cc: Eli Barzilay, Yann Hodique, Andreas Schwab,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

Jeff King <peff-AdEPDUrAXsQ@public.gmane.org> writes:

> This patch flips the rules to:
>
>   1. if the user asked for ref@{0}, always show the index
>
>   2. if the user asked for ref@{now}, always show the date
>
>   3. otherwise, we have just "ref"; show them counted by
>      default, but respect the presence of "--date" as a clue
>      that the user wanted them date-based

The revision.c parser for "git log --date=default -g master" would flip
the "explicit" bit, revs->date_mode is set to DATE_NORMAL, and that value
will eventually come as dmode here.

> diff --git a/reflog-walk.c b/reflog-walk.c
> index 3549318..b974258 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -276,7 +276,8 @@ void get_reflog_selector(struct strbuf *sb,
>  	}
>  
>  	strbuf_addf(sb, "%s@{", printed_ref);
> -	if (commit_reflog->selector == SELECTOR_DATE || dmode) {
> +	if (commit_reflog->selector == SELECTOR_DATE ||
> +	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
>  		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
>  		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));

But DATE_NORMAL happens to be zero ;-) "git log --date=default -g master"
would still show the counted version.

I personally do not care about that behaviour, but I know that I will
later later have to deal with people who do care, which is annoying.

Probably we would internally need to define two values to ask for the
DATE_NORMAL output.  Move DATE_NORMAL to non-zero value, introduce a new
DATE_DEFAULT that is zero, and make their output identical, perhaps
something like the attached (not even compile tested).

The implicit comparison to zero in the above is a bad code (but that is
a problem from the very old days).

diff --git a/cache.h b/cache.h
index 58ff054..fe42e80 100644
--- a/cache.h
+++ b/cache.h
@@ -876,7 +876,8 @@ extern struct object *peel_to_type(const char *name, int namelen,
 				   struct object *o, enum object_type);
 
 enum date_mode {
-	DATE_NORMAL = 0,
+	DATE_DEFAULT = 0,
+	DATE_NORMAL,
 	DATE_RELATIVE,
 	DATE_SHORT,
 	DATE_LOCAL,
diff --git a/reflog-walk.c b/reflog-walk.c
index b974258..d002516 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -277,7 +277,7 @@ void get_reflog_selector(struct strbuf *sb,
 
 	strbuf_addf(sb, "%s@{", printed_ref);
 	if (commit_reflog->selector == SELECTOR_DATE ||
-	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
+	    (commit_reflog->selector == SELECTOR_NONE && (dmode != DATE_DEFAULT))) {
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
 	} else {

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [git] Re: Bug in git-stash(.sh) ?
       [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
                                                 ` (2 preceding siblings ...)
  2012-05-04  5:27                               ` [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors Jeff King
@ 2012-05-04 18:57                               ` Eli Barzilay
       [not found]                                 ` <20388.9885.608325.489624-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
  3 siblings, 1 reply; 26+ messages in thread
From: Eli Barzilay @ 2012-05-04 18:57 UTC (permalink / raw)
  To: Jeff King
  Cc: Yann Hodique, Andreas Schwab, Junio C Hamano,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

Earlier today, Jeff King wrote:
> On Thu, May 03, 2012 at 02:44:01PM -0400, Eli Barzilay wrote:
> 
> > > 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. :)

Apologies -- it wasn't clear to me what should be done to address the
general problem, and I don't think that I'd do a good job with that
anyway.

I can still make a proper patch with the fix for git-stash.sh that
avoids the problem with the spaces.

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [git] Re: Bug in git-stash(.sh) ?
       [not found]                                 ` <20388.9885.608325.489624-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
@ 2012-05-04 22:36                                   ` Eli Barzilay
  0 siblings, 0 replies; 26+ messages in thread
From: Eli Barzilay @ 2012-05-04 22:36 UTC (permalink / raw)
  To: Jeff King, Yann Hodique, Andreas Schwab, Junio C Hamano,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

Four hours ago, Eli Barzilay wrote:
> 
> I can still make a proper patch with the fix for git-stash.sh that
> avoids the problem with the spaces.

Here's another idea for an easier patch that instead of fixing the
issue with space just spits out a better error: before throwing the
"Too many revisions specified" error, check if "$*" matches "{.*}"
and if "$1" matches "{[^}]*" and in that case make the error say that
spaces are not supported.  The change is therefore only in the error,
and with a fixed output, that should be enough.

(Please tell me if the above or this is desirable...)

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
       [not found]                                     ` <7v7gwrc212.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
@ 2012-05-07 21:37                                       ` Jeff King
       [not found]                                         ` <20120507213752.GA19911-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2012-05-07 21:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eli Barzilay, Yann Hodique, Andreas Schwab,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

On Fri, May 04, 2012 at 10:02:49AM -0700, Junio C Hamano wrote:

> Jeff King <peff-AdEPDUrAXsQ@public.gmane.org> writes:
> 
> > This patch flips the rules to:
> >
> >   1. if the user asked for ref@{0}, always show the index
> >
> >   2. if the user asked for ref@{now}, always show the date
> >
> >   3. otherwise, we have just "ref"; show them counted by
> >      default, but respect the presence of "--date" as a clue
> >      that the user wanted them date-based
> 
> The revision.c parser for "git log --date=default -g master" would flip
> the "explicit" bit, revs->date_mode is set to DATE_NORMAL, and that value
> will eventually come as dmode here.
> [...]
> But DATE_NORMAL happens to be zero ;-) "git log --date=default -g master"
> would still show the counted version.

Yeah, I noticed that, but decided not to tackle it, as nobody had really
complained (and you can get the behavior you want with master@{now}).
However, I agree it would be better for "--date=default" to trigger the
date-based selector.

> I personally do not care about that behaviour, but I know that I will
> later later have to deal with people who do care, which is annoying.

Maybe. It has been that way for years and nobody has yet complained. :)

> Probably we would internally need to define two values to ask for the
> DATE_NORMAL output.  Move DATE_NORMAL to non-zero value, introduce a new
> DATE_DEFAULT that is zero, and make their output identical, perhaps
> something like the attached (not even compile tested).

I think that is the right way forward.  I am worried that we will end up
with parts of the code that do not handle the distinction properly (see
below). But maybe it is best to try it and shake the bugs out.

> The implicit comparison to zero in the above is a bad code (but that is
> a problem from the very old days).

It is. The enum at least explicitly starts at 0 for this reason, but I
don't mind at all if it is updated to an explicit '!= DATE_NORMAL'.

> diff --git a/cache.h b/cache.h
> index 58ff054..fe42e80 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -876,7 +876,8 @@ extern struct object *peel_to_type(const char *name, int namelen,
>  				   struct object *o, enum object_type);
>  
>  enum date_mode {
> -	DATE_NORMAL = 0,
> +	DATE_DEFAULT = 0,
> +	DATE_NORMAL,
>  	DATE_RELATIVE,
>  	DATE_SHORT,
>  	DATE_LOCAL,
> diff --git a/reflog-walk.c b/reflog-walk.c
> index b974258..d002516 100644
> --- a/reflog-walk.c
> +++ b/reflog-walk.c
> @@ -277,7 +277,7 @@ void get_reflog_selector(struct strbuf *sb,
>  
>  	strbuf_addf(sb, "%s@{", printed_ref);
>  	if (commit_reflog->selector == SELECTOR_DATE ||
> -	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
> +	    (commit_reflog->selector == SELECTOR_NONE && (dmode != DATE_DEFAULT))) {
>  		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
>  		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
>  	} else {

I think some of the callers set dmode to DATE_NORMAL explicitly. So this
code would be confused into thinking that the user had asked for it
explicitly. Or maybe it happens before the date_mode_explicit check, and
it would be OK. I'd have to do audit the code.

-Peff

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
       [not found]                                         ` <20120507213752.GA19911-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-10 15:37                                           ` Jeff King
       [not found]                                             ` <20120510153754.GA23941-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2012-05-10 15:37 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eli Barzilay, Yann Hodique, Andreas Schwab,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

On Mon, May 07, 2012 at 05:37:52PM -0400, Jeff King wrote:

> >  	strbuf_addf(sb, "%s@{", printed_ref);
> >  	if (commit_reflog->selector == SELECTOR_DATE ||
> > -	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
> > +	    (commit_reflog->selector == SELECTOR_NONE && (dmode != DATE_DEFAULT))) {
> >  		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
> >  		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
> >  	} else {
> 
> I think some of the callers set dmode to DATE_NORMAL explicitly. So this
> code would be confused into thinking that the user had asked for it
> explicitly. Or maybe it happens before the date_mode_explicit check, and
> it would be OK. I'd have to do audit the code.

I just took a look at what you built on top of this topic (55ccf85)
instead of the bit quoted above. I also found it ugly not to pass the
explicit flag all the way down to the point-of-use. I had a nagging
feeling that the original did not do it that way for some good reason,
but looking at your patch, I cannot fathom what that reason could
possibly be. So it looks good to me.

-Peff

PS It would have been nice to see the patch on the list for review. I
   only noticed it because it hit 'next', and had a minor conflict with
   my patches in the area.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
       [not found]                                             ` <20120510153754.GA23941-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-10 16:39                                               ` Junio C Hamano
       [not found]                                                 ` <7vd36cng6n.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Junio C Hamano @ 2012-05-10 16:39 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, Eli Barzilay, Yann Hodique, Andreas Schwab,
	git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

Jeff King <peff-AdEPDUrAXsQ@public.gmane.org> writes:

> PS It would have been nice to see the patch on the list for review. I
>    only noticed it because it hit 'next', and had a minor conflict with
>    my patches in the area.

Heh, it was sent before I gave you this message:

        From: Junio C Hamano <gitster-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
        Subject: Re: [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors
        To: Jeff King <peff-AdEPDUrAXsQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
        Date: Mon, 07 May 2012 14:54:24 -0700

        Jeff King <peff-AdEPDUrAXsQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org> writes:

        > I think some of the callers set dmode to DATE_NORMAL explicitly. So this
        > code would be confused into thinking that the user had asked for it
        > explicitly. Or maybe it happens before the date_mode_explicit check, and
        > it would be OK. I'd have to do audit the code.

        Yeah, that is why today's update I sent does not use DATE_DEFAULT, which
        is after all a hack to piggy-back logically a separate bit in the same
        variable.  What we are trying to tell these two functions are (1) does the
        caller prefer to use counted notation or dated notation?  and (2) if the
        output shows the timestamp, what format should be used.

The message "today's update I sent" refers to is this.

I suspect that the original thread came from a message cross-posted to
magit list whose participants somehow seem to mangle e-mail addresses when
interacting with gmane, and it ended up mangling our e-mail addresses but
the e-mail relay at gmane probably dropped the messages.

-- >8 --

Junio C Hamano <gitster-e+AXbWqSrlAAvxtiuMwx3w-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org> writes:

    Administrivia: these gmane-mangled e-mail addresses are extremely
    annoying.  Please do not cross post with any insane list that choose
    to turn that feature on when sending message to the git list; thanks.

> Jeff King <peff-AdEPDUrAXsQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org> writes:
>
>> This patch flips the rules to:
>>
>>   1. if the user asked for ref@{0}, always show the index
>>
>>   2. if the user asked for ref@{now}, always show the date
>>
>>   3. otherwise, we have just "ref"; show them counted by
>>      default, but respect the presence of "--date" as a clue
>>      that the user wanted them date-based
>
> The revision.c parser for "git log --date=default -g master" would flip
> the "explicit" bit, revs->date_mode is set to DATE_NORMAL, and that value
> will eventually come as dmode here.
> ...
> But DATE_NORMAL happens to be zero ;-) "git log --date=default -g master"
> would still show the counted version.
>
> I personally do not care about that behaviour, but I know that I will
> later later have to deal with people who do care, which is annoying.

How about doing it this way?  After all, we are internally holding a bit
but the problem is that bit is lost near the tip of the callchain.

The two integer arguments to get_reflog_selector() and the two integer
arguments to show_reflog_message() might want to become two bits in one
"unsigned flags", but the former wants "do we want date?  do we want a
short output?" two bits, while the latter wants "do we want date?  do we
want a oneline output?", so I didn't bother squashing them into one flag
word with three-bit assigned.  Perhaps we should, but I dunno.

-- >8 --
Subject: [PATCH] reflog-walk: tell --date=default from not having --date at all

Introduction of opt->date_mode_explicit was a step in the right direction,
but lost that crucial bit at the very end of the callchain, and the callee
could not tell an explicitly specified "I want *date* but in default format"
from the built-in default value passed when there was no --date specified.

Signed-off-by: Junio C Hamano <gitster-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>
---
 log-tree.c             | 7 +++----
 pretty.c               | 5 ++---
 reflog-walk.c          | 8 ++++----
 reflog-walk.h          | 4 ++--
 t/t1411-reflog-show.sh | 8 ++++----
 5 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 5f9e59a..588117e 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -493,10 +493,9 @@ void show_log(struct rev_info *opt)
 			 * graph info here.
 			 */
 			show_reflog_message(opt->reflog_info,
-				    opt->commit_format == CMIT_FMT_ONELINE,
-				    opt->date_mode_explicit ?
-					opt->date_mode :
-					DATE_NORMAL);
+					    opt->commit_format == CMIT_FMT_ONELINE,
+					    opt->date_mode,
+					    opt->date_mode_explicit);
 			if (opt->commit_format == CMIT_FMT_ONELINE)
 				return;
 		}
diff --git a/pretty.c b/pretty.c
index efd62e8..25944de 100644
--- a/pretty.c
+++ b/pretty.c
@@ -956,9 +956,8 @@ static size_t format_commit_one(struct strbuf *sb, const char *placeholder,
 			if (c->pretty_ctx->reflog_info)
 				get_reflog_selector(sb,
 						    c->pretty_ctx->reflog_info,
-						    c->pretty_ctx->date_mode_explicit ?
-						      c->pretty_ctx->date_mode :
-						      DATE_NORMAL,
+						    c->pretty_ctx->date_mode,
+						    c->pretty_ctx->date_mode_explicit,
 						    (placeholder[1] == 'd'));
 			return 2;
 		case 's':	/* reflog message */
diff --git a/reflog-walk.c b/reflog-walk.c
index b84e80f..0c904fb 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -252,7 +252,7 @@ void fake_reflog_parent(struct reflog_walk_info *info, struct commit *commit)
 
 void get_reflog_selector(struct strbuf *sb,
 			 struct reflog_walk_info *reflog_info,
-			 enum date_mode dmode,
+			 enum date_mode dmode, int force_date,
 			 int shorten)
 {
 	struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
@@ -273,7 +273,7 @@ void get_reflog_selector(struct strbuf *sb,
 
 	strbuf_addf(sb, "%s@{", printed_ref);
 	if (commit_reflog->selector == SELECTOR_DATE ||
-	    (commit_reflog->selector == SELECTOR_NONE && dmode)) {
+	    (commit_reflog->selector == SELECTOR_NONE && force_date)) {
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
 		strbuf_addstr(sb, show_date(info->timestamp, info->tz, dmode));
 	} else {
@@ -302,7 +302,7 @@ void get_reflog_message(struct strbuf *sb,
 }
 
 void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
-	enum date_mode dmode)
+			 enum date_mode dmode, int force_date)
 {
 	if (reflog_info && reflog_info->last_commit_reflog) {
 		struct commit_reflog *commit_reflog = reflog_info->last_commit_reflog;
@@ -310,7 +310,7 @@ void show_reflog_message(struct reflog_walk_info *reflog_info, int oneline,
 		struct strbuf selector = STRBUF_INIT;
 
 		info = &commit_reflog->reflogs->items[commit_reflog->recno+1];
-		get_reflog_selector(&selector, reflog_info, dmode, 0);
+		get_reflog_selector(&selector, reflog_info, dmode, force_date, 0);
 		if (oneline) {
 			printf("%s: %s", selector.buf, info->message);
 		}
diff --git a/reflog-walk.h b/reflog-walk.h
index 7bd2cd4..3adccb0 100644
--- a/reflog-walk.h
+++ b/reflog-walk.h
@@ -11,12 +11,12 @@ extern int add_reflog_for_walk(struct reflog_walk_info *info,
 extern void fake_reflog_parent(struct reflog_walk_info *info,
 		struct commit *commit);
 extern void show_reflog_message(struct reflog_walk_info *info, int,
-		enum date_mode);
+				enum date_mode, int force_date);
 extern void get_reflog_message(struct strbuf *sb,
 		struct reflog_walk_info *reflog_info);
 extern void get_reflog_selector(struct strbuf *sb,
 		struct reflog_walk_info *reflog_info,
-		enum date_mode dmode,
+		enum date_mode dmode, int force_date,
 		int shorten);
 
 #endif
diff --git a/t/t1411-reflog-show.sh b/t/t1411-reflog-show.sh
index 7d9b5e3..9a105fe 100755
--- a/t/t1411-reflog-show.sh
+++ b/t/t1411-reflog-show.sh
@@ -73,20 +73,20 @@ test_expect_success 'using @{now} syntax shows reflog date (format=%gd)' '
 '
 
 cat >expect <<'EOF'
-Reflog: HEAD@{1112911993 -0700} (C O Mitter <committer-hcDgGtZH8xNBDgjK7y7TUQ@public.gmane.org>)
+Reflog: HEAD@{Thu Apr 7 15:13:13 2005 -0700} (C O Mitter <committer-hcDgGtZH8xNBDgjK7y7TUQ@public.gmane.org>)
 Reflog message: commit (initial): one
 EOF
 test_expect_success 'using --date= shows reflog date (multiline)' '
-	git log -g -1 --date=raw >tmp &&
+	git log -g -1 --date=default >tmp &&
 	grep ^Reflog <tmp >actual &&
 	test_cmp expect actual
 '
 
 cat >expect <<'EOF'
-e46513e HEAD@{1112911993 -0700}: commit (initial): one
+e46513e HEAD@{Thu Apr 7 15:13:13 2005 -0700}: commit (initial): one
 EOF
 test_expect_success 'using --date= shows reflog date (oneline)' '
-	git log -g -1 --oneline --date=raw >actual &&
+	git log -g -1 --oneline --date=default >actual &&
 	test_cmp expect actual
 '
 
-- 
1.7.10.1.500.g37b1e9a

^ permalink raw reply related	[flat|nested] 26+ messages in thread

* OT: gmane address mangling selectors
       [not found]                                                 ` <7vd36cng6n.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
@ 2012-05-10 17:19                                                   ` Jeff King
       [not found]                                                     ` <20120510171912.GA29972-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
  0 siblings, 1 reply; 26+ messages in thread
From: Jeff King @ 2012-05-10 17:19 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eli Barzilay, git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

On Thu, May 10, 2012 at 09:39:44AM -0700, Junio C Hamano wrote:

> Jeff King <peff-AdEPDUrAXsQ@public.gmane.org> writes:
> 
> > PS It would have been nice to see the patch on the list for review. I
> >    only noticed it because it hit 'next', and had a minor conflict with
> >    my patches in the area.
> 
> Heh, it was sent before I gave you this message:

Ah, I never got that message. Probably because...

>         To: Jeff King <peff-AdEPDUrAXsQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>

...I have no clue where that address would end up.

> Junio C Hamano <gitster-e+AXbWqSrlAAvxtiuMwx3w-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org> writes:
> 
>     Administrivia: these gmane-mangled e-mail addresses are extremely
>     annoying.  Please do not cross post with any insane list that choose
>     to turn that feature on when sending message to the git list; thanks.

Where do they come from? I notice that my address has been mangled
above, but I do not use gmane to either post or read the list. In fact,
looking at the start of the thread in my mailbox, I see:

  1. Eli posts[1], to: git@vger and magit@googlegroups; his from address
     looks normal, and there is no reply-to.

  2. You reply, to: a gmane-mangled address, cc: git@vger.

The presence of the magit list is obviously the unusual thing here, but
he did not involve gmane at all. If I recall correctly, you read the
list via gmane. So I believe it is your workflow that introduces the
mangled addresses on the reading end, not the sender.

It seems that this gmane "feature" is turned on by the presence of the
magit list, which I guess is configured at gmane to obfuscate emails.
But I don't see how the sender should be expected to know or care about
this gmane nonsense. It is your fault that the gateway through which you
read the messages is doing the mangling. So the right fix is not to
arbitrarily restrict cc-ing of other lists, but to fix the gmane bug[2].

-Peff

[1] mid:<20379.9312.943088.350379-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>; the gmane link
    at http://thread.gmane.org/gmane.comp.version-control.git.magit/1308
    is mangled, but the original message as sent by vger looks fine (a
    copy of the headers that I received is included below).

[2] Without looking at the gmane code at all, I suspect the faulty logic
    is "mangle addresses for privacy if the message went to any list
    which requests this feature". But that is not right. The privacy was
    lost as soon as it went to any list that does _not_ mangle. And the
    unmangled version should be given to people accessing the sane list
    (it is slightly pointless to still mangle for the other list, since
    the privacy has been lost, but at least it is not actively bad). But
    given that my search for the mid at gmane turns up the magit
    version, I am guessing that they are storing only a single version
    of the message (mangled).

-- >8 --
Return-Path: <git-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Delivered-To: peff-AdEPDUrAXsQ@public.gmane.org
Received: (qmail 2012 invoked by uid 107); 28 Apr 2012 00:03:12 -0000
X-Spam-Level: *
X-Spam-Status: No, hits=-3.4 required=4.9
	tests=BAYES_00,KB_DATE_CONTAINS_TAB,RCVD_IN_DNSWL_HI,RP_MATCHES_RCVD,TAB_IN_FROM
X-Spam-Check-By: peff.net
Received: from vger.kernel.org (HELO vger.kernel.org) (209.132.180.67)
    by peff.net (qpsmtpd/0.84) with ESMTP; Fri, 27 Apr 2012 20:03:07 -0400
Received: (majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org) by vger.kernel.org via listexpand
	id S1758065Ab2D1ACs (ORCPT <rfc822;peff-AdEPDUrAXsQ@public.gmane.org>);
	Fri, 27 Apr 2012 20:02:48 -0400
Received: from winooski.ccs.neu.edu ([129.10.115.117]:52945 "EHLO
	winooski.ccs.neu.edu" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1757659Ab2D1ACg (ORCPT <rfc822;git-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>);
	Fri, 27 Apr 2012 20:02:36 -0400
X-Greylist: delayed 3897 seconds by postgrey-1.27 at vger.kernel.org; Fri, 27 Apr 2012 20:02:36 EDT
Received: from winooski.ccs.neu.edu (localhost.localdomain [127.0.0.1])
	by winooski.ccs.neu.edu (8.14.4/8.14.4) with ESMTP id q3RMvbEt019273;
	Fri, 27 Apr 2012 18:57:37 -0400
Received: (from eli@localhost)
	by winooski.ccs.neu.edu (8.14.4/8.14.4/Submit) id q3RMvbHJ019269;
	Fri, 27 Apr 2012 18:57:37 -0400
From:	Eli Barzilay <eli-oSK4jVRJLyZg9hUCZPvPmw@public.gmane.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Message-ID: <20379.9312.943088.350379-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
Date:	Fri, 27 Apr 2012 18:57:36 -0400
To:	git-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, magit-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org
Subject: Bug in git-stash(.sh) ?
X-Mailer: VM 8.2.0a under 23.2.1 (x86_64-redhat-linux-gnu)
Sender:	git-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Precedence: bulk
List-ID: <git.vger.kernel.org>
X-Mailing-List:	git-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Status: RO
Content-Length: 3206
Lines: 100

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: OT: gmane address mangling selectors
       [not found]                                                     ` <20120510171912.GA29972-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
@ 2012-05-10 17:35                                                       ` Eli Barzilay
  0 siblings, 0 replies; 26+ messages in thread
From: Eli Barzilay @ 2012-05-10 17:35 UTC (permalink / raw)
  To: Jeff King
  Cc: Junio C Hamano, git-u79uwXL29TY76Z2rM5mHXA, magit-/JYPxA39Uh5TLH3MbocFFw

A few minutes ago, Jeff King wrote:
> 
> The presence of the magit list is obviously the unusual thing here,
> but he did not involve gmane at all. If I recall correctly, you read
> the list via gmane. So I believe it is your workflow that introduces
> the mangled addresses on the reading end, not the sender.

IIRC, gmane intercepts a first-time post to a newsgroup->mailing list
using a mangled email that it gets and later forwards on your behalf
when you prove that you're human.  Or something like that.  The weird
thing is that it did that for a personal email -- maybe someone posted
a reply with CCs, where the usual thing is to have only newsgroups?

-- 
          ((lambda (x) (x x)) (lambda (x) (x x)))          Eli Barzilay:
                    http://barzilay.org/                   Maze is Life!

^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2012-05-10 17:35 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-27 22:57 Bug in git-stash(.sh) ? Eli Barzilay
2012-04-27 23:02 ` Junio C Hamano
2012-04-28  0:16   ` Eli Barzilay
     [not found]     ` <CALO-gut4csy5wef4iGPGD5jVPc1f0iFBfS3MUWrOwc2yczdviw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
     [not found]       ` <m2pqasb8mr.fsf-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
     [not found]         ` <87wr4za9mr.fsf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2012-04-28 23:59           ` Eli Barzilay
2012-04-29 22:01             ` Jeff King
     [not found]               ` <20120429220132.GB4491-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-04-29 22:26                 ` Eli Barzilay
2012-05-01 13:42                   ` Jeff King
     [not found]                     ` <20120501134254.GA11900-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-05-03 18:44                       ` [git] " Eli Barzilay
     [not found]                         ` <20386.53745.200846.115335-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
2012-05-04  5:21                           ` Jeff King
     [not found]                             ` <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-05-04  5:23                               ` [PATCH 1/4] t1411: add more selector index/date tests Jeff King
2012-05-04  5:25                               ` [PATCH 2/4] log: respect date_mode_explicit with --format:%gd Jeff King
2012-05-04  5:27                               ` [PATCH 4/4] reflog-walk: always make HEAD@{0} show indexed selectors Jeff King
     [not found]                                 ` <20120504052725.GD16107-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-05-04 17:02                                   ` Junio C Hamano
     [not found]                                     ` <7v7gwrc212.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
2012-05-07 21:37                                       ` Jeff King
     [not found]                                         ` <20120507213752.GA19911-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-05-10 15:37                                           ` Jeff King
     [not found]                                             ` <20120510153754.GA23941-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-05-10 16:39                                               ` Junio C Hamano
     [not found]                                                 ` <7vd36cng6n.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
2012-05-10 17:19                                                   ` OT: gmane address mangling selectors Jeff King
     [not found]                                                     ` <20120510171912.GA29972-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>
2012-05-10 17:35                                                       ` Eli Barzilay
2012-05-04 18:57                               ` [git] Re: Bug in git-stash(.sh) ? Eli Barzilay
     [not found]                                 ` <20388.9885.608325.489624-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>
2012-05-04 22:36                                   ` Eli Barzilay
2012-05-04  5:26                             ` [PATCH 3/4] reflog-walk: clean up "flag" field of commit_reflog struct Jeff King
2012-04-29 22:07             ` Bug in git-stash(.sh) ? Junio C Hamano
     [not found]               ` <7vlilexkcq.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>
2012-04-29 22:37                 ` Eli Barzilay
2012-05-01 15:02                 ` Jeff King
2012-04-28  7:47 ` Andreas Schwab
2012-04-28 20:23 ` Yann Hodique

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).