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

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

[Mark Levedahl]

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

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

[Junio C Hamano]

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.

	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.

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

[Junio C Hamano]

Junio C Hamano <junkio@cox.net> writes:

> 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);

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

[Mark Levedahl]

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

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

[Junio C Hamano]

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.

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

[Mark Levedahl]

Junio C Hamano wrote:
> 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

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

[Mark Levedahl]

Mark Levedahl wrote:
> 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

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

[Mark Levedahl]

Junio C Hamano wrote:
> 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

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

[Shawn O. Pearce]

Mark Levedahl <mdl123@verizon.net> wrote:
> 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.

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

[Junio C Hamano]

"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?

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

[Shawn O. Pearce]

Junio C Hamano <junkio@cox.net> wrote:
> "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.