Re: [RFC/PATCH] add-patch: handle splitting hunks with diff.suppressBlankEmpty

From: Jeff King <peff@peff.net>
Date: Jul 11, 2024, 21:32 UTC
Message-ID: <20240711213208.GB3648684@coredump.intra.peff.net>
In-Reply-To: <xmqqed81xjl7.fsf@gitster.g>

On Wed, Jul 10, 2024 at 10:06:28AM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> > I don't think we'd ever generate this ourselves, but could somebody
> > manually edit a hunk into this shape? When I tried it in practice, it
> > looks like we fail to apply the result even before my patch, though. I'm
> > not sure why that is. If I put "some garbage" without the blank line, we
> > correctly realize it should be discarded. It's possible I'm just holding
> > it wrong.
> 
> I've given up on the "hunk edit" doing wrong things to a patch
> already.  
> 
> The "edit" codepath used to be a lot less careless before Phillip
> whipped it into a much better shape with the series that ended at
> 436d18f2 (Merge branch 'pw/add-p-recount', 2018-03-14), that
> introduced recount_edited_hunk() that special cases "+/-/ ".  It
> already knows that a completely empty line in a patch is an empty
> and unmodified line (which was ported from f4d35a6b (add -p: fix
> counting empty context lines in edited patches, 2018-06-11)), so
> this patch does not have to do anything new there.

Yeah, I was being paranoid without actually thinking through the implications. As Phillip noted, we do not even run the code I changed on the edited hunk (which already does handle the empty line). So not only could I not break it, but it is already doing the right thing thanks to that earlier work.

> But "recounting" will be fooled by garbage left in the edited
> result, so I think we have done all we can do to resolve possible
> ambiguities.  The patch under discussion is not adding anything new
> and making it any worse, I would say.

Yep, agreed. So modulo a slight inaccuracy in the commit message, I think my patch is OK. That said, I like what Phillip showed in his reply. If he wants to wrap that up into a patch, I think it could replace mine.

-Peff