git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>
Subject: Re: [PATCH 1/2] builtin/mv: remove get_pathspec()
Date: Thu, 6 Aug 2015 13:59:48 -0700	[thread overview]
Message-ID: <CAGZ79ka=Zw3DmvQHwQXCAd1rekR4AJSG8CDDu9thUmvDppVUZA@mail.gmail.com> (raw)
In-Reply-To: <CAPig+cTn=KLryVJEz7_u7nt2AzB1zzc5y40euk=oP3ZWaaYF3A@mail.gmail.com>

On Thu, Aug 6, 2015 at 1:18 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Thu, Aug 6, 2015 at 2:58 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, Aug 6, 2015 at 11:46 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>>> On Thu, Aug 6, 2015 at 2:27 PM, Stefan Beller <sbeller@google.com> wrote:
>>>> `get_pathspec` is deprecated and builtin/mv.c is its last caller, so
>>>> reimplement `get_pathspec` literally in builtin/mv.c
>>>
>>> Curious. Since this is just moving code around, rather than doing the
>>> actual work to complete the final step as stated by the NEEDSWORK
>>> comment, isn't it just moving the "problem" from one location to
>>> another? Is it worth the code churn?
>>
>> Yeah it is moving around the problem a bit. And the code churn is
>> unfortunate. Though when I was reading the documentation on
>> pathspecs, literally the first sentence was "Do not use get_pathspec,
>> it is out dated". And that was a sad taste for reading documentation.
>
> By loudly warning you about deprecation and, more importantly,
> pointing you at the accepted alternative, this documentation saves you
> from wasting time (both time spent reading and time spent going down a
> dead-end path). It would be a "sad taste" if it warned you quietly
> only at the end of the documentation or not at all.
>
>> It's ok to have such warnings in the docs, but as the first sentence as if
>> there was nothing more important than avoiding the out dated stuff?
>
> From a documentation standpoint, there is nothing more important than
> warning you to avoid it since it is outdated and likely to go away in
> the future.
>
>> I mean I want to understand the actual code and how I can use it, right?
>
> No. It's deprecated and not meant for your use.

I need to be more precise:
I mean I want to learn how to deal with <pathspec> things in code.
Assume I know nothing of the history of that code and its deprecated
earlier attempts. So I would expect a documentation like:

    foo-bar(int arg): bars the foos with the specific thing equal `arg`

    foobaz(int a, int b, int c): Note: this is outdated, bars the foos with ...

in that order, so if I were to start reading from top to bottom, I would find
the current version first and could jump off starting writing code based on
foo-bar.

If I was reading code and wonder: "What does foobaz exactly do? Oh
I know! I'll consult the documentation", then I would find the note about
it being out dated and preferably its suggested new replacement.

However I perceived the documentation I read more like this:

    some words on foo-bar, a bit too shallow (a good overview,
    but I'd like to know more details, so keep reading)

    note on foobaz being outdated

    more words on foo-bar

>
> It's a different matter if you want to understand what the function
> does because you've encountered a call in existing code, but that case
> is covered by the existing documentation still being intact (that is,
> it wasn't removed when the deprecation notice was added).
>
>> And there are different approaches to solving the problem.
>> I could have just reworded or even just rearranged the documentation.
>
> The documentation seems fine as-is.
>
>> The approach I take here includes a bit of code churn, but it moves the
>> problematic pieces all in one spot.
>
> Indeed, I had the "localizing the problem to one spot" argument in
> mind, and even wrote it as an answer to my own question, but deleted
> it before hitting "Send". The counterargument (aside from code churn)
> is, that by leaving it alone, it serves as a good reminder of the
> "problem" and is more likely to get noticed and (perhaps) fixed by
> someone than if it is hidden away in builtin/mv.c.

Having code just in one ugly spot however helps people in the other areas,
where they can see the shiny new thing in action and don't have the mental
burden of seeing a NEEDSWORK comment and thinking about that instead
of the feature they want to add there.

  reply	other threads:[~2015-08-06 21:01 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-08-03 17:53 [PATCH] builtin/mv: Get rid of the last caller of get_pathspec Stefan Beller
2015-08-03 22:48 ` Eric Sunshine
2015-08-06 18:27   ` [PATCH 0/2] Remove get_pathspec finally Stefan Beller
2015-08-06 18:27     ` [PATCH 1/2] builtin/mv: remove get_pathspec() Stefan Beller
2015-08-06 18:46       ` Eric Sunshine
2015-08-06 18:58         ` Stefan Beller
2015-08-06 20:18           ` Eric Sunshine
2015-08-06 20:59             ` Stefan Beller [this message]
2015-08-06 18:27     ` [PATCH 2/2] pathspec: remove deprecated get_pathspec Stefan Beller

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to='CAGZ79ka=Zw3DmvQHwQXCAd1rekR4AJSG8CDDu9thUmvDppVUZA@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=sunshine@sunshineco.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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).