address review comments

Author: SandrineP

src/subcommand/tag_subcommand.cpp

@@ -27,73 +27,26 @@ void print_list_lines(const std::string& message, int num_lines)
         return;
     }
 
-    size_t pos = 0;
-	int num = num_lines - 1;   // TODO: check with git re. "- 1"
+    auto lines = split_input_at_newlines(message);
 
-	/** first line - headline */
-	size_t newline_pos = message.find('\n', pos);
-    if (newline_pos != std::string::npos)
-    {
-        std::cout << message.substr(pos, newline_pos - pos);
-        pos = newline_pos;
-    }
-    else
-    {
-        std::cout << message << std::endl;
-        return;
-    }
-
-	/** skip over new lines */
-    while (pos < message.length() && message[pos] == '\n')
-    {
-        pos++;
-    }
-
-    std::cout << std::endl;
+    // header
+    std::cout << lines[0];
 
-	/** print just headline? */
-	if (num == 0)
-	{
-	    return;
-	}
-    if (pos < message.length() && pos + 1 < message.length())
+    // other lines
+    if (num_lines <= 1 || lines.size() <= 2)
     {
         std::cout << std::endl;
     }
-
-	/** print individual commit/tag lines */
-	while (pos < message.length() && num >= 2)
+    else
     {
-        std::cout << "    ";
-
-        newline_pos = message.find('\n', pos);
-        if (newline_pos != std::string::npos)
+        for (size_t i = 1; i < lines.size() ; i++)
         {
-            std::cout << message.substr(pos, newline_pos - pos);
-            pos = newline_pos;
+            if (i < num_lines)
+            {
+                std::cout << "\n\t\t" << lines[i];
+            }
         }
-        else
-        {
-            std::cout << message.substr(pos);
-            break;
-        }
-
-        // Handle consecutive newlines
-        if (pos + 1 < message.length() &&
-            message[pos] == '\n' && message[pos + 1] == '\n')
-        {
-            num--;
-            std::cout << std::endl;
-        }
-
-        while (pos < message.length() && message[pos] == '\n')
-        {
-            pos++;
-        }
-
-        std::cout << std::endl;
-        num--;
-	}
+    }
 }
 
 // Tag listing: Print an actual tag object
@@ -172,9 +125,9 @@ void tag_subcommand::list_tags(repository_wrapper& repo)
     std::string pattern = m_tag_name.empty() ? "*" : m_tag_name;
     auto tag_names = repo.tag_list_match(pattern);
 
-    for (size_t i = 0; i < tag_names.size(); i++)
+    for (const auto& tag_name: tag_names)
     {
-        each_tag(repo, tag_names[i], m_num_lines);
+        each_tag(repo, tag_name, m_num_lines);
     }
 }
 
@@ -201,7 +154,7 @@ void tag_subcommand::delete_tag(repository_wrapper& repo)
     std::cout << "Deleted tag '" << m_delete << "' (was " << oid_str << ")" << std::endl;
 }
 
-void tag_subcommand::create_lightweight_tag(repository_wrapper& repo)
+std::optional<object_wrapper> tag_subcommand::get_target_obj(repository_wrapper& repo)
 {
     if (m_tag_name.empty())
     {
@@ -216,54 +169,43 @@ void tag_subcommand::create_lightweight_tag(repository_wrapper& repo)
         throw git_exception("Unable to resolve target: " + target, git2cpp_error_code::GENERIC_ERROR);
     }
 
-    git_oid oid;
-    size_t force = m_force_flag ? 1 : 0;
-    int error = git_tag_create_lightweight(&oid, repo, m_tag_name.c_str(), target_obj.value(), force);
+    return target_obj;
+}
 
+void tag_subcommand::handle_error(int error)
+{
     if (error < 0)
     {
         if (error == GIT_EEXISTS)
         {
             throw git_exception("tag '" + m_tag_name + "' already exists", git2cpp_error_code::FILESYSTEM_ERROR);
         }
-        throw git_exception("Unable to create lightweight tag", error);
+        throw git_exception("Unable to create annotated tag", error);
     }
 }
 
-void tag_subcommand::create_tag(repository_wrapper& repo)
+void tag_subcommand::create_lightweight_tag(repository_wrapper& repo)
 {
-    if (m_tag_name.empty())
-    {
-        throw git_exception("Tag name required", git2cpp_error_code::GENERIC_ERROR);
-    }
+    auto target_obj = tag_subcommand::get_target_obj(repo);
 
-    if (m_message.empty())
-    {
-        throw git_exception("Message required for annotated tag (use -m)", git2cpp_error_code::GENERIC_ERROR);
-    }
+    git_oid oid;
+    size_t force = m_force_flag ? 1 : 0;
+    int error = git_tag_create_lightweight(&oid, repo, m_tag_name.c_str(), target_obj.value(), force);
 
-    std::string target = m_target.empty() ? "HEAD" : m_target;
+    handle_error(error);
+}
 
