{"thread":{"id":"22172","subject":"default behaviour for `gitmerge` (no arguments)","startedAt":"2010-01-11T18:49:38Z","lastAt":"2010-01-29T11:22:19Z","messageCount":30,"participants":["Gareth Adams","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"131303","messageId":"loom.20100111T185144-655@post.gmane.org","threadId":"22172","inReplyTo":null,"subject":"default behaviour for `gitmerge` (no arguments)","fromName":"Gareth Adams","fromEmail":"gareth.adams@gmail.com","sentAt":"2010-01-11T18:49:38Z","receivedAt":"2010-01-11T18:49:38Z","isPatch":false,"sender":{"key":"gareth.adams@gmail.com","avatar":null},"body":"Hi there, long time user; first time caller here.\n\nI wanted to suggest an improvement to git-merge which will either save some\ntyping or save some network resources. It won't save huge amounts of either but\nevery little helps!\n\nCurrently, some of my colleagues frequently end up typing:\n\n git pull; ...; git checkout otherbranch; git pull\n\nNow, we have quite a low commit rate, it's unlikely (albeit vaguely possible)\nthat two people are working on the branch at the same time. This means the\nsecond pull is doing a fetch which it effectively pointless.\n\nNow of course this is a tiny amount of wastage, and while I could argue that it\nwould be an issue under poor network conditions that's not my point. As a coder\nI'd want to get rid of the redundant fetch if I know it's redundant.\n\nUnfortunately my other option is:\n\n git pull; ...; git checkout otherbranch; git merge myremote/otherbranch\n\nwhich is annoying extra typing. Even with tab completion, it's redundant extra\ntyping because in these cases I'm trying to merge with the branch being tracked.\n\nMy suggestion is that `git merge` defaults to the same merge that a `git pull`\nwould perform, and there are 2 extra factors that make me think it's a workable\nidea:\n\n1) At the moment, `git merge` does nothing. Except mock me for not giving it a\ncommand in a format it recognises. This change wouldn't have any effect that\nwould cause anyone a problem\n\n2) When I checkout a branch which has unmerged changes in the tracking branch,\ngit *tells me* what branch I will be taking action with \"Your branch is behind\nthe tracked remote branch '...' by 4 commits, and can be fast-forwarded\" - but\nthen makes me type it out explicitly anyway!\n\nI appreciate that there are many workflows where there is an advantage in\nperforming a second pull in case there are additional changes since the first\npull, but I still think there is a string case for git merge having a more\nsensible default, as git pull does.\n\nWhat do you think?\n\nThanks,\nGareth\n"},{"id":"131310","messageId":"7v7hrojukz.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"loom.20100111T185144-655@post.gmane.org","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-11T19:43:40Z","receivedAt":"2010-01-11T19:43:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Gareth Adams writes:\n\n> Unfortunately my other option is:\n>\n> git pull; ...; git checkout otherbranch; git merge myremote/otherbranch\n>\n> which is annoying extra typing.\n\nReplace 'pull' with 'fetch' and a bit more regular pattern would emerge.\n\nThe code indeed knows (as you can see \"git pull\" can figure it out) what\nother ref the current branch is configured to merge with by default.\nThere is even a plumbing to do this for script writers.\n\n $ git for-each-ref --format='%(upstream)' $(git symbolic-ref HEAD)\n\nWe can teach this short-hand to \"git merge\", perhaps:\n\n $ git merge --default\n\nBut \"no argument\" cannot be the short-hand, because...\n\n> 1) At the moment, `git merge` does nothing. Except mock me for not giving it a\n> command in a format it recognises. This change wouldn't have any effect that\n> would cause anyone a problem\n\n... except for people who uses a script that does\n\n commits=\n while some condition\n do\n \tcommit=$(find some other commit that should be merged)\n commits=\"$commits$commit \"\n done\n git merge $commits\n\nand expect the last step will fail without doing any damage when the loop\nfinds no new developments. \"no argument means --default\" will break their\nscripts.\n"},{"id":"131387","messageId":"20100112162355.GB25092@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7v7hrojukz.fsf@alter.siamese.dyndns.org","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-12T16:23:55Z","receivedAt":"2010-01-12T16:23:55Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 11, 2010 at 11:43:40AM -0800, Junio C Hamano wrote:\n\n> The code indeed knows (as you can see \"git pull\" can figure it out) what\n> other ref the current branch is configured to merge with by default.\n> There is even a plumbing to do this for script writers.\n> \n> $ git for-each-ref --format='%(upstream)' $(git symbolic-ref HEAD)\n> \n> We can teach this short-hand to \"git merge\", perhaps:\n> \n> $ git merge --default\n> \n> But \"no argument\" cannot be the short-hand, because...\n\nHmm. If we had the oft-discussed-but-never-agreed-upon shorthand for\n\"the upstream of\" then we wouldn't need a special merge option. You\ncould just do:\n\n git merge %HEAD ;# (or git merge %, IIRC the proposal correctly)\n\n-Peff\n"},{"id":"131394","messageId":"7vhbqr2nxt.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100112162355.GB25092@coredump.intra.peff.net","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-12T18:11:26Z","receivedAt":"2010-01-12T18:11:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hmm. If we had the oft-discussed-but-never-agreed-upon shorthand for\n> \"the upstream of\" then we wouldn't need a special merge option. You\n> could just do:\n>\n> git merge %HEAD ;# (or git merge %, IIRC the proposal correctly)\n\nI don't think \"whatever _HEAD_ tracks\" makes sense at the semantic level\n(i.e. you don't do \"branch.HEAD.merge\") but a syntax for \"whatever the\nnamed _branch_ tracks\" with \"if a branch is not named, the current branch\nis implied\" (i.e. the one in parentheses) would.\n\nIt is an entirely different matter what the special syntax to trigger that\n\"upstream-ness\" should be. I vaguely recall @{upstream} or @{u} were the\nconcensus?\n"},{"id":"131396","messageId":"20100112182550.GA15696@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7vhbqr2nxt.fsf@alter.siamese.dyndns.org","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-12T18:25:50Z","receivedAt":"2010-01-12T18:25:50Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 12, 2010 at 10:11:26AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Hmm. If we had the oft-discussed-but-never-agreed-upon shorthand for\n> > \"the upstream of\" then we wouldn't need a special merge option. You\n> > could just do:\n> >\n> > git merge %HEAD ;# (or git merge %, IIRC the proposal correctly)\n> \n> I don't think \"whatever _HEAD_ tracks\" makes sense at the semantic level\n> (i.e. you don't do \"branch.HEAD.merge\") but a syntax for \"whatever the\n> named _branch_ tracks\" with \"if a branch is not named, the current branch\n> is implied\" (i.e. the one in parentheses) would.\n\nThe patch that Dscho provided would actually convert HEAD@{upstream}\ninto the upstream of whatever HEAD pointed at. Which I think makes\nsense. We don't do it for reflogs, but that is because it is useful to\ndistinguish between the reflog for a symref and the thing it points to.\nBut since one would presumably not make such a configuration for a\nsymref, that distinction is not useful.\n\n> It is an entirely different matter what the special syntax to trigger that\n> \"upstream-ness\" should be. I vaguely recall @{upstream} or @{u} were the\n> concensus?\n\nAh, right. I remembered hating \"%\" even as I typed it, but I had\nforgotten about the followup discussion. Looking at it again, I note:\n\n 1. The last posted patch still has a misplaced free() (patch below),\n but I think otherwise is not buggy.\n\n 2. We don't complain on \"git show @{usptream}\" and we probably should.\n I remember there being some complications because the contents of\n @{} were passed to approxidate, but I think we can get around that\n by letting approxidate complain if _nothing_ in the date was\n useful. So \"git show @{2.weeks.and.7.hot.dogs.ago}\" would still\n work, but \"git show @{totally.bogus.input}\" would complain.\n\n 3. I have actually been running with Dscho's patch for the last couple\n of months, and I don't remember using it once. So perhaps it is not\n as useful as I might have thought. :)\n\nAnyway, fixup patch is below. I don't expect you to pick up the topic or\nanything, but since I went to the trouble to find the bug once upon a\ntime, I thought I would post the fix for anybody who does want to pick\nit up.\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex b73b93e..da90ebe 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -275,9 +275,9 @@ static char *substitute_branch_name(const char **string, int *len)\n \t\tchar *ref = xstrndup(*string, *len - ret);\n \t\tstruct branch *tracking = branch_get(*ref ? ref : NULL);\n \n-\t\tfree(ref);\n \t\tif (!tracking)\n \t\t\tdie (\"No tracking branch found for '%s'\", ref);\n+\t\tfree(ref);\n \t\tif (tracking->merge && tracking->merge[0]->dst) {\n \t\t\t*string = xstrdup(tracking->merge[0]->dst);\n \t\t\t*len = strlen(*string);\n"},{"id":"131436","messageId":"7vwrzmqypn.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100112182550.GA15696@coredump.intra.peff.net","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T06:57:08Z","receivedAt":"2010-01-13T06:57:08Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Ah, right. I remembered hating \"%\" even as I typed it, but I had\n> forgotten about the followup discussion. Looking at it again, I note:\n>\n> 1. The last posted patch still has a misplaced free() (patch below),\n> but I think otherwise is not buggy.\n>\n> 2. We don't complain on \"git show @{usptream}\" and we probably should.\n> I remember there being some complications because the contents of\n> @{} were passed to approxidate, but I think we can get around that\n> by letting approxidate complain if _nothing_ in the date was\n> useful. So \"git show @{2.weeks.and.7.hot.dogs.ago}\" would still\n> work, but \"git show @{totally.bogus.input}\" would complain.\n>\n> 3. I have actually been running with Dscho's patch for the last couple\n> of months, and I don't remember using it once. So perhaps it is not\n> as useful as I might have thought. :)\n\nI presume we are discussing this patch?\n\n http://article.gmane.org/gmane.comp.version-control.git/128121\n\nI'll squash the free() fix; thanks.\n\nI wondered why it doesn't hook into interpret_branch_name(), and instead\nadds itself to the static substitute_branch_name(); it forbids the use of\nthe syntax from by callers of strbuf_branchname().\n\nI agree with your point #2 above.\n\nRegarding your point #3, I don't think the notation should be that useful\nif your workflow is sane. The original use case that triggered the\nresurrection of the patch went like this:\n\n git fetch &&\n for local in my set of local branches\n do\n git checkout $local &&\n git merge $local@{upstream} || {\n echo failed to merge on $local\n break\n }\n done\n\nand the new notation might look useful in the scenario. But the thing is,\nconstantly merging with the other side, even if you haven't added anything\nof value since you merged from there last time, is a bad practice to begin\nwith. I added one use case that is sane _and_ will be helped by the new\nnotation to the rewritten version of Dscho's patch (below).\n\nJust to refresh our memory from the old thread and make sure we are\ndiscussing the same patch, here is what I am planning to queue. The log\nmessage and documentation are somewhat updated to avoid the word \"track\"\nbecause it seems that everybody gets confused and starts talking different\nthings whenever that word is used. For the same reason, the test script\nwas renamed.\n\nIn this set-up, for example:\n\n [remote \"filfre\"]\n url = ...\n fetch = +refs/heads/nitfol:refs/heads/rezrov\n [branch \"frotz\"]\n remote = filfre\n merge = refs/heads/nitfol\n\nsome people say rezrov tracks nitfol from filfre but Dscho's patch says\nfrotz tracks nitfol from filfre. They _may_ both track, but they \"track\"\nthe other in a quite differently way, so the word has become meaningless.\n\nI've been trying to be careful and used different words to disambiguate\nwhenever I had to talk about these concepts:\n\n - The purpose of rezrov is to keep a tab on the progress of the nitfol\n branch at the remote end. We say rezrov is a remote tracking branch\n for nitfol from filfre.\n\n - On the other hand, we have branch frotz that forked from nitfol that\n came from filfre. It builds on top of that history by occasionally\n merging with it at key points in the history. So we say frotz builds\n on top of nitfol from filfre. We also say nitfol at filfre is the\n upstream of frotz.\n\n-- >8 --\nDate: Thu, 10 Sep 2009 17:25:57 +0200\nSubject: [PATCH] Introduce @{upstream} notation\n\nA new notation '@{upstream}' refers to the branch is set\nto build on top of. Missing (i.e. '@{upstream}') defaults to the\ncurrent branch.\n\nThis allows you to run, for example,\n\n\tfor l in list of local branches\n\tdo\n\t\tgit log --oneline --left-right $l...$l@{upstream}\n\tdone\n\nto inspect each of the local branches you are interested in for the\ndivergence from its upstream.\n\nSigned-off-by: Johannes Schindelin \nSigned-off-by: Junio C Hamano \n---\n Documentation/git-rev-parse.txt | 4 ++\n sha1_name.c | 39 ++++++++++++++++++++--\n t/t1506-rev-parse-upstream.sh | 69 +++++++++++++++++++++++++++++++++++++++\n 3 files changed, 109 insertions(+), 3 deletions(-)\n create mode 100755 t/t1506-rev-parse-upstream.sh\n\ndiff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt\nindex 82045a2..923b56a 100644\n--- a/Documentation/git-rev-parse.txt\n+++ b/Documentation/git-rev-parse.txt\n@@ -231,6 +231,10 @@ when you run 'git-merge'.\n * The special construct '@\\{-\\}' means the th branch checked out\n before the current one.\n \n+* The suffix '@{upstream}' to a ref (short form 'ref@{u}') refers to\n+ the branch the ref is set to build on top of. Missing ref defaults\n+ to the current branch.\n+\n * A suffix '{caret}' to a revision parameter means the first parent of\n that commit object. '{caret}' means the th parent (i.e.\n 'rev{caret}'\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 44bb62d..fb4e214 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -5,6 +5,7 @@\n #include \"blob.h\"\n #include \"tree-walk.h\"\n #include \"refs.h\"\n+#include \"remote.h\"\n \n static int find_short_object_filename(int len, const char *name, unsigned char *sha1)\n {\n@@ -238,9 +239,24 @@ static int ambiguous_path(const char *path, int len)\n \treturn slash;\n }\n \n+static inline int tracked_suffix(const char *string, int len)\n+{\n+\tconst char *suffix[] = { \"@{upstream}\", \"@{u}\" };\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(suffix); i++) {\n+\t\tint suffix_len = strlen(suffix[i]);\n+\t\tif (len >= suffix_len && !memcmp(string + len - suffix_len,\n+\t\t\t\t\tsuffix[i], suffix_len))\n+\t\t\treturn suffix_len;\n+\t}\n+\treturn 0;\n+}\n+\n /*\n * *string and *len will only be substituted, and *string returned (for\n- * later free()ing) if the string passed in is of the form @{-}.\n+ * later free()ing) if the string passed in is of the form @{-} or\n+ * of the form @{upstream}.\n */\n static char *substitute_branch_name(const char **string, int *len)\n {\n@@ -254,6 +270,21 @@ static char *substitute_branch_name(const char **string, int *len)\n \t\treturn (char *)*string;\n \t}\n \n+\tret = tracked_suffix(*string, *len);\n+\tif (ret) {\n+\t\tchar *ref = xstrndup(*string, *len - ret);\n+\t\tstruct branch *tracking = branch_get(*ref ? ref : NULL);\n+\n+\t\tif (!tracking)\n+\t\t\tdie (\"No tracking branch found for '%s'\", ref);\n+\t\tfree(ref);\n+\t\tif (tracking->merge && tracking->merge[0]->dst) {\n+\t\t\t*string = xstrdup(tracking->merge[0]->dst);\n+\t\t\t*len = strlen(*string);\n+\t\t\treturn (char *)*string;\n+\t\t}\n+\t}\n+\n \treturn NULL;\n }\n \n@@ -340,8 +371,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tif (len && str[len-1] == '}') {\n \t\tfor (at = len-2; at >= 0; at--) {\n \t\t\tif (str[at] == '@' && str[at+1] == '{') {\n-\t\t\t\treflog_len = (len-1) - (at+2);\n-\t\t\t\tlen = at;\n+\t\t\t\tif (!tracked_suffix(str + at, len - at)) {\n+\t\t\t\t\treflog_len = (len-1) - (at+2);\n+\t\t\t\t\tlen = at;\n+\t\t\t\t}\n \t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/t/t1506-rev-parse-upstream.sh b/t/t1506-rev-parse-upstream.sh\nnew file mode 100755\nindex 0000000..5abdc13\n--- /dev/null\n+++ b/t/t1506-rev-parse-upstream.sh\n@@ -0,0 +1,69 @@\n+#!/bin/sh\n+\n+test_description='test @{upstream} syntax'\n+\n+. ./test-lib.sh\n+\n+\n+test_expect_success 'setup' '\n+\n+\ttest_commit 1 &&\n+\tgit checkout -b side &&\n+\ttest_commit 2 &&\n+\tgit checkout master &&\n+\tgit clone . clone &&\n+\ttest_commit 3 &&\n+\t(cd clone &&\n+\t test_commit 4 &&\n+\t git branch --track my-side origin/side)\n+\n+'\n+\n+full_name () {\n+\t(cd clone &&\n+\t git rev-parse --symbolic-full-name \"$@\")\n+}\n+\n+commit_subject () {\n+\t(cd clone &&\n+\t git show -s --pretty=format:%s \"$@\")\n+}\n+\n+test_expect_success '@{upstream} resolves to correct full name' '\n+\ttest refs/remotes/origin/master = \"$(full_name @{upstream})\"\n+'\n+\n+test_expect_success '@{u} resolves to correct full name' '\n+\ttest refs/remotes/origin/master = \"$(full_name @{u})\"\n+'\n+\n+test_expect_success 'my-side@{upstream} resolves to correct full name' '\n+\ttest refs/remotes/origin/side = \"$(full_name my-side@{u})\"\n+'\n+\n+test_expect_success 'my-side@{u} resolves to correct commit' '\n+\tgit checkout side &&\n+\ttest_commit 5 &&\n+\t(cd clone && git fetch) &&\n+\ttest 2 = \"$(commit_subject my-side)\" &&\n+\ttest 5 = \"$(commit_subject my-side@{u})\"\n+'\n+\n+test_expect_success 'not-tracking@{u} fails' '\n+\ttest_must_fail full_name non-tracking@{u} &&\n+\t(cd clone && git checkout --no-track -b non-tracking) &&\n+\ttest_must_fail full_name non-tracking@{u}\n+'\n+\n+test_expect_success '@{u}@{1} resolves correctly' '\n+\ttest_commit 6 &&\n+\t(cd clone && git fetch) &&\n+\ttest 5 = $(commit_subject my-side@{u}@{1})\n+'\n+\n+test_expect_success '@{u} without specifying branch fails on a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_must_fail git rev-parse @{u}\n+'\n+\n+test_done\n-- \n1.6.6.280.ge295b7.dirty\n"},{"id":"131446","messageId":"alpine.DEB.1.00.1001131024420.3043@intel-tinevez-2-302","threadId":"22172","inReplyTo":"7vwrzmqypn.fsf@alter.siamese.dyndns.org","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-13T09:26:05Z","receivedAt":"2010-01-13T09:26:05Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 12 Jan 2010, Junio C Hamano wrote:\n\n> I wondered why it doesn't hook into interpret_branch_name(), and instead \n> adds itself to the static substitute_branch_name(); it forbids the use \n> of the syntax from by callers of strbuf_branchname().\n\nI _think_ it was to allow something like\n\n\tgit log -g @{u}\n\nbut frankly, this is so long ago, I do not remember, I reconstructed this \nreasoning as being the most likely.\n\nCiao,\nDscho\n"},{"id":"131455","messageId":"7v6376pc9w.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"alpine.DEB.1.00.1001131024420.3043@intel-tinevez-2-302","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T09:47:07Z","receivedAt":"2010-01-13T09:47:07Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> I wondered why it doesn't hook into interpret_branch_name(), and instead \n>> adds itself to the static substitute_branch_name(); it forbids the use \n>> of the syntax from by callers of strbuf_branchname().\n>\n> I _think_ it was to allow something like\n>\n> \tgit log -g @{u}\n>\n> but frankly, this is so long ago, I do not remember, I reconstructed this \n> reasoning as being the most likely.\n\nThat is not the question I was asking.\n\nIf you compare substitute_branch_name() and interpret_branch_name() before\nyour patch, you will notice that they are _meant_ to do the same thing,\nwith different external API, only because many callers in sha1_name.c do\nnot use strbuf to hold their names. The primary API is the latter (which\nis extern), and the former (which is static) is merely a helping wrapper\nthat is internal to sha1_name.c\n\nBut with your patch, they suddenly have different semantics, and the\nfunction that implements the primary API doesn't know anything about\nthis new @{upstream} syntax.\n\nThis discrepancy will affect callers of strbuf_branchname(), e.g.\nmerge_name() in builtin-merge.c that prepares the \"Merge branch nitfol of\nremote frotz\" message, or delete_branches() in builtin-branch.c.\n\nNote that I am not saying \"branch -d @{upstream}\" should or should not\nwork (at least not yet---I haven't thought the issues through). But I\nwanted to know if this subtle change in the semantics was a deliberate\nchoice, and if so wanted to see the reason behind it described clearly.\n"},{"id":"131460","messageId":"alpine.DEB.1.00.1001131202510.3043@intel-tinevez-2-302","threadId":"22172","inReplyTo":"7v6376pc9w.fsf@alter.siamese.dyndns.org","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-13T11:04:19Z","receivedAt":"2010-01-13T11:04:19Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 13 Jan 2010, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> >> I wondered why it doesn't hook into interpret_branch_name(), and \n> >> instead adds itself to the static substitute_branch_name(); it \n> >> forbids the use of the syntax from by callers of strbuf_branchname().\n> >\n> > I _think_ it was to allow something like\n> >\n> > \tgit log -g @{u}\n> >\n> > but frankly, this is so long ago, I do not remember, I reconstructed this \n> > reasoning as being the most likely.\n> \n> That is not the question I was asking.\n> \n> If you compare substitute_branch_name() and interpret_branch_name() before\n> your patch, you will notice that they are _meant_ to do the same thing,\n> with different external API, only because many callers in sha1_name.c do\n> not use strbuf to hold their names. The primary API is the latter (which\n> is extern), and the former (which is static) is merely a helping wrapper\n> that is internal to sha1_name.c\n\nSo you meant to say that substitute_branch_name() calls \ninterpret_branch_name(), so the change should be in the latter. (This is \nsupposed to be the summary of your 4 paragraphs.)\n\nI have no problems with that, except that I do not have the time to do it \nmyself.\n\nCiao,\nDscho\n"},{"id":"131515","messageId":"7vpr5dn5vt.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"alpine.DEB.1.00.1001131202510.3043@intel-tinevez-2-302","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T19:48:06Z","receivedAt":"2010-01-13T19:48:06Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> So you meant to say that substitute_branch_name() calls \n> interpret_branch_name(), so the change should be in the latter. (This is \n> supposed to be the summary of your 4 paragraphs.)\n\nNot quite. What I was asking was:\n\n\t*PROVIDED* *IF* you wanted to keep the same semantics between\n them, then you would have patched i-b-n, but you didn't. Was there\n a reason callers of s-b-n should know about @{u} but callers of i-b-n\n\tshouldn't?\n\nExpected answer was either:\n\n\t(a) Codepath X that uses i-b-n shouldn't interpret @{upstream} as\n a symbolic name given by the user, but it should treat it as a\n mere SHA-1 expression instead for *this and that* reason.\n Otherwise we will see *this* breakage when the user does\n *that*. That is why i-b-n doesn't know about the new syntax;\n or\n\n\t(b) It was a thinko; all codepaths that use i-b-n should know the\n new syntax as they _are_ interested in learning the symbolic\n name when the user gives @{upstream}.\n"},{"id":"131542","messageId":"alpine.DEB.1.00.1001132348570.4985@pacific.mpi-cbg.de","threadId":"22172","inReplyTo":"7vpr5dn5vt.fsf@alter.siamese.dyndns.org","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-13T22:49:30Z","receivedAt":"2010-01-13T22:49:30Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 13 Jan 2010, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> What I was asking was:\n> \n> \t*PROVIDED* *IF* you wanted to keep the same semantics between\n> them, then you would have patched i-b-n, but you didn't. Was there\n> a reason callers of s-b-n should know about @{u} but callers of i-b-n\n> \tshouldn't?\n> \n> Expected answer was either:\n> \n> \t(a) Codepath X that uses i-b-n shouldn't interpret @{upstream} as\n> a symbolic name given by the user, but it should treat it as a\n> mere SHA-1 expression instead for *this and that* reason.\n> Otherwise we will see *this* breakage when the user does\n> *that*. That is why i-b-n doesn't know about the new syntax;\n> or\n> \n> \t(b) It was a thinko; all codepaths that use i-b-n should know the\n> new syntax as they _are_ interested in learning the symbolic\n> name when the user gives @{upstream}.\n\nAnd I gave answer (c): I do not remember.\n\nCiao,\nDscho\n"},{"id":"131546","messageId":"7vska9k393.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"alpine.DEB.1.00.1001132348570.4985@pacific.mpi-cbg.de","subject":"Re: default behaviour for `gitmerge` (no arguments)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-13T23:13:12Z","receivedAt":"2010-01-13T23:13:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> What I was asking was:\n>> \n>> \t*PROVIDED* *IF* you wanted to keep the same semantics between\n>> them, then you would have patched i-b-n, but you didn't. Was there\n>> a reason callers of s-b-n should know about @{u} but callers of i-b-n\n>> \tshouldn't?\n>> \n>> Expected answer was either:\n>> \n>> \t(a) Codepath X that uses i-b-n shouldn't interpret @{upstream} as\n>> a symbolic name given by the user, but it should treat it as a\n>> mere SHA-1 expression instead for *this and that* reason.\n>> Otherwise we will see *this* breakage when the user does\n>> *that*. That is why i-b-n doesn't know about the new syntax;\n>> or\n>> \n>> \t(b) It was a thinko; all codepaths that use i-b-n should know the\n>> new syntax as they _are_ interested in learning the symbolic\n>> name when the user gives @{upstream}.\n>\n> And I gave answer (c): I do not remember.\n\nThat's fine. As long as we know it is (c) (your earlier response sounded\nas if you were saying (b)), we know that there might be issues we need to\nfind and carefully look at before using this topic.\n"},{"id":"132172","messageId":"1263980322-4142-1-git-send-email-gitster@pobox.com","threadId":"22172","inReplyTo":"7vwrzmqypn.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] @{u} updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-20T09:38:40Z","receivedAt":"2010-01-20T09:38:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier I wondered if the approach Dscho's patch takes to teach the new\n@{upstream} syntax to substitute_branch_name() (hence dwim_ref()) without\nteaching it to interpret_branch_name() (hence strbuf_branchname()) was a\nbad idea. I thought about this a bit more; there are some downsides for\nnot doing so.\n\nThe first patch adds a handful of tests that show why strbuf_branchname()\ncallers may also want to learn about the new syntax. The second patch\nmoves the logic to interpret_branch_name() to make them happier.\n\nThe name of the key function was changed from tracked_suffix() to\nupstream_mark(), not only because the syntax talks about @{upstream}, but\nbecause the parsing needs to recognize the @{u}/@{upstream} mark at the\nbeginning of the given string (that is a suffix to some other string), and\nstrip it (the earlier code wanted @{u} to be at the very end but the\ncallers need to have it at the beginning).\n\n\nJunio C Hamano (2):\n t1506: more test for @{upstream} syntax\n Teach @{upstream} syntax to strbuf_branchanme()\n\n sha1_name.c | 116 ++++++++++++++++++++++++++---------------\n t/t1506-rev-parse-upstream.sh | 41 ++++++++++++++\n 2 files changed, 115 insertions(+), 42 deletions(-)\n"},{"id":"132173","messageId":"1263980322-4142-2-git-send-email-gitster@pobox.com","threadId":"22172","inReplyTo":"1263980322-4142-1-git-send-email-gitster@pobox.com","subject":"[PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-20T09:38:41Z","receivedAt":"2010-01-20T09:38:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds a few more tests that exercises @{upstream} syntax by commands\nthat operate differently when they are given branch name as opposed to a\nrefname (i.e. where \"master\" and \"refs/heads/master\" makes a difference).\n\nSigned-off-by: Junio C Hamano \n---\n t/t1506-rev-parse-upstream.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 41 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1506-rev-parse-upstream.sh b/t/t1506-rev-parse-upstream.sh\nindex 5abdc13..a2c7f92 100755\n--- a/t/t1506-rev-parse-upstream.sh\n+++ b/t/t1506-rev-parse-upstream.sh\n@@ -66,4 +66,45 @@ test_expect_success '@{u} without specifying branch fails on a detached HEAD' '\n \ttest_must_fail git rev-parse @{u}\n '\n \n+test_expect_success 'checkout -b new my-side@{u} forks from the same' '\n+(\n+\tcd clone &&\n+\tgit checkout -b new my-side@{u} &&\n+\tgit rev-parse --symbolic-full-name my-side@{u} >expect &&\n+\tgit rev-parse --symbolic-full-name new@{u} >actual &&\n+\ttest_cmp expect actual\n+)\n+'\n+\n+test_expect_failure 'merge my-side@{u} records the correct name' '\n+(\n+\tsq=\"'\\''\" &&\n+\tcd clone || exit\n+\tgit checkout master || exit\n+\tgit branch -D new ;# can fail but is ok\n+\tgit branch -t new my-side@{u} &&\n+\tgit merge -s ours new@{u} &&\n+\tgit show -s --pretty=format:%s >actual &&\n+\techo \"Merge remote branch ${sq}origin/side${sq}\" >expect &&\n+\ttest_cmp expect actual\n+)\n+'\n+\n+test_expect_failure 'branch -d other@{u}' '\n+\tgit checkout -t -b other master &&\n+\tgit branch -d @{u} &&\n+\tgit for-each-ref refs/heads/master >actual &&\n+\t>expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'checkout other@{u}' '\n+\tgit branch -f master HEAD &&\n+\tgit checkout -t -b another master &&\n+\tgit checkout @{u} &&\n+\tgit symbolic-ref HEAD >actual &&\n+\techo refs/heads/master >expect &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.6.6.513.g63f4c\n"},{"id":"132174","messageId":"1263980322-4142-3-git-send-email-gitster@pobox.com","threadId":"22172","inReplyTo":"1263980322-4142-1-git-send-email-gitster@pobox.com","subject":"[PATCH 2/2] Teach @{upstream} syntax to strbuf_branchanme()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-20T09:38:42Z","receivedAt":"2010-01-20T09:38:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This teaches @{upstream} syntax to interpret_branch_name(), instead\nof dwim_ref() machinery.\n\nThere are places in git UI that behaves differently when you give a local\nbranch name and when you give an extended SHA-1 expression that evaluates\nto the commit object name at the tip of the branch. The intent is that\nthe special syntax such as @{-1} can stand in as if the user spelled the\nname of the branch in such places.\n\nThe name of the branch \"frotz\" to switch to (\"git checkout frotz\"), and\nthe name of the branch \"nitfol\" to fork a new branch \"frotz\" from (\"git\ncheckout -b frotz nitfol\"), are examples of such places. These places\ntake only the name of the branch (e.g. \"frotz\"), and they are supposed to\nact differently to an equivalent refname (e.g. \"refs/heads/frotz\"), so\nhooking the @{upstream} and @{-N} syntax to dwim_ref() is insufficient\nwhen we want to deal with cases a local branch is forked from another\nlocal branch and use \"forked@{upstream}\" to name the forkee branch.\n\nThe \"upstream\" syntax \"forked@{u}\" is to specify the ref that \"forked\" is\nconfigured to merge with, and most often the forkee is a remote tracking\nbranch, not a local branch. We cannot simply return a local branch name,\nbut that does not necessarily mean we have to returns the full refname\n(e.g. refs/remotes/origin/frotz, when returning origin/frotz is enough).\nThis update calls shorten_unambiguous_ref() to do so.\n\nSigned-off-by: Junio C Hamano \n---\n sha1_name.c | 116 ++++++++++++++++++++++++++---------------\n t/t1506-rev-parse-upstream.sh | 6 +-\n 2 files changed, 77 insertions(+), 45 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex fb4e214..2376c6d 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -239,24 +239,10 @@ static int ambiguous_path(const char *path, int len)\n \treturn slash;\n }\n \n-static inline int tracked_suffix(const char *string, int len)\n-{\n-\tconst char *suffix[] = { \"@{upstream}\", \"@{u}\" };\n-\tint i;\n-\n-\tfor (i = 0; i < ARRAY_SIZE(suffix); i++) {\n-\t\tint suffix_len = strlen(suffix[i]);\n-\t\tif (len >= suffix_len && !memcmp(string + len - suffix_len,\n-\t\t\t\t\tsuffix[i], suffix_len))\n-\t\t\treturn suffix_len;\n-\t}\n-\treturn 0;\n-}\n-\n /*\n * *string and *len will only be substituted, and *string returned (for\n- * later free()ing) if the string passed in is of the form @{-} or\n- * of the form @{upstream}.\n+ * later free()ing) if the string passed in is a magic short-hand form\n+ * to name a branch.\n */\n static char *substitute_branch_name(const char **string, int *len)\n {\n@@ -270,21 +256,6 @@ static char *substitute_branch_name(const char **string, int *len)\n \t\treturn (char *)*string;\n \t}\n \n-\tret = tracked_suffix(*string, *len);\n-\tif (ret) {\n-\t\tchar *ref = xstrndup(*string, *len - ret);\n-\t\tstruct branch *tracking = branch_get(*ref ? ref : NULL);\n-\n-\t\tif (!tracking)\n-\t\t\tdie (\"No tracking branch found for '%s'\", ref);\n-\t\tfree(ref);\n-\t\tif (tracking->merge && tracking->merge[0]->dst) {\n-\t\t\t*string = xstrdup(tracking->merge[0]->dst);\n-\t\t\t*len = strlen(*string);\n-\t\t\treturn (char *)*string;\n-\t\t}\n-\t}\n-\n \treturn NULL;\n }\n \n@@ -354,6 +325,20 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \treturn logs_found;\n }\n \n+static inline int upstream_mark(const char *string, int len)\n+{\n+\tconst char *suffix[] = { \"@{upstream}\", \"@{u}\" };\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(suffix); i++) {\n+\t\tint suffix_len = strlen(suffix[i]);\n+\t\tif (suffix_len <= len\n+\t\t && !memcmp(string, suffix[i], suffix_len))\n+\t\t\treturn suffix_len;\n+\t}\n+\treturn 0;\n+}\n+\n static int get_sha1_1(const char *name, int len, unsigned char *sha1);\n \n static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n@@ -371,7 +356,7 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \tif (len && str[len-1] == '}') {\n \t\tfor (at = len-2; at >= 0; at--) {\n \t\t\tif (str[at] == '@' && str[at+1] == '{') {\n-\t\t\t\tif (!tracked_suffix(str + at, len - at)) {\n+\t\t\t\tif (!upstream_mark(str + at, len - at)) {\n \t\t\t\t\treflog_len = (len-1) - (at+2);\n \t\t\t\t\tlen = at;\n \t\t\t\t}\n@@ -773,17 +758,10 @@ static int grab_nth_branch_switch(unsigned char *osha1, unsigned char *nsha1,\n }\n \n /*\n- * This reads \"@{-N}\" syntax, finds the name of the Nth previous\n- * branch we were on, and places the name of the branch in the given\n- * buf and returns the number of characters parsed if successful.\n- *\n- * If the input is not of the accepted format, it returns a negative\n- * number to signal an error.\n- *\n- * If the input was ok but there are not N branch switches in the\n- * reflog, it returns 0.\n+ * Parse @{-N} syntax, return the number of characters parsed\n+ * if successful; otherwise signal an error with negative value.\n */\n-int interpret_branch_name(const char *name, struct strbuf *buf)\n+static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf)\n {\n \tlong nth;\n \tint i, retval;\n@@ -828,6 +806,60 @@ release_return:\n }\n \n /*\n+ * This reads short-hand syntax that not only evaluates to a commit\n+ * object name, but also can act as if the end user spelled the name\n+ * of the branch from the command line.\n+ *\n+ * - \"@{-N}\" finds the name of the Nth previous branch we were on, and\n+ * places the name of the branch in the given buf and returns the\n+ * number of characters parsed if successful.\n+ *\n+ * - \"@{upstream}\" finds the name of the other ref that\n+ * is configured to merge with (missing defaults\n+ * to the current branch), and places the name of the branch in the\n+ * given buf and returns the number of characters parsed if\n+ * successful.\n+ *\n+ * If the input is not of the accepted format, it returns a negative\n+ * number to signal an error.\n+ *\n+ * If the input was ok but there are not N branch switches in the\n+ * reflog, it returns 0.\n+ */\n+int interpret_branch_name(const char *name, struct strbuf *buf)\n+{\n+\tchar *cp;\n+\tstruct branch *upstream;\n+\tint namelen = strlen(name);\n+\tint len = interpret_nth_prior_checkout(name, buf);\n+\tint tmp_len;\n+\n+\tif (!len)\n+\t\treturn len; /* syntax Ok, not enough switches */\n+\tif (0 < len)\n+\t\treturn len; /* consumed from the front */\n+\tcp = strchr(name, '@');\n+\tif (!cp)\n+\t\treturn -1;\n+\ttmp_len = upstream_mark(cp, namelen - (cp - name));\n+\tif (!tmp_len)\n+\t\treturn -1;\n+\tlen = cp + tmp_len - name;\n+\tcp = xstrndup(name, cp - name);\n+\tupstream = branch_get(*cp ? cp : NULL);\n+\tif (!upstream\n+\t || !upstream->merge\n+\t || !upstream->merge[0]->dst)\n+\t\treturn error(\"No upstream branch found for '%s'\", cp);\n+\tfree(cp);\n+\tcp = shorten_unambiguous_ref(upstream->merge[0]->dst, 0);\n+\tstrbuf_reset(buf);\n+\tstrbuf_addstr(buf, cp);\n+\tfree(cp);\n+\treturn len;\n+}\n+\n+/*\n * This is like \"get_sha1_basic()\", except it allows \"sha1 expressions\",\n * notably \"xyz^\" for \"parent of xyz\"\n */\ndiff --git a/t/t1506-rev-parse-upstream.sh b/t/t1506-rev-parse-upstream.sh\nindex a2c7f92..95c9b09 100755\n--- a/t/t1506-rev-parse-upstream.sh\n+++ b/t/t1506-rev-parse-upstream.sh\n@@ -76,7 +76,7 @@ test_expect_success 'checkout -b new my-side@{u} forks from the same' '\n )\n '\n \n-test_expect_failure 'merge my-side@{u} records the correct name' '\n+test_expect_success 'merge my-side@{u} records the correct name' '\n (\n \tsq=\"'\\''\" &&\n \tcd clone || exit\n@@ -90,7 +90,7 @@ test_expect_failure 'merge my-side@{u} records the correct name' '\n )\n '\n \n-test_expect_failure 'branch -d other@{u}' '\n+test_expect_success 'branch -d other@{u}' '\n \tgit checkout -t -b other master &&\n \tgit branch -d @{u} &&\n \tgit for-each-ref refs/heads/master >actual &&\n@@ -98,7 +98,7 @@ test_expect_failure 'branch -d other@{u}' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'checkout other@{u}' '\n+test_expect_success 'checkout other@{u}' '\n \tgit branch -f master HEAD &&\n \tgit checkout -t -b another master &&\n \tgit checkout @{u} &&\n-- \n1.6.6.513.g63f4c\n"},{"id":"132193","messageId":"alpine.DEB.1.00.1001201408100.31129@intel-tinevez-2-302","threadId":"22172","inReplyTo":"1263980322-4142-1-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 0/2] @{u} updates","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2010-01-20T13:08:24Z","receivedAt":"2010-01-20T13:08:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 20 Jan 2010, Junio C Hamano wrote:\n\n> Earlier I wondered if the approach Dscho's patch takes to teach the new\n> @{upstream} syntax to substitute_branch_name() (hence dwim_ref()) without\n> teaching it to interpret_branch_name() (hence strbuf_branchname()) was a\n> bad idea. I thought about this a bit more; there are some downsides for\n> not doing so.\n> \n> The first patch adds a handful of tests that show why strbuf_branchname()\n> callers may also want to learn about the new syntax. The second patch\n> moves the logic to interpret_branch_name() to make them happier.\n\nLooks good to me.\n\nCiao,\nDscho\n"},{"id":"132675","messageId":"20100126130745.GB28179@coredump.intra.peff.net","threadId":"22172","inReplyTo":"1263980322-4142-2-git-send-email-gitster@pobox.com","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-26T13:07:45Z","receivedAt":"2010-01-26T13:07:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 20, 2010 at 01:38:41AM -0800, Junio C Hamano wrote:\n\n> This adds a few more tests that exercises @{upstream} syntax by commands\n> that operate differently when they are given branch name as opposed to a\n> refname (i.e. where \"master\" and \"refs/heads/master\" makes a difference).\n\nOverall this looks good, but there are a few minor defects. I haven't\nhad a chance to fix them yet, but here are tests showing them. I hope to\nget to them pre-1.7.0, but please feel free to take a crack at them if\nyou want.\n\nThe first one is that @{usptream} silently becomes @{0}. I think\nwe need to double-check whether approxidate found absolutely nothing,\nand complain if that is the case.\n\ndiff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh\nnew file mode 100755\nindex 0000000..da43386\n--- /dev/null\n+++ b/t/t0101-at-syntax.sh\n@@ -0,0 +1,41 @@\n+#!/bin/sh\n+\n+test_description='various @{whatever} syntax tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\ttest_commit two\n+'\n+\n+check_at() {\n+\techo \"$2\" >expect &&\n+\tgit log -1 --format=%s \"$1\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success '@{0} shows current' '\n+\tcheck_at @{0} two\n+'\n+\n+test_expect_success '@{1} shows old' '\n+\tcheck_at @{1} one\n+'\n+\n+test_expect_success '@{now} shows current' '\n+\tcheck_at @{now} two\n+'\n+\n+test_expect_success '@{30.years.ago} shows old' '\n+\tcheck_at @{30.years.ago} one\n+'\n+\n+test_expect_success 'silly approxidates work' '\n+\tcheck_at @{3.hot.dogs.and.30.years.ago} one\n+'\n+\n+test_expect_failure 'complain about total nonsense' '\n+\ttest_must_fail git log -1 --format=%s @{utter.bogosity}\n+'\n+\n+test_done\n\nThe second one is that \"log -g branch@{u}\" shows the correct commits\n(from the upstream of \"branch\"), but displays the incorrect reflog\ninformation (it shows information for \"branch\", not for its upstream).\n\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 95c9b09..cbe1b25 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -107,4 +107,18 @@ test_expect_success 'checkout other@{u}' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <)\n+Reflog message: branch: Created from HEAD\n+Author: A U Thor \n+Date: Thu Apr 7 15:15:13 2005 -0700\n+\n+ 3\n+EOF\n+test_expect_failure 'log -g other@{u}' '\n+\tgit log -1 -g other@{u} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"132708","messageId":"7vockgzliv.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100126130745.GB28179@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-26T19:58:00Z","receivedAt":"2010-01-26T19:58:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The first one is that @{usptream} silently becomes @{0}. I think\n> we need to double-check whether approxidate found absolutely nothing,\n> and complain if that is the case.\n\nThis is not a new problem introduced by Dscho's @{u} series; it was there\neven before 861f00e (fix reflog approxidate parsing bug, 2008-04-30).\n\nNevertheless, it would be nice to fix it.\n\n-- >8 --\nSubject: approxidate_careful() reports errorneous date string\n\nFor a long time, the time based reflog syntax (e.g. master@{yesterday})\ndidn't complain when the \"human readable\" timestamp was misspelled, as\nthe underlying mechanism tried to be as lenient as possible. The funny\nthing was that parsing of \"@{now}\" even relied on the fact that anything\nnot recognized by the machinery returned the current timestamp.\n\nIntroduce approxidate_careful() that takes an optional pointer to an\ninteger, that gets assigned 1 when the input does not make sense as a\ntimestamp.\n\nAs I am too lazy to fix all the callers that use approxidate(), most of\nthe callers do not take advantage of the error checking, but convert the\ncode to parse reflog to use it as a demonstration.\n\nTests are mostly from Jeff King.\n\nSigned-off-by: Junio C Hamano \n---\n cache.h | 3 ++-\n date.c | 43 +++++++++++++++++++++++++++++++++++--------\n sha1_name.c | 5 ++++-\n t/t0101-at-syntax.sh | 45 +++++++++++++++++++++++++++++++++++++++++++++\n 4 files changed, 86 insertions(+), 10 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex b3370eb..f0fea2d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -762,7 +762,8 @@ const char *show_date_relative(unsigned long time, int tz,\n \t\t\t size_t timebuf_size);\n int parse_date(const char *date, char *buf, int bufsize);\n void datestamp(char *buf, int bufsize);\n-unsigned long approxidate(const char *);\n+#define approxidate(s) approxidate_careful(s, NULL)\n+unsigned long approxidate_careful(const char *, int *);\n unsigned long approxidate_relative(const char *date, const struct timeval *now);\n enum date_mode parse_date_format(const char *format);\n \ndiff --git a/date.c b/date.c\nindex 45f3684..002aa3c 100644\n--- a/date.c\n+++ b/date.c\n@@ -696,6 +696,11 @@ static unsigned long update_tm(struct tm *tm, struct tm *now, unsigned long sec)\n \treturn n;\n }\n \n+static void date_now(struct tm *tm, struct tm *now, int *num)\n+{\n+\tupdate_tm(tm, now, 0);\n+}\n+\n static void date_yesterday(struct tm *tm, struct tm *now, int *num)\n {\n \tupdate_tm(tm, now, 24*60*60);\n@@ -770,6 +775,7 @@ static const struct special {\n \t{ \"PM\", date_pm },\n \t{ \"AM\", date_am },\n \t{ \"never\", date_never },\n+\t{ \"now\", date_now },\n \t{ NULL }\n };\n \n@@ -790,7 +796,7 @@ static const struct typelen {\n \t{ NULL }\n };\n \n-static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm *now, int *num)\n+static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm *now, int *num, int *touched)\n {\n \tconst struct typelen *tl;\n \tconst struct special *s;\n@@ -804,6 +810,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\tint match = match_string(date, month_names[i]);\n \t\tif (match >= 3) {\n \t\t\ttm->tm_mon = i;\n+\t\t\t*touched = 1;\n \t\t\treturn end;\n \t\t}\n \t}\n@@ -812,6 +819,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\tint len = strlen(s->name);\n \t\tif (match_string(date, s->name) == len) {\n \t\t\ts->fn(tm, now, num);\n+\t\t\t*touched = 1;\n \t\t\treturn end;\n \t\t}\n \t}\n@@ -821,11 +829,14 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\t\tint len = strlen(number_name[i]);\n \t\t\tif (match_string(date, number_name[i]) == len) {\n \t\t\t\t*num = i;\n+\t\t\t\t*touched = 1;\n \t\t\t\treturn end;\n \t\t\t}\n \t\t}\n-\t\tif (match_string(date, \"last\") == 4)\n+\t\tif (match_string(date, \"last\") == 4) {\n \t\t\t*num = 1;\n+\t\t\t*touched = 1;\n+\t\t}\n \t\treturn end;\n \t}\n \n@@ -835,6 +846,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\tif (match_string(date, tl->type) >= len-1) {\n \t\t\tupdate_tm(tm, now, tl->length * *num);\n \t\t\t*num = 0;\n+\t\t\t*touched = 1;\n \t\t\treturn end;\n \t\t}\n \t\ttl++;\n@@ -852,6 +864,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\t\tdiff += 7*n;\n \n \t\t\tupdate_tm(tm, now, diff * 24 * 60 * 60);\n+\t\t\t*touched = 1;\n \t\t\treturn end;\n \t\t}\n \t}\n@@ -866,6 +879,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\t\ttm->tm_year--;\n \t\t}\n \t\ttm->tm_mon = n;\n+\t\t*touched = 1;\n \t\treturn end;\n \t}\n \n@@ -873,6 +887,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \t\tupdate_tm(tm, now, 0); /* fill in date fields if needed */\n \t\ttm->tm_year -= *num;\n \t\t*num = 0;\n+\t\t*touched = 1;\n \t\treturn end;\n \t}\n \n@@ -929,9 +944,12 @@ static void pending_number(struct tm *tm, int *num)\n \t}\n }\n \n-static unsigned long approxidate_str(const char *date, const struct timeval *tv)\n+static unsigned long approxidate_str(const char *date,\n+\t\t\t\t const struct timeval *tv,\n+\t\t\t\t int *error_ret)\n {\n \tint number = 0;\n+\tint touched = 0;\n \tstruct tm tm, now;\n \ttime_t time_sec;\n \n@@ -951,33 +969,42 @@ static unsigned long approxidate_str(const char *date, const struct timeval *tv)\n \t\tif (isdigit(c)) {\n \t\t\tpending_number(&tm, &number);\n \t\t\tdate = approxidate_digit(date-1, &tm, &number);\n+\t\t\ttouched = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (isalpha(c))\n-\t\t\tdate = approxidate_alpha(date-1, &tm, &now, &number);\n+\t\t\tdate = approxidate_alpha(date-1, &tm, &now, &number, &touched);\n \t}\n \tpending_number(&tm, &number);\n+\tif (!touched)\n+\t\t*error_ret = 1;\n \treturn update_tm(&tm, &now, 0);\n }\n \n unsigned long approxidate_relative(const char *date, const struct timeval *tv)\n {\n \tchar buffer[50];\n+\tint errors = 0;\n \n \tif (parse_date(date, buffer, sizeof(buffer)) > 0)\n \t\treturn strtoul(buffer, NULL, 0);\n \n-\treturn approxidate_str(date, tv);\n+\treturn approxidate_str(date, tv, &errors);\n }\n \n-unsigned long approxidate(const char *date)\n+unsigned long approxidate_careful(const char *date, int *error_ret)\n {\n \tstruct timeval tv;\n \tchar buffer[50];\n+\tint dummy = 0;\n+\tif (!error_ret)\n+\t\terror_ret = &dummy;\n \n-\tif (parse_date(date, buffer, sizeof(buffer)) > 0)\n+\tif (parse_date(date, buffer, sizeof(buffer)) > 0) {\n+\t\t*error_ret = 0;\n \t\treturn strtoul(buffer, NULL, 0);\n+\t}\n \n \tgettimeofday(&tv, NULL);\n-\treturn approxidate_str(date, &tv);\n+\treturn approxidate_str(date, &tv, error_ret);\n }\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 9215ad1..ed4c028 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t} else if (0 <= nth)\n \t\t\tat_time = 0;\n \t\telse {\n+\t\t\tint errors = 0;\n \t\t\tchar *tmp = xstrndup(str + at + 2, reflog_len);\n-\t\t\tat_time = approxidate(tmp);\n+\t\t\tat_time = approxidate_careful(tmp, &errors);\n+\t\t\tif (errors)\n+\t\t\t\tdie(\"Bogus timestamp '%s'\", tmp);\n \t\t\tfree(tmp);\n \t\t}\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\ndiff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh\nnew file mode 100755\nindex 0000000..ccabc37\n--- /dev/null\n+++ b/t/t0101-at-syntax.sh\n@@ -0,0 +1,45 @@\n+#!/bin/sh\n+\n+test_description='various @{whatever} syntax tests'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\ttest_commit one &&\n+\ttest_commit two\n+'\n+\n+check_at() {\n+\techo \"$2\" >expect &&\n+\tgit log -1 --format=%s \"$1\" >actual &&\n+\ttest_cmp expect actual\n+}\n+\n+test_expect_success '@{0} shows current' '\n+\tcheck_at @{0} two\n+'\n+\n+test_expect_success '@{1} shows old' '\n+\tcheck_at @{1} one\n+'\n+\n+test_expect_success '@{now} shows current' '\n+\tcheck_at @{now} two\n+'\n+\n+test_expect_success '@{30.years.ago} shows old' '\n+\tcheck_at @{30.years.ago} one\n+'\n+\n+test_expect_success 'silly approxidates work' '\n+\tcheck_at @{3.hot.dogs.and.30.years.ago} one\n+'\n+\n+test_expect_success 'notice misspelled upstream' '\n+\ttest_must_fail git log -1 --format=%s @{usptream}\n+'\n+\n+test_expect_success 'complain about total nonsense' '\n+\ttest_must_fail git log -1 --format=%s @{utter.bogosity}\n+'\n+\n+test_done\n"},{"id":"132717","messageId":"7vpr4wy2lk.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100126130745.GB28179@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-26T21:32:07Z","receivedAt":"2010-01-26T21:32:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The second one is that \"log -g branch@{u}\" shows the correct commits\n> (from the upstream of \"branch\"), but displays the incorrect reflog\n> information (it shows information for \"branch\", not for its upstream).\n\nThat \"walk-reflog\" code is tricky. How about this?\n\nI don't know if it deals with things like \"@{-1}@{u}@{now}\"; the users\nshould have every right to expect it to, but I didn't consciously try to\nmake that work with this patch.\n\n revision.c | 18 ++++++++++++++----\n t/t1507-rev-parse-upstream.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex f54d43f..75071af 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -134,10 +134,20 @@ static void add_pending_object_with_mode(struct rev_info *revs, struct object *o\n {\n \tif (revs->no_walk && (obj->flags & UNINTERESTING))\n \t\trevs->no_walk = 0;\n-\tif (revs->reflog_info && obj->type == OBJ_COMMIT &&\n-\t\t\tadd_reflog_for_walk(revs->reflog_info,\n-\t\t\t\t(struct commit *)obj, name))\n-\t\treturn;\n+\tif (revs->reflog_info && obj->type == OBJ_COMMIT) {\n+\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\tint len = interpret_branch_name(name, &buf);\n+\t\tint st;\n+\n+\t\tif (len && name[len])\n+\t\t\tstrbuf_addstr(&buf, name + len);\n+\t\tst = add_reflog_for_walk(revs->reflog_info,\n+\t\t\t\t\t (struct commit *)obj,\n+\t\t\t\t\t buf.buf[0] ? buf.buf: name);\n+\t\tstrbuf_release(&buf);\n+\t\tif (st)\n+\t\t\treturn;\n+\t}\n \tadd_object_array_with_mode(obj, name, &revs->pending, mode);\n }\n \ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex 95c9b09..8c8dfda 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -107,4 +107,33 @@ test_expect_success 'checkout other@{u}' '\n \ttest_cmp expect actual\n '\n \n+cat >expect <)\n+Reflog message: branch: Created from HEAD\n+Author: A U Thor \n+Date: Thu Apr 7 15:15:13 2005 -0700\n+\n+ 3\n+EOF\n+test_expect_success 'log -g other@{u}' '\n+\tgit log -1 -g other@{u} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+cat >expect <)\n+Reflog message: branch: Created from HEAD\n+Author: A U Thor \n+Date: Thu Apr 7 15:15:13 2005 -0700\n+\n+ 3\n+EOF\n+\n+test_expect_success 'log -g other@{u}@{now}' '\n+\tgit log -1 -g other@{u}@{now} >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n"},{"id":"132764","messageId":"20100127102417.GD6262@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7vockgzliv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-27T10:24:17Z","receivedAt":"2010-01-27T10:24:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 26, 2010 at 11:58:00AM -0800, Junio C Hamano wrote:\n\n> > The first one is that @{usptream} silently becomes @{0}. I think\n> > we need to double-check whether approxidate found absolutely nothing,\n> > and complain if that is the case.\n> \n> This is not a new problem introduced by Dscho's @{u} series; it was there\n> even before 861f00e (fix reflog approxidate parsing bug, 2008-04-30).\n\nTrue, though now that there is something besides an approxidate to\nmisspell, it is slightly worse. :)\n\n> Introduce approxidate_careful() that takes an optional pointer to an\n> integer, that gets assigned 1 when the input does not make sense as a\n> timestamp.\n\nA minor nit, but wouldn't:\n\n int approxidate_careful(const char *str, unsigned long *out);\n\nreturning an error code be the more usual pattern for a function with\nerror-plus-output (your approxidate wrapper would have to be a function then,\nnot a macro)?\n\n> As I am too lazy to fix all the callers that use approxidate(), most of\n> the callers do not take advantage of the error checking, but convert the\n> code to parse reflog to use it as a demonstration.\n\nI think that is fine, and was the approach I was also going to take.\nApproxidate was meant to be \"try to make sense of anything\". It is\nmainly a problem here because we are combining it with other input, and\nit is hard to tell a misspelling of that other input from a nonsensical\napproxidate. Now that the _careful infrastructure is there, we can fix\nother callsites if people care.\n\n> @@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n> \t\t} else if (0 <= nth)\n> \t\t\tat_time = 0;\n> \t\telse {\n> +\t\t\tint errors = 0;\n> \t\t\tchar *tmp = xstrndup(str + at + 2, reflog_len);\n> -\t\t\tat_time = approxidate(tmp);\n> +\t\t\tat_time = approxidate_careful(tmp, &errors);\n> +\t\t\tif (errors)\n> +\t\t\t\tdie(\"Bogus timestamp '%s'\", tmp);\n> \t\t\tfree(tmp);\n\nI was just going to \"return -1\" here, which yields:\n\n $ git show @{bogosity}\n fatal: ambiguous argument '@{bogosity}': unknown revision or path not in the working tree.\n Use '--' to separate paths from revisions\n\ninstead of\n\n $ git show @{bogosity}\n fatal: Bogus timestamp 'bogosity'\n\nI can't imagine anybody being upset that they had a path '@{usptream}'\nin their repository which was prematurely interpreted as a bogus ref\n(especially since it is _always_ a ref in the current behavior), but it\njust seemed more consistent with the rest of get_sha1_basic, which\ngenerally does not die.\n\nOn the other hand, I think the error message the user sees in your case\nis probably more helpful.\n\n> +test_expect_success '@{30.years.ago} shows old' '\n> +\tcheck_at @{30.years.ago} one\n\nSide note: I chose this because we needed to go back from the current\ntime beyond where test_tick would place the commit. Which means this\ntest has a 2035 bug. :)\n\nI had originally had 100.years.ago, but we seem to have some data-type\nissues with our date handling. We represent seconds-since-epoch as\nunsigned long, which means:\n\n - even though approxidate handles it internally, we can't represent\n dates earlier than the epoch. If you simply store approxidate's\n output as a signed time_t, as test-date does, it does work back to\n 1901. But the reflog code treats it as unsigned, so 100.years.ago,\n though representable, is considered to be far in the future.\n\n - on many platforms ulong is 32 bits, meaning we have 2038 problems\n (though because it's unsigned, I am not sure if we actually have\n 2106 problems; I suspect it may be a mix because of the way\n different parts of the system use time_t versus ulong). Presumably\n time_t will move to 64 bits in the next decade or two, and hopefully\n ulong along with it.\n\nNow obviously neither is a pressing issue. Probably nobody is importing\nany pre-epoch commits, and we have a few decades to worry about future\nissues. But I did legitimately see the bug when trying to guess a \"long\ntime ago\" value. It might be nice to use a signed time_t, and to deal\nwith the overflow (even if it is just to barf and say \"bad time\" instead\nof silently producing wrong, future results).\n\n-Peff\n"},{"id":"132768","messageId":"20100127114009.GE6262@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7vpr4wy2lk.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-27T11:40:09Z","receivedAt":"2010-01-27T11:40:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 26, 2010 at 01:32:07PM -0800, Junio C Hamano wrote:\n\n> I don't know if it deals with things like \"@{-1}@{u}@{now}\"; the users\n> should have every right to expect it to, but I didn't consciously try to\n> make that work with this patch.\n\nNice. This also fixes \"git log -g @{-1}\". Static uses like \"git show\n@{u}@{1.week.ago}\" and \"git show @{-1}@{1.week.ago}\" were already fine,\nso I think the bug was really confined to the reflog walker (and your\nfix is therefore correct).\n\nUsing \"git show @{-1}@{u}\" is still broken, though.\n\nI tried tracing the parsing through get_sha1_basic and\ninterpret_branch_name, but it's pretty confusing. Especially as we seem\nto deal with @{upstream}, @{now}, and @{-1} at different places.\n\nI think the patch below does what we want, but the whole thing feels\noverly complicated to me, especially with the split of parsing @{...}\nbetween get_sha1_basic and interpret_branch_name. I guess we have spots\nthat don't take reflogs but do take branch names, but I think the code\nwould be much simpler if the syntax were parsed in one place, and then\nwe threw out or complained about bogus semantics (like \"checkout\n@{now}\").\n\n---\ndiff --git a/sha1_name.c b/sha1_name.c\nindex ed4c028..ef8f3fa 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -881,8 +881,28 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \n \tif (!len)\n \t\treturn len; /* syntax Ok, not enough switches */\n-\tif (0 < len)\n-\t\treturn len; /* consumed from the front */\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+\t\tstruct strbuf tmp = STRBUF_INIT;\n+\t\tint used = buf->len;\n+\t\tint ret;\n+\n+\t\tstrbuf_add(buf, name + len, namelen - len);\n+\t\tret = interpret_branch_name(buf->buf, &tmp);\n+\t\t/* that data was not interpreted, remove our cruft */\n+\t\tif (ret < 0) {\n+\t\t\tstrbuf_setlen(buf, used);\n+\t\t\treturn len;\n+\t\t}\n+\t\tstrbuf_reset(buf);\n+\t\tstrbuf_addbuf(buf, &tmp);\n+\t\tstrbuf_release(&tmp);\n+\t\t/* tweak for size of {-N} versus expanded ref name */\n+\t\treturn ret - used + len;\n+\t}\n+\n \tcp = strchr(name, '@');\n \tif (!cp)\n \t\treturn -1;\n"},{"id":"132822","messageId":"7viqanqt5s.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100127102417.GD6262@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T18:50:07Z","receivedAt":"2010-01-27T18:50:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> A minor nit, but wouldn't:\n>\n> int approxidate_careful(const char *str, unsigned long *out);\n>\n> returning an error code be the more usual pattern for a function with\n> error-plus-output (your approxidate wrapper would have to be a function then,\n> not a macro)?\n\nI don't have strong preference either way; the one in the patch was\nmodelled after setup_git_directory_gently(&nongit_ok), and slightly easier\nto work with for existing callers that don't care enough.\n\n>> @@ -413,8 +413,11 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n>> \t\t} else if (0 <= nth)\n>> \t\t\tat_time = 0;\n>> \t\telse {\n>> +\t\t\tint errors = 0;\n>> \t\t\tchar *tmp = xstrndup(str + at + 2, reflog_len);\n>> -\t\t\tat_time = approxidate(tmp);\n>> +\t\t\tat_time = approxidate_careful(tmp, &errors);\n>> +\t\t\tif (errors)\n>> +\t\t\t\tdie(\"Bogus timestamp '%s'\", tmp);\n>> \t\t\tfree(tmp);\n>\n> I was just going to \"return -1\" here, which yields:\n>\n> $ git show @{bogosity}\n> fatal: ambiguous argument '@{bogosity}': unknown revision or path not in the working tree.\n> Use '--' to separate paths from revisions\n>\n> instead of\n>\n> $ git show @{bogosity}\n> fatal: Bogus timestamp 'bogosity'\n\nGood point. Let's change it to silently return -1 and let the caller take\ncare of it. Perhaps there are some callers that say \"does this name an\nobject? If not, let's try pathname\".\n\n>> +test_expect_success '@{30.years.ago} shows old' '\n>> +\tcheck_at @{30.years.ago} one\n>\n> Side note: I chose this because we needed to go back from the current\n> time beyond where test_tick would place the commit. Which means this\n> test has a 2035 bug. :)\n\nCan't we use an absolute date, given that test_tick gives fixed timestamp\nsequence to pretend as if we were still in 2005 when we are running these\ntests?\n\n sha1_name.c | 4 ++--\n t/t0101-at-syntax.sh | 4 ++--\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex f4a74fe..04fb3b8 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -398,9 +398,9 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t\tint errors = 0;\n \t\t\tchar *tmp = xstrndup(str + at + 2, reflog_len);\n \t\t\tat_time = approxidate_careful(tmp, &errors);\n-\t\t\tif (errors)\n-\t\t\t\tdie(\"Bogus timestamp '%s'\", tmp);\n \t\t\tfree(tmp);\n+\t\t\tif (errors)\n+\t\t\t\treturn -1;\n \t\t}\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\ndiff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh\nindex ccabc37..5e298c5 100755\n--- a/t/t0101-at-syntax.sh\n+++ b/t/t0101-at-syntax.sh\n@@ -26,8 +26,8 @@ test_expect_success '@{now} shows current' '\n \tcheck_at @{now} two\n '\n \n-test_expect_success '@{30.years.ago} shows old' '\n-\tcheck_at @{30.years.ago} one\n+test_expect_success '@{2001-09-17} (before the first commit) shows old' '\n+\tcheck_at @{2001-09-17} one\n '\n \n test_expect_success 'silly approxidates work' '\n"},{"id":"132825","messageId":"7veilbqs82.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100127114009.GE6262@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-27T19:10:21Z","receivedAt":"2010-01-27T19:10:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Using \"git show @{-1}@{u}\" is still broken, though.\n>\n> I tried tracing the parsing through get_sha1_basic and\n> interpret_branch_name, but it's pretty confusing. Especially as we seem\n> to deal with @{upstream}, @{now}, and @{-1} at different places.\n>\n> I think the patch below does what we want, but the whole thing feels\n> overly complicated to me, especially with the split of parsing @{...}\n> between get_sha1_basic and interpret_branch_name. I guess we have spots\n> that don't take reflogs but do take branch names, but I think the code\n> would be much simpler if the syntax were parsed in one place, and then\n> we threw out or complained about bogus semantics (like \"checkout\n> @{now}\").\n\nI wanted to do something like what your patch does by iterating over the\ninput inside get_sha1_basic() while we still see @{...}, parsing pieces\nfrom the beginning, not from the end like the original \"do we have the\nreflog indicator at the end? If so strip it and deal with what we have at\nthe front\". Your patch to i-b-n does that by recursing inside, which is a\nnice solution.\n\nCare to roll a patch with additional tests, to build on top of 105e473\n(Fix log -g this@{upstream}, 2010-01-26)?\n"},{"id":"132868","messageId":"20100128085218.GE9071@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7viqanqt5s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-28T08:52:18Z","receivedAt":"2010-01-28T08:52:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 27, 2010 at 10:50:07AM -0800, Junio C Hamano wrote:\n\n> > A minor nit, but wouldn't:\n> >\n> > int approxidate_careful(const char *str, unsigned long *out);\n> >\n> > returning an error code be the more usual pattern for a function with\n> > error-plus-output (your approxidate wrapper would have to be a function then,\n> > not a macro)?\n> \n> I don't have strong preference either way; the one in the patch was\n> modelled after setup_git_directory_gently(&nongit_ok), and slightly easier\n> to work with for existing callers that don't care enough.\n\nLooks like you have already pushed out the original patch, so let's not\nworry about it.\n\n> >> +test_expect_success '@{30.years.ago} shows old' '\n> >> +\tcheck_at @{30.years.ago} one\n> >\n> > Side note: I chose this because we needed to go back from the current\n> > time beyond where test_tick would place the commit. Which means this\n> > test has a 2035 bug. :)\n> \n> Can't we use an absolute date, given that test_tick gives fixed timestamp\n> sequence to pretend as if we were still in 2005 when we are running these\n> tests?\n> [...]\n> --- a/t/t0101-at-syntax.sh\n> +++ b/t/t0101-at-syntax.sh\n> @@ -26,8 +26,8 @@ test_expect_success '@{now} shows current' '\n> \tcheck_at @{now} two\n> '\n> \n> -test_expect_success '@{30.years.ago} shows old' '\n> -\tcheck_at @{30.years.ago} one\n> +test_expect_success '@{2001-09-17} (before the first commit) shows old' '\n> +\tcheck_at @{2001-09-17} one\n> '\n> \n> test_expect_success 'silly approxidates work' '\n\nYes, I don't know why I was so concerned with using a relative\napproxidate when an absolute one would suffice. However, we should make\na matching change in the silly approxidate entry, too. Like this:\n\n-- >8 --\nSubject: [PATCH] t0101: use absolute date\n\nThe original version used relative approxidates, which don't\nreproduce as reliably as absolute ones. Commit 6c647a fixed\nthis for one case, but missed the \"silly\" case.\n\nSigned-off-by: Jeff King \n---\n t/t0101-at-syntax.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t0101-at-syntax.sh b/t/t0101-at-syntax.sh\nindex 5e298c5..a1998b5 100755\n--- a/t/t0101-at-syntax.sh\n+++ b/t/t0101-at-syntax.sh\n@@ -31,7 +31,7 @@ test_expect_success '@{2001-09-17} (before the first commit) shows old' '\n '\n \n test_expect_success 'silly approxidates work' '\n-\tcheck_at @{3.hot.dogs.and.30.years.ago} one\n+\tcheck_at @{3.hot.dogs.on.2001-09-17} one\n '\n \n test_expect_success 'notice misspelled upstream' '\n"},{"id":"132875","messageId":"20100128094446.GA14244@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7veilbqs82.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] t1506: more test for @{upstream} syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-28T09:44:46Z","receivedAt":"2010-01-28T09:44:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 27, 2010 at 11:10:21AM -0800, Junio C Hamano wrote:\n\n> I wanted to do something like what your patch does by iterating over the\n> input inside get_sha1_basic() while we still see @{...}, parsing pieces\n> from the beginning, not from the end like the original \"do we have the\n> reflog indicator at the end? If so strip it and deal with what we have at\n> the front\". Your patch to i-b-n does that by recursing inside, which is a\n> nice solution.\n\nYeah, I wanted to do that too, but it just ended up very messy. I\nsuppose the i-b-n solution is reasonably elegant, and it should\ncorrectly handle non-get-sha1 instances like:\n\n git checkout @{-1}@{u}\n\n> Care to roll a patch with additional tests, to build on top of 105e473\n> (Fix log -g this@{upstream}, 2010-01-26)?\n\nYep, series to follow:\n\n [1/3]: test combinations of @{} syntax\n [2/3]: fix parsing of @{-1}@{u} combination\n [3/3]: reject @{-1} not at beginning of object name\n\n-Peff\n"},{"id":"132876","messageId":"20100128095020.GA14253@coredump.intra.peff.net","threadId":"22172","inReplyTo":"20100128094446.GA14244@coredump.intra.peff.net","subject":"[PATCH 1/3] test combinations of @{} syntax","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-28T09:50:20Z","receivedAt":"2010-01-28T09:50:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that we have several different types of @{} syntax, it\nis a good idea to test them together, which reveals some\nfailures.\n\nSigned-off-by: Jeff King \n---\nWhile this is conceptually similar to the tests in t0101, it feels\nbetter to me to test features in combination only after they have been\ntested by themselves. Thus a new script > 1507.\n\n t/t1508-at-combinations.sh | 51 ++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 51 insertions(+), 0 deletions(-)\n create mode 100755 t/t1508-at-combinations.sh\n\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nnew file mode 100755\nindex 0000000..59f0463\n--- /dev/null\n+++ b/t/t1508-at-combinations.sh\n@@ -0,0 +1,51 @@\n+#!/bin/sh\n+\n+test_description='test various @{X} syntax combinations together'\n+. ./test-lib.sh\n+\n+check() {\n+test_expect_${3:-success} \"$1 = $2\" \"\n+\techo '$2' >expect &&\n+\tgit log -1 --format=%s '$1' >actual &&\n+\ttest_cmp expect actual\n+\"\n+}\n+nonsense() {\n+test_expect_${2:-success} \"$1 is nonsensical\" \"\n+\ttest_must_fail git log -1 '$1'\n+\"\n+}\n+fail() {\n+\t\"$@\" failure\n+}\n+\n+test_expect_success 'setup' '\n+\ttest_commit master-one &&\n+\ttest_commit master-two &&\n+\tgit checkout -b upstream-branch &&\n+\ttest_commit upstream-one &&\n+\ttest_commit upstream-two &&\n+\tgit checkout -b old-branch &&\n+\ttest_commit old-one &&\n+\ttest_commit old-two &&\n+\tgit checkout -b new-branch &&\n+\ttest_commit new-one &&\n+\ttest_commit new-two &&\n+\tgit config branch.old-branch.remote . &&\n+\tgit config branch.old-branch.merge refs/heads/master &&\n+\tgit config branch.new-branch.remote . &&\n+\tgit config branch.new-branch.merge refs/heads/upstream-branch\n+'\n+\n+check HEAD new-two\n+check \"@{1}\" new-one\n+check \"@{-1}\" old-two\n+check \"@{-1}@{1}\" old-one\n+check \"@{u}\" upstream-two\n+check \"@{u}@{1}\" upstream-one\n+fail check \"@{-1}@{u}\" master-two\n+fail check \"@{-1}@{u}@{1}\" master-one\n+fail nonsense \"@{u}@{-1}\"\n+nonsense \"@{1}@{u}\"\n+\n+test_done\n-- \n1.7.0.rc0.41.g538720\n"},{"id":"132877","messageId":"20100128095222.GB14253@coredump.intra.peff.net","threadId":"22172","inReplyTo":"20100128094446.GA14244@coredump.intra.peff.net","subject":"[PATCH 2/3] fix parsing of @{-1}@{u} combination","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-28T09:52:22Z","receivedAt":"2010-01-28T09:52:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Previously interpret_branch_name would see @{-1} and stop\nparsing, leaving the @{u} as cruft that provoked an error.\nInstead, we should recurse if there is more to parse.\n\nSigned-off-by: Jeff King \n---\nA straight repost of the previous \"how about this\" patch, but marking\nsuccessful tests.\n\n sha1_name.c | 24 ++++++++++++++++++++++--\n t/t1508-at-combinations.sh | 4 ++--\n 2 files changed, 24 insertions(+), 4 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex c7f1510..00fc415 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -881,8 +881,28 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \n \tif (!len)\n \t\treturn len; /* syntax Ok, not enough switches */\n-\tif (0 < len)\n-\t\treturn len; /* consumed from the front */\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+\t\tstruct strbuf tmp = STRBUF_INIT;\n+\t\tint used = buf->len;\n+\t\tint ret;\n+\n+\t\tstrbuf_add(buf, name + len, namelen - len);\n+\t\tret = interpret_branch_name(buf->buf, &tmp);\n+\t\t/* that data was not interpreted, remove our cruft */\n+\t\tif (ret < 0) {\n+\t\t\tstrbuf_setlen(buf, used);\n+\t\t\treturn len;\n+\t\t}\n+\t\tstrbuf_reset(buf);\n+\t\tstrbuf_addbuf(buf, &tmp);\n+\t\tstrbuf_release(&tmp);\n+\t\t/* tweak for size of {-N} versus expanded ref name */\n+\t\treturn ret - used + len;\n+\t}\n+\n \tcp = strchr(name, '@');\n \tif (!cp)\n \t\treturn -1;\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 59f0463..2a46af2 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -43,8 +43,8 @@ check \"@{-1}\" old-two\n check \"@{-1}@{1}\" old-one\n check \"@{u}\" upstream-two\n check \"@{u}@{1}\" upstream-one\n-fail check \"@{-1}@{u}\" master-two\n-fail check \"@{-1}@{u}@{1}\" master-one\n+check \"@{-1}@{u}\" master-two\n+check \"@{-1}@{u}@{1}\" master-one\n fail nonsense \"@{u}@{-1}\"\n nonsense \"@{1}@{u}\"\n \n-- \n1.7.0.rc0.41.g538720\n"},{"id":"132878","messageId":"20100128095643.GC14253@coredump.intra.peff.net","threadId":"22172","inReplyTo":"20100128094446.GA14244@coredump.intra.peff.net","subject":"[PATCH 3/3] reject @{-1} not at beginning of object name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-28T09:56:43Z","receivedAt":"2010-01-28T09:56:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Something like foo@{-1} is nonsensical, as the @{-N} syntax\nis reserved for \"the Nth last branch\", and is not an actual\nreflog selector. We should not feed such nonsense to\napproxidate at all.\n\nSigned-off-by: Jeff King \n---\nWe didn't discuss this one, but I came across it while trying to be\ncomplete in testing the combinations. Right now \"foo@{-1}\" is\ninterpreted as a reflog entry at approxidate \"-1\". Approxidate doesn't\nsignal an error because it thinks it has found something useful. But\nAFAIK we have declared all @{-...} to be Nth last branch, so it is\nsimply a semantic error.\n\nLet me know if that is not the case (that is, if it was intentional to\nleave foo@{-1} as the reflog at date \"-1\" because it has some meaning\nthat I am missing) and we can drop this patch.\n\n sha1_name.c | 4 ++++\n t/t1508-at-combinations.sh | 2 +-\n 2 files changed, 5 insertions(+), 1 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 00fc415..7729925 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -399,6 +399,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\tunsigned long co_time;\n \t\tint co_tz, co_cnt;\n \n+\t\t/* a @{-N} placed anywhere except the start is an error */\n+\t\tif (str[at+2] == '-')\n+\t\t\treturn -1;\n+\n \t\t/* Is it asking for N-th entry, or approxidate? */\n \t\tfor (i = nth = 0; 0 <= nth && i < reflog_len; i++) {\n \t\t\tchar ch = str[at+2+i];\ndiff --git a/t/t1508-at-combinations.sh b/t/t1508-at-combinations.sh\nindex 2a46af2..d5d6244 100755\n--- a/t/t1508-at-combinations.sh\n+++ b/t/t1508-at-combinations.sh\n@@ -45,7 +45,7 @@ check \"@{u}\" upstream-two\n check \"@{u}@{1}\" upstream-one\n check \"@{-1}@{u}\" master-two\n check \"@{-1}@{u}@{1}\" master-one\n-fail nonsense \"@{u}@{-1}\"\n+nonsense \"@{u}@{-1}\"\n nonsense \"@{1}@{u}\"\n \n test_done\n-- \n1.7.0.rc0.41.g538720\n"},{"id":"132903","messageId":"7vsk9qknf6.fsf@alter.siamese.dyndns.org","threadId":"22172","inReplyTo":"20100128095643.GC14253@coredump.intra.peff.net","subject":"Re: [PATCH 3/3] reject @{-1} not at beginning of object name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-28T20:02:53Z","receivedAt":"2010-01-28T20:02:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Something like foo@{-1} is nonsensical, as the @{-N} syntax\n> is reserved for \"the Nth last branch\", and is not an actual\n> reflog selector. We should not feed such nonsense to\n> approxidate at all.\n>\n> Signed-off-by: Jeff King \n> ---\n> We didn't discuss this one, but I came across it while trying to be\n> complete in testing the combinations. Right now \"foo@{-1}\" is\n> interpreted as a reflog entry at approxidate \"-1\". Approxidate doesn't\n> signal an error because it thinks it has found something useful. But\n> AFAIK we have declared all @{-...} to be Nth last branch, so it is\n> simply a semantic error.\n>\n> Let me know if that is not the case (that is, if it was intentional to\n> leave foo@{-1} as the reflog at date \"-1\" because it has some meaning\n> that I am missing) and we can drop this patch.\n\nI think the patch is fine as is.\n\nWe might want to use @{-some string that has non digit} for other purposes\nand it may be a safer change to tweak the \"do we only have digits\" check\nin the post-context to detect and reject only @{-}.\n\nBut what I am puzzled by the code structure of get_sha1_basic(), which\nlooks like this:\n\n get_sha1_basic() {\n\n - do we have @{...} at end? If so, and if it is not a magic like\n @{u}, set \"len\" (points at the end of stuff that should name a ref)\n and \"reflog_len\" (the length of the reflog time/num specifier that\n is applied to that ref).\n\n - did we find @{...} in the above check, and is it at the beginning?\n Then it is not a reflog syntax, but is a N-th branch switch.\n Substitute that @{...} with the real refname and retry. If it is\n not @{-N}, then that @{...} reflog derefence should apply to the\n current branch, so set it to real_ref and go to \"reflog\" part.\n\n - if we have @{...} at the end, get the canonical name of the ref the\n reflog notation is applied to.\n\n - otherwise, get the canonical name of the ref; in this case,\n there is no @{...} at the end, si this is what is eventually\n returned.\n\n - \"reflog\" part:\n by now, real_ref holds the ref @{...} is being applied to. Read\n from its reflog.\n\n }\n\nAnd the place that parses @{-1} and @{u} are different, even though both\ndwim_log() called by the third one and dwim_ref() called by the fourth one\ncall substitute_branch_name() and they are perfectly capable of resolving\n@{-1} and @{u} (and even nested stuff like @{-1}@{u}@{u} with your patch).\n\nBut somehow we kept the special case code to parse @{-1} in the second\none.\n\n Side note. I am wondering if dwim_log()'s current implementation is\n even correct in the first place. When you have two \"ambiguous\" refs,\n it appears to me that you will get a warning from dwim_ref(), but if\n only one of them has a reflog associated with it, dwim_log() won't\n complain. Why isn't the function be (1) dwim_ref() to find the ref\n from abbreviated refname given in str; and then (2) check if the log\n exists for that ref?\n\nIt might be cleaner if the logic went like this instead:\n\n - find the last @{...} in the string that is not what i-b-n should\n resolve (i.e. @{-1} and @{u}); that is @{time/num} reflog\n reference. You can have at most one reflog reference and it always\n has to come at the end.\n\n - feed the remainder to (an updated) i-b-n that knows how to grok:\n\n - @{-N} is nth-priour checkout; it has to come at the beginning and\n you can have at most one. If you find it, substitute it with the\n real branch name and continue. (e.g. @{-1}@{u} becomes master@{u})\n\n - does it begin with @{...}? If not, the part before @{...} is the\n name of the ref (e.g. \"next\" in \"next@{u}@{u}\") the later magic\n sequence (e.g. \"@{u}@{u}\") is applied to; otherwise apply the\n magic to what HEAD points at (e.g. \"@{u}\" applies @{u} to the\n current branch). Remember that ref and strip it away from the\n input.\n\n - while we see sequence of @{...}, apply the magic to the ref\n repeatedly (e.g. \"next@{u}@{u}\" has remembered \"refs/heads/next\" in\n the previous step, and @{u} is applied to produce \"master\" if next\n follows master, and then applying @{u} to that result will tell us\n that it follows \"refs/remotes/origin/master\").\n\n - now we have what ref the caller was talking about with the beginning\n part of the input (i.e. without \"@{time/num}\" at the end). If we\n had @{time/num} in the original input, open the reflog and find an\n appropriate entry in it. Otherwise what we received from (the\n updated) i-b-n is what we found. Find out what object the ref\n points at.\n\nBut that is a kind of code churn that may not be worth doing. I dunno.\n"},{"id":"132951","messageId":"20100129112219.GC6165@coredump.intra.peff.net","threadId":"22172","inReplyTo":"7vsk9qknf6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] reject @{-1} not at beginning of object name","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2010-01-29T11:22:19Z","receivedAt":"2010-01-29T11:22:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 28, 2010 at 12:02:53PM -0800, Junio C Hamano wrote:\n\n> We might want to use @{-some string that has non digit} for other purposes\n> and it may be a safer change to tweak the \"do we only have digits\" check\n> in the post-context to detect and reject only @{-}.\n\nI considered that, but I didn't think it was really worth it. If we\nlater want to make @{-foobar} meaningful, we can loosen the safety check\nthen.\n\n> But what I am puzzled by the code structure of get_sha1_basic(), which\n> looks like this:\n\nYou are not the only one who is puzzled. :)\n\nBut yes, your analysis of what is there now looks right to me.\n\n> And the place that parses @{-1} and @{u} are different, even though both\n> dwim_log() called by the third one and dwim_ref() called by the fourth one\n> call substitute_branch_name() and they are perfectly capable of resolving\n> @{-1} and @{u} (and even nested stuff like @{-1}@{u}@{u} with your patch).\n\nOoh, gross. I didn't try @{u}@{u} in my tests, but it should work.\n\n> Side note. I am wondering if dwim_log()'s current implementation is\n> even correct in the first place. When you have two \"ambiguous\" refs,\n> it appears to me that you will get a warning from dwim_ref(), but if\n> only one of them has a reflog associated with it, dwim_log() won't\n> complain. Why isn't the function be (1) dwim_ref() to find the ref\n> from abbreviated refname given in str; and then (2) check if the log\n> exists for that ref?\n\nI guess the original rationale was that you might have reflog'd one, so\nby asking for \"foo@{yesterday}\" you are disambiguating as \"the one with\na reflog\". But that seems kind of useless to me since:\n\n 1. It is somewhat error-prone, as it assumes that from the user's\n perspective, the fact that one ref has a log and the other does not\n is somehow a meaningful disambiguation. Which implies that users\n carefully figure out which refs have reflogs and which do not, and\n I don't think that is true.\n\n 2. For quite a while, we have had logallrefupdates on by default (and\n I don't remember the exact semantics before that, but wasn't it\n enough to simply create a \"logs\" directory, which meant that you\n either logged everything or nothing?). So I don't even know how you\n would get into a situation where one ref has a log and the other\n does not.\n\nIn other words, I totally agree with your statement, and we could\nprobably just drop the dwim_log code.\n\n> It might be cleaner if the logic went like this instead:\n\nYour logic makes sense to me. I think we could also simply do a\nleft-to-right parse, eating refs, @{-N}, and @{u} as we go and\nconverting them into a \"real ref\". If we get to something else, we stop.\nIf we have a @{} left, it's a reflog. Otherwise, it's bogus (I think ^\nsuffixes and such have already been stripped off at this point).\n\nInterpret_branch_name already does most of the \"eating...\" part above\n(and it needs to remain separate from get_sha1_basic, as things like\n\"checkout\" need to use it directly).\n\nBut I didn't really look too hard at it, as:\n\n> But that is a kind of code churn that may not be worth doing. I dunno.\n\nYeah, the code was sufficiently nasty and sufficiently core that I\ndidn't really want to risk breaking it for the sake of cleanup\n(especially not during the -rc cycle).\n\n-Peff\n"}]}