Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit
Hi Michael,
On 06/20/2014 03:41 PM, Michael Haggerty wrote:
> On 06/19/2014 05:28 AM, Fabian Ruch wrote:
>> 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. Teach `do_pick` to handle the option
>> `--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to
>> `pick_one` for the way options are parsed.
>>
>> `do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,
>> it cannot just forward `--edit` to the `cherry-pick` command line. The
>> assembled command line is executed within a command substitution for
>> controlling the verbosity of `rebase--interactive`. Passing `--edit`
>> would either hang the terminal or clutter the substituted command output
>> with control sequences. Execute the `reword` code from `do_next` instead
>> if the option `--edit` is specified. Adjust the fragment in two regards.
>> Firstly, discard the additional message which is printed if an error
>> occurs because
>>
>> Aborting commit due to empty commit message. (Duplicate Signed-off-by
>> lines.)
>> Could not amend commit after successfully picking 1234567... Some change
>>
>> is more readable than and contains (almost) the same information as
>>
>> Aborting commit due to empty commit message. (Duplicate Signed-off-by
>> lines.)
>> Could not amend commit after successfully picking 1234567... Some change
>> This is most likely due to an empty commit message, or the pre-commit
>> hook
>> failed. If the pre-commit hook failed, you may need to resolve the issue
>> before
>> you are able to reword the commit.
>>
>> (It is true that a hook might not output any diagnosis but the same
>> problem arises when using git-commit directly. git-rebase at least
>> prints a generic message saying that it failed to commit.) Secondly,
>> sneak in additional git-commit arguments:
>>
>> - `--allow-empty` is missing: `rebase--interactive` suddenly fails if
>>an empty commit is picked using `reword` instead of `pick`. The
>>whether a commit is empty or not is not changed by an altered log
>>message, so do as `pick` does. Add test.
>>
>> - `-n`: Hide the fact that we are committing several times by not
>>executing the pre-commit hook. Caveat: The altered log message is not
>>verified because `-n` also skips the commit-msg hook. Either the log
>>message verification must be included in the post-rewrite hook or
>>git-commit must support more fine-grained control over which hooks
>>are executed.
>>
>> - `-q`: Hide the fact that we are committing several times by not
>>printing the commit summary.
>
> It is preferable that each commit makes one logical change (though it
> must always be a self-contained change; i.e., the code should never be
> broken at the end of a commit). It would be clearer if you would split
> this commit into one refactoring commit (moving the handling of --edit
> to do_pick) plus one commit for each "git commit" option change and
> error message change. That way,
>
> * Each commit (and log message) becomes simpler, making it easier
> to review.
> * The changes can be discussed separately.
> * If there is an error, "git bisect" can help determine which of
> the changes is at fault.
Hmph, I neglected that totally here. I'm sorry. If it's all right, I
will reply with the five separate commits (refactoring, error message,
--allow-empty, -n, -q) to this email. The whole patch series is still
RFC and the combination of the five will be exactly this one, so that
shouldn't confuse or burden anyone.
>> Signed-off-by: Fabian Ruch
>> ---
>> git-rebase--interactive.sh| 52
>> ---
>> t/t3404-rebase-interactive.sh | 8 +++
>> 2 files changed, 52 insertions(+), 8 deletions(-)
>>
>> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
>> index ea5514e..fffdfa5 100644
>> --- a/git-rebase--interactive.sh
>> +++ b/git-rebase--interactive.sh
>> @@ -490,7 +490,42 @@ record_in_rewritten() {
>> esac
>> }
>>
>> +# Apply the changes introduced by the given commit to the current head.
>> +#
>> +# do_pick [--edit]
>> +#
>> +# Wrapper around git-cherry-pick.
>> +#
>> +#
>> +# The commit message title of . Used for information
>> +# purposes only.
>> +#
>> +#
>> +# The commit to cherry-pick.
>
> Unless there is a reason to do otherwise, please order the documentation
> to match the order that the do_pick arguments appear.
Ok.
The reason was to document the non-option arguments first and I ended up
documenting the arguments in reverse order to simply not abandon all
order. Having a look at several man-pages of git commands
Re: [RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit
On 06/19/2014 05:28 AM, Fabian Ruch wrote:
> 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. Teach `do_pick` to handle the option
> `--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to
> `pick_one` for the way options are parsed.
>
> `do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,
> it cannot just forward `--edit` to the `cherry-pick` command line. The
> assembled command line is executed within a command substitution for
> controlling the verbosity of `rebase--interactive`. Passing `--edit`
> would either hang the terminal or clutter the substituted command output
> with control sequences. Execute the `reword` code from `do_next` instead
> if the option `--edit` is specified. Adjust the fragment in two regards.
> Firstly, discard the additional message which is printed if an error
> occurs because
>
> Aborting commit due to empty commit message. (Duplicate Signed-off-by
> lines.)
> Could not amend commit after successfully picking 1234567... Some change
>
> is more readable than and contains (almost) the same information as
>
> Aborting commit due to empty commit message. (Duplicate Signed-off-by
> lines.)
> Could not amend commit after successfully picking 1234567... Some change
> This is most likely due to an empty commit message, or the pre-commit hook
> failed. If the pre-commit hook failed, you may need to resolve the issue
> before
> you are able to reword the commit.
>
> (It is true that a hook might not output any diagnosis but the same
> problem arises when using git-commit directly. git-rebase at least
> prints a generic message saying that it failed to commit.) Secondly,
> sneak in additional git-commit arguments:
>
> - `--allow-empty` is missing: `rebase--interactive` suddenly fails if
>an empty commit is picked using `reword` instead of `pick`. The
>whether a commit is empty or not is not changed by an altered log
>message, so do as `pick` does. Add test.
>
> - `-n`: Hide the fact that we are committing several times by not
>executing the pre-commit hook. Caveat: The altered log message is not
>verified because `-n` also skips the commit-msg hook. Either the log
>message verification must be included in the post-rewrite hook or
>git-commit must support more fine-grained control over which hooks
>are executed.
>
> - `-q`: Hide the fact that we are committing several times by not
>printing the commit summary.
It is preferable that each commit makes one logical change (though it
must always be a self-contained change; i.e., the code should never be
broken at the end of a commit). It would be clearer if you would split
this commit into one refactoring commit (moving the handling of --edit
to do_pick) plus one commit for each "git commit" option change and
error message change. That way,
* Each commit (and log message) becomes simpler, making it easier
to review.
* The changes can be discussed separately.
* If there is an error, "git bisect" can help determine which of
the changes is at fault.
> Signed-off-by: Fabian Ruch
> ---
> git-rebase--interactive.sh| 52
> ---
> t/t3404-rebase-interactive.sh | 8 +++
> 2 files changed, 52 insertions(+), 8 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ea5514e..fffdfa5 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -490,7 +490,42 @@ record_in_rewritten() {
> esac
> }
>
> +# Apply the changes introduced by the given commit to the current head.
> +#
> +# do_pick [--edit]
> +#
> +# Wrapper around git-cherry-pick.
> +#
> +#
> +# The commit message title of . Used for information
> +# purposes only.
> +#
> +#
> +# The commit to cherry-pick.
Unless there is a reason to do otherwise, please order the documentation
to match the order that the do_pick arguments appear.
> +#
> +# -e, --edit
> +# After picking , open an editor and let the user edit the
> +# commit message. The editor contents becomes the commit message of
> +# the new head.
> do_pick () {
> + edit=
> + while test $# -gt 0
> + do
> + case "$1" in
> + -e|--edit)
> + edit=y
> + ;;
> + -*)
> + warn "do_pick: ignored option -- $1"
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
> + test $# -ne 2 && die "do_pick: wrong number of arguments"
> +
> if
[RFC PATCH 2/7] rebase -i: Teach do_pick the option --edit
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. Teach `do_pick` to handle the option
`--edit` and reimplement `reword` in terms of `do_pick --edit`. Refer to
`pick_one` for the way options are parsed.
`do_pick` ought to act as a wrapper around `cherry-pick`. Unfortunately,
it cannot just forward `--edit` to the `cherry-pick` command line. The
assembled command line is executed within a command substitution for
controlling the verbosity of `rebase--interactive`. Passing `--edit`
would either hang the terminal or clutter the substituted command output
with control sequences. Execute the `reword` code from `do_next` instead
if the option `--edit` is specified. Adjust the fragment in two regards.
Firstly, discard the additional message which is printed if an error
occurs because
Aborting commit due to empty commit message. (Duplicate Signed-off-by
lines.)
Could not amend commit after successfully picking 1234567... Some change
is more readable than and contains (almost) the same information as
Aborting commit due to empty commit message. (Duplicate Signed-off-by
lines.)
Could not amend commit after successfully picking 1234567... Some change
This is most likely due to an empty commit message, or the pre-commit hook
failed. If the pre-commit hook failed, you may need to resolve the issue
before
you are able to reword the commit.
(It is true that a hook might not output any diagnosis but the same
problem arises when using git-commit directly. git-rebase at least
prints a generic message saying that it failed to commit.) Secondly,
sneak in additional git-commit arguments:
- `--allow-empty` is missing: `rebase--interactive` suddenly fails if
an empty commit is picked using `reword` instead of `pick`. The
whether a commit is empty or not is not changed by an altered log
message, so do as `pick` does. Add test.
- `-n`: Hide the fact that we are committing several times by not
executing the pre-commit hook. Caveat: The altered log message is not
verified because `-n` also skips the commit-msg hook. Either the log
message verification must be included in the post-rewrite hook or
git-commit must support more fine-grained control over which hooks
are executed.
- `-q`: Hide the fact that we are committing several times by not
printing the commit summary.
Signed-off-by: Fabian Ruch
---
git-rebase--interactive.sh| 52 ---
t/t3404-rebase-interactive.sh | 8 +++
2 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ea5514e..fffdfa5 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -490,7 +490,42 @@ record_in_rewritten() {
esac
}
+# Apply the changes introduced by the given commit to the current head.
+#
+# do_pick [--edit]
+#
+# Wrapper around git-cherry-pick.
+#
+#
+# The commit message title of . Used for information
+# purposes only.
+#
+#
+# The commit to cherry-pick.
+#
+# -e, --edit
+# After picking , open an editor and let the user edit the
+# commit message. The editor contents becomes the commit message of
+# the new head.
do_pick () {
+ edit=
+ while test $# -gt 0
+ do
+ case "$1" in
+ -e|--edit)
+ edit=y
+ ;;
+ -*)
+ warn "do_pick: ignored option -- $1"
+ ;;
+ *)
+ break
+ ;;
+ esac
+ shift
+ done
+ test $# -ne 2 && die "do_pick: wrong number of arguments"
+
if test "$(git rev-parse HEAD)" = "$squash_onto"
then
# Set the correct commit message and author info on the
@@ -512,6 +547,14 @@ do_pick () {
pick_one $1 ||
die_with_patch $1 "Could not apply $1... $2"
fi
+
+ if test -n "$edit"
+ then
+ git commit --allow-empty --amend --no-post-rewrite -n -q
${gpg_sign_opt:+"$gpg_sign_opt"} || {
+ warn "Could not amend commit after successfully picking
$1... $2"
+ exit_with_patch $1 1
+ }
+ fi
}
do_next () {
@@ -532,14 +575,7 @@ do_next () {
comment_for_reflog reword
mark_action_done
- do_pick $sha1 "$rest"
- git commit --amend --no-post-rewrite
${gpg_sign_opt:+"$gpg_sign_opt"} || {
- warn "Could not amend commit after successfully picking
$sha1...
