bug#43464: 28.0.50; vc: Error calling vc-revert for repo root

[Dmitry Gutov]

On 11.10.2020 23:28, Andrii Kolomoiets wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>
> > > Can you please advice me what this change should look like?  Get rid
> > > of calling 'vc-call'?
> >
> > Yes. How about the attached patch?
>
> Small fix: THEN and ELSE blocks of the '(if dir...' should be swapped.

Ah yes, thanks for noticing.

> Does those kind of changes should be applied to any function that uses
> 'vc-call' and can be called on dirs?

I think so. Since none of them should work on directories now, it should 
be accompanied with some doc changes as well.

> Is there any reason to use 'vc-backend' at all?
>
> 'vc-responsible-backend' will call 'vc-backend' on a file that is not a
> directory.

Well, vc-backend is certain to be faster, on average. That is one 
advantage. Minus one disk loopup, or minus extra network interaction 
with Tramp (that's where it might really hurt).

I don't have a very strong argument here, except the current code works, 
and it should be easier to annotate the exceptions where we do want to 
handle directories. And while we do that, consider the performance 
implications for each case.

> > > In this case the function 'vc-version-backup-file'
> > > must be changed as well.
> >
> > Does it actually make sense to use it on a directory?
>
> Looks like it make sense for CVS backend.  Take a look at
> 'vc-cvs-stay-local-p'.

It might be desired for CVS (to cut down on network traffic), but how 
would it work? The function is supposed to return a backup file name. 
But we don't create backup files for whole directories. Only for 
individual files.

> > Something like that. Or 'git init' inside a subdirectory. Not a
> > frequent occurrence, but if we start using directories and files
> > interchangeably in more places, we are likely to start caching other
> > properties on them, too. So it's better to use a different function to
> > detect which backend a directory belongs to.
>
> In this case `vc-call` must use that function, right?

No, vc-call won't be used. Like in the patch I sent previously.

> > Also, your patch makes vc-registered work on directories.
>
> How is that?  'vc-registered' is still returns nil for directories.  The
> changes affects only the side effect of it.

Oh, now I finally understand what it's doing.

You can probably see how it's not ideal control flow (call a function, 
see it return nil, and then rely on its undocumented side-effect).

So if we can avoid doing that and still fix the bug, the alternative 
approach should be preferable.