This is an archive of the discontinued Mercurial Phabricator instance.

util: use ~ as a suffix for a temp file in the same directory as a source file
ClosedPublic

Authored by mbolin on Aug 21 2017, 8:39 PM.

Download Raw Diff

Details

Reviewers

durin42
quark

Group Reviewers

hg-reviewers

Commits

rHG2ad028635ccd: util: use ~ as a suffix for a temp file in the same directory as a source file

Summary

Tools like Buck have patterns to ignore the creation of files (in the working
copy) that match certain patterns:

https://github.com/facebook/buck/blob/39278a4f0701c5239eae148968dc1ed4cc8661f7/src/com/facebook/buck/cli/Main.java#L259-L299

When Buck sees a new source file (as reported by Watchman), it has to invalidate
a number of caches associated with the directory that contains the file.
Using a standard suffix, such as ~, would make it easier for Buck and others
to filter out these types of file creation events.

The other uses of tempfile.mkstemp() in Hg do not appear to be problematic
because they (generally speaking) do not specify the dir parameter, so the
new file is created in the system-appropriate temp directory, which is outside
the working copy.

Test Plan

make tests

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, 8:39 PM

Herald added a subscriber: mercurial-devel. · View Herald TranscriptAug 21 2017, 8:39 PM

quark retitled this revision from Use ~ as a suffix when creating a temp file in the same directory as a source file. to util: use ~ as a suffix when creating a temp file in the same directory as a source file.Aug 21 2017, 9:17 PM

mbolin retitled this revision from util: use ~ as a suffix when creating a temp file in the same directory as a source file to util: use ~ as a suffix for temp file in the same directory as a source file.Aug 23 2017, 12:48 AM

mbolin retitled this revision from util: use ~ as a suffix for temp file in the same directory as a source file to util: use ~ as a suffix for a temp file in the same directory as a source file.

I don't object to this, but maybe others do. Reviewers not on vacation, please feel encouraged to push this.

mbolin edited the test plan for this revision. (Show Details)Aug 23 2017, 1:36 PM

In D468#7748, @durin42 wrote:

I don't object to this, but maybe others do. Reviewers not on vacation, please feel encouraged to push this.

Well, I had some objections on the mailing list. It's sad that those don't show up in phabricator. Basically, I'd like a more unified approach for all types of temp files (commit message, histedit, conflicts, etc).

In D468#7833, @smf wrote:

Basically, I'd like a more unified approach for all types of temp files (commit message, histedit, conflicts, etc).

I think this patch is about low-level util function that shouldn't be coupled with ui or config. It has value on its own and I don't think such "unified approach" should block this patch - we can always add configs and make callers of util.mktempcopy pass suffix down here AFTER this patch.

mercurial/util.py
1517	Could you make `suffix` a keyword argument here, default to `~`? Therefore we can give callsite control about what extension it wants.
1528	And use `suffix=suffix` here.

Back to you to add the keyword argument.

This revision now requires changes to proceed.Aug 23 2017, 3:09 PM

@quark Rather than exposing the additional keyword argument now, why don't we wait until there is a compelling use-case to use something other than ~? As it stands, creating an API for this would just open things up to misuse.

In D468#7846, @mbolin wrote:

@quark Rather than exposing the additional keyword argument now, why don't we wait until there is a compelling use-case to use something other than ~? As it stands, creating an API for this would just open things up to misuse.

@smf: I think this is a question for you.

I'm personally fine with either way.

In D468#7836, @quark wrote:

In D468#7833, @smf wrote:

Basically, I'd like a more unified approach for all types of temp files (commit message, histedit, conflicts, etc).

I think this patch is about low-level util function that shouldn't be coupled with ui or config. It has value on its own and I don't think such "unified approach" should block this patch - we can always add configs and make callers of util.mktempcopy pass suffix down here AFTER this patch.

Um, what? That's the whole point of review. Yes, I know you think you can do this AFTER (why are we yelling?). I don't like this patch as-is and am frankly tired of trying to jam stuff through review and promise to clean up later. I am against this patch.

In D468#7908, @smf wrote:

In D468#7836, @quark wrote:

In D468#7833, @smf wrote:

Basically, I'd like a more unified approach for all types of temp files (commit message, histedit, conflicts, etc).

I think this patch is about low-level util function that shouldn't be coupled with ui or config. It has value on its own and I don't think such "unified approach" should block this patch - we can always add configs and make callers of util.mktempcopy pass suffix down here AFTER this patch.

Um, what? That's the whole point of review. Yes, I know you think you can do this AFTER (why are we yelling?). I don't like this patch as-is and am frankly tired of trying to jam stuff through review and promise to clean up later. I am against this patch.

As a bystander here, I'm not even sure what the proposals are. Can one of you summarize what the competing ideas are?

