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

Ramkumar Ramachandra [off-list ref] 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 <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>

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

What is that trailing double-dash about?

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?

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

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

Ramkumar Ramachandra wrote:

Junio C Hamano writes:

quoted

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.

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

Ramkumar Ramachandra wrote:

Are all these tags useful?

Probably not. :)

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.

Maybe liked or approved instead of acknowledged.

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.

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.

Hi Jonathan,

Jonathan Nieder writes:

Ramkumar Ramachandra wrote:

quoted

Are all these tags useful?

Probably not. :)

[...]

How about this then?

Thanks-to: Jonathan Nieder [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>

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

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.

Heya,

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

On Fri, Oct 1, 2010 at 20:28, Jonathan Nieder [off-list ref] wrote:

Ramkumar Ramachandra wrote:

quoted

+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

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

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

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

On Sat, Oct 2, 2010 at 10:02 AM, Ramkumar Ramachandra
[off-list ref] 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 <redacted>
Signed-off-by: Ramkumar Ramachandra <redacted>

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

-- Ram

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 [off-list ref]
Liked-by: Sverre Rabbelier [off-list ref]
Signed-off-by: Ramkumar Ramachandra <redacted>
---
 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