On 2016-02-26 14:49-0500, Eric Sunshine wrote:
On Fri, Feb 26, 2016 at 3:28 AM, Mathias Nyman <m.nyman@xxxxxx> wrote:
On 2016-02-25 17:23-0500, Eric Sunshine wrote:
On Tue, Feb 23, 2016 at 5:25 AM, Mathias Nyman <mathias.nyman@xxxxxx>
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.
Not fixing any whitespace inconsistencies felt like the consistent way to
go in this commit.
Would be great to have some feedback on the core idea of this change,
before spending more time on it. Anyone?
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.
--
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