Subject: [PATCH v2 0/3] add: use advise_if_enabled

From: Rubén Justo <[email-masked]>
Date: Sat, 30 Mar 2024 15:00:11 +0100

This series is a simple change, in builtin/add.c, from:

	if (advice_enabled(XXX))
		advise(MMM)

to the newer:

	advise_if_enabled(XXX, MMM)

Rubén Justo (3):
  add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
  add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
  add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO

 builtin/add.c              | 18 ++++++---------
 t/t3700-add.sh             | 47 ++++++++++++++++++++++++++++++++++++--
 t/t7400-submodule-basic.sh |  3 +--
 3 files changed, 53 insertions(+), 15 deletions(-)

Range-diff against v1:
1:  c599ff8b98 ! 1:  9462b7045d add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
    @@ Metadata
      ## Commit message ##
         add: use advise_if_enabled for ADVICE_ADD_IGNORED_FILE
     
    -    Use the newer advise_if_enabled() machinery to show the advice.
    +    Since b3b18d1621 (advice: revamp advise API, 2020-03-02), we can use
    +    advise_if_enabled() to display an advice.  This API encapsulates three
    +    actions:
    +            1.- checking the visibility of the advice
    +
    +            2.- displaying the advice when appropriate
    +
    +            3.- displaying instructions on how to disable the advice, when
    +                appropriate
    +
    +    The code we have in builtin/add.c to display the ADVICE_ADD_IGNORED_FILE
    +    advice, is doing these three things.  However, the instructions
    +    displayed on how to disable the hint are not shown in the normalized way
    +    that advise_if_enabled() introduced.  This may cause distraction.
    +
    +    There is no reason not to use the new API here.  On the contrary, by
    +    using it we gain simplicity in the code and avoid possible distractions.
    +
    +    For these reasons, use the newer advise_if_enabled() machinery to show
    +    the ADVICE_ADD_IGNORED_FILE advice, and don't bother checking the
    +    visibility or displaying the instruction on how to disable the advice.
     
         Signed-off-by: Rubén Justo <[email-masked]>
     
2:  76550e01e1 ! 2:  f892013059 add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
    @@ Metadata
      ## Commit message ##
         add: use advise_if_enabled for ADVICE_ADD_EMPTY_PATHSPEC
     
    -    Use the newer advise_if_enabled() machinery to show the advice.
    +    Since 93b0d86aaf (git-add: error out when given no arguments.,
    +    2006-12-20) we display a message when no arguments are given to "git
    +    add".
     
    -    We don't have a test for this.  Add one.
    +    Part of that message was converted to advice in bf66db37f1 (add: use
    +    advise function to display hints, 2020-01-07).
    +
    +    Following the same line of reasoning as in the previous commit, it is
    +    sensible to use advise_if_enabled() here.
    +
    +    Therefore, use advise_if_enabled() in builtin/add.c to show the
    +    ADVICE_ADD_EMPTY_PATHSPEC advice, and don't bother checking there the
    +    visibility of the advice or displaying the instruction on how to disable
    +    it.
    +
    +    Also add a test for these messages, in order to detect a possible
    +    change in them.
     
         Signed-off-by: Rubén Justo <[email-masked]>
     
3:  3017dd2188 ! 3:  254ece0ee4 add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO
    @@ Metadata
      ## Commit message ##
         add: use advise_if_enabled for ADVICE_ADD_EMBEDDED_REPO
     
    -    Use the newer advise_if_enabled() machinery to show the advice.
    +    By following a similar reasoning as in previous commits, there are no
    +    reason why we should not use the advise_if_enabled() API to display the
    +    ADVICE_ADD_EMBEDDED_REPO advice.
     
    -    We don't have a test for this.  Add one.
    +    This advice was introduced in 532139940c (add: warn when adding an
    +    embedded repository, 2017-06-14).  Some tests were included in the
    +    commit, but none is testing this advice.  Which, note, we only want to
    +    display once per run.
     
    +    So, use the advise_if_enabled() machinery to show the
    +    ADVICE_ADD_EMBEDDED_REPO advice and include a test to notice any
    +    possible breakage.
    +
    +    Signed-off-by: Junio C Hamano <[email-masked]>
         Signed-off-by: Rubén Justo <[email-masked]>
     
      ## builtin/add.c ##
    @@ t/t3700-add.sh: test_expect_success '"git add ." in empty repo' '
      	)
      '
      
    -+test_expect_success '"git add" a nested repository' '
    -+	rm -fr empty &&
    -+	git init empty &&
    ++test_expect_success '"git add" a embedded repository' '
    ++	rm -fr outer && git init outer &&
     +	(
    -+		cd empty &&
    -+		git init empty &&
    -+		(
    -+			cd empty &&
    -+			git commit --allow-empty -m "foo"
    -+		) &&
    -+		git add empty 2>actual &&
    ++		cd outer &&
    ++		for i in 1 2
    ++		do
    ++			name=inner$i &&
    ++			git init $name &&
    ++			git -C $name commit --allow-empty -m $name ||
    ++				return 1
    ++		done &&
    ++		git add . 2>actual &&
     +		cat >expect <<-EOF &&
    -+		warning: adding embedded git repository: empty
    ++		warning: adding embedded git repository: inner1
     +		hint: You${SQ}ve added another git repository inside your current repository.
     +		hint: Clones of the outer repository will not contain the contents of
     +		hint: the embedded repository and will not know how to obtain it.
     +		hint: If you meant to add a submodule, use:
     +		hint:
    -+		hint: 	git submodule add <url> empty
    ++		hint: 	git submodule add <url> inner1
     +		hint:
     +		hint: If you added this path by mistake, you can remove it from the
     +		hint: index with:
     +		hint:
    -+		hint: 	git rm --cached empty
    ++		hint: 	git rm --cached inner1
     +		hint:
     +		hint: See "git help submodule" for more information.
     +		hint: Disable this message with "git config advice.addEmbeddedRepo false"
    ++		warning: adding embedded git repository: inner2
     +		EOF
     +		test_cmp expect actual
     +	)

base-commit: 2d8cf94b28de9da683ddd40961a3a572f2741cf3
-- 
2.44.0.417.g254ece0ee4