Re: [PATCHv6 1/3] git-rebase -i: add command "drop" to remove a commit

From: Remi Galan Alfonso <remi.galan-alfonso@ensimag.grenoble-inp.fr>
Date: Jun 23, 2015, 20:08 UTC
Message-ID: <122907552.733385.1435090110363.JavaMail.zimbra@ensimag.grenoble-inp.fr>
In-Reply-To: <xmqqwpyucjj1.fsf@gitster.dls.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> > Galan Rémi  <remi.galan-alfonso@ensimag.grenoble-inp.fr> writes:
> > 
> > > +test_rebase_end () {
> > > +        test_when_finished "git checkout master &&
> > > +        git branch -D $1 &&
> > 
> > Is this one guaranteed to succeed?  Do we want to consider it a
> > failure to remove "$1" (e.g. dropTest)?
> > 
> >     $ git branch -D no-such-branch ; echo $?
> >     error: branch 'no-such-branch' not found.
> >     1
> > 
> > If dropTest branch did not exist before the test that begins with
> > a call to this function, what happens?
>

Since the function is

> 	test_when_finished "git checkout master &&
> 		git branch -D $1 &&
> 		test_might_fail git rebase --abort" &&
> 	git checkout -b $1 master

If $1 doesn't exist, then it means that 'git checkout -b $1 master' failed, thus the test would fail before reaching the part in 'test_when_finished'. However I guess there are more reasons that could cause 'git branch -D $1' to fail so I'll add a 'test_might_fail' in front of it.

> Besides, a function that must be called at the beginning of a test
> piece has a name that ends with _end?  That sounds funny, no?

I see your point but I'm not really sure how to call it, considering
that what it does is creating a branch to test on it, and taking care
of the cleaning at the end of the test.
Maybe something more generic like "setup_and_clean" ?

Show 18 quoted lines

> > +        test_might_fail git rebase --abort" &&
> > +        git checkout -b $1 master
> > +}
> 
> I'm wondering if this is not sufficient.
> 
>         test_rebase_i_drop_prepare () {
>                 git reset --hard &&
>                 git checkout -B "$1" master
>         }
> 
> I am guessing that you named _end because it has when_finished, but
> as far as I can tell, even after these three patches, the tests do
> not really rely on the fact that it is on 'master' branch.  More
> importantly, just being on 'master' branch is not a sufficient
> cleanliness for the next test (and that is why you added these
> "branch -D" and "might-fail rebase --abort" to this function in the
> first place), it seems.  So...

I removed the branch in case someone used the same name when creating a branch in a future test. However he would notice it when writing the test and it's not something that would suddenly break when modifying the code, so it might not be necessary. The "might-fail rebase --abort" is there in case this test fails in the middle (because of a future modification of rebase for example) to avoid failling all the following tests that use rebase.

The name "test_rebase_i_drop_prepare" wouldn't be accurate since 2/3 and 3/3 uses the function as well and don't really have much to do with 'drop'.

Thanks, Rémi