git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] range-diff: update stale summary of --no-dual-color
@ 2018-08-23  2:39 Kyle Meyer
  2018-08-23  2:47 ` Jonathan Nieder
  2018-08-23 20:54 ` Johannes Schindelin
  0 siblings, 2 replies; 20+ messages in thread
From: Kyle Meyer @ 2018-08-23  2:39 UTC (permalink / raw)
  To: git; +Cc: Johannes.Schindelin, Kyle Meyer

275267937b (range-diff: make dual-color the default mode, 2018-08-13)
replaced --dual-color with --no-dual-color but left the option's
summary untouched.  Rewrite the summary to describe --no-dual-color
rather than dual-color.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f52d45d9d6..7dc90a5ec3 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
-			    N_("color both diff and diff-between-diffs")),
+			    N_("restrict coloring to outer diff markers")),
 		OPT_END()
 	};
 	int i, j, res = 0;
-- 
2.18.0


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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  2:39 [PATCH] range-diff: update stale summary of --no-dual-color Kyle Meyer
@ 2018-08-23  2:47 ` Jonathan Nieder
  2018-08-23  3:03   ` Kyle Meyer
  2018-08-23 20:54 ` Johannes Schindelin
  1 sibling, 1 reply; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23  2:47 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: git, Johannes.Schindelin

Hi,

Kyle Meyer wrote:

> 275267937b (range-diff: make dual-color the default mode, 2018-08-13)
> replaced --dual-color with --no-dual-color but left the option's
> summary untouched.  Rewrite the summary to describe --no-dual-color
> rather than dual-color.
>
> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
> ---
>  builtin/range-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, good catch!

> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index f52d45d9d6..7dc90a5ec3 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
>  			    N_("Percentage by which creation is weighted")),
>  		OPT_BOOL(0, "no-dual-color", &simple_color,
> -			    N_("color both diff and diff-between-diffs")),
> +			    N_("restrict coloring to outer diff markers")),

What is an outer diff marker?

Thanks,
Jonathan

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  2:47 ` Jonathan Nieder
@ 2018-08-23  3:03   ` Kyle Meyer
  2018-08-23  3:22     ` Jonathan Nieder
  0 siblings, 1 reply; 20+ messages in thread
From: Kyle Meyer @ 2018-08-23  3:03 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Johannes.Schindelin

Jonathan Nieder <jrnieder@gmail.com> writes:

[...]

> What is an outer diff marker?

The diff markers from the diff of patches as opposed to the ones from
the original patches.  I took the term from git-range-diff.txt:

    --no-dual-color::
        When the commit diffs differ, `git range-diff` recreates the
        original diffs' coloring, and adds outer -/+ diff markers [...]

    Use `--no-dual-color` to revert to color all lines according to the
    outer diff markers (and completely ignore the inner diff when it
    comes to color).

-- 
Kyle

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  3:03   ` Kyle Meyer
@ 2018-08-23  3:22     ` Jonathan Nieder
  2018-08-23  4:04       ` Kyle Meyer
  2018-08-23 14:31       ` [PATCH] " Johannes Schindelin
  0 siblings, 2 replies; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23  3:22 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: git, Johannes.Schindelin

Kyle Meyer wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Kyle Meyer wrote:

>>> -			    N_("color both diff and diff-between-diffs")),
>>> +			    N_("restrict coloring to outer diff markers")),
[...]
>> What is an outer diff marker?
>
> The diff markers from the diff of patches as opposed to the ones from
> the original patches.  I took the term from git-range-diff.txt:
>
>     --no-dual-color::
>         When the commit diffs differ, `git range-diff` recreates the
>         original diffs' coloring, and adds outer -/+ diff markers [...]
>
>     Use `--no-dual-color` to revert to color all lines according to the
>     outer diff markers (and completely ignore the inner diff when it
>     comes to color).

Aha: I think you're missing a few words (e.g. "color only according to
outer diff markers").  Though based on the output, I'm not sure the
focus on diff markers captures the difference.  (After all, some lines
are multiple colors in --no-dual-color mode and have no diff markers.)

"Restrict coloring to outer -/+ diff markers" would mean that
everything will be in plain text, except for the minus or plus sign at
the beginning of each line.  So you'd see a colorful strip on the left
and everything else monochrome.

I think what you mean is something like "color only based on the
diff-between-diffs".  Or it might be simpler to do something like
the following.  What do you think?

