Re: [PATCH 3/5] sha1_name.c: simplify @-parsing in get_sha1_basic()
- From
Felipe Contreras <felipe.contreras@gmail.com>
- Date
- May 1, 2013, 18:09 UTC
- Message-ID
- <CAMP44s1tHC+i+Wug_UuPnprZNvaPgLMNBX9MZi49SFv4iO62SQ@mail.gmail.com>
- In-Reply-To
- <1367425235-14998-4-git-send-email-artagnon@gmail.com>
On Wed, May 1, 2013 at 11:20 AM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:
Show 5 quoted lines
> 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.Show 20 quoted lines
> @@ -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} */Show 19 quoted lines
> @@ -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.
Show 6 quoted lines
> + 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 */Show 7 quoted lines
> + 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".
Show 9 quoted lines
> + }
> }
> - /* 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