[PATCH v2 0/2] Eliminate cryptic "needs update" error message

[PATCH v2 0/2] Eliminate cryptic "needs update" error message

[Ramkumar Ramachandra]

Thanks to Matthieu for reviewing round 1: all the changes he suggested
are incorporated in this round.

Ramkumar Ramachandra (2):
  sh-setup: Write a new require_clean_work_tree function
  Porcelain scripts: Rewrite cryptic "needs update" error message

 git-pull.sh                |    5 +----
 git-rebase--interactive.sh |   21 +++++----------------
 git-rebase.sh              |   14 +-------------
 git-sh-setup.sh            |   28 ++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 33 deletions(-)

-- 
1.7.2.2.409.gdbb11.dirty

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

[Ramkumar Ramachandra]

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>
---
 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 --
+	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 --
+	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
+}
+
 get_author_ident_from_commit () {
 	pick_author_script='
 	/^author /{
-- 
1.7.2.2.409.gdbb11.dirty

[PATCH v2 2/2] Porcelain scripts: Rewrite cryptic "needs update" error message

[Ramkumar Ramachandra]

Although Git interally has the facility to differentiate between
porcelain and plubmbing commands and appropriately print errors,
several shell scripts invoke plubming commands triggering cryptic
plumbing errors to be displayed on a porcelain interface. This patch
replaces the "needs update" message in git-pull and git-rebase, when
`git update-index` is run, with a more friendly message.

Reported-by: Joshua Jensen <jjensen@workspacewhiz.com>
Cc: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 git-pull.sh                |    5 +----
 git-rebase--interactive.sh |   21 +++++----------------
 git-rebase.sh              |   14 +-------------
 3 files changed, 7 insertions(+), 33 deletions(-)

diff --git a/git-pull.sh b/git-pull.sh
index 8eb74d4..a15b545 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -201,10 +201,7 @@ test true = "$rebase" && {
 			die "updating an unborn branch with changes added to the index"
 		fi
 	else
-		git update-index --ignore-submodules --refresh &&
-		git diff-files --ignore-submodules --quiet &&
-		git diff-index --ignore-submodules --cached --quiet HEAD -- ||
-		die "refusing to pull with rebase: your working tree is not up-to-date"
+		require_clean_work_tree "pull with rebase"
 	fi
 	oldremoteref= &&
 	. git-parse-remote &&
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index a27952d..9546975 100755
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -153,14 +153,6 @@ run_pre_rebase_hook () {
 	fi
 }
 
-require_clean_work_tree () {
-	# test if working tree is dirty
-	git rev-parse --verify HEAD > /dev/null &&
-	git update-index --ignore-submodules --refresh &&
-	git diff-files --quiet --ignore-submodules &&
-	git diff-index --cached --quiet HEAD --ignore-submodules -- ||
-	die "Working tree is dirty"
-}
 
 ORIG_REFLOG_ACTION="$GIT_REFLOG_ACTION"
 
@@ -557,12 +549,9 @@ do_next () {
 			exit "$status"
 		fi
 		# Run in subshell because require_clean_work_tree can die.
-		if ! (require_clean_work_tree)
+		if ! (require_clean_work_tree "rebase")
 		then
-			warn "Commit or stash your changes, and then run"
-			warn
-			warn "	git rebase --continue"
-			warn
+			warn "Then run git rebase --continue."
 			exit 1
 		fi
 		;;
@@ -740,7 +729,7 @@ do
 			die "Cannot read HEAD"
 		git update-index --ignore-submodules --refresh &&
 			git diff-files --quiet --ignore-submodules ||
-			die "Working tree is dirty"
+			die "Working tree is dirty. Please commit or stash your changes to proceed."
 
 		# do we have anything to commit?
 		if git diff-index --cached --quiet --ignore-submodules HEAD --
@@ -768,7 +757,7 @@ first and then run 'git rebase --continue' again."
 
 		record_in_rewritten "$(cat "$DOTEST"/stopped-sha)"
 
-		require_clean_work_tree
+		require_clean_work_tree "rebase"
 		do_rest
 		;;
 	--abort)
@@ -866,7 +855,7 @@ first and then run 'git rebase --continue' again."
 
 		comment_for_reflog start
 
-		require_clean_work_tree
+		require_clean_work_tree "rebase"
 
 		if test ! -z "$1"
 		then
diff --git a/git-rebase.sh b/git-rebase.sh
index 3335cee..c3ca8d5 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -416,19 +416,7 @@ else
 	fi
 fi
 
-# The tree must be really really clean.
-if ! git update-index --ignore-submodules --refresh > /dev/null; then
-	echo >&2 "cannot rebase: you have unstaged changes"
-	git diff-files --name-status -r --ignore-submodules -- >&2
-	exit 1
-fi
-diff=$(git diff-index --cached --name-status -r --ignore-submodules HEAD --)
-case "$diff" in
-?*)	echo >&2 "cannot rebase: your index contains uncommitted changes"
-	echo >&2 "$diff"
-	exit 1
-	;;
-esac
+require_clean_work_tree "rebase"
 
 if test -z "$rebase_root"
 then
-- 
1.7.2.2.409.gdbb11.dirty

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

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> 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@grenoble-inp.fr>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

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

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

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

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

Re: [PATCH v2 2/2] Porcelain scripts: Rewrite cryptic "needs update" error message

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> diff --git a/git-pull.sh b/git-pull.sh
> index 8eb74d4..a15b545 100755
> --- a/git-pull.sh
> +++ b/git-pull.sh
> @@ -201,10 +201,7 @@ test true = "$rebase" && {
>  			die "updating an unborn branch with changes added to the index"
>  		fi
>  	else
> -		git update-index --ignore-submodules --refresh &&
> -		git diff-files --ignore-submodules --quiet &&
> -		git diff-index --ignore-submodules --cached --quiet HEAD -- ||
> -		die "refusing to pull with rebase: your working tree is not up-to-date"
> +		require_clean_work_tree "pull with rebase"

Ok, this wants both the working tree and the index clean, and it will exit
with non-zero.  The original message mentioned "working tree" but by
calling require_clean_work_tree the user won't see that word anymore.

> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index a27952d..9546975 100755
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -153,14 +153,6 @@ run_pre_rebase_hook () {
>  	fi
>  }
>  
> -require_clean_work_tree () {
> -	# test if working tree is dirty
> -	git rev-parse --verify HEAD > /dev/null &&
> -	git update-index --ignore-submodules --refresh &&
> -	git diff-files --quiet --ignore-submodules &&
> -	git diff-index --cached --quiet HEAD --ignore-submodules -- ||
> -	die "Working tree is dirty"
> -}

We used to test and exit early if HEAD cannot be read (e.g. empty
history), but now require_clean_work_tree() will probably spit a cryptic
error from diff-index it makes.  Don't you need to have an equivalent
check for HEAD somewhere before you make the first call to r-c-w-t?

> @@ -557,12 +549,9 @@ do_next () {
>  			exit "$status"
>  		fi
>  		# Run in subshell because require_clean_work_tree can die.
> -		if ! (require_clean_work_tree)
> +		if ! (require_clean_work_tree "rebase")
>  		then
> -			warn "Commit or stash your changes, and then run"
> -			warn
> -			warn "	git rebase --continue"
> -			warn
> +			warn "Then run git rebase --continue."

We will see something like:

	cannot rebase: you have unstaged changes.
	M Makefile
	cannot rebase: you index contains uncommitted changes.
	M Makefile
        M hello.c
        M goodbye.c
        ... 147 other paths that make the above scroll away ...
        Please commit or stash them.
        Then run git rebase --continue.

Is this what we really want?  Also the messages seem somewhat
case-challenged.



>  			exit 1
>  		fi
>  		;;
> @@ -740,7 +729,7 @@ do
>  			die "Cannot read HEAD"
>  		git update-index --ignore-submodules --refresh &&
>  			git diff-files --quiet --ignore-submodules ||
> -			die "Working tree is dirty"
> +			die "Working tree is dirty. Please commit or stash your changes to proceed."

I wonder if it is a good advice to choose between committing and stashing.

This codepath is for --continue, and by definition when rebase started
there wasn't any local modification (otherwise it wouldn't have started
and we wouldn't have come this far), so the change must have come from the
end user who wanted to amend (or resolve conflicts), thought that s/he
included all the changes s/he did in the working tree in the amended
commit and told us to continue.  We however found some leftover local
modifications.

"Please commit or stash" is probably the worst advice we can give in this
particular situation.  It is likely to be something s/he forgot to add;
the existing advice that says "Please commit them first and then ..." you
can see a few lines down this part is probably better.

In other codepaths, aborting rebase and cleaning the work area first
before attempting a rebase might be a better choice than either committing
or stashing.  Often, I find that is the least confusing choise, at least.

> diff --git a/git-rebase.sh b/git-rebase.sh
> index 3335cee..c3ca8d5 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -416,19 +416,7 @@ else
>  	fi
>  fi
>  
> -# The tree must be really really clean.
> -if ! git update-index --ignore-submodules --refresh > /dev/null; then
> -	echo >&2 "cannot rebase: you have unstaged changes"
> -	git diff-files --name-status -r --ignore-submodules -- >&2
> -	exit 1
> -fi
> -diff=$(git diff-index --cached --name-status -r --ignore-submodules HEAD --)
> -case "$diff" in
> -?*)	echo >&2 "cannot rebase: your index contains uncommitted changes"
> -	echo >&2 "$diff"
> -	exit 1
> -	;;
> -esac
> +require_clean_work_tree "rebase"

This side is uncontroversial, except that it has the same "is commit/stash
the best advice?" issue.

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

[Ramkumar Ramachandra]

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

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

Re: [PATCH v2 2/2] Porcelain scripts: Rewrite cryptic "needs update" error message

[Ramkumar Ramachandra]

Hi again,

Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> 
> > diff --git a/git-pull.sh b/git-pull.sh
> > index 8eb74d4..a15b545 100755
> > --- a/git-pull.sh
> > +++ b/git-pull.sh
> > @@ -201,10 +201,7 @@ test true = "$rebase" && {
> >  			die "updating an unborn branch with changes added to the index"
> >  		fi
> >  	else
> > -		git update-index --ignore-submodules --refresh &&
> > -		git diff-files --ignore-submodules --quiet &&
> > -		git diff-index --ignore-submodules --cached --quiet HEAD -- ||
> > -		die "refusing to pull with rebase: your working tree is not up-to-date"
> > +		require_clean_work_tree "pull with rebase"
> 
> Ok, this wants both the working tree and the index clean, and it will exit
> with non-zero.  The original message mentioned "working tree" but by
> calling require_clean_work_tree the user won't see that word anymore.

Right. I suppose this hunk is alright.

> > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> > index a27952d..9546975 100755
> > --- a/git-rebase--interactive.sh
> > +++ b/git-rebase--interactive.sh
> > @@ -153,14 +153,6 @@ run_pre_rebase_hook () {
> >  	fi
> >  }
> >  
> > -require_clean_work_tree () {
> > -	# test if working tree is dirty
> > -	git rev-parse --verify HEAD > /dev/null &&
> > -	git update-index --ignore-submodules --refresh &&
> > -	git diff-files --quiet --ignore-submodules &&
> > -	git diff-index --cached --quiet HEAD --ignore-submodules -- ||
> > -	die "Working tree is dirty"
> > -}
> 
> We used to test and exit early if HEAD cannot be read (e.g. empty
> history), but now require_clean_work_tree() will probably spit a cryptic
> error from diff-index it makes.  Don't you need to have an equivalent
> check for HEAD somewhere before you make the first call to r-c-w-t?

Ok. I'll add that in the next iteration.

> > @@ -557,12 +549,9 @@ do_next () {
> >  			exit "$status"
> >  		fi
> >  		# Run in subshell because require_clean_work_tree can die.
> > -		if ! (require_clean_work_tree)
> > +		if ! (require_clean_work_tree "rebase")
> >  		then
> > -			warn "Commit or stash your changes, and then run"
> > -			warn
> > -			warn "	git rebase --continue"
> > -			warn
> > +			warn "Then run git rebase --continue."
> 
> We will see something like:
> 
> 	cannot rebase: you have unstaged changes.
> 	M Makefile
> 	cannot rebase: you index contains uncommitted changes.
> 	M Makefile
>         M hello.c
>         M goodbye.c
>         ... 147 other paths that make the above scroll away ...
>         Please commit or stash them.
>         Then run git rebase --continue.
> 
> Is this what we really want?  Also the messages seem somewhat
> case-challenged.

This is one of the concerns I had while creating this iteration. What
do you suggest? Should we limit the number of paths listed like this?
(I've limited it to 3 paths in this example). Also, is the case
alright now?

 	Cannot rebase: you have unstaged changes.
 	 M Makefile
	 M ram.c
	 M junio.c
	 (and 372 more)
 	Cannot rebase: your index contains uncommitted changes.
 	 M Makefile
         M hello.c
         M goodbye.c
         (and 147 more)
         Please commit or stash them.
	 Then run 'git rebase --continue'.

> > @@ -740,7 +729,7 @@ do
> >  			die "Cannot read HEAD"
> >  		git update-index --ignore-submodules --refresh &&
> >  			git diff-files --quiet --ignore-submodules ||
> > -			die "Working tree is dirty"
> > +			die "Working tree is dirty. Please commit or stash your changes to proceed."
> 
> I wonder if it is a good advice to choose between committing and stashing.
> 
> This codepath is for --continue, and by definition when rebase started
> there wasn't any local modification (otherwise it wouldn't have started
> and we wouldn't have come this far), so the change must have come from the
> end user who wanted to amend (or resolve conflicts), thought that s/he
> included all the changes s/he did in the working tree in the amended
> commit and told us to continue.  We however found some leftover local
> modifications.
> 
> "Please commit or stash" is probably the worst advice we can give in this
> particular situation.  It is likely to be something s/he forgot to add;
> the existing advice that says "Please commit them first and then ..." you
> can see a few lines down this part is probably better.

Right. I didn't think about this enough.
- If it's an "edit", "squash" or "fixup" interactive rebase step, the
  staged changes are automatically committed to the current commit:
  you don't have to explicity commit --amend.
- If the user doesn't want to squash all the changes into the current
  commit, he should stage the changes he wants to be included in the
  commit and stash the rest for use in another step.
- If it's a "reword" step, staged changes/ dirty working tree don't
  mean anything. The use should simply stash everything and continue
  in this case.

I suppose I can come up with a way to check which path was taken and
print more targeted advice. Will do in the next iteration.

> In other codepaths, aborting rebase and cleaning the work area first
> before attempting a rebase might be a better choice than either committing
> or stashing.  Often, I find that is the least confusing choise, at least.

I think we should remove the advice from the r-c-w-t and add more
targeted advice in the caller.

> > diff --git a/git-rebase.sh b/git-rebase.sh
> > index 3335cee..c3ca8d5 100755
> > --- a/git-rebase.sh
> > +++ b/git-rebase.sh
> > @@ -416,19 +416,7 @@ else
> >  	fi
> >  fi
> >  
> > -# The tree must be really really clean.
> > -if ! git update-index --ignore-submodules --refresh > /dev/null; then
> > -	echo >&2 "cannot rebase: you have unstaged changes"
> > -	git diff-files --name-status -r --ignore-submodules -- >&2
> > -	exit 1
> > -fi
> > -diff=$(git diff-index --cached --name-status -r --ignore-submodules HEAD --)
> > -case "$diff" in
> > -?*)	echo >&2 "cannot rebase: your index contains uncommitted changes"
> > -	echo >&2 "$diff"
> > -	exit 1
> > -	;;
> > -esac
> > +require_clean_work_tree "rebase"
> 
> This side is uncontroversial, except that it has the same "is commit/stash
> the best advice?" issue.

Ok, I'll come up with something nicer in the next iteration.

-- Ram

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:
> Junio C Hamano writes:

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

Some noise about Cc and Reviewed-by tags:

 - I have been using Cc lines in patches to say "I consider this
   person something of a maintainer of the subsystem in question
   and would be particularly interested in his or her opinion."
   The idea is that if the patch does not get acked and a Cc line
   remains, people can tell that from the log.  The benefits:

    1) I remember to cc them
    2) later it is easy to find who looks like a maintainer
    3) the lack of ack is more obvious

   Checking Linux's Documentation/SubmittingPatches, I find that
   that is a misuse on my part (sorry).  A person passing on a patch
   to Linus is rather supposed to _add_ a Cc line in the rare event
   that they want to explain that a certain person had an opportunity
   to comment but did not comment (so Linus can know about their
   indifference to the patch, I guess).

   Neither use is as important for git, where many people read the
   list so it is not as important to cc people to get proper review.

 - I also used to abuse Cc lines to fit in contact information for a
   person who helped me, until I learned to use Helped-by and similar
   neologisms for that.  Sorry.

 - Again from Documentation/SubmittingPatches, I learned a while ago
   that Reviewed-by, unlike Acked-by, can only be offered by the
   reviewer and means that she is satisfied that the patch is ready
   for application.

If you just want to credit Matthieu, I suppose it would make sense to
say "Thanks to Matthieu Moy for such-and-such" somewhere.

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

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Some noise about Cc and Reviewed-by tags:
> 
>  - I have been using Cc lines in patches to say "I consider this
>    person something of a maintainer of the subsystem in question
>    and would be particularly interested in his or her opinion."
>    The idea is that if the patch does not get acked and a Cc line
>    remains, people can tell that from the log.  The benefits:
> 
>     1) I remember to cc them
>     2) later it is easy to find who looks like a maintainer
>     3) the lack of ack is more obvious
> 
>    Checking Linux's Documentation/SubmittingPatches, I find that
>    that is a misuse on my part (sorry).  A person passing on a patch
>    to Linus is rather supposed to _add_ a Cc line in the rare event
>    that they want to explain that a certain person had an opportunity
>    to comment but did not comment (so Linus can know about their
>    indifference to the patch, I guess).
> 
>    Neither use is as important for git, where many people read the
>    list so it is not as important to cc people to get proper review.
> 
>  - I also used to abuse Cc lines to fit in contact information for a
>    person who helped me, until I learned to use Helped-by and similar
>    neologisms for that.  Sorry.
> 
>  - Again from Documentation/SubmittingPatches, I learned a while ago
>    that Reviewed-by, unlike Acked-by, can only be offered by the
>    reviewer and means that she is satisfied that the patch is ready
>    for application.
> 
> If you just want to credit Matthieu, I suppose it would make sense to
> say "Thanks to Matthieu Moy for such-and-such" somewhere.

