* [PATCH v12 05/26] stash: improve option parsing test coverage
From: Paul-Sebastian Ungureanu @ 2018-12-20 19:44 UTC (permalink / raw)
To: git; +Cc: t.gummerer, Johannes.Schindelin
In-Reply-To: <cover.1545331726.git.ungureanupaulsebastian@gmail.com>
From: Joel Teichroeb <joel@teichroeb.net>
In preparation for converting the stash command incrementally to
a builtin command, this patch improves test coverage of the option
parsing. Both for having too many parameters, or too few.
Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
t/t3903-stash.sh | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index 5f8272b6f9..ac55629737 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -444,6 +444,36 @@ test_expect_failure 'stash file to directory' '
test foo = "$(cat file/file)"
'
+test_expect_success 'giving too many ref arguments does not modify files' '
+ git stash clear &&
+ test_when_finished "git reset --hard HEAD" &&
+ echo foo >file2 &&
+ git stash &&
+ echo bar >file2 &&
+ git stash &&
+ test-tool chmtime =123456789 file2 &&
+ for type in apply pop "branch stash-branch"
+ do
+ test_must_fail git stash $type stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ test 123456789 = $(test-tool chmtime -g file2) || return 1
+ done
+'
+
+test_expect_success 'drop: too many arguments errors out (does nothing)' '
+ git stash list >expect &&
+ test_must_fail git stash drop stash@{0} stash@{1} 2>err &&
+ test_i18ngrep "Too many revisions" err &&
+ git stash list >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'show: too many arguments errors out (does nothing)' '
+ test_must_fail git stash show stash@{0} stash@{1} 2>err 1>out &&
+ test_i18ngrep "Too many revisions" err &&
+ test_must_be_empty out
+'
+
test_expect_success 'stash create - no changes' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
@@ -479,6 +509,11 @@ test_expect_success 'stash branch - stashes on stack, stash-like argument' '
test $(git ls-files --modified | wc -l) -eq 1
'
+test_expect_success 'stash branch complains with no arguments' '
+ test_must_fail git stash branch 2>err &&
+ test_i18ngrep "No branch name specified" err
+'
+
test_expect_success 'stash show format defaults to --stat' '
git stash clear &&
test_when_finished "git reset --hard HEAD" &&
--
2.20.1.441.g764a526393
^ permalink raw reply related
* [PATCH v12 03/26] strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
From: Paul-Sebastian Ungureanu @ 2018-12-20 19:44 UTC (permalink / raw)
To: git; +Cc: t.gummerer, Johannes.Schindelin
In-Reply-To: <cover.1545331726.git.ungureanupaulsebastian@gmail.com>
Implement `strbuf_insertf()` and `strbuf_vinsertf()` to
insert data using a printf format string.
Original-idea-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
strbuf.c | 36 ++++++++++++++++++++++++++++++++++++
strbuf.h | 9 +++++++++
2 files changed, 45 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index 82e90f1dfe..bfbbdadbf3 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -249,6 +249,42 @@ void strbuf_insert(struct strbuf *sb, size_t pos, const void *data, size_t len)
strbuf_splice(sb, pos, 0, data, len);
}
+void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt, va_list ap)
+{
+ int len, len2;
+ char save;
+ va_list cp;
+
+ if (pos > sb->len)
+ die("`pos' is too far after the end of the buffer");
+ va_copy(cp, ap);
+ len = vsnprintf(sb->buf + sb->len, 0, fmt, cp);
+ va_end(cp);
+ if (len < 0)
+ BUG("your vsnprintf is broken (returned %d)", len);
+ if (!len)
+ return; /* nothing to do */
+ if (unsigned_add_overflows(sb->len, len))
+ die("you want to use way too much memory");
+ strbuf_grow(sb, len);
+ memmove(sb->buf + pos + len, sb->buf + pos, sb->len - pos);
+ /* vsnprintf() will append a NUL, overwriting one of our characters */
+ save = sb->buf[pos + len];
+ len2 = vsnprintf(sb->buf + pos, sb->alloc - sb->len, fmt, ap);
+ sb->buf[pos + len] = save;
+ if (len2 != len)
+ BUG("your vsnprintf is broken (returns inconsistent lengths)");
+ strbuf_setlen(sb, sb->len + len);
+}
+
+void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ strbuf_vinsertf(sb, pos, fmt, ap);
+ va_end(ap);
+}
+
void strbuf_remove(struct strbuf *sb, size_t pos, size_t len)
{
strbuf_splice(sb, pos, len, "", 0);
diff --git a/strbuf.h b/strbuf.h
index be02150df3..8f8fe01e68 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -244,6 +244,15 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);
*/
void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
+/**
+ * Insert data to the given position of the buffer giving a printf format
+ * string. The contents will be shifted, not overwritten.
+ */
+void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
+ va_list ap);
+
+void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
+
/**
* Remove given amount of data from a given position of the buffer.
*/
--
2.20.1.441.g764a526393
^ permalink raw reply related
* [PATCH v12 02/26] strbuf.c: add `strbuf_join_argv()`
From: Paul-Sebastian Ungureanu @ 2018-12-20 19:44 UTC (permalink / raw)
To: git; +Cc: t.gummerer, Johannes.Schindelin
In-Reply-To: <cover.1545331726.git.ungureanupaulsebastian@gmail.com>
Implement `strbuf_join_argv()` to join arguments
into a strbuf.
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
strbuf.c | 15 +++++++++++++++
strbuf.h | 7 +++++++
2 files changed, 22 insertions(+)
diff --git a/strbuf.c b/strbuf.c
index f6a6cf78b9..82e90f1dfe 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -268,6 +268,21 @@ void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2)
strbuf_setlen(sb, sb->len + sb2->len);
}
+const char *strbuf_join_argv(struct strbuf *buf,
+ int argc, const char **argv, char delim)
+{
+ if (!argc)
+ return buf->buf;
+
+ strbuf_addstr(buf, *argv);
+ while (--argc) {
+ strbuf_addch(buf, delim);
+ strbuf_addstr(buf, *(++argv));
+ }
+
+ return buf->buf;
+}
+
void strbuf_addchars(struct strbuf *sb, int c, size_t n)
{
strbuf_grow(sb, n);
diff --git a/strbuf.h b/strbuf.h
index fc40873b65..be02150df3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -288,6 +288,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s)
*/
void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2);
+/**
+ * Join the arguments into a buffer. `delim` is put between every
+ * two arguments.
+ */
+const char *strbuf_join_argv(struct strbuf *buf, int argc,
+ const char **argv, char delim);
+
/**
* This function can be used to expand a format string containing
* placeholders. To that end, it parses the string and calls the specified
--
2.20.1.441.g764a526393
^ permalink raw reply related
* [PATCH v12 01/26] sha1-name.c: add `get_oidf()` which acts like `get_oid()`
From: Paul-Sebastian Ungureanu @ 2018-12-20 19:44 UTC (permalink / raw)
To: git; +Cc: t.gummerer, Johannes.Schindelin
In-Reply-To: <cover.1545331726.git.ungureanupaulsebastian@gmail.com>
Compared to `get_oid()`, `get_oidf()` has as parameters
a pointer to `object_id`, a printf format string and
additional arguments. This will help simplify the code
in subsequent commits.
Original-idea-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
---
cache.h | 1 +
sha1-name.c | 19 +++++++++++++++++++
2 files changed, 20 insertions(+)
diff --git a/cache.h b/cache.h
index b7acd81d2e..28ccc97e10 100644
--- a/cache.h
+++ b/cache.h
@@ -1335,6 +1335,7 @@ struct object_context {
GET_OID_BLOB)
extern int get_oid(const char *str, struct object_id *oid);
+extern int get_oidf(struct object_id *oid, const char *fmt, ...);
extern int get_oid_commit(const char *str, struct object_id *oid);
extern int get_oid_committish(const char *str, struct object_id *oid);
extern int get_oid_tree(const char *str, struct object_id *oid);
diff --git a/sha1-name.c b/sha1-name.c
index b24502811b..9524a09525 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -1516,6 +1516,25 @@ int get_oid(const char *name, struct object_id *oid)
return get_oid_with_context(name, 0, oid, &unused);
}
+/*
+ * This returns a non-zero value if the string (built using printf
+ * format and the given arguments) is not a valid object.
+ */
+int get_oidf(struct object_id *oid, const char *fmt, ...)
+{
+ va_list ap;
+ int ret;
+ struct strbuf sb = STRBUF_INIT;
+
+ va_start(ap, fmt);
+ strbuf_vaddf(&sb, fmt, ap);
+ va_end(ap);
+
+ ret = get_oid(sb.buf, oid);
+ strbuf_release(&sb);
+
+ return ret;
+}
/*
* Many callers know that the user meant to name a commit-ish by
--
2.20.1.441.g764a526393
^ permalink raw reply related
* [PATCH v12 00/26] Convert "git stash" to C builtin
From: Paul-Sebastian Ungureanu @ 2018-12-20 19:44 UTC (permalink / raw)
To: git; +Cc: t.gummerer, Johannes.Schindelin
In-Reply-To: <https://public-inbox.org/git/cover.1542925164.git.ungureanupaulsebastian@gmail.com/>
Hello,
This is a new iteration of git-stash which also takes
sd/stash-wo-user-name into account. I cherry-picked
some of dscho's commits (from [1]) to keep the scripted
version of `git stash` as `git-legacy-stash`.
Thank you for your suggestions!
Best,
Paul
[1]
https://github.com/dscho/git
Joel Teichroeb (5):
stash: improve option parsing test coverage
stash: convert apply to builtin
stash: convert drop and clear to builtin
stash: convert branch to builtin
stash: convert pop to builtin
Johannes Schindelin (4):
ident: add the ability to provide a "fallback identity"
stash: add back the original, scripted `git stash`
stash: optionally use the scripted version again
tests: add a special setup where stash.useBuiltin is off
Paul-Sebastian Ungureanu (17):
sha1-name.c: add `get_oidf()` which acts like `get_oid()`
strbuf.c: add `strbuf_join_argv()`
strbuf.c: add `strbuf_insertf()` and `strbuf_vinsertf()`
t3903: modernize style
stash: rename test cases to be more descriptive
stash: add tests for `git stash show` config
stash: mention options in `show` synopsis
stash: convert list to builtin
stash: convert show to builtin
stash: convert store to builtin
stash: convert create to builtin
stash: convert push to builtin
stash: make push -q quiet
stash: convert save to builtin
stash: optimize `get_untracked_files()` and `check_changes()`
stash: replace all `write-tree` child processes with API calls
stash: convert `stash--helper.c` into `stash.c`
.gitignore | 1 +
Documentation/git-stash.txt | 4 +-
Makefile | 3 +-
builtin.h | 1 +
builtin/stash.c | 1636 +++++++++++++++++++++++++++
cache.h | 2 +
git-stash.sh => git-legacy-stash.sh | 34 +-
git-sh-setup.sh | 1 +
git.c | 6 +
ident.c | 20 +
sha1-name.c | 19 +
strbuf.c | 51 +
strbuf.h | 16 +
t/README | 4 +
t/t3903-stash.sh | 192 ++--
t/t3907-stash-show-config.sh | 83 ++
16 files changed, 2001 insertions(+), 72 deletions(-)
create mode 100644 builtin/stash.c
rename git-stash.sh => git-legacy-stash.sh (97%)
create mode 100755 t/t3907-stash-show-config.sh
--
2.20.1.441.g764a526393
^ permalink raw reply
* Re: [PATCH v4 0/3] Add commit-graph fuzzer and fix buffer overflow
From: Johannes Schindelin @ 2018-12-20 19:35 UTC (permalink / raw)
To: Jeff King; +Cc: git, gitster, stolee, avarab, szeder.dev
In-Reply-To: <20181219155107.GD14802@sigill.intra.peff.net>
Hi Peff,
On Wed, 19 Dec 2018, Jeff King wrote:
> On Tue, Dec 18, 2018 at 01:05:51PM -0800, Josh Steadmon wrote:
>
> > On 2018.12.18 12:35, Jeff King wrote:
> > > On Thu, Dec 13, 2018 at 11:43:55AM -0800, Josh Steadmon wrote:
> > >
> > > > Add a new fuzz test for the commit graph and fix a buffer read-overflow
> > > > that it discovered. Additionally, fix the Makefile instructions for
> > > > building fuzzers.
> > > >
> > > > Changes since V3:
> > > > * Improve portability of the new test functionality.
> > >
> > > I thought there was some question about /dev/zero, which I think is
> > > in this version (I don't actually know whether there are portability
> > > issues or not, but somebody did mention it).
> > >
> > > -Peff
> >
> > I've only found one reference [1] (from 1999) of OS X Server not having
> > a /dev/zero. It appears to be present as of 2010 though [2].
>
> Thanks for digging. That seems like enough to assume we should try it
> and see if any macOS people complain.
>
> I do wonder if we'll run into problems on Windows, though.
As long as we're talking about Unix shell scripts, /dev/zero should be
fine, as we are essentially running in a variant of Cygwin.
If you try to pass /dev/zero as an argument to a Git command, that's an
entirely different thing: this most likely won't work.
Ciao,
Dscho
^ permalink raw reply
* Re: rebase: Ask before doing anything not undoable
From: Ricardo Biehl Pasquali @ 2018-12-20 18:56 UTC (permalink / raw)
To: Thomas Braun; +Cc: git
In-Reply-To: <3a7a52c3-3ae7-6e5e-b660-49fb21dc7d17@virtuell-zuhause.de>
On Thu, Dec 20, 2018 at 07:28:20PM +0100, Thomas Braun wrote:
> Do you know about the reflog? This let's you undo these kind of changes.
> See git help reflog.
I didn't. This should be documented in rebase manual page.
Perhaps a message after the command might be interesting
(e.g.: "to undo use git reflog").
Thank you!
pasquali
^ permalink raw reply
* Re: rebase: Ask before doing anything not undoable
From: Thomas Braun @ 2018-12-20 18:28 UTC (permalink / raw)
To: Ricardo Biehl Pasquali, git
In-Reply-To: <20181220173348.GA5203@localhost.localdomain>
Am 20.12.2018 um 18:33 schrieb Ricardo Biehl Pasquali:
> It was the second time I've typed "git rebase --co",
> hit tab (for completion) and enter, and the result was
> --committer-date-is-author-date instead of --continue .
>
> Please do ask user before run actions like these! I have
> not found a way to undo that. I had to perform a reset in
> the repository.
>
> PS: Notice I was expecting a command of interactive rebase,
> and I got a bunch of commits's date (and hash) changed.
Do you know about the reflog? This let's you undo these kind of changes.
See git help reflog.
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Duy Nguyen @ 2018-12-20 17:37 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, Git Mailing List
In-Reply-To: <20181220173204.GC6684@sigill.intra.peff.net>
On Thu, Dec 20, 2018 at 6:32 PM Jeff King <peff@peff.net> wrote:
>
> On Thu, Dec 20, 2018 at 06:23:42PM +0100, Duy Nguyen wrote:
>
> > On Thu, Dec 20, 2018 at 6:18 PM Jeff King <peff@peff.net> wrote:
> > > > I wonder if --follow-symlinks would be a good alternative for this
> > > > (then if the final destination is unmmapable then we just read the
> > > > file whole in memory without the user asking, so it will work with
> > > > pipes). --follow-symlinks then could be made work with non-"no-index"
> > > > case too. But --literally is also ok.
> > >
> > > It's more than symlinks, though. Reading from a named pipe, we'd want to
> > > see the actual contents with --literally (and not "oops, I don't know
> > > how to represent a named pipe").
> >
> > Yes, but I think at least --no-index it makes sense to just fall back
> > to read() if we can't mmap(). mmap is more of an optimization than a
> > requirement. There's no loss going from "oops I don't know how to
> > represent it" to "here's the content from whatever what that device
> > is". Symlinks are different because we have two possible
> > representations and the user should choose.
>
> Oh, I see. I misread your paragraph. Yeah, I think that is the behavior
> I would want by default, too, though the previous thread makes me thing
> some people would literally rather have the "I can't represent this"
> behavior (because they really do want a diff that can reconstruct the
> filesystem state, and an error if not).
I can't see a good use case for that. But yeah it would not harm to be
a bit more conservative, then make --literally default later and leave
--no-literally as an escape hatch.
--
Duy
^ permalink raw reply
* rebase: Ask before doing anything not undoable
From: Ricardo Biehl Pasquali @ 2018-12-20 17:33 UTC (permalink / raw)
To: git
It was the second time I've typed "git rebase --co",
hit tab (for completion) and enter, and the result was
--committer-date-is-author-date instead of --continue .
Please do ask user before run actions like these! I have
not found a way to undo that. I had to perform a reset in
the repository.
PS: Notice I was expecting a command of interactive rebase,
and I got a bunch of commits's date (and hash) changed.
Cheers!
pasquali
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Jeff King @ 2018-12-20 17:32 UTC (permalink / raw)
To: Duy Nguyen; +Cc: brian m. carlson, Git Mailing List
In-Reply-To: <CACsJy8AFwSgL-YAc2YU2XfqKFkXf-W+2V7tMy3ZOYvm_zhv5Bg@mail.gmail.com>
On Thu, Dec 20, 2018 at 06:23:42PM +0100, Duy Nguyen wrote:
> On Thu, Dec 20, 2018 at 6:18 PM Jeff King <peff@peff.net> wrote:
> > > I wonder if --follow-symlinks would be a good alternative for this
> > > (then if the final destination is unmmapable then we just read the
> > > file whole in memory without the user asking, so it will work with
> > > pipes). --follow-symlinks then could be made work with non-"no-index"
> > > case too. But --literally is also ok.
> >
> > It's more than symlinks, though. Reading from a named pipe, we'd want to
> > see the actual contents with --literally (and not "oops, I don't know
> > how to represent a named pipe").
>
> Yes, but I think at least --no-index it makes sense to just fall back
> to read() if we can't mmap(). mmap is more of an optimization than a
> requirement. There's no loss going from "oops I don't know how to
> represent it" to "here's the content from whatever what that device
> is". Symlinks are different because we have two possible
> representations and the user should choose.
Oh, I see. I misread your paragraph. Yeah, I think that is the behavior
I would want by default, too, though the previous thread makes me thing
some people would literally rather have the "I can't represent this"
behavior (because they really do want a diff that can reconstruct the
filesystem state, and an error if not).
-Peff
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Duy Nguyen @ 2018-12-20 17:23 UTC (permalink / raw)
To: Jeff King; +Cc: brian m. carlson, Git Mailing List
In-Reply-To: <20181220171725.GB6684@sigill.intra.peff.net>
On Thu, Dec 20, 2018 at 6:18 PM Jeff King <peff@peff.net> wrote:
> > I wonder if --follow-symlinks would be a good alternative for this
> > (then if the final destination is unmmapable then we just read the
> > file whole in memory without the user asking, so it will work with
> > pipes). --follow-symlinks then could be made work with non-"no-index"
> > case too. But --literally is also ok.
>
> It's more than symlinks, though. Reading from a named pipe, we'd want to
> see the actual contents with --literally (and not "oops, I don't know
> how to represent a named pipe").
Yes, but I think at least --no-index it makes sense to just fall back
to read() if we can't mmap(). mmap is more of an optimization than a
requirement. There's no loss going from "oops I don't know how to
represent it" to "here's the content from whatever what that device
is". Symlinks are different because we have two possible
representations and the user should choose.
--
Duy
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Jeff King @ 2018-12-20 17:17 UTC (permalink / raw)
To: Duy Nguyen; +Cc: brian m. carlson, Git Mailing List
In-Reply-To: <CACsJy8CScTBbYJt_LLp-rBdmJubEQOZqkPQeszzax9YpbCPUkg@mail.gmail.com>
On Thu, Dec 20, 2018 at 06:06:11PM +0100, Duy Nguyen wrote:
> On Thu, Dec 20, 2018 at 1:26 AM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > @@ -5159,6 +5159,8 @@ int diff_opt_parse(struct diff_options *options,
> > options->flags.funccontext = 1;
> > else if (!strcmp(arg, "--no-function-context"))
> > options->flags.funccontext = 0;
> > + else if (!strcmp(arg, "--literally"))
> > + options->flags.read_literally = 1;
>
> You probably want to check in diff_setup_done() that if
> flags.read_literally is set but flags.no_index is not, then abandon
> ship and die() because --literally is not used with --no-index. Even
> when --no-index is implicit, flags.no_index should be set.
Yeah, good catch. "git diff --literally HEAD" should report an error.
> I wonder if --follow-symlinks would be a good alternative for this
> (then if the final destination is unmmapable then we just read the
> file whole in memory without the user asking, so it will work with
> pipes). --follow-symlinks then could be made work with non-"no-index"
> case too. But --literally is also ok.
It's more than symlinks, though. Reading from a named pipe, we'd want to
see the actual contents with --literally (and not "oops, I don't know
how to represent a named pipe").
-Peff
^ permalink raw reply
* Re: [PATCH] t5570: drop racy test
From: Jeff King @ 2018-12-20 17:14 UTC (permalink / raw)
To: Thomas Gummerer
Cc: Torsten Bögershausen, Git Mailing List, szeder.dev,
Jan Palus, Johannes Schindelin
In-Reply-To: <20181220164150.GB25639@hank.intra.tgummerer.com>
On Thu, Dec 20, 2018 at 04:41:50PM +0000, Thomas Gummerer wrote:
> > That doesn't really fix it, but just broadens the race window. I dunno.
> > Maybe that is enough in practice. We could do something like:
> >
> > repeat_with_timeout () {
> > local i=0
> > while test $i -lt 10
> > do
> > "$@" && return 0
> > sleep 1
> > done
> > # no success even after 10 seconds
> > return 1
> > }
> >
> > repeat_with_timeout grep -i extended.attribute daemon.log
> >
> > to make the pattern a bit more obvious (and make it easy to extend the
> > window arbitrarily; surely 10s is enough?).
>
> I gave this a try, with below patch to lib-git-daemon.sh that you
> proposed in the previous thread about this racyness. That shows
> another problem though, namely when truncating 'daemon.log' before
> running 'git ls-remote' in this test, we're not sure all 'git deamon'
> has flushed everything from previous invocations. That may be an even
> rarer problem in practice, but still something to keep in mind.
Right, that makes sense. Making this race-proof really does require a
separate log stream for each test. I guess we'd need to be able to send
git-daemon a signal to re-open the log (which actually is not as
unreasonable as it may seem; lots of daemons have this for log
rotation).
I think getting rid of the "cat" would also help a lot here.
Unfortunately I think we use it not just for its "tee" effect, but also
to avoid startup races by checking the "Ready to rumble" line. So again,
we'd need some cooperation from git-daemon to tell us out-of-band that
it has completed its startup (e.g., by touching another file).
> Dscho also mentioned on #git-devel a while ago that he may have a look
> at actually making this test race-proof, but I guess he's been busy
> with the 2.20 release. I'm also not sure it's worth spending a lot of
> time trying to fix this test, but I'd definitely be happy if someone
> proposes a different solution.
Yeah. I'm sure it's fixable with enough effort, but I just think there
are more interesting and important things to work on.
> --- >8 ---
> Subject: [PATCH] t5570: drop racy test
So yeah, I'm still fine with this. But...
> ---
> t/t5570-git-daemon.sh | 13 -------------
> 1 file changed, 13 deletions(-)
This is the only user of daemon.log, so we could drop those bits from
lib-git-daemon.sh, too. That would also prevent people from adding new
tests, thinking that this was somehow not horribly racy). I.e.,
reverting 314a73d658 (t/lib-git-daemon: record daemon log, 2018-01-25).
-Peff
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Duy Nguyen @ 2018-12-20 17:06 UTC (permalink / raw)
To: brian m. carlson; +Cc: Git Mailing List, Jeff King
In-Reply-To: <20181220002610.43832-1-sandals@crustytoothpaste.net>
On Thu, Dec 20, 2018 at 1:26 AM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> @@ -5159,6 +5159,8 @@ int diff_opt_parse(struct diff_options *options,
> options->flags.funccontext = 1;
> else if (!strcmp(arg, "--no-function-context"))
> options->flags.funccontext = 0;
> + else if (!strcmp(arg, "--literally"))
> + options->flags.read_literally = 1;
You probably want to check in diff_setup_done() that if
flags.read_literally is set but flags.no_index is not, then abandon
ship and die() because --literally is not used with --no-index. Even
when --no-index is implicit, flags.no_index should be set.
I wonder if --follow-symlinks would be a good alternative for this
(then if the final destination is unmmapable then we just read the
file whole in memory without the user asking, so it will work with
pipes). --follow-symlinks then could be made work with non-"no-index"
case too. But --literally is also ok.
--
Duy
^ permalink raw reply
* Re: Periodic hang during git index-pack
From: Sitsofe Wheeler @ 2018-12-20 16:48 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20181220151037.GC27361@sigill.intra.peff.net>
On Thu, 20 Dec 2018 at 15:11, Jeff King <peff@peff.net> wrote:
>
> OK, that's about what I expected. Here we have clone's sideband-demux
> thread waiting to pull more packfile data from the remote:
>
> > (gdb) thread apply all bt
> >
> > Thread 2 (Thread 0x7faafbf1c700 (LWP 36586)):
> > #0 0x00007faafc805384 in __libc_read (fd=fd@entry=5,
> > buf=buf@entry=0x7faafbf0ddec, nbytes=nbytes@entry=5)
> > at ../sysdeps/unix/sysv/linux/read.c:27
> > #1 0x000055c8ca2f5b23 in read (__nbytes=5, __buf=0x7faafbf0ddec, __fd=5)
> > at /usr/include/x86_64-linux-gnu/bits/unistd.h:44
> > [...coming from packet_read / recv_sideband / sideband_demux...]
>
> I assume fd=5 there is a pipe connected to ssh. You could double check
> with "lsof" or similar, but I don't think it would ever be reading from
> anywhere else.
I checked and in all cases git was reading from a pipe.
[snip]
> with each blocking on read() from its predecessor. So you need to find
> out why "ssh" is blocking. Unfortunately, short of a bug in ssh, the
> likely cause is either:
>
> 1. The git-upload-pack on the remote side stopped generating data for
> some reason. You may or may not have access on the remotehost to
> dig into that.
>
> It's certainly possible there's a deadlock bug between the server
> and client side of a Git conversation. But I'd find it extremely
> unlikely to find such a deadlock bug at this point in the
> conversation, because at this point the client side has nothing
> left to say to the server. The server should just be streaming out
> the packfile bytes and then closing the descriptor.
I think it's highly unlikely too given how many good runs we generally have.
> You mentioned "Phabricator sshd scripts" running on the server.
> I don't know what Phabricator might be sticking in the middle of
> the connection, but that could be the source of the stall.
I think you're right. I set up a seperate sshd on a different port on
the same machine where there were no Phabricator callouts and the
problem never manifested...
> 2. It's possible the network connection dropped but ssh did not
> notice. Maybe try turning on ssh keepalives and seeing if it
> eventually times out?
I had already done this but the problem still manifested. I went so
far as checking if the problem would happen over the loopback device
on the same machine (via localhost) and the problem continued
happening so I'm fairly sure that rules out networking issues.
--
Sitsofe | http://sucs.org/~sits/
^ permalink raw reply
* [PATCH] t5570: drop racy test
From: Thomas Gummerer @ 2018-12-20 16:41 UTC (permalink / raw)
To: Jeff King
Cc: Torsten Bögershausen, Git Mailing List, szeder.dev,
Jan Palus, Johannes Schindelin
In-Reply-To: <20181126164252.GA27711@sigill.intra.peff.net>
On 11/26, Jeff King wrote:
> On Sun, Nov 25, 2018 at 10:01:38PM +0000, Thomas Gummerer wrote:
>
> > On 11/25, Torsten Bögershausen wrote:
> > > After running the "Git 2.20-rc1" testsuite here on a raspi,
> > > the only TC that failed was t5570.
> > > When the "grep" was run on daemon.log, the file was empty (?).
> > > When inspecting it later, it was filled, and grep would have found
> > > the "extended.attribute" it was looking for.
> >
> > I believe this has been reported before in
> > https://public-inbox.org/git/1522783990.964448.1325338528.0D49CC15@webmail.messagingengine.com/,
> > but it seems like the thread never ended with actually fixing it.
> > Reading the first reply Peff seemed to be fine with just removing the
> > test completely, which would be the easiest solution ;) Adding him to
> > Cc: here.
>
> Yes, I don't think there is a way to make this race-proof without
> somehow convincing "cat" to flush (and let us know when it has). Which
> really implies killing the daemon, and wait()ing on cat to process the
> EOF and exit. And that makes the tests a lot more expensive if we have
> to start the daemon for each snippet.
>
> So I'm still fine with just dropping this test.
Alright since this has come up twice on the mailing list now (and I've
seen this racyness as well), and nobody found the time to write a
patch yet, below is a patch to just remove the test.
> > > diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
> > > index 7466aad111..e259fee0ed 100755
> > > --- a/t/t5570-git-daemon.sh
> > > +++ b/t/t5570-git-daemon.sh
> > > @@ -192,6 +192,7 @@ test_expect_success 'daemon log records all attributes' '
> > > GIT_OVERRIDE_VIRTUAL_HOST=localhost \
> > > git -c protocol.version=1 \
> > > ls-remote "$GIT_DAEMON_URL/interp.git" &&
> > > + sleep 1 &&
> > > grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
> > > test_cmp expect actual
> > > '
> > > ----------------
> > > A slightly better approach may be to use a "sleep on demand":
> > >
> > > + ( grep -i -q extended.attribute daemon.log || sleep 1 ) &&
>
> That doesn't really fix it, but just broadens the race window. I dunno.
> Maybe that is enough in practice. We could do something like:
>
> repeat_with_timeout () {
> local i=0
> while test $i -lt 10
> do
> "$@" && return 0
> sleep 1
> done
> # no success even after 10 seconds
> return 1
> }
>
> repeat_with_timeout grep -i extended.attribute daemon.log
>
> to make the pattern a bit more obvious (and make it easy to extend the
> window arbitrarily; surely 10s is enough?).
I gave this a try, with below patch to lib-git-daemon.sh that you
proposed in the previous thread about this racyness. That shows
another problem though, namely when truncating 'daemon.log' before
running 'git ls-remote' in this test, we're not sure all 'git deamon'
has flushed everything from previous invocations. That may be an even
rarer problem in practice, but still something to keep in mind.
diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index edbea2d986..3c7fea169b 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -62,6 +62,7 @@ start_git_daemon() {
(
while read -r line <&7
do
+ sleep 1
printf "%s\n" "$line"
printf >&4 "%s\n" "$line"
done
Dscho also mentioned on #git-devel a while ago that he may have a look
at actually making this test race-proof, but I guess he's been busy
with the 2.20 release. I'm also not sure it's worth spending a lot of
time trying to fix this test, but I'd definitely be happy if someone
proposes a different solution.
> -Peff
--- >8 ---
Subject: [PATCH] t5570: drop racy test
t5570 being racy has been reported twice separately on the mailing
list [*1*, *2*].
To make the test race proof, we'd either have to introduce another
fifo the test snippet is waiting on, or somehow convincing "cat" to
flush (and let us know when it has). Which really implies killing the
daemon, and wait()ing on cat to process the EOF and exit. And that
makes the tests a lot more expensive if we have to start the daemon
for each snippet.
As this is a test for a relatively minor fix (according to the author)
in 19136be3f8 ("daemon: fix off-by-one in logging extended
attributes", 2018-01-24), drop it to avoid this racyness. It doesn't
seem worth making the test code much more complex, or slowing down all
tests just to keep this one.
*1*: 1522783990.964448.1325338528.0D49CC15@webmail.messagingengine.com/
*2*: 9d4e5224-9ff4-f3f8-519d-7b2a6f1ea7cd@web.de
Reported-by: Jan Palus <jpalus@fastmail.com>
Reported-by: Torsten Bögershausen <tboegi@web.de>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Thomas Gummerer <t.gummerer@gmail.com>
---
t/t5570-git-daemon.sh | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
index 7466aad111..58ee787685 100755
--- a/t/t5570-git-daemon.sh
+++ b/t/t5570-git-daemon.sh
@@ -183,19 +183,6 @@ test_expect_success 'hostname cannot break out of directory' '
git ls-remote "$GIT_DAEMON_URL/escape.git"
'
-test_expect_success 'daemon log records all attributes' '
- cat >expect <<-\EOF &&
- Extended attribute "host": localhost
- Extended attribute "protocol": version=1
- EOF
- >daemon.log &&
- GIT_OVERRIDE_VIRTUAL_HOST=localhost \
- git -c protocol.version=1 \
- ls-remote "$GIT_DAEMON_URL/interp.git" &&
- grep -i extended.attribute daemon.log | cut -d" " -f2- >actual &&
- test_cmp expect actual
-'
-
test_expect_success FAKENC 'hostname interpolation works after LF-stripping' '
{
printf "git-upload-pack /interp.git\n\0host=localhost" | packetize
--
2.20.1.415.g653613c723
^ permalink raw reply related
* [PATCH 4/5] travis-ci: switch to Xcode 10.1 macOS image
From: SZEDER Gábor @ 2018-12-20 16:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Johannes Schindelin, git, SZEDER Gábor
In-Reply-To: <20181220162452.17732-1-szeder.dev@gmail.com>
When building something with GCC installed from Homebrew in the
default macOS (with Xcode 9.4) image on Travis CI, it errors out with
something like this:
/usr/local/Cellar/gcc/8.1.0/lib/gcc/8/gcc/x86_64-apple-darwin17.5.0/8.1.0/include-fixed/stdio.h:78:10: fatal error: _stdio.h: No such file or directory
#include <_stdio.h>
^~~~~~~~~~
This seems to be a common problem affecting several projects, and the
common solution is to use a Travis CI macOS image with more recent
Xcode version, e.g. 10 or 10.1.
While we don't use such a GCC yet, in the very next patch we will, so
switch our OSX build jobs to use the Xcode 10.1 image. Compared to
the Xcode 10 image, this has the benefit that it comes with GCC (v8.2)
preinstalled from Homebrew.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
.travis.yml | 2 ++
1 file changed, 2 insertions(+)
diff --git a/.travis.yml b/.travis.yml
index 03c8e4c613..36cbdea7f4 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -8,6 +8,8 @@ os:
- linux
- osx
+osx_image: xcode10.1
+
compiler:
- clang
- gcc
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH 5/5] travis-ci: build with the right compiler
From: SZEDER Gábor @ 2018-12-20 16:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Johannes Schindelin, git, SZEDER Gábor
In-Reply-To: <20181220162452.17732-1-szeder.dev@gmail.com>
Our 'Makefile' hardcodes the compiler to build Git as 'CC = cc'. This
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.
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 related
* [PATCH 3/5] travis-ci: don't be '--quiet' when running the tests
From: SZEDER Gábor @ 2018-12-20 16:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Johannes Schindelin, git, SZEDER Gábor
In-Reply-To: <20181220162452.17732-1-szeder.dev@gmail.com>
All Travis CI build jobs run the test suite with 'make --quiet test'.
On one hand, being quiet doesn't save us from much clutter in the
output:
$ make test |wc -l
861
$ make --quiet test |wc -l
848
It only spares 13 lines, mostly the output of entering the 't/'
directory and the pre- and post-cleanup commands, which is negligible
compared to the ~700 lines printed while building Git and the ~850
lines of 'prove' output.
On the other hand, it's asking for trouble. In our CI build scripts
we build Git and run the test suite in two separate 'make'
invocations. In a prelimiary version of one of the later patches in
this series, to explicitly specify which compiler to use, I changed
them to basically run:
make CC=$CC
make --quiet test
naively thinking that it should Just Work... but then that 'make
--quiet test' got all clever on me, noticed the changed build flags,
and then proceeded to rebuild everything with the default 'cc'. And
because of that '--quiet' option, it did so, well, quietly, only
saying "* new build flags", and it was by mere luck that I happened to
notice that something is amiss.
Let's just drop that '--quiet' option when running the test suite in
all build scripts.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
ci/run-build-and-tests.sh | 4 ++--
ci/run-linux32-build.sh | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
index cda170d5c2..84431c097e 100755
--- a/ci/run-build-and-tests.sh
+++ b/ci/run-build-and-tests.sh
@@ -8,7 +8,7 @@
ln -s "$cache_dir/.prove" t/.prove
make --jobs=2
-make --quiet test
+make test
if test "$jobname" = "linux-gcc"
then
export GIT_TEST_SPLIT_INDEX=yes
@@ -17,7 +17,7 @@ then
export GIT_TEST_OE_DELTA_SIZE=5
export GIT_TEST_COMMIT_GRAPH=1
export GIT_TEST_MULTI_PACK_INDEX=1
- make --quiet test
+ make test
fi
check_unignored_build_artifacts
diff --git a/ci/run-linux32-build.sh b/ci/run-linux32-build.sh
index 2c60d2e70a..26c168a016 100755
--- a/ci/run-linux32-build.sh
+++ b/ci/run-linux32-build.sh
@@ -56,5 +56,5 @@ linux32 --32bit i386 su -m -l $CI_USER -c '
cd /usr/src/git
test -n "$cache_dir" && ln -s "$cache_dir/.prove" t/.prove
make --jobs=2
- make --quiet test
+ make test
'
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH 2/5] .gitignore: ignore external debug symbols from GCC on macOS
From: SZEDER Gábor @ 2018-12-20 16:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Johannes Schindelin, git, SZEDER Gábor
In-Reply-To: <20181220162452.17732-1-szeder.dev@gmail.com>
When Git is build with a "real" GCC on macOS [1], or at least with GCC
installed via Homebrew, and CFLAGS includes the '-g' option (and our
default CFLAGS does), then by default GCC writes the debug symbols
into external files under '<binary>.dSYM/' directories (e.g.
'git-daemon.dSYM/', 'git.dSYM/', etc.).
Update '.gitignore' to ignore these directories, so they don't clutter
the output of 'git status'. Furthermore, these build artifacts then
won't trigger build failures on Travis CI via b92cb86ea1 (travis-ci:
check that all build artifacts are .gitignore-d, 2017-12-31) once one
of the following patches updates our CI build scripts to use a real
GCC in the 'osx-gcc' build job.
[1] On macOS the default '/usr/bin/gcc' executable is not a real GCC,
but merely a compatibility wrapper around Clang:
$ gcc --version
Configured with: --prefix=<...>
Apple LLVM version 9.0.0 (clang-900.0.39.2)
<...>
So even though 'make CC=gcc' does indeed execute a command called
'gcc', in the end Git will be built with Clang all the same.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
.gitignore | 1 +
1 file changed, 1 insertion(+)
diff --git a/.gitignore b/.gitignore
index 0d77ea5894..a9db568712 100644
--- a/.gitignore
+++ b/.gitignore
@@ -229,3 +229,4 @@
*.pdb
/Debug/
/Release/
+*.dSYM
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH 1/5] compat/obstack: fix -Wcast-function-type warnings
From: SZEDER Gábor @ 2018-12-20 16:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Johannes Schindelin, git, SZEDER Gábor
In-Reply-To: <20181220162452.17732-1-szeder.dev@gmail.com>
When building Git with GCC 8.2.0 (at least from Homebrew on macOS,
DEVELOPER flags enabled) one is greeted with a screenful of compiler
errors:
compat/obstack.c: In function '_obstack_begin':
compat/obstack.c:162:17: error: cast between incompatible function types from 'void * (*)(long int)' to 'struct _obstack_chunk * (*)(void *, long int)' [-Werror=cast-function-type]
h->chunkfun = (struct _obstack_chunk * (*)(void *, long)) chunkfun;
^
compat/obstack.c:163:16: error: cast between incompatible function types from 'void (*)(void *)' to 'void (*)(void *, struct _obstack_chunk *)' [-Werror=cast-function-type]
h->freefun = (void (*) (void *, struct _obstack_chunk *)) freefun;
^
compat/obstack.c:116:8: error: cast between incompatible function types from 'struct _obstack_chunk * (*)(void *, long int)' to 'struct _obstack_chunk * (*)(long int)' [-Werror=cast-function-type]
: (*(struct _obstack_chunk *(*) (long)) (h)->chunkfun) ((size)))
^
compat/obstack.c:168:22: note: in expansion of macro 'CALL_CHUNKFUN'
chunk = h->chunk = CALL_CHUNKFUN (h, h -> chunk_size);
^~~~~~~~~~~~~
<snip>
'struct obstack' stores pointers to two functions to allocate and free
"chunks", and depending on how obstack is used, these functions take
either one parameter (like standard malloc() and free() do; this is
how we use it) or two parameters. Presumably to reduce memory
footprint, a single field is used to store the function pointer for
both signatures, and then it's casted to the appropriate signature
when the function pointer is accessed. These casts between function
pointers with different number of parameters are what trigger those
compiler errors.
Modify 'struct obstack' to use unions to store function pointers with
different signatures, and then use the union member with the
appropriate signature when accessing these function pointers. This
eliminates the need for those casts, and thus avoids this compiler
error.
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
compat/obstack.c | 17 +++++++++--------
compat/obstack.h | 18 +++++++++++-------
2 files changed, 20 insertions(+), 15 deletions(-)
diff --git a/compat/obstack.c b/compat/obstack.c
index 4d1d95beeb..dd9504c684 100644
--- a/compat/obstack.c
+++ b/compat/obstack.c
@@ -112,15 +112,15 @@ compat_symbol (libc, _obstack_compat, _obstack, GLIBC_2_0);
# define CALL_CHUNKFUN(h, size) \
(((h) -> use_extra_arg) \
- ? (*(h)->chunkfun) ((h)->extra_arg, (size)) \
- : (*(struct _obstack_chunk *(*) (long)) (h)->chunkfun) ((size)))
+ ? (*(h)->chunkfun.fn_extra_arg) ((h)->extra_arg, (size)) \
+ : (*(h)->chunkfun.fn) ((size)))
# define CALL_FREEFUN(h, old_chunk) \
do { \
if ((h) -> use_extra_arg) \
- (*(h)->freefun) ((h)->extra_arg, (old_chunk)); \
+ (*(h)->freefun.fn_extra_arg) ((h)->extra_arg, (old_chunk)); \
else \
- (*(void (*) (void *)) (h)->freefun) ((old_chunk)); \
+ (*(h)->freefun.fn) ((old_chunk)); \
} while (0)
\f
@@ -159,8 +159,8 @@ _obstack_begin (struct obstack *h,
size = 4096 - extra;
}
- h->chunkfun = (struct _obstack_chunk * (*)(void *, long)) chunkfun;
- h->freefun = (void (*) (void *, struct _obstack_chunk *)) freefun;
+ h->chunkfun.fn = chunkfun;
+ h->freefun.fn = freefun;
h->chunk_size = size;
h->alignment_mask = alignment - 1;
h->use_extra_arg = 0;
@@ -206,8 +206,9 @@ _obstack_begin_1 (struct obstack *h, int size, int alignment,
size = 4096 - extra;
}
- h->chunkfun = (struct _obstack_chunk * (*)(void *,long)) chunkfun;
- h->freefun = (void (*) (void *, struct _obstack_chunk *)) freefun;
+ h->chunkfun.fn_extra_arg = (struct _obstack_chunk * (*)(void *,long)) chunkfun;
+ h->freefun.fn_extra_arg = (void (*) (void *, struct _obstack_chunk *)) freefun;
+
h->chunk_size = size;
h->alignment_mask = alignment - 1;
h->extra_arg = arg;
diff --git a/compat/obstack.h b/compat/obstack.h
index 6bc24b7644..0f9b2232a9 100644
--- a/compat/obstack.h
+++ b/compat/obstack.h
@@ -160,11 +160,15 @@ struct obstack /* control current object in current chunk */
void *tempptr;
} temp; /* Temporary for some macros. */
int alignment_mask; /* Mask of alignment for each object. */
- /* These prototypes vary based on `use_extra_arg', and we use
- casts to the prototypeless function type in all assignments,
- but having prototypes here quiets -Wstrict-prototypes. */
- struct _obstack_chunk *(*chunkfun) (void *, long);
- void (*freefun) (void *, struct _obstack_chunk *);
+ /* These prototypes vary based on `use_extra_arg'. */
+ union {
+ struct _obstack_chunk *(*fn_extra_arg) (void *, long);
+ void *(*fn) (long);
+ } chunkfun;
+ union {
+ void (*fn_extra_arg) (void *, struct _obstack_chunk *);
+ void (*fn) (void *);
+ } freefun;
void *extra_arg; /* first arg for chunk alloc/dealloc funcs */
unsigned use_extra_arg:1; /* chunk alloc/dealloc funcs take extra arg */
unsigned maybe_empty_object:1;/* There is a possibility that the current
@@ -235,10 +239,10 @@ extern void (*obstack_alloc_failed_handler) (void);
(void (*) (void *, void *)) (freefun), (arg))
#define obstack_chunkfun(h, newchunkfun) \
- ((h) -> chunkfun = (struct _obstack_chunk *(*)(void *, long)) (newchunkfun))
+ ((h)->chunkfun.fn_extra_arg = (struct _obstack_chunk *(*)(void *, long)) (newchunkfun))
#define obstack_freefun(h, newfreefun) \
- ((h) -> freefun = (void (*)(void *, struct _obstack_chunk *)) (newfreefun))
+ ((h)->freefun.fn_extra_arg = (void (*)(void *, struct _obstack_chunk *)) (newfreefun))
#define obstack_1grow_fast(h,achar) (*((h)->next_free)++ = (achar))
--
2.20.1.151.gec613c4b75
^ permalink raw reply related
* [PATCH 0/5] travis-ci: build with the right compiler
From: SZEDER Gábor @ 2018-12-20 16:24 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Johannes Schindelin, git, SZEDER Gábor
In-Reply-To: <20181016184537.GN19800@szeder.dev>
On Tue, Oct 16, 2018 at 08:45:37PM +0200, SZEDER Gábor wrote:
> Our Makefile has lines like these:
>
> CFLAGS = -g -O2 -Wall
> CC = cc
> AR = ar
> SPATCH = spatch
>
> Note the use of '=', not '?='. This means that these variables 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 'cc'.
>
> This can be confusing for developers who come from other projects
> where they used to run 'CC=whatever make'.
>
> And our build jobs on Travis CI are badly affected by this. We have
> dedicated build jobs to build Git with GCC and Clang both on Linux and
> OSX from the very beginning (522354d70f (Add Travis CI support,
> 2015-11-27)). But guess how Travis CI specifies which compiler to
> use! With 'export CC=gcc' and 'export CC=clang', respectively.
> Consequently, our Clang Linux build job has always used gcc, because
> that's where 'cc' points at on Linux by default, while the GCC OSX
> build job has always used Clang. Oh, well. Furthermore, see
> 37fa4b3c78 (travis-ci: run gcc-8 on linux-gcc jobs, 2018-05-19), where
> Duy added an 'export CC=gcc-8' in an attempt to use a more modern
> compiler, but this had no effect either.
>
> I'm not sure what to do. I'm fine with updating our 'ci/' scripts to
> explicitly respect CC in the environment (either by running 'make
> CC=$CC' or by writing $CC into 'config.mak').
So, this patch series, in particular the last patch fixes this issue
by setting MAKEFLAGS to contain the right CC from the environment.
The first four patches are necessary cleanups/fixes to make it work,
though, arguably, the third patch is neither strictly necessary nor
that closely related to this series, but it just happened to bite me
while working on this series.
SZEDER Gábor (5):
compat/obstack: fix -Wcast-function-type warnings
.gitignore: ignore external debug symbols from GCC on macOS
travis-ci: don't be '--quiet' when running the tests
travis-ci: switch to Xcode 10.1 macOS image
travis-ci: build with the right compiler
.gitignore | 1 +
.travis.yml | 2 ++
ci/install-dependencies.sh | 5 +++++
ci/lib-travisci.sh | 15 ++++++++++++---
ci/run-build-and-tests.sh | 4 ++--
ci/run-linux32-build.sh | 2 +-
compat/obstack.c | 17 +++++++++--------
compat/obstack.h | 18 +++++++++++-------
8 files changed, 43 insertions(+), 21 deletions(-)
--
2.20.1.151.gec613c4b75
^ permalink raw reply
* Re: [PATCH v3 1/4] pack-protocol.txt: accept error packets in any context
From: Jeff King @ 2018-12-20 15:49 UTC (permalink / raw)
To: Masaya Suzuki, git, Junio C Hamano
In-Reply-To: <20181219233005.GI37614@google.com>
On Wed, Dec 19, 2018 at 03:30:05PM -0800, Josh Steadmon wrote:
> > > > This is outside of the Git pack protocol so having a separate parsing
> > > > mode makes sense to me.
> > >
> > > This sounds like it could be a significant refactoring. Should we go
> > > back to V2 of this series, and then work on the new parsing mode
> > > separately?
> >
> > Which one is v2? :)
> >
> > Just the remote-curl cleanups from me, and then your "die on server-side
> > errors" patch?
>
> Yes, that one :)
Then yes, that sounds reasonable to me.
-Peff
^ permalink raw reply
* Re: [PATCH] diff: add support for reading files literally with --no-index
From: Jeff King @ 2018-12-20 15:48 UTC (permalink / raw)
To: brian m. carlson; +Cc: git, Duy Nguyen, Dennis Kaarsemaker
In-Reply-To: <20181220002610.43832-1-sandals@crustytoothpaste.net>
On Thu, Dec 20, 2018 at 12:26:10AM +0000, brian m. carlson wrote:
> In some shells, such as bash and zsh, it's possible to use a command
> substitution to provide the output of a command as a file argument to
> another process, like so:
>
> diff -u <(printf "a\nb\n") <(printf "a\nc\n")
>
> However, this syntax does not produce useful results with git diff
> --no-index.
>
> On macOS, the arguments to the command are named pipes under /dev/fd,
> and git diff doesn't know how to handle a named pipe. On Linux, the
> arguments are symlinks to pipes, so git diff "helpfully" diffs these
> symlinks, comparing their targets like "pipe:[1234]" and "pipe:[5678]".
>
> Because this behavior is not very helpful, and because git diff has many
> features that people would like to use even on non-Git files, add an
> option to git diff --no-index to read files literally, dereferencing
> symlinks and reading them as a normal file.
>
> Note that this behavior requires that the files be read entirely into
> memory, just as we do when reading from standard input.
Yeah, I think this is a useful feature to have. It seemed familiar, and
indeed there were some patches a while back:
https://public-inbox.org/git/20170113102021.6054-1-dennis@kaarsemaker.net/
Compared to that series, the implementation here seems simpler and less
likely to have unexpected effects.
Reading the patch, one thing I _thought_ it did (but it does not) is to
impact only the actual arguments on the command line, not entries we
find from recursing trees.
I.e., if I said:
ln -s bar a/foo
ln -s baz b/foo
git diff --no-index --literally a/foo b/foo
git diff --no-index --literally a/ b/
should I still see a/foo as a symlink in the second one, or not?
The distinction is a bit subtle, but I think treating only the actual
top-level arguments as symlinks would solve your problem, but still
allow a more detailed diff for the recursive cases.
There's some more discussion on this in the earlier iteration of that
series:
https://public-inbox.org/git/20161111201958.2175-1-dennis@kaarsemaker.net/
I also suspect people might want a config option to make this the
default (which should be OK, as git-diff is, after all, porcelain). But
either way, a command line option is the first step.
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> This is a long-standing annoyance of mine, and it also makes some use
> cases possible (for example, diffing filtered and non-filtered objects).
>
> We don't include a test for the pipe scenario because I couldn't get
> that case to work in portable shell (although of course it works in
> bash). I have, however, tested it on both macOS and Linux. No clue how
> this works on Windows.
I think focusing on symlinks makes sense. You could probably test pipes
with mkfifo, but looking at your implementation, it's pretty clear that
it will operate on anything that works with read().
> diff --git a/Documentation/git-diff.txt b/Documentation/git-diff.txt
> index 030f162f30..4c4695c88d 100644
> --- a/Documentation/git-diff.txt
> +++ b/Documentation/git-diff.txt
> @@ -111,6 +111,11 @@ include::diff-options.txt[]
> "Unmerged". Can be used only when comparing the working tree
> with the index.
>
> +--literally::
> + Read the specified files literally, as `diff` would,
> + dereferencing any symlinks and reading data from pipes.
> + This option only works with `--no-index`.
> +
Looks like spaces for indent, whereas the context uses tabs.
I think "literal" is a good way to describe this concept. If we do grow
a config option to make this the default, then countermanding it would
be "--no-literally", which parses a bit funny as English.
If you agree with my "only the top-level" line of reasoning above, maybe
"--literal-arguments" and "--no-literal-arguments" might make sense.
We could also in theory offer several levels: no literals, top-level
literals, everything literal, at which point it becomes a tri-state.
> -static struct diff_filespec *noindex_filespec(const char *name, int mode)
> +static int populate_literally(struct diff_filespec *s)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + size_t size = 0;
> + int fd = xopen(s->path, O_RDONLY);
> +
> + if (strbuf_read(&buf, fd, 0) < 0)
> + return error_errno("error while reading from '%s'", s->path);
> +
> + s->should_munmap = 0;
> + s->data = strbuf_detach(&buf, &size);
> + s->size = size;
> + s->should_free = 1;
> + s->read_literally = 1;
> + return 0;
> +}
> +
> +static struct diff_filespec *noindex_filespec(const char *name, int mode,
> + struct diff_options *o)
> [...]
> + else if (o->flags.read_literally)
> + populate_literally(s);
The implementation is pleasantly simple. If we want to do the "only
top-level" thing, we'd just have to pass in a depth flag here.
> diff --git a/diff.c b/diff.c
> index dc9965e836..740d0087b9 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4282,18 +4282,18 @@ static void run_diff_cmd(const char *pgm,
> fprintf(o->file, "* Unmerged path %s\n", name);
> }
>
> -static void diff_fill_oid_info(struct diff_filespec *one, struct index_state *istate)
> +static void diff_fill_oid_info(struct diff_filespec *one, struct diff_options *o)
It might be worth breaking these "pass the options around" hunks into a
separate preparatory patch.
-Peff
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox