[RFC] use typechange as rename source

[RFC] use typechange as rename source

[Jeff King]

Today in one of my repositories I did something like this:

  $ mv foo bar
  $ ln -s bar foo
  $ git add .

and I expected git-status to claim:

  typechange: foo
  renamed:    foo -> bar

but it didn't find the rename (without -C) because the path 'foo' still
exists. So there is a disconnect in what git and I think of as "exists".
Should typechanges make a file eligible as a rename src?

A quickie patch to implement this is:

diff --git a/diffcore-rename.c b/diffcore-rename.c
index f9ebea5..5a34e8a 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -417,6 +417,10 @@ void diffcore_rename(struct diff_options *options)
 				p->one->rename_used++;
 			register_rename_src(p->one, p->score);
 		}
+		else if (DIFF_PAIR_TYPE_CHANGED(p)) {
+			p->one->rename_used++;
+			register_rename_src(p->one, p->score);
+		}
 		else if (detect_rename == DIFF_DETECT_COPY) {
 			/*
 			 * Increment the "rename_used" score by

There are a few add-on questions:

  - should typechanges in both directions be used, or just file ->
    symlink?

  - this actually produces a 'copied' status rather than a 'renamed'
    since the 'foo' entry does still exist. Is this reasonable?

-Peff

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> There are a few add-on questions:
>
>   - should typechanges in both directions be used, or just file ->
>     symlink?
>
>   - this actually produces a 'copied' status rather than a 'renamed'
>     since the 'foo' entry does still exist. Is this reasonable?

I do not think this is a risky change; it won't add too many rename
sources we did not consider traditionally (typechanges are usually rare
event anyway).

You are copying the source to elsewhere and then completely rewriting it
(even making it into a different type), so I do not think 'copied' is so
unreasonable.  An alternative would be to say you renamed it and then
created something totally different, which would also be reasonable.

Re: [RFC] use typechange as rename source

[Jeff King]

On Wed, Nov 28, 2007 at 04:02:49PM -0800, Junio C Hamano wrote:

> I do not think this is a risky change; it won't add too many rename
> sources we did not consider traditionally (typechanges are usually rare
> event anyway).

OK. What next? Did the patch I sent make sense? Do you want a cleaned up
version with a commit message and signoff, or does it need work?

> You are copying the source to elsewhere and then completely rewriting it
> (even making it into a different type), so I do not think 'copied' is so
> unreasonable.  An alternative would be to say you renamed it and then
> created something totally different, which would also be reasonable.

I am inclined to leave it as 'copied' for now because it makes some
sense and is the least invasive change (and is independent of adding the
rename source, so we can always change it later).

-Peff

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> OK. What next? Did the patch I sent make sense? Do you want a cleaned up
> version with a commit message and signoff, or does it need work?

It just hit me that breaking (as in diffcore-break) a filepair that is a
typechange may yield the same result, and if it works, that would be
conceptually cleaner.  After all, a typechange is the ultimate form of
total rewriting (the similarity between the preimage and the postimage
is very low -- even their types are different, let alone contents).

Compared to that, the rename_used++ in that codepath you touched feels
more magic to me.

Re: [RFC] use typechange as rename source

[Jeff King]

On Thu, Nov 29, 2007 at 05:10:45PM -0800, Junio C Hamano wrote:

> > OK. What next? Did the patch I sent make sense? Do you want a cleaned up
> > version with a commit message and signoff, or does it need work?
> 
> It just hit me that breaking (as in diffcore-break) a filepair that is a
> typechange may yield the same result, and if it works, that would be
> conceptually cleaner.  After all, a typechange is the ultimate form of
> total rewriting (the similarity between the preimage and the postimage
> is very low -- even their types are different, let alone contents).
> 
> Compared to that, the rename_used++ in that codepath you touched feels
> more magic to me.

I have always been a bit confused about diffcore-break, so I am probably
misunderstanding what you mean. But are you saying that
diffcore-break.c:should_break should return 1 for typechanges? If so,
that does not have the desired effect.

-Peff

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I have always been a bit confused about diffcore-break, so I am probably
> misunderstanding what you mean. But are you saying that
> diffcore-break.c:should_break should return 1 for typechanges?

What I had in mind was to do something like that in spirit, but instead
break such a filepair inside diffcore-rename (iow, even when the user
did not say -B) early on.

But after re-reading your patch and the surrounding code, that is
more or less what you are doing (without actually recording the extra
broken pair to be merged back later).

If we did the "automatic break of typechange" early, instead of your
patch, when we come to the register_rename_src() loop, one half of the
broken pair (i.e. "create a new symlink here") will be processed
in this part of the loop:

		if (!DIFF_FILE_VALID(p->one)) {
			if (!DIFF_FILE_VALID(p->two))
				continue; /* unmerged */
			else if (options->single_follow &&
				 strcmp(options->single_follow, p->two->path))
				continue; /* not interested */
			else
				locate_rename_dst(p->two, 1);
		}

and rename_dst is registered here.  The other half (i.e. "remove the
regular file") will be caught by this part in the loop:

		else if (!DIFF_FILE_VALID(p->two)) {
			/*
			 * If the source is a broken "delete", and
			 * they did not really want to get broken,
			 * that means the source actually stays.
			 * So we increment the "rename_used" score
			 * by one, to indicate ourselves as a user
			 */
			if (p->broken_pair && !p->score)
				p->one->rename_used++;
			register_rename_src(p->one, p->score);
		}

to register a source candidate.

Instead your patch does that with a single:

+		else if (DIFF_PAIR_TYPE_CHANGED(p)) {
+			p->one->rename_used++;
+			register_rename_src(p->one, p->score);
+		}

which is essentially doing the same thing but only for the "remove the
regular file" half.  One has to wonder how the lack of handling the
other half affects the outcome and still produce a result more intuitive
than the current code.

In your test case, the "new" symlink won't have any similar symlink that
is removed from the preimage, so registering it as a rename destination
would not make a difference (it will say "no match found, so create this
as usual"), but I am not convinced if that would work well in general.

Re: [RFC] use typechange as rename source

[Jeff King]

On Fri, Nov 30, 2007 at 06:36:56PM -0800, Junio C Hamano wrote:

> > I have always been a bit confused about diffcore-break, so I am probably
> > misunderstanding what you mean. But are you saying that
> > diffcore-break.c:should_break should return 1 for typechanges?
> 
> What I had in mind was to do something like that in spirit, but instead
> break such a filepair inside diffcore-rename (iow, even when the user
> did not say -B) early on.

Ah, I see. BTW, I totally screwed up the tests I did earlier. Returning
1 from should_break _does_ produce the same results for my simple case
(copy + typechange).

> But after re-reading your patch and the surrounding code, that is
> more or less what you are doing (without actually recording the extra
> broken pair to be merged back later).

I don't think we need to, because they are never actually "broken"; we
simply consider the source a candidate for renaming, but keep the pair
together to note the typechange.

> which is essentially doing the same thing but only for the "remove the
> regular file" half.  One has to wonder how the lack of handling the
> other half affects the outcome and still produce a result more intuitive
> than the current code.

AIUI, because we never broke the pair in the first place, we don't need
to look for a source for that dest (the "add a new symlink" half). It's
already part of the same filepair.

Whether this is by design or simply a happy accident that we record both
renames and typechanges in diff_filepairs, I'm not sure. Or perhaps I'm
totally misunderstanding how the breaking works.

> In your test case, the "new" symlink won't have any similar symlink that
> is removed from the preimage, so registering it as a rename destination
> would not make a difference (it will say "no match found, so create this
> as usual"), but I am not convinced if that would work well in general.

I don't know that it makes a difference. We are impacting only a
'typechange', which implies that we have a filepair in which both p->one
and p->two are valid; thus, the current code doesn't use it as a rename
dst at all.

-Peff

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> In your test case, the "new" symlink won't have any similar symlink that
>> is removed from the preimage, so registering it as a rename destination
>> would not make a difference (it will say "no match found, so create this
>> as usual"), but I am not convinced if that would work well in general.
>
> I don't know that it makes a difference. We are impacting only a
> 'typechange', which implies that we have a filepair in which both p->one
> and p->two are valid; thus, the current code doesn't use it as a rename
> dst at all.

I think it does make a difference, if you have a cross rename that
swaps:

	$ ls -F foo bar
        bar	foo@
        $ mv foo tmp; mv bar foo; mv tmp bar
        $ ls -F foo bar
        bar@	foo

The attached patch does not automatically break a filepair that is a
typechange, so the new t4023 test asks for diffcore-break with -B
explicitly.

I suspect your patch alone would not pass t4023 (with or without -B).

We may want to do the autobreak in diffcore-rename even when -B is not
in effect on top of this patch, but that is a separate topic.

-- >8 --
Subject: [PATCH] rename: Break filepairs with different types.

When we consider if a path has been totally rewritten, we did not
touch changes from symlinks to files or vice versa.  But a change
that modifies even the type of a blob surely should count as a
complete rewrite.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                           |    7 +++
 diffcore-break.c                  |    7 ++-
 t/t4023-diff-rename-typechange.sh |   86 +++++++++++++++++++++++++++++++++++++
 tree-walk.h                       |    7 ---
 4 files changed, 97 insertions(+), 10 deletions(-)
 create mode 100755 t/t4023-diff-rename-typechange.sh

diff --git a/cache.h b/cache.h
index aaa135b..d0e7a71 100644
--- a/cache.h
+++ b/cache.h
@@ -192,6 +192,13 @@ enum object_type {
 	OBJ_MAX,
 };
 
+static inline enum object_type object_type(unsigned int mode)
+{
+	return S_ISDIR(mode) ? OBJ_TREE :
+		S_ISGITLINK(mode) ? OBJ_COMMIT :
+		OBJ_BLOB;
+}
+
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
diff --git a/diffcore-break.c b/diffcore-break.c
index c71a226..69afc07 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -52,8 +52,8 @@ static int should_break(struct diff_filespec *src,
 			     * is the default.
 			     */
 
-	if (!S_ISREG(src->mode) || !S_ISREG(dst->mode))
-		return 0; /* leave symlink rename alone */
+	if (object_type(src->mode) != object_type(dst->mode))
+		return 1; /* even their types are different */
 
 	if (src->sha1_valid && dst->sha1_valid &&
 	    !hashcmp(src->sha1, dst->sha1))
diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
new file mode 100755
index 0000000..255604e
--- /dev/null
+++ b/t/t4023-diff-rename-typechange.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+test_description='typechange rename detection'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	ln -s linklink bar &&
+	git add foo bar &&
+	git commit -a -m Initial &&
+	git tag one &&
+
+	rm -f foo bar &&
+	cat ../../COPYING >bar &&
+	ln -s linklink foo &&
+	git add foo bar &&
+	git commit -a -m Second &&
+	git tag two &&
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	git add foo &&
+	git commit -a -m Third &&
+	git tag three &&
+
+	mv foo bar &&
+	ln -s linklink foo &&
+	git add foo bar &&
+	git commit -a -m Fourth &&
+	git tag four &&
+
+	# This is purely for sanity check
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	cat ../../Makefile >bar &&
+	git add foo bar &&
+	git commit -a -m Fifth &&
+	git tag five &&
+
+	rm -f foo bar &&
+	cat ../../Makefile >foo &&
+	cat ../../COPYING >bar &&
+	git add foo bar &&
+	git commit -a -m Sixth &&
+	git tag six
+
+'
+
+test_expect_success 'cross renames to be detected for regular files' '
+
+	git diff-tree five six -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "R100	bar	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cross renames to be detected for typechange' '
+
+	git diff-tree one two -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "R100	bar	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'moves and renames' '
+
+	git diff-tree three four -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "T100	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_done
diff --git a/tree-walk.h b/tree-walk.h
index 903a7b0..db0fbdc 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -7,13 +7,6 @@ struct name_entry {
 	unsigned int mode;
 };
 
-static inline enum object_type object_type(unsigned int mode)
-{
-	return S_ISDIR(mode) ? OBJ_TREE :
-		S_ISGITLINK(mode) ? OBJ_COMMIT :
-		OBJ_BLOB;
-}
-
 struct tree_desc {
 	const void *buffer;
 	struct name_entry entry;
-- 
1.5.3.6.2090.g4ece0

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> diff --git a/diffcore-break.c b/diffcore-break.c
> index c71a226..69afc07 100644
> --- a/diffcore-break.c
> +++ b/diffcore-break.c
> @@ -52,8 +52,8 @@ static int should_break(struct diff_filespec *src,
>  			     * is the default.
>  			     */
>  
> -	if (!S_ISREG(src->mode) || !S_ISREG(dst->mode))
> -		return 0; /* leave symlink rename alone */
> +	if (object_type(src->mode) != object_type(dst->mode))
> +		return 1; /* even their types are different */

Oops, this part is wrong.  It should be checking ISREG and stuff.

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Sorry for the earlier broken one.  Here is the correct one I wanted to
send out.

By the way, this breaks some tests in t4008 (break-rewrite test) because
the test expects the behaviour you found counterintuitive (which was the
reason we have this thread).

-- >8 --
Subject: rename: Break filepairs with different types.

When we consider if a path has been totally rewritten, we did not
touch changes from symlinks to files or vice versa.  But a change
that modifies even the type of a blob surely should count as a
complete rewrite.

While we are at it, modernise diffcore-break to be aware of gitlinks (we
do not want to touch them).

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 cache.h                           |    7 +++
 diffcore-break.c                  |   12 +++--
 t/t4023-diff-rename-typechange.sh |   86 +++++++++++++++++++++++++++++++++++++
 tree-walk.h                       |    7 ---
 4 files changed, 101 insertions(+), 11 deletions(-)
 create mode 100755 t/t4023-diff-rename-typechange.sh

diff --git a/cache.h b/cache.h
index aaa135b..d0e7a71 100644
--- a/cache.h
+++ b/cache.h
@@ -192,6 +192,13 @@ enum object_type {
 	OBJ_MAX,
 };
 
+static inline enum object_type object_type(unsigned int mode)
+{
+	return S_ISDIR(mode) ? OBJ_TREE :
+		S_ISGITLINK(mode) ? OBJ_COMMIT :
+		OBJ_BLOB;
+}
+
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
diff --git a/diffcore-break.c b/diffcore-break.c
index c71a226..31cdcfe 100644
--- a/diffcore-break.c
+++ b/diffcore-break.c
@@ -52,8 +52,10 @@ static int should_break(struct diff_filespec *src,
 			     * is the default.
 			     */
 
-	if (!S_ISREG(src->mode) || !S_ISREG(dst->mode))
-		return 0; /* leave symlink rename alone */
+	if (S_ISREG(src->mode) != S_ISREG(dst->mode)) {
+		*merge_score_p = (int)MAX_SCORE;
+		return 1; /* even their types are different */
+	}
 
 	if (src->sha1_valid && dst->sha1_valid &&
 	    !hashcmp(src->sha1, dst->sha1))
@@ -168,11 +170,13 @@ void diffcore_break(int break_score)
 		struct diff_filepair *p = q->queue[i];
 		int score;
 
-		/* We deal only with in-place edit of non directory.
+		/*
+		 * We deal only with in-place edit of blobs.
 		 * We do not break anything else.
 		 */
 		if (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two) &&
-		    !S_ISDIR(p->one->mode) && !S_ISDIR(p->two->mode) &&
+		    object_type(p->one->mode) == OBJ_BLOB &&
+		    object_type(p->two->mode) == OBJ_BLOB &&
 		    !strcmp(p->one->path, p->two->path)) {
 			if (should_break(p->one, p->two,
 					 break_score, &score)) {
diff --git a/t/t4023-diff-rename-typechange.sh b/t/t4023-diff-rename-typechange.sh
new file mode 100755
index 0000000..255604e
--- /dev/null
+++ b/t/t4023-diff-rename-typechange.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+test_description='typechange rename detection'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	ln -s linklink bar &&
+	git add foo bar &&
+	git commit -a -m Initial &&
+	git tag one &&
+
+	rm -f foo bar &&
+	cat ../../COPYING >bar &&
+	ln -s linklink foo &&
+	git add foo bar &&
+	git commit -a -m Second &&
+	git tag two &&
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	git add foo &&
+	git commit -a -m Third &&
+	git tag three &&
+
+	mv foo bar &&
+	ln -s linklink foo &&
+	git add foo bar &&
+	git commit -a -m Fourth &&
+	git tag four &&
+
+	# This is purely for sanity check
+
+	rm -f foo bar &&
+	cat ../../COPYING >foo &&
+	cat ../../Makefile >bar &&
+	git add foo bar &&
+	git commit -a -m Fifth &&
+	git tag five &&
+
+	rm -f foo bar &&
+	cat ../../Makefile >foo &&
+	cat ../../COPYING >bar &&
+	git add foo bar &&
+	git commit -a -m Sixth &&
+	git tag six
+
+'
+
+test_expect_success 'cross renames to be detected for regular files' '
+
+	git diff-tree five six -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "R100	bar	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'cross renames to be detected for typechange' '
+
+	git diff-tree one two -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "R100	bar	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_expect_success 'moves and renames' '
+
+	git diff-tree three four -r --name-status -B -M | sort >actual &&
+	{
+		echo "R100	foo	bar"
+		echo "T100	foo"
+	} | sort >expect &&
+	diff -u expect actual
+
+'
+
+test_done
diff --git a/tree-walk.h b/tree-walk.h
index 903a7b0..db0fbdc 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -7,13 +7,6 @@ struct name_entry {
 	unsigned int mode;
 };
 
-static inline enum object_type object_type(unsigned int mode)
-{
-	return S_ISDIR(mode) ? OBJ_TREE :
-		S_ISGITLINK(mode) ? OBJ_COMMIT :
-		OBJ_BLOB;
-}
-
 struct tree_desc {
 	const void *buffer;
 	struct name_entry entry;
-- 
1.5.3.6.2090.g4ece0

Re: [RFC] use typechange as rename source

[Jeff King]

On Fri, Nov 30, 2007 at 10:35:08PM -0800, Junio C Hamano wrote:

> Sorry for the earlier broken one.  Here is the correct one I wanted to
> send out.
>
> By the way, this breaks some tests in t4008 (break-rewrite test) because
> the test expects the behaviour you found counterintuitive (which was the
> reason we have this thread).

OK, this looks reasonable to me, and it produces diff results (with -B)
that make sense for typechanges. Looking at the failing test in t4008, I
like the new behavior better.

And maybe something like this on top for git-status?

diff --git a/wt-status.c b/wt-status.c
index 0e0439f..e77120d 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -250,6 +250,7 @@ static void wt_status_print_updated(struct wt_status *s)
 	rev.diffopt.format_callback_data = s;
 	rev.diffopt.detect_rename = 1;
 	rev.diffopt.rename_limit = 100;
+	rev.diffopt.break_opt = 0;
 	wt_read_cache(s);
 	run_diff_index(&rev, 1);
 }

Re: [RFC] use typechange as rename source

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And maybe something like this on top for git-status?
>
> diff --git a/wt-status.c b/wt-status.c
> index 0e0439f..e77120d 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -250,6 +250,7 @@ static void wt_status_print_updated(struct wt_status *s)
>  	rev.diffopt.format_callback_data = s;
>  	rev.diffopt.detect_rename = 1;
>  	rev.diffopt.rename_limit = 100;
> +	rev.diffopt.break_opt = 0;
>  	wt_read_cache(s);
>  	run_diff_index(&rev, 1);
>  }

I have to wonder how much this is going to make things worse in the real
world, although I agree in the "as we already spend cycles for
detect_rename why not" sense.

With the recent change from Alex not to run status when not interactive,
it probably does not matter.  If we are going to spawn an editor, we are
dealing with human interaction and even -B -M should not be too bad.

Re: [RFC] use typechange as rename source

[Jeff King]

On Sun, Dec 02, 2007 at 11:15:40AM -0800, Junio C Hamano wrote:

> > @@ -250,6 +250,7 @@ static void wt_status_print_updated(struct wt_status *s)
> > +	rev.diffopt.break_opt = 0;
> 
> I have to wonder how much this is going to make things worse in the real
> world, although I agree in the "as we already spend cycles for
> detect_rename why not" sense.
> 
> With the recent change from Alex not to run status when not interactive,
> it probably does not matter.  If we are going to spawn an editor, we are
> dealing with human interaction and even -B -M should not be too bad.

I had more or less the same thinking, but I don't have any real-world
numbers. I would be curious to see averages on how diffcore-break
compares to diffcore-rename. Just thinking about it, it seems intuitive
that breaking would always be cheaper, which means that adding "-B" to
"-M" won't have a significant performance impact.

-Peff

Re: [RFC] use typechange as rename source

[Jeff King]

On Fri, Nov 30, 2007 at 10:08:41PM -0800, Junio C Hamano wrote:

> I think it does make a difference, if you have a cross rename that
> swaps:
> 
> 	$ ls -F foo bar
>         bar	foo@
>         $ mv foo tmp; mv bar foo; mv tmp bar
>         $ ls -F foo bar
>         bar@	foo

OK, I see now what you were saying before. Yes, we do want them actually
broken, and my initial patch is not right in that sense.

> > -	if (!S_ISREG(src->mode) || !S_ISREG(dst->mode))
> > -		return 0; /* leave symlink rename alone */
> > +	if (object_type(src->mode) != object_type(dst->mode))
> > +		return 1; /* even their types are different */
> 
> Oops, this part is wrong.  It should be checking ISREG and stuff.

OK, good. I was sitting there puzzling over what in the world this
change meant.

-Peff