Re: [PATCH 2/2] checkout: introduce "--to-branch" option

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: ZheNing Hu via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
	"Christian Couder" <christian.couder@gmail.com>,
	"Hariom Verma" <hariom18599@gmail.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"ZheNing Hu" <adlternative@gmail.com>
Subject: Re: [PATCH 2/2] checkout: introduce "--to-branch" option
Date: Fri, 10 Dec 2021 09:59:43 +0100	[thread overview]
Message-ID: <211210.86y24sdd7k.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <254b352e31029d8151eb6a974fdf8c127340cf79.1639117329.git.gitgitgadget@gmail.com>

On Fri, Dec 10 2021, ZheNing Hu via GitGitGadget wrote:

> From: ZheNing Hu <adlternative@gmail.com>
>
> When we want checkout to a branch (e.g. dev1) which reference
> to a commit, but sometimes we only remember the tag (e.g. v1.1)
> on it, we will use `git checkout v1.1` to find the commit first,
> git will be in the state of deatching HEAD, so we have to search the
> branches on the commit and checkout the branch we perfer. This will
> be a bit cumbersome.
>
> Introduce "--to-branch" option, `git checkout --to-branch <tag>`
> and `git checkout --to-branch <commit>` will search all branches
> and find a unique branch reference to the commit (or the commit which
> the tag reference to) and checkout to it. If the commit have more
> than one branches, it will report error "here are more than one
> branch on commit".
>
> Signed-off-by: ZheNing Hu <adlternative@gmail.com>
> ---
>  Documentation/git-checkout.txt |  8 +++-
>  builtin/checkout.c             | 33 +++++++++++++
>  t/t2018-checkout-branch.sh     | 85 ++++++++++++++++++++++++++++++++++
>  t/t9902-completion.sh          |  1 +
>  4 files changed, 126 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/git-checkout.txt b/Documentation/git-checkout.txt
> index d473c9bf387..2a240699fd9 100644
> --- a/Documentation/git-checkout.txt
> +++ b/Documentation/git-checkout.txt
> @@ -10,7 +10,7 @@ SYNOPSIS
>  [verse]
>  'git checkout' [-q] [-f] [-m] [<branch>]
>  'git checkout' [-q] [-f] [-m] --detach [<branch>]
> -'git checkout' [-q] [-f] [-m] [--detach] <commit>
> +'git checkout' [-q] [-f] [-m] [--detach] [-w|--to-branch] <commit>
>  'git checkout' [-q] [-f] [-m] [[-b|-B|--orphan] <new_branch>] [<start_point>]
>  'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] [--] <pathspec>...
>  'git checkout' [-f|--ours|--theirs|-m|--conflict=<style>] [<tree-ish>] --pathspec-from-file=<file> [--pathspec-file-nul]
> @@ -210,6 +210,12 @@ variable.
>  	`<commit>` is not a branch name.  See the "DETACHED HEAD" section
>  	below for details.
>  
> +-w::
> +--to-branch::
> +	Rather than checking out a commit to work on it, checkout out
> +	to the unique branch on it. If there are multiple branches on
> +	the commit, the checkout will fail.
> +

So basically what this option implements is something that could be done
as a shellscript of:

	git_checkout_branch_from_oid () {
		rev=$1
		git for-each-ref --format='%(refname:strip=2)' --points-at $rev >/tmp/found
		if test $(wc -l </tmp/found) -ne 1
	        then
			echo "Goldilocks error in finding $rev: $(cat /tmp/found)"
	                return 1
		fi
	        git checkout $found
	}

Which is not to say that it isn't useful, but that I think adding this
to "git checkout" specifically is adding this to the wrong level. Isn't
this useful to most things that parse revisions? I.e. wouldn't a better
interface be via the peel syntax?

    oid=$(git rev-parse HEAD)
    git checkout $oid^{tobranch}

Doing it that way would allow any arbitrary command that takes revisions
now access to that, and we could have e.g. "^{tobranches}" too, so you
could do:

    git for-each-ref --format='%(refname:strip=2)' $oid^{tobranches}

Or:

    git log $oid^{tobranches}

I think implementing that is a bit harder. It's peel_onion() in
object-name.c. I think parse_branchname_arg() via get_oid_mb() is now
only capable of filling in an OID for a given name, and then checking
out that name comes as a separate step, and you can't just return
e.g. "master".

But I don't think anything stops us from adjusting those functions a bit
so that get_oid_with_context(() and friends could pass down say an
optional "struct string_list *", and the "peel" could then be expanded
to that.

Similar to how we have "git chekout -", and the "-" is understood by
some commands, but not all (via some opt-in whose location I forget...).