I have updated the webrev to add the tclone. I also moved the initial clone from non-local source to only the closed repo case. The check isn't necessary for the case where all sub-repos will be relative to the root repo and not in a separate forest.
http://cr.openjdk.java.net/~mduigou/JDK-8038435/1/webrev/ Mike On Mar 26 2014, at 15:15 , Mike Duigou <mike.dui...@oracle.com> wrote: > > On Mar 26 2014, at 14:52 , David Katleman <david.katle...@oracle.com> wrote: > >> Hi Mike, >> >> A few minor comments, overall looks good. >> >> On 3/26/2014 2:11 PM, Mike Duigou wrote: >>> Hello all; >>> >>> I introduced bug in JDK-8030681 which prevents non-clone commands from >>> receiving parameters. >>> >>> I also slightly cleaned up checking for what is an acceptable root repo as >>> I had encountered this in a problem with a local clone and reverted prior >>> change which made the script a bash script. >>> >>> bug: https://bugs.openjdk.java.net/browse/JDK-8038435 >>> >>> webrev: http://cr.openjdk.java.net/~mduigou/JDK-8038435/0/webrev/ >> >>> 116 if [ "${command}" = "clone" -o "${command}" = "fclone" ] ; then >>> 221 if [ "${command}" = "clone" -o "${command}" = "fclone" ] ; then >> >> Perhaps out of scope of this fix, but shouldn't we add -o "${command}" = >> "tclone"? >> The "fclone" & "tclone" are both cosmetic, neither extensions are used. >> Can't remove "fclone" without potentionally breaking scripts. > > I can easily add the tclone. >> >>> 127 pull_default_tail=`echo ${pull_default} | sed -e >>> 's@^.*://[^/]*/\(.*\)@\1@'` >>> 128 if [ "x${pull_default}" = "x${pull_default_tail}" ] ; then >>> 129 echo "ERROR: Need initial clone from non-local source" > >>> ${status_output} >>> 130 exit 1 >>> 131 fi >> >> The above code looks right for what you want to do, but is it necessary to >> restrict hgforest.sh to only operate vs non-local? > > The problem is that the default path is used to figure out the peer name in > the extra (closed) repo. It's not possible to do this from a local path > unless you make assumptions or we change the requirements for the extra path > to include the appropriate root repo. > >> >> Seems one should still be able to have their own forest, and clone from that >> forest locally. >> >>> 238 echo "cd ${i} && hg${global_opts} ${command} ${command_args}" >>> > ${status_output} >>> 239 cd ${i} && (PYTHONUNBUFFERED=true hg${global_opts} ${command} >>> ${command_args}; echo "$?" > ${tmp}/${repopidfile}.pid.rc ) 2>&1 & >> >> Add "PYTHONUNBUFFERED=true" to the echo line. > > I am not sure why this would be important. It would seem to be just clutter > to me. > > Thanks! > > Mike
