git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Shourya Shukla <periperidip@gmail.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, Johannes.Schindelin@gmx.de,
	liu.denton@gmail.com, christian.couder@gmail.com,
	kaartic.sivaraam@gmail.com,
	Shourya Shukla <periperidip@gmail.com>
Subject: [PATCH v3 0/3] submodule: port subcommand add from shell to C
Date: Tue, 15 Dec 2020 04:49:36 +0530	[thread overview]
Message-ID: <20201214231939.644175-1-periperidip@gmail.com> (raw)

Greetings,

This is the v3 of the patch series with the same title. You may view
the v2 here:
https://lore.kernel.org/git/20201007074538.25891-1-shouryashukla.oo@gmail.com/

I have applied almost all of the changes asked before, except a few
which confused me a little. It would be great if I could get some help
about them:

    1. In this mail: https://lore.kernel.org/git/xmqqo8ldznjx.fsf@gitster.c.googlers.com/
       Junio asked me to accomodate for a merge in progress since
       'cache_pos < 0' does not necessarily mean that the path exists.
       I was wondering which function would be the most appropriate for
       the if-statements:
            if (!force) {
		        if (cache_pos >= 0 || dir_in_cache)
            }
       I was thinking of going with 'read_cache_unmerged()'. I thought
       of this by seeing what is triggered in case of a merge conflict
       and cam across this. What is your opinion on this?

    2. In this mail: https://lore.kernel.org/git/xmqqimbky6st.fsf@gitster.c.googlers.com/
       In this section:
            /* strip trailing '/' */
	        if (is_dir_sep(sm_path[strlen(sm_path) -1]))
		        sm_path[strlen(sm_path) - 1] = '\0';

       Junio makes a reasonable argument that we need to make sure that
       multiple trailing slashes are eliminated but my code only takes
       care of a single trailing slash. I was looking into the code of
       'normalize_path_copy()' and saw that the function it essentially
       calls: 'normalize_path_copy_len()' does not perform anything on
       the trailing slashes and this behaviour is mentioned as a
       NEEDSWORK.

       I was thinking of correcting the above function instead of
       putting in an extra loop. Is this feasible?

    3. In the following segment:
        /*
         * NEEDSWORK: In a multi-working-tree world, this needs to be
         * set in the per-worktree config.
         */
        if (!git_config_get_string("submodule.active", &var) && var) {

        There was a comment: "What if this were a valueless true
        ("[submodule] active\n" without "= true")?  Wouldn't get_string()
        fail?"

        I was under the impression that even if the above failed, it
        will not really affect the big picture since at the we will set
        'submodule.name.active" as true irrespective of the above value.
        Is this correct?

Feedback and reviews are appreciated.

Regards,
Shourya Shukla

Shourya Shukla (3):
  dir: change the scope of function 'directory_exists_in_index()'
  submodule: port submodule subcommand 'add' from shell to C
  t7400: add test to check 'submodule add' for tracked paths

 builtin/submodule--helper.c | 410 +++++++++++++++++++++++++++++++++++-
 dir.c                       |  30 ++-
 dir.h                       |   9 +
 git-submodule.sh            | 161 +-------------
 t/t7400-submodule-basic.sh  |  13 +-
 5 files changed, 443 insertions(+), 180 deletions(-)

-- 
2.25.1


             reply	other threads:[~2020-12-14 23:20 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-14 23:19 Shourya Shukla [this message]
2020-12-14 23:19 ` [PATCH v3 1/3] dir: change the scope of function 'directory_exists_in_index()' Shourya Shukla
2020-12-19  0:08   ` Johannes Schindelin
2020-12-19  0:47     ` Junio C Hamano
2020-12-14 23:19 ` [PATCH v3 2/3] submodule: port submodule subcommand 'add' from shell to C Shourya Shukla
2020-12-14 23:19 ` [PATCH v3 3/3] t7400: add test to check 'submodule add' for tracked paths Shourya Shukla
2020-12-15 21:44 ` [PATCH v3 0/3] submodule: port subcommand add from shell to C Junio C Hamano
2020-12-17 14:16   ` Shourya Shukla
2020-12-17 22:20     ` Junio C Hamano
2020-12-22 23:42 ` Junio C Hamano

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=20201214231939.644175-1-periperidip@gmail.com \
    --to=periperidip@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=kaartic.sivaraam@gmail.com \
    --cc=liu.denton@gmail.com \
    --subject='Re: [PATCH v3 0/3] submodule: port 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