threads / patch / 25302

v2, 2 partsEliminate cryptic "needs update" error message

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

## tl;dr

19 messages between Sep 30, 2010 and Oct 3, 2010. Diffs are folded; open one to read it.

replies: 18people: 4as markdown or json

Ramkumar Ramachandra· Sep 30, 2010, 20:03 UTC · lore

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
Ramkumar Ramachandra· Sep 30, 2010, 20:03 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to git-sh-setup.sh +28 −0
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
Junio C Hamano· Sep 30, 2010, 20:38 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 6 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.
Show 19 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?
Show 8 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.

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?

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

Ramkumar Ramachandra· Oct 1, 2010, 04:57 UTC · re: Junio C Hamano · lore

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:
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
Jonathan Nieder· Oct 1, 2010, 05:37 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
> Junio C Hamano writes:
Show 6 quoted lines
>> 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.

Ramkumar Ramachandra· Oct 1, 2010, 07:21 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
Jonathan Nieder writes:
Show 33 quoted lines
> 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< --
Show changes to Documentation/SubmittingPatches +19 −6
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
Jonathan Nieder· Oct 1, 2010, 07:40 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra wrote:
> Are all these tags useful?
Probably not. :)
Show 13 quoted lines
> --- 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.

Show 8 quoted lines
> +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.
Ramkumar Ramachandra· Oct 1, 2010, 12:56 UTC · re: Jonathan Nieder · lore

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

Hi Jonathan,
Jonathan Nieder writes:
Show 5 quoted lines
> 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< --
Show changes to Documentation/SubmittingPatches +15 −6
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
Jonathan Nieder· Oct 1, 2010, 18:28 UTC · re: Ramkumar Ramachandra · lore

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

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.
Sverre Rabbelier· Oct 1, 2010, 20:22 UTC · re: Jonathan Nieder · lore

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

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:
Show 5 quoted lines
> 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
Ramkumar Ramachandra· Oct 2, 2010, 04:32 UTC · re: Sverre Rabbelier · lore

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
Ramkumar Ramachandra· Oct 2, 2010, 04:32 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to git-sh-setup.sh +28 −0
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
Ramkumar Ramachandra· Oct 2, 2010, 04:37 UTC · re: Ramkumar Ramachandra · lore

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

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

On Sat, Oct 2, 2010 at 10:02 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

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

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

-- Ram
Ramkumar Ramachandra· Oct 2, 2010, 04:37 UTC · re: Sverre Rabbelier · lore

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

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(-)
Show changes to Documentation/SubmittingPatches +15 −6
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
Junio C Hamano· Oct 1, 2010, 15:07 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 8 quoted lines
> 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.
Ramkumar Ramachandra· Sep 30, 2010, 20:03 UTC · re: Ramkumar Ramachandra · lore

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

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(-)
Show changes to 3 files +7 −33

git-pull.sh, git-rebase--interactive.sh, git-rebase.sh

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
Junio C Hamano· Sep 30, 2010, 21:08 UTC · re: Ramkumar Ramachandra · lore

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

Ramkumar Ramachandra <artagnon@gmail.com> writes:
Show 13 quoted lines
> 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.

Show 16 quoted lines
> 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?

Show 12 quoted lines
> @@ -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.

Show 9 quoted lines
>  			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.

Show 22 quoted lines
> 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.

Ramkumar Ramachandra· Oct 1, 2010, 05:14 UTC · re: Junio C Hamano · lore

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

Hi again,
Junio C Hamano writes:
Show 19 quoted lines
> 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.
Show 21 quoted lines
> > 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.
Show 27 quoted lines
> > @@ -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'.
Show 21 quoted lines
> > @@ -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.

Show 25 quoted lines
> > 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
Junio C Hamano· Oct 3, 2010, 23:34 UTC · re: Ramkumar Ramachandra · lore

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

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.

← back to recent threads