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

From: Brandon Williams <bmwill@google.com>
Date: Nov 16, 2016, 00:14 UTC
Message-ID: <20161116001402.GH66382@google.com>
In-Reply-To: <20161115230651.23953-12-sbeller@google.com>

On 11/15, Stefan Beller wrote:

Show 38 quoted lines

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

Show 23 quoted lines

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