Re: [Outreachy] Introduction

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Oct 12, 2020, 11:22 UTC
Message-ID: <943d84e6-98fb-29e3-086a-3763c9e73ef8@gmail.com>
In-Reply-To: <CAHjREB6j6BqZ49wX5uqEOiysTAm8Oo7N=EFpcoovWKkBghBjxQ@mail.gmail.com>

Hi Sangeeta,

On 11/10/20 5:00 pm, Sangeeta NB wrote:

Show 12 quoted lines

> 
>> As I understand it if a submodule contains any untracked files (i.e. a
>> file that has not been added with `git add` and is not ignored by any
>> .gitignore or .git/info/exclude entries) then running `git diff` in the
>> superproject will report that the submodule is dirty - there will be a
>> line something like "+Subproject commit abcdef-dirty". However if we run
>> `git describe --dirty` in the submodule directory then it will not
>> append "-dirty" to it's output unless there are changes to tracked files.
> 
> On running `git diff HEAD --ignore-submodules=untracked` the submodule
> wasn't reported as dirty.
>

Right.

> I guess this is what we are expecting. So should I make it the default
> behavior for diff?
>

Yeah. Changing the default behaviour of 'diff' looks like one of the options that Junio mentions in [1].

Show 13 quoted lines

> 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);
>   }
>

I'm not sure about whether 'git_diff_ui_config' is the right place to do this, though. I'm also not sure about what the right approach would be, off-hand. But I believe the Junio's e-mail I reference might be of help in pointing you in the right direction, in general.

Show 6 quoted lines

> 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. I am working on any other workaround for this. Let
> me know whether I am on right path or not. Also any pointers on how to
> proceed would be helpful. Thanks!
>

Some test failures are likely to happens as a consequence of the change given that we would be changing default behaviour. So, adjusting the tests appropriately would indeed be necessary. We would've to be careful in evaluating the failures so that we don't break other _valid_ use cases as a side-effect.

[ References ]

[1]: https://lore.kernel.org/git/xmqq1rixi4cb.fsf@gitster.c.googlers.com/

-- Sivaraam