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

Re: [GSoC][PATCH 5/6] submodule: port submodule subcommand sync from shell to C

From
Stefan Beller <sbeller@google.com>
Date
Jun 20, 2017, 17:35 UTC
Message-ID
<CAGZ79kY=Ws_8BZyLySh0e2ZmUk70gP4RNu=fbzMqRh8n6sLg9Q@mail.gmail.com>
In-Reply-To
<20170619215025.10086-5-pc44800@gmail.com>
On Mon, Jun 19, 2017 at 2:50 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
Show 12 quoted lines
> The mechanism used for porting the submodule subcommand 'sync' is
> similar to that of 'foreach', where we split the function cmd_sync
> from shell into three functions in C, module_sync,
> for_each_submodule_list and sync_submodule.
>
> print_default_remote is introduced as a submodule--helper
> subcommand for getting the default remote as stdout.
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---

Up to this patch, all other patches look good to me, here I stumbled upon a small nit.

Show 81 quoted lines
>  builtin/submodule--helper.c | 180 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  56 +-------------
>  2 files changed, 181 insertions(+), 55 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 78b21ab22..e10cac462 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -43,6 +43,20 @@ static char *get_default_remote(void)
>         return ret;
>  }
>
> +static int print_default_remote(int argc, const char **argv, const char *prefix)
> +{
> +       const char *remote;
> +
> +       if (argc != 1)
> +               die(_("submodule--helper print-default-remote takes no arguments"));
> +
> +       remote = get_default_remote();
> +       if (remote)
> +               puts(remote);
> +
> +       return 0;
> +}
> +
>  static int starts_with_dot_slash(const char *str)
>  {
>         return str[0] == '.' && is_dir_sep(str[1]);
> @@ -311,6 +325,25 @@ static int print_name_rev(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +static char *get_up_path(const char *path)
> +{
> +       int i = count_slashes(path);
> +       struct strbuf sb = STRBUF_INIT;
> +
> +       while (i--)
> +               strbuf_addstr(&sb, "../");
> +
> +       /*
> +        *Check if 'path' ends with slash or not
> +        *for having the same output for dir/sub_dir
> +        *and dir/sub_dir/
> +        */
> +       if (!is_dir_sep(path[i - 1]))
> +               strbuf_addstr(&sb, "../");
> +
> +       return strbuf_detach(&sb, NULL);
> +}
> +
>  struct module_list {
>         const struct cache_entry **entries;
>         int alloc, nr;
> @@ -736,6 +769,151 @@ static int module_name(int argc, const char **argv, const char *prefix)
>         return 0;
>  }
>
> +struct sync_cb {
> +       const char *prefix;
> +       unsigned int quiet: 1;
> +       unsigned int recursive: 1;
> +};
> +#define SYNC_CB_INIT { NULL, 0, 0 }
> +
> +static void sync_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +       struct sync_cb *info = cb_data;
> +       const struct submodule *sub;
> +       char *sub_key, *remote_key;
> +       char *url, *sub_origin_url, *super_config_url, *displaypath;
> +       struct strbuf sb = STRBUF_INIT;
> +       struct child_process cp = CHILD_PROCESS_INIT;
> +
> +       if (!is_submodule_initialized(list_item->name))
> +               return;
> +
> +       sub = submodule_from_path(null_sha1, list_item->name);
> +
> +       if (!sub->url)

'sub' can be NULL as well, which when used to obtain the ->url will crash. So we'd rather want to have (!sub || !sub->url).

I looked through other use cases, others only need (!sub), so this thought did not hint at other bugs in the code base.

> +               die(_("no url found for submodule path '%s' in .gitmodules"),
> +                     list_item->name);
> +
> +       url = xstrdup(sub->url);

Why do we need to duplicate the url here? As we are not modifying it (read: I did not spot the url modification), we could just use sub->url instead, saving a variable.

Previous: Prathamesh ChavanNext: Christian Couder
Message 11 of 16 in “[GSoC] Update: Week 5”
  1. Prathamesh ChavanJun 19, 2017
  2. [GSoC][PATCH 1/6] dir: create function count_slashesPrathamesh Chavan, Jun 19, 2017
  3. [GSoC][PATCH 2/6] submodule--helper: introduce get_submodule_displaypath and for_each_submodule_listPrathamesh Chavan, Jun 19, 2017
  4. Brandon WilliamsJun 20, 2017
  5. Christian CouderJun 22, 2017
  6. [GSoC][PATCH 3/6] submodule: port set_name_rev from shell to CPrathamesh Chavan, Jun 19, 2017
  7. [GSoC][PATCH 6/6] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jun 19, 2017
  8. [GSoC][PATCH 4/6] submodule: port submodule subcommand statusPrathamesh Chavan, Jun 19, 2017
  9. Brandon WilliamsJun 20, 2017
  10. [GSoC][PATCH 5/6] submodule: port submodule subcommand sync from shell to CPrathamesh Chavan, Jun 19, 2017
  11. Stefan BellerJun 20, 2017
  12. Christian CouderJun 22, 2017
  13. Stefan BellerJun 20, 2017
  14. Andrew ArdillJun 20, 2017
  15. Brandon WilliamsJun 20, 2017
  16. Prathamesh ChavanJun 26, 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.