From: Ted Pavlic Date: Wed, 21 Jan 2009 22:33:45 GMT Subject: Re: [PATCH] Added giteditor script to show diff while editing commit message. Message-ID: <4977A2C9.1070502@tedpavlic.com> In-Reply-To: Thanks for your comments. I've responded below. I just want to top-respond to your comment that the fundamental problem is that the diff is in a separate file. In fact, this is the point of the script. I want to be able to scroll through the diff output independent of the commit message. (alternatively, I realize I could do "git commit -v" and then use my editor's "split window" support, but that wouldn't help me with "stg edit") > the subject could use some work. For example, I would prefix it with ... >> From: Ted Pavlic > > As this is exactly what your email said in its header, it is redundant > information. Worse, it is information that made me look back to know why > it needs to be there. Distracting. I add --from to my gitsend alias to prevent git send-email from prompting me for a "From". Is there a way to have git send-email simply not prompt me for "From"? > What information does the README add that is not in the script itself? > If there is none, please refrain from adding the README to begin with. OK. I noticed plenty of other not-very-useful READMEs in contrib/, and so I figured it was a pro forma file. >> +# Find git >> +[ -z "${GIT}" ]&& GIT="git" > Yes, I know it is contrib/, but you may want to adopt Git's coding style > early. Ok. Switching to test. > Besides, I find it funny that you want to override git with $GIT. Isn't it possible that someone has git somewhere else? >> +# If we recognize a popular editor, add necessary flags >> +case "${EDITOR}" in >> + emacs) >> + EDITOR="${EDITOR} -nw" > > Mhm. Should this not be the user's choice? Some like emacs to start up > in a window. I don't use emacs, but it was my impression that the "no window" flag was added to make sure that emacs doesn't fork. That's why "-f" is used in the vim line. >> +# End GITTMP in ".git" so that "*.git/" syntax highlighting recognition >> +# doesn't break >> +GITTMP="${TMPDIR-/tmp}/giteditor.$RANDOM.$RANDOM.$RANDOM.$$.git" >> +(umask 077&& mkdir "${GITTMP}") || { >> + echo "Could not create temporary directory! Exiting." 1>&2 >> + exit 1 >> +} > > Umm. Why? Why do you need a temporary .git directory? The script generates a new "diff" file that I would rather drop elsewhere (e.g., in a /tmp directory) rather than here in the current directory. However, maybe you're right. After all, stg drops ".stgit-edit.txt" in the working directory. I suppose I could use gitdir, but I wasn't sure if it was safe to pollute gitdir. In the next version, I'll get rid of the temp directory and put the file here. >> + # Diff is non-empty, so edit msg and diff >> + ${EDITOR} "${GITTMP}/${COMMITMSG}" "${GITTMP}/diff" || exit $? > > vi users will hate you, as you do not give them a chance to edit the > message after having seen the diff. I don't see what you mean. I am a vi user (exclusively), and this script works very well for me. The "-f -o" flags above ensure that gvim will not fork. I'll add "vi" to the search string that automatically add "-f -o". Will that satisfy you? At the moment, giteditor works exactly like EDITOR (or VISUAL) for me, but it opens up a second buffer (split in the bottom window in my case) with the diff in it. I'm given the opportunity to save. > git commit will abort anyway if the commit message has not changed. Plus, > it does a better job, as it checks only the non-commented-out text. Okay. Using $1 exclusively. > BTW why on earth do you put every single variable name in curly brackets? I always thought that was good practice. It prevents ambiguity, and *I* don't think it's an eyesore. > Besides all that criticism, there is also a fundamental issue. The diff > is in a separate file. That's the point. If I wanted to put the diff in the commit buffer, I would have used "git commit -v". I think many would like to be able to scroll through the diff without having to scroll through the commit. Is there no value in having the diff in a separate file? Thanks -- Ted -- Ted Pavlic Please visit my ALS association page: http://web.alsa.org/goto/tedpavlic My family appreciates your support in the fight to defeat ALS.