All of lore.kernel.org
 help / color / mirror / Atom feed
From: "brian m. carlson" <sandals@crustytoothpaste.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jared Van Bortel <cebtenzzre@gmail.com>
Subject: Re: [PATCH] diff: don't crash with empty argument to -G or -S
Date: Tue, 18 Feb 2025 19:29:03 +0000	[thread overview]
Message-ID: <Z7TffynGALJM4KfH@tapette.crustytoothpaste.net> (raw)
In-Reply-To: <xmqqbjuzxgn3.fsf@gitster.g>

[-- Attachment #1: Type: text/plain, Size: 2083 bytes --]

On 2025-02-18 at 18:16:32, Junio C Hamano wrote:
> I agree BUG is unwelcome.  I am not sure about the value of
> forbidding an empty string (I am sure about forbidding NULL,
> though).  
> 
> If an empty matches everything, "git log -S" would skip changes that
> would keep the number of lines, right?  For the history of a project
> that keeps track of source code, such a "feature" would not be
> useful, but I can see a complaint by somebody who may want to keep
> track of a "list of things" one-item-per-line, if we had been
> allowing an empty string.  It would be a regression for such a niche
> user.

I actually just ran a `git grep -e ''` to see what it does, and it
does indeed match every line, so presumably `git log -G` would do so as
well.

I do see your argument that this could be useful for a limited number of
use cases, but as someone who often keeps track of lists of things in
text files and therefore could be a target for that feature, I still
feel like this would be very much a corner case.

> Luckily, since we have stopped with a "BUG", we do not have to worry
> about backward compatibility in this case ;-)

I agree.  The good news is that we haven't broken anyone's workflow,
unless their workflow involves trying to trigger bugs.

> So I'd say that it may be a bit premature for us to declare
> "anything useful", I am perfectly fine with the patch given here.
> If somebody who wants to maintain a text file, one-item-per-line
> that keeps track of a list of things to omit commits that do not
> change the number of items, they can drop "&& !*arg" part, tweak the
> message and add their own tests, once this fix lands and the dust
> settles.

Exactly.  If there's one thing I've learned, it's that there are lots of
users who will try new things, and I'm sure we'll get a report here or
elsewhere that they'd like to add this feature if there's actually
interest.  Fortunately, I expect that it shouldn't be too hard to add
such a feature.
-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

      reply	other threads:[~2025-02-18 19:29 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-02-17  1:24 Crash on empty pickaxe argument Jared Van Bortel
2025-02-17  1:58 ` brian m. carlson
2025-02-17 17:57   ` [PATCH] diff: don't crash with empty argument to -G or -S brian m. carlson
2025-02-17 22:18     ` Elijah Newren
2025-02-18 18:16     ` Junio C Hamano
2025-02-18 19:29       ` brian m. carlson [this message]

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=Z7TffynGALJM4KfH@tapette.crustytoothpaste.net \
    --to=sandals@crustytoothpaste.net \
    --cc=cebtenzzre@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.