On Mon, Oct 21, 2024 at 02:28:05PM -0400, Eric Sunshine wrote: > On Mon, Oct 21, 2024 at 12:53 PM Taylor Blau <me@xxxxxxxxxxxx> wrote: > > On Mon, Oct 21, 2024 at 02:38:15PM +0000, Samuel Adekunle Abraham via GitGitGadget wrote: > > > + MSG="Edited notes message" git notes add -m "Initial notes message" -e && > > > + echo "Edited notes message" >expect && > > > > Very nice use of the fake_editor script here. > > > > It is a little cumbersome to repeat the same message in MSG= and when > > populating the 'expect' file. Perhaps instead this could be written as: > > > > echo "edited notes message" >expect && > > MSG="$(cat expect)" git notes -add -m "initial" -e > > This suggested rewrite feels unusually roundabout and increases > cognitive load for readers who now have to trace the message flow from > script to file and back into script, and to consider how the loss of > trailing newline caused by use of $(...) impacts the behavior. It also > wastes a subprocess (which can be expensive on some platforms, such as > Windows). If we're really concerned about this minor duplication of > the message, we can instead do this: > > msg="edited notes message" && > echo "$msg" >expect && > MSG="$msg" git notes -add -m "initial" -e I am not sure I agree that the suggested version is clearer. The way I read mine is that we are writing what we expect to see in "expect", and then setting up MSG to be the same thing. I definitely do not feel strongly between the two and would rather avoid nitpicking something as trivial as this when compared to the rest of the patch, especially considering that I would be equally happy with your version. I think the whole thing is a little bit of a moot point, though, because by far the thing that is least clear here is that setting MSG has the side-effect of modifying the behavior of the fake-editor that we set up earlier in the script. So I don't know that optimizing for clarity in how we setup and write to the "expect" file is the right thing to spend our time on. Thanks, Taylor
