* Re: [PATCH] t6026: ensure that long-running script really is
From: Johannes Sixt @ 2016-11-11 6:50 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: git, Jeff King, Andreas Schwab
In-Reply-To: <xmqqfumy51tk.fsf@gitster.mtv.corp.google.com>
Am 11.11.2016 um 00:53 schrieb Junio C Hamano:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> When making sure that background tasks are cleaned up in 5babb5b
>> (t6026-merge-attr: clean up background process at end of test case,
>> 2016-09-07), we considered to let the background task sleep longer, just
>> to be certain that it will still be running when we want to kill it
>> after the test.
>>
>> Sadly, the assumption appears not to hold true that the test case passes
>> quickly enough to kill the background task within a second.
>>
>> Simply increase it to an hour. No system can be possibly slow enough to
>> make above-mentioned assumption incorrect.
>>
>> Reported by Andreas Schwab.
>>
>> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>> ---
>> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1
>> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1
>
> OK, I think this is a much better option. Assuming 3600 is long
> enough for everybody (and if not, we have a bigger problem ;-),
> it will ensure that the stray process will be around when we run the
> 'git merge' test, and by not adding "|| :" after the kill, we check
> that the stray process is still there, i.e. we tested what we wanted
> to test.
>
> Will revert the two patches that were queued previously and then
> queue this one.
Shouldn't we then move the 'kill' out of test_when_finished and make
it a proper condition of the test? Like this?
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 348d78b205..6ad8bd098a 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -187,13 +187,19 @@ test_expect_success 'custom merge does not lock index' '
sleep 3600 &
echo $! >sleep.pid
EOF
- test_when_finished "kill \$(cat sleep.pid)" &&
test_write_lines >.gitattributes \
"* merge=ours" "text merge=sleep-an-hour" &&
test_config merge.ours.driver true &&
test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
- git merge master
+ git merge master &&
+
+ # We are testing that the custom merge driver does not block
+ # index.lock on Windows due to an inherited file handle.
+ # To ensure that this test checks this condition, the process
+ # must still be running at this point (and must have started
+ # in the first place), hence, this kill must not fail:
+ kill "$(cat sleep.pid)"
'
test_done
^ permalink raw reply related
* Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length
From: Jacob Keller @ 2016-11-11 5:29 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <CAOLa=ZRDNGGc-OA+d+T1=9JvoVe2eBZt0DYQEHmeNKhDpxQbdg@mail.gmail.com>
On Thu, Nov 10, 2016 at 9:36 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Nov 9, 2016 at 4:57 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>> From: Karthik Nayak <karthik.188@gmail.com>
>>>
>>> Add support for %(objectname:short=<length>) which would print the
>>> abbreviated unique objectname of given length. When no length is
>>> specified, the length is 'DEFAULT_ABBREV'. The minimum length is
>>> 'MINIMUM_ABBREV'. The length may be exceeded to ensure that the provided
>>> object name is unique.
>>>
>>
>> Ok this makes sense. It may be annoying that the length might go
>> beyond the size that we wanted, but I think it's better than printing
>> a non-unique short abbreviation.
>>
>> I have one suggested change, which is to drop O_LENGTH and have
>> O_SHORT store the length always, setting it to DEFAULT_ABBREV when no
>> length provided. This allows you to drop some code. I don't think it's
>> actually worth a re-roll by itself since the current code is correct.
>>
>> Thanks,
>> Jake
>>
>
> That does make sense, It would also not error out when we use
> %(objectname:short=) and
> not specify the length. Idk, if that's desirable or not. But it does
> make the code a little more
> confusing to read at the same time.
>
I am not sure that would be the case. If you see "objectname:short"
you trreat this as if they had passed "objectname:short=<default
abbrev>" but if you see "objectname:short=" you die, no?
> So since its a small change, I'd be okay going either ways with this.
>
> --
> Regards,
> Karthik Nayak
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Jacob Keller @ 2016-11-11 5:27 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Git mailing list
In-Reply-To: <CAOLa=ZQCP2KtgLQie5p+PBiFjiNVFduUh=AmuLWcOKX5QA_BXA@mail.gmail.com>
On Thu, Nov 10, 2016 at 9:31 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
> On Wed, Nov 9, 2016 at 4:52 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Tue, Nov 8, 2016 at 12:11 PM, Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Ok. How does this handle whitespace? The previous if implementation
>> treated whitespace as trimming to ignore. Does this require an exact
>> whitespace match? It appears by the code that strings must match
>> exactly. Would it make more sense to always trim the value of
>> whitespace first before comparison? Hmm.. I think we should avoid
>> doing that actually.
>>
>
> This does not trim whitespace what so ever and requires an exact match.
> I don't see the reason behind trimming whitespace though.
>
The comment was made in reference to the fact that %(if)<atom>%(then)
essentially trims whitespace, since if the <atom> prints only
whitespace it does not match.
Thus, it might make sense to keep it all the same and have this trim
as well. However, I think there is no benefit or reasoning to do so in
this case.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Jacob Keller @ 2016-11-11 5:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Karthik Nayak, Git mailing list
In-Reply-To: <xmqqoa1m5335.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 3:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>> @@ -49,6 +51,10 @@ static struct used_atom {
>>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>>> unsigned int nlines;
>>> } contents;
>>> + struct {
>>> + const char *if_equals,
>>> + *not_equals;
>>
>>
>> Same here, why do we need both strings here stored separately? Could
>> we instead store which state to check and store the string once? I'm
>> not sure that really buys us any storage.
>
> I am not sure if storage is an issue, but I tend to agree that it
> would be semantically cleaner if this was done as a pair of <what
> operation uses this string constant?, the string constant>, and the
> former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
>
> You could later enhance the comparison operator more easily with
> such an arrangement (e.g. if-equals-case-insensitively).
The main advantage I see is that it ensures in the API that there is
no way for it to be both "equal" and "not equal" at the same time,
where as two separate pointers while not actually done in practice
could both be set somehow, leading to potential questions.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] fetch: do not redundantly calculate tag refmap
From: Jeff King @ 2016-11-11 2:09 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <1478823208-10647-1-git-send-email-jonathantanmy@google.com>
On Thu, Nov 10, 2016 at 04:13:28PM -0800, Jonathan Tan wrote:
> builtin/fetch.c redundantly calculates refmaps for tags twice. Remove
> the first calculation.
>
> This is only a code simplification and slight performance improvement -
> the result is unchanged, as the redundant refmaps are subsequently
> removed by the invocation to "ref_remove_duplicates" anyway.
>
> This was introduced in commit c5a84e9 ("fetch --tags: fetch tags *in
> addition to* other stuff", 2013-10-29) when modifying the effect of the
> --tags parameter to "git fetch". The refmap-for-tag calculation was
> copied instead of moved.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>
> (I noticed this when working on something in this file.)
Coincidentally I noticed this a few weeks ago, too, while working on
[1], but didn't follow it up. Mainly I was worried there was something
hidden or clever going on, but I think it really is just a case of the
code growing the two calls over time. So this looks good to me. Thanks
for digging it up.
-Peff
[1] http://public-inbox.org/git/20161024132932.i42rqn2vlpocqmkq@sigill.intra.peff.net/
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-11 0:26 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqoa1n57u4.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> I'll report back an updated schedule when able.
>
> I pushed some updates out on 'master' today. Between 'master' and
> 'pu' on the first-parent history there is a merge 52975d2b1f ("Merge
> branch 'ls/macos-update' into jch", 2016-11-10) and that matches
> what I expect to be in -rc1 tomorrow (modulo RelNotes and the actual
> version tag), unless there is a showstopper regresion reported, in
> which case we may want to first look into reverting the whole series
> that introduced the regression before considering to pile on fix-up
> patches.
Please make that 71d1bcb661 ("Merge branch 'ls/macos-update' into
jch", 2016-11-10); among the three extra topics that is not yet in
'master', as/merge-attr-sleep topic has been updated with a better
fix from Dscho.
Thanks.
^ permalink raw reply
* [PATCH] fetch: do not redundantly calculate tag refmap
From: Jonathan Tan @ 2016-11-11 0:13 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan
builtin/fetch.c redundantly calculates refmaps for tags twice. Remove
the first calculation.
This is only a code simplification and slight performance improvement -
the result is unchanged, as the redundant refmaps are subsequently
removed by the invocation to "ref_remove_duplicates" anyway.
This was introduced in commit c5a84e9 ("fetch --tags: fetch tags *in
addition to* other stuff", 2013-10-29) when modifying the effect of the
--tags parameter to "git fetch". The refmap-for-tag calculation was
copied instead of moved.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
(I noticed this when working on something in this file.)
builtin/fetch.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b6a5597..1d77e58 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -359,9 +359,6 @@ static struct ref *get_ref_map(struct transport *transport,
for (i = 0; i < fetch_refspec_nr; i++)
get_fetch_map(ref_map, &fetch_refspec[i], &oref_tail, 1);
-
- if (tags == TAGS_SET)
- get_fetch_map(remote_refs, tag_refspec, &tail, 0);
} else if (refmap_array) {
die("--refmap option is only meaningful with command-line refspec(s).");
} else {
--
2.8.0.rc3.226.g39d4020
^ permalink raw reply related
* Re: [PATCH] t6026: ensure that long-running script really is
From: Junio C Hamano @ 2016-11-10 23:53 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Johannes Sixt, Jeff King, Andreas Schwab
In-Reply-To: <16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de>
Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> When making sure that background tasks are cleaned up in 5babb5b
> (t6026-merge-attr: clean up background process at end of test case,
> 2016-09-07), we considered to let the background task sleep longer, just
> to be certain that it will still be running when we want to kill it
> after the test.
>
> Sadly, the assumption appears not to hold true that the test case passes
> quickly enough to kill the background task within a second.
>
> Simply increase it to an hour. No system can be possibly slow enough to
> make above-mentioned assumption incorrect.
>
> Reported by Andreas Schwab.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> Published-As: https://github.com/dscho/git/releases/tag/t6026-sleep-v1
> Fetch-It-Via: git fetch https://github.com/dscho/git t6026-sleep-v1
OK, I think this is a much better option. Assuming 3600 is long
enough for everybody (and if not, we have a bigger problem ;-),
it will ensure that the stray process will be around when we run the
'git merge' test, and by not adding "|| :" after the kill, we check
that the stray process is still there, i.e. we tested what we wanted
to test.
Will revert the two patches that were queued previously and then
queue this one.
Thanks.
>
> t/t6026-merge-attr.sh | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 7a6e33e..348d78b 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -183,16 +183,16 @@ test_expect_success 'up-to-date merge without common ancestor' '
>
> test_expect_success 'custom merge does not lock index' '
> git reset --hard anchor &&
> - write_script sleep-one-second.sh <<-\EOF &&
> - sleep 1 &
> + write_script sleep-an-hour.sh <<-\EOF &&
> + sleep 3600 &
> echo $! >sleep.pid
> EOF
> test_when_finished "kill \$(cat sleep.pid)" &&
>
> test_write_lines >.gitattributes \
> - "* merge=ours" "text merge=sleep-one-second" &&
> + "* merge=ours" "text merge=sleep-an-hour" &&
> test_config merge.ours.driver true &&
> - test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
> + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
> git merge master
> '
>
>
> base-commit: be5a750939c212bc0781ffa04fabcfd2b2bd744e
^ permalink raw reply
* Re: [PATCH v7 04/17] ref-filter: modify "%(objectname:short)" to take length
From: Junio C Hamano @ 2016-11-10 23:32 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, jacob.keller
In-Reply-To: <20161108201211.25213-5-Karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> else if (!strcmp(arg, "short"))
> - atom->u.objectname = O_SHORT;
> - else
> + atom->u.objectname.option = O_SHORT;
> + else if (skip_prefix(arg, "short=", &arg)) {
> + atom->u.objectname.option = O_LENGTH;
> + if (strtoul_ui(arg, 10, &atom->u.objectname.length) ||
> + atom->u.objectname.length == 0)
> + die(_("positive value expected objectname:short=%s"), arg);
> + if (atom->u.objectname.length < MINIMUM_ABBREV)
> + atom->u.objectname.length = MINIMUM_ABBREV;
> + } else
> die(_("unrecognized %%(objectname) argument: %s"), arg);
> }
Users who want to use the default-abbrev, i.e. the autoscaling one
introduced recently, must use "short", not "short=-1", with this
code (especially with the "must be at least MINIMUM_ABBREV" logic),
but I do not think it is a problem, so I think this is good.
> @@ -591,12 +601,15 @@ static int grab_objectname(const char *name, const unsigned char *sha1,
> struct atom_value *v, struct used_atom *atom)
> {
> if (starts_with(name, "objectname")) {
> - if (atom->u.objectname == O_SHORT) {
> + if (atom->u.objectname.option == O_SHORT) {
> v->s = xstrdup(find_unique_abbrev(sha1, DEFAULT_ABBREV));
> return 1;
> - } else if (atom->u.objectname == O_FULL) {
> + } else if (atom->u.objectname.option == O_FULL) {
> v->s = xstrdup(sha1_to_hex(sha1));
> return 1;
> + } else if (atom->u.objectname.option == O_LENGTH) {
> + v->s = xstrdup(find_unique_abbrev(sha1, atom->u.objectname.length));
> + return 1;
^ permalink raw reply
* Re: [PATCH v7 03/17] ref-filter: implement %(if:equals=<string>) and %(if:notequals=<string>)
From: Junio C Hamano @ 2016-11-10 23:26 UTC (permalink / raw)
To: Jacob Keller; +Cc: Karthik Nayak, Git mailing list
In-Reply-To: <CA+P7+xoavgQ=NK8GGGXf90ESuPs+mFwHzEWbsnjWjZ4V7TWpjw@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
>> @@ -49,6 +51,10 @@ static struct used_atom {
>> enum { C_BARE, C_BODY, C_BODY_DEP, C_LINES, C_SIG, C_SUB } option;
>> unsigned int nlines;
>> } contents;
>> + struct {
>> + const char *if_equals,
>> + *not_equals;
>
>
> Same here, why do we need both strings here stored separately? Could
> we instead store which state to check and store the string once? I'm
> not sure that really buys us any storage.
I am not sure if storage is an issue, but I tend to agree that it
would be semantically cleaner if this was done as a pair of <what
operation uses this string constant?, the string constant>, and the
former would be enum { COMPARE_EQUAL, COMPARE_UNEQUAL}.
You could later enhance the comparison operator more easily with
such an arrangement (e.g. if-equals-case-insensitively).
^ permalink raw reply
* Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms
From: Junio C Hamano @ 2016-11-10 23:20 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Jacob Keller, Git mailing list
In-Reply-To: <CAOLa=ZS_V5SNSbiC_sT6E9rJMbxCMm=BRaoPL44xBNXAG2pgbA@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
>> Minor nit. I'm not sure what standard we use here at Git, but
>> traditionally, I prefer to see { } blocks on all sections even if only
>> one of them needs it. (That is, only drop the braces when every
>> section is one line.) It also looks weird with a comment since it
>> appears as multiple lines to the reader. I think the braces improve
>> readability.
>>
>> I don't know whether that's Git's code base standard or not, however.
>> It's not really worth a re-roll unless something else would need to
>> change.
>>
> I believe this is the syntax followed in Git, xdiff/xmerge.c:173 and so on.
That is a bad example for two reasons, if you mean this part:
if ((size = file->recs[i]->size) &&
file->recs[i]->ptr[size - 1] == '\n')
/* Last line; ends in LF; Is it CR/LF? */
return size > 1 &&
file->recs[i]->ptr[size - 2] == '\r';
* if (!i)
/* The only line has no eol */
return -1;
/* Determine eol from second-to-last line */
What Jacob prefers is this:
---------------------------------------------
if (cond)
simple;
else if (cond)
simple;
else
simple;
---------------------------------------------
if (cond) {
simple;
} else if (cond) {
no;
longer;
simple;
} else {
simple;
}
---------------------------------------------
That is, as long as all arms of if/else if/.../else cascade is
simple, {} is used nowhere in the cascade, but once even one of them
requires {} then all others gain {}.
^ permalink raw reply
* Re: [PATCH v7 01/17] ref-filter: implement %(if), %(then), and %(else) atoms
From: Junio C Hamano @ 2016-11-10 23:13 UTC (permalink / raw)
To: Jacob Keller; +Cc: Karthik Nayak, Git mailing list
In-Reply-To: <CA+P7+xoAmYqi0OazZux+rooXW+D_N9L5s+2BzUyJJJXtmLZX-Q@mail.gmail.com>
Jacob Keller <jacob.keller@gmail.com> writes:
> Ok, so I have only one minor nit, but otherwise this looks quite good
> to me. A few comments explaining my understanding, but only one
> suggested
> change which is really a minor nit and not worth re-rolling just for it.
As you didn't snip parts you didn't comment, I'll use this to add my
own for convenience ;-)
>> +if::
>> + Used as %(if)...%(then)...(%end) or
>> + %(if)...%(then)...%(else)...%(end). If there is an atom with
>> + value or string literal after the %(if) then everything after
>> + the %(then) is printed, else if the %(else) atom is used, then
>> + everything after %(else) is printed. We ignore space when
>> + evaluating the string before %(then), this is useful when we
>> + use the %(HEAD) atom which prints either "*" or " " and we
>> + want to apply the 'if' condition only on the 'HEAD' ref.
>> +
>> In addition to the above, for commit and tag objects, the header
>> field names (`tree`, `parent`, `object`, `type`, and `tag`) can
>> be used to specify the value in the header field.
I see a few instances of (%end) that were meant to be %(end).
Aren't the following two paragraphs ...
>> +When a scripting language specific quoting is in effect (i.e. one of
>> +`--shell`, `--perl`, `--python`, `--tcl` is used), except for opening
>> +atoms, replacement from every %(atom) is quoted when and only when it
>> +appears at the top-level (that is, when it appears outside
>> +%($open)...%(end)).
>> +When a scripting language specific quoting is in effect, everything
>> +between a top-level opening atom and its matching %(end) is evaluated
>> +according to the semantics of the opening atom and its result is
>> +quoted.
... saying the same thing?
>> + }
>> + } else if (!if_then_else->condition_satisfied)
>
> Minor nit. I'm not sure what standard we use here at Git, but
> traditionally, I prefer to see { } blocks on all sections even if only
> one of them needs it. (That is, only drop the braces when every
> section is one line.) It also looks weird with a comment since it
> appears as multiple lines to the reader. I think the braces improve
> readability.
>
> I don't know whether that's Git's code base standard or not, however.
> It's not really worth a re-roll unless something else would need to
> change.
>
In principle, we mimick the kernel style of using {} block even on a
single-liner body in if/else if/else cascade when any one of them is
not a single-liner and requires {}. But we often ignore that when a
truly trivial single liner follows if() even if its else clause is a
big block, e.g.
if (cond)
single;
else {
big;
block;
}
I agree with you that this case should just use {} for the following
paragraph, because it is technically a single-liner, but comes with
a big comment block and is very much easier to read with {} around
it.
>> + /*
>> + * No %(else) atom: just drop the %(then) branch if the
>> + * condition is not satisfied.
>> + */
>> + strbuf_reset(&cur->output);
Thanks.
^ permalink raw reply
* Re: [PATCH] doc: fill in omitted word
From: Jeff King @ 2016-11-10 23:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Kristoffer Haugsbakk, git
In-Reply-To: <xmqqd1i356ql.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 02:07:30PM -0800, Junio C Hamano wrote:
> Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com> writes:
>
> > Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
> > ---
> > Documentation/gitcore-tutorial.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
> > index 4546fa0..9860517 100644
> > --- a/Documentation/gitcore-tutorial.txt
> > +++ b/Documentation/gitcore-tutorial.txt
> > @@ -25,7 +25,7 @@ you want to understand Git's internals.
> > The core Git is often called "plumbing", with the prettier user
> > interfaces on top of it called "porcelain". You may not want to use the
> > plumbing directly very often, but it can be good to know what the
> > -plumbing does for when the porcelain isn't flushing.
> > +plumbing does for you when the porcelain isn't flushing.
>
> I need an English teacher here to help me out, but I think this
> changes the meaning of the sentence from what the original author
> intended..
It does. I'd take the original to mean "for the case when", which is
correct (though as you note, just dropping "for" says the same thing
more succinctly).
-Peff
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Jeff King @ 2016-11-10 23:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Andreas Schwab, Johannes Sixt, git
In-Reply-To: <xmqq1syj54j9.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 02:55:06PM -0800, Junio C Hamano wrote:
> If we ensure that the process is still running, then such a check is
> a good belt-and-suspenders way to catch a breakage in the mechanism
> we choose to ensure it. So probably we can require that the kill in
> the "when finished" part to actually send a signal to a process that
> is still running.
>
> Is there an equivalent to pause(2) available to shell scripts? I
> really hate a single "sleep 3600" or anything with a magic number.
I think it is usually spelled "read <some-fifo", but we can't use FIFOs
here because Windows doesn't have them. You could probably do something
with "read <&9" and set up descriptor 9 in the test code. But frankly,
that gets complex pretty quickly, as you have to background things.
This minor issue isn't worth it. Just bumping the sleep to 3600 makes
the raciness problem go away, and everything works in practice. That's
probably good enough for our purposes.
-Peff
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Junio C Hamano @ 2016-11-10 22:55 UTC (permalink / raw)
To: Jeff King; +Cc: Johannes Schindelin, Andreas Schwab, Johannes Sixt, git
In-Reply-To: <20161110223522.4b35ojaz5nhk4sll@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I do think the test would be a lot more obvious if it confirmed at the
> end of the test that the process was still running, as opposed to
> relying on test_when_finished to check it.
I agree that "check that the process is still running" is a wrong
thing to do in the first place. What would it mean if the process
is no longer running? It is a timing-dependent bug in the test;
after all we failed to produce the condition that could trigger a
bug that we are guarding against. And "let's do '|| :'" is sweeping
the bug (not in the code we are testing, but in the test that will
fail to notice a bug we are preparing against) under the rug. So I
agree with Dscho that we should do that first.
If we ensure that the process is still running, then such a check is
a good belt-and-suspenders way to catch a breakage in the mechanism
we choose to ensure it. So probably we can require that the kill in
the "when finished" part to actually send a signal to a process that
is still running.
Is there an equivalent to pause(2) available to shell scripts? I
really hate a single "sleep 3600" or anything with a magic number.
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Jeff King @ 2016-11-10 22:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, Andreas Schwab, Johannes Sixt, git
In-Reply-To: <xmqq60nv55o3.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 02:30:36PM -0800, Junio C Hamano wrote:
> As everybody knows there is no appropriate timeout value that is
> good for everybody. I wonder if we can replace the sleep 1 with
> something like
>
> ( while sleep 3600; do :; done ) &
>
> so that leaked fd will be kept even in any heavily loaded
> environment instead?
I think you may have missed:
http://public-inbox.org/git/16dc9f159b214997f7501006a8d1d8be2ef858e8.1478699463.git.johannes.schindelin@gmx.de/
which does roughly that. It does not loop, but I suspect 3600 is plenty
in practice.
I do think the test would be a lot more obvious if it confirmed at the
end of the test that the process was still running, as opposed to
relying on test_when_finished to check it.
-Peff
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Junio C Hamano @ 2016-11-10 22:30 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Andreas Schwab, Jeff King, Johannes Sixt, git
In-Reply-To: <alpine.DEB.2.20.1611102254340.24684@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> OK. sleep.pid is a reasonable easy-to-access side effect we can
>> observe to make sure that the sleep-one-second merge driver was
>> indeed invoked, which was missing from the earlier round.
>
> No, this is incorrect. The condition that we need to know applies is that
> the script is still running, and blocking if the bug reappears.
OK, I see what you are saying, and I see a few things wrong in here:
* First, the test is titled in a misleading way. In the context of
a patch that was titled ad65f7e3b7 ("t6026-merge-attr: child
processes must not inherit index.lock handles", 2016-08-18), it
might have been clear enough to say "does not lock index", but
the sleeping is to make sure that we would notice if the fd to
the index.lock leaked to the child process by mistake, and the
way to do so is that the child arranges the leaked fd to be kept
open after it exits (by spawning "sleep"). The test was never
about "does not lock index" (the driver does not take any lock by
itself in the first place).
* There are three possible outcome from this test:
- 'git merge' fails.
This is expected to happen only on Windows and if the code gets
broken and starts leaking the fd.
- 'git merge' finishes correctly, the sleep is still running when
test_when_finished goes to cull it.
In this case, we KNOW there wasn't any fd leak IF we are on
Windows where a leaked FD would not allow 'git merge' to
succeed. But on other platforms, fd leak that may cause
trouble for Windows friends will not be caught.
- 'git merge' finishes correctly, the sleep is no longer
running because the machine was heavily loaded; a workaround is
to tolerate failure of culling it.
In this case, we cannot tell anything from the test. Even if
the fd was leaked, 'git merge' may have succeeded even on
Windows.
As everybody knows there is no appropriate timeout value that is
good for everybody. I wonder if we can replace the sleep 1 with
something like
( while sleep 3600; do :; done ) &
so that leaked fd will be kept even in any heavily loaded
environment instead?
^ permalink raw reply
* Re: [PATCH] doc: fill in omitted word
From: Junio C Hamano @ 2016-11-10 22:07 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: git
In-Reply-To: <20161104214357.32477-1-kristoffer.haugsbakk@gmail.com>
Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com> writes:
> Signed-off-by: Kristoffer Haugsbakk <kristoffer.haugsbakk@gmail.com>
> ---
> Documentation/gitcore-tutorial.txt | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/gitcore-tutorial.txt b/Documentation/gitcore-tutorial.txt
> index 4546fa0..9860517 100644
> --- a/Documentation/gitcore-tutorial.txt
> +++ b/Documentation/gitcore-tutorial.txt
> @@ -25,7 +25,7 @@ you want to understand Git's internals.
> The core Git is often called "plumbing", with the prettier user
> interfaces on top of it called "porcelain". You may not want to use the
> plumbing directly very often, but it can be good to know what the
> -plumbing does for when the porcelain isn't flushing.
> +plumbing does for you when the porcelain isn't flushing.
I need an English teacher here to help me out, but I think this
changes the meaning of the sentence from what the original author
intended..
The way I've read this statement for the past ten years since it was
written originally at 8c7fa2478e ("Add first cut at a simple git
tutorial.", 2005-05-31) and then slightly reworded at f35ca9ed3e
("tutorial.txt: start describing how to copy repositories",
2005-06-01) is that
* there are unfortunate occasions in which the porcelain is not
flushing, and
* the knowledge of what the plumbing does is a good thing to have
for such occasions. It will help you figure out what needs to be
done.
The rewritten text means a very different thing, at least to me.
* there are things plumbing does for you.
* the knowledge of what they are helps when the porcelain isn't
flushing.
Just dropping "for" instead of adding "you" may make it easier to
read, grammatically more correct and retain the original intent
better, I suspect.
^ permalink raw reply
* Re: [PATCH v2] t6026-merge-attr: don't fail if sleep exits early
From: Johannes Schindelin @ 2016-11-10 21:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Andreas Schwab, Jeff King, Johannes Sixt, git
In-Reply-To: <xmqqbmxn6t11.fsf@gitster.mtv.corp.google.com>
Hi all,
On Thu, 10 Nov 2016, Junio C Hamano wrote:
> Andreas Schwab <schwab@suse.de> writes:
>
> > Commit 5babb5bdb3 ("t6026-merge-attr: clean up background process at end
> > of test case") added a kill command to clean up after the test, but this
> > can fail if the sleep command exits before the cleanup is executed.
> > Ignore the error from the kill command.
> >
> > Explicitly check for the existence of the pid file to test that the merge
> > driver was actually called.
> >
> > Signed-off-by: Andreas Schwab <schwab@suse.de>
> > ---
>
> OK. sleep.pid is a reasonable easy-to-access side effect we can
> observe to make sure that the sleep-one-second merge driver was
> indeed invoked, which was missing from the earlier round.
No, this is incorrect. The condition that we need to know applies is that
the script is still running, and blocking if the bug reappears.
It is not enough to test that the script *started* running. If it exits
too early, the problematic condition is never tested, and the test is
useless.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Jeff King @ 2016-11-10 21:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <xmqqk2cb57jz.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 01:49:52PM -0800, Junio C Hamano wrote:
> Yes, I recall the IIS one raised and discussed at least twice on the
> list in the past, and it sounded that we want some solution to that.
The patches had some issues. I suspect the population of people who want
to run a git server on IIS is relatively small. I am content to wait for
somebody who has such a setup to produce a working patch.
> > 3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
> > a bundle file (Apache gives you a 404, lighttpd serves the bundle).
>
> That's a bad one. Do we want a client-side "I am connecting to a
> site that knows how to talk smart-http" option or something to work
> it around?
It doesn't matter because we don't actually support fetching HTTP
bundles via "git fetch" yet.
But I ran it across the issue and did make such a fix when I was
implementing that feature long ago. See the discussion of "surprise" in
[1] and [2].
Wow, that series is exactly 5 years old today. Have I really been
procrastinating on re-rolling it that long? Yikes.
-Peff
[1] http://public-inbox.org/git/20111110075052.GI27950@sigill.intra.peff.net/
[2] http://public-inbox.org/git/20111110075052.GI27950@sigill.intra.peff.net/
^ permalink raw reply
* Re: "git subtree --squash" interacts poorly with revert, merge, and rebase
From: Matt McCutchen @ 2016-11-10 21:53 UTC (permalink / raw)
To: git
In-Reply-To: <1477523244.2764.114.camel@mattmccutchen.net>
On Wed, 2016-10-26 at 19:07 -0400, Matt McCutchen wrote:
> Maybe we would never hit any of these problems in practice, but they
> give me a bad enough feeling that I'm planning to write my own tool
> that tracks the upstream commit ID in a file (like a submodule) and
> doesn't generate any extra commits. Without generating extra commits,
> the only place to store the upstream content in the superproject would
> be in another subtree, which would take up disk space in every working
> tree unless developers manually set skip-worktree. I think I prefer to
> not store the upstream content and just have the tool fetch it from a
> local subproject repository each time it's needed.
>
> I'll of course post the tool on the web and would be happy to see it
> integrated into "git subtree" if that makes sense, but I don't know how
> much time I'd be willing to put into making that happen.
I have named my tool "git subtree-lite" and posted it here:
https://mattmccutchen.net/utils/git-subtree-lite.git/
For now, please email any bug reports, enhancement requests, or
proposed patches to me. I have philosophical concerns about hosting my
own projects on services I don't control and practical concerns about a
few "forge" apps that I looked into installing on my own web site, but
if people are seriously interested in collaborating on this, I'll work
something out.
Matt
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Junio C Hamano @ 2016-11-10 21:49 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <20161110214345.cau5i4eybqdv74k3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> I however do not know what the universally available simplest dummy
>> HTTP server would be. There probably are better alternative than
>> Apache with distro-customized ways of configuration that we have to
>> adjust.
>>
>> A solution around HTTP::Server::Simple sounds attractive but is it
>> a realistic alternative or too much effort required? I dunno.
>
> I'm less concerned about the amount of effort (though I agree it may be
> a blocker) than about the fact that it may not behave similarly to real
> servers.
I was wondering if it takes too much effort to make it behave
similarly to real servers, so I guess we are on the same page.
Yes, I recall the IIS one raised and discussed at least twice on the
list in the past, and it sounded that we want some solution to that.
> 3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
> a bundle file (Apache gives you a 404, lighttpd serves the bundle).
That's a bad one. Do we want a client-side "I am connecting to a
site that knows how to talk smart-http" option or something to work
it around?
^ permalink raw reply
* Re: [git-for-windows] [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Johannes Schindelin @ 2016-11-10 21:39 UTC (permalink / raw)
To: Lars Schneider; +Cc: git-for-windows, Git Mailing List, me
In-Reply-To: <B2BEB5B4-5CF0-4CD7-A8E2-50D51E00D2FF@gmail.com>
Hi Lars,
On Wed, 9 Nov 2016, Lars Schneider wrote:
> On 05 Nov 2016, at 10:50, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>
> > I finally got around to rebase the Windows-specific patches (which seem to
> > not make it upstream as fast as we get new ones) on top of upstream Git
> > v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
> >
> > https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
>
>
> I tested a new feature in 2.11 on Windows today and it failed. After some
> confusion I realized that the feature is not on your 2.11 branch.
Oops. That must have been a major snafu on my side, very sorry for that.
I just tagged v2.11.0-rc0.windows.2 in https://github.com/dscho/git and
will make a new prerelease tomorrow.
My apologies!
Dscho
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-10 21:43 UTC (permalink / raw)
To: git
In-Reply-To: <xmqqk2cgc95m.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> I'll report back an updated schedule when able.
I pushed some updates out on 'master' today. Between 'master' and
'pu' on the first-parent history there is a merge 52975d2b1f ("Merge
branch 'ls/macos-update' into jch", 2016-11-10) and that matches
what I expect to be in -rc1 tomorrow (modulo RelNotes and the actual
version tag), unless there is a showstopper regresion reported, in
which case we may want to first look into reverting the whole series
that introduced the regression before considering to pile on fix-up
patches.
http://tinyurl.com/gitCal has been redrawn, with -rc1 scheduled for
tomorrow on 11th, -rc2 on the 17th, and final on the 23rd.
Thanks.
^ permalink raw reply
* Re: [PATCH v1 2/2] travis-ci: disable GIT_TEST_HTTPD for macOS
From: Jeff King @ 2016-11-10 21:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Torsten Bögershausen, git
In-Reply-To: <xmqq1syj6mvq.fsf@gitster.mtv.corp.google.com>
On Thu, Nov 10, 2016 at 01:33:29PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > IMHO, the value in the http tests is not testing the server side, but
> > the client side. Without being able to set up a dummy HTTP server, we do
> > not have any way to exercise the client side of git-over-http at all.
> > And people on macOS _do_ use that. :)
>
> Amen to that.
>
> I however do not know what the universally available simplest dummy
> HTTP server would be. There probably are better alternative than
> Apache with distro-customized ways of configuration that we have to
> adjust.
>
> A solution around HTTP::Server::Simple sounds attractive but is it
> a realistic alternative or too much effort required? I dunno.
I'm less concerned about the amount of effort (though I agree it may be
a blocker) than about the fact that it may not behave similarly to real
servers. We have had real bugs and surprises with the way that various
web servers implement things. A few that come to mind are:
1. Buffering/deadlock issues between the webserver and the CGI (this
was an issue with Apache, but I don't know about other servers).
2. The handling of CONTENT_LENGTH with chunked-encoding (this is still
an issue with IIS).
3. What happens when you ask for "foo.git/info/refs" and "foo.git" is
a bundle file (Apache gives you a 404, lighttpd serves the bundle).
Ideally we'd test against a lot of different webservers, but that's
expensive (in CPU, but also in developer time). But I'd guess that
Apache is at least more representative than HTTP::Server::Simple of real
servers in the wild.
So if we had a simple fallback in addition to Apache, I'd be OK with
that. But we still have a problem that (say) people on MacOS would never
actually test against Apache, because it's not supported there.
-Peff
^ 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