This is an archive of the discontinued Mercurial Phabricator instance.

Differential D1808

debugcommands: print out the editor that was searched for (post shlexsplit)
ClosedPublic

Authored by spectral on Jan 4 2018, 7:04 PM.

Details

Reviewers
krbullock
Group Reviewers
hg-reviewers
Commits
rHG564dec70b50c: debugcommands: print out the editor that was searched for (post shlexsplit)
rHG04e81ae47066: debugcommands: print out the editor that was searched for (post shlexsplit)
Summary

A user was in #mercurial and had the following settings in their hgrc:

[ui]
editor = C:\home\npp\notepad++.exe -multiInst -nosession

After shlexsplit, the first argument was mangled into C:homenppnotepad++.exe,
which was quite unlikely to exist. It took many back-and-forths to identify
that adding " characters around the exe would fix the issue; we were thinking
that it's because something was incorrectly *not* splitting and adding/moving
the " characters fixed the split boundaries, but when testing afterward it
appears that it's just mangled.

I considered adding an informational if pycompat.iswindows and \ in the string
about this issue, but was worried that might have too many false positives and
did not do so at this time.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

spectral created this revision.Jan 4 2018, 7:04 PM
spectral updated this revision to Diff 4694.Jan 4 2018, 7:29 PM
spectral updated this revision to Diff 4695.Jan 4 2018, 7:31 PM
krbullock requested changes to this revision.Jan 4 2018, 10:22 PM
krbullock added a subscriber: krbullock.
krbullock added inline comments.
mercurial/debugcommands.py
1212–1213

In this patch, we end up printing something potentially different in the "checking" line than in the "can't find editor" line. That seems wrong. I think I'd rather we do the shlexsplit before writing the "checking commit editor..." message and print the fully-interpreted path there.

This revision now requires changes to proceed.Jan 4 2018, 10:22 PM
spectral updated this revision to Diff 4714.Jan 5 2018, 2:38 PM
spectral added inline comments.Jan 5 2018, 2:39 PM
mercurial/debugcommands.py
1212–1213

I'm not sure I agree, but I'm not sure I disagree either :)

Anything using this output (it shouldn't afaik, but the user report in #mercurial said that this broke Eclipse, maybe it was only looking at debuginstall to see if there were problems and not actually parsing these values...?) might end up with incorrect results.

  • If we limit both of them to [0], it will end up with just the executable and miss the flags. Maybe this is fine? I don't know what Eclipse is doing with this command, basically, but I don't think we have BC guarantees on debug* commands.
  • If either or both of them are not limited to just [0], then they have to be output as something like "c:\foo\bar.exe" "-arg1" "-arg2" to show where the actual word boundaries are; we can't do something like:

    ' '.join(shlexed_editor)

We have to output it like that to avoid a similar problem, where instead of:

[ui]
editor = c:\foo\bar.exe -arg1 -arg2

the user wrote:

[ui]
editor = "c:\foo\bar.exe -arg1 -arg2"

(Currently, that problem is slightly more obvious, since the output is checking commit editor... ("c:\foo\bar.exe -arg1 -arg2")).

Basically... if we're ok with the rather mild BC break, I think it's probably fine to list only the editor, post-shlexsplit. I don't know what the guarantees are here. Anticipating that we're going to be OK with the output difference, since I can't see how things can actually correctly rely on the non-shlexsplit, non-findexe'd value we emit here (and thus have to assume that either nothing does, or they're already broken), I've made the change.

spectral marked an inline comment as done.Feb 6 2018, 1:23 PM

Sorry, I forgot about this, can you take another look? I believe I addressed your most recent comment.

krbullock accepted this revision as: krbullock.Feb 6 2018, 2:00 PM

This LGTM now. I think the executable as interpreted by shlex, but without the added flags, gives enough signal for the user to figure out what might be wrong without confusing things.

durin42 added a subscriber: durin42.Feb 14 2018, 8:44 PM

This looks good to me - @krbullock should I queue it?

Closed by commit rHG04e81ae47066: debugcommands: print out the editor that was searched for (post shlexsplit) (authored by spectral). · Explain WhyFeb 16 2018, 4:52 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/debugcommands.py (7 lines)
Mtests/test-install.t (28 lines)

Diff 4694

mercurial/debugcommands.py

problems += 1 problems += 1
fm.condwrite(not p, '', fm.condwrite(not p, '',
_(" (templates seem to have been installed incorrectly)\n")) _(" (templates seem to have been installed incorrectly)\n"))
# editor # editor
editor = ui.geteditor() editor = ui.geteditor()
editor = util.expandpath(editor) editor = util.expandpath(editor)
fm.write('editor', _("checking commit editor... (%s)\n"), editor) fm.write('editor', _("checking commit editor... (%s)\n"), editor)
cmdpath = util.findexe(pycompat.shlexsplit(editor)[0]) editorbin = pycompat.shlexsplit(editor)[0]
cmdpath = util.findexe(editorbin)
krbullockUnsubmitted
Done

In this patch, we end up printing something potentially different in the "checking" line than in the "can't find editor" line. That seems wrong. I think I'd rather we do the shlexsplit before writing the "checking commit editor..." message and print the fully-interpreted path there.

krbullock: In this patch, we end up printing something potentially different in the "checking" line than…
spectralAuthorUnsubmitted
Not Done

I'm not sure I agree, but I'm not sure I disagree either :)

Anything using this output (it shouldn't afaik, but the user report in #mercurial said that this broke Eclipse, maybe it was only looking at debuginstall to see if there were problems and not actually parsing these values...?) might end up with incorrect results.

  • If we limit both of them to [0], it will end up with just the executable and miss the flags. Maybe this is fine? I don't know what Eclipse is doing with this command, basically, but I don't think we have BC guarantees on debug* commands.
  • If either or both of them are not limited to just [0], then they have to be output as something like "c:\foo\bar.exe" "-arg1" "-arg2" to show where the actual word boundaries are; we can't do something like:

    ' '.join(shlexed_editor)

We have to output it like that to avoid a similar problem, where instead of:

[ui]
editor = c:\foo\bar.exe -arg1 -arg2

the user wrote:

[ui]
editor = "c:\foo\bar.exe -arg1 -arg2"

(Currently, that problem is slightly more obvious, since the output is checking commit editor... ("c:\foo\bar.exe -arg1 -arg2")).

Basically... if we're ok with the rather mild BC break, I think it's probably fine to list only the editor, post-shlexsplit. I don't know what the guarantees are here. Anticipating that we're going to be OK with the output difference, since I can't see how things can actually correctly rely on the non-shlexsplit, non-findexe'd value we emit here (and thus have to assume that either nothing does, or they're already broken), I've made the change.

spectral: I'm not sure I agree, but I'm not sure I disagree either :) Anything using this output (it…
fm.condwrite(not cmdpath and editor == 'vi', 'vinotfound', fm.condwrite(not cmdpath and editor == 'vi', 'vinotfound',
_(" No commit editor set and can't find %s in PATH\n" _(" No commit editor set and can't find %s in PATH\n"
" (specify a commit editor in your configuration" " (specify a commit editor in your configuration"
" file)\n"), not cmdpath and editor == 'vi' and editor) " file)\n"), not cmdpath and editor == 'vi' and editorbin)
fm.condwrite(not cmdpath and editor != 'vi', 'editornotfound', fm.condwrite(not cmdpath and editor != 'vi', 'editornotfound',
_(" Can't find editor '%s' in PATH\n" _(" Can't find editor '%s' in PATH\n"
" (specify a commit editor in your configuration" " (specify a commit editor in your configuration"
" file)\n"), not cmdpath and editor) " file)\n"), not cmdpath and editorbin)
if not cmdpath and editor != 'vi': if not cmdpath and editor != 'vi':
problems += 1 problems += 1
# check username # check username
username = None username = None
err = None err = None
try: try:
username = ui.username() username = ui.username()

tests/test-install.t

checking available compression engines for wire protocol (*zlib*) (glob) checking available compression engines for wire protocol (*zlib*) (glob)
checking "re2" regexp engine \((available|missing)\) (re) checking "re2" regexp engine \((available|missing)\) (re)
checking templates (*mercurial?templates)... (glob) checking templates (*mercurial?templates)... (glob)
checking default template (*mercurial?templates?map-cmdline.default) (glob) checking default template (*mercurial?templates?map-cmdline.default) (glob)
checking commit editor... ($TESTTMP/tools/testeditor.exe) checking commit editor... ($TESTTMP/tools/testeditor.exe)
checking username (test) checking username (test)
no problems detected no problems detected
print out the binary post-shlexsplit in the error message when commit editor is
not found (this is intentionally using backslashes to mimic a windows usecase).
$ HGEDITOR="c:\foo\bar\baz.exe -y -z" hg debuginstall
checking encoding (ascii)...
checking Python executable (*) (glob)
checking Python version (*) (glob)
checking Python lib (*lib*)... (glob)
checking Python security support (*) (glob)
TLS 1.2 not supported by Python install; network connections lack modern security (?)
SNI not supported by Python install; may have connectivity issues with some servers (?)
checking Mercurial version (*) (glob)
checking Mercurial custom build (*) (glob)
checking module policy (*) (glob)
checking installed modules (*mercurial)... (glob)
checking registered compression engines (*zlib*) (glob)
checking available compression engines (*zlib*) (glob)
checking available compression engines for wire protocol (*zlib*) (glob)
checking "re2" regexp engine \((available|missing)\) (re)
checking templates (*mercurial?templates)... (glob)
checking default template (*mercurial?templates?map-cmdline.default) (glob)
checking commit editor... (c:\foo\bar\baz.exe -y -z)
Can't find editor 'c:foobarbaz.exe' in PATH
(specify a commit editor in your configuration file)
checking username (test)
1 problems detected, please check your install!
[1]
#if test-repo #if test-repo
$ . "$TESTDIR/helpers-testrepo.sh" $ . "$TESTDIR/helpers-testrepo.sh"
$ cat >> wixxml.py << EOF $ cat >> wixxml.py << EOF
> import os, subprocess, sys > import os, subprocess, sys
> import xml.etree.ElementTree as ET > import xml.etree.ElementTree as ET
> >
> # MSYS mangles the path if it expands $TESTDIR > # MSYS mangles the path if it expands $TESTDIR