git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jonathan Nieder <jrnieder@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: bmwill@google.com, gitster@pobox.com, git@vger.kernel.org,
	pc44800@gmail.com, ramsay@ramsayjones.plus.com
Subject: Re: [PATCH] submodule foreach: correct $sm_path in nested submodules from a dir
Date: Mon, 5 Jun 2017 15:20:31 -0700	[thread overview]
Message-ID: <20170605222031.GB21733@aiede.mtv.corp.google.com> (raw)
In-Reply-To: <20170603003710.5558-1-sbeller@google.com>

Hi,

This patch seems to aim to do multiple things.  Initial thoughts:

Stefan Beller wrote:

[...]
> To ameliorate the situation, perform these changes
> * Document 'sm_path' instead of 'path'.
>   As using a variable '$path' may be harmful to users due to
>   capitalization issues, see 64394e3ae9 (git-submodule.sh: Don't
>   use $path variable in eval_gettext string, 2012-04-17). Adjust
>   the documentation to advocate for using $sm_path,  which contains
>   the same value. We still make the 'path' variable available,
>   though not documented.

Making sm_path part of the public API as described here sounds like a
good idea (as a separate patch), to avoid conflicting with $PATH on
Windows.  It's convenient that scripts have access to the private
variable 'sm_path'.  The 'path' variable would still need to be
documented as a deprecated synonym so people working with existing
scripts can know how to update them.

> * Clarify the 'toplevel' variable documentation.
>   It does not contain the topmost superproject as the author assumed,
>   but the direct superproject, such that $toplevel/$sm_path is the
>   actual absolute path of the submodule.

This is very confusing.  I suspect it's a bug.  Can we make 'toplevel'
point to the topmost superproject (as a separate path)?

> * The variable '$displaypath' was accessible but undocumented.
>   Rename it '$displaypath' to '$dpath'. Document what it contains.
>   Users that are broken by the behavior change of 'sm_path' introduced
>   in this commit, can switch from '$path' to '$dpath'.

What does dpath stand for?  Renaming the variable to $dpath means that
scripts trying to adapt to this change would not work with previous
versions of git.  Would it make sense to use $displaypath for this for
compatibility?

What is the intent behind the sm_path behavior change in this patch?
Stepping back, what kind of scripts is this interface meant to support
(e.g., what is an example script that used this interface that would
be affected), and is there a straightforward way to support those use
cases without breaking existing scripts except where necessary?

To summarize, the patch leaves me a bit confused.  I think it would be
best to have multiple patches that solve one problem at a time, which
would hopefully make the story clearer.

Thanks and hope that helps,
Jonathan

      parent reply	other threads:[~2017-06-05 22:20 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  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
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 [this message]

Reply instructions:

You may reply publicly 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=20170605222031.GB21733@aiede.mtv.corp.google.com \
    --to=jrnieder@gmail.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pc44800@gmail.com \
    --cc=ramsay@ramsayjones.plus.com \
    --cc=sbeller@google.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).