object.c: localize global the_repository variable into r
cover object.c: localize global the_repository variable into r
I have created this commit in response to
https://github.com/gitgitgadget/git/issues/379#issue-503866117. All the functions in object.c which relied on 'the_repository' have been modified. The functions calling these functions in object.c consisted calls to other functions using 'the_repository' as well, and although I intended to use 'r' at all those instances, I thought it would make more sense when we would deal with their callee functions in another similar patch. What do you think ?
Parth Gala (5):
object.c: get_max_object_index and get_indexed_object accept 'r'
parameter
object.c: lookup_unknown_object() accept 'r' as parameter object.c: parse_object_or_die() accept 'r' as parameter object.c: clear_object_flags() accept 'r' as parameter object.c: clear_commit_marks_all() accept 'r' as parameter
builtin/checkout.c | 3 ++- builtin/fsck.c | 8 +++++--- builtin/grep.c | 6 ++++-- builtin/index-pack.c | 5 +++-- builtin/log.c | 3 ++- builtin/name-rev.c | 5 +++-- builtin/pack-objects.c | 3 ++- builtin/prune.c | 3 ++- bundle.c | 8 +++++--- http-push.c | 3 ++- object.c | 32 ++++++++++++++++---------------- object.h | 12 ++++++------ pack-bitmap.c | 5 +++-- reachable.c | 6 ++++-- refs.c | 3 ++- revision.c | 3 ++- shallow.c | 13 ++++++++----- t/helper/test-example-decorate.c | 7 ++++--- upload-pack.c | 19 ++++++++++++------- walker.c | 3 ++- 20 files changed, 89 insertions(+), 61 deletions(-)
base-commit: 0ad714499976290d9a0229230cbe4efae930b8dc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-545%2FParthGala2k%2Flocalize-the_repository-variable-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-545/ParthGala2k/localize-the_repository-variable-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/545
Signature · 1 line
gitgitgadget
PATCH 3/5 object.c: parse_object_or_die() accept 'r' as parameter
From: Parth Gala <[email protected]>
'parse_object_or_die()' and its callers are modified to enable passing 'r' as an argument to 'parse_object_or_die()'.
--- builtin/grep.c | 6 ++++-- builtin/prune.c | 3 ++- bundle.c | 8 +++++--- object.c | 4 ++-- object.h | 2 +- pack-bitmap.c | 5 +++-- reachable.c | 6 ++++-- upload-pack.c | 4 +++- 8 files changed, 24 insertions(+), 14 deletions(-)
builtin/grep.c +4 -2
@@ -406,6 +406,7 @@ static int grep_submodule(struct grep_opt *opt, const char *filename, const char *path, int cached) { struct repository subrepo; +struct repository *r = the_repository;struct repository *superproject = opt->repo;const struct submodule *sub =submodule_from_path (superproject, &null_oid, path);
@@ -455,7 +456,7 @@ static int grep_submodule(struct grep_opt *opt,unsigned long size;struct strbuf base =STRBUF_INIT ; - object =parse_object_or_die (oid,oid_to_hex (oid)); + object =parse_object_or_die (r, oid,oid_to_hex (oid));grep_read_lock (); data = read_object_with_reference(&subrepo,
@@ -802,6 +803,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)const char *show_in_pager =NULL , *default_pager ="dummy" ;struct grep_opt opt;struct object_array list =OBJECT_ARRAY_INIT ; +struct repository *r = the_repository;struct pathspec pathspec;struct string_list path_list =STRING_LIST_INIT_NODUP ;int i;
@@ -1037,7 +1039,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)break ; } - object =parse_object_or_die (&oid, arg); + object =parse_object_or_die (r, &oid, arg);if (!seen_dashdash)verify_non_filename (prefix, arg);add_object_array_with_path (object, arg, &list, oc.mode, oc.path);
builtin/prune.c +2 -1
@@ -125,6 +125,7 @@ static void remove_temporary_files(const char *path)int cmd_prune (int argc,const char **argv,const char *prefix) {struct rev_info revs; +struct repository *r = the_repository;int exclude_promisor_objects =0 ;const struct option options[] = {OPT__DRY_RUN (&show_only,N_ ("do not remove, show only" )),
@@ -154,7 +155,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)const char *name = *argv++;if (!get_oid (name, &oid)) { -struct object *object =parse_object_or_die (&oid, +struct object *object =parse_object_or_die (r, &oid, name);add_pending_object (&revs, object,"" ); }
bundle.c +5 -3
@@ -298,6 +298,7 @@ static int compute_and_write_prerequisites(int bundle_fd, {struct child_process rls =CHILD_PROCESS_INIT ;struct strbuf buf =STRBUF_INIT ; +struct repository *r = the_repository;FILE *rls_fout;int i;
@@ -316,13 +317,13 @@ static int compute_and_write_prerequisites(int bundle_fd,if (buf.len >0 && buf.buf[0 ] =='-' ) {write_or_die (bundle_fd, buf.buf, buf.len);if (!get_oid_hex (buf.buf +1 , &oid)) { -struct object *object =parse_object_or_die (&oid, +struct object *object =parse_object_or_die (r, &oid, buf.buf); object->flags |=UNINTERESTING ;add_pending_object (revs, object, buf.buf); } }else if (!get_oid_hex (buf.buf, &oid)) { -struct object *object =parse_object_or_die (&oid, +struct object *object =parse_object_or_die (r, &oid, buf.buf); object->flags |=SHOWN ; }
@@ -347,6 +348,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs) {int i;int ref_count =0 ; +struct repository *r = the_repository;for (i =0 ; i < revs->pending.nr; i++) {struct object_array_entry *e = revs->pending.objects + i;
@@ -407,7 +409,7 @@ static int write_bundle_refs(int bundle_fd, struct rev_info *revs)* end up triggering "empty bundle" * error. */ - obj =parse_object_or_die (&oid, e->name); + obj =parse_object_or_die (r, &oid, e->name); obj->flags |=SHOWN ;add_pending_object (revs, obj, e->name); }
object.c +2 -2
@@ -236,10 +236,10 @@ struct object *parse_object_buffer(struct repository *r, const struct object_idreturn obj; } -struct object *parse_object_or_die (const struct object_id *oid, +struct object *parse_object_or_die (struct repository *r,const struct object_id *oid,const char *name) { -struct object *o =parse_object (the_repository, oid); +struct object *o =parse_object (r, oid);if (o)return o;
object.h +1 -1
@@ -135,7 +135,7 @@ struct object *parse_object(struct repository *r, const struct object_id *oid);* "name" parameter is not NULL, it is included in the error message * (otherwise, the hex object ID is given). */ -struct object *parse_object_or_die (const struct object_id *oid,const char *name); +struct object *parse_object_or_die (struct repository *,const struct object_id *oid,const char *name);/* Given the result of read_sha1_file(), returns the object after * parsing it. eaten_p indicates if the object has a borrowed copy
pack-bitmap.c +3 -2
@@ -682,6 +682,7 @@ static int in_bitmapped_pack(struct bitmap_index *bitmap_git,struct bitmap_index *prepare_bitmap_walk (struct rev_info *revs) {unsigned int i; +struct repository *r = the_repository;struct object_list *wants =NULL ;struct object_list *haves =NULL ;
@@ -699,7 +700,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)struct object *object = revs->pending.objects[i].item;if (object->type ==OBJ_NONE ) -parse_object_or_die (&object->oid,NULL ); +parse_object_or_die (r, &object->oid,NULL );while (object->type ==OBJ_TAG ) {struct tag *tag = (struct tag *) object;
@@ -709,7 +710,7 @@ struct bitmap_index *prepare_bitmap_walk(struct rev_info *revs)else object_list_insert (object, &wants); - object =parse_object_or_die (get_tagged_oid (tag),NULL ); + object =parse_object_or_die (r,get_tagged_oid (tag),NULL ); }if (object->flags &UNINTERESTING )
reachable.c +4 -2
@@ -31,13 +31,14 @@ static int add_one_ref(const char *path, const struct object_id *oid, {struct rev_info *revs = (struct rev_info *)cb_data;struct object *object; +struct repository *r = the_repository;if ((flag &REF_ISSYMREF ) && (flag &REF_ISBROKEN )) {warning ("symbolic ref is dangling: %s" , path);return 0 ; } - object =parse_object_or_die (oid, path); + object =parse_object_or_die (r, oid, path);add_pending_object (revs, object,"" );return 0 ;
@@ -68,6 +69,7 @@ static void add_recent_object(const struct object_id *oid, {struct object *obj;enum object_type type; +struct repository *r = the_repository;if (mtime <= data->timestamp)return ;
@@ -86,7 +88,7 @@ static void add_recent_object(const struct object_id *oid,switch (type) {case OBJ_TAG :case OBJ_COMMIT : - obj =parse_object_or_die (oid,NULL ); + obj =parse_object_or_die (r, oid,NULL );break ;case OBJ_TREE : obj = (struct object *)lookup_tree (the_repository, oid);
upload-pack.c +3 -1
@@ -1210,6 +1210,8 @@ static int parse_want_ref(struct packet_writer *writer, const char *line, struct object_array *want_obj) {const char *arg; +struct repository *r = the_repository; +if (skip_prefix (line,"want-ref " , &arg)) {struct object_id oid;struct string_list_item *item;
@@ -1223,7 +1225,7 @@ static int parse_want_ref(struct packet_writer *writer, const char *line, item =string_list_append (wanted_refs, arg); item->util =oiddup (&oid); - o =parse_object_or_die (&oid, arg); + o =parse_object_or_die (r, &oid, arg);if (!(o->flags &WANTED )) { o->flags |=WANTED ;add_object_array (o,NULL , want_obj);
Signature · 1 line
gitgitgadget
PATCH 1/5 object.c: get_max_object_index and get_indexed_object accept 'r' parameter
In reply to Parth Gala via GitGitGadgetFrom: Parth Gala <[email protected]>
Currently the two functions use global variable 'the_repository' to access the values stored in it. This makes 'the_repository' to be existent even when not required.
This commit replaces it with 'r' which is passed as a parameter to those functions.
--- builtin/fsck.c | 5 +++-- builtin/index-pack.c | 5 +++-- builtin/name-rev.c | 5 +++-- object.c | 8 ++++---- object.h | 4 ++-- shallow.c | 10 ++++++---- upload-pack.c | 10 ++++++---- 7 files changed, 27 insertions(+), 20 deletions(-)
builtin/fsck.c +3 -2
@@ -375,6 +375,7 @@ static void check_object(struct object *obj)static void check_connectivity (void ) {int i, max; +struct repository *r = the_repository;/* Traverse the pending reachable objects */ traverse_reachable ();
@@ -400,12 +401,12 @@ static void check_connectivity(void) }/* Look up all the requirements, warn about missing objects.. */ - max =get_max_object_index (); + max =get_max_object_index (r);if (verbose)fprintf_ln (stderr,_ ("Checking connectivity (%d objects)" ), max);for (i =0 ; i < max; i++) { -struct object *obj =get_indexed_object (i); +struct object *obj =get_indexed_object (r, i);if (obj)check_object (obj);
builtin/index-pack.c +3 -2
@@ -218,14 +218,15 @@ static unsigned check_object(struct object *obj)static unsigned check_objects (void ) {unsigned i, max, foreign_nr =0 ; +struct repository *r = the_repository; - max =get_max_object_index (); + max =get_max_object_index (r);if (verbose) progress =start_delayed_progress (_ ("Checking objects" ), max);for (i =0 ; i < max; i++) { - foreign_nr +=check_object (get_indexed_object (i)); + foreign_nr +=check_object (get_indexed_object (r, i));display_progress (progress, i +1 ); }
builtin/name-rev.c +3 -2
@@ -456,6 +456,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)int cmd_name_rev (int argc,const char **argv,const char *prefix) {struct object_array revs =OBJECT_ARRAY_INIT ; +struct repository *r = the_repository;int all =0 , transform_stdin =0 , allow_undefined =1 , always =0 , peel_tag =0 ;struct name_ref_data data = {0 ,0 ,STRING_LIST_INIT_NODUP ,STRING_LIST_INIT_NODUP };struct option opts[] = {
@@ -553,9 +554,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) }else if (all) {int i, max; - max =get_max_object_index (); + max =get_max_object_index (r);for (i =0 ; i < max; i++) { -struct object *obj =get_indexed_object (i); +struct object *obj =get_indexed_object (r, i);if (!obj || obj->type !=OBJ_COMMIT )continue ; show_name(obj, NULL,
object.c +4 -4
@@ -10,14 +10,14 @@#include "packfile.h" #include "commit-graph.h" -unsigned int get_max_object_index (void ) +unsigned int get_max_object_index (struct repository *r) { -return the_repository->parsed_objects->obj_hash_size; +return r->parsed_objects->obj_hash_size; } -struct object *get_indexed_object (unsigned int idx) +struct object *get_indexed_object (struct repository *r,unsigned int idx) { -return the_repository->parsed_objects->obj_hash[idx]; +return r->parsed_objects->obj_hash[idx]; }static const char *object_type_strings[] = {
object.h +2 -2
@@ -98,12 +98,12 @@ int type_from_string_gently(const char *str, ssize_t, int gentle);/* * Return the current number of buckets in the object hashmap. */ -unsigned int get_max_object_index (void ); +unsigned int get_max_object_index (struct repository *);/* * Return the object from the specified bucket in the object hashmap. */ -struct object *get_indexed_object (unsigned int ); +struct object *get_indexed_object (struct repository *,unsigned int );/* * This can be used to see if we have heard of the object before, but
shallow.c +6 -4
@@ -510,6 +510,7 @@ static void paint_down(struct paint_info *info, const struct object_id *oid, unsigned int id) {unsigned int i, nr; +struct repository *r = the_repository;struct commit_list *head =NULL ;int bitmap_nr =DIV_ROUND_UP (info->nr_bits,32 );size_t bitmap_size =st_mult (sizeof (uint32_t ), bitmap_nr);
@@ -563,9 +564,9 @@ static void paint_down(struct paint_info *info, const struct object_id *oid, } } - nr =get_max_object_index (); + nr =get_max_object_index (r);for (i =0 ; i < nr; i++) { -struct object *o =get_indexed_object (i); +struct object *o =get_indexed_object (r, i);if (o && o->type ==OBJ_COMMIT ) o->flags &= ~SEEN ; }
@@ -608,6 +609,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,struct object_id *oid = info->shallow->oid;struct oid_array *ref = info->ref;unsigned int i, nr; +struct repository *r = the_repository;int *shallow, nr_shallow =0 ;struct paint_info pi;
@@ -622,9 +624,9 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,* Prepare the commit graph to track what refs can reach what * (new) shallow commits. */ - nr =get_max_object_index (); + nr =get_max_object_index (r);for (i =0 ; i < nr; i++) { -struct object *o =get_indexed_object (i); +struct object *o =get_indexed_object (r, i);if (!o || o->type !=OBJ_COMMIT )continue ;
upload-pack.c +6 -4
@@ -450,6 +450,7 @@ static int do_reachable_revlist(struct child_process *cmd, "rev-list", "--stdin", NULL, };struct object *o; +struct repository *r = the_repository;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ int i;const unsigned hexsz = the_hash_algo->hexsz;
@@ -472,8 +473,8 @@ static int do_reachable_revlist(struct child_process *cmd, namebuf[0 ] ='^' ; namebuf[hexsz +1 ] ='\n' ; -for (i =get_max_object_index ();0 < i; ) { - o =get_indexed_object (--i); +for (i =get_max_object_index (r);0 < i; ) { + o =get_indexed_object (r, --i);if (!o)continue ;if (reachable && o->type ==OBJ_COMMIT )
@@ -520,6 +521,7 @@ static int get_reachable_list(struct object_array *src,struct child_process cmd =CHILD_PROCESS_INIT ;int i;struct object *o; +struct repository *r = the_repository;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ const unsigned hexsz = the_hash_algo->hexsz;
@@ -538,8 +540,8 @@ static int get_reachable_list(struct object_array *src, o->flags &= ~TMP_MARK ; } } -for (i =get_max_object_index ();0 < i; i--) { - o =get_indexed_object (i -1 ); +for (i =get_max_object_index (r);0 < i; i--) { + o =get_indexed_object (r, i -1 );if (o && o->type ==OBJ_COMMIT && (o->flags &TMP_MARK )) {add_object_array (o,NULL , reachable);
Signature · 1 line
gitgitgadget
PATCH 4/5 object.c: clear_object_flags() accept 'r' as parameter
In reply to Parth Gala via GitGitGadgetFrom: Parth Gala <[email protected]>
'clear_object_flags()' and its callers are modified to enable passing 'r' as an argument to 'clear_object_flags()'.
--- builtin/log.c | 3 ++- object.c | 6 +++--- object.h | 2 +- revision.c | 3 ++- shallow.c | 3 ++- upload-pack.c | 2 +- 6 files changed, 11 insertions(+), 8 deletions(-)
builtin/log.c +2 -1
@@ -1592,6 +1592,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) {struct commit *commit;struct commit **list =NULL ; +struct repository *r = the_repository;struct rev_info rev;struct setup_revision_opt s_r_opt;int nr =0 , total, i;
@@ -1998,7 +1999,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)if (base_commit) {struct commit *base =get_base_commit (base_commit, list, nr);reset_revision_walk (); -clear_object_flags (UNINTERESTING ); +clear_object_flags (r,UNINTERESTING );prepare_bases (&bases, base, list, nr); }
object.c +3 -3
@@ -432,12 +432,12 @@ void object_array_remove_duplicates(struct object_array *array) } } -void clear_object_flags (unsigned flags) +void clear_object_flags (struct repository *r,unsigned flags) {int i; -for (i=0 ; i < the_repository->parsed_objects->obj_hash_size; i++) { -struct object *obj = the_repository->parsed_objects->obj_hash[i]; +for (i=0 ; i < r->parsed_objects->obj_hash_size; i++) { +struct object *obj = r->parsed_objects->obj_hash[i];if (obj) obj->flags &= ~flags; }
object.h +1 -1
@@ -185,7 +185,7 @@ void object_array_remove_duplicates(struct object_array *array);*/ void object_array_clear (struct object_array *array); -void clear_object_flags (unsigned flags); +void clear_object_flags (struct repository *,unsigned flags);/* * Clear the specified object flags from all in-core commit objects.
revision.c +2 -1
@@ -3086,7 +3086,8 @@ static void set_children(struct rev_info *revs)void reset_revision_walk (void ) { -clear_object_flags (SEEN |ADDED |SHOWN |TOPO_WALK_EXPLORED |TOPO_WALK_INDEGREE ); +struct repository *r = the_repository; +clear_object_flags (r,SEEN |ADDED |SHOWN |TOPO_WALK_EXPLORED |TOPO_WALK_INDEGREE ); } static int mark_uninteresting(const struct object_id *oid,
shallow.c +2 -1
@@ -180,6 +180,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av, {struct commit_list *result =NULL , *p;struct commit_list *not_shallow_list =NULL ; +struct repository *r = the_repository;struct rev_info revs;int both_flags = shallow_flag | not_shallow_flag;
@@ -187,7 +188,7 @@ struct commit_list *get_shallow_commits_by_rev_list(int ac, const char **av,* SHALLOW (excluded) and NOT_SHALLOW (included) should not be * set at this point. But better be safe than sorry. */ -clear_object_flags (both_flags); +clear_object_flags (r, both_flags);is_repository_shallow (the_repository);/* make sure shallows are read */
upload-pack.c +1 -1
@@ -1472,7 +1472,7 @@ int upload_pack_v2(struct repository *r, struct argv_array *keys,struct object_array have_obj =OBJECT_ARRAY_INIT ;struct object_array want_obj =OBJECT_ARRAY_INIT ; -clear_object_flags (ALL_FLAGS ); +clear_object_flags (r,ALL_FLAGS );git_config (upload_pack_config,NULL );
Signature · 1 line
gitgitgadget
PATCH 2/5 object.c: lookup_unknown_object() accept 'r' as parameter
In reply to Parth Gala via GitGitGadgetFrom: Parth Gala <[email protected]>
'lookup_unknown_object()' and its callers are modified to enable passing 'r' as an argument to 'lookup_unknown_object()' in an effort to reduce dependence on global 'the_repository' variable.
--- builtin/fsck.c | 3 ++- builtin/pack-objects.c | 3 ++- http-push.c | 3 ++- object.c | 8 ++++---- object.h | 2 +- refs.c | 3 ++- t/helper/test-example-decorate.c | 7 ++++--- upload-pack.c | 3 ++- walker.c | 3 ++- 9 files changed, 21 insertions(+), 14 deletions(-)
builtin/fsck.c +2 -1
@@ -745,7 +745,8 @@ static int fsck_cache_tree(struct cache_tree *it)static void mark_object_for_connectivity (const struct object_id *oid) { -struct object *obj =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *obj =lookup_unknown_object (r, oid); obj->flags |=HAS_OBJ ; }
builtin/pack-objects.c +2 -1
@@ -2891,6 +2891,7 @@ static void add_objects_in_unpacked_packs(void) {struct packed_git *p;struct in_pack in_pack; +struct repository *r = the_repository;uint32_t i;memset (&in_pack,0 ,sizeof (in_pack));
@@ -2910,7 +2911,7 @@ static void add_objects_in_unpacked_packs(void)for (i =0 ; i < p->num_objects; i++) {nth_packed_object_oid (&oid, p, i); - o =lookup_unknown_object (&oid); + o =lookup_unknown_object (r, &oid);if (!(o->flags &OBJECT_ADDED ))mark_in_pack_object (o, p, &in_pack); o->flags |=OBJECT_ADDED ;
http-push.c +2 -1
@@ -1416,6 +1416,7 @@ static void one_remote_ref(const char *refname) {struct ref *ref;struct object *obj; +struct repository *r = the_repository; ref =alloc_ref (refname);
@@ -1432,7 +1433,7 @@ static void one_remote_ref(const char *refname)* may be required for updating server info later. */ if (repo->can_update_info_refs && !has_object_file (&ref->old_oid)) { - obj =lookup_unknown_object (&ref->old_oid); + obj =lookup_unknown_object (r, &ref->old_oid);fprintf (stderr," fetch %s for %s\n" ,oid_to_hex (&ref->old_oid), refname);add_fetch_request (obj);
object.c +4 -4
@@ -177,12 +177,12 @@ void *object_as_type(struct repository *r, struct object *obj, enum object_type } } -struct object *lookup_unknown_object (const struct object_id *oid) +struct object *lookup_unknown_object (struct repository *r,const struct object_id *oid) { -struct object *obj =lookup_object (the_repository, oid); +struct object *obj =lookup_object (r, oid);if (!obj) - obj =create_object (the_repository, oid, -alloc_object_node (the_repository)); + obj =create_object (r, oid, +alloc_object_node (r));return obj; }
object.h +1 -1
@@ -144,7 +144,7 @@ struct object *parse_object_or_die(const struct object_id *oid, const char *namestruct object *parse_object_buffer (struct repository *r,const struct object_id *oid,enum object_type type,unsigned long size,void *buffer,int *eaten_p);/** Returns the object, with potentially excess memory allocated. **/ -struct object *lookup_unknown_object (const struct object_id *oid); +struct object *lookup_unknown_object (struct repository *,const struct object_id *oid);struct object_list *object_list_insert (struct object *item,struct object_list **list_p);
refs.c +2 -1
@@ -379,7 +379,8 @@ static int filter_refs(const char *refname, const struct object_id *oid,enum peel_status peel_object (const struct object_id *name,struct object_id *oid) { -struct object *o =lookup_unknown_object (name); +struct repository *r = the_repository; +struct object *o =lookup_unknown_object (r, name);if (o->type ==OBJ_NONE ) {int type =oid_object_info (the_repository, name,NULL );
t/helper/test-example-decorate.c +4 -3
@@ -10,6 +10,7 @@ int cmd__example_decorate(int argc, const char **argv)struct object_id two_oid = { {2 } };struct object_id three_oid = { {3 } };struct object *one, *two, *three; +struct repository *r = the_repository;int decoration_a, decoration_b;
@@ -26,8 +27,8 @@ int cmd__example_decorate(int argc, const char **argv)* Add 2 objects, one with a non-NULL decoration and one with a NULL * decoration. */ - one =lookup_unknown_object (&one_oid); - two =lookup_unknown_object (&two_oid); + one =lookup_unknown_object (r, &one_oid); + two =lookup_unknown_object (r, &two_oid); ret =add_decoration (&n, one, &decoration_a);if (ret)BUG ("when adding a brand-new object, NULL should be returned" );
@@ -56,7 +57,7 @@ int cmd__example_decorate(int argc, const char **argv) ret =lookup_decoration (&n, two);if (ret != &decoration_b)BUG ("lookup should return added declaration" ); - three =lookup_unknown_object (&three_oid); + three =lookup_unknown_object (r, &three_oid); ret =lookup_decoration (&n, three);if (ret)BUG ("lookup for unknown object should return NULL" );
upload-pack.c +2 -1
@@ -962,7 +962,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wanstatic int mark_our_ref (const char *refname,const char *refname_full,const struct object_id *oid) { -struct object *o =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *o =lookup_unknown_object (r, oid);if (ref_is_hidden (refname, refname_full)) { o->flags |=HIDDEN_REF ;
walker.c +2 -1
@@ -258,6 +258,7 @@ void walker_targets_free(int targets, char **target, const char **write_ref)int walker_fetch (struct walker *walker,int targets,char **target,const char **write_ref,const char *write_ref_log_details) { +struct repository *r = the_repository;struct strbuf refname =STRBUF_INIT ;struct strbuf err =STRBUF_INIT ;struct ref_transaction *transaction =NULL ;
@@ -285,7 +286,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,error ("Could not interpret response from server '%s' as something to pull" , target[i]); goto done; } -if (process (walker,lookup_unknown_object (&oids[i]))) +if (process (walker,lookup_unknown_object (r, &oids[i]))) goto done; }
Signature · 1 line
gitgitgadget
PATCH 5/5 object.c: clear_commit_marks_all() accept 'r' as parameter
In reply to Parth Gala via GitGitGadgetFrom: Parth Gala <[email protected]>
'clear_commit_marks_all()' and its callers are modified to enable passing 'r' as an argument to 'clear_commit_marks_all()'.
--- builtin/checkout.c | 3 ++- object.c | 6 +++--- object.h | 2 +- 3 files changed, 6 insertions(+), 5 deletions(-)
builtin/checkout.c +2 -1
@@ -989,6 +989,7 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)static void orphaned_commit_warning (struct commit *old_commit,struct commit *new_commit) {struct rev_info revs; +struct repository *r = the_repository;struct object *object = &old_commit->object;repo_init_revisions (the_repository, &revs,NULL );
@@ -1011,7 +1012,7 @@ static void orphaned_commit_warning(struct commit *old_commit, struct commit *nedescribe_detached_head (_ ("Previous HEAD position was" ), old_commit);/* Clean up objects used, as they will be reused. */ -clear_commit_marks_all (ALL_REV_FLAGS ); +clear_commit_marks_all (r,ALL_REV_FLAGS ); } static int switch_branches(const struct checkout_opts *opts,
object.c +3 -3
@@ -443,12 +443,12 @@ void clear_object_flags(struct repository *r, unsigned flags) } } -void clear_commit_marks_all (unsigned int flags) +void clear_commit_marks_all (struct repository *r,unsigned int flags) {int i; -for (i =0 ; i < the_repository->parsed_objects->obj_hash_size; i++) { -struct object *obj = the_repository->parsed_objects->obj_hash[i]; +for (i =0 ; i < r->parsed_objects->obj_hash_size; i++) { +struct object *obj = r->parsed_objects->obj_hash[i];if (obj && obj->type ==OBJ_COMMIT ) obj->flags &= ~flags; }
object.h +1 -1
@@ -190,6 +190,6 @@ void clear_object_flags(struct repository *, unsigned flags);/* * Clear the specified object flags from all in-core commit objects. */ -void clear_commit_marks_all (unsigned int flags); +void clear_commit_marks_all (struct repository *,unsigned int flags);#endif /* OBJECT_H */
Signature · 1 line
gitgitgadget
Hi Parth,
Quoted 2 lines · Parth Gala via GitGitGadget
I have created this commit in response to
https://github.com/gitgitgadget/git/issues/379#issue-503866117.
Fantastic! Thank you for working on this.
Quoted 6 lines
All the functions in
object.cwhich relied on 'the_repository' have been modified. The functions calling these functions inobject.cconsisted calls to other functions using 'the_repository' as well, and although I intended to use 'r' at all those instances, I thought it would make more sense when we would deal with their callee functions in another similar patch. What do you think ?
That makes sense, and follows the conventions that other similar refactorings have done in the past.
Any reason why you decided to start with 'object.c'? Not that any such reason is necessary, I'm just curious about how you came to this decision.
Quoted 1 line
Signed-off-by Parth Gala [email protected] [[email protected]]
Even though you *do* need a 'Signed-off-by' in each of your patches, adding it in the cover letter is not necessary.
Quoted 31 lines
Parth Gala (5):
object.c:get_max_object_indexandget_indexed_objectaccept 'r'
parameter
object.c:lookup_unknown_object()accept 'r' as parameterobject.c:parse_object_or_die()accept 'r' as parameterobject.c:clear_object_flags()accept 'r' as parameterobject.c:clear_commit_marks_all()accept 'r' as parameterbuiltin/checkout.c | 3 ++- builtin/fsck.c | 8 +++++--- builtin/grep.c | 6 ++++-- builtin/index-pack.c | 5 +++-- builtin/log.c | 3 ++- builtin/name-rev.c | 5 +++-- builtin/pack-objects.c | 3 ++- builtin/prune.c | 3 ++- bundle.c | 8 +++++--- http-push.c | 3 ++- object.c | 32 ++++++++++++++++---------------- object.h | 12 ++++++------ pack-bitmap.c | 5 +++-- reachable.c | 6 ++++-- refs.c | 3 ++- revision.c | 3 ++- shallow.c | 13 ++++++++----- t/helper/test-example-decorate.c | 7 ++++--- upload-pack.c | 19 ++++++++++++------- walker.c | 3 ++- 20 files changed, 89 insertions(+), 61 deletions(-)
Quoted 6 lines
base-commit:
0ad714499976290d9a0229230cbe4efae930b8dc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-545%2FParthGala2k%2Flocalize-the_repository-variable-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-545/ParthGala2k/localize-the_repository-variable-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/545
--
gitgitgadget
Thanks,
Taylor
Re: [PATCH 1/5] object.c: get_max_object_index and get_indexed_object accept 'r' parameter
In reply to Parth Gala via GitGitGadgetQuoted 5 lines · Parth Gala via GitGitGadget
From: Parth Gala <[email protected]>
Currently the two functions use global variable '
the_repository' to access the values stored in it. This makes 'the_repository' to be existent even when not required.
Nit: please wrap your commit messages at 72 characters instead of 80.
Quoted 2 lines
This commit replaces it with 'r' which is passed as a parameter to those functions
Makes sense.
Quoted 16 lines
Signed-off-by Parth Gala <[email protected]>--- builtin/fsck.c | 5 +++-- builtin/index-pack.c | 5 +++-- builtin/name-rev.c | 5 +++-- object.c | 8 ++++---- object.h | 4 ++-- shallow.c | 10 ++++++---- upload-pack.c | 10 ++++++---- 7 files changed, 27 insertions(+), 20 deletions(-)builtin/fsck.c +0
-0
builtin/fsck.c static void check_object(struct object *obj) · line 378
Quoted 4 lines
builtin/fsck.c +1
-0@@ -375,6 +375,7 @@ static void check_object(struct object *obj)static void check_connectivity (void ) {int i, max; +struct repository *r = the_repository;
Is there a reason that you assign use/assign 'r' here? I can find a few other such instances of it in:
$ git grep -l 'struct repository \*r = ' builtin/merge-tree.c builtin/sparse-checkout.c builtin/update-index.c fetch-pack.c read-cache.c t/helper/test-reach.c tree.c
but I'm not sure that it's necessary here. Could you instead pass 'the_repository' directly to the functions that now require it?
builtin/fsck.c static void check_connectivity(void) · line 404
Quoted 8 lines
builtin/fsck.c +1
-1/* Traverse the pending reachable objects */ traverse_reachable ();@@ -400,12 +401,12 @@ static void check_connectivity(void) }/* Look up all the requirements, warn about missing objects.. */ - max =get_max_object_index (); + max =get_max_object_index (r);
For example, changing this line to:
max = get_max_object_index(the_repository);
Quoted 184 lines
builtin/fsck.c +1
-1if (verbose)fprintf_ln (stderr,_ ("Checking connectivity (%d objects)" ), max);for (i =0 ; i < max; i++) { -struct object *obj =get_indexed_object (i); +struct object *obj =get_indexed_object (r, i);if (obj)check_object (obj);builtin/index-pack.c +3
-2@@ -218,14 +218,15 @@ static unsigned check_object(struct object *obj)static unsigned check_objects (void ) {unsigned i, max, foreign_nr =0 ; +struct repository *r = the_repository; - max =get_max_object_index (); + max =get_max_object_index (r);if (verbose) progress =start_delayed_progress (_ ("Checking objects" ), max);for (i =0 ; i < max; i++) { - foreign_nr +=check_object (get_indexed_object (i)); + foreign_nr +=check_object (get_indexed_object (r, i));display_progress (progress, i +1 ); }builtin/name-rev.c +3
-2@@ -456,6 +456,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)int cmd_name_rev (int argc,const char **argv,const char *prefix) {struct object_array revs =OBJECT_ARRAY_INIT ; +struct repository *r = the_repository;int all =0 , transform_stdin =0 , allow_undefined =1 , always =0 , peel_tag =0 ;struct name_ref_data data = {0 ,0 ,STRING_LIST_INIT_NODUP ,STRING_LIST_INIT_NODUP };struct option opts[] = {@@ -553,9 +554,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) }else if (all) {int i, max; - max =get_max_object_index (); + max =get_max_object_index (r);for (i =0 ; i < max; i++) { -struct object *obj =get_indexed_object (i); +struct object *obj =get_indexed_object (r, i);if (!obj || obj->type !=OBJ_COMMIT )continue ; show_name(obj, NULL,object.c +4
-4@@ -10,14 +10,14 @@#include "packfile.h" #include "commit-graph.h" -unsigned int get_max_object_index (void ) +unsigned int get_max_object_index (struct repository *r) { -return the_repository->parsed_objects->obj_hash_size; +return r->parsed_objects->obj_hash_size; } -struct object *get_indexed_object (unsigned int idx) +struct object *get_indexed_object (struct repository *r,unsigned int idx) { -return the_repository->parsed_objects->obj_hash[idx]; +return r->parsed_objects->obj_hash[idx]; }static const char *object_type_strings[] = {object.h +2
-2@@ -98,12 +98,12 @@ int type_from_string_gently(const char *str, ssize_t, int gentle);/* * Return the current number of buckets in the object hashmap. */ -unsigned int get_max_object_index (void ); +unsigned int get_max_object_index (struct repository *);/* * Return the object from the specified bucket in the object hashmap. */ -struct object *get_indexed_object (unsigned int ); +struct object *get_indexed_object (struct repository *,unsigned int );/* * This can be used to see if we have heard of the object before, but shallow.c +6
-4@@ -510,6 +510,7 @@ static void paint_down(struct paint_info *info, const struct object_id *oid, unsigned int id) {unsigned int i, nr; +struct repository *r = the_repository;struct commit_list *head =NULL ;int bitmap_nr =DIV_ROUND_UP (info->nr_bits,32 );size_t bitmap_size =st_mult (sizeof (uint32_t ), bitmap_nr);@@ -563,9 +564,9 @@ static void paint_down(struct paint_info *info, const struct object_id *oid, } } - nr =get_max_object_index (); + nr =get_max_object_index (r);for (i =0 ; i < nr; i++) { -struct object *o =get_indexed_object (i); +struct object *o =get_indexed_object (r, i);if (o && o->type ==OBJ_COMMIT ) o->flags &= ~SEEN ; }@@ -608,6 +609,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,struct object_id *oid = info->shallow->oid;struct oid_array *ref = info->ref;unsigned int i, nr; +struct repository *r = the_repository;int *shallow, nr_shallow =0 ;struct paint_info pi;@@ -622,9 +624,9 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,* Prepare the commit graph to track what refs can reach what * (new) shallow commits. */ - nr =get_max_object_index (); + nr =get_max_object_index (r);for (i =0 ; i < nr; i++) { -struct object *o =get_indexed_object (i); +struct object *o =get_indexed_object (r, i);if (!o || o->type !=OBJ_COMMIT )continue ;upload-pack.c +6
-5@@ -450,6 +450,7 @@ static int do_reachable_revlist(struct child_process *cmd, "rev-list", "--stdin", NULL, };struct object *o; +struct repository *r = the_repository;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ int i;const unsigned hexsz = the_hash_algo->hexsz;@@ -472,8 +473,8 @@ static int do_reachable_revlist(struct child_process *cmd, namebuf[0 ] ='^' ; namebuf[hexsz +1 ] ='\n' ; -for (i =get_max_object_index ();0 < i; ) { - o =get_indexed_object (--i); +for (i =get_max_object_index (r);0 < i; ) { + o =get_indexed_object (r, --i);if (!o)continue ;if (reachable && o->type ==OBJ_COMMIT )@@ -520,6 +521,7 @@ static int get_reachable_list(struct object_array *src,struct child_process cmd =CHILD_PROCESS_INIT ;int i;struct object *o; +struct repository *r = the_repository;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ const unsigned hexsz = the_hash_algo->hexsz;@@ -538,8 +540,8 @@ static int get_reachable_list(struct object_array *src, o->flags &= ~TMP_MARK ; } } -for (i =get_max_object_index ();0 < i; i--) { - o =get_indexed_object (i -1 ); +for (i =get_max_object_index (r);0 < i; i--) { + o =get_indexed_object (r, i -1 );if (o && o->type ==OBJ_COMMIT && (o->flags &TMP_MARK )) {add_object_array (o,NULL , reachable); --
Quoted 1 line
gitgitgadget
Otherwise this looks pretty good.
Thanks,
Taylor
Re: [PATCH 2/5] object.c: lookup_unknown_object() accept 'r' as parameter
In reply to Parth Gala via GitGitGadgetQuoted 5 lines · Parth Gala via GitGitGadget
From: Parth Gala <[email protected]>
'
lookup_unknown_object()' and its callers are modified to enable passing 'r' as an argument to 'lookup_unknown_object()' in an effort to reduce dependence on global 'the_repository' variable.
The changes in 'object.[ch]' look sane to me here, again, but I have the same question about why assigning:
struct repository *r = the_repository;
and passing 'r' everywhere is preferable to simply passing 'the_repository' in directly.
Quoted 185 lines
Signed-off-by Parth Gala <[email protected]>--- builtin/fsck.c | 3 ++- builtin/pack-objects.c | 3 ++- http-push.c | 3 ++- object.c | 8 ++++---- object.h | 2 +- refs.c | 3 ++- t/helper/test-example-decorate.c | 7 ++++--- upload-pack.c | 3 ++- walker.c | 3 ++- 9 files changed, 21 insertions(+), 14 deletions(-)builtin/fsck.c +2
-1@@ -745,7 +745,8 @@ static int fsck_cache_tree(struct cache_tree *it)static void mark_object_for_connectivity (const struct object_id *oid) { -struct object *obj =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *obj =lookup_unknown_object (r, oid); obj->flags |=HAS_OBJ ; }builtin/pack-objects.c +2
-1@@ -2891,6 +2891,7 @@ static void add_objects_in_unpacked_packs(void) {struct packed_git *p;struct in_pack in_pack; +struct repository *r = the_repository;uint32_t i;memset (&in_pack,0 ,sizeof (in_pack));@@ -2910,7 +2911,7 @@ static void add_objects_in_unpacked_packs(void)for (i =0 ; i < p->num_objects; i++) {nth_packed_object_oid (&oid, p, i); - o =lookup_unknown_object (&oid); + o =lookup_unknown_object (r, &oid);if (!(o->flags &OBJECT_ADDED ))mark_in_pack_object (o, p, &in_pack); o->flags |=OBJECT_ADDED ;http-push.c +2
-1@@ -1416,6 +1416,7 @@ static void one_remote_ref(const char *refname) {struct ref *ref;struct object *obj; +struct repository *r = the_repository; ref =alloc_ref (refname);@@ -1432,7 +1433,7 @@ static void one_remote_ref(const char *refname)* may be required for updating server info later. */ if (repo->can_update_info_refs && !has_object_file (&ref->old_oid)) { - obj =lookup_unknown_object (&ref->old_oid); + obj =lookup_unknown_object (r, &ref->old_oid);fprintf (stderr," fetch %s for %s\n" ,oid_to_hex (&ref->old_oid), refname);add_fetch_request (obj);object.c +4
-4@@ -177,12 +177,12 @@ void *object_as_type(struct repository *r, struct object *obj, enum object_type } } -struct object *lookup_unknown_object (const struct object_id *oid) +struct object *lookup_unknown_object (struct repository *r,const struct object_id *oid) { -struct object *obj =lookup_object (the_repository, oid); +struct object *obj =lookup_object (r, oid);if (!obj) - obj =create_object (the_repository, oid, -alloc_object_node (the_repository)); + obj =create_object (r, oid, +alloc_object_node (r));return obj; }object.h +1
-1@@ -144,7 +144,7 @@ struct object *parse_object_or_die(const struct object_id *oid, const char *namestruct object *parse_object_buffer (struct repository *r,const struct object_id *oid,enum object_type type,unsigned long size,void *buffer,int *eaten_p);/** Returns the object, with potentially excess memory allocated. **/ -struct object *lookup_unknown_object (const struct object_id *oid); +struct object *lookup_unknown_object (struct repository *,const struct object_id *oid);struct object_list *object_list_insert (struct object *item,struct object_list **list_p);refs.c +2
-1@@ -379,7 +379,8 @@ static int filter_refs(const char *refname, const struct object_id *oid,enum peel_status peel_object (const struct object_id *name,struct object_id *oid) { -struct object *o =lookup_unknown_object (name); +struct repository *r = the_repository; +struct object *o =lookup_unknown_object (r, name);if (o->type ==OBJ_NONE ) {int type =oid_object_info (the_repository, name,NULL );t/helper/test-example-decorate.c +4
-3@@ -10,6 +10,7 @@ int cmd__example_decorate(int argc, const char **argv)struct object_id two_oid = { {2 } };struct object_id three_oid = { {3 } };struct object *one, *two, *three; +struct repository *r = the_repository;int decoration_a, decoration_b;@@ -26,8 +27,8 @@ int cmd__example_decorate(int argc, const char **argv)* Add 2 objects, one with a non-NULL decoration and one with a NULL * decoration. */ - one =lookup_unknown_object (&one_oid); - two =lookup_unknown_object (&two_oid); + one =lookup_unknown_object (r, &one_oid); + two =lookup_unknown_object (r, &two_oid); ret =add_decoration (&n, one, &decoration_a);if (ret)BUG ("when adding a brand-new object, NULL should be returned" );@@ -56,7 +57,7 @@ int cmd__example_decorate(int argc, const char **argv) ret =lookup_decoration (&n, two);if (ret != &decoration_b)BUG ("lookup should return added declaration" ); - three =lookup_unknown_object (&three_oid); + three =lookup_unknown_object (r, &three_oid); ret =lookup_decoration (&n, three);if (ret)BUG ("lookup for unknown object should return NULL" );upload-pack.c +2
-1@@ -962,7 +962,8 @@ static void receive_needs(struct packet_reader *reader, struct object_array *wanstatic int mark_our_ref (const char *refname,const char *refname_full,const struct object_id *oid) { -struct object *o =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *o =lookup_unknown_object (r, oid);if (ref_is_hidden (refname, refname_full)) { o->flags |=HIDDEN_REF ;walker.c +2
-2@@ -258,6 +258,7 @@ void walker_targets_free(int targets, char **target, const char **write_ref)int walker_fetch (struct walker *walker,int targets,char **target,const char **write_ref,const char *write_ref_log_details) { +struct repository *r = the_repository;struct strbuf refname =STRBUF_INIT ;struct strbuf err =STRBUF_INIT ;struct ref_transaction *transaction =NULL ;@@ -285,7 +286,7 @@ int walker_fetch(struct walker *walker, int targets, char **target,error ("Could not interpret response from server '%s' as something to pull" , target[i]); goto done; } -if (process (walker,lookup_unknown_object (&oids[i]))) +if (process (walker,lookup_unknown_object (r, &oids[i]))) goto done; } --
Quoted 2 lines
gitgitgadget
Thanks,
Taylor
Re: [PATCH 2/5] object.c: lookup_unknown_object() accept 'r' as parameter
Quoted 7 lines · Taylor Blau
... same question about why assigning:
struct repository *r = the_repository;and passing 'r' everywhere is preferable to simply passing '
the_repository' in directly.
...
Quoted 7 lines
builtin/fsck.c static int fsck_cache_tree(struct cache_tree *it) · line 751
Quoted 7 lines
static void
mark_object_for_connectivity(const struct object_id *oid){builtin/fsck.c +2
-1-struct object *obj =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *obj =lookup_unknown_object (r, oid); obj->flags |=HAS_OBJ ; }
I do not claim that it applies to this particular function, and the function is too small for it to matter, but when a function is large enough and it always works on one single repository, it would make it easier to later update the function further to set up 'r' upfront, making it point at the_repository for now, and to use 'r' throughout the function. That way, when the time comes to update the function to work on an arbitrary repository, the only change required will be to turn the local variable 'r' to an incoming parameter the caller can supply.
Re: [PATCH 1/5] object.c: get_max_object_index and get_indexed_object accept 'r' parameter
In reply to Taylor BlauQuoted 11 lines · Taylor Blau
builtin/fsck.c static void check_object(struct object *obj) · line 378
Quoted 6 lines · Parth Gala via GitGitGadget
builtin/fsck.c +1
-0@@ -375,6 +375,7 @@ static void check_object(struct object *obj)static void check_connectivity (void ) {int i, max; +struct repository *r = the_repository;Is there a reason that you assign use/assign 'r' here? [...] but I'm not sure that it's necessary here. Could you instead pass '
the_repository' directly to the functions that now require it?
One benefit of doing it this way is that future patches will be much less noisy which convert these callers to also work with any 'repository' object. For instance, after the current patch, we have:
static void check_connectivity(void)
{
struct repository *r = the_repository;
max = get_max_object_index(r);
...A future patch which converts check_connectivity() to work with any repository will then require very little change -- just adding an 'r' argument to the function declaration, and dropping the line which declares 'r':
static void check_connectivity(struct repository *r)
{
max = get_max_object_index(r);
...So, I'm fine with this patch series' approach of declaring a variable 'r' like this.
Quoted 13 lines · Taylor Blau
Hi Parth,
Quoted 2 lines · Parth Gala via GitGitGadget
I have created this commit in response to
https://github.com/gitgitgadget/git/issues/379#issue-503866117.Fantastic! Thank you for working on this.
Quoted 6 lines
All the functions in
object.cwhich relied on 'the_repository' have been modified. The functions calling these functions inobject.cconsisted calls to other functions using 'the_repository' as well, and although I intended to use 'r' at all those instances, I thought it would make more sense when we would deal with their callee functions in another similar patch. What do you think ?
Quoted 6 lines
That makes sense, and follows the conventions that other similar refactorings have done in the past.
Any reason why you decided to start with '
object.c'? Not that any such reason is necessary, I'm just curious about how you came to this decision.
I am new to the git community and while searching for issues to solve I found the issue linked above. I figured solving it would give me a good experience navigating between functions and exploring the huge repository.
Initially I grepped to find all functions using the_repository but randomly chose object.c since the refactoring had to start somewhere. Special thanks to Johannes Schindelin for this.
Quoted 3 lines
Quoted 1 line
Signed-off-by Parth Gala [email protected] [[email protected]]Even though you *do* need a 'Signed-off-by' in each of your patches, adding it in the cover letter is not necessary.
I probably did not check the preview I sent to myself carefully for this. Will make note of it.
Quoted 39 lines
Quoted 31 lines
Parth Gala (5):
object.c:get_max_object_indexandget_indexed_objectaccept 'r'
parameter
object.c:lookup_unknown_object()accept 'r' as parameterobject.c:parse_object_or_die()accept 'r' as parameterobject.c:clear_object_flags()accept 'r' as parameterobject.c:clear_commit_marks_all()accept 'r' as parameterbuiltin/checkout.c | 3 ++- builtin/fsck.c | 8 +++++--- builtin/grep.c | 6 ++++-- builtin/index-pack.c | 5 +++-- builtin/log.c | 3 ++- builtin/name-rev.c | 5 +++-- builtin/pack-objects.c | 3 ++- builtin/prune.c | 3 ++- bundle.c | 8 +++++--- http-push.c | 3 ++- object.c | 32 ++++++++++++++++---------------- object.h | 12 ++++++------ pack-bitmap.c | 5 +++-- reachable.c | 6 ++++-- refs.c | 3 ++- revision.c | 3 ++- shallow.c | 13 ++++++++----- t/helper/test-example-decorate.c | 7 ++++--- upload-pack.c | 19 ++++++++++++------- walker.c | 3 ++- 20 files changed, 89 insertions(+), 61 deletions(-)Quoted 6 lines
base-commit:
0ad714499976290d9a0229230cbe4efae930b8dc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-545%2FParthGala2k%2Flocalize-the_repository-variable-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-545/ParthGala2k/localize-the_repository-variable-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/545
--
gitgitgadgetThanks,
Taylor
Re: [PATCH 1/5] object.c: get_max_object_index and get_indexed_object accept 'r' parameter
In reply to Taylor BlauQuoted 7 lines · Taylor Blau
Quoted 5 lines · Parth Gala via GitGitGadget
From: Parth Gala <[email protected]>
Currently the two functions use global variable '
the_repository' to access the values stored in it. This makes 'the_repository' to be existent even when not required.Nit: please wrap your commit messages at 72 characters instead of 80.
Alright.
Quoted 36 lines
Quoted 2 lines
This commit replaces it with 'r' which is passed as a parameter to those functions
Makes sense.
Quoted 16 lines
Signed-off-by Parth Gala <[email protected]>--- builtin/fsck.c | 5 +++-- builtin/index-pack.c | 5 +++-- builtin/name-rev.c | 5 +++-- object.c | 8 ++++---- object.h | 4 ++-- shallow.c | 10 ++++++---- upload-pack.c | 10 ++++++---- 7 files changed, 27 insertions(+), 20 deletions(-)builtin/fsck.c +0
-0builtin/fsck.c static void check_object(struct object *obj) · line 378
Quoted 4 lines
builtin/fsck.c +1
-0@@ -375,6 +375,7 @@ static void check_object(struct object *obj)static void check_connectivity (void ) {int i, max; +struct repository *r = the_repository;Is there a reason that you assign use/assign 'r' here? I can find a few other such instances of it in:
$ git grep -l 'struct repository \*r = ' builtin/merge-tree.c builtin/sparse-checkout.c builtin/update-index.c fetch-pack.c read-cache.c t/helper/test-reach.c tree.c
Quoted 2 lines
but I'm not sure that it's necessary here. Could you instead pass '
the_repository' directly to the functions that now require it?
This was the more convenient thing to do, given that the ultimate goal is to actually use the r that is passed to these caller functions rather than defining them locally, which would require another layer of refactoring.
Besides it is what was decided in the issue.
Also, it would be easier to differentiate whether or not a function has been refactored, upon seeing an r rather than the_repository(which is otherwise commonly used in a global scope) ie. you would be confused whether the_repository used here was passed to the function or not(especially in longer functions where you'd have to scroll up to the function definition to check).
Quoted 201 lines
builtin/fsck.c static void check_connectivity(void) · line 404
Quoted 8 lines
builtin/fsck.c +1
-1/* Traverse the pending reachable objects */ traverse_reachable ();@@ -400,12 +401,12 @@ static void check_connectivity(void) }/* Look up all the requirements, warn about missing objects.. */ - max =get_max_object_index (); + max =get_max_object_index (r);For example, changing this line to:
max = get_max_object_index(the_repository);Quoted 184 lines
builtin/fsck.c +1
-1if (verbose)fprintf_ln (stderr,_ ("Checking connectivity (%d objects)" ), max);for (i =0 ; i < max; i++) { -struct object *obj =get_indexed_object (i); +struct object *obj =get_indexed_object (r, i);if (obj)check_object (obj);builtin/index-pack.c +3
-2@@ -218,14 +218,15 @@ static unsigned check_object(struct object *obj)static unsigned check_objects (void ) {unsigned i, max, foreign_nr =0 ; +struct repository *r = the_repository; - max =get_max_object_index (); + max =get_max_object_index (r);if (verbose) progress =start_delayed_progress (_ ("Checking objects" ), max);for (i =0 ; i < max; i++) { - foreign_nr +=check_object (get_indexed_object (i)); + foreign_nr +=check_object (get_indexed_object (r, i));display_progress (progress, i +1 ); }builtin/name-rev.c +3
-2@@ -456,6 +456,7 @@ static void name_rev_line(char *p, struct name_ref_data *data)int cmd_name_rev (int argc,const char **argv,const char *prefix) {struct object_array revs =OBJECT_ARRAY_INIT ; +struct repository *r = the_repository;int all =0 , transform_stdin =0 , allow_undefined =1 , always =0 , peel_tag =0 ;struct name_ref_data data = {0 ,0 ,STRING_LIST_INIT_NODUP ,STRING_LIST_INIT_NODUP };struct option opts[] = {@@ -553,9 +554,9 @@ int cmd_name_rev(int argc, const char **argv, const char *prefix) }else if (all) {int i, max; - max =get_max_object_index (); + max =get_max_object_index (r);for (i =0 ; i < max; i++) { -struct object *obj =get_indexed_object (i); +struct object *obj =get_indexed_object (r, i);if (!obj || obj->type !=OBJ_COMMIT )continue ; show_name(obj, NULL,object.c +4
-4@@ -10,14 +10,14 @@#include "packfile.h" #include "commit-graph.h" -unsigned int get_max_object_index (void ) +unsigned int get_max_object_index (struct repository *r) { -return the_repository->parsed_objects->obj_hash_size; +return r->parsed_objects->obj_hash_size; } -struct object *get_indexed_object (unsigned int idx) +struct object *get_indexed_object (struct repository *r,unsigned int idx) { -return the_repository->parsed_objects->obj_hash[idx]; +return r->parsed_objects->obj_hash[idx]; }static const char *object_type_strings[] = {object.h +2
-2@@ -98,12 +98,12 @@ int type_from_string_gently(const char *str, ssize_t, int gentle);/* * Return the current number of buckets in the object hashmap. */ -unsigned int get_max_object_index (void ); +unsigned int get_max_object_index (struct repository *);/* * Return the object from the specified bucket in the object hashmap. */ -struct object *get_indexed_object (unsigned int ); +struct object *get_indexed_object (struct repository *,unsigned int );/* * This can be used to see if we have heard of the object before, but shallow.c +6
-4@@ -510,6 +510,7 @@ static void paint_down(struct paint_info *info, const struct object_id *oid, unsigned int id) {unsigned int i, nr; +struct repository *r = the_repository;struct commit_list *head =NULL ;int bitmap_nr =DIV_ROUND_UP (info->nr_bits,32 );size_t bitmap_size =st_mult (sizeof (uint32_t ), bitmap_nr);@@ -563,9 +564,9 @@ static void paint_down(struct paint_info *info, const struct object_id *oid, } } - nr =get_max_object_index (); + nr =get_max_object_index (r);for (i =0 ; i < nr; i++) { -struct object *o =get_indexed_object (i); +struct object *o =get_indexed_object (r, i);if (o && o->type ==OBJ_COMMIT ) o->flags &= ~SEEN ; }@@ -608,6 +609,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,struct object_id *oid = info->shallow->oid;struct oid_array *ref = info->ref;unsigned int i, nr; +struct repository *r = the_repository;int *shallow, nr_shallow =0 ;struct paint_info pi;@@ -622,9 +624,9 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,* Prepare the commit graph to track what refs can reach what * (new) shallow commits. */ - nr =get_max_object_index (); + nr =get_max_object_index (r);for (i =0 ; i < nr; i++) { -struct object *o =get_indexed_object (i); +struct object *o =get_indexed_object (r, i);if (!o || o->type !=OBJ_COMMIT )continue ;upload-pack.c +6
-5@@ -450,6 +450,7 @@ static int do_reachable_revlist(struct child_process *cmd, "rev-list", "--stdin", NULL, };struct object *o; +struct repository *r = the_repository;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ int i;const unsigned hexsz = the_hash_algo->hexsz;@@ -472,8 +473,8 @@ static int do_reachable_revlist(struct child_process *cmd, namebuf[0 ] ='^' ; namebuf[hexsz +1 ] ='\n' ; -for (i =get_max_object_index ();0 < i; ) { - o =get_indexed_object (--i); +for (i =get_max_object_index (r);0 < i; ) { + o =get_indexed_object (r, --i);if (!o)continue ;if (reachable && o->type ==OBJ_COMMIT )@@ -520,6 +521,7 @@ static int get_reachable_list(struct object_array *src,struct child_process cmd =CHILD_PROCESS_INIT ;int i;struct object *o; +struct repository *r = the_repository;char namebuf[GIT_MAX_HEXSZ +2 ];/* ^ + hash + LF */ const unsigned hexsz = the_hash_algo->hexsz;@@ -538,8 +540,8 @@ static int get_reachable_list(struct object_array *src, o->flags &= ~TMP_MARK ; } } -for (i =get_max_object_index ();0 < i; i--) { - o =get_indexed_object (i -1 ); +for (i =get_max_object_index (r);0 < i; i--) { + o =get_indexed_object (r, i -1 );if (o && o->type ==OBJ_COMMIT && (o->flags &TMP_MARK )) {add_object_array (o,NULL , reachable); --Quoted 1 line
gitgitgadget
Otherwise this looks pretty good.
Thanks,
Taylor
Re: [PATCH 2/5] object.c: lookup_unknown_object() accept 'r' as parameter
In reply to Junio C HamanoQuoted 18 lines · Junio C Hamano
Quoted 7 lines · Taylor Blau
... same question about why assigning:
struct repository *r = the_repository;and passing 'r' everywhere is preferable to simply passing '
the_repository' in directly.
...Quoted 7 lines
builtin/fsck.c static int fsck_cache_tree(struct cache_tree *it) · line 751
Quoted 7 lines
static void
mark_object_for_connectivity(const struct object_id *oid){builtin/fsck.c +2
-1-struct object *obj =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *obj =lookup_unknown_object (r, oid); obj->flags |=HAS_OBJ ; }I do not claim that it applies to this particular function, and the
Quoted 8 lines
function is too small for it to matter, but when a function is large enough and it always works on one single repository, it would make it easier to later update the function further to set up 'r' upfront, making it point at
the_repositoryfor now, and to use 'r' throughout the function. That way, when the time comes to update the function to work on an arbitrary repository, the only change required will be to turn the local variable 'r' to an incoming parameter the caller can supply.
Right, but my suggestion was that this advice doesn't apply to this particular instance since I don't expect that we'd ever passing something other than 'the_repository'.
Specifically, I was worried that we'd get bitten by re-assigning 'r' in the middle of the function and then end up in some odd broken state.
Maybe I'm worrying too much.
Thanks,
Taylor
Re: [PATCH 2/5] object.c: lookup_unknown_object() accept 'r' as parameter
Quoted 18 lines · Taylor Blau
Quoted 9 lines · Junio C Hamano
Quoted 7 lines · Taylor Blau
... same question about why assigning:
struct repository *r = the_repository;and passing 'r' everywhere is preferable to simply passing '
the_repository' in directly.
...Quoted 8 lines
Quoted 7 lines
Quoted 7 lines
static void
mark_object_for_connectivity(const struct object_id *oid){builtin/fsck.c +2
-1-struct object *obj =lookup_unknown_object (oid); +struct repository *r = the_repository; +struct object *obj =lookup_unknown_object (r, oid); obj->flags |=HAS_OBJ ; }...
Quoted 6 lines
Right, but my suggestion was that this advice doesn't apply to this particular instance since I don't expect that we'd ever passing something other than '
the_repository'.Specifically, I was worried that we'd get bitten by re-assigning 'r' in the middle of the function and then end up in some odd broken state.
"git fsck" works only in a single, "the", repository, so I guess you are right to be worried about unnecessary complexity here.
Re: [PATCH 2/5] object.c: lookup_unknown_object() accept 'r' as parameter
Quoted 9 lines · Junio C Hamano
Quoted 6 lines
Right, but my suggestion was that this advice doesn't apply to this particular instance since I don't expect that we'd ever passing something other than '
the_repository'.Specifically, I was worried that we'd get bitten by re-assigning 'r' in the middle of the function and then end up in some odd broken state.
"git fsck" works only in a single, "the", repository, so I guess you are right to be worried about unnecessary complexity here.
I think the end-game for this whole repository transition would be to get rid of the_repository, though. I.e., I'd envision the progression something like this:
1. Teach all of the library code to take (and operate on) "struct repository".
2. Teach static local functions like this to pass in the_repository.
3. Teach top-level commands like cmd_fsck() to pass the_repository to all of those static local helpers.
4. Teach top-level commands to get a real repository pointer, either from the git.c wrapper (when RUN_SETUP is used) or by calling setup_git_repository() themselves.
5. Grep for the_repository and drop it everywhere.
Here we're at step 2 now, but declaring "r" makes moving to step 3 just a little easier. And I think the existence of steps 4 and 5 implies that it would eventually be worth going through step 3.
Of course I just wrote those steps down for the first time, so maybe nobody else shares my vision. ;)
-Peff
Re: [PATCH 2/5] object.c: lookup_unknown_object() accept 'r' as parameter
Quoted 28 lines · Jeff King
Quoted 9 lines · Junio C Hamano
Quoted 6 lines
Right, but my suggestion was that this advice doesn't apply to this particular instance since I don't expect that we'd ever passing something other than '
the_repository'.Specifically, I was worried that we'd get bitten by re-assigning 'r' in the middle of the function and then end up in some odd broken state.
"git fsck" works only in a single, "the", repository, so I guess you are right to be worried about unnecessary complexity here.
I think the end-game for this whole repository transition would be to get rid of
the_repository, though. I.e., I'd envision the progression something like this:1. Teach all of the library code to take (and operate on) "struct repository".
2. Teach static local functions like this to pass in
the_repository.3. Teach top-level commands like
cmd_fsck()to passthe_repositoryto all of those static local helpers.4. Teach top-level commands to get a real repository pointer, either from the
git.cwrapper (whenRUN_SETUPis used) or by callingsetup_git_repository()themselves.
Quoted 5 lines
5. Grep for
the_repositoryand drop it everywhere.Here we're at step 2 now, but declaring "r" makes moving to step 3 just a little easier. And I think the existence of steps 4 and 5 implies that it would eventually be worth going through step 3.
Ah, the transition to step 3 justifies this, I think. I wasn't aware that steps 3+ existed. If they didn't, I'd stand by my original advice, but given that they do, the approach here makes more sense long-term.
Quoted 2 lines
Of course I just wrote those steps down for the first time, so maybe nobody else shares my vision. ;)
Thanks for writing it down. I'm sure that it has been loosely discussed over a while, but this is the first time that I've seen it all in one place.
Quoted 1 line
-Peff
Thanks,
Taylor