Re: [PATCH v7 20/22] commit-graph: add '--reachable' option
On Wed, Jun 27, 2018 at 3:24 PM, Derrick Stolee wrote: > When writing commit-graph files, it can be convenient to ask for all > reachable commits (starting at the ref set) in the resulting file. This > is particularly helpful when writing to stdin is complicated, such as a > future integration with 'git gc'. It would be nice if the "Future Work" section of Documentation/technical/commit-graph.txt had something about integration with 'git gc'. > With the `--stdin-commits` option, generate the new commit graph by > walking commits starting at the commits specified in stdin as a list > of OIDs in hex, one OID per line. (Cannot be combined with > ---stdin-packs.) > +`--stdin-packs` or `--reachable`.) > ++ > +With the `--reachable` option, generate the new commit graph by walking > +commits starting at all refs. (Cannot be combined with `--stdin-commits` > +or `--stdin-packs`.) > + > With the `--append` option, include all commits that are present in the > existing commit-graph file. The "EXAMPLES" section still contains: * Write a graph file containing all reachable commits. + $ git show-ref -s | git commit-graph write --stdin-commits I wonder if this should have been changed to use `--reachable`. Thanks!
Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero
Junio C Hamano wrote: > Jonathan Nieder writes: >> RFC 3875 (the CGI specification) explains: >> >>The CONTENT_LENGTH variable contains the size of the message-body >>attached to the request, if any, in decimal number of octets. If no >>data is attached, then NULL (or unset). [...] >> But that specification was written before HTTP/1.1 and chunked >> encoding. > > RFC 3875 is from October 2004, while RFC 2616 (HTTP/1.1) is from > June 1999; I presume that 3875 only writes down what has already > been an established practice and that is where the date discrepancy > comes from. Yes, CGI 1.1 is from 1995. More details are at https://www.w3.org/CGI/. [...] >> Fortunately, there's a way out. Use the HTTP_TRANSFER_ENCODING >> environment variable to distinguish the two cases. > > Cute. > > I'm anxious to learn how well this works in practice. Or is this a > trick you know somebody else's system already uses (in which case, > that's wonderful)? Alas, I came up with it today so I don't know yet how well it will work in practice. I can poke around a little tomorrow in Apache to sanity-check the approach. Results from anyone able to test using various HTTP servers (lighttpd, etc) would also be very welcome. Thanks, Jonathan
Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero
Jonathan Nieder writes:
> RFC 3875 (the CGI specification) explains:
>
>The CONTENT_LENGTH variable contains the size of the message-body
>attached to the request, if any, in decimal number of octets. If no
>data is attached, then NULL (or unset).
>
> CONTENT_LENGTH = "" | 1*digit
>
> And:
>
>This specification does not distinguish between zero-length (NULL)
>values and missing values.
>
> But that specification was written before HTTP/1.1 and chunked
> encoding.
RFC 3875 is from October 2004, while RFC 2616 (HTTP/1.1) is from
June 1999; I presume that 3875 only writes down what has already
been an established practice and that is where the date discrepancy
comes from.
This is a bit old but some of those who participated in the
discussion archived at https://lists.gt.net/apache/users/373042
seemed to know what they were talking about. In short, lack of
content-length for CGI scripts driven by Apache seems to mean that
the content length is unknown and we are expected to read through to
the eof, and we are expected to ignore the CGI spec, which says
missing CONTENT_LENGTH and CONTENT_LENGTH set to an empty string
both must mean there is no message body. Instead, we need to take
the former as a sign to read through to the end.
> Fortunately, there's a way out. Use the HTTP_TRANSFER_ENCODING
> environment variable to distinguish the two cases.
Cute.
I'm anxious to learn how well this works in practice. Or is this a
trick you know somebody else's system already uses (in which case,
that's wonderful)?
> + if (!str || !*str) {
> + /*
> + * According to RFC 3875, an empty or missing
> + * CONTENT_LENGTH means "no body", but RFC 3875
> + * precedes HTTP/1.1 and chunked encoding. Apache and
> + * its imitators leave CONTENT_LENGTH unset for
> + * chunked requests, for which we should use EOF to
> + * detect the end of the request.
> + */
> + str = getenv("HTTP_TRANSFER_ENCODING");
> + if (str && !strcmp(str, "chunked"))
> + return -1;
> +
> + return 0;
> + }
> + if (!git_parse_ssize_t(str, ))
> die("failed to parse CONTENT_LENGTH: %s", str);
> return val;
> }
Re: [PATCH v3] http-backend: allow empty CONTENT_LENGTH
Max Kirillov wrote: > On Sun, Sep 09, 2018 at 10:17:48PM -0700, Jonathan Nieder wrote: >> From: Max Kirillov >> Subject: http-backend test: make empty CONTENT_LENGTH test more realistic > > Thank you, yes, this is what should have left Oh, tying up this loose end: do you know why the test passes without 574c513e8d (http-backend: allow empty CONTENT_LENGTH, 2018-09-10)?
Re: [PATCH] http-backend: treat empty CONTENT_LENGTH as zero
Kicking off the reviews: ;-)
Jonathan Nieder wrote:
> --- a/http-backend.c
> +++ b/http-backend.c
> @@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t
> req_len, unsigned char **o
>
> static ssize_t get_content_length(void)
[...]
> + /*
> + * According to RFC 3875, an empty or missing
> + * CONTENT_LENGTH means "no body", but RFC 3875
> + * precedes HTTP/1.1 and chunked encoding. Apache and
> + * its imitators leave CONTENT_LENGTH unset for
Which imitators? Maybe this should just say "Apache leaves [...]".
> + * chunked requests, for which we should use EOF to
> + * detect the end of the request.
> + */
> + str = getenv("HTTP_TRANSFER_ENCODING");
> + if (str && !strcmp(str, "chunked"))
RFC 2616 says Transfer-Encoding is a list of transfer-codings applied,
in the order that they were applied, and that "chunked" is always
applied last. That means a transfer-encoding like
Transfer-Encoding: identity chunked
would be permitted, or e.g.
Transfer-Encoding: gzip chunked
Does that means we should be using a check like
str && (!strcmp(str, "chunked") || ends_with(str, " chunked"))
?
That said, a quick search of codesearch.debian.net mostly finds
examples using straight comparison, so maybe the patch is fine as-is.
Thanks,
Jonathan
[PATCH] http-backend: treat empty CONTENT_LENGTH as zero
As discussed in v2.19.0-rc0~45^2~2 (http-backend: respect
CONTENT_LENGTH as specified by rfc3875, 2018-06-10), HTTP servers such
as IIS do not close a CGI script's standard input at the end of a
request, instead expecting CGI scripts to stop reading after
CONTENT_LENGTH bytes. That commit taught http-backend to respect this
convention except when CONTENT_LENGTH is unset, in which case it
preserved the previous behavior of reading until EOF.
RFC 3875 (the CGI specification) explains:
The CONTENT_LENGTH variable contains the size of the message-body
attached to the request, if any, in decimal number of octets. If no
data is attached, then NULL (or unset).
CONTENT_LENGTH = "" | 1*digit
And:
This specification does not distinguish between zero-length (NULL)
values and missing values.
But that specification was written before HTTP/1.1 and chunked
encoding. With chunked encoding, the length of a request is not known
early and it is useful to start a CGI script to process it anyway, so
Apache and many other servers violate the spec: they leave
CONTENT_LENGTH unset and rely on EOF to indicate the end of request.
This is reproducible using t5510-fetch.sh, which hangs if http-backend
is patched to treat a missing CONTENT_LENGTH as zero.
So we are in a bind: to support HTTP servers that don't produce EOF,
http-backend should respect an unset or empty CONTENT_LENGTH that
represents zero, and to support chunked encoding, http-backend should
respect an unset CONTENT_LENGTH that represents "read until EOF".
Fortunately, there's a way out. Use the HTTP_TRANSFER_ENCODING
environment variable to distinguish the two cases.
Reported-by: Jeff King
Helped-by: Max Kirillov
Signed-off-by: Jonathan Nieder
---
How about this?
http-backend.c | 19 +--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 458642ef72..7902eeb0b3 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -350,10 +350,25 @@ static ssize_t read_request_fixed_len(int fd, ssize_t
req_len, unsigned char **o
static ssize_t get_content_length(void)
{
- ssize_t val = -1;
+ ssize_t val;
const char *str = getenv("CONTENT_LENGTH");
- if (str && *str && !git_parse_ssize_t(str, ))
+ if (!str || !*str) {
+ /*
+* According to RFC 3875, an empty or missing
+* CONTENT_LENGTH means "no body", but RFC 3875
+* precedes HTTP/1.1 and chunked encoding. Apache and
+* its imitators leave CONTENT_LENGTH unset for
+* chunked requests, for which we should use EOF to
+* detect the end of the request.
+*/
+ str = getenv("HTTP_TRANSFER_ENCODING");
+ if (str && !strcmp(str, "chunked"))
+ return -1;
+
+ return 0;
+ }
+ if (!git_parse_ssize_t(str, ))
die("failed to parse CONTENT_LENGTH: %s", str);
return val;
}
--
2.19.0.397.gdd90340f6a
Re: [PATCH 1/2] trace: add trace_print_string_list_key
On Mon, Sep 10, 2018 at 5:52 PM Junio C Hamano wrote:
>
> Stefan Beller writes:
>
> >> Of course, even though these are 1/2 and 2/2, only one of them and
> >> not both would apply.
> >
> > Or you could squash them once we reach consensus that both are good.
>
> Ah, sorry, I completely misread the first one. I thought that it
> was extending the implementation of existing unused function by
> using trace API, which explains why I mistook them as an either-or
> choice. I did not realize 1/2 was adding yet another unused one
> without doing anything to the existing unused one.
>
> So the choice being offered are:
>
> (0) take 2/2 only, keeping zero unused helper.
> (1) take 1/2 only, keeping two unused helpers.
> (2) do nothing, keeping the simple unused helper we had from the
> beginning of time.
> (3) take 1/2 and 2/2, replacing one simple unused helper with
> another unused helper that is more complex and capable.
>
> Are you planning to, or do you know somebody who plans to, use one
> or the other if available in a near future? If so, it would be OK
> to take choice (2) or (3), and it probably is preferrable to take
> (3) between them. A more complex and capable one would require
> maintenance over time (trace API is being updated with the trace2 in
> another topic that will start flying soon, so it would be expected a
> user of trace API may need update), but as long as that is actually
> used and help developers, that maintenance cost is worth paying.
>
> If not, I would say that the option (1) or (3) are worse choices
> than either (0) or (2). It would be better to minimize maintenance
> cost for unused helper(s), and the simpler one is likely to stay
> maintenance free for longer than the more complex and capable one,
> so (1) and (3) do not make much sense unless we plan to start using
> these real soon.
Yes, I think (0) is the way to go, actually.
I wrote patch 1/2 to show Peff and you to prove otherwise that I am
not contributing "only grudgingly".
If the current unused function would be actually helpful in debugging
I would not remove it, but actually use it.
>
> >> It is not costing us much to leave it in the code. It's not like
> >> the function costed a lot of maintenance burden since it was added
> >> in 8fd2cb40 ("Extract helper bits from c-merge-recursive work",
> >> 2006-07-25), so the alternative #3 might be to do nothing.
> >
> > True, but ...
> >
> >> somebody else in the future to propose removing
> >
> > is what is actually happening here already, see
> >
> > https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovm...@gmail.com/
> >
> >> I am inclined to say we'd take
> >> 2/2 ;-)
> >>
> >> Thanks.
> >
> > Feel free to take Alexanders patch from 2015 instead.
>
> I prefer to take 2/2 over the one from 2015, especially if we can
> explain the removal better. We had three extra years that the
> helper stayed unused and unchanged, which gives us a better
> indication that it won't be missed.
Will send a patch with better reasons tomorrow,
Thanks,
Stefan
Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
On Mon, Sep 10, 2018 at 07:20:28PM -0700, Jonathan Nieder wrote:
> Jeff King wrote:
> > On Mon, Sep 10, 2018 at 02:22:21PM -0700, Jonathan Nieder wrote:
>
> >> Thanks. I am wondering if we should go all the way and do
> >>
> >>ssize_t val;
> >>const char *str = getenv("CONTENT_LENGTH");
> >>
> >>if (!str || !*str)
> >>return 0;
> >>if (!git_parse_ssize_t(str, ))
> >>die(...);
> >>return val;
> >>
> >> That would match the RFC, but it seems to make t5510-fetch.sh hang,
> [...]
> >> Do you know why?
> >
> > Yes. :)
> >
> > It's due to this comment in the patch you are replying to:
> >
> > + if (!str) {
> > + /*
> > +* RFC3875 says this must mean "no body", but in practice we
> > +* receive chunked encodings with no CONTENT_LENGTH. Tell
> > the
> > +* caller to read until EOF.
> > +*/
> > + val = -1;
>
> Ah! So "in practice" includes "in Apache". An old discussion[1] on
> Apache's httpd-users list agrees.
>
> The question then becomes: what does IIS do for zero-length requests?
> Does any other web server fail to support "read until EOF" in general?
>
> The CGI standard does not cover chunked encoding so we can't lean on
> the standard for advice. It's not clear to me yet whether this patch
> improves on what's in "master".
I'd note that the case in question (no CONTENT_LENGTH at all) is not
changed between this patch and master. It's only the case of
CONTENT_LENGTH set to an empty string. But I agree that it is not clear
to me whether it is actually improving anything in practice.
-Peff
Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
Jeff King wrote:
> On Mon, Sep 10, 2018 at 02:22:21PM -0700, Jonathan Nieder wrote:
>> Thanks. I am wondering if we should go all the way and do
>>
>> ssize_t val;
>> const char *str = getenv("CONTENT_LENGTH");
>>
>> if (!str || !*str)
>> return 0;
>> if (!git_parse_ssize_t(str, ))
>> die(...);
>> return val;
>>
>> That would match the RFC, but it seems to make t5510-fetch.sh hang,
[...]
>> Do you know why?
>
> Yes. :)
>
> It's due to this comment in the patch you are replying to:
>
> + if (!str) {
> + /*
> +* RFC3875 says this must mean "no body", but in practice we
> +* receive chunked encodings with no CONTENT_LENGTH. Tell the
> +* caller to read until EOF.
> +*/
> + val = -1;
Ah! So "in practice" includes "in Apache". An old discussion[1] on
Apache's httpd-users list agrees.
The question then becomes: what does IIS do for zero-length requests?
Does any other web server fail to support "read until EOF" in general?
The CGI standard does not cover chunked encoding so we can't lean on
the standard for advice. It's not clear to me yet whether this patch
improves on what's in "master".
Thanks,
Jonathan
[1]
http://mail-archives.apache.org/mod_mbox/httpd-users/200909.mbox/%3c4aaacc38.3070...@rowe-clan.net%3E
Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
On Mon, Sep 10, 2018 at 11:53:59PM +0300, Max Kirillov wrote: > From: Jeff King > Subject: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero > > There is no known case where empty body it used by a server as > instruction to read until EOF, so there is no need to violate the RFC. > Make get_content_length() return 0 in this case. > > Currently there is no practical difference, as the GET request > where it can be empty is handled without actual reading the body > (in get_info_refs() function), but it is better to stick to the correct > behavior. There could be a difference if there is a server which actually sets CONTENT_LENGTH to the empty string for a chunked body. But we don't know of any such server at this point. > Signed-off-by: Max Kirillov > --- > The incremental. Hopefully I described the reason right. Needs "signed-off-by" Certainly this is: Signed-off-by: Jeff King -Peff
Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
On Mon, Sep 10, 2018 at 02:22:21PM -0700, Jonathan Nieder wrote:
> Thanks. I am wondering if we should go all the way and do
>
> ssize_t val;
> const char *str = getenv("CONTENT_LENGTH");
>
> if (!str || !*str)
> return 0;
> if (!git_parse_ssize_t(str, ))
> die(...);
> return val;
>
> That would match the RFC, but it seems to make t5510-fetch.sh hang,
> right after
>
> ok 165 - --negotiation-tip understands abbreviated SHA-1
>
> When I run with -v -i -x, it stalls at
>
> ++ git -C '/usr/local/google/home/jrn/src/git/t/trash
> directory.t5510-fetch/httpd/www/server' tag -d alpha_1 alpha_2 beta_1 beta_2
> Deleted tag 'alpha_1' (was a84e4a9)
> Deleted tag 'alpha_2' (was 7dd5cf4)
> Deleted tag 'beta_1' (was bcb5c65)
> Deleted tag 'beta_2' (was d3b6dcd)
> +++ pwd
> ++ GIT_TRACE_PACKET='/usr/local/google/home/jrn/src/git/t/trash
> directory.t5510-fetch/trace'
> ++ git -C client fetch --negotiation-tip=alpha_1 --negotiation-tip=beta_1
> origin alpha_s beta_s
>
> Do you know why?
Yes. :)
It's due to this comment in the patch you are replying to:
+ if (!str) {
+ /*
+* RFC3875 says this must mean "no body", but in practice we
+* receive chunked encodings with no CONTENT_LENGTH. Tell the
+* caller to read until EOF.
+*/
+ val = -1;
-Peff
Re: [PATCH 1/2] trace: add trace_print_string_list_key
Stefan Beller writes:
>> Of course, even though these are 1/2 and 2/2, only one of them and
>> not both would apply.
>
> Or you could squash them once we reach consensus that both are good.
Ah, sorry, I completely misread the first one. I thought that it
was extending the implementation of existing unused function by
using trace API, which explains why I mistook them as an either-or
choice. I did not realize 1/2 was adding yet another unused one
without doing anything to the existing unused one.
So the choice being offered are:
(0) take 2/2 only, keeping zero unused helper.
(1) take 1/2 only, keeping two unused helpers.
(2) do nothing, keeping the simple unused helper we had from the
beginning of time.
(3) take 1/2 and 2/2, replacing one simple unused helper with
another unused helper that is more complex and capable.
Are you planning to, or do you know somebody who plans to, use one
or the other if available in a near future? If so, it would be OK
to take choice (2) or (3), and it probably is preferrable to take
(3) between them. A more complex and capable one would require
maintenance over time (trace API is being updated with the trace2 in
another topic that will start flying soon, so it would be expected a
user of trace API may need update), but as long as that is actually
used and help developers, that maintenance cost is worth paying.
If not, I would say that the option (1) or (3) are worse choices
than either (0) or (2). It would be better to minimize maintenance
cost for unused helper(s), and the simpler one is likely to stay
maintenance free for longer than the more complex and capable one,
so (1) and (3) do not make much sense unless we plan to start using
these real soon.
>> It is not costing us much to leave it in the code. It's not like
>> the function costed a lot of maintenance burden since it was added
>> in 8fd2cb40 ("Extract helper bits from c-merge-recursive work",
>> 2006-07-25), so the alternative #3 might be to do nothing.
>
> True, but ...
>
>> somebody else in the future to propose removing
>
> is what is actually happening here already, see
>
> https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovm...@gmail.com/
>
>> I am inclined to say we'd take
>> 2/2 ;-)
>>
>> Thanks.
>
> Feel free to take Alexanders patch from 2015 instead.
I prefer to take 2/2 over the one from 2015, especially if we can
explain the removal better. We had three extra years that the
helper stayed unused and unchanged, which gives us a better
indication that it won't be missed.
Thanks.
Re: [PATCH 1/2] trace: add trace_print_string_list_key
On Mon, Sep 10, 2018 at 3:32 PM Junio C Hamano wrote:
>
> Stefan Beller writes:
>
> > I separated this from the other series, making it into 2 patches:
> > This first patch adds tracing for string lists and the next patch that
> > removes the unused function from the string list API.
> > That way we can decide on these two patches separately if needed.
>
> Of course, even though these are 1/2 and 2/2, only one of them and
> not both would apply.
Or you could squash them once we reach consensus that both are good.
> Thanks for sticking to the topic.
>
> Given how simple that "dump them to standard output" code is, I am
> inclined to say that anybody who needs to inspect the contents of
> string list at various points in the code under development can
> create one from scratch even if we did not have this implementation,
> so perhaps 2/2 is a better choice between the two.
This sounds like the consensus is not to take both but only 2/2,
which I'd be happy with, too.
> It is not costing us much to leave it in the code. It's not like
> the function costed a lot of maintenance burden since it was added
> in 8fd2cb40 ("Extract helper bits from c-merge-recursive work",
> 2006-07-25), so the alternative #3 might be to do nothing.
True, but ...
> somebody else in the future to propose removing
is what is actually happening here already, see
https://public-inbox.org/git/1421343725-3973-1-git-send-email-kuleshovm...@gmail.com/
> I am inclined to say we'd take
> 2/2 ;-)
>
> Thanks.
Feel free to take Alexanders patch from 2015 instead.
Thanks,
Stefan
Re: [PATCH 1/2] trace: add trace_print_string_list_key
Stefan Beller writes:
> I separated this from the other series, making it into 2 patches:
> This first patch adds tracing for string lists and the next patch that
> removes the unused function from the string list API.
> That way we can decide on these two patches separately if needed.
Of course, even though these are 1/2 and 2/2, only one of them and
not both would apply.
Thanks for sticking to the topic.
Given how simple that "dump them to standard output" code is, I am
inclined to say that anybody who needs to inspect the contents of
string list at various points in the code under development can
create one from scratch even if we did not have this implementation,
so perhaps 2/2 is a better choice between the two.
It is not costing us much to leave it in the code. It's not like
the function costed a lot of maintenance burden since it was added
in 8fd2cb40 ("Extract helper bits from c-merge-recursive work",
2006-07-25), so the alternative #3 might be to do nothing.
I have no strong preference between #2 and #3. The benefit of #2
compared to #3 is that, if we remove it today, there will not be
somebody else in the future to come and propose removing the
otherwise unused function, which would cost us time to review and
discuss, so unless somebody stops me, I am inclined to say we'd take
2/2 ;-)
Thanks.
Re: [RFC PATCH 5/5] split-index: smudge and add racily clean cache entries to split index
Hello, On 06.09.2018 20:53, Ævar Arnfjörð Bjarmason wrote: On Thu, Sep 06 2018, Ævar Arnfjörð Bjarmason wrote: On Thu, Sep 06 2018, SZEDER Gábor wrote: On Thu, Sep 06, 2018 at 02:26:49PM +0200, Ævar Arnfjörð Bjarmason wrote: On Thu, Sep 06 2018, SZEDER Gábor wrote: Several tests failed occasionally when the test suite was run with 'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace back to this racy split index problem, starting with those failing more frequently, with a link to a failing Travis CI build job for each. The highlighted line shows when the racy file was written, which is not always in the failing test (but note that those lines are in the 'after failure' fold, and your browser might unhelpfully fold it up before you could take a good look). Thanks for working on this. When I package up git I run the tests under a few different modes, in the case of split index I've been doing: GIT_TEST_SPLIT_INDEX=true GIT_SKIP_TESTS="t3903 t4015.77" Yeah, I noticed that you mentioned this in an unrelated thread the other day, that's why I put you on Cc. ... and that's why I pulled this series from the backburner and cleaned it up for submission. (Gah, most of these commits have an author date back in February...) Since those were the ones I spotted failing under that mode, but I still had occasional other failures, I don't have a record of which, maybe some of these other tests you mention, maybe not. I poked around the Travis CI API, and managed to get the logs of all build jobs that failed with 'GIT_TEST_SPLIT_INDEX=yes' but succeeded without it. Here is the list of failed test scripts, along with how many times they failed: 1 t0027-auto-crlf.sh 1 t0090-cache-tree.sh 1 t3404-rebase-interactive.sh 1 t5520-pull.sh 1 t5573-pull-verify-signatures.sh 1 t5608-clone-2gb.sh 1 t7406-submodule-update.sh 2 t2200-add-update.sh 2 t4002-diff-basic.sh 2 t5504-fetch-receive-strict.sh 3 t0028-working-tree-encoding.sh 4 t1000-read-tree-m-3way.sh 6 t4015-diff-whitespace.sh 10 t4024-diff-optimize-common.sh 17 t3030-merge-recursive.sh 17 t3402-rebase-merge.sh 17 t3501-revert-cherry-pick.sh 17 t6022-merge-rename.sh 17 t6032-merge-large-rename.sh 17 t6034-merge-rename-nocruft.sh 17 t6042-merge-rename-corner-cases.sh 17 t6043-merge-rename-directories.sh 17 t6046-merge-skip-unneeded-updates.sh 17 t7003-filter-branch.sh 17 t7601-merge-pull-config.sh 23 t3903-stash.sh I doubt that this racy split index problem is responsible for all these failures; e.g. I looked at the failures of a few merge-related test scripts, and these logs show that they tend to fail because of memory corruption, i.e. with 'Aborted (core dumped)' or 'Segmentation fault (core dumped)'. To test how this this series improves things, I've been running this on a 56 core CentOS 7.5 machine: while true; do GIT_TEST_SPLIT_INDEX=yes prove -j$(parallel --number-of-cores) t3903-stash.sh t4024-diff-optimize-common.sh t4015-diff-whitespace.sh t2200-add-update.sh t0090-cache-tree.sh && echo "OK $(date) $(git describe)" >>log2 || echo "FAIL $(date) $(git describe)" >>log2; done While, in another window to get some load on the machine (these seem to fail more under load): while true; do prove -j$(parallel --number-of-cores) t[156789]*.sh; done The results with this series applied up to 4/5. I.e. without the actual fix: 92 OK v2.19.0-rc2-6-ged839bd155 8 FAIL v2.19.0-rc2-6-ged839bd155 I.e. when running this 100 times, I got 8 failures. So 8%. Lucky you ;) I could only get t3903 to fail on me, but not the others. That was enough to eventually track down and fix the problem (fun! ;), and then I looked at the logs of failed git/git Travis CI build jobs to see, what other failures might have been caused by it. With this patch applied: 389 OK v2.19.0-rc2-5-g05a5a13935 11 FAIL v2.19.0-rc2-5-g05a5a13935 This time I ran the tests 400 times, and got 11 failures, i.e. a ~2.8% failure rate. I don't have a full account of what stuff failed (this was just scrolling past in my terminal), but most were: t0090-cache-tree.sh (Wstat: 256 Tests: 21 Failed: 3) Failed tests: 10-12 Non-zero exit status: 1 I.e. these tests: ok 10 - commit --interactive gives cache-tree on partial commit ok 11 - commit in child dir has cache-tree ok 12 - reset --hard gives cache-tree But hey, 't0090 --verbose-log -x -i' just failed on me with the fix applied while writing this email, yay! In its logs I see the following errors in all three tests you mention: error: index uses ?½þA extension, which we do not understand fatal: index file corrupt Test 13 then starts with 'rm -f .git/index', and then all is well for the remaining tests. There was a recent discussion about a similar error starting at:
Re: [PATCH v2] config: document value 2 for protocol.version
Jonathan Nieder writes: > Josh Steadmon wrote: > >> From: Brandon Williams >> >> Update the config documentation to note the value `2` as an acceptable >> value for the protocol.version config. >> >> Signed-off-by: Brandon Williams >> Signed-off-by: Josh Steadmon >> --- >> Documentation/config.txt | 2 ++ >> 1 file changed, 2 insertions(+) > > Reviewed-by: Jonathan Nieder > > Thanks. Thanks.
Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes
Johannes Sixt writes:
>> +#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
I think you already have mingw_is_dir_sep() and its shorter alias
is_dir_sep() available to you.
>> +/*
>> + * Does the pathname map to the local named pipe filesystem?
>> + * That is, does it have a "//./pipe/" prefix?
>> + */
>> +static int mingw_is_local_named_pipe_path(const char *filename)
There is no need to prefix mingw_ to this function that is file
local static. Isn't is_local_named_pipe() descriptive and unique
enough?
>> +{
>> +return (IS_SBS(filename[0]) &&
>> +IS_SBS(filename[1]) &&
>> +filename[2] == '.' &&
>> +IS_SBS(filename[3]) &&
>> +!strncasecmp(filename+4, "pipe", 4) &&
>> +IS_SBS(filename[8]) &&
>> +filename[9]);
>> +}
>> +#undef IS_SBS
It is kind-of surprising that there hasn't been any existing need
for a helper function that would allow us to write this function
like so:
static int is_local_named_pipe(const char *path)
{
return path_is_in_directory(path, "//./pipe/");
}
Not a suggestion to add such a thing; as long as we know there is no
other codepath that would benefit from having one, a generalization
like that can and should wait.
>> int mingw_open (const char *filename, int oflags, ...)
>> {
>> typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
>> @@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
>> if (filename && !strcmp(filename, "/dev/null"))
>> filename = "nul";
>> - if (oflags & O_APPEND)
>> +if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
>> open_fn = mingw_open_append;
>> else
>> open_fn = _wopen;
>
> This looks reasonable.
>
> I wonder which part of the code uses local named pipes. Is it
> downstream in Git for Windows or one of the topics in flight?
>
> -- Hannes
[PATCH 1/2] trace: add trace_print_string_list_key
Similar to trace_strbuf or trace_argv_printf, we might want to output
a string list in tracing. Add such a function.
Signed-off-by: Stefan Beller
---
I separated this from the other series, making it into 2 patches:
This first patch adds tracing for string lists and the next patch that
removes the unused function from the string list API.
That way we can decide on these two patches separately if needed.
Documentation/technical/api-trace.txt | 8 ++
trace.c | 39 +++
trace.h | 16 +++
3 files changed, 63 insertions(+)
diff --git a/Documentation/technical/api-trace.txt
b/Documentation/technical/api-trace.txt
index fadb5979c48..ad0ea99d930 100644
--- a/Documentation/technical/api-trace.txt
+++ b/Documentation/technical/api-trace.txt
@@ -62,6 +62,14 @@ Functions
Prints the strbuf, without additional formatting (i.e. doesn't
choke on `%` or even `\0`).
+`void trace_print_string_list_key(struct trace_key *key, const struct
string_list *p, const char *text)::
+
+ Print the string-pointer pairs of the string_list,
+ each one in its own line.
+ It takes an optional key and header argument.
+ If the tracing key is not given, use the default key.
+ The header text is printed before the list itself.
+
`uint64_t getnanotime(void)`::
Returns nanoseconds since the epoch (01/01/1970), typically used
diff --git a/trace.c b/trace.c
index fc623e91fdd..e3a12a092f9 100644
--- a/trace.c
+++ b/trace.c
@@ -176,6 +176,38 @@ void trace_strbuf_fl(const char *file, int line, struct
trace_key *key,
strbuf_release();
}
+void trace_print_string_list_key_fl(const char *file, int line,
+ struct trace_key *key,
+ const struct string_list *p,
+ const char *text)
+{
+ int i, buf_prefix;
+ struct strbuf buf = STRBUF_INIT;
+
+ if (!key)
+ key = _default_key;
+
+ if (!prepare_trace_line(file, line, key, ))
+ return;
+
+ buf_prefix = buf.len;
+
+ if (text) {
+ strbuf_addstr(, text);
+ print_trace_line(key, );
+ }
+
+ for (i = 0; i < p->nr; i++) {
+ strbuf_setlen(, buf_prefix);
+ strbuf_addf(, "%s:%p\n",
+ p->items[i].string,
+ p->items[i].util);
+ print_trace_line(key, );
+ }
+
+ strbuf_release();
+}
+
static void trace_performance_vprintf_fl(const char *file, int line,
uint64_t nanos, const char *format,
va_list ap)
@@ -227,6 +259,13 @@ void trace_strbuf(struct trace_key *key, const struct
strbuf *data)
trace_strbuf_fl(NULL, 0, key, data);
}
+void trace_print_string_list_key(struct trace_key *key,
+const struct string_list *p,
+const char *text)
+{
+ trace_print_string_list_key_fl(NULL, 0, key, p, list);
+}
+
void trace_performance(uint64_t nanos, const char *format, ...)
{
va_list ap;
diff --git a/trace.h b/trace.h
index 2b6a1bc17c2..0e083891e1f 100644
--- a/trace.h
+++ b/trace.h
@@ -37,6 +37,10 @@ extern void trace_argv_printf(const char **argv, const char
*format, ...);
extern void trace_strbuf(struct trace_key *key, const struct strbuf *data);
+extern void trace_print_string_list_key(struct trace_key *key,
+ const struct string_list *p,
+ const char *text);
+
/* Prints elapsed time (in nanoseconds) if GIT_TRACE_PERFORMANCE is enabled. */
__attribute__((format (printf, 2, 3)))
extern void trace_performance(uint64_t nanos, const char *format, ...);
@@ -103,6 +107,13 @@ extern void trace_performance_since(uint64_t start, const
char *format, ...);
trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
} while (0)
+#define trace_print_string_list_key(key, list, text) \
+ do {\
+ if (trace_pass_fl(key)) \
+ trace_print_string_list_key_fl(TRACE_CONTEXT, \
+ __LINE__, key, data);\
+ } while (0)
+
#define trace_performance(nanos, ...) \
do {\
if (trace_pass_fl(_perf_key)) \
@@ -127,6 +138,11 @@ extern void trace_argv_printf_fl(const char *file, int
line, const char **argv,
const char *format, ...);
extern void trace_strbuf_fl(const char *file, int line, struct trace_key *key,
[PATCH 2/2] string-list: remove unused function print_string_list
Signed-off-by: Stefan Beller
---
string-list.c | 10 --
string-list.h | 8
2 files changed, 18 deletions(-)
diff --git a/string-list.c b/string-list.c
index 771c4550980..1f6063f2a27 100644
--- a/string-list.c
+++ b/string-list.c
@@ -195,16 +195,6 @@ void string_list_clear_func(struct string_list *list,
string_list_clear_func_t c
list->nr = list->alloc = 0;
}
-
-void print_string_list(const struct string_list *p, const char *text)
-{
- int i;
- if ( text )
- printf("%s\n", text);
- for (i = 0; i < p->nr; i++)
- printf("%s:%p\n", p->items[i].string, p->items[i].util);
-}
-
struct string_list_item *string_list_append_nodup(struct string_list *list,
char *string)
{
diff --git a/string-list.h b/string-list.h
index ff8f6094a33..18c718c12ce 100644
--- a/string-list.h
+++ b/string-list.h
@@ -113,14 +113,6 @@ typedef int (*string_list_each_func_t)(struct
string_list_item *, void *);
void filter_string_list(struct string_list *list, int free_util,
string_list_each_func_t want, void *cb_data);
-/**
- * Dump a string_list to stdout, useful mainly for debugging
- * purposes. It can take an optional header argument and it writes out
- * the string-pointer pairs of the string_list, each one in its own
- * line.
- */
-void print_string_list(const struct string_list *p, const char *text);
-
/**
* Free a string_list. The `string` pointer of the items will be freed
* in case the `strdup_strings` member of the string_list is set. The
--
2.19.0.rc2.392.g5ba43deb5a-goog
Re: [PATCH v2] config: document value 2 for protocol.version
Josh Steadmon wrote: > From: Brandon Williams > > Update the config documentation to note the value `2` as an acceptable > value for the protocol.version config. > > Signed-off-by: Brandon Williams > Signed-off-by: Josh Steadmon > --- > Documentation/config.txt | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Jonathan Nieder Thanks.
Re: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
Hi,
Max Kirillov wrote:
> From: Jeff King
> Subject: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
micronit: s/Treat/treat/
> There is no known case where empty body it used by a server as
> instruction to read until EOF, so there is no need to violate the RFC.
> Make get_content_length() return 0 in this case.
>
> Currently there is no practical difference, as the GET request
> where it can be empty is handled without actual reading the body
> (in get_info_refs() function), but it is better to stick to the correct
> behavior.
>
> Signed-off-by: Max Kirillov
> ---
> The incremental. Hopefully I described the reason right. Needs "signed-off-by"
Thanks. I am wondering if we should go all the way and do
ssize_t val;
const char *str = getenv("CONTENT_LENGTH");
if (!str || !*str)
return 0;
if (!git_parse_ssize_t(str, ))
die(...);
return val;
That would match the RFC, but it seems to make t5510-fetch.sh hang,
right after
ok 165 - --negotiation-tip understands abbreviated SHA-1
When I run with -v -i -x, it stalls at
++ git -C '/usr/local/google/home/jrn/src/git/t/trash
directory.t5510-fetch/httpd/www/server' tag -d alpha_1 alpha_2 beta_1 beta_2
Deleted tag 'alpha_1' (was a84e4a9)
Deleted tag 'alpha_2' (was 7dd5cf4)
Deleted tag 'beta_1' (was bcb5c65)
Deleted tag 'beta_2' (was d3b6dcd)
+++ pwd
++ GIT_TRACE_PACKET='/usr/local/google/home/jrn/src/git/t/trash
directory.t5510-fetch/trace'
++ git -C client fetch --negotiation-tip=alpha_1 --negotiation-tip=beta_1
origin alpha_s beta_s
Do you know why?
Thanks,
Jonathan
[PATCH v2] config: document value 2 for protocol.version
From: Brandon Williams Update the config documentation to note the value `2` as an acceptable value for the protocol.version config. Signed-off-by: Brandon Williams Signed-off-by: Josh Steadmon --- Documentation/config.txt | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index eb66a1197..ee3b5dd8e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2828,6 +2828,8 @@ protocol.version:: * `1` - the original wire protocol with the addition of a version string in the initial response from the server. +* `2` - link:technical/protocol-v2.html[wire protocol version 2]. + -- pull.ff:: -- 2.19.0.rc2.392.g5ba43deb5a-goog
[PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
From: Jeff King
Subject: [PATCH] http-backend: Treat empty CONTENT_LENGTH as zero
There is no known case where empty body it used by a server as
instruction to read until EOF, so there is no need to violate the RFC.
Make get_content_length() return 0 in this case.
Currently there is no practical difference, as the GET request
where it can be empty is handled without actual reading the body
(in get_info_refs() function), but it is better to stick to the correct
behavior.
Signed-off-by: Max Kirillov
---
The incremental. Hopefully I described the reason right. Needs "signed-off-by"
http-backend.c | 24 ++--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/http-backend.c b/http-backend.c
index 458642ef72..ea36a52118 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -353,8 +353,28 @@ static ssize_t get_content_length(void)
ssize_t val = -1;
const char *str = getenv("CONTENT_LENGTH");
- if (str && *str && !git_parse_ssize_t(str, ))
- die("failed to parse CONTENT_LENGTH: %s", str);
+ if (!str) {
+ /*
+* RFC3875 says this must mean "no body", but in practice we
+* receive chunked encodings with no CONTENT_LENGTH. Tell the
+* caller to read until EOF.
+*/
+ val = -1;
+ } else if (!*str) {
+ /*
+* An empty length should be treated as "no body" according to
+* RFC3875, and this seems to hold in practice.
+*/
+ val = 0;
+ } else {
+ /*
+* We have a non-empty CONTENT_LENGTH; trust what's in it as
long
+* as it can be parsed.
+*/
+ if (!git_parse_ssize_t(str, ))
+ die("failed to parse CONTENT_LENGTH: '%s'", str);
+ }
+
return val;
}
--
2.17.0.1185.g782057d875
Re: [PATCH v3] http-backend: allow empty CONTENT_LENGTH
On Sun, Sep 09, 2018 at 10:17:48PM -0700, Jonathan Nieder wrote: > From: Max Kirillov > Subject: http-backend test: make empty CONTENT_LENGTH test more realistic Thank you, yes, this is what should have left
Re: [PATCH v2 10/11] builtin rebase: only store fully-qualified refs in `options.head_name`
On Mon, Sep 10, 2018 at 09:55:30AM -0700, Junio C Hamano wrote:
> SZEDER Gábor writes:
>
> >>} else {
> >> - options.head_name = xstrdup("detached HEAD");
> >> + free(options.head_name);
> >> + options.head_name = NULL;
> >
> > Please use FREE_AND_NULL(options.head_name) here.
>
> Good; did contrib/coccinelle/free.cocci catch this?
Yes.
But now that you mention it, I see that it didn't catch it in 'pu' or
in most of the later 'pk/rebase-in-c-X-...' branches, even though they
all have these lines. It bisects to 0073df2bd3 (builtin rebase:
support --rebase-merges[=[no-]rebase-cousins], 2018-09-04), which
doesn't touch these lines at all.
Strange; I have no idea what's going on.
Re: git silently ignores include directive with single quotes
Stas Bekman writes: > [include] > path = .gitconfig > > Not realizing that the two were not in the same folder. And probably > assuming that .git/config was referring to the root of repository, and > not relative to .git/, which is a reasonable assumption. > > Of course he had no way of resolving this as git wasn't telling him > where it wasn't finding the file. i.e. > > Can't find: ~/myrepo/.git/.gitconfig > > which would have instantly told him where the problem was. Yeah, I think Peff's idea of introducing a variant of include.path that reports missing file would make sense for such a use case. The code needs to turn a relative path to an absolute one by taking the value as path relative to the including configuration file, so we should already have the path to use in the error reporting, if we were to go that route.
Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes
On 9/10/2018 3:45 PM, Johannes Sixt wrote:
Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget:
diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..f87376b26a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
return ret;
}
+/*
+ * Calling CreateFile() using FILE_APPEND_DATA and without
FILE_WRITE_DATA
+ * is documented in [1] as opening a writable file handle in append
mode.
+ * (It is believed that) this is atomic since it is maintained by the
+ * kernel unlike the O_APPEND flag which is racily maintained by the
CRT.
+ *
+ * [1]
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
+ *
+ * This trick does not appear to work for named pipes. Instead it
creates
+ * a named pipe client handle that cannot be written to. Callers should
+ * just use the regular _wopen() for them. (And since client handle
gets
+ * bound to a unique server handle, it isn't really an issue.)
+ */
static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
{
HANDLE handle;
@@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const
*wfilename, int oflags, ...)
NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE)
return errno = err_win_to_posix(GetLastError()), -1;
+
/*
* No O_APPEND here, because the CRT uses it only to reset the
- * file pointer to EOF on write(); but that is not necessary
- * for a file created with FILE_APPEND_DATA.
+ * file pointer to EOF before each write(); but that is not
+ * necessary (and may lead to races) for a file created with
+ * FILE_APPEND_DATA.
*/
fd = _open_osfhandle((intptr_t)handle, O_BINARY);
if (fd < 0)
@@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const
*wfilename, int oflags, ...)
return fd;
}
+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
+/*
+ * Does the pathname map to the local named pipe filesystem?
+ * That is, does it have a "//./pipe/" prefix?
+ */
+static int mingw_is_local_named_pipe_path(const char *filename)
+{
+ return (IS_SBS(filename[0]) &&
+ IS_SBS(filename[1]) &&
+ filename[2] == '.' &&
+ IS_SBS(filename[3]) &&
+ !strncasecmp(filename+4, "pipe", 4) &&
+ IS_SBS(filename[8]) &&
+ filename[9]);
+}
+#undef IS_SBS
+
int mingw_open (const char *filename, int oflags, ...)
{
typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags,
...);
@@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags,
...)
if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
- if (oflags & O_APPEND)
+ if ((oflags & O_APPEND) &&
!mingw_is_local_named_pipe_path(filename))
open_fn = mingw_open_append;
else
open_fn = _wopen;
This looks reasonable.
Thanks for the review.
I wonder which part of the code uses local named pipes. Is it downstream
in Git for Windows or one of the topics in flight?
-- Hannes
I'm wanting to use them as a tracing target option in my trace2 series
currently in progress.
Jeff
Re: git silently ignores include directive with single quotes
To add another report of a similar problem, of silent skipping and not of filepath quoting, I found this one: https://stackoverflow.com/questions/31203634/git-clean-filter-python-script-on-windows/52264440#52264440 The user created .gitconfig and added to .git/config: [include] path = .gitconfig Not realizing that the two were not in the same folder. And probably assuming that .git/config was referring to the root of repository, and not relative to .git/, which is a reasonable assumption. Of course he had no way of resolving this as git wasn't telling him where it wasn't finding the file. i.e. Can't find: ~/myrepo/.git/.gitconfig which would have instantly told him where the problem was. -- Stas Bekman <'>< <'>< https://stasosphere.com https://chestofbooks.com https://experientialsexlab.com https://stason.org https://stasosphere.com/experience-life/my-books
Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes
Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget:
diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..f87376b26a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
return ret;
}
+/*
+ * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA
+ * is documented in [1] as opening a writable file handle in append mode.
+ * (It is believed that) this is atomic since it is maintained by the
+ * kernel unlike the O_APPEND flag which is racily maintained by the CRT.
+ *
+ * [1]
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
+ *
+ * This trick does not appear to work for named pipes. Instead it creates
+ * a named pipe client handle that cannot be written to. Callers should
+ * just use the regular _wopen() for them. (And since client handle gets
+ * bound to a unique server handle, it isn't really an issue.)
+ */
static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
{
HANDLE handle;
@@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename,
int oflags, ...)
NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE)
return errno = err_win_to_posix(GetLastError()), -1;
+
/*
* No O_APPEND here, because the CRT uses it only to reset the
-* file pointer to EOF on write(); but that is not necessary
-* for a file created with FILE_APPEND_DATA.
+* file pointer to EOF before each write(); but that is not
+* necessary (and may lead to races) for a file created with
+* FILE_APPEND_DATA.
*/
fd = _open_osfhandle((intptr_t)handle, O_BINARY);
if (fd < 0)
@@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int
oflags, ...)
return fd;
}
+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
+/*
+ * Does the pathname map to the local named pipe filesystem?
+ * That is, does it have a "//./pipe/" prefix?
+ */
+static int mingw_is_local_named_pipe_path(const char *filename)
+{
+ return (IS_SBS(filename[0]) &&
+ IS_SBS(filename[1]) &&
+ filename[2] == '.' &&
+ IS_SBS(filename[3]) &&
+ !strncasecmp(filename+4, "pipe", 4) &&
+ IS_SBS(filename[8]) &&
+ filename[9]);
+}
+#undef IS_SBS
+
int mingw_open (const char *filename, int oflags, ...)
{
typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
@@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
- if (oflags & O_APPEND)
+ if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
open_fn = mingw_open_append;
else
open_fn = _wopen;
This looks reasonable.
I wonder which part of the code uses local named pipes. Is it downstream
in Git for Windows or one of the topics in flight?
-- Hannes
Re: [PATCH v1] git-mv: allow submodules and fsmonitor to work together
On 9/10/2018 1:07 PM, Stefan Beller wrote:
On Mon, Sep 10, 2018 at 9:29 AM Ben Peart wrote:
It was reported that
GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
breaks as the fsmonitor data is out of sync with the state of the .gitmodules
file. Update is_staging_gitmodules_ok() so that it no longer tells
ie_match_stat() to ignore refreshing the fsmonitor data.
Wondering how this came to be,
7da9aba4178 (submodule: used correct index in is_staging_gitmodules_ok,
2017-12-12) last touched this line, but is unrelated as the fsmonitor
behavior was
there before.
Before that, we have 883e248b8a0 (fsmonitor: teach git to optionally utilize a
file system monitor to speed up detecting new or changed files., 2017-09-22)
that was written by you, who knows the fsmonitor better than I do (or Brandon
who wrote the commit referenced above).
Looking through the archive, it seems that we might have more such hidden
gems?
Fortunately, the only one left is the one in preload_index() which is
what the flag was created to handle so I think we're ok.
https://public-inbox.org/git/f50825a4-fa15-9f28-a079-853e78ee8...@gmail.com/
Anyway, I think this is a better fix than what I proposed for sure.
Thanks for looking into this!
Stefan
Reported-by: Ævar Arnfjörð Bjarmason
Helped-by: Stefan Beller
Signed-off-by: Ben Peart
---
Notes:
Base Ref: v2.19.0-rc2
Web-Diff: https://github.com/benpeart/git/commit/ed30e1a885
Checkout: git fetch https://github.com/benpeart/git fsmonitor-t7411-v1 &&
git checkout ed30e1a885
submodule.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index 50cbf5f13e..1e7194af28 100644
--- a/submodule.c
+++ b/submodule.c
@@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
if ((pos >= 0) && (pos < istate->cache_nr)) {
struct stat st;
if (lstat(GITMODULES_FILE, ) == 0 &&
- ie_match_stat(istate, istate->cache[pos], ,
- CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
+ ie_match_stat(istate, istate->cache[pos], , 0) &
DATA_CHANGED)
return 0;
}
base-commit: c05048d43925ab8edcb36663752c2b4541911231
--
2.18.0.windows.1
Re: [PATCH] t3701-add-interactive: tighten the check of trace output
On Mon, Sep 10, 2018 at 09:19:32PM +0200, SZEDER Gábor wrote: > > I'd have thought that anybody > > adding index-specific tracing would do it as GIT_TRACE_INDEX. > > Depends on the purpose, I guess. For tracing that is aimed to become > part of in git, definitely. However, for my own ad-hoc tracing used > to try to make sense of some split-index corner cases, trace_printf() > is perfect. I don't mind it as a temporary debug thing, but it would be nice if it wasn't a temptation for people to include in their actual patches. -Peff
Re: [PATCH v3 14/23] patch-ids.c: remove implicit dependency on the_index
Stefan Beller writes: > On Sun, Sep 9, 2018 at 1:54 AM Nguyễn Thái Ngọc Duy wrote: >> >> --- > > Junio would have to forge your Sign off here? > (I just realize this holds true for the whole series, > but it occurred to me in this patch, as I was looking at > the diff_setup[_done] part on the previous round. That is fine; as I said elsewhere, I am treating patches, other than regression fixes, sent to the list during prerelease freeze as mere "test data" for the 2.19-pre version of Git, and I may end up keeping the result of testing "git am" on them as topic branches, but there is no promise I'd do so. Reading patches on the list and commenting on them can be done without sign-offs; you can treat them as an early draft that is not meant for the final application. I may come back to leftover patches in the list archive after the release is made, but honestly I'd prefer to see them sent afresh after the final for the next round, taking input from the review comments as necessary.
Re: [PATCH 0/4] un-breaking pack-objects with bitmaps
Jeff King writes: >> Either is fine. I am not moving 'next' beyond what is necessary for >> this release cycle during the pre-release freeze period, and because >> I thought that Peff was in favor of squashing in the changes to the >> original when the next cycle starts, I haven't bothered to merge the >> fix there yet. > > I think Ævar's point is just that the current tip of next is badly > broken if you use bitmaps, and it's worth hot-fixing that in the > meantime. I know that ;-) My point is that anybody who *needs* handholding from the upstream project (i.e. not Ævar, but those whom he is trying to help with the suggestion) should not be looking at 'next' during the prerelease freeze. They should be looking at 'master' instead, and that is why I am not spending more than minimum cycles of my own worrying about 'next' during the prerelease. In any case, I think the final is ready to go, almost. I still need to do the preformatted docs, though.
Re: [PATCH] t3701-add-interactive: tighten the check of trace output
On Mon, Sep 10, 2018 at 11:44:54AM -0400, Jeff King wrote: > On Mon, Sep 10, 2018 at 04:07:14PM +0200, SZEDER Gábor wrote: > > > The test 'add -p does not expand argument lists' in > > 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do > > not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE > > of 'git add -p' to ensure that the name of a tracked file wasn't > > passed around as argument to any of the commands executed as a result > > of undesired pathspec expansion. This check is done with 'grep' using > > the filename on its own as the pattern, which is too loose a pattern, > > and would match any occurrences of the filename in the trace output, > > not just those as command arguments. E.g. if a developer were to > > litter the index handling code with trace_printf()s printing, among > > other things, the name of the just processed cache entry, then that > > pattern would mistakenly match these as well, and would fail the test. > > Is this a real thing we're running into? Well, we, in general, don't, but that example mentioned in the commit message does contain autobiographical elements :) > I'd have thought that anybody > adding index-specific tracing would do it as GIT_TRACE_INDEX. Depends on the purpose, I guess. For tracing that is aimed to become part of in git, definitely. However, for my own ad-hoc tracing used to try to make sense of some split-index corner cases, trace_printf() is perfect. > It's > unfortunate that "trace commands and processes" is just GIT_TRACE, and not > GIT_TRACE_RUN or similar. But that's mostly historical. I wouldn't > expect people to add other subsystems to it. > > Not that I'm totally opposed to your patch, but it's a little sad that > we have to match the specific text used in GIT_TRACE now (and if they > ever changed we won't even notice, but rather the test will just become > a silent noop). > > I think it would be nice if we could move towards something like: > > - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar > > - abolish trace_printf() without a specific subsystem key Nah, please leave trace_printf() alone. > - do one of: > > - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that > keeps things working as they are now > > - have GIT_TRACE enable _all_ tracing; that's a change in behavior, > but arguably a more useful thing to have going forward (e.g., when > you're not sure which traces are even available) > > And then a test like this would just use GIT_TRACE_COMMAND. Except for removing keyless trace_printf(), I agree.
Re: git silently ignores include directive with single quotes
Jonathan Nieder writes: > Updated proposal: > > 1. Treat strings starting or ending with single-quote as worth > quoting in config.c::write_pair (line 3269). This would already > help with the original issue, since the config would say > > [foo] > bar = "'baz'" > > allowing a quick diagnosis. This does not change anything essential from Git's point of view since the previous round. But I changed my mind anyway ;-) Earlier I said "if we do not intend to make sq special, we shouldn't do #1", and it still is a good direction to go. But making sq special does not have to be making sq a character that quotes. A character (or a character sequence) that is *not* quoting can still be special---for example, we can say certain character or a character sequence *must* be quoted, and make sq such a character. That is, even if we stop at your step 3. or step 2., and did not go to step 4. (which I think is a bad idea for little gain), we are already treating sq as a special character, and step 1. above is a reasonable way to start the transition to that better world. The reason why it is a better world that have "must be quoted, even though they are not quoting characters themselves" is solely because that would avoid confusion for those who are not familiar with the file format, even when we stop at step #2. In addition to a single quote a the beginning of the value, I think two or more SP deserve to be such a "must be quoted" sequence, i.e. instead of producing this result, which we see with today's Git: $ git config a.test0 "'foo" $ git config a.test1 "foo bar" ;# two spaces $ grep -A2 '\[a\]' config [a] test0 = 'foo test1 = foo bar we'd produce $ grep -A2 '\[a\]' config [a] test0 = "'foo" test1 = "foo bar" but we can still interpret what we have historically written the same way. I do not know if step #3 is a good idea, and I do not think step #2 is particularly a good stopping point. Step #1 is probably slightly a better stopping point if the aim is to avoid user confusion than step #2. > 2. (optional) Warn if a value is surrounded in single-quotes, > encouraging using surrounding double-quotes to disambiguate. > > 3. (optional) Error out if a value is surrounded in single-quotes, > encouraging replacing with or surrounding with double-quote, > depending on the user's intention. > > 4. (optional) Start treating wrapping single-quotes specially > somehow. > > As before, I think step 1 is a good idea, but I'm not convinced about > any of the later steps. > > Thanks, > Jonathan
Re: [PATCH v3 14/23] patch-ids.c: remove implicit dependency on the_index
On Sun, Sep 9, 2018 at 1:54 AM Nguyễn Thái Ngọc Duy wrote: > > --- Junio would have to forge your Sign off here? (I just realize this holds true for the whole series, but it occurred to me in this patch, as I was looking at the diff_setup[_done] part on the previous round.
Re: [PATCH 12/21] patch-ids.c: remove implicit dependency on the_index
On Sun, Sep 9, 2018 at 1:02 AM Duy Nguyen wrote:
>
> On Tue, Sep 4, 2018 at 9:54 PM Stefan Beller wrote:
> >
> > On Mon, Sep 3, 2018 at 11:03 AM Duy Nguyen wrote:
> > >
> > > On Mon, Aug 27, 2018 at 9:13 PM Stefan Beller wrote:
> > > >
> > > > > -int init_patch_ids(struct patch_ids *ids)
> > > > > +int init_patch_ids(struct patch_ids *ids, struct repository *repo)
> > > > > {
> > > > > memset(ids, 0, sizeof(*ids));
> > > > > - diff_setup(>diffopts, the_repository);
> > > > > + diff_setup(>diffopts, repo);
> > > >
> > > > Just realized when looking at this diff, though it applies to
> > > > other patches as well. (and reading Documentation/technical/api-diff.txt
> > > > confirms my thinking IMHO)
> > > >
> > > > What makes the repository argument any special compared
> > > > to the rest of the diff options?
> > > >
> > > > So I would expect the setup to look like
> > > >
> > > > memset(ids, 0, sizeof(*ids));
> > > > ids->diffopts->repo = the_repository;
> > > > diff_setup(>diffopts);
> > > >
> > > > here and in diff_setup, we'd have
> > > >
> > > > if (!options->repo)
> > > > options->repo = the_repository;
> > > >
> > > > or even put the_repository into default_diff_options,
> > > > but then I wonder how this deals with no-repo invocations
> > > > (git diff --no-index examples for bug reports)
> > >
> > > That makes "repo" field optional and I'm very much against falling
> > > back to the_repository. revisions.c in the end does not have any
> > > the_repository reference, and it's actually undefined for most files.
> > > This makes accidentally adding the_repository back much more
> > > difficult.
> >
> > Thanks for the clear explanation. I agree that this is a good approach
> > with these reasons given. So in case a resend is needed, maybe add
> > these to the commit message, as it explains why we deviate from
> > the pattern here.
>
> Actually I looked at it again and diff_setup() clears diffopts as the
> first step, so any assignments prior to the diff_setup() call has no
> effect.
Eh, I botched that code in email, I meant to put it between
diff_setup();
diffopts.repo =
diff_setup_done();
but I still agree with you that we want to have the repository as a
non-optional field.
>
> > > Yes the --no-index stuff will have to be taken care of at some point,
> > > but I think for now we could just put "struct repository *" in place
> > > first to see what it looks like, then go from there.
> >
> > I would think repo = NULL would do? But we can defer this
> > discussion to later.
>
> Yes. But the the_hash_algo currently points to
> the_repository->hash_algo. We need to at least make the_hash_algo
> independent from 'struct repository *', then figure out how to set
> the_hash_algo without $GIT_DIR/config file...
That sounds as if it is unwise to rely on the_hash_algo and
rather prefer the repositories configured hash algorithm instead;
only falling back to the_hash_algo when we have no repository
handle available.
Stefan
Re: [PATCH 0/4] un-breaking pack-objects with bitmaps
On Mon, Sep 10, 2018 at 09:53:56AM -0700, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason writes: > > > I'm just reverting jk/pack-delta-reuse-with-bitmap out of next when > > building my own package of git, but I think this really should be fixed > > in that branch, either by merging the fix down or reverting the original > > series out of next, I think just merging the fix down makes sense, but > > have no strong opinion on it. > > Either is fine. I am not moving 'next' beyond what is necessary for > this release cycle during the pre-release freeze period, and because > I thought that Peff was in favor of squashing in the changes to the > original when the next cycle starts, I haven't bothered to merge the > fix there yet. I think Ævar's point is just that the current tip of next is badly broken if you use bitmaps, and it's worth hot-fixing that in the meantime. After the release, I'm OK with either one of squashing that first patch in, or just merging as-is. -Peff
Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH
On Mon, Sep 10, 2018 at 09:37:20AM -0700, Junio C Hamano wrote: > Jeff King writes: > > > But that couldn't have been what older versions were doing, since they > > never looked at CONTENT_LENGTH at all, and instead always read to EOF. > > So presumably the original problem wasn't that we tried to read a body, > > but that the empty string caused git_parse_ssize_t to report failure, > > and we called die(). Which probably should be explained by 574c513e8d > > (http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too > > late for that. > > > > So after that patch, we really do have the original behavior, and that's > > enough for v2.19. > > To recap to make sure I am following it correctly: > > - pay attention to content-length when it is clearly given with a >byte count, which is an improvement over v2.18 > > - mimick what we have been doing until now when content-length is >missing or set to an empty string, so we are regression free and >bug-to-bug compatible relative to v2.18 in these two cases. That (and what you wrote below) matches my current understanding, too. Though I did already admit to being confused. ;) -Peff
Re: [Possible GIT Bug]
"Eckhard Maaß" writes: > On Mon, Sep 10, 2018 at 09:03:10AM -0700, Junio C Hamano wrote: >> It is neither but if I have to pick one between the two, it is much >> closer to the former than the latter. The primary source of this is >> that we have only *one* pathspec given to the diff machinery, but in >> order to implement your ideal "find harder", you'd need *two*. That >> is, one set of paths for which you are interested in their origin, >> and the other set that you allow the machinery to consider as possible >> origins. Since we can only give one pathspec machinery, that one >> pathspec is used to specify both of these sets. > > How does tihs compare to `--follow`? With that knob active the machinery > indeed uses the whole repository for finding renames and/or copies. Is > this the only exception then? '--follow' is a checkbox hack that is not even properly integrated with the diff machinery (it only exists on the "log" side of the tool), so I do not think it is productive to find a comparison.
Re: git silently ignores include directive with single quotes
Junio C Hamano wrote: > Jonathan Nieder writes: >> 1. Treat single-quote as worth quoting in config.c::write_pair (line >> 2516). This would already help with the original issue, since the >> config would say >> >> [foo] >> bar = \'baz\' >> >> allowing a quick diagnosis. > > I am mildly against this, as long as you feel that all the remaining > steps need to be marked with "(optional)", because this will give > readers an impression that somehow single-quote is special. If we > do not intend to make it special at all, we shouldn't. That's fair, especially because it would be inconsistent with shell command language, where single-quote inside double quotes is not special: $ printf '%s\n' "\'" \' (I realize that backslash means something different in Git config; I'm just saying it would be another source of cognitive dissonance.) Updated proposal: 1. Treat strings starting or ending with single-quote as worth quoting in config.c::write_pair (line 3269). This would already help with the original issue, since the config would say [foo] bar = "'baz'" allowing a quick diagnosis. 2. (optional) Warn if a value is surrounded in single-quotes, encouraging using surrounding double-quotes to disambiguate. 3. (optional) Error out if a value is surrounded in single-quotes, encouraging replacing with or surrounding with double-quote, depending on the user's intention. 4. (optional) Start treating wrapping single-quotes specially somehow. As before, I think step 1 is a good idea, but I'm not convinced about any of the later steps. Thanks, Jonathan
Re: [PATCH v3 00/23] Kill the_index part 4
On Sun, Sep 9, 2018 at 1:54 AM Nguyễn Thái Ngọc Duy wrote: > > The last patch 24/24 has been merged into individual patches to add > repo_ prefix and avoid breaking in-flight topics. "repo" argument is > also moved up in rerere(), diff_setup() and init_revisions(). yay! So eventually we'd want to rename back to short names or we just postpone that decision if we want that in the future? > diff --git a/Documentation/technical/api-revision-walking.txt > b/Documentation/technical/api-revision-walking.txt > index 55b878ade8..83e62533a0 100644 > --- a/Documentation/technical/api-revision-walking.txt > +++ b/Documentation/technical/api-revision-walking.txt > @@ -15,7 +15,7 @@ revision list. > Functions > - > > -`init_revisions`:: > +`repo_init_revisions`:: > > Initialize a rev_info structure with default values. The second > parameter may be NULL or can be prefix path, and then the `.prefix` > diff --git a/bisect.c b/bisect.c > index 560493acd2..6ae5e5b49e 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -632,7 +632,7 @@ static void bisect_rev_setup(struct rev_info *revs, const > char *prefix, > struct argv_array rev_argv = ARGV_ARRAY_INIT; > int i; > > - repo_init_revisions(revs, the_repository, prefix); > + repo_init_revisions(the_repository, revs, prefix); Apparently I did not communicate clearly enough on the last round, Please note that the documentation is wrong with these patches applied: `repo_init_revisions`:: [...] The second parameter may be NULL or can be prefix path, and then the `.prefix` but the prefix parameter is the _third_ parameter in either order.
Re: git silently ignores include directive with single quotes
Jonathan Nieder writes: > 1. Treat single-quote as worth quoting in config.c::write_pair (line > 2516). This would already help with the original issue, since the > config would say > > [foo] > bar = \'baz\' > > allowing a quick diagnosis. I am mildly against this, as long as you feel that all the remaining steps need to be marked with "(optional)", because this will give readers an impression that somehow single-quote is special. If we do not intend to make it special at all, we shouldn't. If we do commit to make it special, then this is a very sensible first step that is backward compatible, of course, and I find that the following steps form a reasonable transition plan, if we intend to follow all the way through. > 2. (optional) Warn if a value is surrounded in single-quotes, > encouraging using backslash to disambiguate. > > 3. (optional) Error out if a value is surrounded in single-quotes, > encouraging using double-quote or backslash, depending on the > user's intention. > > 4. (optional) Start treating wrapping single-quotes specially > somehow. > > I think step 1 is a good idea, but I'm not convinced about any of the > later steps. > > I also agree with the comments upthread about wanting a way to do a > '[include] path' that errors out if the file doesn't exist, and maybe > even starting a transition to repurpose standard [include] path to do > that.
Re: [GIT PULL] l10n updates for 2.19.0 round 2
Jiang Xin writes: > Hi Junio, > > The following changes since commit 2f743933341f27603550fbf383a34dfcfd38: > > Git 2.19-rc1 (2018-08-28 12:01:01 -0700) > > are available in the Git repository at: > > git://github.com/git-l10n/git-po tags/l10n-2.19.0-rnd2 > > for you to fetch changes up to c1ac5258dccbb62438c8df73d728271f7a316c99: > > l10n: zh_CN: for git v2.19.0 l10n round 1 to 2 (2018-09-09 22:38:39 +0800) > > > l10n for Git 2.19.0 round 2 Thanks.
Re: [Possible GIT Bug]
On Mon, Sep 10, 2018 at 09:03:10AM -0700, Junio C Hamano wrote: > It is neither but if I have to pick one between the two, it is much > closer to the former than the latter. The primary source of this is > that we have only *one* pathspec given to the diff machinery, but in > order to implement your ideal "find harder", you'd need *two*. That > is, one set of paths for which you are interested in their origin, > and the other set that you allow the machinery to consider as possible > origins. Since we can only give one pathspec machinery, that one > pathspec is used to specify both of these sets. How does tihs compare to `--follow`? With that knob active the machinery indeed uses the whole repository for finding renames and/or copies. Is this the only exception then? Take care, Eckhard
Re: [PATCH] t3701-add-interactive: tighten the check of trace output
Jeff King writes:
> Not that I'm totally opposed to your patch, but it's a little sad that
> we have to match the specific text used in GIT_TRACE now (and if they
> ever changed we won't even notice, but rather the test will just become
> a silent noop).
>
> I think it would be nice if we could move towards something like:
>
> - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar
>
> - abolish trace_printf() without a specific subsystem key
>
> - do one of:
>
> - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that
> keeps things working as they are now
>
> - have GIT_TRACE enable _all_ tracing; that's a change in behavior,
> but arguably a more useful thing to have going forward (e.g., when
> you're not sure which traces are even available)
>
> And then a test like this would just use GIT_TRACE_COMMAND.
Yup. Sounds like a better world.
>
>> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
>> index 609fbfdc31..65dfbc033a 100755
>> --- a/t/t3701-add-interactive.sh
>> +++ b/t/t3701-add-interactive.sh
>> @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument
>> lists' '
>> # update it, but we want to be sure that our "." pathspec
>> # was not expanded into the argument list of any command.
>> # So look only for "not-changed".
>> -! grep not-changed trace.out
>> +! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out
>
> I had a vague recollection that we preferred "egrep" to "grep -E" due to
> portability. But digging in the history, I could only find "fgrep" over
> "grep -F". And we seem to have plenty of "grep -E" invocations already.
I had the same reaction and came to the same conclusion. FWIW, the
"favor fgrep over -F" was done by you with 87539416 ("tests: grep
portability fixes", 2008-09-30).
Re: [PATCH v6 17/21] range-diff: populate the man page
Ævar Arnfjörð Bjarmason writes: > Looking at the git-range-diff manpage though it recommends > over ... when the topic has been rebased, which is > usually the case for e.g. a topic that's submitted to git.git (usually > be the time feedback has been gathered & a re-submission has been made > Junio has pushed another "master"). > > So isn't " " the right thing to use over > "..." for git.git use? I think so, but I'm not sure. If is forked from different base than where was forked, thenwould give you more sensible range. And such an update is inevitable when must rely on new things that recently appeared on since forked from the mainline. But otherwise ... should work just fine. > In any case, there are going to be those use-case where you should be > using " ", and a rebase will be propagated by a > force-push, so I thought it made sense that range-diff could directly > consume the output of "fetch" in that case... I am not absolutely sure if there is *more* useful interpretation that " ..." may want to mean than to serve as a synonym for " " for those who are too lazy to type. But if there isn't, I'd say it is a reasonable synonym to want.
Re: git silently ignores include directive with single quotes
Hi, Junio C Hamano wrote: > Jeff King writes: >> So I think it's been covered elsewhere that single quotes aren't a thing >> in git's config format. I will say that this was actually a minor >> surprise to me, after a decade of working with the format. ;) >> >> I don't know if it's worth changing now or not It would be >> backwards-incompatible, but I wonder if we could do it in a sane way. >> E.g., with a rule like: >> >> - if the first non-whitespace character of the value is a >> single-quote, assume the value is quoted and apply normal shell >> rules (i.e., no backslash escapes until the ending single-quote) >> >> - otherwise, single-quotes are not special at all > > At least the rule would not force those with ' in the middle of > their family names to surround the user.name with extra double > quotes, and it would probably be a good and safe practical solution. > Being safe "by magic" tend to become hard to explain, but in this > case the magic part is probably still simple enough. Given that today, git config foo.bar "'baz'" produces [foo] bar = 'baz' I don't think this would be safe to do. Since the underlying problem is that the latter syntax is confusing, I wonder if we can do the following: 1. Treat single-quote as worth quoting in config.c::write_pair (line 2516). This would already help with the original issue, since the config would say [foo] bar = \'baz\' allowing a quick diagnosis. 2. (optional) Warn if a value is surrounded in single-quotes, encouraging using backslash to disambiguate. 3. (optional) Error out if a value is surrounded in single-quotes, encouraging using double-quote or backslash, depending on the user's intention. 4. (optional) Start treating wrapping single-quotes specially somehow. I think step 1 is a good idea, but I'm not convinced about any of the later steps. I also agree with the comments upthread about wanting a way to do a '[include] path' that errors out if the file doesn't exist, and maybe even starting a transition to repurpose standard [include] path to do that. Thanks, Jonathan
Re: [PATCH] Revert "Merge branch 'sb/submodule-core-worktree'" (was Re: Old submodules broken in 2.19rc1 and 2.19rc2)
> >> Like this (generated using "git revert -m1)? > > > > OK. Thanks for taking care of it. Yes that looks good to me, thanks! > > Please don't forget to remove the corresponding release notes entry. Makes sense, too. Thanks, Stefan
Re: [PATCH v1] git-mv: allow submodules and fsmonitor to work together
On Mon, Sep 10, 2018 at 9:29 AM Ben Peart wrote:
>
> It was reported that
>
>GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
>
> breaks as the fsmonitor data is out of sync with the state of the .gitmodules
> file. Update is_staging_gitmodules_ok() so that it no longer tells
> ie_match_stat() to ignore refreshing the fsmonitor data.
Wondering how this came to be,
7da9aba4178 (submodule: used correct index in is_staging_gitmodules_ok,
2017-12-12) last touched this line, but is unrelated as the fsmonitor
behavior was
there before.
Before that, we have 883e248b8a0 (fsmonitor: teach git to optionally utilize a
file system monitor to speed up detecting new or changed files., 2017-09-22)
that was written by you, who knows the fsmonitor better than I do (or Brandon
who wrote the commit referenced above).
Looking through the archive, it seems that we might have more such hidden
gems?
https://public-inbox.org/git/f50825a4-fa15-9f28-a079-853e78ee8...@gmail.com/
Anyway, I think this is a better fix than what I proposed for sure.
Thanks for looking into this!
Stefan
>
> Reported-by: Ævar Arnfjörð Bjarmason
> Helped-by: Stefan Beller
> Signed-off-by: Ben Peart
> ---
>
> Notes:
> Base Ref: v2.19.0-rc2
> Web-Diff: https://github.com/benpeart/git/commit/ed30e1a885
> Checkout: git fetch https://github.com/benpeart/git fsmonitor-t7411-v1 &&
> git checkout ed30e1a885
>
> submodule.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 50cbf5f13e..1e7194af28 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
> if ((pos >= 0) && (pos < istate->cache_nr)) {
> struct stat st;
> if (lstat(GITMODULES_FILE, ) == 0 &&
> - ie_match_stat(istate, istate->cache[pos], ,
> - CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
> + ie_match_stat(istate, istate->cache[pos], , 0) &
> DATA_CHANGED)
> return 0;
> }
>
>
> base-commit: c05048d43925ab8edcb36663752c2b4541911231
> --
> 2.18.0.windows.1
>
[PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes
From: Jeff Hostetler
Signed-off-by: Jeff Hostetler
---
compat/mingw.c| 38 ---
t/t0051-windows-named-pipe.sh | 2 +-
2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..f87376b26a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
return ret;
}
+/*
+ * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA
+ * is documented in [1] as opening a writable file handle in append mode.
+ * (It is believed that) this is atomic since it is maintained by the
+ * kernel unlike the O_APPEND flag which is racily maintained by the CRT.
+ *
+ * [1]
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
+ *
+ * This trick does not appear to work for named pipes. Instead it creates
+ * a named pipe client handle that cannot be written to. Callers should
+ * just use the regular _wopen() for them. (And since client handle gets
+ * bound to a unique server handle, it isn't really an issue.)
+ */
static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
{
HANDLE handle;
@@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename,
int oflags, ...)
NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE)
return errno = err_win_to_posix(GetLastError()), -1;
+
/*
* No O_APPEND here, because the CRT uses it only to reset the
-* file pointer to EOF on write(); but that is not necessary
-* for a file created with FILE_APPEND_DATA.
+* file pointer to EOF before each write(); but that is not
+* necessary (and may lead to races) for a file created with
+* FILE_APPEND_DATA.
*/
fd = _open_osfhandle((intptr_t)handle, O_BINARY);
if (fd < 0)
@@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int
oflags, ...)
return fd;
}
+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))
+/*
+ * Does the pathname map to the local named pipe filesystem?
+ * That is, does it have a "//./pipe/" prefix?
+ */
+static int mingw_is_local_named_pipe_path(const char *filename)
+{
+ return (IS_SBS(filename[0]) &&
+ IS_SBS(filename[1]) &&
+ filename[2] == '.' &&
+ IS_SBS(filename[3]) &&
+ !strncasecmp(filename+4, "pipe", 4) &&
+ IS_SBS(filename[8]) &&
+ filename[9]);
+}
+#undef IS_SBS
+
int mingw_open (const char *filename, int oflags, ...)
{
typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
@@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
- if (oflags & O_APPEND)
+ if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
open_fn = mingw_open_append;
else
open_fn = _wopen;
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
index e3c36341a0..10ac92d225 100755
--- a/t/t0051-windows-named-pipe.sh
+++ b/t/t0051-windows-named-pipe.sh
@@ -4,7 +4,7 @@ test_description='Windows named pipes'
. ./test-lib.sh
-test_expect_failure MINGW 'o_append write to named pipe' '
+test_expect_success MINGW 'o_append write to named pipe' '
GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
{ test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
pid=$! &&
--
gitgitgadget
[PATCH v2 0/2] Fixup for js/mingw-o-append
The recent change mingw O_APPEND change breaks writing to named pipes on
Windows. The first commit adds a new test to confirm the breakage and the
second commit fixes the problem. These could be squashed together or we can
just keep the fix and omit the test if that would be better.
d641097589 (js/mingw-o-append) mingw: enable atomic O_APPEND
The new mingw_open_append() routine successfully opens the client side of
the named pipe, but the first write() to it fails with EBADF. Adding the
FILE_WRITE_DATA corrects the problem.
Signed-off-by: Jeff Hostetler jeffh...@microsoft.com
[jeffh...@microsoft.com]
Cc: j6t@kdbg.orgCc: johannes.schinde...@gmx.decc: gitster@pobox.comCc:
p...@peff.net
Jeff Hostetler (2):
t0051: test GIT_TRACE to a windows named pipe
mingw: fix mingw_open_append to work with named pipes
Makefile | 1 +
compat/mingw.c | 38 ++--
t/helper/test-tool.c | 3 ++
t/helper/test-tool.h | 3 ++
t/helper/test-windows-named-pipe.c | 72 ++
t/t0051-windows-named-pipe.sh | 17 +++
6 files changed, 131 insertions(+), 3 deletions(-)
create mode 100644 t/helper/test-windows-named-pipe.c
create mode 100755 t/t0051-windows-named-pipe.sh
base-commit: d641097589160eb795127d8dbcb14c877c217b60
Published-As:
https://github.com/gitgitgadget/git/releases/tags/pr-35%2Fjeffhostetler%2Ffixup-mingw-o-append-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git
pr-35/jeffhostetler/fixup-mingw-o-append-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/35
Range-diff vs v1:
1: 03453cb521 ! 1: ecb30eb47c t0051: test GIT_TRACE to a windows named pipe
@@ -140,7 +140,7 @@
+
+. ./test-lib.sh
+
-+test_expect_success MINGW 'o_append write to named pipe' '
++test_expect_failure MINGW 'o_append write to named pipe' '
+ GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
+ { test-tool windows-named-pipe t0051 >actual 2>&1 & } &&
+ pid=$! &&
2: f433937d55 < -: -- mingw: fix mingw_open_append to work with
named pipes
-: -- > 2: f0361dd306 mingw: fix mingw_open_append to work with
named pipes
--
gitgitgadget
[PATCH v2 1/2] t0051: test GIT_TRACE to a windows named pipe
From: Jeff Hostetler
Create a test-tool helper to create the server side of
a windows named pipe, wait for a client connection, and
copy data written to the pipe to stdout.
Create t0051 test to route GIT_TRACE output of a command
to a named pipe using the above test-tool helper.
Signed-off-by: Jeff Hostetler
---
Makefile | 1 +
t/helper/test-tool.c | 3 ++
t/helper/test-tool.h | 3 ++
t/helper/test-windows-named-pipe.c | 72 ++
t/t0051-windows-named-pipe.sh | 17 +++
5 files changed, 96 insertions(+)
create mode 100644 t/helper/test-windows-named-pipe.c
create mode 100755 t/t0051-windows-named-pipe.sh
diff --git a/Makefile b/Makefile
index e4b503d259..b1b934d295 100644
--- a/Makefile
+++ b/Makefile
@@ -731,6 +731,7 @@ TEST_BUILTINS_OBJS += test-submodule-config.o
TEST_BUILTINS_OBJS += test-subprocess.o
TEST_BUILTINS_OBJS += test-urlmatch-normalization.o
TEST_BUILTINS_OBJS += test-wildmatch.o
+TEST_BUILTINS_OBJS += test-windows-named-pipe.o
TEST_BUILTINS_OBJS += test-write-cache.o
TEST_PROGRAMS_NEED_X += test-dump-fsmonitor
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index 805a45de9c..7a9764cd5c 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -41,6 +41,9 @@ static struct test_cmd cmds[] = {
{ "subprocess", cmd__subprocess },
{ "urlmatch-normalization", cmd__urlmatch_normalization },
{ "wildmatch", cmd__wildmatch },
+#ifdef GIT_WINDOWS_NATIVE
+ { "windows-named-pipe", cmd__windows_named_pipe },
+#endif
{ "write-cache", cmd__write_cache },
};
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index 7116ddfb94..01c34fe5e7 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -35,6 +35,9 @@ int cmd__submodule_config(int argc, const char **argv);
int cmd__subprocess(int argc, const char **argv);
int cmd__urlmatch_normalization(int argc, const char **argv);
int cmd__wildmatch(int argc, const char **argv);
+#ifdef GIT_WINDOWS_NATIVE
+int cmd__windows_named_pipe(int argc, const char **argv);
+#endif
int cmd__write_cache(int argc, const char **argv);
#endif
diff --git a/t/helper/test-windows-named-pipe.c
b/t/helper/test-windows-named-pipe.c
new file mode 100644
index 00..b4b752b01a
--- /dev/null
+++ b/t/helper/test-windows-named-pipe.c
@@ -0,0 +1,72 @@
+#include "test-tool.h"
+#include "git-compat-util.h"
+#include "strbuf.h"
+
+#ifdef GIT_WINDOWS_NATIVE
+static const char *usage_string = "";
+
+#define TEST_BUFSIZE (4096)
+
+int cmd__windows_named_pipe(int argc, const char **argv)
+{
+ const char *filename;
+ struct strbuf pathname = STRBUF_INIT;
+ int err;
+ HANDLE h;
+ BOOL connected;
+ char buf[TEST_BUFSIZE + 1];
+
+ if (argc < 2)
+ goto print_usage;
+ filename = argv[1];
+ if (strchr(filename, '/') || strchr(filename, '\\'))
+ goto print_usage;
+ strbuf_addf(, "//./pipe/%s", filename);
+
+ /*
+* Create a single instance of the server side of the named pipe.
+* This will allow exactly one client instance to connect to it.
+*/
+ h = CreateNamedPipeA(
+ pathname.buf,
+ PIPE_ACCESS_INBOUND | FILE_FLAG_FIRST_PIPE_INSTANCE,
+ PIPE_TYPE_MESSAGE | PIPE_READMODE_MESSAGE | PIPE_WAIT,
+ PIPE_UNLIMITED_INSTANCES,
+ TEST_BUFSIZE, TEST_BUFSIZE, 0, NULL);
+ if (h == INVALID_HANDLE_VALUE) {
+ err = err_win_to_posix(GetLastError());
+ fprintf(stderr, "CreateNamedPipe failed: %s\n",
+ strerror(err));
+ return err;
+ }
+
+ connected = ConnectNamedPipe(h, NULL)
+ ? TRUE
+ : (GetLastError() == ERROR_PIPE_CONNECTED);
+ if (!connected) {
+ err = err_win_to_posix(GetLastError());
+ fprintf(stderr, "ConnectNamedPipe failed: %s\n",
+ strerror(err));
+ CloseHandle(h);
+ return err;
+ }
+
+ while (1) {
+ DWORD nbr;
+ BOOL success = ReadFile(h, buf, TEST_BUFSIZE, , NULL);
+ if (!success || nbr == 0)
+ break;
+ buf[nbr] = 0;
+
+ write(1, buf, nbr);
+ }
+
+ DisconnectNamedPipe(h);
+ CloseHandle(h);
+ return 0;
+
+print_usage:
+ fprintf(stderr, "usage: %s %s\n", argv[0], usage_string);
+ return 1;
+}
+#endif
diff --git a/t/t0051-windows-named-pipe.sh b/t/t0051-windows-named-pipe.sh
new file mode 100755
index 00..e3c36341a0
--- /dev/null
+++ b/t/t0051-windows-named-pipe.sh
@@ -0,0 +1,17 @@
+#!/bin/sh
+
+test_description='Windows named pipes'
+
+. ./test-lib.sh
+
+test_expect_failure MINGW 'o_append write to named pipe' '
+ GIT_TRACE="$(pwd)/expect" git status >/dev/null 2>&1 &&
+ { test-tool
Re: git silently ignores include directive with single quotes
Jeff King writes: > On Sat, Sep 08, 2018 at 11:58:47AM -0700, Stas Bekman wrote: > >> This doesn’t: >> >> [include] >> path = '../.gitconfig' > > So I think it's been covered elsewhere that single quotes aren't a thing > in git's config format. I will say that this was actually a minor > surprise to me, after a decade of working with the format. ;) > > I don't know if it's worth changing now or not It would be > backwards-incompatible, but I wonder if we could do it in a sane way. > E.g., with a rule like: > > - if the first non-whitespace character of the value is a > single-quote, assume the value is quoted and apply normal shell > rules (i.e., no backslash escapes until the ending single-quote) > > - otherwise, single-quotes are not special at all At least the rule would not force those with ' in the middle of their family names to surround the user.name with extra double quotes, and it would probably be a good and safe practical solution. Being safe "by magic" tend to become hard to explain, but in this case the magic part is probably still simple enough.
Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes
On 9/10/2018 12:42 PM, Junio C Hamano wrote: Jeff Hostetler writes: Yeah, this whole thing is a little under-documented for my tastes. Let's leave it as you have it. I'll re-roll with a fix to route named pipes to the existing _wopen() code. OK, I have these two patches in 'next', but let's postpone it for now. Windows port will be built with extra topics over what we have a the release anyway, so your improved version may become part of that and then can come back to benefit us in the next cycle ;-) Thanks. That's fine. This can easily wait until after 2.19.0. Thanks Jeff
Re: [PATCH v2 10/11] builtin rebase: only store fully-qualified refs in `options.head_name`
SZEDER Gábor writes:
>> } else {
>> -options.head_name = xstrdup("detached HEAD");
>> +free(options.head_name);
>> +options.head_name = NULL;
>
> Please use FREE_AND_NULL(options.head_name) here.
Good; did contrib/coccinelle/free.cocci catch this?
>
>> branch_name = "HEAD";
>> }
>> if (get_oid("HEAD", _head))
>> --
>> gitgitgadget
>>
Re: [PATCH 0/4] un-breaking pack-objects with bitmaps
Ævar Arnfjörð Bjarmason writes: > I'm just reverting jk/pack-delta-reuse-with-bitmap out of next when > building my own package of git, but I think this really should be fixed > in that branch, either by merging the fix down or reverting the original > series out of next, I think just merging the fix down makes sense, but > have no strong opinion on it. Either is fine. I am not moving 'next' beyond what is necessary for this release cycle during the pre-release freeze period, and because I thought that Peff was in favor of squashing in the changes to the original when the next cycle starts, I haven't bothered to merge the fix there yet.
Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes
Jeff Hostetler writes: > Yeah, this whole thing is a little under-documented for my tastes. > Let's leave it as you have it. I'll re-roll with a fix to route > named pipes to the existing _wopen() code. OK, I have these two patches in 'next', but let's postpone it for now. Windows port will be built with extra topics over what we have a the release anyway, so your improved version may become part of that and then can come back to benefit us in the next cycle ;-) Thanks.
Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH
Jeff King writes: > But that couldn't have been what older versions were doing, since they > never looked at CONTENT_LENGTH at all, and instead always read to EOF. > So presumably the original problem wasn't that we tried to read a body, > but that the empty string caused git_parse_ssize_t to report failure, > and we called die(). Which probably should be explained by 574c513e8d > (http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too > late for that. > > So after that patch, we really do have the original behavior, and that's > enough for v2.19. To recap to make sure I am following it correctly: - pay attention to content-length when it is clearly given with a byte count, which is an improvement over v2.18 - mimick what we have been doing until now when content-length is missing or set to an empty string, so we are regression free and bug-to-bug compatible relative to v2.18 in these two cases. > But the remaining question then is: what should clients expect on an > empty variable? We know what the RFC says, and we know what dulwich > expected, but I'm not sure we have real world cases beyond that. So it > might actually make sense to punt until we see one, though I don't mind > doing what the rfc says in the meantime. And then the explanation in the > commit message would be "do what the rfc says", and any test probably > ought to be feeding a non-empty empty and confirming that we don't read > it. The RFC is pretty clear that no data is signaled by "NULL (or unset)", meaning an empty string value and missing variable both mean the same "no message body", but it further says that the servers MUST set CONTENT_LENGTH if and only if there is a message-body, which contradicts with itself (if you adhered to 'if and only if', in no case you would set it to NULL). Googling "cgi chunked encoding" seems to give us tons of hits to show that people are puzzled, just like us, that the scripts would not get to see Chunked (as the server is supposed to deChunk to count content-length before calling the backend). So I agree "do what the rfc says" is a good thing to try early in the next cycle.
[PATCH v1] git-mv: allow submodules and fsmonitor to work together
It was reported that
GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
breaks as the fsmonitor data is out of sync with the state of the .gitmodules
file. Update is_staging_gitmodules_ok() so that it no longer tells
ie_match_stat() to ignore refreshing the fsmonitor data.
Reported-by: Ævar Arnfjörð Bjarmason
Helped-by: Stefan Beller
Signed-off-by: Ben Peart
---
Notes:
Base Ref: v2.19.0-rc2
Web-Diff: https://github.com/benpeart/git/commit/ed30e1a885
Checkout: git fetch https://github.com/benpeart/git fsmonitor-t7411-v1 &&
git checkout ed30e1a885
submodule.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/submodule.c b/submodule.c
index 50cbf5f13e..1e7194af28 100644
--- a/submodule.c
+++ b/submodule.c
@@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
if ((pos >= 0) && (pos < istate->cache_nr)) {
struct stat st;
if (lstat(GITMODULES_FILE, ) == 0 &&
- ie_match_stat(istate, istate->cache[pos], ,
- CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
+ ie_match_stat(istate, istate->cache[pos], , 0) &
DATA_CHANGED)
return 0;
}
base-commit: c05048d43925ab8edcb36663752c2b4541911231
--
2.18.0.windows.1
Re: [Possible GIT Bug]
Jeff King writes: > Your explanation is correct. To be fair, though, it seems like > --find-copies-harder is made a lot less useful by the not considering > the larger set of sources, since that's kind of its point. I'm not sure > if this behavior actually is intentional, or simply what happens to > occur based on the combination of features. It is neither but if I have to pick one between the two, it is much closer to the former than the latter. The primary source of this is that we have only *one* pathspec given to the diff machinery, but in order to implement your ideal "find harder", you'd need *two*. That is, one set of paths for which you are interested in their origin, and the other set that you allow the machinery to consider as possible origins. Since we can only give one pathspec machinery, that one pathspec is used to specify both of these sets.
zsh completion does not support branches with quotes/apostrophes
Hi, I have the latest git and it includes a nice auto-completion for zsh: contrib/completion/git-completion.zsh I’ve been using it happily for.. years I guess, until I ran into a problem yesterday. I have a branch with apostrophe, named like: somebody's_business When I want to switch to that branch, I type: git checkout some[tab] and the result is: `git checkout somebody's_business` (and that does not work in zsh, of course). For this to work it should escape the apostrophe and the completion should result it: git checkout somebody\'s_business When I try to use the quotes in the parameters, this does not work at all: i.e. when I type `git checkout -b “some[tab]` nothing is completed. So the only work-around for now seems to be manually fixing the completion results, escaping the quote. However, it would be very nice if this could be fixed. Thanks. -- Vasily Korytov https://chillum.github.io
Re: [PATCH] git-mv: allow submodules and fsmonitor to work together
On 9/6/2018 4:34 PM, Stefan Beller wrote:
It was reported that
GIT_FSMONITOR_TEST=$PWD/t7519/fsmonitor-all ./t7411-submodule-config.sh
breaks as the .gitmodules file is modified and staged after the fsmonitor
considers it clean. Mark the .gitmodules file to be not clean before
staging.
Reported-by: Ævar Arnfjörð Bjarmason
Inspired-by: Ben Peart
Signed-off-by: Stefan Beller
---
I am not quite sure if this is the correct approach and handling of the
fsmonitor API, but it unbreaks the test.
Just naively adding mark_fsmonitor_invalid doesn't work, as then ...
Adding it before the staging, works.
Please double check!
I took a look at this bug/patch and wondered why add_file_to_index()
wasn't properly handling the .gitmodules file. On investigation, I
chased it down to what looks like a faulty test in
is_staging_gitmodules_ok().
I believe the following is a better patch for this bug:
diff --git a/submodule.c b/submodule.c
index 50cbf5f13e..1e7194af28 100644
--- a/submodule.c
+++ b/submodule.c
@@ -65,8 +65,7 @@ int is_staging_gitmodules_ok(struct index_state *istate)
if ((pos >= 0) && (pos < istate->cache_nr)) {
struct stat st;
if (lstat(GITMODULES_FILE, ) == 0 &&
- ie_match_stat(istate, istate->cache[pos], ,
- CE_MATCH_IGNORE_FSMONITOR) & DATA_CHANGED)
+ ie_match_stat(istate, istate->cache[pos], , 0) &
DATA_CHANGED)
return 0;
}
Please double check but I just don't understand why the .gitmodules file
should force the fsmonitor data to be ignored. This flag was added to
enable proper behavior in the preload_thread() logic and I don't believe
it is appropriate here.
Ben
Thanks,
Stefan
submodule.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/submodule.c b/submodule.c
index 50cbf5f13ed..56b0d5fe24e 100644
--- a/submodule.c
+++ b/submodule.c
@@ -22,6 +22,7 @@
#include "worktree.h"
#include "parse-options.h"
#include "object-store.h"
+#include "fsmonitor.h"
static int config_update_recurse_submodules = RECURSE_SUBMODULES_OFF;
static struct string_list changed_submodule_names = STRING_LIST_INIT_DUP;
@@ -149,6 +150,15 @@ int remove_path_from_gitmodules(const char *path)
void stage_updated_gitmodules(struct index_state *istate)
{
+ struct cache_entry *ce;
+ int pos;
+
+ pos = index_name_pos(istate, GITMODULES_FILE, strlen(GITMODULES_FILE));
+ ce = (0 <= pos) ? istate->cache[pos] : NULL;
+
+ if (ce)
+ mark_fsmonitor_invalid(istate, ce);
+
if (add_file_to_index(istate, GITMODULES_FILE, 0))
die(_("staging updated .gitmodules failed"));
}
Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes
On 9/8/2018 2:31 PM, Johannes Sixt wrote: Am 08.09.2018 um 11:26 schrieb Johannes Sixt: Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget: diff --git a/compat/mingw.c b/compat/mingw.c index 858ca14a57..ef03bbe5d2 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...) * FILE_SHARE_WRITE is required to permit child processes * to append to the file. */ - handle = CreateFileW(wfilename, FILE_APPEND_DATA, + handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA, FILE_SHARE_WRITE | FILE_SHARE_READ, NULL, create, FILE_ATTRIBUTE_NORMAL, NULL); if (handle == INVALID_HANDLE_VALUE) I did not go with this version because the documentation https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants says: FILE_APPEND_DATA: For a file object, the right to append data to the file. (For local files, write operations will not overwrite existing data if this flag is specified without FILE_WRITE_DATA.) [...] which could be interpreted as: Only if FILE_WRITE_DATA is not set, we have the guarantee that existing data in local files is not overwritten, i.e., new data is appended atomically. Is this interpretation too narrow and we do get atomicity even when FILE_WRITE_DATA is set? Here is are some comments on stackoverflow which let me think that FILE_APPEND_DATA with FILE_WRITE_DATA is no longer atomic: https://stackoverflow.com/questions/20093571/difference-between-file-write-data-and-file-append-data#comment29995346_20108249 -- Hannes Yeah, this whole thing is a little under-documented for my tastes. Let's leave it as you have it. I'll re-roll with a fix to route named pipes to the existing _wopen() code. thanks Jeff
Re: [PATCH] t3701-add-interactive: tighten the check of trace output
On Mon, Sep 10, 2018 at 04:07:14PM +0200, SZEDER Gábor wrote: > The test 'add -p does not expand argument lists' in > 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do > not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE > of 'git add -p' to ensure that the name of a tracked file wasn't > passed around as argument to any of the commands executed as a result > of undesired pathspec expansion. This check is done with 'grep' using > the filename on its own as the pattern, which is too loose a pattern, > and would match any occurrences of the filename in the trace output, > not just those as command arguments. E.g. if a developer were to > litter the index handling code with trace_printf()s printing, among > other things, the name of the just processed cache entry, then that > pattern would mistakenly match these as well, and would fail the test. Is this a real thing we're running into? I'd have thought that anybody adding index-specific tracing would do it as GIT_TRACE_INDEX. It's unfortunate that "trace commands and processes" is just GIT_TRACE, and not GIT_TRACE_RUN or similar. But that's mostly historical. I wouldn't expect people to add other subsystems to it. Not that I'm totally opposed to your patch, but it's a little sad that we have to match the specific text used in GIT_TRACE now (and if they ever changed we won't even notice, but rather the test will just become a silent noop). I think it would be nice if we could move towards something like: - move current GIT_TRACE messages to use GIT_TRACE_COMMAND or similar - abolish trace_printf() without a specific subsystem key - do one of: - keep GIT_TRACE as a historical synonym for GIT_TRACE_COMMAND; that keeps things working as they are now - have GIT_TRACE enable _all_ tracing; that's a change in behavior, but arguably a more useful thing to have going forward (e.g., when you're not sure which traces are even available) And then a test like this would just use GIT_TRACE_COMMAND. > diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh > index 609fbfdc31..65dfbc033a 100755 > --- a/t/t3701-add-interactive.sh > +++ b/t/t3701-add-interactive.sh > @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument > lists' ' > # update it, but we want to be sure that our "." pathspec > # was not expanded into the argument list of any command. > # So look only for "not-changed". > - ! grep not-changed trace.out > + ! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out I had a vague recollection that we preferred "egrep" to "grep -E" due to portability. But digging in the history, I could only find "fgrep" over "grep -F". And we seem to have plenty of "grep -E" invocations already. -Peff
Re: [PATCH] t3701-add-interactive: tighten the check of trace output
> Tighten this 'grep' pattern to only match trace lines that show the > executed commands. Looks good, and I think that this is the only such occurrence in t3701 that needs this treatment. Signed-off-by: Taylor Blau
[PATCH] t3701-add-interactive: tighten the check of trace output
The test 'add -p does not expand argument lists' in 't3701-add-interactive.sh', added in 7288e12cce (add--interactive: do not expand pathspecs with ls-files, 2017-03-14), checks the GIT_TRACE of 'git add -p' to ensure that the name of a tracked file wasn't passed around as argument to any of the commands executed as a result of undesired pathspec expansion. This check is done with 'grep' using the filename on its own as the pattern, which is too loose a pattern, and would match any occurrences of the filename in the trace output, not just those as command arguments. E.g. if a developer were to litter the index handling code with trace_printf()s printing, among other things, the name of the just processed cache entry, then that pattern would mistakenly match these as well, and would fail the test. Tighten this 'grep' pattern to only match trace lines that show the executed commands. Signed-off-by: SZEDER Gábor --- t/t3701-add-interactive.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh index 609fbfdc31..65dfbc033a 100755 --- a/t/t3701-add-interactive.sh +++ b/t/t3701-add-interactive.sh @@ -540,7 +540,7 @@ test_expect_success 'add -p does not expand argument lists' ' # update it, but we want to be sure that our "." pathspec # was not expanded into the argument list of any command. # So look only for "not-changed". - ! grep not-changed trace.out + ! grep -E "^trace: (built-in|exec|run_command): .*not-changed" trace.out ' test_expect_success 'hunk-editing handles custom comment char' ' -- 2.19.0.rc2.140.g09cf9e37c9
Re: [PATCH v6 17/21] range-diff: populate the man page
On Sun, Sep 09, 2018 at 07:19:51PM +0200, Ævar Arnfjörð Bjarmason wrote:
> >> And then I turn that into:
> >>
> >> # @{u} because I happen to be on 'master' and it's shorter to type
> >> # than origin/master...
> >> git range-diff @{u} 38b5f0fe72...718fbdedbc
> >
> > I don't understand what you want with that @{u} or 'origin/master' in
> > the first place. It's unnecessary, the three-dot notation on its own
> > works just fine.
>
> Maybe I've been using the wrong mode all along, I passed over by habits
> from tbdiff, which were surely copy/pasted from somewhere.
>
> Looking at the git-range-diff manpage though it recommends
> over ... when the topic has been rebased, which is
> usually the case for e.g. a topic that's submitted to git.git (usually
> be the time feedback has been gathered & a re-submission has been made
> Junio has pushed another "master").
>
> So isn't " " the right thing to use over
> "..." for git.git use? I think so, but I'm not sure.
The problem with ... is that it finds the actual merge base,
not the beginning of the topic. So if you have a 5-patch topic, but the
first two patches weren't changed in the rebase, it won't show them at
all! I made this mistake in [1], for example.
For a force-push, though, you may not care about seeing the topic as a
whole, and that mid-topic merge-base could be just fine. So pasting just
the "A...B" works.
I don't think your "@{u} A...B" makes any sense. You're giving _two_
bases, which is weird. But even if you wanted to ignore the "..." base
as a convenience to users of fetch, @{u} does not necessarily have
anything to do with the @{upstream} of the topic at "A". You really want
branch@{u}, which is on a separate part of the fetch output line (and
your branch@{u} and the remote's are not necessarily the same, either;
in this case you probably do not even have that branch checked out).
-Peff
[1] https://public-inbox.org/git/20180821195102.gb...@sigill.intra.peff.net/
RE: Multiple GIT Accounts & HTTPS Client Certificates - Config
On September 10, 2018 4:09 AM, Sergei Haller wrote: > my problem is basically the following: my git server (https) requires > authentication using a clent x509 certificate. > > And I have multiple x509 certificates that match the server. > > when I access the https server using a browser, the browser asks which > certificate to use and everything is fine. > > When I try to access the git server from the command line (git pull or > similar), > the git will pick one of the available certificates (randomly or > alphabetically) > and try to access the server with that client certificate. Ending in the > situation that git picks the wrong certificate. > > I can workaround by deleting all client certificates from the windows > certificate store except the "correct" one => then git command line will pick > the correct certificate (the only one available) and everything works as > expected. > > Workaround is a workaround, I need to use all of the certificates repeatedly > for different repos and different other aplications (non-git), so I've been > deliting and reinstalling the certificates all the time in the last weeks... > > How can I tell git cmd (per config option??) to use a particular client > certificate for authenticating to the https server (I could provide > fingerprint > or serial number or sth like that) > > current environment: windows 10 and git version 2.18.0.windows.1 > > Would be absolutely acceptable if git would ask interactively which client > certificate to use (in case its not configurable) > > (I asked this question here before: > https://stackoverflow.com/questions/51952568/multiple-git-accounts- > https-client-certificates-config > ) Would you consider using SSH to authenticate? You can control which private key you use based on your ~/.ssh/config entries, which are case sensitive. You can choose the SSH key to use by playing with the case of the host name, like: github.com Github.com gitHub.com even if your user is "git" in all cases above. It is a bit hacky but it is part of the SSH spec and is supported by git and EGit (as of 5.x). Cheers, Randall -- Randall S. Becker Managing Director, Nexbridge Inc. LinkedIn.com/in/randallbecker +1.416.984.9826
Re: [Possible GIT Bug]
On Sun, Sep 09, 2018 at 12:04:58PM -0700, Bryan Turner wrote: > Here, though, you've _explicitly limited_ Git to only the copied file. > It's not allowed to consider any others, which means it can't "see" > the source path anymore. As a result, the copy is detected as a > straight add. Note that --find-copies-harder means the diff machinery > is allowed to consider files that weren't modified in the commit as > possible sources for copies, but that's still subject to your explicit > filtering. In other words, if PATH_TO_SOURCE_FILE wasn't modified, > running this would _not_ see a copy: > > git show -C 055f6c89fa4506037d1621761f13430f469b8029 -- > PATH_TO_MY_COPIED_FILE PATH_TO_SOURCE_FILE > > But running this would: > > git show -C -C 055f6c89fa4506037d1621761f13430f469b8029 -- > PATH_TO_MY_COPIED_FILE PATH_TO_SOURCE_FILE > > No bugs here. Everything is working as intended, if not, perhaps, as > you expected. Your explanation is correct. To be fair, though, it seems like --find-copies-harder is made a lot less useful by the not considering the larger set of sources, since that's kind of its point. I'm not sure if this behavior actually is intentional, or simply what happens to occur based on the combination of features. You can do: git log -C C --full-diff $commit -- $path to limit a traversal to commits touching $path, but still see the full diff (including possible copy sources). But AFAIK there's no option to limit the diff, but include extra copy sources. I'd be tempted to say we should do that automatically when --find-copies-harder is in effect, but it's possible that some people actually do want the current behavior. For a single path it's silly, but if you did something like this: git show -C -C $commit -- foo/ that would find differences in the foo/ directory, and find copies only from sources in foo/. That limits the result, but also limits the effort, which can be important given the cost of copy detection. -Peff
Re: [PATCH 1/2] t0051: test GIT_TRACE to a windows named pipe
On 9/9/2018 3:28 AM, Sebastian Schuberth wrote: On 9/7/2018 8:19 PM, Jeff Hostetler via GitGitGadget wrote: +test_expect_success MINGW 'o_append write to named pipe' ' Shouldn't this be "test_expect_failure" here, and then be changed to "test_expect_success" by your second patch? yes. thanks! Jeff
Re: [PATCH v4] http-backend: allow empty CONTENT_LENGTH
On Sun, Sep 09, 2018 at 10:25:58PM -0700, Jonathan Nieder wrote:
> > --- a/http-backend.c
> > +++ b/http-backend.c
> > @@ -353,8 +353,28 @@ static ssize_t get_content_length(void)
> > ssize_t val = -1;
> > const char *str = getenv("CONTENT_LENGTH");
> >
> > - if (str && !git_parse_ssize_t(str, ))
> > - die("failed to parse CONTENT_LENGTH: %s", str);
> > + if (!str) {
> > + /*
> > +* RFC3875 says this must mean "no body", but in practice we
> > +* receive chunked encodings with no CONTENT_LENGTH. Tell the
> > +* caller to read until EOF.
> > +*/
> > + val = -1;
> > + } else if (!*str) {
> > + /*
> > +* An empty length should be treated as "no body" according to
> > +* RFC3875, and this seems to hold in practice.
> > +*/
> > + val = 0;
>
> Are there example callers that this version fixes? Where can I read
> more, or what can I run to experience it?
>
> For example, v2.19.0-rc0~45^2~2 (http-backend: respect CONTENT_LENGTH
> as specified by rfc3875, 2018-06-10) mentions IIS/Windows; does IIS
> make use of this distinction?
So this code is what I recommended based on my reading of the RFC, and
based on my understanding of the Debian bug. But I admit I'm confused.
I thought the complaint was that this:
CONTENT_LENGTH= git http-backend
was reading a body, when it shouldn't be. And so setting it to 0 here
made sense.
But that couldn't have been what older versions were doing, since they
never looked at CONTENT_LENGTH at all, and instead always read to EOF.
So presumably the original problem wasn't that we tried to read a body,
but that the empty string caused git_parse_ssize_t to report failure,
and we called die(). Which probably should be explained by 574c513e8d
(http-backend: allow empty CONTENT_LENGTH, 2018-09-07), but it's too
late for that.
So after that patch, we really do have the original behavior, and that's
enough for v2.19.
But the remaining question then is: what should clients expect on an
empty variable? We know what the RFC says, and we know what dulwich
expected, but I'm not sure we have real world cases beyond that. So it
might actually make sense to punt until we see one, though I don't mind
doing what the rfc says in the meantime. And then the explanation in the
commit message would be "do what the rfc says", and any test probably
ought to be feeding a non-empty empty and confirming that we don't read
it.
-Peff
Re: Temporary git files for the gitdir created on a separate drive in workdir
Sending again without HTML Den mån 10 sep. 2018 kl 12:28 skrev Hultqvist : > > First I need to correct my previous observations. > > Today there appeared new set of config files in the root. > I looked into a few of them and found that their content doesn't match that > of the repo at "G:/Min enhet". > Instead separate files had content from separate git repos within the G drive. > These repos are not like the one we're discussed previously, they are > completely within G: using a classical .git directory. > > I guess git is creating the temporary files as close as possible to the root, > since "G:\" can't be written to, only "G:\Min enhet". and then copy them to > the final destination which in this case is the same drive. > If so then we can't get away from the duplicate files, the duplicates > themselves are the fault of Google Drive File Stream. > If git would create the config/master/index files within the .git dir to > start with that would hide the clutter from the root but not prevent the > duplicates. > Is this observation correct, that git creates temporary files closer to the > root than inside the .git directory or a sub directory thereof? > > I haven't experiences any issues working with these repos themselves. > One risk could be that the final "index" is an older version than "index > (45)". > I'm certain this haven't been the case since I push every commit to a remote > repo that doesn't accept changes in history. > > I understand that master and index needs to be updated with regular use. > Does config need the same amount of updates? > I've considered that file to only change when I make explicit changes, not > during regular use(push, commit, status). > > Thanks for the help looking into this for me. > > > Den sön 9 sep. 2018 kl 17:30 skrev Hultqvist : >> >> Since this thread started I haven't seen a single file mentioned being >> created, >> Usually they appear during work days when there is more activity. >> I've never seen the files created directly, only a larger amount of >> them once in a while. >> >> I will run process monitor and get back once I find out more. >> Den lör 8 sep. 2018 kl 15:44 skrev Duy Nguyen : >> > >> > On Sat, Sep 8, 2018 at 3:09 PM Duy Nguyen wrote: >> > > >> > > On Sat, Sep 8, 2018 at 11:28 AM Hultqvist >> > > wrote: >> > > > >> > > > The bash commands are using a git and bash bundle that was installed >> > > > in parallel with gitextensions(a gui for git) >> > > > >> > > > G:\Min enhet> set GIT_TRACE_SETUP=1 >> > > > G:\Min enhet> git st >> > > > 10:40:28.881927 trace.c:318 setup: git_dir: >> > > > C:/Users/hultqvist/Drive.git >> > > > 10:40:28.881927 trace.c:319 setup: git_common_dir: >> > > > C:/Users/hultqvist/Drive.git >> > > > 10:40:28.881927 trace.c:320 setup: worktree: G:/Min enhet >> > > > 10:40:28.881927 trace.c:321 setup: cwd: G:/Min enhet >> > > > 10:40:28.881927 trace.c:322 setup: prefix: (null) >> > > > 10:40:28.882930 chdir-notify.c:67 setup: chdir from 'G:/Min >> > > > enhet' to 'G:/Min enhet' >> > > >> > > Unfortunately this looks good. Whenever those files 'index', >> > > 'config'... are created, they should always be created in >> > > C:\Users\hultqvist\Drive.git, not G:\Min enhet, including their >> > > temporary versions. I don't know if there are any more changes on the >> > > windows fork that could affect this though, I only checked git.git. >> > >> > BTW do you notice these files showing up after any particular command >> > or they're always there after cloning? Perhaps some command got the >> > ".git" directory discovery wrong and assumed $GIT_DIR=$GIT_WORK_TREE. >> > We have a much bigger problem then. >> > -- >> > Duy
Multiple GIT Accounts & HTTPS Client Certificates - Config
Hi folks, my problem is basically the following: my git server (https) requires authentication using a clent x509 certificate. And I have multiple x509 certificates that match the server. when I access the https server using a browser, the browser asks which certificate to use and everything is fine. When I try to access the git server from the command line (git pull or similar), the git will pick one of the available certificates (randomly or alphabetically) and try to access the server with that client certificate. Ending in the situation that git picks the wrong certificate. I can workaround by deleting all client certificates from the windows certificate store except the "correct" one => then git command line will pick the correct certificate (the only one available) and everything works as expected. Workaround is a workaround, I need to use all of the certificates repeatedly for different repos and different other aplications (non-git), so I've been deliting and reinstalling the certificates all the time in the last weeks... How can I tell git cmd (per config option??) to use a particular client certificate for authenticating to the https server (I could provide fingerprint or serial number or sth like that) current environment: windows 10 and git version 2.18.0.windows.1 Would be absolutely acceptable if git would ask interactively which client certificate to use (in case its not configurable) (I asked this question here before: https://stackoverflow.com/questions/51952568/multiple-git-accounts-https-client-certificates-config ) Thanks! -- ser...@sergei-haller.de .
