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