| Message ID | d45ec31eeaf42ee042bad04efd69668144df3138.1678878623.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fetch: refactor code that prints reference updates | expand |
Patrick Steinhardt <ps@pks.im> writes: > Both callsites that call `format_display()` and then print the result to > standard error use the same formatting directive " %s\n" to print the > reference to disk, thus duplicating a small part of the logic. Hmph. If format_display() were a function whose role was to prepare the contents on a single line, it can be argued that it is caller's job to give a leading indent that is appropriate for the line in the context of the display it is producing. "store-updated-refs" and "prune-refs" may be showing a list of refs that were affected under different heading, together with different kind of information, and depending on the way each of these callers organize its output, the appropriate indentation level for the line might be different. So I think the current product format_display() gives its callers is perfectly defensible in that sense. On the other hand, if format_display() is only about showing a single line in the tightly limited context (in other words, both of its callers promise that they will forever be happy with the function showing exactly the same output), then this refactoring would be OK. In addition, it may even make more sense, if that were the role of this callee, to do the actual printing, not just preparing a line of string into a strbuf, in this callee, by moving the fputs() from caller to callee. So, I dunno. The result of applying this patch leaves it in an in-between state, where the division of labor between the caller and the callee smells somewhat iffy. Thanks.
On Wed, Mar 15, 2023 at 03:45:28PM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > Both callsites that call `format_display()` and then print the result to > > standard error use the same formatting directive " %s\n" to print the > > reference to disk, thus duplicating a small part of the logic. > > Hmph. > > If format_display() were a function whose role was to prepare the > contents on a single line, it can be argued that it is caller's job > to give a leading indent that is appropriate for the line in the > context of the display it is producing. "store-updated-refs" and > "prune-refs" may be showing a list of refs that were affected under > different heading, together with different kind of information, and > depending on the way each of these callers organize its output, the > appropriate indentation level for the line might be different. So I > think the current product format_display() gives its callers is > perfectly defensible in that sense. > > On the other hand, if format_display() is only about showing a > single line in the tightly limited context (in other words, both of > its callers promise that they will forever be happy with the > function showing exactly the same output), then this refactoring > would be OK. In addition, it may even make more sense, if that were > the role of this callee, to do the actual printing, not just > preparing a line of string into a strbuf, in this callee, by moving > the fputs() from caller to callee. > > So, I dunno. The result of applying this patch leaves it in an > in-between state, where the division of labor between the caller and > the callee smells somewhat iffy. > > Thanks. I totally agree with you here. From my point of view this "division of labor" is getting fixed in the final patch that then also moves the printing logic into `format_display()`. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> So, I dunno. The result of applying this patch leaves it in an >> in-between state, where the division of labor between the caller and >> the callee smells somewhat iffy. >> >> Thanks. > > I totally agree with you here. From my point of view this "division of > labor" is getting fixed in the final patch that then also moves the > printing logic into `format_display()`. Yes, again I smell the same "isn't this series going a bit too roundabout route to its goal?" which I expressed in my response to an earlier step. The endgame of the series, even though I may not agree with it 100%, is self consistent and does not leave the "this ends at an in-between state" aftertaste in my mouth. Thanks.
On Thu, Mar 16, 2023 at 09:50:38AM -0700, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> So, I dunno. The result of applying this patch leaves it in an > >> in-between state, where the division of labor between the caller and > >> the callee smells somewhat iffy. > >> > >> Thanks. > > > > I totally agree with you here. From my point of view this "division of > > labor" is getting fixed in the final patch that then also moves the > > printing logic into `format_display()`. > > Yes, again I smell the same "isn't this series going a bit too > roundabout route to its goal?" which I expressed in my response to > an earlier step. The endgame of the series, even though I may not > agree with it 100%, is self consistent and does not leave the "this > ends at an in-between state" aftertaste in my mouth. > > Thanks. Yeah. I did have some problems to lay out this series in a sensible way. In an earlier iteration I tried moving the printing logic in one of the initial patches, but from my point of view that resulted in an even more awkward in-between state where the formatting and printing logic was kind of all over the place. And another try with "big-bang-refactoring" was barely reviewable, either. Maybe the solution is to keep the order, but document intent better in each of the patches leading towards the unified printing logic. Patrick
Patrick Steinhardt <ps@pks.im> writes: > Maybe the solution is to keep the order, but document intent better in > each of the patches leading towards the unified printing logic. ;-).
diff --git a/builtin/fetch.c b/builtin/fetch.c index bf2f01245a..6fc2fd0d46 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -885,13 +885,14 @@ static void format_display(struct display_state *display, width = (summary_width + strlen(summary) - gettext_width(summary)); - strbuf_addf(display_buffer, "%c %-*s ", code, width, summary); + strbuf_addf(display_buffer, " %c %-*s ", code, width, summary); if (!display->compact_format) print_remote_to_local(display, display_buffer, remote, prettify_refname(local)); else print_compact(display, display_buffer, remote, prettify_refname(local)); if (error) strbuf_addf(display_buffer, " (%s)", error); + strbuf_addch(display_buffer, '\n'); } static int update_local_ref(struct ref *ref, @@ -1271,7 +1272,7 @@ static int store_updated_refs(struct display_state *display, url_len, url); shown_url = 1; } - fprintf(stderr, " %s\n", note.buf); + fputs(note.buf, stderr); } } } @@ -1432,7 +1433,7 @@ static int prune_refs(struct display_state *display, format_display(display, &sb, '-', _("[deleted]"), NULL, _("(none)"), ref->name, summary_width); - fprintf(stderr, " %s\n",sb.buf); + fputs(sb.buf, stderr); strbuf_release(&sb); warn_dangling_symref(stderr, dangling_msg, ref->name); }
Both callsites that call `format_display()` and then print the result to standard error use the same formatting directive " %s\n" to print the reference to disk, thus duplicating a small part of the logic. Refactor the code to handle this in `format_display()` itself. This paves the way for handling the printing logic in that function completely. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- builtin/fetch.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)