{"thread":{"id":"27138","subject":"[PATCH RFC] Rename detection and whitespace","startedAt":"2011-04-19T20:13:19Z","lastAt":"2011-04-20T06:03:44Z","messageCount":2,"participants":["Ciaran","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"166101","messageId":"BANLkTikiH7bfWFGjFCDL-SnO9HQR-9Uofw@mail.gmail.com","threadId":"27138","inReplyTo":null,"subject":"[PATCH RFC] Rename detection and whitespace","fromName":"Ciaran","fromEmail":"ciaranj@gmail.com","sentAt":"2011-04-19T20:13:19Z","receivedAt":"2011-04-19T20:13:19Z","isPatch":true,"sender":{"key":"ciaranj@gmail.com","avatar":"https://avatars.githubusercontent.com/u/55368?v=4"},"body":"Hi,\n\nI recently had an issue with a merge where there was a branch of some\ncode made to do some format standardisation e.g. re-indenting,\nnormalising of whitespace etc. etc. and another branch to do with\ntidying up the codebase (moving some files around etc.)\n\nWhen we brought the two branches back together git struggled (failed)\nto apply the formatting changes to the files that were moved (-s\nrecursive -X ignore-all-space).  Now in terms of best practice\nobviously what we did was stupid, and we should never have\nparallelised the two pieces of work and I imagine that this may well\nbe considered 'known behaviour / won't fix' but I wanted to raise it\nand get told that rather than assume it :)\n\nAfter a minimal amount of investigation I found that with a small\npatch I could make my local git instance perform the merge flawlessly\n(in my case).\n\nFor me I tracked the behaviour down to the blob similarity calculation\nthat takes place in the diffcore-delta.c#hash_chars method.  In our\ncase the problem was we were adjusting the whitespace at the front of\neach line which meant that the 64 byte segment hashes were\ndifferent/mis-aligned between the 2 equivalent files.   This code\nalready 'normalises' out CRLF/LF differences by skipping any CR\ncharacters when followed by LF so my question is that would it be\nconsidered wrong/evil to ignore *all* whitespace characters when -X\nignore-all-space has been passed.\n\nThe behaviour is trivial to reproduce without a merge:\n  i) Create a text file with a few lines in it.\n ii) Add the file and commit it.\niii) Insert a space at the front of each line and rename the file\niv) git add -A .\nv) git status will now show a 'new file' and a 'deleted file'\n\nWith the patch inlined below (not suitable for inclusion, naive in so\nmany ways (what is whitespace, and should be optional/configurable to\nname just two!) git status will report a rename instead.  This\nbehaviour carries across to merging.\n\nPresumably I'm missing something important, but the improved rename\ndetection for us was outstanding!\n\n---\n diffcore-delta.c |    3 +++\n 1 files changed, 3 insertions(+), 0 deletions(-)\n\ndiff --git a/diffcore-delta.c b/diffcore-delta.c\nindex 7cf431d..5429b8d 100644\n--- a/diffcore-delta.c\n+++ b/diffcore-delta.c\n@@ -146,6 +146,9 @@ static struct spanhash_top *hash_chars(struct\ndiff_filespec *one)\n \t\tif (is_text && c == '\\r' && sz && *buf == '\\n')\n \t\t\tcontinue;\n\n+\t\tif (is_text && ( c == '\\t' || c ==' ' ) )\n+\t\t\tcontinue;\n+\n \t\taccum1 = (accum1 << 7) ^ (accum2 >> 25);\n \t\taccum2 = (accum2 << 7) ^ (old_1 >> 25);\n \t\taccum1 += c;\n-- \n1.7.4.1\n"},{"id":"166128","messageId":"20110420060343.GD28597@sigill.intra.peff.net","threadId":"27138","inReplyTo":"BANLkTikiH7bfWFGjFCDL-SnO9HQR-9Uofw@mail.gmail.com","subject":"Re: [PATCH RFC] Rename detection and whitespace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-20T06:03:44Z","receivedAt":"2011-04-20T06:03:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 19, 2011 at 09:13:19PM +0100, Ciaran wrote:\n\n> For me I tracked the behaviour down to the blob similarity calculation\n> that takes place in the diffcore-delta.c#hash_chars method.  In our\n> case the problem was we were adjusting the whitespace at the front of\n> each line which meant that the 64 byte segment hashes were\n> different/mis-aligned between the 2 equivalent files.   This code\n> already 'normalises' out CRLF/LF differences by skipping any CR\n> characters when followed by LF so my question is that would it be\n> considered wrong/evil to ignore *all* whitespace characters when -X\n> ignore-all-space has been passed.\n\nI think what you are proposing makes sense. If the diff which spawned\nthe rename detection doesn't care about whitespace, then probably it\nshould be ignored in the rename, too. That would go for merging with \"-X\nignore-all-space\", but also for \"git diff -w\" (and probably \"-b\").\n\nI do think it should be conditional on those, though. We _know_ that\nCRLF versus LF in a text file is irrelevant to the file's content. But\nwhether whitespace is irrelevant varies from file to file.\n\nSo your patch is in the right direction, in my opinion.\n\n-Peff\n"}]}