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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Dscho,

On Fri, May 13, 2016 at 8:32 AM, Johannes Schindelin
<Johannes.Schindelin@xxxxxx> wrote:
> 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.

>> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]