Commits

Anonymous committed 03adeea Merge

Merge branch 'jk/maint-null-in-trees' into maint-1.7.11

"git diff" had a confusion between taking data from a path in the
working tree and taking data from an object that happens to have
name 0{40} recorded in a tree.

* jk/maint-null-in-trees:
fsck: detect null sha1 in tree entries
do not write null sha1s to on-disk index
diff: do not use null sha1 as a sentinel value

Comments (0)

Files changed (18)

 struct diff_options;
 extern void setup_diff_pager(struct diff_options *);
 
-extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, char **buf, unsigned long *buf_size);
+extern int textconv_object(const char *path, unsigned mode, const unsigned char *sha1, int sha1_valid, char **buf, unsigned long *buf_size);
 
 extern int cmd_add(int argc, const char **argv, const char *prefix);
 extern int cmd_annotate(int argc, const char **argv, const char *prefix);
 int textconv_object(const char *path,
 		    unsigned mode,
 		    const unsigned char *sha1,
+		    int sha1_valid,
 		    char **buf,
 		    unsigned long *buf_size)
 {
 	struct userdiff_driver *textconv;
 
 	df = alloc_filespec(path);
-	fill_filespec(df, sha1, mode);
+	fill_filespec(df, sha1, sha1_valid, mode);
 	textconv = get_textconv(df);
 	if (!textconv) {
 		free_filespec(df);
 
 		num_read_blob++;
 		if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
-		    textconv_object(o->path, o->mode, o->blob_sha1, &file->ptr, &file_size))
+		    textconv_object(o->path, o->mode, o->blob_sha1, 1, &file->ptr, &file_size))
 			;
 		else
 			file->ptr = read_sha1_file(o->blob_sha1, &type, &file_size);
 		switch (st.st_mode & S_IFMT) {
 		case S_IFREG:
 			if (DIFF_OPT_TST(opt, ALLOW_TEXTCONV) &&
-			    textconv_object(read_from, mode, null_sha1, &buf_ptr, &buf_len))
+			    textconv_object(read_from, mode, null_sha1, 0, &buf_ptr, &buf_len))
 				strbuf_attach(&buf, buf_ptr, buf_len, buf_len + 1);
 			else if (strbuf_read_file(&buf, read_from, st.st_size) != st.st_size)
 				die_errno("cannot open or read '%s'", read_from);
 			die("no such path %s in %s", path, final_commit_name);
 
 		if (DIFF_OPT_TST(&sb.revs->diffopt, ALLOW_TEXTCONV) &&
-		    textconv_object(path, o->mode, o->blob_sha1, (char **) &sb.final_buf,
+		    textconv_object(path, o->mode, o->blob_sha1, 1, (char **) &sb.final_buf,
 				    &sb.final_buf_size))
 			;
 		else
 			die("git cat-file --textconv %s: <object> must be <sha1:path>",
 			    obj_name);
 
-		if (!textconv_object(obj_context.path, obj_context.mode, sha1, &buf, &size))
+		if (!textconv_object(obj_context.path, obj_context.mode, sha1, 1, &buf, &size))
 			die("git cat-file --textconv: unable to run textconv on %s",
 			    obj_name);
 		break;
 			 unsigned old_mode, unsigned new_mode,
 			 const unsigned char *old_sha1,
 			 const unsigned char *new_sha1,
+			 int old_sha1_valid,
+			 int new_sha1_valid,
 			 const char *old_name,
 			 const char *new_name)
 {
 
 	one = alloc_filespec(old_name);
 	two = alloc_filespec(new_name);
-	fill_filespec(one, old_sha1, old_mode);
-	fill_filespec(two, new_sha1, new_mode);
+	fill_filespec(one, old_sha1, old_sha1_valid, old_mode);
+	fill_filespec(two, new_sha1, new_sha1_valid, new_mode);
 
 	diff_queue(&diff_queued_diff, one, two);
 }
 	stuff_change(&revs->diffopt,
 		     blob[0].mode, canon_mode(st.st_mode),
 		     blob[0].sha1, null_sha1,
+		     1, 0,
 		     path, path);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	stuff_change(&revs->diffopt,
 		     blob[0].mode, blob[1].mode,
 		     blob[0].sha1, blob[1].sha1,
+		     1, 1,
 		     blob[0].name, blob[1].name);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 		return xcalloc(1, 1);
 	} else if (textconv) {
 		struct diff_filespec *df = alloc_filespec(path);
-		fill_filespec(df, sha1, mode);
+		fill_filespec(df, sha1, 1, mode);
 		*size = fill_textconv(textconv, df, &blob);
 		free_filespec(df);
 	} else {
 						   &result_size, NULL, NULL);
 		} else if (textconv) {
 			struct diff_filespec *df = alloc_filespec(elem->path);
-			fill_filespec(df, null_sha1, st.st_mode);
+			fill_filespec(df, null_sha1, 0, st.st_mode);
 			result_size = fill_textconv(textconv, df, &result);
 			free_filespec(df);
 		} else if (0 <= (fd = open(elem->path, O_RDONLY))) {
 			if (silent_on_removed)
 				continue;
 			diff_addremove(&revs->diffopt, '-', ce->ce_mode,
-				       ce->sha1, ce->name, 0);
+				       ce->sha1, !is_null_sha1(ce->sha1),
+				       ce->name, 0);
 			continue;
 		}
 		changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
 		newmode = ce_mode_from_stat(ce, st.st_mode);
 		diff_change(&revs->diffopt, oldmode, newmode,
 			    ce->sha1, (changed ? null_sha1 : ce->sha1),
+			    !is_null_sha1(ce->sha1), (changed ? 0 : !is_null_sha1(ce->sha1)),
 			    ce->name, 0, dirty_submodule);
 
 	}
 static void diff_index_show_file(struct rev_info *revs,
 				 const char *prefix,
 				 struct cache_entry *ce,
-				 const unsigned char *sha1, unsigned int mode,
+				 const unsigned char *sha1, int sha1_valid,
+				 unsigned int mode,
 				 unsigned dirty_submodule)
 {
 	diff_addremove(&revs->diffopt, prefix[0], mode,
-		       sha1, ce->name, dirty_submodule);
+		       sha1, sha1_valid, ce->name, dirty_submodule);
 }
 
 static int get_stat_data(struct cache_entry *ce,
 	    &dirty_submodule, &revs->diffopt) < 0)
 		return;
 
