[PATCH] git stash: one bug and one feature request

[PATCH] git stash: one bug and one feature request

[Marco Costalba]

Currently git-stash writes to stderr also if there is nothing to error
out, also it would be very nice ;-) if git 'stash clear command' would
support deleting of only one patch, so as example to write

stg stash clear stash@{0}

To remove only the last added.


------------------  cut --------------------------

From: Marco Costalba <mcostalba@gmail.com>
Date: Fri, 4 Jan 2008 17:08:01 +0100
Subject: [PATCH] git-stash: avoid writing to stderr when is not an error

Otherwise git-stash is unusable by scripts that check
stderr to detect fail/success of launched command.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 git-stash.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 06cb177..a05a47a 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -86,7 +86,7 @@ save_stash () {

 	if no_changes
 	then
-		echo >&2 'No local changes to save'
+		echo > 'No local changes to save'
 		exit 0
 	fi
 	test -f "$GIT_DIR/logs/$ref_stash" ||
@@ -99,7 +99,7 @@ save_stash () {

 	git update-ref -m "$stash_msg" $ref_stash $w_commit ||
 		die "Cannot save the current status"
-	printf >&2 'Saved working directory and index state "%s"\n' "$stash_msg"
+	printf > 'Saved working directory and index state "%s"\n' "$stash_msg"
 }

 have_stash () {
@@ -229,7 +229,7 @@ create)
 	if test $# -eq 0
 	then
 		save_stash &&
-		echo >&2 '(To restore them type "git stash apply")' &&
+		echo > '(To restore them type "git stash apply")' &&
 		git-reset --hard
 	else
 		usage
-- 
1.5.4.rc2.18.g530e6

Re: [PATCH] git stash: one bug and one feature request

[Brandon Casey]

On Fri, 4 Jan 2008, Marco Costalba wrote:

> Currently git-stash writes to stderr also if there is nothing to error
> out, also it would be very nice ;-) if git 'stash clear command' would
> support deleting of only one patch, so as example to write
>
> stg stash clear stash@{0}
>
> To remove only the last added.

Maybe it should be named 'drop'. 'drop' sounds better than
'clear' for this usage.

   git stash drop [<stash>]

Not sure how often such a command would be used though, so
it may not be worth it.

-brandon

Re: [PATCH] git stash: one bug and one feature request

[Pascal Obry]

Brandon Casey a écrit :
> Not sure how often such a command would be used though, so
> it may not be worth it.

I've missed it many times. Especially in some scripts when I want to use
the stash-stack to store current working tree and clear it before
exiting. This is not possible today as all the stash-stack would be cleared.

I agree that drop seems better.

Pascal.

-- 

--|------------------------------------------------------
--| Pascal Obry                           Team-Ada Member
--| 45, rue Gabriel Peri - 78114 Magny Les Hameaux FRANCE
--|------------------------------------------------------
--|              http://www.obry.net
--| "The best way to travel is by means of imagination"
--|
--| gpg --keyserver wwwkeys.pgp.net --recv-key C1082595

Re: [PATCH] git stash: one bug and one feature request

[Jakub Narebski]

Pascal Obry <pascal@obry.net> writes:
> Brandon Casey a écrit :
> >
> > Not sure how often such a command would be used though, so
> > it may not be worth it.
> 
> I've missed it many times. Especially in some scripts when I want to use
> the stash-stack to store current working tree and clear it before
> exiting. This is not possible today as all the stash-stack would be cleared.
> 
> I agree that drop seems better.

or "git stash delete"

This probably would require the command to delete single reflog,
which was posted some time ago and is in either pu or in offcuts,
or in next.

But I guess this is post 1.5.4
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] git stash: one bug and one feature request

[Brandon Casey]

Marco Costalba wrote:
> Ok, drop is better then clear, but, if we need to add a new command I
> vote for 'delete' or 'rm' to be consistent with git naming.

If the stash list is thought of as a stack, then drop makes sense.

I imagine using it like

   git stash apply
   git stash drop
   git stash apply stash@{3}
   git stash drop stash@{3}

'git stash delete' and 'git stash rm' when used without arguments
both sound like 'git stash clear' to me.

-brandon

Re: [PATCH] git stash: one bug and one feature request

[Marco Costalba]

