Re: [PATCH 2/4] pull: use --quiet rather than 2>/dev/null
On Mar 20, 2010, at 8:35 AM, Jonathan Nieder wrote:
Show 16 quoted lines
> Benjamin C Meyer wrote:
>
>> - for reflog in $(git rev-list -g $remoteref 2>/dev/null)
>> + for reflog in $(git rev-list --quiet -g $remoteref)
>
> Are you sure? My local copy of git-rev-list.1 says
>
> --quiet
> Don’t print anything to standard output. This form is primarily meant
> to allow the caller to test the exit status to see if a range of
> objects is fully connected (or not). It is faster than redirecting
> stdout to /dev/null as the output does not have to be formatted.
>
> A similar question applies to the other patches in this series: are
> you sure they suppress the right output? (I haven’t checked, just
> asking.)
Thanks for reviewing the change
Yah looks like I made a mistake there, rev-list --quiet suppresses everything and not just stderr. When going through this janitor task I noticed that the --quiet is very inconsistent among git commands. Some suppress error messages, some suppress all messages. This might be something to improve in the future as even though I kept referring to the documentation this error slipped in.
re-checking the other patches I think they are correct in their usage. I ran the matching tests, the rebase ones passed, 3903-stash.sh is already failing before my patch and t3904-stash-patch.sh continues passing with the patch. Other then running the tests in t/ is there anything I should do to verify patches?
Show 22 quoted lines
> Aside: that for loop looks like it could be improved. Maybe it is worth
> factoring this into a separate function, something like:
>
> old_upstream() {
> remoteref=$1 &&
> curr_branch=$2 &&
>
> oldremoteref="$(git rev-parse -q --verify "$remoteref")" &&
> { git rev-list -g "$remoteref" 2>/dev/null || return $?; } |
> while read reflog
> do
> if test -z "$(git rev-list "$curr_branch".."$reflog" | head -n 1)"
> then
> printf "%s\n" "$reflog"
> return 0
> fi
> done &&
> printf "%s\n" "$oldremoteref"
> }
>
> In other words, we can avoid walking the whole reflog before starting
> to look for an ancestor for the current branch.That looks like it would speed up that bit of code, but I am still figuring out the internals of git so I can't really comment on if this is the best solution etc.
-Benjamin Meyer