Patches for Emacs 25.2

Patches for Emacs 25.2

[Michael Albinus]

Hi,

yesterday, I've shown two candidate patches for Tramp (bug#24698,
bug#24478). Given, that there will be an Emacs 25.2 soon, I'm undecided
whether they shall go into the emacs-25 branch, once confirmed by the
bug reporters. Any policy for that? Will there be a test period for
Emacs 25.2?

One policy could be, that only fixes shall be pushed to the emacs-25
branch for bugs which have been marked as blocking bug#21966. In this
case, what would be the policy to add a bug there? bug#24478 belongs to
that list already, bug#24698 doesn't.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Tue, 18 Oct 2016 09:09:55 +0200
> 
> yesterday, I've shown two candidate patches for Tramp (bug#24698,
> bug#24478). Given, that there will be an Emacs 25.2 soon, I'm undecided
> whether they shall go into the emacs-25 branch, once confirmed by the
> bug reporters. Any policy for that?

I described what I thought should be the policy here:

  http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00007.html

to which no one objected.

Bug#24478 is listed as blocking 25.2 release, so its fix should
definitely go to the emacs-25 branch.  But I understand you are still
waiting for Glenn to tell whether the solution is correct?

Not sure about bug#24698.  Does it fit the criteria described in the
above message?

> Will there be a test period for Emacs 25.2?

Yes, but I'd like it to be very short.  If possible, I'd like to start
the 25.2 pretest in a couple of weeks.

> One policy could be, that only fixes shall be pushed to the emacs-25
> branch for bugs which have been marked as blocking bug#21966. In this
> case, what would be the policy to add a bug there? bug#24478 belongs to
> that list already, bug#24698 doesn't.

See above.  Not every bug that fits the criteria is listed as
blocking, primarily because we simply failed to mark them as blocking.

Thanks.

Re: Patches for Emacs 25.2

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> I described what I thought should be the policy here:
>
>   http://lists.gnu.org/archive/html/emacs-devel/2016-10/msg00007.html
>
> to which no one objected.

