Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue #16 Use JCR context for checkout #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #16 Use JCR context for checkout #17

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 26 additions & 19 deletions repo/repo
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
#!/bin/bash

###############################################################################
# #
# Copyright 2014 Adobe #
Expand Down Expand Up @@ -286,6 +285,12 @@ function urldecode() {
printf '%b' "${1//%/\\x}"
}

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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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 :.

  1. 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 _.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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):

  1. Right, regexp usually takes the longest string that is able to match so we need to include both boundaries: / & : instead of just :
  2. /: 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 _.
  3. Probably because of _ escape __ will be treated as _ in that case so we cannot drop 2.

}

function filesystem_to_jcr() {
# convert filesystem to JCR paths to be used as filters in content pkg
local filter=$1
Expand Down Expand Up @@ -323,7 +328,7 @@ function to_xml() {

function create_pkg_meta_inf() {
local base=$1
local filter=$(filesystem_to_jcr "$2")
local filter=$2
local pkgGroup=$3
local pkgName=$4
local pkgVersion=$5
Expand Down Expand Up @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# check if there is a jcr_root
if [[ "$PWD" == */jcr_root* ]]; then
Expand All @@ -553,12 +559,11 @@ if [ $action == "checkout" ]; then
echo
fi

path="$rootpath$filter"
path="$rootpath$fsFilter"
mkdir -p "$path"

# from here on, same as get
action="get"

else
# get, put, diff, etc.
# path argument is a file system path, jcr path must be deducted
Expand All @@ -577,15 +582,18 @@ else
# get jcr path after jcr_root
filter=${path##*/jcr_root}

# Deducting JCR path
fsFilter="${filter}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

jcrFilter=$(filesystem_to_jcr "${filter}")
rootpath=${path%/jcr_root*}/jcr_root
fi

# filter validation
if [[ $filter == "" ]]; then
filter="/"
if [[ "$jcrFilter" == "" ]]; then
jcrFilter="/"
fi

if [[ $filter == "/" ]]; then
if [[ "$jcrFilter" == "/" ]]; then
userfail "refusing to work on repository root (would be too slow or overwrite everything)"
fi

Expand Down Expand Up @@ -627,17 +635,16 @@ fi
if $fromVlt; then
print "Server $server taken from vault checkout $rootpath/.vlt"
fi

# get dirname and basename for each
pathDirname=${path%/*}
filterDirname=${filter%/*}
filterDirname=${fsFilter%/*}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to rename this to fsFilterDirname for consistent naming.

pathBasename=${path##*/}
filterBasename=${filter##*/}
filterBasename=${fsFilter##*/}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to rename this to fsFilterBasename for consistent naming.


if [ -d "$path" ]; then
humanFilter=$filter/*
humanFilter=$fsFilter/*
else
humanFilter=$filter
humanFilter=$jcrFilter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

fi

if [ $action == "put" ]; then
Expand All @@ -659,7 +666,7 @@ tmpDir=`mktemp -d -t repo.XXX`
excludes=$tmpDir/.excludes
write_excludes $excludes

create_pkg_meta_inf "$tmpDir" "$filter" "$packageGroup" "$packageName" "$packageVersion"
create_pkg_meta_inf "$tmpDir" "$jcrFilter" "$packageGroup" "$packageName" "$packageVersion"

pkg="$packageGroup/$packageName-$packageVersion.zip"

Expand All @@ -676,7 +683,7 @@ if [ $action == "put" ]; then
build_zip $tmpDir

if $verbose; then
unzip -l $zipfile | grep " jcr_root$filter"
unzip -l $zipfile | grep " jcr_root$fsFilter"
fi

if ! $force; then
Expand Down Expand Up @@ -734,25 +741,25 @@ elif [[ $action == "get" || $action == "diff" || $action == "status" ]]; then
right="REMOTE"
fi

do_diff "$filter"
do_diff "$fsFilter"
else
# status
do_diff_stat "$filter"
do_diff_stat "$fsFilter"
fi
popd > /dev/null

elif [ $action == "get" ]; then

if $verbose; then
unzip -l $zipfile | grep " jcr_root$filter"
unzip -l $zipfile | grep " jcr_root$fsFilter"
fi

if ! $force; then
prompt "download and overwrite locally?"
fi

# copy extracted content to local path
rsync -avq --delete --exclude-from=$excludes "$tmpDir/jcr_root/$filter" "$pathDirname"
rsync -avq --delete --exclude-from=$excludes "$tmpDir/jcr_root/$fsFilter" "$pathDirname"

# if git checkout is present, show git status for the path
if $verbose && git rev-parse --git-dir > /dev/null 2>&1; then
Expand Down