[PATCHv2 0/2] git apply: cope with whitespace differences

[PATCHv2 0/2] git apply: cope with whitespace differences

[Giuseppe Bilotta]

This second version includes significant cleanups and improvements to my
previous attempt at a --ignore-whitespace option for git-apply.

Following Junio's review, the patch now includes proper handling for
leading whitespace (specifically the case when one part has it and the
other one doesn't), and more test cases.

The second patch fixes the updating of the postimage when whitespace is
adjusted to match the target.

Giuseppe Bilotta (2):
  git apply: option to ignore whitespace differences
  git apply: preserve original whitespace with --ignore-whitespace

 builtin-apply.c                        |  108 +++++++++++++++++++++++++++++---
 contrib/completion/git-completion.bash |    2 +
 git-am.sh                              |    3 +-
 git-rebase.sh                          |    3 +
 t/t4107-apply-ignore-whitespace.sh     |  107 +++++++++++++++++++++++++++++++
 5 files changed, 214 insertions(+), 9 deletions(-)
 create mode 100755 t/t4107-apply-ignore-whitespace.sh

[PATCHv2 1/2] git apply: option to ignore whitespace differences

[Giuseppe Bilotta]

Introduce --ignore-whitespace option to ignore whitespace differences
while applying the patch. The feature is based on Robert Fitzsimons'
work from
http://permalink.gmane.org/gmane.comp.version-control.git/7876

A test is included, and 'git am' and 'git rebase' are made aware of this
option and pass it through to 'git apply', and so is the bash git
completion.
---
 builtin-apply.c                        |   55 ++++++++++++++++-
 contrib/completion/git-completion.bash |    2 +
 git-am.sh                              |    3 +-
 git-rebase.sh                          |    3 +
 t/t4107-apply-ignore-whitespace.sh     |  107 ++++++++++++++++++++++++++++++++
 5 files changed, 167 insertions(+), 3 deletions(-)
 create mode 100755 t/t4107-apply-ignore-whitespace.sh

diff --git a/builtin-apply.c b/builtin-apply.c
index dc0ff5e..01230f1 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -39,6 +39,7 @@ static int diffstat;
 static int numstat;
 static int summary;
 static int check;
+static int ignore_whitespace;
 static int apply = 1;
 static int apply_in_reverse;
 static int apply_with_reject;
@@ -214,6 +215,52 @@ static uint32_t hash_line(const char *cp, size_t len)
 	return h;
 }
 
+/*
+ * Compare s1 to s2 up to length n, ignoring whitespace differences.
+ * It is acceptable if s2 is a substring of s1.
+ */
+static int memcmp_ignore_whitespace(const char *s1, const char *s2, size_t n)
+{
+	const char *stop = s2 + n;
+	int result;
+
+	if (!n)
+		return 0;
+
+	/* skip leading whitespace */
+	while (isspace(*s1))
+		s1++;
+	while (isspace(*s2))
+		s2++;
+	while (!result && s2 < stop) {
+		result = *s1++ - *s2++;
+		/*
+		 * skip whitespace inside if we have whitespace
+		 * on both buffers
+		 */
+		if (isspace(*s1) && isspace(*s2)) {
+			while (isspace(*s1))
+				s1++;
+			while (isspace(*s2))
+				s2++;
+		}
+	}
+
+	return result;
+}
+
+/*
+ * Returns true if the lines in s2 match the buffer in s1 up to length n,
+ * according to the ignore_whitespace setting
+ */
+static int lines_match(const char *s1, const char *s2, size_t n)
+{
+	if (ignore_whitespace)
+		return !memcmp_ignore_whitespace(s1, s2, n);
+	else
+		return !memcmp(s1, s2, n);
+}
+
 static void add_line_info(struct image *img, const char *bol, size_t len, unsigned flag)
 {
 	ALLOC_GROW(img->line_allocated, img->nr + 1, img->alloc);
@@ -1690,7 +1737,7 @@ static int match_fragment(struct image *img,
 	if ((match_end
 	     ? (try + preimage->len == img->len)
 	     : (try + preimage->len <= img->len)) &&
-	    !memcmp(img->buf + try, preimage->buf, preimage->len))
+	    lines_match(img->buf + try, preimage->buf, preimage->len))
 		return 1;
 
 	if (ws_error_action != correct_ws_error)
@@ -1698,7 +1745,9 @@ static int match_fragment(struct image *img,
 
 	/*
 	 * The hunk does not apply byte-by-byte, but the hash says
-	 * it might with whitespace fuzz.
+	 * it might with whitespace fuzz. We haven't been asked to
+	 * ignore whitespace, we were asked to correct whitespace
+	 * errors, so let's try matching after whitespace correction.
 	 */
 	fixed_buf = xmalloc(preimage->len + 1);
 	buf = fixed_buf;
@@ -3304,6 +3353,8 @@ int cmd_apply(int argc, const char **argv, const char *unused_prefix)
 		{ OPTION_CALLBACK, 0, "whitespace", &whitespace_option, "action",
 			"detect new or modified lines that have whitespace errors",
 			0, option_parse_whitespace },
+		OPT_BOOLEAN(0, "ignore-whitespace", &ignore_whitespace,
+			"ignore whitespace differences when finding context"),
 		OPT_BOOLEAN('R', "reverse", &apply_in_reverse,
 			"apply the patch in reverse"),
 		OPT_BOOLEAN(0, "unidiff-zero", &unidiff_zero,
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ddb71e2..d3415b5 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -663,6 +663,7 @@ _git_am ()
 	--*)
 		__gitcomp "
 			--3way --committer-date-is-author-date --ignore-date
+			--ignore-whitespace
 			--interactive --keep --no-utf8 --signoff --utf8
 			--whitespace=
 			"
@@ -684,6 +685,7 @@ _git_apply ()
 			--stat --numstat --summary --check --index
 			--cached --index-info --reverse --reject --unidiff-zero
 			--apply --no-add --exclude=
+			--ignore-whitespace
 			--whitespace= --inaccurate-eof --verbose
 			"
 		return
diff --git a/git-am.sh b/git-am.sh
index d64d997..fe024b1 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -16,6 +16,7 @@ s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
 whitespace=     pass it through git-apply
+ignore-whitespace pass it through git-apply
 directory=      pass it through git-apply
 C=              pass it through git-apply
 p=              pass it through git-apply
@@ -303,7 +304,7 @@ do
 		git_apply_opt="$git_apply_opt $(sq "$1$2")"; shift ;;
 	--patch-format)
 		shift ; patch_format="$1" ;;
-	--reject)
+	--reject|--ignore-whitespace)
 		git_apply_opt="$git_apply_opt $1" ;;
 	--committer-date-is-author-date)
 		committer_date_is_author_date=t ;;
