* 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
* Re: Back quote typo in error messages (?)
From: Jeff King @ 2017-02-15 21:56 UTC (permalink / raw)
To: Fabrizio Cucci; +Cc: git
In-Reply-To: <CAOxYW4xqk4j6Uu86jq2Vi9Bpgihxfr2Tw-DQLc+7YTZiPmDtiA@mail.gmail.com>
On Wed, Feb 15, 2017 at 09:51:30PM +0000, Fabrizio Cucci wrote:
> > Some people use the matched backtick/single-quote to emulate the
> > non-symmetric start/end quotes used in traditional typography (and in
> > fact, ``foo'' in languages like asciidoc are typically rendered using
> > smart-quotes).
>
> I definitely didn't know about the use of them in traditional typography!
> But I couldn't find any example of non-symmetric quotes in AsciiDoc...
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.
> > I don't know how much we care about standardizing that punctuation.
>
> I mentioned it was very minor but, still, in my opinion a project like
> Git deserves consistent punctuation! :)
I think patches would be welcome, but as Junio said, it probably should
wait for the next cycle.
-Peff
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Jeff King @ 2017-02-15 21:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Christian Couder, git, Maxim Kuvyrkov, Pranit Bauva
In-Reply-To: <xmqqfujf2kfk.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 01:50:07PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I see the patches are marked for 'next' in the latest What's Cooking.
> > If it is not too late in today's integration cycle, here is a re-roll of
> > patch 3 that squashes in Pranit's suggestion (if it is too late, then
> > Pranit, you may want to re-send it as a squash on top).
>
> Thanks.
>
> I think that matches what I queued last night, except for the
> Helped-by: line. Will replace.
Oh, indeed. I should have actually checked what you queued. Thanks.
-Peff
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Jeff King @ 2017-02-15 21:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jáchym Barvínek, git
In-Reply-To: <xmqqk28r2kk4.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 01:47:23PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:
> > If FETCH_HEAD failed to write because of a full disk (or any other
> > reason), then the right thing is for "git fetch" to write an error to
> > stderr, and git-pull should not continue the operation at all.
> >
> > If we're not doing that, then that is certainly a bug.
>
> One suspect would be store_updated_refs(). We do catch failure from
> fopen("a") of FETCH_HEAD (it is truncated earlier in the code when
> the --append option is not given), but all the writes go through
> stdio without error checking.
>
> I wonder if this lazy patch is sufficient. I want to avoid having
> to sprinkle
>
> if (fputs("\\n", fp))
> error(...);
>
> all over the code.
Heh, I was just tracking down the exact same spot.
I think that yes, the lazy check-error-flag-at-the-end approach is fine
for stdio.
I tried to reproduce the original problem on a full loopback filesystem,
but got:
fatal: update_ref failed for ref 'ORIG_HEAD': could not write to '.git/ORIG_HEAD'
I suspect you'd need the _exact_ right amount of free space to get all
of the predecessor steps done, and then run out of space right when
trying to flush the FETCH_HEAD contents.
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index b5ad09d046..72347f0054 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
> 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);
-Peff
^ permalink raw reply
* Re: Back quote typo in error messages (?)
From: Fabrizio Cucci @ 2017-02-15 21:51 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170215212157.qgscyglgzrd5cplf@sigill.intra.peff.net>
On 15 February 2017 at 21:21, Jeff King <peff@peff.net> wrote:
> On Wed, Feb 15, 2017 at 09:06:46PM +0000, Fabrizio Cucci wrote:
>> Shouldn't the wrong flag be surrounded by two single quotes instead of
>> a back quote and a single quote?
>
> Some people use the matched backtick/single-quote to emulate the
> non-symmetric start/end quotes used in traditional typography (and in
> fact, ``foo'' in languages like asciidoc are typically rendered using
> smart-quotes).
I definitely didn't know about the use of them in traditional typography!
But I couldn't find any example of non-symmetric quotes in AsciiDoc...
> So I think what you are seeing is not wrong in the sense of being
> unintended by the author of the message.
I had the opposite impression from the quick search in the GitHub
repo, this is why I wrote here looking for some confirmation.
> I don't know how much we care about standardizing that punctuation.
I mentioned it was very minor but, still, in my opinion a project like
Git deserves consistent punctuation! :)
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Junio C Hamano @ 2017-02-15 21:50 UTC (permalink / raw)
To: Jeff King; +Cc: Christian Couder, git, Maxim Kuvyrkov, Pranit Bauva
In-Reply-To: <20170215214052.5py4pxkcz4g2bmtk@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I see the patches are marked for 'next' in the latest What's Cooking.
> If it is not too late in today's integration cycle, here is a re-roll of
> patch 3 that squashes in Pranit's suggestion (if it is too late, then
> Pranit, you may want to re-send it as a squash on top).
Thanks.
I think that matches what I queued last night, except for the
Helped-by: line. Will replace.
> -- >8 --
> Subject: [PATCH] show-branch: use skip_prefix to drop magic numbers
>
> We make several starts_with() calls, only to advance
> pointers. This is exactly what skip_prefix() is for, which
> lets us avoid manually-counted magic numbers.
>
> Helped-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> builtin/show-branch.c | 19 ++++++++-----------
> 1 file changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 404c4d09a..19756595d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
> pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
> pretty_str = pretty.buf;
> }
> - if (starts_with(pretty_str, "[PATCH] "))
> - pretty_str += 8;
> + skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
>
> if (!no_name) {
> if (name && name->head_name) {
> @@ -470,17 +469,14 @@ static void snarf_refs(int head, int remotes)
> }
> }
>
> -static int rev_is_head(char *head, char *name,
> +static int rev_is_head(const char *head, const char *name,
> unsigned char *head_sha1, unsigned char *sha1)
> {
> if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
> return 0;
> - if (starts_with(head, "refs/heads/"))
> - head += 11;
> - if (starts_with(name, "refs/heads/"))
> - name += 11;
> - else if (starts_with(name, "heads/"))
> - name += 6;
> + skip_prefix(head, "refs/heads/", &head);
> + if (!skip_prefix(name, "refs/heads/", &name))
> + skip_prefix(name, "heads/", &name);
> return !strcmp(head, name);
> }
>
> @@ -799,8 +795,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
> has_head++;
> }
> if (!has_head) {
> - int offset = starts_with(head, "refs/heads/") ? 11 : 0;
> - append_one_rev(head + offset);
> + const char *name = head;
> + skip_prefix(name, "refs/heads/", &name);
> + append_one_rev(name);
> }
> }
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Junio C Hamano @ 2017-02-15 21:47 UTC (permalink / raw)
To: Jeff King; +Cc: Jáchym Barvínek, git
In-Reply-To: <20170215213221.lnraiktneokpk3mg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Sun, Feb 12, 2017 at 05:37:30PM +0100, Jáchym Barvínek wrote:
> If FETCH_HEAD failed to write because of a full disk (or any other
> reason), then the right thing is for "git fetch" to write an error to
> stderr, and git-pull should not continue the operation at all.
>
> If we're not doing that, then that is certainly a bug.
One suspect would be store_updated_refs(). We do catch failure from
fopen("a") of FETCH_HEAD (it is truncated earlier in the code when
the --append option is not given), but all the writes go through
stdio without error checking.
I wonder if this lazy patch is sufficient. I want to avoid having
to sprinkle
if (fputs("\\n", fp))
error(...);
all over the code.
builtin/fetch.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b5ad09d046..72347f0054 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -868,7 +868,10 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
abort:
strbuf_release(¬e);
free(url);
- fclose(fp);
+ if (ferror(fp))
+ rc = -1;
+ if (fclose(fp))
+ rc = -1;
return rc;
}
^ permalink raw reply related
* Re: [PATCH] show-branch: fix crash with long ref name
From: Jeff King @ 2017-02-15 21:40 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, git, Maxim Kuvyrkov, Pranit Bauva
In-Reply-To: <CAP8UFD0EfUgfmTB4dj-A+rw79F7SWKxYvatNfR+Nj-8ukWYAQA@mail.gmail.com>
On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:
> > I notice Christian's patch added a few tests. I don't know if we'd want
> > to squash them in (I didn't mean to override his patch at all; I was
> > about to send mine out when I noticed his, and I wondered if we wanted
> > to combine the two efforts).
>
> I think it would be nice to have at least one test. Feel free to
> squash mine if you want.
I started to add some tests, but I had second thoughts. It _is_ nice
to show off the fix, but as far as regressions go, this specific case is
unlikely to come up again. What would be more valuable, I think, is a
test script which set up a very long refname (not just 150 bytes or
whatever) and ran it through a series of git commands.
But then you run into all sorts of portability annoyances with pathname
restrictions (you can hack around creation by writing the refname
directly into packed-refs, but most manipulations will want to take the
.lock in the filesystem). So I dunno. It seems like being thorough is a
lot of hassle for not much gain. Being not-thorough is easy, but is
mostly a token that is unlikely to find any real bugs.
So I punted, at least for now.
I see the patches are marked for 'next' in the latest What's Cooking.
If it is not too late in today's integration cycle, here is a re-roll of
patch 3 that squashes in Pranit's suggestion (if it is too late, then
Pranit, you may want to re-send it as a squash on top).
-- >8 --
Subject: [PATCH] show-branch: use skip_prefix to drop magic numbers
We make several starts_with() calls, only to advance
pointers. This is exactly what skip_prefix() is for, which
lets us avoid manually-counted magic numbers.
Helped-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/show-branch.c | 19 ++++++++-----------
1 file changed, 8 insertions(+), 11 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 404c4d09a..19756595d 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -275,8 +275,7 @@ static void show_one_commit(struct commit *commit, int no_name)
pp_commit_easy(CMIT_FMT_ONELINE, commit, &pretty);
pretty_str = pretty.buf;
}
- if (starts_with(pretty_str, "[PATCH] "))
- pretty_str += 8;
+ skip_prefix(pretty_str, "[PATCH] ", &pretty_str);
if (!no_name) {
if (name && name->head_name) {
@@ -470,17 +469,14 @@ static void snarf_refs(int head, int remotes)
}
}
-static int rev_is_head(char *head, char *name,
+static int rev_is_head(const char *head, const char *name,
unsigned char *head_sha1, unsigned char *sha1)
{
if (!head || (head_sha1 && sha1 && hashcmp(head_sha1, sha1)))
return 0;
- if (starts_with(head, "refs/heads/"))
- head += 11;
- if (starts_with(name, "refs/heads/"))
- name += 11;
- else if (starts_with(name, "heads/"))
- name += 6;
+ skip_prefix(head, "refs/heads/", &head);
+ if (!skip_prefix(name, "refs/heads/", &name))
+ skip_prefix(name, "heads/", &name);
return !strcmp(head, name);
}
@@ -799,8 +795,9 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
has_head++;
}
if (!has_head) {
- int offset = starts_with(head, "refs/heads/") ? 11 : 0;
- append_one_rev(head + offset);
+ const char *name = head;
+ skip_prefix(name, "refs/heads/", &name);
+ append_one_rev(name);
}
}
--
2.12.0.rc1.541.g3e32dea89
^ 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