Re: [PATCH] alias: detect loops in mixed execution mode

> 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.

>> 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,