Re: [PATCH 11/14] replay: use standard revision ranges

From: Elijah Newren <newren@gmail.com>
Date: Apr 30, 2023, 06:45 UTC
Message-ID: <CABPp-BGRtcBQ_6fkMrTskV9dk71ffycXZ8hEE_RaOrAdza_wLA@mail.gmail.com>
In-Reply-To: <58f7918f-6ca3-2158-7d9e-bfcd8eb24b0d@github.com>

On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee <derrickstolee@github.com> wrote:

Show 25 quoted lines

>
> On 4/22/2023 9:18 PM, Elijah Newren wrote:
> > On Thu, Apr 20, 2023 at 6:44 AM Derrick Stolee <derrickstolee@github.com> wrote:
> >>
> >> On 4/20/2023 12:53 AM, Elijah Newren wrote:
> >>> On Tue, Apr 18, 2023 at 6:10 AM Derrick Stolee <derrickstolee@github.com> wrote:
>
> >>  3. (Ordering options) Modifications to how those commits are ordered,
> >>     such as --topo-order, --date-order, and --reverse. These seem to
> >>     be overridden by git-replay (although, --reverse probably causes
> >>     some confusion right now).
> >
> > Yep, intentionally overridden.
> >
> > Could you elaborate on what you mean by --reverse causing confusion?
>
> It's very unlikely that a list of patches will successfully apply
> when applied in the reverse order. If we allow the argument, then
> someone will try it thinking they can flip their commits. Then they
> might be surprised when there are a bunch of conflicts on every
> commit.
>
> Basically, I'm not super thrilled about exposing options that are
> unlikely to be valuable to users and instead are more likely to cause
> confusion due to changes that won't successfully apply.

Oh, I got thrown by the "right now" portion of your comment; I couldn't see how time or future changes would affect anything to make it less (or more) confusing for users.

Quick clarification, though: while you correctly point out the type of confusion the user would experience without my overriding, my overriding of rev.reverse (after setup_revisions() returns, not before it is called) precludes that experience. The override means none of the above happens, and they would instead just wonder why their option is being ignored.

[...]

Show 13 quoted lines

> > The rest is all fair, but I'd like to point out that there are a few
> > problems here:
> >
> >   * Tests should generally be passing before submitting upstream, so
> > all the code to implement them needs to be sent too
> >   * Submitted patch series have to be digestible sizes; not everything
> > can be submitted at once
> >   * Christian and Dscho wanted some of what I had implemented despite
> > other parts not being ready
>
> Outside of my first response (using positional arguments, recommended
> before you provided the extra context I was missing) I have not
> suggested implementing something that can't be forward compatible.

True, and maybe I was misunderstanding, but I thought you were trying to make it forward compatible to only slight deviations while ruling out most potential future changes, including ones I was specifically highlighting as minimum eventual requirements. While it was clear that _some_ of the limitations you were trying to impose were only on v1, it sounded like you were also trying to rule out a wide swath of things permanently. That's where things really concerned me, and why I specifically asked which you were trying to do.

To be honest, I'm still unsure what your original intent was.  But it
probably doesn't matter now, as it sounds like you've shifted.  I
_think_ an accurate summary of where we are now at the end of
this thread is:
  * I am predominantly concerned about flexibility/potential for
git-replay in the future (what goes in v1 doesn't matter *if* it
doesn't affect the future, but that's a huge "if").
  * You are predominantly concerned about v1 and only including the
portions of functionality that have associated tests

I think there's enough wiggle room to mostly satisfy both viewpoints.

