Re: [PATCHv4 15/15] Replace "unset VAR" with "unset VAR;" in testsuite as per t/README

Elijah Newren [off-list ref] writes:

These three hunks look wrong as they break the && cascades.  All others
(including the change to 7502) look Ok.

Personally I do not agree that ";" at the end is explicit enough as
t/README seems to think.  If we want to be explicit, I'd say we should do
so by saying something like:

	unset VAR ;# can fail

On Wed, Sep 29, 2010 at 19:48, Junio C Hamano [off-list ref] wrote:

Elijah Newren [off-list ref] writes:

quoted

Acked-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Elijah Newren <redacted>
---

@@ -175,8 +175,8 @@ test_expect_success 'init with init.templatedir set' '

              git config -f "$test_config"  init.templatedir "${HOME}/templatedir-source" &&
              mkdir templatedir-set &&
              cd templatedir-set &&
-             unset GIT_CONFIG_NOGLOBAL &&
-             unset GIT_TEMPLATE_DIR &&
+             unset GIT_CONFIG_NOGLOBAL;
+             unset GIT_TEMPLATE_DIR;
              NO_SET_GIT_TEMPLATE_DIR=t &&
              export NO_SET_GIT_TEMPLATE_DIR &&
              git init

@@ -187,7 +187,7 @@ test_expect_success 'init with init.templatedir set' '

 test_expect_success 'init --bare/--shared overrides system/global config' '
      (
              test_config="$HOME"/.gitconfig &&
-             unset GIT_CONFIG_NOGLOBAL &&
+             unset GIT_CONFIG_NOGLOBAL;
              git config -f "$test_config" core.bare false &&
              git config -f "$test_config" core.sharedRepository 0640 &&
              mkdir init-bare-shared-override &&

@@ -202,7 +202,7 @@ test_expect_success 'init --bare/--shared overrides system/global config' '

 test_expect_success 'init honors global core.sharedRepository' '
      (
              test_config="$HOME"/.gitconfig &&
-             unset GIT_CONFIG_NOGLOBAL &&
+             unset GIT_CONFIG_NOGLOBAL;
              git config -f "$test_config" core.sharedRepository 0666 &&
              mkdir shared-honor-global &&
              cd shared-honor-global &&

These three hunks look wrong as they break the && cascades.  All others
(including the change to 7502) look Ok.

Personally I do not agree that ";" at the end is explicit enough as
t/README seems to think.  If we want to be explicit, I'd say we should do
so by saying something like:

       unset VAR ;# can fail

Or, as previously suggested:

    test_might_fail unset VAR &&

Doesn't test_might_fail work for the unset build-in too?

Hi,

Thanks for all the reviews.  I'll fix things up.  One question on this
last one...

On Wed, Sep 29, 2010 at 1:48 PM, Junio C Hamano [off-list ref] wrote:

Elijah Newren [off-list ref] writes:

quoted

Acked-by: Ævar Arnfjörð Bjarmason <redacted>
Signed-off-by: Elijah Newren <redacted>
---

@@ -175,8 +175,8 @@ test_expect_success 'init with init.templatedir set' '

              git config -f "$test_config"  init.templatedir "${HOME}/templatedir-source" &&
              mkdir templatedir-set &&
              cd templatedir-set &&
-             unset GIT_CONFIG_NOGLOBAL &&
-             unset GIT_TEMPLATE_DIR &&
+             unset GIT_CONFIG_NOGLOBAL;
+             unset GIT_TEMPLATE_DIR;
              NO_SET_GIT_TEMPLATE_DIR=t &&
              export NO_SET_GIT_TEMPLATE_DIR &&
              git init

@@ -187,7 +187,7 @@ test_expect_success 'init with init.templatedir set' '

 test_expect_success 'init --bare/--shared overrides system/global config' '
      (
              test_config="$HOME"/.gitconfig &&
-             unset GIT_CONFIG_NOGLOBAL &&
+             unset GIT_CONFIG_NOGLOBAL;
              git config -f "$test_config" core.bare false &&
              git config -f "$test_config" core.sharedRepository 0640 &&
              mkdir init-bare-shared-override &&

@@ -202,7 +202,7 @@ test_expect_success 'init --bare/--shared overrides system/global config' '

 test_expect_success 'init honors global core.sharedRepository' '
      (
              test_config="$HOME"/.gitconfig &&
-             unset GIT_CONFIG_NOGLOBAL &&
+             unset GIT_CONFIG_NOGLOBAL;
              git config -f "$test_config" core.sharedRepository 0666 &&
              mkdir shared-honor-global &&
              cd shared-honor-global &&

These three hunks look wrong as they break the && cascades.  All others
(including the change to 7502) look Ok.

They break the && cascades, but aren't the && cascades already
inherently broken due to the unportable return value of unset?

Personally I do not agree that ";" at the end is explicit enough as
t/README seems to think.  If we want to be explicit, I'd say we should do
so by saying something like:

       unset VAR ;# can fail

That makes sense.  Alternatively, I asked in an earlier posting of
this series whether it would it make sense to add a
  portable_unset () {
    unset $* || true
  }
helper function to test-lib, and just use it instead of unset?  That
would allow proper chaining everywhere.  Thoughts?