Philip Kaludercic <philipk@posteo.net> writes:

>>>>  (defcustom package-vc-default-backend 'Git
>>>> -  "Default VC backend used when cloning a package repository.
>>>> -If no repository type was specified or could be guessed by
>>>> -`package-vc-heuristic-alist', this is the default VC backend
>>>> -used as fallback.  The value must be a member of
>>>> -`vc-handled-backends' and the named backend must implement
>>>> -the `clone' function."
>>>> -  :type `(choice ,@(mapcar (lambda (b) (list 'const b))
>>>> -                           vc-handled-backends))
>>>> +  "Default VC backend to use for cloning package repositories.
>>>> +`package-vc-install' uses this backend when you specify neither
>>>> +the backend nor a repository URL that's recognized via
>>>> +`package-vc-heuristic-alist'.
>>>> +
>>>> +The value must be a member of `vc-handled-backends' that supports
>>>> +the `clone' VC function."
>>>> +  :type `(choice ,@(seq-keep
>>>> +                    (lambda (be)
>>>> +                      (when (or (vc-find-backend-function be 'clone)
>>>> +                                (alist-get 'clone (get be 'vc-functions)))
>>>> +                        (list 'const be)))
>>>> +                    vc-handled-backends))
>>>
>>> This is good, but shouldn't we do something like this in a separate
>>> patch?  No hard opinions on that though.
>>
>> I thought it was a small enough change to include along with the doc
>> improvements, but if you prefer I can update the `:type` in a separate
>> patch.  BTW, this should probably be applied also to
>> `package-vc-heuristic-alist` either way.
>
> In that case I think it would be better to prepare a separate patch.

Alright, I've extracted the `:type` refinement to a separate patch.  See
below.

> Also, would it make sense to determine this at compile-time?  On the
> other hand, if a VC backend is installed later on from ELPA, we would
> want the custom type to reflect this.

Yes, I couldn't find a way to defer computing the set of candidates to
"customization type", I'm not sure if that even makes total sense.  I
think it's not that crucial since someone adding a VC backend and
immediately trying to customize these options seems to me like a very
minor edge case, and we had the same issue prior to my patch anyhow.

> The patch looks fine to me, but considering my history with
> documentation I think it is best to let a few others take a look at your
> revision as well.

No problem, and thanks.

> Other than that, the plan is to pull out the
> `package-vc-default-backend' custom type and rename
> `package-vc-allow-side-effects', right?

Yes, I'm attaching three patches:

1. an updated (v5) doc-only patch,
2. a follow-up for refining the `defcustom` types as discussed above, and
3. a patch renaming `package-vc-allow-side-effects` to `package-vc-allow-build-commands`.