[clangd] Make git ignore index directories

[sums]

Summary

This change makes clangd add .gitignore files to the index directories that ignore everything using an asterisk (*) value. We only add these gitignore files when creating a new directory, to allow cases where the user doesn't want this behavior (unlikely but not sure).

This behavior is similar to meson, which automatically puts a gitignore file in all build directories.

[kadircet]

I am not sure if clangd is the right tool the create those .gitignore files, e.g. what if the project's VCS isn't git? I believe the right thing to do is for projects that want to make use of such tools to ignore the relevant directory explicitly (e.g. `.cache).

[sums]

In D130863#3690275, @kadircet wrote:

I am not sure if clangd is the right tool the create those .gitignore files, e.g. what if the project's VCS isn't git? I believe the right thing to do is for projects that want to make use of such tools to ignore the relevant directory explicitly (e.g. `.cache).

I believe clangd is the right tool, as the choice to use clangd or not is made by individual developers rather than project maintainers. Every git controlled C++ project would potentially have to include this directory to their gitignore otherwise.

I only thought of this after seeing how the meson build system behaves. A user can give any name to their build directory, so they add a gitignore to ignore it automatically. Users that don't want this behavior can simply remove the gitignore file, and it won't get recreated. The same behavior is present (and tested) with this change as well.

Lastly, this also has the benefit of only ignoring .cache/clangd/index/* instead of .cache which might be relevant to the git repo, and it'll also stay up to date if the directory path/structure changes.

[sammccall]

My 2c, though I'm away with kids right now so will leave to @kadircet to make a call

Given that this is fully contained (logically and physically) within clangd's index directory, this seems elegant and harmless to me.

(At first i thought it was modifying project-level .gitignores, which would be scary).

In D130863#3690275, @kadircet wrote:

I am not sure if clangd is the right tool the create those .gitignore files, e.g. what if the project's VCS isn't git?

The wrong-tool/slippery-slope argument is legitimate. I'd be pretty comfortable personally drawing the line at "git and nothing else" though.

I believe the right thing to do is for projects that want to make use of such tools to ignore the relevant directory explicitly (e.g. `.cache).

It would be great if there were some general "transient" name patterns that were implicitly ignored. Sadly not the case AFAIK

clang-tools-extra/clangd/test/background-index.test

23

Bleh, ideally this would go in clangd/.gitignore but we'd have to break a bunch of abstractions to do that. I think this is fine.

[sums]

clang-tools-extra/clangd/test/background-index.test

23

Doing it this way also ignores the .gitignore file itself :)

[ilya-biryukov]

I'm with Kadir on this one, but maybe I'm too attached to my .git/info/excludes. I somehow don't feel it's a big burden specific to clangd. I have to configure .gitignore for my projects and .git/info/excludes for other projects I work on because of many tools I personally use (editors, etc).

If this lands, I feel we should say * in the added .gitignore and put it into .cache/clangd.
That way the .cache folder does not appear in the git status at all and this improves the workflow for people who use git, i.e. they don't see anything related to clangd's internal storage in their VCS by default.

Otherwise, I would personally still put .cache/clangd into the global .gitignore or .git/info/exclude. IMO .cache/clangd should not be in the VCS history at all, even if only for the .gitignore files.
So landing this as is wouldn't really improve the situation for me personally.
Maybe my workflow is different from what others do, but hopefully this can be helpful.

[sammccall]

In D130863#3693135, @ilya-biryukov wrote:

Otherwise, I would personally still put .cache/clangd into the global .gitignore or .git/info/exclude. IMO .cache/clangd should not be in the VCS history at all, even if only for the .gitignore files.
So landing this as is wouldn't really improve the situation for me personally.

Oh, i was under the impression that git didn't track empty directories and this was recursive: if .cache/clangd/index/* are ignored then index/ doesn't exist, so nor does clangd/ or .cache/.

If this is not the case i agree the value is less... (Can't check, don't have a real computer this week)

[ilya-biryukov]

In D130863#3693293, @sammccall wrote:

In D130863#3693135, @ilya-biryukov wrote:

Otherwise, I would personally still put .cache/clangd into the global .gitignore or .git/info/exclude. IMO .cache/clangd should not be in the VCS history at all, even if only for the .gitignore files.
So landing this as is wouldn't really improve the situation for me personally.

Oh, i was under the impression that git didn't track empty directories and this was recursive: if .cache/clangd/index/* are ignored then index/ doesn't exist, so nor does clangd/ or .cache/.

If this is not the case i agree the value is less... (Can't check, don't have a real computer this week)

No, you're right. I was mistaken about what the patch is doing.
Forget what I said, this change is fine. No .gitignore files are in the git status output.

I'm still think that I need to ignore files for other tools I use anyway, so not doing this for clangd is not a big improvement.
But that's just my opinion, please disregard other concerns.

[sums]

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.

[kadircet]

In 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).

clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp

38

no need for llvm::sys::path::filename, as .gitignore is already a file name.

39

rather than calling out std::string you can do return ShardRootSS.str().str();

49

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).

95

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).

[kadircet]

clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp

58

nit:

if(llvm::Error E = addGitignore(..)) {
 elog(".."), std::move(E));
}

std::move is actually important, otherwise destruction of Error on failure state will trigger a crash.

94

we're passing in Directory but using DiskShardRoot here. can you actually make addGitignore a free-function (same as getGitignorePathForShardRoot).

[sammccall]

@sums I think this just needs minor comments addressed and can be landed. Are you still interested in finishing it?

clang-tools-extra/clangd/index/BackgroundIndexStorage.cpp

std::string getShardPathFromFilePath(llvm::StringRef ShardRoot,
                                      llvm::StringRef FilePath) {
   llvm::SmallString<128> ShardRootSS(ShardRoot);
   llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(FilePath) +
                                            "." + llvm::toHex(digest(FilePath)) +
                                            ".idx");
   return std::string(ShardRootSS.str());
 }
 
+std::string getGitignorePathForShardRoot(llvm::StringRef ShardRoot) {
+  llvm::SmallString<128> ShardRootSS(ShardRoot);
+  llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(".gitignore"));
+  return std::string(ShardRootSS.str());
+}
+
 // Uses disk as a storage for index shards.
 class DiskBackedIndexStorage : public BackgroundIndexStorage {
   std::string DiskShardRoot;
 
 public:
   // Creates `DiskShardRoot` and any parents during construction.
   DiskBackedIndexStorage(llvm::StringRef Directory) : DiskShardRoot(Directory) {
+    bool CreatingNewDirectory = !llvm::sys::fs::is_directory(DiskShardRoot);
     std::error_code OK;
     std::error_code EC = llvm::sys::fs::create_directories(DiskShardRoot);
     if (EC != OK) {
       elog("Failed to create directory {0} for index storage: {1}",
            DiskShardRoot, EC.message());
     }
+
+    if (CreatingNewDirectory) {
+      llvm::Error error = addGitignore(DiskShardRoot);
+      if (error) {
+        elog("Failed to add .gitignore to directory {0} for index storage: {1}",
+             DiskShardRoot, error);
+      }
+    }
   }
 
   std::unique_ptr<IndexFileIn>
   loadShard(llvm::StringRef ShardIdentifier) const override {
     const std::string ShardPath =
         getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
     auto Buffer = llvm::MemoryBuffer::getFile(ShardPath);
     if (!Buffer)
llvm::Error storeShard(llvm::StringRef ShardIdentifier,
                          IndexFileOut Shard) const override {
     auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
     return llvm::writeFileAtomically(ShardPath + ".tmp.%%%%%%%%", ShardPath,
                                      [&Shard](llvm::raw_ostream &OS) {
                                        OS << Shard;
                                        return llvm::Error::success();
                                      });
   }
+
+private:
+  llvm::Error addGitignore(llvm::StringRef Directory) {
+    auto FilePath = getGitignorePathForShardRoot(DiskShardRoot);
+    return llvm::writeFileAtomically(
+        FilePath + ".tmp", FilePath,
+        "# This file is autogenerated by clangd. If you change or delete it, "
+        "it won't be recreated.\n*");
+  }
 };
 
 // Doesn't persist index shards anywhere (used when the CDB dir is unknown).
 // We could consider indexing into ~/.clangd/ or so instead.
 class NullStorage : public BackgroundIndexStorage {
 public:
   std::unique_ptr<IndexFileIn>
   loadShard(llvm::StringRef ShardIdentifier) const override {

clang-tools-extra/clangd/test/background-index.test

 # The background index should allow us to go-to-definition on foo().
 # We should also see indexing progress notifications.
 # RUN: clangd -background-index -lit-test < %/t/definition.jsonrpc | FileCheck %/t/definition.jsonrpc --check-prefixes=CHECK,BUILD
 
 # Test that the index is writing files in the expected location.
 # RUN: ls %/t/.cache/clangd/index/foo.cpp.*.idx
 # RUN: ls %/t/sub_dir/.cache/clangd/index/foo.h.*.idx
 
+# Test that the the newly created index directories also have a .gitignore file present.
+# RUN: ls %/t/.cache/clangd/index/.gitignore
+# RUN: ls %/t/sub_dir/.cache/clangd/index/.gitignore
+
+# Delete .gitignore files to test that they're not recreated when loading an existing index directory.
+# RUN: rm %/t/.cache/clangd/index/.gitignore
+# RUN: rm %/t/sub_dir/.cache/clangd/index/.gitignore
+
 # Test the index is read from disk: delete code and restart clangd.
 # RUN: rm %/t/foo.cpp
 # RUN: clangd -background-index -lit-test < %/t/definition.jsonrpc | FileCheck %/t/definition.jsonrpc --check-prefixes=CHECK,USE
+
+# Test that the existing index directories do not have recreated .gitignore files.
+# RUN: test ! -e %/t/.cache/clangd/index/.gitignore
+# RUN: test ! -e %/t/sub_dir/.cache/clangd/index/.gitignore