[PATCH 0/2] notes: fix editor invocation regression

[PATCH 0/2] notes: fix editor invocation regression

[David Disseldorp]

This patchset attempts to revert editor invocation behavior for empty
notes, which was inadvertantly changed via 90bc19b3ae ("notes.c:
introduce '--separator=<paragraph-break>' option").
The new test passes prior to 90bc19b3ae and after this fix.

Please cc me in any replies as I'm not subscribed.

Cheers, David

--

The following changes since commit
ad57f148c6b5f8735b62238dda8f571c582e0e54:

  Git 2.46-rc2 (2024-07-23 16:54:35 -0700)

are available in the Git repository at:

  https://github.com/ddiss/git.git notes_empty_editor_add

for you to fetch changes up to fa40003e560348e146f11aa85ac1fc5a3d86e31e:

  notes: revert note_data.given behavior with empty notes add
(2024-07-25 15:41:56 +0200)

----------------------------------------------------------------
David Disseldorp (2):
      t3301-notes: check editor isn't invoked for empty notes add
      notes: revert note_data.given behavior with empty notes add

 builtin/notes.c  | 5 +++--
 t/t3301-notes.sh | 5 +++++
 2 files changed, 8 insertions(+), 2 deletions(-)

[PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[David Disseldorp]

90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
changed note_data.given logic such that it's no longer set if a zero
length file or blob object is provided.

Add a test for this regression by checking whether GIT_EDITOR is
invoked.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 t/t3301-notes.sh | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
index 536bd11ff4..c0dbacc161 100755
--- a/t/t3301-notes.sh
+++ b/t/t3301-notes.sh
@@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' '
 	test_cmp expect actual
 '
 
+test_expect_success 'empty notes do not invoke the editor' '
+	test_commit 18th &&
+	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
+'
+
 test_done
-- 
2.43.0

[PATCH 2/2] notes: revert note_data.given behavior with empty notes add

[David Disseldorp]

Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F
parameter for add / append. Following 90bc19b3ae, note_data.given is
only set if the notes data buffer length is non-zero, which results in
GIT_EDITOR invocation if e.g. a zero length blob object is provided.

Revert to pre-90bc19b3ae note_data.given behavior, to fix
non-interactive callers.

Fixes: 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
Link: https://github.com/ddiss/icyci/issues/12
Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 builtin/notes.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index d9c356e354..3ccb3eb602 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
 	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
 	d->messages[d->msg_nr - 1] = msg;
 	msg->stripspace = STRIPSPACE;
+	d->given = 1;
 	return 0;
 }
 
@@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
 	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
 	d->messages[d->msg_nr - 1] = msg;
 	msg->stripspace = STRIPSPACE;
+	d->given = 1;
 	return 0;
 }
 
@@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
 	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
 	d->messages[d->msg_nr - 1] = msg;
 	msg->stripspace = NO_STRIPSPACE;
+	d->given = 1;
 	return 0;
 }
 
@@ -515,7 +518,6 @@ static int add(int argc, const char **argv, const char *prefix)
 
 	if (d.msg_nr)
 		concat_messages(&d);
-	d.given = !!d.buf.len;
 
 	object_ref = argc > 1 ? argv[1] : "HEAD";
 
@@ -692,7 +694,6 @@ static int append_edit(int argc, const char **argv, const char *prefix)
 
 	if (d.msg_nr)
 		concat_messages(&d);
