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

>>  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?

>>> I feel like you're brushing aside those repeated attempts to point out
>>> those usecases as though they don't exist or don't matter, with some
>>> separate new motivation needed for these options to be allowed?  And
>>> perhaps saying those usecases should all be *ruled out* apriori,
>>> because you're concerned the implementation *might* also allow other
>>> unintended uses?
>>>
>>> Something isn't working here.
>>
>> I'm trying to read the patches and make sense of what is written there.
>>
>> The current patch especially is far too lightly documented for what
>> it is actually implementing.
>>
>> Even its documentation states this:
>>
>> +<revision-range>::
>> +       Range of commits to replay; see "Specifying Ranges" in
>> +       linkgit:git-rev-parse.
>>
>> This "Specifying Ranges" section describes exactly category (1) of
>> what I was talking about, but really the patch enables everything
>> in "Commit Limiting" from git-rev-list.
>>
>> Based on what I see in the patch, I can't help but think that the
>> extra options are an accident.
> 
> This all would be a fine review comment, if you had said that.
> Instead, you stated that setup_revisions() shouldn't be used (which
> might have been fine as an initial comment), and when I pointed out
> exact options and flags that were needed at a minimum you still said
> they shouldn't generally be supported, and after pointing out
> usecases, multiple times, you responded with "if we can find a
> motivation".
> 
> There are two reasons I found that problematic:
> 
> Sometimes it makes sense to say that a usecase shouldn't be supported.
> But, when there's months of effort so far put into implementing those
> usecases, I would expect strong statements about ruling out usecases
> to be accompanied with really good explanations of why those exact
> cases are deemed "bad" and the best possible alternative solutions
> that should instead be implemented to solve something close to the
> proposer's stated desires.  I didn't see that.
> 
> Similarly, there are times when a reviewer should say that code should
> not be implemented a certain way.  But when the person responds with
> some alternatives and their _minimum_ needs trying to find some common
> ground, and has put months of efforts into this wider goal, I would
> expect that ruling out certain methods and their follow up
> alternatives to be accompanied with alternative solutions of your own
> that can solve the proposer's goals; not merely attempting to veto
> suggestions immediately with seemingly no path forward.
> 
> It was an unfortunate set of examples that seemed far below your
> typically excellent reviews.  In contrast, this latest email of yours
> is another high quality response in line with your emails prior to this thread.

> 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

> Importantly, the first two issues in particular mean that when the
> first series comes:
> 
>   * If you insist on (or even just suggest) certain changes that
> happen to break capabilities in the pipeline, especially in a fashion
> that cannot backward-compatibly be fixed, then I think the only path
> forward (other than dropping your suggestions) is to engage in
> discussions about those usecases & design *without* the testcases and
> code being available yet.

>> But back to my original suggestion: you can also do something simpler
>> for v1 of git-replay
> 
> That's workable.
> 
>> (say, very limited revision parsing such as one A..B range)
> 
> I'm not fine with that, though.  We already have tests of multiple
> positive refs, so there's no reason to exclude those.  In fact,
> keeping them in is especially important as a means of avoiding having
> other reviewers make suggestions to copy rebase's broken design
> (namely, being limited to a couple commits passed as positional
> arguments with one implicitly being an exclude).

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