Re: [PATCH v2] t/t0021: convert the rot13-filter.pl script to C

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Matheus,

On Sat, 30 Jul 2022, Matheus Tavares wrote:

> On Thu, Jul 28, 2022 at 1:58 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > > On Sun, 24 Jul 2022, Matheus Tavares wrote:
> > >
> > > +static void command_loop(void)
> > > +{
> > > +     while (1) {
> > > +             char *command = packet_key_val_read("command");
> > > +             if (!command) {
> > > +                     fprintf(logfile, "STOP\n");
> > > +                     break;
> > > +             }
> > > +             fprintf(logfile, "IN: %s", command);
> >
> > We will also need to `fflush(logfile)` here, to imitate the Perl script's
> > behavior more precisely.
>
> I was somewhat intrigued as to why the flushes were needed in the Perl
> script. But reading [1] and [2], now, it seems to have been an
> oversight.
>
> That is, Eric suggested splictily flushing stdout because it is a
> pipe, but the author ended up erroneously disabling autoflush for
> stdout too, so that's why we needed the flushes there. They later
> acknowledged that and said that they would re-enabled it (see [2]),
> but it seems to have been forgotten. So I think we can safely drop the
> flush calls.
>
> [1]: http://public-inbox.org/git/20160723072721.GA20875%40starla/
> [2]: https://lore.kernel.org/git/7F1F1A0E-8FC3-4FBD-81AA-37786DE0EF50@gmail.com/

I am somewhat weary of introducing a change of behavior while
reimplementing a Perl script in C at the same time, but in this instance I
think that the benefit of _not_ touching the `pkt-line.c` code is a
convincing reason to do so.

> > > +
> > > +             if (!strcmp(command, "list_available_blobs")) {
> > > +                     struct hashmap_iter iter;
> > > +                     struct strmap_entry *ent;
> > > +                     struct string_list_item *str_item;
> > > +                     struct string_list paths = STRING_LIST_INIT_NODUP;
> > > +
> > > +                     /* flush */
> > > +                     if (packet_read_line(0, NULL))
> > > +                             die("bad list_available_blobs end");
> > > +
> > > +                     strmap_for_each_entry(&delay, &iter, ent) {
> > > +                             struct delay_entry *delay_entry = ent->value;
> > > +                             if (!delay_entry->requested)
> > > +                                     continue;
> > > +                             delay_entry->count--;
> > > +                             if (!strcmp(ent->key, "invalid-delay.a")) {
> > > +                                     /* Send Git a pathname that was not delayed earlier */
> > > +                                     packet_write_fmt(1, "pathname=unfiltered");
> > > +                             }
> > > +                             if (!strcmp(ent->key, "missing-delay.a")) {
> > > +                                     /* Do not signal Git that this file is available */
> > > +                             } else if (!delay_entry->count) {
> > > +                                     string_list_insert(&paths, ent->key);
> > > +                                     packet_write_fmt(1, "pathname=%s", ent->key);
> > > +                             }
> > > +                     }
> > > +
> > > +                     /* Print paths in sorted order. */
> >
> > The Perl script does not order them specifically. Do we really have to do
> > that here?
>
> It actually prints them in sorted order:
>
>         foreach my $pathname ( sort keys %DELAY )

Whoops, sorry for missing that!

> > > +                             fprintf(logfile, " [OK]\n");
> > > +
> > > +                             packet_flush(1);
> > > +                             strbuf_release(&sb);
> > > +                     }
> > > +                     free(pathname);
> > > +                     strbuf_release(&input);
> > > +             }
> > > +             free(command);
> > > +     }
> > > +}
> > > [...]
> > > +static void packet_initialize(const char *name, int version)
> > > +{
> > > +     struct strbuf sb = STRBUF_INIT;
> > > +     int size;
> > > +     char *pkt_buf = packet_read_line(0, &size);
> > > +
> > > +     strbuf_addf(&sb, "%s-client", name);
> > > +     if (!pkt_buf || strncmp(pkt_buf, sb.buf, size))
> >
> > We do not need the flexibility of the Perl package, where `name` is a
> > parameter. We can hard-code `git-filter-client` here. I.e. something like
> > this:
> >
> >         if (!pkt_buf || size != 17 ||
> >             strncmp(pkt_buf, "git-filter-client", 17))
>
> Good idea! Thanks. Perhaps, can't we do:
>
>         if (!pkt_buf || strncmp(pkt_buf, "git-filter-client", size))
>
> to avoid the hard-coded and possibly error-prone 17?

I am afraid that this is not idempotent. If `pkt_buf` is "git" and `size`
is 3, then the suggested `strncmp()` would return 0, but we would want it
to be non-zero.

The best way to avoid the hard-coded 17 would be to introduce a local
constant and use `strlen()` on it (which modern compilers would evaluate
already at compile time).

> > > +             die("bad initialize: '%s'", xstrndup(pkt_buf, size));
> > > +
> > > +     strbuf_reset(&sb);
> > > +     strbuf_addf(&sb, "version=%d", version);
>
> Thanks for a very detailed review and great suggestions!

Thank you for your contribution that is very much relevant to my
interests!

Ciao,
Dscho