* 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
[parent not found: <CALO-gut4csy5wef4iGPGD5jVPc1f0iFBfS3MUWrOwc2yczdviw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>]
[parent not found: <m2pqasb8mr.fsf-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>]
[parent not found: <87wr4za9mr.fsf-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]
* 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
[parent not found: <20120429220132.GB4491-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* 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) ? 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
[parent not found: <20120501134254.GA11900-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* [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
[parent not found: <20386.53745.200846.115335-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>]
* 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
[parent not found: <20120504052106.GA15970-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* [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 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
[parent not found: <20120504052725.GD16107-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* 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
[parent not found: <7v7gwrc212.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>]
* 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
[parent not found: <20120507213752.GA19911-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* 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
[parent not found: <20120510153754.GA23941-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* 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
[parent not found: <7vd36cng6n.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>]
* 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
[parent not found: <20120510171912.GA29972-bBVMEuqLR+SYVEpFpFwlB0AkDMvbqDRI@public.gmane.org>]
* 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
* 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
[parent not found: <20388.9885.608325.489624-a5nvgYPMCZcx/1z6v04GWfZ8FUJU4vz8@public.gmane.org>]
* 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
* [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
* 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
[parent not found: <7vlilexkcq.fsf-s2KvWo2KEQL18tm6hw+yZpy9Z0UEorGK@public.gmane.org>]
* 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) ? [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
* 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
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).