[PATCH] builtin/describe.c: ignore untracked changes in submodules

[PATCH] builtin/describe.c: ignore untracked changes in submodules

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Since 'git describe' does not append -dirty to the version string it
produces when untracked files exist in the working directory of the main
repository, it should not do so for submodules either.

Add --ignore-submodules=untracked to the call to diff-index which is used
to decide whether or not the '-dirty' string is necessary.

Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
---
 builtin/describe.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index 43caff2..6c4f15b 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -29,7 +29,8 @@ static const char *dirty;
 
 /* diff-index command arguments to check if working tree is dirty. */
 static const char *diff_index_args[] = {
-	"diff-index", "--quiet", "HEAD", "--", NULL
+	"diff-index", "--quiet", "--ignore-submodules=untracked", "HEAD",
+	"--", NULL
 };
 
 
-- 
1.7.2.1

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Junio C Hamano]

Brandon Casey <casey@nrlssc.navy.mil> writes:

> From: Brandon Casey <drafnel@gmail.com>
>
> Since 'git describe' does not append -dirty to the version string it
> produces when untracked files exist in the working directory of the main
> repository, it should not do so for submodules either.
>
> Add --ignore-submodules=untracked to the call to diff-index which is used
> to decide whether or not the '-dirty' string is necessary.
>
> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
> ---

Hmm, this changes the behaviour in a big way but it probably is for the
better.  At least it is consistent with the recent fixes to the
interaction between diff and submodules.

Objections from submodule users?

>  builtin/describe.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index 43caff2..6c4f15b 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -29,7 +29,8 @@ static const char *dirty;
>  
>  /* diff-index command arguments to check if working tree is dirty. */
>  static const char *diff_index_args[] = {
> -	"diff-index", "--quiet", "HEAD", "--", NULL
> +	"diff-index", "--quiet", "--ignore-submodules=untracked", "HEAD",
> +	"--", NULL
>  };
>  
>  
> -- 
> 1.7.2.1

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Jens Lehmann]

>Brandon Casey <casey@nrlssc.navy.mil> writes:
>
>> From: Brandon Casey <drafnel@gmail.com>
>>
>> Since 'git describe' does not append -dirty to the version string it
>> produces when untracked files exist in the working directory of the main
>> repository, it should not do so for submodules either.
>>
>> Add --ignore-submodules=untracked to the call to diff-index which is used
>> to decide whether or not the '-dirty' string is necessary.
>>
>> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
>> ---
>
>Hmm, this changes the behaviour in a big way but it probably is for the
>better.  At least it is consistent with the recent fixes to the
>interaction between diff and submodules.

Hmm, by default the diff family considers submodules with untracked files as
dirty unless configured otherwise (and AFAICS the recent fixes to the interaction
between diff and submodule were options to configure your own default).

So when git status tells you the subodule is modified, e.g. because of an untracked
file, I would expect git describe to add '-dirty' to its output when requested. To get rid
of that I would expect you either fix the .gitignore of the submodule or configure that
you don't care about untracked files in submodules at all (either only for this
submodule or in the config).

So if I didn't misunderstand something here I would rather vote against this change,
git describe should append a '-dirty' when git status would show modifications, no?

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Jens Lehmann]

>>Brandon Casey <casey@nrlssc.navy.mil> writes:
>>
>>> From: Brandon Casey <drafnel@gmail.com>
>>>
>>> Since 'git describe' does not append -dirty to the version string it
>>> produces when untracked files exist in the working directory of the main
>>> repository, it should not do so for submodules either.
>>>
>>> Add --ignore-submodules=untracked to the call to diff-index which is used
>>> to decide whether or not the '-dirty' string is necessary.
>>>
>>> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
>>> ---
>>
>>Hmm, this changes the behaviour in a big way but it probably is for the
>>better.  At least it is consistent with the recent fixes to the
>>interaction between diff and submodules.
>
>Hmm, by default the diff family considers submodules with untracked files as
>dirty unless configured otherwise (and AFAICS the recent fixes to the interaction
>between diff and submodule were options to configure your own default).
>
>So when git status tells you the subodule is modified, e.g. because of an untracked
>file, I would expect git describe to add '-dirty' to its output when requested. To get rid
>of that I would expect you either fix the .gitignore of the submodule or configure that
>you don't care about untracked files in submodules at all (either only for this
>submodule or in the config).
>
>So if I didn't misunderstand something here I would rather vote against this change,
>git describe should append a '-dirty' when git status would show modifications, no?

And maybe we should teach "git describe" the "--ignore-submodules" option, then
you could tell describe what to pass to the diff-index command. Thoughts?

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>>So if I didn't misunderstand something here I would rather vote against this change,
>>git describe should append a '-dirty' when git status would show modifications, no?
>
> And maybe we should teach "git describe" the "--ignore-submodules" option, then
> you could tell describe what to pass to the diff-index command. Thoughts?

It is sensible to add the option, and handle_ignore_submodules_arg() call
to grab "diff.ignoresubmodules" configuration) to the command, perhaps.

Both "status" and "diff" are described in the documentation as having
"all" as the default value for --ignore-submodules option, but by default
neither ignores changes in the submodules.  So it seems consistent for the
command to take submodule changes into account by default.

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Jens Lehmann]

>Jens Lehmann <Jens.Lehmann@web.de> writes:
>> And maybe we should teach "git describe" the "--ignore-submodules" option, then
>> you could tell describe what to pass to the diff-index command. Thoughts?
>
>It is sensible to add the option, and handle_ignore_submodules_arg() call
>to grab "diff.ignoresubmodules" configuration) to the command, perhaps.

Ok, I'll look into that and prepare a patch.

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[yj2133011]

http://www.Gucci2u.com/ Gucci Shoes  now produces flavor styles of shoes,
loafers, clogs, and boots – and now even heels, Gucci Shoes have grown from
the chief feature innate supplies and emphasizing durability, comfort, and
strong feet, Gucci shoes for men makes men’s and women’s footwear. Having a
group of high responsibility, especially inquiries department, authentic
Gucci Shoes has put huge inquiries time studying how the time, Gucci changed
by the recurring shock of daily walking and workmanship. Despite some worth
issues over the person body is fitting a cult desired and is well on the
person body. Sometimes mistaken for Birkenstocks, Discount Gucci Shoes will
be assured that Gucci delivers the finest in value and operation. The
self-proclaimed “finest walking shoes in the world”, 
http://www.Gucci2u.com/ Gucci Shoes  takes an international success. Gucci
women 
boots are physiological footwear intended.







----------------------------------------------------------------------------------------

Visit our site for reviews, news and information about flight simulator
games ,
http://www.tomtop.com/toy-kids-baby/rc-electric-toys/helicopters-accessories/6ch-usb-3d-rc-helicopter-airplane-flight-simulator.html?aid=z
helicopter simulator .A 
http://www.tomtop.com/mini-spy-car-dv-key-chain-camera-dvr-video-recorder-dc.html?aid=z
keychain camera  is a very small and inexpensive digital camera.
-- 
View this message in context: http://git.661346.n2.nabble.com/PATCH-builtin-describe-c-ignore-untracked-changes-in-submodules-tp5515750p5522622.html
Sent from the git mailing list archive at Nabble.com.

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>>Jens Lehmann <Jens.Lehmann@web.de> writes:
>>> And maybe we should teach "git describe" the "--ignore-submodules" option, then
>>> you could tell describe what to pass to the diff-index command. Thoughts?
>>
>>It is sensible to add the option, and handle_ignore_submodules_arg() call
>>to grab "diff.ignoresubmodules" configuration) to the command, perhaps.
>
> Ok, I'll look into that and prepare a patch.

Thanks; and thanks for biting my sanity check weatherbaloon ;-)

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Brandon Casey]

On Fri, Sep 10, 2010 at 1:40 PM, Jens Lehmann <Jens.Lehmann@web.de> wrote:
>>Brandon Casey <casey@nrlssc.navy.mil> writes:
>>
>>> From: Brandon Casey <drafnel@gmail.com>
>>>
>>> Since 'git describe' does not append -dirty to the version string it
>>> produces when untracked files exist in the working directory of the main
>>> repository, it should not do so for submodules either.
>>>
>>> Add --ignore-submodules=untracked to the call to diff-index which is used
>>> to decide whether or not the '-dirty' string is necessary.
>>>
>>> Signed-off-by: Brandon Casey <casey@nrlssc.navy.mil>
>>> ---
>>
>>Hmm, this changes the behaviour in a big way but it probably is for the
>>better.  At least it is consistent with the recent fixes to the
>>interaction between diff and submodules.
>
> Hmm, by default the diff family considers submodules with untracked files as
> dirty unless configured otherwise (and AFAICS the recent fixes to the interaction
> between diff and submodule were options to configure your own default).
>
> So when git status tells you the subodule is modified, e.g. because of an untracked
> file, I would expect git describe to add '-dirty' to its output when requested.

Triple hmm.  Perhaps a deeper level change is necessary than what I originally
thought.

It appears to me now, that the behavior of the entire diff family is
inconsistent
with respect to how untracked content is handled at the super-project level and
at the submodule level.

At the super-project level, git only considers as 'modified', changes to
_tracked_ content.  Any untracked content is ignored by git-describe, git-diff,
and friends, and git-status places it in its own 'Untracked files' section.

At the submodule level, all files, tracked and untracked, are considered.

> To get rid
> of that I would expect you either fix the .gitignore of the submodule or configure that
> you don't care about untracked files in submodules at all (either only for this
> submodule or in the config).
>
> So if I didn't misunderstand something here I would rather vote against this change,
> git describe should append a '-dirty' when git status would show modifications, no?

Do you agree that there is an inconsistency between how untracked content is
treated at the super-project level and at the submodule level?  Any thoughts
about how the behavior should be made to be consistent?

Perhaps the default setting of submodule.<name>.ignore should be 'untracked'?

-Brandon

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jens Lehmann <Jens.Lehmann@web.de> writes:
>
>>>Jens Lehmann <Jens.Lehmann@web.de> writes:
>>>> And maybe we should teach "git describe" the "--ignore-submodules" option, then
>>>> you could tell describe what to pass to the diff-index command. Thoughts?
>>>
>>>It is sensible to add the option, and handle_ignore_submodules_arg() call
>>>to grab "diff.ignoresubmodules" configuration) to the command, perhaps.
>>
>> Ok, I'll look into that and prepare a patch.

By the way, I think that route of action would make the resulting git
internally consistent in that everything by default will report submodules
with untracked paths in its working tree as dirty, but sidesteps the
original issue Brandon raised, which I think is a valid concern.

 - In the "Untracked" section of "git status" output, we list an untracked
   path in the superproject (i.e. the one in which "git status" was run)
   to remind the user that the path might be a new file forgotten to be
   added (unless of course it is ignored).  But it does not make the
   working tree dirty.

 - When you have an untracked path in a submodule:

   - the submodule is listed in the "Changed but not updated" section.
     This also makes the working tree of the superproject dirty, even
     though the working tree of the submodule is _not_.

   - "git diff" output at the superproject level shows that the submodule
     has modifications (i.e. "-dirty" is shown), but when run inside the
     submodule, there is no change shown.

I think this is a misdesign at the UI level; reporting an untracked and
unignored path as potential mistake to remind the user is a good thing,
but the current way "status" and "diff" does so does not make much sense
to me.

In the "git status" output, there are three sections.  The way to view
them at the user level has always been:

 - "Changes to be committed": they are included if you say "git commit"
   without any pathspec nor option (aka "What you would commit");

 - "Changed but not updated": they are included if you say "git commit -a"
   (aka "What you could commit");

 - "Untracked files": they may be paths you forgot to add, and you might
   want to say "git add" on some of them and add ignore patterns to cover
   the others.

I have a suspicion that the change we made in 1.7.1 to say an untracked
path in a submodule counts as a "Changed but not updated" was a mistake.
In a project with submodules, if a "git commit" were to be run without any
other option, with the "-a" option, and with the "-A" option, shouldn't
the above three category behave exactly like how they behave in a project
without submodules?

IOW, for a submodule that is not pristine, shouldn't we be doing this?

 - "Changes to be committed": show a submodule path for which the
   superproject index records a commit that is different from what is
   recorded in the superproject HEAD tree (currently we are doing this
   correctly);

 - "Changed but not updated": show a submodule path for which the HEAD in
   the submodule differs from what is recorded in the superproject HEAD
   tree (we are contaminating this list with "untracked content");

 - "Untracked files": show a submodule path that is not in the index.

A submodule with untracked files may be worth knowing, but I don't think
they should fall into any of the above categories.  Perhaps they should be
listed in their own separate section (they may be listed in multiple
sections just like "edit F; git add F; edit F" may result in a path listed
both in to-be-committed and not-updated sections)?

I haven't formed an opinion as to what to do with "git diff" output that
adds "-dirty" for untracked paths.  c.f.

  8e08b41 (Teach diff that modified submodule directory is dirty, 2010-01-16)
  721ceec (Teach diff --submodule that modified submodule directory is dirty, 2010-01-24)
  

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Jens Lehmann]

>Do you agree that there is an inconsistency between how untracked content is
>treated at the super-project level and at the submodule level? 

Yes, but I - and others included in that discussion some time ago - could not
come up with a sane and simple solution to that problem.


> Any thoughts
>about how the behavior should be made to be consistent?

