[PATCH v2] diff -c -p: do not die on submodules

Subsystems: the rest

DORMANTno replies

7 messages, 4 authors, 2016-06-15 · open the first message on its own page

[PATCH v2] diff -c -p: do not die on submodules

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:46:41

The combine diff logic knew only about blobs (and their checked-out form
in the work tree, either regular files or symlinks), and barfed when fed
submodules.  This "externalizes" gitlinks in the same way as the normal
patch generation codepath does (i.e. "Subproject commit Xxx\n") to fix the
issue.

Signed-off-by: Junio C Hamano <redacted>
---

 This is a re-roll of the earlier "[PATCH] Teach gitlinks to combine-diff",
 meant to apply to maint-1.6.0 and later.

 combine-diff.c            |   38 ++++++++++++++++++++++++++------------
 t/t4027-diff-submodule.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+), 12 deletions(-)
diff --git a/combine-diff.c b/combine-diff.c
index aa9d79e..f617e9d 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -6,6 +6,7 @@
 #include "quote.h"
 #include "xdiff-interface.h"
 #include "log-tree.h"
+#include "refs.h"
 
 static struct combine_diff_path *intersect_paths(struct combine_diff_path *curr, int n, int num_parent)
 {
@@ -90,18 +91,24 @@ struct sline {
 	unsigned long *p_lno;
 };
 
-static char *grab_blob(const unsigned char *sha1, unsigned long *size)
+static char *grab_blob(const unsigned char *sha1, unsigned int mode, unsigned long *size)
 {
 	char *blob;
 	enum object_type type;
-	if (is_null_sha1(sha1)) {
+
+	if (S_ISGITLINK(mode)) {
+		blob = xmalloc(100);
+		*size = snprintf(blob, 100,
+				 "Subproject commit %s\n", sha1_to_hex(sha1));
+	} else if (is_null_sha1(sha1)) {
 		/* deleted blob */
 		*size = 0;
 		return xcalloc(1, 1);
+	} else {
+		blob = read_sha1_file(sha1, &type, size);
+		if (type != OBJ_BLOB)
+			die("object '%s' is not a blob!", sha1_to_hex(sha1));
 	}
-	blob = read_sha1_file(sha1, &type, size);
-	if (type != OBJ_BLOB)
-		die("object '%s' is not a blob!", sha1_to_hex(sha1));
 	return blob;
 }
 
@@ -197,7 +204,8 @@ static void consume_line(void *state_, char *line, unsigned long len)
 	}
 }
 
-static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
+static void combine_diff(const unsigned char *parent, unsigned int mode,
+			 mmfile_t *result_file,
 			 struct sline *sline, unsigned int cnt, int n,
 			 int num_parent)
 {
@@ -213,7 +221,7 @@ static void combine_diff(const unsigned char *parent, mmfile_t *result_file,
 	if (!cnt)
 		return; /* result deleted */
 
-	parent_file.ptr = grab_blob(parent, &sz);
+	parent_file.ptr = grab_blob(parent, mode, &sz);
 	parent_file.size = sz;
 	xpp.flags = XDF_NEED_MINIMAL;
 	memset(&xecfg, 0, sizeof(xecfg));
@@ -692,7 +700,7 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	context = opt->context;
 	/* Read the result of merge first */
 	if (!working_tree_file)
-		result = grab_blob(elem->sha1, &result_size);
+		result = grab_blob(elem->sha1, elem->mode, &result_size);
 	else {
 		/* Used by diff-tree to read from the working tree */
 		struct stat st;
@@ -712,9 +720,13 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			}
 			result[len] = 0;
 			elem->mode = canon_mode(st.st_mode);
-		}
-		else if (0 <= (fd = open(elem->path, O_RDONLY)) &&
-			 !fstat(fd, &st)) {
+		} else if (S_ISDIR(st.st_mode)) {
+			unsigned char sha1[20];
+			if (resolve_gitlink_ref(elem->path, "HEAD", sha1) < 0)
+				result = grab_blob(elem->sha1, elem->mode, &result_size);
+			else
+				result = grab_blob(sha1, elem->mode, &result_size);
+		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
 			size_t len = xsize_t(st.st_size);
 			ssize_t done;
 			int is_file, i;
@@ -807,7 +819,9 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 			}
 		}
 		if (i <= j)
-			combine_diff(elem->parent[i].sha1, &result_file, sline,
+			combine_diff(elem->parent[i].sha1,
+				     elem->parent[i].mode,
+				     &result_file, sline,
 				     cnt, i, num_parent);
 		if (elem->parent[i].mode != elem->mode)
 			mode_differs = 1;
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index ba6679c..718efe8 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -57,4 +57,43 @@ test_expect_success 'git diff (empty submodule dir)' '
 	test_cmp empty actual.empty
 '
 
+test_expect_success 'conflicted submodule setup' '
+
+	# 39 efs
+	c=fffffffffffffffffffffffffffffffffffffff
+	(
+		echo "000000 $_z40 0	sub"
+		echo "160000 1$c 1	sub"
+		echo "160000 2$c 2	sub"
+		echo "160000 3$c 3	sub"
+	) | git update-index --index-info &&
+	echo >expect.nosub '\''diff --cc sub
+index 2ffffff,3ffffff..0000000
+--- a/sub
++++ b/sub
+@@@ -1,1 -1,1 +1,1 @@@
+- Subproject commit 2fffffffffffffffffffffffffffffffffffffff
+ -Subproject commit 3fffffffffffffffffffffffffffffffffffffff
+++Subproject commit 0000000000000000000000000000000000000000'\'' &&
+
+	hh=$(git rev-parse HEAD) &&
+	sed -e "s/$_z40/$hh/" expect.nosub >expect.withsub
+
+'
+
+test_expect_success 'combined (empty submodule)' '
+	rm -fr sub && mkdir sub &&
+	git diff >actual &&
+	test_cmp expect.nosub actual
+'
+
+test_expect_success 'combined (with submodule)' '
+	rm -fr sub &&
+	git clone --no-checkout . sub &&
+	git diff >actual &&
+	test_cmp expect.withsub actual
+'
+
+
+
 test_done
-- 
1.6.3.rc3.16.gb37759

Re: [PATCH v2] diff -c -p: do not die on submodules

From: Alex Riesen <hidden>
Date: 2016-06-15 22:46:41

2009/4/29 Junio C Hamano [off-list ref]:
+
+       if (S_ISGITLINK(mode)) {
+               blob = xmalloc(100);
+               *size = snprintf(blob, 100,
+                                "Subproject commit %s\n", sha1_to_hex(sha1));
snprintf returns a signed value. It also has a bad record of returning
negative values for obscure reasons (on obscure platforms, admittedly).

For this particular case,

  strcpy(blob, "Subproject commit ");
  strcat(blob, sha1_to_hex(sha1));
  strcat(blob, "\n");
  *size = strlen(blob); /* that's a constant */

could be considered.

Re: [PATCH v2] diff -c -p: do not die on submodules

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:46:41

Hi,

On Wed, 29 Apr 2009, Alex Riesen wrote:
2009/4/29 Junio C Hamano [off-list ref]:
quoted
+
+       if (S_ISGITLINK(mode)) {
+               blob = xmalloc(100);
+               *size = snprintf(blob, 100,
+                                "Subproject commit %s\n", sha1_to_hex(sha1));
snprintf returns a signed value. It also has a bad record of returning
negative values for obscure reasons (on obscure platforms, admittedly).

For this particular case,

  strcpy(blob, "Subproject commit ");
  strcat(blob, sha1_to_hex(sha1));
  strcat(blob, "\n");
  *size = strlen(blob); /* that's a constant */

could be considered.
Actually, we know _exactly_ the size of the thing.  It is 18+40+1.  But I 
think that *size wants to have the size, not the length.  So add 1.

In any case, I don't think that we have to jump through hoops here: 
snprintf() is _most_ unlikely to return something negative here.  So I'd 
say that readability trumps paranoia here.

Ciao,
Dscho

Re: [PATCH v2] diff -c -p: do not die on submodules

From: Alex Riesen <hidden>
Date: 2016-06-15 22:46:41

2009/4/30 Johannes Schindelin [off-list ref]:
Hi,

On Wed, 29 Apr 2009, Alex Riesen wrote:
quoted
2009/4/29 Junio C Hamano [off-list ref]:
quoted
+
+       if (S_ISGITLINK(mode)) {
+               blob = xmalloc(100);
+               *size = snprintf(blob, 100,
+                                "Subproject commit %s\n", sha1_to_hex(sha1));
snprintf returns a signed value. It also has a bad record of returning
negative values for obscure reasons (on obscure platforms, admittedly).

For this particular case,

  strcpy(blob, "Subproject commit ");
  strcat(blob, sha1_to_hex(sha1));
  strcat(blob, "\n");
  *size = strlen(blob); /* that's a constant */

could be considered.
Actually, we know _exactly_ the size of the thing.  It is 18+40+1.  But I
think that *size wants to have the size, not the length.  So add 1.

In any case, I don't think that we have to jump through hoops here:
snprintf() is _most_ unlikely to return something negative here.  So I'd
say that readability trumps paranoia here.
http://www.google.com/search?q=snprintf+negative+return+value

First link: http://bytes.com/groups/c/590845-snprintf-return-value

Look for "(Windows, mingw)"

Re: [PATCH v2] diff -c -p: do not die on submodules

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:46:41

Hi,

On Thu, 30 Apr 2009, Alex Riesen wrote:
2009/4/30 Johannes Schindelin [off-list ref]:
quoted
Hi,

On Wed, 29 Apr 2009, Alex Riesen wrote:
quoted
2009/4/29 Junio C Hamano [off-list ref]:
quoted
+
+       if (S_ISGITLINK(mode)) {
+               blob = xmalloc(100);
+               *size = snprintf(blob, 100,
+                                "Subproject commit %s\n", sha1_to_hex(sha1));
snprintf returns a signed value. It also has a bad record of returning
negative values for obscure reasons (on obscure platforms, admittedly).

For this particular case,

  strcpy(blob, "Subproject commit ");
  strcat(blob, sha1_to_hex(sha1));
  strcat(blob, "\n");
  *size = strlen(blob); /* that's a constant */

could be considered.
Actually, we know _exactly_ the size of the thing.  It is 18+40+1.  But I
think that *size wants to have the size, not the length.  So add 1.

In any case, I don't think that we have to jump through hoops here:
snprintf() is _most_ unlikely to return something negative here.  So I'd
say that readability trumps paranoia here.
http://www.google.com/search?q=snprintf+negative+return+value

First link: http://bytes.com/groups/c/590845-snprintf-return-value

Look for "(Windows, mingw)"
No, I will not.

This is a _lousy_ time balance you propose: yourself saving a few minutes, 
everybody spending more time, trying to figure out what your point is, 
only so that you do not have to summarize in a concise manner why you 
thing that I am wrong.

I fully disagree with that, and refuse to obey your command,
Dscho

Re: [PATCH v2] diff -c -p: do not die on submodules

From: Alex Riesen <hidden>
Date: 2016-06-15 22:46:41

2009/4/30 Johannes Schindelin [off-list ref]:
On Thu, 30 Apr 2009, Alex Riesen wrote:
quoted
http://www.google.com/search?q=snprintf+negative+return+value

First link: http://bytes.com/groups/c/590845-snprintf-return-value

Look for "(Windows, mingw)"
No, I will not.
"The following are the results from different systems:
gcc 3.3 (Mac OS X): 10
gcc 3.4.5 (Windows, mingw): -1
BC++ 5.5.1 (Windows): 10
MS VC++ 6.0 and 2005 (Windows, _snprintf() used): -1
DigitalMars 8.42n (Windows): -1"

Re: [PATCH v2] diff -c -p: do not die on submodules

From: Finn Arne Gangstad <hidden>
Date: 2016-06-15 22:46:41

On Thu, Apr 30, 2009 at 07:47:57AM +0200, Alex Riesen wrote:
2009/4/30 Johannes Schindelin [off-list ref]:
quoted
On Thu, 30 Apr 2009, Alex Riesen wrote:
quoted
http://www.google.com/search?q=snprintf+negative+return+value

First link: http://bytes.com/groups/c/590845-snprintf-return-value

Look for "(Windows, mingw)"
No, I will not.
"The following are the results from different systems:
gcc 3.3 (Mac OS X): 10
gcc 3.4.5 (Windows, mingw): -1
BC++ 5.5.1 (Windows): 10
MS VC++ 6.0 and 2005 (Windows, _snprintf() used): -1
DigitalMars 8.42n (Windows): -1"
snprintf always (on all these arechitectures) returns the number of
characters written if the buffer is large enough. In the code we are
discussing here the buffer is always large enough, so this is not a
problem.

- Finn Arne
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help