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

[Mathias Nyman <m.nyman@iki.fi>]

On 2016-02-25 17:23-0500, Eric Sunshine wrote:
>On Tue, Feb 23, 2016 at 5:25 AM, Mathias Nyman <mathias.nyman@iki.fi> 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...
>

Thank you for reviewing; will send PATCH v2 shortly.

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

Using a mixutre of heredoc and echo felt messy. But I'll change it
back to heredoc here, and through out the commit aim for near-zero
refactoring.

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

Will undo the whitespace fixing.

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

Yes.

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

Will leave this as is; your suggestion would mean refactoring the 'set
--' quirks, which I tried not to do.

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

I think it makes sense to refactor the general 'set --' dance in
git-subtree.sh all together.

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

I'll readd this 'set --' dance here for consistency.

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

Great typo find!

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

Because technically the result can still be more then one rev I guess.
Consider 'git rev-parse HEAD~1..HEAD', which would return two hashes.

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

This will handle the case where 'git rev-parse' caught more than one
hash earlier.

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

No. But I will not deviate from the function parameter passing method
('set --') used throughout git-subtree.sh in this commit. I do think
parameter passing in git-subtree.sh deserves a separate
no-functional-changes refactoring commit though.

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