Re: [PATCH v2] sha1_name: reorganize get_sha1_basic()
- From
Ramkumar Ramachandra <artagnon@gmail.com>
- Date
- May 2, 2013, 18:55 UTC
- Message-ID
- <CALkWK0k4YisL0Nc6906FRsx7556UVqy_DD9Wo=1j4OWMkwqK-A@mail.gmail.com>
- In-Reply-To
- <1367516919-4735-1-git-send-email-felipe.contreras@gmail.com>
Felipe Contreras wrote:
> [...]
Okay, you used nth_prior in this one.
> There is no need to call this function recursively with the branch of
> @{-N} substituted because dwim_{ref,log} already replaces it.I figured that the recursion is because dwim_{ref,log} didn't exist when this was written.
Show 5 quoted lines
> However, there's one corner-case where @{-N} resolves to a detached
> HEAD, in which case we wouldn't get any ref back.
>
> So we parse the nth-prior manually, and deal with it depending on
> weather it's a SHA-1, or a ref.Right. _This_ is the special case, which the old logic didn't quite convey. The end-user version of this is: 'git checkout -' won't bring you back to the branch if you said git checkout HEAD~1 earlier.
Show 9 quoted lines
> diff --git a/sha1_name.c b/sha1_name.c > index 3820f28..6428001 100644 > --- a/sha1_name.c > +++ b/sha1_name.c > @@ -431,13 +431,14 @@ static inline int upstream_mark(const char *string, int len) > } > > static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned lookup_flags); > +static int interpret_nth_prior_checkout(const char *name, struct strbuf *buf);
It didn't strike me to use interpret_nth_prior_checkout() directly. I was still stuck at interpret_branch_name() returning a positive value.
Show 18 quoted lines
> static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
> {
> static const char *warn_msg = "refname '%.*s' is ambiguous.";
> char *real_ref = NULL;
> int refs_found = 0;
> - int at, reflog_len;
> + int at, reflog_len, nth_prior = 0;
>
> if (len == 40 && !get_sha1_hex(str, sha1))
> return 0;
> @@ -447,6 +448,10 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
> if (len && str[len-1] == '}') {
> for (at = len-2; at >= 0; at--) {
> if (str[at] == '@' && str[at+1] == '{') {
> + if (at == 0 && str[2] == '-') {
> + nth_prior = 1;
> + continue;
> + }Looking at this closely once again. You've already hit the beginning. What are you continuing? Take the example of a compound expression with @{-
@{-1}@{0}
^ at is here
"@{-" is not matched@{-1}@{0} ^ at is here "@{-" is matched What's to continue? at is already at 0
On another note, I think you've fixed a bug: @{-1}{0} was parsing to the same value as @{-1}@{0} before your patch.
Show 6 quoted lines
> @@ -460,20 +465,22 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
> if (len && ambiguous_path(str, len))
> return -1;
>
> - if (!len && reflog_len) {
> + if (nth_prior) {nth_prior makes this much cleaner overall.
Show 16 quoted lines
> struct strbuf buf = STRBUF_INIT;
> - int ret;
> - /* try the @{-N} syntax for n-th checkout */
> - ret = interpret_branch_name(str+at, &buf);
> - if (ret > 0) {
> - /* substitute this branch name and restart */
> - return get_sha1_1(buf.buf, buf.len, sha1, 0);
> - } else if (ret == 0) {
> - return -1;
> + int detached;
> +
> + if (interpret_nth_prior_checkout(str, &buf) > 0) {
> + detached = (buf.len == 40 && !get_sha1_hex(buf.buf, sha1));
> + strbuf_release(&buf);
> + if (detached)
> + return 0;Neat. I'd set reflog_len to zero and made sure that the last part of the function wouldn't be executed. How did you get away without setting refs_found to 1 though?
Show 6 quoted lines
> }
> + }
> +
> + if (!len && reflog_len)
> /* allow "@{...}" to mean the current branch reflog */
> refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);I got this part wrong too: I said dwim_log() instead of dwim_ref().
Show 8 quoted lines
> - } else if (reflog_len) > + else if (reflog_len) > refs_found = dwim_log(str, len, sha1, &real_ref); > else > refs_found = dwim_ref(str, len, sha1, &real_ref); > -- > 1.8.3.rc0.401.g45bba44 >