[PATCH 1/4] convert: Safer handling of $Id$ contraction.

[PATCH 1/4] convert: Safer handling of $Id$ contraction.

[Henrik Grubbström]

From: Henrik Grubbström (Grubba) <grubba@grubba.org>

The code to contract $Id:xxxxx$ strings could eat an arbitrary amount
of source text if the terminating $ was lost. It now refuses to
contract $Id:xxxxx$ strings spanning multiple lines.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 convert.c |   17 +++++++++++++++--
 1 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index 4f8fcb7..91207ab 100644
--- a/convert.c
+++ b/convert.c
@@ -425,6 +425,7 @@ static int count_ident(const char *cp, unsigned long size)
 				cnt++;
 				break;
 			}
+			if (ch == '\n') break;
 		}
 	}
 	return cnt;
@@ -433,7 +434,7 @@ 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)
 {
-	char *dst, *dollar;
+	char *dst, *dollar, *nl;
 
 	if (!ident || !count_ident(src, len))
 		return 0;
@@ -455,6 +456,12 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 			dollar = memchr(src + 3, '$', len - 3);
 			if (!dollar)
 				break;
+			nl = memchr(src + 3, '\n', len - 3);
+			if (nl && nl < dollar) {
+				/* Line break before the next dollar. */
+				continue;
+			}
+
 			memcpy(dst, "Id$", 3);
 			dst += 3;
 			len -= dollar + 1 - src;
@@ -470,7 +477,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
                              struct strbuf *buf, int ident)
 {
 	unsigned char sha1[20];
-	char *to_free = NULL, *dollar;
+	char *to_free = NULL, *dollar, *nl;
 	int cnt;
 
 	if (!ident)
@@ -514,6 +521,12 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 				break;
 			}
 
+			nl = memchr(src + 3, '\n', len - 3);
+			if (nl && nl < dollar) {
+				/* Line break before the next dollar. */
+				continue;
+			}
+
 			len -= dollar + 1 - src;
 			src  = dollar + 1;
 		} else {
-- 
1.6.4.122.g6ffd7

[PATCH 2/4] convert: Keep foreign $Id$ on checkout.

[Henrik Grubbström]

From: Henrik Grubbström (Grubba) <grubba@grubba.org>

If there are foreign $Id$ keywords in the repository, they are most
likely there for a reason. Let's keep them on checkout (which is also
what the documentation indicates). Foreign $Id$ keywords are now
recognized by there being multiple space separated fields in $Id:xxxxx$.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 convert.c |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/convert.c b/convert.c
index 91207ab..28aeea3 100644
--- a/convert.c
+++ b/convert.c
@@ -513,7 +513,10 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 		} else if (src[2] == ':') {
 			/*
 			 * It's possible that an expanded Id has crept its way into the
-			 * repository, we cope with that by stripping the expansion out
+			 * repository, we cope with that by stripping the expansion out.
+			 * This is probably not a good idea, since it will cause changes
+			 * on checkout, which won't go away by stash, but let's keep it
+			 * for git-style ids.
 			 */
 			dollar = memchr(src + 3, '$', len - 3);
 			if (!dollar) {
@@ -527,6 +530,15 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 				continue;
 			}
 
+			if ((src[3] != ' ') ||
+			    (memchr(src + 4, ' ', len - 4) != dollar-1)) {
+				/* There are spaces in unexpected places.
+				 * This is probably an id from some other
+				 * versioning system. Keep it for now.
+				 */
+				continue;
+			}
+
 			len -= dollar + 1 - src;
 			src  = dollar + 1;
 		} else {
-- 
1.6.4.122.g6ffd7

[PATCH 3/4] convert: Inhibit contraction of foreign $Id$ during stats.

[Henrik Grubbström]

From: Henrik Grubbström (Grubba) <grubba@grubba.org>

Files containing foreign $Id$'s were reported as modified directly
on checkout, which ment that it was difficult to keep a clean working
tree when handling commits with files containing such. convert_to_git()
now takes one more mode parameter for controlling this.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 builtin-apply.c |    2 +-
 builtin-blame.c |    2 +-
 cache.h         |    8 +++++++-
 combine-diff.c  |    2 +-
 convert.c       |   20 +++++++++++++++++---
 diff.c          |    2 +-
 sha1_file.c     |    3 ++-
 7 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 3af4ae0..25adef8 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 10f7eac..f21bf3d 100644
--- a/builtin-blame.c
+++ b/builtin-blame.c
@@ -2050,7 +2050,7 @@ static struct commit *fake_working_tree_commit(const char *path, const char *con
 		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 d478eff..1c9f491 100644
--- a/cache.h
+++ b/cache.h
@@ -547,6 +547,11 @@ enum safe_crlf {
 	SAFE_CRLF_WARN = 2,
 };
 
+enum ident_mode {
+	IDENT_MODE_FALSE = 0,
+	IDENT_MODE_KEEP_FOREIGN = 1,
+};
+
 extern enum safe_crlf safe_crlf;
 
 enum branch_track {
@@ -996,7 +1001,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,
+                          enum ident_mode identmode);
 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 6162691..8c9320a 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 28aeea3..c053bcd 100644
--- a/convert.c
+++ b/convert.c
@@ -432,7 +432,8 @@ 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,
+                        enum ident_mode identmode)
 {
 	char *dst, *dollar, *nl;
 
@@ -462,6 +463,18 @@ static int ident_to_git(const char *path, const char *src, size_t len,
 				continue;
 			}
 
+			if ((identmode == IDENT_MODE_KEEP_FOREIGN) &&
+			    ((src[3] != ' ') ||
+			     (memchr(src + 4, ' ', len - 4) != dollar-1))) {
+				/* Foreign id.
+				 * Contraction of these is inhibited during
+				 * status operations to avoid all files
+				 * containing such being marked as modified
+				 * on checkout. cf sha1_file.c:index_mem().
+				 */
+				continue;
+			}
+
 			memcpy(dst, "Id$", 3);
 			dst += 3;
 			len -= dollar + 1 - src;
@@ -594,7 +607,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)
 }
 
 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,
+                   enum ident_mode identmode)
 {
 	struct git_attr_check check[3];
 	int crlf = CRLF_GUESS;
@@ -621,7 +635,7 @@ 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, identmode);
 }
 
 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 989dbc5..64726c8 100644
--- a/diff.c
+++ b/diff.c
@@ -2113,7 +2113,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 657825e..fd8c5df 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2419,7 +2419,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 ? 0 : IDENT_MODE_KEEP_FOREIGN)) {
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
 		}
-- 
1.6.4.122.g6ffd7

[PATCH 4/4] convert: Added core.refilteronadd feature.

[Henrik Grubbström]

From: Henrik Grubbström (Grubba) <grubba@grubba.org>

When having $Id$ tags in other versioning systems, it is customary
to recalculate the tags in the source on commit or equvivalent.
This commit adds a configuration option to git that causes source
files to pass through a conversion roundtrip when added to the index.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
 Documentation/config.txt |    6 +++++
 cache.h                  |    1 +
 config.c                 |    5 ++++
 environment.c            |    1 +
 sha1_file.c              |   57 ++++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 70 insertions(+), 0 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 664de6b..900b095 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -533,6 +533,12 @@ core.sparseCheckout::
 	Enable "sparse checkout" feature. See section "Sparse checkout" in
 	linkgit:git-read-tree[1] for more information.
 
+core.refilterOnAdd::
+	Enable "refilter on add" feature. This causes source files to be
+	behave as if they were checked out after a linkgit:git-add[1].
+	This is typically usefull if eg the `ident` attribute is active,
+	in which case the $Id$ tags will be updated.
+
 add.ignore-errors::
 	Tells 'git add' to continue adding files when some files cannot be
 	added due to indexing errors. Equivalent to the '--ignore-errors'