-	diff_index_show_file(revs, "+", new, sha1, mode, dirty_submodule);
+	diff_index_show_file(revs, "+", new, sha1, !is_null_sha1(sha1), mode, dirty_submodule);
 }
 
 static int show_modified(struct rev_info *revs,
 			  &dirty_submodule, &revs->diffopt) < 0) {
 		if (report_missing)
 			diff_index_show_file(revs, "-", old,
-					     old->sha1, old->ce_mode, 0);
+					     old->sha1, 1, old->ce_mode, 0);
 		return -1;
 	}
 
 		return 0;
 
 	diff_change(&revs->diffopt, oldmode, mode,
-		    old->sha1, sha1, old->name, 0, dirty_submodule);
+		    old->sha1, sha1, 1, !is_null_sha1(sha1),
+		    old->name, 0, dirty_submodule);
 	return 0;
 }
 
 		struct diff_filepair *pair;
 		pair = diff_unmerge(&revs->diffopt, idx->name);
 		if (tree)
-			fill_filespec(pair->one, tree->sha1, tree->ce_mode);
+			fill_filespec(pair->one, tree->sha1, 1, tree->ce_mode);
 		return;
 	}
 
 	 * Something removed from the tree?
 	 */
 	if (!idx) {
-		diff_index_show_file(revs, "-", tree, tree->sha1, tree->ce_mode, 0);
+		diff_index_show_file(revs, "-", tree, tree->sha1, 1, tree->ce_mode, 0);
 		return;
 	}
 
 	if (!name)
 		name = "/dev/null";
 	s = alloc_filespec(name);
-	fill_filespec(s, null_sha1, mode);
+	fill_filespec(s, null_sha1, 0, mode);
 	if (name == file_from_standard_input)
 		populate_from_stdin(s);
 	return s;
 }
 
 void fill_filespec(struct diff_filespec *spec, const unsigned char *sha1,
-		   unsigned short mode)
+		   int sha1_valid, unsigned short mode)
 {
 	if (mode) {
 		spec->mode = canon_mode(mode);
 		hashcpy(spec->sha1, sha1);
-		spec->sha1_valid = !is_null_sha1(sha1);
+		spec->sha1_valid = sha1_valid;
 	}
 }
 
 void diff_addremove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
+		    int sha1_valid,
 		    const char *concatpath, unsigned dirty_submodule)
 {
 	struct diff_filespec *one, *two;
 	two = alloc_filespec(concatpath);
 
 	if (addremove != '+')
-		fill_filespec(one, sha1, mode);
+		fill_filespec(one, sha1, sha1_valid, mode);
 	if (addremove != '-') {
-		fill_filespec(two, sha1, mode);
+		fill_filespec(two, sha1, sha1_valid, mode);
 		two->dirty_submodule = dirty_submodule;
 	}
 
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
+		 int old_sha1_valid, int new_sha1_valid,
 		 const char *concatpath,
 		 unsigned old_dirty_submodule, unsigned new_dirty_submodule)
 {
 		const unsigned char *tmp_c;
 		tmp = old_mode; old_mode = new_mode; new_mode = tmp;
 		tmp_c = old_sha1; old_sha1 = new_sha1; new_sha1 = tmp_c;
+		tmp = old_sha1_valid; old_sha1_valid = new_sha1_valid;
+			new_sha1_valid = tmp;
 		tmp = old_dirty_submodule; old_dirty_submodule = new_dirty_submodule;
 			new_dirty_submodule = tmp;
 	}
 
 	one = alloc_filespec(concatpath);
 	two = alloc_filespec(concatpath);
-	fill_filespec(one, old_sha1, old_mode);
-	fill_filespec(two, new_sha1, new_mode);
+	fill_filespec(one, old_sha1, old_sha1_valid, old_mode);
+	fill_filespec(two, new_sha1, new_sha1_valid, new_mode);
 	one->dirty_submodule = old_dirty_submodule;
 	two->dirty_submodule = new_dirty_submodule;
 
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
+		 int old_sha1_valid, int new_sha1_valid,
 		 const char *fullpath,
 		 unsigned old_dirty_submodule, unsigned new_dirty_submodule);
 
 typedef void (*add_remove_fn_t)(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
+		    int sha1_valid,
 		    const char *fullpath, unsigned dirty_submodule);
 
 typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
 			   int addremove,
 			   unsigned mode,
 			   const unsigned char *sha1,
+			   int sha1_valid,
 			   const char *fullpath, unsigned dirty_submodule);
 
 extern void diff_change(struct diff_options *,
 			unsigned mode1, unsigned mode2,
 			const unsigned char *sha1,
 			const unsigned char *sha2,
+			int sha1_valid,
+			int sha2_valid,
 			const char *fullpath,
 			unsigned dirty_submodule1, unsigned dirty_submodule2);
 
 		memmove(rename_dst + first + 1, rename_dst + first,
 			(rename_dst_nr - first - 1) * sizeof(*rename_dst));
 	rename_dst[first].two = alloc_filespec(two->path);
-	fill_filespec(rename_dst[first].two, two->sha1, two->mode);
+	fill_filespec(rename_dst[first].two, two->sha1, two->sha1_valid, two->mode);
 	rename_dst[first].pair = NULL;
 	return &(rename_dst[first]);
 }
 extern struct diff_filespec *alloc_filespec(const char *);
 extern void free_filespec(struct diff_filespec *);
 extern void fill_filespec(struct diff_filespec *, const unsigned char *,
-			  unsigned short);
+			  int, unsigned short);
 
 extern int diff_populate_filespec(struct diff_filespec *, int);
 extern void diff_free_filespec_data(struct diff_filespec *);
 static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 {
 	int retval;
+	int has_null_sha1 = 0;
 	int has_full_path = 0;
 	int has_empty_name = 0;
 	int has_zero_pad = 0;
 	while (desc.size) {
 		unsigned mode;
 		const char *name;
+		const unsigned char *sha1;
 
-		tree_entry_extract(&desc, &name, &mode);
+		sha1 = tree_entry_extract(&desc, &name, &mode);
 
+		if (is_null_sha1(sha1))
+			has_null_sha1 = 1;
 		if (strchr(name, '/'))
 			has_full_path = 1;
 		if (!*name)
 	}
 
 	retval = 0;
+	if (has_null_sha1)
+		retval += error_func(&item->object, FSCK_WARN, "contains entries pointing to null sha1");
 	if (has_full_path)
 		retval += error_func(&item->object, FSCK_WARN, "contains full pathnames");
 	if (has_empty_name)
 			continue;
 		if (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))
 			ce_smudge_racily_clean_entry(ce);
