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

From: Ramkumar Ramachandra <artagnon@gmail.com>
Date: Oct 1, 2010, 04:57 UTC
Message-ID: <20101001045713.GE20098@kytes>
In-Reply-To: <7v4od7hsqt.fsf@alter.siamese.dyndns.org>

Hi Junio,

Thanks for the review.

Junio C Hamano writes:

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

Oh. What I intended to say was that Matthieu reviewed my previous iteration- should I just put that information in the cover letter or is there some other notation I should use? I can't use "Reviewed-by" either because he only reviewed the previous iteration- not this one.

Show 21 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?

Hm, I think I got confused between the options that git-diff-index and git-diff-files take. I'll get rid of this in the next iteration.

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

Ok. I suppose `git diff-index --cached --quiet --ignore-submodules HEAD --` is better. Should I keep the double-dash or is it unnecessary?

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

Oops, sorry about that -- it's a slightly dated log message: While writing the patch, I thought I'd be clever and pass a `$2` to make this optional, but decided against it later.

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

Oh, ok. I'll make it a single patch in the next iteration.

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

Not necessarily, but I figured that many of them want it.

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

As you've pointed out in the second patch, it's probably not a good idea to print out the advice here.

-- Ram