Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fetch only required remotes. Saves disk space and bandwidth by default. #6

Merged
merged 3 commits into from
Jan 3, 2017
Merged

Fetch only required remotes. Saves disk space and bandwidth by default. #6

merged 3 commits into from
Jan 3, 2017

Conversation

yajo
Copy link
Contributor

@yajo yajo commented Jan 2, 2017

With this simple patch, we will save lots of time, disk space and bandwidth by default, by only fetching required remotes. In projects like Odoo, this means saving thousands of commits between versions.

@Tecnativa

@coveralls
Copy link

coveralls commented Jan 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 88.129% when pulling 6e49d47 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@lmignon
Copy link
Member

lmignon commented Jan 2, 2017

@yajo Thank you for this nice contrib. Can you update the README.rst file to describe this new parameter and also to adapt the examples? Indeed with this change the example with the merge of a specific commit is not right...

@coveralls
Copy link

coveralls commented Jan 2, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 87.943% when pulling 3e18c61 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 87.234% when pulling 2d4ac4d on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@coveralls
Copy link

coveralls commented Jan 2, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling 797aad1 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling 797aad1 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling 797aad1 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@yajo
Copy link
Contributor Author

yajo commented Jan 2, 2017

@lmignon Thanks for your interest and patience, this should be ready to merge now. You have new tests, all checks are ✔️.

However I think you should raise the version when pushing this to pypi. This change can introduce an incompatibility in some specific scenarios, where they would still be working if they froze their requirements.

@lmignon
Copy link
Member

lmignon commented Jan 2, 2017

@yajo Thank you to you for the contribution!

However I think you should raise the version when pushing this to pypi. This change can introduce an incompatibility in some specific scenarios, where they would still be working if they froze their requirements.

The version on pypi is 1.0.0 and the first change done after the first release has been to bump the version to 1.0.1.dev0 😏 faafa0d
The version will be bumped again before the release... may be 2.0.0...

@lmignon lmignon requested a review from sbidoul January 2, 2017 13:16
sbidoul
sbidoul reviewed Jan 2, 2017
View reviewed changes
git_aggregator/repo.py
@@ -33,7 +33,7 @@ class Repo(object):

_git_version = None

def __init__(self, cwd, remotes, merges, target,
def __init__(self, cwd, remotes, merges, target, fetch_all=False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To preserve backward compatibility it's better to add new optional parameters at the end.

sbidoul
sbidoul requested changes Jan 3, 2017
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yajo thanks for this contrib. Just one minor remark regarding the new constructor parameter, otherwise looks good to me.

@yajo
Copy link
Contributor Author

yajo commented Jan 3, 2017

Fixed! 😊

@coveralls
Copy link

coveralls commented Jan 3, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling af14fb6 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@yajo
Fix README mess.
23c5647 
- Unify tabs and spaces.
- Delete remainders of Markdown markup.
- Correct indentation for all blocks.
- Separate configuration from usage instructions.
- Add sections where needed.
@yajo
Copy link
Contributor Author

yajo commented Jan 3, 2017

I was preparing another PR and found it out very hard to document because currently README is a little messy, so I took the chance and fixed it here too. I hope you don't mind. This should be ready now.

@coveralls
Copy link

coveralls commented Jan 3, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling 23c5647 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling 23c5647 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.9%) to 89.007% when pulling 23c5647 on Tecnativa:master-fetch-only-required into d2782cf on acsone:master.

@lmignon
Copy link
Member

lmignon commented Jan 3, 2017

@yajo

I hope you don't mind.

For sure no!

sbidoul
sbidoul approved these changes Jan 3, 2017
View reviewed changes
Copy link
Member

@sbidoul sbidoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@lmignon lmignon merged commit 115f7e9 into acsone:master Jan 3, 2017
@yajo yajo mentioned this pull request Jan 3, 2017
Merged
1 task
@yajo yajo deleted the master-fetch-only-required branch January 3, 2017 11:58
yajo added a commit to Tecnativa/doodba that referenced this pull request Jan 24, 2017
@yajo
Do not use own git-aggregator fork.
5916014 
Now acsone/git-aggregator#6 and acsone/git-aggregator#7 are merged.

However, we still have to use a development version, so we hash-pin it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lmignon lmignon lmignon approved these changes

@sbidoul sbidoul sbidoul approved these changes

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yajo @coveralls @lmignon @sbidoul