diffs in changelogs

PATCH New Andrew Morton 2 participants 5 messages Jun 11, 2015 – Jun 12, 2015 Checkpatch

Share & tools

Andrew Morton

author

2015-06-11 20:40 UTC #0 lore raw

People often put diff snippets in changelogs. This causes problems when one tries to apply a file containing both the changelog and the diff because patch(1) tries to apply the diff which it found in the changelog.

eg, something like

	git show d24a6e1087030b6da | patch -p1

will go haywire.

So can we please have a checkpatch test warning people away from doing this?

patch(1) seems to be really promiscuous in its detection of a patch. I haven't had much success searching for "^--- " and similar. What works best for me is searching for "^[whitespace]@@ -".

Thanks.

Joe Perches

maintainer

2015-06-12 03:13 UTC #1 lore raw

Quoted 4 lines · Andrew Morton

People often put diff snippets in changelogs. This causes problems when one tries to apply a file containing both the changelog and the diff because patch(1) tries to apply the diff which it found in the changelog.

That

Quoted 6 lines

eg, something like
	git show d24a6e1087030b6da | patch -p1
will go haywire.

Quoted 7 lines

So can we please have a checkpatch test warning people away from doing this?
patch(1) seems to be really promiscuous in its detection of a patch. I haven't had much success searching for "^--- " and similar. What works best for me is searching for "^[whitespace]@@ -".

I don't think that's a good test.
Coccinelle uses @@

And how did that commit actually get applied?

I tried applying it to a new branch checked out at ce2b3f595e1c56639085645e0130426e443008c0, it fails.

Anyway, maybe:

---
 scripts/checkpatch.pl | 12 ++++++++++++
 1 file changed, 12 insertions(+)

scripts/checkpatch.pl +12 -0

index 69c4716..2d87e37 100755
@@ -2399,6 +2399,18 @@ sub process {
 			$in_commit_log = 1;
 		}
 
