Skip to content

Use a tempfile instead of building our own.#4195

Merged
tenderlove merged 3 commits intomasterfrom
ruby-2.5
Jul 12, 2018
Merged

Use a tempfile instead of building our own.#4195
tenderlove merged 3 commits intomasterfrom
ruby-2.5

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.

3 participants