range-diff: update stale summary of --no-dual-color

PATCH Superseded Kyle Meyer 4 participants 20 messages Aug 23, 2018 – Aug 27, 2018

Share & tools

Kyle Meyer

author

2018-08-23 02:40 UTC #0 lore raw

PATCH 1/1 range-diff: update stale summary of --no-dual-color

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 <[email protected]>

---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/range-diff.c +1 -1

index f52d45d9d6..7dc90a5ec3 100644
@@ -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;

Signature · 1 line

2.18.0

Jonathan Nieder

2018-08-23 02:47 UTC #1 lore raw

Hi,

Quoted 9 lines · 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 <[email protected]>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, good catch!

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28

Quoted 10 lines

builtin/range-diff.c +1 -1

index f52d45d9d6..7dc90a5ec3 100644
@@ -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

Kyle Meyer

author

2018-08-23 03:03 UTC #2 lore raw

Jonathan Nieder <[email protected]> writes:

[...]

Quoted 1 line

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

Signature · 1 line

Kyle

Jonathan Nieder

2018-08-23 03:22 UTC #3 lore raw

Quoted 5 lines · Kyle Meyer

Quoted 4 lines · Jonathan Nieder
Quoted 2 lines · Kyle Meyer
(quoted diff) +1 -1
-			    N_("color both diff and diff-between-diffs")),
+			    N_("restrict coloring to outer diff markers")),

[...]

Quoted 5 lines

Quoted 1 line
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:

Quoted 7 lines

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

builtin/range-diff.c +5 -5

index f52d45d9d6..88c19f48d3 100644
@@ -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;

Kyle Meyer

author

2018-08-23 04:12 UTC #4 lore raw

Quoted 2 lines · Jonathan Nieder

Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
(quoted diff) +1 -1
-			    N_("color both diff and diff-between-diffs")),
+			    N_("restrict coloring to outer diff markers")),

[...]

Quoted 9 lines

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.

Quoted 2 lines

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

Quoted 10 lines

Or it might be simpler to do something like
the following. What do you think?
builtin/range-diff.c +0 -0
index f52d45d9d6..88c19f48d3 100644
@@ -20,12 +20,12 @@ int cmd_range_diff(int argc, const char **argv, const char *prefix)
 {
 	int creation_factor = 60;

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28

Quoted 10 lines

builtin/range-diff.c +3 -3

 	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.

Jonathan Nieder

2018-08-23 08:27 UTC #5 lore raw

Quoted 6 lines · Kyle Meyer

Quoted 2 lines · Jonathan Nieder
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

Kyle Meyer

author

2018-08-23 12:00 UTC #6 lore raw

PATCH v2 1/1 range-diff: update stale summary of --no-dual-color

Helped-by Jonathan Nieder <[email protected]>

Signed-off-by Kyle Meyer <[email protected]>

---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/range-diff.c +1 -1

index f52d45d9d6..057afdbf46 100644
@@ -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;

Signature · 1 line

2.18.0

Jonathan Nieder

Reviewed-by on v2 1/1

2018-08-23 21:10 UTC #7 lore raw

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28

Quoted 3 lines · Kyle Meyer

builtin/range-diff.c +1 -1

 		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 <[email protected]>

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

Thanks,
Jonathan

Kyle Meyer

author

2018-08-23 21:57 UTC #8 lore raw

PATCH v3 1/1 range-diff: update stale summary of --no-dual-color

Helped-by Jonathan Nieder <[email protected]>

Helped-by Johannes Schindelin <[email protected]>

Signed-off-by Kyle Meyer <[email protected]>

---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

builtin/range-diff.c +1 -1

index f52d45d9d6..0aa9bed41f 100644
@@ -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;

Signature · 1 line

2.18.0

Jonathan Nieder

Reviewed-by on v3 1/1

2018-08-23 22:02 UTC #9 lore raw

Quoted 6 lines · 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 <[email protected]>

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

Quoted 5 lines

Helped-by Johannes Schindelin <[email protected]>
Signed-off-by Kyle Meyer <[email protected]>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by Jonathan Nieder <[email protected]>

Junio C Hamano

2018-08-23 22:09 UTC #10 lore raw

Quoted 11 lines · Jonathan Nieder

Quoted 6 lines · 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 <[email protected]>
Ha, I don't think I deserve much credit here, but I'll take it. ;-)

