Re: [GSoC][RFC] Proposal: Improve consistency of sequencer commands

[Elijah Newren <newren@gmail.com>]

Hi Rohit!

On Fri, Mar 22, 2019 at 8:12 AM Rohit Ashiwal
<rohit.ashiwal265@gmail.com> wrote:
>
> Hey People
>
> I am Rohit Ashiwal and here my first draft of the proposal for the project
> titled: `Improve consistency of sequencer commands' this summer. I need your
> feedback and more than that I need help to improve the timeline of this
> proposal since it looks very weak. Basically, it lacks the "how" component
> as I don't know much about the codebase in detail.
>
> Thanks
> Rohit
>
> PS: Point one is missing in the timeline from the ideas page[0], can someone
>     explain what exactly it wants?

I don't understand the question; could you restate it?

> Points to work on:
> ------------------
>     - Add `git cherry-pick --skip`

I'd reword this section as 'Consistently suggest --skip for operations
that have such a concept'.[1]

Adding a --skip flag to cherry-pick is useful, but was only meant as a
step.  Let me explain in more detail and use another comparison point.
Each of the git commands cherry-pick, merge, rebase take the flags
"--continue" and "--abort"; but they didn't always do so and so
continuing or aborting an operation often used special case-specific
commands for each (e.g. git reset --hard (or later --merge) to abort a
merge, git commit to continue it, etc.)  Those commands don't
necessarily make sense to users, whereas '<operation> --continue' and
'<operation> --abort' do make intuitive sense and is thus memorable.
We want the same for --skip.

Both am-based rebases and am itself will give advice to the user to
use 'git rebase --skip' or 'git am --skip' when a patch isn't needed.
That's good.  In contrast, interactive-based rebases and cherry-pick
will suggest that the user run 'git reset' (with no arguments). The
place that suggests that command should instead suggest either 'git
rebase --skip' or 'git cherry-pick --skip', depending on which
operation is in progress.  The first step for doing that, is making
sure that cherry-pick actually has a '--skip' option.

>     - Implement flags that am-based rebases support, but not interactive
>           or merge based, in interactive/merge based rebases

The "merge-based" rebase backend was deleted in 2.21.0, with all its
special flags reimplemented on the top of the interactive backend.  So
we can omit the deleted backend from the descriptions (instead just
talk about the am-based and interactive backends).

>     - [Bonus] Deprecate am-based rebases
>     - [Bonus] Make a flag to allow rebase to rewrite commit messages that
>           refer to older commits that were also rebased

I'd reorder these two.  I suspect the second won't be too hard and
will provide a new user-visible feature, while the former will
hopefully not be visible to users; if the former has more than
cosmetic differences visible to user, it might transform the problem
into more of a social problem than a technical one or just make into
something we can't do.

> Proposed Timeline
> -----------------
>     + Community Bonding (May 6th - May 26th):
>         - Introduction to community
>         - Get familiar with the workflow
>         - Study and understand the workflow and implementation of the project in detail
>
>     + Phase 1  (May 27th - June 23rd):
>         - Start with implementing `git cherry-pick --skip`
>         - Write new tests for the just introduced flag(s)
>         - Analyse the requirements and differences of am-based and other rebases flags

Writing or finding tests to trigger all the --skip codepaths might be
the biggest part of this phase.  Implementing `git cherry-pick --skip`
just involves making it run the code that `git reset` invokes.  The
you change the error message to reference `<command> --skip` instead
of `git reset`.  What you're calling phase 1 here isn't quite
microproject sized, but it should be relatively quick and easy; I'd
plan to spend much more of your time on phase 2.

>     + Phase 2  (June 24th - July 21st):
>         - Introduce flags of am-based rebases to other kinds.
>         - Add tests for the same.

You should probably mention the individual cases from "INCOMPATIBLE
FLAGS" of the git rebase manpage.  Also, some advice for order of
tackling these: I think you should probably do --ignore-whitespace
first; my guess is that one is the easiest.  Close up would be
--committer-date-is-author-date and --ignore-date.  Re-reading, I'm
not sure -C even makes sense at all; it might be that the solution is
just accepting the flag and ignoring it, or perhaps it remains the one
flag the interactive backend won't support, or maybe there is
something that makes sense to be done.  There'd need to be a little
investigation for that one, but it might turn out simple too.  The
--whitespace={nowarn|warn|fix|error|error-all} flag will be the
kicker.  I don't know how long that one will take, but I'm certain
it's harder than the other flags and it might conceivably take up most
the summer or even extend beyond.

>     + Phase 3  (July 22th - August 19th):
>         - Act on [Bonus] features
>         - Documentation
>         - Clean up tasks

I'd prefer that Documentation updates were made as you went; you'll
particularly need to look at Documentation/git-cherry-pick.txt and
Documentation/rebase.txt, especially the "INCOMPATIBLE OPTIONS" and
"BEHAVIORAL DIFFERENCES" sections of the latter.

Also, as far as timing goes, the rewriting of commit messages seems
relatively straightforward; you may want to consider doing it before
the --whitespace flag (despite the fact that I originally suggested it
as a bonus item).  Deprecating am-based rebases, on the other hand, is
a bit of a wildcard.  It depends on Phase 2 being completed so it
definitely makes sense to be last.  If phase 2 is complete, it's
conceivable that deprecating am-based rebases only takes a little more
work, but it might expand to use up a lot of time.

> Relevant Work
> =============
> Dscho and I had a talk on how a non-am backend should implement `git rebase
> --whitespace=fix`, which he warned may become a large project (as it turns
> out it is a sub-task in one of the proposed ideas[0]), we were trying to
> integrate this on git-for-windows first.
> Keeping warning in mind, I discussed this project with Rafael and he suggested
> (with a little bit uncertainty in mind) that I should work on implementing
> a git-diff flag that generates a patch that when applied, will remove whitespace
> errors which I am currently working on.

It's awesome that you're looking in to this, but it may make more
sense to knock out the easy parts of this project first.  That way the
project gets some value out of your work for sure, you gain confidence
and familiarity with the codebase, and then you can tackle the more
difficult items.  Of course, if you're just exploring to learn what's
possible in order to write the proposal, that's fine, I just think
once you start on this project, it'd make more sense to do the easier
ones first.


Hope that helps,
Elijah