diff --git i/builtin/range-diff.c w/builtin/range-diff.c
index f52d45d9d6..88c19f48d3 100644
--- i/builtin/range-diff.c
+++ w/builtin/range-diff.c
@@ -20,12 +20,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = 60;
 	struct diff_options diffopt = { NULL };
-	int simple_color = -1;
+	int dual_color = -1;
 	struct option options[] = {
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
-		OPT_BOOL(0, "no-dual-color", &simple_color,
-			    N_("color both diff and diff-between-diffs")),
+		OPT_BOOL(0, "dual-color", &dual_color,
+			    N_("color both diff and diff-between-diffs (default)")),
 		OPT_END()
 	};
 	int i, j, res = 0;
@@ -63,8 +63,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 			     options + ARRAY_SIZE(options) - 1, /* OPT_END */
 			     builtin_range_diff_usage, 0);
 
-	if (simple_color < 1) {
-		if (!simple_color)
+	if (dual_color != 0) {
+		if (dual_color > 0)
 			/* force color when --dual-color was used */
 			diffopt.use_color = 1;
 		diffopt.flags.dual_color_diffed_diffs = 1;

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  3:22     ` Jonathan Nieder
@ 2018-08-23  4:04       ` Kyle Meyer
  2018-08-23  8:27         ` Jonathan Nieder
  2018-08-23 14:31       ` [PATCH] " Johannes Schindelin
  1 sibling, 1 reply; 20+ messages in thread
From: Kyle Meyer @ 2018-08-23  4:04 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Johannes.Schindelin

Jonathan Nieder <jrnieder@gmail.com> writes:

>>>> -			    N_("color both diff and diff-between-diffs")),
>>>> +			    N_("restrict coloring to outer diff markers")),

[...]

> Aha: I think you're missing a few words (e.g. "color only according to
> outer diff markers").  Though based on the output, I'm not sure the
> focus on diff markers captures the difference.  (After all, some lines
> are multiple colors in --no-dual-color mode and have no diff markers.)
>
> "Restrict coloring to outer -/+ diff markers" would mean that
> everything will be in plain text, except for the minus or plus sign at
> the beginning of each line.  So you'd see a colorful strip on the left
> and everything else monochrome.

Eh, you're right, it would read like that.  Thanks.

> I think what you mean is something like "color only based on the
> diff-between-diffs".

Yeah, I that sounds OK to me.  I played around with a few different
summary lines and couldn't come up with anything that I thought was
particularly good, and then of course I ended up settling on a summary
line that didn't preserve my intended meaning :/

> Or it might be simpler to do something like
> the following.  What do you think?
>
> diff --git i/builtin/range-diff.c w/builtin/range-diff.c
> index f52d45d9d6..88c19f48d3 100644
> --- i/builtin/range-diff.c
> +++ w/builtin/range-diff.c
> @@ -20,12 +20,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
>  {
>  	int creation_factor = 60;
>  	struct diff_options diffopt = { NULL };
> -	int simple_color = -1;
> +	int dual_color = -1;
>  	struct option options[] = {
>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
>  			    N_("Percentage by which creation is weighted")),
> -		OPT_BOOL(0, "no-dual-color", &simple_color,
> -			    N_("color both diff and diff-between-diffs")),
> +		OPT_BOOL(0, "dual-color", &dual_color,
> +			    N_("color both diff and diff-between-diffs (default)")),

I don't have a strong preference, though I lean towards making 'git
range-diff -h' show --no-dual-color since it's not the default.

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  4:04       ` Kyle Meyer
@ 2018-08-23  8:27         ` Jonathan Nieder
  2018-08-23 12:00           ` [PATCH v2] " Kyle Meyer
  0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23  8:27 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: git, Johannes.Schindelin

Kyle Meyer wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> I think what you mean is something like "color only based on the
>> diff-between-diffs".
>
> Yeah, I that sounds OK to me.

Thanks.  Want to prepare a patch along those lines to finish this off?

I'm off to sleep now, can look at this again tomorrow morning.

Thanks,
Jonathan

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

* [PATCH v2] range-diff: update stale summary of --no-dual-color
  2018-08-23  8:27         ` Jonathan Nieder
@ 2018-08-23 12:00           ` Kyle Meyer
  2018-08-23 21:10             ` Jonathan Nieder
  0 siblings, 1 reply; 20+ messages in thread
From: Kyle Meyer @ 2018-08-23 12:00 UTC (permalink / raw)
  To: git; +Cc: jrnieder, Johannes.Schindelin, Kyle Meyer

275267937b (range-diff: make dual-color the default mode, 2018-08-13)
replaced --dual-color with --no-dual-color but left the option's
summary untouched.  Rewrite the summary to describe --no-dual-color
rather than dual-color.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f52d45d9d6..057afdbf46 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
-			    N_("color both diff and diff-between-diffs")),
+			    N_("color only based on the diff-between-diffs")),
 		OPT_END()
 	};
 	int i, j, res = 0;
