Hi Junio,
Thanks for the review.
Junio C Hamano writes:
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ramkumar Ramachandra <artagnon@xxxxxxxxx>
>
> 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.
> > ---
> > 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.
> > + 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.
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html