(In general I agree with smf that a more unified error-resistant API should be a goal, and I'm -0 on adding complexity to hg in the name of buck performance if there's a better proposal on the table (but if there is I've missed it.))

In D468#7909, @durin42 wrote:

In D468#7908, @smf wrote:

In D468#7836, @quark wrote:

In D468#7833, @smf wrote:

Basically, I'd like a more unified approach for all types of temp files (commit message, histedit, conflicts, etc).

I think this patch is about low-level util function that shouldn't be coupled with ui or config. It has value on its own and I don't think such "unified approach" should block this patch - we can always add configs and make callers of util.mktempcopy pass suffix down here AFTER this patch.

Um, what? That's the whole point of review. Yes, I know you think you can do this AFTER (why are we yelling?). I don't like this patch as-is and am frankly tired of trying to jam stuff through review and promise to clean up later. I am against this patch.

As a bystander here, I'm not even sure what the proposals are. Can one of you summarize what the competing ideas are?
(In general I agree with smf that a more unified error-resistant API should be a goal, and I'm -0 on adding complexity to hg in the name of buck performance if there's a better proposal on the table (but if there is I've missed it.))

From the mailing list (just posting this here for now so I can reply to it in the next message):

Jun Wu <quark@fb.com> writes:

I think this patch is about changing the suffix. I guess some less powerful
editors may only support matching file types by suffixes.

Yeah, it seems that way.

Please correct me if I'm wrong, but I failed to see how the
"experimental.editortmpinhg" setting could be used as-is to change the
suffix. Therefore I think the patch is still needed.
Maybe we can change it to a config option? Like, instead of saying:
extra_defaults = {
    'prefix': 'editor',
    'suffix': '.txt',
}
Changing ".txt" there to be a config option (maybe
experimental.editorsuffix).

That seems a bit over complicated to me. Why not just just use the
random tmp as a directory instead of a file while renaming the file at
the same time:

/tmp/XYZ123/hg-editor (or HG_EDITOR or whatever)

This would allow 'editortmpinhg'[1] to just be .hg/hg-editor (or
HG_EDITOR etc). The '~' seems a bit like a quick hack and I think I'd
prefer to do this cleanly. My logic here is:

if we can append a character to the suffix, then we should be able to

change the directory

we might want different 'file types' for different tmp files and '~' seems that it might not get us far enough. For instance: a) editor, b) conflicts c) histedit etc

For (a) and (c), I hacked something together for my fork of magit
(called mahgic) so that I can have different modes for commit (to show
the diff) and for histedit (so that 'tab' and 'enter' will show the
commit at the point).

[1] I planned on making editortmpinhg a more thorough thing so that
everything would be in the .hg directory so that programs (like emacs)
would be able to find the correct repo given only the tmp file.

@smf As I put in the summary, I think this use of tempfile.mkstemp() is different than the others in the codebase because it uses the dir= argument to create a file in the working copy. As such, I'd argue that it's reasonable to consider it separately from the others.

In particular, I think it's independent of how temp files are created for things like commit messages as those are paths that are intended to be exposed to the user insofar as they are opened in the user's editor. As it stands, I have D464 out for review as a first step to impose some order on path names for those types of files.

As far as this change is concerned, the focus is to preserve the existing behavior of mktempcopy() while minimizing its impact on other developer tools.

In D468#7910, @smf wrote:

That seems a bit over complicated to me. Why not just just use the
random tmp as a directory instead of a file while renaming the file at
the same time:
/tmp/XYZ123/hg-editor (or HG_EDITOR or whatever)
This would allow 'editortmpinhg'[1] to just be .hg/hg-editor (or
HG_EDITOR etc). The '~' seems a bit like a quick hack and I think I'd
prefer to do this cleanly. My logic here is:

if we can append a character to the suffix, then we should be able to

change the directory

we might want different 'file types' for different tmp files and '~' seems that it might not get us far enough. For instance: a) editor, b) conflicts c) histedit etc

For (a) and (c), I hacked something together for my fork of magit
(called mahgic) so that I can have different modes for commit (to show
the diff) and for histedit (so that 'tab' and 'enter' will show the
commit at the point).
[1] I planned on making editortmpinhg a more thorough thing so that
everything would be in the .hg directory so that programs (like emacs)
would be able to find the correct repo given only the tmp file.

Basically, I was pondering aloud if '~' would be enough to future-proof us and if we shouldn't just rename all temp files to something unique (HG_EDITOR for commits, HG_HISTEDIT for histedit, etc). What I was hoping for was a discussion on that.

@smf Personally, I think that D464 is a better place to have that discussion.

In D468#7914, @smf wrote:

Basically, I was pondering aloud if '~' would be enough to future-proof us and if we shouldn't just rename all temp files to something unique (HG_EDITOR for commits, HG_HISTEDIT for histedit, etc). What I was hoping for was a discussion on that.

This sounds like a good idea. However, if I'm understanding @mbolin correctly, we /also/ want a ~ suffix (or similar) so that devtools don't need to grow hg-specific logic for tempfiles (since, in the case of a ~ suffix, we'll be enough like vim that they will already correctly ignore things). Do I have a reasonable handle on this, or is there more nuance that I'm missing?

In D468#7912, @mbolin wrote:

@smf As I put in the summary, I think this use of tempfile.mkstemp() is different than the others in the codebase because it uses the dir= argument to create a file in the working copy. As such, I'd argue that it's reasonable to consider it separately from the others.
In particular, I think it's independent of how temp files are created for things like commit messages as those are paths that are intended to be exposed to the user insofar as they are opened in the user's editor. As it stands, I have D464 out for review as a first step to impose some order on path names for those types of files.
As far as this change is concerned, the focus is to preserve the existing behavior of mktempcopy() while minimizing its impact on other developer tools.

Jesus, I confused myself on this patch. Yes, you're right. I completely mistook this for other patches I worked on mucking around with the tmp directories.

That's definitely on me, sorry!

In D468#7916, @durin42 wrote:

In D468#7914, @smf wrote:

Basically, I was pondering aloud if '~' would be enough to future-proof us and if we shouldn't just rename all temp files to something unique (HG_EDITOR for commits, HG_HISTEDIT for histedit, etc). What I was hoping for was a discussion on that.

This sounds like a good idea. However, if I'm understanding @mbolin correctly, we /also/ want a ~ suffix (or similar) so that devtools don't need to grow hg-specific logic for tempfiles (since, in the case of a ~ suffix, we'll be enough like vim that they will already correctly ignore things). Do I have a reasonable handle on this, or is there more nuance that I'm missing?

Yeah, I don't know what happened. I guess I misread this and thought it was similar to work I did before. I withdraw my concerns and apologize for derailing this.

In D468#7915, @mbolin wrote:

@smf Personally, I think that D464 is a better place to have that discussion.

Sigh. Yes, this is what I was confused about.

@smf so are you OK with this patch as-is?

In D468#7920, @mbolin wrote:

@smf so are you OK with this patch as-is?

Yeah, should be fine.

smf mentioned this in D464: editor: use an unambiguous path suffix for editor files.Aug 23 2017, 5:07 PM

quark resigned from this revision.Aug 24 2017, 12:53 AM

@durin42 I believe we've come to a resolution on this thread that this is fine as-is, so are you OK to take this now?

durin42 accepted this revision.Aug 29 2017, 11:20 AM

This revision is now accepted and ready to land.Aug 29 2017, 11:20 AM

Closed by commit rHG2ad028635ccd: util: use ~ as a suffix for a temp file in the same directory as a source file (authored by mbolin). · Explain WhyAug 29 2017, 11:43 AM

This revision was automatically updated to reflect the committed changes.

quark mentioned this in D611: checknlink: use a random temp file name for checking.Sep 1 2017, 8:29 PM

quark mentioned this in rHG1104718fb090: checknlink: use a random temp file name for checking.Sep 6 2017, 10:51 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/util.py (2 lines)

Commit	Local	Parents	Author	Summary	Date
f320e0cfd247	36201	833f70277f0e	Michael Bolin	Use ~ as a suffix when creating a temp file in the same directory as a source… (Show More…)	Aug 21 2017, 8:38 PM

Diff 1144

mercurial/util.py

	# check if a CoreGraphics session is available			# check if a CoreGraphics session is available
	return osutil.isgui()			return osutil.isgui()
	else:			else:
	# pure build; use a safe default			# pure build; use a safe default
	return True			return True
	else:			else:
	return pycompat.osname == "nt" or encoding.environ.get("DISPLAY")			return pycompat.osname == "nt" or encoding.environ.get("DISPLAY")

	def mktempcopy(name, emptyok=False, createmode=None):			def mktempcopy(name, emptyok=False, createmode=None):
				quarkUnsubmitted Not Done Could you make `suffix` a keyword argument here, default to `~`? Therefore we can give callsite control about what extension it wants. quark: Could you make `suffix` a keyword argument here, default to `~`? Therefore we can give…
	"""Create a temporary file with the same contents from name			"""Create a temporary file with the same contents from name

	The permission bits are copied from the original file.			The permission bits are copied from the original file.

	If the temporary file is going to be truncated immediately, you			If the temporary file is going to be truncated immediately, you
	can use emptyok=True as an optimization.			can use emptyok=True as an optimization.

	Returns the name of the temporary file.			Returns the name of the temporary file.
	"""			"""
	d, fn = os.path.split(name)			d, fn = os.path.split(name)
	fd, temp = tempfile.mkstemp(prefix='.%s-' % fn, dir=d)			fd, temp = tempfile.mkstemp(prefix='.%s-' % fn, suffix='~', dir=d)
				quarkUnsubmitted Not Done And use `suffix=suffix` here. quark: And use `suffix=suffix` here.
	os.close(fd)			os.close(fd)
	# Temporary files are created with mode 0600, which is usually not			# Temporary files are created with mode 0600, which is usually not
	# what we want. If the original file already exists, just copy			# what we want. If the original file already exists, just copy
	# its mode. Otherwise, manually obey umask.			# its mode. Otherwise, manually obey umask.
	copymode(name, temp, createmode)			copymode(name, temp, createmode)
	if emptyok:			if emptyok:
	return temp			return temp
	try:			try:

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	1144		Aug 21 2017, 8:39 PM	★	★
Diff 2	1396	rHG2ad028635ccd4d47e565c5d59af98ca32165eb3e	Aug 21 2017, 8:38 PM	★	★