Re: [PATCH v2 5/6] stash: default listing to --cc --simplify-combined-diff
On Wed, 2014-07-30 at 20:09 -0400, Jeff King wrote: On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote: git log --cc is one of the things I wanted for a long time to fix. When the user explicitly asks --cc, we currently ignore it, but because we know the user wants to view combined diff, we should turn -p on automatically. And the change this patch introduces will be broken when we fix log --cc (stash list will end up always showing the patch, without a way to disable it). Can you make this conditional? Do this only when options are given to git stash list command and that includes -p or something? I'm definitely sympathetic. Using --cc with the diff family _does_ imply -p already, so it would be nice to do the same for the log family. Perhaps something along this line? git-stash.sh | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index ae73ba4..0db1b19 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -297,8 +297,15 @@ have_stash () { list_stash () { have_stash || return 0 - git log --format=%gd: %gs -g --cc --simplify-combined-diff \ - $@ $ref_stash -- + case $* in + *' --cc '*) + ;; # the user knows what she is doing + *' -p '* | *' -U'*) + set x --cc --simplify-combined-diff $@ + shift + ;; + esac + git log --format=%gd: %gs -g $@ $ref_stash -- Ugh. I'd generally like to avoid this sort of ad-hoc command line parsing, as it is easy to get confused by option arguments or even non-options. At least we do not have to worry about pathspecs here, as we already do an explicit -- (so we might be confused by them, but they are broken anyway). Still, I wonder if a cleaner solution is to provide alternate default to this options for the revision.c option parser. We already have --default to pick the default starting point. Could we do something like --default-merge-handling=cc or something? There's a similar ad-hoc parsing case in stash show, where we add --stat if no arguments are given, but we have no clue if a diff format was given (so git stash show --color accidentally turns on patch format!). Something like --default-diff-format=stat would be more robust. I dunno. Maybe it is over-engineering. I just hate to see solutions that work most of the time and crumble in weird corner cases, even if people don't hit those corner cases very often. -Peff I agree with you on this one. Those corner cases tend to bite the worst. Thanks, Jake -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] stash: default listing to --cc --simplify-combined-diff
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: When you list stashes, you can provide arbitrary git-log options to change the display. However, adding just -p does nothing, because each stash is actually a merge commit. This implementation detail is easy to forget, leading to confused users who think -p is not working. We can make this easier by specifying --cc as a default ourselves (which does nothing if no diff format is requested by the user). Sigh. git log --cc is one of the things I wanted for a long time to fix. When the user explicitly asks --cc, we currently ignore it, but because we know the user wants to view combined diff, we should turn -p on automatically. And the change this patch introduces will be broken when we fix log --cc (stash list will end up always showing the patch, without a way to disable it). Can you make this conditional? Do this only when options are given to git stash list command and that includes -p or something? Perhaps something along this line? git-stash.sh | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index ae73ba4..0db1b19 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -297,8 +297,15 @@ have_stash () { list_stash () { have_stash || return 0 - git log --format=%gd: %gs -g --cc --simplify-combined-diff \ - $@ $ref_stash -- + case $* in + *' --cc '*) + ;; # the user knows what she is doing + *' -p '* | *' -U'*) + set x --cc --simplify-combined-diff $@ + shift + ;; + esac + git log --format=%gd: %gs -g $@ $ref_stash -- } show_stash () { -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] stash: default listing to --cc --simplify-combined-diff
On Wed, Jul 30, 2014 at 12:43:09PM -0700, Junio C Hamano wrote: git log --cc is one of the things I wanted for a long time to fix. When the user explicitly asks --cc, we currently ignore it, but because we know the user wants to view combined diff, we should turn -p on automatically. And the change this patch introduces will be broken when we fix log --cc (stash list will end up always showing the patch, without a way to disable it). Can you make this conditional? Do this only when options are given to git stash list command and that includes -p or something? I'm definitely sympathetic. Using --cc with the diff family _does_ imply -p already, so it would be nice to do the same for the log family. Perhaps something along this line? git-stash.sh | 11 +-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/git-stash.sh b/git-stash.sh index ae73ba4..0db1b19 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -297,8 +297,15 @@ have_stash () { list_stash () { have_stash || return 0 - git log --format=%gd: %gs -g --cc --simplify-combined-diff \ - $@ $ref_stash -- + case $* in + *' --cc '*) + ;; # the user knows what she is doing + *' -p '* | *' -U'*) + set x --cc --simplify-combined-diff $@ + shift + ;; + esac + git log --format=%gd: %gs -g $@ $ref_stash -- Ugh. I'd generally like to avoid this sort of ad-hoc command line parsing, as it is easy to get confused by option arguments or even non-options. At least we do not have to worry about pathspecs here, as we already do an explicit -- (so we might be confused by them, but they are broken anyway). Still, I wonder if a cleaner solution is to provide alternate default to this options for the revision.c option parser. We already have --default to pick the default starting point. Could we do something like --default-merge-handling=cc or something? There's a similar ad-hoc parsing case in stash show, where we add --stat if no arguments are given, but we have no clue if a diff format was given (so git stash show --color accidentally turns on patch format!). Something like --default-diff-format=stat would be more robust. I dunno. Maybe it is over-engineering. I just hate to see solutions that work most of the time and crumble in weird corner cases, even if people don't hit those corner cases very often. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 5/6] stash: default listing to --cc --simplify-combined-diff
When you list stashes, you can provide arbitrary git-log options to change the display. However, adding just -p does nothing, because each stash is actually a merge commit. This implementation detail is easy to forget, leading to confused users who think -p is not working. We can make this easier by specifying --cc as a default ourselves (which does nothing if no diff format is requested by the user). The resulting diff would then be a combined diff between the base commit and the stashed index state. In many cases, though, the user did not stash anything in the index. We can further simplify this to a normal pairwise diff by using --simplify-combined-diff. Signed-off-by: Jeff King p...@peff.net --- git-stash.sh | 3 ++- t/t3903-stash.sh | 63 2 files changed, 65 insertions(+), 1 deletion(-) diff --git a/git-stash.sh b/git-stash.sh index bcc757b..a0246d5 100755 --- a/git-stash.sh +++ b/git-stash.sh @@ -297,7 +297,8 @@ have_stash () { list_stash () { have_stash || return 0 - git log --format=%gd: %gs -g $@ $ref_stash -- + git log --format=%gd: %gs -g --cc --simplify-combined-diff \ + $@ $ref_stash -- } show_stash () { diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh index 5b79b21..465f824 100755 --- a/t/t3903-stash.sh +++ b/t/t3903-stash.sh @@ -685,4 +685,67 @@ test_expect_success 'handle stash specification with spaces' ' grep pig file ' +test_expect_success 'stash list implies --cc' ' + git stash clear + git reset --hard + echo index file + git add file + echo working file + git stash + git stash list -p actual + cat expect -\EOF + stash@{0}: WIP on master: b27a2bc subdir + + diff --cc file + index 257cc56,9015a7a..d26b33d + --- a/file + +++ b/file + @@@ -1,1 -1,1 +1,1 @@@ + - foo +-index + ++working + EOF + test_cmp expect actual +' + +test_expect_success 'stash list implies --simplify-combined-diff' ' + git stash clear + git reset --hard + echo working file + git stash + git stash list -p actual + cat expect -\EOF + stash@{0}: WIP on master: b27a2bc subdir + + diff --git a/file b/file + index 257cc56..d26b33d 100644 + --- a/file + +++ b/file + @@ -1 +1 @@ + -foo + +working + EOF + test_cmp expect actual +' + +test_expect_success '--no-simplify-combined-diff overrides default' ' + git stash clear + git reset --hard + echo working file + git stash + git stash list -p --no-simplify-combined-diff actual + cat expect -\EOF + stash@{0}: WIP on master: b27a2bc subdir + + diff --cc file + index 257cc56,257cc56..d26b33d + --- a/file + +++ b/file + @@@ -1,1 -1,1 +1,1 @@@ + --foo + ++working + EOF + test_cmp expect actual +' + test_done -- 2.1.0.rc0.286.g5c67d74 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] stash: default listing to --cc --simplify-combined-diff
Jeff King p...@peff.net writes: When you list stashes, you can provide arbitrary git-log options to change the display. However, adding just -p does nothing, because each stash is actually a merge commit. This implementation detail is easy to forget, leading to confused users who think -p is not working. We can make this easier by specifying --cc as a default ourselves (which does nothing if no diff format is requested by the user). Sigh. git log --cc is one of the things I wanted for a long time to fix. When the user explicitly asks --cc, we currently ignore it, but because we know the user wants to view combined diff, we should turn -p on automatically. And the change this patch introduces will be broken when we fix log --cc (stash list will end up always showing the patch, without a way to disable it). Can you make this conditional? Do this only when options are given to git stash list command and that includes -p or something? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html