On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra
<artagnon@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html