git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to C

From
Stefan Beller <sbeller@google.com>
Date
Jul 31, 2017, 21:42 UTC
Message-ID
<CAGZ79kbyyR54me_+wQDZRrikqKTp_a98yozVfr8P85QHfyyy=Q@mail.gmail.com>
In-Reply-To
<20170731205621.24305-7-pc44800@gmail.com>
On Mon, Jul 31, 2017 at 1:56 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
Show 14 quoted lines
> The same mechanism is used even for porting this submodule
> subcommand, as used in the ported subcommands till now.
> The function cmd_deinit in split up after porting into three
> functions: module_deinit(), for_each_submodule_list() and
> deinit_submodule().
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> In this new version, the following changes have been made:
> * In the function deinit_submodule, since the test is_git_directory()
>   adds an additional condition, instead is_directory() is used to check
>   if "sm_path/.git" is a directory.

Thanks for writing these patches. I wonder if (some of) these notes are best put into the code as a comment such as

    /* NEEDSWORK: convert to is_submodule_active */

such that people reading this code later realize that checking for a directory may not be the "correct" thing, but a thing which was easy to express using shell.

Show 5 quoted lines
> +struct deinit_cb {
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int force: 1;
> +       unsigned int all: 1;
The value 'all' seems to be unused, i.e. we assign it but never read it?
Show 25 quoted lines
> +};
> +#define DEINIT_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void deinit_submodule(const struct cache_entry *list_item,
> +                            void *cb_data)
> +{
> +       struct deinit_cb *info = cb_data;
> +       const struct submodule *sub;
> +       char *displaypath = NULL;
> +       struct child_process cp_config = CHILD_PROCESS_INIT;
> +       struct strbuf sb_config = STRBUF_INIT;
> +       char *sub_git_dir = xstrfmt("%s/.git", list_item->name);
> +       mode_t mode = 0777;
> +
> +       sub = submodule_from_path(null_sha1, list_item->name);
> +
> +       if (!sub || !sub->name)
> +               goto cleanup;
> +
> +       displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +       /* remove the submodule work tree (unless the user already did it) */
> +       if (is_directory(list_item->name)) {
> +               struct stat st;
> +               /* protect submodules containing a .git directory */
Here may a good place to put:
  /* NEEDSWORK: automatically call absorbgitdirs before warning/die. */
(It was not in the shell version, so feel free to ignore)
Show 5 quoted lines
> +               if (!info->force) {
> +                       struct child_process cp_rm = CHILD_PROCESS_INIT;
> +                       cp_rm.git_cmd = 1;
> +                       argv_array_pushl(&cp_rm.args, "rm", "-qn",
> +                                        list_item->name, NULL);

A bug that exists in the shell version as well as here: What if the submodule has the name '--cached', which happens to be a valid argument for git-rm?

The call to git-rm would die claiming that the <file> is missing, as the file name was miss-interpreted as another flag.

To solve this problem we would insert a '--' after the options, before the file name to state that the last argument is a <file>.

Not sure if we want to fix the bug while we're here or if we rather want to add

    /* NEEDSWORK: add '--' to confirm <file> argument */
> +       if (mkdir(list_item->name, mode))
> +               die(_("could not create empty submodule directory %s"),
> +                     displaypath);

In the shell version we used the shell mkdir, which on failure would print the error to stderr on its own. The added "|| say" is just to clarify the bigger picture.

mkdir in C doesn't print the actual error (e.g. no diskspace, permissions) so we have to do it. Use 'die_errno' instead as then the reason will be given as well.

> +       cp_config.git_cmd = 1;
> +       argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
> +       argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);

It would be nice if the commit message could give hints as why a literal translation was chosen here and not e.g. one of the builtin config calls, as that would omit spawning a process.

Previous: Prathamesh ChavanNext: Prathamesh Chavan
Message 11 of 25 in “Update: Week-11”
  1. Prathamesh ChavanJul 31, 2017
  2. [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath()Prathamesh Chavan, Jul 31, 2017
  3. [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list()Prathamesh Chavan, Jul 31, 2017
  4. [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to CPrathamesh Chavan, Jul 31, 2017
  5. [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to CPrathamesh Chavan, Jul 31, 2017
  6. Stefan BellerJul 31, 2017
  7. Prathamesh ChavanAug 1, 2017
  8. [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' from shell to CPrathamesh Chavan, Jul 31, 2017
  9. Stefan BellerJul 31, 2017
  10. [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jul 31, 2017
  11. Stefan BellerJul 31, 2017
  12. Prathamesh ChavanAug 1, 2017
  13. [GSoC][PATCH 07/13] diff: change scope of the function count_lines()Prathamesh Chavan, Jul 31, 2017
  14. [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to CPrathamesh Chavan, Jul 31, 2017
  15. Stefan BellerJul 31, 2017
  16. Christian CouderJul 31, 2017
  17. Prathamesh ChavanAug 5, 2017
  18. Christian CouderAug 5, 2017
  19. Prathamesh ChavanAug 5, 2017
  20. [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectoryPrathamesh Chavan, Jul 31, 2017
  21. [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path'Prathamesh Chavan, Jul 31, 2017
  22. [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentationPrathamesh Chavan, Jul 31, 2017
  23. [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath'Prathamesh Chavan, Jul 31, 2017
  24. [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to CPrathamesh Chavan, Jul 31, 2017
  25. Stefan BellerJul 31, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.