Re: master babe6a5e948 1/2: Introduce a new TRAMP method `androidsu'

[Michael Albinus]

Po Lu <luangruo@yahoo.com> writes:

Hi,

>> The Author: is missing.
>
> I'd prefer not to leave my e-mail address in this file, subject to
> change as it is.

Then use "Author: Po Lu", w/o an address. It is better than nothing.

>> (defvar tramp-androidsu-su-mm-supported 'unknown
>>   "Whether `su -mm' is supported on this system.")
>>
>> Looks to me as candidate for a connection property.
>
> How so?  From what I understand, connection properties apply to either
> complete connection lists or connection processes, while the presence of
> an option provided by `su' is unaffected by any connection parameters
> except the method itself that influence a connection's identify.

Connection properties are always good for a whole tramp-file-name
structure w/o localname and hop. So you could do

(with-tramp-connection-property vec "mm-supported"
  <code to check whether it is supported>)

The first time it is used, the <code ...> is evaluated, cached, and
returned. Any further call of this construct returns the cache value.

If you use VEC, the result is cached persistently. If you don't like
this, use PROC instead. It works similar, but the cache exists only for
the lifetime of PROC.

>> Should `tramp-remote-shell' and `tramp-tmpdir' be hard coded? I know
>> that the respective strings are hard coded in tramp-adb.el, but now
>> seems to be time to use them via a defconst.
>>
>>  (add-to-list 'tramp-default-host-alist
>>               `(,tramp-androidsu-method nil "localhost"))
>>
>>
>> I would also add
>>
>>  (add-to-list 'tramp-default-user-alist
>>            `(,tramp-androidsu-method nil ,tramp-root-id-string))
>>
>> See also tramp-sh.el.
>>
>>                   (p (start-process (tramp-get-connection-name vec)
>>                                     (tramp-get-connection-buffer vec)
>>                                        ;; Disregard
>>                                        ;; tramp-encoding-shell, as
>>                                        ;; there's no guarantee that it's
>>                                        ;; possible to execute it with
>>                                        ;; `android-use-exec-loader' off.
>>                                     "/system/bin/sh" "-i"))
>>
>>
>> Pls use the constant for the shell name.
>
> Where's the harm in this, though?  The location of /system/bin/sh will
> never change, so there will never be reason for either users or
> developers to replace it by anything else.

Codong style. I also fail to apply it consequently, but experience shows
that such remote device specific settings shall be customizable. Even if
they *look* like they will never change.

This is from 20 years of Tramp maintainership.

>>              (tramp-adb-send-command vec command t t)
>>              ;; Android su binaries contact a background service to
>>              ;; obtain authentication; during this process, input
>>              ;; received is discarded, so input cannot be
>>              ;; guaranteed to reach the root shell until its prompt
>>              ;; is displayed.
>>              (with-current-buffer (process-buffer p)
>>                (tramp-wait-for-regexp p tramp-connection-timeout
>>                                       "#[[:space:]]*$"))
>>
>>
>> Why not (tramp-adb-wait-for-output p tramp-connection-timeout) ?
>> It knows the adb shell prompt, and it uses the proper buffer (no need
>> for (with-current-buffer ...)
>
> tramp-adb-wait-for-output also attempts to detect and delete certain
> character sequences printed by adb, which is inappropriate when the
> shell is being directly run in an Android device.

Yes, this is an ugly hack in tramp-adb.el. I'll rather appreciate
patches for tramp-adb.el which removes this hack forever.

>>                 ;; Disable Unicode.
>>                 (tramp-adb-send-command vec "set +U")
>>
>> Why?
>
> File names with Unicode characters outside the BMP cannot be opened
> otherwise.

Hmm. Then improve the comment, please.

>> I really don't know whether this is a good idea. We have basic file
>> functions which need to handle more than one file. If you have for
>> example '(copy-file SOURCE TARGET)', and SOURCE uses the androidsu
>> method, then `tramp-androidsu-sh-handle-copy-file' will be called. It
>> does everything for SOURCE, but it needs also to do some basic file
>> operations for TARGET. And TARGET could be a remote file name with
>> *another* Tramp method, which could fail then due to your hack.
>
> This won't be a problem, since this wrapper only overrides functions
> defined in tramp-adb.el, which I don't anticipate anyone using on
> Android.

I let this open for now. But I still have a bad gut feeling.

>> That's it for the moment.
>
> Okay, thanks.

Best regards, Michael.