* Re: master 4a1e97b: New user option for Git log switches
[not found] ` <20210608233620.E895F209C6@vcs0.savannah.gnu.org>
@ 2021-06-09 11:14 ` Michael Albinus
2021-06-09 16:01 ` Dmitry Gutov
0 siblings, 1 reply; 6+ messages in thread
From: Michael Albinus @ 2021-06-09 11:14 UTC (permalink / raw)
To: emacs-devel; +Cc: Utkarsh Singh
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: master 4a1e97b: New user option for Git log switches
2021-06-09 11:14 ` master 4a1e97b: New user option for Git log switches Michael Albinus
@ 2021-06-09 16:01 ` Dmitry Gutov
2021-06-10 7:34 ` Michael Albinus
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Gutov @ 2021-06-09 16:01 UTC (permalink / raw)
To: Michael Albinus, emacs-devel; +Cc: Utkarsh Singh
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: master 4a1e97b: New user option for Git log switches
2021-06-09 16:01 ` Dmitry Gutov
@ 2021-06-10 7:34 ` Michael Albinus
2021-06-10 14:56 ` Utkarsh Singh
2021-06-11 1:40 ` Dmitry Gutov
0 siblings, 2 replies; 6+ messages in thread
From: Michael Albinus @ 2021-06-10 7:34 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: Utkarsh Singh, emacs-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: master 4a1e97b: New user option for Git log switches
2021-06-10 7:34 ` Michael Albinus
@ 2021-06-10 14:56 ` Utkarsh Singh
2021-06-11 1:41 ` Dmitry Gutov
2021-06-11 1:40 ` Dmitry Gutov
1 sibling, 1 reply; 6+ messages in thread
From: Utkarsh Singh @ 2021-06-10 14:56 UTC (permalink / raw)
To: Michael Albinus, Dmitry Gutov; +Cc: emacs-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: master 4a1e97b: New user option for Git log switches
2021-06-10 7:34 ` Michael Albinus
2021-06-10 14:56 ` Utkarsh Singh
@ 2021-06-11 1:40 ` Dmitry Gutov
1 sibling, 0 replies; 6+ messages in thread
From: Dmitry Gutov @ 2021-06-11 1:40 UTC (permalink / raw)
To: Michael Albinus; +Cc: Utkarsh Singh, emacs-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: master 4a1e97b: New user option for Git log switches
2021-06-10 14:56 ` Utkarsh Singh
@ 2021-06-11 1:41 ` Dmitry Gutov
0 siblings, 0 replies; 6+ messages in thread
From: Dmitry Gutov @ 2021-06-11 1:41 UTC (permalink / raw)
To: Utkarsh Singh, Michael Albinus; +Cc: emacs-devel
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-06-11 1:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20210608233619.8439.15341@vcs0.savannah.gnu.org>
[not found] ` <20210608233620.E895F209C6@vcs0.savannah.gnu.org>
2021-06-09 11:14 ` master 4a1e97b: New user option for Git log switches Michael Albinus
2021-06-09 16:01 ` Dmitry Gutov
2021-06-10 7:34 ` Michael Albinus
2021-06-10 14:56 ` Utkarsh Singh
2021-06-11 1:41 ` Dmitry Gutov
2021-06-11 1:40 ` Dmitry Gutov
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).