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

From: Erick Mattos <erick.mattos@gmail.com>
Date: Nov 3, 2009, 23:51 UTC
Message-ID: <55bacdd30911031551k1bfd3151t940864e4793f5a37@mail.gmail.com>
In-Reply-To: <20091104073822.6117@nanako3.lavabit.com>

2009/11/3 Nanako Shiraishi <nanako3@lavabit.com>:

Show 6 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 6 quoted lines

> 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.  ;-)

Show 5 quoted lines

> --
> 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!