Re: [lmi] [PATCH] Add script allowing to install any wxWidgets version from Git

[Vadim Zeitlin]

On Sun, 8 Apr 2018 23:36:21 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2018-04-07 11:20, Vadim Zeitlin wrote:
GC> > 
GC> [...moving this command
GC> > GC> -   [ -n "$skip_update" ] || git checkout "$wx_commit_sha"
GC> ...as was ultimately done in commit fdad3593f ...]
GC> > 
GC> >  To summarize, it's true that we don't have to do this test at all here 
any
GC> > more (sorry for leaving it in, I didn't realize it became unnecessary 
after
GC> > the refactoring I did), but the checkout command must be done in the 
"else"
GC> > branch, i.e. only if HEAD != wx_commit_sha.
GC> 
GC> Why must git-checkout not be run if [ HEAD = wx_commit_sha ]?

 It doesn't need to be run as it won't do anything anyhow.

GC> The less conditional logic, the better.

 I disagree with this because IMO it's confusing to perform clearly useless
actions, but it doesn't matter anyhow...

GC> problem here: if the repository hasn't been cloned yet, then the
GC> script clones it, implicitly checking out HEAD...and then it seems
GC> not to check out the desired version.

... because there is indeed a real problem here. My previously mentioned
refactoring was indeed wrong, but not in the way I thought it was :-( We
definitely do need run "git checkout" in any case unless skip_update was
explicitly set to 1.

GC> In the same vein, is there any strong reason not to unconditionalize
GC> the code that handles submodules?

 Same one as above: it's an optimization, we know for sure that these
commands won't do anything, so why bother running them?

GC> Here's my proposed patch:
GC> 
GC> 
---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
GC> diff --git a/install_wx.sh b/install_wx.sh
GC> index 70f2119a..90f42da4 100755
GC> --- a/install_wx.sh
GC> +++ b/install_wx.sh
GC> @@ -49,51 +49,40 @@ then
GC>      [ -d $wx_dir_parent ] || mkdir -p $wx_dir_parent
GC>      cd $wx_dir_parent
GC>      git clone "$wx_git_url" ${wx_dir##*/}
GC> -    cd $wx_dir
GC> -else
GC> -    cd $wx_dir
GC> -    if [ $(git rev-parse HEAD) = "$wx_commit_sha" ]
GC> -    then
GC> -        # Don't bother updating anything if we already had the correct 
version
GC> -        # of the tree.
GC> -        skip_update=1
GC> -    else
GC> -        # Get the missing commit from the upstream repository if we don't 
have
GC> -        # it yet.
GC> -        if ! git rev-parse --quiet --verify "$wx_commit_sha^{commit}" 
>/dev/null
GC> -        then
GC> -            git fetch "$wx_git_url"
GC> -        fi
GC> -        git checkout "$wx_commit_sha"
GC> -    fi
GC>  fi
GC>  
GC> -if [ "$skip_update" != 1 ]
GC> +cd $wx_dir
GC> +
GC> +# Fetch the desired commit from the upstream repository if it's missing.
GC> +if ! git rev-parse --quiet --verify "$wx_commit_sha^{commit}" >/dev/null

 Sorry, I really don't like this change as it tests for something that just
can't happen: we must have all commits after cloning a repository.

 Ideally I would just move the checkout command into the right place, i.e.
---------------------------------- >8 --------------------------------------
diff --git a/install_wx.sh b/install_wx.sh
index e1db49bd8..80b7aafaa
--- a/install_wx.sh
+++ b/install_wx.sh
@@ -64,12 +64,13 @@ else
         then
             git fetch "$wx_git_url"
         fi
-        git checkout "$wx_commit_sha"
     fi
 fi

 if [ "$skip_update" != 1 ]
 then
+    git checkout "$wx_commit_sha"
+
     # Initialize all the not yet initialized submodules (except for the known
     # exceptions, i.e. the submodules that we know that we won't need). This
     # will be necessary after the initial clone, but may also need doing after
---------------------------------- >8 --------------------------------------

 If you really want to update submodules during each update, you can make
this unconditional, although I'd prefer to avoid it. But please let's not
make the commit presence and the associated fetch calls unconditional, this
seems just wrong.

 Regards,
VZ