| Message ID | abcf1f5cf428072d19639dc4209e0c1554f3eb53.1705659748.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 35122daebc8030ccc42f54fe4f08272091e4aa94 |
| Headers | show |
| Series | refs: convert special refs to become normal pseudo-refs | expand |
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"??? > 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. Thanks.
On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote: > 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" :) > > 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
On 22/01/2024 10:51, Patrick Steinhardt wrote: > On Fri, Jan 19, 2024 at 12:09:25PM -0800, Junio C Hamano wrote: >> Patrick Steinhardt <ps@pks.im> writes: >> 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. Exactly "rebase-merge/autostash" is a detail of the rebase implementation, it is not a ref that users should be using. > So overall I think it's fine to leave this internal sequencer state as > self-contained as it is. That's my feeling too Best Wishes Phillip
Phillip Wood <phillip.wood123@gmail.com> writes: >> ... 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. > > Exactly "rebase-merge/autostash" is a detail of the rebase > implementation, it is not a ref that users should be using. > >> So overall I think it's fine to leave this internal sequencer state as >> self-contained as it is. > > That's my feeling too Good. As long as "git rev-parse rebase-merge/autostash" fails, and regardless of the ref backend in use, we always do "read one line from the on-disk file and run get_oid_hex() on it", I would be happy with that direction. Thanks. [Footnote] * We seem to overuse strbuf_read_file() even when we expect that the file is a single-liner. We have read_line_from_git_path() in wt-status.c that only reads one line and we may want to split it into two: one that takes a filename and calls the other with git_path("%s", filename), and the other that takes a path for any on-disk file, reads a single line, and returns the string that was read, or something like that. We might later want to update the "other" function so that it errors out if there are extra trailing lines (i.e. we expect it is a single-liner, but that expectation is violated---now what?).
diff --git a/sequencer.c b/sequencer.c index 47c8d17cbb..91de546b32 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4463,12 +4463,17 @@ static enum todo_command peek_command(struct todo_list *todo_list, int offset) return -1; } -void create_autostash(struct repository *r, const char *path) +static void create_autostash_internal(struct repository *r, + const char *path, + const char *refname) { struct strbuf buf = STRBUF_INIT; struct lock_file lock_file = LOCK_INIT; int fd; + if (path && refname) + BUG("can only pass path or refname"); + fd = repo_hold_locked_index(r, &lock_file, 0); refresh_index(r->index, REFRESH_QUIET, NULL, NULL, NULL); if (0 <= fd) @@ -4495,10 +4500,16 @@ void create_autostash(struct repository *r, const char *path) strbuf_reset(&buf); strbuf_add_unique_abbrev(&buf, &oid, DEFAULT_ABBREV); - if (safe_create_leading_directories_const(path)) - die(_("Could not create directory for '%s'"), - path); - write_file(path, "%s", oid_to_hex(&oid)); + if (path) { + if (safe_create_leading_directories_const(path)) + die(_("Could not create directory for '%s'"), + path); + write_file(path, "%s", oid_to_hex(&oid)); + } else { + refs_update_ref(get_main_ref_store(r), "", refname, + &oid, null_oid(), 0, UPDATE_REFS_DIE_ON_ERR); + } + printf(_("Created autostash: %s\n"), buf.buf); if (reset_head(r, &ropts) < 0) die(_("could not reset --hard")); @@ -4509,6 +4520,16 @@ void create_autostash(struct repository *r, const char *path) strbuf_release(&buf); } +void create_autostash(struct repository *r, const char *path) +{ + create_autostash_internal(r, path, NULL); +} + +void create_autostash_ref(struct repository *r, const char *refname) +{ + create_autostash_internal(r, NULL, refname); +} + static int apply_save_autostash_oid(const char *stash_oid, int attempt_apply) { struct child_process child = CHILD_PROCESS_INIT; @@ -4586,6 +4607,41 @@ int apply_autostash_oid(const char *stash_oid) return apply_save_autostash_oid(stash_oid, 1); } +static int apply_save_autostash_ref(struct repository *r, const char *refname, + int attempt_apply) +{ + struct object_id stash_oid; + char stash_oid_hex[GIT_MAX_HEXSZ + 1]; + int flag, ret; + + if (!refs_ref_exists(get_main_ref_store(r), refname)) + return 0; + + if (!refs_resolve_ref_unsafe(get_main_ref_store(r), refname, + RESOLVE_REF_READING, &stash_oid, &flag)) + return -1; + if (flag & REF_ISSYMREF) + return error(_("autostash reference is a symref")); + + oid_to_hex_r(stash_oid_hex, &stash_oid); + ret = apply_save_autostash_oid(stash_oid_hex, attempt_apply); + + refs_delete_ref(get_main_ref_store(r), "", refname, + &stash_oid, REF_NO_DEREF); + + return ret; +} + +int save_autostash_ref(struct repository *r, const char *refname) +{ + return apply_save_autostash_ref(r, refname, 0); +} + +int apply_autostash_ref(struct repository *r, const char *refname) +{ + return apply_save_autostash_ref(r, refname, 1); +} + static int checkout_onto(struct repository *r, struct replay_opts *opts, const char *onto_name, const struct object_id *onto, const struct object_id *orig_head) diff --git a/sequencer.h b/sequencer.h index 913a0f652d..dcef7bb99c 100644 --- a/sequencer.h +++ b/sequencer.h @@ -225,9 +225,12 @@ void commit_post_rewrite(struct repository *r, const struct object_id *new_head); void create_autostash(struct repository *r, const char *path); +void create_autostash_ref(struct repository *r, const char *refname); int save_autostash(const char *path); +int save_autostash_ref(struct repository *r, const char *refname); int apply_autostash(const char *path); int apply_autostash_oid(const char *stash_oid); +int apply_autostash_ref(struct repository *r, const char *refname); #define SUMMARY_INITIAL_COMMIT (1 << 0) #define SUMMARY_SHOW_AUTHOR_DATE (1 << 1)
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- 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(-)