Re: Test::Smoke failing to test most recent commit to Perl 5 blead

On 01/27/2017 10:58 PM, demerphq wrote:
>
> On 28 Jan 2017 6:32 a.m., "James E Keenan" <[email protected]
> <mailto:[email protected]>> wrote:
>
>     On 01/14/2017 10:10 AM, James E Keenan wrote:
>     Here is the first part of the smokecurrent.log for this run -- with
>     some comments:
>
>     #####
>     [2017-01-27 08:11:22-0500] Read configuration from:
>     /usr/home/jkeenan/p5smoke/install/smokecurrent_config
>     [2017-01-27 08:11:22-0500] Commitlevel before sync:
>     d6115793d6cc41755a3ed4baaa38d30653656f41
>
>     # d611579 was HEAD in the previous branch being smoked:
>     smoke-me/jkeenan/130635-storable
>
>     [2017-01-27 08:11:22-0500] ==> Starting synctree
>     [2017-01-27 08:11:22-0500] Reading branch to smoke from:
>     '/usr/home/jkeenan/p5smoke/install/smokecurrent.gitbranch'
>     [2017-01-27 08:11:22-0500] In
>     pwd(/usr/home/jkeenan/p5smoke/git-perl) running:
>     [2017-01-27 08:11:22-0500] qx[/usr/local/bin/git pull --all]
>     From git://perl5.git.perl.org/perl <http://perl5.git.perl.org/perl>
>        1f74a12..9a7b7fb  blead      -> origin/blead
>     [2017-01-27 08:11:37-0500] Fetching origin
>     [2017-01-27 08:11:37-0500] Already up-to-date.
>     [2017-01-27 08:11:37-0500] In
>     pwd(/usr/home/jkeenan/p5smoke/git-perl) running:
>     [2017-01-27 08:11:37-0500] qx[/usr/local/bin/git remote prune origin]
>     [2017-01-27 08:11:38-0500] In
>     pwd(/usr/home/jkeenan/p5smoke/git-perl) running:
>     [2017-01-27 08:11:38-0500] qx[/usr/local/bin/git checkout blead
>     [2017-01-27 08:11:38-0500]  2>&1]
>     Switched to branch 'blead'
>     [2017-01-27 08:11:41-0500] Your branch is behind 'origin/blead' by 7
>     commits, and can be fast-forwarded.
>     [2017-01-27 08:11:41-0500]   (use "git pull" to update your local
>     branch)
>
>     # Note: No indication that 'git pull' was actually run!  Why not?
>
>     [2017-01-27 08:11:41-0500] In
>     pwd(/usr/home/jkeenan/p5smoke/perl-current) running:
>     [2017-01-27 08:11:41-0500] qx[/usr/local/bin/git reset --hard]
>
>     # Note: Although 'man git-reset' is not explicit about this, we can
>     probably assume
>     # that 'git reset --hard' with no <commit> resets to HEAD -- i.e.,
>     no update to checkout.
>
>
> Yes. It throws away any changes to the current branch. That should say
>
> git reset --hard origin/blead
>
>
>     [2017-01-27 08:11:43-0500] HEAD is now at d611579 Fix stack buffer
>     overflow in deserialization of hooks.
>     #####
>
>     So why does Test-Smoke not update the branch being tested in cases
>     like this?
>
>
> This is an educated guess: whoever wrote that code did not understand
> git well and got confused about what git pull does, and what git pull
> --all do.
>
> Seeing git pull in a script like this is a red flag, seeing  --all is a
> bigger red flag for me. ( It does not "pull all", it does a fetch
> against all remotes.)
>
> IMO git pull is not really suitable for scripting as it can trigger a
> rebase or merge, and trigger an editor.
>
> I would expect to see a sequence like this:
>
> git remote update -p
> git checkout $branch
> git reset --hard origin/$branch
>
> or like this:
>
> git remote prune --all
> git fetch --all
> git checkout $branch
> git reset --hard origin/$branch
>
> or even like this:
>
> git fetch origin
> git checkout $branch
> git reset --hard origin/$branch
>
> The difference between the variants being about whether the code should
> be managing multiple upstream repos or not. The --all introduces
> ambiguity about this, as it means "fetch code from --all remotes", so
> one might guess someone wanted to support multiple upstreams. On the
> other hand, my guess is that the person who coded it to do a 'git pull
> --all' thought that the -all would update --all branches, which it does
> not. Bolstering this view is that it seems to make little sense to smoke
> branches from multiple upstreams, especially for Perl. I would have
> expected only the canonical branches in the master upstream repo should
> be smoked, so the --all probably was a bug.
>
> What appears to be happening is that when the pull is executed it is in
> a different branch, so it fetches and then updates *THAT* branch only
> (via a merge!). When it then checks-out the new branch branch it is on
> an old commit, and requires a reset to the latest code.
>
> I would recommend that the smoke scripts do not use pull *ever*. They
> should never ever modify code, so doing a pull makes no sense.  git pull
> is basically a smart wrapper around git fetch + git merge. A smoker
> should *NEVER* be doing git-merge, so it should *never* be using git-pull.
>
> Note, I am fully aware that under perfect circumstances you /can/ script
> this using git pull. I would recommend not to. First it is confusing.
> Second, if circumstances are less than perfect then it could lead to
> unwanted behavior. For instance someone tinkers in a branch used by the
> smoker, then one could imaging the script breaking because of
> uncommitted changes, or breaking because pull has triggered a merge
> which requires a edited text message.
>
> FWIW, i have in the past recommended that new users to git do NOT use
> "git pull", until they have mastered using "git fetch" and "git rebase"
> or "git merge" as independent commands. Only once having experience of
> the three basic commands should people use git pull. Since it is a
> wrapper around three distinct commands it easily leads to confusion in
> the inexperienced.
>
> yves

Thanks for the feedback. Pull request filed:
https://github.com/abeltje/Test-Smoke/pull/26

jimk