[PATCH/RFC v2 0/3] fast-import: disallow empty branches as parents

[PATCH/RFC v2 0/3] fast-import: disallow empty branches as parents

[Dmitry Ivankov]

This is a rewrite of [1].

First of all the patch is split into several parts now.

1/3 prevents writing invalid commit objects (with null_sha1 parents)

For 2/3 and 3/3 I've changed my mind almost completely. The commands
in question are:
A 'from null_sha1'
B 'from empty_branch'
C 'from itself'
D 'merge null_sha1'
E 'merge empty_branch'
F 'merge itself'

Currently C is disallowed, D and E lead to 1/3 bug, F looks broken, A and B are allowed.

In [1] I kept A allowed, but made B, C, D, E disallowed and "fixed" F.
The idea was too keep A as legacy, fix F as it may have applications and disallow others
as they look like errors in import stream.

This time I keep A and B allowed, allow D and E, disallow F.
Now I think of null_sha1 as of a special feature, empty_branch things as a mix of legacy
and this feature (one can 'reset' branch to null_sha1, then use it's name and expect it
to work as if null_sha1 was used, and null_sha1 is allowed). "Fix" for F is dropped for
now and will later go separately with it's own set of tests and a new discussion I guess.

[1] http://thread.gmane.org/gmane.comp.version-control.git/200339

Dmitry Ivankov (3):
  fast-import: do not write null_sha1 as a merge parent
  fast-import: allow "merge $null_sha1" command
  fast-import: disallow "merge $itself" command

 fast-import.c          |   29 +++++++++++++++++++----------
 t/t9300-fast-import.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 10 deletions(-)

-- 
1.7.3.4

[PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent

[Dmitry Ivankov]

null_sha1 is used in fast-import to indicate "empty" branches and
should never be actually written out as a commit parent. 'merge'
command lacks is_null_sha1 checks and must be fixed.

It looks like using null_sha1 or empty branches in 'from' command
is legal and/or an intended option (it has been here from the very
beginning and survived). So leave it allowed for 'merge' command too,
and just like with 'from' command silently skip null_sha1 parents.

Add a simple test for null_sha1 merge parents.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |    3 ++-
 t/t9300-fast-import.sh |   21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index eed97c8..419e435 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2734,7 +2734,8 @@ static void parse_new_commit(void)
 		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
 	while (merge_list) {
 		struct hash_list *next = merge_list->next;
-		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));
+		if (!is_null_sha1(merge_list->sha1))
+			strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));
 		free(merge_list);
 		merge_list = next;
 	}
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index c17f52e..5716420 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -850,6 +850,27 @@ INPUT_END
 test_expect_success \
 	'J: tag must fail on empty branch' \
 	'test_must_fail git fast-import <input'
+
+cat >input <<INPUT_END
+reset refs/heads/J3
+
+reset refs/heads/J4
+from 0000000000000000000000000000000000000000
+
+commit refs/heads/J5
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J3, J4 into fresh J5.
+COMMIT
+merge refs/heads/J3
+merge refs/heads/J4
+
+INPUT_END
+test_expect_success \
+	'J: allow merge with empty branch' \
+	'git fast-import <input &&
+	git rev-parse --verify J5 &&
+	test_must_fail git rev-parse --verify J5^'
 ###
 ### series K
 ###
-- 
1.7.3.4

Re: [PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent

[Jonathan Nieder]

Dmitry Ivankov wrote:

> null_sha1 is used in fast-import to indicate "empty" branches and
> should never be actually written out as a commit parent. 'merge'
> command lacks is_null_sha1 checks and must be fixed.

Yeah.

> It looks like using null_sha1 or empty branches in 'from' command
> is legal and/or an intended option (it has been here from the very
> beginning and survived). So leave it allowed for 'merge' command too,
> and just like with 'from' command silently skip null_sha1 parents.

Ok, fair enough.  Are there any tests in the test script for the
"create new branch from unborn branch" trick?  Is this worth
documenting so other backend authors know what they need to do to
support frontends that work with git fast-import?

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2734,7 +2734,8 @@ static void parse_new_commit(void)
>  		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
>  	while (merge_list) {
>  		struct hash_list *next = merge_list->next;
> -		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));
> +		if (!is_null_sha1(merge_list->sha1))
> +			strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));

Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent

[Jonathan Nieder]

Hi,

In June, Dmitry Ivankov wrote:

> null_sha1 is used in fast-import to indicate "empty" branches and
> should never be actually written out as a commit parent. 'merge'
> command lacks is_null_sha1 checks and must be fixed.
>
> It looks like using null_sha1 or empty branches in 'from' command
> is legal and/or an intended option (it has been here from the very
> beginning and survived). So leave it allowed for 'merge' command too,
> and just like with 'from' command silently skip null_sha1 parents.

As Junio mentioned, this might have just been an implementation
accident --- without a use case in mind, it is hard to say that
support for the 'from 0{40}' was really intended to be part of the
supported fast-import syntax.

On the other hand it seems possible and even likely that some frontend
has taken advantage of the feature to avoid having to use conditional
logic to decide whether to emit a "from" command, since it has been
around so long.  So you are right that it's safest not to remove it.

That means that adding the same support for the "merge" command could
be a pretty bad thing, since it would be making a new promise of
continued support and would place a new burden on other implementers
of backends.

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2734,7 +2734,8 @@ static void parse_new_commit(void)
>  		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
>  	while (merge_list) {
>  		struct hash_list *next = merge_list->next;
> -		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));
> +		if (!is_null_sha1(merge_list->sha1))
> +			strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));

Since these "merge" commands produced invalid results in the past,
would it be safe to do

		if (is_null_sha1(merge_list->sha1))
			die("cannot use unborn branch or all-zeroes hash as merge parent";

instead?

> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -850,6 +850,27 @@ INPUT_END
>  test_expect_success \
>  	'J: tag must fail on empty branch' \
>  	'test_must_fail git fast-import <input'
> +
> +cat >input <<INPUT_END
> +reset refs/heads/J3
> +
> +reset refs/heads/J4
> +from 0000000000000000000000000000000000000000
> +
> +commit refs/heads/J5
> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +data <<COMMIT
> +Merge J3, J4 into fresh J5.
> +COMMIT
> +merge refs/heads/J3
> +merge refs/heads/J4
> +
> +INPUT_END
> +test_expect_success \
> +	'J: allow merge with empty branch' \
> +	'git fast-import <input &&
> +	git rev-parse --verify J5 &&
> +	test_must_fail git rev-parse --verify J5^'

Thanks for the test --- in any case, we should test the behavior.  How
about this, for now?

-- >8 --
From: Dmitry Ivankov <divanorama@gmail.com>
Subject: test: demonstrate fast-import bug that produces invalid commits with null parent

null_sha1 is used in fast-import to indicate "empty" branches and
should never be actually written out as a commit parent. 'merge'
command lacks is_null_sha1 checks and must be fixed.

[jn: extracted from a patch with a proposed fix; split into two tests]

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t9300-fast-import.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2fcf2694..f13b85b8 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -850,6 +850,42 @@ INPUT_END
 test_expect_success \
 	'J: tag must fail on empty branch' \
 	'test_must_fail git fast-import <input'
+
+cat >input <<INPUT_END
+reset refs/heads/J-unborn
+
+commit refs/heads/J-merge-unborn
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J-unborn into fresh J-merge-unborn.
+COMMIT
+merge refs/heads/J-unborn
+
+INPUT_END
+test_expect_failure \
+	'J: reject or ignore merge with unborn branch' \
+	'test_when_finished "git update-ref -d refs/heads/J-merge-unborn" &&
+	 test_might_fail git fast-import <input &&
+	 git fsck'
+
+cat >input <<INPUT_END
+reset refs/heads/J-null-sha1
+from 0000000000000000000000000000000000000000
+
+commit refs/heads/J-merge-null
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J-null-sha1 into fresh J-merge-null.
+COMMIT
+merge refs/heads/J-null-sha1
+
+INPUT_END
+test_expect_failure \
+	'J: reject or ignore merge with unborn branch' \
+	'test_when_finished "git update-ref -d refs/heads/J-merge-null" &&
+	 test_might_fail git fast-import <input &&
+	 git fsck'
+
 ###
 ### series K
 ###
-- 
1.7.10.4

[PATCH v2 2/3] fast-import: allow "merge $null_sha1" command

[Dmitry Ivankov]

"from $null_sha1" and "merge $empty_branch" are already allowed so
allow "merge $null_sha1" command too.

However such 'merge' has no effect on the import. It's made allowed
just to unify null_sha1 commits handling a little bit.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |   14 ++++++++------
 t/t9300-fast-import.sh |    1 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 419e435..f03da1e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2631,12 +2631,14 @@ static struct hash_list *parse_merge(unsigned int *count)
 				die("Mark :%" PRIuMAX " not a commit", idnum);
 			hashcpy(n->sha1, oe->idx.sha1);
 		} else if (!get_sha1(from, n->sha1)) {
-			unsigned long size;
-			char *buf = read_object_with_reference(n->sha1,
-				commit_type, &size, n->sha1);
-			if (!buf || size < 46)
-				die("Not a valid commit: %s", from);
-			free(buf);
+			if (!is_null_sha1(n->sha1)) {
+				unsigned long size;
+				char *buf = read_object_with_reference(n->sha1,
+					commit_type, &size, n->sha1);
+				if (!buf || size < 46)
+					die("Not a valid commit: %s", from);
+				free(buf);
+			}
 		} else
 			die("Invalid ref name or SHA1 expression: %s", from);
 
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 5716420..6f4c988 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -864,6 +864,7 @@ Merge J3, J4 into fresh J5.
 COMMIT
 merge refs/heads/J3
 merge refs/heads/J4
+merge 0000000000000000000000000000000000000000
 
 INPUT_END
 test_expect_success \
-- 
1.7.3.4

Re: [PATCH v2 2/3] fast-import: allow "merge $null_sha1" command

[Jonathan Nieder]

Dmitry Ivankov wrote:

> "from $null_sha1" and "merge $empty_branch" are already allowed so
> allow "merge $null_sha1" command too.

The reader might not realize that null_sha1 means
0000000000000000000000000000000000000000 until she reads the test
script.  Is it possible to help her save time?

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2631,12 +2631,14 @@ static struct hash_list *parse_merge(unsigned int *count)
>  				die("Mark :%" PRIuMAX " not a commit", idnum);
>  			hashcpy(n->sha1, oe->idx.sha1);
>  		} else if (!get_sha1(from, n->sha1)) {
> -			unsigned long size;
> -			char *buf = read_object_with_reference(n->sha1,
> -				commit_type, &size, n->sha1);
> -			if (!buf || size < 46)
> -				die("Not a valid commit: %s", from);
> -			free(buf);
> +			if (!is_null_sha1(n->sha1)) {
> +				unsigned long size;
> +				char *buf = read_object_with_reference(n->sha1,
> +					commit_type, &size, n->sha1);
> +				if (!buf || size < 46)
> +					die("Not a valid commit: %s", from);
> +				free(buf);
> +			}

Hm, ok.  Maybe the "peel onion" call guarded by this "if" could be a
separate function to make this cleaner (and avoid some duplication of
code with other functions while at it)?

e.g.,

	static int peel_to_commit(unsigned char sha1[20])
	{
		unsigned long size;

		char *buf = read_object_with_reference(...);
		if (!buf)
			return -1;
		free(buf);
		if (size < strlen("commit ") + 40)
			return -1;
		return 0;
	}

	...
		if (is_null_sha1(n->sha1))
			; /* ok */
		else if (peel_to_commit(n->sha1))
			die("Not a valid commit: %s", from);

I like the direction, but as it is, this patch feels kind of "meh" to
me.

Thanks again and hope that helps,
Jonathan

Re: [PATCH v2 2/3] fast-import: allow "merge $null_sha1" command

[Junio C Hamano]

Dmitry Ivankov <divanorama@gmail.com> writes:

> "from $null_sha1" and "merge $empty_branch" are already allowed so
> allow "merge $null_sha1" command too.

Would accepting such a "merge oops-do-not-do-anything" allow
exporters' job to be simpler?

Without a convincing "it makes sense to treat this nonsense request
as a no-op" argument, I fail to see why this is a change in the
right direction.  If there are two other nonsense request that
silently become no-op, shouldn't they be diagnosed as bugs in the
input stream, or do these two have valid uses?

Very confused.

Re: [PATCH v2 2/3] fast-import: allow "merge $null_sha1" command

[Jonathan Nieder]

Junio C Hamano wrote:
> Dmitry Ivankov <divanorama@gmail.com> writes:

>> "from $null_sha1" and "merge $empty_branch" are already allowed so
>> allow "merge $null_sha1" command too.
>
> Would accepting such a "merge oops-do-not-do-anything" allow
> exporters' job to be simpler?

Good question.

I was uncomfortable with the patch and couldn't pin down why and I
think you've hit it.

I can imagine an importer that does

	cat <<EOF
	commit refs/heads/master
	from $parent
	merge $second_parent
[etc]
	EOF

and uses parent=0000000000000000000000000000000000000000 in the
degenerate case, but it is not hard to use

	cat <<EOF
	commit refs/heads/master
	$optional_from_line$optional_second_parent
[etc]
	EOF

so this is not a very strong justification.  Mostly it felt like a
step in the right direction because once you can do it for "from",
someone might try it with "merge" and it's simplest to explain the
syntax if we're consistent.

On the other side to be weighed against that is the danger that
someone might actually start using "merge" this way.  They would be
making their frontend break compatibility with old versions of git
fast-import for no good reason.

So on second thought, it does not seem like a good direction at all.
[Though the cleanup I mentioned might be nice in any case. ;-)]

