Re: [PATCH v2] sequencer: refuse to pick or revert shallow boundary commits
On 31/10/2026 14:05, Nakul V S via GitGitGadget wrote:
Show 12 quoted lines
>
> Changes since v1:
> > * Also refuse cherry-pick and rebase (merge, interactive and --apply
> backends), as suggested by Phillip.
> * Check when the list of commits is built rather than in
> do_pick_commit(), so a refused command leaves no state behind. Uses a
> single shared helper.
> * --allow-shallow-boundary is now accepted by cherry-pick as well, and
> is saved across --continue.
> * Dropped the option from the revert synopsis.
> * rebase --autostash restores the user's changes if the rebase is
> refused.
> Not covered here: git commit --amend, and pick/revert lines typed into
> the rebase todo by hand.
As Junio said, we can add those later.
Show 13 quoted lines
> diff --git a/Documentation/git-cherry-pick.adoc b/Documentation/git-cherry-pick.adoc
> index f4cd8b9db7..ff6f1bf01d 100644
> --- a/Documentation/git-cherry-pick.adoc
> +++ b/Documentation/git-cherry-pick.adoc
> @@ -178,6 +178,15 @@ fail unless one of `--empty=keep` or `--allow-empty` are specified.
> Pass the merge strategy-specific option through to the
> merge strategy. See linkgit:git-merge[1] for details.
> > +--allow-shallow-boundary::
> + By default, 'git cherry-pick' refuses to pick a commit that is a
> + shallow boundary (i.e. a commit listed in `.git/shallow`).
> + Its parents are missing from this repository, so it looks like
> + a root commit that added every file in its tree, and picking it
> + would add all of them. This option overrides that safeguard.
Unless someone can come up with a realistic use for this option I'd be inclined to drop it.
Show 10 quoted lines
> diff --git a/Documentation/git-rebase.adoc b/Documentation/git-rebase.adoc
> index 6536ebdc90..ed2ce2d46a 100644
> --- a/Documentation/git-rebase.adoc
> +++ b/Documentation/git-rebase.adoc
> @@ -822,6 +822,13 @@ repository that you share.
> See also <<RECOVERING_FROM_UPSTREAM_REBASE,RECOVERING FROM UPSTREAM REBASE>>
> below.
> > +If the commits to be rebased include a shallow boundary commit (a
> +commit listed in `.git/shallow`), the rebase refuses to start, because
> +the missing parents make that commit look like a root commit that added
I think
because it looks like a root commit. Run `git fetch --unshallow`
to fetch the missing history first.
would be sufficient
> +every file in its tree. This applies to both the `merge` and `apply`
> +backends, and nothing is changed when the rebase is refused. Run
> +`git fetch --unshallow` to get the missing history first.
> +
Show 14 quoted lines
> diff --git a/Documentation/git-revert.adoc b/Documentation/git-revert.adoc
> index ffba365e63..288eddcf6e 100644
> --- a/Documentation/git-revert.adoc
> +++ b/Documentation/git-revert.adoc
> @@ -122,6 +122,16 @@ include::rerere-options.adoc[]
> configuration variable can be used to enable this option by
> default.
> > +--allow-shallow-boundary::
> + By default, 'git revert' refuses to revert a commit that is a
> + shallow boundary (i.e. a commit listed in `.git/shallow`).
> + Its parents are missing from this repository, so it looks like
> + a root commit that added every file in its tree, and reverting
> + it would remove all of them. This option overrides that
> + safeguard.
As above, do we really need this option?
Show 17 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 535db60181..2305a3a495 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -34,6 +34,7 @@
> #include "sequencer.h"
> #include "rebase-interactive.h"
> #include "reset.h"
> +#include "shallow.h"
> #include "trace2.h"
> #include "hook.h"
> #include "trailer.h"
> @@ -288,6 +289,8 @@ static int init_basic_state(struct replay_opts *opts, const char *head_name,
> return write_basic_state(opts, head_name, onto, orig_head);
> }
> > +static int cleanup_autostash(struct rebase_options *opts);
> +
Lets move cleaunup_autostash() in a separate, preparatory, commit, rather than littering the code with unnecessary forward declarations.
Show 9 quoted lines
> static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
> {
> int ret = -1;
> @@ -309,6 +312,8 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
> &make_script_args, flags);
> if (ret) {
> error(_("could not generate todo list"));
> + /* Nothing has been started yet; put back what we stashed */
> + cleanup_autostash(opts);Is this related to the other changes in this patch?
Show 19 quoted lines
> goto cleanup;
> }
> > @@ -618,6 +623,55 @@ static int move_to_original_branch(struct rebase_options *opts)
> return ret;
> }
> > +/*
> + * The "apply" backend does not go through the sequencer, so check here that
> + * none of the commits "git format-patch" is about to turn into patches is a
> + * shallow boundary commit. This walks the same set of commits as run_am().
> + */
> +static int check_no_shallow_boundary_to_apply(struct rebase_options *opts)
> +{
> + struct rev_info revs;
> + struct strvec args = STRVEC_INIT;
> + struct commit *commit;
> + int ret = 0;
> +
> + if (!is_repository_shallow(the_repository))
> + return 0;It's good that this returns early in non-shallow repositories, but cherry-pick detection is expensive as we have to calculate the diff of each commit on both sides of the merge-base. Ideally we'd only walk the commits once and feed the commit object ids into
git format-patch --stdin --no-walk=unsorted ...
to generate the patches. The way the code is currently structured makes that slightly tricky as it checks out "onto" before generating the patches. I think we could move the checkout of "onto" into run_am() so that it is separate to the fast-forward case in cmd_rebase(). Then we could generate the patches and check for shallow grafts with a single revision walk before checking out "onto".
Show 18 quoted lines
> diff --git a/sequencer.c b/sequencer.c
> index 508ec71792..8d38ac3878 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> > +/*
> + * A shallow boundary commit has had its parents hidden, so it looks like
> + * a root commit that added every file at once. Picking or reverting it
> + * would not do what the user expects, so refuse before anything is
> + * started.
> + */
> +int sequencer_refuse_shallow_boundary(struct repository *r,
> + const struct commit *commit, int allow)
> +{
> + if (allow || !is_shallow_boundary_commit(r, &commit->object.oid))
> + return 0;
> + return error(_("commit %s is a shallow boundary commit; its parents "
> + "are missing from this clone (use 'git fetch --unshallow' "
> + "to get them)"),I wonder if it would be helpful to add a hint explaining why we refuse to cherry-pick, revert or rebase shallow boundary commits. There is nothing specific to the sequencer here, so if we want to add checks to other commands like "git commit --amend" in the future, maybe this should live in shallow.c? Alternatively we may want to use separate, more specific messages in each case.
Show 24 quoted lines
> + oid_to_hex(&commit->object.oid));
> +}
> +
> static int walk_revs_populate_todo(struct todo_list *todo_list,
> struct replay_opts *opts)
> {
> @@ -3481,13 +3502,18 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
> encoding = get_log_output_encoding();
> > while ((commit = get_revision(opts->revs))) {
> - struct todo_item *item = append_new_todo(todo_list);
> - const char *commit_buffer = repo_logmsg_reencode(the_repository,
> - commit, NULL,
> - encoding);
> + struct todo_item *item;
> + const char *commit_buffer;
> const char *subject;
> int subject_len;
> > + if (sequencer_refuse_shallow_boundary(the_repository, commit,
> + opts->allow_shallow_boundary))
> + return -1;
> +
> + item = append_new_todo(todo_list);
> + commit_buffer = repo_logmsg_reencode(the_repository, commit,
> + NULL, encoding);This covers cherry-picking or reverting multiple commits
Show 5 quoted lines
> @@ -5692,6 +5721,9 @@ static int single_pick(struct repository *r,
> int check_todo;
> struct todo_item item;
> > + if (sequencer_refuse_shallow_boundary(r, cmit, opts->allow_shallow_boundary))
> + return -1;
This covers cherry-picking or reverting a single commit.
Show 19 quoted lines
> item.command = opts->action == REPLAY_PICK ?
> TODO_PICK : TODO_REVERT;
> item.commit = cmit;
> @@ -6078,6 +6110,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> int rebase_cousins = flags & TODO_LIST_REBASE_COUSINS;
> int root_with_onto = flags & TODO_LIST_ROOT_WITH_ONTO;
> int skipped_commit = 0;
> + int ret = 0;
> struct strbuf buf = STRBUF_INIT, oneline = STRBUF_INIT;
> struct strbuf label_from_message = STRBUF_INIT;
> struct commit_list *commits = NULL, **tail = &commits, *iter;
> @@ -6141,6 +6174,10 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> }
> if (is_empty && !keep_empty)
> continue;
> + if (sequencer_refuse_shallow_boundary(the_repository, commit, 0)) {
> + ret = -1;
> + goto cleanup;
> + }This covers "git rebase -r", and on error we cleanup properly - good
Show 8 quoted lines
> @@ -6394,6 +6432,10 @@ int sequencer_make_script(struct repository *r, struct strbuf *out,
> }
> if (is_empty && !keep_empty)
> continue;
> + if (sequencer_refuse_shallow_boundary(r, commit, 0)) {
> + ret = -1;
> + goto cleanup;
> + }This covers a regular rebase.
Show 8 quoted lines
> diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
> index 939e7a16a6..49da8427cf 100755
> --- a/t/t3501-revert-cherry-pick.sh
> +++ b/t/t3501-revert-cherry-pick.sh
> @@ -256,4 +256,212 @@ test_expect_success 'cherry-pick is unaware of --reference (for now)' '
> test_grep "^usage: git cherry-pick" actual
> '
> > +# Tests for shallow boundary safeguard
There are an awful lot of new tests here, many more than there are places where we check for a shallow boundary. It would be a good idea to check whether each test actually adds any new coverage. Also the rebase tests should live in one of the rebase test files (maybe t3400?)
Thanks for re-rolling, this is definitely moving in the right direction.
Thanks
Phillip
Show 208 quoted lines
> +test_expect_success 'setup shallow boundary tests' '
> + git init shallow-src &&
> + test_commit -C shallow-src A &&
> + test_commit -C shallow-src B &&
> + test_commit -C shallow-src C &&
> + git clone --no-local --depth=2 "file://$(pwd)/shallow-src" shallow-repo &&
> + git -C shallow-repo rev-parse --verify B >boundary &&
> + test_cmp shallow-repo/.git/shallow boundary
> +'
> +
> +test_expect_success 'revert of shallow boundary commit is refused' '
> + git -C shallow-repo reset --hard C &&
> + test_must_fail git -C shallow-repo revert --no-edit B 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/sequencer &&
> + test_path_is_missing shallow-repo/.git/REVERT_HEAD &&
> + git -C shallow-repo rev-parse C >expect &&
> + git -C shallow-repo rev-parse HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'cherry-pick of shallow boundary commit is refused' '
> + git -C shallow-repo checkout -q --detach C &&
> + git -C shallow-repo rm -rq . &&
> + git -C shallow-repo commit -q -m empty &&
> + git -C shallow-repo rev-parse HEAD >expect &&
> + test_must_fail git -C shallow-repo cherry-pick B 2>err &&
> + test_grep "shallow boundary commit" err &&
> + git -C shallow-repo rev-parse HEAD >actual &&
> + test_cmp expect actual &&
> + git -C shallow-repo reset -q --hard C
> +'
> +
> +test_expect_success 'revert of non-boundary commit in shallow repo works' '
> + git -C shallow-repo revert --no-edit C &&
> + git -C shallow-repo reset -q --hard C
> +'
> +
> +test_expect_success 'multi-commit revert is refused before anything is reverted' '
> + test_must_fail git -C shallow-repo revert --no-edit C B 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/sequencer &&
> + git -C shallow-repo rev-parse C >expect &&
> + git -C shallow-repo rev-parse HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success '--allow-shallow-boundary overrides the revert check' '
> + git -C shallow-repo revert --no-edit --allow-shallow-boundary B &&
> + test_path_is_missing shallow-repo/B.t &&
> + git -C shallow-repo reset -q --hard C
> +'
> +
> +test_expect_success '--allow-shallow-boundary overrides the cherry-pick check' '
> + git -C shallow-repo checkout -q --detach C &&
> + git -C shallow-repo rm -rq . &&
> + git -C shallow-repo commit -q -m empty &&
> + git -C shallow-repo cherry-pick --allow-shallow-boundary B &&
> + test_path_is_file shallow-repo/B.t &&
> + git -C shallow-repo reset -q --hard C
> +'
> +
> +test_expect_success '--allow-shallow-boundary survives --continue' '
> + test_commit -C shallow-repo conflict-1 file one &&
> + test_commit -C shallow-repo conflict-2 file two &&
> + test_must_fail git -C shallow-repo revert --no-edit \
> + --allow-shallow-boundary conflict-1 B &&
> + echo resolved >shallow-repo/file &&
> + git -C shallow-repo add file &&
> + git -C shallow-repo revert --continue &&
> + test_path_is_missing shallow-repo/B.t &&
> + git -C shallow-repo reset -q --hard C
> +'
> +
> +test_expect_success 'revert and cherry-pick in a non-shallow repo are unaffected' '
> + git init full-repo &&
> + test_commit -C full-repo A &&
> + test_commit -C full-repo B &&
> + git -C full-repo revert --no-edit B &&
> + git -C full-repo cherry-pick B &&
> + test_path_is_file full-repo/B.t
> +'
> +
> +test_expect_success 'rebase of a range containing a shallow boundary is refused' '
> + git -C shallow-repo checkout -q --detach C &&
> + git -C shallow-repo rev-parse C >expect &&
> + test_must_fail git -C shallow-repo rebase --root 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/rebase-merge &&
> + git -C shallow-repo rev-parse HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'rebase --rebase-merges is refused as well' '
> + test_must_fail git -C shallow-repo rebase --rebase-merges --root 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/rebase-merge
> +'
> +
> +test_expect_success 'rebase --apply of a range containing a shallow boundary is refused' '
> + git -C shallow-repo checkout -q --detach C &&
> + git -C shallow-repo checkout -q --orphan unrelated &&
> + git -C shallow-repo rm -rfq . &&
> + test_commit -C shallow-repo unrelated &&
> + git -C shallow-repo checkout -q --detach C &&
> + git -C shallow-repo rev-parse C >expect &&
> + test_must_fail git -C shallow-repo rebase --apply \
> + --onto unrelated unrelated 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/rebase-apply &&
> + git -C shallow-repo rev-parse HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'rebase --apply restores an autostash when refusing' '
> + echo dirty >shallow-repo/C.t &&
> + test_must_fail git -C shallow-repo rebase --apply --autostash \
> + --onto unrelated unrelated 2>err &&
> + test_grep "shallow boundary commit" err &&
> + echo dirty >expect &&
> + test_cmp expect shallow-repo/C.t &&
> + git -C shallow-repo stash list >stashes &&
> + test_must_be_empty stashes &&
> + git -C shallow-repo checkout -q -- C.t
> +'
> +
> +test_expect_success 'rebase --apply that does not replay the boundary is unaffected' '
> + git -C shallow-repo checkout -q -B apply-topic C &&
> + test_commit -C shallow-repo E &&
> + git -C shallow-repo rebase --apply --onto unrelated C &&
> + test_path_is_file shallow-repo/E.t
> +'
> +
> +test_expect_success 'rebase that does not replay the boundary is unaffected' '
> + git -C shallow-repo checkout -q -B topic C &&
> + test_commit -C shallow-repo D &&
> + git -C shallow-repo rebase C
> +'
> +
> +test_expect_success 'rebase restores an autostash when refusing (merge backend)' '
> + git -C shallow-repo checkout -q --detach C &&
> + echo dirty >shallow-repo/C.t &&
> + test_must_fail git -C shallow-repo rebase --autostash --root 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/rebase-merge &&
> + echo dirty >expect &&
> + test_cmp expect shallow-repo/C.t &&
> + git -C shallow-repo stash list >stashes &&
> + test_must_be_empty stashes &&
> + git -C shallow-repo checkout -q -- C.t
> +'
> +
> +test_expect_success 'interactive rebase restores an autostash when refusing' '
> + echo dirty >shallow-repo/C.t &&
> + test_must_fail env GIT_SEQUENCE_EDITOR=: \
> + git -C shallow-repo rebase -i --autostash --root 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_missing shallow-repo/.git/rebase-merge &&
> + echo dirty >expect &&
> + test_cmp expect shallow-repo/C.t &&
> + git -C shallow-repo stash list >stashes &&
> + test_must_be_empty stashes &&
> + git -C shallow-repo checkout -q -- C.t
> +'
> +
> +test_expect_success 'rebase onto new upstream work in a shallow clone still works' '
> + git clone --no-local --depth=1 "file://$(pwd)/shallow-src" shallow-local &&
> + test_commit -C shallow-local local &&
> + test_commit -C shallow-src upstream &&
> + git -C shallow-local fetch origin &&
> + git -C shallow-local rebase origin/HEAD &&
> + test_path_is_file shallow-local/upstream.t &&
> + test_path_is_file shallow-local/local.t
> +'
> +
> +test_expect_success 'single-commit shallow clone: revert and cherry-pick are refused' '
> + git init one-src &&
> + test_commit -C one-src only &&
> + git clone --no-local --depth=1 "file://$(pwd)/one-src" one-shallow &&
> + git -C one-shallow rev-parse HEAD >expect &&
> + test_must_fail git -C one-shallow revert --no-edit HEAD 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_must_fail git -C one-shallow cherry-pick -n HEAD 2>err &&
> + test_grep "shallow boundary commit" err &&
> + test_path_is_file one-shallow/only.t &&
> + git -C one-shallow rev-parse HEAD >actual &&
> + test_cmp expect actual
> +'
> +
> +test_expect_success 'revert and cherry-pick work once the clone is unshallowed' '
> + git clone --no-local --depth=2 "file://$(pwd)/shallow-src" unshallowed &&
> + test_must_fail git -C unshallowed revert --no-edit HEAD~1 2>err &&
> + test_grep "shallow boundary commit" err &&
> + git -C unshallowed fetch --unshallow &&
> + git -C unshallowed revert --no-edit C &&
> + test_path_is_missing unshallowed/C.t &&
> + git -C unshallowed cherry-pick C &&
> + test_path_is_file unshallowed/C.t
> +'
> +
> +test_expect_success '--allow-shallow-boundary cannot be combined with --abort' '
> + test_must_fail git -C shallow-repo revert --abort \
> + --allow-shallow-boundary 2>err &&
> + test_grep "cannot be used with" err
> +'
> +
> test_done
> > base-commit: 50fe5cc63f9cd6e89b87a2c9ccf96a444f4cfdb7