[WIP] Git support: first steps

[wilzbach]

This is a WIP approach to support to new features.
Based on a label (or maybe comment, but label is easier for now as authorization is implicitly given), support the following "actions":

• "bot-rebase" (label) -> "git clone" -> rebase to latest master -> "git push" (if no failure happened)
• "bot-squash" (label) -> "git clone" -> squash all commits -> "git push"

Imho both are very useful features

• rebase can be used to re-trigger Jenkins
• squash can be used by reviewers to automatically squash everything before applying "auto-merge"

This PR implements only "rebase", but adding "squash" later won't be difficult

What's there?

• Implemented SmartGit server protocol (only the needed parts, of course)
• Added unittest abstraction over SmartGit server

This means that we can fully handle git push commands as "faked" server, inject the requests and control the responses like we already do for other mocked APIs.

> git push -v --force http://0.0.0.0:9006/aa/bar HEAD:Issue_8573

The command will be handled by the mocked backend :)

// first request by the git client
// server returns a list of all refs it knows
gitInfoRefs = (string user, string repo) {
    "requesting refs".writeln;
    return [
        GitInfoRef("6ed2e15cf4a35e274adb8385b3ee8125326509f9", "refs/heads/foo"),
        GitInfoRef("6e79d22fdfda446601d969ce77e406b9a5506de9", "refs/heads/Issue_8573"),
        GitInfoRef("6e79d22fdfda446601d969ce77e406b9a5506de8", "refs/heads/master"),
    ];
};

// git client "reports" new refs to the server
gitReportRefs = (ClientReq clientReq) {
    clientReq.writeln;
    // GitClientRequest([], [Command(update, "6e79d22fdfda446601d969ce77e406b9a5506de9", "e4afacadb8a291f51444e948dd7b53592e799dbe", "refs/heads/Issue_8573")])
    return [
        GitReportRef(GitReportRef.status.ok, "refs/heads/Issue_8573")
    ];
};

What's missing?

• fix reason for Vibe.d blocking (not sure about this, but std.process.wait seems to block the entire Vibe.d thread even if we are in a new fiber (e.g. runTask), on a worker thread (e.g. runWorkerTask) and when spawned with std.concurrency`)
• add simple, "immutable" test repository with a couple of small PRs
• extensive testing (on GH with dummy repo)
• remove hard-coded gitURL
• add support for authentication (this requires saving the bot's password or private key on heroku)
• final cleanup

[dlang-bot]

Thanks for your pull request, @wilzbach!

[CyberShadow]

Implemented SmartGit server protocol (only the needed parts, of course)

That seems like overkill. Why not just use a bare repo?

[CyberShadow]

spawnProcess > spawnShell in all situations, unless you need to use a shell built-in or run a user-supplied shell command

[CyberShadow]

Why?

[wilzbach]

This is just to test things - as explained if I use wait Vibe.d blocks entirely (and thus even the dummy git server backend doesn't answer).

[CyberShadow]

Vibe really needs an async wait for processes. I considered using Vibe for a project before I discovered it didn't have this. In any case, you can always wait in a thread.

[wilzbach]

Vibe really needs an async wait for processes. I considered using Vibe for a project before I discovered it didn't have this

Do you care enough to write an issue?

In any case, you can always wait in a thread.

I tried that before, it didn't work, but I will try again tonight.

[CyberShadow]

I tried that before, it didn't work, but I will try again tonight.

It's really bizarre if that wouldn't work. If it comes to that, you could always create a native pthread that not even the D runtime knows about.

[CyberShadow]

Do you care enough to write an issue?

Where would I even file it? I think it needs to be implemented in the driver. It needs to be implemented as a SIGCHILD handler on POSIX and WaitFor*Object* on Windows.

[CyberShadow]

std.process.wait seems to block the entire Vibe.d thread even if we are in a new fiber

BTW, zero surprise there - the fiber is not going to return to the scheduler because wait is a blocking OS API essentially.

[MartinNowak]

Think you can use a file event descriptor combined with tryWait.
Function createFileDescriptorEvent - vibe.d

    fcntl(p.stdout.fileno, F_SETFL, O_NONBLOCK);
    scope readEvt = createFileDescriptorEvent(p.stdout.fileno, FileDescriptorEvent.Trigger.read);
    while (readEvt.wait(5.seconds, FileDescriptorEvent.Trigger.read))
    {
        auto rc = tryWait(p.pid);
        if (rc.terminated)
        { /* do something */ }
    }

Think the event will trigger when the process terminates, not too sure though.

Where would I even file it? I think it needs to be implemented in the driver. It needs to be implemented as a SIGCHILD handler on POSIX and WaitForObject on Windows.

On the issue tracker? vibe-d/vibe.d#1832
Combining signalfd, epoll, and handling SIGCHILD shouldn't be too hard, but maybe you can leave some more details on that issue. Indeed async subprocesses would be a welcome addition.
Shouldn't be too hard to add, and since you

[CyberShadow]

Think you can use a file event descriptor combined with tryWait.

Yeah, timers (and threads) are always a solution, but such an unsatisfying one.

Timers are imprecise, and threads are not cheap.

Combining signalfd, epoll, and handling SIGCHILD shouldn't be too hard, but maybe you can leave some more details on that issue.

I started a thread on D.learn a while ago here, it should have some useful discussion and links: http://forum.dlang.org/post/[email protected]

Shouldn't be too hard to add, and since you

Did you mean to add something here?

[wilzbach]

That seems like overkill. Why not just use a bare repo?

Why? I explicitly want to test what the bot will send to GitHub, so mocking the API seemed very reasonable.
Btw this is the way we test the other bot actions as well and force rebasing is a potentially dangerous action, hence I want to have an extensive test coverage before testing this in the wild.

[CyberShadow]

Why?

Well, it tests implementation detail of third-party software, relies on the git implementation used to behave as it does now, and it would make it overly onerous to write tests for more complicated functionality in the same vein.

It would make sense if dlang-bot had its own git client and SmartGit protocol implementation, but it doesn't (and shouldn't) - it uses the git shell commands, so I think it would be more logical to test their effects, not implementation details.

IMHO. shrug

[wilzbach]

It would make sense if dlang-bot had its own git client and SmartGit protocol implementation, but it doesn't - it uses the git shell commands, so I think it would be more logical to test their effects, not implementation details.

How would you test the effect of git push?

[CyberShadow]

You can push to a bare repo on the filesystem.

[PetarKirov]

What do you guys think about using comments, instead of labels for this? Specifically about squashing, it would be much better if a reviewer could manually specify a commit message.

[JinShil]

What do you guys think about using comments, instead of labels for this?

I kindof like the idea of giving some functionality to contributors through the comments. If you want to build the basic infrastructure around labels, you could still have commands in the comments to label/unlabel a PR.