On 18/11/19 01:02PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > Pratyush Yadav <me@xxxxxxxxxxxxxxxxx> writes:
> >
> >> I am working on a simple little feature which shows the "amended
> >> content" when running 'git-commit -v'. Currently, only the changes in
> >> the _entire_ commit are shown. In a large commit, it is difficult to
> >> spot a line or two that were amended. So, show just the amended content
> >> in a different section.
> >
> > [jc: even though the diff generation is done before the final commit
> > is made, let me refer to the commits with refs _after_ the amend is
> > done].
> >
> > You want to show changes between HEAD@{1}..HEAD (which is what the
> > "amend" did) in addition to changes between HEAD^..HEAD (which is
> > what the "amended commit" does) separately.
Yes, that is what the patch does. It shows _both_ the entire diff and
the "amended diff".
> > The reason why "git commit -v" lets you see the diff since HEAD^ is
> > to help you write the commit log message. So it is wrong to show
> > only "what the amend did", as the message you would be writing while
> > amending is to explain the entire "why the amended commit does what
> > it does" and by definition the log message for "amend" should not
> > talk about "why the amend did what it did"---the readers would not
> > even have access to the older version before the amend.
> >
> > It too makes quite a lot of sense to allow readers to see what the
> > 'amend' did, but that is not something that would help write the log
> > message.
It would help _amend_ the log message though. This is the use-case which
motivated me to write this patch. When I make some changes to a commit
(like when re-rolling), I often want to update the commit message too.
If the commit content is a lot, then it becomes difficult to easily see
what exactly I changed, and in turn makes it difficult to quickly spot
what parts of the log message need updating.
> > And that is why "git commit -v --amend" does not show it.
> > It should be inspected even _before_ the user contemplates to run
> > "git commit --amend" (e.g. "git diff HEAD" before starting to amend).
> >
> > So, I am not enthused with this change---it sends a wrong message
> > (i.e. what the diff in the editor "commit -v" gives the user for).
>
> Having said that, I also wonder two things. Assuming that it may be
> a good idea to show "what the amend does" in addition to "what the
> amended commit does",
> 1. would it make sense to show a combined diff to show the
> differences among the state being recorded in the amended commit
> as if it were a merge between the state in the original commit
> and the state in the parent commit?
I'm afraid I don't follow what exactly this would do, and how it would
help differentiate between the "what the amend does" and "what the
amended commit does". Wouldn't the diff of a merge between the original
commit and the parent be exactly the diff (iow, the output of 'git
show') of the original commit, since the merge is a fast-forward?
> 2. would it make sense to show the differences between
> HEAD^..HEAD@{1} and between HEAD^..HEAD using the range-diff
> machinery.
I considered using range-diff, but didn't go with it because of my
personal dislike for range-diff. But if you strongly think that
range-diff is a better idea, then I can do that too.
> I think #1 may turn out to be more useful (I haven't tried it,
> though) because we already show a moral equivalent elsewhere, namely
> in "git stash show".
>
> Conceptually, it would be similar to showing a stash entry that
> records the state where some changes have been already added to the
> index and some other changes are still in the working tree---the
> base commit of such a stash entry corresponds to the parent commit
> of the commit being amended, the contents from the index of such a
> stash entry corresponds to the commit being amended, and the
> contents from the working tree of such a stash entry corresponds to
> the final contents you are trying to record as an amended commit.
>
--
Regards,
Pratyush Yadav