On Jan 4, 2008 7:00 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
> Marco Costalba wrote:
> > Ok, drop is better then clear, but, if we need to add a new command I
> > vote for 'delete' or 'rm' to be consistent with git naming.
>
> If the stash list is thought of as a stack, then drop makes sense.
>

Yes, but is _not_ as a stack because you can say

git stash apply stash@{3}
git stash apply stash@{1}
git stash apply stash@{4}

i.e. you can access reflogs in any order, so thinking to a stack is
misleading IMHO.

Marco

Re: [PATCH] git stash: one bug and one feature request

[Brandon Casey]

Marco Costalba wrote:
> On Jan 4, 2008 7:00 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>> Marco Costalba wrote:
>>> Ok, drop is better then clear, but, if we need to add a new command I
>>> vote for 'delete' or 'rm' to be consistent with git naming.
>> If the stash list is thought of as a stack, then drop makes sense.
>>
> 
> Yes, but is _not_ as a stack because you can say
> 
> git stash apply stash@{3}
> git stash apply stash@{1}
> git stash apply stash@{4}
> 
> i.e. you can access reflogs in any order, so thinking to a stack is
> misleading IMHO.

I think it is like a stack because new things are always added to the top
and shift everything else down.
i.e. we can't say 'git stash replace stash@{3}' and we probably wouldn't
want to.

When we call git stash, the previous item on 'top' is pushed down
so that it is the second item stash@{1}. The new item just stashed
(pushed), is now on top at stash@{0}.

Doesn't seem like too far of a stretch.

-brandon

Re: [PATCH] git stash: one bug and one feature request

[Marco Costalba]

On Jan 4, 2008 7:34 PM, Brandon Casey <casey@nrlssc.navy.mil> wrote:
>
> Doesn't seem like too far of a stretch.
>

I'm very bad at naming, so I don't argue any more.

Just one thing (that is the only that matters) call this command as
you want but let it take one argument, the name of the reflog to
remove:

git stash drop stash@{3}

should be allowed.

git stash drop

defaults to  stash@{0}

Thanks
Marco

Re: [PATCH] git stash: one bug and one feature request

[Brian Swetland]

[Jakub Narebski <jnareb@gmail.com>]
> Pascal Obry <pascal@obry.net> writes:
> > Brandon Casey a écrit :
> > >
> > > Not sure how often such a command would be used though, so
> > > it may not be worth it.
> > 
> > I've missed it many times. Especially in some scripts when I want to use
> > the stash-stack to store current working tree and clear it before
> > exiting. This is not possible today as all the stash-stack would be cleared.
> > 
> > I agree that drop seems better.
> 
> or "git stash delete"

Something like drop or delete would be nice.

I tried to "clear" a single stash once. Oops!

Is there a reason that git stash apply couldn't take a small integer
as the argument (at least as an alternative) instead of stash@{0}, etc?

Brian

Re: [PATCH] git stash: one bug and one feature request

[Jeff King]

On Fri, Jan 04, 2008 at 09:51:05AM -0800, Jakub Narebski wrote:

> or "git stash delete"
> 
> This probably would require the command to delete single reflog,
> which was posted some time ago and is in either pu or in offcuts,
> or in next.
> 
> But I guess this is post 1.5.4

There is a "git reflog delete" in next (but not in master). See
552cecc2. Using the same name makes sense, since they are equivalent
actions (and "git stash delete" should be very easy, since it is
implemented in terms of reflogs).

-Peff

Re: [PATCH] git stash: one bug and one feature request

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> Otherwise git-stash is unusable by scripts that check
> stderr to detect fail/success of launched command.

Sorry, but I happen to disagree with your notion of "having
something on stderr is an error" to begin with.  I think scripts
written that way are either simply bogus, or are working around
a defect in the underlying command it calls (perhaps it does not
signal error with exit status properly).

A command that produces machine parsable output should write
that out to stdout, and if it needs to emit other informational
messages meant for human consumption (this includes progress
bars), that should be sent to stderr so that scripts can get the
meat of the output without having to filter cruft out.

If the command does not signal an error by exiting with non-zero
status, that would be a bug indeed and you can fix that instead,
I think.

Re: [PATCH] git stash: one bug and one feature request

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> I'm very bad at naming, so I don't argue any more.
>
> Just one thing (that is the only that matters) call this command as
> you want but let it take one argument, the name of the reflog to
> remove:
>
> git stash drop stash@{3}
>
> should be allowed.
>
> git stash drop
>
> defaults to  stash@{0}

I do not care the wording either way, but my prediction is that
people will mistype "stash clear" when they meant "stash drop",
and we will end up not allowing the implicit "drop the top one
only" behaviour.

Re: [PATCH] git stash: one bug and one feature request

[Wayne Davison]

On Fri, Jan 04, 2008 at 05:14:42PM +0100, Marco Costalba wrote:
> -		echo >&2 'No local changes to save'
> +		echo > 'No local changes to save'

That change and the other two following it each put a newline in a
strangely named file.  You should just drop the >&2 altogether if you
want the output to go to stdout.

..wayne..

Re: [PATCH] git stash: one bug and one feature request

[Junio C Hamano]

Wayne Davison <wayne@opencoder.net> writes:

> On Fri, Jan 04, 2008 at 05:14:42PM +0100, Marco Costalba wrote:
>> -		echo >&2 'No local changes to save'
>> +		echo > 'No local changes to save'
>
> That change and the other two following it each put a newline in a
> strangely named file.  You should just drop the >&2 altogether if you
> want the output to go to stdout.

Lol...  Good eyes.  I did not even notice it ;-).

Thanks.

Re: [PATCH] git stash: one bug and one feature request

[Marco Costalba]

On Jan 5, 2008 1:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Marco Costalba" <mcostalba@gmail.com> writes:
>
> > Otherwise git-stash is unusable by scripts that check
> > stderr to detect fail/success of launched command.
>
> Sorry, but I happen to disagree with your notion of "having
> something on stderr is an error" to begin with.  I think scripts
> written that way are either simply bogus, or are working around
> a defect in the underlying command it calls (perhaps it does not
> signal error with exit status properly).
>

I understand your point. The problem is that in git there isn't an
unique way to test success/fail for any command, as example, regarding
checking the exit status:

$ git status; echo $?
# On branch master
nothing to commit (working directory clean)
1


You get a value different from zero also in case of no error. The
checking for stderr I have found is more reliable for the git
command/scripts I use.

> A command that produces machine parsable output should write
> that out to stdout, and if it needs to emit other informational
> messages meant for human consumption (this includes progress
> bars), that should be sent to stderr so that scripts can get the
> meat of the output without having to filter cruft out.
>

I agree with this, but I fail to see the machine parsable output and
human consumption sideband info in case of git-stash that I would say
does not foreseen machine  parsable output at all, so in this case
choice of writing to stderr is less clear to me.

> If the command does not signal an error by exiting with non-zero
> status, that would be a bug indeed and you can fix that instead,
> I think.
>

If we don't want to have general rule for exit status and stderr at
least we could add a -q option to git stash, altough I would prefer
git stash writing on stdout if is not an error.

Please let me explain again why I need a reliable way to detect
success/fail of a command. When a function wants to execute a git
command it passes a string with the command + arguments to a low level
routine, say run(), that is command agnostic. This run() function
adapts and formats the command line according to the OS environment
then runs the command, saves the results and check for an error, the
result buffer is then passed as is to the caller that has the semantic
knowledge of what the command have produced.

This low level run() should know nothing about the semantic of the
command or the outputted data, but should detect command failing,
because failing reporting framework is unified and is the same for
each type of command.

A good and reliable way is to check for stderr, because it happens to
be more reliable then exit codes.

Please note that also gitk uses the same approach, indeed from
http://ftp.tcl.tk/man/tcl8.5/tutorial/Tcl26.html you can read:

--------------------

The 'exec' treats any output to standard error to be an indication
that the external program failed. This is simply a conservative
assumption: many programs behave that way and they are sloppy in
setting return codes.

Some programs however write to standard error without intending this
as an indication of an error. You can guard against this from
upsetting your script by using the catch command:

if { [catch { exec ls *.tcl } msg] } {
   puts "Something seems to have gone wrong but we will ignore it"
}

-------------------------

Indeed in gitk you find something like

    # Unfortunately git-cherry-pick writes stuff to stderr even when
    # no error occurs, and exec takes that as an indication of error...
    if {[catch {exec sh -c "git cherry-pick -r $rowmenuid 2>&1"} err]} {
	notbusy cherrypick
	error_popup $err
	return
    }



I can also black list not commonly behaving programs, but in case of
git-stash a fail to see why to choose a not standard behaviour when
not needed.


Thanks
Marco

Re: [PATCH] git stash: one bug and one feature request

[Marco Costalba]

On Jan 5, 2008 7:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Wayne Davison <wayne@opencoder.net> writes:
>
> > On Fri, Jan 04, 2008 at 05:14:42PM +0100, Marco Costalba wrote:
> >> -            echo >&2 'No local changes to save'
> >> +            echo > 'No local changes to save'
> >
> > That change and the other two following it each put a newline in a
> > strangely named file.  You should just drop the >&2 altogether if you
> > want the output to go to stdout.
>
> Lol...  Good eyes.  I did not even notice it ;-).
>

Very sorry for this, I should have been more careful. Please let me
know if you want me to resend the patch.

Marco

Re: [PATCH] git stash: one bug and one feature request

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> This low level run() should know nothing about the semantic of the
> command or the outputted data, but should detect command failing,
> because failing reporting framework is unified and is the same for
> each type of command.

That sounds like a framework generalized in a wrong way to me.

> Please note that also gitk uses the same approach, indeed from
> http://ftp.tcl.tk/man/tcl8.5/tutorial/Tcl26.html you can read:
> ...

Heh, as if tcl is a textbook of good programming style.

> I can also black list not commonly behaving programs, but in case of
> git-stash a fail to see why to choose a not standard behaviour when
> not needed.

I do not offhand see a reason it would _hurt_ for this
particular case (git-stash) to write the diagnostics we
currently spit out to stderr to stdout.  My objection is
primarily because I do not think "never writing to stderr if
there is no error" is standard behaviour AT ALL.

IOW, I do have much less objections to what your patch actually
does, than I have problems with the way the reason for the
change is stated.  The change is not fixing anything to conform
to some standard behaviour.  It is more about bending
(admittedly only slightly) backwards to help broken callers.
That is what I have most trouble with.

Re: [PATCH] git stash: one bug and one feature request

[Marco Costalba]

On Jan 5, 2008 9:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> IOW, I do have much less objections to what your patch actually
> does, than I have problems with the way the reason for the
> change is stated.  The change is not fixing anything to conform
> to some standard behaviour.  It is more about bending
> (admittedly only slightly) backwards to help broken callers.
> That is what I have most trouble with.
>
>

Thanks for your understanding.

--------------------- CUT -----------------------------------

Subject: [PATCH] git-stash: use stdout instead of stderr for not error messages

Some scripts/libraries commonly check stderr to detect a
failing command. This is not standard nor good behaviour but
is quite common and in this case the change does not seem to hurt.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>
---
 git-stash.sh |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 06cb177..4d5e5c0 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -86,7 +86,7 @@ save_stash () {

 	if no_changes
 	then
-		echo >&2 'No local changes to save'
+		echo 'No local changes to save'
 		exit 0
 	fi
 	test -f "$GIT_DIR/logs/$ref_stash" ||
@@ -99,7 +99,7 @@ save_stash () {

 	git update-ref -m "$stash_msg" $ref_stash $w_commit ||
 		die "Cannot save the current status"
-	printf >&2 'Saved working directory and index state "%s"\n' "$stash_msg"
+	printf 'Saved working directory and index state "%s"\n' "$stash_msg"
 }

 have_stash () {
@@ -229,7 +229,7 @@ create)
 	if test $# -eq 0
 	then
 		save_stash &&
-		echo >&2 '(To restore them type "git stash apply")' &&
+		echo '(To restore them type "git stash apply")' &&
 		git-reset --hard
 	else
 		usage
-- 
1.5.4.rc2.18.g530e6-dirty

Re: [PATCH] git stash: one bug and one feature request

[Junio C Hamano]

"Marco Costalba" <mcostalba@gmail.com> writes:

> I understand your point. The problem is that in git there isn't an
> unique way to test success/fail for any command, as example, regarding
> checking the exit status:
>
> $ git status; echo $?
> # On branch master
> nothing to commit (working directory clean)
> 1

That is a bad example, with a slight historical background.

When you say "git status $args", you are asking the command this
question.

	I am contemplating to issue "git commit $args", but will
	there actually be changes if I issued that command?

When there will be no changes staged with the given $args (in
your case that happens to be empty), there won't be anything to
be committed if you issued "git commit $args" at that point.
The command answers "Eh, by issuing 'git commit' you will get
an 'Nothing to commit', which is an error" --- and that is
reported with its exit status.