GH-127: Modify do/while in GitFindRootFromPath to avoid infinite loop

[kcamp]

This change addresses GH-127 to look at the current iteration of parentDir and provides a graceful termination consistent with the path simply not existing.

Using System.IO.DirectoryInfo seemed to be the most straightforward/deterministic approach here; if there's a preferred approach, please feel free to suggest something different.

[pascalberger]

@kcamp Thanks for fixing this 👍

Code LGTM. Would it be possible for you to also add a test case testing the reported issue?

[kcamp]

@pascalberger No problem, got those added. Let me know if there's anything else you'd like.

[kcamp]

Looking into the test failures on the latest push. I think it's going to wind up being environmental on the Travis builds.

[kcamp]

@pascalberger I'm at a bit of a loss for how to address this. In the latest Travis OSX job we get the following:

========================================
Git-Find-Root-From-Path-TempDirectory
========================================
Attempting to resolve Git root directory from temp directory '/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/'...
...
Error: One or more errors occurred.
	Path at '/var/folders/bb/n7t3rs157850byt_jfdcq9k80000gn/T/' should not be a valid Git repository.  Found Git root at '.'.

To me, this seems unexpected (in terms of finding a valid git repository at that path), but I am not familiar with how Travis is configured. The behavior definitely seems to indicate that Repository.IsValid(".") is evaluating true for that path and the loop is being broken gracefully without necessarily invoking the fix applied here.

Does this simply become a matter of scoping this new test to the Local-Tests task chain? I added it to both Default-Tests and Local-Tests for good measure. I patterned it after the Git-IsValidRepository-TempDirectory tests, so if we need to make the new test a bit more deterministic, I can start digging in on that in the next few days.

edit: I changed the scoping of the tests around a bit as a what-if and found that the CI builds are actually running Local-Tests due to how the targets are being passed from the respective runners + evaluated for the tests. So I think what we'd have to do to scope the tests to escape inclusion in CI (short of reworking for determinism) would be to include them in Default-Tests and not Local-Tests.

Ideas or advice on how to proceed? Thanks! 👍

[nils-a]

@kcamp Thanks for your contribution. As far as I can see, the build fails because the fix you implemented does not work on non-windows systems.

The main problem seems to be the line var parentDir = fsPath.Combine("../").Collapse(); which on non-windows systems produces . as parent for /tmp. (And it will produce C:\ as parent for C:\ - which explains the infinite loop you observed.)

I have put up a solution that could be used to solve this at cake-build/cake#3387 (reply in thread)

Are you in a position to rebase this PR and implement a more general solution?

[kcamp]

@nils-a Yes, I can rework based on what you've commented. Thanks for the feedback 👍

[nils-a]

@kcamp sorry for being so rude and implementing the changes myself, but I wanted to get this into the next release. (Which should come soonish...)

[devlead]

@nils-a looks good to me, just need a rebase.

[augustoproiete]

LGTM

[augustoproiete]

Suggested change

	if (context.Environment.Platform.Family == PlatformFamily.Windows) {
	// no more parents
	return null;
	}
	if (context.Environment.Platform.Family == PlatformFamily.Windows)
	{
	// no more parents
	return null;
	}

nitpick / C# brackets police 🛃

The requested test was added.

[nils-a]

@kcamp your changes have been merged, thanks for your contribution 👍