June 2022 - Wine-Devel - list.winehq.org

Re: HLSL offsetting

by Francisco Casas

Hello, thanks for commenting on the patch! On 15-06-22 19:20, Zebediah Figura wrote: > > From skimming the patch, here are some immediate thoughts: > > * I have a patch which has been around for a while which replaces the > field linked list with an array. I think we wanted that anyway, but we > especially want it given list_get(). > This seems like a good idea, I will see if I can add it before this patch. > * All current (upstream) uses of hlsl_new_store() and hlsl_new_load() > with a nonzero offset fall into one of two categories: > > - retrieving the element at a specific offset (initializer and matrix > parsing) > > - adding another offset to an existing hlsl_deref (splitting passes, > initial parsing of field/element derefs) > > I might have missed something, but assuming I've got that right, I > think that the hlsl_new_load() and hlsl_new_store() should take > arguments to reflect that. (I.e. split each into two functions, one > taking an unsigned int and the other taking a const struct hlsl_deref > *). In that case struct path_arg would be limited to hlsl.c, if it even > needs to exist. > Good observation, I can see some problems though: a) The version of the function that receives the component index can generate the path of constant nodes alright, but it also needs to insert these nodes in the corresponding instruction list. The instruction list can be received in this case and the new constant nodes can be added at the head -- or -- we can allow the "steps" in the path to be either an hlsl_src or an unsigned int, and then add a pass that adds the constant nodes at the beginning of the program: --- struct hlsl_step { struct hlsl_src *src; unsigned int c; /* used if src is NULL */ }; struct hlsl_deref { struct hlsl_ir_var *var; unsigned int path_len; struct hlsl_step *path; }; --- I don't like this idea too much, but it also covers for the field accesses that Giovanni mentioned. b) In my patch, accessing matrix components requires paths of length 2 (which I think is the right thing to do) so the version of the functions that receive a hlsl_deref should also receive 2 optional nodes instead of 1. So for instance, hlsl_new_store would be: --- struct hlsl_ir_store *hlsl_new_store(struct hlsl_ctx *ctx, struct hlsl_ir_var *var, const hlsl_deref *deref, struct hlsl_ir_node *node1, struct hlsl_ir_node *node2, struct hlsl_ir_node *rhs, unsigned int writemask, struct vkd3d_shader_location loc); --- this adds a little clutter. c) We also have to change other functions, for instance, add_load() in upstream adds an arbitrary register offset to an hlsl_deref offset. In the patch, this generality is achieved by concatenating paths. Given the current uses of add_load() we would also need to create two versions of this function, the "hlsl_deref + 2 optional nodes" one and the "component index" one, and solve this particular case of problem (a) too. I am not sure that loosing the generality of using path_arg is a good thing, it can simplify some calls to loads and stores but that's the only benefit I see (unless I am missing something). I can switch to this approach though, if we agree in a way of solving (a). > Note that this step could also be done independently of anything > else, which would help make the "big" patch more reviewable. > You mean something like adding a "Replace path_arg with specific load and store functions." patch afterwards? > * Similarly I think hlsl_new_resource_load() and friends should just > take a "const struct hlsl_deref *" or two. Yes, this seems consistent if we do stores and loads this way. > I'm not immediately sure what > to do with prepend_input_copy() and friends—maybe those should be > converted to iterative passes instead of recursive ones. I don't see any problem keeping them as recursive passes. > * Stuff like free_hlsl_deref() can be made into a separate patch. Hmm, sorry, I am not sure I see a clear division here. What other things could be in this separate patch?

June 16, 2022

Re: Gitlab experiment wrapup

by Gabriel Ivăncescu

On 16/06/2022 19:33, Alex Henrie wrote: > On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu > <gabrielopcode(a)gmail.com> wrote: >> >> There's one other (pretty big, for me) problem I can't seem to find how >> to replicate with MRs compared to sending patches: how do I add notes >> for each commit that shouldn't actually be committed? For patches I used >> to add below the --- line, and these are super useful when you just want >> to tell the information to the reviewer, which wouldn't make much sense >> to have in the codebase itself. >> >> These seem to get lost when I push. I have to admit `git notes` seems >> pretty convoluted to me and I've no idea how it gets "stored" especially >> for merge requests. Patches were much easier to comprehend. >> >> I mean, I guess I can add it to the MR description but that's not >> pointing out to a specific commit/patch... sigh. > > In a GitLab merge request, I can click on a specific commit to see its > diff, then click on the commit hash again, and GitLab gives me a box > to leave a comment either about a specific line of the commit or about > the commit as a whole. Would that work for you? > > -Alex > I see, the click on the hash again is definitely more convoluted than I intuitively expected, but at least there's a way to add such comment. It would be super if it could be automated (scripted) though. I hate clicking all over the place. At least I found that a quick URL to get to such commit is: https://gitlab.winehq.org/wine/wine/-/commit/<commit_sha>?merge_request_iid=<mr id> That said I still have to copy and paste it manually to gitlab's web UI, which also requires logging in and loads slowly due to javascript bloat... Is there a way to add this kind of comment with glab-cli?

