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

[Erick Mattos <erick.mattos@gmail.com>]

2009/11/3 Nanako Shiraishi <nanako3@lavabit.com>:
> If you are sending an update to a previous patch (I am comparing
> this patch with the "show by example" patch Junio sent on 11/02
> http://article.gmane.org/gmane.comp.version-control.git/131893),
> it is a common courtesy to summarize what you changed relative
> to the base version after the three dashes line, so that people
> will know which part can be skipped while reviewing your patch.

I got your point.  I will try to improve that.

I have been talking to Junio during the weekend and with a lot of
emails sent to each other.  It happens that when he sent gmane/131893
(I had to find out what that code meant because I was using
[marc.info]! :-) ) I had already sent another patch with the
suggestions he made in a previous email.
So his message was late.  While I was waiting for his acknowledgment I
started thinking he could be lost on those e-mails so I sent the one
you are replying to make it the last on the queue.

> I have to say that the test script is much worse than what
> gmane/131893 had.
>
> The old test made sure that -C copied the message, with or
> without the --mine option. But this test only checks the
> author line (and it doesn't even make sure that "^author"
> matches only in the header). The messages are unchecked,
> and it will let a bug when someone breaks --reset-author
> logic in the future in such a way that it corrupts the
> message by mistake go unnoticed.

I think you misunderstood something here:
* On his patch (which he sent before seeing mine), while testing -C,
on first check, he is checking author_header only.
* While testing -C on mine I compare both messages without "parent",
"tree" and "committer".  Whole!

After check one I did concentrate only on author data.  But on mine I
separate the tests between timestamp and author (name and email),
making sure the author was set to the actual committer and that the
timestamp was behaving as expected.

I am not saying mine is better.  What I am saying is that I expected
him to notice and change/improve THIS patch.  Not the other.

The new option only touches on getting new author or copying the
original so that is why I made the first check in whole and the others
only by author.  If people think that this operation is so uncertain,
then everything should be compared: parent, author and message on all
tests.

It is not a problem for me adding more code to the test even if I
consider it unnecessary.  I am doing this only to give a pay-back for
all the good service this free software gave to me so I am very
patient to all demands.  I will be letting this effort go only at the
real end.  No matter how long it takes.

> Also the old test was much more readable because it used
> shell functions to avoid repeating cat-file and pipe to sed
> script. It also tagged the initial commit, which had a nice
> effect that a failure in any intermediate test will not change
> which earlier commit is reused (eg. yours say "-C HEAD^" but
> old test said "-C Initial").

I am so used to scripts that I really haven't thought it difficult to
read but I can do some cosmetic too if it is important.  As I said
early, I was waiting for Junio's jugdement over my later patch.

> It looks silly to create an "Initial Commit" in the middle
> of history, too (^_^).

This is something more laborious but which I thought was important to
let murphy's law act on a real case.  We never do an amend on an
initial commit so I only did the tests on a later one.

> I think it is much better to replace "--mine" in gmane/131893
> with "--reset-author" and make no other change to the test.

Let's see Junio's opinion...  We have changed names a lot since start.  ;-)

> --
> Nanako Shiraishi
> http://ivory.ap.teacup.com/nanako3/
>
>

Thank you very much for all your comments.  I really appreciate about
being noticed by you people.  It is nice for a newcomer!