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

login
register
mail settings
Submitter phabricator
Date Jan. 5, 2018, 12:04 a.m.
Message ID <differential-rev-PHID-DREV-3plearrtyxfprj2kqvzn-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26543/
State Superseded
Headers show

Comments

phabricator - Jan. 5, 2018, 12:04 a.m.
spectral created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

AFFECTED FILES
  mercurial/debugcommands.py

CHANGE DETAILS




To: spectral, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 5, 2018, 3:23 a.m.
krbullock requested changes to this revision.
krbullock added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> debugcommands.py:1212
>      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)

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

To: spectral, #hg-reviewers, krbullock
Cc: krbullock, mercurial-devel
phabricator - Jan. 5, 2018, 7:39 p.m.
spectral added inline comments.

INLINE COMMENTS

> krbullock wrote in debugcommands.py:1212
> 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.

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

To: spectral, #hg-reviewers, krbullock
Cc: krbullock, mercurial-devel
phabricator - Feb. 6, 2018, 6:23 p.m.
spectral marked an inline comment as done.
spectral added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

To: spectral, #hg-reviewers, krbullock
Cc: krbullock, mercurial-devel
phabricator - Feb. 6, 2018, 7 p.m.
krbullock accepted this revision as: krbullock.
krbullock added a comment.


  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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

To: spectral, #hg-reviewers, krbullock
Cc: krbullock, mercurial-devel
phabricator - Feb. 15, 2018, 1:44 a.m.
durin42 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

To: spectral, #hg-reviewers, krbullock
Cc: durin42, krbullock, mercurial-devel
phabricator - Feb. 15, 2018, 3:26 a.m.
krbullock added a comment.


  @durin42 Go for it

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D1808

To: spectral, #hg-reviewers, krbullock
Cc: durin42, krbullock, mercurial-devel

Patch

diff --git a/mercurial/debugcommands.py b/mercurial/debugcommands.py
--- a/mercurial/debugcommands.py
+++ b/mercurial/debugcommands.py
@@ -1209,15 +1209,16 @@ 
     editor = ui.geteditor()
     editor = util.expandpath(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)
     fm.condwrite(not cmdpath and editor == 'vi', 'vinotfound',
                  _(" No commit editor set and can't find %s in PATH\n"
                    " (specify a commit editor in your configuration"
-                   " file)\n"), not cmdpath and editor == 'vi' and editor)
+                   " file)\n"), editorbin)
     fm.condwrite(not cmdpath and editor != 'vi', 'editornotfound',
                  _(" Can't find editor '%s' in PATH\n"
                    " (specify a commit editor in your configuration"
-                   " file)\n"), not cmdpath and editor)
+                   " file)\n"), editorbin)
     if not cmdpath and editor != 'vi':
         problems += 1