Re: [PATCH] contrib/subtree: add repo url to commit messages

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Feb 25, 2016, 22:23 UTC
Message-ID: <CAPig+cSwQmbvZYbk3T-XYDfMYaMdJ=bFbDwEUtaR121pBrYJOQ@mail.gmail.com>
In-Reply-To: <20160223102559.GA18668@iki.fi>

On Tue, Feb 23, 2016 at 5:25 AM, Mathias Nyman <mathias.nyman@iki.fi> wrote:

Show 9 quoted lines

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

Show 26 quoted lines

> Signed-off-by: Mathias Nyman <mathias.nyman@iki.fi>
> Based-on-patch-by: Nicola Paolucci <npaolucci@atlassian.com>
> ---
> 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.

Show 15 quoted lines

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

Show 53 quoted lines

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

Show 6 quoted lines

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

Show 8 quoted lines

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

Show 25 quoted lines

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

Show 10 quoted lines

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

Show 34 quoted lines

> +       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

Show 5 quoted lines

>  }
>
>  cmd_push()
> --
> 2.7.1