git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Michael Haggerty <mhagger@alum.mit.edu>
To: Junio C Hamano <gitster@pobox.com>, Stefan Beller <sbeller@google.com>
Cc: "git@vger.kernel.org" <git@vger.kernel.org>,
	Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Subject: Re: [PATCH] refs.h: rename submodule arguments to submodule_path
Date: Fri, 21 Apr 2017 08:32:56 +0200	[thread overview]
Message-ID: <cb1dd8b6-3357-57ff-650d-c55a7eb38d34@alum.mit.edu> (raw)
In-Reply-To: <xmqqy3uu7ee3.fsf@gitster.mtv.corp.google.com>

On 04/21/2017 03:12 AM, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
> 
>> + Junio
> 
> Just like Michael, I do not have strong enough opinion for or
> against this patch to comment on it.
> 
> I do agree with you that it would be a good longer-term direction to
> use "submodule" for a "struct submodule" (i.e. submodule object),
> and call a string that names a submodule either "submodule_name" or
> "submodule_path" depending on how it names one, for maintainability
> of the code.  
> 
> However I am not convinced that this patch is an improvement.  Even
> though parameter names in decls only serve documentation purpose and
> it is even OK to only have type without name there, if we are going
> to _have_ names, it would make sense to match them to the parameter
> names actually used in the implementation.  
> 
> Updating these names used in refs.c would make a very noisy patch,
> of course.  But I am not sure if it is a good middle ground to avoid
> that and to update only refs.h.

One should never infer too much from my silence. As often as not it's
because I'm simply busy with other things.

But in this case Junio's right. I think it is a good idea to use
argument names in declarations as documentation, and I also agree that
it is a minus for the names in the declarations not to agree with the
names in the definition. But the code that would have to be touched
already has a lot of work going on in it, so conflicts would be likely.

I've CCed Duy because I don't know whether he has more plans regarding
submodule references. A natural followup to his recent work would be to
add a feature to the `submodule` module that allows a caller to look up
the `ref_store` object for the submodule. Then client code could use the
`refs_for_each_ref(struct ref_store *, ...)` family of functions to
access such references, and we could get rid of the
`for_each_ref_submodule()` family of functions entirely.

So perhaps the code that this patch touches won't be around long anyway.

Michael


  reply	other threads:[~2017-04-21  6:33 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-12 20:00 [PATCH] refs.h: rename submodule arguments to submodule_path Stefan Beller
2017-04-20 18:21 ` Stefan Beller
2017-04-21  1:12   ` Junio C Hamano
2017-04-21  6:32     ` Michael Haggerty [this message]
2017-04-21  6:42       ` Michael Haggerty
2017-04-21  9:38         ` Duy Nguyen

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=cb1dd8b6-3357-57ff-650d-c55a7eb38d34@alum.mit.edu \
    --to=mhagger@alum.mit.edu \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.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).