Skip to content

stash: add 'reword' subcommand - #2180

Open
ozemin wants to merge 1 commit into
gitgitgadget:masterfrom
ozemin:stash-rename
Open

stash: add 'reword' subcommand#2180
ozemin wants to merge 1 commit into
gitgitgadget:masterfrom
ozemin:stash-rename

Conversation

@ozemin

@ozemin ozemin commented Jul 15, 2026

Copy link
Copy Markdown

eo/stash-reword

"git stash reword" learned to change the message of an existing stash entry
without changing its position, its contents or its reflog timestamp.

This came up in 2010 and again in 2013, and was rejected back then on the
grounds that reflogs are append-only recovery logs. refs/stash is the
exception Michael Haggerty pointed out in that thread: its reflog is the
primary data store for stash entries, and "git stash drop" rewrites it
already. So this rewrites the refs/stash reflog through
ref_transaction_update_reflog(), which "git remote rename" and "git refs
migrate" already use to rewrite existing reflogs, and touches nothing else.
Details and links to the old threads are in the commit message.

Costs, so nobody has to dig for them: the whole reflog is read and written
back once whatever the target's depth, so the cost is linear in its length.
The rewrite is not crash-safe. The reflog is cleared before the transaction
writes it back, so a process killed in that window leaves refs/stash without
the reflog that "git stash list" reads. If the transaction itself fails, the
collected entries are re-stored best-effort, and whatever cannot be written
back is reported with its object id so "git stash store" can recover it. The
stash commits are never deleted either way, so "git fsck" still finds them.
Closing that window for real needs a refs API operation that replaces a
reflog in one step, which I would rather do as a follow-up.

I picked a positional message argument over an -m option ("stash store"
style); no strong opinion, happy to switch.

Changes since v1, all from Patrick's review: the subcommand is 'reword'
rather than 'rename', the reflog is rewritten in a single transaction
instead of one drop-and-store per entry, and the commit message opens with
the use case rather than with the observation that stash messages cannot be
changed.

t3903 passes with GIT_TEST_DEFAULT_REF_FORMAT=files and reftable.

@gitgitgadget

gitgitgadget Bot commented Jul 15, 2026

Copy link
Copy Markdown

Welcome to GitGitGadget

Hi @ozemin, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that either:

  • Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
  • Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.

You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <revi.ewer@example.com>, Ill Takalook <ill.takalook@example.net>

NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description,
because it will result in a malformed CC list on the mailing list. See
example.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@ozemin
ozemin force-pushed the stash-rename branch 2 times, most recently from 4105491 to 86af7ff Compare July 15, 2026 15:30
@mmontalbo

Copy link
Copy Markdown

/allow

@gitgitgadget

gitgitgadget Bot commented Jul 15, 2026

Copy link
Copy Markdown

User ozemin is now allowed to use GitGitGadget.

WARNING: ozemin has no public email address set on GitHub; GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.

@ozemin

ozemin commented Jul 15, 2026

Copy link
Copy Markdown
Author

/preview

@gitgitgadget

gitgitgadget Bot commented Jul 15, 2026

Copy link
Copy Markdown

Preview email sent as pull.2180.git.1784142999053.gitgitgadget@gmail.com

@ozemin

ozemin commented Jul 16, 2026

Copy link
Copy Markdown
Author

/preview

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

Preview email sent as pull.2180.git.1784190582932.gitgitgadget@gmail.com

@ozemin

ozemin commented Jul 16, 2026

Copy link
Copy Markdown
Author

/submit

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

Submitted as pull.2180.git.1784190706028.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2180/ozemin/stash-rename-v1

To fetch this version to local tag pr-2180/ozemin/stash-rename-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2180/ozemin/stash-rename-v1

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

Patrick Steinhardt wrote on the Git mailing list (how to reply to this email):

On Thu, Jul 16, 2026 at 08:31:45AM +0000, Emin Özata via GitGitGadget wrote:
> From: =?UTF-8?q?Emin=20=C3=96zata?= <eminozata@proton.me>
> 
> There is no way to change the message of a stash entry after the
> fact.  The only option is dropping the entry and re-storing it by
> hand, which moves it to the top of the stash list and gets fiddly
> for deeper entries.
> 
> Add 'git stash rename <message> [<stash>]', defaulting to the
> latest entry like the other subcommands do.  It reads the object id
> and reflog message of the target entry and of the entries above it,
> drops them all like 'git stash drop' would, and stores them back in
> the same order, with the new message going to the target.  Position,
> contents and the reflog chain stay as they were.
> 
> The command checks every entry it is about to rewrite and refuses
> to start if one of them does not look like a stash commit, which
> can only happen when refs/stash was written to by hand.  Finding
> that out halfway through the sequence would lose entries.  Should a
> write-back fail anyway, the entry's object id is reported so it can
> be recovered with 'git stash store', and the command only reports
> success when the reflog ended up in the requested state.
> 
> This was proposed before: in 2010, as a "git reflog update" command
> that edited reflog entries in place [1].  When it came up again in
> 2013 [2], Junio rejected it on the grounds that reflogs are
> append-only recovery logs, and that whoever really cares about a
> stash message can pop and re-stash [3].  Michael Haggerty pointed
> out in that thread that refs/stash does not fit the description:
> its reflog is the primary data store for stash entries, and 'git
> stash drop' rewrites it all the time [4].  So this patch stays away
> from the reflog machinery entirely and does the suggested
> pop-and-re-stash workaround mechanically, without the detour
> through the working tree.

Hm. It's good to refer to to previous discussions. But I think it would
make sense to also document why explicitly _you_ want to have this
functionality. Like, what use case does it enable that you currently
cannot have right now? How is this different to what was proposed back
then that should make us reconsider whether or not to include it now?

> diff --git a/Documentation/git-stash.adoc b/Documentation/git-stash.adoc
> index 50bb89f483..03f2e03096 100644
> --- a/Documentation/git-stash.adoc
> +++ b/Documentation/git-stash.adoc
> @@ -163,6 +164,12 @@ with no conflicts.
>  	created by `export`, and add them to the list of stashes.  To replace the
>  	existing stashes, use `clear` first.
>  
> +`rename [-q | --quiet] <message> [<stash>]`::
> +	Change the message of a single stash entry.  The entry keeps its
> +	position and its contents.  _<stash>_ must name an entry by
> +	index (e.g. `stash@{1}`); renaming refreshes the reflog
> +	timestamps of the entry and of the entries above it.

I think "rename" is a bit of a misleading name, doubly so with the
recently introduced `git refs rename` feature that renames a reference.
I'd suggest "reword" instead.

> diff --git a/builtin/stash.c b/builtin/stash.c
> index c4809f299a..94e66d6074 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1190,6 +1204,166 @@ out:
[snip]
> +static int do_rename_stash(struct stash_info *info, size_t idx,
> +			   const char *msg, int quiet)
> +{
> +	struct rename_data data = { .want = idx + 1 };
> +	size_t i, missing = 0;
> +	int ret = -1;
> +
> +	refs_for_each_reflog_ent_reverse(get_main_ref_store(the_repository),
> +					 ref_stash, collect_rename_entries,
> +					 &data);
> +	if (data.nr <= idx) {
> +		error(_("%s does not exist"), info->revision.buf);
> +		goto cleanup;
> +	}
> +
> +	if (!oideq(&info->w_commit, &data.entries[idx].oid)) {
> +		error(_("%s changed concurrently; try again"),
> +		      info->revision.buf);
> +		goto cleanup;
> +	}
> +
> +	/* refuse up front; do_store_stash() would die halfway through */
> +	for (i = 0; i < data.nr; i++) {
> +		struct commit *stash = lookup_commit_reference(the_repository,
> +							       &data.entries[i].oid);
> +
> +		if (!stash || check_stash_topology(the_repository, stash)) {
> +			error(_("%s does not look like a stash commit"),
> +			      oid_to_hex(&data.entries[i].oid));
> +			goto cleanup;
> +		}
> +	}

This loop here has potentially-quadratic runtime. Not so much with the
"files" backend, where we'll simply append the data to the log. But with
the reftable backend we'll basically end up writing each reflog entry
into a new table, and we'll end up compacting the tables many times
over.

> +
> +	while (missing <= idx) {
> +		if (drop_reflog_entry("stash@{0}"))
> +			goto restore;
> +		missing++;
> +	}

Same here, this will not perform well if you have a huge reflog.

We really should do all of this atomically, where we ideally delete the
old reflog and create the new reflog in a single transaction.

Thanks!

Patrick

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

User Patrick Steinhardt <ps@pks.im> has been added to the cc: list.

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

Patrick Steinhardt <ps@pks.im> writes:

> Hm. It's good to refer to to previous discussions. But I think it would
> make sense to also document why explicitly _you_ want to have this
> functionality. Like, what use case does it enable that you currently
> cannot have right now? How is this different to what was proposed back
> then that should make us reconsider whether or not to include it now?

We have one previous discussion thread on this, and there is no
need to refer to every single message in it.  Just pointing to
the discussion starter should be enough to let us read what was
said.  It was also very nice of Emin to summarize the viewpoints
expressed there, so that readers can follow along without having to
dig up external resources.

Asking for the actual use case is indeed very useful, and I highly
appreciate the nudge.

"I cannot reword the message shown in 'git stash list' without this
change" is not a use case.  It would be far more useful to know
what real-world task you are blocked from accomplishing just
because you cannot reword a misspelled entry in 'git stash list'.

Personally, I cannot think of any such roadblock.  If I find that
a stash entry is worth tweaking right now, I would probably
create a new branch for the task, unstash it there, and either
make further changes or record the exact state with an updated
message to help my future self.  After all, having a branch I
can start working on immediately is far more useful than having
the same stash entry with its spelling corrected.

Thanks.

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

"brian m. carlson" wrote on the Git mailing list (how to reply to this email):

On 2026-07-16 at 17:46:13, Junio C Hamano wrote:
> Asking for the actual use case is indeed very useful, and I highly
> appreciate the nudge.
> 
> "I cannot reword the message shown in 'git stash list' without this
> change" is not a use case.  It would be far more useful to know
> what real-world task you are blocked from accomplishing just
> because you cannot reword a misspelled entry in 'git stash list'.
> 
> Personally, I cannot think of any such roadblock.  If I find that
> a stash entry is worth tweaking right now, I would probably
> create a new branch for the task, unstash it there, and either
> make further changes or record the exact state with an updated
> message to help my future self.  After all, having a branch I
> can start working on immediately is far more useful than having
> the same stash entry with its spelling corrected.

I have a large number of stashes in my git.git repository (211 at
present) and it may be that I, in a mad rush to solve some problem or
another, went with the default message.  I do this kind of thing with
repositories at work all the time.  But it's inconvenient to have to
search through `git stash list -p` to see which change it was and which
stash I need to pop to resume my work, so I can see the utility of this
approach.

In my case, the stash is likely on an existing branch (such as my
sha256-interop branch), so I don't want to create another branch right
now, but I do want to distinguish a couple of different stashes.

Of course, I don't know what Emin's use case is, but I think it would in
fact be useful to have and I can see myself using it. If my use case is
valuable to others, anyone is free to summarize it in the commit message
as a justification.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

@gitgitgadget

gitgitgadget Bot commented Jul 16, 2026

Copy link
Copy Markdown

User "brian m. carlson" <sandals@crustytoothpaste.net> has been added to the cc: list.

@gitgitgadget

gitgitgadget Bot commented Jul 17, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I have a large number of stashes in my git.git repository (211 at
> present) and it may be that I, in a mad rush to solve some problem or
> another, went with the default message.  I do this kind of thing with
> repositories at work all the time.  But it's inconvenient to have to
> search through `git stash list -p` to see which change it was and which
> stash I need to pop to resume my work, so I can see the utility of this
> approach.

Hmph, are you saying that you are going to wade through 211
stash entries one by one and reword them?

> In my case, the stash is likely on an existing branch (such as my
> sha256-interop branch), so I don't want to create another branch right
> now, but I do want to distinguish a couple of different stashes.

This is not necessarily related to what Emin's patch does or tries
to do, but it suggests to me that it would be ultra-nice if we could
have a stash that is associated with each individual branch.  It is
as if we have been living in a world with only detached HEADs and
you suddenly invented named branches.  ;-)

Well, the original design motivation behind "git stash" was the
"boss is here" emergency mode, and the expectation was that such
an emergency would not nest to leave anyone with more than a
handful of entries.  Thus, a single stash shared repository-wide
across your branches was not a huge problem.  But our workflows
have evolved, so our tool support may have to evolve alongside
them.

Thanks for the food for thought.

@gitgitgadget

gitgitgadget Bot commented Jul 26, 2026

Copy link
Copy Markdown

erik88 wrote on the Git mailing list (how to reply to this email):

On 26/07/16 06:11PM, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
> > I have a large number of stashes in my git.git repository (211 at
> > present) and it may be that I, in a mad rush to solve some problem or
> > another, went with the default message.  I do this kind of thing with
> > repositories at work all the time.  But it's inconvenient to have to
> > search through `git stash list -p` to see which change it was and which
> > stash I need to pop to resume my work, so I can see the utility of this
> > approach.

Same. I always accumulate 100+ stashes. I never bother with a message
when I stash. 90-95% of the stashes are useless cruft but there's always
an experiment, a script file of some sort, which turns out to be useful.

> Hmph, are you saying that you are going to wade through 211
> stash entries one by one and reword them?

Not me but my AI agent ;-)

@gitgitgadget

gitgitgadget Bot commented Jul 26, 2026

Copy link
Copy Markdown

User erik88 <erik88@gmail.com> has been added to the cc: list.

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

erik88 <erik88@gmail.com> writes:

> On 26/07/16 06:11PM, Junio C Hamano wrote:
>> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>>
>> > I have a large number of stashes in my git.git repository (211 at
>> > present) and it may be that I, in a mad rush to solve some problem or
>> > another, went with the default message.  I do this kind of thing with
>> > repositories at work all the time.  But it's inconvenient to have to
>> > search through `git stash list -p` to see which change it was and which
>> > stash I need to pop to resume my work, so I can see the utility of this
>> > approach.
>
> Same. I always accumulate 100+ stashes. I never bother with a message
> when I stash. 90-95% of the stashes are useless cruft but there's always
> an experiment, a script file of some sort, which turns out to be useful.

OK, so editing the log message for each stash entry is already a use
case.  In that case, I no longer object to the feature in principle.
At least, I cannot think of a way for it to encourage any bad
workflows.

The next step is for someone to review the implementation.  I have
not read the implementation at all, and it will be quite a while
until I get to it (if I ever do).

>> Hmph, are you saying that you are going to wade through 211
>> stash entries one by one and reword them?
>
> Not me but my AI agent ;-)

I have no sympathy for you, then.  Your AI agent should be able to
unstash and restash 211 entries one by one and reword them just fine
;-).

@ozemin ozemin changed the title stash: add 'rename' subcommand stash: add 'reword' subcommand Jul 27, 2026
@ozemin

ozemin commented Jul 27, 2026

Copy link
Copy Markdown
Author

/preview

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Preview email sent as pull.2180.v2.git.1785143490435.gitgitgadget@gmail.com

@ozemin

ozemin commented Jul 27, 2026

Copy link
Copy Markdown
Author

/preview

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Preview email sent as pull.2180.v2.git.1785143909700.gitgitgadget@gmail.com

