Hi Chris,

On Fri, 13 May 2016, Christian Couder wrote:

On Fri, May 13, 2016 at 8:32 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

On Wed, 11 May 2016, Christian Couder wrote:

quoted

I consider that the apply functionality is properly libified before
these patches, and that they should be in a separate series, but
unfortunately using the libified apply in "git am" unmasks the fact
that "git am", since it was a shell script, has been silencing the
apply functionality by "futzing with file descriptors". And
unfortunately this makes some reviewers unhappy.

It is a misrepresentation to claim that this makes some reviewers
unhappy.  Speaking for myself, I am very happy. Despite having had to
point out that the previous iteration of this patch series had a
serious flaw.

It is also incorrect to say that the shell script had been "futzing
with the file descriptors". You see, the shell script's *own* file
descriptors had been left completely unaffected by the redirection of
the spawned process' output. That was perfectly fine a thing to do,
even if it possibly hid fatal errors. Shell scripts are simply very
limiting. The problem was introduced by v1 of this patch series, which
changed *the caller's file descriptors* back and forth simply because
the called code no longer runs in a separate process. And *that* was,
and is, improper.

I think we should just agree that we disagree on what we think the v1
was doing and move on to v2.

It is obvious that you want to disagree. But that is counterproductive.

It is important to keep in mind, and needs to be remembered in future
libification efforts, that extra care is required when we no longer spawn
a separate process: you no longer have the option to change global state
*just* for the called process.

This is very true when redirecting (global) file descriptors. I struggle
with this myself in the rebase--helper branch, so it is not only you who
has to face, and address, this issue.

The same goes for die(), too, of course. And for statically allocated
memory. And even worse: when all of a sudden reusing static lockfile
structs. And, and, and. The list goes on.

It really comes back at us that we originally simple did not care about
cleaning up after us "because it is a short-lived process, anyway".

The thing is: this is not at all the philosophical discussion as which
your comment tries to color it. We *have* to address these issues when
libifying code. Yes, it's hard. Yes, it's tedious. Yes, I also want to
bitch about it.

And yes, it must be done.

quoted

quoted

By the way there are no tests yet for this new feature, and I am not
sure at all that "--silent" and "be_silent" are good names.

If you want to follow existing code's example, we typically call this
option "quiet".

In the documentation there is: [... snip ...]
...

So it looks to me that --quiet means something like "don't tell the
story of your life, but in case of problem you are allowed to
complain". In other word --quiet generally doesn't suppress error
messages from error() or die().

Right.

And if you care to take a step back, this is most likely what we want in
libified code.

Modulo die(), of course!

On the contrary the new feature I tentatively called --silent does
suppress all output including error messages from error().

And what would be the point of that? Now that we are libifying code, in
contrast to the spawned-process approach, we *can* discern between "prints
to stderr" and "displays an error". I'd wager that you won't find any
error() call in the code path that we want to silence.

Now if people think that it is not worth making a difference between the
different behaviors, then I am ok to rename it --quiet, though I wonder
what will happen if people later want a --quiet that does only what
--quiet usually does to the other commands.

You know, I get the distinct impression that you do not want my feedback
because you always want "people" to agree with my assessment. I hoped that
my arguments would make sense, but I guess I should spend my time
differently.

quoted

quoted

Sorry if this patch series is long. I can split it into two or more
series if it is prefered.

It is preferred. Much.

Ok, I will split it then.

Thank you. Maybe you take me off of the Cc: list, too?

Thanks,
Dscho