Junio C Hamano <gitster@xxxxxxxxx> writes: > Lucas Oshiro <lucasseikioshiro@xxxxxxxxx> writes: > >> Improve code readability by moving tag body reading to a new function called >> get_tag_body. > > Quite honestly, I think the result of this splitting is harder to > follow than the original. > > For example, the value of opt->message_given and the validity of > given_msg is very closely related, so if you made the helper > function receive non-NULL given_msg when !opt->message_given, the > helper could only check !given_msg without having to worry about > opt->message_given; with such a change, I could buy that the split > improves code readability, but I do not see any such change in the > patch. Just to avoid misunderstanding, I am not suggesting to change the interface into the helper you introduced to be like this prepare_tag_template(opt->message_given ? buf, NULL, opt, prev, path, tag); That still needs to pass the entire opt structure into the helper and forces the helper to look at opt->cleanup_mode and behave differently. If a restructuring of the code can be done in such a way that the helper no longer need to look at opt (hence no need to pass the entire opt structure into it), I can see that the change improves the readability of the resulting code, but I am not all that hopeful. On the other hand (and this is the more important hand among the two ;-), I do not mind splitting a helper function out of an existing codepath and make the existing codepath call the new helper, so that the same helper can be reused from another codepath later. Not at all. What I do mind is to mislabel a change that is done for such a later code reuse as one that improves readability, when it does not. Thanks. >> Enhance legibility by encapsulating code that loads previous tag message >> (if any) in new function prepare_tag_template.... > > The helper seems to be used to _write_ into path, and not load or > read any message from either an object or a file on disk.
