Re: [PATCH 1/2] config.mak.uname: OpenBSD uses BSD semantics with fread for directories
FWIW this patch doesn't have any other siblings and subject should had been just [PATCH]; apologize for the confusion and the spam (including that other duplicated email, and most likely this one) Carlo
Re: [PATCH] config.mak.dev: enable -Wpedantic in clang
On Tue, Nov 27, 2018 at 2:53 AM Eric Sunshine wrote: > On Tue, Nov 27, 2018 at 5:06 AM Carlo Marcelo Arenas Belón > > +ifneq ($(filter clang10,$(COMPILER_FEATURES)),) > > +CFLAGS += -Wpedantic > > +endif > > Should this condition be tightened to match only for OSX since there > is no such clang version number in the rest world outside of Apple? instead, I changed it to clang4 and tested it in a FreeBSD 11 box I was able to get a hold off, would that work better? will update patch accordingly then Carlo
Re: [PATCH] files-backend.c: fix build error on Solaris
Signed-off-by: Carlo Marcelo Arenas Belón clang with -Wpedantic also catch this (at least with Apple LLVM version 10.0.0); recent versions of gcc also include that flag and at least 8.2.0 shows a warning for it, so it might be worth adding it to developer mode (maybe under the pedantic DEVOPTS), would this be something I should be adding?, what are the expectations around warnings and compilers? Carlo
Re: [PATCH] t5562: fix perl path
Tested-by: Carlo Marcelo Arenas Belón IMHO leaving the shebang might be better if only for consistency but could go eitherway Carlo
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Thu, Nov 22, 2018 at 3:43 PM Max Kirillov wrote: > also edited the test to include only push_plain case, > and repeat it several times, to avoid running irrelevant > cases, the failure never happened again. as I explained previously[1] and as odd as it might seem the push_plain case ONLY fails if your run them together with the other tests that return errors with compressed input frankly I don't understand how one could affect the other as they should be running in independent processes but it happens fairly consistently in NetBSD (tested 7.1, 7.2, 8) with only one CPU (tested i386 and amd64) Carlo [1] https://public-inbox.org/git/20181119213924.ga2...@sigill.intra.peff.net/T/#m041e9703432c39dcb04fe10e86fc53d5254474b4
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Wed, Nov 21, 2018 at 10:37 PM Max Kirillov wrote: > > On Wed, Nov 21, 2018 at 05:04:25PM -0800, Carlo Arenas wrote: > > the last child of its children long gone with an error as shown by : > > > > 9255 1 git-http-backend CALL close(1) > ... > > 9255 1 git-http-backend CALL write(2,0xbfb2a604,0x36) > > 9255 1 git-http-backend GIO fd 2 wrote 54 bytes > >"fatal: request ended in the middle of the gzip stream\n" > > This should be some other test than push_plain, some of the > gzip related ones. Are there other tests failing? it should, but I should note that for test 9 to fail, then either (or both) tests 7 and 8 should first succeed; not that I'd seen any other test fail (after I locally patched the perl path, of course) even when reordering them and while making sure tests 1 and 2 run first to create the dependencies for the rest Peff, could you elaborate on your "load testing" setup? which could give us any hints on what to look for?, FWIW I hadn't been able to reproduce the problem anywhere else (and not for a lack of trying) > > 9255 1 git-http-backend RET write 54/0x36 > > 9255 1 git-http-backend CALL write(1,0xb781f0e0,0x94) > > 9255 1 git-http-backend RET write -1 errno 9 Bad file descriptor > > This is interesting. http-backend for some reason closes its > stdout. Here it then tries to write there something. I have > not seen it in my push_plain run. Maybe it worth redirecting instead > to stderr, to avoid losing some diagnostics? that should help with the garbled output from stderr, AFAIK the process API allows creating a pipe specifically for that with would be better than redirecting stderr into stdout. the fact we got EBADF means that there is a problem somewhere though in the way the previous failure that closed stdout got handled (which should had been most likely in the call to die) Carlo PS. upstreaming the PERL_PATH fix is likely to be good to do soonish as I presume at least all BSD might be affected, let me know if you would rather me do that instead as I suspect we might be deadlocked otherwise ;)
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Wed, Nov 21, 2018 at 2:49 PM Max Kirillov wrote: > > On Wed, Nov 21, 2018 at 04:02:04AM -0800, Carlo Arenas wrote: > > for some tracing, it would seem that it gets 0 when > > trying to read 4 bytes from what I think is a pipe that connects to a > > child that has been gone already for a while. > > Could you clarify it? I'm afraid I don't understand. the error that gets eventually to stderr in the caller comes from get_packet_data, who is trying to read 4 bytes and gets 0. when looking at the trace (obtained with ktrace) I see there is no longer any other process running, the last child of it is long gone with an error as shown by : 9255 1 git-http-backend CALL close(1) 9255 1 git-http-backend RET close 0 9255 1 git-http-backend CALL read(0,0xbfb2bb14,0) 9255 1 git-http-backend GIO fd 0 read 0 bytes "" 9255 1 git-http-backend RET read 0 9255 1 git-http-backend CALL write(2,0xbfb2a604,0x36) 9255 1 git-http-backend GIO fd 2 wrote 54 bytes "fatal: request ended in the middle of the gzip stream\n" 9255 1 git-http-backend RET write 54/0x36 9255 1 git-http-backend CALL write(1,0xb781f0e0,0x94) 9255 1 git-http-backend RET write -1 errno 9 Bad file descriptor not sure how it got into that state, though Carlo
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Wed, Nov 21, 2018 at 2:49 PM Max Kirillov wrote: > > Should I install bash for it to work? I cannot say I understand what the > message is about. yes, you need to install bash and use SHELL_PATH=/usr/pkg/bin/bash; PERL_PATH=/usr/pkg/bin/perl for the perl script Carlo
Re: [PATCH] t5562: skip if NO_CURL is enabled
FWIW the issue goes away when more than 1 CPU is used in NetBSD 8,0 (32-bit) and for some tracing, it would seem that it gets 0 when trying to read 4 bytes from what I think is a pipe that connects to a child that has been gone already for a while. Carlo
Re: [PATCH 1/1] legacy-rebase: backport -C and --whitespace= checks
Tested-by: Carlo Marcelo Arenas Belón the C version prepends: "fatal: " unlike the shell version for both error messages Carlo
Re: [PATCH] clone: fix colliding file detection on APFS
Tested-by: Carlo Marcelo Arenas Belón in macOS 10.14.1 with APFS in Linux using VFAT (for the lulz) IMHO it would be ideal if test would be enabled/validated for windows (native, not only cygwin) as it might even work without the override and if we are to see conflicts, that is probably where most users with file insensitive filesystems might be found Carlo
Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems
ok 99 - colliding file detection as well in macOS with APFS Carlo
Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems
On Mon, Nov 19, 2018 at 9:23 AM Ramsay Jones wrote: > ok 99 # skip colliding file detection (missing !CYGWIN of > !MINGW,!CYGWIN,CASE_INSENSITIVE_FS) you need to enable this specific test first (removing !CYGWIN) so it doesn't get skipped Carlo
Re: [PATCH] t5562: skip if NO_CURL is enabled
On Mon, Nov 19, 2018 at 10:40 AM Max Kirillov wrote:
>
> On Mon, Nov 19, 2018 at 02:15:35AM -0800, Carlo Marcelo Arenas Belón wrote:
> > 6c213e863a ("http-backend: respect CONTENT_LENGTH for receive-pack",
> > 2018-07-27)
> > introduced all tests but without a check for CURL support from git.
>
> The tests should not be using curl, they just pipe data to
> http-backend's standard input.
NO_CURL reflects the build setting (for http support); CURL checks for
the curl binary, but as Ævar points out the requirements must be from
somewhere else since a NO_CURL=1 build (tested in macOS) still passes
the test, but not in NetBSD.
tests 3-8 seem to fail because perl is hardcoded to /urs/bin/perl in
t5562/invoke-with-content-length.pl, while I seem to be getting some
sporadic errors in 9 with the following output :
++ env CONTENT_TYPE=application/x-git-receive-pack-request
QUERY_STRING=/repo.git/git-receive-pack
'PATH_TRANSLATED=/home/carenas/src/git/t/trash
directory.t5562-http-backend-content-length/.git/git-receive-pack'
GIT_HTTP_EXPORT_ALL=TRUE REQUEST_METHOD=POST
/home/carenas/src/git/t/t5562/invoke-with-content-length.pl push_body
git http-backend
++ verify_http_result '200 OK'
++ grep fatal: act.err
Binary file act.err matches
++ return 1
error: last command exited with $?=1
not ok 9 - push plain
and the following output in act.err (with a 200 in act)
fatal: the remote end hung up unexpectedly
Carlo
Re: [PATCH v5] clone: report duplicate entries on case-insensitive filesystems
On Mon, Nov 19, 2018 at 4:28 AM Torsten Bögershausen wrote: > > Did you test it on Mac ? macOS 10.14.1 but only using APFS, did you test my patch with HFS+? > So what exactly are you trying to fix ? I get not ok 99 - colliding file detection # # grep X icasefs/warning && # grep x icasefs/warning && # test_i18ngrep "the following paths have collided" icasefs/warning # and the output of "warning" only shows one of the conflicting files, instead of both: Cloning into 'bogus'... done. warning: the following paths have collided (e.g. case-sensitive paths on a case-insensitive filesystem) and only one from the same colliding group is in the working tree: 'x' Carlo
Re: [PATCH 2/2] read-cache: use time_t instead of unsigned long
On Tue, Nov 13, 2018 at 12:49 AM Johannes Schindelin wrote: > On Mon, 12 Nov 2018, Carlo Marcelo Arenas Belón wrote: >> > > if time_t can't represent a valid time keep the indexes for failsafe > > Is this sentence incomplete? What are those "indexes"? the indexes that are created when core.splitIndex = true the code that was modified looks for a configuration parameter (or the default) that says something like "2 weeks ago" and that then converts that into a timestamp that can be compared with the modification time for the files that compose that index and then decides to delete anyone that is older than the "expiration date". since time_t is used for the stat.mtime there is a chance that the timestamp might overflow its size, so if an overflow is detected we assume the value to be equivalent to "never" and keep the files. note this scenario will only matter around 2038 and it should be fixed by then as part of the Year 2038 problem if we still care about 32-bit UNIX by then, 32-bit Windows has until next century. Carlo
Re: [PATCH 2/3] approxidate: handle pending number for "specials"
On Thu, Nov 1, 2018 at 10:24 PM Jeff King wrote:
>
> static void date_yesterday(struct tm *tm, struct tm *now, int *num)
> {
> + *num = 0;
the only caller (date_time) for this sends num = NULL, so this
triggers a segfault.
the only reference I could find to that apparently unused parameter comes from:
93cfa7c7a8 ("approxidate_careful() reports errorneous date string", 2010-01-26)
and seems to indicate it is optional and therefore a check for NULL
might make sense for all other cases
Carlo
Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues
On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt wrote: > > + timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed); nitpick: cast to DWORD instead of int Carlo
Re: [PATCH v3 3/3] commit-slab: missing definitions and forward declarations (hdr-check)
On Thu, Oct 25, 2018 at 2:09 PM Ramsay Jones wrote: > Yes, this will 'fix' the 'commit-reach.h' header (not surprising), > but I prefer my patch. ;-) I apologize, I joined the list recently and so might had missed a reroll; the merged series in pu doesn't seem to include it and the error was around the code I changed, so wanted to make sure it would be addressed sooner rather than later. eitherway, I agree with you my patch (or something better) would fit better in your topic branch than on mine and while I haven't seen your patch I am sure is most likely better. > Still puzzled. this are the last lines of a `make hdr-check` in Fedora Rawhide, it should behave the same regardless of OS or compiler used IMHO HDR commit-reach.h commit-reach.h:45:28: warning: ‘struct object_id’ declared inside parameter list will not be visible outside of this definition or declaration int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid); ^ In file included from commit-slab.h:5, from commit-reach.h:4: commit-reach.h: In function ‘contains_cache_at_peek’: commit-slab-impl.h:47:14: error: dereferencing pointer to incomplete type ‘const struct commit’ nth_slab = c->index / s->slab_size;\ ^~ commit-slab-impl.h:7:2: note: in expansion of macro ‘implement_commit_slab’ implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED) ^ commit-slab.h:49:2: note: in expansion of macro ‘implement_static_commit_slab’ implement_static_commit_slab(slabname, elemtype) ^~~~ commit-reach.h:57:1: note: in expansion of macro ‘define_commit_slab’ define_commit_slab(contains_cache, enum contains_result); ^~ commit-reach.h: At top level: commit-reach.h:69:41: warning: ‘struct object_array’ declared inside parameter list will not be visible outside of this definition or declaration int can_all_from_reach_with_flag(struct object_array *from, ^~~~ make: *** [Makefile:2685: commit-reach.hco] Error 1 Carlo
Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning
On Thu, Oct 25, 2018 at 1:08 AM Junio C Hamano wrote:
> For a single-use, not using the macro and just using "%s", "" should
> suffice.
OK, will send it as v2 then but would think it will be better if
applied as a "fixup" on top of the original branch:
34b47315d9 ("rebase -i: move rebase--helper modes to
rebase--interactive", 2018-09-27)
would be a good idea to include also enabling this warning in
developer mode so it doesn't sneak back?, presume as the last patch of
the refactor below?, FWIW this is the only case in current pu, and I
suspect the sooner we add it to next the less likely we will find
more.
> If we want to hide the "%s", "" trickery from distracting
> the readers (which is what you are trying to address with your
> touch_file() proposal, I think, and I also suspect that it may be a
> legitimate one), I tend to think that it may be a better solution to
> introduce the EMPTY_PRINTF_ARG macro and use it everywhere, in
> builtin/commit.c, builtin/difftool.c and wt-status.c and not not
> just here.
will work on this in a different feature branch, but I had to admit I
don't like it for status_printf case where it was IMHO a hack to get
the new lines to begin with.
I would think it would make more sense to call a function that says
"write_ttycolor_ln" instead for those cases.
Carlo
Re: [PATCH] sequencer: cleanup for gcc 8.2.1 warning
On Wed, Oct 24, 2018 at 11:22 PM Junio C Hamano wrote: > and they would read naturally. But may be it is a bit too cute an > idea? I dunno. my first idea was to replace it with a helper called touch_file, since I was expecting it will be a popular operation as flag files are common in shell scripts and that name will be second nature to anyone porting them. the fact that the quiet flag was created with a single '\n' in the code just immediately above this make me go for the proposed "solution" instead (which I verified wouldn't change behaviour as you described in your post; I apologize for not documenting it in the commit and wasting your time). would something like this work better? (not to apply, and probably mangled) --- a/sequencer.c +++ b/sequencer.c @@ -35,6 +35,8 @@ #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" +#define touch_file(path) write_file(path, "%s", "") + const char sign_off_header[] = "Signed-off-by: "; static const char cherry_picked_prefix[] = "(cherry picked from commit "; @@ -2389,7 +2391,7 @@ int write_basic_state(struct replay_opts *opts, const char *head_name, write_file(rebase_path_quiet(), "\n"); if (opts->verbose) - write_file(rebase_path_verbose(), ""); + touch_file(rebase_path_verbose()); if (opts->strategy) write_file(rebase_path_strategy(), "%s\n", opts->strategy); if (opts->xopts_nr > 0)
Re: [PATCH v4 10/12] Add a base implementation of SHA-256 support
On Wed, Oct 24, 2018 at 7:41 PM brian m. carlson wrote: > diff --git a/sha256/block/sha256.h b/sha256/block/sha256.h > new file mode 100644 > index 00..38f02f7e6c > --- /dev/null > +++ b/sha256/block/sha256.h > @@ -0,0 +1,26 @@ > +#ifndef SHA256_BLOCK_SHA256_H > +#define SHA256_BLOCK_SHA256_H > + > +#include "git-compat-util.h" this shouldn't be needed and might be discouraged as per the instructions in Documentation/CodingGuidelines Carlo
Re: [PATCH v2 1/2] commit-slabs: move MAYBE_UNUSED out
On Tue, Oct 23, 2018 at 3:00 PM Jeff King wrote: > On Tue, Oct 23, 2018 at 02:50:19PM -0700, Carlo Marcelo Arenas Belón wrote: > > #define implement_static_commit_slab(slabname, elemtype) \ > > - implement_commit_slab(slabname, elemtype, static MAYBE_UNUSED) > > + implement_commit_slab(slabname, elemtype, MAYBE_UNUSED static) > > Is this hunk necessary? it works eitherway but the proposed syntax is IMHO better aligned (when used in a function definition) as it matches better the syntax from C++ attribute: maybe_unused (since C++17) [1] __attribute__(unused) (the GCC extension) is meant to be applied to function declarations, but we are not generating those. Carlo [1] https://en.cppreference.com/w/cpp/language/attributes/maybe_unused
Re: [PATCH] khash: silence -Wunused-function
On Tue, Oct 23, 2018 at 9:19 AM René Scharfe wrote: > With Clang 6 and GCC 8 on Debian I don't get any warnings on master or > 36da893114. I see it with Clang 7 on Fedora (at least Rawhide but suspect also to affect the next release, now in beta: 29) > With Clang 6 on OpenBSD I get warnings about the unused khash functions > in delta-islands.c on master, though. Apple LLVM 9 and 10 will also trigger similar warnings to what you see in OpenBSD and as Junio mentioned recently in a different thread about semantic patches, this currently affects the CI for "Apple": https://travis-ci.org/git/git/builds/444969342 Carlo
Re: New semantic patches vs. in-flight topics [was: Re: [PATCH 00/19] Bring more repository handles into our code base]
On Tue, Oct 23, 2018 at 2:40 AM Junio C Hamano wrote: > > The tip of 'pu' has trouble with -Wunused on Apple around the > delta-islands series. FWIW the "problem" is actually with -Wunused-function and is AFAIK not related to the semantic changes or Apple (AKA macOS) Indeed, I saw this issue before also in Linux (Fedora Rawhide) when using the latest clang and presumed it was just expected because the topic branch was most likely incomplete. Carlo
Re: [PATCH 2/3] git-compat-util: define MIN through compat
I agree with you; dropped
Re: [PATCH] builtin/receive-pack: dead initializer for retval in check_nonce
On Sat, Oct 20, 2018 at 9:45 AM Torsten Bögershausen wrote:
>
> The motivation feels a little bit weak, at least to me.
I have to admit, I was sitting on this patch for a while for the same reason
but I should had made a more compelling commit message either way and
will definitely fix that with v2.
the point was that setting the variable to BAD originally seemed to be legacy
from the original version of the code, specially considering that the newer
version was setting it to SLOB at the last "else".
the code was written in a way that would make all those assignments to BAD
explicit (even if it wasn't needed, since it was initialized to that
value) and so
it would seem better IMHO to use the compiler to remind us that this variable
MUST be set to the right value than setting the most likely value by default.
> Initializing a variable to "BAD" in the beginning can be a good thing
> for two reasons:
> - There is a complex if-elseif chain, which should set retval
> in any case, this is at least what I expect taking a very quick look at the
> code:
> const char *retval = NONCE_BAD;
>
> if (!nonce) {
> retval = NONCE_MISSING;
> goto leave;
> } else if (!push_cert_nonce) {
> retval = NONCE_UNSOLICITED;
> goto leave;
> } else if (!strcmp(push_cert_nonce, nonce)) {
> retval = NONCE_OK;
> goto leave;
> }
> # And here I started to wonder if we should have an else or not.
> # Having retval NONCE_BAD set to NONCE:BAD in the beginning makes
> # it clear, that we are save without the else.
> # As an alternative, we could have coded like this:
>
> const char *retval;
>
> if (!nonce) {
> retval = NONCE_MISSING;
> goto leave;
> } else if (!push_cert_nonce) {
> retval = NONCE_UNSOLICITED;
> goto leave;
> } else if (!strcmp(push_cert_nonce, nonce)) {
> retval = NONCE_OK;
> goto leave;
> } else {
> /* Set to BAD, until we know better further down */
> retval = NONCE_BAD;
> }
>
> # The second reason is that some compilers don't understand this complex
> # stuff either, and through out a warning, like
> # "retval may be uninitialized" or something in that style.
> # This is very compiler dependent.
FWIW I did test with gcc (from 4.9 to 8) and clang 7 (linux) and 10
(macos) and didn't
trigger any warning.
> So yes, the current code may seem to be over-eager and ask for optimization,
> but we don't gain more that a couple of nano-seconds or so.
> The good thing is that we have the code a little bit more robust, when
> changes are done
> in the future.
on the other hand was able to trigger a warning if the code was changed so some
path will leave retval uninitialized (after adding
-Wmaybe-uninitialized to gcc and -Wsometimes-uninitialized to clang)
since there was no longer a default of BAD (probably incorrectly) that
would be returned in case setting retval to the
right value was forgotten.
Carlo
Re: [PATCH] multi-pack-index: avoid dead store for struct progress
On Thu, Oct 18, 2018 at 12:36 PM Derrick Stolee wrote: > > Is there a tool that reports a wasted > initialization that you used to find this? I'd used clang's analyzer recently to track a similar issue before in a different codebase, but not for this specific case. Carlo
