git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Sangeeta Jain <sangunb09@gmail.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH v4] diff: do not show submodule with untracked files as "-dirty"
Date: Fri, 23 Oct 2020 08:56:51 -0700	[thread overview]
Message-ID: <xmqq7drhhr5o.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20201023111711.38739-1-sangunb09@gmail.com> (Sangeeta Jain's message of "Fri, 23 Oct 2020 16:47:11 +0530")

Sangeeta Jain <sangunb09@gmail.com> writes:

> From: sangu09 <sangunb09@gmail.com>
>
> Make `--ignore-submodules=untracked` the default for `git diff` when
> there is no configuration variable or command line option, so that the
> command would not give '-dirty' suffix to a submodule whose working
> tree has untracked files, to make it consistent with `git
> describe --dirty` that is run in the submodule working tree.

What happened to the first/introductory paragraph that was
excellently written in the previous round?

The usaul way to compose a log message is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), and
   discuss what you perceive as a problem in it.

 - Propose a solution (optional---often, problem description
   trivially leads to an obvious solution in reader's minds).

 - Give commands to the codebase to "become like so".

The first paragraph you had in the previous round was a good example
of the "observation".  What we see above is only the "commands"
part.  To those who have been intimately following the discussion,
it often is understandable without both, but we are not writing for
those who review the patches.  We are writing for future readers who
are not aware of these exchanges we are having, so we should give
something to prepare them by setting the stage and stating the
objective.

> diff --git a/diff.c b/diff.c
> index 2bb2f8f57e..5a80695da8 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4585,6 +4585,9 @@ void repo_diff_setup(struct repository *r, struct diff_options *options)
>  		DIFF_XDL_SET(options, INDENT_HEURISTIC);
>  
>  	options->orderfile = diff_order_file_cfg;
> +	
> +	if (!options->flags.ignore_submodule_set)
> +		options->flags.ignore_untracked_in_submodules = 1;

I think I already touched about this one in the previous review.

>  	strvec_pushl(&cp.args, "status", "--porcelain=2", NULL);
>  	if (ignore_untracked)
> -		strvec_push(&cp.args, "-uno");
> +		strvec_push (&cp.args, "-uno");

Noise.

> diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
> index ca04fac417..98e46ad1ae 100755
> --- a/t/t3701-add-interactive.sh
> +++ b/t/t3701-add-interactive.sh
> @@ -761,7 +761,7 @@ test_expect_success 'setup different kinds of dirty submodules' '
>  		echo dirty >>initial &&
>  		: >untracked
>  	) &&
> -	git -C for-submodules diff-files --name-only >actual &&
> +	git -C for-submodules diff-files --name-only --ignore-submodules=none >actual &&
>  	cat >expected <<-\EOF &&
>  	dirty-both-ways
>  	dirty-head

Hmph, it seems that you chose to give options that are no longer the
default to command invocations, so that the expected output from the
previous world order before this patch can be reused, but is that a
sensible way to go?  If we are changing the default behaviour, shouldn't
we rather be testing that new behaviour, I wonder?

> diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
> index 537787e598..78cd86be3a 100755
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -503,6 +503,31 @@ test_expect_success 'untracked changes in added submodule (AM S..U)' '
>  		1 AM S..U 000000 160000 160000 $ZERO_OID $HSUB sub1
>  		EOF
>  
> +		git status --porcelain=v2 --branch --untracked-files=all --ignore-submodules=none >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'untracked changes in added submodule (A. S...) (untracked ignored)' '
> +	(	cd super_repo &&
> +		## create untracked file in the submodule.
> +		(	cd sub1 &&
> +			echo "xxxx" >file_in_sub
> +		) &&
> +
> +		HMOD=$(git hash-object -t blob -- .gitmodules) &&
> +		HSUP=$(git rev-parse HEAD) &&
> +		HSUB=$HSUP &&
> +
> +		cat >expect <<-EOF &&
> +		# branch.oid $HSUP
> +		# branch.head master
> +		# branch.upstream origin/master
> +		# branch.ab +0 -0
> +		1 A. N... 000000 100644 100644 $ZERO_OID $HMOD .gitmodules
> +		1 A. S... 000000 160000 160000 $ZERO_OID $HSUB sub1
> +		EOF
> +
>  		git status --porcelain=v2 --branch --untracked-files=all >actual &&
>  		test_cmp expect actual
>  	)
> @@ -582,6 +607,33 @@ test_expect_success 'staged and untracked changes in added submodule (AM S.MU)'
>  		1 AM S.MU 000000 160000 160000 $ZERO_OID $HSUB sub1
>  		EOF
>  
> +		git status --porcelain=v2 --branch --untracked-files=all --ignore-submodules=none >actual &&
> +		test_cmp expect actual
> +	)
> +'
> +
> +test_expect_success 'staged and untracked changes in added submodule (AM S.M.) (untracked ignored)' '
> +	(	cd super_repo &&
> +		(	cd sub1 &&
> +			## stage new changes in tracked file.
> +			git add file_in_sub &&
> +			## create new untracked file.
> +			echo "yyyy" >>another_file_in_sub
> +		) &&
> +
> +		HMOD=$(git hash-object -t blob -- .gitmodules) &&
> +		HSUP=$(git rev-parse HEAD) &&
> +		HSUB=$HSUP &&
> +
> +		cat >expect <<-EOF &&
> +		# branch.oid $HSUP
> +		# branch.head master
> +		# branch.upstream origin/master
> +		# branch.ab +0 -0
> +		1 A. N... 000000 100644 100644 $ZERO_OID $HMOD .gitmodules
> +		1 AM S.M. 000000 160000 160000 $ZERO_OID $HSUB sub1
> +		EOF
> +
>  		git status --porcelain=v2 --branch --untracked-files=all >actual &&
>  		test_cmp expect actual
>  	)

OK.

By the way, in builtin/describe.c there is an invocation of
"diff-index" without any --ignore-submodules=<what> option.

        /* diff-index command arguments to check if working tree is dirty. */
        static const char *diff_index_args[] = {
                "diff-index", "--quiet", "HEAD", "--", NULL
        };

Would the behaviour of diff-index invocation in "git describe --dirty"
affected by the change of the default in any way?

Thanks.

  reply	other threads:[~2020-10-23 15:57 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 17:08 [PATCH] diff: do not show submodule with untracked files as "-dirty" Sangeeta via GitGitGadget
2020-10-20 13:38 ` [OUTREACHY][PATCH] " Phillip Wood
2020-10-20 18:10   ` Sangeeta NB
2020-10-21 11:28     ` Phillip Wood
2020-10-21 13:10 ` [Outreachy] [PATCH v2] " Sangeeta Jain
2020-10-21 17:43   ` Eric Sunshine
2020-10-21 19:40     ` Sangeeta NB
2020-10-21 23:04       ` Eric Sunshine
2020-10-22 11:22 ` [Outreachy] [PATCH v3] " Sangeeta Jain
2020-10-22 18:07   ` Junio C Hamano
2020-10-23  5:23     ` Sangeeta NB
2020-10-23 15:19       ` Junio C Hamano
2020-10-23 18:17         ` Sangeeta NB
2020-10-23 18:55           ` Junio C Hamano
2020-10-23 19:08             ` Sangeeta NB
2020-10-23 11:17 ` [PATCH v4] " Sangeeta Jain
2020-10-23 15:56   ` Junio C Hamano [this message]
2020-10-23 18:32     ` Sangeeta NB
2020-10-23 20:22       ` Junio C Hamano
2020-10-23 11:18 ` [Outreachy] " Sangeeta Jain
2020-10-23 21:28   ` Junio C Hamano
2020-10-25 10:23     ` Sangeeta NB
2020-10-26 17:36       ` Junio C Hamano
2020-10-23 19:29 ` [Outreachy] [PATCH v5] " Sangeeta Jain
2020-10-26 17:57 ` [Outreachy][PATCH v6] " Sangeeta Jain
2020-11-03 10:46   ` Sangeeta
2020-11-03 17:55     ` Junio C Hamano
2020-11-07 10:47       ` Sangeeta
2020-12-08 21:02         ` Junio C Hamano
2020-11-07 11:10   ` Đoàn Trần Công Danh
2020-11-09 15:19     ` Sangeeta
2020-11-09 17:01       ` Junio C Hamano
2020-11-10  8:39 ` [Outreachy][PATCH v7] " Sangeeta Jain
2020-11-10 17:09   ` Đoàn Trần Công Danh
2020-12-08 13:36   ` Sangeeta
2020-12-08 22:26     ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq7drhhr5o.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=sangunb09@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).