On Tue, Feb 23, 2016 at 5:25 AM, Mathias Nyman <mathias.nyman@xxxxxx> wrote:
> For recalling where a subtree came from; git-subtree operations 'add'
> and 'pull', when called with the <repository> parameter add this to the
> commit message:
> git-subtree-repo: <repo_url>
>
> Other operations that don't have the <repository> information, like
> 'merge' and 'add' without <repository>, are unchanged. Users with such a
> workflow will continue to be on their own with the --message parameter,
> if they'd like to record where the subtree came from.
I'm not a subtree user, so review comments below are superficial...
> Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxx>
> Based-on-patch-by: Nicola Paolucci <npaolucci@xxxxxxxxxxxxx>
> ---
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> @@ -335,18 +335,21 @@ add_msg()
> dir="$1"
> latest_old="$2"
> latest_new="$3"
> + repo="$4" # optional
> if [ -n "$message" ]; then
> commit_message="$message"
> else
> commit_message="Add '$dir/' from commit '$latest_new'"
> fi
> - cat <<-EOF
> - $commit_message
> -
> - git-subtree-dir: $dir
> - git-subtree-mainline: $latest_old
> - git-subtree-split: $latest_new
> - EOF
> + echo $commit_message
> + echo
> + echo git-subtree-dir: $dir
> + echo git-subtree-mainline: $latest_old
> + echo git-subtree-split: $latest_new
It's not clear why this code was changed to use a series of echo's in
place of the single cat. Although the net result is the same, this
appears to be mere code churn. If your intention was to make it
similar to how squash_msg() uses a series of echo's, then that might
make sense, however, rejoin_msg() uses the same single 'cat' as
add_msg(), so inconsistency remains. Thus, it's not clear what the
intention is.
> + if [ -n "$repo" ]; then
> + repo_url=$(get_repository_url "$repo")
> + echo "git-subtree-repo: $repo_url"
> + fi
> }
>
> add_squashed_msg()
> @@ -382,8 +385,9 @@ squash_msg()
> dir="$1"
> oldsub="$2"
> newsub="$3"
> + repo="$4" # optional
> newsub_short=$(git rev-parse --short "$newsub")
> -
> +
Okay, this change is removing an unnecessary tab. Perhaps the commit
message can say that the patch fixes a few whitespace inconsistencies
while touching nearby code.
More below...
> if [ -n "$oldsub" ]; then
> oldsub_short=$(git rev-parse --short "$oldsub")
> echo "Squashed '$dir/' changes from $oldsub_short..$newsub_short"
> @@ -397,6 +401,10 @@ squash_msg()
> echo
> echo "git-subtree-dir: $dir"
> echo "git-subtree-split: $newsub"
> + if [ -n "$repo" ]; then
> + repo_url=$(get_repository_url "$repo")
> + echo "git-subtree-repo: $repo_url"
> + fi
> }
>
> toptree_for_commit()
> @@ -440,12 +448,13 @@ new_squash_commit()
> old="$1"
> oldsub="$2"
> newsub="$3"
> + repo="$4" # optional
> tree=$(toptree_for_commit $newsub) || exit $?
> if [ -n "$old" ]; then
> - squash_msg "$dir" "$oldsub" "$newsub" |
> + squash_msg "$dir" "$oldsub" "$newsub" "$repo" |
> git commit-tree "$tree" -p "$old" || exit $?
> else
> - squash_msg "$dir" "" "$newsub" |
> + squash_msg "$dir" "" "$newsub" "$repo" |
> git commit-tree "$tree" || exit $?
> fi
> }
> @@ -517,6 +526,16 @@ ensure_valid_ref_format()
> die "'$1' does not look like a ref"
> }
>
> +get_repository_url()
> +{
> + repo=$1
> + repo_url=$(git config --get remote.$repo.url)
> + if [ -z "$repo_url" ]; then
> + repo_url=$repo
> + fi
> + echo $repo_url
> +}
> +
> cmd_add()
> {
> if [ -e "$dir" ]; then
> @@ -548,19 +567,18 @@ cmd_add()
> cmd_add_repository()
> {
> echo "git fetch" "$@"
> - repository=$1
> + repo=$1
Hmm, so 'repository' was present already but unused in this function,
and now you're using it. I suppose you renamed it 'repo' for
consistency with other 'repo' variable the patch introduces elsewhere.
> refspec=$2
> git fetch "$@" || exit $?
> revs=FETCH_HEAD
> - set -- $revs
> + set -- $revs $repo
> cmd_add_commit "$@"
The original code intentionally allowed passing a set of revs to
cmd_add_commit(), however, you've repurposed it (below) so that it
accepts one rev and an (optional) repo. Therefore, there doesn't seem
to be much value anymore to using "set --" when you could just do:
cmd_add_commit $revs $repo
Or am I missing something obvious?
(Of course, the original code unconditionally used "set --" even while
setting 'revs' to hardcoded FETCH_HEAD, so I suppose this isn't any
worse, but still...)
> }
>
> cmd_add_commit()
> {
> - revs=$(git rev-parse $default --revs-only "$@") || exit $?
> - set -- $revs
> - rev="$1"
> + rev=$(git rev-parse $default --revs-only "$1") || exit $?
An audit of call callers of cmd_add_commit() shows that it was only
ever invoked with a single rev, so this change to make it accept a
single rev plus an optional repo seems safe. However, I wonder if it
would make sense to keep the more flexible interface (in case future
callers might need the functionality) by passing repo in as the first
argument (using an empty string, for instance, for the optional bit)
and then taking all subsequent arguments as revs, but perhaps that's
overkill since it doesn't seem to care about revs other than the first
one.
cmd_merge() still goes through the "set --" dance which you've removed
here, even though an audit of all its callers pass in only a single
rev, so that seems inconsistent...
> + repo="$2" # optional
>
> debug "Adding $dir as '$rev'..."
> git read-tree --prefix="$dir" $rev || exit $?
> @@ -575,12 +593,12 @@ cmd_add_commit()
> fi
>
> if [ -n "$squash" ]; then
> - rev=$(new_squash_commit "" "" "$rev") || exit $?
> + rev=$(new_squash_commit "" "" "$rev" "$repo") || exit $?
> commit=$(add_squashed_msg "$rev" "$dir" |
> git commit-tree $tree $headp -p "$rev") || exit $?
> else
> revp=$(peel_committish "$rev") &&
> - commit=$(add_msg "$dir" "$headrev" "$rev" |
> + commit=$(add_msg "$dir" "$headrev" "$rev" "$repo" |
> git commit-tree $tree $headp -p "$revp") || exit $?
> fi
> git reset "$commit" || exit $?
> @@ -609,7 +627,8 @@ cmd_split()
> else
> unrevs="$(find_existing_splits "$dir" "$revs")"
> fi
> -
> +e
So, you're replacing a line containing a single tab with a line
containing a single 'e'. Seems fishy.
> + rev="$1"
> # We can't restrict rev-list to only $dir here, because some of our
> # parents have the $dir contents the root, and those won't match.
> # (and rev-list --follow doesn't seem to solve this)
> @@ -683,15 +702,20 @@ cmd_split()
>
> cmd_merge()
> {
> - revs=$(git rev-parse $default --revs-only "$@") || exit $?
> + revs=$(git rev-parse $default --revs-only "$1") || exit $?
Why is this variable still named 'revs' (plural) since you're only
passing in $1 now rather than $@?
> ensure_clean
> -
> set -- $revs
Do you still need this "set --" or am I missing something?
> if [ $# -ne 1 ]; then
> die "You must provide exactly one revision. Got: '$revs'"
> fi
Ditto with the conditional, considering that you only ever look at $1
now rather than $@.
> + do_merge "$@"
> +}
> +
> +do_merge()
> +{
> rev="$1"
> -
> + repo="$2" # optional
> +
> if [ -n "$squash" ]; then
> first_split="$(find_latest_squash "$dir")"
> if [ -z "$first_split" ]; then
> @@ -704,7 +728,7 @@ cmd_merge()
> say "Subtree is already at commit $rev."
> exit 0
> fi
> - new=$(new_squash_commit "$old" "$sub" "$rev") || exit $?
> + new=$(new_squash_commit "$old" "$sub" "$rev" "$repo") || exit $?
> debug "New squash commit: $new"
> rev="$new"
> fi
> @@ -730,12 +754,13 @@ cmd_pull()
> if [ $# -ne 2 ]; then
> die "You must provide <repository> <ref>"
> fi
> + repo=$1
> ensure_clean
> ensure_valid_ref_format "$2"
> git fetch "$@" || exit $?
> revs=FETCH_HEAD
> - set -- $revs
> - cmd_merge "$@"
> + set -- $revs $repo
> + do_merge "$@"
Same question as above. Is "set --" still buying you anything over just:
do_merge $revs $repo
?
> }
>
> cmd_push()
> --
> 2.7.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html