Re: [PATCH v5 00/12] rebase: update branches in multi-part topic

[Derrick Stolee <derrickstolee@github.com>]

On 7/21/2022 3:43 PM, Elijah Newren wrote:
> On Thu, Jul 21, 2022 at 5:13 AM Derrick Stolee <derrickstolee@github.com> wrote:
>>
>> On 7/21/2022 12:35 AM, Elijah Newren wrote:
>>
>>> One thing I'm curious about (sorry to bring this up so late):
>>> "pick" commands come with the old commit hash.  Perhaps the update-ref
>>> commands should too?  (e.g. "update-ref refs/heads/topic from
>>> <OLDHASH>")
>>
>> I don't personally see the value here other than to make it harder
>> for someone to add new commands (and confusing when wanting to
>> create a brand new ref).
> 
> I apologize if my late query caused frustration. 

I intended this as "I don't understand, please enlighten me", but
looking back now it definitely sounds more frustrated. Sorry about
that.

> I was thinking it
> made more sense to ask now than later and was genuinely curious for
> thoughts on the idea, but perhaps it'd have been better if I just
> didn't bring it up.  Sorry.
 
It's definitely the time to bring it up, because it will be harder
to change it once it's shipped.

> But, if you're curious why I was thinking about it...
> 
> 
> There are four ways I can think of to handle the <oldvalue>: (A)
> include it in the "update-ref" instruction, (B) manually get the
> <oldvalue> at program invocation, (C) accept an increased race-window
> by not grabbing <oldvalue> until the line is parsed, or (D) ignore
> races and just don't provide <oldvalue> when updating refs.  If one
> chooses (B), they can either (B1) pre-parse the entire todo list and
> look up the current values of any refs mentioned in an update-ref
> instruction, or (B2) record the value of all existing refs at
> invocation.
> 
> Rebase is already paying the cost for pre-parsing the entire todo list
> (i.e. B1).  In fact, you _also_ preemptively locking the refs
> mentioned.  So, in a sense, rebase is already covered. However:
>   * adding such a thing could potentially remove a small race window
> (perhaps the user takes a while to edit the todo list, and the ref
> somehow gets updated before they even finish that step; being able to
> specify the oldref might help in such a case).
>   * existing commands (pick, merge, etc.) give the oldrefs (or labels
> to them), so having update-ref also do so might provide some "feel
> good" consistency.
> 
> Perhaps those reasons are pretty weak for the rebase case, given you
> already do B1.  However, `git replay` isn't going to do B1, and nor
> will it preemptively lock refs.  I see `git replay` as a patch-based
> analogue to the snapshot-based tools of fast-export and fast-import.
> (fast-import doesn't pre-parse its entire input before starting to
> process the first commit, nor does it preemptively lock refs.)  B2 is
> not appealing because some repositories have gargantuan numbers of
> refs.  Also, the update-ref-during-edit-of-todo rationale is more
> important for `git replay`, because I want `git replay` to accept a
> filename containing a todo list in lieu of a range of revisions to
> replay -- perhaps users spent quite some time or programming effort
> generating their todo list and it took some time to generate/review.
> And that further undercuts both B1 and B2.  So, between all those
> reasons, (A) is very appealing to me.
> 
> All that said, `git replay` already has other planned differences in
> its todo list (e.g. a "reset" directive must be the first non-comment
> directive, there's no separate "merge" directive, etc.), so if I have
> to add another one, it's no big deal.

I'm happy that you're working on 'git replay'. I'm excited to see how
it works. Also, it would be helpful to keep these things in line as
much as possible.

> Your point about specifying <oldref> being seen as friction for some
> users is well taken, and I'm thinking of making the "from <oldref>"
> part of the instruction optional (though documenting that users are
> accepting some race risk by switching from (A) to (C) when they omit
> it).
>
>> We could consider adding a comment in the
>> future without any backwards compatibility issues:
>>
>>         update-ref refs/heads/my-ref # was 0123dead
> 
> Yes, this could certainly be added later (though none of my reasons
> for wanting <oldref> were geared around pointers for the end user, so
> this form wouldn't be helpful to me).
> 
> Also, if "from <oldref>" is optional, it could also be added later.

The optional value makes this something that we can add later, which
I think is good flexibility. I was thinking about doing the comment
thing as a follow-up (in addition to Phillip's error mode comments)
but maybe I'll explore this direction instead.
 
> If you've read this far, let me just take a minute to again thank you
> for your work on this --update-refs option to rebase.  It's some cool
> work.  Also, thanks for listening to my random ideas and musings along
> the way.  :-)

I really appreciate the attention you've given it. It's already
improved a lot due to your help.

Thanks,
-Stolee