Re: [PATCH v2 00/94] libify apply and use lib in am

From: Christian Couder <christian.couder@gmail.com>
Date: May 13, 2016, 18:49 UTC
Message-ID: <CAP8UFD2fSW0nds7myfb_09c+EaCdn6+1OqsNEzfY2rVTeA5TAw@mail.gmail.com>
In-Reply-To: <alpine.DEB.2.20.1605130820160.4092@virtualbox>

Hi Dscho,

On Fri, May 13, 2016 at 8:32 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 23 quoted lines

> Hi Chris,
>
> On Wed, 11 May 2016, Christian Couder wrote:
>
>> 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.

Show 5 quoted lines

>> 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:

Documentation/git-am.txt:--quiet:: Documentation/git-am.txt: Be quiet. Only print error messages. Documentation/git-branch.txt:--quiet:: Documentation/git-branch.txt: Be more quiet when creating or deleting a branch, suppressing Documentation/git-branch.txt- non-error messages. Documentation/git-checkout-index.txt:--quiet:: Documentation/git-checkout-index.txt: be quiet if files exist or are not in the index Documentation/git-checkout.txt:--quiet:: Documentation/git-checkout.txt- Quiet, suppress feedback messages. Documentation/git-clean.txt:--quiet:: Documentation/git-clean.txt: Be quiet, only report errors, but not the files that are Documentation/git-clean.txt- successfully removed. Documentation/git-clone.txt--q:: Documentation/git-clone.txt: Operate quietly. Progress is not reported to the standard Documentation/git-clone.txt- error stream. ocumentation/git-commit.txt:--quiet:: Documentation/git-commit.txt- Suppress commit summary message. Documentation/git-fast-import.txt:--quiet:: Documentation/git-fast-import.txt- Disable all non-fatal output, making fast-import silent when it ...

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

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

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.

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

Thanks, Christian.