+		if (is_null_sha1(ce->sha1))
+			return error("cache entry has null sha1: %s", ce->name);
 		if (ce_write_entry(&c, newfd, ce, previous_name) < 0)
 			return -1;
 	}
 static void file_add_remove(struct diff_options *options,
 		    int addremove, unsigned mode,
 		    const unsigned char *sha1,
+		    int sha1_valid,
 		    const char *fullpath, unsigned dirty_submodule)
 {
 	int diff = addremove == '+' ? REV_TREE_NEW : REV_TREE_OLD;
 		 unsigned old_mode, unsigned new_mode,
 		 const unsigned char *old_sha1,
 		 const unsigned char *new_sha1,
+		 int old_sha1_valid, int new_sha1_valid,
 		 const char *fullpath,
 		 unsigned old_dirty_submodule, unsigned new_dirty_submodule)
 {
 	grep -q "error: sha1 mismatch 63ffffffffffffffffffffffffffffffffffffff" out
 '
 
+_bz='\0'
+_bz5="$_bz$_bz$_bz$_bz$_bz"
+_bz20="$_bz5$_bz5$_bz5$_bz5"
+
+test_expect_success 'fsck notices blob entry pointing to null sha1' '
+	(git init null-blob &&
+	 cd null-blob &&
+	 sha=$(printf "100644 file$_bz$_bz20" |
+	       git hash-object -w --stdin -t tree) &&
+	  git fsck 2>out &&
+	  cat out &&
+	  grep "warning.*null sha1" out
+	)
+'
+
+test_expect_success 'fsck notices submodule entry pointing to null sha1' '
+	(git init null-commit &&
+	 cd null-commit &&
+	 sha=$(printf "160000 submodule$_bz$_bz20" |
+	       git hash-object -w --stdin -t tree) &&
+	  git fsck 2>out &&
+	  cat out &&
+	  grep "warning.*null sha1" out
+	)
+'
+
 test_done

t/t2107-update-index-basic.sh

 	grep "[Uu]sage: git update-index" broken/usage
 '
 
+test_expect_success '--cacheinfo does not accept blob null sha1' '
+	echo content >file &&
+	git add file &&
+	git rev-parse :file >expect &&
+	test_must_fail git update-index --cacheinfo 100644 $_z40 file &&
+	git rev-parse :file >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--cacheinfo does not accept gitlink null sha1' '
+	git init submodule &&
+	(cd submodule && test_commit foo) &&
+	git add submodule &&
+	git rev-parse :submodule >expect &&
+	test_must_fail git update-index --cacheinfo 160000 $_z40 submodule &&
+	git rev-parse :submodule >actual &&
+	test_cmp expect actual
+'
+
 test_done

t/t4054-diff-bogus-tree.sh

+#!/bin/sh
+
+test_description='test diff with a bogus tree containing the null sha1'
+. ./test-lib.sh
+
+empty_tree=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+
+test_expect_success 'create bogus tree' '
+	bogus_tree=$(
+		printf "100644 fooQQQQQQQQQQQQQQQQQQQQQ" |
+		q_to_nul |
+		git hash-object -w --stdin -t tree
+	)
+'
+
+test_expect_success 'create tree with matching file' '
+	echo bar >foo &&
+	git add foo &&
+	good_tree=$(git write-tree)
+	blob=$(git rev-parse :foo)
+'
+
+test_expect_success 'raw diff shows null sha1 (addition)' '
+	echo ":000000 100644 $_z40 $_z40 A	foo" >expect &&
+	git diff-tree $empty_tree $bogus_tree >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'raw diff shows null sha1 (removal)' '
+	echo ":100644 000000 $_z40 $_z40 D	foo" >expect &&
+	git diff-tree $bogus_tree $empty_tree >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'raw diff shows null sha1 (modification)' '
+	echo ":100644 100644 $blob $_z40 M	foo" >expect &&
+	git diff-tree $good_tree $bogus_tree >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'raw diff shows null sha1 (other direction)' '
+	echo ":100644 100644 $_z40 $blob M	foo" >expect &&
+	git diff-tree $bogus_tree $good_tree >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'raw diff shows null sha1 (reverse)' '
+	echo ":100644 100644 $_z40 $blob M	foo" >expect &&
+	git diff-tree -R $good_tree $bogus_tree >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'raw diff shows null sha1 (index)' '
+	echo ":100644 100644 $_z40 $blob M	foo" >expect &&
+	git diff-index $bogus_tree >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'patch fails due to bogus sha1 (addition)' '
+	test_must_fail git diff-tree -p $empty_tree $bogus_tree
+'
+
+test_expect_success 'patch fails due to bogus sha1 (removal)' '
+	test_must_fail git diff-tree -p $bogus_tree $empty_tree
+'
+
+test_expect_success 'patch fails due to bogus sha1 (modification)' '
+	test_must_fail git diff-tree -p $good_tree $bogus_tree
+'
+
+test_expect_success 'patch fails due to bogus sha1 (other direction)' '
+	test_must_fail git diff-tree -p $bogus_tree $good_tree
+'
+
+test_expect_success 'patch fails due to bogus sha1 (reverse)' '
+	test_must_fail git diff-tree -R -p $good_tree $bogus_tree
+'
+
+test_expect_success 'patch fails due to bogus sha1 (index)' '
+	test_must_fail git diff-index -p $bogus_tree
+'
+
+test_done
 	if (DIFF_OPT_TST(opt, RECURSIVE) && S_ISDIR(mode1)) {
 		if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE)) {
 			opt->change(opt, mode1, mode2,
-				    sha1, sha2, base->buf, 0, 0);
+				    sha1, sha2, 1, 1, base->buf, 0, 0);
 		}
 		strbuf_addch(base, '/');
 		diff_tree_sha1(sha1, sha2, base->buf, opt);
 	} else {
-		opt->change(opt, mode1, mode2, sha1, sha2, base->buf, 0, 0);
+		opt->change(opt, mode1, mode2, sha1, sha2, 1, 1, base->buf, 0, 0);
 	}
 	strbuf_setlen(base, old_baselen);
 	return 0;
 			die("corrupt tree sha %s", sha1_to_hex(sha1));
 
 		if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))
-			opt->add_remove(opt, *prefix, mode, sha1, base->buf, 0);
+			opt->add_remove(opt, *prefix, mode, sha1, 1, base->buf, 0);
 
 		strbuf_addch(base, '/');
 
 		show_tree(opt, prefix, &inner, base);
 		free(tree);
 	} else
-		opt->add_remove(opt, prefix[0], mode, sha1, base->buf, 0);
+		opt->add_remove(opt, prefix[0], mode, sha1, 1, base->buf, 0);
 
 	strbuf_setlen(base, old_baselen);
 }