{"thread":{"id":"25143","subject":"[PATCH v3 0/4] fix normalization of foreign idents","startedAt":"2010-08-23T07:05:23Z","lastAt":"2010-09-18T22:38:04Z","messageCount":7,"participants":["Marcus Comstedt","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"150951","messageId":"b56d7f50198f63a810b304ae77043f58a240f743.1284820251.git.marcus@mc.pp.se","threadId":"25143","inReplyTo":"cover.1284820251.git.marcus@mc.pp.se","subject":"[PATCH v3 2/4] convert: fix normalization of foreign idents","fromName":"Marcus Comstedt","fromEmail":"marcus@mc.pp.se","sentAt":"2010-08-23T07:05:23Z","receivedAt":"2010-08-23T07:05:23Z","isPatch":true,"sender":{"key":"marcus@mc.pp.se","avatar":"https://avatars.githubusercontent.com/u/411296?v=4"},"body":"Since ident_to_worktree() does not touch $Id$ tags which contain\nforeign expansions in the repository, make sure that ident_to_git()\ndoes not either.  This fixes the problem that such files show\nspurious modification upon checkout.\n\nThere is however one case where we want ident_to_git() to normalize\nthe tag to $Id$ despite the asymmetry:  When committing a modification\nto a file which has a foreign ident, the foreign ident should be\nreplaced with a regular git ident.  Thus, add a new parameter to\nconvert_to_git() that indicates if we want the foreign idents\nnormalized after all.\n\nSigned-off-by: Marcus Comstedt <marcus@mc.pp.se>\n---\n builtin/apply.c |    2 +-\n builtin/blame.c |    2 +-\n cache.h         |    8 +++++++-\n combine-diff.c  |    2 +-\n convert.c       |   23 ++++++++++++++++++-----\n diff.c          |    2 +-\n sha1_file.c     |    3 ++-\n 7 files changed, 31 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 638e7be..fe8d638 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1932,7 +1932,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED);\n+\t        convert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 850e165..8d8cbf3 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2095,7 +2095,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\tif (strbuf_read(&buf, 0, 0) < 0)\n \t\t\tdie_errno(\"failed to read from stdin\");\n \t}\n-\tconvert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED);\n+\tconvert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n \tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\ndiff --git a/cache.h b/cache.h\nindex 250abc1..3010e20 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -586,6 +586,11 @@ enum allow_checks {\n \tCHECKS_ALLOWED = 1,\n };\n \n+enum normalize_mode {\n+        NORMALIZE_FOR_COMPARE = 0,\n+\tNORMALIZE_FOR_COMMIT = 1,\n+};\n+\n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n \tBRANCH_TRACK_NEVER = 0,\n@@ -1064,7 +1069,8 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n /* convert.c */\n /* returns 1 if *dst was used */\n extern int convert_to_git(const char *path, const char *src, size_t len,\n-                          struct strbuf *dst, enum allow_checks checksallowed);\n+                          struct strbuf *dst, enum allow_checks checksallowed,\n+\t\t\t  enum normalize_mode mode);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n extern int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst);\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex c7f132d..e36bf61 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -758,7 +758,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tif (is_file) {\n \t\t\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\t\t\tif (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED)) {\n+\t\t\t\tif (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {\n \t\t\t\t\tfree(result);\n \t\t\t\t\tresult = strbuf_detach(&buf, &len);\n \t\t\t\t\tresult_size = len;\ndiff --git a/convert.c b/convert.c\nindex 8050c24..4eb28d8 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -520,9 +520,10 @@ static int count_ident(const char *cp, unsigned long size)\n }\n \n static int ident_to_git(const char *path, const char *src, size_t len,\n-                        struct strbuf *buf, int ident)\n+\t\t\tstruct strbuf *buf, int ident,\n+\t\t\tint normalize_foreign_ident)\n {\n-\tchar *dst, *dollar;\n+\tchar *dst, *dollar, *spc;\n \n \tif (!ident || !count_ident(src, len))\n \t\treturn 0;\n@@ -549,6 +550,16 @@ static int ident_to_git(const char *path, const char *src, size_t len,\n \t\t\t\tcontinue;\n \t\t\t}\n \n+\t\t\tspc = memchr(src + 4, ' ', dollar - src - 4);\n+\t\t\tif (spc && spc < dollar-1 &&\n+\t\t\t    !normalize_foreign_ident) {\n+\t\t\t\t/* There are spaces in unexpected places.\n+\t\t\t\t * This is probably an id from some other\n+\t\t\t\t * versioning system. Keep it for now.\n+\t\t\t\t */\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n \t\t\tmemcpy(dst, \"Id$\", 3);\n \t\t\tdst += 3;\n \t\t\tlen -= dollar + 1 - src;\n@@ -706,7 +717,8 @@ static enum action determine_action(enum action text_attr, enum eol eol_attr)\n }\n \n int convert_to_git(const char *path, const char *src, size_t len,\n-                   struct strbuf *dst, enum allow_checks checksallowed)\n+                   struct strbuf *dst, enum allow_checks checksallowed,\n+\t\t   enum normalize_mode mode)\n {\n \tstruct git_attr_check check[5];\n \tenum action action = CRLF_GUESS;\n@@ -739,7 +751,8 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\treturn ret | ident_to_git(path, src, len, dst, ident);\n+\treturn ret | ident_to_git(path, src, len, dst, ident,\n+\t\t\t\t  mode == NORMALIZE_FOR_COMMIT);\n }\n \n static int convert_to_working_tree_internal(const char *path, const char *src,\n@@ -797,5 +810,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);\n+\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n }\ndiff --git a/diff.c b/diff.c\nindex ed74f6b..eebe3dd 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2375,7 +2375,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t/*\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 13624a6..cbebb75 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2434,7 +2434,8 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \tif ((type == OBJ_BLOB) && path) {\n \t\tstruct strbuf nbuf = STRBUF_INIT;\n \t\tif (convert_to_git(path, buf, size, &nbuf,\n-\t\t                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED)) {\n+\t\t                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED,\n+\t\t\t\t   write_object ? NORMALIZE_FOR_COMMIT : NORMALIZE_FOR_COMPARE)) {\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\n \t\t}\n-- \n1.7.2\n"},{"id":"150952","messageId":"8221caee454ac58983c657e306341059061218aa.1284820251.git.marcus@mc.pp.se","threadId":"25143","inReplyTo":"cover.1284820251.git.marcus@mc.pp.se","subject":"[PATCH v3 3/4] t0021: test checkout and commit of foreign idents","fromName":"Marcus Comstedt","fromEmail":"marcus@mc.pp.se","sentAt":"2010-09-07T19:16:03Z","receivedAt":"2010-09-07T19:16:03Z","isPatch":true,"sender":{"key":"marcus@mc.pp.se","avatar":"https://avatars.githubusercontent.com/u/411296?v=4"},"body":"Add test cases for the following behaviors:\n\n  * Checking out a file with a foreign ident should not flag\n    the file as modified.  This is to prevent a mess when checking\n    out old versions, and to allow a migration model where files\n    are allowed to keep their foreign ident as long as their\n    content is also \"foreign\", i.e. not modified since the migration\n    to git.\n\n  * Committing to a file with a foreign ident should replace the\n    foreign ident with a native ident.  This is simply to get\n    the normal behavior of ident:  When the contents of the file is\n    updated, so is the ident.\n\nSigned-off-by: Marcus Comstedt <marcus@mc.pp.se>\n---\n t/t0021-conversion.sh |   58 +++++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 58 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh\nindex 828e35b..cf83c02 100755\n--- a/t/t0021-conversion.sh\n+++ b/t/t0021-conversion.sh\n@@ -93,4 +93,62 @@ test_expect_success expanded_in_repo '\n \tcmp expanded-keywords expected-output\n '\n \n+# Check that a file containing idents (native or foreign) is not\n+# spuriously flagged as modified on checkout\n+test_expect_success 'ident pristine after checkout' '\n+\t{\n+\t\techo \"File with foreign ident\"\n+\t\techo \"\\$Id\\$\"\n+\t\techo \"\\$Id: Foreign Commit With Spaces \\$\"\n+\t} > native-and-foreign-idents &&\n+\n+\t{\n+\t\techo \"File with foreign ident\"\n+\t\techo \"\\$Id: c389f8936d7baa13f463254d55b72e00e5496e3f \\$\"\n+\t\techo \"\\$Id: Foreign Commit With Spaces \\$\"\n+\t} > expected-output &&\n+\n+\tgit add native-and-foreign-idents &&\n+\tgit commit -m \"File with native and foreign idents\" &&\n+\n+\techo \"native-and-foreign-idents ident\" >> .gitattributes &&\n+\n+\trm -f native-and-foreign-idents &&\n+\tgit checkout -- native-and-foreign-idents &&\n+\tcat native-and-foreign-idents &&\n+\tcmp native-and-foreign-idents expected-output &&\n+\ttouch native-and-foreign-idents &&\n+\tgit status --porcelain native-and-foreign-idents > output &&\n+\ttest ! -s output &&\n+\tgit diff -- native-and-foreign-idents > output &&\n+\ttest ! -s output\n+'\n+\n+# Check that actually modifying the file and committing it produces a\n+# new ident on checkout\n+test_expect_success 'foreign ident replaced on commit' '\n+\t{\n+\t\techo \"File with foreign ident\"\n+\t\techo \"\\$Id: cc874844b7868ce341059e6a87c50b6f37b75807 \\$\"\n+\t\techo \"\\$Id: cc874844b7868ce341059e6a87c50b6f37b75807 \\$\"\n+\t\techo \"Some new content\"\n+\t} > expected-output &&\n+\n+\techo \"1\t0\tnative-and-foreign-idents\" > expected-stat1 &&\n+\techo \"2\t1\tnative-and-foreign-idents\" > expected-stat2 &&\n+\n+\techo \"Some new content\" >> native-and-foreign-idents &&\n+\tgit diff --numstat -- native-and-foreign-idents > output &&\n+\tcmp output expected-stat1 &&\n+\tgit add native-and-foreign-idents &&\n+\tgit commit -m \"Modified file\" &&\n+\tgit diff --numstat HEAD^ HEAD -- native-and-foreign-idents > output &&\n+\tcmp output expected-stat2 &&\n+\trm -f native-and-foreign-idents &&\n+\tgit checkout -- native-and-foreign-idents &&\n+\tcat native-and-foreign-idents &&\n+\tcmp native-and-foreign-idents expected-output\n+'\n+\n+\n test_done\n-- \n1.7.2\n"},{"id":"150953","messageId":"dc98d53e2cc922c4ba7f21043969f77463e72c58.1284820251.git.marcus@mc.pp.se","threadId":"25143","inReplyTo":"cover.1284820251.git.marcus@mc.pp.se","subject":"[PATCH v3 1/4] convert: generalize checksafe parameter","fromName":"Marcus Comstedt","fromEmail":"marcus@mc.pp.se","sentAt":"2010-09-13T22:00:49Z","receivedAt":"2010-09-13T22:00:49Z","isPatch":true,"sender":{"key":"marcus@mc.pp.se","avatar":"https://avatars.githubusercontent.com/u/411296?v=4"},"body":"The convert_to_git() function used to have a checksafe parameter,\nwhich could be used to prevent safe_crlf checks by passing 0\ninstead of the value of the global variable safe_crlf.\n\nSince preventing checks is a wider concept than just disabling\nsafe_crlf, generalize the parameter so that it can be used for other\ntypes of checks as well.\n\nSigned-off-by: Marcus Comstedt <marcus@mc.pp.se>\n---\n builtin/apply.c |    2 +-\n builtin/blame.c |    2 +-\n cache.h         |    7 ++++++-\n combine-diff.c  |    2 +-\n convert.c       |    7 ++++---\n diff.c          |    2 +-\n sha1_file.c     |    2 +-\n 7 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 23c18c5..638e7be 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -1932,7 +1932,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n \tcase S_IFREG:\n \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n \t\t\treturn error(\"unable to open or read %s\", path);\n-\t\tconvert_to_git(path, buf->buf, buf->len, buf, 0);\n+\t\tconvert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED);\n \t\treturn 0;\n \tdefault:\n \t\treturn -1;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 1015354..850e165 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -2095,7 +2095,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n \t\tif (strbuf_read(&buf, 0, 0) < 0)\n \t\t\tdie_errno(\"failed to read from stdin\");\n \t}\n-\tconvert_to_git(path, buf.buf, buf.len, &buf, 0);\n+\tconvert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED);\n \torigin->file.ptr = buf.buf;\n \torigin->file.size = buf.len;\n \tpretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);\ndiff --git a/cache.h b/cache.h\nindex 2ef2fa3..250abc1 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -581,6 +581,11 @@ enum eol {\n \n extern enum eol eol;\n \n+enum allow_checks {\n+\tCHECKS_DISALLOWED = 0,\n+\tCHECKS_ALLOWED = 1,\n+};\n+\n enum branch_track {\n \tBRANCH_TRACK_UNSPECIFIED = -1,\n \tBRANCH_TRACK_NEVER = 0,\n@@ -1059,7 +1064,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n /* convert.c */\n /* returns 1 if *dst was used */\n extern int convert_to_git(const char *path, const char *src, size_t len,\n-                          struct strbuf *dst, enum safe_crlf checksafe);\n+                          struct strbuf *dst, enum allow_checks checksallowed);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n extern int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst);\n \ndiff --git a/combine-diff.c b/combine-diff.c\nindex 655fa89..c7f132d 100644\n--- a/combine-diff.c\n+++ b/combine-diff.c\n@@ -758,7 +758,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n \t\t\tif (is_file) {\n \t\t\t\tstruct strbuf buf = STRBUF_INIT;\n \n-\t\t\t\tif (convert_to_git(elem->path, result, len, &buf, safe_crlf)) {\n+\t\t\t\tif (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED)) {\n \t\t\t\t\tfree(result);\n \t\t\t\t\tresult = strbuf_detach(&buf, &len);\n \t\t\t\t\tresult_size = len;\ndiff --git a/convert.c b/convert.c\nindex 01de9a8..8050c24 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -706,7 +706,7 @@ static enum action determine_action(enum action text_attr, enum eol eol_attr)\n }\n \n int convert_to_git(const char *path, const char *src, size_t len,\n-                   struct strbuf *dst, enum safe_crlf checksafe)\n+                   struct strbuf *dst, enum allow_checks checksallowed)\n {\n \tstruct git_attr_check check[5];\n \tenum action action = CRLF_GUESS;\n@@ -733,7 +733,8 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \t\tlen = dst->len;\n \t}\n \taction = determine_action(action, eol_attr);\n-\tret |= crlf_to_git(path, src, len, dst, action, checksafe);\n+\tret |= crlf_to_git(path, src, len, dst, action,\n+\t\t\t   (checksallowed? safe_crlf : 0));\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n@@ -796,5 +797,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\treturn ret | convert_to_git(path, src, len, dst, 0);\n+\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);\n }\ndiff --git a/diff.c b/diff.c\nindex 9a5c77c..ed74f6b 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2375,7 +2375,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n \t\t/*\n \t\t * Convert from working tree format to canonical git format\n \t\t */\n-\t\tif (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {\n+\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED)) {\n \t\t\tsize_t size = 0;\n \t\t\tmunmap(s->data, s->size);\n \t\t\ts->should_munmap = 0;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 0cd9435..13624a6 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -2434,7 +2434,7 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n \tif ((type == OBJ_BLOB) && path) {\n \t\tstruct strbuf nbuf = STRBUF_INIT;\n \t\tif (convert_to_git(path, buf, size, &nbuf,\n-\t\t                   write_object ? safe_crlf : 0)) {\n+\t\t                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED)) {\n \t\t\tbuf = strbuf_detach(&nbuf, &size);\n \t\t\tre_allocated = 1;\n \t\t}\n-- \n1.7.2\n"},{"id":"150950","messageId":"c2829398f87f35f24522a780fa82b1250a15b7c8.1284820251.git.marcus@mc.pp.se","threadId":"25143","inReplyTo":"cover.1284820251.git.marcus@mc.pp.se","subject":"[PATCH v3 4/4] Documentation: document foreign idents","fromName":"Marcus Comstedt","fromEmail":"marcus@mc.pp.se","sentAt":"2010-09-18T13:53:59Z","receivedAt":"2010-09-18T13:53:59Z","isPatch":true,"sender":{"key":"marcus@mc.pp.se","avatar":"https://avatars.githubusercontent.com/u/411296?v=4"},"body":"Add a short paragraph to the \"ident\" documentation about the\nsemantics and intended use of foreign idents.\n\nSigned-off-by: Marcus Comstedt <marcus@mc.pp.se>\nSigned-off-by: Henrik Grubbström <grubba@grubba.org>\n---\n Documentation/gitattributes.txt |    7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex e5a27d8..9af1d6f 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -272,6 +272,13 @@ sign `$` upon checkout.  Any byte sequence that begins with\n `$Id:` and ends with `$` in the worktree file is replaced\n with `$Id$` upon check-in.\n \n+When converting a repository from a different version control\n+system, it can be useful to create blobs which contain expanded\n+`$Id$` tags.  Git will recognize such \"foreign idents\" if they\n+contain at least one space within the payload.  A foreign ident\n+will be replaced by `$Id$` upon check-in, but is left unaltered\n+upon checkout.\n+\n \n `filter`\n ^^^^^^^^\n-- \n1.7.2\n"},{"id":"150949","messageId":"cover.1284820251.git.marcus@mc.pp.se","threadId":"25143","inReplyTo":null,"subject":"[PATCH v3 0/4] fix normalization of foreign idents","fromName":"Marcus Comstedt","fromEmail":"marcus@mc.pp.se","sentAt":"2010-09-18T14:30:51Z","receivedAt":"2010-09-18T14:30:51Z","isPatch":true,"sender":{"key":"marcus@mc.pp.se","avatar":"https://avatars.githubusercontent.com/u/411296?v=4"},"body":"New in this reroll:\n\n  * Added documentation of foreign idents\n\n  * Raised the level of abstraction for the arguments to\n    convert_to_git which control the conversion process (both\n    old and new), as suggested by Junio\n\nMarcus Comstedt (4):\n  convert: generalize checksafe parameter\n  convert: fix normalization of foreign idents\n  t0021: test checkout and commit of foreign idents\n  Documentation: document foreign idents\n\n Documentation/gitattributes.txt |    7 +++++\n builtin/apply.c                 |    2 +-\n builtin/blame.c                 |    2 +-\n cache.h                         |   13 ++++++++-\n combine-diff.c                  |    2 +-\n convert.c                       |   26 +++++++++++++----\n diff.c                          |    2 +-\n sha1_file.c                     |    3 +-\n t/t0021-conversion.sh           |   58 +++++++++++++++++++++++++++++++++++++++\n 9 files changed, 103 insertions(+), 12 deletions(-)\n\n-- \n1.7.2\n"},{"id":"150969","messageId":"7v8w2yzqkc.fsf@alter.siamese.dyndns.org","threadId":"25143","inReplyTo":"b56d7f50198f63a810b304ae77043f58a240f743.1284820251.git.marcus@mc.pp.se","subject":"Re: [PATCH v3 2/4] convert: fix normalization of foreign idents","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-18T21:31:47Z","receivedAt":"2010-09-18T21:31:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marcus Comstedt <marcus@mc.pp.se> writes:\n\n> Since ident_to_worktree() does not touch $Id$ tags which contain\n> foreign expansions in the repository, make sure that ident_to_git()\n> does not either.\n\nThanks.\n\n> There is however one case where we want ident_to_git() to normalize\n> the tag to $Id$ despite the asymmetry...\n\nI'd actually think that is a bad idea.  The user _can_ choose to do so by\nremoving the stale part from '$Id: obsolate garbage$\", of course.  Or we\ncan always normalize, which _might_ turn out to be a better solution.  In\neither case, it would be better to be consistent.\n\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index 638e7be..fe8d638 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -1932,7 +1932,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)\n>  \tcase S_IFREG:\n>  \t\tif (strbuf_read_file(buf, path, st->st_size) != st->st_size)\n>  \t\t\treturn error(\"unable to open or read %s\", path);\n> -\t\tconvert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED);\n> +\t        convert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n\nIn order to apply a patch to a file in the working tree, we first need to\nread it, and this codepath is the place to do so.  The file in the working\ntree may be \"smudged\" (in the sense of smudge/clean filter) and we \"clean\"\nit so that we can compare and update with what we read from the patch (the\npatch text is expected to be always \"clean\").\n\nWe don't check \"safe-crlf\", as it can \"die\" if it is allowed to when\ncheck_safe_clrf() finds things it does not like.\n\nWe expect to write the result out to the working tree, and at that point\nwe will run convert_to_working_tree().  The result can also be added to\nthe index after we are done.  The resulting index may be used to create\nthe next commit.  Why \"for compare\" and not \"for commit\"?\n\nIf you get a patch to a file that contains a line with an ident, either as\na context or old line.  Both your working tree file and your index would\nhave \"$Id: stale garbage$\".\n\nA patch may have\n\n - \"$Id: stale garbage$\" (made against the identical foreign source),\n - \"$Id: updated garbage$\" (made against an updated foreign source), or\n - \"$Id$\": (made against a conversion to git done elsewhere).\n\nBy telling git not to normalize \"$Id: stale garbage$\" you have, aren't you\nmaking the patch made not to apply 2 out of 3 above cases, especially if\nit came from your git friends (the last one)?\n\nThis patch doesn't touch \"apply --cached\" at all, which introduces yet\nmore unnecessary inconsistency.  If you made changes to the index through\nthat codepath, shouldn't the resulting object lose the $Id: stale garbage$\nsomewhere before it is made into the next commit?\n\nMy gut feeling is that this kind of complications aren't worth it.  If we\nwere to address the $Id$ issue, I wonder if we can fix it the other way\naround, by making both directions always convert (i.e. ident_to_worktree()\nand ident_to_git()); the end result of such a change feels much simpler to\nexplain to the users.\n\nAnyway, let's continue reading your patch.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 850e165..8d8cbf3 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -2095,7 +2095,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,\n>  \t\tif (strbuf_read(&buf, 0, 0) < 0)\n>  \t\t\tdie_errno(\"failed to read from stdin\");\n>  \t}\n> -\tconvert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED);\n> +\tconvert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n\nNot questionable.  We are \"read-only\".\n\n> diff --git a/cache.h b/cache.h\n> index 250abc1..3010e20 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -586,6 +586,11 @@ enum allow_checks {\n>  \tCHECKS_ALLOWED = 1,\n>  };\n>  \n> +enum normalize_mode {\n> +        NORMALIZE_FOR_COMPARE = 0,\n> +\tNORMALIZE_FOR_COMMIT = 1,\n> +};\n\nFunny use of whitespaces.\n\n> diff --git a/combine-diff.c b/combine-diff.c\n> index c7f132d..e36bf61 100644\n> --- a/combine-diff.c\n> +++ b/combine-diff.c\n> @@ -758,7 +758,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,\n>  \t\t\tif (is_file) {\n>  \t\t\t\tstruct strbuf buf = STRBUF_INIT;\n>  \n> -\t\t\t\tif (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED)) {\n> +\t\t\t\tif (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {\n\nNot questionable.  We are \"read-only\".\n\n> @@ -797,5 +810,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str\n>  \t\tsrc = dst->buf;\n>  \t\tlen = dst->len;\n>  \t}\n> -\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);\n> +\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n\nThis is called from merge operation to read from the object store, and\napply double-conversion (first from git to working tree and then back to\ngit).  But the result is written out as an object and hopefully be\nrecorded as a merge commit.  Why \"for compare\"?  We would want a normalized\nresult, no?\n\n> diff --git a/diff.c b/diff.c\n> index ed74f6b..eebe3dd 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -2375,7 +2375,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n>  \t\t/*\n>  \t\t * Convert from working tree format to canonical git format\n>  \t\t */\n> -\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED)) {\n> +\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {\n\nThis feels wrong.  This is a \"read-only\" access just like combine-diff and\nblame one.  I think CHECKS_ALLOWED is probably a bug in the original.\nThere is no reason to complain against what you haven't added.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 13624a6..cbebb75 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -2434,7 +2434,8 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,\n>  \tif ((type == OBJ_BLOB) && path) {\n>  \t\tstruct strbuf nbuf = STRBUF_INIT;\n>  \t\tif (convert_to_git(path, buf, size, &nbuf,\n> -\t\t                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED)) {\n> +\t\t                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED,\n> +\t\t\t\t   write_object ? NORMALIZE_FOR_COMMIT : NORMALIZE_FOR_COMPARE)) {\n\nAlthough I can sort-of see why Steffen wanted to allow the former less\nstrict in these two commands:\n\n    git hash-object <foo\n    git hash-object -w <foo\n\nthe code now says that they will produce different results, which feels\nutterly wrong.\n\nI was hoping that from this patch a simple pattern, a correlation between\nthe two conversion tweak flags you now have, would emerge[*1*].  And\nindeed it looks like it does.\n\nIf we fix the potential bug in diff_populate_filespec(), we see that\ncheckcrlf==0 goes hand in hand with \"for compare\".  The meaning of both\noption is \"Are we in read-only codepath?\"[*2*] and both conversion filters\nchange their behaviour based on that single bit.\n\nBut the \"for compare\"/\"for commit\" we looked at during this review may\nprobably have to change, and in the end it may not be a simple matter of\n\"if we are writing we convert this way, but if we are reading we convert\nthat other way\".  My gut feeling is still that if we want consistency, we\nshould just always convert, not the other way around, though.\n\n\n[footnote]\n\n*1* By the way, as I already said, I do not like adding more and more\nconversion tweak flags (it makes it even more distasteful that this patch\ndoes so especially to support a check-box \"feature\" like ident).\n\n*2* That is the kind of option that specifies what it _means_, not what or\nhow it does, I suggested you to think about in the first round of review.\n\"Do we allow checks?\" wasn't the kind of a meaningful option I was hoping\nto see; we would want the code to clarify _why_ we allow or forego checks\nat individual callsites.\n"},{"id":"150973","messageId":"yf9eicq1xv7.fsf@chiyo.mc.pp.se","threadId":"25143","inReplyTo":"7v8w2yzqkc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/4] convert: fix normalization of foreign idents","fromName":"Marcus Comstedt","fromEmail":"marcus@mc.pp.se","sentAt":"2010-09-18T22:38:04Z","receivedAt":"2010-09-18T22:38:04Z","isPatch":true,"sender":{"key":"marcus@mc.pp.se","avatar":"https://avatars.githubusercontent.com/u/411296?v=4"},"body":"\nHi Junio.\n\nJunio C Hamano <gitster@pobox.com> writes:\n\n>> There is however one case where we want ident_to_git() to normalize\n>> the tag to $Id$ despite the asymmetry...\n>\n> I'd actually think that is a bad idea.  The user _can_ choose to do so by\n> removing the stale part from '$Id: obsolate garbage$\", of course.\n\nYes, but it's easy to miss doing so.  And having two versions with the\nsame ident would be bad.  Furthermore, I couldn't think of a use case\nwhere you want to use idents (as indicated by setting the \"ident\"\nattribute), and do _not_ want stale data removed.\n\n\n> Or we can always normalize, which _might_ turn out to be a better\n> solution.\n\nI'm not quite sure I understand what you mean by \"always normalize\" here.\nDoes this mean reverting the original \"foreign ident\" change, which is\nwhat's preserving these idents on checkout?\n\n\n[...]\n> In order to apply a patch to a file in the working tree, we first need to\n> read it, and this codepath is the place to do so.  The file in the working\n> tree may be \"smudged\" (in the sense of smudge/clean filter) and we \"clean\"\n> it so that we can compare and update with what we read from the patch (the\n                    ^^^^^^^\n> patch text is expected to be always \"clean\").\n>\n> We don't check \"safe-crlf\", as it can \"die\" if it is allowed to when\n> check_safe_clrf() finds things it does not like.\n>\n> We expect to write the result out to the working tree, and at that point\n> we will run convert_to_working_tree().  The result can also be added to\n> the index after we are done.  The resulting index may be used to create\n> the next commit.  Why \"for compare\" and not \"for commit\"?\n\nI believe you answered that question yourself above.  ;-)\n\nThe file will be converted for commit once you add and commit the\nresult that was written to the working tree.  That happens in\nindex_mem(), not here.\n\n\n> If you get a patch to a file that contains a line with an ident, either as\n> a context or old line.  Both your working tree file and your index would\n> have \"$Id: stale garbage$\".\n>\n> A patch may have\n>\n>  - \"$Id: stale garbage$\" (made against the identical foreign source),\n>  - \"$Id: updated garbage$\" (made against an updated foreign source), or\n>  - \"$Id$\": (made against a conversion to git done elsewhere).\n>\n> By telling git not to normalize \"$Id: stale garbage$\" you have, aren't you\n> making the patch made not to apply 2 out of 3 above cases, especially if\n> it came from your git friends (the last one)?\n\nBut by normalizing it, it would just be two of the other cases where\nthe patch does not apply, no?\n\nBy not normalizing it, a patch made against the head of a clone of the\nsame repository would apply at least.\n\nOr am I missing something here?\n\n\n> This patch doesn't touch \"apply --cached\" at all, which introduces yet\n> more unnecessary inconsistency.  If you made changes to the index through\n> that codepath, shouldn't the resulting object lose the $Id: stale garbage$\n> somewhere before it is made into the next commit?\n\nForeign idents should always be removed by a commit which alters the\nfile in any other way, so there might be additional handling needed\nfor this case, I'll have to take a closer look at it.\n\n\n> My gut feeling is that this kind of complications aren't worth it.  If we\n> were to address the $Id$ issue, I wonder if we can fix it the other way\n> around, by making both directions always convert (i.e. ident_to_worktree()\n> and ident_to_git()); the end result of such a change feels much simpler to\n> explain to the users.\n\nWell, that's how it worked before 07814d9.\n\n\n>> +enum normalize_mode {\n>> +        NORMALIZE_FOR_COMPARE = 0,\n>> +\tNORMALIZE_FOR_COMMIT = 1,\n>> +};\n>\n> Funny use of whitespaces.\n\nYes, it seems one of the lines got 8 spaces and the other one a tab.\nNot intentional.\n\n\n>> @@ -797,5 +810,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str\n>>  \t\tsrc = dst->buf;\n>>  \t\tlen = dst->len;\n>>  \t}\n>> -\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);\n>> +\treturn ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);\n>\n> This is called from merge operation to read from the object store, and\n> apply double-conversion (first from git to working tree and then back to\n> git).  But the result is written out as an object and hopefully be\n> recorded as a merge commit.  Why \"for compare\"?  We would want a normalized\n> result, no?\n\nOk, this was a new function and I didn't fully understand its purpose.\nIf the resulting buffer is written as an object, then \"for commit\"\nshould be used, yes.\n\n\n>> @@ -2375,7 +2375,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)\n>>  \t\t/*\n>>  \t\t * Convert from working tree format to canonical git format\n>>  \t\t */\n>> -\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED)) {\n>> +\t\tif (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {\n>\n> This feels wrong.  This is a \"read-only\" access just like combine-diff and\n> blame one.  I think CHECKS_ALLOWED is probably a bug in the original.\n> There is no reason to complain against what you haven't added.\n\nYes, I was also a bit curious about this one.  If it is indeed a bug,\nit probably ought to be fixed independently of this patch series.\n\n\n> *2* That is the kind of option that specifies what it _means_, not what or\n> how it does, I suggested you to think about in the first round of review.\n> \"Do we allow checks?\" wasn't the kind of a meaningful option I was hoping\n> to see; we would want the code to clarify _why_ we allow or forego checks\n> at individual callsites.\n\nWell, maybe it was a little unrealistic to hope that someone new to\nthe codebase should be able to discern exactly what you guys were\nthinking when you wrote a particular piece of code...  ;-)\n\n\n  // Marcus\n"}]}