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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 26, 2014, 21:02 UTC
Message-ID: <xmqqppc9ptf3.fsf@gitster.dls.corp.google.com>
In-Reply-To: <2b44016c4a233348ef0c7600ca4aaf8aa50948e8.1417033080.git.johannes.schindelin@gmx.de>

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

Show 6 quoted lines

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

Show 9 quoted lines

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

Show 15 quoted lines

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

Show 22 quoted lines

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