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
next prev parent 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 [GSoC][PATCH] submodule: port submodule subcommand 'add' from shell to C 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
List information: http://vger.kernel.org/majordomo-info.html
* 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).