-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rationalize line endings for scissors-cleanup #2714
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR also wants a more verbose commit message, I think. Something similar to d540b70's commit message strikes me as desirable.
I take your point and apologize for a scant commit message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to the suggestions I gave, I would like to ask you to use the interactive rebase to squash the fixup commits into the commits that were not quite right in the first place, and to drop the merge commit.
t/t7502-commit-porcelain.sh
Outdated
echo >>negative && | ||
cat >text <<-\EOF && | ||
|
||
# ------------------------ >8 ------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks as if the lines above were indented using spaces, but here you use a tab. Git's coding style is to use tabs, always.
I cannot fail to see that you left an empty line after the test_expect_success
line (which BTW is too long, it has to be at most 80 columns). Both coding style violations are already present in the test script, so I don't fault you for copy/editing those. But I would like to request to fix the style of the lines you added. (You don't need to bother with lines you did not touch, of course, that's not your responsibility.)
This change enhances `git commit --cleanup=scissors` by detecting scissors lines ending in either LF (UNIX-style) or CR/LF (DOS-style). Regression tests are included to specifically test for trailing comments after a CR/LF-terminated scissors line. Signed-off-by: Luke Bonanomi <[email protected]> Signed-off-by: Johannes Schindelin <[email protected]>
I hope you don't mind that I made a couple of edits. Nothing major, just a few touch-ups that I hope will increase the chances that the core Git project will accept this patch as-is. Here is the range-diff (if you have not worked with range-diffs before, here is an explanation): 1: 17fffb7c9383 ! 1: d508f45ebfed This change enhances git-commit "scissors" cleanup by making scissors lines ("------------------------ >8 ------------------------") ending in either LF (UNIX-style) or CR/LF (DOS-style) detectable by git-commit.
@@
## Metadata ##
-Author: lbonanomİ <[email protected]>
+Author: Luke Bonanomi <[email protected]>
## Commit message ##
- This change enhances git-commit "scissors" cleanup by making scissors lines
- ("------------------------ >8 ------------------------") ending in either LF
- (UNIX-style) or CR/LF (DOS-style) detectable by git-commit.
+ commit: accept "scissors" with CR/LF line endings
- Regression tests are included to specifically test for trailing comments after
- a CR/LF-terminated scissors line.
+ This change enhances `git commit --cleanup=scissors` by detecting
+ scissors lines ending in either LF (UNIX-style) or CR/LF (DOS-style).
+
+ Regression tests are included to specifically test for trailing
+ comments after a CR/LF-terminated scissors line.
Signed-off-by: Luke Bonanomi <[email protected]>
+ Signed-off-by: Johannes Schindelin <[email protected]>
## t/t7502-commit-porcelain.sh ##
@@ t/t7502-commit-porcelain.sh: test_expect_success 'cleanup commit messages (scissors option,-F,-e, scissors on
- git commit --cleanup=scissors -e -F text -a --allow-empty-message &&
- git cat-file -p HEAD >raw &&
- sed -e "1,/^\$/d" raw >actual &&
-+ test_must_be_empty actual
-+'
-+
+ test_must_be_empty actual
+ '
+
+test_expect_success 'helper-editor' '
+
-+ write_script "$PWD"/lf-to-crlf.sh <<-\EOF
++ write_script lf-to-crlf.sh <<-\EOF
+ sed "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
-+ cp "$1".new /tmp
+ mv -f "$1".new "$1"
+ EOF
+'
+
+test_expect_success 'cleanup commit messages (scissors option,-F,-e, CR/LF line endings)' '
+
-+ echo >>negative &&
+ test_config core.editor "\"$PWD/lf-to-crlf.sh\"" &&
+ scissors="# ------------------------ >8 ------------------------" &&
++
+ test_write_lines >text \
+ "# Keep this comment" "" " $scissors" \
+ "# Keep this comment, too" "$scissors" \
-+ "# Removed this comment" "$scissors" \
++ "# Remove this comment" "$scissors" \
+ "Remove this comment, too" &&
-+ true &&
++
+ test_write_lines >expect \
+ "# Keep this comment" "" " $scissors" \
+ "# Keep this comment, too" &&
-+ git commit --cleanup=scissors -e -F text -a &&
++
++ git commit --cleanup=scissors -e -F text --allow-empty &&
+ git cat-file -p HEAD >raw &&
+ sed -e "1,/^\$/d" raw >actual &&
+ test_cmp expect actual
@@ t/t7502-commit-porcelain.sh: test_expect_success 'cleanup commit messages (sciss
+
+test_expect_success 'cleanup commit messages (scissors option,-F,-e, scissors on first line, CR/LF line endings)' '
+
-+ echo >>negative &&
+ scissors="# ------------------------ >8 ------------------------" &&
+ test_write_lines >text \
+ "$scissors" \
+ "# Remove this comment and any following lines" &&
+ cp text /tmp/test2-text &&
-+ git commit --cleanup=scissors -e -F text -a --allow-empty-message &&
++ git commit --cleanup=scissors -e -F text --allow-empty --allow-empty-message &&
+ git cat-file -p HEAD >raw &&
+ sed -e "1,/^\$/d" raw >actual &&
- test_must_be_empty actual
- '
++ test_must_be_empty actual
++'
++
+ test_expect_success 'cleanup commit messages (strip option,-F)' '
+ echo >>negative &&
## wt-status.c ##
@@
@@ wt-status.c: static void wt_longstatus_print_other(struct wt_status *s,
strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
- if (starts_with(s, pattern.buf + 1))
-+ if (starts_with(s, pattern.buf + 1) && starts_with_newline(s + pattern.len - 1))
++ if (starts_with(s, pattern.buf + 1) &&
++ starts_with_newline(s + pattern.len - 1))
len = 0;
- else if ((p = strstr(s, pattern.buf)))
-+ else if ((p = strstr(s, pattern.buf)) && starts_with_newline(p + pattern.len))
++ else if ((p = strstr(s, pattern.buf)) &&
++ starts_with_newline(p + pattern.len))
len = p - s + 1;
strbuf_release(&pattern);
return len;
-
- ## wt-status.h ##
-@@ wt-status.h: static inline int is_from_cherry_pick(enum commit_whence whence)
- whence == FROM_CHERRY_PICK_MULTI;
- }
-
-+
- static inline int is_from_rebase(enum commit_whence whence)
- {
- return whence == FROM_REBASE_PICK; |
This comment has been minimized.
This comment has been minimized.
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
The cleanup mode called "scissors" in `git commit` [now handles CR/LF line endings correctly](git-for-windows/git#2714). Signed-off-by: Johannes Schindelin <[email protected]>
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
Rationalize line endings for scissors-cleanup
This PR attempts to resolve #2623
Strip
cut_line[]
definition to enhance interoperability between UNIX and DOS line-ending stylesSigned-off-by: Luke Bonanomi [email protected]