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: 1140 bytes --]

This is a feature request that depends on resolution of 23075 
<http://debbugs.gnu.org/cgi/bugreport.cgi?bug=23075>.

Git for windows expects one coding system for log input, but returns a 
different one in log output.

In my specific example, with Russian language, I have to set up 
vc-git-commits-coding-system to windows-1251. But then, log output would 
still be in utf-8. Of course I can change this by setting git variable 
logoutputencoding, but this would screw up command line git log output. 
See this thread on help-gnu-emacs 
<https://lists.gnu.org/archive/html/help-gnu-emacs/2014-11/msg00426.html> and 
this thread on Git for Windows google group 
<https://groups.google.com/forum/#%21topic/msysgit/1YQKCNhWTqo> for more 
details.

So, I propose adding another variable called 
vc-git-log-output-coding-system, that would serve as emacs counterpart 
to git's logoutputencoding, just as vc-git-commits-coding-system serves 
a counterpart to commitencoding.

When and if 23075 is fixed we can just set coding-system-for-read to 
this new variable(whenever it's set) in vc-git-print-log.

-- 
Best Regards,
Nikolay Kudryavtsev


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

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

[Nikolay Kudryavtsev]

Since I did this for the other one, here are more detailed steps on how 
to repeat this:
1. (setq vc-git-commits-coding-system 'windows-1251)
2. Clone https://github.com/sg2002/vc-git-bugs
3. Run vc-log on it. This would give you broken output.
4. Evaluate the whole fix.el file from the repo.
5. Run vc-log again. Now you would get the proper "Русскоязычный 
коммит." in it.

Adding a new variable is the only way to setup git with Russian commit 
logs on Windows, while fulfilling all three of the following requirements:
1. Committing from Emacs in Russian should work.
2. vc-log should work in Emacs.
3. git log should work outside of Emacs.

The same should be true for other non-latin alphabets.

-- 
Best Regards,
Nikolay Kudryavtsev

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

[Eli Zaretskii]

> From: Nikolay Kudryavtsev <nikolay.kudryavtsev@gmail.com>
> Date: Mon, 21 Mar 2016 22:01:59 +0300
> 
> 1. (setq vc-git-commits-coding-system 'windows-1251)
> 2. Clone https://github.com/sg2002/vc-git-bugs
> 3. Run vc-log on it. This would give you broken output.
> 4. Evaluate the whole fix.el file from the repo.
> 5. Run vc-log again. Now you would get the proper "Русскоязычный 
> коммит." in it.

(By "vc-log", I presume you meant "C-x v l".  There's no vc-log
command, AFAICT.)

Please try the latest emacs-25 branch in the Emacs repository and tell
which problems for this bug remain.

I actually don't understand the reasons for the 1st step above: the
log message is reported by Git in UTF-8, so if I avoid the first step,
then step 3 produces the correct output.

> Adding a new variable is the only way to setup git with Russian commit 
> logs on Windows, while fulfilling all three of the following requirements:
> 1. Committing from Emacs in Russian should work.
> 2. vc-log should work in Emacs.
> 3. git log should work outside of Emacs.

So if any problems remain, they are either in

  1. Committing from Emacs in Russian should work.

or in

  3. git log should work outside of Emacs.

Please tell which one(s) of these don't work, and why.  I tried "git
log" outside of Emacs, and it produced UTF-8 output.  If it doesn't do
that for you, perhaps you've customized Git itself?  If so, what
happens if you remove that customization?

As for committing from Emacs in Russian, that should again work if you
use UTF-8, so I'm unsure what problems prevent you from having that.

Thanks.

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

[Nikolay Kudryavtsev]

Hello Eli.

Just to explain the underlying issue.

With emacs -Q try committing to the same repository by copy-pasting the 
previous commit message. Then do git log from shell. Your commit message 
would get broken.

This happens because git on Windows expects the commit message to be in 
your Windows "language for non-Unicode programs" encoding. Then it 
recodes from it to utf-8.

So, to be able to commit in russian we:
1. Change language for non-Unicode programs to russian.
2. (setq vc-git-commits-coding-system 'windows-1251)

After doing this, commiting in russian would work. But now our C-x v l 
is broken. We can either fix it by setting logoutputencoding in git, but 
this would break git log outside of emacs, or add a new variable to vc, 
and that's what I want.

That's a relatively recent change in git, from 2013 or 2014, so if 
you're using some really old version, everything might just work out of box.

-- 
Best Regards,
Nikolay Kudryavtsev

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: Sun, 3 Apr 2016 23:34:13 +0300
> 
> Hello Eli.
> 
> Just to explain the underlying issue.

Thanks.

> With emacs -Q try committing to the same repository by copy-pasting the previous commit message. Then do git log from shell. Your commit message would get broken.
> 
> This happens because git on Windows expects the commit message to be in your Windows "language for non-Unicode programs" encoding. Then it recodes from it to utf-8.

I think this conclusion is wrong.  The real reason for the problem is
that Emacs on Windows invokes subordinate programs in a way that
non-ASCII characters in the command-line arguments can only be encoded
in the system codepage.  And Emacs uses the -m command-line argument
to pass the commit log message to Git.  IOW, the problem is not with
Git, the problem is with how Emacs on Windows invokes it.  (For
complicated reasons I won't go into, this general problem cannot be
easily fixed in Emacs.)

So any non-ASCII text encoded in some encoding other than the current
system codepage will become garbled even before it gets to Git.

> So, to be able to commit in russian we:
> 1. Change language for non-Unicode programs to russian.
> 2. (setq vc-git-commits-coding-system 'windows-1251)

This solution doesn't really work for the reasons explained above.

> After doing this, commiting in russian would work. But now our C-x v l is broken.

"C-x v l" is broken because it uses the same value of
vc-git-commits-coding-system to read what Gt outputs, whereas Git
outputs in UTF-8.

> We can either fix it by setting logoutputencoding in git, but this would break git log outside of emacs, or add a new variable to vc, and that's what I want.

I don't think this is the right solution, see below.

> That's a relatively recent change in git, from 2013 or 2014, so if you're using some really old version, everything might just work out of box. 

I have Git 2.8.0, the latest official release.

Since the problem is (a) specific to MS-Windows, and (b) related to
encoding the command-line arguments, the solution should target the
root cause and nothing else, IMO.  Introducing a separate variable
that users should need to configure sounds therefore as not the best
idea.  Moreover, on MS-Windows any value of that additional variable
that is not exactly equal to the current system codepage will simply
fail to work.

So instead, I can suggest one of the following alternatives, to be
done only when invoking Git to commit on MS-Windows:

 1) ignore vc-git-commits-coding-system and always encode the
    command-line arguments using the system locale (in your case,
    codepage 1251); or

 2) put the log message in a temporary file, encoded in
    vc-git-commits-coding-system, then use -F instead of -m; the rest
    of command-line arguments will be encoded in the system locale's
    codepage

The 1st solution is essentially what you wanted, but without the need
to introduce an additional variable or ask the users to configure it.

The 2nd solution is somewhat slower, but it is better, because it will
allow to write log messages using any characters, not just those
representable in the current codepage.  Note that it still doesn't
solve all the problems with non-ASCII characters, because those could
be in the "author" or any of the other arguments with which we call
Git, such as the names of the files whose changes are to be committed
(as Emacs does support arbitrary characters in file names).

Comments?

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

[Eli Zaretskii]

> Date: Mon, 04 Apr 2016 18:22:30 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 23076@debbugs.gnu.org
> 
> So instead, I can suggest one of the following alternatives, to be
> done only when invoking Git to commit on MS-Windows:
> 
>  1) ignore vc-git-commits-coding-system and always encode the
>     command-line arguments using the system locale (in your case,
>     codepage 1251); or
> 
>  2) put the log message in a temporary file, encoded in
>     vc-git-commits-coding-system, then use -F instead of -m; the rest
>     of command-line arguments will be encoded in the system locale's
>     codepage
> 
> The 1st solution is essentially what you wanted, but without the need
> to introduce an additional variable or ask the users to configure it.
> 
> The 2nd solution is somewhat slower, but it is better, because it will
> allow to write log messages using any characters, not just those
> representable in the current codepage.  Note that it still doesn't
> solve all the problems with non-ASCII characters, because those could
> be in the "author" or any of the other arguments with which we call
> Git, such as the names of the files whose changes are to be committed
> (as Emacs does support arbitrary characters in file names).
> 
> Comments?

Ping!

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

[Nikolay Kudryavtsev]

My suggestion was based on the idea that introducing a new variable is 
guaranteed to not break anyone's setup.

Solution 1 seems to be more dangerous in this regard.

Also #1 seems inferior to #2 for this case - what if you don't have 
control over system non-Unicode encoding? Let's say someone wants to 
commit org-mode notes in his native language, from a workplace, where he 
has no admin rights for the machine and no ability to change that 
windows setting. That's probably a rare case, but still, seems like 
something that may happen.

I also did some testing of #2 and noted this thing - the current git 
behaves somewhat weirdly in regards with git commitencoding and message 
files. That is:
1. Let's say your message.txt is encoded in windows-1251. Trying to 
commit it with "git commit -F message.txt" would result in a broken 
commit and this:
> Warning: commit message did not conform to UTF-8.
> You may want to amend it after fixing the message, or set the config
> variable i18n.commitencoding to the encoding your project uses.
2. Let's try doing so and set commitencoding to windows-1251 and commit 
again. Now we get no warning, but our message is a badly coded mess, 
though differently from the previous step, so it did something extra 
while encoding the message.
3. Even when our commitencoding = windows-1251 committing message.txt in 
utf-8 works fine.

So, it seems like we want to always use utf-8 for messages.

-- 
Best Regards,
Nikolay Kudryavtsev

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: Sat, 9 Apr 2016 15:30:02 +0300
> 
> My suggestion was based on the idea that introducing a new variable is 
> guaranteed to not break anyone's setup.
> 
> Solution 1 seems to be more dangerous in this regard.

How can it break someone's setup, if using the current system codepage
is the _only_ way of invoking programs from Emacs on MS-Windows?  If
someone has setup that doesn't already follow this, their setup is
already broken.

> Also #1 seems inferior to #2 for this case - what if you don't have 
> control over system non-Unicode encoding?

Are we talking about MS-Windows here?  If so, no control of
non-Unicode encoding is needed, we only need to _obey_ that encoding.
Emacs always knows what that encoding is, so we just need to use it.

Am I missing something?

> Let's say someone wants to commit org-mode notes in his native
> language, from a workplace, where he has no admin rights for the
> machine and no ability to change that windows setting. That's
> probably a rare case, but still, seems like something that may
> happen.

The solutions I proposed don't require changing any system-wide
settings.  They just _use_ the _existing_ settings to encode non-ASCII
characters passed to Git via its command-line arguments.

Submitting the commit message via a temporary file doesn't need to use
the system codepage, it should use UTF-8, which allows to use almost
any character in any language.

> I also did some testing of #2 and noted this thing - the current git 
> behaves somewhat weirdly in regards with git commitencoding and message 
> files. That is:
> 1. Let's say your message.txt is encoded in windows-1251. Trying to 
> commit it with "git commit -F message.txt" would result in a broken 
> commit and this:
> > Warning: commit message did not conform to UTF-8.
> > You may want to amend it after fixing the message, or set the config
> > variable i18n.commitencoding to the encoding your project uses.

Files submitted via -F should always be in UTF-8.  If we adopt #2,
Emacs will encode the message in UTF-8 when it writes the file which
will then be submitted to Git via -F.

> 2. Let's try doing so and set commitencoding to windows-1251 and commit 
> again. Now we get no warning, but our message is a badly coded mess, 
> though differently from the previous step, so it did something extra 
> while encoding the message.
> 3. Even when our commitencoding = windows-1251 committing message.txt in 
> utf-8 works fine.
> 
> So, it seems like we want to always use utf-8 for messages.

Yes, and that's what I had in mind for alternative #2.

So given these clarifications, which alternative is preferable, in
your opinion?

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

[Nikolay Kudryavtsev]

Ok, I was actually wrong in my last message, that file with commit 
message should always be in the same encoding as git commitencoding. 
There's no weirdness.

> How can it break someone's setup, if using the current system codepage
> is the_only_  way of invoking programs from Emacs on MS-Windows?
Download this:
https://github.com/msysgit/msysgit/releases/download/Git-1.7.9-preview20120201/PortableGit-1.7.9-preview20120201.7z
Then, from emacs -Q:
(setq vc-git-program "d:/Downloads/PortableGit/bin/git.exe")
Note that the git is used from bin, and not from cmd. Russian commits do 
work with this version, when vc-git-commits-coding system is utf-8. 
That's why I kept blaming git.
So we can define breaking someone's setup - if that someone uses this 
old git version, forcing windows-1251 would break his setup.
1.7.9 is the last working version.

> Am I missing something?
No, I just provided a practical example of:

> The 2nd solution is somewhat slower, but it is better, because it will
> allow to write log messages using any characters, not just those
> representable in the current codepage


In the end, of course I like solution #2.

-- 
Best Regards,
Nikolay Kudryavtsev

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: Sat, 9 Apr 2016 17:14:07 +0300
> 
> > How can it break someone's setup, if using the current system codepage
> > is the_only_  way of invoking programs from Emacs on MS-Windows?
> Download this:
> https://github.com/msysgit/msysgit/releases/download/Git-1.7.9-preview20120201/PortableGit-1.7.9-preview20120201.7z
> Then, from emacs -Q:
> (setq vc-git-program "d:/Downloads/PortableGit/bin/git.exe")
> Note that the git is used from bin, and not from cmd. Russian commits do 
> work with this version, when vc-git-commits-coding system is utf-8. 

This cannot work except by accident, and/or for only some specific
characters.  Windows attempts to translate non-ASCII characters passed
to programs to the current codepage, so a byte that is not possible in
the codepage will be mangled.  For example, try a log message that
uses a UTF-8 string which has a 0x98 byte in it -- the only byte that
is not a valid cp1251 character -- you will see it mangled on the Git
side.

> So we can define breaking someone's setup - if that someone uses this 
> old git version, forcing windows-1251 would break his setup.
> 1.7.9 is the last working version.

Even if I'm wrong about this, v1.7.9 is too old to care about.  It's
too easy to upgrade.

> In the end, of course I like solution #2.

Thanks, I will work on implementing it.

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

[Eli Zaretskii]

> Date: Sat, 09 Apr 2016 17:48:13 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 23076@debbugs.gnu.org
> 
> > In the end, of course I like solution #2.
> 
> Thanks, I will work on implementing it.

Done, see the latest emacs-25 branch.

Let me add a few comments to past discussions:

> 1. Let's say your message.txt is encoded in windows-1251. Trying to commit it with "git commit -F message.txt" would result in a broken commit and this:
> 
>     Warning: commit message did not conform to UTF-8.
>     You may want to amend it after fixing the message, or set the config
>     variable i18n.commitencoding to the encoding your project uses.
> 
> 2. Let's try doing so and set commitencoding to windows-1251 and commit again. Now we get no warning, but our message is a badly coded mess, though differently from the previous step, so it did something extra while encoding the message. 

That's not what I see.  What I see is that the warning is gone, and
"git log" produces the log message in its original encoding,
i.e. windows-1251 in your case.  You didn't say what kind of "badly
coded mess" you saw, but one suggestion is to redirect the output of
"git log" to a file, and then visit that file with windows-1251 as its
encoding (i.e. "C-x RET c cp1251 RET C-x C-f THE-FILE RET").  I'm
quite sure you will see your log message intact.

One other thing to keep in mind is that Git apparently records the
value of commitencoding which was in effect at the time of the commit,
so changing the value will only affect future commits.

> 3. Even when our commitencoding = windows-1251 committing message.txt in utf-8 works fine.
> 
> So, it seems like we want to always use utf-8 for messages.

My conclusion from these experiments is different.  It seems like Git
simply records the log messages and outputs them without any
conversions or interpretation, disregarding i18n.commitEncoding.
Which is consistent with the documentation:

  i18n.commitEncoding

  Character encoding the commit messages are stored in; Git itself does not care per se, but this information is necessary e.g. when importing commits from emails or in the gitk graphical history browser 

Note the "Git itself doesn't care" part.  (I don't understand the part
about importing commits from emails -- what do they mean by
"important"?)

I think i18n.commitEncoding is only important if
i18n.logOutputEncoding is set as well, and only if it's set to a
different value (or if you use the --encode option to "git log", and
specify a different encoding).  Then, and only then, the encoding of
the log message is important, because Git will attempt to re-encode it
into the output encoding.

With that in mind, I eventually decided to also add a customizable
option vc-git-log-output-coding-system, that should shadow the
i18n.logOutputEncoding setting.

Please try the latest emacs-25 branch, both without any customizations
of commit and log-output encoding, and with them.  I think the
problems you described should now be gone, as long as you keep the
values of vc-git-commits-coding-system and
vc-git-log-output-coding-system in sync with the corresponding Git
config values.

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

[Nikolay Kudryavtsev]

Eli, sorry for not describing this before in more details, but my data 
in those 3 points was wrong, as I said in the next letter after that one.

So here's how -F flag actually behaves with regards to encodings:
1. When you try to commit using the -F flag with Russian message.txt in 
windows-1251, it gives that warning:
> Warning: commit message did not conform to UTF-8.
> You may want to amend it after fixing the message, or set the config
> variable i18n.commitencoding to the encoding your project uses.

  Then it recodes the log message as windows-1252(sic!). So the russian:
Коммит.
Would become:
Êîììèò.
2. Doing the same while having commitencoding = windows-1251 results in 
a legitimate commit.
3. Having message.txt in utf-8 and commitencoding = windows-1251 results in:
РљРѕРјРјРёС‚.
This is utf-8 recoded to windows-1251.


> My conclusion from these experiments is different.  It seems like Git
> simply records the log messages and outputs them without any
> conversions or interpretation, disregarding i18n.commitEncoding.
That's not true. commitencoding has the immediate effect on the log 
encoding. You can see this by setting commitencoding = windows-1251 and 
doing git log for that test repo.

Log output is decoded by commitencoding, but logoutputencoding overrides 
it. You can test this by:
commitencoding = windows-1251
logoutputencoding = utf-8
This would make git log work again for that repo.

-- 
Best Regards,
Nikolay Kudryavtsev

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

[Nikolay Kudryavtsev]

After testing the latest Emacs 25 dev branch, I can confirm that this 
bug is fixed.

Commit messages are displayed correctly as long as they were committed 
with the same value of commitEncoding.

Eli, thanks for all your help.

-- 
Best Regards,
Nikolay Kudryavtsev

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: Tue, 12 Apr 2016 18:07:30 +0300
> 
> After testing the latest Emacs 25 dev branch, I can confirm that this 
> bug is fixed.
> 
> Commit messages are displayed correctly as long as they were committed 
> with the same value of commitEncoding.
> 
> Eli, thanks for all your help.

Thanks, closing.

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

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

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.

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.