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

[Dmitry Gutov <dgutov@yandex.ru>]

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.