Joey Hess <[email protected]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to