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

[Junio C Hamano <gitster@pobox.com>]

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