kisslinux / kiss

KISS Linux - Package Manager
https://kisslinux.github.io
MIT License
464 stars 62 forks source link

`kiss update`: false exit on non-remote git repos #269

Closed FriendlyNeighborhoodShane closed 3 years ago

FriendlyNeighborhoodShane commented 3 years ago

Description

kiss update errors out on hitting a non-remote git repo, and thus terminates.

https://github.com/kisslinux/kiss/blob/fd8cfe1c698fb4d3d75c91b25a5bca60353af157/kiss#L1590 This line was probably intended to determine whether a repo has tracking remotes or not but it doesn't seem to work that way currently. On my testing across several platforms, git remote returns a status code of 0 even when there are no remotes (I do not have much information on the historical behavioir of that command at the moment). Perhaps it can be replaced by a test of its output being non-null?

Also, I have not tested it, but I'm fairly sure that this approach will also error out on repos that do have remotes, but are checked out on a branch that aren't tracking a remote. I have found some alternate approaches to this question I found online that might be more accurate here.

https://stackoverflow.com/questions/171550/find-out-which-remote-branch-a-local-branch-is-tracking

In particular: git for-each-ref --format='%(upstream:short)' "$(git symbolic-ref -q HEAD)" (output being non-null).

Considering all this, even after tackling these specific situations, should kiss just ignore the exit status of all git commands in pkg_update_git anyway? Even actual failure to update one repo is no reason to terminate the update process of all other repos and packages, IMO.

Also, welcome back, Dylan! Great to know you've been doing alright.

dylanaraps commented 3 years ago

Please try https://github.com/kisslinux/kiss/commit/60c32c5fa2f5f3a0fde9af9f8f02ecb033d077c7

Also, I have not tested it, but I'm fairly sure that this approach will also error out on repos that do have remotes, but are checked out on a branch that aren't tracking a remote. I have found some alternate approaches to this question I found online that might be more accurate here.

I don't think this should be an issue. Can you send steps to reproduce?

Considering all this, even after tackling these specific situations, should kiss just ignore the exit status of all git commands in pkg_update_git anyway? Even actual failure to update one repo is no reason to terminate the update process of all other repos and packages, IMO.

Failures should always be fatal here to prevent risk of partial updates. Besides, nothing stops you from manually updating each desired repository and running kiss U/kiss upgrade to then perform builds.

Also, welcome back, Dylan! Great to know you've been doing alright.

Thanks :)

dylanaraps commented 3 years ago

This is an alternative:

diff --git a/kiss b/kiss
index 0cf31cc..f7ef8c2 100755
--- a/kiss
+++ b/kiss
@@ -1587,7 +1587,7 @@ pkg_update() {

     # Update each repository in '$KISS_PATH'.
     for repo do
-        if ok "$(git -C "$repo" remote 2>/dev/null)"; then
+        if git -C "$repo" ls-remote --heads >/dev/null 2>&1; then
             repo_type=git

             # Get the Git repository root directory.
FriendlyNeighborhoodShane commented 3 years ago

Please try 60c32c5

Thanks! That worked!

Failures should always be fatal here to prevent risk of partial updates. Besides, nothing stops you from manually updating each desired repository and running kiss U/kiss upgrade to then perform builds.

That's fair.

I don't think this should be an issue. Can you send steps to reproduce?

Steps:

$ git clone https://github.com/kisslinux/repo
$ cd repo
$ git switch -c new
$ # make changes; commit them; (not actually required to see this error, just a thing people would do)
$ KISS_PATH=. kiss u

And you see kiss proceeds to try to make a pull, instead of skipping a repo that has no upstream for the checked-out branch. The fetch step happens, then the merge step fails.

(Note that we created an --orphan branch rather artificially just now, but there are tons of ways that you might end up with a branch that's not tracking an upstream e.g. playing around with commits/refs, manipulating history in unusual ways. You might even have encountered a git advice message when you tried to push a branch with no upstream tracking, which then tells you to configure one using push with the -u/--set-upstream-to option. Only for convenience does git automatically make new branches track an appropriate upstream when it can determine a trivial case.)

(I suddenly realised that git actually doesn't start tracking new branches automatically unless you have a specific configuration set up. It's trivial to get a repo setup with a branch that's not tracking a remote (and should not be until the user explicitly sets it up to). Steps updated accordingly.)

This is an alternative:

This seems to suffer similarly in the given situation. For reasons I can't find mentioned in the manual, git ls-remote seemingly lists from all remotes of the repo, and not just the upstream of the branch we're on. Plus it makes network requests, unnecesarily, when all of this information is perfectly local.

FriendlyNeighborhoodShane commented 3 years ago

The command I mentioned above handles all cases correctly because it specifically looks for the existence of an upstream for the HEAD branch. (or more precisely, for the branch that HEAD is a symref to.)

FriendlyNeighborhoodShane commented 3 years ago

For reasons I can't find mentioned in the manual, git ls-remote seemingly lists from all remotes of the repo, and not just the upstream of the branch we're on.

https://github.com/git/git/blob/d486ca60a51c9cb1fe068803c3f540724e95e83a/remote.c#L477-L487

This seems to be why. If ls-remote has no explicit remote name supplied to it, it calls this function (indirectly through its call to remote_get()). This function returns the remote being tracked by the current branch if there is a current branch and it's tracking a remote, and always returns origin otherwise. Which is probably not what we want.

dylanaraps commented 3 years ago

I think I have found the magic command needed. Let me know if the issue still occurs.

FriendlyNeighborhoodShane commented 3 years ago

Can confirm. Thanks!