[FIX] Freeze correct revision when using git merges

[tremlin]

The current code doesn't handle freezes of git repositories correctly when

• the revision is not specified as a tag, and
• the merge option is used.

The merge option in git produces local commits hashes which are not reproducible on other systems. Currently the freeze-to command references this local merge commit. This PR attempts to solve this issue by calling git merge-base to identify the correct base revision before the merges.

It's probably not an optimal solution because it requires API changes to all VCS classes, so I am happy to hear your suggestions.

[coveralls]

Coverage decreased (-0.01%) to 86.344% when pulling 6ae4b0a7e68909c6da7ddc91ab8dbf68c5aecdc4 on initOS:master-FIX_git_merge_parent into b80c95a7770c2902d6aaf2305c68036dd7715862 on anybox:master.

[rvalyi]

Hello no time to dig into this right now, but I would like to point a mature project that properly converge git repos. It is chef and here is what the do for the initial checkout:

https://github.com/chef/chef/blob/master/lib/chef/provider/git.rb#L158

and for the updates:

https://github.com/chef/chef/blob/master/lib/chef/provider/git.rb#L181

The entry point where checkout or update will be decided is here:

https://github.com/chef/chef/blob/master/lib/chef/provider/git.rb#L96

I advise copying from Chef as much as possible as they are really a mature project (we were using it at Akretion before using that recipe and certainly the fact that the recipe project re-invent VCS resources on its own was one of the point that made us hesitate to do the switch...).

[tremlin]

Thanks for the hints. As far as I can see the git-provider of chef doesn't offer a merge features like this recipe does. The logic for checking out and updating works fine, the only problem is the freeze-to when a merge was applied before.

[lmignon]

@tremlin at ACSONE we use git-aggregator to manage the aggregation of git repositories in a reconciled one. https://pypi.python.org/pypi/git-aggregator.

[gracinet]

Hi, all of this is interesting. I'm currently on the road, wanted precisely to work on thé recipe this week, I'll take a look.

PS yes it has also always bothered me to take care of the vcses, lots of work.

Le 22 août 2016 09:11:44 GMT+02:00, "Laurent Mignon (ACSONE)" [email protected] a écrit :

@tremlin at ACSONE we use git-aggregator to manage the aggregation of git repositories in a reconciled one. https://pypi.python.org/pypi/git-aggregator.

You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/anybox/anybox.recipe.odoo/pull/79#issuecomment-241331073

Envoyé de mon appareil Android avec K-9 Mail. Veuillez excuser ma brièveté.

[gracinet]

@lmignon, i see you are the author of gitaggregator, that should help :)

Le 22 août 2016 09:11:44 GMT+02:00, "Laurent Mignon (ACSONE)" [email protected] a écrit :

@tremlin at ACSONE we use git-aggregator to manage the aggregation of git repositories in a reconciled one. https://pypi.python.org/pypi/git-aggregator.

You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/anybox/anybox.recipe.odoo/pull/79#issuecomment-241331073

Envoyé de mon appareil Android avec K-9 Mail. Veuillez excuser ma brièveté.

[gracinet]

@tremlin first of all thanks for the contribution, this is a problem I didn't have the courage to tackle for a very long time, mostly because I suspected it could need a much larger refactor than that, for instance storing for freeze the revision produced by the main fetch addons or version option.

What makes the matter so complicated is that the merge command of other VCSes don't produce a commit, but local changes ready to be committed. Actually, IIRC, it's plain impossible to use the freeze and merges together with them.

The change of API for all VCS classes doesn't bother me per se, but I have a few reservations:

• I don't find the name revspec for the new kwarg to be telling (granted parents would not for anyone not familiar with Mercurial). If I understand correctly what happens with your changes (correct me if I'm wrong), in the case where the addons option specifies a branch (say master), then the freeze will recognize that it's not a "local fixed revision" and pass it, and that's about the only case. Then it works because the recipe creates local branches that are named after the one in the (unique) remote. So something like parents(in_remote_branch=revspec) would be more appropriate and somehow would make parents itself less awkward.
• If we claim that this is supported, which I definitely want to, I'd certainly require a unit test for this. I can help you with that if need be, but I won't be available to work on the recipe until 2016-09-12

So, I'm not going to merge this right now (for 1.9.2b1), but I hope to do so for 1.9.2, which I plan to cut mid-september.

And then I'll be thinking really hard for 2.0 about:

• evaluating if we can integrate with git-aggregator
• switch to a completely separate freeze/extract logic, since the current one has been silly-but-good-enough for years.

[gracinet]

@rvalyi I'll check what Chef does in that department, but since you know it well and I'm not a fluent Ruby reader, it'd probably more efficient if you could suggest improvements on the VCS subsystem of the recipe or on git-aggregator.

[sbidoul]

Here are some precisions on our workflow with git-aggregator:

• declare the required merges in a git-aggregator config file
• use gitaggregate to execute the merges and push the result to dedicated remote branches named after the project
• reference such remote branches in the buildout.cfg or requirements.txt
• when releasing, buildout freeze or pip freeze and place tags on the resulting shas so they are never garbage collected

Long story short: we abandoned the recipe merge feature, which indeed does not easily permit reproducible builds, to replace it with an external tool that does one only thing (and hopefully does it well).

[gracinet]

Thanks for the indications @sbidoul.

It's obvious there must be a limit as to what the recipe should take care of. It's been easy to consider pushing to be out of scope. Further questions, if you don't mind.

• Do you automatically place those tags ? Do you reference them in buildout.cfg or requirements.txt for direct use ? Your wording suggests that you don't use them more than a protection against the GC.
• Does your CI bot also run gitaggregate, or is it supposed to be a manual (conscious) operation from a developer, for instance wishing to integrate a new security patch ?

[sbidoul]

@gracinet I don't mind at all.

Do you automatically place those tags ? Do you reference them in buildout.cfg or requirements.txt for direct use ? Your wording suggests that you don't use them more than a protection against the GC.

We have a script that automatically place those tags on all shas present in a requirements.txt. https://github.com/acsone/acsoo/blob/master/acsoo/tag_editable_requirements.py We reference the branches in buildout.cfg/requirements.txt and let buildout/pip freeze automatically without manual intervention. Indeed the tags are only there to prevent these shas to be GC'ed. They are also useful for further reference by humans but not required in frozen buildout.cfg/requirements.txt.

Of course the frozen requirements are tagged on the main project too, and that is sufficient to reproduce the exact same build.

Does your CI bot also run gitaggregate, or is it supposed to be a manual (conscious) operation from a developer, for instance wishing to integrate a new security patch ?

No, the CI bot does not need to run gitaggregate. It is exactly as you say, a manual (conscious) operation from a developer, for instance wishing to integrate a new security patch (or refresh to get the latest HEAD from Odoo/OCA).