On Thu, 14 Jan 2021 at 00:31, Taylor Blau <me@xxxxxxxxxxxx> wrote:
>
> On Fri, Jan 08, 2021 at 02:53:41PM +0530, Charvi Mendiratta wrote:
> > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> >
> > When squashing commit messages the squash!/fixup! subjects are not of
> > interest so comment them out to stop them becoming part of the final
> > message.
> >
> > This change breaks a bunch of --autosquash tests which rely on the
> > "squash! <subject>" line appearing in the final commit message. This is
> > addressed by adding a second line to the commit message of the "squash!
> > ..." commits and testing for that.
> >
> > Signed-off-by: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> > Signed-off-by: Charvi Mendiratta <charvi077@xxxxxxxxx>
> > ---
> > sequencer.c | 25 ++++++++++++++++++++++++-
> > t/t3415-rebase-autosquash.sh | 27 +++++++++++++--------------
> > t/t3900-i18n-commit.sh | 4 ----
> > 3 files changed, 37 insertions(+), 19 deletions(-)
> >
> > diff --git a/sequencer.c b/sequencer.c
> > index 5062976d10..b050a9a212 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -1718,15 +1718,38 @@ static int is_pick_or_similar(enum todo_command command)
> > }
> > }
> >
> > +static size_t subject_length(const char *body)
> > +{
> > + size_t i, len = 0;
> > + char c;
> > + int blank_line = 1;
> > + for (i = 0, c = body[i]; c; c = body[++i]) {
> > + if (c == '\n') {
> > + if (blank_line)
> > + return len;
> > + len = i + 1;
> > + blank_line = 1;
> > + } else if (!isspace(c)) {
> > + blank_line = 0;
> > + }
> > + }
> > + return blank_line ? len : i;
> > +}
> > +
>
> OK, so this gets the length of the subject in "body", which is defined
> as the run of characters before a newline and then a space character. So
> "foo bar\n\nbaz" would return 7, but "foo bar\nbaz" would return 11.
>
> Makes sense. (Apologies for stating the obvious here, I just had to read
> this function to myself a couple of times to make sure that I understood
> what it was doing.)
>
Earlier while testing patch, I also went through in the same way and
now got confirmed as you described here.
> > static void append_squash_message(struct strbuf *buf, const char *body,
> > struct replay_opts *opts)
> > {
> > + size_t commented_len = 0;
> > +
> > unlink(rebase_path_fixup_msg());
> > + if (starts_with(body, "squash!") || starts_with(body, "fixup!"))
> > + commented_len = subject_length(body);
> > strbuf_addf(buf, "\n%c ", comment_line_char);
> > strbuf_addf(buf, _("This is the commit message #%d:"),
> > ++opts->current_fixup_count + 1);
> > strbuf_addstr(buf, "\n\n");
> > - strbuf_addstr(buf, body);
> > + strbuf_add_commented_lines(buf, body, commented_len);
> > + strbuf_addstr(buf, body + commented_len);
>
> Very nice; the subject gets commented when it starts with "squash!" or
> "fixup!", but the body remains uncommented. Makes sense to me.
>
I agree and Thanks to Phillip, for the patch.
> > @@ -224,7 +223,7 @@ test_expect_success 'auto squash that matches longer sha1' '
> > git cat-file blob HEAD^:file1 >actual &&
> > test_cmp expect actual &&
> > git cat-file commit HEAD^ >commit &&
> > - grep squash commit >actual &&
> > + grep "extra para" commit >actual &&
> > test_line_count = 1 actual
> > '
>
> Worth checking that "squash" doesn't appear in an uncommented part of
> actual? Or better yet, checking that "# squash ..." _does_ appear.
>
> I.e., that we would leave this as:
>
> - grep squash commit >actual &&
> + grep "^# squash" commit >actual &&
> + grep "extra para" commit >actual &&
>
> > @@ -342,8 +341,8 @@ test_expect_success C_LOCALE_OUTPUT 'autosquash with custom inst format' '
> > git cat-file blob HEAD^:file1 >actual &&
> > test_cmp expect actual &&
> > git cat-file commit HEAD^ >commit &&
> > - grep squash commit >actual &&
> > - test_line_count = 2 actual
> > + grep first commit >actual &&
> > + test_line_count = 3 actual
> > '
>
> Ditto.
Okay, I will add it .
Thanks and Regards,
Charvi
>
> Thanks,
> Taylor