Ahh, thanks. I read this two weeks ago, but I happily forgot meanwhile :-(

> Bug#24478 is listed as blocking 25.2 release, so its fix should
> definitely go to the emacs-25 branch.  But I understand you are still
> waiting for Glenn to tell whether the solution is correct?

Yes, I'm waiting for Glenn. As usual I will push the fix after one week
w/o any response.

> Not sure about bug#24698.  Does it fit the criteria described in the
> above message?

No. But the OP said "As a result, Tramp hangs while connecting to NetBSD
host." To me, it looks also like a valid reason to add this to Emacs 25.2.

> Thanks.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

Tramp-related commit 2c0506173d92dd9d6de409a045668c6b5cf1fcef that fixes 
a problem introduced in 25.1 by a fix for #23076 seems to comply with 
that policy. May I propose it for inclusion into 25.2?

-- 
Best Regards,
Nikolay Kudryavtsev

Re: Patches for Emacs 25.2

[Michael Albinus]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> Tramp-related commit 2c0506173d92dd9d6de409a045668c6b5cf1fcef that
> fixes a problem introduced in 25.1 by a fix for #23076 seems to comply
> with that policy. May I propose it for inclusion into 25.2?

Well, bug#23076 is marked as wishlist. And the commit message for
2c0506173d92dd9d6de409a045668c6b5cf1fcef is rather long, pointing to a
new Emacs feature. 13 files are modified. This is far away from the
policy Eli has published.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Glenn Morris <rgm@gnu.org>,  emacs-devel@gnu.org
> Date: Tue, 18 Oct 2016 09:47:55 +0200
> 
> > Not sure about bug#24698.  Does it fit the criteria described in the
> > above message?
> 
> No. But the OP said "As a result, Tramp hangs while connecting to NetBSD
> host." To me, it looks also like a valid reason to add this to Emacs 25.2.

Can you tell when (in which Emacs version) was this problem
introduced?

Thanks.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Date: Tue, 18 Oct 2016 10:25:34 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:
> 
> > Tramp-related commit 2c0506173d92dd9d6de409a045668c6b5cf1fcef that
> > fixes a problem introduced in 25.1 by a fix for #23076 seems to comply
> > with that policy. May I propose it for inclusion into 25.2?
> 
> Well, bug#23076 is marked as wishlist. And the commit message for
> 2c0506173d92dd9d6de409a045668c6b5cf1fcef is rather long, pointing to a
> new Emacs feature. 13 files are modified. This is far away from the
> policy Eli has published.

Yes, that was my impression as well.

If there's a safer fix, perhaps in only a subset of use cases, I'm
willing to consider it for the release branch.

Re: Patches for Emacs 25.2

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> No. But the OP said "As a result, Tramp hangs while connecting to NetBSD
>> host." To me, it looks also like a valid reason to add this to Emacs 25.2.
>
> Can you tell when (in which Emacs version) was this problem
> introduced?

Well, it started with bug#17295, which set remote $HISTFILE to
"/dev/null". Released with Emacs 24.4.

Then there were bug reports 19731, 21697 and 23041, which reported a bash
bug when setting this. So the new defcustom `tramp-histfile-override'
was introduced in Emacs 25.1.

With bug#20446, Glenn reported a new problem with the previous fix. His
bash history was nuked. So I've committed another fix, also for Emacs 25.1.

And now we have bug#24478, also purely due to a remote bash. Given this
history, a fix shall go to Emacs 25.2.

And I pray, that this will be the final fix. Likely it won't help, I'm a
nonbeliever.

> Thanks.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: rgm@gnu.org,  emacs-devel@gnu.org
> Date: Tue, 18 Oct 2016 11:55:55 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> No. But the OP said "As a result, Tramp hangs while connecting to NetBSD
> >> host." To me, it looks also like a valid reason to add this to Emacs 25.2.
> >
> > Can you tell when (in which Emacs version) was this problem
> > introduced?
> 
> Well, it started with bug#17295, which set remote $HISTFILE to
> "/dev/null". Released with Emacs 24.4.
> 
> Then there were bug reports 19731, 21697 and 23041, which reported a bash
> bug when setting this. So the new defcustom `tramp-histfile-override'
> was introduced in Emacs 25.1.
> 
> With bug#20446, Glenn reported a new problem with the previous fix. His
> bash history was nuked. So I've committed another fix, also for Emacs 25.1.
> 
> And now we have bug#24478, also purely due to a remote bash. Given this
> history, a fix shall go to Emacs 25.2.

It sounds like you are telling me the history of bug#24478, which I
have no doubt should be fixed in 25.2.  I asked about bug#24698, which
is about TAB expansion, AFAIU.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

bug#23076 was wishlist, that's true. And it's fixed in 25.1 already. It's just that the fix introduces a separate problem breaking tramp git commits. That problem is NOT a wishlist. As for modified - the largest modified part in that commit is tramp-tests the rest are trivial changes to ob-core.el, vc-hooks.el, gud.el. If it's still too large, we can just take only that new tramp function, vc-git fix part(that depends on it) and use that. Even if the function is broken somehow, it would not break anything that is not already broken(tramp vc git commits).

-- 
Best Regards,
Nikolay Kudryavtsev

Re: Patches for Emacs 25.2

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> It sounds like you are telling me the history of bug#24478, which I
> have no doubt should be fixed in 25.2.  I asked about bug#24698, which
> is about TAB expansion, AFAIU.

Indeed, sorry. I shouldn't work on several topics in parallel; I'm not
multi-tasking capable :-(

Starting again: "stty tab0" was added back in July 2015 (commit
8e03731cb9083330939b2c9b2d3318f32e93e41d), released for the first time
with Emacs 25.1. No bug report available, very likely I'm the only one
who uses remote HP-UX machines.

bug#24698 reports compatibility problems with *BSD, which I have tried
to fix. So it classifies as patch worth for Emacs 25.2, according to
your criteria.

It is a pity, that nobody did report the problem for over a year
pretesting Emacs 25.1.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: emacs-devel@gnu.org
> Date: Tue, 18 Oct 2016 14:05:46 +0300
> 
> bug#23076 was wishlist, that's true. And it's fixed in 25.1 already. It's just that the fix introduces a separate problem breaking tramp git commits. That problem is NOT a wishlist. As for modified - the largest modified part in that commit is tramp-tests the rest are trivial changes to ob-core.el, vc-hooks.el, gud.el. If it's still too large, we can just take only that new tramp function, vc-git fix part(that depends on it) and use that. Even if the function is broken somehow, it would not break anything that is not already broken(tramp vc git commits).

The changeset adds 2 new functions, adds 2 new handlers, and uses
these handlers in several files.  It also adds a defcustom, which
already makes it something other than just a bugfix, because it has
clear user-visible changes.  The added code will most probably have
non-trivial consequences, given what it does and how many problems we
had in that area.

So I don't really see how you can claim that this changeset is safe
for 25.2.

Once again, if someone suggests a safer change that fixes the bug
without introducing new features, I will consider it for the release
branch.  But that changeset in its present form is out of the question
for emacs-25, and Michael feels the same.  Sorry.  (People who must
have this change can always patch their local Tramp.)

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: rgm@gnu.org,  emacs-devel@gnu.org
> Date: Tue, 18 Oct 2016 13:24:08 +0200
> 
> Starting again: "stty tab0" was added back in July 2015 (commit
> 8e03731cb9083330939b2c9b2d3318f32e93e41d), released for the first time
> with Emacs 25.1. No bug report available, very likely I'm the only one
> who uses remote HP-UX machines.
> 
> bug#24698 reports compatibility problems with *BSD, which I have tried
> to fix. So it classifies as patch worth for Emacs 25.2, according to
> your criteria.

Indeed, it does.  So please push to the release branch.

> It is a pity, that nobody did report the problem for over a year
> pretesting Emacs 25.1.

Stuff happens.

Thanks.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

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

Ok, would my original patch for it 
<https://debbugs.gnu.org/cgi/bugreport.cgi?att=1;bug=23076;filename=remote-vc-git-windows.patch;msg=53> 
count as a safer way? It uses internal tramp function, as discussed 
before, but the change itself is local to vc-git.

-- 
Best Regards,
Nikolay Kudryavtsev


[-- Attachment #2: Type: text/html, Size: 552 bytes --]

Re: Patches for Emacs 25.2

[Michael Albinus]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> Ok, would my original patch for it count as a safer way? It uses
> internal tramp function, as discussed before, but the change itself is
> local to vc-git.

I don't decide whether such a patch shall be applied; it shall be
decided by the vc maintainer. But in case of, the patch could be written
shorter:

--8<---------------cut here---------------start------------->8---
*** /usr/local/src/emacs-25/lisp/vc/vc-git.el.~12da149670a40c6d6c1bc107e8c29d7fcdcf7824~	2016-10-18 14:29:18.559722705 +0200
--- /usr/local/src/emacs-25/lisp/vc/vc-git.el	2016-10-18 14:29:18.691725102 +0200
***************
*** 705,718 ****
            ;; arguments must be in the system codepage, and therefore
            ;; might not support the non-ASCII characters in the log
            ;; message.
!           (if (eq system-type 'windows-nt) (make-temp-file "git-msg"))))
      (cl-flet ((boolean-arg-fn
                 (argument)
                 (lambda (value) (when (equal value "yes") (list argument)))))
        ;; When operating on the whole tree, better pass "-a" than ".", since "."
        ;; fails when we're committing a merge.
        (apply 'vc-git-command nil 0 (if only files)
!              (nconc (if msg-file (list "commit" "-F" msg-file)
                        (list "commit" "-m"))
                      (let ((args
                             (log-edit-extract-headers
--- 705,727 ----
            ;; arguments must be in the system codepage, and therefore
            ;; might not support the non-ASCII characters in the log
            ;; message.
!           (if (eq system-type 'windows-nt)
!               (if (file-remote-p file1)
! 		  (with-parsed-tramp-file-name file1 nil
!                     (tramp-make-tramp-file-name
! 		     method user host
! 		     (tramp-make-tramp-temp-file v)))
!                   (make-temp-file "git-msg")))))
      (cl-flet ((boolean-arg-fn
                 (argument)
                 (lambda (value) (when (equal value "yes") (list argument)))))
        ;; When operating on the whole tree, better pass "-a" than ".", since "."
        ;; fails when we're committing a merge.
        (apply 'vc-git-command nil 0 (if only files)
!              (nconc (if msg-file
!                         (list
! 			 "commit" "-F"
! 			 (or (file-remote-p msg-file 'localname)  msg-file))
                        (list "commit" "-m"))
                      (let ((args
                             (log-edit-extract-headers
--8<---------------cut here---------------end--------------->8---

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org, Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 18 Oct 2016 14:32:07 +0200
> 
> Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:
> 
> > Ok, would my original patch for it count as a safer way? It uses
> > internal tramp function, as discussed before, but the change itself is
> > local to vc-git.
> 
> I don't decide whether such a patch shall be applied; it shall be
> decided by the vc maintainer. But in case of, the patch could be written
> shorter:

Thanks.  I'm fine with for emacs-25 this if Dmitry approves.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

Hi Michael,

On 18.10.2016 15:32, Michael Albinus wrote:

> I don't decide whether such a patch shall be applied; it shall be
> decided by the vc maintainer. But in case of, the patch could be written
> shorter:

Thanks. I don't like it (for obvious reason you mentioned yourself), but 
since it's emacs-25-only, it should be fine, with the following minor 
concerns:

> !           (if (eq system-type 'windows-nt)
> !               (if (file-remote-p file1)
> ! 		  (with-parsed-tramp-file-name file1 nil
> !                     (tramp-make-tramp-file-name
> ! 		     method user host
> ! 		     (tramp-make-tramp-temp-file v)))
> !                   (make-temp-file "git-msg")))))

Please do not introduce tabs here.

> !              (nconc (if msg-file
> !                         (list
> ! 			 "commit" "-F"
> ! 			 (or (file-remote-p msg-file 'localname)  msg-file))

Can't we move the (file-remote-p ... 'localname) call to the first hunk 
here? Then the patch will only change one place, and we get rid of the 
added `or' here.

Re: Patches for Emacs 25.2

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> Hi Michael,

Hi Dmitry,

>> !              (nconc (if msg-file
>> !                         (list
>> ! 			 "commit" "-F"
>> ! 			 (or (file-remote-p msg-file 'localname)  msg-file))
>
> Can't we move the (file-remote-p ... 'localname) call to the first
> hunk here? Then the patch will only change one place, and we get rid
> of the added `or' here.

I let it to Nikolay. However, given that this construct

  (or (file-remote-p FILE 'localname) FILE)

happens several times in the lisp sources, I wonder whether we shall
give it an own defmacro or defsubst (in master, of course).

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 07.11.2016 11:01, Michael Albinus wrote:

>> Can't we move the (file-remote-p ... 'localname) call to the first
>> hunk here? Then the patch will only change one place, and we get rid
>> of the added `or' here.
>
> I let it to Nikolay.

Not sure I understand.

> However, given that this construct
>
>   (or (file-remote-p FILE 'localname) FILE)
>
> happens several times in the lisp sources, I wonder whether we shall
> give it an own defmacro or defsubst (in master, of course).

I have no strong opinion either way, but your commit in master fixing 
this problem does not have this construct.

Re: Patches for Emacs 25.2

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 07.11.2016 11:01, Michael Albinus wrote:
>
>>> Can't we move the (file-remote-p ... 'localname) call to the first
>>> hunk here? Then the patch will only change one place, and we get rid
>>> of the added `or' here.
>>
>> I let it to Nikolay.
>
> Not sure I understand.

The patch for the emacs-25 branch is proposed by Nikolay.

>> However, given that this construct
>>
>>   (or (file-remote-p FILE 'localname) FILE)
>>
>> happens several times in the lisp sources, I wonder whether we shall
>> give it an own defmacro or defsubst (in master, of course).
>
> I have no strong opinion either way, but your commit in master fixing
> this problem does not have this construct.

Sure. It was a rather general comment, given that we find this construct
under .../emacs/lisp for at least 11 times, and under .../elpa 4 times.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 07.11.2016 12:31, Michael Albinus wrote:

>>>> Can't we move the (file-remote-p ... 'localname) call to the first
>>>> hunk here? Then the patch will only change one place, and we get rid
>>>> of the added `or' here.
>>>
>>> I let it to Nikolay.
>>
>> Not sure I understand.
>
> The patch for the emacs-25 branch is proposed by Nikolay.

Yes. And my comment applies to both it and your modified version.

>>> However, given that this construct
>>>
>>>   (or (file-remote-p FILE 'localname) FILE)
>>>
>>> happens several times in the lisp sources, I wonder whether we shall
>>> give it an own defmacro or defsubst (in master, of course).
>>
>> I have no strong opinion either way, but your commit in master fixing
>> this problem does not have this construct.
>
> Sure. It was a rather general comment, given that we find this construct
> under .../emacs/lisp for at least 11 times, and under .../elpa 4 times.

I wouldn't mind such a function if we find a good name for it. No point 
in making it a macro or a defsubst, I think.

Re: Patches for Emacs 25.2

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>>> However, given that this construct
>>>>
>>>>   (or (file-remote-p FILE 'localname) FILE)
>>>>
>>>> happens several times in the lisp sources, I wonder whether we shall
>>>> give it an own defmacro or defsubst (in master, of course).
>>>
>>> I have no strong opinion either way, but your commit in master fixing
>>> this problem does not have this construct.
>>
>> Sure. It was a rather general comment, given that we find this construct
>> under .../emacs/lisp for at least 11 times, and under .../elpa 4 times.
>
> I wouldn't mind such a function if we find a good name for it. No
> point in making it a macro or a defsubst, I think.

`file-localname'?

Best regards, Michael.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 07.11.2016 12:58, Michael Albinus wrote:

>> I wouldn't mind such a function if we find a good name for it. No
>> point in making it a macro or a defsubst, I think.
>
> `file-localname'?

Fine by me. Although the code it would be replacing is pretty short 
anyway, so there's no hurry about introducing the function.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 07.11.2016 12:39, Dmitry Gutov wrote:
> On 07.11.2016 12:31, Michael Albinus wrote:
>
>>>>> Can't we move the (file-remote-p ... 'localname) call to the first
>>>>> hunk here? Then the patch will only change one place, and we get rid
>>>>> of the added `or' here.
>>>>
>>>> I let it to Nikolay.
>>>
>>> Not sure I understand.
>>
>> The patch for the emacs-25 branch is proposed by Nikolay.
>
> Yes. And my comment applies to both it and your modified version.

Could one of you (Michael or Nikolay) re-submit the patch with this 
addressed? I can do it myself, but I'd rather avoid thinking about who 
to attribute the resulting patch to.

Re: Patches for Emacs 25.2

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

>>>>>> Can't we move the (file-remote-p ... 'localname) call to the first
>>>>>> hunk here? Then the patch will only change one place, and we get rid
>>>>>> of the added `or' here.
>>>>>
>>>>> I let it to Nikolay.
>>>>
>>>> Not sure I understand.
>>>
>>> The patch for the emacs-25 branch is proposed by Nikolay.
>>
>> Yes. And my comment applies to both it and your modified version.
>
> Could one of you (Michael or Nikolay) re-submit the patch with this
> addressed? I can do it myself, but I'd rather avoid thinking about who
> to attribute the resulting patch to.

I let it to Nikolay.

Best regards, Michael.

file-local-name (was: Patches for Emacs 25.2)

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 07.11.2016 12:58, Michael Albinus wrote:
>
>>> I wouldn't mind such a function if we find a good name for it. No
>>> point in making it a macro or a defsubst, I think.
>>
>> `file-localname'?
>
> Fine by me. Although the code it would be replacing is pretty short
> anyway, so there's no hurry about introducing the function.

Finally, I've chosen `file-local-name'. Patch committed to master.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

Hi Eli,

Here's a patch I'd like to install into the release branch.

It should be "obviously safe" (js-mode doesn't set a 
forward-sexp-function), and will improve performance for js2-mode users.

Some backstory: https://github.com/mooz/js2-mode/issues/369

Is it OK?

diff --git a/lisp/progmodes/js.el b/lisp/progmodes/js.el
index 6d995a0..0ba7aac 100644
--- a/lisp/progmodes/js.el
+++ b/lisp/progmodes/js.el
@@ -1851,7 +1851,8 @@ js--multi-line-declaration-indentation
    "Helper function for `js--proper-indentation'.
  Return the proper indentation of the current line if it belongs to a 
declaration
  statement spanning multiple lines; otherwise, return nil."
-  (let (at-opening-bracket)
+  (let (forward-sexp-function ; use Lisp version even in js2-mode
+        at-opening-bracket)
      (save-excursion
        (back-to-indentation)
        (when (not (looking-at js--declaration-keyword-re))

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 16 Dec 2016 02:05:19 +0200
> 
> Here's a patch I'd like to install into the release branch.
> 
> It should be "obviously safe" (js-mode doesn't set a 
> forward-sexp-function), and will improve performance for js2-mode users.
> 
> Some backstory: https://github.com/mooz/js2-mode/issues/369
> 
> Is it OK?

I think it's OK, but I'd like to know the details before I give the
final word.  Could you please file a bug report about that, and
describe there your analysis of the problem which led to the patch?
This will also be good for posterity (the URL you posted doesn't have
that information).

Thanks.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 16.12.2016 10:10, Eli Zaretskii wrote:

> I think it's OK, but I'd like to know the details before I give the
> final word.  Could you please file a bug report about that, and
> describe there your analysis of the problem which led to the patch?
> This will also be good for posterity (the URL you posted doesn't have
> that information).

Done, hopefully. That's https://debbugs.gnu.org/25215.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Fri, 16 Dec 2016 20:33:26 +0200
> 
> On 16.12.2016 10:10, Eli Zaretskii wrote:
> 
> > I think it's OK, but I'd like to know the details before I give the
> > final word.  Could you please file a bug report about that, and
> > describe there your analysis of the problem which led to the patch?
> > This will also be good for posterity (the URL you posted doesn't have
> > that information).
> 
> Done, hopefully. That's https://debbugs.gnu.org/25215.

Thanks, approved.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

Sorry, I don't follow on what you have in mind for removing that check.

When we commit to git on a server we now create a file with path like 
"/pscp:server:/tmp/tramp.29256XNq" which we store in msg-file local 
variable. After we created that file we need to:

a) Create a git command like "git commit -F /tmp/tramp.29256XNq". Here 
we need the remote part of path only.

b) Write to that file. Here we need the absolute path. We also extract 
some args during writing.

With this in mind I don't see how we can get rid of (file-remote-p 
msg-file 'localname) other than storing it in a local variable, which is 
a questionable improvement.

Note: we cannot use file-local-name here, since it was only introduced 
on master.

I guess I'm kind of late(again) with the new pretest out, but I really 
think this bug is pretty major and worth the effort, though YMMV.

-- 
Best Regards,
Nikolay Kudryavtsev

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 31.12.2016 07:20, Nikolay Kudryavtsev wrote:

> b) Write to that file. Here we need the absolute path. We also extract
> some args during writing.

Doesn't the master version of vc-git-checkin use the local name for this 
operation?

> With this in mind I don't see how we can get rid of (file-remote-p
> msg-file 'localname) other than storing it in a local variable, which is
> a questionable improvement.

Not rid of it, but compute msg-file as the local name just once.

> Note: we cannot use file-local-name here, since it was only introduced
> on master.

You can inline its definition. It's tiny.

> I guess I'm kind of late(again) with the new pretest out, but I really
> think this bug is pretty major and worth the effort, though YMMV.

I get that it's a real problem, but since you didn't hurry to reply, and 
nobody else complained about this problem in the meantime, it's probably 
not too urgent IMHO.

Though it's not up to me to decide.

Re: Patches for Emacs 25.2

[Eli Zaretskii]

> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Sat, 31 Dec 2016 14:32:51 +0300
> Cc: Michael Albinus <michael.albinus@gmx.de>, emacs-devel@gnu.org
> 
> > I guess I'm kind of late(again) with the new pretest out, but I really
> > think this bug is pretty major and worth the effort, though YMMV.
> 
> I get that it's a real problem, but since you didn't hurry to reply, and 
> nobody else complained about this problem in the meantime, it's probably 
> not too urgent IMHO.
> 
> Though it's not up to me to decide.

I don't see what decisions need to be made here, as long as we don't
have a patch that is agreed upon.  (Apologies if there is such a patch
and I missed it.)

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 31.12.2016 14:40, Eli Zaretskii wrote:

> I don't see what decisions need to be made here, as long as we don't
> have a patch that is agreed upon.

Indeed, not yet.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

You're right, master indeed uses the local name. I was pretty sure that 
was impossible. So, I tried to figure out how does that work. The answer 
- it does not. Master tries to write to a file on a local machine, which 
does not exist. So I don't think we can replace absolute filename with 
the local one.

P. S. Happy new year.

-- 
Best Regards,
Nikolay Kudryavtsev

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 02.01.2017 13:21, Nikolay Kudryavtsev wrote:
> You're right, master indeed uses the local name. I was pretty sure that
> was impossible. So, I tried to figure out how does that work. The answer
> - it does not. Master tries to write to a file on a local machine, which
> does not exist. So I don't think we can replace absolute filename with
> the local one.

Thank you for testing. Michael, what do you say to this?

> P. S. Happy new year.

Happy new year to you, too. :)

Re: Patches for Emacs 25.2

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 02.01.2017 13:21, Nikolay Kudryavtsev wrote:
>> You're right, master indeed uses the local name. I was pretty sure that
>> was impossible. So, I tried to figure out how does that work. The answer
>> - it does not. Master tries to write to a file on a local machine, which
>> does not exist. So I don't think we can replace absolute filename with
>> the local one.
>
> Thank you for testing. Michael, what do you say to this?

I say that I don't understand it. Nikolay, could you pls give an example
and explain in detail what happens? Note, that I don't own an MS Windows
machine. And if I hijack one, temporarily, it does not run git. So I
have no chance to test anything for the blamed scenario. What I could do
is code reading across the example.

But this looks different to the Emacs 25.2 issue. Nikolay did propose a
patch in bug#23076, and I did propose another version of that patch
(Message-ID: <877f95deig.fsf@gmx.de>), which was later modified to the
patch applied in master. So we have to decided, which of the two offered
patches shall go into the emacs-25 branch, and whether it works. Again,
I cannot test myself. That's why I gave it to Nikolay, several times already.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

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

Hello, Michael.

Just to be clear - we're talking about the current master. That one does 
not work. My original patch, or your rewrite in this thread works.

So here's what happens.

1. Msg-file has a local name as its value. Let's say I'm trying to 
commit into repository /pscp:server:/home/user/git-test/. 
Make-nearby-temp-file creates a file 
"/pscp:server:/home/user/git-test/git-msg23408zHn". Msg-file becomes 
"/home/user/git-test/git-msg23408zHn".

2. We concat a git command like "git commit 
/home/user/git-test/git-msg23408zHn". No problem here, since we need a 
local name.

3. (write-region (car args) nil msg-file) happens and it fails, because 
local name "/home/user/git-test/git-msg23408zHn" expands to 
"c:/home/user/git-test/git-msg23408Ncz" which is a file on a local 
machine and it does not exist.

The fix would be having absolute name for 
msg-file(/pscp:server:/home/user/git-test/git-msg23408zHn) and then 
using (or (file-remote-p msg-file 'localname)  msg-file) for git commit. 
Everywhere else we need the absolute name.

I've attached those changes as patch.

-- 
Best Regards,
Nikolay Kudryavtsev


[-- Attachment #2: remote-vc-git-master-fixed.patch --]
[-- Type: text/plain, Size: 1159 bytes --]

diff --git lisp/vc/vc-git.el lisp/vc/vc-git.el
index c670280..616c150 100644
--- lisp/vc/vc-git.el
+++ lisp/vc/vc-git.el
@@ -707,14 +707,16 @@
           ;; message.  Handle also remote files.
           (if (eq system-type 'windows-nt)
               (let ((default-directory (file-name-directory file1)))
-                (file-local-name (make-nearby-temp-file "git-msg"))))))
+                (make-nearby-temp-file "git-msg")))))
     (cl-flet ((boolean-arg-fn
                (argument)
                (lambda (value) (when (equal value "yes") (list argument)))))
       ;; When operating on the whole tree, better pass "-a" than ".", since "."
       ;; fails when we're committing a merge.
       (apply 'vc-git-command nil 0 (if only files)
-             (nconc (if msg-file (list "commit" "-F" msg-file)
+             (nconc (if msg-file (list "commit" "-F"
+                                       (or (file-remote-p msg-file 'localname)
+                                           msg-file))
                       (list "commit" "-m"))
                     (let ((args
                            (log-edit-extract-headers

Re: Patches for Emacs 25.2

[Michael Albinus]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> Hello, Michael.

Hi Nikolay,

> Just to be clear - we're talking about the current master. That one
> does not work. My original patch, or your rewrite in this thread
> works.

Thanks for the reply; I'll review it later.

But this message is about *Emacs 25.2*, and whether a backport of the
patch in master shall be applied there. Any of your original patch or my
rewrite could do; it's your decision.

Eli is waiting for this answer (or not; perhaps it is too late).

Best regards, Michael.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 02.01.2017 21:54, Michael Albinus wrote:

> But this message is about *Emacs 25.2*, and whether a backport of the
> patch in master shall be applied there.

Not really, my question was why the code in master is different. I was 
uneasy to accept the patch against 25.2 without clearly knowing the 
reason of the difference.

> Eli is waiting for this answer (or not; perhaps it is too late).

I imagine he's waiting for my choice now.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 18.10.2016 15:32, Michael Albinus wrote:

> I don't decide whether such a patch shall be applied; it shall be
> decided by the vc maintainer. But in case of, the patch could be written
> shorter:

Trying this patch again, I'm getting these byte-compilation warnings:

712  22 warning         reference to free variable ‘method’ (emacs-lisp)
712  29 warning         reference to free variable ‘user’ (emacs-lisp)
712  34 warning         reference to free variable ‘host’ (emacs-lisp)
713  50 warning         reference to free variable ‘v’ (emacs-lisp)
1503   1 warning         the following functions are not known to be 
defined: with-parsed-tramp-file-name, tramp-make-tramp-file-name, 
tramp-make-tramp-temp-file (emacs-lisp)

Which probably means that we need to load Tramp somehow at least during 
compilation, for the macro. And also declare the missing functions.

Re: Patches for Emacs 25.2

[Michael Albinus]

Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com> writes:

> Hello, Michael.

Hi Nikolay,

> -             (nconc (if msg-file (list "commit" "-F" msg-file)
> +             (nconc (if msg-file (list "commit" "-F"
> +                                       (or (file-remote-p msg-file 'localname)
> +                                           msg-file))

What's wrong with

(nconc (if msg-file (list "commit" "-F" (file-local-name msg-file))

> Best Regards,
> Nikolay Kudryavtsev

Best regards, Michael.

Re: Patches for Emacs 25.2

[Michael Albinus]

Dmitry Gutov <dgutov@yandex.ru> writes:

> On 02.01.2017 21:54, Michael Albinus wrote:
>
>> But this message is about *Emacs 25.2*, and whether a backport of the
>> patch in master shall be applied there.
>
> Not really, my question was why the code in master is different. I was
> uneasy to accept the patch against 25.2 without clearly knowing the
> reason of the difference.

The code in master uses newly introduced functions, which are not
available in Emacs 25.

Best regards, Michael.

Re: Patches for Emacs 25.2

[Dmitry Gutov]

On 02.01.2017 22:56, Michael Albinus wrote:

> The code in master uses newly introduced functions, which are not
> available in Emacs 25.

We were discussing the use of local vs. remote file names, in particular 
with write-region.

Newly introduced functions are tangential.

Re: Patches for Emacs 25.2

[Nikolay Kudryavtsev]

> (nconc (if msg-file (list "commit" "-F" (file-local-name msg-file))
Yeah, this seems like a better way. Just haven't thought of it.

-- 
Best Regards,
Nikolay Kudryavtsev