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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 5, 2008, 08:36 UTC
Message-ID: <7vbq80d5yp.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <e5bfff550801050025g6758bfb6p751e69e93d4299be@mail.gmail.com>

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