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

[BUG] Spaces in file names are not handled correctly #216

Closed
2 tasks done
vertxxyz opened this issue Oct 20, 2021 · 8 comments · Fixed by #218
Closed
2 tasks done

[BUG] Spaces in file names are not handled correctly #216

vertxxyz opened this issue Oct 20, 2021 · 8 comments · Fixed by #218
Labels
bug Something isn't working

Comments

@vertxxyz
Copy link

Is there an existing issue for this?

  • I have searched the existing issues

Describe the bug?

If you have spaces in your file names and you provide a custom separator (eg. separator: ",") all spaces are replaced by the separator and the results sorted.

To Reproduce

  1. On a repo containing the paths:
  • embeds/general 412874536418279426/read-me-1 899830563651854336.json
  • Test 1/Test 1.json
  1. Running
- id: json_changes
  uses: tj-actions/changed-files@v10.1
  with:
      files: |
        .json$
      separator: ","

- name: List modified files
  run: |
    echo ${{ steps.json_changes.outputs.all_modified_files }}
  1. Sees the result of:
    1.json,1/Test,412874536418279426/read-me-1,899830563651854336.json,Test,embeds/general

What OS are you seeing the problem on?

ubuntu-latest or ubuntu-20.04

Expected behavior?

I expect:
embeds/general 412874536418279426/read-me-1 899830563651854336.json,Test 2/Test 1.json

Relevant log output

No response

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@vertxxyz vertxxyz added the bug Something isn't working label Oct 20, 2021
@github-actions
Copy link
Contributor

Thanks for reporting this issue.

@jackton1
Copy link
Member

jackton1 commented Oct 22, 2021

I'll strongly discourage use of spaces in the file names as leads to more errors down the line.

bash loops by default rely on spaces as the Internal field separator and might result in bugs like

for file in embeds/general 412874536418279426/read-me-1 899830563651854336.json Test 1/Test 1.json
do
   echo "$file"
done

Which prints out

embeds/general
412874536418279426/read-me-1
899830563651854336.json
Test
1/Test
1.json

But we should be able to support this use case.

See: https://askubuntu.com/a/344418/385863

@vertxxyz
Copy link
Author

I appreciate it, thanks.
I'd note that the test file in this very project also contains a space! (test/test new.txt)

@jackton1
Copy link
Member

jackton1 commented Oct 22, 2021

Correct, It's a known limitation and that file was created for testing and future support of file names with spaces.

@jackton1
Copy link
Member

jackton1 commented Oct 23, 2021

@vertxxyz This should now be available in the latest release v11

@psychemedia
Copy link

Hi

Is this patch in 11.2? I am getting an error here trying to run some tests against a checkin with 1 or 2 files that each contain spaces:

image

   steps:
    - uses: actions/checkout@master
      with:
        fetch-depth: 0 # or 2?
#        ref: nbval-test-tags
    - id: changed-files
      uses: tj-actions/changed-files@v11.2
      with:
          files: |
            .ipynb$

    - name: test changed files
      run: |
        for file in ${{ steps.changed-files.outputs.all_modified_files }}; do
            py.test  --nbval "$file" || continue
          done
        done
      continue-on-error: true

If I use for file in "${{ steps.changed-files.outputs.all_modified_files }}" then all the files are quoted as one filename. Do I need to use another separator style and then map onto an array?

(Apols for the naive question, I am a bit out of my depth...!)

@jackton1
Copy link
Member

jackton1 commented Oct 26, 2021

@psychemedia No worries, you can use IFS=$',' and set the separator input to ,

- id: changed-files
      uses: tj-actions/changed-files@v11.2
      with:
          separator: ","
          files: |
            .ipynb$

For example, you'll need to use

...
      - name: List all modified files
         run: |
           IFS=$',' read -a MODIFIED_FILES_ARRAY <<< "${{ steps.changed-files-comma.outputs.modified_files }}"
           for file in "${MODIFIED_FILES_ARRAY[@]}"; do
             echo $file
           done
         shell:
           bash

See this PR for more information.

@marc-hb
Copy link

marc-hb commented Jan 3, 2022

It requires discipline but shell scripting does support whitespace in filenames just fine:
https://mywiki.wooledge.org/Quotes
https://mywiki.wooledge.org/ParsingLs
https://mywiki.wooledge.org/BashGuide/Arrays

Shellcheck catches 99.99% missing quotes: https://github.com/koalaman/shellcheck/wiki/SC2046

bash loops by default rely on spaces as the Internal field separator and might result in bugs like

Only loops "buggy by default". Fix:

- for file in embeds/general 412874536418279426/read-me-1 899830563651854336.json Test 1/Test 1.json
+ for file in embeds/general 412874536418279426/read-me-1 899830563651854336.json 'Test 1/Test 1.json'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
4 participants