On Sat, Dec 27, 2014 at 6:37 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Fri, Dec 26, 2014 at 6:11 PM, Doug Kelly <dougk.ff7@xxxxxxxxx> wrote:
> > git am will break when using diff.submodule=log; add some test cases
> > to illustrate this breakage as simply as possible. There are
> > currently two ways this can fail:
> >
> > * With errors ("unrecognized input"), if only change
> > * Silently (no submodule change), if other files change
> >
> > Test for both conditions and ensure without diff.submodule this works.
> >
> > Signed-off-by: Doug Kelly <dougk.ff7@xxxxxxxxx>
> > ---
> > diff --git a/t/t4255-am-submodule.sh b/t/t4255-am-submodule.sh
> > index 8bde7db..d9a1d79 100755
> > --- a/t/t4255-am-submodule.sh
> > +++ b/t/t4255-am-submodule.sh
> > @@ -18,4 +18,87 @@ am_3way () {
> > KNOWN_FAILURE_NOFF_MERGE_ATTEMPTS_TO_MERGE_REMOVED_SUBMODULE_FILES=1
> > test_submodule_switch "am_3way"
> >
> > +test_expect_success 'setup diff.submodule' '
>
> Since the tests are actually expected to fail at this point (before
> you've fixed the problem), use test_expect_failure. The follow-up
> patch, which fixes the problem, should flip them to
> test_expect_success.
Okay, that's easy enough to do.
>
> > + echo one >one &&
> > + git add one &&
> > + test_tick &&
> > + git commit -m initial &&
>
> Rather than performing these steps manually (here and below), perhaps
> test_commit would be suitable and more succinct.
>
> > + git rev-parse HEAD >initial &&
>
> Other scripts in the test suite don't bother with this indirection.
> Instead, they assign the variable here, then reference it in
> subsequent tests (and no need to redirect to a file).
>
> INITIAL=$(git rev-parse HEAD) &&
>
Both good comments; wasn't sure if that'd work. But easy to do!
> > +
> > + git init submodule &&
> > + (cd submodule &&
> > + echo two >two &&
> > + git add two &&
> > + test_tick &&
> > + git commit -m "initial submodule" &&
> > + git rev-parse HEAD >../initial-submodule) &&
>
> Style: Format the subshell like this:
>
> (
> ...commands...
> ) &&
Yeah, I was unsure on this style (docs were unclear), but that's easy to follow.
>
> > + git submodule add ./submodule &&
> > + test_tick &&
> > + git commit -m first &&
> > + git rev-parse HEAD >first &&
>
> Is file 'first' ever used anywhere?
Nope; somewhat vestigial code (that could probably be cleaned out --
and should).
Originally, I was testing the first commit, and then I realized that
the second commit
updating the submodule introduces the failure I was looking for.
>
> > + (cd submodule &&
> > + echo three >three &&
> > + git add three &&
> > + test_tick &&
> > + git commit -m "first submodule" &&
> > + git rev-parse HEAD >../first-submodule) &&
> > + git add submodule &&
> > + test_tick &&
> > + git commit -m second &&
> > + git rev-parse HEAD >second &&
> > +
> > + (cd submodule &&
> > + git mv two four &&
> > + test_tick &&
> > + git commit -m "second submodule" &&
> > + git rev-parse HEAD >../second-submodule) &&
> > + git add submodule &&
> > + echo four >four &&
> > + git add four &&
> > + test_tick &&
> > + git commit -m third &&
> > + git rev-parse HEAD >third &&
> > + git submodule update --init
> > +'
> > +
> > +INITIAL=$(cat initial)
> > +SECOND=$(cat second)
> > +THIRD=$(cat third)
>
> No need for this extra level of indirection. See above.
>
> > +run_test() {
> > + START_COMMIT=$1
> > + EXPECT=$2
>
> Although it's not specifically wrong here, someone adding code above
> these two lines later on may not notice the broken &&-chain, so it
> would be a good idea to keep the &&-chain intact.
OK, easy enough.
>
> > + (git am --abort || true) &&
> > + git reset --hard $START_COMMIT &&
> > + rm -f *.patch &&
> > + git format-patch -1 &&
> > + git reset --hard $START_COMMIT^ &&
> > + git submodule update &&
> > + git am *.patch &&
> > + git submodule update &&
> > + (cd submodule && git rev-parse HEAD >../actual) &&
> > + test_cmp $EXPECT actual
> > +}
> > +
> > +test_expect_success 'diff.submodule unset' '
> > + (git config --unset diff.submodule || true) &&
> > + run_test $SECOND 'first-submodule'
>
> Note that you're already inside a single-quoted string here, so
> 'first-submodule' is not quite doing what you expect. Double quotes
> would be more appropriate. Or, better, drop the quoting of
> first-submodule altogether since it's unnecessary.
Yep. Good note.
>
> > +'
> > +
> > +test_expect_success 'diff.submodule unset with extra file' '
> > + (git config --unset diff.submodule || true) &&
> > + run_test $THIRD 'second-submodule'
> > +'
> > +
> > +test_expect_success 'diff.submodule=log' '
> > + git config diff.submodule log &&
> > + run_test $SECOND 'first-submodule'
> > +'
> > +
> > +test_expect_success 'diff.submodule=log with extra file' '
> > + git config diff.submodule log &&
> > + run_test $THIRD 'second-submodule'
> > +'
> > +
> > test_done
> > --
> > 2.0.5
One other note that might simplify this extra test case that I thought about
while driving home yesterday evening was changing lib-submodule-update to
set diff.submodule=log inside prolog(). This wouldn't provide very
clear failure causes, but it would perhaps reduce duplicated code and simplify
the test case.
Thanks for the feedback, though, I'll send out a new version momentarily.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html