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

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

[Andy Parkins]

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 <andyparkins@gmail.com>
---
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

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

[Joshua N Pritikin]

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.

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

[Andy Parkins]

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

[PATCH] Don't allow newlines to occur in $Id:$ collapse

[Andy Parkins]

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 <andyparkins@gmail.com>
---
 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

Re: [PATCH] Don't allow newlines to occur in $Id:$ collapse

[Joshua N Pritikin]

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.

Re: [PATCH] Don't allow newlines to occur in $Id:$ collapse

[Nicolas Pitre]

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

> 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.

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

Re: [PATCH] Don't allow newlines to occur in $Id:$ collapse

[Andy Parkins]

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

Re: [PATCH] Don't allow newlines to occur in $Id:$ collapse

[Joshua N Pritikin]

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.

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> 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.

Actually I like well commented code, although some of your
comments feel a tad too much at places.  For example,

>  	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;

These are too much.

> +		/* 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"
> +		 */

But this is very good, if you fix it to read the current 3 are
"$Id" ;-).

> +		/*
> +		 * It's possible that an expanded Id has crept its way into the
> +		 * repository, we cope with that by stripping the expansion out
> +		 */

So are all the other comments.

Thanks for the fix.  It would be very nice for the patch to be
accompanied with a new test to expose the bug and demonstrate
that the patch fixes it.

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

[Andy Parkins]

On Saturday 2007, May 26, Junio C Hamano wrote:

> >  	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;
>
> These are too much.

Absolutely.  I always find when bug hunting though that I like to 
comment every block, sometimes to excess, as reminder that I've 
understood what its doing.

I should have done the final pass once I'd found it to clear up the 
overkill ones :-)

> But this is very good, if you fix it to read the current 3 are
> "$Id" ;-).

"and in the ability to count competition, Andy comes second... let's 
have a big hand for our runner up" :-)

> Thanks for the fix.  It would be very nice for the patch to be
> accompanied with a new test to expose the bug and demonstrate
> that the patch fixes it.

I had to jump through quite a few hoops to get the expanded $Id$ into a 
repository (originally it was because I used an older version of git in 
one place, and a newer one in another).

I'll see what I can do to make a test case though.


Andy

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

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

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> I had to jump through quite a few hoops to get the expanded $Id$ into a 
> repository (originally it was because I used an older version of git in 
> one place, and a newer one in another).
>
> I'll see what I can do to make a test case though.

Wouldn't it be sufficient to:

 (1) prepare a file with "$Id$", use ident in .gitattributes,
     check it in and commit;

 (2) remove it from the working tree, check it out with
     "checkout -f";

 (3) temorarily move away .gitattributes, modify the file, and
     check it in;

 (4) move .gitattributes back into its place, and commit.

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

[Andy Parkins]

On Saturday 2007, May 26, Junio C Hamano wrote:

> Wouldn't it be sufficient to:
>
>  (1) prepare a file with "$Id$", use ident in .gitattributes,
>      check it in and commit;
>
>  (2) remove it from the working tree, check it out with
>      "checkout -f";
>
>  (3) temorarily move away .gitattributes, modify the file, and
>      check it in;
>
>  (4) move .gitattributes back into its place, and commit.

I'm glad to have you confirm that.  I wasn't sure if git would do 
something clever and reading the .gitattributes from the same commit as 
the file for which is being checked out.

If the above would work, then even simpler:

 (1) Commit a file with $Id: blah blah blah $ in it.
 (2) Add a .gitattributes with ident
 (3) Check out.

Patch to follow.


Andy

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

[PATCH] Add test case for $Id$ expanded in the repository

[Andy Parkins]

This test case would have caught the bug fixed by revision
c23290d5.

It puts various forms of $Id$ into a file in the repository,
without allowing git to collapse them to uniformity.  Then enables the
$Id$ expansion on checkout, and checks that what is checked out has
coped with the various forms.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
 t/t0021-conversion.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 6c26fd8..a839f4e 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -45,4 +45,40 @@ test_expect_success check '
 	test "z$id" = "z$embedded"
 '
 
+# If an expanded ident ever gets into the repository, we want to make sure that
+# it is collapsed before being expanded again on checkout
+test_expect_success expanded_in_repo '
+	{
+		echo "File with expanded keywords"
+		echo "\$Id\$"
+		echo "\$Id:\$"
+		echo "\$Id: 0000000000000000000000000000000000000000 \$"
+		echo "\$Id: NoSpaceAtEnd\$"
+		echo "\$Id:NoSpaceAtFront \$"
+		echo "\$Id:NoSpaceAtEitherEnd\$"
+		echo "\$Id: NoTerminatingSymbol"
+	} > expanded-keywords &&
+
+	{
+		echo "File with expanded keywords"
+		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: 4f21723e7b15065df7de95bd46c8ba6fb1818f4c \$"
+		echo "\$Id: NoTerminatingSymbol"
+	} > expected-output &&
+
+	git add expanded-keywords &&
+	git commit -m "File with keywords expanded" &&
+
+	echo "expanded-keywords ident" >> .gitattributes &&
+
+	rm -f expanded-keywords &&
+	git checkout -- expanded-keywords &&
+	cat expanded-keywords &&
+	cmp expanded-keywords expected-output
+'
+
 test_done
-- 
1.5.2.86.g99b5-dirty