Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
Hi Michael, On 08/13/2014 02:47 PM, Michael Haggerty wrote: > On 08/07/2014 01:59 AM, Fabian Ruch wrote: >> pick and reword are atomic to-do list commands in the sense that they >> open a new task which is closed after the respective command is >> completed. squash and fixup are not atomic. They create a new task >> which is not completed until the last squash or fixup is processed. > > I don't understand the distinction that you are attempting to draw > between "atomic" and "non-atomic" commands. For example, in the > following command list: > > pick 111 > squash 222 > fixup 333 > > the "pick" command doesn't seem very atomic, because the *end* result of > the three commands is a single commit that is affected by all three > commands. Right, when I wrote the commit message I was thinking in abstract terms so I implicitly thought of your example as a (single) squash/fixup command. Now it has become obvious that I wasn't very thorough with the implementation part. The git-rebase implementation is oblivious to the context when it processes 'pick' lines and your example shows how 'pick' lines can be part of squash/fixup command context. In conclusion, I intended to keep options disabled for squash/fixup commands but failed to do so because I neglected that a 'pick' line can initiate a squash/fixup command. > Furthermore, if we change the example to > > pick 111 > squash --reset-author 222 > fixup --signoff 333 > > then isn't it clear that the user's intention was to apply both options, > "--reset-author" and "--signoff", to the resulting commit? This seems to suggest an interpretation of todo lists similar to what I was thinking of when writing the commit message, that is one in which pick is not oblivious to the neighbouring commands. It might be a problem that it forbids the (admittedly improbable) use case where --reset-author is used to rewrite the authorship to something recent and fixup to have an even more recent committership. To reconcile this kind of vertical interpretation with the horizontal specification of options one could introduce a todo list command taking the list of commits to be squashed as an argument. However, that seems to make it difficult to obtain the squash behaviour for some commits and the fixup behaviour for others that are part of the same chain. The alternative interpretation of todo lists as simplified batch scripts for git commands would allow the intended behaviour (--reset-author and --signoff applied to the resulting commit), not restrict the user relatively to what she can already do on the command line and give actually different meanings to the syntactically different todo lists pick 111 squash --reset-author 222 fixup --signoff 333 and pick 111 squash --signoff 222 fixup --reset-author 333 , which would be treated identically by an implementation that collects the options. The current meaning of squash/fixup seems to be valid in the batch interpretation. > In other > words, it seems to me that any options on such a chain of lines should > be collected and applied to the final commit as a whole. To summarise, I think line options might be confusing if we interpret pick 111 squash --reset-author 222 fixup --signoff 333 as combine the changes of 111, 222, 333 concatenate the messages of 111 and 222 edit the message reset the authorship to the committership add a Signed-off-by: line and not as pick 111 pick -n 222 commit amend --reset-author -m $squash_msg pick -n 333 commit amend --signoff --edit . Thanks for pointing me at these issues. "Atomic" and "non-atomic" are really poorly-chosen terminology and the squash-initiating 'pick' might not be implemented correctly. Fabian -- 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 v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
Hi, Michael Haggerty writes: > On 08/07/2014 01:59 AM, Fabian Ruch wrote: >> Lift the general unknown option blockade for the pick and reword >> commands. If `do_cmd` comes across one of the options `--signoff` and >> `--reset-author` while parsing a to-do entry and the scheduled >> command is either `pick` or `reword`, relay the option to `do_pick`. > > The new user-exposed options should be documented in the git-rebase(1) > manpage and probably also in the help text that is appended to every > "rebase -i" todo list. The next reroll will add the following paragraph to the git-rebase man page right after the introduction of the 'reword' command in the section "INTERACTIVE MODE": > The commands "pick" and "reword" understand some well-known options. > To add a Signed-off-by line at the end of the commit message, pass > the `--signoff` option. The authorship can be renewed by specifying > the `--reset-author` option. For instance, before you decide to > publish a heavily edited commit you might want to reset the > authorship and add your signature. You can do so on a per line basis: > > --- > pick deadbee The oneline of this commit > pick --reset-author --signoff fa1afe1 The oneline of the next commit > ... > --- By saying "heavily edited commit" I tried to describe a commit that has been amended, reworded and reordered in such a way that the actual author information has become meaningless. The help text at the end of every to-do list would look like this: > Commands: > p, pick = use commit > r, reword = use commit, but edit the commit message > e, edit = use commit, but stop for amending > s, squash = use commit, but meld into previous commit > f, fixup = like "squash", but discard this commit's log message > x, exec = run command (the rest of the line) using shell > > Options: > [pick | reword] --signoff = add a Signed-off-by line > [pick | reword] --reset-author = renew authorship > > These lines can be re-ordered; they are executed from top to bottom. > > If you remove a line here THAT COMMIT WILL BE LOST. New about this is the "Options" headline. Fabian -- 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 v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
On 08/07/2014 01:59 AM, Fabian Ruch wrote: > pick and reword are atomic to-do list commands in the sense that they > open a new task which is closed after the respective command is > completed. squash and fixup are not atomic. They create a new task > which is not completed until the last squash or fixup is processed. I don't understand the distinction that you are attempting to draw between "atomic" and "non-atomic" commands. For example, in the following command list: pick 111 squash 222 fixup 333 the "pick" command doesn't seem very atomic, because the *end* result of the three commands is a single commit that is affected by all three commands. Furthermore, if we change the example to pick 111 squash --reset-author 222 fixup --signoff 333 then isn't it clear that the user's intention was to apply both options, "--reset-author" and "--signoff", to the resulting commit? In other words, it seems to me that any options on such a chain of lines should be collected and applied to the final commit as a whole. > Lift the general unknown option blockade for the pick and reword > commands. If `do_cmd` comes across one of the options `--signoff` and > `--reset-author` while parsing a to-do entry and the scheduled > command is either `pick` or `reword`, relay the option to `do_pick`. The new user-exposed options should be documented in the git-rebase(1) manpage and probably also in the help text that is appended to every "rebase -i" todo list. > The `do_pick` options `--gpg-sign` and `--file` are not yet supported > because `do_cmd` cannot handle option arguments and options with > spaces at the moment. It is true that edit is one of the atomic > commands but it displays hash information when the rebase is stopped > and some options rewrite the picked commit which alters that > information. squash and fixup still do not accept user options as the > interplay of `--reset-author` and the author script are yet to be > determined. > [...] Michael -- Michael Haggerty mhag...@alum.mit.edu -- 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 v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
Hi Thomas,
Thomas Rast writes:
> Fabian Ruch writes:
>> @@ -634,21 +644,24 @@ do_replay () {
>> comment_for_reflog pick
>>
>> mark_action_done
>> -do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
>> $rest"
>> +eval do_pick $opts $sha1 \
>> +|| die_with_patch $sha1 "Could not apply $sha1... $rest"
>
> You had me a little puzzled at the switch to 'eval' here. That is
> necessary to match the quoting added in 20/23, not for any change in
> this commit. This commit is simply the first one to trigger this.
This patch switches to 'eval' here because it is the first time
'opts' occurs. However, I agree that it might be confusing that
'opts' wasn't already added to the 'do_pick' lines by 20/23. By
"trigger" you mean that this commit is the first to actually fill
'opts' with contents? I will move these changes to 20/23 then.
> Also, are you sure $sha1 does not require quoting through an eval?
At least if we can assume that it is really a SHA-1 object name. As
such it does not contain characters interpreted by the shell, like
backslashes, quotes or whitespace.
> Please add tests to this patch.
The ones I had in mind are attached below the scissors line. The
current reroll fails the authorship checks and the 'git rebase
--continue' test cases. As the necessary changes would obfuscate this
sub-thread, they will be included in the next reroll.
Fabian
-- >8 --
diff --git a/t/t3427-rebase-line-options.sh b/t/t3427-rebase-line-options.sh
index 5881162..a5a9e66 100755
--- a/t/t3427-rebase-line-options.sh
+++ b/t/t3427-rebase-line-options.sh
@@ -6,10 +6,32 @@ test_description='git rebase -i with line options'
. "$TEST_DIRECTORY"/lib-rebase.sh
+commit_message () {
+ git cat-file commit "$1" | sed '1,/^$/d'
+}
+
+commit_authorship () {
+ git cat-file commit "$1" | sed -n '/^$/q;/^author /p'
+}
+
+authorship () {
+ echo "author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE"
+}
+
+test_diff_file () {
+ if cmp "$1" "$2" >/dev/null
+ then
+ echo "'$1' and '$2' are the same"
+ return 1
+ fi
+}
+
test_expect_success 'Set up repository' '
test_commit Initial &&
test_commit Commit1 &&
- test_commit Commit2
+ test_commit Commit2 &&
+ git checkout -b branch Commit1 &&
+ test_commit Commit2_ Commit2.t
'
test_expect_success 'Unknown option' '
@@ -23,4 +45,137 @@ test_expect_success 'Unknown option' '
git rebase --continue
'
+test_msg_author () {
+ set_fake_editor &&
+ FAKE_LINES="1 $1 2" git rebase -i HEAD~2 &&
+ commit_message HEAD >actual.msg &&
+ commit_authorship HEAD >actual.author &&
+ test_cmp expected.msg actual.msg &&
+ test_cmp expected.author actual.author
+}
+
+test_msg_author_misspelled () {
+ set_cat_todo_editor &&
+ test_must_fail git rebase -i HEAD^ >todo &&
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="1 $1-misspelled 2" git rebase -i HEAD~2
&&
+ set_fixed_todo_editor "$(pwd)"/todo &&
+ FAKE_LINES="$1 1" git rebase --edit-todo &&
+ git rebase --continue &&
+ commit_message HEAD >actual.msg &&
+ commit_authorship HEAD >actual.author &&
+ test_cmp expected.msg actual.msg &&
+ test_cmp expected.author actual.author
+}
+
+test_msg_author_conflicted () {
+ set_fake_editor &&
+ test_must_fail env FAKE_LINES="$1 1" git rebase -i master &&
+ git checkout --theirs Commit2.t &&
+ git add Commit2.t &&
+ git rebase --continue &&
+ commit_message HEAD >actual.msg &&
+ commit_authorship HEAD >actual.author &&
+ test_cmp expected.msg actual.msg &&
+ test_cmp expected.author actual.author
+}
+
+test_expect_success 'Misspelled pick --signoff' '
+ git checkout -b misspelled-pick--signoff master &&
+ cat >expected.msg <<-EOF &&
+ $(commit_message HEAD)
+
+ Signed-off-by: C O Mitter
+ EOF
+ commit_authorship HEAD >expected.author &&
+ test_msg_author_misspelled pick_--signoff
+'
+
+test_expect_success 'Conflicted pick --signoff' '
+ git checkout -b conflicted-pick--signoff branch &&
+ cat >expected.msg <<-EOF &&
+ $(commit_message HEAD)
+
+ Signed-off-by: C O Mitter
+ EOF
+ commit_authorship HEAD >expected.author &&
+ test_msg_author_conflicted pick_--signoff
+'
+
+test_expect_success 'pick --signoff' '
+ git checkout -b pick--signoff master &&
+ cat >expected.msg <<-EOF &&
+ $(commit_message HEAD)
+
+ Signed-off-by: C O Mitter
+ EOF
+ commit_authorship HEAD >expected.author &&
+ test_msg_author pick_--signoff
+'
+
+test_expect_success 'reword --signoff' '
+ git checkout -b reword--signoff master &&
+ cat >expected.msg <<-EOF &&
+ $(commit_message HEAD)
+
+ Signed-off-by: C O Mitter
+ EOF
+
Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
Fabian Ruch writes:
> @@ -634,21 +644,24 @@ do_replay () {
> comment_for_reflog pick
>
> mark_action_done
> - do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
> $rest"
> + eval do_pick $opts $sha1 \
> + || die_with_patch $sha1 "Could not apply $sha1... $rest"
You had me a little puzzled at the switch to 'eval' here. That is
necessary to match the quoting added in 20/23, not for any change in
this commit. This commit is simply the first one to trigger this.
Also, are you sure $sha1 does not require quoting through an eval?
Please add tests to this patch.
--
Thomas Rast
t...@thomasrast.ch
--
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 v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword
pick and reword are atomic to-do list commands in the sense that they
open a new task which is closed after the respective command is
completed. squash and fixup are not atomic. They create a new task
which is not completed until the last squash or fixup is processed.
Lift the general unknown option blockade for the pick and reword
commands. If `do_cmd` comes across one of the options `--signoff` and
`--reset-author` while parsing a to-do entry and the scheduled
command is either `pick` or `reword`, relay the option to `do_pick`.
The `do_pick` options `--gpg-sign` and `--file` are not yet supported
because `do_cmd` cannot handle option arguments and options with
spaces at the moment. It is true that edit is one of the atomic
commands but it displays hash information when the rebase is stopped
and some options rewrite the picked commit which alters that
information. squash and fixup still do not accept user options as the
interplay of `--reset-author` and the author script are yet to be
determined.
Signed-off-by: Fabian Ruch
---
git-rebase--interactive.sh | 19 ---
1 file changed, 16 insertions(+), 3 deletions(-)
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a22459f..4c05734 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -614,6 +614,16 @@ do_replay () {
while test $# -gt 0
do
case "$1" in
+ --signoff|--reset-author)
+ case "$command" in
+ pick|reword)
+ ;;
+ *)
+ warn "Unsupported option: $1"
+ command=unknown
+ ;;
+ esac
+ ;;
-*)
warn "Unknown option: $1"
command=unknown
@@ -634,21 +644,24 @@ do_replay () {
comment_for_reflog pick
mark_action_done
- do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
$rest"
+ eval do_pick $opts $sha1 \
+ || die_with_patch $sha1 "Could not apply $sha1... $rest"
record_in_rewritten $sha1
;;
reword|r)
comment_for_reflog reword
mark_action_done
- do_pick --edit $sha1 || die_with_patch $sha1 "Could not apply
$sha1... $rest"
+ eval do_pick --edit $opts $sha1 \
+ || die_with_patch $sha1 "Could not apply $sha1... $rest"
record_in_rewritten $sha1
;;
edit|e)
comment_for_reflog edit
mark_action_done
- do_pick $sha1 || die_with_patch $sha1 "Could not apply $sha1...
$rest"
+ eval do_pick $opts $sha1 \
+ || die_with_patch $sha1 "Could not apply $sha1... $rest"
warn "Stopped at $sha1... $rest"
exit_with_patch $sha1 0
;;
--
2.0.1
--
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