-- 
2.18.0


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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  3:22     ` Jonathan Nieder
  2018-08-23  4:04       ` Kyle Meyer
@ 2018-08-23 14:31       ` Johannes Schindelin
  2018-08-23 21:12         ` Jonathan Nieder
  1 sibling, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2018-08-23 14:31 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Kyle Meyer, git

Hi Jonathan,

On Wed, 22 Aug 2018, Jonathan Nieder wrote:

>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
>  			    N_("Percentage by which creation is weighted")),
> -		OPT_BOOL(0, "no-dual-color", &simple_color,
> -			    N_("color both diff and diff-between-diffs")),
> +		OPT_BOOL(0, "dual-color", &dual_color,
> +			    N_("color both diff and diff-between-diffs (default)")),

There is one very good reason *not* to do that. And that reason is the
output of `git range-diff -h`. If anybody read that the option
`--dual-color` exists, they are prone to believe that the default is *not*
dual color. In contrast, when reading `--no-dual-color`, it is clear that
dual color mode is the default.

Ciao,
Dscho

>  		OPT_END()
>  	};
>  	int i, j, res = 0;
> @@ -63,8 +63,8 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
>  			     options + ARRAY_SIZE(options) - 1, /* OPT_END */
>  			     builtin_range_diff_usage, 0);
>  
> -	if (simple_color < 1) {
> -		if (!simple_color)
> +	if (dual_color != 0) {
> +		if (dual_color > 0)
>  			/* force color when --dual-color was used */
>  			diffopt.use_color = 1;
>  		diffopt.flags.dual_color_diffed_diffs = 1;
> 

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23  2:39 [PATCH] range-diff: update stale summary of --no-dual-color Kyle Meyer
  2018-08-23  2:47 ` Jonathan Nieder
@ 2018-08-23 20:54 ` Johannes Schindelin
  2018-08-23 21:18   ` Junio C Hamano
  1 sibling, 1 reply; 20+ messages in thread
From: Johannes Schindelin @ 2018-08-23 20:54 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: git

Hi Kyle,

On Wed, 22 Aug 2018, Kyle Meyer wrote:

> 275267937b (range-diff: make dual-color the default mode, 2018-08-13)
> replaced --dual-color with --no-dual-color but left the option's
> summary untouched.  Rewrite the summary to describe --no-dual-color
> rather than dual-color.
> 
> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
> ---
>  builtin/range-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> index f52d45d9d6..7dc90a5ec3 100644
> --- a/builtin/range-diff.c
> +++ b/builtin/range-diff.c
> @@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
>  			    N_("Percentage by which creation is weighted")),
>  		OPT_BOOL(0, "no-dual-color", &simple_color,
> -			    N_("color both diff and diff-between-diffs")),
> +			    N_("restrict coloring to outer diff markers")),

How about "use simple diff colors" instead?

Ciao,
Johannes

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

* Re: [PATCH v2] range-diff: update stale summary of --no-dual-color
  2018-08-23 12:00           ` [PATCH v2] " Kyle Meyer
@ 2018-08-23 21:10             ` Jonathan Nieder
  2018-08-23 21:57               ` [PATCH v3] " Kyle Meyer
  0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23 21:10 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: git, Johannes.Schindelin

Kyle Meyer wrote:

>  		OPT_BOOL(0, "no-dual-color", &simple_color,
> -			    N_("color both diff and diff-between-diffs")),
> +			    N_("color only based on the diff-between-diffs")),

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Dscho's suggestion "use simple diff colors" also sounds fine to me
(probably even better).

Thanks,
Jonathan

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23 14:31       ` [PATCH] " Johannes Schindelin
@ 2018-08-23 21:12         ` Jonathan Nieder
  0 siblings, 0 replies; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23 21:12 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Kyle Meyer, git

Hi,

Johannes Schindelin wrote:
> On Wed, 22 Aug 2018, Jonathan Nieder wrote:

>>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
>>  			    N_("Percentage by which creation is weighted")),
>> -		OPT_BOOL(0, "no-dual-color", &simple_color,
>> -			    N_("color both diff and diff-between-diffs")),
>> +		OPT_BOOL(0, "dual-color", &dual_color,
>> +			    N_("color both diff and diff-between-diffs (default)")),
>
> There is one very good reason *not* to do that. And that reason is the
> output of `git range-diff -h`. If anybody read that the option
> `--dual-color` exists, they are prone to believe that the default is *not*
> dual color. In contrast, when reading `--no-dual-color`, it is clear that
> dual color mode is the default.

The whole patch is about "git range-diff -h" output, and of course I
tested it.  Did you see the "(default)" part of the string in the
patch?

That said, the conversation continued and I agree with the conclusion
it led to (which is better than the patch you're replying to).

Thanks,
Jonathan

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23 20:54 ` Johannes Schindelin
@ 2018-08-23 21:18   ` Junio C Hamano
  2018-08-23 21:27     ` Kyle Meyer
  2018-08-23 21:41     ` Johannes Schindelin
  0 siblings, 2 replies; 20+ messages in thread
From: Junio C Hamano @ 2018-08-23 21:18 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Kyle Meyer, git

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 22 Aug 2018, Kyle Meyer wrote:
>
>> 275267937b (range-diff: make dual-color the default mode, 2018-08-13)
>> replaced --dual-color with --no-dual-color but left the option's
>> summary untouched.  Rewrite the summary to describe --no-dual-color
>> rather than dual-color.
>> 
>> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
>> ---
>>  builtin/range-diff.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
>> index f52d45d9d6..7dc90a5ec3 100644
>> --- a/builtin/range-diff.c
>> +++ b/builtin/range-diff.c
>> @@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
>>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
>>  			    N_("Percentage by which creation is weighted")),
>>  		OPT_BOOL(0, "no-dual-color", &simple_color,
>> -			    N_("color both diff and diff-between-diffs")),
>> +			    N_("restrict coloring to outer diff markers")),
>
> How about "use simple diff colors" instead?

I am wondering if it makes sense to remove the option altogether.
I've been trying to view the comparison of the same ranges in both
styles for the past few days, and I never found a reason to choose
"no dual color" option myself.

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23 21:18   ` Junio C Hamano
@ 2018-08-23 21:27     ` Kyle Meyer
  2018-08-23 21:48       ` Junio C Hamano
  2018-08-23 21:41     ` Johannes Schindelin
  1 sibling, 1 reply; 20+ messages in thread
From: Kyle Meyer @ 2018-08-23 21:27 UTC (permalink / raw)
  To: Junio C Hamano, Johannes Schindelin; +Cc: git, Jonathan Nieder

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

[...]

>>> -			    N_("color both diff and diff-between-diffs")),
>>> +			    N_("restrict coloring to outer diff markers")),
>>
>> How about "use simple diff colors" instead?

That's certainly better than the one above, and I also prefer it to
"color only based on the diff-between-diffs" in v2.

> I am wondering if it makes sense to remove the option altogether.
> I've been trying to view the comparison of the same ranges in both
> styles for the past few days, and I never found a reason to choose
> "no dual color" option myself.

But I like this suggestion even better.

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23 21:18   ` Junio C Hamano
  2018-08-23 21:27     ` Kyle Meyer
