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: Tue, 30 May 2017 14:53:47 -0700
Message-ID: <CAGZ79kYc9wx23N9quxhuYAf2H1Rm3tGNg_0Ydz0KO4qPH-Kz5w@mail.gmail.com> (raw)
In-Reply-To: <24ebb17b-4324-c6ef-7e3a-5576cda3b595@ramsayjones.plus.com>

On Fri, May 26, 2017 at 6:10 PM, Ramsay Jones
<ramsay@ramsayjones.plus.com> wrote:
>
>
> On 26/05/17 18:07, Stefan Beller wrote:
>> On Fri, May 26, 2017 at 9:31 AM, Ramsay Jones
>> <ramsay@ramsayjones.plus.com> wrote:
>>> 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.
>
> I suspected as much, but I was wondering specifically if $sm_path
> had been documented anywhere. I didn't think so, but ...
>
>> 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. :)
>
> Sure, but what is that world view? :-D
>
> I suspect that commit 091a6eb0fe did not intend (should not have)
> used $sm_path in that test. If we were to 'fix' that test, would
> it still work?
>
> Back in 2012, the submodule list was generated by filtering the
> output of 'git ls-files --error-unmatch --stage --'; but I don't
> recall if (at that time) git-ls-files required being at the top
> of the working tree, or if it would execute fine in a sub-directory.
> So, it's possible that the documentation of $path was wrong all along.
> ;-)
>
> At that time, by definition, $path == $sm_path. However, you know this
> stuff much better than me (I don't use git-submodule), so ...

Don't take that stance. Sometimes I shoot quickly from the hip without
considering consequences (Figuratively).

In a foreach command I can see value both in the 'displaypath'
(what sm_path would become here) and the 'submodule path'
from the superproject. The naming of 'sm_path' hints at that it ought
to be the 'submodule path'.

>>
>>     $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.
>
> Hmm, at what point did '$sm_path yields non-sense results' start
> being the case? (perhaps the corner cases need to be fixed first).

Well the corner case is described in the patchs notes.
So that patch would fix it to be consistent with the new world view
(that I have in mind) as I do not know about the 2012 ideas how submodules
ought to behave correctly.

>> 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).
>
> OK.
>
>>                      This documentation also fits into the narrative of
>>     the test in t7407.
>
> Hmm, does it?

After rereading that test, I would think so?

Thanks for keeping discussing this.

So maybe we want to
* keep path=sm_path
* fix the documentation via s/$path/$sm_path/g in that section quoted above.
* Introduce a new variable sm_display_path that contains what this patch
  proposes sm_path to be.
* fix the test in t7407 by checking both sm_path (fixed) as well
  as sm_display_path (what is currently recorded in sm_path)
---
In the next patch:
* Additionally in the rewrite in C, we would do an

    #ifndef WINDOWS /* need to lookup the exact macro */
        argv_array_push(env_vars, "path=%s", sm_path);
    #endif

such that Windows users are forced to migrate to sm_path
as path/Path is case sensitive there. sm_path being documented
value, so it should work fine?

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] " 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                       ` [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value 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                         ` [GSoC][PATCH v5 1/3] submodule: fix buggy $path and $sm_path variable's value Ramsay Jones
2017-05-26 17:07                           ` Stefan Beller
2017-05-27  1:10                             ` Ramsay Jones
2017-05-30 21:53                               ` Stefan Beller [this message]
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] " 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                     ` [GSoC][PATCH v4 1/2] t7407: test "submodule foreach --recursive" from subdirectory added 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=CAGZ79kYc9wx23N9quxhuYAf2H1Rm3tGNg_0Ydz0KO4qPH-Kz5w@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