* Re: [Patch v5 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Eric Sunshine @ 2019-01-03 21:38 UTC (permalink / raw)
To: randall.s.becker; +Cc: Git List, Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-3-randall.s.becker@rogers.com>
On Thu, Jan 3, 2019 at 4:04 PM <randall.s.becker@rogers.com> wrote:
> A number of configuration options are not automatically detected by
> configure mechanisms, including the location of Perl and Python.
>
> There was a problem at a specific set of operating system versions
> that caused getopt to have compile errors. Account for this by
> providing emulation defines for those versions.
>
> Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
> ---
> diff --git a/config.mak.uname b/config.mak.uname
> @@ -470,7 +487,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> NO_MKDTEMP = YesPlease
> OLD_ICONV = UnfortunatelyYes
> - NO_REGEX = YesPlease
> + NO_REGEX=NeedsStartEnd
> NO_PTHREADS = UnfortunatelyYes
Style nit (probably not worth a re-roll): you lost the whitespace
surrounding '='
^ permalink raw reply
* Re: [PATCH v4 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Junio C Hamano @ 2019-01-03 21:36 UTC (permalink / raw)
To: randall.s.becker; +Cc: git, 'Randall S. Becker'
In-Reply-To: <006501d4a39e$b99fe0d0$2cdfa270$@rogers.com>
<randall.s.becker@rogers.com> writes:
> I will reissue the whole package for you. I think I hacked it badly. Will
> get to it after $DAYJOB is done.
Thanks.
^ permalink raw reply
* Re: [PATCH 3/3] t0006-date.sh: add `human` date format tests.
From: Philip Oakley @ 2019-01-03 21:14 UTC (permalink / raw)
To: Junio C Hamano, Stephen P. Smith
Cc: git, Linus Torvalds, Ævar Arnfjörð Bjarmason
In-Reply-To: <xmqqva37j595.fsf@gitster-ct.c.googlers.com>
On 02/01/2019 18:15, Junio C Hamano wrote:
> We perhaps can use "test-tool date timestamp", like so
>
> check_human_date $(test-tool date timestamp "18000 seconds ago") ...
>
> or moving the part that munges 18000 into the above form inside
> check_human_date helper function, e.g.
>
> check_human_date () {
> commit_date=$(test-tool date timestamp "$1 seconds ago")
> commit_date="$commit_date +0200"
> expect=$2
> ...
> }
>
> which would let us write
>
> check_human_date 432000" $THIS_YEAR_REGEX # 5 days ago
Just a quick bikeshed: if used, would this have a year end 5 day
roll-over error potential, or will it always use the single date?
(I appreciate it is just suggestion code, not tested)
--
Philip
^ permalink raw reply
* [Patch v5 4/4] compat/regex/regcomp.c: define intptr_t and uintptr_t on NonStop
From: randall.s.becker @ 2019-01-03 21:03 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-1-randall.s.becker@rogers.com>
From: "Randall S. Becker" <randall.becker@nexbridge.ca>
The system definition header files on HPE NonStop do not define
intptr_t and uintptr_t as do other platforms. These typedefs
are added specifically wrapped in a __TANDEM ifdef.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
compat/regex/regcomp.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/compat/regex/regcomp.c b/compat/regex/regcomp.c
index 51cd60baa..c0d838834 100644
--- a/compat/regex/regcomp.c
+++ b/compat/regex/regcomp.c
@@ -17,6 +17,14 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
+#if defined __TANDEM
+ /* This is currently duplicated from git-compat-utils.h */
+# ifdef NO_INTPTR_T
+ typedef long intptr_t;
+ typedef unsigned long uintptr_t;
+# endif
+#endif
+
static reg_errcode_t re_compile_internal (regex_t *preg, const char * pattern,
size_t length, reg_syntax_t syntax);
static void re_compile_fastmap_iter (regex_t *bufp,
--
2.12.3
^ permalink raw reply related
* [Patch v5 3/4] git-compat-util.h: add FLOSS headers for HPE NonStop
From: randall.s.becker @ 2019-01-03 21:03 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-1-randall.s.becker@rogers.com>
From: "Randall S. Becker" <randall.becker@nexbridge.ca>
The HPE NonStop (a.k.a. __TANDEM) platform cannot build git without
using the FLOSS package supplied by HPE. The convenient location
for including the relevant headers is in this file.
The NSIG define is also not defined on __TANDEM, so we define it
here as 100 if it is not defined only for __TANDEM builds.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
git-compat-util.h | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/git-compat-util.h b/git-compat-util.h
index 09b0102ca..3da6f0673 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -397,6 +397,17 @@ static inline char *git_find_last_dir_sep(const char *path)
#define query_user_email() NULL
#endif
+#ifdef __TANDEM
+#include <floss.h(floss_execl,floss_execlp,floss_execv,floss_execvp)>
+#include <floss.h(floss_getpwuid)>
+#ifndef NSIG
+/* NonStop NSE and NSX do not provide NSIG. SIGGUARDIAN(99) is the highest
+ known, by detective work using kill -l as a list is all signals
+ instead of signal.h where it should be. */
+# define NSIG 100
+#endif
+#endif
+
#if defined(__HP_cc) && (__HP_cc >= 61000)
#define NORETURN __attribute__((noreturn))
#define NORETURN_PTR
--
2.12.3
^ permalink raw reply related
* [Patch v5 2/4] config.mak.uname: support for modern HPE NonStop config.
From: randall.s.becker @ 2019-01-03 21:03 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-1-randall.s.becker@rogers.com>
From: "Randall S. Becker" <randall.becker@nexbridge.ca>
A number of configuration options are not automatically detected by
configure mechanisms, including the location of Perl and Python.
There was a problem at a specific set of operating system versions
that caused getopt to have compile errors. Account for this by
providing emulation defines for those versions.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
config.mak.uname | 29 +++++++++++++++++++++++------
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/config.mak.uname b/config.mak.uname
index 3ee7da0e2..686156d53 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -441,26 +441,43 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
# INLINE='' would just replace one set of warnings with another and
# still not compile in c89 mode, due to non-const array initializations.
CC = cc -c99
+ # Build down-rev compatible objects that don't use our new getopt_long.
+ ifeq ($(uname_R).$(uname_V),J06.21)
+ CC += -WRVU=J06.20
+ endif
+ ifeq ($(uname_R).$(uname_V),L17.02)
+ CC += -WRVU=L16.05
+ endif
# Disable all optimization, seems to result in bad code, with -O or -O2
# or even -O1 (default), /usr/local/libexec/git-core/git-pack-objects
# abends on "git push". Needs more investigation.
- CFLAGS = -g -O0
+ CFLAGS = -g -O0 -Winline
# We'd want it to be here.
prefix = /usr/local
- # Our's are in ${prefix}/bin (perl might also be in /usr/bin/perl).
- PERL_PATH = ${prefix}/bin/perl
- PYTHON_PATH = ${prefix}/bin/python
-
+ # perl and python must be in /usr/bin on NonStop - supplied by HPE
+ # with operating system in that managed directory.
+ PERL_PATH = /usr/bin/perl
+ PYTHON_PATH = /usr/bin/python
+ # The current /usr/coreutils/rm at lowest support level does not work
+ # with the git test structure. Long paths as in
+ # 'trash directory...' cause rm to terminate prematurely without fully
+ # removing the directory at OS releases J06.21 and L17.02.
+ # Default to the older rm until those two releases are deprecated.
+ RM = /bin/rm -f
# As detected by './configure'.
# Missdetected, hence commented out, see below.
#NO_CURL = YesPlease
# Added manually, see above.
+ NEEDS_SSL_WITH_CURL = YesPlease
+ NEEDS_CRYPTO_WITH_SSL = YesPlease
+ HAVE_DEV_TTY = YesPlease
HAVE_LIBCHARSET_H = YesPlease
HAVE_STRINGS_H = YesPlease
NEEDS_LIBICONV = YesPlease
NEEDS_LIBINTL_BEFORE_LIBICONV = YesPlease
NO_SYS_SELECT_H = UnfortunatelyYes
NO_D_TYPE_IN_DIRENT = YesPlease
+ NO_GETTEXT = YesPlease
NO_HSTRERROR = YesPlease
NO_STRCASESTR = YesPlease
NO_MEMMEM = YesPlease
@@ -470,7 +487,7 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
NO_MKDTEMP = YesPlease
# Currently libiconv-1.9.1.
OLD_ICONV = UnfortunatelyYes
- NO_REGEX = YesPlease
+ NO_REGEX=NeedsStartEnd
NO_PTHREADS = UnfortunatelyYes
# Not detected (nor checked for) by './configure'.
--
2.12.3
^ permalink raw reply related
* [Patch v5 1/4] transport-helper: use xread instead of read
From: randall.s.becker @ 2019-01-03 21:03 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker, Randall S . Becker
In-Reply-To: <20190103210351.13920-1-randall.s.becker@rogers.com>
From: "Randall S. Becker" <randall.becker@nexbridge.ca>
This fix was needed on HPE NonStop NSE and NSX where SSIZE_MAX is less than
BUFFERSIZE resulting in EINVAL. The call to read in transport-helper.c
was the only place outside of wrapper.c where it is used instead of xread.
Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
---
transport-helper.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/transport-helper.c b/transport-helper.c
index bf225c698..5afead9f8 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -1225,9 +1225,8 @@ static int udt_do_read(struct unidirectional_transfer *t)
return 0; /* No space for more. */
transfer_debug("%s is readable", t->src_name);
- bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
- if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
- errno != EINTR) {
+ bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
+ if (bytes < 0 && errno != EINTR) {
error_errno(_("read(%s) failed"), t->src_name);
return -1;
} else if (bytes == 0) {
--
2.12.3
^ permalink raw reply related
* [Patch v5 0/4] HPE NonStop Port Commits
From: randall.s.becker @ 2019-01-03 21:03 UTC (permalink / raw)
To: git; +Cc: Randall S. Becker
From: "Randall S. Becker" <randall.becker@nexbridge.ca>
This set of patches is a distilled version of the minimal
set of changes to git that will allow it to run as client
and server on HPE NonStop NSE and NSX systems. NSR systems
are no longer under support so references to them have
been removed. Each patch in this set is independent but
required for correctness.
Randall S. Becker (4):
transport-helper: use xread instead of read
config.mak.uname: support for modern HPE NonStop config.
git-compat-util.h: add FLOSS headers for HPE NonStop
compat/regex/regcomp.c: define intptr_t and uintptr_t on NonStop
compat/regex/regcomp.c | 8 ++++++++
config.mak.uname | 29 +++++++++++++++++++++++------
git-compat-util.h | 11 +++++++++++
transport-helper.c | 5 ++---
4 files changed, 44 insertions(+), 9 deletions(-)
--
2.12.3
^ permalink raw reply
* git add --intent-to-add + git stash "Cannot save the current worktree state"
From: Anthony Sottile @ 2019-01-03 20:46 UTC (permalink / raw)
To: Git Mailing List
Minimal reproduction
```
git init t
git -C t commit --allow-empty -m 'initial commit'
touch t/a
git -C t add --intent-to-add a
git -C t stash
```
```
+ git init t
Initialized empty Git repository in /private/tmp/t/t/.git/
+ git -C t commit --allow-empty -m 'initial commit'
[master (root-commit) 858132e] initial commit
+ touch t/a
+ git -C t add --intent-to-add a
+ git -C t stash
error: Entry 'a' not uptodate. Cannot merge.
Cannot save the current worktree state
```
Anthony
^ permalink raw reply
* Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files
From: Anthony Sottile @ 2019-01-03 20:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Ben Peart, Git Mailing List
In-Reply-To: <xmqqef9th4iy.fsf@gitster-ct.c.googlers.com>
On Thu, Jan 3, 2019 at 12:26 PM Junio C Hamano <gitster@pobox.com> wrote:
> A "fix" to Ben's optimization for this particular case should be
> fairly straight-forward. I think we have a special case in the
> checkout codepath for an initial checkout and disable "carry forward
> the fact that the user wanted all the paths removed", so it would be
> the matter of adding yet another condition (is_cache_unborn(), which
> is used to set topts.initial_checkout) to the large collection of
> conditions in skip_merge_working_tree().
>
I think it might be simpler than that even -- the optimization treats
the following as equivalent when the current checked out revision is
deadbeef (even if the index / worktree differ), when before they were
not:
- git checkout -b newbranch
- git checkout deadbeef -b newbranch
If a revision is specified on the commandline it should be checked out.
Anthony
^ permalink raw reply
* Re: [PATCH v2] test-lib: check Bash version for '-x' without using shell arrays
From: Junio C Hamano @ 2019-01-03 20:29 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Johannes Sixt, Max Kirillov, Carlo Arenas, Jeff King,
Eric Sunshine, git
In-Reply-To: <20190103114317.11523-1-szeder.dev@gmail.com>
SZEDER Gábor <szeder.dev@gmail.com> writes:
> One of our test scripts, 't1510-repo-setup.sh' [1], still can't be
> reliably run with '-x' tracing enabled, unless it's executed with a
> Bash version supporting BASH_XTRACEFD (since v4.1). We have a lengthy
> condition to check the version of the shell running the test script,
> and disable tracing if it's not executed with a suitable Bash version
> [2].
>
> This condition uses non-portable shell array accesses to easily get
> Bash's major and minor version number. This didn't seem to be
> problematic, because the simple commands expanding those array
> accesses are only executed when the test script is actually run with
> Bash. When run with Dash, the only shell I have at hand that doesn't
> support shell arrays, there are no issues, as it apparently skips
> right over the non-executed simple commands without noticing the
> non-supported constructs.
>
> Alas, it has been reported that NetBSD's /bin/sh does complain about
> them:
>
> ./test-lib.sh: 327: Syntax error: Bad substitution
>
> where line 327 contains the first ${BASH_VERSINFO[0]} array access.
>
> To my understanding both shells are right and conform to POSIX,
> because the standard allows both behaviors by stating the following
> under '2.8.1 Consequences of Shell Errors' [3]:
>
> "An expansion error is one that occurs when the shell expansions
> define in wordexp are carried out (for example, "${x!y}", because
> '!' is not a valid operator); an implementation may treat these as
> syntax errors if it is able to detect them during tokenization,
> rather than during expansion."
>
> Avoid this issue with NetBSD's /bin/sh (and potentially with other,
> less common shells) by hiding the shell array syntax behind 'eval'
> that is only executed with Bash.
>
> [1] 5827506928 (t1510-repo-setup: mark as untraceable with '-x',
> 2018-02-24)
> [2] 5fc98e79fc (t: add means to disable '-x' tracing for individual
> test scripts, 2018-02-24)
> [3] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_01
>
> Reported-by: Max Kirillov <max@max630.net>
> Helped-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>
> Changes since v1:
>
> - Hide the shell array syntax behind 'eval'.
> (I'm fine with both versions, take your pick.)
> - Corrected typo in commit message that Eric pointed out.
> - Added a link to the relevant section in POSIX.
Let's treat this as an independent and more urgent fix-up. I think
it is sufficient to apply it to 2.20.x track, even though we could
go back to 2.17.x and above.
And then let's tentatively kick the "stress test" series out of
'pu', and have that series rebuilt on top of 'master' and this
patch.
Thanks.
>
> t/test-lib.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0f1faa24b2..c34831a4de 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -323,12 +323,12 @@ do
> # this test is marked as such, and ignore '-x' if it
> # isn't executed with a suitable Bash version.
> if test -z "$test_untraceable" || {
> - test -n "$BASH_VERSION" && {
> + test -n "$BASH_VERSION" && eval '
> test ${BASH_VERSINFO[0]} -gt 4 || {
> test ${BASH_VERSINFO[0]} -eq 4 &&
> test ${BASH_VERSINFO[1]} -ge 1
> }
> - }
> + '
> }
> then
> trace=t
^ permalink raw reply
* Re: Regression `git checkout $rev -b branch` while in a `--no-checkout` clone does not check out files
From: Junio C Hamano @ 2019-01-03 20:25 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Anthony Sottile, Ben Peart, Git Mailing List
In-Reply-To: <CACsJy8C=O=ZDvD0ReSJOyAsNDEb5Yz-iFvs7oV5zAXaFf-dw5g@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> I plan to revert this commit anyway when the new command "git
> switch-branch" comes. The optimization will be unconditionally in the
> new command without this hack and users are encouraged to use that one
> instead of "git checkout".
I tend to think that the behaviour is perfectly in line with what
Ben wanted to have, which is to make "checkout -b new [HEAD]" not to
touch anything in the index or the working tree at all.
It further is possible to argue that what is strange in the whole
episode is what "clone --no-checkout" does. In such a repository,
if you say "git status", you'd notice that it is reported that all
paths have been deleted.
Now, if you instead do
git clone $src dst
cd dst
git rm file
git checkout -b new
i.e. starting from a clone with normal working tree, manually
removing a path or two, and then create a new branch starting from
that state while carrying all the local changes, you *do* want to
see that 'file' to stay missing. After all, "do not lose the local
changes; carry them forward" is what switching branches is about.
And from that point of view, we could consider that
git clone --no-checkout $src $dst
is equivalent to
git clone $src $dst && git -C $dst rm -r .
Having said all that.
> Meanwhile, let's see if Ben wants to fix this or revert it.
A "fix" to Ben's optimization for this particular case should be
fairly straight-forward. I think we have a special case in the
checkout codepath for an initial checkout and disable "carry forward
the fact that the user wanted all the paths removed", so it would be
the matter of adding yet another condition (is_cache_unborn(), which
is used to set topts.initial_checkout) to the large collection of
conditions in skip_merge_working_tree().
Back when the "optimization" was discussed, all reviewers said that
it would become maintenance nightmare to ensure that the set of
conditions accurately tracks the case where the optimization is
safe. Now they are entitled to say "we told you so".
^ permalink raw reply
* RE: [PATCH v4 2/4] config.mak.uname: support for modern HPE NonStop config.
From: randall.s.becker @ 2019-01-03 19:58 UTC (permalink / raw)
To: 'Junio C Hamano'; +Cc: git, 'Randall S. Becker'
In-Reply-To: <xmqqimz5h6et.fsf@gitster-ct.c.googlers.com>
On January 3, 2019 14:45, Junio C Hamano wrote:
> To: randall.s.becker@rogers.com
> Cc: git@vger.kernel.org; Randall S. Becker <rsbecker@nexbridge.com>
> Subject: Re: [PATCH v4 2/4] config.mak.uname: support for modern HPE
> NonStop config.
>
> randall.s.becker@rogers.com writes:
>
> > @@ -470,8 +489,13 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> > NO_MKDTEMP = YesPlease
> > # Currently libiconv-1.9.1.
> > OLD_ICONV = UnfortunatelyYes
> > - NO_REGEX = YesPlease
> > + NO_REGEX=NeedsStartEnd
> > NO_PTHREADS = UnfortunatelyYes
> >
> > # Not detected (nor checked for) by './configure'.
> > # We don't have SA_RESTART on NonStop, unfortunalety.
>
> The hunk header claims that the preimage has 8 lines while the postimage
> has 13 lines, adding 5 new lines in total. But that is not what we can
see in
> the hunk.
>
> It is unclear to me if the numbers on the hunk header are bogus, or the
patch
> text was truncated, so I cannot use these two patches with confidence.
The
> first hunk had the same issue, and 1/4 too.
>
> I do not see v4 3/4 and v4 4/4, either. It's not like you are the only
person
> who sends patches to the mailing list, and not having the patches as
> responses to a cover letter for proper threading makes it very hard to see
> which patches belong to the same series and if all the necessary patches
in a
> series have become available.
> Is it possible to arrange that to happen?
>
> Thanks.
I will reissue the whole package for you. I think I hacked it badly. Will
get to it after $DAYJOB is done.
^ permalink raw reply
* Re: [PATCH v4 2/4] config.mak.uname: support for modern HPE NonStop config.
From: Junio C Hamano @ 2019-01-03 19:45 UTC (permalink / raw)
To: randall.s.becker; +Cc: git, Randall S. Becker
In-Reply-To: <20181228200243.19728-1-randall.s.becker@rogers.com>
randall.s.becker@rogers.com writes:
> @@ -470,8 +489,13 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> NO_MKDTEMP = YesPlease
> # Currently libiconv-1.9.1.
> OLD_ICONV = UnfortunatelyYes
> - NO_REGEX = YesPlease
> + NO_REGEX=NeedsStartEnd
> NO_PTHREADS = UnfortunatelyYes
>
> # Not detected (nor checked for) by './configure'.
> # We don't have SA_RESTART on NonStop, unfortunalety.
The hunk header claims that the preimage has 8 lines while the
postimage has 13 lines, adding 5 new lines in total. But that is
not what we can see in the hunk.
It is unclear to me if the numbers on the hunk header are bogus, or
the patch text was truncated, so I cannot use these two patches with
confidence. The first hunk had the same issue, and 1/4 too.
I do not see v4 3/4 and v4 4/4, either. It's not like you are the
only person who sends patches to the mailing list, and not having
the patches as responses to a cover letter for proper threading
makes it very hard to see which patches belong to the same series
and if all the necessary patches in a series have become available.
Is it possible to arrange that to happen?
Thanks.
^ permalink raw reply
* Re: [PATCH v4 1/4] transport-helper: use xread instead of read
From: Junio C Hamano @ 2019-01-03 19:31 UTC (permalink / raw)
To: Jeff King; +Cc: randall.s.becker, git, Randall S. Becker
In-Reply-To: <20190103071632.GB24149@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 02, 2019 at 12:55:51PM -0800, Junio C Hamano wrote:
>
>> > Signed-off-by: Randall S. Becker <rsbecker@nexbridge.com>
>> > ---
>> > transport-helper.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/transport-helper.c b/transport-helper.c
>> > index bf225c698f..a290695a12 100644
>> > --- a/transport-helper.c
>> > +++ b/transport-helper.c
>> > @@ -1225,7 +1225,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
>> > return 0; /* No space for more. */
>> >
>> > transfer_debug("%s is readable", t->src_name);
>> > - bytes = read(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
>> > + bytes = xread(t->src, t->buf + t->bufuse, BUFFERSIZE - t->bufuse);
>> > - if (bytes < 0 && errno != EWOULDBLOCK && errno != EAGAIN &&
>> > - errno != EINTR) {
>> > + if (bytes < 0 && errno != EINTR) {
>> > error_errno(_("read(%s) failed"), t->src_name);
>>
>> Can't we also lose EINTR check, though? When read() returns
>> negative, we check errno and if it is EINTR, continue the loop.
>
> Yes.
>
> I also wondered if this caller might actually be relying on the current
> non-looping behavior, but it looks like I already traced through and
> determined this was OK:
>
> https://public-inbox.org/git/20180111063110.GB31213@sigill.intra.peff.net/
>
> (the cleanup is correct either way, but that is what makes the
> conversion to xread() OK).
>
> We may want to just take the xread() conversion and then do the patch
> that I linked above on top, since it also cleans up the xwrite() spot,
> too.
OK. That does sound cleaner.
^ permalink raw reply
* Re: [PATCH v2 1/2] Change how HTTP response body is returned
From: Junio C Hamano @ 2019-01-03 19:09 UTC (permalink / raw)
To: Masaya Suzuki; +Cc: git, jrnieder, peff, sunshine
In-Reply-To: <20181229194447.157763-2-masayasuzuki@google.com>
Masaya Suzuki <masayasuzuki@google.com> writes:
> Subject: Re: [PATCH v2 1/2] Change how HTTP response body is returned
Perhaps:
Subject: http: change the way response body is returned
but if we can state why we want to do so concisely, that would be
even better.
> This changes the way HTTP response body is returned in
> http_request_reauth and post_rpc.
>
> 1. http_request_reauth makes up to two requests; one without a
> credential and one with a credential. The first request can fail if
> it needs a credential. When the keep_error option is specified, the
> response to the first request can be written to the HTTP response
> body destination. If the response body destination is a string
> buffer, it erases the buffer before making the second request. By
> introducing http_response_dest, it can handle the case that the
> destination is a file handle.
> 2. post_rpc makes an HTTP request and the response body is directly
> written to a file descriptor. This makes it check the HTTP status
> code before writing it, and do not write the response body if it's an
> error response. It's ok without this check now because post_rpc makes
> a request with CURLOPT_FAILONERROR, and libcurl won't call the
> callback if the response has an error status code.
The above may be an accurate description of what the code will do
with this patch, but I cannot quite read the reason why we would
want the code to behave so in the first place.
>
> Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>
> ---
> http.c | 99 +++++++++++++++++++++++++++++----------------------
> remote-curl.c | 29 ++++++++++++---
> 2 files changed, 81 insertions(+), 47 deletions(-)
>
> diff --git a/http.c b/http.c
> index eacc2a75e..d23417670 100644
> --- a/http.c
> +++ b/http.c
> @@ -165,6 +165,19 @@ static int http_schannel_check_revoke = 1;
> */
> static int http_schannel_use_ssl_cainfo;
>
> +/*
> + * Where to store the result of http_request.
> + *
> + * At most one of buffer or file can be non-NULL. The buffer and file are not
> + * allocated by http_request, and the caller is responsible for releasing them.
> + */
> +struct http_response_dest {
> + struct strbuf *buffer;
> +
> + FILE *file;
> + const char *filename;
> +};
> +
> size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> {
> size_t size = eltsize * nmemb;
> @@ -1794,12 +1807,8 @@ static void http_opt_request_remainder(CURL *curl, off_t pos)
> curl_easy_setopt(curl, CURLOPT_RANGE, buf);
> }
>
> -/* http_request() targets */
> -#define HTTP_REQUEST_STRBUF 0
> -#define HTTP_REQUEST_FILE 1
> -
> static int http_request(const char *url,
> - void *result, int target,
> + struct http_response_dest *dest,
> const struct http_get_options *options)
> {
> struct active_request_slot *slot;
> @@ -1812,21 +1821,23 @@ static int http_request(const char *url,
> slot = get_active_slot();
> curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
>
> - if (result == NULL) {
> - curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 1);
> - } else {
> + if (dest->file) {
> + off_t posn = ftello(dest->file);
> curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
> - curl_easy_setopt(slot->curl, CURLOPT_FILE, result);
OK, so it used to be that we can either discard the result
(i.e. NOBODY) or send the result to CURLOPT_FILE, and the latter has
two options (sent to a file, or sent to in-core buffer). The way
these three choices are given were with the result pointer and the
target enum.
That is replaced by a struct that allows the same three choices.
Makes sense so far.
> @@ -1930,10 +1941,10 @@ static int update_url_from_redirect(struct strbuf *base,
> }
>
> static int http_request_reauth(const char *url,
> - void *result, int target,
> + struct http_response_dest *dest,
> struct http_get_options *options)
> {
> - int ret = http_request(url, result, target, options);
> + int ret = http_request(url, dest, options);
Just adjusting the calling convention to the change we saw above.
> @@ -1949,32 +1960,34 @@ static int http_request_reauth(const char *url,
> if (ret != HTTP_REAUTH)
> return ret;
>
> - /*
> - * If we are using KEEP_ERROR, the previous request may have
> - * put cruft into our output stream; we should clear it out before
> - * making our next request. We only know how to do this for
> - * the strbuf case, but that is enough to satisfy current callers.
> - */
> - if (options && options->keep_error) {
> - switch (target) {
> - case HTTP_REQUEST_STRBUF:
> - strbuf_reset(result);
> - break;
> - default:
> - BUG("HTTP_KEEP_ERROR is only supported with strbufs");
Now it gets interesting. We used to allow keep-error only when
receiving to in-core buffer.
> + if (dest->file) {
> + /*
> + * At this point, the file contains the response body of the
> + * previous request. We need to truncate the file.
> + */
> + FILE *new_file = freopen(dest->filename, "w", dest->file);
Now freopen() lets us restart the file anew with a new "FILE *".
> + if (new_file == NULL) {
> + error("Unable to open local file %s", dest->filename);
error_errno(), perhaps?
At this point, I presume that dest->file is closed by the failed
freopen(), but dest->file is still non-NULL and causes further calls
to http_request() with this dest would be a disaster? As long as
the caller of this function reacts to HTTP_ERROR and kill the dest,
it would be fine.
> + return HTTP_ERROR;
> }
> + dest->file = new_file;
> + } else if (dest->buffer) {
> + strbuf_reset(dest->buffer);
> }
OK.
> credential_fill(&http_auth);
>
> - return http_request(url, result, target, options);
> + return http_request(url, dest, options);
> }
So far, I spotted one reason why this patch wants to exist: it used
to be that keep_error was impossible when sending to a file. It
somehow wants to allow us to do so (even though it still is unclear
who that new caller that wants to use keep_error is, and for what
purpose it wants to use that facility).
Perhaps this step can be split into at least two steps? The first
one is to turn <result, target> to <dest> without changing any other
behaviour, and then the second one implements keep_error handling
for the "dest->file != NULL" case.
There may be other things this patch does, in which case it may
deserve to become three or more steps, but we haven't seen enough to
decide if that is the case. Let's read on.
> int http_get_strbuf(const char *url,
> - struct strbuf *result,
> + struct strbuf *dest_buffer,
> struct http_get_options *options)
> {
> - return http_request_reauth(url, result, HTTP_REQUEST_STRBUF, options);
> + struct http_response_dest dest;
> + dest.file = NULL;
> + dest.buffer = dest_buffer;
> + return http_request_reauth(url, &dest, options);
This is merely adjusting for the updated calling convention, which
makes sense.
> }
>
> /*
> @@ -1988,18 +2001,20 @@ static int http_get_file(const char *url, const char *filename,
> {
> int ret;
> struct strbuf tmpfile = STRBUF_INIT;
> - FILE *result;
> + struct http_response_dest dest;
>
> strbuf_addf(&tmpfile, "%s.temp", filename);
> - result = fopen(tmpfile.buf, "a");
> - if (!result) {
> + dest.buffer = NULL;
> + dest.file = fopen(tmpfile.buf, "a");
> + if (!dest.file) {
> error("Unable to open local file %s", tmpfile.buf);
> ret = HTTP_ERROR;
> goto cleanup;
> }
> + dest.filename = tmpfile.buf;
Hmph. I somehow expected that the presence of dest.filename field
would allow callers to just set it and let the fopen() call handled
in http_request(), e.g. at the beginning of the function, it would
do something like
if (!dest->file && dest->filename)
dest->file = fopen(...);
But obviously that is not within the scope of this change. Leaving
the caller responsible for opening and reporting errors as before
like the above is preferrable.
> diff --git a/remote-curl.c b/remote-curl.c
> index 1220dffcd..48656bf18 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -546,14 +546,31 @@ static curlioerr rpc_ioctl(CURL *handle, int cmd, void *clientp)
> }
> #endif
>
> +struct rpc_in_data {
> + struct rpc_state *rpc;
> + struct active_request_slot *slot;
> +};
> +
> +/*
> + * A callback for CURLOPT_WRITEFUNCTION. The return value is the bytes consumed
> + * from ptr.
> + */
> static size_t rpc_in(char *ptr, size_t eltsize,
> size_t nmemb, void *buffer_)
> {
> size_t size = eltsize * nmemb;
> - struct rpc_state *rpc = buffer_;
> + struct rpc_in_data *data = buffer_;
> + long response_code;
> +
> + if (curl_easy_getinfo(data->slot->curl, CURLINFO_RESPONSE_CODE,
> + &response_code) != CURLE_OK)
> + return size;
> + if (response_code != 200)
> + return size;
> +
> if (size)
> - rpc->any_written = 1;
> - write_or_die(rpc->in, ptr, size);
> + data->rpc->any_written = 1;
> + write_or_die(data->rpc->in, ptr, size);
> return size;
> }
This is not necessarily related to the change we saw in http.c but
making it more careful in general? That is, we avoid writing the
payload to the destination (by the way, rpc->IN being the target of
write(2) is somewhat a brain-twister). It may deserve to become its
own step in a patch series, with separate justification (i.e. "when
rpc channel receives an error from the cURL library, we used to
write the data to the file anyway, and that caused such and such
problems, as demonstrated by the new test added by this patch. we
fix it by checking for the error before writing to the file").
> @@ -633,6 +650,7 @@ static int post_rpc(struct rpc_state *rpc)
> size_t gzip_size = 0;
> int err, large_request = 0;
> int needs_100_continue = 0;
> + struct rpc_in_data rpc_in_data;
>
> /* Try to load the entire request, if we can fit it into the
> * allocated buffer space we can use HTTP/1.0 and avoid the
> @@ -765,8 +783,9 @@ static int post_rpc(struct rpc_state *rpc)
>
> curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
> curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
> - curl_easy_setopt(slot->curl, CURLOPT_FILE, rpc);
> -
> + rpc_in_data.rpc = rpc;
> + rpc_in_data.slot = slot;
> + curl_easy_setopt(slot->curl, CURLOPT_FILE, &rpc_in_data);
>
> rpc->any_written = 0;
> err = run_slot(slot, NULL);
These two hunks look like mere adjustments for the new callback
type, which makes sense.
Thanks.
^ permalink raw reply
* Re: [PATCH] config.mak.dev: add -Wformat
From: Jonathan Nieder @ 2019-01-03 18:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Gummerer, Jeff King, git, Josh Steadmon, Masaya Suzuki
In-Reply-To: <xmqqa7khisue.fsf@gitster-ct.c.googlers.com>
Hi,
Junio C Hamano wrote:
> I think it is a good idea to give fallback/redundancy for this
> case. I do not have strong opinion between -Wall and -Wformat,
> but I'd probably vote for the former if pressed.
>
> -- >8 --
> From: Thomas Gummerer <t.gummerer@gmail.com>
> Date: Fri, 12 Oct 2018 19:40:37 +0100
> Subject: [PATCH] config.mak.dev: add -Wformat
>
> 801fa63a90 ("config.mak.dev: add -Wformat-security", 2018-09-08)
> added the "-Wformat-security" to the flags set in config.mak.dev.
> In the gcc man page this is documented as:
>
> If -Wformat is specified, also warn about uses of format
> functions that represent possible security problems. [...]
>
> The commit did however not add the "-Wformat" flag, but instead
> relied on the fact that "-Wall" is set in the Makefile by default
> and that "-Wformat" is part of "-Wall".
>
> Unfortunately, those who use config.mak.autogen generated with the
> autoconf to configure toolchain do *not* get "-Wall" in their CFLAGS
> and the added -Wformat-security had no effect. Worse yet, newer
> versions of gcc (gcc 8.2.1 in this particular case) warn about the
> lack of "-Wformat" and thus compilation fails only with this option
> set.
>
> We could fix it by adding "-Wformat", but in general we do want all
> checks included in "-Wall", so let's add it to config.mak.dev to
> cover more cases.
>
> Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
> Helped-by: Jeff King <peff@peff.net>
> Helped-by: Jonathan Nieder <jrnieder@gmail.com>
> [jc: s/-Wformat/-Wall/]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> config.mak.dev | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for tying up this loose end.
^ permalink raw reply
* Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.
From: Junio C Hamano @ 2019-01-03 18:09 UTC (permalink / raw)
To: Jeff King; +Cc: Erin Dahlgren, git, Johannes Schindelin
In-Reply-To: <20190103051432.GE20047@sigill.intra.peff.net>
>Subject: Re: [PATCH v3] Simplify handling of setup_git_directory_gently() failure cases.
Perhaps
Subject: setup: simplify setup_git_directory_gently() failure cases
to clarify which part of the entire world this patch is touching.
Jeff King <peff@peff.net> writes:
> This patch isn't _too_ big, so I don't think it's worth the effort at
> this point for this particular case, but just something to think about
> for the future. A series around this topic would probably be something
> like:
>
> 1: drop the useless chdir and fold setup_nongit() into the main
> function
>
> 2: stop doing the early return from HIT_MOUNT_POINT, and treat it just
> like HIT_CEILING (and drop the useless strbuf release)
>
> 3: treat GIT_DIR_NONE as a BUG
>
> 4: rearrange the nongit logic at the end of the function for clarity
Yeah, that organization does make sense. And I agree with your rule
of thumb to use the length and complexity of the log message to
judge if a single step is getting too big.
> We usually avoid "//" comments, even for single lines (though that is
> perhaps superstition at this point, as we've started to embrace several
> other C99-isms). IMHO this particular comment is not even really
> necessary, as the whole conditional is suitably short and obvious after
> your refactor.
FWIW "git grep" for // seems to show that we reserve use of //
inside commented out code samples.
I also agree the comments behind // in this patch are probably
unneeded.
>
>> @@ -1132,7 +1142,10 @@ const char *setup_git_directory_gently(int *nongit_ok)
>> * the user has set GIT_DIR. It may be beneficial to disallow bogus
>> * GIT_DIR values at some point in the future.
>> */
>> - if (startup_info->have_repository || getenv(GIT_DIR_ENVIRONMENT)) {
>> + if (// GIT_DIR_EXPLICIT, GIT_DIR_DISCOVERED, GIT_DIR_BARE
>> + startup_info->have_repository ||
>> + // GIT_DIR_EXPLICIT
>> + getenv(GIT_DIR_ENVIRONMENT)) {
>
> Same "//" style issue as above. I'm not sure how much value there is in
> repeating the GIT_DIR_* cases here, as they're likely to grow out of
> sync with the switch() statement above.
It is unclear to me if the original code is doing the right thing
under one condition, and this patch does not seem to change the
behaviour.
What happens if GIT_DIR environment is set to an invalid path and
nongit_ok is non-NULL? setup_explicit_git_dir() would have assigned
1 to *nongit_ok, so have_repository is false at this point.
We enter the if() statement in such a case, and end up calling
setup_git_env(gitdir) using the bogus path that is not pointing at a
repository. We leave have_repository to be false but the paths
recorded in the_repository by setup_git_env() would all point at
bogus places.
> At first I thought this could all be folded into the "else" clause of
> the conditional above (which would make the logic much easier to
> follow), but that wouldn't cover the case of GIT_DIR=/bogus, which is
> what that getenv() is trying to handle here.
Yes, but should GIT_DIR=/bogus even be touching the_repository?
It is a separate clean-up and does not affect the validity of this
simplification patchd, so I agreee with ...
> So I think this is the best we can do for now.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/4] t3502: validate '-m 1' argument is now accepted for non-merge commits
From: SZEDER Gábor @ 2019-01-03 17:22 UTC (permalink / raw)
To: Sergey Organov; +Cc: git, gitster
In-Reply-To: <ccfe8ae38301b6ee1b0924fbf00eb5d20242ea5d.1544764226.git.sorganov@gmail.com>
On Fri, Dec 14, 2018 at 07:53:51AM +0300, Sergey Organov wrote:
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
> t/t3502-cherry-pick-merge.sh | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
> index b160271..3259bd5 100755
> --- a/t/t3502-cherry-pick-merge.sh
> +++ b/t/t3502-cherry-pick-merge.sh
> @@ -40,12 +40,12 @@ test_expect_success 'cherry-pick -m complains of bogus numbers' '
> test_expect_code 129 git cherry-pick -m 0 b
> '
>
> -test_expect_success 'cherry-pick a non-merge with -m should fail' '
> +test_expect_success 'cherry-pick explicit first parent of a non-merge' '
>
> git reset --hard &&
> git checkout a^0 &&
> - test_expect_code 128 git cherry-pick -m 1 b &&
> - git diff --exit-code a --
> + git cherry-pick -m 1 b &&
> + git diff --exit-code c --
>
> '
>
> @@ -84,12 +84,12 @@ test_expect_success 'cherry pick a merge relative to nonexistent parent should f
>
> '
>
> -test_expect_success 'revert a non-merge with -m should fail' '
> +test_expect_success 'revert explicit first parent of a non-merge' '
>
> git reset --hard &&
> git checkout c^0 &&
> - test_must_fail git revert -m 1 b &&
> - git diff --exit-code c
> + git revert -m 1 b &&
> + git diff --exit-code a
You need disambiguaion here, otherwise this test fails on
case-insensitive file systems:
++git diff --exit-code a
fatal: ambiguous argument 'a': both revision and filename
Use '--' to separate paths from revisions, like this:
'git <command> [<revision>...] -- [<file>...]'
error: last command exited with $?=128
not ok 8 - revert explicit first parent of a non-merge
>
> '
>
> --
> 2.10.0.1.g57b01a3
>
^ permalink raw reply
* Re: [PATCH] config.mak.dev: add -Wformat
From: Junio C Hamano @ 2019-01-03 16:55 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Thomas Gummerer, Jeff King, git, Josh Steadmon, Masaya Suzuki
In-Reply-To: <20181227185900.GE146609@google.com>
Jonathan Nieder <jrnieder@gmail.com> writes:
> In October, Thomas Gummerer wrote:
>> On 10/12, Jonathan Nieder wrote:
>>> Jeff King wrote:
>>> ...
>>>> -Wformat is part of -Wall, which we already turn on by default (even for
>>>> non-developer builds).
> ...
> As discussed in [1], autoconf appears to not put -Wall in CFLAGS:
>
> $ make configure
> GEN configure
> $ ./configure
> [...]
> config.status: creating config.mak.autogen
> config.status: executing config.mak.autogen commands
> $ grep CFLAGS config.mak.autogen
> CFLAGS = -g -O2
> PTHREAD_CFLAGS=-pthread
>
> So this trap for the unwary is still around.
>
> Can we revive this patch? Does it just need a clearer commit message,
> or were there other objections?
I think it is a good idea to give fallback/redundancy for this
case. I do not have strong opinion between -Wall and -Wformat,
but I'd probably vote for the former if pressed.
-- >8 --
From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Fri, 12 Oct 2018 19:40:37 +0100
Subject: [PATCH] config.mak.dev: add -Wformat
801fa63a90 ("config.mak.dev: add -Wformat-security", 2018-09-08)
added the "-Wformat-security" to the flags set in config.mak.dev.
In the gcc man page this is documented as:
If -Wformat is specified, also warn about uses of format
functions that represent possible security problems. [...]
The commit did however not add the "-Wformat" flag, but instead
relied on the fact that "-Wall" is set in the Makefile by default
and that "-Wformat" is part of "-Wall".
Unfortunately, those who use config.mak.autogen generated with the
autoconf to configure toolchain do *not* get "-Wall" in their CFLAGS
and the added -Wformat-security had no effect. Worse yet, newer
versions of gcc (gcc 8.2.1 in this particular case) warn about the
lack of "-Wformat" and thus compilation fails only with this option
set.
We could fix it by adding "-Wformat", but in general we do want all
checks included in "-Wall", so let's add it to config.mak.dev to
cover more cases.
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Jonathan Nieder <jrnieder@gmail.com>
[jc: s/-Wformat/-Wall/]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
config.mak.dev | 1 +
1 file changed, 1 insertion(+)
diff --git a/config.mak.dev b/config.mak.dev
index bfbd3df4e8..74337f1f92 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -1,6 +1,7 @@
ifeq ($(filter no-error,$(DEVOPTS)),)
CFLAGS += -Werror
endif
+CFLAGS += -Wall
CFLAGS += -Wdeclaration-after-statement
CFLAGS += -Wformat-security
CFLAGS += -Wno-format-zero-length
--
2.20.1-2-gb21ebb671b
^ permalink raw reply related
* Re: [PATCH 5/5] travis-ci: build with the right compiler
From: Johannes Schindelin @ 2019-01-03 16:01 UTC (permalink / raw)
To: SZEDER Gábor; +Cc: Junio C Hamano, Jonathan Nieder, git
In-Reply-To: <20181220162452.17732-6-szeder.dev@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4158 bytes --]
Hi Gábor,
On Thu, 20 Dec 2018, SZEDER Gábor wrote:
> Our 'Makefile' hardcodes the compiler to build Git as 'CC = cc'. This
... This CC variable ...
> can be overridden from the command line, i.e. 'make CC=gcc-X.Y' will
> build with that particular GCC version, but not from the environment,
> i.e. 'CC=gcc-X.Y make' will still build with whatever 'cc' happens to
> be on the platform.
Without this edit, it read to me as if the commit message claimed that
CC cannot be overridden via the environment *at all*, even with MAKEFLAGS.
The rest of the entire patch series looks good to me, I did not dig as
deeply as Ævar about that obstack patch, but if there is *some* sort of
upstream from where we can get a fix, I think we should try to go for that
(rather than risking to diverge even further).
Thanks,
Dscho
>
> Our build jobs on Travis CI are badly affected by this. In the build
> matrix we have dedicated build jobs to build Git with GCC and Clang
> both on Linux and macOS from the very beginning (522354d70f (Add
> Travis CI support, 2015-11-27)). Alas, this never really worked as
> supposed to, because Travis CI specifies the compiler for those build
> jobs as 'export CC=gcc' and 'export CC=clang' (which works fine for
> projects built with './configure && make'). Consequently, our
> 'linux-clang' build job has always used GCC, because that's where 'cc'
> points at in Travis CI's Linux images, while the 'osx-gcc' build job
> has always used Clang. Furthermore, 37fa4b3c78 (travis-ci: run gcc-8
> on linux-gcc jobs, 2018-05-19) added an 'export CC=gcc-8' in an
> attempt to build with a more modern compiler, but to no avail.
>
> Set MAKEFLAGS with CC based on the $CC environment variable, so 'make'
> will run the "right" compiler. The Xcode 10.1 macOS image on Travis
> CI already contains the gcc@8 package from Homebrew, but we have to
> 'brew link' it first to be able to use it.
>
> So with this patch our build jobs will build Git with the following
> compiler versions:
>
> linux-clang: clang version 5.0.0 (tags/RELEASE_500/final)
> linux-gcc: gcc-8 (Ubuntu 8.1.0-5ubuntu1~14.04) 8.1.0
>
> osx-clang: Apple LLVM version 10.0.0 (clang-1000.11.45.5)
> osx-gcc: gcc-8 (Homebrew GCC 8.2.0) 8.2.0
>
> GETTEXT_POISON: gcc (Ubuntu 4.8.4-2ubuntu1~14.04.3) 4.8.4
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
> ci/install-dependencies.sh | 5 +++++
> ci/lib-travisci.sh | 15 ++++++++++++---
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/ci/install-dependencies.sh b/ci/install-dependencies.sh
> index 06c3546e1e..dc719876bb 100755
> --- a/ci/install-dependencies.sh
> +++ b/ci/install-dependencies.sh
> @@ -40,6 +40,11 @@ osx-clang|osx-gcc)
> brew install git-lfs gettext
> brew link --force gettext
> brew install caskroom/cask/perforce
> + case "$jobname" in
> + osx-gcc)
> + brew link gcc@8
> + ;;
> + esac
> ;;
> StaticAnalysis)
> sudo apt-get -q update
> diff --git a/ci/lib-travisci.sh b/ci/lib-travisci.sh
> index 69dff4d1ec..a479613a57 100755
> --- a/ci/lib-travisci.sh
> +++ b/ci/lib-travisci.sh
> @@ -99,12 +99,14 @@ export DEFAULT_TEST_TARGET=prove
> export GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
> export GIT_TEST_OPTS="--verbose-log -x --immediate"
> export GIT_TEST_CLONE_2GB=YesPlease
> -if [ "$jobname" = linux-gcc ]; then
> - export CC=gcc-8
> -fi
>
> case "$jobname" in
> linux-clang|linux-gcc)
> + if [ "$jobname" = linux-gcc ]
> + then
> + export CC=gcc-8
> + fi
> +
> export GIT_TEST_HTTPD=YesPlease
>
> # The Linux build installs the defined dependency versions below.
> @@ -118,6 +120,11 @@ linux-clang|linux-gcc)
> export PATH="$GIT_LFS_PATH:$P4_PATH:$PATH"
> ;;
> osx-clang|osx-gcc)
> + if [ "$jobname" = osx-gcc ]
> + then
> + export CC=gcc-8
> + fi
> +
> # t9810 occasionally fails on Travis CI OS X
> # t9816 occasionally fails with "TAP out of sequence errors" on
> # Travis CI OS X
> @@ -127,3 +134,5 @@ GIT_TEST_GETTEXT_POISON)
> export GIT_TEST_GETTEXT_POISON=YesPlease
> ;;
> esac
> +
> +export MAKEFLAGS="CC=${CC:-cc}"
> --
> 2.20.1.151.gec613c4b75
>
>
^ permalink raw reply
* RE: Regression in git-subtree.sh, introduced in 2.20.1, after 315a84f9aa0e2e629b0680068646b0032518ebed
From: Strain, Roger L. @ 2019-01-03 15:30 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Marc Balmer, Duy Nguyen, Git Mailing List, Junio C Hamano
In-Reply-To: <nycvar.QRO.7.76.6.1901031448260.45@tvgsbejvaqbjf.bet>
> -----Original Message-----
> From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> Hi Roger,
>
>
> On Wed, 2 Jan 2019, Strain, Roger L. wrote:
>
> > TL;DR: Current script uses git rev-list to retrieve all commits which
> > are reachable from HEAD but not from <abc123>. Is there a syntax that
> > will instead return all commits reachable from HEAD, but stop
> traversing
> > when <abc123> is encountered? It's a subtle distinction, but
> important.
>
> Maybe you are looking for the --ancestry-path option? Essentially, `git
> rev-list --ancestry-path A..B` will list only commits that are reachable
> from B, not reachable from A, but that *can* reach A (i.e. that are
> descendants of A).
>
> Ciao,
> Johannes
Thanks for the suggestion, but I don't think that one does quite what is needed here. It did provide a good sample graph to consider, though. Subtree needs to rebuild history and tie things in to previously reconstructed commits. Here's the sample graph from the --ancestry-path portion of the git-rev-list manpage:
D---E-------F
/ \ \
B---C---G---H---I---J
/ \
A-------K---------------L--M
Subtree maps mainline commits to known subtree commits, so let's assume we have a mapping of D to D'. As documented, if we were to rev-list D..M normally, we'd get all commits except D itself, and D's ancestors B and A. So the "normal" result would be:
E-------F
\ \
C---G---H---I---J
\
K---------------L--M
This is bad for subtree, because commit C's parent is B, which is not a known commit to subtree, and which wasn't included in the list of commits to convert. It therefore assumes C is an initial commit, which is wrong. Likewise K's parent A isn't in the list to convert, so K is assumed to be an initial commit, which also is wrong. (E is okay here, because E's parent is D, and D maps to D', so we can stitch that history together properly.)
By using --ancestry-path, we would instead get only the things directly between D and M, as documented:
E-------F
\ \
G---H---I---J
\
L--M
This actually moves us in the wrong direction, as now both G and L have one known parent and one unknown parent; I'm not sure how the script would handle this, but we actually end up with less information.
In this case, what I need is a way to trace back history along all merge parents, stopping only when I hit one of multiple known commits that I can directly tie back to. In this instance, subtree *knows* what D maps to, so any time D is encountered, we can stop tracing back. But if I can get to one of D's ancestors through another path, I need to keep following that path. Here's what I need for this to work properly:
E-------F
\ \
B---C---G---H---I---J
/ \
A-------K---------------L--M
To give one more example (since removing a single commit frankly isn't very interesting) let's say that I have known subtree mappings for both D = D' and G = G'. I would therefore need to find all commits which are ancestors of M, but stop tracing history when I reach *either* D or G. Note that if I can reach a commit from any other path, I still need to know about it. Here's what we ultimately would want to find:
E-------F
\
H---I---J
\
A-------K---------------L--M
In this case, commit E will reference known commit D as a parent and maps to D', and is good. Commit H references known commit G as a parent and maps to G', and is good. Commit K references A, which itself is an initial commit so is converted to A' (just as it has been previous times subtree has run), and is good.
I'll keep digging around a little bit, but I'm starting to think the necessary plumbing for this operation might not exist. If I can't find it, I'll see if there's some way to unroll that recursive call.
--
Roger
^ permalink raw reply
* RE: Regression in git-subtree.sh, introduced in 2.20.1, after 315a84f9aa0e2e629b0680068646b0032518ebed
From: Johannes Schindelin @ 2019-01-03 13:50 UTC (permalink / raw)
To: Strain, Roger L.
Cc: Marc Balmer, Duy Nguyen, Git Mailing List, Junio C Hamano
In-Reply-To: <59718f73a0a14b828a6d4fd4c8c222d1@MBX260.adm.swri.edu>
Hi Roger,
On Wed, 2 Jan 2019, Strain, Roger L. wrote:
> TL;DR: Current script uses git rev-list to retrieve all commits which
> are reachable from HEAD but not from <abc123>. Is there a syntax that
> will instead return all commits reachable from HEAD, but stop traversing
> when <abc123> is encountered? It's a subtle distinction, but important.
Maybe you are looking for the --ancestry-path option? Essentially, `git
rev-list --ancestry-path A..B` will list only commits that are reachable
from B, not reachable from A, but that *can* reach A (i.e. that are
descendants of A).
Ciao,
Johannes
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2018, #02; Fri, 28)
From: Johannes Schindelin @ 2019-01-03 13:27 UTC (permalink / raw)
To: Elijah Newren; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CABPp-BEd5-0Vcv8YApUxo0jK_ofxCORSG5H0wU=kiR2aOY1ztQ@mail.gmail.com>
Hi Elijah,
On Fri, 28 Dec 2018, Elijah Newren wrote:
> On Fri, Dec 28, 2018 at 10:04 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> > * en/rebase-merge-on-sequencer (2018-11-08) 2 commits
> > - rebase: implement --merge via git-rebase--interactive
> > - git-rebase, sequencer: extend --quiet option for the interactive machinery
> >
> > "git rebase --merge" as been reimplemented by reusing the internal
> > machinery used for "git rebase -i".
> >
> > Expecting a reroll.
> > cf. <CABPp-BF8RupyfP69iqAVTXxEhBGyzVd-wUgp3y0pf+CbBFAQeg@mail.gmail.com>
>
> Quick update: Two re-rolls have been sent in[1]; v3 on November 22 and
> v4 with only a minor error message tweak on Dec 11. I think I've
> addressed all review comments from v2, but neither v3 nor v4 has
> received much review -- Dscho was also heavily busy during the run up
> to 2.20 and needed some recovery time afterward.
Yep. There have been quite a few problems in the -rc period, and at least
one frantic bug fix of mine introduced another regression, and then there
was the problem with cURL where it would try to use HTTP/2 with NTLM
(which does not work, and probably never will) and as you probably
suspect, NTLM/Kerberos authentication is *quite* common on Windows, so
that would have been a total non-starter if we had shipped Git for Windows
v2.20.0 with an unfixed cURL.
So yes, I was quite exhausted after those weeks.
> I was going to re-ping in early January. Anyway, it may be worth at
> least updating your note to "reroll exists".
It is early January! ;-)
Ciao,
Dscho
^ permalink raw reply
* ps/stash-in-c, was Re: What's cooking in git.git (Dec 2018, #02; Fri, 28)
From: Johannes Schindelin @ 2019-01-03 13:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh8explya.fsf@gitster-ct.c.googlers.com>
Hi Junio,
On Fri, 28 Dec 2018, Junio C Hamano wrote:
> * ps/stash-in-c (2018-11-26) 22 commits
> . stash: replace all `write-tree` child processes with API calls
> . stash: optimize `get_untracked_files()` and `check_changes()`
> . stash: convert `stash--helper.c` into `stash.c`
> . stash: convert save to builtin
> . stash: make push -q quiet
> . stash: convert push to builtin
> . stash: convert create to builtin
> . stash: convert store to builtin
> . stash: convert show to builtin
> . stash: convert list to builtin
> . stash: convert pop to builtin
> . stash: convert branch to builtin
> . stash: convert drop and clear to builtin
> . stash: convert apply to builtin
> . stash: mention options in `show` synopsis
> . stash: add tests for `git stash show` config
> . stash: rename test cases to be more descriptive
> . t3903: modernize style
> . stash: improve option parsing test coverage
> . strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
> . strbuf.c: add `strbuf_join_argv()`
> . sha1-name.c: add `get_oidf()` which acts like `get_oid()`
>
> "git stash" rewritten in C.
>
> Expecting a reroll, probably on top of the sd/stash-wo-user-name
> topic after it stabilizes, with an escape hatch like the one in
> "rebase in C".
There you go:
https://public-inbox.org/git/cover.1545331726.git.ungureanupaulsebastian@gmail.com/
Happy new year!
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox