Re: [PATCH] Added giteditor script to show diff while editing commit message.

[Ted Pavlic <ted@tedpavlic.com>]

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<ted@tedpavlic.com>
>
> 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 <ted@tedpavlic.com>

   Please visit my ALS association page:
         http://web.alsa.org/goto/tedpavlic
   My family appreciates your support in the fight to defeat ALS.