ingydotnet / git-subrepo

MIT License
3.23k stars 267 forks source link

Git subrepo push sometimes picks merge commits instead of real commits #431

Open mvz opened 4 years ago

mvz commented 4 years ago

When the history in the main repo is complex with lots of merges, I'm seeing changes being pushed to the subrepo's remote with a message from a merge commit instead of the real commit. Sometimes the merge commit is from a completely unrelated pull request.

I have created a test file that demonstrates at least part of the problem (although in this case, the message is related): push-with-merges.t

The problem seems to stem from git-subrepo's logic of skipping non-linear commits, i.e., this part of subrepo:branch:

# Only include the commit if it's a child of the previous commit
# This way we create a single path between $subrepo_parent..HEAD
if [[ -n "$ancestor" ]]; then
  local is_direct_child=$(git show -s --pretty=format:"%P" $commit | grep "$ancestor")
  o "is child: $is_direct_child"
  if [[ -z "$is_direct_child" ]]; then
    o "Ignore $commit, it's not in the selected path"
    continue
  fi
fi
admorgan commented 4 years ago

Great find. I am not sure how long it will take to find a solution to this.

mvz commented 4 years ago

I've been experimenting with a re-implementation of git-subrepo in Ruby (and learning the hard way about all the difficulties that have probably already been encountered in git-subrepo itself).

In particular, I've been trying some alternative implementations of the push command, and one thing I found useful while creating the branch to be pushed is keeping a map between commits in the main repo and the branch-to-be-pushed.

Unfortunately, my knowledge of bash programming is too limited to even begin trying such a solution here as a way to fix this bug. Does bashplus come with some hash implementation?

mvz commented 4 years ago

If extended this test so it checks the full log graph of the main and upstream repos: https://github.com/mvz/git-subrepo/blob/push-fix/test/push-with-merges.t

This makes it easier to see where things go wrong.

admorgan commented 4 years ago

Just want you to know that this is on my radar, just don't have a ETA to look at it yet.