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

Do not migrate the symlinks to LFS objects. #2983

Merged
merged 3 commits into from
Apr 27, 2018

Conversation

patrickmarlier
Copy link
Contributor

No description provided.

Copy link
Contributor

@ttaylorr ttaylorr left a comment

Choose a reason for hiding this comment

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

Hi @patrickmarlier, thanks for making this change. I think that the implementation looks great, as does your comment, which I very much appreciate 😄.

I know that it's nitpick-y, but do you think you could split this into a new helper and corresponding test in test/test-migrate-fixtures.sh and test/test-migrate-import.sh? I think that that file does a good job of keeping its test very focused, and I think that this change deserves its own test.

@patrickmarlier
Copy link
Contributor Author

Hi @ttaylorr ,

It is good to be picky. I created a new test and new fixture for the symlink. Unfortunately the Windows CI failed. I would imagine due to the symlink that is not a symlink in the Windows CI. Excerpt of the log:
"create mode 100644 link.txt"
So here I can propose to skip the test in case of Windows or find why the symlink is created as a regular file. In both case, some help is welcome because it would take time to test on Windows.

@ttaylorr
Copy link
Contributor

ttaylorr commented Apr 27, 2018

It is good to be picky. I created a new test and new fixture for the symlink.

Thank you 😄.

So here I can propose to skip the test in case of Windows or find why the symlink is created as a regular file. In both case, some help is welcome because it would take time to test on Windows.

It can be a little tricky to use symlinks correctly on Windows. What about modifying the index directly? I propose something like this, in test/testhelpers.sh:

add_symlink() {
  local src=$1
  local dest=$2

  prefix=`git rev-parse --show-prefix`
  hash=`printf "$src" | git hash-object -w --stdin`

  git update-index --add --cacheinfo 120000 "$hash" "$prefix$dest"
  git checkout -- "$dest"
}

(with modifications from https://coderwall.com/p/z86txw/make-symlink-on-windows-in-a-git-repo).

We could call that in your new test and I think that it would work OK. If not, you are welcome to skip this test for now and I can come back to it later.

EDIT: to skip a test on Windows only, look here.

begin_test "my-test-name"
(
  set -e

  if [[ $(uname) == *"MINGW"* ]]; then
    echo >&2 "skip"
    exit 0
  fi

  # ...
)
end_test

@patrickmarlier
Copy link
Contributor Author

I like the add_symlink proposal. I will do that.

In Windows, "ln -s" might not create a symbolic link depending on the
configuration.
Copy link
Contributor

@ttaylorr ttaylorr left a comment

Choose a reason for hiding this comment

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

Thanks for your wonderful contribution, @patrickmarlier! I appreciate you responding to all my feedback. Will merge.

@ttaylorr ttaylorr merged commit 41794df into git-lfs:master Apr 27, 2018
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