Re: master 4a1e97b: New user option for Git log switches

Re: master 4a1e97b: New user option for Git log switches

[Michael Albinus]

dgutov@yandex.ru (Dmitry Gutov) writes:

Hi,

sorry for being late.

> +(defcustom vc-git-log-switches t
> +  "String or list of strings specifying switches for Git log under VC."
> +  :type '(choice (const :tag "None" t)
> +                 (string :tag "Argument String")
> +                 (repeat :tag "Argument List" :value ("") string))
> +  :version "28.1")

Is there a reason to use the t value for "None"? All other log switches
(vc-bzr-log-switches, vc-hg-log-switches, vc-src-log-switches) use nil
for this purpose, and it sounds also more natural to me as being an
empty list of strings.

Best regards, Michael.

Re: master 4a1e97b: New user option for Git log switches

[Dmitry Gutov]

Hi Michael,

On 09.06.2021 14:14, Michael Albinus wrote:

>> +(defcustom vc-git-log-switches t
>> +  "String or list of strings specifying switches for Git log under VC."
>> +  :type '(choice (const :tag "None" t)
>> +                 (string :tag "Argument String")
>> +                 (repeat :tag "Argument List" :value ("") string))
>> +  :version "28.1")
> 
> Is there a reason to use the t value for "None"? All other log switches
> (vc-bzr-log-switches, vc-hg-log-switches, vc-src-log-switches) use nil
> for this purpose, and it sounds also more natural to me as being an
> empty list of strings.

nil also works.

The author followed the example of 'diff switches', where there is a 
certain hierarchy of variables to follow if the backend-specific one 
returns nil.

I figured it wouldn't be too bad to allow that possibility in the 
future, but you are probably right and we should simplify this.

'hg log' and 'git log' are pretty different after all.

Re: master 4a1e97b: New user option for Git log switches

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

>>> +(defcustom vc-git-log-switches t
>>> +  "String or list of strings specifying switches for Git log under VC."
>>> +  :type '(choice (const :tag "None" t)
>>> +                 (string :tag "Argument String")
>>> +                 (repeat :tag "Argument List" :value ("") string))
>>> +  :version "28.1")
>> Is there a reason to use the t value for "None"? All other log
>> switches
>> (vc-bzr-log-switches, vc-hg-log-switches, vc-src-log-switches) use nil
>> for this purpose, and it sounds also more natural to me as being an
>> empty list of strings.
>
> nil also works.
>
> The author followed the example of 'diff switches', where there is a
> certain hierarchy of variables to follow if the backend-specific one
> returns nil.

Existing vc-*-diff-switches declare

  :type '(choice (const :tag "Unspecified" nil)
                 (const :tag "None" t)
                 (string :tag "Argument String")
                 (repeat :tag "Argument List" :value ("") string)))

Existing vc-*-log-switches declare

  :type '(choice (const :tag "None" nil)
                 (string :tag "Argument String")
                 (repeat :tag "Argument List" :value ("") string)))

> I figured it wouldn't be too bad to allow that possibility in the
> future, but you are probably right and we should simplify this.

In favor of uniformity, vc-git-log-switches might follow the scheme.

> 'hg log' and 'git log' are pretty different after all.

Sure. But we're speaking about defcustom types, aren't we?

Best regards, Michael.

Re: master 4a1e97b: New user option for Git log switches

[Utkarsh Singh]

Hi Michael,

Sorry for the late reply.

On 2021-06-10, 09:34 +0200, Michael Albinus <michael.albinus@gmx.de> wrote:

> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> Hi Michael,
>
> Hi Dmitry,
>
>>>> +(defcustom vc-git-log-switches t
>>>> +  "String or list of strings specifying switches for Git log under VC."
>>>> +  :type '(choice (const :tag "None" t)
>>>> +                 (string :tag "Argument String")
>>>> +                 (repeat :tag "Argument List" :value ("") string))
>>>> +  :version "28.1")
>>> Is there a reason to use the t value for "None"? All other log
>>> switches
>>> (vc-bzr-log-switches, vc-hg-log-switches, vc-src-log-switches) use nil
>>> for this purpose, and it sounds also more natural to me as being an
>>> empty list of strings.
>>
>> nil also works.
>>
>> The author followed the example of 'diff switches', where there is a
>> certain hierarchy of variables to follow if the backend-specific one
>> returns nil.
>
> Existing vc-*-diff-switches declare
>
>   :type '(choice (const :tag "Unspecified" nil)
>                  (const :tag "None" t)
>                  (string :tag "Argument String")
>                  (repeat :tag "Argument List" :value ("") string)))
>
> Existing vc-*-log-switches declare
>
>   :type '(choice (const :tag "None" nil)
>                  (string :tag "Argument String")
>                  (repeat :tag "Argument List" :value ("") string)))
>
>> I figured it wouldn't be too bad to allow that possibility in the
>> future, but you are probably right and we should simplify this.
>
> In favor of uniformity, vc-git-log-switches might follow the scheme.
>
>> 'hg log' and 'git log' are pretty different after all.
>
> Sure. But we're speaking about defcustom types, aren't we?

Indeed! I think default value of `nil' is much more suitable but these
are just my gut feelings as currently I don't have much knowledge of how
VC is designed.

-- 
Utkarsh Singh
http://utkarshsingh.xyz

Re: master 4a1e97b: New user option for Git log switches

[Dmitry Gutov]

On 10.06.2021 17:56, Utkarsh Singh wrote:
> Indeed! I think default value of `nil' is much more suitable but these
> are just my gut feelings as currently I don't have much knowledge of how
> VC is designed.

I just pushed a simplifying commit that changed the default to nil.

Check it out sometime.

Re: master 4a1e97b: New user option for Git log switches

[Dmitry Gutov]

On 10.06.2021 10:34, Michael Albinus wrote:

> Existing vc-*-diff-switches declare
> 
>    :type '(choice (const :tag "Unspecified" nil)
>                   (const :tag "None" t)
>                   (string :tag "Argument String")
>                   (repeat :tag "Argument List" :value ("") string)))
> 
> Existing vc-*-log-switches declare
> 
>    :type '(choice (const :tag "None" nil)
>                   (string :tag "Argument String")
>                   (repeat :tag "Argument List" :value ("") string)))

Right, so it's either follow the example of existing vc-git*-switches 
options, or of vc-*-log-switches options.

But to be frank the 'vc-annotate-switches' option is pretty useless, so 
the latter seems to be the better example.

>> I figured it wouldn't be too bad to allow that possibility in the
>> future, but you are probably right and we should simplify this.
> 
> In favor of uniformity, vc-git-log-switches might follow the scheme.

Now done in commit 86309efb4a.

>> 'hg log' and 'git log' are pretty different after all.
> 
> Sure. But we're speaking about defcustom types, aren't we?

We were talking about whether having a possible value 't' is helpful for 
the discussed option. And that value allows delegating to a 
cross-backend variable, which is only useful when different backends can 
accept the same options.