* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Junio C Hamano @ 2017-02-16 0:20 UTC (permalink / raw)
To: Philip Oakley; +Cc: Johannes Schindelin, Christian Couder, git-for-windows, git
In-Reply-To: <77C7E23E18774409AA1818B12C844985@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> It may even be worth 'splitting' the pu branch sequence into the
> existing pu (with merges from series that are selected as reasonable),
> and then a pr branch (public review?) on top of that holding the rest
> of the series that have been submitted, so that the CI can do a full
> test on the tips of them to support those devs with limited test
> capability.
I won't stop you from publishing such a pr branch yourself.
For patches whose merit is not clear because the problem they try to
solve is under-explained, whose solution is ill-designed, etc., IOW,
with issues that makes me judge that they are not interesting enough
for 'pu', it is not worth my time to deal with whitespace brekages
in them to make them not even apply, to figure out what base the
patches are meant to apply to, or to resolve conflicts caused by
them with topics already in flight, etc.
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Jeff King @ 2017-02-15 23:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jáchym Barvínek, git
In-Reply-To: <xmqq37ff2hn8.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 02:50:19PM -0800, Junio C Hamano wrote:
> > That works, but the fact that we need a comment is a good sign that it's
> > kind of gross. It's too bad stdio does not specify the return of fclose
> > to report an error in the close _or_ any previous error. I guess we
> > could wrap it with our own function.
>
> Sure. I am happy to add something like this:
>
> /*
> * closes a FILE *, returns 0 if closing and all the
> * previous stdio operations on fp were successful,
> * otherwise non-zero.
> */
> 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).
> I do not think we should try to do anything fancier to allow the
> caller to tell ferror() and fclose() apart, as such a caller would
> then need to do
Absolutely. If they care, they can call the two separately.
You are right that errno is untrustworthy in the ferror() case, though.
Maybe that is reason not to add xfclose, and just force this caller to
do something like:
if (ferror(fp))
rc = error("unable to write to %s", filename);
if (fclose(fp))
rc = error_errno("unable to write to %s", filename);
Of course, if the earlier error persists through fclose, we'd print two
errors. This would all be much easier if the filehandles kept not just
an error bit, but the original errno. <sigh>
Maybe a not-terrible compromise is to fake the errno in the ferror case,
like:
int xfclose(FILE *fp)
{
int error_flag = ferror(fp);
/*
* If we get an error from fclose, the current errno value is
* trustworthy. But if it succeeds and we had a previous error,
* we need to report failure, but the value of errno could
* be unrelated. Make up a generic errno value.
*/
if (fclose(fp))
return EOF;
} else if (error_flag) {
errno = EINVAL; /* or EIO? */
return EOF;
} else {
return 0;
}
}
Or maybe that would just confuse people when they later get "invalid
argument" in the error message. I wish there was an errno value for "I
don't remember what the error was".
I dunno. This whole thing is ending up a lot more complicated than I had
hoped. I just didn't want to have to say "unable to write to %s" twice. ;)
-Peff
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Philip Oakley @ 2017-02-15 23:57 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Christian Couder, Junio C Hamano, git-for-windows, git
In-Reply-To: <alpine.DEB.2.20.1702151509251.3496@virtualbox>
From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> Hi Philip,
>
> On Tue, 14 Feb 2017, Philip Oakley wrote:
>
>> From: "Christian Couder" <christian.couder@gmail.com>
>> > On Tue, Feb 14, 2017 at 10:08 PM, Junio C Hamano <gitster@pobox.com>
>> > wrote:
>> > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > >
>> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:
>> > > >
>> > > > > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> > > > >
>> > > > > > That is why I taught the Git for Windows CI job that tests the
>> > > > > > four upstream Git integration branches to *also* bisect test
>> > > > > > breakages and then upload comments to the identified commit on
>> > > > > > GitHub
>> > > > >
>> > > > > Good. I do not think it is useful to try 'pu' as an aggregate
>> > > > > and expect it to always build and work [*1*], but your "bisect
>> > > > > and pinpoint" approach makes it useful to identify individual
>> > > > > topic that brings in a breakage.
>> > > >
>> > > > Sadly the many different merge bases[*1*] between `next` and `pu`
>> > > > (which are the obvious good/bad points for bisecting
>> > > > automatically) bring my build agents to its knees. I may have to
>> > > > disable the bisecting feature as a consequence.
>> >
>> > Yeah, this is a bug in the bisect algorithm. Fixing it is in the GSoC
>> > 2017 Ideas.
>>
>> There are also a few ideas at the SO answers:
>> http://stackoverflow.com/a/5652323/717355
>
> Thanks for that link!
>
> However, my main aim was not to get distracted into yet another corner of
> Git that needs to be fixed (I am on enough of those projects already).
>
> I was merely surprised (and not in a good way) that a plenty ordinary
> bisect between `next` and `pu` all of a sudden tested a *one year old*
> commit whether it was good or not.
>
> And I doubt that the strategy to mark all second parents of all merge
> commits in pu..next as "good" would work well, as the merge bases *still*
> would have to be tested.
I was expecting that if all the second parents were marked as good, then
there would be no merge bases, as there shouldn't be a forked graph, just
the linear string of pearls - if bisect doesn't do that then there's an
failed optimisation to be had.
I don't see anything in the `git bisect --help` page that would indicate
that the merges themselves are omitted from the bisection.
>
> I guess what I have to resort to is this: if I know that `next` tests
> fine, and that `pu` fails, I shall mark all merge bases as "good". I am
> sure this has its own set of pitfalls, undoubtedly costing me more time on
> that front...
>
> But at least my cursory analysis of this idea seems to make sense: I use
> `next` essentially as a catch-all to verify that the breakage has entered
> `pu`, but not yet `next`. This reasoning makes sense, given that we know
> the waterfall topology of pu/next/master/maint: patches enter from left to
> right, i.e. anything that entered `pu` may later enter `next`, but not
> vice versa.
It may even be worth 'splitting' the pu branch sequence into the existing pu
(with merges from series that are selected as reasonable), and then a pr
branch (public review?) on top of that holding the rest of the series that
have been submitted, so that the CI can do a full test on the tips of them
to support those devs with limited test capability.
>
> Ciao,
> Dscho
>
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Junio C Hamano @ 2017-02-15 23:53 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jonathan Tan, Lars Schneider, git@vger.kernel.org
In-Reply-To: <CAGZ79kZgmtH1-1i5Fenq8kELbafBL1tCx66SGqGVBmjbpLoBGQ@mail.gmail.com>
Ahh, that would work, too.
On Wed, Feb 15, 2017 at 3:43 PM, Stefan Beller <sbeller@google.com> wrote:
> On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> Yes; though I'd place it in strbuf.{c,h} as it is operating
>>> on the internals of the strbuf. (Do we make any promises outside of
>>> strbuf about the internals? I mean we use .buf all the time, so maybe
>>> I am overly cautious here)
>>
>> I'd rather have it not use struct strbuf as an interface. It only
>> needs to pass "char *" and its promise that it touches the string
>> in-place without changing the length need to be documented as a
>> comment before the function.
>>
>>>> config.c | 16 +++++++++++++++-
>>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/config.c b/config.c
>>>> index c6b874a7bf..98bf8fee32 100644
>>>> --- a/config.c
>>>> +++ b/config.c
>>>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>>>> strbuf_release(&env);
>>>> }
>>>>
>>>> +static void canonicalize_config_variable_name(struct strbuf *var)
>>>> +{
>>>> + char *first_dot = strchr(var->buf, '.');
>>>> + char *last_dot = strrchr(var->buf, '.');
>>>
>>> If first_dot != NULL, then last_dot !+ NULL as well.
>>> (either both are NULL or none of them),
>>> so we can loose one condition below.
>>
>> I do not think it is worth it, though.
>>
>>>> + char *cp;
>>>> +
>>>> + if (first_dot)
>>>> + for (cp = var->buf; *cp && cp < first_dot; cp++)
>>>> + *cp = tolower(*cp);
>>>> + if (last_dot)
>>>> + for (cp = last_dot; *cp; cp++)
>>>> + *cp = tolower(*cp);
>>
>> if (first_dot) {
>> scan up to first dot
>> if (last_dot)
>
> just leave out the 'if (last_dot)' ?
>
> if (first_dot) {
> /* also implies last_dot */
> do 0 -> first
> do last -> end
> }
^ permalink raw reply
* [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-15 23:48 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Lars Schneider, git, sbeller
In-Reply-To: <xmqqtw7v123n.fsf@gitster.mtv.corp.google.com>
The "git -c <var>=<val> cmd" mechanism is to pretend that a
configuration variable <var> is set to <val> while the cmd is
running. The code to do so however downcased <var> in its entirety,
which is wrong for a three-level <section>.<subsection>.<variable>.
The <subsection> part needs to stay as-is.
Reported-by: Lars Schneider <larsxschneider@gmail.com>
Diagnosed-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
config.c | 22 +++++++++++++++++++++-
1 file changed, 21 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 0dfed682b8..e9b93b5304 100644
--- a/config.c
+++ b/config.c
@@ -199,6 +199,26 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
+/*
+ * downcase the <section> and <variable> in <section>.<variable> or
+ * <section>.<subsection>.<variable> and do so in place. <subsection>
+ * is left intact.
+ */
+static void canonicalize_config_variable_name(char *varname)
+{
+ char *dot, *cp;
+
+ dot = strchr(varname, '.');
+ if (dot)
+ for (cp = varname; cp < dot; cp++)
+ *cp = tolower(*cp);
+ dot = strrchr(varname, '.');
+ if (dot)
+ for (cp = dot + 1; *cp; cp++)
+ *cp = tolower(*cp);
+}
+
+
int git_config_parse_parameter(const char *text,
config_fn_t fn, void *data)
{
@@ -221,7 +241,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
- strbuf_tolower(pair[0]);
+ canonicalize_config_variable_name(pair[0]->buf);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
--
2.12.0-rc1-258-g3d3d1e383b
^ permalink raw reply related
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Stefan Beller @ 2017-02-15 23:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, Lars Schneider, git@vger.kernel.org
In-Reply-To: <xmqqh93v10vy.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 3:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> Yes; though I'd place it in strbuf.{c,h} as it is operating
>> on the internals of the strbuf. (Do we make any promises outside of
>> strbuf about the internals? I mean we use .buf all the time, so maybe
>> I am overly cautious here)
>
> I'd rather have it not use struct strbuf as an interface. It only
> needs to pass "char *" and its promise that it touches the string
> in-place without changing the length need to be documented as a
> comment before the function.
>
>>> config.c | 16 +++++++++++++++-
>>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/config.c b/config.c
>>> index c6b874a7bf..98bf8fee32 100644
>>> --- a/config.c
>>> +++ b/config.c
>>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>>> strbuf_release(&env);
>>> }
>>>
>>> +static void canonicalize_config_variable_name(struct strbuf *var)
>>> +{
>>> + char *first_dot = strchr(var->buf, '.');
>>> + char *last_dot = strrchr(var->buf, '.');
>>
>> If first_dot != NULL, then last_dot !+ NULL as well.
>> (either both are NULL or none of them),
>> so we can loose one condition below.
>
> I do not think it is worth it, though.
>
>>> + char *cp;
>>> +
>>> + if (first_dot)
>>> + for (cp = var->buf; *cp && cp < first_dot; cp++)
>>> + *cp = tolower(*cp);
>>> + if (last_dot)
>>> + for (cp = last_dot; *cp; cp++)
>>> + *cp = tolower(*cp);
>
> if (first_dot) {
> scan up to first dot
> if (last_dot)
just leave out the 'if (last_dot)' ?
if (first_dot) {
/* also implies last_dot */
do 0 -> first
do last -> end
}
^ permalink raw reply
* Re: how are "untracked working tree files" even possible in this case?
From: Stefan Beller @ 2017-02-15 23:40 UTC (permalink / raw)
To: G. Sylvie Davies; +Cc: Git Users
In-Reply-To: <CAAj3zPx6uP5WbA68Co0yX_yh-e5C+jze2T1hJ0NYS7hHBzgdqg@mail.gmail.com>
On Wed, Feb 15, 2017 at 12:36 PM, G. Sylvie Davies
<sylvie@bit-booster.com> wrote:
> Hi,
>
> I have a script that runs the following sequence of commands within a clone:
>
> -----
> /usr/bin/git rebase --abort (took 148ms)
> /usr/bin/git cherry-pick --abort (took 103ms)
Is there more happening before?
> /usr/bin/git clean -d -f -x (took 2007ms)
> /usr/bin/git reset --hard --quiet
I think the order is important:
git add <file> # add to the index
git clean -dfx # <file> is not untracked, hence not removed
git reset --hard # <file> is untracked now?
So I would expect reset before running clean would fix the problem?
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Junio C Hamano @ 2017-02-15 23:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: Jonathan Tan, Lars Schneider, git@vger.kernel.org
In-Reply-To: <CAGZ79kaKhjNPGRVJ6H=CMKQ1RKXmVvSPOMo4c3haNeS60aWQXA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> Yes; though I'd place it in strbuf.{c,h} as it is operating
> on the internals of the strbuf. (Do we make any promises outside of
> strbuf about the internals? I mean we use .buf all the time, so maybe
> I am overly cautious here)
I'd rather have it not use struct strbuf as an interface. It only
needs to pass "char *" and its promise that it touches the string
in-place without changing the length need to be documented as a
comment before the function.
>> config.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/config.c b/config.c
>> index c6b874a7bf..98bf8fee32 100644
>> --- a/config.c
>> +++ b/config.c
>> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
>> strbuf_release(&env);
>> }
>>
>> +static void canonicalize_config_variable_name(struct strbuf *var)
>> +{
>> + char *first_dot = strchr(var->buf, '.');
>> + char *last_dot = strrchr(var->buf, '.');
>
> If first_dot != NULL, then last_dot !+ NULL as well.
> (either both are NULL or none of them),
> so we can loose one condition below.
I do not think it is worth it, though.
>> + char *cp;
>> +
>> + if (first_dot)
>> + for (cp = var->buf; *cp && cp < first_dot; cp++)
>> + *cp = tolower(*cp);
>> + if (last_dot)
>> + for (cp = last_dot; *cp; cp++)
>> + *cp = tolower(*cp);
if (first_dot) {
scan up to first dot
if (last_dot)
scan from last dot to the end
}
would be uglier.
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-15 23:34 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <xmqqbmu32iyb.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> FWIW I wish it were different, that git.git's `master` reflected more
>> closely what the current Git for Windows version has.
>
> Well, we two wishing the same thing together without doing anything
> else would make it happen.
ehh, would *not* make it happen, of course.
> As an experiment to see if our process can be improved, I've been
> meaning to suggest (which is what was behind my "question at a bit
> higher level" to Hannes [*1*]) asking you to throw me occasional
> pull requests for changes that are only about Windows specific
> issues, bypassing "patches on the list" for things like a hotfix to
> js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
> essentially treating Windows specific changes as "a sub-maintainer
> makes pull requests" we already do with Paul, Eric and Pat.
While this may ease the flow of upstreaming windows specific
changes, we need a separate thing to address the on-going issue you
raised in your message. A Windows-less person would not know his
change to a generic code that is innocuous-looking has fallouts on
Windows (read this sentence with "Windows" replaced with any
specific platform name). When somebody writes c == '/' that should
have been written as is_dir_sep(c), you or Hannes often finds it
during the review here, and after repeatedly seeing such reviews,
that (slowly) rubs off on other Window-less folks. A new code may
still hit 'next' and 'master' with such an issue if it goes
unnoticed during the review.
The CI you are setting up [*1*] may certainly be a step in the good
direction. Having more people like Hannes working off of upstream
may also be a great way to help the "forget 'next' and upstream in
general" issue. Any other ideas?
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Philip Oakley @ 2017-02-15 23:33 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Johannes Schindelin, git-for-windows, git
In-Reply-To: <xmqqvasb2li8.fsf@gitster.mtv.corp.google.com>
[sorry for the repeated emails - I'd prepared it off line, and then suffered
a number of auto send actions]
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> In the next..pu case the abstraction is in the other direction, we
>> have potentially multiple points of infection (from feature branches),
>> and a broad test (the whole test suite). In this case I believe we
>> would like to investigate initially the --first-parent line with a
>> classic bisect for the first point of failure (obviously including
>> feature branch merges). This would identify which feature merge, or
>> regular commit, created the first breakage.
>
> If you are going first-parent, you would limit the bisection to a
> single-strand-of-pearls, and I agree that it is a good strategy to
> find which topic branch merge broke the tip of 'pu'.
>
> If we assume that there is no funny interaction among topics that
> cancel a breakage brought in by one topic with another breakage by
> another topic, then no matter how many broken topics there are, I
> agree that we would get to the first broken topic.
>
> A good thing that comes once we assume that topics are more-or-less
> independent is that we could rebuild 'pu' minus the broken topic
> identified by the above procedure and repeat it to find other broken
> topics, still using the --first-parent bisection, because master..pu
> is a linear sequence of merges of individual topics.
>
For an integrator, or especially a CI tool, simply checking the second
parents of each topic merge (post fail) should at least indicate if the
basics of the feature actually passed the tests, though it doesn't check for
interaction issues. This could give direct author feedback!
--
Philip
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Jonathan Tan @ 2017-02-15 23:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, git, sbeller
In-Reply-To: <xmqqtw7v123n.fsf@gitster.mtv.corp.google.com>
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.
> config.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index c6b874a7bf..98bf8fee32 100644
> --- a/config.c
> +++ b/config.c
> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
> strbuf_release(&env);
> }
>
> +static void canonicalize_config_variable_name(struct strbuf *var)
> +{
> + char *first_dot = strchr(var->buf, '.');
> + char *last_dot = strrchr(var->buf, '.');
> + char *cp;
> +
> + if (first_dot)
> + for (cp = var->buf; *cp && cp < first_dot; cp++)
"*cp &&" is unnecessary, as far as I can tell.
> + *cp = tolower(*cp);
> + if (last_dot)
> + for (cp = last_dot; *cp; cp++)
> + *cp = tolower(*cp);
> +}
> +
> int git_config_parse_parameter(const char *text,
> config_fn_t fn, void *data)
> {
> @@ -223,7 +237,7 @@ int git_config_parse_parameter(const char *text,
> strbuf_list_free(pair);
> return error("bogus config parameter: %s", text);
> }
> - strbuf_tolower(pair[0]);
> + canonicalize_config_variable_name(pair[0]);
> if (fn(pair[0]->buf, value, data) < 0) {
> strbuf_list_free(pair);
> return -1;
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Stefan Beller @ 2017-02-15 23:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, Lars Schneider, git@vger.kernel.org
In-Reply-To: <xmqqtw7v123n.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 3:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>> I had some time to look into this, and yes, command-line parameters
>>> are too aggressively downcased ("git_config_parse_parameter" calls
>>> "strbuf_tolower" on the entire key part in config.c).
>>
>> Ahh, thanks. So this is not about submodules at all; it is -c var=VAL
>> where var is downcased too aggressively.
>
> Perhaps something like this?
Yes; though I'd place it in strbuf.{c,h} as it is operating
on the internals of the strbuf. (Do we make any promises outside of
strbuf about the internals? I mean we use .buf all the time, so maybe
I am overly cautious here)
>
> config.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/config.c b/config.c
> index c6b874a7bf..98bf8fee32 100644
> --- a/config.c
> +++ b/config.c
> @@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
> strbuf_release(&env);
> }
>
> +static void canonicalize_config_variable_name(struct strbuf *var)
> +{
> + char *first_dot = strchr(var->buf, '.');
> + char *last_dot = strrchr(var->buf, '.');
If first_dot != NULL, then last_dot !+ NULL as well.
(either both are NULL or none of them),
so we can loose one condition below.
> + char *cp;
> +
> + if (first_dot)
> + for (cp = var->buf; *cp && cp < first_dot; cp++)
> + *cp = tolower(*cp);
> + if (last_dot)
> + for (cp = last_dot; *cp; cp++)
> + *cp = tolower(*cp);
> +}
> +
> int git_config_parse_parameter(const char *text,
> config_fn_t fn, void *data)
^ permalink raw reply
* Re: [PATCH 04/14] connect_work_tree_and_git_dir: safely create leading directories
From: Junio C Hamano @ 2017-02-15 23:19 UTC (permalink / raw)
To: Stefan Beller
Cc: git@vger.kernel.org, Brandon Williams, Jonathan Nieder,
brian m. carlson
In-Reply-To: <CAGZ79kaX1URNvzp8QuFEFnJDzSW9nE971+nvhPLOQQx4aSyBkA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
> On Wed, Feb 15, 2017 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> In a later patch we'll use connect_work_tree_and_git_dir when the
>>> directory for the gitlink file doesn't exist yet. Safely create
>>> the directory first.
>>>
>>> Signed-off-by: Stefan Beller <sbeller@google.com>
>>
>> Among the existing two callers, the "absorb" logic in submodule.c
>> has safe-create-leading-directory (SCLD) immediately before the call
>> to this function, which can now be lost with this change. The other
>> one in cmd_mv() has the target directory as the result of moving the
>> original directory, and I do not think there is any corresponding
>> call that can be lost from there after this change, but it is not an
>> error to call SCLD on a path that already exists, so all is OK.
>>
>> Is it sensible to let the code continue with just an fprintf() (not
>> even warning() or error()) when SCLD fails?
>
> I'll move the code from the absorbing here (i.e. lose the
> SCLD lines there) and make it a die(_(..)) instead of fprintf here.
OK. I didn't actually meant to suggest the former (I meant that I
expect that would happen in the future steps of this series, or it
can be left as a follow-up patch after the series settles); the
latter may be worth doing.
Thanks.
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Junio C Hamano @ 2017-02-15 23:11 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Lars Schneider, git, sbeller
In-Reply-To: <xmqqy3x712if.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> I had some time to look into this, and yes, command-line parameters
>> are too aggressively downcased ("git_config_parse_parameter" calls
>> "strbuf_tolower" on the entire key part in config.c).
>
> Ahh, thanks. So this is not about submodules at all; it is -c var=VAL
> where var is downcased too aggressively.
Perhaps something like this?
config.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index c6b874a7bf..98bf8fee32 100644
--- a/config.c
+++ b/config.c
@@ -201,6 +201,20 @@ void git_config_push_parameter(const char *text)
strbuf_release(&env);
}
+static void canonicalize_config_variable_name(struct strbuf *var)
+{
+ char *first_dot = strchr(var->buf, '.');
+ char *last_dot = strrchr(var->buf, '.');
+ char *cp;
+
+ if (first_dot)
+ for (cp = var->buf; *cp && cp < first_dot; cp++)
+ *cp = tolower(*cp);
+ if (last_dot)
+ for (cp = last_dot; *cp; cp++)
+ *cp = tolower(*cp);
+}
+
int git_config_parse_parameter(const char *text,
config_fn_t fn, void *data)
{
@@ -223,7 +237,7 @@ int git_config_parse_parameter(const char *text,
strbuf_list_free(pair);
return error("bogus config parameter: %s", text);
}
- strbuf_tolower(pair[0]);
+ canonicalize_config_variable_name(pair[0]);
if (fn(pair[0]->buf, value, data) < 0) {
strbuf_list_free(pair);
return -1;
^ permalink raw reply related
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Junio C Hamano @ 2017-02-15 23:02 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Lars Schneider, git, sbeller
In-Reply-To: <f238248f-0df2-19a5-581d-95c8a61b4632@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> I had some time to look into this, and yes, command-line parameters
> are too aggressively downcased ("git_config_parse_parameter" calls
> "strbuf_tolower" on the entire key part in config.c).
Ahh, thanks. So this is not about submodules at all; it is -c var=VAL
where var is downcased too aggressively.
^ permalink raw reply
* Re: [BUG] submodule config does not apply to upper case submodules?
From: Jonathan Tan @ 2017-02-15 22:54 UTC (permalink / raw)
To: Junio C Hamano, Lars Schneider; +Cc: git, sbeller
In-Reply-To: <xmqqzihn2smp.fsf@gitster.mtv.corp.google.com>
On 02/15/2017 10:53 AM, Junio C Hamano wrote:
> Lars Schneider <larsxschneider@gmail.com> writes:
>
>> It looks like as if submodule configs ("submodule.*") for submodules
>> with upper case names are ignored.
>
> This observation is surprising, as the second level in three-level
> names like "<section>.<name>.<variable>" is designed to be case
> sensitive. A code that uses the config API needs to do extra things
> to cause the behaviour you showed, i.e. to get submodule.U.update
> ignored while submodule.l.update to be honoured. Perhaps somebody
> downcases things too aggressively before comparing?
>
> This is worth making it work as expected, needless to say ;-)
I had some time to look into this, and yes, command-line parameters are
too aggressively downcased ("git_config_parse_parameter" calls
"strbuf_tolower" on the entire key part in config.c). Updating the
original patch to use "test_global_config" makes the test pass, and
commenting out the "strbuf_tolower" line in config.c also makes the test
pass.
I'll see if I can fix this.
^ permalink raw reply
* Re: [PATCH 04/14] connect_work_tree_and_git_dir: safely create leading directories
From: Stefan Beller @ 2017-02-15 22:52 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, Jonathan Nieder,
brian m. carlson
In-Reply-To: <xmqqh93v48mv.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 10:22 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> In a later patch we'll use connect_work_tree_and_git_dir when the
>> directory for the gitlink file doesn't exist yet. Safely create
>> the directory first.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> Among the existing two callers, the "absorb" logic in submodule.c
> has safe-create-leading-directory (SCLD) immediately before the call
> to this function, which can now be lost with this change. The other
> one in cmd_mv() has the target directory as the result of moving the
> original directory, and I do not think there is any corresponding
> call that can be lost from there after this change, but it is not an
> error to call SCLD on a path that already exists, so all is OK.
>
> Is it sensible to let the code continue with just an fprintf() (not
> even warning() or error()) when SCLD fails?
I'll move the code from the absorbing here (i.e. lose the
SCLD lines there) and make it a die(_(..)) instead of fprintf here.
Thanks,
Stefan
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Junio C Hamano @ 2017-02-15 22:50 UTC (permalink / raw)
To: Jeff King; +Cc: Jáchym Barvínek, git
In-Reply-To: <20170215223246.mkaz22yrovnscnne@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Good catch. I think we use a nasty bitwise-OR elsewhere to do that.
> Ah, here it is, in tempfile.c:
>
> /*
> * Note: no short-circuiting here; we want to fclose()
> * in any case!
> */
> err = ferror(fp) | fclose(fp);
>
> That works, but the fact that we need a comment is a good sign that it's
> kind of gross. It's too bad stdio does not specify the return of fclose
> to report an error in the close _or_ any previous error. I guess we
> could wrap it with our own function.
Sure. I am happy to add something like this:
/*
* closes a FILE *, returns 0 if closing and all the
* previous stdio operations on fp were successful,
* otherwise non-zero.
*/
int xfclose(FILE *fp)
{
return ferror(fp) | fclose(fp);
}
I do not think we should try to do anything fancier to allow the
caller to tell ferror() and fclose() apart, as such a caller would
then need to do
switch (xfclose(fp)) {
case 0: /* happy */ break;
case XFCLOSE_CLOSE: do "close failed" thing; break;
case XFCLOSE_ERROR: do "other things failed" thing; break;
}
and at that point, "other things failed" code would not have much to
work with to do more detailed diagnosis anyway (the errno is likely
not trustable), and it is not too much to write
if (ferror(fp))
do "saw some failure before" thing;
if (fclose(fp))
do "close failed" thing;
instead.
^ permalink raw reply
* Re: [PATCH v2 2/2] completion: checkout: complete paths when ref given
From: Cornelius Weig @ 2017-02-15 22:45 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Git mailing list, Richard Wagner
In-Reply-To: <CAM0VKjkUu2k73+PxZ2UNKrnBg0nW_za+10O7eHEgcko6BaGx6Q@mail.gmail.com>
On 02/15/2017 03:26 PM, SZEDER Gábor wrote:
> On Tue, Feb 14, 2017 at 10:24 PM, <cornelius.weig@tngtech.com> wrote:
>
>> + *)
>> + __git_complete_tree_file "$ref" "$cur"
>> + ;;
>
> There is one more caveat here.
>
> Both our __git_complete_index_file() and Bash's builtin filename
> completion lists matching paths like this:
>
> $ git rm contrib/co<TAB>
> coccinelle/ contacts/
> completion/ convert-grafts-to-replace-refs.sh
>
> i.e. the leading path components are not redundantly repeated.
>
> Now, with this patch in this code path the list would look like this:
>
> $ git checkout completion-refs-speedup contrib/co<TAB>
> contrib/coccinelle/
> contrib/completion/
> contrib/contacts/
> contrib/convert-grafts-to-replace-refs.sh
>
> See the difference?
Now that you say it.. I had never noticed it though.
> I once made a feeble attempt to make completion of the <ref>:<path>
> notation (i.e. what you extracted into __git_complete_tree_file())
> look like regular filename completion, but couldn't.
Can you dig up what you tried out? Maybe somebody comes up with a good idea.
^ permalink raw reply
* Re: [PATCH 03/14] make is_submodule_populated gently
From: Stefan Beller @ 2017-02-15 22:42 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, Jonathan Nieder,
brian m. carlson
In-Reply-To: <xmqqlgt7495z.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 10:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> We need the gentle version in a later patch. As we have just one caller,
>> migrate the caller.
>
> Ordinarily, we keep the original helper implemented as a thin
> wrapper that passes NULL as retun_error_code, which causes it to
> die() on error for existing callers. But because we only have one
> caller (and topics in-flight do not add new ones), we do not bother
> with that.
Right.
>
> The reasoning makes sense, at least to me.
>
> We may want to add a comment about the behaviour upon error for the
> helper function? I see resolve_gitdir_gently() does not do so and
> the readers have to follow the callflow down to read_gitfile_gently()
> which does have the comment, so perhaps we are OK without any.
>
> Looks good to me.
Will do in a reroll.
Thanks,
Stefan
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Jeff King @ 2017-02-15 22:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jáchym Barvínek, git
In-Reply-To: <xmqq7f4r2io5.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 02:28:10PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> abort:
> >> strbuf_release(¬e);
> >> free(url);
> >> - fclose(fp);
> >> + if (ferror(fp))
> >> + rc = -1;
> >> + if (fclose(fp))
> >> + rc = -1;
> >> return rc;
> >
> > Yeah, I think this works. Normally you'd want to flush before checking
> > ferror(), but since you detect errors from fclose, too, it should be
> > fine.
> >
> > We probably should write something stderr, though. Maybe:
> >
> > if (ferror(fp) || fclose(fp))
> > rc = error_errno("unable to write to %s", filename);
>
> Yes, and somehow make sure we do fclose(fp) even when we have an
> error already ;-)
Good catch. I think we use a nasty bitwise-OR elsewhere to do that.
Ah, here it is, in tempfile.c:
/*
* Note: no short-circuiting here; we want to fclose()
* in any case!
*/
err = ferror(fp) | fclose(fp);
That works, but the fact that we need a comment is a good sign that it's
kind of gross. It's too bad stdio does not specify the return of fclose
to report an error in the close _or_ any previous error. I guess we
could wrap it with our own function.
-Peff
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Junio C Hamano @ 2017-02-15 22:28 UTC (permalink / raw)
To: Jeff King; +Cc: Jáchym Barvínek, git
In-Reply-To: <20170215215151.a5chtxyjhbe3og4p@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> abort:
>> strbuf_release(¬e);
>> free(url);
>> - fclose(fp);
>> + if (ferror(fp))
>> + rc = -1;
>> + if (fclose(fp))
>> + rc = -1;
>> return rc;
>
> Yeah, I think this works. Normally you'd want to flush before checking
> ferror(), but since you detect errors from fclose, too, it should be
> fine.
>
> We probably should write something stderr, though. Maybe:
>
> if (ferror(fp) || fclose(fp))
> rc = error_errno("unable to write to %s", filename);
Yes, and somehow make sure we do fclose(fp) even when we have an
error already ;-)
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-15 22:22 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702151332540.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Junio,
> ...
> The hat of a person who sees how patches are reviewed before they enter
> pu/next/master/maint of git.git.
> ...
> make sense. This makes my life harder, but I believe that the alternative
> would be *not* to have those patches in git.git *at all*.
You wrote a lot, what you wrote were readable, and perhaps were good
material to support your answer/conclusion, but you forgot to answer
a simple question I asked. I think your description of where things
currently are would support any of the possible answers below, and I
cannot guess which one it would be.
The question was:
Is our position "unless you are extremely competent and are willing
to cherry-pick missing things from Git for Windows tree as needed,
we recommend you to build Git for Windows source instead"?
Or is it "please ignore upstream and work off of Git for Windows?"
Or is it "please try to work with upstream and if you find Windows
specific glitches, after checking to see if Git for Windows has
already a fix for it and otherwise feed your fix to Git for Windows,
so that we can upstream your fix for you, together with changes from
others"?
Or is it "please try to work with upstream and if you find Windows
specific glitches, after checking to see if Git for Windows has
already a fix for it and otherwise upstream your fix, so that next
pull from upstream into Git for Windows would have your fix"?
Or is it something else?
> FWIW I wish it were different, that git.git's `master` reflected more
> closely what the current Git for Windows version has.
Well, we two wishing the same thing together without doing anything
else would make it happen.
As an experiment to see if our process can be improved, I've been
meaning to suggest (which is what was behind my "question at a bit
higher level" to Hannes [*1*]) asking you to throw me occasional
pull requests for changes that are only about Windows specific
issues, bypassing "patches on the list" for things like a hotfix to
js/mingw-isatty [*2*] and use of OpenSSL SHA-1 on Windows [*3*],
essentially treating Windows specific changes as "a sub-maintainer
makes pull requests" we already do with Paul, Eric and Pat.
Interested parties would instead see only the pull request sent by
you to me, either on the list or directly CC'ed to them, and do
their own fetch to assess if it is a good idea for me to actually
pull. Suggestions to the changes from bystanders like Peff's
comment [*4*] may need to reproduce the patch text when sent to the
list, which would burden reviewers a bit more, but they still are
possible.
Such a pull-request workflow would either hide the communication
from the list or force people to go to multiple places (i.e. both to
the list and to GitHub comments) in order to view the whole picture,
and I am still reluctant to extend it to other areas (e.g. a change
that adds "#if WINDOWS" to a more generic codepath), but a trial on
a limited scope may give us a better feel of how well such an
updated workflow would work for us.
[References]
*1* <xmqq60kdev2r.fsf@gitster.mtv.corp.google.com>
*2* <c88612da0a62bfcbc3e278296f9d3eb010057071.1487025228.git.johannes.schindelin@gmx.de>
*3* <6a29f8c60d315a24292c1fa9f5e84df4dfdbf813.1486679254.git.johannes.schindelin@gmx.de>
*4* <20170210050237.gajicliueuvk6s5d@sigill.intra.peff.net>
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Philip Oakley @ 2017-02-15 22:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Johannes Schindelin, git-for-windows, git
In-Reply-To: <xmqqshng5osz.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> There are also a few ideas at the SO answers:
>> http://stackoverflow.com/a/5652323/717355
>
> I vaguely recall that I saw somebody said the same "mark tips of
> topics as good" on the list and answered with why it does not quite
> work, though.
>
I think you may mean
https://public-inbox.org/git/7v8vyam5la.fsf@alter.siamese.dyndns.org/
I think we are thinking of opposite abstractions.
For regular bisect, the assumption (to a first order) is that there is a
single point of infection of a single persistent bug with a well defined
test, and that the goal is to find the point of first infection, as all
other incidents of the bug are in successor commits, which are all infected.
The fail-fix-break again sequence you mentioned in that thread is to my mind
a red herring as it contradicts the normal bisection assumptions (but see
below).
In the next..pu case the abstraction is in the other direction, we have
potentially multiple points of infection (from feature branches), and a
broad test (the whole test suite). In this case I believe we would like to
investigate initially the --first-parent line with a classic bisect for the
first point of failure (obviously including feature branch merges). This
would identify which feature merge, or regular commit, created the first
breakage.
Once the first point of failure has been identified, for the next..pu case,
each of the post-fail second parents of merge commits _could_ then also be
checked (which is a linear search, not a bisection), to identify any
additional feature branches that need attention. This second stage search
would probably be an option, but if the merging sequence onto pu is
generally from good to bad, then the search is likely to be short. At least
for a CI system this 2nd stage could provide useful feedback to the authors
of their mistakes...
I haven't looked back at the actual patches in that thread, so they may not
have followed my expectation of the --multi-bug (TM) search algorithm.
--
Philip
^ permalink raw reply
* Re: [git-for-windows] Re: Continuous Testing of Git on Windows
From: Philip Oakley @ 2017-02-15 22:19 UTC (permalink / raw)
To: Junio C Hamano
Cc: Christian Couder, Johannes Schindelin, git-for-windows, git
In-Reply-To: <xmqqshng5osz.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> There are also a few ideas at the SO answers:
>> http://stackoverflow.com/a/5652323/717355
>
> I vaguely recall that I saw somebody said the same "mark tips of
> topics as good" on the list and answered with why it does not quite
> work, though.
>
I think you may mean
https://public-inbox.org/git/7v8vyam5la.fsf@alter.siamese.dyndns.org/
I think we are thinking of opposite abstractions.
For regular bisect, the assumption (to a first order) is that there is a
single point of infection of a single persistent bug with a well defined
test, and that the goal is to find the point of first infection, as all
other incidents of the bug are in successor commits, which are all infected.
The fail-fix-break again sequence you mentioned in that thread is to my mind
a red herring as it contradicts the normal bisection assumptions (but see
below).
In the next..pu case the abstraction is in the other direction, we have
potentially multiple points of infection (from feature branches), and a
broad test (the whole test suite). In this case I believe we would like to
investigate initially the --first-parent line with a classic bisect for the
first point of failure (obviously including feature branch merges). This
would identify which feature merge, or regular commit, created the first
breakage.
Once the first point of failure has been identified, for the next..pu case,
each of the post-fail second parents of merge commits _could_ then also be
checked (which is a linear search, not a bisection), to identify any
additional feature branches that need attention. This second stage search
would probably be an option, but if the merging sequence onto pu is
generally from good to bad, then the search is likely to be short. At least
for a CI system this 2nd stage could provide useful feedback to the authors
of their mistakes...
I haven't looked back at the actual patches in that thread, so they may not
have followed my expectation of the --multi-bug (TM) search algorithm.
--
Philip
^ 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