| *** rlandy|bbl is now known as rlandy|out | 01:10 | |
| *** dviroel|rover|afk is now known as dviroel|rover | 01:17 | |
| *** ysandeep|out is now known as ysandeep | 01:37 | |
| opendevreview | OpenStack Proposal Bot proposed openstack/project-config master: Normalize projects.yaml https://review.opendev.org/c/openstack/project-config/+/849909 | 02:25 |
|---|---|---|
| *** ysandeep is now known as ysandeep|afk | 03:45 | |
| *** ysandeep|afk is now known as ysandeep | 06:16 | |
| *** chandankumar is now known as chkumar|rover | 06:59 | |
| opendevreview | daniel.pawlik proposed openstack/ci-log-processing master: Push container images as latest tag after merge https://review.opendev.org/c/openstack/ci-log-processing/+/847880 | 07:04 |
| *** akekane_ is now known as abhishekk | 08:04 | |
| *** rlandy|out is now known as rlandy | 10:31 | |
| opendevreview | Merged openstack/project-config master: Normalize projects.yaml https://review.opendev.org/c/openstack/project-config/+/849909 | 11:22 |
| *** dviroel|rover is now known as dviroel | 11:28 | |
| *** ysandeep is now known as ysandeep|afk | 11:56 | |
| opendevreview | James Page proposed openstack/project-config master: Add OpenStack K8S charms https://review.opendev.org/c/openstack/project-config/+/849996 | 12:18 |
| opendevreview | James Page proposed openstack/project-config master: Add OpenStack K8S charms https://review.opendev.org/c/openstack/project-config/+/849996 | 12:19 |
| opendevreview | James Page proposed openstack/project-config master: Add OpenStack K8S charms https://review.opendev.org/c/openstack/project-config/+/849996 | 12:21 |
| *** dasm|off is now known as dasm|ruck | 13:33 | |
| *** ysandeep|afk is now known as ysandeep | 14:16 | |
| *** blarnath is now known as d34dh0r53 | 15:02 | |
| *** dviroel is now known as dviroel|lunch | 15:16 | |
| *** ysandeep is now known as ysandeep|out | 15:28 | |
| *** rlandy is now known as rlandy|brb | 15:42 | |
| *** rlandy|brb is now known as rlandy | 16:05 | |
| dansmith | fungi: clarkb: fwiw, I just pushed up another glance patch to a completely unrelated stack of patches and had to --no-thin again | 16:08 |
| dansmith | I was thinking that was just some transient thing about the specific tree I was working on, but I wonder if it's something now out of sync in the repo? | 16:09 |
| dansmith | or my copy maybe? | 16:09 |
| clarkb | dansmith: I believe it is a known problem with jgit. Its been an issue since liek 2012 | 16:09 |
| clarkb | the jgit maintainers apparently understand it but fixing it is difficult | 16:09 |
| clarkb | I had a thread with upstream gerrit about the problem a year or two ago aroudn when we added the override to git review | 16:09 |
| fungi | and yeah, it's likely nothing to do with your system, just luck of the draw between the state of the remote branch and your local branch | 16:10 |
| fungi | are both stacks currently based on the same remote branch tip? | 16:11 |
| fungi | that may increase the odds that one hits this if the other does | 16:11 |
| *** akekane_ is now known as ahishekk | 16:14 | |
| dansmith | fungi: meaning are they rebased on master? No, in both cases this has been me stacking a patch on top of another without rebasing the author's version | 16:16 |
| dansmith | clarkb: ack I know it's a known issue with no known solution, I was just saying I was surprised to hit it all of a sudden in two different unrelated patch stacks | 16:18 |
| clarkb | I think its mostly chance unless you're operating on the same dag structures as fungi points out | 16:19 |
| dansmith | I've just not hit this (that I know of before) and then twice in two days seems like long odds, but okay, just wanted to highlight it in case there's something up with glance's repo itself | 16:20 |
| clarkb | were both changes to glance? | 16:20 |
| clarkb | and were both based on the same parent? | 16:20 |
| dansmith | they were both to glance and no, they were totally unrelated | 16:22 |
| clarkb | ok, my hunch is that there is something with how jgit is currently internally modeling the history of glance and that it isn't entirely random as a result | 16:22 |
| dansmith | I mean I guess it's possible they were both rebased on master at the same time, but seems unlikely | 16:22 |
| clarkb | it is my understanding that this issue is largely harmless other than the user needing to push with --no-thin | 16:23 |
| dansmith | yeah, different parents for sure, just checked | 16:23 |
| dansmith | well, I know, but if something were to get stuck such that everyone has to use the flag for glance forever that would kinda suck | 16:24 |
| dansmith | especially because I'll just alias it locally for all my pushes :) | 16:24 |
| clarkb | I don't disagree but if the jgit experts have already said fixing this is super difficult for them I don't have a good answer for how I'd fix it | 16:24 |
| dansmith | yeah, not pushing for a fix, just pointing it out | 16:25 |
| clarkb | I just pushed https://review.opendev.org/c/openstack/glance/+/850028 based on glance master and it went through fine. I don't think this is a universal issue thankfully | 16:26 |
| dansmith | yeah, let me rebase that on one of the other parents | 16:26 |
| dansmith | hrm, I can't rebase onto a merge commit I guess | 16:29 |
| dansmith | bah, whatever | 16:30 |
| fungi | rebase via the gerrit webui, or locally? | 16:30 |
| dansmith | locally | 16:30 |
| fungi | if locally using git, you should be able to rebase onto a merge commit without issue | 16:30 |
| fungi | i usually do something like `git rebase -i origin/master` | 16:30 |
| fungi | (after remote update of course) | 16:31 |
| dansmith | I'm trying to rebase to the specific commit that is the parent of the one I was having trouble with, which isn't ending up the same | 16:32 |
| *** dviroel|lunch is now known as dviroel | 16:32 | |
| dansmith | but whatever, it's not a big deal, I'll just keep quiet. it just seemed like an incredible coincidence and wanted to point it out in case there's some corruption going on | 16:33 |
| fungi | ahh | 16:33 |
| fungi | and yes, it does seem like an odd coincidence, i agree | 16:33 |
| clarkb | re corruption it is my understanding (and this was based on upstream discussions and its been a year or two so maybe I've got it wrong) that your --no-thin would fail if corruption was the cause | 16:34 |
| clarkb | the --no-thin succeeding implies it is purely at the negotiation level and not the repo level that the failrue is occuring | 16:35 |
| dansmith | okay | 16:35 |
| fungi | also possible that if you repacked your local repo and then cherry-picked those commits onto a new topic branch the situation would be sufficiently different as to not result in the same exact push | 16:36 |
| fungi | since the cherry-picking would create new commit ids | 16:37 |
| dansmith | yeah, often what I'm trying to do is not disturb the patch underneath me so as not to create conflicts from someone who is also working on their copy of the base patch simulataneously | 16:37 |
| fungi | right | 16:38 |
| fungi | and also that's a lot of effort for what's effectively a known issue | 16:38 |
| fungi | just noting it's how we initially worked around the problem whenever it showed up, until the relationship to thin pushes was discovered | 16:39 |
| dansmith | is it likely to be less of an issue if you're not passing -R (no rebase)? | 16:40 |
| dansmith | because making the default --no-thin when -R is passed might be a nice optimization, which is definitely less frequent than regular review pushes | 16:40 |
| clarkb | -R is largely a noop in modern git-review now | 16:40 |
| dansmith | how so? if I don't use -R it rebases the base patch every time | 16:40 |
| dansmith | (I mean, assuming master has moved) | 16:41 |
| clarkb | dansmith: the way modern git-review (for like the last 5 years?) should work is it does a local test rebase but then resets to the original state if the rebase succeeds and pushes as is | 16:41 |
| fungi | -R tells git-review not to perform a test rebase. git-review doesn't keep the rebase | 16:41 |
| clarkb | if the rebase fails it bails out then and there and tells you you have a merge conflict you should deal with before pushing | 16:41 |
| dansmith | you mean it might not push the rebased version up, but still tries the local rebase right? | 16:41 |
| clarkb | dansmith: it should never push the rebased version up. It should only do a test one locally | 16:42 |
| fungi | right, without -R it attempts a rebase and then rolls it back whether it succeeds or fails | 16:42 |
| fungi | the attempted rebase is simply done so that it can warn you whether the change you're asking to propose has a merge conflict | 16:42 |
| dansmith | I'm pretty sure that's not the behavior I see, however it doesn't matter, because when I'm pushing on top of someone else's patch, I'm often doing it because I'm offering them some contribution that is going to iterate from their patch, and so a local rebase *will* hit conflicts quite often | 16:43 |
| dansmith | but I guess your point is that it shouldn't have any impact on the need for --no-thin right? | 16:43 |
| clarkb | dansmith: correct it should never affect that as it doesn't chagne what you are pushing | 16:43 |
| clarkb | basically -R should only be used if you want to push a change with a merge conflict | 16:43 |
| dansmith | I must be running some old version, because I've definitely had it push up a rebased base patch in the last year or so | 16:44 |
| clarkb | dansmith: ime people who run into that are using cherry picks and don't realize that a cherry pick is an auto rebase | 16:44 |
| dansmith | git-review version 2.2.0 | 16:44 |
| clarkb | cherry picking changes the timestamp which changes the commit sha and is a common way people get in trouble and then think it is related to git review rebeasing behavior from a decade ago (which it did do then) | 16:45 |
| fungi | looking in the git-review source, if rebase is enabled (not -R) and if force rebase is disabled (not -F) then the undo_rebase() function is called after rebasing | 16:49 |
| fungi | undo_rebase() runs `git reset --hard` to the original head from before the rebase | 16:50 |
| fungi | so if the test rebase sticks around (without passing -F) then that seems like a bug | 16:50 |
| dansmith | okay well maybe I'm misremembering or confusing multiple things that are going on along with historical calcified beliefs, I dunno | 16:51 |
| fungi | the undo_rebase() function itself was last touched in 2013, and the call to it in 2012, so in theory should have worked like that for the past 9-10 years | 16:52 |
| dansmith | when I'm augmenting other people | 16:52 |
| dansmith | people's patches I do a lot of review -R because I want to give them some stuff, but not rebase their patch either locally or remotely | 16:52 |
| dansmith | and both of the thin problems I had this week were on -R pushes, so I just thought maybe that was more likely to introduce problems with non-matching history, but sounds like not | 16:53 |
| fungi | sure. that makes sense. -R is specifically how you tell git-review "yes this may be in merge conflict with the target branch but go ahead and push it anyway" | 16:53 |
| fungi | the --help output from git-review could be a little more clear on that point though, it just says "don't rebase" but fails to explain that it wouldn't automatically push the rebase it's doing anyway | 16:54 |
| dansmith | right, sounds like that needs to be "Don't test for merge conflicts" or "Don't perform a test rebase" | 16:55 |
| fungi | the fact that -R doesn't perform the test rebase actually seems like more of an implementation detail, given the actual use case for it | 16:55 |
| fungi | since the goal is to allow you to push things even if they merge conflict, not specifically to avoid testing for a merge conflict | 16:56 |
| fungi | but if you don't care that there may be a merge conflict, then there's obviously no point in testing for one | 16:56 |
| dansmith | tbh, I really wish that -R was the default, I'm not really sure why it matters.. I assume it's to make sure that what I'm pushing up can be merged against master, but.. that should be my call, IMHO :) | 16:57 |
| fungi | well, can be merged to whatever branch you're pushing it for, but yes | 16:57 |
| dansmith | well, right | 16:57 |
| fungi | it's more to let less-experienced contributors know that they'll need to rebase their changes before they can be approved, but that's also something you can toggle in the [gitreview] section of your .git/config if it's annoying | 16:58 |
| dansmith | well, I'm used to it now, so it's not a big deal | 16:59 |
| fungi | i wouldn't expect new contributors to know to explicitly turn on an option that checks for merge conflicts, but we can probably do a much better job of letting seasoned contributors know where to turn it off | 16:59 |
| dansmith | well, and making it clear what -R actually (is supposed to) mean | 17:00 |
| fungi | yes, i'll have a patch up for that shortly | 17:00 |
| * dansmith checks his watch | 17:00 | |
| fungi | right after my union-mandated tea break, of course! ;) | 17:02 |
| dansmith | lolol | 17:03 |
| fungi | dansmith: https://review.opendev.org/850054 Clarify that test rebases are not kept | 17:29 |
| fungi | if you have time, let me know if those edits make sense | 17:29 |
| fungi | no rush, obviously | 17:29 |
| dansmith | done | 17:31 |
| fungi | maybe part of the confusion is that if git-review detects a merge conflict it leaves you in an incomplete rebase so you can fix your merge conflicts, rather than automatically aborting the rebase and expecting you to rerun the rebase yourself. i'm open to ideas on how to make that smoother | 17:31 |
| dansmith | I'm going to pay more attention for the next little while to see if I observe the behavior that I was sure still happens before I opine on that further | 17:33 |
| dansmith | perhaps I'm so muscle-memory'd now that I just don't open the option, | 17:33 |
| dansmith | but I was sure I had accidentally rebased someone's stack just before the end of 2021 due to forgetting -R | 17:33 |
| dansmith | but I musta-have dreamed it or rebased locally and forgot, based on what ya'll're saying | 17:34 |
| dansmith | (that's right I can contract a contraction) | 17:34 |
| fungi | well, by default it will leave you in an incomplete rebase if there's a merge conflict, so it's possible you fixed a change up in that situation. also possible there are bugs. i mean, it's software, so... | 17:37 |
| fungi | but yes, if you do see it do what you described, we definitely consider that incorrect/defective behavior | 17:38 |
| dansmith | I doubt it.. I tend to fear rebases of big stacks, so I would almost certainly abort and then go back and rebase -i it | 17:38 |
| dansmith | but yeah | 17:38 |
| clarkb | fungi: I left a thought on how you might be more clear about rebase abort behavior | 17:40 |
| clarkb | yes, I tend to git rebase --abort and then manually restart it myself | 17:40 |
| dansmith | https://paste.opendev.org/show/bawnS9EG2zoGc7OyxxLU/ | 17:41 |
| dansmith | this ^ goes against the "it is always aborted" statement right? | 17:41 |
| fungi | though also, git-review is running `git rebase -i` anyway | 17:42 |
| fungi | dansmith: yeah, that's what i was getting at with my review comment, in the case of a merge conflict it tells you how to abort and push with the conflict intact instead of fixing it, but doesn't automatically run rebase --abort for you | 17:43 |
| dansmith | ack okay | 17:43 |
| fungi | which i agree is a hard decision as to which should be the base behavior, since most times a user is probably going to want to fix their merge conflict and push the result, so if git-review does a rebase --abort then they just end up running rebase a second time (which might take a while depending on the repo and the commits) | 17:45 |
| fungi | i think the original idea was that not aborting the failed rebase would be the more efficient option, but maybe it's also more confusing at the same time | 17:46 |
| dansmith | I dunno, I just always want to control the rebase myself, knowing exactly what I'm rebasing onto | 17:46 |
| dansmith | so I would just never not abort when some tool said "here, I half-did this for you.. good luck!" | 17:46 |
| fungi | yep, i definitely get that | 17:47 |
| dansmith | and just saying "I couldn't rebase this for you, so check yo'sef" is more consistent with it being a test vs. an intentional rebase | 17:47 |
| fungi | i happen to trust it because i know that it actually ran `git rebase -i` (also the output from the tool says that's what it ran), but i understand not trusting that | 17:47 |
| dansmith | but onto a master commit it just pulled right? | 17:48 |
| dansmith | which isn't even my origin/master ref necessarily right? | 17:48 |
| dansmith | like I assume it does a git fetch from the gerrit remote but if I rebase on my own origin/master right after, it'll change again right? | 17:48 |
| fungi | yes, along the lines of `git remote update && git rebase -i origin/master` | 17:48 |
| dansmith | is it origin or the gerrit remote? | 17:49 |
| fungi | it's gerrit/master in that case so as not to disturb your other remotes | 17:49 |
| dansmith | right, so that's definitely not what I'd be doing if manual | 17:49 |
| fungi | and also because some environments (not ours thankfully) may not keep their gerrit and origin remotes in sync | 17:49 |
| dansmith | right | 17:49 |
| dansmith | since I'm just as dumb as clarkb thinks I am, I just like to be very deliberate with my rebasing and updating of my origin/master so I don't get confused and have to come here and embarrass myself :) | 17:50 |
| clarkb | fwiw I'm deliberate about it too. I almost always git rebase --abort when git review hits that | 17:51 |
| fungi | i'll make the proposed manpage edit better in that regard, and shop around the idea of aborting failed rebases by default (maybe introduce a --keep-rebase option or something) | 17:51 |
| clarkb | then I update my remotes and rebase by hand | 17:51 |
| dansmith | fungi: to be clear, I don't care if you change it now, it's just not what *I* want the default behavior to be | 17:52 |
| dansmith | having my git stuff in my prompt generates a "the right side doesn't look right, better rebase --abort | 17:52 |
| dansmith | knee-jerk reaction :) | 17:52 |
| clarkb | that prompt stuff is what caused git to do the "does the current user own this repo" check before operating on the repo | 17:54 |
| clarkb | I've since disabled it which is super annoying | 17:54 |
| clarkb | I run git status a lot more now | 17:55 |
| clarkb | but at least it is deliberate | 17:55 |
| dansmith | yeah, there is just going to have to be some other solution than "don't have git stuff in my prompt" | 17:59 |
| dansmith | like not running any hooks during show or if the perms don't match or whatever.. | 17:59 |
| clarkb | ya would be nice to see them grow a more limited "safe mode" for stuff like that | 18:00 |
| clarkb | I would definitely make use of it | 18:00 |
| dansmith | yeah | 18:01 |
| fungi | (WIP) https://review.opendev.org/850061 Don't keep incomplete rebase state by default | 18:29 |
| fungi | strawman, so i don't forget | 18:29 |
| *** tobias-urdin is now known as tobias-urdin_pto | 19:19 | |
| *** dviroel is now known as dviroel|biab | 20:09 | |
| *** dviroel|biab is now known as dviroel | 21:08 | |
| *** dasm|ruck is now known as dasm|off | 21:37 | |
| *** dviroel is now known as dviroel|out | 21:37 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!