You helped with my concern by stating that categories (1) and (2) should be fine for git-replay in the future. (It at least handles the forms of potential lock-in that I already know about, though I'm worried there are others.)

I believe we can give you what you want via a tweak to the hack you previously suggested for v1 only: rule out passing any options with a leading dash to setup_revisions(), *and* throw an error if revs->prune_data.nr is nonzero after calling setup_revisions(). But I'd want us to include a comment pointing out that it's a hack, and include in that note that in the future we want to allow all options in categories (1) and (2) to be passed to setup_revisions() (or passed to a suitable refactoring thereof).

[...]

Show 29 quoted lines

> >> so your progress here isn't blocked on refactoring the revisions
> >> API.
> >
> > Is that a positive or a negative?  That question may surprise you, so
> > let me explain.
> >
> > I have a conflict of interest of sorts.  When Christian and Dscho
> > (separately) approached me earlier this year and pointed out that
> > git-replay had the functionality they wanted, I was happy that others
> > liked it, and gave some pointers here and there, but I was _very_
> > worried that upstreaming it would result in something getting
> > backward-incompatibly locked into place that prevents the pieces that
> > are  still in progress from getting implemented.  And I was concerned
> > that my plans to create a tool people could experiment with, and that
> > we could get usability feedback on (as I suggested at the Git
> > contributors summit last year) might be in jeopardy as pieces of its
> > functionality get locked into place before it's even ready for
> > usability testing.  I was silently hoping they'd lose steam on
> > rebasing and cleaning up my patches or choose to just delay until I
> > could get real time to work on it with them.  (Since then, the one
> > person who had ever advocated for my Git work at $DAYJOB, the best
> > manager I had ever worked under, became not-a-manager.  I was
> > blindsided by that in February.  Also, I have been transferred to a
> > different team and am spinning up there.  And so, to my dismay, I'm
> > worried my little sliver of Git time at work may evaporate entirely
> > rather than return to previous healthier levels.)  Anyway, I've been
> > fretting about things getting "locked-in" for a few months.
>
> I'm also upset that you have been disrupted like this.

Thanks.

Show 12 quoted lines

> > They didn't lose steam, as I found out when these patches were
> > submitted.  But, uh, I'm silently torn because I want to help
> > Christian and Dscho get what they want, but having them be blocked on
> > progress would reduce my stress.  A lot.
>
> From my perspective, git-replay's most important use is being able
> to generate rebases without a worktree or an interactive user. For
> now, I don't even care if that includes conflict resolution. That's
> enough of a lift that has enough unknowns that adding a complex CLI
> at this early stage seems like a hasty decision to me.  I'm voicing
> my opinion that we should avoid backwards-compatibility issues by
> implementing only the essentials.

That comment is mostly reasonable, but I'd like to nit-pick the last sentence: while limiting the series further for now is okay, I don't believe that only implementing the essentials is any kind of guarantee against backwards-compatibility issues. More on that below.

> That said, I also want to make sure that you eventually get the
> super-flexible command that you want, but only when the framework
> is ready for that kind of flexibility.

Yaay!!!

Show 8 quoted lines

> > Is there any chance people would be willing to accept a "NO BACKWARD
> > COMPATIBILITY GUARANTEES" disclaimer on this command for a (long?)
> > while, like Victoria suggested at Review club?  That might be an
> > alternate solution that would lower my worries.
>
> I'm not crazy about this idea, especially when it is easy to do
> something simpler and avoid the need for it. But I'm just one voice
> and one opinion.

Sorry I wasn't clear. I wasn't suggesting this as a way of avoiding something simpler; I was suggesting it as an addition to doing something simpler, because it's not clear to me that doing something simpler is sufficient.

Actually, I think it goes a bit further than that. Most of my objections to various forms of simplifying are due to the fact that I think that simplifying greatly increases the risks of accidental backward compatibility issues.

Part of the reason why I feel that way is that simplifying has to go through future rounds of review and people may well respond with, "oh, that'd be easier if you just did <X> instead of <Y>" with an implicit assumption that there's no difference between them if <simplification Z> is being presented as all we need, since <X> and <Y> handle that simplification equally well. It's kind of hard to guess in advance all the forms that <X>, <Y>, and <Z> combined can take, and I also have no idea if I'll have time to respond to upcoming reviews in a timely fashion given current conditions, especially as I'm not the one submitting the patches, so simplifying is a *big* risk in my book. And this clearly isn't just theoretical, as this thread started precisely with a simplification suggestion that would have broken everything. But even more importantly, I didn't start git-replay until I found too many backward compatibility issues in rebase that just couldn't be resolved, and yet I know that when folks review patches they will make suggestions in line with what they are familiar with, likely meaning rebase-like behavior. So, "limiting to the essentials" sounds like "increase the risk of future problems" to me...unless we include a backward-compatibility-explicitly-not-guaranteed-right-now disclaimer from the beginning.