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

don't fetch for unborn repositories #2598

Merged
merged 7 commits into from Sep 20, 2017
Merged

don't fetch for unborn repositories #2598

merged 7 commits into from Sep 20, 2017

Conversation

shiftkey
Copy link
Contributor

@shiftkey shiftkey commented Sep 19, 2017

Fixes #2592

This currently works by checking for HEAD ref after a clone and quietly moving on, but I'm open to different approaches to tackling this if we want to make the actions here more resilient and less noisy for this error case.

Any pointers for adding new tests to catch this regression would also be welcome!

@shiftkey shiftkey changed the title [WIP] don't fetch for unborn repositories don't fetch for unborn repositories Sep 19, 2017
Copy link
Contributor

@technoweenie technoweenie left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. I added a few notes. I also wrote some bash tests for the bottom of test/test-clone.sh:

begin_test "clone empty repository"
(
  set -e

  reponame="clone_empty"
  setup_remote_repo "$reponame"

  cd "$TRASHDIR"
  git lfs clone "$GITSERVER/$reponame" "$reponame" 2>&1 | tee clone.log
  if [ "0" -ne "${PIPESTATUS[0]}" ]; then
    echo >&2 "fatal: expected clone to succeed ..."
    exit 1
  fi
)
end_test

begin_test "clone bare empty repository"
(
  set -e

  reponame="clone_bare_empty"
  setup_remote_repo "$reponame"

  cd "$TRASHDIR"
  git lfs clone "$GITSERVER/$reponame" "$reponame" --bare 2>&1 | tee clone.log
  if [ "0" -ne "${PIPESTATUS[0]}" ]; then
    echo >&2 "fatal: expected clone to succeed ..."
    exit 1
  fi
  exit 1
)
end_test

Confirmed they fail on master, and pass in this PR.

}
} else {
Print("Repository is unborn, unable to fetch any LFS assets")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure a message is even necessary in this case. I think it should be silently skipped if the repository is empty. The output from the wrapped git clone already tells you the repository is empty:

    Cloning into bare repository 'clone_bare_empty'...
    warning: You appear to have cloned an empty repository.

Exit("Error performing 'git lfs pull' for submodules: %v", err)
ref, err := git.CurrentRef()

if ref != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a common Go idiom to check for err == nil instead of ref != nil. You could even use if ref, err := git.CurrentRef(); err == nil {.

@shiftkey
Copy link
Contributor Author

  if [ "0" -ne "${PIPESTATUS[0]}" ]; then
    echo >&2 "fatal: expected clone to succeed ..."
    exit 1
  fi
  exit 1

i see what you did there @technoweenie 😈

@shiftkey
Copy link
Contributor Author

🍎

@technoweenie
Copy link
Contributor

I guess Circle doesn't like your fork or something? We use Circle for Mac, and I can confirm this works locally, so I'm merging it.

@technoweenie technoweenie merged commit 72b3f4b into git-lfs:master Sep 20, 2017
@shiftkey shiftkey deleted the dont-fetch-for-empty-repo branch September 20, 2017 23:24
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.

None yet

2 participants