On Sat, Oct 20 2018, Jeff King wrote:
> On Sat, Oct 20, 2018 at 01:14:28PM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> > I'd guess this sort of thing is pretty rare. But I wonder if we're
>> > crossing the line of trying to assume too much about what the user's
>> > arbitrary code does.
>> >
>> > A simple depth counter can limit the fork bomb, and with a high enough
>> > depth would be unlikely to trigger a false positive. It could also
>> > protect non-aliases more reasonably, too (e.g., if you have a 1000-deep
>> > git process hierarchy, there's a good chance you've found an infinite
>> > loop in git itself).
>>
>> I don't think this edge case you're describing is very plausible, and I
>> doubt it exists in the wild.
>>
>> But going by my personal incredulity and a git release breaking code in
>> the wild would suck, so agree that I need to re-roll this to anticipate
>> that.
>
> I agree it's probably quite rare, if it exists at all. But I also wonder
> how important looping alias protection is. It's also rare, and the
> outcome is usually "gee, I wonder why this is taking so long? ^C".
>
> At least that's my instinct. I don't remember having run into this at
> all myself (though certainly I have written my fair share of infinite
> loops in other systems, like bash aliases, and that is what happened).
>
>> I don't have time to write it now, but what do you think about a version
>> of this where we introduce a core.recursionLimit setting, and by default
>> set it to "1" (for one recursion), so by default die just as we do now,
>> but with some advice() saying that we've bailed out early because this
>> looks crazy, but you can set it to e.g. "1000" if you think you know
>> what you're doing, or "0" for no limit.
>>
>> The reason I'd like to do that is because I think it's *way* more common
>> to do this accidentally than intentionally, and by having a default
>> limit of 1000 we'd print a really long error message, or alternatively
>> would have to get into the mess of de-duplicating the callstack as we
>> print the error.
>
> Would we print a long error message? I'd assume that we'd just recurse
> for longer and print one error message that says:
>
> fatal: woah, you're 1000-levels deep in Git commands!
>
> That doesn't help the user find the recursion, but re-running with
> GIT_TRACE=1 would make it pretty clear, I'd think.
Yeah the reason I'd like the core.recursionLimit=1 setting by default is
so that we can also print the same pretty and easy to grok error message
we do now for non-! aliases by default without spewing out ~3-4k lines
of mostly duplicate output (with a default limit of 1000).
We didn't support chained aliases at all before, so I think the odds
that people will run into this now will increase as they add "!" to
existing aliases, and I'd like to have git's UI friendly enough to tell
users what went wrong by default, and not have to resort to the likes of
GIT_TRACE=1 which really should be left to powerusers.
>> It also has the advantage that if people in the wild really use this
>> they'll chime in about this new annoying core.recursionLimit=1 setting,
>> at the cost of me having annoyed them all by breaking their working
>> code.
>
> Right, I'm not too happy about that annoyance. But it seems clear that I
> think the loop protection is way less important than you do, so I'm
> willing to sacrifice (or more accurately, risk the possibility of
> sacrificing) a lot less for it. :)
>
> I dunno. I doubt it is likely to help or hinder that many people either
> way.
>
>> >> + cmd_history = strbuf_split_buf(old, strlen(old), ' ', 0);
>> >> + for (ptr = cmd_history; *ptr; ptr++) {
>> >> + strbuf_rtrim(*ptr);
>> >> + string_list_append(cmd_list, (*ptr)->buf);
>> >> + }
>> >> + strbuf_list_free(cmd_history);
>> >
>> > Maybe string_list_split() would be a little simpler?
>>
>> Yeah looks like it. I cargo-culted this from elsewhere without looking
>> at that API. I'll look into it.
>
> I cheated before writing that and confirmed that it does seem to work. ;)
>
> Here's the patch in case it is useful. IMHO we should be trying to get
> rid of strbuf_split, because it's a pretty crappy interface.
>
> diff --git a/git.c b/git.c
> index cba242836c..9d1b66a1fa 100644
> --- a/git.c
> +++ b/git.c
> @@ -675,7 +675,6 @@ static void execv_dashed_external(const char **argv)
> static void init_cmd_history(struct strbuf *env, struct string_list *cmd_list)
> {
> const char *old = getenv(COMMAND_HISTORY_ENVIRONMENT);
> - struct strbuf **cmd_history, **ptr;
>
> if (!old || !*old)
> return;
> @@ -683,12 +682,7 @@ static void init_cmd_history(struct strbuf *env, struct string_list *cmd_list)
> strbuf_addstr(env, old);
> strbuf_rtrim(env);
>
> - cmd_history = strbuf_split_buf(old, strlen(old), ' ', 0);
> - for (ptr = cmd_history; *ptr; ptr++) {
> - strbuf_rtrim(*ptr);
> - string_list_append(cmd_list, (*ptr)->buf);
> - }
> - strbuf_list_free(cmd_history);
> + string_list_split(cmd_list, env->buf, ' ', -1);
> }
>
> static void add_cmd_history(struct strbuf *env, struct string_list *cmd_list,
Thanks! Will squash it.
