From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 12 Jan 2026 15:37:00 GMT
Subject: Re: [PATCH v10 1/8] builtin/replay: extract core logic to replay revisions
Message-ID: <aWUVHKMfG0vRb8_G@pks.im>
In-Reply-To: <xmqqv7h6na0s.fsf@gitster.g>

On Mon, Jan 12, 2026 at 07:08:51AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > -	die_for_incompatible_opt2(!!onto_name, "--onto",
> > -				  !!*advance_name, "--advance");
> > +	if (!(!!onto_name ^ !!*advance_name))
> > +		BUG("expected either onto_name or *advance_name in this function");
> 
> This brings our crypticness to a whole new level.  onto_name not
> being NULL is a sign that "--onto" was given, while *advance_name
> pointer points at the string that "--advance" option has received.
> We are saying that only one of these two must be non-NULL, and the
> other must be NULL.
> 
> I know !!VAR is an idiom to turn any pointer into 0 (=NULL) or 1
> (!=NULL), but isn't the latter (i.e., normalizing all non-NULL
> pointer to 1) a bit overkill, which becomes only necessary because
> the construction wants to use "^" as "sides of this operator are
> different Boolean values" operator.  And then to add on top, the
> whole thing is !(negated).  I wonder if
> 
> 	if (!onto_name != !*advance_name)
> 		BUG("one and only one of --onto/--advance must be given");
> 
> is easier to follow without being overly cute?

It should be:

    if (!onto_name == !*advance_name)
        BUG(...);

But other than that it reads way better indeed. Fixed up locally,
thanks!

Patrick