bug#20637: incompatible, undocumented change to vc-working-revision

bug#20637: incompatible, undocumented change to vc-working-revision

[Glenn Morris]

Package: emacs
Version: 25.0.50

mkdir /tmp/git
cd /tmp/git
git init
touch 1

emacs-24.5:
  (vc-working-revision "1")  ;  -> nil

Current master:
  (vc-working-revision "1")  ;  "master"

C-h f vc-working-revision:
  If FILE is not registered, this function always returns nil.

This changes documented behaviour without updating the doc or making a
NEWS entry.

Example breakage:
http://lists.gnu.org/archive/html/help-gnu-emacs/2015-05/msg00384.html

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 05/24/2015 02:49 AM, Glenn Morris wrote:

> Current master:
>   (vc-working-revision "1")  ;  "master"

This has been caused by the commit 
7f9b037245ddb662ad98685e429a2498ae6b7c62, which made both vc-state and 
vc-working-revision use vc-responsible-backend instead of vc-backend.

As a result, in some backends these functions started return non-nil 
values for unknown files or directories, as long as they lie inside a VC 
repository.

This change is indeed backward-incompatible, and it breaks the previous 
assumption of some backend functions that if FILE has been passed to it, 
then it's surely registered with the current backend. That's why the 
commit included changes adding lines like (unless (memq (vc-state file) 
'(nil unregistered))...), but it didn't get all affected code.

In particular, it breaks an assumption I made when fixing #11757, that 
vc-git-state never receives an unregistered file as input. So if you 
evaluate (vc-state "1") now, it'll return `up-to-date'.

While reverting the change makes some tests fail, we should fix them in 
different ways.

For some backends, maybe, we should accept that (vc-state 
default-directory) and (vc-working-revision default-directory) will 
return nil. Alternatively, fix that problem inside the respective 
backends, without changing the dispatching functions.

Also, reverting this commit also seems to uncover tests that shouldn't 
pass anyway. Checks like

   (should (eq (vc-state default-directory)
               (vc-state default-directory backend)))

don't verify much, and in this case they seem to verify the wrong thing. 
More on that in the respective threads in emacs-devel later.

Michael, thoughts?

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry

> Also, reverting this commit also seems to uncover tests that shouldn't
> pass anyway. Checks like
>
>   (should (eq (vc-state default-directory)
>               (vc-state default-directory backend)))
>
> don't verify much, and in this case they seem to verify the wrong
> thing. More on that in the respective threads in emacs-devel later.
>
> Michael, thoughts?

Why is verifying such tests "the wrong thing"? It's a while ago that I
wrote the tests, but IIRC I've added them exactly because I did expect
that such tests should pass, and they didn't. I even fixed some trivial
corner cases when writing the tests. as far as I understood the code.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 03/29/2016 09:13 PM, Michael Albinus wrote:

> Why is verifying such tests "the wrong thing"? It's a while ago that I
> wrote the tests, but IIRC I've added them exactly because I did expect
> that such tests should pass, and they didn't.

It's certainly not very meaningful to test this (it's better to compare 
to actual values; for all we know, the above method returns `fooled-ya' 
in both cases).

As far as it being wrong: it is, if you consider that some existing 
implementations don't expect to be called with FILE that's not 
registered. So different return values in these two cases are to be 
expected.

> I even fixed some trivial
> corner cases when writing the tests. as far as I understood the code.

Yes, you found some of those cases (but, like mentioned, not all), and 
that required double-checking that the file is indeed registered.

You can argue that the new semantics are more straightforward, and I 
don't disagree (the docstring of vc-state seems to agree already; 
vc-working-revision's docstring disagrees).

But the cost to that is extra process calls. I'm not sure if the changes 
in 7f9b037245ddb662ad98685e429a2498ae6b7c62 add any extra process calls, 
but they do add some interaction with the filesystem.

Fixing the newly introduced problem with vc-git-state would require an 
extra process call, more or less reverting the fix for bug#11757. I 
don't know how much of a problem that is (I haven't used Windows in a 
while, and my current laptop is faster that what I had back then 
anyway), but it would certainly be nice not to introduce a regression in 
features, or performance.

As far as vc-git-state, one way to do that is reimplementing some 
commands using 'git status --porcelain', introduced in Git 1.7.0. We 
should double-check if we're allowed to rely on this version being 
available (which Git does the the oldest relevant version of CentOS 
install now?), and it might be too late for Emacs 25.1 anyway.

Calling vc-responsible-backend is also inherently slower than 
vc-backend, though not perceptibly so on this localhost (4e-5s vs 
4e-6s). But it's likely more painful for remove hosts; how is it, in 
your experience?

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

you have written several things I would like to move for later
discussion. I believe, we shall start again from the basics.

I have extended vc-test-*01-register tests by calls to vc-backend and
vc-responsible-backend. Mainly in order to understand how they work, but
also for covering these functions. One problem I've found is that
vc-file-*prop functions do not work well for relative file names; I've
fixed this. With this fix, vc-test-src02-state failed. I've masked the
test for future investigations; first I would like to make all
vc-test-*01-register tests run proper.

Several problems I have marked with FIXME in the working horse of those
tests, vc-test--register:

- For some backends (CVS, RCS and SVN), vc-backend returns the backend
  name for the newly created repo directory, and the directory is
  registered already. For the other backends, vc-backend returns nil as
  expected. Shouldn't this be consistent for all backends?

- vc-backend accepts also a list of files, vc-responsible-backend
  doesn't. Is this right?

- There is no common function vc-unregister, just some backend specific
  vc-<backend>-unregister. Shouldn't vc-unregister exist? It should call
  common code, like vc-file-clearprops. For the time being, I have
  emulated this.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

Hi Michael,

On 04/09/2016 10:34 PM, Michael Albinus wrote:

> you have written several things I would like to move for later
> discussion. I believe, we shall start again from the basics.

OK, but the questions seem tangential to this bug report, which is a 
blocker for 25.1 (whereas investigating how various commands should 
work, isn't).

> I have extended vc-test-*01-register tests by calls to vc-backend and
> vc-responsible-backend. Mainly in order to understand how they work, but
> also for covering these functions. One problem I've found is that
> vc-file-*prop functions do not work well for relative file names; I've
> fixed this.

The change looks good, but nevertheless seeing the commit 5e1c32e in 
master makes me worried about conflicts when merging the necessary 
future fix for this bug from emacs-25 to master.

> Several problems I have marked with FIXME in the working horse of those
> tests, vc-test--register:
>
> - For some backends (CVS, RCS and SVN), vc-backend returns the backend
>   name for the newly created repo directory, and the directory is
>   registered already. For the other backends, vc-backend returns nil as
>   expected. Shouldn't this be consistent for all backends?

I'm not quite clear on what you are saying here. If you're calling 
vc-backend on a directory, I believe the result is undefined. As in, the 
function is allowed to return any value. Maybe we could check 
file-directory-p in vc-backend, and signal an error if it is.

For directories, one has to call vc-responsible-backend.

> - vc-backend accepts also a list of files, vc-responsible-backend
>   doesn't. Is this right?

I suppose. The function signatures say so. But I don't see any callers 
of vc-backend that actually pass a list to it.

> - There is no common function vc-unregister, just some backend specific
>   vc-<backend>-unregister.

Those are the implementations of the `unregister' backend command. It's 
only used in vc-transfer-file currently.

> Shouldn't vc-unregister exist?

Maybe it should. Would you ever use it interactively?

> It should call
>   common code, like vc-file-clearprops. For the time being, I have
>   emulated this.

Are you doing that just to test the `unregister' implementations?

Because otherwise, to clean up after a test, you can simply delete the 
directory with the test repository.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

>> you have written several things I would like to move for later
>> discussion. I believe, we shall start again from the basics.
>
> OK, but the questions seem tangential to this bug report, which is a
> blocker for 25.1 (whereas investigating how various commands should
> work, isn't).

If there is a simple and obvious fix for this problem, let apply it to
the emacs-25 branch. Even if we must change it later in master.

But I doubt that's possible. This bug report was written on 23 May 2015,
it was marked as release blocker on the same day, and still no fix. I
believe that there won't be a fix in emacs-25, and whatever we might
find on our way to stabilize vc, it will be too late and too much to be
applied to the emacs-25 branch.

For all those problems in vc I have the impression, that we must
stabilize vc first. Starting with the very basic functionality - that's
why I've added tests for vc-backend and vc-responsible-backend. Often,
there might be not an error, but missing or wrong documentation. As
Glenn has said also in bug#19548, another release blocker for 25.1.

I don't care whether we discuss it here, in bug#19548 or in the
emacs-devel ML. But at least *I* am not able to handle those issues
differently. And nobody but you and me seem to work on vc problems in
general.

>> I have extended vc-test-*01-register tests by calls to vc-backend and
>> vc-responsible-backend. Mainly in order to understand how they work, but
>> also for covering these functions. One problem I've found is that
>> vc-file-*prop functions do not work well for relative file names; I've
>> fixed this.
>
> The change looks good, but nevertheless seeing the commit 5e1c32e in
> master makes me worried about conflicts when merging the necessary
> future fix for this bug from emacs-25 to master.

As said, I doubt that we will find something which could go into
emacs-25. Of course I would be happy if I'm wrong :-)

>> Several problems I have marked with FIXME in the working horse of those
>> tests, vc-test--register:
>>
>> - For some backends (CVS, RCS and SVN), vc-backend returns the backend
>>   name for the newly created repo directory, and the directory is
>>   registered already. For the other backends, vc-backend returns nil as
>>   expected. Shouldn't this be consistent for all backends?
>
> I'm not quite clear on what you are saying here. If you're calling
> vc-backend on a directory, I believe the result is undefined. As in,
> the function is allowed to return any value. Maybe we could check
> file-directory-p in vc-backend, and signal an error if it is.

The docstring says nothing about directories, so the call I've applied
is legal. If vc-backend is not intended for directories we should
document it first. But ...

> For directories, one has to call vc-responsible-backend.

... I even doubt that directories are out of scope of vc-backend.
Directories can be registered for some VCS, for example for CVS, or for
ClearCase which I use at work (I know, we have no official support for
ClearCase in vc). Therefore we shall precise the docstring of
vc-backend, that it returns the backend also for directories for VCSes
which support rgistration of directories.

A pointer to vc-responsible-backend might also be helpful in the
docstring of vc-backend.

>> - vc-backend accepts also a list of files, vc-responsible-backend
>>   doesn't. Is this right?
>
> I suppose. The function signatures say so. But I don't see any callers
> of vc-backend that actually pass a list to it.

I know what the docstring says :-) My question is, whether we shall
offer the same argument list for both vc-backend and vc-responsible-backend.

>> - There is no common function vc-unregister, just some backend specific
>>   vc-<backend>-unregister.
>
> Those are the implementations of the `unregister' backend
> command. It's only used in vc-transfer-file currently.
>
>> Shouldn't vc-unregister exist?
>
> Maybe it should. Would you ever use it interactively?

Don't know. But I believe "interactively" is not the criterion whether a
general vc function shall exist. I also don't call vc-registered interactively.

>> It should call
>>   common code, like vc-file-clearprops. For the time being, I have
>>   emulated this.
>
> Are you doing that just to test the `unregister' implementations?

Yes. And I'm still in favor of adding vc-unregister in vc.el.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/10/2016 11:00 AM, Michael Albinus wrote:

> If there is a simple and obvious fix for this problem, let apply it to
> the emacs-25 branch. Even if we must change it later in master.

Why not revert 7f9b037245ddb662ad98685e429a2498ae6b7c62? I believe I've 
mentioned that suggestion before.

The only difficulty here, as far as I'm concerned, is to update the 
corresponding tests so they don't break, but are not disabled, and still 
look somewhat reasonable. That's where the merge conflict concerns come 
into play. But "no disabled tests" is not a hard requirement for release 
anyway.

> But I doubt that's possible. This bug report was written on 23 May 2015,
> it was marked as release blocker on the same day, and still no fix.

Not because it's too difficult to resolve.

> For all those problems in vc I have the impression, that we must
> stabilize vc first. Starting with the very basic functionality - that's
> why I've added tests for vc-backend and vc-responsible-backend.

I agree with the sentiment, but not with certain choices of the areas to 
"stabilize" it in. You've basically been discovering aspects of the 
current commands and functions that are poorly specified. But those 
aspects (with some exceptions, I suppose) are not regressions from Emacs 
24. They have been there for a while.

> I don't care whether we discuss it here, in bug#19548 or in the
> emacs-devel ML.

I don't mind too much any way, but 19548 is for the manual and such. A 
separate bug report (or several) or discussions on emacs-devel seem 
preferable. Unless I'm mistaken about these problems being orthogonal, 
of course.

> The docstring says nothing about directories, so the call I've applied
> is legal.

Best wishes to whoever wrote that docstring.

> ... I even doubt that directories are out of scope of vc-backend.
> Directories can be registered for some VCS, for example for CVS, or for
> ClearCase which I use at work (I know, we have no official support for
> ClearCase in vc).

In general, VC supports the lowest common denominator across the 
backends. Or at least, a feature should be supported in a few important 
ones. CVS is on its way out, and ClearCase is a relatively niche tool.

Anyway, the point is VC is for people to be able to write code depending 
on the public API and see it work across many VCSes. And Git and 
Mercurial, at least, don't track directories.

> Therefore we shall precise the docstring of
> vc-backend, that it returns the backend also for directories for VCSes
> which support rgistration of directories.

Why? Just tell anyone who wants to know a directory's backend to use 
vc-responsible-backend instead.

> I know what the docstring says :-) My question is, whether we shall
> offer the same argument list for both vc-backend and vc-responsible-backend.

We could, but honestly, that question doesn't sound particularly 
important right now. Yes, it's a wart, and if there are no users that 
pass a list to it, we can remove this possibility.

Note that vc-backend and vc-responsible-backend have different 
performance characteristics (and, I imagine, different behavior in case 
of nested repositories), so simply replacing all uses of the former with 
the latter is not a good idea.

> Don't know. But I believe "interactively" is not the criterion whether a
> general vc function shall exist. I also don't call vc-registered interactively.

Interactive use would be a strong justification. vc-register can be used 
interactively, and it's called from vc-next-action.

How will we use vc-unregister?

>>> It should call
>>>   common code, like vc-file-clearprops. For the time being, I have
>>>   emulated this.
>>
>> Are you doing that just to test the `unregister' implementations?
>
> Yes.

That seems very low priority. I wonder how often vc-transfer-file gets 
used in practice these days.

> And I'm still in favor of adding vc-unregister in vc.el.

I don't really mind.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 04/10/2016 11:00 AM, Michael Albinus wrote:
>
>> If there is a simple and obvious fix for this problem, let apply it to
>> the emacs-25 branch. Even if we must change it later in master.
>
> Why not revert 7f9b037245ddb662ad98685e429a2498ae6b7c62? I believe
> I've mentioned that suggestion before.

Why that? That patch fixes problems. How would you solve them otherwise?

And what problem has been introduced with that patch? This one we are
speaking about, bug#20637? Maybe you have mentioned this already, and
I've overseen this.

> The only difficulty here, as far as I'm concerned, is to update the
> corresponding tests so they don't break, but are not disabled, and
> still look somewhat reasonable. That's where the merge conflict
> concerns come into play. But "no disabled tests" is not a hard
> requirement for release anyway.

Could you show a patch which reverts
7f9b037245ddb662ad98685e429a2498ae6b7c62, and also updates the tests?
Maybe it is simpler to speak about this concrete proposal.

>> But I doubt that's possible. This bug report was written on 23 May 2015,
>> it was marked as release blocker on the same day, and still no fix.
>
> Not because it's too difficult to resolve.

But nobody has taken any action for more than 10 months :-(

>> For all those problems in vc I have the impression, that we must
>> stabilize vc first. Starting with the very basic functionality - that's
>> why I've added tests for vc-backend and vc-responsible-backend.
>
> I agree with the sentiment, but not with certain choices of the areas
> to "stabilize" it in. You've basically been discovering aspects of the
> current commands and functions that are poorly specified. But those
> aspects (with some exceptions, I suppose) are not regressions from
> Emacs 24. They have been there for a while.

Oh, there are regressions. ESR has undertaken his rewrite of vc*.el end
of 2014. Emacs 25.1 is the first release which brings them to the
public.

See also vc.el, and compare the functions listed in the Commentary
section between Emacs 24.5 and 25.1. Read also "Changes from the
pre-25.1 API" at the same place in the 25.1 version of vc.el.

Unfortunately, etc/NEWS of Emacs 25.1 is very silent about those
changes.

>> I don't care whether we discuss it here, in bug#19548 or in the
>> emacs-devel ML.
>
> I don't mind too much any way, but 19548 is for the manual and such. A
> separate bug report (or several) or discussions on emacs-devel seem
> preferable. Unless I'm mistaken about these problems being orthogonal,
> of course.

I might be wrong, but Glenn didn't write this bug only because of some
missing docs. I believe he was unhappy about the whole process, how
those changes did arrive. But I'm not Glenn, and I shouldn't interpret
too much ...

>> ... I even doubt that directories are out of scope of vc-backend.
>> Directories can be registered for some VCS, for example for CVS, or for
>> ClearCase which I use at work (I know, we have no official support for
>> ClearCase in vc).
>
> In general, VC supports the lowest common denominator across the
> backends. Or at least, a feature should be supported in a few
> important ones. CVS is on its way out, and ClearCase is a relatively
> niche tool.

You have read my comment in vc-tests.el? At least CVS, RCS and SVN seem
to behave this way. So it is reasonable to handle the case, that
directories are registered.

> Anyway, the point is VC is for people to be able to write code
> depending on the public API and see it work across many VCSes. And Git
> and Mercurial, at least, don't track directories.

So what? We could say in the doc that registering directories is not
supported for all backends. People who write a backend know what they
do. I hope.

>> Therefore we shall precise the docstring of
>> vc-backend, that it returns the backend also for directories for VCSes
>> which support rgistration of directories.
>
> Why? Just tell anyone who wants to know a directory's backend to use
> vc-responsible-backend instead.

I have no problem to recommend vc-responsible-backend. But vc-backend
does already TRT: when a directory is registered, it returns the backend
name. When a directory is not registered, it returns nil. This is *not*
a statement why a directory is not registered, and whether registering
directories is possible at all for a backend.

>> I know what the docstring says :-) My question is, whether we shall
>> offer the same argument list for both vc-backend and vc-responsible-backend.
>
> We could, but honestly, that question doesn't sound particularly
> important right now. Yes, it's a wart, and if there are no users that
> pass a list to it, we can remove this possibility.
>
> Note that vc-backend and vc-responsible-backend have different
> performance characteristics (and, I imagine, different behavior in
> case of nested repositories), so simply replacing all uses of the
> former with the latter is not a good idea.

I haven't proposed this! I simply want to understand (and document) what
both functions are doing. Which of them is taken by a backend is the
decision of the maintainer of the backend.

I'm pretty much in favor to also document the differences in performance. 

>> Don't know. But I believe "interactively" is not the criterion whether a
>> general vc function shall exist. I also don't call vc-registered
>> interactively.
>
> Interactive use would be a strong justification. vc-register can be
> used interactively, and it's called from vc-next-action.

I haven't spoken about vc-register. I have spoken about
vc-registered. This one exist as general function, and it isn't interactive.

> How will we use vc-unregister?

Every backend can use it instead of its backend specific function, like
vc-git-unregister. It runs common code then like removing properties,
and calls the backend specific function then.

>>>> It should call
>>>>   common code, like vc-file-clearprops. For the time being, I have
>>>>   emulated this.
>>>
>>> Are you doing that just to test the `unregister' implementations?
>>
>> Yes.
>
> That seems very low priority. I wonder how often vc-transfer-file gets
> used in practice these days.

I understand that's low priority for you, and I accept this. For me,
while extending vc-tests.el, it isn't low priority. I need solid ground
under my feed, and therefore I'm starting with the very basic
functions. Understanding, testing, maybe fixing bugs, maybe improving
documentation. Function by function.

>> And I'm still in favor of adding vc-unregister in vc.el.
>
> I don't really mind.

So I will do, unless somebody stops me :-)

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/10/2016 09:09 PM, Michael Albinus wrote:

>> Why not revert 7f9b037245ddb662ad98685e429a2498ae6b7c62? I believe
>> I've mentioned that suggestion before.
>
> Why that? That patch fixes problems. How would you solve them otherwise?

It just fixed the tests you've introduced a little while before that. 
I'm not aware of any related bug reports.

The new tests assumed new semantics, so you fixed them by changing 
semantics, thereby causing the "incompatible, undocumented change".

> And what problem has been introduced with that patch? This one we are
> speaking about, bug#20637?

Yes.

> Maybe you have mentioned this already, and
> I've overseen this.

I wrote about that in the very first message to this thread: 
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20637#10

> Could you show a patch which reverts
> 7f9b037245ddb662ad98685e429a2498ae6b7c62, and also updates the tests?
> Maybe it is simpler to speak about this concrete proposal.

Unless you have a different proposal, I'll write and simply commit that 
patch.

>>> But I doubt that's possible. This bug report was written on 23 May 2015,
>>> it was marked as release blocker on the same day, and still no fix.
>>
>> Not because it's too difficult to resolve.
>
> But nobody has taken any action for more than 10 months :-(

I would expect that to be the responsibility of the person who caused 
the breakage.

> Oh, there are regressions. ESR has undertaken his rewrite of vc*.el end
> of 2014. Emacs 25.1 is the first release which brings them to the
> public.

There are regressions, but the questions you've been asking don't seem 
to number among them.

> Unfortunately, etc/NEWS of Emacs 25.1 is very silent about those
> changes.

That's bug bug#19548. Could we keep that discussion out of this one?

> I might be wrong, but Glenn didn't write this bug only because of some
> missing docs. I believe he was unhappy about the whole process, how
> those changes did arrive. But I'm not Glenn, and I shouldn't interpret
> too much ...

This bug is about vc-working-revision. It's in the title.

>> In general, VC supports the lowest common denominator across the
>> backends. Or at least, a feature should be supported in a few
>> important ones. CVS is on its way out, and ClearCase is a relatively
>> niche tool.
>
> You have read my comment in vc-tests.el?

Which comment are you referring to?

> At least CVS, RCS and SVN seem
> to behave this way.

You've mentioned that in the previous message. Out of these three, only 
SVN is in any way recommended for production use these days. In any 
case, they're aging and only decreasing in popularity.

> So it is reasonable to handle the case, that
> directories are registered.

I do not understand this argument.

>> Anyway, the point is VC is for people to be able to write code
>> depending on the public API and see it work across many VCSes. And Git
>> and Mercurial, at least, don't track directories.
>
> So what?

"So what" doesn't seem like a meaningful response to the first sentence 
above.

> We could say in the doc that registering directories is not
> supported for all backends. People who write a backend know what they
> do. I hope.

We could say a lot of things. We could even say that every backend 
behaves in its own special way (and enumerate all those ways), but where 
will that get us?

Again, what's the benefit of introducing this complication?

> But vc-backend
> does already TRT: when a directory is registered, it returns the backend
> name. When a directory is not registered, it returns nil.

It just does the thing that's more convenient for each backend. That 
doesn't make it TRT.

> This is *not*
> a statement why a directory is not registered, and whether registering
> directories is possible at all for a backend.

Indeed, and that's a problem: if the caller receives nil, it cannot know 
whether that's because the directory is not a part of a VCS checkout, or 
because the backend does not support tracking directories.

This is a reason not to support calling vc-backend on directories: it 
leads to unportable code.

>> Note that vc-backend and vc-responsible-backend have different
>> performance characteristics (and, I imagine, different behavior in
>> case of nested repositories), so simply replacing all uses of the
>> former with the latter is not a good idea.
>
> I haven't proposed this!

Good. I've just made a guess on why you've asked the question, and 
7f9b037245ddb662ad98685e429a2498ae6b7c62 hinted at that conclusion.

 > I simply want to understand (and document) what
 > both functions are doing.

OK. It's not like I'm a big hoard of knowledge about them. To answer the 
questions, I've just looked at their definitions and usages.

 > Which of them is taken by a backend is the
 > decision of the maintainer of the backend.

I don't understand this. What kind of decisions would backend 
maintainers make about them?

> I'm pretty much in favor to also document the differences in performance.

OK.

> I haven't spoken about vc-register. I have spoken about
> vc-registered. This one exist as general function, and it isn't interactive.

I see. But vc-registered is a relatively meaty function, and it has two 
callers outside of tests. That looks better justified to me.

>> How will we use vc-unregister?
>
> Every backend can use it instead of its backend specific function, like
> vc-git-unregister. It runs common code then like removing properties,
> and calls the backend specific function then.

vc-git-unregister has no internal callers. Anyway, please go ahead with 
`vc-unregister' is you feel so strongly about it.

> I understand that's low priority for you, and I accept this. For me,
> while extending vc-tests.el, it isn't low priority. I need solid ground
> under my feed, and therefore I'm starting with the very basic
> functions.

My point was, the `unregister' command is a fringe one, not a basic one.

> Understanding, testing, maybe fixing bugs, maybe improving
> documentation. Function by function.

Sure. But I think we should fix the current bug first.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

>> But nobody has taken any action for more than 10 months :-(
>
> I would expect that to be the responsibility of the person who caused
> the breakage.

Of course!

Until now I even didn't realize this was broken by my patch. Sorry, but
I have been in hospital last year, and when I recovered there were too
many messages to scan.

I'll check what could be done. I'll return later.

Best regards, Michael.

PS: the discussion about my other changes in vc-tests.el shall be moved
to the emacs-devel ML. Let's restart it there, when this problem is solved.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 2201 bytes --]

Dmitry Gutov <dgutov@yandex.ru> writes:

> This has been caused by the commit
> 7f9b037245ddb662ad98685e429a2498ae6b7c62, which made both vc-state and
> vc-working-revision use vc-responsible-backend instead of vc-backend.

Yes.

> As a result, in some backends these functions started return non-nil
> values for unknown files or directories, as long as they lie inside a
> VC repository.

vc-working-revision shall return nil for unregistered files. vc-state
shall return a non-nil value, 'unregistered.

> This change is indeed backward-incompatible, and it breaks the
> previous assumption of some backend functions that if FILE has been
> passed to it, then it's surely registered with the current
> backend.

You cannot guarantee this. Anybody is free to call the functions with
unregistered files. And in the vc-state case, it is even documented that
this could happen.

> In particular, it breaks an assumption I made when fixing #11757, that
> vc-git-state never receives an unregistered file as input. So if you
> evaluate (vc-state "1") now, it'll return `up-to-date'.

This assumption could be kept if vc-state filters such unregistered
files out.

> While reverting the change makes some tests fail, we should fix them
> in different ways.
>
> For some backends, maybe, we should accept that (vc-state
> default-directory) and (vc-working-revision default-directory) will
> return nil. Alternatively, fix that problem inside the respective
> backends, without changing the dispatching functions.
>
> Also, reverting this commit also seems to uncover tests that shouldn't
> pass anyway. Checks like
>
>   (should (eq (vc-state default-directory)
>               (vc-state default-directory backend)))
>
> don't verify much, and in this case they seem to verify the wrong
> thing. More on that in the respective threads in emacs-devel later.
>
> Michael, thoughts?

I've prepared a patch which just covers the case that a file is
unregistered, in both vc-state and vc-working-revision. It is a very
small change, that I believe it could still go into the emacs-25 branch.

Patch towards emacs-25 branch is appended, including modification of
vc-tests.el. Comments?

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: diff --]
[-- Type: text/x-patch, Size: 7217 bytes --]

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 0826744..dc7cc61 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -475,10 +475,11 @@ vc-state
   ;; FIXME: New (sub)states needed (?):
   ;; - `copied' and `moved' (might be handled by `removed' and `added')
   (or (vc-file-getprop file 'vc-state)
+      (and (not (vc-registered file)) 'unregistered)
       (when (> (length file) 0)         ;Why??  --Stef
 	(setq backend (or backend (vc-responsible-backend file)))
 	(when backend
-          (vc-state-refresh file backend)))))
+	  (vc-state-refresh file backend)))))

 (defun vc-state-refresh (file backend)
   "Quickly recompute the `state' of FILE."
@@ -494,11 +495,12 @@ vc-working-revision
   "Return the repository version from which FILE was checked out.
 If FILE is not registered, this function always returns nil."
   (or (vc-file-getprop file 'vc-working-revision)
-      (progn
-	(setq backend (or backend (vc-responsible-backend file)))
-	(when backend
-	  (vc-file-setprop file 'vc-working-revision
-			   (vc-call-backend backend 'working-revision file))))))
+      (and (vc-registered file)
+	   (progn
+	     (setq backend (or backend (vc-responsible-backend file)))
+	     (when backend
+	       (vc-file-setprop file 'vc-working-revision
+				(vc-call-backend backend 'working-revision file)))))))

 ;; Backward compatibility.
 (define-obsolete-function-alias
diff --git a/test/automated/vc-tests.el b/test/automated/vc-tests.el
index 2faa143..7f6196b 100644
--- a/test/automated/vc-tests.el
+++ b/test/automated/vc-tests.el
@@ -285,10 +285,9 @@ vc-test--state
 	  (make-directory default-directory)
 	  (vc-test--create-repo-function backend)

-	  ;; nil: Hg Mtn RCS
-          ;; added: Git
-          ;; unregistered: CVS SCCS SRC
-	  ;; up-to-date: Bzr SVN
+	  ;; nil: RCS
+          ;; unregistered: Bzr CVS Git Hg Mtn SCCS SRC
+	  ;; up-to-date: SVN
           (message "vc-state1 %s" (vc-state default-directory))
 	  (should (eq (vc-state default-directory)
 		      (vc-state default-directory backend)))
@@ -298,51 +297,43 @@ vc-test--state
 	  (let ((tmp-name (expand-file-name "foo" default-directory)))
 	    ;; Check state of an empty file.

-	    ;; nil: Hg Mtn SRC SVN
-            ;; added: Git
-	    ;; unregistered: RCS SCCS
-	    ;; up-to-date: Bzr CVS
+	    ;; unregistered: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-state2 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+	    (should (eq (vc-state tmp-name) 'unregistered))

 	    ;; Write a new file.  Check state.
 	    (write-region "foo" nil tmp-name nil 'nomessage)

-            ;; nil: Mtn
-            ;; added: Git
-            ;; unregistered: Hg RCS SCCS SRC SVN
-            ;; up-to-date: Bzr CVS
+            ;; unregistered: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-state3 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+	    (should (eq (vc-state tmp-name) 'unregistered))

 	    ;; Register a file.  Check state.
 	    (vc-register
 	     (list backend (list (file-name-nondirectory tmp-name))))

-            ;; added: Git Mtn
-            ;; unregistered: Hg RCS SCCS SRC SVN
-            ;; up-to-date: Bzr CVS
+	    ;; nil: SRC
+            ;; added: Bzr CVS Git Hg Mtn
+            ;; unregistered: SVN
+	    ;; up-to-date: RCS SCCS
             (message "vc-state4 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name) '(added unregistered up-to-date)))
+	    (should (memq (vc-state tmp-name)
+			  '(nil added unregistered up-to-date)))

 	    ;; Unregister the file.  Check state.
 	    (condition-case nil
 		(progn
 		  (vc-test--unregister-function backend tmp-name)

-		  ;; added: Git
-		  ;; unregistered: Hg RCS
+		  ;; added: Bzr Git Hg
 		  ;; unsupported: CVS Mtn SCCS SRC SVN
-		  ;; up-to-date: Bzr
+		  ;; up-to-date: RCS
                   (message "vc-state5 %s" (vc-state tmp-name))
 		  (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-		  (should (memq (vc-state tmp-name)
-				'(added unregistered up-to-date))))
+		  (should (memq (vc-state tmp-name) '(added up-to-date))))
 	      (vc-not-supported (message "vc-state5 unsupported")))))

       ;; Save exit.
@@ -370,8 +361,8 @@ vc-test--working-revision
 	  (make-directory default-directory)
 	  (vc-test--create-repo-function backend)

-	  ;; nil: CVS Git Mtn RCS SCCS
-	  ;; "0": Bzr Hg SRC SVN
+	  ;; nil: Bzr CVS Git Hg Mtn RCS SCCS SRC
+	  ;; "0": SVN
           (message
            "vc-working-revision1 %s" (vc-working-revision default-directory))
 	  (should (eq (vc-working-revision default-directory)
@@ -382,33 +373,32 @@ vc-test--working-revision
 	    ;; Check initial working revision, should be nil until
             ;; it's registered.

-	    ;; nil: CVS Git Mtn RCS SCCS SVN
-	    ;; "0": Bzr Hg SRC
+	    ;; nil: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-working-revision2 %s" (vc-working-revision tmp-name))
 	    (should (eq (vc-working-revision tmp-name)
 			(vc-working-revision tmp-name backend)))
-	    (should (member (vc-working-revision tmp-name) '(nil "0")))
+	    (should-not (vc-working-revision tmp-name))

 	    ;; Write a new file.  Check working revision.
 	    (write-region "foo" nil tmp-name nil 'nomessage)

-	    ;; nil: CVS Git Mtn RCS SCCS SVN
-	    ;; "0": Bzr Hg SRC
+	    ;; nil: Bzr CVS Git Hg Mtn RCS SCCS SRC SVN
             (message "vc-working-revision3 %s" (vc-working-revision tmp-name))
 	    (should (eq (vc-working-revision tmp-name)
 			(vc-working-revision tmp-name backend)))
-	    (should (member (vc-working-revision tmp-name) '(nil "0")))
+	    (should-not (vc-working-revision tmp-name))

 	    ;; Register a file.  Check working revision.
 	    (vc-register
 	     (list backend (list (file-name-nondirectory tmp-name))))

-	    ;; nil: Mtn Git RCS SCCS
+	    ;; nil: Git Mtn
 	    ;; "0": Bzr CVS Hg SRC SVN
+	    ;; "1.1": RCS SCCS
             (message "vc-working-revision4 %s" (vc-working-revision tmp-name))
 	    (should (eq (vc-working-revision tmp-name)
 			(vc-working-revision tmp-name backend)))
-	    (should (member (vc-working-revision tmp-name) '(nil "0")))
+	    (should (member (vc-working-revision tmp-name) '(nil "0" "1.1")))

 	    ;; Unregister the file.  Check working revision.
 	    (condition-case nil
@@ -417,12 +407,13 @@ vc-test--working-revision

 		  ;; nil: Git RCS
 		  ;; "0": Bzr Hg
+		  ;; "1.1": RCS
 		  ;; unsupported: CVS Mtn SCCS SRC SVN
                   (message
                    "vc-working-revision5 %s" (vc-working-revision tmp-name))
 		  (should (eq (vc-working-revision tmp-name)
 			      (vc-working-revision tmp-name backend)))
-		  (should (member (vc-working-revision tmp-name) '(nil "0"))))
+		  (should (member (vc-working-revision tmp-name) '(nil "0" "1.1"))))
 	      (vc-not-supported (message "vc-working-revision5 unsupported")))))

       ;; Save exit.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

Hey Michael,

On 04/13/2016 06:14 PM, Michael Albinus wrote:

> vc-working-revision shall return nil for unregistered files. vc-state
> shall return a non-nil value, 'unregistered.

That sounds fine to me in principle, but I don't think we can get there 
for Emacs 25.1, without paying with lower performance (*). The current 
code is slower than what's in 24.5 already, as a result of the 
aforementioned revision.

> You cannot guarantee this. Anybody is free to call the functions with
> unregistered files.And in the vc-state case, it is even documented that
> this could happen.

Both true. However, that vc-state's behavior has been different from its 
documentation in this regard, for many releases.

>> In particular, it breaks an assumption I made when fixing #11757, that
>> vc-git-state never receives an unregistered file as input. So if you
>> evaluate (vc-state "1") now, it'll return `up-to-date'.
>
> This assumption could be kept if vc-state filters such unregistered
> files out.

Problem: vc-registered is slower than vc-backend. Like, orders of 
magnitude slower. vc-backend caches the result of the previous 
vc-registered invocation in vc-file-prop-obarray. But if we call 
vc-registered directly, we go the whole way each time, including calling 
vc-BACKEND-registered.

Yes, both vc-state and vc-working-revision cache their results in a 
property, so we're only paying the added overhead when the file is 
opened, reverted, etc, but it's still a price. Don't you think it is a 
problem?

> I've prepared a patch which just covers the case that a file is
> unregistered, in both vc-state and vc-working-revision. It is a very
> small change, that I believe it could still go into the emacs-25 branch.

Aside from the above, is there a reason to keep using 
vc-responsible-backend instead of vc-backend, in vc-state and 
vc-working-revision? It's also slower than vc-backend.

(*) If you _really_ think that vc-state should ever return 
`unregistered' (personally, I've never found that distinction useful; we 
could just as well update the docstring that it returns nil in that 
case), I think the way to get that is to make vc-git-registered return 
non-nil for all files inside Git-controlled repositories, and to make 
vc-git-state return `unregistered' for unregistered files (and same for 
other backends). But it would be a bigger change, better suitable for 
the next release.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/11/2016 09:55 AM, Michael Albinus wrote:

>>> But nobody has taken any action for more than 10 months :-(
>>
>> I would expect that to be the responsibility of the person who caused
>> the breakage.
>
> Of course!
>
> Until now I even didn't realize this was broken by my patch. Sorry, but
> I have been in hospital last year, and when I recovered there were too
> many messages to scan.

To be clear, I don't mean to blame you for the time that has passed. But 
you were probably the best person to notice this regression, and 
understand why it happened, early enough.

Personally, I've looked at this bug report before, and could only tell 
that the documentation needed updating (but the changed behavior is 
fine). That's not my impression anymore.

Thanks.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

>> Until now I even didn't realize this was broken by my patch. Sorry, but
>> I have been in hospital last year, and when I recovered there were too
>> many messages to scan.
>
> To be clear, I don't mean to blame you for the time that has
> passed. But you were probably the best person to notice this
> regression, and understand why it happened, early enough.

No problem, I didn't felt being blamed. It was just an unfortune
coincidence, that this bug wasn't handled earlier.

And I still feel unhappy that none of the active Emacs maintainers feels
responsible for vc*.el. I'm trying to support fixing problems here, but
most of the code, and the VCSes being used, are still terra incognita for me.

> Thanks.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hey Michael,

Hi Dmitry,

> Problem: vc-registered is slower than vc-backend. Like, orders of
> magnitude slower. vc-backend caches the result of the previous
> vc-registered invocation in vc-file-prop-obarray. But if we call
> vc-registered directly, we go the whole way each time, including
> calling vc-BACKEND-registered.

I haven't thought too much about performance. But you are right, we
shouldn't add serious performance penalties to the code. And improving
performance for the 25.1 release is much too late.

So we might revert the patch for vc-state and vc-working-revision indeed
for the emacs-25 branch, going back to using vc-backend.

In the master branch we might apply my proposed patch using
vc-registered or something similar, and start to improve performance. In
parallel, we shall start to write a VCS section for the elisp manual,
describing vc-* functionality in more detail. We could start with
vc-backend and vc-responsible-backend and their intended use. I'm
missing such documentation for years.

I'll come back later today with the patch for emacs-25, if you agree.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/14/2016 10:10 AM, Michael Albinus wrote:

> And I still feel unhappy that none of the active Emacs maintainers feels
> responsible for vc*.el.

I do feel responsible, and I try to take care of most bug reports in 
that area.

> I'm trying to support fixing problems here, but
> most of the code, and the VCSes being used, are still terra incognita for me.

It would be great if the problems we're fixing were formulated like "VC 
behaves like this, and it causes that problem in practice (or could 
cause, at least)", rather than "here is a test case, and I feel it 
should pass". I think that would encourage more practical choices.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/14/2016 10:21 AM, Michael Albinus wrote:

> I haven't thought too much about performance. But you are right, we
> shouldn't add serious performance penalties to the code. And improving
> performance for the 25.1 release is much too late.

It's hard for me to judge how serious those are, really (I only have a 
fast laptop with GNU/Linux these days), but being wary of extra process 
calls seems prudent. Ideally, we'd reduce their number, not increase it.

> So we might revert the patch for vc-state and vc-working-revision indeed
> for the emacs-25 branch, going back to using vc-backend.

Thanks, I agree.

> In the master branch we might apply my proposed patch using
> vc-registered or something similar, and start to improve performance.

Improve how? Would you like to comment on the last paragraph of my 
previous email in this subthread?

I don't really see a point in returning `unregistered' from `vc-state'. 
When would the caller treat it differently from nil? And returning nil 
seems like an easier choice, implementation-wise, and well as a more 
conservative one from the backward compatibility perspective.

The `dir-status-files' backend command would continue including the 
`unregistered' entries (we could make it skip the up-to-date ones, 
though, in the interest of improving performance).

> In
> parallel, we shall start to write a VCS section for the elisp manual,
> describing vc-* functionality in more detail. We could start with
> vc-backend and vc-responsible-backend and their intended use. I'm
> missing such documentation for years.

I'd rather put the missing information into the docstrings, really. It 
seems unlikely that we're missing more than a few sentences in these two 
functions' descriptions, and we could also rephrase the existing ones.

But if you'd be more comfortable with having that information in the 
manual as well, don't let me stop you.

> I'll come back later today with the patch for emacs-25, if you agree.

In any case, I definitely agree with reverting vc-state and 
vc-working-revision to use vc-backend in Emacs 25.1.

bug#20637: incompatible, undocumented change to vc-working-revision

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Thu, 14 Apr 2016 09:10:09 +0200
> Cc: 20637@debbugs.gnu.org
> 
> And I still feel unhappy that none of the active Emacs maintainers feels
> responsible for vc*.el.

We have similar void with some other packages, unfortunately.
Volunteers are welcome.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> And I still feel unhappy that none of the active Emacs maintainers feels
>> responsible for vc*.el.
>
> I do feel responsible, and I try to take care of most bug reports in
> that area.

I know that you do and I really appreciate this!

But since ESRs rewrite in fall 2014, there are still several
inconsistencies. Eric confirmed that in an email to me. So it's not only
bug hunting, a systematic review of the state is needed.

I've started to write systematic tests (up to my limited knowledge), and
I've asked several times in emacs-devel ML for support, but nearly no
response. That's why I'm kinda frustrated.

Well, forget my lamenting. I'm still willing to help to improve. But I
cannot do this alone, as in the past.

>> I'm trying to support fixing problems here, but
>> most of the code, and the VCSes being used, are still terra incognita for me.
>
> It would be great if the problems we're fixing were formulated like
> "VC behaves like this, and it causes that problem in practice (or
> could cause, at least)", rather than "here is a test case, and I feel
> it should pass". I think that would encourage more practical choices.

I agree that the tests shall be much more commented. But I see also the
lack of documentation and implicit design decisions (you spoke about
somewhere), which must be improved I believe.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> I haven't thought too much about performance. But you are right, we
>> shouldn't add serious performance penalties to the code. And improving
>> performance for the 25.1 release is much too late.
>
> It's hard for me to judge how serious those are, really (I only have a
> fast laptop with GNU/Linux these days), but being wary of extra
> process calls seems prudent. Ideally, we'd reduce their number, not
> increase it.

Yes. I hope we could use more file properties caches. To be investigated.

>> So we might revert the patch for vc-state and vc-working-revision indeed
>> for the emacs-25 branch, going back to using vc-backend.
>
> Thanks, I agree.

I've committed this to emacs-25. Plus commenting the now failing checks
in vc-tests.el.

>> In the master branch we might apply my proposed patch using
>> vc-registered or something similar, and start to improve performance.
>
> Improve how? Would you like to comment on the last paragraph of my
> previous email in this subthread?

You've proposed an interface change in vc-state, returning just nil
instead of unregistered. Yes, this might be an option. Worth to be
checked.

Another possibility is to use cached properties more aggressively, as
said above.

> I don't really see a point in returning `unregistered' from
> `vc-state'. When would the caller treat it differently from nil? And
> returning nil seems like an easier choice, implementation-wise, and
> well as a more conservative one from the backward compatibility
> perspective.
>
> The `dir-status-files' backend command would continue including the
> `unregistered' entries (we could make it skip the up-to-date ones,
> though, in the interest of improving performance).

I cannot comment about this today. And as said already several times, if
we would document vc-* functions in the manual, it would allow us to
have a more global view on proposed changes. I trust you that you have
all involved interfaces in your mind. I haven't, and I would like to see
how an interface change compares to the other interfaces.

>> In
>> parallel, we shall start to write a VCS section for the elisp manual,
>> describing vc-* functionality in more detail. We could start with
>> vc-backend and vc-responsible-backend and their intended use. I'm
>> missing such documentation for years.
>
> I'd rather put the missing information into the docstrings, really. It
> seems unlikely that we're missing more than a few sentences in these
> two functions' descriptions, and we could also rephrase the existing
> ones.
>
> But if you'd be more comfortable with having that information in the
> manual as well, don't let me stop you.

Maybe docstrings are already sufficient. But you have spoken about
design decisions in the past (for example whether unregistered files
could be an argument), which I believe is not documented.

And at least for me the "global view" about vc-* functions is missing,
and how they are related.

>> I'll come back later today with the patch for emacs-25, if you agree.
>
> In any case, I definitely agree with reverting vc-state and
> vc-working-revision to use vc-backend in Emacs 25.1.

Yep. Pls test my patch, and confirm whether it is sufficient. Same for
Glenn, if possible. I would like to close this bug then, removing a
release blocker for Emacs 25.1.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 331 bytes --]

On 04/14/2016 09:31 PM, Michael Albinus wrote:

> I've committed this to emacs-25. Plus commenting the now failing checks
> in vc-tests.el.

Thanks. Any objections to committing this follow-up patch?

It makes the tests look a lot more meaningful, and it passes for all 
backends, AFAICT, but I don't have e.g. Monotone installed.

[-- Attachment #2: vc-tests.diff --]
[-- Type: text/x-patch, Size: 3082 bytes --]

diff --git a/test/automated/vc-tests.el b/test/automated/vc-tests.el
index 5042196..f9c0ce1 100644
--- a/test/automated/vc-tests.el
+++ b/test/automated/vc-tests.el
@@ -205,12 +205,10 @@ vc-test--create-repo
 (defun vc-test--unregister-function (backend file)
   "Run the `vc-unregister' backend function.
 For backends which dont support it, `vc-not-supported' is signalled."
-
-  (let ((symbol (intern (downcase (format "vc-%s-unregister" backend)))))
-    (if (functionp symbol)
-	(funcall symbol file)
-      ;; CVS, SVN, SCCS, SRC and Mtn are not supported.
-      (signal 'vc-not-supported (list 'unregister backend)))))
+  ;; CVS, SVN, SCCS, SRC and Mtn are not supported, and will signal
+  ;; `vc-not-supported'.
+  (vc-call-backend backend 'unregister file)
+  (vc-file-clearprops file))
 
 (defun vc-test--register (backend)
   "Register and unregister a file."
@@ -289,6 +287,9 @@ vc-test--state
           ;; added: Git
           ;; unregistered: CVS SCCS SRC
 	  ;; up-to-date: Bzr SVN
+          ;; FIXME: Delete this check.  A directory does not have a
+          ;; state, only files inside of it have states, and there is no way
+          ;; to combine those states into a single value unambiguously.
           (message "vc-state1 %s" (vc-state default-directory))
 	  ;;(should (eq (vc-state default-directory)
 		      ;;(vc-state default-directory backend)))
@@ -305,7 +306,7 @@ vc-test--state
             (message "vc-state2 %s" (vc-state tmp-name))
 	    ;;(should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
 	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+			  '(nil)))
 
 	    ;; Write a new file.  Check state.
 	    (write-region "foo" nil tmp-name nil 'nomessage)
@@ -317,19 +318,19 @@ vc-test--state
             (message "vc-state3 %s" (vc-state tmp-name))
 	    ;;(should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
 	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+			  '(nil)))
 
 	    ;; Register a file.  Check state.
 	    (vc-register
-	     (list backend (list (file-name-nondirectory tmp-name))))
+	     (list backend (list tmp-name)))
 
             ;; added: Git Mtn
             ;; unregistered: Hg RCS SCCS SRC SVN
             ;; up-to-date: Bzr CVS
             (message "vc-state4 %s" (vc-state tmp-name))
-	    ;;(should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
+	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
 	    (should (memq (vc-state tmp-name)
-                          '(nil added unregistered up-to-date)))
+                          '(added up-to-date)))
 
 	    ;; Unregister the file.  Check state.
 	    (condition-case nil
@@ -343,7 +344,7 @@ vc-test--state
                   (message "vc-state5 %s" (vc-state tmp-name))
 		  ;;(should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
 		  (should (memq (vc-state tmp-name)
-				'(nil added unregistered up-to-date))))
+				'(nil unregistered))))
 	      (vc-not-supported (message "vc-state5 unsupported")))))
 
       ;; Save exit.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/14/2016 06:26 PM, Michael Albinus wrote:

> But since ESRs rewrite in fall 2014, there are still several
> inconsistencies.

Do you mean any of the changes introduced in the rewrite? If so, which ones?

> Eric confirmed that in an email to me. So it's not only
> bug hunting, a systematic review of the state is needed.

Sure, VC has problems, but (vc-state FILE) not always being equal to 
(vc-state FILE BACKEND) does not seem like the biggest one. It hasn't 
come up on my radar even once before.

> I've started to write systematic tests (up to my limited knowledge), and
> I've asked several times in emacs-devel ML for support, but nearly no
> response. That's why I'm kinda frustrated.

I remember. I'm here now, open to questions and dicussions. I'd like to 
avoid switching to master myself for the time being, though.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/14/2016 09:31 PM, Michael Albinus wrote:

> Yes. I hope we could use more file properties caches. To be investigated.

OK, that could be a fine solution to the problem of vc-registered's 
slowness, but it adds complexity. So I'm still in favor of equating 
`unregistered' with nil, .

> And as said already several times, if
> we would document vc-* functions in the manual, it would allow us to
> have a more global view on proposed changes.

I disagree. The manual is the documentation for the users, to explain in 
depth, give examples, et cetera. The docstrings and VC's internal 
documentation have to stand on their own. It would be silly if the 
difference between `vc-backend' and `vc-responsible-backend' were to 
only be explained in the manual, but not in the docstrings.

That would also be unfair to people such as myself who prefer to consult 
the latter.

So, do you need anything from me in this area? E.g., feel free to give a 
list of docstrings that seem insufficient to you, together with what you 
feel they are missing.

> I trust you that you have
> all involved interfaces in your mind. I haven't, and I would like to see
> how an interface change compares to the other interfaces.

I don't really know everything about VC, I just have some recollections 
about dealing with it, as well as experience writing a third-party 
package depending on VC's API.

To get an opinion about the current bug report, I still had to dig into 
the code and investigate, look at the commit history, search for call 
occurrences, etc.

> But you have spoken about
> design decisions in the past (for example whether unregistered files
> could be an argument), which I believe is not documented.

BTW, we've mentioned it before when fixing my old bug report about VC 
using too many process calls 
(http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11757#77).

It may not have even been a deliberate design decision, but it's the way 
`vc-state' is used. Which, in turn, allows backend implementations to be 
sloppy in the cases that are (almost?) never exercised.

> And at least for me the "global view" about vc-* functions is missing,
> and how they are related.

I usually tease that kind of information out by reading the source code. 
Is there anything in particular I could help add to your understanding 
of the "global view"?

> Yep. Pls test my patch, and confirm whether it is sufficient. Same for
> Glenn, if possible. I would like to close this bug then, removing a
> release blocker for Emacs 25.1.

It must fix this bug, since it reverts to the old code, and testing 
Glenn's example from the description confirms as much.

So I think it can be closed, and the discussion should move to emacs-devel.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/15/2016 04:01 AM, Dmitry Gutov wrote:
> So I'm still in favor of equating
> `unregistered' with nil, .

...pending further arguments in favor of the other solution.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Thanks. Any objections to committing this follow-up patch?
>
> It makes the tests look a lot more meaningful, and it passes for all
> backends, AFAICT, but I don't have e.g. Monotone installed.

I would prefer to apply further patches to the master branch only. Only
patches absolutely necessary for fixing problems shall go to the
emacs-25 branch, in order not to threaten the release.

After applying your patch, vc-tests.el fails for me in vc-test-src02-state
and vc-test-rcs03-working-revision.

> @@ -205,12 +205,10 @@ vc-test--create-repo
>  (defun vc-test--unregister-function (backend file)
>    "Run the `vc-unregister' backend function.
>  For backends which dont support it, `vc-not-supported' is signalled."
> -
> -  (let ((symbol (intern (downcase (format "vc-%s-unregister" backend)))))
> -    (if (functionp symbol)
> -	(funcall symbol file)
> -      ;; CVS, SVN, SCCS, SRC and Mtn are not supported.
> -      (signal 'vc-not-supported (list 'unregister backend)))))
> +  ;; CVS, SVN, SCCS, SRC and Mtn are not supported, and will signal
> +  ;; `vc-not-supported'.
> +  (vc-call-backend backend 'unregister file)
> +  (vc-file-clearprops file))

As said, in the master branch I would prefer to add a vc-unregister
function. Then we won't need this anymore.

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> I've started to write systematic tests (up to my limited knowledge), and
>> I've asked several times in emacs-devel ML for support, but nearly no
>> response. That's why I'm kinda frustrated.
>
> I remember. I'm here now, open to questions and dicussions. I'd like
> to avoid switching to master myself for the time being, though.

I have installed both versions in parallel on my laptop. No need to
switch, therefore :-)

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 04/14/2016 09:31 PM, Michael Albinus wrote:
>
>> Yes. I hope we could use more file properties caches. To be investigated.
>
> OK, that could be a fine solution to the problem of vc-registered's
> slowness, but it adds complexity. So I'm still in favor of equating
> `unregistered' with nil, .

I'm not against this. But I would like to see the whole picture
first. Where else unregistered is used, and whether we run into
conflicts when using nil instead of unregistered.

>> And as said already several times, if
>> we would document vc-* functions in the manual, it would allow us to
>> have a more global view on proposed changes.
>
> I disagree. The manual is the documentation for the users, to explain
> in depth, give examples, et cetera. The docstrings and VC's internal
> documentation have to stand on their own. It would be silly if the
> difference between `vc-backend' and `vc-responsible-backend' were to
> only be explained in the manual, but not in the docstrings.

Are we speaking about different manuals? I'm speaking about the ‘GNU
Emacs Lisp Reference Manual’, and not the ‘GNU Emacs Manual’ (the manual
dedicated to users).

> That would also be unfair to people such as myself who prefer to
> consult the latter.

With your argument, we could nuke the Emacs Lisp manual. Shall we?

> So, do you need anything from me in this area? E.g., feel free to give
> a list of docstrings that seem insufficient to you, together with what
> you feel they are missing.

I will start somehow, and show you for review.

>> I trust you that you have all involved interfaces in your mind. I
>> haven't, and I would like to see how an interface change compares to
>> the other interfaces.
>
> I don't really know everything about VC, I just have some
> recollections about dealing with it, as well as experience writing a
> third-party package depending on VC's API.
>
> To get an opinion about the current bug report, I still had to dig
> into the code and investigate, look at the commit history, search for
> call occurrences, etc.

My first sentence above is a rhetorical one, of course :-)

>> But you have spoken about
>> design decisions in the past (for example whether unregistered files
>> could be an argument), which I believe is not documented.
>
> BTW, we've mentioned it before when fixing my old bug report about VC
> using too many process calls
> (http://debbugs.gnu.org/cgi/bugreport.cgi?bug=11757#77).

Yes, but that's not the documentation! I'm pretty good in forgetting
everything, and I don't remember what was said in message 77 of bug
report 11757, months ago ...

> It may not have even been a deliberate design decision, but it's the
> way `vc-state' is used. Which, in turn, allows backend implementations
> to be sloppy in the cases that are (almost?) never exercised.
>
>> And at least for me the "global view" about vc-* functions is missing,
>> and how they are related.
>
> I usually tease that kind of information out by reading the source
> code. Is there anything in particular I could help add to your
> understanding of the "global view"?

Even if I understand it, it won't help any other developer. Let's
document it.

>> Yep. Pls test my patch, and confirm whether it is sufficient. Same for
>> Glenn, if possible. I would like to close this bug then, removing a
>> release blocker for Emacs 25.1.
>
> It must fix this bug, since it reverts to the old code, and testing
> Glenn's example from the description confirms as much.
>
> So I think it can be closed, and the discussion should move to emacs-devel.

OK, I'm waiting for some few days (maybe Glenn want to say something
about), and then I'll close this bug. Thanks for all your help this way!

Best regards, Michael.

Docstrings and manuals, was: Re: bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/15/2016 04:23 PM, Michael Albinus wrote:

>> I disagree. The manual is the documentation for the users, to explain
>> in depth, give examples, et cetera. The docstrings and VC's internal
>> documentation have to stand on their own. It would be silly if the
>> difference between `vc-backend' and `vc-responsible-backend' were to
>> only be explained in the manual, but not in the docstrings.
>
> Are we speaking about different manuals? I'm speaking about the ‘GNU
> Emacs Lisp Reference Manual’, and not the ‘GNU Emacs Manual’ (the manual
> dedicated to users).

OK, so the Emacs Lisp Reference is a counter-example. And it contains a 
narrative that explains different aspects of Emacs Lisp in a sort-of 
seamless fashion, which is great, because there's no good way to do that 
with just docstrings.

And then it goes ahead and provides its own versions of descriptions for 
functions and variables that are different from the docstrings, and are 
often longer, but sometimes shorter (e.g. for `obarray'), and at times 
it seems like the only reasons for the existence of the two versions is 
to have docstrings in the imperative voice, and the pieces of text in 
the manual, in the passive voice.

And then the two versions start to diverge. For instance, the docstring 
for `mapatoms' is rather short. The manual entry for it is longer, *and* 
it documents that `mapatoms' returns nil. Why would someone put that 
piece of information into the manual, but not the docstring, is beyond me.

So, to be clear, I dislike the ambiguity that manuals that try to be 
all-encompassing, like this one, create about the main source of 
information about each function.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/15/2016 04:23 PM, Michael Albinus wrote:

> I'm not against this. But I would like to see the whole picture
> first. Where else unregistered is used, and whether we run into
> conflicts when using nil instead of unregistered.

Sure. I'm reasonably confident, but please take the time and do your own 
evaluation.

>> I disagree. The manual is the documentation for the users, to explain
>> in depth, give examples, et cetera. The docstrings and VC's internal
>> documentation have to stand on their own. It would be silly if the
>> difference between `vc-backend' and `vc-responsible-backend' were to
>> only be explained in the manual, but not in the docstrings.
>
> Are we speaking about different manuals? I'm speaking about the ‘GNU
> Emacs Lisp Reference Manual’, and not the ‘GNU Emacs Manual’ (the manual
> dedicated to users).

I've split off a tangent to emacs-devel. Emacs Lisp Reference is a 
counter-example, but I think the last sentence in my paragraph above is 
still correct.

VC has been documented in the docstrings and the Commentary sections 
(especially the one at the begining of vc.el). The new important 
information should go there first.

If then you'd like to create a manual based on that information, to 
present it in more digestible form, sure, but let's not put anything 
essential into the manual only. We might avoid publishing that manual, 
though, until we're sure that VC is rock-solid to build third-party code on.

>> That would also be unfair to people such as myself who prefer to
>> consult the latter.
>
> With your argument, we could nuke the Emacs Lisp manual. Shall we?

Does your argument allow nuking all docstrings and comments?

>> So, do you need anything from me in this area? E.g., feel free to give
>> a list of docstrings that seem insufficient to you, together with what
>> you feel they are missing.
>
> I will start somehow, and show you for review.

Thanks.

>> I usually tease that kind of information out by reading the source
>> code. Is there anything in particular I could help add to your
>> understanding of the "global view"?
>
> Even if I understand it, it won't help any other developer. Let's
> document it.

Sure.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/15/2016 04:11 PM, Michael Albinus wrote:

> I would prefer to apply further patches to the master branch only. Only
> patches absolutely necessary for fixing problems shall go to the
> emacs-25 branch, in order not to threaten the release.

I can do that.

> After applying your patch, vc-tests.el fails for me in vc-test-src02-state

How do you install src? Do you build it from source?

> and vc-test-rcs03-working-revision.

This one (failure in vc-checkout-model5) seems to be a real bug 
uncovered by using vc-file-clearprops. I suppose we should add a FIXME 
there and skip that check for RCS.

>> -  (let ((symbol (intern (downcase (format "vc-%s-unregister" backend)))))
>> -    (if (functionp symbol)
>> -	(funcall symbol file)
>> -      ;; CVS, SVN, SCCS, SRC and Mtn are not supported.
>> -      (signal 'vc-not-supported (list 'unregister backend)))))
>> +  ;; CVS, SVN, SCCS, SRC and Mtn are not supported, and will signal
>> +  ;; `vc-not-supported'.
>> +  (vc-call-backend backend 'unregister file)
>> +  (vc-file-clearprops file))
>
> As said, in the master branch I would prefer to add a vc-unregister
> function. Then we won't need this anymore.

We'll need this code either way. Where the function resides and how it 
is called is less important to me.

Re: Docstrings and manuals

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> So, to be clear, I dislike the ambiguity that manuals that try to be
> all-encompassing, like this one, create about the main source of
> information about each function.

We will always find examples where the entries in the manual are
inferior, compared with the docstring. We will also find examples with
incompatible information in the docstring and the manual.

This does not mean that manuals are useless. It only means, that there's
a bug to be fixed.

Best regards, Michael.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 11:49 AM, Michael Albinus wrote:

> We will always find examples where the entries in the manual are
> inferior, compared with the docstring. We will also find examples with
> incompatible information in the docstring and the manual.

The `mapatoms' manual entry is neither. And yet, wouldn't you agree that 
it's problematic?

> This does not mean that manuals are useless. It only means, that there's
> a bug to be fixed.

Sure. But I think that means that we should have a policy that the 
manual is secondary to the information contained in the source files. 
Right now, I have no idea which one is supposed to be the primary source 
of truth.

Re: Docstrings and manuals

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> This does not mean that manuals are useless. It only means, that there's
>> a bug to be fixed.
>
> Sure. But I think that means that we should have a policy that the
> manual is secondary to the information contained in the source
> files. Right now, I have no idea which one is supposed to be the
> primary source of truth.

No primary or secondary. If docstring and manual are inconsistent, it is
a bug which must be fixed. There is no automatism that the docstring is
always right, and the manual is wrong in this case. It could be also
vice versa.

See also the Elisp Manual (info "(elisp)Caveats")

--8<---------------cut here---------------start------------->8---
   The manual should be fully correct in what it does cover, and it is
therefore open to criticism on anything it says—from specific examples
and descriptive text, to the ordering of chapters and sections.  If
something is confusing, or you find that you have to look at the sources
or experiment to learn something not covered in the manual, then perhaps
the manual should be fixed.  Please let us know.
--8<---------------cut here---------------end--------------->8---

Best regards, Michael.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 02:16 PM, Michael Albinus wrote:

> No primary or secondary. If docstring and manual are inconsistent, it is
> a bug which must be fixed.

What's "inconsistent" in this case? They are almost always different.

> There is no automatism that the docstring is
> always right, and the manual is wrong in this case. It could be also
> vice versa.

Wouldn't it be better if we had this "automatism"?

> See also the Elisp Manual (info "(elisp)Caveats")
>
> --8<---------------cut here---------------start------------->8---
>    The manual should be fully correct in what it does cover, and it is
> therefore open to criticism on anything it says—from specific examples
> and descriptive text, to the ordering of chapters and sections.  If
> something is confusing, or you find that you have to look at the sources
> or experiment to learn something not covered in the manual, then perhaps
> the manual should be fixed.  Please let us know.
> --8<---------------cut here---------------end--------------->8---

I'm not arguing in favor of leaving mistakes in the manual. But I think 
it should be strictly a derivative work. I.e. the docstrings must 
contain the complete information (if maybe presented in a terse 
fashion), and the manual could rephrase that, only to make it more 
accessible (but not more informative).

Re: Docstrings and manuals

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> I'm not arguing in favor of leaving mistakes in the manual. But I
> think it should be strictly a derivative work. I.e. the docstrings
> must contain the complete information (if maybe presented in a terse
> fashion), and the manual could rephrase that, only to make it more
> accessible (but not more informative).

I still don't understand what problem you discuss. There are docstrings,
and there are manuals. Both have their reasons to exist.

What do you want else? Nobody urges you to read the manuals, if you
don't want.

I believe it would be good if vc-* functions are covered in a
manual. What is the problem with this?

Best regards, Michael.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 03:19 PM, Michael Albinus wrote:
> Dmitry Gutov <dgutov@yandex.ru> writes:
>
>> I'm not arguing in favor of leaving mistakes in the manual. But I
>> think it should be strictly a derivative work. I.e. the docstrings
>> must contain the complete information (if maybe presented in a terse
>> fashion), and the manual could rephrase that, only to make it more
>> accessible (but not more informative).
>
> I still don't understand what problem you discuss. There are docstrings,
> and there are manuals. Both have their reasons to exist.

My problem is that you stated the lack of a manual as the reason for 
your lack of understanding of VC's internals. I'm saying the problem is 
them being documented insufficiently in the code. And, like I mentioned, 
if you go ahead and write the newfound revelations in the manual, but 
not anywhere else, a certain slice of developers is going to miss out.

To expand on my message in 
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20637#91, an example:

If we make a decision that vc-BACKEND-state can rely on FILE not having 
state `unregistered' (I'm not saying we should; it's just one option), 
that information should go into the Commentary at the top of vc.el 
(probably into the description of the `state' command), but...

> I believe it would be good if vc-* functions are covered in a
> manual. What is the problem with this?

...if it also finds its way into a manual later, I don't see any harm.

To put a different spin on my statement earlier: if neither the 
docstrings, nor the manual are considered the Single Source of Truth, to 
read a reasonably complete description of a function, I'd have to read 
both the docstring and the manual. And that's just wasteful.

And even the personal preferences aside, we can't make the manual to be 
the SSOT because most functions are not documented there.

Re: Docstrings and manuals

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 17 Apr 2016 13:50:35 +0300
> Cc: Glenn Morris <rgm@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> 
> On 04/17/2016 11:49 AM, Michael Albinus wrote:
> 
> > We will always find examples where the entries in the manual are
> > inferior, compared with the docstring. We will also find examples with
> > incompatible information in the docstring and the manual.
> 
> The `mapatoms' manual entry is neither. And yet, wouldn't you agree that 
> it's problematic?

If you mean that mapatoms' doc string is too terse and omits some
details it shouldn't, I agree.  Otherwise, I'm not sure what you mean;
please elaborate.

> > This does not mean that manuals are useless. It only means, that there's
> > a bug to be fixed.
> 
> Sure. But I think that means that we should have a policy that the 
> manual is secondary to the information contained in the source files. 

No, it's not secondary.  It should be an expanded and augmented
version of the same information.

> Right now, I have no idea which one is supposed to be the primary source 
> of truth.

Both.  There's more than one way to tell the truth.

Re: Docstrings and manuals

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 17 Apr 2016 14:42:28 +0300
> Cc: Glenn Morris <rgm@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> 
> On 04/17/2016 02:16 PM, Michael Albinus wrote:
> 
> > No primary or secondary. If docstring and manual are inconsistent, it is
> > a bug which must be fixed.
> 
> What's "inconsistent" in this case? They are almost always different.

They can be different, but still consistent.  There's more than one
way to tell the same story, without contradicting the others.

> > There is no automatism that the docstring is
> > always right, and the manual is wrong in this case. It could be also
> > vice versa.
> 
> Wouldn't it be better if we had this "automatism"?

No, not IMO.

> I'm not arguing in favor of leaving mistakes in the manual. But I think 
> it should be strictly a derivative work. I.e. the docstrings must 
> contain the complete information (if maybe presented in a terse 
> fashion), and the manual could rephrase that, only to make it more 
> accessible (but not more informative).

Once you allow rephrasing, you already allow deviation.  And there
really is no way around allowing it, because a manual that includes
only the doc strings with a minimum "glue" is much less palatable.
You can look at Guile as one example; GnuTLS is another.  Such manuals
are much less useful, IME, than what we have in Emacs.

One thing that you will never find in doc strings is a discussion of
merits and demerits of different ways of solving some problem,
especially if such a discussion requires to jump back and forth
between these ways, in order to make some point.

IOW, writing a good manual is still a human activity that cannot be
easily automated.  Ideally, doc strings should be phrased like
reference material, covering all the traits as tersely as possible,
while the manual should provide an easier reading with more continuity
text and even some explanations why things are the way they are.  At
least IMO.

Re: Docstrings and manuals

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 17 Apr 2016 16:12:05 +0300
> Cc: Glenn Morris <rgm@gnu.org>, emacs-devel <emacs-devel@gnu.org>
> 
> My problem is that you stated the lack of a manual as the reason for 
> your lack of understanding of VC's internals. I'm saying the problem is 
> them being documented insufficiently in the code. And, like I mentioned, 
> if you go ahead and write the newfound revelations in the manual, but 
> not anywhere else, a certain slice of developers is going to miss out.

yes, we should update both the doc strings and the manual(s).

> To put a different spin on my statement earlier: if neither the 
> docstrings, nor the manual are considered the Single Source of Truth, to 
> read a reasonably complete description of a function, I'd have to read 
> both the docstring and the manual. And that's just wasteful.
> 
> And even the personal preferences aside, we can't make the manual to be 
> the SSOT because most functions are not documented there.

The doc strings and the manual should convey the same information,
where they both cover the same turf.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 06:03 PM, Eli Zaretskii wrote:

>> The `mapatoms' manual entry is neither. And yet, wouldn't you agree that
>> it's problematic?
>
> If you mean that mapatoms' doc string is too terse and omits some
> details it shouldn't, I agree.  Otherwise, I'm not sure what you mean;
> please elaborate.

That someone saw fit to put "Then it returns ‘nil’" into the manual when 
it's not in the docstring. And that it's easy to make such a mistake.

>> Sure. But I think that means that we should have a policy that the
>> manual is secondary to the information contained in the source files.
>
> No, it's not secondary.  It should be an expanded and augmented
> version of the same information.

Either you're saying the same as me (when writing a manual, you 
elaborate, but not add new essential new information, and thus make a 
derivative), or I don't understand how to produce the manual entries.

> Both.  There's more than one way to tell the truth.

Both of them are necessarily fallible, it seems.

RE: Docstrings and manuals

[Drew Adams]

> > > No primary or secondary. If docstring and manual are inconsistent, it
> > > is a bug which must be fixed.
> >
> > What's "inconsistent" in this case? They are almost always different.
> 
> They can be different, but still consistent.  There's more than one
> way to tell the same story, without contradicting the others.
> 
> > > There is no automatism that the docstring is
> > > always right, and the manual is wrong in this case. It could be also
> > > vice versa.
> >
> > Wouldn't it be better if we had this "automatism"?
> 
> No, not IMO.
> 
> > I'm not arguing in favor of leaving mistakes in the manual. But I think
> > it should be strictly a derivative work. I.e. the docstrings must
> > contain the complete information (if maybe presented in a terse
> > fashion), and the manual could rephrase that, only to make it more
> > accessible (but not more informative).
> 
> Once you allow rephrasing, you already allow deviation.  And there
> really is no way around allowing it, because a manual that includes
> only the doc strings with a minimum "glue" is much less palatable.
> You can look at Guile as one example; GnuTLS is another.  Such manuals
> are much less useful, IME, than what we have in Emacs.
> 
> One thing that you will never find in doc strings is a discussion of
> merits and demerits of different ways of solving some problem,
> especially if such a discussion requires to jump back and forth
> between these ways, in order to make some point.
> 
> IOW, writing a good manual is still a human activity that cannot be
> easily automated.  Ideally, doc strings should be phrased like
> reference material, covering all the traits as tersely as possible,
> while the manual should provide an easier reading with more continuity
> text and even some explanations why things are the way they are.  At
> least IMO.

+10 to what Eli and Michael are saying.

Neither doc string nor manual serves the purpose of
the other.  The manual is not Javadoc.

It is up to humans to write the most appropriate text
for each, and to make coherent any differences in
message or presentation.

Differences should be based on the context (including
readers) and should not involve contradiction in
meaning (semantics, behavior) of the things described.

IOW, both need to be faithful descriptions of those
things.  There can be differences in degree of
abstraction or of precision.  But within a given level
of abstraction/precision, each must be a faithful
picture of the behavior it describes.

The same is true of any other communication about these
things to users, including tooltips and error messages:
The behavior/meaning of the program being described is
the same, and that needs to be reflected in a lack of
contradiction wrt description of that behavior/meaning.

Re: Docstrings and manuals

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, rgm@gnu.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 17 Apr 2016 18:15:17 +0300
> 
> On 04/17/2016 06:03 PM, Eli Zaretskii wrote:
> 
> >> The `mapatoms' manual entry is neither. And yet, wouldn't you agree that
> >> it's problematic?
> >
> > If you mean that mapatoms' doc string is too terse and omits some
> > details it shouldn't, I agree.  Otherwise, I'm not sure what you mean;
> > please elaborate.
> 
> That someone saw fit to put "Then it returns ‘nil’" into the manual when 
> it's not in the docstring. And that it's easy to make such a mistake.

Making mistakes is always easier than avoiding them.  I don't think
this fact is significant.

> >> Sure. But I think that means that we should have a policy that the
> >> manual is secondary to the information contained in the source files.
> >
> > No, it's not secondary.  It should be an expanded and augmented
> > version of the same information.
> 
> Either you're saying the same as me (when writing a manual, you 
> elaborate, but not add new essential new information, and thus make a 
> derivative), or I don't understand how to produce the manual entries.

To produce a good manual text, you need to try to describe and explain
the subject in some logical way.  The result is not a derivative, in
that it doesn't necessarily repeat the doc strings and then adds some
more stuff (although doing that is sometimes TRT).  It could be that
most of the text of the manual section has no direct relation at all
to the doc strings.  It could even be an entirely different POV of
looking at the subject.  E.g., compare the node "Generic Functions" in
the ELisp manual with the doc strings of the symbols covered by that
node.  As an even more extreme example, compare the node
"Bidirectional Editing" in the user manual with the doc strings of the
3 variables it covers.

> > Both.  There's more than one way to tell the truth.
> 
> Both of them are necessarily fallible, it seems.

Of course.  "To err is human".

Re: Docstrings and manuals

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> My problem is that you stated the lack of a manual as the reason for
> your lack of understanding of VC's internals. I'm saying the problem
> is them being documented insufficiently in the code. And, like I
> mentioned, if you go ahead and write the newfound revelations in the
> manual, but not anywhere else, a certain slice of developers is going
> to miss out.
>
> To expand on my message in
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=20637#91, an example:
>
> If we make a decision that vc-BACKEND-state can rely on FILE not
> having state `unregistered' (I'm not saying we should; it's just one
> option), that information should go into the Commentary at the top of
> vc.el (probably into the description of the `state' command), but...

So we are in agreement :-)

Of course, every relevant documentation must be up-to-date. Docstring,
commentary section, manual.

Personally, I believe a commentary section is a poor man's manual. Once
the information is in the manual, the commentary section is not needed
anymore. But this is nothing we need to worry about just now; let's
discuss it once a proper manual entry about vc exist.

Best regards, Michael.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 07:39 PM, Michael Albinus wrote:

> So we are in agreement :-)

Kind of. I disagree that "writing the manual" is going to solve any of 
the pressing problems, but it is a valid activity to partake in.

> Personally, I believe a commentary section is a poor man's manual.

Yes and no. It's certainly easier to navigate, at least while it's below 
a certain size (which is still true of vc.el's Commentary).

> Once
> the information is in the manual, the commentary section is not needed
> anymore. But this is nothing we need to worry about just now; let's
> discuss it once a proper manual entry about vc exist.

...so I have my reservations about replacing it. But yes, we can discuss 
it later.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 06:12 PM, Eli Zaretskii wrote:

> Once you allow rephrasing, you already allow deviation.  And there
> really is no way around allowing it, because a manual that includes
> only the doc strings with a minimum "glue" is much less palatable.
> You can look at Guile as one example; GnuTLS is another.  Such manuals
> are much less useful, IME, than what we have in Emacs.

I'd rather minimize rephrasing in the manuals, while still allowing the 
"glue" text. Instead of having two ways to describe each important 
variable, I'd rather have just the docstrings, which could be adjusted 
to be more approachable. That would require some new Texinfo syntax to 
"insert the docstring here".

The narrative can still be free-form, but at least there would be one 
fewer source of duplication.

> One thing that you will never find in doc strings is a discussion of
> merits and demerits of different ways of solving some problem,
> especially if such a discussion requires to jump back and forth
> between these ways, in order to make some point.

The counterpart to "glue" text outside of the manuals is the Commentary 
section usually. While they're also terse usually (but not always), they 
aren't bound by the limitation that a docstring has to relate to a 
particular symbol.

> IOW, writing a good manual is still a human activity that cannot be
> easily automated.  Ideally, doc strings should be phrased like
> reference material, covering all the traits as tersely as possible,
> while the manual should provide an easier reading with more continuity
> text and even some explanations why things are the way they are.  At
> least IMO.

That sounds fine, and maybe this way is best for the more complex 
subjects, but in general I'd prefer to see docstrings also written with 
readability in mind. Then, when writing a manual section, you'll only 
need to write the "glue" text, and pick which symbol references, with 
docstrings, to add, and in which order.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/17/2016 07:23 PM, Eli Zaretskii wrote:

> To produce a good manual text, you need to try to describe and explain
> the subject in some logical way.  The result is not a derivative, in
> that it doesn't necessarily repeat the doc strings and then adds some
> more stuff (although doing that is sometimes TRT).

It should be a derivative in the sense that it's produced based on 
knowing the docstrings and the common context they're used it. But not 
some additional inner details about each symbol that never made it into 
docstrings, I think.

> It could be that
> most of the text of the manual section has no direct relation at all
> to the doc strings.  It could even be an entirely different POV of
> looking at the subject.  E.g., compare the node "Generic Functions" in
> the ELisp manual with the doc strings of the symbols covered by that
> node.  As an even more extreme example, compare the node
> "Bidirectional Editing" in the user manual with the doc strings of the
> 3 variables it covers.

Bidirectional Editing is fine. It describes a facility that's largely 
implemented in C, so the three variables are just a small part of it.

Generic Functions, on the other hand, may be indicative of the problem 
that I've brought up: the descriptions of `cl-defgeneric' and 
`cl-defmethod' and much longer in the manual than their docstrings 
(especially the latter one). I see no particular reason for them to be 
different in this case.

Maybe that will require restructuring the manual text to move the 
enumeration of the possible types (defined in cl--generic-typeof-types) 
to a separate paragraph or section, but that seems only beneficial.

Why would we want to mention that :before methods get called "in the 
most-specific-first order" only in the manual, but not in the docstring? 
And so on.

When a docstring gets too complicated, we can go the way of `cl-loop' 
to, again, avoid repeating ourselves. In that case I at least know to 
consult the manual, and which node to visit.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/14/2016 09:31 PM, Michael Albinus wrote:

> I've committed this to emacs-25. Plus commenting the now failing checks
> in vc-tests.el.

We should also consider reverting the changes to vc-rcs-unregister and 
vc-sccs-working-revision. They don't seem particularly necessary (the 
tests pass without them).

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

On 04/15/2016 04:11 PM, Michael Albinus wrote:

> I would prefer to apply further patches to the master branch only. Only
> patches absolutely necessary for fixing problems shall go to the
> emacs-25 branch, in order not to threaten the release.
>
> After applying your patch, vc-tests.el fails for me in vc-test-src02-state
> and vc-test-rcs03-working-revision.

Here's the patch against master. The latter one doesn't fail there.

Could you handle the SRC test failure yourself? Either by fixing the 
relevant code, or by simply exempting SRC from the relevant check.

[-- Attachment #2: vc-backend-and-vc-tests.diff --]
[-- Type: text/x-patch, Size: 7978 bytes --]

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 4c0161d..43cbb3f 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -476,7 +476,7 @@ vc-state
   ;; - `copied' and `moved' (might be handled by `removed' and `added')
   (or (vc-file-getprop file 'vc-state)
       (when (> (length file) 0)         ;Why??  --Stef
-	(setq backend (or backend (vc-responsible-backend file)))
+	(setq backend (or backend (vc-backend file)))
 	(when backend
           (vc-state-refresh file backend)))))
 
@@ -495,7 +495,7 @@ vc-working-revision
 If FILE is not registered, this function always returns nil."
   (or (vc-file-getprop file 'vc-working-revision)
       (progn
-	(setq backend (or backend (vc-responsible-backend file)))
+	(setq backend (or backend (vc-backend file)))
 	(when backend
 	  (vc-file-setprop file 'vc-working-revision
 			   (vc-call-backend backend 'working-revision file))))))
diff --git a/test/lisp/vc/vc-tests.el b/test/lisp/vc/vc-tests.el
index 2b3445a..cabd560 100644
--- a/test/lisp/vc/vc-tests.el
+++ b/test/lisp/vc/vc-tests.el
@@ -205,16 +205,10 @@ vc-test--create-repo
 (defun vc-test--unregister-function (backend file)
   "Run the `vc-unregister' backend function.
 For backends which dont support it, `vc-not-supported' is signalled."
-
-  (unwind-protect
-      (let ((symbol (intern (downcase (format "vc-%s-unregister" backend)))))
-        (if (functionp symbol)
-            (funcall symbol file)
-          ;; CVS, SVN, SCCS, SRC and Mtn are not supported.
-          (signal 'vc-not-supported (list 'unregister backend))))
-
-    ;; FIXME This shall be called in `vc-unregister'.
-    (vc-file-clearprops file)))
+  ;; CVS, SVN, SCCS, SRC and Mtn are not supported, and will signal
+  ;; `vc-not-supported'.
+  (vc-call-backend backend 'unregister file)
+  (vc-file-clearprops file))
 
 (defun vc-test--register (backend)
   "Register and unregister a file.
@@ -312,43 +306,30 @@ vc-test--state
 	   'vc-test--cleanup-hook
 	   `(lambda () (delete-directory ,default-directory 'recursive)))
 
-	  ;; Create empty repository.  Check repository state.
+	  ;; Create empty repository.
 	  (make-directory default-directory)
 	  (vc-test--create-repo-function backend)
 
-	  ;; nil: Hg Mtn RCS
-          ;; added: Git
-          ;; unregistered: CVS SCCS SRC
-	  ;; up-to-date: Bzr SVN
-          (message "vc-state1 %s" (vc-state default-directory))
-	  (should (eq (vc-state default-directory)
-		      (vc-state default-directory backend)))
-	  (should (memq (vc-state default-directory)
-			'(nil added unregistered up-to-date)))
-
 	  (let ((tmp-name (expand-file-name "foo" default-directory)))
-	    ;; Check state of an empty file.
+	    ;; Check state of a nonexistent file.
 
-	    ;; nil: Hg Mtn SRC SVN
+            (message "vc-state2 %s" (vc-state tmp-name))
+            ;; FIXME:
             ;; added: Git
 	    ;; unregistered: RCS SCCS
 	    ;; up-to-date: Bzr CVS
-            (message "vc-state2 %s" (vc-state tmp-name))
-	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+	    ;; (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
+	    (should (null (vc-state tmp-name)))
 
 	    ;; Write a new file.  Check state.
 	    (write-region "foo" nil tmp-name nil 'nomessage)
 
-            ;; nil: Mtn
-            ;; added: Git
-            ;; unregistered: Hg RCS SCCS SRC SVN
-            ;; up-to-date: Bzr CVS
             (message "vc-state3 %s" (vc-state tmp-name))
-	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name)
-			  '(nil added unregistered up-to-date)))
+            ;; FIXME:
+            ;; added: Git
+            ;; unregistered: Bzr Hg RCS SCCS SRC SVN CVS
+	    ;; (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
+	    (should (null (vc-state tmp-name)))
 
 	    ;; Register a file.  Check state.
 	    (vc-register
@@ -359,21 +340,20 @@ vc-test--state
             ;; up-to-date: Bzr CVS
             (message "vc-state4 %s" (vc-state tmp-name))
 	    (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-	    (should (memq (vc-state tmp-name) '(added unregistered up-to-date)))
+	    (should (memq (vc-state tmp-name) '(added up-to-date)))
 
 	    ;; Unregister the file.  Check state.
 	    (condition-case err
 		(progn
 		  (vc-test--unregister-function backend tmp-name)
 
-		  ;; added: Git
-		  ;; unregistered: Hg RCS
 		  ;; unsupported: CVS Mtn SCCS SRC SVN
-		  ;; up-to-date: Bzr
                   (message "vc-state5 %s" (vc-state tmp-name))
-		  (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
-		  (should (memq (vc-state tmp-name)
-				'(added unregistered up-to-date))))
+                  ;; FIXME:
+                  ;; added: Git
+		  ;; unregistered: Hg RCS Bzr
+		  ;; (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))
+		  (should (null (vc-state tmp-name))))
 	      (vc-not-supported (message "vc-state5 unsupported"))
               (t (signal (car err) (cdr err))))))
 
@@ -406,8 +386,8 @@ vc-test--working-revision
 	  ;; "0": Bzr Hg SRC SVN
           (message
            "vc-working-revision1 %s" (vc-working-revision default-directory))
-	  (should (eq (vc-working-revision default-directory)
-		      (vc-working-revision default-directory backend)))
+	  ;; (should (eq (vc-working-revision default-directory)
+	  ;;             (vc-working-revision default-directory backend)))
 	  (should (member (vc-working-revision default-directory) '(nil "0")))
 
 	  (let ((tmp-name (expand-file-name "foo" default-directory)))
@@ -417,8 +397,8 @@ vc-test--working-revision
 	    ;; nil: CVS Git Mtn RCS SCCS SVN
 	    ;; "0": Bzr Hg SRC
             (message "vc-working-revision2 %s" (vc-working-revision tmp-name))
-	    (should (eq (vc-working-revision tmp-name)
-			(vc-working-revision tmp-name backend)))
+	    ;; (should (eq (vc-working-revision tmp-name)
+	    ;;     	(vc-working-revision tmp-name backend)))
 	    (should (member (vc-working-revision tmp-name) '(nil "0")))
 
 	    ;; Write a new file.  Check working revision.
@@ -427,8 +407,8 @@ vc-test--working-revision
 	    ;; nil: CVS Git Mtn RCS SCCS SVN
 	    ;; "0": Bzr Hg SRC
             (message "vc-working-revision3 %s" (vc-working-revision tmp-name))
-	    (should (eq (vc-working-revision tmp-name)
-			(vc-working-revision tmp-name backend)))
+	    ;; (should (eq (vc-working-revision tmp-name)
+	    ;;     	(vc-working-revision tmp-name backend)))
 	    (should (member (vc-working-revision tmp-name) '(nil "0")))
 
 	    ;; Register a file.  Check working revision.
@@ -439,22 +419,22 @@ vc-test--working-revision
 	    ;; "0": Bzr CVS Hg SRC SVN
             ;; "1.1"  RCS SCCS
             (message "vc-working-revision4 %s" (vc-working-revision tmp-name))
-	    (should (eq (vc-working-revision tmp-name)
-			(vc-working-revision tmp-name backend)))
+	    ;; (should (eq (vc-working-revision tmp-name)
+	    ;;     	(vc-working-revision tmp-name backend)))
 	    (should (member (vc-working-revision tmp-name) '(nil "0" "1.1")))
 
 	    ;; Unregister the file.  Check working revision.
 	    (condition-case err
 		(progn
-		  (vc-test--unregister-function backend tmp-name)
+                  (vc-test--unregister-function backend tmp-name)
 
 		  ;; nil: Git RCS
 		  ;; "0": Bzr Hg
 		  ;; unsupported: CVS Mtn SCCS SRC SVN
                   (message
                    "vc-working-revision5 %s" (vc-working-revision tmp-name))
-		  (should (eq (vc-working-revision tmp-name)
-			      (vc-working-revision tmp-name backend)))
+                  ;; (should (eq (vc-working-revision tmp-name)
+                  ;;             (vc-working-revision tmp-name backend)))
 		  (should (member (vc-working-revision tmp-name) '(nil "0"))))
 	      (vc-not-supported (message "vc-working-revision5 unsupported"))
               (t (signal (car err) (cdr err))))))

Re: Docstrings and manuals

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, rgm@gnu.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 17 Apr 2016 22:59:21 +0300
> 
> > IOW, writing a good manual is still a human activity that cannot be
> > easily automated.  Ideally, doc strings should be phrased like
> > reference material, covering all the traits as tersely as possible,
> > while the manual should provide an easier reading with more continuity
> > text and even some explanations why things are the way they are.  At
> > least IMO.
> 
> That sounds fine, and maybe this way is best for the more complex 
> subjects, but in general I'd prefer to see docstrings also written with 
> readability in mind.

This would make them too long ad wordy, something we'd like to avoid,
I think.

Re: Docstrings and manuals

[Eli Zaretskii]

> Cc: michael.albinus@gmx.de, rgm@gnu.org, emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sun, 17 Apr 2016 23:22:59 +0300
> 
> Generic Functions, on the other hand, may be indicative of the problem 
> that I've brought up: the descriptions of `cl-defgeneric' and 
> `cl-defmethod' and much longer in the manual than their docstrings 
> (especially the latter one). I see no particular reason for them to be 
> different in this case.

You are looking at the documentation of the specific symbols, while I
invited you to look at the stuff that isn't.  To my mind, that
additional stuff is important for the presentation of the subject as a
whole, and it has no place in the doc strings.

Re: Docstrings and manuals

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > If you mean that mapatoms' doc string is too terse and omits some
  > > details it shouldn't, I agree.  Otherwise, I'm not sure what you mean;
  > > please elaborate.

  > That someone saw fit to put "Then it returns ‘nil’" into the manual when 
  > it's not in the docstring. And that it's easy to make such a mistake.

That's not necessarily a flaw.  The fact that mapatoms returns nil is
not very important for using it.  Maybe it is worth mentioning in the
manual but not important enough for the doc string.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/18/2016 11:38 AM, Richard Stallman wrote:

> That's not necessarily a flaw.  The fact that mapatoms returns nil is
> not very important for using it.  Maybe it is worth mentioning in the
> manual but not important enough for the doc string.

How come it can be more important to mention in the manual? Returning 
nil is either a part of the function's contract, or not.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> After applying your patch, vc-tests.el fails for me in vc-test-src02-state
>
> How do you install src? Do you build it from source?

I've checked it out from <https://gitorious.org/src-vcs/src-vcs.git>.
See the README / INSTALL files.

>> and vc-test-rcs03-working-revision.
>
> This one (failure in vc-checkout-model5) seems to be a real bug
> uncovered by using vc-file-clearprops. I suppose we should add a FIXME
> there and skip that check for RCS.

I'm also thinking about how to solve it (in master). vc-checkout-model
knows only three answers, implicit, locking and announce. I will ask on
emacs-devel, what is appropriate for RCS, and fix it then.

>> As said, in the master branch I would prefer to add a vc-unregister
>> function. Then we won't need this anymore.
>
> We'll need this code either way. Where the function resides and how it
> is called is less important to me.

For the time being, I have applied this part of your patch locally (not
pushed yet to savannah).

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> I've committed this to emacs-25. Plus commenting the now failing checks
>> in vc-tests.el.
>
> We should also consider reverting the changes to vc-rcs-unregister and
> vc-sccs-working-revision. They don't seem particularly necessary (the
> tests pass without them).

As you know I'm conservative with the emacs-25 branch. Is it absolutely
necessary that we do this?

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/18/2016 03:27 PM, Michael Albinus wrote:

> I've checked it out from <https://gitorious.org/src-vcs/src-vcs.git>.
> See the README / INSTALL files.

OK, thanks.

>  I'm also thinking about how to solve it (in master). vc-checkout-model
> knows only three answers, implicit, locking and announce. I will ask on
> emacs-devel, what is appropriate for RCS, and fix it then.

Sorry, the failure was in working-revision5, I think. So it's unrelated 
to checkout-model.

Anyway, I don't see it with the latest patch.

>>> As said, in the master branch I would prefer to add a vc-unregister
>>> function. Then we won't need this anymore.
>>
>> We'll need this code either way. Where the function resides and how it
>> is called is less important to me.
>
> For the time being, I have applied this part of your patch locally (not
> pushed yet to savannah).

Just the usage of vc-file-clearprops? Why?

Anyway, I've sent a better patch yesterday.

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/18/2016 03:28 PM, Michael Albinus wrote:

> As you know I'm conservative with the emacs-25 branch. Is it absolutely
> necessary that we do this?

It would be a revert to the code that has been in use for a while.

Nothing absolutely necessary about that, but it might be prudent to 
revert the changes made without clear understanding why they should be 
made. Please correct me on this if I'm wrong, of course.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>  I'm also thinking about how to solve it (in master). vc-checkout-model
>> knows only three answers, implicit, locking and announce. I will ask on
>> emacs-devel, what is appropriate for RCS, and fix it then.
>
> Sorry, the failure was in working-revision5, I think. So it's
> unrelated to checkout-model.

Ah, sorry. But there is also an error with RCS and checkout-model.
That's why Glenn has marked vc-test-rcs04-checkout-model as expected to
fail a while ago.

>> For the time being, I have applied this part of your patch locally (not
>> pushed yet to savannah).
>
> Just the usage of vc-file-clearprops? Why?

Also the use of vc-call-backend. I didn't know that.

> Anyway, I've sent a better patch yesterday.

Yes, it's on my todo. I'm handling easy-to-answer messages first :-)

Best regards, Michael.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 04/18/2016 03:28 PM, Michael Albinus wrote:
>
>> As you know I'm conservative with the emacs-25 branch. Is it absolutely
>> necessary that we do this?
>
> It would be a revert to the code that has been in use for a while.

The current code has been in use for 11 months. Beside this bug report,
nobody has worried about that. And the problems this bug report speaks
about are solved, I believe.

> Nothing absolutely necessary about that, but it might be prudent to
> revert the changes made without clear understanding why they should be
> made. Please correct me on this if I'm wrong, of course.

Again, it is a matter of not touching code so close to a release. 35+
years experience in software development tell me so. Often enough I've
changed just one single line, because "it is obvious it cannot be
wrong". Haha.

Best regards, Michael.

Re: Docstrings and manuals

[Phillip Lord]

Eli Zaretskii <eliz@gnu.org> writes:
>> I'm not arguing in favor of leaving mistakes in the manual. But I think 
>> it should be strictly a derivative work. I.e. the docstrings must 
>> contain the complete information (if maybe presented in a terse 
>> fashion), and the manual could rephrase that, only to make it more 
>> accessible (but not more informative).
>
> Once you allow rephrasing, you already allow deviation.  And there
> really is no way around allowing it, because a manual that includes
> only the doc strings with a minimum "glue" is much less palatable.
> You can look at Guile as one example; GnuTLS is another.  Such manuals
> are much less useful, IME, than what we have in Emacs.
>
> One thing that you will never find in doc strings is a discussion of
> merits and demerits of different ways of solving some problem,
> especially if such a discussion requires to jump back and forth
> between these ways, in order to make some point.
>
> IOW, writing a good manual is still a human activity that cannot be
> easily automated.  Ideally, doc strings should be phrased like
> reference material, covering all the traits as tersely as possible,
> while the manual should provide an easier reading with more continuity
> text and even some explanations why things are the way they are.  At
> least IMO.


I think that the two should have a different purpose. I would like more
"tutorial" information in the manual (in the sense of "here is a bit of
code that does something with the functions we are discussing at this
point"), while the docstrings should be the main documentation.

For myself, I think, being able to get to the docstring easily (probably
with a tooltip) from any occurrence of a function or variable in the
manual would be excellent.

Phil

bug#20637: incompatible, undocumented change to vc-working-revision

[Dmitry Gutov]

On 04/18/2016 03:53 PM, Michael Albinus wrote:

>> It would be a revert to the code that has been in use for a while.
>
> The current code has been in use for 11 months. Beside this bug report,
> nobody has worried about that. And the problems this bug report speaks
> about are solved, I believe.

They are.

>> Nothing absolutely necessary about that, but it might be prudent to
>> revert the changes made without clear understanding why they should be
>> made. Please correct me on this if I'm wrong, of course.
>
> Again, it is a matter of not touching code so close to a release. 35+
> years experience in software development tell me so. Often enough I've
> changed just one single line, because "it is obvious it cannot be
> wrong". Haha.

Fair enough. We can revert them on master, though.

bug#20637: incompatible, undocumented change to vc-working-revision

[Michael Albinus]

Version: 25.1

Dmitry Gutov <dgutov@yandex.ru> writes:

>> The current code has been in use for 11 months. Beside this bug report,
>> nobody has worried about that. And the problems this bug report speaks
>> about are solved, I believe.
>
> They are.

So I'm closing this bug.

Further discussion shall start on emacs-devel in a *new* thread. This bug
report has served its purpose.

Best regards, Michael.

vc-state and unregistered (was: bug#20637: incompatible, undocumented change to vc-working-revision)

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

> I don't really see a point in returning `unregistered' from
> `vc-state'. When would the caller treat it differently from nil? And
> returning nil seems like an easier choice, implementation-wise, and
> well as a more conservative one from the backward compatibility
> perspective.

I've checked a little bit more. If we use nil instead of unregistered as
result from vc-state, we cannot use file properties for this function
anymore. nil is meant as "no property set". This would be a regression.

Best regards, Michael.

Re: Docstrings and manuals

[Marcin Borkowski]

On 2016-04-18, at 12:55, Phillip Lord <phillip.lord@russet.org.uk> wrote:

> For myself, I think, being able to get to the docstring easily (probably
> with a tooltip) from any occurrence of a function or variable in the
> manual would be excellent.

Oleh's Lispy does (almost) that with C-1.

> Phil

Hth,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University

Re: Docstrings and manuals

[Stefan Monnier]

> I think that the two should have a different purpose. I would like more
> "tutorial" information in the manual (in the sense of "here is a bit of
> code that does something with the functions we are discussing at this
> point"), while the docstrings should be the main documentation.
> For myself, I think, being able to get to the docstring easily (probably
> with a tooltip) from any occurrence of a function or variable in the
> manual would be excellent.

FWIW, I like this idea.  I think we could reduce the amount of
"reference info" in the manual (a part that's already available in the
docstrings) by referring to the docstring instead, and instead increase
the amount of explanation giving tips/examples about how to use it.

This said, the manual was also intended to be used as
a plain-old-burnable book.  So if we decide to reduce the amount of
"reference info" by referring to docstrings, that means we mostly give
up on using lispref as a book and instead we force its use from
within Emacs.  I think it's an acceptable choice, but I wouldn't be
surprised to hear other opinions.


        Stefan

Re: Docstrings and manuals

[Marcin Borkowski]

On 2016-04-18, at 15:47, Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> This said, the manual was also intended to be used as
> a plain-old-burnable book.  So if we decide to reduce the amount of
> "reference info" by referring to docstrings, that means we mostly give
> up on using lispref as a book and instead we force its use from
> within Emacs.  I think it's an acceptable choice, but I wouldn't be
> surprised to hear other opinions.

Like mine.  Back in the old days, when I was a student and had a lot of
time, I read most of the Emacs manual.  (Yes, I did it on my computer,
but today I'd probably do it on my ebook reader.  And I have the Elisp
reference on said reader, and I used to read it while commuting some
time ago.  Referring to docstrings would only aggravate me.)

>         Stefan

Best,

-- 
Marcin Borkowski
http://octd.wmi.amu.edu.pl/en/Marcin_Borkowski
Faculty of Mathematics and Computer Science
Adam Mickiewicz University

bug#20637: incompatible, undocumented change to vc-working-revision

[John Wiegley]

[-- Attachment #1: Type: text/plain, Size: 509 bytes --]

>>>>> Michael Albinus <michael.albinus@gmx.de> writes:

> Again, it is a matter of not touching code so close to a release. 35+ years
> experience in software development tell me so. Often enough I've changed
> just one single line, because "it is obvious it cannot be wrong". Haha.

I simply couldn't agree with you more on this one, Michael. :)

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 629 bytes --]

Re: Docstrings and manuals

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Mon, 18 Apr 2016 11:47:17 -0400
> 
> > I think that the two should have a different purpose. I would like more
> > "tutorial" information in the manual (in the sense of "here is a bit of
> > code that does something with the functions we are discussing at this
> > point"), while the docstrings should be the main documentation.
> > For myself, I think, being able to get to the docstring easily (probably
> > with a tooltip) from any occurrence of a function or variable in the
> > manual would be excellent.
> 
> FWIW, I like this idea.  I think we could reduce the amount of
> "reference info" in the manual (a part that's already available in the
> docstrings) by referring to the docstring instead, and instead increase
> the amount of explanation giving tips/examples about how to use it.

Then the manual will be a very awkward reading, even on-line.

To say nothing of the fact that the current doc strings are usually
much worse than the documentation in the manual.

IOW, let's first make sure every changeset is accompanied by good
documentation, and only then consider such significant changes.

Re: Docstrings and manuals

[Stefan Monnier]

>> FWIW, I like this idea.  I think we could reduce the amount of
>> "reference info" in the manual (a part that's already available in the
>> docstrings) by referring to the docstring instead, and instead increase
>> the amount of explanation giving tips/examples about how to use it.
> Then the manual will be a very awkward reading, even on-line.

Maybe we could just change the manual so it's not as
complete-and-definitive, but it's still self-standing (tho with easy
ways to get more details via docstrings).

> To say nothing of the fact that the current doc strings are usually
> much worse than the documentation in the manual.

Docstrings should (ideally) always be as complete as the manual, in the
sense that the actual info is there.  In practice, I find it's usually
the case.  But it's often present in a much rougher shape, indeed, so
you can only figure out that info after reading the manual to figure out
what the docstring really means.

> IOW, let's first make sure every changeset is accompanied by good
> documentation, and only then consider such significant changes.

Agreed,


        Stefan

Re: Docstrings and manuals

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Mon, 18 Apr 2016 15:33:20 -0400
> 
> >> FWIW, I like this idea.  I think we could reduce the amount of
> >> "reference info" in the manual (a part that's already available in the
> >> docstrings) by referring to the docstring instead, and instead increase
> >> the amount of explanation giving tips/examples about how to use it.
> > Then the manual will be a very awkward reading, even on-line.
> 
> Maybe we could just change the manual so it's not as
> complete-and-definitive, but it's still self-standing (tho with easy
> ways to get more details via docstrings).

I think going that way would need a way of inserting doc strings into
the displayed manual, which will require some infrastructure first.
Without having reference material, the manual won't be able to be
self-sufficient.

> > To say nothing of the fact that the current doc strings are usually
> > much worse than the documentation in the manual.
> 
> Docstrings should (ideally) always be as complete as the manual, in the
> sense that the actual info is there.  In practice, I find it's usually
> the case.  But it's often present in a much rougher shape, indeed, so
> you can only figure out that info after reading the manual to figure out
> what the docstring really means.

Indeed, in my experience I frequently need to read the manual to make
sense of the doc string.

Re: vc-state and unregistered

[Dmitry Gutov]

Hi Michael,

On 04/18/2016 05:55 PM, Michael Albinus wrote:

> I've checked a little bit more. If we use nil instead of unregistered as
> result from vc-state, we cannot use file properties for this function
> anymore. nil is meant as "no property set". This would be a regression.

Regression how? We already do exactly that: return `nil' in vc-state to 
mean "unregistered or unknown", and have been doing this for a while.

This is not a significant problem, for two reasons:

- The "unregistered" status is tracked by a different property: 
unregistered files have no backend associated with them. vc-registered 
stores `none' as the value of the file's backend in this case. So we 
don't have vc-git-registered called again and again, even for 
unregistered files.

- If the above scheme is deemed insufficient, we could use the same 
approach for the `vc-state' property.

Re: Docstrings and manuals

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > How come it can be more important to mention in the manual? Returning 
  > nil is either a part of the function's contract, or not.

We don't divide up the function's behaviors in that all-or-nothing way.

Since the return value is documented in the manual, it is something
we would not change without a good reason.  However, that doesn't mean
it needs to be stated in the doc string.

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: Docstrings and manuals

[Dmitry Gutov]

On 04/19/2016 03:25 AM, Richard Stallman wrote:

> Since the return value is documented in the manual, it is something
> we would not change without a good reason.  However, that doesn't mean
> it needs to be stated in the doc string.

That sounds silly to me. At the very least, not mentioning a behavior in 
the docstring makes it easier to violate.

Docstrings are changes together with the code, but we allow deferring 
manual updates.

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

>> I've checked a little bit more. If we use nil instead of unregistered as
>> result from vc-state, we cannot use file properties for this function
>> anymore. nil is meant as "no property set". This would be a regression.
>
> Regression how? We already do exactly that: return `nil' in vc-state
> to mean "unregistered or unknown", and have been doing this for a
> while.

I don't mean this. The problem is how vc caches values. It uses file
properties, and in case `vc-file-getprop' returns nil, it assumes the
property is not cached. Consequently, we can never cache the value nil
for the file property `vc-state'. That is a regression, compared with
caching the value `unregistered'.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/19/2016 11:10 AM, Michael Albinus wrote:

> I don't mean this. The problem is how vc caches values. It uses file
> properties, and in case `vc-file-getprop' returns nil, it assumes the
> property is not cached. Consequently, we can never cache the value nil
> for the file property `vc-state'.

Which part of this hasn't been addressed in my previous email?

What are the practical problems that you anticipate?

> That is a regression, compared with
> caching the value `unregistered'.

We've never returned that value anyway. And as such, never cached it. So 
it wouldn't be a regression.

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 04/19/2016 11:10 AM, Michael Albinus wrote:
>
>> I don't mean this. The problem is how vc caches values. It uses file
>> properties, and in case `vc-file-getprop' returns nil, it assumes the
>> property is not cached. Consequently, we can never cache the value nil
>> for the file property `vc-state'.
>
> Which part of this hasn't been addressed in my previous email?
>
> What are the practical problems that you anticipate?

The very first line in `vc-state' doesn't make sense for unregistered
files:

  (or (vc-file-getprop file 'vc-state)

Even if we know that a file is unregistered, and we have set the file
property to nil accordingly, This information isn't used. For
unregistered files, the cached value isn't used, and the file's state is
computed again.

IOW: if vc-file-getprop returns nil, we don't know what this
means. Either "don't know", because the state hasn't been computed
yet. Or it means "unregistered".

My problem is *not* what vc-state returns to the caller. Here I could
live with a return value of nil, meaning unregistered.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/19/2016 11:31 AM, Michael Albinus wrote:

> The very first line in `vc-state' doesn't make sense for unregistered
> files:
>
>   (or (vc-file-getprop file 'vc-state)
>
> Even if we know that a file is unregistered, and we have set the file
> property to nil accordingly, This information isn't used. For
> unregistered files, the cached value isn't used, and the file's state is
> computed again.

That's not really true, try stepping through the function with Edebug, 
and see for yourself. It does go farther than the first line, but it 
doesn't call `vc-git-state' or `vc-git-registered' every time.

> IOW: if vc-file-getprop returns nil, we don't know what this
> means. Either "don't know", because the state hasn't been computed
> yet. Or it means "unregistered".

That could be fixed with using `none' instead, like `vc-backend' does. 
Which I've mentioned already.

But I'm not convinced we even need to make that effort.

Re: Docstrings and manuals

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > That sounds silly to me. At the very least, not mentioning a behavior in 
  > the docstring makes it easier to violate.

I don't know what you mean by "violate" in this specific context
(that mapatoms returns nil).

-- 
Dr Richard Stallman
President, Free Software Foundation (gnu.org, fsf.org)
Internet Hall-of-Famer (internethalloffame.org)
Skype: No way! See stallman.org/skype.html.

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

Hi Dmitry,

>> IOW: if vc-file-getprop returns nil, we don't know what this
>> means. Either "don't know", because the state hasn't been computed
>> yet. Or it means "unregistered".
>
> That could be fixed with using `none' instead, like `vc-backend'
> does. Which I've mentioned already.

That's the direction I have thought also. But in `vc-state' there is the
following comment:

  ;; Note: in Emacs 22 and older, return of nil meant the file was
  ;; unregistered.  This is potentially a source of
  ;; backward-compatibility bugs.

Since I don't know why nil has been replaced by `unregistered', it might
be a problem to use nil again as indication of unregistered files. And
we shouldn't change the interface wantonly, I believe.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 03:11 PM, Michael Albinus wrote:

>> That could be fixed with using `none' instead, like `vc-backend'
>> does. Which I've mentioned already.
>
> That's the direction I have thought also. But in `vc-state' there is the
> following comment:

The comment is about the API and which value to return, not how the 
value is cached. My sentence, quoted above, was about how to cache it.

>   ;; Note: in Emacs 22 and older, return of nil meant the file was
>   ;; unregistered.  This is potentially a source of
>   ;; backward-compatibility bugs.

...and I have no idea how to interpret this comment. In Emacs 24, *and* 
in the upcoming 25.1, nil is *still* returned for unregistered files.

> Since I don't know why nil has been replaced by `unregistered', it might
> be a problem to use nil again as indication of unregistered files.

The salient question here is *in what way* has nil been replaced by 
`unregistered'.

> And
> we shouldn't change the interface wantonly, I believe.

The actual behavior will not change. This is what I'm proposing: to put 
the current behavior of (vc-state FILE) into spec.

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> The actual behavior will not change. This is what I'm proposing: to
> put the current behavior of (vc-state FILE) into spec.

Sometimes it returns nil, sometimes unregistered. Is this really what we
want to offer as a spec?

I'm going to push my changes accumulated over the last weeks. You will
see by the comments in vc-test--state, that the situation has been
improved. Less FIXME comments. I'm not saying that I'm satisfied already ...

We still need to check the performance, as said by you (due to external
processes).

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 03:45 PM, Michael Albinus wrote:

>> The actual behavior will not change. This is what I'm proposing: to
>> put the current behavior of (vc-state FILE) into spec.
>
> Sometimes it returns nil, sometimes unregistered.

When does it return `unregistered'?

> Is this really what we
> want to offer as a spec?

Just nil, obviously.

> I'm going to push my changes accumulated over the last weeks. You will
> see by the comments in vc-test--state, that the situation has been
> improved. Less FIXME comments. I'm not saying that I'm satisfied already ...

What about my changes?

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 04/24/2016 03:45 PM, Michael Albinus wrote:
>
>>> The actual behavior will not change. This is what I'm proposing: to
>>> put the current behavior of (vc-state FILE) into spec.
>>
>> Sometimes it returns nil, sometimes unregistered.
>
> When does it return `unregistered'?

See my recent commit.

>> Is this really what we
>> want to offer as a spec?
>
> Just nil, obviously.

That's not what's specified.

>> I'm going to push my changes accumulated over the last weeks. You will
>> see by the comments in vc-test--state, that the situation has been
>> improved. Less FIXME comments. I'm not saying that I'm satisfied already ...
>
> What about my changes?

I have applied some of them. I have not applied the reversion of calling
vc-responsible-backend instead of vc-backend, because I'm not convinced
yet that this is proper.

vc-state returns unregistered now in most of the cases when it should,
as it is specified. Pls show me the bugs, I might have introduced.

And yes, I have ackmnowledged already that we must speak about performance.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 03:49 PM, Dmitry Gutov wrote:

>> I'm going to push my changes accumulated over the last weeks. You will
>> see by the comments in vc-test--state, that the situation has been
>> improved. Less FIXME comments. I'm not saying that I'm satisfied
>> already ...
>
> What about my changes?

I mean, seriously. I gave you a patch that improved the situation with 
tests without regressing performance, but avoided committing it so far 
out of courtesy.

You still haven't commented on it properly, but went ahead with 
different, incompatible changes. The very same changes we've discussed 
at length before, which lead to extra process calls.

Please avoid doing that in the future. I'm going to revert the offending 
parts.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 04:07 PM, Michael Albinus wrote:

>> When does it return `unregistered'?
>
> See my recent commit.

Could you put that into words?

>> Just nil, obviously.
>
> That's not what's specified.

Please re-read my previous messages.

>> What about my changes?
>
> I have applied some of them. I have not applied the reversion of calling
> vc-responsible-backend instead of vc-backend, because I'm not convinced
> yet that this is proper.

I don't have to convince you. Quote the opposite, see "lisp/vc/*" in 
admin/MAINTAINERS.

Or do you want to take over VC maintenance?

> vc-state returns unregistered now in most of the cases when it should,
> as it is specified. Pls show me the bugs, I might have introduced.

We've discussed your current code at length already. It's pretty much 
what was in emacs-25 previously, with some new additions (those I don't 
mind, so far).

> And yes, I have ackmnowledged already that we must speak about performance.

And yet, you've decided to disregard the previous messages on that subject.

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> I have applied some of them. I have not applied the reversion of calling
>> vc-responsible-backend instead of vc-backend, because I'm not convinced
>> yet that this is proper.
>
> I don't have to convince you. Quote the opposite, see "lisp/vc/*" in
> admin/MAINTAINERS.
>
> Or do you want to take over VC maintenance?

No. Please revert everything I've committed to vc. Remove also vc-tests.el.

I've tried to fix the bugs in vc, respecting the existing interface. If
you have other plans, go ahead.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 04:24 PM, Michael Albinus wrote:

> Please revert everything I've committed to vc. Remove also vc-tests.el.

Why would I do that?

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 04/24/2016 04:24 PM, Michael Albinus wrote:
>
>> Please revert everything I've committed to vc. Remove also vc-tests.el.
>
> Why would I do that?

'Cos you have other plans with vc. Don't count me anymore working on
vc-tests.el.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 05:03 PM, Michael Albinus wrote:

> 'Cos you have other plans with vc. Don't count me anymore working on
> vc-tests.el.

That is unfortunate. Even so, having a test suite with some level of 
coverage is very beneficial for VC's stability.

Thank you very much for getting it started.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 04:07 PM, Michael Albinus wrote:

> Pls show me the bugs, I might have introduced.

As far as bugs go, here's one: you never return nil from vc-state 
anymore. Try evaluating:

(progn
   (vc-file-clearprops buffer-file-name)
   (vc-state buffer-file-name 'CVS))

And if the goal is to always return non-nil (which would be a change to 
the documented behavior), there is an easier way to do that than 
checking (vc-registered file), like:

diff --git a/lisp/vc/vc-hooks.el b/lisp/vc/vc-hooks.el
index 0535565..5c31d20 100644
--- a/lisp/vc/vc-hooks.el
+++ b/lisp/vc/vc-hooks.el
@@ -475,11 +475,11 @@ vc-state
    ;; FIXME: New (sub)states needed (?):
    ;; - `copied' and `moved' (might be handled by `removed' and `added')
    (or (vc-file-getprop file 'vc-state)
-      (and (not (vc-registered file)) 'unregistered)
        (when (> (length file) 0)         ;Why??  --Stef
-	(setq backend (or backend (vc-responsible-backend file)))
+	(setq backend (or backend (vc-backend file)))
  	(when backend
-	  (vc-state-refresh file backend)))))
+	  (vc-state-refresh file backend)))
+      'unregistered))

  (defun vc-state-refresh (file backend)
    "Quickly recompute the `state' of FILE."

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> Pls show me the bugs, I might have introduced.
>
> As far as bugs go, here's one: you never return nil from vc-state
> anymore. Try evaluating:
>
> (progn
>   (vc-file-clearprops buffer-file-name)
>   (vc-state buffer-file-name 'CVS))

See the docstring of vc-state:

"A return of nil from this function means we have no information on the
status of this file."

I would expect this for files which are located in a directory not under
version control. None of the tests in vc-tests.el cover this case.

> And if the goal is to always return non-nil (which would be a change
> to the documented behavior), there is an easier way to do that than
> checking (vc-registered file), like:

I haven't said that my changes are perfect. But we differ in the
understanding of the API.

Best regards, Michael.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 09:13 PM, Michael Albinus wrote:

> See the docstring of vc-state:
>
> "A return of nil from this function means we have no information on the
> status of this file."
>
> I would expect this for files which are located in a directory not under
> version control. None of the tests in vc-tests.el cover this case.

And if buffer-file-name is outside of any version control, does your 
version of vc-state return nil?

Re: vc-state and unregistered

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>> See the docstring of vc-state:
>>
>> "A return of nil from this function means we have no information on the
>> status of this file."
>>
>> I would expect this for files which are located in a directory not under
>> version control. None of the tests in vc-tests.el cover this case.
>
> And if buffer-file-name is outside of any version control, does your
> version of vc-state return nil?

No. I didn't test this. As said, the tests must be improved, that's what
they are good for. And bugs must be fixed.

But for the other cases, files under version control, my last commits
have shown improvements I believe. That's why I find your reaction to
revert everything ... annoying.

Re: vc-state and unregistered

[Dmitry Gutov]

On 04/24/2016 10:41 PM, Michael Albinus wrote:

>> And if buffer-file-name is outside of any version control, does your
>> version of vc-state return nil?
>
> No. I didn't test this. As said, the tests must be improved, that's what
> they are good for. And bugs must be fixed.

We could add a separate test for that, but the simplest fix would 
require updating the existing tests as well, and then that test wouldn't 
be necessary. See the commit I pushed.

Anyway, feel free to add it. We didn't finish the discussion about 
semantics, so it didn't seem proper to me to add tests for them.

> But for the other cases, files under version control, my last commits
> have shown improvements I believe. That's why I find your reaction to
> revert everything ... annoying.

Not everything, just the parts that conflicted with the patch I've sent 
previously. Which I had to recreate.

So in the interest of speed, I've also removed all

   (should (eq (vc-state tmp-name) (vc-state tmp-name backend)))

checks. They aren't essential, and finding which of them are failing 
exactly is too time-consuming.

Let's only add each of them back after they pass. I'd also like to have 
the different steps of each scenario as separate tests, so that ERT can 
tell us which failed.