-
Notifications
You must be signed in to change notification settings - Fork 57
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
Proposal: patches and diffs support #34
Comments
This proposal makes sense to me. I would even say that diff/patch it is safer than merge, because I have seen people doing a merge of a PR branched off of a more recent version of their base branch and thinking git-aggregate would apply the diff while it was actually getting more commit from the main branch too. So 👍 |
I've done similar thing for Dockery Odoo, it should also allow to use a patch file locally (out of a directory). |
Please do, I still didn't have time to do it. |
For those interested to experiment with this approach today, |
This is a useful case for Doodba users, where possibly you want to pre-merge a patch done for Odoo v11 into your Odoo v13 deployment, before Odoo approves it. Using just git branches is not much helpful in this case because no git history is shared, so conflicts would arise always. In fact, this might be the best way to apply merges *always*. That's to be investigated. Upstream support for diffs and patches is being tracked in acsone/git-aggregator#34, but in the mean time this option is available today, so it's better to make it more obvious. @Tecnativa TT19951
This is a useful case for Doodba users, where possibly you want to pre-merge a patch done for Odoo v11 into your Odoo v13 deployment, before Odoo approves it. Using just git branches is not much helpful in this case because no git history is shared, so conflicts would arise always. In fact, this might be the best way to apply merges *always*. That's to be investigated. Upstream support for diffs and patches is being tracked in acsone/git-aggregator#34, but in the mean time this option is available today, so it's better to make it more obvious. @Tecnativa TT19951
For my own clarity, I start with typing the problem: @dataclass
class TargetDirectory(DataClassYAMLMixin):
origin: str
upstreams: List[Upstream] = field(default_factory=list)
target: str
postprocess: List[str]
@dataclass
class Upstream(DataClassYAMLMixin):
name: str = "upstream"
url: str
merges: List[str] = field(default_factory=list) How do we ensure the proposed patches don't conflict with merges? |
I think that is true. Could we go so far to state, that the additional history a merge brings us is not even a necesary feature? This presumes we would need a stronger semantic separating:
Does this make sense to you? Is there a use case that brakes when history is only preserved for a primary upstream? see also: #43 |
Merge 1st, patch 2nd. Error = exit code 1. It's quite simple, you don't need to ensure anything AFAICS... 🙄
IMHO patches should just be a list of URLs or paths. If it's just a path, it could be absolute or relative to the repos.yaml file. |
@sbidoul how do you think about patches be safer than merge? I agree, but should that opinion translate into some action? Maybe not on this PR, though. But it might still validly inform the implementation. |
This is one idea that came to my mind. The quick summary is to be able to do things like this:
The effect of these new configuration items would be the same as executing:
The point of adding
patches
is maybe not so much important because almost the same you can get with just plain git, and it shares the same limitations. For example, thedepth: 100
key will make thegit am
command fail if the cloned shallow repository doesn't have the parent commit.1However, adding
diffs
would enable us to merge patches that don't share parent commits. In this concrete case, I'm applying a patch for branch 11.0 on top of the 12.0 branch, without sharing a parent commit in git history. This enables both a fast repo download and a flexible merging system that doesn't depend on the present branch but only on the present code.1 For this concrete case, if you open https://github.com/odoo/odoo/pull/37142.patch you'll see the required parent commit is 23c3fb1abce11d3d5b71cf31a3925198b53d1ea9.
@Tecnativa TT19524
The text was updated successfully, but these errors were encountered: