bug#23076: 24.5; vc-git: add a new variable for log output coding system

bug#23076: 24.5; vc-git: add a new variable for log output coding system

[Nikolay Kudryavtsev]

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

Here's a patch that fixes vc-git commits over tramp on windows.

I don't have the papers signed, but I think this counts as a trivial 
change. If not I'm willing to sign them.

Hope this would be applied into emacs-25 before release.

Thanks.

-- 
Best Regards,
Nikolay Kudryavtsev


[-- Attachment #2: remote-vc-git-windows.patch --]
[-- Type: text/plain, Size: 2387 bytes --]

diff --git lisp/vc/vc-git.el lisp/vc/vc-git.el
index 9eac5b2..dd2f7af 100644
*** lisp/vc/vc-git.el
--- lisp/vc/vc-git.el
***************
*** 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,731 ----
            ;; 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)
!                   (let ((vec (tramp-dissect-file-name file1)))
!                     (tramp-make-tramp-file-name
!                      (tramp-file-name-method vec)
!                      (tramp-file-name-user vec)
!                      (tramp-file-name-host vec)
!                      (tramp-make-tramp-temp-file vec)))
!                   (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"
!                               (if (file-remote-p msg-file)
!                                   (tramp-file-name-localname
!                                    (tramp-dissect-file-name msg-file))
!                                 msg-file))
                        (list "commit" "-m"))
                      (let ((args
                             (log-edit-extract-headers

bug#23076: 24.5; vc-git: add a new variable for log output coding system

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Cc: 23076@debbugs.gnu.org
> Date: Mon, 25 Jul 2016 21:46:27 +0300
> 
> Here's a patch that fixes vc-git commits over tramp on windows.

Thanks.  Michael, could you please review this?

> I don't have the papers signed, but I think this counts as a trivial 
> change. If not I'm willing to sign them.

Yes, it's small enough.  But I encourage you to start the paperwork
rolling anyway, so we could continue accepting your contributions in
the future.

> Hope this would be applied into emacs-25 before release.

Sorry, too late for that: the first release candidate is already out.

bug#23076: 24.5; vc-git: add a new variable for log output coding system

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

>> Here's a patch that fixes vc-git commits over tramp on windows.
>
> Thanks.  Michael, could you please review this?

In general, it looks OK. Although I have reservations to use Tramp
internal functions outside the Tramp package. But there's no Tramp
equivalent to make-temp-file yet. Maybe we shall add such a function?

Best regards, Michael.

bug#23076: 24.5; vc-git: add a new variable for log output coding system

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>,  23076@debbugs.gnu.org
> Date: Tue, 26 Jul 2016 21:01:13 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Here's a patch that fixes vc-git commits over tramp on windows.
> >
> > Thanks.  Michael, could you please review this?
> 
> In general, it looks OK. Although I have reservations to use Tramp
> internal functions outside the Tramp package. But there's no Tramp
> equivalent to make-temp-file yet. Maybe we shall add such a function?

Yes, I think we should.

Remote temporary directory (was: bug#23076: 24.5; vc-git: add a new variable for log output coding system)

[Michael Albinus]

[Moving the discussion to emacs-devel]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> In general, it looks OK. Although I have reservations to use Tramp
>> internal functions outside the Tramp package. But there's no Tramp
>> equivalent to make-temp-file yet. Maybe we shall add such a function?
>
> Yes, I think we should.

Thinking about, a more general solution seems to be a function which
returns a temporary directory on a remote host. Something like this:

--8<---------------cut here---------------start------------->8---
(defun temporary-file-directory (&optional base)
  "The directory for writing temporary files.
BASE indicates the host where this directory is located.  If this
is a remote file name, the directory for writing temporary files
is located on the same host.  A local file name for BASE lets the
function return the value of the variable `temporary-file-directory'.

The default value of BASE is `default-directory'."
--8<---------------cut here---------------end--------------->8---

Then you could call

(let ((temporary-file-directory (temporary-file-directory)))
  (make-temp-file "my-prefix))

Similar for `make-temp-name'. And other use cases in `process-file' and
`start-file-process'.

WDYT?

Best regards, Michael.

Re: Remote temporary directory

[Nikolay Kudryavtsev]

Another way would be abstracting away the remoteness and creating a 
function that makes a temporary file based on some filepath regardless 
of whether it's local or not.

In particular case of that bug, you still have to strip tramp method and 
host when passing path to git, but in general I think such a function 
would work well.

-- 
Best Regards,
Nikolay Kudryavtsev

Re: Remote temporary directory

[Michael Albinus]

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

> Another way would be abstracting away the remoteness and creating a
> function that makes a temporary file based on some filepath regardless
> of whether it's local or not.

`make-temp-file' does it already. It depends only on the settings of
`temporary-file-directory'.

> In particular case of that bug, you still have to strip tramp method
> and host when passing path to git, but in general I think such a
> function would work well.

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

This returns always the local part of a file name. It does not need an
explicit Tramp function call, and it works also for other file name
handlers, like `ange-ftp-hook-function' or `url-file-handler'.

Best regards, Michael.

Re: Remote temporary directory (was: bug#23076: 24.5; vc-git: add a new variable for log output coding system)

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: nikolay.kudryavtsev@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 27 Jul 2016 11:57:42 +0200
> 
> (defun temporary-file-directory (&optional base)
>   "The directory for writing temporary files.
> BASE indicates the host where this directory is located.  If this
> is a remote file name, the directory for writing temporary files
> is located on the same host.  A local file name for BASE lets the
> function return the value of the variable `temporary-file-directory'.
> 
> The default value of BASE is `default-directory'."
> --8<---------------cut here---------------end--------------->8---
> 
> Then you could call
> 
> (let ((temporary-file-directory (temporary-file-directory)))
>   (make-temp-file "my-prefix))
> 
> Similar for `make-temp-name'.

I don't understand what you meant by the last sentence.  Currently,
make-temp-name doesn't support remote files and doesn't pay attention
to temporary-file-directory.

It seems to me that we need to extend make-temp-file to support remote
files, but I see no reason to extend temporary-file-directory for that
purpose, sincea remote version of make-temp-file will invoke utilities
that might have their own ideas about where temporary file should live
and how they should be called.

Re: Remote temporary directory

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

Hi Eli,

>> (defun temporary-file-directory (&optional base)
>>   "The directory for writing temporary files.
>> BASE indicates the host where this directory is located.  If this
>> is a remote file name, the directory for writing temporary files
>> is located on the same host.  A local file name for BASE lets the
>> function return the value of the variable `temporary-file-directory'.
>> 
>> The default value of BASE is `default-directory'."
>> --8<---------------cut here---------------end--------------->8---
>> 
>> Then you could call
>> 
>> (let ((temporary-file-directory (temporary-file-directory)))
>>   (make-temp-file "my-prefix))
>> 
>> Similar for `make-temp-name'.
>
> I don't understand what you meant by the last sentence.  Currently,
> make-temp-name doesn't support remote files and doesn't pay attention
> to temporary-file-directory.

You're right, my bad. `make-temp-name' doesn't pay attention to any directory.

> It seems to me that we need to extend make-temp-file to support remote
> files, but I see no reason to extend temporary-file-directory for that
> purpose, sincea remote version of make-temp-file will invoke utilities
> that might have their own ideas about where temporary file should live
> and how they should be called.

These "utilities that might have their own ideas about where temporary
file should live" are triggered by the function `temporary-file-directory'
proposed above. It would call a file name handler, if BASE (or its
default value `default-directory') indicates this.

If you find it too confusing, that the function has the same name like
the existing variable, we could choose any other name.

Best regards, Michael.

Re: Remote temporary directory

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: nikolay.kudryavtsev@gmail.com,  emacs-devel@gnu.org
> Date: Wed, 27 Jul 2016 18:56:07 +0200
> 
> > It seems to me that we need to extend make-temp-file to support remote
> > files, but I see no reason to extend temporary-file-directory for that
> > purpose, sincea remote version of make-temp-file will invoke utilities
> > that might have their own ideas about where temporary file should live
> > and how they should be called.
> 
> These "utilities that might have their own ideas about where temporary
> file should live" are triggered by the function `temporary-file-directory'
> proposed above. It would call a file name handler, if BASE (or its
> default value `default-directory') indicates this.

Yes, but temporary-file-directory just returns the directory, you
still need to create the file itself.  And due to remote
communications, the window between these two events can be quite
large, so we can be a victim of a race condition with probability that
is too high.

That is why I thought that maybe a single function that actually
creates the file and returns its name could be better.

Re: Remote temporary directory

[Michael Albinus]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, but temporary-file-directory just returns the directory, you
> still need to create the file itself.  And due to remote
> communications, the window between these two events can be quite
> large, so we can be a victim of a race condition with probability that
> is too high.
>
> That is why I thought that maybe a single function that actually
> creates the file and returns its name could be better.

I see. Yes, it makes sense. But we shouldn't extend just
`make-temp-file' with a file name handler; often it is mandatory to get
a local temporary file, even if `default-directory' is remote. Another
function, with the same footprint as `make-temp-file', would be
useful. A pair like we have already for `call-process'/`process-file',
for example.

`make-remote-temp-file' comes to mind, but this would imply that the
result resides on a remote host. It should still behave like
`make-temp-file' when `default-directory' is local, and even in case it
calls a file name handler, that handler must not support remote files.
Maybe you have a better name.

A `temporary-file-directory' function might still be useful, for example
when you call a remote process, and you want it to pass as argument, or
make it the default remote directory while running the process.

Best regards, Michael.

Re: Remote temporary directory

[Michael Albinus]

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

Hi Eli,

> `make-remote-temp-file' comes to mind, but this would imply that the
> result resides on a remote host. It should still behave like
> `make-temp-file' when `default-directory' is local, and even in case it
> calls a file name handler, that handler must not support remote files.
> Maybe you have a better name.

I'm  a little bit undecided. Shall I try it this way? A better name?

> A `temporary-file-directory' function might still be useful, for example
> when you call a remote process, and you want it to pass as argument, or
> make it the default remote directory while running the process.

And also this?

Best regards, Michael.

Re: Remote temporary directory

[Stefan Monnier]

>> `make-remote-temp-file' comes to mind, but this would imply that the
>> result resides on a remote host. It should still behave like
>> `make-temp-file' when `default-directory' is local, and even in case it
>> calls a file name handler, that handler must not support remote files.
>> Maybe you have a better name.
> I'm  a little bit undecided. Shall I try it this way? A better name?

Maybe a new `make-nearby-temp-file` would work, where Emacs would try to
create the temp file "as close as possible" to some target directory.

This could also be used for things like "save into a temp file and then
rename that file to the actual destination".

This said, there are some non-trivial security issues at stake here:
make-temp-file should usually only be used in directories which are
either only writable by "trusted" users (i.e. only by the current user),
or which have the magical "sticky" bit set.  So using just any writable
"nearby" directory is generally unsafe.


        Stefan

Re: Remote temporary directory

[Michael Albinus]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Maybe a new `make-nearby-temp-file` would work, where Emacs would try to
> create the temp file "as close as possible" to some target directory.
>
> This could also be used for things like "save into a temp file and then
> rename that file to the actual destination".

Will do.

> This said, there are some non-trivial security issues at stake here:
> make-temp-file should usually only be used in directories which are
> either only writable by "trusted" users (i.e. only by the current user),
> or which have the magical "sticky" bit set.  So using just any writable
> "nearby" directory is generally unsafe.

I agree. But `make-temp-file' does not check:

(let ((temporary-file-directory "~/")) (make-temp-file "tmp"))
=> "/home/albinus/tmp17866D3i"

It will be even harder to fulfill this in the remote case. Shall we add
an optional argument SAVE-DIR-ONLY to `make-temp-file' /
`make-nearby-temp-file'?  If non-nil, the checks above are performed.

Furthermore, there are file name handlers which do not know of a
temporary directory. Think about "/davs:" or "/gdrive:" etc. On those
remote locations, the function `temporary-file-directory' will return
the current remote directory, and `make-nearby-temp-file' will create
the temp file here.

>         Stefan

Best regards, Michael.

Re: Remote temporary directory

[Stefan Monnier]

>> This said, there are some non-trivial security issues at stake here:
>> make-temp-file should usually only be used in directories which are
>> either only writable by "trusted" users (i.e. only by the current user),
>> or which have the magical "sticky" bit set.  So using just any writable
>> "nearby" directory is generally unsafe.
> I agree. But `make-temp-file' does not check:
> (let ((temporary-file-directory "~/")) (make-temp-file "tmp"))
> => "/home/albinus/tmp17866D3i"

No, indeed, it's hard to check it reliably.  So instead make-temp-file
relies on temporary-file-directory having a sane value.

> It will be even harder to fulfill this in the remote case.

Exactly.  And for make-nearby-temp-file the intention to save "nearby"
(in the same mount point so that `rename' works "atomically".  On AFS
file-systems this basically mean it can only be in the *same* directory)
might be impossible to satisfy while still using a safe directory.

> Shall we add
> an optional argument SAVE-DIR-ONLY to `make-temp-file' /
                       ^^^^
You mean               safe   ?

> `make-nearby-temp-file'?  If non-nil, the checks above are performed.

I don't know how to implement those checks in a robust way.


        Stefan

Re: Remote temporary directory

[Michael Albinus]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>> Shall we add
>> an optional argument SAVE-DIR-ONLY to `make-temp-file' /
>                        ^^^^
> You mean               safe   ?

Sure.

>> `make-nearby-temp-file'?  If non-nil, the checks above are performed.
>
> I don't know how to implement those checks in a robust way.

It might be possible only in some cases. If it isn't possible to find
such a safe directory, the function shall return nil, in case
SAFE-DIR-ONLY is set and non-nil.

>         Stefan

Best regards, Michael.

Re: Remote temporary directory

[Eli Zaretskii]

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: nikolay.kudryavtsev@gmail.com,  emacs-devel@gnu.org
> Date: Fri, 29 Jul 2016 22:15:58 +0200
> 
> > `make-remote-temp-file' comes to mind, but this would imply that the
> > result resides on a remote host. It should still behave like
> > `make-temp-file' when `default-directory' is local, and even in case it
> > calls a file name handler, that handler must not support remote files.
> > Maybe you have a better name.
> 
> I'm  a little bit undecided. Shall I try it this way? A better name?
> 
> > A `temporary-file-directory' function might still be useful, for example
> > when you call a remote process, and you want it to pass as argument, or
> > make it the default remote directory while running the process.
> 
> And also this?

Yes, I think your suggestions are both good.

Thanks.

bug#23076: 24.5; vc-git: add a new variable for log output coding system

[Michael Albinus]

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

> Here's a patch that fixes vc-git commits over tramp on windows.
>
> I don't have the papers signed, but I think this counts as a trivial
> change. If not I'm willing to sign them.
>
> Hope this would be applied into emacs-25 before release.

I've committed a simplified version of this patch to the master branch. 

> Thanks.

Best regards, Michael.