git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
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 17:35:04 -0700
Message-ID: <CAGZ79kYb_a3oXCPAyL=140pyQb=CmzSWx2wVVOcyXy2VPSLNZQ@mail.gmail.com> (raw)
In-Reply-To: <20180327172451.34f60c483133df629b1e8ec6@google.com>

On Tue, Mar 27, 2018 at 5:24 PM, Jonathan Tan <jonathantanmy@google.com> wrote:
> On Tue, 27 Mar 2018 16:20:25 -0700
> Stefan Beller <sbeller@google.com> wrote:
>
>> 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.
>
> I don't really think of it as a reversion, since grep_objects() didn't
> fully support repos other than the_repository anyway.
>
> Besides that, that's fine, but then:
>
>  (1) You would have to explain both in the gitmodules_config_oid() and
>      in this patch why "gitmodules_config_oid(...)" ->
>      "gitmodules_config_oid(repo, ...)" and "submodule_free()" ->
>      "submodule_free(repo)" are safe, since they have different behavior
>      upon first glance. (They have the same behavior only because
>      grep_objects() is always called with the_repository.) I was trying
>      to explain this in as clear a way as possible, by showing (with
>      code) that grep_objects() only works with, and is only called with,
>      the_repository.
>  (2) The code path where repo != the_repository would still never be
>      exercised, and I prefer to not have such code paths.
>
> I don't feel too strongly about (1), since they just concern commit
> messages, of which there are many valid opinions on how to write them. I
> feel a bit more strongly about (2), but can concede my point if the
> project is OK with a code path not being exercised.

Ok. I admit not having looked at the code beforehand, and I was
just arguing based on intuition. Turns out I was wrong.

I thought that we'd have to have arbitrary repos for proper submodule
recursion, but this specific code is for grepping thru given tree objects,
which for now is assumed that the user can only give trees of
the_repository and we'd error out in case of a submodule tree.
Makes sense.

In that case, I agree with your patch. Thanks for writing it.
I'll take it into the series.

Thanks,
Stefan

  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
2018-03-28  0:24       ` Jonathan Tan
2018-03-28  0:35         ` Stefan Beller [this message]
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='CAGZ79kYb_a3oXCPAyL=140pyQb=CmzSWx2wVVOcyXy2VPSLNZQ@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