Re: [PATCH] sequencer: beautify subject of reverts of reverts

[Junio C Hamano <gitster@pobox.com>]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..853b4ed334 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2234,6 +2234,9 @@ static int do_pick_commit(struct repository *r,
>  		if (opts->commit_use_reference) {
>  			strbuf_addstr(&msgbuf,
>  				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
> +		} else if (starts_with(msg.subject, "Revert \"")) {
> +			strbuf_addstr(&msgbuf, "Reapply ");
> +			strbuf_addstr(&msgbuf, msg.subject + 7);
>  		} else {
>  			strbuf_addstr(&msgbuf, "Revert \"");
>  			strbuf_addstr(&msgbuf, msg.subject);

Two and half comments:

 * The hard-coded +7 looks fragile.  Perhaps use skip_prefix?

 * During transition to introduce a new version of Git with this
   feature, when reverting an existing revert of a revert, care must
   be taken.  Such a commit would begin as

	Revert "Revert ...

   and turning it to

	Reapply "Revert ...

   may not be a good way to label such a reversion of a double
   revert without end-user confusion.  As it is very likely that
   such a reversion commit was created by existing versions of Git,
   the easiest and least confusing way out would be to notice and
   refrain from touching such a commit.

 * The change lacks tests.

Removal of hardcoded +7 (i.e. the first point) may look like this.

 sequencer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git i/sequencer.c w/sequencer.c
index 853b4ed334..520113ec63 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -2227,6 +2227,8 @@ static int do_pick_commit(struct repository *r,
 	 */
 
 	if (command == TODO_REVERT) {
+		const char *original_title;
+
 		base = commit;
 		base_label = msg.label;
 		next = parent;
@@ -2234,9 +2236,9 @@ static int do_pick_commit(struct repository *r,
 		if (opts->commit_use_reference) {
 			strbuf_addstr(&msgbuf,
 				"# *** SAY WHY WE ARE REVERTING ON THE TITLE LINE ***");
-		} else if (starts_with(msg.subject, "Revert \"")) {
+		} else if (skip_prefix(msg.subject, "Revert \"", &original_title)) {
-			strbuf_addstr(&msgbuf, "Reapply ");
+			strbuf_addstr(&msgbuf, "Reapply \"");
-			strbuf_addstr(&msgbuf, msg.subject + 7);
+			strbuf_addstr(&msgbuf, original_title);
 		} else {
 			strbuf_addstr(&msgbuf, "Revert \"");
 			strbuf_addstr(&msgbuf, msg.subject);