| Message ID | 20200513201737.55778-2-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] submodule: port subcommand 'set-branch' from shell to C | expand |
Hi Shourya, I'm not really sure if we should have this patch at all since I don't think that set-branch should be printing anything at all. But I'll give some comments anyway. Hopefully they'll be enlightening. On Thu, May 14, 2020 at 01:47:37AM +0530, Shourya Shukla wrote: > The subcommand 'set-branch' had the 'quiet' option which was > introduced in b57e8119e6 by Denton Liu but was never utilised due to We typically refer to commits by the "reference" format. You can get that as follows: $ git show --pretty=ref -s b57e8119e6 b57e8119e6 (submodule: teach set-branch subcommand, 2019-02-08) In addition, I don't think it's necessary to mention me by name in this case. > not setting of the 'GIT_QUIET' variable. Add functionality to > utilise the 'quiet' function. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > There was an existence of the `quiet` option in the shell version of > 'set-branch' but it was not used anywhere. I decided to add a utility > of the option here by setting GIT_QUIET to 1 in case of a `quiet` as > well as ensure proper functioning in the C version regarding the same. > The if-statement is inspired from what Junio suggested me in my previous > conversion of 'set-url'. > > builtin/submodule--helper.c | 6 ++++-- > git-submodule.sh | 2 +- > 2 files changed, 5 insertions(+), 3 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 5a8815b76e..36b69df5c4 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2321,7 +2321,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) > config_name = xstrfmt("submodule.%s.branch", path); > config_set_in_gitmodules_file_gently(config_name, newbranch); > > - printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); > + if (!(quiet ? OPT_QUIET : 0)) This is needlessly complicated... Can't this just be written as if (!quiet) > + printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); > free(config_name); > } > > @@ -2334,7 +2335,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) > config_name = xstrfmt("submodule.%s.branch", path); > config_set_in_gitmodules_file_gently(config_name, NULL); > > - printf(_("Default tracking branch set to 'master' successfully\n")); > + if (!(quiet ? OPT_QUIET : 0)) > + printf(_("Default tracking branch set to 'master' successfully\n")); > free(config_name); > } > > diff --git a/git-submodule.sh b/git-submodule.sh > index 2438ef576e..0cdc77ace6 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -725,7 +725,7 @@ cmd_set_branch() { > do > case "$1" in > -q|--quiet) > - # we don't do anything with this but we need to accept it > + GIT_QUIET=1 > ;; > -d|--default) > default=1 > -- > 2.26.2 >
On 14/05 06:15, Denton Liu wrote: > Hi Shourya, > > I'm not really sure if we should have this patch at all since I don't > think that set-branch should be printing anything at all. I thought that the Documentation has the mention of the `quiet` and it wouldn't harm printing something when the branch is set. Is this not the right way to tackle this? > But I'll give some comments anyway. Hopefully they'll be enlightening. > > On Thu, May 14, 2020 at 01:47:37AM +0530, Shourya Shukla wrote: > > The subcommand 'set-branch' had the 'quiet' option which was > > introduced in b57e8119e6 by Denton Liu but was never utilised due to > > We typically refer to commits by the "reference" format. You can get > that as follows: > > $ git show --pretty=ref -s b57e8119e6 > b57e8119e6 (submodule: teach set-branch subcommand, 2019-02-08) > > In addition, I don't think it's necessary to mention me by name in this > case. Okay, I did not know that, will change in the next version. > > - printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); > > + if (!(quiet ? OPT_QUIET : 0)) > > This is needlessly complicated... Can't this just be written as > > if (!quiet) Okay, will do.
On Sat, May 16, 2020 at 11:20:16AM +0530, Shourya Shukla wrote: > On 14/05 06:15, Denton Liu wrote: > > Hi Shourya, > > > > I'm not really sure if we should have this patch at all since I don't > > think that set-branch should be printing anything at all. > > I thought that the Documentation has the mention of the `quiet` and it > wouldn't harm printing something when the branch is set. Is this not the > right way to tackle this? I would argue that it's unnecessary to print anything. If a user provides an invalid option, then an error message will be printed. If no error is provided, then a user would assume that the command succeeded. Take, for example, `git submodule set-url` which behaves similarly. It's silent on success. Printing on success would just be noise.
On 16/05 04:56, Denton Liu wrote: > I would argue that it's unnecessary to print anything. If a user > provides an invalid option, then an error message will be printed. If no > error is provided, then a user would assume that the command succeeded. > Take, for example, `git submodule set-url` which behaves similarly. It's > silent on success. Printing on success would just be noise. Okay, I have dropped the commit. When exactly is printing on success not deemed as noise?
On Sat, May 16, 2020 at 04:10:15PM +0530, Shourya Shukla wrote: > On 16/05 04:56, Denton Liu wrote: > > I would argue that it's unnecessary to print anything. If a user > > provides an invalid option, then an error message will be printed. If no > > error is provided, then a user would assume that the command succeeded. > > Take, for example, `git submodule set-url` which behaves similarly. It's > > silent on success. Printing on success would just be noise. > > Okay, I have dropped the commit. When exactly is printing on success not > deemed as noise? There's not really a hard rule on this. As a rule of thumb, though, if it's between a success and a failure, the failure should print and the success should be silent. If the user is requesting some information, then it should always print the information. There are probably some commands that are exceptions to these rules, though.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 5a8815b76e..36b69df5c4 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2321,7 +2321,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) config_name = xstrfmt("submodule.%s.branch", path); config_set_in_gitmodules_file_gently(config_name, newbranch); - printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); + if (!(quiet ? OPT_QUIET : 0)) + printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); free(config_name); } @@ -2334,7 +2335,8 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) config_name = xstrfmt("submodule.%s.branch", path); config_set_in_gitmodules_file_gently(config_name, NULL); - printf(_("Default tracking branch set to 'master' successfully\n")); + if (!(quiet ? OPT_QUIET : 0)) + printf(_("Default tracking branch set to 'master' successfully\n")); free(config_name); } diff --git a/git-submodule.sh b/git-submodule.sh index 2438ef576e..0cdc77ace6 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -725,7 +725,7 @@ cmd_set_branch() { do case "$1" in -q|--quiet) - # we don't do anything with this but we need to accept it + GIT_QUIET=1 ;; -d|--default) default=1
The subcommand 'set-branch' had the 'quiet' option which was introduced in b57e8119e6 by Denton Liu but was never utilised due to not setting of the 'GIT_QUIET' variable. Add functionality to utilise the 'quiet' function. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- There was an existence of the `quiet` option in the shell version of 'set-branch' but it was not used anywhere. I decided to add a utility of the option here by setting GIT_QUIET to 1 in case of a `quiet` as well as ensure proper functioning in the C version regarding the same. The if-statement is inspired from what Junio suggested me in my previous conversion of 'set-url'. builtin/submodule--helper.c | 6 ++++-- git-submodule.sh | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-)