kadircet added a comment.
In D130863#3696419 <https://reviews.llvm.org/D130863#3696419>, @sums wrote:
> Thank you for the reviews and discussion, everyone!
>
> TIL about `.git/info/exclude`. It'll let me exclude stuff without updating
> the checked in `.gitignore`. I was achieving the same effect for clangd by
> manually creating a `.gitignore` similar to this change.
>
> The ideal scenario would be one where all tools ignore their temporary
> directories automatically. Will send patches whenever I encounter a case like
> this :)
>
> @kadircet kindly take another look.
thanks! sorry for forgetting about this one. so in the end it looks like there
are two cases:
- clangd is being used on a git-based project, and this is going to be
"invisible" to the user
- clangd is being used on a non-git project, then the user already needs to
ignore this directory somehow and the extra `.gitignore` we generate there will
be ignored by that rule.
So I guess this is a win overall.
Let me know if I should land this for you (and an email address for attribution
of the commit).
================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp:38
+ llvm::SmallString<128> ShardRootSS(ShardRoot);
+ llvm::sys::path::append(ShardRootSS,
llvm::sys::path::filename(".gitignore"));
+ return std::string(ShardRootSS.str());
----------------
no need for `llvm::sys::path::filename`, as `.gitignore` is already a file name.
================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp:39
+ llvm::sys::path::append(ShardRootSS,
llvm::sys::path::filename(".gitignore"));
+ return std::string(ShardRootSS.str());
+}
----------------
rather than calling out `std::string` you can do `return
ShardRootSS.str().str();`
================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp:49
DiskBackedIndexStorage(llvm::StringRef Directory) : DiskShardRoot(Directory)
{
+ bool CreatingNewDirectory = !llvm::sys::fs::is_directory(DiskShardRoot);
std::error_code OK;
----------------
i don't think this check is really needed, we won't really lose much by
overwriting it even if the directory already exists (and this is done once per
compilation database we discover, so it isn't saving much IO).
================
Comment at: clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp:95
+ auto FilePath = getGitignorePathForShardRoot(DiskShardRoot);
+ return llvm::writeFileAtomically(
+ FilePath + ".tmp", FilePath,
----------------
this atomic write has the same chance of colliding with a direct write to
`.gitignore` file by another clangd process.
can you change `.tmp` to `.tmp.%%%%%%%%` to make the collision less likely?
(each `%` is replaced by a random hex digit).
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130863/new/
https://reviews.llvm.org/D130863
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
