* Re: Git in GSoC 2024
From: Patrick Steinhardt @ 2024-01-31 13:10 UTC (permalink / raw)
To: Christian Couder
Cc: Kaartic Sivaraam, git, Taylor Blau, Junio C Hamano, Victoria Dye
In-Reply-To: <Zbi8pfvGpYrlZXAu@tanuki>
[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]
On Tue, Jan 30, 2024 at 10:08:53AM +0100, Patrick Steinhardt wrote:
> On Tue, Jan 30, 2024 at 09:38:48AM +0100, Christian Couder wrote:
> > On Mon, Jan 29, 2024 at 7:16 PM Kaartic Sivaraam
> > <kaartic.sivaraam@gmail.com> wrote:
> [snip]
> > > The GSoC contributor application deadline is April 2 - 18:00 UTC, so
> > > (co-)mentors and org admins are already welcome to volunteer. As usual,
> > > we also need project ideas to refresh our idea page from last year
> > > (https://git.github.io/SoC-2023-Ideas/). Feel free to share your
> > > thoughts and discuss.
> >
> > I am volunteering as both an Org Admin and a mentor too.
> >
> > I am not sure how many tests there are left to be ported to the new
> > unit test framework. Patrick told me about porting some reftable unit
> > tests to the new unit test framework though. So it might still work as
> > a GSoC project.
>
> Yes, the tests in t0032-reftable-unittest.sh should be ported over to
> the new unit test framework eventually, and I think that this might be a
> good GSoC project indeed.
>
> If there is interest I'd also be happy to draft up some more topics in
> the context of refs and the reftable backend. I'm sure there should be
> some topics here that would be a good fit for the GSoC project, and I'd
> be happy to mentor any such project in this context.
I noticed that the starting period falls right into my honeymoon from
June 17th until July 19th. This unfortunately makes it quite a lot
harder for me to mentor projects alone. Still, I'd be happy to co-mentor
or help out in other ways.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 3/5] reftable/writer: simplify writing index records
From: Toon Claes @ 2024-01-31 13:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <b0982baacf74a4501ce5c543b294bc15d6937875.1706263918.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> When finishing the current section we may end up writing index records
> for the section to the table. The logic to do so essentially copies what
> we already have in `writer_add_record()`, making this more complicated
> than it really has to be.
I didn't feel like this commit message made it easier for me to
understand, because I interpreted words differently than you intended.
Using "may end up" makes it sound like it's unexpected behavior. Also
the use of "copies" implies to me it's doing a copy operation. I would
rephrase it to something like:
When finishing the current section some index records might be written
for the section to the table. The logic to do so is essentially
duplicated from what we already have in `writer_add_record()`, making
this more complicated than it really has to be.
Other than that, I don't have any comments about this patch series.
--
Toon
^ permalink raw reply
* [PATCH] This PR enables a successful git build on z/OS.
From: Haritha via GitGitGadget @ 2024-01-31 14:21 UTC (permalink / raw)
To: git; +Cc: Haritha, Haritha D
From: Haritha D <harithamma.d@ibm.com>
Since the z/OS linker does not support searching dynamic libraries,
and the current setting of CC_LD_DYNPATH results in a directory
to be supplied to the link step with no option as the suffix,
it causes a linker error because the z/OS LD linker
does not accept directories as input.
Therefore, we workaround this by adding the -L option.
And, Introduced z/OS (OS/390) as a platform in config.mak.uname
Signed-off-by: Haritha D <harithamma.d@ibm.com>
---
This PR enables a successful git build on z/OS.
Since the z/OS linker does not support searching dynamic libraries, and
the current setting of CC_LD_DYNPATH results in a directory to be
supplied to the link step with no option as the suffix, it causes a
linker error because the z/OS LD linker does not accept directories as
input. Therefore, we workaround this by adding the -L option. And,
Introduced z/OS (OS/390) as a platform in config.mak.uname
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1663%2FHarithaIBM%2Fzos-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1663/HarithaIBM/zos-v1
Pull-Request: https://github.com/git/git/pull/1663
config.mak.uname | 12 ++++++++++++
configure.ac | 3 +++
2 files changed, 15 insertions(+)
diff --git a/config.mak.uname b/config.mak.uname
index dacc95172dc..c8006f854e5 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
SHELL_PATH = /usr/coreutils/bin/bash
endif
+ifeq ($(uname_S),OS/390)
+ NO_SYS_POLL_H = YesPlease
+ NO_STRCASESTR = YesPlease
+ NO_REGEX = YesPlease
+ NO_MMAP = YesPlease
+ NO_NSEC = YesPlease
+ NO_STRLCPY = YesPlease
+ NO_MEMMEM = YesPlease
+ NO_GECOS_IN_PWENT = YesPlease
+ HAVE_STRINGS_H = YesPlease
+ NEEDS_MODE_TRANSLATION = YesPlease
+endif
ifeq ($(uname_S),MINGW)
ifeq ($(shell expr "$(uname_R)" : '1\.'),2)
$(error "Building with MSys is no longer supported")
diff --git a/configure.ac b/configure.ac
index d1a96da14eb..64569a80d53 100644
--- a/configure.ac
+++ b/configure.ac
@@ -463,6 +463,9 @@ else
CC_LD_DYNPATH=-Wl,+b,
else
CC_LD_DYNPATH=
+ if test "$(uname -s)" = "OS/390"; then
+ CC_LD_DYNPATH=-L
+ fi
AC_MSG_WARN([linker does not support runtime path to dynamic libraries])
fi
fi
base-commit: bc7ee2e5e16f0d1e710ef8fab3db59ab11f2bbe7
--
gitgitgadget
^ permalink raw reply related
* Re: [PATCH 3/5] reftable/writer: simplify writing index records
From: Kristoffer Haugsbakk @ 2024-01-31 15:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <b0982baacf74a4501ce5c543b294bc15d6937875.1706263918.git.ps@pks.im>
Hi
It looks like this isn’t in `next` yet so I’ll leave a comment.
> @@ -405,6 +405,7 @@ static int writer_finish_section(struct reftable_writer *w)
> w->index = NULL;
> w->index_len = 0;
> w->index_cap = 0;
> +
This part and the next quoted one seem to be an unrelated edit by
`clang-format`. They could perhaps be grouped in their own patch.
> - abort();
> - }
> }
> - for (i = 0; i < idx_len; i++) {
> +
> + for (i = 0; i < idx_len; i++)
> strbuf_release(&idx[i].last_key);
> - }
> reftable_free(idx);
> }
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] This PR enables a successful git build on z/OS.
From: Kristoffer Haugsbakk @ 2024-01-31 16:12 UTC (permalink / raw)
To: Josh Soref; +Cc: Haritha, git
In-Reply-To: <pull.1663.git.git.1706710861778.gitgitgadget@gmail.com>
Hi
> [PATCH] This PR enables a successful git build on z/OS.
Maybe the subject could be:
> [PATCH] build: support z/OS (OS/390)
There’s some indirectly related things in `SubmittingPatches`:
“ Describe your changes in imperative mood, e.g. "make xyzzy do frotz"
instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy
to do frotz", as if you are giving orders to the codebase to change
its behavior. Try to make sure your explanation can be understood
without external resources. Instead of giving a URL to a mailing
list archive, summarize the relevant points of the discussion.
From which I infer that “This PR” is considered redundant.
As for the `build:` prefix: many different prefixes have been used for
this file (`git log --no-merges -- config.mak.uname`).
--
Kristoffer Haugsbakk
^ permalink raw reply
* Re: [PATCH] merge-tree: accept 3 trees as arguments
From: Elijah Newren @ 2024-01-31 16:34 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <xmqqbk92bv43.fsf@gitster.g>
On Tue, Jan 30, 2024 at 9:15 AM Junio C Hamano <gitster@pobox.com> wrote:
>
[...]
> >> opt.branch1 = branch1;
> >> opt.branch2 = branch2;
> >
> > If branch1 and branch2 refer to trees, then when users hit conflicts
> > they'll see e.g.
> >
> > <<<<<<< aaaaaaa
> > somecode();
> > =======
> > othercode();
> >>>>>>>> bbbbbbb
> >
> > but aaaaaaa and bbbbbbb are not commits that they can find.
>
> Correct. They are what they fed as two trees to be merged, aren't
> they? They (or the script that drives merge-tree and fed these two
> trees) should be recognise these two trees, as long as they are told
> that is what we show, no?
>
> > That raises the question: if the user passes trees in, should we
> > require helpful names be provided as additional parameters?
>
> If the user wants to give human-readable name to override whatever
> the code would internally produce, such new options may help them,
> and should probably apply equally to input that happens to be a
> commit (or a tag that is a tree-ish) as well, I would think.
>
> > Or are
> > the usecases such that we don't expect callers to have any useful
> > information about where these trees come from and these suboptimal
> > conflicts are the best we can do?
>
> I do not necessarily think the output is "suboptimal" from the point
> of view of our users, exactly because that is what they fed into the
> command themselves.
Yeah, I was worried though that the end user wasn't the one running
the git-merge-tree command, and was trying to dig more into usage
cases. Johannes example of using revision:path specification lessens
my worry, and you are right to point out that we can add additional
options in the future to allow overriding with more human readable
names.
^ permalink raw reply
* Re: [PATCH] merge-tree: accept 3 trees as arguments
From: Elijah Newren @ 2024-01-31 16:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Johannes Schindelin via GitGitGadget, git
In-Reply-To: <557604aa-dea4-995a-4fb2-a71b515a8129@gmx.de>
Hi Johannes,
On Tue, Jan 30, 2024 at 12:04 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Mon, 29 Jan 2024, Elijah Newren wrote:
>
> > On Fri, Jan 26, 2024 at 6:18 AM Johannes Schindelin via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> > >
[...]
> That's funny: I asked Victoria Dye to look over the patch, and she pointed
> out the exact opposite: I had written `<tree>` and she remarked that most
> of Git's manual pages would call this a `<tree-ish>` :-)
A code review isn't complete until you get two contradictory requests, I guess?
> On another funny note, I tried to establish the term "ent" for this category
> almost 222 months ago because I also disliked the "-ish" convention:
> https://lore.kernel.org/git/Pine.LNX.4.63.0508051655480.8418@wgmdd8.biozentrum.uni-wuerzburg.de/
:-)
> > > diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
> > > index 3bdec53fbe5..cbd8e15af6d 100644
> > > --- a/builtin/merge-tree.c
> > > +++ b/builtin/merge-tree.c
> > > @@ -429,35 +429,43 @@ static int real_merge(struct merge_tree_options *o,
> > > struct merge_options opt;
> > >
> > > copy_merge_options(&opt, &o->merge_options);
> > > - parent1 = get_merge_parent(branch1);
> > > - if (!parent1)
> > > - help_unknown_ref(branch1, "merge-tree",
> > > - _("not something we can merge"));
> > > -
> > > - parent2 = get_merge_parent(branch2);
> > > - if (!parent2)
> > > - help_unknown_ref(branch2, "merge-tree",
> > > - _("not something we can merge"));
> > > -
> > > opt.show_rename_progress = 0;
> > >
> > > opt.branch1 = branch1;
> > > opt.branch2 = branch2;
> >
> > If branch1 and branch2 refer to trees, then when users hit conflicts
> > they'll see e.g.
> >
> > <<<<<<< aaaaaaa
> > somecode();
> > =======
> > othercode();
> > >>>>>>> bbbbbbb
> >
> > but aaaaaaa and bbbbbbb are not commits that they can find.
>
> That is true. And it is not totally obvious to many users that they could
> then call `git show aaaaaaa:file` to see the full pre-image on the
> first-parent side.
>
> On the other hand, the label that is shown is precisely what the user
> specified on the command-line.
So this is only for direct use? I was curious if the user was using
some other tool of yours, perhaps even some web GUI, and thus that
something else was controlling what was passed to git-merge-tree.
> For example:
>
> $ git merge-tree --merge-base=v2.42.0:t v2.43.0~11:t v2.43.0~10^2:t
>
> will result in the following conflict markers:
>
> $ git show 021c3ce211:t0091-bugreport.sh
> [...]
> <<<<<<< v2.43.0~11:t
> grep usage output &&
> test_path_is_missing git-bugreport-*
> '
>
> test_expect_success 'incorrect positional arguments abort with usage and hint' '
> test_must_fail git bugreport false 2>output &&
> grep usage output &&
> grep false output &&
> =======
> test_grep usage output &&
> >>>>>>> v2.43.0~10^2:t
> [...]
>
> which I personally find very pleasing output.
Oh, good point -- if users provide trees in the revision:path format
then they still have access to more information about why the change
was made via the revision.
Of course, if users are using the tool directly, presumably they have
access to more information about where those trees came from anyway
even without the conflict label.
> Besides, the manual page of `git merge-tree` says in no sugar-coated
> words:
>
> Do NOT look through the resulting toplevel tree to try to find which
> files conflict; [...]
>
> :-)
Right, but this isn't using the tree to find which files conflict;
they already are looking at the conflict. They are instead wanting to
learn why certain textual changes were made on one side of history to
better inform how to resolve an otherwise less than obvious conflict
resolution. At least, that's the only thing I've seen those conflict
labels be used for.
^ permalink raw reply
* Re: [PATCH] This PR enables a successful git build on z/OS.
From: Junio C Hamano @ 2024-01-31 17:12 UTC (permalink / raw)
To: Haritha via GitGitGadget; +Cc: git, Haritha
In-Reply-To: <pull.1663.git.git.1706710861778.gitgitgadget@gmail.com>
"Haritha via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH] This PR enables a successful git build on z/OS.
Please think with longer term effects in mind when formulating the
title of the commit. What title will your next patch have if we
break the build for z/OS next time, after this fix goes in?
"enables a successful build again"? How would one tell which commit
changed what aspect of the build procedure to adjust to z/OS?
Perhaps
[PATCH] Makefile: adjust for z/OS that lack dynamic library support
or something would be specific enough.
> From: Haritha D <harithamma.d@ibm.com>
>
> Since the z/OS linker does not support searching dynamic libraries,
> and the current setting of CC_LD_DYNPATH results in a directory
> to be supplied to the link step with no option as the suffix,
> it causes a linker error because the z/OS LD linker
> does not accept directories as input.
Hmph, it is not quite clear to me where that "current setting of
CC_LD_DYNPATH" comes from and what exact value it is set to.
Here is my attempt (blind guesses are involved, so please correct
whatever errors you spot):
The autoconf generated configuration gives an empty string to
CC_LD_DYNPATH when it cannot find a way to use a shared library
and gives up with "linker does not support runtime path to
dynamic libraries" message. This leaves the directory path that
is usually appended to -Wl,-rpath, or -R, or whatever alone on
the command line of the linker, e.g. "-L/usr/lib /usr/lib", which
breaks the linker.
Work it around by setting CD_LD_DYNPATH to -L; we will end up
giving the same directory twice, e.g., "-L/usr/lib -L/usr/lib",
but it is only ugly without breaking anything.
While at it, define appropriate settings for z/OS (OS/390) in
the config.mak.uname file.
> Therefore, we workaround this by adding the -L option.
> And, Introduced z/OS (OS/390) as a platform in config.mak.uname
>
> Signed-off-by: Haritha D <harithamma.d@ibm.com>
> ---
> This PR enables a successful git build on z/OS.
>
> Since the z/OS linker does not support searching dynamic libraries, and
> the current setting of CC_LD_DYNPATH results in a directory to be
> supplied to the link step with no option as the suffix, it causes a
> linker error because the z/OS LD linker does not accept directories as
> input. Therefore, we workaround this by adding the -L option. And,
> Introduced z/OS (OS/390) as a platform in config.mak.uname
You do not have to write the same thing twice. The text under "---"
is meant for extra explanation that does not need to become part of
the final commit log message.
> diff --git a/config.mak.uname b/config.mak.uname
> index dacc95172dc..c8006f854e5 100644
> --- a/config.mak.uname
> +++ b/config.mak.uname
> @@ -638,6 +638,18 @@ ifeq ($(uname_S),NONSTOP_KERNEL)
> SANE_TOOL_PATH = /usr/coreutils/bin:/usr/local/bin
> SHELL_PATH = /usr/coreutils/bin/bash
> endif
> +ifeq ($(uname_S),OS/390)
> + NO_SYS_POLL_H = YesPlease
> + NO_STRCASESTR = YesPlease
> + NO_REGEX = YesPlease
> + NO_MMAP = YesPlease
> + NO_NSEC = YesPlease
> + NO_STRLCPY = YesPlease
> + NO_MEMMEM = YesPlease
> + NO_GECOS_IN_PWENT = YesPlease
> + HAVE_STRINGS_H = YesPlease
> + NEEDS_MODE_TRANSLATION = YesPlease
> +endif
I cannot tell if these are reasonable for z/OS myself and I'll take
your word for it ;-) After all you're the expert.
> diff --git a/configure.ac b/configure.ac
> index d1a96da14eb..64569a80d53 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -463,6 +463,9 @@ else
> CC_LD_DYNPATH=-Wl,+b,
> else
> CC_LD_DYNPATH=
> + if test "$(uname -s)" = "OS/390"; then
> + CC_LD_DYNPATH=-L
> + fi
> AC_MSG_WARN([linker does not support runtime path to dynamic libraries])
> fi
> fi
The use of "uname -s" looks totally out of place.
Wouldn't it be a better approach to set it in config.mak.uname for
OS/390 above and leave this part untouched, I wonder?
^ permalink raw reply
* [PATCH 2/2] Makefile: simplify output of the libpath_template
From: Junio C Hamano @ 2024-01-31 17:42 UTC (permalink / raw)
To: git
In-Reply-To: <20240131174220.4160560-1-gitster@pobox.com>
If a platform lacks the support to specify the dynamic library path,
there is no suitable value to give to the CC_LD_DYNPATH variable.
Allow them to be set to an empty string to signal that they do not
need to add the usual -Wl,-rpath, or -R or whatever option followed
by a directory name. This way,
$(call libpath_tempate,$(SOMELIBDIR))
would expand to just a single mention of that directory, i.e.
-L$(SOMELIBDIR)
when CC_LD_DYNPATH is set to an empty string (or a "-L", which
would have repeated the same "-L$(SOMELIBDIR)" twice without any
ill effect).
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* This obviously makes it impossible to set CC_LD_DYNPATH to an
empty string to say "-L /usr/local/lib /usr/local/lib" on the
linker's command line. I do not think it would serve any useful
purpose to be able to have just a directory name on the command
line of the linker there, so it would not regress anything on
exotic platforms, I hope ;-).
shared.mak | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/shared.mak b/shared.mak
index f33cab8a4e..c59cea75a1 100644
--- a/shared.mak
+++ b/shared.mak
@@ -112,5 +112,5 @@ endef
## Getting sick of writing -L$(SOMELIBDIR) $(CC_LD_DYNPATH)$(SOMELIBDIR)?
## Write $(call libpath_template,$(SOMELIBDIR)) instead, perhaps?
define libpath_template
--L$(1) $(CC_LD_DYNPATH)$(1)
+-L$(1) $(if $(filter-out -L,$(CC_LD_DYNPATH)),$(CC_LD_DYNPATH)$(1))
endef
--
2.43.0-493-gbc7ee2e5e1
^ permalink raw reply related
* [PATCH 0/2] CC_LD_DYNPATH improvements
From: Junio C Hamano @ 2024-01-31 17:42 UTC (permalink / raw)
To: git; +Cc: Haritha D
We seem to repeat ourselves many times in Makefile with lines like
this:
EXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)
but we should be able to express ourselves without repeating the
same long string twice, perhaps like so:
EXTLIBS += $(call libpath_template,$(LIBPCREDIR)/$(lib))
I originally wrote this as a practice to use the $(call template)
pattern in Makefile, but it may make porting to a platform without
dynamic library path support simpler.
Junio C Hamano (2):
Makefile: reduce repetitive library paths
Makefile: simplify output of the libpath_template
Makefile | 12 ++++++------
shared.mak | 6 ++++++
2 files changed, 12 insertions(+), 6 deletions(-)
--
2.43.0-493-gbc7ee2e5e1
^ permalink raw reply
* [PATCH 1/2] Makefile: reduce repetitive library paths
From: Junio C Hamano @ 2024-01-31 17:42 UTC (permalink / raw)
To: git
In-Reply-To: <20240131174220.4160560-1-gitster@pobox.com>
When we take a library package we depend on (e.g., LIBPCRE) from a
directory other than the default location of the system, we add the
same directory twice on the linker command like, like so:
EXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)
Introduce a template "libpath_template" that takes the path to the
directory, which can be used like so:
EXTLIBS += $(call libpath_template,$(LIBPCREDIR)/$(lib))
and expand it into the "-L$(DIR) $(CC_LD_DYNPATH)$(DIR)" form.
Hopefully we can reduce the chance of typoes this way.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Makefile | 12 ++++++------
shared.mak | 6 ++++++
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/Makefile b/Makefile
index 03adcb5a48..13f256ca13 100644
--- a/Makefile
+++ b/Makefile
@@ -1575,7 +1575,7 @@ endif
ifdef LIBPCREDIR
BASIC_CFLAGS += -I$(LIBPCREDIR)/include
- EXTLIBS += -L$(LIBPCREDIR)/$(lib) $(CC_LD_DYNPATH)$(LIBPCREDIR)/$(lib)
+ EXTLIBS += $(call libpath_template,$(LIBPCREDIR)/$(lib))
endif
ifdef HAVE_ALLOCA_H
@@ -1595,7 +1595,7 @@ else
ifdef CURLDIR
# Try "-Wl,-rpath=$(CURLDIR)/$(lib)" in such a case.
CURL_CFLAGS = -I$(CURLDIR)/include
- CURL_LIBCURL = -L$(CURLDIR)/$(lib) $(CC_LD_DYNPATH)$(CURLDIR)/$(lib)
+ CURL_LIBCURL = $(call libpath_template,$(CURLDIR)/$(lib))
else
CURL_CFLAGS =
CURL_LIBCURL =
@@ -1631,7 +1631,7 @@ else
ifndef NO_EXPAT
ifdef EXPATDIR
BASIC_CFLAGS += -I$(EXPATDIR)/include
- EXPAT_LIBEXPAT = -L$(EXPATDIR)/$(lib) $(CC_LD_DYNPATH)$(EXPATDIR)/$(lib) -lexpat
+ EXPAT_LIBEXPAT = $(call libpath_template,$(EXPATDIR)/$(lib)) -lexpat
else
EXPAT_LIBEXPAT = -lexpat
endif
@@ -1644,7 +1644,7 @@ IMAP_SEND_LDFLAGS += $(OPENSSL_LINK) $(OPENSSL_LIBSSL) $(LIB_4_CRYPTO)
ifdef ZLIB_PATH
BASIC_CFLAGS += -I$(ZLIB_PATH)/include
- EXTLIBS += -L$(ZLIB_PATH)/$(lib) $(CC_LD_DYNPATH)$(ZLIB_PATH)/$(lib)
+ EXTLIBS += $(call libpath_template,$(ZLIB_PATH)/$(lib))
endif
EXTLIBS += -lz
@@ -1652,7 +1652,7 @@ ifndef NO_OPENSSL
OPENSSL_LIBSSL = -lssl
ifdef OPENSSLDIR
BASIC_CFLAGS += -I$(OPENSSLDIR)/include
- OPENSSL_LINK = -L$(OPENSSLDIR)/$(lib) $(CC_LD_DYNPATH)$(OPENSSLDIR)/$(lib)
+ OPENSSL_LINK = $(call libpath_template,$(OPENSSLDIR)/$(lib))
else
OPENSSL_LINK =
endif
@@ -1679,7 +1679,7 @@ ifndef NO_ICONV
ifdef NEEDS_LIBICONV
ifdef ICONVDIR
BASIC_CFLAGS += -I$(ICONVDIR)/include
- ICONV_LINK = -L$(ICONVDIR)/$(lib) $(CC_LD_DYNPATH)$(ICONVDIR)/$(lib)
+ ICONV_LINK = $(call libpath_template,$(ICONVDIR)/$(lib))
else
ICONV_LINK =
endif
diff --git a/shared.mak b/shared.mak
index aeb80fc4d5..f33cab8a4e 100644
--- a/shared.mak
+++ b/shared.mak
@@ -108,3 +108,9 @@ endif
define mkdir_p_parent_template
$(if $(wildcard $(@D)),,$(QUIET_MKDIR_P_PARENT)$(shell mkdir -p $(@D)))
endef
+
+## Getting sick of writing -L$(SOMELIBDIR) $(CC_LD_DYNPATH)$(SOMELIBDIR)?
+## Write $(call libpath_template,$(SOMELIBDIR)) instead, perhaps?
+define libpath_template
+-L$(1) $(CC_LD_DYNPATH)$(1)
+endef
--
2.43.0-493-gbc7ee2e5e1
^ permalink raw reply related
* Re: [PATCH 1/9] reftable: introduce macros to grow arrays
From: Eric Sunshine @ 2024-01-31 17:44 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <0597e6944a1a65720d050f47bc82766d5bcf860b.1706687982.git.ps@pks.im>
On Wed, Jan 31, 2024 at 3:01 AM Patrick Steinhardt <ps@pks.im> wrote:
> Throughout the reftable library we have many cases where we need to grow
> arrays. In order to avoid too many reallocations, we roughly double the
> capacity of the array on each iteration. The resulting code pattern is
> thus duplicated across many sites.
>
> We have similar patterns in our main codebase, which is why we have
> eventually introduced an `ALLOC_GROW()` macro to abstract it away and
> avoid some code duplication. We cannot easily reuse this macro here
> though because `ALLOC_GROW()` uses `REALLOC_ARRAY()`, which in turn will
> call realloc(3P) to grow the array. The reftable code is structured as a
> library though (even if the boundaries are fuzzy), and one property this
> brings with it is that it is possible to plug in your own allocators. So
> instead of using realloc(3P), we need to use `reftable_realloc()` that
> knows to use the user-provided implementation.
>
> So let's introduce two new macros `REFTABLE_REALLOC_ARRAY()` and
> `REFTABLE_ALLOC_GROW()` that mirror what we do in our main codebase,
> with two modifications:
>
> - They use `reftable_realloc()`, as explained above.
>
> - They use a different growth factor of `2 * cap + 1` instead of `(cap
> + 16) * 3 / 2`.
>
> The second change is because we know a bit more about the allocation
> patterns in the reftable library. For In most cases, we end up only having a
s/For In/In/
> single item in the array, so the initial capacity that our global growth
> factor uses (which is 24), significantly overallocates in a lot of code
Perhaps? s/overallocates/over-allocates/
> paths. This effect is indeed measurable:
> [...]
>
> Convert the reftable library to use these new macros.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
(Neither above comment is worth a reroll.)
^ permalink raw reply
* Re: [PATCH 7/9] reftable/merged: refactor seeking of records
From: Eric Sunshine @ 2024-01-31 17:55 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <00aeaeee63dc1371d3338b72f779c6f165bd14f7.1706687982.git.ps@pks.im>
On Wed, Jan 31, 2024 at 3:02 AM Patrick Steinhardt <ps@pks.im> wrote:
> The code to seek reftable records in the merged table code is quite hard
> to read and does not conform to our coding style in multiple ways:
>
> - We have multiple exit paths where we release resources even though
> that is not really necessary
s/$/./
> - We use a scoped error variable `e` which is hard to reason about.
> This variable is not required at all.
>
> - We allocate memory in the variable declarations, which is easy to
> miss.
>
> Refactor the function so that it becomes more maintainable in the
> future.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
^ permalink raw reply
* Re: Git in GSoC 2024
From: Kaartic Sivaraam @ 2024-01-31 17:57 UTC (permalink / raw)
To: Patrick Steinhardt, Christian Couder
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye
In-Reply-To: <ZbpGzAd6FGEeTdrh@tanuki>
Hi Patrick,
On 31 January 2024 6:40:36 pm IST, Patrick Steinhardt <ps@pks.im> wrote:
>On Tue, Jan 30, 2024 at 10:08:53AM +0100, Patrick Steinhardt wrote:
>> On Tue, Jan 30, 2024 at 09:38:48AM +0100, Christian Couder wrote:
>>
>> Yes, the tests in t0032-reftable-unittest.sh should be ported over to
>> the new unit test framework eventually, and I think that this might be a
>> good GSoC project indeed.
>>
Nice. Good to hear that.
>> If there is interest I'd also be happy to draft up some more topics in
>> the context of refs and the reftable backend. I'm sure there should be
>> some topics here that would be a good fit for the GSoC project, and I'd
>> be happy to mentor any such project in this context.
>
Great. Thanks for your interest in willing to mentor!
I created a fairly rough SoC ideas page for now including a barebones
information about the unit test migration idea:
https://git.github.io/SoC-2024-Ideas/
Note well that the existing idea's description is far from complete and
I mostly just cooked it up to serve as a template for how the idea entry
could look like. Kindly contribute towards elaborating the same :-)
Also, feel free to suggest ideas you have around refs and reftable
backed, Patrick. Those would be helpful.
>I noticed that the starting period falls right into my honeymoon from
>June 17th until July 19th. This unfortunately makes it quite a lot
>harder for me to mentor projects alone. Still, I'd be happy to co-mentor
>or help out in other ways.
>
I too don't believe your vacation is going to be a deal breaker for you
being a mentor. It should be totally fine given that we get a backup
mentor who is also willing to mentor the candidate. (side note: I myself
have no knowledge about refs backends. So, I suppose I might not be able
to help co-mentor this one).
Reg. the timline [1] Jun 17th won't be much of the start. The community
bonding period starts May 1. Many contributors typically start dipping
their toes into their project right away. So, you would have ample time
before the start of the vacation to set the contributor up and going
with the project.
[1]: https://developers.google.com/open-source/gsoc/timeline
--
Sivaraam
^ permalink raw reply
* Re: [PATCH] This PR enables a successful git build on z/OS.
From: Junio C Hamano @ 2024-01-31 17:58 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Josh Soref, Haritha, git
In-Reply-To: <4d0efd0f-f35d-4a3a-aea1-e08cc55bd2d9@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> Hi
>
>> [PATCH] This PR enables a successful git build on z/OS.
>
> Maybe the subject could be:
>
>> [PATCH] build: support z/OS (OS/390)
Nice. This is nicer than what I came up with, that placed too much
stress on the runtime path to the dynamic library. With all the
stuff added to config.mak.uname, yours is much more appropriate.
Thanks.
^ permalink raw reply
* Re: Git in GSoC 2024
From: Kaartic Sivaraam @ 2024-01-31 18:03 UTC (permalink / raw)
To: Patrick Steinhardt, Christian Couder
Cc: git, Taylor Blau, Junio C Hamano, Victoria Dye
In-Reply-To: <c61322de-8cd9-42b8-a04b-a8ae47b25c5e@gmail.com>
Hi,
On 31/01/24 23:27, Kaartic Sivaraam wrote:
>
> Great. Thanks for your interest in willing to mentor!
>
> I created a fairly rough SoC ideas page for now including a barebones
> information about the unit test migration idea:
>
> https://git.github.io/SoC-2024-Ideas/
>
For the note, I also put up the 2024 microprojects page by basically
renaming the 2022 page. I'm supposing the projects are still relevant.
Kindly correct me if that's not the case.
https://git.github.io/SoC-2024-Microprojects/
Also, feel free to suggest other microproject ideas that you may have.
--
Sivaraam
^ permalink raw reply
* Re: [PATCH] merge-tree: accept 3 trees as arguments
From: Junio C Hamano @ 2024-01-31 18:14 UTC (permalink / raw)
To: Elijah Newren
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <CABPp-BEDLsVFxjr13XJX9eBLeqD+tRoeHxwJyPsc_+AgGY2GTw@mail.gmail.com>
Elijah Newren <newren@gmail.com> writes:
> On Tue, Jan 30, 2024 at 9:15 AM Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>> > but aaaaaaa and bbbbbbb are not commits that they can find.
>>
>> Correct. They are what they fed as two trees to be merged, aren't
>> they? They (or the script that drives merge-tree and fed these two
>> trees) should be recognise these two trees, as long as they are told
I notice "able to" is missing here...
>> that is what we show, no?
> ...
> Yeah, I was worried though that the end user wasn't the one running
> the git-merge-tree command, and was trying to dig more into usage
> cases.
Sure. That is exactly why I wrote "They (or the script that
drives..." above.
^ permalink raw reply
* Re: [PATCH v3 02/10] trailer: move interpret_trailers() to interpret-trailers.c
From: Junio C Hamano @ 2024-01-31 18:54 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <cafa39d10489f90897227a244e012457989a7710.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> The interpret-trailers.c builtin is the only place we need to call
> interpret_trailers(), so move its definition there.
A few helper functions that are only called by interpret_trailers()
are also moved, naturally. I would have less surprised to see
the addtion near the beginning of builtin/interpret-trailers.c if
this part said:
..., so move its definition there, together with a few helper
functions called only by it, and remove its external declaration
from <trailer.h>.
> Delete the corresponding declaration from trailer.h, which then forces
> us to expose the working innards of that function.
This was a bit confusing, at least to me. The reason why several
other helper functions that are called by interpret_trailers() need
to be declared in trailer.h is not because the declaration of
interpret_trailers() is deleted from trailer.h but that is how I
read the above.
Several helper functions that are called by interpret_trailers()
remain in trailer.c because other callers in the same file still
call them, so add declaration for them to <trailer.h>.
> This enriches
> trailer.h with a more granular API, which can then be unit-tested in the
> future (because interpret_trailers() by itself does too many things to
> be able to be easily unit-tested).
>
> Take this opportunity to demote some file-handling functions out of the
> trailer API implementation, as these have nothing to do with trailers.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/interpret-trailers.c | 94 +++++++++++++++++++++++++++
> trailer.c | 120 ++++-------------------------------
> trailer.h | 20 +++++-
Together with the preparatory renaming in the previous step, this
made quite a pleasant read. Thanks.
^ permalink raw reply
* [PATCH] ci: update FreeBSD cirrus job
From: Carlo Marcelo Arenas Belón @ 2024-01-31 19:13 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Carlo Marcelo Arenas Belón
FreeBSD 12 is EOL and no longer available, causing errors in this job.
Upgrade to 13.2, which is the next oldest release with support and that
should keep it for at least another 4 months.
This will be upgraded again once 13.3 is released to avoid furtheer
surprises.
"Not enough compute credits to prioritize tasks!" seems to be just a
reminder that the credit allocate for the Free Tier by Cirrus is all
used up and which might result in additional delays getting a result.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
.cirrus.yml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/.cirrus.yml b/.cirrus.yml
index b6280692d2..77346a4929 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -1,7 +1,7 @@
env:
CIRRUS_CLONE_DEPTH: 1
-freebsd_12_task:
+freebsd_task:
env:
GIT_PROVE_OPTS: "--timer --jobs 10"
GIT_TEST_OPTS: "--no-chain-lint --no-bin-wrappers"
@@ -9,7 +9,7 @@ freebsd_12_task:
DEFAULT_TEST_TARGET: prove
DEVELOPER: 1
freebsd_instance:
- image_family: freebsd-12-3
+ image_family: freebsd-13-2
memory: 2G
install_script:
pkg install -y gettext gmake perl5
--
2.39.3 (Apple Git-145)
^ permalink raw reply related
* Re: [PATCH 2/2] t/Makefile: get UNIT_TESTS list from C sources
From: Adam Dinwoodie @ 2024-01-31 19:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Patrick Steinhardt, git, Phillip Wood
In-Reply-To: <xmqqeddzfywg.fsf@gitster.g>
On Tue, 30 Jan 2024 at 00:28, Junio C Hamano <gitster@pobox.com> wrote:
>
> Adam Dinwoodie <adam@dinwoodie.org> writes:
>
> >> Hmm, good point. It seems like the answer should obviously be "yes", but
> >> Windows CI seemed to pass all the same (and I checked that it indeed ran
> >> the unit tests). Do we only get the $X suffix for MSVC builds or
> >> something? Looks like maybe cygwin, as well.
> >
> > Cygwin will automatically append ".exe" when doing directory listings;
> > a check if the file "a" exists will return true on Cygwin if "a" or
> > "a.exe" exists; a glob for "a*" in a directory containing files "a1"
> > and "a2.exe" will return "a1" and "a2". This causes problems in some
> > edge cases, but it means *nix scripts and applications are much more
> > likely to work without any Cygwin-specific handling. I *think* this
> > logic is carried downstream to MSYS2 and thence to Git for Windows.
>
> Interesting, especially that "a*" is globbed to "a2" and not
> "a2.exe".
My error, sorry! I've just double-checked and Cygwin's globbing will
report the file with the .exe extension. I clearly misremembered how
this works.
Having looked up a bit more of the implementation is simply that, if
Cygwin tries to open a file named "x" and doesn't find it, it will
attempt to open "x.exe" before it returns the failure. This means that
scripts that call (say) `/usr/bin/env bash` or `cat` or `[ "$x" = "$y"
]` or whatever will broadly Just Work(TM) rather than needing to be
rewritten with the extension added. But the behaviour only applies
when Cygwin is looking for a specific filename.
^ permalink raw reply
* Re: FreeBSD CI suspended on git/git and gitgitgadget/git
From: Carlo Marcelo Arenas Belón @ 2024-01-31 19:23 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <d2d7da84-e2a3-a7b2-3f95-c8d53ad4dd5f@gmx.de>
On Wed, Jan 31, 2024 at 08:11:46AM +0100, Johannes Schindelin wrote:
> Team,
>
> I noticed that there is a problem with the FreeBSD runs on Cirrus CI (see
> e.g. https://cirrus-ci.com/task/6611218006278144):
Thanks and sorry for not catching this earlier.
The proposed[1] "fix" runs successful and might allow (albeit with the
warning about prioritization) for this job to be restarted:
https://cirrus-ci.com/task/6173598017126400
As explained there, FreeBSD 12 has been EOL for a while and therefore this
update was long overdue and will need further tweaks.
Carlo
[1] https://lore.kernel.org/git/20240131191325.33228-1-carenas@gmail.com/
^ permalink raw reply
* Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Josh Steadmon @ 2024-01-31 20:02 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Junio C Hamano, Emily Shaffer,
Randall S. Becker, Linus Arver
In-Reply-To: <5c7a2354df0f4a29841f9ab8294ead0e1c3b9cf5.1706664145.git.gitgitgadget@gmail.com>
On 2024.01.31 01:22, Linus Arver via GitGitGadget wrote:
> This unification will allow us to delete the format_trailer_info() and
> print_tok_val() functions in the next patch. They are not deleted here
> in order to keep the diff small.
Needs to be removed after squashing v2 patch 4 :)
^ permalink raw reply
* Re: [PATCH v3 03/10] trailer: unify trailer formatting machinery
From: Junio C Hamano @ 2024-01-31 20:13 UTC (permalink / raw)
To: Linus Arver via GitGitGadget
Cc: git, Christian Couder, Emily Shaffer, Josh Steadmon,
Randall S. Becker, Linus Arver
In-Reply-To: <5c7a2354df0f4a29841f9ab8294ead0e1c3b9cf5.1706664145.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> Currently have two functions for formatting trailers exposed in
> trailer.h:
>
> void format_trailers(FILE *outfile, struct list_head *head,
> const struct process_trailer_options *opts);
>
> void format_trailers_from_commit(struct strbuf *out, const char *msg,
> const struct process_trailer_options *opts);
>
> and previously these functions, although similar enough (even taking the
> same process_trailer_options struct pointer), did not build on each
> other.
>
> Make format_trailers_from_commit() rely on format_trailers(). Teach
> format_trailers() to process trailers with the additional
> process_trailer_options fields like opts->key_only which is only used by
> format_trailers_from_commit() and not builtin/interpret-trailers.c.
> While we're at it, reorder parameters to put the trailer processing
> options first, and the out parameter (strbuf we write into) at the end.
>
> This unification will allow us to delete the format_trailer_info() and
> print_tok_val() functions in the next patch. They are not deleted here
> in order to keep the diff small.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
I am not sure how I helped this particularly (say, compared to all
the other patches on the list from everybody), though.
The updated format_trailers() does so much more than the original
and does so quite differently, it is hard to tell if the existing
caller is getting the same behaviour (modulo obviously it now needs
to prepare a strbuf and print the resulting string in the strbuf,
which is expected) as before.
The conversion for the sole existing caller looks like this, ...
> LIST_HEAD(head);
> struct strbuf sb = STRBUF_INIT;
> + struct strbuf trailer_block = STRBUF_INIT;
> struct trailer_info info;
> ...
> - format_trailers(opts, &head, outfile);
> + /* Print trailer block. */
> + format_trailers(opts, &head, &trailer_block);
> + fwrite(trailer_block.buf, 1, trailer_block.len, outfile);
> + strbuf_release(&trailer_block);
... and it is very straight-forward.
But what it called was this:
> -static void print_tok_val(FILE *outfile, const char *tok, const char *val)
> -{
> - char c;
> -
> - if (!tok) {
> - fprintf(outfile, "%s\n", val);
> - return;
> - }
> -
> - c = last_non_space_char(tok);
> - if (!c)
> - return;
> - if (strchr(separators, c))
> - fprintf(outfile, "%s%s\n", tok, val);
> - else
> - fprintf(outfile, "%s%c %s\n", tok, separators[0], val);
> -}
> -
> -void format_trailers(const struct process_trailer_options *opts,
> - struct list_head *trailers, FILE *outfile)
> -{
> - struct list_head *pos;
> - struct trailer_item *item;
> - list_for_each(pos, trailers) {
> - item = list_entry(pos, struct trailer_item, list);
> - if ((!opts->trim_empty || strlen(item->value) > 0) &&
> - (!opts->only_trailers || item->token))
> - print_tok_val(outfile, item->token, item->value);
> - }
> -}
i.e. iterate over trailers, and sometimes call print_tok_val(),
which does not do all that much. Print only the val, or when tok is
not empty, show "tok: val" while taking into account the possibility
that the last byte of tok may already be the separator.
But the new thing is way more complex.
> +void format_trailers(const struct process_trailer_options *opts,
> + struct list_head *trailers,
> + struct strbuf *out)
> +{
> + struct list_head *pos;
> + struct trailer_item *item;
> + int need_separator = 0;
> +
> + list_for_each(pos, trailers) {
> + item = list_entry(pos, struct trailer_item, list);
> + if (item->token) {
> + char c;
> +
> + struct strbuf tok = STRBUF_INIT;
> + struct strbuf val = STRBUF_INIT;
> + strbuf_addstr(&tok, item->token);
> + strbuf_addstr(&val, item->value);
> +
> + /*
> + * Skip key/value pairs where the value was empty. This
> + * can happen from trailers specified without a
> + * separator, like `--trailer "Reviewed-by"` (no
> + * corresponding value).
> + */
> + if (opts->trim_empty && !strlen(item->value))
> + continue;
OK, this matches the first condition to stay silent in the original.
> + if (!opts->filter || opts->filter(&tok, opts->filter_data)) {
For the original caller of format_trailers(), which is the
interpret_trailers(), .filter member is never set, so we always come
here, I presume. cmd_interpret_trailers(), before calling
interpret_trailers() could affect the following members:
.in_place .trim_empty .only_trailers .only_input .unfold .no_divider
so we should make sure this new implementation does not change the
behaviour from the original with various combination of these
options.
> + if (opts->unfold)
> + unfold_value(&val);
So, ... how would this affect an invocation of
$ git interpret-trailers --unfold
which would have set opts.unfold to true in cmd_interpret_trailers()
and eventually called process_trailers() with it, which eventually
called print_all(), which conditionally called print_tok_val() but
never gave the val to unfold_value()?
... goes and digs a bit more ...
Ahhhh, original process_trailers() did this by calling
parse_trailers() that munged the value. So its print_all()
codepath only had to print what has already been munged.
But then in this new code, do we unfold only here? I thought that
in the new code you moved around, the caller, whose name is now
interpret_trailers(), still calls parse_trailers() and that calls
the unfold_value(). So, are we doing redundant work that may merely
be unneeded (if we are lucky) or could be harmful (if things, other
than the unfold thing I just noticed, that are done both in
parse_trailers() and this function are not idempotent)?
It could be that a bit more preparatory refactoring would clarify.
I dunno.
> + if (opts->separator && need_separator)
> + strbuf_addbuf(out, opts->separator);
> + if (!opts->value_only)
> + strbuf_addbuf(out, &tok);
> + if (!opts->key_only && !opts->value_only) {
> + if (opts->key_value_separator)
> + strbuf_addbuf(out, opts->key_value_separator);
> + else {
> + c = last_non_space_char(tok.buf);
> + if (c) {
> + if (!strchr(separators, c))
> + strbuf_addf(out, "%c ", separators[0]);
> + }
> + }
> + }
> + if (!opts->key_only)
> + strbuf_addbuf(out, &val);
> + if (!opts->separator)
> + strbuf_addch(out, '\n');
> +
> + need_separator = 1;
> + }
> +
> + strbuf_release(&tok);
> + strbuf_release(&val);
> + } else if (!opts->only_trailers) {
> + if (opts->separator && need_separator) {
> + strbuf_addbuf(out, opts->separator);
> + }
> + strbuf_addstr(out, item->value);
> + if (opts->separator)
> + strbuf_rtrim(out);
> + else
> + strbuf_addch(out, '\n');
> +
> + need_separator = 1;
> + }
> +
> + }
> +}
^ permalink raw reply
* Re: FreeBSD CI suspended on git/git and gitgitgadget/git
From: Junio C Hamano @ 2024-01-31 20:24 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: Johannes Schindelin, git
In-Reply-To: <22cfob7wthmveupp5w7dbdtbparybcsdagoitwneqw6f2cmhs5@x3tnbbftvtyw>
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
> On Wed, Jan 31, 2024 at 08:11:46AM +0100, Johannes Schindelin wrote:
>> Team,
>>
>> I noticed that there is a problem with the FreeBSD runs on Cirrus CI (see
>> e.g. https://cirrus-ci.com/task/6611218006278144):
>
> Thanks and sorry for not catching this earlier.
>
> The proposed[1] "fix" runs successful and might allow (albeit with the
> warning about prioritization) for this job to be restarted:
>
> https://cirrus-ci.com/task/6173598017126400
>
> As explained there, FreeBSD 12 has been EOL for a while and therefore this
> update was long overdue and will need further tweaks.
>
> Carlo
>
> [1] https://lore.kernel.org/git/20240131191325.33228-1-carenas@gmail.com/
Thanks for reporting and a quick fix. Kudos to both of you.
^ permalink raw reply
* Re: [PATCH 1/9] reftable: introduce macros to grow arrays
From: Junio C Hamano @ 2024-01-31 20:35 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git
In-Reply-To: <0597e6944a1a65720d050f47bc82766d5bcf860b.1706687982.git.ps@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
> patterns in the reftable library. For In most cases, we end up only having a
> single item in the array, so the initial capacity that our global growth
> factor uses (which is 24), significantly overallocates in a lot of code
> paths.
You need to know not just that you very often initially have only
one but you rarely grow it beyond 3, or something like that to
explain "significantly overallocates", though.
> This effect is indeed measurable:
And measuring is very good, but I somehow expected that you would
measure not the time (if you often under-allocate and end up
reallocating too many times, it might consume more time, though) but
the peak memory usage. I cannot quite tell what to think of that 2%
time difference.
> Convert the reftable library to use these new macros.
In any case, the conversion shortens the code and is a good thing.
I wish we had a way to tell these macros that we are actually using
the same single allocator (which we are doing in our code when
linking the reftable thing to us anyway), which would have made this
even simpler because you did not have to introduce separate macros..
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> reftable/basics.c | 8 ++------
> reftable/basics.h | 11 +++++++++++
> reftable/block.c | 7 +------
> reftable/merged_test.c | 20 ++++++--------------
> reftable/pq.c | 8 ++------
> reftable/stack.c | 29 ++++++++++++-----------------
> reftable/writer.c | 14 ++------------
> 7 files changed, 36 insertions(+), 61 deletions(-)
>
> diff --git a/reftable/basics.c b/reftable/basics.c
> index f761e48028..af9004cec2 100644
> --- a/reftable/basics.c
> +++ b/reftable/basics.c
> @@ -89,17 +89,13 @@ void parse_names(char *buf, int size, char ***namesp)
> next = end;
> }
> if (p < next) {
> - if (names_len == names_cap) {
> - names_cap = 2 * names_cap + 1;
> - names = reftable_realloc(
> - names, names_cap * sizeof(*names));
> - }
> + REFTABLE_ALLOC_GROW(names, names_len + 1, names_cap);
> names[names_len++] = xstrdup(p);
> }
> p = next + 1;
> }
>
> - names = reftable_realloc(names, (names_len + 1) * sizeof(*names));
> + REFTABLE_REALLOC_ARRAY(names, names_len + 1);
> names[names_len] = NULL;
> *namesp = names;
> }
> diff --git a/reftable/basics.h b/reftable/basics.h
> index 096b36862b..2f855cd724 100644
> --- a/reftable/basics.h
> +++ b/reftable/basics.h
> @@ -53,6 +53,17 @@ void *reftable_realloc(void *p, size_t sz);
> void reftable_free(void *p);
> void *reftable_calloc(size_t sz);
>
> +#define REFTABLE_REALLOC_ARRAY(x, alloc) (x) = reftable_realloc((x), st_mult(sizeof(*(x)), (alloc)))
> +#define REFTABLE_ALLOC_GROW(x, nr, alloc) \
> + do { \
> + if ((nr) > alloc) { \
> + alloc = 2 * (alloc) + 1; \
> + if (alloc < (nr)) \
> + alloc = (nr); \
> + REFTABLE_REALLOC_ARRAY(x, alloc); \
> + } \
> + } while (0)
> +
> /* Find the longest shared prefix size of `a` and `b` */
> struct strbuf;
> int common_prefix_size(struct strbuf *a, struct strbuf *b);
> diff --git a/reftable/block.c b/reftable/block.c
> index 1df3d8a0f0..6952d0facf 100644
> --- a/reftable/block.c
> +++ b/reftable/block.c
> @@ -51,12 +51,7 @@ static int block_writer_register_restart(struct block_writer *w, int n,
> if (2 + 3 * rlen + n > w->block_size - w->next)
> return -1;
> if (is_restart) {
> - if (w->restart_len == w->restart_cap) {
> - w->restart_cap = w->restart_cap * 2 + 1;
> - w->restarts = reftable_realloc(
> - w->restarts, sizeof(uint32_t) * w->restart_cap);
> - }
> -
> + REFTABLE_ALLOC_GROW(w->restarts, w->restart_len + 1, w->restart_cap);
> w->restarts[w->restart_len++] = w->next;
> }
>
> diff --git a/reftable/merged_test.c b/reftable/merged_test.c
> index 46908f738f..e05351e035 100644
> --- a/reftable/merged_test.c
> +++ b/reftable/merged_test.c
> @@ -231,14 +231,10 @@ static void test_merged(void)
> while (len < 100) { /* cap loops/recursion. */
> struct reftable_ref_record ref = { NULL };
> int err = reftable_iterator_next_ref(&it, &ref);
> - if (err > 0) {
> + if (err > 0)
> break;
> - }
> - if (len == cap) {
> - cap = 2 * cap + 1;
> - out = reftable_realloc(
> - out, sizeof(struct reftable_ref_record) * cap);
> - }
> +
> + REFTABLE_ALLOC_GROW(out, len + 1, cap);
> out[len++] = ref;
> }
> reftable_iterator_destroy(&it);
> @@ -368,14 +364,10 @@ static void test_merged_logs(void)
> while (len < 100) { /* cap loops/recursion. */
> struct reftable_log_record log = { NULL };
> int err = reftable_iterator_next_log(&it, &log);
> - if (err > 0) {
> + if (err > 0)
> break;
> - }
> - if (len == cap) {
> - cap = 2 * cap + 1;
> - out = reftable_realloc(
> - out, sizeof(struct reftable_log_record) * cap);
> - }
> +
> + REFTABLE_ALLOC_GROW(out, len + 1, cap);
> out[len++] = log;
> }
> reftable_iterator_destroy(&it);
> diff --git a/reftable/pq.c b/reftable/pq.c
> index dcefeb793a..2461daf5ff 100644
> --- a/reftable/pq.c
> +++ b/reftable/pq.c
> @@ -75,13 +75,9 @@ void merged_iter_pqueue_add(struct merged_iter_pqueue *pq, const struct pq_entry
> {
> int i = 0;
>
> - if (pq->len == pq->cap) {
> - pq->cap = 2 * pq->cap + 1;
> - pq->heap = reftable_realloc(pq->heap,
> - pq->cap * sizeof(struct pq_entry));
> - }
> -
> + REFTABLE_ALLOC_GROW(pq->heap, pq->len + 1, pq->cap);
> pq->heap[pq->len++] = *e;
> +
> i = pq->len - 1;
> while (i > 0) {
> int j = (i - 1) / 2;
> diff --git a/reftable/stack.c b/reftable/stack.c
> index bf3869ce70..1dfab99e96 100644
> --- a/reftable/stack.c
> +++ b/reftable/stack.c
> @@ -551,7 +551,7 @@ struct reftable_addition {
> struct reftable_stack *stack;
>
> char **new_tables;
> - int new_tables_len;
> + size_t new_tables_len, new_tables_cap;
> uint64_t next_update_index;
> };
>
> @@ -602,8 +602,9 @@ static int reftable_stack_init_addition(struct reftable_addition *add,
>
> static void reftable_addition_close(struct reftable_addition *add)
> {
> - int i = 0;
> struct strbuf nm = STRBUF_INIT;
> + size_t i;
> +
> for (i = 0; i < add->new_tables_len; i++) {
> stack_filename(&nm, add->stack, add->new_tables[i]);
> unlink(nm.buf);
> @@ -613,6 +614,7 @@ static void reftable_addition_close(struct reftable_addition *add)
> reftable_free(add->new_tables);
> add->new_tables = NULL;
> add->new_tables_len = 0;
> + add->new_tables_cap = 0;
>
> delete_tempfile(&add->lock_file);
> strbuf_release(&nm);
> @@ -631,8 +633,8 @@ int reftable_addition_commit(struct reftable_addition *add)
> {
> struct strbuf table_list = STRBUF_INIT;
> int lock_file_fd = get_tempfile_fd(add->lock_file);
> - int i = 0;
> int err = 0;
> + size_t i;
>
> if (add->new_tables_len == 0)
> goto done;
> @@ -660,12 +662,12 @@ int reftable_addition_commit(struct reftable_addition *add)
> }
>
> /* success, no more state to clean up. */
> - for (i = 0; i < add->new_tables_len; i++) {
> + for (i = 0; i < add->new_tables_len; i++)
> reftable_free(add->new_tables[i]);
> - }
> reftable_free(add->new_tables);
> add->new_tables = NULL;
> add->new_tables_len = 0;
> + add->new_tables_cap = 0;
>
> err = reftable_stack_reload_maybe_reuse(add->stack, 1);
> if (err)
> @@ -792,11 +794,9 @@ int reftable_addition_add(struct reftable_addition *add,
> goto done;
> }
>
> - add->new_tables = reftable_realloc(add->new_tables,
> - sizeof(*add->new_tables) *
> - (add->new_tables_len + 1));
> - add->new_tables[add->new_tables_len] = strbuf_detach(&next_name, NULL);
> - add->new_tables_len++;
> + REFTABLE_ALLOC_GROW(add->new_tables, add->new_tables_len + 1,
> + add->new_tables_cap);
> + add->new_tables[add->new_tables_len++] = strbuf_detach(&next_name, NULL);
> done:
> if (tab_fd > 0) {
> close(tab_fd);
> @@ -1367,17 +1367,12 @@ static int stack_check_addition(struct reftable_stack *st,
> while (1) {
> struct reftable_ref_record ref = { NULL };
> err = reftable_iterator_next_ref(&it, &ref);
> - if (err > 0) {
> + if (err > 0)
> break;
> - }
> if (err < 0)
> goto done;
>
> - if (len >= cap) {
> - cap = 2 * cap + 1;
> - refs = reftable_realloc(refs, cap * sizeof(refs[0]));
> - }
> -
> + REFTABLE_ALLOC_GROW(refs, len + 1, cap);
> refs[len++] = ref;
> }
>
> diff --git a/reftable/writer.c b/reftable/writer.c
> index ee4590e20f..4483bb21c3 100644
> --- a/reftable/writer.c
> +++ b/reftable/writer.c
> @@ -200,12 +200,7 @@ static void writer_index_hash(struct reftable_writer *w, struct strbuf *hash)
> return;
> }
>
> - if (key->offset_len == key->offset_cap) {
> - key->offset_cap = 2 * key->offset_cap + 1;
> - key->offsets = reftable_realloc(
> - key->offsets, sizeof(uint64_t) * key->offset_cap);
> - }
> -
> + REFTABLE_ALLOC_GROW(key->offsets, key->offset_len + 1, key->offset_cap);
> key->offsets[key->offset_len++] = off;
> }
>
> @@ -674,12 +669,7 @@ static int writer_flush_nonempty_block(struct reftable_writer *w)
> if (err < 0)
> return err;
>
> - if (w->index_cap == w->index_len) {
> - w->index_cap = 2 * w->index_cap + 1;
> - w->index = reftable_realloc(
> - w->index,
> - sizeof(struct reftable_index_record) * w->index_cap);
> - }
> + REFTABLE_ALLOC_GROW(w->index, w->index_len + 1, w->index_cap);
>
> ir.offset = w->next;
> strbuf_reset(&ir.last_key);
^ 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