This is an archive of the discontinued Mercurial Phabricator instance.

editor: use an unambiguous path suffix for editor files
ClosedPublic

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

Details

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
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

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
3219

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 Sounds good: I'll work on cleaning this up.

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

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
3219

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
1555

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.

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

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?

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.

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
15 ↗(On Diff #1448)

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
15 ↗(On Diff #1448)

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
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
342

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
342

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 ↗(On Diff #1501)

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 ↗(On Diff #1501)

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

8 ↗(On Diff #1501)

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
342

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