Re: [Outreachy] Introduction

From: Sangeeta NB <sangunb09@gmail.com>
Date: Oct 14, 2020, 15:52 UTC
Message-ID: <CAHjREB4Qw78xS-T=LA80yVf_u3=Hbpxg5tqBnP1QK44FRkBFOQ@mail.gmail.com>
In-Reply-To: <xmqq362jsahk.fsf@gitster.c.googlers.com>

Hey everyone,

I created a PR at gitgitgadget here[1] but it is failing at three tests of git rm[2]. I looked at the behavior of git status at some other places( by pausing 'git diff HEAD with dirty submodule(untracked)' in t/t4027-diff-submodule.sh and looking at `git status` behavior) but it was working perfectly fine(was giving what output was expected). But here[2] I couldn't understand why is it failing. Can someone please have a look at the PR and give me some pointers? I know I am asking way out of too much but I tried a lot on what could have been missing but couldn't find anything.

[1] https://github.com/gitgitgadget/git/pull/751 [2] https://github.com/git/git/blob/master/t/t3600-rm.sh#L691

Regards, Sangeeta

On Mon, Oct 12, 2020 at 9:27 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 60 quoted lines

>
> Sangeeta NB <sangunb09@gmail.com> writes:
>
> > A fix for making this as the default behaviour can be:
> >
> > --- a/diff.c
> > +++ b/diff.c
> > @@ -422,6 +422,7 @@ int git_diff_ui_config(const char *var, const char
> > *value, void *cb)
> >         if (git_color_config(var, value, cb) < 0)
> >                 return -1;
> >
> > +       handle_ignore_submodules_arg(&default_diff_options, "untracked");
> >         return git_diff_basic_config(var, value, cb);
>
> This function is called for each and every element of configuration
> item in your ~/.gitconfig and .git/config; by definition, the
> default behaviour is what is used when the user did not specify
> anything so what is usually done is to do that kind of defaulting
> before the code calls git_config() with a callback function like
> this.
>
> And more importantly, the users may have
>
>     [diff] ignoresubmodules=<value>
>
> in their configuration file.  After calling handle_ignore_submodules_arg()
> with the value the user desires, the above code will overwrite it with
> a hardcoded default---at that point that is no longer "the default"
> to be used when the user didn't specify.
>
> I am wondering if the init_diff_ui_defaults() function is the right
> location to add the above call.
>
> >  }
> >
> > But this would also involve a lot of changes in the way tests are
> > written as 12 out of 19 tests in t4027-diff-submodule.sh failed after
> > adding this patch.
>
> If the tests expect that the -dirty suffix is added at the end of
> "Subproject commit 2f256705..." when the submodule directory has a
> untracked file, it is expected that such tests need to be updated
> to the new world order you are introducing, which is "just like 'git
> describe --dirty' does not consider having an untracked file does not
> make otherwise clean checkout a dirty one, 'git diff' should not
> show that a submodule is dirty in its output if its working tree has
> an untracked file but is otherwise clean".
>
>
>
> What follows is a note for more experienced developers, but I notice
> that over the years, we seems to have done a shoddy job adjusting
> the implementation in diff.c file in the hope of adding support to
> work in multiple repositories; most file-scope static globals like
> default_diff_options and diff_detect_rename_default are still only
> read while in the main repository, yet repo_diff_setup() pretends as
> if an invocation of the diff machinery in a different repository can
> use settings that are repository specific.  Again, this is not
> something you need to be worried about.