Fix remote & branch being overwritten in .gitrepo without --update flag set

[neko-kai]

The bug was that in this code:

if $newfile || [[ $update_wanted && $override_remote ]]; then

Command if [[ $update_wanted ]] will always succeed, even though update_wanted is false, because [[ only checks that the string is not empty. Unfortunately, the tests didn't catch the fact that remote/branch were always updated due to this, so I've added some minimal checks that would fail before the fix.

cc @admorgan I've set the base to release/0.4.5 as you asked of the other pending PRs, in case this could help release the fix sooner.

[neko-kai]

@ingydotnet @admorgan Pinging in case there's a chance you may look at this 🙏

[admorgan]

I am in the middle of a big release, but looking at this is certainly in on my TODO list.

[admorgan]

There have been a few discussions about changing the branch and remote on a push/pull. My recollection is that @ingydotnet has expressed this is intentional.

Let's break this down. git subrepo pull, it seems disingenuous to have a .gitrepo file that says the code is from a remote and branch that distinctly isn't after doing a git pull -b . It is likely that the commit SHA won't even be on the branch the .gitrepo points to and therefore future git subrepo actions will fail for good reason, and be very hard to diagnose.

git subrepo push, I personally don't use push because many of the projects I use git subrepo with require the patch to go through change-control, or review that change the patch (sign-off trailers if nothing else) before it is added to the remove repository. Therefore any git push to a project that uses gerrit will never work as expected. I also see an issue because push creates a commit that "finalizes" the syncing process, but again the branch and/or remote will not represent that sync.

I like patches 3e17a77 and 6b4a4e4, but not forcing forcing --update when doing a push or pull would require more work in my opinion.

[neko-kai]

@admorgan
The main use case I have for this change is to allow contributors to use git subrepo pull with a git protocol remote instead of a default https remote, without having to revert the subsequent remote change in the .gitrepo file.
For this use case there's no danger of pointing to a missing commit since it's pulling from the same remote, only with a different protocol.

I'm not against trying another way to achieve the above use case, if the current behavior is deemed to be more correct. Instead of mimicking default git behavior with an opt-in --update, it would be fine to have an opt-out --no-update flag instead. As it is, the update flag is misleading since it's always applied. Specifying update as the default behavior with an explicit --no-update flag may be more usable, since the user would have to explicitly opt-in to enable a behavior that may break their subrepo if a pulled remote/branch is not related to the original remote/branch.

[admorgan]

I agree with your conclusion. a --no-update flag makes more sense in this case. I would accept such a patch.