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.

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

Initial attempt at #42 #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shardulc
Copy link
Member

No description provided.

sync.py Outdated

self.dir = os.path.join(clone_dir, name)
self.check_call = functools.partial(subprocess.check_call, cwd=self.dir)

def clone(self, init_submodules=True):
if not os.path.isdir(os.path.join(self.clone_dir, self.name)):
logging.info('Cloning meta repository %s', self.name)
subprocess.check_call(shlex.split('git clone --depth 1 [email protected]:{}/{}.git'.format(ORGANIZATION, self.name)), cwd=self.clone_dir)
subprocess.check_call(shlex.split('git clone --depth {} [email protected]:{}/{}.git'.format(self.sync_depth, ORGANIZATION, self.name)), cwd=self.clone_dir)
if init_submodules and self._has_submodules():
self.check_call(shlex.split('git submodule update --depth 1 --init --jobs 8'))
Copy link
Member Author

Choose a reason for hiding this comment

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

Should the same depth apply to this? Should we have a different parameter?

Copy link
Member

Choose a reason for hiding this comment

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

Same depth. There shouldn't be any need for a separate one.

@shardulc
Copy link
Member Author

ref. #42. Also, this is completely untested.

Copy link
Member

@sushain97 sushain97 left a comment

Choose a reason for hiding this comment

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

Looks good aside from the minor nit. Should indeed be tested :)

sync.py Outdated
self.clone_dir = clone_dir
self.name = name
self.submodules = submodules
self.author = author
self.sync_depth = sync_depth
Copy link
Member

Choose a reason for hiding this comment

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

Do you mind changing this to just depth? The sync prefix inside the sync class is a bit redundant.

@shardulc
Copy link
Member Author

I can test this on mock-apertium. What depths should I test with (i.e., what is the problem that this is supposed to solve)?

@sushain97
Copy link
Member

sushain97 commented Mar 30, 2018

#42 (comment)

I think the problem is when things get into an inconsistent state and --depth 1 is too shallow to fix them/causes errors but I can't remember the exact problem to save my life...

@shardulc
Copy link
Member Author

@sushain97 Ping about testing this. mock-apertium has been deleted?

@sushain97
Copy link
Member

sushain97 commented Apr 12, 2018 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants