Issue #16 Use JCR context for checkout #17

kitarek · 2018-05-11T16:26:53Z

For the other commands filesystem context is retained.
As I proposed in issue #16 I was able to prepare and test a PR.

For the other commands filesystem context is retained.

alexkli

Thanks for the patch! A few comments inline.

alexkli · 2018-05-11T18:02:01Z

repo/repo

+function jcr_to_filesystem() {
+ local jcrPath=$1
+ # Handle namespaces and a default empty one
+ echo ${jcrPath} | sed -e 's#\/\([^:]*\):#/_\1_#g' | sed -e 's#/:#/#g'


sed

AFAICS, it seems it could match /path/jcr for input /path/jcr:content which we wouldn't want.

It probably should include the slash in the not-match array [^:\/] so that it does not go beyond a / when it searches for the next :.

sed

How did you come across /:? Isn't that an input error that we should probably throw on? Or is it a special case cleanup of the 1. sed and if yes, shouldn't that be fixed in the first one?

While we are at it, we need to double-escape a leading _ IIUC. This is the logic we have to copy here – jcr (repo) to filesystem (platform): https://github.com/apache/jackrabbit-filevault/blob/trunk/vault-core/src/main/java/org/apache/jackrabbit/vault/util/PlatformNameFormat.java#L66-L114
Note the StringBuffer initialized with a leading _.

BTW, we could apply the regexp approach to filesystem_to_jcr() as well instead of the hardcoded list. The underscore escaping can only happen for the first char after a slash, and if it's /path/__something then it shall become /path/_something IIUC:

https://github.com/apache/jackrabbit-filevault/blob/trunk/vault-core/src/main/java/org/apache/jackrabbit/vault/util/PlatformNameFormat.java#L137-L174

@alexkli: Thanks for thorough review! It makes sense to reiterate on this and fix/simplify the code. At least it was working for me. Let me find some time this week to fix this and I hope we can finish it soon.

My replies (but not code yet fixed):

Right, regexp usually takes the longest string that is able to match so we need to include both boundaries: / & : instead of just :

/: is a special allowed case of a default namespace (I briefly reviewed JCR standard looking for so called XML namespace prefix). The problem I see here is that we will end up with: __propertyName which is not exactly the same as propertyName. We probably should review how Vault deals with such things under the hood. I'm okay to drop this if Vault manages double _.

Probably because of _ escape __ will be treated as _ in that case so we cannot drop 2.

alexkli · 2018-05-11T18:16:29Z

repo/repo

 else
- humanFilter=$filter
+ humanFilter=$jcrFilter


I wonder if it now makes more sense to always print the filesystem path upon put and the jcr path upon get (see messages below). AFAICS, the path exists in the checkout case, so it would print the filesystem path here, whereas in later gets it would print the jcr path, a bit confusing. wdyt?

alexkli · 2018-05-11T18:19:41Z

repo/repo

 # get dirname and basename for each
 pathDirname=${path%/*}
-filterDirname=${filter%/*}
+filterDirname=${fsFilter%/*}


Might want to rename this to fsFilterDirname for consistent naming.

alexkli · 2018-05-11T18:19:51Z

repo/repo

 pathBasename=${path##*/}
-filterBasename=${filter##*/}
+filterBasename=${fsFilter##*/}


Might want to rename this to fsFilterBasename for consistent naming.

alexkli · 2018-05-11T18:21:40Z

repo/repo

@@ -577,15 +582,18 @@ else
 # get jcr path after jcr_root
 filter=${path##*/jcr_root}

+ # Deducting JCR path
+ fsFilter="${filter}"


$filter isn't used anymore later, so I suggest to merge the lines above and just have

fsFilter=${path##*/jcr_root}

and below use

jcrFilter=$(filesystem_to_jcr "${fsFilter}")

which should make it more obvious what's converted.

alexkli · 2018-05-11T18:23:06Z

repo/repo

@@ -535,7 +540,8 @@ done
 if [ $action == "checkout" ]; then
 # special checkout init steps
 # path argument must be a jcr path
- filter="$path"
+ jcrFilter="$path"
+ fsFilter=$(jcr_to_filesystem "$path")


Suggest to do fsFilter=$(jcr_to_filesystem "$jcrFilter") so it's a little more obvious.

kitarek · 2018-05-17T12:00:20Z

BTW: apart from the above comments, I haven't check yet the script with shellcheck tool.

WDYT to review it via that tool or it was already done in the past for repo, @alexkli ?

I would feel personally safer applying this PR after reviewing additionally shellcheck report on top of all your suggestions.

alexkli · 2018-05-17T18:42:19Z

Haven't run shellcheck or the like, curious to see what it will say 😄

kitarek · 2018-05-17T18:56:50Z

@alexkli I'm committing to fix most of them :-D

This is the output from: shellcheck -s bash repo/repo

In repo/repo line 46:
readonly PROGNAME=$(basename $0)
                             ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 131:
if [ $action == "checkout" ]; then
     ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 137:
if [ $action == "checkout" ]; then
     ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 144:
elif [ $action == "put" ]; then
       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 146:
elif [ $action == "get" ]; then
       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 148:
elif [ $action == "status" ]; then
       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 150:
elif [ $action == "diff" ]; then
       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 155:
elif [ $action == "localdiff" ]; then
       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 160:
elif [ $action == "serverdiff" ]; then
       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 167:
if [ $action == "checkout" ]; then
     ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 226:
    exit $1
         ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 233:
    cleanup ${2:-1}
            ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 256:
        echo "$(cd "$1"; pwd)"
                ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.


In repo/repo line 260:
            echo "$(cd "${1%/*}"; pwd)/${1##*/}"
                    ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.


In repo/repo line 291:
    echo ${jcrPath} | sed -e 's#\/\([^:]*\):#/_\1_#g' | sed -e 's#/:#/#g'
         ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 313:
    filter=$(urldecode $filter)
                       ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 315:
    echo $filter
         ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 326:
    echo $name
         ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 342:
    <filter root="$(to_xml $filter)"/>
                           ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 350:
<entry key="name">$(to_xml $pkgName)</entry>
                           ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 351:
<entry key="version">$(to_xml $pkgVersion)</entry>
                              ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 352:
<entry key="group">$(to_xml $pkgGroup)</entry>
                            ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 374:
    for ignoreFile in `find "$path" -name .vltignore`; do
                      ^-- SC2044: For loops over find output are fragile. Use find -exec or a while read loop.
                      ^-- SC2006: Use $(..) instead of legacy `..`.


In repo/repo line 379:
        cat $ignoreFile | sed -e "s,^,$relPath," >> "$1"
            ^-- SC2086: Double quote to prevent globbing and word splitting.
            ^-- SC2002: Useless cat. Consider 'cmd < file | ..' or 'cmd file | ..' instead.


In repo/repo line 385:
    pushd "$1" > /dev/null    
    ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.


In repo/repo line 387:
    popd > /dev/null
    ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.


In repo/repo line 395:
    curl -u $credentials -f -s -S $1
                                  ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 423:
    if [ $? -ne 0 ]; then
         ^-- SC2181: Check exit code directly with e.g. 'if mycmd;', not indirectly with $?.


In repo/repo line 500:
if [ $action == "st" ]; then action="status"; fi
     ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 604:
    source "$repoCfg"
    ^-- SC1090: Can't follow non-constant source. Use a directive to specify location.


In repo/repo line 641:
pathBasename=${path##*/}
^-- SC2034: pathBasename appears unused. Verify it or export it.


In repo/repo line 642:
filterBasename=${fsFilter##*/}
^-- SC2034: filterBasename appears unused. Verify it or export it.


In repo/repo line 645:
    humanFilter=$fsFilter/*
                ^-- SC2125: Brace expansions and globs are literal in assignments. Quote it or use an array.


In repo/repo line 663:
tmpDir=`mktemp -d -t repo.XXX`
       ^-- SC2006: Use $(..) instead of legacy `..`.


In repo/repo line 667:
write_excludes $excludes
               ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 678:
    rsync -avq --exclude-from=$excludes "$path" "$tmpDir/jcr_root$filterDirname"
                              ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 681:
    rm -f $excludes
          ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 683:
    build_zip $tmpDir
              ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 686:
        unzip -l $zipfile | grep "   jcr_root$fsFilter"
                 ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 694:
    upload_pkg $zipfile
               ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 695:
    install_pkg $pkg
                ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 696:
    delete_pkg $pkg
               ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 702:
    build_zip $tmpDir
              ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 705:
    upload_pkg $zipfile
               ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 706:
    build_pkg $pkg
              ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 709:
    rm -rf $tmpDir/*
           ^-- SC2115: Use "${var:?}" to ensure this never expands to /* .
           ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 712:
    download_pkg $pkg $zipfile
                 ^-- SC2086: Double quote to prevent globbing and word splitting.
                      ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 713:
    delete_pkg $pkg
               ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 715:
    pushd $tmpDir > /dev/null
    ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
          ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 717:
    popd > /dev/null
    ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.


In repo/repo line 726:
        rsync -avq --exclude-from=$excludes "$path" "$diffbase/LOCAL$filterDirname"
                                  ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 729:
        ln -s $tmpDir/jcr_root $diffbase/REMOTE
              ^-- SC2086: Double quote to prevent globbing and word splitting.
                               ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 732:
        pushd $diffbase > /dev/null
        ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
              ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 749:
        popd > /dev/null
        ^-- SC2164: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.


In repo/repo line 754:
            unzip -l $zipfile | grep "   jcr_root$fsFilter"
                     ^-- SC2086: Double quote to prevent globbing and word splitting.


In repo/repo line 762:
        rsync -avq --delete --exclude-from=$excludes "$tmpDir/jcr_root/$fsFilter" "$pathDirname"
                                           ^-- SC2086: Double quote to prevent globbing and word splitting.

alexkli · 2018-05-17T20:07:17Z

Ok, but I would suggest to do the shellcheck work in a separate PR, since it will affect the whole script, and shouldn't be mixed with the changes for this issue.

Issue Adobe-Marketing-Cloud#16 Use JCR context for checkout

aeba3d7

For the other commands filesystem context is retained.

alexkli requested changes May 11, 2018

View reviewed changes

alexkli added bug waiting for response Waiting for a response from the reporter(s) labels Jul 22, 2019

alexkli added this to In Progress in repo tool Jul 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #16 Use JCR context for checkout #17

Issue #16 Use JCR context for checkout #17

kitarek commented May 11, 2018

alexkli left a comment

alexkli May 11, 2018

alexkli May 11, 2018

kitarek May 17, 2018

alexkli May 11, 2018

alexkli May 11, 2018

alexkli May 11, 2018

alexkli May 11, 2018

alexkli May 11, 2018

kitarek commented May 17, 2018

alexkli commented May 17, 2018

kitarek commented May 17, 2018

alexkli commented May 17, 2018

Issue #16 Use JCR context for checkout #17

Are you sure you want to change the base?

Issue #16 Use JCR context for checkout #17

Conversation

kitarek commented May 11, 2018

alexkli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitarek commented May 17, 2018

alexkli commented May 17, 2018

kitarek commented May 17, 2018

alexkli commented May 17, 2018