[PATCH] D130863: [clangd] Make git ignore index directories

[Kadir Cetinkaya via Phabricator via cfe-commits]

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