-	d.given = !!d.buf.len;
 
 	if (d.given && edit)
 		fprintf(stderr, _("The -m/-F/-c/-C options have been deprecated "
-- 
2.43.0

Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[Kristoffer Haugsbakk]

Hi

On Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote:
> 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> changed note_data.given logic such that it's no longer set if a zero
> length file or blob object is provided.

This project uses the `git show -s --pretty=reference` format:

    90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>'
    option, 2023-05-27)

>  t/t3301-notes.sh | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 536bd11ff4..c0dbacc161 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by
> git log' '
>  	test_cmp expect actual
>  '
>
> +test_expect_success 'empty notes do not invoke the editor' '
> +	test_commit 18th &&
> +	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
> +'
> +
>  test_done
> --
> 2.43.0

This test fails, obviously. Maybe you can reorder the patches so that
both two patches pass the test suite?

Introducing a regression test in one patch and then fixing the bug (and
making the test pass) in the next patch is a style that some prefer. But
I have received feedback before that it’s best to avoid that.

-- 
Kristoffer Haugsbakk

Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[David Disseldorp]

On Thu, 25 Jul 2024 17:52:25 +0200, Kristoffer Haugsbakk wrote:

> Hi
> 
> On Thu, Jul 25, 2024, at 16:41, David Disseldorp wrote:
> > 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> > changed note_data.given logic such that it's no longer set if a zero
> > length file or blob object is provided.  
> 
> This project uses the `git show -s --pretty=reference` format:
> 
>     90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>'
>     option, 2023-05-27)

Okay, will fix in a subsequent version. 

> >  t/t3301-notes.sh | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index 536bd11ff4..c0dbacc161 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by
> > git log' '
> >  	test_cmp expect actual
> >  '
> >
> > +test_expect_success 'empty notes do not invoke the editor' '
> > +	test_commit 18th &&
> > +	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
> > +'
> > +
> >  test_done
> > --
> > 2.43.0  
> 
> This test fails, obviously. Maybe you can reorder the patches so that
> both two patches pass the test suite?
> 
> Introducing a regression test in one patch and then fixing the bug (and
> making the test pass) in the next patch is a style that some prefer. But
> I have received feedback before that it’s best to avoid that.

Makes sense, thanks for the feedback.

Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[Junio C Hamano]

David Disseldorp <ddiss@suse.de> writes:

> 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> changed note_data.given logic such that it's no longer set if a zero
> length file or blob object is provided.
>
> Add a test for this regression by checking whether GIT_EDITOR is
> invoked.
>
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  t/t3301-notes.sh | 5 +++++
>  1 file changed, 5 insertions(+)

Having this test separate from 2/2 breaks bisectability.  For a
small test like this, it is often a lot more preferrable to squash
it into the patch that updates the behaviour.  It is easy to apply
the whole thing, and when the reviewer/tester is curious, it is easy
to tentatively "revert" only the behaviour changes out of the
working tree to see how the original code behaved against the test
to verify the alleged breakages were indeed there in the original.

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 536bd11ff4..c0dbacc161 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'empty notes do not invoke the editor' '
> +	test_commit 18th &&
> +	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
> +'

Clever.  By setting the editor to something that always fails, we
will notice if the command invoked it, when we do not expect the
editor to be used.

Not questioning the usefulness of this fix, and not suggesting to
remove the "--allow-empty" support out of the "git notes" command,
but out of curiosity, what are these empty notes used for?

Thanks.

Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add

[Junio C Hamano]

David Disseldorp <ddiss@suse.de> writes:

> Subject: Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add

Maybe it is just me, but "revert" has a specific connotation in the
context of the command this project develops, and when your patch is
not doing so, it gets misleading.  If you said you are restoring the
behaviour, that would be more easily understood.

    notes: do not trigger editor when adding an empty note

perhaps?

> Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F

Please make the first mention of a commit using "git show -s --pretty=reference"
format, i.e.

    90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27)

Using the reference format, besides being consistent, is very much
preferrable to allow us to tell how old the problem goes back
immediately by having the datestamp at the end.

more generally, this proposed log message starts with implementation
details.  When we have a user-visible breakage, please start from
describing that instead.  The usual way to compose a log message of
this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y" or "X
   does Y"), and discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

in this order.  So something along this line:

	With "git notes add -C $blob", the given blob contents are
	to be made into a note without involving an editor.  But
	when "--allow-empty" is given, the editor is invoked.

	This behaviour started when 90bc19b3 (notes.c: introduce
	'--separator=<paragraph-break>' option, 2023-05-27).  Prior
	to that commit, we used to do this and that ... describe
	implementation details here if you want ...

	Restore the original behaviour of "git note" that takes the
	contents given via the "-m", "-C", "-F" options without
	invoking an editor, by doing ... this and that ... describe
	implementation details here if you want ...

would be more customary.

This is not a fault of this patch, and certainly not a fault of
90bc19b3 (notes.c: introduce '--separator=<paragraph-break>' option,
2023-05-27), but unlike "git commit" and "git tag" that can take
pre-prepared contents from some source and by default use that
intact without involving an editor, "git notes" seems to lack the
ability to countermand this default and spawn an editor (e.g., "git
commit -F myfile -e").  We may want to #leftoverbits fix that.

> Fixes: 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> Link: https://github.com/ddiss/icyci/issues/12

We generally refrain from using these two trailers.  Please drop them.

Especially "Fixes" claim can later prove incorrect (we thought this
was a good fix when committing, but it later turned out to be a bad
one), and besides you will be referring to the problematic commit in
your proposed log message already anyway.

> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  builtin/notes.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index d9c356e354..3ccb3eb602 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
>  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
>  	d->messages[d->msg_nr - 1] = msg;
>  	msg->stripspace = STRIPSPACE;
> +	d->given = 1;
>  	return 0;
>  }
>  
> @@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
>  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
>  	d->messages[d->msg_nr - 1] = msg;
>  	msg->stripspace = STRIPSPACE;
> +	d->given = 1;
>  	return 0;
>  }
>  
> @@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
>  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
>  	d->messages[d->msg_nr - 1] = msg;
>  	msg->stripspace = NO_STRIPSPACE;
> +	d->given = 1;
>  	return 0;
>  }

All the above places that resurrects setting d.given looks OK, but
it makes me wonder if you need to add them in the first place.

Wouldn't it be sufficient to see if d->msg_nr is non-zero after all
the parsing is done?  If any of the message came from "-c", a
separate flag d->use_editor is set to force you run the editor, and
otherwise you'd not be using the editor, right?

> @@ -515,7 +518,6 @@ static int add(int argc, const char **argv, const char *prefix)
>  
>  	if (d.msg_nr)
>  		concat_messages(&d);
> -	d.given = !!d.buf.len;

Here is what I mean.  If you didn't do any of the "d->given = 1"
above during parsing, and instead say

	if (d.msg_nr)
		concat_messages(&d);
	d.given = d.msg_nr;

shouldn't it be sufficient to convince prepare_note_data() to do the
right thing?

Thanks.

Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[Junio C Hamano]

"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:

> Introducing a regression test in one patch and then fixing the bug (and
> making the test pass) in the next patch is a style that some prefer. But
> I have received feedback before that it’s best to avoid that.

Especially for something very small like this topic, yes.

Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add

[Kristoffer Haugsbakk]

On Thu, Jul 25, 2024, at 19:09, Junio C Hamano wrote:
>> Fixes: 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
>> Link: https://github.com/ddiss/icyci/issues/12
>
> We generally refrain from using these two trailers.  Please drop them.
>
> Especially "Fixes" claim can later prove incorrect (we thought this
> was a good fix when committing, but it later turned out to be a bad
> one), and besides you will be referring to the problematic commit in
> your proposed log message already anyway.

David, if you want to mention your downstream issue you can use a
“footnote” in the commit message:

    With "git notes add -C $blob", the given blob contents are
    to be made into a note without involving an editor.  But
    when "--allow-empty" is given, the editor is invoked.[1]

    <rest of the commit message>

    [1] https://github.com/ddiss/icyci/issues/12

    Signed-off-by: …

This is widely practiced.

-- 
Kristoffer Haugsbakk

Re: [PATCH 1/2] t3301-notes: check editor isn't invoked for empty notes add

