* Re: [PATCH v2] submodule: port subcommand 'set-url' from shell to C
2020-05-01 7:32 [PATCH v2] submodule: port subcommand 'set-url' from shell to C Shourya Shukla
@ 2020-05-01 10:01 ` Denton Liu
2020-05-01 18:54 ` Junio C Hamano
2020-05-01 18:27 ` Junio C Hamano
1 sibling, 1 reply; 4+ messages in thread
From: Denton Liu @ 2020-05-01 10:01 UTC (permalink / raw)
To: Shourya Shukla; +Cc: git, christian.couder, peff, heba.waly, gitster
Hi Shourya,
I ran into a few whitespace errors when applying this patch. You should
run `git diff --check` before committing so that you can fix these.
On Fri, May 01, 2020 at 01:02:32PM +0530, Shourya Shukla wrote:
> This aims to convert submodule subcommand 'set-url' to a builtin.
> 'set-url' is ported to 'submodule--helper.c' and the latter is called
> via 'git-submodule.sh'.
Commit messages should be written in imperative mood. This could be
rewritten as
Convert submodule subcommand 'set-url' to a builtin. Port
'set-url' to 'submodule--helper.c' and call the latter via
'git-submodule.sh'.
> 'module_set_url()' accepts the the user input, which is parsed by
s/the the/the/
> 'parse_options()', followed by the setting of the appropriate entry
> (i.e., the submodule URL here). Finally, the URL is synced, via
> 'sync_submodule()', with the upstream.
Others might disagree with me but I believe that this paragraph dives
in too deeply. It might be more helpful to give a higher level overview
than repeating what the code.
> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> ---
> I finally fixed the implementation, was making a silly mistake.
> The test './t7420-submodule-set-url.sh' passes (both tests 1 and 2
> are successful now). I have deleted the function,
> 'update_url_in_gitmodules()' because I was able to suffice that
> functionality in the 'module_set_url()' function itself.
>
> Thank you so much Christian and Denton for helping me :)
No problem :)
> builtin/submodule--helper.c | 55 +++++++++++++++++++++++++++++++++++++
> git-submodule.sh | 25 ++---------------
> 2 files changed, 58 insertions(+), 22 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..59334d4286 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,60 @@ static int module_config(int argc, const char **argv, const char *prefix)
> usage_with_options(git_submodule_helper_usage, module_config_options);
> }
>
> +struct set_url_cb {
> + const char *prefix;
> + unsigned int flags;
> +};
> +
> +#define SET_URL_CB_INIT { NULL, 0 }
> +
> +static int module_set_url(int argc, const char **argv, const char *prefix)
> +{
> + struct set_url_cb info = SET_URL_CB_INIT;
> + int quiet = 0;
> +
> + const char *newurl = NULL;
> + const char *path = NULL;
> +
> + struct strbuf entry = STRBUF_INIT;
> +
> + struct option module_set_url_options[] = {
> + OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
I know that many options in the rest of the file don't follow this
convention but the help string should begin with a lowercase.
> + OPT_END()
> + };
> +
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule--helper set-url [--quiet] [<path>] [<newurl>]"),
<path> and <newurl> should be mandatory arguments.
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, NULL, module_set_url_options,
How come you aren't passing in the prefix here?
> + git_submodule_helper_usage, 0);
> +
> + info.prefix = prefix;
> + if (quiet)
> + info.flags |= OPT_QUIET;
> +
Over here, you should check argc to ensure that we have exactly two
arguments...
> + path = argv[0];
> + newurl = argv[1];
...otherwise this may segfault.
> +
> + if(!path || !newurl){
nit: space after `if` and betwen `) {`. Also, this won't be necessary
once you check the argc.
> + usage_with_options(git_submodule_helper_usage,module_set_url_options);
nit: space after the `,`
> + return 1;
> + }
> +
> + strbuf_addstr(&entry, "submodule.");
> + strbuf_addstr(&entry, path);
> + strbuf_addstr(&entry, ".url");
> +
> + /* Setting the new URL in .gitmodules */
I don't think these comments are necessary. It's pretty obvious what's
happening in the code.
> + config_set_in_gitmodules_file_gently(entry.buf, newurl);
> + /* Syncing the updated URL */
> + sync_submodule(path, prefix, info.flags);
> +
Make sure you strbuf_release() the strbuf here.
> + return 0;
> +}
> +
> #define SUPPORT_SUPER_PREFIX (1<<0)
>
> struct cmd_struct {
> @@ -2276,6 +2330,7 @@ static struct cmd_struct commands[] = {
> {"is-active", is_active, 0},
> {"check-name", check_name, 0},
> {"config", module_config, 0},
> + {"set-url", module_set_url, 0},
> };
>
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 89f915cae9..ea72d5a5f5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -783,11 +783,13 @@ cmd_set_branch() {
> # $@ = requested path, requested url
> #
> cmd_set_url() {
> +
> while test $# -ne 0
> do
> case "$1" in
> -q|--quiet)
> GIT_QUIET=1
> + shift
> ;;
> --)
> shift
> @@ -800,30 +802,9 @@ cmd_set_url() {
> break
> ;;
> esac
> - shift
Hmmm, I don't understand why the above hunk and this line are necessary.
> done
>
> - if test $# -ne 2
> - then
> - usage
> - fi
> -
> - # we can't use `git submodule--helper name` here because internally, it
> - # hashes the path so a trailing slash could lead to an unintentional no match
> - name="$(git submodule--helper list "$1" | cut -f2)"
> - if test -z "$name"
> - then
> - exit 1
> - fi
> -
> - url="$2"
> - if test -z "$url"
> - then
> - exit 1
> - fi
> -
> - git submodule--helper config submodule."$name".url "$url"
> - git submodule--helper sync ${GIT_QUIET:+--quiet} "$name"
> + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@"
This looks good, though :)
Thanks,
Denton
> }
>
> #
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH v2] submodule: port subcommand 'set-url' from shell to C
2020-05-01 7:32 [PATCH v2] submodule: port subcommand 'set-url' from shell to C Shourya Shukla
2020-05-01 10:01 ` Denton Liu
@ 2020-05-01 18:27 ` Junio C Hamano
1 sibling, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-05-01 18:27 UTC (permalink / raw)
To: Shourya Shukla; +Cc: git, christian.couder, peff, heba.waly, liu.denton
Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 1a4b391c88..59334d4286 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2246,6 +2246,60 @@ static int module_config(int argc, const char **argv, const char *prefix)
> usage_with_options(git_submodule_helper_usage, module_config_options);
> }
>
> +struct set_url_cb {
> + const char *prefix;
> + unsigned int flags;
> +};
> +
> +#define SET_URL_CB_INIT { NULL, 0 }
Is any of the above even necessary? I cannot see the place
info.prefix is used (I can see where it is set, but the only other
appearance of string 'prefix' is a call to sync_submodule() and
info.prefix is not used there at all), and info.flags is assigned
OPT_QUIET and passed to sync_submodule(). If you must have a
variable, a single "unsigned int" local variable in the function
would be sufficient, but it is unclear if even that is needed.
Worse yet, it claims to have something to do with a callback API by
having _cb suffix, but nobody calls anything with the data here.
> +static int module_set_url(int argc, const char **argv, const char *prefix)
> +{
> + struct set_url_cb info = SET_URL_CB_INIT;
And this is the only place where SET_URL_CB_INIT is used.
I think all of the above should be discarded.
> + int quiet = 0;
This is a good initialization, as parse_options() may not even see
any option related to this variable.
> + const char *newurl = NULL;
> + const char *path = NULL;
These are unnecessary, as we will _always_ assign to them before
doing anything else. It is sufficient to say:
const char *newurl, *path;
> + struct strbuf entry = STRBUF_INIT;
> +
Why is this called entry? All other occurences of string 'entry' in
thsi file always refer to a cache_entry. Read on for a better name.
> + struct option module_set_url_options[] = {
> + OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")),
> + OPT_END()
> + };
I think you are copying what other functions do, but for a local
variable to this function, I do not see the need to call this array
with such a long name. Calling it "set_url_options[]" or even just
"options[]" is sufficient, because there is no other "struct option"
involved in this function.
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule--helper set-url [--quiet] [<path>] [<newurl>]"),
> + NULL
> + };
Likewise. "usage[]" should be plenty clear.
> + argc = parse_options(argc, argv, NULL, module_set_url_options,
> + git_submodule_helper_usage, 0);
And this would become
argc = parse_options(argc, argv, NULL, options, usage, 0);
without losing any clarity.
> + info.prefix = prefix;
> + if (quiet)
> + info.flags |= OPT_QUIET;
Drop these three lines.
> + path = argv[0];
> + newurl = argv[1];
Wait. If path is NULL, it is likely that argv[1] is an
out-of-bounds access.
> + if(!path || !newurl){
Style.
if (!path || !newurl) {
> + usage_with_options(git_submodule_helper_usage,module_set_url_options);
This would become a short-and-sweet
usage_with_options(usage, options);
Note that we require a SP after that comma.
> + return 1;
OK. This follows a (bit unnatural) convention used throughout this
file that module_<action> helpers signal a failure with positive 1,
and signal a success with 0.
> + }
> +
Taking all these together,
if (argc != 3) {
usage_with_options(usage, options);
return 1;
}
path = argv[0];
newurl = argv[1];
If you feel paranoid, you can check these two are not NULL, too,
i.e.
if (argc != 3 || !(path = argv[0]) || !(newurl = argv[1])) {
usage_with_options(usage, options);
return 1;
}
I have no strong preference either way. Perhaps the latter is more
concise and more careful at the same time, so some people may prefer
it.
> + strbuf_addstr(&entry, "submodule.");
> + strbuf_addstr(&entry, path);
> + strbuf_addstr(&entry, ".url");
The strbuf is to hold a configuration variable name and has nothing
to do with a cache entry. Perhaps
strbuf_addf(&config_name, "submodule.%s.url", path);
> + /* Setting the new URL in .gitmodules */
> + config_set_in_gitmodules_file_gently(entry.buf, newurl);
It is dubious the comment above this call (and above the next call
to sync) adds any value. The function name and its parameters tells
us enough, especially if you rename the strbuf variable and in a
single strbuf_addf(), which makes it pretty clear what the variable
contains (i.e. "submodule.<path>.url" variable in the .gitmodules
file).
> + /* Syncing the updated URL */
> + sync_submodule(path, prefix, info.flags);
This will become
sync_submodule(path, prefix, quiet ? OPT_QUIET : 0);
So, there was no need for 'flags' variable, let alone set_url_cb
structure, at all.
> + return 0;
> +}
> +
> #define SUPPORT_SUPER_PREFIX (1<<0)
>
> struct cmd_struct {
> @@ -2276,6 +2330,7 @@ static struct cmd_struct commands[] = {
> {"is-active", is_active, 0},
> {"check-name", check_name, 0},
> {"config", module_config, 0},
> + {"set-url", module_set_url, 0},
> };
>
> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 89f915cae9..ea72d5a5f5 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -783,11 +783,13 @@ cmd_set_branch() {
> # $@ = requested path, requested url
> #
> cmd_set_url() {
> +
> while test $# -ne 0
> do
> case "$1" in
> -q|--quiet)
> GIT_QUIET=1
> + shift
> ;;
> --)
> shift
> @@ -800,30 +802,9 @@ cmd_set_url() {
> break
> ;;
> esac
> - shift
> done
Why any of the above change? It may not change the behaviour from
the current implementation, but why do we even want to touch it?
> - then
> - usage
> - fi
> -
> - # we can't use `git submodule--helper name` here because internally, it
> - # hashes the path so a trailing slash could lead to an unintentional no match
> - name="$(git submodule--helper list "$1" | cut -f2)"
> - if test -z "$name"
> - then
> - exit 1
> - fi
> -
> - url="$2"
> - if test -z "$url"
> - then
> - exit 1
> - fi
> -
The loss of these lines is a good change ;-) Very pleased to see
fewer lines of shell script.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread