git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Ramsay Jones <ramsay@ramsayjones.plus.com>
Cc: Prathamesh Chavan <pc44800@gmail.com>, "git@vger.kernel.org" <git@vger.kernel.org>, Brandon Williams <bmwill@google.com>, Christian Couder <christian.couder@gmail.com>
Subject: Re: [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value
Date: Fri, 26 May 2017 10:07:30 -0700
Message-ID: <CAGZ79kYbi5QxWAsxdfPkuWEyMt9Qg753sm0vExsKaWyksDVw+Q@mail.gmail.com> (raw)
In-Reply-To: <01e8c552-fd3a-ee05-1ff1-3b3ea0f7feeb@ramsayjones.plus.com>

On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 26/05/17 16:17, Prathamesh Chavan wrote:
>> According to the documentation about git-submodule foreach subcommand's
>> $path variable:
>> $path is the name of the submodule directory relative to the superproject
>>
>> But it was observed when the value of the $path value deviates from this
>> for the nested submodules when the <command> is run from a subdirectory.
>> This patch aims for its correction.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
>> ---
>> This series of patch is based on gitster/jk/bug-to-abort for untilizing its
>> BUG() macro.
>>
>> The observation made was as follows:
>> For a project - super containing dir (not a submodule) and a submodule sub
>> which contains another submodule subsub. When we run a command from super/dir:
>>
>> git submodule foreach "echo \$path-\$sm_path"
>>
>> actual results:
>> Entering '../sub'
>> ../sub-../sub
>> Entering '../sub/subsub'
>> ../subsub-../subsub
>>
>> expected result wrt documentation and current test suite:
>> Entering '../sub'
>> sub-../sub
>> Entering '../sub/subsub'
>> subsub-../sub/subsub
>>
>> This make the value of $path confusing and I also feel it deviates from its
>> documentation:
>> $path is the name of the submodule directory relative to the superproject.
>> Hence, this patch corrects the value assigned to the $path and $sm_path.
>>
>>  git-submodule.sh | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index c0d0e9a4c..ea6f56337 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -344,9 +344,9 @@ cmd_foreach()
>>                               prefix="$prefix$sm_path/"
>>                               sanitize_submodule_env
>>                               cd "$sm_path" &&
>> -                             sm_path=$(git submodule--helper relative-path "$sm_path" "$wt_prefix") &&
>>                               # we make $path available to scripts ...
>>                               path=$sm_path &&
>> +                             sm_path=$displaypath &&
>>                               if test $# -eq 1
>>                               then
>>                                       eval "$1"
>>
>
> Hmm, I'm not sure which documentation you are referring to,

Quite likely our fine manual pages. ;)

       foreach [--recursive] <command>
           Evaluates an arbitrary shell command in each checked out submodule.
           The command has access to the variables $name, $path, $sha1 and
           $toplevel: $name is the name of the relevant submodule section in
           .gitmodules, $path is the name of the submodule directory relative
           to the superproject, $sha1 is the commit as recorded in the
           superproject, and $toplevel is the absolute path to the top-level
           of the superproject. Any submodules defined in the superproject but
           not checked out are ignored by this command. Unless given --quiet,
           foreach prints the name of each submodule before evaluating the
           command. If --recursive is given, submodules are traversed
           recursively (i.e. the given shell command is evaluated in nested
           submodules as well). A non-zero return from the command in any
           submodule causes the processing to terminate. This can be
           overridden by adding || : to the end of the command.

As $path is documented and $sm_path is not, we should care about
$path first to be correct and either fix the documentation or the implementation
such that we have a consistent world view. :)

> but if
> $path != $sm_path then something is wrong. (unless their definition
> has changed, of course).

I would lean in doing so (changing their definition):

    $path (as documented) is the name of the submodule directory
    relative to the direct superproject (so in nested submodules you
    go up only one level).

$sm_path on the other hand is not documented at all and yields
non-sense results in corner cases.

With this patch it becomes less non-sensey and could be documented as:

    $sm_path is the relative path from the current working directory
    to the submodule (ignoring relations to the superproject or nesting
    of submodules). This documentation also fits into the narrative of
    the test in t7407.

Thanks,
Stefan

  reply index

Thread overview: 48+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-04-19 17:05 [GSoC][RFC/PATCH] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-04-19 18:08 ` Stefan Beller
2017-04-22 19:58   ` [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-04-24  2:24     ` Junio C Hamano
2017-04-24 20:03     ` Stefan Beller
2017-04-24 22:11       ` Ramsay Jones
2017-04-24 22:17         ` Stefan Beller
2017-04-24 22:43           ` Ramsay Jones
2017-05-12 11:44             ` [GSoC][RFC/PATCH v3 1/2] t7407: test "submodule foreach --recursive" from subdirectory added Prathamesh Chavan
2017-05-12 11:44               ` [GSoC][RFC/PATCH v3 2/2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-05-15 17:22                 ` Stefan Beller
2017-05-15 18:34                 ` Brandon Williams
2017-05-21 12:58                   ` [GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory added Prathamesh Chavan
2017-05-21 12:58                     ` [GSoC][PATCH v4 2/2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-05-22 20:04                       ` Stefan Beller
2017-05-23 19:09                         ` Brandon Williams
2017-05-23 19:36                       ` Brandon Williams
2017-05-23 20:57                         ` Stefan Beller
2017-05-23 21:05                           ` Brandon Williams
2017-05-26 15:17                       ` Prathamesh Chavan
2017-05-26 15:17                         ` [GSoC][PATCH v5 2/3] t7407: test "submodule foreach --recursive" from subdirectory added Prathamesh Chavan
2017-05-26 16:19                           ` Stefan Beller
2017-05-26 16:33                           ` Brandon Williams
2017-05-26 15:17                         ` [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-05-26 16:14                           ` Stefan Beller
2017-05-26 16:44                           ` Brandon Williams
2017-05-26 21:54                           ` Johannes Sixt
2017-05-26 22:03                             ` Brandon Williams
2017-05-27  1:20                             ` Ramsay Jones
2017-05-27 14:06                               ` Ramsay Jones
2017-05-27 21:24                                 ` Johannes Sixt
2017-05-26 16:31                         ` Ramsay Jones
2017-05-26 17:07                           ` Stefan Beller [this message]
2017-05-27  1:10                             ` Ramsay Jones
2017-05-30 21:53                               ` Stefan Beller
2017-05-30 23:07                                 ` Ramsay Jones
2017-05-30 23:29                                   ` Stefan Beller
2017-05-31  0:13                                     ` Ramsay Jones
2017-05-31  0:48                                       ` Ramsay Jones
2017-06-02 11:24                                         ` [GSoC][PATCH v6 1/2] submodule: fix buggy $path and $sm_path variable's value Prathamesh Chavan
2017-06-02 11:24                                           ` [GSoC][PATCH v6 2/2] submodule: port subcommand foreach from shell to C Prathamesh Chavan
2017-06-03  2:13                                             ` Stefan Beller
2017-06-04 10:32                                               ` Prathamesh Chavan
2017-05-23 19:06                     ` Brandon Williams
2017-06-03  0:37                   ` [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir Stefan Beller
2017-06-03 14:07                     ` Ramsay Jones
2017-06-04 15:05                       ` Ramsay Jones
2017-06-05 22:20                     ` Jonathan Nieder

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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGZ79kYbi5QxWAsxdfPkuWEyMt9Qg753sm0vExsKaWyksDVw+Q@mail.gmail.com \
    --to=sbeller@google.com \
    --cc=bmwill@google.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pc44800@gmail.com \
    --cc=ramsay@ramsayjones.plus.com \
    /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