[David Disseldorp]

On Thu, 25 Jul 2024 09:31:27 -0700, Junio C Hamano wrote:

> David Disseldorp <ddiss@suse.de> writes:
> 
> > 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> > changed note_data.given logic such that it's no longer set if a zero
> > length file or blob object is provided.
> >
> > Add a test for this regression by checking whether GIT_EDITOR is
> > invoked.
> >
> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> >  t/t3301-notes.sh | 5 +++++
> >  1 file changed, 5 insertions(+)  
> 
> Having this test separate from 2/2 breaks bisectability.  For a
> small test like this, it is often a lot more preferrable to squash
> it into the patch that updates the behaviour.  It is easy to apply
> the whole thing, and when the reviewer/tester is curious, it is easy
> to tentatively "revert" only the behaviour changes out of the
> working tree to see how the original code behaved against the test
> to verify the alleged breakages were indeed there in the original.

Understood. In that case I'll squash both patches together in the next
version.

> > diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> > index 536bd11ff4..c0dbacc161 100755
> > --- a/t/t3301-notes.sh
> > +++ b/t/t3301-notes.sh
> > @@ -1557,4 +1557,9 @@ test_expect_success 'empty notes are displayed by git log' '
> >  	test_cmp expect actual
> >  '
> >  
> > +test_expect_success 'empty notes do not invoke the editor' '
> > +	test_commit 18th &&
> > +	GIT_EDITOR="false" git notes add -C "$empty_blob" --allow-empty
> > +'  
> 
> Clever.  By setting the editor to something that always fails, we
> will notice if the command invoked it, when we do not expect the
> editor to be used.
> 
> Not questioning the usefulness of this fix, and not suggesting to
> remove the "--allow-empty" support out of the "git notes" command,
> but out of curiosity, what are these empty notes used for?

