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

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

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

> 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);
>  * refs_reflog_edit_in_bulk(ref_stash, num_edit, reflog_edit[]);
>
> will become the foundations of such a feature.

On further thought, I think this fits pretty well into the general
architecture of the refs subsystem.  Both backends would need
refs_reflog_edit_in_bulk() in their vtable, while the single-entry
edit can just be a thin wrapper passing a single-element
reflog_edit[] array with a 'replace' operation.

If someone is interested in implementing this, there are a few
tricky details to be careful about:

 * With delete/insert, indices drift.  In "insert at stash@{5},
   replace stash@{10}", the second instruction targets what was
   originally position #10, which becomes #11 after the insertion
   at #5.  Pre-scanning the reflog_edit[] array in user order to
   annotate each element with an effective '.idx' value should
   resolve this, or something along those lines.

 * Multiple reflog_edit[] elements may target the same '.idx'.  In
   "replace stash@{4} with 'hello', replace stash@{4} with 'bye'",
   stash@{4} should end up as 'bye'.  If a backend sorts
   reflog_edit[] by '.idx' (or in reverse, as the files backend
   might do when copying from largest index to smallest),
   processing must produce the same result as unsorted execution.
   The sort needs to be stable, probably keyed on effective '.idx'
   and tiebroken by original array position.

 * A reflog_edit[] array with "delete stash@{4}" followed by
   "replace stash@{4}" asks for an impossible operation and must
   error out.  Swapping the order (edit then delete) is technically
   valid, though it feels like a user mistake.  I am undecided on
   that one.

Although "git stash reword" needs only 'replace', edit_in_bulk()
could consolidate existing operations like "reflog delete", "stash
drop", and "stash pop", and help clean up refs_reflog_expire().
Even if initial support is limited to 'replace', designing for
'delete' and 'insert' upfront saves us from a future rewrite.

As for "git stash reword" handling multi-line messages, the
flat-file reflog format pretty much expects single-line entries.
Since "git stash push -m" already squishes contiguous whitespace
(including newlines) into a single space, "stash reword" should
probably follow suit.

That is about all for now.