fix normalization of foreign idents

PATCH In review Marcus Comstedt 2 participants 7 messages Jun 15, 2016 Documentation

lore

Share & tools

lore

Marcus Comstedt

author

2016-06-15 22:49 UTC #0 lore raw

v3 cover fix normalization of foreign idents

New in this reroll:

* Added documentation of foreign idents

* Raised the level of abstraction for the arguments to convert_to_git which control the conversion process (both old and new), as suggested by Junio

Marcus Comstedt (4):
convert: generalize checksafe parameter
convert: fix normalization of foreign idents
t0021: test checkout and commit of foreign idents
Documentation: document foreign idents

 Documentation/gitattributes.txt |    7 +++++
 builtin/apply.c                 |    2 +-
 builtin/blame.c                 |    2 +-
 cache.h                         |   13 ++++++++-
 combine-diff.c                  |    2 +-
 convert.c                       |   26 +++++++++++++----
 diff.c                          |    2 +-
 sha1_file.c                     |    3 +-
 t/t0021-conversion.sh           |   58 +++++++++++++++++++++++++++++++++++++++
 9 files changed, 103 insertions(+), 12 deletions(-)

Signature · 1 line

1.7.2

Marcus Comstedt

author

2016-06-15 22:49 UTC #1 lore raw

PATCH v3 1/4 convert: generalize checksafe parameter

The convert_to_git() function used to have a checksafe parameter, which could be used to prevent safe_crlf checks by passing 0 instead of the value of the global variable safe_crlf.

Since preventing checks is a wider concept than just disabling safe_crlf, generalize the parameter so that it can be used for other types of checks as well.

Signed-off-by Marcus Comstedt <[email protected]>

---
 builtin/apply.c |    2 +-
 builtin/blame.c |    2 +-
 cache.h         |    7 ++++++-
 combine-diff.c  |    2 +-
 convert.c       |    7 ++++---
 diff.c          |    2 +-
 sha1_file.c     |    2 +-
 7 files changed, 15 insertions(+), 9 deletions(-)

builtin/apply.c +1 -1

index 23c18c5..638e7be 100644
@@ -1932,7 +1932,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, CHECKS_DISALLOWED);
 		return 0;
 	default:
 		return -1;

builtin/blame.c +1 -1

index 1015354..850e165 100644
@@ -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, CHECKS_DISALLOWED);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);

cache.h +6 -1

index 2ef2fa3..250abc1 100644
@@ -581,6 +581,11 @@ enum eol {
 
 extern enum eol eol;
 
+enum allow_checks {
+	CHECKS_DISALLOWED = 0,
+	CHECKS_ALLOWED = 1,
+};
+
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,

@@ -1059,7 +1064,7 @@ 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 allow_checks checksallowed);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 extern int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst);

combine-diff.c +1 -1

index 655fa89..c7f132d 100644
@@ -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, CHECKS_ALLOWED)) {
 					free(result);
 					result = strbuf_detach(&buf, &len);
 					result_size = len;

convert.c +4 -3

index 01de9a8..8050c24 100644
@@ -706,7 +706,7 @@ static 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 allow_checks checksallowed)
 {
 	struct git_attr_check check[5];
 	enum action action = CRLF_GUESS;

@@ -733,7 +733,8 @@ int convert_to_git(const char *path, const char *src, size_t len,
 		len = dst->len;
 	}
 	action = determine_action(action, eol_attr);
-	ret |= crlf_to_git(path, src, len, dst, action, checksafe);
+	ret |= crlf_to_git(path, src, len, dst, action,
+			   (checksallowed? safe_crlf : 0));
 	if (ret) {
 		src = dst->buf;
 		len = dst->len;

@@ -796,5 +797,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str
 		src = dst->buf;
 		len = dst->len;
 	}
-	return ret | convert_to_git(path, src, len, dst, 0);
+	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);
 }

diff.c +1 -1

index 9a5c77c..ed74f6b 100644
@@ -2375,7 +2375,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, CHECKS_ALLOWED)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;

sha1_file.c +1 -1

index 0cd9435..13624a6 100644
@@ -2434,7 +2434,7 @@ 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 ? CHECKS_ALLOWED : CHECKS_DISALLOWED)) {
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
 		}

Signature · 1 line

1.7.2

Marcus Comstedt

author

2016-06-15 22:49 UTC #2 lore raw

PATCH v3 2/4 convert: fix normalization of foreign idents

In reply to Marcus Comstedt

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 <[email protected]>

---
 builtin/apply.c |    2 +-
 builtin/blame.c |    2 +-
 cache.h         |    8 +++++++-
 combine-diff.c  |    2 +-
 convert.c       |   23 ++++++++++++++++++-----
 diff.c          |    2 +-
 sha1_file.c     |    3 ++-
 7 files changed, 31 insertions(+), 11 deletions(-)

builtin/apply.c +1 -1

index 638e7be..fe8d638 100644
@@ -1932,7 +1932,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, CHECKS_DISALLOWED);
+	        convert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);
 		return 0;
 	default:
 		return -1;

builtin/blame.c +1 -1

index 850e165..8d8cbf3 100644
@@ -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, CHECKS_DISALLOWED);
+	convert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);
 	origin->file.ptr = buf.buf;
 	origin->file.size = buf.len;
 	pretend_sha1_file(buf.buf, buf.len, OBJ_BLOB, origin->blob_sha1);

cache.h +7 -1

index 250abc1..3010e20 100644
@@ -586,6 +586,11 @@ enum allow_checks {
 	CHECKS_ALLOWED = 1,
 };
 
+enum normalize_mode {
+        NORMALIZE_FOR_COMPARE = 0,
+	NORMALIZE_FOR_COMMIT = 1,
+};
+
 enum branch_track {
 	BRANCH_TRACK_UNSPECIFIED = -1,
 	BRANCH_TRACK_NEVER = 0,

@@ -1064,7 +1069,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 allow_checks checksallowed);
+                          struct strbuf *dst, enum allow_checks checksallowed,
+			  enum normalize_mode mode);
 extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);
 extern int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst);

combine-diff.c +1 -1

index c7f132d..e36bf61 100644
@@ -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, CHECKS_ALLOWED)) {
+				if (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {
 					free(result);
 					result = strbuf_detach(&buf, &len);
 					result_size = len;

convert.c +18 -5

index 8050c24..4eb28d8 100644
@@ -520,9 +520,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;

@@ -549,6 +550,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;

@@ -706,7 +717,8 @@ static 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 allow_checks checksallowed)
+                   struct strbuf *dst, enum allow_checks checksallowed,
+		   enum normalize_mode mode)
 {
 	struct git_attr_check check[5];
 	enum action action = CRLF_GUESS;

@@ -739,7 +751,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,
+				  mode == NORMALIZE_FOR_COMMIT);
 }
 
 static int convert_to_working_tree_internal(const char *path, const char *src,

@@ -797,5 +810,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str
 		src = dst->buf;
 		len = dst->len;
 	}
-	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);
+	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);
 }

diff.c +1 -1

index ed74f6b..eebe3dd 100644
@@ -2375,7 +2375,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, CHECKS_ALLOWED)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {
 			size_t size = 0;
 			munmap(s->data, s->size);
 			s->should_munmap = 0;

sha1_file.c +2 -1

index 13624a6..cbebb75 100644
@@ -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 ? CHECKS_ALLOWED : CHECKS_DISALLOWED)) {
+		                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED,
+				   write_object ? NORMALIZE_FOR_COMMIT : NORMALIZE_FOR_COMPARE)) {
 			buf = strbuf_detach(&nbuf, &size);
 			re_allocated = 1;
 		}

Signature · 1 line

1.7.2

Junio C Hamano

2016-06-15 22:49 UTC #3 lore raw

Re: [PATCH v3 2/4] convert: fix normalization of foreign idents

Quoted 3 lines · Marcus Comstedt

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.

Thanks.

Quoted 2 lines

There is however one case where we want ident_to_git() to normalize the tag to $Id$ despite the asymmetry...

I'd actually think that is a bad idea. The user _can_ choose to do so by removing the stale part from '$Id: obsolate garbage$", of course. Or we can always normalize, which _might_ turn out to be a better solution. In either case, it would be better to be consistent.

builtin/apply.c static int read_old_data(struct stat *st, const char *path, struct strbuf *buf) · line 1935

Quoted 10 lines

builtin/apply.c +1 -1

index 638e7be..fe8d638 100644
@@ -1932,7 +1932,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, CHECKS_DISALLOWED);
+	        convert_to_git(path, buf->buf, buf->len, buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);

In order to apply a patch to a file in the working tree, we first need to read it, and this codepath is the place to do so. The file in the working tree may be "smudged" (in the sense of smudge/clean filter) and we "clean" it so that we can compare and update with what we read from the patch (the patch text is expected to be always "clean").

We don't check "safe-crlf", as it can "die" if it is allowed to when check_safe_clrf() finds things it does not like.

We expect to write the result out to the working tree, and at that point we will run convert_to_working_tree(). The result can also be added to the index after we are done. The resulting index may be used to create the next commit. Why "for compare" and not "for commit"?

If you get a patch to a file that contains a line with an ident, either as a context or old line. Both your working tree file and your index would have "$Id: stale garbage$".

A patch may have

- "$Id: stale garbage$" (made against the identical foreign source),
- "$Id: updated garbage$" (made against an updated foreign source), or
- "$Id$": (made against a conversion to git done elsewhere).

By telling git not to normalize "$Id: stale garbage$" you have, aren't you making the patch made not to apply 2 out of 3 above cases, especially if it came from your git friends (the last one)?

This patch doesn't touch "apply --cached" at all, which introduces yet more unnecessary inconsistency. If you made changes to the index through that codepath, shouldn't the resulting object lose the $Id: stale garbage$ somewhere before it is made into the next commit?

My gut feeling is that this kind of complications aren't worth it. If we were to address the $Id$ issue, I wonder if we can fix it the other way around, by making both directions always convert (i.e. ident_to_worktree() and ident_to_git()); the end result of such a change feels much simpler to explain to the users.

Anyway, let's continue reading your patch.

builtin/blame.c static struct commit *fake_working_tree_commit(struct diff_options *opt, · line 2098

Quoted 10 lines

builtin/blame.c +1 -1

index 850e165..8d8cbf3 100644
@@ -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, CHECKS_DISALLOWED);
+	convert_to_git(path, buf.buf, buf.len, &buf, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);

Not questionable. We are "read-only".

cache.h enum allow_checks { · line 592

Quoted 12 lines

cache.h +4 -0

index 250abc1..3010e20 100644
@@ -586,6 +586,11 @@ enum allow_checks {
 	CHECKS_ALLOWED = 1,
 };
 
+enum normalize_mode {
+        NORMALIZE_FOR_COMPARE = 0,
+	NORMALIZE_FOR_COMMIT = 1,
+};

Funny use of whitespaces.

combine-diff.c static void show_patch_diff(struct combine_diff_path *elem, int num_parent, · line 761

Quoted 10 lines

combine-diff.c +1 -1

index c7f132d..e36bf61 100644
@@ -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, CHECKS_ALLOWED)) {
+				if (convert_to_git(elem->path, result, len, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {

Not questionable. We are "read-only".

combine-diff.c int renormalize_buffer(const char *path, const char *src, size_t len, struct str · line 813

Quoted 6 lines

combine-diff.c +1 -1

@@ -797,5 +810,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str
 		src = dst->buf;
 		len = dst->len;
 	}
-	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);
+	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);

This is called from merge operation to read from the object store, and apply double-conversion (first from git to working tree and then back to git). But the result is written out as an object and hopefully be recorded as a merge commit. Why "for compare"? We would want a normalized result, no?

diff.c int diff_populate_filespec(struct diff_filespec *s, int size_only) · line 2378

Quoted 10 lines

diff.c +1 -1

index ed74f6b..eebe3dd 100644
@@ -2375,7 +2375,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, CHECKS_ALLOWED)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {

This feels wrong. This is a "read-only" access just like combine-diff and blame one. I think CHECKS_ALLOWED is probably a bug in the original. There is no reason to complain against what you haven't added.

sha1_file.c static int index_mem(unsigned char *sha1, void *buf, size_t size, · line 2438

Quoted 11 lines

sha1_file.c +2 -1

index 13624a6..cbebb75 100644
@@ -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 ? CHECKS_ALLOWED : CHECKS_DISALLOWED)) {
+		                   write_object ? CHECKS_ALLOWED : CHECKS_DISALLOWED,
+				   write_object ? NORMALIZE_FOR_COMMIT : NORMALIZE_FOR_COMPARE)) {

Although I can sort-of see why Steffen wanted to allow the former less strict in these two commands:

git hash-object <foo
git hash-object -w <foo

the code now says that they will produce different results, which feels utterly wrong.

I was hoping that from this patch a simple pattern, a correlation between the two conversion tweak flags you now have, would emerge[*1*]. And indeed it looks like it does.

If we fix the potential bug in diff_populate_filespec(), we see that checkcrlf==0 goes hand in hand with "for compare". The meaning of both option is "Are we in read-only codepath?"[*2*] and both conversion filters change their behaviour based on that single bit.

But the "for compare"/"for commit" we looked at during this review may probably have to change, and in the end it may not be a simple matter of "if we are writing we convert this way, but if we are reading we convert that other way". My gut feeling is still that if we want consistency, we should just always convert, not the other way around, though.

[footnote]

*1* By the way, as I already said, I do not like adding more and more conversion tweak flags (it makes it even more distasteful that this patch does so especially to support a check-box "feature" like ident).

*2* That is the kind of option that specifies what it _means_, not what or how it does, I suggested you to think about in the first round of review. "Do we allow checks?" wasn't the kind of a meaningful option I was hoping to see; we would want the code to clarify _why_ we allow or forego checks at individual callsites.

Marcus Comstedt

author

2016-06-15 22:49 UTC #4 lore raw

Re: [PATCH v3 2/4] convert: fix normalization of foreign idents

Hi Junio.

Quoted 5 lines · Junio C Hamano

Quoted 2 lines
There is however one case where we want ident_to_git() to normalize the tag to $Id$ despite the asymmetry...
I'd actually think that is a bad idea. The user _can_ choose to do so by removing the stale part from '$Id: obsolate garbage$", of course.

Yes, but it's easy to miss doing so. And having two versions with the same ident would be bad. Furthermore, I couldn't think of a use case where you want to use idents (as indicated by setting the "ident" attribute), and do _not_ want stale data removed.

Quoted 2 lines

Or we can always normalize, which _might_ turn out to be a better solution.

I'm not quite sure I understand what you mean by "always normalize" here. Does this mean reverting the original "foreign ident" change, which is what's preserving these idents on checkout?

[...]

Quoted 4 lines

In order to apply a patch to a file in the working tree, we first need to read it, and this codepath is the place to do so. The file in the working tree may be "smudged" (in the sense of smudge/clean filter) and we "clean" it so that we can compare and update with what we read from the patch (the

                    ^^^^^^^

Quoted 9 lines

patch text is expected to be always "clean").
We don't check "safe-crlf", as it can "die" if it is allowed to when check_safe_clrf() finds things it does not like.
We expect to write the result out to the working tree, and at that point we will run convert_to_working_tree(). The result can also be added to the index after we are done. The resulting index may be used to create the next commit. Why "for compare" and not "for commit"?

I believe you answered that question yourself above. ;-)

The file will be converted for commit once you add and commit the result that was written to the working tree. That happens in index_mem(), not here.

Quoted 6 lines

If you get a patch to a file that contains a line with an ident, either as a context or old line. Both your working tree file and your index would have "$Id: stale garbage$".
A patch may have

Quoted 7 lines

- "$Id: stale garbage$" (made against the identical foreign source),
- "$Id: updated garbage$" (made against an updated foreign source), or
- "$Id$": (made against a conversion to git done elsewhere).
By telling git not to normalize "$Id: stale garbage$" you have, aren't you making the patch made not to apply 2 out of 3 above cases, especially if it came from your git friends (the last one)?

But by normalizing it, it would just be two of the other cases where the patch does not apply, no?

By not normalizing it, a patch made against the head of a clone of the same repository would apply at least.

Or am I missing something here?

Quoted 4 lines

This patch doesn't touch "apply --cached" at all, which introduces yet more unnecessary inconsistency. If you made changes to the index through that codepath, shouldn't the resulting object lose the $Id: stale garbage$ somewhere before it is made into the next commit?

Foreign idents should always be removed by a commit which alters the file in any other way, so there might be additional handling needed for this case, I'll have to take a closer look at it.

Quoted 5 lines

My gut feeling is that this kind of complications aren't worth it. If we were to address the $Id$ issue, I wonder if we can fix it the other way around, by making both directions always convert (i.e. ident_to_worktree() and ident_to_git()); the end result of such a change feels much simpler to explain to the users.

Well, that's how it worked before 07814d9.

Quoted 6 lines

cache.h enum allow_checks { · line 592
Quoted 4 lines
cache.h +4 -0
+enum normalize_mode {
+        NORMALIZE_FOR_COMPARE = 0,
+	NORMALIZE_FOR_COMMIT = 1,
+};
Funny use of whitespaces.

Yes, it seems one of the lines got 8 spaces and the other one a tab. Not intentional.

Quoted 4 lines

convert.c +0 -0

@@ -797,5 +810,5 @@ int renormalize_buffer(const char *path, const char *src, size_t len, struct str
 		src = dst->buf;
 		len = dst->len;
 	}

Quoted 8 lines

convert.c int renormalize_buffer(const char *path, const char *src, size_t len, struct str · line 813
Quoted 2 lines
convert.c +1 -1
-	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED);
+	return ret | convert_to_git(path, src, len, dst, CHECKS_DISALLOWED, NORMALIZE_FOR_COMPARE);
This is called from merge operation to read from the object store, and apply double-conversion (first from git to working tree and then back to git). But the result is written out as an object and hopefully be recorded as a merge commit. Why "for compare"? We would want a normalized result, no?

Ok, this was a new function and I didn't fully understand its purpose. If the resulting buffer is written as an object, then "for commit" should be used, yes.

Quoted 10 lines

diff.c int diff_populate_filespec(struct diff_filespec *s, int size_only) · line 2378
Quoted 6 lines
diff.c +1 -1
@@ -2375,7 +2375,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, CHECKS_ALLOWED)) {
+		if (convert_to_git(s->path, s->data, s->size, &buf, CHECKS_ALLOWED, NORMALIZE_FOR_COMPARE)) {
This feels wrong. This is a "read-only" access just like combine-diff and blame one. I think CHECKS_ALLOWED is probably a bug in the original. There is no reason to complain against what you haven't added.

Yes, I was also a bit curious about this one. If it is indeed a bug, it probably ought to be fixed independently of this patch series.

Quoted 5 lines

*2* That is the kind of option that specifies what it _means_, not what or how it does, I suggested you to think about in the first round of review. "Do we allow checks?" wasn't the kind of a meaningful option I was hoping to see; we would want the code to clarify _why_ we allow or forego checks at individual callsites.

Well, maybe it was a little unrealistic to hope that someone new to the codebase should be able to discern exactly what you guys were thinking when you wrote a particular piece of code... ;-)

// Marcus

Marcus Comstedt

author

2016-06-15 22:49 UTC #5 lore raw

PATCH v3 3/4 t0021: test checkout and commit of foreign idents

In reply to Marcus Comstedt

Add test cases for the following behaviors:

* Checking out a file with a foreign ident should not flag the file as modified. This is to prevent a mess when checking out old versions, and to allow a migration model where files are allowed to keep their foreign ident as long as their content is also "foreign", i.e. not modified since the migration to git.

* Committing to a file with a foreign ident should replace the foreign ident with a native ident. This is simply to get the normal behavior of ident: When the contents of the file is updated, so is the ident.

Signed-off-by Marcus Comstedt <[email protected]>

---
 t/t0021-conversion.sh |   58 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 58 insertions(+), 0 deletions(-)

t/t0021-conversion.sh +58 -0

index 828e35b..cf83c02 100755
@@ -93,4 +93,62 @@ test_expect_success expanded_in_repo '
 	cmp expanded-keywords expected-output
 '
 
+# Check that a file containing idents (native or foreign) is not
+# spuriously flagged as modified on checkout
+test_expect_success 'ident pristine after checkout' '
+	{
+		echo "File with foreign ident"
+		echo "\$Id\$"
+		echo "\$Id: Foreign Commit With Spaces \$"
+	} > native-and-foreign-idents &&
+
+	{
+		echo "File with foreign ident"
+		echo "\$Id: c389f8936d7baa13f463254d55b72e00e5496e3f \$"
+		echo "\$Id: Foreign Commit With Spaces \$"
+	} > expected-output &&
+
+	git add native-and-foreign-idents &&
+	git commit -m "File with native and foreign idents" &&
+
+	echo "native-and-foreign-idents ident" >> .gitattributes &&
+
+	rm -f native-and-foreign-idents &&
+	git checkout -- native-and-foreign-idents &&
+	cat native-and-foreign-idents &&
+	cmp native-and-foreign-idents expected-output &&
+	touch native-and-foreign-idents &&
+	git status --porcelain native-and-foreign-idents > output &&
+	test ! -s output &&
+	git diff -- native-and-foreign-idents > output &&
+	test ! -s output
+'
+
+# Check that actually modifying the file and committing it produces a
+# new ident on checkout
+test_expect_success 'foreign ident replaced on commit' '
+	{
+		echo "File with foreign ident"
+		echo "\$Id: cc874844b7868ce341059e6a87c50b6f37b75807 \$"
+		echo "\$Id: cc874844b7868ce341059e6a87c50b6f37b75807 \$"
+		echo "Some new content"
+	} > expected-output &&
+
+	echo "1	0	native-and-foreign-idents" > expected-stat1 &&
+	echo "2	1	native-and-foreign-idents" > expected-stat2 &&
+
+	echo "Some new content" >> native-and-foreign-idents &&
+	git diff --numstat -- native-and-foreign-idents > output &&
+	cmp output expected-stat1 &&
+	git add native-and-foreign-idents &&
+	git commit -m "Modified file" &&
+	git diff --numstat HEAD^ HEAD -- native-and-foreign-idents > output &&
+	cmp output expected-stat2 &&
+	rm -f native-and-foreign-idents &&
+	git checkout -- native-and-foreign-idents &&
+	cat native-and-foreign-idents &&
+	cmp native-and-foreign-idents expected-output
+'
+
+
 test_done

Signature · 1 line

1.7.2

Marcus Comstedt

author

2016-06-15 22:49 UTC #6 lore raw

PATCH v3 4/4 Documentation: document foreign idents

In reply to Marcus Comstedt

Add a short paragraph to the "ident" documentation about the semantics and intended use of foreign idents.

Signed-off-by Marcus Comstedt <[email protected]>

Signed-off-by Henrik Grubbström <[email protected]>

---
 Documentation/gitattributes.txt |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

Documentation/gitattributes.txt +7 -0

index e5a27d8..9af1d6f 100644
@@ -272,6 +272,13 @@ sign `$` upon checkout.  Any byte sequence that begins with
 `$Id:` and ends with `$` in the worktree file is replaced
 with `$Id$` upon check-in.
 
+When converting a repository from a different version control
+system, it can be useful to create blobs which contain expanded
+`$Id$` tags.  Git will recognize such "foreign idents" if they
+contain at least one space within the payload.  A foreign ident
+will be replaced by `$Id$` upon check-in, but is left unaltered
+upon checkout.
+
 
 `filter`
 ^^^^^^^^

Signature · 1 line

1.7.2