unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* 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

unofficial mirror of emacs-devel@gnu.org 

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://yhetil.org/emacs-devel/0 emacs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 emacs-devel emacs-devel/ https://yhetil.org/emacs-devel \
		emacs-devel@gnu.org
	public-inbox-index emacs-devel

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.yhetil.org/yhetil.emacs.devel
	nntp://news.gmane.io/gmane.emacs.devel


code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs.git

AGPL code for this site: git clone http://ou63pmih66umazou.onion/public-inbox.git