* Re: Confusing git messages when disk is full.
From: Jeff King @ 2017-02-16 16:44 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <87tw7uv439.fsf@linux-m68k.org>
On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
> >> int xfclose(FILE *fp)
> >> {
> >> return ferror(fp) | fclose(fp);
> >> }
> >
> > Yes, that's exactly what I had in mind (might be worth calling out the
> > bitwise-OR, though, just to make it clear it's not a typo).
>
> Since the order of evaluation is unspecified, it would be better to
> force sequencing ferror before fclose.
Good point. Arguably the call in tempfile.c is buggy.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option
From: Matthieu Moy @ 2017-02-16 16:48 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com>
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.
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.
Other that bisectability, this makes review harder: at this point the
reader knows it's broken, guesses that it will be repaired later, but
does not know in which patch.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-16 16:59 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jonathan Tan, git, sbeller
In-Reply-To: <D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> On 16 Feb 2017, at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The "git -c <var>=<val> cmd" mechanism is to pretend that a
>
> The problem is also present for gitconfig variables e.g.
> git config --local submodule.UPPERSUB.update none
Yuck.
> But your patch below fixes that, too!
Heh.
> Should we add a test case to this patch?
> If not, do you want me to improve my test case patch [1]
> or do you want to ditch the test?
I think a new test for submodule (although it already exists thanks
t you) is a bit too roundabout way to demonstrate the breakage and
protect the fix.
We'd perhaps want some tests for "git config", probably.
In a repository with /etc/gitconfig and $HOME/ that are tightly
controlled (I think our test framework already do so), running these
would demonstrate both breakages, I would think, but I am sure
people can come up with other forms that are a lot easier to read..
$ git -c v.A.r=val -c v.a.r=ue config --get-all v.a.r
$ git -c v.A.r=val -c v.a.r=ue config --get-all v.A.r
$ git -c v.a.r=val -c v.A.r=ue config --get-all v.a.r
$ git -c v.a.r=val -c v.A.r=ue config --get-all v.A.r
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] add git_psprintf helper function
From: Jeff King @ 2017-02-16 17:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Maxim Moseychuk, git
In-Reply-To: <616a2b08-71f9-d594-d46c-2687bf20ae2b@google.com>
On Thu, Feb 16, 2017 at 07:51:14AM -0800, Jonathan Tan wrote:
> On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:
> > There are a number of places in the code where we call
> > xsnprintf(), with the assumption that the output will fit into
> > the buffer. If the buffer is small, then git die.
> > In many places buffers have compile-time size, but generated string
> > depends from current system locale (gettext)and can have size
> > greater the buffer.
> > Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8"
> > on linux sources - it impossible.
> >
> > git_psprintf is similar to the standard C sprintf() function
> > but safer, since it calculates the maximum space required
> > and allocates memory to hold the result.
> > The returned string should be freed with free() when no longer needed.
>
> If I understand this correctly, xstrfmt (in strbuf.h) should already do what
> you need, so you do not need a new function.
Yes, this is exactly what xstrfmt is for.
-Peff
^ permalink raw reply
* [PATCH V2 0/2] Fix l10n
From: Maxim Moseychuk @ 2017-02-16 17:07 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In some places static size buffers can't store formatted string.
If it be happen then git die.
Jonathan Tan: Thanks a lot for your help.
Maxim Moseychuk (2):
bisect_next_all: convert xsnprintf to xstrfmt
stop_progress_msg: convert xsnprintf to xstrfmt
bisect.c | 9 +++++----
progress.c | 9 +++------
2 files changed, 8 insertions(+), 10 deletions(-)
--
2.11.1
^ permalink raw reply
* [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
* [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
* 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
* 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 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: [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 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
* 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] 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
* 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 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: [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 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: 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: 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: 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: [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: 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/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: [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
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