Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 28, 2016, 19:27 UTC
Message-ID: <xmqqa8cjjtfn.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <alpine.DEB.2.20.1611281830040.117539@virtualbox>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 6 quoted lines

> However, I have been bitten time and again by problems that occurred only
> in production, our test suite (despite taking already waaaaaay too long to
> be truly useful in my daily development) was simply not good enough.
>
> So my plan was different: to let end users opt-in to test this new beast
> thoroughly, more thoroughly than any review would.

I agree with that 100%.

We need to ensure "fallback to known working code" escape hatch is robust for that plan to work well, and that is why (1) I have been more focused on getting 1/2 right, and (2) I do not think it should be Windows-only like in your early plan, and (3) I do not think it would be "this will merely be there for only a month or so", like you said earlier.

> And for that, environment variables are just not an option. I need
> something that can be configured in a portable application, so that the
> main Git for Windows installation is unaffected.

I am not sure I follow here.

Are you saying that the users who are opting into the experiment will keep two installations, one for daily use that avoids getting hit by the experimental code and the other that is used for testing? How are they switching between the two? By using different %PATH%? I am not sure how it is different from setting an environment $GIT_TEST_BUILTIN_DIFFTOOL.

In any case, I do not have strong preference between environment and configuration. If you can make 1/2 robust with configuration, that is just as well. My message you are responding to was merely to suggest another possibility.

The latter two points in my four-bullet list are hopefully still viable if you go with the configuration; it may go something like:

 - The bulk of the tests is moved into a common dot-sourced file,
   with (1) PERL prerequite stripped and (2) "git difftool" replaced
   with $git_difftool

 - Two test files do one of

    git_difftool="git difftool"
    git_difftool="git -c difftool.useBuiltin=true difftool"

   and include the dot-sourced file.  The one that does the former
   needs to give up early depending on PERL prerequisite.

perhaps.

> My original "create a file in libexec/git-core/" was simple, did the job
> reliably, and worked also for testing.

It may have been OK for quick-and-dirty hack during development, but I do not think it was good in anything released.