diff --git a/cache.h b/cache.h
index 1c9f491..beb60f9 100644
--- a/cache.h
+++ b/cache.h
@@ -540,6 +540,7 @@ extern int read_replace_refs;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int core_refilter_on_add;
 
 enum safe_crlf {
 	SAFE_CRLF_FALSE = 0,
diff --git a/config.c b/config.c
index 6963fbe..b1db505 100644
--- a/config.c
+++ b/config.c
@@ -523,6 +523,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.refilteronadd")) {
+		core_refilter_on_add = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 739ec27..eed7ef1 100644
--- a/environment.c
+++ b/environment.c
@@ -52,6 +52,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
 char *notes_ref_name;
 int grafts_replace_parents = 1;
 int core_apply_sparse_checkout;
+int core_refilter_on_add;
 
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
diff --git a/sha1_file.c b/sha1_file.c
index fd8c5df..f2659cb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2459,6 +2459,54 @@ int index_fd(unsigned char *sha1, int fd, struct stat *st, int write_object,
 	return ret;
 }
 
+static int refilter_fd(int fd, struct stat *st, const char *path)
+{
+	int ret = -1;
+	size_t size = xsize_t(st->st_size);
+	struct strbuf gbuf = STRBUF_INIT;
+
+	if (!S_ISREG(st->st_mode)) {
+		struct strbuf sbuf = STRBUF_INIT;
+		if (strbuf_read(&sbuf, fd, 4096) >= 0)
+			ret = convert_to_git(path, sbuf.buf, sbuf.len, &gbuf, safe_crlf, 0);
+		else
+			ret = -1;
+		strbuf_release(&sbuf);
+	} else if (size) {
+		void *buf = xmmap(NULL, size, PROT_READ, MAP_PRIVATE, fd, 0);
+		ret = convert_to_git(path, buf, size, &gbuf, safe_crlf, 0);
+		munmap(buf, size);
+	} else
+		ret = -1;
+
+	if (ret > 0) {
+		/* Something happened during conversion to git.
+		 * Now convert it back, and save the result.
+		 */
+		struct strbuf obuf = STRBUF_INIT;
+
+		lseek(fd, 0, SEEK_SET);
+
+		if (convert_to_working_tree(path, gbuf.buf, gbuf.len, &obuf)) {
+			if (write_or_whine(fd, obuf.buf, obuf.len, path))
+				ftruncate(fd, obuf.len);
+			else
+				ret = -1;
+		} else {
+			if (write_or_whine(fd, gbuf.buf, gbuf.len, path))
+				ftruncate(fd, gbuf.len);
+			else
+				ret = -1;
+		}
+
+		strbuf_release(&obuf);
+	}
+	strbuf_release(&gbuf);
+
+	close(fd);
+	return ret;
+}
+
 int index_path(unsigned char *sha1, const char *path, struct stat *st, int write_object)
 {
 	int fd;
@@ -2473,6 +2521,15 @@ int index_path(unsigned char *sha1, const char *path, struct stat *st, int write
 		if (index_fd(sha1, fd, st, write_object, OBJ_BLOB, path) < 0)
 			return error("%s: failed to insert into database",
 				     path);
+		if (write_object && core_refilter_on_add) {
+			fd = open(path, O_RDWR);
+			if (fd < 0)
+				return error("open(\"%s\"): %s", path,
+					     strerror(errno));
+			if (refilter_fd(fd, st, path) < 0)
+				return error("%s: failed to refilter file",
+					     path);
+		}
 		break;
 	case S_IFLNK:
 		if (strbuf_readlink(&sb, path, st->st_size)) {
-- 
1.6.4.122.g6ffd7

Re: [PATCH 1/4] convert: Safer handling of $Id$ contraction.

[Junio C Hamano]

Henrik Grubbström <grubba@grubba.org> writes:

> From: Henrik Grubbström (Grubba) <grubba@grubba.org>

You can omit this line, as it is the same as your From: header.

> The code to contract $Id:xxxxx$ strings could eat an arbitrary amount
> of source text if the terminating $ was lost. It now refuses to
> contract $Id:xxxxx$ strings spanning multiple lines.

Hmm, at least when going from working tree to the index, shouldn't the
code refuse _and_ die(), instead of silently pass the garbage through?

Re: [PATCH 2/4] convert: Keep foreign $Id$ on checkout.

[Junio C Hamano]

Henrik Grubbström <grubba@grubba.org> writes:

> If there are foreign $Id$ keywords in the repository, they are most
> likely there for a reason.

If so what is the user doing by using "ident" attribute?

Re: [PATCH 1/4] convert: Safer handling of $Id$ contraction.

[Henrik Grubbström]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 791 bytes --]

On Tue, 2 Mar 2010, Junio C Hamano wrote:

> Henrik Grubbström <grubba@grubba.org> writes:
>
>> From: Henrik Grubbström (Grubba) <grubba@grubba.org>
>
> You can omit this line, as it is the same as your From: header.

I blame git-send-email(1).

>> The code to contract $Id:xxxxx$ strings could eat an arbitrary amount
>> of source text if the terminating $ was lost. It now refuses to
>> contract $Id:xxxxx$ strings spanning multiple lines.
>
> Hmm, at least when going from working tree to the index, shouldn't the
> code refuse _and_ die(), instead of silently pass the garbage through?

It depends; it could be part of some code that scans for the $Id: tag.
A warning might be appropriate though.

--
Henrik Grubbström					grubba@grubba.org
Roxen Internet Software AB				grubba@roxen.com

Re: [PATCH 2/4] convert: Keep foreign $Id$ on checkout.

[Henrik Grubbström]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 701 bytes --]

On Tue, 2 Mar 2010, Junio C Hamano wrote:

> Henrik Grubbström <grubba@grubba.org> writes:
>
>> If there are foreign $Id$ keywords in the repository, they are most
>> likely there for a reason.
>
> If so what is the user doing by using "ident" attribute?

Example:

   The user has recently converted from a different versioning system,
   and kept the $Id$ tags from that system verbatim since they have been
   refered to by external systems (eg bug reports, etc).

   Having the ident property active (with my patches) will cause the $Id$
   tag to be zapped as soon as (but not before) the file is altered.

--
Henrik Grubbström					grubba@grubba.org
Roxen Internet Software AB				grubba@roxen.com

Re: [PATCH 1/4] convert: Safer handling of $Id$ contraction.

[Henrik Grubbström]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1104 bytes --]

On Wed, 3 Mar 2010, Henrik Grubbström wrote:

> On Tue, 2 Mar 2010, Junio C Hamano wrote:
>
>> Henrik Grubbström <grubba@grubba.org> writes:
>> 
>>> The code to contract $Id:xxxxx$ strings could eat an arbitrary amount
>>> of source text if the terminating $ was lost. It now refuses to
>>> contract $Id:xxxxx$ strings spanning multiple lines.
>> 
>> Hmm, at least when going from working tree to the index, shouldn't the
>> code refuse _and_ die(), instead of silently pass the garbage through?
>
> It depends; it could be part of some code that scans for the $Id: tag.
> A warning might be appropriate though.

A nonscientific survey of some version control systems gives:

VCS		Id-keyword	Eats linefeed
-------------------------------------------------
bzr		no		-
cvs		yes		no, silent accept
git(1.7.0)	yes		yes, silent
hg		yes(hgext)	no, silent accept
monotone	no		-
rcs		yes		no, silent accept
svn		yes		no, silent accept

So it seems my original patch is in line with what other version control 
systems do.

--
Henrik Grubbström					grubba@grubba.org
Roxen Internet Software AB				grubba@roxen.com

[PATCH 2/4] convert: Keep foreign $Id$ on checkout.

[Henrik Grubbström (Grubba)]

If there are foreign $Id$ keywords in the repository, they are most
likely there for a reason. Let's keep them on checkout (which is also
what the documentation indicates). Foreign $Id$ keywords are now
recognized by there being multiple space separated fields in $Id:xxxxx$.