diff --git a/git-rebase.sh b/git-rebase.sh
index 18bc694..d741752 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -333,6 +333,9 @@ do
 			;;
 		esac
 		;;
+	--ignore-whitespace)
+		git_am_opt="$git_am_opt $1"
+		;;
 	--committer-date-is-author-date|--ignore-date)
 		git_am_opt="$git_am_opt $1"
 		force_rebase=t
diff --git a/t/t4107-apply-ignore-whitespace.sh b/t/t4107-apply-ignore-whitespace.sh
new file mode 100755
index 0000000..b9e6f14
--- /dev/null
+++ b/t/t4107-apply-ignore-whitespace.sh
@@ -0,0 +1,107 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+# Copyright (c) 2005 Robert Fitzsimons
+#
+
+test_description='git-apply --ignore-whitespace.
+
+'
+. ./test-lib.sh
+
+# setup
+
+cat > patch1.patch <<\EOF
+diff --git a/main.c b/main.c
+new file mode 100644
+--- /dev/null
++++ b/main.c
+@@ -0,0 +1,22 @@
++#include <stdio.h>
++
++void print_int(int num);
++int func(int num);
++
++int main() {
++       int i;
++
++       for (i = 0; i < 10; i++) {
++               print_int(func(i)); /* stuff */
++       }
++
++       return 0;
++}
++
++int func(int num) {
++       return num * num;
++}
++
++void print_int(int num) {
++       printf("%d", num);
++}
+EOF
+cat > patch2.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -10,6 +10,8 @@
+ 		print_int(func(i)); /* stuff */
+ 	}
+ 
++	printf("\n");
++
+ 	return 0;
+ }
+ 
+EOF
+
+cat > patch3.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -10,1 +10,1 @@
+ 		print_int(func(i)); 
+EOF
+
+cat > patch4.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -21,1 +21,1 @@
+	}; 
+\ No newline at end of file
+EOF
+
+cat > patch5.patch <<\EOF
+diff --git a/main.c b/main.c
+--- a/main.c
++++ b/main.c
+@@ -2,1 +2,1 @@
+ 	void print_int(int num);
+EOF
+
+test_expect_success "S = patch1" \
+    'git-apply patch1.patch'
+
+test_expect_failure "F = patch2" \
+    'git-apply patch2.patch'
+
+test_expect_success "S = patch2 (--ignore-whitespace)" \
+    'git-apply --ignore-whitespace patch2.patch'
+
+test_expect_failure "S = patch3 (missing string at EOL)" \
+    'git-apply --ignore-whitespace patch3.patch'
+
+test_expect_failure "S = patch4 (missing EOL at EOF)" \
+    'git-apply --ignore-whitespace patch4.patch'
+
+test_expect_success "S = patch5 (leading whitespace)" \
+    'git-apply --ignore-whitespace patch5.patch'
+
+rm -f main.c
+test_expect_success "S = patch1 (--ignore-whitespace)" \
+    'git-apply --ignore-whitespace patch1.patch'
+
+test_done
+
+
-- 
1.6.3.3.512.g4fff

[PATCHv2 2/2] git apply: preserve original whitespace with --ignore-whitespace

[Giuseppe Bilotta]

---
 builtin-apply.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++-------
 1 files changed, 48 insertions(+), 7 deletions(-)

