patchShowing stash state in bash prompt

Greetings Daniel

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

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

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

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.

Ok.

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

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.

Junio C Hamano wrote:

[...]

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@student.ethz.ch> writes:

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

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

"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

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

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

Try this:

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

~~ B

2009/5/15 Brian Gernhardt <benji@silverinsanity.com>:

Tho it should really be "stash pop"

-miles

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:

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

On 2009-05-28 09:40:15, Thomas Rast <trast@student.ethz.ch> wrote:

[snip]

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.

FWIW, I think the change makes sense.