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

Shallow git clone #4939

Merged
merged 2 commits into from
Dec 4, 2018
Merged

Shallow git clone #4939

merged 2 commits into from
Dec 4, 2018

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 29, 2018

Close #1888

Tested this locally with a couple of projects, and I didn't find any problems. I also experiment with this outside rtd. The only downside I can see is people using an extension that needs to read the git history. So, this still needs a decision:

  1. Merge as it is
  2. Add a feature flag and test some projects before
  3. Make this a project option, so users can enable/disable it (we can have this on by default)

@codecov
Copy link

codecov bot commented Nov 29, 2018

Codecov Report

Merging #4939 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4939      +/-   ##
==========================================
+ Coverage   76.81%   76.81%   +<.01%     
==========================================
  Files         158      158              
  Lines       10092    10093       +1     
  Branches     1270     1270              
==========================================
+ Hits         7752     7753       +1     
  Misses       1998     1998              
  Partials      342      342
Impacted Files Coverage Δ
readthedocs/vcs_support/backends/git.py 80.95% <100%> (+0.11%) ⬆️

@ericholscher
Copy link
Member

ericholscher commented Nov 29, 2018

I do wonder if using a higher depth is a bit better. I mentioned back here that travis uses depth=50, to better support extensions that read history, etc.

I think this is a great idea though. I'd be happy to either feature flag it, or just ship it and see what happens :)

@stsewd
Copy link
Member Author

stsewd commented Nov 30, 2018

I'm fine with 50 as depth, and skip feature flag, but I wonder if those extensions really exist p: I'll try to search tomorrow.

@stsewd
Copy link
Member Author

stsewd commented Nov 30, 2018

I just found https:/OddBloke/sphinx-git and https:/bitprophet/releases, not sure if some use those extensions to generate a full changelog, if not, looks like 50 as depth is good enough

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

This is a really good change and will save a lot of data (and size) in our builders.

I don't think that this will break, but I'm not sure though. If you already tested it with different types of projects (small and big ones) I suppose we can ship it as is and see what happen with the rest.

@humitos
Copy link
Member

humitos commented Dec 3, 2018

I know that git is the most used here, but what about the other VCS? Do we want to implement a similar approach with them?

@ericholscher
Copy link
Member

I think we should focus on git. It's definitely an 80/20 (or more) situation, so it gives us the most benefit.

@stsewd
Copy link
Member Author

stsewd commented Dec 3, 2018

I tested this with projects that have a lot of versions, but I'll test this again because I want to be sure that this works with #4433

@stsewd
Copy link
Member Author

stsewd commented Dec 3, 2018

Tested this with master, still works

@ericholscher
Copy link
Member

I wonder if we can make this a default depth, but allow it to be user configurable, so that we have an answer if someone needs more history? We can't have this be a YAML option because we don't have the code on disk yet. I think we can ship this with what we have, but I do imagine we'll need a bit more configurability with it over time.

@humitos
Copy link
Member

humitos commented Dec 4, 2018

We can't have this be a YAML option because we don't have the code on disk yet. I think we can ship this with what we have, but I do imagine we'll need a bit more configurability with it over time.

To solve this we can:

  1. make it a db option, or
  2. implement a feature flag (but it's not possible to set by the user, though)

Although, I don't think we will have any problem in the near future, at least.

@stsewd
Copy link
Member Author

stsewd commented Dec 4, 2018

We can make this a web option, something like having all the history or just 50 commits

@stsewd stsewd merged commit 92fc83e into readthedocs:master Dec 4, 2018
@stsewd stsewd deleted the shallow-clone branch December 4, 2018 16:02
@humitos
Copy link
Member

humitos commented Dec 6, 2018

Considering that we are using shallow clone now, shouldn't we use also --shallow-submodules?

   --depth <depth>
      Create a shallow clone with a history truncated to the specified number of commits. Implies --single-branch unless --no-single-branch is given to fetch the histories near the
      tips of all branches. If you want to clone submodules shallowly, also pass --shallow-submodules.

@stsewd
Copy link
Member Author

stsewd commented Dec 6, 2018

That only works when we are cloning the repo and the submodules at the same time, but we don't do that anymore. As the submodules update is separated, I found that we need to update the subdmodules' config to archive this https://stackoverflow.com/questions/2144406/how-to-make-shallow-git-submodules but that looks a little hacky, not sure

ericholscher pushed a commit that referenced this pull request Dec 26, 2018
This adds a project feature that allows us to use a standard clone and
fetch rather than the shallow clone/fetch introduced in #4939.
Eventually we should move this to the web UI, but doing so requires some
work to make sure, for example, that git options are only show when
'Project.repo_type' is 'git'.

Signed-off-by: Stephen Finucane <[email protected]>
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.

3 participants