Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update.sh: improve verbose output and readability. #486

Merged
merged 1 commit into from
Jul 12, 2016
Merged

update.sh: improve verbose output and readability. #486

merged 1 commit into from
Jul 12, 2016

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Jul 10, 2016

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew tests with your changes locally?

Would have made it easier to debug Homebrew/homebrew-core#2804 where wasn't clear which directory was causing the issue.

@BrewTestBot BrewTestBot added the in progress Maintainers are working on this label Jul 10, 2016
git rebase "${QUIET_ARGS[@]}" "origin/$UPSTREAM_BRANCH"
else
if [[ -n "$HOMEBREW_VERBOSE" ]]
then
echo "Merging origin/$UPSTREAM_BRANCH into $DIR..."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tiniest nit, but there's a missing space between $DIR and ... which sticks out because it is present in the other message you added.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably rather remove the space in the other location if that's 🆒?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with either solution as long as it's consistent and I'm not 100% sure about typographical conventions about this in the English language. Still I'm a tiny bit bothered that the ... could be misinterpreted to be a part of the directory (as silly as that might sound).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typographically (not ...) without space before. Yet…

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, in normal text I always use “…” (it's even directly accessible from my keyboard layout). It looks kinda weird in output on the command line though (squeezed into a single column) and can be problematic in source code, where we tend to stick to ASCII-only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, in "ASCII art" I would not use "LaTeX quality" and add a space before ... in this case, in order to avoid the misinterpretation you pointed out.

@UniqMartin
Copy link
Contributor

While I'm generally in favor of this PR, I think in its current state it might contribute to the confusion when output related to <repo2> will appear under a heading that suggests it's still operating on <repo1>. It would be really nice if all verbose Git output related to a given directory/repository was grouped under a common heading that indicates this directory/repository.

To address this, I'd suggest to either move the added verbose output to the very top of the pull function or to add additional verbose output there. (Something like Updating $DIR ... will probably do.)

(I once proposed something similar in Homebrew/legacy-homebrew#49905 but I guess it had a bad start as it was originally disabling parallelization—for the sake of clearer debug output—and then was blocked by discussion about the structure of the output and implementation details of ohai et al. in Bash, that I also made part of the PR. Just wanted to mention this for some context and prior discussion.)

@MikeMcQuaid
Copy link
Member Author

@UniqMartin Take a look now; have changed the approach somewhat.

@MikeMcQuaid MikeMcQuaid changed the title update.sh: verbosely output rebase/merge messages. update.sh: improve verbose output and readability. Jul 12, 2016
@@ -249,6 +252,8 @@ pull() {
pop_stash_message
fi

[[ -n "$HOMEBREW_VERBOSE" ]] && echo
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can omit this here and instead move the echo to the top of the pull function, before Updating $DIR... is output, which simplifies things a tiny bit and will also create a visual separation between the fetch phase and the rebase/merge phase.

But you could still add this line after the loop over all repositories in homebrew-update to create some visual separation between the rebase/merge phase and the output of update-report.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fetch doesn't echo every time; put it at the beginning of this line and you will sometimes see the first line of output as an empty line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I didn't think about this case, probably because I'm running brew update at most every few hours, so there's essentially always something that needs to be fetched. Feel free to leave things as-is or eliminate this line of code completely, whichever you prefer.

The important change is that the output now contains some extra lines that announce what repository brew update is currently operating on and that's valuable information. A few missing or superfluous empty lines for readability won't detract from that.

@UniqMartin
Copy link
Contributor

Thanks for making this change; I'm a lot happier with the output! In fact I'm 👍 on the change as-is. (There's one suggestion in a code comment you might want to integrate beforehand.)

Would have made it easier to debug
Homebrew/homebrew-core#2804 where wasn't clear which
directory was causing the issue.
@MikeMcQuaid MikeMcQuaid merged commit ed1d1e5 into Homebrew:master Jul 12, 2016
@MikeMcQuaid MikeMcQuaid deleted the update-rebase-merge-verbose branch July 12, 2016 18:45
@BrewTestBot BrewTestBot removed the in progress Maintainers are working on this label Jul 12, 2016
souvik1997 pushed a commit to souvik1997/brew that referenced this pull request Jul 25, 2016
Would have made it easier to debug
Homebrew/homebrew-core#2804 where wasn't clear which
directory was causing the issue.
@Homebrew Homebrew locked and limited conversation to collaborators May 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants