Fix for normalization of foreign idents

Hi.

The new behaviour that $Id$ tags containing expanded idents from
other version control systems, nice though it is, has a rather
serious problem.  This is because convert_to_git is no longer
the inverse operation of convert_to_working_tree.  For native
git idents, the transformations are

  $Id$ --(c_t_w_t)--> $Id: 123...$ --(c_t_g)--> $Id$

but for foreign idents, it becomes

  $Id: blah$ --(c_t_w_t)--> $Id: blah$ --(c_t_g)--> $Id$

The result of this is that git may consider even newly checked out
files as modified, even though neither the file contents nor its
attributes have been modified after the checkout.  I say _may_, because
it can also happen that it decides based on the time stamps that it
doesn't need to compare the actual contents, in which case the file
does not show as modified.

The following patch fixes this by preserving the foreign ident
also in convert_to_git, meaning that convert_to_git is again the
inverse operation of convert_to_working_tree, with the following
transformation series:

  $Id: blah$ --(c_t_w_t)--> $Id: blah$ --(c_t_g)--> $Id: blah$

This restores correct and deterministic operation of status and
diff, meaning that if the file hasn't actually been modified, no
modifications are shown.

As you might suggest, always keeping the foreign ident would mean it
is never updated when you commit new versions of the file, which isn't
really what we want.  Keeping the foreign ident as long as the last
modification to the file was made in the previous version control
system makes perfect sense, but once we make a commit to the file
within git, it should be replaced with a git ident.  The patch is
therefore slightly more complex, adding an extra parameter to control
whether foreign idents are collapsed or not.  This parameter is set
to true only in the case when index_mem is called with write_object
set to true, which is to say when we create a new blob from the
working tree (i.e. we are committing the file).

I hope we can agree that this is a sound and unintrusive way of
handling the problem.  :-)

Incidentally, should one want to create a commit to replace a foreign
ident with a native git one without making any other changes to the
file, this is still simple to do.  All that is needed is to change any
character inside the expanded ident in the working tree, and the file
will show as modified, and will have the foreign ident removed on
commit.


  // Marcus

Since ident_to_worktree() does not touch $Id$ tags which contain
foreign expansions in the repository, make sure that ident_to_git()
does not either.  This fixes the problem that such files show
spurious modification upon checkout.

There is however one case where we want ident_to_git() to normalize
the tag to $Id$ despite the asymmetry:  When committing a modification
to a file which has a foreign ident, the foreign ident should be
replaced with a regular git ident.  Thus, add a new parameter to
convert_to_git() that indicates if we want the foreign idents
normalized after all.

Signed-off-by: Marcus Comstedt <redacted>
---
 builtin/apply.c |    2 +-
 builtin/blame.c |    2 +-
 cache.h         |    3 ++-
 combine-diff.c  |    2 +-
 convert.c       |   21 +++++++++++++++++----
 diff.c          |    2 +-
 sha1_file.c     |    3 ++-
 7 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/builtin/apply.c b/builtin/apply.c
index f38c1f7..7e503f4 100644
--- a/builtin/apply.c
+++ b/builtin/apply.c

@@ -1759,7 +1759,7 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	case S_IFREG:
 		if (strbuf_read_file(buf, path, st->st_size) != st->st_size)
 			return error("unable to open or read %s", path);
-		convert_to_git(path, buf->buf, buf->len, buf, 0);
+		convert_to_git(path, buf->buf, buf->len, buf, 0, 0);
 		return 0;
 	default:
 		return -1;

diff --git a/builtin/blame.c b/builtin/blame.c
index 437b1a4..83c6561 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c

@@ -2095,7 +2095,7 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
 		if (strbuf_read(&buf, 0, 0) < 0)
 			die_errno("failed to read from stdin");
 	}
-	convert_to_git(path, buf.buf, buf.len, &buf, 0);
+	convert_to_git(path, buf.buf, buf.len, &buf, 0, 0);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);

diff --git a/cache.h b/cache.h
index eb77e1d..b98042f 100644
--- a/cache.h
+++ b/cache.h

@@ -1055,7 +1055,8 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);
 /* convert.c */
 /* returns 1 if *dst was used */
 extern int convert_to_git(const char *path, const char *src, size_t len,
-                          struct strbuf *dst, enum safe_crlf checksafe);
+			  struct strbuf *dst, enum safe_crlf checksafe,
+			  int normalize_foreign_ident);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 
 /* add */

diff --git a/combine-diff.c b/combine-diff.c
index 655fa89..e81aa7d 100644
--- a/combine-diff.c
+++ b/combine-diff.c

@@ -758,7 +758,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			if (is_file) {
 				struct strbuf buf = STRBUF_INIT;
 
-				if (convert_to_git(elem->path, result, len, &buf, safe_crlf)) {
+				if (convert_to_git(elem->path, result, len, &buf, safe_crlf, 0)) {
 					free(result);
 					result = strbuf_detach(&buf, &len);
 					result_size = len;

diff --git a/convert.c b/convert.c
index e41a31e..00d0612 100644
--- a/convert.c
+++ b/convert.c

@@ -519,9 +519,10 @@ static int count_ident(const char *cp, unsigned long size)
 }
 
 static int ident_to_git(const char *path, const char *src, size_t len,
-                        struct strbuf *buf, int ident)
+			struct strbuf *buf, int ident,
+			int normalize_foreign_ident)
 {
-	char *dst, *dollar;
+	char *dst, *dollar, *spc;
 
 	if (!ident || !count_ident(src, len))
 		return 0;

@@ -548,6 +549,16 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 				continue;
 			}
 
+			spc = memchr(src + 4, ' ', dollar - src - 4);
+			if (spc && spc < dollar-1 &&
+			    !normalize_foreign_ident) {
+				/* There are spaces in unexpected places.
+				 * This is probably an id from some other
+				 * versioning system. Keep it for now.
+				 */
+				continue;
+			}
+
 			memcpy(dst, "Id$", 3);
 			dst += 3;
 			len -= dollar + 1 - src;

@@ -704,7 +715,8 @@ enum action determine_action(enum action text_attr, enum eol eol_attr) {
 }
 
 int convert_to_git(const char *path, const char *src, size_t len,
-                   struct strbuf *dst, enum safe_crlf checksafe)
+		   struct strbuf *dst, enum safe_crlf checksafe,
+		   int normalize_foreign_ident)
 {
 	struct git_attr_check check[5];
 	enum action action = CRLF_GUESS;

@@ -736,7 +748,8 @@ int convert_to_git(const char *path, const char *src, size_t len,
 		src = dst->buf;
 		len = dst->len;
 	}
-	return ret | ident_to_git(path, src, len, dst, ident);
+	return ret | ident_to_git(path, src, len, dst, ident,
+				  normalize_foreign_ident);
 }
 
 int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst)

diff --git a/diff.c b/diff.c
index 6fb97d4..c5be513 100644
--- a/diff.c
+++ b/diff.c

@@ -2372,7 +2372,7 @@ int diff_populate_filespec(struct diff_filespec *s, int size_only)
 		/*
 		 * Convert from working tree format to canonical git format
 		 */
-		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, safe_crlf, 0)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;

diff --git a/sha1_file.c b/sha1_file.c
index 0cd9435..37e8657 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -2434,7 +2434,8 @@ static int index_mem(unsigned char *sha1, void *buf, size_t size,
 	if ((type == OBJ_BLOB) && path) {
 		struct strbuf nbuf = STRBUF_INIT;
 		if (convert_to_git(path, buf, size, &nbuf,
-		                   write_object ? safe_crlf : 0)) {
+				   write_object ? safe_crlf : 0,
+				   write_object)) {
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
 		}

-- 
1.7.2

Hi,

Marcus Comstedt wrote:

  $Id: blah$ --(c_t_w_t)--> $Id: blah$ --(c_t_g)--> $Id: blah$

This restores correct and deterministic operation of status and
diff, meaning that if the file hasn't actually been modified, no
modifications are shown.

As you might suggest, always keeping the foreign ident would mean it
is never updated when you commit new versions of the file, which isn't
really what we want.  Keeping the foreign ident as long as the last
modification to the file was made in the previous version control
system makes perfect sense, but once we make a commit to the file
within git, it should be replaced with a git ident.

I was with you up to here.  Is commit time really the right moment
to clobber a foreign ident?  I suspect it would be confusing.

It might be simpler to just never clobber a foreign ident and instead
rely on local policy (scripts, pre-commit hooks, and so on) to remove
them at the appropriate time.