{"thread":{"id":"33755","subject":"[PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head","startedAt":"2013-05-08T21:12:29Z","lastAt":"2013-05-13T12:04:58Z","messageCount":7,"participants":["Junio C Hamano","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"216726","messageId":"7vk3n9dvlu.fsf@alter.siamese.dyndns.org","threadId":"33755","inReplyTo":null,"subject":"[PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T21:12:29Z","receivedAt":"2013-05-08T21:12:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The original API read \"checkout: moving from (.*) to ...\" from the\nreflog of the HEAD, and returned the substring between \"from\" and\n\"to\", but there was no way, if the substring was a 40-hex string, to\ntell if we were on a detached HEAD at that commit object, or on a\nbranch whose name happened to be the 40-hex string.\n\nAt this point, we cannot afford to change the format recorded in the\nreflog, so introduce a heuristics to see if the 40-hex matches the\nobject name of the commit we are switching out of.  This will\nunfortunately mishandle this case:\n\n\tHEX=$(git rev-parse master)\n\tgit checkout -b $HEX master\n\tgit checkout master\n\nwhere we were indeed on a non-detached $HEX branch (i.e. HEAD was\npointing at refs/heads/$HEX, not storing $HEX), of course, but\notherwise should be fairly reliable.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This is a preparatory step for the beginning of a much larger\n   series.  Peff is Cc'ed because one of the most tricky issue\n   involves what d46a8301930a (fix parsing of @{-1}@{u} combination,\n   2010-01-28) did.\n\n sha1_name.c | 16 ++++++++++++++--\n 1 file changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3820f28..1473bb6 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -862,6 +862,7 @@ static int get_sha1_oneline(const char *prefix, unsigned char *sha1,\n struct grab_nth_branch_switch_cbdata {\n \tint remaining;\n \tstruct strbuf buf;\n+\tint detached;\n };\n \n static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n@@ -880,9 +881,14 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n \tif (!match || !target)\n \t\treturn 0;\n \tif (--(cb->remaining) == 0) {\n+\t\tunsigned char sha1[20];\n+\n \t\tlen = target - match;\n \t\tstrbuf_reset(&cb->buf);\n \t\tstrbuf_add(&cb->buf, match, len);\n+\t\tcb->detached = (len == 40 &&\n+\t\t\t\t!get_sha1_hex(match, sha1) &&\n+\t\t\t\t!hashcmp(osha1, sha1));\n \t\treturn 1; /* we are done */\n \t}\n \treturn 0;\n@@ -891,8 +897,12 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n /*\n  * Parse @{-N} syntax, return the number of characters parsed\n  * if successful; otherwise signal an error with negative value.\n+ * The string in buf.buf is either a branch name (needs to be\n+ * prefixed with \"refs/heads/\" if the caller wants to make it\n+ * a fully spelled refname) or 40-hex object name of the detached\n+ * HEAD, and *detached is set to true for the latter.\n  */\n-static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf, int *detached)\n {\n \tlong nth;\n \tint retval;\n@@ -917,6 +927,8 @@ static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n \tif (0 < for_each_reflog_ent_reverse(\"HEAD\", grab_nth_branch_switch, &cb)) {\n \t\tstrbuf_reset(buf);\n \t\tstrbuf_add(buf, cb.buf.buf, cb.buf.len);\n+\t\tif (detached)\n+\t\t\t*detached = cb.detached;\n \t\tretval = brace - name + 1;\n \t}\n \n@@ -992,7 +1004,7 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \tchar *cp;\n \tstruct branch *upstream;\n \tint namelen = strlen(name);\n-\tint len = interpret_nth_prior_checkout(name, buf);\n+\tint len = interpret_nth_prior_checkout(name, buf, NULL);\n \tint tmp_len;\n \n \tif (!len)\n-- \n1.8.3-rc1-182-gc61d106\n"},{"id":"216727","messageId":"7vd2t1dvbu.fsf@alter.siamese.dyndns.org","threadId":"33755","inReplyTo":"7vk3n9dvlu.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/2] interpret_branch_name(): unconfuse @{-1}@{u} when @{-1} is detached","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T21:18:29Z","receivedAt":"2013-05-08T21:18:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Now interpret_nth_prior_checkout() can tell the caller if the result\nof expansion of @{-1} is a real branch name or the commit object\nname for a detached HEAD state, let's avoid re-interpreting $HEX@{u}\nin the latter case.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * This prevents us from mistakenly refering to the upstream of an\n   unrelated branch in this sequence:\n\n     HEX=$(git rev-parse --verify HEAD)\n     git branch $HEX     \n     git checkout HEAD^0\n     git checkout -\n     git log @{-1}@{u}\n\n   The branch created in the first step has never been checked out,\n   and @{-1} does not refer to it.  @{-1}@{u} would first turn into \n   $HEX@{u} but we should not look for upstream of refs/heads/$HEX.\n\n sha1_name.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 1473bb6..d3b6897 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1004,15 +1004,21 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \tchar *cp;\n \tstruct branch *upstream;\n \tint namelen = strlen(name);\n-\tint len = interpret_nth_prior_checkout(name, buf, NULL);\n+\tint detached;\n+\tint len = interpret_nth_prior_checkout(name, buf, &detached);\n \tint tmp_len;\n \n \tif (!len)\n \t\treturn len; /* syntax Ok, not enough switches */\n \tif (0 < len && len == namelen)\n \t\treturn len; /* consumed all */\n-\telse if (0 < len) {\n-\t\t/* we have extra data, which might need further processing */\n+\telse if (0 < len && !detached) {\n+\t\t/*\n+\t\t * We have extra data, which might need further\n+\t\t * processing.  E.g. for the original \"@{-1}@{u}\" we\n+\t\t * have converted @{-1} into buf and yet to process\n+\t\t * the remaining @{u} part.\n+\t\t */\n \t\tstruct strbuf tmp = STRBUF_INIT;\n \t\tint used = buf->len;\n \t\tint ret;\n-- \n1.8.3-rc1-182-gc61d106\n"},{"id":"216728","messageId":"7vwqr9cg4k.fsf@alter.siamese.dyndns.org","threadId":"33755","inReplyTo":"7vk3n9dvlu.fsf@alter.siamese.dyndns.org","subject":"[non PATCH */2] preparatory analysis of remaining @{...} issues","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-08T21:32:11Z","receivedAt":"2013-05-08T21:32:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Currently interpret_branch_name() takes either @{-N}, @{u}, or\nsome@{u} and returns an abbreviated refname, or a detached HEAD if\n@{-N} resolves to such a state.  Local branch names coming back for\n@{-N} are returned as branch names without \"refs/heads/\", upstream\nnames coming back for @{u} are given after cleaning it up with\nshorten_unambiguous_ref().  @{-N} resolved to a detached HEAD yields\na bare 40-HEX.\n\nThis makes the caller unnecessarily fragile.  I started asking\n\n(1) perhaps interpret_branch_name() can be updated to return a full\n    refname when it does return a ref;\n\n(2) perhaps it can also be updated to say if its input @{-N} refers\n    to a detached HEAD state.\n\nI looked at all existing callers of interpret_branch_name() to see\nhow feasible such a change is.  Here is a preparatory analysis.\n\n\nrefs.c: substitute_branch_name()\n\n    The call to interpret_branch_name() by this function only wants to\n    see the branch name replaced to a full refname for the consumption\n    of dwim_ref() and dwim_log(), and does not want to see a detached\n    HEAD state at all.\n\n    Changing @{-N} that returns the bare branch name to return\n    refs/heads/$name is welcome.  Changing $name@{u} that returns an\n    abbreviation to return a full refname is also welcome.\n    And we can error out early if @{-N} referred to a detached HEAD.\n\n    [answer: Yes/Yes]\n\n\nrevision.c: add_pending_object_with_mode()\n\n    The call to interpret_branch_name() by this function wants to turn\n    @{-N} and $name@{u} to a refname so that it can be passed to\n    add_reflog_for_walk().  Obviously it does not want to see a\n    detached state.\n\n    Again, this benefits if interpret_branch_name returned a full\n    refname, as add_reflog_for_walk() eventually ends up passing the\n    name to dwim_log().\n\n    [answer: Yes/Yes]\n\n\nsha1_name.c: get_sha1_basic()\n\n    The call to interpret_branch_name() by this function wants to turn\n    @{-N} and $name@{u} to a string that can be fed to get_sha1_1() to\n    get an object name.  Returning full refname or full object name is\n    a good change.\n\n    [answer: Yes/Yes]\n\n\nsha1_name.c: interpret_branch_name()\n\n    @{-N}@{u} is first given to interpret_nth_prior_checkout() to turn\n    @{-N} into a branch name, and then appends @{u} to the result and\n    recursively call this function to expand $branch@{u}.  Expansion\n    of @{-N} to full refname can break the second expansion, and needs\n    to be adjusted, but if @{-N} turns out to be a detaches state, we\n    would want to error out the resulting $HEX@{u} anyway, so updating\n    the API would result in a pair of real bug fixes.\n\n    [answer: Yes/Yes -- but this caller needs further adjustment]\n\n\nsha1_name.c: strbuf_branchname()\n\n    The function wants to return an abbreviated branch name.\n    The callers of this function are:\n\n    builtin/branch.c: delete_branches()\n\n\t\"git branch -d -r master@{u}\" may expand to \"git branch -d -r\n\torigin/master\" and delete \"refs/remotes/origin/master\", and \"git\n\tbranch -d @{-2}\" would expand to \"git branch -d next\" and delete\n\t\"refs/heads/next\".\n\n\tstrbuf_branchname() that returns a full refname would break\n\tthis caller and the caller needs to be updated to shorten\n\tits output.\n\n        But if strbuf_branchname() can tell @{-N} were detached, we\n\tcan prevent removal of an unintended local branch.\n\n    builtin/checkout.c: setup_branch_path()\n\n        With the current code, \"git checkout -b master@{u}\" would\n        expand to \"git checkout -b origin/master\" and end up creating\n        \"refs/heads/origin/master\", which may not be a good thing.\n\n\tstrbuf_branchname() that returns a full refname would break\n\tthis caller and the caller needs to be updated to shorten\n\tits output.\n\n        If strbuf_branchname() can tell @{-N} were detached, we can\n\terror out instead of creating a bogus local branch.\n\n    builtin/merge.c: merge_name()\n\n\tUses strbuf_branchname() to turn @{-N} and $name@{u} into\n\tsomething that can be fed to dwim_ref().\n\n        This does benefit if strbuf_branchname() can find out the\n\tfull refname.\n\n        If strbuf_branchname() can tell @{-N} were detached, we can\n\terror out instead of creating a bogus local branch.\n\n    sha1_name.c: strbuf_check_branch_ref()\n\n\tUsed to see if the name is reasonable when appended to\n\trefs/heads/.\n\n\tstrbuf_branchname() that returns a full refname would break\n\tthis caller and the caller needs to be updated to shorten\n\tits output.\n\n    So strbuf_branchname() itself can tolerate an updated\n    interpret_branch_name() that returns full refname and it will\n    benefit if it can learn about detached state (many of its\n    callers would appreciate the latter).\n\n    [answer: Yes/Yes -- but this caller and its callers need some\n    adjustment].\n"},{"id":"216750","messageId":"20130509064607.GA11985@sigill.intra.peff.net","threadId":"33755","inReplyTo":"7vk3n9dvlu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-09T06:46:07Z","receivedAt":"2013-05-09T06:46:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 08, 2013 at 02:12:29PM -0700, Junio C Hamano wrote:\n\n> The original API read \"checkout: moving from (.*) to ...\" from the\n> reflog of the HEAD, and returned the substring between \"from\" and\n> \"to\", but there was no way, if the substring was a 40-hex string, to\n> tell if we were on a detached HEAD at that commit object, or on a\n> branch whose name happened to be the 40-hex string.\n> \n> At this point, we cannot afford to change the format recorded in the\n> reflog, so introduce a heuristics to see if the 40-hex matches the\n> object name of the commit we are switching out of.  This will\n> unfortunately mishandle this case:\n> \n> \tHEX=$(git rev-parse master)\n> \tgit checkout -b $HEX master\n> \tgit checkout master\n\nI do not think I've ever seen a 40-hex branch name in practice, but I\nwould think a branch named after the commit tip would be a reasonably\ncommon reason to have one, and would trigger this case.\n\nSince the point of marking the detached HEAD is to turn off things like\n\"@{-1}@{u}\", we would want to be generous and err on the side of\nassuming it is a branch if it _might_ be one. IOW, shouldn't we treat\nthe above sequence as a branch, and therefore mishandle:\n\n  git checkout $HEX^0 master\n  git checkout master\n\nby erroneously assuming that we moved to the branch $HEX?\n\n-Peff\n"},{"id":"216783","messageId":"7vhaicaxo7.fsf@alter.siamese.dyndns.org","threadId":"33755","inReplyTo":"20130509064607.GA11985@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-09T17:08:24Z","receivedAt":"2013-05-09T17:08:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Since the point of marking the detached HEAD is to turn off things like\n> \"@{-1}@{u}\", we would want to be generous and err on the side of\n> assuming it is a branch if it _might_ be one.\n\nI am not sure X and Y mesh well in your \"Since X, we would want Y\".\nIt seems to argue for erring on the side of detached HEAD to me.\n\nChecking the \"from\" name $HEX against old_sha1 is a local and cheap\nmeasure I added there for the first level of disambiguation.  If\nthey do not match, we _know_ we didn't come back from a detached\nHEAD state.\n\nIn order to err on the \"favor branch when it could have been one\",\nyou could additionally look for the reflog .git/logs/refs/heads/$HEX\nwhen the \"from\" name $HEX matches old_sha1 (which is likely to be\ndetached, but it is possible that we were on the $HEX branch when\nits tip was at $HEX) and making sure the tip of that $HEX branch\nonce used to be at $HEX at the time recorded for @{-N} in the HEAD\nreflog in question.\n\nThat is far more expensive, though; I doubt it is worth doing.\n"},{"id":"216803","messageId":"CAPig+cS-SyANELP5N9ch2UQgsDaJmfnm0qqPBQ+71ctGDXzzYg@mail.gmail.com","threadId":"33755","inReplyTo":"7vk3n9dvlu.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-09T18:24:42Z","receivedAt":"2013-05-09T18:24:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 8, 2013 at 5:12 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> sha1_name.c: signal if @{-N} was a true branch nameor a detached head\n\ns/nameor/name or/\n\n> The original API read \"checkout: moving from (.*) to ...\" from the\n> reflog of the HEAD, and returned the substring between \"from\" and\n> \"to\", but there was no way, if the substring was a 40-hex string, to\n> tell if we were on a detached HEAD at that commit object, or on a\n> branch whose name happened to be the 40-hex string.\n>\n> At this point, we cannot afford to change the format recorded in the\n> reflog, so introduce a heuristics to see if the 40-hex matches the\n> object name of the commit we are switching out of.  This will\n> unfortunately mishandle this case:\n>\n>         HEX=$(git rev-parse master)\n>         git checkout -b $HEX master\n>         git checkout master\n>\n> where we were indeed on a non-detached $HEX branch (i.e. HEAD was\n> pointing at refs/heads/$HEX, not storing $HEX), of course, but\n> otherwise should be fairly reliable.\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n"},{"id":"217153","messageId":"20130513120456.GA4000@sigill.intra.peff.net","threadId":"33755","inReplyTo":"7vhaicaxo7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sha1_name.c: signal if @{-N} was a true branch nameor a detached head","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-13T12:04:58Z","receivedAt":"2013-05-13T12:04:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 09, 2013 at 10:08:24AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Since the point of marking the detached HEAD is to turn off things like\n> > \"@{-1}@{u}\", we would want to be generous and err on the side of\n> > assuming it is a branch if it _might_ be one.\n> \n> I am not sure X and Y mesh well in your \"Since X, we would want Y\".\n> It seems to argue for erring on the side of detached HEAD to me.\n\nThinking on it more, I don't see that one is actually better than the\nother. If you claim a detached HEAD when there isn't one, the user says\n\"stupid git, that was a branch, and you should tell me its upstream\".\nBut if you claim an undetached HEAD when there isn't one, asking for the\nupstream provides wildly inaccurate results (e.g., \"git checkout\n@{-1}@{u}\" taking you somewhere unexpected).\n\n> Checking the \"from\" name $HEX against old_sha1 is a local and cheap\n> measure I added there for the first level of disambiguation.  If\n> they do not match, we _know_ we didn't come back from a detached\n> HEAD state.\n> \n> In order to err on the \"favor branch when it could have been one\",\n> you could additionally look for the reflog .git/logs/refs/heads/$HEX\n> when the \"from\" name $HEX matches old_sha1 (which is likely to be\n> detached, but it is possible that we were on the $HEX branch when\n> its tip was at $HEX) and making sure the tip of that $HEX branch\n> once used to be at $HEX at the time recorded for @{-N} in the HEAD\n> reflog in question.\n\nI was thinking in terms of @{-1}@{u}, so that you could say \"well, do we\nhave upstream config for such a branch currently?\". Because even though\nwe are digging into history (and it _may_ have been a branch at the\ntime, but isn't now), if we are ultimately going to ask about the\nupstream config (as it is _now_, not when the entry was made), then it\ndoes not matter if the branch was detached or not: we are still going to\nreturn failure either way.\n\nBut there are _other_ uses for @{-1}, and I am probably being too\nfocused on this one use-case.\n\nSo given all of the above, I think I am fine with the direction of the\nseries.\n\n-Peff\n"}]}