Thanks for the lengthy explanation. Perhaps we can document this in
Git's SubmittingPatches?

Are all these tags useful? Should I include more such as "Mentored-by"
or explicity mention that the contributor is free to come up with
other freeform tags as she deems appropriate?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
-- 8< --
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index ece3c77..84c9eaa 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -264,12 +264,25 @@ the change to its true author (see (2) above).
 Also notice that a real name is used in the Signed-off-by: line. Please
 don't hide your real name.
 
-Some people also put extra tags at the end.
-
-"Acked-by:" says that the patch was reviewed by the person who
-is more familiar with the issues and the area the patch attempts
-to modify.  "Tested-by:" says the patch was tested by the person
-and found to have the desired effect.
+Some extra tags you can use in the end along with their meanings are:
+
+1. "Reported-by:" is used to to credit someone who found the bug that
+   the patch attempts to fix.
+2. "Acked-by:" says that the patch was acknowledged by the person who
+   is more familiar with the issues and the area the patch attempts to
+   modify.
+3. "Reviewed-by:", unlike the other tags, can only be offered by the
+   reviewer and means that she is completely satisfied that the patch
+   is ready for application.  It is usually offered only after a
+   detailed review.
+4. "Tested-by:" is used to indicate that the person applied the patch
+   and found it to have the desired effect.
+5. "Thanks-to:" is a more broad term used to credit someone who helped
+   with the patch in some way. The person perhaps gave an idea or
+   reviewed some part of the patch without awarding a "Reviewed-by".
+6. "Based-on-patch-by:" is used to credit the person whose patch yours
+   is based on. The original patch was probably not considered for
+   inclusion due to several reasons.
 
 ------------------------------------------------
 An ideal patch flow

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> Are all these tags useful?

