scolladon / sfdx-git-delta

Generate the sfdx content in source format from two git commits
Other
428 stars 115 forks source link

Project Submodule Support #729

Closed Russman12 closed 5 months ago

Russman12 commented 9 months ago

Is your proposal related to a problem?

no


SGD executions don't include changes to submodules.

Describe a solution you propose


When a submodule is updated, the process should include diffs from submodules within the project. The following git command achieves this: git diff sha1 sha2 --submodule=diff

There could also be a new flag to go with this functionality. something like --include-submodules. This would preserve the existing functionality and allow users to opt in. However, I'm not sure if there's actually a need to not diff submodules.

Describe alternatives you've considered


I've tried setting my git config to automatically diff submodules. I confirmed that it was working when running git diff, but that did not affect the SGD commands and I was unable to produce a manifest with the changed submodule files. Another potential solution would be to make the diff command respect the git config. If the project is using git to determine the diffs, I'm not exactly sure why the git config wouldn't be respected.

Additional context


This pull request seems to relate to submodules. After reading it I thought that this functionality was already supported. Perhaps I am doing something wrong?

Russman12 commented 9 months ago

I did some investigating and I think this may be harder to implement than I had initially hoped. I am having trouble coming up with a git command which simply lists the changed files in the base project as well as submodules. Running git diff commit1 commit2 --numstat --submodule=diff only prints changes to base files, and only has 1 entry for the submodule as a whole.

There's 2 solutions I've come up with that I think would work:

  1. Remove --numstat flag and add --submodule=diff flag in the diff command. This would require changes to how the diff results are parsed as it would have to extract file names from the complete diff to determine changed files. The considerations with this approach are the following:

    • Diff command results will include a ton of fluff.
    • The logic which parses the diff result would need to be made more complicated
  2. Compare the old submodule commit sha and the new one. This can be done with a command like git diff commit1 commit2 --submodule=short. Once the submodule commit shas are determined, the function would have to cd into the submodule directory, then run another diff command with the 2 commit shas and add the resulting files to the list of changed files. However, this has several problems:

    • There would be more commands executed (another diff for each submodule).
    • This process would have to be recursive to accommodate a submodule which contains submodules (git diff commit1 commit2 --submodule=diff already handles this recursion concern).
    • File names within the submodule start at the submodule root, so there's additional logic necessary to determine to full path.
    • Seems like this would add quite a bit more complexity to the project.

I think the best approach is solution 1. I don't think this is an ideal solution, but I think this would be the easiest and most reliable way to add this functionality.

scolladon commented 9 months ago

Hi @Russman12 !

Thanks for raising this feature request and thanks for contributing in making this project better!

Thank you very much for all the interesting spiking you've done so far, very helpful.

Indeed this PR allow sgd to work on submodules. But it does not provide the capability to introspect submodules as well to detect changes inside. It just makes the plugin works if the repository folder is a submodule (--repo parameter).

For the solution 1/, we are using --numstat because it works with --ignore-all-space --ignore-blank-lines --ignore-cr-at-eol --word-diff-regex=|[^[:space:]] params whereas it does not with --name-status. cf this PR

For the solution 2/ it would work but it is indeed a complex solution to build and maintain.

We are currently under a refactoring around how we interact with git. The goal is to use a library to interact with git and not use our own code to do it.

I'll try to add this feature inside the refactoring.

Stay tuned !

scolladon commented 8 months ago

Hi @Russman12

I'm giving you an update as I'm currently spiking on this feature.

So far, I think it would be doable to include submodules in the git refactoring in order to get the list of changes. It would add a bit of complexity but I think it would be feasible. I still need a bit of spiking to be 100% sure.

What I'm not sure yet is for the --generate-delta feature because for each changed lines we need to get the content of the file at the git revision --to to write the file in the --output. We also have more complex use case where we need to also have the file at the git revision --from to compare them both, but if we manage to get it for the --to we will be able to do it for the --from. This is not simple with the current architecture, and would require a lot of complex work to build and maintain.

Plus, it seems all this thinking would not work with subTree.

I'm considering another approach in parallel: documenting a way to script multiple calls to SGD, one for the main repository and one for each submodules of the main repository and find a way to reconciliate it in the output. Meaning each call to SGD will have the same output and the package.xml / destructiveChanges.xml will be merged at the end via scripting. We could document this approach in the README.md and/or in a Show/Tell article. Have you already considered this approach @Russman12 ? Would it work ?

Russman12 commented 8 months ago

Hey @scolladon,

Thanks for the update. I had not considered documenting a script with merging functionality. I think it could work, though the script could get complicated. Additionally, if the script is written in bash, then it might not be very portable. For example, I run git bash on windows, and I can't run jq commands. I'm not sure if a similar limitation exists for a bash script that modifies xml.

scolladon commented 7 months ago

Hi @Russman12 !

I found it pretty hard to know how to compare from a submodule because the program needs to have the sha the submodule were pointing to at the --from sha of the main repository.

I come up with this little bash script to handle submodules (not well tested):

#!/bin/bash

# store the output folder path (or default it to output)
output="${1:-output}"
# store the repo path
repo="${2:-.}"
from="${3:-HEAD~1}"
to="${4:-HEAD}"
current_sha=`git rev-parse HEAD`

pushd $repo
submodules_list=`git config --file .gitmodules --get-regexp path | awk '{ print $2 }'`
submodules=(${submodules_list// / })

main () {
    # current repo use case
    generate_delta_and_merge "." "$from" "$to"
    git submodule status | while read submodules_list
    do 
        handle_submodule $submodules_list
    done
    cleanup
}

generate_delta_and_merge () {
    local repo=$1
    local from=$2
    local to="${3:-HEAD}"
    sfdx sgd:source:delta -d -f $from -t $to -o $output -r $repo
    merge-manifest "package"
    merge-manifest "destructiveChanges"
}

handle_submodule () {
    local su_to=$1
    local su_path=$2

    git checkout -f $from
    pushd $su_path
    local su_from=`git rev-parse HEAD`
    popd
    git checkout -f $current_sha

    # submodules use case
    if [ "$su_from" != "$su_to" ]; then
        generate_delta_and_merge "." "$su_from" "$su_to"
    fi
}

cleanup () {
    rm -rf $output/package/merged-*.xml
    rm -rf $output/destructiveChanges/merged-*.xml
}

merge-manifest () {
    smp -p $output/$1/** -o "$output/$1/merged-$1.xml"
}

main
popd

It uses smp, a small a fast package.xml merge tool

I think it would be easy to make it works for subtree as well

Does it helps you ?

Russman12 commented 7 months ago

Hello @scolladon,

Thanks for the script! I will test this as soon as I can and report back.

Thanks,

Russman12 commented 5 months ago

Hey @scolladon,

Sorry it has taken so long for me to get back with this. I have not been able to test the bash script yet, but the concept seems pretty simple. I will close this issue.

Thanks,