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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Oct 26 2018, Jeff King wrote:

> On Sat, Oct 20, 2018 at 02:58:53PM -0400, 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".
>
> Hmph. So I was speaking before purely hypothetically, but now that your
> patch is in 'next', it is part of my daily build. And indeed, I hit a
> false positive within 5 minutes of building it. ;)
>
> I have an alias like this:
>
>   $ git help dotgit
>   'dotgit' is aliased to '!git rev-parse 2>/dev/null || cd ~/compile/git; git'
>
> The idea being that I can run "git dotgit foo" to run "git foo" in the
> current directory, or if it is not a git repository, in my checkout of
> git.git.
>
> I use it in two ways:
>
>   - some of my aliases know about it themselves. So I have an alias "ll"
>     that does:
>
>       $ git help ll
>       'll' is aliased to '!git dotgit --no-pager log --no-walk=unsorted --format='%h (%s, %ad)' --date=short'
>
>     with the idea being to produce a nice annotation for a commit id.
>     Using "git dotgit" there lets me just run it from any directory,
>     since 99% of the time I am working on git.git anyway.
>
>   - I have a vim command defined:
>
>       command! -nargs=* Git :call MaybeInlineCommand("git dotgit <args>")
>
>     so I can do ":Git foo" inside vim and it uses either the current
>     repo (e.g., if I'm writing a commit message) or git.git (e.g., if
>     I'm writing an email and didn't start in the repo).
>
> So of course the alias expansion is something like (in older versions of
> Git):
>
>   1. "git dotgit ll" runs the dotgit alias, which sees that we need to go
>      to the git.git checkout
>
>   2. that runs "git ll"
>
>   3. that runs "git dotgit log"; this second dotgit invocation sees we're
>      already in a repository and is a noop
>
>   4. git-log runs
>
> With your patch, step 3 complains:
>
>   $ git dotgit ll
>   fatal: alias loop detected: expansion of 'dotgit' does not terminate:
>   dotgit <==
>   ll ==>
>
> So I would really prefer a depth counter that can be set sufficiently
> high to make this case work. ;)
>
>
> As an aside, I got to experience this error message as an unsuspecting
> user would. Unfortunately the output was not super helpful for figuring
> out the cause. I scratched my head for a while before remembering that
> "ll" uses "dotgit" explicitly (which was quite apparent when running
> GIT_TRACE=1, or "git help ll"). I think showing the alias definitions in
> the loop output would have made it much more obvious (if perhaps a bit
> uglier).  E.g., something like:
>
>   fatal: alias loop...
>   ==> dotgit is aliased to '!git rev-parse ...'
>   <== ll is aliased to '!git dotgit ...'
>
> -Peff

Yikes.

Junio: After your previous "What's cooking" in
<xmqq8t2u1nkh.fsf@gitster-ct.c.googlers.com> I sent
<87ftx0dg4r.fsf@evledraar.gmail.com>, but should have just replied to
"What's cooking".

I.e. I think this topic should just be ejected, I'll try to submit a
re-roll, but don't know if I have time in the next few days.

Can you please queue a "git revert" of it (or rewind next, but not sure
if you want to do that...).