This code is a significant part of my runtime, so any performance improving observations would be appreciated.
I'm not sure the following will bring significant improvements, but if you want to squeeze out every bit of performance, I have a few ideas.
(optimizing for perf) Replace printf with interpolated strings
The main reasons to use printf:
- Print text within trailing newline (better than non-portable
echo -n ...)
- Use advanced formatting features as in the C-function with similar name
In many places in the code these features are not needed, either because the strings don't use special formatting features, or because the strings are only used in $(...) context, where the trailing newlines will be stripped anyway.
So I suggest to replace these:
treeData="$(printf '%s\n%s' "$treeData" "$(createSubtreeRec "$fullpath")")"
...
printf '040000 tree %s\t%s' $hash "$(basename "$path")"
...
printf '%s blob %s\t%s' $mode $hash "$(basename "$path")"
...
local entry="$(printf '%s %s %s\t%s' '040000' tree $hash $(basename "$path"))"
With:
treeData=$treeData$'\n'$(createSubtreeRec "$fullpath")
...
echo "040000 tree $hash"$'\t'"$(basename "$path")"
...
echo "$mode blob $hash"$'\t'"$(basename "$path")"
...
local entry="040000 tree $hash"$'\t'$(basename "$path")
You could embed tab characters in a string, it would make those lines easier to read:
echo "040000 tree $hash $(basename "$path")"
# ^
# tab
But I recommend to use $'\t' as above, because I don't like invisible characters :-)
(optimizing for perf) Replace basename "..." with substitution
Instead of basename "$path", you could write ${path##*/}.
This might be a bit faster because substitution is native Bash, while basename is a program, with the overhead of executing another process.
And I think the result should be equivalent, because the path separator / is not allowed in file and directory names.
It may be tempting to replace dirname "$path" with ${path%/*}. That requires a bit more careful thought, because the return value won't always be the same.
Use here-strings
Instead of echo ... | cmd, use cmd <<< "...".
(optimizing for perf) Rearrange operations to avoid redundant conditions
Consider this piece of code from your post, with minor optimizations to remove some noise:
if [ "$path" ]; then
entry=$(createSubtreeRec "$workdir/$path")
else
entry=$(createSubtreeRec "$workdir")
fi
if [ "$entry" ]; then
while [ "$path" ] && [ "$path" != "." ] ; do
hash=$(git mktree <<< "$entry")
path=$(dirname "$path")
entry="040000 tree $hash"$'\t'"${path##*/}"
done
echo "$hash"
fi
Can you spot some odd execution paths?
$path is either empty to begin with, or it will never be empty- In the last iteration of the
while loop, entry is computed and it will not be used
- If
$path is . to begin with, then createSubtreeRec "$workdir/." will get executed, and I doubt that's what you want.
To eliminate some redundant paths, sometimes it helps to first increase the redundancy. This is logically equivalent to the above code:
if [ "$path" ]; then
entry=$(createSubtreeRec "$workdir/$path")
if [ "$entry" ]; then
while [ "$path" ] && [ "$path" != "." ] ; do
hash=$(git mktree <<< "$entry")
path=$(dirname "$path")
entry="040000 tree $hash"$'\t'"${path##*/}"
done
echo "$hash"
fi
else
entry=$(createSubtreeRec "$workdir")
if [ "$entry" ]; then
while [ "$path" ] && [ "$path" != "." ] ; do
hash=$(git mktree <<< "$entry")
path=$(dirname "$path")
entry="040000 tree $hash"$'\t'"${path##*/}"
done
echo "$hash"
fi
fi
Now let's try to simplify it. In the else branch we know that $path is empty, and it cannot possibly change, so we can drop the while loop:
else
entry=$(createSubtreeRec "$workdir")
if [ "$entry" ]; then
echo "$hash"
fi
fi
And since $hash is only ever set in the while loop we dropped, we can drop that too:
else
entry=$(createSubtreeRec "$workdir")
if [ "$entry" ]; then
echo
fi
fi
At this point I wonder if you really wanted to print a blank line in this case, or if it's a bug. (I let you decide!)
Back to the if branch, we have this code now:
entry=$(createSubtreeRec "$workdir/$path")
if [ "$entry" ]; then
while [ "$path" ] && [ "$path" != "." ] ; do
hash=$(git mktree <<< "$entry")
path=$(dirname "$path")
entry="040000 tree $hash"$'\t'"${path##*/}"
done
echo "$hash"
fi
Since at this point $path is not empty and will never be, we can simplify the while statement, eliminating one unnecessary evaluation per iteration:
while [ "$path" != "." ] ; do
This is still not ideal, since this condition will always be true for the first time, and the last computation of entry will not be used. We can rearrange the loop to improve on both of these points:
while true; do
hash=$(git mktree <<< "$entry")
path=$(dirname "$path")
[ "$path" != "." ] || break
entry="040000 tree $hash"$'\t'"${path##*/}"
done
echo "$hash"
I doubt any of this would have a measurable performance difference except in extremely deep paths, I thought it was an interesting exercise to share.
Missed one important double-quoting
There is one spot that missed an important double-quoting:
local entry="$(printf '%s %s %s\t%s' '040000' tree $hash $(basename "$path"))"
If the basename of $path contains spaces, then the result of $(basename "$path") will be subject to word-splitting. Correctly quoted, the above line would be:
local entry=$(printf '%s %s %s\t%s' '040000' tree "$hash" "$(basename "$path")")
Note:
It's true that in this example we know that $hash is safe even without double-quoting. You never know how the script might evolve, and somebody might assign something unsafe to this variable. Also, when you assign something to a variable, you don't want to verify if all uses of the variable are safe enough. If you develop the habit to always double-quote variables used in command arguments, you can rest assured that your code is safe.
I dropped the double-quotes around the right-hand side of the assignment, because it's already safe like that. If the value contained literal spaces, that would have to be quoted though. If you feel safer to always write v="...", that's fine.
No need for input validation?
From the posted code it's not clear if createSubtree is guaranteed to get safe input. It seems to me that path is expected to be valid relative path from worktree, but I don't see how that is actually enforced.
You might want to add some sanity checks and fail fast (and with a big bang) when receiving something unexpected.
Minor things
I'm not a fan of assigning a default value and overwriting it like this:
mode=100644
if [ -x "$path" ]; then
mode=100755
fi
I would rather spell out an else.
Instead of duplicating the pattern %(mode) %(type) %(hash)\t%(name) in multiple places, it might be a good idea to create a helper function for it.
The type variable is assigned once and never used.
