Re: [PATCH v2] stash: add 'reword' subcommand

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 27, 2026, 17:00 UTC
Message-ID: <xmqqbjbsmkom.fsf@gitster.g>
In-Reply-To: <pull.2180.v2.git.1785149687514.gitgitgadget@gmail.com>

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

Show 46 quoted lines

> +	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.