Re: [PATCH v2] clean: improve -n and -f implementation and documentation

From: Sergey Organov <sorganov@gmail.com>
Date: Mar 4, 2024, 20:19 UTC
Message-ID: <87sf157nsv.fsf@osv.gnss.ru>
In-Reply-To: <xmqqo7btom4u.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 24 quoted lines

> Sergey Organov <sorganov@gmail.com> writes:
>
>>> The reason for the behaviour can be explained this way:
>>>
>>>  * "git clean" (with neither -i nor -n.  The user wants the default
>>>    mode that has no built-in protection will be stopped without -f.
>>>
>>>  * "git clean -n".  The user wants the dry-run mode that has its own
>>>    protection, i.e. being always no-op to the files, so there is no
>>>    need to fail here for the lack of "-f".
>>>
>>>  * "git clean --interactive".  The user wants the interactive mode
>>>    that has its own protection, i.e. giving the end-user a chance to
>>>    say "oh, I didn't mean to remove these files, 'q'uit from this
>>>    mistake", so there is no need to fail here for the lack of "-f".
>>
>> Well, if we remove -i from error message as well, then yes, this makes
>> sense.
>> ...
>> I then suggest to consider to remove mention of -i from
>> clean.requireForce description as well.
>
> The follow-up patch you just reviewed in the other thread does
> exactly that.

Yeah, the follow-up patch somehow didn't thread correctly with original discussion, so I've noticed it only after I wrote the above, and the patch is fine indeed.

Show 12 quoted lines

>
> This is a tangent, but before finalizing the version that complains
> "clean.requireForce is in effect and you did not give me -f" without
> mentioning "-i" or "-n", I asked gemini.google.com to proofread the
> patch and and one of its suggestion was to use this:
>
>     "clean.requireForce is true.  Use -f to override, or consider
>     using -n (dry-run) or -i (interactive) for a safer workflow."
>
> as a possibly cleaner message.  It is the opposite of what both of
> us concluded to be good in this exchange, but in some sense, it does
> sound more helpful to end users, which I somehow found amusing.

The added advice looks fine to me, as it explicitly separates -f from the other ways of using "git clean". However, starting phrase with "clean.requireForce is true" sounds strange. I'd rather say:

   "Refusing to remove files: use -f to force removal. Alternatively,
   consider using -n (dry-run) or -i (interactive) for a safer workflow.
   Set clean.requireForce to false to get rid of this message"

Here we first state what has happened, and then mention solutions in most-probable-first order.

However, if I were gemini, I'd probably start from noticing that no error message is required at all unless there is something to delete in the first place. I.e., the error should probably occur not here, but rather at every attempt to delete, and then explanation should be given later, e.g.:

  Refusing to remove FILE1
  Refusing to remove FILE2

  No files were removed: use -f to force removal. Alternatively,
  consider using -n (dry-run) or -i (interactive) for a safer workflow.

  Set clean.requireForce to false to disable this protection.

With this, user effectively gets functionality similar to "git clean -n" by default.

Just saying.

Thanks, -- Sergey Organov