Re: [PATCH 3/3] git-am: add am.threeWay config variable

From: Junio C Hamano <gitster@pobox.com>
Date: May 28, 2015, 17:57 UTC
Message-ID: <xmqqy4k8h9f8.fsf@gitster.dls.corp.google.com>
In-Reply-To: <CACRoPnSnwA5GcYZjHHTepz4WzXYuJ+9nJqGsL92r_vnsNrN_-w@mail.gmail.com>

Paul Tan <pyokagan@gmail.com> writes:

Show 12 quoted lines

>> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
>> index 0d8ba48..3190c05 100644
>> --- a/Documentation/git-am.txt
>> +++ b/Documentation/git-am.txt
>> @@ -89,11 +89,13 @@ default.   You can use `--no-utf8` to override this.
>>         linkgit:git-mailinfo[1]).
>>
>>  -3::
>> ---3way::
>> +--[no-]3way::
>
> There's no need to mention --no-3way,...

Actually, we prefer to do it this way:

	-3::
	--3way::
	--no-3way::
		Describe what --3way does here.

$ git grep -e '^--no-' -e '^--\[no-\]' Documentation/

Show 10 quoted lines

>>         When the patch does not apply cleanly, fall back on
>>         3-way merge if the patch records the identity of blobs
>>         it is supposed to apply to and we have those blobs
>> -       available locally.
>> +       available locally.  `am.threeWay` configuration variable
>> +       can be used to specify the default behaviour.  `--no-3way`
>> +       is useful to override `am.threeWay`.
>
> Usually configuration settings are mentioned in a separate section in
> the documentation "CONFIGURATION" (or not mentioned at all).

I can go either way, actually. But if the description mentions am.threeWay as a way to tweak the default, it also should spell out the default when the configuration is not there at all.

> Also, there's no need to mention that --no-3way can be used to
> mention the configuration, as its usual (and expected) that the
> configuration value sets the default behavior, and the
> command-line switch can override i.

Yes.  Also --3way is useful to override `am.threeWay` set to `false` ;-)

Show 15 quoted lines

> To end off, some off-tangent issues that are not related to the patch
> series in question, but since I'm looking at git-am.sh....
>
> I've noticed that in the block above that initializes all the variables,
>
>     sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
>     messageid= resolvemsg= resume= scissors= no_inbody_headers=
>     git_apply_opt=
>     committer_date_is_author_date=
>     ignore_date=
>     allow_rerere_autoupdate=
>     gpg_sign_opt=
>
> threeway is not initialized at all, and thus I think running
> "threeway=t git am blah" will affect the behavior of git-am.

Correct. I overlooked this when I originally did threeway. Perhaps a preparatory bugfix patch is warranted before this one.

> Also, I noticed that we do not check for --no-interactive,
> --no-signoff, --no-keep, --no-whitespace, etc.

Even though adding support for them would not hurt, lack of these are OK, as long as we do not have configuration variables to tweak their defaults.