Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using .git directory instead of gitdir redirect in submodules. #653

Merged
merged 6 commits into from Aug 12, 2021

Conversation

@crummel
Copy link
Member

@crummel crummel commented Sep 30, 2020

This is not a scenario for normal git repos, but is still legal, and the Arcade uberclone purposely uses this scenario for minimal SourceLink metadata.

Copy link
Member

@tmat tmat left a comment

We need a test that validates this behavior.

It'd also be useful to document a sequence of git commands that create this kind of repo structure.

@crummel crummel requested a review from tmat Oct 6, 2020
@crummel
Copy link
Member Author

@crummel crummel commented Oct 8, 2020

I think I've addressed your comments @tmat, could you take a look when you have a chance?

Base automatically changed from master to main Mar 17, 2021
@crummel
Copy link
Member Author

@crummel crummel commented Mar 23, 2021

@tmat I think @MichaelSimons is running into this in another case, could you take a look at the PR again?

@MichaelSimons
Copy link

@MichaelSimons MichaelSimons commented Aug 5, 2021

@tmat, @crummel - Friendly ping. Can we get a fix in? This is needed to unblock the offline source-build builds for 6.0

@crummel crummel force-pushed the submodulesWithGitFolders branch from ff6a5d2 to 88e7e5e Aug 5, 2021
tmat
tmat approved these changes Aug 5, 2021
Copy link
Member

@tmat tmat left a comment

:shipit:


var gitDirectory =
Directory.Exists(dotGitPath) ? dotGitPath :
File.Exists(dotGitPath) ? ReadDotGitFile(dotGitPath) :
Copy link
Member

@tmat tmat Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test is failing - looking at it I think we should keep throwing if the file can't be read, rather then returning null.

var gitDirectory = Directory.Exists(dotGitPath) ? dotGitPath : ReadDotGitFile(dotGitPath);

if (!IsGitDirectory(gitDirectory, out var commonDirectory))
{
    return null;
}

Copy link
Member

@tmat tmat Aug 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With this change you'd still need to update the Submodules_Errors test - remove case S6:

[submodule ""S6""]             # sub6/.git is a directory, but should be a file
  path = sub6
  url = http://github.com

Copy link
Member

@tmat tmat Aug 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or rather update it - since in this case the directory is not a valid git directory (it's empty) so failure is expected of some sort

@tmat
Copy link
Member

@tmat tmat commented Aug 12, 2021

@crummel Committed a fix to the test.

@crummel
Copy link
Member Author

@crummel crummel commented Aug 12, 2021

Thanks! Sorry about that, I got pulled away by some servicing work.

@tmat tmat merged commit 5e2c4f1 into dotnet:main Aug 12, 2021
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants