Re: [PATCH v4] Add another option for receive.denyCurrentBranch

[Junio C Hamano <gitster@pobox.com>]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> +Another option is "updateInstead" which will update the working
> +directory (must be clean) if pushing into the current branch. This option is
> +intended for synchronizing working directories when one side is not easily
> +accessible via interactive ssh (e.g. a live web site, hence the requirement
> +that the working directory be clean). This mode also comes in handy when
> +developing inside a VM to test and fix code on different Operating Systems.

Thanks; this explains the intent very clearly.

> @@ -730,11 +733,90 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)
>  	return 0;
>  }
>  
> +static const char *update_worktree(unsigned char *sha1)
> +{
> +	const char *update_refresh[] = {
> +		"update-index", "--ignore-submodules", "--refresh", "-q", NULL
> +	};

Please have "-q" as the first parameter.

    $ git reset --hard
    $ echo >Makefile
    $ git update-index -q --refresh ; echo $?
    0
    $ git update-index --refresh -q ; echo $?
    Makefile: needs update
    1

Yes, I understand and agree that this is a "WAT???".

But update-index has processed its options from left to right from
the beginning of time, which we may want to change someday, but this
topic is more important than updating that "WAT???".

> +...
> +	const char *read_tree[] = {
> +		"read-tree", "-u", "-m", sha1_to_hex(sha1), NULL
> +	};

Is everybody's compiler OK with this initialization with computed
value?  Just checking to see if somebody else says "that would not
work for my setting".

> +	/* run_command() does not clean up completely; reinitialize */
> +	child_process_init(&child);

I do not think you need that comment.  The name run_command() does
not imply "run and clean up for reuse" at all, at least to me.

> +	child.argv = diff_files;
> +	...
> +	if (run_command(&child)) {
> +		argv_array_clear(&env);
> +		return "Working directory not clean";
> +	}
> +
> +	/* run_command() does not clean up completely; reinitialize */
> +	child_process_init(&child);
> +	child.argv = diff_index;
> +	...
> +	if (run_command(&child)) {
> +		argv_array_clear(&env);
> +		return "Working directory not clean";
> +	}

Do we want to give the same message for these two cases?

> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index f4da20a..b8df39c 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1330,4 +1330,25 @@ test_expect_success 'fetch into bare respects core.logallrefupdates' '
>  	)
>  '
>  
> +test_expect_success 'receive.denyCurrentBranch = updateInstead' '
> +	git push testrepo master &&
> +	(cd testrepo &&
> +		git reset --hard &&
> +		git config receive.denyCurrentBranch updateInstead
> +	) &&
> +	test_commit third path2 &&
> +	git push testrepo master &&
> +	test $(git rev-parse HEAD) = $(cd testrepo && git rev-parse HEAD) &&
> +	test third = "$(cat testrepo/path2)" &&
> +	(cd testrepo &&
> +		git update-index --refresh &&
> +		git diff-files --quiet &&
> +		git diff-index --cached HEAD -- &&

This "diff-index", without "--quiet" would not signal if the index
matches HEAD with its exit status.

> +		echo changed > path2 &&

s/> />/;

> +		git add path2
> +	) &&

This made sure that the update happens in a clean state.

> +	test_commit fourth path2 &&
> +	test_must_fail git push testrepo master

And this made sure that the push fails, but does not check what
happened on the receiving repository; minimally something like this
perhaps?

	test_must_fail git push testrepo master &&
        test $(git rev-parse HEAD^) = $(git -C testrepo rev-parse HEAD) &&
	(
		cd testrepo &&
		git diff --quiet &&
		test changed = "$(cat path2)"
	)

That is, we expect that "third" is still the HEAD in testrepo, there
is no difference between the working tree and the index (because you
did "git add path2" over there previously), and path2 still has the
locally updated string in the working tree.

> +'
> +
>  test_done

Other than that, this looks pretty well done.

Thanks.

P.S. I'll be doing 2.2 final today, so I won't have time to apply
this with local tweaking to address the issues above.