[PATCH] Showing stash state in bash prompt

[PATCH] Showing stash state in bash prompt

[Daniel Trstenjak]

[-- Attachment #1: Type: text/plain, Size: 828 bytes --]


Greetings
Daniel

-- 
                                                                                                                                           
 Daniel Trstenjak         Tel   : +49 (0)7071-9457-264
 science + computing ag   FAX   : +49 (0)7071-9457-511
 Hagellocher Weg 73       mailto: Daniel.Trstenjak@science-computing.de
 D-72070 Tübingen         WWW   : http://www.science-computing.de/                                                                      
-- 
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Roland Niemeier, 
Dr. Arno Steitz, Dr. Ingrid Zech
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Michel Lepert
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196 


[-- Attachment #2: showing_stash_state.txt --]
[-- Type: text/plain, Size: 836 bytes --]

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1683e6d..86e39a5 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -117,6 +117,7 @@ __git_ps1 ()
 
 		local w
 		local i
+		local s
 		local c
 
 		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
@@ -136,15 +137,19 @@ __git_ps1 ()
 					else
 						i="#"
 					fi
+					stash_list=`git stash list`
+					if [ -n "$stash_list" ]; then
+					        s="$"
+				        fi
 				fi
 			fi
 		fi
 
 		if [ -n "$b" ]; then
 			if [ -n "${1-}" ]; then
-				printf "$1" "$c${b##refs/heads/}$w$i$r"
+				printf "$1" "$c${b##refs/heads/}$w$i$s$r"
 			else
-				printf " (%s)" "$c${b##refs/heads/}$w$i$r"
+				printf " (%s)" "$c${b##refs/heads/}$w$i$s$r"
 			fi
 		fi
 	fi

Re: [PATCH] Showing stash state in bash prompt

[Sverre Rabbelier]

Heya,

On Wed, May 13, 2009 at 11:44, Daniel Trstenjak
<Daniel.Trstenjak@science-computing.de> wrote:

Please have a look at Documentation/SubmittingPatches, available
online at [0], main points that you need to address:
1. inline your patch rather than attaching it
2. commit message
3. signed-off-by line

Feel free to ask if you have any questions wrt how to submit your
patch if the document is unclear.

[0] http://repo.or.cz/w/git.git?a=blob;f=Documentation/SubmittingPatches

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] Showing stash state in bash prompt

[Daniel Trstenjak]

Showing stash state in bash prompt.

Signed-off-by: Daniel Trstenjak <daniel.trstenjak@science-computing.de>
---
 contrib/completion/git-completion.bash |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1683e6d..86e39a5 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -117,6 +117,7 @@ __git_ps1 ()
 
 		local w
 		local i
+		local s
 		local c
 
 		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
@@ -136,15 +137,19 @@ __git_ps1 ()
 					else
 						i="#"
 					fi
+					stash_list=`git stash list`
+					if [ -n "$stash_list" ]; then
+					        s="$"
+				        fi
 				fi
 			fi
 		fi
 
 		if [ -n "$b" ]; then
 			if [ -n "${1-}" ]; then
-				printf "$1" "$c${b##refs/heads/}$w$i$r"
+				printf "$1" "$c${b##refs/heads/}$w$i$s$r"
 			else
-				printf " (%s)" "$c${b##refs/heads/}$w$i$r"
+				printf " (%s)" "$c${b##refs/heads/}$w$i$s$r"
 			fi
 		fi
 	fi
-- 
1.6.2

-- 
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Roland Niemeier, 
Dr. Arno Steitz, Dr. Ingrid Zech
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Michel Lepert
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196 

Re: [PATCH] Showing stash state in bash prompt

[Junio C Hamano]

Daniel Trstenjak <Daniel.Trstenjak@science-computing.de> writes:

> Showing stash state in bash prompt.

That's what you already said in the "Subject:" ;-)

Here in the proposed commit message, you describe what the code after
applying your patch does in a bit more detail, defend why such a new
behaviour is desirable, and defend why your implementation is superior
than possible alternative solutions to achieve that goal.

Going back to the "Subject: ", we typically put the name of the affected
area and colon to the message, and use imperative mood, as if you are
giving an order to the code to "do this (instead of what you currently
do)".

