Jeff King <p...@peff.net> writes:

> On Tue, Feb 11, 2014 at 11:51:18AM -0800, Junio C Hamano wrote:
>
>> Those who run buildfarms may want to disable the networking test if
>> the buildfarms are not isolated well, for example.  They have to be
>> told somewhere that now they need to explicitly disable these tests
>> and how.
>
> I think they should be OK. The daemons run on the loopback interface, so
> there is hopefully not a security implication. If multiple buildfarms
> are sharing the same loopback space (e.g., running in separate
> directories on the same machine), the "auto" setting should degrade
> gracefully. One daemon will "win" the setup, and the tests will run, and
> on the other, they will be skipped.
>
>> I am in favor of this change but just pointing out possible fallouts
>> might be larger than we think.
>
> Agreed, but I think the only way to know the size of those fallouts is
> to try it and see who complains.  I would not normally be so cavalier
> with git itself, but I think for the test infrastructure, we have a
> small, tech-savvy audience that can help us iterate on it without too
> much pain.

Sure. One immediate complaint is that I would probably need to do
something like this in the build automation:

        if testing a branch without this patch
        then
                : do nothing
        else
                GIT_TEST_GIT_DAEMON=false
        fi

Arguably, it is the fault of the current/original code that treated
*any* non-empty value that is set in the environment variable as
"true"---if it paid attention to GIT_TEST_GIT_DAEMON=NoThanks, we
wouldn't have to have a workaround like this.

I wonder if GIT_TEST_X=$(test_tristate "$GIT_TEST_X") pattern can be
made a bit more friendly, though.  For example, can we behave
differently depending on the reason why $GIT_TEST_X is empty?

 - People who have *not* been opting in to the expensive tests have
   not done anything special; GIT_TEST_X environment variable did
   not exist for them (i.e. unset), and we used to skip when
   "$GIT_TEST_X" is an empty string.

 - We want to encourage people who do not care to run these tests.
   If people do not do anything, their $GIT_TEST_X will continue to
   be an empty string without GIT_TEST_X variable in the
   environment.

If we let people who *do* want to opt out of the expensive tests by
explicitly setting $GIT_TEST_X to an empty string in the new scheme,
wouldn't the transition go a lot smoother?

The caller may have to pass the name of the variable:

        GIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)

and then the callee may become

        test_tristate () {
                variable=$1
                if eval "test x\"\${$variable+isset}\" = xisset"
                then
                        # explicitly set
                        eval "value=\$$variable"
                        case "$value" in
                        "")
                                echo false ;;
                        false | auto)
                                echo "$value" ;;
                        *)
                                echo true ;;
                        esac
                else
                        echo auto
                fi
        }

so that

 - Any non-empty string other than the magic strings "false" and
   "auto" continue to mean "please I want to test";

 - Setting the variable explicitly to an empty string will continue
   to mean "no I do not want to test";

 - Leaving the variable unset will continue to mean "I don't really
   care; just follow the default the project gives me".

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to