@ 2018-08-23 21:41     ` Johannes Schindelin
  1 sibling, 0 replies; 20+ messages in thread
From: Johannes Schindelin @ 2018-08-23 21:41 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Kyle Meyer, git

Hi Junio,

On Thu, 23 Aug 2018, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Wed, 22 Aug 2018, Kyle Meyer wrote:
> >
> >> 275267937b (range-diff: make dual-color the default mode, 2018-08-13)
> >> replaced --dual-color with --no-dual-color but left the option's
> >> summary untouched.  Rewrite the summary to describe --no-dual-color
> >> rather than dual-color.
> >> 
> >> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
> >> ---
> >>  builtin/range-diff.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/builtin/range-diff.c b/builtin/range-diff.c
> >> index f52d45d9d6..7dc90a5ec3 100644
> >> --- a/builtin/range-diff.c
> >> +++ b/builtin/range-diff.c
> >> @@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
> >>  		OPT_INTEGER(0, "creation-factor", &creation_factor,
> >>  			    N_("Percentage by which creation is weighted")),
> >>  		OPT_BOOL(0, "no-dual-color", &simple_color,
> >> -			    N_("color both diff and diff-between-diffs")),
> >> +			    N_("restrict coloring to outer diff markers")),
> >
> > How about "use simple diff colors" instead?
> 
> I am wondering if it makes sense to remove the option altogether.
> I've been trying to view the comparison of the same ranges in both
> styles for the past few days, and I never found a reason to choose
> "no dual color" option myself.

We do have a track record of making decisions based on our little bubble,
don't we.

On IRC, there is at least on publicly viewable comment by a user who
preferred the simple color diff, at least in one use case:

http://colabti.org/irclogger/irclogger_log/git-devel?date=2018-07-13#l97

And I am living in my own bubble, too. I think I heard feedback regarding
range-diff from some dozen people. Multiplying 6% by the download numbers
of Git for Windows alone... that's a lot of people who can put
--no-dual-color to good use at least in *some* situations.

In short: I am hesitant to remove a feature that would help some users.

Ciao,
Dscho

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

* Re: [PATCH] range-diff: update stale summary of --no-dual-color
  2018-08-23 21:27     ` Kyle Meyer
@ 2018-08-23 21:48       ` Junio C Hamano
  0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2018-08-23 21:48 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: Johannes Schindelin, git, Jonathan Nieder

Kyle Meyer <kyle@kyleam.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> [...]
>
>>>> -			    N_("color both diff and diff-between-diffs")),
>>>> +			    N_("restrict coloring to outer diff markers")),
>>>
>>> How about "use simple diff colors" instead?
>
> That's certainly better than the one above, and I also prefer it to
> "color only based on the diff-between-diffs" in v2.
>
>> I am wondering if it makes sense to remove the option altogether.
>> I've been trying to view the comparison of the same ranges in both
>> styles for the past few days, and I never found a reason to choose
>> "no dual color" option myself.
>
> But I like this suggestion even better.

That wasn't even a suggestion.  I just did not see the point of the
optional behaviour myself, and was soliciting concrete examples
where the --no-dual mode would help.


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

* [PATCH v3] range-diff: update stale summary of --no-dual-color
  2018-08-23 21:10             ` Jonathan Nieder
@ 2018-08-23 21:57               ` Kyle Meyer
  2018-08-23 22:02                 ` Jonathan Nieder
  0 siblings, 1 reply; 20+ messages in thread
From: Kyle Meyer @ 2018-08-23 21:57 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Kyle Meyer, git, Johannes.Schindelin

275267937b (range-diff: make dual-color the default mode, 2018-08-13)
replaced --dual-color with --no-dual-color but left the option's
summary untouched.  Rewrite the summary to describe --no-dual-color
rather than dual-color.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/range-diff.c b/builtin/range-diff.c
index f52d45d9d6..0aa9bed41f 100644
--- a/builtin/range-diff.c
+++ b/builtin/range-diff.c
@@ -25,7 +25,7 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 		OPT_INTEGER(0, "creation-factor", &creation_factor,
 			    N_("Percentage by which creation is weighted")),
 		OPT_BOOL(0, "no-dual-color", &simple_color,
-			    N_("color both diff and diff-between-diffs")),
+			    N_("use simple diff colors")),
 		OPT_END()
 	};
 	int i, j, res = 0;
-- 
2.18.0


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

* Re: [PATCH v3] range-diff: update stale summary of --no-dual-color
  2018-08-23 21:57               ` [PATCH v3] " Kyle Meyer
@ 2018-08-23 22:02                 ` Jonathan Nieder
  2018-08-23 22:08                   ` Junio C Hamano
  0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23 22:02 UTC (permalink / raw)
  To: Kyle Meyer; +Cc: git, Johannes.Schindelin

Kyle Meyer wrote:

> 275267937b (range-diff: make dual-color the default mode, 2018-08-13)
> replaced --dual-color with --no-dual-color but left the option's
> summary untouched.  Rewrite the summary to describe --no-dual-color
> rather than dual-color.
>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>

Ha, I don't think I deserve much credit here, but I'll take it. ;-)

> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
> ---
>  builtin/range-diff.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

* Re: [PATCH v3] range-diff: update stale summary of --no-dual-color
  2018-08-23 22:02                 ` Jonathan Nieder
