On 2019/10/03 10:45, Stefan Sperling wrote:
> On Wed, Oct 02, 2019 at 09:28:35AM +0100, Stuart Henderson wrote:
> > There's a problem doing a checkout when a tree contains an empty dir.
> 
> Thanks for the report, Stuart.
> 
> I would pin-point problem in 'got import' rather than 'got checkout'.
> 
> Git does not allow empty directories to be added to version control.
> This is because Git tracks files only. Directories appear as a mere
> side-effect of the existence of a versioned file at a particular path.
> 
> We could in fact allow 'got import' to import empty directories and have
> 'got checkout' check them out. This would work. However, Git will ignore
> empty directories, and Got users cannot add additional empty directories with
> 'got add' later. Making 'got import' special in this regard seems like a bad
> idea so I'd prefer to prevent 'got import' from importing empty directories.
> 
> > Sorry I don't have time to figure out the regress infrastructure to add a
> > proper test for this and wanted to send a mail before I forget, but here's
> > a simple reproducer:
> 
> Below is a fix and a regress test based on your reproduction recipe.
> After reading the test I hope you will agree that the regress
> "infrastructure" is not all that complicated :)
> 
> Does this look good?

Thanks, that makes sense and works how I'd expect.


> diff --git a/lib/object_parse.c b/lib/object_parse.c
> index 4066dbb..a037ceb 100644
> --- a/lib/object_parse.c
> +++ b/lib/object_parse.c
> @@ -728,15 +728,15 @@ got_object_parse_tree(struct got_tree_object **tree, uint8_t *buf, size_t len)
>  
>  	TAILQ_INIT(&pathlist);
>  
> -	if (remain == 0)
> -		return got_error(GOT_ERR_BAD_OBJ_DATA);
> -
>  	*tree = calloc(1, sizeof(**tree));
>  	if (*tree == NULL)
>  		return got_error_from_errno("calloc");
>  
>  	SIMPLEQ_INIT(&(*tree)->entries.head);
>  
> +	if (remain == 0)
> +		return NULL; /* tree is empty */
> +
>  	while (remain > 0) {
>  		struct got_tree_entry *te;
>  		struct got_pathlist_entry *new = NULL;
> diff --git a/lib/repository.c b/lib/repository.c
> index b32361c..22355c1 100644
> --- a/lib/repository.c
> +++ b/lib/repository.c
> @@ -1492,8 +1492,12 @@ write_tree(struct got_object_id **new_tree_id, const char *path_dir,
>  		if (de->d_type == DT_DIR) {
>  			err = import_subdir(&new_te, de, path_dir,
>  			    ignores, repo, progress_cb, progress_arg);
> -			if (err)
> -				goto done;
> +			if (err) {
> +				if (err->code != GOT_ERR_NO_TREE_ENTRY)
> +					goto done;
> +				err = NULL;
> +				continue;
> +			}
>  		} else if (de->d_type == DT_REG) {
>  			err = import_file(&new_te, de, path_dir, repo);
>  			if (err)
> @@ -1506,6 +1510,11 @@ write_tree(struct got_object_id **new_tree_id, const char *path_dir,
>  			goto done;
>  	}
>  
> +	if (TAILQ_EMPTY(&paths)) {
> +		err = got_error(GOT_ERR_NO_TREE_ENTRY);
> +		goto done;
> +	}
> +
>  	TAILQ_FOREACH(pe, &paths, entry) {
>  		struct got_tree_entry *te = pe->data;
>  		char *path;
> diff --git a/regress/cmdline/import.sh b/regress/cmdline/import.sh
> index ce3cb34..42ff367 100755
> --- a/regress/cmdline/import.sh
> +++ b/regress/cmdline/import.sh
> @@ -196,10 +196,52 @@ function test_import_ignores {
>  		diff -u $testroot/stdout.expected $testroot/stdout
>  	fi
>  	test_done "$testroot" "$ret"
> +}
> +
> +function test_import_empty_dir {
> +	local testname=import_empty_dir
> +	local testroot=`mktemp -p /tmp -d got-test-$testname-XXXXXXXX`
> +
> +	got init $testroot/repo
> +
> +	mkdir $testroot/tree
> +	mkdir -p $testroot/tree/empty $testroot/tree/notempty
> +	echo "alpha" > $testroot/tree/notempty/alpha
> +
> +	got import -m 'init' -r $testroot/repo $testroot/tree > $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
>  
> +	local head_commit=`git_show_head $testroot/repo`
> +	echo "A  $testroot/tree/notempty/alpha" >> $testroot/stdout.expected
> +	echo "Created branch refs/heads/master with commit $head_commit" \
> +		>> $testroot/stdout.expected
>  
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	# Verify that Got did not import the empty directory
> +	echo "notempty/" > $testroot/stdout.expected
> +	echo "notempty/alpha" >> $testroot/stdout.expected
> +
> +	got tree -r $testroot/repo -R > $testroot/stdout
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret="$?"
> +	if [ "$ret" != "0" ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +	fi
> +	test_done "$testroot" "$ret"
>  }
>  
>  run_test test_import_basic
>  run_test test_import_requires_new_branch
>  run_test test_import_ignores
> +run_test test_import_empty_dir
>