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 use binary for gitattributes for patch files #1834

Open
1 task done
asmeurer opened this issue Jan 19, 2024 · 3 comments
Open
1 task done

Don't use binary for gitattributes for patch files #1834

asmeurer opened this issue Jan 19, 2024 · 3 comments
Labels

Comments

@asmeurer
Copy link
Member

Solution to issue cannot be found in the documentation.

  • I checked the documentation.

Issue

The .gitattributes file uses

*.patch binary
*.diff binary

This makes it so that git diff and git show do not show the contents of patch files.

This is quite annoying as you typically have to update patch files manually, so you actually need to use git to update them.

This is evidently done to handle different line endings, but there are other ways this could be done that are less annoying:

  • Update conda build to call patch with the appropriate flags to ignore line endings. Basically, it already does this by default, but currently it is called with --binary which disables this.

  • Use a separate extension from .patch for files with non-standard line endings, and use that. The only benefit of using a standard file extension is syntax coloring on GitHub (which frankly, makes things more confusing anyway). If some feedstock has an actual binary patche, not just something with different line endings, I would suggest doing this for those.

  • For the repos that do have nonstandard endings, they can use

    skip_render:
      - .gitattributes
    

    and manually set *.patch binary in their .gitattributes (or really, the attribute corresponding to the specific type of line ending for that feedstock's line endings). This is probably the least ideal option as it would mean those repos don't actually get any of the other updates to that file.

See the discussion at conda-forge/emacs-feedstock#80 (comment)

Installed packages

.

Environment info

.
@asmeurer asmeurer added the bug label Jan 19, 2024
@isuruf isuruf self-assigned this Jan 19, 2024
@mbargull
Copy link
Member

conda-forge/emacs-feedstock#80 (comment) :

We might be able to do either

*.path -text
*.diff -text

or

*.path binary diff
*.diff binary diff

. IDK which one would work/be preferred.

Looking at https://github.com/git/git/blob/v2.43.0/attr.c#L662 it seems we have binary defined as

[attr]binary -diff -merge -text

. So,

*.path -merge -text
*.diff -merge -text

would probably suffice to address the reported issue while still retaining the desired behavior with Git not break source line ending depending on the users' configurations.

@isuruf isuruf removed their assignment Jan 19, 2024
@jakirkham
Copy link
Member

Maybe we can try one of these changes in a PR?

IIRC the line ending issue was tricky to get right depending on whether the user was on UNIX or Windows also depending on whether the patch/source code was written on UNIX or Windows. So we probably want to test all 4 cases to make sure that things behave as intended

@mbargull
Copy link
Member

Maybe we can try one of these changes in a PR?

Yes, it would be nice if someone could try it out.

IIRC the line ending issue was tricky to get right depending on whether the user was on UNIX or Windows also depending on whether the patch/source code was written on UNIX or Windows.

Yes, depending on the users' Git settings and platforms they use things can break easily.
A good example to test things out for would be python-feedstock since we do have patches for files with different line endings in there.

So we probably want to test all 4 cases to make sure that things behave as intended

No, we'd only need to test the

*.patch -merge -text
*.diff -merge -text

case.
binary is just an alias for -diff -merge -text.
So, the minimal change to get diff to display contents of *.patch/*.diff without risking further issues is just to remove the -diff part of binary.
See https://git-scm.com/docs/gitattributes/2.43.0 for details (search for -diff/-text).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants