Re: [PATCH 4/7] sequencer: introduce functions to handle autostashes via refs

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 22, 2024, 10:51 UTC
Message-ID: <Za5IoEjs0q7cf354@tanuki>
In-Reply-To: <xmqqbk9hjdai.fsf@gitster.g>

On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote:

Show 14 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > We're about to convert the MERGE_AUTOSTASH ref to become non-special,
> > using the refs API instead of direct filesystem access to both read and
> > write the ref. The current interfaces to write autostashes is entirely
> > path-based though, so we need to extend them to also support writes via
> > the refs API instead.
> >
> > Ideally, we would be able to fully replace the old set of path-based
> > interfaces. But the sequencer will continue to write state into
> > "rebase-merge/autostash". This path is not considered to be a ref at all
> > and will thus stay is-is for now, which requires us to keep both path-
> 
> "is-is"???

Oops, "as-is" :)

Show 11 quoted lines

> > and refs-based interfaces to handle autostashes.
> >
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  sequencer.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++----
> >  sequencer.h |  3 +++
> >  2 files changed, 64 insertions(+), 5 deletions(-)
> 
> The conversion (rather, the introduction to allow refs API to be
> used to access them) look correct, but offhand I do not know what
> the implication of leaving the file based interface would be.

We have said in past discussions that the sequencer state should remain self contained, and that requires us to keep the files-based interface. Refactoring it would likely be a larger undertaking, as we have also said that refs must either have pseudo-ref syntax or start with "refs/".

The sequencer with its "rebase-merge/autostash" files doesn't conform to either of those requirements, so we would also have to rename those reflike files. I doubt that this is really worth it, so I would keep those around as internal implementation details of how the sequencer works. These refs are not supposed to be accessed by the user in the first place, and we do not document their existence to the best of my knowledge. Also, `git rev-parse rebase-merge/autostash` would fail.

So overall I think it's fine to leave this internal sequencer state as self-contained as it is.

Patrick