+# Check if the commit log has a diff which confuse patch
+
+		print("icl: <$in_commit_log> line: <$line>\n");
+		if ($in_commit_log &&
+		    (($line =~ m@^\s+diff\b.*a/[\w/]+@ &&
+		      $line =~ m@^\s+diff\b.*a/([\w/]+)\s+b/$1\b@) ||
+		     $line =~ m@^\s*(?:\-\-\-\s+a/|\+\+\+\s+b/)@ ||
+		     $line =~ m/^\s*\@\@ \-\d+,\d+ \+\d+,\d+ \@\@/)) {
+			ERROR("DIFF_IN_COMMIT_MSG",
+			      "It seems a diff exists in the commit message. This can confuse patch\n" . $herecurr);
+		}
+
 # Check if there is UTF-8 in a commit log when a mail header has explicitly
 # declined it, i.e defined some charset where it is missing.
 		if ($in_header_lines &&

Andrew Morton

author

2015-06-12 03:18 UTC #2 lore raw

Quoted 16 lines · On Thu, 11 Jun 2015 20:12:59 -0700 Joe Perches <[email protected]> wrote:

Quoted 4 lines · Andrew Morton
People often put diff snippets in changelogs. This causes problems when one tries to apply a file containing both the changelog and the diff because patch(1) tries to apply the diff which it found in the changelog.
That
Quoted 10 lines
eg, something like
	git show d24a6e1087030b6da | patch -p1
will go haywire.
So can we please have a checkpatch test warning people away from doing this?

Quoted 8 lines

Quoted 3 lines
patch(1) seems to be really promiscuous in its detection of a patch. I haven't had much success searching for "^--- " and similar. What works best for me is searching for "^[whitespace]@@ -".
I don't think that's a good test.
Coccinelle uses @@
And how did that commit actually get applied?

Good question. Maybe `git apply' is smarter about this than patch(1)?

Quoted 2 lines

I tried applying it to a new branch checked out at ce2b3f595e1c56639085645e0130426e443008c0, it fails.

There are tons of them:

z:/usr/src/git26> git log | grep "^[ ]*@@ -" | wc -l
120

Quoted 1 line

Anyway, maybe:

Looks good, thanks. -ENOCHANGELOG. Please send it for real when convenient.

Joe Perches

maintainer

2015-06-12 03:54 UTC #3 lore raw

Quoted 22 lines · Andrew Morton

Quoted 20 lines · On Thu, 11 Jun 2015 20:12:59 -0700 Joe Perches <[email protected]> wrote:
Quoted 4 lines · Andrew Morton
People often put diff snippets in changelogs. This causes problems when one tries to apply a file containing both the changelog and the diff because patch(1) tries to apply the diff which it found in the changelog.
That
Quoted 13 lines
eg, something like
	git show d24a6e1087030b6da | patch -p1
will go haywire.
So can we please have a checkpatch test warning people away from doing this?
patch(1) seems to be really promiscuous in its detection of a patch. I haven't had much success searching for "^--- " and similar. What works best for me is searching for "^[whitespace]@@ -".

Quoted 6 lines

Quoted 4 lines
I don't think that's a good test.
Coccinelle uses @@
And how did that commit actually get applied?
Good question. Maybe `git apply' is smarter about this than patch(1)?

Dunno. git am failed.
My guess is it was applied normally and then
git commit --amend added the other diff content

Quoted 11 lines

Quoted 2 lines
I tried applying it to a new branch checked out at ce2b3f595e1c56639085645e0130426e443008c0, it fails.
There are tons of them:
z:/usr/src/git26> git log | grep "^[ ]*@@ -" | wc -l
120
Quoted 1 line
Anyway, maybe:
Looks good, thanks. -ENOCHANGELOG. Please send it for real when convenient.

RFCPATCH

Joe Perches

maintainer

2015-06-12 17:51 UTC #4 lore raw

PATCH 1/1 checkpatch: Emit an error when there's a diff in a changelog

In reply to Andrew Morton

Warn once when what seems to be a diff snippet in the changelog exists.

Signed-off-by Joe Perches <[email protected]>

---
 scripts/checkpatch.pl | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

scripts/checkpatch.pl +14 -1

index 69c4716..86e4fb1 100755
@@ -1954,6 +1954,7 @@ sub process {
 	my $in_header_lines = $file ? 0 : 1;
 	my $in_commit_log = 0;		#Scanning lines before patch
 	my $commit_log_long_line = 0;
+	my $commit_log_has_diff = 0;
 	my $reported_maintainer_file = 0;
 	my $non_utf8_charset = 0;

@@ -2087,7 +2088,8 @@ sub process {
 		my $rawline = $rawlines[$linenr - 1];
 
 #extract the line range in the file after the patch is applied
-		if ($line=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
+		if (!$in_commit_log &&
+		    $line =~ /^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
 			$is_patch = 1;
 			$first_line = $linenr + 1;
 			$realline=$1-1;

@@ -2181,6 +2183,17 @@ sub process {
 
 		$cnt_lines++ if ($realcnt != 0);
 
+# Check if the commit log has what seems like a diff which can confuse patch
+		if ($in_commit_log && !$commit_log_has_diff &&
+		    (($line =~ m@^\s+diff\b.*a/[\w/]+@ &&
+		      $line =~ m@^\s+diff\b.*a/([\w/]+)\s+b/$1\b@) ||
+		     $line =~ m@^\s*(?:\-\-\-\s+a/|\+\+\+\s+b/)@ ||
+		     $line =~ m/^\s*\@\@ \-\d+,\d+ \+\d+,\d+ \@\@/)) {
+			ERROR("DIFF_IN_COMMIT_MSG",
+			      "Avoid using diff content in the commit message - patch(1) might not work\n" . $herecurr);
+			$commit_log_has_diff = 1;
+		}
+
 # Check for incorrect file permissions
 		if ($line =~ /^new (file )?mode.*[7531]\d{0,2}$/) {
 			my $permhere = $here . "FILE: $realfile\n";