| Message ID | 20240626065223.28154-1-abhijeet.nkt@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] describe: refresh the index when 'broken' flag is used | expand |
Abhijeet Sonar <abhijeet.nkt@gmail.com> writes: > 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> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com> > --- > builtin/describe.c | 11 +++++++++++ > t/t6120-describe.sh | 24 ++++++++++++++++++++++++ > 2 files changed, 35 insertions(+) > > diff --git a/builtin/describe.c b/builtin/describe.c > index e5287eddf2..7cb9d50b36 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,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix) > if (argc == 0) { > if (broken) { > struct child_process cp = CHILD_PROCESS_INIT; > + strvec_pushv(&cp.args, update_index_args); > + cp.git_cmd = 1; > + cp.no_stdin = 1; > + cp.no_stdout = 1; > + if (run_command(&cp)) > + child_process_clear(&cp); > + > 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..6c396e7abc 100755 > --- a/t/t6120-describe.sh > +++ b/t/t6120-describe.sh > @@ -671,4 +671,28 @@ 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' ' > + 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 && > + > + cat file >file.new && > + mv file.new file && > + git describe --dirty --broken >actual && > + echo "A" >expected && > + test_cmp expected actual Not worth a reroll, but you don't have to create file.new twice. diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh index 6c396e7abc..6c4b20fec7 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -683,14 +683,12 @@ test_expect_success 'describe --dirty with a file with changed stat' ' cat file >file.new && mv file.new file && - git describe --dirty >actual && echo "A" >expected && + + git describe --dirty >actual && test_cmp expected actual && - cat file >file.new && - mv file.new file && git describe --dirty --broken >actual && - echo "A" >expected && test_cmp expected actual ) ' > + ) > +' > + > test_done > > Range-diff against v4: > 1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used > @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr > + cp.git_cmd = 1; > + cp.no_stdin = 1; > + cp.no_stdout = 1; > -+ run_command(&cp); > -+ strvec_clear(&cp.args); > ++ if (run_command(&cp)) > ++ child_process_clear(&cp); > + > strvec_pushv(&cp.args, diff_index_args); > cp.git_cmd = 1; > -- > 2.45.2.606.g9005149a4a.dirty Other than this, this looks good to me. Thanks!
On 26/06/24 17:00, Karthik Nayak wrote: > Not worth a reroll, but you don't have to create file.new twice. Actually, now that I think of it, those two were better off being separate tests. It might so happen the first call to describe refreshes the index, due to which the second call with the --broken option does not bug-out in the way it would if the command was run by itself. Having them separate would give them enough isolation so that previous command does not interfere with the later. >> Range-diff against v4: >> 1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used >> @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr >> + cp.git_cmd = 1; >> + cp.no_stdin = 1; >> + cp.no_stdout = 1; >> -+ run_command(&cp); >> -+ strvec_clear(&cp.args); >> ++ if (run_command(&cp)) >> ++ child_process_clear(&cp); >> + >> strvec_pushv(&cp.args, diff_index_args); >> cp.git_cmd = 1; >> -- >> 2.45.2.606.g9005149a4a.dirty > > Other than this, this looks good to me. I am not sure if I follow this one. Am I expected to not share the struct child_process between the two sub-process calls? Thanks
Karthik Nayak <karthik.188@gmail.com> writes: >> + cat file >file.new && >> + mv file.new file && >> + git describe --dirty >actual && >> + echo "A" >expected && >> + test_cmp expected actual && >> + >> + cat file >file.new && >> + mv file.new file && >> + git describe --dirty --broken >actual && >> + echo "A" >expected && >> + test_cmp expected actual > > Not worth a reroll, but you don't have to create file.new twice. I think you have to make the "file" stat-dirty again after the first test, as a successful first test _should_ refresh the index (and that was why my manual illustration in an earlier message <xmqqsex2b4ti.fsf@gitster.g> did "--dirty --broken" first before "--dirty" alone, knowing that the former fails to refresh the index). You are right to point out that the expected results for "--dirty" and "--dirty --broken" are the same and we do not have to create it twice, though. If the filesystem has a usable inode number, then replacing "file" with a copy, i.e. "cat file >file.new && mv file.new file", is an excellent way to ensure that the stat data becomes dirty even when it is done as a part of a quick series of commands. But not all filesystems we run our tests on may have usable inode number, so it may not be the best thing to do in an automated test. We could use "test-tool chmtime" instead, perhaps like: ... make the index and the working tree are clean wrt HEAD ... # we do not expect -dirty suffix in the output echo A >expect && # make "file" stat-dirty test-tool chmtime -10 file && # "describe --dirty" refreshes and does not get fooled git describe --dirty >actual && test_cmp expect actual && # make "file" stat-dirty again test-tool chmtime -10 file && # "describe --dirty --broken" refreshes and does not get fooled git describe --dirty --broken >actual && test_cmp expect actual with the extra comments stripped (I added them only to explain what is going on to the readers of this e-mail message).
Abhijeet Sonar <abhijeet.nkt@gmail.com> writes: > 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> > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com> > --- As I screwed up with the suggestion to do child_process_clear() after a failed run_command(), let me fix that up. A suggested change that can be squashed into this patch is attached at the end. * (style) a blank line between a block of variable declarations and the first statement; * run_command(&cp) cleans cp so no need for separate child_process_clear(&cp); * but child_process_init(&cp) is needed, just as 4d0984be (fsck: do not reuse child_process structs, 2018-11-12) explains, before reusing the structure for a separate call. * instead of "replace with a different file" which relies on having a working inum, use "test-tool chmtime" to reliably force dirty mtime. * everybody else compares "actual" against "expect", not "expected". imitate them. * test "--dirty" and "--dirty --broken" separately in two separate tests. Thanks. builtin/describe.c | 5 +++-- t/t6120-describe.sh | 28 ++++++++++++++++++++-------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git c/builtin/describe.c w/builtin/describe.c index a58f6134f0..e936d2c19f 100644 --- c/builtin/describe.c +++ w/builtin/describe.c @@ -649,13 +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; + strvec_pushv(&cp.args, update_index_args); cp.git_cmd = 1; cp.no_stdin = 1; cp.no_stdout = 1; - if (run_command(&cp)) - child_process_clear(&cp); + run_command(&cp); + child_process_init(&cp); strvec_pushv(&cp.args, diff_index_args); cp.git_cmd = 1; cp.no_stdin = 1; diff --git c/t/t6120-describe.sh w/t/t6120-describe.sh index 6c396e7abc..79e0f19deb 100755 --- c/t/t6120-describe.sh +++ w/t/t6120-describe.sh @@ -672,6 +672,7 @@ 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' ' + test_when_finished "rm -fr stat-dirty" && git init stat-dirty && ( cd stat-dirty && @@ -680,18 +681,29 @@ test_expect_success 'describe --dirty with a file with changed stat' ' git add file && git commit -m A && git tag A -a -m A && + echo "A" >expect && - cat file >file.new && - mv file.new file && + test-tool chmtime -10 file && git describe --dirty >actual && - echo "A" >expected && - test_cmp expected actual && + test_cmp expect actual + ) +' + +test_expect_success 'describe --broken --dirty with a file with changed stat' ' + test_when_finished "rm -fr stat-dirty" && + git init stat-dirty && + ( + cd stat-dirty && - cat file >file.new && - mv file.new file && + echo A >file && + git add file && + git commit -m A && + git tag A -a -m A && + echo "A" >expect && + + test-tool chmtime -10 file && git describe --dirty --broken >actual && - echo "A" >expected && - test_cmp expected actual + test_cmp expect actual ) '
diff --git a/builtin/describe.c b/builtin/describe.c index e5287eddf2..7cb9d50b36 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,13 @@ int cmd_describe(int argc, const char **argv, const char *prefix) if (argc == 0) { if (broken) { struct child_process cp = CHILD_PROCESS_INIT; + strvec_pushv(&cp.args, update_index_args); + cp.git_cmd = 1; + cp.no_stdin = 1; + cp.no_stdout = 1; + if (run_command(&cp)) + child_process_clear(&cp); + 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..6c396e7abc 100755 --- a/t/t6120-describe.sh +++ b/t/t6120-describe.sh @@ -671,4 +671,28 @@ 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' ' + 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 && + + cat file >file.new && + mv file.new file && + git describe --dirty --broken >actual && + echo "A" >expected && + test_cmp expected actual + ) +' + test_done
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> Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: Abhijeet Sonar <abhijeet.nkt@gmail.com> --- builtin/describe.c | 11 +++++++++++ t/t6120-describe.sh | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+) Range-diff against v4: 1: 1da5fa48d9 ! 1: 52f590b70f describe: refresh the index when 'broken' flag is used @@ builtin/describe.c: int cmd_describe(int argc, const char **argv, const char *pr + cp.git_cmd = 1; + cp.no_stdin = 1; + cp.no_stdout = 1; -+ run_command(&cp); -+ strvec_clear(&cp.args); ++ if (run_command(&cp)) ++ child_process_clear(&cp); + strvec_pushv(&cp.args, diff_index_args); cp.git_cmd = 1;