git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Prathamesh Chavan <pc44800@gmail.com>
To: Stefan Beller <sbeller@google.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>, Christian Couder <christian.couder@gmail.com>, Brandon Williams <bmwill@google.com>
Subject: Re: [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to C
Date: Wed, 2 Aug 2017 02:44:57 +0530
Message-ID: <CAME+mvXsh53kLJ4se4uKY=SJcvSbHtEZQ6K2CgAPs=1wxUxk1A@mail.gmail.com> (raw)
In-Reply-To: <CAGZ79kaL48k=tCL-nnzFJBm4Qx2Q9uON=Cv82RhUr4ArG66rDA@mail.gmail.com>

On Tue, Aug 1, 2017 at 2:42 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Jul 31, 2017 at 1:56 PM, Prathamesh Chavan <pc44800@gmail.com> wrote:
>> This aims to make git-submodule 'status' a built-in. Hence, the function
>> cmd_status() is ported from shell to C. This is done by introducing
>> three functions: module_status(), submodule_status() and print_status().
>>
>> The function module_status() acts as the front-end of the subcommand.
>> It parses subcommand's options and then calls the function
>> module_list_compute() for computing the list of submodules. Then
>> this functions calls for_each_submodule_list() looping through the
>> list obtained.
>>
>> Then for_each_submodule_list() calls submodule_status() for each of the
>> submodule in its list. The function submodule_status() is responsible
>> for generating the status each submodule it is called for, and
>> then calls print_status().
>>
>> Finally, the function print_status() handles the printing of submodule's
>> status.
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
>> ---
>> In this new version, the following changes have been made:
>> * parameters passed to the function print_status() have been changed.
>>   Instead of passing char *sub_sha1, instead the object_id is being passed.
>>
>> * Also, since the passed parameter displaypath's value isn't changed
>>   by the function, it is passed to the funcition as const char *displaypath
>>   instead of char *displaypath.
>>
>> * the output type of the function handle_submodule_head_ref() is changed
>>   from strbuf to object_id, as we will use the object_id instead of the
>>   hex of sha1 being stored in a struct strbuf.
>>
>> * diff_files_args is cleared after using it by passing it as args in the
>>   function cmd_diff_files.
>>
>> * In the function status_submodule(), for checking if a submodule has merge
>>   conflicts, the patch currently checks if the value of any of the ce_flags
>>   is non-zero. Currently, I think the we aren't interested in a partiular flag,
>>   but I'm not sure on this.
>>
>> * Debugging leftovers and suprious new-lines are removed.
>>
>> * The confusion with displaypath being passed as te super-prefix in many
>>   of the ported subcommands may be a result of the fact that the
>>   function generating the displaypath: get_submodule_displaypath()
>>   uses the super-prefix as simply a path concatenated with the current
>>   submodule name to denote our current location.
>>   The function get_super_prefix() is declared in cache.h and defined in
>>   environment.c, but is majorly used in the builtin/submodule--helper.c
>>   and also in unpack-trees.c
>>   Also, for generating any submodule's displaypath, it would be important to
>>   have ".." passed to the submodule, and currently it is possible only via the
>>   super-prefix.
>>   This is also other instaces where the super-prefix contained ".." as well.
>>   One of such instance is Test 4 from t7406-submodule-update.sh
>>   Hence, maybe documenting the value of displaypath might a solution
>>   for the above problem.
>>   I'm just stating my views and would like to recieve your opinion on this
>>   matter.
>
> Yes, I agree that the display path is not quite easily understood as it can be
> ambiguous.  I am confused by this paragraph:
> * does test 4 from 7406 fail here, or was it just the starting point
>   of the discussion and it all works fine?

Sorry for misleading there. In the discussion earlier, Brandon mentioned that
displaypath's value may contain ".." as well.
To this, I replied pointing out one of the test which checks for the value of
displaypath.
In this test, ( Test 4 from t7406-submodule-update.sh), the value of displaypath
is being calculated via the submodule_init() function present in
submodule--helper.c
Here, I wanted to point out that, even in the case of this function,
the variable
displaypath is evaluated after receiving the value of "../super/" as
the value returned
by the function get_super_prefix().
Hence, to correct this confusion about the usage of super-prefix,
(which as pointed out by Brandon has been traditionally defined
as the path from the root of the superproject down to the root of
the submodule which further means that there should not ever be
any relative '..'s.), we may either have to get the value of super-prefix's
Documentation to accomodate this change, and to accept the way it is
used in submodule--helper and the current patch series, or else,
change the way it is being used in the various function, and used
another method for evaluation of displaypath.

Thanks,
Prathamesh Chavan

  reply index

Thread overview: 28+ messages in thread (expand / mbox.gz / Atom feed / [top])
2017-07-24 20:34 [GSoC][PATCH 00/13] Update: Week 10 Prathamesh Chavan
2017-07-24 20:34 ` [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to C Prathamesh Chavan
2017-07-24 21:30   ` Brandon Williams
2017-07-31 20:56 [GSoC][PATCH 00/13] Update: Week-11 Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath() Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list() Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to C Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' " Prathamesh Chavan
2017-07-31 21:12   ` Stefan Beller
2017-08-01 21:14     ` Prathamesh Chavan [this message]
2017-07-31 20:56 ` [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' " Prathamesh Chavan
2017-07-31 21:19   ` Stefan Beller
2017-07-31 20:56 ` [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' " Prathamesh Chavan
2017-07-31 21:42   ` Stefan Beller
2017-08-01 21:19     ` Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 07/13] diff: change scope of the function count_lines() Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to C Prathamesh Chavan
2017-07-31 22:15   ` Stefan Beller
2017-07-31 23:27   ` Christian Couder
2017-08-05 10:28     ` Prathamesh Chavan
2017-08-05 16:55       ` Christian Couder
2017-08-05 18:03         ` Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectory Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path' Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentation Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath' Prathamesh Chavan
2017-07-31 20:56 ` [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to C Prathamesh Chavan
2017-07-31 22:20   ` Stefan Beller
2017-08-07 21:18 [GSoC][PATCH 00/13] Update: Week-12 Prathamesh Chavan
2017-08-07 21:18 ` [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to C Prathamesh Chavan

Reply instructions:

You may reply publically 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 to all the recipients using the --to, --cc,
  and --in-reply-to switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAME+mvXsh53kLJ4se4uKY=SJcvSbHtEZQ6K2CgAPs=1wxUxk1A@mail.gmail.com' \
    --to=pc44800@gmail.com \
    --cc=bmwill@google.com \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=sbeller@google.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

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox