* Re: [PATCH v5 04/24] files-backend: convert git_path() to strbuf_git_path()
From: Michael Haggerty @ 2017-02-28 17:06 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Johannes Schindelin, Ramsay Jones, Stefan Beller,
novalis
In-Reply-To: <20170222140450.30886-5-pclouds@gmail.com>
On 02/22/2017 03:04 PM, Nguyễn Thái Ngọc Duy wrote:
> git_path() and friends are going to be killed in files-backend.c in near
> future. And because there's a risk with overwriting buffer in
> git_path(), let's convert them all to strbuf_git_path(). We'll have
> easier time killing/converting strbuf_git_path() then because we won't
> have to worry about memory management again.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> refs/files-backend.c | 139 +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 106 insertions(+), 33 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 4676525de..435db1293 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> [...]
> @@ -2586,9 +2603,15 @@ static int files_rename_ref(struct ref_store *ref_store,
> int flag = 0, logmoved = 0;
> struct ref_lock *lock;
> struct stat loginfo;
> - int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
> + struct strbuf sb_oldref = STRBUF_INIT;
> + struct strbuf sb_newref = STRBUF_INIT;
> + struct strbuf tmp_renamed_log = STRBUF_INIT;
> + int log, ret;
> struct strbuf err = STRBUF_INIT;
>
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + log = !lstat(sb_oldref.buf, &loginfo);
> + strbuf_release(&sb_oldref);
> if (log && S_ISLNK(loginfo.st_mode))
> return error("reflog for %s is a symlink", oldrefname);
>
> @@ -2602,7 +2625,12 @@ static int files_rename_ref(struct ref_store *ref_store,
> if (!rename_ref_available(oldrefname, newrefname))
> return 1;
>
> - if (log && rename(git_path("logs/%s", oldrefname), git_path(TMP_RENAMED_LOG)))
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> + ret = log && rename(sb_oldref.buf, tmp_renamed_log.buf);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
> + if (ret)
> return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
>
> @@ -2681,13 +2709,19 @@ static int files_rename_ref(struct ref_store *ref_store,
> log_all_ref_updates = flag;
>
> rollbacklog:
> - if (logmoved && rename(git_path("logs/%s", newrefname), git_path("logs/%s", oldrefname)))
> + strbuf_git_path(&sb_newref, "logs/%s", newrefname);
> + strbuf_git_path(&sb_oldref, "logs/%s", oldrefname);
> + if (logmoved && rename(sb_newref.buf, sb_oldref.buf))
> error("unable to restore logfile %s from %s: %s",
> oldrefname, newrefname, strerror(errno));
> + strbuf_git_path(&tmp_renamed_log, TMP_RENAMED_LOG);
> if (!logmoved && log &&
> - rename(git_path(TMP_RENAMED_LOG), git_path("logs/%s", oldrefname)))
> + rename(tmp_renamed_log.buf, sb_oldref.buf))
> error("unable to restore logfile %s from "TMP_RENAMED_LOG": %s",
> oldrefname, strerror(errno));
It feels like you're writing, releasing, re-writing these strbufs more
than necessary. Maybe it would be clearer to set them once, and on
errors set `ret = error()` then jump to a label here...
> + strbuf_release(&sb_newref);
> + strbuf_release(&sb_oldref);
> + strbuf_release(&tmp_renamed_log);
>
...and change this to `return ret`?
> return 1;
> }
> [...]
> @@ -4108,18 +4171,28 @@ static int files_reflog_expire(struct ref_store *ref_store,
>
> static int files_init_db(struct ref_store *ref_store, struct strbuf *err)
> {
> + struct strbuf sb = STRBUF_INIT;
> +
> /* Check validity (but we don't need the result): */
> files_downcast(ref_store, 0, "init_db");
>
> /*
> * Create .git/refs/{heads,tags}
> */
> - safe_create_dir(git_path("refs/heads"), 1);
> - safe_create_dir(git_path("refs/tags"), 1);
> + strbuf_git_path(&sb, "refs/heads");
> + safe_create_dir(sb.buf, 1);
> + strbuf_reset(&sb);
> + strbuf_git_path(&sb, "refs/tags");
> + safe_create_dir(sb.buf, 1);
> + strbuf_reset(&sb);
> if (get_shared_repository()) {
> - adjust_shared_perm(git_path("refs/heads"));
> - adjust_shared_perm(git_path("refs/tags"));
> + strbuf_git_path(&sb, "refs/heads");
> + adjust_shared_perm(sb.buf);
> + strbuf_reset(&sb);
> + strbuf_git_path(&sb, "refs/tags");
> + adjust_shared_perm(sb.buf);
> }
> + strbuf_release(&sb);
> return 0;
> }
It looks to me like `safe_create_dir()` already has the ability to
`adjust_shared_perm()`, or am I missing something? (I realize that this
is preexisting code.)
Michael
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: René Scharfe @ 2017-02-28 16:38 UTC (permalink / raw)
To: Jeff King, Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <20170228142802.hu5esthnqdsgc2po@sigill.intra.peff.net>
Am 28.02.2017 um 15:28 schrieb Jeff King:
> On Mon, Feb 27, 2017 at 10:30:20PM +0100, Johannes Schindelin wrote:
>
>> One notable fallout of this patch series is that on 64-bit Linux (and
>> other platforms where `unsigned long` is 64-bit), we now limit the range
>> of dates to LONG_MAX (i.e. the *signed* maximum value). This needs to be
>> done as `time_t` can be signed (and indeed is at least on my Ubuntu
>> setup).
>>
>> Obviously, I think that we can live with that, and I hope that all
>> interested parties agree.
>
> I do not just agree, but I think the move to a signed timestamp is a big
> improvement. Git's object format is happy to represent times before
> 1970, but the code is not. I know this has been a pain for people who
> import ancient histories into Git.
>
> It looks from the discussion like the sanest path forward is our own
> signed-64bit timestamp_t. That's unfortunate compared to using the
> standard time_t, but hopefully it would reduce the number of knobs (like
> TIME_T_IS_INT64) in the long run.
Glibc will get a way to enable 64-bit time_t on 32-bit platforms
eventually (https://sourceware.org/glibc/wiki/Y2038ProofnessDesign).
Can platforms that won't provide a 64-bit time_t by 2038 be actually
used at that point? How would we get time information on them? How
would a custom timestamp_t help us?
Regarding the need for knobs: We could let the compiler chose between
strtoll() and strtol() based on the size of time_t, in an inline
function. The maximum value can be calculated using its size as well.
And we could use PRIdMAX and cast to intmax_t for printing.
René
^ permalink raw reply
* format-patch subject-prefix gets truncated when using the --numbered flag
From: Adrian Dudau @ 2017-02-28 15:59 UTC (permalink / raw)
To: git@vger.kernel.org
Hello,
I noticed that the --subject-prefix string gets truncated sometimes,
but only when using the --numbered flat. Here's an example:
addu@sestofb11:/data/fb/addu/git$ export longm="very very very very
very very very very very very very very very very long prefix"
addu@sestofb11:/data/fb/addu/git$ git format-patch -1 --subject-
prefix="$longm][PATCH"
As expected, in the generated patch file we have:
Subject: [very very very very very very very very very very very very
very very long prefix][PATCH]
First batch after 2.12
But now, if I pass the --numbered flag too:
addu@sestofb11:/data/fb/addu/git$ git format-patch -1 --numbered --
subject-prefix="$longm][PATCH"
In the generated patch file we get this:
Subject: [very very very very very very very very very very verFirst
batch
after 2.12
This is happening on the latest master branch, so I dug through the
code and tracked the issue to this piece of code in log-tree.c:
if (opt->total > 0) {
static char buffer[64];
snprintf(buffer, sizeof(buffer),
"Subject: [%s%s%0*d/%d] ",
opt->subject_prefix,
*opt->subject_prefix ? " " : "",
digits_in_number(opt->total),
opt->nr, opt->total);
subject = buffer;
} else if (opt->total == 0 && opt->subject_prefix && *opt-
>subject_prefix) {
static char buffer[256];
snprintf(buffer, sizeof(buffer),
"Subject: [%s] ",
opt->subject_prefix);
subject = buffer;
} else {
subject = "Subject: ";
}
Apparently the size of the "buffer" var is different in the two
situations. Anybody knows if this is by design or just an old
oversight?
I can send a patch to fix it but I'm not very familiar with the git
code and I'm afraid some hidden consequence I don't see right now.
Cheers,
--Adrian
^ permalink raw reply
* gpg verify git sub modules useful?
From: Patrick Schleizer @ 2017-02-28 15:50 UTC (permalink / raw)
To: git; +Cc: Whonix-devel
When using git submodules, is there value in iterating about the git
submodules running "git verfiy-commit HEAD" or would that be already
covered by the git submodule verification?
Cheers,
Patrick
^ permalink raw reply
* RE: Unconventional roles of git
From: Randall S. Becker @ 2017-02-28 15:27 UTC (permalink / raw)
To: 'ankostis'; +Cc: 'Git Mailing List', 'Jason Cooper'
In-Reply-To: <CA+dhYEXtkrZk4g7KHUK_JPWCE=o4cCC9hU8inp1GJuk-V9gtew@mail.gmail.com>
>From: ankostis [mailto:ankostis@gmail.com]
>Sent: February 28, 2017 8:01 AM
>To: Randall S. Becker <rsbecker@nexbridge.com>
>Cc: Git Mailing List <git@vger.kernel.org>; Jason Cooper <git@lakedaemon.net>
>Subject: Re: Unconventional roles of git
>On 27 February 2017 at 20:16, Randall S. Becker <mailto:rsbecker@nexbridge.com> wrote:
>> On February 26, 2017 6:52 AM, Kostis Anagnostopoulos <mailto:ankostis@gmail.com> worte:
>>> On 26 February 2017 at 02:13, Jason Cooper <mailto:git@lakedaemon.net> wrote:
>>> > As someone looking to deploy (and having previously deployed) git in
>>> > unconventional roles, I'd like to add ...
>>>
>>> We are developing a distributed storage for type approval files regarding all
>>> vehicles registered in Europe.[1] To ensure integrity even after 10 or 30
>>> years, the hash of a commit of these files (as contained in a tag) are to be
>>> printed on the the paper certificates.
>>>
>>> - Can you provide some hints for other similar unconventional roles of git?
>>> - Any other comment on the above usage of git are welcomed.
>>
>> I am involved in managing manufacturing designs and parts configurations and approvals with git being intimately involved in the process of developing and deploying tested designs to >computerized manufacturing environments. It's pretty cool actually to see things become real.
>Thanks for the tip.
>Can you provide some links or the legislation involved?
I have not done much in the way of write-ups other than PowerPoint-based training material for the companies involved. So far, this does not seem to be subject to regulatory or legislation - but that depends on what is being manufactured. In the current situation, I'm helping organize cabinet parts, components, GCode, optimizations, and other arcane artifacts in the woodworking community for CNC and related process support. It is an evolving domain. I do wish that Cloud providers like Atlassian would provide more comprehensive integrated code reviews (a.k.a. Gerrit) for some of this work. Surprisingly that's a harder sell to dedicate a server internally.
Cheers,
Randall
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Johannes Schindelin @ 2017-02-28 15:01 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano
In-Reply-To: <20170228142802.hu5esthnqdsgc2po@sigill.intra.peff.net>
Hi Peff,
On Tue, 28 Feb 2017, Jeff King wrote:
> On Mon, Feb 27, 2017 at 10:30:20PM +0100, Johannes Schindelin wrote:
>
> > One notable fallout of this patch series is that on 64-bit Linux (and
> > other platforms where `unsigned long` is 64-bit), we now limit the
> > range of dates to LONG_MAX (i.e. the *signed* maximum value). This
> > needs to be done as `time_t` can be signed (and indeed is at least on
> > my Ubuntu setup).
> >
> > Obviously, I think that we can live with that, and I hope that all
> > interested parties agree.
>
> I do not just agree, but I think the move to a signed timestamp is a big
> improvement. Git's object format is happy to represent times before
> 1970, but the code is not. I know this has been a pain for people who
> import ancient histories into Git.
>
> It looks from the discussion like the sanest path forward is our own
> signed-64bit timestamp_t. That's unfortunate compared to using the
> standard time_t, but hopefully it would reduce the number of knobs (like
> TIME_T_IS_INT64) in the long run.
Boy am I happy that I did not go ahead and changed the code to use
uint64_t yet...
I'll let the dust settle a bit and then make further changes and send out
v2.
Ciao,
Dscho
^ permalink raw reply
* [PATCH 2/8] strbuf_branchname: drop return value
From: Jeff King @ 2017-02-28 12:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
The return value from strbuf_branchname() is confusing and
useless: it's 0 if the whole name was consumed by an @-mark,
but otherwise is the length of the original name we fed.
No callers actually look at the return value, so let's just
get rid of it.
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 5 +----
strbuf.h | 2 +-
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index 28865b3a1..4c1e91184 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1279,17 +1279,14 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
return -1;
}
-int strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name)
{
int len = strlen(name);
int used = interpret_branch_name(name, len, sb);
- if (used == len)
- return 0;
if (used < 0)
used = 0;
strbuf_add(sb, name + used, len - used);
- return len;
}
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
diff --git a/strbuf.h b/strbuf.h
index cf1b5409e..47df0500d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -560,7 +560,7 @@ static inline void strbuf_complete_line(struct strbuf *sb)
strbuf_complete(sb, '\n');
}
-extern int strbuf_branchname(struct strbuf *sb, const char *name);
+extern void strbuf_branchname(struct strbuf *sb, const char *name);
extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* Re: 'git submodules update' ignores credential.helper config of the parent repository
From: Jeff King @ 2017-02-28 14:37 UTC (permalink / raw)
To: Stefan Beller; +Cc: Dmitry Neverov, Duy Nguyen, Junio C Hamano, Git List
In-Reply-To: <CAGZ79ka8saQMKeutE415WxOQ71MnEw1A4uV3b0Pa4gcehx8pdw@mail.gmail.com>
On Mon, Feb 27, 2017 at 11:09:12AM -0800, Stefan Beller wrote:
> For worktrees these multiple config files sounded like
> the obvious solution, but I wonder if there was also
> some bike shedding about other solutions?
>
> I could imagine that we would want to have attributes
> for specific configuration, e.g.:
>
> --8<--
> [core]
> repositoryformatversion = 0
> filemode = true
> bare = false
> logallrefupdates = true
> [remote "origin"]
> url = git://github.com/gitster/git
> fetch = +refs/heads/*:refs/remotes/origin/*
> [attribute "submodules"]
> read = true
> # this will be read and respected by submodules as well:
> [url."internal-git-miror"]
> insteadOf = github.com
> [attribute "submodules"]
> read = false
> # This (and the beginning of this file) will not be respected
> # by submodules
> [credential]
> helper =
> -->8--
>
> This would change the semantics of a config file as the attribute for
> each setting depends on the location (was attribute.FOO.read =
> {true, false} read before).
I'm not enthused by this, just because there is a hidden dependency
between attribute.* sections and other ones. They _look_ like regular
config keys, but they really aren't.
I have a feeling that something like this would create unwelcome corner
cases in the config-writer, which is otherwise does not have to care
about which existing section of a file it adds a key to.
-Peff
^ permalink raw reply
* Re: [PATCH 0/6] Use time_t
From: Jeff King @ 2017-02-28 14:28 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <cover.1488231002.git.johannes.schindelin@gmx.de>
On Mon, Feb 27, 2017 at 10:30:20PM +0100, Johannes Schindelin wrote:
> One notable fallout of this patch series is that on 64-bit Linux (and
> other platforms where `unsigned long` is 64-bit), we now limit the range
> of dates to LONG_MAX (i.e. the *signed* maximum value). This needs to be
> done as `time_t` can be signed (and indeed is at least on my Ubuntu
> setup).
>
> Obviously, I think that we can live with that, and I hope that all
> interested parties agree.
I do not just agree, but I think the move to a signed timestamp is a big
improvement. Git's object format is happy to represent times before
1970, but the code is not. I know this has been a pain for people who
import ancient histories into Git.
It looks from the discussion like the sanest path forward is our own
signed-64bit timestamp_t. That's unfortunate compared to using the
standard time_t, but hopefully it would reduce the number of knobs (like
TIME_T_IS_INT64) in the long run.
-Peff
^ permalink raw reply
* Re: [BUG] branch renamed to 'HEAD'
From: Jeff King @ 2017-02-28 12:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228005302.k6fyfinaxyl3ti76@sigill.intra.peff.net>
On Mon, Feb 27, 2017 at 07:53:02PM -0500, Jeff King wrote:
> On Mon, Feb 27, 2017 at 04:33:36PM -0800, Junio C Hamano wrote:
>
> > A flag to affect the behaviour (as opposed to &flag as a secondary
> > return value, like Peff's patch does) can be made to work. Perhaps
> > a flag that says "keep the input as is if the result is not a local
> > branch name" would pass an input "@" intact and that may be
> > sufficient to allow "git branch -m @" to rename the current branch
> > to "@" (I do not think it is a sensible rename, though ;-). But
> > probably some callers need to keep the original input and compare
> > with the result to see if we expanded anything if we go that route.
> > At that point, I am not sure if there are much differences in the
> > ease of use between the two approaches.
>
> I just went into more detail in my reply to Jacob, but I do think this
> is a workable approach (and fortunately we seem to have banned bare "@"
> as a name, along with anything containing "@{}", so I think we would end
> up rejecting these nonsense names).
>
> I'll see if I can work up a patch. We'll still need to pass the flag
> around through the various functions, but at least it will be a flag and
> not a confusing negated out-parameter.
OK, I have a series which fixes this (diffstat below). When I audited
the other callers of interpret_branch_name() and strbuf_branchname(), it
turned out to be even more complicated. The callers basically fall into
a few buckets:
1. Callers like get_sha1() and merge_name() pass the result to
dwim_ref(), and are prepared to handle anything.
2. Some callers stick "refs/heads/" in front of the result, and
obviously only want local names. Most of git-branch and
git-checkout fall into this boat.
3. "git branch -d" can delete local _or_ remote branches, depending on
the "-r" flag. So the expansion it wants varies, and we need to
handle "just local" or "just remote".
So I converted the "only_branch" flag to an "allowed" bit-field. No
callers actually ask for more than a single type at once, but it was
easy to do it that way. It serves all of the callers, and will easily
adapt for the future (e.g., if "git branch -a -d" were ever allowed).
[1/8]: interpret_branch_name: move docstring to header file
[2/8]: strbuf_branchname: drop return value
[3/8]: strbuf_branchname: add docstring
[4/8]: interpret_branch_name: allow callers to restrict expansions
[5/8]: t3204: test git-branch @-expansion corner cases
[6/8]: branch: restrict @-expansions when deleting
[7/8]: strbuf_check_ref_format(): expand only local branches
[8/8]: checkout: restrict @-expansions when finding branch
builtin/branch.c | 5 +-
builtin/checkout.c | 2 +-
builtin/merge.c | 2 +-
cache.h | 32 ++++++++-
refs.c | 2 +-
revision.c | 2 +-
sha1_name.c | 76 ++++++++++-----------
strbuf.h | 21 +++++-
t/t3204-branch-name-interpretation.sh | 122 ++++++++++++++++++++++++++++++++++
9 files changed, 220 insertions(+), 44 deletions(-)
create mode 100755 t/t3204-branch-name-interpretation.sh
-Peff
^ permalink raw reply
* Re: [PATCH] http: attempt updating base URL only if no error
From: Jeff King @ 2017-02-28 13:28 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <20170228025311.6347-1-jonathantanmy@google.com>
On Mon, Feb 27, 2017 at 06:53:11PM -0800, Jonathan Tan wrote:
> http.c supports HTTP redirects of the form
>
> http://foo/info/refs?service=git-upload-pack
> -> http://anything
> -> http://bar/info/refs?service=git-upload-pack
>
> (that is to say, as long as the Git part of the path and the query
> string is preserved in the final redirect destination, the intermediate
> steps can have any URL). However, if one of the intermediate steps
> results in an HTTP exception, a confusing "unable to update url base
> from redirection" message is printed instead of a Curl error message
> with the HTTP exception code.
Right, your patch makes sense. A real HTTP error should take precedence
over the url-update trickery.
Acked-by: Jeff King <peff@peff.net>
> Therefore, teach http to check the HTTP status before attempting to
> check if only the "base" part of the URL differed. This commit teaches
> http_request_reauth to return early without updating options->base_url
> upon an error; the only invoker of this function that passes a non-NULL
> "options" is remote-curl.c (through "http_get_strbuf"), which only uses
> options->base_url for an informational message in the situations that
> this commit cares about (that is, when the return value is not HTTP_OK).
Running your included test, we get:
fatal: unable to access 'http://127.0.0.1:5550/redir-to/502/': The
requested URL returned error: 502
but the error really happened in the intermediate step. I wonder if we
should show the effective_url in that case, as it's more likely to
pinpoint the problem. OTOH, we do not mention the intermediate redirect
at all, so they might be confused about where that URL came from. If you
really want to debug HTTP confusion, you should use GIT_TRACE_CURL.
At any rate, that's orthogonal to your patch, which is obviously the
right thing to do.
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: brian m. carlson @ 2017-02-28 13:25 UTC (permalink / raw)
To: René Scharfe; +Cc: Duy Nguyen, Joey Hess, Git Mailing List
In-Reply-To: <13bb2033-fedd-d7da-f584-21a3142852d3@web.de>
[-- Attachment #1: Type: text/plain, Size: 3137 bytes --]
On Mon, Feb 27, 2017 at 02:29:18PM +0100, René Scharfe wrote:
> Am 25.02.2017 um 20:04 schrieb brian m. carlson:
> >>> So I think that the current scope left is best estimated by the
> >>> following command:
> >>>
> >>> git grep -P 'unsigned char\s+(\*|.*20)' | grep -v '^Documentation'
> >>>
> >>> So there are approximately 1200 call sites left, which is quite a bit of
> >>> work. I estimate between the work I've done and other people's
> >>> refactoring work (such as the refs backend refactor), we're about 40%
> >>> done.
> >
> > As a note, I've been working on this pretty much nonstop since the
> > collision announcement was made. After another 27 commits, I've got it
> > down from 1244 to 1119.
> >
> > I plan to send another series out sometime after the existing series has
> > hit next. People who are interested can follow the object-id-part*
> > branches at https://github.com/bk2204/git.
>
> Perhaps the following script can help a bit; it converts local and static
> variables in specified files. It's just a simplistic parser which can get
> at least shadowing variables, strings and comments wrong, so its results
> need to be reviewed carefully.
>
> I failed to come up with an equivalent Coccinelle patch so far. :-/
>
> René
>
>
> #!/bin/sh
> while test $# -gt 0
> do
> file="$1"
> tmp="$file.new"
> test -f "$file" &&
> perl -e '
> use strict;
> my %indent;
> my %old;
> my %new;
> my $in_struct = 0;
> while (<>) {
> if (/^(\s*)}/) {
> my $len = length $1;
> foreach my $key (keys %indent) {
> if ($len < length($indent{$key})) {
> delete $indent{$key};
> delete $old{$key};
> delete $new{$key};
> }
> }
> $in_struct = 0;
> }
> if (!$in_struct and /^(\s*)(static )?unsigned char (\w+)\[20\];$/) {
> my $prefix = "$1$2";
> my $name = $3;
> $indent{$.} = $1;
> $old{$.} = qr/(?<!->)(?<!\.)(?<!-)\b$name\b/;
> $name =~ s/sha1/oid/;
> print $prefix . "struct object_id " . $name . ";\n";
> $new{$.} = $name . ".hash";
> next;
> }
> if (/^(\s*)(static )?struct (\w+ )?\{$/) {
> $in_struct = 1;
> }
> if (!$in_struct and ! /\/\*/) {
> foreach my $key (keys %indent) {
> s/$old{$key}/$new{$key}/g;
> }
> }
> print;
> }
> ' "$file" >"$tmp" &&
> mv "$tmp" "$file" ||
> exit 1
> shift
> done
I'll see how it works. I'm currently in New Orleans visiting a friend
until Thursday, so I'll have less time than normal to look at these, but
I'll definitely give it a try.
Most of the issue is not the actual conversion, but finding the right
order in which to convert functions. For example, the object-id-part8
branch on my GitHub account converts parse_object, but
parse_tree_indirect has to be converted before you can do parse_object.
That leads to another handful of patches that have to be done.
--
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* [PATCH 5/8] t3204: test git-branch @-expansion corner cases
From: Jeff King @ 2017-02-28 12:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
git-branch feeds the branch names from the command line to
strbuf_branchname(), but we do not yet tell that function
which kinds of expansions should be allowed. Let's create a
set of tests that cover both the allowed and disallowed
cases.
That shows off some breakages where we currently create or
delete the wrong ref (and will make sure that we do not
break any cases that _should_ be working when we do add more
restrictions).
Note that we check branch creation and deletion, but do not
bother with renames. Those follow the same code path as
creation.
Signed-off-by: Jeff King <peff@peff.net>
---
t/t3204-branch-name-interpretation.sh | 112 ++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
create mode 100755 t/t3204-branch-name-interpretation.sh
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
new file mode 100755
index 000000000..2fe696ba6
--- /dev/null
+++ b/t/t3204-branch-name-interpretation.sh
@@ -0,0 +1,112 @@
+#!/bin/sh
+
+test_description='interpreting exotic branch name arguments
+
+Branch name arguments are usually names which are taken to be inside of
+refs/heads/, but we interpret some magic syntax like @{-1}, @{upstream}, etc.
+This script aims to check the behavior of those corner cases.
+'
+. ./test-lib.sh
+
+expect_branch() {
+ git log -1 --format=%s "$1" >actual &&
+ echo "$2" >expect &&
+ test_cmp expect actual
+}
+
+expect_deleted() {
+ test_must_fail git rev-parse --verify "$1"
+}
+
+test_expect_success 'set up repo' '
+ test_commit one &&
+ test_commit two &&
+ git remote add origin foo.git
+'
+
+test_expect_success 'update branch via @{-1}' '
+ git branch previous one &&
+
+ git checkout previous &&
+ git checkout master &&
+
+ git branch -f @{-1} two &&
+ expect_branch previous two
+'
+
+test_expect_success 'update branch via local @{upstream}' '
+ git branch local one &&
+ git branch --set-upstream-to=local &&
+
+ git branch -f @{upstream} two &&
+ expect_branch local two
+'
+
+test_expect_failure 'disallow updating branch via remote @{upstream}' '
+ git update-ref refs/remotes/origin/remote one &&
+ git branch --set-upstream-to=origin/remote &&
+
+ test_must_fail git branch -f @{upstream} two
+'
+
+test_expect_success 'create branch with pseudo-qualified name' '
+ git branch refs/heads/qualified two &&
+ expect_branch refs/heads/refs/heads/qualified two
+'
+
+test_expect_success 'delete branch via @{-1}' '
+ git branch previous-del &&
+
+ git checkout previous-del &&
+ git checkout master &&
+
+ git branch -D @{-1} &&
+ expect_deleted previous-del
+'
+
+test_expect_success 'delete branch via local @{upstream}' '
+ git branch local-del &&
+ git branch --set-upstream-to=local-del &&
+
+ git branch -D @{upstream} &&
+ expect_deleted local-del
+'
+
+test_expect_success 'delete branch via remote @{upstream}' '
+ git update-ref refs/remotes/origin/remote-del two &&
+ git branch --set-upstream-to=origin/remote-del &&
+
+ git branch -r -D @{upstream} &&
+ expect_deleted origin/remote-del
+'
+
+# Note that we create two oddly named local branches here. We want to make
+# sure that we do not accidentally delete either of them, even if
+# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
+test_expect_failure 'delete @{upstream} expansion matches -r option' '
+ git update-ref refs/remotes/origin/remote-del two &&
+ git branch --set-upstream-to=origin/remote-del &&
+ git update-ref refs/heads/origin/remote-del two &&
+ git update-ref refs/heads/remotes/origin/remote-del two &&
+
+ test_must_fail git branch -D @{upstream} &&
+ expect_branch refs/heads/origin/remote-del two &&
+ expect_branch refs/heads/remotes/origin/remote-del two
+'
+
+# The thing we are testing here is that "@" is the real branch refs/heads/@,
+# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
+# sane thing, but it _is_ technically allowed for now. If we disallow it, these
+# can be switched to test_must_fail.
+test_expect_failure 'create branch named "@"' '
+ git branch -f @ one &&
+ expect_branch refs/heads/@ one
+'
+
+test_expect_failure 'delete branch named "@"' '
+ git update-ref refs/heads/@ two &&
+ git branch -D @ &&
+ expect_deleted refs/heads/@
+'
+
+test_done
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* Re: [PATCH 1/2] wrapper.c: remove unused git_mkstemp() function
From: Jeff King @ 2017-02-28 13:12 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <976c40d1-42dc-1fc5-45d5-05bdcf04d97e@ramsayjones.plus.com>
On Tue, Feb 28, 2017 at 01:24:10AM +0000, Ramsay Jones wrote:
> The last caller of git_mkstemp() was removed in commit 6fec0a89
> ("verify_signed_buffer: use tempfile object", 16-06-2016). Since
> the introduction of the 'tempfile' APIs, along with git_mkstemp_mode,
> it is unlikely that new callers will materialize. Remove the dead
> code.
Yeah, I think the tempfile API is a better choice for anybody who wants
to add a new call. Removing the temptation (and the dead code) is a good
move.
-Peff
^ permalink raw reply
* Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
From: Jeff King @ 2017-02-28 12:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228122338.xkefanyhtwbomoit@sigill.intra.peff.net>
On Tue, Feb 28, 2017 at 07:23:38AM -0500, Jeff King wrote:
> > -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> > +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
> > + unsigned allowed)
> > {
> > char *at;
> > const char *start;
> > @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> > if (len == namelen)
> > return len; /* consumed all */
> > else
> > - return reinterpret(name, namelen, len, buf);
> > + return reinterpret(name, namelen, len, buf, allowed);
> > }
>
> It's hard to see from this context, but a careful reader may note that
> we do not check "allowed" at all before calling
> interpret_nth_prior_checkout(). This is looking for branch names via
> HEAD, so I don't think it can ever return anything but a local name.
>
> Which, hmm. I guess was valid when the flag was "only_branches", but
> would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if
>
> git branch -r -D @{-1}
>
> incorrectly deletes refs/remotes/origin/master if you previously had
> refs/heads/origin/master checked out.
The answer is "yes", it's broken. So interpret_branch_name() should do
an "allowed" check before expanding the nth-prior. The fix should be
easy, especially on top of the earlier 426f76595 (which, incidentally, I
already based this series on).
I'll hold off on re-rolling to see if it collects any other review.
-Peff
^ permalink raw reply
* Re: [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
From: Jeff King @ 2017-02-28 12:23 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228121434.2dhngs4peq5acic2@sigill.intra.peff.net>
On Tue, Feb 28, 2017 at 07:14:34AM -0500, Jeff King wrote:
> However, out-parameters make calling interface somewhat
> cumbersome. Instead, let's do the opposite: let the caller
> tell us which elements to expand. That's easier to pass in,
> and none of the callers give more precise error messages
> than "@{upstream} isn't a valid branch name" anyway (which
> should be sufficient).
Two things to call attention to:
> -extern int interpret_branch_name(const char *str, int len, struct strbuf *);
> + *
> + * If "allowed" is non-zero, it is a treated as a bitfield of allowable
> + * expansions: local branches ("refs/heads/"), remote branches
> + * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
> + * allowed, even ones to refs outside of those namespaces.
> + */
> +#define INTERPRET_BRANCH_LOCAL (1<<0)
> +#define INTERPRET_BRANCH_REMOTE (1<<1)
> +#define INTERPRET_BRANCH_HEAD (1<<2)
Is the "0 allows everything, but any bit turns on restrictions"
convention too confusing? It's convenient to use in the callers which do
not need restrictions, but we could add an INTERPRET_BRANCH_ALL flag if
that is more clear (but note that it _isn't_ just bitwise-AND of the
other flags, because technically an @{upstream} could point to
"refs/foo" or some other location).
> -int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> +int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
> + unsigned allowed)
> {
> char *at;
> const char *start;
> @@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
> if (len == namelen)
> return len; /* consumed all */
> else
> - return reinterpret(name, namelen, len, buf);
> + return reinterpret(name, namelen, len, buf, allowed);
> }
It's hard to see from this context, but a careful reader may note that
we do not check "allowed" at all before calling
interpret_nth_prior_checkout(). This is looking for branch names via
HEAD, so I don't think it can ever return anything but a local name.
Which, hmm. I guess was valid when the flag was "only_branches", but
would not be valid under INTERPRET_BRANCH_REMOTE. I wonder if
git branch -r -D @{-1}
incorrectly deletes refs/remotes/origin/master if you previously had
refs/heads/origin/master checked out.
-Peff
^ permalink raw reply
* [PATCH 7/8] strbuf_check_ref_format(): expand only local branches
From: Jeff King @ 2017-02-28 12:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
This function asks strbuf_branchname() to expand any @-marks
in the branchname, and then we blindly stick refs/heads/ in
front of the result. This is obviously nonsense if the
expansion is "HEAD" or a ref in refs/remotes/.
The most obvious end-user effect is that creating or
renaming a branch with an expansion may have confusing
results (e.g., creating refs/heads/origin/master from
"@{upstream}" when the operation should be disallowed).
We can fix this by telling strbuf_branchname() that we are
only interested in local expansions. Any unexpanded bits are
then fed to check_ref_format(), which either disallows them
(in the case of "@{upstream}") or lets them through
("refs/heads/@" is technically valid, if a bit silly).
Signed-off-by: Jeff King <peff@peff.net>
---
sha1_name.c | 2 +-
t/t3204-branch-name-interpretation.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index b21997c29..c0cfb69a4 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1317,7 +1317,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
{
- strbuf_branchname(sb, name, 0);
+ strbuf_branchname(sb, name, INTERPRET_BRANCH_LOCAL);
if (name[0] == '-')
return -1;
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index c8fec5b8c..6115ad504 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -42,7 +42,7 @@ test_expect_success 'update branch via local @{upstream}' '
expect_branch local two
'
-test_expect_failure 'disallow updating branch via remote @{upstream}' '
+test_expect_success 'disallow updating branch via remote @{upstream}' '
git update-ref refs/remotes/origin/remote one &&
git branch --set-upstream-to=origin/remote &&
@@ -98,7 +98,7 @@ test_expect_success 'delete @{upstream} expansion matches -r option' '
# and not refs/heads/HEAD. These tests should not imply that refs/heads/@ is a
# sane thing, but it _is_ technically allowed for now. If we disallow it, these
# can be switched to test_must_fail.
-test_expect_failure 'create branch named "@"' '
+test_expect_success 'create branch named "@"' '
git branch -f @ one &&
expect_branch refs/heads/@ one
'
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* [PATCH 6/8] branch: restrict @-expansions when deleting
From: Jeff King @ 2017-02-28 12:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
We use strbuf_branchname() to expand the branch name from
the command line, so you can delete the branch given by
@{-1}, for example. However, we allow other nonsense like
"@", and we do not respect our "-r" flag (so we may end up
deleting an oddly-named local ref instead of a remote one).
We can fix this by passing the appropriate "allowed" flag to
strbuf_branchname().
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/branch.c | 5 ++++-
t/t3204-branch-name-interpretation.sh | 4 ++--
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index cf0ece55d..291fe90de 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -191,17 +191,20 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
int ret = 0;
int remote_branch = 0;
struct strbuf bname = STRBUF_INIT;
+ unsigned allowed_interpret;
switch (kinds) {
case FILTER_REFS_REMOTES:
fmt = "refs/remotes/%s";
/* For subsequent UI messages */
remote_branch = 1;
+ allowed_interpret = INTERPRET_BRANCH_REMOTE;
force = 1;
break;
case FILTER_REFS_BRANCHES:
fmt = "refs/heads/%s";
+ allowed_interpret = INTERPRET_BRANCH_LOCAL;
break;
default:
die(_("cannot use -a with -d"));
@@ -216,7 +219,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
char *target = NULL;
int flags = 0;
- strbuf_branchname(&bname, argv[i], 0);
+ strbuf_branchname(&bname, argv[i], allowed_interpret);
free(name);
name = mkpathdup(fmt, bname.buf);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 2fe696ba6..c8fec5b8c 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -83,7 +83,7 @@ test_expect_success 'delete branch via remote @{upstream}' '
# Note that we create two oddly named local branches here. We want to make
# sure that we do not accidentally delete either of them, even if
# shorten_unambiguous_ref() tweaks the name to avoid ambiguity.
-test_expect_failure 'delete @{upstream} expansion matches -r option' '
+test_expect_success 'delete @{upstream} expansion matches -r option' '
git update-ref refs/remotes/origin/remote-del two &&
git branch --set-upstream-to=origin/remote-del &&
git update-ref refs/heads/origin/remote-del two &&
@@ -103,7 +103,7 @@ test_expect_failure 'create branch named "@"' '
expect_branch refs/heads/@ one
'
-test_expect_failure 'delete branch named "@"' '
+test_expect_success 'delete branch named "@"' '
git update-ref refs/heads/@ two &&
git branch -D @ &&
expect_deleted refs/heads/@
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* Re: [PATCH 2/2] docs/diffcore: unquote "Complete Rewrites" in headers
From: Jeff King @ 2017-02-28 12:37 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Patrick Steinhardt
In-Reply-To: <576fa5be072ebd503aa553ef24290c77ed34eeb1.1488272203.git.patrick.steinhardt@elego.de>
On Tue, Feb 28, 2017 at 09:59:05AM +0100, Patrick Steinhardt wrote:
> The gitdiffcore documentation quotes the term "Complete Rewrites" in
> headers for no real gain. This would make sense if the term could be
> easily confused if not properly grouped together. But actually, the term
> is quite obvious and thus does not really need any quoting, especially
> regarding that it is not used anywhere else.
>
> But more importanly, this brings up a bug when rendering man pages: when
> trying to render quotes inside of a section header, we end up with
> quotes which have been misaligned to the end of line. E.g.
>
> diffcore-break: For Splitting Up Complete Rewrites
> --------------------------------------------------
>
> renders as
>
> DIFFCORE-BREAK: FOR SPLITTING UP COMPLETE REWRITES""
>
> , which is obviously wrong. While this is fixable for the man pages by
> using double-quotes (e.g. ""COMPLETE REWRITES""), this again breaks it
> for our generated HTML pages.
>
> So fix the issue by simply dropping quotes inside of section headers,
> which is currently only done for the term "Complete Rewrites".
Thanks for a nice explanation of the issue. I was curious whether
asciidoctor gets this right. It does, though I suppose that's because we
only look at the HTML output. It sounds like the issue is in the
docbook->roff path.
At any rate, I agree with your analysis. It's not worth futzing with the
formatting when it reads just as well without the quotes.
-Peff
^ permalink raw reply
* [PATCH 8/8] checkout: restrict @-expansions when finding branch
From: Jeff King @ 2017-02-28 12:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
When we parse "git checkout $NAME", we try to interpret
$NAME as a local branch-name. If it is, then we point HEAD
to that branch. Otherwise, we detach the HEAD at whatever
commit $NAME points to.
We do the interpretation by calling strbuf_branchname(), and
then blindly sticking "refs/heads/" on the front. This leads
to nonsense results when expansions like "@{upstream}" or
"@" point to something besides a local branch. We end up
with a local branch name like "refs/heads/origin/master" or
"refs/heads/HEAD".
Normally this has no user-visible effect because those
branches don't exist, and so we fallback to feeding the
result to get_sha1(), which resolves them correctly.
But as the new test in t3204 shows, there are corner cases
where the effect is observable, and we check out the wrong
local branch rather than detaching to the correct one.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/checkout.c | 2 +-
t/t3204-branch-name-interpretation.sh | 10 ++++++++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 05eefd994..81f07c3ef 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_branchname(&buf, branch->name, 0);
+ strbuf_branchname(&buf, branch->name, INTERPRET_BRANCH_LOCAL);
if (strcmp(buf.buf, branch->name))
branch->name = xstrdup(buf.buf);
strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
diff --git a/t/t3204-branch-name-interpretation.sh b/t/t3204-branch-name-interpretation.sh
index 6115ad504..b8e396009 100755
--- a/t/t3204-branch-name-interpretation.sh
+++ b/t/t3204-branch-name-interpretation.sh
@@ -109,4 +109,14 @@ test_expect_success 'delete branch named "@"' '
expect_deleted refs/heads/@
'
+test_expect_success 'checkout does not treat remote @{upstream} as a branch' '
+ git update-ref refs/remotes/origin/checkout one &&
+ git branch --set-upstream-to=origin/checkout &&
+ git update-ref refs/heads/origin/checkout two &&
+ git update-ref refs/heads/remotes/origin/checkout two &&
+
+ git checkout @{upstream} &&
+ expect_branch HEAD one
+'
+
test_done
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* [PATCH 3/8] strbuf_branchname: add docstring
From: Jeff King @ 2017-02-28 12:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
This function and its companion, strbuf_check_branch_ref(),
did not have their purpose or semantics explained. Let's do
so.
Signed-off-by: Jeff King <peff@peff.net>
---
strbuf.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/strbuf.h b/strbuf.h
index 47df0500d..6b51b2604 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -560,7 +560,22 @@ static inline void strbuf_complete_line(struct strbuf *sb)
strbuf_complete(sb, '\n');
}
+/*
+ * Copy "name" to "sb", expanding any special @-marks as handled by
+ * interpret_branch_name(). The result is a non-qualified branch name
+ * (so "foo" or "origin/master" instead of "refs/heads/foo" or
+ * "refs/remotes/origin/master").
+ *
+ * Note that the resulting name may not be a syntactically valid refname.
+ */
extern void strbuf_branchname(struct strbuf *sb, const char *name);
+
+/*
+ * Like strbuf_branchname() above, but confirm that the result is
+ * syntactically valid to be used as a local branch name in refs/heads/.
+ *
+ * The return value is "0" if the result is valid, and "-1" otherwise.
+ */
extern int strbuf_check_branch_ref(struct strbuf *sb, const char *name);
extern void strbuf_addstr_urlencode(struct strbuf *, const char *,
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* [PATCH 4/8] interpret_branch_name: allow callers to restrict expansions
From: Jeff King @ 2017-02-28 12:14 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
The original purpose of interpret_branch_name() was to be
used by get_sha1() in resolving refs. As such, some of its
expansions may point to refs outside of the local
"refs/heads" namespace.
Over time, the function has been picked up by other callers
who want to use the ref-expansion to give the user access to
the same shortcuts (e.g., allowing "git branch" to delete
via "@{-1}" or "@{upstream}"). These uses have confusing
corner cases when the expansion isn't in refs/heads/ (for
instance, deleting "@" tries to delete refs/heads/HEAD,
which is nonsense).
Callers can't know from the returned string how the
expansion happened (e.g., did the user really ask for a
branch named "HEAD", or did we do a bogus expansion?). One
fix would be to return some out-parameters describing the
types of expansion that occurred. This has the benefit that
the caller can generate precise error messages ("I
understood @{upstream} to mean origin/master, but that is a
remote tracking branch, so you cannot create it as a local
name").
However, out-parameters make calling interface somewhat
cumbersome. Instead, let's do the opposite: let the caller
tell us which elements to expand. That's easier to pass in,
and none of the callers give more precise error messages
than "@{upstream} isn't a valid branch name" anyway (which
should be sufficient).
The strbuf_branchname() function needs a similar parameter,
as most of the callers access interpret_branch_name()
through it. For now, we'll pass "0" for "no restrictions" in
each caller, and update them individually in subsequent
patches.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin/branch.c | 2 +-
builtin/checkout.c | 2 +-
builtin/merge.c | 2 +-
cache.h | 13 +++++++++++--
refs.c | 2 +-
revision.c | 2 +-
sha1_name.c | 52 +++++++++++++++++++++++++++++++++++++++-------------
strbuf.h | 6 +++++-
8 files changed, 60 insertions(+), 21 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 94f7de7fa..cf0ece55d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -216,7 +216,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
char *target = NULL;
int flags = 0;
- strbuf_branchname(&bname, argv[i]);
+ strbuf_branchname(&bname, argv[i], 0);
free(name);
name = mkpathdup(fmt, bname.buf);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f174f5030..05eefd994 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -452,7 +452,7 @@ static void setup_branch_path(struct branch_info *branch)
{
struct strbuf buf = STRBUF_INIT;
- strbuf_branchname(&buf, branch->name);
+ strbuf_branchname(&buf, branch->name, 0);
if (strcmp(buf.buf, branch->name))
branch->name = xstrdup(buf.buf);
strbuf_splice(&buf, 0, 0, "refs/heads/", 11);
diff --git a/builtin/merge.c b/builtin/merge.c
index a96d4fb50..848a29855 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -438,7 +438,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
char *found_ref;
int len, early;
- strbuf_branchname(&bname, remote);
+ strbuf_branchname(&bname, remote, 0);
remote = bname.buf;
memset(branch_head, 0, sizeof(branch_head));
diff --git a/cache.h b/cache.h
index c67995caa..a8816c914 100644
--- a/cache.h
+++ b/cache.h
@@ -1383,8 +1383,17 @@ extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as s
*
* If the input was ok but there are not N branch switches in the
* reflog, it returns 0.
- */
-extern int interpret_branch_name(const char *str, int len, struct strbuf *);
+ *
+ * If "allowed" is non-zero, it is a treated as a bitfield of allowable
+ * expansions: local branches ("refs/heads/"), remote branches
+ * ("refs/remotes/"), or "HEAD". If no "allowed" bits are set, any expansion is
+ * allowed, even ones to refs outside of those namespaces.
+ */
+#define INTERPRET_BRANCH_LOCAL (1<<0)
+#define INTERPRET_BRANCH_REMOTE (1<<1)
+#define INTERPRET_BRANCH_HEAD (1<<2)
+extern int interpret_branch_name(const char *str, int len, struct strbuf *,
+ unsigned allowed);
extern int get_oid_mb(const char *str, struct object_id *oid);
extern int validate_headref(const char *ref);
diff --git a/refs.c b/refs.c
index 6d0961921..da62119c2 100644
--- a/refs.c
+++ b/refs.c
@@ -405,7 +405,7 @@ int refname_match(const char *abbrev_name, const char *full_name)
static char *substitute_branch_name(const char **string, int *len)
{
struct strbuf buf = STRBUF_INIT;
- int ret = interpret_branch_name(*string, *len, &buf);
+ int ret = interpret_branch_name(*string, *len, &buf, 0);
if (ret == *len) {
size_t size;
diff --git a/revision.c b/revision.c
index b37dbec37..771d079f6 100644
--- a/revision.c
+++ b/revision.c
@@ -147,7 +147,7 @@ static void add_pending_object_with_path(struct rev_info *revs,
revs->no_walk = 0;
if (revs->reflog_info && obj->type == OBJ_COMMIT) {
struct strbuf buf = STRBUF_INIT;
- int len = interpret_branch_name(name, 0, &buf);
+ int len = interpret_branch_name(name, 0, &buf, 0);
int st;
if (0 < len && name[len] && buf.len)
diff --git a/sha1_name.c b/sha1_name.c
index 4c1e91184..b21997c29 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1176,7 +1176,8 @@ static int interpret_empty_at(const char *name, int namelen, int len, struct str
return 1;
}
-static int reinterpret(const char *name, int namelen, int len, struct strbuf *buf)
+static int reinterpret(const char *name, int namelen, int len,
+ struct strbuf *buf, unsigned allowed)
{
/* we have extra data, which might need further processing */
struct strbuf tmp = STRBUF_INIT;
@@ -1184,7 +1185,7 @@ static int reinterpret(const char *name, int namelen, int len, struct strbuf *bu
int ret;
strbuf_add(buf, name + len, namelen - len);
- ret = interpret_branch_name(buf->buf, buf->len, &tmp);
+ ret = interpret_branch_name(buf->buf, buf->len, &tmp, allowed);
/* that data was not interpreted, remove our cruft */
if (ret < 0) {
strbuf_setlen(buf, used);
@@ -1205,11 +1206,27 @@ static void set_shortened_ref(struct strbuf *buf, const char *ref)
free(s);
}
+static int branch_interpret_allowed(const char *refname, unsigned allowed)
+{
+ if (!allowed)
+ return 1;
+
+ if ((allowed & INTERPRET_BRANCH_LOCAL) &&
+ starts_with(refname, "refs/heads/"))
+ return 1;
+ if ((allowed & INTERPRET_BRANCH_REMOTE) &&
+ starts_with(refname, "refs/remotes/"))
+ return 1;
+
+ return 0;
+}
+
static int interpret_branch_mark(const char *name, int namelen,
int at, struct strbuf *buf,
int (*get_mark)(const char *, int),
const char *(*get_data)(struct branch *,
- struct strbuf *))
+ struct strbuf *),
+ unsigned allowed)
{
int len;
struct branch *branch;
@@ -1234,11 +1251,15 @@ static int interpret_branch_mark(const char *name, int namelen,
if (!value)
die("%s", err.buf);
+ if (!branch_interpret_allowed(value, allowed))
+ return -1;
+
set_shortened_ref(buf, value);
return len + at;
}
-int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
+int interpret_branch_name(const char *name, int namelen, struct strbuf *buf,
+ unsigned allowed)
{
char *at;
const char *start;
@@ -1254,24 +1275,29 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
if (len == namelen)
return len; /* consumed all */
else
- return reinterpret(name, namelen, len, buf);
+ return reinterpret(name, namelen, len, buf, allowed);
}
for (start = name;
(at = memchr(start, '@', namelen - (start - name)));
start = at + 1) {
- len = interpret_empty_at(name, namelen, at - name, buf);
- if (len > 0)
- return reinterpret(name, namelen, len, buf);
+ if (!allowed || (allowed & INTERPRET_BRANCH_HEAD)) {
+ len = interpret_empty_at(name, namelen, at - name, buf);
+ if (len > 0)
+ return reinterpret(name, namelen, len, buf,
+ allowed);
+ }
len = interpret_branch_mark(name, namelen, at - name, buf,
- upstream_mark, branch_get_upstream);
+ upstream_mark, branch_get_upstream,
+ allowed);
if (len > 0)
return len;
len = interpret_branch_mark(name, namelen, at - name, buf,
- push_mark, branch_get_push);
+ push_mark, branch_get_push,
+ allowed);
if (len > 0)
return len;
}
@@ -1279,10 +1305,10 @@ int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
return -1;
}
-void strbuf_branchname(struct strbuf *sb, const char *name)
+void strbuf_branchname(struct strbuf *sb, const char *name, unsigned allowed)
{
int len = strlen(name);
- int used = interpret_branch_name(name, len, sb);
+ int used = interpret_branch_name(name, len, sb, allowed);
if (used < 0)
used = 0;
@@ -1291,7 +1317,7 @@ void strbuf_branchname(struct strbuf *sb, const char *name)
int strbuf_check_branch_ref(struct strbuf *sb, const char *name)
{
- strbuf_branchname(sb, name);
+ strbuf_branchname(sb, name, 0);
if (name[0] == '-')
return -1;
strbuf_splice(sb, 0, 0, "refs/heads/", 11);
diff --git a/strbuf.h b/strbuf.h
index 6b51b2604..17e5f29a5 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -567,8 +567,12 @@ static inline void strbuf_complete_line(struct strbuf *sb)
* "refs/remotes/origin/master").
*
* Note that the resulting name may not be a syntactically valid refname.
+ *
+ * If "allowed" is non-zero, restrict the set of allowed expansions. See
+ * interpret_branch_name() for details.
*/
-extern void strbuf_branchname(struct strbuf *sb, const char *name);
+extern void strbuf_branchname(struct strbuf *sb, const char *name,
+ unsigned allowed);
/*
* Like strbuf_branchname() above, but confirm that the result is
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* [PATCH 1/8] interpret_branch_name: move docstring to header file
From: Jeff King @ 2017-02-28 12:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jacob Keller, Karthik Nayak, Luc Van Oostenryck, Git List
In-Reply-To: <20170228120633.zkwfqms57fk7dkl5@sigill.intra.peff.net>
We generally put docstrings with function declarations,
because it's the callers who need to know how the function
works. Let's do so for interpret_branch_name().
Signed-off-by: Jeff King <peff@peff.net>
---
cache.h | 21 +++++++++++++++++++++
sha1_name.c | 21 ---------------------
2 files changed, 21 insertions(+), 21 deletions(-)
diff --git a/cache.h b/cache.h
index 80b6372cf..c67995caa 100644
--- a/cache.h
+++ b/cache.h
@@ -1363,6 +1363,27 @@ extern char *oid_to_hex_r(char *out, const struct object_id *oid);
extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
extern char *oid_to_hex(const struct object_id *oid); /* same static buffer as sha1_to_hex */
+/*
+ * This reads short-hand syntax that not only evaluates to a commit
+ * object name, but also can act as if the end user spelled the name
+ * of the branch from the command line.
+ *
+ * - "@{-N}" finds the name of the Nth previous branch we were on, and
+ * places the name of the branch in the given buf and returns the
+ * number of characters parsed if successful.
+ *
+ * - "<branch>@{upstream}" finds the name of the other ref that
+ * <branch> is configured to merge with (missing <branch> defaults
+ * to the current branch), and places the name of the branch in the
+ * given buf and returns the number of characters parsed if
+ * successful.
+ *
+ * If the input is not of the accepted format, it returns a negative
+ * number to signal an error.
+ *
+ * If the input was ok but there are not N branch switches in the
+ * reflog, it returns 0.
+ */
extern int interpret_branch_name(const char *str, int len, struct strbuf *);
extern int get_oid_mb(const char *str, struct object_id *oid);
diff --git a/sha1_name.c b/sha1_name.c
index 9b5d14b4b..28865b3a1 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1238,27 +1238,6 @@ static int interpret_branch_mark(const char *name, int namelen,
return len + at;
}
-/*
- * This reads short-hand syntax that not only evaluates to a commit
- * object name, but also can act as if the end user spelled the name
- * of the branch from the command line.
- *
- * - "@{-N}" finds the name of the Nth previous branch we were on, and
- * places the name of the branch in the given buf and returns the
- * number of characters parsed if successful.
- *
- * - "<branch>@{upstream}" finds the name of the other ref that
- * <branch> is configured to merge with (missing <branch> defaults
- * to the current branch), and places the name of the branch in the
- * given buf and returns the number of characters parsed if
- * successful.
- *
- * If the input is not of the accepted format, it returns a negative
- * number to signal an error.
- *
- * If the input was ok but there are not N branch switches in the
- * reflog, it returns 0.
- */
int interpret_branch_name(const char *name, int namelen, struct strbuf *buf)
{
char *at;
--
2.12.0.359.gd4c8c42e9
^ permalink raw reply related
* Re: [PATCH 0/6] Use time_t
From: Johannes Schindelin @ 2017-02-28 11:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqlgsrmesc.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 27 Feb 2017, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
> > One notable fallout of this patch series is that on 64-bit Linux (and
> > other platforms where `unsigned long` is 64-bit), we now limit the
> > range of dates to LONG_MAX (i.e. the *signed* maximum value). This
> > needs to be done as `time_t` can be signed (and indeed is at least on
> > my Ubuntu setup).
> >
> > Obviously, I think that we can live with that, and I hope that all
> > interested parties agree.
>
> s/ulong/time_t/ is definintely a good change, and it will take us to a
> place we would want to be in in some future.
Actually. I have to take back the part where I hoped that all interested
parties would agree. The problem is 32-bit Linux:
$ cat >a1.c <<-\EOF
#include <stdio.h>
#include <limits.h>
#include <time.h>
int main(int argc, char **argv)
{
printf("sizeof(long): %d, sizeof(time_t): %d, ulong_max: %lu\n",
(int)sizeof(long), (int)sizeof(time_t), ULONG_MAX);
return 0;
}
EOF
$ gcc -m32 -Wall -o a1 a1.c
$ ./a1
sizeof(long): 4, sizeof(time_t): 4, ulong_max: 4294967295
So. Not only is `long` a 32-bit on 32-bit Linux, but so is `time_t`. And
with that, switching from `ULONG_MAX` as the maximal time we can represent
in Git to `LONG_MAX` is kind of a serious problem.
> As long as there remains no platform we care about whose time_t and long
> are still 32-bit signed integer, there will be a fallout to them with
> this change.
Sorry, I do not understand the verb "remains" in conjunction with "no
platform"...
Do you mean to say that currently no platform we care about has 32-bit
signed time_t/long?
If so, I just demonstrated this to be unfortunately incorrect.
> It appears that we use uint64_t in many places in our code. So
> while philosophically time_t is the right type, uint64_t might be
> practically a safer alternative type to use at the endgame patch in
> this series.
Yes, I think you are right. We should use uint64_t instead of time_t, but
*semantically* we should not even use uint64_t. We should introduce our
own data type instead of repeating the mistake to use a data type that
does not convey its role to the reader.
Currently, I am favoring timestamp_t.
> I haven't seen it yet, but presumably the last one 6/6 is the endgame?
Maybe it took a while to get sent out, but it made it into public inbox:
http://public-inbox.org/git/75efe76cbb0636741a7c3aec9e21459bc1dc3cbe.1488231002.git.johannes.schindelin@gmx.de/
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 2/6] Specify explicitly where we parse timestamps
From: Johannes Schindelin @ 2017-02-28 10:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqh93fmemg.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Mon, 27 Feb 2017, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> >> - unsigned long number = strtoul(date, &end, 10);
> >> + time_t number = parse_timestamp(date, &end, 10);
> >
> > This hunk does not belong to this step. Everybody else in this step
>
> obviously I meant "the left half of this hunk" ;-)
Obviously ;-)
Ciao,
Johannes
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox