hang in git-upload-pack
Hi there In HP-Nonstop we're experiencing hangs in git-upload-pack, which seems to be the result of reads from / writes to pipes don't ever finish or don't get interrupted properly (SIGPIPE, SIGCHLD?) Any idea why that might be and how to fix it? bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Hi there While investigating the problem with hung git-upload-pack we think to have found a bug in wrapper.c: #define MAX_IO_SIZE (8*1024*1024) This is then used in xread() to split read()s into suitable chunks. So far so good, but read() is only guaranteed to read as much as SSIZE_MAX bytes at a time. And on our platform that is way lower than those 8MB (only 52kB, POSIX allows it to be as small as 32k), and as a (rather strange) consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), because xpread() returns something > 0. How large is SSIZE_MAX on other platforms? What happens there if you try to read() more? Should't we rather use SSIZE_MAX on all platforms? If I'm reading the header files right, on Linux it is LONG_MAX (2TB?), so I guess we should really go for MIN(8*1024*1024,SSIZE_MAX)? bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz schmitz-digital.de> writes: > because xpread() returns something > 0. something < 0 of course (presumably -1)... bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Torsten Bögershausen web.de> writes: > > On 2015-02-07 17.45, Joachim Schmitz wrote: > > How about changing wrapper.c like this: > > #ifndef MAX_IO_SIZE > #define MAX_IO_SIZE (8*1024*1024) > #endif > - > and to change config.mak.uname like this: > > ifeq ($(uname_S),NONSTOP_KERNEL) > > BASIC_CFLAGS += -DMAX_IO_SIZE=(32*1024) > Does this work for you ? Of course it would, but, a) 32k is smaller than we can go (and yes, we could make it 52k) b) never ever should read() be asked to read more than SSIZE_MAX, this should be true for every platform on the planet? You may want to have is smaller than SSIZE_MAX (like the current 8MB vs. the possible 2TB on Linux), but surely never larger? Bye, Jojo
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz schmitz-digital.de> writes: > > Torsten Bögershausen web.de> writes: > > > > > On 2015-02-07 17.45, Joachim Schmitz wrote: > b) never ever should read() be asked to read more than SSIZE_MAX, this > should be true for every platform on the planet? You may want to have is > smaller than SSIZE_MAX (like the current 8MB vs. the possible 2TB on > Linux), but surely never larger? Se also $gmane/232469, where that issue cropped up for MacOS X 64bit? Bye, Jojo
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Randall S. Becker nexbridge.com> writes: > > On 2015-02-07 13:07PM Randall S. Becker wrote: > >On 2015-02-07 12:30PM Torsten Bögershausen wrote: > >>On 2015-02-07 17.45, Joachim Schmitz wrote: > Although I do agree with Jojo, that MAX_IO_SIZE seems to be a platform > constant and should be defined in terms of SSIZE_MAX. So something like: > > #ifndef MAX_IO_SIZE > # ifdef SSIZE_MAX > # define MAX_IO_SIZE (SSIZE_MAX) > # else > # define MAX_IO_SIZE (8*1024*1024) > # endif > #endif > > would be desirable. It would be way too large on some platforms. those 8MB had been chosen for a good reason, I assume: /* * Limit size of IO chunks, because huge chunks only cause pain. OS X * 64-bit is buggy, returning EINVAL if len >= INT_MAX; and even in * the absence of bugs, large chunks can result in bad latencies when * you decide to kill the process. */ However it should never be larger than SSIZE_MAX
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz schmitz-digital.de> writes: > and as a (rather strange) > consequence mmap() (from compat/mmap.c) fails with EACCESS (why EACCESS?), > because xpread() returns something > 0. Seems mmap() should either set errno to EINVAL or not set it at all an just 'forward' whatever xpread() has set. As per http://man7.org/linux/man-pages/man2/mmap.2.html mmap() sets EINVAL if (amongst other things) it doesn't like the value of len, exactly the case here. bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Junio C Hamano pobox.com> writes:
>
> On Sat, Feb 7, 2015 at 12:32 PM, Torsten Bögershausen
web.de> wrote:
> > I don't know every platform of the planet well enough to be helpful
here,
> > especially the ones which don't follow all the specifications.
> >
> > In other words: As long as we can not guarantee that SSIZE_MAX is
defined,
> > (and is defined to somethong useful for xread()/xwrite() )
> > we should be more defensive here:
> >
> > tweak only on platform where we know it is needed and we know that it
works.
>
> Yup, I agree that is a sensible way to go.
>
> (1) if Makefile overrides the size, use it; otherwise
> (2) if SSIZE_MAX is defined, and it is smaller than our internal
> default, use it; otherwise
> (3) use our internal default.
>
> And leave our internal default to 8MB.
>
> That way, nobody needs to do anything differently from his current build
set-up,
> and I suspect that it would make step (1) optional.
>
something like this:
/* allow overwriting from e.g. Makefile */
#if !defined(MAX_IO_SIZE)
# define MAX_IO_SIZE (8*1024*1024)
#endif
/* for plattforms that have SSIZE and have it smaller */
#if defined(SSIZE_MAX && (SSIZE_MAX < MAX_IO_SIZE)
# undef MAX_IO_SIZE /* avoid warning */
# define MAX_IO_SIZE SSIZE_MAX
#endif
Steps 2 and 3 only , indeed step 1 not needed...
Bye,
JojoN�r��yb�X��ǧv�^�){.n�+ا���ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Junio C Hamano pobox.com> writes: > > > > something like this: > > > > /* allow overwriting from e.g. Makefile */ > > #if !defined(MAX_IO_SIZE) > > # define MAX_IO_SIZE (8*1024*1024) > > #endif > > /* for plattforms that have SSIZE and have it smaller */ > > #if defined(SSIZE_MAX && (SSIZE_MAX < MAX_IO_SIZE) > > # undef MAX_IO_SIZE /* avoid warning */ > > # define MAX_IO_SIZE SSIZE_MAX > > #endif > > No, not like that. If you do (1), that is only so that the Makefile can override > a broken definition a platform may give to SSIZE_MAX. So > > (1) if Makefile gives one, use it without second-guessing with SSIZE_MAX. > (2) if SSIZE_MAX is defined, and if it is smaller than our internal > default, use it. > (3) all other cases, us our internal default. oops, yes, of course /* allow overwriting from e.g. Makefile */ #ifndef(MAX_IO_SIZE) # define MAX_IO_SIZE (8*1024*1024) /* for plattforms that have SSIZE and have it smaller */ # if defined(SSIZE_MAX) && (SSIZE_MAX < MAX_IO_SIZE) # undef MAX_IO_SIZE /* avoid warning */ # define MAX_IO_SIZE SSIZE_MAX # endif #endif -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Junio C Hamano pobox.com> writes: > OK, then let's do this. > Yep, that'd do, thanks. bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Joachim Schmitz schmitz-digital.de> writes: > > Junio C Hamano pobox.com> writes: > > > > OK, then let's do this. > > Except for the type "taht" -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: read() MAX_IO_SIZE bytes, more than SSIZE_MAX?
Sorry to be a pain, but i think this sententence neede mending + * to override this, if the definition of SSIZE_MAX platform is broken. Bye, Jojo
Re: t5570 trap use in start/stop_git_daemon
Jeff King peff.net> writes:
>
> On Fri, Feb 13, 2015 at 02:44:03AM -0500, Jeff King wrote:
>
> > On Thu, Feb 12, 2015 at 03:31:12PM -0500, Randall S. Becker wrote:
> >
> Hmm, today I learned something new about ksh. Apparently when you use
> the "function" keyword to define a function like:
>
> function foo {
> trap 'echo trapped' EXIT
> }
> echo before
> foo
> echo after
>
> then the trap runs when the function exits! If you declare the same
> function as:
>
> foo() {
> trap 'echo trapped' EXIT
> }
>
> it behaves differently. POSIX shell does not have the function keyword,
> of course, and we are not using it here. Bash _does_ have the function
> keyword, but seems to behave POSIX-y even when it is present. I.e.,
> running the first script:
>
> $ ksh foo.sh
> before
> trapped
> after
>
> $ bash foo.sh
> before
> after
> trapped
>
> $ dash foo.sh
> foo.sh: 3: foo.sh: function: not found
> foo.sh: 5: foo.sh: Syntax error: "}" unexpected
>
> Switching to the second form, all three produce:
>
> before
> after
> trapped
>
> I don't know if that is all helpful to your bug-tracking or analysis,
> but for whatever reason it looks like your ksh is using localized traps
> for both forms of function. But as far as I know, bash has never behaved
> that way (I just grepped its CHANGES file for mentions of trap and found
> nothing likely).
>
> -Peff
>
Both versions produce your first output on our platform
$ ksh foo1.sh
before
trapped
after
$ bash foo1.sh
before
after
trapped
$ ksh foo2.sh
before
trapped
after
$ bash foo2.sh
before
after
trapped
$
This might have been one (or even _the_) reason why we picked bash as our
SHELL_PATH in config.mak.uname (I don't remember, it's more than 2 years
ago), not sure which shell Randall's test used?
bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Completion must sort before using uniq
Marc Khouzam wrote: The uniq program only works with sorted input. The man page states "uniq prints the unique lines in a sorted file". ... --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -321,7 +321,7 @@ __git_refs () if [[ "$ref" == "$cur"* ]]; then echo "$ref" fi - done | uniq -u + done | sort | uniq -u Is 'sort -u' not universally available and sufficient here? It is POSIX at least: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sort.html Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/8] fix git-config with duplicate variable entries
Junio C Hamano wrote: Jeff King writes: ... Not exactly. There are three classes of people: - wrote scripts using --get; found out that --get barfs if you feed two or more of the same, and have long learned to accept it as a limitation and adjusted their configuration files to avoid it. They have been doing just fine and wouldn't benefit from this series at all. - wrote scripts using --get, relying on it barfing if fed zero (i.e. missing) or two or more (i.e. ambiguous), perhaps a way to keep their configuration files (arguably unnecessarily) clean. They are directly harmed by this series. - wrote scripts using --get-all and did the last-one-wins themselves. They wouldn't benefit directly from this series, unless they are willing to spend a bit of time to remove their own last-one-wins logic and replace --get-all with --get (but the same effort is needed to migrate to --get-one). - wanted to write scripts using --get, but after finding out that it barfs if you feed two, gave up emulating the internal, without realizing that they could do so with --get-all. They can now write their scripts without using --get-all. There are three classes ofpeople: those that can count and those that can't Sorry could not resist ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Completion must sort before using uniq
Re-adding git@vger... > From: Marc Khouzam [mailto:marc.khou...@gmail.com] > Sent: Friday, November 23, 2012 11:51 AM > To: Joachim Schmitz > Cc: sze...@ira.uka.de; felipe.contre...@gmail.com > Subject: Re: [PATCH] Completion must sort before using uniq > > On Fri, Nov 23, 2012 at 3:10 AM, Joachim Schmitz > wrote: > > Marc Khouzam wrote: > >> The uniq program only works with sorted input. The man page states > >> "uniq prints the unique lines in a sorted file". > > ... > >> --- a/contrib/completion/git-completion.bash > >> +++ b/contrib/completion/git-completion.bash > >> @@ -321,7 +321,7 @@ __git_refs () > >>if [[ "$ref" == "$cur"* ]]; then > >>echo "$ref" > >>fi > >> - done | uniq -u > >> + done | sort | uniq -u > > > > Is 'sort -u' not universally available and sufficient here? It is POSIX > > at least: > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sort.html > > "-u Unique: suppress all but one in each set of lines having equal > keys. If used with the -c option, check that there are no lines with > duplicate keys, in addition to checking that the input file is > sorted." > > What the code aims to do is to only show lines that are not > duplicated. 'sort -u' would still output one line for each duplicated > one. It seems 'sort -u' is the equivalent of 'sort | uniq' but won't > replace 'sort | uniq -u'. I can't see the difference and in fact don't understand uniq's -u option al all Linux man pages say: "only print unique lines", but that is what uniq does by default anyway?!? > Is 'sort | uniq -u' not POSIX? It is. It is one process more though. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Completion must sort before using uniq
> From: Sascha Cunz [mailto:sascha...@babbelbox.org] > Sent: Friday, November 23, 2012 1:26 PM > To: Joachim Schmitz > Cc: 'Marc Khouzam'; git@vger.kernel.org; sze...@ira.uka.de; > felipe.contre...@gmail.com > Subject: Re: [PATCH] Completion must sort before using uniq > > > I can't see the difference and in fact don't understand uniq's -u option al > > all Linux man pages say: "only print unique lines", but that is what uniq > > does by default anyway?!? > > From the german translation of uniq's man-page, you can deduct that "only > print unique lines" actually means: "print lines that are _not repeated_ in > the input". > > A short test confirms that. i.e.: > > printf "a\nb\nb\nc\n" | uniq -u > > gives: > a > c Ah, OK, then I rest my case. Sorry for the noise. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix sizeof usage in get_permutations
Matthew Daley wrote:
Currently it gets the size of an otherwise unrelated, unused variable
instead of the expected struct size.
Signed-off-by: Matthew Daley
---
builtin/pack-redundant.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index f5c6afc..7544687 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -301,14 +301,14 @@ static void pll_free(struct pll *l)
*/
static struct pll * get_permutations(struct pack_list *list, int n)
{
- struct pll *subset, *ret = NULL, *new_pll = NULL, *pll;
+ struct pll *subset, *ret = NULL, *new_pll = NULL;
if (list == NULL || pack_list_size(list) < n || n == 0)
return NULL;
if (n == 1) {
while (list) {
- new_pll = xmalloc(sizeof(pll));
+ new_pll = xmalloc(sizeof(struct pll));
new_pll->pl = NULL;
pack_list_insert(&new_pll->pl, list);
new_pll->next = ret;
@@ -321,7 +321,7 @@ static struct pll * get_permutations(struct
pack_list *list, int n) while (list->next) {
subset = get_permutations(list->next, n - 1);
while (subset) {
- new_pll = xmalloc(sizeof(pll));
+ new_pll = xmalloc(sizeof(struct pll));
Why not just
new_pll = xmalloc(sizeof(*new_pll));
new_pll->pl = subset->pl;
pack_list_insert(&new_pll->pl, list);
new_pll->next = ret;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Build fixes for another obscure Unix
David Michael wrote: Hi, On Thu, Dec 13, 2012 at 12:18 PM, Pyeron, Jason J CTR (US) wrote: Would there be any interest in applying such individual compatibility fixes for this system, even if a full port doesn't reach completion? What are the down sides? Can your changes be shown to not impact builds on other systems? I've pushed a handful of small compatibility patches to GitHub[1] which are enough to successfully compile the project. The default values of the new variables should make them unnoticeable to other systems. Are there any concerns with this type of change? If they would be acceptable, I can try sending the first four of those patches to the list properly. (I expect the last two may be tweaked as I continue working with the port.) I do have a concern with strings.h, though. That file will be included for most people who run ./configure, when it wasn't before. Do you think it's worth making a more detailed test to see if strcasecmp is still undefined after string.h is included, rather than just testing for the header's existence? Thanks. David [1] https://github.com/dm0-/git/commits For what's it worth: I ACK your HP-NonStop patch (as you can see by my comment in git-compat-util.h I was thinking along the same line) https://github.com/dm0-/git/commit/933d72a5cfdc63fa9c3c68afa2f4899d9c3f791e together with its prerequisit https://github.com/dm0-/git/commit/301032c6488aeabb94ccc81bfb6d65ff2c23b924 ACKed by: Joachim Schmitz -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h
Johannes Sixt wrote: Am 14.12.2012 20:57, schrieb David Michael: This platform previously included strings.h automatically. However, the build system now requires an explicit option to do so. Signed-off-by: David Michael --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index fb78f7f..e84b0cb 100644 --- a/Makefile +++ b/Makefile @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) # Added manually, see above. NEEDS_SSL_WITH_CURL = YesPlease HAVE_LIBCHARSET_H = YesPlease +HAVE_STRINGS_H = YesPlease NEEDS_LIBICONV = YesPlease NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease NO_SYS_SELECT_H = UnfortunatelyYes If NONSTOP_KERNEL is the platform that defines __TANDEM, then this should be squashed into the previous patch, shouldn't it? Patch 4/4 does not work without 3/4, Not for HP-NonStop. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/4] Declare that HP NonStop systems require strings.h
> From: Johannes Sixt [mailto:j...@kdbg.org] > Sent: Saturday, December 15, 2012 12:17 AM > To: Joachim Schmitz > Cc: gits...@pobox.com; fedora@gmail.com; Git Mailing List > Subject: Re: [PATCH 4/4] Declare that HP NonStop systems require strings.h > > Am 14.12.2012 23:46, schrieb Joachim Schmitz: > > Johannes Sixt wrote: > >> Am 14.12.2012 20:57, schrieb David Michael: > >>> This platform previously included strings.h automatically. However, > >>> the build system now requires an explicit option to do so. > >>> > >>> Signed-off-by: David Michael > >>> --- > >>> Makefile | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/Makefile b/Makefile > >>> index fb78f7f..e84b0cb 100644 > >>> --- a/Makefile > >>> +++ b/Makefile > >>> @@ -1357,6 +1357,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL) > >>> # Added manually, see above. > >>> NEEDS_SSL_WITH_CURL = YesPlease > >>> HAVE_LIBCHARSET_H = YesPlease > >>> +HAVE_STRINGS_H = YesPlease > >>> NEEDS_LIBICONV = YesPlease > >>> NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease > >>> NO_SYS_SELECT_H = UnfortunatelyYes > >> > >> If NONSTOP_KERNEL is the platform that defines __TANDEM, then this > >> should be squashed into the previous patch, shouldn't it? > > > > Patch 4/4 does not work without 3/4, Not for HP-NonStop. > > That is clear from the order of the patches in the series. > > To put it in a different way: Do all supported platforms still work > after 3/4, but without 4/4? Sorry I didn't make myself clear, I left out a "and vice versa" Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] Port to QNX
Matt Kraai wrote: From: Matt Kraai Signed-off-by: Matt Kraai --- Makefile | 19 +++ git-compat-util.h | 8 ++-- 2 files changed, 25 insertions(+), 2 deletions(-) snip diff --git a/git-compat-util.h b/git-compat-util.h index 2e79b8a..6c588ca 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -75,7 +75,7 @@ # endif #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \ !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \ - !defined(__TANDEM) + !defined(__TANDEM) && !defined(__QNX__) #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 for S_ISLNK() */ #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ #endif @@ -99,7 +99,7 @@ #include #include #include -#ifdef __TANDEM /* or HAVE_STRINGS_H or !NO_STRINGS_H? */ +#if defined(__TANDEM) || defined(__QNX__) /* or HAVE_STRINGS_H or !NO_STRINGS_H? */ #include /* for strcasecmp() */ There's another recent thread, discussing to change this to "#ifdef HAVE_STRING_H" plus the infrastructure in Makefile and configure.ac. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Dec 2012, #04; Sun, 16)
Matt Kraai wrote: Junio C Hamano wrote: It could turn out that we may be able to get rid of sys/param.h altogether, but one step at a time. Inputs from people on minority platforms are very much appreciated---does your platform build fine when the inclusion of the file is removed from git-compat-util.h? QNX builds fine when sys/param.h is not included. HP-NonStop build fine too without it. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] mergetools/p4merge: Honor $TMPDIR for the /dev/null placeholder
Junio C Hamano wrote: David Aguilar writes: Use mktemp to create the /dev/null placeholder for p4merge. This keeps it out of the current directory. Reported-by: Jeremy Morton Signed-off-by: David Aguilar --- I consider this a final finishing touch on a new 1.8.1 feature, so hopefully we can get this in before 1.8.1. Does everybody have mktemp(1), which is not even in POSIX.1? HP-NonStop doesn't have it, unless you're on the latest release, which added GNU coreutils. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Python version auditing followup
Junio C Hamano wrote: e...@thyrsus.com (Eric S. Raymond) writes: That was the first of three patches I have promised. In order to do the next one, which will be a development guidelines recommend compatibility back to some specific version X, I need a policy decision. How do we set X? I don't think X can be < 2.4, nor does it need to be - 2.4 came out in 2004 and eight years is plenty of deployment time. The later we set it, the more convenient for developers. But of course by setting it late we trade away some portability to older systems. In previous discussion of this issue I recommended X = 2.6. That is still my recommendation. Thoughts, comments, objections? I personally would think 2.6 is recent enough. Which platforms that are long-term-maintained by their vendors still pin their Python at 2.4.X? 2.4.6 was in 2008 that was source only, 2.4.4 was in late 2006 that was the last 2.4 with binary release. Objections? Comments? We have a working 2.4.2 for HP-NonStop and some major problems getting 2.7.3 to work. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Python version auditing followup
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Thursday, December 20, 2012 10:39 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: Python version auditing followup > > "Joachim Schmitz" writes: > > > Junio C Hamano wrote: > >> I personally would think 2.6 is recent enough. Which platforms that > >> are long-term-maintained by their vendors still pin their Python at > >> 2.4.X? 2.4.6 was in 2008 that was source only, 2.4.4 was in late > >> 2006 that was the last 2.4 with binary release. > >> > >> Objections? Comments? > > > > We have a working 2.4.2 for HP-NonStop and some major problems getting > > 2.7.3 to work. > > I do not think a platform that stops at 2.4.2 instead of going to > higher 2.4.X series deserves to be called "long term maintained by > their vendors". It sounds more like "attempted to supply 2.4.X and > abandoned the users once one port was done" to me. Well, not entirely wrong, but not all true at too. I guess I need to defend the vendor here: It is not really the Vendor (HP) that provided Python 2.4.2 or tries to provide 2.7.3, it is a volunteer and community effort. HP did sponsor the 2.4.2 port though (by allowing an HP employee to do the port inn his regular working hours). It is not doing this any longer (since 2007). Since then it is a small group doing this on a purely voluntary basis in their spare time (one HP employee amongst them, me). Same goes for the git port BTW. And for every other port provided on http://ituglib.connect-cummunity.org (this machine is sponsored by HP). Almost every other port, as some pretty recently made it into the officially supported O/S version, so far: Samba, bash, coreutils, vim, gzip, bzip2, Perl, PHP. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Python version auditing followup
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, December 21, 2012 7:28 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: Python version auditing followup > > "Joachim Schmitz" writes: > > >> > We have a working 2.4.2 for HP-NonStop and some major problems getting > >> > 2.7.3 to work. > >> > >> I do not think a platform that stops at 2.4.2 instead of going to > >> higher 2.4.X series deserves to be called "long term maintained by > >> their vendors". It sounds more like "attempted to supply 2.4.X and > >> abandoned the users once one port was done" to me. > > > > Well, not entirely wrong, but not all true at too. > > I guess I need to defend the vendor here: It is not really the > > Vendor (HP) that provided Python 2.4.2 or tries to provide 2.7.3, > > it is a volunteer and community effort. HP did sponsor the 2.4.2 > > port though (by allowing an HP employee to do the port inn his > > regular working hours). It is not doing this any longer (since > > 2007). Since then it is a small group doing this on a purely > > voluntary basis in their spare time (one HP employee amongst them, > > me). Same goes for the git port BTW. > > For the purpose of "if we draw the line at 2.6, would it hurt many > people who have been happily using the existing release of Git that > was happy with 2.4", it is dubious HP-NonStop counts. It is not > like the users on that platform have been happily using Python based > Porcelain at the fringe of Git, and drawing the line at 2.6 will not > give them any regression. You asked for opions and obhections, you got mine ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Test failures with python versions when building git 1.8.1
Jeff King wrote:
On Tue, Jan 01, 2013 at 11:18:46PM -0800, Junio C Hamano wrote:
Jeff King writes:
[1] This symlink is doubly wrong, because any use of symbolic links
in the test scripts needs to depend on the SYMLINKS prereq, and
this does not.
Yeah, I think we have discussed this once already in
http://thread.gmane.org/gmane.comp.version-control.git/210688/focus=210714
Thanks for the pointer; it looks like nothing productive came of the
earlier discussion. To give a hat trick of failure to this line of
code, I notice that the existing code also does not properly put
quotes around $GIT_BUILD_DIR.
[2] In both the current code and what I showed above, the test
scripts depend on things in contrib/. This is probably a bad
idea in general, as the quality of what goes into contrib is
not as closely watched (especially with respect to things like
portability). Certainly I would not have known to look more
carefully at a patch to contrib/svn-fe for breakage to the test
suite.
As long as such tests are made skippable with appropriate
prerequisites, I do not think it is bad to have their tests in t/; I
would say it is rather better than having them in contrib/ and leave
it not run by anybody, which happened to some of the stuff in
contrib/ already.
Good point. While my sense of decorum wants to keep contrib totally
split out, from a practical point of view, it is better to have more
people run the tests and report failures than not.
Whether we end up doing something with contrib and tests or not, the
patch below gives a minimal fix in the meantime. Dan, does it fix your
problem?
-- >8 --
Subject: [PATCH] t9020: don't run python from $PATH
In t9020, we symlink in a python script from contrib to help
with the testing. However, we don't munge its #!-line, which
means we may run the wrong python (we want the one in
PYTHON_PATH). On top of this, we use a symlink without
checking the SYMLINKS prereq, and we fail to properly quote
GIT_BUILD_DIR, which may have spaces.
Instead of symlinking, let's just write a small script which
will feed the contrib script to PYTHON_PATH. To avoid
quoting issues, we just export the variables the script
needs to run.
Signed-off-by: Jeff King
---
t/t9020-remote-svn.sh | 5 -
t/test-lib.sh | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/t/t9020-remote-svn.sh b/t/t9020-remote-svn.sh
index 4f2dfe0..416623b 100755
--- a/t/t9020-remote-svn.sh
+++ b/t/t9020-remote-svn.sh
@@ -14,7 +14,10 @@ export PATH="$HOME:$PATH"
# We override svnrdump by placing a symlink to the svnrdump-emulator
in . export PATH="$HOME:$PATH"
With this patch that comment is no longer true.
-ln -sf $GIT_BUILD_DIR/contrib/svn-fe/svnrdump_sim.py "$HOME/svnrdump"
+export GIT_BUILD_DIR
+write_script svnrdump <<\EOF
+exec "$PYTHON_PATH" "$GIT_BUILD_DIR"/contrib/svn-fe/svnrdump_sim.py
"$@" +EOF
init_git () {
rm -fr .git &&
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] archive-tar: fix sanity check in config parsing
Jeff King wrote:
On Sun, Jan 13, 2013 at 06:42:01PM +0100, René Scharfe wrote:
When parsing these config variable names, we currently check that
the second dot is found nine characters into the name, disallowing
filter names with a length of five characters. Additionally,
git archive crashes when the second dot is omitted:
$ ./git -c tar.foo=bar archive HEAD >/dev/null
fatal: Data too large to fit into virtual memory space.
Instead we should check if the second dot exists at all, or if
we only found the first one.
Eek. Thanks for finding it. Your fix is obviously correct.
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -335,7 +335,7 @@ static int tar_filter_config(const char *var,
const char *value, void *data) if (prefixcmp(var, "tar."))
return 0;
dot = strrchr(var, '.');
- if (dot == var + 9)
+ if (dot == var + 3)
return 0;
For the curious, the original version of the patch[1] read:
+ if (prefixcmp(var, "tarfilter."))
+ return 0;
+ dot = strrchr(var, '.');
+ if (dot == var + 9)
+ return 0;
and when I shortened the config section to "tar" in a re-roll of the
series, I missed the corresponding change to the offset.
Wouldn't it then be better ti use strlen("tar") rather than a 3? Or at least
a comment?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: erratic behavior commit --allow-empty
Jan Engelhardt wrote: On Tuesday 2012-10-02 10:26, Johannes Sixt wrote: Note that git commit -m A --allow-empty *DID* create a commit. Only, that it received the same name (SHA1) as the commit you created before it because it had the exact same contents (files, parents, author, committer, and timestamps). Obviously, your script was executed sufficiently fast that the two commits happend in the same second. What about introducing nanosecond-granular timestamps into Git? Not every platform (supported by git) does have a nanosecond clock resolution Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fast-import: Remove redundant assignment of 'oe' to itself.
Stefano Lattarini wrote:
On 05/26/2013 10:05 PM, Stefan Beller wrote:
Reported by cppcheck.
Signed-off-by: Stefan Beller
---
fast-import.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fast-import.c b/fast-import.c
index 5f539d7..0142e3a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2914,7 +2914,7 @@ static void cat_blob(struct object_entry *oe,
unsigned char sha1[20]) static void parse_cat_blob(void)
{
const char *p;
- struct object_entry *oe = oe;
This was done on purpose, to avoid spurious warnings with (at least)
some versions of GCC.
+ struct object_entry *oe;
unsigned char sha1[20];
/* cat-blob SP LF */
This strange construct has been removed in other places meanwhile. It is
violating C-standards (C89, C99) and as such causes warnings with other
compilers, so this is fighting fire with fire. As it is a pointer it may be
more sensible to initialize with NULL, should appease all compilers and
still be correct.
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] cherry-pick: add --skip-commits option
Felipe Contreras wrote: Pretty much what it says on the tin. Only that it add --skip-empty and not --skip-commit ?!? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] sequencer: trivial fix
Felipe Contreras wrote:
Junio C Hamano wrote:
Neil Horman writes:
On Mon, May 27, 2013 at 11:52:18AM -0500, Felipe Contreras wrote:
We should free objects before leaving.
Signed-off-by: Felipe Contreras
---
sequencer.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index ab6f8a7..7eeae2f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -626,12 +626,15 @@ static int do_pick_commit(struct commit
*commit, struct replay_opts *opts)
rerere(opts->allow_rerere_auto); } else {
int allow = allow_empty(opts, commit);
- if (allow < 0)
- return allow;
+ if (allow < 0) {
+ res = allow;
+ goto leave;
+ }
if (!opts->no_commit)
res = run_git_commit(defmsg, opts, allow);
}
+leave:
free_message(&msg);
free(defmsg);
--
1.8.3.rc3.312.g47657de
Acked-by: Neil Horman
This is better done without "goto" in general.
The other patch 2/2/ adds one more "we need to exit from the middle
of the flow" and makes it look handier to add an exit label here,
but it would be even better to express the logic of that patch as a
normal cascade of if/else if/..., which is small enough and we do
not need the "leave:" label.
Linux kernel developers would disagree. In C 'goto' is quite of then
the only sane option, and you can see 'goto' used in the Linux kernel
all over the place for that reason.
In this particular case it also makes perfect sense.
It probably is better to fold this patch into the other one when it
is rerolled to correct the option name gotcha "on the tin".
Why? This patch is standalone and fixes an issue that is independent
of the other patch. Why squash two patches that do *two* different
things?
Anyway, I'll happily drop this patch if you want this memory leak to
remain. But then I'll do the same in the other patch.
This mantra of avodiing 'goto' is not helping anybody.
adding 5 letters (to change the next "if" into an "else if") versus your
addition of several lines and some 15 additional letters (ignoring the
whitsspace) is IMHO enough to see what is better?
bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] sequencer: trivial fix
> From: Felipe Contreras [mailto:felipe.contre...@gmail.com] > Sent: Wednesday, May 29, 2013 12:52 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH 1/2] sequencer: trivial fix > > On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz > wrote: > > Felipe Contreras wrote: > >> > >> Junio C Hamano wrote: > > >>> It probably is better to fold this patch into the other one when it > >>> is rerolled to correct the option name gotcha "on the tin". > >> > >> > >> Why? This patch is standalone and fixes an issue that is independent > >> of the other patch. Why squash two patches that do *two* different > >> things? > >> > >> Anyway, I'll happily drop this patch if you want this memory leak to > >> remain. But then I'll do the same in the other patch. > >> > >> This mantra of avodiing 'goto' is not helping anybody. > > > > > > adding 5 letters (to change the next "if" into an "else if") versus your > > addition of several lines and some 15 additional letters (ignoring the > > whitsspace) is IMHO enough to see what is better? > > This has nothing to do with what Junio said. Well, it has, but you had snipped it. But replied to the goto issue regardless > This is better done without "goto" in general. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] sequencer: trivial fix
> From: Felipe Contreras [mailto:felipe.contre...@gmail.com] > Sent: Wednesday, May 29, 2013 1:24 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH 1/2] sequencer: trivial fix > > On Wed, May 29, 2013 at 6:13 AM, Joachim Schmitz > wrote: > >> From: Felipe Contreras [mailto:felipe.contre...@gmail.com] > >> Sent: Wednesday, May 29, 2013 12:52 PM > >> To: Joachim Schmitz > >> Cc: git@vger.kernel.org > >> Subject: Re: [PATCH 1/2] sequencer: trivial fix > >> > >> On Wed, May 29, 2013 at 4:58 AM, Joachim Schmitz > >> wrote: > >> > Felipe Contreras wrote: > >> >> > >> >> Junio C Hamano wrote: > >> > >> >>> It probably is better to fold this patch into the other one when it > >> >>> is rerolled to correct the option name gotcha "on the tin". > >> >> > >> >> > >> >> Why? This patch is standalone and fixes an issue that is independent > >> >> of the other patch. Why squash two patches that do *two* different > >> >> things? > >> >> > >> >> Anyway, I'll happily drop this patch if you want this memory leak to > >> >> remain. But then I'll do the same in the other patch. > >> >> > >> >> This mantra of avodiing 'goto' is not helping anybody. > >> > > >> > > >> > adding 5 letters (to change the next "if" into an "else if") versus your > >> > addition of several lines and some 15 additional letters (ignoring the > >> > whitsspace) is IMHO enough to see what is better? > >> > >> This has nothing to do with what Junio said. > > > > Well, it has, but you had snipped it. But replied to the goto issue > > regardless > > I didn't snip anything, this is a different context. You did in your reply to me > >> This is better done without "goto" in general. > > He din't say: > __ > It probably is better to fold this patch into the other one when it > is rerolled to correct the option name gotcha "on the tin", AND you > fix the goto issue. > __ > > You added that last part in your mind. Moreover, he didn't say goto > was an issue, he simply stated an opinion about some generality. I added nothing in my mind, I just copy/paste that statement and was commenting on that and only that. At least intended to. Whenever anybody added more else branches, that's the time to possible switch to the goto style. And for the record: I agree with you that these 2 things should rather not be in a single patch as they are completely unrelated. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] sequencer: trivial fix
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de] > Sent: Wednesday, May 29, 2013 1:30 PM > To: 'Felipe Contreras' > Cc: 'git@vger.kernel.org' > Subject: RE: [PATCH 1/2] sequencer: trivial fix > > And for the record: I agree with you that these 2 things should rather not be > in a single patch as they are completely unrelated. I take that back: your patches 'overlap' so the 2nd won't apply without the 1st Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 11/13] remote-hg: force remote push
Jed Brown wrote: Felipe Contreras writes: On Thu, Apr 4, 2013 at 2:48 PM, Jed Brown wrote: ... We have to assume that every Git (remote-hg) User is dealing with Hg Team No, we don't. Really? If there is no Hg Team, why bother with an Hg upstream? Huh? the counterpart of "every user" wpuld be "some users" and not "no user" or "no HG team", isn't it? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Monday, August 20, 2012 4:42 PM > To: Joachim Schmitz > Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbec...@nexbridge.com > Subject: Re: Porting git to HP NonStop > > "Joachim Schmitz" writes: > > > OK, I'll go for a compat/mkdir.c though. > > No. See below. > > > We shouldn't call it tandem.c as Tandem, the Company, doesn't exist > > anymore and since more than a decade (bough by Compaq, then HP), only > > the __TANDEM survived in our compiler and headers/libraries. Could > > call it NonStop.c, but I don't really like that idea either, I'd > > rather keep it more generic, just in case someone else might need it > > too, or that issue someday gets fixed for NonStop. > > compat/hp_nonstop.c is also fine, but I think matching "#ifdef __TANDEM" is > the most sensible. > > And I wouldn't call it just "mkdir", as it is more likely than not that we will find > other incompatibilities that needs to be absorbed in the compat/ layer, and we > can add it to compat/tandem.c, but not to compat/mkdir.c, as that will be > another nonstop specific tweak. I haven't found any other to be needed. Well, poll, maybe, but with only minor tweaks for the win32 one works for me (and those tweaks are compatible with win32 > A separate file, compat/tandem/mkdir.c, is fine, though. > > > I'll go for git_mkdir(), similar to other git wrappers, (like for > > mmap, pread, fopen, snprintf, vsnprintf, qsort). > > Again, no. Your breakage is that having underlying system mkdir that does not > understand trailing slash, which may not be specific to __TANDEM, but still is > _not_ the only possible mode of breakage. Well, it is the only one GNUlib's mkdir caters for and I'd regard that an authoritative source... > Squatting on a generic "git_mkdir()" name makes it harder for other people to > name their compat mkdir functions to tweak for the breakage on their > platforms. The examples you listed are all "the platform does not offer it, so > we implement the whole thing" kind, so it is in a different genre. Nope, git_fopen() definitly is a wrapper for fopen(), as is git_vsnprintf() for vsnprintf(). Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: git on HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Monday, August 20, 2012 6:30 PM
> To: Johannes Sixt
> Cc: Joachim Schmitz; 'Jan Engelhardt'; git@vger.kernel.org
> Subject: Re: git on HP NonStop
>
> Johannes Sixt writes:
>
> > Am 8/20/2012 12:36, schrieb Joachim Schmitz:
> >> int var = var;
> >> char *othervar = othervar;
> >>
> >> ...
> >>
> >> What is the reason for using that self-init stuff? I don't think it
> >> is really portable, is it?
> >
> > It is used to avoid "var may be used uninitialized" warnings for some
> > compilers. Officially (according to the C standard), it is undefined
> > behavior. But I've observed considerable resistance by Junio to fix
> > this properly.
>
> I had resisted
>
> - int foo = foo;
> + int foo = 0;
>
> in the past. If some compiler is not seeing that "foo" is never used
uninitialized,
> such a compiler will generate an unnecessary initialization, so it is not
a
> _proper_ fix anyway (in fact, I do not think a proper fix exists, short of
> simplifying the code so that less sophisticated compilers can see that
"foo" is
> never used uninitialized).
>
> So, no, I never resisted a "proper" fix. I resisted swapping an
unsatisfactory
> workaround with another.
>
> Between the two unsatisfactory workarounds, the latter (explicit and
> unnecessary assignment to an innocuous value) is lessor of two evils, so I
do not
> particularly mind it, though. Indeed, I think more recent history shows
that we
> have such changes.
OK, so let's have a look at code, current git,
builtin/cat-file.c, line 196:
void *contents = contents;
This variable is set later in an if branch (if (print_contents == BATCH),
but not in the else branch. It is later used always under the same condition
as the one under which it is set.
Apparently is is malloc_d storage (there a "free(content);"), so there's no
harm al all in initializing it with NULL, even if it only appeases a stupid
compiler.
The next one, builtin/fast-export.c, line 486:
struct commit *commit = commit;
it is set in a switch statement, but not in every case, as far as I can see.
Hmm, maybe it is, and I just get lost in the code
And it is used directly after the switch, hopefully set to something
reasonable.
Why take the risk and not set it to NULL?
Next one, builtin/rev-list.c, line 390:
int reaches = reaches, all = all;
revs.commits = find_bisection(revs.commits, &reaches, &all,
bisect_find_all);
Seem pretty pointless to initialize them, provided find_bisection doesn't
read them. Does it?
I'm too Next one, lazy to check... I'd just set them to 0 and stop worrying.
Next one, fast-import.c, line 2268:
struct object_entry *oe = oe;
os gets set in en if and an else branch, but not in then intermediate else
if branch!
It is checked for !NULL later, so it should really get initialized to NULL
in the first place!
Same file, line 2437, same variable name, same story!
Same file, line 2616, variable e, it is used in an if branch but set after
that!
Same file again, line 2917, variable oe again. Same story as above.
Next file, ll-merge.c, line
static const struct ll_merge_options default_opts;
Somewhat different story here, compiler warning claims " const variable
"default_opts" requires an initializer"
Possible fix:
static const struct ll_merge_options default_opts = {0};
next file, match-trees.c, line 75ff:
const unsigned char *elem1 = elem1;
const unsigned char *elem2 = elem2;
const char *path1 = path1;
const char *path2 = path2;
unsigned mode1 = mode1;
unsigned mode2 = mode2;
Some get set, some not, depending on code path, but all get used, with
possibly bogus content.
Next file, merge-recursive.c, line 1903:
struct tree *mrtree = mrtree;
passed on my address to another function, which hopefully knows how to treat
it. It woult be learer and simpler to just have
struct tree *mrtree = NULL;
wouldn't it?
Next file, run-command.c, line 272:
int failed_errno = failed_errno;
Set deeply nested in some cases. Seems to be used reasonably, but again, why
take chanses= 0 is a goot errno ;-)
Next file, submodule.c, line 265:
struct commit *left = left, *right = right;
As far as I can see it is not set properly before it gets used in some
cases.
Next filen, transport.c, line 106:
int cmp = cmp, len;
I seem to see code paths whet it is used without being set properly
Next file, vcs-svn/svndiff.c, line 299 oh, that one has been fi
[PATCH] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
Signed-off-by: Joachim Schmitz --- Makefile| 18 ++ compat/win32/poll.c | 8 ++-- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 6b0c961..2af4db3 100644 --- a/Makefile +++ b/Makefile @@ -152,6 +152,11 @@ all:: # # Define NO_MMAP if you want to avoid mmap. # +# Define NO_SYS_POLL_H if you don't have sys/poll.h. +# +# Define NO_POLL if you do not have or do not want to use poll. +# This also implies NO_SYS_POLL_H. +# # Define NO_PTHREADS if you do not have or do not want to use Pthreads. # # Define NO_PREAD if you have a problem with pread() system call (e.g. @@ -1216,7 +1221,7 @@ ifeq ($(uname_S),Windows) NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease - NO_SYS_POLL_H = YesPlease + NO_POLL = YesPlease NO_SYMLINK_HEAD = YesPlease NO_IPV6 = YesPlease NO_UNIX_SOCKETS = YesPlease @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows) BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE COMPAT_OBJS = compat/msvc.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ - compat/win32/poll.o compat/win32/dirent.o + compat/win32/dirent.o COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\" BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib @@ -1312,7 +1317,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease - NO_SYS_POLL_H = YesPlease + NO_POLL = YesPlease NO_SYMLINK_HEAD = YesPlease NO_UNIX_SOCKETS = YesPlease NO_SETENV = YesPlease @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ - compat/win32/poll.o compat/win32/dirent.o + compat/win32/dirent.o EXTLIBS += -lws2_32 PTHREAD_LIBS = X = .exe @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT USE_GETTEXT_SCHEME ?= fallthrough endif +ifdef NO_POLL + NO_SYS_POLL_H = YesPlease + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it find poll.h + COMPAT_OBJS += compat/win32/poll.c +endif ifdef NO_STRCASESTR COMPAT_CFLAGS += -DNO_STRCASESTR COMPAT_OBJS += compat/strcasestr.o diff --git a/compat/win32/poll.c b/compat/win32/poll.c index 403eaa7..49541f1 100644 --- a/compat/win32/poll.c +++ b/compat/win32/poll.c @@ -24,7 +24,9 @@ # pragma GCC diagnostic ignored "-Wtype-limits" #endif -#include +#if defined(WIN32) +# include +#endif #include @@ -48,7 +50,9 @@ #else # include # include -# include +# ifndef NO_SYS_SELECT_H +# include +# endif # include #endif -- 1.7.12 HP NonStop is using this, patch to add this is to follow. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
Signed-off-by: Joachim Schmitz
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index b61ac85..18bc6bf 100644
--- a/http.c
+++ b/http.c
@@ -806,10 +806,12 @@ static int http_request(const char *url, void *result,
int target, int options)
ret = HTTP_REAUTH;
}
} else {
+#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
curl_easy_strerror(results.curl_result),
sizeof(curl_errorstr));
+#endif
ret = HTTP_ERROR;
}
} else {
--
1.7.12
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
Signed-off-by: Joachim Schmitz
---
sha1_file.c | 4
1 file changed, 4 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..76714ad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
return error("packfile %s index unavailable", p->pack_name);
if (!pack_max_fds) {
+#ifdef _SC_OPEN_MAX
+ unsigned int max_fds = sysconf(_SC_OPEN_MAX);
+#else
struct rlimit lim;
unsigned int max_fds;
@@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
die_errno("cannot get RLIMIT_NOFILE");
max_fds = lim.rlim_cur;
+#endif
/* Save 3 for stdin/stdout/stderr, 22 for work */
if (25 < max_fds)
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Monday, August 20, 2012 6:54 PM > To: Joachim Schmitz > Cc: 'Shawn Pearce'; git@vger.kernel.org; rsbec...@nexbridge.com > Subject: Re: Porting git to HP NonStop > > "Joachim Schmitz" writes: > > > I haven't found any other to be needed. Well, poll, maybe, but with > > only minor tweaks for the win32 one works for me (and those tweaks are > > compatible with win32 > > > >> A separate file, compat/tandem/mkdir.c, is fine, though. > > If you wouldn't have dozens of them, so compat/tandem/mkdir.c is not suitable; > compat/tandem.c would be good, then. > > >> > I'll go for git_mkdir(), similar to other git wrappers, (like for > >> > mmap, pread, fopen, snprintf, vsnprintf, qsort). > >> > >> Again, no. Your breakage is that having underlying system mkdir that > >> does > > not > >> understand trailing slash, which may not be specific to __TANDEM, but > > still is > >> _not_ the only possible mode of breakage. True. > > Well, it is the only one GNUlib's mkdir caters for and I'd regard that > > an authoritative source... > > I suspect that you may be misunderstanding what compat/ is about I don't think so, it server the same purpose for git as gnulib does for others. >, so let's try again. > Platform difference in mkdir may not be limited to "on this platform, the > underlying one supplied by the system does not like path ending with a slash". > > What I am saying is that it is unacceptable to call something that caters to that > specific kind of difference from what the codebase expects with a generic > name such as "git_mkdir()". Look at mingw's replacement. The platform > difference over there is that the one from the system does not take mode > parameter. Imagine that one was already called "git_mkdir()". Now we have > two different kind of differences, and one has more officially-looking > "git_mkdir()" name; yours cannot take it---what would you do in that case? > Neither kind of difference is more officially sanctioned difference; don't call > yours any more official/generic than necessary. Gnulib's rpl_mkdir caters for 3 possible problems, the WIN32 one which mkdir taking only one argument, the trailing slash one discussed here (victims being at least NetBSD 1.5.2 and current HP NonStop) and a trailing dot one (that allegedly Cygwin 1.5 suffered from). As far as I can see git will not suffer from the latter, but even if, at that time a git_mkdir() could be expanded to cater for this to, just like gnulib's one does, there it is an additional section inside their rpl_mkdir(). And the WIN32 one is already being taken care of in compat/mingw.h. However, this could as easily get integrated into a git_mkdir(), just like in gnulib. > Your wrapper is not limited to tandem, but is applicable to ancient BSDs, so it is > fine to call it as compat_mkdir_wo_trailing_slash(), so that it can be shared > among platforms whose mkdir do not want to see trailing slashes. If you are > going that route, the function should live in its own file (without any other > wrapper), and not be named after specific platform (should be named after the > specific difference from what we expect, instead). I am perfectly fine with that > approach as well. > > >> Squatting on a generic "git_mkdir()" name makes it harder for other > >> people > > to > >> name their compat mkdir functions to tweak for the breakage on their > >> platforms. The examples you listed are all "the platform does not > >> offer > > it, so > >> we implement the whole thing" kind, so it is in a different genre. > > > > Nope, git_fopen() definitly is a wrapper for fopen(), as is > > git_vsnprintf() for vsnprintf(). > > I was talking more about mmap() and pread(). > > For the two you mentioned, ideally they should have been named after the > specific breakages they cover (fopen that does not error out on directories is > primarily AIX thing IIRC, and snprintf returns bogus result are shared between > HPUX and Windows), but over these years we haven't seen any other kind of > differences from various platforms, so the need to rename them away is very > low. > > On the other hand, we already know there are at least two kinds of platform > mkdir() that need different compat/ layer support, so calling one "git_mkdir()" > to cover one particular kind of difference does not make any sense. > > Besides, an earlier mistake is not a valid excuse to add new mistakes. OK, so how about this:
RE: git on HP NonStop
> -Original Message- > From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Tuesday, August 21, 2012 4:06 AM > To: Joachim Schmitz > Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org > Subject: Re: git on HP NonStop > > "Joachim Schmitz" writes: > > > OK, so let's have a look at code, current git, builtin/cat-file.c, > > line 196: > > void *contents = contents; > > > > This variable is set later in an if branch (if (print_contents == > > BATCH), but not in the else branch. It is later used always under the > > same condition as the one under which it is set. > > Apparently is is malloc_d storage (there a "free(content);"), so > > there's no harm al all in initializing it with NULL, even if it only > > appeases a stupid compiler. > > It actually is harmful. See below. Harmful to initialize with NULL or to use that undefined behavoir? I checked what our compiler does here: after having warned about "vlues us used before it is set: it actually dies seem to have initializes the value to 0 resp. NULL. So here there's no harm done in avoiding undefined behavior and set it to 0 resp NULL in the first place. > > The next one, builtin/fast-export.c, line 486: > > struct commit *commit = commit; it is set in a switch > > statement, but not in every case, as far as I can see. > > Hmm, maybe it is, and I just get lost in the code And it is used > > directly after the switch, hopefully set to something reasonable. > > Why take the risk and not set it to NULL? > > Ditto. > > > Next one, builtin/rev-list.c, line 390: > > int reaches = reaches, all = all; > > > > revs.commits = find_bisection(revs.commits, &reaches, &all, > > bisect_find_all); > > > > Seem pretty pointless to initialize them, provided find_bisection > > doesn't read them. Does it? > > That is why they are not initializations but marks to the compiler to signal "you > may be stupid enough to think they are used before initialized or assigned, but > that is not the case". Initializing them would be pointless. > > > Next one, fast-import.c, line 2268: > > struct object_entry *oe = oe; > > > > os gets set in en if and an else branch, but not in then intermediate > > else if branch! > > Look again. If the recent code is too complex for you to understand, go back to > 10e8d68 (Correct compiler warnings in fast-import., 2007-02-06) and read the > function. > > The control flow of the early part of that function dictates that either oe is > assigned *or* inline_data is set to 1. When inline_data is false, oe is always > set. > > The compiler was too stupid to read that, and that is why the > (confusing) idiom to mark it for the stupid compiler was used. > > There are a few reasons why I do not think this self-assignment idiom or > initializing the variable to an innocuous-looking random value is a particularly > good thing to do when you see compiler warnings. > > If the compiler suspects the variable might be unused, you should always look > at it and follow the flow yourself. Once you know it is a false alarm, you can > use the idiom to squelch the warning, and it at the same serves as a note to > others that you verified the flow and made sure it is a false warning. > > When the next person wants to touch the code, if the person knows the use of > the idiom, it only serves as a warning to be extra careful not to introduce a new > codepath that reads the variable without setting, as the compiler no longer > helps him. If the person who touches the code is as clueless as the compiler > and cannot follow the codepath to see the variable is never used uninitialized, > the result will be a lot worse. > > That is the reason why I do not think the idiom to squelch the compiler is such a > good thing. Careless people touch the code, so "oe = oe" initialization carefully > placed in the original version does not necessarily stay as a useful > documentation. > > But if you use "oe = NULL" as a way to squelch the warning in the first place, it > is no better than "oe = oe". In a sense, it is even worse, as it just looks like any > other initialization and gives a false impression that the remainder of the code > is written in such a way that it tolerates oe being NULL in any codepath, or > there is some assignment before that before the code reaches places where oe > cannot be NULL. That is different from what "oe = oe" initializaion documents-- &
RE: Porting git to HP NonStop
> From: Brandon Casey [mailto:draf...@gmail.com]
> Sent: Wednesday, August 22, 2012 7:01 PM
> To: Joachim Schmitz
> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> rsbec...@nexbridge.com
> Subject: Re: Porting git to HP NonStop
>
> On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
> wrote:
>
> > OK, so how about this:
> > /usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
> > --- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
> > +++ ./compat/mkdir.c2012-08-21 05:02:11 -0500
> > @@ -0,0 +1,24 @@
> > +#include "../git-compat-util.h"
> > +#undef mkdir
> > +
> > +/* for platforms that can't deal with a trailing '/' */ int
> > +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
> > + int retval;
> > + char *tmp_dir = NULL;
> > + size_t len = strlen(dir);
> > +
> > + if (len && dir[len-1] == '/') {
> > + if ((tmp_dir = strdup(dir)) == NULL)
> > + return -1;
> > + tmp_dir[len-1] = '\0';
> > + }
> > + else
> > + tmp_dir = (char *)dir;
> > +
> > + retval = mkdir(tmp_dir, mode);
> > + if (tmp_dir != dir)
> > + free(tmp_dir);
> > +
> > + return retval;
> > +}
>
> Why not rearrange this so that you assign to dir the value of tmp_dir and then
> just pass dir to mkdir. Then you can avoid the recast of dir to (char*) in
> the
> else branch. Later, just call free(tmp_dir). Also, we have xstrndup. So I
> think
> the body of your function can become something like:
>
>if (len && dir[len-1] == '/')
>dir = tmp_dir = xstrndup(dir, len-1);
xstndup() can't fail?
>retval = mkdir(dir, mode);
>free(tmp_dir);
>
> -Brandon
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Wednesday, August 22, 2012 7:19 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
>
> "Joachim Schmitz" writes:
>
> > Signed-off-by: Joachim Schmitz
>
> At the very least, please mention that this reverts be22d92 (http:
> avoid empty error messages for some curl errors, 2011-09-05) on platforms
> with older versions of libcURL.
a) Of course I didn't know that, thanks for telling me.
b) How and where to add that? In the Subject of an email, in the body, right
at the front?
Like this:
This reverts be22d92 (http: avoid empty error messages for some curl errors,
2011-09-05) on platforms with older versions of libcURL.
Signed-off-by: Joachim Schmitz
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index b61ac85..18bc6bf 100644
--- a/http.c
+++ b/http.c
@@ -806,10 +806,12 @@ static int http_request(const char *url, void *result,
int target, int options)
ret = HTTP_REAUTH;
}
} else {
+#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
curl_easy_strerror(results.curl_result),
sizeof(curl_errorstr));
+#endif
ret = HTTP_ERROR;
}
} else {
--
1.7.12
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> From: Brandon Casey [mailto:draf...@gmail.com]
> Sent: Wednesday, August 22, 2012 7:23 PM
> To: Joachim Schmitz
> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> rsbec...@nexbridge.com
> Subject: Re: Porting git to HP NonStop
>
> On Wed, Aug 22, 2012 at 10:18 AM, Joachim Schmitz
> wrote:
> >> From: Brandon Casey [mailto:draf...@gmail.com]
> >> Sent: Wednesday, August 22, 2012 7:01 PM
> >> To: Joachim Schmitz
> >> Cc: Junio C Hamano; Shawn Pearce; git@vger.kernel.org;
> >> rsbec...@nexbridge.com
> >> Subject: Re: Porting git to HP NonStop
> >>
> >> On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
> >>
> >> wrote:
> >>
> >> > OK, so how about this:
> >> > /usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
> >> > --- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
> >> > +++ ./compat/mkdir.c2012-08-21 05:02:11 -0500
> >> > @@ -0,0 +1,24 @@
> >> > +#include "../git-compat-util.h"
> >> > +#undef mkdir
> >> > +
> >> > +/* for platforms that can't deal with a trailing '/' */ int
> >> > +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
> >> > + int retval;
> >> > + char *tmp_dir = NULL;
> >> > + size_t len = strlen(dir);
> >> > +
> >> > + if (len && dir[len-1] == '/') {
> >> > + if ((tmp_dir = strdup(dir)) == NULL)
> >> > + return -1;
> >> > + tmp_dir[len-1] = '\0';
> >> > + }
> >> > + else
> >> > + tmp_dir = (char *)dir;
> >> > +
> >> > + retval = mkdir(tmp_dir, mode);
> >> > + if (tmp_dir != dir)
> >> > + free(tmp_dir);
> >> > +
> >> > + return retval;
> >> > +}
> >>
> >> Why not rearrange this so that you assign to dir the value of tmp_dir
> >> and then just pass dir to mkdir. Then you can avoid the recast of
> >> dir to (char*) in the else branch. Later, just call free(tmp_dir).
> >> Also, we have xstrndup. So I think the body of your function can become
> something like:
> >>
> >>if (len && dir[len-1] == '/')
> >>dir = tmp_dir = xstrndup(dir, len-1);
> >
> > xstndup() can't fail?
>
> Correct. It will either succeed or die. It will also try to free up some
> memory
> used by git if possible.
OK. So let's use that then.
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Wednesday, August 22, 2012 7:13 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; Erik Faye-Lund; Johannes Sixt; Marius Storm-Olsen > Subject: Re: [PATCH] Support non-WIN32 system lacking poll() while keeping > the WIN32 part intact > > "Joachim Schmitz" writes: > > > Signed-off-by: Joachim Schmitz > > --- > > Makefile| 18 ++ > > compat/win32/poll.c | 8 ++-- > > 2 files changed, 20 insertions(+), 6 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 6b0c961..2af4db3 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -152,6 +152,11 @@ all:: > > # > > # Define NO_MMAP if you want to avoid mmap. > > # > > +# Define NO_SYS_POLL_H if you don't have sys/poll.h. > > +# > > +# Define NO_POLL if you do not have or do not want to use poll. > > +# This also implies NO_SYS_POLL_H. > > Do you really need to have both? I suspect "If you do not have a usable > sys/poll.h, set NO_SYS_POLL_H" may be a simpler alternative, but there must Hmm, Not having and not having poll() are different thinks, aren't they? Using NO_SYS_POL_H so far only affects BASIC_CFLAGS: ifdef NO_SYS_POLL_H BASIC_CFLAGS += -DNO_SYS_POLL_H endif It does not add compat/win32/poll.c to COMPAT_OBJS, NO_POLL does that. > be a reason why you had to add a new one instead of going that route. It would > be a good idea to describe that reason in the log message above, in the space > before your Sign-off. > > > @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows) > > BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild > > -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H > > -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE > > COMPAT_OBJS = compat/msvc.o compat/winansi.o \ > > compat/win32/pthread.o compat/win32/syslog.o \ > > - compat/win32/poll.o compat/win32/dirent.o > > + compat/win32/dirent.o > > COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI - > DHAVE_STRING_H > > ... > > @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) > > COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" > > COMPAT_OBJS += compat/mingw.o compat/winansi.o \ > > compat/win32/pthread.o compat/win32/syslog.o \ > > - compat/win32/poll.o compat/win32/dirent.o > > + compat/win32/dirent.o > > EXTLIBS += -lws2_32 > > ... > > @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT ... > > +ifdef NO_POLL > > + NO_SYS_POLL_H = YesPlease > > + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it find poll.h > > + COMPAT_OBJS += compat/win32/poll.c endif > > In general, I think this is a good direction to go. If the existing emulation in > win32/poll.c turns out to be usable across platforms and not windows specific, > sharing it would be a good idea. > > But if the emulation is no longer windows specific, shouldn't you also move it > outside compat/win32/ and somewhere more generic? Should be possible. Esp. as with the current setup make issues a warning: Makefile:2329: target `compat/win32/poll.c' doesn't match the target pattern Haven't yet been able to spot where that comes from. > > diff --git a/compat/win32/poll.c b/compat/win32/poll.c index > > 403eaa7..49541f1 100644 > > --- a/compat/win32/poll.c > > +++ b/compat/win32/poll.c > > @@ -24,7 +24,9 @@ > > # pragma GCC diagnostic ignored "-Wtype-limits" > > #endif > > > > -#include > > +#if defined(WIN32) > > +# include > > +#endif > > Hrm, are the Windows folks OK with this? MINGW and MSVC are affected; > Cygwin should be OK, I think. I stole that define from another place in git (compat/nedmalloc/nedmalloc.c" line 36), there too it was used to hide , so I assumed it to be the proper method? > > #include > > > > @@ -48,7 +50,9 @@ > > #else > > # include > > # include > > -# include > > +# ifndef NO_SYS_SELECT_H > > +# include > > +# endif > > # include > > #endif -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Wednesday, August 22, 2012 7:23 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over
> getrlimit(RLIMIT_NOFILE,...)
>
> "Joachim Schmitz" writes:
>
> > Signed-off-by: Joachim Schmitz
> > ---
> > sha1_file.c | 4
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/sha1_file.c b/sha1_file.c index af5cfbd..76714ad 100644
> > --- a/sha1_file.c
> > +++ b/sha1_file.c
> > @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
> > return error("packfile %s index unavailable",
> > p->pack_name);
> >
> > if (!pack_max_fds) {
> > +#ifdef _SC_OPEN_MAX
> > + unsigned int max_fds = sysconf(_SC_OPEN_MAX); #else
> > struct rlimit lim;
> > unsigned int max_fds;
> >
> > @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
> > die_errno("cannot get RLIMIT_NOFILE");
> >
> > max_fds = lim.rlim_cur;
> > +#endif
> >
> > /* Save 3 for stdin/stdout/stderr, 22 for work */
> > if (25 < max_fds)
> > --
> > 1.7.12
>
> Looks sane but it would be more readable to make this a small helper
function,
> so that we do not need to have #ifdef/#endif in the primary flow of the
code.
Hmm, in compat/? Worth the effort fort hat single occrence?
> By the way, I noticed that you seem to be sending patches out of git,
instead of
> "diff -ru", which is a good sign ;-).
Not quite, I'm generating them with "git format-patch origin", on the
NonStop machine, but can't send email from there (a) behind a firewall and
b) no email client available), so I copy/paste the resulting file into
Outlook.
>But all of your patches are whitespace
> damaged and cannot be applied X-<.
May well be Outlooks fault? How to solve?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Wednesday, August 22, 2012 7:41 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
>
> "Joachim Schmitz" writes:
>
> > Like this:
> >
> >
> > This reverts be22d92 (http: avoid empty error messages for some curl
> > errors,
> > 2011-09-05) on platforms with older versions of libcURL.
> >
> > Signed-off-by: Joachim Schmitz
> > ---
>
> Perfect ;-)
>
> > http.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/http.c b/http.c
> > index b61ac85..18bc6bf 100644
> > --- a/http.c
> > +++ b/http.c
> > @@ -806,10 +806,12 @@ static int http_request(const char *url, void
> > *result, int target, int options)
> > ret = HTTP_REAUTH;
> > }
>
> Now we need to see where these whitespace breakages are coming from and
> fix it, and I think it should be done earlier than later, as I expect we
will be
> seeing more patches from you in the coming weeks.
OK, next attempt (this time I downloaded the patch file to my PC first and
used Wordpad for copy/past rather than cat in a PuTTY session)
---
This reverts be22d92 (http: avoid empty error messages for some curl errors,
2011-09-05) on platforms with older versions of libcURL.
Signed-off-by: Joachim Schmitz
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index b61ac85..18bc6bf 100644
--- a/http.c
+++ b/http.c
@@ -806,10 +806,12 @@ static int http_request(const char *url, void *result,
int target, int options)
ret = HTTP_REAUTH;
}
} else {
+#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
curl_easy_strerror(results.curl_result),
sizeof(curl_errorstr));
+#endif
ret = HTTP_ERROR;
}
} else {
--
1.7.12
Better?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> -Original Message-
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Wednesday, August 22, 2012 7:30 PM
> To: Brandon Casey
> Cc: Joachim Schmitz; Shawn Pearce; git@vger.kernel.org;
> rsbec...@nexbridge.com
> Subject: Re: Porting git to HP NonStop
>
> Brandon Casey writes:
>
> > On Wed, Aug 22, 2012 at 9:30 AM, Joachim Schmitz
> > wrote:
> >
> >> OK, so how about this:
> >> /usr/local/bin/diff -EBbu ./compat/mkdir.c.orig ./compat/mkdir.c
> >> --- ./compat/mkdir.c.orig 2012-08-21 05:02:11 -0500
> >> +++ ./compat/mkdir.c2012-08-21 05:02:11 -0500
> >> @@ -0,0 +1,24 @@
> >> +#include "../git-compat-util.h"
> >> +#undef mkdir
> >> +
> >> +/* for platforms that can't deal with a trailing '/' */ int
> >> +compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode) {
> >> + int retval;
> >> + char *tmp_dir = NULL;
> >> + size_t len = strlen(dir);
> >> + ...
> > Why not rearrange this so that you assign to dir the value of tmp_dir
> > and then just pass dir to mkdir. Then you can avoid the recast of dir
> > to (char*) in the else branch. Later, just call free(tmp_dir). Also,
> > we have xstrndup. So I think the body of your function can become
> > something like:
> >
> >if (len && dir[len-1] == '/')
> >dir = tmp_dir = xstrndup(dir, len-1);
> >
> >retval = mkdir(dir, mode);
> >free(tmp_dir);
>
> Nice. And we have xmemdupz() would be even better as you followed-up.
How's that one used?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> -Original Message- > From: Johannes Sixt [mailto:j...@kdbg.org] > Sent: Wednesday, August 22, 2012 7:41 PM > To: Brandon Casey > Cc: Joachim Schmitz; Junio C Hamano; Shawn Pearce; git@vger.kernel.org; > rsbec...@nexbridge.com > Subject: Re: Porting git to HP NonStop > > Am 22.08.2012 19:00, schrieb Brandon Casey: > > So I think the body of [compat_mkdir] can become something like: > > > >if (len && dir[len-1] == '/') > >dir = tmp_dir = xstrndup(dir, len-1); > > Don't use x* wrappers in the compat layer, at least not those that allocate > memory: They behave unpredictably due to try_to_free_routine and may lead > to recursive invocations. I was just following orders ;-) What about the other proposal, xmemdupz? Same story I guess? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> Sent: Wednesday, August 22, 2012 7:53 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over
> getrlimit(RLIMIT_NOFILE,...)
>
> > From: Junio C Hamano [mailto:gits...@pobox.com]
> > Sent: Wednesday, August 22, 2012 7:23 PM
> > To: Joachim Schmitz
> > Cc: git@vger.kernel.org
> > Subject: Re: [PATCH] Prefer sysconf(_SC_OPEN_MAX) over
> > getrlimit(RLIMIT_NOFILE,...)
> >
> > "Joachim Schmitz" writes:
> >
> > > Signed-off-by: Joachim Schmitz
> > > ---
> > > sha1_file.c | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/sha1_file.c b/sha1_file.c index af5cfbd..76714ad 100644
> > > --- a/sha1_file.c
> > > +++ b/sha1_file.c
> > > @@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
> > > return error("packfile %s index unavailable",
> > > p->pack_name);
> > >
> > > if (!pack_max_fds) {
> > > +#ifdef _SC_OPEN_MAX
> > > + unsigned int max_fds = sysconf(_SC_OPEN_MAX); #else
> > > struct rlimit lim;
> > > unsigned int max_fds;
> > >
> > > @@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
> > > die_errno("cannot get RLIMIT_NOFILE");
> > >
> > > max_fds = lim.rlim_cur;
> > > +#endif
> > >
> > > /* Save 3 for stdin/stdout/stderr, 22 for work */
> > > if (25 < max_fds)
> > > --
> > > 1.7.12
> >
> > Looks sane but it would be more readable to make this a small helper
> > function, so that we do not need to have #ifdef/#endif in the primary
flow of
> the code.
>
> Hmm, in compat/? Worth the effort fort hat single occrence?
>
> > By the way, I noticed that you seem to be sending patches out of git,
> > instead of "diff -ru", which is a good sign ;-).
>
> Not quite, I'm generating them with "git format-patch origin", on the
NonStop
> machine, but can't send email from there (a) behind a firewall and b) no
email
> client available), so I copy/paste the resulting file into Outlook.
>
> >But all of your patches are whitespace
> > damaged and cannot be applied X-<.
>
> May well be Outlooks fault? How to solve?
Let's try this then:
---
Signed-off-by: Joachim Schmitz
---
sha1_file.c | 4
1 file changed, 4 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..76714ad 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -747,6 +747,9 @@ static int open_packed_git_1(struct packed_git *p)
return error("packfile %s index unavailable", p->pack_name);
if (!pack_max_fds) {
+#ifdef _SC_OPEN_MAX
+ unsigned int max_fds = sysconf(_SC_OPEN_MAX);
+#else
struct rlimit lim;
unsigned int max_fds;
@@ -754,6 +757,7 @@ static int open_packed_git_1(struct packed_git *p)
die_errno("cannot get RLIMIT_NOFILE");
max_fds = lim.rlim_cur;
+#endif
/* Save 3 for stdin/stdout/stderr, 22 for work */
if (25 < max_fds)
--
1.7.12
OK this way?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> -Original Message- > From: Johannes Sixt [mailto:j...@kdbg.org] > Sent: Wednesday, August 22, 2012 8:09 PM > To: Joachim Schmitz > Cc: 'Brandon Casey'; 'Junio C Hamano'; 'Shawn Pearce'; git@vger.kernel.org; > rsbec...@nexbridge.com > Subject: Re: Porting git to HP NonStop > > Am 22.08.2012 20:02, schrieb Joachim Schmitz: > >> From: Johannes Sixt [mailto:j...@kdbg.org] Don't use x* wrappers in > >> the compat layer, at least not those that allocate > >> memory: They behave unpredictably due to try_to_free_routine and may > >> lead to recursive invocations. > > > > I was just following orders ;-) > > What about the other proposal, xmemdupz? Same story I guess? > > xmemdupz calls xmalloc, so, yes, same story. So back to my original patch, using strdup, check the return value, etc. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> -Original Message- > From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Wednesday, August 22, 2012 8:17 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0 > > "Joachim Schmitz" writes: > > > > > This reverts be22d92 (http: avoid empty error messages for some curl > > errors, > > 2011-09-05) on platforms with older versions of libcURL. > > > > Signed-off-by: Joachim Schmitz > > --- > > http.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/http.c b/http.c > > index b61ac85..18bc6bf 100644 > > --- a/http.c > > +++ b/http.c > > @@ -806,10 +806,12 @@ static int http_request(const char *url, void > > *result, int target, int options) > > Likewrapped X-< Any suggestion? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Wednesday, August 22, 2012 8:25 PM
> To: Joachim Schmitz
> Cc: 'Brandon Casey'; 'Shawn Pearce'; git@vger.kernel.org;
> rsbec...@nexbridge.com
> Subject: Re: Porting git to HP NonStop
>
> "Joachim Schmitz" writes:
>
> >> Nice. And we have xmemdupz() would be even better as you followed-up.
> >
> > How's that one used?
>
> I forgot that we frown upon use of any x() wrapper in the
compat/
> layer as J6t mentioned.
>
> So probably something along these lines...
>
> int retval;
> char *dir_to_free = NULL;
> size_t len = strlen(dir);
>
> if (len && dir[len - 1] == '/') {
> dir_to_free = malloc(len);
> if (!dir_to_free) {
> fprintf(stderr, "malloc failed!\n");
> exit(1);
> }
> memcpy(dir_to_free, dir, len - 1);
> dir_to_free[len - 1] = '\0';
> dir = dir_to_free;
> }
> retval = mkdir(dir, mode);
> free(dir_to_free);
> return retval;
So why not just strdup? I stole the idea from gnulib...
int
rpl_mkdir (char const *dir, mode_t mode maybe_unused)
{
int ret_val;
char *tmp_dir;
size_t len = strlen (dir);
if (len && dir[len - 1] == '/')
{
tmp_dir = strdup (dir);
if (!tmp_dir)
{
/* Rather than rely on strdup-posix, we set errno ourselves. */
errno = ENOMEM;
return -1;
}
strip_trailing_slashes (tmp_dir);
}
else
{
tmp_dir = (char *) dir;
}
They strip more than one trailing slash, but for git's purpose I believed
this to be too much overhead. Also the errno stuff doesn't seem to be really
needed IMHO. Same for the following code
#if FUNC_MKDIR_DOT_BUG
/* Additionally, cygwin 1.5 mistakenly creates a directory "d/./". */
{
char *last = last_component (tmp_dir);
if (*last == '.' && (last[1] == '\0'
|| (last[1] == '.' && last[2] == '\0')))
{
struct stat st;
if (stat (tmp_dir, &st) == 0)
errno = EEXIST;
return -1;
}
}
#endif /* FUNC_MKDIR_DOT_BUG */
Then it goes on like mine:
ret_val = mkdir (tmp_dir, mode);
if (tmp_dir != dir)
free (tmp_dir);
return ret_val;
}
Compare:
$ cat compat/mkdir.c
#include "../git-compat-util.h"
#undef mkdir
/* for platforms that can't deal with a trailing '/' */
int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
{
int retval;
char *tmp_dir = NULL;
size_t len = strlen(dir);
if (len && dir[len-1] == '/') {
if ((tmp_dir = strdup(dir)) == NULL)
return -1;
tmp_dir[len-1] = '\0';
}
else
tmp_dir = (char *)dir;
retval = mkdir(tmp_dir, mode);
if (tmp_dir != dir)
free(tmp_dir);
return retval;
}
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de] > Sent: Wednesday, August 22, 2012 7:44 PM > To: 'Junio C Hamano' > Cc: 'git@vger.kernel.org'; 'Erik Faye-Lund'; 'Johannes Sixt'; 'Marius Storm- > Olsen' > Subject: RE: [PATCH] Support non-WIN32 system lacking poll() while keeping > the WIN32 part intact > > > From: Junio C Hamano [mailto:gits...@pobox.com] > > Sent: Wednesday, August 22, 2012 7:13 PM > > To: Joachim Schmitz > > Cc: git@vger.kernel.org; Erik Faye-Lund; Johannes Sixt; Marius > > Storm-Olsen > > Subject: Re: [PATCH] Support non-WIN32 system lacking poll() while > > keeping the WIN32 part intact > > > > "Joachim Schmitz" writes: > > > > > Signed-off-by: Joachim Schmitz > > > --- > > > Makefile| 18 ++ > > > compat/win32/poll.c | 8 ++-- > > > 2 files changed, 20 insertions(+), 6 deletions(-) > > > > > > diff --git a/Makefile b/Makefile ... > > > @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT ... > > > +ifdef NO_POLL > > > + NO_SYS_POLL_H = YesPlease > > > + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it find poll.h > > > + COMPAT_OBJS += compat/win32/poll.c This is of course wrong! Needs to be poll.o, not poll.c > > > endif > > > > In general, I think this is a good direction to go. If the existing > > emulation in win32/poll.c turns out to be usable across platforms and > > not windows specific, sharing it would be a good idea. > > > > But if the emulation is no longer windows specific, shouldn't you also > > move it outside compat/win32/ and somewhere more generic? > > Should be possible. Esp. as with the current setup make issues a warning: > > Makefile:2329: target `compat/win32/poll.c' doesn't match the target pattern > > Haven't yet been able to spot where that comes from. See above. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
Hi folks
There another API missing on HP NonStop and that is setitimer(), used in
progress.c and build/log.c
I do have a homebrewed implementation, on top of alarm(), it goes like this:
#include "../git-compat-util.h"
#undef getitimer
#undef setitimer
int
git_getitimer(int which, struct itimerval *value)
{
int ret = 0;
switch (which) {
case ITIMER_REAL:
value->it_value.tv_usec = 0;
value->it_value.tv_sec = alarm(0);
ret = 0; /* if alarm() fails we get a SIGLIMIT */
break;
case ITIMER_VIRTUAL: /* FALLTHRU */
case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
default: errno = EINVAL; ret = -1;
}
return ret;
}
int
git_setitimer(int which, const struct itimerval *value,
struct itimerval *ovalue)
{
int ret = 0;
if (!value
|| value->it_value.tv_usec < 0
|| value->it_value.tv_usec > 100
|| value->it_value.tv_sec < 0) {
errno = EINVAL;
return -1;
}
else if (ovalue)
if (!git_getitimer(which, ovalue))
return -1; /* errno set in git_getitimer() */
else
switch (which) {
case ITIMER_REAL:
alarm(value->it_value.tv_sec +
(value->it_value.tv_usec > 0) ? 1 : 0);
ret = 0; /* if alarm() fails we get a SIGLIMIT */
break;
case ITIMER_VIRTUAL: /* FALLTHRU */
case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
default: errno = EINVAL; ret = -1;
}
return ret;
}
Worth being added to compat/, e.g. as setitimer.c, or, as itimer.c (as a
by-product, it has getitimer() too)?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Wednesday, August 22, 2012 10:50 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; rsbec...@nexbridge.com > Subject: Re: Porting git to HP NonStop > > "Joachim Schmitz" writes: > > > Hi folks > > > > There another API missing on HP NonStop and that is setitimer(), used > > in progress.c and build/log.c I do have a homebrewed implementation, on top > of alarm(), it goes like this: > > > > #include "../git-compat-util.h" > > #undef getitimer > > #undef setitimer > > > > > > int > > git_getitimer(int which, struct itimerval *value) > > See Documentation/CodingGuidelines for style nits. Will do and adjust code accordingly. Here I was more concerned about content though ;-) ... > > Worth being added to compat/, e.g. as setitimer.c, or, as itimer.c (as > > a by-product, it has getitimer() too)? > > If it helps your port, compat/itimer.c sounds like a good place. > Doesn't it need a new header file to introduce structures and constants, too? You mean the ITIMER_* and struct itimerval, right? On NonStop these are available in , so here's no need to add them. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Wednesday, August 22, 2012 11:02 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0 > > "Joachim Schmitz" writes: > > >> > diff --git a/http.c b/http.c > >> > index b61ac85..18bc6bf 100644 > >> > --- a/http.c > >> > +++ b/http.c > >> > @@ -806,10 +806,12 @@ static int http_request(const char *url, void > >> > *result, int target, int options) > >> > >> Likewrapped X-< > > > > Any suggestion? > > Other than "don't wrap" (or "get a real MUA and MTA" X-<), unfortunately no. > > I do not know if you have checked MUA specific hints section of > Documentation/SubmittingPatches; there may be environment specific hints > described for a MUA you may have at hand (or not). I checked, but Outlook isn't mentioned :-( Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Porting git to HP NonStop
> -Original Message- > From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Wednesday, August 22, 2012 11:12 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; rsbec...@nexbridge.com > Subject: Re: Porting git to HP NonStop > > "Joachim Schmitz" writes: > > >> If it helps your port, compat/itimer.c sounds like a good place. > >> Doesn't it need a new header file to introduce structures and > >> constants, > > too? > > > > You mean the ITIMER_* and struct itimerval, right? > > On NonStop these are available in , so here's no need to > > add them. > > At least you would need a header to declare these two functions and make > them visible so that the remainder of the codebase will not have to know about > git_setitimer(), no? Or does your header files on NonStop declare setitimer() > but does not implement it? No it doesn't, at least not if a form visible to a compiler... > As your proposed name is not compat/tandem.c but more generic sounding > compat/itimer.c, we would have to plan for systems other than NonStop, so we > may later have to introduce makefile variables to ask that header file to > declare the structure and define the constants that are missing from such a > system. While you are porting to NonStop, you may not have to define/declare > them, but knowing that these files are the place to later do so is part of the > planning. I thought of having the function decclaration in git-compat-util.h, just like for eg. setenv, gitmkdtemp, etc. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> Sent: Wednesday, August 22, 2012 11:06 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
>
> > From: Junio C Hamano [mailto:gits...@pobox.com]
> > Sent: Wednesday, August 22, 2012 11:02 PM
> > To: Joachim Schmitz
> > Cc: git@vger.kernel.org
> > Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> >
> > "Joachim Schmitz" writes:
> >
> > >> > diff --git a/http.c b/http.c
> > >> > index b61ac85..18bc6bf 100644
> > >> > --- a/http.c
> > >> > +++ b/http.c
> > >> > @@ -806,10 +806,12 @@ static int http_request(const char *url, void
> > >> > *result, int target, int options)
> > >>
> > >> Likewrapped X-<
> > >
> > > Any suggestion?
> >
> > Other than "don't wrap" (or "get a real MUA and MTA" X-<), unfortunately no.
> >
> > I do not know if you have checked MUA specific hints section of
> > Documentation/SubmittingPatches; there may be environment specific hints
> > described for a MUA you may have at hand (or not).
>
> I checked, but Outlook isn't mentioned :-(
OK, Outlook inserts line breaks at position 76. I can't switch it off
completely, but can set it to anything between 30 and 132. I
set it to 132 now, does that look OK now?
---
This reverts be22d92 (http: avoid empty error messages for some curl errors,
2011-09-05) on platforms with older versions of
libcURL.
Signed-off-by: Joachim Schmitz
---
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index b61ac85..18bc6bf 100644
--- a/http.c
+++ b/http.c
@@ -806,10 +806,12 @@ static int http_request(const char *url, void *result,
int target, int options)
ret = HTTP_REAUTH;
}
} else {
+#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
curl_easy_strerror(results.curl_result),
sizeof(curl_errorstr));
+#endif
ret = HTTP_ERROR;
}
} else {
--
1.7.12
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: git on HP NonStop
> From: Andreas Ericsson [mailto:a...@op5.se] > Sent: Thursday, August 23, 2012 10:24 AM > To: Joachim Schmitz > Cc: 'Junio C Hamano'; 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org > Subject: Re: git on HP NonStop > > On 08/22/2012 06:38 PM, Joachim Schmitz wrote: > > > > > >> -Original Message- > >> From: Junio C Hamano [mailto:gits...@pobox.com] > >> Sent: Tuesday, August 21, 2012 4:06 AM > >> To: Joachim Schmitz > >> Cc: 'Johannes Sixt'; 'Jan Engelhardt'; git@vger.kernel.org > >> Subject: Re: git on HP NonStop > >> > >> "Joachim Schmitz" writes: > >> > >>> OK, so let's have a look at code, current git, builtin/cat-file.c, > >>> line 196: > >>> void *contents = contents; > >>> > >>> This variable is set later in an if branch (if (print_contents == > >>> BATCH), but not in the else branch. It is later used always under the > >>> same condition as the one under which it is set. > >>> Apparently is is malloc_d storage (there a "free(content);"), so > >>> there's no harm al all in initializing it with NULL, even if it only > >>> appeases a stupid compiler. > >> > >> It actually is harmful. See below. > > > > Harmful to initialize with NULL or to use that undefined behavoir? > > > > I checked what our compiler does here: after having warned about "vlues us > > used before it is set: it actually dies seem to have initializes the value > > to 0 resp. NULL. > > So here there's no harm done in avoiding undefined behavior and set it to 0 > > resp NULL in the first place. > > > > There is harm in tricking future programmers into thinking that the > initialization actually means something, which some of them do. Hmm, OK, I can agree to that. > It's unlikely that you're the one to maintain that code forever, It is unlike for me to ever have to maintain this code. Currently that's Junio's job and I won't apply for in ;-) > and the "var = var" idiom is used widely within git This is overstating it a bit. I went thru the entire code and reported all places I could find in an earlier email. I went back and counted: It is used in 11 files, at 15 places, for 21 variables. OK, I may have missed a few more that were in code paths my compiler didn't see, but still some 21+ isn't really much. >with a clear meaning Only if you call undefined behavior a 'clear meaning"! > as a hint to programmers who read a bit of git code. If they aren't > used to that idiom, they usually investigate it in the code and > pretty quickly realize that what it means. Whether I realize what it means, is irrelevant, my compiler does not and warns about it, and as per the ANSI/ICO C standard it invokes undefined behavior. If a proper initialization is meaningless for these cases, don't do them at all, let the stupid compiler complain about it and the clever programmer check whether the warning is useful, but don't avoid a compiler warning on one compiler by introducing undefined behavior and provoke a compiler warning on another. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Thursday, August 23, 2012 11:27 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0 > > "Joachim Schmitz" writes: > > >> From: Joachim Schmitz [mailto:j...@schmitz-digital.de] > >> Sent: Wednesday, August 22, 2012 11:06 PM > >> To: 'Junio C Hamano' > >> Cc: 'git@vger.kernel.org' > >> Subject: RE: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0 > >> > >> > From: Junio C Hamano [mailto:gits...@pobox.com] > >> > Sent: Wednesday, August 22, 2012 11:02 PM > >> > To: Joachim Schmitz > >> > Cc: git@vger.kernel.org > >> > Subject: Re: [PATCH] Don't use curl_easy_strerror prior to curl-7.12.0 > >> > > >> > "Joachim Schmitz" writes: > >> > > >> > >> > diff --git a/http.c b/http.c > >> > >> > index b61ac85..18bc6bf 100644 > >> > >> > --- a/http.c > >> > >> > +++ b/http.c > >> > >> > @@ -806,10 +806,12 @@ static int http_request(const char *url, void > >> > >> > *result, int target, int options) > >> > >> > >> > >> Likewrapped X-< > >> > > > >> > > Any suggestion? > >> > > >> > Other than "don't wrap" (or "get a real MUA and MTA" X-<), unfortunately > >> > no. > >> > > >> > I do not know if you have checked MUA specific hints section of > >> > Documentation/SubmittingPatches; there may be environment specific hints > >> > described for a MUA you may have at hand (or not). > >> > >> I checked, but Outlook isn't mentioned :-( > > > > OK, Outlook inserts line breaks at position 76. I can't switch it off > > completely, but can set it to anything between 30 and 132. I > > set it to 132 now, does that look OK now? > > Yeah, modulo that with all the above noise and with "---" before the > log message you inserted by hand before the real commit log message, OK, those are not allowed then? Or would they need to look different? > and also the log message is not properly line-wrapped, it is getting > closer to what is expected of a patch to be applied. > > I've applied it by hand and fixed the log message up, so no need to > resend this particular one. Thanks. Thanks > > This reverts be22d92 (http: avoid empty error messages for some curl > > errors, 2011-09-05) on platforms with older versions of > > libcURL. You meant that linewrap between "of" and libcURL?, Or well, that was the 132 position... Or do you want these messages being wrapped at <=80? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
Signed-off-by: Joachim Schmitz --- This time I hopefully didn't screw up whitespace and line breaks. Makefile| 18 ++ compat/win32/poll.c | 8 ++-- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/Makefile b/Makefile index 66e8216..e150816 100644 --- a/Makefile +++ b/Makefile @@ -152,6 +152,11 @@ all:: # # Define NO_MMAP if you want to avoid mmap. # +# Define NO_SYS_POLL_H if you don't have sys/poll.h. +# +# Define NO_POLL if you do not have or do not want to use poll. +# This also implies NO_SYS_POLL_H. +# # Define NO_PTHREADS if you do not have or do not want to use Pthreads. # # Define NO_PREAD if you have a problem with pread() system call (e.g. @@ -1216,7 +1221,7 @@ ifeq ($(uname_S),Windows) NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease - NO_SYS_POLL_H = YesPlease + NO_POLL = YesPlease NO_SYMLINK_HEAD = YesPlease NO_IPV6 = YesPlease NO_UNIX_SOCKETS = YesPlease @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows) BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE COMPAT_OBJS = compat/msvc.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ - compat/win32/poll.o compat/win32/dirent.o + compat/win32/dirent.o COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H -DHAVE_ALLOCA_H -Icompat -Icompat/regex -Icompat/win32 -DSTRIP_EXTENSION=\".exe\" BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE -NODEFAULTLIB:MSVCRT.lib EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib @@ -1312,7 +1317,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_PREAD = YesPlease NEEDS_CRYPTO_WITH_SSL = YesPlease NO_LIBGEN_H = YesPlease - NO_SYS_POLL_H = YesPlease + NO_POLL = YesPlease NO_SYMLINK_HEAD = YesPlease NO_UNIX_SOCKETS = YesPlease NO_SETENV = YesPlease @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/winansi.o \ compat/win32/pthread.o compat/win32/syslog.o \ - compat/win32/poll.o compat/win32/dirent.o + compat/win32/dirent.o EXTLIBS += -lws2_32 PTHREAD_LIBS = X = .exe @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT BASIC_CFLAGS += -DNO_GETTEXT USE_GETTEXT_SCHEME ?= fallthrough endif +ifdef NO_POLL + NO_SYS_POLL_H = YesPlease + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it finds poll.h + COMPAT_OBJS += compat/win32/poll.o +endif ifdef NO_STRCASESTR COMPAT_CFLAGS += -DNO_STRCASESTR COMPAT_OBJS += compat/strcasestr.o diff --git a/compat/win32/poll.c b/compat/win32/poll.c index 403eaa7..49541f1 100644 --- a/compat/win32/poll.c +++ b/compat/win32/poll.c @@ -24,7 +24,9 @@ # pragma GCC diagnostic ignored "-Wtype-limits" #endif -#include +#if defined(WIN32) +# include +#endif #include @@ -48,7 +50,9 @@ #else # include # include -# include +# ifndef NO_SYS_SELECT_H +# include +# endif # include #endif -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
Signed-off-by: Joachim Schmitz
---
As discussed now as a small helper function rather than #ifdef/#endif in the
primary flow of the code.
And hopefully without having screwed up whitespace and line breaks
sha1_file.c | 22 +++---
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/sha1_file.c b/sha1_file.c
index af5cfbd..427f9e6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -731,6 +731,20 @@ void free_pack_by_name(const char *pack_name)
}
}
+static unsigned int get_max_fd_limit(void)
+{
+#ifdef _SC_OPEN_MAX
+ return sysconf(_SC_OPEN_MAX);
+#else
+ struct rlimit lim;
+
+ if (getrlimit(RLIMIT_NOFILE, &lim))
+ die_errno("cannot get RLIMIT_NOFILE");
+
+ return lim.rlim_cur;
+#endif
+}
+
/*
* Do not call this directly as this leaks p->pack_fd on error return;
* call open_packed_git() instead.
@@ -747,13 +761,7 @@ static int open_packed_git_1(struct packed_git *p)
return error("packfile %s index unavailable", p->pack_name);
if (!pack_max_fds) {
- struct rlimit lim;
- unsigned int max_fds;
-
- if (getrlimit(RLIMIT_NOFILE, &lim))
- die_errno("cannot get RLIMIT_NOFILE");
-
- max_fds = lim.rlim_cur;
+ unsigned int max_fds = get_max_fd_limit();
/* Save 3 for stdin/stdout/stderr, 22 for work */
if (25 < max_fds)
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] Don't use curl_easy_strerror prior to curl-7.12.0
This reverts be22d92 (http: avoid empty error messages for some curl errors,
2011-09-05) on platforms with older versions of libcURL.
Signed-off-by: Joachim Schmitz
---
Resend, regardless that Junio said this not to be needed, as I don't see it
applied yet.
Also tried to fix the formatting issues too, to practice submissions ;-)
http.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/http.c b/http.c
index b61ac85..18bc6bf 100644
--- a/http.c
+++ b/http.c
@@ -806,10 +806,12 @@ static int http_request(const char *url, void *result,
int target, int options)
ret = HTTP_REAUTH;
}
} else {
+#if LIBCURL_VERSION_NUM >= 0x070c00
if (!curl_errorstr[0])
strlcpy(curl_errorstr,
curl_easy_strerror(results.curl_result),
sizeof(curl_errorstr));
+#endif
ret = HTTP_ERROR;
}
} else {
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
Signed-off-by: Joachim Schmitz
---
compat/mkdir.c | 24
1 file changed, 24 insertions(+)
create mode 100644 compat/mkdir.c
diff --git a/compat/mkdir.c b/compat/mkdir.c
new file mode 100644
index 000..9e253fb
--- /dev/null
+++ b/compat/mkdir.c
@@ -0,0 +1,24 @@
+#include "../git-compat-util.h"
+#undef mkdir
+
+/* for platforms that can't deal with a trailing '/' */
+int compat_mkdir_wo_trailing_slash(const char *dir, mode_t mode)
+{
+ int retval;
+ char *tmp_dir = NULL;
+ size_t len = strlen(dir);
+
+ if (len && dir[len-1] == '/') {
+ if ((tmp_dir = strdup(dir)) == NULL)
+ return -1;
+ tmp_dir[len-1] = '\0';
+ }
+ else
+ tmp_dir = (char *)dir;
+
+ retval = mkdir(tmp_dir, mode);
+ if (tmp_dir != dir)
+ free(tmp_dir);
+
+ return retval;
+}
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
Signed-off-by: Joachim Schmitz --- git-compat-util.h | 5 + 1 file changed, 5 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 35b095e..34f040f 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -162,6 +162,11 @@ #define probe_utf8_pathname_composition(a,b) #endif +#ifdef MKDIR_WO_TRAILING_SLASH +#define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b)) +extern int compat_mkdir_wo_trailing_slash(const char*, mode_t); +#endif + #ifndef NO_LIBGEN_H #include #else -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] Support for setitimer() on platforms lacking it
Implementation includes getitimer(), but for now it is static.
Supports ITIMER_REAL only.
Signed-off-by: Joachim Schmitz
---
May need a header file for ITIMER_*, struct itimerval and the prototypes,
But for now, and the HP NonStop platform this isn't needed, here
has ITIMER_* and struct timeval, and the prototypes can
vo into git-compat-util.h for now (Patch 2/2)
compat/itimer.c | 50 ++
1 file changed, 50 insertions(+)
create mode 100644 compat/itimer.c
diff --git a/compat/itimer.c b/compat/itimer.c
new file mode 100644
index 000..713f1ff
--- /dev/null
+++ b/compat/itimer.c
@@ -0,0 +1,50 @@
+#include "../git-compat-util.h"
+
+static int git_getitimer(int which, struct itimerval *value)
+{
+ int ret = 0;
+
+ switch (which) {
+ case ITIMER_REAL:
+ value->it_value.tv_usec = 0;
+ value->it_value.tv_sec = alarm(0);
+ ret = 0; /* if alarm() fails, we get a SIGLIMIT */
+ break;
+ case ITIMER_VIRTUAL: /* FALLTHRU */
+ case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
+ default: errno = EINVAL; ret = -1;
+ }
+ return ret;
+}
+
+int git_setitimer(int which, const struct itimerval *value,
+ struct itimerval *ovalue)
+{
+ int ret = 0;
+
+ if (!value
+ || value->it_value.tv_usec < 0
+ || value->it_value.tv_usec > 100
+ || value->it_value.tv_sec < 0) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ else if (ovalue)
+ if (!git_getitimer(which, ovalue))
+ return -1; /* errno set in git_getitimer() */
+
+ else
+ switch (which) {
+ case ITIMER_REAL:
+ alarm(value->it_value.tv_sec +
+ (value->it_value.tv_usec > 0) ? 1 : 0);
+ ret = 0; /* if alarm() fails, we get a SIGLIMIT */
+ break;
+ case ITIMER_VIRTUAL: /* FALLTHRU */
+ case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
+ default: errno = EINVAL; ret = -1;
+ }
+
+ return ret;
+}
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/2] Support for setitimer() on platforms lacking it
Signed-off-by: Joachim Schmitz --- Seems it needs my mkdir() "ignoretraile slash" patch first to be applied cleanly... It is independent of it otherwise. git-compat-util.h | 5 + 1 file changed, 5 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index 34f040f..a047221 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -162,6 +162,11 @@ #define probe_utf8_pathname_composition(a,b) #endif +#ifdef NO_SETITIMER +#define setitimer(a,b,c) git_setitimer((a),(b),(c)) +extern int git_setitimer(int, const struct itimerval *, struct itimerval *); +#endif + #ifdef MKDIR_WO_TRAILING_SLASH #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b)) extern int compat_mkdir_wo_trailing_slash(const char*, mode_t); -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de] > Sent: Friday, August 24, 2012 11:45 AM > To: Junio C Hamano (gits...@pobox.com) > Cc: git@vger.kernel.org; Erik Faye-Lund (kusmab...@gmail.com) > Subject: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the > WIN32 part intact > > > Signed-off-by: Joachim Schmitz > --- > This time I hopefully didn't screw up whitespace and line breaks. > > Makefile| 18 ++ > compat/win32/poll.c | 8 ++-- > 2 files changed, 20 insertions(+), 6 deletions(-) > > diff --git a/Makefile b/Makefile > index 66e8216..e150816 100644 > --- a/Makefile > +++ b/Makefile > @@ -152,6 +152,11 @@ all:: > # > # Define NO_MMAP if you want to avoid mmap. > # > +# Define NO_SYS_POLL_H if you don't have sys/poll.h. > +# > +# Define NO_POLL if you do not have or do not want to use poll. > +# This also implies NO_SYS_POLL_H. > +# > # Define NO_PTHREADS if you do not have or do not want to use Pthreads. > # > # Define NO_PREAD if you have a problem with pread() system call (e.g. > @@ -1216,7 +1221,7 @@ ifeq ($(uname_S),Windows) > NO_PREAD = YesPlease > NEEDS_CRYPTO_WITH_SSL = YesPlease > NO_LIBGEN_H = YesPlease > - NO_SYS_POLL_H = YesPlease > + NO_POLL = YesPlease > NO_SYMLINK_HEAD = YesPlease > NO_IPV6 = YesPlease > NO_UNIX_SOCKETS = YesPlease > @@ -1257,7 +1262,7 @@ ifeq ($(uname_S),Windows) > BASIC_CFLAGS = -nologo -I. -I../zlib -Icompat/vcbuild > -Icompat/vcbuild/include -DWIN32 -D_CONSOLE -DHAVE_STRING_H - > D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_DEPRECATE > COMPAT_OBJS = compat/msvc.o compat/winansi.o \ > compat/win32/pthread.o compat/win32/syslog.o \ > - compat/win32/poll.o compat/win32/dirent.o > + compat/win32/dirent.o > COMPAT_CFLAGS = -D__USE_MINGW_ACCESS -DNOGDI -DHAVE_STRING_H > -DHAVE_ALLOCA_H -Icompat -Icompat/regex - > Icompat/win32 -DSTRIP_EXTENSION=\".exe\" > BASIC_LDFLAGS = -IGNORE:4217 -IGNORE:4049 -NOLOGO -SUBSYSTEM:CONSOLE > -NODEFAULTLIB:MSVCRT.lib > EXTLIBS = user32.lib advapi32.lib shell32.lib wininet.lib ws2_32.lib > @@ -1312,7 +1317,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) > NO_PREAD = YesPlease > NEEDS_CRYPTO_WITH_SSL = YesPlease > NO_LIBGEN_H = YesPlease > - NO_SYS_POLL_H = YesPlease > + NO_POLL = YesPlease > NO_SYMLINK_HEAD = YesPlease > NO_UNIX_SOCKETS = YesPlease > NO_SETENV = YesPlease > @@ -1347,7 +1352,7 @@ ifneq (,$(findstring MINGW,$(uname_S))) > COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" > COMPAT_OBJS += compat/mingw.o compat/winansi.o \ > compat/win32/pthread.o compat/win32/syslog.o \ > - compat/win32/poll.o compat/win32/dirent.o > + compat/win32/dirent.o > EXTLIBS += -lws2_32 > PTHREAD_LIBS = > X = .exe > @@ -1601,6 +1606,11 @@ ifdef NO_GETTEXT > BASIC_CFLAGS += -DNO_GETTEXT > USE_GETTEXT_SCHEME ?= fallthrough > endif > +ifdef NO_POLL > + NO_SYS_POLL_H = YesPlease > + COMPAT_CFLAGS += -DNO_POLL -Icompat/win32 # so it finds poll.h > + COMPAT_OBJS += compat/win32/poll.o > +endif > ifdef NO_STRCASESTR > COMPAT_CFLAGS += -DNO_STRCASESTR > COMPAT_OBJS += compat/strcasestr.o > diff --git a/compat/win32/poll.c b/compat/win32/poll.c > index 403eaa7..49541f1 100644 > --- a/compat/win32/poll.c > +++ b/compat/win32/poll.c > @@ -24,7 +24,9 @@ > # pragma GCC diagnostic ignored "-Wtype-limits" > #endif > > -#include > +#if defined(WIN32) > +# include > +#endif > > #include > > @@ -48,7 +50,9 @@ > #else > # include > # include > -# include > +# ifndef NO_SYS_SELECT_H > +# include > +# endif > # include > #endif > > -- > 1.7.12 There is a downside with this: In order to make use of it, in Makefile it adds "-Icompat/win32" to COMPAR_CFLAGS. This results in compat/win32/dirent.h to be found, rather than /usr/include/dirent.h. This should be fine for WIN32, but for everybody else may not. For HP NonStop in particular it results in a warning: }; ^ "... /compat/win32/dirent.h", line 17: warning(133): expected an identifier And this is because there it uses an unnamed union, which is a GCC extension (just like unnamed struct), but not part of C89/C99/POSIX. One possible solution might be to move compat/win32/poll.[ch] to compat/. Another is to just ignore the warning, at least here it seems to work just fine? Or to avoid using an unnamed union. But the later 2 cases would still mean that we include the wrong dirent.h, so the 1st solution seems the cleanest. Any other idea? Let me know your thoughts... Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Ignore trailing slash in mkdir() on platforms that can't deal with this
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 7:44 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [PATCH 1/2] Ignore trailing slash in mkdir() on platforms that > can't deal with this > > As the compat/mkdir.c file includes git-compat-util.h and expects > the declaration of the new function to be found in it, it does not > make any sense to have this as two patches. I'll squash them into > one for now, but it would have been even more complete to have an > update to the Makefile to actually compile these files in the same > change. These things go together. A changed Makefile is in the pipeline, but right now it is pretty NonStop specific, while I tried to keep compat/itimer.cm compat/mkdir.c and git-compat-util.h as NonStop independent as I possible could. That Makefile also depends on the outcome of the discussion about my NonStop specific changed in git-compat.util.h > The other itimer set shares the same issue. I've queued mkdir and > itimer series as one patch each; please check the result in 'pu' > after I push it out. Thanks. What does 'pu' stand for? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over getrlimit(RLIMIT_NOFILE,...)
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Friday, August 24, 2012 6:44 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH v2] Prefer sysconf(_SC_OPEN_MAX) over
> getrlimit(RLIMIT_NOFILE,...)
>
> "Joachim Schmitz" writes:
>
> > Signed-off-by: Joachim Schmitz
> > ---
> > As discussed now as a small helper function rather than #ifdef/#endif in
> > the primary flow of the code.
> > And hopefully without having screwed up whitespace and line breaks
>
> The formatting looks fine.
>
> Perhaps I am being overly paranoid, but I would prefer not to change
> things for people who have been using getrlimit(). For them, if
> they also have sysconf(_SC_OPEN_MAX), your code _ought to_ work, but
> if it does not work for whatever reason (perhaps some platforms
> claim to have both, but getrlimit() works and sysconf(_SC_OPEN_MAX)
> is broken), it will given them an unnecessary regression.
Sounds reasonable, so reasonable that I wonder why I didn't have that idea ;-)
> So how about doing it this way instead?
>
> -- >8 --
> Subject: sha1_file.c: introduce get_max_fd_limit() helper
>
> Not all platforms have getrlimit(), but there are other ways to see
> the maximum number of files that a process can have open. If
> getrlimit() is unavailable, fall back to sysconf(_SC_OPEN_MAX) if
> available, and use OPEN_MAX from .
>
> Signed-off-by: Joachim Schmitz
> Signed-off-by: Junio C Hamano
> ---
> sha1_file.c | 26 +++---
> 1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git c/sha1_file.c w/sha1_file.c
> index af5cfbd..9152974 100644
> --- c/sha1_file.c
> +++ w/sha1_file.c
> @@ -731,6 +731,24 @@ void free_pack_by_name(const char *pack_name)
> }
> }
>
> +static unsigned int get_max_fd_limit(void)
> +{
> +#ifdef RLIMIT_NOFILE
> + struct rlimit lim;
> +
> + if (getrlimit(RLIMIT_NOFILE, &lim))
> + die_errno("cannot get RLIMIT_NOFILE");
> +
> + return lim.rlim_cur;
> +#elif defined(_SC_OPEN_MAX)
> + return sysconf(_SC_OPEN_MAX);
> +#elif defined(OPEN_MAX)
> + return OPEN_MAX;
> +#else
> + return 1; /* see the caller ;-) */
> +#endif
> +}
> +
> /*
> * Do not call this directly as this leaks p->pack_fd on error return;
> * call open_packed_git() instead.
> @@ -747,13 +765,7 @@ static int open_packed_git_1(struct packed_git *p)
> return error("packfile %s index unavailable", p->pack_name);
>
> if (!pack_max_fds) {
> - struct rlimit lim;
> - unsigned int max_fds;
> -
> - if (getrlimit(RLIMIT_NOFILE, &lim))
> - die_errno("cannot get RLIMIT_NOFILE");
> -
> - max_fds = lim.rlim_cur;
> + unsigned int max_fds = get_max_fd_limit();
>
> /* Save 3 for stdin/stdout/stderr, 22 for work */
> if (25 < max_fds)
Looks good to me.
Stupid newbie question: how would I revert my commit to my clone, to then add
(and test) this one?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 6:07 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; Erik Faye-Lund > Subject: Re: [PATCH v2] Support non-WIN32 system lacking poll() while keeping > the WIN32 part intact > > "Joachim Schmitz" writes: > > > There is a downside with this: In order to make use of it, in Makefile it > > adds "-Icompat/win32" to COMPAR_CFLAGS. This results in > > compat/win32/dirent.h to be found, rather than /usr/include/dirent.h. > > This should be fine for WIN32, but for everybody else may not. > > For HP NonStop in particular it results in a warning: > > > > }; > > ^ > > "... /compat/win32/dirent.h", line 17: warning(133): expected an identifier > > > > And this is because there it uses an unnamed union, which is a GCC extension > > (just like unnamed struct), but not part of C89/C99/POSIX. > > > > One possible solution might be to move compat/win32/poll.[ch] to compat/. > > I think that is the most sensible way to go, because poll.[ch] > > (1) has proven itself to be useful outside the context of win32, and > (2) the code is coming from gnulib anyway. > > I thought I already suggested going that route in my previous > review, but perhaps I forgot to do so. No, I believe you did, but I had forgotten about it. Could/should that be a 2nd patch? Or a v3 of this one? Different, but related question: would poll.[ch] be allowed to #include "git-compat-util.h"? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC] Support for HP NonStop
Hi folks On top of the patches Ive submitted so far, which were needed for HP NonStop, but possibly useful for other platforms too, here is one that is at least in parts NonStop specific diff --git a/git-compat-util.h b/git-compat-util.h index a047221..d6a142a 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -74,7 +74,8 @@ # define _XOPEN_SOURCE 500 # endif #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && \ - !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) + !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \ + !defined(__TANDEM) #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs 600 fo #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ #endif +#ifdef __TANDEM /* or HAVE_STRINGS_H ? */ +#include /* for strcasecmp() */ +#endif #include #include #include @@ -141,6 +145,10 @@ #else #include #endif +#ifdef __TANDEM /* or NO_INTPTR_T resp. NO_UINTPTR_T? */ +typedef int intptr_t; +typedef unsigned int uintptr_t; +#endif #if defined(__CYGWIN__) #undef _XOPEN_SOURCE #include The 1st hunk avoids a is already defined with a different value warning, and I believe this is the only and right way to fix this, but on the 2nd and 3rd hunk Id need advice on how to properly add those. The #ifdef __TANDEM #endif works fine for me, but doesnt seem 100% clean to me. In the comment I mention alternatives. strcasecamp() is declared in as per C99/POSIX, and in C99 mode a prototype has to be seen by the compiler. intptr_t and uintprt_t seem to be optional in C99 and are not provided for NonStop What do you think? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] Support for HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 10:13 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [RFC] Support for HP NonStop > > "Joachim Schmitz" writes: > > > Hi folks > > > > On top of the patches I’ve submitted so far, which were needed for HP > > NonStop, > > but possibly useful for other platforms too, here is one that is at least > > in parts NonStop specific > > > > diff --git a/git-compat-util.h b/git-compat-util.h > > index a047221..d6a142a 100644 > > --- a/git-compat-util.h > > +++ b/git-compat-util.h > > @@ -74,7 +74,8 @@ > > # define _XOPEN_SOURCE 500 > > # endif > > #elif !defined(__APPLE__) && !defined(__FreeBSD__) && !defined(__USLC__) && > > \ > > - !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) > > + !defined(_M_UNIX) && !defined(__sgi) && !defined(__DragonFly__) && \ > > + !defined(__TANDEM) > > #define _XOPEN_SOURCE 600 /* glibc2 and AIX 5.3L need 500, OpenBSD needs > > 600 fo > > #define _XOPEN_SOURCE_EXTENDED 1 /* AIX 5.3L needs this */ > > #endif > > +#ifdef __TANDEM /* or HAVE_STRINGS_H ? */ > > +#include /* for strcasecmp() */ > > +#endif > > #include > > #include > > #include > > Yeah, it appears that glibc headers have strcasecmp() and friends in > the and that was why majority of us were fine without > including . A cursory look of /usr/include/strings.h on > a GNU system suggests that it is safe to include after > we incude on that platform. > > I think it is OK to leave it "__TANDEM /* or HAVE_STRINGS_H? */" for > now and let the next person who wants to port us to a platform that > needs this inclusion turn it to HAVE_STRINGS_H. Alternatively, we > bite the bullet now and include on any platform that has > the header file and see if anybody complains That's exaclty why I'm asking here ;-), seems a decision needs to be made. How would one differentiate platrots that have strings.h from those that don't? Guess it wont'f work without some ifdef. But it could be NO_STRINGS_H and force the platforms that don't have to update this in Makefile? Reminds me of a related issue: in compat/fnmatch/fnmatch.c there is this: #if HAVE_STRING_H || defined _LIBC # include #else # include #endif There's no place where HAVE_STRING_H get set This looks wrong to me, as here, at least for NonStop, I have to takes measure in Makefile, because there's no other place where HAVE_STRING_H ever gets set: COMPAT_CFLAGS += -DHAVE_STRING_H=1 # needed in compat/fnmatch/fnmatch.c Do platforms exist without string.h? Maybe fnmatch.c should look like this instead? #if HAVE_STRING_H || defined _LIBC # include #endif # ifndef NO_STRINGS_H # include #endif > (that reminds me; I at > least should get one flavor of BSD build environment for this kind > of thing myself). > > > @@ -141,6 +145,10 @@ > > #else > > #include > > #endif > > +#ifdef __TANDEM /* or NO_INTPTR_T resp. NO_UINTPTR_T? */ > > +typedef int intptr_t; > > +typedef unsigned int uintptr_t; > > +#endif > > A bit wider context for this hunk is > > #ifndef NO_INTTYPES_H > #include > #else > #include > #endif > > So we have been assuming that has intptr_t but __TANDEM > apparently doesn't. Exactly. Our stdint.h says: /* * Special integer types (optional types intptr_t/uintptr_t not defined) */ This may change in the future though. One reason why __TANDEM might not be the best check :-) > POSIX requires intptr_t and uintptr_t to be > declared for systems conforming to XSI, but otherwise these are > optional (in other words, some XSI non-conforming platforms may have > them in ), so it would not help to check _XOPEN_UNIX to > see if the system is XSI X-<. We would need NO_INTPTR_T as you > hinted above, perhaps like this. > > #ifndef NO_INTTYPES_H > #include > #else > #include > #endif > #ifdef NO_INTPTR_T > typedef int intptr_t; > typedef unsigned int uintptr_t; > #endif NO_INTPTR_T for both types? OK by me. If need be an NOUINTPTR could get added later, I guess > By the way, is "int" wide enough, or should they be "long"? int and long have the same size, 32-bit, here on NonStop. But we do have 64-bit types too. Not sure which to take though. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC] Support for HP NonStop
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 11:51 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org > Subject: Re: [RFC] Support for HP NonStop > > "Joachim Schmitz" writes: > > > Reminds me of a related issue: in compat/fnmatch/fnmatch.c there is this: > > #if HAVE_STRING_H || defined _LIBC > > # include > > #else > > # include > > #endif > > > > There's no place where HAVE_STRING_H get set > > This looks wrong to me,... > > This is because it is a borrowed file from glibc, and we try to > minimize changes to such a file. > > If you need HAVE_STRING_H to force inclusion of on your > platform, doing this: > > >COMPAT_CFLAGS += -DHAVE_STRING_H=1 # needed in > > compat/fnmatch/fnmatch.c > > is perfectly the right thing to do. It should be set by default and those that don't have it, should (have to) disable it. In Makefile: ifndef NO_STRING_H COMPAT_CFLAGS += -DHAVE_STRING_H=1 # needed in compat/fnmatch/fnmatch.c endif > > Do platforms exist without string.h? > > Maybe fnmatch.c should look like this instead? > > We try to minimize changes to such a file we borrow from upstream; Valid point, but... > especially we do not do so lightly when we have to ask "do platforms > exist?" Of course, they do---otherwise glibc folks wouldn't have > written such a conditional. Hmm, well, why then doesn't git-compat.util.h use the same mechanism? (and no ,it should not!) I guess because as of now nobody ever tried to port git to a platform that doesn't have string.h. string.h is C89 ANSI/ISO standard (and was part of even the earliest K&C C), in place since more than 22 years now! It may not be available on embedded platforms, but those won't be able to run git anyway I'd guess. Newer version of gnulib's fnmatch.c don't use this anymore, git's is from 99, according to the Copyright: Copyright (C) 1991, 92, 93, 96, 97, 98, 99 The current gnulib one shows: Copyright (C) 1991-1993, 1996-2007, 2009-2012 Time to upgrade, if you'd ask me... Same may go for poll.c BTW Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 15/16] remote-svn: add marks-file regeneration
"Florian Achleitner" schrieb im
Newsbeitrag
news:<1345662961-9587-16-git-send-email-florian.achleitner.2.6...@gmail.com>...
> fast-import mark files are stored outside the object database and are
> therefore not fetched and can be lost somehow else. marks provide a
> svn revision --> git sha1 mapping, while the notes that are attached
> to each commit when it is imported provide a git sha1 --> svn revision
> mapping.
>
> If the marks file is not available or not plausible, regenerate it by
> walking through the notes tree. , i.e. The plausibility check tests
> if the highest revision in the marks file matches the revision of the
> top ref. It doesn't ensure that the mark file is completely correct.
> This could only be done with an effort equal to unconditional
> regeneration.
>
> Signed-off-by: Florian Achleitner
> Signed-off-by: Junio C Hamano
> ---
> remote-testsvn.c | 68
> ++
> 1 file changed, 68 insertions(+)
>
> diff --git a/remote-testsvn.c b/remote-testsvn.c
> index e90d221..09dc304 100644
> --- a/remote-testsvn.c
> +++ b/remote-testsvn.c
...
> +static void check_or_regenerate_marks(int latestrev) {
> + FILE *marksfile;
> + char *line = NULL;
> + size_t linelen = 0;
> + struct strbuf sb = STRBUF_INIT;
> + int found = 0;
> +
> + if (latestrev < 1)
> + return;
> +
> + init_notes(NULL, notes_ref, NULL, 0);
> + marksfile = fopen(marksfilename, "r");
> + if (!marksfile) {
> + regenerate_marks();
> + marksfile = fopen(marksfilename, "r");
> + if (!marksfile)
> + die_errno("cannot read marks file %s!", marksfilename);
> + fclose(marksfile);
> + } else {
> + strbuf_addf(&sb, ":%d ", latestrev);
> + while (getline(&line, &linelen, marksfile) != -1) {
getline() is not available to anybody, e.g. it is not in HP NonStop.
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/RFC v4 02/13] read-cache.c: Re-read index if index file changed
"Thomas Gummerer" schrieb im Newsbeitrag
news:<134529-6925-3-git-send-email-t.gumme...@gmail.com>...
> Add the possibility of re-reading the index file, if it changed
> while reading.
>
> The index file might change during the read, causing outdated
> information to be displayed. We check if the index file changed
> by using its stat data as heuristic.
>
> Signed-off-by: Thomas Gummerer
> ---
> read-cache.c | 87
> +---
> 1 file changed, 60 insertions(+), 27 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 6a8b4b1..cdd8480 100644
> --- a/read-cache.c
> +++ b/read-cache.c
...
> @@ -1186,38 +1209,48 @@ int read_index_from(struct index_state *istate, const
> char *path)
> errno = ENOENT;
> istate->timestamp.sec = 0;
> istate->timestamp.nsec = 0;
> - fd = open(path, O_RDONLY);
> - if (fd < 0) {
> - if (errno == ENOENT)
> - return 0;
> - die_errno("index file open failed");
> - }
> + do {
> + err = 0;
> + fd = open(path, O_RDONLY);
> + if (fd < 0) {
> + if (errno == ENOENT)
> + return 0;
> + die_errno("index file open failed");
> + }
>
> - if (fstat(fd, &st))
> - die_errno("cannot stat the open index");
> + if (fstat(fd, &st_old))
> + die_errno("cannot stat the open index");
>
> - errno = EINVAL;
> - mmap_size = xsize_t(st.st_size);
> - mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd,
> 0);
> - close(fd);
> - if (mmap == MAP_FAILED)
> - die_errno("unable to map index file");
> + errno = EINVAL;
> + mmap_size = xsize_t(st_old.st_size);
> + mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
> MAP_PRIVATE, fd, 0);
> + close(fd);
> + if (mmap == MAP_FAILED)
> + die_errno("unable to map index file");
>
> - hdr = mmap;
> - if (verify_hdr_version(istate, hdr, mmap_size) < 0)
> - goto unmap;
> + hdr = mmap;
> + if (verify_hdr_version(istate, hdr, mmap_size) < 0)
> + err = 1;
>
> - if (istate->ops->verify_hdr(mmap, mmap_size) < 0)
> - goto unmap;
> + if (istate->ops->verify_hdr(mmap, mmap_size) < 0)
> + err = 1;
>
> - istate->ops->read_index(istate, mmap, mmap_size);
> - istate->timestamp.sec = st.st_mtime;
> - istate->timestamp.nsec = ST_MTIME_NSEC(st);
> + if (istate->ops->read_index(istate, mmap, mmap_size) < 0)
> + err = 1;
> + istate->timestamp.sec = st_old.st_mtime;
> + istate->timestamp.nsec = ST_MTIME_NSEC(st_old);
> + if (lstat(path, &st_new))
> + die_errno("cannot stat the open index");
>
> - munmap(mmap, mmap_size);
> - return istate->cache_nr;
> + munmap(mmap, mmap_size);
> +
> + if (!index_changed(st_old, st_new) && !err)
> + return istate->cache_nr;
> +
> + usleep(10*1000);
usleep() is not available to anybody, e.g. it is not in HP NonStop (not in
every case at least)
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v6 15/16] remote-svn: add marks-file regeneration
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> Sent: Saturday, August 25, 2012 5:55 PM
> To: 'florian.achleitner.2.6...@gmail.com'
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH v6 15/16] remote-svn: add marks-file regeneration
>
> "Florian Achleitner" schrieb im
> Newsbeitrag news:<1345662961-9587-16-git-send-email-
> florian.achleitner.2.6...@gmail.com>...
> > fast-import mark files are stored outside the object database and are
> > therefore not fetched and can be lost somehow else. marks provide a
> > svn revision --> git sha1 mapping, while the notes that are attached
> > to each commit when it is imported provide a git sha1 --> svn revision
> > mapping.
> >
> > If the marks file is not available or not plausible, regenerate it by
> > walking through the notes tree. , i.e. The plausibility check tests
> > if the highest revision in the marks file matches the revision of the
> > top ref. It doesn't ensure that the mark file is completely correct.
> > This could only be done with an effort equal to unconditional
> > regeneration.
> >
> > Signed-off-by: Florian Achleitner
> > Signed-off-by: Junio C Hamano
> > ---
> > remote-testsvn.c | 68
> > ++
> > 1 file changed, 68 insertions(+)
> >
> > diff --git a/remote-testsvn.c b/remote-testsvn.c
> > index e90d221..09dc304 100644
> > --- a/remote-testsvn.c
> > +++ b/remote-testsvn.c
> ...
> > +static void check_or_regenerate_marks(int latestrev) {
> > + FILE *marksfile;
> > + char *line = NULL;
> > + size_t linelen = 0;
> > + struct strbuf sb = STRBUF_INIT;
> > + int found = 0;
> > +
> > + if (latestrev < 1)
> > + return;
> > +
> > + init_notes(NULL, notes_ref, NULL, 0);
> > + marksfile = fopen(marksfilename, "r");
> > + if (!marksfile) {
> > + regenerate_marks();
> > + marksfile = fopen(marksfilename, "r");
> > + if (!marksfile)
> > + die_errno("cannot read marks file %s!", marksfilename);
> > + fclose(marksfile);
> > + } else {
> > + strbuf_addf(&sb, ":%d ", latestrev);
> > + while (getline(&line, &linelen, marksfile) != -1) {
>
> getline() is not available to anybody, e.g. it is not in HP NonStop.
I'd like to confirm that Ramsey's patch works for me too, so I second his
request.
(Subject: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension
function)
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] remote-testsvn.c: Avoid the getline() GNU extension function
Ramsay Jones wrote:
The getline() function is a GNU extension (you need to define
_GNU_SOURCE before including stdio.h) and is, therefore, not
portable. In particular, getline() is not available on MinGW.
In order to support non-GNU systems, we replace the call to
getline() with (almost) equivalent code using strbuf_getline().
Note that, unlike getline(), strbuf_getline() removes the
newline terminator from the returned string. This difference
in semantics does not matter at this call-site. Also, we note
that the original code was leaking the memory allocated to
'line' by getline().
Signed-off-by: Ramsay Jones ram...@ramsay1.demon.co.uk
Tested-by: Joachim Schmitz j...@schmitz-digital.de
---
Hi Florian,
Could you please squash this into commit 0320cef0 ("remote-svn: add
marks-file regeneration", 22-08-2012).
ATB,
Ramsay Jones
remote-testsvn.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/remote-testsvn.c b/remote-testsvn.c
index 09dc304..d0b81d5 100644
--- a/remote-testsvn.c
+++ b/remote-testsvn.c
@@ -121,9 +121,8 @@ static void regenerate_marks(void)
static void check_or_regenerate_marks(int latestrev) {
FILE *marksfile;
- char *line = NULL;
- size_t linelen = 0;
struct strbuf sb = STRBUF_INIT;
+ struct strbuf line = STRBUF_INIT;
int found = 0;
if (latestrev < 1)
@@ -139,8 +138,8 @@ static void check_or_regenerate_marks(int
latestrev) { fclose(marksfile);
} else {
strbuf_addf(&sb, ":%d ", latestrev);
- while (getline(&line, &linelen, marksfile) != -1) {
- if (!prefixcmp(line, sb.buf)) {
+ while (strbuf_getline(&line, marksfile, '\n') != EOF) {
+ if (!prefixcmp(line.buf, sb.buf)) {
found++;
break;
}
@@ -151,6 +150,7 @@ static void check_or_regenerate_marks(int
latestrev) { }
free_notes(NULL);
strbuf_release(&sb);
+ strbuf_release(&line);
}
static int cmd_import(const char *line)
I'd like to second this request, having the same problem on HP NonStop and
this patch fixes it for me too.
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Tuesday, August 28, 2012 10:16 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
>
> "Joachim Schmitz" writes:
>
> > Implementation includes getitimer(), but for now it is static.
> > Supports ITIMER_REAL only.
> >
> > Signed-off-by: Joachim Schmitz
> > ---
> > May need a header file for ITIMER_*, struct itimerval and the prototypes,
> > But for now, and the HP NonStop platform this isn't needed, here
> > has ITIMER_* and struct timeval, and the prototypes can
> > vo into git-compat-util.h for now (Patch 2/2)
> >
> > compat/itimer.c | 50 ++
> > 1 file changed, 50 insertions(+)
> > create mode 100644 compat/itimer.c
> >
> > diff --git a/compat/itimer.c b/compat/itimer.c
> > new file mode 100644
> > index 000..713f1ff
> > --- /dev/null
> > +++ b/compat/itimer.c
> > @@ -0,0 +1,50 @@
> > +#include "../git-compat-util.h"
> > +
> > +static int git_getitimer(int which, struct itimerval *value)
> > +{
> > + int ret = 0;
> > +
> > + switch (which) {
> > + case ITIMER_REAL:
> > + value->it_value.tv_usec = 0;
> > + value->it_value.tv_sec = alarm(0);
> > + ret = 0; /* if alarm() fails, we get a SIGLIMIT */
> > + break;
> > + case ITIMER_VIRTUAL: /* FALLTHRU */
> > + case ITIMER_PROF: errno = ENOTSUP; ret = -1; break;
> > + default: errno = EINVAL; ret = -1;
> > + }
>
> Just a style thing, but we align case arms and switch statements,
> like this:
>
> switch (which) {
> case ...:
> stmt;
> break;
> default:
> stmt;
> break;
> }
OK, I'll fix the syle
> Because alarm() runs in integral seconds granularity, this could
> return 0.0 sec (i.e. "do not re-trigger this alarm any more") in
> ovalue after setting alarm(1) (via git_setitimer()) and calling this
> function (via git_setitimer() again) before the timer expires, no?
> Is it a desired behaviour?
Unintentional, never really thought about this.
> What I am most worried about is that callers _might_ take this
> emulation too seriously, grab the remainder from getitimer(), and
> drives a future call to getitimer() with the returned value, and
> accidentally cause the "recurring" nature of the request to be
> disabled.
>
> I see no existing code calls setitimer() with non-NULL ovalue, and I
> do not think we would add a new caller that would do so in any time
> soon, so it may not be a bad idea to drop support of returning the
> remaining timer altogether from this emulation layer (just like
> giving anything other than ITIMER_REAL gives us ENOTSUP). That
> would sidestep the whole "we cannot answer how many milliseconds are
> still remaining on the timer when using emulation based on alarm()".
Should we leave tv_usec untouched then? That was we round up on the next (and
subsequent?) round(s). Or just set to ENOTSUP in
setitimer if ovalue is !NULL?
> > +int git_setitimer(int which, const struct itimerval *value,
> > + struct itimerval *ovalue)
> > +{
> > + int ret = 0;
> > +
> > + if (!value
> > + || value->it_value.tv_usec < 0
> > + || value->it_value.tv_usec > 100
> > + || value->it_value.tv_sec < 0) {
> > + errno = EINVAL;
> > + return -1;
> > + }
> > +
> > + else if (ovalue)
> > + if (!git_getitimer(which, ovalue))
> > + return -1; /* errno set in git_getitimer() */
> > +
> > + else
> > + switch (which) {
> > + case ITIMER_REAL:
> > + alarm(value->it_value.tv_sec +
> > + (value->it_value.tv_usec > 0) ? 1 : 0);
>
> Why is this capped to 1 second? Is this because no existing code
> uses the timer for anything other than 1 second or shorter? If that
> is the case, that needs at least some documenting (or a possibly
> support for longer expiration, if it is not too cumbersome to add).
As you mention alarm() has only seconds resolution. It is tv_sec plus 1 if
there are tv_usecs > 0, it is rounding up, so we don't
cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to me?
> > + ret = 0; /* i
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Thursday, August 30, 2012 7:14 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
>
> "Joachim Schmitz" writes:
>
> >> I see no existing code calls setitimer() with non-NULL ovalue, and I
> >> do not think we would add a new caller that would do so in any time
> >> soon, so it may not be a bad idea to drop support of returning the
> >> remaining timer altogether from this emulation layer (just like
> >> giving anything other than ITIMER_REAL gives us ENOTSUP). That
> >> would sidestep the whole "we cannot answer how many milliseconds are
> >> still remaining on the timer when using emulation based on alarm()".
> >
> > Should we leave tv_usec untouched then? That was we round up on
> > the next (and subsequent?) round(s). Or just set to ENOTSUP in
> > setitimer if ovalue is !NULL?
>
> I was alluding to the latter.
OK, will do that then.
> >> > +switch (which) {
> >> > +case ITIMER_REAL:
> >> > +alarm(value->it_value.tv_sec +
> >> > +(value->it_value.tv_usec > 0) ? 1 : 0);
> >>
> >> Why is this capped to 1 second? Is this because no existing code
> >> uses the timer for anything other than 1 second or shorter? If that
> >> is the case, that needs at least some documenting (or a possibly
> >> support for longer expiration, if it is not too cumbersome to add).
> >
> > As you mention alarm() has only seconds resolution. It is tv_sec
> > plus 1 if there are tv_usecs > 0, it is rounding up, so we don't
> > cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to
> > me?
>
> Can a caller use setitimer to be notified in 5 seconds?
Yes, by setting tv_sec to 5 and tv_usec to 0, or be setting tv_sec to 4 and
tv_usec to something > 0.
Unless I screwed up the operator precedence?
To make it clearer (any possibly correct?):
switch (which) {
case ITIMER_REAL:
alarm(value->it_value.tv_sec +
((value->it_value.tv_usec > 0) ? 1 : 0));
Or even just
switch (which) {
case ITIMER_REAL:
alarm(value->it_value.tv_sec + (value->it_value.tv_usec
> 0));
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> Sent: Thursday, August 30, 2012 7:23 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
>
> > From: Junio C Hamano [mailto:gits...@pobox.com]
> > Sent: Thursday, August 30, 2012 7:14 PM
> > To: Joachim Schmitz
> > Cc: git@vger.kernel.org
> > Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
> >
> > "Joachim Schmitz" writes:
> >
> > >> I see no existing code calls setitimer() with non-NULL ovalue, and I
> > >> do not think we would add a new caller that would do so in any time
> > >> soon, so it may not be a bad idea to drop support of returning the
> > >> remaining timer altogether from this emulation layer (just like
> > >> giving anything other than ITIMER_REAL gives us ENOTSUP). That
> > >> would sidestep the whole "we cannot answer how many milliseconds are
> > >> still remaining on the timer when using emulation based on alarm()".
> > >
> > > Should we leave tv_usec untouched then? That was we round up on
> > > the next (and subsequent?) round(s). Or just set to ENOTSUP in
> > > setitimer if ovalue is !NULL?
> >
> > I was alluding to the latter.
>
> OK, will do that then.
>
> > >> > + switch (which) {
> > >> > + case ITIMER_REAL:
> > >> > + alarm(value->it_value.tv_sec +
> > >> > + (value->it_value.tv_usec > 0) ? 1 : 0);
> > >>
> > >> Why is this capped to 1 second? Is this because no existing code
> > >> uses the timer for anything other than 1 second or shorter? If that
> > >> is the case, that needs at least some documenting (or a possibly
> > >> support for longer expiration, if it is not too cumbersome to add).
> > >
> > > As you mention alarm() has only seconds resolution. It is tv_sec
> > > plus 1 if there are tv_usecs > 0, it is rounding up, so we don't
> > > cancel the alarm() if tv_sec is 0 but tv_usec is not. Looks OK to
> > > me?
> >
> > Can a caller use setitimer to be notified in 5 seconds?
>
> Yes, by setting tv_sec to 5 and tv_usec to 0, or be setting tv_sec to 4 and
> tv_usec to something > 0.
>
> Unless I screwed up the operator precedence?
> To make it clearer (any possibly correct?):
>
> switch (which) {
> case ITIMER_REAL:
> alarm(value->it_value.tv_sec +
> ((value->it_value.tv_usec > 0) ? 1 : 0));
>
> Or even just
> switch (which) {
> case ITIMER_REAL:
> alarm(value->it_value.tv_sec + (value->it_value.tv_usec
> > 0));
OK, here it goes again, not yet as a patch, just plain code for comment:
$ cat itimer.c
/*
* Rely on system headers () to contain struct itimerval
* and git-compat-util.h to have the prototype for git_getitimer().
* As soon as there's a platform where that is not the case, we'd need
* an itimer .h.
*/
#include "../git-compat-util.h"
#ifndef NO_GETITIMER /* not yet needed anywhere else in git */
static
#endif
int git_getitimer(int which, struct itimerval *value)
{
int ret = 0;
if (!value) {
errno = EFAULT;
return -1;
}
switch (which) {
case ITIMER_REAL:
#if 0
value->it_value.tv_usec = 0;
value->it_value.tv_sec = alarm(0);
ret = 0; /* if alarm() fails, we get a SIGLIMIT */
break;
#else
/*
* As an emulation via alarm(0) won't tell us how many
* usecs are left, we don't support it altogether.
*/
#endif
case ITIMER_VIRTUAL:
case ITIMER_PROF:
errno = ENOTSUP;
ret = -1;
break;
default:
errno = EINVAL;
ret = -1;
break;
}
return ret;
}
int git_setitimer(int which, const struct itimerval *value,
struct itimerval *ovalue)
{
int ret = 0;
if (!value ) {
errno = EFAULT;
return -1;
}
if ( value->it_value.tv_sec < 0
|| value->it_value.tv_usec > 100
|| value->it_value.tv_usec < 0) {
errno = EINVAL;
return -1;
}
if ((ovalue) && (git_getitimer(which, ovalu
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Sunday, September 02, 2012 10:44 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; Johannes Sixt
> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
>
> "Joachim Schmitz" writes:
>
> >> > > Should we leave tv_usec untouched then? That was we round up on
> >> > > the next (and subsequent?) round(s). Or just set to ENOTSUP in
> >> > > setitimer if ovalue is !NULL?
> >> >
> >> > I was alluding to the latter.
> >>
> >> OK, will do that then.
>
> Thanks.
>
> >> Unless I screwed up the operator precedence?
>
> I think you did, but not in the version we see below.
>
> > int git_setitimer(int which, const struct itimerval *value,
> > struct itimerval *ovalue)
> > {
> > int ret = 0;
> >
> > if (!value ) {
>
> Style: space before ')'?
Will fix.
> > errno = EFAULT;
> > return -1;
>
> EFAULT is good ;-)
That's what 'man setitimer()' on Linux says to happen if invalid value is found.
> The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and
> sigaction()., 2007-11-13) may want to be tightened in a similar way.
Hmm, I see that there the errors are handled differently, like this:
if (ovalue != NULL)
return errno = EINVAL,
error("setitimer param 3 != NULL not implemented");
Should this be done in my setitimer() too? Or rather be left to the caller?
I tend to the later.
> > }
> >
> > if ( value->it_value.tv_sec < 0
>
> Style: space after ')'?
After '(', I guess? Will fix.
> > || value->it_value.tv_usec > 100
> > || value->it_value.tv_usec < 0) {
> > errno = EINVAL;
> > return -1;
> > }
> >
> > if ((ovalue) && (git_getitimer(which, ovalue) == -1))
> > return -1; /* errno set in git_getitimer() */
>
> As nobody passes non-NULL ovalue to setitimer(), I think we should
> instead get rid of git_getitmier() implemenation, and change this to
True.
> if (ovalue) {
> errno = ENOTSUP;
> return -1;
> }
>
> which is how I understood what "the latter" in the paragraph I
> quoted from you above meant.
OK, will do this and then I'll rename the entire file into getitimer.c.
> > switch (which) {
> > case ITIMER_REAL:
> > /* If tv_usec is > 0, round up to next full sec */
> > alarm(value->it_value.tv_sec + (value->it_value.tv_usec > 0));
>
> OK.
>
> > ret = 0; /* if alarm() fails, we get a SIGLIMIT */
> > break;
> > case ITIMER_VIRTUAL:
> > case ITIMER_PROF:
> > errno = ENOTSUP;
> > ret = -1;
> > break;
> > default:
> > errno = EINVAL;
> > ret = -1;
> > break;
> > }
> >
> > return ret;
> > }
>
> Other than that, looks good.
>
> Thanks.
I had a closer look at the places in git where setitimer() is used. It is in 2
files, progress.c and builtin/log.c.
In progress.c :
static void set_progress_signal(void)
{
struct sigaction sa;
struct itimerval v;
progress_update = 0;
memset(&sa, 0, sizeof(sa));
sa.sa_handler = progress_interval;
sigemptyset(&sa.sa_mask);
sa.sa_flags = SA_RESTART;
sigaction(SIGALRM, &sa, NULL);
v.it_interval.tv_sec = 1;
v.it_interval.tv_usec = 0;
v.it_value = v.it_interval;
setitimer(ITIMER_REAL, &v, NULL);
}
static void clear_progress_signal(void)
{
struct itimerval v = {{0,},};
setitimer(ITIMER_REAL, &v, NULL);
signal(SIGALRM, SIG_IGN);
progress_update = 0;
}
So it uses a 1 sec timeout, which is a good match to my implementation, but
also uses it_interval, meant to 're-arm' the timer after
it expired.
My implementation doesn't do that at all, and I also don't see how it possibly
could (short of installing a signal handler, which
then conflicts with the one use in progress.c).
On top here SA_RESTART is used, which is not available in HP NonStop (so I have
a "-DSA_RESTART=0" in COMPAT_CFLAGS).
In builtin/log.c it doesn't use it_interval, which is a good match to my
implementation, but uses 1/2 a sec and 1/10 sec, so here
would be a victim of a 1 sec upgrade. This is probably accepta
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com]
> Sent: Monday, September 03, 2012 9:03 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; 'Johannes Sixt'
> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it
>
> "Joachim Schmitz" writes:
>
> >> > if (!value ) {
> >>
> >> Style: space before ')'?
> >
> > Will fix.
> >
> >> > errno = EFAULT;
> >> > return -1;
> >>
> >> EFAULT is good ;-)
> >
> > That's what 'man setitimer()' on Linux says to happen if invalid value is
> > found.
> >
> >> The emulation in mingw.c 6072fc3 (Windows: Implement setitimer() and
> >> sigaction()., 2007-11-13) may want to be tightened in a similar way.
> >
>
> > Hmm, I see that there the errors are handled differently, like this:
> >
> > if (ovalue != NULL)
> > return errno = EINVAL,
> > error("setitimer param 3 != NULL not implemented");
> >
> > Should this be done in my setitimer() too? Or rather be left to the caller?
> > I tend to the later.
>
> I don't care too deeply either way. The above was not a comment
> meant for you, but was to point out the error checking when the
> newvalue is NULL---it is missing in mingw.c and I think the
> condition should be checked.
Ah, OK. Guess Johannes and I misunderstood ;-)
> > On top here SA_RESTART is used, which is not available in HP
> > NonStop (so I have a "-DSA_RESTART=0" in COMPAT_CFLAGS).
>
> If you cannot re-trigger the timer, then you will see "20%" shown
> after one second, silence for 4 seconds and then "done", for an
> operation that takes 5 seconds. Which is not the end of the world,
> though. It does not affect correctness.
That does seem to work, if I do e.g. a "git clone" on git itself (being a
fairly large repository), I see it updating the % values
about once per second.
> The other use of itimer in our codebase is the early-output timer,
> but that also is about perceived latency, and not about correctness,
> so it is possible that you do not have to support anything (i.e. not
> even setting an alarm) at all.
OK, I'll go for that one-liner in git-compat-utils.h then
#ifdef NO_SETITIMER /* poor man's setitimer() */
#define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))
#endif
It certainly seems to work just fine for me.
Could as well be #ifdef __TANDEM, I won't mind.
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support for setitimer() on platforms lacking it
Poor man's getitimer(), simply based on alarm().
Currently needed on HP NonStop ("#ifdef __TANDEM"),
which does provide "struct itimerval", but no setitimer().
Alarm times are rounded up to the next full second.
Signed-off-by: Joachim Schmitz
---
Revert/remove my previous 2 patches for this first (from 'pu').
git-compat-util.h | 4
1 file changed, 4 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 18089f0..55b9421 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h@@ -163,6 +163,10 @@
#define probe_utf8_pathname_composition(a,b)
#endif
+#ifdef NO_SETITIMER /* poor man's setitimer() */
+#define setitimer(w,v,o) alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))
+#endif
+
#ifdef MKDIR_WO_TRAILING_SLASH
#define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);
--
1.7.12
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support for setitimer() on platforms lacking it
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> Sent: Tuesday, September 04, 2012 12:31 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'
> Subject: RE: [PATCH v2] Support for setitimer() on platforms lacking it
>
>
> Poor man's getitimer(), simply based on alarm().
>
> Currently needed on HP NonStop ("#ifdef __TANDEM"),
> which does provide "struct itimerval", but no setitimer().
> Alarm times are rounded up to the next full second.
>
> Signed-off-by: Joachim Schmitz
> ---
> Revert/remove my previous 2 patches for this first (from 'pu').
Actually: https://github.com/git/git/commit/55c96a1325
> git-compat-util.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 18089f0..55b9421 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h@@ -163,6 +163,10 @@
> #define probe_utf8_pathname_composition(a,b)
> #endif
>
> +#ifdef NO_SETITIMER /* poor man's setitimer() */
> +#define setitimer(w,v,o)
> alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0))
> +#endif
> +
> #ifdef MKDIR_WO_TRAILING_SLASH
> #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
> extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);
> --
> 1.7.12
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH v2] Support non-WIN32 system lacking poll() while keeping the WIN32 part intact
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Friday, August 24, 2012 9:47 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; 'Erik Faye-Lund' > Subject: Re: [PATCH v2] Support non-WIN32 system lacking poll() while keeping > the WIN32 part intact > > "Joachim Schmitz" writes: > > > Different, but related question: would poll.[ch] be allowed to #include > > "git-compat-util.h"? > > Seeing other existing generic wrappers directly under compat/, > e.g. fopen.c, mkdtemp.c, doing so, I would say why not. > > Windows folks (I see Erik is already CC'ed, which is good ;-), > please work with Joachim to make sure such a move won't break your > builds. I believe that it should just be the matter of updating a > couple of paths in the top-level Makefile. Haven't heard anything from the Windows folks yet. I'd prefer to move compat/win32/poll.[ch] into compat/poll. Then adjust a few paths in Makefile and that would be the 1st patch A 2nd patch would be my already proposed ones that make this usable for others (me in this case ;-)), namely wrapping 2 #inludes. diff --git a/compat/poll/poll.c b/compat/poll/poll.c index 403eaa7..49541f1 100644 --- a/compat/poll/poll.c +++ b/compat/poll/poll.c @@ -24,7 +24,9 @@ # pragma GCC diagnostic ignored "-Wtype-limits" #endif -#include +#if defined(WIN32) +# include +#endif #include @@ -48,7 +50,9 @@ #else # include # include -# include +# ifndef NO_SYS_SELECT_H +# include +# endif # include #endif -- 1.7.12 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Tuesday, September 04, 2012 6:58 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; 'Johannes Sixt' > Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it > > "Joachim Schmitz" writes: > > >> If you cannot re-trigger the timer, then you will see "20%" shown > >> after one second, silence for 4 seconds and then "done", for an > >> operation that takes 5 seconds. Which is not the end of the world, > >> though. It does not affect correctness. > > > > That does seem to work, if I do e.g. a "git clone" on git itself > > (being a fairly large repository), I see it updating the % values > > about once per second. > > Ehh, so somebody is re-arming the alarm(). I am not sure where, > though. > > ... thinks for a while, then a lightbulb slowly starts to glow ... > > Where are you cloning from, and does the other side of the clone > (i.e. upload-pack) also run on your tandem port? If you are cloning > from one of my public distribution points (e.g. k.org, repo.or.cz, > or github.com), then I think the progress indicator you are seeing > is coming from the other side, not generated by your local timer. I used GutHub The cloning from NonStop doesn't work at all, different story, but looks like poll isn#t working. Not poll's fault tough, but on out plaftom ssh (non-interactive) give a pipe rather than a socket and recv(...MSG_PEEK) then fails with ENOTSOCK > Only with the observation of "clone", I cannot tell if your timer is > working. You can try repacking the test repository you created by > your earlier "git clone" with "git repack -a -d -f" and see what > happens. It does update the counter too. > > OK, I'll go for that one-liner in git-compat-utils.h then > > > > #ifdef NO_SETITIMER /* poor man's setitimer() */ > > #define setitimer(w,v,o) > > alarm((v)->it_value.tv_sec+((v)->it_value.tv_usec>0)) > > #endif > > > > It certainly seems to work just fine for me. Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Tuesday, September 04, 2012 8:47 PM > To: Joachim Schmitz > Cc: git@vger.kernel.org; 'Johannes Sixt' > Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it > > Junio C Hamano writes: > > > "Joachim Schmitz" writes: > > > >>> Only with the observation of "clone", I cannot tell if your timer is > >>> working. You can try repacking the test repository you created by > >>> your earlier "git clone" with "git repack -a -d -f" and see what > >>> happens. > >> > >> It does update the counter too. > > > > Yeah, that was not a very good way to diagnose it. > > > > You see the progress from pack-objects (which is the underlying > > machinery "git repack" uses) only because it knows how many objects > > it is going to pack, and it updates the progress meter for every > > per-cent progress it makes, without any help from the timer > > interrupt. > > I think the "Counting objects: $number" phase is purely driven by > the timer, as there is no way to say "we are done X per-cent so > far". > > Doesn't your repack show "Counting objects: " with a number once, > pause forever and then show "Counting objects: $number, done."? Yes, only once, when it is done $ ./git repack -a -d -f warning: no threads support, ignoring --threads Counting objects: 140302, done. Compressing objects: 1% (1385/138407) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 1/2] Support for setitimer() on platforms lacking it
> From: Junio C Hamano [mailto:gits...@pobox.com] > Sent: Wednesday, September 05, 2012 12:45 AM > To: Joachim Schmitz > Cc: git@vger.kernel.org; 'Johannes Sixt' > Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it > > "Joachim Schmitz" writes: > > >> From: Junio C Hamano [mailto:gits...@pobox.com] > >> Sent: Tuesday, September 04, 2012 8:47 PM > >> To: Joachim Schmitz > >> Cc: git@vger.kernel.org; 'Johannes Sixt' > >> Subject: Re: [PATCH 1/2] Support for setitimer() on platforms lacking it > >> > >> Junio C Hamano writes: > >> > >> > "Joachim Schmitz" writes: > >> > > >> >>> Only with the observation of "clone", I cannot tell if your timer is > >> >>> working. You can try repacking the test repository you created by > >> >>> your earlier "git clone" with "git repack -a -d -f" and see what > >> >>> happens. > >> >> > >> >> It does update the counter too. > >> > > >> > Yeah, that was not a very good way to diagnose it. > >> > > >> > You see the progress from pack-objects (which is the underlying > >> > machinery "git repack" uses) only because it knows how many objects > >> > it is going to pack, and it updates the progress meter for every > >> > per-cent progress it makes, without any help from the timer > >> > interrupt. > >> > >> I think the "Counting objects: $number" phase is purely driven by > >> the timer, as there is no way to say "we are done X per-cent so > >> far". > >> > >> Doesn't your repack show "Counting objects: " with a number once, > >> pause forever and then show "Counting objects: $number, done."? > > > > Yes, only once, when it is done > > $ ./git repack -a -d -f > > warning: no threads support, ignoring --threads > > Counting objects: 140302, done. > > Compressing objects: 1% (1385/138407) > > So this strongly suggests that (1) your "poor-man's" is not a real > substitute for recurring itimer, and (2) users could live with the > progress.c code without any itimer firing. OK > Perhaps a no-op macro would work equally well? Like the following: diff --git a/git-compat-util.h b/git-compat-util.h index 18089f0..55b9421 100644 --- a/git-compat-util.h +++ b/git-compat-util.h@@ -163,6 +163,10 @@ #define probe_utf8_pathname_composition(a,b) #endif +#ifdef NO_SETITIMER +#define setitimer(w,v,o) /* NOP */ +#endif + #ifdef MKDIR_WO_TRAILING_SLASH #define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b)) extern int compat_mkdir_wo_trailing_slash(const char*, mode_t); Does work for me and does not seem to make any difference, not in those test cases at least Does the inability to re-arm the timer depend on SA_RESTART, possibly? If so we may instead want #if SA_RSTART == 0 && defined(NO_SETITIMER) Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: poll() emulation in git
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> Sent: Tuesday, September 04, 2012 1:49 PM
> To: 'Junio C Hamano'
> Cc: 'git@vger.kernel.org'; 'Erik Faye-Lund'
> Subject: RE: [PATCH v2] Support non-WIN32 system lacking poll() while keeping
> the WIN32 part intact
>
> > From: Junio C Hamano [mailto:gits...@pobox.com]
> > Sent: Friday, August 24, 2012 9:47 PM
> > To: Joachim Schmitz
> > Cc: git@vger.kernel.org; 'Erik Faye-Lund'
> > Subject: Re: [PATCH v2] Support non-WIN32 system lacking poll() while
> > keeping the WIN32 part intact
> >
> > "Joachim Schmitz" writes:
> >
> > > Different, but related question: would poll.[ch] be allowed to #include
> > > "git-compat-util.h"?
> >
> > Seeing other existing generic wrappers directly under compat/,
> > e.g. fopen.c, mkdtemp.c, doing so, I would say why not.
> >
> > Windows folks (I see Erik is already CC'ed, which is good ;-),
> > please work with Joachim to make sure such a move won't break your
> > builds. I believe that it should just be the matter of updating a
> > couple of paths in the top-level Makefile.
>
> Haven't heard anything from the Windows folks yet.
>
> I'd prefer to move compat/win32/poll.[ch] into compat/poll.
> Then adjust a few paths in Makefile and that would be the 1st patch
>
> A 2nd patch would be my already proposed ones that make this usable for
> others (me in this case ;-)), namely wrapping 2 #inludes.
>
> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> index 403eaa7..49541f1 100644
> --- a/compat/poll/poll.c
> +++ b/compat/poll/poll.c
> @@ -24,7 +24,9 @@
> # pragma GCC diagnostic ignored "-Wtype-limits"
> #endif
>
> -#include
> +#if defined(WIN32)
> +# include
> +#endif
>
> #include
>
> @@ -48,7 +50,9 @@
> #else
> # include
> # include
> -# include
> +# ifndef NO_SYS_SELECT_H
> +# include
> +# endif
> # include
> #endif
>
> --
> 1.7.12
However: this poll implementation, while compiling OK, doesn't work properly.
Because it uses recv(...,MSG_PEEK), it works on sockets only (returns ENOTSOCK
on anything else), while the real poll() works on all
kind if file descriptors, at least that is my understanding.
Here on HP NonStop, when being connected via an non-interactive SSH, we get a
set of pipes (stdin, stdout, stderr) instead of a
socket to talk to, so the poll() just hangs/loops.
As git's implementation is based on ('stolen' from?) gnulib's and still pretty
similar, CC to the gnulib list and Paolo
Any idea how this could get solved? I.e. how to implement a poll() that works
on non-sockets too?
There is some code that pertains to a seemingly similar problem in Mac OS X,
but my problem is not identical, as that fix doesn't
help.
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: poll() emulation in git
> From: Bastien ROUCARIES [mailto:roucaries.bast...@gmail.com]
> Sent: Wednesday, September 05, 2012 1:55 PM
> To: Joachim Schmitz
> Cc: Junio C Hamano; Paolo Bonzini; bug-gnu...@gnu.org; git@vger.kernel.org;
> Erik Faye-Lund
> Subject: Re: poll() emulation in git
>
> On Wed, Sep 5, 2012 at 1:24 PM, Joachim Schmitz
> wrote:
> >> From: Joachim Schmitz [mailto:j...@schmitz-digital.de]
> >> Sent: Tuesday, September 04, 2012 1:49 PM
> >> To: 'Junio C Hamano'
> >> Cc: 'git@vger.kernel.org'; 'Erik Faye-Lund'
> >> Subject: RE: [PATCH v2] Support non-WIN32 system lacking poll() while
> >> keeping the WIN32 part intact
> >>
> >> > From: Junio C Hamano [mailto:gits...@pobox.com]
> >> > Sent: Friday, August 24, 2012 9:47 PM
> >> > To: Joachim Schmitz
> >> > Cc: git@vger.kernel.org; 'Erik Faye-Lund'
> >> > Subject: Re: [PATCH v2] Support non-WIN32 system lacking poll() while
> >> > keeping the WIN32 part intact
> >> >
> >> > "Joachim Schmitz" writes:
> >> >
> >> > > Different, but related question: would poll.[ch] be allowed to
> >> > > #include "git-compat-util.h"?
> >> >
> >> > Seeing other existing generic wrappers directly under compat/,
> >> > e.g. fopen.c, mkdtemp.c, doing so, I would say why not.
> >> >
> >> > Windows folks (I see Erik is already CC'ed, which is good ;-),
> >> > please work with Joachim to make sure such a move won't break your
> >> > builds. I believe that it should just be the matter of updating a
> >> > couple of paths in the top-level Makefile.
> >>
> >> Haven't heard anything from the Windows folks yet.
> >>
> >> I'd prefer to move compat/win32/poll.[ch] into compat/poll.
> >> Then adjust a few paths in Makefile and that would be the 1st patch
> >>
> >> A 2nd patch would be my already proposed ones that make this usable for
> >> others (me in this case ;-)), namely wrapping 2 #inludes.
> >>
> >> diff --git a/compat/poll/poll.c b/compat/poll/poll.c
> >> index 403eaa7..49541f1 100644
> >> --- a/compat/poll/poll.c
> >> +++ b/compat/poll/poll.c
> >> @@ -24,7 +24,9 @@
> >> # pragma GCC diagnostic ignored "-Wtype-limits"
> >> #endif
> >>
> >> -#include
> >> +#if defined(WIN32)
> >> +# include
> >> +#endif
> >>
> >> #include
> >>
> >> @@ -48,7 +50,9 @@
> >> #else
> >> # include
> >> # include
> >> -# include
> >> +# ifndef NO_SYS_SELECT_H
> >> +# include
> >> +# endif
> >> # include
> >> #endif
> >>
> >> --
> >> 1.7.12
> >
> > However: this poll implementation, while compiling OK, doesn't work
> > properly.
> > Because it uses recv(...,MSG_PEEK), it works on sockets only (returns
> > ENOTSOCK on anything else), while the real poll() works on all
> > kind if file descriptors, at least that is my understanding.
> >
> > Here on HP NonStop, when being connected via an non-interactive SSH, we get
> > a set of pipes (stdin, stdout, stderr) instead of a
> > socket to talk to, so the poll() just hangs/loops.
> >
> > As git's implementation is based on ('stolen' from?) gnulib's and still
> > pretty similar, CC to the gnulib list and Paolo
> >
> > Any idea how this could get solved? I.e. how to implement a poll() that
> > works on non-sockets too?
> > There is some code that pertains to a seemingly similar problem in Mac OS
> > X, but my problem is not identical, as that fix doesn't
> > help.
>
> Could you give more context ? Are you speaking about win32 or about HP non
> stop?
HP NonStop
> If so pipe are broken and unfixable under windows see
> http://www.mail-archive.com/bug-gnulib@gnu.org/msg23365.html
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: poll() emulation in git
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo > Bonzini > Sent: Wednesday, September 05, 2012 2:05 PM > To: Joachim Schmitz > Cc: 'Junio C Hamano'; git@vger.kernel.org; 'Erik Faye-Lund'; > bug-gnu...@gnu.org > Subject: Re: poll() emulation in git > > Il 05/09/2012 13:24, Joachim Schmitz ha scritto: > > However: this poll implementation, while compiling OK, doesn't work > > properly. > > Because it uses recv(...,MSG_PEEK), it works on sockets only (returns > > ENOTSOCK on anything else), while the real poll() works on all > > kind if file descriptors, at least that is my understanding. > > Actually recv(...,MSG_PEEK) on most Unix variants works on non-sockets > too. The trick is taken from GNU Pth in turn. > > > Here on HP NonStop, when being connected via an non-interactive SSH, we get > > a set of pipes (stdin, stdout, stderr) instead of a > > socket to talk to, so the poll() just hangs/loops. > > Does your system have a working FIONREAD ioctl for pipes? It does have FIONREAD ioctl. Whether it works properly is to be determined... I'll test if you could show me how? Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: poll() emulation in git
> From: Joachim Schmitz [mailto:j...@schmitz-digital.de] > Sent: Wednesday, September 05, 2012 2:58 PM > To: 'Paolo Bonzini' > Cc: 'Junio C Hamano'; 'git@vger.kernel.org'; 'Erik Faye-Lund'; > 'bug-gnu...@gnu.org' > Subject: RE: poll() emulation in git > > > From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo > > Bonzini > > Sent: Wednesday, September 05, 2012 2:05 PM > > To: Joachim Schmitz > > Cc: 'Junio C Hamano'; git@vger.kernel.org; 'Erik Faye-Lund'; > > bug-gnu...@gnu.org > > Subject: Re: poll() emulation in git > > > > Il 05/09/2012 13:24, Joachim Schmitz ha scritto: > > > However: this poll implementation, while compiling OK, doesn't work > > > properly. > > > Because it uses recv(...,MSG_PEEK), it works on sockets only (returns > > > ENOTSOCK on anything else), while the real poll() works on all > > > kind if file descriptors, at least that is my understanding. > > > > Actually recv(...,MSG_PEEK) on most Unix variants works on non-sockets > > too. The trick is taken from GNU Pth in turn. > > > > > Here on HP NonStop, when being connected via an non-interactive SSH, we > > > get a set of pipes (stdin, stdout, stderr) instead of a > > > socket to talk to, so the poll() just hangs/loops. > > > > Does your system have a working FIONREAD ioctl for pipes? > > It does have FIONREAD ioctl. Whether it works properly is to be determined... > I'll test if you could show me how? Oh, now I see what you aimed at, but no, that Mac OS X method doesn't work for me, I tried (at least I think I did). And has /* * Normal IOCTL's supported by the socket interface */ #define FIONREAD_IOR(0, 8, _ioctl_int) /* Num of bytes to read */ #define FIONBIO _IOW(0, 9, _ioctl_int) /* Non-blocking I/O */ So these seem to be supported on sockets only, I guess. And indeed the man pages for ioctl confirms: Valid values for the request parameter for AF_INET or AF_INET6 sockets are: FIONREAD Gets the number of bytes available for reading and stores it at the int pointed at by arg. So not even AF_UNIX sockets, not to mention pipes... Bye, Jojo -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: poll() emulation in git
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Wednesday, September 05, 2012 5:26 PM
> To: Joachim Schmitz
> Cc: 'Junio C Hamano'; git@vger.kernel.org; 'Erik Faye-Lund';
> bug-gnu...@gnu.org
> Subject: Re: poll() emulation in git
>
> Il 05/09/2012 15:36, Joachim Schmitz ha scritto:
> >>> > > Does your system have a working FIONREAD ioctl for pipes?
> >> >
> >> > It does have FIONREAD ioctl. Whether it works properly is to be
> >> > determined...
> >> > I'll test if you could show me how?
> > Oh, now I see what you aimed at, but no, that Mac OS X method doesn't work
> > for me, I tried (at least I think I did).
> >
> > And has
> > /*
> > * Normal IOCTL's supported by the socket interface
> > */
> > #define FIONREAD_IOR(0, 8, _ioctl_int) /* Num of bytes to
> > read */
> > #define FIONBIO _IOW(0, 9, _ioctl_int) /* Non-blocking I/O
> > */
> >
> > So these seem to be supported on sockets only, I guess.
> > And indeed the man pages for ioctl confirms:
> >
> > Valid values for the request parameter for AF_INET or
> > AF_INET6 sockets are:
> >
> >
> > FIONREAD Gets the number of bytes available for reading and
> > stores it at the int pointed at by arg.
> >
> >
> > So not even AF_UNIX sockets, not to mention pipes...
>
> So there's no way you can support POLLHUP. Your system is quite
> crippled. :(
Unfortunatly.
But is there something that could be done to make git work even without poll()?
It is used in 5 places:
$ grep -n poll\( *.c */*.c
credential-cache--daemon.c:175: if (poll(&pfd, 1, 1000 * wakeup) < 0) {
daemon.c:1018: if (poll(pfd, socklist->nr, -1) < 0) {
help.c:361: poll(NULL, 0, autocorrect * 100);
upload-pack.c:232: if (poll(pfd, pollsize, -1) < 0) {
builtin/upload-archive.c:125: if (poll(pfd, 2, -1) < 0) {
Don't quite understand why in help.c it has that NULL, which should always
result in an EFAULT and other than that basically is a
NOP (at least in the poll() emulation)? Seems a usleep(autocorrect * 100) is
meant to happen here instead?
So I think here a poll() isn't needed at all. But also the 'broken' one
shouldn't harm too much.
In daemon.c it seems to be all sockets it polls on, so it should work on
NonStop.
Same in credential-cache--daemon.c
Remains upload-pack.c and builtin/upload-archive.c
In both start_command() gathers the FDs to poll() on and that indeed works on
pipes -> problem on NonStop!
Seeing that in those cases xread() takes care of EAGAIN, I've now used 'brute
force' in poll.c:
...
# else
char data[64];
r = recv (fd, data, sizeof (data), MSG_PEEK);
socket_errno = (r < 0) ? errno : 0;
# endif
if (r == 0)
happened |= POLLHUP;
/* If the event happened on an unconnected server socket,
that's fine. */
else if (r > 0 || ( /* (r == -1) && */ socket_errno == ENOTCONN))
happened |= (POLLIN | POLLRDNORM) & sought;
/* Distinguish hung-up sockets from other errors. */
else if (socket_errno == ESHUTDOWN || socket_errno == ECONNRESET
|| socket_errno == ECONNABORTED || socket_errno == ENETRESET)
happened |= POLLHUP;
#ifdef __TANDEM /* as we can't recv(...,MSG_PEEK) on a non-socket */
else if (socket_errno == ENOTSOCK)
happened |= (POLLIN | POLLRDNORM) & sought;
#endif
else
happened |= POLLERR;
}
...
We won't detect POLLHUP that way I think. However it seems to work, we've been
able to clone, push, pull, branch that way with
NonStop being the (ssh-)server, something that didn't work at all without that
hack (and yes, I believe it is just that).
Someone in for a cleaner way of managing this?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: poll() emulation in git
> From: Paolo Bonzini [mailto:paolo.bonz...@gmail.com] On Behalf Of Paolo
> Bonzini
> Sent: Thursday, September 06, 2012 4:32 PM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org; 'Junio C Hamano'; 'Erik Faye-Lund';
> bug-gnu...@gnu.org; rsbec...@nexbridge.com
> Subject: Re: poll() emulation in git
>
> Il 06/09/2012 16:02, Joachim Schmitz ha scritto:
> >
> > But is there something that could be done to make git work even without
> > poll()?
> > It is used in 5 places:
> >
> > $ grep -n poll\( *.c */*.c
> > credential-cache--daemon.c:175: if (poll(&pfd, 1, 1000 * wakeup) < 0) {
> > daemon.c:1018: if (poll(pfd, socklist->nr, -1) < 0) {
> > help.c:361: poll(NULL, 0, autocorrect * 100);
> > upload-pack.c:232: if (poll(pfd, pollsize, -1) < 0) {
> > builtin/upload-archive.c:125: if (poll(pfd, 2, -1) < 0) {
> >
> > Don't quite understand why in help.c it has that NULL, which should always
> > result in an EFAULT and other than that basically is
a
> > NOP (at least in the poll() emulation)? Seems a usleep(autocorrect * 100)
> > is meant to happen here instead?
> > So I think here a poll() isn't needed at all. But also the 'broken' one
> > shouldn't harm too much.
>
> Yes, it's an usleep(autocorrect * 10) basically (poll takes
> milliseconds, not micro).
OK, it is _supposed_ to do this usleep(), but is does not, as poll() returns
early with EFAULT in this case:
/* EFAULT is not necessary to implement, but let's do it in the
simplest case. */
if (!pfd)
{
errno = EFAULT;
return -1;
}
poll() is doing this before calling select(), so won't sleep.
So there's a bug in {gnulib|git}'s poll(), right?
> > ...
> > # else
> > char data[64];
> > r = recv (fd, data, sizeof (data), MSG_PEEK);
> > socket_errno = (r < 0) ? errno : 0;
> > # endif
> > if (r == 0)
> > happened |= POLLHUP;
> >
> > /* If the event happened on an unconnected server socket,
> > that's fine. */
> > else if (r > 0 || ( /* (r == -1) && */ socket_errno == ENOTCONN))
> > happened |= (POLLIN | POLLRDNORM) & sought;
> >
> > /* Distinguish hung-up sockets from other errors. */
> > else if (socket_errno == ESHUTDOWN || socket_errno == ECONNRESET
> >|| socket_errno == ECONNABORTED || socket_errno == ENETRESET)
> > happened |= POLLHUP;
> >
> > #ifdef __TANDEM /* as we can't recv(...,MSG_PEEK) on a non-socket */
> > else if (socket_errno == ENOTSOCK)
> > happened |= (POLLIN | POLLRDNORM) & sought;
> > #endif
> > else
> > happened |= POLLERR;
> > }
> > ...
> >
> > We won't detect POLLHUP that way I think. However it seems to work, we've
> > been able to clone, push, pull, branch that way with
> > NonStop being the (ssh-)server, something that didn't work at all without
> > that hack (and yes, I believe it is just that).
> > Someone in for a cleaner way of managing this?
>
> I suppose it works to always handle ENOTSOCK that way, even on
> non-__TANDEM systems.
So you think this is a clean way of dealing with it?
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH/RFC v4 02/13] read-cache.c: Re-read index if index file changed
> From: Thomas Gummerer [mailto:t.gumme...@gmail.com]
> Sent: Monday, August 27, 2012 11:40 AM
> To: Joachim Schmitz
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH/RFC v4 02/13] read-cache.c: Re-read index if index file
> changed
>
> On 08/25, Joachim Schmitz wrote:
> > "Thomas Gummerer" schrieb im Newsbeitrag
> > news:<134529-6925-3-git-send-email-
> t.gumme...@gmail.com>...
> > > [...]
> > > + usleep(10*1000);
> >
> > usleep() is not available to anybody, e.g. it is not in HP NonStop (not in
> > every case at least)
> >
> > Bye, Jojo
> >
> Thanks for noticing, will be fixed in the re-roll.
Instead of
usleep(10*1000);
You could use
poll(NULL, 0, 10);
instead, similar to what help.c is doing
This may need a fix in compat/win32/poll.c though:
diff --git a/compat/win32/poll.c b/compat/win32/poll.c
--- a/compat/win32/poll.c
+++ b/compat/win32/poll.c
@@ -350,7 +350,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
/* EFAULT is not necessary to implement, but let's do it in the
simplest case. */
- if (!pfd)
+ if (!pfd && nfd)
{
errno = EFAULT;
return -1;
That fix would be needed anyhow...
Bye, Jojo
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
