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?