Quoted 7 lines

Quoted 5 lines
Helped-by Johannes Schindelin <[email protected]>
Signed-off-by Kyle Meyer <[email protected]>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by Jonathan Nieder <[email protected]>

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.

Jonathan Nieder

2018-08-23 22:11 UTC #11 lore raw

Quoted 4 lines · Junio C Hamano

Quoted 3 lines · Jonathan Nieder
Quoted 1 line · Kyle Meyer
Subject: [PATCH v3] range-diff: update stale summary of --no-dual-color

[...]

Quoted 5 lines

Quoted 1 line
Reviewed-by Jonathan Nieder <[email protected]>
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

Junio C Hamano

2018-08-27 17:57 UTC #12 lore raw

Quoted 5 lines · Jonathan Nieder

Quoted 4 lines · Junio C Hamano
Quoted 3 lines · Jonathan Nieder
Quoted 1 line · Kyle Meyer
Subject: [PATCH v3] range-diff: update stale summary of --no-dual-color

Quoted 8 lines

[...]
Quoted 5 lines
Quoted 1 line
Reviewed-by Jonathan Nieder <[email protected]>
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.

Johannes Schindelin

2018-08-23 14:31 UTC #13 lore raw

In reply to Jonathan Nieder

Hi Jonathan,

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28

Quoted 6 lines · Jonathan Nieder

builtin/range-diff.c +2 -2

 		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

Quoted 15 lines

builtin/range-diff.c +2 -2

 		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;

Jonathan Nieder

2018-08-23 21:12 UTC #14 lore raw

Hi,

Quoted 14 lines · Johannes Schindelin

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28
Quoted 6 lines · Jonathan Nieder
builtin/range-diff.c +2 -2
 		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

Johannes Schindelin

2018-08-23 20:54 UTC #15 lore raw

In reply to Kyle Meyer

Hi Kyle,

Quoted 15 lines · 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 <[email protected]>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/range-diff.c +0 -0
index f52d45d9d6..7dc90a5ec3 100644

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28

Quoted 5 lines

builtin/range-diff.c +1 -1

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

Junio C Hamano

2018-08-23 21:18 UTC #16 lore raw

Quoted 12 lines · Johannes Schindelin

Quoted 10 lines · 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 <[email protected]>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Quoted 12 lines

builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28

Quoted 10 lines

builtin/range-diff.c +1 -1

index f52d45d9d6..7dc90a5ec3 100644
@@ -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.

Kyle Meyer

author

2018-08-23 21:27 UTC #17 lore raw

Quoted 1 line · Junio C Hamano

Johannes Schindelin <[email protected]> writes:

[...]

Quoted 4 lines

Quoted 4 lines
builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28
Quoted 2 lines
builtin/range-diff.c +1 -1
-			    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.

Quoted 4 lines

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.

Junio C Hamano

2018-08-23 21:48 UTC #18 lore raw

Quoted 14 lines · Kyle Meyer

Quoted 1 line · Junio C Hamano
Johannes Schindelin <[email protected]> writes:
[...]
Quoted 4 lines
Quoted 4 lines
builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28
Quoted 2 lines
builtin/range-diff.c +1 -1
-			    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.

Quoted 6 lines

Quoted 4 lines
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.

Johannes Schindelin

2018-08-23 21:41 UTC #19 lore raw

In reply to Junio C Hamano

Hi Junio,

Quoted 27 lines · Junio C Hamano

Quoted 12 lines · Johannes Schindelin
Quoted 10 lines · 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 <[email protected]>
---
 builtin/range-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Quoted 12 lines
builtin/range-diff.c int cmd_range_diff(int argc, const char **argv, const char *prefix) · line 28
Quoted 10 lines
builtin/range-diff.c +1 -1
index f52d45d9d6..7dc90a5ec3 100644
@@ -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?

Quoted 4 lines

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