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

>On Fri, Feb 26, 2016 at 3:28 AM, Mathias Nyman <m.nyman@iki.fi> wrote:
>> 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:
>>>> -       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.
>
>An alternative would be to have a preparatory patch which unifies the
>heredoc vs. echo issue across add_msg(), squash_msg(), rejoin_msg(),
>but I wouldn't insist upon it (that's just more work for you). Leaving
>this bit alone is a reasonable choice.
>
>>>> +       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.
>>
>> Will undo the whitespace fixing.
>
>Oh, I wasn't insisting that you should undo the whitespace fix.
>Typically, you'd make such fixes in a preparatory cleanup patch, but
>since there are only two cases here that you've fixed, it probably
>wouldn't hurt to retain them (if that's all there are in the file).
>The reason I suggested mentioning the whitespace fixes in the commit
>message is to let the reviewer know that they weren't cases of you
>accidentally making unwanted whitespace changes (like inserting tabs
>rather than removing them). As it was, as a reviewer, I had to go
>through extra effort to determine whether you had made a fix or had
>accidentally botched something.
>

>>>>  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.
>
>Okay, so I was missing something obvious.