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

Hi Junio,

Thanks for the review.

Junio C Hamano writes:

quoted

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 <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>

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.

quoted

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

quoted

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

quoted

+	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