Hi all!
I just ran into a problem that I'm pretty sure is a bug in git. Just read and run this (fairly trivial) shell script to replicate.
Thanks!
---John Whitney
Phil HordRe: Bug report
On Thu, Oct 4, 2012 at 12:35 AM, John Whitney <jjw@emsoftware.com> wrote:
> I just ran into a problem that I'm pretty sure is a bug in git. Just read > and run this (fairly trivial) shell script to replicate.
When you added "* text=auto" in the .gitattributes file, you changed what git considers to be the checked-in file content state for test.txt. The file contents in your working directory do not match what git expects to check in. Therefore, the file appears to be different. If you commit the file "changes" the problem goes away.
This is more of a workaround than an a satisfying explanation. If you then checkout the original HEAD commit (but with .gitattributes present) you will see the problem appear again. But in a sense, adding .gitattributes this way is an act of foot-shooting. The best way forward may be to normalize your repository by removing all CR's from files in history. If you do not have this freedom, your best bet may be to normalize the repo in the current commit and move on.
Others with more intimate insight into the CRLF journey in git's past may have better advice.
Phil
Re: Bug report
Phil,
Thank you for your response. I do see the dilemma, but having no possible "unmodified" state is extremely inconvenient and, as shown, breaks basic git operations.
I guess my thought is that if git doesn't allow CRs to be checked in, then it should strip the CRs when checking the file out, and consider that form (or both forms) as "unmodified". It just doesn't make sense to me that files are considered modified immediately after checkout.
Any thoughts as to why this would not work?
---John
On 10/4/12 9:19 AM, Phil Hord wrote:
Show 22 quoted lines
> On Thu, Oct 4, 2012 at 12:35 AM, John Whitney <jjw@emsoftware.com> wrote: >> I just ran into a problem that I'm pretty sure is a bug in git. Just read >> and run this (fairly trivial) shell script to replicate. > When you added "* text=auto" in the .gitattributes file, you changed > what git considers to be the checked-in file content state for > test.txt. The file contents in your working directory do not match > what git expects to check in. Therefore, the file appears to be > different. If you commit the file "changes" the problem goes away. > > This is more of a workaround than an a satisfying explanation. If you > then checkout the original HEAD commit (but with .gitattributes > present) you will see the problem appear again. But in a sense, > adding .gitattributes this way is an act of foot-shooting. The best > way forward may be to normalize your repository by removing all CR's > from files in history. If you do not have this freedom, your best bet > may be to normalize the repo in the current commit and move on. > > Others with more intimate insight into the CRLF journey in git's past > may have better advice. > > Phil >
-- Great support for great users! Please visit http://emsoftware.com/support/ for our support policies, instructions and FAQs.
Jeff KingRe: Bug report
On Thu, Oct 04, 2012 at 11:10:40AM -0500, John Whitney wrote:
> Thank you for your response. I do see the dilemma, but having > no possible "unmodified" state is extremely inconvenient and, > as shown, breaks basic git operations.
But you have asked for an impossible state. You have said "this file cannot have CR when you check it in, because we erase them". And yet the version of the file in HEAD has CRs in it. So it must appear modified with respect to HEAD. And the solution is to make a commit with the normalized content.
You said in your test script:
# Committing test.txt or clearing .gitattributes does clear # the "modified" status, but those options are undesirable
Why is the commit undesirable? You have decided that CRs will no longer be tolerated in your repository (by setting .gitattributes). Now you need to record that change in history with a commit that strips out the CRs.
Show 5 quoted lines
> I guess my thought is that if git doesn't allow CRs to be checked > in, then it should strip the CRs when checking the file out, and > consider that form (or both forms) as "unmodified". It just > doesn't make sense to me that files are considered modified > immediately after checkout.
It is not about having CRs in the working tree file. Those are now considered uninteresting and stripped by git when comparing to the HEAD. The problem is that what's in your _repository_ has CRs.
I wonder if this is a fundamental misunderstanding of how the CRLF handling in git works. It is not "magically make me not care about line endings anymore". It is "the canonical version in the repo is LF-only. Strip anything coming into the repository to match that, and (optionally) add CR to anything going out to the filesystem for my convenience". But you need a flag day to update the in-repository versions to the new scheme.
-Peff
Re: Bug report
On 10/6/12 8:31 AM, Jeff King wrote:
Show 10 quoted lines
> On Thu, Oct 04, 2012 at 11:10:40AM -0500, John Whitney wrote: > >> Thank you for your response. I do see the dilemma, but having >> no possible "unmodified" state is extremely inconvenient and, >> as shown, breaks basic git operations. > But you have asked for an impossible state. You have said "this file > cannot have CR when you check it in, because we erase them". And yet the > version of the file in HEAD has CRs in it. So it must appear modified > with respect to HEAD. And the solution is to make a commit with the > normalized content.
I guess I'd really like to see git ignore all line endings of text files in the repository. Text files would then never be marked as "modified" for this reason and there would be no need to "fix" the line endings. I think that should be the default, but just having the option would be nice.
Show 9 quoted lines
> You said in your test script: > > # Committing test.txt or clearing .gitattributes does clear > # the "modified" status, but those options are undesirable > > Why is the commit undesirable? You have decided that CRs will no longer > be tolerated in your repository (by setting .gitattributes). Now you > need to record that change in history with a commit that strips out the > CRs.
In some cases it's undesirable. In my example, all I want to do is merge in the change that deletes the file, so I don't want to have to add that extra commit when I'm just going to delete the file anyway. It's also very inconvenient to have to deal with this issue when you're in the middle of a complex rebase operation.
Show 8 quoted lines
>> I guess my thought is that if git doesn't allow CRs to be checked >> in, then it should strip the CRs when checking the file out, and >> consider that form (or both forms) as "unmodified". It just >> doesn't make sense to me that files are considered modified >> immediately after checkout. > It is not about having CRs in the working tree file. Those are now > considered uninteresting and stripped by git when comparing to the HEAD. > The problem is that what's in your _repository_ has CRs.
Yes, but does that really have to be an issue? Is there any technical or practical reason you can think of that the repository shouldn't ignore those CRs?
Show 9 quoted lines
> I wonder if this is a fundamental misunderstanding of how the CRLF > handling in git works. It is not "magically make me not care about line > endings anymore". It is "the canonical version in the repo is LF-only. > Strip anything coming into the repository to match that, and > (optionally) add CR to anything going out to the filesystem for my > convenience". But you need a flag day to update the in-repository > versions to the new scheme. > > -Peff
You're right, we can't magically avoid all the line ending issues that people will run into. In this case, though, I think git can sidestep a fairly obnoxious problem. My example was simple, but when you've got multiple branches that need to be rebased/merged, it can get pretty hairy. The repository will never be truly "clean" unless you rewrite the whole thing (using filter-branch, for instance).
Maybe my above suggestion is more of a feature request than a bug, but there is the obvious bug that after changing .gitattributes, git still doesn't notice that files are "modified" until you modify them again in some way (touch works). I only noticed the CRs in our own repository after I tried to rebase a branch and got strange errors. To make git notice all the files, I had to "find . -type f -exec touch {} \;".
Jeff KingRe: Bug report
On Sat, Oct 06, 2012 at 09:23:59PM -0500, John Whitney wrote:
Show 12 quoted lines
> >You said in your test script: > > > > # Committing test.txt or clearing .gitattributes does clear > > # the "modified" status, but those options are undesirable > > > >Why is the commit undesirable? You have decided that CRs will no longer > >be tolerated in your repository (by setting .gitattributes). Now you > >need to record that change in history with a commit that strips out the > >CRs. > In some cases it's undesirable. In my example, all I want to do is > merge in the change that deletes the file, so I don't want to have to > add that extra commit when I'm just going to delete the file anyway.
Yes, but that is conflating two operations. You only don't want to do the commit because you are anticipating what is coming next (the cherry-picked deletion). But if you want to conflate, then you could also realize that you can simply delete the file, CRs or no, and you do not need to care about its modified state.
I think a much stronger argument for your position is that the cherry pick would not happen without a conflict after such a commit, because it would be deleting files with two different sets of content (the cherry-pick would want to delete the CR version, but you would not have that version).
In other words, you want the cherry-pick to happen and ignore the modification that could be committed, because you know the modification is not relevant (but git does not). But there is not a way to do that (even once you overcome the confusion), because the usual way to do so is to drop the local modification with "git reset" (which would not work in this case).
Show 6 quoted lines
> >It is not about having CRs in the working tree file. Those are now > >considered uninteresting and stripped by git when comparing to the HEAD. > >The problem is that what's in your _repository_ has CRs. > Yes, but does that really have to be an issue? Is there any technical > or practical reason you can think of that the repository shouldn't > ignore those CRs?
It's significantly less efficient. Right now git only has to do the conversion when updating the index cache of what's on the filesystem (i.e., when it would be doing a sha1 over the file contents _anyway_). And then it can compare sha1s internally, because it knows that all of the sha1s it has computed are for the canonical in-repo versions of the file.
If we assume that the in-repo file might need to have CRs stripped, then we need to actually follow up every sha1 mismatch with an actual content diff in order to discover if it really is different or not. We could cache the "true" sha1 of the canonical stripped version to avoid this, but now we are getting much more complex. In most cases it is sufficient to just commit the cleaned up contents and then never worry about it again.
Show 6 quoted lines
> You're right, we can't magically avoid all the line ending issues > that people will run into. In this case, though, I think git can > sidestep a fairly obnoxious problem. My example was simple, but when > you've got multiple branches that need to be rebased/merged, it can > get pretty hairy. The repository will never be truly "clean" unless > you rewrite the whole thing (using filter-branch, for instance).
Right. Git's current approach is very hairy when you are looking at history that crosses a CRLF flag-day boundary. It's definitely a weakness of the canonicalization approach. But other approaches also have downsides; I don't want to catalogue them all here, but you can certainly search the archive for various discussions and flamewars about how line endings are handled.
> Maybe my above suggestion is more of a feature request than a bug,
Fair enough. I think your complaint is real, but I think nobody has been clever enough yet to devise a solution that does not have too many other downsides. And of course you are free to propose such an approach if you have thought of one. :)
Show 6 quoted lines
> but there is the obvious bug that after changing .gitattributes, git
> still doesn't notice that files are "modified" until you modify them
> again in some way (touch works). I only noticed the CRs in our own
> repository after I tried to rebase a branch and got strange errors.
> To make git notice all the files, I had to "find . -type f -exec
> touch {} \;".I think the idea has been floated before of unconditionally refreshing the index when you update the crlf config via "git config". But of course that can only fix a fraction of the cases. You might edit it with an editor. Or they may be new lines in .gitattributes. Or a change of wildcard lines in .gitattributes.
Really, the issue is that the index contains a cache of what's in the files that is considered valid unless the stat information of the file changes. But that is obviously not the full story, as the canonicalization rules (CRLF handling or smudge/clean filters) can change, too, and that is not considered as part of the cache's validity. Doing it "right" would mean that anytime the attributes or config files changed, we would consider the cache entry dirty and re-read (and re-canonicalize) the file.
But that has either:
1. Bad complexity. It means our cache validity needs to know about
exactly which rules were applied to yield the cached sha1. And
those rules can be complex, consisting of wildcard matching,
cross-referencing custom filters from config, etc. 2. Bad performance. If you instead just invalidate cached sha1s when
the gitattributes or .git/config file changes, you catch way too
many cases. E.g., if you checkout a branch that changes
.gitattributes, we have to re-read every file in the repository,
even though most of them will not be affected.So I think it's possible to handle this case correctly, but doing it right is quite complex. So we have the "just manually poke the files when you make such a change". Which is a horrible user experience, but works OK in practice (and many people do not run into it at all, because on new projects they set the filter attributes very early on, before they have an existing history).
IOW, no, it is not pretty, but these are all known issues that nobody has felt it worth tackling yet.
-Peff
Re: Bug report
On 10/7/12 6:52 PM, Jeff King wrote:
Show 82 quoted lines
>> Yes, but does that really have to be an issue? Is there any technical
>> or practical reason you can think of that the repository shouldn't
>> ignore those CRs?
> It's significantly less efficient. Right now git only has to do the
> conversion when updating the index cache of what's on the filesystem
> (i.e., when it would be doing a sha1 over the file contents _anyway_).
> And then it can compare sha1s internally, because it knows that all of
> the sha1s it has computed are for the canonical in-repo versions of the
> file.
>
> If we assume that the in-repo file might need to have CRs stripped, then
> we need to actually follow up every sha1 mismatch with an actual content
> diff in order to discover if it really is different or not. We could
> cache the "true" sha1 of the canonical stripped version to avoid this,
> but now we are getting much more complex. In most cases it is sufficient
> to just commit the cleaned up contents and then never worry about it
> again.
>
>> You're right, we can't magically avoid all the line ending issues
>> that people will run into. In this case, though, I think git can
>> sidestep a fairly obnoxious problem. My example was simple, but when
>> you've got multiple branches that need to be rebased/merged, it can
>> get pretty hairy. The repository will never be truly "clean" unless
>> you rewrite the whole thing (using filter-branch, for instance).
> Right. Git's current approach is very hairy when you are looking at
> history that crosses a CRLF flag-day boundary. It's definitely a
> weakness of the canonicalization approach. But other approaches also
> have downsides; I don't want to catalogue them all here, but you can
> certainly search the archive for various discussions and flamewars about
> how line endings are handled.
>
>> Maybe my above suggestion is more of a feature request than a bug,
> Fair enough. I think your complaint is real, but I think nobody has been
> clever enough yet to devise a solution that does not have too many other
> downsides. And of course you are free to propose such an approach if you
> have thought of one. :)
>
>> but there is the obvious bug that after changing .gitattributes, git
>> still doesn't notice that files are "modified" until you modify them
>> again in some way (touch works). I only noticed the CRs in our own
>> repository after I tried to rebase a branch and got strange errors.
>> To make git notice all the files, I had to "find . -type f -exec
>> touch {} \;".
> I think the idea has been floated before of unconditionally refreshing
> the index when you update the crlf config via "git config". But of
> course that can only fix a fraction of the cases. You might edit it with
> an editor. Or they may be new lines in .gitattributes. Or a change of
> wildcard lines in .gitattributes.
>
> Really, the issue is that the index contains a cache of what's in the
> files that is considered valid unless the stat information of the file
> changes. But that is obviously not the full story, as the
> canonicalization rules (CRLF handling or smudge/clean filters) can
> change, too, and that is not considered as part of the cache's validity.
> Doing it "right" would mean that anytime the attributes or config files
> changed, we would consider the cache entry dirty and re-read (and
> re-canonicalize) the file.
>
> But that has either:
>
> 1. Bad complexity. It means our cache validity needs to know about
> exactly which rules were applied to yield the cached sha1. And
> those rules can be complex, consisting of wildcard matching,
> cross-referencing custom filters from config, etc.
>
> 2. Bad performance. If you instead just invalidate cached sha1s when
> the gitattributes or .git/config file changes, you catch way too
> many cases. E.g., if you checkout a branch that changes
> .gitattributes, we have to re-read every file in the repository,
> even though most of them will not be affected.
>
> So I think it's possible to handle this case correctly, but doing it
> right is quite complex. So we have the "just manually poke the files
> when you make such a change". Which is a horrible user experience, but
> works OK in practice (and many people do not run into it at all, because
> on new projects they set the filter attributes very early on, before
> they have an existing history).
>
> IOW, no, it is not pretty, but these are all known issues that nobody
> has felt it worth tackling yet.
>
> -PeffThank you very much for your detailed explanations. I suspected that efficiency concerns might be preventing a clean solution.
How about this idea... When git stores files, it could include a bit of metadata that tells it whether the file is a binary blob or text. (Perhaps it already does this?) If a binary blob (in the repository) is being compared with a text file (on the filesystem), git could re-process the blob and get the "sha1 of the canonical stripped version". In all other situations, the original SHA1 should be correct, since git already removes CRs from the line endings in files it recognizes as text.
I would think that this solution would have no performance penalty for "fixed" repositories. (It would only have a small performance hit when binary blobs are compared against text files, which is rare even in broken repositories.) Git could even throw a warning like: "File xyz.txt was originally stored as a binary blob."
What do you think?
---John
Re: Bug report
On 10/9/12 12:17 PM, John Whitney wrote:
Show 22 quoted lines
> Thank you very much for your detailed explanations. I suspected that > efficiency concerns might be preventing a clean solution. > > How about this idea... When git stores files, it could include a bit > of metadata that tells it whether the file is a binary blob or text. > (Perhaps it already does this?) If a binary blob (in the repository) > is being compared with a text file (on the filesystem), git could > re-process the blob and get the "sha1 of the canonical stripped > version". In all other situations, the original SHA1 should be > correct, since git already removes CRs from the line endings in files > it recognizes as text. > > I would think that this solution would have no performance penalty for > "fixed" repositories. (It would only have a small performance hit when > binary blobs are compared against text files, which is rare even in > broken repositories.) Git could even throw a warning like: "File > xyz.txt was originally stored as a binary blob." > > What do you think? > > ---John >
I'm going to reply to myself, to save you the trouble of replying. (You've been very helpful and I do appreciate it.)
I guess the problem with this idea is that git doesn't have any way to distinguish between binary blobs and text files in the repository. I think it would be useful information, but I guess that bridge burned a long time ago. So any metadata would have to be stored separately. Jeff, that's roughly equivalent to your idea of caching, which would take a lot of work to implement.
Thank you so much for helping me to understand the reason git behaves the way it does. It's a great tool!
---John
Re: Bug report
On 10/04/2012 12:35 AM, John Whitney wrote:
> I just ran into a problem that I'm pretty sure is a bug in git. Just > read and run this (fairly trivial) shell script to replicate.
I tried your steps on a Mac, but I wasn't able to produce the issue. Perhaps I don't have the right CRLF configs to trigger the issue. I've tried it on v1.7.9.6, which came with Xcode, and v1.7.7. What git version are you using? And, if any, what are your configs for "core.eol", "core.safecrlf", and "core.autocrlf" ?
What Phil said also makes sense though.
Re: Bug report
Andrew,
Thanks for checking this on your machine.
This problem occurs on Mac, Windows, and Linux, and with multiple versions of git. Note that in my script a CR is appended to test.txt. On the Mac, you can generate this in Terminal by pressing Ctrl-V Ctrl-M. Or, alternatively, just download and run the script like this: "sh git_failure.sh"
---John
On 10/4/12 10:21 AM, Andrew Wong wrote:
Show 11 quoted lines
> On 10/04/2012 12:35 AM, John Whitney wrote: >> I just ran into a problem that I'm pretty sure is a bug in git. Just >> read and run this (fairly trivial) shell script to replicate. > I tried your steps on a Mac, but I wasn't able to produce the issue. > Perhaps I don't have the right CRLF configs to trigger the issue. I've > tried it on v1.7.9.6, which came with Xcode, and v1.7.7. What git > version are you using? And, if any, what are your configs for > "core.eol", "core.safecrlf", and "core.autocrlf" ? > > What Phil said also makes sense though. >
-- Great support for great users! Please visit http://emsoftware.com/support/ for our support policies, instructions and FAQs.
Re: Bug report
Andrew,
I forgot to say that all of the config settings are not changed from the default.
---John
On 10/4/12 11:16 AM, John Whitney wrote:
Show 27 quoted lines
> Andrew, > > Thanks for checking this on your machine. > > This problem occurs on Mac, Windows, and Linux, and > with multiple versions of git. Note that in my script a CR > is appended to test.txt. On the Mac, you can generate this > in Terminal by pressing Ctrl-V Ctrl-M. Or, alternatively, > just download and run the script like this: "sh git_failure.sh" > > ---John > > > On 10/4/12 10:21 AM, Andrew Wong wrote: >> On 10/04/2012 12:35 AM, John Whitney wrote: >>> I just ran into a problem that I'm pretty sure is a bug in git. Just >>> read and run this (fairly trivial) shell script to replicate. >> I tried your steps on a Mac, but I wasn't able to produce the issue. >> Perhaps I don't have the right CRLF configs to trigger the issue. >> I've tried it on v1.7.9.6, which came with Xcode, and v1.7.7. What >> git version are you using? And, if any, what are your configs for >> "core.eol", "core.safecrlf", and "core.autocrlf" ? >> >> What Phil said also makes sense though. >> > >
-- Great support for great users! Please visit http://emsoftware.com/support/ for our support policies, instructions and FAQs.
Re: Bug report
On 10/04/2012 12:16 PM, John Whitney wrote:
Show 5 quoted lines
> This problem occurs on Mac, Windows, and Linux, and > with multiple versions of git. Note that in my script a CR > is appended to test.txt. On the Mac, you can generate this > in Terminal by pressing Ctrl-V Ctrl-M. Or, alternatively, > just download and run the script like this: "sh git_failure.sh"
Ah, yes. I can reproduce the problem. I was pasting the lines from your script. And I saw a new line in the shell when I pasted, so I thought the ^M was kept properly. But somewhere during the pasting, the ^M must have got translated to a \n automatically. Silly me.