{"thread":{"id":"6952","subject":"autoCRLF, git status, git-gui, what is the desired behavior?","startedAt":"2007-02-25T19:33:16Z","lastAt":"2007-02-26T15:54:42Z","messageCount":11,"participants":["Mark Levedahl","Junio C Hamano","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"35450","messageId":"45E1E47C.5090908@verizon.net","threadId":"6952","inReplyTo":null,"subject":"autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Mark Levedahl","fromEmail":"mlevedahl@verizon.net","sentAt":"2007-02-25T19:33:16Z","receivedAt":"2007-02-25T19:33:16Z","isPatch":false,"sender":{"key":"mlevedahl@verizon.net","avatar":null},"body":"I am trying autoCRLF in git compiled from next (75415c455dd307), find \nsome behavior that is probably different than desired dealing with a \nfile where the only changes are to line endings:\n\ncreate a text file (foo) with \\n endings, check it in.\n$ u2d foo\n$ git diff foo\ndiff --git a/foo b/foo\n$ git status\n# On branch master\n# Changed but not updated:\n# (use \"git add ...\" to update what will be committed)\n#\n# modified: foo\n#\n$ git ci -m 'x' foo\n# On branch master\nnothing to commit (working directory clean)\n\nSo, git commit will not check in the file, but git status shows an \nunclean file and git diff shows no actual differences.\n\nAlso, git-gui shows the file as modified, but clicking on that file \ngives a warning box saying \"No differences detected...\", does a rescan, \nand then shows the file as modified again.\n\nI'm not sure of the correct fix:\n1) Place the file in a separate category, perhaps \"broken line-endings\", \nand indicate that git will not check this in?\n2) Overwrite the file with a fresh checkout (erasing the crlf differences)?\n3) ?\n\nMark Levedahl\n"},{"id":"35452","messageId":"7vlkimrp1f.fsf@assigned-by-dhcp.cox.net","threadId":"6952","inReplyTo":"45E1E47C.5090908@verizon.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-25T19:54:36Z","receivedAt":"2007-02-25T19:54:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> I am trying autoCRLF in git compiled from next (75415c455dd307), find\n> some behavior that is probably different than desired dealing with a\n> file where the only changes are to line endings:\n>\n> create a text file (foo) with \\n endings, check it in.\n> $ u2d foo\n> $ git diff foo\n> diff --git a/foo b/foo\n> $ git status\n> # On branch master\n> # Changed but not updated:\n> # (use \"git add ...\" to update what will be committed)\n> #\n> # modified: foo\n> #\n> $ git ci -m 'x' foo\n> # On branch master\n> nothing to commit (working directory clean)\n>\n> So, git commit will not check in the file, but git status shows an\n> unclean file and git diff shows no actual differences.\n\nUnless you are doing something other than what you demonstrated\nabove, I think what 'diff' and 'commit' steps show is expected,\neven without autoCRLF. 'git status' might be buggy.\n\n\tcreate a file (foo), check it in.\n\t$ touch foo\n $ git diff foo\n diff --git a/foo b/foo\n $ git commit -m 'x' foo\n # On branch master\n nothing to commit (working directory clean)\n\nSo in order to validate my conjecture that 'git-status' is\nbuggy, can you try this:\n\n\t(1) Do your sequence from \"create a text file (foo) with\n \\n endings\" to \"git ci -m 'x' foo\", as you depicted\n above.\n\n\t(2) Without doing anything else, run \"git diff\" again, \n\nWith my sequence above, \"git diff\" should say nothing because \n\"update-index --refresh\" run inside \"git-status\" (and \"git-commit\")\nwould notice 'foo' has not changed.\n\nAh, I know what is going on. \"update-index --refresh\" notices\nthat lstat(2) says the size is different between what is\nrecorded in the index, and does not actually compare and refresh\nthe entry.\n\nBut that is a very important optimization, and I do not think we\nwould want to cripple that for autoCRLF.\n\nI think this should work for you.\n\n create a text file (foo) with \\n endings, check it in.\n $ u2d foo\n\t$ git update-index foo\n $ git diff foo\n $ git status\n\t$ git commit\n\nI think the same --refresh check kicks in for \"git add\" (I did\nnot try), so if you replace the above \"git update-index foo\"\nwith \"git add foo\" it may not work. You would want to try that,\ntoo.\n"},{"id":"35453","messageId":"7vfy8urngi.fsf@assigned-by-dhcp.cox.net","threadId":"6952","inReplyTo":"7vlkimrp1f.fsf@assigned-by-dhcp.cox.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-25T20:28:45Z","receivedAt":"2007-02-25T20:28:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Ah, I know what is going on. \"update-index --refresh\" notices\n> that lstat(2) says the size is different between what is\n> recorded in the index, and does not actually compare and refresh\n> the entry.\n>\n> But that is a very important optimization, and I do not think we\n> would want to cripple that for autoCRLF.\n\nIt might be interesting to try this patch.\n\nUsually we _trust_ the index and say that the path has been\nmodified if what its length on the filesystem returned by\nlstat(2) does not match with what is recorded in the index.\n\nWhat this patch does is to disable that optimization when\nautocrlf is in effect. The change would make all paths whose\nsize, read by lstat(2) from the filesystem, does not match what\nis recorded in the index to be re-validated by comparing the\ndata, and if it is found not to have been modified, refresh the\nindex by updating the size information (and other information\nsuch as mtime). In other words, this would probably make it\nprohibitibly expensive on autocrlf filesystems if you leave many\npaths dirty to run update-index --refresh (hence status and\ncommit).\n\n\ndiff --git a/read-cache.c b/read-cache.c\nindex 605b352..11b8b56 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -225,11 +225,15 @@ int ce_modified(struct cache_entry *ce, struct stat *st, int really)\n \tif (changed & (MODE_CHANGED | TYPE_CHANGED))\n \t\treturn changed;\n \n-\t/* Immediately after read-tree or update-index --cacheinfo,\n+\t/*\n+\t * Immediately after read-tree or update-index --cacheinfo,\n \t * the length field is zero. For other cases the ce_size\n \t * should match the SHA1 recorded in the index entry.\n+\t * However, use of core.autocrlf can screw us up badly.\n \t */\n-\tif ((changed & DATA_CHANGED) && ce->ce_size != htonl(0))\n+\tif ((changed & DATA_CHANGED) &&\n+\t ce->ce_size != htonl(0) &&\n+\t !auto_crlf)\n \t\treturn changed;\n \n \tchanged_fs = ce_modified_check_fs(ce, st);\n"},{"id":"35454","messageId":"45E1F6B5.8030907@verizon.net","threadId":"6952","inReplyTo":"7vlkimrp1f.fsf@assigned-by-dhcp.cox.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-02-25T20:51:01Z","receivedAt":"2007-02-25T20:51:01Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Junio C Hamano wrote:\n> Mark Levedahl writes:\n> \n>> I am trying autoCRLF in git compiled from next (75415c455dd307), find\n>> some behavior that is probably different than desired dealing with a\n>> file where the only changes are to line endings:\n>>\n>> create a text file (foo) with \\n endings, check it in.\n>> $ u2d foo\n>> $ git diff foo\n>> diff --git a/foo b/foo\n>> $ git status\n>> # On branch master\n>> # Changed but not updated:\n>> # (use \"git add ...\" to update what will be committed)\n>> #\n>> # modified: foo\n>> #\n>> $ git ci -m 'x' foo\n>> # On branch master\n>> nothing to commit (working directory clean)\n>>\n>> So, git commit will not check in the file, but git status shows an\n>> unclean file and git diff shows no actual differences.\n> \n> Unless you are doing something other than what you demonstrated\n> above, I think what 'diff' and 'commit' steps show is expected,\n> even without autoCRLF. 'git status' might be buggy.\n\nI forgot the vital \"-a\" argument to git commit above. Adding -a gets the\ndesired behavior (the difference disappears). Here is a sequence that\nis clearly counter-intuitive:\n\ncreate foo with CRLF endings, then ...\n$ git config core.autocrlf input\n$ git add foo\n$ git commit -m x foo\nCreated commit a9e9d4e1b88087462a4e15ff9044fa31e16d11bc\n 1 files changed, 935 insertions(+), 0 deletions(-)\n create mode 100644 foo\n$ git diff\ndiff --git a/foo b/foo\n$ git status\n# On branch master\n# Changed but not updated:\n# (use \"git add ...\" to update what will be committed)\n#\n# modified: foo\n#\n$ git add foo\n$ git status\n# On branch master\nnothing to commit (working directory clean)\n\n--- git should not show a just checked in file as being different.\nNote: a simple \"git add foo\" clears the above up, as would\ngit-update-index foo\n\nAlso, if I invoke git-gui on the above repository showing foo as modified...\n\n1) foo shows up in the \"Changed But Not Updated\" list.\n2) Clicking on foo gives message box with \"No differences detected. ...\n Clicking the \"ok\" button invokes a rescan, back to step 1.\n3) Adding foo to the commit list in git-gui works.\n4) Committing the above from git-gui gives a commit with no\n changes (commit is made, shows up in git log, but has no\n changes associated).\n\n--- I don't think git-gui should make create an empty commit in the \nabove case.\n\nMark\n"},{"id":"35456","messageId":"45E1FC1C.4090409@verizon.net","threadId":"6952","inReplyTo":"7vfy8urngi.fsf@assigned-by-dhcp.cox.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Mark Levedahl","fromEmail":"mlevedahl@verizon.net","sentAt":"2007-02-25T21:14:04Z","receivedAt":"2007-02-25T21:14:04Z","isPatch":false,"sender":{"key":"mlevedahl@verizon.net","avatar":null},"body":"Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> It might be interesting to try this patch.\n> \nThis patch makes no difference to the problems I noted in my second \nmessage where the file undergoes crlf->lf translation on commit, so \nworking copy is known to be different than blob. Is it the case that the \nsize info stored in the index reflects the size of the blob rather than \nof the working copy? Absent autoCRLF these are of course identical, but \nwith autoCRLF they are not and what we need stored is the working file \ninfo (at least for checking dirty-ness).\n\nMark\n"},{"id":"35457","messageId":"7v649qrkzg.fsf@assigned-by-dhcp.cox.net","threadId":"6952","inReplyTo":"45E1FC1C.4090409@verizon.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-25T21:22:11Z","receivedAt":"2007-02-25T21:22:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl writes:\n\n> ... Is it the case that\n> the size info stored in the index reflects the size of the blob rather\n> than of the working copy?\n\nThe size field among other fields is to cache the last lstat(2)\ninformation so that later \"is the path modified?\" question can\nbe answered efficiently. So the size should in general match\nboth blob and filesystem but on CRLF filesystems it is compared\nagainst and updated with the data from the filesystem. There\ncould be a subtle bug that when updating an index entry we might\nbe incorrectly storing the size of the blob, but I haven't\nchecked.\n"},{"id":"35461","messageId":"45E20BC2.3000305@verizon.net","threadId":"6952","inReplyTo":"7v649qrkzg.fsf@assigned-by-dhcp.cox.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-02-25T22:20:50Z","receivedAt":"2007-02-25T22:20:50Z","isPatch":false,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Junio C Hamano wrote:\n> Mark Levedahl writes:\n> \n>> ... Is it the case that\n>> the size info stored in the index reflects the size of the blob rather\n>> than of the working copy?\n> \n> The size field among other fields is to cache the last lstat(2)\n> information so that later \"is the path modified?\" question can\n> be answered efficiently. So the size should in general match\n> both blob and filesystem but on CRLF filesystems it is compared\n> against and updated with the data from the filesystem. There\n> could be a subtle bug that when updating an index entry we might\n> be incorrectly storing the size of the blob, but I haven't\n> checked.\n> \n> \n> \nI instrumented read-cache.c with:\n\n@@ -818,6 +822,8 @@ int read_cache_from(const char *path)\n struct cache_entry *ce = (struct cache_entry *) ((char *) \ncache_mmap + offset);\n offset = offset + ce_size(ce);\n active_cache[i] = ce;\n+ printf(\"name: %s\\n\", ce->name);\n+ printf(\"size: %u\\n\", ntohl(ce->ce_size)\n }\n index_file_timestamp = st.st_mtime;\n while (offset <= cache_mmap_size - 20 - 8) {\n\nAnd I get, post commit:\nname: foo\nsize: 21452\n\n$ git-update-index\n$ git-runstatus\n...\nname: foo\nsize: 20517\n...\n\nNote: foo's size with lf endings is 20517\n with crlf endings is 21452\n\nSo, what I think is happening:\n\nI add a file with crlf endings: it gets converted to lf, but the file \nsize with crlf is saved in the index.\n\nPost commit, the file is replaced with lf endings in the working \ndirectory and now has size 205167. However, the index reflects the \npre-converted file with crlf endings, not the post-converted with lf \nendings.\n\nRemember: I have core.autocrlf=input, so all files have lf on output. \nApparently the working file is updated by this process. The problem is \nthe index is not updated to reflect that.\n\nMark\n"},{"id":"35477","messageId":"45E221F4.4080900@verizon.net","threadId":"6952","inReplyTo":"45E20BC2.3000305@verizon.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Mark Levedahl","fromEmail":"mlevedahl@verizon.net","sentAt":"2007-02-25T23:55:32Z","receivedAt":"2007-02-25T23:55:32Z","isPatch":false,"sender":{"key":"mlevedahl@verizon.net","avatar":null},"body":"Mark Levedahl wrote:\n> Junio C Hamano wrote:\n>> The size field among other fields is to cache the last lstat(2)\n>> information so that later \"is the path modified?\" question can\n>> be answered efficiently. So the size should in general match\n>> both blob and filesystem but on CRLF filesystems it is compared\n>> against and updated with the data from the filesystem. There\n>> could be a subtle bug that when updating an index entry we might\n>> be incorrectly storing the size of the blob, but I haven't\n>> checked.\nNever mind: I should have triggered off the observation that something \nwas changing the file, but as far as I know git does not change the \nworking directory on commit and the autoCRLF code does not introduce \nthat \"feature\". My trusty old editor (visual slickedit) occasionally \ncorrupts its macros and starts doing strange and wonderful things, \nalways something I never saw before. In this case, I had foo open \n(absent crlf endings) in it and and the editor was apparently re-saving \nall of its files in the middle of my tests as I flipped between various \nwindows. Killing the editor stopped the behavior, rebuilding all the \nmacros from scratch got rid of the problem for good.\n\nSo, after all that, it appears that the index does reflect the size in \nthe working repository and the problems I was having were nothing to do \nwith git.\n\nSorry for the noise.\n\nMark\n"},{"id":"35483","messageId":"20070226020657.GA1884@spearce.org","threadId":"6952","inReplyTo":"45E1F6B5.8030907@verizon.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-26T02:06:57Z","receivedAt":"2007-02-26T02:06:57Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Mark Levedahl wrote:\n> Also, if I invoke git-gui on the above repository showing foo as modified...\n> \n> 1) foo shows up in the \"Changed But Not Updated\" list.\n> 2) Clicking on foo gives message box with \"No differences detected. ...\n> Clicking the \"ok\" button invokes a rescan, back to step 1.\n> 3) Adding foo to the commit list in git-gui works.\n> 4) Committing the above from git-gui gives a commit with no\n> changes (commit is made, shows up in git log, but has no\n> changes associated).\n> \n> --- I don't think git-gui should make create an empty commit in the \n> above case.\n\nHmm. Probably not. In pg I used to compare HEAD^{tree} to the\ntree output by git-write-tree and refuse to make the commit if\nthey had the same value. git-gui just blindly assumes that if a\nfile is staged for committing then it won't make an empty commit;\nthis is also the behavior in git-commit.sh.\n\nYet in the case of a merge you may want the same tree and not even\nrealize it. Like if I merge a commit from a coworker, get a merge\nconflict, pick my version, but that just modified the tree to match\nmine, effectively doing an `-s ours` style merge. Of course here\nwe have MERGE_HEAD and know we are merging...\n\n-- \nShawn.\n"},{"id":"35485","messageId":"7v649pr60q.fsf@assigned-by-dhcp.cox.net","threadId":"6952","inReplyTo":"20070226020657.GA1884@spearce.org","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-26T02:45:25Z","receivedAt":"2007-02-26T02:45:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Hmm. Probably not. In pg I used to compare HEAD^{tree} to the\n> tree output by git-write-tree and refuse to make the commit if\n> they had the same value. git-gui just blindly assumes that if a\n> file is staged for committing then it won't make an empty commit;\n> this is also the behavior in git-commit.sh.\n>\n> Yet in the case of a merge you may want the same tree and not even\n> realize it...\n\ngit-commit has been raised with all of these logic during its\nevolution. Is it a possibility to reuse it somehow?\n"},{"id":"35504","messageId":"20070226155442.GA1639@spearce.org","threadId":"6952","inReplyTo":"7v649pr60q.fsf@assigned-by-dhcp.cox.net","subject":"Re: autoCRLF, git status, git-gui, what is the desired behavior?","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-02-26T15:54:42Z","receivedAt":"2007-02-26T15:54:42Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> \"Shawn O. Pearce\" writes:\n> \n> > Hmm. Probably not. In pg I used to compare HEAD^{tree} to the\n> > tree output by git-write-tree and refuse to make the commit if\n> > they had the same value. git-gui just blindly assumes that if a\n> > file is staged for committing then it won't make an empty commit;\n> > this is also the behavior in git-commit.sh.\n> >\n> > Yet in the case of a merge you may want the same tree and not even\n> > realize it...\n> \n> git-commit has been raised with all of these logic during its\n> evolution. Is it a possibility to reuse it somehow?\n \nAnything's possible. ;-)\n\nI'd rather not reuse git-commit in git-gui. git-commit is strictly\nporcelain-ish, while git-gui tries hard to only rely on the plumbing\nlayer[*1*], while also trying to autodetect and honor status data\nused in the porcelain-ish (e.g. MERGE_HEAD, MERGE_MSG).\n\nWith the exception of this empty-commit case git-gui's commit\npath is stable and doing the same actions as git-commit, only the\ngit-gui way. I'd rather not churn that code just to avoid an empty\ncommit case. Its easy enough to check the trees, and git-gui knows\nif there are additional parents (and what those are) at the time of\ncommit, so its easy enough to not do the tree comparsion if there\nis more than one parent.\n\n\nI actually just found another way to make git-gui create an empty\ncommit. I'm going to patch it to check the trees - because this\nshouldn't be allowed.\n\n\n*1*: With the exception of git-fetch, git-push, git-merge and\n git-repack. The latter two of which I would like to get\n\t rewritten in pure Tcl, as I want more control over what\n\t is happening.\n-- \nShawn.\n"}]}