git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Ben Walton <bdwalton@gmail.com>
To: Michael Haggerty <mhagger@alum.mit.edu>
Cc: gitster@pobox.com, git@vger.kernel.org
Subject: Re: [PATCH] Remove the hard coded length limit on variable names in config files
Date: Sun, 30 Sep 2012 19:20:07 +0100	[thread overview]
Message-ID: <CAP30j15wKa7wbLyaLo8omHcAh5u7O=Yt8Tjy4bD3V_0nT1zJPQ@mail.gmail.com> (raw)
In-Reply-To: <5067C51F.6020908@alum.mit.edu>

Hi Michael,

> The patch doesn't apply to the current master; it appears to have been
> built against master 883a2a3504 (2012-02-23) or older.  It will have to
> be rebased to the current master.

Junio had asked that it be based on maint so that's what I (thought
I?) did.  I'm happy to redo it against master if that's better though.

> The preferred format for multiline comments in the git project is
>
>     /*
>      * Truncate the var name back to the section header prior to
>      * grabbing the suffix part of the name and the value.
>      */

Oops; Will fix.

> In the old code, get_base_var() read the string into var and returned
> var's length (or -1 on error).  The fact that the length of var was
> first "reset" to zero is somewhat implicit in the fact that no length
> parameter is being passed to get_base_var().
>
> But in the new version, get_base_var() is passed a strbuf.  Often,
> operations with strbufs append to the strbuf, and this is what I first
> assumed.  It took me a while to realize that get_base_var() calls
> strbuf_reset() before getting to work.  Moreover, get_base_var() still
> returns the length of what it found, which is redundant with a strbuf
> and therefore unexpected.  So when the return value of get_base_var() is
> stored into baselen, it is not really obvious that it is the string's
> length.

Ok, that's a fair criticism.  When I was creating the patch, I thought
that placing
the strbuf_reset in get_base_var() seemed nicer as it matched the
baselen = 0 which
effectively reset the old character array.  Your point is well taken
though and I think
it makes sense to switch things around the way you've suggested.

> Finally, I realize that the MAXNAME constant was not exported and I
> can't find the old length limits documented anywhere, but I nevertheless
> worry a little bit that one of the users of the config API has a
> built-in assumption that names can never be longer than 256 characters
> (for example, a config_fn_t function might try to store the name into a
> fixed-length buffer).  Hopefully such code would never have been written
> or accepted, but...?  If you have thought about this or audited the
> callers, please mention that in your commit message.

I did look through the code to see if anything was relying on fixed
size buffers and I didn't see anything.  I'll update the commit
message with this info too.

I'll send a modified patch shortly.

Thanks for the review!
-Ben
-- 
---------------------------------------------------------------------------------------------------------------------------
Take the risk of thinking for yourself.  Much more happiness,
truth, beauty and wisdom will come to you that way.

-Christopher Hitchens
---------------------------------------------------------------------------------------------------------------------------

  reply	other threads:[~2012-09-30 18:20 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-07  0:47 [PATCH] Prevent git-config from storing section keys that are too long Ben Walton
2012-09-07  1:33 ` Junio C Hamano
2012-09-07  2:34   ` Ben Walton
2012-09-29 10:19   ` [PATCH] Remove the hard coded length limit on variable names in config files Ben Walton
2012-09-30  4:05     ` Michael Haggerty
2012-09-30 18:20       ` Ben Walton [this message]
2012-09-30 19:44         ` Ben Walton
2012-10-01 19:33           ` Junio C Hamano
2012-10-01  3:16         ` Michael Haggerty

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='CAP30j15wKa7wbLyaLo8omHcAh5u7O=Yt8Tjy4bD3V_0nT1zJPQ@mail.gmail.com' \
    --to=bdwalton@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=mhagger@alum.mit.edu \
    /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).