bug#37189: 25.4.1: vc-hg-ignore implementation is missing

[Dmitry Gutov]

Hi Wolfgang,

Sorry for the wait.

On 05.01.2020 6:46, Wolfgang Scherer wrote:
> > On 29.08.2019 18:52, Wolfgang Scherer wrote:
> > > +  "Ignore FILE of DIRECTORY (default is `default-directory').
> >
> > IF this function needs a docstring at all (which is not obvious since it should 
> > be following vc-ignore), I think I'd rather this followed the latter's 
> > docstring. Where the clarification about the default is not in the first 
> > sentence.
>
> vc-hg-ignore needs a docstring, since it exhibits specific behavior for the 
> backend (e.g. glob/regex syntax), which is not and cannot be covered by the 
> rather generic vc-ignore docstring.

OK, I'm fine with that. But user-facing one is vc-ignore, so it can have 
some generic language to that effect (saying something like "wildcard or 
other syntax supported by the backend").

> Most of the description in  vc-ignore is not even applicable to the backend 
> specific functions, e.g. there is no interactive functionality in the backend 
> function and no backend is determined.

OK, true, but otherwise the description should be closer. And we also 
have the description of this backend command in the header commentary of 
vc.el.

> If a backend does not provide a vc-BACKEND-ignore function, vc-default-ignore 
> is invoked. This function has a docstring of its own (which contains the 
> clarification about the default in the first sentence). According to your 
> argument, this function should also have no docstring of its own. However, I 
> cannot see how both docstrings are equivalent.

Actually, I'd have more doubt in its necessity, yes.

This one really should be covered by the docstring of vc-ignore, as well 
as the description of the 'ignore' VC command in the header commentary. 
I mean, it can have a docstring, but it would be a plain copy of the 
existing descriptions elsewhere.

> vc-hg-ignore is modeled after vc-default-ignore, the docstring was copied from 
> vc-default-ignore and modified to fit the implementation. I have modified the 
> docstring further to match other backend specific ignore functions.

Actually, let's talk about vc-default-ignore.

In a recent patch you submitted in debbugs#37217 you changed its 
docstring to say "either relative to DIRECTORY or absolute" whereas it 
originally said "either relative to the root directory of DIRECTORY or 
absolute. That sounds like a change in documented behavior. And it could 
be a problem if it were in a docstring users are actually likely to see.

In any case, I think the docstrings should really be in accord, and the 
"more private" functions shouldn't have crucial information about 
command's behavior that the users won't be able to see.

> > Also, I think saying "Ignore FILE under DIRECTORY" would be better, if you 
> > intend to add this particular semantics to relative names.
> All other ignore functions say "Ignore FILE under VCS". I have modified the 
> docstring accordingly.
> > > +Otherwise, FILE is either relative to DIRECTORY or absolute. FILE
> > > +is converted to a path relative to the project root of DIRECTORY.
> >
> > Isn't it a bit odd that vc-ignore's docstring doesn't specify this distinction, 
> > and yet we're trying to implement it in vc-hg-ignore?
>
> No. vc-ignore's semantic roots stem from SCCS/RCS/CVS/subversion with single 
> directory ignore files. The ignore files for those VCS only contain file 
> patterns.

vc-ignore is still the command the user calls, isn't it? Or are you 
using some other command?

(This seems like the key question of this email at this point).

If yes, I also wonder what are the cases when the DIRECTORY argument is 
important. I.e., when is it not the same as default-directory?

vc-ignore, when called interactively, sets this argument to nil (so it 
is set to default-directory). vc-dir-ignore doesn't pass the second 
argument either.

Importantly, when do we really expect FILE to be something other than 
absolute, relative to the repository root, or a simple glob (e.g. *.c) 
which is going to apply to files regardless of the directory? BTW, it 
seems to me like f144c87f92b broke the last case. Or at least made it 
work worse (calling vc-ignore with '*.c' somewhere deep inside the tree 
would prepend its subdirectory to it).

There is some *valuable* code in this patch, but let's resolve this 
relative-names confusion first.

From what I see, when vc-ignore receives the FILE argument 
interactively, it's either absolute (as a product of read-file-name when 
the user choose a file with completion), or a free-form glob when the 
user just went ahead and wrote one anyway.

So instead of the current always-relativizing logic, I think we should 
choose what to do by calling file-name-absolute-p. And if it's not 
absolute, just take the value as-is, because there is no telling if the 
user meant "this file in the current dir" or "all files with that name". 
If it is absolute, on the other hand, yes, make it relative to the 
repository root.

> > > +                (concat pattern (and (file-directory-p file-path) "*"))))))
> >
> > I think it needs to asterisks for the glob to become recursive. At least 
> > according to https://stackoverflow.com/a/255094/615245.
> No, according to https://stackoverflow.com/a/255094/2127439, all of "bin/", "bin/*", 
> "bin/**" are equivalent under glob syntax. I also tested this specifically and extensivlely.

Very well.