diff --git a/builtin-apply.c b/builtin-apply.c
index 01230f1..6c6ccbd 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -1639,10 +1639,17 @@ static int read_old_data(struct stat *st, const char *path, struct strbuf *buf)
 	}
 }
 
+/*
+ * Update the preimage, and the common lines in postimage,
+ * from buffer buf of length len. If postlen is 0 the postimage
+ * is updated in place, otherwise it's updated on a new buffer
+ * of length postlen
+ */
+
 static void update_pre_post_images(struct image *preimage,
 				   struct image *postimage,
 				   char *buf,
-				   size_t len)
+				   size_t len, size_t postlen)
 {
 	int i, ctx;
 	char *new, *old, *fixed;
@@ -1661,11 +1668,19 @@ static void update_pre_post_images(struct image *preimage,
 	*preimage = fixed_preimage;
 
 	/*
-	 * Adjust the common context lines in postimage, in place.
-	 * This is possible because whitespace fixing does not make
-	 * the string grow.
+	 * Adjust the common context lines in postimage. This can be
+	 * done in-place when we are just doing whitespace fixing,
+	 * which does not make the string grow, but needs a new buffer
+	 * when ignore_whitespace causes the update, since in this case
+	 * we could have e.g. tabs converted to multiple spaces.
+	 * We trust the caller to tell us if the update can be done
+	 * in place (postlen==0) or not.
 	 */
-	new = old = postimage->buf;
+	old = postimage->buf;
+	if (postlen)
+		new = postimage->buf = xmalloc(postlen);
+	else
+		new = old;
 	fixed = preimage->buf;
 	for (i = ctx = 0; i < postimage->nr; i++) {
 		size_t len = postimage->line[i].len;
@@ -1737,8 +1752,34 @@ static int match_fragment(struct image *img,
 	if ((match_end
 	     ? (try + preimage->len == img->len)
 	     : (try + preimage->len <= img->len)) &&
-	    lines_match(img->buf + try, preimage->buf, preimage->len))
+	    lines_match(img->buf + try, preimage->buf, preimage->len)) {
+		/*
+		 * When ignoring whitespace, we want to preserve the
+		 * target image whitespace in lines that are not modified,
+		 * so we update the preimage and the common lines in
+		 * the postimage to the target whitespace.
+		 */
+		if (ignore_whitespace) {
+			/*
+			 * New length for the updated pre and postimages
+			 */
+			size_t prelen = 0;
+			size_t postlen = postimage->len;
+			for (i = 0; i < preimage->nr; i++) {
+				size_t len = img->line[try_lno + i].len;
+				if (preimage->line[i].flag & LINE_COMMON)
+					postlen += len - preimage->line[i].len;
+				prelen += preimage->line[i].len = len;
+			}
+			fixed_buf = xmalloc(prelen);
+			memcpy(fixed_buf, img->buf + try, prelen);
+
+			update_pre_post_images(
+				preimage, postimage,
+				fixed_buf, prelen, postlen);
+		}
 		return 1;
+	}
 
 	if (ws_error_action != correct_ws_error)
 		return 0;
@@ -1799,7 +1840,7 @@ static int match_fragment(struct image *img,
 	 * hunk match.  Update the context lines in the postimage.
 	 */
 	update_pre_post_images(preimage, postimage,
-			       fixed_buf, buf - fixed_buf);
+			       fixed_buf, buf - fixed_buf, 0);
 	return 1;
 
  unmatch_exit:
-- 
1.6.3.3.512.g4fff

Re: [PATCHv2 1/2] git apply: option to ignore whitespace differences

[Ramsay Jones]

[Sorry for the late reply to this; I've been catching up on
email after several days away - so if this has already been
addressed, just ignore and sorry for the noise!]

Giuseppe Bilotta wrote:
[snip]
> + * Compare s1 to s2 up to length n, ignoring whitespace differences.
> + * It is acceptable if s2 is a substring of s1.
> + */
> +static int memcmp_ignore_whitespace(const char *s1, const char *s2, size_t n)
> +{
> +	const char *stop = s2 + n;
> +	int result;

result is uninitialized here...

> +
> +	if (!n)
> +		return 0;
> +
> +	/* skip leading whitespace */
> +	while (isspace(*s1))
> +		s1++;
> +	while (isspace(*s2))
> +		s2++;
> +	while (!result && s2 < stop) {

... and is still uninitialized the first time it hits
the loop condition...

> +		result = *s1++ - *s2++;

... so this may or may not be set...

> +		/*
> +		 * skip whitespace inside if we have whitespace
> +		 * on both buffers
> +		 */
> +		if (isspace(*s1) && isspace(*s2)) {
> +			while (isspace(*s1))
> +				s1++;
> +			while (isspace(*s2))
> +				s2++;
> +		}
> +	}
> +
> +	return result;

... so this may still be uninitialized.
> +}
> +

I haven't been following this thread; I just noticed this problem
as the patch floated past...

ATB,
Ramsay Jones