-    auto target_obj = repo.revparse_single(target);
-    if (!target_obj.has_value())
-    {
-        throw git_exception("Unable to resolve target: " + target, git2cpp_error_code::GENERIC_ERROR);
-    }
+void tag_subcommand::create_tag(repository_wrapper& repo)
+{
+    auto target_obj = tag_subcommand::get_target_obj(repo);
 
     auto tagger = signature_wrapper::get_default_signature_from_env(repo);
 
     git_oid oid;
     size_t force = m_force_flag ? 1 : 0;
     int error = git_tag_create(&oid, repo, m_tag_name.c_str(), target_obj.value(), tagger.first, m_message.c_str(), force);
 
-    if (error < 0)
-    {
-        if (error == GIT_EEXISTS)
-        {
-            throw git_exception("tag '" + m_tag_name + "' already exists", git2cpp_error_code::FILESYSTEM_ERROR);
-        }
-        throw git_exception("Unable to create annotated tag", error);
-    }
+    handle_error(error);
 }
 
 void tag_subcommand::run()

src/subcommand/tag_subcommand.hpp

@@ -19,6 +19,8 @@ class tag_subcommand
     void delete_tag(repository_wrapper& repo);
     void create_lightweight_tag(repository_wrapper& repo);
     void create_tag(repository_wrapper& repo);
+    std::optional<object_wrapper> get_target_obj(repository_wrapper& repo);
+    void handle_error(int error);
 
     std::string m_delete;
     std::string m_message;

src/utils/common.cpp

@@ -4,6 +4,7 @@
 #include <sstream>
 #include <unistd.h>
 #include <map>
+#include <ranges>
 
 #include <git2.h>
 
@@ -121,3 +122,12 @@ std::string read_file(const std::string& path)
     buffer << file.rdbuf();
     return buffer.str();
 }
+
+std::vector<std::string> split_input_at_newlines(std::string_view str)
+{
+    auto split = str | std::ranges::views::split('\n')
+                     | std::ranges::views::transform([](auto&& range) {
+                         return std::string(range.begin(), range.end());
+                     });
+    return std::vector<std::string>{split.begin(), split.end()};
+}

src/utils/common.hpp

@@ -68,3 +68,5 @@ class git_strarray_wrapper
 };
 
 std::string read_file(const std::string& path);
+
+std::vector<std::string> split_input_at_newlines(std::string_view str);

src/utils/terminal_pager.cpp

@@ -12,6 +12,7 @@
 #include "ansi_code.hpp"
 #include "output.hpp"
 #include "terminal_pager.hpp"
+#include "common.hpp"
 
 terminal_pager::terminal_pager()
     : m_rows(0), m_columns(0), m_start_row_index(0)
@@ -167,7 +168,7 @@ void terminal_pager::show()
 {
     release_cout();
 
-    split_input_at_newlines(m_stringbuf.view());
+    m_lines = split_input_at_newlines(m_stringbuf.view());
 
     update_terminal_size();
     if (m_rows == 0 || m_lines.size() <= m_rows - 1)
@@ -196,15 +197,6 @@ void terminal_pager::show()
     m_start_row_index = 0;
 }
 
-void terminal_pager::split_input_at_newlines(std::string_view str)
-{
-    auto split = str | std::ranges::views::split('\n')
-                     | std::ranges::views::transform([](auto&& range) {
-                           return std::string(range.begin(), range.end());
-                       });
-    m_lines = std::vector<std::string>{split.begin(), split.end()};
-}
-
 void terminal_pager::update_terminal_size()
 {
     struct winsize size;

src/utils/terminal_pager.hpp

@@ -49,8 +49,6 @@ class terminal_pager
 
     void scroll(bool up, bool page);
 
-    void split_input_at_newlines(std::string_view str);
-
     void update_terminal_size();
 
 

src/wrapper/repository_wrapper.cpp

@@ -513,11 +513,7 @@ std::vector<std::string> repository_wrapper::tag_list_match(std::string pattern)
     git_strarray tag_names;
     throw_if_error(git_tag_list_match(&tag_names, pattern.c_str(), *this));
 
-    std::vector<std::string> result;
-    for (size_t i = 0; i < tag_names.count; ++i)
-    {
-        result.emplace_back(tag_names.strings[i]);
-    }
+    std::vector<std::string> result(tag_names.strings, tag_names.strings + tag_names.count);
 
     git_strarray_dispose(&tag_names);
     return result;

test/test_tag.py

@@ -175,7 +175,7 @@ def test_tag_list_with_message_lines(xtl_clone, commit_env_config, git2cpp_path,
     xtl_path = tmp_path / "xtl"
 
     # Create an annotated tag with a message
-    create_cmd = [git2cpp_path, 'tag', '-m', 'First line\nSecond line\nThird line', 'v1.0.0']
+    create_cmd = [git2cpp_path, 'tag', '-m', 'First line\nSecond line\nThird line\nForth line', 'v1.0.0']
     p_create = subprocess.run(create_cmd, capture_output=True, cwd=xtl_path, text=True)
     assert p_create.returncode == 0
 
@@ -184,7 +184,10 @@ def test_tag_list_with_message_lines(xtl_clone, commit_env_config, git2cpp_path,
     p_list = subprocess.run(list_cmd, capture_output=True, cwd=xtl_path, text=True)
     assert p_list.returncode == 0
     assert 'v1.0.0' in p_list.stdout
-    # TODO: another assert after checking what should be printed with git
+    assert 'First line' in p_list.stdout
+    assert 'Second line' in p_list.stdout
+    assert 'Third line' in p_list.stdout
+    assert 'Forth line' not in p_list.stdout
 
 
 @pytest.mark.parametrize("force_flag", ["-f", "--force"])