Feature: Optional Git cmd support

[sharpSteff]

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Feature

⤵️ What is the current behavior?

No git cmd support

🆕 What is the new behavior (if this is a feature change)?

optional git cmd instead of Lib2GitSharp

💥 Does this PR introduce a breaking change?

should not, since its an additional feature

🐛 Recommendations for testing

Maybe provide a git cmd resource for unit testing.

All unit tests which does git interactions should also use the GitCmdDriver as IGitDriver

📝 Links to relevant issues/docs

#799

🤔 Checklist before submitting

• All projects build
• Relevant documentation was updated

[MarcBruins]

Cool! @skolima worked on this before and i think there where some issues he encountered

[sharpSteff]

I tested it on gitea and GitHub. I think the biggest benifit of this Feature is, that you can rely on your already working.gitconfig in case of special stuff like proxy.

Do you have any Idea how you run unittests for this?

[skolima]

#659 was extremely unreliable and I gave up - the unpredictability of this approach was not worth the gain (even though I have huge repositories where doing a shallow clone would be a huge speedup). There's little in terms of test coverage, though, so for the spike I was relying on running this by hand for various repositories. In order to be able to merge this, you'd need to write the tests as well (current NuKeeper libgit integration is lacking them - not ideal).

[sharpSteff]

@skolima thanks for the Feedback. I see this Feature as a fallback solution in case lib2git does not the job. any ideas how to get git cmd running on build servers?

[skolima]

What I've tried doing before was checking if %PATH% actually contains git already - this would be the most common situation, I think. If not, then the build definition would need to be changed to install it.

[MarcBruins]

Two things i noticed is that the GitDiscoveryDriver isn't updated and there are no docs update with this PR.

[sharpSteff]

Will fix that tomorrow

[skolima]

In StartGitProzess (typo!) you've got your own Process handling. This should be using ExternalProcess as other places that launch executables from NuKeeper do. 2 reasons for this: a ) consistency b) if there's any bugs in ExternalProcess, those are rather critical to how NuKeeper works, so, we need to fix them. And if this works, then definitely the logging support should be used here as well.

[skolima]

ExternalProcess.Run is async, you'd need (AFAIK) to await it, calling .Result directly leads to all kinds of problems. Which means the async keyword will need to propagate a bit up the call stack.

[sharpSteff]

@skolima calling .Result is indeed a blocking call, but in this case there should be no problems.

However a better approach as you mentioned, to include async

[sharpSteff]

@skolima managed to get rid of the .Result.

Also added some unit tests.
I still have to check whether everything runs also on Linux.
And in addition I have to finish the GitDiscoveryDriver

[MarcBruins]

There are still merge conflicts can you check them? Are you also confident that this works now?

[sharpSteff]

@MarcBruins
there are still 2 open points:

• update docu
• The IGitDiscoveryDriver is currently as a singleton implemented. Any ideas to inject dynamically the GitCmdDiscoveryDriver, when the command line is applied?

[sharpSteff]

updated docu

[sharpSteff]

@MarcBruins I'm confident everything works. We're already running our nukeeper on windows with git cli (repo/update) commands. I tested everything as well on Ubuntu. Also I added unit tests to be sure.

[StephanTuinder]

Would be nice to see a full-implementation of the GitCmdDriver.

[sharpSteff]

@StephanTuinder
What additional Features are needed ?

[StephanTuinder]

Weird, in my mind there where a lot of NotImplemented routines. Probably my branch was behind. (So you can ignore my remark)

While making my PR (#841) I did found a bug in the GitCmdDiscoveryDriver.cs, which wasn't caught by the tests. This I fixed and I extended the test to cover that. If you are interested: it is the ShouldGetRemotes test. Look in debug at the differences between remotes and classicRemotes in Debug when you run the test on your own fork.

[sharpSteff]

Never mind!

Great that you find that issue in GitCmdDiscoveryDriver. I'll take a look at the tests

[StephanTuinder]

I fixed it in my PR, so check there