git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2] describe: refresh the index when 'broken' flag is used

From
Karthik Nayak <karthik.188@gmail.com>
Date
Jun 25, 2024, 12:27 UTC
Message-ID
<CAOLa=ZT5asAE8-=jY7iR2hsGGshjQtBvpjcVacQk5YFa0u3Sfg@mail.gmail.com>
In-Reply-To
<20240625064504.58286-1-abhijeet.nkt@gmail.com>
Abhijeet Sonar <abhijeet.nkt@gmail.com> writes:
Show 50 quoted lines
> When describe is run with 'dirty' flag, we refresh the index
> to make sure it is in sync with the filesystem before
> determining if the working tree is dirty.  However, this is
> not done for the codepath where the 'broken' flag is used.
>
> This causes `git describe --broken --dirty` to false
> positively report the worktree being dirty if a file has
> different stat info than what is recorded in the index.
> Running `git update-index -q --refresh` to refresh the index
> before running diff-index fixes the problem.
>
> Also add tests to deliberately update stat info of a
> file before running describe to verify it behaves correctly.
>
> Reported-by: Paul Millar <paul.millar@desy.de>
> Suggested-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com>
> ---
>  builtin/describe.c  | 12 ++++++++++++
>  t/t6120-describe.sh | 32 ++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/builtin/describe.c b/builtin/describe.c
> index e5287eddf2..3e751f1239 100644
> --- a/builtin/describe.c
> +++ b/builtin/describe.c
> @@ -53,6 +53,10 @@ static const char *diff_index_args[] = {
>  	"diff-index", "--quiet", "HEAD", "--", NULL
>  };
>
> +static const char *update_index_args[] = {
> +	"update-index", "--unmerged", "-q", "--refresh", NULL
> +};
> +
>  struct commit_name {
>  	struct hashmap_entry entry;
>  	struct object_id peeled;
> @@ -645,6 +649,14 @@ int cmd_describe(int argc, const char **argv, const char *prefix)
>  	if (argc == 0) {
>  		if (broken) {
>  			struct child_process cp = CHILD_PROCESS_INIT;
> +			struct child_process update_index_cp = CHILD_PROCESS_INIT;
> +
> +			strvec_pushv(&update_index_cp.args, update_index_args);
> +			update_index_cp.git_cmd = 1;
> +			update_index_cp.no_stdin = 1;
> +			update_index_cp.no_stdout = 1;
> +			run_command(&update_index_cp);
> +

We should also call `strvec_clear(&update_index_cp.args);` to clear up used memory.

Nit: we could actually `cp` for both the child processes here.
Show 12 quoted lines
>  			strvec_pushv(&cp.args, diff_index_args);
>  			cp.git_cmd = 1;
>  			cp.no_stdin = 1;
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index e78315d23d..ac781a7b52 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -671,4 +671,36 @@ test_expect_success 'setup misleading taggerdates' '
>
>  check_describe newer-tag-older-commit~1 --contains unique-file~2
>
> +test_expect_success 'describe --dirty with a file with changed stat' '

It would be nice to cleanup the repo at the end of the test by adding `test_when_finished "rm -rf stat-dirty" &&` here

Show 14 quoted lines
> +	git init stat-dirty &&
> +	cd stat-dirty &&
> +
> +	echo A >file &&
> +	git add file &&
> +	git commit -m A &&
> +	git tag A -a -m A &&
> +
> +	cat file >file.new &&
> +	mv file.new file &&
> +	git describe --dirty >actual &&
> +	echo "A" >expected &&
> +	test_cmp expected actual
> +'

You want to do everything apart from the repo init in a subshell, this ensures we don't carry over the working directory to the next test.

Show 15 quoted lines
> +test_expect_success 'describe --dirty --broken with a file with changed stat' '
> +	git init stat-dirty-broken &&
> +	cd stat-dirty-broken &&
> +
> +	echo A >file &&
> +	git add file &&
> +	git commit -m A &&
> +	git tag A -a -m A &&
> +
> +	cat file >file.new &&
> +	mv file.new file &&
> +	git describe --dirty --broken >actual &&
> +	echo "A" >expected &&
> +	test_cmp expected actual
> +'
Can't this be merged with the previous test?
Previous: Abhijeet SonarNext: Abhijeet Sonar
Message 12 of 14 in “describe: refresh the index when 'broken' flag is used”
  1. describe: refresh the index when 'broken' flag is usedAbhijeet Sonar, Jun 23, 2024
  2. Abhijeet SonarJun 23, 2024
  3. Karthik NayakJun 24, 2024
  4. Phillip WoodJun 24, 2024
  5. Abhijeet SonarJun 24, 2024
  6. Abhijeet SonarJun 24, 2024
  7. Junio C HamanoJun 24, 2024
  8. Abhijeet SonarJun 24, 2024
  9. Junio C HamanoJun 24, 2024
  10. Abhijeet SonarJun 25, 2024
  11. describe: refresh the index when 'broken' flag is usedAbhijeet Sonar, Jun 25, 2024
  12. Karthik NayakJun 25, 2024
  13. Abhijeet SonarJun 25, 2024
  14. Karthik NayakJun 25, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.