Probably not. :)

> --- a/Documentation/SubmittingPatches
> +++ b/Documentation/SubmittingPatches
> @@ -264,12 +264,25 @@ the change to its true author (see (2) above).
>  Also notice that a real name is used in the Signed-off-by: line. Please
>  don't hide your real name.
>  
> -Some people also put extra tags at the end.
> -
> -"Acked-by:" says that the patch was reviewed by the person who
> -is more familiar with the issues and the area the patch attempts
> -to modify.  "Tested-by:" says the patch was tested by the person
> -and found to have the desired effect.
> +Some extra tags you can use in the end along with their meanings are:

I like the old "Some people" phrasing; maybe we can get the same effect
(i.e., making it clear that you don't really have to use these) by saying

 If you'd like, you can put extra tags at end:

> +1. "Reported-by:" is used to to credit someone who found the bug that
> +   the patch attempts to fix.

Sensible.

> +2. "Acked-by:" says that the patch was acknowledged by the person who
> +   is more familiar with the issues and the area the patch attempts to
> +   modify.

Maybe liked or approved instead of acknowledged.

> +3. "Reviewed-by:", unlike the other tags, can only be offered by the
> +   reviewer and means that she is completely satisfied that the patch
> +   is ready for application.  It is usually offered only after a
> +   detailed review.

Yeah.  Linux's Documentation/SubmittingPatches includes a nice
"reviewer's statement of oversight" by Jonathan Corbet, explaining
what exactly a reviewed-by is and is not supposed to signify.

> +4. "Tested-by:" is used to indicate that the person applied the patch
> +   and found it to have the desired effect.
> +5. "Thanks-to:" is a more broad term used to credit someone who helped
> +   with the patch in some way. The person perhaps gave an idea or
> +   reviewed some part of the patch without awarding a "Reviewed-by".
> +6. "Based-on-patch-by:" is used to credit the person whose patch yours
> +   is based on. The original patch was probably not considered for
> +   inclusion due to several reasons.

These seem intuitive without explanation.  I suppose Tested-by is
common enough and worth encouraging, though.  In the end, a person can
put what they want.  (e.g. the mysterious Whatevered-by:
http://lwn.net/Articles/399052/.)

Anyway, thanks for clearing this up.

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

[Ramkumar Ramachandra]

Hi Jonathan,

Jonathan Nieder writes:
> Ramkumar Ramachandra wrote:
> 
> > Are all these tags useful?
> 
> Probably not. :)
[...]

How about this then?

Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>

-- 8< --
diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index ece3c77..72741eb 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -264,12 +264,21 @@ the change to its true author (see (2) above).
 Also notice that a real name is used in the Signed-off-by: line. Please
 don't hide your real name.
 
-Some people also put extra tags at the end.
-
-"Acked-by:" says that the patch was reviewed by the person who
-is more familiar with the issues and the area the patch attempts
-to modify.  "Tested-by:" says the patch was tested by the person
-and found to have the desired effect.
+If you like, you can put extra tags at the end:
+
+1. "Reported-by:" is used to to credit someone who found the bug that
+   the patch attempts to fix.
+2. "Acked-by:" says that the person who is more familiar with the area
+   the patch attempts to modify liked the patch.
+3. "Reviewed-by:", unlike the other tags, can only be offered by the
+   reviewer and means that she is completely satisfied that the patch
+   is ready for application.  It is usually offered only after a
+   detailed review.
+4. "Tested-by:" is used to indicate that the person applied the patch
+   and found it to have the desired effect.
+
+You can also create your own tag or use one that's in common usage
+such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
 
 ------------------------------------------------
 An ideal patch flow

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

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Some noise about Cc and Reviewed-by tags:
>
>    Checking Linux's Documentation/SubmittingPatches, I find that
>    that is a misuse on my part (sorry).  A person passing on a patch
>    to Linus is rather supposed to _add_ a Cc line in the rare event
>    that they want to explain that a certain person had an opportunity
>    to comment but did not comment (so Linus can know about their
>    indifference to the patch, I guess).

Ah, that makes sense.  Thanks for an explanation.

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

[Jonathan Nieder]

Ramkumar Ramachandra wrote:

> How about this then?
[...]
> +You can also create your own tag or use one that's in common usage
> +such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".

I like it.

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

[Sverre Rabbelier]

Heya,

Wow, I totally did not notice this very interesting sub-thread!

On Fri, Oct 1, 2010 at 20:28, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ramkumar Ramachandra wrote:
>> +You can also create your own tag or use one that's in common usage
>> +such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
>
> I like it.

Thanks, Jonathan, for the looking up this stuff from the kernel's
SubmittinPatches.

FWIW I like Ram's patch to SubmittingPatches too.

-- 
Cheers,

Sverre Rabbelier

[PATCH v2 0/2] Eliminate cryptic "needs update" error message

[Ramkumar Ramachandra]

Thanks to Matthieu for reviewing round 1: all the changes he suggested
are incorporated in this round.

Ramkumar Ramachandra (2):
  sh-setup: Write a new require_clean_work_tree function
  Porcelain scripts: Rewrite cryptic "needs update" error message

 git-pull.sh                |    5 +----
 git-rebase--interactive.sh |   21 +++++----------------
 git-rebase.sh              |   14 +-------------
 git-sh-setup.sh            |   28 ++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 33 deletions(-)

-- 
1.7.2.2.409.gdbb11.dirty

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

[Ramkumar Ramachandra]

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>
---
 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 --
+	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 --
+	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
+}
+
 get_author_ident_from_commit () {
 	pick_author_script='
 	/^author /{
-- 
1.7.2.2.409.gdbb11.dirty

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

[Ramkumar Ramachandra]

Oops, please ignore the previous email- sent by mistake.

On Sat, Oct 2, 2010 at 10:02 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> 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>

I'll post a new revision of this patch shortly. Will post the patch to
SubmittingPatches now.

-- Ram

[PATCH] SubmittingPatches: Document some extra tags used in commit messages

[Ramkumar Ramachandra]

Document the meanings of the tags "Reported-by:", "Acked-by:",
"Reviewed-by:" and "Tested-by:" clearly. Also mention that the user is
free to use any custom tags.

Helped-by: Jonathan Nieder <jrnieder@gmail.com>
Liked-by: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/SubmittingPatches |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches
index ece3c77..72741eb 100644
--- a/Documentation/SubmittingPatches
+++ b/Documentation/SubmittingPatches
@@ -264,12 +264,21 @@ the change to its true author (see (2) above).
 Also notice that a real name is used in the Signed-off-by: line. Please
 don't hide your real name.
 
-Some people also put extra tags at the end.
-
-"Acked-by:" says that the patch was reviewed by the person who
-is more familiar with the issues and the area the patch attempts
-to modify.  "Tested-by:" says the patch was tested by the person
-and found to have the desired effect.
+If you like, you can put extra tags at the end:
+
+1. "Reported-by:" is used to to credit someone who found the bug that
+   the patch attempts to fix.
+2. "Acked-by:" says that the person who is more familiar with the area
+   the patch attempts to modify liked the patch.
+3. "Reviewed-by:", unlike the other tags, can only be offered by the
+   reviewer and means that she is completely satisfied that the patch
+   is ready for application.  It is usually offered only after a
+   detailed review.
+4. "Tested-by:" is used to indicate that the person applied the patch
+   and found it to have the desired effect.
+
+You can also create your own tag or use one that's in common usage
+such as "Thanks-to:", "Based-on-patch-by:", or "Mentored-by:".
 
 ------------------------------------------------
 An ideal patch flow
-- 
1.7.2.2.409.gdbb11.dirty

Re: [PATCH v2 2/2] Porcelain scripts: Rewrite cryptic "needs update" error message

[Junio C Hamano]

Ramkumar Ramachandra <artagnon@gmail.com> writes:

> This is one of the concerns I had while creating this iteration. What
> do you suggest? Should we limit the number of paths listed like this?

Personally, I do not think you should list any paths.  If the program
states clearly that it does not like to see a dirty working tree (or a
dirty working tree is likely to be a mistake by a forgetful user), the
user already has enough tools such as "git status" and "git diff" to
figure out which ones are dirty.

> - If it's an "edit", "squash" or "fixup" interactive rebase step, the
>   staged changes are automatically committed to the current commit:
>   you don't have to explicity commit --amend.

It might be somewhat irritating to see "you haven't committed the amend"
against "rebase --continue" after running "git add" or "git add -p" to
shape what should be the final commit, and that is why the codepath runs
"commit --amend" for the user, but I personally think this is an attempt
to help users that is ill-thought-out.  The codepath stops when there are
changes to the working tree files that are not added, to avoid mistakes by
a forgetful user, but what if the change the user forgot to add was an
addition of a new file?

Also, this is one special case that the user has to remember.

It is too late to change this now, but it would have been a lot nicer if
we insisted that the "commit --amend" is always done by the user, and
never automatically by "rebase-i --continue" codepath.

> - If it's a "reword" step, staged changes/ dirty working tree don't
>   mean anything. The use should simply stash everything and continue
>   in this case.

Why would "reword" involve _any_ local changes?

> I suppose I can come up with a way to check which path was taken and
> print more targeted advice. Will do in the next iteration.

In general, I am mildly against giving advices like "commit or stash",
especially when the program does not know the workflow the end user in
each situation is using better than the end user.