From: Sergey Organov <sorganov@gmail.com>
To: "Jean-Noël AVILA" <avila.jn@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [PATCH] clean: improve -n and -f implementation and documentation
Date: Sat, 02 Mar 2024 23:09:03 +0300 [thread overview]
Message-ID: <87r0gs8kgw.fsf@osv.gnss.ru> (raw)
In-Reply-To: <6033073.lOV4Wx5bFT@cayenne> ("Jean-Noël AVILA"'s message of "Sat, 02 Mar 2024 20:47:55 +0100")
Jean-Noël AVILA <avila.jn@gmail.com> writes:
> On Friday, 1 March 2024 15:34:52 CET Sergey Organov wrote:
>> Jean-Noël Avila <avila.jn@gmail.com> writes:
>>
>> > Putting my documentation/translator hat:
>> >
>> > Le 29/02/2024 à 20:07, Sergey Organov a écrit :
>> >> What -n actually does in addition to its documented behavior is
>> >> ignoring of configuration variable clean.requireForce, that makes
>> >> sense provided -n prevents files removal anyway.
>> >>
>> >> So, first, document this in the manual, and then modify implementation
>> >> to make this more explicit in the code.
>> >>
>> >> Improved implementation also stops to share single internal variable
>> >> 'force' between command-line -f option and configuration variable
>> >> clean.requireForce, resulting in more clear logic.
>> >>
>> >> The error messages now do not mention -n as well, as it seems
>> >> unnecessary and does not reflect clarified implementation.
>> >>
>> >> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> >> ---
>> >> Documentation/git-clean.txt | 2 ++
>> >> builtin/clean.c | 26 +++++++++++++-------------
>> >> 2 files changed, 15 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/Documentation/git-clean.txt b/Documentation/git-clean.txt
>> >> index 69331e3f05a1..662eebb85207 100644
>> >> --- a/Documentation/git-clean.txt
>> >> +++ b/Documentation/git-clean.txt
>> >> @@ -49,6 +49,8 @@ OPTIONS
>> >> -n::
>> >> --dry-run::
>> >> Don't actually remove anything, just show what would be done.
>> >> + Configuration variable clean.requireForce is ignored, as
>> >> + nothing will be deleted anyway.
>> >
>> > Please use backticks for options, configuration and environment names:
>> > `clean.requireForce`
>>
>> I did consider this. However, existing text already has exactly this one
>> unquoted, so I just did the same. Hopefully it will be fixed altogether
>> later, or are you positive I better resend the patch with quotes?
>>
>> >>
>> >> -q::
>> >> --quiet::
>> >> diff --git a/builtin/clean.c b/builtin/clean.c
>> >> index d90766cad3a0..fcc50d08ee9b 100644
>> >> --- a/builtin/clean.c
>> >> +++ b/builtin/clean.c
>> >> @@ -25,7 +25,7 @@
>> >> #include "help.h"
>> >> #include "prompt.h"
>> >>
>> >> -static int force = -1; /* unset */
>> >> +static int require_force = -1; /* unset */
>> >> static int interactive;
>> >> static struct string_list del_list = STRING_LIST_INIT_DUP;
>> >> static unsigned int colopts;
>> >> @@ -128,7 +128,7 @@ static int git_clean_config(const char *var, const
> char *value,
>> >> }
>> >>
>> >> if (!strcmp(var, "clean.requireforce")) {
>> >> - force = !git_config_bool(var, value);
>> >> + require_force = git_config_bool(var, value);
>> >> return 0;
>> >> }
>> >>
>> >> @@ -920,7 +920,7 @@ int cmd_clean(int argc, const char **argv, const char
> *prefix)
>> >> {
>> >> int i, res;
>> >> int dry_run = 0, remove_directories = 0, quiet = 0, ignored = 0;
>> >> - int ignored_only = 0, config_set = 0, errors = 0, gone = 1;
>> >> + int ignored_only = 0, force = 0, errors = 0, gone = 1;
>> >> int rm_flags = REMOVE_DIR_KEEP_NESTED_GIT;
>> >> struct strbuf abs_path = STRBUF_INIT;
>> >> struct dir_struct dir = DIR_INIT;
>> >> @@ -946,21 +946,21 @@ int cmd_clean(int argc, const char **argv, const
> char *prefix)
>> >> };
>> >>
>> >> git_config(git_clean_config, NULL);
>> >> - if (force < 0)
>> >> - force = 0;
>> >> - else
>> >> - config_set = 1;
>> >>
>> >> argc = parse_options(argc, argv, prefix, options,
> builtin_clean_usage,
>> >> 0);
>> >>
>> >> - if (!interactive && !dry_run && !force) {
>> >> - if (config_set)
>> >> - die(_("clean.requireForce set to true and
> neither -i, -n, nor -f given; "
>> >> + /* Dry run won't remove anything, so requiring force makes no
> sense */
>> >> + if(dry_run)
>> >> + require_force = 0;
>> >> +
>> >> + if (!force && !interactive) {
>> >> + if (require_force > 0)
>> >> + die(_("clean.requireForce set to true and
> neither -f, nor -i given; "
>> >> + "refusing to clean"));
>> >> + else if (require_force < 0)
>> >> + die(_("clean.requireForce defaults to true
> and neither -f, nor -i given; "
>> >> "refusing to clean"));
>> >> - else
>> >> - die(_("clean.requireForce defaults to true
> and neither -i, -n, nor -f given;"
>> >> - " refusing to clean"));
>> >> }
>> >>
>> >
>> > The last two cases can be coalesced into a single case (the last one),
>> > because the difference in the messages does not bring more information
>> > to the user.
>>
>> Did you misread the patch? There are only 2 cases here, the last (third)
>> one is marked with '-' (removed). Too easy to misread this, I'd say. New
>> code is:
>>
>> if (require_force > 0)
>> die(_("clean.requireForce set to true and
> neither -f, nor -i given; "
>> "refusing to clean"));
>> else if (require_force < 0)
>> die(_("clean.requireForce defaults to true
> and neither -f, nor -i given; "
>>
>> and is basically unchanged from the original, except reference to '-n' has
> been
>> removed. Btw, is now comma needed after -f, and isn't it better to
>> substitute ':' for ';'?
>>
>> Thank you for review!
>>
>> -- Sergey Organov
>>
>>
>
> Oh, sorry, I misinterpreted the patch. But yet, I'm not sure that
> specifying that this is the default or not is really useful. If the
> configuration was set to true, it is was a no-op. If set to false, no
> message will appear.
I'm not sure either, and as it's not the topic of this particular patch,
I'd like to delegate the decision on the issue.
next prev parent reply other threads:[~2024-03-02 20:09 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-09 20:20 what should "git clean -n -f [-d] [-x] <pattern>" do? Junio C Hamano
2024-01-09 22:04 ` Sergey Organov
2024-01-19 2:07 ` Elijah Newren
2024-01-23 15:10 ` Sergey Organov
2024-01-23 18:34 ` Junio C Hamano
2024-01-24 8:23 ` Sergey Organov
2024-01-24 17:21 ` Junio C Hamano
2024-01-25 17:11 ` Sergey Organov
2024-01-25 17:46 ` Junio C Hamano
2024-01-25 20:27 ` Sergey Organov
2024-01-25 20:31 ` Sergey Organov
2024-01-26 7:44 ` Junio C Hamano
2024-01-26 12:09 ` Sergey Organov
2024-01-27 10:00 ` Junio C Hamano
2024-01-27 13:25 ` Sergey Organov
2024-01-29 19:40 ` Kristoffer Haugsbakk
2024-01-31 13:04 ` Sergey Organov
2024-01-29 9:35 ` Sergey Organov
2024-01-29 18:20 ` Jeff King
2024-01-29 21:49 ` Sergey Organov
2024-01-30 5:44 ` Jeff King
2024-01-30 5:53 ` Junio C Hamano
2024-02-29 19:07 ` [PATCH] clean: improve -n and -f implementation and documentation Sergey Organov
2024-03-01 13:20 ` Jean-Noël Avila
2024-03-01 14:34 ` Sergey Organov
2024-03-01 15:29 ` Kristoffer Haugsbakk
2024-03-01 18:07 ` Junio C Hamano
2024-03-02 19:47 ` Jean-Noël AVILA
2024-03-02 20:09 ` Sergey Organov [this message]
2024-03-02 21:07 ` Junio C Hamano
2024-03-02 23:48 ` Sergey Organov
2024-03-03 9:54 ` Sergey Organov
2024-03-01 18:07 ` Junio C Hamano
2024-03-01 18:30 ` Junio C Hamano
2024-03-01 19:31 ` Sergey Organov
2024-03-02 16:31 ` Junio C Hamano
2024-03-02 19:59 ` Sergey Organov
2024-03-03 9:50 ` [PATCH v2] " Sergey Organov
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=87r0gs8kgw.fsf@osv.gnss.ru \
--to=sorganov@gmail.com \
--cc=avila.jn@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).