branches: Check symlinked subdirectories - #4388

Hi @doanac - thanks for the pull request! Sorry I haven't been able to take a look, I was at a conference this week.

Definitely we should support symlinks in branches. Sorry about the bug! I'll take a look this weekend at your pr!

A little background on our iterators: these are generally used for comparing two file sources, be they trees, the index or the working folder. The iterators drive our diff, merge, rebase, etc interfaces.

It turns out that they also are used for branch enumeration.

My concern with this patch is that there's now an edge case in symlink handling. I'm going to poke around - but I wonder if there's now a way to trick a symlink in the working directory to being examined as a folder and we "read through" the symlink.

(I feel like we should have some tests here, but it's possible that we do not.)

In the meantime, I think that the safe thing to do here is to add a new iterator flag that indicates that we should return the contents of symlinks and only enable it during branch enumeration.

I'm also curious: do we handle symbolic links to files correctly in .git/refs?

As my debugging lead me into the iterator files I got a little nervous making such a change as a drive-by-fix . I'm away from my computer right now, but I think the .git/refs/tags//tag was broke so I assume its .git/refs will be the same. I'll take a look into that as well as the iterator flag when I'm back at my desk tomorrow night.

One thing I was wondering was if thought the location of the is_directory was okay? It kind of felt like an odd place to re-do almost identical logic that was done earlier. I was almost thinking we could add a member to the "filesystem_iterator_entry" struct that held the value of the call to "stat" like you do for lstat now. Then is_directoy has less to and could just check the iterator flag you mentioned.

Also, just let me know if you'd rather treat this like a bug and do it yourself. I'm sure you might spend more time walking me through this than you would fix it yourself, but I'm more than happy to try and do this.

I've just done a new push that adds an iterator flag for this option.

What is the use-case for having symlinked directories inside the refs database?

I'm sceptical of a change to such a core part as the iterators when no Git implementation should ever put this into the database and a packe-refs --all should be removing it anyway, so you shouldn't be reliant on the symlink continuing to exist past garbage collection.

The main reason I brought this up was because of something we were prototyping on the server side. We basically wanted a single "common" repo and then have a bunch of "subscriber" repos that could see the sha and refs in that repo. Then we'd push to the common and give some branches or tags like "magic-symlink/v2.0" and all the subscribers would instantly see it all.

I used the objects/info/alternates to handle the git objects, and when playing with native Git the symlinks worked fine. However, I noticed GitLab and PyGit couldn't do it, so I realized it must be something slightly different in libgit2.

When I opened this, I had no idea it would wind up in such in invasive part of the code base... I figured it would be a one-liner 's/lstat/stat' in some refs.c file. This is an edge case, and to be honest we've realized how crazy the idea was and have moved on to a different approach.

Feel free to Nack this patch if the concerns are still too high.

If you want to keep different repositories in sync server-side, you should do this work server-side. Having these symlinks means you are likely to end up removing or rolling back the values for these symlinked references. Just like with --shared/odb alternates, except that nothing in Git would consider pack-refs --all destructive.

While the current repository format uses plain files for refernce storage, this is part of its database format and should be treated as opaquely as you might treat any other form of database.

I think that this is a good change, and is reasonably low impact. I'm not particularly skeptical about this change - the alternative would be rearchitecting the refs layer to not use the iterators, reserving them only for repository operations like diff and merge. I think that's much riskier and should be avoided unless a compelling argument presents itself.

I wanted to exercise this a bit before merging it, so I added a test on top. While I was in there, I added a bit of error checking as well. I opened #4455 with this change instead of merging this manually, so that I could validate the CI before merging.

Thanks @doanac for the fix and to @EmbeddedAndroid for validating this locally. ✨