E.g.

	Subject: [PATCH] completion: show presense of stashed changes

	Users often forget that there are stashed changes that want to be
	unstashed.  Add a '$' in the prompt string to remind them.

	Signed-off-by: ...

Note that I do not necessarily agree with the above justification; I am
just illustrating the established style around here.

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 1683e6d..86e39a5 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -117,6 +117,7 @@ __git_ps1 ()
>  
>  		local w
>  		local i
> +		local s
>  		local c
>  
>  		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then

Ok.

> @@ -136,15 +137,19 @@ __git_ps1 ()
>  					else
>  						i="#"
>  					fi
> +					stash_list=`git stash list`

Don't you want to localize this variable to avoid contaminating the
namespace?

> +					if [ -n "$stash_list" ]; then
> +					        s="$"

Is the presense the only thing that matters?  Notice that I reworded your
"stash state" to "presense of" to clarify that you are giving only one bit
of information in the counter-proposed "Subject:" above.

I personally prefer your "only one bit" patch over a possible alternative
to count the stash entries with "git stash list | wc -l" that would be way
more costly, and if you thought about such an alternative and discarded
it, it would save other people's time if you say so in your proposed
commit message.  So...

	Subject: [PATCH] completion: show presense of stashed changes

	Users often forget that there are stashed changes that want to be
	unstashed.  Add a '$' in the prompt string to remind them when
	there is a stashed state.

	We could count and show the number of stash entries instead, but
        that would be more costly at runtime than it is worth.

	Signed-off-by: ...

and with localization of stash_list variable I think the patch becomes
better.

Thanks.

Re: [PATCH] Showing stash state in bash prompt

[Thomas Rast]

[-- Attachment #1: Type: text/plain, Size: 1385 bytes --]

Junio C Hamano wrote:
> Daniel Trstenjak <Daniel.Trstenjak@science-computing.de> writes:
> 
> > Showing stash state in bash prompt.
[...]
> 	Subject: [PATCH] completion: show presense of stashed changes
> 
> 	Users often forget that there are stashed changes that want to be
> 	unstashed.  Add a '$' in the prompt string to remind them.

I'd hate to see this go in without any sort of configurability, since
I would have to patch it out again for my own builds.

The way git-stash is currently documented teaches a stash/apply
workflow, in which pop is not used at all.  For example, in
git-stash.txt itself, the description of 'apply' has all the meat
while 'pop' just refers to 'apply'.  In user-manual.txt, there is an
example that teaches stash/apply.

This may historically be because 'git stash pop' was added later on:

  bd56ff5 (git-stash: add new 'pop' subcommand, 2008-02-22)
  f2c66ed (Add git-stash script, 2007-06-30)

But IMHO it would not be a good idea to teach people stash/pop anyway:
'stash drop' is irreversible, because the stash is itself implemented
through the reflog and thus not guarded by one.

And unfortunately, for people who use stash/apply instead of stash/pop
(including me :-), the proposed indicator merely shows if they have
ever used stash in the current repository.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Showing stash state in bash prompt

[Miles Bader]

Thomas Rast <trast@student.ethz.ch> writes:
> But IMHO it would not be a good idea to teach people stash/pop anyway:
> 'stash drop' is irreversible, because the stash is itself implemented
> through the reflog and thus not guarded by one.

I don't understand why you say this -- sure "drop" is dangerous, but
that's exactly why you should use "pop" instead, because it makes sure
the changes are _somewhere_.  I found with the old (pre-"pop") stash,
I'd often end up in a situation where I'd lose track of whether I had
done a stash apply or not, and the risk of inadvertently doing a drop
_without_ a corresponding apply was very real.

stash/pop is safer, more convenient, and I think it better matches how
people actually work.

-Miles

-- 
What the fuck do white people have to be blue about!?  Banana Republic ran
out of Khakis?  The Espresso Machine is jammed?  Hootie and The Blowfish
are breaking up??!  Shit, white people oughtta understand, their job is to
GIVE people the blues, not to get them!  -- George Carlin

Re: [PATCH] Showing stash state in bash prompt

[Jeff King]

On Fri, May 15, 2009 at 09:57:20AM +0900, Miles Bader wrote:

> Thomas Rast <trast@student.ethz.ch> writes:
> > But IMHO it would not be a good idea to teach people stash/pop anyway:
> > 'stash drop' is irreversible, because the stash is itself implemented
> > through the reflog and thus not guarded by one.
> 
> I don't understand why you say this -- sure "drop" is dangerous, but
> that's exactly why you should use "pop" instead, because it makes sure
> the changes are _somewhere_.  I found with the old (pre-"pop") stash,
> I'd often end up in a situation where I'd lose track of whether I had
> done a stash apply or not, and the risk of inadvertently doing a drop
> _without_ a corresponding apply was very real.

"pop" doesn't always succeed. If you have conflicts in applying, then
you end up with conflict markers, and the stash remains. You then fix up
and commit as you see fit, but your stash is still there. So this bash
prompt will nag you, which I think is what Thomas was complaining about
(but perhaps the nagging would then convince you to keep a cleaner stash
area by dropping the resolved stash).

-Peff

Re: [PATCH] Showing stash state in bash prompt

[John Tapsell]

>> I'd often end up in a situation where I'd lose track of whether I had
>> done a stash apply or not, and the risk of inadvertently doing a drop

While we're on this - would anyone else like to see a "git unstash" as
an alias to "git stash apply" ?
For me it seems more natural to be able to do :

git stash
something
git unstash

Re: [PATCH] Showing stash state in bash prompt

[Brian Gernhardt]

On May 15, 2009, at 2:39 AM, John Tapsell wrote:

>>> I'd often end up in a situation where I'd lose track of whether I  
>>> had
>>> done a stash apply or not, and the risk of inadvertently doing a  
>>> drop
>
> While we're on this - would anyone else like to see a "git unstash" as
> an alias to "git stash apply" ?
> For me it seems more natural to be able to do :
>
> git stash
> something
> git unstash

Try this:

git config --global alias.unstash "stash apply"

~~ B

Re: [PATCH] Showing stash state in bash prompt

[Miles Bader]

2009/5/15 Brian Gernhardt <benji@silverinsanity.com>:
> git config --global alias.unstash "stash apply"

Tho it should really be "stash pop"

-miles
-- 
Do not taunt Happy Fun Ball.

[RFC PATCH] Documentation: teach stash/pop workflow instead of stash/apply

[Thomas Rast]

Recent discussion on the list showed some comments in favour of a
stash/pop workflow:

  http://marc.info/?l=git&m=124234911423358&w=2
  http://marc.info/?l=git&m=124235348327711&w=2

Change the stash documentation and examples to document pop in its own
right (and apply in terms of pop), and use stash/pop in the examples.

Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---

I meant to write this for a while now, but never got around to it.

Jeff King wrote:
> On Fri, May 15, 2009 at 09:57:20AM +0900, Miles Bader wrote:
> > I don't understand why you say this -- sure "drop" is dangerous, but
> > that's exactly why you should use "pop" instead, because it makes sure
> > the changes are _somewhere_.  I found with the old (pre-"pop") stash,
> > I'd often end up in a situation where I'd lose track of whether I had
> > done a stash apply or not, and the risk of inadvertently doing a drop
> > _without_ a corresponding apply was very real.
> 
> "pop" doesn't always succeed. If you have conflicts in applying, then
> you end up with conflict markers, and the stash remains. You then fix up
> and commit as you see fit, but your stash is still there. So this bash
> prompt will nag you, which I think is what Thomas was complaining about
> (but perhaps the nagging would then convince you to keep a cleaner stash
> area by dropping the resolved stash).

Actually I was mostly concerned about dropping the stashes at all.
But I guess if you treat the stash as a short-term stack that holds a
change or two while you're working on something else, stash/pop fits
better.

I'd still prefer some configurability in the original patch, as I
think nagging the user into discarding data is a bad thing, even
though I now agree that if the 'pop' actually went through, it's not
really discarded.  (Also, ISTR a discussion about automatic gc'ing of
the stash reflog where a few people said they expect to hit any
reasonably short time limit in some of their repos and thus risk
losing work; regardless of cleanup disclipline, they would also have
the prompt on all the time).


By the way, why doesn't gmane find these mails?  I tried things like

  http://search.gmane.org/?query=stash&group=gmane.comp.version-control.git&author=miles@gnu.org

but the entire thread seems to be missing from gmane.



 Documentation/git-stash.txt   |   30 ++++++++++++++++--------------
 Documentation/user-manual.txt |    4 ++--
 2 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
index 051f94d..1cc24cc 100644
--- a/Documentation/git-stash.txt
+++ b/Documentation/git-stash.txt
@@ -75,14 +75,22 @@ show [<stash>]::
 	it will accept any format known to 'git-diff' (e.g., `git stash show
 	-p stash@\{1}` to view the second most recent stash in patch form).
 
-apply [--index] [<stash>]::
+pop [<stash>]::
 
-	Restore the changes recorded in the stash on top of the current
-	working tree state.  When no `<stash>` is given, applies the latest
-	one.  The working directory must match the index.
+	Remove a single stashed state from the stash list and apply it
+	on top of the current working tree state, i.e., do the inverse
+	operation of `git stash save`. The working directory must
+	match the index.
 +
-This operation can fail with conflicts; you need to resolve them
-by hand in the working tree.
+Applying the state can fail with conflicts; in this case, it is not
+removed from the stash list. You need to resolve the conflicts by hand
+and call `git stash drop` manually afterwards.
++
+When no `<stash>` is given, `stash@\{0}` is assumed. See also `apply`.
+
+apply [--index] [<stash>]::
+
+	Like `pop`, but do not remove the state from the stash list.
 +
 If the `--index` option is used, then tries to reinstate not only the working
 tree's changes, but also the index's ones. However, this can fail, when you
@@ -112,12 +120,6 @@ drop [<stash>]::
 	Remove a single stashed state from the stash list. When no `<stash>`
 	is given, it removes the latest one. i.e. `stash@\{0}`
 
-pop [<stash>]::
-
-	Remove a single stashed state from the stash list and apply on top
-	of the current working tree state. When no `<stash>` is given,
-	`stash@\{0}` is assumed. See also `apply`.
-
 create::
 
 	Create a stash (which is a regular commit object) and return its
@@ -163,7 +165,7 @@ $ git pull
 file foobar not up to date, cannot merge.
 $ git stash
 $ git pull
-$ git stash apply
+$ git stash pop
 ----------------------------------------------------------------
 
 Interrupted workflow::
@@ -192,7 +194,7 @@ You can use 'git-stash' to simplify the above, like this:
 $ git stash
 $ edit emergency fix
 $ git commit -a -m "Fix in a hurry"
-$ git stash apply
+$ git stash pop
 # ... continue hacking ...
 ----------------------------------------------------------------
 
diff --git a/Documentation/user-manual.txt b/Documentation/user-manual.txt
index dbbeb7e..0b88a51 100644
--- a/Documentation/user-manual.txt
+++ b/Documentation/user-manual.txt
@@ -1520,10 +1520,10 @@ $ git commit -a -m "blorpl: typofix"
 ------------------------------------------------
 
 After that, you can go back to what you were working on with
-`git stash apply`:
+`git stash pop`:
 
 ------------------------------------------------
-$ git stash apply
+$ git stash pop
 ------------------------------------------------
 
 
-- 
1.6.3.1.276.gb65cd

Re: [RFC PATCH] Documentation: teach stash/pop workflow instead of stash/apply

[Sitaram Chamarty]

On 2009-05-28 09:40:15, Thomas Rast <trast@student.ethz.ch> wrote:
> Recent discussion on the list showed some comments in favour of a
> stash/pop workflow:
>
>   http://marc.info/?l=git&m=124234911423358&w=2
>   http://marc.info/?l=git&m=124235348327711&w=2
>
> Change the stash documentation and examples to document pop in its own
> right (and apply in terms of pop), and use stash/pop in the examples.

[snip]

> +Applying the state can fail with conflicts; in this case, it is not
> +removed from the stash list. You need to resolve the conflicts by hand
> +and call `git stash drop` manually afterwards.

For what it is worth, when I take classes on git in $DAYJOB
I insist on using only save/pop (and drop when needed as
described above).  My students are largely freshers with
little or no dev experience, and this just seems much more
intuitive.  Even the aspect of having to drop manually is
logical, because something exceptional happened.

I certainly would appreciate this patch, to answer the "RFC"
aspect in your subject line.

Re: [RFC PATCH] Documentation: teach stash/pop workflow instead of stash/apply

[Junio C Hamano]

FWIW, I think the change makes sense.