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

Make sha work for shallow git copies #1048

Merged
merged 1 commit into from Apr 11, 2016

Conversation

Projects
None yet
5 participants
@nparley
Contributor

nparley commented Jan 26, 2016

Fix fetching the latest git commit so that it also works for shallow git copies (e.g. ropensci/git2r#219). Fixes #1046

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 29, 2016

Thanks for looking into the problem and the fix. This actually will happen often in practice because travis does a shallow clone of the repository for every build.

It would be great to have a test for this case, though I would prefer it be of a local git repository rather than a remote one.

Could you also add a note to NEWS.md.

@nparley

This comment has been minimized.

Contributor

nparley commented Jan 29, 2016

@jimhester writing a test for this is not actually straight forward as git2r does not support creating shallow clones (e.g. ropensci/git2r#129). So the git repo to test against would have to be made another way, for example with the command line, or package a repo inside test?

@nparley

This comment has been minimized.

Contributor

nparley commented Jan 29, 2016

@jimhester also would the note go under 1.10.0.9000 in the bug fixes, thanks.

@hadley

This comment has been minimized.

Member

hadley commented Jan 29, 2016

Oops, I messed up the headings. Just put it directly under 1.10.0.9000

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 29, 2016

@nparley I would make a dummy package with a shallow checkout using command line git and put it in tests/testthat, then use that for testing.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 29, 2016

You'll probably need some trick with the .git file, AFAIR that is ignored/complained about by R CMD build or check. So you might need to put in a git file without a dot, and then copy it to .git in the test, and then remove it in on.exit().

I am not completely sure, but don't be surprised if it happens.

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 29, 2016

Good point Gabor, an easy fix is to use a --bare clone, so you don't have any .git directory. We don't actually need any files for this test, just the git objects.

git clone --depth=50 --bare https://github.com/hadley/devtools.git /tmp/devtools
Rscript -e 'r <- git2r::repository("/tmp/devtools")' -e 'git2r::branch_target(git2r::head(r))'
[1] "21d5d9401101dcf2baada3db424e4de0e9c7f869"
@jennybc

This comment has been minimized.

Member

jennybc commented Jan 30, 2016

What @gaborcsardi says will definitely happen (.git will be Rbuildignored). I wrestle with this elsewhere and can't necessarily use @jimhester's bare repo solution. You know how you can invert gitignores with !? Is there anything like that but for Rbuildignore? So you can Rbuild-UN-ignore things instead of fiddling with the names? I can't find any evidence of such but hope I am just missing something.

@gaborcsardi

This comment has been minimized.

Member

gaborcsardi commented Jan 30, 2016

@jennybc I don't think that you can do this with .Rbuildignore. But you can create a .git or the whole test folder dynamically. Maybe the best is to just zip up the folder and extract it in the test. Quite cumbersome....

@jimhester

This comment has been minimized.

Member

jimhester commented Jan 30, 2016

@jennybc You can store .git with other names, see variety of answers at http://stackoverflow.com/questions/505467/can-i-store-the-git-folder-outside-the-files-i-want-tracked

I even wrote a post about this a few years ago (http://www.jimhester.com/blog/2012/11/08/external-git/) although the solution there still has a .git file, so won't help here.

@nparley

This comment has been minimized.

Contributor

nparley commented Feb 2, 2016

I have added a test for the shallow clone repo and added a shallow bare copy into the testthat directory to test against. CMD check is now giving the note:

Found the following non-portable file paths:
  devtools/tests/testthat/testBareDepthRepo/objects/pack/pack-c4e0f1d1d68408f260cbbf0a533ad5f6bfd5524e.idx
  devtools/tests/testthat/testBareDepthRepo/objects/pack/pack-c4e0f1d1d68408f260cbbf0a533ad5f6bfd5524e.pack

also the test

pkg_sha <- git_sha1(path='testBareDepthRepo')
expect_equal(pkg_sha, '21d5d94011')

assumes something of the path and so might not work in all situations?

@jimhester

This comment has been minimized.

Member

jimhester commented Feb 2, 2016

The Found the following non-portable file paths: NOTE occurs because the filename is more than 100 characters long. See (https://stat.ethz.ch/R-manual/R-devel/library/utils/html/tar.html) for details.

nchar("devtools/tests/testthat/testBareDepthRepo/objects/pack/pack-c4e0f1d1d68408f260cbbf0a533ad5f6bfd5524e.idx")
#> [1] 104

renaming the testBareDepthRepo directory to be shorter should fix the NOTE.

testthat tests are run with tests/testthat as the working directory, so that test should be correct.

You can remove all the sample hooks from tests/testthat/testBareDepthRepo/hooks/*, might as well not include them as they won't be used.

@nparley

This comment has been minimized.

Contributor

nparley commented Feb 3, 2016

OK that's solved the non-portable file paths problem and the sample hooks have been removed.

@hadley

This comment has been minimized.

Member

hadley commented Feb 5, 2016

Can you please merge/rebase and squash?

git_sha1() Fix fetching the latest git commit so that it also works for
shallow git clones, i.e. git clones which make use of depth. (#1048, #1046, @nparley)
@nparley

This comment has been minimized.

Contributor

nparley commented Feb 8, 2016

Done

@hadley

This comment has been minimized.

Member

hadley commented Apr 11, 2016

Thanks!

@hadley hadley merged commit b98ee44 into r-lib:master Apr 11, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment