Re: [PATCH v2 2/2] wt-status: tolerate dangling marks

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 31, 2020, 17:37 UTC
Message-ID: <xmqqzh6ag1ih.fsf@gitster.c.googlers.com>
In-Reply-To: <20200831171732.1199755-1-jonathantanmy@google.com>

Jonathan Tan <jonathantanmy@google.com> writes:

Show 17 quoted lines

>> For example, what is the reasoning behind making dwim_ref() unable
>> to ask the "do so gently" variant, while allowing repo_dwim_ref()
>> to?
>> 
>> I am NOT necessarily saying these two functions MUST be able to
>> access the same set of features and the only difference between them
>> MUST be kept to the current "repo_* variant can work on an arbitrary
>> repository, while the variant without repo_* would work on the
>> primary repository only".  As long as there is a good reason to make
>> their power diverge, it is OK---I just do not see why and I'd like
>> to know.
>
> Maybe add the following to the end of the last paragraph of the commit
> message:
>
>   (dwim_ref() is unchanged because I expect more and more code to use
>   repo_dwim_ref(), and to reduce the size of the diff.)

If that is the reasoning, I totally disagree. We may be staring problems in submodules too long to think everybody should use the variant with repo_ prefix, but a single repository operations will continue to exist, and when you know you only need to access the current repository, not having to use the function with longer name without having to pass an extra parameter is a plus.

I even wonder why dwim_ref() is not defined like so in a header:

	#define dwim_ref(s, l, o, r) \
		repo_dwim_ref(the_repository, (s), (l), (o), (r))

Which would force a change like the one we are discussing to keep them in parallel and keep the promise that only difference between the two is that dwim_ref() works with the_repository and the other can take an arbitrary repository. Perhaps that can be a preliminary clean-up patch before these two patches ;-)

Doing so would mean that coders have to remember one fewer thing than "dwim_ref(), not only cannot take a repository pointer, cannot be told to report error gently". The worst part is that we know that the difference will GROW, because the purpose of adding the extra option argument to the API is exactly to make it easy to do so.

Reducing the size of the diff is a good justification only when the end result is the same. If it burdens future developers more, that is "I write less at the expense of forcing others to write more", which is not quite the same thing.

Thanks.