Signed-off-by: Henrik Grubbström <grubba@grubba.org>
---
The typical use case is for repositories that have been converted
from some other VCS, where it is desirable to keep the old identifiers
around until there's some other reason to alter the file.

 convert.c             |   16 ++++++++++++++--
 t/t0021-conversion.sh |    2 +-
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/convert.c b/convert.c
index 91207ab..4165385 100644
--- a/convert.c
+++ b/convert.c
@@ -477,7 +477,7 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
                              struct strbuf *buf, int ident)
 {
 	unsigned char sha1[20];
-	char *to_free = NULL, *dollar, *nl;
+	char *to_free = NULL, *dollar, *nl, *spc;
 	int cnt;
 
 	if (!ident)
@@ -513,7 +513,10 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 		} else if (src[2] == ':') {
 			/*
 			 * It's possible that an expanded Id has crept its way into the
-			 * repository, we cope with that by stripping the expansion out
+			 * repository, we cope with that by stripping the expansion out.
+			 * This is probably not a good idea, since it will cause changes
+			 * on checkout, which won't go away by stash, but let's keep it
+			 * for git-style ids.
 			 */
 			dollar = memchr(src + 3, '$', len - 3);
 			if (!dollar) {
@@ -527,6 +530,15 @@ static int ident_to_worktree(const char *path, const char *src, size_t len,
 				continue;
 			}
 
+			spc = memchr(src + 4, ' ', len - 4);
+			if (spc && spc < dollar-1) {
+				/* There are spaces in unexpected places.
+				 * This is probably an id from some other
+				 * versioning system. Keep it for now.
+				 */
+				continue;
+			}
+
 			len -= dollar + 1 - src;
 			src  = dollar + 1;
 		} else {
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index a21a8d2..248efcc 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -78,7 +78,7 @@ test_expect_success expanded_in_repo '
 		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
 		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 \$"
 		echo "\$Id: NoTerminatingSymbol"
-		echo "\$Id: fd0478f5f1486f3d5177d4c3f6eb2765e8fc56b9 $"
+		echo "\$Id: Foreign Commit With Spaces $"
 		echo "\$Id: NoTerminatingSymbolAtEOF"
 	} > expected-output &&
 
-- 
1.6.4.122.g6ffd7