This is an archive of the discontinued Mercurial Phabricator instance.

Differential D464

editor: use an unambiguous path suffix for editor files
ClosedPublic

Authored by mbolin on Aug 21 2017, 3:46 PM.

Details

Reviewers
quark
durin42
ryanmce
Group Reviewers
hg-reviewers
Commits
rHG6e6452bc441d: editor: use an unambiguous path suffix for editor files
Summary

Changes the API of ui.edit() to take an optional action argument,
which is used when constructing the suffix of the temp file.
Previously, it was possible to set the suffix by specifying a suffix to the
optional extra dict that was passed to ui.edit(), but the goal is to
drop support for extra.suffix and make action a required argument.
To this end, ui.edit() now yields a develwarn() if action is not set
or if extra.suffix is set.

I updated all calls to ui.edit() I could find in hg-crew to specify the
appropriate action. This means that when creating a commit, instead
of the path to the editor file being something like:

/tmp/hg-editor-XXXXXX.txt

it is now something like:

/tmp/hg-editor-XXXXXX.commit.hg.txt

Some editors (such as Atom) make it possible to statically define a [TextMate]
grammar for files with a particular suffix. For example, because Git reliably
uses .git/COMMIT_EDITMSG and .git/MERGE_MSG as the paths for commit-type
messages, it is trivial to define a grammar that is applied when files of
either name are opened in Atom:

https://github.com/atom/language-git/blob/v0.19.1/grammars/git%20commit%20message.cson#L4-L5

Because Hg historically used a generic .txt suffix, it was much harder to
disambiguate whether a file was an arbitrary text file as opposed to one
created for the specific purpose of authoring an Hg commit message.

This also makes it easier to add special support for histedit, as it has its own
suffix that is distinct from a commit:

/tmp/hg-histedit-XXXXXX.histedit.hg.txt

Test Plan

Added an integration test: test-editor-filename.t.

Manually tested: ran hg ci --amend for this change and saw that it
used /tmp/hg-editor-ZZjcz0.commit.hg.txt as the path instead of
/tmp/hg-editor-ZZjcz0.txt as the path.

Verified make tests passes.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mbolin created this revision.Aug 21 2017, 3:46 PM
quark retitled this revision from Use an unambigious path suffix for the commit editor file. to commit: use an unambiguous path suffix for the commit editor file.Aug 21 2017, 8:04 PM
quark accepted this revision.Aug 21 2017, 8:07 PM

Looks good to me. I have changed the title to follow the topic: uncapitalized, no trailing period pattern suggested by ContributingChanges wiki.

ryanmce accepted this revision.Aug 22 2017, 5:50 AM
ryanmce added a subscriber: ryanmce.

Should be safe and effective.

ryanmce requested changes to this revision.Aug 22 2017, 6:46 AM

But you should include a test to prevent regressions.

This revision now requires changes to proceed.Aug 22 2017, 6:46 AM
quark requested changes to this revision.Aug 22 2017, 5:15 PM

