[PATCH/RFC 0/3] --seed as an alias for --dissociate --reference

[PATCH/RFC 0/3] --seed as an alias for --dissociate --reference

[Jeff King]

In a thread a few months ago[1], we discussed the idea that the
"--dissociate --reference=foo" interface was somewhat awkward for
somebody who just wants to optimize their clone. This is mostly due to
the historical development of the features. The logical interface for
somebody who just wants a faster clone is something like

   git clone --optimize-my-clone-from=foo.git git://example.com/bar.git

But we got stuck in that thread on coming up with a decent name for the
option. Having just read through it, I think a succinct name for the
idea is "seed". That is, we seed the clone with objects from another
repository.

That thread also brought up the idea that we do not necessarily need to
seed from a local repository; we could do something like:

  1. Fetch from the seed repo into refs/seed/*

  2. Fetch from the real clone source; the fetch is optimized by the
     presence of refs/seed/*.

  3. Delete refs/seed/*. Optionally repack to drop any objects needed
     only by the seed refs.

This is awkward with the "--reference" interface, because its
implementation is publicly tied to the concept of alternates. Whereas
"--seed" is about the end result you want; we can implement it using
alternates or with a clone, depending on where the repo is located.

There are a few open issues with this series:

  1. Assuming that "seed" is a reasonable verb for this concept, is
     "--seed=<repo>" OK for the option?  Would "--seed-from=<repo>" be
     better? (Also, the response "bleh, seed is a terrible name" is
     fine, too, but only if accompanied by your own suggestion :) ).

  2. My main goal here is making the concept easier to explain to users.
     The documentation in the third patch explains "--seed" as an alias
     for the other options, which probably isn't helping much. It might
     make sense to have a patch 4/3 that explains "--seed" first, and
     then explains "--reference" as "like --seed, but keep the
     relationship after the clone". Or maybe they should just get their
     own descriptions entirely.

  3. We can't dissociate from a specific alternate, so using "--seed"
     implies that all "--reference" options get dissociated. In this
     series, I issue a warning in that case.  But that would be easily
     solved if "--seed" used the fetch strategy described above, even
     for local clones (which would probably still be quite fast if we
     took clone's usual hard-link shortcut instead of actually fetching
     from a local clone).

I don't have particular plans to implement generic "--seed" from remotes
anytime soon. I think this takes us a step in the right direction
interface-wise, and it does introduce a succinct concept and option. But
the abstraction does leak (e.g., in that it implies "--dissociate"). So
one response might be "yes, this is a good building block, and later we
can extend --seed; how it works is an implementation detail". But
equally valid would be "eh, I like the name and the concept, but this
implementation is too hacky; let's wait for somebody to implement it for
real". Hence the RFC label.

The patches are:

  [1/3]: clone: use OPT_STRING_LIST for --reference
  [2/3]: clone: reorder --dissociate and --reference options
  [3/3]: clone: add `--seed` shorthand

The third one is the interesting one, and the first two are nearby
cleanups. Whether we pursue the third one or not, I think the first two
are worth taking by themselves.

-Peff

[1] http://thread.gmane.org/gmane.comp.version-control.git/264178/focus=264234

[PATCH 1/3] clone: use OPT_STRING_LIST for --reference

[Jeff King]

Not only does this save us having to implement a custom
callback, but it handles "--no-reference" in the usual way
(to clear the list).

The generic callback does copy the string, which we don't
technically need, but that should not hurt anything.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 166a645..1426ef5 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -51,15 +51,6 @@ static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
 
-static int opt_parse_reference(const struct option *opt, const char *arg, int unset)
-{
-	struct string_list *option_reference = opt->value;
-	if (!arg)
-		return -1;
-	string_list_append(option_reference, arg);
-	return 0;
-}
-
 static struct option builtin_clone_options[] = {
 	OPT__VERBOSITY(&option_verbosity),
 	OPT_BOOL(0, "progress", &option_progress,
@@ -83,8 +74,8 @@ static struct option builtin_clone_options[] = {
 		    N_("initialize submodules in the clone")),
 	OPT_STRING(0, "template", &option_template, N_("template-directory"),
 		   N_("directory from which templates will be used")),
-	OPT_CALLBACK(0 , "reference", &option_reference, N_("repo"),
-		     N_("reference repository"), &opt_parse_reference),
+	OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
+			N_("reference repository")),
 	OPT_STRING('o', "origin", &option_origin, N_("name"),
 		   N_("use <name> instead of 'origin' to track upstream")),
 	OPT_STRING('b', "branch", &option_branch, N_("branch"),
-- 
2.4.1.528.g00591e3

[PATCH 2/3] clone: reorder --dissociate and --reference options

[Jeff King]

These options are intimately related, so it makes sense to
list them nearby in the "-h" output (they are already
adjacent in the manpage).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/clone.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 1426ef5..a0ec1a9 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -76,6 +76,8 @@ static struct option builtin_clone_options[] = {
 		   N_("directory from which templates will be used")),
 	OPT_STRING_LIST(0, "reference", &option_reference, N_("repo"),
 			N_("reference repository")),
+	OPT_BOOL(0, "dissociate", &option_dissociate,
+		 N_("use --reference only while cloning")),
 	OPT_STRING('o', "origin", &option_origin, N_("name"),
 		   N_("use <name> instead of 'origin' to track upstream")),
 	OPT_STRING('b', "branch", &option_branch, N_("branch"),
@@ -86,8 +88,6 @@ static struct option builtin_clone_options[] = {
 		    N_("create a shallow clone of that depth")),
 	OPT_BOOL(0, "single-branch", &option_single_branch,
 		    N_("clone only one branch, HEAD or --branch")),
-	OPT_BOOL(0, "dissociate", &option_dissociate,
-		 N_("use --reference only while cloning")),
 	OPT_STRING(0, "separate-git-dir", &real_git_dir, N_("gitdir"),
 		   N_("separate git dir from working tree")),
 	OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
-- 
2.4.1.528.g00591e3

[PATCH 3/3] clone: add `--seed` shorthand

[Jeff King]

The safe way to use `--reference` is to add in the recent
`--dissociate` option, which optimizes the initial clone,
but does not create any obligation to avoid pruning or
deleting the reference repository. However, it can be rather
tricky to explain why two options are necessary, and why
using `--reference` alone is unsafe.

This patch introduces a single option, `--seed`, which does
the right thing; we can steer users towards it rather than
explaining the complexities. It also provides a natural
interface if we later want to allow seeding from non-local
repositories.

Note that git-repack cannot selectively dissociate from
particular alternates. Therefore using `--reference` and
`--seed` together will dissociate from _all_ referenced
repositories. We issue a warning to the user in this case.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-clone.txt |  3 +++
 builtin/clone.c             | 12 +++++++++++-
 t/t5700-clone-reference.sh  |  6 ++++--
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index f1f2a3f..ffeb03b 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -107,6 +107,9 @@ objects from the source repository into a pack in the cloned repository.
 	transfer and stop borrowing from them after a clone is made
 	by making necessary local copies of borrowed objects.
 
+--seed <repository>::
+	A convenient shorthand for `--dissociate --reference=<repository>`.
+
 --quiet::
 -q::
 	Operate quietly.  Progress is not reported to the standard
diff --git a/builtin/clone.c b/builtin/clone.c
index a0ec1a9..dd53bbd 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -49,6 +49,7 @@ static int option_verbosity;
 static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
+static struct string_list option_seed;
 static int option_dissociate;
 
 static struct option builtin_clone_options[] = {
@@ -78,6 +79,8 @@ static struct option builtin_clone_options[] = {
 			N_("reference repository")),
 	OPT_BOOL(0, "dissociate", &option_dissociate,
 		 N_("use --reference only while cloning")),
+	OPT_STRING_LIST(0, "seed", &option_seed, N_("repo"),
+			N_("reference and dissociate from repo")),
 	OPT_STRING('o', "origin", &option_origin, N_("name"),
 		   N_("use <name> instead of 'origin' to track upstream")),
 	OPT_STRING('b', "branch", &option_branch, N_("branch"),
@@ -263,6 +266,7 @@ static int add_one_reference(struct string_list_item *item, void *cb_data)
 static void setup_reference(void)
 {
 	for_each_string_list(&option_reference, add_one_reference, NULL);
+	for_each_string_list(&option_seed, add_one_reference, NULL);
 }
 
 static void copy_alternates(struct strbuf *src, struct strbuf *dst,
@@ -884,7 +888,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	git_config_set(key.buf, repo);
 	strbuf_reset(&key);
 
-	if (option_reference.nr)
+	if (option_seed.nr) {
+		if (option_reference.nr)
+			warning(_("--seed and --reference used together implies --dissociate"));
+		option_dissociate = 1;
+	}
+
+	if (option_reference.nr || option_seed.nr)
 		setup_reference();
 	else if (option_dissociate) {
 		warning(_("--dissociate given, but there is no --reference"));
diff --git a/t/t5700-clone-reference.sh b/t/t5700-clone-reference.sh
index 3e783fc..80a794c 100755
--- a/t/t5700-clone-reference.sh
+++ b/t/t5700-clone-reference.sh
@@ -209,10 +209,12 @@ test_expect_success 'clone and dissociate from reference' '
 	) &&
 	git clone --no-local --reference=P Q R &&
 	git clone --no-local --reference=P --dissociate Q S &&
-	# removing the reference P would corrupt R but not S
+	git clone --no-local --seed=P Q T &&
+	# removing the reference P would corrupt R but not S or T
 	rm -fr P &&
 	test_must_fail git -C R fsck &&
-	git -C S fsck
+	git -C S fsck &&
+	git -C T fsck
 '
 
 test_done
-- 
2.4.1.528.g00591e3

Re: [PATCH/RFC 0/3] --seed as an alias for --dissociate --reference

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> .... Having just read through it, I think a succinct name for the
> idea is "seed". That is, we seed the clone with objects from another
> repository.

That's a nice name.

> That thread also brought up the idea that we do not necessarily need to
> seed from a local repository; we could do something like:
>
>   1. Fetch from the seed repo into refs/seed/*
>
>   2. Fetch from the real clone source; the fetch is optimized by the
>      presence of refs/seed/*.
>
>   3. Delete refs/seed/*. Optionally repack to drop any objects needed
>      only by the seed refs.
>
> This is awkward with the "--reference" interface, because its
> implementation is publicly tied to the concept of alternates. Whereas
> "--seed" is about the end result you want; we can implement it using
> alternates or with a clone, depending on where the repo is located.

> There are a few open issues with this series:
>
>   1. Assuming that "seed" is a reasonable verb for this concept, is
>      "--seed=<repo>" OK for the option?  Would "--seed-from=<repo>" be
>      better? (Also, the response "bleh, seed is a terrible name" is
>      fine, too, but only if accompanied by your own suggestion :) ).

The seed may not even have to be a repository.  A bundle file hosted
on CDN that is reachable via (resumable) wget would be another good
way to prime the well, and it would fit with the above framework
nicely.  Grab it, fetch from it into a temporary hierarchy and then
run "fetch --prune" against the repository you originally wanted to
clone from.

>   2. My main goal here is making the concept easier to explain to users.
>      The documentation in the third patch explains "--seed" as an alias
>      for the other options, which probably isn't helping much. It might
>      make sense to have a patch 4/3 that explains "--seed" first, and
>      then explains "--reference" as "like --seed, but keep the
>      relationship after the clone". Or maybe they should just get their
>      own descriptions entirely.
>
>   3. We can't dissociate from a specific alternate, so using "--seed"
>      implies that all "--reference" options get dissociated. In this
>      series, I issue a warning in that case.  But that would be easily
>      solved if "--seed" used the fetch strategy described above, even
>      for local clones (which would probably still be quite fast if we
>      took clone's usual hard-link shortcut instead of actually fetching
>      from a local clone).
>
> I don't have particular plans to implement generic "--seed" from remotes
> anytime soon. I think this takes us a step in the right direction
> interface-wise, and it does introduce a succinct concept and
> option.

Yes.  I like the name, the concept and the general direction.

Re: [PATCH/RFC 0/3] --seed as an alias for --dissociate --reference

[Jeff King]

On Wed, May 20, 2015 at 10:01:49PM -0700, Junio C Hamano wrote:

> >   1. Assuming that "seed" is a reasonable verb for this concept, is
> >      "--seed=<repo>" OK for the option?  Would "--seed-from=<repo>" be
> >      better? (Also, the response "bleh, seed is a terrible name" is
> >      fine, too, but only if accompanied by your own suggestion :) ).
> 
> The seed may not even have to be a repository.  A bundle file hosted
> on CDN that is reachable via (resumable) wget would be another good
> way to prime the well, and it would fit with the above framework
> nicely.  Grab it, fetch from it into a temporary hierarchy and then
> run "fetch --prune" against the repository you originally wanted to
> clone from.

Yeah, I was just looking over the list archives for the past few months,
for things I had marked as "to read and think about later"[1]. That's
how I recalled our prior discussion on --dissociate.

Anyway, I happened upon the "prime the clone from a bundle" concept
being discussed again recently, and had the same thought. We already
treat local bundles as a possible source for fetching/cloning. Once upon
a time I had some patches that would let you clone straight from a
bundle over http (it just spooled to disk, which is not the _most_
efficient way to do it, but trying to massage the bundle straight into a
packfile[2] ends up every complex very quickly). I should resurrect those
patches.

-Peff

[1] My "think about later" mailbox has ~5000 messages in it, some of
    which are from 2010. I think I may need to just declare bankruptcy.

[2] There's that word again.

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Johannes Schindelin]

Hi Peff,

On 2015-05-21 06:16, Jeff King wrote:

> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index f1f2a3f..ffeb03b 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -107,6 +107,9 @@ objects from the source repository into a pack in
> the cloned repository.
>  	transfer and stop borrowing from them after a clone is made
>  	by making necessary local copies of borrowed objects.
>  
> +--seed <repository>::
> +	A convenient shorthand for `--dissociate --reference=<repository>`.
> +

Since you want to advertise this as an easier way than `--dissociate --reference=<repository>`, it might make sense to avoid sending the reader that way, too.

Maybe something like

--seed <repository>::
    Fetch objects from <repository> instead of the clone URL when possible. This is useful when a (possibly partial) clone already exists locally, to avoid transferring the same objects again.

Ciao,
Dscho

Re: [PATCH/RFC 0/3] --seed as an alias for --dissociate --reference

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I was just looking over the list archives for the past few months,
> for things I had marked as "to read and think about later"[1].
> ...
> [1] My "think about later" mailbox has ~5000 messages in it, some of
>     which are from 2010. I think I may need to just declare bankruptcy.

Were you absolutely bored and didn't have anything better to do ;-)?

Thanks for sweeping the backlog.  I usually do the same myself while
the tree is more or less frozen first week of the new cycle waiting
for brown paper bag bugs to be discovered, but since we somehow kept
moving immediately after 2.4.0 release, I didn't have a chance to do
that.

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Philip Oakley]

From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
> Hi Peff,
>
> On 2015-05-21 06:16, Jeff King wrote:
>
>> diff --git a/Documentation/git-clone.txt 
>> b/Documentation/git-clone.txt
>> index f1f2a3f..ffeb03b 100644
>> --- a/Documentation/git-clone.txt
>> +++ b/Documentation/git-clone.txt
>> @@ -107,6 +107,9 @@ objects from the source repository into a pack in
>> the cloned repository.
>>  transfer and stop borrowing from them after a clone is made
>>  by making necessary local copies of borrowed objects.
>>
>> +--seed <repository>::
>> + A convenient shorthand for `--dissociate --reference=<repository>`.
>> +
>
> Since you want to advertise this as an easier way than 
> `--dissociate --reference=<repository>`, it might make sense to avoid 
> sending the reader that way, too.
>
> Maybe something like
>
> --seed <repository>::
>    Fetch objects from <repository> instead of the clone URL when 
> possible. This is useful when a (possibly partial) clone already 
> exists locally, to avoid transferring the same objects again.
>

Would it be worth mentioning here that a bundle is a satisfactory 
alternative to repository?

+--seed <repository|bundle>::
+    Fetch objects from <repository> or <bundle> instead of the clone 
URL when possible. This is useful when a (possibly partial) clone 
already exists locally, to avoid transferring the same objects again.

I haven't checked if the invocation would accept a bundle filename, but 
I'm presuming it can in the same way that clone does.

--
Philip 

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Johannes Schindelin]

Hi Philip,

On 2015-05-21 21:45, Philip Oakley wrote:
> From: "Johannes Schindelin" <johannes.schindelin@gmx.de>
>>
>> On 2015-05-21 06:16, Jeff King wrote:
>>
>>> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
>>> index f1f2a3f..ffeb03b 100644
>>> --- a/Documentation/git-clone.txt
>>> +++ b/Documentation/git-clone.txt
>>> @@ -107,6 +107,9 @@ objects from the source repository into a pack in
>>> the cloned repository.
>>>  transfer and stop borrowing from them after a clone is made
>>>  by making necessary local copies of borrowed objects.
>>>
>>> +--seed <repository>::
>>> + A convenient shorthand for `--dissociate --reference=<repository>`.
>>> +
>>
>> Since you want to advertise this as an easier way than `--dissociate --reference=<repository>`, it might make sense to avoid sending the reader that way, too.
>>
>> Maybe something like
>>
>> --seed <repository>::
>>    Fetch objects from <repository> instead of the clone URL when possible. This is useful when a (possibly partial) clone already exists locally, to avoid transferring the same objects again.
>>
> 
> Would it be worth mentioning here that a bundle is a satisfactory
> alternative to repository?
> 
> +--seed <repository|bundle>::
> +    Fetch objects from <repository> or <bundle> instead of the clone
> URL when possible. This is useful when a (possibly partial) clone
> already exists locally, to avoid transferring the same objects again.
> 
> I haven't checked if the invocation would accept a bundle filename,
> but I'm presuming it can in the same way that clone does.

The proof would lie in the pudding ;-) Would you mind testing whether it works with bundles?

Ciao,
Dscho

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Jeff King]

On Fri, May 22, 2015 at 08:37:56AM +0200, Johannes Schindelin wrote:

> > +--seed <repository|bundle>::
> > +    Fetch objects from <repository> or <bundle> instead of the clone
> > URL when possible. This is useful when a (possibly partial) clone
> > already exists locally, to avoid transferring the same objects again.
> > 
> > I haven't checked if the invocation would accept a bundle filename,
> > but I'm presuming it can in the same way that clone does.
> 
> The proof would lie in the pudding ;-) Would you mind testing whether it works with bundles?

I can't imagine that it would with my patch. The implementation is based
on --reference, which is going to try to set up the bundle as an
alternate.

Having slept on it, I really think "--seed" should be "fetch from the
seed into temp refs", and not what I posted earlier.

-Peff

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Jeff King]

On Thu, May 21, 2015 at 06:05:31PM +0200, Johannes Schindelin wrote:

> > +--seed <repository>::
> > +	A convenient shorthand for `--dissociate --reference=<repository>`.
> > +
> 
> Since you want to advertise this as an easier way than `--dissociate --reference=<repository>`, it might make sense to avoid sending the reader that way, too.
> 
> Maybe something like
> 
> --seed <repository>::
>     Fetch objects from <repository> instead of the clone URL when
>     possible. This is useful when a (possibly partial) clone already
>     exists locally, to avoid transferring the same objects again.

Yeah, I agree that is much better (it's the "4/3" I suggested in the
cover letter).

-Peff

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Having slept on it, I really think "--seed" should be "fetch from the
> seed into temp refs", and not what I posted earlier.

Yeah, I think that is the right way to do it.

And it happens to mesh well with the (not so well advertised but not
so well hidden) plan to allow loaded server side to advise --seed
from bundle hosted elsewhere, e.g.

     * "clone" connects to upload-pack

     * upload-pack advertises --seed=http://cdn.github.com/project.bundle
       via capability

     * "clone" disconnects from upload-pack, and runs (resumable)
       wget to the seed to grab bundle.

     * "clone" then fetches refs/*:refs/remotes/origin/* from the bundle

     * "clone" then continues to fetch into +refs/remotes/origin/* as
       usual, but does an equivalent of using --prune for this fetch
       to drop anything extra/stale that the seed bundle may have
       had.

     * optionally "clone" can immediately "repack".

... which I wanted to see happen in a near future.

And that --seed thing that can name a local bundle file is a very
good first step toward the direction, I think.

Thanks.

Re: [PATCH/RFC 0/3] --seed as an alias for --dissociate --reference

[Michael Haggerty]

Thanks for working on this. I have one little bikeshedding comment...

On 05/21/2015 06:14 AM, Jeff King wrote:
> [...]
> There are a few open issues with this series:
> 
>   1. Assuming that "seed" is a reasonable verb for this concept, is
>      "--seed=<repo>" OK for the option?  Would "--seed-from=<repo>" be
>      better? (Also, the response "bleh, seed is a terrible name" is
>      fine, too, but only if accompanied by your own suggestion :) ).

I think "seed" is a pretty good name. The only downside is that "seed"
suggests that the process injects just a few seeds that are much smaller
than the whole. But in fact, (hopefully) this option causes the bulk of
the needed objects to be pre-fetched.

I can't think of any name that is clearly better. Some brainstorming:

* prepare -- meh
* prime (as in "prime the pump"). But "prime" alone could have too many
meanings, so...
* prime-from
* pre-fetch or pre-fetch-from
* pre-load or pre-load-from

BTW I think "--seed-from=<repo>" is more self-explanatory than
"--seed=<repo>".

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Jeff King]

On Sun, May 24, 2015 at 12:07:53PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Having slept on it, I really think "--seed" should be "fetch from the
> > seed into temp refs", and not what I posted earlier.
> 
> Yeah, I think that is the right way to do it.

In the meantime, do you want to pick up patches 1 and 2? I think they
are cleanups that stand on their own, whether we do patch 3 or not.

-Peff

Re: [PATCH 3/3] clone: add `--seed` shorthand

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, May 24, 2015 at 12:07:53PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > Having slept on it, I really think "--seed" should be "fetch from the
>> > seed into temp refs", and not what I posted earlier.
>> 
>> Yeah, I think that is the right way to do it.
>
> In the meantime, do you want to pick up patches 1 and 2? I think they
> are cleanups that stand on their own, whether we do patch 3 or not.

Thanks for reminding.  Let me take a look.

Thanks.