On Wed, Feb 24, 2016 at 4:27 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >> --- >> 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. > 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. >> + /* 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. >> + strbuf_addstr("ed, var); >> + strbuf_addch("ed, '='); >> + strbuf_addstr("ed, value); > > strbuf_addf("ed, "%s=%s", var, value); > >> + /* safely quote them for shell use */ > > Comment repeats what the code already says. > Will drop this as well. > > 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. >> + 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. >> + >> +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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html