Re: [PATCH v6 p2 3/9] transport-helper: trivial code shuffle

Felipe Contreras [off-list ref] writes:

This is not just "just shuffle the die to make it explicit" but it
does change the semantics; earlier ref->deletion was perfectly fine
as long as data->refspecs is not given, but the new code always
dies.

If this semantic change is a good thing, please explain why it is so
in the log message.  If the change is "it does not matter because
when data->refspecs is not given and ref->deletion is set, we die
later elsewhere in the code anyway", then it needs to be described.


Thanks.

On Tue, Nov 27, 2012 at 6:00 PM, Junio C Hamano [off-list ref] wrote:

Felipe Contreras [off-list ref] writes:

quoted

Just shuffle the die() part to make it more explicit, and cleanup the
code-style.

Signed-off-by: Felipe Contreras <redacted>
---
 transport-helper.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index 32ad877..0c95101 100644
--- a/transport-helper.c
+++ b/transport-helper.c

@@ -775,6 +775,9 @@ static int push_refs_with_export(struct transport *transport,
              char *private;
              unsigned char sha1[20];

+             if (ref->deletion)
+                     die("remote-helpers do not support ref deletion");
+
              if (!data->refspecs)
                      continue;

This is not just "just shuffle the die to make it explicit" but it
does change the semantics; earlier ref->deletion was perfectly fine
as long as data->refspecs is not given, but the new code always
dies.

If this semantic change is a good thing, please explain why it is so
in the log message.  If the change is "it does not matter because
when data->refspecs is not given and ref->deletion is set, we die
later elsewhere in the code anyway", then it needs to be described.

refspecs are optional, but when they are not present the code doesn't
work at all. This patch changes the behavior that was totally broken
anyway.

-- 
Felipe Contreras