Re: [PATCH v2] Make running git under other debugger-like programs easy

From: Elijah Newren <newren@gmail.com>
Date: Apr 10, 2018, 00:48 UTC
Message-ID: <CABPp-BF4x-ppjptq1TBj+VicvGeWGbfj4e3f5ne_13AEKmvSQQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1804092316280.55@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz>

[Re-sending, making sure the annoying rich text mode isn't turned on in gmail...]

Hi Dscho,

On Mon, Apr 9, 2018 at 2:19 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 13 quoted lines

> Hi Elijah,
>
> On Mon, 9 Apr 2018, Elijah Newren wrote:
>
>>  debug () {
>> -      GIT_TEST_GDB=1 "$@" <&6 >&5 2>&7
>> +     case "$1" in
>> +     -d)
>> +             DBG_FLAGS="$2" &&
>
> Maybe we can find a way that does not require setting a variable other
> than GIT_DEBUGGER? After all, DBG_FLAGS will be visible to the script
> calling `debug`...

I guess we could instead do a
   export GIT_DEBUGGER="$2"
here.  Short of explicitly using 'export', I think we'd need another
environment variable.

I would have stuck with your original suggestion, except that by running:
    GIT_DEBUGGER="$1" debug_aux "$@"
GIT_DEBUGGER would be set within the debug_aux function and would NOT
be set once bin-wrappers/git was called, making git not run under the
debugger as desired.

Show 9 quoted lines

>> +*)
>> +     GIT_DEBUGGER_ARGS="$GIT_DEBUGGER"
>> +     unset GIT_DEBUGGER
>> +     exec ${GIT_DEBUGGER_ARGS} "${GIT_EXEC_PATH}/@@PROG@@" "$@"
>
> It may not be a big deal, bug GIT_DEBUGGER_ARGS (if it was exported e.g.
> by the user calling the script) is now visible by the called process... (I
> thought I had tried my best to avoid such a leaking variable in my
> patch...)

You had a separate, per-process variable: GIT_DEBUGGER_$$="$GIT_DEBUGGER"

The problem with that line is that the $$ apparently makes bash treat it as a command to run rather than as a variable and a value with the desire to set one to the other. Prepending the line with 'declare' or 'local' or perhaps 'readonly' would fix that, but those aren't posix and my reading suggested that while some other shells did support some of those builtins, the support was somewhat spotty. Since wrap-for-bin.sh runs under /bin/sh, not /bin/bash, I didn't have a way of making the per-process piece work and just fell back to a separate variable. Maybe we want to change that script to /bin/bash?

Do you see any alternatives I'm missing?

Thanks, Elijah