@ 2018-08-23 22:08                   ` Junio C Hamano
  2018-08-23 22:11                     ` Jonathan Nieder
  0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2018-08-23 22:08 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Kyle Meyer, git, Johannes.Schindelin

Jonathan Nieder <jrnieder@gmail.com> writes:

> Kyle Meyer wrote:
>
>> 275267937b (range-diff: make dual-color the default mode, 2018-08-13)
>> replaced --dual-color with --no-dual-color but left the option's
>> summary untouched.  Rewrite the summary to describe --no-dual-color
>> rather than dual-color.
>>
>> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Ha, I don't think I deserve much credit here, but I'll take it. ;-)
>
>> Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>> Signed-off-by: Kyle Meyer <kyle@kyleam.com>
>> ---
>>  builtin/range-diff.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Sorry, too late.  I'll revert the merge of the previous round out of
'next' and requeue this one, but that will have to wait until the
next integration cycle.

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

* Re: [PATCH v3] range-diff: update stale summary of --no-dual-color
  2018-08-23 22:08                   ` Junio C Hamano
@ 2018-08-23 22:11                     ` Jonathan Nieder
  2018-08-27 17:57                       ` Junio C Hamano
  0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Nieder @ 2018-08-23 22:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Kyle Meyer, git, Johannes.Schindelin

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>> Kyle Meyer wrote:

>>> Subject: [PATCH v3] range-diff: update stale summary of --no-dual-color
[...]
>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Sorry, too late.  I'll revert the merge of the previous round out of
> 'next' and requeue this one, but that will have to wait until the
> next integration cycle.

Thanks for the heads up.  Sounds like a fine plan.

Jonathan

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

* Re: [PATCH v3] range-diff: update stale summary of --no-dual-color
  2018-08-23 22:11                     ` Jonathan Nieder
@ 2018-08-27 17:57                       ` Junio C Hamano
  0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2018-08-27 17:57 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Kyle Meyer, git, Johannes.Schindelin

Jonathan Nieder <jrnieder@gmail.com> writes:

> Junio C Hamano wrote:
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>> Kyle Meyer wrote:
>
>>>> Subject: [PATCH v3] range-diff: update stale summary of --no-dual-color
> [...]
>>> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
>>
>> Sorry, too late.  I'll revert the merge of the previous round out of
>> 'next' and requeue this one, but that will have to wait until the
>> next integration cycle.
>
> Thanks for the heads up.  Sounds like a fine plan.

Having said that, I do not think the change from v2 to v3 is an
improvement.  At least the one in v2 explained what the input is to
the logic to determine colors, helping the users to understand what
is painted and why and decide if that coloring is useful to them.

The phrasing in v3, "use simple diff colors", does not give much
information over saying something like "paint it differently" (which
is silly because "differently" is a given, once you give an option
to cause a non-default behaviour).

Not limited to this particular case, but in general, subjective
words like "simple" have much less information density than more
specific words, and we need to be careful when spending bits on a
limited space (like option description) to them.


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

end of thread, other threads:[~2018-08-27 17:57 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-23  2:39 [PATCH] range-diff: update stale summary of --no-dual-color Kyle Meyer
2018-08-23  2:47 ` Jonathan Nieder
2018-08-23  3:03   ` Kyle Meyer
2018-08-23  3:22     ` Jonathan Nieder
2018-08-23  4:04       ` Kyle Meyer
2018-08-23  8:27         ` Jonathan Nieder
2018-08-23 12:00           ` [PATCH v2] " Kyle Meyer
2018-08-23 21:10             ` Jonathan Nieder
2018-08-23 21:57               ` [PATCH v3] " Kyle Meyer
2018-08-23 22:02                 ` Jonathan Nieder
2018-08-23 22:08                   ` Junio C Hamano
2018-08-23 22:11                     ` Jonathan Nieder
2018-08-27 17:57                       ` Junio C Hamano
2018-08-23 14:31       ` [PATCH] " Johannes Schindelin
2018-08-23 21:12         ` Jonathan Nieder
2018-08-23 20:54 ` Johannes Schindelin
2018-08-23 21:18   ` Junio C Hamano
2018-08-23 21:27     ` Kyle Meyer
2018-08-23 21:48       ` Junio C Hamano
2018-08-23 21:41     ` Johannes Schindelin

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