* Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...
From: Junio C Hamano @ 2024-02-07 16:34 UTC (permalink / raw)
To: Linus Arver
Cc: Christian Couder, git, Patrick Steinhardt, John Cai,
Christian Couder, Elijah Newren, Jeff Hostetler
In-Reply-To: <owlyttmkmwaf.fsf@fine.c.googlers.com>
Linus Arver <linusa@google.com> writes:
> IOW, make the minority (certainly not majority, I think?) of users who
> really need the error propagation use the (new) extra flag, while the
> rest of us (including the version of you who was surprised by the
> limited behavior of "--missing=...", enough to write this series) don't
> have to.
I am skeptical that we even want that. Those who want to use the
"--missing" and care about special casing missing starting points
can easily check with things like "cat-file -e" before even running
the "rev-list --missing".
^ permalink raw reply
* Re: [PATCH 3/3] rev-list: add --allow-missing-tips to be used with --missing=...
From: Christian Couder @ 2024-02-07 16:38 UTC (permalink / raw)
To: Linus Arver
Cc: Junio C Hamano, git, Patrick Steinhardt, John Cai,
Christian Couder, Elijah Newren, Jeff Hostetler
In-Reply-To: <owlyttmkmwaf.fsf@fine.c.googlers.com>
On Wed, Feb 7, 2024 at 10:57 AM Linus Arver <linusa@google.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:
>
> > On Thu, Feb 1, 2024 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
> >>
> >> Christian Couder <christian.couder@gmail.com> writes:
> >>
> >> > When such a command is used to find the dependencies of some objects,
> >> > for example the dependencies of quarantined objects, it would be
> >> > better if the command would instead consider such missing objects,
> >> > especially commits, in the same way as other missing objects.
> >> >
> >> > If, for example `--missing=print` is used, it would be nice for some
> >> > use cases if the missing tips passed as arguments were reported in
> >> > the same way as other missing objects instead of the command just
> >> > failing.
> >> >
> >> > Let's introduce a new `--allow-missing-tips` option to make it work
> >> > like this.
> >>
> >> An obvious question is if this even needs to be a new option. What
> >> are expected use cases where --missing=print without this option is
> >> beneficial?
> >
> > I am not sure if such a case is really beneficial but some
> > people/script/forges might rely on an error from `git rev-list
> > --missing=print` to propagate back an error to some user interface.
>
> I currently learn toward just making the new flag's behavior be absorved
> into the existing "--missing=..." flag.
Ok, then I am going to implement that in the next version I will send.
> Nevertheless, you raise an
> interesting concern.
>
> Perhaps a compromise would be to make "--missing=..." learn the new
> behavior of this patch as Junio suggested, but to introduce a new flag,
> something like "--fail-on-missing-tips" to fail early if any of the tip
> commits' objects are missing? That way we could keep the current
> "strict" behavior of complaining if we feed rev-list any tips whose
> objects are missing. And for the vast majority of cases the
> "--missing=..." flag could (intuitively) gracefully handle tips with
> missing objects and you wouldn't have to pass in the additional flag.
>
> IOW, make the minority (certainly not majority, I think?) of users who
> really need the error propagation use the (new) extra flag, while the
> rest of us (including the version of you who was surprised by the
> limited behavior of "--missing=...", enough to write this series) don't
> have to.
I agree that the majority of users would prefer `git rev-list` with
"--missing=<arg>" (when <arg> is not "error") not to error out when
one of the tips is missing. And yeah, indeed at GitLab, we are among
this majority of users. I was worried about a small minority relying
on the fact that it would error out in such case. But maybe we don't
need to care unless it appears that this minority actually exists.
^ permalink raw reply
* Re: [PATCH 2/2] sequencer: unset GIT_CHERRY_PICK_HELP for 'exec' commands
From: Junio C Hamano @ 2024-02-07 16:39 UTC (permalink / raw)
To: Phillip Wood; +Cc: Vegard Nossum, Kristoffer Haugsbakk, git, Jonathan Nieder
In-Reply-To: <4e6d503a-8564-4536-82a7-29c489f5fec3@gmail.com>
Phillip Wood <phillip.wood123@gmail.com> writes:
> On 06/02/2024 03:54, Junio C Hamano wrote:
>> Vegard Nossum <vegard.nossum@oracle.com> writes:
>>
>>> On 06/02/2024 00:09, Junio C Hamano wrote:
>> Perhaps it is a good idea to squash them together as a single bugfix
>> patch?
>
> I think so, I'm not sure we want to add a new test file just for this
> either. Having the test in a separate file was handy for debugging but
> I think something like the diff below would suffice though I wouldn't
> object to checking the author of the cherry-picked commit
Very true (I didn't even notice that the original "bug report
disguised as a test addition" was inventing a totally new file).
Thanks.
>
> Best Wishes
>
> Phillip
>
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index c5f30554c6..84a92d6da0 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -153,6 +153,18 @@ test_expect_success 'rebase -i with the exec command checks tree cleanness' '
> git rebase --continue
> '
> +test_expect_success 'cherry-pick works with rebase --exec' '
> + test_when_finished "git cherry-pick --abort; \
> + git rebase --abort; \
> + git checkout primary" &&
> + echo "exec git cherry-pick G" >todo &&
> + (
> + set_replace_editor todo &&
> + test_must_fail git rebase -i D D
> + ) &&
> + test_cmp_rev G CHERRY_PICK_HEAD
> +'
> +
> test_expect_success 'rebase -x with empty command fails' '
> test_when_finished "git rebase --abort ||:" &&
> test_must_fail env git rebase -x "" @ 2>actual &&
^ permalink raw reply
* Re: [PATCH] restore: allow --staged on unborn branch
From: Ghanshyam Thakkar @ 2024-02-07 16:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqle7wcl7h.fsf@gitster.g>
On Wed Feb 7, 2024 at 9:36 PM IST, Junio C Hamano wrote:
> Ghanshyam Thakkar <shyamthakkar001@gmail.com> writes:
>
> > Some users expect that being on an unborn branch is similar to having an
> > empty tree checked out. However, when running "git restore --staged ."
> > on unborn branch having staged changes, the follwing error gets printed,
> >
> > fatal: could not resolve HEAD
>
> Sounds like a sensible behaviour---there is no HEAD so there is
> nothing to resolve. With "git resetore --staged ." in such a state,
> what did the user try to do? "git reset" (no other arguments)?
Yeah, I did "git reset" (I am that user btw). But I suppose this is a
case of UX. If a user is using "git restore --staged ." every time they
want to unstage something, then why would they expect it to fail on an
unborn branch when something similar (e.g "git reset") does not?
Also that HEAD, I suppose, is our assumption of the user's intent. And
intent of the user using "git restore --staged ." on unborn branch is
to unstage the changes relative to empty tree. Besides "git reset" already
does this so the matter of assuming empty tree on unborn, I suppose, would
already be settled. Therefore, wouldn't assuming empty tree on unborn
when also using "git restore --staged ." be reasonable and consistent?
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Junio C Hamano @ 2024-02-07 16:46 UTC (permalink / raw)
To: Karthik Nayak; +Cc: Phillip Wood, phillip.wood, git, ps
In-Reply-To: <CAOLa=ZQzz7_L_9cBmK+pgFwd_DFqfWDVRiaZMAxU+54kBq6Pcw@mail.gmail.com>
Karthik Nayak <karthik.188@gmail.com> writes:
> This is a bit of a grey area, what I mean is that we do allow users to create
> non "refs/" prefixed refs:
>
> $ git update-ref foo @~1
>
> $ cat .git/foo
> 2b52187cd2930931c6d34436371f470bb26eef4f
>
> What I mean to say is that, by saying "--include-root-refs" it seems to imply
> that any such refs should be included too, but this simply is not the case.
But isn't that a quality of implementation issue? I'd consider it a
bug once we have and enforce the definition of what pseudorefs are.
^ permalink raw reply
* [PATCH v2 0/5] merge-tree: handle missing objects correctly
From: Johannes Schindelin via GitGitGadget @ 2024-02-07 16:47 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <pull.1651.git.1707212981.gitgitgadget@gmail.com>
I recently looked into issues where git merge-tree calls returned bogus data
(in one instance returning an empty tree for non-empty merge parents). By
the time I had a look at the corresponding repository, the issue was no
longer reproducible, but a closer look at the code combined with some manual
experimenting turned up the fact that missing tree objects aren't handled as
errors by git merge-tree.
While at it, I added a commit on top that tries to catch all remaining
unchecked parse_tree() calls.
This patch series is based on js/merge-tree-3-trees because I introduced
three unchecked parse_tree() calls in that topic branch.
Changes since v1:
* Simplified the test case, avoiding a subshell and a pipe in the process.
* Added a patch to remove a superfluous subtree->object.parsed guard around
a parse_tree(subtree) call.
Johannes Schindelin (5):
merge-tree: fail with a non-zero exit code on missing tree objects
merge-ort: do check `parse_tree()`'s return value
t4301: verify that merge-tree fails on missing blob objects
Always check `parse_tree*()`'s return value
cache-tree: avoid an unnecessary check
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 | 16 +++++++++++-----
merge-recursive.c | 3 ++-
merge.c | 5 ++++-
reset.c | 5 +++++
sequencer.c | 4 ++++
t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++++++++++++++
13 files changed, 84 insertions(+), 15 deletions(-)
base-commit: 5f43cf5b2e4b68386d3774bce880b0f74d801635
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1651%2Fdscho%2Fmerge-tree-and-missing-objects-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1651/dscho/merge-tree-and-missing-objects-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1651
Range-diff vs v1:
1: a3e8ae86114 ! 1: 01dfd66568c merge-tree: fail with a non-zero exit code on missing tree objects
@@ merge-ort.c: static int collect_merge_info(struct merge_options *opt,
## t/t4301-merge-tree-write-tree.sh ##
@@ t/t4301-merge-tree-write-tree.sh: test_expect_success '--merge-base with tree OIDs' '
- git merge-tree --merge-base=side1^^{tree} side1^{tree} side3^{tree} >with-trees &&
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 &&
-+ git rev-parse side3^:
-+ ) | git pack-objects missing-tree.git/objects/pack/side3-tree-is-missing &&
++ 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 &&
+ test_must_be_empty actual
+'
-
++
test_done
2: 3e5b787fc03 = 2: a1bbb7e06e5 merge-ort: do check `parse_tree()`'s return value
3: 85d3e672871 = 3: be1dadf2850 t4301: verify that merge-tree fails on missing blob objects
4: 93abd7000b8 = 4: ffd38ad602a Always check `parse_tree*()`'s return value
-: ----------- > 5: 43c04749513 cache-tree: avoid an unnecessary check
--
gitgitgadget
^ permalink raw reply
* [PATCH v2 1/5] merge-tree: fail with a non-zero exit code on missing tree objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-07 16:47 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.v2.git.1707324461.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When `git merge-tree` encounters a missing tree object, it should error
out and not continue quietly as if nothing had happened.
However, as of time of writing, `git merge-tree` _does_ continue, and
then offers the empty tree as result.
Let's fix this.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
merge-ort.c | 7 ++++---
t/t4301-merge-tree-write-tree.sh | 10 ++++++++++
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/merge-ort.c b/merge-ort.c
index 6491070d965..c37fc035f13 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -1659,9 +1659,10 @@ static int collect_merge_info(struct merge_options *opt,
info.data = opt;
info.show_all_errors = 1;
- 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;
init_tree_desc(t + 0, merge_base->buffer, merge_base->size);
init_tree_desc(t + 1, side1->buffer, side1->size);
init_tree_desc(t + 2, side2->buffer, side2->size);
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 7d0fa74da74..7d588557bdf 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -951,4 +951,14 @@ 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 &&
+ test_must_be_empty actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 2/5] merge-ort: do check `parse_tree()`'s return value
From: Johannes Schindelin via GitGitGadget @ 2024-02-07 16:47 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.v2.git.1707324461.gitgitgadget@gmail.com>
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The previous commit fixed a bug where a missing tree was reported, but
not treated as an error.
This patch addresses the same issue for the remaining two callers of
`parse_tree()`.
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).
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);
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects
From: Johannes Schindelin via GitGitGadget @ 2024-02-07 16:47 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.v2.git.1707324461.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 | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
index 7d588557bdf..9211cb58aa1 100755
--- a/t/t4301-merge-tree-write-tree.sh
+++ b/t/t4301-merge-tree-write-tree.sh
@@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '
test_must_be_empty actual
'
+test_expect_success 'error out on missing blob objects' '
+ seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&
+ seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&
+ seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&
+ tree1=$(printf "100644 blob %s\tsequence" $seq1 | git mktree) &&
+ tree2=$(printf "100644 blob %s\tsequence" $seq2 | git mktree) &&
+ tree3=$(printf "100644 blob %s\tsequence" $seq3 | git mktree) &&
+ git init --bare missing-blob.git &&
+ test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |
+ git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
+ test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&
+ test_must_be_empty actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related
* [PATCH v2 4/5] Always check `parse_tree*()`'s return value
From: Johannes Schindelin via GitGitGadget @ 2024-02-07 16:47 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.v2.git.1707324461.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 v2 5/5] cache-tree: avoid an unnecessary check
From: Johannes Schindelin via GitGitGadget @ 2024-02-07 16:47 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Johannes Schindelin, Johannes Schindelin
In-Reply-To: <pull.1651.v2.git.1707324461.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
* [PATCH 1/2] show-ref --verify: accept pseudorefs
From: Phillip Wood via GitGitGadget @ 2024-02-07 16:44 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1654.git.1707324277.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
"git show-ref --verify" is useful for scripts that want to look up a
fully qualified refname without falling back to the DWIM rules used by
"git rev-parse" rules when the ref does not exist. Currently it will
only accept "HEAD" or a refname beginning with "refs/". Running
git show-ref --verify CHERRY_PICK_HEAD
will always result in
fatal: 'CHERRY_PICK_HEAD' - not a valid ref
even when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
of comparing the refname to "HEAD" we can accept all one-level refs that
contain only uppercase ascii letters and underscores.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
builtin/show-ref.c | 2 +-
t/t1403-show-ref.sh | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index 79955c2856e..1c15421e600 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
while (*refs) {
struct object_id oid;
- if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
+ if ((starts_with(*refs, "refs/") || refname_is_safe(*refs)) &&
!read_ref(*refs, &oid)) {
show_one(show_one_opts, *refs, &oid);
}
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index d0a8f7b121c..33fb7a38fff 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -174,6 +174,14 @@ test_expect_success 'show-ref --verify HEAD' '
test_must_be_empty actual
'
+test_expect_success 'show-ref --verify pseudorefs' '
+ git update-ref CHERRY_PICK_HEAD HEAD $ZERO_OID &&
+ test_when_finished "git update-ref -d CHERRY_PICK_HEAD" &&
+ git show-ref -s --verify HEAD >actual &&
+ git show-ref -s --verify CHERRY_PICK_HEAD >expect &&
+ test_cmp actual expect
+'
+
test_expect_success 'show-ref --verify with dangling ref' '
sha1_file() {
echo "$*" | sed "s#..#.git/objects/&/#"
--
gitgitgadget
^ permalink raw reply related
* [PATCH 2/2] t1400: use show-ref to check pseudorefs
From: Phillip Wood via GitGitGadget @ 2024-02-07 16:44 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Phillip Wood, Phillip Wood
In-Reply-To: <pull.1654.git.1707324277.gitgitgadget@gmail.com>
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Now that "git show-ref --verify" accepts pseudorefs use that in
preference to "git rev-parse" when checking pseudorefs as we do when
checking branches etc.
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
t/t1400-update-ref.sh | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index f18843bf7aa..78a09abc35e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -524,51 +524,51 @@ test_expect_success 'given old value for missing pseudoref, do not create' '
test_expect_success 'create pseudoref' '
git update-ref PSEUDOREF $A &&
- test $A = $(git rev-parse PSEUDOREF)
+ test $A = $(git show-ref -s --verify PSEUDOREF)
'
test_expect_success 'overwrite pseudoref with no old value given' '
git update-ref PSEUDOREF $B &&
- test $B = $(git rev-parse PSEUDOREF)
+ test $B = $(git show-ref -s --verify PSEUDOREF)
'
test_expect_success 'overwrite pseudoref with correct old value' '
git update-ref PSEUDOREF $C $B &&
- test $C = $(git rev-parse PSEUDOREF)
+ test $C = $(git show-ref -s --verify PSEUDOREF)
'
test_expect_success 'do not overwrite pseudoref with wrong old value' '
test_must_fail git update-ref PSEUDOREF $D $E 2>err &&
- test $C = $(git rev-parse PSEUDOREF) &&
+ test $C = $(git show-ref -s --verify PSEUDOREF) &&
test_grep "cannot lock ref.*expected" err
'
test_expect_success 'delete pseudoref' '
git update-ref -d PSEUDOREF &&
- test_must_fail git rev-parse PSEUDOREF
+ test_must_fail git show-ref -s --verify PSEUDOREF
'
test_expect_success 'do not delete pseudoref with wrong old value' '
git update-ref PSEUDOREF $A &&
test_must_fail git update-ref -d PSEUDOREF $B 2>err &&
- test $A = $(git rev-parse PSEUDOREF) &&
+ test $A = $(git show-ref -s --verify PSEUDOREF) &&
test_grep "cannot lock ref.*expected" err
'
test_expect_success 'delete pseudoref with correct old value' '
git update-ref -d PSEUDOREF $A &&
- test_must_fail git rev-parse PSEUDOREF
+ test_must_fail git show-ref -s --verify PSEUDOREF
'
test_expect_success 'create pseudoref with old OID zero' '
git update-ref PSEUDOREF $A $Z &&
- test $A = $(git rev-parse PSEUDOREF)
+ test $A = $(git show-ref -s --verify PSEUDOREF)
'
test_expect_success 'do not overwrite pseudoref with old OID zero' '
test_when_finished git update-ref -d PSEUDOREF &&
test_must_fail git update-ref PSEUDOREF $B $Z 2>err &&
- test $A = $(git rev-parse PSEUDOREF) &&
+ test $A = $(git show-ref -s --verify PSEUDOREF) &&
test_grep "already exists" err
'
--
gitgitgadget
^ permalink raw reply related
* [PATCH 0/2] show-ref --verify: accept psuedorefs
From: Phillip Wood via GitGitGadget @ 2024-02-07 16:44 UTC (permalink / raw)
To: git; +Cc: Patrick Steinhardt, Phillip Wood
Running
git show-ref --verify CHERRY_PICK_HEAD
will always result in
fatal: 'CHERRY_PICK_HEAD' - not a valid ref
even when CHERRY_PICK_HEAD exists. The first patch in this series fixes that
and the second patch then replaces some instances of "git rev-parse " with
"git show-ref --verify " in our test suite.
Phillip Wood (2):
show-ref --verify: accept pseudorefs
t1400: use show-ref to check pseudorefs
builtin/show-ref.c | 2 +-
t/t1400-update-ref.sh | 18 +++++++++---------
t/t1403-show-ref.sh | 8 ++++++++
3 files changed, 18 insertions(+), 10 deletions(-)
base-commit: 2a540e432fe5dff3cfa9d3bf7ca56db2ad12ebb9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1654%2Fphillipwood%2Fshow-ref-pseudorefs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1654/phillipwood/show-ref-pseudorefs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1654
--
gitgitgadget
^ permalink raw reply
* Re: [PATCH v2 5/5] cache-tree: avoid an unnecessary check
From: Junio C Hamano @ 2024-02-07 16:58 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <43c04749513d07733f5fa2c15a694d99d31fe6e3.1707324462.git.gitgitgadget@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> 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.
Makes sense, and it doubly makes sense to keep this separate from
the change done to the same location in [4/5].
Will queue.
^ permalink raw reply
* Re: [PATCH v4 3/3] add -p tests: remove Perl prerequisite
From: Eric Sunshine @ 2024-02-07 16:58 UTC (permalink / raw)
To: phillip.wood; +Cc: Ghanshyam Thakkar, git, gitster, ps
In-Reply-To: <df1fc65f-8716-47bb-b379-1e1f1eeece8b@gmail.com>
On Wed, Feb 7, 2024 at 8:51 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 07/02/2024 10:50, Phillip Wood wrote:
> > On 06/02/2024 22:50, Ghanshyam Thakkar wrote:
> > > The Perl version of the add -i/-p commands has been removed since
> > > 20b813d (add: remove "add.interactive.useBuiltin" & Perl "git
> > > add--interactive", 2023-02-07)
> > >
> > > Therefore, Perl prerequisite in t2071-restore-patch and
> > > t7105-reset-patch is not necessary.
> >
> > Thanks for adding this patch. If you do re-roll I've just noticed that
> > one of the tests in t7106-reset-unborn-branch.sh and another in
> > t2024-checkout-dwim.sh still have PERL prerequisites as well. I don't
> > think it is worth re-rolling just for that as we can clean them up
> > separately if needed.
>
> I didn't cast my net wide enough when I was grepping earlier,
> t7514-commit-patch.sh and t3904-stash-patch.sh also have unnecessary
> PERL prerequisites
Additionally, patch [2/3] drops a PERL prerequisite when it moves an
existing test into a loop, but the removal of the prerequisite is not
mentioned in the commit message. Presumably, the relocation-into-loop
and prerequisite-removal should have been done separately (in patches
[2/3] and [3/3], respectively), and that's how I'd suggest doing it.
^ permalink raw reply
* Re: [PATCH v2 1/5] merge-tree: fail with a non-zero exit code on missing tree objects
From: Eric Sunshine @ 2024-02-07 17:02 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <01dfd66568c1818819e81e001cc189f9066d0cf0.1707324462.git.gitgitgadget@gmail.com>
On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> When `git merge-tree` encounters a missing tree object, it should error
> out and not continue quietly as if nothing had happened.
>
> However, as of time of writing, `git merge-tree` _does_ continue, and
> then offers the empty tree as result.
>
> Let's fix this.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> @@ -951,4 +951,14 @@ test_expect_success '--merge-base with tree OIDs' '
> +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 &&
Isn't the git-rev-parse invocation simply overwriting "list" rather
than appending to it? Did you mean:
git rev-list side3 >list &&
git rev-parse side3^: >>list &&
An alternative would be:
{
git rev-list side3 &&
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 &&
> + test_must_be_empty actual
> +'
^ permalink raw reply
* Re: [PATCH v3 4/4] for-each-ref: avoid filtering on empty pattern
From: Karthik Nayak @ 2024-02-07 17:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Phillip Wood, phillip.wood, git, ps
In-Reply-To: <xmqq1q9ocje3.fsf@gitster.g>
On Wed, Feb 7, 2024 at 5:46 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Karthik Nayak <karthik.188@gmail.com> writes:
>
> > This is a bit of a grey area, what I mean is that we do allow users to create
> > non "refs/" prefixed refs:
> >
> > $ git update-ref foo @~1
> >
> > $ cat .git/foo
> > 2b52187cd2930931c6d34436371f470bb26eef4f
> >
> > What I mean to say is that, by saying "--include-root-refs" it seems to imply
> > that any such refs should be included too, but this simply is not the case.
>
> But isn't that a quality of implementation issue? I'd consider it a
> bug once we have and enforce the definition of what pseudorefs are.
Yeah, that makes sense. I'll use "--include-root-refs" then.
Thanks
^ permalink raw reply
* Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range
From: René Scharfe @ 2024-02-07 17:09 UTC (permalink / raw)
To: Chandra Pratap via GitGitGadget, git
Cc: Kyle Lippincott, Chandra Pratap, Chandra Pratap
In-Reply-To: <pull.1652.v2.git.1707314246530.gitgitgadget@gmail.com>
Am 07.02.24 um 14:57 schrieb Chandra Pratap via GitGitGadget:
> From: Chandra Pratap <chandrapratap3519@gmail.com>
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
> ---
> commit.c: ensure find_header_mem() doesn't scan beyond given range
>
> Thanks for the feedback, Kyle and René! I have update the patch to
> actually solve the problem at hand but I am not very sure about the
> resulting dropping of const-ness of 'eol' from this and how big of a
> problem it might create (if any). I wonder if a custom strchrnul() is
> the best solution to this after all.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1652%2FChand-ra%2Fstrchrnul-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1652/Chand-ra/strchrnul-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1652
>
> Range-diff vs v1:
>
> 1: 1c62f6ee353 ! 1: dcb2de3faea commit.c: ensure strchrnul() doesn't scan beyond range
> @@ Metadata
> Author: Chandra Pratap <chandrapratap3519@gmail.com>
>
> ## Commit message ##
> - commit.c: ensure strchrnul() doesn't scan beyond range
> + commit.c: ensure find_header_mem() doesn't scan beyond given range
>
> Signed-off-by: Chandra Pratap <chandrapratap3519@gmail.com>
>
> @@ commit.c: const char *find_header_mem(const char *msg, size_t len,
> - * at msg beyond the len provided by the caller.
> - */
> while (line && line < msg + len) {
> - const char *eol = strchrnul(line, '\n');
> -+ assert(eol - line <= len);
> +- const char *eol = strchrnul(line, '\n');
> ++ char *eol = (char *) line;
> ++ for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
> ++ eol++;
> ++ }
>
> if (line == eol)
> return NULL;
>
>
> commit.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index ef679a0b939..9a460b2fd6f 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1743,15 +1743,11 @@ const char *find_header_mem(const char *msg, size_t len,
> int key_len = strlen(key);
> const char *line = msg;
>
> - /*
> - * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
> - * given by len. However, current callers are safe because they compute
> - * len by scanning a NUL-terminated block of memory starting at msg.
> - * Nonetheless, it would be better to ensure the function does not look
> - * at msg beyond the len provided by the caller.
> - */
> while (line && line < msg + len) {
> - const char *eol = strchrnul(line, '\n');
> + char *eol = (char *) line;
> + for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
> + eol++;
> + }
This uses the pointer eol only for reading, so you can keep it const.
The loop starts counting from 0 to len for each line, which cannot be
right. find_header_mem("headers\nfoo bar", 9, "foo", &len) would still
return "bar" instead of NULL.
You could initialize i to the offset of line within msg instead (i.e.
i = line - msg). Or check eol < msg + len instead of i < len -- then
you don't even need to introduce that separate counter.
Style nit: We tend to omit curly braces if they contain only a single
statement.
> if (line == eol)
> return NULL;
>
> base-commit: a54a84b333adbecf7bc4483c0e36ed5878cac17b
^ permalink raw reply
* Re: [PATCH 1/2] show-ref --verify: accept pseudorefs
From: Junio C Hamano @ 2024-02-07 17:12 UTC (permalink / raw)
To: Phillip Wood via GitGitGadget; +Cc: git, Patrick Steinhardt, Phillip Wood
In-Reply-To: <4dedc5704c3af6ab4ec8cc7503dc826480775b8e.1707324277.git.gitgitgadget@gmail.com>
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> ... when CHERRY_PICK_HEAD exists. By calling refname_is_safe() instead
> of comparing the refname to "HEAD" we can accept all one-level refs that
> contain only uppercase ascii letters and underscores.
Geez. We have at least three implementations to determine if a ref
is a valid name?
> diff --git a/builtin/show-ref.c b/builtin/show-ref.c
> index 79955c2856e..1c15421e600 100644
> --- a/builtin/show-ref.c
> +++ b/builtin/show-ref.c
> @@ -172,7 +172,7 @@ static int cmd_show_ref__verify(const struct show_one_options *show_one_opts,
> while (*refs) {
> struct object_id oid;
>
> - if ((starts_with(*refs, "refs/") || !strcmp(*refs, "HEAD")) &&
> + if ((starts_with(*refs, "refs/") || refname_is_safe(*refs)) &&
I think the helper you picked is the most sensible one, modulo a few
nits.
- We would want to teach refname_is_safe() to honor is_pseudoref()
from Karthik's series to make rules more consistent.
- The refname_is_safe() helper is not just about the stuff at the
root level. While starts_with("refs/") is overly lenient, it
rejects nonsense like "refs/../trash". We would want to lose
"starts_with() ||" part of the condition from here.
These are minor non-blocking nits that we should keep in mind only
for longer term maintenance, #leftoverbits after the dust settles.
Will queue.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] commit.c: ensure find_header_mem() doesn't scan beyond given range
From: Junio C Hamano @ 2024-02-07 17:23 UTC (permalink / raw)
To: René Scharfe
Cc: Chandra Pratap via GitGitGadget, git, Kyle Lippincott,
Chandra Pratap, Chandra Pratap
In-Reply-To: <e7b269ea-6a10-4f3c-ae97-a58eb7ccc6ef@web.de>
René Scharfe <l.s.r@web.de> writes:
>> - /*
>> - * NEEDSWORK: It's possible for strchrnul() to scan beyond the range
>> - * given by len. However, current callers are safe because they compute
>> - * len by scanning a NUL-terminated block of memory starting at msg.
>> - * Nonetheless, it would be better to ensure the function does not look
>> - * at msg beyond the len provided by the caller.
>> - */
>> while (line && line < msg + len) {
>> - const char *eol = strchrnul(line, '\n');
>> + char *eol = (char *) line;
>> + for (size_t i = 0; i < len && *eol && *eol != '\n'; i++) {
>> + eol++;
>> + }
>
> This uses the pointer eol only for reading, so you can keep it const.
>
> The loop starts counting from 0 to len for each line, which cannot be
> right. find_header_mem("headers\nfoo bar", 9, "foo", &len) would still
> return "bar" instead of NULL.
>
> You could initialize i to the offset of line within msg instead (i.e.
> i = line - msg). Or check eol < msg + len instead of i < len -- then
> you don't even need to introduce that separate counter.
>
> Style nit: We tend to omit curly braces if they contain only a single
> statement.
All true. As we already use an extra variable 'i' for counting, we
can do without eol and reference line[i] instead, which would make
the whole thing something like
while (line && line < msg + len) {
size_t i;
for (i = 0;
i < len && line[i] && line[i] != '\n';
i++)
;
if (key_len < i &&
!strncmp(line, key, ken_len) &&
linhe[key_len] == ' ') {
*out_len = i - key_len - 1;
return line + key_len + 1;
}
line = line[i] ? line + i + 1 : NULL;
}
which is not too bad, simply because the original already needed to
know the length of the current line and due to lack of this "i" you
introduced, it used "eol-line" instead. Now you have "i", the code
may get even simpler by getting rid of "eol".
^ permalink raw reply
* Re: [PATCH v2 3/5] t4301: verify that merge-tree fails on missing blob objects
From: Eric Sunshine @ 2024-02-07 17:24 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <be1dadf28502fe3e9662fa61523e8c57ce3352f1.1707324462.git.gitgitgadget@gmail.com>
On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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>
> ---
> diff --git a/t/t4301-merge-tree-write-tree.sh b/t/t4301-merge-tree-write-tree.sh
> @@ -961,4 +961,18 @@ test_expect_success 'error out on missing tree objects' '
> +test_expect_success 'error out on missing blob objects' '
> + seq1=$(test_seq 1 10 | git hash-object -w --stdin) &&
> + seq2=$(test_seq 1 11 | git hash-object -w --stdin) &&
> + seq3=$(test_seq 0 10 | git hash-object -w --stdin) &&
Is there significance to the ranges passed to test_seq()? Or, can the
same be achieved by using arbitrary content for each blob?
blob1=$(echo "one" | git hash-object -w --stdin) &&
blob2=$(echo "two" | git hash-object -w --stdin) &&
blob3=$(echo "three" | git hash-object -w --stdin) &&
> + tree1=$(printf "100644 blob %s\tsequence" $seq1 | git mktree) &&
> + tree2=$(printf "100644 blob %s\tsequence" $seq2 | git mktree) &&
> + tree3=$(printf "100644 blob %s\tsequence" $seq3 | git mktree) &&
I found the lack of terminating "\n" in the `printf` confusing,
especially since the variable names (seq1, seq2, etc.) and the use of
`printf` seem to imply, at first glance, that each git-mktree
invocation is receiving multiple lines as input which, it turns out,
is not the case. Adding the missing "\n" would help:
tree1=$(printf "100644 blob %s\tsequence\n" $seq1 | git mktree) &&
tree2=$(printf "100644 blob %s\tsequence\n" $seq2 | git mktree) &&
tree3=$(printf "100644 blob %s\tsequence\n" $seq3 | git mktree) &&
Interpolating the $seqN variable directly into the string rather than
using %s would make it even clearer that only a single line is being
generated as input to git-mktree:
tree1=$(printf "100644 blob $seq1\tsequence\n" | git mktree) &&
tree2=$(printf "100644 blob $seq2\tsequence\n" | git mktree) &&
tree3=$(printf "100644 blob $seq3\tsequence\n" | git mktree) &&
Alternatively `echo` could be used, though it's not necessarily any nicer:
tree1=$(echo "100644 blob $seq1Qsequence" | q_to_tab | git mktree) &&
tree2=$(echo "100644 blob $seq2Qsequence" | q_to_tab | git mktree) &&
tree3=$(echo "100644 blob $seq3Qsequence" | q_to_tab | git mktree) &&
> + git init --bare missing-blob.git &&
> + test_write_lines $seq1 $seq3 $tree1 $tree2 $tree3 |
> + git pack-objects missing-blob.git/objects/pack/side1-whatever-is-missing &&
> + test_must_fail git --git-dir=missing-blob.git merge-tree --merge-base=$tree1 $tree2 $tree3 >actual &&
> + test_must_be_empty actual
> +'
^ permalink raw reply
* Re: [PATCH v2 4/5] Always check `parse_tree*()`'s return value
From: Eric Sunshine @ 2024-02-07 17:26 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Patrick Steinhardt, Johannes Schindelin
In-Reply-To: <ffd38ad602a53dfe72cdbfe0d098ca43e7443895.1707324462.git.gitgitgadget@gmail.com>
On Wed, Feb 7, 2024 at 11:48 AM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> Always check `parse_tree*()`'s return value
If you happen to reroll for some reason, perhaps: s/Always/always/
> 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>
^ permalink raw reply
* [RFH] "git -C there add foo" completes, s/add/diff/ does not
From: Junio C Hamano @ 2024-02-07 18:34 UTC (permalink / raw)
To: git
As some of you may already know, I keep an untracked directory
called "Meta" at the top-level of the working tree of the Git
source tree. This "Meta" directory is actually a single-branch
clone of the git.kernel.org/pub/scm/git/git.git that checks out
its "todo" branch, where files like whats-cooking.txt lives.
So, what I often would do is
$ git -C Meta add whats-cooking.txt
after updating the draft of the next issue of the "What's cooking"
report. The command line completion support for "git add" knows how
to complete this when I stopped typing the above after whats-" and
hit <TAB>. It seems that __git_find_repo_path helper function that
notices "-C there" and discovers the $GIT_DIR, and _git_add helper
uses __git_complete_index_file that honors the discovered $GIT_DIR
to find paths in the correct index, which is wonderful.
But the same does not work for the step before I can decide to
actually "add" the contents, which is to "diff", i.e.
$ git -C Meta diff whats-<TAB>
does not complete.
Anybody wants to take a crack at it?
Thanks.
^ permalink raw reply
* [PATCH] tag: fix sign_buffer() call to create a signed tag
From: Junio C Hamano @ 2024-02-07 18:46 UTC (permalink / raw)
To: git; +Cc: Sergey Kosukhin
The command "git tag -s" internally calls sign_buffer() to make a
cryptographic signature using the chosen backend like GPG and SSH.
The internal helper functions used by "git tag" implementation seem
to use a "negative return values are errors, zero or positive return
values are not" convention, and there are places (e.g., verify_tag()
that calls gpg_verify_tag()) that these internal helper functions
translate return values that signal errors to conform to this
convention, but do_sign() that calls sign_buffer() forgets to do so.
Fix it, so that a failed call to sign_buffer() that can return the
exit status from pipe_command() will not be overlooked.
Reported-by: Sergey Kosukhin <skosukhin@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* We alternatively could fix individual sign_buffer() backend that
signals an error with a positive value (sign_buffer_ssh() in this
case) to return a negative value, but this would hopefully be
more future-proof.
builtin/tag.c | 2 +-
gpg-interface.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 3918eacbb5..b28ead06ea 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -176,7 +176,7 @@ static int verify_tag(const char *name, const char *ref UNUSED,
static int do_sign(struct strbuf *buffer)
{
- return sign_buffer(buffer, buffer, get_signing_key());
+ return sign_buffer(buffer, buffer, get_signing_key()) ? -1 : 0;
}
static const char tag_template[] =
diff --git a/gpg-interface.h b/gpg-interface.h
index 143cdc1c02..7cd98161f7 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -66,7 +66,7 @@ size_t parse_signed_buffer(const char *buf, size_t size);
* Create a detached signature for the contents of "buffer" and append
* it after "signature"; "buffer" and "signature" can be the same
* strbuf instance, which would cause the detached signature appended
- * at the end.
+ * at the end. Returns 0 on success, non-zero on failure.
*/
int sign_buffer(struct strbuf *buffer, struct strbuf *signature,
const char *signing_key);
--
2.43.0-561-g235986be82
^ 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