From: Stefan Beller <sbeller@google.com> To: Jonathan Tan <jonathantanmy@google.com> Cc: git <git@vger.kernel.org>, Brandon Williams <bmwill@google.com>, Heiko Voigt <hvoigt@hvoigt.net>, seanwbehan@riseup.net Subject: Re: [PATCH] grep: remove "repo" arg from non-supporting funcs Date: Tue, 27 Mar 2018 16:20:25 -0700 Message-ID: <CAGZ79kY-E5FZRJAg6QG0DX1TzWXgo9LqJ-b7JojpkD6_BdF-wQ@mail.gmail.com> (raw) In-Reply-To: <20180327225850.166523-1-jonathantanmy@google.com> On Tue, Mar 27, 2018 at 3:58 PM, Jonathan Tan <jonathantanmy@google.com> wrote: > As part of commit f9ee2fcdfa ("grep: recurse in-process using 'struct > repository'", 2017-08-02), many functions in builtin/grep.c were > converted to also take "struct repository *" arguments. Among them were > grep_object() and grep_objects(). > > However, at least grep_objects() was converted incompletely - it calls > gitmodules_config_oid(), which references the_repository. > > But it turns out that the conversion was extraneous anyway - there has > been no user-visible effect - because grep_objects() is never invoked > except with the_repository. > > Revert the changes to grep_objects() and grep_object() (which conversion > is also extraneous) to show that both these functions do not support > repositories other than the_repository. I'd rather convert gitmodules_config_oid instead of reverting the other functions into a world without an arbitrary repository object. > > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > Patch 1/5 of your series is obviously correct. > > I investigated the change to grep_objects() in patch 2/5, and here is a > patch summarizing my findings. Consider including this patch before 2/5 > (or before 1/5). You'll probably need to write > "submodule_free(the_repository);" instead of what you have currently, > but other than that, I don't think this affects your patch set much. Thanks for looking at the patches! I'd think this patch is orthogonal to the series, as this is about the effort of converting parts of git-grep whereas this series is fixing a bug (by converting parts of the submodule config machinery))? Thanks, Stefan
next prev parent reply index Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-27 21:39 [PATCH 0/5] Moving submodules with nested submodules Stefan Beller 2018-03-27 21:39 ` [PATCH 1/5] submodule.h: drop declaration of connect_work_tree_and_git_dir Stefan Beller 2018-03-27 21:39 ` [PATCH 2/5] submodule-config: allow submodule_free to handle arbitrary repositories Stefan Beller 2018-03-27 22:57 ` Brandon Williams 2018-03-27 22:58 ` [PATCH] grep: remove "repo" arg from non-supporting funcs Jonathan Tan 2018-03-27 23:20 ` Stefan Beller [this message] 2018-03-28 0:24 ` Jonathan Tan 2018-03-28 0:35 ` Stefan Beller 2018-03-27 21:39 ` [PATCH 3/5] submodule-config: add repository argument to submodule_from_{name, path} Stefan Beller 2018-03-27 23:04 ` Jonathan Tan 2018-03-27 23:53 ` Stefan Beller 2018-03-27 21:39 ` [PATCH 4/5] submodule-config: remove submodule_from_cache Stefan Beller 2018-03-27 23:07 ` Jonathan Tan 2018-03-27 21:39 ` [PATCH 5/5] submodule: fixup nested submodules after moving the submodule Stefan Beller 2018-03-27 23:25 ` Brandon Williams 2018-03-28 0:07 ` Jonathan Tan 2018-03-28 0:42 ` Stefan Beller 2018-03-28 17:24 ` [PATCHv2 0/6] Moving submodules with nested submodules Stefan Beller 2018-03-28 17:24 ` [PATCH 1/6] submodule.h: drop declaration of connect_work_tree_and_git_dir Stefan Beller 2018-03-28 17:24 ` [PATCH 2/6] submodule-config: allow submodule_free to handle arbitrary repositories Stefan Beller 2018-03-28 17:24 ` [PATCH 3/6] submodule-config: add repository argument to submodule_from_{name, path} Stefan Beller 2018-03-28 17:24 ` [PATCH 4/6] submodule-config: remove submodule_from_cache Stefan Beller 2018-03-28 17:24 ` [PATCH 5/6] submodule: fixup nested submodules after moving the submodule Stefan Beller 2018-03-28 17:35 ` Brandon Williams 2018-03-28 19:08 ` Stefan Beller 2018-03-28 17:46 ` Jonathan Tan 2018-03-28 17:24 ` [PATCH 6/6] grep: remove "repo" arg from non-supporting funcs Stefan Beller 2018-03-28 17:54 ` [PATCHv2 0/6] Moving submodules with nested submodules Jonathan Tan 2018-03-28 22:35 ` [PATCHv3 " Stefan Beller 2018-03-28 22:35 ` [PATCHv3 1/6] submodule.h: drop declaration of connect_work_tree_and_git_dir Stefan Beller 2018-03-28 22:35 ` [PATCHv3 2/6] grep: remove "repo" arg from non-supporting funcs Stefan Beller 2018-03-28 22:35 ` [PATCHv3 3/6] submodule-config: allow submodule_free to handle arbitrary repositories Stefan Beller 2018-03-28 22:35 ` [PATCHv3 4/6] submodule-config: add repository argument to submodule_from_{name, path} Stefan Beller 2018-03-28 22:35 ` [PATCHv3 5/6] submodule-config: remove submodule_from_cache Stefan Beller 2018-03-28 22:35 ` [PATCHv3 6/6] submodule: fixup nested submodules after moving the submodule Stefan Beller 2018-03-28 23:17 ` [PATCHv3 0/6] Moving submodules with nested submodules Jonathan Tan
Reply instructions: You may reply publically to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAGZ79kY-E5FZRJAg6QG0DX1TzWXgo9LqJ-b7JojpkD6_BdF-wQ@mail.gmail.com \ --to=sbeller@google.com \ --cc=bmwill@google.com \ --cc=git@vger.kernel.org \ --cc=hvoigt@hvoigt.net \ --cc=jonathantanmy@google.com \ --cc=seanwbehan@riseup.net \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
git@vger.kernel.org mailing list mirror (one of many) Archives are clonable: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.org/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ or Tor2web: https://www.tor2web.org/ AGPL code for this site: git clone https://public-inbox.org/ public-inbox