Subject: Re: [PATCH v4 8/8] use smudgeToFile filter in recursive merge

From: Junio C Hamano <[email-masked]>
Date: Wed, 22 Jun 2016 14:39:24 -0700

Joey Hess <[email-masked]> writes:

> @@ -781,6 +773,7 @@ static void update_file_flags(struct merge_options *o,
>  		}
>  		if (S_ISREG(mode) || (!has_symlinks && S_ISLNK(mode))) {
>  			int fd;
> +			int isreg = S_ISREG(mode);

You probably want to move this isreg business up one scope
(i.e. inside "if (update_wd) {").  Then the if () condition
for this block can use it already.

>  			if (mode & 0100)
>  				mode = 0777;
>  			else
> @@ -788,8 +781,37 @@ static void update_file_flags(struct merge_options *o,
>  			fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
>  			if (fd < 0)
>  				die_errno(_("failed to open '%s'"), path);
> -			write_in_full(fd, buf, size);
> -			close(fd);
> +
> +			int smudge_to_file = can_smudge_to_file(path);

This does not compile with decl-after-statement.  I suspect other
patches in this series have the same issue but I did not check.  Do
you say "make DEVELOPER=1"?

> +			if (smudge_to_file) {
> +				close(fd);
> +				fd = convert_to_working_tree_filter_to_file(path, path, buf, size);
> +				if (fd < 0) {
> +					/* smudgeToFile filter failed;
> +					 * continue with regular file
> +					 * creation. */

/*
 * Style: We format our multi-line
 * comments like this.
 */

> +					smudge_to_file = 0;

Ahh, I was wondering why this is not "if (smudge_to_file) ... else ...".

> +					fd = open(path, O_WRONLY | O_TRUNC | O_CREAT, mode);
> +					if (fd < 0)
> +						die_errno(_("failed to open '%s'"), path);
> +				}
> +				else {
> +					close(fd);
> +				}
> +			}
> +
> +			if (! smudge_to_file) {

Style: if (!smudge_to_file) {

> +test_expect_success 'smudgeToFile filter is used in merge' '
> +	test_config filter.rot13.smudgeToFile ./rot13-to-file.sh &&
> +
> +	git commit -m "added fstest.t" fstest.t &&
> +	git checkout -b old &&
> +	git reset --hard HEAD^ &&
> +	git merge master &&
> +
> +	test -e rot13-to-file.ran &&
> +	rm -f rot13-to-file.ran &&
> +
> +	cmp test fstest.t &&

"test_cmp test fstest.t"?  The difference matters when running the
test with -v option.

> +	git checkout master

What happens if any of the previous steps failed?  Does the next
test get confused because you would fail to go back to the master
branch?

> +'
> +
>  test_expect_success 'smudgeToFile filter is used by git am' '
>  	test_config filter.rot13.smudgeToFile ./rot13-to-file.sh &&
>  
> -	git commit fstest.t -m "added fstest.t" &&
>  	git format-patch HEAD^ --stdout > fstest.patch &&

Style: 

	git format-patch HEAD^ --stdout >fstest.patch &&

>  	git reset --hard HEAD^ &&
>  	git am < fstest.patch &&

Style: 

	git am <fstest.patch &&

but in this case you do not even need to redirect, i.e.

	git am fstest.patch &&

is enough.

Thanks.