* [PATCH] clean.c, ls-files.c: respect encapsulation of exclude_list_groups
From: Adam Spiers @ 2013-01-16 13:25 UTC (permalink / raw)
To: git list
In-Reply-To: <CAOkDyE-p9WLrsFZjPb9sY+YEypkF2wDxMybBkCT-76jBbKOmCA@mail.gmail.com>
Consumers of the dir.c traversal API should avoid assuming knowledge
of the internal implementation of exclude_list_groups. Therefore
when adding items to an exclude list, it should be accessed via the
pointer returned from add_exclude_list(), rather than by referencing
a location within dir.exclude_list_groups[EXC_CMDL].
Signed-off-by: Adam Spiers <git@adamspiers.org>
---
builtin/clean.c | 6 +++---
builtin/ls-files.c | 15 ++++++++++-----
2 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index b098288..b9cb7ad 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -45,6 +45,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
static const char **pathspec;
struct strbuf buf = STRBUF_INIT;
struct string_list exclude_list = STRING_LIST_INIT_NODUP;
+ struct exclude_list *el;
const char *qname;
char *seen = NULL;
struct option options[] = {
@@ -97,10 +98,9 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
if (!ignored)
setup_standard_excludes(&dir);
- add_exclude_list(&dir, EXC_CMDL, "--exclude option");
+ el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
for (i = 0; i < exclude_list.nr; i++)
- add_exclude(exclude_list.items[i].string, "", 0,
- &dir.exclude_list_group[EXC_CMDL].el[0], -(i+1));
+ add_exclude(exclude_list.items[i].string, "", 0, el, -(i+1));
pathspec = get_pathspec(prefix, argv);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index fa9ccb8..b4d8b01 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -421,10 +421,10 @@ static int option_parse_z(const struct option *opt,
static int option_parse_exclude(const struct option *opt,
const char *arg, int unset)
{
- struct exclude_list_group *group = opt->value;
+ struct string_list *exclude_list = opt->value;
exc_given = 1;
- add_exclude(arg, "", 0, &group->el[0], --exclude_args);
+ string_list_append(exclude_list, arg);
return 0;
}
@@ -453,9 +453,11 @@ static int option_parse_exclude_standard(const struct option *opt,
int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
{
- int require_work_tree = 0, show_tag = 0;
+ int require_work_tree = 0, show_tag = 0, i;
const char *max_prefix;
struct dir_struct dir;
+ struct exclude_list *el;
+ struct string_list exclude_list = STRING_LIST_INIT_NODUP;
struct option builtin_ls_files_options[] = {
{ OPTION_CALLBACK, 'z', NULL, NULL, NULL,
"paths are separated with NUL character",
@@ -490,7 +492,7 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
OPT_BOOLEAN(0, "resolve-undo", &show_resolve_undo,
"show resolve-undo information"),
{ OPTION_CALLBACK, 'x', "exclude",
- &dir.exclude_list_group[EXC_CMDL], "pattern",
+ &exclude_list, "pattern",
"skip files matching pattern",
0, option_parse_exclude },
{ OPTION_CALLBACK, 'X', "exclude-from", &dir, "file",
@@ -525,9 +527,12 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
if (read_cache() < 0)
die("index file corrupt");
- add_exclude_list(&dir, EXC_CMDL, "--exclude option");
argc = parse_options(argc, argv, prefix, builtin_ls_files_options,
ls_files_usage, 0);
+ el = add_exclude_list(&dir, EXC_CMDL, "--exclude option");
+ for (i = 0; i < exclude_list.nr; i++) {
+ add_exclude(exclude_list.items[i].string, "", 0, el, --exclude_args);
+ }
if (show_tag || show_valid_bit) {
tag_cached = "H ";
tag_unmerged = "M ";
--
1.8.1.291.g0730ed6
^ permalink raw reply related
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Max Horn @ 2013-01-16 13:32 UTC (permalink / raw)
To: Chris Rorvick
Cc: git, Angelo Borsotti, Drew Northup, Michael Haggerty,
Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
Felipe Contreras, Junio C Hamano
In-Reply-To: <1354239700-3325-1-git-send-email-chris@rorvick.com>
Hi there,
I was just working on improving git-remote-helper.txt by documenting how remote helper can signal error conditions to git. This lead me to notice a (to me) surprising change in behavior between master and next that I traced back to this patch series.
Specifically:
On 30.11.2012, at 02:41, Chris Rorvick wrote:
> This patch series originated in response to the following thread:
>
> http://thread.gmane.org/gmane.comp.version-control.git/208354
>
> I made some adjustments based on Junio's last round of feedback
> including a new patch reworking the "push rules" comment in remote.c.
> Also refined some of the log messages--nothing major. Finally, took a
> stab at putting something together for the release notes, see below.
>From the discussion in that gmane thread and from the commits in this series, I had the impression that it should mostly affect pushing tags. However, this is not the case: It also changes messages upon regular push "conflicts. Consider this test script:
#!/bin/sh -ex
git init repo_orig
cd repo_orig
echo a > a
git add a
git commit -m a
cd ..
git clone repo_orig repo_clone
cd repo_orig
echo b > b
git add b
git commit -m b
cd ..
cd repo_clone
echo B > b
git add b
git commit -m B
git push
With git 1.8.1, I get this message:
! [rejected] master -> master (non-fast-forward)
error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
hint: Updates were rejected because the tip of your current branch is behind
hint: its remote counterpart. Merge the remote changes (e.g. 'git pull')
hint: before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.
But with next, I get this:
! [rejected] master -> master (already exists)
error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
hint: Updates were rejected because the destination reference already exists
hint: in the remote.
This looks like a regression to me. No tags were involve, and the new message is very confusing if not outright wrong -- at least in my mind, but perhaps I am missing a way to interpret it "correctly" ? What am I missing?
Cheers,
Max
^ permalink raw reply
* Unable to convert a subversion repo to git
From: Timothy Kretschmer @ 2013-01-16 14:06 UTC (permalink / raw)
To: git
I am seeing the following output while converting a subversion repo to git.
>Found possible branch point: <repo-url>/trunk =>
<repo-url>/branches/CMT_PHASE3, 18441
> fatal: Not a valid object name refs/remotes/BlueSimViewer 5.0 20110316 Branch
> cat-file commit refs/remotes/BlueSimViewer 5.0 20110316 Branch: command returned error: 128
The command I am running to convert the repo is
> git svn clone <repo-url> -A authors-transform.txt --stdlayout bluebox-git > svnlist
I am running git version 1.8.1.1 on an Ubuntu 12.10 server. I am happy
to provide any other information that would be helpful.
I appreciate any assistance you can provide in this matter,
-Tim
^ permalink raw reply
* [PATCH] fix some clang warnings
From: Max Horn @ 2013-01-16 14:53 UTC (permalink / raw)
To: git; +Cc: Johannes Sixt, Jeff King, Max Horn
Signed-off-by: Max Horn <max@quendi.de>
---
cache.h | 2 +-
git-compat-util.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index c257953..5c8440b 100644
--- a/cache.h
+++ b/cache.h
@@ -1148,7 +1148,7 @@ extern int check_repository_format_version(const char *var, const char *value, v
extern int git_env_bool(const char *, int);
extern int git_config_system(void);
extern int config_error_nonbool(const char *);
-#ifdef __GNUC__
+#if defined(__GNUC__) && ! defined(__clang__)
#define config_error_nonbool(s) (config_error_nonbool(s), -1)
#endif
extern const char *get_log_output_encoding(void);
diff --git a/git-compat-util.h b/git-compat-util.h
index 4f022a3..cc2abee 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -310,7 +310,7 @@ extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)))
* behavior. But since we're only trying to help gcc, anyway, it's OK; other
* compilers will fall back to using the function as usual.
*/
-#ifdef __GNUC__
+#if defined(__GNUC__) && ! defined(__clang__)
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
#endif
--
1.8.1.1.435.g4e2ebdf
^ permalink raw reply related
* Re: [PATCH 0/7] guilt patches, including git 1.8 support
From: Josef 'Jeff' Sipek @ 2013-01-16 15:04 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Jonathan Nieder, git, Per Cederqvist, Iulian Udrea, Axel Beckert
In-Reply-To: <20130116032606.GA6052@thunk.org>
On Tue, Jan 15, 2013 at 10:26:06PM -0500, Theodore Ts'o wrote:
> On Tue, Jan 15, 2013 at 06:26:06PM -0800, Jonathan Nieder wrote:
> > Hi Jeff and other guilty parties,
> >
> > I collected all the guilt patches I could find on-list and added one
> > of my own. Completely untested, except for running the regression
> > tests. These are also available via git protocol from
> >
> > git://repo.or.cz/guilt/mob.git mob
>
> Jonathan, thanks for collecting all of the guilt patches! Your repro
> was also very much really useful since I hadn't grabbed the latest
> patches from jeffpc's repo before it disappeared after the kernel.org
> security shutdown.
I had repo.or.cz mirroring all along. :)
> Jeff, do you need some help getting your repro on kernel.org
> re-established?
Yes and no. I was hoping to find some time to restore all the web content
on my server, and start using repo.or.cz as the public git repo. With that
said, I have only two sigs for my gpg key. (Guilt isn't really related to
linux...)
Thanks,
Jeff.
--
Only two things are infinite, the universe and human stupidity, and I'm not
sure about the former.
- Albert Einstein
^ permalink raw reply
* Re: [PATCH v2 07/14] imap-send.c: inline imap_parse_list() in imap_list()
From: Junio C Hamano @ 2013-01-16 15:34 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Matt Kraai, git
In-Reply-To: <50F66422.3010502@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 01/15/2013 07:51 PM, Matt Kraai wrote:
>> On Tue, Jan 15, 2013 at 09:06:25AM +0100, Michael Haggerty wrote:
>>> -static struct imap_list *parse_imap_list(struct imap *imap, char **sp)
>>> +static struct imap_list *parse_list(char **sp)
>>
>> The commit subject refers to imap_parse_list and imap_list whereas the
>> code refers to parse_imap_list and parse_list.
>
> Yes, you're right. Thanks.
I think I've fixed this (and some other minor points in other
patches in the series) while queuing; please check master..3691031c
after fetching from me.
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] Move Git::SVN::get_tz to Git::get_tz_offset
From: Junio C Hamano @ 2013-01-16 15:37 UTC (permalink / raw)
To: Ben Walton; +Cc: esr, git
In-Reply-To: <1358291405-10173-2-git-send-email-bdwalton@gmail.com>
Ben Walton <bdwalton@gmail.com> writes:
> +sub get_tz_offset {
> + # some systmes don't handle or mishandle %z, so be creative.
Hmph. I wonder if we can use %z if it is handled correctly and fall
back to this code only on platforms that are broken?
^ permalink raw reply
* Re: [PATCH 2/7] contrib/subtree: Use %B for Split Subject/Body
From: Junio C Hamano @ 2013-01-16 15:43 UTC (permalink / raw)
To: greened; +Cc: 郑文辉(Techlive Zheng), git
In-Reply-To: <87zk09q0i4.fsf@waller.obbligato.org>
greened@obbligato.org writes:
> Are you incorporating the other patches? Should I drop them
> from my list?
I actually was planning to accept patches to this subdirectory only
through you, hopefully as messages that forward others' changes with
your Acked-by: tagline. That frees me from having to keeping track
of what goes on there ;-)
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Junio C Hamano @ 2013-01-16 15:50 UTC (permalink / raw)
To: Michael J Gruber; +Cc: Jardel Weyrich, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <50F668FB.5000805@drmicha.warpmail.net>
Michael J Gruber <git@drmicha.warpmail.net> writes:
> Junio C Hamano venit, vidit, dixit 15.01.2013 16:53:
> ...
>> * When there are more than one URLs, and there is no pushURL, then
>> show the first URL as (fetch/push), and the remainder in a
>> notation that says it is used only for push, but it shouldn't be
>> the same "(push)"; the user has to be able to distinguish it from
>> the pushURLs in a repository that also has URLs.
>
> Maybe "(fetch fallback/push)" if we do use it as a fallback? If we don't
> we probably should?
I actually think my earlier "it shouldn't be the same (push)" is not
needed and probably is actively wrong. Just like you can tell
between
(only one .url) (both .url and .pushurl)
origin there (fetch/push) origin there (fetch)
origin there (push)
even when the value of the URL/PushURL, i.e. "there", is the same
between .url and .pushurl, you should be able to tell between
(two .url, no .pushurl) (one .url and one .pushurl)
origin there (fetch/push) origin there (fetch)
origin another (push) origin another (push)
So let's not make it too complex and forget about the different kind
of "(push)".
A case that is a potential misconfiguration would look like:
(two .url, one .pushurl)
origin there (fetch)
origin some (unused)
origin another (push)
I think.
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Phil Hord @ 2013-01-16 16:15 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael J Gruber, Jardel Weyrich, Sascha Cunz,
git@vger.kernel.org
In-Reply-To: <7v4nii5tp2.fsf@alter.siamese.dyndns.org>
On Tue, Jan 15, 2013 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> That being said, I don't mind changing the behaviour of set-url.
>
> I do not think we want to change the behaviour of set-url.
I agree with Michael that changing the set-url behavior would be
appropriate here. If I say "--add" this pushUrl, don't I mean to
create an additional url which is pushed to?
I agree that it makes the config situation messy; this is currently a
"clean" sequence, in that it leaves the config unchanged after both
steps are completed:
git remote set-url --add --push origin /tmp/foo
git remote set-url --delete --push origin /tmp/foo
If the behavior is changed like Michael suggested, it would not leave
the config clean (unless heroic steps were taken to keep track). But
I'm not sure that's such a bad thing. In simple command sequences,
the results would be clean and the only behavior change is that the
initial "--add" really acts like "add" and not "replace". But more
complex sequences could be devised which were affected by this change.
I'm curious, Junio. Do you think the set-url behavior is correct
as-is, or that changing it will cause breakage for some workflows, or
that it complicates the operation too much for people who are already
used to the config layout?
Phil
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Michael J Gruber @ 2013-01-16 16:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jardel Weyrich, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <7v622xyvnd.fsf@alter.siamese.dyndns.org>
Junio C Hamano venit, vidit, dixit 16.01.2013 16:50:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
>
>> Junio C Hamano venit, vidit, dixit 15.01.2013 16:53:
>> ...
>>> * When there are more than one URLs, and there is no pushURL, then
>>> show the first URL as (fetch/push), and the remainder in a
>>> notation that says it is used only for push, but it shouldn't be
>>> the same "(push)"; the user has to be able to distinguish it from
>>> the pushURLs in a repository that also has URLs.
>>
>> Maybe "(fetch fallback/push)" if we do use it as a fallback? If we don't
>> we probably should?
>
> I actually think my earlier "it shouldn't be the same (push)" is not
> needed and probably is actively wrong. Just like you can tell
> between
>
> (only one .url) (both .url and .pushurl)
>
> origin there (fetch/push) origin there (fetch)
> origin there (push)
>
> even when the value of the URL/PushURL, i.e. "there", is the same
> between .url and .pushurl, you should be able to tell between
>
> (two .url, no .pushurl) (one .url and one .pushurl)
>
> origin there (fetch/push) origin there (fetch)
> origin another (push) origin another (push)
>
> So let's not make it too complex and forget about the different kind
> of "(push)".
>
> A case that is a potential misconfiguration would look like:
>
> (two .url, one .pushurl)
>
> origin there (fetch)
> origin some (unused)
> origin another (push)
>
> I think.
I'm sorry but E_NOPARSE. I can't grok the above at all. But I'll try
again tomorrow ;)
In any case, the issue with (push)instead of that John mentions bothers
me: there are "two specified URLs" but one URL in config only; my patch
doesn't make that case clearer at all. My early attempts at amending
struct remote produced too many segfaults to continue today...
Michael
^ permalink raw reply
* Re: [BUG] Possible bug in `remote set-url --add --push`
From: Michael J Gruber @ 2013-01-16 16:24 UTC (permalink / raw)
To: Phil Hord
Cc: Junio C Hamano, Jardel Weyrich, Sascha Cunz, git@vger.kernel.org
In-Reply-To: <CABURp0rR_wB6vcjrZajQU_=AVVvBq-aTGpggh5XxdCMYis3-ag@mail.gmail.com>
Phil Hord venit, vidit, dixit 16.01.2013 17:15:
> On Tue, Jan 15, 2013 at 10:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Michael J Gruber <git@drmicha.warpmail.net> writes:
>>
>>> That being said, I don't mind changing the behaviour of set-url.
>>
>> I do not think we want to change the behaviour of set-url.
>
> I agree with Michael that changing the set-url behavior would be
> appropriate here. If I say "--add" this pushUrl, don't I mean to
> create an additional url which is pushed to?
I said I wouldn't mind, I didn't vote for it.
> I agree that it makes the config situation messy; this is currently a
> "clean" sequence, in that it leaves the config unchanged after both
> steps are completed:
>
> git remote set-url --add --push origin /tmp/foo
> git remote set-url --delete --push origin /tmp/foo
>
> If the behavior is changed like Michael suggested, it would not leave
> the config clean (unless heroic steps were taken to keep track). But
> I'm not sure that's such a bad thing. In simple command sequences,
> the results would be clean and the only behavior change is that the
> initial "--add" really acts like "add" and not "replace". But more
> complex sequences could be devised which were affected by this change.
>
> I'm curious, Junio. Do you think the set-url behavior is correct
> as-is, or that changing it will cause breakage for some workflows, or
> that it complicates the operation too much for people who are already
> used to the config layout?
For "set url --add --push" on top of a push url only being defaulted
from a fetch url, both behaviours (replace or add, i.e. current or new)
make sense to me. So the questions are:
- Is it worth and possible changing?
- How to best describe it in "remote -v" and "remote show" output?
My patch answered to "no" to the first question and answers the second
one in cases where (push)insteadof is not used to transform one fetch
config into two different urls for fetch and push. I think :)
Michael
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 16:36 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Jeff King, Felipe Contreras
In-Reply-To: <DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de>
Max Horn <max@quendi.de> writes:
> But with next, I get this:
>
>
> ! [rejected] master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.
It is an outright bug. The new helper function is_forwrdable() is
bogus to assume that both original and updated objects can be
locally inspected, but you do not necessarily have the original
locally.
remote.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/remote.c b/remote.c
index aa6b719..4a253ef 100644
--- a/remote.c
+++ b/remote.c
@@ -1286,9 +1286,12 @@ static inline int is_forwardable(struct ref* ref)
if (!prefixcmp(ref->name, "refs/tags/"))
return 0;
- /* old object must be a commit */
+ /*
+ * old object must be a commit, but we may be forcing
+ * without having it in the first place!
+ */
o = parse_object(ref->old_sha1);
- if (!o || o->type != OBJ_COMMIT)
+ if (o && o->type != OBJ_COMMIT)
return 0;
/* new object must be commit-ish */
^ permalink raw reply related
* Re: [PATCH] fix some clang warnings
From: Jeff King @ 2013-01-16 16:04 UTC (permalink / raw)
To: Max Horn; +Cc: git, Johannes Sixt
In-Reply-To: <1358348003-11130-1-git-send-email-max@quendi.de>
On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
> -#ifdef __GNUC__
> +#if defined(__GNUC__) && ! defined(__clang__)
> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
> #endif
You don't say what the warning is, but I'm guessing it's complaining
about throwing away the return value from config_error_nonbool?
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 16:48 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Jeff King, Felipe Contreras
In-Reply-To: <7vsj61xez2.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Max Horn <max@quendi.de> writes:
>
>> But with next, I get this:
>>
>> ! [rejected] master -> master (already exists)
>> error: failed to push some refs to '/Users/mhorn/Proje...o_orig'
>> hint: Updates were rejected because the destination reference already exists
>> hint: in the remote.
>>
>> This looks like a regression to me.
>
> It is an outright bug. The new helper function is_forwrdable() is
> bogus to assume that both original and updated objects can be
> locally inspected, but you do not necessarily have the original
> locally.
The way the caller uses the result of this function is equally
questionable. If this function says "we do not want to let this
push go through", it translates that unconditionally into "we
blocked it because the destination already exists".
It is fine when pushing into "refs/tags/" hierarchy. It is *NOT*
OK if the type check does not satisfy this function. In that case,
we do not actually see the existence of the destination as a
problem, but it is reported as such. We are blocking because we do
not like the type of the new object or the type of the old object.
If the destination points at a commit, the push can succeed if the
user changes what object to push, so saying "you cannot push because
the destination already exists" is just wrong in such a case.
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Junio C Hamano @ 2013-01-16 16:53 UTC (permalink / raw)
To: Jeff King; +Cc: Max Horn, git, Johannes Sixt
In-Reply-To: <20130116160410.GC22400@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
>
>> -#ifdef __GNUC__
>> +#if defined(__GNUC__) && ! defined(__clang__)
>> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
>> #endif
>
> You don't say what the warning is, but I'm guessing it's complaining
> about throwing away the return value from config_error_nonbool?
Yeah, I was wondering about the same thing. The other one looks
similar, ignoring the return value of error().
Also, is this "some versions of clang do not like this"? Or are all
versions of clang affected?
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Jeff King @ 2013-01-16 16:01 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras, Junio C Hamano
In-Reply-To: <DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de>
On Wed, Jan 16, 2013 at 02:32:03PM +0100, Max Horn wrote:
> With git 1.8.1, I get this message:
>
> ! [rejected] master -> master (non-fast-forward)
> [...]
> But with next, I get this:
>
> ! [rejected] master -> master (already exists)
Thanks for the detailed report. I was able to reproduce easily here.
The problem is the logic in is_forwardable:
static inline int is_forwardable(struct ref* ref)
{
struct object *o;
if (!prefixcmp(ref->name, "refs/tags/"))
return 0;
/* old object must be a commit */
o = parse_object(ref->old_sha1);
if (!o || o->type != OBJ_COMMIT)
return 0;
/* new object must be commit-ish */
o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
if (!o || o->type != OBJ_COMMIT)
return 0;
return 1;
}
The intent is to allow fast-forward only between objects that both point
to commits eventually. But we are doing this check on the client, which
does not necessarily have the object for ref->old_sha1 at all. So it
cannot know the type, and cannot enforce this condition accurately.
I.e., we trigger the "!o" branch after the parse_object in your example.
-Peff
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 16:00 UTC (permalink / raw)
To: Max Horn
Cc: Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Jeff King, Felipe Contreras
In-Reply-To: <DBF53EC2-A669-4B77-B88E-BFCDF43C862E@quendi.de>
Max Horn <max@quendi.de> writes:
> But with next, I get this:
>
> ! [rejected] master -> master (already exists)
> error: failed to push some refs to '/Users/mhorn/Projekte/foreign/gitifyhg/bugs/git-push-conflict/repo_orig'
> hint: Updates were rejected because the destination reference already exists
> hint: in the remote.
>
> This looks like a regression to me.
It is in master now X-<, and this looks like a bug to me.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Junio C Hamano @ 2013-01-16 17:10 UTC (permalink / raw)
To: Jeff King
Cc: Max Horn, Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <20130116160131.GB22400@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I.e., we trigger the "!o" branch after the parse_object in your example.
Heh, I didn't see this message until now (gmane seems to be lagging
a bit).
I am very tempted to do this.
* Remove unnecessary not_forwardable from "struct ref"; it is only
used inside set_ref_status_for_push();
* "refs/tags/" is the only hierarchy that cannot be replaced
without --force;
* Remove the misguided attempt to force that everything that
updates an existing ref has to be a commit outside "refs/tags/"
hierarchy. This code does not know what kind of objects the user
wants to place in "refs/frotz/" hierarchy it knows nothing about.
I feel moderately strongly about the last point. Defining special
semantics for one hierarchy (e.g. "refs/tags/") and implementing a
policy for enforcement is one thing, but a random policy that
depends on object type that applies globally is simply insane. The
user may want to do "refs/tested/" hierarchy that is meant to hold
references to commit, with one annotated tag "refs/tested/latest"
that points at the "latest tested version" with some commentary, and
maintain the latter by keep pushing to it. If that is the semantics
the user wanted to ahve in the "refs/tested/" hierarchy, it is not
reasonable to require --force for such a workflow. The user knows
better than Git in such a case.
cache.h | 1 -
remote.c | 24 +-----------------------
t/t5516-fetch-push.sh | 21 ---------------------
3 files changed, 1 insertion(+), 45 deletions(-)
diff --git a/cache.h b/cache.h
index a32a0ea..a942bbd 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,7 +1004,6 @@ struct ref {
requires_force:1,
merge:1,
nonfastforward:1,
- not_forwardable:1,
update:1,
deletion:1;
enum {
diff --git a/remote.c b/remote.c
index aa6b719..2c747c4 100644
--- a/remote.c
+++ b/remote.c
@@ -1279,26 +1279,6 @@ int match_push_refs(struct ref *src, struct ref **dst,
return 0;
}
-static inline int is_forwardable(struct ref* ref)
-{
- struct object *o;
-
- if (!prefixcmp(ref->name, "refs/tags/"))
- return 0;
-
- /* old object must be a commit */
- o = parse_object(ref->old_sha1);
- if (!o || o->type != OBJ_COMMIT)
- return 0;
-
- /* new object must be commit-ish */
- o = deref_tag(parse_object(ref->new_sha1), NULL, 0);
- if (!o || o->type != OBJ_COMMIT)
- return 0;
-
- return 1;
-}
-
void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
int force_update)
{
@@ -1344,8 +1324,6 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
* passing the --force argument
*/
- ref->not_forwardable = !is_forwardable(ref);
-
ref->update =
!ref->deletion &&
!is_null_sha1(ref->old_sha1);
@@ -1355,7 +1333,7 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
!has_sha1_file(ref->old_sha1)
|| !ref_newer(ref->new_sha1, ref->old_sha1);
- if (ref->not_forwardable) {
+ if (!prefixcmp(ref->name, "refs/tags/")) {
ref->requires_force = 1;
if (!force_ref_update) {
ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 6009372..8f024a0 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -950,27 +950,6 @@ test_expect_success 'push requires --force to update lightweight tag' '
)
'
-test_expect_success 'push requires --force to update annotated tag' '
- mk_test heads/master &&
- mk_child child1 &&
- mk_child child2 &&
- (
- cd child1 &&
- git tag -a -m "message 1" Tag &&
- git push ../child2 Tag:refs/tmp/Tag &&
- git push ../child2 Tag:refs/tmp/Tag &&
- >file1 &&
- git add file1 &&
- git commit -m "file1" &&
- git tag -f -a -m "message 2" Tag &&
- test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
- git push --force ../child2 Tag:refs/tmp/Tag &&
- git tag -f -a -m "message 3" Tag HEAD~ &&
- test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
- git push --force ../child2 Tag:refs/tmp/Tag
- )
-'
-
test_expect_success 'push --porcelain' '
mk_empty &&
echo >.git/foo "To testrepo" &&
^ permalink raw reply related
* Re: [PATCH] fix some clang warnings
From: Antoine Pelisse @ 2013-01-16 17:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Max Horn, git, Johannes Sixt
In-Reply-To: <7vk3rdxe5y.fsf@alter.siamese.dyndns.org>
FWIW, I also happen to have the warning:
advice.c:69:2: warning: expression result unused [-Wunused-value]
error("'%s' is not possible because you have unmerged files.", me);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
./git-compat-util.h:314:55: note: expanded from:
#define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
^~
with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
(based on LLVM 3.0)
I can't say about other versions.
On Wed, Jan 16, 2013 at 5:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> On Wed, Jan 16, 2013 at 03:53:23PM +0100, Max Horn wrote:
>>
>>> -#ifdef __GNUC__
>>> +#if defined(__GNUC__) && ! defined(__clang__)
>>> #define config_error_nonbool(s) (config_error_nonbool(s), -1)
>>> #endif
>>
>> You don't say what the warning is, but I'm guessing it's complaining
>> about throwing away the return value from config_error_nonbool?
>
> Yeah, I was wondering about the same thing. The other one looks
> similar, ignoring the return value of error().
>
> Also, is this "some versions of clang do not like this"? Or are all
> versions of clang affected?
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: John Keeping @ 2013-01-16 17:18 UTC (permalink / raw)
To: Antoine Pelisse; +Cc: Junio C Hamano, Jeff King, Max Horn, git, Johannes Sixt
In-Reply-To: <CALWbr2z4TiynwOR3Lk4005dbZaLtcHK3J01ZF73wp8Q7Rm6YBA@mail.gmail.com>
On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
> FWIW, I also happen to have the warning:
>
> advice.c:69:2: warning: expression result unused [-Wunused-value]
> error("'%s' is not possible because you have unmerged files.", me);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ./git-compat-util.h:314:55: note: expanded from:
> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
> ^~
>
> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
> (based on LLVM 3.0)
I have the same output with:
clang version 3.2 (tags/RELEASE_32/final)
^ permalink raw reply
* Re: [PATCH] fix some clang warnings
From: Max Horn @ 2013-01-16 17:26 UTC (permalink / raw)
To: John Keeping
Cc: Antoine Pelisse, Junio C Hamano, Jeff King, git, Johannes Sixt
In-Reply-To: <20130116171809.GA2476@farnsworth.metanate.com>
On 16.01.2013, at 18:18, John Keeping wrote:
> On Wed, Jan 16, 2013 at 06:12:57PM +0100, Antoine Pelisse wrote:
>> FWIW, I also happen to have the warning:
>>
>> advice.c:69:2: warning: expression result unused [-Wunused-value]
>> error("'%s' is not possible because you have unmerged files.", me);
>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ./git-compat-util.h:314:55: note: expanded from:
>> #define error(fmt, ...) (error((fmt), ##__VA_ARGS__), -1)
>> ^~
>>
>> with clang: Ubuntu clang version 3.0-6ubuntu3 (tags/RELEASE_30/final)
>> (based on LLVM 3.0)
>
> I have the same output with:
>
> clang version 3.2 (tags/RELEASE_32/final)
Sorry for not being more specific in my message. I have this with
Apple clang version 4.1 (tags/Apple/clang-421.11.66) (based on LLVM 3.1svn)
Max
^ permalink raw reply
* Re: [PATCH] Add Auto-Submitted header to post-receive-email
From: Chris Hiestand @ 2013-01-16 17:29 UTC (permalink / raw)
To: Andy Parkins, Junio C Hamano; +Cc: git
In-Reply-To: <7v392b8fv3.fsf@alter.siamese.dyndns.org>
Andy, do you have any thoughts on adding this email header to
contrib/hooks/post-receive-email? This patch shouldn't cause problems for anyone
with a sanely configured mail delivery agent, and the additional header is very
useful in toggling auto responses.
This conforms to RFC3834 and is useful in preventing eg
vacation auto-responders from replying by default
Signed-off-by: Chris Hiestand <chiestand@salk.edu>
---
contrib/hooks/post-receive-email | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
index b2171a0..0e5b72d 100755
--- a/contrib/hooks/post-receive-email
+++ b/contrib/hooks/post-receive-email
@@ -237,6 +237,7 @@ generate_email_header()
X-Git-Reftype: $refname_type
X-Git-Oldrev: $oldrev
X-Git-Newrev: $newrev
+ Auto-Submitted: auto-generated
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
--
1.7.10.4
On Sep 21, 2012, at 10:06 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Chris Hiestand <chiestand@salk.edu> writes:
>
>> My email in April went unanswered so I'm resending it. An Auto-Submitted header
>> would be an improvement to the standard [git] post receive email.
>>
>> Thanks,
>> Chris
>>
>>
>> Begin forwarded message:
>>
>>> From: Chris Hiestand <chiestand@salk.edu>
>>> Subject: [PATCH] Add Auto-Submitted header to post-receive-email
>>> Date: April 14, 2012 6:15:10 PM PDT
>>> To: git@vger.kernel.org, gitster@pobox.com
>>>
>>> Hi,
>>>
>>> I think the Auto-Submitted header is a useful hook mail header to include by default.
>>>
>>> This conforms to RFC3834 and is useful in preventing e.g. vacation auto-responders
>>> from replying by default.
>>>
>>> Perhaps you have already considered this and decided not to include it, but I found
>>> no record of such a conversation on this list.
>
> I think the lack of response is generally lack of interest, and the
> primary reason for that was because the To/Cc list did not contain
> anybody who touched this particular file in the past (and no, I am
> not among them; as contrib/README says, I am often the wrong person
> to ask if a patch to contrib/ material makes sense).
>
>>> From 358fc3ae1ebfd7723d54e4033d3e9a9a0322c873 Mon Sep 17 00:00:00 2001
>>> From: Chris Hiestand <chiestand@salk.edu>
>>> Date: Sat, 14 Apr 2012 17:58:39 -0700
>>> Subject: [PATCH] Add Auto-Submitted header to post-receive-email
>
> These four lines should not be in the body of the e-mail message
> (see Documentation/SubmittingPatches).
>
>>> Adds Auto-Submitted: auto-generated to post-receive-email header
>>> This conforms to RFC3834 and is useful in preventing eg
>>> vacation auto-responders from replying by default
>>> ---
>>> contrib/hooks/post-receive-email | 1 +
>>> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> Even for contrib/ material, please always sign-off your patch (see
> Documentation/SubmittingPatches).
>
>>>
>>> diff --git a/contrib/hooks/post-receive-email b/contrib/hooks/post-receive-email
>>> index 01af9df..282507c 100755
>>> --- a/contrib/hooks/post-receive-email
>>> +++ b/contrib/hooks/post-receive-email
>>> @@ -237,6 +237,7 @@ generate_email_header()
>>> X-Git-Reftype: $refname_type
>>> X-Git-Oldrev: $oldrev
>>> X-Git-Newrev: $newrev
>>> + Auto-Submitted: auto-generated
>>>
>>> This is an automated email from the git hooks/post-receive script. It was
>>> generated because a ref change was pushed to the repository containing
>
> I think the choice of "auto-generated" is a sensible one, as
> responding to a 'push' is like triggered by 'cron'.
>
> I'd however appreciate comments from people who either worked on
> this code or list regulars who actually use this code in the
> production.
>
> Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH v3 2/3] config: Introduce diff.algorithm variable
From: Junio C Hamano @ 2013-01-16 17:42 UTC (permalink / raw)
To: Michal Privoznik; +Cc: git, trast, peff
In-Reply-To: <4e2aacd5bbf005f0e372589bf423a8cbd776bc6d.1358322212.git.mprivozn@redhat.com>
Will replace the one in 'pu' with this round. Looking good.
Thanks.
^ permalink raw reply
* Re: [PATCH v6 0/8] push: update remote tags only with force
From: Jeff King @ 2013-01-16 17:43 UTC (permalink / raw)
To: Junio C Hamano
Cc: Max Horn, Chris Rorvick, git, Angelo Borsotti, Drew Northup,
Michael Haggerty, Philip Oakley, Johannes Sixt, Kacper Kornet,
Felipe Contreras
In-Reply-To: <7vfw21xde5.fsf@alter.siamese.dyndns.org>
On Wed, Jan 16, 2013 at 09:10:10AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I.e., we trigger the "!o" branch after the parse_object in your example.
>
> Heh, I didn't see this message until now (gmane seems to be lagging
> a bit).
I think it is vger lagging, actually.
> I am very tempted to do this.
>
> * Remove unnecessary not_forwardable from "struct ref"; it is only
> used inside set_ref_status_for_push();
>
> * "refs/tags/" is the only hierarchy that cannot be replaced
> without --force;
Agreed.
> * Remove the misguided attempt to force that everything that
> updates an existing ref has to be a commit outside "refs/tags/"
> hierarchy. This code does not know what kind of objects the user
> wants to place in "refs/frotz/" hierarchy it knows nothing about.
I agree with what your patch does, but my thinking is a bit different.
My original suggestion with respect to object types was that the rule
for --force should be "do not ever lose any objects without --force". So
a fast-forward is OK, as the new objects reference the old. A non-fast
forward is not, because objects become unreferenced. Replacing a tag
object is not OK, even if it points to the same commit, as you are
losing the old tag object (replacing an object with a tag that points to
the original object or its descendent is OK in theory, though I doubt it
is common enough to worry about).
I think that is a reasonable rule that could be applied across all parts
of the namespace hierarchy. And it could be applied by the client,
because all you need to know is whether ref->old_sha1 is reachable from
ref->new_sha1.
But it is somewhat orthogonal to the "already exists" idea, and checking
refs/tags/. Those ideas are about enforcing sane rules on the tag
hierarchy. My rule is a safety valve that is meant to extend the idea of
"is fast-forwardable" to non-commit object types. If we do it at all, it
should be part of the fast-forward check (e.g., as part of ref_newer).
The current code conflates the two under the "already exists" condition,
which is just wrong. I think the best thing at this point is to split
the two ideas apart, keep the refs/tags check (and translate it to
"already exists" in the UI, as we do), and table the safety valve. I am
not even sure if it is something that is useful, and it can come later
if we decide it is.
> I feel moderately strongly about the last point. Defining special
> semantics for one hierarchy (e.g. "refs/tags/") and implementing a
> policy for enforcement is one thing, but a random policy that
> depends on object type that applies globally is simply insane. The
> user may want to do "refs/tested/" hierarchy that is meant to hold
> references to commit, with one annotated tag "refs/tested/latest"
> that points at the "latest tested version" with some commentary, and
> maintain the latter by keep pushing to it. If that is the semantics
> the user wanted to ahve in the "refs/tested/" hierarchy, it is not
> reasonable to require --force for such a workflow. The user knows
> better than Git in such a case.
I see what you are saying, but I think the ship has already sailed to
some degree. We already implement the non-fast-forward check everywhere,
and I cannot have a "refs/tested" hierarchy that pushes arbitrary
commits without regard to their history. If I have such a hierarchy, I
have to use "--force" (or more likely, mark the refspec with "+").
In my mind, the object-type checking is just making that fast-forward
check more thorough (i.e., extending it to non-commit objects).
> cache.h | 1 -
> remote.c | 24 +-----------------------
> t/t5516-fetch-push.sh | 21 ---------------------
> 3 files changed, 1 insertion(+), 45 deletions(-)
The patch itself looks fine to me. Whether we agree on the fast-forward
object-type checking or not, it is the correct first step to take in
either case.
-Peff
^ 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