Re: [PATCH] Support long format for log-based submodule diff

[Robert Dailey <rcdailey.lists@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5704 bytes --]

On Tue, Mar 27, 2018 at 5:17 PM, Stefan Beller <sbeller@google.com> wrote:
>> >> $ git diff --submodule=log --submodule-log-detail=(long|short)
>> >>
>> >> I'm not sure what makes sense here. I welcome thoughts/discussion and
>> >> will provide follow-up patches.
>> >
>> > The case of merges is usually configured with --[no-]merges, or
>> > --min-parents=<n>.
>
>> But that is a knob that controls an irrelevant aspect of the detail
>> in the context of this discussion, isn't it?  This code is about "to
>> what degree the things that happened between two submodule commits
>> in an adjacent pair of commits in the superproject are summarized?"
>
> And I took it a step further and wanted to give a general solution, which
> allows giving any option that the diff machinery accepts to only apply
> to the submodule diffing part of the current diff.
>
>> The hack Robert illustrates below is to change it to stop favouring
>> such projects with "clean" histories, and show "log --oneline
>> --no-merges --left-right".  When presented that way, clean histories
>> of topic-branch based projects will suffer by losing conciseness,
>> but clean histories of totally linear projects will still be shown
>> the same way, and messy history that sometimes merges, sometimes
>> merges mergy histories, and sometimes directly builds on the trunk
>> will be shown as an enumeration of individual commits in a flat way
>> by ignoring merges and not restricting the traversal to the first
>> parent chains, which would appear more uniform than what the current
>> code shows.
>
> Oh, I realize this is in the *summary* code path, I was thinking about the
> show_submodule_inline_diff, which would benefit from more diff options.
>
>> I do not see a point in introducing --min/max-parents as a knob to
>> control how the history is summarized.
>
> For a summary a flat list of commits may be fine, ignoring
> (ideally non-evil) merges.
>
>> This is a strongly related tangent, but I wonder if we can and/or
>> want to share more code with the codepath that prepares the log
>> message for a merge.  It summarizes what happened on the side branch
>> since it forked from the history it is joining back to (I think it
>> is merge.c::shortlog() that computes this)
>
> I do not find code there. To me it looks like builtin/fmt-merge-msg.c
> is responsible for coming up with a default merge message?
> In that file there is a shortlog() function, which walks revisions
> and puts together the subject lines of commits.
>
>> and it is quite similar
>> to what Robert wants to use for submodules here.  On the other hand,
>> in a project _without_ submodule, if you are pulling history made by
>> your lieutenant whose history is full of linear merges of topic
>> branches to the mainline, it may not be a bad idea to allow
>> fmt-merge-msg to alternatively show something similar to the "diff
>> --submodule=log" gives us, i.e. summarize the history of the side
>> branch being merged by just listing the commits on the first-parent
>> chain.  So I sense some opportunity for cross pollination here.
>
> The cross pollination that I sense is the desire in both cases to freely
> specify the format as it may depend on the workflow.

First I want to apologize for having taken so long to get back with
each of you about this. I actually have a lot of work started to
expand the --submodule option to add a "full-log" option in addition
to the existing "log". This is a pretty big task for me already,
mostly because I'm unfamiliar with git and have limited personal time
to do this at home (this is part of what I am apologizing for). I kind
of get what Stefan and Junio are saying. There's a lot of opportunity
for cleanup. More specific to my use case, adding some functionality
to generate a log message (although I've developed a bash script to do
this since I wrote my original email. I'll attach it to this email for
those interested). Also I get that taking this a notch higher and
adding a new option to pass options down to submodules also addresses
my case. Before I waste anyone's time on this, I want to make sure
that my very narrow and specific implementation will be ideal. By all
means I do not want to do things the easy way which ends up adding
"cruft" you'll have to deal with later. If there's a larger effort to
generalize this and other things related to submodules maybe I can
just wait for that to happen instead? What direction would you guys
recommend?

Junio basically hit the nail on the head with the comparisons of
different mainlines. I think some repositories are more disciplined
than others. At my workplace, I deal with a lot of folks that aren't
interested in learning git beyond the required day to day
responsibilities. It's difficult to enforce very specific branching,
rebase, and merge habits. As such, the best I can do to work around
that for building release notes is to exclude merge commits (since
most of the time, people keep the default message which is generally
useless) and include all commits in the ancestry path (since often
times commits on the right side of a merge will have important
information such as JIRA issue keys, which if shown in the parent repo
will cause appropriate links back to parent repositories to show when
changes in submodules were introduced there as well).

Based on how constructive this email thread has gotten since I started
it, I'm starting to feel like my solution is too narrowly-focused and
doesn't have the long term appeal expected. Let me know, I'm happy to
do what I can but I think it will be limited due to my lack of domain
expertise in the code base and inability to invest the required time
for significant scope of work.

[-- Attachment #2: git-smcommit --]
[-- Type: application/octet-stream, Size: 1609 bytes --]

#!/usr/bin/env bash
#smcommit = "!f() { git commit -m \"$(printf \"Updated $1 Submodule\n\n\"; git diff \"$1\")\" --edit -- $1; }; f"

submod="${1%/}"

# Check if the specified submodule exists. If it doesn't, this will have a non-zero return code.
# This will cause the script to exit. The "./" in front allows relative paths to the submodule
# directories to be used from any directory
last_sha1=$(git rev-parse ":./$submod" 2> /dev/null)
if [[ $? != 0 ]]; then
    echo "ERROR: The specified submodule does not exist"
    exit 1
fi

# If the submodule has not physically changed (i.e. pointing to a different SHA1) then we don't
# care.
diff_result=$(git diff --submodule=short --ignore-submodules=dirty -- "$submod")
if [[ -z "$diff_result" ]]; then
    echo "ERROR: That submodule is already up to date"
    exit 1
fi

cd "$1"

getChangelogs() {
    git --no-pager log --oneline --no-decorate --no-merges $1..$2 | sed "s/^/    /g"
}

commits_forward=$(getChangelogs $last_sha1 HEAD)
commits_backward=$(getChangelogs HEAD $last_sha1)

cd - > /dev/null

buildLogSection() {
    # $1: The header message for the group of logs
    # $2: The string of logs to add
    if [[ ! -z "$2" ]]; then
        echo -e "$1\n\n$2"
    fi
}

introduced_section=$(buildLogSection "Introduced Commits:" "$commits_forward")
rewound_section=$(buildLogSection "Rewound Commits:" "$commits_backward")

if [ "$commits_forward" ]; then addspace=$'\n\n'; fi

read -r -d '' commit_message << EOF
Updated $submod Submodule

${introduced_section}${addspace}${rewound_section}
EOF

git commit -m "$commit_message" --edit -- "$submod"