Re: [PATCH] push: Provide situational hints for non-fast-forward errors

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:

I think that having three different config keys for the three
different advices makes sense, because the advices will be displayed
at different times.

I don't think it really makes sense to be such fine-grained. We already
have 6 different advices, so an advance user who do not want them need
to set these 6 variables. I think we want to keep this number relatively
low.

The advice messages do not point explicitely to the way to disable them,
so users who know how to set advice.* are users who know a little about
configuration files, and who read the docs. Instead of having too
fine-grained configuration variables, we can have a better doc,
explaining shortly the 3 possible cases under advice.nonfastforward in
config.txt. The user who disable the advice can read the doc (I usually
think that "users don't read documentation" is a better assumption, but
since the user knows about the name of the variable, it is OK here).

Also, if I read correctly the patch, the old variable is left in the doc
and in advice.{c,h}, but is no longer used. This means old-timers who
have set it will see the message poping-up again after they upgrade,
which I think is inconveinient for them.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:

quoted

I think that having three different config keys for the three
different advices makes sense, because the advices will be displayed
at different times.

I don't think it really makes sense to be such fine-grained. We already
have 6 different advices, so an advance user who do not want them need
to set these 6 variables. I think we want to keep this number relatively
low.

The advice messages do not point explicitely to the way to disable them,
so users who know how to set advice.* are users who know a little about
configuration files, and who read the docs. 

Elsewhere in this thread it was proposed to add an actual 'git config'
command to the advice.

Instead of having too
fine-grained configuration variables, we can have a better doc,
explaining shortly the 3 possible cases under advice.nonfastforward in
config.txt. The user who disable the advice can read the doc (I usually
think that "users don't read documentation" is a better assumption, but
since the user knows about the name of the variable, it is OK here).

Also, if I read correctly the patch, the old variable is left in the doc
and in advice.{c,h}, but is no longer used. This means old-timers who
have set it will see the message poping-up again after they upgrade,
which I think is inconveinient for them.

So it seems that the old variable should be respected, not to annoy
"old-timers".

Zbyszek

On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:

On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:

quoted

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:

quoted

I think that having three different config keys for the three
different advices makes sense, because the advices will be displayed
at different times.

I don't think it really makes sense to be such fine-grained. We already
have 6 different advices, so an advance user who do not want them need
to set these 6 variables. I think we want to keep this number relatively
low.

The advice messages do not point explicitely to the way to disable them,
so users who know how to set advice.* are users who know a little about
configuration files, and who read the docs. 

Elsewhere in this thread it was proposed to add an actual 'git config'
command to the advice.

After considering it, I tend to agree that three different config keys
is overkill. I feel like users who disable advice are doing it because
they find the messages annoying, not because they've mastered that
particular situation and no longer need the reminder. Forcing them to
disable three different options to get an advice-less 'git push' seems
like it'd just be irritating.

I could be wrong about that. Perhaps users who graduate workflows as you
described above are more common? I don't disable any advice locally and
thus can't speak well to what motivates that decision.

quoted

Instead of having too
fine-grained configuration variables, we can have a better doc,
explaining shortly the 3 possible cases under advice.nonfastforward in
config.txt. The user who disable the advice can read the doc (I usually
think that "users don't read documentation" is a better assumption, but
since the user knows about the name of the variable, it is OK here).

Also, if I read correctly the patch, the old variable is left in the doc
and in advice.{c,h}, but is no longer used. This means old-timers who
have set it will see the message poping-up again after they upgrade,
which I think is inconveinient for them.

So it seems that the old variable should be respected, not to annoy
"old-timers".

I hadn't considered users who already have the variable set. I'll
correct for that. I'll also attempt to improve the doc for
advice.nonfastforward.

--
Christopher Tiwald

On Wed, Mar 14, 2012 at 03:27:52PM +0100, Zbigniew Jędrzejewski-Szmek wrote:

On Wed, Mar 14, 2012 at 02:00:38PM +0100, Matthieu Moy wrote:

quoted

Zbigniew Jędrzejewski-Szmek [off-list ref] writes:

quoted

I think that having three different config keys for the three
different advices makes sense, because the advices will be displayed
at different times.

I don't think it really makes sense to be such fine-grained. We already
have 6 different advices, so an advance user who do not want them need
to set these 6 variables. I think we want to keep this number relatively
low.

The advice messages do not point explicitely to the way to disable them,
so users who know how to set advice.* are users who know a little about
configuration files, and who read the docs. 

Elsewhere in this thread it was proposed to add an actual 'git config'
command to the advice.

The proposed command does not turn off the advice. It only changes
push.default. The advice about push.default is effectively disabled once
they change push.default, but the other warnings are still in effect.