* Re: [PATCH 1/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section
From: Junio C Hamano @ 2017-02-16 19:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: David Pursehouse, git@vger.kernel.org, David Pursehouse
In-Reply-To: <CAGZ79kaPFP3aQB8=gZ+BvRUqJa+NPuDQ+5kvKNqqYs3S28EEew@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Feb 15, 2017 at 9:05 PM, David Pursehouse
> <david.pursehouse@gmail.com> wrote:
>> From: David Pursehouse <dpursehouse@collab.net>
>>
>> Add missing `::` after the title.
>>
>> Signed-off-by: David Pursehouse <dpursehouse@collab.net>
>
> Thanks!
> Stefan
Thanks, both.
^ permalink raw reply
* Re: [PATCH v3] reset: add an example of how to split a commit into two
From: Junio C Hamano @ 2017-02-16 19:19 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller
In-Reply-To: <20170216002212.31088-1-jacob.e.keller@intel.com>
Jacob Keller <jacob.e.keller@intel.com> writes:
> The interdiff between v2 and v3 is not really worth showing since I
> basically re-wrote the entire section a bit.
Could this be made into an incremental, now that v2 has been in
'next' for about 10 days, please?
> +Split a commit apart into a sequence of commits::
> ++
> +Suppose that you have create lots of logically separate changes and commit them
s/create/&d/; s/commit/&ed/
> +together. Then, later you decide that it might be better to have each logical
> +chunk associated with its own commit. You can use git reset to rewind history
> +without changing the contents of your local files, and then successively use
> +git add -p to interactively select which hunks to include into each commit,
> +using git commit -c to pre-populate the commit message.
> ++
> +------------
> +$ git reset -N HEAD^ <1>
> +$ git add -p <2>
> +$ git diff --cached <3>
> +$ git commit -c HEAD@{1} <4>
> +... <5>
> +$ git add ... <6>
> +$ git diff --cached <7>
> +$ git commit ... <8>
> +------------
> ++
> +<1> First, reset the history back one commit so that we remove the original
> + commit, but leave the working tree with all the changes. The -N ensures
> + that any new files added with HEAD are still marked so that git add -p
> + will find them.
> +<2> Next, we interactively select diff hunks to add using the git add -p
> + facility. This will ask you about each diff hunk in sequence and you can
> + use simple commands such as "yes, include this", "No don't include this"
> + or even the very powerful "edit" facility.
> +<3> Once satisfied with the hunks you want to include, you should verify what
> + has been prepared for the first commit by using git diff --cached. This
> + shows all the changes that have been moved into the index and are about
> + to be committed.
> +<4> Next, commit the changes stored in the index. The -c option specifies to
> + pre-populate the commit message from the original message that you started
> + with in the first commit. This is helpful to avoid retyping it. The HEAD@{1}
> + is a special notation for the commit that HEAD used to be at prior to the
> + original reset commit (1 change ago). See linkgit:git-reflog[1] for more
> + details. You may also use any other valid commit reference.
> +<5> You can repeat steps 2-4 multiple times to break the original code into
> + any number of commits.
> +<6> Now you've split out many of the changes into their own commits, and might
> + no longer use the patch mode of git add, in order to select all remaining
> + uncommitted changes.
> +<7> Once again, check to verify that you've included what you want to. You may
> + also wish to verify that git diff doesn't show any remaining changes to be
> + committed later.
> +<8> And finally create the final commit.
> +
Nicely done. We could talk more "best practice" things in this
sequence (e.g. "'stash --keep' then test in isolation"), but it is
already sufficiently long, so extending it may hurt the readability
more than it helps by guiding the readers to better ways.
^ permalink raw reply
* Re: [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Junio C Hamano @ 2017-02-16 19:08 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1487258054-32292-5-git-send-email-kannan.siddharth12@gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> Instead of replacing the whole string, we would expand it accordingly using:
>
> if (*name == '-') {
> if (len == 1) {
> name = "@{-1}";
> len = 5;
> } else {
> struct strbuf changed_argument = STRBUF_INIT;
>
> strbuf_addstr(&changed_argument, "@{-1}");
> strbuf_addstr(&changed_argument, name + 1);
>
> strbuf_setlen(&changed_argument, strlen(name) + 4);
>
> name = strbuf_detach(&changed_argument, NULL);
> }
> }
>
> Junio's comments on a previous version of the patch which used this same
> approach but inside setup_revisions [1]
>
> [1]: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>
What I said is that when we know we got "-", there is no reason to
replace it with and textually parse "@{-1}".
> + if (*name == '-' && len == 1) {
> + name = "@{-1}";
> + len = 5;
> + }
> +
> ret = get_sha1_basic(name, len, sha1, lookup_flags);
If we look at get_sha1_basic(), it obviously is not prepared to
understand "-" as "@{-1}", and the primary obstacle is that the
underlying interpret_nth_prior_checkout() does two things. It
expects to take "@{-<num>}" as a string, and the first half parses
the <num> into "long nth". The latter half then finds the nth prior
checkout. We probably should factor out the latter half into a
separate function find_nth_prior_checkout() that takes "long nth" as
input, and call it from interpret_nth_prior_checkout(), as a
preparatory step. Once it is done, get_sha1_basic() can notice that
it was fed (len == 1 && str[0] == '-') and make a direct call to
find_nth_prior_checkout() without going through the "pass '@{-1}' as
text, have interpret_nth_prior_checkout() to parse it to recover 1",
which is a roundabout way to do what you want to do.
Having said all that, I do not think the remainder of the code is
prepared to take "-", not yet anyway [*1*], so turning "-" into
"@{-1}" this patch does before it calls get_sha1_basic(), while it
is not an ideal final state, is probably an acceptable milestone to
stop at.
It is a separate matter if this patch is sufficient to produce
correct results, though. I haven't studied the callers of this
change to make sure yet, and may find bugs in this approach later.
[Footnote]
*1* For example, the existing callsite in get_sha1_basic() that
calls interpret_nth_prior_checkout() does not replace "str" with
what was returned when the HEAD is not detached. The callpath
then depends on dwim_ref() to also understand "@{-1}" it got
from the caller. If we really want to keep what came from the
end user as-is so that error message can include it, we'd need
to teach dwim_ref() about the new "-" convention. The extent of
necessary change will become a lot larger. On the other hand,
if we allow error messages and reports to use a real refname
instead of parrotting exactly what the user gave us, I think we
may be able to arrange to replace str/len in get_sha1_basic()
when we call interpret/find_nth_prior_checkout() and get a ref,
without having to teach the new "-" convention all over the
place.
^ permalink raw reply
* Re: Back quote typo in error messages (?)
From: Jeff King @ 2017-02-16 19:02 UTC (permalink / raw)
To: Fabrizio Cucci; +Cc: git
In-Reply-To: <CAOxYW4x93cjMJoXYzSXCwqYVEstSLLcxzad_BPdvxfasrkxapw@mail.gmail.com>
On Thu, Feb 16, 2017 at 06:41:26PM +0000, Fabrizio Cucci wrote:
> On 15 February 2017 at 21:56, Jeff King <peff@peff.net> wrote:
> > Grep for "``" in Git's documentation directory, and you will see many
> > examples (asciidoc only accepts the double-quote form, not singles).
> >
> > You can also try:
> >
> > echo "this is \`\`quoted'' text" >foo.txt
> > asciidoc foo.txt
> >
> > and then open "foo.html" in your browser.
>
> We are probably going a bit OT here :) but AFAIK there is no such
> thing as non-symmetric start/end quotes in AsciiDoc.
>
> Even enclosing something in curved quotes is done as follows:
>
> '`single curved quotes`'
>
> "`double curved quotes`"
>
> (http://asciidoctor.org/docs/asciidoc-syntax-quick-reference/)
Try the "Quoted Text" section of the original asciidoc user manual:
http://www.methods.co.nz/asciidoc/userguide.html#X51
Asciidoctor has introduced some new syntax (almost certainly because the
original asymmetric formulations have a bunch of ambiguous corner
cases). By default, it disables the asymmetric versions, but they work
in "compat" mode (and the newer ones do not).
Git's documentation is all written for the original asciidoc. If you
build it with asciidoctor, it must be done in compat mode. This is the
default when asciidoctor sees a two-line (i.e., underlined) section
title, which all of our manpages have.
More details at:
http://asciidoctor.org/docs/migration/#migration-cheatsheet
Yes, we are off-topic,. but I think it is worth clarifying Git's
asciidoc compatibility expectations. :)
-Peff
^ permalink raw reply
* Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"
From: Junio C Hamano @ 2017-02-16 18:49 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Siddharth Kannan, git, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <vpqa89mnl4z.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> This is as per our discussion[1]. The patches and commit messages are based on
>> Junio's patches that were posted as a reply to
>> <20170212184132.12375-1-gitster@pobox.com>.
>>
>> As per Matthieu's comments, I have updated the tests, but there is still one
>> thing that is not working: log -@{yesterday} or log -@{2.days.ago}
>
> Note that I did not request that these things work, just that they seem
> to be relevant tests: IMHO it's OK to reject them, but for example we
> don't want them to segfault. And having a test is a good hint that you
> thought about what could happen and to document it.
The branch we were on before would be a ref, and the ref may know
where it was yesterday? If @{-1}@{1.day} works it would be natural
to expect -@{1.day} to, too, but there probably is some disambiguity
or other reasons that they cannot or should not work that way I am
missing, in which case it is fine ("too much work for too obscure
feature that is not expected to be used often" is also an acceptable
reason) to punt or deliberately not support it, as long as it is
explained in the log and/or doc (future developers need to know if
we are simply punting, or if we found a case where it would hurt end
user experience if we supported the feature), and as long as it does
not do a wrong thing (dying with "we do not support it" is OK,
segfaulting or doing random other things is not).
Thanks.
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Junio C Hamano @ 2017-02-16 18:49 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Lars Schneider, git, sbeller
In-Reply-To: <c609866a-f1b9-6fe5-f97a-d2180c290983@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> On 02/15/2017 03:11 PM, Junio C Hamano wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> Perhaps something like this?
>
> This looks good. I was hoping to unify the processing logic between
> this CLI parsing and the usual stream parsing, but this approach is
> probably simpler.
I looked at the stream parsing before I wrote the patch for the same
reason, and I agree that the stream parsing of course does not get a
single variable name at one place [*1*], so there is nothing that
can be shared.
[Footnote]
*1* Instead "[<section> "<subsection>"] <var> = <val>" is split and
each piece is assembled into section.subsection.var with its own
downcasing.
^ permalink raw reply
* Re: [PATCH 1/1] config.txt: Fix formatting of submodule.alternateErrorStrategy section
From: Stefan Beller @ 2017-02-16 18:41 UTC (permalink / raw)
To: David Pursehouse; +Cc: git@vger.kernel.org, David Pursehouse
In-Reply-To: <20170216050535.64593-2-david.pursehouse@gmail.com>
On Wed, Feb 15, 2017 at 9:05 PM, David Pursehouse
<david.pursehouse@gmail.com> wrote:
> From: David Pursehouse <dpursehouse@collab.net>
>
> Add missing `::` after the title.
>
> Signed-off-by: David Pursehouse <dpursehouse@collab.net>
Thanks!
Stefan
^ permalink raw reply
* Re: Back quote typo in error messages (?)
From: Fabrizio Cucci @ 2017-02-16 18:41 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170215215633.deyxp76j7o3ceoq3@sigill.intra.peff.net>
On 15 February 2017 at 21:56, Jeff King <peff@peff.net> wrote:
> Grep for "``" in Git's documentation directory, and you will see many
> examples (asciidoc only accepts the double-quote form, not singles).
>
> You can also try:
>
> echo "this is \`\`quoted'' text" >foo.txt
> asciidoc foo.txt
>
> and then open "foo.html" in your browser.
We are probably going a bit OT here :) but AFAIK there is no such
thing as non-symmetric start/end quotes in AsciiDoc.
Even enclosing something in curved quotes is done as follows:
'`single curved quotes`'
"`double curved quotes`"
(http://asciidoctor.org/docs/asciidoc-syntax-quick-reference/)
> I think patches would be welcome, but as Junio said, it probably should
> wait for the next cycle.
It can definitely wait and I would be glad to contribute!
^ permalink raw reply
* Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option
From: Matthieu Moy @ 2017-02-16 18:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Siddharth Kannan, git, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <xmqqwpcqxay0.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
>
>> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>>
>>> handle_revision_opt() tries to recognize and handle the given argument. If an
>>> option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
>>> increment of unkc causes the variable in the caller to change.
>>>
>>> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
>>> This is now the responsibility of the caller.
>>>
>>> There are two callers of this function:
>>>
>>> 1. setup_revision: Changes have been made so that setup_revision will now
>>> update the unknown option in argv
>>
>> You're writting "Changes have been made", but I did not see any up to
>> this point in the series.
>
> Actually, I think you misread the patch and explanation.
> handle_revision_opt() used to be responsible for stuffing unknown
> ones to unkv[] array passed from the caller even when it returns 0
> (i.e. "I do not know what they are" case, as opposed to "I know what
> they are, I am not handling them here and leaving them in unkv[]"
> case--the latter returns non-zero). The first hunk makes the
> function stop doing so, and to compensate, the second hunk, which is
> in setup_revisions()
Indeed, I misread the patch. The explanation could be a little bit more
"tired-reviewer-proof" by not using a past tone, perhaps
1. setup_revision, which is changed to ...
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: body-CC-comment regression
From: Matthieu Moy @ 2017-02-16 18:16 UTC (permalink / raw)
To: Johan Hovold; +Cc: git, Jeff King, Kevin Daudt, Junio C Hamano, Larry Finger
In-Reply-To: <20170216174924.GB2625@localhost>
Johan Hovold <johan@kernel.org> writes:
> Hi,
>
> I recently noticed that after an upgrade, git-send-email (2.10.2)
> started aborting when trying to send patches that had a linux-kernel
> stable-tag in its body. For example,
>
> Cc: <stable@vger.kernel.org> # 4.4
>
> was now parsed as
>
> "stable@vger.kernel.org#4.4"
>
> which resulted in
>
> Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1.
This has changed in e3fdbcc8e1 (parse_mailboxes: accept extra text after
<...> address, 2016-10-13), released v2.11.0 as you noticed:
> The problem with the resulting fixes that are now in 2.11.1 is that
> git-send-email no longer discards the trailing comment but rather
> shoves it into the name after adding some random white space:
>
> "# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>"
>
> This example is based on the example from
> Documentation/process/stable-kernel-rules.rst:
>
> Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
>
> and this format for stable-tags has been documented at least since 2009
> and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
> has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
> remove garbage after email address") I believe.
>
> Can we please revert to the old behaviour of simply discarding such
> comments (from body-CC:s) or at least make it configurable through a
> configuration option?
The problem is that we now accept list of emails instead of just one
email, so it's hard to define what "comments after the email", for
example
Cc: <foo@example.com> # , <boz@example.com>
Is not accepted as two emails.
So, just stripping whatever comes after # before parsing the list of
emails would change the behavior once more, and possibly break other
user's flow. Dropping the garbage after the email while parsing is
possible, but only when we use our in-house parser (and we currently use
Perl's Mail::Address when available).
So, a proper fix is far from obvious, and unfortunately I won't have
time to work on that, at least not before a while.
OTOH, the current behavior isn't that bad. It accepts the input, and
extracts a valid email out of it. Just the display name is admitedly
suboptimal ...
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: body-CC-comment regression
From: Johan Hovold @ 2017-02-16 18:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johan Hovold, git, Jeff King, Matthieu Moy, Kevin Daudt,
Larry Finger
In-Reply-To: <xmqq60kayq2q.fsf@gitster.mtv.corp.google.com>
On Thu, Feb 16, 2017 at 09:59:25AM -0800, Junio C Hamano wrote:
> Johan Hovold <johan@kernel.org> writes:
>
> > I recently noticed that after an upgrade, git-send-email (2.10.2)
> > started aborting when trying to send patches that had a linux-kernel
> > stable-tag in its body. For example,
> >
> > Cc: <stable@vger.kernel.org> # 4.4
> >
> > was now parsed as
> >
> > "stable@vger.kernel.org#4.4"
> > ...
>
> It sounds like a fallout of this:
>
> https://public-inbox.org/git/41164484-309b-bfff-ddbb-55153495d41a@lwfinger.net/#t
>
> and any change to "fix" you may break the other person.
Yes, that's the thread I was referring to as well, and the reported
breakage is the same even if the reporter used a non-standard stable-tag
format (e.g. a "[4.8+]" suffix).
What I'm wondering is whether the alternative fix proposed in that
thread, to revert to the old behaviour of discarding trailing comments,
should be considered instead of what was implemented.
> > Can we please revert to the old behaviour of simply discarding such
> > comments (from body-CC:s) or at least make it configurable through a
> > configuration option?
>
> If I recall the old thread correctly, it was reported that using
> Mail::Address without forcing git-send-email fall back to its own
> non-parsing-but-paste-address-looking-things-together code would
> solve it, so can the "make it configurable" be just "install
> Mail::Address"?
I believe git-send-email's parser was changed to mimic Mail::Address,
and installing it does not seem to change the behaviour of including any
trailing comments in the name.
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Johannes Sixt @ 2017-02-16 18:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702131815351.3496@virtualbox>
Am 13.02.2017 um 18:16 schrieb Johannes Schindelin:
> On Sat, 11 Feb 2017, Johannes Sixt wrote:
>> Am 10.02.2017 um 00:41 schrieb Junio C Hamano:
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>>
>>>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>>>
>>>> Use OpenSSL's SHA-1 routines rather than builtin block-sha1
>>>> routines. This improves performance on SHA1 operations on Intel
>>>> processors.
>>>> ...
>>>>
>>>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>>>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>>>> ---
>>>
>>> Nice. Will queue as jh/mingw-openssl-sha1 topic; it is a bit too
>>> late for today's integration cycle to be merged to 'next', but let's
>>> have this by the end of the week in 'master'.
>>
>> Please don't rush this through. I didn't have a chance to cross-check the
>> patch; it will have to wait for Monday. I would like to address Peff's
>> concerns about additional runtime dependencies.
>
> I never meant this to be fast-tracked into git.git. We have all the time
> in our lives to get this in, as Git for Windows already carries this patch
> for a while, and shall continue to do so.
I've been working with this patch for the past few days, and I did not
notice any disadvantage during interactive work even though there is a
new dependency on libcrypto.dll.
Here are some unscientific numbers collected during test suite runs:
bash -c "time make -j4 -k test"
with this patch:
real 34m47.242s
user 9m55.827s
sys 25m20.483s
without this patch:
real 34m2.330s
user 9m56.556s
sys 25m5.520s
It looks like BLK_SHA1 has some advantage, but I would not count on
these figures too much. (I certainly did not sit idly in front of the
workstation during these tests, for example. That may have skewed the
numbers somewhat.) (And, no, I'm not going to measure best-of-five
timings, not even best-of-two. ;)
In summary: Interactive response times do not decline noticably. I do
not object the patch.
-- Hannes
^ permalink raw reply
* Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option
From: Junio C Hamano @ 2017-02-16 18:11 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Siddharth Kannan, git, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <vpqwpcqm69k.fsf@anie.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
>
>> handle_revision_opt() tries to recognize and handle the given argument. If an
>> option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
>> increment of unkc causes the variable in the caller to change.
>>
>> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
>> This is now the responsibility of the caller.
>>
>> There are two callers of this function:
>>
>> 1. setup_revision: Changes have been made so that setup_revision will now
>> update the unknown option in argv
>
> You're writting "Changes have been made", but I did not see any up to
> this point in the series.
Actually, I think you misread the patch and explanation.
handle_revision_opt() used to be responsible for stuffing unknown
ones to unkv[] array passed from the caller even when it returns 0
(i.e. "I do not know what they are" case, as opposed to "I know what
they are, I am not handling them here and leaving them in unkv[]"
case--the latter returns non-zero). The first hunk makes the
function stop doing so, and to compensate, the second hunk, which is
in setup_revisions() that calls the function, now makes the caller
do the equivalent "argv[left++] = arg" there after it receives 0.
So "Changes have been made" to setup_revisions() to compensate for
the change of behaviour in the called function.
The enumerated point 2. (not in your response) explains why such a
corresponding compensatory change is not there for the other caller
of this function whose behaviour has changed.
> We write patch series so that they are bisectable, i.e. each commit
> should be correct (compileable, pass tests, consistent
> documentation, ...). Here, it seems you are introducing a breakage to
> repair it later.
That is a very good point to stress, but 1. is exactly to avoid
breakage in this individual step (and 2. is an explanation why the
change does not break the other caller).
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-16 18:01 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jeff Hostetler
In-Reply-To: <d1c65a34-3809-ee76-5e00-69ba715e0093@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:
>> On Wed, 15 Feb 2017, Johannes Sixt wrote:
>>> I do not see how dup2() causes a change in stdio state. I
>>> must be missing something (and that may be a basic misunderstanding of how
>>> stdio is initialized).
>>
>> It appears that dup2()ing fd 2 resets that stdio state.
>
> OK, thanks. It's surprising, but so be it.
>
> Thank you for the quick fix!
I am happy to see two Windows folks being happy. I tentatively
marked this as "undecided" after merging it to 'next', but let's
have it in the -rc2 and final. Two people familiar with Windows
agreeing that there is no longer mystery why it fixes, after seeing
that the update fixes a regression, is a strong enough sign to me
that including it is better than leaving it out.
Thanks.
^ permalink raw reply
* Re: [PATCH 12/15] unpack-trees: check if we can perform the operation for submodules
From: Brandon Williams @ 2017-02-16 18:01 UTC (permalink / raw)
To: Stefan Beller; +Cc: git, sandals, jrnieder, gitster
In-Reply-To: <20170216003811.18273-13-sbeller@google.com>
On 02/15, Stefan Beller wrote:
> +static void reload_gitmodules_file(struct index_state *index,
> + struct checkout *state)
> +{
> + int i;
> + for (i = 0; i < index->cache_nr; i++) {
> + struct cache_entry *ce = index->cache[i];
> + if (ce->ce_flags & CE_UPDATE) {
> +
> + int r = strcmp(ce->name, ".gitmodules");
> + if (r < 0)
> + continue;
> + else if (r == 0) {
> + checkout_entry(ce, state, NULL);
> + } else
> + break;
> + }
> + }
> + gitmodules_config();
> + git_config(submodule_config, NULL);
> +}
If we are reloading the gitmodules file do you think it would makes
sense to add in a call to 'submodule_free()' to clear the cache used to
store the gitmodules config?
--
Brandon Williams
^ permalink raw reply
* Re: body-CC-comment regression
From: Junio C Hamano @ 2017-02-16 17:59 UTC (permalink / raw)
To: Johan Hovold; +Cc: git, Jeff King, Matthieu Moy, Kevin Daudt, Larry Finger
In-Reply-To: <20170216174924.GB2625@localhost>
Johan Hovold <johan@kernel.org> writes:
> I recently noticed that after an upgrade, git-send-email (2.10.2)
> started aborting when trying to send patches that had a linux-kernel
> stable-tag in its body. For example,
>
> Cc: <stable@vger.kernel.org> # 4.4
>
> was now parsed as
>
> "stable@vger.kernel.org#4.4"
> ...
It sounds like a fallout of this:
https://public-inbox.org/git/41164484-309b-bfff-ddbb-55153495d41a@lwfinger.net/#t
and any change to "fix" you may break the other person.
> Can we please revert to the old behaviour of simply discarding such
> comments (from body-CC:s) or at least make it configurable through a
> configuration option?
If I recall the old thread correctly, it was reported that using
Mail::Address without forcing git-send-email fall back to its own
non-parsing-but-paste-address-looking-things-together code would
solve it, so can the "make it configurable" be just "install
Mail::Address"?
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Sixt @ 2017-02-16 17:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702161753050.3496@virtualbox>
Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:
> On Wed, 15 Feb 2017, Johannes Sixt wrote:
>> I do not see how dup2() causes a change in stdio state. I
>> must be missing something (and that may be a basic misunderstanding of how
>> stdio is initialized).
>
> It appears that dup2()ing fd 2 resets that stdio state.
OK, thanks. It's surprising, but so be it.
Thank you for the quick fix!
-- Hannes
^ permalink raw reply
* body-CC-comment regression
From: Johan Hovold @ 2017-02-16 17:49 UTC (permalink / raw)
To: git; +Cc: Jeff King, Matthieu Moy, Kevin Daudt, Junio C Hamano,
Larry Finger
Hi,
I recently noticed that after an upgrade, git-send-email (2.10.2)
started aborting when trying to send patches that had a linux-kernel
stable-tag in its body. For example,
Cc: <stable@vger.kernel.org> # 4.4
was now parsed as
"stable@vger.kernel.org#4.4"
which resulted in
Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1.
This tends to happen in the middle of a series after the cover letter
had been sent just to make things worse...
I finally got around to looking into this today and discovered this
thread ("Re: Formatting problem send_mail in version 2.10.0"):
https://marc.info/?l=git&m=147633706724793&w=2
The problem with the resulting fixes that are now in 2.11.1 is that
git-send-email no longer discards the trailing comment but rather
shoves it into the name after adding some random white space:
"# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>"
This example is based on the example from
Documentation/process/stable-kernel-rules.rst:
Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
and this format for stable-tags has been documented at least since 2009
and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
remove garbage after email address") I believe.
Can we please revert to the old behaviour of simply discarding such
comments (from body-CC:s) or at least make it configurable through a
configuration option?
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH 0/3] Fix l10n
From: Jeff King @ 2017-02-16 17:27 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git
In-Reply-To: <20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com>
On Thu, Feb 16, 2017 at 02:28:26PM +0300, Maxim Moseychuk wrote:
> In some places static size buffers can't store formatted string.
> If it be happen then git die.
>
> Maxim Moseychuk (3):
> add git_psprintf helper function
> bisect_next_all: convert xsnprintf to git_psprintf
> stop_progress_msg: convert xsnprintf to git_psprintf
Thanks for providing a series (and I think this is your first series, so
welcome!).
The overall goal looks good, and I dropped a few comments in reply to
the individual patches.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf
From: Jeff King @ 2017-02-16 17:26 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git
In-Reply-To: <20170216112829.18079-4-franchesko.salias.hudro.pedros@gmail.com>
On Thu, Feb 16, 2017 at 02:28:29PM +0300, Maxim Moseychuk wrote:
> Replace local safe string buffer allocation by git_psprintf.
Hmm. Is this one actually broken in practice?
I see:
> @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
> *p_progress = NULL;
> if (progress->last_value != -1) {
> /* Force the last update */
> - char buf[128], *bufp;
> - size_t len = strlen(msg) + 5;
> + char *bufp;
We have a fixed-size buffer here, but...
> struct throughput *tp = progress->throughput;
>
> - bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
If it's not big enough we allocate a new one. So this works for any size
of translated string. It just tries to skip the allocation in the
"short" case.
If this were in the normal progress code, I might care more about trying
to skip an allocation for performance. But since this is just in
stop_progress_msg(), I don't think the complexity is worth it. I'm
especially happy to see the magic "5" above go away.
So I think this is worth doing, but the motivation is a simplification,
not a bugfix.
> if (tp) {
> unsigned int rate = !tp->avg_misecs ? 0 :
> tp->avg_bytes / tp->avg_misecs;
> throughput_string(&tp->display, tp->curr_total, rate);
> }
> progress_update = 1;
> - xsnprintf(bufp, len + 1, ", %s.\n", msg);
> + bufp = git_psprintf(", %s.\n", msg);
> display(progress, progress->last_value, bufp);
> - if (buf != bufp)
> - free(bufp);
> + free(bufp);
This parts looks good (modulo using xstrfmt). It's probably worth
renaming "bufp" to the more usual "buf", I would think.
I do wonder if the punctuation here will give translators headaches.
E.g., in a RTL language you probably wouldn't want that period at the
end. I don't know enough to say, so I'm willing to punt on that for now.
But I suspect in the long run we may need to just take the whole
translated message, punctuation included, from the caller. There are
only two callers that actually provide a string.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf
From: Jeff King @ 2017-02-16 17:14 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git
In-Reply-To: <20170216112829.18079-3-franchesko.salias.hudro.pedros@gmail.com>
On Thu, Feb 16, 2017 at 02:28:28PM +0300, Maxim Moseychuk wrote:
> Git can't run bisect between 2048+ commits if use russian translation.
> Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
>
> Dummy solution: just increase buffer size but is not safe.
> Size gettext string is a runtime value.
Hmm. I wondered if this used to work (because xsnprintf operated on a
fixed-size fmt) and was broken in the translation. And as a consequence,
if we needed to be searching for other cases with similar bugs.
But no, in this case the fixed-size buffer was actually introduced
during the i18n step from 14dc4899e (i18n: bisect: mark strings for
translation, 2016-06-17).
I guess the other type of bug could still exist, though.
> diff --git a/bisect.c b/bisect.c
> index 21bc6daa4..8670cc97a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
> struct commit_list *tried;
> int reaches = 0, all = 0, nr, steps;
> const unsigned char *bisect_rev;
> - char steps_msg[32];
> + char *steps_msg;
So one solution would be to just bump the "32" higher here. The format
comes from the translation, so in practice it only needs to be large
enough to fit any of our translations.
That feels pretty hacky, though. In practice the set of translations is
contained, but it doesn't have to be (and certainly nobody would notice
if a new translation was added with a longer name until a user
complained).
> @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
>
> nr = all - reaches - 1;
> steps = estimate_bisect_steps(all);
> - xsnprintf(steps_msg, sizeof(steps_msg),
> - Q_("(roughly %d step)", "(roughly %d steps)", steps),
> - steps);
> +
> + steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
> + steps), steps);
> /* TRANSLATORS: the last %s will be replaced with
> "(roughly %d steps)" translation */
> printf(Q_("Bisecting: %d revision left to test after this %s\n",
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
> + free(steps_msg);
I wondered if a viable solution would be to make the whole thing one
single translatable string. It would avoid the need for the TRANSLATORS
comment. But I guess we have two "Q_" invocations here, and they might
need to be handled separately (e.g., "2 revisions", "1 step").
I also wondered if we could just make this into two printf statements
("revisions left to test", followed by "roughly N steps"). But the
commit message for 14dc4899e mentions RTL languages. So I think we
really do need to build it up block by block and let the translators
adjust the ordering.
So I think your solution is the best we can do.
It's probably worth outlining these alternatives in the commit message.
-Peff
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #04; Tue, 14)
From: Junio C Hamano @ 2017-02-16 17:10 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <df42ec26-d1b1-e31b-b16d-554441a335f0@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 14.02.2017 um 23:59 schrieb Junio C Hamano:
>> * rs/ls-files-partial-optim (2017-02-13) 2 commits
>> - ls-files: move only kept cache entries in prune_cache()
>> - ls-files: pass prefix length explicitly to prune_cache()
>>
>> "ls-files" run with pathspec has been micro-optimized to avoid one
>> extra call to memmove().
>
> Nit: The number of memmove(3) calls stays the same, but the number of
> bytes that are moved is reduced.
Blush X-< you are right of course. Will fix to say "... to avoid
having to memmove(3) unnecessary bytes."
Thanks.
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Schindelin @ 2017-02-16 17:10 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <0275af7b-eb7a-1094-a891-674300175e56@kdbg.org>
Hi Hannes,
On Wed, 15 Feb 2017, Johannes Sixt wrote:
> Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:
> > On Tue, 14 Feb 2017, Johannes Sixt wrote:
> > > Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > > > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > > > > > What we forgot was to mark stderr as unbuffered again.
> > >
> > > I do not see how the earlier patch turned stderr from unbuffered to
> > > buffered, as it did not add or remove any setvbuf() call. Can you
> > > explain?
> >
> > [ motivation and history of js/mingw-isatty snipped ]
> >
> > So instead of "bending" the target HANDLE of the existing
> > stdout/stderr (which would *naturally* have kept the
> > buffered/unbuffered nature as-is), we now redirect with correct API
> > calls.
>
> Your statement implies that at the time when winansi_init() begins,
> stdio is already initialized and the buffered/unbuffered state has been
> set for stderr. I would think that this is true.
>
> Then we swap out the file handle underlying stderr in swap_osfhnd()
> using dup2(). Why would that change the buffered state of stdio?
The file handle we swap in for stderr points to the pipe that a
freshly-started thread consumes for parsing the ANSI color sequences. This
handle is used both for stdout and stderr. The dup2() call then implicitly
reopens stderr, with the default buffering.
> > And the patch I provided at the bottom of this mail thread reinstates
> > the unbuffered nature of stderr now that it gets reopened.
> >
> > Hopefully that makes it clear why the setvbuf() call is required now,
> > but was previously unnecessary?
>
> Unfortunately, no. I do not see how dup2() causes a change in stdio state. I
> must be missing something (and that may be a basic misunderstanding of how
> stdio is initialized).
It appears that dup2()ing fd 2 resets that stdio state.
Ciao,
Dscho
^ permalink raw reply
* [PATCH V2 2/2] stop_progress_msg: convert xsnprintf to xstrfmt
From: Maxim Moseychuk @ 2017-02-16 17:07 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In-Reply-To: <20170216170713.10065-1-franchesko.salias.hudro.pedros@gmail.com>
Replace local safe string buffer allocation by xstrfmt.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
progress.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/progress.c b/progress.c
index 76a88c573..29d0dad58 100644
--- a/progress.c
+++ b/progress.c
@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
*p_progress = NULL;
if (progress->last_value != -1) {
/* Force the last update */
- char buf[128], *bufp;
- size_t len = strlen(msg) + 5;
+ char *bufp;
struct throughput *tp = progress->throughput;
- bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
if (tp) {
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
throughput_string(&tp->display, tp->curr_total, rate);
}
progress_update = 1;
- xsnprintf(bufp, len + 1, ", %s.\n", msg);
+ bufp = xstrfmt(", %s.\n", msg);
display(progress, progress->last_value, bufp);
- if (buf != bufp)
- free(bufp);
+ free(bufp);
}
clear_progress_signal();
if (progress->throughput)
--
2.11.1
^ permalink raw reply related
* [PATCH V2 1/2] bisect_next_all: convert xsnprintf to xstrfmt
From: Maxim Moseychuk @ 2017-02-16 17:07 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In-Reply-To: <20170216170713.10065-1-franchesko.salias.hudro.pedros@gmail.com>
Git can't run bisect between 2048+ commits if use russian translation.
Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
Dummy solution: just increase buffer size but is not safe.
Size gettext string is a runtime value.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
bisect.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/bisect.c b/bisect.c
index 21bc6daa4..787543cad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
- char steps_msg[32];
+ char *steps_msg;
read_bisect_terms(&term_bad, &term_good);
if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
nr = all - reaches - 1;
steps = estimate_bisect_steps(all);
- xsnprintf(steps_msg, sizeof(steps_msg),
- Q_("(roughly %d step)", "(roughly %d steps)", steps),
- steps);
+
+ steps_msg = xstrfmt(Q_("(roughly %d step)", "(roughly %d steps)",
+ steps), steps);
/* TRANSLATORS: the last %s will be replaced with
"(roughly %d steps)" translation */
printf(Q_("Bisecting: %d revision left to test after this %s\n",
"Bisecting: %d revisions left to test after this %s\n",
nr), nr, steps_msg);
+ free(steps_msg);
return bisect_checkout(bisect_rev, no_checkout);
}
--
2.11.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox