git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[non PATCH */2] preparatory analysis of remaining @{...} issues

From
Junio C Hamano <gitster@pobox.com>
Date
May 8, 2013, 21:32 UTC
Message-ID
<7vwqr9cg4k.fsf@alter.siamese.dyndns.org>
In-Reply-To
<7vk3n9dvlu.fsf@alter.siamese.dyndns.org>

Currently interpret_branch_name() takes either @{-N}, @{u}, or some@{u} and returns an abbreviated refname, or a detached HEAD if @{-N} resolves to such a state. Local branch names coming back for @{-N} are returned as branch names without "refs/heads/", upstream names coming back for @{u} are given after cleaning it up with shorten_unambiguous_ref(). @{-N} resolved to a detached HEAD yields a bare 40-HEX.

This makes the caller unnecessarily fragile.  I started asking
(1) perhaps interpret_branch_name() can be updated to return a full
    refname when it does return a ref;
(2) perhaps it can also be updated to say if its input @{-N} refers
    to a detached HEAD state.

I looked at all existing callers of interpret_branch_name() to see how feasible such a change is. Here is a preparatory analysis.

refs.c: substitute_branch_name()
    The call to interpret_branch_name() by this function only wants to
    see the branch name replaced to a full refname for the consumption
    of dwim_ref() and dwim_log(), and does not want to see a detached
    HEAD state at all.
    Changing @{-N} that returns the bare branch name to return
    refs/heads/$name is welcome.  Changing $name@{u} that returns an
    abbreviation to return a full refname is also welcome.
    And we can error out early if @{-N} referred to a detached HEAD.
    [answer: Yes/Yes]
revision.c: add_pending_object_with_mode()
    The call to interpret_branch_name() by this function wants to turn
    @{-N} and $name@{u} to a refname so that it can be passed to
    add_reflog_for_walk().  Obviously it does not want to see a
    detached state.
    Again, this benefits if interpret_branch_name returned a full
    refname, as add_reflog_for_walk() eventually ends up passing the
    name to dwim_log().
    [answer: Yes/Yes]
sha1_name.c: get_sha1_basic()
    The call to interpret_branch_name() by this function wants to turn
    @{-N} and $name@{u} to a string that can be fed to get_sha1_1() to
    get an object name.  Returning full refname or full object name is
    a good change.
    [answer: Yes/Yes]
sha1_name.c: interpret_branch_name()
    @{-N}@{u} is first given to interpret_nth_prior_checkout() to turn
    @{-N} into a branch name, and then appends @{u} to the result and
    recursively call this function to expand $branch@{u}.  Expansion
    of @{-N} to full refname can break the second expansion, and needs
    to be adjusted, but if @{-N} turns out to be a detaches state, we
    would want to error out the resulting $HEX@{u} anyway, so updating
    the API would result in a pair of real bug fixes.
    [answer: Yes/Yes -- but this caller needs further adjustment]
sha1_name.c: strbuf_branchname()
    The function wants to return an abbreviated branch name.
    The callers of this function are:
    builtin/branch.c: delete_branches()
	"git branch -d -r master@{u}" may expand to "git branch -d -r
	origin/master" and delete "refs/remotes/origin/master", and "git
	branch -d @{-2}" would expand to "git branch -d next" and delete
	"refs/heads/next".
	strbuf_branchname() that returns a full refname would break
	this caller and the caller needs to be updated to shorten
	its output.
        But if strbuf_branchname() can tell @{-N} were detached, we
	can prevent removal of an unintended local branch.
    builtin/checkout.c: setup_branch_path()
        With the current code, "git checkout -b master@{u}" would
        expand to "git checkout -b origin/master" and end up creating
        "refs/heads/origin/master", which may not be a good thing.
	strbuf_branchname() that returns a full refname would break
	this caller and the caller needs to be updated to shorten
	its output.
        If strbuf_branchname() can tell @{-N} were detached, we can
	error out instead of creating a bogus local branch.
    builtin/merge.c: merge_name()
	Uses strbuf_branchname() to turn @{-N} and $name@{u} into
	something that can be fed to dwim_ref().
        This does benefit if strbuf_branchname() can find out the
	full refname.
        If strbuf_branchname() can tell @{-N} were detached, we can
	error out instead of creating a bogus local branch.
    sha1_name.c: strbuf_check_branch_ref()
	Used to see if the name is reasonable when appended to
	refs/heads/.
	strbuf_branchname() that returns a full refname would break
	this caller and the caller needs to be updated to shorten
	its output.
    So strbuf_branchname() itself can tolerate an updated
    interpret_branch_name() that returns full refname and it will
    benefit if it can learn about detached state (many of its
    callers would appreciate the latter).
    [answer: Yes/Yes -- but this caller and its callers need some
    adjustment].
Previous: Junio C HamanoNext: Jeff King
Message 3 of 7 in “sha1_name.c: signal if @{-N} was a true branch nameor a detached head”
  1. 1/2 sha1_name.c: signal if @{-N} was a true branch nameor a detached headJunio C Hamano, May 8, 2013
  2. 2/2 interpret_branch_name(): unconfuse @{-1}@{u} when @{-1} is detachedJunio C Hamano, May 8, 2013
  3. preparatory analysis of remaining @{...} issuesJunio C Hamano, May 8, 2013
  4. Jeff KingMay 9, 2013
  5. Junio C HamanoMay 9, 2013
  6. Jeff KingMay 13, 2013
  7. Eric SunshineMay 9, 2013

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.