From: Elijah Newren <newren@gmail.com>
Date: Fri, 28 Aug 2026 07:38:48 GMT
Subject: Re: [PATCH v2 1/3] commit: reword the empty-commit rebase errors
Message-ID: <CABPp-BEtoN+zA=vyyEAgruNSy5SKWjTdVW=weDjbM8NcenRbGg@mail.gmail.com>
In-Reply-To: <xmqq1pbjbj4x.fsf@gitster.g>

On Thu, Aug 27, 2026 at 9:52 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> +                    die(_("cannot do a partial commit while resolving a commit that became empty."));
> >
> > That is a mouthful.  It also is awkward to say "while resolving a commit".
>
> This still stands, but I haven't come up with a better alternative yet.
>
> > More importantly, I am not sure if whence == FROM_REBASE_PICK at
> > this point in the code flow is a sufficient sign to tell that we
> > were not just in the middle of a rebase, not just a rebase stopped
> > with _some_ conflict, but the way the rebase stopped was because a
> > step in rebase resulted in a commit that is no-op relative to the
> > previous commit.  What makes us certain that the rebase-pick is
> > empty?
>
> This confusion was because FROM_REBASE_PICK is a misleading name.
>
> sequencer_determine_whence() is the only place that declares the
> whence is FROM_REBASE_PICK, and it specifically checks if the
> rebase-head and cherry-pick-head are identical before yielding that
> value, so by definition we are dealing with an empty-pick situation.
>
> This came from 430b75f720 (commit: give correct advice for empty
> commit during a rebase, 2019-12-06); interestingly, the name of
> FROM_REBASE_PICK and is_from_rebase() seem to have confused even the
> originating commit ;-)  The lines in question
>
> +               else if (is_from_rebase(whence))
> +                       die(_("cannot do a partial commit during a rebase."));
>
> are from that commit, which wanted to "give correct advice for empty
> commit during a rebase".
>
> We may want to
>
>  * change the code that does whence == FROM_REBASE_PICK to use
>    is_from_rebase(whence) everywhere (other than the implementation
>    of is_from_rebase() itself, of course).
>
>  * give FROM_REBASE_PICK and is_from_rebase() better names that
>    contain "empty" somewhere.
>
> to unconfuse me.

That really confused me too.  I figured my series was already growing
too quickly and decided to leave it out, but since it confused you as
well, I agree we should fix this up.  I'll add a preparatory patch in
v3.