This repository has been archived by the owner. It is now read-only.

update: use GitHub API to avoid unneeded fetches. #49219

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@MikeMcQuaid
Copy link
Member

MikeMcQuaid commented Feb 16, 2016

Check to see if HEAD is the same as what we have locally. If it is: don't bother to git fetch.

Note that this is a preview GitHub API so it may change in future.

CC @xu-cheng @UniqMartin

-H "Accept: application/vnd.github.chitauri-preview+sha" \
-H "If-None-Match: \"$UPSTREAM_BRANCH_LOCAL_SHA\"" \
"https://api.github.com/repos/$UPSTREAM_REPOSITORY/commits/$UPSTREAM_BRANCH")"
fi

This comment has been minimized.

@xu-cheng

xu-cheng Feb 16, 2016

Contributor

It could improve the code style by early return here. i.e

if [[ "$UPSTREAM_REPOSITORY_URL" = "https://github.com/"* ]]
then
  ...
  [[ "$UPSTREAM_SHA_HTTP_CODE" = "304" ]] && return
fi

As result, we won't have the case where UPSTREAM_SHA_HTTP_CODE never being set, which looks weird.

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 16, 2016

Member

🆒 but think it's exit as it's a subshell. Will try both,though.

if [[ "$UPSTREAM_REPOSITORY_URL" = "https://github.com/"* ]]
then
UPSTREAM_REPOSITORY="$(echo "$UPSTREAM_REPOSITORY_URL" | \
sed -e "s|^https://github.com/||" -e "s|.git$||")"

This comment has been minimized.

@MikeMcQuaid

MikeMcQuaid Feb 16, 2016

Member

@UniqMartin can you suggest the magic Bash way of doing this?

This comment has been minimized.

@UniqMartin

UniqMartin Feb 17, 2016

Contributor

The Bash way of doing this would be to remove the https://github.com/ prefix and then to remove the .git suffix. The parameter substitution used for this cannot be nested, so the code becomes slightly more repetitive as it requires two separate statements:

UPSTREAM_REPOSITORY="${UPSTREAM_REPOSITORY_URL#https://github.com/}"
UPSTREAM_REPOSITORY="${UPSTREAM_REPOSITORY%.git}"
@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Feb 17, 2016

No objection to the general approach, but here's a random thought: Would it make sense to move everything that doesn't deal with the network (namely checking/processing the upstream repository URL) outside of the parentheses and thus out of the subshell, such that only the curl and git fetch (with some minimal logic around them) remain? I guess one upside could be that it is easier to debug, if there's ever something to debug in this part of the code.

@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 17, 2016

Would it make sense to move everything that doesn't deal with the network (namely checking/processing the upstream repository URL) outside of the parentheses and thus out of the subshell, such that only the curl and git fetch (with some minimal logic around them) remain?

Good idea. None of that stuff seems like a performance bottle-neck. 👍 to using parallel processing only to speed things up 👍

update: use GitHub API to avoid unneeded fetches.
Check to see if `HEAD` is the same as what we have locally. If it is:
don't bother to `git fetch`.

Closes #47888.
@MikeMcQuaid

This comment has been minimized.

Copy link
Member

MikeMcQuaid commented Feb 18, 2016

No objection to the general approach, but here's a random thought: Would it make sense to move everything that doesn't deal with the network (namely checking/processing the upstream repository URL) outside of the parentheses and thus out of the subshell, such that only the curl and git fetch (with some minimal logic around them) remain?

Decided against this just due to Bash's global state + multithreading = 😭

Thanks for the review, pulling!

@MikeMcQuaid MikeMcQuaid deleted the MikeMcQuaid:update-bash-curl-api branch Feb 18, 2016

@UniqMartin

This comment has been minimized.

Copy link
Contributor

UniqMartin commented Feb 18, 2016

Decided against this just due to Bash's global state + multithreading = 😭

As far as I understand this, this shouldn't be an issue because the parallelism is entirely process- and not thread-based. Thus once the variables have been set up, the subshell gets created and gets its own copy of the variables from the parent shell. Whatever the subshell does to those variables doesn't influence the parent shell and vice versa.

That said, the change is good as is and we can iterate on it later, maybe once another change needs to touch this code and pulling out those variables creates some kind of benefit.

Thanks for the review, pulling!

👏

I'm looking forward to the reports that claim brew update is broken because it finished too quickly. 😏

@Homebrew Homebrew locked and limited conversation to collaborators Jul 10, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.