June 16, 2022

Re: Gitlab experiment wrapup

by Alex Henrie

On Thu, Jun 16, 2022 at 10:04 AM Gabriel Ivăncescu <gabrielopcode(a)gmail.com> wrote: > > There's one other (pretty big, for me) problem I can't seem to find how > to replicate with MRs compared to sending patches: how do I add notes > for each commit that shouldn't actually be committed? For patches I used > to add below the --- line, and these are super useful when you just want > to tell the information to the reviewer, which wouldn't make much sense > to have in the codebase itself. > > These seem to get lost when I push. I have to admit `git notes` seems > pretty convoluted to me and I've no idea how it gets "stored" especially > for merge requests. Patches were much easier to comprehend. > > I mean, I guess I can add it to the MR description but that's not > pointing out to a specific commit/patch... sigh. In a GitLab merge request, I can click on a specific commit to see its diff, then click on the commit hash again, and GitLab gives me a box to leave a comment either about a specific line of the commit or about the commit as a whole. Would that work for you? -Alex

June 16, 2022

[tools v2 2/2] testbot/Engine: Also handle notifications originating from the Engine.

by Francois Gouget

In particular notifications of job status changes must be processed in order for the result emails to be sent and for the patch status to be updated. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: Fixed a typo in Engine::Notify. I believe this explains why a number of developer emails were getting lost during some tests I ran. I had suspected ISP-level spam filtering issues, or too big emails, but I recently noticed cases where WineSendLog was not getting called which turned out to be because JobStatusChange() was being called by the Engine itself. --- testbot/bin/Engine.pl | 2 +- testbot/lib/WineTestBot/Engine/Notify.pm | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index 29dce183b..9d836529c 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -751,7 +751,7 @@ sub main() $SIG{CHLD} = \&REAPER; WineTestBot::Log::SetupRedirects(); - $WineTestBot::Engine::Notify::RunningInEngine = 1; + SetNotificationHandler(\&HandleClientCmd); LogMsg "Starting the WineTestBot Engine\n"; # Validate and adjust the configuration options diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm index 9c1802984..78ece0676 100644 --- a/testbot/lib/WineTestBot/Engine/Notify.pm +++ b/testbot/lib/WineTestBot/Engine/Notify.pm @@ -2,6 +2,7 @@ # Notification of WineTestBot engine # # Copyright 2009 Ge van Geldorp +# Copyright 2012-2022 Francois Gouget # # This library is free software; you can redistribute it and/or # modify it under the terms of the GNU Lesser General Public @@ -29,8 +30,8 @@ WineTestBot::Engine::Notify - Engine notification use Exporter 'import'; our $RunningInEngine; -our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl - RescheduleJobs VMStatusChange +our @EXPORT = qw(SetNotificationHandler Shutdown PingEngine JobStatusChange + JobControl RescheduleJobs VMStatusChange WinePatchMLSubmission WinePatchWebSubmission UpdateFailure GetScreenshot); our @EXPORT_OK = qw($RunningInEngine); @@ -38,14 +39,21 @@ our @EXPORT_OK = qw($RunningInEngine); use Socket; use WineTestBot::Config; +my $_CmdHandler; + +sub SetNotificationHandler($) +{ + my ($Handler) = @_; + $_CmdHandler = $Handler; +} sub SendCmdReceiveReply($) { my ($Cmd) = @_; - if (defined($RunningInEngine)) + if ($_CmdHandler) { - return "1"; + return &$_CmdHandler(split ' ', $Cmd); } if (! socket(SOCK, PF_UNIX, SOCK_STREAM, 0)) -- 2.30.2

June 16, 2022

[tools v2 1/2] testbot/web: Automatically refresh the failure bug information.

by Francois Gouget

Automatically refresh the failure bug information when the bug id is modified. Also trigger a refresh when a failure is undeleted. Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48912 Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- v2: No change, only part 2/2 changed. This ensures failures have an initial bug status and description as soon as they are added. Later refreshes happen when UpdateFailures is called by cron. --- testbot/bin/Engine.pl | 39 ++++++++++++++++++++++++ testbot/lib/WineTestBot/Engine/Notify.pm | 20 +++++++++++- testbot/lib/WineTestBot/Failures.pm | 31 +++++++++++++++++++ testbot/web/FailureDetails.pl | 1 - testbot/web/FailuresList.pl | 5 ++- 5 files changed, 91 insertions(+), 5 deletions(-) diff --git a/testbot/bin/Engine.pl b/testbot/bin/Engine.pl index 88852730e..29dce183b 100755 --- a/testbot/bin/Engine.pl +++ b/testbot/bin/Engine.pl @@ -49,6 +49,7 @@ use WineTestBot::Config; use WineTestBot::Engine::Events; use WineTestBot::Engine::Notify; use WineTestBot::Engine::Scheduler; +use WineTestBot::Failures; use WineTestBot::Jobs; use WineTestBot::Log; use WineTestBot::Patches; @@ -548,6 +549,43 @@ sub HandleGetScreenshot($) return "1" . $ImageBytes; } +sub HandleUpdateFailure($) +{ + my ($FailureId) = @_; + + # Validate FailureId + if ($FailureId !~ /^(\d+)$/) + { + LogMsg "Invalid failure id\n"; + return "0Invalid Failure Id"; + } + $FailureId = $1; + + if (!CreateFailures()->GetItem($FailureId)) + { + LogMsg "Unknown Failure $FailureId\n"; + return "0Unknown Failure $FailureId"; + } + + my $Pid = fork; + if (!defined $Pid) + { + LogMsg "Unable to fork for UpdateFailures.pl: $!\n"; + } + elsif (!$Pid) + { + # Clean up the child environment + CloseAllDBBackEnds(); + WineTestBot::Log::SetupRedirects(); + + exec("$BinDir/UpdateFailures.pl --log-only $FailureId") or + LogMsg "Unable to exec ${ProjectName}UpdateFailures.pl: $!\n"; + exit(1); + } + + return "1"; +} + my %Handlers=( "getscreenshot" => \&HandleGetScreenshot, "jobcontrol" => \&HandleJobControl, @@ -558,6 +596,7 @@ my %Handlers=( "vmstatuschange" => \&HandleVMStatusChange, "winepatchmlsubmission" => \&HandleWinePatchMLSubmission, "winepatchwebsubmission" => \&HandleWinePatchWebSubmission, + "updatefailure" => \&HandleUpdateFailure, ); sub HandleClientCmd(@) diff --git a/testbot/lib/WineTestBot/Engine/Notify.pm b/testbot/lib/WineTestBot/Engine/Notify.pm index 82221c9ed..9c1802984 100644 --- a/testbot/lib/WineTestBot/Engine/Notify.pm +++ b/testbot/lib/WineTestBot/Engine/Notify.pm @@ -31,7 +31,8 @@ use Exporter 'import'; our $RunningInEngine; our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl RescheduleJobs VMStatusChange - WinePatchMLSubmission WinePatchWebSubmission GetScreenshot); + WinePatchMLSubmission WinePatchWebSubmission + UpdateFailure GetScreenshot); our @EXPORT_OK = qw($RunningInEngine); use Socket; @@ -194,6 +195,23 @@ sub WinePatchWebSubmission() return substr($Reply, 1); } +sub UpdateFailure($) +{ + my ($FailureId) = @_; + + my $Reply = SendCmdReceiveReply("updatefailure $FailureId\n"); + if (length($Reply) < 1) + { + return "The Engine did not acknowledge the UpdateFailure command"; + } + if (substr($Reply, 0, 1) eq "1") + { + return (undef, substr($Reply, 1)); + } + + return (substr($Reply, 1), undef); +} + sub GetScreenshot($) { my ($VMName) = @_; diff --git a/testbot/lib/WineTestBot/Failures.pm b/testbot/lib/WineTestBot/Failures.pm index b37b9e046..a9118f64b 100644 --- a/testbot/lib/WineTestBot/Failures.pm +++ b/testbot/lib/WineTestBot/Failures.pm @@ -40,6 +40,8 @@ TaskFailure objects. use WineTestBot::WineTestBotObjects; our @ISA = qw(WineTestBot::WineTestBotItem); +use WineTestBot::Engine::Notify; + sub Compare($$) { @@ -55,6 +57,35 @@ sub Compare($$) return $Cmp } +sub BugId($;$) +{ + my ($self, $NewBugId) = @_; + + my $CurrentBugId = $self->SUPER::BugId; + return $CurrentBugId if (!defined $NewBugId); + + if (!defined $CurrentBugId or $NewBugId ne $CurrentBugId) + { + $self->{OldBugId} = $CurrentBugId if (!defined $self->{OldBugId}); + $self->SUPER::BugId($NewBugId); + } + + return $NewBugId; +} + +sub OnSaved($) +{ + my ($self) = @_; + + $self->SUPER::OnSaved(); + if ((defined $self->{OldBugId} and $self->{OldBugId} ne $self->BugId) or + !$self->BugStatus or !$self->BugDescription) + { + UpdateFailure($self->Id); + } + delete $self->{OldBugId}; +} + sub Validate($) { my ($self) = @_; diff --git a/testbot/web/FailureDetails.pl b/testbot/web/FailureDetails.pl index 7cab1d2a6..c05146448 100644 --- a/testbot/web/FailureDetails.pl +++ b/testbot/web/FailureDetails.pl @@ -105,7 +105,6 @@ sub OnAction($$) $self->{Item}->BugDescription(""); } return undef if (!$self->Save()); - # FIXME Notify the TestBot Engine so it updates the bug information exit($self->RedirectToParent()); } return $self->SUPER::OnAction($Action); diff --git a/testbot/web/FailuresList.pl b/testbot/web/FailuresList.pl index f2608dc45..8825fea69 100644 --- a/testbot/web/FailuresList.pl +++ b/testbot/web/FailuresList.pl @@ -129,9 +129,9 @@ sub OnItemAction($$$) return 1 if ($Action eq "Restore" and $Failure->BugStatus ne "deleted"); my $NewStatus = $Action eq "Delete" ? "deleted" : - $Action eq "Restore" ? "unknown" : + $Action eq "Restore" ? "" : undef; - if ($NewStatus) + if (defined $NewStatus) { $Failure->BugStatus($NewStatus); my ($_ErrProperty, $ErrMessage) = $Failure->Save(); @@ -141,7 +141,6 @@ sub OnItemAction($$$) $self->{EnclosingPage}->SetError(undef, $ErrMessage); return 0; } - # FIXME Notify the TestBot Engine so it updates the bug information return 1; } } -- 2.30.2

June 16, 2022

[tools] testbot/web: Add failure links to full logs on the job details page.

by Francois Gouget

Long logs can make it hard to find the lines for a given failure. So add an index pointing to the first line of a given failure (identified by its bug id). Also add links to the first and last failure. Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- testbot/web/JobDetails.pl | 38 ++++++++++++++++++++++++++++++++------ 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/testbot/web/JobDetails.pl b/testbot/web/JobDetails.pl index efdd4d9b0..6d3f13cd0 100644 --- a/testbot/web/JobDetails.pl +++ b/testbot/web/JobDetails.pl @@ -404,9 +404,9 @@ sub GetFailureLinks($$) } @$FailureIds); } -sub GenerateFullLog($$$$) +sub GenerateFullLog($$$$$) { - my ($self, $Dir, $LogName, $HideLog) = @_; + my ($self, $Dir, $LogId, $LogName, $HideLog) = @_; my $LogInfo = LoadLogErrors("$Dir/$LogName"); if (defined $LogInfo->{BadLog}) @@ -426,7 +426,7 @@ sub GenerateFullLog($$$$) print "<pre class='log-note'>Some WineTest results could not be used to detect new errors: $LogInfo->{BadRef}</pre>\n"; } - my (%ErrCategory, %ErrFailures); + my (%ErrCategory, %ErrFailures, %LogBugs); foreach my $GroupName (@{$LogInfo->{ErrGroupNames}}) { my $Group = $LogInfo->{ErrGroups}->{$GroupName}; @@ -437,8 +437,31 @@ sub GenerateFullLog($$$$) { $ErrCategory{$LineNo} = $Group->{IsNew}->[$ErrIndex] ? "fullnew" : "error"; $ErrFailures{$LineNo} = $Group->{Failures}->{$ErrIndex}; + + if ($ErrFailures{$LineNo}) + { + foreach my $FailureId (@{$ErrFailures{$LineNo}}) + { + my $Failure = $self->{Failures}->GetItem($FailureId); + $LogBugs{$Failure->BugId} ||= [$LineNo, $Failure->BugDescription]; + } + } } + $LogBugs{-1} ||= [$LineNo, ""]; + $LogBugs{9999999} = [$LineNo, ""]; + } + } + if (%LogBugs) + { + print "<p>Failures index:"; + foreach my $Label (sort { $a <=> $b } keys %LogBugs) + { + my ($LineNo, $Title) = @{$LogBugs{$Label}}; + $Label = $Label < 0 ? "First" : $Label == 9999999 ? "Last" : $Label; + $Title = " title='". $self->escapeHTML($Title) ."'" if ($Title); + print " <a href='#k${LogId}L$LineNo'$Title>$Label</a>" } + print "</p>\n"; } my $GetCategory = $LogName =~ /\.report$/ ? \&GetReportLineCategory : @@ -463,7 +486,7 @@ sub GenerateFullLog($$$$) my $Category = $ErrCategory{$LineNo} || $GetCategory->($Line); if ($Category ne "none") { - $Html =~ s~^(.*\S)\s*\r?$~<span class='log-$Category'>$1</span>~; + $Html =~ s~^(.*\S)\s*\r?$~<span id='k${LogId}L$LineNo' class='log-$Category'>$1</span>~; } if ($ErrFailures{$LineNo}) { @@ -487,7 +510,10 @@ sub GenerateFullLog($$$$) # Extra groups don't have a line number next if ($Group->{LineNo}); - print "<div class='LogDllName'>$GroupName</div>\n"; + # Extra errors don't have line numbers but there should be few of them. + # So just create one id for them all so the shortcut to the first / last + # error has somewhere to go even if it's not 100% accurate. + print "<div id='k${LogId}L0' class='LogDllName'>$GroupName</div>\n"; for my $ErrIndex (0..$#{$Group->{Errors}}) { if ($IsEmpty) @@ -592,7 +618,7 @@ EOF $Url = $self->escapeHTML($Url); my $HideLog = $Action eq "Hide" ? " ondblclick='HideLog(event, \"$Url\")'" : ""; - my $LogIsEmpty = $self->GenerateFullLog($TaskDir, $MoreInfo->{Full}, $HideLog); + my $LogIsEmpty = $self->GenerateFullLog($TaskDir, $StepTask->GetKey(), $MoreInfo->{Full}, $HideLog); if ($LogIsEmpty) { if ($StepTask->Status eq "canceled") -- 2.30.2

June 16, 2022

[tools] testbot: Update the failures 'last seen' fields.

by Francois Gouget

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=48912 Signed-off-by: Francois Gouget <fgouget(a)codeweavers.com> --- I lost that bit at some point :-( --- testbot/lib/WineTestBot/LogUtils.pm | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/testbot/lib/WineTestBot/LogUtils.pm b/testbot/lib/WineTestBot/LogUtils.pm index b8aaaa43c..e25ea3ae2 100644 --- a/testbot/lib/WineTestBot/LogUtils.pm +++ b/testbot/lib/WineTestBot/LogUtils.pm @@ -1435,11 +1435,14 @@ sub MatchLogFailures($$) =item C<SaveLogFailures()> -Updates the TaskFailures objects for the task and log specified by the -$LogFailures structure. +Updates the Failures and TaskFailures objects for the task and log specified +by the $LogFailures structure. -Note that this implies deleting any preexisting TaskFailure to avoid leaving -obsolete data. +Notes: +* This implies deleting any preexisting TaskFailure to avoid leaving obsolete + data. +* This also updates the failures LastNew and LastOld fields which can however + not be undone. =back =cut @@ -1457,15 +1460,26 @@ sub SaveLogFailures($) return $ErrMessage if (defined $ErrMessage); return undef if (!$LogFailures->{Failures}); + my $Started = $LogFailures->{Task}->Started; foreach my $LogFailure (values %{$LogFailures->{Failures}}) { - my $TaskFailure = $LogFailure->{Failure}->TaskFailures->Add(); + my $Failure = $LogFailure->{Failure}; + my $TaskFailure = $Failure->TaskFailures->Add(); my $OldKey = $TaskFailure->GetKey(); $TaskFailure->Task($LogFailures->{Task}); $TaskFailure->TaskLog($LogFailures->{LogName}); $TaskFailure->KeyChanged($OldKey, $TaskFailure->GetKey()); $TaskFailure->NewCount($LogFailure->{NewCount}); $TaskFailure->OldCount($LogFailure->{OldCount}); + + if ($LogFailure->{NewCount} and ($Failure->LastNew || 0) < $Started) + { + $Failure->LastNew($Started); + } + if ($LogFailure->{OldCount} and ($Failure->LastOld || 0) < $Started) + { + $Failure->LastOld($Started); + } } (my $_ErrKey, my $_ErrProperty, $ErrMessage) = $LogFailures->{Collection}->Save(); -- 2.30.2

June 16, 2022

Re: Gitlab transition roadmap

by Gabriel Ivăncescu

Hi, Just a couple requests if I may. On 15/06/2022 15:08, Alexandre Julliard wrote: > - Refine behavior of mailing list gateway, possibly send output to > separate list. > I don't mind a separate list or anything else, but please keep a way for some of us to still be able to check the "patch flow" like now via email. I don't mind if it this requires tinkering on my end as long as it's "possible"... > - Work with respective maintainers to plan if/when/how to move other > Wine projects to Gitlab: > - tools (done) > - website (done) > - wine-gecko (done) > - wine-mono > - wine-staging > - vkd3d > - appdb > - bugzilla > - fontforge > For bugzilla I'd rather prefer if it remained because, well, it works without javascript and is simple and lightweight... I don't mind web UIs if they're simple but gitlab overall leaves a lot to be desired. > - Retire the gitweb browser, redirect to Gitlab. > As above, is it too much hassle to keep this around? Even if it's just a mirror. I like that it's simple and lightweight and can be browsed easily, gitlab is way too bloated both in design and accessing it, and doesn't work without javascript either. I have to admit when I first saw the gitlab experiment, I didn't expect a total overhaul like this even for stuff not related to patches, I thought it would be optional. Just my 2 cents.

June 16, 2022

Re: [tools 2/2] testbot/Engine: Also handle notifications originating from the Engine.

by Francois Gouget

On Thu, 16 Jun 2022, Francois Gouget wrote: [...] > -our @EXPORT = qw(Shutdown PingEngine JobStatusChange JobControl > - RescheduleJobs VMStatusChange > +our @EXPORT = qw(SetNotificationHandlerShutdown PingEngine JobStatusChange Oups. I introduced a typo here during a rebase. I will resubmit. -- Francois Gouget <fgouget(a)codeweavers.com>

June 16, 2022

Re: Gitlab experiment wrapup

by Gabriel Ivăncescu

On 15/06/2022 01:16, Zebediah Figura wrote: > On 6/14/22 15:29, Gabriel Ivăncescu wrote: >> Hi, >> >> On 14/06/2022 13:55, Alexandre Julliard wrote: >>> * Some other things I like: >>> >>> - Updating status doesn't need to go through me, people can assign >>> reviewers, supersede patches, etc. directly. That reduces my workload >>> and improves the bus factor. Once we have figured out how to make >>> testbot results reliable, we could also have maintainers merge >>> commits >>> directly. >>> >> >> I created my first MR since the switch seems inevitable. I completely >> fail at assigning a reviewer. I even tried the web UI and I can't find >> any setting. >> >> I also tried the /assign_reviewer "quick action" on a comment and it >> said: >> >> `Could not apply assign_reviewer command.` >> >> Is this feature not working currently? > > FWIW, there's a "lab" command line tool that lets you do > > lab mr edit --assign <username> > > and also > > lab mr edit --review <username> > > and I'm sure the "glab" has something with identical syntax. > > Not sure what the difference between those are, though? > Thanks. These tools make it a lot more bearable. Also I think "assign" is for someone who's working on the code (for example when a reviewer actually got to it, or when you're refactoring code based on reviews and taking a while). But can't say for sure. There's one other (pretty big, for me) problem I can't seem to find how to replicate with MRs compared to sending patches: how do I add notes for each commit that shouldn't actually be committed? For patches I used to add below the --- line, and these are super useful when you just want to tell the information to the reviewer, which wouldn't make much sense to have in the codebase itself. These seem to get lost when I push. I have to admit `git notes` seems pretty convoluted to me and I've no idea how it gets "stored" especially for merge requests. Patches were much easier to comprehend. I mean, I guess I can add it to the MR description but that's not pointing out to a specific commit/patch... sigh.

June 16, 2022