The core of this issue is that for git a file is either untracked, modified or clean.
But submodules can have every combination of all these states - as they consist
of multiple files - and additionally their HEAD can differ from the commit recorded
in the superproject. So basically I see two ways to handle that:
a) add new states for an entry to represent all missing combinations of possible
   states for submodules and tell all porcelain to handle these.
b) simplify this problem by having a submodule show up as modified when
   either of these three conditions are met (and enable the user to choose what
   conditions she wants to see and what not).

Obviously a) will complicate all git by a large degree just for the sake of submodules.
I am arguing for b), because submodules itself can be seen as a bunch of files which
don't interest me as single entities until I want to take a closer look. I think the issue
we are discussing here is the price we have to pay for this abstraction. I am very
open to proposals how to better handle that but so far I haven't seen any.


>Perhaps the default setting of submodule.<name>.ignore should be 'untracked'?

I still vote for none. I think the default should be to not have untracked files in
your projects (like you should not have warnings when compiling your project).
If that is not wanted, just use the configuration options git provides to change it.

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Jens Lehmann]

>>Perhaps the default setting of submodule.<name>.ignore should be 'untracked'?
>
>I still vote for none. I think the default should be to not have untracked files in
>your projects (like you should not have warnings when compiling your project).
>If that is not wanted, just use the configuration options git provides to change it.

I forgot to mention: I think untracked files should mark a submodule as modified
because otherwise it is too easy to forget adding new files inside submodules,
as they won't show up when the 'ignore' setting is 'untracked' .

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

>>>Perhaps the default setting of submodule.<name>.ignore should be 'untracked'?
>>
>>I still vote for none. I think the default should be to not have untracked files in
>>your projects (like you should not have warnings when compiling your project).
>>If that is not wanted, just use the configuration options git provides to change it.
>
> I forgot to mention: I think untracked files should mark a submodule as modified
> because otherwise it is too easy to forget adding new files inside submodules,
> as they won't show up when the 'ignore' setting is 'untracked' .

What makes untracked paths in the superproject different from the ones in
a submodule?  "git diff" cannot be it as it does not show untracked paths
in the superproject, so you are talking about "the user cannot tell from
the 'git status' output", right?

How about giving a new section in the status output that lists submodules
with untracked paths, and do so only when ignore-submodules is set not to
ignore them?

Re: [PATCH] builtin/describe.c: ignore untracked changes in submodules

[Jens Lehmann]

Am 14.09.2010 01:14, schrieb Junio C Hamano:
> What makes untracked paths in the superproject different from the ones in
> a submodule?

That you can have a different state for each path inside the superproject
(modified, untracked etc.) while you can't have that for the paths in the
submodule (when looked at from the superproject): There is only a single
state available for the whole submodule, it's either modified or it isn't.
So IMO a modified submodule should tell the user: "There is a change in
this submodule so that when you commit/push your superproject now, others
might run into problems when fetching it; you want to be sure this is not
the case before doing that". And this is just the same thing you could
say about a file in the superproject when it shows up as modified, no?
And for submodules this definition must also include new yet untracked
files, as they are very likely to be missing in every but your work tree.


> "git diff" cannot be it as it does not show untracked paths
> in the superproject, so you are talking about "the user cannot tell from
> the 'git status' output", right?

Nope, it's "git diff" too. The thing that got me started working on this
topic was that "git gui" and "gitk" were quiet about submodules which
had modified tracked files and/or new untracked files, which lead to
real world problems where I work. And both use diff-index and diff-files
to get the paths they should display *and* to display the actual changes.
(And as "git diff" uses the same machinery under the hood as "git status"
does, everything fell into place pretty easily)

And I argue that this is sane behavior, as I'm sure other tools rely on
"git diff" or "git status" too to check if there are modifications to the
work tree (or they call run_diff_files() or run_diff_index() directly to
do that). So all of these should agree on what they are saying about the
state of a submodule, or things will get interesting. (Same goes for
describe, it should append the "-dirty" when "git status" or "git diff"
say a submodule is modified)

And this approach works really well at my dayjob. Since we are using it,
me and my colleagues are really happy with it, because we can't forget to
commit changes inside a submodule anymore. So judging from this real life
experience "ignore=none" is a very sane default.

But I admit that this change in behavior can be strange for long time
submodule users when they first encounter it. And if they still don't
like the new behavior after some consideration, they can disable it
easily using the new configuration options. But one of the advantages I
really liked when I started using git was that is was not able to forget
to commit new files anymore. So I suspect ignore=none is especially
useful for new users of submodules, because it is on the safe side, and
therefore should be the default setting. You can later turn the 'noise'
down if you want (just like some users do when using the "-uno" option
to "git status" if they don't want to be told about untracked files in
the superproject or its submodules).