Re: [PATCH v2] sequencer: refuse to pick or revert shallow boundary commits

[Junio C Hamano <gitster@pobox.com>]

"Nakul V S via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Nakul V S <vsnakul@gmail.com>
>
> In a shallow repository, the commits at the edge of the downloaded
> history are recorded in $GIT_DIR/shallow and have their parents hidden
> from the revision machinery.  Such a "shallow boundary" commit looks
> like a root commit that added every file in its tree at once.
>
> Reverting it therefore removes every tracked file, and cherry-picking
> or rebasing it adds all of them again.  In a "git clone --depth=1" this
> is the only commit there is, so "git revert HEAD" empties the working
> tree, which is almost never what the user meant.
>
> Refuse to do so by default for "revert", "cherry-pick" and "rebase".
> For "revert" and "cherry-pick" only, add --allow-shallow-boundary to
> override the check; it is remembered across --continue.  "rebase"
> always refuses, and the error message suggests "git fetch --unshallow"
> instead.
>
> The check is made when the list of commits to replay is built, not when
> each commit is picked, so a command that is going to be refused does so
> before it has done anything: no commit is created, HEAD and the working
> tree are untouched, and no sequencer or rebase state is left behind for
> the user to clean up.
>
> The "apply" backend of "rebase" does not go through the sequencer, so
> it checks the commits "git format-patch" is about to turn into patches
> before it rewinds HEAD.  This is skipped altogether in a repository
> that is not shallow.
>
> A "pick" or "revert" line for a shallow boundary commit that the user
> types into the todo list by hand is not checked, as the check is not
> made again after the todo list has been edited.  "git commit --amend"
> is also left alone for now.
>
> Signed-off-by: Nakul V S <vsnakul@gmail.com>
> ---
>     sequencer: refuse to pick or revert shallow boundary commits by default
>     
>     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.
>      * Rebased onto current master.
>     
>     Not covered here: git commit --amend, and pick/revert lines typed into
>     the rebase todo by hand.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2273%2Fvsnakul%2Fsequencer-shallow-boundary-check-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2273/vsnakul/sequencer-shallow-boundary-check-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/2273
>
> Range-diff vs v1:

For a single-patch series a large range-diff like this before the
patch makes it very distracting.  Isn't there a way to tell
GitGitGadget not to do this (or perhaps add it after the patch)?

> diff --git a/shallow.c b/shallow.c
> index 8e244a5669..bc31ff5232 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -92,6 +92,15 @@ int is_repository_shallow(struct repository *r)
>  	return r->parsed_objects->is_shallow;
>  }
>  
> +int is_shallow_boundary_commit(struct repository *r, const struct object_id *oid)
> +{
> +	struct commit_graft *graft;
> +	if (!is_repository_shallow(r))
> +		return 0;
> +	graft = lookup_commit_graft(r, oid);
> +	return graft && graft->nr_parent < 0;
> +}
> +

Please check how badly your change conflicts with at least 'next'
and 'seen' before sending any non-trivial patch.  Doing so would
have shown you that you do not need to write this function, and
should instead discuss the best course to resolve the conflict with
Harald, who wrote f8bc48ea42 (object-name: explain why <rev>~N fails
in a shallow clone, 2026-09-23).

That other topic is currently stagnant.  You can see how topics are
doing by visiting the mailing list archive.  For example, a trial
merge of your patch into 'seen' would have shown that Harald's
f8bc48ea42 is highly relevant.  Using its Message-ID, you can find
it on the lore archive:

  https://lore.kernel.org/git/pull.2413.v3.git.git.1790189759802.gitgitgadget@gmail.com/

The archive shows that v3 was posted without replying to reviews on
v2, and no new reviewers have shown interest since then, leaving the
topic stuck.

The best path forward is for someone with a stake in getting this
topic unstuck, such as yourself, to provide a fresh review.  This
can help push it to the finish line, or help us realize that it is
not viable so the topic can be dropped.  In the former case, you can
reuse the function introduced by that topic in your patch.  In the
latter case, we no longer need to worry about the two topics
conflicting.

Thanks.