Hi Matheus,
On Sat, 30 Jul 2022, Matheus Tavares wrote:
> On Thu, Jul 28, 2022 at 1:58 PM Johannes Schindelin
> <Johannes.Schindelin@xxxxxx> 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@xxxxxxxxx/
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
