feat(core): fix autocommit LFS files in pre-commit hook

renku/data/pre-commit.sh

@@ -39,7 +39,8 @@ if [ ${#MODIFIED_FILES[@]} -ne 0 ] || [ ${#ADDED_FILES[@]} -ne 0 ]; then
 fi
 
 if [ ${#MODIFIED_FILES[@]} -ne 0 ] ; then
-  MODIFIED_OUTPUTS=$(renku show outputs "${MODIFIED_FILES[@]}")
+  IFS=$'\n' read -r -d '' -a MODIFIED_OUTPUTS \
+    <<< "$(renku show outputs "${MODIFIED_FILES[@]}")"
   EXIT_CODE=$?
   if [ $EXIT_CODE -eq 3 ]; then
     echo "Cannot verify validity of the commit: Project metadata is outdated."
@@ -55,7 +56,7 @@ if [ ${#MODIFIED_FILES[@]} -ne 0 ] ; then
     echo 'To commit anyway, use "git commit --no-verify".'
     exit 1
   fi
-  if [ "$MODIFIED_OUTPUTS" ]; then
+  if [ ${#MODIFIED_OUTPUTS[@]} -ne 0 ]; then
     echo 'You are trying to update generated files.'
     echo
     echo 'Modified files:'
@@ -66,8 +67,9 @@ if [ ${#MODIFIED_FILES[@]} -ne 0 ] ; then
     echo 'To commit anyway, use "git commit --no-verify".'
     exit 1
   fi
-  IMMUTABLE_TEMPLATE_FILES=$(renku check-immutable-template-files "${MODIFIED_FILES[@]}")
-  if [ "$IMMUTABLE_TEMPLATE_FILES" ]; then
+  IFS=$'\n' read -r -d '' -a IMMUTABLE_TEMPLATE_FILES \
+    <<< "$(renku check-immutable-template-files "${MODIFIED_FILES[@]}")"
+  if [ ${#IMMUTABLE_TEMPLATE_FILES[@]} -ne 0 ]; then
     echo 'You are trying to update files marked as immutable in your project template.'
     echo 'This would prevent the project from being updated with new versions of the template in the future.'
     echo
@@ -82,9 +84,9 @@ if [ ${#MODIFIED_FILES[@]} -ne 0 ] ; then
 fi
 
 if [ ${#ADDED_FILES[@]} -ne 0 ]; then
-  UNTRACKED_PATHS=$(renku storage check-lfs-hook "${ADDED_FILES[@]}")
-  if [ "$UNTRACKED_PATHS" ]; then
-
+  IFS=$'\n' read -r -d '' -a UNTRACKED_PATHS \
+    <<< "$(renku storage check-lfs-hook "${ADDED_FILES[@]}")"
+  if [ ${#UNTRACKED_PATHS[@]} -ne 0 ]; then
     echo 'You are trying to commit large files to Git instead of Git-LFS.'
     echo
     AUTOCOMMIT_LFS=${AUTOCOMMIT_LFS:=$(renku config show autocommit_lfs)}
@@ -94,13 +96,9 @@ if [ ${#ADDED_FILES[@]} -ne 0 ]; then
         echo "$file"
       done
       echo
-      saveIFS=$IFS
-      IFS=$' '
-      files=${UNTRACKED_PATHS[*]}
-      git lfs track -- "$files"
+      git lfs track -- "${UNTRACKED_PATHS[@]}"
       git add .gitattributes
-      git add -- "$files"
-      IFS=$saveIFS
+      git add -- "${UNTRACKED_PATHS[@]}"
     else
       echo 'Large files:'
       for file in "${UNTRACKED_PATHS[@]}"; do

tests/cli/test_datasets.py

@@ -23,6 +23,7 @@
 import textwrap
 from pathlib import Path
 
+import git
 import pytest
 
 from renku.cli import cli
@@ -1321,66 +1322,63 @@ def test_pull_data_from_lfs(runner, client, tmpdir, subdirectory, no_lfs_size_li
     assert 0 == result.exit_code, format_result_exception(result)
 
 
-def test_lfs_hook(runner, client, subdirectory, large_file):
+def test_lfs_hook(client, subdirectory, large_file):
     """Test committing large files to Git."""
-    import git
+    filenames = {"large-file", "large file with whitespace", "large*file?with wildcards"}
 
-    shutil.copy(large_file, client.path)
+    for filename in filenames:
+        shutil.copy(large_file, client.path / filename)
     client.repo.git.add("--all")
 
     # Commit fails when file is not tracked in LFS
     with pytest.raises(git.exc.HookExecutionError) as e:
         client.repo.index.commit("large files not in LFS")
 
     assert "You are trying to commit large files to Git" in e.value.stdout
-    assert large_file.name in e.value.stdout
+    for filename in filenames:
+        assert filename in e.value.stdout
 
     # Can be committed after being tracked in LFS
-    client.track_paths_in_storage(large_file.name)
+    client.track_paths_in_storage(*filenames)
+    client.repo.git.add("--all")
     commit = client.repo.index.commit("large files tracked")
     assert "large files tracked" == commit.message
 
-
-def test_lfs_hook_autocommit(runner, client, subdirectory, large_file):
-    """Test committing large files to Git gets automatically added to lfs."""
-    result = runner.invoke(cli, ["config", "set", "autocommit_lfs", "true"])
-    assert 0 == result.exit_code, format_result_exception(result)
-
-    shutil.copy(large_file, client.path)
-    client.repo.git.add("--all")
-
-    result = client.repo.git.commit(
-        message="large files not in LFS",
-        with_extended_output=True,
-        env={"LC_ALL": "en_US.UTF-8", "LANG": "en_US.UTF-8"},
-    )
-    assert large_file.name in result[1]
-    assert ".gitattributes" in result[1]
-    assert "You are trying to commit large files to Git instead of Git-LFS" in result[2]
-    assert "Adding files to LFS" in result[2]
-    assert 'Tracking "large-file"' in result[2]
-    assert len(client.dirty_paths) == 0  # NOTE: make sure repo is clean
+    tracked_lfs_files = set(client.repo.git.lfs("ls-files", "--name-only").split("\n"))
+    assert filenames == tracked_lfs_files
 
 
-def test_lfs_hook_autocommit_env(runner, client, subdirectory, large_file):
+@pytest.mark.parametrize("use_env_var", [False, True])
+def test_lfs_hook_autocommit(runner, client, subdirectory, large_file, use_env_var):
     """Test committing large files to Git gets automatically added to lfs."""
-    os.environ["AUTOCOMMIT_LFS"] = "true"
+    if use_env_var:
+        os.environ["AUTOCOMMIT_LFS"] = "true"
+    else:
+        assert 0 == runner.invoke(cli, ["config", "set", "autocommit_lfs", "true"]).exit_code
 
-    shutil.copy(large_file, client.path)
+    filenames = {"large-file", "large file with whitespace", "large*file?with wildcards"}
+
+    for filename in filenames:
+        shutil.copy(large_file, client.path / filename)
     client.repo.git.add("--all")
 
     result = client.repo.git.commit(
         message="large files not in LFS",
         with_extended_output=True,
         env={"LC_ALL": "en_US.UTF-8", "LANG": "en_US.UTF-8"},
     )
-    assert large_file.name in result[1]
+    for filename in filenames:
+        assert filename in result[1]
     assert ".gitattributes" in result[1]
     assert "You are trying to commit large files to Git instead of Git-LFS" in result[2]
     assert "Adding files to LFS" in result[2]
-    assert 'Tracking "large-file"' in result[2]
+    for filename in filenames:
+        assert f'Tracking "{filename}"' in result[2]
     assert len(client.dirty_paths) == 0  # NOTE: make sure repo is clean
 
+    tracked_lfs_files = set(client.repo.git.lfs("ls-files", "--name-only").split("\n"))
+    assert filenames == tracked_lfs_files
+
 
 def test_lfs_hook_can_be_avoided(runner, project, subdirectory, large_file):
     """Test committing large files to Git."""