git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Patrick Steinhardt <ps@pks.im>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Ben Stav <benstav@miggo.io>
Subject: Re: [PATCH] attr: avoid recursion when expanding attribute macros
Date: Wed, 12 Nov 2025 11:21:08 +0100	[thread overview]
Message-ID: <aRRflKWKpUtfn9tw@pks.im> (raw)
In-Reply-To: <20251112071651.GB431661@coredump.intra.peff.net>

On Wed, Nov 12, 2025 at 02:16:51AM -0500, Jeff King wrote:
> On Wed, Nov 12, 2025 at 07:57:14AM +0100, Patrick Steinhardt wrote:
> 
> > So personally I would've probably leaned into the direction of enforcing
> > a hard limit. I don't see a reason why anybody would need more than a
> > couple of recursions, it culls both compute and memory growth, and it
> > allows us to have a proper error message in case the limit is busted.
> > Furthermore, we can demonstrate right now that it wasn't possible to
> > have unlimited recursion anyway, which makes it easier to put a new
> > limit into place.
> > 
> > But following my above reasoning I think it's okay to turn this into
> > iteration, as well, though, but I'd like to hear whether my train of
> > thought matches yours.
> 
> Yeah, it does match mine. If I wanted to waste a bunch of CPU and memory
> on a hosting site, there are a lot easier ways to do that than with
> really long gitattributes.
> 
> I'm not at all opposed to putting in a hard limit on top. My general
> feeling is that it never hurts to convert recursion to iteration; it
> only gives us more options. I'm not planning to work on a hard limit
> myself, but if you want to, be my guest. :)
> 
> I think if we do (or even if we don't), it may also be reasonable to
> shrink the max attribute file size to 10MB or even smaller.

I think for now it's okay to convert this into iteration and not
introduce a limit, at least as long as we keep an open mind about
introducing such a limit in the future. I don't really expect that
anyone will ever abuse this, but if I'm wrong and this happens at one
point in time we may have to introduce the limit retroactively.

So: I'm happy with your patch, but it might make sense to summarize the
discussion in the commit message.

Thanks!

Patrick

  reply	other threads:[~2025-11-12 10:21 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-11-11 22:36 [PATCH] attr: avoid recursion when expanding attribute macros Jeff King
2025-11-12  1:30 ` Ben Knoble
2025-11-12  7:09   ` Jeff King
2025-11-12  7:17     ` Jeff King
2025-11-12  6:57 ` Patrick Steinhardt
2025-11-12  7:16   ` Jeff King
2025-11-12 10:21     ` Patrick Steinhardt [this message]
2025-11-12 17:40   ` Junio C Hamano

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

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=aRRflKWKpUtfn9tw@pks.im \
    --to=ps@pks.im \
    --cc=benstav@miggo.io \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /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.
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 NNTP newsgroup(s).