Re: [PATCH v2 1/2] sh-setup: Write a new require_clean_work_tree function

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2010, 20:38 UTC
Message-ID: <7v4od7hsqt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1285877017-8060-2-git-send-email-artagnon@gmail.com>

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Show 6 quoted lines

> Write a new require_clean_work_tree function to error out when
> unstaged changes are present in the working tree and (optionally)
> uncommitted changes in the index.
>
> Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

Please don't do this in-body "Cc:"; it is meaningless.

Show 19 quoted lines

> ---
>  git-sh-setup.sh |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index 6131670..215ec33 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -145,6 +145,34 @@ require_work_tree () {
>  	die "fatal: $0 cannot be used without a working tree."
>  }
>  
> +require_clean_work_tree () {
> +	# Update the index
> +	git update-index -q --ignore-submodules --refresh
> +	err=0
> +
> +	# Disallow unstaged changes in the working tree
> +	if ! git diff-files --quiet --ignore-submodules --

What is that trailing double-dash about?

Show 8 quoted lines

> +	then
> +		echo >&2 "cannot $1: you have unstaged changes."
> +		git diff-files --name-status -r --ignore-submodules -- >&2
> +		err=1
> +	fi
> +
> +	# Disallow uncommitted changes in the index
> +	if ! git diff-index --cached --quiet HEAD --ignore-submodules --

Do not write HEAD there that sets a wrong example; the command line arguments are flag-options, revs, double-dash and pathspec.

Contrary to what your proposed log message says, I do not see anything "optional" in the way how this check is done here... What is going on?

Unfortunately we cannot judge if unconditional check is the right thing to do without looking at the callers; why did you make this into two-patch series?

Mental note before reviewing the second patch: do all callers want the same "both working tree and index are spiffy clean" check?

Show 12 quoted lines

> +	then
> +		echo >&2 "cannot $1: your index contains uncommitted changes."
> +		git diff-index --cached --name-status -r --ignore-submodules HEAD -- >&2
> +		err=1
> +	fi
> +
> +	if [ $err = 1 ]
> +	then
> +	    echo >&2 "Please commit or stash them."
> +	    exit 1
> +	fi
> +}

Mental note before reviewing the second patch: warning/error messages from this codepath are all written without warning: or error: prefixes.