* Re: [PATCH v4 11/15] replay: use standard revision ranges
From: Linus Arver @ 2023-09-08 22:55 UTC (permalink / raw)
To: Christian Couder, git
Cc: Junio C Hamano, Patrick Steinhardt, Johannes Schindelin,
Elijah Newren, John Cai, Derrick Stolee, Phillip Wood, Calvin Wan,
Toon Claes, Christian Couder
In-Reply-To: <20230907092521.733746-12-christian.couder@gmail.com>
Hi Christian,
I am only reviewing the docs. To assume the mindset of a Git user
unfamiliar with this command, I purposely did not read the cover letter
until after this review was done.
Christian Couder <christian.couder@gmail.com> writes:
> [...]
>
> diff --git a/Documentation/git-replay.txt b/Documentation/git-replay.txt
> new file mode 100644
> index 0000000000..9a2087b01a
> --- /dev/null
> +++ b/Documentation/git-replay.txt
> @@ -0,0 +1,90 @@
> +git-replay(1)
> +=============
> +
> +NAME
> +----
> +git-replay - Replay commits on a different base, without touching working tree
How about using the same language ("new location") as in the DESCRIPTION
heading? Also, the "without touching working tree" part is incomplete
because as explained later on, the index and refs are also left alone.
How about just "safely"?
git-replay - Replay commits onto a new location, safely
> +SYNOPSIS
> +--------
> +[verse]
> +'git replay' --onto <newbase> <revision-range>...
> +
> +DESCRIPTION
> +-----------
> +
> +Takes a range of commits, and replays them onto a new location.
OK.
> Does
> +not touch the working tree or index, and does not update any
> +references.
How about this version?
The new commits are created without modifying the working tree,
index, or any references.
Also, by "references" we mean the refs in ".git/refs/*". In the
gitrevisions man page we use the term "refnames" to refer to these bits,
so maybe "refnames" is better than "references"? The simpler "branches"
is another option.
> However, the output of this command is meant to be used
> +as input to `git update-ref --stdin`, which would update the relevant
> +branches.
Before we get to this sentence, it would be great to explain why this
command is useful (what problem does it solve)?
Also, it would help to add "(see OUTPUT section below)" as a
navigational aid in case some readers are wondering what the output
looks like (and have not yet gotten to that section).
I've noticed that you're using the phrase "starting point" in the
OPTIONS section. I think this is better than "location" or "base" (FWIW
we started using "starting point" in 0a02ca2383 (SubmittingPatches:
simplify guidance for choosing a starting point, 2023-07-14)).
The following is a version of this section that attempts to address my
comments above (I've included the other bits already reviewed earlier to
make it easier to read):
Takes a range of commits, and replays them onto a new starting
point. The new commits are created without modifying the working
tree, index, or any branches. If there are branches that point to
the commits in <revision-range>, list them in the output with both
the original commit hash and the corresponding (replayed) commit
hash (see OUTPUT section below) .
This command is like linkgit:git-rebase[1], but notably does not
require a working tree or index. This means you can run this command
in a bare repo (useful for server-side environments). And because
nothing is modified (only new commits are created), it's like a "dry
run" rebase.
By combining this command with `git update-ref --stdin`, the
relevant branches can be updated. That is, the branches that were
pointing originally to the commits given in the <revision-range>
will be updated to point to the replayed commits. This is similar to
the way how `git rebase --update-refs` updates multiple branches in
the affected range.
> +THIS COMMAND IS EXPERIMENTAL. THE BEHAVIOR MAY CHANGE.
> +
> +OPTIONS
> +-------
> +
> +--onto <newbase>::
How about 'starting-point' instead of 'newbase'?
> + Starting point at which to create the new commits. May be any
> + valid commit, and not just an existing branch name.
Add "See linkgit:gitrevisions[7]." at the end?
> +The update-ref command(s) in the output will update the branch(es) in
> +the revision range to point at the new commits, similar to the way how
> +`git rebase --update-refs` updates multiple branches in the affected
> +range.
Ah, good example. I've moved this to my larger example above, so I don't
think this paragraph is needed here any more (it probably didn't belong
in OPTIONS anyway).
> +<revision-range>::
> + Range of commits to replay; see "Specifying Ranges" in
> + linkgit:git-rev-parse.
OK.
> +OUTPUT
> +------
> +
> +When there are no conflicts, the output of this command is usable as
> +input to `git update-ref --stdin`.
What happens if there are conflicts? Probably good to mention in the
DISCUSSION section. Some questions you may want to answer for the
reader:
(1) Is git-replay an all-or-nothing operation? That is, if there are any
conflicts, is the output always empty (or do we still output those
branches that *can* be updated without conflicts)?
(2) What is meant by "conflict" for git-replay? Is it the same meaning
as the case for git-rebase?
For (1), in your cover letter under "# Important limitations" you say
"No resumability" but I am not sure if this means git-replay will output
*something* before exiting with an error, or simply nothing at all.
Speaking of the limitations section, perhaps it's worth pointing those
out under DISCUSSION as well?
> It is basically of the form:
Why "basically"? Are there cases where the output can be different than
the example given below? If not, then perhaps drop the word "basically"?
> + update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
> + update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
> + update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
> +
> +where the number of refs updated depends on the arguments passed and
> +the shape of the history being replayed.
Let's use "number of branches" instead of "number of refs" here to be
consistent with the language elsewhere.
> +EXIT STATUS
> +-----------
> +
> +For a successful, non-conflicted replay, the exit status is 0. When
> +the replay has conflicts, the exit status is 1.
OK.
> If the replay is not
> +able to complete (or start) due to some kind of error, the exit status
> +is something other than 0 or 1.
Not sure how useful "due to some kind of error" is here --- presumably
the inability to replay is always due to some kind of error.
Would it be worth including a brief explanation about why git-replay
might be unable to complete or start (is this behavior in practice a
common-enough thing to document here)?
> +EXAMPLES
> +--------
> +
> +To simply rebase mybranch onto target:
Looking at the CLI arguments, I think this phrase should be:
Replay the commits in `origin/main..mybranch` onto `target`:
> +------------
> +$ git replay --onto target origin/main..mybranch
> +update refs/heads/mybranch ${NEW_mybranch_HASH} ${OLD_mybranch_HASH}
> +------------
> +
> +When calling `git replay`, one does not need to specify a range of
> +commits to replay using the syntax `A..B`; any range expression will
> +do:
> +
> +------------
> +$ git replay --onto origin/main ^base branch1 branch2 branch3
Instead of `base`, how about `olderbranch`?
> +update refs/heads/branch1 ${NEW_branch1_HASH} ${OLD_branch1_HASH}
> +update refs/heads/branch2 ${NEW_branch2_HASH} ${OLD_branch2_HASH}
> +update refs/heads/branch3 ${NEW_branch3_HASH} ${OLD_branch3_HASH}
> +------------
> +
> +This will simultaneously rebase branch1, branch2, and branch3 -- all
> +commits they have since base, playing them on top of origin/main.
How about
This will rebase the commits in `branch1`, `branch2`, and `branch3`
(excluding those in `base`), preplaying them on top of `origin/main`.
> +These three branches may have commits on top of base that they have in
> +common, but that does not need to be the case.
s/base/`base`
> +GIT
> +---
> +Part of the linkgit:git[1] suite
One question I do have is what happens if you run git-replay twice
(successfully)? Does the second invocation create another set of (new)
successfully replayed commits? I ask because I'm interested in informing
the readers of our docs about any potential pitfalls from abusing this
command by mistake.
^ permalink raw reply
* Re: [PATCH] grep: use OPT_INTEGER_F for --max-depth
From: René Scharfe @ 2023-09-08 22:28 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20230907204027.GA941945@coredump.intra.peff.net>
Am 07.09.23 um 22:40 schrieb Jeff King:
> On Thu, Sep 07, 2023 at 10:20:53PM +0200, René Scharfe wrote:
>
>> Am 05.09.23 um 09:21 schrieb Jeff King:
>>> On Sat, Sep 02, 2023 at 08:54:54PM +0200, René Scharfe wrote:
>>>
>>> In general, I wonder how many of the results from:
>>>
>>> git grep '{ OPTION'
>>>
>>> could be converted to use the macros and end up more readable. There are
>>> a number of OPTARG ones, which I guess can't use macros. Looks like
>>> there are a handful of others (mostly for OPT_HIDDEN).
>>
>> Indeed, and I have a semantic patch for that, but mostly because the
>> macros allow injecting a type check.
>>
>> OPTARG would need a new macro to allow specifying the default value. Or
>> is there a variadic macro trick that we could use?
>
> Hmm, I had just assumed OPTARG was a lost cause (or we would need an
> "OPTARG" variant of each macro; yuck).
Only for OPT_INTEGER and OPT_STRING AFAICS.
> I suspect variadic macros could be made to work, but you'd lose some
> compile-time safety. If I say:
>
> OPT_BOOL('x', NULL, &v, NULL, "turn on x")
>
> now, the compiler will complain about the number of arguments. In a
> variadic world, it silently ignores the final one. I feel like I've made
> this kind of error before (e.g., when switching to/from _F variants, or
> between types).
OPT_BOOL has PARSE_OPT_NOARG. Just saying.
It's true that a macro that accepts a variable number of arguments would
accept accidental extra arguments of the right type, but I don't see how
it would ignore excessive ones.
> You'd want some semantic check between what's in flags (i.e., is the
> OPTARG flag set), but I think that's beyond what the compiler itself can
> do (you could probably write a coccinelle rule for it, though).
Actually I'd want the macro to set that flag for me.
> I think it also squats on the variadic concept for the macro, so that no
> other features can use it. I.e., if you accidentally give _two_ extra
> arguments, I don't know that we can parse them out individually.
In case of an accident I'd just expect a compiler error. A cryptic one,
probably, alas, but no silence.
I was thinking more about something like the solutions discussed at
https://stackoverflow.com/questions/47674663/variable-arguments-inside-a-macro.
It allows selecting variants based on argument count.
That could look like this (untested except on https://godbolt.org/; the
EVALs are needed for MSVC for some reason):
#define OPT_INTEGER_FULL(s, l, v, h, f, d) { \
.type = OPTION_INTEGER, \
.short_name = (s), \
.long_name = (l), \
.value = (v), \
.argh = N_("n"), \
.help = (h), \
.flags = (f), \
.defval = (d), \
}
#define OPT_INTEGER_4(s, l, v, h) \
OPT_INTEGER_FULL(s, l, v, h, 0, 0)
#define OPT_INTEGER_5(s, l, v, h, f) \
OPT_INTEGER_FULL(s, l, v, h, f, 0)
#define OPT_INTEGER_6(s, l, v, h, f, d) \
OPT_INTEGER_FULL(s, l, v, h, (f) | PARSE_OPT_OPTARG, d)
#define EVAL(x) x
#define SEVENTH(_1, _2, _3, _4, _5, _6, x, ...) x
#define OPT_INTEGER(...) \
EVAL(EVAL(SEVENTH(__VA_ARGS__, OPT_INTEGER_6, OPT_INTEGER_5, OPT_INTEGER_4, 0))(__VA_ARGS__))
So OPT_INTEGER(s, l, v, h) would be the same as before. Add an argument
and it becomes current OPT_INTEGER_F, add another one and it acts as
your _OPTARG_F variant.
> So yeah, I think you'd really want a separate macro. The combinations
> start to add up (or multiply up, if you prefer ;) ). They _could_ be
> generated mechanically, I think, as they can all be implemented in terms
> of a master macro that knows about all features:
>
> #define OPT_BOOL_F(s, l, v, h, f) OPT_BOOL_ALL(s, l, v, h, f, 0)
> #define OPT_BOOL(s, l, v, h, f) OPT_BOOL_F(s, l, v, h, 0)
The "f" arg needs to go...
> #define OPT_BOOL_OPTARG_F(s, l, v, h, arg) OPT_BOOL_ALL(s, l, v, h, f | PARSE_OPT_OPTARG, arg)
... here, possibly.
> #define OPT_BOOL_OPTARG(s, l, v, h, arg) OPT_BOOL_OPTARG_F(s, l, v, h, 0, arg)
>
> But I'm not sure if cpp is up to that, or if I'd want to see what the
> resulting code looks like.
You mean having a macro define another macro? I don't think that's
possible.
René
^ permalink raw reply
* Re: [PATCH v3 09/13] trailer --parse docs: add explanation for its usefulness
From: Junio C Hamano @ 2023-09-08 21:57 UTC (permalink / raw)
To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver
In-Reply-To: <ef6b77016cd50c438fb58d79ffb10f748ddc5244.1694125210.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> --parse::
> A convenience alias for `--only-trailers --only-input
> - --unfold`.
> + --unfold`. This makes it easier to only see the trailers coming from the
> + input without influencing them with any command line options or
> + configuration variables, while also making the output machine-friendly with
> + --unfold.
Nicely explained. Without a concise explanation like this, it is
hard to see what the value of the "convenience" alias actually is.
^ permalink raw reply
* Re: [PATCH v3 06/13] trailer --no-divider help: describe usual "---" meaning
From: Junio C Hamano @ 2023-09-08 21:53 UTC (permalink / raw)
To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver
In-Reply-To: <3e58b6f5ea264a2c42ffbd008405b46626e0f864.1694125210.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> It's unclear what treating something "specially" means.
Good.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> builtin/interpret-trailers.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/interpret-trailers.c b/builtin/interpret-trailers.c
> index cf4f703c4e2..a7623dbfb2e 100644
> --- a/builtin/interpret-trailers.c
> +++ b/builtin/interpret-trailers.c
> @@ -109,7 +109,7 @@ int cmd_interpret_trailers(int argc, const char **argv, const char *prefix)
> OPT_BOOL(0, "unfold", &opts.unfold, N_("join whitespace-continued values")),
> OPT_CALLBACK_F(0, "parse", &opts, NULL, N_("set parsing options"),
> PARSE_OPT_NOARG | PARSE_OPT_NONEG, parse_opt_parse),
> - OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat --- specially")),
> + OPT_BOOL(0, "no-divider", &opts.no_divider, N_("do not treat \"---\" as the end of input")),
> OPT_CALLBACK(0, "trailer", &trailers, N_("trailer"),
> N_("trailer(s) to add"), option_parse_trailer),
> OPT_END()
^ permalink raw reply
* Re: [PATCH v3 03/13] trailer: add tests to check defaulting behavior with --no-* flags
From: Junio C Hamano @ 2023-09-08 21:52 UTC (permalink / raw)
To: Linus Arver via GitGitGadget; +Cc: git, Christian Couder, Linus Arver
In-Reply-To: <4b5c458ef436c2d208e6d6d0a1f99c65e9a11125.1694125210.git.gitgitgadget@gmail.com>
"Linus Arver via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Linus Arver <linusa@google.com>
>
> While the "--no-where" flag is tested, the "--no-if-exists" and
> "--no-if-missing" flags are not, so add tests for them. But also add
> tests for all "--no-*" flags to check their effects, both when (1) there
> are relevant configuration variables set, and (2) they are not set.
>
> Signed-off-by: Linus Arver <linusa@google.com>
> ---
> Documentation/git-interpret-trailers.txt | 14 ++-
> t/t7513-interpret-trailers.sh | 130 +++++++++++++++++++++++
> 2 files changed, 140 insertions(+), 4 deletions(-)
The commentary before each test_expect_success makes it quite a
pleasant read.
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index ed0fc04bd95..832aff06167 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -812,6 +812,53 @@ test_expect_success 'using "--where after" with "--no-where"' '
> test_cmp expected actual
> '
>
> +# Check whether using "--no-where" clears out only the "--where after", such
> +# that we still use the configuration in trailer.where (which is different from
> +# the hardcoded default (in WHERE_END) assuming the absence of .gitconfig).
> +# Here, the "start" setting of trailer.where is respected, so the new "Acked-by"
> +# and "Bug" trailers are placed at the beginning, and not at the end which is
> +# the harcoded default.
> +test_expect_success 'using "--where after" with "--no-where" defaults to configuration' '
> + test_config trailer.ack.key "Acked-by= " &&
> + test_config trailer.bug.key "Bug #" &&
> + test_config trailer.separators ":=#" &&
> + test_config trailer.where "start" &&
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Bug #42
> + Acked-by= Peff
> + Fixes: Z
> + Acked-by= Z
> + Reviewed-by: Z
> + Signed-off-by: Z
> + EOF
> + git interpret-trailers --where after --no-where --trailer "ack: Peff" \
> + --trailer "bug: 42" complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> +# The "--where after" will only get respected for the trailer that came
> +# immediately after it. For the next trailer (Bug #42), we default to using the
> +# hardcoded WHERE_END because we don't have any "trailer.where" or
> +# "trailer.bug.where" configured.
> +test_expect_success 'using "--no-where" defaults to harcoded default if nothing configured' '
> + test_config trailer.ack.key "Acked-by= " &&
> + test_config trailer.bug.key "Bug #" &&
> + test_config trailer.separators ":=#" &&
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Fixes: Z
> + Acked-by= Z
> + Acked-by= Peff
> + Reviewed-by: Z
> + Signed-off-by: Z
> + Bug #42
> + EOF
> + git interpret-trailers --where after --trailer "ack: Peff" --no-where \
> + --trailer "bug: 42" complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'using "where = after"' '
> test_config trailer.ack.key "Acked-by= " &&
> test_config trailer.ack.where "after" &&
> @@ -1176,6 +1223,56 @@ test_expect_success 'overriding configuration with "--if-exists replace"' '
> test_cmp expected actual
> '
>
> +# "trailer.ifexists" is set to "doNothing", so using "--no-if-exists" defaults
> +# to this "doNothing" behavior. So the "Fixes: 53" trailer does not get added.
> +test_expect_success 'using "--if-exists replace" with "--no-if-exists" defaults to configuration' '
> + test_config trailer.ifexists "doNothing" &&
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Fixes: Z
> + Acked-by: Z
> + Reviewed-by: Z
> + Signed-off-by: Z
> + EOF
> + git interpret-trailers --if-exists replace --no-if-exists --trailer "Fixes: 53" \
> + <complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> +# No "ifexists" configuration is set, so using "--no-if-exists" makes it default
> +# to addIfDifferentNeighbor. Because we do have a different neighbor "Fixes: 53"
> +# (because it got added by overriding with "--if-exists replace" earlier in the
> +# arguments list), we add "Signed-off-by: addme".
> +test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured' '
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Acked-by: Z
> + Reviewed-by: Z
> + Signed-off-by: Z
> + Fixes: 53
> + Signed-off-by: addme
> + EOF
> + git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
> + --trailer "Signed-off-by: addme" <complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> +# The second "Fixes: 53" trailer is discarded, because the "--no-if-exists" here
> +# makes us default to addIfDifferentNeighbor, and we already added the "Fixes:
> +# 53" trailer earlier in the argument list.
> +test_expect_success 'using "--no-if-exists" defaults to hardcoded default if nothing configured (no addition)' '
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Acked-by: Z
> + Reviewed-by: Z
> + Signed-off-by: Z
> + Fixes: 53
> + EOF
> + git interpret-trailers --if-exists replace --trailer "Fixes: 53" --no-if-exists \
> + --trailer "Fixes: 53" <complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'using "ifExists = replace"' '
> test_config trailer.fix.key "Fixes: " &&
> test_config trailer.fix.ifExists "replace" &&
> @@ -1425,6 +1522,39 @@ test_expect_success 'using "ifMissing = doNothing"' '
> test_cmp expected actual
> '
>
> +# Ignore the "IgnoredTrailer" because of "--if-missing doNothing", but also
> +# ignore the "StillIgnoredTrailer" because we set "trailer.ifMissing" to
> +# "doNothing" in configuration.
> +test_expect_success 'using "--no-if-missing" defaults to configuration' '
> + test_config trailer.ifMissing "doNothing" &&
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Fixes: Z
> + Acked-by: Z
> + Reviewed-by: Z
> + Signed-off-by: Z
> + EOF
> + git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
> + --trailer "StillIgnoredTrailer: ignoreme" <complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> +# Add the "AddedTrailer" because the "--no-if-missing" clears the "--if-missing
> +# doNothing" from earlier in the argument list.
> +test_expect_success 'using "--no-if-missing" defaults to hardcoded default if nothing configured' '
> + cat complex_message_body >expected &&
> + sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
> + Fixes: Z
> + Acked-by: Z
> + Reviewed-by: Z
> + Signed-off-by: Z
> + AddedTrailer: addme
> + EOF
> + git interpret-trailers --if-missing doNothing --trailer "IgnoredTrailer: ignoreme" --no-if-missing \
> + --trailer "AddedTrailer: addme" <complex_message >actual &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'default "where" is now "after"' '
> git config trailer.where "after" &&
> test_config trailer.ack.ifExists "add" &&
^ permalink raw reply
* Re: [PATCH v3 0/6] Introduce Git Standard Library
From: Junio C Hamano @ 2023-09-08 21:30 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, nasamuffin, jonathantanmy, linusa, phillip.wood123, vdye
In-Reply-To: <xmqqr0n8s8ps.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
> Calvin Wan <calvinwan@google.com> writes:
>
>> I have taken this series out of RFC since there weren't any significant
>> concerns with the overall concept and design of this series. This reroll
>> incorporates some smaller changes such as dropping the "push pager
>> dependency" patch in favor of stubbing it out. The main change this
>> reroll cleans up the Makefile rules and stubs, as suggested by
>> Phillip Wood (appreciate the help on this one)!
>
> What is your plan for the "config-parse" stuff? The "create new library"
> step in this series seem to aim for the same goal in a different ways.
Actually, this one is far less ambitious in touching "config"
subsystem, in that it only deals with parsing strings as values.
The other one knows how a config file is laid out, what key the
value we are about to read is expected for, etc., and it will
benefit by having the "parse" code separated out by this series, but
they are more or less orthogonal.
Queued. Thanks.
^ permalink raw reply
* Re: [PATCH v3 0/6] Introduce Git Standard Library
From: Junio C Hamano @ 2023-09-08 20:36 UTC (permalink / raw)
To: Calvin Wan; +Cc: git, nasamuffin, jonathantanmy, linusa, phillip.wood123, vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
> I have taken this series out of RFC since there weren't any significant
> concerns with the overall concept and design of this series. This reroll
> incorporates some smaller changes such as dropping the "push pager
> dependency" patch in favor of stubbing it out. The main change this
> reroll cleans up the Makefile rules and stubs, as suggested by
> Phillip Wood (appreciate the help on this one)!
What is your plan for the "config-parse" stuff? The "create new library"
step in this series seem to aim for the same goal in a different ways.
> This series has been rebased onto 1fc548b2d6a: The sixth batch
>
> Originally this series was built on other patches that have since been
> merged, which is why the range-diff is shown removing many of them.
Good. Previous rounds did not really attract much interest from the
public if I recall correctly. Let's see how well this round fares.
> Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++
It is interesting to see that there is no "std.*lib\.c" in the set
of source files, or "std.*lib\.a" target in the Makefile.
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Eric Wong @ 2023-09-08 19:57 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Phillip Wood
In-Reply-To: <pull.1582.git.1694167540231.gitgitgadget@gmail.com>
Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> wrote:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> In order to avoid invoking a signal handler in the child process between
> fork() and exec(), start_command() blocks all signals before calling
> fork and then in the child resets the disposition of all signals that
> are not ignored to SIG_DFL before unblocking them. The exception for
> ignored signals seems to been inspired by ruby's process handling[1]
> based on the misconception that execve() will reset them to
> SIG_DFL. Unfortunately this is not the case [2] and any signals that are
> ignored in the parent will default to being ignored by the program
> executed by start_command().
>
> When git ignores SIGPIPE before forking a child process it is to stop
> git from being killed if the child exits while git is writing to the
> child's stdin. We do not want to ignore SIGPIPE in the child. When git
> ignores SIGINT and SIGQUIT before forking a child process it is to stop
> git from being killed if the user interrupts the child with Ctrl-C or
> Ctrl-\ we do not want the child to ignore those signals [3].
> Fortunately the fix is easy - reset the disposition of all signals
> regardless of their previous disposition.
>
> [1] https://lore.kernel.org/git/20170413211428.GA5961@whir/
>
> [2] The man page for execve(2) states:
>
> POSIX.1 specifies that the dispositions of any signals that are
> ignored or set to the default are left unchanged. POSIX.1
> specifies one exception: if SIGCHLD is being ignored, then an
> implementation may leave the disposition unchanged or reset it
> to the default; Linux does the former.
Yeah, the old code seems like an error on my part. Oops :x
> diff --git a/run-command.c b/run-command.c
> index a558042c876..765775a1f42 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -823,11 +823,8 @@ fail_pipe:
> * restore default signal handlers here, in case
> * we catch a signal right before execve below
> */
> - for (sig = 1; sig < NSIG; sig++) {
> - /* ignored signals get reset to SIG_DFL on execve */
> - if (signal(sig, SIG_DFL) == SIG_IGN)
> - signal(sig, SIG_IGN);
> - }
> + for (sig = 1; sig < NSIG; sig++)
> + signal(sig, SIG_DFL);
Looks good to me and matches what I did in some other (A)GPL-3
projects, actually.
Acked-by: Eric Wong <e@80x24.org>
^ permalink raw reply
* Re: [PATCH] revision: add `--ignore-missing-links` user option
From: Junio C Hamano @ 2023-09-08 19:19 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git
In-Reply-To: <20230908174208.249184-1-karthik.188@gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> The revision backend is used by multiple porcelain commands such as
> git-rev-list(1) and git-log(1). The backend currently supports ignoring
> missing links by setting the `ignore_missing_links` bit. This allows the
> revision walk to skip any objects links which are missing.
> Currently there is no way to use git-rev-list(1) to traverse the objects
> of the main object directory (GIT_OBJECT_DIRECTORY) and print the
> boundary objects when moving from the main object directory to the
> alternate object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES).
The above description needs tightened up a bit, I think.
What is left unsaid is that you arranged a repository to borrow from
an alternate object directory (or two), and plan to walk objects
with this bit on in the repository, while leaving the alternates
disabled. Without stating that you plan to disable the alternates
while this mode of operation happens, nothing would happen when the
traversal goes from the main to the alternate because no links are
broken, no?
> By exposing this new flag `--ignore-missing-links`, users can set the
> required env variables (GIT_OBJECT_DIRECTORY and
> GIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to
> find the boundary objects between object directories.
This command being a plumbing, there is not much reason to object to
surfacing features that already internally exist to the command line
option. Having said that,
* Suppose your traversal with --ignore-missing-links from the tip
of a branch reaches a tree object A, and the tree object A has a
link to a blob B and a blob C. But B is in a separate object
store that you usually access via the alternate mechanism.
Instead of barfing "The repository is corrupt---object A points
at object B that does not exist", we pretend that A does not have
the link to B and keep traversing, discovering C and other
objects.
That much we can read from the above and also the documentation
part of the patch. The interaction with --boundary needs to be
clarified in this description and the documentation, though. It
is unclear if you show 'A' or 'B' in this scenario.
* Some traversals use the ignore-missing-links bit implicitly and
currently there is no way to turn it off. Is it plausible that
user may want to explicitly toggle it off, with the option
negated, i.e. --no-ignore-missing-links? I do not immediately
see the utility of such an option, but that is only due to my
lack of imagination. For now, I think it makes sense not to
allow negating this option, until somebody comes up with a useful
use case.
> +--ignore-missing-links::
> + When an object points to another object that is missing, pretend as if the
> + link did not exist. These missing links are not written to stdout unless
> + the --boundary flag is passed.
Does "git rev-list" ever writes "links"? I thought not.
"These missing objects are not written" would be more sensible, but
we never write missing objects with or without the option, so it
is not even worth saying.
When "--boundary" is passed, do they appear as if they are
available? If not, then the above description is very misleading.
During traversal, if an object that is referenced does not
exist, pretend as if the reference itself does not exist,
instead of dying of a repository corruption. Running the
command with the "--boundary" option makes these missing
objects, together with the objects on the edge of revision
ranges (i.e. true boundary objects), appear on the output,
prefixed with '-'.
or something like that, perhaps?
> +# With `--ignore-missing-links`, we stop the traversal when we encounter a
> +# missing link.
> +test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '
> + test_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD
> +'
> +
> +# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
> +# commits.
> +test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '
> + git -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&
> + test_stdout_line_count = 6 cat list-output &&
> + test_stdout_line_count = 1 cat list-output | grep "^-"
> +'
These tests are way too loose. Not only you want to see certain
number of boundary objects, you _know_ exactly which object should
be on the boundary, and you should check that instead. That will
allow you to find a mistake to write commit 'A' that refers to a
missing commit 'B', when they wanted to write the missing comit 'B',
as a boundary object, for example.
Thanks.
^ permalink raw reply
* [PATCH v3 6/6] git-std-lib: add test file to call git-std-lib.a functions
From: Calvin Wan @ 2023-09-08 17:44 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
Add test file that directly or indirectly calls all functions defined in
git-std-lib.a object files to showcase that they do not reference
missing objects and that git-std-lib.a can stand on its own.
Certain functions that cause the program to exit or are already called
by other functions are commented out.
TODO: replace with unit tests
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
t/Makefile | 4 +
t/stdlib-test.c | 231 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 235 insertions(+)
create mode 100644 t/stdlib-test.c
diff --git a/t/Makefile b/t/Makefile
index 3e00cdd801..b6d0bc9daa 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -150,3 +150,7 @@ perf:
.PHONY: pre-clean $(T) aggregate-results clean valgrind perf \
check-chainlint clean-chainlint test-chainlint
+
+test-git-std-lib:
+ cc -It -o stdlib-test stdlib-test.c -L. -l:../git-std-lib.a
+ ./stdlib-test
diff --git a/t/stdlib-test.c b/t/stdlib-test.c
new file mode 100644
index 0000000000..76fed9ecbf
--- /dev/null
+++ b/t/stdlib-test.c
@@ -0,0 +1,231 @@
+#include "../git-compat-util.h"
+#include "../abspath.h"
+#include "../hex-ll.h"
+#include "../parse.h"
+#include "../strbuf.h"
+#include "../string-list.h"
+
+/*
+ * Calls all functions from git-std-lib
+ * Some inline/trivial functions are skipped
+ */
+
+void abspath_funcs(void) {
+ struct strbuf sb = STRBUF_INIT;
+
+ fprintf(stderr, "calling abspath functions\n");
+ is_directory("foo");
+ strbuf_realpath(&sb, "foo", 0);
+ strbuf_realpath_forgiving(&sb, "foo", 0);
+ real_pathdup("foo", 0);
+ absolute_path("foo");
+ absolute_pathdup("foo");
+ prefix_filename("foo/", "bar");
+ prefix_filename_except_for_dash("foo/", "bar");
+ is_absolute_path("foo");
+ strbuf_add_absolute_path(&sb, "foo");
+ strbuf_add_real_path(&sb, "foo");
+}
+
+void hex_ll_funcs(void) {
+ unsigned char c;
+
+ fprintf(stderr, "calling hex-ll functions\n");
+
+ hexval('c');
+ hex2chr("A1");
+ hex_to_bytes(&c, "A1", 2);
+}
+
+void parse_funcs(void) {
+ intmax_t foo;
+ ssize_t foo1 = -1;
+ unsigned long foo2;
+ int foo3;
+ int64_t foo4;
+
+ fprintf(stderr, "calling parse functions\n");
+
+ git_parse_signed("42", &foo, maximum_signed_value_of_type(int));
+ git_parse_ssize_t("42", &foo1);
+ git_parse_ulong("42", &foo2);
+ git_parse_int("42", &foo3);
+ git_parse_int64("42", &foo4);
+ git_parse_maybe_bool("foo");
+ git_parse_maybe_bool_text("foo");
+ git_env_bool("foo", 1);
+ git_env_ulong("foo", 1);
+}
+
+static int allow_unencoded_fn(char ch) {
+ return 0;
+}
+
+void strbuf_funcs(void) {
+ struct strbuf *sb = xmalloc(sizeof(void*));
+ struct strbuf *sb2 = xmalloc(sizeof(void*));
+ struct strbuf sb3 = STRBUF_INIT;
+ struct string_list list = STRING_LIST_INIT_NODUP;
+ char *buf = "foo";
+ int fd = open("/dev/null", O_RDONLY);
+
+ fprintf(stderr, "calling strbuf functions\n");
+
+ starts_with("foo", "bar");
+ istarts_with("foo", "bar");
+ // skip_to_optional_arg_default(const char *str, const char *prefix,
+ // const char **arg, const char *def)
+ strbuf_init(sb, 0);
+ strbuf_init(sb2, 0);
+ strbuf_release(sb);
+ strbuf_attach(sb, strbuf_detach(sb, NULL), 0, 0); // calls strbuf_grow
+ strbuf_swap(sb, sb2);
+ strbuf_setlen(sb, 0);
+ strbuf_trim(sb); // calls strbuf_rtrim, strbuf_ltrim
+ // strbuf_rtrim() called by strbuf_trim()
+ // strbuf_ltrim() called by strbuf_trim()
+ strbuf_trim_trailing_dir_sep(sb);
+ strbuf_trim_trailing_newline(sb);
+ strbuf_reencode(sb, "foo", "bar");
+ strbuf_tolower(sb);
+ strbuf_add_separated_string_list(sb, " ", &list);
+ strbuf_list_free(strbuf_split_buf("foo bar", 8, ' ', -1));
+ strbuf_cmp(sb, sb2);
+ strbuf_addch(sb, 1);
+ strbuf_splice(sb, 0, 1, "foo", 3);
+ strbuf_insert(sb, 0, "foo", 3);
+ // strbuf_vinsertf() called by strbuf_insertf
+ strbuf_insertf(sb, 0, "%s", "foo");
+ strbuf_remove(sb, 0, 1);
+ strbuf_add(sb, "foo", 3);
+ strbuf_addbuf(sb, sb2);
+ strbuf_join_argv(sb, 0, NULL, ' ');
+ strbuf_addchars(sb, 1, 1);
+ strbuf_addf(sb, "%s", "foo");
+ strbuf_add_commented_lines(sb, "foo", 3, '#');
+ strbuf_commented_addf(sb, '#', "%s", "foo");
+ // strbuf_vaddf() called by strbuf_addf()
+ strbuf_addbuf_percentquote(sb, &sb3);
+ strbuf_add_percentencode(sb, "foo", STRBUF_ENCODE_SLASH);
+ strbuf_fread(sb, 0, stdin);
+ strbuf_read(sb, fd, 0);
+ strbuf_read_once(sb, fd, 0);
+ strbuf_write(sb, stderr);
+ strbuf_readlink(sb, "/dev/null", 0);
+ strbuf_getcwd(sb);
+ strbuf_getwholeline(sb, stderr, '\n');
+ strbuf_appendwholeline(sb, stderr, '\n');
+ strbuf_getline(sb, stderr);
+ strbuf_getline_lf(sb, stderr);
+ strbuf_getline_nul(sb, stderr);
+ strbuf_getwholeline_fd(sb, fd, '\n');
+ strbuf_read_file(sb, "/dev/null", 0);
+ strbuf_add_lines(sb, "foo", "bar", 0);
+ strbuf_addstr_xml_quoted(sb, "foo");
+ strbuf_addstr_urlencode(sb, "foo", allow_unencoded_fn);
+ strbuf_humanise_bytes(sb, 42);
+ strbuf_humanise_rate(sb, 42);
+ printf_ln("%s", sb);
+ fprintf_ln(stderr, "%s", sb);
+ xstrdup_tolower("foo");
+ xstrdup_toupper("foo");
+ // xstrvfmt() called by xstrfmt()
+ xstrfmt("%s", "foo");
+ // strbuf_addftime(struct strbuf *sb, const char *fmt, const struct tm *tm,
+ // int tz_offset, int suppress_tz_name)
+ // strbuf_stripspace(struct strbuf *sb, char comment_line_char)
+ // strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
+ // strbuf_strip_file_from_path(struct strbuf *sb)
+}
+
+static void error_builtin(const char *err, va_list params) {}
+static void warn_builtin(const char *err, va_list params) {}
+
+static report_fn error_routine = error_builtin;
+static report_fn warn_routine = warn_builtin;
+
+void usage_funcs(void) {
+ fprintf(stderr, "calling usage functions\n");
+ // Functions that call exit() are commented out
+
+ // usage()
+ // usagef()
+ // die()
+ // die_errno();
+ error("foo");
+ error_errno("foo");
+ die_message("foo");
+ die_message_errno("foo");
+ warning("foo");
+ warning_errno("foo");
+
+ // set_die_routine();
+ get_die_message_routine();
+ set_error_routine(error_builtin);
+ get_error_routine();
+ set_warn_routine(warn_builtin);
+ get_warn_routine();
+ // set_die_is_recursing_routine();
+}
+
+void wrapper_funcs(void) {
+ void *ptr = xmalloc(1);
+ int fd = open("/dev/null", O_RDONLY);
+ struct strbuf sb = STRBUF_INIT;
+ int mode = 0444;
+ char host[PATH_MAX], path[PATH_MAX], path1[PATH_MAX];
+ xsnprintf(path, sizeof(path), "out-XXXXXX");
+ xsnprintf(path1, sizeof(path1), "out-XXXXXX");
+ int tmp;
+
+ fprintf(stderr, "calling wrapper functions\n");
+
+ xstrdup("foo");
+ xmalloc(1);
+ xmallocz(1);
+ xmallocz_gently(1);
+ xmemdupz("foo", 3);
+ xstrndup("foo", 3);
+ xrealloc(ptr, 2);
+ xcalloc(1, 1);
+ xsetenv("foo", "bar", 0);
+ xopen("/dev/null", O_RDONLY);
+ xread(fd, &sb, 1);
+ xwrite(fd, &sb, 1);
+ xpread(fd, &sb, 1, 0);
+ xdup(fd);
+ xfopen("/dev/null", "r");
+ xfdopen(fd, "r");
+ tmp = xmkstemp(path);
+ close(tmp);
+ unlink(path);
+ tmp = xmkstemp_mode(path1, mode);
+ close(tmp);
+ unlink(path1);
+ xgetcwd();
+ fopen_for_writing(path);
+ fopen_or_warn(path, "r");
+ xstrncmpz("foo", "bar", 3);
+ // xsnprintf() called above
+ xgethostname(host, 3);
+ tmp = git_mkstemps_mode(path, 1, mode);
+ close(tmp);
+ unlink(path);
+ tmp = git_mkstemp_mode(path, mode);
+ close(tmp);
+ unlink(path);
+ read_in_full(fd, &sb, 1);
+ write_in_full(fd, &sb, 1);
+ pread_in_full(fd, &sb, 1, 0);
+}
+
+int main() {
+ abspath_funcs();
+ hex_ll_funcs();
+ parse_funcs();
+ strbuf_funcs();
+ usage_funcs();
+ wrapper_funcs();
+ fprintf(stderr, "all git-std-lib functions finished calling\n");
+ return 0;
+}
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v3 5/6] git-std-lib: introduce git standard library
From: Calvin Wan @ 2023-09-08 17:44 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
The Git Standard Library intends to serve as the foundational library
and root dependency that other libraries in Git will be built off of.
That is to say, suppose we have libraries X and Y; a user that wants to
use X and Y would need to include X, Y, and this Git Standard Library.
Add Documentation/technical/git-std-lib.txt to further explain the
design and rationale.
Signed-off-by: Calvin Wan <calvinwan@google.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
---
Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++++++
Makefile | 39 ++++-
git-compat-util.h | 7 +-
| 6 +
| 6 +
stubs/trace2.c | 27 ++++
symlinks.c | 2 +
wrapper.c | 1 -
8 files changed, 276 insertions(+), 3 deletions(-)
create mode 100644 Documentation/technical/git-std-lib.txt
create mode 100644 stubs/pager.c
create mode 100644 stubs/pager.h
create mode 100644 stubs/trace2.c
diff --git a/Documentation/technical/git-std-lib.txt b/Documentation/technical/git-std-lib.txt
new file mode 100644
index 0000000000..397c1da8c8
--- /dev/null
+++ b/Documentation/technical/git-std-lib.txt
@@ -0,0 +1,191 @@
+Git Standard Library
+================
+
+The Git Standard Library intends to serve as the foundational library
+and root dependency that other libraries in Git will be built off of.
+That is to say, suppose we have libraries X and Y; a user that wants to
+use X and Y would need to include X, Y, and this Git Standard Library.
+This does not mean that the Git Standard Library will be the only
+possible root dependency in the future, but rather the most significant
+and widely used one.
+
+Dependency graph in libified Git
+================
+
+If you look in the Git Makefile, all of the objects defined in the Git
+library are compiled and archived into a singular file, libgit.a, which
+is linked against by common-main.o with other external dependencies and
+turned into the Git executable. In other words, the Git executable has
+dependencies on libgit.a and a couple of external libraries. The
+libfication of Git will not affect this current build flow, but instead
+will provide an alternate method for building Git.
+
+With our current method of building Git, we can imagine the dependency
+graph as such:
+
+ Git
+ /\
+ / \
+ / \
+ libgit.a ext deps
+
+In libifying parts of Git, we want to shrink the dependency graph to
+only the minimal set of dependencies, so libraries should not use
+libgit.a. Instead, it would look like:
+
+ Git
+ /\
+ / \
+ / \
+ libgit.a ext deps
+ /\
+ / \
+ / \
+object-store.a (other lib)
+ | /
+ | /
+ | /
+ config.a /
+ | /
+ | /
+ | /
+git-std-lib.a
+
+Instead of containing all of the objects in Git, libgit.a would contain
+objects that are not built by libraries it links against. Consequently,
+if someone wanted their own custom build of Git with their own custom
+implementation of the object store, they would only have to swap out
+object-store.a rather than do a hard fork of Git.
+
+Rationale behind Git Standard Library
+================
+
+The rationale behind what's in and what's not in the Git Standard
+Library essentially is the result of two observations within the Git
+codebase: every file includes git-compat-util.h which defines functions
+in a couple of different files, and wrapper.c + usage.c have
+difficult-to-separate circular dependencies with each other and other
+files.
+
+Ubiquity of git-compat-util.h and circular dependencies
+========
+
+Every file in the Git codebase includes git-compat-util.h. It serves as
+"a compatibility aid that isolates the knowledge of platform specific
+inclusion order and what feature macros to define before including which
+system header" (Junio[1]). Since every file includes git-compat-util.h, and
+git-compat-util.h includes wrapper.h and usage.h, it would make sense
+for wrapper.c and usage.c to be a part of the root library. They have
+difficult to separate circular dependencies with each other so they
+can't be independent libraries. Wrapper.c has dependencies on parse.c,
+abspath.c, strbuf.c, which in turn also have dependencies on usage.c and
+wrapper.c -- more circular dependencies.
+
+Tradeoff between swappability and refactoring
+========
+
+From the above dependency graph, we can see that git-std-lib.a could be
+many smaller libraries rather than a singular library. So why choose a
+singular library when multiple libraries can be individually easier to
+swap and are more modular? A singular library requires less work to
+separate out circular dependencies within itself so it becomes a
+tradeoff question between work and reward. While there may be a point in
+the future where a file like usage.c would want its own library so that
+someone can have custom die() or error(), the work required to refactor
+out the circular dependencies in some files would be enormous due to
+their ubiquity so therefore I believe it is not worth the tradeoff
+currently. Additionally, we can in the future choose to do this refactor
+and change the API for the library if there becomes enough of a reason
+to do so (remember we are avoiding promising stability of the interfaces
+of those libraries).
+
+Reuse of compatibility functions in git-compat-util.h
+========
+
+Most functions defined in git-compat-util.h are implemented in compat/
+and have dependencies limited to strbuf.h and wrapper.h so they can be
+easily included in git-std-lib.a, which as a root dependency means that
+higher level libraries do not have to worry about compatibility files in
+compat/. The rest of the functions defined in git-compat-util.h are
+implemented in top level files and are hidden behind
+an #ifdef if their implementation is not in git-std-lib.a.
+
+Rationale summary
+========
+
+The Git Standard Library allows us to get the libification ball rolling
+with other libraries in Git. By not spending many
+more months attempting to refactor difficult circular dependencies and
+instead spending that time getting to a state where we can test out
+swapping a library out such as config or object store, we can prove the
+viability of Git libification on a much faster time scale. Additionally
+the code cleanups that have happened so far have been minor and
+beneficial for the codebase. It is probable that making large movements
+would negatively affect code clarity.
+
+Git Standard Library boundary
+================
+
+While I have described above some useful heuristics for identifying
+potential candidates for git-std-lib.a, a standard library should not
+have a shaky definition for what belongs in it.
+
+ - Low-level files (aka operates only on other primitive types) that are
+ used everywhere within the codebase (wrapper.c, usage.c, strbuf.c)
+ - Dependencies that are low-level and widely used
+ (abspath.c, date.c, hex-ll.c, parse.c, utf8.c)
+ - low-level git/* files with functions defined in git-compat-util.h
+ (ctype.c)
+ - compat/*
+ - stubbed out dependencies in stubs/ (stubs/pager.c, stubs/trace2.c)
+
+There are other files that might fit this definition, but that does not
+mean it should belong in git-std-lib.a. Those files should start as
+their own separate library since any file added to git-std-lib.a loses
+its flexibility of being easily swappable.
+
+Wrapper.c and usage.c have dependencies on pager and trace2 that are
+possible to remove at the cost of sacrificing the ability for standard Git
+to be able to trace functions in those files and other files in git-std-lib.a.
+In order for git-std-lib.a to compile with those dependencies, stubbed out
+versions of those files are implemented and swapped in during compilation time.
+
+Files inside of Git Standard Library
+================
+
+The initial set of files in git-std-lib.a are:
+abspath.c
+ctype.c
+date.c
+hex-ll.c
+parse.c
+strbuf.c
+usage.c
+utf8.c
+wrapper.c
+relevant compat/ files
+
+When these files are compiled together with the following files (or
+user-provided files that provide the same functions), they form a
+complete library:
+stubs/pager.c
+stubs/trace2.c
+
+Pitfalls
+================
+
+There are a small amount of files under compat/* that have dependencies
+not inside of git-std-lib.a. While those functions are not called on
+Linux, other OSes might call those problematic functions. I don't see
+this as a major problem, just moreso an observation that libification in
+general may also require some minor compatibility work in the future.
+
+Testing
+================
+
+Unit tests should catch any breakages caused by changes to files in
+git-std-lib.a (i.e. introduction of a out of scope dependency) and new
+functions introduced to git-std-lib.a will require unit tests written
+for them.
+
+[1] https://lore.kernel.org/git/xmqqwn17sydw.fsf@gitster.g/
\ No newline at end of file
diff --git a/Makefile b/Makefile
index 9226c719a0..0a2d1ae3cc 100644
--- a/Makefile
+++ b/Makefile
@@ -669,6 +669,7 @@ FUZZ_PROGRAMS =
GIT_OBJS =
LIB_OBJS =
SCALAR_OBJS =
+STUB_OBJS =
OBJECTS =
OTHER_PROGRAMS =
PROGRAM_OBJS =
@@ -956,6 +957,7 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
LIB_H = $(FOUND_H_SOURCES)
+ifndef GIT_STD_LIB
LIB_OBJS += abspath.o
LIB_OBJS += add-interactive.o
LIB_OBJS += add-patch.o
@@ -1196,6 +1198,27 @@ LIB_OBJS += write-or-die.o
LIB_OBJS += ws.o
LIB_OBJS += wt-status.o
LIB_OBJS += xdiff-interface.o
+else ifdef GIT_STD_LIB
+LIB_OBJS += abspath.o
+LIB_OBJS += ctype.o
+LIB_OBJS += date.o
+LIB_OBJS += hex-ll.o
+LIB_OBJS += parse.o
+LIB_OBJS += strbuf.o
+LIB_OBJS += usage.o
+LIB_OBJS += utf8.o
+LIB_OBJS += wrapper.o
+
+ifdef STUB_TRACE2
+STUB_OBJS += stubs/trace2.o
+endif
+
+ifdef STUB_PAGER
+STUB_OBJS += stubs/pager.o
+endif
+
+LIB_OBJS += $(STUB_OBJS)
+endif
BUILTIN_OBJS += builtin/add.o
BUILTIN_OBJS += builtin/am.o
@@ -2162,6 +2185,11 @@ ifdef FSMONITOR_OS_SETTINGS
COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o
endif
+ifdef GIT_STD_LIB
+ BASIC_CFLAGS += -DGIT_STD_LIB
+ BASIC_CFLAGS += -DNO_GETTEXT
+endif
+
ifeq ($(TCLTK_PATH),)
NO_TCLTK = NoThanks
endif
@@ -3668,7 +3696,7 @@ clean: profile-clean coverage-clean cocciclean
$(RM) git.res
$(RM) $(OBJECTS)
$(RM) headless-git.o
- $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
+ $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB) $(STD_LIB_FILE)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
$(RM) $(TEST_PROGRAMS)
$(RM) $(FUZZ_PROGRAMS)
@@ -3849,3 +3877,12 @@ $(FUZZ_PROGRAMS): all
$(XDIFF_OBJS) $(EXTLIBS) git.o $@.o $(LIB_FUZZING_ENGINE) -o $@
fuzz-all: $(FUZZ_PROGRAMS)
+
+### Libified Git rules
+
+# git-std-lib
+# `make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease STUB_PAGER=YesPlease`
+STD_LIB = git-std-lib.a
+
+$(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
+ $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
diff --git a/git-compat-util.h b/git-compat-util.h
index 3e7a59b5ff..14bf71c530 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -455,8 +455,8 @@ static inline int noop_core_config(const char *var UNUSED,
#define platform_core_config noop_core_config
#endif
+#if !defined(__MINGW32__) && !defined(_MSC_VER) && !defined(GIT_STD_LIB)
int lstat_cache_aware_rmdir(const char *path);
-#if !defined(__MINGW32__) && !defined(_MSC_VER)
#define rmdir lstat_cache_aware_rmdir
#endif
@@ -966,9 +966,11 @@ const char *inet_ntop(int af, const void *src, char *dst, size_t size);
#endif
#ifdef NO_PTHREADS
+#ifdef GIT_STD_LIB
#define atexit git_atexit
int git_atexit(void (*handler)(void));
#endif
+#endif
static inline size_t st_add(size_t a, size_t b)
{
@@ -1462,14 +1464,17 @@ static inline int is_missing_file_error(int errno_)
return (errno_ == ENOENT || errno_ == ENOTDIR);
}
+#ifndef GIT_STD_LIB
int cmd_main(int, const char **);
/*
* Intercept all calls to exit() and route them to trace2 to
* optionally emit a message before calling the real exit().
*/
+
int common_exit(const char *file, int line, int code);
#define exit(code) exit(common_exit(__FILE__, __LINE__, (code)))
+#endif
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
--git a/stubs/pager.c b/stubs/pager.c
new file mode 100644
index 0000000000..4f575cada7
--- /dev/null
+++ b/stubs/pager.c
@@ -0,0 +1,6 @@
+#include "pager.h"
+
+int pager_in_use(void)
+{
+ return 0;
+}
--git a/stubs/pager.h b/stubs/pager.h
new file mode 100644
index 0000000000..b797910881
--- /dev/null
+++ b/stubs/pager.h
@@ -0,0 +1,6 @@
+#ifndef PAGER_H
+#define PAGER_H
+
+int pager_in_use(void);
+
+#endif /* PAGER_H */
diff --git a/stubs/trace2.c b/stubs/trace2.c
new file mode 100644
index 0000000000..7d89482228
--- /dev/null
+++ b/stubs/trace2.c
@@ -0,0 +1,27 @@
+#include "git-compat-util.h"
+#include "trace2.h"
+
+struct child_process { int stub; };
+struct repository { int stub; };
+struct json_writer { int stub; };
+
+void trace2_region_enter_fl(const char *file, int line, const char *category,
+ const char *label, const struct repository *repo, ...) { }
+void trace2_region_leave_fl(const char *file, int line, const char *category,
+ const char *label, const struct repository *repo, ...) { }
+void trace2_data_string_fl(const char *file, int line, const char *category,
+ const struct repository *repo, const char *key,
+ const char *value) { }
+void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names) { }
+void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
+ va_list ap) { }
+void trace2_cmd_name_fl(const char *file, int line, const char *name) { }
+void trace2_thread_start_fl(const char *file, int line,
+ const char *thread_base_name) { }
+void trace2_thread_exit_fl(const char *file, int line) { }
+void trace2_data_intmax_fl(const char *file, int line, const char *category,
+ const struct repository *repo, const char *key,
+ intmax_t value) { }
+int trace2_is_enabled(void) { return 0; }
+void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
+void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
diff --git a/symlinks.c b/symlinks.c
index b29e340c2d..bced721a0c 100644
--- a/symlinks.c
+++ b/symlinks.c
@@ -337,6 +337,7 @@ void invalidate_lstat_cache(void)
reset_lstat_cache(&default_cache);
}
+#if !defined(__MINGW32__) && !defined(_MSC_VER) && !defined(GIT_STD_LIB)
#undef rmdir
int lstat_cache_aware_rmdir(const char *path)
{
@@ -348,3 +349,4 @@ int lstat_cache_aware_rmdir(const char *path)
return ret;
}
+#endif
diff --git a/wrapper.c b/wrapper.c
index 7da15a56da..eeac3741cf 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -5,7 +5,6 @@
#include "abspath.h"
#include "parse.h"
#include "gettext.h"
-#include "repository.h"
#include "strbuf.h"
#include "trace2.h"
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v3 4/6] parse: create new library for parsing strings and env values
From: Calvin Wan @ 2023-09-08 17:44 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
While string and environment value parsing is mainly consumed by
config.c, there are other files that only need parsing functionality and
not config functionality. By separating out string and environment value
parsing from config, those files can instead be dependent on parse,
which has a much smaller dependency chain than config. This ultimately
allows us to inclue parse.[ch] in an independent library since it
doesn't have dependencies to Git-specific internals unlike in
config.[ch].
Move general string and env parsing functions from config.[ch] to
parse.[ch].
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
Makefile | 1 +
attr.c | 2 +-
config.c | 180 +-----------------------------------
config.h | 14 +--
pack-objects.c | 2 +-
pack-revindex.c | 2 +-
parse-options.c | 3 +-
parse.c | 182 +++++++++++++++++++++++++++++++++++++
parse.h | 20 ++++
pathspec.c | 2 +-
preload-index.c | 2 +-
progress.c | 2 +-
prompt.c | 2 +-
rebase.c | 2 +-
t/helper/test-env-helper.c | 2 +-
unpack-trees.c | 2 +-
wrapper.c | 2 +-
write-or-die.c | 2 +-
18 files changed, 219 insertions(+), 205 deletions(-)
create mode 100644 parse.c
create mode 100644 parse.h
diff --git a/Makefile b/Makefile
index 861e643708..9226c719a0 100644
--- a/Makefile
+++ b/Makefile
@@ -1091,6 +1091,7 @@ LIB_OBJS += pack-write.o
LIB_OBJS += packfile.o
LIB_OBJS += pager.o
LIB_OBJS += parallel-checkout.o
+LIB_OBJS += parse.o
LIB_OBJS += parse-options-cb.o
LIB_OBJS += parse-options.o
LIB_OBJS += patch-delta.o
diff --git a/attr.c b/attr.c
index 71c84fbcf8..3c0b4fb3d9 100644
--- a/attr.c
+++ b/attr.c
@@ -7,7 +7,7 @@
*/
#include "git-compat-util.h"
-#include "config.h"
+#include "parse.h"
#include "environment.h"
#include "exec-cmd.h"
#include "attr.h"
diff --git a/config.c b/config.c
index 7dde0aaa02..c7bc21a25d 100644
--- a/config.c
+++ b/config.c
@@ -11,6 +11,7 @@
#include "date.h"
#include "branch.h"
#include "config.h"
+#include "parse.h"
#include "convert.h"
#include "environment.h"
#include "gettext.h"
@@ -1165,129 +1166,6 @@ static int git_parse_source(struct config_source *cs, config_fn_t fn,
return error_return;
}
-static uintmax_t get_unit_factor(const char *end)
-{
- if (!*end)
- return 1;
- else if (!strcasecmp(end, "k"))
- return 1024;
- else if (!strcasecmp(end, "m"))
- return 1024 * 1024;
- else if (!strcasecmp(end, "g"))
- return 1024 * 1024 * 1024;
- return 0;
-}
-
-static int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
-{
- if (value && *value) {
- char *end;
- intmax_t val;
- intmax_t factor;
-
- if (max < 0)
- BUG("max must be a positive integer");
-
- errno = 0;
- val = strtoimax(value, &end, 0);
- if (errno == ERANGE)
- return 0;
- if (end == value) {
- errno = EINVAL;
- return 0;
- }
- factor = get_unit_factor(end);
- if (!factor) {
- errno = EINVAL;
- return 0;
- }
- if ((val < 0 && -max / factor > val) ||
- (val > 0 && max / factor < val)) {
- errno = ERANGE;
- return 0;
- }
- val *= factor;
- *ret = val;
- return 1;
- }
- errno = EINVAL;
- return 0;
-}
-
-static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
-{
- if (value && *value) {
- char *end;
- uintmax_t val;
- uintmax_t factor;
-
- /* negative values would be accepted by strtoumax */
- if (strchr(value, '-')) {
- errno = EINVAL;
- return 0;
- }
- errno = 0;
- val = strtoumax(value, &end, 0);
- if (errno == ERANGE)
- return 0;
- if (end == value) {
- errno = EINVAL;
- return 0;
- }
- factor = get_unit_factor(end);
- if (!factor) {
- errno = EINVAL;
- return 0;
- }
- if (unsigned_mult_overflows(factor, val) ||
- factor * val > max) {
- errno = ERANGE;
- return 0;
- }
- val *= factor;
- *ret = val;
- return 1;
- }
- errno = EINVAL;
- return 0;
-}
-
-int git_parse_int(const char *value, int *ret)
-{
- intmax_t tmp;
- if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))
- return 0;
- *ret = tmp;
- return 1;
-}
-
-static int git_parse_int64(const char *value, int64_t *ret)
-{
- intmax_t tmp;
- if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))
- return 0;
- *ret = tmp;
- return 1;
-}
-
-int git_parse_ulong(const char *value, unsigned long *ret)
-{
- uintmax_t tmp;
- if (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(long)))
- return 0;
- *ret = tmp;
- return 1;
-}
-
-int git_parse_ssize_t(const char *value, ssize_t *ret)
-{
- intmax_t tmp;
- if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
- return 0;
- *ret = tmp;
- return 1;
-}
-
NORETURN
static void die_bad_number(const char *name, const char *value,
const struct key_value_info *kvi)
@@ -1363,23 +1241,6 @@ ssize_t git_config_ssize_t(const char *name, const char *value,
return ret;
}
-static int git_parse_maybe_bool_text(const char *value)
-{
- if (!value)
- return 1;
- if (!*value)
- return 0;
- if (!strcasecmp(value, "true")
- || !strcasecmp(value, "yes")
- || !strcasecmp(value, "on"))
- return 1;
- if (!strcasecmp(value, "false")
- || !strcasecmp(value, "no")
- || !strcasecmp(value, "off"))
- return 0;
- return -1;
-}
-
static const struct fsync_component_name {
const char *name;
enum fsync_component component_bits;
@@ -1454,16 +1315,6 @@ static enum fsync_component parse_fsync_components(const char *var, const char *
return (current & ~negative) | positive;
}
-int git_parse_maybe_bool(const char *value)
-{
- int v = git_parse_maybe_bool_text(value);
- if (0 <= v)
- return v;
- if (git_parse_int(value, &v))
- return !!v;
- return -1;
-}
-
int git_config_bool_or_int(const char *name, const char *value,
const struct key_value_info *kvi, int *is_bool)
{
@@ -2126,35 +1977,6 @@ void git_global_config(char **user_out, char **xdg_out)
*xdg_out = xdg_config;
}
-/*
- * Parse environment variable 'k' as a boolean (in various
- * possible spellings); if missing, use the default value 'def'.
- */
-int git_env_bool(const char *k, int def)
-{
- const char *v = getenv(k);
- int val;
- if (!v)
- return def;
- val = git_parse_maybe_bool(v);
- if (val < 0)
- die(_("bad boolean environment value '%s' for '%s'"),
- v, k);
- return val;
-}
-
-/*
- * Parse environment variable 'k' as ulong with possibly a unit
- * suffix; if missing, use the default value 'val'.
- */
-unsigned long git_env_ulong(const char *k, unsigned long val)
-{
- const char *v = getenv(k);
- if (v && !git_parse_ulong(v, &val))
- die(_("failed to parse %s"), k);
- return val;
-}
-
int git_config_system(void)
{
return !git_env_bool("GIT_CONFIG_NOSYSTEM", 0);
diff --git a/config.h b/config.h
index 6332d74904..14f881ecfa 100644
--- a/config.h
+++ b/config.h
@@ -4,7 +4,7 @@
#include "hashmap.h"
#include "string-list.h"
#include "repository.h"
-
+#include "parse.h"
/**
* The config API gives callers a way to access Git configuration files
@@ -243,16 +243,6 @@ int config_with_options(config_fn_t fn, void *,
* The following helper functions aid in parsing string values
*/
-int git_parse_ssize_t(const char *, ssize_t *);
-int git_parse_ulong(const char *, unsigned long *);
-int git_parse_int(const char *value, int *ret);
-
-/**
- * Same as `git_config_bool`, except that it returns -1 on error rather
- * than dying.
- */
-int git_parse_maybe_bool(const char *);
-
/**
* Parse the string to an integer, including unit factors. Dies on error;
* otherwise, returns the parsed result.
@@ -385,8 +375,6 @@ int git_config_rename_section(const char *, const char *);
int git_config_rename_section_in_file(const char *, const char *, const char *);
int git_config_copy_section(const char *, const char *);
int git_config_copy_section_in_file(const char *, const char *, const char *);
-int git_env_bool(const char *, int);
-unsigned long git_env_ulong(const char *, unsigned long);
int git_config_system(void);
int config_error_nonbool(const char *);
#if defined(__GNUC__)
diff --git a/pack-objects.c b/pack-objects.c
index 1b8052bece..f403ca6986 100644
--- a/pack-objects.c
+++ b/pack-objects.c
@@ -3,7 +3,7 @@
#include "pack.h"
#include "pack-objects.h"
#include "packfile.h"
-#include "config.h"
+#include "parse.h"
static uint32_t locate_object_entry_hash(struct packing_data *pdata,
const struct object_id *oid,
diff --git a/pack-revindex.c b/pack-revindex.c
index 7fffcad912..a01a2a4640 100644
--- a/pack-revindex.c
+++ b/pack-revindex.c
@@ -6,7 +6,7 @@
#include "packfile.h"
#include "strbuf.h"
#include "trace2.h"
-#include "config.h"
+#include "parse.h"
#include "midx.h"
#include "csum-file.h"
diff --git a/parse-options.c b/parse-options.c
index e8e076c3a6..093eaf2db8 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1,11 +1,12 @@
#include "git-compat-util.h"
#include "parse-options.h"
#include "abspath.h"
-#include "config.h"
+#include "parse.h"
#include "commit.h"
#include "color.h"
#include "gettext.h"
#include "strbuf.h"
+#include "string-list.h"
#include "utf8.h"
static int disallow_abbreviated_options;
diff --git a/parse.c b/parse.c
new file mode 100644
index 0000000000..42d691a0fb
--- /dev/null
+++ b/parse.c
@@ -0,0 +1,182 @@
+#include "git-compat-util.h"
+#include "gettext.h"
+#include "parse.h"
+
+static uintmax_t get_unit_factor(const char *end)
+{
+ if (!*end)
+ return 1;
+ else if (!strcasecmp(end, "k"))
+ return 1024;
+ else if (!strcasecmp(end, "m"))
+ return 1024 * 1024;
+ else if (!strcasecmp(end, "g"))
+ return 1024 * 1024 * 1024;
+ return 0;
+}
+
+int git_parse_signed(const char *value, intmax_t *ret, intmax_t max)
+{
+ if (value && *value) {
+ char *end;
+ intmax_t val;
+ intmax_t factor;
+
+ if (max < 0)
+ BUG("max must be a positive integer");
+
+ errno = 0;
+ val = strtoimax(value, &end, 0);
+ if (errno == ERANGE)
+ return 0;
+ if (end == value) {
+ errno = EINVAL;
+ return 0;
+ }
+ factor = get_unit_factor(end);
+ if (!factor) {
+ errno = EINVAL;
+ return 0;
+ }
+ if ((val < 0 && -max / factor > val) ||
+ (val > 0 && max / factor < val)) {
+ errno = ERANGE;
+ return 0;
+ }
+ val *= factor;
+ *ret = val;
+ return 1;
+ }
+ errno = EINVAL;
+ return 0;
+}
+
+static int git_parse_unsigned(const char *value, uintmax_t *ret, uintmax_t max)
+{
+ if (value && *value) {
+ char *end;
+ uintmax_t val;
+ uintmax_t factor;
+
+ /* negative values would be accepted by strtoumax */
+ if (strchr(value, '-')) {
+ errno = EINVAL;
+ return 0;
+ }
+ errno = 0;
+ val = strtoumax(value, &end, 0);
+ if (errno == ERANGE)
+ return 0;
+ if (end == value) {
+ errno = EINVAL;
+ return 0;
+ }
+ factor = get_unit_factor(end);
+ if (!factor) {
+ errno = EINVAL;
+ return 0;
+ }
+ if (unsigned_mult_overflows(factor, val) ||
+ factor * val > max) {
+ errno = ERANGE;
+ return 0;
+ }
+ val *= factor;
+ *ret = val;
+ return 1;
+ }
+ errno = EINVAL;
+ return 0;
+}
+
+int git_parse_int(const char *value, int *ret)
+{
+ intmax_t tmp;
+ if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int)))
+ return 0;
+ *ret = tmp;
+ return 1;
+}
+
+int git_parse_int64(const char *value, int64_t *ret)
+{
+ intmax_t tmp;
+ if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(int64_t)))
+ return 0;
+ *ret = tmp;
+ return 1;
+}
+
+int git_parse_ulong(const char *value, unsigned long *ret)
+{
+ uintmax_t tmp;
+ if (!git_parse_unsigned(value, &tmp, maximum_unsigned_value_of_type(long)))
+ return 0;
+ *ret = tmp;
+ return 1;
+}
+
+int git_parse_ssize_t(const char *value, ssize_t *ret)
+{
+ intmax_t tmp;
+ if (!git_parse_signed(value, &tmp, maximum_signed_value_of_type(ssize_t)))
+ return 0;
+ *ret = tmp;
+ return 1;
+}
+
+int git_parse_maybe_bool_text(const char *value)
+{
+ if (!value)
+ return 1;
+ if (!*value)
+ return 0;
+ if (!strcasecmp(value, "true")
+ || !strcasecmp(value, "yes")
+ || !strcasecmp(value, "on"))
+ return 1;
+ if (!strcasecmp(value, "false")
+ || !strcasecmp(value, "no")
+ || !strcasecmp(value, "off"))
+ return 0;
+ return -1;
+}
+
+int git_parse_maybe_bool(const char *value)
+{
+ int v = git_parse_maybe_bool_text(value);
+ if (0 <= v)
+ return v;
+ if (git_parse_int(value, &v))
+ return !!v;
+ return -1;
+}
+
+/*
+ * Parse environment variable 'k' as a boolean (in various
+ * possible spellings); if missing, use the default value 'def'.
+ */
+int git_env_bool(const char *k, int def)
+{
+ const char *v = getenv(k);
+ int val;
+ if (!v)
+ return def;
+ val = git_parse_maybe_bool(v);
+ if (val < 0)
+ die(_("bad boolean environment value '%s' for '%s'"),
+ v, k);
+ return val;
+}
+
+/*
+ * Parse environment variable 'k' as ulong with possibly a unit
+ * suffix; if missing, use the default value 'val'.
+ */
+unsigned long git_env_ulong(const char *k, unsigned long val)
+{
+ const char *v = getenv(k);
+ if (v && !git_parse_ulong(v, &val))
+ die(_("failed to parse %s"), k);
+ return val;
+}
diff --git a/parse.h b/parse.h
new file mode 100644
index 0000000000..07d2193d69
--- /dev/null
+++ b/parse.h
@@ -0,0 +1,20 @@
+#ifndef PARSE_H
+#define PARSE_H
+
+int git_parse_signed(const char *value, intmax_t *ret, intmax_t max);
+int git_parse_ssize_t(const char *, ssize_t *);
+int git_parse_ulong(const char *, unsigned long *);
+int git_parse_int(const char *value, int *ret);
+int git_parse_int64(const char *value, int64_t *ret);
+
+/**
+ * Same as `git_config_bool`, except that it returns -1 on error rather
+ * than dying.
+ */
+int git_parse_maybe_bool(const char *);
+int git_parse_maybe_bool_text(const char *value);
+
+int git_env_bool(const char *, int);
+unsigned long git_env_ulong(const char *, unsigned long);
+
+#endif /* PARSE_H */
diff --git a/pathspec.c b/pathspec.c
index 3a3a5724c4..7f88f1c02b 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -1,6 +1,6 @@
#include "git-compat-util.h"
#include "abspath.h"
-#include "config.h"
+#include "parse.h"
#include "dir.h"
#include "environment.h"
#include "gettext.h"
diff --git a/preload-index.c b/preload-index.c
index e44530c80c..63fd35d64b 100644
--- a/preload-index.c
+++ b/preload-index.c
@@ -7,7 +7,7 @@
#include "environment.h"
#include "fsmonitor.h"
#include "gettext.h"
-#include "config.h"
+#include "parse.h"
#include "preload-index.h"
#include "progress.h"
#include "read-cache.h"
diff --git a/progress.c b/progress.c
index f695798aca..c83cb60bf1 100644
--- a/progress.c
+++ b/progress.c
@@ -17,7 +17,7 @@
#include "trace.h"
#include "trace2.h"
#include "utf8.h"
-#include "config.h"
+#include "parse.h"
#define TP_IDX_MAX 8
diff --git a/prompt.c b/prompt.c
index 3baa33f63d..8935fe4dfb 100644
--- a/prompt.c
+++ b/prompt.c
@@ -1,5 +1,5 @@
#include "git-compat-util.h"
-#include "config.h"
+#include "parse.h"
#include "environment.h"
#include "run-command.h"
#include "strbuf.h"
diff --git a/rebase.c b/rebase.c
index 17a570f1ff..69a1822da3 100644
--- a/rebase.c
+++ b/rebase.c
@@ -1,6 +1,6 @@
#include "git-compat-util.h"
#include "rebase.h"
-#include "config.h"
+#include "parse.h"
#include "gettext.h"
/*
diff --git a/t/helper/test-env-helper.c b/t/helper/test-env-helper.c
index 66c88b8ff3..1c486888a4 100644
--- a/t/helper/test-env-helper.c
+++ b/t/helper/test-env-helper.c
@@ -1,5 +1,5 @@
#include "test-tool.h"
-#include "config.h"
+#include "parse.h"
#include "parse-options.h"
static char const * const env__helper_usage[] = {
diff --git a/unpack-trees.c b/unpack-trees.c
index 87517364dc..761562a96e 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -2,7 +2,7 @@
#include "advice.h"
#include "strvec.h"
#include "repository.h"
-#include "config.h"
+#include "parse.h"
#include "dir.h"
#include "environment.h"
#include "gettext.h"
diff --git a/wrapper.c b/wrapper.c
index 453a20ed99..7da15a56da 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -3,7 +3,7 @@
*/
#include "git-compat-util.h"
#include "abspath.h"
-#include "config.h"
+#include "parse.h"
#include "gettext.h"
#include "repository.h"
#include "strbuf.h"
diff --git a/write-or-die.c b/write-or-die.c
index d8355c0c3e..42a2dc73cd 100644
--- a/write-or-die.c
+++ b/write-or-die.c
@@ -1,5 +1,5 @@
#include "git-compat-util.h"
-#include "config.h"
+#include "parse.h"
#include "run-command.h"
#include "write-or-die.h"
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v3 3/6] config: correct bad boolean env value error message
From: Calvin Wan @ 2023-09-08 17:44 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
An incorrectly defined boolean environment value would result in the
following error message:
bad boolean config value '%s' for '%s'
This is a misnomer since environment value != config value. Instead of
calling git_config_bool() to parse the environment value, mimic the
functionality inside of git_config_bool() but with the correct error
message.
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
config.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/config.c b/config.c
index 3846a37be9..7dde0aaa02 100644
--- a/config.c
+++ b/config.c
@@ -2133,7 +2133,14 @@ void git_global_config(char **user_out, char **xdg_out)
int git_env_bool(const char *k, int def)
{
const char *v = getenv(k);
- return v ? git_config_bool(k, v) : def;
+ int val;
+ if (!v)
+ return def;
+ val = git_parse_maybe_bool(v);
+ if (val < 0)
+ die(_("bad boolean environment value '%s' for '%s'"),
+ v, k);
+ return val;
}
/*
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v3 1/6] hex-ll: split out functionality from hex
From: Calvin Wan @ 2023-09-08 17:44 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
Separate out hex functionality that doesn't require a hash algo into
hex-ll.[ch]. Since the hash algo is currently a global that sits in
repository, this separation removes that dependency for files that only
need basic hex manipulation functions.
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
Makefile | 1 +
color.c | 2 +-
hex-ll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
hex-ll.h | 27 +++++++++++++++++++++++++++
hex.c | 47 -----------------------------------------------
hex.h | 24 +-----------------------
mailinfo.c | 2 +-
strbuf.c | 2 +-
url.c | 2 +-
urlmatch.c | 2 +-
10 files changed, 83 insertions(+), 75 deletions(-)
create mode 100644 hex-ll.c
create mode 100644 hex-ll.h
diff --git a/Makefile b/Makefile
index 5776309365..861e643708 100644
--- a/Makefile
+++ b/Makefile
@@ -1040,6 +1040,7 @@ LIB_OBJS += hash-lookup.o
LIB_OBJS += hashmap.o
LIB_OBJS += help.o
LIB_OBJS += hex.o
+LIB_OBJS += hex-ll.o
LIB_OBJS += hook.o
LIB_OBJS += ident.o
LIB_OBJS += json-writer.o
diff --git a/color.c b/color.c
index b24b19566b..f663c06ac4 100644
--- a/color.c
+++ b/color.c
@@ -3,7 +3,7 @@
#include "color.h"
#include "editor.h"
#include "gettext.h"
-#include "hex.h"
+#include "hex-ll.h"
#include "pager.h"
#include "strbuf.h"
diff --git a/hex-ll.c b/hex-ll.c
new file mode 100644
index 0000000000..4d7ece1de5
--- /dev/null
+++ b/hex-ll.c
@@ -0,0 +1,49 @@
+#include "git-compat-util.h"
+#include "hex-ll.h"
+
+const signed char hexval_table[256] = {
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 00-07 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 08-0f */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 10-17 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 18-1f */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 20-27 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 28-2f */
+ 0, 1, 2, 3, 4, 5, 6, 7, /* 30-37 */
+ 8, 9, -1, -1, -1, -1, -1, -1, /* 38-3f */
+ -1, 10, 11, 12, 13, 14, 15, -1, /* 40-47 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 48-4f */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 50-57 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 58-5f */
+ -1, 10, 11, 12, 13, 14, 15, -1, /* 60-67 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 68-67 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 70-77 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 78-7f */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 80-87 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 88-8f */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 90-97 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* 98-9f */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* a0-a7 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* a8-af */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* b0-b7 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* b8-bf */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* c0-c7 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* c8-cf */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* d0-d7 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* d8-df */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* e0-e7 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* e8-ef */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* f0-f7 */
+ -1, -1, -1, -1, -1, -1, -1, -1, /* f8-ff */
+};
+
+int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
+{
+ for (; len; len--, hex += 2) {
+ unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
+
+ if (val & ~0xff)
+ return -1;
+ *binary++ = val;
+ }
+ return 0;
+}
diff --git a/hex-ll.h b/hex-ll.h
new file mode 100644
index 0000000000..a381fa8556
--- /dev/null
+++ b/hex-ll.h
@@ -0,0 +1,27 @@
+#ifndef HEX_LL_H
+#define HEX_LL_H
+
+extern const signed char hexval_table[256];
+static inline unsigned int hexval(unsigned char c)
+{
+ return hexval_table[c];
+}
+
+/*
+ * Convert two consecutive hexadecimal digits into a char. Return a
+ * negative value on error. Don't run over the end of short strings.
+ */
+static inline int hex2chr(const char *s)
+{
+ unsigned int val = hexval(s[0]);
+ return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
+}
+
+/*
+ * Read `len` pairs of hexadecimal digits from `hex` and write the
+ * values to `binary` as `len` bytes. Return 0 on success, or -1 if
+ * the input does not consist of hex digits).
+ */
+int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
+
+#endif
diff --git a/hex.c b/hex.c
index 01f17fe5c9..d42262bdca 100644
--- a/hex.c
+++ b/hex.c
@@ -2,53 +2,6 @@
#include "hash.h"
#include "hex.h"
-const signed char hexval_table[256] = {
- -1, -1, -1, -1, -1, -1, -1, -1, /* 00-07 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 08-0f */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 10-17 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 18-1f */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 20-27 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 28-2f */
- 0, 1, 2, 3, 4, 5, 6, 7, /* 30-37 */
- 8, 9, -1, -1, -1, -1, -1, -1, /* 38-3f */
- -1, 10, 11, 12, 13, 14, 15, -1, /* 40-47 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 48-4f */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 50-57 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 58-5f */
- -1, 10, 11, 12, 13, 14, 15, -1, /* 60-67 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 68-67 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 70-77 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 78-7f */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 80-87 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 88-8f */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 90-97 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* 98-9f */
- -1, -1, -1, -1, -1, -1, -1, -1, /* a0-a7 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* a8-af */
- -1, -1, -1, -1, -1, -1, -1, -1, /* b0-b7 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* b8-bf */
- -1, -1, -1, -1, -1, -1, -1, -1, /* c0-c7 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* c8-cf */
- -1, -1, -1, -1, -1, -1, -1, -1, /* d0-d7 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* d8-df */
- -1, -1, -1, -1, -1, -1, -1, -1, /* e0-e7 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* e8-ef */
- -1, -1, -1, -1, -1, -1, -1, -1, /* f0-f7 */
- -1, -1, -1, -1, -1, -1, -1, -1, /* f8-ff */
-};
-
-int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
-{
- for (; len; len--, hex += 2) {
- unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
-
- if (val & ~0xff)
- return -1;
- *binary++ = val;
- }
- return 0;
-}
-
static int get_hash_hex_algop(const char *hex, unsigned char *hash,
const struct git_hash_algo *algop)
{
diff --git a/hex.h b/hex.h
index 87abf66602..e0b83f776f 100644
--- a/hex.h
+++ b/hex.h
@@ -2,22 +2,7 @@
#define HEX_H
#include "hash-ll.h"
-
-extern const signed char hexval_table[256];
-static inline unsigned int hexval(unsigned char c)
-{
- return hexval_table[c];
-}
-
-/*
- * Convert two consecutive hexadecimal digits into a char. Return a
- * negative value on error. Don't run over the end of short strings.
- */
-static inline int hex2chr(const char *s)
-{
- unsigned int val = hexval(s[0]);
- return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
-}
+#include "hex-ll.h"
/*
* Try to read a hash (specified by the_hash_algo) in hexadecimal
@@ -34,13 +19,6 @@ int get_oid_hex(const char *hex, struct object_id *oid);
/* Like get_oid_hex, but for an arbitrary hash algorithm. */
int get_oid_hex_algop(const char *hex, struct object_id *oid, const struct git_hash_algo *algop);
-/*
- * Read `len` pairs of hexadecimal digits from `hex` and write the
- * values to `binary` as `len` bytes. Return 0 on success, or -1 if
- * the input does not consist of hex digits).
- */
-int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
-
/*
* Convert a binary hash in "unsigned char []" or an object name in
* "struct object_id *" to its hex equivalent. The `_r` variant is reentrant,
diff --git a/mailinfo.c b/mailinfo.c
index 931505363c..a07d2da16d 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -1,7 +1,7 @@
#include "git-compat-util.h"
#include "config.h"
#include "gettext.h"
-#include "hex.h"
+#include "hex-ll.h"
#include "utf8.h"
#include "strbuf.h"
#include "mailinfo.h"
diff --git a/strbuf.c b/strbuf.c
index 4c9ac6dc5e..7827178d8e 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,6 +1,6 @@
#include "git-compat-util.h"
#include "gettext.h"
-#include "hex.h"
+#include "hex-ll.h"
#include "strbuf.h"
#include "string-list.h"
#include "utf8.h"
diff --git a/url.c b/url.c
index 2e1a9f6fee..282b12495a 100644
--- a/url.c
+++ b/url.c
@@ -1,5 +1,5 @@
#include "git-compat-util.h"
-#include "hex.h"
+#include "hex-ll.h"
#include "strbuf.h"
#include "url.h"
diff --git a/urlmatch.c b/urlmatch.c
index 1c45f23adf..1d0254abac 100644
--- a/urlmatch.c
+++ b/urlmatch.c
@@ -1,6 +1,6 @@
#include "git-compat-util.h"
#include "gettext.h"
-#include "hex.h"
+#include "hex-ll.h"
#include "strbuf.h"
#include "urlmatch.h"
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH v3 2/6] wrapper: remove dependency to Git-specific internal file
From: Calvin Wan @ 2023-09-08 17:44 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <20230908174134.1026823-1-calvinwan@google.com>
In order for wrapper.c to be built independently as part of a smaller
library, it cannot have dependencies to other Git specific
internals. remove_or_warn() creates an unnecessary dependency to
object.h in wrapper.c. Therefore move the function to entry.[ch] which
performs changes on the worktree based on the Git-specific file modes in
the index.
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
entry.c | 5 +++++
entry.h | 6 ++++++
wrapper.c | 6 ------
wrapper.h | 5 -----
4 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/entry.c b/entry.c
index 43767f9043..076e97eb89 100644
--- a/entry.c
+++ b/entry.c
@@ -581,3 +581,8 @@ void unlink_entry(const struct cache_entry *ce, const char *super_prefix)
return;
schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
+
+int remove_or_warn(unsigned int mode, const char *file)
+{
+ return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
+}
diff --git a/entry.h b/entry.h
index 7329f918a9..ca3ed35bc0 100644
--- a/entry.h
+++ b/entry.h
@@ -62,4 +62,10 @@ int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st)
void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,
struct stat *st);
+/*
+ * Calls the correct function out of {unlink,rmdir}_or_warn based on
+ * the supplied file mode.
+ */
+int remove_or_warn(unsigned int mode, const char *path);
+
#endif /* ENTRY_H */
diff --git a/wrapper.c b/wrapper.c
index 48065c4f53..453a20ed99 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -5,7 +5,6 @@
#include "abspath.h"
#include "config.h"
#include "gettext.h"
-#include "object.h"
#include "repository.h"
#include "strbuf.h"
#include "trace2.h"
@@ -632,11 +631,6 @@ int rmdir_or_warn(const char *file)
return warn_if_unremovable("rmdir", file, rmdir(file));
}
-int remove_or_warn(unsigned int mode, const char *file)
-{
- return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
-}
-
static int access_error_is_ok(int err, unsigned flag)
{
return (is_missing_file_error(err) ||
diff --git a/wrapper.h b/wrapper.h
index 79c7321bb3..1b2b047ea0 100644
--- a/wrapper.h
+++ b/wrapper.h
@@ -106,11 +106,6 @@ int unlink_or_msg(const char *file, struct strbuf *err);
* not exist.
*/
int rmdir_or_warn(const char *path);
-/*
- * Calls the correct function out of {unlink,rmdir}_or_warn based on
- * the supplied file mode.
- */
-int remove_or_warn(unsigned int mode, const char *path);
/*
* Call access(2), but warn for any error except "missing file"
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply related
* [PATCH] revision: add `--ignore-missing-links` user option
From: Karthik Nayak @ 2023-09-08 17:42 UTC (permalink / raw)
To: git; +Cc: Karthik Nayak
The revision backend is used by multiple porcelain commands such as
git-rev-list(1) and git-log(1). The backend currently supports ignoring
missing links by setting the `ignore_missing_links` bit. This allows the
revision walk to skip any objects links which are missing.
Currently there is no way to use git-rev-list(1) to traverse the objects
of the main object directory (GIT_OBJECT_DIRECTORY) and print the
boundary objects when moving from the main object directory to the
alternate object directories (GIT_ALTERNATE_OBJECT_DIRECTORIES).
By exposing this new flag `--ignore-missing-links`, users can set the
required env variables (GIT_OBJECT_DIRECTORY and
GIT_ALTERNATE_OBJECT_DIRECTORIES) along with the `--boundary` flag to
find the boundary objects between object directories.
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/rev-list-options.txt | 5 ++++
revision.c | 2 ++
t/t6022-rev-list-alternates.sh | 43 ++++++++++++++++++++++++++++++
3 files changed, 50 insertions(+)
create mode 100755 t/t6022-rev-list-alternates.sh
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b2..a0b48db8a8 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -227,6 +227,11 @@ explicitly.
Upon seeing an invalid object name in the input, pretend as if
the bad input was not given.
+--ignore-missing-links::
+ When an object points to another object that is missing, pretend as if the
+ link did not exist. These missing links are not written to stdout unless
+ the --boundary flag is passed.
+
ifndef::git-rev-list[]
--bisect::
Pretend as if the bad bisection ref `refs/bisect/bad`
diff --git a/revision.c b/revision.c
index 2f4c53ea20..cbfcbf6e28 100644
--- a/revision.c
+++ b/revision.c
@@ -2595,6 +2595,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--ignore-missing")) {
revs->ignore_missing = 1;
+ } else if (!strcmp(arg, "--ignore-missing-links")) {
+ revs->ignore_missing_links = 1;
} else if (opt && opt->allow_exclude_promisor_objects &&
!strcmp(arg, "--exclude-promisor-objects")) {
if (fetch_if_missing)
diff --git a/t/t6022-rev-list-alternates.sh b/t/t6022-rev-list-alternates.sh
new file mode 100755
index 0000000000..626ebb2dce
--- /dev/null
+++ b/t/t6022-rev-list-alternates.sh
@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='handling of alternates in rev-list'
+
+TEST_PASSES_SANITIZE_LEAK=true
+. ./test-lib.sh
+
+# We create 5 commits and move them to the alt directory and
+# create 5 more commits which will stay in the main odb.
+test_expect_success 'create repository and alternate directory' '
+ git init main &&
+ test_commit_bulk -C main 5 &&
+ mkdir alt &&
+ mv main/.git/objects/* alt &&
+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_commit_bulk --start=6 -C main 5
+'
+
+# When the alternate odb is provided, all commits are listed.
+test_expect_success 'rev-list passes with alternate object directory' '
+ GIT_ALTERNATE_OBJECT_DIRECTORIES=$PWD/alt test_stdout_line_count = 10 git -C main rev-list HEAD
+'
+
+# When the alternate odb is not provided, rev-list fails since the 5th commit's
+# parent is not present in the main odb.
+test_expect_success 'rev-list fails without alternate object directory' '
+ test_must_fail git -C main rev-list HEAD
+'
+
+# With `--ignore-missing-links`, we stop the traversal when we encounter a
+# missing link.
+test_expect_success 'rev-list only prints main odb commits with --ignore-missing-links' '
+ test_stdout_line_count = 5 git -C main rev-list --ignore-missing-links HEAD
+'
+
+# With `--ignore-missing-links` and `--boundary`, we can even print those boundary
+# commits.
+test_expect_success 'rev-list prints boundary commit with --ignore-missing-links' '
+ git -C main rev-list --ignore-missing-links --boundary HEAD >list-output &&
+ test_stdout_line_count = 6 cat list-output &&
+ test_stdout_line_count = 1 cat list-output | grep "^-"
+'
+
+test_done
--
2.41.0
^ permalink raw reply related
* [PATCH v3 0/6] Introduce Git Standard Library
From: Calvin Wan @ 2023-09-08 17:41 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <a0f04bd7-3a1e-b303-fd52-eee2af4d38b3@gmail.com>
Original cover letter:
https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/
I have taken this series out of RFC since there weren't any significant
concerns with the overall concept and design of this series. This reroll
incorporates some smaller changes such as dropping the "push pager
dependency" patch in favor of stubbing it out. The main change this
reroll cleans up the Makefile rules and stubs, as suggested by
Phillip Wood (appreciate the help on this one)!
This series has been rebased onto 1fc548b2d6a: The sixth batch
Originally this series was built on other patches that have since been
merged, which is why the range-diff is shown removing many of them.
Calvin Wan (6):
hex-ll: split out functionality from hex
wrapper: remove dependency to Git-specific internal file
config: correct bad boolean env value error message
parse: create new library for parsing strings and env values
git-std-lib: introduce git standard library
git-std-lib: add test file to call git-std-lib.a functions
Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++
Makefile | 41 ++++-
attr.c | 2 +-
color.c | 2 +-
config.c | 173 +-----------------
config.h | 14 +-
entry.c | 5 +
entry.h | 6 +
git-compat-util.h | 7 +-
hex-ll.c | 49 +++++
hex-ll.h | 27 +++
hex.c | 47 -----
hex.h | 24 +--
mailinfo.c | 2 +-
pack-objects.c | 2 +-
pack-revindex.c | 2 +-
parse-options.c | 3 +-
parse.c | 182 +++++++++++++++++++
parse.h | 20 ++
pathspec.c | 2 +-
preload-index.c | 2 +-
progress.c | 2 +-
prompt.c | 2 +-
rebase.c | 2 +-
strbuf.c | 2 +-
stubs/pager.c | 6 +
stubs/pager.h | 6 +
stubs/trace2.c | 27 +++
symlinks.c | 2 +
t/Makefile | 4 +
t/helper/test-env-helper.c | 2 +-
t/stdlib-test.c | 231 ++++++++++++++++++++++++
unpack-trees.c | 2 +-
url.c | 2 +-
urlmatch.c | 2 +-
wrapper.c | 9 +-
wrapper.h | 5 -
write-or-die.c | 2 +-
38 files changed, 824 insertions(+), 287 deletions(-)
create mode 100644 Documentation/technical/git-std-lib.txt
create mode 100644 hex-ll.c
create mode 100644 hex-ll.h
create mode 100644 parse.c
create mode 100644 parse.h
create mode 100644 stubs/pager.c
create mode 100644 stubs/pager.h
create mode 100644 stubs/trace2.c
create mode 100644 t/stdlib-test.c
Range-diff against v2:
1: 121788f263 < -: ---------- strbuf: clarify API boundary
2: 5e91404ecd < -: ---------- strbuf: clarify dependency
3: 5c05f40181 < -: ---------- abspath: move related functions to abspath
4: e1addc77e5 < -: ---------- credential-store: move related functions to credential-store file
5: 62e8c42f59 < -: ---------- object-name: move related functions to object-name
6: 0abba57acb < -: ---------- path: move related function to path
7: d33267a390 < -: ---------- strbuf: remove global variable
8: 665d2c2089 < -: ---------- init-db: document existing bug with core.bare in template config
9: 68d0a8ff16 < -: ---------- init-db: remove unnecessary global variable
10: 8c8ec85507 < -: ---------- init-db, clone: change unnecessary global into passed parameter
11: d555e2b365 < -: ---------- setup: adopt shared init-db & clone code
12: 689a7bc8aa < -: ---------- read-cache: move shared commit and ls-files code
13: 392f8e75b7 < -: ---------- add: modify add_files_to_cache() to avoid globals
14: 49ce237013 < -: ---------- read-cache: move shared add/checkout/commit code
15: c5d8370d40 < -: ---------- statinfo: move stat_{data,validity} functions from cache/read-cache
16: 90a72b6f86 < -: ---------- run-command.h: move declarations for run-command.c from cache.h
17: f27516c780 < -: ---------- name-hash.h: move declarations for name-hash.c from cache.h
18: 895c38a050 < -: ---------- sparse-index.h: move declarations for sparse-index.c from cache.h
19: 8678d4ad20 < -: ---------- preload-index.h: move declarations for preload-index.c from elsewhere
20: 4a463abaae < -: ---------- diff.h: move declaration for global in diff.c from cache.h
21: 3440e762c7 < -: ---------- merge.h: move declarations for merge.c from cache.h
22: e70853e398 < -: ---------- repository.h: move declaration of the_index from cache.h
23: ccd2014d73 < -: ---------- read-cache*.h: move declarations for read-cache.c functions from cache.h
24: d3a482afa9 < -: ---------- cache.h: remove this no-longer-used header
25: eaa087f446 < -: ---------- log-tree: replace include of revision.h with simple forward declaration
26: 5d2b0a9c75 < -: ---------- repository: remove unnecessary include of path.h
27: 250f83014e < -: ---------- diff.h: remove unnecessary include of oidset.h
28: d0f9913958 < -: ---------- list-objects-filter-options.h: remove unneccessary include
29: 03a2b2a515 < -: ---------- builtin.h: remove unneccessary includes
30: 15edc22d00 < -: ---------- git-compat-util.h: remove unneccessary include of wildmatch.h
31: e4e1bec8bd < -: ---------- merge-ll: rename from ll-merge
32: 9185495fd0 < -: ---------- khash: name the structs that khash declares
33: 15fb05e453 < -: ---------- object-store-ll.h: split this header out of object-store.h
34: 2608fe4b23 < -: ---------- hash-ll, hashmap: move oidhash() to hash-ll
35: 5e8dc5b574 < -: ---------- fsmonitor-ll.h: split this header out of fsmonitor.h
36: 37d32fc3fd < -: ---------- git-compat-util: move strbuf.c funcs to its header
37: 6ed19d5fe2 < -: ---------- git-compat-util: move wrapper.c funcs to its header
38: 555d1b8942 < -: ---------- sane-ctype.h: create header for sane-ctype macros
39: 72d591e282 < -: ---------- kwset: move translation table from ctype
40: 5d1dc2a118 < -: ---------- common.h: move non-compat specific macros and functions
41: 33e07e552e < -: ---------- git-compat-util: move usage.c funcs to its header
42: 417a8aa733 < -: ---------- treewide: remove unnecessary includes for wrapper.h
43: 65e35d00c1 < -: ---------- common: move alloc macros to common.h
44: 78634bc406 ! 1: 2f99eb2ca4 hex-ll: split out functionality from hex
@@ hex.h
+#include "hex-ll.h"
/*
- * Try to read a SHA1 in hexadecimal format from the 40 characters
-@@ hex.h: int get_oid_hex(const char *hex, struct object_id *sha1);
+ * Try to read a hash (specified by the_hash_algo) in hexadecimal
+@@ hex.h: int get_oid_hex(const char *hex, struct object_id *oid);
/* Like get_oid_hex, but for an arbitrary hash algorithm. */
int get_oid_hex_algop(const char *hex, struct object_id *oid, const struct git_hash_algo *algop);
45: 21ec1d276e ! 2: 7b2d123628 object: move function to object.c
@@ Metadata
Author: Calvin Wan <calvinwan@google.com>
## Commit message ##
- object: move function to object.c
+ wrapper: remove dependency to Git-specific internal file
- While remove_or_warn() is a simple ternary operator to call two other
- wrapper functions, it creates an unnecessary dependency to object.h in
- wrapper.c. Therefore move the function to object.[ch] where the concept
- of GITLINKs is first defined.
+ In order for wrapper.c to be built independently as part of a smaller
+ library, it cannot have dependencies to other Git specific
+ internals. remove_or_warn() creates an unnecessary dependency to
+ object.h in wrapper.c. Therefore move the function to entry.[ch] which
+ performs changes on the worktree based on the Git-specific file modes in
+ the index.
- ## object.c ##
-@@ object.c: void parsed_object_pool_clear(struct parsed_object_pool *o)
- FREE_AND_NULL(o->object_state);
- FREE_AND_NULL(o->shallow_stat);
+ ## entry.c ##
+@@ entry.c: void unlink_entry(const struct cache_entry *ce, const char *super_prefix)
+ return;
+ schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
+
+int remove_or_warn(unsigned int mode, const char *file)
@@ object.c: void parsed_object_pool_clear(struct parsed_object_pool *o)
+ return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
+}
- ## object.h ##
-@@ object.h: void clear_object_flags(unsigned flags);
- */
- void repo_clear_commit_marks(struct repository *r, unsigned int flags);
+ ## entry.h ##
+@@ entry.h: int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st)
+ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,
+ struct stat *st);
+/*
+ * Calls the correct function out of {unlink,rmdir}_or_warn based on
@@ object.h: void clear_object_flags(unsigned flags);
+ */
+int remove_or_warn(unsigned int mode, const char *path);
+
- #endif /* OBJECT_H */
+ #endif /* ENTRY_H */
## wrapper.c ##
@@
46: 41dcf8107c = 3: b37beb206a config: correct bad boolean env value error message
47: 3e800a41c4 ! 4: 3a827cf45c parse: create new library for parsing strings and env values
@@ Commit message
config.c, there are other files that only need parsing functionality and
not config functionality. By separating out string and environment value
parsing from config, those files can instead be dependent on parse,
- which has a much smaller dependency chain than config.
+ which has a much smaller dependency chain than config. This ultimately
+ allows us to inclue parse.[ch] in an independent library since it
+ doesn't have dependencies to Git-specific internals unlike in
+ config.[ch].
Move general string and env parsing functions from config.[ch] to
parse.[ch].
@@ config.c: static int git_parse_source(struct config_source *cs, config_fn_t fn,
- return 1;
-}
-
- static int reader_config_name(struct config_reader *reader, const char **out);
- static int reader_origin_type(struct config_reader *reader,
- enum config_origin_type *type);
-@@ config.c: ssize_t git_config_ssize_t(const char *name, const char *value)
+ NORETURN
+ static void die_bad_number(const char *name, const char *value,
+ const struct key_value_info *kvi)
+@@ config.c: ssize_t git_config_ssize_t(const char *name, const char *value,
return ret;
}
@@ config.c: static enum fsync_component parse_fsync_components(const char *var, co
- return -1;
-}
-
- int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
+ int git_config_bool_or_int(const char *name, const char *value,
+ const struct key_value_info *kvi, int *is_bool)
{
- int v = git_parse_maybe_bool_text(value);
@@ config.c: void git_global_config(char **user_out, char **xdg_out)
*xdg_out = xdg_config;
}
@@ config.c: void git_global_config(char **user_out, char **xdg_out)
## config.h ##
@@
-
#include "hashmap.h"
#include "string-list.h"
+ #include "repository.h"
-
+#include "parse.h"
48: 7a4a088bc3 < -: ---------- date: push pager.h dependency up
49: c9002734d0 ! 5: f8e4ac50a0 git-std-lib: introduce git standard library
@@ Documentation/technical/git-std-lib.txt (new)
+Rationale behind Git Standard Library
+================
+
-+The rationale behind Git Standard Library essentially is the result of
-+two observations within the Git codebase: every file includes
-+git-compat-util.h which defines functions in a couple of different
-+files, and wrapper.c + usage.c have difficult-to-separate circular
-+dependencies with each other and other files.
++The rationale behind what's in and what's not in the Git Standard
++Library essentially is the result of two observations within the Git
++codebase: every file includes git-compat-util.h which defines functions
++in a couple of different files, and wrapper.c + usage.c have
++difficult-to-separate circular dependencies with each other and other
++files.
+
+Ubiquity of git-compat-util.h and circular dependencies
+========
@@ Documentation/technical/git-std-lib.txt (new)
+ - low-level git/* files with functions defined in git-compat-util.h
+ (ctype.c)
+ - compat/*
-+ - stubbed out dependencies in stubs/ (stubs/repository.c, stubs/trace2.c)
++ - stubbed out dependencies in stubs/ (stubs/pager.c, stubs/trace2.c)
+
+There are other files that might fit this definition, but that does not
+mean it should belong in git-std-lib.a. Those files should start as
+their own separate library since any file added to git-std-lib.a loses
+its flexibility of being easily swappable.
+
-+Wrapper.c and usage.c have dependencies on repository and trace2 that are
++Wrapper.c and usage.c have dependencies on pager and trace2 that are
+possible to remove at the cost of sacrificing the ability for standard Git
+to be able to trace functions in those files and other files in git-std-lib.a.
+In order for git-std-lib.a to compile with those dependencies, stubbed out
@@ Documentation/technical/git-std-lib.txt (new)
+usage.c
+utf8.c
+wrapper.c
-+stubs/repository.c
-+stubs/trace2.c
+relevant compat/ files
+
++When these files are compiled together with the following files (or
++user-provided files that provide the same functions), they form a
++complete library:
++stubs/pager.c
++stubs/trace2.c
++
+Pitfalls
+================
+
@@ Makefile: LIB_OBJS += write-or-die.o
+LIB_OBJS += utf8.o
+LIB_OBJS += wrapper.o
+
-+ifdef STUB_REPOSITORY
-+STUB_OBJS += stubs/repository.o
-+endif
-+
+ifdef STUB_TRACE2
+STUB_OBJS += stubs/trace2.o
+endif
+
++ifdef STUB_PAGER
++STUB_OBJS += stubs/pager.o
++endif
++
+LIB_OBJS += $(STUB_OBJS)
+endif
@@ Makefile: ifdef FSMONITOR_OS_SETTINGS
NO_TCLTK = NoThanks
endif
@@ Makefile: clean: profile-clean coverage-clean cocciclean
- $(RM) po/git.pot po/git-core.pot
$(RM) git.res
$(RM) $(OBJECTS)
+ $(RM) headless-git.o
- $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
+ $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB) $(STD_LIB_FILE)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
@@ Makefile: $(FUZZ_PROGRAMS): all
+### Libified Git rules
+
+# git-std-lib
-+# `make git-std-lib GIT_STD_LIB=YesPlease STUB_REPOSITORY=YesPlease STUB_TRACE2=YesPlease`
++# `make git-std-lib.a GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease STUB_PAGER=YesPlease`
+STD_LIB = git-std-lib.a
+
+$(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
+ $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
-+
-+TEMP_HEADERS = temp_headers/
-+
-+git-std-lib:
-+# Move headers to temporary folder and replace them with stubbed headers.
-+# After building, move headers and stubbed headers back.
-+ifneq ($(STUB_OBJS),)
-+ mkdir -p $(TEMP_HEADERS); \
-+ for d in $(STUB_OBJS); do \
-+ BASE=$${d%.*}; \
-+ mv $${BASE##*/}.h $(TEMP_HEADERS)$${BASE##*/}.h; \
-+ mv $${BASE}.h $${BASE##*/}.h; \
-+ done; \
-+ $(MAKE) $(STD_LIB); \
-+ for d in $(STUB_OBJS); do \
-+ BASE=$${d%.*}; \
-+ mv $${BASE##*/}.h $${BASE}.h; \
-+ mv $(TEMP_HEADERS)$${BASE##*/}.h $${BASE##*/}.h; \
-+ done; \
-+ rm -rf temp_headers
-+else
-+ $(MAKE) $(STD_LIB)
-+endif
## git-compat-util.h ##
@@ git-compat-util.h: static inline int noop_core_config(const char *var UNUSED,
@@ git-compat-util.h: const char *inet_ntop(int af, const void *src, char *dst, siz
#endif
+#endif
- /*
- * Limit size of IO chunks, because huge chunks only cause pain. OS X
-@@ git-compat-util.h: int git_access(const char *path, int mode);
- # endif
- #endif
+ static inline size_t st_add(size_t a, size_t b)
+ {
+@@ git-compat-util.h: static inline int is_missing_file_error(int errno_)
+ return (errno_ == ENOENT || errno_ == ENOTDIR);
+ }
+#ifndef GIT_STD_LIB
int cmd_main(int, const char **);
@@ git-compat-util.h: int git_access(const char *path, int mode);
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
- ## stubs/repository.c (new) ##
+ ## stubs/pager.c (new) ##
@@
-+#include "git-compat-util.h"
-+#include "repository.h"
++#include "pager.h"
+
-+struct repository *the_repository;
++int pager_in_use(void)
++{
++ return 0;
++}
- ## stubs/repository.h (new) ##
+ ## stubs/pager.h (new) ##
@@
-+#ifndef REPOSITORY_H
-+#define REPOSITORY_H
++#ifndef PAGER_H
++#define PAGER_H
+
-+struct repository { int stub; };
++int pager_in_use(void);
+
-+extern struct repository *the_repository;
-+
-+#endif /* REPOSITORY_H */
++#endif /* PAGER_H */
## stubs/trace2.c (new) ##
@@
+#include "git-compat-util.h"
+#include "trace2.h"
+
++struct child_process { int stub; };
++struct repository { int stub; };
++struct json_writer { int stub; };
++
+void trace2_region_enter_fl(const char *file, int line, const char *category,
+ const char *label, const struct repository *repo, ...) { }
+void trace2_region_leave_fl(const char *file, int line, const char *category,
@@ stubs/trace2.c (new)
+ const struct repository *repo, const char *key,
+ intmax_t value) { }
+int trace2_is_enabled(void) { return 0; }
++void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
+void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
- ## stubs/trace2.h (new) ##
-@@
-+#ifndef TRACE2_H
-+#define TRACE2_H
-+
-+struct child_process { int stub; };
-+struct repository;
-+struct json_writer { int stub; };
-+
-+void trace2_region_enter_fl(const char *file, int line, const char *category,
-+ const char *label, const struct repository *repo, ...);
-+
-+#define trace2_region_enter(category, label, repo) \
-+ trace2_region_enter_fl(__FILE__, __LINE__, (category), (label), (repo))
-+
-+void trace2_region_leave_fl(const char *file, int line, const char *category,
-+ const char *label, const struct repository *repo, ...);
-+
-+#define trace2_region_leave(category, label, repo) \
-+ trace2_region_leave_fl(__FILE__, __LINE__, (category), (label), (repo))
-+
-+void trace2_data_string_fl(const char *file, int line, const char *category,
-+ const struct repository *repo, const char *key,
-+ const char *value);
-+
-+#define trace2_data_string(category, repo, key, value) \
-+ trace2_data_string_fl(__FILE__, __LINE__, (category), (repo), (key), \
-+ (value))
-+
-+void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names);
-+
-+#define trace2_cmd_ancestry(v) trace2_cmd_ancestry_fl(__FILE__, __LINE__, (v))
-+
-+void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
-+ va_list ap);
-+
-+#define trace2_cmd_error_va(fmt, ap) \
-+ trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
-+
-+
-+void trace2_cmd_name_fl(const char *file, int line, const char *name);
-+
-+#define trace2_cmd_name(v) trace2_cmd_name_fl(__FILE__, __LINE__, (v))
-+
-+void trace2_thread_start_fl(const char *file, int line,
-+ const char *thread_base_name);
-+
-+#define trace2_thread_start(thread_base_name) \
-+ trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
-+
-+void trace2_thread_exit_fl(const char *file, int line);
-+
-+#define trace2_thread_exit() trace2_thread_exit_fl(__FILE__, __LINE__)
-+
-+void trace2_data_intmax_fl(const char *file, int line, const char *category,
-+ const struct repository *repo, const char *key,
-+ intmax_t value);
-+
-+#define trace2_data_intmax(category, repo, key, value) \
-+ trace2_data_intmax_fl(__FILE__, __LINE__, (category), (repo), (key), \
-+ (value))
-+
-+enum trace2_process_info_reason {
-+ TRACE2_PROCESS_INFO_STARTUP,
-+ TRACE2_PROCESS_INFO_EXIT,
-+};
-+int trace2_is_enabled(void);
-+void trace2_collect_process_info(enum trace2_process_info_reason reason);
-+
-+#endif /* TRACE2_H */
-+
-
## symlinks.c ##
@@ symlinks.c: void invalidate_lstat_cache(void)
reset_lstat_cache(&default_cache);
@@ symlinks.c: int lstat_cache_aware_rmdir(const char *path)
return ret;
}
+#endif
+
+ ## wrapper.c ##
+@@
+ #include "abspath.h"
+ #include "parse.h"
+ #include "gettext.h"
+-#include "repository.h"
+ #include "strbuf.h"
+ #include "trace2.h"
+
50: 0bead8f980 ! 6: 7840e1830a git-std-lib: add test file to call git-std-lib.a functions
@@ t/stdlib-test.c (new)
+ struct strbuf sb3 = STRBUF_INIT;
+ struct string_list list = STRING_LIST_INIT_NODUP;
+ char *buf = "foo";
-+ struct strbuf_expand_dict_entry dict[] = {
-+ { "foo", NULL, },
-+ { "bar", NULL, },
-+ };
+ int fd = open("/dev/null", O_RDONLY);
+
+ fprintf(stderr, "calling strbuf functions\n");
@@ t/stdlib-test.c (new)
+ strbuf_add_commented_lines(sb, "foo", 3, '#');
+ strbuf_commented_addf(sb, '#', "%s", "foo");
+ // strbuf_vaddf() called by strbuf_addf()
-+ strbuf_expand(sb, "%s", strbuf_expand_literal_cb, NULL);
-+ strbuf_expand(sb, "%s", strbuf_expand_dict_cb, &dict);
-+ // strbuf_expand_literal_cb() called by strbuf_expand()
-+ // strbuf_expand_dict_cb() called by strbuf_expand()
+ strbuf_addbuf_percentquote(sb, &sb3);
+ strbuf_add_percentencode(sb, "foo", STRBUF_ENCODE_SLASH);
+ strbuf_fread(sb, 0, stdin);
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Junio C Hamano @ 2023-09-08 17:38 UTC (permalink / raw)
To: Phillip Wood; +Cc: Phillip Wood via GitGitGadget, git, Eric Wong, Phillip Wood
In-Reply-To: <376d3ea0-a3eb-4b25-8bf2-ca40c4699e26@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> Oh I should have looked more carefully at the existing uses. It looks
> like it is only my sequencer patch that does
>
> sigchain_push(SIGINT, SIG_IGN);
> sigchain_push(SIGQUIT, SIG_IGN);
> res = run_command(...);
Hmph, does it mean this patch would become unnecessary, once you fix
the above sequence to follow the pattern "to spawn and then ignore"?
^ permalink raw reply
* [PATCH v3 0/6] Introduce Git Standard Library
From: Calvin Wan @ 2023-09-08 17:03 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, nasamuffin, jonathantanmy, linusa, phillip.wood123,
vdye
In-Reply-To: <https://lore.kernel.org/git/20230810163346.274132-1-calvinwan@google.com/>
Original cover letter:
https://lore.kernel.org/git/20230627195251.1973421-1-calvinwan@google.com/
I have taken this series out of RFC since there weren't any significant
concerns with the overall concept and design of this series. This reroll
incorporates some smaller changes such as dropping the "push pager
dependency" patch in favor of stubbing it out. The main change this
reroll is cleaning up the Makefile rules and stubs, as suggested by
Phillip Wood (appreciate the help on this one)!
This series has been rebased onto 1fc548b2d6a: The sixth batch
Originally this series was built on other patches that have since been
merged, which is why the range-diff is shown removing many of them.
Calvin Wan (6):
hex-ll: split out functionality from hex
wrapper: remove dependency to Git-specific internal file
config: correct bad boolean env value error message
parse: create new library for parsing strings and env values
git-std-lib: introduce git standard library
git-std-lib: add test file to call git-std-lib.a functions
Documentation/technical/git-std-lib.txt | 191 ++++++++++++++++++++
Makefile | 41 ++++-
attr.c | 2 +-
color.c | 2 +-
config.c | 173 +-----------------
config.h | 14 +-
entry.c | 5 +
entry.h | 6 +
git-compat-util.h | 7 +-
hex-ll.c | 49 +++++
hex-ll.h | 27 +++
hex.c | 47 -----
hex.h | 24 +--
mailinfo.c | 2 +-
pack-objects.c | 2 +-
pack-revindex.c | 2 +-
parse-options.c | 3 +-
parse.c | 182 +++++++++++++++++++
parse.h | 20 ++
pathspec.c | 2 +-
preload-index.c | 2 +-
progress.c | 2 +-
prompt.c | 2 +-
rebase.c | 2 +-
strbuf.c | 2 +-
stubs/pager.c | 6 +
stubs/pager.h | 6 +
stubs/trace2.c | 27 +++
symlinks.c | 2 +
t/Makefile | 4 +
t/helper/test-env-helper.c | 2 +-
t/stdlib-test.c | 231 ++++++++++++++++++++++++
unpack-trees.c | 2 +-
url.c | 2 +-
urlmatch.c | 2 +-
wrapper.c | 9 +-
wrapper.h | 5 -
write-or-die.c | 2 +-
38 files changed, 824 insertions(+), 287 deletions(-)
create mode 100644 Documentation/technical/git-std-lib.txt
create mode 100644 hex-ll.c
create mode 100644 hex-ll.h
create mode 100644 parse.c
create mode 100644 parse.h
create mode 100644 stubs/pager.c
create mode 100644 stubs/pager.h
create mode 100644 stubs/trace2.c
create mode 100644 t/stdlib-test.c
Range-diff against v2:
1: 121788f263 < -: ---------- strbuf: clarify API boundary
2: 5e91404ecd < -: ---------- strbuf: clarify dependency
3: 5c05f40181 < -: ---------- abspath: move related functions to abspath
4: e1addc77e5 < -: ---------- credential-store: move related functions to credential-store file
5: 62e8c42f59 < -: ---------- object-name: move related functions to object-name
6: 0abba57acb < -: ---------- path: move related function to path
7: d33267a390 < -: ---------- strbuf: remove global variable
8: 665d2c2089 < -: ---------- init-db: document existing bug with core.bare in template config
9: 68d0a8ff16 < -: ---------- init-db: remove unnecessary global variable
10: 8c8ec85507 < -: ---------- init-db, clone: change unnecessary global into passed parameter
11: d555e2b365 < -: ---------- setup: adopt shared init-db & clone code
12: 689a7bc8aa < -: ---------- read-cache: move shared commit and ls-files code
13: 392f8e75b7 < -: ---------- add: modify add_files_to_cache() to avoid globals
14: 49ce237013 < -: ---------- read-cache: move shared add/checkout/commit code
15: c5d8370d40 < -: ---------- statinfo: move stat_{data,validity} functions from cache/read-cache
16: 90a72b6f86 < -: ---------- run-command.h: move declarations for run-command.c from cache.h
17: f27516c780 < -: ---------- name-hash.h: move declarations for name-hash.c from cache.h
18: 895c38a050 < -: ---------- sparse-index.h: move declarations for sparse-index.c from cache.h
19: 8678d4ad20 < -: ---------- preload-index.h: move declarations for preload-index.c from elsewhere
20: 4a463abaae < -: ---------- diff.h: move declaration for global in diff.c from cache.h
21: 3440e762c7 < -: ---------- merge.h: move declarations for merge.c from cache.h
22: e70853e398 < -: ---------- repository.h: move declaration of the_index from cache.h
23: ccd2014d73 < -: ---------- read-cache*.h: move declarations for read-cache.c functions from cache.h
24: d3a482afa9 < -: ---------- cache.h: remove this no-longer-used header
25: eaa087f446 < -: ---------- log-tree: replace include of revision.h with simple forward declaration
26: 5d2b0a9c75 < -: ---------- repository: remove unnecessary include of path.h
27: 250f83014e < -: ---------- diff.h: remove unnecessary include of oidset.h
28: d0f9913958 < -: ---------- list-objects-filter-options.h: remove unneccessary include
29: 03a2b2a515 < -: ---------- builtin.h: remove unneccessary includes
30: 15edc22d00 < -: ---------- git-compat-util.h: remove unneccessary include of wildmatch.h
31: e4e1bec8bd < -: ---------- merge-ll: rename from ll-merge
32: 9185495fd0 < -: ---------- khash: name the structs that khash declares
33: 15fb05e453 < -: ---------- object-store-ll.h: split this header out of object-store.h
34: 2608fe4b23 < -: ---------- hash-ll, hashmap: move oidhash() to hash-ll
35: 5e8dc5b574 < -: ---------- fsmonitor-ll.h: split this header out of fsmonitor.h
36: 37d32fc3fd < -: ---------- git-compat-util: move strbuf.c funcs to its header
37: 6ed19d5fe2 < -: ---------- git-compat-util: move wrapper.c funcs to its header
38: 555d1b8942 < -: ---------- sane-ctype.h: create header for sane-ctype macros
39: 72d591e282 < -: ---------- kwset: move translation table from ctype
40: 5d1dc2a118 < -: ---------- common.h: move non-compat specific macros and functions
41: 33e07e552e < -: ---------- git-compat-util: move usage.c funcs to its header
42: 417a8aa733 < -: ---------- treewide: remove unnecessary includes for wrapper.h
43: 65e35d00c1 < -: ---------- common: move alloc macros to common.h
44: 78634bc406 ! 1: 2f99eb2ca4 hex-ll: split out functionality from hex
@@ hex.h
+#include "hex-ll.h"
/*
- * Try to read a SHA1 in hexadecimal format from the 40 characters
-@@ hex.h: int get_oid_hex(const char *hex, struct object_id *sha1);
+ * Try to read a hash (specified by the_hash_algo) in hexadecimal
+@@ hex.h: int get_oid_hex(const char *hex, struct object_id *oid);
/* Like get_oid_hex, but for an arbitrary hash algorithm. */
int get_oid_hex_algop(const char *hex, struct object_id *oid, const struct git_hash_algo *algop);
45: 21ec1d276e ! 2: 7b2d123628 object: move function to object.c
@@ Metadata
Author: Calvin Wan <calvinwan@google.com>
## Commit message ##
- object: move function to object.c
+ wrapper: remove dependency to Git-specific internal file
- While remove_or_warn() is a simple ternary operator to call two other
- wrapper functions, it creates an unnecessary dependency to object.h in
- wrapper.c. Therefore move the function to object.[ch] where the concept
- of GITLINKs is first defined.
+ In order for wrapper.c to be built independently as part of a smaller
+ library, it cannot have dependencies to other Git specific
+ internals. remove_or_warn() creates an unnecessary dependency to
+ object.h in wrapper.c. Therefore move the function to entry.[ch] which
+ performs changes on the worktree based on the Git-specific file modes in
+ the index.
- ## object.c ##
-@@ object.c: void parsed_object_pool_clear(struct parsed_object_pool *o)
- FREE_AND_NULL(o->object_state);
- FREE_AND_NULL(o->shallow_stat);
+ ## entry.c ##
+@@ entry.c: void unlink_entry(const struct cache_entry *ce, const char *super_prefix)
+ return;
+ schedule_dir_for_removal(ce->name, ce_namelen(ce));
}
+
+int remove_or_warn(unsigned int mode, const char *file)
@@ object.c: void parsed_object_pool_clear(struct parsed_object_pool *o)
+ return S_ISGITLINK(mode) ? rmdir_or_warn(file) : unlink_or_warn(file);
+}
- ## object.h ##
-@@ object.h: void clear_object_flags(unsigned flags);
- */
- void repo_clear_commit_marks(struct repository *r, unsigned int flags);
+ ## entry.h ##
+@@ entry.h: int fstat_checkout_output(int fd, const struct checkout *state, struct stat *st)
+ void update_ce_after_write(const struct checkout *state, struct cache_entry *ce,
+ struct stat *st);
+/*
+ * Calls the correct function out of {unlink,rmdir}_or_warn based on
@@ object.h: void clear_object_flags(unsigned flags);
+ */
+int remove_or_warn(unsigned int mode, const char *path);
+
- #endif /* OBJECT_H */
+ #endif /* ENTRY_H */
## wrapper.c ##
@@
46: 41dcf8107c = 3: b37beb206a config: correct bad boolean env value error message
47: 3e800a41c4 ! 4: 3a827cf45c parse: create new library for parsing strings and env values
@@ Commit message
config.c, there are other files that only need parsing functionality and
not config functionality. By separating out string and environment value
parsing from config, those files can instead be dependent on parse,
- which has a much smaller dependency chain than config.
+ which has a much smaller dependency chain than config. This ultimately
+ allows us to inclue parse.[ch] in an independent library since it
+ doesn't have dependencies to Git-specific internals unlike in
+ config.[ch].
Move general string and env parsing functions from config.[ch] to
parse.[ch].
@@ config.c: static int git_parse_source(struct config_source *cs, config_fn_t fn,
- return 1;
-}
-
- static int reader_config_name(struct config_reader *reader, const char **out);
- static int reader_origin_type(struct config_reader *reader,
- enum config_origin_type *type);
-@@ config.c: ssize_t git_config_ssize_t(const char *name, const char *value)
+ NORETURN
+ static void die_bad_number(const char *name, const char *value,
+ const struct key_value_info *kvi)
+@@ config.c: ssize_t git_config_ssize_t(const char *name, const char *value,
return ret;
}
@@ config.c: static enum fsync_component parse_fsync_components(const char *var, co
- return -1;
-}
-
- int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
+ int git_config_bool_or_int(const char *name, const char *value,
+ const struct key_value_info *kvi, int *is_bool)
{
- int v = git_parse_maybe_bool_text(value);
@@ config.c: void git_global_config(char **user_out, char **xdg_out)
*xdg_out = xdg_config;
}
@@ config.c: void git_global_config(char **user_out, char **xdg_out)
## config.h ##
@@
-
#include "hashmap.h"
#include "string-list.h"
+ #include "repository.h"
-
+#include "parse.h"
48: 7a4a088bc3 < -: ---------- date: push pager.h dependency up
49: c9002734d0 ! 5: b0e8739ced git-std-lib: introduce git standard library
@@ Documentation/technical/git-std-lib.txt (new)
+Rationale behind Git Standard Library
+================
+
-+The rationale behind Git Standard Library essentially is the result of
-+two observations within the Git codebase: every file includes
-+git-compat-util.h which defines functions in a couple of different
-+files, and wrapper.c + usage.c have difficult-to-separate circular
-+dependencies with each other and other files.
++The rationale behind what's in and what's not in the Git Standard
++Library essentially is the result of two observations within the Git
++codebase: every file includes git-compat-util.h which defines functions
++in a couple of different files, and wrapper.c + usage.c have
++difficult-to-separate circular dependencies with each other and other
++files.
+
+Ubiquity of git-compat-util.h and circular dependencies
+========
@@ Documentation/technical/git-std-lib.txt (new)
+ - low-level git/* files with functions defined in git-compat-util.h
+ (ctype.c)
+ - compat/*
-+ - stubbed out dependencies in stubs/ (stubs/repository.c, stubs/trace2.c)
++ - stubbed out dependencies in stubs/ (stubs/pager.c, stubs/trace2.c)
+
+There are other files that might fit this definition, but that does not
+mean it should belong in git-std-lib.a. Those files should start as
+their own separate library since any file added to git-std-lib.a loses
+its flexibility of being easily swappable.
+
-+Wrapper.c and usage.c have dependencies on repository and trace2 that are
++Wrapper.c and usage.c have dependencies on pager and trace2 that are
+possible to remove at the cost of sacrificing the ability for standard Git
+to be able to trace functions in those files and other files in git-std-lib.a.
+In order for git-std-lib.a to compile with those dependencies, stubbed out
@@ Documentation/technical/git-std-lib.txt (new)
+usage.c
+utf8.c
+wrapper.c
-+stubs/repository.c
-+stubs/trace2.c
+relevant compat/ files
+
++When these files are compiled together with the following files (or
++user-provided files that provide the same functions), they form a
++complete library:
++stubs/pager.c
++stubs/trace2.c
++
+Pitfalls
+================
+
@@ Makefile: LIB_OBJS += write-or-die.o
+LIB_OBJS += utf8.o
+LIB_OBJS += wrapper.o
+
-+ifdef STUB_REPOSITORY
-+STUB_OBJS += stubs/repository.o
-+endif
-+
+ifdef STUB_TRACE2
+STUB_OBJS += stubs/trace2.o
+endif
+
++ifdef STUB_PAGER
++STUB_OBJS += stubs/pager.o
++endif
++
+LIB_OBJS += $(STUB_OBJS)
+endif
@@ Makefile: ifdef FSMONITOR_OS_SETTINGS
NO_TCLTK = NoThanks
endif
@@ Makefile: clean: profile-clean coverage-clean cocciclean
- $(RM) po/git.pot po/git-core.pot
$(RM) git.res
$(RM) $(OBJECTS)
+ $(RM) headless-git.o
- $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB)
+ $(RM) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(REFTABLE_TEST_LIB) $(STD_LIB_FILE)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
@@ Makefile: $(FUZZ_PROGRAMS): all
+### Libified Git rules
+
+# git-std-lib
-+# `make git-std-lib GIT_STD_LIB=YesPlease STUB_REPOSITORY=YesPlease STUB_TRACE2=YesPlease`
++# `make git-std-lib GIT_STD_LIB=YesPlease STUB_TRACE2=YesPlease STUB_PAGER=YesPlease`
+STD_LIB = git-std-lib.a
+
+$(STD_LIB): $(LIB_OBJS) $(COMPAT_OBJS) $(STUB_OBJS)
+ $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
-+
-+TEMP_HEADERS = temp_headers/
-+
-+git-std-lib:
-+# Move headers to temporary folder and replace them with stubbed headers.
-+# After building, move headers and stubbed headers back.
-+ifneq ($(STUB_OBJS),)
-+ mkdir -p $(TEMP_HEADERS); \
-+ for d in $(STUB_OBJS); do \
-+ BASE=$${d%.*}; \
-+ mv $${BASE##*/}.h $(TEMP_HEADERS)$${BASE##*/}.h; \
-+ mv $${BASE}.h $${BASE##*/}.h; \
-+ done; \
-+ $(MAKE) $(STD_LIB); \
-+ for d in $(STUB_OBJS); do \
-+ BASE=$${d%.*}; \
-+ mv $${BASE##*/}.h $${BASE}.h; \
-+ mv $(TEMP_HEADERS)$${BASE##*/}.h $${BASE##*/}.h; \
-+ done; \
-+ rm -rf temp_headers
-+else
-+ $(MAKE) $(STD_LIB)
-+endif
## git-compat-util.h ##
@@ git-compat-util.h: static inline int noop_core_config(const char *var UNUSED,
@@ git-compat-util.h: const char *inet_ntop(int af, const void *src, char *dst, siz
#endif
+#endif
- /*
- * Limit size of IO chunks, because huge chunks only cause pain. OS X
-@@ git-compat-util.h: int git_access(const char *path, int mode);
- # endif
- #endif
+ static inline size_t st_add(size_t a, size_t b)
+ {
+@@ git-compat-util.h: static inline int is_missing_file_error(int errno_)
+ return (errno_ == ENOENT || errno_ == ENOTDIR);
+ }
+#ifndef GIT_STD_LIB
int cmd_main(int, const char **);
@@ git-compat-util.h: int git_access(const char *path, int mode);
/*
* You can mark a stack variable with UNLEAK(var) to avoid it being
- ## stubs/repository.c (new) ##
+ ## stubs/pager.c (new) ##
@@
-+#include "git-compat-util.h"
-+#include "repository.h"
++#include "pager.h"
+
-+struct repository *the_repository;
++int pager_in_use(void)
++{
++ return 0;
++}
- ## stubs/repository.h (new) ##
+ ## stubs/pager.h (new) ##
@@
-+#ifndef REPOSITORY_H
-+#define REPOSITORY_H
++#ifndef PAGER_H
++#define PAGER_H
+
-+struct repository { int stub; };
++int pager_in_use(void);
+
-+extern struct repository *the_repository;
-+
-+#endif /* REPOSITORY_H */
++#endif /* PAGER_H */
## stubs/trace2.c (new) ##
@@
+#include "git-compat-util.h"
+#include "trace2.h"
+
++struct child_process { int stub; };
++struct repository { int stub; };
++struct json_writer { int stub; };
++
+void trace2_region_enter_fl(const char *file, int line, const char *category,
+ const char *label, const struct repository *repo, ...) { }
+void trace2_region_leave_fl(const char *file, int line, const char *category,
@@ stubs/trace2.c (new)
+ const struct repository *repo, const char *key,
+ intmax_t value) { }
+int trace2_is_enabled(void) { return 0; }
++void trace2_counter_add(enum trace2_counter_id cid, uint64_t value) { }
+void trace2_collect_process_info(enum trace2_process_info_reason reason) { }
- ## stubs/trace2.h (new) ##
-@@
-+#ifndef TRACE2_H
-+#define TRACE2_H
-+
-+struct child_process { int stub; };
-+struct repository;
-+struct json_writer { int stub; };
-+
-+void trace2_region_enter_fl(const char *file, int line, const char *category,
-+ const char *label, const struct repository *repo, ...);
-+
-+#define trace2_region_enter(category, label, repo) \
-+ trace2_region_enter_fl(__FILE__, __LINE__, (category), (label), (repo))
-+
-+void trace2_region_leave_fl(const char *file, int line, const char *category,
-+ const char *label, const struct repository *repo, ...);
-+
-+#define trace2_region_leave(category, label, repo) \
-+ trace2_region_leave_fl(__FILE__, __LINE__, (category), (label), (repo))
-+
-+void trace2_data_string_fl(const char *file, int line, const char *category,
-+ const struct repository *repo, const char *key,
-+ const char *value);
-+
-+#define trace2_data_string(category, repo, key, value) \
-+ trace2_data_string_fl(__FILE__, __LINE__, (category), (repo), (key), \
-+ (value))
-+
-+void trace2_cmd_ancestry_fl(const char *file, int line, const char **parent_names);
-+
-+#define trace2_cmd_ancestry(v) trace2_cmd_ancestry_fl(__FILE__, __LINE__, (v))
-+
-+void trace2_cmd_error_va_fl(const char *file, int line, const char *fmt,
-+ va_list ap);
-+
-+#define trace2_cmd_error_va(fmt, ap) \
-+ trace2_cmd_error_va_fl(__FILE__, __LINE__, (fmt), (ap))
-+
-+
-+void trace2_cmd_name_fl(const char *file, int line, const char *name);
-+
-+#define trace2_cmd_name(v) trace2_cmd_name_fl(__FILE__, __LINE__, (v))
-+
-+void trace2_thread_start_fl(const char *file, int line,
-+ const char *thread_base_name);
-+
-+#define trace2_thread_start(thread_base_name) \
-+ trace2_thread_start_fl(__FILE__, __LINE__, (thread_base_name))
-+
-+void trace2_thread_exit_fl(const char *file, int line);
-+
-+#define trace2_thread_exit() trace2_thread_exit_fl(__FILE__, __LINE__)
-+
-+void trace2_data_intmax_fl(const char *file, int line, const char *category,
-+ const struct repository *repo, const char *key,
-+ intmax_t value);
-+
-+#define trace2_data_intmax(category, repo, key, value) \
-+ trace2_data_intmax_fl(__FILE__, __LINE__, (category), (repo), (key), \
-+ (value))
-+
-+enum trace2_process_info_reason {
-+ TRACE2_PROCESS_INFO_STARTUP,
-+ TRACE2_PROCESS_INFO_EXIT,
-+};
-+int trace2_is_enabled(void);
-+void trace2_collect_process_info(enum trace2_process_info_reason reason);
-+
-+#endif /* TRACE2_H */
-+
-
## symlinks.c ##
@@ symlinks.c: void invalidate_lstat_cache(void)
reset_lstat_cache(&default_cache);
@@ symlinks.c: int lstat_cache_aware_rmdir(const char *path)
return ret;
}
+#endif
+
+ ## wrapper.c ##
+@@
+ #include "abspath.h"
+ #include "parse.h"
+ #include "gettext.h"
+-#include "repository.h"
+ #include "strbuf.h"
+ #include "trace2.h"
+
50: 0bead8f980 ! 6: ab5dac5232 git-std-lib: add test file to call git-std-lib.a functions
@@ t/stdlib-test.c (new)
+ struct strbuf sb3 = STRBUF_INIT;
+ struct string_list list = STRING_LIST_INIT_NODUP;
+ char *buf = "foo";
-+ struct strbuf_expand_dict_entry dict[] = {
-+ { "foo", NULL, },
-+ { "bar", NULL, },
-+ };
+ int fd = open("/dev/null", O_RDONLY);
+
+ fprintf(stderr, "calling strbuf functions\n");
@@ t/stdlib-test.c (new)
+ strbuf_add_commented_lines(sb, "foo", 3, '#');
+ strbuf_commented_addf(sb, '#', "%s", "foo");
+ // strbuf_vaddf() called by strbuf_addf()
-+ strbuf_expand(sb, "%s", strbuf_expand_literal_cb, NULL);
-+ strbuf_expand(sb, "%s", strbuf_expand_dict_cb, &dict);
-+ // strbuf_expand_literal_cb() called by strbuf_expand()
-+ // strbuf_expand_dict_cb() called by strbuf_expand()
+ strbuf_addbuf_percentquote(sb, &sb3);
+ strbuf_add_percentencode(sb, "foo", STRBUF_ENCODE_SLASH);
+ strbuf_fread(sb, 0, stdin);
--
2.42.0.283.g2d96d420d3-goog
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Phillip Wood @ 2023-09-08 16:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: Phillip Wood via GitGitGadget, git, Eric Wong, Phillip Wood
In-Reply-To: <xmqqmsxwtyy3.fsf@gitster.g>
On 08/09/2023 17:24, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
>
>> On 08/09/2023 16:42, Junio C Hamano wrote:
>>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>>
>>>> [3] This is really a work-around for not moving the child into its own
>>>> process group and changing the foreground process group of the
>>>> controlling terminal.
>>> I am puzzled, as I somehow thought that "does the user conceive a
>>> subprocess as external and different-from-git entity, or is it
>>> merely an implementation detail? many use of subprocesses in our
>>> codebase, it is the latter." from Peff was a good argument against
>>> such isolation between spawning "git" and spawned subprocesses.
>>
>> It is and in those cases we do not ignore SIGINT and SIGQUIT in the
>> parent when we fork the subprocess. What I was trying to say is that
>> in the few cases where we do ignore SIGINT and SIGQUIT in the parent
>> when we fork a subprocess we're working round the child being in the
>> same process group at the parent.
>
> Hmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at
> random, the typical pattern seem to be (this example was taken from
> builtin/receive-pack.c):
>
> code = start_command(&proc);
> if (code) {
> ...
> return code;
> }
> sigchain_push(SIGPIPE, SIG_IGN);
> while (1) {
> ...
> }
> close(proc.in);
> sigchain_pop(SIGPIPE);
> return finish_command(&proc);
>
> The way we spawn an editor in editor.c looks the same:
>
> p.use_shell = 1;
> p.trace2_child_class = "editor";
> if (start_command(&p) < 0) {
> strbuf_release(&realpath);
> return error("unable to start editor '%s'", editor);
> }
>
> sigchain_push(SIGINT, SIG_IGN);
> sigchain_push(SIGQUIT, SIG_IGN);
> ret = finish_command(&p);
>
> IOW, we do not ignore then spawn. We spawn and ignore only in the
> parent, so there shouldn't be any reason to worry about our child
> inheriting the "we the parent git process do not want to be killed
> by \C-c" settings, should there?
Oh I should have looked more carefully at the existing uses. It looks
like it is only my sequencer patch that does
sigchain_push(SIGINT, SIG_IGN);
sigchain_push(SIGQUIT, SIG_IGN);
res = run_command(...);
In that case the existing behavior is a problem but maybe I should
change those call sites to use start_command() and finish_command()
instead if we decide that other patch is a good idea.
> I have a vague recollection that the "propagate what was already
> ignored to be ignored down to the child, too" was not about signals
> we ignored, but inherited from the end-user who started git with
> certain signals ignored, but it is so old a piece of code that the
> details of the rationale escapes me.
The comment
/* ignored signals get reset to SIG_DFL on execve */
in start_command() makes it look like the code assumes ignored signals
will be reset to SIG_DFL by execve() which is not what happens. Maybe
that comment is just wrong and there is a good reason for the current
behavior.
Best Wishes
Phillip
^ permalink raw reply
* Re: [ANNOUNCE] Virtual Contributor's Summit 2023
From: Taylor Blau @ 2023-09-08 16:29 UTC (permalink / raw)
To: git
In-Reply-To: <ZMATKIaU1A1D0wJg@nand.local>
On Tue, Jul 25, 2023 at 02:23:36PM -0400, Taylor Blau wrote:
> Hi everybody,
>
> As I mentioned a ~month ago[1], GitHub is not hosting an in-person Git
> Merge this year. Instead, we're focusing our efforts on hosting a
> Virtual Contributor's Summit, the details of which are below.
>
> - We'll host the Contributor's Summit on either Zoom or Microsoft
> Teams (since GitHub is in the process of transitioning from the
> former to the latter).
>
> - The schedule/duration is not fixed, there are options to vote on
> preferred days, length, and timezones in the form(s) below.
>
> - To participate, you must be either (a) an active Git contributor,
> (b) planning on contributing soon, or (c) working on a Git-related
> project that has interest in Git's internals. If you aren't sure
> whether or not you are welcome, please ask!
Now that most folks have filled out the forms, it looks like the winning
dates are 2029-09-26 and 2023-09-27. Among those dates, there was a
pretty even split between preferring a single day (with 9 votes), and
preferring two days (with 8 votes).
Let's plan on meeting on both:
- 2023-09-26: from 10am to 2pm CDT (UTC-5)
- 2023-09-27: from 10am to 2pm CDT (UTC-5)
The meeting will be hosted on Zoom, and I'll email folks separately
off-list with the meeting URL and passcode to join.
If you have any topics you'd like to discuss or lead, make sure to add
them to the "Topics" sheet at:
https://docs.google.com/spreadsheets/d/1EnhmTeEqRBlEI2pMAO3oZ4rO1xEwBzYp2vS4CMtvge8
Between now and then, please also feel free to share any thoughts on how
we should schedule topics, organize note-takers, make sure the discussion
keeps flowing and includes everybody, etc.
See you then!
Thanks,
Taylor
^ permalink raw reply
* Re: [PATCH] completion(switch/checkout): treat --track and -t the same
From: Junio C Hamano @ 2023-09-08 16:25 UTC (permalink / raw)
To: Todd Zullinger
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <ZPtISYVufU0MazO_@pobox.com>
Todd Zullinger <tmz@pobox.com> writes:
> Junio C Hamano wrote:
>> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
>> writes:
>>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>>> index 8835e16e811..df8bc44c285 100755
>>> --- a/t/t9902-completion.sh
>>> +++ b/t/t9902-completion.sh
>>> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
>>> '
>>>
>>> test_expect_success 'git switch - with --track, complete only remote branches' '
>>> - test_completion "git switch --track " <<-\EOF
>>> +: test_completion "git switch --track " <<-\EOF &&
>
> Is this new leading ":" intended? It looks out of place
> (though perhaps I just don't unerstand the context well
> enough).
Good eyes. It makes it an expensive no-op ;-)
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Junio C Hamano @ 2023-09-08 16:24 UTC (permalink / raw)
To: Phillip Wood; +Cc: Phillip Wood via GitGitGadget, git, Eric Wong, Phillip Wood
In-Reply-To: <ba69ab35-3204-4360-a36d-3253680b2479@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 08/09/2023 16:42, Junio C Hamano wrote:
>> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> [3] This is really a work-around for not moving the child into its own
>>> process group and changing the foreground process group of the
>>> controlling terminal.
>> I am puzzled, as I somehow thought that "does the user conceive a
>> subprocess as external and different-from-git entity, or is it
>> merely an implementation detail? many use of subprocesses in our
>> codebase, it is the latter." from Peff was a good argument against
>> such isolation between spawning "git" and spawned subprocesses.
>
> It is and in those cases we do not ignore SIGINT and SIGQUIT in the
> parent when we fork the subprocess. What I was trying to say is that
> in the few cases where we do ignore SIGINT and SIGQUIT in the parent
> when we fork a subprocess we're working round the child being in the
> same process group at the parent.
Hmph, but picking a few grep hits for 'sigchain_push.*SIG_IGN' at
random, the typical pattern seem to be (this example was taken from
builtin/receive-pack.c):
code = start_command(&proc);
if (code) {
...
return code;
}
sigchain_push(SIGPIPE, SIG_IGN);
while (1) {
...
}
close(proc.in);
sigchain_pop(SIGPIPE);
return finish_command(&proc);
The way we spawn an editor in editor.c looks the same:
p.use_shell = 1;
p.trace2_child_class = "editor";
if (start_command(&p) < 0) {
strbuf_release(&realpath);
return error("unable to start editor '%s'", editor);
}
sigchain_push(SIGINT, SIG_IGN);
sigchain_push(SIGQUIT, SIG_IGN);
ret = finish_command(&p);
IOW, we do not ignore then spawn. We spawn and ignore only in the
parent, so there shouldn't be any reason to worry about our child
inheriting the "we the parent git process do not want to be killed
by \C-c" settings, should there?
I have a vague recollection that the "propagate what was already
ignored to be ignored down to the child, too" was not about signals
we ignored, but inherited from the end-user who started git with
certain signals ignored, but it is so old a piece of code that the
details of the rationale escapes me.
^ permalink raw reply
* Re: [PATCH] completion(switch/checkout): treat --track and -t the same
From: Todd Zullinger @ 2023-09-08 16:14 UTC (permalink / raw)
To: Junio C Hamano
Cc: Johannes Schindelin via GitGitGadget, git, Johannes Schindelin
In-Reply-To: <xmqq1qf8vf1e.fsf@gitster.g>
Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 8835e16e811..df8bc44c285 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -1622,14 +1622,22 @@ test_expect_success 'git checkout - with -d, complete only references' '
>> '
>>
>> test_expect_success 'git switch - with --track, complete only remote branches' '
>> - test_completion "git switch --track " <<-\EOF
>> +: test_completion "git switch --track " <<-\EOF &&
Is this new leading ":" intended? It looks out of place
(though perhaps I just don't unerstand the context well
enough).
>> + other/branch-in-other Z
>> + other/main-in-other Z
>> + EOF
>> + test_completion "git switch -t " <<-\EOF
>> other/branch-in-other Z
>> other/main-in-other Z
>> EOF
>> '
>
> So, this demonstrates that '-t' behaves the same way as '--track'.
--
Todd
^ permalink raw reply
* Re: [PATCH] start_command: reset disposition of all signals in child
From: Phillip Wood @ 2023-09-08 15:53 UTC (permalink / raw)
To: Junio C Hamano, Phillip Wood via GitGitGadget
Cc: git, Eric Wong, Phillip Wood
In-Reply-To: <xmqqedj8vfht.fsf@gitster.g>
On 08/09/2023 16:42, Junio C Hamano wrote:
> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> [3] This is really a work-around for not moving the child into its own
>> process group and changing the foreground process group of the
>> controlling terminal.
>
> I am puzzled, as I somehow thought that "does the user conceive a
> subprocess as external and different-from-git entity, or is it
> merely an implementation detail? many use of subprocesses in our
> codebase, it is the latter." from Peff was a good argument against
> such isolation between spawning "git" and spawned subprocesses.
It is and in those cases we do not ignore SIGINT and SIGQUIT in the
parent when we fork the subprocess. What I was trying to say is that in
the few cases where we do ignore SIGINT and SIGQUIT in the parent when
we fork a subprocess we're working round the child being in the same
process group at the parent.
Best Wishes
Phillip
^ 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