Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Junio C Hamano]

Stefan Beller  writes:

> Side-question:
> Is there some doc (commit message), that explains the difference
> between CE_REMOVE and CE_WT_REMOVE ?

That's something you need to ask Duy, I think, as it was introduced
at e663db2f ("unpack-trees(): add CE_WT_REMOVE to remove on worktree
alone", 2009-08-20) and was added for the sparse checkout stuff.

During my review of the series that added the feature, I only had
time to make sure that the patches do not change the behaviour when
it is not in use, ignoring the other side of "if" statement that
checked ce_skip_worktree(ce) ;-)

Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Stefan Beller]

On Tue, Mar 7, 2017 at 5:14 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> submodule_from_ce returns always NULL, when such flag is not given.
>> From 10/18:
>>
>> +const struct submodule *submodule_from_ce(const struct cache_entry *ce)
>> +{
>> +   if (!S_ISGITLINK(ce->ce_mode))
>> +   return NULL;
>> +
>> +   if (!should_update_submodules())
>> +   return NULL;
>> +
>> +   return submodule_from_path(null_sha1, ce->name);
>> +}
>>
>> should_update_submodules is always false if such a flag is not set,
>> such that we end up in the else case which is literally the same as
>> the removed lines (they are just indented).
>
> I see.
>
> I didn't think a function this deep in the callchain that does not
> take any parameter could possibly change the behaviour based on the
> end-user input.  I was expecting that such a state (i.e. are we
> recursive?  are we allowed to forcibly update the working tree
> files?) would be kept part of something like "struct checkout" and
> passed around the callchain.
>
> That was why I didn't look at how that function answers "should
> update?" question, and got puzzled.  Because it would imply there is
> some hidden state that is accessible by everybody--a global variable
> or something--which would point at a deeper design issue.

Well it is just as deep as e.g. reacting on some bits of
struct unpack_trees_options in unpack-trees.c ?

But I can see how this is an issue with design.
I also think my previous answer was a straw man.

We only need to differentiate between 'ce' being a submodule
or not, because it should not be marked CE_REMOVE in
the non-recursive state.

Side-question:
Is there some doc (commit message), that explains the difference
between CE_REMOVE and CE_WT_REMOVE ?

Thanks,
Stefan

Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Junio C Hamano]

Stefan Beller  writes:

> submodule_from_ce returns always NULL, when such flag is not given.
> From 10/18:
>
> +const struct submodule *submodule_from_ce(const struct cache_entry *ce)
> +{
> +   if (!S_ISGITLINK(ce->ce_mode))
> +   return NULL;
> +
> +   if (!should_update_submodules())
> +   return NULL;
> +
> +   return submodule_from_path(null_sha1, ce->name);
> +}
>
> should_update_submodules is always false if such a flag is not set,
> such that we end up in the else case which is literally the same as
> the removed lines (they are just indented).

I see.  

I didn't think a function this deep in the callchain that does not
take any parameter could possibly change the behaviour based on the
end-user input.  I was expecting that such a state (i.e. are we
recursive?  are we allowed to forcibly update the working tree
files?) would be kept part of something like "struct checkout" and
passed around the callchain.

That was why I didn't look at how that function answers "should
update?" question, and got puzzled.  Because it would imply there is
some hidden state that is accessible by everybody--a global variable
or something--which would point at a deeper design issue.

Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Stefan Beller]

On Tue, Mar 7, 2017 at 2:42 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> Signed-off-by: Stefan Beller 

>> + submodule_move_head(sub->path, "HEAD", NULL, \
>
> What is this backslash doing here?

I am still bad at following coding conventions, apparently.
will remove.

>> @@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state 
>> *istate)
>>
>>   for (i = j = 0; i < istate->cache_nr; i++) {
>>   if (ce_array[i]->ce_flags & CE_REMOVE) {
>> - remove_name_hash(istate, ce_array[i]);
>> - save_or_free_index_entry(istate, ce_array[i]);
>> + const struct submodule *sub = 
>> submodule_from_ce(ce_array[i]);
>> + if (sub) {
>> + remove_submodule_according_to_strategy(sub);
>> + } else {
>> + remove_name_hash(istate, ce_array[i]);
>> + save_or_free_index_entry(istate, ce_array[i]);
>> + }
>
> I cannot readily tell as the proposed log message is on the sketchy
> side to explain the reasoning behind this, but this behaviour change
> is done unconditionally (in other words, without introducing a flag
> that is set by --recurse-submodules command line flag and switches
> behaviour based on the flag) here.  What is the end-user visible
> effect with this change?

submodule_from_ce returns always NULL, when such flag is not given.
>From 10/18:

+const struct submodule *submodule_from_ce(const struct cache_entry *ce)
+{
+   if (!S_ISGITLINK(ce->ce_mode))
+   return NULL;
+
+   if (!should_update_submodules())
+   return NULL;
+
+   return submodule_from_path(null_sha1, ce->name);
+}

should_update_submodules is always false if such a flag is not set,
such that we end up in the else case which is literally the same as
the removed lines (they are just indented).

>  Can we have a new test in this same patch
> to demonstrate the desired behaviour introduced by this change (or
> the bug this change fixes)?

This doesn't fix a bug, but in case the flag is given (in patches 17,18)
this new code removes submodules that are no longer recorded in
the tree.

Re: [PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Junio C Hamano]

Stefan Beller  writes:

> Signed-off-by: Stefan Beller 
> ---
>  read-cache.c | 27 +--
>  1 file changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/read-cache.c b/read-cache.c
> index 9054369dd0..9a2abacf7a 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -18,6 +18,8 @@
>  #include "varint.h"
>  #include "split-index.h"
>  #include "utf8.h"
> +#include "submodule.h"
> +#include "submodule-config.h"
>  
>  /* Mask for the name length in ce_flags in the on-disk index */
>  
> @@ -520,6 +522,22 @@ int remove_index_entry_at(struct index_state *istate, 
> int pos)
>   return 1;
>  }
>  
> +static void remove_submodule_according_to_strategy(const struct submodule 
> *sub)
> +{
> + switch (sub->update_strategy.type) {
> + case SM_UPDATE_UNSPECIFIED:
> + case SM_UPDATE_CHECKOUT:
> + case SM_UPDATE_REBASE:
> + case SM_UPDATE_MERGE:
> + submodule_move_head(sub->path, "HEAD", NULL, \

What is this backslash doing here?

> + SUBMODULE_MOVE_HEAD_FORCE);
> + break;
> + case SM_UPDATE_NONE:
> + case SM_UPDATE_COMMAND:
> + ; /* Do not touch the submodule. */
> + }
> +}
> +
>  /*
>   * Remove all cache entries marked for removal, that is where
>   * CE_REMOVE is set in ce_flags.  This is much more effective than
> @@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state 
> *istate)
>  
>   for (i = j = 0; i < istate->cache_nr; i++) {
>   if (ce_array[i]->ce_flags & CE_REMOVE) {
> - remove_name_hash(istate, ce_array[i]);
> - save_or_free_index_entry(istate, ce_array[i]);
> + const struct submodule *sub = 
> submodule_from_ce(ce_array[i]);
> + if (sub) {
> + remove_submodule_according_to_strategy(sub);
> + } else {
> + remove_name_hash(istate, ce_array[i]);
> + save_or_free_index_entry(istate, ce_array[i]);
> + }

I cannot readily tell as the proposed log message is on the sketchy
side to explain the reasoning behind this, but this behaviour change
is done unconditionally (in other words, without introducing a flag
that is set by --recurse-submodules command line flag and switches
behaviour based on the flag) here.  What is the end-user visible
effect with this change?  Can we have a new test in this same patch
to demonstrate the desired behaviour introduced by this change (or
the bug this change fixes)?

>   }
>   else
>   ce_array[j++] = ce_array[i];

[PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 read-cache.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 9054369dd0..9a2abacf7a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -18,6 +18,8 @@
 #include "varint.h"
 #include "split-index.h"
 #include "utf8.h"
+#include "submodule.h"
+#include "submodule-config.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -520,6 +522,22 @@ int remove_index_entry_at(struct index_state *istate, int 
pos)
return 1;
 }
 
+static void remove_submodule_according_to_strategy(const struct submodule *sub)
+{
+   switch (sub->update_strategy.type) {
+   case SM_UPDATE_UNSPECIFIED:
+   case SM_UPDATE_CHECKOUT:
+   case SM_UPDATE_REBASE:
+   case SM_UPDATE_MERGE:
+   submodule_move_head(sub->path, "HEAD", NULL, \
+   SUBMODULE_MOVE_HEAD_FORCE);
+   break;
+   case SM_UPDATE_NONE:
+   case SM_UPDATE_COMMAND:
+   ; /* Do not touch the submodule. */
+   }
+}
+
 /*
  * Remove all cache entries marked for removal, that is where
  * CE_REMOVE is set in ce_flags.  This is much more effective than
@@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state 
*istate)
 
for (i = j = 0; i < istate->cache_nr; i++) {
if (ce_array[i]->ce_flags & CE_REMOVE) {
-   remove_name_hash(istate, ce_array[i]);
-   save_or_free_index_entry(istate, ce_array[i]);
+   const struct submodule *sub = 
submodule_from_ce(ce_array[i]);
+   if (sub) {
+   remove_submodule_according_to_strategy(sub);
+   } else {
+   remove_name_hash(istate, ce_array[i]);
+   save_or_free_index_entry(istate, ce_array[i]);
+   }
}
else
ce_array[j++] = ce_array[i];
-- 
2.12.0.rc1.52.ge239d7e709.dirty

[PATCH 15/18] read-cache, remove_marked_cache_entries: wipe selected submodules.

[Stefan Beller]

Signed-off-by: Stefan Beller 
---
 read-cache.c | 27 +--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 9054369dd0..9a2abacf7a 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -18,6 +18,8 @@
 #include "varint.h"
 #include "split-index.h"
 #include "utf8.h"
+#include "submodule.h"
+#include "submodule-config.h"
 
 /* Mask for the name length in ce_flags in the on-disk index */
 
@@ -520,6 +522,22 @@ int remove_index_entry_at(struct index_state *istate, int 
pos)
return 1;
 }
 
+static void remove_submodule_according_to_strategy(const struct submodule *sub)
+{
+   switch (sub->update_strategy.type) {
+   case SM_UPDATE_UNSPECIFIED:
+   case SM_UPDATE_CHECKOUT:
+   case SM_UPDATE_REBASE:
+   case SM_UPDATE_MERGE:
+   submodule_move_head(sub->path, "HEAD", NULL, \
+   SUBMODULE_MOVE_HEAD_FORCE);
+   break;
+   case SM_UPDATE_NONE:
+   case SM_UPDATE_COMMAND:
+   ; /* Do not touch the submodule. */
+   }
+}
+
 /*
  * Remove all cache entries marked for removal, that is where
  * CE_REMOVE is set in ce_flags.  This is much more effective than
@@ -532,8 +550,13 @@ void remove_marked_cache_entries(struct index_state 
*istate)
 
for (i = j = 0; i < istate->cache_nr; i++) {
if (ce_array[i]->ce_flags & CE_REMOVE) {
-   remove_name_hash(istate, ce_array[i]);
-   save_or_free_index_entry(istate, ce_array[i]);
+   const struct submodule *sub = 
submodule_from_ce(ce_array[i]);
+   if (sub) {
+   remove_submodule_according_to_strategy(sub);
+   } else {
+   remove_name_hash(istate, ce_array[i]);
+   save_or_free_index_entry(istate, ce_array[i]);
+   }
}
else
ce_array[j++] = ce_array[i];
-- 
2.12.0.rc1.52.ge239d7e709.dirty