I wonder if anyone using "from" with a branch name that resolves in
the internal branch table to $null_sha1 was actually intending that.
Would any importers break if we started to forbid it?  Would it make
sense to add that check in "next" for a release or two and see if
anyone complains?

Looking at the patch for 00e2b884 (Remove branch creation command from
fast-import, 2006-08-24), it looks like support for "from $null_sha1"
was intentional.  Maybe mailing list discussions from around then have
insight.

Thanks for some food for thought,
Jonathan

Re: [PATCH v2 2/3] fast-import: allow "merge $null_sha1" command

[Jonathan Nieder]

Hi Dmitry,

Junio C Hamano wrote:
>> Dmitry Ivankov <divanorama@gmail.com> writes:

>>> "from $null_sha1" and "merge $empty_branch" are already allowed so
>>> allow "merge $null_sha1" command too.
>>
>> Would accepting such a "merge oops-do-not-do-anything" allow
>> exporters' job to be simpler?
>
> Good question.

I think this patch series had some good parts and I would like to pass
them to Junio when they're ready.

Can you send me the current version of the patches and remind me of
their status and what's left to be done?

Thanks much,
Jonathan

[PATCH v2 3/3] fast-import: disallow "merge $itself" command

[Dmitry Ivankov]

"merge $itself" may be used to create commits with previous branch tip
being repeated as n-th parent or even moved from being 1-st to be just
n-th. This is not a documented use case and doesn't look like a common
one.

In presence of "from $some" command "merge $itself" acts the same as
"merge $some" would. Which is completely undocumented and looks like
a bug (caused by parse_from() temporarily rewriting b->sha1 with $some).

Just deny "merge $itself" for now. It was a bit broken and btw "from
$itself" was and is a forbidden command too.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |   12 +++++++++---
 t/t9300-fast-import.sh |   13 +++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index f03da1e..781c614 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2611,7 +2611,7 @@ static int parse_from(struct branch *b)
 	return 1;
 }
 
