Re: [PATCH v2] commit -c/-C/--amend: reset timestamp and authorship to committer with --reset-author

Erick Mattos <erick.mattos@gmail.com> writes:

Sorry, but I cannot help feeling a bit frustrated and mildly irritated.

I had an impression that we have already established that setting the author with --author="Somebody Else <s@b.e>" and committing with the current time does not make much sense from the workflow point of view long time ago in this thread.

The mail transport might have mangled the name, and when using --amend (or read-tree followed by commit -c), it is handy to fix the mangled name by using --author, but in such a case you would actively want to keep the timestamp obtained from the e-mail via either --amend or -c.

But allowing this combination, even though it might not make much sense, is just giving extra length to the rope, so it may not be such a big deal.

I didn't feel motivated enough to read the whole thing while other patches are in my inbox, so I instead ran diff between the previous one (without my suggestion today) and this round.

I see that you fixed a lot of grammar in the log message of my earlier suggestion, all of which looked very good. Also you added a check in the program to make sure that --renew is given only when -C/-c/--amend is given, which is also good. Neither of our set of tests checks this condition, though. IOW, we would need to add something like this at the end of my version (adjust to --reset-author for your version):

    test_expect_success '--mine should be rejected without -c/-C/--amend' '
            git checkout Initial &&
            echo "Test 7" >>foo &&
            test_tick &&
            test_must_fail git commit -a --mine -m done
    '

I am not sure why you insist to use your version of test script and keep changing it, though. It looks a lot worse even only after reviewing its early part.

 - author_id runs an extra grep that is unnecessary.  The separation of
   _id and _timestamp are unnecessary if you checked against an expected
   author ident and timestamp as a single string, i.e.

   FRIGATE='Frigate <flying@over.world>' ;# do this only once at the beginning
   ...
   git commit -C HEAD --reset-author --author="$FRIGATE" &&
   echo "author $FRIGATE $GIT_AUTHOR_TIME" >expect &&
   author_header HEAD >actual &&
   test_cmp expect actual

   This becomes irrelevant if we don't support mixing --renew and
   --author, of course.

 - message_body() now has a backslash whose sole purpose is to be an
   eyesore.

 - initiate_test() does not string the commands together with &&

I might change my mind after I take a break, review others' patches, and spend some time on my own hacking on other topics before revisiting this patch, but at this point I find that reviewing newer rounds of this series has rather quickly diminishing value, and more time is being spent on teaching shell scripting to you rather than on polishing the end result.

Sorry, but I cannot help feeling a bit frustrated and mildly irritated. Time to take a break and attend other topics for a change.