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

From: Nanako Shiraishi <nanako3@lavabit.com>
Date: Nov 3, 2009, 22:38 UTC
Message-ID: <20091104073822.6117@nanako3.lavabit.com>
In-Reply-To: <1257282551-9999-1-git-send-email-erick.mattos@gmail.com>

Quoting Erick Mattos <erick.mattos@gmail.com>

Show 16 quoted lines

> When we use one of the options above we are normally trying to do mainly
> two things: using the source as a template or recreating a commit with
> corrections.
>
> When they are used, the authorship and timestamp recorded in the newly
> created commit are always taken from the original commit.  And they
> should not when we are using it as a template or when our change to the
> code is so significant that we should take over the authorship (with the
> blame for bugs we introduce, of course).
>
> The new --reset-author option is meant to solve this need by
> regenerating the timestamp and setting as the new author the committer
> or the one specified by --author option.
>
> Signed-off-by: Erick Mattos <erick.mattos@gmail.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 was originally puzzled by Junio's "mine" because I thought "what does taking over the authorship have to do with digging diamonds out of earth?". "reset-author" makes it very clear, and I think it is a much better name for the option.

Your first paragraph mentions "one of the options", but subject mentions four (-c, -C, --amend and --reset-author) and you want only the first three of them.

The second paragraph of gmane/131893 says "borrow the commit log message" instead of repeating "a template". It has the effect of clarifying that only the message part is borrowed but not the authorship information.

I think gmane/131893 has much better description than your first two paragraphs for the these reasons.

Show 5 quoted lines

> diff --git a/t/t7509-commit.sh b/t/t7509-commit.sh
> new file mode 100755
> index 0000000..7290242
> --- /dev/null
> +++ b/t/t7509-commit.sh

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.

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

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

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

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