Re: [PATCH/RFCv2 2/2] git rebase -i: warn about removed commits

[Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>]

Eric Sunshine <sunshine@sunshineco.com> writes:
> > +rebase.checkLevel::
> > +       If set to "warn", git rebase -i will print a warning if some
> > +       commits are removed (i.e. a line was deleted) or if some
> > +       commits appear more than one time (e.g. the same commit is
> > +       picked twice), however the rebase will still proceed. If set
> 
> The cover letter says that v2 no longer checks for a duplicate,
> however, this documentation still mentions it.
> 
> > +rebase.checkLevel::
> > +       If set to "warn" print warnings about removed commits and
> > +       duplicated commits in interactive mode. If set to "error"
> 
> Same here.

I forgot to modify the documentation, really sorry about
that.
Corrected here.

Eric Sunshine <sunshine@sunshineco.com> writes:
> >  test_expect_success 'drop' '
> > -       git checkout master &&
> >         test_when_finished "git checkout master" &&
> >         git checkout -b dropBranchTest master &&
> 
> This "cleanup" change might deserve its own patch (or at least a
> mention in the commit message).

I will rather move the this cleanup to the first part of this patch,
where the test was added (it should cause no problem, since the patch
is still in discussion), it makes more sense. I actually thought that
was what I did but seems like I was wrong.

I forgot to double check my patch this time, and it is showing, my
bad.

Eric Sunshine <sunshine@sunshineco.com> writes:
> > +test_expect_success 'rebase -i respects rebase.checkLevel' '
> > +       test_config rebase.checkLevel error &&
> > +       test_when_finished "git checkout master" &&
> > +       git checkout -b tmp2 master &&
> > +       set_fake_editor &&
> > +       test_must_fail env FAKE_LINES="1 2 3 4" git rebase -i --root &&
> > +       test E = $(git cat-file commit HEAD | sed -ne \$p)
> > +'
> 
> Shouldn't you also explicitly test "warn" and "ignore" modes?

I don't think testing "ignore" is really necessary since it
corresponds to the default behaviour, it is thus silently tested by
the other tests.
Either way, I will add a test for "warn".

Rémi