[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP
From: |
Bruno Barbier |
Subject: |
Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP |
Date: |
Sat, 03 Sep 2022 19:20:17 +0200 |
Ihor Radchenko <yantar92@gmail.com> writes:
> Thanks for the patch!
>
Thanks for the review!
>> I've also included a test, as the problem is reproducible with TRAMP
>> "/mock::" connection. But, that test will only work on GNU/Linux
>> systems.
> Then you also need to guard the tests against system-type variable
> value. If we cannot tests things on Windows, we should at least make the
> tests not fail when they should not.
Done. I've told ERT to skip this new test for ms-dos and windows-nt
systems. Thanks for the variable to use.
> You patch is >15LOC so we do need your copyright assignment before
> merging. Let me know if you face any difficulties with the copyright
> process. Note that FSF should reply within 5 working days.
Done. I just learnt a few days ago the process is done.
> Note that we quote symbols like `symbols'.
> See https://orgmode.org/worg/org-contribute.html#commit-messages
> Also, please link to the bug report in the commit message for future
> reference.
Should be now OK too.
> > + (list shell-command-switch
> > + (concat (file-local-name script-file) " "
> > cmdline)))
>
> Probably you do not need concat here.
> AFAIU, (list shell-command-switch (file-local-name script-file) cmdline)
> should be good enough as ARGS argument of `process-file'.
The shell process should receive 2 arguments: the switch and the
command to execute. I think the 'concat' is mandatory here.
Am I missing something?
>> + (:stdin t :shebang t)
>> + (:cmdline t :stdin t :shebang t)
>> + ))
>
> Please do not leave closing parenthesis at a separate line. See D.1
> Emacs Lisp Coding Conventions section of Elisp manual for details.
Oops. Fixed.
>> +(defconst org-test-tramp-remote-dir "/mock::/tmp/"
>> + "Remote tramp directory.
>> +We really should use 'tramp-test-temporary-file-directory', but that would
>> require TRAMP sources.")
>
> Since TRAMP sources are not normally available, we can add this variable
> as defined in tramp-tests.el somewhere into testing/org-test.el, for
> example.
I've copy/pasted the logic used in GNU Emacs Tramp and added a macro.
I've added (require 'tramp) in the body; should I move it to the top
of the file ?
Thanks again for the review,
Bruno
0001-ob-shell-Use-process-file-when-stdin-or-cmdline.patch
Description: patch
- Re: [BUG] ob-shell: cmdline and stdin broken when used with TRAMP,
Bruno Barbier <=