Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents

[Brandon Williams <bmwill@google.com>]

On 11/15, Stefan Beller wrote:
> Extend rmdir_or_warn() to remove the directories of those submodules which
> are scheduled for removal. Also teach verify_clean_submodule() to check
> that a submodule configured to be removed is not modified before scheduling
> it for removal.
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  unpack-trees.c | 6 ++----
>  wrapper.c      | 4 ++++
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/unpack-trees.c b/unpack-trees.c
> index ea6bdd2..576e1d5 100644
> --- a/unpack-trees.c
> +++ b/unpack-trees.c
> @@ -9,6 +9,7 @@
>  #include "refs.h"
>  #include "attr.h"
>  #include "split-index.h"
> +#include "submodule.h"
>  #include "dir.h"
>  
>  /*
> @@ -1361,15 +1362,12 @@ static void invalidate_ce_path(const struct cache_entry *ce,
>  /*
>   * Check that checking out ce->sha1 in subdir ce->name is not
>   * going to overwrite any working files.
> - *
> - * Currently, git does not checkout subprojects during a superproject
> - * checkout, so it is not going to overwrite anything.
>   */
>  static int verify_clean_submodule(const struct cache_entry *ce,
>  				  enum unpack_trees_error_types error_type,
>  				  struct unpack_trees_options *o)
>  {
> -	return 0;
> +	return submodule_is_interesting(ce->name, null_sha1) && is_submodule_modified(ce->name, 0);
>  }

So what does the return value from this function meant to mean? Is '1'
mean the submodule is clean while '0' indicates it is dirty or is it the
reverse of that?  Reading this it seems to me a value of '1' means "yes
the submodule is clean!" but the way the return value is calculated
tells a different story.  Either I'm understanding it incorrectly or I
think the return should be something like this:

  return submodule_is_interesting(ce->name, null_sha1) && !is_submodule_modified(ce->name, 0);

Where we return '1' if the submodule is interesting and it hasn't been
modified.

>  
>  static int verify_clean_subdirectory(const struct cache_entry *ce,
> diff --git a/wrapper.c b/wrapper.c
> index e7f1979..17c08de 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -2,6 +2,7 @@
>   * Various trivial helper wrappers around standard functions
>   */
>  #include "cache.h"
> +#include "submodule.h"
>  
>  static void do_nothing(size_t size)
>  {
> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)
>  
>  int rmdir_or_warn(const char *file)
>  {
> +	if (submodule_is_interesting(file, null_sha1)
> +	    && depopulate_submodule(file))
> +		return -1;
>  	return warn_if_unremovable("rmdir", file, rmdir(file));
>  }

It seems weird to me that rmdir is doing checks to see if the file being
removed is a submodule.  Shouldn't those checks have occurred before
calling rmdir?

-- 
Brandon Williams