* [PATCH v3 3/5] t4301: verify that merge-tree fails on missing blob objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-22 14:36 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
In-Reply-To: <pull.1651.v3.git.1708612605.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
We just fixed a problem where `merge-tree` would not fail on missing
tree objects. Let's ensure that that problem does not occur with blob
objects (and won't, in the future, either).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 908c9b540c8..d4463a45706 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '
test_must_be_empty actual
'
+test_expect_success 'error out on missing blob objects' '
+ echo 1 | git hash-object -w --stdin >blob1 &&
+ echo 2 | git hash-object -w --stdin >blob2 &&
+ echo 3 | git hash-object -w --stdin >blob3 &&
+ printf "100644 blob $(cat blob1)\tblob\n" | git mktree >tree1 &&
+ printf "100644 blob $(cat blob2)\tblob\n" | git mktree >tree2 &&
+ printf "100644 blob $(cat blob3)\tblob\n" | git mktree >tree3 &&
+ git init --bare missing-blob.git &&
+ cat blob1 blob3 tree1 tree2 tree3 |
+ git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+ test_must_fail git --git-dir=missing-blob.git >actual 2>err \
+ merge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&
+ test_grep "unable to read blob object $(cat blob2)" err &&
+ test_must_be_empty actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 4/5] Always check `parse_tree*()`'s return value
From: Johannes Schindelin via GitGitGadget @ 2024-02-22 14:36 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
In-Reply-To: <pull.1651.v3.git.1708612605.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
Otherwise we may easily run into serious crashes: For example, if we run
`init_tree_desc()` directly after a failed `parse_tree()`, we are
accessing uninitialized data or trying to dereference `NULL`.
Note that the `parse_tree()` function already takes care of showing an
error message. The `parse_tree_indirectly()` and
`repo_get_commit_tree()` functions do not, therefore those latter call
sites need to show a useful error message while the former do not.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/checkout.c | 19 ++++++++++++++++---
builtin/clone.c | 3 ++-
builtin/commit.c | 3 ++-
builtin/merge-tree.c | 6 ++++++
builtin/read-tree.c | 3 ++-
builtin/reset.c | 4 ++++
cache-tree.c | 4 ++--
merge-ort.c | 3 +++
merge-recursive.c | 3 ++-
merge.c | 5 ++++-
reset.c | 5 +++++
sequencer.c | 4 ++++
12 files changed, 52 insertions(+), 10 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f02434bc155..84108ec3635 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -707,7 +707,8 @@ static int reset_tree(struct tree *tree, const struct checkout_opts *o,
init_checkout_metadata(&opts.meta, info->refname,
info->commit ? &info->commit->object.oid : null_oid(),
NULL);
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ return 128;
init_tree_desc(&tree_desc, tree->buffer, tree->size);
switch (unpack_trees(1, &tree_desc, &opts)) {
case -2:
@@ -786,9 +787,15 @@ static int merge_working_tree(const struct checkout_opts *opts,
if (new_branch_info->commit)
BUG("'switch --orphan' should never accept a commit as starting point");
new_tree = parse_tree_indirect(the_hash_algo->empty_tree);
- } else
+ if (!new_tree)
+ BUG("unable to read empty tree");
+ } else {
new_tree = repo_get_commit_tree(the_repository,
new_branch_info->commit);
+ if (!new_tree)
+ return error(_("unable to read tree %s"),
+ oid_to_hex(&new_branch_info->commit->object.oid));
+ }
if (opts->discard_changes) {
ret = reset_tree(new_tree, opts, 1, writeout_error, new_branch_info);
if (ret)
@@ -823,7 +830,8 @@ static int merge_working_tree(const struct checkout_opts *opts,
oid_to_hex(old_commit_oid));
init_tree_desc(&trees[0], tree->buffer, tree->size);
- parse_tree(new_tree);
+ if (parse_tree(new_tree) < 0)
+ exit(128);
tree = new_tree;
init_tree_desc(&trees[1], tree->buffer, tree->size);
@@ -1239,10 +1247,15 @@ static void setup_new_branch_info_and_source_tree(
if (!new_branch_info->commit) {
/* not a commit */
*source_tree = parse_tree_indirect(rev);
+ if (!*source_tree)
+ die(_("unable to read tree %s"), oid_to_hex(rev));
} else {
parse_commit_or_die(new_branch_info->commit);
*source_tree = repo_get_commit_tree(the_repository,
new_branch_info->commit);
+ if (!*source_tree)
+ die(_("unable to read tree %s"),
+ oid_to_hex(&new_branch_info->commit->object.oid));
}
}
diff --git a/builtin/clone.c b/builtin/clone.c
index c6357af9498..4410b55be98 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -736,7 +736,8 @@ static int checkout(int submodule_progress, int filter_submodules)
tree = parse_tree_indirect(&oid);
if (!tree)
die(_("unable to parse commit %s"), oid_to_hex(&oid));
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ exit(128);
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts) < 0)
die(_("unable to checkout working tree"));
diff --git a/builtin/commit.c b/builtin/commit.c
index 781af2e206c..0723f06de7a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -339,7 +339,8 @@ static void create_base_index(const struct commit *current_head)
tree = parse_tree_indirect(¤t_head->object.oid);
if (!tree)
die(_("failed to unpack HEAD tree object"));
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ exit(128);
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts))
exit(128); /* We've already reported the error, finish dying */
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 2d4ce5b3886..ba84d00deee 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -447,12 +447,18 @@ static int real_merge(struct merge_tree_options *o,
if (repo_get_oid_treeish(the_repository, merge_base, &base_oid))
die(_("could not parse as tree '%s'"), merge_base);
base_tree = parse_tree_indirect(&base_oid);
+ if (!base_tree)
+ die(_("unable to read tree %s"), oid_to_hex(&base_oid));
if (repo_get_oid_treeish(the_repository, branch1, &head_oid))
die(_("could not parse as tree '%s'"), branch1);
parent1_tree = parse_tree_indirect(&head_oid);
+ if (!parent1_tree)
+ die(_("unable to read tree %s"), oid_to_hex(&head_oid));
if (repo_get_oid_treeish(the_repository, branch2, &merge_oid))
die(_("could not parse as tree '%s'"), branch2);
parent2_tree = parse_tree_indirect(&merge_oid);
+ if (!parent2_tree)
+ die(_("unable to read tree %s"), oid_to_hex(&merge_oid));
opt.ancestor = merge_base;
merge_incore_nonrecursive(&opt, base_tree, parent1_tree, parent2_tree, &result);
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index 8196ca9dd85..5923ed36893 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -263,7 +263,8 @@ int cmd_read_tree(int argc, const char **argv, const char *cmd_prefix)
cache_tree_free(&the_index.cache_tree);
for (i = 0; i < nr_trees; i++) {
struct tree *tree = trees[i];
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ return 128;
init_tree_desc(t+i, tree->buffer, tree->size);
}
if (unpack_trees(nr_trees, t, &opts))
diff --git a/builtin/reset.c b/builtin/reset.c
index 4b018d20e3b..f030f57f4e9 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -119,6 +119,10 @@ static int reset_index(const char *ref, const struct object_id *oid, int reset_t
if (reset_type == MIXED || reset_type == HARD) {
tree = parse_tree_indirect(oid);
+ if (!tree) {
+ error(_("unable to read tree %s"), oid_to_hex(oid));
+ goto out;
+ }
prime_cache_tree(the_repository, the_repository->index, tree);
}
diff --git a/cache-tree.c b/cache-tree.c
index 641427ed410..c6508b64a5c 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -779,8 +779,8 @@ static void prime_cache_tree_rec(struct repository *r,
struct cache_tree_sub *sub;
struct tree *subtree = lookup_tree(r, &entry.oid);
- if (!subtree->object.parsed)
- parse_tree(subtree);
+ if (!subtree->object.parsed && parse_tree(subtree) < 0)
+ exit(128);
sub = cache_tree_sub(it, entry.path);
sub->cache_tree = cache_tree();
diff --git a/merge-ort.c b/merge-ort.c
index 79d9e18f63d..534ddaf16ba 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4983,6 +4983,9 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
if (result->clean >= 0) {
result->tree = parse_tree_indirect(&working_tree_oid);
+ if (!result->tree)
+ die(_("unable to read tree %s"),
+ oid_to_hex(&working_tree_oid));
/* existence of conflicted entries implies unclean */
result->clean &= strmap_empty(&opt->priv->conflicted);
}
diff --git a/merge-recursive.c b/merge-recursive.c
index e3beb0801b1..10d41bfd487 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -410,7 +410,8 @@ static inline int merge_detect_rename(struct merge_options *opt)
static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
{
- parse_tree(tree);
+ if (parse_tree(tree) < 0)
+ exit(128);
init_tree_desc(desc, tree->buffer, tree->size);
}
diff --git a/merge.c b/merge.c
index b60925459c2..14a7325859d 100644
--- a/merge.c
+++ b/merge.c
@@ -80,7 +80,10 @@ int checkout_fast_forward(struct repository *r,
return -1;
}
for (i = 0; i < nr_trees; i++) {
- parse_tree(trees[i]);
+ if (parse_tree(trees[i]) < 0) {
+ rollback_lock_file(&lock_file);
+ return -1;
+ }
init_tree_desc(t+i, trees[i]->buffer, trees[i]->size);
}
diff --git a/reset.c b/reset.c
index 48da0adf851..a93fdbc12e3 100644
--- a/reset.c
+++ b/reset.c
@@ -158,6 +158,11 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)
}
tree = parse_tree_indirect(oid);
+ if (!tree) {
+ ret = error(_("unable to read tree %s"), oid_to_hex(oid));
+ goto leave_reset_head;
+ }
+
prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0) {
diff --git a/sequencer.c b/sequencer.c
index d584cac8ed9..407473bab28 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -715,6 +715,8 @@ static int do_recursive_merge(struct repository *r,
o.show_rename_progress = 1;
head_tree = parse_tree_indirect(head);
+ if (!head_tree)
+ return error(_("unable to read tree %s"), oid_to_hex(head));
next_tree = next ? repo_get_commit_tree(r, next) : empty_tree(r);
base_tree = base ? repo_get_commit_tree(r, base) : empty_tree(r);
@@ -3887,6 +3889,8 @@ static int do_reset(struct repository *r,
}
tree = parse_tree_indirect(&oid);
+ if (!tree)
+ return error(_("unable to read tree %s"), oid_to_hex(&oid));
prime_cache_tree(r, r->index, tree);
if (write_locked_index(r->index, &lock, COMMIT_LOCK) < 0)
--
gitgitgadget
^ permalink raw reply related
* [PATCH v3 5/5] cache-tree: avoid an unnecessary check
From: Johannes Schindelin via GitGitGadget @ 2024-02-22 14:36 UTC (permalink / raw)
To: git
Cc: Patrick Steinhardt, Eric Sunshine, Johannes Schindelin,
Johannes Schindelin
In-Reply-To: <pull.1651.v3.git.1708612605.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The first thing the `parse_tree()` function does is to return early if
the tree has already been parsed. Therefore we do not need to guard the
`parse_tree()` call behind a check of that flag.
As of time of writing, there are no other instances of this in Git's
code bases: whenever the `parsed` flag guards a `parse_tree()` call, it
guards more than just that call.
Suggested-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
cache-tree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/cache-tree.c b/cache-tree.c
index c6508b64a5c..78d6ba92853 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -779,7 +779,7 @@ static void prime_cache_tree_rec(struct repository *r,
struct cache_tree_sub *sub;
struct tree *subtree = lookup_tree(r, &entry.oid);
- if (!subtree->object.parsed && parse_tree(subtree) < 0)
+ if (parse_tree(subtree) < 0)
exit(128);
sub = cache_tree_sub(it, entry.path);
sub->cache_tree = cache_tree();
--
gitgitgadget
^ permalink raw reply related
* Re: Git in GSoC 2024
From: Patrick Steinhardt @ 2024-02-22 15:52 UTC (permalink / raw)
To: Kaartic Sivaraam
Cc: Christian Couder, Karthik Nayak, git, Taylor Blau, Junio C Hamano,
Victoria Dye
In-Reply-To: <B6C95613-B316-404F-9076-FAC5955B8890@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1756 bytes --]
On Thu, Feb 22, 2024 at 07:35:33PM +0530, Kaartic Sivaraam wrote:
> Hi Patrick, Karthik, Christian and all,
>
>
> On 22 February 2024 1:19:42 pm IST, Patrick Steinhardt <ps@pks.im> wrote:
> >On Thu, Feb 22, 2024 at 10:01:54AM +0530, Kaartic Sivaraam wrote:
> >> Hi Christian, Patrick, Karthik and all,
> >>
> >> On 21/02/24 10:32, Kaartic Sivaraam wrote:
> >>
> >> Also, it's official now. Git has been selected as one of the participating
> >> organizations[2] in GSoC 2024!
> >>
> >> Let's look forward towards a summer with great GSoC contributors who
> >> hopefully become continued contributors to the community :-)
> >>
> >> [[ References ]]
> >>
> >> [1]:
> >> https://summerofcode.withgoogle.com/organizations/git/programs/2024/timeline
> >>
> >> [2]: https://summerofcode.withgoogle.com/programs/2024/organizations/git
> >
> >I can access the second link, but the first one is broken for me. First
> >it claimed that my Google account wasn't connected to GSoC, and after a
> >reload it stays blank now.
> >
>
> That's strange. Could you possibly try logging into the Summer of code website [3] directly in an incognito window using your GitLab account?
>
> I've previously faced issues with logging into the summer of code website due to an add-on blocking access to other Google domains. So, if you have add-ons that might block resources accessed by the website, could you possibly try disabling them?
>
> If you face issues despite all this, the only resort is to write to GSoC support about this issue at gsoc-support@google.com
>
> [3]: https://summerofcode.withgoogle.com/
>
> Hope this helps,
> Sivaraam
Things work now after a re-login. Kinda strange, but so be it. Thanks!
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2024, #07; Tue, 20)
From: Junio C Hamano @ 2024-02-22 16:15 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Karthik Nayak, git
In-Reply-To: <ZdcFDEg4Gl8YpCQi@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
> This version already has the updated UI with `--include-root-refs`.
> There has been some discussion around whether we want to refactor the
> refname checking interfaces so that we do not have to introduce the two
> new helper functions `is_pseudoref()` and `is_headref()`. But that would
> result in a lot of churn, and we thus agreed that it is fine to do that
> in a follow up patch series.
>
> I then forgot to have a look at the remaining patches. I can do that
> today.
Thanks.
^ permalink raw reply
* Re: Segfault: git show-branch --reflog refs/pullreqs/1
From: Junio C Hamano @ 2024-02-22 16:32 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: Jeff King, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <ZdcNtxw04MtybTWZ@tanuki>
Patrick Steinhardt <ps@pks.im> writes:
>> Even though it may feel wrong to successfully resolve foo@{0} when
>> reflog for foo does not exist at the mechanical level (read: the
>> implementors of reflog mechanism may find the usability hack a bad
>> idea), I suspect at the end-user level it may be closer to what
>> people expect out of foo@{0} (i.e. "give me the latest").
>
> Hum, I dunno. I don't really understand what the benefit of this
> fallback is. If a user wants to know the latest object ID of the ref
> they shouldn't ask for `foo@{0}`, they should ask for `foo`. On the
> other hand, if I want to know "What is the latest entry in the ref's
> log", I want to ask for `foo@{0}`.
The usability hack helps small things like "List up to 4 most recent
states from a branch", e.g.
for nth in $(seq 0 3)
do
git rev-parse --quiet --verify @$nth || break
git show -s --format="@$nth %h %s" @$nth
done
vs
for rev in HEAD @{1} @{2} @{3}
do
git rev-parse --quiet --verify "$rev" || break
git show -s --format="$rev %h %s" "$rev"
done
by not forcing you to special case the "current".
Ideally, "foo@{0}" should have meant "the state immediately before
the current state of foo" so that "foo" is the unambiguous and only
way to refer to "the current state of foo", but that was not how we
implemented the reflog, allowing a subtle repository corruption
where the latest state of a branch according to the reflog and the
current commit pointed by the branch can diverge. But that wasn't
what we did, and instead both "foo@{0}" and "foo" mean to refer to
"the latest state of foo". We can take advantage of that misdesign
and allow "foo@{0}" to refer to the same commit as "foo", at least
at the get_oid_basic() level, whether a reflog actually exists or
not, and that would make the whole thing more consistent.
In any case, I do not know how this "usability" actually helps in
the field, and I wouldn't personally shed tears if it gets removed.
The above is just an explanation why it exists.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 3/8] rebase: update `--empty=ask` to `--empty=drop`
From: phillip.wood123 @ 2024-02-22 16:34 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster
In-Reply-To: <20240210074859.552497-4-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> When git-am(1) got its own `--empty` option in 7c096b8d61 (am: support
> --empty=<option> to handle empty patches, 2021-12-09), `stop` was used
> instead of `ask`. `stop` is a more accurate term for describing what
> really happens, and consistency is good.
>
> Update git-rebase(1) to also use `stop`, while keeping `ask` as a
> deprecated synonym. Update the tests to primarily use `stop`, but also
> ensure that `ask` is still allowed.
>
> In a future commit, we'll be adding a new `--empty` option for
> git-cherry-pick(1) as well, making the consistency even more relevant.
I'm sightly nervous of deprecating "ask" as the warnings have the
potential to annoy users but it would be good to use consistent
terminology so it may well be worth it. This patch the previous ones
look good apart from one minor issue ...
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> Reported-by: Elijah Newren <newren@gmail.com>
I think we normally put Reported-by: and Helped-by: etc above the patch
authors Signed-off-by: trailer.
Best Wishes
Phillip
> ---
> Documentation/git-rebase.txt | 15 ++++++++-------
> builtin/rebase.c | 16 ++++++++++------
> t/t3424-rebase-empty.sh | 21 ++++++++++++++++-----
> 3 files changed, 34 insertions(+), 18 deletions(-)
>
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 68cdebd2aa..6f64084a95 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -289,23 +289,24 @@ See also INCOMPATIBLE OPTIONS below.
> +
> See also INCOMPATIBLE OPTIONS below.
>
> ---empty=(ask|drop|keep)::
> +--empty=(drop|keep|stop)::
> How to handle commits that are not empty to start and are not
> clean cherry-picks of any upstream commit, but which become
> empty after rebasing (because they contain a subset of already
> upstream changes):
> +
> --
> -`ask`;;
> - The rebase will halt when the commit is applied, allowing you to
> - choose whether to drop it, edit files more, or just commit the empty
> - changes. This option is implied when `-i`/`--interactive` is
> - specified.
> `drop`;;
> The commit will be dropped. This is the default behavior.
> `keep`;;
> The commit will be kept. This option is implied when `--exec` is
> specified unless `-i`/`--interactive` is also specified.
> +`stop`;;
> +`ask`;;
> + The rebase will halt when the commit is applied, allowing you to
> + choose whether to drop it, edit files more, or just commit the empty
> + changes. This option is implied when `-i`/`--interactive` is
> + specified. `ask` is a deprecated synonym of `stop`.
> --
> +
> Note that commits which start empty are kept (unless `--no-keep-empty`
> @@ -705,7 +706,7 @@ be dropped automatically with `--no-keep-empty`).
> Similar to the apply backend, by default the merge backend drops
> commits that become empty unless `-i`/`--interactive` is specified (in
> which case it stops and asks the user what to do). The merge backend
> -also has an `--empty=(ask|drop|keep)` option for changing the behavior
> +also has an `--empty=(drop|keep|stop)` option for changing the behavior
> of handling commits that become empty.
>
> Directory rename detection
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 4084a6abb8..3b9bb2fa06 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -58,7 +58,7 @@ enum empty_type {
> EMPTY_UNSPECIFIED = -1,
> EMPTY_DROP,
> EMPTY_KEEP,
> - EMPTY_ASK
> + EMPTY_STOP
> };
>
> enum action {
> @@ -954,10 +954,14 @@ static enum empty_type parse_empty_value(const char *value)
> return EMPTY_DROP;
> else if (!strcasecmp(value, "keep"))
> return EMPTY_KEEP;
> - else if (!strcasecmp(value, "ask"))
> - return EMPTY_ASK;
> + else if (!strcasecmp(value, "stop"))
> + return EMPTY_STOP;
> + else if (!strcasecmp(value, "ask")) {
> + warning(_("--empty=ask is deprecated; use '--empty=stop' instead."));
> + return EMPTY_STOP;
> + }
>
> - die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"ask\"."), value);
> + die(_("unrecognized empty type '%s'; valid values are \"drop\", \"keep\", and \"stop\"."), value);
> }
>
> static int parse_opt_keep_empty(const struct option *opt, const char *arg,
> @@ -1136,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> "instead of ignoring them"),
> 1, PARSE_OPT_HIDDEN),
> OPT_RERERE_AUTOUPDATE(&options.allow_rerere_autoupdate),
> - OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|ask)",
> + OPT_CALLBACK_F(0, "empty", &options, "(drop|keep|stop)",
> N_("how to handle commits that become empty"),
> PARSE_OPT_NONEG, parse_opt_empty),
> OPT_CALLBACK_F('k', "keep-empty", &options, NULL,
> @@ -1553,7 +1557,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>
> if (options.empty == EMPTY_UNSPECIFIED) {
> if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
> - options.empty = EMPTY_ASK;
> + options.empty = EMPTY_STOP;
> else if (options.exec.nr > 0)
> options.empty = EMPTY_KEEP;
> else
> diff --git a/t/t3424-rebase-empty.sh b/t/t3424-rebase-empty.sh
> index 73ff35ced2..1ee6b00fd5 100755
> --- a/t/t3424-rebase-empty.sh
> +++ b/t/t3424-rebase-empty.sh
> @@ -72,6 +72,17 @@ test_expect_success 'rebase --merge --empty=keep' '
> test_cmp expect actual
> '
>
> +test_expect_success 'rebase --merge --empty=stop' '
> + git checkout -B testing localmods &&
> + test_must_fail git rebase --merge --empty=stop upstream &&
> +
> + git rebase --skip &&
> +
> + test_write_lines D C B A >expect &&
> + git log --format=%s >actual &&
> + test_cmp expect actual
> +'
> +
> test_expect_success 'rebase --merge --empty=ask' '
> git checkout -B testing localmods &&
> test_must_fail git rebase --merge --empty=ask upstream &&
> @@ -101,9 +112,9 @@ test_expect_success 'rebase --interactive --empty=keep' '
> test_cmp expect actual
> '
>
> -test_expect_success 'rebase --interactive --empty=ask' '
> +test_expect_success 'rebase --interactive --empty=stop' '
> git checkout -B testing localmods &&
> - test_must_fail git rebase --interactive --empty=ask upstream &&
> + test_must_fail git rebase --interactive --empty=stop upstream &&
>
> git rebase --skip &&
>
> @@ -112,7 +123,7 @@ test_expect_success 'rebase --interactive --empty=ask' '
> test_cmp expect actual
> '
>
> -test_expect_success 'rebase --interactive uses default of --empty=ask' '
> +test_expect_success 'rebase --interactive uses default of --empty=stop' '
> git checkout -B testing localmods &&
> test_must_fail git rebase --interactive upstream &&
>
> @@ -194,9 +205,9 @@ test_expect_success 'rebase --exec uses default of --empty=keep' '
> test_cmp expect actual
> '
>
> -test_expect_success 'rebase --exec --empty=ask' '
> +test_expect_success 'rebase --exec --empty=stop' '
> git checkout -B testing localmods &&
> - test_must_fail git rebase --exec "true" --empty=ask upstream &&
> + test_must_fail git rebase --exec "true" --empty=stop upstream &&
>
> git rebase --skip &&
>
^ permalink raw reply
* Re: [PATCH v2 4/8] sequencer: treat error reading HEAD as unborn branch
From: phillip.wood123 @ 2024-02-22 16:34 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster, Phillip Wood
In-Reply-To: <20240210074859.552497-5-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> When using git-cherry-pick(1) with `--allow-empty` while on an unborn
> branch, an error is thrown. This is inconsistent with the same
> cherry-pick when `--allow-empty` is not specified.
>
> Treat a failure reading HEAD as an unborn branch in
> `is_index_unchanged`. This is consistent with other sequencer logic such
> as `do_pick_commit`. When on an unborn branch, use the `empty_tree` as
> the tree to compare against.
>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>
> This is another new commit that was not present in v1.
>
> See this comment[1] from Phillip for context.
>
> [1]: https://lore.kernel.org/git/b5213705-4cd6-40ef-8c5f-32b214534b8b@gmail.com/
Thanks for fixing this and adding a test, I've left a few small comments
below.
> static int is_index_unchanged(struct repository *r)
> {
> - struct object_id head_oid, *cache_tree_oid;
> + struct object_id head_oid, *cache_tree_oid, head_tree_oid;
I think we can make `head_tree_oid` a pointer like cache_tree_oid and
avoid de-referencing `the_hash_algo->empty_tree` and the return value of
`get_commit_tree_oid()`. I think the only reason to copy it would be if
the underlying object had a shorter lifetime than `head_tree_oid` but I
don't think that's the case.
> +test_expect_success 'cherry-pick on unborn branch with --allow-empty' '
> + git checkout main &&
I'm a bit confused by this - are we already on the branch "unborn" and
so need to move away from it to delete it?
> + git branch -D unborn &&
> + git checkout --orphan unborn &&
> + git rm --cached -r . &&
> + rm -rf * &&
"git switch --orphan" leaves us with an empty index and working copy
without having to remove the files ourselves.
> + git cherry-pick initial --allow-empty &&
> + git diff --quiet initial &&
I'd drop "--quiet" here as it makes debugging easier if we can see the
diff if the test fails.
> + test_cmp_rev ! initial HEAD
> +'
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH v2 5/8] sequencer: do not require `allow_empty` for redundant commit options
From: phillip.wood123 @ 2024-02-22 16:35 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster
In-Reply-To: <20240210074859.552497-6-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> A consumer of the sequencer that wishes to take advantage of either the
> `keep_redundant_commits` or `drop_redundant_commits` feature must also
> specify `allow_empty`. However, these refer to two distinct types of
> empty commits:
>
> - `allow_empty` refers specifically to commits which start empty
> - `keep_redundant_commits` refers specifically to commits that do not
> start empty, but become empty due to the content already existing in
> the target history
>
> Conceptually, there is no reason that the behavior for handling one of
> these should be entangled with the other. It is particularly unintuitive
> to require `allow_empty` in order for `drop_redundant_commits` to have
> an effect: in order to prevent redundant commits automatically,
> initially-empty commits would need to be kept automatically as well.
>
> Instead, rewrite the `allow_empty()` logic to remove the over-arching
> requirement that `allow_empty` be specified in order to reach any of the
> keep/drop behaviors. Only if the commit was originally empty will
> `allow_empty` have an effect.
>
> Note that no behavioral changes should result from this commit -- it
> merely sets the stage for future commits.
Thanks for clarifying that. I think splitting this change out is a good
idea. The patch looks good.
Best Wishes
Phillip
> In one such future commit, an
> `--empty` option will be added to git-cherry-pick(1), meaning that
> `drop_redundant_commits` will be used by that command.
>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
>
> This is the first half of the first commit[1] in v1, which has now been
> split up. While the next commit may be considered somewhat
> controversial, this part of the change should not be.
>
> [1]: https://lore.kernel.org/git/20240119060721.3734775-2-brianmlyles@gmail.com/
>
> sequencer.c | 23 +++++++----------------
> 1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index b1b19512de..3f41863dae 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1725,34 +1725,25 @@ static int allow_empty(struct repository *r,
> int index_unchanged, originally_empty;
>
> /*
> - * Four cases:
> + * For a commit that is initially empty, allow_empty determines if it
> + * should be kept or not
> *
> - * (1) we do not allow empty at all and error out.
> - *
> - * (2) we allow ones that were initially empty, and
> - * just drop the ones that become empty
> - *
> - * (3) we allow ones that were initially empty, but
> - * halt for the ones that become empty;
> - *
> - * (4) we allow both.
> + * For a commit that becomes empty, keep_redundant_commits and
> + * drop_redundant_commits determine whether the commit should be kept or
> + * dropped. If neither is specified, halt.
> */
> - if (!opts->allow_empty)
> - return 0; /* let "git commit" barf as necessary */
> -
> index_unchanged = is_index_unchanged(r);
> if (index_unchanged < 0)
> return index_unchanged;
> if (!index_unchanged)
> return 0; /* we do not have to say --allow-empty */
>
> - if (opts->keep_redundant_commits)
> - return 1;
> -
> originally_empty = is_original_commit_empty(commit);
> if (originally_empty < 0)
> return originally_empty;
> if (originally_empty)
> + return opts->allow_empty;
> + else if (opts->keep_redundant_commits)
> return 1;
> else if (opts->drop_redundant_commits)
> return 2;
^ permalink raw reply
* Re: [PATCH v2 6/8] cherry-pick: decouple `--allow-empty` and `--keep-redundant-commits`
From: Phillip Wood @ 2024-02-22 16:35 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster
In-Reply-To: <20240210074859.552497-7-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> As noted in the git-cherry-pick(1) docs, `--keep-redundant-commits`
> implies `--allow-empty`, despite the two having distinct,
> non-overlapping meanings:
>
> - `allow_empty` refers specifically to commits which start empty, as
> indicated by the documentation for `--allow-empty` within
> git-cherry-pick(1):
>
> "Note also, that use of this option only keeps commits that were
> initially empty (i.e. the commit recorded the same tree as its
> parent). Commits which are made empty due to a previous commit are
> dropped. To force the inclusion of those commits use
> --keep-redundant-commits."
>
> - `keep_redundant_commits` refers specifically to commits that do not
> start empty, but become empty due to the content already existing in
> the target history. This is indicated by the documentation for
> `--keep-redundant-commits` within git-cherry-pick(1):
>
> "If a commit being cherry picked duplicates a commit already in the
> current history, it will become empty. By default these redundant
> commits cause cherry-pick to stop so the user can examine the commit.
> This option overrides that behavior and creates an empty commit
> object. Implies --allow-empty."
>
> This implication of `--allow-empty` therefore seems incorrect: One
> should be able to keep a commit that becomes empty without also being
> forced to pick commits that start as empty. However, the following
> series of commands results in both the commit that became empty and the
> commit that started empty being picked, despite only
> `--keep-redundant-commits` being specified:
>
> git init
> echo "a" >test
> git add test
> git commit -m "Initial commit"
> echo "b" >test
> git commit -am "a -> b"
> git commit --allow-empty -m "empty"
> git cherry-pick --keep-redundant-commits HEAD^ HEAD
>
> The same cherry-pick with `--allow-empty` would fail on the redundant
> commit, and with neither option would fail on the empty commit.
>
> Do not imply `--allow-empty` when using `--keep-redundant-commits` with
> git-cherry-pick(1).
>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
>
> This is the second half of the first commit[1] in v1, which has now been
> split up.
>
> This commit proposes a breaking change, albeit one that seems correct
> and relatively minor to me. If this change is deemed too controversial,
> I am prepared to drop it from the series. See Junio's[2] and
> Phillip's[3] comments on v1 for additional context.
I agree that if we were starting from scratch there would be no reason
to tie --apply-empty and --keep-redundant-commits together but I'm not
sure it is worth the disruption of changing it now. We're about to add
empty=keep which won't imply --allow-empty for anyone who wants that
behavior and I still tend to think the practical effect of implying
--allow-empty with --keep-redundant-commits is largely beneficial as I'm
skeptical that users want to keep commits that become empty but not the
ones that started empty.
Best Wishes
Phillip
> [1]: https://lore.kernel.org/git/20240119060721.3734775-2-brianmlyles@gmail.com/
> [2]: https://lore.kernel.org/git/xmqqy1cfnca7.fsf@gitster.g/
> [3]: https://lore.kernel.org/git/8ff4650c-f84f-41bd-a46c-3b845ff29b70@gmail.com/
>
> Documentation/git-cherry-pick.txt | 10 +++++++---
> builtin/revert.c | 4 ----
> t/t3505-cherry-pick-empty.sh | 6 ++++++
> 3 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/git-cherry-pick.txt b/Documentation/git-cherry-pick.txt
> index fdcad3d200..c88bb88822 100644
> --- a/Documentation/git-cherry-pick.txt
> +++ b/Documentation/git-cherry-pick.txt
> @@ -131,8 +131,8 @@ effect to your index in a row.
> even without this option. Note also, that use of this option only
> keeps commits that were initially empty (i.e. the commit recorded the
> same tree as its parent). Commits which are made empty due to a
> - previous commit are dropped. To force the inclusion of those commits
> - use `--keep-redundant-commits`.
> + previous commit will cause the cherry-pick to fail. To force the
> + inclusion of those commits, use `--keep-redundant-commits`.
>
> --allow-empty-message::
> By default, cherry-picking a commit with an empty message will fail.
> @@ -144,7 +144,11 @@ effect to your index in a row.
> current history, it will become empty. By default these
> redundant commits cause `cherry-pick` to stop so the user can
> examine the commit. This option overrides that behavior and
> - creates an empty commit object. Implies `--allow-empty`.
> + creates an empty commit object. Note that use of this option only
> + results in an empty commit when the commit was not initially empty,
> + but rather became empty due to a previous commit. Commits that were
> + initially empty will cause the cherry-pick to fail. To force the
> + inclusion of those commits use `--allow-empty`.
>
> --strategy=<strategy>::
> Use the given merge strategy. Should only be used once.
> diff --git a/builtin/revert.c b/builtin/revert.c
> index 89821bab95..d83977e36e 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -134,10 +134,6 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> prepare_repo_settings(the_repository);
> the_repository->settings.command_requires_full_index = 0;
>
> - /* implies allow_empty */
> - if (opts->keep_redundant_commits)
> - opts->allow_empty = 1;
> -
> if (cleanup_arg) {
> opts->default_msg_cleanup = get_cleanup_mode(cleanup_arg, 1);
> opts->explicit_cleanup = 1;
> diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
> index eba3c38d5a..2709cfc677 100755
> --- a/t/t3505-cherry-pick-empty.sh
> +++ b/t/t3505-cherry-pick-empty.sh
> @@ -59,6 +59,12 @@ test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
> test_must_fail git cherry-pick empty-change-branch
> '
>
> +test_expect_success 'cherry pick an empty non-ff commit with --keep-redundant-commits' '
> + git checkout main &&
> + test_must_fail git cherry-pick --keep-redundant-commits empty-change-branch 2>output &&
> + test_grep "The previous cherry-pick is now empty" output
> +'
> +
> test_expect_success 'cherry pick an empty non-ff commit with --allow-empty' '
> git checkout main &&
> git cherry-pick --allow-empty empty-change-branch
^ permalink raw reply
* Re: [PATCH v2 7/8] cherry-pick: enforce `--keep-redundant-commits` incompatibility
From: Phillip Wood @ 2024-02-22 16:35 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster
In-Reply-To: <20240210074859.552497-8-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> When `--keep-redundant-commits` was added in b27cfb0d8d
> (git-cherry-pick: Add keep-redundant-commits option, 2012-04-20), it was
> not marked as incompatible with the various operations needed to
> continue or exit a cherry-pick (`--continue`, `--skip`, `--abort`, and
> `--quit`).
>
> Enforce this incompatibility via `verify_opt_compatible` like we do for
> the other various options.
>
> Signed-off-by: Brian Lyles <brianmlyles@gmail.com>
> ---
>
> This commit was not present in v1 either. It addresses an existing issue
> that I noticed after Phillip pointed out the same deficiency for my new
> `--empty` option introduced in the ultimate commit in this series.
Well spotted, do we really need a new test file just for this though? I
wonder if the new test would be better off living in
t3505-cherry-pick-empty.sh or t3507-cherry-pick-conflict.sh
Best Wishes
Phillip
> [1]: https://lore.kernel.org/git/CAHPHrSf+joHe6ikErHLgWrk-_qjSROS-dXCHagxWGDAF=2deDg@mail.gmail.com/
>
> builtin/revert.c | 1 +
> t/t3515-cherry-pick-incompatible-options.sh | 34 +++++++++++++++++++++
> 2 files changed, 35 insertions(+)
> create mode 100755 t/t3515-cherry-pick-incompatible-options.sh
>
> diff --git a/builtin/revert.c b/builtin/revert.c
> index d83977e36e..48c426f277 100644
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -163,6 +163,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
> "--ff", opts->allow_ff,
> "--rerere-autoupdate", opts->allow_rerere_auto == RERERE_AUTOUPDATE,
> "--no-rerere-autoupdate", opts->allow_rerere_auto == RERERE_NOAUTOUPDATE,
> + "--keep-redundant-commits", opts->keep_redundant_commits,
> NULL);
> }
>
> diff --git a/t/t3515-cherry-pick-incompatible-options.sh b/t/t3515-cherry-pick-incompatible-options.sh
> new file mode 100755
> index 0000000000..6100ab64fd
> --- /dev/null
> +++ b/t/t3515-cherry-pick-incompatible-options.sh
> @@ -0,0 +1,34 @@
> +#!/bin/sh
> +
> +test_description='test if cherry-pick detects and aborts on incompatible options'
> +
> +. ./test-lib.sh
> +
> +test_expect_success setup '
> +
> + echo first > file1 &&
> + git add file1 &&
> + test_tick &&
> + git commit -m "first" &&
> +
> + echo second > file1 &&
> + git add file1 &&
> + test_tick &&
> + git commit -m "second"
> +'
> +
> +test_expect_success '--keep-redundant-commits is incompatible with operations' '
> + test_must_fail git cherry-pick HEAD 2>output &&
> + test_grep "The previous cherry-pick is now empty" output &&
> + test_must_fail git cherry-pick --keep-redundant-commits --continue 2>output &&
> + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --continue" output &&
> + test_must_fail git cherry-pick --keep-redundant-commits --skip 2>output &&
> + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --skip" output &&
> + test_must_fail git cherry-pick --keep-redundant-commits --abort 2>output &&
> + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --abort" output &&
> + test_must_fail git cherry-pick --keep-redundant-commits --quit 2>output &&
> + test_grep "fatal: cherry-pick: --keep-redundant-commits cannot be used with --quit" output &&
> + git cherry-pick --abort
> +'
> +
> +test_done
^ permalink raw reply
* Re: [PATCH v2 8/8] cherry-pick: add `--empty` for more robust redundant commit handling
From: phillip.wood123 @ 2024-02-22 16:36 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster
In-Reply-To: <20240210074859.552497-9-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> As with git-rebase(1) and git-am(1), git-cherry-pick(1) can result in a
> commit being made redundant if the content from the picked commit is
> already present in the target history. However, git-cherry-pick(1) does
> not have the same options available that git-rebase(1) and git-am(1) have.
>
> There are three things that can be done with these redundant commits:
> drop them, keep them, or have the cherry-pick stop and wait for the user
> to take an action. git-rebase(1) has the `--empty` option added in commit
> e98c4269c8 (rebase (interactive-backend): fix handling of commits that
> become empty, 2020-02-15), which handles all three of these scenarios.
> Similarly, git-am(1) got its own `--empty` in 7c096b8d61 (am: support
> --empty=<option> to handle empty patches, 2021-12-09).
>
> git-cherry-pick(1), on the other hand, only supports two of the three
> possiblities: Keep the redundant commits via `--keep-redundant-commits`,
> or have the cherry-pick fail by not specifying that option. There is no
> way to automatically drop redundant commits.
>
> In order to bring git-cherry-pick(1) more in-line with git-rebase(1) and
> git-am(1), this commit adds an `--empty` option to git-cherry-pick(1). It
> has the same three options (keep, drop, and stop), and largely behaves
> the same. The notable difference is that for git-cherry-pick(1), the
> default will be `stop`, which maintains the current behavior when the
> option is not specified.
>
> The `--keep-redundant-commits` option will be documented as a deprecated
> synonym of `--empty=keep`, and will be supported for backwards
> compatibility for the time being.
I'm leaning towards leaving `--keep-redundant-commits` alone. That
introduces an inconsistency between `--keep-redundant-commits` and
`--empty=keep` as the latter does not imply `--allow-empty` but it does
avoid breaking existing users. We could document
`--keep-redundant-commits` as predating `--empty` and behaving like
`--empty=keep --allow-empty`. The documentation and implementation of
the new option look good modulo the typo that has already been pointed
out and a couple of small comments below.
> +enum empty_action {
> + EMPTY_COMMIT_UNSPECIFIED = 0,
We tend to use -1 for unspecified options
> + STOP_ON_EMPTY_COMMIT, /* output errors and stop in the middle of a cherry-pick */
> + DROP_EMPTY_COMMIT, /* skip with a notice message */
> + KEEP_EMPTY_COMMIT, /* keep recording as empty commits */
> +};
> +test_expect_success 'cherry-pick persists --empty=stop correctly' '
> + pristine_detach initial &&
> + # to make sure that the session to cherry-pick a sequence
> + # gets interrupted, use a high-enough number that is larger
> + # than the number of parents of any commit we have created
> + mainline=4 &&
> + test_expect_code 128 git cherry-pick -s -m $mainline --empty=stop initial..anotherpick &&
> + test_path_is_file .git/sequencer/opts &&
> + test_must_fail git config --file=.git/sequencer/opts --get-all options.keep-redundant-commits &&
> + test_must_fail git config --file=.git/sequencer/opts --get-all options.drop-redundant-commits
> +'
Thanks for adding these tests to check that the --empty option persists.
Usually for tests like this we prefer to check the user visible behavior
rather than the implementation details (I suspect we have some older
tests that do the latter). To check the behavior we usually arrange for
a merge conflict but using -m is a creative alternative, then we'd run
"git cherry-pick --continue" and check that the commits that become
empty have been preserved or dropped or that the cherry-pick stops.
Best Wishes
Phillip
^ permalink raw reply
* Re: [PATCH 1/3] doc: git-rev-parse: enforce command-line description syntax
From: Junio C Hamano @ 2024-02-22 16:38 UTC (permalink / raw)
To: Jean-Noël AVILA; +Cc: Jean-Noël Avila via GitGitGadget, git
In-Reply-To: <2926790.e9J7NaK4W3@cayenne>
Jean-Noël AVILA <jn.avila@free.fr> writes:
>> So, perhaps we do not have to do a lot of 'word' -> _word_
>> replacements, hopefully?
> ... At least, we
> should try to stick as much as possible to the common markup _ for emphasis.
OK, that clears up my confusion. Thanks.
We do not want to rely on an external party indefinitely maintaining
what they consider backward compatibility wart, so the mark-up migration
would need to happen before it becomes too late.
> This would have the added benefit of differentiating single quotes from
> backticks, because single quotes would completely disappear in the end, except
> when a real single quote is needed.
Given enough time, yes. Or we can actively disable AsciiDoctor's
compatibility mode and/or tweak asciidoc.conf to do the equivalent
for AsciiDoc, to start early. Until then, we cannot really use "a
real single quote", right?
> For the migration to "standard" asciidoc, I would also recommend using "="
> prefix for titles instead of underlines which require changing two lines when
> modifying a title and are a pain for translators in languages with variable
> width characters.
I personally strongly prefer the underline format because I care
about readability of sources, but that is just me. Is that also
getting deprecated?
Thanks.
^ permalink raw reply
* Re: [PATCH v2 0/8] cherry-pick: add `--empty`
From: phillip.wood123 @ 2024-02-22 16:39 UTC (permalink / raw)
To: Brian Lyles, git; +Cc: newren, me, gitster
In-Reply-To: <20240210074859.552497-1-brianmlyles@gmail.com>
Hi Brian
On 10/02/2024 07:43, Brian Lyles wrote:
> The ultimate goal of this series is to allow git-cherry-pick(1) to
> automatically drop redundant commits. The mechanism chosen is an
> `--empty` option that provides the same flexibility as the `--empty`
> options for git-rebase(1) and git-am(1).
>
> Some secondary goals are to improve the consistency in the values and
> documentation for this option across the three commands.
>
> See "Does extending `--empty` to git-cherry-pick make sense?" [1] for
> some context for why this option is desired in git-cherry-pick(1).
>
> [1]: https://lore.kernel.org/git/CAHPHrSevBdQF0BisR8VK=jM=wj1dTUYEVrv31gLerAzL9=Cd8Q@mail.gmail.com
>
> Along the way, I (with some help from Elijah and Phillip) found a few
> other things in the docs and related sequencer code to clean up.
Thanks for the revised patches - they are looking good and were a
pleasant read. I've left a few small comments, my main concern is the
change to `--keep-redundant-commits` in patch 6 which I'm not sure is
really worth the disruption.
Best Wishes
Phillip
> Brian Lyles (8):
> docs: address inaccurate `--empty` default with `--exec`
> docs: clean up `--empty` formatting in git-rebase(1) and git-am(1)
> rebase: update `--empty=ask` to `--empty=drop`
> sequencer: treat error reading HEAD as unborn branch
> sequencer: do not require `allow_empty` for redundant commit options
> cherry-pick: decouple `--allow-empty` and `--keep-redundant-commits`
> cherry-pick: enforce `--keep-redundant-commits` incompatibility
> cherry-pick: add `--empty` for more robust redundant commit handling
>
> Documentation/git-am.txt | 20 ++++---
> Documentation/git-cherry-pick.txt | 30 +++++++---
> Documentation/git-rebase.txt | 26 ++++++---
> builtin/rebase.c | 16 +++--
> builtin/revert.c | 40 +++++++++++--
> sequencer.c | 65 +++++++++++----------
> t/t3424-rebase-empty.sh | 55 ++++++++++++++++-
> t/t3501-revert-cherry-pick.sh | 11 ++++
> t/t3505-cherry-pick-empty.sh | 29 ++++++++-
> t/t3510-cherry-pick-sequence.sh | 40 +++++++++++++
> t/t3515-cherry-pick-incompatible-options.sh | 48 +++++++++++++++
> 11 files changed, 312 insertions(+), 68 deletions(-)
> create mode 100755 t/t3515-cherry-pick-incompatible-options.sh
>
^ permalink raw reply
* Re: [PATCH] rebase: make warning less passive aggressive
From: Junio C Hamano @ 2024-02-22 16:46 UTC (permalink / raw)
To: Jean-Noël AVILA
Cc: Michal Suchánek, Harmen Stoppels via GitGitGadget, git,
Harmen Stoppels
In-Reply-To: <7633780.EvYhyI6sBW@cayenne>
Jean-Noël AVILA <jn.avila@free.fr> writes:
> About "again and again", I was more refering to strings such as "could not
> stat '%s'" and then "could not stat file '%s'".
I completely agree with that.
But the message you responded was not about that kind of burden to
translators. It was arguing that it somehow is better to update the
in-code _("string") at the same time as updating msgid and msgstr in
the same patch (it may not have been directly "arguing for", but
essentially doing so with a rhetorical question).
So your "I do not want to translate again and again" thrown into
that context utterly confused me. I didn't quite see how the
"everything in one patch" approach would help translaters by not
having them translate "again and again".
Thanks for clearing my confusion.
^ permalink raw reply
* Re: [PATCH v2] rebase: make warning less passive aggressive
From: Junio C Hamano @ 2024-02-22 17:00 UTC (permalink / raw)
To: Kristoffer Haugsbakk; +Cc: Josh Soref, Harmen Stoppels, git
In-Reply-To: <759cbb30-96dd-456a-baab-b9451d400dcb@app.fastmail.com>
"Kristoffer Haugsbakk" <code@khaugsbakk.name> writes:
> I’ve interpreted this error as: I’m not quite sure but it looks like you
> are not in the middle of a rebase? Like it can’t be completely
> sure. Maybe from a more “primitive” time, implementation-wise?[1] :)
It certainly is a good point. At least one other message in the
same command is equally less assertive, e.g.
die(_("It looks like 'git am' is in progress. Cannot rebase."));
It may have been OK to be uncertain in the earlier times back when
we did not much know what we were doing, but by now we should be
more assertive, so that the end-users can more easily complain,
saying "The command said I was not rebasing, but I was! Here is
what I did", which would hopefully be a valuable feedback for
improving the logic to diagnose what state we are in.
So, making it clear that it is a statement of a fact, or at least we
are telling the users what we _think_ the state is assertively, is a
good idea either way.
Thanks.
^ permalink raw reply
* Re: [PATCH v2 4/5] Always check `parse_tree*()`'s return value
From: Junio C Hamano @ 2024-02-22 17:05 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Eric Sunshine, Johannes Schindelin via GitGitGadget, git,
Patrick Steinhardt
In-Reply-To: <dbde46bc-501e-8433-0b8d-b83d0ca9e759@gmx.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> If you happen to reroll for some reason, perhaps: s/Always/always/
>
> Unless I am missing something we only ask the part of a oneline after an
> initial "<something>:" to be downcased.
>
> At least all the "Merge branch [...]" commits are still capitalized and
> nobody complains ;-)
Of course. Merge commits and single-parent commits are very
different.
I suspect that the lack of "<area>:" was what triggered the comment,
but this being more about the callers all over the place in the
code, it may be hard to say what "area" this belongs to. If I were
pressed to choose:
Subject: parse_tree*(): make callers always check return value
would probably be what I would choose, but I usually opt for going
the lazy way of not labelling the area at all ;-).
^ permalink raw reply
* Re: [PATCH v3 1/5] merge-tree: fail with a non-zero exit code on missing tree objects
From: Junio C Hamano @ 2024-02-22 17:13 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <11b9cd8c5da5e6792ce940ea29d2e93e57731555.1708612605.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> - parse_tree(merge_base);
> - parse_tree(side1);
> - parse_tree(side2);
> + if (parse_tree(merge_base) < 0 ||
> + parse_tree(side1) < 0 ||
> + parse_tree(side2) < 0)
> + return -1;
Obviously good.
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 7d0fa74da74..908c9b540c8 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -951,4 +951,15 @@ test_expect_success '--merge-base with tree OIDs' '
> test_cmp with-commits with-trees
> '
>
> +test_expect_success 'error out on missing tree objects' '
> + git init --bare missing-tree.git &&
> + git rev-list side3 >list &&
> + git rev-parse side3^: >>list &&
> + git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing <list &&
> + side3=$(git rev-parse side3) &&
> + test_must_fail git --git-dir=missing-tree.git merge-tree $side3^ $side3 >actual 2>err &&
> + test_grep "Could not read $(git rev-parse $side3:)" err &&
> + test_must_be_empty actual
> +'
I very much like the way this test emulates an operation in a
repository that lack certaion objects so cleanly, and wish we
had used this pattern instead of poking at loose object files
in hundreds of existing tests (#leftoverbits obviously).
It also justifies why a silent "return -1" in the patch is
sufficient ;-)
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/5] merge-ort: do check `parse_tree()`'s return value
From: Junio C Hamano @ 2024-02-22 17:18 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <f01f4eb011b400faeff1c33934775a521dec7a3d.1708612605.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> This change is not accompanied by a regression test because the code in
> question is only reached at the `checkout` stage, i.e. after the merge
> has happened (and therefore the tree objects could only be missing if
> the disk had gone bad in that short time window, or something similarly
> tricky to recreate in the test suite).
Makes sense.
A complete tangent I wonder is if unit-test-minded folks have clever
ideas to allow better test coverage, perhaps injecting a failure on
demand to any codepath (in this case, the codepath to write the
resulting tree) to simulate a situation where we fail to parse the
tree.
In any case, the patch looks good, of course, and I see no need for
further comments.
Thanks.
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> merge-ort.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index c37fc035f13..79d9e18f63d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4379,9 +4379,11 @@ static int checkout(struct merge_options *opt,
> unpack_opts.verbose_update = (opt->verbosity > 2);
> unpack_opts.fn = twoway_merge;
> unpack_opts.preserve_ignored = 0; /* FIXME: !opts->overwrite_ignore */
> - parse_tree(prev);
> + if (parse_tree(prev) < 0)
> + return -1;
> init_tree_desc(&trees[0], prev->buffer, prev->size);
> - parse_tree(next);
> + if (parse_tree(next) < 0)
> + return -1;
> init_tree_desc(&trees[1], next->buffer, next->size);
>
> ret = unpack_trees(2, trees, &unpack_opts);
^ permalink raw reply
* Re: Segfault: git show-branch --reflog refs/pullreqs/1
From: Jeff King @ 2024-02-22 17:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Patrick Steinhardt, Yasushi SHOJI, Denton Liu, Git Mailing List
In-Reply-To: <xmqq34tkiho9.fsf@gitster.g>
On Thu, Feb 22, 2024 at 08:32:06AM -0800, Junio C Hamano wrote:
> > Hum, I dunno. I don't really understand what the benefit of this
> > fallback is. If a user wants to know the latest object ID of the ref
> > they shouldn't ask for `foo@{0}`, they should ask for `foo`. On the
> > other hand, if I want to know "What is the latest entry in the ref's
> > log", I want to ask for `foo@{0}`.
>
> The usability hack helps small things like "List up to 4 most recent
> states from a branch", e.g.
>
> for nth in $(seq 0 3)
> do
> git rev-parse --quiet --verify @$nth || break
> git show -s --format="@$nth %h %s" @$nth
> done
>
> vs
>
> for rev in HEAD @{1} @{2} @{3}
> do
> git rev-parse --quiet --verify "$rev" || break
> git show -s --format="$rev %h %s" "$rev"
> done
>
> by not forcing you to special case the "current".
In those examples, though, it is useful precisely because you _do_ have
a reflog, and ref@{0} is conceptually the top entry (which brought us to
the same state as just "ref").
The question to me is more "is ref@{0} useful on its own, even when you
do not necessarily have a reflog". That I am less sure of.
> Ideally, "foo@{0}" should have meant "the state immediately before
> the current state of foo" so that "foo" is the unambiguous and only
> way to refer to "the current state of foo", but that was not how we
> implemented the reflog, allowing a subtle repository corruption
> where the latest state of a branch according to the reflog and the
> current commit pointed by the branch can diverge. But that wasn't
> what we did, and instead both "foo@{0}" and "foo" mean to refer to
> "the latest state of foo". We can take advantage of that misdesign
> and allow "foo@{0}" to refer to the same commit as "foo", at least
> at the get_oid_basic() level, whether a reflog actually exists or
> not, and that would make the whole thing more consistent.
I think there is some confusion here between how get_oid_basic() behaves
and how read_ref_at() is used for something like show-branch. In the
former case, we only care about getting an oid as output, but in the
latter we actually want the reflog entry (because we care about its
timestamp, message, and so on).
So in terms of reflog entries, ref@{0} should refer to the most recent
entry. And the oid it returns should be the end-result of that entry,
which (in a non corrupted repository) is identical to the current ref
value. And that "should" is reinforced by stuff like:
git log -g "%gd %gs"
which shows the most recent entry as HEAD@{0}.
I think 6436a20284 (refs: allow @{n} to work with n-sized reflog,
2021-01-07) confused things mightily by having read_ref_at() with a
count of "n" find entry "n-1" instead, and then return the oid for the
"old" value. That makes get_oid_basic() work, because it doesn't care
about which entry we found, only the oid. But for show-branch, now we
are confused about which reflog entry ref@{1}, etc, refers to (but
ref@{0} still works because of the weird special-casing done by that
commit).
I think we should fix that (and I have the start of some patches to do
so). But in that world-view, having read_ref_at() return anything for a
count of "0" when there is no reflog does not make sense. There is no
such entry!
OTOH, we face the same problem when asking about ref@{N} when there are
only N entries. We can provide an oid (based on the "old" value from the
oldest entry we did see), but we have to "fake" the reflog entry data
(like the messsage), since there wasn't one.
So the open questions to me are:
- should this faking happen in read_ref_at(), just returning a dummy
reflog message? Or should we keep read_ref_at() purely about finding
the entry, and put the special-casing into get_oid_basic(), which
only cares about the oid result?
- wherever we put the faking, should we only fake ref@{N} when N > 0?
Or should we also fake ref@{0} when there is no reflog at all?
If none of this makes sense, it is because I am only now untangling what
is going on with 6436a20284. ;) I will try to polish my proposed patches
and hopefully that will explain it a bit more clearly (I may not get to
it until tomorrow though).
-Peff
^ permalink raw reply
* Re: [PATCH v3 3/5] t4301: verify that merge-tree fails on missing blob objects
From: Junio C Hamano @ 2024-02-22 17:27 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Eric Sunshine, Johannes Schindelin
In-Reply-To: <e82fdf7fbcbf12fffdf4a720927c2f4f006068f8.1708612605.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> We just fixed a problem where `merge-tree` would not fail on missing
> tree objects. Let's ensure that that problem does not occur with blob
> objects (and won't, in the future, either).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/t4301-merge-tree-write-tree.sh | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> index 908c9b540c8..d4463a45706 100755
> --- a/t/t4301-merge-tree-write-tree.sh
> +++ b/t/t4301-merge-tree-write-tree.sh
> @@ -962,4 +962,20 @@ test_expect_success 'error out on missing tree objects' '
> test_must_be_empty actual
> '
>
> +test_expect_success 'error out on missing blob objects' '
> + echo 1 | git hash-object -w --stdin >blob1 &&
> + echo 2 | git hash-object -w --stdin >blob2 &&
> + echo 3 | git hash-object -w --stdin >blob3 &&
> + printf "100644 blob $(cat blob1)\tblob\n" | git mktree >tree1 &&
> + printf "100644 blob $(cat blob2)\tblob\n" | git mktree >tree2 &&
> + printf "100644 blob $(cat blob3)\tblob\n" | git mktree >tree3 &&
> + git init --bare missing-blob.git &&
> + cat blob1 blob3 tree1 tree2 tree3 |
> + git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
> + test_must_fail git --git-dir=missing-blob.git >actual 2>err \
> + merge-tree --merge-base=$(cat tree1) $(cat tree2) $(cat tree3) &&
> + test_grep "unable to read blob object $(cat blob2)" err &&
> + test_must_be_empty actual
> +'
It would have been even easier to see that blob2 is what we expect
to be complained about, if you listed all objects and filtered blob2
out, but the number of objects involved here is so small, a "cat" of
all objects we want to keep is OK here.
Again, I very much love the way the test repository/object store
that lack certain objects are constructed without making our hands
dirty.
I see no need for further comments. Looking very good.
^ permalink raw reply
* [PATCH v5 0/3] Introduce Git Standard Library
From: Calvin Wan @ 2024-02-22 17:50 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, Jonathan Tan, phillip.wood123, Junio C Hamano
In-Reply-To: <cover.1696021277.git.jonathantanmy@google.com>
While it has been a while since the last reroll of this series[1], the
contents and boundaries of git-std-lib have not changed. The focus for
this reroll are improvements to the Makefile, test file, and
documentation. Patch 1 contains a small fix for a missing include
discovered by Jonathan Tan. Patch 2 introduces the Git Standard Library.
And patch 3 introduces preliminary testing and usage examples for the
Git Standard Library.
One important piece of feedback I received from the previous series is
that Git should be the first consumer of its own libraries. Objects in
git-std-lib.a are no longer contained in LIB_OBJS, but rather directly
built into git-std-lib.a and then linked into git.a. There is some
functionality that is used by git-std-lib.a that's not included in
git-std-lib.a, such as tracing support. These have been stubbed out into
git-stub-lib.a and can be built with git-std-lib.a to be used
externally. Thank you to Philip Wood for these suggestions[2]!
The test file and Makefile have been updated to include cleanups
suggested by Junio. Since git-std-lib.a is now a dependency of Git, the
test file has also been included as part of the test suite. The series
has been rebased onto a recent version of `next` and function calls that
have been added/changed since the last reroll have also been included
into the test file.
Finally, through our libification syncs, there have been various topics
and questions brought up that would be better clarified with additional
documentation in technical/git-std-lib.txt.
[1]
https://lore.kernel.org/git/20230908174134.1026823-1-calvinwan@google.com/
[2]
https://lore.kernel.org/git/98f3edcf-7f37-45ff-abd2-c0038d4e0589@gmail.com/
Calvin Wan (2):
git-std-lib: introduce Git Standard Library
test-stdlib: show that git-std-lib is independent
Jonathan Tan (1):
pager: include stdint.h because uintmax_t is used
Documentation/Makefile | 1 +
Documentation/technical/git-std-lib.txt | 170 +++++++++++++++
Makefile | 71 +++++--
pager.h | 2 +
strbuf.h | 2 +
stubs/misc.c | 34 +++
stubs/pager.c | 6 +
stubs/trace2.c | 27 +++
t/helper/.gitignore | 1 +
t/helper/test-stdlib.c | 266 ++++++++++++++++++++++++
t/t0082-std-lib.sh | 11 +
11 files changed, 575 insertions(+), 16 deletions(-)
create mode 100644 Documentation/technical/git-std-lib.txt
create mode 100644 stubs/misc.c
create mode 100644 stubs/pager.c
create mode 100644 stubs/trace2.c
create mode 100644 t/helper/test-stdlib.c
create mode 100755 t/t0082-std-lib.sh
Range-diff against v4:
1: 2f99eb2ca4 < -: ---------- hex-ll: split out functionality from hex
2: 7b2d123628 < -: ---------- wrapper: remove dependency to Git-specific internal file
3: b37beb206a < -: ---------- config: correct bad boolean env value error message
4: 3a827cf45c < -: ---------- parse: create new library for parsing strings and env values
5: f8e4ac50a0 < -: ---------- git-std-lib: introduce git standard library
6: 7840e1830a < -: ---------- git-std-lib: add test file to call git-std-lib.a functions
-: ---------- > 1: 57b751a497 pager: include stdint.h because uintmax_t is used
-: ---------- > 2: e64f3c73c2 git-std-lib: introduce Git Standard Library
-: ---------- > 3: e2d930f729 test-stdlib: show that git-std-lib is independent
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply
* [PATCH v5 1/3] pager: include stdint.h because uintmax_t is used
From: Calvin Wan @ 2024-02-22 17:50 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, Calvin Wan, phillip.wood123, Junio C Hamano
In-Reply-To: <cover.1696021277.git.jonathantanmy@google.com>
From: Jonathan Tan <jonathantanmy@google.com>
pager.h uses uintmax_t but does not include stdint.h. Therefore, add
this include statement.
This was discovered when writing a stub pager.c file.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
Signed-off-by: Calvin Wan <calvinwan@google.com>
---
| 2 ++
1 file changed, 2 insertions(+)
--git a/pager.h b/pager.h
index b77433026d..015bca95e3 100644
--- a/pager.h
+++ b/pager.h
@@ -1,6 +1,8 @@
#ifndef PAGER_H
#define PAGER_H
+#include <stdint.h>
+
struct child_process;
const char *git_pager(int stdout_is_tty);
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v5 2/3] git-std-lib: introduce Git Standard Library
From: Calvin Wan @ 2024-02-22 17:50 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, Jonathan Tan, phillip.wood123, Junio C Hamano
In-Reply-To: <cover.1696021277.git.jonathantanmy@google.com>
This commit contains:
- Makefile rules for git-std-lib.a
- code and Makefile rules for git-stub-lib.a
- description and rationale of the above in Documentation/
Quoting from documentation introduced in this commit:
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.
Code demonstrating the use of git-std-lib.a and git-stub-lib.a will be
in a subsequent commit.
Signed-off-by: Calvin Wan <calvinwan@google.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Documentation/Makefile | 1 +
Documentation/technical/git-std-lib.txt | 170 ++++++++++++++++++++++++
Makefile | 48 +++++--
stubs/misc.c | 33 +++++
| 6 +
stubs/trace2.c | 27 ++++
6 files changed, 274 insertions(+), 11 deletions(-)
create mode 100644 Documentation/technical/git-std-lib.txt
create mode 100644 stubs/misc.c
create mode 100644 stubs/pager.c
create mode 100644 stubs/trace2.c
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 3f2383a12c..f1dc673838 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -110,6 +110,7 @@ TECH_DOCS += SubmittingPatches
TECH_DOCS += ToolsForGit
TECH_DOCS += technical/bitmap-format
TECH_DOCS += technical/bundle-uri
+TECH_DOCS += technical/git-std-lib
TECH_DOCS += technical/hash-function-transition
TECH_DOCS += technical/long-running-process-protocol
TECH_DOCS += technical/multi-pack-index
diff --git a/Documentation/technical/git-std-lib.txt b/Documentation/technical/git-std-lib.txt
new file mode 100644
index 0000000000..3d9aa121ac
--- /dev/null
+++ b/Documentation/technical/git-std-lib.txt
@@ -0,0 +1,170 @@
+= 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. Git itself is also built off of the Git Standard
+Library.
+
+== Dependency graph in libified Git
+
+Before the introduction of the Git Standard Library, all objects defined
+in the Git library are compiled and archived into a singular file,
+libgit.a, which is then 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 slightly alters this build
+flow by separating out libgit.a into libgit.a and git-std-lib.a.
+
+With our current method of building Git, we can imagine the dependency
+graph as such:
+
+ Git
+ /\
+ / \
+ / \
+ libgit.a ext deps
+
+We want to separate out potential libraries from libgit.a and have
+libgit.a depend on them, which would possibly look like:
+
+ Git
+ /\
+ / \
+ / \
+ libgit.a ext deps
+ /\
+ / \
+ / \
+object-store.a (other lib)
+ | /
+ | /
+ | /
+ | /
+ | /
+ | /
+ | /
+git-std-lib.a
+
+Instead of containing all objects in Git, libgit.a would contain objects
+that are not built by libraries it links against. Consequently, if
+someone wanted a custom build of Git with a 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 the selected object files in the Git Standard
+Library is the result of two observations within the Git
+codebase:
+ 1. every file includes git-compat-util.h which defines functions
+ in a couple of different files
+ 2. 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 it
+would impractical for them to 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/*
+
+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
+(see STUB_LIB_OBJS in the Makefile).
+
+== Files inside of Git Standard Library
+
+The set of files in git-std-lib.a can be found in STD_LIB_OBJS and COMPAT_OBJS
+in the Makefile.
+
+When these files are compiled together with the files in STUB_LIB_OBJS (or
+user-provided files that provide the same functions), they form a complete
+library.
+
+== 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/
diff --git a/Makefile b/Makefile
index 4e255c81f2..d37ea9d34b 100644
--- a/Makefile
+++ b/Makefile
@@ -669,6 +669,8 @@ FUZZ_PROGRAMS =
GIT_OBJS =
LIB_OBJS =
SCALAR_OBJS =
+STD_LIB_OBJS =
+STUB_LIB_OBJS =
OBJECTS =
OTHER_PROGRAMS =
PROGRAM_OBJS =
@@ -923,6 +925,8 @@ TEST_SHELL_PATH = $(SHELL_PATH)
LIB_FILE = libgit.a
XDIFF_LIB = xdiff/lib.a
+STD_LIB_FILE = git-std-lib.a
+STUB_LIB_FILE = git-stub-lib.a
REFTABLE_LIB = reftable/libreftable.a
REFTABLE_TEST_LIB = reftable/libreftable_test.a
@@ -962,7 +966,6 @@ COCCI_SOURCES = $(filter-out $(THIRD_PARTY_SOURCES),$(FOUND_C_SOURCES))
LIB_H = $(FOUND_H_SOURCES)
-LIB_OBJS += abspath.o
LIB_OBJS += add-interactive.o
LIB_OBJS += add-patch.o
LIB_OBJS += advice.o
@@ -1004,8 +1007,6 @@ LIB_OBJS += convert.o
LIB_OBJS += copy.o
LIB_OBJS += credential.o
LIB_OBJS += csum-file.o
-LIB_OBJS += ctype.o
-LIB_OBJS += date.o
LIB_OBJS += decorate.o
LIB_OBJS += delta-islands.o
LIB_OBJS += diagnose.o
@@ -1046,7 +1047,6 @@ 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
@@ -1097,7 +1097,6 @@ 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
@@ -1152,7 +1151,6 @@ LIB_OBJS += sparse-index.o
LIB_OBJS += split-index.o
LIB_OBJS += stable-qsort.o
LIB_OBJS += statinfo.o
-LIB_OBJS += strbuf.o
LIB_OBJS += streaming.o
LIB_OBJS += string-list.o
LIB_OBJS += strmap.o
@@ -1189,21 +1187,32 @@ LIB_OBJS += unpack-trees.o
LIB_OBJS += upload-pack.o
LIB_OBJS += url.o
LIB_OBJS += urlmatch.o
-LIB_OBJS += usage.o
LIB_OBJS += userdiff.o
-LIB_OBJS += utf8.o
LIB_OBJS += varint.o
LIB_OBJS += version.o
LIB_OBJS += versioncmp.o
LIB_OBJS += walker.o
LIB_OBJS += wildmatch.o
LIB_OBJS += worktree.o
-LIB_OBJS += wrapper.o
LIB_OBJS += write-or-die.o
LIB_OBJS += ws.o
LIB_OBJS += wt-status.o
LIB_OBJS += xdiff-interface.o
+STD_LIB_OBJS += abspath.o
+STD_LIB_OBJS += ctype.o
+STD_LIB_OBJS += date.o
+STD_LIB_OBJS += hex-ll.o
+STD_LIB_OBJS += parse.o
+STD_LIB_OBJS += strbuf.o
+STD_LIB_OBJS += usage.o
+STD_LIB_OBJS += utf8.o
+STD_LIB_OBJS += wrapper.o
+
+STUB_LIB_OBJS += stubs/trace2.o
+STUB_LIB_OBJS += stubs/pager.o
+STUB_LIB_OBJS += stubs/misc.o
+
BUILTIN_OBJS += builtin/add.o
BUILTIN_OBJS += builtin/am.o
BUILTIN_OBJS += builtin/annotate.o
@@ -1352,7 +1361,7 @@ UNIT_TEST_OBJS = $(patsubst %,$(UNIT_TEST_DIR)/%.o,$(UNIT_TEST_PROGRAMS))
UNIT_TEST_OBJS += $(UNIT_TEST_DIR)/test-lib.o
# xdiff and reftable libs may in turn depend on what is in libgit.a
-GITLIBS = common-main.o $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
+GITLIBS = common-main.o $(STD_LIB_FILE) $(LIB_FILE) $(XDIFF_LIB) $(REFTABLE_LIB) $(LIB_FILE)
EXTLIBS =
GIT_USER_AGENT = git/$(GIT_VERSION)
@@ -2693,6 +2702,8 @@ OBJECTS += $(XDIFF_OBJS)
OBJECTS += $(FUZZ_OBJS)
OBJECTS += $(REFTABLE_OBJS) $(REFTABLE_TEST_OBJS)
OBJECTS += $(UNIT_TEST_OBJS)
+OBJECTS += $(STD_LIB_OBJS)
+OBJECTS += $(STUB_LIB_OBJS)
ifndef NO_CURL
OBJECTS += http.o http-walker.o remote-curl.o
@@ -3686,7 +3697,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) $(STUB_LIB_FILE)
$(RM) $(ALL_PROGRAMS) $(SCRIPT_LIB) $(BUILT_INS) $(OTHER_PROGRAMS)
$(RM) $(TEST_PROGRAMS)
$(RM) $(FUZZ_PROGRAMS)
@@ -3878,3 +3889,18 @@ $(UNIT_TEST_PROGS): $(UNIT_TEST_BIN)/%$X: $(UNIT_TEST_DIR)/%.o $(UNIT_TEST_DIR)/
build-unit-tests: $(UNIT_TEST_PROGS)
unit-tests: $(UNIT_TEST_PROGS)
$(MAKE) -C t/ unit-tests
+
+### Libified Git rules
+
+# git-std-lib.a
+# Programs other than git should compile this with
+# make NO_GETTEXT=YesPlease git-std-lib.a
+# and link against git-stub-lib.a (if the default no-op functionality is fine)
+# or a custom .a file with the same interface as git-stub-lib.a (if custom
+# functionality is needed) as well.
+$(STD_LIB_FILE): $(STD_LIB_OBJS) $(COMPAT_OBJS)
+ $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
+
+# git-stub-lib.a
+$(STUB_LIB_FILE): $(STUB_LIB_OBJS)
+ $(QUIET_AR)$(RM) $@ && $(AR) $(ARFLAGS) $@ $^
diff --git a/stubs/misc.c b/stubs/misc.c
new file mode 100644
index 0000000000..92da76fd46
--- /dev/null
+++ b/stubs/misc.c
@@ -0,0 +1,33 @@
+#include <assert.h>
+#include <stdlib.h>
+
+#ifndef NO_GETTEXT
+/*
+ * NEEDSWORK: This is enough to link our unit tests against
+ * git-std-lib.a built with gettext support. We don't really support
+ * programs other than git using git-std-lib.a with gettext support
+ * yet. To do that we need to start using dgettext() rather than
+ * gettext() in our code.
+ */
+int git_gettext_enabled = 0;
+#endif
+
+int common_exit(const char *file, int line, int code);
+
+int common_exit(const char *file, int line, int code)
+{
+ exit(code);
+}
+
+#if !defined(__MINGW32__) && !defined(_MSC_VER)
+int lstat_cache_aware_rmdir(const char *path);
+
+int lstat_cache_aware_rmdir(const char *path)
+{
+ /*
+ * This function should not be called by programs linked
+ * against git-stub-lib.a
+ */
+ assert(0);
+}
+#endif
--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;
+}
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) { }
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
* [PATCH v5 3/3] test-stdlib: show that git-std-lib is independent
From: Calvin Wan @ 2024-02-22 17:50 UTC (permalink / raw)
To: git; +Cc: Calvin Wan, Jonathan Tan, phillip.wood123, Junio C Hamano
In-Reply-To: <cover.1696021277.git.jonathantanmy@google.com>
Add a test file that calls some functions defined in git-std-lib.a
object files to showcase that they do not reference missing objects and
that, together with git-stub-lib.a, git-std-lib.a can stand on its own.
As described in test-stdlib.c, this can probably be removed once we have
unit tests.
The variable TEST_PROGRAMS is moved lower in the Makefile after
NO_POSIX_GOODIES is defined in config.make.uname. TEST_PROGRAMS isn't
used earlier than that so this change should be safe.
Signed-off-by: Calvin Wan <calvinwan@google.com>
Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
Makefile | 23 +++-
strbuf.h | 2 +
stubs/misc.c | 1 +
t/helper/.gitignore | 1 +
t/helper/test-stdlib.c | 266 +++++++++++++++++++++++++++++++++++++++++
t/t0082-std-lib.sh | 11 ++
6 files changed, 299 insertions(+), 5 deletions(-)
create mode 100644 t/helper/test-stdlib.c
create mode 100755 t/t0082-std-lib.sh
diff --git a/Makefile b/Makefile
index d37ea9d34b..1d762ce13a 100644
--- a/Makefile
+++ b/Makefile
@@ -870,9 +870,7 @@ TEST_BUILTINS_OBJS += test-xml-encode.o
# Do not add more tests here unless they have extra dependencies. Add
# them in TEST_BUILTINS_OBJS above.
TEST_PROGRAMS_NEED_X += test-fake-ssh
-TEST_PROGRAMS_NEED_X += test-tool
-
-TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+TEST_PROGRAMS_NEED_X += $(info tpnxnpg=$(NO_POSIX_GOODIES))test-tool
# List built-in command $C whose implementation cmd_$C() is not in
# builtin/$C.o but is linked in as part of some other command.
@@ -2678,6 +2676,16 @@ REFTABLE_TEST_OBJS += reftable/stack_test.o
REFTABLE_TEST_OBJS += reftable/test_framework.o
REFTABLE_TEST_OBJS += reftable/tree_test.o
+ifndef NO_POSIX_GOODIES
+TEST_PROGRAMS_NEED_X += test-stdlib
+MY_VAR = not_else
+$(info insideifndefnpg=$(NO_POSIX_GOODIES))
+else
+MY_VAR = else
+endif
+
+TEST_PROGRAMS = $(info tptpnx=$(TEST_PROGRAMS_NEED_X) myvar=$(MY_VAR))$(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
+
TEST_OBJS := $(patsubst %$X,%.o,$(TEST_PROGRAMS)) $(patsubst %,t/helper/%,$(TEST_BUILTINS_OBJS))
.PHONY: test-objs
@@ -3204,7 +3212,11 @@ GIT-PYTHON-VARS: FORCE
fi
endif
-test_bindir_programs := $(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+test_bindir_programs := $(info tbptpnx=$(TEST_PROGRAMS_NEED_X))$(patsubst %,bin-wrappers/%,$(BINDIR_PROGRAMS_NEED_X) $(BINDIR_PROGRAMS_NO_X) $(TEST_PROGRAMS_NEED_X))
+
+t/helper/test-stdlib$X: t/helper/test-stdlib.o GIT-LDFLAGS $(STD_LIB_FILE) $(STUB_LIB_FILE) $(GITLIBS)
+ $(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
+ $< $(STD_LIB_FILE) $(STUB_LIB_FILE) $(EXTLIBS)
all:: $(TEST_PROGRAMS) $(test_bindir_programs) $(UNIT_TEST_PROGS)
@@ -3635,7 +3647,8 @@ ifneq ($(INCLUDE_DLLS_IN_ARTIFACTS),)
OTHER_PROGRAMS += $(shell echo *.dll t/helper/*.dll t/unit-tests/bin/*.dll)
endif
-artifacts-tar:: $(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
+# Added an info for debugging
+artifacts-tar:: $(info npg=$(NO_POSIX_GOODIES) cc=$(COMPAT_CFLAGS) tp=$(TEST_PROGRAMS))$(ALL_COMMANDS_TO_INSTALL) $(SCRIPT_LIB) $(OTHER_PROGRAMS) \
GIT-BUILD-OPTIONS $(TEST_PROGRAMS) $(test_bindir_programs) \
$(UNIT_TEST_PROGS) $(MOFILES)
$(QUIET_SUBDIR0)templates $(QUIET_SUBDIR1) \
diff --git a/strbuf.h b/strbuf.h
index e959caca87..f775416307 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -1,6 +1,8 @@
#ifndef STRBUF_H
#define STRBUF_H
+#include "git-compat-util.h"
+
/*
* NOTE FOR STRBUF DEVELOPERS
*
diff --git a/stubs/misc.c b/stubs/misc.c
index 92da76fd46..8d80581e39 100644
--- a/stubs/misc.c
+++ b/stubs/misc.c
@@ -9,6 +9,7 @@
* yet. To do that we need to start using dgettext() rather than
* gettext() in our code.
*/
+#include "gettext.h"
int git_gettext_enabled = 0;
#endif
diff --git a/t/helper/.gitignore b/t/helper/.gitignore
index 8c2ddcce95..5cec3b357f 100644
--- a/t/helper/.gitignore
+++ b/t/helper/.gitignore
@@ -1,2 +1,3 @@
/test-tool
/test-fake-ssh
+/test-stdlib
diff --git a/t/helper/test-stdlib.c b/t/helper/test-stdlib.c
new file mode 100644
index 0000000000..460b472fb4
--- /dev/null
+++ b/t/helper/test-stdlib.c
@@ -0,0 +1,266 @@
+#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
+ *
+ * NEEDSWORK: The purpose of this file is to show that an executable can be
+ * built with git-std-lib.a and git-stub-lib.a, and then executed. If there
+ * is another executable that demonstrates this (for example, a unit test that
+ * takes the form of an executable compiled with git-std-lib.a and git-stub-
+ * lib.a), this file can be removed.
+ */
+
+static 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");
+}
+
+static void hex_ll_funcs(void) {
+ unsigned char c;
+
+ fprintf(stderr, "calling hex-ll functions\n");
+
+ hexval('c');
+ hex2chr("A1");
+ hex_to_bytes(&c, "A1", 1);
+}
+
+static 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;
+}
+
+static void strbuf_funcs(void) {
+ struct strbuf *sb = xmalloc(sizeof(*sb));
+ struct strbuf *sb2 = xmalloc(sizeof(*sb2));
+ struct strbuf sb3 = STRBUF_INIT;
+ struct string_list list = STRING_LIST_INIT_NODUP;
+ int fd = open("/dev/null", O_RDONLY);
+
+ fprintf(stderr, "calling strbuf functions\n");
+
+ fprintf(stderr, "at line %d\n", __LINE__);
+ starts_with("foo", "bar");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ istarts_with("foo", "bar");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_init(sb, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_init(sb2, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_release(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_attach(sb, strbuf_detach(sb, NULL), 0, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_swap(sb, sb2);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_setlen(sb, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_trim(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_trim_trailing_dir_sep(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_trim_trailing_newline(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_reencode(sb, "foo", "bar");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_tolower(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_separated_string_list(sb, " ", &list);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_list_free(strbuf_split_buf("foo bar", 8, ' ', -1));
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_cmp(sb, sb2);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addch(sb, 1);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_splice(sb, 0, 1, "foo", 3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_insert(sb, 0, "foo", 3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_insertf(sb, 0, "%s", "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_remove(sb, 0, 1);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add(sb, "foo", 3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addbuf(sb, sb2);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_join_argv(sb, 0, NULL, ' ');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addchars(sb, 1, 1);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addstr(sb, "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_commented_lines(sb, "foo", 3, '#');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_commented_addf(sb, '#', "%s", "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addbuf_percentquote(sb, &sb3);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_percentencode(sb, "foo", STRBUF_ENCODE_SLASH);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_fread(sb, 0, stdin);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_read(sb, fd, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_read_once(sb, fd, 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_write(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_readlink(sb, "/dev/null", 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getcwd(sb);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getwholeline(sb, stderr, '\n');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_appendwholeline(sb, stderr, '\n');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getline(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getline_lf(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getline_nul(sb, stderr);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_getwholeline_fd(sb, fd, '\n');
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_read_file(sb, "/dev/null", 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_add_lines(sb, "foo", "bar", 0);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addstr_xml_quoted(sb, "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_addstr_urlencode(sb, "foo", allow_unencoded_fn);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_humanise_bytes(sb, 42);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ strbuf_humanise_rate(sb, 42);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ printf_ln("%s", sb->buf);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ fprintf_ln(stderr, "%s", sb->buf);
+ fprintf(stderr, "at line %d\n", __LINE__);
+ xstrdup_tolower("foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ xstrdup_toupper("foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+ xstrfmt("%s", "foo");
+ fprintf(stderr, "at line %d\n", __LINE__);
+}
+
+static void error_builtin(const char *err, va_list params) {}
+static void warn_builtin(const char *err, va_list params) {}
+
+static void usage_funcs(void) {
+ fprintf(stderr, "calling usage functions\n");
+ error("foo");
+ error_errno("foo");
+ die_message("foo");
+ die_message_errno("foo");
+ warning("foo");
+ warning_errno("foo");
+
+ get_die_message_routine();
+ set_error_routine(error_builtin);
+ get_error_routine();
+ set_warn_routine(warn_builtin);
+ get_warn_routine();
+}
+
+static void wrapper_funcs(void) {
+ int tmp;
+ 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");
+
+ 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);
+ 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(int argc, const char **argv) {
+ 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;
+}
diff --git a/t/t0082-std-lib.sh b/t/t0082-std-lib.sh
new file mode 100755
index 0000000000..0d5a024deb
--- /dev/null
+++ b/t/t0082-std-lib.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+
+test_description='Test git-std-lib compilation'
+
+. ./test-lib.sh
+
+test_expect_success !WINDOWS 'stdlib-test compiles and runs' '
+ test-stdlib
+'
+
+test_done
--
2.44.0.rc0.258.g7320e95886-goog
^ permalink raw reply related
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