[PATCH] Fix mishandling of $Id$ expanded in the repository copy in convert.c

If the repository contained an expanded ident keyword (i.e. $Id:XXXX$),
then the wrong bytes were discarded, and the Id keyword was not
expanded.  The fault was in convert.c:ident_to_worktree().

Previously, when a "$Id:" was found in the repository version,
ident_to_worktree() would search for the next "$" after this, and
discarded everything it found until then.  That was done with the loop:

    do {
        ch = *cp++;
        if (ch == '$')
            break;
        rem--;
    } while (rem);

The above loop left cp pointing one character _after_ the final "$"
(because of ch = *cp++).  This was different from the non-expanded case,
were cp is left pointing at the "$", and was different from the comment
which stated "discard up to but not including the closing $".  This
patch fixes that by making the loop:

    do {
        ch = *cp;
        if (ch == '$')
            break;
        cp++;
        rem--;
    } while (rem);

That is, cp is tested _then_ incremented.

This loop exits if it finds a "$" or if it runs out of bytes in the
source.  After this loop, if there was no closing "$" the expansion is
skipped, and the outer loop is allowed to continue leaving this
non-keyword as it was.  However, when the "$" is found, size is
corrected, before running the expansion:

    size -= (cp - src);

This is wrong; size is going to be corrected anyway after the expansion,
so there is no need to do it here.  This patch removes that redundant
correction.

To help find this bug, I heavily commented the routine; those comments
are included here as a bonus.

Signed-off-by: Andy Parkins <redacted>
---
You wouldn't believe that I managed to get a file into the repository
with $Id$ stored expanded would you?  :-)

Anyway, it's fortunate that I did, because it revealed the above bugs
in the ident_to_worktree() code.

I've included the comments I wrote while debugging in this patch, which
I'm sure will annoy you, because you'd rather the fix and the comments
separately.  I'll supply that if you wish - just holler.

 convert.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/convert.c b/convert.c
index 4b26b1a..3c44e3d 100644
--- a/convert.c
+++ b/convert.c

@@ -509,36 +509,71 @@ static char *ident_to_worktree(const char *path, const char *src, unsigned long
 
 	for (dst = buf; size; size--) {
 		const char *cp;
+		/* Fetch next source character, move the pointer on */
 		char ch = *src++;
+		/* Copy the current character to the destination */
 		*dst++ = ch;
+		/* If the current character is "$" or there are less than three
+		 * remaining bytes or the two bytes following this one are not
+		 * "Id", then simply read the next character */
 		if ((ch != '$') || (size < 3) || memcmp("Id", src, 2))
 			continue;
+		/*
+		 * Here when
+		 *  - There are more than 2 bytes remaining
+		 *  - The current three bytes are "$Id$"
+		 * with
+		 *  - ch == "$"
+		 *  - src[0] == "I"
+		 */
 
+		/*
+		 * It's possible that an expanded Id has crept its way into the
+		 * repository, we cope with that by stripping the expansion out
+		 */
 		if (src[2] == ':') {
+			/* Expanded keywords have "$Id:" at the front */
+
 			/* discard up to but not including the closing $ */
 			unsigned long rem = size - 3;
+			/* Point at first byte after the ":" */
 			cp = src + 3;
+			/*
+			 * Throw away characters until either
+			 *  - we reach a "$"
+			 *  - we run out of bytes (rem == 0)
+			 */
 			do {
-				ch = *cp++;
+				ch = *cp;
 				if (ch == '$')
 					break;
+				cp++;
 				rem--;
 			} while (rem);
+			/* If the above finished because it ran out of characters, then
+			 * this is an incomplete keyword, so don't run the expansion */
 			if (!rem)
 				continue;
-			size -= (cp - src);
 		} else if (src[2] == '$')
 			cp = src + 2;
 		else
+			/* Anything other than "$Id:XXX$" or $Id$ and we skip the
+			 * expansion */
 			continue;
 
+		/* cp is now pointing at the last $ of the keyword */
+
 		memcpy(dst, "Id: ", 4);
 		dst += 4;
 		memcpy(dst, sha1_to_hex(sha1), 40);
 		dst += 40;
 		*dst++ = ' ';
+
+		/* Adjust for the characters we've discarded */
 		size -= (cp - src);
 		src = cp;
+
+		/* Copy the final "$" */
 		*dst++ = *src++;
 		size--;
 	}

-- 
1.5.2.763.g8c5e-dirty

On Fri, May 25, 2007 at 11:50:08AM +0100, Andy Parkins wrote:

+			/*
+			 * Throw away characters until either
+			 *  - we reach a "$"
+			 *  - we run out of bytes (rem == 0)
+			 */
 			do {
-				ch = *cp++;
+				ch = *cp;
 				if (ch == '$')
 					break;
+				cp++;
 				rem--;
 			} while (rem);

Can this loop throw away newlines? Removing newlines seems like a bad 
idea.

On Friday 2007 May 25, Joshua N Pritikin wrote:

Can this loop throw away newlines? Removing newlines seems like a bad
idea.

It can and does.  My patch is only a bug fix though, not a change in 
functionality.

It's probably not likely that they will appear inside the $Id: XXXXX $ 
expansion, and even less likely that that expansion will make it into the 
repository copy; however it's easily fixed ...  patch to follow.  


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

If a newline ever made it into an repository-side expanded $Id$ field,
the keyword would still be detected as a keyword and collapsed, before
rexpansion, e.g.

 $Id: all of this text would be removed, even if there
 were a newline in the middle of it$

This patch catches newlines in this case and abandons treating this as a
keyword expansion, this text would be left untouched in the working
checkout.

Signed-off-by: Andy Parkins <redacted>
---
 convert.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/convert.c b/convert.c
index 3c44e3d..051366c 100644
--- a/convert.c
+++ b/convert.c

@@ -547,12 +547,14 @@ static char *ident_to_worktree(const char *path, const char *src, unsigned long
 				ch = *cp;
 				if (ch == '$')
 					break;
+				if (ch == '\n')
+					break;
 				cp++;
 				rem--;
 			} while (rem);
 			/* If the above finished because it ran out of characters, then
 			 * this is an incomplete keyword, so don't run the expansion */
-			if (!rem)
+			if (!rem || ch == '\n')
 				continue;
 		} else if (src[2] == '$')
 			cp = src + 2;

-- 
1.5.2.763.g8c5e-dirty

On Fri, May 25, 2007 at 02:13:42PM +0100, Andy Parkins wrote:

If a newline ever made it into an repository-side expanded $Id$ field,
the keyword would still be detected as a keyword and collapsed, before
rexpansion, e.g.

 $Id: all of this text would be removed, even if there
 were a newline in the middle of it$

This patch catches newlines in this case and abandons treating this as a
keyword expansion, this text would be left untouched in the working
checkout.

That's better but I would error out instead of silently ignoring it.
Your choice.

On Fri, 25 May 2007, Joshua N Pritikin wrote:

On Fri, May 25, 2007 at 02:13:42PM +0100, Andy Parkins wrote:

quoted

If a newline ever made it into an repository-side expanded $Id$ field,
the keyword would still be detected as a keyword and collapsed, before
rexpansion, e.g.

 $Id: all of this text would be removed, even if there
 were a newline in the middle of it$

This patch catches newlines in this case and abandons treating this as a
keyword expansion, this text would be left untouched in the working
checkout.

That's better but I would error out instead of silently ignoring it.
Your choice.

Erroring out in such a case would simply make the system too obnoxious.

I don't think it is really worth aborting a commit just because you have 
a bad $Id:$ in one of your file.


Nicolas

On Friday 2007 May 25, Joshua N Pritikin wrote:

That's better but I would error out instead of silently ignoring it.
Your choice.

We can't error out on checking a file out - that file is in the repository 
already, if it's got problems - so be it, it's got to be possible to check it 
out.

One could even argue that it's not actually an error, if we define keywords to 
be such that they are not allowed to contain newlines, then the fact that 
someone has written "$Id:" in their file, with no closing "$" just means that 
it's not a keyword; and like every other non-keyword bit of data in the file 
it should be left untouched.



Andy

-- 
Dr Andy Parkins, M Eng (hons), MIET
andyparkins@gmail.com

On Fri, May 25, 2007 at 02:50:53PM +0100, Andy Parkins wrote:

One could even argue that it's not actually an error, if we define keywords to 
be such that they are not allowed to contain newlines, then the fact that 
someone has written "$Id:" in their file, with no closing "$" just means that 
it's not a keyword; and like every other non-keyword bit of data in the file 
it should be left untouched.

Ah, so the original patch, before I started kibitzing, was correct.

Sorry for the noise.