Re: [PATCH RFC v2 05/19] rebase -i: Implement reword in terms of do_pick

2014-08-04 Thread Fabian Ruch
Hi Matthieu,

thanks for taking a look at this patch series. I might have caused some
confusion by starting with version v1 again after removing the RFC tag.
The current reroll[1] is tagged with PATCH v1, not PATCH RFC v2.

I'm sorry if this is the reason why your reply appears on this sub-thread.

Your concerns below are of course noted.

   Fabian

[1] http://article.gmane.org/gmane.comp.version-control.git/254361

Matthieu Moy writes:
> Fabian Ruch  writes:
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -555,20 +555,7 @@ do_next () {
>>  comment_for_reflog reword
>>  
>>  mark_action_done
>> -do_pick $sha1 "$rest"
>> -# TODO: Work around the fact that git-commit lets us
>> -# disable either both the pre-commit and the commit-msg
>> -# hook or none. Disable the pre-commit hook because the
>> -# tree is left unchanged but run the commit-msg hook
>> -# from here because the log message is altered.
>> -git commit --allow-empty --amend --no-post-rewrite -n 
>> ${gpg_sign_opt:+"$gpg_sign_opt"} &&
>> -if test -x "$GIT_DIR"/hooks/commit-msg
>> -then
>> -"$GIT_DIR"/hooks/commit-msg 
>> "$GIT_DIR"/COMMIT_EDITMSG
>> -fi || {
>> -warn "Could not amend commit after successfully 
>> picking $sha1... $rest"
>> -exit_with_patch $sha1 1
>> -}
>> +do_pick --edit $sha1 "$rest"
> 
> I would have found this easier to review if squashed into the previous
> patch. My reaction reading the previous patch was "Uh, why duplicate
> code?", and reading this one "Ah, that's OK". A single patch doing both
> would have avoided the confusion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH RFC v2 05/19] rebase -i: Implement reword in terms of do_pick

2014-08-04 Thread Matthieu Moy
Fabian Ruch  writes:

> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -555,20 +555,7 @@ do_next () {
>   comment_for_reflog reword
>  
>   mark_action_done
> - do_pick $sha1 "$rest"
> - # TODO: Work around the fact that git-commit lets us
> - # disable either both the pre-commit and the commit-msg
> - # hook or none. Disable the pre-commit hook because the
> - # tree is left unchanged but run the commit-msg hook
> - # from here because the log message is altered.
> - git commit --allow-empty --amend --no-post-rewrite -n 
> ${gpg_sign_opt:+"$gpg_sign_opt"} &&
> - if test -x "$GIT_DIR"/hooks/commit-msg
> - then
> - "$GIT_DIR"/hooks/commit-msg 
> "$GIT_DIR"/COMMIT_EDITMSG
> - fi || {
> - warn "Could not amend commit after successfully 
> picking $sha1... $rest"
> - exit_with_patch $sha1 1
> - }
> + do_pick --edit $sha1 "$rest"

I would have found this easier to review if squashed into the previous
patch. My reaction reading the previous patch was "Uh, why duplicate
code?", and reading this one "Ah, that's OK". A single patch doing both
would have avoided the confusion.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH RFC v2 05/19] rebase -i: Implement reword in terms of do_pick

2014-07-02 Thread Fabian Ruch
The to-do list command `reword` replays a commit like `pick` but lets
the user also edit the commit's log message. If one thinks of `pick`
entries as scheduled `cherry-pick` command lines, then `reword`
becomes an alias for the command line `cherry-pick --edit`. The
porcelain `rebase--interactive` defines a function `do_pick` for
processing the `pick` entries on to-do lists. Reimplement `reword` in
terms of `do_pick --edit`.

If the user picks a commit using the to-do list line

reword fa1afe1 Some change

execute the command `do_pick --edit fa1afe1 "Some change"` which
carries out exactly the same steps as the case arm for `reword` in
`do_next` so far.

Signed-off-by: Fabian Ruch 
---
 git-rebase--interactive.sh | 15 +--
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index e06d9b6..4c875d5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -555,20 +555,7 @@ do_next () {
comment_for_reflog reword
 
mark_action_done
-   do_pick $sha1 "$rest"
-   # TODO: Work around the fact that git-commit lets us
-   # disable either both the pre-commit and the commit-msg
-   # hook or none. Disable the pre-commit hook because the
-   # tree is left unchanged but run the commit-msg hook
-   # from here because the log message is altered.
-   git commit --allow-empty --amend --no-post-rewrite -n 
${gpg_sign_opt:+"$gpg_sign_opt"} &&
-   if test -x "$GIT_DIR"/hooks/commit-msg
-   then
-   "$GIT_DIR"/hooks/commit-msg 
"$GIT_DIR"/COMMIT_EDITMSG
-   fi || {
-   warn "Could not amend commit after successfully 
picking $sha1... $rest"
-   exit_with_patch $sha1 1
-   }
+   do_pick --edit $sha1 "$rest"
record_in_rewritten $sha1
;;
edit|e)
-- 
2.0.0

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html