Re: [GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-18 Thread Junio C Hamano 
On Tue, Jul 18, 2017 at 3:44 PM, Stefan Beller  wrote:
> On Tue, Jul 18, 2017 at 3:32 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
 +   if (!lstat(list_item->name, ) && !ce_match_stat(list_item, , 
 0)) {
 +   print_status(info, ' ', list_item->name, sub_sha1, 
 displaypath);
>>>
>>> The question from the last round still stands
>>> https://public-inbox.org/git/cagz79kb18z5zc9iu3vv5avzwjmozzmwbmvpy89vc-t-ei2m...@mail.gmail.com/
>>>
>>>   I am not an expert in the diff area  and wonder how
>>>   the cmd_diff_files functionality is achieved with just a stat call
>>>   and then comparing it to  ce_match_stat. 'Using "dirty" ignores
>>>   all changes to the work tree of submodules, only changes to the
>>>   commits stored in the superproject are shown.' So I'd have
>>>   expected ce->oid to be compared (is there an index entry differing,
>>>   i.e. more than one stage?)
>>
>> ce_match_stat() calls into ce_compare_gitlink() for a 16 entry,
>> which would resolve HEAD ref there and compares ce->oid with it.
>
> Oh in that case this should be fine, as in the original we did
> "git diff-files --ignore-submodules=dirty ",
> which did precisely that.

Are you sure that this will work correctly when ce is unmerged?
run_diff_files() has quite a lot of code for that case.

Re: [GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-18 Thread Stefan Beller 
On Tue, Jul 18, 2017 at 3:32 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>>> +   if (!lstat(list_item->name, ) && !ce_match_stat(list_item, , 
>>> 0)) {
>>> +   print_status(info, ' ', list_item->name, sub_sha1, 
>>> displaypath);
>>
>> The question from the last round still stands
>> https://public-inbox.org/git/cagz79kb18z5zc9iu3vv5avzwjmozzmwbmvpy89vc-t-ei2m...@mail.gmail.com/
>>
>>   I am not an expert in the diff area  and wonder how
>>   the cmd_diff_files functionality is achieved with just a stat call
>>   and then comparing it to  ce_match_stat. 'Using "dirty" ignores
>>   all changes to the work tree of submodules, only changes to the
>>   commits stored in the superproject are shown.' So I'd have
>>   expected ce->oid to be compared (is there an index entry differing,
>>   i.e. more than one stage?)
>
> ce_match_stat() calls into ce_compare_gitlink() for a 16 entry,
> which would resolve HEAD ref there and compares ce->oid with it.

Oh in that case this should be fine, as in the original we did
"git diff-files --ignore-submodules=dirty ",
which did precisely that.

> But as you said, this is probably insufficient to emulate the
> original.  Shouldn't it call into run_diff_files(), which is the
> in-core way to run the equivalent of "diff-files"?

Oh, your comment in [1] was related to cmd_diff_files,
which is more complicated than run_diff_files?
run_diff_files also iterates over all cache entries.
I think we need to be looking at match_stat_with_submodule
to figure out what we need to do.

[1] https://public-inbox.org/git/xmqq60fdoyyt@gitster.mtv.corp.google.com/

Re: [GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-18 Thread Junio C Hamano 
Stefan Beller  writes:

>> +   if (!lstat(list_item->name, ) && !ce_match_stat(list_item, , 
>> 0)) {
>> +   print_status(info, ' ', list_item->name, sub_sha1, 
>> displaypath);
>
> The question from the last round still stands
> https://public-inbox.org/git/cagz79kb18z5zc9iu3vv5avzwjmozzmwbmvpy89vc-t-ei2m...@mail.gmail.com/
>
>   I am not an expert in the diff area  and wonder how
>   the cmd_diff_files functionality is achieved with just a stat call
>   and then comparing it to  ce_match_stat. 'Using "dirty" ignores
>   all changes to the work tree of submodules, only changes to the
>   commits stored in the superproject are shown.' So I'd have
>   expected ce->oid to be compared (is there an index entry differing,
>   i.e. more than one stage?)

ce_match_stat() calls into ce_compare_gitlink() for a 16 entry,
which would resolve HEAD ref there and compares ce->oid with it.

But as you said, this is probably insufficient to emulate the
original.  Shouldn't it call into run_diff_files(), which is the
in-core way to run the equivalent of "diff-files"?

Re: [GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-18 Thread Stefan Beller 
On Tue, Jul 18, 2017 at 1:49 PM, Prathamesh Chavan  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 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 146 
> 
>  git-submodule.sh|  49 +--
>  2 files changed, 147 insertions(+), 48 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 80f744407..9c1630495 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -560,6 +560,151 @@ static int module_init(int argc, const char **argv, 
> const char *prefix)
> return 0;
>  }
>
> +struct status_cb {
> +   const char *prefix;
> +   unsigned int quiet: 1;
> +   unsigned int recursive: 1;
> +   unsigned int cached: 1;
> +};
> +#define STATUS_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void print_status(struct status_cb *info, char state, const char 
> *path,
> +char *sub_sha1, char *displaypath)
> +{
> +   if (info->quiet)
> +   return;
> +
> +   printf("%c%s %s", state, sub_sha1, displaypath);
> +
> +   if (state == ' ' || state == '+') {
> +   struct argv_array name_rev_args = ARGV_ARRAY_INIT;
> +
> +   argv_array_pushl(_rev_args, "print-name-rev",
> +path, sub_sha1, NULL);
> +   print_name_rev(name_rev_args.argc, name_rev_args.argv,
> +  info->prefix);
> +   } else {
> +   printf("\n");
> +   }
> +}
> +
> +static int handle_submodule_head_ref(const char *refname,
> +const struct object_id *oid, int flags,
> +void *cb_data)
> +{
> +   struct strbuf *output = cb_data;
> +   if (oid)
> +   strbuf_addstr(output, oid_to_hex(oid));
> +   return 0;
> +}
> +
> +static void status_submodule(const struct cache_entry *list_item, void 
> *cb_data)
> +{
> +   struct status_cb *info = cb_data;
> +   char *sub_sha1 = xstrdup(oid_to_hex(_item->oid));
> +   char *displaypath;
> +   struct stat st;
> +
> +   if (!submodule_from_path(null_sha1, list_item->name))
> +   die(_("no submodule mapping found in .gitmodules for path 
> '%s'"),
> + list_item->name);
> +
> +   displaypath = get_submodule_displaypath(list_item->name, 
> info->prefix);
> +
> +   if (list_item->ce_flags) {
> +   print_status(info, 'U', list_item->name,
> +sha1_to_hex(null_sha1), displaypath);
> +   goto cleanup;
> +   }
> +
> +   if (!is_submodule_active(the_repository, list_item->name)) {
> +   print_status(info, '-', list_item->name, sub_sha1, 
> displaypath);
> +   goto cleanup;
> +   }
> +
> +   if (!lstat(list_item->name, ) && !ce_match_stat(list_item, , 
> 0)) {
> +   print_status(info, ' ', list_item->name, sub_sha1, 
> displaypath);

The question from the last round still stands
https://public-inbox.org/git/cagz79kb18z5zc9iu3vv5avzwjmozzmwbmvpy89vc-t-ei2m...@mail.gmail.com/

  I am not an expert in the diff area  and wonder how
  the cmd_diff_files functionality is achieved with just a stat call
  and then comparing it to  ce_match_stat. 'Using "dirty" ignores
  all changes to the work tree of submodules, only changes to the
  commits stored in the superproject are shown.' So I'd have
  expected ce->oid to be compared (is there an index entry differing,
  i.e. more than one stage?)

[GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-18 Thread Prathamesh Chavan 
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 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 146 
 git-submodule.sh|  49 +--
 2 files changed, 147 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 80f744407..9c1630495 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -560,6 +560,151 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct status_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+   unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+char *sub_sha1, char *displaypath)
+{
+   if (info->quiet)
+   return;
+
+   printf("%c%s %s", state, sub_sha1, displaypath);
+
+   if (state == ' ' || state == '+') {
+   struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+   argv_array_pushl(_rev_args, "print-name-rev",
+path, sub_sha1, NULL);
+   print_name_rev(name_rev_args.argc, name_rev_args.argv,
+  info->prefix);
+   } else {
+   printf("\n");
+   }
+}
+
+static int handle_submodule_head_ref(const char *refname,
+const struct object_id *oid, int flags,
+void *cb_data)
+{
+   struct strbuf *output = cb_data;
+   if (oid)
+   strbuf_addstr(output, oid_to_hex(oid));
+   return 0;
+}
+
+static void status_submodule(const struct cache_entry *list_item, void 
*cb_data)
+{
+   struct status_cb *info = cb_data;
+   char *sub_sha1 = xstrdup(oid_to_hex(_item->oid));
+   char *displaypath;
+   struct stat st;
+
+   if (!submodule_from_path(null_sha1, list_item->name))
+   die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
+ list_item->name);
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (list_item->ce_flags) {
+   print_status(info, 'U', list_item->name,
+sha1_to_hex(null_sha1), displaypath);
+   goto cleanup;
+   }
+
+   if (!is_submodule_active(the_repository, list_item->name)) {
+   print_status(info, '-', list_item->name, sub_sha1, displaypath);
+   goto cleanup;
+   }
+
+   if (!lstat(list_item->name, ) && !ce_match_stat(list_item, , 0)) {
+   print_status(info, ' ', list_item->name, sub_sha1, displaypath);
+   } else {
+   if (!info->cached) {
+   struct strbuf sb = STRBUF_INIT;
+   if (head_ref_submodule(list_item->name,
+  handle_submodule_head_ref, ))
+   die(_("could not resolve HEAD ref inside the"
+ "submodule '%s'"), list_item->name);
+   print_status(info, '+', list_item->name, sb.buf,
+displaypath);
+   strbuf_release();
+   } else {
+   print_status(info, '+', list_item->name, sub_sha1,
+displaypath);
+   }
+   }
+
+   if (info->recursive) {
+   struct child_process cpr = CHILD_PROCESS_INIT;
+
+   cpr.git_cmd = 1;
+   cpr.dir = list_item->name;
+   prepare_submodule_repo_env(_array);
+
+   argv_array_pushl(, "--super-prefix", displaypath,
+"submodule--helper", "status", "--recursive",
+NULL);
+
+   if (info->cached)
+   argv_array_push(, "--cached");
+
+   if (info->quiet)
+

Re: [GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-10 Thread Brandon Williams 
On 07/11, Prathamesh Chavan 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 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 154 
> 
>  git-submodule.sh|  49 +-
>  2 files changed, 155 insertions(+), 48 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 80f744407..980b8ed27 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -560,6 +560,159 @@ static int module_init(int argc, const char **argv, 
> const char *prefix)
>   return 0;
>  }
>  
> +struct status_cb {
> + const char *prefix;
> + unsigned int quiet: 1;
> + unsigned int recursive: 1;
> + unsigned int cached: 1;
> +};
> +#define STATUS_CB_INIT { NULL, 0, 0, 0 }
> +
> +static void print_status(struct status_cb *info, char state, const char 
> *path,
> +  char *sub_sha1, char *displaypath)
> +{
> + if (info->quiet)
> + return;
> +
> + printf("%c%s %s", state, sub_sha1, displaypath);
> +
> + if (state == ' ' || state == '+') {
> + struct argv_array name_rev_args = ARGV_ARRAY_INIT;

This struct needs to be cleared to prevent a memory leak.

> +
> + argv_array_pushl(_rev_args, "print-name-rev",
> +  path, sub_sha1, NULL);
> + print_name_rev(name_rev_args.argc, name_rev_args.argv,
> +info->prefix);
> + } else {
> + printf("\n");
> + }
> +}
> +
> +static void status_submodule(const struct cache_entry *list_item, void 
> *cb_data)
> +{
> + struct status_cb *info = cb_data;
> + char *sub_sha1 = xstrdup(oid_to_hex(_item->oid));
> + char *displaypath;
> + struct argv_array diff_files_args = ARGV_ARRAY_INIT;
> +
> + if (!submodule_from_path(null_sha1, list_item->name))
> + die(_("no submodule mapping found in .gitmodules for path 
> '%s'"),
> +   list_item->name);
> +
> + displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> + if (list_item->ce_flags) {
> + print_status(info, 'U', list_item->name,
> +  sha1_to_hex(null_sha1), displaypath);
> + goto cleanup;
> + }
> +
> + if (!is_submodule_active(the_repository, list_item->name)) {
> + print_status(info, '-', list_item->name, sub_sha1, displaypath);
> + goto cleanup;
> + }
> +
> + argv_array_pushl(_files_args, "diff-files",
> +  "--ignore-submodules=dirty", "--quiet", "--",
> +  list_item->name, NULL);
> +
> + /* NEEDSWORK: future optimization possible */

What sort of optimization? maybe you could document that?

> + if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
> + info->prefix)) {
> + print_status(info, ' ', list_item->name, sub_sha1, displaypath);
> + } else {
> + if (!info->cached) {
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> +
> + prepare_submodule_repo_env(_array);
> + cp.git_cmd = 1;
> + cp.dir = list_item->name;
> +
> + argv_array_pushl(, "rev-parse",
> +  "--verify", "HEAD", NULL);
> +
> + /* NEEDSWORK: future optimization possible */

Same here.

> + if (capture_command(, , 0))
> + die(_("could not run 'git rev-parse --verify"
> +   "HEAD' in submodule %s"),
> +   list_item->name);
> +
> + strbuf_strip_suffix(, "\n");
> + print_status(info, '+', list_item->name, sb.buf,
> +  displaypath);
> + strbuf_release();
> + } else {
> +

[GSoC][PATCH 4/8] submodule: port submodule subcommand 'status' from shell to C

2017-07-10 Thread Prathamesh Chavan 
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 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 154 
 git-submodule.sh|  49 +-
 2 files changed, 155 insertions(+), 48 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 80f744407..980b8ed27 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -560,6 +560,159 @@ static int module_init(int argc, const char **argv, const 
char *prefix)
return 0;
 }
 
+struct status_cb {
+   const char *prefix;
+   unsigned int quiet: 1;
+   unsigned int recursive: 1;
+   unsigned int cached: 1;
+};
+#define STATUS_CB_INIT { NULL, 0, 0, 0 }
+
+static void print_status(struct status_cb *info, char state, const char *path,
+char *sub_sha1, char *displaypath)
+{
+   if (info->quiet)
+   return;
+
+   printf("%c%s %s", state, sub_sha1, displaypath);
+
+   if (state == ' ' || state == '+') {
+   struct argv_array name_rev_args = ARGV_ARRAY_INIT;
+
+   argv_array_pushl(_rev_args, "print-name-rev",
+path, sub_sha1, NULL);
+   print_name_rev(name_rev_args.argc, name_rev_args.argv,
+  info->prefix);
+   } else {
+   printf("\n");
+   }
+}
+
+static void status_submodule(const struct cache_entry *list_item, void 
*cb_data)
+{
+   struct status_cb *info = cb_data;
+   char *sub_sha1 = xstrdup(oid_to_hex(_item->oid));
+   char *displaypath;
+   struct argv_array diff_files_args = ARGV_ARRAY_INIT;
+
+   if (!submodule_from_path(null_sha1, list_item->name))
+   die(_("no submodule mapping found in .gitmodules for path 
'%s'"),
+ list_item->name);
+
+   displaypath = get_submodule_displaypath(list_item->name, info->prefix);
+
+   if (list_item->ce_flags) {
+   print_status(info, 'U', list_item->name,
+sha1_to_hex(null_sha1), displaypath);
+   goto cleanup;
+   }
+
+   if (!is_submodule_active(the_repository, list_item->name)) {
+   print_status(info, '-', list_item->name, sub_sha1, displaypath);
+   goto cleanup;
+   }
+
+   argv_array_pushl(_files_args, "diff-files",
+"--ignore-submodules=dirty", "--quiet", "--",
+list_item->name, NULL);
+
+   /* NEEDSWORK: future optimization possible */
+   if (!cmd_diff_files(diff_files_args.argc, diff_files_args.argv,
+   info->prefix)) {
+   print_status(info, ' ', list_item->name, sub_sha1, displaypath);
+   } else {
+   if (!info->cached) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   struct strbuf sb = STRBUF_INIT;
+
+   prepare_submodule_repo_env(_array);
+   cp.git_cmd = 1;
+   cp.dir = list_item->name;
+
+   argv_array_pushl(, "rev-parse",
+"--verify", "HEAD", NULL);
+
+   /* NEEDSWORK: future optimization possible */
+   if (capture_command(, , 0))
+   die(_("could not run 'git rev-parse --verify"
+ "HEAD' in submodule %s"),
+ list_item->name);
+
+   strbuf_strip_suffix(, "\n");
+   print_status(info, '+', list_item->name, sb.buf,
+displaypath);
+   strbuf_release();
+   } else {
+   print_status(info, '+', list_item->name, sub_sha1,
+displaypath);
+   }
+   }
+
+   if (info->recursive) {
+   struct child_process cpr = CHILD_PROCESS_INIT;
+
+   cpr.git_cmd = 1;
+