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

[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

[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

[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

[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

[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

[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]

[-- 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

[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: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

[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

[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

[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.

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

[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

[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

[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))))))

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:

> 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.

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/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

[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: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:

> 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.

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.

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 --]