From: Jeff King <peff@peff.net>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
Tim Schumacher <timschumi@gmx.de>, Duy Nguyen <pclouds@gmail.com>
Subject: Re: [PATCH] alias: detect loops in mixed execution mode
Date: Fri, 26 Oct 2018 04:39:05 -0400 [thread overview]
Message-ID: <20181026083905.GA1705@sigill.intra.peff.net> (raw)
In-Reply-To: <20181020185852.GA6234@sigill.intra.peff.net>
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
next prev parent reply other threads:[~2018-10-26 8:39 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-09-05 8:54 [RFC PATCH v2] Allow aliases that include other aliases Tim Schumacher
2018-09-05 15:48 ` Duy Nguyen
2018-09-05 19:02 ` Tim Schumacher
2018-09-05 17:12 ` Junio C Hamano
2018-09-05 19:12 ` Tim Schumacher
2018-09-05 17:34 ` Jeff King
2018-09-05 20:02 ` Tim Schumacher
2018-09-06 13:38 ` Ævar Arnfjörð Bjarmason
2018-09-06 14:17 ` Ævar Arnfjörð Bjarmason
2018-10-18 22:57 ` [PATCH] alias: detect loops in mixed execution mode Ævar Arnfjörð Bjarmason
2018-10-19 8:28 ` Ævar Arnfjörð Bjarmason
2018-10-19 22:09 ` Jeff King
2018-10-20 10:52 ` Ævar Arnfjörð Bjarmason
2018-10-19 22:07 ` Jeff King
2018-10-20 11:14 ` Ævar Arnfjörð Bjarmason
2018-10-20 18:58 ` Jeff King
2018-10-20 19:18 ` Ævar Arnfjörð Bjarmason
2018-10-22 21:15 ` Jeff King
2018-10-22 21:28 ` Ævar Arnfjörð Bjarmason
2018-10-22 1:23 ` Junio C Hamano
2018-10-26 8:39 ` Jeff King [this message]
2018-10-26 12:44 ` Ævar Arnfjörð Bjarmason
2018-10-29 3:44 ` Junio C Hamano
2018-10-29 14:17 ` Jeff King
2018-09-05 21:51 ` [RFC PATCH v2] Allow aliases that include other aliases Junio C Hamano
2018-09-06 10:16 ` [PATCH v3] " Tim Schumacher
2018-09-06 14:01 ` Ævar Arnfjörð Bjarmason
2018-09-06 14:57 ` Jeff King
2018-09-06 15:10 ` Ævar Arnfjörð Bjarmason
2018-09-06 16:18 ` Jeff King
2018-09-06 19:05 ` Tim Schumacher
2018-09-06 19:17 ` Jeff King
2018-09-06 14:59 ` Jeff King
2018-09-06 18:40 ` Junio C Hamano
2018-09-06 19:05 ` Jeff King
2018-09-06 19:31 ` Tim Schumacher
2018-09-07 22:44 ` [RFC PATCH v4 1/3] Add support for nested aliases Tim Schumacher
2018-09-07 22:44 ` [RFC PATCH v4 2/3] Show the call history when an alias is looping Tim Schumacher
2018-09-08 13:34 ` Duy Nguyen
2018-09-08 16:29 ` Jeff King
2018-09-07 22:44 ` [RFC PATCH v4 3/3] t0014: Introduce alias testing suite Tim Schumacher
2018-09-07 23:38 ` Eric Sunshine
2018-09-14 23:12 ` Tim Schumacher
2018-09-16 7:21 ` Eric Sunshine
2018-09-08 13:28 ` [RFC PATCH v4 1/3] Add support for nested aliases Duy Nguyen
2018-09-16 7:46 ` Tim Schumacher
2018-09-17 15:37 ` Junio C Hamano
2018-09-21 12:45 ` Tim Schumacher
2018-09-21 15:59 ` Junio C Hamano
2018-09-16 7:50 ` [PATCH v5 " Tim Schumacher
2018-09-16 7:50 ` [PATCH v5 2/3] Show the call history when an alias is looping Tim Schumacher
2018-09-16 7:50 ` [PATCH v5 3/3] t0014: Introduce an alias testing suite Tim Schumacher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181026083905.GA1705@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=timschumi@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).