Re: More builtin git-am issues..

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 5, 2015, 16:10 UTC
Message-ID: <xmqqk2s4deyo.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20150905080325.GA25039@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> [1] I think part of the reason people are interested in "fancy" here is
>     that this topic extends beyond "git am". There's "commit -s", of
>     course, but there's also the generic "interpret-trailers" command,
>     which is supposed to be a generalization of the "--signoff" option.
>     It would be nice if the rules remained consistent for when we add a
>     trailer to an existing block, rather than special-casing signoffs.
>
>     But again, I think that's something to shoot for in the long run.
>     It's more important in the short term not to regress "am".

Yes. I personally think the global append_signoff() everybody else uses can and should implement the same logic (whichever the exact logic is) as whatever is used by "am" because the caller that makes a call to that function knows it is adding a "Signed-off-by:" line, so existing "Signed-off-by" lines are already special in that context.

But for the purpose of 2.6-rc period, I think we should start from doing a separate implementation inside builtin/am.c without touching append_signoff().

We can do a more thoughtful refactoring for append_signoff() in the next cycle. Another thing that needs consideration is the other user of the has_conforming_footer() helper append_signoff() uses, which is the codepath to add "cherry-picked-from"; it is less obvious that "find Signed-off-by: anywhere in the text to decide if the new footer needs its own paragraph" is a good logic in that context.

To salvage "interpret-trailers" needs a lot more, as we are realizing that the definition that led to its external design does not match the way users use footers in the real world. This affects the internal data representation and the whole thing needs to be rethought.

Here is a quick attempt to do the "just fix am regression without changing anything else".

 builtin/am.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 83b3d86..c63d238 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1213,9 +1213,24 @@ static void NORETURN die_user_resolve(const struct am_state *state)
 static void am_append_signoff(struct am_state *state)
 {
 	struct strbuf sb = STRBUF_INIT;
+	char *cp;
 
 	strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);
-	append_signoff(&sb, 0, 0);
+	strbuf_complete_line(&sb);
+
+	for (cp = sb.buf;
+	     cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;
+	     cp = strchr(cp, '\n')) {
+		if (sb.buf < cp && cp[-1] == '\n')
+			break;
+	}
+	if (!cp)
+		strbuf_addch(&sb, '\n');
+	strbuf_addf(&sb, "%s%s\n",
+		    sign_off_header,
+		    fmt_name(getenv("GIT_COMMITTER_NAME"),
+			     getenv("GIT_COMMITTER_EMAIL")));
+	strbuf_addch(&sb, '\n');
 	state->msg = strbuf_detach(&sb, &state->msg_len);
 }