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

From: Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
Date: Jun 2, 2015, 07:42 UTC
Message-ID: <53678876.5326.1433230965582.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <CAPig+cRGT4qgCNgVi_x-==GKze7VUPVbVPGHYui7y5ZXie72Qw@mail.gmail.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 14 quoted lines

> > +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:

Show 7 quoted lines

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

Show 10 quoted lines

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