Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a tempfile instead of building our own. #4195

Merged
merged 3 commits into from
Jul 12, 2018
Merged

Use a tempfile instead of building our own. #4195

merged 3 commits into from
Jul 12, 2018

Conversation

tenderlove
Copy link
Contributor

@tenderlove tenderlove commented Jul 12, 2018

This commit changes cache writing to use the Tempfile class rather than
constructing our own tempfile. It means we don't have to deal with
unlinking the tempfile in case there's an exception, and it also fixes
this code on Ruby 2.5 (Dir::Tmpname.make_tmpname is no longer
available)

Before this change:

[aaron@TC linguist (master)]$ ruby -I lib:~/git/charlock_holmes/lib:~/github/rugged/lib bin/git-linguist -c 15885701cd883b633847979583f128e4ba1beac9 breakdown
{"Ruby":["Gemfile","Rakefile","bin/git-linguist","bin/linguist","github-linguist.gemspec","lib/linguist.rb","lib/linguist/blob.rb","lib/linguist/blob_helper.rb","lib/linguist/classifier.rb","lib/linguist/file_blob.rb","lib/linguist/generated.rb","lib/linguist/grammars.rb","lib/linguist/heuristics.rb","lib/linguist/language.rb","lib/linguist/lazy_blob.rb","lib/linguist/md5.rb","lib/linguist/repository.rb","lib/linguist/samples.rb","lib/linguist/shebang.rb","lib/linguist/strategy/extension.rb","lib/linguist/strategy/filename.rb","lib/linguist/strategy/modeline.rb","lib/linguist/tokenizer.rb","lib/linguist/version.rb","script/add-grammar","script/convert-grammars","script/fast-submodule-update","script/licensed","script/list-grammars","script/prune-grammars","script/set-language-ids","test/helper.rb","test/test_blob.rb","test/test_classifier.rb","test/test_color_proximity.rb","test/test_file_blob.rb","test/test_generated.rb","test/test_grammars.rb","test/test_heuristics.rb","test/test_instrumentation.rb","test/test_language.rb","test/test_md5.rb","test/test_modelines.rb","test/test_pedantic.rb","test/test_repository.rb","test/test_samples.rb","test/test_shebang.rb","test/test_tokenizer.rb"],"Shell":["script/bootstrap","script/cibuild","script/travis/before_install"]}
Traceback (most recent call last):
	4: from bin/git-linguist:150:in `<main>'
	3: from bin/git-linguist:135:in `git_linguist'
	2: from bin/git-linguist:37:in `linguist'
	1: from bin/git-linguist:50:in `save_language_stats'
bin/git-linguist:80:in `write_cache': uninitialized constant GitLinguist::Tempfile (NameError)

After this change:


[aaron@TC linguist (ruby-2.5)]$ ruby -I lib:~/git/charlock_holmes/lib:~/github/rugged/lib bin/git-linguist -c 15885701cd883b633847979583f128e4ba1beac9 breakdown
{"Ruby":["Gemfile","Rakefile","bin/git-linguist","bin/linguist","github-linguist.gemspec","lib/linguist.rb","lib/linguist/blob.rb","lib/linguist/blob_helper.rb","lib/linguist/classifier.rb","lib/linguist/file_blob.rb","lib/linguist/generated.rb","lib/linguist/grammars.rb","lib/linguist/heuristics.rb","lib/linguist/language.rb","lib/linguist/lazy_blob.rb","lib/linguist/md5.rb","lib/linguist/repository.rb","lib/linguist/samples.rb","lib/linguist/shebang.rb","lib/linguist/strategy/extension.rb","lib/linguist/strategy/filename.rb","lib/linguist/strategy/modeline.rb","lib/linguist/tokenizer.rb","lib/linguist/version.rb","script/add-grammar","script/convert-grammars","script/fast-submodule-update","script/licensed","script/list-grammars","script/prune-grammars","script/set-language-ids","test/helper.rb","test/test_blob.rb","test/test_classifier.rb","test/test_color_proximity.rb","test/test_file_blob.rb","test/test_generated.rb","test/test_grammars.rb","test/test_heuristics.rb","test/test_instrumentation.rb","test/test_language.rb","test/test_md5.rb","test/test_modelines.rb","test/test_pedantic.rb","test/test_repository.rb","test/test_samples.rb","test/test_shebang.rb","test/test_tokenizer.rb"],"Shell":["script/bootstrap","script/cibuild","script/travis/before_install"]}
[aaron@TC linguist (ruby-2.5)]$

tenderlove and others added 3 commits July 12, 2018 11:32
This commit changes cache writing to use the Tempfile class rather than
constructing our own tempfile.  It means we don't have to deal with
unlinking the tempfile in case there's an exception, and it also fixes
this code on Ruby 2.5 (`Dir::Tmpname.make_tmpname` is no longer
available)
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @tenderlove. Merge when ready. Lemme know when you want this on .com and I'll make a new release (tomorrow 🇬🇧 time at the earliest).

@tenderlove tenderlove merged commit 63a4959 into master Jul 12, 2018
@tenderlove tenderlove deleted the ruby-2.5 branch July 12, 2018 20:37
@tenderlove
Copy link
Contributor Author

@lildude thank you! Whenever you have time. It isn't urgent, but I can't upgrade us to Ruby 2.5 without this. Thanks!

@kivikakk
Copy link
Contributor

Going to do a release now so we can see what the 2.5 PR looks like!

@kivikakk
Copy link
Contributor

Unfortunately this fails in some cases since we try to File.rename the created tempfile across mounts. It uses rename(2), which fails with EXDEV.

Dir::Tmpname.make_tmpname takes basename as the first argument, which can include a directory — it's always the prefix of the generated filename. (No directory in basename = no directory in resulting path.) Instead we're now always dropping them in the global temp dir, which you could reasonably expect to be on a different mount (/tmp).

We'll need to change this to drop it in the same directory like we were before.

@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants