Charvi Mendiratta [off-list ref] 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.
quoted hunk
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" ;-).
quoted hunk
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.
Hi Junio,
On Thu, 21 Jan 2021 at 07:08, Junio C Hamano [off-list ref] wrote:
Charvi Mendiratta [off-list ref] writes:
quoted
+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.
Oops, I think Phillip and Christian also pointed in the last revision
to look for alternatives to make it easy. I mistook that point and
forgot to look at it.
- 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)
yes, it returns the same result as given in this example (But I am
not sure what you are taking " SP, HT, CR, etc " ? otherwise if its
whitespace, then its works the same).
- 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.
I followed and agreed with the above.
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?).
I look into the code again and wonder if we can change this function like this :
static int subject_length(const char *body)
{
const char *p = body;
while (*p) {
if (*p == '\n' && p[1] =='\n') {
break;
} else {
p++;
}
}
return p - body;
}
I think checking again '\n' will also serve the purpose as we separate
the commit message subject and its body with the newline. Also, this
is also
true that this function is only called when the message starts with
(squash! or amend! or fixup!)
In any case, let's keep reading.
quoted
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.
quoted
+ 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.
I agree this working does the same and comments out the subject of the
commit message starting with squash! or fixup! or amend!, upon
squashing the two or more commits.
quoted
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" ;-).
Okay, maybe we can use "message body" here.
quoted
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?
Ohh yes ..
quoted
+ 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?
.. but now I think to remove this `grep -v "squash" commit` as also
discussed with Phillip earlier that in this test script we are not
checking for the commented commit message.
Perhaps look for
grep "^extra para" commit
to ensure that you are not seeing a commented out but somehow failed
to get stripspaced out?
I am not sure, what does failing to get stripspaced mean?
Thanks for the review !
Thanks and Regards,
Charvi
On Thu, Jan 21, 2021 at 3:02 PM Charvi Mendiratta [off-list ref] wrote:
Hi Junio,
On Thu, 21 Jan 2021 at 07:08, Junio C Hamano [off-list ref] wrote:
quoted
Charvi Mendiratta [off-list ref] writes:
quoted
+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.
Oops, I think Phillip and Christian also pointed in the last revision
to look for alternatives to make it easy. I mistook that point and
forgot to look at it.
Yes, please take a look at find_commit_subject() in "commit.c".
quoted
quoted
+ 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?
.. but now I think to remove this `grep -v "squash" commit` as also
discussed with Phillip earlier that in this test script we are not
checking for the commented commit message.
quoted
Perhaps look for
grep "^extra para" commit
to ensure that you are not seeing a commented out but somehow failed
to get stripspaced out?
I am not sure, what does failing to get stripspaced mean?
I think this refers to:
https://git-scm.com/docs/git-stripspace
Best,
Christian.
Hi Christian and Charvi
On 21/01/2021 15:21, Christian Couder wrote:
On Thu, Jan 21, 2021 at 3:02 PM Charvi Mendiratta [off-list ref] wrote:
quoted
Hi Junio,
On Thu, 21 Jan 2021 at 07:08, Junio C Hamano [off-list ref] wrote:
quoted
Charvi Mendiratta [off-list ref] writes:
quoted
+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.
Oops, I think Phillip and Christian also pointed in the last revision
to look for alternatives to make it easy. I mistook that point and
forgot to look at it.
Yes, please take a look at find_commit_subject() in "commit.c".
That looks like it is taking the commit header and finding the start of
the message. We have just the message, I think we probably want to use
format_subject() in pretty.c which does what my hard to follow code does
with the option to replace newlines in the subject with another character.
Best Wishes
Phillip
quoted
quoted
quoted
+ 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?
.. but now I think to remove this `grep -v "squash" commit` as also
discussed with Phillip earlier that in this test script we are not
checking for the commented commit message.
quoted
Perhaps look for
grep "^extra para" commit
to ensure that you are not seeing a commented out but somehow failed
to get stripspaced out?
I am not sure, what does failing to get stripspaced mean?
I think this refers to:
https://git-scm.com/docs/git-stripspace
Best,
Christian.
Hi,
On Thu, 21 Jan 2021 at 20:51, Christian Couder
[off-list ref] wrote:
[...]
quoted
quoted
quoted
+ 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?
.. but now I think to remove this `grep -v "squash" commit` as also
discussed with Phillip earlier that in this test script we are not
checking for the commented commit message.
quoted
Perhaps look for
grep "^extra para" commit
to ensure that you are not seeing a commented out but somehow failed
to get stripspaced out?
I am not sure, what does failing to get stripspaced mean?
I think this refers to:
https://git-scm.com/docs/git-stripspace
Okay, thanks for referring and also here in this test script - so that
the test does not fail
due to stripspace, it is using the `test_line_count` function. Also I
agree with above
so maybe it is right to just replace with:
grep "^extra para" commit &&
grep "extra para" commit >actual &&
Thanks and Regards,
Charvi