could we save the call to getFileReader here at all, if skipEnhancedLanguageDetection?

I think we can drop the nil check and move the getFileReader call downwards. Previously nil indicated that the language check should be skipped, but we now have that as a bool.

Previous nil init (see deleted code in this PR):

	if !useEnhancedLanguageDetection && !forceEnhancedLanguageDetection {
		// If USE_ENHANCED_LANGUAGE_DETECTION is disabled, do not read file contents to determine
		// the language. This means we won't calculate the number of lines per language.
		invCtx.NewFileReader = func(ctx context.Context, path string) (io.ReadCloser, error) {
			return nil, nil
		}
	}

New init of according bool:

ShouldSkipEnhancedLanguageDetection: !useEnhancedLanguageDetection && !forceEnhancedLanguageDetection,

I'm not sure if we had that before, but this is the code path that would allow for incremental indexing. We can leave that out for now, but should monitor the latency and load this causes for large repos (i.e., let's check on large cloud customers after the release).

At the end of May I gave it a try to cache based on the directory, and found that it's a bit harder than before. This is mainly because now we get a flat list of records, files and directories in them.

I don't recall exactly if the directory came before the files or after, and am not sure if the order of files and directories is guaranteed. Can you confirm that @eseliger?

I had two ideas: (1) Remember the directory, process files, and when a new directory appears, cache the above. Discarded that again because it felt too hacky. (2) Load everything into a tree structure and process like before, but then we'll need to load everything and actually lose the benefit directory level caching would bring us.

I don't recall exactly if the directory came before the files or after, and am not sure if the order of files and directories is guaranteed. Can you confirm that @eseliger?

I think the order should be guaranteed within one commit, but might differ between commits. No 100% guarantee - I'd need to check in the git code to be sure. How important is that? I don't think that's documented behavior of the API today, so would be something implicit we rely on.

Within one commit is fine. I tried to use the folder as an open or close marker to its files. To determine when all files in a folder have been read and caching that before the whole process completes, the guaranteed order is essential.

Here's what I recall seeing:

...
folder1
folder1/a
folder2/a
folder2/b
folder2/c
folder2
folder3/a
...

If the enclosing folder always comes before or after the files, we can use the leading or trailing folder to determine when all files have been read. That's all the guarantees I'm looking for here. Either before or after, but always right before or right after all of its files.

This is starting to sound like a leetcode problem 🙂

It looks like git will always print the directory node first, then go into that tree, and so forth (depth first).

So it would be

folder (d)
folder/file
folder/nested (d)
folder/nested/file
folder/otherfile
rootfile

It looks like git will always print the directory node first, then go into that tree, and so forth (depth first).

So it would be

folder (d)
folder/file
folder/nested (d)
folder/nested/file
folder/otherfile
rootfile

(blocking)
When is FileReader closed?

This happens in inventory.go with a defer right after the function to get the file reader is called.

	rc, err := getFileReader(ctx, file.Name())
	if err != nil {
		return Lang{}, errors.Wrap(err, "Failed to create a file reader.")
	}
	if rc != nil {
		defer rc.Close()
	}

With https://github.com/sourcegraph/sourcegraph/pull/62946/commits/0c60af57a2f9536a910089e04c17b028a27b36c0 and https://github.com/sourcegraph/sourcegraph/pull/62946/commits/f0443e5d19768717b462d0982e5ad930a6e45906 I updated the code so that the file io.NopCloser (n.FileReader) is only created when the getFileReader function is called.

With https://github.com/sourcegraph/sourcegraph/commit/f8c5be275b9ededc64df577dadfa874628052fb3 I added unit tests to make sure that the file reader is closed after getLang access the function.

I feel like I have a gap when it comes to understanding how and when io readers are opened/closed, and what the impact of un-closed readers in golang are. Will try to read up on that, but if there are some dangerous that you'd like to call out, please don't hesitate! :)

As far as I can see, it's just the general io problems with leaks when you don't close readers.

In the All code we have an ArchiveReader (which now gets closed with https://github.com/sourcegraph/sourcegraph/pull/62946/commits/2d90cef9cacbc557b32c57d6e32da2ac8fc489b7) that gets handed to various io.NopClosers. I think with the ArchiveReader's closing and the other calls to close, we should be safe.

Is there any automatic mechanism in our stack to close all remaining sockets if the request that opened them is done, i.e. garbage collection of handlers and sockets?