address review cooment

SandrineP · SandrineP · commit 3aa613e2cc21 · 2026-03-11T11:55:57.000+01:00
diff --git a/src/subcommand/log_subcommand.cpp b/src/subcommand/log_subcommand.cpp
@@ -16,7 +16,7 @@ log_subcommand::log_subcommand(const libgit2_object&, CLI::App& app)
 {
     auto *sub = app.add_subcommand("log", "Shows commit logs");
 
-    sub->add_option("--format", m_format_flag, "Pretty-print the contents of the commit logs in a given format, where <format> can be one of full and fuller");
+    sub->add_option("--format", m_format_flag, "Pretty-print the contents of the commit logs in a given format, where <format> can be one of full, fuller or oneline");
     sub->add_option("-n,--max-count", m_max_count_flag, "Limit the output to <number> commits.");
     sub->add_flag("--abbrev-commit", m_abbrev_commit_flag, "Instead of showing the full 40-byte hexadecimal commit object name, show a prefix that names the object uniquely. --abbrev=<n> (which also modifies diff output, if it is displayed) option can be used to specify the minimum length of the prefix.");
     sub->add_option("--abbrev", m_abbrev, "Instead of showing the full 40-byte hexadecimal object name in diff-raw format output and diff-tree header lines, show the shortest prefix that is at least <n> hexdigits long that uniquely refers the object.");
@@ -230,7 +230,9 @@ void log_subcommand::print_commit(repository_wrapper& repo, const commit_wrapper
 
     std::string message = commit.message();
     while (!message.empty() && message.back() == '\n')
+    {
         message.pop_back();
+    }
 
     if (oneline)
     {
diff --git a/src/subcommand/revparse_subcommand.cpp b/src/subcommand/revparse_subcommand.cpp
@@ -8,10 +8,9 @@ revparse_subcommand::revparse_subcommand(const libgit2_object&, CLI::App& app)
 
     auto* bare_opt = sub->add_flag("--is-bare-repository", m_is_bare_repository_flag, "When the repository is bare print \"true\", otherwise \"false\".");
     auto* shallow_opt = sub->add_flag("--is-shallow-repository", m_is_shallow_repository_flag, "When the repository is shallow print \"true\", otherwise \"false\".");
+    auto* rev_opt = sub->add_option("<rev>", m_revisions, "Revision(s) to parse (e.g. HEAD, main, HEAD~1, dae86e, ...)");
 
-    sub->add_option("<rev>", m_revisions, "Revision(s) to parse (e.g. HEAD, main, HEAD~1, dae86e, ...)");
-
-    sub->parse_complete_callback([this, sub, bare_opt, shallow_opt]() {
+    sub->parse_complete_callback([this, sub, bare_opt, shallow_opt, rev_opt]() {
         for (CLI::Option* opt : sub->parse_order())
         {
             if (opt == bare_opt)
@@ -22,6 +21,10 @@ revparse_subcommand::revparse_subcommand(const libgit2_object&, CLI::App& app)
             {
                 m_queries_in_order.push_back("is_shallow");
             }
+            else if (opt == rev_opt)
+            {
+                m_queries_in_order.push_back("is_rev");
+            }
         }
     });
 
@@ -33,6 +36,7 @@ void revparse_subcommand::run()
     auto directory = get_current_git_path();
     auto repo = repository_wrapper::open(directory);
 
+    size_t i = 0;
     if (!m_queries_in_order.empty())
     {
         for (const auto& q : m_queries_in_order)
@@ -41,31 +45,25 @@ void revparse_subcommand::run()
             {
                 std::cout << std::boolalpha << repo.is_bare() << std::endl;
             }
-            if (q == "is_shallow")
+            else if (q == "is_shallow")
             {
                 std::cout << std::boolalpha << repo.is_shallow() << std::endl;
             }
-        }
-        return;
-    }
+            else if (q == "is_rev")
+            {
+                const auto& rev = m_revisions[i];
+                auto obj = repo.revparse_single(rev.c_str());
 
-    if (!m_revisions.empty())
-    {
-        for (const auto& rev : m_revisions)
-        {
-            auto obj = repo.revparse_single(rev.c_str());
+                if (!obj.has_value())
+                {
+                    throw git_exception("bad revision '" + rev + "'", git2cpp_error_code::BAD_ARGUMENT);
+                }
 
-            if (!obj.has_value())
-            {
-                throw git_exception("bad revision '" + rev + "'", git2cpp_error_code::BAD_ARGUMENT);
-                return;
+                auto oid = obj.value().oid();
+                std::cout << git_oid_tostr_s(&oid) << std::endl;
+                i += 1;
             }
-
-            auto oid = obj.value().oid();
-            std::cout << git_oid_tostr_s(&oid) << std::endl;
         }
-        return;
     }
-
-    std::cout << "revparse only supports --is-bare-repository, --is-shallow-repository and parsing revisions for now" << std::endl;
+    return;
 }
diff --git a/test/test_revparse.py b/test/test_revparse.py
@@ -70,6 +70,55 @@ def test_revparse_multiple_revs(repo_init_with_commit, git2cpp_path, tmp_path):
     assert p.returncode == 0
 
     lines = p.stdout.splitlines()
+    print()
     assert len(lines) == 2
     assert all(len(x) == 40 for x in lines)
     assert lines[0] != lines[1]
+
+
+def test_revparse_multiple_opts(git2cpp_path, tmp_path, run_in_tmp_path):
+    """Test the options are printed in order"""
+    url = "https://github.com/xtensor-stack/xtl.git"
+    cmd = [git2cpp_path, "clone", "--depth", "2", url]
+    p = subprocess.run(cmd, capture_output=True, text=True, cwd=tmp_path)
+    assert p.returncode == 0
+    assert (tmp_path / "xtl").exists()
+
+    xtl_path = tmp_path / "xtl"
+
+    p = subprocess.run(
+        [
+            git2cpp_path,
+            "rev-parse",
+            "HEAD",
+            "--is-shallow-repository",
+            "--is-bare-repository",
+            "HEAD~1",
+        ],
+        capture_output=True,
+        text=True,
+        cwd=xtl_path,
+    )
+    assert p.returncode == 0
+
+    lines = p.stdout.splitlines()
+    assert len(lines) == 4
+    assert len(lines[0]) == 40
+    assert len(lines[3]) == 40
+    assert lines[0] != lines[1]
+    assert "true" in lines[1]
+    assert "false" in lines[2]
+
+
+def test_revparse_errors(repo_init_with_commit, git2cpp_path, tmp_path):
+    assert (tmp_path / "initial.txt").exists()
+
+    rev_cmd = [git2cpp_path, "rev-parse", "HEAD~1"]
+    p_rev = subprocess.run(rev_cmd, capture_output=True, text=True, cwd=tmp_path)
+    assert p_rev.returncode == 129
+    assert "bad revision" in p_rev.stderr
+
+    opt_cmd = [git2cpp_path, "rev-parse", "--parseopt"]
+    p_opt = subprocess.run(opt_cmd, capture_output=True, text=True, cwd=tmp_path)
+    assert p_opt.returncode != 0
+    assert "The following argument was not expected:" in p_opt.stderr
diff --git a/test/test_tag.py b/test/test_tag.py
@@ -135,8 +135,8 @@ def test_tag_delete_nonexistent(repo_init_with_commit, git2cpp_path, tmp_path):
     assert "not found" in p_delete.stderr
 
 
-@pytest.mark.parametrize("list_flag", ["-l", "--list"])
-def test_tag_list_with_flag(
+@pytest.mark.parametrize("list_flag", ["", "-l", "--list"])
+def test_tag_list(
     repo_init_with_commit, commit_env_config, git2cpp_path, tmp_path, list_flag
 ):
     """Test listing tags with -l or --list flag."""
@@ -350,3 +350,18 @@ def test_tag_annotate_flag_requires_message(
     p = subprocess.run(create_cmd, capture_output=True, cwd=tmp_path, text=True)
     assert p.returncode != 0
     assert "requires -m" in p.stderr
+
+
+def test_tag_errors(repo_init_with_commit, commit_env_config, git2cpp_path, tmp_path):
+    assert (tmp_path / "initial.txt").exists()
+
+    # Test that -a/--annotate without -m fails.
+    create_cmd = [git2cpp_path, "tag", "-a", "v1.0.0"]
+    p_create = subprocess.run(create_cmd, capture_output=True, cwd=tmp_path, text=True)
+    assert p_create.returncode != 0
+    assert "requires -m" in p_create.stderr
+
+    # Test that command fails when no message
+    del_cmd = [git2cpp_path, "tag", "-d"]
+    p_del = subprocess.run(del_cmd, capture_output=True, cwd=tmp_path, text=True)
+    assert p_del.returncode != 0

Original file line number	Diff line number	Diff line change
`@@ -8,10 +8,9 @@ revparse_subcommand::revparse_subcommand(const libgit2_object&, CLI::App& app)`
`8`	`8`
`9`	`9`	`auto* bare_opt = sub->add_flag("--is-bare-repository", m_is_bare_repository_flag, "When the repository is bare print \"true\", otherwise \"false\".");`
`10`	`10`	`auto* shallow_opt = sub->add_flag("--is-shallow-repository", m_is_shallow_repository_flag, "When the repository is shallow print \"true\", otherwise \"false\".");`
	`11`	`+ auto* rev_opt = sub->add_option("<rev>", m_revisions, "Revision(s) to parse (e.g. HEAD, main, HEAD~1, dae86e, ...)");`
`11`	`12`
`12`		`- sub->add_option("<rev>", m_revisions, "Revision(s) to parse (e.g. HEAD, main, HEAD~1, dae86e, ...)");`
`13`		`-`
`14`		`- sub->parse_complete_callback([this, sub, bare_opt, shallow_opt]() {`
	`13`	`+ sub->parse_complete_callback([this, sub, bare_opt, shallow_opt, rev_opt]() {`
`15`	`14`	`for (CLI::Option* opt : sub->parse_order())`
`16`	`15`	`{`
`17`	`16`	`if (opt == bare_opt)`
`@@ -22,6 +21,10 @@ revparse_subcommand::revparse_subcommand(const libgit2_object&, CLI::App& app)`
`22`	`21`	`{`
`23`	`22`	`m_queries_in_order.push_back("is_shallow");`
`24`	`23`	`}`
	`24`	`+ else if (opt == rev_opt)`
	`25`	`+ {`
	`26`	`+ m_queries_in_order.push_back("is_rev");`
	`27`	`+ }`
`25`	`28`	`}`
`26`	`29`	`});`
`27`	`30`
`@@ -33,6 +36,7 @@ void revparse_subcommand::run()`
`33`	`36`	`auto directory = get_current_git_path();`
`34`	`37`	`auto repo = repository_wrapper::open(directory);`
`35`	`38`
	`39`	`+ size_t i = 0;`
`36`	`40`	`if (!m_queries_in_order.empty())`
`37`	`41`	`{`
`38`	`42`	`for (const auto& q : m_queries_in_order)`
`@@ -41,31 +45,25 @@ void revparse_subcommand::run()`
`41`	`45`	`{`
`42`	`46`	`std::cout << std::boolalpha << repo.is_bare() << std::endl;`
`43`	`47`	`}`
`44`		`- if (q == "is_shallow")`
	`48`	`+ else if (q == "is_shallow")`
`45`	`49`	`{`
`46`	`50`	`std::cout << std::boolalpha << repo.is_shallow() << std::endl;`
`47`	`51`	`}`
`48`		`- }`
`49`		`- return;`
`50`		`- }`
	`52`	`+ else if (q == "is_rev")`
	`53`	`+ {`
	`54`	`+ const auto& rev = m_revisions[i];`
	`55`	`+ auto obj = repo.revparse_single(rev.c_str());`
`51`	`56`
`52`		`- if (!m_revisions.empty())`
`53`		`- {`
`54`		`- for (const auto& rev : m_revisions)`
`55`		`- {`
`56`		`- auto obj = repo.revparse_single(rev.c_str());`
	`57`	`+ if (!obj.has_value())`
	`58`	`+ {`
	`59`	`+ throw git_exception("bad revision '" + rev + "'", git2cpp_error_code::BAD_ARGUMENT);`
	`60`	`+ }`
`57`	`61`
`58`		`- if (!obj.has_value())`
`59`		`- {`
`60`		`- throw git_exception("bad revision '" + rev + "'", git2cpp_error_code::BAD_ARGUMENT);`
`61`		`- return;`
	`62`	`+ auto oid = obj.value().oid();`
	`63`	`+ std::cout << git_oid_tostr_s(&oid) << std::endl;`
	`64`	`+ i += 1;`
`62`	`65`	`}`
`63`		`-`
`64`		`- auto oid = obj.value().oid();`
`65`		`- std::cout << git_oid_tostr_s(&oid) << std::endl;`
`66`	`66`	`}`
`67`		`- return;`
`68`	`67`	`}`
`69`		`-`
`70`		`- std::cout << "revparse only supports --is-bare-repository, --is-shallow-repository and parsing revisions for now" << std::endl;`
	`68`	`+ return;`
`71`	`69`	`}`