git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jonathan Tan <jonathantanmy@google.com>
Cc: git@vger.kernel.org, jrnieder@gmail.com
Subject: Re: [PATCH v2 2/2] wt-status: tolerate dangling marks
Date: Mon, 31 Aug 2020 10:37:26 -0700	[thread overview]
Message-ID: <xmqqzh6ag1ih.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <20200831171732.1199755-1-jonathantanmy@google.com> (Jonathan Tan's message of "Mon, 31 Aug 2020 10:17:32 -0700")

Jonathan Tan <jonathantanmy@google.com> writes:

>> 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.

  reply	other threads:[~2020-08-31 17:37 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13  0:40 [PATCH] wt-status: expand, not dwim, a "detached from" ref Jonathan Tan
2020-05-13  5:33 ` Junio C Hamano
2020-05-13 14:59   ` Junio C Hamano
2020-05-18 22:24     ` Jonathan Tan
2020-08-27  1:47 ` Jonathan Nieder
2020-08-27  2:10   ` Junio C Hamano
2020-08-29  1:02 ` [PATCH v2 0/2] Fix for git checkout @{u} (non-local) then git status Jonathan Tan
2020-08-29  1:02   ` [PATCH v2 1/2] sha1-name: replace unsigned int with option struct Jonathan Tan
2020-08-29 18:44     ` Junio C Hamano
2020-08-29  1:02   ` [PATCH v2 2/2] wt-status: tolerate dangling marks Jonathan Tan
2020-08-29 18:55     ` Junio C Hamano
2020-08-31 17:17       ` Jonathan Tan
2020-08-31 17:37         ` Junio C Hamano [this message]
2020-09-01 22:28 ` [PATCH v3 0/3] Fix for git checkout @{u} (non-local) then git status Jonathan Tan
2020-09-01 22:28   ` [PATCH v3 1/3] sha1-name: replace unsigned int with option struct Jonathan Tan
2020-09-01 22:28   ` [PATCH v3 2/3] refs: move dwim_ref() to header file Jonathan Tan
2020-09-01 22:28   ` [PATCH v3 3/3] wt-status: tolerate dangling marks Jonathan Tan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqzh6ag1ih.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=jrnieder@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).