There are some discussion in the list (https://www.mercurial-scm.org/pipermail/mercurial-devel/2017-August/103382.html). Just mentioning it here since Phabricator In-Bound email handling is not configured properly.

smf added a subscriber: smf.Aug 23 2017, 5:07 PM

Everything I was talking about in D468 was meant to be posted here. (I am not doing well with phabricator it seems)

durin42 requested changes to this revision.Aug 28 2017, 2:58 PM
durin42 added inline comments.
mercurial/cmdutil.py
3221

I agree with smf: this patch is accomplishing a reasonable thing, but we should clean up this interface if we're going to use it. Let's add an action= parameter that's optional in the 4.4 cycle, with a devel warning if it's not specified. Then in 4.4 we can make it a mandatory parameter for ui.edit().

Right now the only client of this poke-something-in-extra API is histedit, so the time is right to clean up the API before more consumers do silly things. For your immediate goal in this patch, here's what I'd suggest as a compromise:

  1. Add the new action=None business to ui.edit(), which then makes suffix='.hg$ACTION.txt' or similar
  2. Update histedit to use that instead of screwing around with 'suffix' in extra
  3. Remove support for 'suffix' in extra
  4. Do this patch, but with action='commit' instead of poking '.hgcommit.txt' in the extra

How does that sound to everyone?

mbolin added a comment.Aug 28 2017, 5:50 PM

@durin42 Sounds good: I'll work on cleaning this up.

smf added inline comments.Aug 28 2017, 6:00 PM
mercurial/cmdutil.py
3221

Yeah, this is pretty much what I had in mind (sorry again for the confusion!). I also don't have any preference for '.hg.$ACTION.txt' just for it to be something sane / stable.

durin42 added inline comments.Aug 28 2017, 6:24 PM
mercurial/cmdutil.py
3221

A thought: should we do .$ACTION.hg.txt so we can do suffix-only detection of "something from hg" vs specific actions?

mbolin edited edge metadata.Aug 28 2017, 11:04 PM
mbolin updated this revision to Diff 1380.

Created optional action param as suggested by @durin42

mbolin updated this revision to Diff 1381.Aug 28 2017, 11:05 PM

Changed format string to .%s.hg.txt.

mbolin retitled this revision from commit: use an unambiguous path suffix for the commit editor file to commit: use an unambiguous path suffix for editor files.Aug 29 2017, 12:17 AM
mbolin edited the summary of this revision. (Show Details)
mbolin edited the test plan for this revision. (Show Details)
mbolin retitled this revision from commit: use an unambiguous path suffix for editor files to editor: use an unambiguous path suffix for editor files.

@durin42 Note that I substantially revised the commit message and test plan to reflect the changes.

mbolin edited the test plan for this revision. (Show Details)Aug 29 2017, 12:19 AM
ryanmce requested changes to this revision.Aug 29 2017, 7:00 AM

One small issue noted inline.

A new test would be super nice to prevent regressions here.

mercurial/crecord.py
1566

Why does this have a . in it, but commit, above does not?

Seems like this should just be diff because the . is added in the code below.

This revision now requires changes to proceed.Aug 29 2017, 7:00 AM
mbolin edited edge metadata.Aug 29 2017, 12:19 PM
mbolin marked an inline comment as done.
mbolin updated this revision to Diff 1407.

Remove extra dot in action for diff as caught by @ryanmce.

mbolin added a comment.Aug 29 2017, 12:38 PM

@ryanmce I'm happy to write a test, but what do you want me to target: ui.edit() itself?

mbolin added a comment.Aug 29 2017, 12:48 PM

Also, it looks like I would have to refactor ui.edit() quite a bit to create a unit test since the file name never escapes the method and that's what you would like me to verify, right?

durin42 added a comment.Aug 29 2017, 1:38 PM
In D464#9054, @mbolin wrote:

Also, it looks like I would have to refactor ui.edit() quite a bit to create a unit test since the file name never escapes the method and that's what you would like me to verify, right?

You could do that, but what I'd do is make an "editor" that prints the filename to stderr or something, and then invoke some commands that call that editor to see what the filename is like.

ryanmce added a comment.Aug 29 2017, 2:06 PM

You could do that, but what I'd do is make an "editor" that prints the filename to stderr or something, and then invoke some commands that call that editor to see what the filename is like.

Yeah, that's what I had in mind too. Something like:

editor.sh:

#!/bin/bash
echo "$@"
exit 1

test-editor-filename.t:

$ hg commit --config ui.editor=editor.sh
*.commit.hg.txt (glob)
[1]
mbolin updated this revision to Diff 1447.Aug 30 2017, 3:29 PM

Added an integration test to verify editor filenames.

mbolin edited the test plan for this revision. (Show Details)Aug 30 2017, 3:30 PM
mbolin updated this revision to Diff 1448.Aug 30 2017, 3:37 PM

Address "don't export and assign at once" issue.

durin42 added inline comments.Aug 30 2017, 3:42 PM
tests/test-editor-filename.t
16

realpath is not portable. I think you can use $TESTTMP/editor.sh though.

mbolin updated this revision to Diff 1449.Aug 30 2017, 4:27 PM

Removed realpath in favor of $TESTTMP/editor.sh. Now hg init in the current directory instead of a subdirectory.

mbolin marked an inline comment as done.Aug 30 2017, 4:27 PM
mbolin added inline comments.
tests/test-editor-filename.t
16

Good catch: done!

mbolin marked an inline comment as done.Aug 31 2017, 1:53 PM

Anything else I can do for this?

durin42 accepted this revision.Aug 31 2017, 2:07 PM
Closed by commit rHG6e6452bc441d: editor: use an unambiguous path suffix for editor files (authored by mbolin). · Explain WhyAug 31 2017, 3:05 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Sep 1 2017, 10:09 AM
yuja added inline comments.
mercurial/cmdutil.py
344

Sorry for late review, but doesn't it change the suffix from
.diff to .diff.hg.txt?

durin42 added inline comments.Sep 1 2017, 10:18 AM
mercurial/cmdutil.py
344

Argh, yes it does. @mbolin can you send a follow-up to fix that?

mbolin added inline comments.Sep 1 2017, 3:46 PM
tests/test-editor-filename.t
7

I think this has to change to:

$ cat > $TESTTMP/editor.sh << EOF

to fix the FreeBSD build.

quark added inline comments.Sep 1 2017, 4:10 PM
tests/test-editor-filename.t
7

This is not the root test. run-tests.py guarantees the current directory is at $TESTTMP when the test starts.

8

FreeBSD does not have bash installed by default. Maybe we should have a check-rule for that. Removing this line and the test will pass.

mbolin added inline comments.Sep 1 2017, 4:18 PM
mercurial/cmdutil.py
344

OK, I have a fix in D607 and I'm working on an integration test.

Revision Contents
Changeset List

PathPackages
Mhgext/histedit.py (2 lines)
Mhgext/patchbomb.py (3 lines)
Mmercurial/cmdutil.py (4 lines)
Mmercurial/crecord.py (3 lines)
Mmercurial/ui.py (15 lines)
AMtests/test-editor-filename.t (35 lines)

Diff 1501

hgext/histedit.py

for na, l in newact.iteritems(): for na, l in newact.iteritems():
actions.append(na) actions.append(na)
actions += l actions += l
rules = '\n'.join([act.torule() for act in actions]) rules = '\n'.join([act.torule() for act in actions])
rules += '\n\n' rules += '\n\n'
rules += editcomment rules += editcomment
rules = ui.edit(rules, ui.username(), {'prefix': 'histedit'}, rules = ui.edit(rules, ui.username(), {'prefix': 'histedit'},
repopath=repo.path) repopath=repo.path, action='histedit')
# Save edit rules in .hg/histedit-last-edit.txt in case # Save edit rules in .hg/histedit-last-edit.txt in case
# the user needs to ask for help after something # the user needs to ask for help after something
# surprising happens. # surprising happens.
f = open(repo.vfs.join('histedit-last-edit.txt'), 'w') f = open(repo.vfs.join('histedit-last-edit.txt'), 'w')
f.write(rules) f.write(rules)
f.close() f.close()

hgext/patchbomb.py

the user through the editor. the user through the editor.
""" """
ui = repo.ui ui = repo.ui
if opts.get('desc'): if opts.get('desc'):
body = open(opts.get('desc')).read() body = open(opts.get('desc')).read()
else: else:
ui.write(_('\nWrite the introductory message for the ' ui.write(_('\nWrite the introductory message for the '
'patch series.\n\n')) 'patch series.\n\n'))
body = ui.edit(defaultbody, sender, repopath=repo.path) body = ui.edit(defaultbody, sender, repopath=repo.path,
action='patchbombbody')
# Save series description in case sendmail fails # Save series description in case sendmail fails
msgfile = repo.vfs('last-email.txt', 'wb') msgfile = repo.vfs('last-email.txt', 'wb')
msgfile.write(body) msgfile.write(body)
msgfile.close() msgfile.close()
return body return body
def _getbundlemsgs(repo, sender, bundle, **opts): def _getbundlemsgs(repo, sender, bundle, **opts):
"""Get the full email for sending a given bundle """Get the full email for sending a given bundle

mercurial/cmdutil.py

fp.seek(0) fp.seek(0)
# 2.5 optionally review / modify patch in text editor # 2.5 optionally review / modify patch in text editor
if opts.get('review', False): if opts.get('review', False):
patchtext = (crecordmod.diffhelptext patchtext = (crecordmod.diffhelptext
+ crecordmod.patchhelptext + crecordmod.patchhelptext
+ fp.read()) + fp.read())
reviewedpatch = ui.edit(patchtext, "", reviewedpatch = ui.edit(patchtext, "",
extra={"suffix": ".diff"}, action="diff",
yujaUnsubmitted
Not Done

Sorry for late review, but doesn't it change the suffix from
.diff to .diff.hg.txt?

yuja: Sorry for late review, but doesn't it change the suffix from `.diff` to `.diff.hg.txt`?
durin42Unsubmitted
Not Done

Argh, yes it does. @mbolin can you send a follow-up to fix that?

durin42: Argh, yes it does. @mbolin can you send a follow-up to fix that?
mbolinAuthorUnsubmitted
Not Done

OK, I have a fix in D607 and I'm working on an integration test.

mbolin: OK, I have a fix in D607 and I'm working on an integration test.
repopath=repo.path) repopath=repo.path)
fp.truncate(0) fp.truncate(0)
fp.write(reviewedpatch) fp.write(reviewedpatch)
fp.seek(0) fp.seek(0)
[os.unlink(repo.wjoin(c)) for c in newlyaddedandmodifiedfiles] [os.unlink(repo.wjoin(c)) for c in newlyaddedandmodifiedfiles]
# 3a. apply filtered patch to clean repo (clean) # 3a. apply filtered patch to clean repo (clean)
if backups: if backups:
# make in-memory changes visible to external process # make in-memory changes visible to external process
tr = repo.currenttransaction() tr = repo.currenttransaction()
repo.dirstate.write(tr) repo.dirstate.write(tr)
pending = tr and tr.writepending() and repo.root pending = tr and tr.writepending() and repo.root
editortext = repo.ui.edit(committext, ctx.user(), ctx.extra(), editortext = repo.ui.edit(committext, ctx.user(), ctx.extra(),
editform=editform, pending=pending, editform=editform, pending=pending,
repopath=repo.path) repopath=repo.path, action='commit')
text = editortext text = editortext
durin42Unsubmitted
Not Done

I agree with smf: this patch is accomplishing a reasonable thing, but we should clean up this interface if we're going to use it. Let's add an action= parameter that's optional in the 4.4 cycle, with a devel warning if it's not specified. Then in 4.4 we can make it a mandatory parameter for ui.edit().

Right now the only client of this poke-something-in-extra API is histedit, so the time is right to clean up the API before more consumers do silly things. For your immediate goal in this patch, here's what I'd suggest as a compromise:

  1. Add the new action=None business to ui.edit(), which then makes suffix='.hg$ACTION.txt' or similar
  2. Update histedit to use that instead of screwing around with 'suffix' in extra
  3. Remove support for 'suffix' in extra
  4. Do this patch, but with action='commit' instead of poking '.hgcommit.txt' in the extra

How does that sound to everyone?

durin42: I agree with smf: this patch is accomplishing a reasonable thing, but we should clean up this…
smfUnsubmitted
Not Done

Yeah, this is pretty much what I had in mind (sorry again for the confusion!). I also don't have any preference for '.hg.$ACTION.txt' just for it to be something sane / stable.

smf: Yeah, this is pretty much what I had in mind (sorry again for the confusion!). I also don't…
durin42Unsubmitted
Not Done

A thought: should we do .$ACTION.hg.txt so we can do suffix-only detection of "something from hg" vs specific actions?

durin42: A thought: should we do .$ACTION.hg.txt so we can do suffix-only detection of "something from…
# strip away anything below this special string (used for editors that want # strip away anything below this special string (used for editors that want
# to display the diff) # to display the diff)
stripbelow = re.search(_linebelow, text, flags=re.MULTILINE) stripbelow = re.search(_linebelow, text, flags=re.MULTILINE)
if stripbelow: if stripbelow:
text = text[:stripbelow.start()] text = text[:stripbelow.start()]
text = re.sub("(?m)^HG:.*(\n|$)", "", text) text = re.sub("(?m)^HG:.*(\n|$)", "", text)

mercurial/crecord.py

# write the initial patch # write the initial patch
patch = stringio() patch = stringio()
patch.write(diffhelptext + hunkhelptext) patch.write(diffhelptext + hunkhelptext)
chunk.header.write(patch) chunk.header.write(patch)
chunk.write(patch) chunk.write(patch)
# start the editor and wait for it to complete # start the editor and wait for it to complete
try: try:
patch = self.ui.edit(patch.getvalue(), "", patch = self.ui.edit(patch.getvalue(), "", action="diff")
ryanmceUnsubmitted
Done

Why does this have a . in it, but commit, above does not?

Seems like this should just be diff because the . is added in the code below.

ryanmce: Why does this have a `.` in it, but `commit`, above does not? Seems like this should just be…
extra={"suffix": ".diff"})
except error.Abort as exc: except error.Abort as exc:
self.errorstr = str(exc) self.errorstr = str(exc)
return None return None
# remove comment lines # remove comment lines
patch = [line + '\n' for line in patch.splitlines() patch = [line + '\n' for line in patch.splitlines()
if not line.startswith('#')] if not line.startswith('#')]
return patchmod.parsepatch(patch) return patchmod.parsepatch(patch)

mercurial/ui.py

This adds an output label of "ui.debug". This adds an output label of "ui.debug".
''' '''
if self.debugflag: if self.debugflag:
opts[r'label'] = opts.get(r'label', '') + ' ui.debug' opts[r'label'] = opts.get(r'label', '') + ' ui.debug'
self.write(*msg, **opts) self.write(*msg, **opts)
def edit(self, text, user, extra=None, editform=None, pending=None, def edit(self, text, user, extra=None, editform=None, pending=None,
repopath=None): repopath=None, action=None):
if action is None:
self.develwarn('action is None but will soon be a required '
'parameter to ui.edit()')
extra_defaults = { extra_defaults = {
'prefix': 'editor', 'prefix': 'editor',
'suffix': '.txt', 'suffix': '.txt',
} }
if extra is not None: if extra is not None:
if extra.get('suffix') is not None:
self.develwarn('extra.suffix is not None but will soon be '
'ignored by ui.edit()')
extra_defaults.update(extra) extra_defaults.update(extra)
extra = extra_defaults extra = extra_defaults
if action:
suffix = '.%s.hg.txt' % action
else:
suffix = extra['suffix']
rdir = None rdir = None
if self.configbool('experimental', 'editortmpinhg'): if self.configbool('experimental', 'editortmpinhg'):
rdir = repopath rdir = repopath
(fd, name) = tempfile.mkstemp(prefix='hg-' + extra['prefix'] + '-', (fd, name) = tempfile.mkstemp(prefix='hg-' + extra['prefix'] + '-',
suffix=extra['suffix'], suffix=suffix,
dir=rdir) dir=rdir)
try: try:
f = os.fdopen(fd, r'wb') f = os.fdopen(fd, r'wb')
f.write(util.tonativeeol(text)) f.write(util.tonativeeol(text))
f.close() f.close()
environ = {'HGUSER': user} environ = {'HGUSER': user}
if 'transplant_source' in extra: if 'transplant_source' in extra:

tests/test-editor-filename.t

  • This file was added.
Test temp file used with an editor has the expected suffix.
$ hg init
Create an editor that writes its arguments to stdout and set it to $HGEDITOR.
$ cat > editor.sh << EOF
mbolinAuthorUnsubmitted
Not Done

I think this has to change to:

$ cat > $TESTTMP/editor.sh << EOF

to fix the FreeBSD build.

mbolin: I think this has to change to: ``` $ cat > $TESTTMP/editor.sh << EOF ``` to fix the FreeBSD…
quarkUnsubmitted
Not Done

This is not the root test. run-tests.py guarantees the current directory is at $TESTTMP when the test starts.

quark: This is not the root test. `run-tests.py` guarantees the current directory is at `$TESTTMP`…
> #!/bin/bash
quarkUnsubmitted
Not Done

FreeBSD does not have bash installed by default. Maybe we should have a check-rule for that. Removing this line and the test will pass.

quark: FreeBSD does not have bash installed by default. Maybe we should have a check-rule for that.
> echo "\$@"
> exit 1
> EOF
$ chmod +x editor.sh
$ hg add editor.sh
$ HGEDITOR=$TESTTMP/editor.sh
$ export HGEDITOR
durin42Unsubmitted
Done

realpath is not portable. I think you can use $TESTTMP/editor.sh though.

durin42: realpath is not portable. I think you can use $TESTTMP/editor.sh though.
mbolinAuthorUnsubmitted
Not Done

Good catch: done!

mbolin: Good catch: done!
Verify that the path for a commit editor has the expected suffix.
$ hg commit
*.commit.hg.txt (glob)
abort: edit failed: editor.sh exited with status 1
[255]
Verify that the path for a histedit editor has the expected suffix.
$ cat >> $HGRCPATH <<EOF
> [extensions]
> rebase=
> histedit=
> EOF
$ hg commit --message 'At least one commit for histedit.'
$ hg histedit
*.histedit.hg.txt (glob)
abort: edit failed: editor.sh exited with status 1
[255]