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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 3, 2020, 17:55 UTC
Message-ID: <xmqqblges4ue.fsf@gitster.c.googlers.com>
In-Reply-To: <CAHjREB7W2P9_P4LoMHVVzV_YR5-_51zdbRZ0fpEDVkgkd7sh8w@mail.gmail.com>

Sangeeta <sangunb09@gmail.com> writes:

Show 18 quoted lines

> Hey Johannes and Junio,
>
> In continuation of the discussion from here[1]
>
> [1] https://lore.kernel.org/git/nycvar.QRO.7.76.6.2011020251520.18437@tvgsbejvaqbjf.bet/
>
>> >> * sj/untracked-files-in-submodule-directory-is-not-dirty (2020-10-26) 1 commit
>> >>  - diff: do not show submodule with untracked files as "-dirty"
>> >>
>> >>  "git diff" showed a submodule working tree with untracked cruft as
>> >>  "Submodule commit <objectname>-dirty", but a natural expectation is
>> >>  that the "-dirty" indicator would align with "git describe --dirty",
>> >>  which does not consider having untracked files in the working tree
>> >>  as source of dirtiness.  The inconsistency has been fixed.
>> >>
>> >>  Needs doc update.
>
> Do I need to add the doc update in the same patch?

It is ideal if code change, tests and documentation are done atomically. For a large series the story may be different, but I thought a single commit would be sufficient for this topic?

Show 8 quoted lines

>> > I *think* the original rationale for marking submodules with untracked
>> > (_un-ignored_) files was to avoid deleting a submodule that has
>> > uncommitted (because untracked) files.
>>
>> I agree with you that that the motivation was exactly that, but I
>> have a suspicion that its execution was misguided.
>
> I am really very sorry if I ended up adding some unwanted code.

But you did not. The "motivation" and "execution" are both from long time ago, back when submodule support was invented and "git diff" in the superproject was taught to pay attention to submodules.

Dscho suspects that "git diff" were taught to add "-dirty" to submodules with untracked files in their working tree in order to protect them from those who are about to delete them as a way to check if it is safe. I agreed that the motivation may have been so, i.e. they wanted to protect users from losing untracked files that they forgot to add and commit in the submodule, but the execution was wrong, i.e. "git diff" is not the right tool to achieve that protection.

See that you didn't add any unwanted code---it all happened long time ago ;-)

> So we want that `git status` should behave in the same way as it was
> behaving before? Can we do this by passing --ignore-submodules=none as
> the default args for status?

Yes, I do not recall offhand where in wt-status.c "git status" does so, but there should be an internal invocation of diff-files somewhere, and I suspect it is not passing any --ignore-submodules bit. Just like wt_status_collect_changes_index() calls handle_ignore_submodules_arg() with hardcoded "dirty", we may have to unignore untracked files in submodule working trees when checking the working tree changes.

> Another approach might be to figure out
> how the diff is being called(like is this being called from git status
> or git diff) and then add the --ignore-submodules=untracked behavior
> to it accordingly. Though I have no idea how to do that now.

No, callee shouldn't have knowledge of its caller to perform a special case action. The caller should say how it wants its callees to behave.

Thanks.