is this a bug of git-diff?

The output of git-diff is different from my expectation. It may skip some lines of context. For the case of the diff result attached here, a blank line and a line with a leading slash is skipped.

Please check out the attached files for details.

Thanks.

index 1ef2bf5..ef2206b 100644

On Wed, May 15, 2013 at 8:23 AM, eric liou <accwuya@gmail.com> wrote:

git-diff is using a default of 3 lines of context above and below the changes. In your example, there is only two lines of context below the change, so only two lines are displayed. Above the change, three lines are displayed, as expected. That's why the blank line and leading slash line are not displayed. You can change the number of context lines by invoking git-diff with -U<n>.

Hope that helps, Antoine

On Wed, May 15, 2013 at 8:52 AM, eric liou <accwuya@gmail.com> wrote:

Oh OK, I see. Git tries to name the function where the changes take place. This is purely informative. In your example, you don't have any function so of course the information is not very helpful.

Typically it will look like the following, helping the reader by giving the function name:

Antoine Pelisse <apelisse@gmail.com> writes:

Antoine's answer is correct. In addition, I'd say that you may want to enable color in the output to make it clearer (the @@ ... @@ part would be colored, but not the function name):

  git config --global color.ui auto

On Wed, May 15, 2013 at 11:34:41AM +0200, Matthieu Moy wrote:

I wonder if that should be the default. I've advised a lot of people to turn it on and it seems to me that a user is much more likely to go looking for a "turn color off" option than realise that color is an option at all.

John Keeping <john@keeping.me.uk> writes:

I'd love to see this by default, yes. Maybe a 2.0 change?

If people agree that this is a good change, would we need a transition plan? I'd say no, as there is no real backward incompatibility involved. People who dislike colors can already set color.ui=false, and seeing colors can hardly harm them, just temporarily reduce the comfort for them.

On Wed, May 15, 2013 at 5:03 AM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

I vote for this. It's the first thing I do in any setup, even the ones that are note mine. I've also seen it in basically all the tutorials, even before setting user.name/email.

I also don't see the point of a transition plan.

Most users seem to like having colors enabled, and colors can help beginners to understand the output of some commands (e.g. notice immediately the boundary between commits in the output of "git log").

Many tutorials tell the users to set color.ui=auto as a very first step. These tutorials would benefit from skiping this step and starting the real Git manipualtions earlier. Other beginners do not know about color.ui=auto, and may not discover it by themselves, hence live with black&white outputs while they may have prefered colors.

A few people (e.g. color-blind) prefer having no colors, but they can easily set color.ui=never for this (and googling "disable colors in git" already tells them how to do so).

A transition period with Git emitting a warning when color.ui is unset would be possible, but the discomfort of having the warning seems superior to the benefit: users may be surprised by the change, but not harmed by it.

The default value is changed, and the documentation is reworded to mention "color.ui=false" first, since the primary use of color.ui after this change is to disable colors, not to enable it.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

OK, then let's try turning the discussion into code.

I'm starting to wonder why we didn't do this earlier ;-).

 Documentation/config.txt | 11 ++++++-----
 color.c                  |  2 +-
 2 files changed, 7 insertions(+), 6 deletions(-)

On Wed, May 15, 2013 at 2:09 PM, Matthieu Moy <Matthieu.Moy@imag.fr> wrote:

Reviewed and supported-by: Johan Herland <johan@herland.net>

...Johan

Most users seem to like having colors enabled, and colors can help beginners to understand the output of some commands (e.g. notice immediately the boundary between commits in the output of "git log").

Many tutorials tell the users to set color.ui=auto as a very first step. These tutorials would benefit from skiping this step and starting the real Git manipualtions earlier. Other beginners do not know about color.ui=auto, and may not discover it by themselves, hence live with black&white outputs while they may have prefered colors.

A few people (e.g. color-blind) prefer having no colors, but they can easily set color.ui=never for this (and googling "disable colors in git" already tells them how to do so).

A transition period with Git emitting a warning when color.ui is unset would be possible, but the discomfort of having the warning seems superior to the benefit: users may be surprised by the change, but not harmed by it.

The default value is changed, and the documentation is reworded to mention "color.ui=false" first, since the primary use of color.ui after this change is to disable colors, not to enable it.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---

Apparently not well enough ;-).

In v1, "git config --get-colorbool" was not affected, hence "git add -p" wasn't colored. v2 fixes this.

 Documentation/config.txt | 11 ++++++-----
 builtin/config.c         |  2 +-
 color.c                  |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

It is curious to notice that we have these three and only one is initialized to the new default value, while the other two get -1 at the beginning of get_colorbool().

I wonder if it would be cleaner to statically initialize all three to -1 here, drop the assignment of -1 to two of them from the beginning of get_colorbool(), and then have a final fallback inside the want_color() call itself, i.e.

	get_colorbool_found = want_color(get_colorbool_found < 0
        				? GIT_COLOR_AUTO
                                        : get_colorbool_found);

