Re: [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C

2017-08-21 Thread Prathamesh Chavan
On Mon, Aug 21, 2017 at 10:17 PM, Heiko Voigt  wrote:
> On Mon, Aug 21, 2017 at 09:45:14PM +0530, Prathamesh Chavan wrote:
>> Function set_name_rev() is ported from git-submodule to the
>> submodule--helper builtin. The function get_name_rev() generates the
>> value of the revision name as required, and the function
>> print_name_rev() handles the formating and printing of the obtained
>> revision name.
>>
>> Mentored-by: Christian Couder 
>> Mentored-by: Stefan Beller 
>> Signed-off-by: Prathamesh Chavan 
>> ---
>>  builtin/submodule--helper.c | 63 
>> +
>>  git-submodule.sh| 16 ++--
>>  2 files changed, 65 insertions(+), 14 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 7803457ba..a4bff3f38 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>
> [...]
>
>> @@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {
>>   {"relative-path", resolve_relative_path, 0},
>>   {"resolve-relative-url", resolve_relative_url, 0},
>>   {"resolve-relative-url-test", resolve_relative_url_test, 0},
>> + {"print-name-rev", print_name_rev, 0},
>
> I see the function in git-submodule.sh was named kind of reverse. How
> about we do it more naturally here and call this 'rev-name' instead?
> That makes is more clear to me and is also similar to the used variable
> name 'revname'.

The functions 'print_name_rev()' and 'get_name_rev()' are the ported
C functions of the function 'set_name_rev()'
Their names were assigned so due to the existing function's name,
and hence to faithfully port the functions.

But, thanks for the above suggestion, and also for reviewing
the patch. I will update the names as print_rev_name() and
get_rev_name() respectively.
>
> I would also prefix it differently like 'get' or 'calculate' instead of
> 'print' since it tries to find a name and is not just a simple lookup.

The former function from the shell script, 'set_name_rev()' is split
into two functions, namely: 'print_name_rev()' and 'get_name_rev()'
The function print_name_rev() is just the front_end of the function,
and exists to printf the return value of the get_name_rev() function
is the required format.
Calculation of the value is actually done by the function
get_name_rev().
Hence, I named the functions the way they are.

Thanks,
Prathamesh Chavan


Re: [GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C

2017-08-21 Thread Heiko Voigt
On Mon, Aug 21, 2017 at 09:45:14PM +0530, Prathamesh Chavan wrote:
> Function set_name_rev() is ported from git-submodule to the
> submodule--helper builtin. The function get_name_rev() generates the
> value of the revision name as required, and the function
> print_name_rev() handles the formating and printing of the obtained
> revision name.
> 
> Mentored-by: Christian Couder 
> Mentored-by: Stefan Beller 
> Signed-off-by: Prathamesh Chavan 
> ---
>  builtin/submodule--helper.c | 63 
> +
>  git-submodule.sh| 16 ++--
>  2 files changed, 65 insertions(+), 14 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 7803457ba..a4bff3f38 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c

[...]

> @@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {
>   {"relative-path", resolve_relative_path, 0},
>   {"resolve-relative-url", resolve_relative_url, 0},
>   {"resolve-relative-url-test", resolve_relative_url_test, 0},
> + {"print-name-rev", print_name_rev, 0},

I see the function in git-submodule.sh was named kind of reverse. How
about we do it more naturally here and call this 'rev-name' instead?
That makes is more clear to me and is also similar to the used variable
name 'revname'.

I would also prefix it differently like 'get' or 'calculate' instead of
'print' since it tries to find a name and is not just a simple lookup.

So in summary I would prefer something like 'get-rev-name' as a name for
the subcommand here.

>   {"init", module_init, SUPPORT_SUPER_PREFIX},
>   {"remote-branch", resolve_remote_submodule_branch, 0},
>   {"push-check", push_check, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index e131760ee..e988167e0 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -759,18 +759,6 @@ cmd_update()
>   }
>  }
>  
> -set_name_rev () {
> - revname=$( (
> - sanitize_submodule_env
> - cd "$1" && {
> - git describe "$2" 2>/dev/null ||
> - git describe --tags "$2" 2>/dev/null ||
> - git describe --contains "$2" 2>/dev/null ||
> - git describe --all --always "$2"
> - }
> - ) )
> - test -z "$revname" || revname=" ($revname)"
> -}
>  #
>  # Show commit summary for submodules in index or working tree
>  #
> @@ -1042,14 +1030,14 @@ cmd_status()
>   fi
>   if git diff-files --ignore-submodules=dirty --quiet -- 
> "$sm_path"
>   then
> - set_name_rev "$sm_path" "$sha1"
> + revname=$(git submodule--helper print-name-rev 
> "$sm_path" "$sha1")
>   say " $sha1 $displaypath$revname"
>   else
>   if test -z "$cached"
>   then
>   sha1=$(sanitize_submodule_env; cd "$sm_path" && 
> git rev-parse --verify HEAD)
>   fi
> - set_name_rev "$sm_path" "$sha1"
> + revname=$(git submodule--helper print-name-rev 
> "$sm_path" "$sha1")
>   say "+$sha1 $displaypath$revname"
>   fi
>  
> -- 
> 2.13.0
> 


[GSoC][PATCH 3/4] submodule: port set_name_rev() from shell to C

2017-08-21 Thread Prathamesh Chavan
Function set_name_rev() is ported from git-submodule to the
submodule--helper builtin. The function get_name_rev() generates the
value of the revision name as required, and the function
print_name_rev() handles the formating and printing of the obtained
revision name.

Mentored-by: Christian Couder 
Mentored-by: Stefan Beller 
Signed-off-by: Prathamesh Chavan 
---
 builtin/submodule--helper.c | 63 +
 git-submodule.sh| 16 ++--
 2 files changed, 65 insertions(+), 14 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 7803457ba..a4bff3f38 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -244,6 +244,68 @@ static char *get_submodule_displaypath(const char *path, 
const char *prefix)
}
 }
 
+static char *get_name_rev(const char *sub_path, const char* object_id)
+{
+   struct strbuf sb = STRBUF_INIT;
+   const char ***d;
+
+   static const char *describe_bare[] = {
+   NULL
+   };
+
+   static const char *describe_tags[] = {
+   "--tags", NULL
+   };
+
+   static const char *describe_contains[] = {
+   "--contains", NULL
+   };
+
+   static const char *describe_all_always[] = {
+   "--all", "--always", NULL
+   };
+
+   static const char **describe_argv[] = {
+   describe_bare, describe_tags, describe_contains,
+   describe_all_always, NULL
+   };
+
+   for (d = describe_argv; *d; d++) {
+   struct child_process cp = CHILD_PROCESS_INIT;
+   prepare_submodule_repo_env(_array);
+   cp.dir = sub_path;
+   cp.git_cmd = 1;
+   cp.no_stderr = 1;
+
+   argv_array_push(, "describe");
+   argv_array_pushv(, *d);
+   argv_array_push(, object_id);
+
+   if (!capture_command(, , 0) && sb.len) {
+   strbuf_strip_suffix(, "\n");
+   return strbuf_detach(, NULL);
+   }
+   }
+
+   strbuf_release();
+   return NULL;
+}
+
+static int print_name_rev(int argc, const char **argv, const char *prefix)
+{
+   char *namerev;
+   if (argc != 3)
+   die("print-name-rev only accepts two arguments:  ");
+
+   namerev = get_name_rev(argv[1], argv[2]);
+   if (namerev && namerev[0])
+   printf(" (%s)", namerev);
+   printf("\n");
+
+   free(namerev);
+   return 0;
+}
+
 struct module_list {
const struct cache_entry **entries;
int alloc, nr;
@@ -1242,6 +1304,7 @@ static struct cmd_struct commands[] = {
{"relative-path", resolve_relative_path, 0},
{"resolve-relative-url", resolve_relative_url, 0},
{"resolve-relative-url-test", resolve_relative_url_test, 0},
+   {"print-name-rev", print_name_rev, 0},
{"init", module_init, SUPPORT_SUPER_PREFIX},
{"remote-branch", resolve_remote_submodule_branch, 0},
{"push-check", push_check, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index e131760ee..e988167e0 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -759,18 +759,6 @@ cmd_update()
}
 }
 
-set_name_rev () {
-   revname=$( (
-   sanitize_submodule_env
-   cd "$1" && {
-   git describe "$2" 2>/dev/null ||
-   git describe --tags "$2" 2>/dev/null ||
-   git describe --contains "$2" 2>/dev/null ||
-   git describe --all --always "$2"
-   }
-   ) )
-   test -z "$revname" || revname=" ($revname)"
-}
 #
 # Show commit summary for submodules in index or working tree
 #
@@ -1042,14 +1030,14 @@ cmd_status()
fi
if git diff-files --ignore-submodules=dirty --quiet -- 
"$sm_path"
then
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say " $sha1 $displaypath$revname"
else
if test -z "$cached"
then
sha1=$(sanitize_submodule_env; cd "$sm_path" && 
git rev-parse --verify HEAD)
fi
-   set_name_rev "$sm_path" "$sha1"
+   revname=$(git submodule--helper print-name-rev 
"$sm_path" "$sha1")
say "+$sha1 $displaypath$revname"
fi
 
-- 
2.13.0