Re: [Outreachy] [PATCH v3] diff: do not show submodule with untracked files as "-dirty"

From: Sangeeta NB <sangunb09@gmail.com>
Date: Oct 23, 2020, 18:17 UTC
Message-ID: <CAHjREB4egt95fCz6pzON2h5rVH-XvYKf1oGnyU-8gVL-U3WrmA@mail.gmail.com>
In-Reply-To: <xmqqr1pphsvs.fsf@gitster.c.googlers.com>

Show 5 quoted lines

> The four-line paragraph I wrote above is not a question.  It is
> normal to "think aloud" to express what the reader understood what
> the patch does/intends to do during the review, which gives the
> contributor a chance to spot misunderstanding by the reviewer.
>

Oh okay. I thought you meant to ask something. But ya "think aloud" is a nice practice. Sorry for the confusion.

Show 7 quoted lines

> In any case, when "git diff --ignore-submodules=<when>" command line
> option is parsed, diff_opt_ignore_submodules() gets called, which in
> turn calls handle_ignore_submodules_arg(), and you set the bit
> there.
>
> So this is not limited to configuration, I think.  Command line
> option given by the user can also affect it.

Umm, so when diff.ignoreSubmodules is set in user config, git_diff_ui_config() is called which is called(which calls handle_ignore_submodules_arg) even before repo_git_setup() and therefore it sets the bit there. But in the case of command-line options, as you mentioned, diff_opt_ignore_submodules() gets called by the prep_parse_options() which is called after we are checking for that bit. So when we are checking for the bit, prep_parse_options() is not yet called, and therefore that bit is not set.

>
> Yeah, flipping it to on is setting it to 1.  We are saying the same
> thing, aren't we?

Ya, sorry for the misunderstanding. We are saying the same thing.

Show 20 quoted lines

>
>
> The reason why a helper function handle_ignore_submodules_arg()
> exists in the first place is because the way ignore_* bits are
> managed in the diff_options.flags is tricky and with a helper in
> between the callers and the actual bits, is easier to update in the
> future if needed.
>
> So even if we assume that everybody who reacted to end-user
> preference would have called handle_ignore_submodules_arg(), hence
> we do not want to force our default when the .ignore_submodule_set
> bit set and we do want to do so when the bit is unset, we should
> do
>
>         if (!options->flags.ignore_submodule_set)
>                 handle_ignore_submodules_arg("untracked");
>
> instead of only toggling the .ignore_untracked_in_submodules bit on
> manually.
>

Oh okay, that makes sense. Would update that in the next patch.

Show 11 quoted lines

>
> Have you looked for "dirty" (with double-quote around the word) in
> wt-status.c already?  The call to handle_ignore_submodules_arg(),
> which would cause your new bit set, is made not as a reaction to any
> end-user input.  Rather it is "if end-user did not say anything, use
> this hardcoded setting".  And it overrides the "if the end user did
> not specify anything, we want to use this default" logic you added
> with this change, because it dictates "dirty" there without end-user
> asking.
>
> Is that desirable?  I dunno.

This change was added in this[1] commit and as said there the "dirty" was used because here[2] if no arguments are provided with ignoreSubmodules then it set it to all and it might be confusing for `git status` as then even when the user chooses to record a new commit for an ignored submodule by adding it manually this change won't show up under the to-be-committed changes.(as mentioned in [1])

[1] https://github.com/git/git/commit/1d2f393ac9bfb4c20f14d6ead7bb4c56e766ab77 [2] https://github.com/git/git/blob/master/diff.c#L5139

So I think that's desirable.

> I found this change a "noise":

Oh okay, Again sorry for the misunderstanding.

Show 12 quoted lines

>
>         strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
>         if (ignore_untracked)
> -               strvec_push(&cp.args, "-uno");
> +               strvec_push (&cp.args, "-uno");
>
> If it were going the other direction, "we fix coding style violation
> while at it" may be a good justification to do so, but this
> particular change (1) is not neeeded for the purpose of this patch,
> and (2) is making the code worse by deviating from the coding
> guideline.  Please drop it.
>

This part of the change was introduced because we had a failing test here[3]. There was some problem it getting both the flags propagated through the nested submodule and that's why this change. Maybe we can do something better to fix that. It would be great if you can have a look at it.

[3] https://github.com/git/git/blob/master/t/t3600-rm.sh#L690

Thanks and Regards, Sangeeta