so that it is clear that -1 consistently mean "We haven't read any value from the configuration file for this variable", instead of making get_color_ui_found mean slightly different thing (the value read from the configuration; GIT_COLOR_AUTO means we cannot tell if we saw this variable or the user specified auto) from the other two (the value read from the configuration; -1 means we did not find any).

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

Right. The meaning of the _found suffix is clear for the first two, but not the last.

I've left the assignments within the function (I like the initialisation right before usage, I don't have to worry about how many times the function is called then), but I've added a patch that initializes get_color_ui_found to -1 like the others, and does essentially this:

Except I've made it a separate if statement. Then PATCH 2/2 is really crystal clear.

Reroll comming, with an improved commit message that should adress the points in the other message.

The meaning of get_colorbool_found and get_diff_color_found is "the config value if found, and -1 otherwise", but get_color_ui_found had a slightly different meaning, as it has the value 0 (which corresponds to the default value from the user point of view) when color.ui is unset.

Make get_color_ui_found default to -1, and make it explicit that 0 is the default value when nothing else is found.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
So, this is new, as suggested by Junio.

 builtin/config.c | 5 +++++
 1 file changed, 5 insertions(+)

Most users seem to like having colors enabled, and colors can help beginners to understand the output of some commands (e.g. notice immediately the boundary between commits in the output of "git log").

Many tutorials tell the users to set color.ui=auto as a very first step, which tend to indicate that color.ui=none is not the recommanded value, hence should not be the default.

These tutorials would benefit from skipping this step and starting the real Git manipulations earlier. Other beginners do not know about color.ui=auto, and may not discover it by themselves, hence live with black&white outputs while they may have preferred colors.

A few people (e.g. color-blind) prefer having no colors, but they can easily set color.ui=never for this (and googling "disable colors in git" already tells them how to do so), but this needs not occupy space in beginner-oriented documentations.

A transition period with Git emitting a warning when color.ui is unset would be possible, but the discomfort of having the warning seems superior to the benefit: users may be surprised by the change, but not harmed by it.

The default value is changed, and the documentation is reworded to mention "color.ui=false" first, since the primary use of color.ui after this change is to disable colors, not to enable it.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
Adapted after PATCH 1/2, and commit message updated.

 Documentation/config.txt | 11 ++++++-----
 builtin/config.c         |  2 +-
 color.c                  |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Yeah, sounds good.

Hmm, I don't see much improvement in the message, though. It seems to talk about "may not discover", "live with", "a few people", and "they can easily", none of which should be there.

On 05/15/2013 02:09 PM, Matthieu Moy wrote:

s/skiping/skipping/

s/manipualtions/manipulations/

s/prefered/preferred/

With the typos above fixed:

  Reviewed and supported-by: Stefano Lattarini <stefano.lattarini@gmail.com>

Thanks,
  Stefano

Most users seem to like having colors enabled, and colors can help beginners to understand the output of some commands (e.g. notice immediately the boundary between commits in the output of "git log").

Many tutorials tell the users to set color.ui=auto as a very first step. These tutorials would benefit from skipping this step and starting the real Git manipulations earlier. Other beginners do not know about color.ui=auto, and may not discover it by themselves, hence live with black&white outputs while they may have preferred colors.

A few people (e.g. color-blind) prefer having no colors, but they can easily set color.ui=never for this (and googling "disable colors in git" already tells them how to do so).

A transition period with Git emitting a warning when color.ui is unset would be possible, but the discomfort of having the warning seems superior to the benefit: users may be surprised by the change, but not harmed by it.

The default value is changed, and the documentation is reworded to mention "color.ui=false" first, since the primary use of color.ui after this change is to disable colors, not to enable it.

Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
v2 crossed Stefano's email with typos. v3 just fixes these typos in
the commit message.

 Documentation/config.txt | 11 ++++++-----
 builtin/config.c         |  2 +-
 color.c                  |  2 +-
 3 files changed, 8 insertions(+), 7 deletions(-)

Matthieu Moy <Matthieu.Moy@imag.fr> writes:

The above two paragraphs do not make a good justification [*1*]. The former can just as easily websearch for "enable colours in git" as the latter would for "disable" in order to avoid having to live with distraction while they may have preferred monochrome.

The train of thought that is a sufficient justification for this change is "Our document and third-party tutorials often start with setting color.ui=auto configuration." leading to "Our recommendation is to enable colour on terminals." which in turn leading to "Why is our default monochrome, against our own recommendation?". Saying anything more, like who are the majority or how easily the default can be overridden, is unnecessary, I think [*2*].

As this is purely a UI thing, and since daa0c3d97176 (color: delay auto-color decision until point of use, 2011-08-17), the logic to decide when "auto colouring" is triggered is centrary controlled (hence it is much less likely than before that color.ui=auto could misfire when it shouldn't), I agree that this does not even deserve a warning. You could even sell it as a pure bugfix ("we recommend users to use auto colouring but we did not set it up for users").

Good.

OK, so this is planned for 2.0?

[Footnote]

*1* Unless you have some statistical fact to demonstrate that beginners who prefer colours are of lessor intelligence than those who do not, that is.

*2* It unnecessarily muddies the water to bring up "which is majority?". A poll might reveal more people prefer monochrome, but in that case, either we keep the default monochrome *and* fix the tutorial not to suggest auto, or we stick to the recommendation to use auto colouring. In other words, I see this change as merely making the code in line with the spirit of the documentation.

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

I disagree: I do not know anyone who would be really harmed by colors (and such users would most likely have a terminal configured without colors I guess), so although I can imagine some people feeling less comfortable, disabling colors can be deferred to much later in the learning process.

When I teach Git to students (a relatively short tutorial), I currently ask them to type a ~/.gitconfig containing color.ui=auto before anything else. If this was the default, I would skip this completely from the beginner-oriented doc, and I would mention color.ui=never only to people complaining about colors. It's really about _skipping_ the color-related stuff from the newbie docs, not about reverting them.

Also, as my message points out, with "disabled by default", many people do not know that it is possible to have it, hence won't google for anything related to colors. There's no symmetry either here: with colors enabled by default, people will know that Git can use colors.

We've lived without this for years, so I'd say it can wait untill Git 2.0. It may give a "Wow" effect to some users when upgrading ;-).

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

I actually am one of them (light cyan or green on white background with small font is very hard to read for me), but I think you are missing the entire point, which is not "is anyone harmed?"

This patch is not even about deciding if colored output should be the default. That has already been decided by documentation for us long time ago; the patch does not have to (and should not) argue for and justify why color is good. Our recommendation has been "use color=auto", and change of the in-code default is merely to make the default in line with that recommendation.

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

Again, it is. If the new default is really harmful for too many people, then documentations will have to mention how to fix it.

And really, I do not forsee any newbie-oriented starting with "here's how to disable colors in case you need it", because of the reasons mentionned in the message.

Not really. Neither Documentation/gittutorial.txt nor Documentation/user-manual.txt mention colors. Pro Git mentions it, but more as a possibility than as a recommandation. This is the recommandation of the rest of the world, not "ours".

It's not "either we update the docs or we update the code", it's "follow what the rest of the world is doing", and "rest of the world" has to imply a notion of majority (not all tutorials talk about color.ui).

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Do you mean the git users who learn and use Git without being in the circle of people who updates Documentation/ hierarchy "the rest of the world"?

I think that is a flawed mentality.  They are part of "us".

Yes, exactly.

Read the statement you made again, with the assumption that everybody ("the rest of the world") already knows (and/or agreed to) colouring is a good thing.

Now, realize that after switching the default, these "few people" have to live with distracting (or unreadable) output. Because these people are minority, their websearch "disable colors in git" will by definition have smaller number of hits than "enable colors in git" the above claims people "may not discover it by themselves". In a way, you are making things even harder because these minority do not have many similar others to ask help for.

That is the honest way to express what you said in the second paragraph.

If we really want to justify the changing of the default, we should not try to weasel out by using asymmetric wording from the fact that we are making things less convenient for one kind of people. We should be honest and say what we are doing: "it will make things easier for majority while making it less convenient for minority".

I am however saying that in this case, we are better off not even trying to come up with such a lame excuse for us to hurt color-blind people in order to make things easier for majority. Just saying "the rest of the world prefer automatic color and that is what we recommend, so make the code match" should be sufficient.

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

As my message says, "disable colors in git" already gives you the answer, today (1st hit in Google). I'm not worried about the difficulty to find the information in the future.

I thought this was what I did, but your first complain was I was mentionning the majority, and you are now suggesting something about majority/minority, so I'm lost.

In any case, feel free to change the commit message, what's really important is the actual change, and it does not seem controversial.

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

Not really. My main complaint is that you were making it sound as if the inconvenience for the "majority" is very severe with "many not discover", "live with", and such phrases, while making the inconveience you are placing on the "minority" trivial with "easily set" and "already tells them". That sounds a lot more like making a lame excuse than doing a balanced analysis of pros and cons of the change.

On Wed, May 15, 2013 at 1:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

I could barely parse this, but I've found that many colleagues didn't know about this configuration. And I don't see why anybody would not want this. The minority that don't want this can search the interwebs to find out how to disable the unwanted behavior, so the majority that do want this don't have to enable it all the time (*if* they know about it).

On Wed, May 15, 2013 at 08:42:39AM -0700, Junio C Hamano wrote:

I would vote for just considering this a bugfix as you say above and therefore not worthy of any special treatment, so it should end up in whatever the next release is after it hits master.

The changes that are being held back for 2.0 change how commands operate and we don't provide any overrides for those; this is just a cosmetic change to the default output format.

On Wed, May 15, 2013 at 10:50:25AM +0100, John Keeping wrote:

+1. My settings have been there for so long that I thought it was the default.

Mike