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

[Prathamesh Chavan <pc44800@gmail.com>]

On Mon, Aug 21, 2017 at 10:17 PM, Heiko Voigt <hvoigt@hvoigt.net> 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 <christian.couder@gmail.com>
>> Mentored-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
>> ---
>>  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