git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kaartic Sivaraam <kaartic.sivaraam@gmail.com>
To: Shourya Shukla <shouryashukla.oo@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	git@vger.kernel.org, christian.couder@gmail.com,
	johannes.schindelin@gmx.de, liu.denton@gmail.com
Subject: Re: [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C
Date: Mon, 31 Aug 2020 01:28:53 +0530	[thread overview]
Message-ID: <ce151a1408291bb0991ce89459e36ee13ccdfa52.camel@gmail.com> (raw)
In-Reply-To: <20200826091502.GA29471@konoha>

On Wed, 2020-08-26 at 14:45 +0530, Shourya Shukla wrote:
> 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.

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



  reply	other threads:[~2020-08-30 19:59 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-24  9:03 Shourya Shukla
2020-08-24 18:35 ` Junio C Hamano
2020-08-24 20:30   ` Kaartic Sivaraam
2020-08-24 20:46     ` Junio C Hamano
2020-08-26  9:27     ` Shourya Shukla
2020-08-26 10:54       ` Kaartic Sivaraam
2020-08-26  9:15   ` Shourya Shukla
2020-08-30 19:58     ` Kaartic Sivaraam [this message]
2020-08-31 13:04       ` Shourya Shukla
2020-09-01 20:35         ` Kaartic Sivaraam
2020-09-02 12:04           ` Shourya Shukla
2020-09-03  8:46             ` Kaartic Sivaraam

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ce151a1408291bb0991ce89459e36ee13ccdfa52.camel@gmail.com \
    --to=kaartic.sivaraam@gmail.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmx.de \
    --cc=liu.denton@gmail.com \
    --cc=shouryashukla.oo@gmail.com \
    --subject='Re: [GSoC][PATCH] submodule: port submodule subcommand '\''add'\'' from shell to C' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox