threads / discuss / 6952

autoCRLF, git status, git-gui, what is the desired behavior?

Subject: autoCRLF, git status, git-gui, what is the desired behavior?

## tl;dr

11 messages between Feb 25, 2007 and Feb 26, 2007.

replies: 10people: 4as markdown or json

Mark Levedahl· Feb 25, 2007, 19:33 UTC · lore

I am trying autoCRLF in git compiled from next (75415c455dd307), find some behavior that is probably different than desired dealing with a file where the only changes are to line endings:

create a text file (foo) with \n endings, check it in. $ u2d foo $ git diff foo

diff --git a/foo b/foo
$ git status
# On branch master
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#
#       modified:   foo
#
$ git ci -m 'x' foo
# On branch master
nothing to commit (working directory clean)

So, git commit will not check in the file, but git status shows an 
unclean file and git diff shows no actual differences.

Also, git-gui shows the file as modified, but clicking on that file 
gives a warning box saying "No differences detected...", does a rescan, 
and then shows the file as modified again.

I'm not sure of the correct fix:
1) Place the file in a separate category, perhaps "broken line-endings", 
and indicate that git will not check this in?
2) Overwrite the file with a fresh checkout (erasing the crlf differences)?
3) ?

Mark Levedahl
Junio C Hamano· Feb 25, 2007, 19:54 UTC · re: Mark Levedahl · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Mark Levedahl <mlevedahl@verizon.net> writes:
Show 21 quoted lines
> I am trying autoCRLF in git compiled from next (75415c455dd307), find
> some behavior that is probably different than desired dealing with a
> file where the only changes are to line endings:
>
> create a text file (foo) with \n endings, check it in.
> $ u2d foo
> $ git diff foo
> diff --git a/foo b/foo
> $ git status
> # On branch master
> # Changed but not updated:
> #   (use "git add <file>..." to update what will be committed)
> #
> #       modified:   foo
> #
> $ git ci -m 'x' foo
> # On branch master
> nothing to commit (working directory clean)
>
> So, git commit will not check in the file, but git status shows an
> unclean file and git diff shows no actual differences.

Unless you are doing something other than what you demonstrated above, I think what 'diff' and 'commit' steps show is expected, even without autoCRLF. 'git status' might be buggy.

	create a file (foo), check it in.
	$ touch foo
        $ git diff foo
        diff --git a/foo b/foo
        $ git commit -m 'x' foo
        # On branch master
        nothing to commit (working directory clean)

So in order to validate my conjecture that 'git-status' is buggy, can you try this:

	(1) Do your sequence from "create a text file (foo) with
            \n endings" to "git ci -m 'x' foo", as you depicted
            above.
	(2) Without doing anything else, run "git diff" again,

With my sequence above, "git diff" should say nothing because "update-index --refresh" run inside "git-status" (and "git-commit") would notice 'foo' has not changed.

Ah, I know what is going on. "update-index --refresh" notices that lstat(2) says the size is different between what is recorded in the index, and does not actually compare and refresh the entry.

But that is a very important optimization, and I do not think we would want to cripple that for autoCRLF.

I think this should work for you.
        create a text file (foo) with \n endings, check it in.
        $ u2d foo
	$ git update-index foo
        $ git diff foo
        $ git status
	$ git commit

I think the same --refresh check kicks in for "git add" (I did not try), so if you replace the above "git update-index foo" with "git add foo" it may not work. You would want to try that, too.

Junio C Hamano· Feb 25, 2007, 20:28 UTC · re: Junio C Hamano · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Junio C Hamano <junkio@cox.net> writes:
Show 7 quoted lines
> Ah, I know what is going on.  "update-index --refresh" notices
> that lstat(2) says the size is different between what is
> recorded in the index, and does not actually compare and refresh
> the entry.
>
> But that is a very important optimization, and I do not think we
> would want to cripple that for autoCRLF.
It might be interesting to try this patch.

Usually we _trust_ the index and say that the path has been modified if what its length on the filesystem returned by lstat(2) does not match with what is recorded in the index.

What this patch does is to disable that optimization when autocrlf is in effect. The change would make all paths whose size, read by lstat(2) from the filesystem, does not match what is recorded in the index to be re-validated by comparing the data, and if it is found not to have been modified, refresh the index by updating the size information (and other information such as mtime). In other words, this would probably make it prohibitibly expensive on autocrlf filesystems if you leave many paths dirty to run update-index --refresh (hence status and commit).

diff --git a/read-cache.c b/read-cache.c
index 605b352..11b8b56 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -225,11 +225,15 @@ int ce_modified(struct cache_entry *ce, struct stat *st, int really)
 	if (changed & (MODE_CHANGED | TYPE_CHANGED))
 		return changed;
 
-	/* Immediately after read-tree or update-index --cacheinfo,
+	/*
+	 * Immediately after read-tree or update-index --cacheinfo,
 	 * the length field is zero.  For other cases the ce_size
 	 * should match the SHA1 recorded in the index entry.
+	 * However, use of core.autocrlf can screw us up badly.
 	 */
-	if ((changed & DATA_CHANGED) && ce->ce_size != htonl(0))
+	if ((changed & DATA_CHANGED) &&
+	    ce->ce_size != htonl(0) &&
+	    !auto_crlf)
 		return changed;
 
 	changed_fs = ce_modified_check_fs(ce, st);
Mark Levedahl· Feb 25, 2007, 21:14 UTC · re: Junio C Hamano · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Junio C Hamano wrote:
> Junio C Hamano <junkio@cox.net> writes:
>   
> It might be interesting to try this patch.
>

This patch makes no difference to the problems I noted in my second message where the file undergoes crlf->lf translation on commit, so working copy is known to be different than blob. Is it the case that the size info stored in the index reflects the size of the blob rather than of the working copy? Absent autoCRLF these are of course identical, but with autoCRLF they are not and what we need stored is the working file info (at least for checking dirty-ness).

Mark
Junio C Hamano· Feb 25, 2007, 21:22 UTC · re: Mark Levedahl · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Mark Levedahl <mlevedahl@verizon.net> writes:
> ... Is it the case that
> the size info stored in the index reflects the size of the blob rather
> than of the working copy?

The size field among other fields is to cache the last lstat(2) information so that later "is the path modified?" question can be answered efficiently. So the size should in general match both blob and filesystem but on CRLF filesystems it is compared against and updated with the data from the filesystem. There could be a subtle bug that when updating an index entry we might be incorrectly storing the size of the blob, but I haven't checked.

Mark Levedahl· Feb 25, 2007, 22:20 UTC · re: Junio C Hamano · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Junio C Hamano wrote:
Show 17 quoted lines
> Mark Levedahl <mlevedahl@verizon.net> writes:
> 
>> ... Is it the case that
>> the size info stored in the index reflects the size of the blob rather
>> than of the working copy?
> 
> The size field among other fields is to cache the last lstat(2)
> information so that later "is the path modified?" question can
> be answered efficiently.  So the size should in general match
> both blob and filesystem but on CRLF filesystems it is compared
> against and updated with the data from the filesystem.  There
> could be a subtle bug that when updating an index entry we might
> be incorrectly storing the size of the blob, but I haven't
> checked.
> 
> 
> 
I instrumented read-cache.c with:
@@ -818,6 +822,8 @@ int read_cache_from(const char *path)
         struct cache_entry *ce = (struct cache_entry *) ((char *) 
cache_mmap + offset);
         offset = offset + ce_size(ce);
         active_cache[i] = ce;
+       printf("name: %s\n", ce->name);
+       printf("size: %u\n", ntohl(ce->ce_size)
     }
     index_file_timestamp = st.st_mtime;
     while (offset <= cache_mmap_size - 20 - 8) {

And I get, post commit:
name: foo
size: 21452

$ git-update-index
$ git-runstatus
...
name: foo
size: 20517
...

Note:  foo's size with lf endings is 20517
                   with crlf endings is 21452

So, what I think is happening:

I add a file with crlf endings: it gets converted to lf, but the file 
size with crlf is saved in the index.

Post commit, the file is replaced with lf endings in the working 
directory and now has size 205167. However, the index reflects the 
pre-converted file with crlf endings, not the post-converted with lf 
endings.

Remember: I have core.autocrlf=input, so all files have lf on output. 
Apparently the working file is updated by this process. The problem is 
the index is not updated to reflect that.

Mark
Mark Levedahl· Feb 25, 2007, 23:55 UTC · re: Mark Levedahl · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Mark Levedahl wrote:
Show 9 quoted lines
> Junio C Hamano wrote:
>> The size field among other fields is to cache the last lstat(2)
>> information so that later "is the path modified?" question can
>> be answered efficiently.  So the size should in general match
>> both blob and filesystem but on CRLF filesystems it is compared
>> against and updated with the data from the filesystem.  There
>> could be a subtle bug that when updating an index entry we might
>> be incorrectly storing the size of the blob, but I haven't
>> checked.

Never mind: I should have triggered off the observation that something was changing the file, but as far as I know git does not change the working directory on commit and the autoCRLF code does not introduce that "feature". My trusty old editor (visual slickedit) occasionally corrupts its macros and starts doing strange and wonderful things, always something I never saw before. In this case, I had foo open (absent crlf endings) in it and and the editor was apparently re-saving all of its files in the middle of my tests as I flipped between various windows. Killing the editor stopped the behavior, rebuilding all the macros from scratch got rid of the problem for good.

So, after all that, it appears that the index does reflect the size in the working repository and the problems I was having were nothing to do with git.

Sorry for the noise.
Mark
Mark Levedahl· Feb 25, 2007, 20:51 UTC · re: Junio C Hamano · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Junio C Hamano wrote:
Show 27 quoted lines
> Mark Levedahl <mlevedahl@verizon.net> writes:
> 
>> I am trying autoCRLF in git compiled from next (75415c455dd307), find
>> some behavior that is probably different than desired dealing with a
>> file where the only changes are to line endings:
>>
>> create a text file (foo) with \n endings, check it in.
>> $ u2d foo
>> $ git diff foo
>> diff --git a/foo b/foo
>> $ git status
>> # On branch master
>> # Changed but not updated:
>> #   (use "git add <file>..." to update what will be committed)
>> #
>> #       modified:   foo
>> #
>> $ git ci -m 'x' foo
>> # On branch master
>> nothing to commit (working directory clean)
>>
>> So, git commit will not check in the file, but git status shows an
>> unclean file and git diff shows no actual differences.
> 
> Unless you are doing something other than what you demonstrated
> above, I think what 'diff' and 'commit' steps show is expected,
> even without autoCRLF.  'git status' might be buggy.

I forgot the vital "-a" argument to git commit above. Adding -a gets the desired behavior (the difference disappears). Here is a sequence that is clearly counter-intuitive:

create foo with CRLF endings, then ...
$ git config core.autocrlf input
$ git add foo
$ git commit -m x foo
Created commit a9e9d4e1b88087462a4e15ff9044fa31e16d11bc
  1 files changed, 935 insertions(+), 0 deletions(-)
  create mode 100644 foo
$ git diff
diff --git a/foo b/foo
$ git status
# On branch master
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#
#       modified:   foo
#
$ git add foo
$ git status
# On branch master
nothing to commit (working directory clean)

--- git should not show a just checked in file as being different.
Note: a simple "git add foo" clears the above up, as would
git-update-index foo

Also, if I invoke git-gui on the above repository showing foo as modified...

1) foo shows up in the "Changed But Not Updated" list.
2) Clicking on foo gives message box with "No differences detected. ...
    Clicking the "ok" button invokes a rescan, back to step 1.
3) Adding foo to the commit list in git-gui works.
4) Committing the above from git-gui gives a commit with no
    changes (commit is made, shows up in git log, but has no
    changes associated).

--- I don't think git-gui should make create an empty commit in the 
above case.

Mark
Shawn O. Pearce· Feb 26, 2007, 02:06 UTC · re: Mark Levedahl · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Mark Levedahl <mdl123@verizon.net> wrote:
Show 12 quoted lines
> Also, if I invoke git-gui on the above repository showing foo as modified...
> 
> 1) foo shows up in the "Changed But Not Updated" list.
> 2) Clicking on foo gives message box with "No differences detected. ...
>    Clicking the "ok" button invokes a rescan, back to step 1.
> 3) Adding foo to the commit list in git-gui works.
> 4) Committing the above from git-gui gives a commit with no
>    changes (commit is made, shows up in git log, but has no
>    changes associated).
> 
> --- I don't think git-gui should make create an empty commit in the 
> above case.

Hmm. Probably not. In pg I used to compare HEAD^{tree} to the tree output by git-write-tree and refuse to make the commit if they had the same value. git-gui just blindly assumes that if a file is staged for committing then it won't make an empty commit; this is also the behavior in git-commit.sh.

Yet in the case of a merge you may want the same tree and not even realize it. Like if I merge a commit from a coworker, get a merge conflict, pick my version, but that just modified the tree to match mine, effectively doing an `-s ours` style merge. Of course here we have MERGE_HEAD and know we are merging...

-- 
Shawn.
Junio C Hamano· Feb 26, 2007, 02:45 UTC · re: Shawn O. Pearce · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

"Shawn O. Pearce" <spearce@spearce.org> writes:
Show 8 quoted lines
> Hmm.  Probably not.  In pg I used to compare HEAD^{tree} to the
> tree output by git-write-tree and refuse to make the commit if
> they had the same value.  git-gui just blindly assumes that if a
> file is staged for committing then it won't make an empty commit;
> this is also the behavior in git-commit.sh.
>
> Yet in the case of a merge you may want the same tree and not even
> realize it...

git-commit has been raised with all of these logic during its evolution. Is it a possibility to reuse it somehow?

Shawn O. Pearce· Feb 26, 2007, 15:54 UTC · re: Junio C Hamano · lore

Re: autoCRLF, git status, git-gui, what is the desired behavior?

Junio C Hamano <junkio@cox.net> wrote:
Show 13 quoted lines
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > Hmm.  Probably not.  In pg I used to compare HEAD^{tree} to the
> > tree output by git-write-tree and refuse to make the commit if
> > they had the same value.  git-gui just blindly assumes that if a
> > file is staged for committing then it won't make an empty commit;
> > this is also the behavior in git-commit.sh.
> >
> > Yet in the case of a merge you may want the same tree and not even
> > realize it...
> 
> git-commit has been raised with all of these logic during its
> evolution.  Is it a possibility to reuse it somehow?
 
Anything's possible.  ;-)

I'd rather not reuse git-commit in git-gui. git-commit is strictly porcelain-ish, while git-gui tries hard to only rely on the plumbing layer[*1*], while also trying to autodetect and honor status data used in the porcelain-ish (e.g. MERGE_HEAD, MERGE_MSG).

With the exception of this empty-commit case git-gui's commit path is stable and doing the same actions as git-commit, only the git-gui way. I'd rather not churn that code just to avoid an empty commit case. Its easy enough to check the trees, and git-gui knows if there are additional parents (and what those are) at the time of commit, so its easy enough to not do the tree comparsion if there is more than one parent.

I actually just found another way to make git-gui create an empty commit. I'm going to patch it to check the trees - because this shouldn't be allowed.

*1*: With the exception of git-fetch, git-push, git-merge and
     git-repack.  The latter two of which I would like to get
	 rewritten in pure Tcl, as I want more control over what
	 is happening.
-- 
Shawn.

← back to recent threads