icyci (https://github.com/ddiss/icyci) attaches test-suite stderr and
stdout notes following test completion. There's no real value in
attaching empty e.g. stderr notes, but tests can also provide their own
arbitrary notes files and may wish to use (empty) note existence as a
flag in results reporting.

Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add

[David Disseldorp]

On Thu, 25 Jul 2024 10:09:26 -0700, Junio C Hamano wrote:

> David Disseldorp <ddiss@suse.de> writes:
> 
> > Subject: Re: [PATCH 2/2] notes: revert note_data.given behavior with empty notes add  
> 
> Maybe it is just me, but "revert" has a specific connotation in the
> context of the command this project develops, and when your patch is
> not doing so, it gets misleading.  If you said you are restoring the
> behaviour, that would be more easily understood.
> 
>     notes: do not trigger editor when adding an empty note
> 
> perhaps?

Sure, sounds good to me.

> > Prior to 90bc19b3ae, note_data.given was set alongside an -m, -C or -F  
> 
> Please make the first mention of a commit using "git show -s --pretty=reference"
> format, i.e.
> 
>     90bc19b3ae (notes.c: introduce '--separator=<paragraph-break>' option, 2023-05-27)
> 
> Using the reference format, besides being consistent, is very much
> preferrable to allow us to tell how old the problem goes back
> immediately by having the datestamp at the end.

Ack, will do.

> more generally, this proposed log message starts with implementation
> details.  When we have a user-visible breakage, please start from
> describing that instead.  The usual way to compose a log message of
> this project is to
> 
>  - Give an observation on how the current system work in the present
>    tense (so no need to say "Currently X is Y", just "X is Y" or "X
>    does Y"), and discuss what you perceive as a problem in it.
> 
>  - Propose a solution (optional---often, problem description
>    trivially leads to an obvious solution in reader's minds).
> 
>  - Give commands to the codebase to "become like so".
> 
> in this order.  So something along this line:
> 
> 	With "git notes add -C $blob", the given blob contents are
> 	to be made into a note without involving an editor.  But
> 	when "--allow-empty" is given, the editor is invoked.
> 
> 	This behaviour started when 90bc19b3 (notes.c: introduce
> 	'--separator=<paragraph-break>' option, 2023-05-27).  Prior
> 	to that commit, we used to do this and that ... describe
> 	implementation details here if you want ...
> 
> 	Restore the original behaviour of "git note" that takes the
> 	contents given via the "-m", "-C", "-F" options without
> 	invoking an editor, by doing ... this and that ... describe
> 	implementation details here if you want ...
> 
> would be more customary.
> 
> This is not a fault of this patch, and certainly not a fault of
> 90bc19b3 (notes.c: introduce '--separator=<paragraph-break>' option,
> 2023-05-27), but unlike "git commit" and "git tag" that can take
> pre-prepared contents from some source and by default use that
> intact without involving an editor, "git notes" seems to lack the
> ability to countermand this default and spawn an editor (e.g., "git
> commit -F myfile -e").  We may want to #leftoverbits fix that.
> 
> > Fixes: 90bc19b3ae ("notes.c: introduce '--separator=<paragraph-break>' option")
> > Link: https://github.com/ddiss/icyci/issues/12  
> 
> We generally refrain from using these two trailers.  Please drop them.
> 
> Especially "Fixes" claim can later prove incorrect (we thought this
> was a good fix when committing, but it later turned out to be a bad
> one), and besides you will be referring to the problematic commit in
> your proposed log message already anyway.

Okay, will drop the Fixes: and go with a footnote for the downstream
issue, as proposed by Kristoffer.

> > Signed-off-by: David Disseldorp <ddiss@suse.de>
> > ---
> >  builtin/notes.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/notes.c b/builtin/notes.c
> > index d9c356e354..3ccb3eb602 100644
> > --- a/builtin/notes.c
> > +++ b/builtin/notes.c
> > @@ -282,6 +282,7 @@ static int parse_msg_arg(const struct option *opt, const char *arg, int unset)
> >  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
> >  	d->messages[d->msg_nr - 1] = msg;
> >  	msg->stripspace = STRIPSPACE;
> > +	d->given = 1;
> >  	return 0;
> >  }
> >  
> > @@ -302,6 +303,7 @@ static int parse_file_arg(const struct option *opt, const char *arg, int unset)
> >  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
> >  	d->messages[d->msg_nr - 1] = msg;
> >  	msg->stripspace = STRIPSPACE;
> > +	d->given = 1;
> >  	return 0;
> >  }
> >  
> > @@ -335,6 +337,7 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
> >  	ALLOC_GROW_BY(d->messages, d->msg_nr, 1, d->msg_alloc);
> >  	d->messages[d->msg_nr - 1] = msg;
> >  	msg->stripspace = NO_STRIPSPACE;
> > +	d->given = 1;
> >  	return 0;
> >  }  
> 
> All the above places that resurrects setting d.given looks OK, but
> it makes me wonder if you need to add them in the first place.
> 
> Wouldn't it be sufficient to see if d->msg_nr is non-zero after all
> the parsing is done?  If any of the message came from "-c", a
> separate flag d->use_editor is set to force you run the editor, and
> otherwise you'd not be using the editor, right?
> 
> > @@ -515,7 +518,6 @@ static int add(int argc, const char **argv, const char *prefix)
> >  
> >  	if (d.msg_nr)
> >  		concat_messages(&d);
> > -	d.given = !!d.buf.len;  
> 
> Here is what I mean.  If you didn't do any of the "d->given = 1"
> above during parsing, and instead say
> 
> 	if (d.msg_nr)
> 		concat_messages(&d);
> 	d.given = d.msg_nr;
> 
> shouldn't it be sufficient to convince prepare_note_data() to do the
> right thing?

Yes, I also noticed that msg_nr could also be used for this, but chose
to revert back to the old given logic for clarity. I'll revisit the
msg_nr approach for v2.

Thanks for the feedback,
David