[Outreachy] Introduction

[Outreachy] Introduction

[Sangeeta NB]

Hello everyone,

My name is Sangeeta and I’m one of the Outreachy applicants.  I would
like to work on the microproject "Unify the meaning of dirty between
diff and describe".

While looking at the files for `describe` and `diff` commands I found
that the `describe.c`  is present in builtin[1] folder whereas diff.c
is found in the root[2] folder as well as builtin[3] folder. I could
not find any implementation of --dirty in the diff.c present in
builtin[3] folder. So is it that I have to compare the implementation
of describe.c[1] and diff.c(of root folder)?

Also, I was curious to know why is there a builtin folder when many
commands described in that are described again in the root folder?

Looking forward to working with you all.

Sangeeta
[1] https://github.com/git/git/blob/master/builtin/describe.c
[2] https://github.com/git/git/blob/master/builtin/diff.c
[3] https://github.com/git/git/blob/master/diff.c

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 07/10/2020 21:10, Sangeeta NB wrote:
> Hello everyone,

Welcome to the list

> My name is Sangeeta and I’m one of the Outreachy applicants.  I would
> like to work on the microproject "Unify the meaning of dirty between
> diff and describe".
> 
> While looking at the files for `describe` and `diff` commands I found
> that the `describe.c`  is present in builtin[1] folder whereas diff.c
> is found in the root[2] folder as well as builtin[3] folder. I could
> not find any implementation of --dirty in the diff.c present in
> builtin[3] folder. So is it that I have to compare the implementation
> of describe.c[1] and diff.c(of root folder)?
> 
> Also, I was curious to know why is there a builtin folder when many
> commands described in that are described again in the root folder?

The files in the root directory are (mostly) library code that ends up 
in libgit.a. The builtin directory contains the individual git commands 
that form the git binary that is linked with libgit.a. builtin/diff.c 
contains cmd_diff() which will be called when the user runs `git diff`. 
That function parses the command line options and sets up the necessary 
data to pass to the diff implementation in /diff.c. The diff and log 
family of commands are a bit different to most of the other commands in 
that the option parsing is mostly done by calling setup_revisions() in 
/revision.c rather than using the option parsing library routines in 
/parse-options.c directly. I think the `--dirty` option for diff ends up 
being handled by handle_ignore_submodules_arg() in submodule.c, I'll 
leave it to you to see where that is called from (you can use `git grep`).

I'm going to be off line for the rest of today, hopefully someone else 
will be able to help if you get stuck or I'll try and answer any other 
questions tomorrow.

Best Wishes

Phillip

> Looking forward to working with you all.
> 
> Sangeeta
> [1] https://github.com/git/git/blob/master/builtin/describe.c
> [2] https://github.com/git/git/blob/master/builtin/diff.c
> [3] https://github.com/git/git/blob/master/diff.c
> 

Re: [Outreachy] Introduction

[Sangeeta NB]

Thanks for the explanation, Philips. I think there's a long road ahead
to understand how everything is implemented and put together.

Coming to the microproject, it was said that there is an inconsistency
in --dirty behavior shown by `git diff` and `git describe --dirty` for
submodule state when the files are untracked.

From what I understood by looking at the code, the diff files states
that we should ignore untracked submodule states. So is it that I have
to make changes in the way git describe is implemented by ignoring the
changes in the untracked submodule?

Also, I wasn't able to look for this inconsistency in my local
machine. Any pointers on how to reproduce this might be helpful.

Thanks and regards,

Sangeeta

On Thu, Oct 8, 2020 at 2:37 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> Hi Sangeeta
>
> On 07/10/2020 21:10, Sangeeta NB wrote:
> > Hello everyone,
>
> Welcome to the list
>
> > My name is Sangeeta and I’m one of the Outreachy applicants.  I would
> > like to work on the microproject "Unify the meaning of dirty between
> > diff and describe".
> >
> > While looking at the files for `describe` and `diff` commands I found
> > that the `describe.c`  is present in builtin[1] folder whereas diff.c
> > is found in the root[2] folder as well as builtin[3] folder. I could
> > not find any implementation of --dirty in the diff.c present in
> > builtin[3] folder. So is it that I have to compare the implementation
> > of describe.c[1] and diff.c(of root folder)?
> >
> > Also, I was curious to know why is there a builtin folder when many
> > commands described in that are described again in the root folder?
>
> The files in the root directory are (mostly) library code that ends up
> in libgit.a. The builtin directory contains the individual git commands
> that form the git binary that is linked with libgit.a. builtin/diff.c
> contains cmd_diff() which will be called when the user runs `git diff`.
> That function parses the command line options and sets up the necessary
> data to pass to the diff implementation in /diff.c. The diff and log
> family of commands are a bit different to most of the other commands in
> that the option parsing is mostly done by calling setup_revisions() in
> /revision.c rather than using the option parsing library routines in
> /parse-options.c directly. I think the `--dirty` option for diff ends up
> being handled by handle_ignore_submodules_arg() in submodule.c, I'll
> leave it to you to see where that is called from (you can use `git grep`).
>
> I'm going to be off line for the rest of today, hopefully someone else
> will be able to help if you get stuck or I'll try and answer any other
> questions tomorrow.
>
> Best Wishes
>
> Phillip
>
> > Looking forward to working with you all.
> >
> > Sangeeta
> > [1] https://github.com/git/git/blob/master/builtin/describe.c
> > [2] https://github.com/git/git/blob/master/builtin/diff.c
> > [3] https://github.com/git/git/blob/master/diff.c
> >

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 09/10/2020 08:41, Sangeeta NB wrote:
> Thanks for the explanation, Philips. I think there's a long road ahead
> to understand how everything is implemented and put together.
> 
> Coming to the microproject, it was said that there is an inconsistency
> in --dirty behavior shown by `git diff` and `git describe --dirty` for
> submodule state when the files are untracked.

I struggled to find the mircoprojects page - I must have missed the link 
on the outreachy site. In case anyone else is struggling to find it here 
is the project

     Unify the meaning of -dirty between diff and describe

     git diff reports a submodule directory, whose tracked
     contents match the commit at the HEAD in the submodule, as
     -dirty when there is an untracked file in the submodule
     directory. This is inconsistent with what git describe
     --dirty says when run in the submodule directory in that
     state. [1]

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

>  From what I understood by looking at the code, the diff files states
> that we should ignore untracked submodule states. So is it that I have
> to make changes in the way git describe is implemented by ignoring the
> changes in the untracked submodule?

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.

> Also, I wasn't able to look for this inconsistency in my local
> machine. Any pointers on how to reproduce this might be helpful.

I'd start my trying to build git and running t4027-diff-submodule.sh.
If you look at the start of the test 'git diff HEAD with dirty submodule 
(untracked)' in t/t4027-diff-submodule.sh it sets up a submodule with an 
untracked file. If you add "test_pause &&" after the diff command in 
that test it will start a shell in the test directory and you can run 
`git diff HEAD` yourself  to see the output and also `git -C sub diff 
HEAD` which will run diff in the submodule directory. The latter command 
should show that there are no changes in the tracked files of the 
submodule. Just exit the shell to get the test to continue. (you can see 
in builtin/describe.c that when it is run with `--dirty` it runs `git 
diff-index HEAD` to determine if a repository is dirty). To change the 
output of diff I would look for the string "Subproject commit" in diff.c 
to find the code that adds '-dirty' and try working backwards from 
there. Let me know if you get stuck - it took we a while to work 
backwards to find where we check if the submodule is dirty.

Best Wishes

Phillip


> Thanks and regards,
> 
> Sangeeta
> 
> On Thu, Oct 8, 2020 at 2:37 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Sangeeta
>>
>> On 07/10/2020 21:10, Sangeeta NB wrote:
>>> Hello everyone,
>>
>> Welcome to the list
>>
>>> My name is Sangeeta and I’m one of the Outreachy applicants.  I would
>>> like to work on the microproject "Unify the meaning of dirty between
>>> diff and describe".
>>>
>>> While looking at the files for `describe` and `diff` commands I found
>>> that the `describe.c`  is present in builtin[1] folder whereas diff.c
>>> is found in the root[2] folder as well as builtin[3] folder. I could
>>> not find any implementation of --dirty in the diff.c present in
>>> builtin[3] folder. So is it that I have to compare the implementation
>>> of describe.c[1] and diff.c(of root folder)?
>>>
>>> Also, I was curious to know why is there a builtin folder when many
>>> commands described in that are described again in the root folder?
>>
>> The files in the root directory are (mostly) library code that ends up
>> in libgit.a. The builtin directory contains the individual git commands
>> that form the git binary that is linked with libgit.a. builtin/diff.c
>> contains cmd_diff() which will be called when the user runs `git diff`.
>> That function parses the command line options and sets up the necessary
>> data to pass to the diff implementation in /diff.c. The diff and log
>> family of commands are a bit different to most of the other commands in
>> that the option parsing is mostly done by calling setup_revisions() in
>> /revision.c rather than using the option parsing library routines in
>> /parse-options.c directly. I think the `--dirty` option for diff ends up
>> being handled by handle_ignore_submodules_arg() in submodule.c, I'll
>> leave it to you to see where that is called from (you can use `git grep`).
>>
>> I'm going to be off line for the rest of today, hopefully someone else
>> will be able to help if you get stuck or I'll try and answer any other
>> questions tomorrow.
>>
>> Best Wishes
>>
>> Phillip
>>
>>> Looking forward to working with you all.
>>>
>>> Sangeeta
>>> [1] https://github.com/git/git/blob/master/builtin/describe.c
>>> [2] https://github.com/git/git/blob/master/builtin/diff.c
>>> [3] https://github.com/git/git/blob/master/diff.c
>>>

Re: [Outreachy] Introduction

[Sangeeta NB]

Thanks for the help, Philip.

On Fri, Oct 9, 2020 at 11:59 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

> I struggled to find the mircoprojects page - I must have missed the link
> on the outreachy site.

In case anyone else is struggling to find the microprojects page,
here's the link [1]

[1] https://git.github.io/Outreachy-21-Microprojects/

> 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.

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

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

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!

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 11/10/2020 12:30, Sangeeta NB wrote:
> Thanks for the help, Philip.
> 
> On Fri, Oct 9, 2020 at 11:59 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>> I struggled to find the mircoprojects page - I must have missed the link
>> on the outreachy site.
> 
> In case anyone else is struggling to find the microprojects page,
> here's the link [1]
> 
> [1] https://git.github.io/Outreachy-21-Microprojects/
> 
>> 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.

That's great

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

I think that is a good route forward, we probably want to change the 
default for `diff-index` and `diff-files` as well

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

We'd expect some tests to fail but only the ones that are testing if 
untracked files cause the submodule to be considered dirty.

git_diff_ui_config() is a callback that is invoked once per config key 
in the config files so I don't think it is a good place to make the 
change as it is inefficient and overrides the users' 
`diff.ignoreSubmodules` setting . It also only applies to `diff` and not 
`diff-index` or `diff-files`. I think it would be better to set the 
default in diff_setup() though we need to be careful not to override 
`diff.ignoreSubmodules` setting so we might need to add a global flag to 
remember if the user has set `diff.ignoreSubmodules` in their config

Best Wishes

Phillip

> 

Re: [Outreachy] Introduction

[Kaartic Sivaraam]

Hi Sangeeta,

On 11/10/20 5:00 pm, Sangeeta NB wrote:
> 
>> 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].

> 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.

> 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

Re: [Outreachy] Introduction

[Junio C Hamano]

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.

Re: [Outreachy] Introduction

[Sangeeta NB]

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:
>
> 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.

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 14/10/2020 16:52, Sangeeta NB wrote:
> 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.

I've spent some time looking at it and I cannot understand what is going 
on :-( I think it may possibly be something to do with that test looking 
at a nested submodule and the options not being properly propagated down 
to that submodule - it might be worth looking at the diff code that 
handles submodules.

I've got a fixup which I'll post after this which gets rid of the global 
flag and instead uses a flag in struct diff_options. I had a quick look 
through the test changes and I thinking it would be worth considering if 
some of them should instead be changed to pass --ignore-submodules=none 
rather than changing the expected result.

Best Wishes

Phillip

> 
> [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:
>>
>> 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.

[PATCH] fixup! diff: do not show submodule with untracked files as "-dirty"

[Phillip Wood]

Simplify checking if we've already set the flags for ignoring submodules
---
 builtin/checkout.c |  1 -
 diff.c             | 12 +++++-------
 diff.h             |  7 +------
 submodule.c        |  5 ++---
 wt-status.c        |  3 ---
 5 files changed, 8 insertions(+), 20 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 428201380e..0951f8fee5 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1095,7 +1095,6 @@ static int git_checkout_config(const char *var, const char *value, void *cb)
 {
 	if (!strcmp(var, "diff.ignoresubmodules")) {
 		struct checkout_opts *opts = cb;
-		diff_ignore_submodule_config =1;
 		handle_ignore_submodules_arg(&opts->diff_options, value);
 		return 0;
 	}
diff --git a/diff.c b/diff.c
index 6c9d28d363..93bbb57f88 100644
--- a/diff.c
+++ b/diff.c
@@ -51,7 +51,6 @@ static int diff_no_prefix;
 static int diff_relative;
 static int diff_stat_graph_width;
 static int diff_dirstat_permille_default = 30;
-int diff_ignore_submodule_config = 0;
 static struct diff_options default_diff_options;
 static long diff_algorithm;
 static unsigned ws_error_highlight_default = WSEH_NEW;
@@ -403,10 +402,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)
 	if (!strcmp(var, "diff.orderfile"))
 		return git_config_pathname(&diff_order_file_cfg, var, value);
 
-	if (!strcmp(var, "diff.ignoresubmodules")) {
-		diff_ignore_submodule_config =1;
+	if (!strcmp(var, "diff.ignoresubmodules"))
 		handle_ignore_submodules_arg(&default_diff_options, value);
-	}
 
 	if (!strcmp(var, "diff.submodule")) {
 		if (parse_submodule_params(&default_diff_options, value))
@@ -4598,8 +4595,10 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
 
 	options->color_moved = diff_color_moved_default;
 	options->color_moved_ws_handling = diff_color_moved_ws_default;
-	if(!diff_ignore_submodule_config)
-		handle_ignore_submodules_arg(options, "untracked");
+
+	if(!options->flags.ignore_submodule_set)
+		options->flags.ignore_untracked_in_submodules = 1;
+
 	prep_parse_options(options);
 }
 
@@ -5142,7 +5141,6 @@ static int diff_opt_ignore_submodules(const struct option *opt,
 	if (!arg)
 		arg = "all";
 	options->flags.override_submodule_config = 1;
-	diff_ignore_submodule_config =1;
 	handle_ignore_submodules_arg(options, arg);
 	return 0;
 }
diff --git a/diff.h b/diff.h
index 9bfce084d8..2f5612ff7d 100644
--- a/diff.h
+++ b/diff.h
@@ -180,6 +180,7 @@ struct diff_flags {
 	unsigned ignore_untracked_in_submodules;
 	unsigned ignore_dirty_submodules;
 	unsigned override_submodule_config;
+	unsigned ignore_submodule_set;
 	unsigned dirstat_by_line;
 	unsigned funccontext;
 	unsigned default_follow_renames;
@@ -556,12 +557,6 @@ int diff_queue_is_empty(void);
 void diff_flush(struct diff_options*);
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
 
-/* 
- * This is for keeping a track whether ignoreSubmodules
- * is  present in user defined config or not.
- */
-extern int diff_ignore_submodule_config;
-
 /* diff-raw status letters */
 #define DIFF_STATUS_ADDED		'A'
 #define DIFF_STATUS_COPIED		'C'
diff --git a/submodule.c b/submodule.c
index a526c96ce2..8f6227c993 100644
--- a/submodule.c
+++ b/submodule.c
@@ -198,10 +198,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
 			ignore = submodule->ignore;
 		free(key);
 
-		if (ignore){
-			diff_ignore_submodule_config =1;
+		if (ignore)
 			handle_ignore_submodules_arg(diffopt, ignore);
-		}
 		else if (is_gitmodules_unmerged(the_repository->index))
 			diffopt->flags.ignore_submodules = 1;
 	}
@@ -422,6 +420,7 @@ const char *submodule_strategy_to_string(const struct submodule_update_strategy
 void handle_ignore_submodules_arg(struct diff_options *diffopt,
 				  const char *arg)
 {
+	diffopt->flags.ignore_submodule_set = 1;
 	diffopt->flags.ignore_submodules = 0;
 	diffopt->flags.ignore_untracked_in_submodules = 0;
 	diffopt->flags.ignore_dirty_submodules = 0;
diff --git a/wt-status.c b/wt-status.c
index 791af9321a..7074bbdd53 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -605,7 +605,6 @@ static void wt_status_collect_changes_worktree(struct wt_status *s)
 		rev.diffopt.flags.ignore_untracked_in_submodules = 1;
 	if (s->ignore_submodule_arg) {
 		rev.diffopt.flags.override_submodule_config = 1;
-		diff_ignore_submodule_config =1;
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	}
 	rev.diffopt.format_callback = wt_status_collect_changed_cb;
@@ -630,7 +629,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 	rev.diffopt.flags.override_submodule_config = 1;
 	rev.diffopt.ita_invisible_in_index = 1;
 	if (s->ignore_submodule_arg) {
-		diff_ignore_submodule_config =1;
 		handle_ignore_submodules_arg(&rev.diffopt, s->ignore_submodule_arg);
 	} else {
 		/*
@@ -641,7 +639,6 @@ static void wt_status_collect_changes_index(struct wt_status *s)
 		 * shown any submodules she manually added (and which are
 		 * staged to be committed), which would be really confusing.
 		 */
-		diff_ignore_submodule_config =1;
 		handle_ignore_submodules_arg(&rev.diffopt, "dirty");
 	}
 
-- 
2.25.1.551.gd3318bf0d3.dirty

Re: [Outreachy] Introduction

[Sangeeta NB]

Hey,

> I've got a fixup which I'll post after this which gets rid of the global
> flag and instead uses a flag in struct diff_options.

Thanks for the patch, I was thinking about something on similar lines
but couldn't come up with anything.

Also, one thing I observed that when I add a printf statement in
wt-status.c, something like this:

--- a/wt-status.c
+++ b/wt-status.c
@@ -601,11 +601,11 @@ static void
wt_status_collect_changes_worktree(struct wt_status *s)
        rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
        rev.diffopt.flags.dirty_submodules = 1;
        rev.diffopt.ita_invisible_in_index = 1;
+       printf("a printf statement\n");
        if (!s->show_untracked_files)

then git status shows output:

sh-3.2$ git status -s -uno --ignore-submodules=none
a printf statement
 m submod

which is what is expected. But when I comment out the printf statement
it again gives no output. I couldn't understand why this is taking
place and how can a printf line modify the behavior of git status.

> I thinking it would be worth considering if
> some of them should instead be changed to pass --ignore-submodules=none
> rather than changing the expected result.

Ya, that's a good suggestion. Would look at those tests again and see
if I can pass the --ignore-submodules=none option.

Thanks

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 15/10/2020 11:18, Sangeeta NB wrote:
> Hey,
> 
>> I've got a fixup which I'll post after this which gets rid of the global
>> flag and instead uses a flag in struct diff_options.
> 
> Thanks for the patch, I was thinking about something on similar lines
> but couldn't come up with anything.

As we store the config options in default_diff_options and then copy 
them across at the beginning of repo_setup_diff() we can use a flag in 
struct diff_options which is set by handle_ignore_submodule_arg() to 
tell if we need to initialize opts->flags.ignore_untracked_in_submodules 
in repo_setup_diff()

> Also, one thing I observed that when I add a printf statement in
> wt-status.c, something like this:
> 
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -601,11 +601,11 @@ static void
> wt_status_collect_changes_worktree(struct wt_status *s)
>          rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
>          rev.diffopt.flags.dirty_submodules = 1;
>          rev.diffopt.ita_invisible_in_index = 1;
> +       printf("a printf statement\n");
>          if (!s->show_untracked_files)
> 
> then git status shows output:
> 
> sh-3.2$ git status -s -uno --ignore-submodules=none
> a printf statement
>   m submod
> 
> which is what is expected. But when I comment out the printf statement
> it again gives no output. I couldn't understand why this is taking
> place and how can a printf line modify the behavior of git status.

Are you adding the printf and then running t3600? If so then the extra 
line of output breaks a lot of tests which in turn breaks to setup for 
the test that was failing so there are uncommitted changes. 
Unfortunately it is hard to run a subset of tests in a lot the test 
scripts as there are implicit dependencies between the individual tests 
them.

>> I thinking it would be worth considering if
>> some of them should instead be changed to pass --ignore-submodules=none
>> rather than changing the expected result.
> 
> Ya, that's a good suggestion. Would look at those tests again and see
> if I can pass the --ignore-submodules=none option.

For the diff tests I think we want to test the new default and check 
that --ignore-submodules=none works. I think for the other tests we 
probably want to just add --ignore-submodules=none

I'm afraid I'm still no closer to figuring out why that test in t3600 fails

Phillip

> Thanks
> 

Re: [Outreachy] Introduction

[Sangeeta NB]

On Thu, Oct 15, 2020 at 7:09 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
Hey Phillip,

> As we store the config options in default_diff_options and then copy
> them across at the beginning of repo_setup_diff() we can use a flag in
> struct diff_options which is set by handle_ignore_submodule_arg() to
> tell if we need to initialize opts->flags.ignore_untracked_in_submodules
> in repo_setup_diff()

Even if we don't set a global flag it is working fine because we are
setting the default first, and would let the config override it. I
have updated the code in the PR and you can have a look at it. I have
also added --ignore-submodules=none in some tests to get the results
mentioned earlier.

> Are you adding the printf and then running t3600? If so then the extra
> line of output breaks a lot of tests which in turn breaks to setup for
> the test that was failing so there are uncommitted changes.
> Unfortunately it is hard to run a subset of tests in a lot the test
> scripts as there are implicit dependencies between the individual tests
> them.
>
Oh, okay it makes sense.

>
> I'm afraid I'm still no closer to figuring out why that test in t3600 fails

What it is like debugging in Git? I have seen people writing debug
statements(print statements in between the code) to figure out how
things are working. But I guess we might not be able to do that. Do we
have to create the exact environment that is been created by that test
to check for the code?

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 15/10/2020 14:57, Sangeeta NB wrote:
> On Thu, Oct 15, 2020 at 7:09 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> Hey Phillip,
> 
>> As we store the config options in default_diff_options and then copy
>> them across at the beginning of repo_setup_diff() we can use a flag in
>> struct diff_options which is set by handle_ignore_submodule_arg() to
>> tell if we need to initialize opts->flags.ignore_untracked_in_submodules
>> in repo_setup_diff()
> 
> Even if we don't set a global flag it is working fine because we are
> setting the default first, and would let the config override it. I
> have updated the code in the PR and you can have a look at it. I have
> also added --ignore-submodules=none in some tests to get the results
> mentioned earlier.

Thanks, I'll have a look later

>> Are you adding the printf and then running t3600? If so then the extra
>> line of output breaks a lot of tests which in turn breaks to setup for
>> the test that was failing so there are uncommitted changes.
>> Unfortunately it is hard to run a subset of tests in a lot the test
>> scripts as there are implicit dependencies between the individual tests
>> them.
>>
> Oh, okay it makes sense.
> 
>>
>> I'm afraid I'm still no closer to figuring out why that test in t3600 fails

diff --git a/submodule.c b/submodule.c
index 8f6227c993..c4182be633 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1679,6 +1679,8 @@ unsigned is_submodule_modified(const char *path, 
int ignore_untracked)
         strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
         if (ignore_untracked)
                 strvec_push(&cp.args, "-uno");
+       else
+               strvec_push (&cp.args, "--ignore-submodules=none");

         prepare_submodule_repo_env(&cp.env_array);
         cp.git_cmd = 1;

fixes it, I'm unsure at the moment if we should be adding the extra flag 
here or setting the appropriate option in status when -uno and 
--ignore-submodules=<option> are both omitted though

> What it is like debugging in Git? I have seen people writing debug
> statements(print statements in between the code) to figure out how
> things are working. But I guess we might not be able to do that. Do we
> have to create the exact environment that is been created by that test
> to check for the code?

Have you setup a config.mak file? Mine looks like

DEVELOPER = 1
SANITIZE = address,leak
CFLAGS += -ggdb3
CFLAGS += -fvar-tracking-assignments
CFLAGS += -fno-omit-frame-pointer

Which will build git with warnings enabled, debugging information and 
enables the address sanitizer. Then you can run the git you have built 
under gdb with

	GIT_DEBUGGER=1 bin-wrappers/git

If you want to debug a particular test then I find adding `test_pause` 
to the test and then running

	GIT_DEBUGGER=1 git

in the shell that the test opens (it sets up the path appropriately). 
You may want to add LSAN_OPTIONS=detect_leaks=0 to the commands above or 
set up a suppressions file

I also use printf quite a bit but it does tend to break other tests 
which can be awkward.

Best Wishes

Phillip

Re: [Outreachy] Introduction

[Sangeeta NB]

Hey everyone,

On Thu, Oct 15, 2020 at 8:15 PM Phillip Wood <phillip.wood123@gmail.com> wrote:

> diff --git a/submodule.c b/submodule.c
> index 8f6227c993..c4182be633 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1679,6 +1679,8 @@ unsigned is_submodule_modified(const char *path,
> int ignore_untracked)
>          strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
>          if (ignore_untracked)
>                  strvec_push(&cp.args, "-uno");
> +       else
> +               strvec_push (&cp.args, "--ignore-submodules=none");
>
>          prepare_submodule_repo_env(&cp.env_array);
>          cp.git_cmd = 1;
>
> fixes it, I'm unsure at the moment if we should be adding the extra flag
> here or setting the appropriate option in status when -uno and
> --ignore-submodules=<option> are both omitted though

Ya, that does work and the PR passed all the tests after this correction.
I have submitted the patch[1] for it and would be glad to have reviews
on it from the git community.

[1] https://public-inbox.org/git/pull.751.git.1602781723670.gitgitgadget@gmail.com/T/#u

>
> Have you setup a config.mak file? Mine looks like
>
> DEVELOPER = 1
> SANITIZE = address,leak
> CFLAGS += -ggdb3
> CFLAGS += -fvar-tracking-assignments
> CFLAGS += -fno-omit-frame-pointer
>
> Which will build git with warnings enabled, debugging information and
> enables the address sanitizer. Then you can run the git you have built
> under gdb with
>
>         GIT_DEBUGGER=1 bin-wrappers/git
>
> If you want to debug a particular test then I find adding `test_pause`
> to the test and then running
>
>         GIT_DEBUGGER=1 git
>
> in the shell that the test opens (it sets up the path appropriately).
> You may want to add LSAN_OPTIONS=detect_leaks=0 to the commands above or
> set up a suppressions file
>
> I also use printf quite a bit but it does tend to break other tests
> which can be awkward.
>

No, not yet. I would set it up. Thanks again!

As my next step, I was looking for some #good-first-issue to work on
where I found an issue[2]. Has someone already worked on it? If not, I
would love to work on this.
Or if you have anything else in mind that I could work on please do
suggest to me.

[2] https://github.com/gitgitgadget/git/issues/486[3]

Thanks and Regards,
Sangeeta

Re: [Outreachy] Introduction

[Phillip Wood]

Hi Sangeeta

On 16/10/2020 06:27, Sangeeta NB wrote:
> Hey everyone,
> 
> On Thu, Oct 15, 2020 at 8:15 PM Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
>> diff --git a/submodule.c b/submodule.c
>> index 8f6227c993..c4182be633 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -1679,6 +1679,8 @@ unsigned is_submodule_modified(const char *path,
>> int ignore_untracked)
>>           strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
>>           if (ignore_untracked)
>>                   strvec_push(&cp.args, "-uno");
>> +       else
>> +               strvec_push (&cp.args, "--ignore-submodules=none");
>>
>>           prepare_submodule_repo_env(&cp.env_array);
>>           cp.git_cmd = 1;
>>
>> fixes it, I'm unsure at the moment if we should be adding the extra flag
>> here or setting the appropriate option in status when -uno and
>> --ignore-submodules=<option> are both omitted though
> 
> Ya, that does work and the PR passed all the tests after this correction.
> I have submitted the patch[1] for it and would be glad to have reviews
> on it from the git community.
> 
> [1] https://public-inbox.org/git/pull.751.git.1602781723670.gitgitgadget@gmail.com/T/#u

I'll try and have a proper read through at the beginning of next week. 
Hopefully others who use submodules more regularly will be have time to 
comment as well

>>
>> Have you setup a config.mak file? Mine looks like
>>
>> DEVELOPER = 1
>> SANITIZE = address,leak
>> CFLAGS += -ggdb3
>> CFLAGS += -fvar-tracking-assignments
>> CFLAGS += -fno-omit-frame-pointer
>>
>> Which will build git with warnings enabled, debugging information and
>> enables the address sanitizer. Then you can run the git you have built
>> under gdb with
>>
>>          GIT_DEBUGGER=1 bin-wrappers/git
>>
>> If you want to debug a particular test then I find adding `test_pause`
>> to the test and then running
>>
>>          GIT_DEBUGGER=1 git
>>
>> in the shell that the test opens (it sets up the path appropriately).
>> You may want to add LSAN_OPTIONS=detect_leaks=0 to the commands above or
>> set up a suppressions file
>>
>> I also use printf quite a bit but it does tend to break other tests
>> which can be awkward.
>>
> 
> No, not yet. I would set it up. Thanks again!
> 
> As my next step, I was looking for some #good-first-issue to work on
> where I found an issue[2]. Has someone already worked on it? If not, I
> would love to work on this.
> Or if you have anything else in mind that I could work on please do
> suggest to me.

I'm not sure if someone else has worked on that - there has been some 
work to convert more of bisect to C recently [1]. It should be easy 
enough to test if bisect works from a subdirectory or not. I don't have 
anything else in mind - my advice would be to pick things that interest you

[1] https://lore.kernel.org/git/20201015133838.85524-1-mirucam@gmail.com

Best Wishes

Phillip

> [2] https://github.com/gitgitgadget/git/issues/486[3]
> 
> Thanks and Regards,
> Sangeeta
>