[Rpm-maint] [rpm-software-management/rpm] Add "local_generator" (PR #2734)

[Panu Matilainen]

@pmatilai commented on this pull request.



>      if (rpmGlob(attrPath, NULL, &files) == 0) {
-	nattrs = argvCount(files);
-	fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
-	for (int i = 0; i < nattrs; i++) {
-	    char *bn = basename(files[i]);
-	    bn[strlen(bn)-strlen(".attr")] = '\0';
-	    fc->atypes[i] = rpmfcAttrNew(bn);
-	}
-	fc->atypes[nattrs] = NULL;
-	argvFree(files);
+	nfiles = argvCount(files);
+    }
+    for (int i = 0; i < nfiles; i++) {
+	char *bn = basename(files[i]);
+	bn[strlen(bn)-strlen(".attr")] = '\0';
+	argvAddUniq(&all_attrs, bn);

Doh, I was somehow thinking rpmGlob() returned basenames there but, but (of course) it doesn't. So scratch that part, sorry.

The location is configurable but it's still always just a single directory, it cannot possibly have multiple files by the same name. It's not that argvAddUniq() is *wrong*, it just gives the reader the wrong impression basically. Similarly moving this .attr splitting loop out of the rpmGlob() if-block seems strange, because there's only ever anything to do on rpmGlob() success. So that's kinda backwards to what you say about not wanting to think about the case where it doesn't find anything because that's already handled, and just obfuscates the diff for no good reason. It's always better to let the actual change stand out in the diff when reasonably possible.

This is the *actual* change to the first part here. nattr gets recalculated later on, but most of the time it's already correct from here:

```
     /* Discover known attributes from pathnames + initialize them */
     if (rpmGlob(attrPath, NULL, &files) == 0) {
        nattrs = argvCount(files);
-       fc->atypes = xcalloc(nattrs + 1, sizeof(*fc->atypes));
        for (int i = 0; i < nattrs; i++) {
            char *bn = basename(files[i]);
            bn[strlen(bn)-strlen(".attr")] = '\0';
-           fc->atypes[i] = rpmfcAttrNew(bn);
+           argvAdd(&all_attrs, bn);
        }
-       fc->atypes[nattrs] = NULL;
        argvFree(files);
     }
+
+    /* Get file attributes from _local_file_attrs macro */
+    /* ... /

```
(actually one could just pass &nattrs to rpmGlob() and avoid the argvCount(), but that too would kinda obfuscate the diff)


Patch cleanliness aside, this all needs to be just one commit. It's a single new feature with its tests, we are not interested in PR review in the commit history.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/2734#discussion_r1489081084
You are receiving this because you are subscribed to this thread.

Message ID: <rpm-software-management/rpm/pull/2734/review/1879642452 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rpm.org/pipermail/rpm-maint/attachments/20240214/298f7efd/attachment-0001.html>