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
---------------------------------------------------------------------------------------------------------------------------
next prev parent 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).