| Message ID | 20210423194230.1388945-13-lukeshu@lukeshu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | subtree: clean up, improve UX | expand |
On Fri, 23 Apr 2021 13:42:12 -0600,
Luke Shumaker wrote:
> "Ignore space change" is probably helpful when viewing this diff.
For mailing-list reading, the `git show -w` is
---
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
index 868e18b9a1..d1ed7f9a6c 100755
--- a/contrib/subtree/git-subtree.sh
+++ b/contrib/subtree/git-subtree.sh
@@ -4,10 +4,7 @@
#
# Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com>
#
-if test $# -eq 0
-then
- set -- -h
-fi
+
OPTS_SPEC="\
git subtree add --prefix=<prefix> <commit>
git subtree add --prefix=<prefix> <repository> <ref>
@@ -30,12 +27,8 @@ rejoin merge the new branch back into HEAD
options for 'add', 'merge', and 'pull'
squash merge subtree changes as a single commit
"
-eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
PATH=$PATH:$(git --exec-path)
-. git-sh-setup
-
-require_work_tree
quiet=
branch=
@@ -84,6 +77,15 @@ ensure_single_rev () {
fi
}
+main () {
+ if test $# -eq 0
+ then
+ set -- -h
+ fi
+ eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
+ . git-sh-setup
+ require_work_tree
+
while test $# -gt 0
do
opt="$1"
@@ -205,6 +207,9 @@ debug "dir: {$dir}"
debug "opts: {$*}"
debug
+ "cmd_$command" "$@"
+}
+
cache_setup () {
cachedir="$GIT_DIR/subtree-cache/$$"
rm -rf "$cachedir" ||
@@ -898,4 +903,4 @@ cmd_push () {
fi
}
-"cmd_$command" "$@"
+main "$@"
On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > Shove all of the loose code inside of a main() function. > > "Ignore space change" is probably helpful when viewing this diff. > > Signed-off-by: Luke Shumaker <lukeshu@datawire.io> What is the purpose of this change? Does some subsequent commit depend upon this or is it just a personal preference? The commit message explains the "what" of the change but not the "why".
On Fri, 23 Apr 2021 14:23:18 -0600, Eric Sunshine wrote: > > On Fri, Apr 23, 2021 at 3:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > > Shove all of the loose code inside of a main() function. > > > > "Ignore space change" is probably helpful when viewing this diff. > > > > Signed-off-by: Luke Shumaker <lukeshu@datawire.io> > > What is the purpose of this change? Does some subsequent commit depend > upon this or is it just a personal preference? The commit message > explains the "what" of the change but not the "why". Dropping the commit would surely cause me much trouble with rebasing both the subsequent commits in this patchset and the commits I haven't yet submitted. But I don't think they "depend" on it. I guess it is personal preference... one that I've developed from years of maintaining Bash scripts. It's in a nearby part of my brain to "avoid global variables". It's related to my notion that the reason most people think shell scripts are so terrible is that most people don't treat it like a real programming language and don't apply normal programming best practices; that not littering code around outside of a function is part of treating it like a real language (at least once the program grows beyond a certain size, which git-subtree surely has). It's probably related to the Python idiom if __name__ == "__main__": main() that is often seen at the bottom of Python scripts. In this specific case, it's also moving the `set -- -h`, the `git rev-parse --parseopt`, and the `. git-sh-setup` to be closer to all the rest of the argument parsing, which is a readability win on its own, IMO.
On Fri, Apr 23, 2021 at 6:43 PM Luke Shumaker <lukeshu@lukeshu.com> wrote: > On Fri, 23 Apr 2021 14:23:18 -0600, Eric Sunshine wrote: > > What is the purpose of this change? Does some subsequent commit depend > > upon this or is it just a personal preference? The commit message > > explains the "what" of the change but not the "why". > > Dropping the commit would surely cause me much trouble with rebasing > both the subsequent commits in this patchset and the commits I haven't > yet submitted. But I don't think they "depend" on it. > > I guess it is personal preference... [...] That's fine. I wasn't suggesting removing the commit but was interested in having the commit message explain the reason for the change, which is often the most important part of the commit message to convince readers (reviewers) that the patch isn't just needless noise. Obviously, if changes in subsequent patches become easier by making this change, then that's easier to explain than something which is mere personal preference -- which isn't to say that such preference isn't important, especially if you're going to be living in the code for a while. > In this specific case, it's also moving the `set -- -h`, the `git > rev-parse --parseopt`, and the `. git-sh-setup` to be closer to all > the rest of the argument parsing, which is a readability win on its > own, IMO. If you happen to re-roll, perhaps update the commit message to include a small bit of your explanation (which I've mostly snipped here). The last bit about moving `set -- -h` and whatnot closer to the rest of the argument parsing -- basically improving encapsulation -- may be justification enough for readers.
On Fri, 23 Apr 2021 17:11:11 -0600, Eric Sunshine wrote: > If you happen to re-roll, perhaps update the commit message to include > a small bit of your explanation (which I've mostly snipped here). The > last bit about moving `set -- -h` and whatnot closer to the rest of > the argument parsing -- basically improving encapsulation -- may be > justification enough for readers. Whenever I end up explaining something on-list, I assume that I should update either a comment or a commit-message when I re-roll :)
diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh index 868e18b9a1..d1ed7f9a6c 100755 --- a/contrib/subtree/git-subtree.sh +++ b/contrib/subtree/git-subtree.sh @@ -4,10 +4,7 @@ # # Copyright (C) 2009 Avery Pennarun <apenwarr@gmail.com> # -if test $# -eq 0 -then - set -- -h -fi + OPTS_SPEC="\ git subtree add --prefix=<prefix> <commit> git subtree add --prefix=<prefix> <repository> <ref> @@ -30,12 +27,8 @@ rejoin merge the new branch back into HEAD options for 'add', 'merge', and 'pull' squash merge subtree changes as a single commit " -eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)" PATH=$PATH:$(git --exec-path) -. git-sh-setup - -require_work_tree quiet= branch= @@ -84,126 +77,138 @@ ensure_single_rev () { fi } -while test $# -gt 0 -do - opt="$1" - shift +main () { + if test $# -eq 0 + then + set -- -h + fi + eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)" + . git-sh-setup + require_work_tree - case "$opt" in - -q) - quiet=1 - ;; - -d) - debug=1 - ;; - --annotate) - annotate="$1" - shift - ;; - --no-annotate) - annotate= - ;; - -b) - branch="$1" - shift - ;; - -P) - prefix="${1%/}" - shift - ;; - -m) - message="$1" - shift - ;; - --no-prefix) - prefix= - ;; - --onto) - onto="$1" + while test $# -gt 0 + do + opt="$1" shift + + case "$opt" in + -q) + quiet=1 + ;; + -d) + debug=1 + ;; + --annotate) + annotate="$1" + shift + ;; + --no-annotate) + annotate= + ;; + -b) + branch="$1" + shift + ;; + -P) + prefix="${1%/}" + shift + ;; + -m) + message="$1" + shift + ;; + --no-prefix) + prefix= + ;; + --onto) + onto="$1" + shift + ;; + --no-onto) + onto= + ;; + --rejoin) + rejoin=1 + ;; + --no-rejoin) + rejoin= + ;; + --ignore-joins) + ignore_joins=1 + ;; + --no-ignore-joins) + ignore_joins= + ;; + --squash) + squash=1 + ;; + --no-squash) + squash= + ;; + --) + break + ;; + *) + die "Unexpected option: $opt" + ;; + esac + done + + command="$1" + shift + + case "$command" in + add|merge|pull) + default= ;; - --no-onto) - onto= - ;; - --rejoin) - rejoin=1 - ;; - --no-rejoin) - rejoin= - ;; - --ignore-joins) - ignore_joins=1 - ;; - --no-ignore-joins) - ignore_joins= - ;; - --squash) - squash=1 + split|push) + default="--default HEAD" ;; - --no-squash) - squash= + *) + die "Unknown command '$command'" ;; - --) - break + esac + + if test -z "$prefix" + then + die "You must provide the --prefix option." + fi + + case "$command" in + add) + test -e "$prefix" && + die "prefix '$prefix' already exists." ;; *) - die "Unexpected option: $opt" + test -e "$prefix" || + die "'$prefix' does not exist; use 'git subtree add'" ;; esac -done - -command="$1" -shift - -case "$command" in -add|merge|pull) - default= - ;; -split|push) - default="--default HEAD" - ;; -*) - die "Unknown command '$command'" - ;; -esac - -if test -z "$prefix" -then - die "You must provide the --prefix option." -fi - -case "$command" in -add) - test -e "$prefix" && - die "prefix '$prefix' already exists." - ;; -*) - test -e "$prefix" || - die "'$prefix' does not exist; use 'git subtree add'" - ;; -esac - -dir="$(dirname "$prefix/.")" - -if test "$command" != "pull" && - test "$command" != "add" && - test "$command" != "push" -then - revs=$(git rev-parse $default --revs-only "$@") || exit $? - dirs=$(git rev-parse --no-revs --no-flags "$@") || exit $? - ensure_single_rev $revs - if test -n "$dirs" - then - die "Error: Use --prefix instead of bare filenames." - fi -fi - -debug "command: {$command}" -debug "quiet: {$quiet}" -debug "revs: {$revs}" -debug "dir: {$dir}" -debug "opts: {$*}" -debug + + dir="$(dirname "$prefix/.")" + + if test "$command" != "pull" && + test "$command" != "add" && + test "$command" != "push" + then + revs=$(git rev-parse $default --revs-only "$@") || exit $? + dirs=$(git rev-parse --no-revs --no-flags "$@") || exit $? + ensure_single_rev $revs + if test -n "$dirs" + then + die "Error: Use --prefix instead of bare filenames." + fi + fi + + debug "command: {$command}" + debug "quiet: {$quiet}" + debug "revs: {$revs}" + debug "dir: {$dir}" + debug "opts: {$*}" + debug + + "cmd_$command" "$@" +} cache_setup () { cachedir="$GIT_DIR/subtree-cache/$$" @@ -898,4 +903,4 @@ cmd_push () { fi } -"cmd_$command" "$@" +main "$@"