git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: christian.couder@gmail.com, congdanhqx@gmail.com,
	git@vger.kernel.org, gitster@pobox.com, liu.denton@gmail.com,
	sunshine@sunshineco.com, Johannes.Schindelin@gmx.de,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v4] submodule: port subcommand 'set-branch' from shell to C
Date: Sun, 24 May 2020 00:19:38 +0530
Message-ID: <33127873-fb19-2bd5-3028-bcd1757e92e5@gmail.com> (raw)
In-Reply-To: <20200523163929.7040-1-shouryashukla.oo@gmail.com>

Hi Shourya,

I believe you missed Danh's v3 comments[1]. I'm mentioning them inline 
with some additional comments.

On 23-05-2020 22:09, Shourya Shukla wrote:
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index f50745a03f..7e844e8971 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2284,6 +2284,50 @@ static int module_set_url(int argc, const char **argv, const char *prefix)
>   	return 0;
>   }
>   
> +static int module_set_branch(int argc, const char **argv, const char *prefix)
> +{
> +	/*
> +	 * We accept the `quiet` option for uniformity across subcommands,
> +	 * though there is nothing to make less verbose in this subcommand.
> +	 */
> +	int quiet = 0, opt_default = 0, ret;
> +	const char *opt_branch = NULL;
> +	const char *path;
> +	char *config_name;
> +
> +	struct option options[] = {
> +		OPT__QUIET(&quiet,
> +			N_("suppress output for setting default tracking branch")),

As '--quiet' in 'set-branch' is a no-op and is being accepted only for 
uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of 
OPT__QUIET for specifying it, as suggested by Danh.

Also, the description "suppress output for setting default tracking 
branch" doesn't seem to be valid anymore as we don't print anything when 
set-branch succeeds.

> +		OPT_BOOL(0, "default", &opt_default,
> +			N_("set the default tracking branch to master")),
> +		OPT_STRING(0, "branch", &opt_branch, N_("branch"),
> +			N_("set the default tracking branch")),
> +		OPT_END()
> +	};
> +	const char *const usage[] = {
> +		N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"),
> +		N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"),
> +		NULL
> +	};
> +

I also agree with the Danh here that '--quiet' could be removed from 
usage. There's no point in mentioning '--quiet' in the usage when it has 
no effect.

> diff --git a/git-submodule.sh b/git-submodule.sh
> index 39ebdf25b5..8c56191f77 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -750,33 +750,7 @@ cmd_set_branch() {
>   		shift
>   	done
>   
> -	if test $# -ne 1
> -	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
> -
> -	test -n "$branch"; has_branch=$?
> -	test "$unset_branch" = true; has_unset_branch=$?
> -
> -	if test $((!$has_branch != !$has_unset_branch)) -eq 0
> -	then
> -		usage
> -	fi
> -
> -	if test $has_branch -eq 0
> -	then
> -		git submodule--helper config submodule."$name".branch "$branch"
> -	else
> -		git submodule--helper config --unset submodule."$name".branch
> -	fi
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@"
>   }
>

Danh questioned whether '$branch' needs to be quoted here. I too think 
it needs to be quoted unless I'm missing something.


---
Footnotes:
[1]: 
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e

Thanks,
Sivaraam

  reply index

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-21 16:38 [PATCH v3] " Shourya Shukla
2020-05-21 18:44 ` Junio C Hamano
2020-05-21 19:03   ` Denton Liu
2020-05-21 19:50     ` Junio C Hamano
2020-05-22 19:39       ` Shourya Shukla
2020-05-24 16:07         ` Junio C Hamano
2020-05-21 23:04 ` Đoàn Trần Công Danh
2020-05-22 22:21 ` Johannes Schindelin
2020-05-24 23:15   ` Junio C Hamano
2020-05-24 23:18     ` Junio C Hamano
2020-05-23 16:39 ` [PATCH v4] " Shourya Shukla
2020-05-23 18:49   ` Kaartic Sivaraam [this message]
2020-05-23 23:18     ` Đoàn Trần Công Danh
2020-05-27 17:13     ` Shourya Shukla
2020-05-28 12:21       ` Đoàn Trần Công Danh
2020-05-28 14:01         ` Đoàn Trần Công Danh
2020-05-28 15:55           ` Đoàn Trần Công Danh
2020-06-02 16:35   ` [GSoC][PATCH v5] " Shourya Shukla
2020-06-02 17:58     ` Junio C Hamano
2020-06-03  0:12       ` Đoàn Trần Công Danh
2020-06-03 20:02         ` Junio C Hamano
2020-06-04  7:17           ` Shourya Shukla
2020-06-04  7:49             ` Christian Couder
2020-06-04 15:03             ` Junio C Hamano
2020-06-02 19:01     ` Kaartic Sivaraam
2020-06-02 19:10       ` Kaartic Sivaraam
2020-06-02 19:45       ` Christian Couder
2020-06-04  7:09         ` Shourya Shukla
2020-06-04 19:26         ` Kaartic Sivaraam

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=33127873-fb19-2bd5-3028-bcd1757e92e5@gmail.com \
    --to=kaartic.sivaraam@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=liu.denton@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --cc=sunshine@sunshineco.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 list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror http://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

Example config snippet for mirrors

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.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git