t3512 & t3513 'unexpected passes'

t3512 & t3513 'unexpected passes'

[Ramsay Jones]

Hi Junio,

For several days, I have been staring at some 'unexpected passes' in
the t3512-cherry-pick-submodule.sh and t3513-revert-submodule.sh test
files (tests #11-13 in both cases).

I finally found time tonight to 'git bisect' the 'problem', and found
that bisect fingered commit b5a812b298 ("sequencer: try to commit without
 forking 'git commit'", 17-11-2017).

During the bisection I stumbled across (several times) the following
compilation error:

      CC sequencer.o
  sequencer.c: In function ‘do_commit’:
  sequencer.c:1142:9: error: void value not ignored as it ought to be
       res = print_commit_summary(NULL, &oid,
           ^
  Makefile:2105: recipe for target 'sequencer.o' failed
  make: *** [sequencer.o] Error 1

which I 'fixed' like so: s/res = //

So, given that the 'fingered' commit didn't immediately seem to be
related to the problem, along with the above errors, this may well
not be the culprit.

Just FYI.

ATB,
Ramsay Jones

Re: t3512 & t3513 'unexpected passes'

[Adam Dinwoodie]

On Monday 20 November 2017 at 08:16 pm +0000, Ramsay Jones wrote:
> For several days, I have been staring at some 'unexpected passes' in
> the t3512-cherry-pick-submodule.sh and t3513-revert-submodule.sh test
> files (tests #11-13 in both cases).
> 
> I finally found time tonight to 'git bisect' the 'problem', and found
> that bisect fingered commit b5a812b298 ("sequencer: try to commit without
>  forking 'git commit'", 17-11-2017).
> 
> During the bisection I stumbled across (several times) the following
> compilation error:
> 
>       CC sequencer.o
>   sequencer.c: In function ‘do_commit’:
>   sequencer.c:1142:9: error: void value not ignored as it ought to be
>        res = print_commit_summary(NULL, &oid,
>            ^
>   Makefile:2105: recipe for target 'sequencer.o' failed
>   make: *** [sequencer.o] Error 1
> 
> which I 'fixed' like so: s/res = //
> 
> So, given that the 'fingered' commit didn't immediately seem to be
> related to the problem, along with the above errors, this may well
> not be the culprit.

I've seen the same unexpected passes, and had just completed the same
bisect run myself, although I fixed the build failure by cherry-picking
82921316a ("SQUASH???", 2017-11-18) onto commits that wouldn't build,
given that commit seems to exist entirely to fix that build breakage.  I
think that adds more weight to b5a812b29 being the culprit for these
unexpected passes.

It's definitely the case that these unexpected passes exist at 8e4ff0ae1
("Merge branch 'pw/sequencer-in-process-commit' into pu", 2017-11-21)
and do not exist at its immediate left-hand parent, e017a4ccc ("Merge
branch 'jn/ssh-wrappers' into jch", 2017-11-21), which means it's
clearly _something_ in the branch merged at 8e4ff0ae1.

Re: t3512 & t3513 'unexpected passes'

[Phillip Wood]

On 21/11/17 10:44, Adam Dinwoodie wrote:
> On Monday 20 November 2017 at 08:16 pm +0000, Ramsay Jones wrote:
>> For several days, I have been staring at some 'unexpected passes' in
>> the t3512-cherry-pick-submodule.sh and t3513-revert-submodule.sh test
>> files (tests #11-13 in both cases).

Thanks for pointing this out.

<snip>

> I've seen the same unexpected passes, and had just completed the same
> bisect run myself, although I fixed the build failure by cherry-picking
> 82921316a ("SQUASH???", 2017-11-18) onto commits that wouldn't build,
> given that commit seems to exist entirely to fix that build breakage.  I
> think that adds more weight to b5a812b29 being the culprit for these
> unexpected passes.
> 
> It's definitely the case that these unexpected passes exist at 8e4ff0ae1
> ("Merge branch 'pw/sequencer-in-process-commit' into pu", 2017-11-21)
> and do not exist at its immediate left-hand parent, e017a4ccc ("Merge
> branch 'jn/ssh-wrappers' into jch", 2017-11-21), which means it's
> clearly _something_ in the branch merged at 8e4ff0ae1.

If I've understood the comments by those tests this has to do with the
new code detecting empty commits where the old code does not or vice
versa. The new code detects an attempt to create an empty commit by
comparing the tree that is about to be committed to HEAD and complains
if they are the same. I've had a quick look through the code for 'git
commit' but I need more time to figure out exactly what it does in this
case (I've a feeling from when I looked a while ago that it does this
test before it writes the tree). If anyone has any insight as to why the
tests fail with the current commit implementation that would be helpful
for understanding what is going on here - unfortunately I've never
really used submodules.

Best Wishes

Phillip

Re: t3512 & t3513 'unexpected passes'

[Junio C Hamano]

Adam Dinwoodie <adam@dinwoodie.org> writes:

> I've seen the same unexpected passes, and had just completed the same
> bisect run myself, although I fixed the build failure by cherry-picking
> 82921316a ("SQUASH???", 2017-11-18) onto commits that wouldn't build,
> given that commit seems to exist entirely to fix that build breakage.

Your assumption is correct wrt "SQUASH???".  When I am in a hurry
and cannot spend extra cycles to pinpoint which one of the commits
in a multi-patch series breaks a build, I just fix the breakage on
top of the tip commit, and leave it to the original author of the
series to apply them down to the original breakage in individual
commits when the topic is rerolled.

It seems that the compilation breakage 82921316a fixes comes from
its immediate parent, so I'll squash them together, just in case a
rerolled topic does not materialize.

Thanks.