Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()

[Felipe Contreras <felipe.contreras@gmail.com>]

On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@gmail.com> wrote:
> Presently, the logic for @-parsing is horribly convoluted.  Prove that
> it is very straightforward by laying out the three cases (@{N},
> @{u[upstream], and @{-N}) and explaining how each case should be
> handled in comments.  All tests pass, and no functional changes have
> been introduced.

> @@ -463,9 +463,26 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)
>                  */
>                 for (at = len - 4; at >= 0; at--) {
>                         if (str[at] == '@' && str[at+1] == '{') {
> +                               /* Set reflog_len only if we
> +                                * don't see a @{u[pstream]}.  The
> +                                * @{N} and @{-N} forms have to do
> +                                * with reflog digging.
> +                                */
> +
> +                               /* Setting len to at means that we are
> +                                * only going to process the part
> +                                * before the @ until we reach "if
> +                                * (reflog)" at the end of the
> +                                * function.  That is only applicable
> +                                * in the @{N} case; in the @{-N} and
> +                                * @{u[pstream]} cases, we will run it
> +                                * through interpret_branch_name().
> +                                */
> +

Overkill.

/* set reflog_len when using the form: @{N} */

> @@ -476,22 +493,34 @@ 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) {
> -               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;
> +       if (reflog_len) {
> +               if (!len)
> +                       /* In the @{N} case where there's nothing
> +                        * before the @.
> +                        */

I think !len makes it clear.

> +                       refs_found = dwim_log("HEAD", 4, sha1, &real_ref);
> +               else {
> +                       /* The @{N} case where there is something
> +                        * before the @ for dwim_log to figure out,
> +                        * and the @{-N} case.
> +                        */

I think 'else' makes it clear.

> +                       refs_found = dwim_log(str, len, sha1, &real_ref);
> +
> +                       if (!refs_found && str[2] == '-') {

You are changing the behavior, there's no need for that in this
reorganization patch.

> +                               /* The @{-N} case that resolves to a
> +                                * detached HEAD (ie. not a ref)
> +                                */

This is not true, it resolves to a ref.

git rev-parse --symbolic-full-name @{-1}

Also, you removed a useful comment:

/* try the @{-N} syntax for n-th checkout */

> +                               struct strbuf buf = STRBUF_INIT;
> +                               if (interpret_branch_name(str, &buf) > 0) {
> +                                       get_sha1_hex(buf.buf, sha1);
> +                                       refs_found = 1;
> +                                       reflog_len = 0;
> +                               }
> +                               strbuf_release(&buf);

I'm pretty sure this is doing something totally different now. This is
certainly not "no functional changes".

> +                       }
>                 }
> -               /* allow "@{...}" to mean the current branch reflog */
> -               refs_found = dwim_ref("HEAD", 4, sha1, &real_ref);
> -       } else if (reflog_len)
> -               refs_found = dwim_log(str, len, sha1, &real_ref);
> -       else
> +       } else
> +               /* The @{u[pstream]} case */

It's not true, there might not be any @{u} in there.

Some of these changes might be good, but I would do them truly without
introducing functional changes, without removing useful comments, and
without adding paragraphs of explanation for what you are doing.

With the principle of self-documenting code, if you need paragraphs to
explain what you are doing, you already lost.

Cheers.

-- 
Felipe Contreras