From: Junio C Hamano <junkio@cox•net>
To: Andy Parkins <andyparkins@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH] Change "refs/" references to symbolic constants
Date: Mon, 19 Feb 2007 12:07:30 -0800 [thread overview]
Message-ID: <7vlkit7vy5.fsf@assigned-by-dhcp.cox.net> (raw)
In-Reply-To: <200702191839.05784.andyparkins@gmail.com> (Andy Parkins's message of "Mon, 19 Feb 2007 18:39:05 +0000")
Andy Parkins <andyparkins@gmail•com> writes:
> Changed repeated use of the same constants for the ref paths to be
> symbolic constants. I've defined them in refs.h
>
> refs/ is now PATH_REFS
> refs/heads/ is now PATH_REFS_HEADS
> refs/tags/ is now PATH_REFS_TAGS
> refs/remotes/ is now PATH_REFS_REMOTES
Your example:
> ... This has clarified the code in some places; for
> example:
>
> - len = strlen(refs[i]) + 11;
> + len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;
shows that you've carefully looked at what the code does,
instead of mindlessly replacing, which is a very good sign, but
how much testing has this seen, I wonder.
> diff --git a/builtin-describe.c b/builtin-describe.c
> index bcc6456..0f78363 100644
> --- a/builtin-describe.c
> +++ b/builtin-describe.c
> @@ -52,7 +52,7 @@ static int get_name(const char *path, const unsigned char *sha1, int flag, void
> * If --tags, then any tags are used.
> * Otherwise only annotated tags are used.
> */
> - if (!strncmp(path, "refs/tags/", 10)) {
> + if (!strncmp(path, PATH_TAGS, STRLEN_PATH_TAGS)) {
> if (object->type == OBJ_TAG)
> prio = 2;
> else
This is PATH_REFS_TAGS isn't it?
> @@ -231,7 +232,7 @@ static int create_default_files(const char *git_dir, const char *template_path)
> strcpy(path + len, "HEAD");
> reinit = !read_ref("HEAD", sha1);
> if (!reinit) {
> - if (create_symref("HEAD", "refs/heads/master", NULL) < 0)
> + if (create_symref("HEAD", PATH_REFS_HEADS "master", NULL) < 0)
> exit(1);
> }
>
I mildly mind this one, as it hurts grep-ability. I know this is one
of the the only two places in git that 'master' branch is treated
specially (and I think we would like to keep it that way --- that's
why I want to be able to grep for "refs/heads/master" and see very few
hits), so introducing PATH_REFS_HEADS_MASTER is probably not very
productive either, but... hmmmm.
> diff --git a/builtin-pack-refs.c b/builtin-pack-refs.c
> index 3de9b3e..ac7543d 100644
> --- a/builtin-pack-refs.c
> +++ b/builtin-pack-refs.c
> @@ -36,7 +36,7 @@ static int handle_one_ref(const char *path, const unsigned char *sha1,
> /* Do not pack the symbolic refs */
> if ((flags & REF_ISSYMREF))
> return 0;
> - is_tag_ref = !strncmp(path, "refs/tags/", 10);
> + is_tag_ref = !strncmp(path, PATH_REFS_TAGS, STRLEN_PATH_REFS_TAGS);
These repeated strncmp(p, X, STRLEN_X) almost makes me wonder if we
want to introduce:
inline int prefixcmp(a, b)
{
return (strncmp(a, b, strlen(b));
}
with clever preprocessor optimization to have compiler do strlen()
when b is a string literal.
next prev parent reply other threads:[~2007-02-19 20:07 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-02-19 18:39 [PATCH] Change "refs/" references to symbolic constants Andy Parkins
2007-02-19 18:55 ` Bill Lear
2007-02-19 20:01 ` [PATCH] Replace literal STRLEN_ #defines in refs.h with compiler evaluated expressions Andy Parkins
2007-02-19 20:50 ` [PATCH] Change "refs/" references to symbolic constants Krzysztof Halasa
2007-02-19 20:56 ` Shawn O. Pearce
2007-02-19 20:07 ` Junio C Hamano [this message]
2007-02-19 20:12 ` Shawn O. Pearce
2007-02-19 22:08 ` Johannes Schindelin
2007-02-20 8:41 ` Andy Parkins
2007-02-20 9:04 ` Junio C Hamano
2007-02-20 9:42 ` Andy Parkins
2007-02-20 9:50 ` Junio C Hamano
2007-02-20 10:21 ` Andy Parkins
2007-02-20 10:30 ` Junio C Hamano
2007-02-20 10:57 ` Andy Parkins
2007-02-20 11:37 ` Johannes Schindelin
2007-02-20 12:24 ` Simon 'corecode' Schubert
2007-02-20 13:26 ` Johannes Schindelin
2007-02-20 13:26 ` Andy Parkins
2007-02-20 15:46 ` Nicolas Pitre
2007-02-20 9:51 ` [PATCH 1/4] Add prefixcmp() Junio C Hamano
2007-02-20 10:04 ` David Kågedal
2007-02-20 10:20 ` Junio C Hamano
2007-02-20 9:53 ` [PATCH 2/4] Mechanical conversion to use prefixcmp() Junio C Hamano
2007-02-20 10:19 ` Junio C Hamano
2007-02-20 11:53 ` Johannes Schindelin
2007-02-21 6:43 ` Junio C Hamano
2007-02-21 12:41 ` Johannes Schindelin
2007-02-20 9:54 ` [PATCH 3/4] prefixcmp(): fix-up mechanical conversion Junio C Hamano
2007-02-20 9:55 ` [PATCH 4/4] prefixcmp(): fix-up leftover strncmp() Junio C Hamano
-- strict thread matches above, loose matches on Subject: below --
2007-10-02 15:58 [PATCH 1/2] Change "refs/" references to symbolic constants Jeff King
2007-10-02 18:16 ` [PATCH] " Andy Parkins
2007-10-02 19:11 ` Jeff King
2007-10-02 19:47 ` Junio C Hamano
2007-10-02 20:48 ` Jeff King
2007-10-03 0:22 ` Junio C Hamano
2007-10-03 2:58 ` Jeff King
2007-10-03 4:05 ` Johannes Schindelin
2007-10-03 4:30 ` Jeff King
2007-10-03 11:30 ` Andreas Ericsson
2007-10-03 7:37 ` Andy Parkins
2007-10-03 7:50 ` Andy Parkins
2007-10-03 11:13 ` Andy Parkins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vlkit7vy5.fsf@assigned-by-dhcp.cox.net \
--to=junkio@cox$(echo .)net \
--cc=andyparkins@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox