Re: [PATCH v4] untracked-cache: support '--untracked-files=all' if configured

From: Tao Klerks <tao@klerks.biz>
Date: Mar 2, 2022, 08:47 UTC
Message-ID: <CAPMMpohqPa3bqnPLKJ851JJFQhbF_ZA+hAGuE_+0z+BMF2yz0Q@mail.gmail.com>
In-Reply-To: <220228.86y21v3wyj.gmgdl@evledraar.gmail.com>

On Mon, Feb 28, 2022 at 3:08 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 15 quoted lines

>
> In reviewing this I found the addition of very long lines & indentation
> made this a bit harder to read. I came up with the below which should be
> squashable on top, and perhaps makes this easier to read.
>
> I.e. the one caller that needs custom flags passes them, others pass -1
> now.
>
> For throwaway "char *" variables we usually use "char *p", "char *val"
> or whatever. A "status_untracked_files_config_value" is a bit much for
> something function local whose body is <10 lines of code.
>
> And if you drop the "int config_outcome" + rename the variable the
> repo_config_get_string() fits on one line:
>

Thank you, this looks much cleaner!

One question I have about this patch itself is why you changed the arg to new_untracked_cache_flags / configured_default_dir_flags from "repo" back to "istate" / whether that was intentional; I had understood Junio's recommendation that the function get the minimum it needed (the repo in this case) as very sensible... especially now that we need to pass in that state less often.

Another question is whether I should add a comment in new_untracked_cache_flags explaining the new meaning of "-1" for the flags argument, or whether this is sufficiently standard in this project or in C generally that I should leave it implied.

A third question is whether you have an opinion on the better approach to getting these changes merged: does it make more sense to argue that this is an improvement overall, and that the case of configured "all" and argument-provided "normal", which will now have worse performance, can and should be ignored, *or* will it be easier to get this reviewed/merged if the new behavior is made conditional upon a new configuration key?

Last question is about protocol/workflow in this project: When I squash your proposal in, should I add an extra "Signed-Off-By: Ævar Arnfjörð Bjarmason <avarab@gmail.com>" line, or just include the changes with mine?? (I've read SubmittingPatches but don't really understand the whole signing off business; I do understand that as your changes improve mine directly, they should indeed be "squashed" in, rather than added as an extra commit)