Change "refs/" references to symbolic constants
PATCH 1/2 Change "refs/" references to symbolic constants
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
I've changed all references to them and made constants for the string lengths as well. This has clarified the code in some places; for example:
- len = strlen(refs[i]) + 11;
+ len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;
In this case 11 isn't STRLEN_PATH_REFS_HEADS, as it is in most other cases, it's TAGS + 1. With the change to symbolic constants it's much clearer what is happening.
---
The primary purpose of this patch is to clarify the string math that's done whenever "refs/" paths are used. As with the HASH_WIDTH patch I sent, when we're dealing with lines like:
url = xmalloc(strlen(repo->base) + 64);
It's much easier to tell where that 64 came from when it says:
url = xmalloc(strlen(repo->base) + STRLEN_PATH_OBJECTS + HASH_WIDTH_ASCII
+ 16);
There is more to be done really because that 16 is really
STRLEN_PATH_PACK + STRLEN_PACK_PREFIX + STRLEN_PACK_INDEX_EXTENSION
but I think that would perhaps be a bridge too far.
builtin-branch.c | 28 ++++++++++++++-------------- builtin-describe.c | 2 +- builtin-fetch--tool.c | 10 +++++----- builtin-fmt-merge-msg.c | 5 +++-- builtin-fsck.c | 4 ++-- builtin-init-db.c | 15 ++++++++------- builtin-name-rev.c | 14 +++++++------- builtin-pack-refs.c | 2 +- builtin-push.c | 6 +++--- builtin-show-branch.c | 34 +++++++++++++++++----------------- builtin-show-ref.c | 6 +++--- builtin-tag.c | 4 ++-- connect.c | 10 +++++----- fetch-pack.c | 6 +++--- http-fetch.c | 31 ++++++++++++++++--------------- http-push.c | 42 +++++++++++++++++++++--------------------- local-fetch.c | 13 +++++++------ path.c | 5 +++-- receive-pack.c | 4 ++-- reflog-walk.c | 6 +++--- refs.c | 18 +++++++++--------- refs.h | 17 +++++++++++++++++ remote.c | 14 +++++++------- setup.c | 5 +++-- sha1_name.c | 10 +++++----- wt-status.c | 5 +++-- 26 files changed, 170 insertions(+), 146 deletions(-)
builtin-branch.c +14 -14
@@ -92,12 +92,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)switch (kinds) {case REF_REMOTE_BRANCH : - fmt ="refs/remotes/%s" ; + fmt =PATH_REFS_REMOTES "%s" ; remote ="remote " ; force =1 ;break ;case REF_LOCAL_BRANCH : - fmt ="refs/heads/%s" ; + fmt =PATH_REFS_HEADS "%s" ; remote ="" ;break ;default :
@@ -189,15 +189,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,int len;/* Detect kind */ -if (!prefixcmp (refname,"refs/heads/" )) { +if (!prefixcmp (refname,PATH_REFS_HEADS )) { kind =REF_LOCAL_BRANCH ; - refname +=11 ; - }else if (!prefixcmp (refname,"refs/remotes/" )) { + refname +=STRLEN_PATH_REFS_HEADS ; + }else if (!prefixcmp (refname,PATH_REFS_REMOTES )) { kind =REF_REMOTE_BRANCH ; - refname +=13 ; - }else if (!prefixcmp (refname,"refs/tags/" )) { + refname +=STRLEN_PATH_REFS_REMOTES ; + }else if (!prefixcmp (refname,PATH_REFS_TAGS )) { kind =REF_TAG ; - refname +=10 ; + refname +=STRLEN_PATH_REFS_TAGS ; }/* Don't add types the caller doesn't want */
@@ -400,7 +400,7 @@ static void create_branch(const char *name, const char *start_name,char *real_ref, ref[PATH_MAX ], msg[PATH_MAX +20 ];int forcing =0 ; -snprintf (ref,sizeof ref,"refs/heads/%s" , name); +snprintf (ref,sizeof ref,PATH_REFS_HEADS "%s" , name);if (check_ref_format (ref))die ("'%s' is not a valid branch name." , name);
@@ -469,13 +469,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)if (!oldname)die ("cannot rename the current branch while not on any." ); -if (snprintf (oldref,sizeof (oldref),"refs/heads/%s" , oldname) >sizeof (oldref)) +if (snprintf (oldref,sizeof (oldref),PATH_REFS_HEADS "%s" , oldname) >sizeof (oldref))die ("Old branchname too long" );if (check_ref_format (oldref))die ("Invalid branch name: %s" , oldref); -if (snprintf (newref,sizeof (newref),"refs/heads/%s" , newname) >sizeof (newref)) +if (snprintf (newref,sizeof (newref),PATH_REFS_HEADS "%s" , newname) >sizeof (newref))die ("New branchname too long" );if (check_ref_format (newref))
@@ -602,9 +602,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) detached =1 ; }else { -if (prefixcmp (head,"refs/heads/" )) -die ("HEAD not found below refs/heads!" ); - head +=11 ; +if (prefixcmp (head,PATH_REFS_HEADS )) +die ("HEAD not found below " PATH_REFS_HEADS "!" ); + head +=STRLEN_PATH_REFS_HEADS ; }if (delete)
builtin-describe.c +1 -1
@@ -53,7 +53,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 (!prefixcmp (path,"refs/tags/" )) { +if (!prefixcmp (path,PATH_TAGS )) {if (object->type ==OBJ_TAG ) prio =2 ;else
builtin-fetch--tool.c +5 -5
@@ -76,7 +76,7 @@ static int update_local_ref(const char *name,char *msg; just_store:/* new ref */ -if (!strncmp (name,"refs/tags/" ,10 )) +if (!prefixcmp (name,PATH_REFS_TAGS )) msg ="storing tag" ;else msg ="storing head" ;
@@ -94,7 +94,7 @@ static int update_local_ref(const char *name,return 0 ; } -if (!strncmp (name,"refs/tags/" ,10 )) { +if (!prefixcmp (name,PATH_REFS_TAGS )) {fprintf (stderr,"* %s: updating with %s\n" , name, note);show_new (type, sha1_new);return update_ref_env ("updating tag" , name, sha1_new,NULL );
@@ -151,15 +151,15 @@ static int append_fetch_head(FILE *fp, kind ="" ; what ="" ; } -else if (!strncmp (remote_name,"refs/heads/" ,11 )) { +else if (!prefixcmp (remote_name,PATH_REFS_HEADS )) { kind ="branch" ; what = remote_name +11 ; } -else if (!strncmp (remote_name,"refs/tags/" ,10 )) { +else if (!prefixcmp (remote_name,PATH_REFS_TAGS )) { kind ="tag" ; what = remote_name +10 ; } -else if (!strncmp (remote_name,"refs/remotes/" ,13 )) { +else if (!prefixcmp (remote_name,PATH_REFS_REMOTES )) { kind ="remote branch" ; what = remote_name +13 ; }
builtin-fmt-merge-msg.c +3 -2
@@ -4,6 +4,7 @@#include "diff.h" #include "revision.h" #include "tag.h" +#include "refs.h" static const char *fmt_merge_msg_usage ="git-fmt-merge-msg [--summary] [--no-summary] [--file <file>]" ;
@@ -282,8 +283,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) current_branch =resolve_ref ("HEAD" , head_sha1,1 ,NULL );if (!current_branch)die ("No current branch" ); -if (!prefixcmp (current_branch,"refs/heads/" )) - current_branch +=11 ; +if (!prefixcmp (current_branch,PATH_REFS_HEADS )) + current_branch +=STRLEN_PATH_REFS_HEADS ;while (fgets (line,sizeof (line), in)) { i++;
builtin-fsck.c +2 -2
@@ -629,14 +629,14 @@ static int fsck_head_link(void)if (!strcmp (head_points_at,"HEAD" ))/* detached HEAD */ null_is_error =1 ; -else if (prefixcmp (head_points_at,"refs/heads/" )) +else if (prefixcmp (head_points_at,PATH_REFS_HEADS ))return error ("HEAD points to something strange (%s)" , head_points_at);if (is_null_sha1 (sha1)) {if (null_is_error)return error ("HEAD: detached HEAD points at nothing" );fprintf (stderr,"notice: HEAD points to an unborn branch (%s)\n" , - head_points_at +11 ); + head_points_at +STRLEN_PATH_REFS_HEADS ); }return 0 ; }
builtin-init-db.c +8 -7
@@ -5,6 +5,7 @@*/ #include "cache.h" #include "builtin.h" +#include "refs.h" #ifndef DEFAULT_GIT_TEMPLATE_DIR #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -194,11 +195,11 @@ static int create_default_files(const char *git_dir, const char *template_path)/* * Create .git/refs/{heads,tags} */ -strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );safe_create_dir (path,1 );/* First copy the templates -- we might have the default
@@ -217,11 +218,11 @@ static int create_default_files(const char *git_dir, const char *template_path)if (shared_repository) { path[len] =0 ;adjust_shared_perm (path); -strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );adjust_shared_perm (path); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );adjust_shared_perm (path); -strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );adjust_shared_perm (path); }
@@ -232,7 +233,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 ); }
builtin-name-rev.c +7 -7
@@ -96,7 +96,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, voidstruct name_ref_data *data = cb_data;int deref =0 ; -if (data->tags_only &&prefixcmp (path,"refs/tags/" )) +if (data->tags_only &&prefixcmp (path,PATH_REFS_TAGS ))return 0 ;if (data->ref_filter &&fnmatch (data->ref_filter, path,0 ))
@@ -112,14 +112,14 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, voidif (o && o->type ==OBJ_COMMIT ) {struct commit *commit = (struct commit *)o; -if (!prefixcmp (path,"refs/heads/" )) - path = path +11 ; +if (!prefixcmp (path,PATH_REFS_HEADS )) + path = path +STRLEN_PATH_REFS_HEADS ;else if (data->tags_only && data->name_only - && !prefixcmp (path,"refs/tags/" )) - path = path +10 ; -else if (!prefixcmp (path,"refs/" )) - path = path +5 ; + && !prefixcmp (path,PATH_REFS_TAGS )) + path = path +STRLEN_PATH_REFS_TAGS ; +else if (!prefixcmp (path,PATH_REFS )) + path = path +STRLEN_PATH_REFS ;name_rev (commit,xstrdup (path),0 ,0 , deref); }
builtin-pack-refs.c +1 -1
@@ -39,7 +39,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 = !prefixcmp (path,"refs/tags/" ); + is_tag_ref = !prefixcmp (path,PATH_REFS_TAGS );/* ALWAYS pack refs that were already packed or are tags */ if (!(cb->flags &PACK_REFS_ALL ) && !is_tag_ref && !(flags &REF_ISPACKED ))
builtin-push.c +3 -3
@@ -33,9 +33,9 @@ static void set_refspecs(const char **refs, int nr)int len;if (nr <= ++i)die ("tag shorthand without <tag>" ); - len =strlen (refs[i]) +11 ; + len =strlen (refs[i]) +STRLEN_PATH_REFS_TAGS +1 ; tag =xmalloc (len); -strcpy (tag,"refs/tags/" ); +strcpy (tag,PATH_REFS_TAGS );strcat (tag, refs[i]); ref = tag; }
@@ -148,7 +148,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)continue ; }if (!strcmp (arg,"--tags" )) { -add_refspec ("refs/tags/*" ); +add_refspec (PATH_REFS_TAGS "*" );continue ; }if (!strcmp (arg,"--force" ) || !strcmp (arg,"-f" )) {
builtin-show-branch.c +17 -17
@@ -380,36 +380,36 @@ static int append_ref(const char *refname, const unsigned char *sha1,static int append_head_ref (const char *refname,const unsigned char *sha1,int flag,void *cb_data) {unsigned char tmp[20 ]; -int ofs =11 ; -if (prefixcmp (refname,"refs/heads/" )) +int ofs =STRLEN_PATH_REFS_HEADS ; +if (prefixcmp (refname,PATH_REFS_HEADS ))return 0 ;/* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ if (get_sha1 (refname + ofs, tmp) ||hashcmp (tmp, sha1)) - ofs =5 ; + ofs =STRLEN_PATH_REFS ;return append_ref (refname + ofs, sha1,0 ); }static int append_remote_ref (const char *refname,const unsigned char *sha1,int flag,void *cb_data) {unsigned char tmp[20 ]; -int ofs =13 ; -if (prefixcmp (refname,"refs/remotes/" )) +int ofs =STRLEN_PATH_REFS_REMOTES ; +if (prefixcmp (refname,PATH_REFS_REMOTES ))return 0 ;/* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ if (get_sha1 (refname + ofs, tmp) ||hashcmp (tmp, sha1)) - ofs =5 ; + ofs =STRLEN_PATH_REFS ;return append_ref (refname + ofs, sha1,0 ); }static int append_tag_ref (const char *refname,const unsigned char *sha1,int flag,void *cb_data) { -if (prefixcmp (refname,"refs/tags/" )) +if (prefixcmp (refname,PATH_REFS_TAGS ))return 0 ; -return append_ref (refname +5 , sha1,0 ); +return append_ref (refname +STRLEN_PATH_REFS , sha1,0 ); }static const char *match_ref_pattern =NULL ;
@@ -438,9 +438,9 @@ static int append_matching_ref(const char *refname, const unsigned char *sha1, ireturn 0 ;if (fnmatch (match_ref_pattern, tail,0 ))return 0 ; -if (!prefixcmp (refname,"refs/heads/" )) +if (!prefixcmp (refname,PATH_REFS_HEADS ))return append_head_ref (refname, sha1, flag, cb_data); -if (!prefixcmp (refname,"refs/tags/" )) +if (!prefixcmp (refname,PATH_REFS_TAGS ))return append_tag_ref (refname, sha1, flag, cb_data);return append_ref (refname, sha1,0 ); }
@@ -465,12 +465,12 @@ static int rev_is_head(char *head, int headlen, char *name,if ((!head[0 ]) || (head_sha1 && sha1 &&hashcmp (head_sha1, sha1)))return 0 ; -if (!prefixcmp (head,"refs/heads/" )) - head +=11 ; -if (!prefixcmp (name,"refs/heads/" )) - name +=11 ; -else if (!prefixcmp (name,"heads/" )) - name +=6 ; +if (!prefixcmp (head,PATH_REFS_HEADS )) + head +=STRLEN_PATH_REFS_HEADS ; +if (!prefixcmp (name,PATH_REFS_HEADS )) + name +=STRLEN_PATH_REFS_HEADS ; +else if (!prefixcmp (name,PATH_HEADS )) + name +=STRLEN_PATH_HEADS ;return !strcmp (head, name); }
@@ -781,7 +781,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) has_head++; }if (!has_head) { -int pfxlen =strlen ("refs/heads/" ); +int pfxlen =STRLEN_PATH_REFS_HEADS ;append_one_rev (head + pfxlen); } }
builtin-show-ref.c +3 -3
@@ -29,8 +29,8 @@ static int show_ref(const char *refname, const unsigned char *sha1, int flag, voif (tags_only || heads_only) {int match; - match = heads_only && !prefixcmp (refname,"refs/heads/" ); - match |= tags_only && !prefixcmp (refname,"refs/tags/" ); + match = heads_only && !prefixcmp (refname,PATH_REFS_HEADS ); + match |= tags_only && !prefixcmp (refname,PATH_REFS_TAGS );if (!match)return 0 ; }
@@ -227,7 +227,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)while (*pattern) {unsigned char sha1[20 ]; -if (!prefixcmp (*pattern,"refs/" ) && +if (!prefixcmp (*pattern,PATH_REFS ) &&resolve_ref (*pattern, sha1,1 ,NULL )) {if (!quiet)show_one (*pattern, sha1);
builtin-tag.c +2 -2
@@ -146,7 +146,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)unsigned char sha1[20 ];for (p = argv; *p; p++) { -if (snprintf (ref,sizeof (ref),"refs/tags/%s" , *p) +if (snprintf (ref,sizeof (ref),PATH_REFS_TAGS "%s" , *p) >=sizeof (ref)) {error ("tag name too long: %.*s..." ,50 , *p); had_error =1 ;
@@ -440,7 +440,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)if (get_sha1 (object_ref, object))die ("Failed to resolve '%s' as a valid ref." , object_ref); -if (snprintf (ref,sizeof (ref),"refs/tags/%s" , tag) >sizeof (ref) -1 ) +if (snprintf (ref,sizeof (ref),PATH_REFS_TAGS "%s" , tag) >sizeof (ref) -1 )die ("tag name too long: %.*s..." ,50 , tag);if (check_ref_format (ref))die ("'%s' is not a valid tag name." , tag);
connect.c +5 -5
@@ -13,23 +13,23 @@ static int check_ref(const char *name, int len, unsigned int flags)if (!flags)return 1 ; -if (len <5 ||memcmp (name,"refs/" ,5 )) +if (len <STRLEN_PATH_REFS ||memcmp (name,PATH_REFS ,STRLEN_PATH_REFS ))return 0 ;/* Skip the "refs/" part */ - name +=5 ; - len -=5 ; + name +=STRLEN_PATH_REFS ; + len -=STRLEN_PATH_REFS ;/* REF_NORMAL means that we don't want the magic fake tag refs */ if ((flags &REF_NORMAL ) &&check_ref_format (name) <0 )return 0 ;/* REF_HEADS means that we want regular branch heads */ -if ((flags &REF_HEADS ) && !memcmp (name,"heads/" ,6 )) +if ((flags &REF_HEADS ) && !memcmp (name,PATH_HEADS ,STRLEN_PATH_HEADS ))return 1 ;/* REF_TAGS means that we want tags */ -if ((flags &REF_TAGS ) && !memcmp (name,"tags/" ,5 )) +if ((flags &REF_TAGS ) && !memcmp (name,PATH_TAGS ,STRLEN_PATH_TAGS ))return 1 ;/* All type bits clear means that we are ok with anything */
fetch-pack.c +3 -3
@@ -344,11 +344,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)for (ref = *refs; ref; ref = next) { next = ref->next; -if (!memcmp (ref->name,"refs/" ,5 ) && -check_ref_format (ref->name +5 )) +if (!memcmp (ref->name,PATH_REFS ,STRLEN_PATH_REFS ) && +check_ref_format (ref->name +STRLEN_PATH_REFS )) ;/* trash */ else if (fetch_all && - (!depth ||prefixcmp (ref->name,"refs/tags/" ) )) { + (!depth ||prefixcmp (ref->name,PATH_REFS_TAGS ) )) { *newtail = ref; ref->next =NULL ; newtail = &ref->next;
http-fetch.c +16 -15
@@ -3,6 +3,7 @@#include "pack.h" #include "fetch.h" #include "http.h" +#include "refs.h" #define PREV_BUF_SIZE 4096#define RANGE_HEADER_SIZE 30
@@ -157,11 +158,11 @@ static void start_object_request(struct object_request *obj_req)SHA1_Init (&obj_req->c); - url =xmalloc (strlen (obj_req->repo->base) +51 ); - obj_req->url =xmalloc (strlen (obj_req->repo->base) +51 ); + url =xmalloc (strlen (obj_req->repo->base) +STRLEN_PATH_OBJECTS +43 ); + obj_req->url =xmalloc (strlen (obj_req->repo->base) +STRLEN_PATH_OBJECTS +43 );strcpy (url, obj_req->repo->base); posn = url +strlen (obj_req->repo->base); -strcpy (posn,"/objects/" ); +strcpy (posn,"/" PATH_OBJECTS ); posn +=9 ;memcpy (posn, hex,2 ); posn +=2 ;
@@ -398,8 +399,8 @@ static int fetch_index(struct alt_base *repo, unsigned char *sha1)if (get_verbosely)fprintf (stderr,"Getting index for pack %s\n" , hex); - url =xmalloc (strlen (repo->base) +64 ); -sprintf (url,"%s/objects/pack/pack-%s.idx" , repo->base, hex); + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +56 ); +sprintf (url,"%s/" PATH_OBJECTS "pack/pack-%s.idx" , repo->base, hex); filename =sha1_pack_index_name (sha1);snprintf (tmpfile,sizeof (tmpfile),"%s.temp" , filename);
@@ -478,7 +479,7 @@ static void process_alternates_response(void *callback_data)/* Try reusing the slot to get non-http alternates */ alt_req->http_specific =0 ; -sprintf (alt_req->url,"%s/objects/info/alternates" , +sprintf (alt_req->url,"%s/" PATH_OBJECTS "info/alternates" , base);curl_easy_setopt (slot->curl,CURLOPT_URL , alt_req->url);
@@ -625,8 +626,8 @@ static void fetch_alternates(const char *base)if (get_verbosely)fprintf (stderr,"Getting alternates list for %s\n" , base); - url =xmalloc (strlen (base) +31 ); -sprintf (url,"%s/objects/info/http-alternates" , base); + url =xmalloc (strlen (base) +STRLEN_PATH_OBJECTS +23 ); +sprintf (url,"%s/" PATH_OBJECTS "info/http-alternates" , base);/* Use a callback to process the result, since another request may fail and need to have alternates loaded before continuing */
@@ -675,8 +676,8 @@ static int fetch_indices(struct alt_base *repo)if (get_verbosely)fprintf (stderr,"Getting pack list for %s\n" , repo->base); - url =xmalloc (strlen (repo->base) +21 ); -sprintf (url,"%s/objects/info/packs" , repo->base); + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +13 ); +sprintf (url,"%s/" PATH_OBJECTS "info/packs" , repo->base); slot =get_active_slot (); slot->results = &results;
@@ -757,8 +758,8 @@ static int fetch_pack(struct alt_base *repo, unsigned char *sha1) sha1_to_hex(sha1)); } - url =xmalloc (strlen (repo->base) +65 ); -sprintf (url,"%s/objects/pack/pack-%s.pack" , + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +57 ); +sprintf (url,"%s/" PATH_OBJECTS "pack/pack-%s.pack" , repo->base,sha1_to_hex (target->sha1)); filename =sha1_pack_name (target->sha1);
@@ -930,14 +931,14 @@ static char *quote_ref_url(const char *base, const char *ref)int len, baselen, ch; baselen =strlen (base); - len = baselen +7 ;/* "/refs/" + NUL */ + len = baselen +STRLEN_PATH_REFS +2 ;/* "/" + "refs/" + NUL */ for (cp = ref; (ch = *cp) !=0 ; cp++, len++)if (needs_quote (ch)) len +=2 ;/* extra two hex plus replacement % */ qref =xmalloc (len);memcpy (qref, base, baselen); -memcpy (qref + baselen,"/refs/" ,6 ); -for (cp = ref, dp = qref + baselen +6 ; (ch = *cp) !=0 ; cp++) { +memcpy (qref + baselen,PATH_REFS ,STRLEN_PATH_REFS ); +for (cp = ref, dp = qref + baselen +STRLEN_PATH_REFS ; (ch = *cp) !=0 ; cp++) {if (needs_quote (ch)) { *dp++ ='%' ; *dp++ =hex ((ch >>4 ) &0xF );
http-push.c +21 -21
114 lines not on this page · raw message
local-fetch.c +7 -6
@@ -4,6 +4,7 @@#include "cache.h" #include "commit.h" #include "fetch.h" +#include "refs.h" static int use_link;static int use_symlink;
@@ -23,7 +24,7 @@ static void setup_index(unsigned char *sha1)struct packed_git *new_pack;char filename[PATH_MAX ];strcpy (filename, path); -strcat (filename,"/objects/pack/pack-" ); +strcat (filename,"/" PATH_OBJECTS "pack/pack-" );strcat (filename,sha1_to_hex (sha1));strcat (filename,".idx" ); new_pack =parse_pack_index_file (sha1, filename);
@@ -37,7 +38,7 @@ static int setup_indices(void)struct dirent *de;char filename[PATH_MAX ];unsigned char sha1[20 ]; -sprintf (filename,"%s/objects/pack/" , path); +sprintf (filename,"%s/" PATH_OBJECTS "pack/" , path); dir =opendir (filename);if (!dir)return -1 ;
@@ -122,11 +123,11 @@ static int fetch_pack(const unsigned char *sha1)fprintf (stderr," which contains %s\n" ,sha1_to_hex (sha1)); } -sprintf (filename,"%s/objects/pack/pack-%s.pack" , +sprintf (filename,"%s/" PATH_OBJECTS "pack/pack-%s.pack" , path,sha1_to_hex (target->sha1));copy_file (filename,sha1_pack_name (target->sha1),sha1_to_hex (target->sha1),1 ); -sprintf (filename,"%s/objects/pack/pack-%s.idx" , +sprintf (filename,"%s/" PATH_OBJECTS "pack/pack-%s.idx" , path,sha1_to_hex (target->sha1));copy_file (filename,sha1_pack_index_name (target->sha1),sha1_to_hex (target->sha1),1 );
@@ -143,7 +144,7 @@ static int fetch_file(const unsigned char *sha1)if (object_name_start <0 ) {strcpy (filename, path);/* e.g. git.git */ -strcat (filename,"/objects/" ); +strcat (filename,"/" PATH_OBJECTS ); object_name_start =strlen (filename); } filename[object_name_start+0 ] = hex[0 ];
@@ -169,7 +170,7 @@ int fetch_ref(char *ref, unsigned char *sha1)int ifd;if (ref_name_start <0 ) { -sprintf (filename,"%s/refs/" , path); +sprintf (filename,"%s/" PATH_REFS , path); ref_name_start =strlen (filename); }strcpy (filename + ref_name_start, ref);
path.c +3 -2
@@ -11,6 +11,7 @@* which is what it's designed for. */ #include "cache.h" +#include "refs.h" static char bad_path[] ="/bad-path/" ;
@@ -99,7 +100,7 @@ int validate_headref(const char *path)/* Make sure it is a "refs/.." symlink */ if (S_ISLNK (st.st_mode)) { len =readlink (path, buffer,sizeof (buffer)-1 ); -if (len >=5 && !memcmp ("refs/" , buffer,5 )) +if (len >=5 && !memcmp (PATH_REFS , buffer,STRLEN_PATH_REFS ))return 0 ;return -1 ; }
@@ -123,7 +124,7 @@ int validate_headref(const char *path) len -=4 ;while (len &&isspace (*buf)) buf++, len--; -if (len >=5 && !memcmp ("refs/" , buf,5 )) +if (len >=STRLEN_PATH_REFS && !memcmp (PATH_REFS , buf,STRLEN_PATH_REFS ))return 0 ; }
receive-pack.c +2 -2
@@ -165,7 +165,7 @@ static const char *update(struct command *cmd)unsigned char *new_sha1 = cmd->new_sha1;struct ref_lock *lock; -if (!prefixcmp (name,"refs/" ) &&check_ref_format (name +5 )) { +if (!prefixcmp (name,PATH_REFS ) &&check_ref_format (name +STRLEN_PATH_REFS )) {error ("refusing to create funny ref '%s' locally" , name);return "funny refname" ; }
@@ -177,7 +177,7 @@ static const char *update(struct command *cmd) }if (deny_non_fast_forwards && !is_null_sha1 (new_sha1) && !is_null_sha1 (old_sha1) && - !prefixcmp (name,"refs/heads/" )) { + !prefixcmp (name,PATH_REFS_HEADS )) {struct commit *old_commit, *new_commit;struct commit_list *bases, *ent;
reflog-walk.c +3 -3
@@ -55,11 +55,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref) }if (reflogs->nr ==0 ) {int len =strlen (ref); -char *refname =xmalloc (len +12 ); -sprintf (refname,"refs/%s" , ref); +char *refname =xmalloc (len +STRLEN_PATH_REFS_HEADS +1 ); +sprintf (refname,PATH_REFS "%s" , ref);for_each_reflog_ent (refname, read_one_reflog, reflogs);if (reflogs->nr ==0 ) { -sprintf (refname,"refs/heads/%s" , ref); +sprintf (refname,PATH_REFS_HEADS "%s" , ref);for_each_reflog_ent (refname, read_one_reflog, reflogs); }free (refname);
refs.c +9 -9
@@ -409,7 +409,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int */* Follow "normalized" - ie "refs/.." symlinks by hand */ if (S_ISLNK (st.st_mode)) { len =readlink (path, buffer,sizeof (buffer)-1 ); -if (len >=5 && !memcmp ("refs/" , buffer,5 )) { +if (len >=STRLEN_PATH_REFS && !memcmp (PATH_REFS , buffer,STRLEN_PATH_REFS )) { buffer[len] =0 ;strcpy (ref_buffer, buffer); ref = ref_buffer;
@@ -561,22 +561,22 @@ int head_ref(each_ref_fn fn, void *cb_data)int for_each_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/" , fn,0 , cb_data); +return do_for_each_ref (PATH_REFS , fn,0 , cb_data); }int for_each_tag_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/tags/" , fn,10 , cb_data); +return do_for_each_ref (PATH_REFS_TAGS , fn,STRLEN_PATH_REFS_TAGS , cb_data); }int for_each_branch_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/heads/" , fn,11 , cb_data); +return do_for_each_ref (PATH_REFS_HEADS , fn,STRLEN_PATH_REFS_HEADS , cb_data); }int for_each_remote_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/remotes/" , fn,13 , cb_data); +return do_for_each_ref (PATH_REFS_REMOTES , fn,STRLEN_PATH_REFS_REMOTES , cb_data); }/* NEEDSWORK: This is only used by ssh-upload and it should go; the
@@ -588,7 +588,7 @@ int get_ref_sha1(const char *ref, unsigned char *sha1) {if (check_ref_format (ref))return -1 ; -return read_ref (mkpath ("refs/%s" , ref), sha1); +return read_ref (mkpath (PATH_REFS "%s" , ref), sha1); }/*
@@ -824,7 +824,7 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)char refpath[PATH_MAX ];if (check_ref_format (ref))return NULL ; -strcpy (refpath,mkpath ("refs/%s" , ref)); +strcpy (refpath,mkpath (PATH_REFS "%s" , ref));return lock_ref_sha1_basic (refpath, old_sha1,0 ,NULL ); }
@@ -1078,8 +1078,8 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, log_file =git_path ("logs/%s" , ref_name);if (log_all_ref_updates && - (!prefixcmp (ref_name,"refs/heads/" ) || - !prefixcmp (ref_name,"refs/remotes/" ) || + (!prefixcmp (ref_name,PATH_REFS_HEADS ) || + !prefixcmp (ref_name,PATH_REFS_REMOTES ) || !strcmp (ref_name,"HEAD" ))) {if (safe_create_leading_directories (log_file) <0 ) return error("unable to create directory for %s",
refs.h +17 -0
@@ -13,6 +13,23 @@ struct ref_lock {#define REF_ISSYMREF 01#define REF_ISPACKED 02 +#define PATH_OBJECTS "objects/" +#define STRLEN_PATH_OBJECTS 8 +#define PATH_REFS "refs/" +#define STRLEN_PATH_REFS 5 +#define PATH_HEADS "heads/" +#define STRLEN_PATH_HEADS 6 +#define PATH_TAGS "tags/" +#define STRLEN_PATH_TAGS 5 +#define PATH_REMOTES "remotes/" +#define STRLEN_PATH_REMOTES 8 +#define PATH_REFS_HEADS PATH_REFS PATH_HEADS +#define STRLEN_PATH_REFS_HEADS (STRLEN_PATH_REFS+STRLEN_PATH_HEADS) +#define PATH_REFS_TAGS PATH_REFS PATH_TAGS +#define STRLEN_PATH_REFS_TAGS (STRLEN_PATH_REFS+STRLEN_PATH_TAGS) +#define PATH_REFS_REMOTES PATH_REFS PATH_REMOTES +#define STRLEN_PATH_REFS_REMOTES (STRLEN_PATH_REFS+STRLEN_PATH_REMOTES) +/* * Calls the specified function for each ref file until it returns nonzero, * and returns the value
remote.c +7 -7
@@ -211,8 +211,8 @@ static void read_config(void) current_branch =NULL ; head_ref =resolve_ref ("HEAD" , sha1,0 , &flag);if (head_ref && (flag &REF_ISSYMREF ) && - !prefixcmp (head_ref,"refs/heads/" )) { - current_branch = head_ref +strlen ("refs/heads/" ); + !prefixcmp (head_ref,PATH_REFS_HEADS )) { + current_branch = head_ref +strlen (PATH_REFS_HEADS ); current_branch_len =strlen (current_branch); }git_config (handle_config);
@@ -398,9 +398,9 @@ static int count_refspec_match(const char *pattern,* at the remote site. */ if (namelen != patlen && - patlen != namelen -5 && -prefixcmp (name,"refs/heads/" ) && -prefixcmp (name,"refs/tags/" )) { + patlen != namelen -STRLEN_PATH_REFS_HEADS && +prefixcmp (name,PATH_REFS_HEADS ) && +prefixcmp (name,PATH_REFS_HEADS )) {/* We want to catch the case where only weak * matches are found and there are multiple * matches, and where more than one strong
@@ -511,7 +511,7 @@ static int match_explicit(struct ref *src, struct ref *dst,case 1 :break ;case 0 : -if (!memcmp (dst_value,"refs/" ,5 )) +if (!prefixcmp (dst_value,PATH_REFS )) matched_dst =make_linked_ref (dst_value, dst_tail);else error("dst refspec %s does not match any "
@@ -594,7 +594,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,if (!pat)continue ; } -else if (prefixcmp (src->name,"refs/heads/" )) +else if (prefixcmp (src->name,PATH_REFS_HEADS ))/* * "matching refs"; traditionally we pushed everything * including refs outside refs/heads/ hierarchy, but
setup.c +3 -2
@@ -1,5 +1,6 @@#include "cache.h" #include "dir.h" +#include "refs.h" static int inside_git_dir =-1 ;static int inside_work_tree =-1 ;
@@ -158,12 +159,12 @@ static int is_git_directory(const char *suspect)return 0 ; }else { -strcpy (path + len,"/objects" ); +strcpy (path + len,"/" PATH_OBJECTS );if (access (path,X_OK ))return 0 ; } -strcpy (path + len,"/refs" ); +strcpy (path + len,"/" PATH_REFS );if (access (path,X_OK ))return 0 ;
sha1_name.c +5 -5
@@ -241,11 +241,11 @@ static int ambiguous_path(const char *path, int len)static const char *ref_fmt[] = {"%.*s" , -"refs/%.*s" , -"refs/tags/%.*s" , -"refs/heads/%.*s" , -"refs/remotes/%.*s" , -"refs/remotes/%.*s/HEAD" , +PATH_REFS "%.*s" , +PATH_REFS_TAGS "%.*s" , +PATH_REFS_HEADS "%.*s" , +PATH_REFS_REMOTES "%.*s" , +PATH_REFS_REMOTES "%.*s/HEAD" ,NULL };
wt-status.c +3 -2
@@ -7,6 +7,7 @@#include "diff.h" #include "revision.h" #include "diffcore.h" +#include "refs.h" int wt_status_use_color =0 ;static char wt_status_colors[][COLOR_MAXLEN ] = {
@@ -311,8 +312,8 @@ void wt_status_print(struct wt_status *s)if (s->branch) {const char *on_what ="On branch " ;const char *branch_name = s->branch; -if (!prefixcmp (branch_name,"refs/heads/" )) - branch_name +=11 ; +if (!prefixcmp (branch_name,PATH_REFS_HEADS )) + branch_name +=STRLEN_PATH_REFS_HEADS ;else if (!strcmp (branch_name,"HEAD" )) { branch_name ="" ; on_what ="Not currently on any branch." ;
Signature · 1 line
1.5.3.rc5.11.g312e
Quoted 2 lines · Andy Parkins
Changed repeated use of the same constants for the ref paths to be symbolic constants. I've defined them in
refs.h
Please hold off on applying this. I'm getting this when running the tests:
*** t5516-fetch-push.sh ***
* ok 1: setup * ok 2: fetch without wildcard * ok 3: fetch with wildcard * ok 4: push without wildcard * ok 5: push with wildcard * ok 6: push with matching heads * ok 7: push with no ambiguity (1) * ok 8: push with no ambiguity (2) * ok 9: push with weak ambiguity (1) * ok 10: push with weak ambiguity (2) * ok 11: push with ambiguity (1)
* FAIL 12: push with ambiguity (2)
I'm having trouble seeing where the fault is at the moment though.
Andy
Signature · 2 lines
Dr Andy Parkins, M Eng (hons), MIET [email protected]
Quoted 14 lines · Andy Parkins
Please hold off on applying this. I'm getting this when running the tests:
***
t5516-fetch-push.sh**** ok 1: setup * ok 2: fetch without wildcard * ok 3: fetch with wildcard * ok 4: push without wildcard * ok 5: push with wildcard * ok 6: push with matching heads * ok 7: push with no ambiguity (1) * ok 8: push with no ambiguity (2) * ok 9: push with weak ambiguity (1) * ok 10: push with weak ambiguity (2) * ok 11: push with ambiguity (1)
Quoted 5 lines
* FAIL 12: push with ambiguity (2)
I'm having trouble seeing where the fault is at the moment though.
From your patch:
- patlen != namelen - 5 &&
- prefixcmp(name, "refs/heads/") &&
- prefixcmp(name, "refs/tags/")) {
+ patlen != namelen - STRLEN_PATH_REFS_HEADS &&
+ prefixcmp(name, PATH_REFS_HEADS) &&
+ prefixcmp(name, PATH_REFS_HEADS)) {This is totally bogus. You meant STRLEN_PATH_REFS, and the second path should be PATH_REFS_TAGS. With those changes, t5516 passes.
I haven't combed through your patch in detail, so there might be similar problems lurking. I did notice one or two spots where you call strlen(PATH_REFS_*), which should of course also be changed to STRLEN_PATH_REFS_*.
And as a final comment, your patch doesn't apply to next at all because of the reorganization of the fetching API (e.g., fetch-pack.c doesn't exist at all anymore). You should probably prepare a parallel patch for next.
-Peff
Quoted 9 lines · Jeff King
remote.c +3
-3- patlen != namelen - 5 && - prefixcmp(name, "refs/heads/") && - prefixcmp(name, "refs/tags/")) { + patlen != namelen - STRLEN_PATH_REFS_HEADS && + prefixcmp(name, PATH_REFS_HEADS) && + prefixcmp(name, PATH_REFS_HEADS)) {This is totally bogus. You meant
STRLEN_PATH_REFS, and the second path should bePATH_REFS_TAGS. With those changes, t5516 passes.
Excellent! Well done. I spent a couple of hours last night going through every changed line and have spotted the TAGS mistake but didn't spot the STRLEN being wrong. Amazing how easy it is to become blind to these things. There were a couple of errors in "/" placement too, but I don't think they were causing any trouble, just doubled up "/" characters.
Quoted 4 lines
I haven't combed through your patch in detail, so there might be similar problems lurking. I did notice one or two spots where you call
strlen(PATH_REFS_*), which should of course also be changed to STRLEN_PATH_REFS_*.
I think I caught a few of them in my review. I think I had originally left them like that on purpose. My reasoning was that a patch like that should leave the resultant code identical. So I did replacements like:
- strlen("refs/heads/");
+ strlen(PATH_REFS_HEADS);
However, I think it was just pedantry, so I've been correcting them too.
I noticed a couple of places where memcmp() has been used where prefixcmp() would work fine. I'm tempted to change them too - what do you think? Perhaps a separate patch?
Quoted 4 lines
And as a final comment, your patch doesn't apply to next at all because of the reorganization of the fetching API (e.g.,
fetch-pack.cdoesn't exist at all anymore). You should probably prepare a parallel patch for next.
I'm happy to do prepare a patch against any revision, I was really waiting for feedback from Junio as to how he'd like to manage it. Last time I submitted this patch he (quite correctly) asked that I delay until after the next point release; of course I promptly found other things to do and never resubmitted :-)
Andy
Signature · 2 lines
Dr Andy Parkins, M Eng (hons), MIET [email protected]
Quoted 5 lines · Andy Parkins
Excellent! Well done. I spent a couple of hours last night going through every changed line and have spotted the TAGS mistake but didn't spot the STRLEN being wrong. Amazing how easy it is to become blind to these things. There were a couple of errors in "/" placement too, but I don't think they were causing any trouble, just doubled up "/" characters.
I wonder if you could check the patch mechanically (i.e., write a script to confirm that '5' got replaced by tokens equal to '5'), though that might require some tricky parsing of C.
If you a post an updated version, I will try to read through it carefully, as two eyes are better than one (er, four eyes, I guess).
Quoted 3 lines
I noticed a couple of places where
memcmp()has been used whereprefixcmp()would work fine. I'm tempted to change them too - what do you think? Perhaps a separate patch?
When in doubt, I would suggest a separate patch, as it makes the review easier.
Quoted 5 lines
I'm happy to do prepare a patch against any revision, I was really waiting for feedback from Junio as to how he'd like to manage it. Last time I submitted this patch he (quite correctly) asked that I delay until after the next point release; of course I promptly found other things to do and never resubmitted :-)
Yes, you should definitely listen to Junio on such issues, and not me. :)
-Peff
PATCH 1/1 Change "refs/" references to symbolic constants
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
I've changed all references to them and made constants for the string lengths as well. This has clarified the code in some places; for example:
- len = strlen(refs[i]) + 11;
+ len = strlen(refs[i]) + STRLEN_PATH_REFS_TAGS + 1;
In this case 11 isn't STRLEN_PATH_REFS_HEADS, as it is in most other cases, it's TAGS + 1. With the change to symbolic constants it's much clearer what is happening.
--- builtin-branch.c | 28 ++++++++++++++-------------- builtin-describe.c | 2 +- builtin-fetch--tool.c | 10 +++++----- builtin-fmt-merge-msg.c | 5 +++-- builtin-fsck.c | 4 ++-- builtin-init-db.c | 15 ++++++++------- builtin-name-rev.c | 14 +++++++------- builtin-pack-refs.c | 2 +- builtin-push.c | 6 +++--- builtin-show-branch.c | 34 +++++++++++++++++----------------- builtin-show-ref.c | 6 +++--- builtin-tag.c | 4 ++-- connect.c | 10 +++++----- fetch-pack.c | 6 +++--- http-fetch.c | 31 ++++++++++++++++--------------- http-push.c | 42 +++++++++++++++++++++--------------------- local-fetch.c | 13 +++++++------ path.c | 5 +++-- receive-pack.c | 4 ++-- reflog-walk.c | 6 +++--- refs.c | 18 +++++++++--------- refs.h | 17 +++++++++++++++++ remote.c | 14 +++++++------- setup.c | 5 +++-- sha1_name.c | 10 +++++----- wt-status.c | 5 +++-- 26 files changed, 170 insertions(+), 146 deletions(-)
builtin-branch.c +14 -14
@@ -92,12 +92,12 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)switch (kinds) {case REF_REMOTE_BRANCH : - fmt ="refs/remotes/%s" ; + fmt =PATH_REFS_REMOTES "%s" ; remote ="remote " ; force =1 ;break ;case REF_LOCAL_BRANCH : - fmt ="refs/heads/%s" ; + fmt =PATH_REFS_HEADS "%s" ; remote ="" ;break ;default :
@@ -189,15 +189,15 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,int len;/* Detect kind */ -if (!prefixcmp (refname,"refs/heads/" )) { +if (!prefixcmp (refname,PATH_REFS_HEADS )) { kind =REF_LOCAL_BRANCH ; - refname +=11 ; - }else if (!prefixcmp (refname,"refs/remotes/" )) { + refname +=STRLEN_PATH_REFS_HEADS ; + }else if (!prefixcmp (refname,PATH_REFS_REMOTES )) { kind =REF_REMOTE_BRANCH ; - refname +=13 ; - }else if (!prefixcmp (refname,"refs/tags/" )) { + refname +=STRLEN_PATH_REFS_REMOTES ; + }else if (!prefixcmp (refname,PATH_REFS_TAGS )) { kind =REF_TAG ; - refname +=10 ; + refname +=STRLEN_PATH_REFS_TAGS ; }/* Don't add types the caller doesn't want */
@@ -400,7 +400,7 @@ static void create_branch(const char *name, const char *start_name,char *real_ref, ref[PATH_MAX ], msg[PATH_MAX +20 ];int forcing =0 ; -snprintf (ref,sizeof ref,"refs/heads/%s" , name); +snprintf (ref,sizeof ref,PATH_REFS_HEADS "%s" , name);if (check_ref_format (ref))die ("'%s' is not a valid branch name." , name);
@@ -469,13 +469,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)if (!oldname)die ("cannot rename the current branch while not on any." ); -if (snprintf (oldref,sizeof (oldref),"refs/heads/%s" , oldname) >sizeof (oldref)) +if (snprintf (oldref,sizeof (oldref),PATH_REFS_HEADS "%s" , oldname) >sizeof (oldref))die ("Old branchname too long" );if (check_ref_format (oldref))die ("Invalid branch name: %s" , oldref); -if (snprintf (newref,sizeof (newref),"refs/heads/%s" , newname) >sizeof (newref)) +if (snprintf (newref,sizeof (newref),PATH_REFS_HEADS "%s" , newname) >sizeof (newref))die ("New branchname too long" );if (check_ref_format (newref))
@@ -602,9 +602,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) detached =1 ; }else { -if (prefixcmp (head,"refs/heads/" )) -die ("HEAD not found below refs/heads!" ); - head +=11 ; +if (prefixcmp (head,PATH_REFS_HEADS )) +die ("HEAD not found below " PATH_REFS_HEADS "!" ); + head +=STRLEN_PATH_REFS_HEADS ; }if (delete)
builtin-describe.c +1 -1
@@ -53,7 +53,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 (!prefixcmp (path,"refs/tags/" )) { +if (!prefixcmp (path,PATH_REFS_TAGS )) {if (object->type ==OBJ_TAG ) prio =2 ;else
builtin-fetch--tool.c +5 -5
@@ -76,7 +76,7 @@ static int update_local_ref(const char *name,char *msg; just_store:/* new ref */ -if (!strncmp (name,"refs/tags/" ,10 )) +if (!prefixcmp (name,PATH_REFS_TAGS )) msg ="storing tag" ;else msg ="storing head" ;
@@ -94,7 +94,7 @@ static int update_local_ref(const char *name,return 0 ; } -if (!strncmp (name,"refs/tags/" ,10 )) { +if (!prefixcmp (name,PATH_REFS_TAGS )) {fprintf (stderr,"* %s: updating with %s\n" , name, note);show_new (type, sha1_new);return update_ref_env ("updating tag" , name, sha1_new,NULL );
@@ -151,15 +151,15 @@ static int append_fetch_head(FILE *fp, kind ="" ; what ="" ; } -else if (!strncmp (remote_name,"refs/heads/" ,11 )) { +else if (!prefixcmp (remote_name,PATH_REFS_HEADS )) { kind ="branch" ; what = remote_name +11 ; } -else if (!strncmp (remote_name,"refs/tags/" ,10 )) { +else if (!prefixcmp (remote_name,PATH_REFS_TAGS )) { kind ="tag" ; what = remote_name +10 ; } -else if (!strncmp (remote_name,"refs/remotes/" ,13 )) { +else if (!prefixcmp (remote_name,PATH_REFS_REMOTES )) { kind ="remote branch" ; what = remote_name +13 ; }
builtin-fmt-merge-msg.c +3 -2
@@ -4,6 +4,7 @@#include "diff.h" #include "revision.h" #include "tag.h" +#include "refs.h" static const char *fmt_merge_msg_usage ="git-fmt-merge-msg [--summary] [--no-summary] [--file <file>]" ;
@@ -282,8 +283,8 @@ int cmd_fmt_merge_msg(int argc, const char **argv, const char *prefix) current_branch =resolve_ref ("HEAD" , head_sha1,1 ,NULL );if (!current_branch)die ("No current branch" ); -if (!prefixcmp (current_branch,"refs/heads/" )) - current_branch +=11 ; +if (!prefixcmp (current_branch,PATH_REFS_HEADS )) + current_branch +=STRLEN_PATH_REFS_HEADS ;while (fgets (line,sizeof (line), in)) { i++;
builtin-fsck.c +2 -2
@@ -629,14 +629,14 @@ static int fsck_head_link(void)if (!strcmp (head_points_at,"HEAD" ))/* detached HEAD */ null_is_error =1 ; -else if (prefixcmp (head_points_at,"refs/heads/" )) +else if (prefixcmp (head_points_at,PATH_REFS_HEADS ))return error ("HEAD points to something strange (%s)" , head_points_at);if (is_null_sha1 (sha1)) {if (null_is_error)return error ("HEAD: detached HEAD points at nothing" );fprintf (stderr,"notice: HEAD points to an unborn branch (%s)\n" , - head_points_at +11 ); + head_points_at +STRLEN_PATH_REFS_HEADS ); }return 0 ; }
builtin-init-db.c +8 -7
@@ -5,6 +5,7 @@*/ #include "cache.h" #include "builtin.h" +#include "refs.h" #ifndef DEFAULT_GIT_TEMPLATE_DIR #define DEFAULT_GIT_TEMPLATE_DIR "/usr/share/git-core/templates"
@@ -194,11 +195,11 @@ static int create_default_files(const char *git_dir, const char *template_path)/* * Create .git/refs/{heads,tags} */ -strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );safe_create_dir (path,1 );/* First copy the templates -- we might have the default
@@ -217,11 +218,11 @@ static int create_default_files(const char *git_dir, const char *template_path)if (shared_repository) { path[len] =0 ;adjust_shared_perm (path); -strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );adjust_shared_perm (path); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );adjust_shared_perm (path); -strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );adjust_shared_perm (path); }
@@ -232,7 +233,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 ); }
builtin-name-rev.c +7 -7
@@ -96,7 +96,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, voidstruct name_ref_data *data = cb_data;int deref =0 ; -if (data->tags_only &&prefixcmp (path,"refs/tags/" )) +if (data->tags_only &&prefixcmp (path,PATH_REFS_TAGS ))return 0 ;if (data->ref_filter &&fnmatch (data->ref_filter, path,0 ))
@@ -112,14 +112,14 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, voidif (o && o->type ==OBJ_COMMIT ) {struct commit *commit = (struct commit *)o; -if (!prefixcmp (path,"refs/heads/" )) - path = path +11 ; +if (!prefixcmp (path,PATH_REFS_HEADS )) + path = path +STRLEN_PATH_REFS_HEADS ;else if (data->tags_only && data->name_only - && !prefixcmp (path,"refs/tags/" )) - path = path +10 ; -else if (!prefixcmp (path,"refs/" )) - path = path +5 ; + && !prefixcmp (path,PATH_REFS_TAGS )) + path = path +STRLEN_PATH_REFS_TAGS ; +else if (!prefixcmp (path,PATH_REFS )) + path = path +STRLEN_PATH_REFS ;name_rev (commit,xstrdup (path),0 ,0 , deref); }
builtin-pack-refs.c +1 -1
@@ -39,7 +39,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 = !prefixcmp (path,"refs/tags/" ); + is_tag_ref = !prefixcmp (path,PATH_REFS_TAGS );/* ALWAYS pack refs that were already packed or are tags */ if (!(cb->flags &PACK_REFS_ALL ) && !is_tag_ref && !(flags &REF_ISPACKED ))
builtin-push.c +3 -3
@@ -33,9 +33,9 @@ static void set_refspecs(const char **refs, int nr)int len;if (nr <= ++i)die ("tag shorthand without <tag>" ); - len =strlen (refs[i]) +11 ; + len =strlen (refs[i]) +STRLEN_PATH_REFS_TAGS +1 ; tag =xmalloc (len); -strcpy (tag,"refs/tags/" ); +strcpy (tag,PATH_REFS_TAGS );strcat (tag, refs[i]); ref = tag; }
@@ -148,7 +148,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)continue ; }if (!strcmp (arg,"--tags" )) { -add_refspec ("refs/tags/*" ); +add_refspec (PATH_REFS_TAGS "*" );continue ; }if (!strcmp (arg,"--force" ) || !strcmp (arg,"-f" )) {
builtin-show-branch.c +17 -17
@@ -380,36 +380,36 @@ static int append_ref(const char *refname, const unsigned char *sha1,static int append_head_ref (const char *refname,const unsigned char *sha1,int flag,void *cb_data) {unsigned char tmp[20 ]; -int ofs =11 ; -if (prefixcmp (refname,"refs/heads/" )) +int ofs =STRLEN_PATH_REFS_HEADS ; +if (prefixcmp (refname,PATH_REFS_HEADS ))return 0 ;/* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ if (get_sha1 (refname + ofs, tmp) ||hashcmp (tmp, sha1)) - ofs =5 ; + ofs =STRLEN_PATH_REFS ;return append_ref (refname + ofs, sha1,0 ); }static int append_remote_ref (const char *refname,const unsigned char *sha1,int flag,void *cb_data) {unsigned char tmp[20 ]; -int ofs =13 ; -if (prefixcmp (refname,"refs/remotes/" )) +int ofs =STRLEN_PATH_REFS_REMOTES ; +if (prefixcmp (refname,PATH_REFS_REMOTES ))return 0 ;/* If both heads/foo and tags/foo exists, get_sha1 would * get confused. */ if (get_sha1 (refname + ofs, tmp) ||hashcmp (tmp, sha1)) - ofs =5 ; + ofs =STRLEN_PATH_REFS ;return append_ref (refname + ofs, sha1,0 ); }static int append_tag_ref (const char *refname,const unsigned char *sha1,int flag,void *cb_data) { -if (prefixcmp (refname,"refs/tags/" )) +if (prefixcmp (refname,PATH_REFS_TAGS ))return 0 ; -return append_ref (refname +5 , sha1,0 ); +return append_ref (refname +STRLEN_PATH_REFS , sha1,0 ); }static const char *match_ref_pattern =NULL ;
@@ -438,9 +438,9 @@ static int append_matching_ref(const char *refname, const unsigned char *sha1, ireturn 0 ;if (fnmatch (match_ref_pattern, tail,0 ))return 0 ; -if (!prefixcmp (refname,"refs/heads/" )) +if (!prefixcmp (refname,PATH_REFS_HEADS ))return append_head_ref (refname, sha1, flag, cb_data); -if (!prefixcmp (refname,"refs/tags/" )) +if (!prefixcmp (refname,PATH_REFS_TAGS ))return append_tag_ref (refname, sha1, flag, cb_data);return append_ref (refname, sha1,0 ); }
@@ -465,12 +465,12 @@ static int rev_is_head(char *head, int headlen, char *name,if ((!head[0 ]) || (head_sha1 && sha1 &&hashcmp (head_sha1, sha1)))return 0 ; -if (!prefixcmp (head,"refs/heads/" )) - head +=11 ; -if (!prefixcmp (name,"refs/heads/" )) - name +=11 ; -else if (!prefixcmp (name,"heads/" )) - name +=6 ; +if (!prefixcmp (head,PATH_REFS_HEADS )) + head +=STRLEN_PATH_REFS_HEADS ; +if (!prefixcmp (name,PATH_REFS_HEADS )) + name +=STRLEN_PATH_REFS_HEADS ; +else if (!prefixcmp (name,PATH_HEADS )) + name +=STRLEN_PATH_HEADS ;return !strcmp (head, name); }
@@ -781,7 +781,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix) has_head++; }if (!has_head) { -int pfxlen =strlen ("refs/heads/" ); +int pfxlen =STRLEN_PATH_REFS_HEADS ;append_one_rev (head + pfxlen); } }
builtin-show-ref.c +3 -3
@@ -29,8 +29,8 @@ static int show_ref(const char *refname, const unsigned char *sha1, int flag, voif (tags_only || heads_only) {int match; - match = heads_only && !prefixcmp (refname,"refs/heads/" ); - match |= tags_only && !prefixcmp (refname,"refs/tags/" ); + match = heads_only && !prefixcmp (refname,PATH_REFS_HEADS ); + match |= tags_only && !prefixcmp (refname,PATH_REFS_TAGS );if (!match)return 0 ; }
@@ -227,7 +227,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)while (*pattern) {unsigned char sha1[20 ]; -if (!prefixcmp (*pattern,"refs/" ) && +if (!prefixcmp (*pattern,PATH_REFS ) &&resolve_ref (*pattern, sha1,1 ,NULL )) {if (!quiet)show_one (*pattern, sha1);
builtin-tag.c +2 -2
@@ -146,7 +146,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)unsigned char sha1[20 ];for (p = argv; *p; p++) { -if (snprintf (ref,sizeof (ref),"refs/tags/%s" , *p) +if (snprintf (ref,sizeof (ref),PATH_REFS_TAGS "%s" , *p) >=sizeof (ref)) {error ("tag name too long: %.*s..." ,50 , *p); had_error =1 ;
@@ -440,7 +440,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)if (get_sha1 (object_ref, object))die ("Failed to resolve '%s' as a valid ref." , object_ref); -if (snprintf (ref,sizeof (ref),"refs/tags/%s" , tag) >sizeof (ref) -1 ) +if (snprintf (ref,sizeof (ref),PATH_REFS_TAGS "%s" , tag) >sizeof (ref) -1 )die ("tag name too long: %.*s..." ,50 , tag);if (check_ref_format (ref))die ("'%s' is not a valid tag name." , tag);
connect.c +5 -5
@@ -13,23 +13,23 @@ static int check_ref(const char *name, int len, unsigned int flags)if (!flags)return 1 ; -if (len <5 ||memcmp (name,"refs/" ,5 )) +if (len <STRLEN_PATH_REFS ||memcmp (name,PATH_REFS ,STRLEN_PATH_REFS ))return 0 ;/* Skip the "refs/" part */ - name +=5 ; - len -=5 ; + name +=STRLEN_PATH_REFS ; + len -=STRLEN_PATH_REFS ;/* REF_NORMAL means that we don't want the magic fake tag refs */ if ((flags &REF_NORMAL ) &&check_ref_format (name) <0 )return 0 ;/* REF_HEADS means that we want regular branch heads */ -if ((flags &REF_HEADS ) && !memcmp (name,"heads/" ,6 )) +if ((flags &REF_HEADS ) && !memcmp (name,PATH_HEADS ,STRLEN_PATH_HEADS ))return 1 ;/* REF_TAGS means that we want tags */ -if ((flags &REF_TAGS ) && !memcmp (name,"tags/" ,5 )) +if ((flags &REF_TAGS ) && !memcmp (name,PATH_TAGS ,STRLEN_PATH_TAGS ))return 1 ;/* All type bits clear means that we are ok with anything */
fetch-pack.c +3 -3
@@ -344,11 +344,11 @@ static void filter_refs(struct ref **refs, int nr_match, char **match)for (ref = *refs; ref; ref = next) { next = ref->next; -if (!memcmp (ref->name,"refs/" ,5 ) && -check_ref_format (ref->name +5 )) +if (!memcmp (ref->name,PATH_REFS ,STRLEN_PATH_REFS ) && +check_ref_format (ref->name +STRLEN_PATH_REFS )) ;/* trash */ else if (fetch_all && - (!depth ||prefixcmp (ref->name,"refs/tags/" ) )) { + (!depth ||prefixcmp (ref->name,PATH_REFS_TAGS ) )) { *newtail = ref; ref->next =NULL ; newtail = &ref->next;
http-fetch.c +16 -15
@@ -3,6 +3,7 @@#include "pack.h" #include "fetch.h" #include "http.h" +#include "refs.h" #define PREV_BUF_SIZE 4096#define RANGE_HEADER_SIZE 30
@@ -157,11 +158,11 @@ static void start_object_request(struct object_request *obj_req)SHA1_Init (&obj_req->c); - url =xmalloc (strlen (obj_req->repo->base) +51 ); - obj_req->url =xmalloc (strlen (obj_req->repo->base) +51 ); + url =xmalloc (strlen (obj_req->repo->base) +STRLEN_PATH_OBJECTS +43 ); + obj_req->url =xmalloc (strlen (obj_req->repo->base) +STRLEN_PATH_OBJECTS +43 );strcpy (url, obj_req->repo->base); posn = url +strlen (obj_req->repo->base); -strcpy (posn,"/objects/" ); +strcpy (posn,"/" PATH_OBJECTS ); posn +=9 ;memcpy (posn, hex,2 ); posn +=2 ;
@@ -398,8 +399,8 @@ static int fetch_index(struct alt_base *repo, unsigned char *sha1)if (get_verbosely)fprintf (stderr,"Getting index for pack %s\n" , hex); - url =xmalloc (strlen (repo->base) +64 ); -sprintf (url,"%s/objects/pack/pack-%s.idx" , repo->base, hex); + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +56 ); +sprintf (url,"%s/" PATH_OBJECTS "pack/pack-%s.idx" , repo->base, hex); filename =sha1_pack_index_name (sha1);snprintf (tmpfile,sizeof (tmpfile),"%s.temp" , filename);
@@ -478,7 +479,7 @@ static void process_alternates_response(void *callback_data)/* Try reusing the slot to get non-http alternates */ alt_req->http_specific =0 ; -sprintf (alt_req->url,"%s/objects/info/alternates" , +sprintf (alt_req->url,"%s/" PATH_OBJECTS "info/alternates" , base);curl_easy_setopt (slot->curl,CURLOPT_URL , alt_req->url);
@@ -625,8 +626,8 @@ static void fetch_alternates(const char *base)if (get_verbosely)fprintf (stderr,"Getting alternates list for %s\n" , base); - url =xmalloc (strlen (base) +31 ); -sprintf (url,"%s/objects/info/http-alternates" , base); + url =xmalloc (strlen (base) +STRLEN_PATH_OBJECTS +23 ); +sprintf (url,"%s/" PATH_OBJECTS "info/http-alternates" , base);/* Use a callback to process the result, since another request may fail and need to have alternates loaded before continuing */
@@ -675,8 +676,8 @@ static int fetch_indices(struct alt_base *repo)if (get_verbosely)fprintf (stderr,"Getting pack list for %s\n" , repo->base); - url =xmalloc (strlen (repo->base) +21 ); -sprintf (url,"%s/objects/info/packs" , repo->base); + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +13 ); +sprintf (url,"%s/" PATH_OBJECTS "info/packs" , repo->base); slot =get_active_slot (); slot->results = &results;
@@ -757,8 +758,8 @@ static int fetch_pack(struct alt_base *repo, unsigned char *sha1) sha1_to_hex(sha1)); } - url =xmalloc (strlen (repo->base) +65 ); -sprintf (url,"%s/objects/pack/pack-%s.pack" , + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +57 ); +sprintf (url,"%s/" PATH_OBJECTS "pack/pack-%s.pack" , repo->base,sha1_to_hex (target->sha1)); filename =sha1_pack_name (target->sha1);
@@ -930,14 +931,14 @@ static char *quote_ref_url(const char *base, const char *ref)int len, baselen, ch; baselen =strlen (base); - len = baselen +7 ;/* "/refs/" + NUL */ + len = baselen +STRLEN_PATH_REFS +2 ;/* "/" + "refs/" + NUL */ for (cp = ref; (ch = *cp) !=0 ; cp++, len++)if (needs_quote (ch)) len +=2 ;/* extra two hex plus replacement % */ qref =xmalloc (len);memcpy (qref, base, baselen); -memcpy (qref + baselen,"/refs/" ,6 ); -for (cp = ref, dp = qref + baselen +6 ; (ch = *cp) !=0 ; cp++) { +memcpy (qref + baselen,"/" PATH_REFS ,STRLEN_PATH_REFS +1 ); +for (cp = ref, dp = qref + baselen +STRLEN_PATH_REFS +1 ; (ch = *cp) !=0 ; cp++) {if (needs_quote (ch)) { *dp++ ='%' ; *dp++ =hex ((ch >>4 ) &0xF );
http-push.c +21 -21
114 lines not on this page · raw message
local-fetch.c +7 -6
@@ -4,6 +4,7 @@#include "cache.h" #include "commit.h" #include "fetch.h" +#include "refs.h" static int use_link;static int use_symlink;
@@ -23,7 +24,7 @@ static void setup_index(unsigned char *sha1)struct packed_git *new_pack;char filename[PATH_MAX ];strcpy (filename, path); -strcat (filename,"/objects/pack/pack-" ); +strcat (filename,"/" PATH_OBJECTS "pack/pack-" );strcat (filename,sha1_to_hex (sha1));strcat (filename,".idx" ); new_pack =parse_pack_index_file (sha1, filename);
@@ -37,7 +38,7 @@ static int setup_indices(void)struct dirent *de;char filename[PATH_MAX ];unsigned char sha1[20 ]; -sprintf (filename,"%s/objects/pack/" , path); +sprintf (filename,"%s/" PATH_OBJECTS "pack/" , path); dir =opendir (filename);if (!dir)return -1 ;
@@ -122,11 +123,11 @@ static int fetch_pack(const unsigned char *sha1)fprintf (stderr," which contains %s\n" ,sha1_to_hex (sha1)); } -sprintf (filename,"%s/objects/pack/pack-%s.pack" , +sprintf (filename,"%s/" PATH_OBJECTS "pack/pack-%s.pack" , path,sha1_to_hex (target->sha1));copy_file (filename,sha1_pack_name (target->sha1),sha1_to_hex (target->sha1),1 ); -sprintf (filename,"%s/objects/pack/pack-%s.idx" , +sprintf (filename,"%s/" PATH_OBJECTS "pack/pack-%s.idx" , path,sha1_to_hex (target->sha1));copy_file (filename,sha1_pack_index_name (target->sha1),sha1_to_hex (target->sha1),1 );
@@ -143,7 +144,7 @@ static int fetch_file(const unsigned char *sha1)if (object_name_start <0 ) {strcpy (filename, path);/* e.g. git.git */ -strcat (filename,"/objects/" ); +strcat (filename,"/" PATH_OBJECTS ); object_name_start =strlen (filename); } filename[object_name_start+0 ] = hex[0 ];
@@ -169,7 +170,7 @@ int fetch_ref(char *ref, unsigned char *sha1)int ifd;if (ref_name_start <0 ) { -sprintf (filename,"%s/refs/" , path); +sprintf (filename,"%s/" PATH_REFS , path); ref_name_start =strlen (filename); }strcpy (filename + ref_name_start, ref);
path.c +3 -2
@@ -11,6 +11,7 @@* which is what it's designed for. */ #include "cache.h" +#include "refs.h" static char bad_path[] ="/bad-path/" ;
@@ -99,7 +100,7 @@ int validate_headref(const char *path)/* Make sure it is a "refs/.." symlink */ if (S_ISLNK (st.st_mode)) { len =readlink (path, buffer,sizeof (buffer)-1 ); -if (len >=5 && !memcmp ("refs/" , buffer,5 )) +if (len >=STRLEN_PATH_REFS && !memcmp (PATH_REFS , buffer,STRLEN_PATH_REFS ))return 0 ;return -1 ; }
@@ -123,7 +124,7 @@ int validate_headref(const char *path) len -=4 ;while (len &&isspace (*buf)) buf++, len--; -if (len >=5 && !memcmp ("refs/" , buf,5 )) +if (len >=STRLEN_PATH_REFS && !memcmp (PATH_REFS , buf,STRLEN_PATH_REFS ))return 0 ; }
receive-pack.c +2 -2
@@ -165,7 +165,7 @@ static const char *update(struct command *cmd)unsigned char *new_sha1 = cmd->new_sha1;struct ref_lock *lock; -if (!prefixcmp (name,"refs/" ) &&check_ref_format (name +5 )) { +if (!prefixcmp (name,PATH_REFS ) &&check_ref_format (name +STRLEN_PATH_REFS )) {error ("refusing to create funny ref '%s' locally" , name);return "funny refname" ; }
@@ -177,7 +177,7 @@ static const char *update(struct command *cmd) }if (deny_non_fast_forwards && !is_null_sha1 (new_sha1) && !is_null_sha1 (old_sha1) && - !prefixcmp (name,"refs/heads/" )) { + !prefixcmp (name,PATH_REFS_HEADS )) {struct commit *old_commit, *new_commit;struct commit_list *bases, *ent;
reflog-walk.c +3 -3
@@ -55,11 +55,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref) }if (reflogs->nr ==0 ) {int len =strlen (ref); -char *refname =xmalloc (len +12 ); -sprintf (refname,"refs/%s" , ref); +char *refname =xmalloc (len +STRLEN_PATH_REFS_HEADS +1 ); +sprintf (refname,PATH_REFS "%s" , ref);for_each_reflog_ent (refname, read_one_reflog, reflogs);if (reflogs->nr ==0 ) { -sprintf (refname,"refs/heads/%s" , ref); +sprintf (refname,PATH_REFS_HEADS "%s" , ref);for_each_reflog_ent (refname, read_one_reflog, reflogs); }free (refname);
refs.c +9 -9
@@ -409,7 +409,7 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int */* Follow "normalized" - ie "refs/.." symlinks by hand */ if (S_ISLNK (st.st_mode)) { len =readlink (path, buffer,sizeof (buffer)-1 ); -if (len >=5 && !memcmp ("refs/" , buffer,5 )) { +if (len >=STRLEN_PATH_REFS && !memcmp (PATH_REFS , buffer,STRLEN_PATH_REFS )) { buffer[len] =0 ;strcpy (ref_buffer, buffer); ref = ref_buffer;
@@ -561,22 +561,22 @@ int head_ref(each_ref_fn fn, void *cb_data)int for_each_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/" , fn,0 , cb_data); +return do_for_each_ref (PATH_REFS , fn,0 , cb_data); }int for_each_tag_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/tags/" , fn,10 , cb_data); +return do_for_each_ref (PATH_REFS_TAGS , fn,STRLEN_PATH_REFS_TAGS , cb_data); }int for_each_branch_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/heads/" , fn,11 , cb_data); +return do_for_each_ref (PATH_REFS_HEADS , fn,STRLEN_PATH_REFS_HEADS , cb_data); }int for_each_remote_ref (each_ref_fn fn,void *cb_data) { -return do_for_each_ref ("refs/remotes/" , fn,13 , cb_data); +return do_for_each_ref (PATH_REFS_REMOTES , fn,STRLEN_PATH_REFS_REMOTES , cb_data); }/* NEEDSWORK: This is only used by ssh-upload and it should go; the
@@ -588,7 +588,7 @@ int get_ref_sha1(const char *ref, unsigned char *sha1) {if (check_ref_format (ref))return -1 ; -return read_ref (mkpath ("refs/%s" , ref), sha1); +return read_ref (mkpath (PATH_REFS "%s" , ref), sha1); }/*
@@ -824,7 +824,7 @@ struct ref_lock *lock_ref_sha1(const char *ref, const unsigned char *old_sha1)char refpath[PATH_MAX ];if (check_ref_format (ref))return NULL ; -strcpy (refpath,mkpath ("refs/%s" , ref)); +strcpy (refpath,mkpath (PATH_REFS "%s" , ref));return lock_ref_sha1_basic (refpath, old_sha1,0 ,NULL ); }
@@ -1078,8 +1078,8 @@ static int log_ref_write(const char *ref_name, const unsigned char *old_sha1, log_file =git_path ("logs/%s" , ref_name);if (log_all_ref_updates && - (!prefixcmp (ref_name,"refs/heads/" ) || - !prefixcmp (ref_name,"refs/remotes/" ) || + (!prefixcmp (ref_name,PATH_REFS_HEADS ) || + !prefixcmp (ref_name,PATH_REFS_REMOTES ) || !strcmp (ref_name,"HEAD" ))) {if (safe_create_leading_directories (log_file) <0 ) return error("unable to create directory for %s",
refs.h +17 -0
@@ -13,6 +13,23 @@ struct ref_lock {#define REF_ISSYMREF 01#define REF_ISPACKED 02 +#define PATH_OBJECTS "objects/" +#define STRLEN_PATH_OBJECTS 8 +#define PATH_REFS "refs/" +#define STRLEN_PATH_REFS 5 +#define PATH_HEADS "heads/" +#define STRLEN_PATH_HEADS 6 +#define PATH_TAGS "tags/" +#define STRLEN_PATH_TAGS 5 +#define PATH_REMOTES "remotes/" +#define STRLEN_PATH_REMOTES 8 +#define PATH_REFS_HEADS PATH_REFS PATH_HEADS +#define STRLEN_PATH_REFS_HEADS (STRLEN_PATH_REFS+STRLEN_PATH_HEADS) +#define PATH_REFS_TAGS PATH_REFS PATH_TAGS +#define STRLEN_PATH_REFS_TAGS (STRLEN_PATH_REFS+STRLEN_PATH_TAGS) +#define PATH_REFS_REMOTES PATH_REFS PATH_REMOTES +#define STRLEN_PATH_REFS_REMOTES (STRLEN_PATH_REFS+STRLEN_PATH_REMOTES) +/* * Calls the specified function for each ref file until it returns nonzero, * and returns the value
remote.c +7 -7
@@ -211,8 +211,8 @@ static void read_config(void) current_branch =NULL ; head_ref =resolve_ref ("HEAD" , sha1,0 , &flag);if (head_ref && (flag &REF_ISSYMREF ) && - !prefixcmp (head_ref,"refs/heads/" )) { - current_branch = head_ref +strlen ("refs/heads/" ); + !prefixcmp (head_ref,PATH_REFS_HEADS )) { + current_branch = head_ref +STRLEN_PATH_REFS_HEADS ; current_branch_len =strlen (current_branch); }git_config (handle_config);
@@ -398,9 +398,9 @@ static int count_refspec_match(const char *pattern,* at the remote site. */ if (namelen != patlen && - patlen != namelen -5 && -prefixcmp (name,"refs/heads/" ) && -prefixcmp (name,"refs/tags/" )) { + patlen != namelen -STRLEN_PATH_REFS && +prefixcmp (name,PATH_REFS_HEADS ) && +prefixcmp (name,PATH_REFS_TAGS )) {/* We want to catch the case where only weak * matches are found and there are multiple * matches, and where more than one strong
@@ -511,7 +511,7 @@ static int match_explicit(struct ref *src, struct ref *dst,case 1 :break ;case 0 : -if (!memcmp (dst_value,"refs/" ,5 )) +if (!prefixcmp (dst_value,PATH_REFS )) matched_dst =make_linked_ref (dst_value, dst_tail);else error("dst refspec %s does not match any "
@@ -594,7 +594,7 @@ int match_refs(struct ref *src, struct ref *dst, struct ref ***dst_tail,if (!pat)continue ; } -else if (prefixcmp (src->name,"refs/heads/" )) +else if (prefixcmp (src->name,PATH_REFS_HEADS ))/* * "matching refs"; traditionally we pushed everything * including refs outside refs/heads/ hierarchy, but
setup.c +3 -2
@@ -1,5 +1,6 @@#include "cache.h" #include "dir.h" +#include "refs.h" static int inside_git_dir =-1 ;static int inside_work_tree =-1 ;
@@ -158,12 +159,12 @@ static int is_git_directory(const char *suspect)return 0 ; }else { -strcpy (path + len,"/objects" ); +strcpy (path + len,"/" PATH_OBJECTS );if (access (path,X_OK ))return 0 ; } -strcpy (path + len,"/refs" ); +strcpy (path + len,"/" PATH_REFS );if (access (path,X_OK ))return 0 ;
sha1_name.c +5 -5
@@ -241,11 +241,11 @@ static int ambiguous_path(const char *path, int len)static const char *ref_fmt[] = {"%.*s" , -"refs/%.*s" , -"refs/tags/%.*s" , -"refs/heads/%.*s" , -"refs/remotes/%.*s" , -"refs/remotes/%.*s/HEAD" , +PATH_REFS "%.*s" , +PATH_REFS_TAGS "%.*s" , +PATH_REFS_HEADS "%.*s" , +PATH_REFS_REMOTES "%.*s" , +PATH_REFS_REMOTES "%.*s/HEAD" ,NULL };
wt-status.c +3 -2
@@ -7,6 +7,7 @@#include "diff.h" #include "revision.h" #include "diffcore.h" +#include "refs.h" int wt_status_use_color =0 ;static char wt_status_colors[][COLOR_MAXLEN ] = {
@@ -311,8 +312,8 @@ void wt_status_print(struct wt_status *s)if (s->branch) {const char *on_what ="On branch " ;const char *branch_name = s->branch; -if (!prefixcmp (branch_name,"refs/heads/" )) - branch_name +=11 ; +if (!prefixcmp (branch_name,PATH_REFS_HEADS )) + branch_name +=STRLEN_PATH_REFS_HEADS ;else if (!strcmp (branch_name,"HEAD" )) { branch_name ="" ; on_what ="Not currently on any branch." ;
Signature · 1 line
1.5.3.rc5.11.g312e
Quoted 2 lines · Andy Parkins
Changed repeated use of the same constants for the ref paths to be symbolic constants. I've defined them in
refs.h
I've manually inspected the patch. Comments are below.
builtin-branch.c int cmd_branch(int argc, const char **argv, const char *prefix) · line 607
Quoted 6 lines
builtin-branch.c +3
-3-if (prefixcmp (head,"refs/heads/" )) -die ("HEAD not found below refs/heads!" ); - head +=11 ; +if (prefixcmp (head,PATH_REFS_HEADS )) +die ("HEAD not found below " PATH_REFS_HEADS "!" ); + head +=STRLEN_PATH_REFS_HEADS ;
This slightly changes the message (extra "/"), but I don't think that is a big deal...
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 203
Quoted 9 lines
builtin-init-db.c +3
-3-strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );safe_create_dir (path,1 );
...but here it's not immediately obvious if the extra trailing "/" is OK. Looks like the path just gets handed off to system calls trhough safe_create_dir, and they are happy with the trailing slash. But it is a behavior change.
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 226
Quoted 9 lines
builtin-init-db.c +3
-3-strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );adjust_shared_perm (path); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );adjust_shared_perm (path); -strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );adjust_shared_perm (path);
And of course ditto here.
builtin-tag.c static int for_each_tag_name(const char **argv, each_tag_name_fn fn) · line 149
Quoted 2 lines
builtin-tag.c +1
-1- if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p) + if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)
I find the 'PATH_REFS_TAGS "%s"' (with a space) you used earlier a little easier to read.
connect.c static int check_ref(const char *name, int len, unsigned int flags) · line 16
Quoted 2 lines
connect.c +1
-1-if (len <5 ||memcmp (name,"refs/" ,5 )) +if (len <STRLEN_PATH_REFS ||memcmp (name,PATH_REFS ,STRLEN_PATH_REFS ))
I imagine this was one of the times you mentioned before where prefixcmp would be more readable. I would agree.
http-fetch.c static void start_object_request(struct object_request *obj_req) · line 166
Quoted 3 lines
http-fetch.c +1
-1-strcpy (posn,"/objects/" ); +strcpy (posn,"/" PATH_OBJECTS ); posn +=9 ;
should be posn += 1 + STRLEN_PATH_OBJECTS ?
http-fetch.c static int fetch_index(struct alt_base *repo, unsigned char *sha1) · line 403
Quoted 4 lines
http-fetch.c +2
-2- url =xmalloc (strlen (repo->base) +64 ); -sprintf (url,"%s/objects/pack/pack-%s.idx" , repo->base, hex); + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +56 ); +sprintf (url,"%s/" PATH_OBJECTS "pack/pack-%s.idx" , repo->base, hex);
The '56' is still quite hard to verify as correct ("/" + "pack/pack-" + ".idx" + "\0"). But I wonder if trying to fix that will just make it harder to read (perhaps a comment is in order?).
Or maybe using a strbuf here would be much more obviously correct?
http-fetch.c static void fetch_alternates(const char *base) · line 630
Quoted 4 lines
http-fetch.c +2
-2- url =xmalloc (strlen (base) +31 ); -sprintf (url,"%s/objects/info/http-alternates" , base); + url =xmalloc (strlen (base) +STRLEN_PATH_OBJECTS +23 ); +sprintf (url,"%s/" PATH_OBJECTS "info/http-alternates" , base);
Also a potential strbuf. Ther are more of this same form, but I'm not going to bother pointing out each one.
Quoted 2 lines
Signature · 1 line
1.5.3.rc5.11.g312e
Man that was tedious. But I think every other change is purely syntactic, so there shouldn't be any bugs.
-Peff
Quoted 7 lines · Jeff King
Quoted 2 lines · Andy Parkins
Changed repeated use of the same constants for the ref paths to be symbolic constants. I've defined them in
refs.hI've manually inspected the patch. Comments are below.
Quoted 9 lines
builtin-branch.c int cmd_branch(int argc, const char **argv, const char *prefix) · line 607
Quoted 6 lines
builtin-branch.c +3
-3-if (prefixcmp (head,"refs/heads/" )) -die ("HEAD not found below refs/heads!" ); - head +=11 ; +if (prefixcmp (head,PATH_REFS_HEADS )) +die ("HEAD not found below " PATH_REFS_HEADS "!" ); + head +=STRLEN_PATH_REFS_HEADS ;This slightly changes the message (extra "/"), but I don't think that is a big deal...
die("HEAD not found below %.*%s!",
PATH_REFS_HEADS, STRLEN_PATH_REFS_HEADS-1)Quoted 5 lines
builtin-tag.c static int for_each_tag_name(const char **argv, each_tag_name_fn fn) · line 149
Quoted 2 lines
builtin-tag.c +1
-1- if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p) + if (snprintf(ref, sizeof(ref), PATH_REFS_TAGS"%s", *p)I find the '
PATH_REFS_TAGS"%s"' (with a space) you used earlier a little easier to read.
Even though we all know that PATH_REFS_* do not have any '%' in them, it is somewhat unnerving to see such an opaque string in the format specifier part of _any_printf() function. It just makes you think twice, disrupting the flow of thoughts.
This applies to die() and friends as well; see my above rewrite.
To me, the valid reasons for this kind of rewrite are if:
- it makes typo harder to make and easier to spot (e.g. "refs/head/");
- it makes miscount harder to make and easier to spot (e.g. what is this magic constant 11? Is it strlen("refs/heads/")?);
- it makes reviewing the resulting code, and more importantly, future patches on the resulting code, easier.
- it makes it easier for us to later revamp the strings wholesale (e.g. "refs/heads/" => "refs/branches/").
- it saves us repeated instances of the same string constant; using C literal string as values for PATH_REFS_HEADS would not help and you would need (const char []) strings instead, but the compiler may be clever enough to do so.
Unquestionably, this series helps on the first two counts.
It however actively hurts on the third count. These long constants in CAPITAL_LETTERS_WITH_UNDERSCORE shout too loudly to the eye, overwhelming the surrounding code. I wonder if we can do anything about this point to resurrect the first two benefits, which I like very much.
The forth is a myth we shouldn't care about. If we later would want to change refs/heads to refs/branches, we would want to rename PATH_REFS_HEADS to PATH_REFS_BRANCHES at the same time as well, so the kind of rewrite this patch does does not buy us anything there. More importantly, such a change would need to be made in a backward compatible way (e.g. "if we have heads then keep using them but in new repositories we favor branches"), so it won't be straight token replacement anyway.
And the fifth do not apply to us. This matters only if we were an embedded application on memory starved machine and string constants are far smaller matter compared to the amount of other data we use in-core.
Quoted 10 lines · Junio C Hamano
- it makes typo harder to make and easier to spot (e.g. "refs/head/");
- it makes miscount harder to make and easier to spot (e.g. what is this magic constant 11? Is it
strlen("refs/heads/")?);- it makes reviewing the resulting code, and more importantly, future patches on the resulting code, easier.
[...]
It however actively hurts on the third count. These long
Yes, I find some of the substitutions more readable, but some are a bit less readable. The parts of the patch I found the _most_ improved are the ones that get rid of a memcmp in favor of a prefixcmp (i.e., removing the count entirely).
Perhaps a better quest would be to eliminate all of those counts entirely with code that is obviously correct. I think it is much more readable to replace:
url = xmalloc(strlen(repo->base) + 64); sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
with something like:
strbuf_init(&url); strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
which has the same number of lines, but no magic numbers at all. Or since most of the uses of things like PATH_OBJECTS are more or less the same, maybe something like:
mkpath_object(&url, "pack/pack-%s.idx", hex);
i.e., rather than fiddling with string constants, wrap them functionally.
Quoted 1 line
constants in
CAPITAL_LETTERS_WITH_UNDERSCOREshout too loudly to
Part of the problem is also that they're long. Perhaps REFS_HEADS, while being less unique in the C namespace, would look better?
-Peff
Quoted 3 lines · Jeff King
Perhaps a better quest would be to eliminate all of those counts entirely with code that is obviously correct. I think it is much more readable to replace:
I've got a patch replacing every appropriate memcmp() with prefixcmp(), but it goes on top of this one, so wanted to get this through review to save constantly spamming the list with the same patch slightly modified because of changes in a different patch.
Quoted 7 lines
url = xmalloc(strlen(repo->base) + 64); sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);with something like:
strbuf_init(&url); strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
I've not been following the strbuf() changes, so have missed the appearance of these handy new functions. They would appear to be an improvement for cases just like this.
Quoted 4 lines
Quoted 1 line
constants in
CAPITAL_LETTERS_WITH_UNDERSCOREshout too loudly toPart of the problem is also that they're long. Perhaps
REFS_HEADS, while being less unique in the C namespace, would look better?
I completely agree with the length and loudness concerns, but my worry was polluting the namespace while maintaining some sort of rationality between PATH_REFS_HEADS and STRLEN_PATH_REFS_HEADS. My reasoning was that
"refs/heads" -> PATH_REFS_HEADS
is only three extra characters, and
strlen("refs/heads/") -> STRLEN_PATH_REFS_HEADS
is only one extra character.
However I have no strong feelings about changing them.
Andy
Signature · 2 lines
Dr Andy Parkins, M Eng (hons), MIET [email protected]
Quoted 11 lines · Jeff King
Perhaps a better quest would be to eliminate all of those counts entirely with code that is obviously correct. I think it is much more readable to replace:
url = xmalloc(strlen(repo->base) + 64); sprintf(url, "%s/objects/pack/pack-%s.idx", repo->base, hex);with something like:
strbuf_init(&url); strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);
Ugh, this typically calls snprintf() twice doesn't it?
Quoted 4 lines · Junio C Hamano
Quoted 2 lines
strbuf_init(&url); strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);Ugh, this typically calls
snprintf()twice doesn't it?
Yes, it probably does. However, I think it is considerably easier to read and more maintainable. Are you "ugh"ing because of the performance impact (which should be negligible unless this is in a tight loop) or because of the portability problems associated with va_copy?
-Peff
Hi,
Quoted 11 lines · Jeff King
Quoted 4 lines · Junio C Hamano
Quoted 2 lines
strbuf_init(&url); strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);Ugh, this typically calls
snprintf()twice doesn't it?Yes, it probably does. However, I think it is considerably easier to read and more maintainable. Are you "ugh"ing because of the performance impact (which should be negligible unless this is in a tight loop) or because of the portability problems associated with
va_copy?
I wonder, I wonder, if
strbuf_addstr(&url, repo->base); strbuf_addstr(&url, "/objects/pack/pack-"); strbuf_addstr(&url, hex); strbuf_addstr(&url, ".idx");
would make anybody else but me happy...
Ciao,
Dscho
Quoted 8 lines · Johannes Schindelin
I wonder, I wonder, if
strbuf_addstr(&url, repo->base); strbuf_addstr(&url, "/objects/pack/pack-"); strbuf_addstr(&url, hex); strbuf_addstr(&url, ".idx");would make anybody else but me happy...
I actually wrote that originally, and then switched to the formatted version for readability. But I would be happy with that, as well, if we are truly concerned about the cost of 2 snprintfs.
-Peff
Quoted 14 lines · Johannes Schindelin
Hi,
Quoted 9 lines · Jeff King
Quoted 3 lines · Junio C Hamano
Quoted 2 lines
strbuf_init(&url); strbuf_addf(&url, "%s/objects/pack/pack-%s.idx", repo->base, hex);Ugh, this typically calls
snprintf()twice doesn't it?Yes, it probably does. However, I think it is considerably easier to read and more maintainable. Are you "ugh"ing because of the performance impact (which should be negligible unless this is in a tight loop) or because of the portability problems associated with
va_copy?
Quoted 8 lines
I wonder, I wonder, if
strbuf_addstr(&url, repo->base); strbuf_addstr(&url, "/objects/pack/pack-"); strbuf_addstr(&url, hex); strbuf_addstr(&url, ".idx");would make anybody else but me happy...
strbuf_addstr_many(&url, repo->base, "/objects/pack/pack-", hex, ".idx", NULL);
is what I'd prefer. It's not overly complicated, requires no *printf(), and doesn't introduce any new portability issues (va_arg() is C89).
Signature · 3 lines
Andreas Ericsson [email protected] OP5 AB www.op5.se Tel: +46 8-230225 Fax: +46 8-230231
Quoted 9 lines · Jeff King
builtin-branch.c int cmd_branch(int argc, const char **argv, const char *prefix) · line 607
Quoted 6 lines
builtin-branch.c +3
-3-if (prefixcmp (head,"refs/heads/" )) -die ("HEAD not found below refs/heads!" ); - head +=11 ; +if (prefixcmp (head,PATH_REFS_HEADS )) +die ("HEAD not found below " PATH_REFS_HEADS "!" ); + head +=STRLEN_PATH_REFS_HEADS ;This slightly changes the message (extra "/"), but I don't think that is a big deal...
Actually, I felt that was an improvement. Personally I always like paths shown to a user to have the trailing slash so that they can be clear that it is a path not a file.
Quoted 6 lines
Quoted 6 lines
builtin-init-db.c +2
-2-strcpy (path + len,"refs" ); +strcpy (path + len,PATH_REFS );safe_create_dir (path,1 ); -strcpy (path + len,"refs/heads" ); +strcpy (path + len,PATH_REFS_HEADS );safe_create_dir (path,1 );
Quoted 8 lines
builtin-init-db.c static int create_default_files(const char *git_dir, const char *template_path) · line 203
Quoted 3 lines
builtin-init-db.c +1
-1-strcpy (path + len,"refs/tags" ); +strcpy (path + len,PATH_REFS_TAGS );safe_create_dir (path,1 );...but here it's not immediately obvious if the extra trailing "/" is OK. Looks like the path just gets handed off to system calls trhough
safe_create_dir, and they are happy with the trailing slash. But it is a behavior change.
It's been a while, but at the time I did it I think I checked safe_create_dir() to be sure that it was happy with trailing slashes.
Quoted 2 lines
I find the '
PATH_REFS_TAGS"%s"' (with a space) you used earlier a little easier to read.
Okay.
Quoted 6 lines
Quoted 3 lines
(quoted diff) +1
-1- if (len < 5 || memcmp(name, "refs/", 5)) + if (len < STRLEN_PATH_REFS || memcmp(name, PATH_REFS,
STRLEN_PATH_REFS))I imagine this was one of the times you mentioned before where prefixcmp would be more readable. I would agree.
It is. A patch to fix these is in my pending list.
Quoted 5 lines
http-fetch.c static void start_object_request(struct object_request *obj_req) · line 166
Quoted 3 lines
http-fetch.c +1
-1-strcpy (posn,"/objects/" ); +strcpy (posn,"/" PATH_OBJECTS ); posn +=9 ;should be posn += 1 +
STRLEN_PATH_OBJECTS?
Well spotted. Fixed.
Quoted 8 lines
http-fetch.c static int fetch_index(struct alt_base *repo, unsigned char *sha1) · line 403
Quoted 4 lines
http-fetch.c +2
-2- url =xmalloc (strlen (repo->base) +64 ); -sprintf (url,"%s/objects/pack/pack-%s.idx" , repo->base, hex); + url =xmalloc (strlen (repo->base) +STRLEN_PATH_OBJECTS +56 ); +sprintf (url,"%s/" PATH_OBJECTS "pack/pack-%s.idx" , repo->base, hex);The '56' is still quite hard to verify as correct ("/" + "pack/pack-" + ".idx" + "\0"). But I wonder if trying to fix that will just make it harder to read (perhaps a comment is in order?).
I put a comment above the other changes like this in the same file, but thought I was being overly verbose putting it in every single time. I'm happy to copy the comment around in the file though.
Quoted 9 lines
Or maybe using a strbuf here would be much more obviously correct?
http-fetch.c static void fetch_alternates(const char *base) · line 630
Quoted 4 lines
http-fetch.c +2
-2- url =xmalloc (strlen (base) +31 ); -sprintf (url,"%s/objects/info/http-alternates" , base); + url =xmalloc (strlen (base) +STRLEN_PATH_OBJECTS +23 ); +sprintf (url,"%s/" PATH_OBJECTS "info/http-alternates" , base);Also a potential strbuf. Ther are more of this same form, but I'm not going to bother pointing out each one.
I was trying, as far as I could, to leave the code unchanged. If strbuf would be better I think I'd rather do it with another patch after this.
Quoted 2 lines
Man that was tedious. But I think every other change is purely syntactic, so there shouldn't be any bugs.
It really was wasn't it? :-) While I was trying to find that bug that you
caught yesterday I thought I was going blind. I have this to say though:
thank heaven for git's colourised diffs. Those that think coloured output is just for prettiness sake should try that review with and without.
Thank you for expending so much effort on this, it is appreciated.
Andy
Signature · 2 lines
Dr Andy Parkins, M Eng (hons), MIET [email protected]
Quoted 3 lines · Andy Parkins
I put a comment above the other changes like this in the same file, but thought I was being overly verbose putting it in every single time. I'm happy to copy the comment around in the file though.
I'm a liar; I remember writing them, but they're not in the patch. Perhaps they are still in my working tree at home. Anyway; you're point was correct, I'll find the explanatory comments or write new ones and add them to the patch.
Andy
Signature · 2 lines
Dr Andy Parkins, M Eng (hons), MIET [email protected]
Quoted 3 lines · Andy Parkins
I noticed a couple of places where
memcmp()has been used whereprefixcmp()would work fine. I'm tempted to change them too - what do you think? Perhaps a separate patch?
In general, probably it is preferable to have a separate "preliminary patch" to normalize the existing code without using the new infrastructure (i.e. REF_* macros), and then to have the main patch. That way would make the main patch more about mechanical conversion, which would be easier to verify independently.
Quoted 11 lines · Junio C Hamano
Quoted 3 lines · Andy Parkins
I noticed a couple of places where
memcmp()has been used whereprefixcmp()would work fine. I'm tempted to change them too - what do you think? Perhaps a separate patch?In general, probably it is preferable to have a separate "preliminary patch" to normalize the existing code without using the new infrastructure (i.e. REF_* macros), and then to have the main patch. That way would make the main patch more about mechanical conversion, which would be easier to verify independently.
I only noticed them memcmp() use while I was reviewing the PATH_REFS patch :-) So making it preliminary would have meant travelling backwards in time.
I imagine I can do a bit of rebase-interactive to rearrange should that be the route you'd like me to go. Your call.
Andy
Signature · 2 lines
Dr Andy Parkins, M Eng (hons), MIET [email protected]