-static struct hash_list *parse_merge(unsigned int *count)
+static struct hash_list *parse_merge(unsigned int *count, struct branch *b)
 {
 	struct hash_list *list = NULL, *n, *e = e;
 	const char *from;
@@ -2622,7 +2622,13 @@ static struct hash_list *parse_merge(unsigned int *count)
 		from = strchr(command_buf.buf, ' ') + 1;
 		n = xmalloc(sizeof(*n));
 		s = lookup_branch(from);
-		if (s)
+		if (b == s)
+			/*
+			 * Also if there were a 'from' command, b will point to
+			 * 'from' commit, because parse_from stores it there.
+			 */
+			die("Can't merge a branch with itself: %s", b->name);
+		else if (s)
 			hashcpy(n->sha1, s->sha1);
 		else if (*from == ':') {
 			uintmax_t idnum = parse_mark_ref_eol(from);
@@ -2686,7 +2692,7 @@ static void parse_new_commit(void)
 	parse_data(&msg, 0, NULL);
 	read_next_command();
 	parse_from(b);
-	merge_list = parse_merge(&merge_count);
+	merge_list = parse_merge(&merge_count, b);
 
 	/* ensure the branch is active/loaded */
 	if (!b->branch_tree.tree || !max_active_branches) {
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 6f4c988..79cb72a 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -872,6 +872,19 @@ test_expect_success \
 	'git fast-import <input &&
 	git rev-parse --verify J5 &&
 	test_must_fail git rev-parse --verify J5^'
+
+cat >input <<INPUT_END
+commit refs/heads/J5
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J5 with itself.
+COMMIT
+merge refs/heads/J5
+
+INPUT_END
+test_expect_success \
+	'J: disallow merge with itself' \
+	'test_must_fail git fast-import <input'
 ###
 ### series K
 ###
-- 
1.7.3.4

Re: [PATCH v2 3/3] fast-import: disallow "merge $itself" command

[Jonathan Nieder]

Dmitry Ivankov wrote:

> "merge $itself" may be used to create commits with previous branch tip
> being repeated as n-th parent or even moved from being 1-st to be just
> n-th. This is not a documented use case and doesn't look like a common
> one.
[...]
> Just deny "merge $itself" for now. It was a bit broken and btw "from
> $itself" was and is a forbidden command too.

Lovely.  Thanks for a clear patch and clear explanation.

For what it's worth, this one is
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

I'll think more about the other two and get back to you.

Re: [PATCH v2 3/3] fast-import: disallow "merge $itself" command

[Jonathan Nieder]

Hi,

In June, Dmitry Ivankov wrote:

> In presence of "from $some" command "merge $itself" acts the same as
> "merge $some" would. Which is completely undocumented and looks like
> a bug (caused by parse_from() temporarily rewriting b->sha1 with $some).

Could you give an example?

> Just deny "merge $itself" for now. It was a bit broken and btw "from
> $itself" was and is a forbidden command too.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>

Yes, this one still looks good.

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2611,7 +2611,7 @@ static int parse_from(struct branch *b)
>  	return 1;
>  }
>  
> -static struct hash_list *parse_merge(unsigned int *count)
> +static struct hash_list *parse_merge(unsigned int *count, struct branch *b)
>  {
>  	struct hash_list *list = NULL, *n, *e = e;
>  	const char *from;
> @@ -2622,7 +2622,13 @@ static struct hash_list *parse_merge(unsigned int *count)
>  		from = strchr(command_buf.buf, ' ') + 1;
>  		n = xmalloc(sizeof(*n));
>  		s = lookup_branch(from);
> -		if (s)
> +		if (b == s)

Style: "if (s == b)" would make it clearer that b is known (the current
branch) and s unknown.  Giving the 'b' parameter a meaningful name
like 'this_branch' would help even more.

> +			/*
> +			 * Also if there were a 'from' command, b will point to
> +			 * 'from' commit, because parse_from stores it there.
> +			 */
> +			die("Can't merge a branch with itself: %s", b->name);

It's not clear to me what the "Also" is referring to here.  How
about:

			/*
			 * If there was a 'from' command, b->sha1 refers to
			 * that commit instead of the previous commit on the
			 * current branch, which is probably what no one
			 * expected.
			 *
			 * Let's just reject attempts to merge a branch into
			 * itself.
			 */
			die("Can't merge a ...");

[...]
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -871,6 +871,19 @@ test_expect_success \
>  	'git fast-import <input &&
>  	git rev-parse --verify J5 &&
>  	test_must_fail git rev-parse --verify J5^'
> +
> +cat >input <<INPUT_END
> +commit refs/heads/J5
> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
> +data <<COMMIT
> +Merge J5 with itself.
> +COMMIT
> +merge refs/heads/J5
> +
> +INPUT_END
> +test_expect_success \
> +	'J: disallow merge with itself' \
> +	'test_must_fail git fast-import <input'

Looks sensible.

If the changes suggested above look good to you, I can amend locally.
Otherwise, I'll be happy to see what you come up with next.

Thanks,
Jonathan