Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 13 May 2023 08:05:21 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Zhuo, Qiuxu" <qiuxu.zhuo@intel.com>
Cc:     Joel Fernandes <joel@joelfernandes.org>,
        "rcu@vger.kernel.org" <rcu@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kernel-team@meta.com" <kernel-team@meta.com>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>
Subject: Re: [PATCH rcu 3/6] rcu/rcuscale: Move rcu_scale_*() after
 kfree_scale_cleanup()
Message-ID: <527de9fa-1ec6-4ba7-8bce-b0447e751c07@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <IA1PR11MB6171B1AD7716B95B0B2C683889759@IA1PR11MB6171.namprd11.prod.outlook.com>
 <30F06C03-6950-4E2B-B3CE-3939B3CDD295@joelfernandes.org>
 <4d508096-300c-4d16-9c39-18598d00b500@paulmck-laptop>
 <IA1PR11MB6171866D6AEE79DD3413157E897A9@IA1PR11MB6171.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <IA1PR11MB6171866D6AEE79DD3413157E897A9@IA1PR11MB6171.namprd11.prod.outlook.com>
Precedence: bulk

On Sat, May 13, 2023 at 09:52:46AM +0000, Zhuo, Qiuxu wrote:
> > From: Paul E. McKenney <paulmck@kernel.org>
> > ...
> > > >>>> I wish diff was better at showing what really changed. The meld
> > > >>>> tool can help but its gui...
> > > >>>>
> > > >>>> Should I run meld later (I'm out at a conference so no access to
> > > >>>> meld-capable
> > > >>>> machines) or are we sufficiently confident that the lines were
> > > >>>> moved as-is ? :)
> > > >>>>
> > > >>>
> > > >>> Thank you, Joel for this concern. Good to know the meld diff GUI tool.
> > > >>> I just run the command below and confirmed that the lines were
> > > >>> moved
> > > >>> as-is: rcu_scale_{cleanup,shutdown}() follows kfree_scale_cleanup().
> > > >>> You may double check it ;-).
> > > >>>
> > > >>>      meld --diff ./rcuscale.c.before ./rcuscale.c.after
> > > >>
> > > >> Nice, thank you both!
> > > >>
> > > >> Another option is to check out the commit corresponding to this
> > > >> patch, then do "git blame -M kernel/rcu/rcuscale.c".  Given a
> > > >> move-only commit, there should be no line tagged with this commit's
> > SHA-1.
> > > >
> > > > Just had a good experiment with the "git blame -M" option:
> > > > - Used this option to prove a move-only commit quickly (no line tagged
> > with that commit) (the fastest method to me).
> > > > - Then just only needed to quickly check the positions of the moved code
> > chunk by myself (easy).
> > > >
> > > > Thank you, Paul for sharing this. It's very useful to me.
> > >
> > > Looks good to me as well and thank you both for sharing the tips.
> > 
> > Here is one way to script this, where "SHA" identifies the commit to be
> > checked and PATHS the affected pathnames:
> > 
> > 	git checkout SHA^
> > 	git show SHA | git apply -
> > 	git blame -M PATHS | grep '^0* '
> 
> Cool ~. Thank you, Paul.  
> I took them and made them into a script below for future use ;-)

Nice!!!

> #!/bin/bash
> 
> SHA=$1
> 
> if [ -z "$SHA" ]; then
>     echo "Usage: $0 <commit-id>"
>     exit 1
> fi
> 
> if ! git cat-file -t "$SHA" &> /dev/null; then
>         echo "$SHA does not exist in the repository"
>         exit 1
> fi

You might want to record the current position so that you can return
to it automatically.  One approach is to parse the output of
"git status".

> git checkout ${SHA}^ &> /dev/null
> git show ${SHA} | git apply - &> /dev/null
> 
> PATHS=`git status| grep "modified:" | cut -d: -f2 | xargs`

The '--porcelain' argument makes 'git status' is a bit easier to parse
robustly.

> for P in ${PATHS}; do
>         R=`git blame -M $P | grep '^0* '`

You can avoid any bash-variable length limitations by using
'grep -q' and capturing the exit status using "$?".

							Thanx, Paul

>         if test -n "$R"; then
>                 echo "$SHA is NOT a move-only commit"
>                 exit 1
>         fi
> done
> 
> echo "$SHA is a move-only commit"
> 
> > If there is no output, there were no non-move changes.
> > 
> > Or just do the "git blame -M PATHS | grep '^0* '" before doing the checking.
> > 
> > And yes, you can derive PATHS using "git status" if you want.  ;-)
> > 							Thanx, Paul