Thank you Erik. -Jaikiran
On Monday, July 29, 2019, Erik Joelsson <erik.joels...@oracle.com> wrote: > This looks good to me. I can sponsor the change. > > /Erik > > On 2019-07-29 04:12, Jaikiran Pai wrote: > >> I got some more inputs from Stuart Marks on the linked JBS issue[1] and >> I've incorporated those in the latest updated webrev which is at [2]. >> >> I've tested this latest change locally on mercurial version 5.0.1 and it >> has worked as expected (now ignores JTreport and JTwork from the root as >> well as sub directories). Plus, with this change I haven't seen any >> files/directories that are now being unexpectedly not ignored. >> >> Anyone willing to review and sponsor this please? >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8227170 >> >> [2] http://cr.openjdk.java.net/~jpai/webrev/8227170/3/webrev/ >> >> -Jaikiran >> >> On 12/07/19 4:04 PM, Jaikiran Pai wrote: >> >>> Hello Dean, >>> >>> Thank you for testing this. I have now updated the webrev to use the >>> JTreport/* syntax and have also verified that it works for me too. It >>> correctly ignores both root level JTreport and JTwork directories and >>> any such sub-directories. I am on a macOS with: >>> >>> hg --version >>> Mercurial Distributed SCM (version 5.0.1) >>> (see https://mercurial-scm.org for more information) >>> >>> The updated webrev is at >>> http://cr.openjdk.java.net/~jpai/webrev/8227170/2/webrev/ >>> >>> -Jaikiran >>> >>> On 11/07/19 1:16 AM, dean.l...@oracle.com wrote: >>> >>>> The ** syntax isn't working for me in Mercurial 2.9, but the following: >>>> >>>> syntax: glob >>>> JTreport/* >>>> JTwork/* >>>> >>>> seems to work in both 2.9 and 4.6.1. This also seems to work: >>>> >>>> syntax: glob >>>> JTreport >>>> JTwork >>>> >>>> but it matches files and not just directories. >>>> >>>> dl >>>> >>>> On 7/10/19 4:48 AM, Jaikiran Pai wrote: >>>> >>>>> Ping. Anyone willing to review and sponsor this change? >>>>> >>>>> -Jaikiran >>>>> >>>>> On 03/07/19 5:43 PM, Jaikiran Pai wrote: >>>>> >>>>>> Hello, >>>>>> >>>>>> Can I please get a review and a sponsor for the patch for the >>>>>> enhancement noted in https://bugs.openjdk.java.net/browse/JDK-8227170 >>>>>> ? >>>>>> >>>>>> The webrev containing the patch is here >>>>>> http://cr.openjdk.java.net/~jpai/webrev/8227170/00/webrev/ >>>>>> >>>>>> For some context about this change - there's been some discussion on >>>>>> this in the jdk-dev list here >>>>>> https://mail.openjdk.java.net/pipermail/jdk-dev/2019-June/003076.html >>>>>> . >>>>>> >>>>>> The patch in the webrev, does the following things: >>>>>> >>>>>> - In addition to ignoring the JTwork and JTreport sub-directories, >>>>>> this >>>>>> now also ignores these directories at the root of the repo. This patch >>>>>> uses "syntax: glob" to easily ignore such directories, instead of the >>>>>> (default) "regex" syntax. In the jdk-dev mailing list discussion, >>>>>> Gustavo suggested to continue using the regex syntax and listing the >>>>>> following in the .hgignore: >>>>>> >>>>>> .*JTreport/.* >>>>>> >>>>>> .*JTwork/.* >>>>>> >>>>>> But that would then end up ignoring directories of the form >>>>>> "foobarJTreport" and files under it like "foobarJTreport/blah.txt". I >>>>>> guess we can still stick with a regex syntax and come up with a regex >>>>>> which ignores these directories both at the root as well as >>>>>> sub-directories, maybe something like: >>>>>> >>>>>> (.*/JTreport/.*|^JTreport/.*) >>>>>> >>>>>> (.*/JTwork/.*|^JTwork/.*) >>>>>> >>>>>> but IMO, that appears a bit too complex when compared to the glob >>>>>> syntax >>>>>> that's used in the webrev. >>>>>> >>>>>> - The other change in this webrev is to ignore the >>>>>> src/utils/hsdis/build/ directory which gets generated when >>>>>> src/utils/hsdis is built. I included this in this patch based on >>>>>> Gustavo's suggestion in that linked thread. I verified that this is >>>>>> indeed the "build" location used by hsdis tool, by checking the >>>>>> src/utils/hsdis/Makefile as well src/utils/hsdis/README. >>>>>> >>>>>> Gustavo also had a suggestion that we ignore the "oprofile_data" >>>>>> directory/files. But I don't have knowledge of what those files are >>>>>> and >>>>>> didn't have a way to ascertain the right location/rule to add to the >>>>>> .hgignore. So I have not included that in the current version of this >>>>>> webrev. I'm however open to including this in the webrev if Gustavo >>>>>> and >>>>>> others feel that's OK/needed. >>>>>> >>>>>> -Jaikiran >>>>>> >>>>>> >>>>>>
