git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Charvi Mendiratta <charvi077@gmail.com>
Cc: git@vger.kernel.org, chriscool@tuxfamily.org,
	phillip.wood@dunelm.org.uk, me@ttaylorr.com
Subject: Re: [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from squash message
Date: Wed, 20 Jan 2021 17:38:04 -0800	[thread overview]
Message-ID: <xmqqmtx3dq83.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20210119074102.21598-4-charvi077@gmail.com> (Charvi Mendiratta's message of "Tue, 19 Jan 2021 13:10:58 +0530")

Charvi Mendiratta <charvi077@gmail.com> writes:

> +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;
> +}

I cannot quite tell what this loop is trying to compute at the first
glance.

 - If body[0] == '\n', then i==0, c==LF, blank_line==1 and len==0
   so len==0 is returned immediately.

 - If the first line has only SP, HT, CR, etc. whitespace,
   blank_line stays 1 and at the end of the line when we see
   c=='\n', body[i] is pointing at that '\n', blank_line is true, so
   len is returned from the previous iteration (e.g. body="   \n"
   returns 0)

 - If the first line has some non space, blank_line becomes false,
   so at the end of that line when we see c=='\n', body[i] is
   pointing at that '\n', len==i+1 becomes one past that LF and then
   we reset blank_line to true??? and go on to the next line.

So when we see LF, if we have seen any non whitespace byte on that
line, blank_line is false.  Only when we saw LF followed by zero or
more whitespace before seeing another LF, we return len that was set
when we saw the previous LF (which is one past that LF).
    
So... is this trying to find the first paragraph-break-looking line
to find the end of the first paragraph.  OK.

There must be an easier-to-read way to write all this, though, I
would think (or don't we already have an existing code that is
waiting to be factored out?).

In any case, let's keep reading.

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

As add_commented_lines places the comment character at the beginning
of each line, it is OK for body[0..commented_len) to contain more than
one lines.  Good.

> +	strbuf_addstr(buf, body + commented_len);

And we add everything after the beginning of the paragraph-break
looking line.  This code may add a line, immediately after the
previous "commented out" block, bunch of whitespaces and then a LF.
It will be cleaned up with stripspace most of the time, but
depending on the end-user settings, it may be left behind.  I am
guessing that is what we want, but thought it would not hurt to
double check.

> diff --git a/t/t3415-rebase-autosquash.sh b/t/t3415-rebase-autosquash.sh
> index 7bab6000dc..551dc06bc3 100755
> --- a/t/t3415-rebase-autosquash.sh
> +++ b/t/t3415-rebase-autosquash.sh
> @@ -81,8 +81,7 @@ test_auto_squash () {
>  	echo 1 >file1 &&
>  	git add -u &&
>  	test_tick &&
> -	git commit -m "squash! first" &&
> -
> +	git commit -m "squash! first" -m "extra para for first" &&

It is not "extra"; that's the beginning of the "body" ;-).

>  	git tag $1 &&
>  	test_tick &&
>  	git rebase $2 -i HEAD^^^ &&
> @@ -139,7 +138,7 @@ test_expect_success 'auto squash that matches 2 commits' '
>  	echo 1 >file1 &&
>  	git add -u &&
>  	test_tick &&
> -	git commit -m "squash! first" &&
> +	git commit -m "squash! first" -m "extra para for first" &&
>  	git tag final-multisquash &&
>  	test_tick &&
>  	git rebase --autosquash -i HEAD~4 &&
> @@ -192,7 +191,7 @@ test_expect_success 'auto squash that matches a sha1' '
>  	git add -u &&
>  	test_tick &&
>  	oid=$(git rev-parse --short HEAD^) &&
> -	git commit -m "squash! $oid" &&
> +	git commit -m "squash! $oid" -m "extra para" &&
>  	git tag final-shasquash &&
>  	test_tick &&
>  	git rebase --autosquash -i HEAD^^^ &&
> @@ -203,7 +202,8 @@ test_expect_success 'auto squash that matches a sha1' '
>  	git cat-file blob HEAD^:file1 >actual &&
>  	test_cmp expect actual &&
>  	git cat-file commit HEAD^ >commit &&
> -	grep squash commit >actual &&
> +	grep -v "squash" commit &&

This says that the file must have at least one line that does not
say "squash" or the test is a failure.  It does not say "there
should be no line that has "squash" on it".  Intended?

> +	grep "extra para" commit >actual &&

I can tell that you want the "extra para" to still remain, but how
does the grep that is not anchored guarantee that?  Perhaps look for

	grep "^extra para" commit

to ensure that you are not seeing a commented out but somehow failed
to get stripspaced out?

>  	test_line_count = 1 actual
>  '

Thanks.

  reply	other threads:[~2021-01-21  3:47 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-01-08  9:23 [RFC PATCH 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-13 18:43   ` Taylor Blau
2021-01-14  8:12     ` Charvi Mendiratta
2021-01-14 10:46       ` Phillip Wood
2021-01-15  8:38         ` Charvi Mendiratta
2021-01-15 17:22           ` Junio C Hamano
2021-01-16  4:49             ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-13 19:01   ` Taylor Blau
2021-01-14  8:27     ` Charvi Mendiratta
2021-01-14 10:29       ` Phillip Wood
2021-01-15  8:35         ` Charvi Mendiratta
2021-01-15  8:44           ` Christian Couder
2021-01-15 11:12             ` Charvi Mendiratta
2021-01-17  3:39         ` Charvi Mendiratta
2021-01-18 18:29           ` Phillip Wood
2021-01-19  4:08             ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-13 19:14   ` Taylor Blau
2021-01-13 20:37     ` Junio C Hamano
2021-01-14  7:40       ` Christian Couder
2021-01-14  8:57     ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-14  9:23   ` Christian Couder
2021-01-14  9:45     ` Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-08  9:23 ` [RFC PATCH 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-24 17:03   ` [PATCH v3 " Charvi Mendiratta
2021-01-24 17:03     ` [PATCH v3 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-24 17:04     ` [PATCH v3 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-29 18:20     ` [PATCH v4 0/9][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-02-02  0:47         ` Eric Sunshine
2021-02-02 15:29           ` Charvi Mendiratta
2021-02-03  5:05             ` Eric Sunshine
2021-02-04  0:00               ` Charvi Mendiratta
2021-02-04  0:14                 ` Eric Sunshine
2021-01-29 18:20       ` [PATCH v4 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-02-02  2:01         ` Eric Sunshine
2021-02-02 10:02           ` Christian Couder
2021-02-02 15:31             ` Charvi Mendiratta
2021-02-03  5:44             ` Eric Sunshine
2021-02-04  0:01               ` Charvi Mendiratta
2021-02-04 10:46           ` Phillip Wood
2021-02-04 16:14             ` Eric Sunshine
2021-02-04 19:12           ` Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-02-02  3:20         ` Eric Sunshine
2021-02-02 15:29           ` Charvi Mendiratta
2021-01-29 18:20       ` [PATCH v4 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-02-02  3:23         ` Eric Sunshine
2021-02-02 14:12           ` Marc Branchaud
2021-02-02 15:30           ` Charvi Mendiratta
2021-02-04 19:04       ` [PATCH v5 0/8][Outreachy] rebase -i: add options to fixup command Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 1/8] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 2/8] sequencer: factor out code to append squash message Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 3/8] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 4/8] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 5/8] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 6/8] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 7/8] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-02-04 19:05         ` [PATCH v5 8/8] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-02-05  7:30         ` [PATCH v5 0/8][Outreachy] rebase -i: add options to fixup command Eric Sunshine
2021-02-05  9:42           ` Charvi Mendiratta
2021-02-05 18:25             ` Christian Couder
2021-02-05 18:56               ` Eric Sunshine
2021-02-06  5:36                 ` Charvi Mendiratta
2021-02-05 19:13               ` Junio C Hamano
2021-02-06  5:37                 ` Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 1/9] rebase -i: only write fixup-message when it's needed Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 2/9] sequencer: factor out code to append squash message Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 3/9] rebase -i: comment out squash!/fixup! subjects from " Charvi Mendiratta
2021-01-21  1:38   ` Junio C Hamano [this message]
2021-01-21 14:02     ` Charvi Mendiratta
2021-01-21 15:21       ` Christian Couder
2021-01-21 16:58         ` Phillip Wood
2021-01-21 20:56         ` Junio C Hamano
2021-01-22 19:41           ` Charvi Mendiratta
2021-01-22 19:41         ` Charvi Mendiratta
2021-01-19  7:40 ` [PATCH v2 4/9] sequencer: pass todo_item to do_pick_commit() Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 5/9] sequencer: use const variable for commit message comments Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 6/9] rebase -i: add fixup [-C | -c] command Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 7/9] t3437: test script for fixup [-C|-c] options in interactive rebase Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 8/9] rebase -i: teach --autosquash to work with amend! Charvi Mendiratta
2021-01-19  7:41 ` [PATCH v2 9/9] doc/git-rebase: add documentation for fixup [-C|-c] options Charvi Mendiratta
2021-01-19 14:37   ` Marc Branchaud
2021-01-19 17:13     ` Charvi Mendiratta
2021-01-19 22:05       ` Marc Branchaud
2021-01-20  7:10         ` Charvi Mendiratta
2021-01-20 11:04       ` Phillip Wood
2021-01-20 12:31         ` Charvi Mendiratta
2021-01-20 14:29           ` Phillip Wood
2021-01-20 16:09             ` Charvi Mendiratta

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmtx3dq83.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=charvi077@gmail.com \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=me@ttaylorr.com \
    --cc=phillip.wood@dunelm.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).