Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword

[Fabian Ruch]

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

[Fabian Ruch]

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

[Michael Haggerty]

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

[Fabian Ruch]

Hi Thomas,

Thomas Rast writes:
 Fabian Ruch baf...@gmail.com 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 commit...@example.com
+   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 commit...@example.com
+   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 commit...@example.com
+   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 commit...@example.com
+   EOF
+   commit_authorship HEAD expected.author 
+   

Re: [PATCH v2 23/23] rebase -i: enable options --signoff, --reset-author for pick, reword

[Thomas Rast]

Fabian Ruch baf...@gmail.com 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

[Fabian Ruch]

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 baf...@gmail.com
---
 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