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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 22, 2016, 21:39 UTC
Message-ID: <xmqq7fdglx83.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1466629758-8035-9-git-send-email-joeyh@joeyh.name>

Joey Hess <joeyh@joeyh.name> writes:

Show 5 quoted lines

> @@ -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.

Show 11 quoted lines

>  			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"?

Show 7 quoted lines

> +			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 ...".

Show 10 quoted lines

> +					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) {

Show 12 quoted lines

> +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?

Show 7 quoted lines

> +'
> +
>  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.