Re: [PATCH v2] git: submodule honor -c credential.* from command line

From: Jacob Keller <jacob.keller@gmail.com>
Date: Feb 25, 2016, 06:19 UTC
Message-ID: <CA+P7+xpkbZyuQ7gpC+_KzJPfGo+xHpVFWEmr1oS7FD30jjJSOQ@mail.gmail.com>
In-Reply-To: <CAPig+cROoc+Y-V_6hw6Hx4X9pFGmRjfhpzYGKhswwa=bA5b=+g@mail.gmail.com>

On Wed, Feb 24, 2016 at 4:27 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 14 quoted lines

>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> @@ -255,6 +255,61 @@ static int module_clone(int argc, const char **argv, const char *prefix)
>> +/* Rules to sanitize configuration variables that are Ok to be passed into
>> + * submodule operations from the parent project using "-c". Should only
>> + * include keys which are both (a) safe and (b) necessary for proper
>> + * operation. Right now only "credential.*" fits both criteria.
>> + */
>
> Drop the final sentence for a couple reasons:
>
> 1. It's merely repeating what the code itself already says, and...
> 2. It's likely to become outdated when additional variables are added.
>

Yep. I'll drop that, makes sense.

Show 7 quoted lines

> Also, style:
>
>     /*
>      * Multi-line comment
>      * style.
>      */
>

Hah. I contribute to the netdev kernel a lot, and for some reason they prefer the style above (but only in net/ code). I get into a habbit of doing that too much I'll fix this.

Show 6 quoted lines

>> +               /* combined all the values before we quote them */
>
> Comment repeats what the code already says, thus not terribly useful.
>
> Also: s/combined/combine/
>

I tend to make comments like this when I change the obvious way it was done, I commented this because I based it on a scratch patch from Jeff that didn't have them put together before quoting.

Show 10 quoted lines

>> +               strbuf_addstr(&quoted, var);
>> +               strbuf_addch(&quoted, '=');
>> +               strbuf_addstr(&quoted, value);
>
> strbuf_addf(&quoted, "%s=%s", var, value);
>
>> +               /* safely quote them for shell use */
>
> Comment repeats what the code already says.
>

Will drop this as well.

Show 5 quoted lines

>
> Perhaps not a big deal since the program exits immediately after, but you could:
>
>     strbuf_release(&sanitized_config);
>

Yep, just an oversight, same for the quoted string as well.

Show 5 quoted lines

>> +       local sanitized_config = $(git submodule--helper sanitize-config)
>
> Is 'local' a bashism? (Although, I see that 'local' is already being
> used in relative_path(); perhaps that ought to be cleaned up.)
>

Apparently it is, I wasn't aware of that. It's supported on more than bash, but it's not POSIX.

Show 7 quoted lines

>> +
>> +test_description='Basic plumbing support of submodule--helper
>> +
>> +This test tries to verify the submodule--helper plumbing command used
>
> Maybe: s/tries to verify/verifies/
>

Yes. I used "tries" with the intention of needing tests for the actual functionality as submodule--helper is expected to go away.

Thanks for the review, I'll have these cleaned up in v3

Regards, Jake