Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C

From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
Date: Aug 30, 2020, 19:58 UTC
Message-ID: <ce151a1408291bb0991ce89459e36ee13ccdfa52.camel@gmail.com>
In-Reply-To: <20200826091502.GA29471@konoha>

On Wed, 2020-08-26 at 14:45 +0530, Shourya Shukla wrote:

Show 18 quoted lines

> On 24/08 11:35, Junio C Hamano wrote:
> > Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> > 
> > The shell version would error out with anything in the index, so I'd
> > expect that a faithful conversion would not call is_directory() nor
> > submodule_from_path() at all---it would just look path up in the_index
> > and complains if anything is found.  For example, the quoted part in
> > the original above is what gives the error message when I do
> > 
> > 	$ git submodule add ./Makefile
> > 	'Makefile' already exists in the index.
> > 
> > I think.  And the above code won't trigger the "already exists" at
> > all because 'path' is not a directory.
> 
> Alright. That is correct. I tried to use a multitude of functions but
> did not find luck with any of them. The functions I tried:
>

It would've been nice to see the actual code you tried so that it's easier for others to more easily identify if you're using the wrong function or using the correct function in the wrong way.

Show 14 quoted lines

>     - index_path() to check if the path is in the index. For some
>       reason, it switched to the 'default' case and return the
>       'unsupported file type' error.
> 
>     - A combination of doing an OR with index_file_exists() and
>       index_dir_exists(). Still no luck. t7406.43 fails.
> 
>     - Using index_name_pos() along with the above two functions. Again a
>       failure in the same test.
> 
> I feel that index_name_pos() should suffice this task but it fails in
> t7406.43. The SM is in index since 'git ls-files --error-unmatch s1'
> does return 's1' (s1 is the submodule). What am I missing here?
>

You're likely missing the fact that you should call `read_cache` before using `index_name_pos` or the likes of it.

For instance, the following works without issues for most cases (more on that below):

        if (read_cache() < 0)
                die(_("index file corrupt"));

        cache_pos = cache_name_pos(path, strlen(path));
        if (cache_pos >= 0) {
                if (!force) {
                        die(_("'%s' already exists in the index"),
path);
                }
                else {
                        struct cache_entry *ce = the_index.cache[cache_pos];

                        if (!S_ISGITLINK(ce->ce_mode))
                                die(_("'%s' already exists in the index and is not a "
                                      "submodule"), path);
                }
        }

This is more close to what the shell version did but misses one case which might or might not be covered by the test suite[1]. The case when path is a directory that has tracked contents. In the shell version we would get:

   $ git submodule add ../git-crypt/ builtin
   'builtin' already exists in the index
   $ git submodule add --force ../git-crypt/ builtin
   'builtin' already exists in the index and is not a submodule

   In the C version with the above snippet we get:

   $ git submodule add --force ../git-crypt/ builtin
   fatal: 'builtin' does not have a commit checked out
   $ git submodule add ../git-crypt/ builtin
   fatal: 'builtin' does not have a commit checked out

   That's not appropriate and should be fixed. I believe we could do
   something with `cache_dir_exists` to fix this.

   Footnote
   ===

   [1]: If it's not covered already, it might be a good idea to add a test
   for the above case.

   --
   Sivaraam