Stash entries accumulate with the default "WIP on <branch>" message
when they are created in a hurry, and there is no way to relabel the
ones worth keeping afterwards.  The only option is dropping an entry
and re-storing it by hand, which moves it to the top of the stash
list and gets fiddly for deeper entries.

Add 'git stash reword <message> [<stash>]', which defaults to the
latest entry like the other subcommands do.  The name follows the
verb git already uses for changing only a message, as in the reword
command of 'git rebase -i' and in 'git history reword'.  It reads
the whole reflog and writes it back in a single transaction, with
the new message on the target and every other entry left as it was.
Position, contents, timestamps and the reflog chain all stay put.

Before touching anything, the command inspects the target entry and
the ones above it, and refuses to start if any of them does not look
like a stash commit; that can only happen when refs/stash was
written to by hand.  The rewrite clears the reflog before writing
the new one, so if the transaction fails afterwards, the command
reports the object id of each collected entry and re-stores it
best-effort.  Whatever it fails to re-store can be recovered with
'git stash store'.

This was proposed before: in 2010, as a "git reflog update" command
that edited reflog entries in place [1].  When it came up again in
2013 [2], Junio rejected it on the grounds that reflogs are
append-only recovery logs, and that whoever really cares about a
stash message can pop and re-stash [3].  Michael Haggerty pointed
out in that thread that refs/stash does not fit the description:
its reflog is the primary data store for stash entries, and 'git
stash drop' rewrites it all the time [4].  So this patch rewrites
refs/stash directly, the way 'git stash drop' already does, through
ref_transaction_update_reflog(), which 'git remote rename' and 'git
refs migrate' already use to rewrite existing reflogs.

Name the target by index (stash@{1}); the command rejects
time-based selectors, because it needs the entry's position in the
reflog.  Writing the reflog back at once keeps the cost linear in
its length whatever that position is, and spares the reftable
backend from emitting and compacting a table per entry, which is
what dropping and re-storing the entries one by one would cost.

[1] https://lore.kernel.org/git/20100620093142.GF24805@occam.hewgill.net/
[2] https://lore.kernel.org/git/loom.20130104T192132-16@post.gmane.org/
[3] https://lore.kernel.org/git/7vbod4tynt.fsf@alter.siamese.dyndns.org/
[4] https://lore.kernel.org/git/50ED2C78.1030300@alum.mit.edu/

Signed-off-by: Emin Özata <eminozata@proton.me>
@ozemin

ozemin commented Jul 27, 2026

Copy link
Copy Markdown
Author

/preview

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Preview email sent as pull.2180.v2.git.1785144347131.gitgitgadget@gmail.com

@ozemin

ozemin commented Jul 27, 2026

Copy link
Copy Markdown
Author

/submit

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Submitted as pull.2180.v2.git.1785149687514.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2180/ozemin/stash-rename-v2

To fetch this version to local tag pr-2180/ozemin/stash-rename-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2180/ozemin/stash-rename-v2

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Emin wrote on the Git mailing list (how to reply to this email):

Junio C Hamano <gitster@pobox.com> writes:

> OK, so editing the log message for each stash entry is already a use
> case.  In that case, I no longer object to the feature in principle.

Thanks, and thanks to brian and erik for describing the use case better
than I did in the original message.  Mine is the same: entries pile up
with the default "WIP on <branch>" message, and the ones worth keeping
are the hardest to find again later.

I will send a v2 shortly that addresses Patrick's review.  It renames
the subcommand to 'reword' and rewrites the reflog in a single
transaction.

Thanks.

@gitgitgadget

gitgitgadget Bot commented Jul 27, 2026

Copy link
Copy Markdown

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Emin Özata via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +	refs_for_each_reflog_ent_reverse(refs, ref_stash,
> +					 collect_reword_entries, &data);
> +	if (data.nr <= idx) {
> +		error(_("%s does not exist"), info->revision.buf);
> +		goto cleanup;
> +	}
> +
> +	if (!oideq(&info->w_commit, &data.entries[idx].new_oid)) {
> +		error(_("%s changed concurrently; try again"),
> +		      info->revision.buf);
> +		goto cleanup;
> +	}
> +
> +	for (i = 0; i <= idx; i++) {
> +		struct commit *stash = lookup_commit_reference(the_repository,
> +							       &data.entries[i].new_oid);
> +
> +		if (!stash || check_stash_topology(the_repository, stash)) {
> +			error(_("%s does not look like a stash commit"),
> +			      oid_to_hex(&data.entries[i].new_oid));
> +			goto cleanup;
> +		}
> +	}
> +
> +	if (refs_delete_reflog(refs, ref_stash)) {
> +		error(_("could not rewrite %s"), ref_stash);
> +		goto cleanup;
> +	}
> +
> +	transaction = ref_store_transaction_begin(refs, 0, &err);
> +	if (!transaction)
> +		goto restore;
> +
> +	for (i = data.nr; i-- > 0; ) {
> +		if (ref_transaction_update_reflog(transaction, ref_stash,
> +						  &data.entries[i].new_oid,
> +						  &data.entries[i].old_oid,
> +						  data.entries[i].committer,
> +						  i == idx ? reworded_msg :
> +							     data.entries[i].msg,
> +						  index++, &err))
> +			goto restore;
> +	}
> +
> +	if (ref_transaction_commit(transaction, &err))
> +		goto restore;

Is this a joke implementation, or is our reflog API so feature-poor
that it does not even allow replacing a single entry, leaving the
application to slurp everything, remove it, and recreate everything
from scratch with only a single entry modified in the middle?

What happens if your process gets killed after refs_delete_reflog()
returns but before finishing writing out what you collected?  The
copy you hoard in memory is the only copy, and we will lose the
data.

Use of a transaction here does not help us at all.  When we abort,
we end up losing the reflog we had on disk before starting that
transaction.

Am I reading the code incorrectly?  If I am not, I doubt that the
above implementation is acceptable.

I wonder if the reflog API needs to be extended before we can
implement this properly.  I imagine a set of functions like (there
may be others)

 * refs_reflog_replace(ref_stash, idx, &reflog_data);

   This would replace the reflog entry at idx with the data supplied,
   which would probably be a pointer to something like:

        struct reflog_data {
                const struct object_id *new_oid;
                const struct object_id *old_oid;
                const char *committer_info;
                const char *msg;
        } reflog_data;

   The files backend would implement this by doing something like
   the following sequence:

   - open a temporary file for writing, and the current reflog file
     for reading (with a lock);
   - copy the contents of the current reflog file to it, up to the
     specified index;
   - write out the single entry supplied in the reflog_data
     structure;
   - skip one entry in the current reflog file (which we are
     replacing);
   - copy the remainder of the current reflog file;
   - atomically replace the current reflog file with the temporary
     file.

   The implementation for reftable may be vastly different, but
   being a more database-oriented backend, it may be simpler to
   replace a single entry in it.  I dunno.

 * refs_reflog_edit_in_bulk(ref_stash, num_edit, reflog_edit[]);

   This would give us a bulk-edit interface, where reflog_edit would
   be an array of structures, perhaps like this:

        struct reflog_edit {
                int idx;
                enum {
                        DELETE_REFLOG_ENT,
                        REPLACE_REFLOG_ENT,
                        INSERT_REFLOG_ENT,
                } what;
                struct reflog_data data;
        } reflog_edit[];

   The '.what' member would instruct the function what to do at the
   specified '.idx' in the reflog, whether to delete the existing
   entry, replace it, or insert a new entry.  The '.data' member is
   used when replacing or inserting, but is ignored when deleting.

   You may require the caller to sort the elements in this array in
   increasing order of the '.idx' member if it makes the
   implementation easier.  Or the implementation can sort the array
   internally before starting to process the request.

will become the foundations of such a feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants