[PATCH] error_resolve_conflict: rewrap advice message
If you try to commit with unresolved conflicts in the index,
you get this message:
$ git commit
U foo
error: 'commit' is not possible because you have unmerged files.
hint: Fix them up in the work tree,
hint: and then use 'git add/rm ' as
hint: appropriate to mark resolution and make a commit,
hint: or use 'git commit -a'.
fatal: Exiting because of an unresolved conflict.
The irregular line-wrapping makes this awkward to read, and
it takes up more lines than necessary. Instead, let's rewrap
it to about 60 characters per line:
$ git commit
U foo
error: 'commit' is not possible because you have unmerged files.
hint: Fix them up in the work tree, and then use 'git add/rm '
hint: as appropriate to mark resolution and make a commit, or use
hint: 'git commit -a'.
fatal: Exiting because of an unresolved conflict.
Signed-off-by: Jeff King
---
This always bugs me when I see it, so I thought I'd finally do something
about it. I left the wording alone, though I'd also be happy to see it
shortened to just advise "git add/rm" and not mention "git commit -a" at
all.
advice.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/advice.c b/advice.c
index 486f823..ef24733 100644
--- a/advice.c
+++ b/advice.c
@@ -82,10 +82,9 @@ int error_resolve_conflict(const char *me)
* Message used both when 'git commit' fails and when
* other commands doing a merge do.
*/
- advise(_("Fix them up in the work tree,\n"
-"and then use 'git add/rm ' as\n"
-"appropriate to mark resolution and make a commit,\n"
-"or use 'git commit -a'."));
+ advise(_("Fix them up in the work tree, and then use 'git
add/rm '\n"
+"as appropriate to mark resolution and make a commit,
or use\n"
+"'git commit -a'."));
return -1;
}
--
2.0.0.rc1.436.g03cb729
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] error_resolve_conflict: drop quotations around operation
When you try to commit with unmerged entries, you get an
error like:
$ git commit
error: 'commit' is not possible because you have unmerged files.
The quotes around "commit" are clunky; the user doesn't care
that this message is a template with the command-name filled
in. Saying:
error: commit is not possible because you have unmerged files
is easier to read. As this code is called from other places,
we may also end up with:
$ git merge
error: merge is not possible because you have unmerged files
$ git cherry-pick foo
error: cherry-pick is not possible because you have unmerged files
$ git revert foo
error: revert is not possible because you have unmerged files
All of which look better without the quotes. This also
happens to match the behavior of "git pull", which generates
a similar message (but does not share code, as it is a shell
script).
Signed-off-by: Jeff King
---
I realize this may just be a matter of taste, but I thought I'd put it
forth as a possibility.
I also considered switching the wording to:
error: cannot commit because you have unmerged files
which I find a little more natural.
advice.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/advice.c b/advice.c
index ef24733..c50ebdf 100644
--- a/advice.c
+++ b/advice.c
@@ -76,7 +76,7 @@ int git_default_advice_config(const char *var, const char
*value)
int error_resolve_conflict(const char *me)
{
- error("'%s' is not possible because you have unmerged files.", me);
+ error("%s is not possible because you have unmerged files.", me);
if (advice_resolve_conflict)
/*
* Message used both when 'git commit' fails and when
--
2.0.0.rc1.436.g03cb729
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 1/2] config: Add hashtable for config parsing & retrieval
On Mon, Jun 02, 2014 at 07:47:39AM -0700, Tanay Abhra wrote:
> +static int config_cache_entry_cmp_icase(const struct config_cache_entry *e1,
> + const struct config_cache_entry *e2, const
> char* key)
> +{
> + return strcasecmp(e1->key, key ? key : e2->key);
> +}
> +
> +static void config_cache_init(void)
> +{
> + hashmap_init(&config_cache,
> (hashmap_cmp_fn)config_cache_entry_cmp_icase, 0);
> +}
It looks like this is meant to treat config keys like "diff.renames" as
case-insensitive. That's OK for two-part names, but longer names with a
middle section, like "remote.Foo.url", should have their middle sections
be case-sensitive.
Also, please avoid casting function ptrs. Give your function the correct
hashmap_cmp_fn signature, and cast from "void *" internally if you need
to.
> +static void config_cache_free(void)
> +{
> + struct config_cache_entry* entry;
> + struct hashmap_iter iter;
> + hashmap_iter_init(&config_cache, &iter);
> + while (entry = hashmap_iter_next(&iter)) {
Please wrap assignment inside a loop condition with an extra (), like:
while ((entry = hashmap_iter_next(&iter)))
This suppresses gcc's "did you mean ==" warning. If you are not
compiling with "-Wall -Werror" for development, you probably should be.
> +static struct config_cache_entry *config_cache_find_entry(const char *key)
> +{
> + struct hashmap_entry k;
> + hashmap_entry_init(&k, strihash(key));
> + return hashmap_get(&config_cache, &k, key);
> +}
As above, strihash isn't quite right here. It needs a custom function
that takes into account the case-sensitivity of the middle section of
the key name.
An alternative is to only feed already-normalized names into the hash
code, and then tell the hash to be case-sensitive.
> +static void config_cache_set_value(const char *key, const char *value)
> +{
> + struct config_cache_entry *e;
> +
> + e = config_cache_find_entry(key);
> + if (!e) {
> + e = malloc(sizeof(*e));
> + hashmap_entry_init(e, strihash(key));
> + e->key=xstrdup(key);
Style: please keep whitespace around "=".
> + } else {
> + if (!unsorted_string_list_has_string(e->value_list, value))
> + string_list_append(e->value_list, value);
> + }
I don't think we want to suppress duplicates here. If a caller reads the
value with git_config_get_string(), then the duplicates do not matter
(we show only the one with precedence). If they use the "multi()" form,
then they _should_ see each version. It is not up to the config code to
decide that duplicate values for the same key are not interesting; only
the caller knows that.
> +static void config_cache_remove_value(const char *key, const char *value)
> +{
> + struct config_cache_entry *e;
> + int i;
> +
> + e = config_cache_find_entry(key);
> + if(!e)
Style: if (!e)
> + /* If value == NULL then remove all the entries for the key */
> + if(!value) {
Ditto (and elsewhere, but I'll stop noting each).
> +extern const char *git_config_get_string(const char *name)
> +{
> + struct string_list *values;
> + values = config_cache_get_value(name);
> + if (!values)
> + return NULL;
> + return values->items[values->nr - 1].string;
> +}
The assumption here is that values->nr is never 0. I think that is true,
because we do not create the cache entry until we get a value (and
remove it if we drop the last value). However, it might be worth
documenting that with an assertion.
> @@ -333,6 +441,7 @@ static int get_value(config_fn_t fn, void *data, struct
> strbuf *name)
> if (!value)
> return -1;
> }
> + config_cache_set_value(name->buf, value);
> return fn(name->buf, value, data);
> }
The function get_value gets called for each entry from a parsed config
file. However, it does not get called for command-line config from "git
-c". You'd need to update git_config_parse_parameter for that.
However, I wonder if this is the right place to hook into the cache at
all. The cache should be able to sit _atop_ the existing callback
system. So you could easily implement it as a separate layer, and just
lazily initialize it like:
static int config_cache_callback(const char *key, const char *val, void
*cache)
{
config_cache_set_value(cache, key, val);
return 0;
}
static struct hashmap *get_config_cache(void)
{
static int initialized;
static struct hashmap cache;
if (!initialized) {
hash_map_init(&cache,
git_config(config_cache_callback, &cache);
initialized = 1;
}
}
and then teach config_cache_get_value and friends to access the cache
through "get_config_cache".
> @@ -1570,6 +1684,7 @@ int git_config_set_multivar_in_file(const char
> *config_filename,
> if (!store_write_section(fd, key) ||
> !s
Re: [PATCH] environment: enable core.preloadindex by default
Am 03.06.2014 08:18, schrieb Steve Hoelzer: > On Mon, Jun 2, 2014 at 3:01 PM, Karsten Blees wrote: >> Git for Windows users may want to try core.fscache=true as well [1]. This >> eliminates the UAC penalties for repositories located on the Windows system >> drive [2]. >> >> [1] https://github.com/msysgit/git/pull/94 >> [2] https://code.google.com/p/msysgit/issues/detail?id=320 > > Thanks for the tip! I didn't know about fscache, but I'll definitely > give it a try. Is there a reason it is not turned on by default in Git > for Windows? > > Steve > The feature has been merged only a few months ago, and I believe no one has found the time yet to do a thorough review of the workhorse compat/win32/fscache.[ch]. So the safe bet is to keep it turned off by default. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] environment: enable core.preloadindex by default
On Mon, Jun 2, 2014 at 11:43 PM, Steve Hoelzer wrote: > There is consensus that the default should change because it will > benefit nearly all users (some just a little, but some a lot). > See [1] and replies. > > [1]: > http://git.661346.n2.nabble.com/git-status-takes-30-seconds-on-Windows-7-Why-tp7580816p7580853.html Not only Windows. core.preloadindex helps large repos on Linux as well [2]. So +1. [2] http://article.gmane.org/gmane.comp.version-control.git/248013 -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config: Add documentation for writing config files
On 06/02/2014 12:37 PM, Matthieu Moy wrote: > Tanay Abhra writes: > >> Signed-off-by: Tanay Abhra >> --- >> Documentation/technical/api-config.txt | 31 ++- >> 1 file changed, 30 insertions(+), 1 deletion(-) > > Even though the reason to replace a TODO with real stuff is rather > straigthforward, the reader appriates a short commit message (ideally > pointing to the commit introducing the TODO). My first thought reading > the patch was "wtf, is that a patch in the middle of a series, where > does this TODO come from" ;-). Ok, I will send a new patch with the updated commit message. . >> +In the end they both all call `git_config_set_multivar_in_file` which takes >> +four parameters: > > Do we really want to document this as part of the config API? i.e. does > a normal user of the API want to know about this? My understanding is > that git_config_set_multivar_in_file is essentially a private function, > and then the best place to document is with comments near the function > definition (it already has such comment). Sorry, but I don't think so. In cache.h, the following functions have been provided as externs, git_config_set_in_file() git_config_set() git_config_set_multivar() git_config_set_multivar_in_file() extern int git_config_rename_section() extern int git_config_rename_section_in_file() Thus, they seem to be the part of the API. In most of the technical API docs I have read there is at least a description of all parameters of the main function also, relevant data structures if any. Also a certain amount of redundancy about details is allowed. A good example is api-hashmap.txt which provides detailed descriptions of each function and data structure which was very much helpful to me. Reverse is api-string-list.txt which was useless to me, so I had to go through the whole code to understand how to use it. Thanks for the review. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] string-list: Add a value to string_list initializer lists
STRING_LIST_INIT_{NODUP,DUP} initializers list values only
for earlier structure members, relying on the usual
convention in C that the omitted members are initailized to
0, i.e. the former is expanded to the latter:
struct string_list l = STRING_LIST_INIT_DUP;
struct string_list l = { NULL, 0, 0, 1 };
and the last member that is not mentioned (i.e. 'cmp') is
initialized to NULL.
While there is nothing wrong in this construct, spelling out
all the values where the macros are defined will serve also
as a documentation, so let's do so.
Signed-off-by: Tanay Abhra
---
V1: http://thread.gmane.org/gmane.comp.version-control.git/250560
Documentation/technical/api-string-list.txt | 2 ++
string-list.h | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-string-list.txt
b/Documentation/technical/api-string-list.txt
index 20be348..f1add51 100644
--- a/Documentation/technical/api-string-list.txt
+++ b/Documentation/technical/api-string-list.txt
@@ -200,3 +200,5 @@ Represents the list itself.
You should not tamper with it.
. Setting the `strdup_strings` member to 1 will strdup() the strings
before adding them, see above.
+. The `compare_strings_fn` member is used to specify a custom compare
+ function, otherwise `strcmp()` is used as the default function.
diff --git a/string-list.h b/string-list.h
index de6769c..dd5e294 100644
--- a/string-list.h
+++ b/string-list.h
@@ -15,8 +15,8 @@ struct string_list {
compare_strings_fn cmp; /* NULL uses strcmp() */
};
-#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0 }
-#define STRING_LIST_INIT_DUP { NULL, 0, 0, 1 }
+#define STRING_LIST_INIT_NODUP { NULL, 0, 0, 0, NULL }
+#define STRING_LIST_INIT_DUP { NULL, 0, 0, 1, NULL }
void print_string_list(const struct string_list *p, const char *text);
void string_list_clear(struct string_list *list, int free_util);
--
1.9.0.GIT
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0
On Mon, 2 Jun 2014 19:59:53 -0700 Linus Torvalds wrote: > On Mon, Jun 2, 2014 at 7:08 PM, NeilBrown wrote: > > > > git request-pull master git://neil.brown.name/md > > > > after tagging the current commit as "md/3.15-fixes" and pushing out the tag > > You should *tell* "git request-pull" what you're actually requesting to pull. > > The old "let's guess based on the commit at the top of your tree" may > have worked for you (because you only ever had one matching thing), > but it did not work in general. > > So the new "git request-pull" does not guess. If you want to request a > tag to be pulled, you name it. Because if you don't name it, it now > defaults to HEAD (like all other git log commands) rather than > guessing. So please write it as > >git request-pull master git://neil.brown.name/md md/3.15-fixes > > I guess the man-page should be made more explicit about this too. > > Linus OK, thanks. I guess I can live with being a bit more explicit. NeilBrown signature.asc Description: PGP signature
Re: [PATCH v2] string-list: Add a value to string_list initializer lists
Tanay Abhra writes: > diff --git a/Documentation/technical/api-string-list.txt > b/Documentation/technical/api-string-list.txt > index 20be348..f1add51 100644 > --- a/Documentation/technical/api-string-list.txt > +++ b/Documentation/technical/api-string-list.txt > @@ -200,3 +200,5 @@ Represents the list itself. >You should not tamper with it. > . Setting the `strdup_strings` member to 1 will strdup() the strings >before adding them, see above. > +. The `compare_strings_fn` member is used to specify a custom compare > + function, otherwise `strcmp()` is used as the default function. Is this change intentional? It seems good, but not really related to the change described in the commit message. In any case, this remark should not block patch inclusion, it's trivial enough. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC/PATCH v2 2/2] config: Add new query functions to the api docs
Tanay Abhra writes: > Add explanations for `git_config_get_string_multi` and `git_config_get_string` > which utilize the config hash table for querying in a non-callback manner for > a specific variable. I'd squash this into the previous patch: the reader appreciates having the documentation when reviewing the code itself (so that one can check that the documented behavior matches the implementation). > +the highest priority(i.e. value in the repo config will be preferred over Missing space before (. Other than that, this seems good. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] refs.c: change read_ref_at to use the reflog iterators
Thanks.
On Mon, Jun 2, 2014 at 2:21 PM, Junio C Hamano wrote:
> Ronnie Sahlberg writes:
>
>> read_ref_at has its own parsing of the reflog file for no really good reason
>> so lets change this to use the existing reflog iterators. This removes one
>> instance where we manually unmarshall the reflog file format.
>>
>> Log messages for errors are changed slightly. We no longer print the file
>> name for the reflog, instead we refer to it as 'Log for ref '.
>> This might be a minor useability regression, but I don't really think so,
>> since
>> experienced users would know where the log is anyway and inexperienced users
>> would not know what to do about/how to repair 'Log ... has gap ...' anyway.
>>
>> Adapt the t1400 test to handle the change in log messages.
>>
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> refs.c| 202
>> ++
>> t/t1400-update-ref.sh | 4 +-
>> 2 files changed, 107 insertions(+), 99 deletions(-)
>>
>> diff --git a/refs.c b/refs.c
>> index 6898263..b45bb2f 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -2936,109 +2936,117 @@ static char *ref_msg(const char *line, const char
>> *endp)
>> return xmemdupz(line, ep - line);
>> }
>
> If I am not mistaken, this function will become unused with this
> rewrite. Let's drop it and justify it in the log message.
Done.
>
>> +struct read_ref_at_cb {
>> + const char *refname;
>> + unsigned long at_time;
>> + int cnt;
>> + int reccnt;
>> + unsigned char *sha1;
>> + int found_it;
>> +
>> + char osha1[20];
>> + char nsha1[20];
>
> These should be unsigned char, shouldn't they?
Done.
>
>> + for_each_reflog_ent_reverse(refname, read_ref_at_ent, &cb);
>> +
>> + if (!cb.reccnt)
>> + die("Log for %s is empty.", refname);
>> + if (cb.found_it)
>> + return 0;
>> +
>> + for_each_reflog_ent(refname, read_ref_at_ent_oldest, &cb);
>
> Hmph.
>
> We have already scanned the same reflog in the backward direction in
> full. Do we need to make another call just to pick up the entry at
> the beginning? Is this because the callback is not told that it is
> fed the last entry (in other words, if there is some clue that this
> is the last call from for-each-reflog-ent-reverse to the callback,
> the callback could record the value it received in its cb-data)?
>
It is mainly because the callback does not provide the information
"this is the last entry".
We could add a flag for that to the callback arguments but I don't
know if it is worth it for this special case
since read_ref_at() for a timestamp that is outside the reflog horizon
should be uncommon.
regards
ronnie sahlberg
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] refs.c: change read_ref_at to use the reflog iterators
read_ref_at has its own parsing of the reflog file for no really good reason
so lets change this to use the existing reflog iterators. This removes one
instance where we manually unmarshall the reflog file format.
Remove the now redundant ref_msg function.
Log messages for errors are changed slightly. We no longer print the file
name for the reflog, instead we refer to it as 'Log for ref '.
This might be a minor useability regression, but I don't really think so, since
experienced users would know where the log is anyway and inexperienced users
would not know what to do about/how to repair 'Log ... has gap ...' anyway.
Adapt the t1400 test to handle the change in log messages.
Signed-off-by: Ronnie Sahlberg
---
refs.c| 208 +-
t/t1400-update-ref.sh | 4 +-
2 files changed, 105 insertions(+), 107 deletions(-)
diff --git a/refs.c b/refs.c
index 6898263..29eb7eb 100644
--- a/refs.c
+++ b/refs.c
@@ -2926,119 +2926,117 @@ int create_symref(const char *ref_target, const char
*refs_heads_master,
return 0;
}
-static char *ref_msg(const char *line, const char *endp)
-{
- const char *ep;
- line += 82;
- ep = memchr(line, '\n', endp - line);
- if (!ep)
- ep = endp;
- return xmemdupz(line, ep - line);
+struct read_ref_at_cb {
+ const char *refname;
+ unsigned long at_time;
+ int cnt;
+ int reccnt;
+ unsigned char *sha1;
+ int found_it;
+
+ unsigned char osha1[20];
+ unsigned char nsha1[20];
+ int tz;
+ unsigned long date;
+ char **msg;
+ unsigned long *cutoff_time;
+ int *cutoff_tz;
+ int *cutoff_cnt;
+};
+
+static int read_ref_at_ent(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp, int tz,
+ const char *message, void *cb_data)
+{
+ struct read_ref_at_cb *cb = cb_data;
+
+ cb->reccnt++;
+ cb->tz = tz;
+ cb->date = timestamp;
+
+ if (timestamp <= cb->at_time || cb->cnt == 0) {
+ if (cb->msg)
+ *cb->msg = xstrdup(message);
+ if (cb->cutoff_time)
+ *cb->cutoff_time = timestamp;
+ if (cb->cutoff_tz)
+ *cb->cutoff_tz = tz;
+ if (cb->cutoff_cnt)
+ *cb->cutoff_cnt = cb->reccnt - 1;
+ /*
+* we have not yet updated cb->[n|o]sha1 so they still
+* hold the values for the previous record.
+*/
+ if (!is_null_sha1(cb->osha1)) {
+ hashcpy(cb->sha1, nsha1);
+ if (hashcmp(cb->osha1, nsha1))
+ warning("Log for ref %s has gap after %s.",
+ cb->refname, show_date(cb->date,
cb->tz, DATE_RFC2822));
+ }
+ else if (cb->date == cb->at_time)
+ hashcpy(cb->sha1, nsha1);
+ else if (hashcmp(nsha1, cb->sha1))
+ warning("Log for ref %s unexpectedly ended on %s.",
+ cb->refname, show_date(cb->date, cb->tz,
+ DATE_RFC2822));
+ hashcpy(cb->osha1, osha1);
+ hashcpy(cb->nsha1, nsha1);
+ cb->found_it = 1;
+ return 1;
+ }
+ hashcpy(cb->osha1, osha1);
+ hashcpy(cb->nsha1, nsha1);
+ if (cb->cnt > 0)
+ cb->cnt--;
+ return 0;
+}
+
+static int read_ref_at_ent_oldest(unsigned char *osha1, unsigned char *nsha1,
+ const char *email, unsigned long timestamp,
+ int tz, const char *message, void *cb_data)
+{
+ struct read_ref_at_cb *cb = cb_data;
+
+ if (cb->msg)
+ *cb->msg = xstrdup(message);
+ if (cb->cutoff_time)
+ *cb->cutoff_time = timestamp;
+ if (cb->cutoff_tz)
+ *cb->cutoff_tz = tz;
+ if (cb->cutoff_cnt)
+ *cb->cutoff_cnt = cb->reccnt;
+ hashcpy(cb->sha1, osha1);
+ if (is_null_sha1(cb->sha1))
+ hashcpy(cb->sha1, nsha1);
+ /* We just want the first entry */
+ return 1;
}
int read_ref_at(const char *refname, unsigned long at_time, int cnt,
unsigned char *sha1, char **msg,
unsigned long *cutoff_time, int *cutoff_tz, int *cutoff_cnt)
{
- const char *logfile, *logdata, *logend, *rec, *lastgt, *lastrec;
- char *tz_c;
- int logfd, tz, reccnt = 0;
- struct stat st;
- unsigned long date;
- unsigned char logged_sha1[20];
- void *log_mapped;
- size_t mapsz;
+ struct read_ref_at_cb cb;
- logfile = git_path("logs/%s", refname);
- logfd = open(logfile, O_RDONLY, 0);
- if (logfd < 0)
-
Re: [PATCH v11 35/41] refs.c: pack all refs before we start to rename a ref
Thanks! On Fri, May 30, 2014 at 11:08 AM, Jonathan Nieder wrote: > Ronnie Sahlberg wrote: > >> We want to do this to make it easier to handle atomic renames in rename_ref >> for >> the case 'git branch -m foo/bar foo'. > > In an ideal world, a part of me would rather see "git branch -m" doing > something more targeted by only packing the two refs it is working > with, and only when it knows it has to so the normal "git branch -m > foo bar" case doesn't have to suffer. But: > > That would be more complicated. > > Packing refs is not very slow and has some good benefits for > performance of later commands. Once we've committed to writing out a > new pack-refs file, it's not so bad to enumerate all loose refs to get > more benefit from writing out the new packed-refs file. > > Renaming refs is something usually done by humans and not by scripts > or remote clients. I'm not too worried about "git branch -m" in a > tight loop getting slower. > > So I think this is an okay thing to do. > >> If we can guarantee that foo/bar does >> not >> exist as a loose ref we can avoid locking foo/bar.lock during transaction >> commit > > That is not quite true --- there's still value in locking foo/bar to > avoid clobbering a concurrent ref update. > > If git performed the entire rename transaction in the packed-refs > file, we could avoid that race completely (for 'git branch -m foo/bar > foo': lock refs, make sure the loose refs are pruned, perform the > update in packed-refs, unlock refs. for 'git branch -m foo foo/bar': > lock foo, prune foo, lock foo/bar, prune foo/bar, perform the update > in packed-refs, unlock refs). > > Even without doing that, packing refs first has a benefit in terms of > ordering: if you do (1) delete loose foo/bar, (2) write loose foo, (3) > rewrite packed-refs without foo/bar, then because you've packed refs > first, no objects become unreferenced during step (1), which means > that a hypothetical version of "git gc" that used filesystem snapshots > would not see any relevant objects as safe to clean up no matter when > it runs. > >> Signed-off-by: Ronnie Sahlberg >> --- >> refs.c| 3 +++ >> t/t3200-branch.sh | 6 +++--- >> 2 files changed, 6 insertions(+), 3 deletions(-) > > With a clearer commit message, > Reviewed-by: Jonathan Nieder -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 32/41] refs.c: make delete_ref use a transaction
Thanks!
On Fri, May 30, 2014 at 10:28 AM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> Signed-off-by: Ronnie Sahlberg
>> ---
>> refs.c | 34 +-
>> 1 file changed, 13 insertions(+), 21 deletions(-)
>
> Reviewed-by: Jonathan Nieder
>
> [...]
>> +++ b/refs.c
> [...]
>> @@ -2542,24 +2537,21 @@ static int delete_ref_loose(struct ref_lock *lock,
>> int flag, struct strbuf *err)
>>
>> int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>> {
> [...]
>> + if (!transaction ||
>> + ref_transaction_delete(transaction, refname, sha1, delopt,
>> +sha1 && !is_null_sha1(sha1), &err) ||
>> + ref_transaction_commit(transaction, NULL, &err)) {
>> + error("%s", err.buf);
>> + ref_transaction_free(transaction);
>> + strbuf_release(&err);
>> return 1;
>> + }
> [...]
>> - ret |= repack_without_ref(lock->ref_name);
>
> The old return value could be 1 or -1 depending on how the deletion
> failed. Now it's consistently 1.
>
> The only callers I see that care are cmd_symbolic_ref and
> cmd_update_ref, for which 1 is better (-1 would result in an exit
> status of 255, which means something like "died with signal 127").
>
> Thanks,
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit
On Fri, May 30, 2014 at 10:38 AM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>
>> Change the reference transactions so that we pass the reflog message
>> through to the create/delete/update function instead of the commit message.
>> This allows for individual messages for each change in a multi ref
>> transaction.
>
> Nice.
>
> That reminds me: in the future, do we want to have some way to figure
> out what ref updates happened together? E.g., cvsnt introduced commit
> identifiers to answer a similar kind of question in CVS per-file
> history. If some backend wants to support that, the API this patch
> introduces would handle it fine --- good.
>
> [...]
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url,
>> const char *remote_name,
>> }
>> }
>> }
>> -
>> if (rc & STORE_REF_ERROR_DF_CONFLICT)
>> error(_("some local refs could not be updated; try running\n"
>> - " 'git remote prune %s' to remove any old, conflicting "
>> + "'git remote prune %s' to remove any old, conflicting "
>> "branches"), remote_name);
>
> Unrelated change snuck in?
Yeah, there shouldn't be a space there.
>
> The rest of the patch is
> Reviewed-by: Jonathan Nieder
Thanks!
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index faa1233..55f457c 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -673,9 +673,10 @@ static int store_updated_refs(const char *raw_url, const
> char *remote_name,
> }
> }
> }
> +
> if (rc & STORE_REF_ERROR_DF_CONFLICT)
> error(_("some local refs could not be updated; try running\n"
> - "'git remote prune %s' to remove any old, conflicting "
> + " 'git remote prune %s' to remove any old, conflicting "
> "branches"), remote_name);
>
> abort:
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] error_resolve_conflict: drop quotations around operation
Jeff King writes:
> When you try to commit with unmerged entries, you get an
> error like:
>
> $ git commit
> error: 'commit' is not possible because you have unmerged files.
>
> The quotes around "commit" are clunky; the user doesn't care
> that this message is a template with the command-name filled
> in. Saying:
>
> error: commit is not possible because you have unmerged files
>
> is easier to read. As this code is called from other places,
> we may also end up with:
>
> $ git merge
> error: merge is not possible because you have unmerged files
>
> $ git cherry-pick foo
> error: cherry-pick is not possible because you have unmerged files
>
> $ git revert foo
> error: revert is not possible because you have unmerged files
>
> All of which look better without the quotes. This also
> happens to match the behavior of "git pull", which generates
> a similar message (but does not share code, as it is a shell
> script).
>
> Signed-off-by: Jeff King
> ---
> I realize this may just be a matter of taste, but I thought I'd put it
> forth as a possibility.
If the template were filled with 'git commit', 'git merge', etc.,
then the quotes around %s may make it clearer, but 'me' here is
designed to be a single subcommand name, so this change is good.
> I also considered switching the wording to:
>
> error: cannot commit because you have unmerged files
>
> which I find a little more natural.
Perhaps. As long as the subcommand name that can come here as 'me'
is always a verb, that would work (I didn't think about l10n, tho).
> advice.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/advice.c b/advice.c
> index ef24733..c50ebdf 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -76,7 +76,7 @@ int git_default_advice_config(const char *var, const char
> *value)
>
> int error_resolve_conflict(const char *me)
> {
> - error("'%s' is not possible because you have unmerged files.", me);
> + error("%s is not possible because you have unmerged files.", me);
> if (advice_resolve_conflict)
> /*
>* Message used both when 'git commit' fails and when
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/2] refs.c: optimize check_refname_component()
David Turner writes:
> static int check_refname_component(const char *refname, int flags)
> {
> const char *cp;
> char last = '\0';
>
> for (cp = refname; ; cp++) {
> - char ch = *cp;
> - if (ch == '\0' || ch == '/')
> + unsigned char ch = (unsigned char) *cp;
Hmph, this cast bothers me. I am fine with either of these two, though.
int ch = *cp & 0377;
unsigned char ch = *((unsigned char *)cp);
> + unsigned char disp = refname_disposition[ch];
> + switch(disp) {
> + case 1:
> + goto out;
> + case 2:
> + if (last == '.')
> + return -1; /* Refname contains "..". */
> + break;
> + case 3:
> + if (last == '@')
> + return -1; /* Refname contains "@{". */
> break;
> - if (bad_ref_char(ch))
> - return -1; /* Illegal character in refname. */
> - if (last == '.' && ch == '.')
> - return -1; /* Refname contains "..". */
> - if (last == '@' && ch == '{')
> - return -1; /* Refname contains "@{". */
> + case 4:
> + return -1;
> + }
> last = ch;
> }
> +out:
> if (cp == refname)
> return 0; /* Component has zero length. */
> if (refname[0] == '.') {
> diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
> index c289322..1571176 100755
> --- a/t/t5511-refspec.sh
> +++ b/t/t5511-refspec.sh
> @@ -5,7 +5,6 @@ test_description='refspec parsing'
> . ./test-lib.sh
>
> test_refspec () {
> -
> kind=$1 refspec=$2 expect=$3
> git config remote.frotz.url "." &&
> git config --remove-section remote.frotz &&
> @@ -84,4 +83,9 @@ test_refspec push
> 'refs/heads/*/*/for-linus:refs/remotes/mine/*' invalid
> test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*'
> test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*'
>
> +good=$(echo -n '\0377')
I think we avoid "echo -n" and use "printf" to be portable across
different echo implementations.
Use of \0377, which most likely to be just half-a-character, does
not feel a particularly good example, by the way.
> +test_refspec fetch "refs/heads/${good}"
> +bad=$(echo -n '\011')
Likewise.
> +test_refspec fetch "refs/heads/${bad}" invalid
> +
> test_done
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
On Fri, May 30, 2014 at 5:22 PM, Jonathan Nieder wrote:
> Hi,
>
> Ronnie Sahlberg wrote:
>
>> For these cases, save the errno value and abort and make sure that the caller
>> can see errno==ENOTDIR.
>>
>> Also start defining specific return codes for _commit, assign -1 as a generic
>> error and -2 as the error that refers to a name conflict. Callers can (and
>> should) use that return value inspecting errno directly.
>
> Heh. Here's a patch on top to hopefully finish that part of the job.
Thanks. Updated.
>
> Unfortunately, the value of errno after calling lock_ref_sha1_basic is
> not reliable.
I have changed the patch for lock_ref_sha1_basic so it now does :
if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
errno = EINVAL;
return NULL;
}
so this function should no longer fail without changing errno to a,
hopefully, sane value.
> http://thread.gmane.org/gmane.comp.version-control.git/249159/focus=249407
> lists the error paths that are broken (marked with "[!]" in that
> message).
>
> diff --git i/builtin/fetch.c w/builtin/fetch.c
> index b13e8f9..0a01cda 100644
> --- i/builtin/fetch.c
> +++ w/builtin/fetch.c
> @@ -377,6 +377,7 @@ static int s_update_ref(const char *action,
> char *rla = getenv("GIT_REFLOG_ACTION");
> struct ref_transaction *transaction;
> struct strbuf err = STRBUF_INIT;
> + int ret, df_conflict = 0;
>
> if (dry_run)
> return 0;
> @@ -387,16 +388,22 @@ static int s_update_ref(const char *action,
> transaction = ref_transaction_begin(&err);
> if (!transaction ||
> ref_transaction_update(transaction, ref->name, ref->new_sha1,
> - ref->old_sha1, 0, check_old, msg, &err) ||
> - ref_transaction_commit(transaction, &err)) {
> - ref_transaction_free(transaction);
> - error("%s", err.buf);
> - strbuf_release(&err);
> - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
> - STORE_REF_ERROR_OTHER;
> - }
> + ref->old_sha1, 0, check_old, msg, &err))
> + goto fail;
> +
> + ret = ref_transaction_commit(transaction, &err);
> + if (ret == UPDATE_REFS_NAME_CONFLICT)
> + df_conflict = 1;
> + if (ret)
> + goto fail;
> ref_transaction_free(transaction);
> return 0;
> +fail:
> + ref_transaction_free(transaction);
> + error("%s", err.buf);
> + strbuf_release(&err);
> + return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
> + : STORE_REF_ERROR_OTHER;
> }
>
> #define REFCOL_WIDTH 10
> diff --git i/refs.c w/refs.c
> index dbaabba..b22b99b 100644
> --- i/refs.c
> +++ w/refs.c
> @@ -3499,7 +3499,7 @@ static int ref_update_reject_duplicates(struct
> ref_update **updates, int n,
> int ref_transaction_commit(struct ref_transaction *transaction,
>struct strbuf *err)
> {
> - int ret = 0, delnum = 0, i, save_errno = 0;
> + int ret = 0, delnum = 0, i, df_conflict = 0;
> const char **delnames;
> int n = transaction->nr;
> struct ref_update **updates = transaction->updates;
> @@ -3535,7 +3535,7 @@ int ref_transaction_commit(struct ref_transaction
> *transaction,
>delnames, delnum);
> if (!update->lock) {
> if (errno == ENOTDIR)
> - save_errno = errno;
> + df_conflict = 1;
> if (err)
> strbuf_addf(err, "Cannot lock the ref '%s'.",
> update->refname);
> @@ -3590,8 +3590,7 @@ cleanup:
> if (updates[i]->lock)
> unlock_ref(updates[i]->lock);
> free(delnames);
> - errno = save_errno;
> - if (save_errno == ENOTDIR)
> + if (df_conflict)
> ret = -2;
> return ret;
> }
> diff --git i/refs.h w/refs.h
> index 88732a1..1583097 100644
> --- i/refs.h
> +++ w/refs.h
> @@ -325,9 +325,11 @@ int ref_transaction_delete(struct ref_transaction
> *transaction,
> * problem.
> * If the transaction is already in failed state this function will return
> * an error.
> - * Function returns 0 on success, -1 for generic failures and -2 if the
> - * failure was due to a name collision (ENOTDIR).
> + * Function returns 0 on success, -1 for generic failures and
> + * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
> + * collision (ENOTDIR).
> */
> +#define UPDATE_REFS_NAME_CONFLICT -2
> int ref_transaction_commit(struct ref_transaction *transaction,
>struct strbuf *err);
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
[PATCH v5 2/2] refs.c: SSE4.2 optimizations for check_refname_component
Optimize check_refname_component using SSE4.2, where available.
git rev-parse HEAD is a good test-case for this, since it does almost
nothing except parse refs. For one particular repo with about 60k
refs, almost all packed, the timings are:
Look up table: 29 ms
SSE4.2:25 ms
This is about a 15% improvement.
The configure.ac changes include code from the GNU C Library written
by Joseph S. Myers .
Signed-off-by: David Turner
---
Makefile | 6 +++
aclocal.m4 | 6 +++
configure.ac | 17
git-compat-util.h | 20 +
refs.c | 116 +
t/t5511-refspec.sh | 13 ++
6 files changed, 161 insertions(+), 17 deletions(-)
diff --git a/Makefile b/Makefile
index a53f3a8..dd2127a 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,6 +1326,11 @@ else
COMPAT_OBJS += compat/win32mmap.o
endif
endif
+ifdef NO_SSE42
+ BASIC_CFLAGS += -DNO_SSE42
+else
+ BASIC_CFLAGS += -msse4.2
+endif
ifdef OBJECT_CREATION_USES_RENAMES
COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
endif
@@ -2199,6 +2204,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
+ @echo NO_SSE42=\''$(subst ','\'',$(subst ','\'',$(NO_SSE42)))'\' >>$@
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@
endif
diff --git a/aclocal.m4 b/aclocal.m4
index f11bc7e..d9f3f19 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -38,3 +38,9 @@ AC_DEFUN([TYPE_SOCKLEN_T],
[#include
#include ])
])
+
+dnl Test a compiler option or options with an empty input file.
+dnl LIBC_TRY_CC_OPTION([options], [action-if-true], [action-if-false])
+AC_DEFUN([LIBC_TRY_CC_OPTION],
+[AS_IF([AC_TRY_COMMAND([${CC-cc} $1 -xc /dev/null -S -o /dev/null])],
+ [$2], [$3])])
diff --git a/configure.ac b/configure.ac
index b711254..3a5bda9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -382,6 +382,23 @@ AS_HELP_STRING([],[Tcl/Tk interpreter will be found in a
system.]),
GIT_PARSE_WITH(tcltk))
#
+# Declare the with-sse42/without-sse42 options.
+AC_ARG_WITH(sse42,
+AS_HELP_STRING([--with-sse42],[use SSE4.2 instructions])
+AS_HELP_STRING([],[(default is YES if your compiler supports -msse4.2)]),
+GIT_PARSE_WITH(sse42))
+
+if test "$NO_SSE42" != "YesPlease"; then
+ dnl Check if -msse4.2 works.
+ AC_CACHE_CHECK(for SSE4.2 support, cc_cv_sse42, [dnl
+ LIBC_TRY_CC_OPTION([-msse4.2], [cc_cv_sse42=yes], [cc_cv_sse42=no])
+ ])
+ if test $cc_cv_sse42 = no; then
+ NO_SSE42=1
+ fi
+fi
+
+GIT_CONF_SUBST([NO_SSE42])
## Checks for programs.
AC_MSG_NOTICE([CHECKS for programs])
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..254487a 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -668,6 +668,26 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#endif
#endif
+#ifndef NO_SSE42
+#include
+/* Clang ships with a version of nmmintrin.h that's incomplete; if
+ * necessary, we define the constants that we're going to use. */
+#ifndef _SIDD_UBYTE_OPS
+#define _SIDD_UBYTE_OPS 0x00
+#define _SIDD_CMP_EQUAL_ANY 0x00
+#define _SIDD_CMP_RANGES0x04
+#define _SIDD_CMP_EQUAL_ORDERED 0x0c
+#define _SIDD_NEGATIVE_POLARITY 0x10
+#endif
+
+/* This is the system memory page size; it's used so that we can read
+ * outside the bounds of an allocation without segfaulting. It is
+ * assumed to be a power of 2. */
+#ifndef PAGE_SIZE
+#define PAGE_SIZE 4096
+#endif
+#endif
+
#ifdef UNRELIABLE_FSTAT
#define fstat_is_reliable() 0
#else
diff --git a/refs.c b/refs.c
index 46139d2..532aaf4 100644
--- a/refs.c
+++ b/refs.c
@@ -24,6 +24,25 @@ static unsigned char refname_disposition[256] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
};
+static int check_refname_component_trailer(const char *cp, const char
*refname, int flags)
+{
+ if (cp == refname)
+ return 0; /* Component has zero length. */
+ if (refname[0] == '.') {
+ if (!(flags & REFNAME_DOT_COMPONENT))
+ return -1; /* Component starts with '.'. */
+ /*
+* Even if leading dots are allowed, don't allow "."
+* as a component (".." is prevented by a rule above).
+*/
+ if (refname[1] == '\0')
+ return -1; /* Component equals ".". */
+ }
+ if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
+ return -1; /* Refname ends with ".lock". */
+ return cp - refname;
+}
+
/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component i
[PATCH v5 1/2] refs.c: optimize check_refname_component()
In a repository with many refs, check_refname_component can be a major
contributor to the runtime of some git commands. One such command is
git rev-parse HEAD
Timings for one particular repo, with about 60k refs, almost all
packed, are:
Old: 35 ms
New: 29 ms
Many other commands which read refs are also sped up.
Signed-off-by: David Turner
---
refs.c | 67 +++---
t/t5511-refspec.sh | 6 -
2 files changed, 44 insertions(+), 29 deletions(-)
diff --git a/refs.c b/refs.c
index 28d5eca..46139d2 100644
--- a/refs.c
+++ b/refs.c
@@ -6,8 +6,29 @@
#include "string-list.h"
/*
- * Make sure "ref" is something reasonable to have under ".git/refs/";
- * We do not like it if:
+ * How to handle various characters in refnames:
+ * 0: An acceptable character for refs
+ * 1: End-of-component
+ * 2: ., look for a preceding . to reject .. in refs
+ * 3: {, look for a preceding @ to reject @{ in refs
+ * 4: A bad character: ASCII control characters, "~", "^", ":" or SP
+ */
+static unsigned char refname_disposition[256] = {
+ 1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
+ 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
+ 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 2, 1,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
+};
+
+/*
+ * Try to read one refname component from the front of refname.
+ * Return the length of the component found, or -1 if the component is
+ * not legal. It is legal if it is something reasonable to have under
+ * ".git/refs/"; We do not like it if:
*
* - any path component of it begins with ".", or
* - it has double dots "..", or
@@ -16,41 +37,31 @@
* - it ends with ".lock"
* - it contains a "\" (backslash)
*/
-
-/* Return true iff ch is not allowed in reference names. */
-static inline int bad_ref_char(int ch)
-{
- if (((unsigned) ch) <= ' ' || ch == 0x7f ||
- ch == '~' || ch == '^' || ch == ':' || ch == '\\')
- return 1;
- /* 2.13 Pattern Matching Notation */
- if (ch == '*' || ch == '?' || ch == '[') /* Unsupported */
- return 1;
- return 0;
-}
-
-/*
- * Try to read one refname component from the front of refname. Return
- * the length of the component found, or -1 if the component is not
- * legal.
- */
static int check_refname_component(const char *refname, int flags)
{
const char *cp;
char last = '\0';
for (cp = refname; ; cp++) {
- char ch = *cp;
- if (ch == '\0' || ch == '/')
+ int ch = *cp & 255;
+ unsigned char disp = refname_disposition[ch];
+ switch(disp) {
+ case 1:
+ goto out;
+ case 2:
+ if (last == '.')
+ return -1; /* Refname contains "..". */
+ break;
+ case 3:
+ if (last == '@')
+ return -1; /* Refname contains "@{". */
break;
- if (bad_ref_char(ch))
- return -1; /* Illegal character in refname. */
- if (last == '.' && ch == '.')
- return -1; /* Refname contains "..". */
- if (last == '@' && ch == '{')
- return -1; /* Refname contains "@{". */
+ case 4:
+ return -1;
+ }
last = ch;
}
+out:
if (cp == refname)
return 0; /* Component has zero length. */
if (refname[0] == '.') {
diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
index c289322..de6db86 100755
--- a/t/t5511-refspec.sh
+++ b/t/t5511-refspec.sh
@@ -5,7 +5,6 @@ test_description='refspec parsing'
. ./test-lib.sh
test_refspec () {
-
kind=$1 refspec=$2 expect=$3
git config remote.frotz.url "." &&
git config --remove-section remote.frotz &&
@@ -84,4 +83,9 @@ test_refspec push
'refs/heads/*/*/for-linus:refs/remotes/mine/*' invalid
test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*'
test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*'
+good=$(printf '\303\204')
+test_refspec fetch "refs/heads/${good}"
+bad=$(printf '\011tab')
+test_refspec fetch "refs/heads/${bad}" invalid
+
test_done
--
2.0.0.rc1.18.gf763c0f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Paper cut bug: Why isn't "git clone xxxx" recursive by default?
Hello git devs! I'd like to start off by saying that git is an amazing piece of software and every one of you deserve major kudos for your work on the project. However, I'd like to point out a few "paper cut" bugs (to use the Ubuntu parlance). Apologies if this question has been asked already, but what is the reasoning behind making git clone not recursive (--recursive) by default? I have just recently started splitting my projects into submodules, and I feel like this is a major usability issue, especially for newbies. Wouldn't it be better to have a "--non-recursive" option and clone recursively by default? Similarly, I feel that "git pull" should automatically "git submodule update --recursive --init" as well, with the current behavior able to be specified with a "--non-recursive" option. I feel like these sorts of choices make submodules seem very much like second class citizens in git and make git much less user friendly. I feel that the most common use case that people want is to keep submodules properly in sync. In addition, I feel that power users that really want to make shallow clones, non-recursive clones, etc. could still be served with a simple option. I guess there are problems with changes in submodules being overwritten, so I suppose there would need to be additional warnings or even just refusal to pull into dirty directories, similar to the way git behaves in a regular repository. Thanks for the excellent work, Mara Kim Ph.D. Candidate Computational Biology Vanderbilt University Nashville, TN -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 00/41] Use ref transactions
Hi, Ronnie Sahlberg wrote: >It > converts all ref updates, inside refs.c as well as external, to use the > transaction API for updates. This makes most of the ref updates to become > atomic when there are failures locking or writing to a ref. I'm still excited about this series. Most of it looks ready. The remaining problems I see fall into a few categories: * The rename_ref codepath is strange. You've convinced me that the approach you're taking there is not much of a regression, but it's confusing enough that I'd be happier if someone else takes a closer look (or I can try to find time to). * I think the approach taken in the patch "add transaction.status and track OPEN/CLOSED/ERROR" is a mistake and would make callers more error-prone. The basic idea of checking that the caller is using the API right is valuable, so there is something in that patch I really like --- it's just the details (involving the same kind of easy-to-clobber error messages as errno provides with stdio) that seem broken. I suspect I'm just not communicating very well there. Maybe mhagger or someone else could give it a sanity check. * Some commit messages (e.g., the one to "pack all refs before we start to rename a ref") are confusing. That might be a sign that what those patches are trying to do is confusing. * The error handling in "add an err argument to repack_without_refs" is a thorny thicket. It still has bugs. I can completely understand not wanting to take that on but I think it is important to add a NEEDSWORK comment describing the known bugs to help people when they work with this code in the future. I realize the process of addressing review comments in such a long series has been a bit of a pain, and if there's anything I can do to make it easier please let me know. Hopefully adding some other reviewers can help. Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic
On Fri, May 30, 2014 at 11:12 AM, Jonathan Nieder wrote: > Ronnie Sahlberg wrote: > >> Move the check for check_refname_format from lock_any_ref_for_update >> to lock_ref_sha1_basic. At some later stage we will get rid of >> lock_any_ref_for_update completely. > > Do you know if this will cause any functional change? > > What is the potential impact? Is that impact worth it? (Given how > broken the recovery codepaths currently are, I suspect this change is > worth it, but it seems worth documenting in the log message.) Thanks. Updated the commit message to mention this. This changes semantics for lock_ref_sha1_basic slightly. With this change it is no longer possible to open a ref that has a badly name which breaks any codepaths that tries to open and repair badly named refs. The normal refs API should not allow neither creating nor accessing refs with invalid names. If we need such recovery code we could add it as an option to git fsck and have git fsck be the only sanctioned way of bypassing the normal API and checks. > > Thanks, > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] receive-pack: optionally deny case-clone refs
It is possible to have two branches which are the same but for case.
This works great on the case-sensitive filesystems, but not so well on
case-insensitive filesystems. It is fairly typical to have
case-insensitive clients (Macs, say) with a case-sensitive server
(GNU/Linux).
Should a user attempt to pull on a Mac when there are case-clone
branches with differing contents, they'll get an error message
containing something like "Ref refs/remotes/origin/lower is at
[sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]
(unable to update local ref)"
With a case-insensitive git server, if a branch called capital-M
Master (that differs from lowercase-m-master) is pushed, nobody else
can push to (lowercase-m) master until the branch is removed.
Create the option receive.denycaseclonebranches, which checks pushed
branches to ensure that they are not case-clones of an existing
branch. This setting is turned on by default if core.ignorecase is
set, but not otherwise.
Signed-off-by: David Turner
---
builtin/receive-pack.c | 29 -
t/t5400-send-pack.sh | 20
2 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..0894ded 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -27,6 +27,7 @@ enum deny_action {
static int deny_deletes;
static int deny_non_fast_forwards;
+static int deny_case_clone_branches = -1;
static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
static int receive_fsck_objects = -1;
@@ -69,6 +70,11 @@ static int receive_pack_config(const char *var, const char
*value, void *cb)
if (status)
return status;
+ if (strcmp(var, "receive.denycaseclonebranches") == 0) {
+ deny_case_clone_branches = git_config_bool(var, value);
+ return 0;
+ }
+
if (strcmp(var, "receive.denydeletes") == 0) {
deny_deletes = git_config_bool(var, value);
return 0;
@@ -468,6 +474,24 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
+static int is_case_clone(const char *refname, const unsigned char *sha1,
+ int flags, void *cb_data)
+{
+ const char* incoming_refname = cb_data;
+ return !strcasecmp(refname, incoming_refname) &&
+ strcmp(refname, incoming_refname);
+}
+
+static int ref_is_denied_case_clone(const char *name)
+{
+
+ if (!deny_case_clone_branches)
+ return 0;
+
+ return for_each_ref(is_case_clone, (void *) name);
+
+}
+
static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd->ref_name;
@@ -478,7 +502,8 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
struct ref_lock *lock;
/* only refs/... are allowed */
- if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
+ if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0) ||
+ ref_is_denied_case_clone(name)) {
rp_error("refusing to create funny ref '%s' remotely", name);
return "funny refname";
}
@@ -1171,6 +1196,8 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
die("'%s' does not appear to be a git repository", dir);
git_config(receive_pack_config, NULL);
+ if (deny_case_clone_branches == -1)
+ deny_case_clone_branches = ignore_case;
if (0 <= transfer_unpack_limit)
unpack_limit = transfer_unpack_limit;
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 0736bcb..099c0e3 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force' '
test "$victim_orig" = "$victim_head"
'
+if ! test_have_prereq CASE_INSENSITIVE_FS
+then
+test_expect_success 'denyCaseCloneBranches works' '
+ (
+ cd victim &&
+ git config receive.denyCaseCloneBranches true
+ git config receive.denyDeletes false
+ ) &&
+ git checkout -b caseclone &&
+ git send-pack ./victim caseclone &&
+ git checkout -b CaseClone &&
+ test_must_fail git send-pack ./victim CaseClone &&
+ git checkout -b notacaseclone &&
+ git send-pack ./victim notacaseclone &&
+ test_must_fail git send-pack ./victim :CaseClone &&
+ git send-pack ./victim :caseclone &&
+ git send-pack ./victim CaseClone
+'
+fi
+
test_expect_success 'push --all excludes remote-tracking hierarchy' '
mkdir parent &&
(
--
2.0.0.rc1.18.gf763c0f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/maj
Re: Paper cut bug: Why isn't "git clone xxxx" recursive by default?
Mara Kim writes: > Apologies if this question has been asked already, but what is the > reasoning behind making git clone not recursive (--recursive) by > default? The primary reason why submodules are separate repositories is not to require people to have everything. Some people want recursive, some others don't, and the world is not always "majority wins" (not that I am saying that majority will want recursive). Inertia, aka backward compatibility and not surprising existing users, plays some role when deciding the default. Also, going --recursive when the user did not want is a lot more expensive mistake to fix than not being --recursive when the user wanted to. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reset by checkout?
On 03/06/2014 00:54, Junio C Hamano wrote:
Not that I can think of a better way to update these descriptions,
and not that I am opposing to update these descriptions to make it
easier for new people to learn, but I am not sure if these "treat
ORIG_HEAD and the changes since that commit as separate entities"
is a good approach to do so.
Somewhat frustrated, not by your patch but by being unable to
suggest a better way X-<.
I know. I started off myself knowing what I meant to say, and then got
bogged down somewhat trying to be detailed enough for a full
explanation. I think it's just inherently very hard for anyone to
visualise what these do in the /general/ case.
This is one of those commands where the structure of a man page gets in
the way. We have to give a summary of what the mode options /do/, but
that's not what people want to know. They want to know what they're /for/.
(And, to some extent, reset, like checkout, is two separate commands.
One being the path manipulator, the other being the HEAD manipulator.
Just bogs us down further).
I think these are the most important HEAD resets, covering 95%+ of uses:
git reset --soft HEAD~
git reset HEAD~
git reset --keep HEAD~
git reset --keep ORIG_HEAD
git reset --keep @{}
git reset --keep
(and possibly
git reset --merge
although I think this should be fully covered by "git xxx --abort" -
maybe a couple of those missing like git stash pop/apply --abort?)
Anything more than those, I think, are pretty far-fetched. I can't 100%
grok "--soft/--mixed" onto a different branch, for example. (But at
least we do define those cases in the A/B/C/D "discussion" section for
the real geeks.)
Maybe we just need to tighten up the EXAMPLES section? Give it
easy-to-locate /--soft/--mixed/--keep subheadings, covering all
those common use cases (in clean trees...), including a before/after git
status views. Then normal users could skip the top technical section
waffling about indexes and go straight there instead.
Kevin
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 06/41] refs.c: add an err argument to repack_without_refs
Thanks. I have done all the additions of save_errno you suggested. On Thu, May 29, 2014 at 11:17 AM, Jonathan Nieder wrote: > Hi, > > Ronnie Sahlberg wrote: > >> Update repack_without_refs to take an err argument and update it if there >> is a failure. Pass the err variable from ref_transaction_commit to this >> function so that callers can print a meaningful error message if _commit >> fails due to a problem in repack_without_refs. >> >> Add a new function unable_to_lock_message that takes a strbuf argument and >> fills in the reason for the failure. >> >> In commit_packed_refs, make sure that we propagate any errno that >> commit_lock_file might have set back to our caller. >> >> Signed-off-by: Ronnie Sahlberg >> --- >> cache.h| 2 ++ >> lockfile.c | 21 - >> refs.c | 25 +++-- >> 3 files changed, 33 insertions(+), 15 deletions(-) > > I don't want to sound like a broken record, but this still has the > same issues I described before at > http://thread.gmane.org/gmane.comp.version-control.git/250197/focus=250309. > > The commit message or documentation or notes after the three dashes > above could explain what I missed when making my suggestions. > Otherwise I get no reality check as a reviewer, other reviewers get > less insight into what's happening in the patch, people in the future > looking into the patch don't understand its design as well, etc. > > As a general rule, that is a good practice anyway --- even when a > reviewer was confused, what they got confused about can be an > indication of where to make the code or design documentation (commit > message) more clear, and when reposting a patch it can be a good > opportunity to explain how the patch evolved. > > What would be wrong with the line of API documentation and the TODO > comment for a known bug I suggested? If they are a bad idea, can you > explain that so I can learn from it? Or if they were confusing, would > you like a patch that explains what I mean? > > Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] Add an explicit GIT_DIR to the list of excludes
On Thu, May 29, 2014 at 4:42 AM, Duy Nguyen wrote:
+ const char *worktree = get_git_work_tree();
+
+ if (worktree == NULL ||
+ dir_inside_of(n_git, get_git_work_tree()) >= 0) {
+ struct exclude_list *el = add_exclude_list(dir,
EXC_CMDL,
+ "GIT_DIR setup");
+
+ /* append a trailing slash to exclude directories
*/
+ n_git[len] = '/';
+ n_git[len + 1] = '\0';
+ add_exclude(basename, "", 0, el, 0);
>
> Hmm.. I overlooked this bit before. So if $GIT_DIR is /something/foo,
> we set to ignore "foo/". Because we know n_git must be part of
> (normalized) get_git_work_tree() at this point, could we path n_git +
> strlen(get_git_work_tree()) to add_exclude() instead of basename? Full
> path makes sure we don't accidentally exclude too much.
I guess so. In fact, dir_inside_of() already returns the relative
position, can reuse that (however, that function doesn't include the
leading path, making it a relative path; but it's not difficult to
work around). The only uncovered situation is when GIT_DIR=WORK_TREE.
But that's user's fault and I don't think we need to guarantee that
GIT_DIR will be excluded then
>
> The case when $GIT_DIR points to a _file_ seems uncovered.
> setup_git_directory() will transform the file to the directory
> internally and we never know the .git file's path (so we can't exclude
> it). So people could accidentally add the .git file in, then remove it
> from from work tree and suddenly the work tree becomes repo-less. It's
> not as bad as .git _directory_ because we don't lose valuable data. I
> don't know if you want to cover this too.
That's right, there is no way of knowing what the original .git file
was. I guess the only way to work around this problem is to modify
"read_gitfile()" so it saves the name of the original file. Then we
can add both that .git-file and GIT_DIR to the exclude list. Not a big
problem with me, but need to see what you guys think
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Paper cut bug: Why isn't "git clone xxxx" recursive by default?
Junio C Hamano writes: > Mara Kim writes: > >> Apologies if this question has been asked already, but what is the >> reasoning behind making git clone not recursive (--recursive) by >> default? > > The primary reason why submodules are separate repositories is not > to require people to have everything. Some people want recursive, > some others don't, and the world is not always "majority wins" (not > that I am saying that majority will want recursive). > > Inertia, aka backward compatibility and not surprising existing > users, plays some role when deciding the default. > > Also, going --recursive when the user did not want is a lot more > expensive mistake to fix than not being --recursive when the user > wanted to. Having said all that, I do not mean to say that I am opposed to introduce some mechanism to let the users express their preference between recursive and non-recursive better, so that "git clone" without an explicit --recursive (or --no-recursive) can work to their taste. A configuration in $HOME/.gitconfig might be a place to start, even though that has the downside of assuming that the given user would want to use the same settings for all his projects, which may not be the case in practice. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 25/41] fast-import.c: use a ref transaction when dumping tags
Thanks.
I have changed the transaction functions to die(BUG:) if the user
tries to call _update/_create/_delete on a failed transaction.
On Thu, May 29, 2014 at 10:41 AM, Jonathan Nieder wrote:
> Ronnie Sahlberg wrote:
>> On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder wrote:
>
>>> Usually when ref_transaction_commit is called I can do
>>>
>>> struct strbuf err = STRBUF_INIT;
>>> if (ref_transaction_commit(..., &err))
>>> die("%s", err.buf);
>>>
>>> and I know that since ref_transaction_commit has returned a nonzero
>>> result, err.buf is populated with a sensible message that will
>>> describe what went wrong.
> [...]
>>> But the guarantee you are describing removes that property. It
>>> creates a case where ref_transaction_commit can return nonzero without
>>> updating err. So I get the following message:
>>>
>>> fatal:
>>>
>>> I don't think that's a good outcome.
>>
>> In this case "fatal:" can not happen.
>> This is no more subtle than most of the git core.
>>
>> I have changed this function to explicitly abort on _update failing
>> but I think this is making the api too restrictive.
>
> I don't want to push you toward making a change you think is wrong. I
> certainly don't own the codebase, and there are lots of other people
> (e.g., Michael, Junio, Jeff) to get advice from. So I guess I should
> try to address this.
>
> I'm not quite sure what you mean by too restrictive.
>
> a. Having API constraints that aren't enforced by the function makes
> using the API too fussy.
>
> I agree with that. That was something I liked about keeping track
> of the OPEN/CLOSED state of a transaction, which would let
> functions like _commit die() if someone is misusing the API so the
> problem gets detected early.
>
> b. Having to check the return value from _update() is too fussy.
>
> It certainly seems *possible* to have an API that doesn't require
> checking the return value, while still avoiding the usability
> problem I described in the quoted message above. For example:
>
> * _update() returns void and has no strbuf parameter
> * error handling happens by checking the error from _commit()
>
> That would score well on the scale described at
> http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html
>
> An API where checking the return value is optional would be
> doable, too. For example:
>
> * _update() returns int and has a strbuf parameter
> * if the strbuf parameter is NULL, the caller is expected to
>wait for _commit() to check for errors, and a relevant
>message will be passed back then
> * if the strbuf parameter is non-NULL, then calling _commit()
>after an error is an API violation
>
> I don't understand the comment about no more subtle than most of git.
> Are you talking about the errno action at a distance you found in some
> functions? I thought we agreed that those were mistakes that accrue
> when people aim for a quick fix without thinking about maintainability
> and something git should have less of.
>
> Jonathan
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] receive-pack: optionally deny case-clone refs
David Turner writes:
> It is possible to have two branches which are the same but for case.
> This works great on the case-sensitive filesystems, but not so well on
> case-insensitive filesystems. It is fairly typical to have
> case-insensitive clients (Macs, say) with a case-sensitive server
> (GNU/Linux).
>
> Should a user attempt to pull on a Mac when there are case-clone
> branches with differing contents, they'll get an error message
> containing something like "Ref refs/remotes/origin/lower is at
> [sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]
> (unable to update local ref)"
>
> With a case-insensitive git server, if a branch called capital-M
> Master (that differs from lowercase-m-master) is pushed, nobody else
> can push to (lowercase-m) master until the branch is removed.
>
> Create the option receive.denycaseclonebranches, which checks pushed
> branches to ensure that they are not case-clones of an existing
> branch. This setting is turned on by default if core.ignorecase is
> set, but not otherwise.
>
> Signed-off-by: David Turner
> ---
I do not object to this new feature in principle, but I do not know
if we want to introduce a new word "case-clone refs" without adding
it to the glossary documentation.
It feels a bit funny to tie this to core.ignorecase, which is an
attribute of the filesystem used for the working tree, though.
Updates to Documentation/config.txt and Documentation/git-push.txt
are also needed.
> builtin/receive-pack.c | 29 -
> t/t5400-send-pack.sh | 20
> 2 files changed, 48 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index c323081..0894ded 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -27,6 +27,7 @@ enum deny_action {
>
> static int deny_deletes;
> static int deny_non_fast_forwards;
> +static int deny_case_clone_branches = -1;
> static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
> static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
Would it make sense to reuse the enum deny_action for this new
settings, with an eye to later convert the older boolean ones also
to use enum deny_action to make them consistent and more flexible?
> @@ -69,6 +70,11 @@ static int receive_pack_config(const char *var, const char
> *value, void *cb)
> if (status)
> return status;
>
> + if (strcmp(var, "receive.denycaseclonebranches") == 0) {
> + deny_case_clone_branches = git_config_bool(var, value);
> + return 0;
> + }
> +
> if (strcmp(var, "receive.denydeletes") == 0) {
> deny_deletes = git_config_bool(var, value);
> return 0;
> @@ -468,6 +474,24 @@ static int update_shallow_ref(struct command *cmd,
> struct shallow_info *si)
> return 0;
> }
>
> +static int is_case_clone(const char *refname, const unsigned char *sha1,
> + int flags, void *cb_data)
> +{
> + const char* incoming_refname = cb_data;
We write C not C++ around here; the pointer-asterisk sticks to the
variable name, not typename.
> + return !strcasecmp(refname, incoming_refname) &&
> + strcmp(refname, incoming_refname);
(Mental note to the reviewer himself) This returns true iff there is
an existing ref whose name is only different in case, and cause
for-each-ref to return early with true. In a sane case of not
receiving problematic refs, this will have to iterate over all the
existing refnames. Wonder if there are better ways to optimize this
in a repository with hundreds or thousands of refs, which is not all
that uncommon.
> +}
> +
> +static int ref_is_denied_case_clone(const char *name)
> +{
> +
> + if (!deny_case_clone_branches)
> + return 0;
> +
> + return for_each_ref(is_case_clone, (void *) name);
> +
The trailing blank line inside a function at the end is somewhat
unusual.
> +}
> +
> diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
> index 0736bcb..099c0e3 100755
> --- a/t/t5400-send-pack.sh
> +++ b/t/t5400-send-pack.sh
> @@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps --force'
> '
> test "$victim_orig" = "$victim_head"
> '
>
> +if ! test_have_prereq CASE_INSENSITIVE_FS
> +then
Hmm, don't we want the feature to kick in for both case sensitive
and case insensitive filesystems?
> +test_expect_success 'denyCaseCloneBranches works' '
> + (
> + cd victim &&
> + git config receive.denyCaseCloneBranches true
> + git config receive.denyDeletes false
> + ) &&
> + git checkout -b caseclone &&
> + git send-pack ./victim caseclone &&
> + git checkout -b CaseClone &&
> + test_must_fail git send-pack ./victim CaseClone &&
At this point, we would want to see not just that send-pack fails
but also that "victim" does not have CaseClone branch and does have
caseclone branch pointing at the original va
[PATCH v13 03/41] refs.c: constify the sha arguments for ref_transaction_create|delete|update
ref_transaction_create|delete|update has no need to modify the sha1
arguments passed to it so it should use const unsigned char* instead
of unsigned char*.
Some functions, such as fast_forward_to(), already have its old/new
sha1 arguments as consts. This function will at some point need to
use ref_transaction_update() in which case this change is required.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 7 ---
refs.h | 7 ---
2 files changed, 8 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 33541f4..a767ef6 100644
--- a/refs.c
+++ b/refs.c
@@ -,7 +,8 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3347,7 +3348,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags)
{
struct ref_update *update = add_update(transaction, refname);
@@ -3361,7 +3362,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old)
{
struct ref_update *update = add_update(transaction, refname);
diff --git a/refs.h b/refs.h
index 306d833..b893838 100644
--- a/refs.h
+++ b/refs.h
@@ -237,7 +237,8 @@ struct ref_transaction *ref_transaction_begin(void);
*/
void ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1, unsigned char *old_sha1,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
@@ -248,7 +249,7 @@ void ref_transaction_update(struct ref_transaction
*transaction,
*/
void ref_transaction_create(struct ref_transaction *transaction,
const char *refname,
- unsigned char *new_sha1,
+ const unsigned char *new_sha1,
int flags);
/*
@@ -258,7 +259,7 @@ void ref_transaction_create(struct ref_transaction
*transaction,
*/
void ref_transaction_delete(struct ref_transaction *transaction,
const char *refname,
- unsigned char *old_sha1,
+ const unsigned char *old_sha1,
int flags, int have_old);
/*
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 02/41] refs.c: ref_transaction_commit should not free the transaction
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 1 +
refs.c | 1 -
refs.h | 5 ++---
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 405267f..1fd7a89 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -369,6 +369,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
update_refs_stdin();
ret = ref_transaction_commit(transaction, msg,
UPDATE_REFS_DIE_ON_ERR);
+ ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.c b/refs.c
index 48573e3..33541f4 100644
--- a/refs.c
+++ b/refs.c
@@ -3483,7 +3483,6 @@ cleanup:
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
free(delnames);
- ref_transaction_free(transaction);
return ret;
}
diff --git a/refs.h b/refs.h
index a07a5d0..306d833 100644
--- a/refs.h
+++ b/refs.h
@@ -216,8 +216,7 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or freed by
- * calling ref_transaction_free().
+ * be freed by calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
@@ -265,7 +264,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
/*
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
- * problem. The ref_transaction is freed by this function.
+ * problem.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 19/41] commit.c: use ref transactions for updates
Change commit.c to use ref transactions for all ref updates.
Make sure we pass a NULL pointer to ref_transaction_update if have_old
is false.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/commit.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 99c2044..14cd9f4 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1613,11 +1613,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
const char *index_file, *reflog_msg;
char *nl;
unsigned char sha1[20];
- struct ref_lock *ref_lock;
struct commit_list *parents = NULL, **pptr = &parents;
struct stat statbuf;
struct commit *current_head = NULL;
struct commit_extra_header *extra = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_commit_usage,
builtin_commit_options);
@@ -1739,16 +1740,6 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_release(&author_ident);
free_commit_extra_headers(extra);
- ref_lock = lock_any_ref_for_update("HEAD",
- !current_head
- ? NULL
- : current_head->object.sha1,
- 0, NULL);
- if (!ref_lock) {
- rollback_index_files();
- die(_("cannot lock HEAD ref"));
- }
-
nl = strchr(sb.buf, '\n');
if (nl)
strbuf_setlen(&sb, nl + 1 - sb.buf);
@@ -1757,10 +1748,17 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
- if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", sha1,
+ current_head ?
+ current_head->object.sha1 : NULL,
+ 0, !!current_head, &err) ||
+ ref_transaction_commit(transaction, sb.buf, &err)) {
rollback_index_files();
- die(_("cannot update HEAD ref"));
+ die("%s", err.buf);
}
+ ref_transaction_free(transaction);
unlink(git_path("CHERRY_PICK_HEAD"));
unlink(git_path("REVERT_HEAD"));
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 18/41] replace.c: use the ref transaction functions for updates
Update replace.c to use ref transactions for updates.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/replace.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/replace.c b/builtin/replace.c
index 4b3705d..cf92e5d 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -154,7 +154,8 @@ static int replace_object_sha1(const char *object_ref,
unsigned char prev[20];
enum object_type obj_type, repl_type;
char ref[PATH_MAX];
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
obj_type = sha1_object_info(object, NULL);
repl_type = sha1_object_info(repl, NULL);
@@ -167,12 +168,14 @@ static int replace_object_sha1(const char *object_ref,
check_ref_valid(object, prev, ref, sizeof(ref), force);
- lock = lock_any_ref_for_update(ref, prev, 0, NULL);
- if (!lock)
- die("%s: cannot lock the ref", ref);
- if (write_ref_sha1(lock, repl, NULL) < 0)
- die("%s: cannot update the ref", ref);
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref, repl, prev,
+ 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_commit(transaction, NULL, &err))
+ die("%s", err.buf);
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 01/41] refs.c: remove ref_transaction_rollback
We do not yet need both a rollback and a free function for transactions.
Remove ref_transaction_rollback and use ref_transaction_free instead.
At a later stage we may reintroduce a rollback function if we want to start
adding reusable transactions and similar.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 7 +--
refs.h | 16 +++-
2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index 6898263..48573e3 100644
--- a/refs.c
+++ b/refs.c
@@ -3308,7 +3308,7 @@ struct ref_transaction *ref_transaction_begin(void)
return xcalloc(1, sizeof(struct ref_transaction));
}
-static void ref_transaction_free(struct ref_transaction *transaction)
+void ref_transaction_free(struct ref_transaction *transaction)
{
int i;
@@ -3319,11 +3319,6 @@ static void ref_transaction_free(struct ref_transaction
*transaction)
free(transaction);
}
-void ref_transaction_rollback(struct ref_transaction *transaction)
-{
- ref_transaction_free(transaction);
-}
-
static struct ref_update *add_update(struct ref_transaction *transaction,
const char *refname)
{
diff --git a/refs.h b/refs.h
index 09ff483..a07a5d0 100644
--- a/refs.h
+++ b/refs.h
@@ -216,18 +216,12 @@ enum action_on_err {
/*
* Begin a reference transaction. The reference transaction must
- * eventually be commited using ref_transaction_commit() or rolled
- * back using ref_transaction_rollback().
+ * eventually be commited using ref_transaction_commit() or freed by
+ * calling ref_transaction_free().
*/
struct ref_transaction *ref_transaction_begin(void);
/*
- * Roll back a ref_transaction and free all associated data.
- */
-void ref_transaction_rollback(struct ref_transaction *transaction);
-
-
-/*
* The following functions add a reference check or update to a
* ref_transaction. In all of them, refname is the name of the
* reference to be affected. The functions make internal copies of
@@ -235,7 +229,6 @@ void ref_transaction_rollback(struct ref_transaction
*transaction);
* can be REF_NODEREF; it is passed to update_ref_lock().
*/
-
/*
* Add a reference update to transaction. new_sha1 is the value that
* the reference should have after the update, or zeros if it should
@@ -277,6 +270,11 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
int ref_transaction_commit(struct ref_transaction *transaction,
const char *msg, enum action_on_err onerr);
+/*
+ * Free an existing transaction and all associated data.
+ */
+void ref_transaction_free(struct ref_transaction *transaction);
+
/** Lock a ref and then write its file */
int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 11/41] refs.c: remove the onerr argument to ref_transaction_commit
Since all callers now use QUIET_ON_ERR we no longer need to provide an onerr
argument any more. Remove the onerr argument from the ref_transaction_commit
signature.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 3 +--
refs.c | 22 +++---
refs.h | 3 +--
3 files changed, 9 insertions(+), 19 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index aec9004..88ab785 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -366,8 +366,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- if (ref_transaction_commit(transaction, msg, &err,
- UPDATE_REFS_QUIET_ON_ERR))
+ if (ref_transaction_commit(transaction, msg, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
return 0;
diff --git a/refs.c b/refs.c
index 6696082..fb44978 100644
--- a/refs.c
+++ b/refs.c
@@ -3481,8 +3481,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
- struct strbuf *err,
- enum action_on_err onerr)
+ struct strbuf *err)
{
int i;
for (i = 1; i < n; i++)
@@ -3492,22 +3491,13 @@ static int ref_update_reject_duplicates(struct
ref_update **updates, int n,
if (err)
strbuf_addf(err, str, updates[i]->refname);
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR:
- error(str, updates[i]->refname); break;
- case UPDATE_REFS_DIE_ON_ERR:
- die(str, updates[i]->refname); break;
- case UPDATE_REFS_QUIET_ON_ERR:
- break;
- }
return 1;
}
return 0;
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr)
+ const char *msg, struct strbuf *err)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3522,7 +3512,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err);
if (ret)
goto cleanup;
@@ -3534,7 +3524,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
(update->have_old ?
update->old_sha1 : NULL),
update->flags,
- &update->type, onerr);
+ &update->type,
+ UPDATE_REFS_QUIET_ON_ERR);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
@@ -3552,7 +3543,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- update->lock, err, onerr);
+ update->lock, err,
+ UPDATE_REFS_QUIET_ON_ERR);
update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
diff --git a/refs.h b/refs.h
index 94d4cd4..6ee9c9e 100644
--- a/refs.h
+++ b/refs.h
@@ -270,8 +270,7 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, struct strbuf *err,
- enum action_on_err onerr);
+ const char *msg, struct strbuf *err);
/*
* Free an existing transaction and all associated data.
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 13/41] refs.c: change ref_transaction_create to do error checking and return status
Do basic error checking in ref_transaction_create() and make it return
non-zero on error. Update all callers to check the result of
ref_transaction_create(). There are currently no conditions in _create that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 4 +++-
refs.c | 18 +++--
refs.h | 55 +---
3 files changed, 63 insertions(+), 14 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 3067b11..41121fa 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("create %s: extra input: %s", refname, next);
- ref_transaction_create(transaction, refname, new_sha1, update_flags);
+ if (ref_transaction_create(transaction, refname, new_sha1,
+ update_flags, &err))
+ die("%s", err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index f5e7a12..3cb99f6 100644
--- a/refs.c
+++ b/refs.c
@@ -3439,18 +3439,24 @@ int ref_transaction_update(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_create(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- int flags)
+int ref_transaction_create(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ int flags,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (!new_sha1 || is_null_sha1(new_sha1))
+ die("BUG: create ref with null new_sha1");
+
+ update = add_update(transaction, refname);
- assert(!is_null_sha1(new_sha1));
hashcpy(update->new_sha1, new_sha1);
hashclr(update->old_sha1);
update->flags = flags;
update->have_old = 1;
+ return 0;
}
void ref_transaction_delete(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 32edf3f..bda790e6 100644
--- a/refs.h
+++ b/refs.h
@@ -10,6 +10,45 @@ struct ref_lock {
int force_write;
};
+/*
+ * A ref_transaction represents a collection of ref updates
+ * that should succeed or fail together.
+ *
+ * Calling sequence
+ *
+ * - Allocate and initialize a `struct ref_transaction` by calling
+ * `ref_transaction_begin()`.
+ *
+ * - List intended ref updates by calling functions like
+ * `ref_transaction_update()` and `ref_transaction_create()`.
+ *
+ * - Call `ref_transaction_commit()` to execute the transaction.
+ * If this succeeds, the ref updates will have taken place and
+ * the transaction cannot be rolled back.
+ *
+ * - At any time call `ref_transaction_free()` to discard the
+ * transaction and free associated resources. In particular,
+ * this rolls back the transaction if it has not been
+ * successfully committed.
+ *
+ * Error handling
+ * --
+ *
+ * On error, transaction functions append a message about what
+ * went wrong to the 'err' argument. The message mentions what
+ * ref was being updated (if any) when the error occurred so it
+ * can be passed to 'die' or 'error' as-is.
+ *
+ * The message is appended to err without first clearing err.
+ * This allows the caller to prepare preamble text to the generated
+ * error message:
+ *
+ * strbuf_addf(&err, "Error while doing foo-bar: ");
+ * if (ref_transaction_update(..., &err)) {
+ * ret = error("%s", err.buf);
+ * goto cleanup;
+ * }
+ */
struct ref_transaction;
/*
@@ -236,7 +275,7 @@ struct ref_transaction *ref_transaction_begin(void);
* it must not have existed beforehand.
* Function returns 0 on success and non-zero on failure. A failure to update
* means that the transaction as a whole has failed and will need to be
- * rolled back. On failure the err buffer will be updated.
+ * rolled back.
*/
int ref_transaction_update(struct ref_transaction *transaction,
const char *refname,
@@ -250,11 +289,15 @@ int ref_transaction_update(struct ref_transaction
*transaction,
* that the reference should have after the update; it must not be the
* null SHA-1. It is verified that the reference does not exist
* already.
+ * Function returns 0 on success and non-zero on failure. A failure to create
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_create(struct ref_transaction *transaction,
- co
[PATCH v13 28/41] refs.c: remove the update_ref_lock function
Since we now only call update_ref_lock with onerr==QUIET_ON_ERR we no longer
need this function and can replace it with just calling lock_any_ref_for_update
directly.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 30 ++
1 file changed, 6 insertions(+), 24 deletions(-)
diff --git a/refs.c b/refs.c
index d2fd419..3796957 100644
--- a/refs.c
+++ b/refs.c
@@ -3323,24 +3323,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static struct ref_lock *update_ref_lock(const char *refname,
- const unsigned char *oldval,
- int flags, int *type_p,
- enum action_on_err onerr)
-{
- struct ref_lock *lock;
- lock = lock_any_ref_for_update(refname, oldval, flags, type_p);
- if (!lock) {
- const char *str = "Cannot lock the ref '%s'.";
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- }
- return lock;
-}
-
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
struct strbuf *err, enum action_on_err onerr)
@@ -3590,12 +3572,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = update_ref_lock(update->refname,
- (update->have_old ?
- update->old_sha1 : NULL),
- update->flags,
- &update->type,
- UPDATE_REFS_QUIET_ON_ERR);
+ update->lock = lock_any_ref_for_update(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 20/41] sequencer.c: use ref transactions for all ref updates
Change to use ref transactions for all updates to refs.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
sequencer.c | 24
1 file changed, 16 insertions(+), 8 deletions(-)
diff --git a/sequencer.c b/sequencer.c
index 0a80c58..fd8acaf 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -272,23 +272,31 @@ static int error_dirty_index(struct replay_opts *opts)
static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
{
- struct ref_lock *ref_lock;
+ struct ref_transaction *transaction;
struct strbuf sb = STRBUF_INIT;
- int ret;
+ struct strbuf err = STRBUF_INIT;
read_cache();
if (checkout_fast_forward(from, to, 1))
exit(1); /* the callee should have complained already */
- ref_lock = lock_any_ref_for_update("HEAD", unborn ? null_sha1 : from,
- 0, NULL);
- if (!ref_lock)
- return error(_("Failed to lock HEAD during fast_forward_to"));
strbuf_addf(&sb, "%s: fast-forward", action_name(opts));
- ret = write_ref_sha1(ref_lock, to, sb.buf);
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, "HEAD", to, from,
+ 0, !unborn, &err) ||
+ ref_transaction_commit(transaction, sb.buf, &err)) {
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&sb);
+ strbuf_release(&err);
+ return -1;
+ }
strbuf_release(&sb);
- return ret;
+ ref_transaction_free(transaction);
+ return 0;
}
static int do_recursive_merge(struct commit *base, struct commit *next,
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 21/41] fast-import.c: change update_branch to use ref transactions
Change update_branch() to use ref transactions for updates.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
fast-import.c | 25 +++--
1 file changed, 15 insertions(+), 10 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 6707a66..4a7b196 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1679,39 +1679,44 @@ found_entry:
static int update_branch(struct branch *b)
{
static const char *msg = "fast-import";
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
unsigned char old_sha1[20];
+ struct strbuf err = STRBUF_INIT;
if (read_ref(b->name, old_sha1))
hashclr(old_sha1);
+
if (is_null_sha1(b->sha1)) {
if (b->delete)
delete_ref(b->name, old_sha1, 0);
return 0;
}
- lock = lock_any_ref_for_update(b->name, old_sha1, 0, NULL);
- if (!lock)
- return error("Unable to lock %s", b->name);
if (!force_update && !is_null_sha1(old_sha1)) {
struct commit *old_cmit, *new_cmit;
old_cmit = lookup_commit_reference_gently(old_sha1, 0);
new_cmit = lookup_commit_reference_gently(b->sha1, 0);
- if (!old_cmit || !new_cmit) {
- unlock_ref(lock);
+ if (!old_cmit || !new_cmit)
return error("Branch %s is missing commits.", b->name);
- }
if (!in_merge_bases(old_cmit, new_cmit)) {
- unlock_ref(lock);
warning("Not updating %s"
" (new tip %s does not contain %s)",
b->name, sha1_to_hex(b->sha1),
sha1_to_hex(old_sha1));
return -1;
}
}
- if (write_ref_sha1(lock, b->sha1, msg) < 0)
- return error("Unable to update %s", b->name);
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, b->name, b->sha1, old_sha1,
+ 0, 1, &err) ||
+ ref_transaction_commit(transaction, msg, &err)) {
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return -1;
+ }
+ ref_transaction_free(transaction);
return 0;
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 37/41] refs.c: call lock_ref_sha1_basic directly from commit
Skip using the lock_any_ref_for_update wrapper and call lock_ref_sha1_basic
directly from the commit function.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 12 ++--
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 22fb166..f13c0be 100644
--- a/refs.c
+++ b/refs.c
@@ -3567,12 +3567,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
for (i = 0; i < n; i++) {
struct ref_update *update = updates[i];
- update->lock = lock_any_ref_for_update(update->refname,
- (update->have_old ?
- update->old_sha1 :
- NULL),
- update->flags,
- &update->type);
+ update->lock = lock_ref_sha1_basic(update->refname,
+ (update->have_old ?
+ update->old_sha1 :
+ NULL),
+ update->flags,
+ &update->type);
if (!update->lock) {
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 26/41] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring
to use ref transactions we also fix a potential memory leak where in the
original code if write_ref_sha1() would fail we would end up returning from
the function without free()ing the msg string.
Note that this function is only called when fetching from a remote HTTP
repository onto the local (most of the time single-user) repository which
likely means that the type of collissions that the previous locking would
protect against and cause the fetch to fail for to be even more rare.
Signed-off-by: Ronnie Sahlberg
---
walker.c | 59 +++
1 file changed, 35 insertions(+), 24 deletions(-)
diff --git a/walker.c b/walker.c
index 1dd86b8..60d9f9e 100644
--- a/walker.c
+++ b/walker.c
@@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target,
const char **write_ref)
int walker_fetch(struct walker *walker, int targets, char **target,
const char **write_ref, const char *write_ref_log_details)
{
- struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *));
+ struct strbuf ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction = NULL;
unsigned char *sha1 = xmalloc(targets * 20);
- char *msg;
- int ret;
+ char *msg = NULL;
int i;
save_commit_buffer = 0;
- for (i = 0; i < targets; i++) {
- if (!write_ref || !write_ref[i])
- continue;
-
- lock[i] = lock_ref_sha1(write_ref[i], NULL);
- if (!lock[i]) {
- error("Can't lock ref %s", write_ref[i]);
- goto unlock_and_fail;
+ if (write_ref) {
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
}
}
-
if (!walker->get_recover)
for_each_ref(mark_complete, NULL);
for (i = 0; i < targets; i++) {
if (interpret_target(walker, target[i], &sha1[20 * i])) {
error("Could not interpret response from server '%s' as
something to pull", target[i]);
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (process(walker, lookup_unknown_object(&sha1[20 * i])))
- goto unlock_and_fail;
+ goto rollback_and_fail;
}
if (loop(walker))
- goto unlock_and_fail;
+ goto rollback_and_fail;
if (write_ref_log_details) {
msg = xmalloc(strlen(write_ref_log_details) + 12);
@@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char
**target,
for (i = 0; i < targets; i++) {
if (!write_ref || !write_ref[i])
continue;
- ret = write_ref_sha1(lock[i], &sha1[20 * i], msg ? msg : "fetch
(unknown)");
- lock[i] = NULL;
- if (ret)
- goto unlock_and_fail;
+ strbuf_reset(&ref_name);
+ strbuf_addf(&ref_name, "refs/%s", write_ref[i]);
+ if (ref_transaction_update(transaction, ref_name.buf,
+ &sha1[20 * i], NULL, 0, 0,
+ &err)) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
+ }
+ }
+ if (write_ref) {
+ if (ref_transaction_commit(transaction,
+ msg ? msg : "fetch (unknown)",
+ &err)) {
+ error("%s", err.buf);
+ goto rollback_and_fail;
+ }
+ ref_transaction_free(transaction);
}
- free(msg);
+ free(msg);
return 0;
-unlock_and_fail:
- for (i = 0; i < targets; i++)
- if (lock[i])
- unlock_ref(lock[i]);
+rollback_and_fail:
+ ref_transaction_free(transaction);
+ free(msg);
+ strbuf_release(&err);
+ strbuf_release(&ref_name);
return -1;
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 31/41] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also
need to add a new flag REF_ISPRUNING that will tell the transaction that we
do not want to delete this ref from the packed refs. This flag is private to
refs.c and not exposed to external callers.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 28 +---
refs.h | 11 ++-
2 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index bc750f4..e19041a 100644
--- a/refs.c
+++ b/refs.c
@@ -30,6 +30,12 @@ static inline int bad_ref_char(int ch)
}
/*
+ * Used as a flag to ref_transaction_delete when a loose ref is being
+ * pruned.
+ */
+#define REF_ISPRUNING 0x0100
+
+/*
* Try to read one refname component from the front of refname. Return
* the length of the component found, or -1 if the component is not
* legal.
@@ -2347,17 +2353,24 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
if (check_refname_format(r->name + 5, 0))
return;
- lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
- if (lock) {
- unlink_or_warn(git_path("%s", r->name));
- unlock_ref(lock);
- try_remove_empty_parents(r->name);
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, r->name, r->sha1,
+ REF_ISPRUNING, 1, &err) ||
+ ref_transaction_commit(transaction, NULL, &err)) {
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return;
}
+ ref_transaction_free(transaction);
+ try_remove_empty_parents(r->name);
}
static void prune_refs(struct ref_to_prune *r)
@@ -3586,9 +3599,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->lock) {
- delnames[delnum++] = update->lock->ref_name;
ret |= delete_ref_loose(update->lock, update->type,
err);
+ if (!(update->flags & REF_ISPRUNING))
+ delnames[delnum++] = update->lock->ref_name;
}
}
diff --git a/refs.h b/refs.h
index c38ee09..dee7c8f 100644
--- a/refs.h
+++ b/refs.h
@@ -171,8 +171,17 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks any ref (for 'HEAD' type refs). */
+/*
+ * Flags controlling lock_any_ref_for_update(), ref_transaction_update(),
+ * ref_transaction_create(), etc.
+ * REF_NODEREF: act on the ref directly, instead of dereferencing
+ * symbolic references.
+ *
+ * Flags >= 0x100 are reserved for internal use.
+ */
#define REF_NODEREF0x01
+
+/** Locks any ref (for 'HEAD' type refs). */
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 35/41] refs.c: pack all refs before we start to rename a ref
This means that most loose refs will no longer be present after the rename
which triggered a test failure since it assumes the file for an unrelated
ref would still be present after the rename. This also makes the rename
itself slightly slower, but as it now results in all refs being packed future
commands accessing refs might become slightly faster.
We want to do this to make it easier to handle atomic renames in rename_ref for
the case 'git branch -m foo/bar foo'. If we can guarantee that foo/bar does not
exist as a loose ref we can avoid locking foo/bar.lock during transaction
commit and thus make it possible to delete the foo directory and re-create
it as a file(branch) in a single transaction.
There is a small race between when we pack all refs and we eventually remove
the old ref from the packed refs file. If a concurrent process deletes the old
ref from the packed refs file, then creates a new ref with that name and
finally repacks the packed refs file, then our transaction will delete the new
ref without realizing it had changed during the transaction. The window for this
is very short and is still a slightly less race than the ones in the old
code for ref_rename.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c| 3 +++
t/t3200-branch.sh | 6 +++---
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index 8848dbf..03cff59 100644
--- a/refs.c
+++ b/refs.c
@@ -2652,6 +2652,9 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
return error("unable to move logfile logs/%s to
"TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
+ if (pack_refs(PACK_REFS_ALL | PACK_REFS_PRUNE))
+ return error("unable to pack refs");
+
if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
error("unable to delete old %s", oldrefname);
goto rollback;
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index ac31b71..de0c2b9 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -288,9 +288,9 @@ test_expect_success 'deleting a dangling symref' '
test_expect_success 'renaming a symref is not allowed' '
git symbolic-ref refs/heads/master2 refs/heads/master &&
test_must_fail git branch -m master2 master3 &&
- git symbolic-ref refs/heads/master2 &&
- test_path_is_file .git/refs/heads/master &&
- test_path_is_missing .git/refs/heads/master3
+ git rev-parse --verify refs/heads/master &&
+ test_must_fail git symbolic-ref refs/heads/master3 &&
+ test_must_fail git rev-parse refs/heads/master3
'
test_expect_success SYMLINKS 'git branch -m u v should fail when the reflog
for u is a symlink' '
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 34/41] refs.c: pass NULL as *flags to read_ref_full
We call read_ref_full with a pointer to flags from rename_ref but since
we never actually use the returned flags we can just pass NULL here instead.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 88d9351..8848dbf 100644
--- a/refs.c
+++ b/refs.c
@@ -2657,7 +2657,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
goto rollback;
}
- if (!read_ref_full(newrefname, sha1, 1, &flag) &&
+ if (!read_ref_full(newrefname, sha1, 1, NULL) &&
delete_ref(newrefname, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s",
newrefname))) {
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 40/41] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update.
Signed-off-by: Ronnie Sahlberg
---
builtin/fetch.c | 33 +++--
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index faa1233..52f1ebc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -375,23 +375,36 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv("GIT_REFLOG_ACTION");
- static struct ref_lock *lock;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
if (!rla)
rla = default_rla.buf;
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- lock = lock_any_ref_for_update(ref->name,
- check_old ? ref->old_sha1 : NULL,
- 0, NULL);
- if (!lock)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
- if (write_ref_sha1(lock, ref->new_sha1, msg) < 0)
- return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT :
- STORE_REF_ERROR_OTHER;
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, msg, &err))
+ goto fail;
+
+ ret = ref_transaction_commit(transaction, &err);
+ if (ret == UPDATE_REFS_NAME_CONFLICT)
+ df_conflict = 1;
+ if (ret)
+ goto fail;
+
+ ref_transaction_free(transaction);
return 0;
+fail:
+ ref_transaction_free(transaction);
+ error("%s", err.buf);
+ strbuf_release(&err);
+ return df_conflict ? STORE_REF_ERROR_DF_CONFLICT
+ : STORE_REF_ERROR_OTHER;
}
#define REFCOL_WIDTH 10
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 15/41] refs.c: make ref_transaction_begin take an err argument
Add an err argument to _begin so that on non-fatal failures in future ref
backends we can report a nice error back to the caller.
While _begin can currently never fail for other reasons than OOM, in which
case we die() anyway, we may add other types of backends in the future.
For example, a hypothetical MySQL backend could fail in _being with
"Can not connect to MySQL server. No route to host".
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 2 +-
refs.c | 2 +-
refs.h | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7c9c248..c6ad0be 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -365,7 +365,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
die("Refusing to perform update with empty message.");
if (read_stdin) {
- transaction = ref_transaction_begin();
+ transaction = ref_transaction_begin(&err);
if (delete || no_deref || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
diff --git a/refs.c b/refs.c
index d9b99c7..ee87eda 100644
--- a/refs.c
+++ b/refs.c
@@ -3387,7 +3387,7 @@ struct ref_transaction {
size_t nr;
};
-struct ref_transaction *ref_transaction_begin(void)
+struct ref_transaction *ref_transaction_begin(struct strbuf *err)
{
return xcalloc(1, sizeof(struct ref_transaction));
}
diff --git a/refs.h b/refs.h
index 58ba365..743358f 100644
--- a/refs.h
+++ b/refs.h
@@ -257,7 +257,7 @@ enum action_on_err {
* Begin a reference transaction. The reference transaction must
* be freed by calling ref_transaction_free().
*/
-struct ref_transaction *ref_transaction_begin(void);
+struct ref_transaction *ref_transaction_begin(struct strbuf *err);
/*
* The following functions add a reference check or update to a
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 00/41] Use ref transactions
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions This patch series is based on next and expands on the transaction API. It converts all ref updates, inside refs.c as well as external, to use the transaction API for updates. This makes most of the ref updates to become atomic when there are failures locking or writing to a ref. This version completes the work to convert all ref updates to use transactions. Now that all updates are through transactions I will start working on cleaning up the reading of refs and to create an api for managing reflogs but all that will go in a different patch series. Version 13: - This version should cover all of JNs suggestions on the previous series. If I missed anything I appologize. Ronnie Sahlberg (41): refs.c: remove ref_transaction_rollback refs.c: ref_transaction_commit should not free the transaction refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: add an err argument to repack_without_refs refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: add an err argument to delete_ref_loose refs.c: make update_ref_write update a strbuf on failure update-ref: use err argument to get error from ref_transaction_commit refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status refs.c: change ref_transaction_create to do error checking and return status refs.c: update ref_transaction_delete to check for error and return status refs.c: make ref_transaction_begin take an err argument refs.c: add transaction.status and track OPEN/CLOSED/ERROR tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make lock_ref_sha1 static refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full refs.c: pack all refs before we start to rename a ref refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: pass a skip list to name_conflict_fn refs.c: propagate any errno==ENOTDIR from _commit back to the callers fetch.c: change s_update_ref to use a ref transaction refs.c: make write_ref_sha1 static branch.c | 30 +-- builtin/commit.c | 24 ++- builtin/fetch.c| 36 ++-- builtin/receive-pack.c | 32 +++- builtin/replace.c | 15 +- builtin/tag.c | 15 +- builtin/update-ref.c | 34 ++-- cache.h| 2 + fast-import.c | 54 -- lockfile.c | 37 ++-- refs.c | 503 - refs.h | 118 +--- sequencer.c| 24 ++- t/t3200-branch.sh | 6 +- walker.c | 58 +++--- 15 files changed, 647 insertions(+), 341 deletions(-) -- 2.0.0.567.g64a7adf -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 05/41] refs.c: add a strbuf argument to ref_transaction_commit for error logging
Add a strbuf argument to _commit so that we can pass an error string back to
the caller. So that we can do error logging from the caller instead of from
_commit.
Longer term plan is to first convert all callers to use onerr==QUIET_ON_ERR
and craft any log messages from the callers themselves and finally remove the
onerr argument completely.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 2 +-
refs.c | 6 +-
refs.h | 5 -
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 1fd7a89..22617af 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -367,7 +367,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg,
+ ret = ref_transaction_commit(transaction, msg, NULL,
UPDATE_REFS_DIE_ON_ERR);
ref_transaction_free(transaction);
return ret;
diff --git a/refs.c b/refs.c
index 0faed29..25c3a93 100644
--- a/refs.c
+++ b/refs.c
@@ -3418,7 +3418,8 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
}
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr)
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr)
{
int ret = 0, delnum = 0, i;
const char **delnames;
@@ -3447,6 +3448,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
update->flags,
&update->type, onerr);
if (!update->lock) {
+ if (err)
+ strbuf_addf(err, "Cannot lock the ref '%s'.",
+ update->refname);
ret = 1;
goto cleanup;
}
diff --git a/refs.h b/refs.h
index b893838..94d4cd4 100644
--- a/refs.h
+++ b/refs.h
@@ -266,9 +266,12 @@ void ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If err is non-NULL we will add an error string to it to explain why
+ * the transaction failed. The string does not end in newline.
*/
int ref_transaction_commit(struct ref_transaction *transaction,
- const char *msg, enum action_on_err onerr);
+ const char *msg, struct strbuf *err,
+ enum action_on_err onerr);
/*
* Free an existing transaction and all associated data.
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 06/41] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there
is a failure. Pass the err variable from ref_transaction_commit to this
function so that callers can print a meaningful error message if _commit
fails due to a problem in repack_without_refs.
Add a new function unable_to_lock_message that takes a strbuf argument and
fills in the reason for the failure.
In commit_packed_refs, make sure that we propagate any errno that
commit_lock_file might have set back to our caller.
Make sure both commit_packed_refs and lock_file has errno set to a meaningful
value on error. Update a whole bunch of other places to keep errno from
beeing clobbered.
Signed-off-by: Ronnie Sahlberg
---
cache.h| 2 ++
lockfile.c | 37 +
refs.c | 111 ++---
3 files changed, 110 insertions(+), 40 deletions(-)
diff --git a/cache.h b/cache.h
index cbe1935..8b12aa8 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,8 @@ struct lock_file {
#define LOCK_DIE_ON_ERROR 1
#define LOCK_NODEREF 2
extern int unable_to_lock_error(const char *path, int err);
+extern void unable_to_lock_message(const char *path, int err,
+ struct strbuf *buf);
extern NORETURN void unable_to_lock_index_die(const char *path, int err);
extern int hold_lock_file_for_update(struct lock_file *, const char *path,
int);
extern int hold_lock_file_for_append(struct lock_file *, const char *path,
int);
diff --git a/lockfile.c b/lockfile.c
index 8fbcb6a..f5da18c 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
return p;
}
-
+/* Make sure errno contains a meaningful value on error */
static int lock_file(struct lock_file *lk, const char *path, int flags)
{
/*
@@ -130,8 +130,10 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
*/
static const size_t max_path_len = sizeof(lk->filename) - 5;
- if (strlen(path) >= max_path_len)
+ if (strlen(path) >= max_path_len) {
+ errno = ENAMETOOLONG;
return -1;
+ }
strcpy(lk->filename, path);
if (!(flags & LOCK_NODEREF))
resolve_symlink(lk->filename, max_path_len);
@@ -148,42 +150,49 @@ static int lock_file(struct lock_file *lk, const char
*path, int flags)
lock_file_list = lk;
lk->on_list = 1;
}
- if (adjust_shared_perm(lk->filename))
- return error("cannot fix permission bits on %s",
-lk->filename);
+ if (adjust_shared_perm(lk->filename)) {
+ int save_errno = errno;
+ error("cannot fix permission bits on %s",
+ lk->filename);
+ errno = save_errno;
+ return -1;
+ }
}
else
lk->filename[0] = 0;
return lk->fd;
}
-static char *unable_to_lock_message(const char *path, int err)
+void unable_to_lock_message(const char *path, int err, struct strbuf *buf)
{
- struct strbuf buf = STRBUF_INIT;
if (err == EEXIST) {
- strbuf_addf(&buf, "Unable to create '%s.lock': %s.\n\n"
+ strbuf_addf(buf, "Unable to create '%s.lock': %s.\n\n"
"If no other git process is currently running, this
probably means a\n"
"git process crashed in this repository earlier. Make sure
no other git\n"
"process is running and remove the file manually to
continue.",
absolute_path(path), strerror(err));
} else
- strbuf_addf(&buf, "Unable to create '%s.lock': %s",
+ strbuf_addf(buf, "Unable to create '%s.lock': %s",
absolute_path(path), strerror(err));
- return strbuf_detach(&buf, NULL);
}
int unable_to_lock_error(const char *path, int err)
{
- char *msg = unable_to_lock_message(path, err);
- error("%s", msg);
- free(msg);
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_message(path, err, &buf);
+ error("%s", buf.buf);
+ strbuf_release(&buf);
return -1;
}
NORETURN void unable_to_lock_index_die(const char *path, int err)
{
- die("%s", unable_to_lock_message(path, err));
+ struct strbuf buf = STRBUF_INIT;
+
+ unable_to_lock_message(path, err, &buf);
+ die("%s", buf.buf);
}
int hold_lock_file_for_update(struct lock_file *lk, const char *path, int
flags)
diff --git a/refs.c b/refs.c
index 25c3a93..7ec1f32 100644
--- a/refs.c
+++ b/refs.c
@@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname,
unsigned char *sha1, int rea
if (flag)
*flag = 0;
- if (check_refname_format(refname, REFNA
[PATCH v13 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message
through to the create/delete/update function instead of the commit message.
This allows for individual messages for each change in a multi ref
transaction.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
branch.c | 4 ++--
builtin/commit.c | 4 ++--
builtin/fetch.c| 3 +--
builtin/receive-pack.c | 7 ---
builtin/replace.c | 4 ++--
builtin/tag.c | 4 ++--
builtin/update-ref.c | 13 +++--
fast-import.c | 8
refs.c | 34 +-
refs.h | 8
sequencer.c| 4 ++--
walker.c | 5 ++---
12 files changed, 53 insertions(+), 45 deletions(-)
diff --git a/branch.c b/branch.c
index c1eae00..e0439af 100644
--- a/branch.c
+++ b/branch.c
@@ -301,8 +301,8 @@ void create_branch(const char *head,
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, &err) ||
- ref_transaction_commit(transaction, msg, &err))
+ null_sha1, 0, !forcing, msg, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 14cd9f4..e01b333 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1753,8 +1753,8 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
ref_transaction_update(transaction, "HEAD", sha1,
current_head ?
current_head->object.sha1 : NULL,
- 0, !!current_head, &err) ||
- ref_transaction_commit(transaction, sb.buf, &err)) {
+ 0, !!current_head, sb.buf, &err) ||
+ ref_transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
}
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..faa1233 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const
char *remote_name,
}
}
}
-
if (rc & STORE_REF_ERROR_DF_CONFLICT)
error(_("some local refs could not be updated; try running\n"
- " 'git remote prune %s' to remove any old, conflicting "
+ "'git remote prune %s' to remove any old, conflicting "
"branches"), remote_name);
abort:
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 13f4a63..5653fa2 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -586,10 +586,11 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, namespaced_name,
- new_sha1, old_sha1, 0, 1, &err) ||
- ref_transaction_commit(transaction, "push", &err)) {
-
+ new_sha1, old_sha1, 0, 1, "push",
+ &err) ||
+ ref_transaction_commit(transaction, &err)) {
const char *str;
+
string_list_append(&error_strings, err.buf);
str = error_strings.items[error_strings.nr - 1].string;
strbuf_release(&err);
diff --git a/builtin/replace.c b/builtin/replace.c
index cf92e5d..c42b26e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -171,8 +171,8 @@ static int replace_object_sha1(const char *object_ref,
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref, repl, prev,
- 0, !is_null_sha1(prev), &err) ||
- ref_transaction_commit(transaction, NULL, &err))
+ 0, !is_null_sha1(prev), NULL, &err) ||
+ ref_transaction_commit(transaction, &err))
die("%s", err.buf);
ref_transaction_free(transaction);
diff --git a/builtin/tag.c b/builtin/tag.c
index c9bfc9a..74af63e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, object, prev,
- 0, !is_null_sha1(prev), &err) ||
- ref_transaction_commit(tran
[PATCH v13 10/41] update-ref: use err argument to get error from ref_transaction_commit
Call ref_transaction_commit with QUIET_ON_ERR and use the strbuf that is
returned to print a log message if/after the transaction fails.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 22617af..aec9004 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -342,6 +342,7 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
@@ -359,18 +360,17 @@ int cmd_update_ref(int argc, const char **argv, const
char *prefix)
die("Refusing to perform update with empty message.");
if (read_stdin) {
- int ret;
transaction = ref_transaction_begin();
-
if (delete || no_deref || argc > 0)
usage_with_options(git_update_ref_usage, options);
if (end_null)
line_termination = '\0';
update_refs_stdin();
- ret = ref_transaction_commit(transaction, msg, NULL,
-UPDATE_REFS_DIE_ON_ERR);
+ if (ref_transaction_commit(transaction, msg, &err,
+ UPDATE_REFS_QUIET_ON_ERR))
+ die("%s", err.buf);
ref_transaction_free(transaction);
- return ret;
+ return 0;
}
if (end_null)
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 30/41] refs.c: remove lock_ref_sha1
lock_ref_sha1 was only called from one place in refc.c and only provided
a check that the refname was sane before adding back the initial "refs/"
part of the ref path name, the initial "refs/" that this caller had already
stripped off before calling lock_ref_sha1.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/refs.c b/refs.c
index 245c540..bc750f4 100644
--- a/refs.c
+++ b/refs.c
@@ -2139,15 +2139,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
-{
- char refpath[PATH_MAX];
- if (check_refname_format(refname, 0))
- return NULL;
- strcpy(refpath, mkpath("refs/%s", refname));
- return lock_ref_sha1_basic(refpath, old_sha1, 0, NULL);
-}
-
struct ref_lock *lock_any_ref_for_update(const char *refname,
const unsigned char *old_sha1,
int flags, int *type_p)
@@ -2356,8 +2347,12 @@ static void try_remove_empty_parents(char *name)
/* make sure nobody touched the ref, and unlink */
static void prune_ref(struct ref_to_prune *r)
{
- struct ref_lock *lock = lock_ref_sha1(r->name + 5, r->sha1);
+ struct ref_lock *lock;
+
+ if (check_refname_format(r->name + 5, 0))
+ return;
+ lock = lock_ref_sha1_basic(r->name, r->sha1, 0, NULL);
if (lock) {
unlink_or_warn(git_path("%s", r->name));
unlock_ref(lock);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 04/41] refs.c: allow passing NULL to ref_transaction_free
Allow ref_transaction_free(NULL) as a no-op. This makes ref_transaction_free
easier to use and more similar to plain 'free'.
In particular, it lets us rollback unconditionally as part of cleanup code
after setting 'transaction = NULL' if a transaction has been committed or
rolled back already.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/refs.c b/refs.c
index a767ef6..0faed29 100644
--- a/refs.c
+++ b/refs.c
@@ -3312,6 +3312,9 @@ void ref_transaction_free(struct ref_transaction
*transaction)
{
int i;
+ if (!transaction)
+ return;
+
for (i = 0; i < transaction->nr; i++)
free(transaction->updates[i]);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 23/41] refs.c: change update_ref to use a transaction
Change the update_ref helper function to use a ref transaction internally.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 25 +
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index dd98202..6d2caf7 100644
--- a/refs.c
+++ b/refs.c
@@ -3514,11 +3514,28 @@ int update_ref(const char *action, const char *refname,
const unsigned char *sha1, const unsigned char *oldval,
int flags, enum action_on_err onerr)
{
- struct ref_lock *lock;
- lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
- if (!lock)
+ struct ref_transaction *t;
+ struct strbuf err = STRBUF_INIT;
+
+ t = ref_transaction_begin(&err);
+ if (!t ||
+ ref_transaction_update(t, refname, sha1, oldval, flags,
+ !!oldval, &err) ||
+ ref_transaction_commit(t, action, &err)) {
+ const char *str = "update_ref failed for ref '%s': %s";
+
+ ref_transaction_free(t);
+ switch (onerr) {
+ case UPDATE_REFS_MSG_ON_ERR:
+ error(str, refname, err.buf); break;
+ case UPDATE_REFS_DIE_ON_ERR:
+ die(str, refname, err.buf); break;
+ case UPDATE_REFS_QUIET_ON_ERR: break;
+ }
+ strbuf_release(&err);
return 1;
- return update_ref_write(action, refname, sha1, lock, NULL, onerr);
+ }
+ return 0;
}
static int ref_update_compare(const void *r1, const void *r2)
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for
sanity, i.e. that status must be OPEN when _commit/_create/_delete or
_update is called or else die(BUG:...)
Signed-off-by: Ronnie Sahlberg
---
refs.c | 40 +++-
1 file changed, 39 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index ee87eda..dd98202 100644
--- a/refs.c
+++ b/refs.c
@@ -3377,6 +3377,25 @@ struct ref_update {
};
/*
+ * Transaction states.
+ * OPEN: The transaction is in a valid state and can accept new updates.
+ * An OPEN transaction can be committed.
+ * CLOSED: If an open transaction is successfully committed the state will
+ * change to CLOSED. No further changes can be made to a CLOSED
+ * transaction.
+ * CLOSED means that all updates have been successfully committed and
+ * the only thing that remains is to free the completed transaction.
+ * ERROR: The transaction has failed and is no longer committable.
+ * No further changes can be made to a CLOSED transaction and it must
+ * be rolled back using transaction_free.
+ */
+enum ref_transaction_state {
+ REF_TRANSACTION_OPEN = 0,
+ REF_TRANSACTION_CLOSED = 1,
+ REF_TRANSACTION_ERROR = 2,
+};
+
+/*
* Data structure for holding a reference transaction, which can
* consist of checks and updates to multiple references, carried out
* as atomically as possible. This structure is opaque to callers.
@@ -3385,6 +3404,8 @@ struct ref_transaction {
struct ref_update **updates;
size_t alloc;
size_t nr;
+ enum ref_transaction_state state;
+ int status;
};
struct ref_transaction *ref_transaction_begin(struct strbuf *err)
@@ -3427,6 +3448,9 @@ int ref_transaction_update(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: update called for transaction that is not open");
+
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
@@ -3447,6 +3471,9 @@ int ref_transaction_create(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: create called for transaction that is not open");
+
if (!new_sha1 || is_null_sha1(new_sha1))
die("BUG: create ref with null new_sha1");
@@ -3467,6 +3494,9 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
{
struct ref_update *update;
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: delete called for transaction that is not open");
+
if (have_old && !old_sha1)
die("BUG: have_old is true but old_sha1 is NULL");
@@ -3522,8 +3552,13 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
- if (!n)
+ if (transaction->state != REF_TRANSACTION_OPEN)
+ die("BUG: commit called for transaction that is not open");
+
+ if (!n) {
+ transaction->state = REF_TRANSACTION_CLOSED;
return 0;
+ }
/* Allocate work space */
delnames = xmalloc(sizeof(*delnames) * n);
@@ -3586,6 +3621,9 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
clear_loose_ref_cache(&ref_cache);
cleanup:
+ transaction->state = ret ? REF_TRANSACTION_ERROR
+ : REF_TRANSACTION_CLOSED;
+
for (i = 0; i < n; i++)
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 22/41] branch.c: use ref transaction for all ref updates
Change create_branch to use a ref transaction when creating the new branch.
This also fixes a race condition in the old code where two concurrent
create_branch could race since the lock_any_ref_for_update/write_ref_sha1
did not protect against the ref already existing. I.e. one thread could end up
overwriting a branch even if the forcing flag is false.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
branch.c | 30 --
1 file changed, 16 insertions(+), 14 deletions(-)
diff --git a/branch.c b/branch.c
index 660097b..c1eae00 100644
--- a/branch.c
+++ b/branch.c
@@ -226,7 +226,6 @@ void create_branch(const char *head,
int force, int reflog, int clobber_head,
int quiet, enum branch_track track)
{
- struct ref_lock *lock = NULL;
struct commit *commit;
unsigned char sha1[20];
char *real_ref, msg[PATH_MAX + 20];
@@ -285,15 +284,6 @@ void create_branch(const char *head,
die(_("Not a valid branch point: '%s'."), start_name);
hashcpy(sha1, commit->object.sha1);
- if (!dont_change_ref) {
- lock = lock_any_ref_for_update(ref.buf, NULL, 0, NULL);
- if (!lock)
- die_errno(_("Failed to lock ref for update"));
- }
-
- if (reflog)
- log_all_ref_updates = 1;
-
if (forcing)
snprintf(msg, sizeof msg, "branch: Reset to %s",
start_name);
@@ -301,13 +291,25 @@ void create_branch(const char *head,
snprintf(msg, sizeof msg, "branch: Created from %s",
start_name);
+ if (reflog)
+ log_all_ref_updates = 1;
+
+ if (!dont_change_ref) {
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, sha1,
+ null_sha1, 0, !forcing, &err) ||
+ ref_transaction_commit(transaction, msg, &err))
+ die("%s", err.buf);
+ ref_transaction_free(transaction);
+ }
+
if (real_ref && track)
setup_tracking(ref.buf + 11, real_ref, track, quiet);
- if (!dont_change_ref)
- if (write_ref_sha1(lock, sha1, msg) < 0)
- die_errno(_("Failed to write ref"));
-
strbuf_release(&ref);
free(real_ref);
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 38/41] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn.
There are some conditions where we want to allow a temporary conflict and skip
checking those refs. For example if we have a transaction that
1, guarantees that m is a packed refs and there is no loose ref for m
2, the transaction will delete m from the packed ref
3, the transaction will create conflicting m/m
For this case we want to be able to lock and create m/m since we know that the
conflict is only transient. I.e. the conflict will be automatically resolved
by the transaction when it deletes m.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 41 ++---
1 file changed, 26 insertions(+), 15 deletions(-)
diff --git a/refs.c b/refs.c
index f13c0be..f2ca96a 100644
--- a/refs.c
+++ b/refs.c
@@ -791,15 +791,18 @@ static int names_conflict(const char *refname1, const
char *refname2)
struct name_conflict_cb {
const char *refname;
- const char *oldrefname;
const char *conflicting_refname;
+ const char **skip;
+ int skipnum;
};
static int name_conflict_fn(struct ref_entry *entry, void *cb_data)
{
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
- if (data->oldrefname && !strcmp(data->oldrefname, entry->name))
- return 0;
+ int i;
+ for (i = 0; i < data->skipnum; i++)
+ if (!strcmp(entry->name, data->skip[i]))
+ return 0;
if (names_conflict(data->refname, entry->name)) {
data->conflicting_refname = entry->name;
return 1;
@@ -812,15 +815,18 @@ static int name_conflict_fn(struct ref_entry *entry, void
*cb_data)
* conflicting with the name of an existing reference in dir. If
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
- * operation).
+ * operation). skip contains a list of refs we want to skip checking for
+ * conflicts with.
*/
-static int is_refname_available(const char *refname, const char *oldrefname,
- struct ref_dir *dir)
+static int is_refname_available(const char *refname,
+ struct ref_dir *dir,
+ const char **skip, int skipnum)
{
struct name_conflict_cb data;
data.refname = refname;
- data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
+ data.skip = skip;
+ data.skipnum = skipnum;
sort_ref_dir(dir);
if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, &data)) {
@@ -2047,7 +2053,8 @@ int dwim_log(const char *str, int len, unsigned char
*sha1, char **log)
static struct ref_lock *lock_ref_sha1_basic(const char *refname,
const unsigned char *old_sha1,
- int flags, int *type_p)
+ int flags, int *type_p,
+ const char **skip, int skipnum)
{
char *ref_file;
const char *orig_refname = refname;
@@ -2096,7 +2103,8 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
* name is a proper prefix of our refname.
*/
if (missing &&
-!is_refname_available(refname, NULL, get_packed_refs(&ref_cache)))
{
+!is_refname_available(refname, get_packed_refs(&ref_cache),
+ skip, skipnum)) {
last_errno = ENOTDIR;
goto error_return;
}
@@ -2154,7 +2162,7 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
+ return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0);
}
/*
@@ -2645,10 +2653,12 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
if (!symref)
return error("refname %s not found", oldrefname);
- if (!is_refname_available(newrefname, oldrefname,
get_packed_refs(&ref_cache)))
+ if (!is_refname_available(newrefname, get_packed_refs(&ref_cache),
+ &oldrefname, 1))
return 1;
- if (!is_refname_available(newrefname, oldrefname,
get_loose_refs(&ref_cache)))
+ if (!is_refname_available(newrefname, get_loose_refs(&ref_cache),
+ &oldrefname, 1))
return 1;
if (log && rename(git_path("logs/%s", oldrefname),
git_path(TMP_RENAMED_LOG)))
@@ -2681,7 +2691,7 @@ int rename_ref(const char *oldrefname, const char
*newrefname, const char *logms
logmoved = log;
- lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL);
+ lock = lock_ref
[PATCH v13 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update
to lock_ref_sha1_basic. At some later stage we will get rid of
lock_any_ref_for_update completely.
If lock_ref_sha1_basic fails the check_refname_format test, set errno to
EINVAL before returning NULL. This to guarantee that we will not return an
error without updating errno.
This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed.
But this wrapper is also called from an external caller and we will soon
make changes to the signature to lock_ref_sha1_basic that we do not want to
expose to that caller.
This changes semantics for lock_ref_sha1_basic slightly. With this change
it is no longer possible to open a ref that has a badly name which breaks
any codepaths that tries to open and repair badly named refs. The normal refs
API should not allow neither creating nor accessing refs with invalid names.
If we need such recovery code we could add it as an option to git fsck and have
git fsck be the only sanctioned way of bypassing the normal API and checks.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 03cff59..22fb166 100644
--- a/refs.c
+++ b/refs.c
@@ -2058,6 +2058,11 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
int missing = 0;
int attempts_remaining = 3;
+ if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) {
+ errno = EINVAL;
+ return NULL;
+ }
+
lock = xcalloc(1, sizeof(struct ref_lock));
lock->lock_fd = -1;
@@ -2149,8 +2154,6 @@ struct ref_lock *lock_any_ref_for_update(const char
*refname,
const unsigned char *old_sha1,
int flags, int *type_p)
{
- if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL))
- return NULL;
return lock_ref_sha1_basic(refname, old_sha1, flags, type_p);
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 32/41] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time
since we no longer have any callers of repack_without_ref we can now delete
this function.
Change delete_ref to return 0 on success and 1 on failure instead of the
previous 0 on success either 1 or -1 on failure.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 34 +-
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/refs.c b/refs.c
index e19041a..4cdbc26 100644
--- a/refs.c
+++ b/refs.c
@@ -2513,11 +2513,6 @@ static int repack_without_refs(const char **refnames,
int n, struct strbuf *err)
return ret;
}
-static int repack_without_ref(const char *refname)
-{
- return repack_without_refs(&refname, 1, NULL);
-}
-
static int add_err_if_unremovable(const char *op, const char *file,
struct strbuf *e, int rc)
{
@@ -2557,24 +2552,21 @@ static int delete_ref_loose(struct ref_lock *lock, int
flag, struct strbuf *err)
int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
{
- struct ref_lock *lock;
- int ret = 0, flag = 0;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
- lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
- if (!lock)
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_delete(transaction, refname, sha1, delopt,
+ sha1 && !is_null_sha1(sha1), &err) ||
+ ref_transaction_commit(transaction, NULL, &err)) {
+ error("%s", err.buf);
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
return 1;
- ret |= delete_ref_loose(lock, flag, NULL);
-
- /* removing the loose one could have resurrected an earlier
-* packed one. Also, if it was not loose we need to repack
-* without it.
-*/
- ret |= repack_without_ref(lock->ref_name);
-
- unlink_or_warn(git_path("logs/%s", lock->ref_name));
- clear_loose_ref_cache(&ref_cache);
- unlock_ref(lock);
- return ret;
+ }
+ ref_transaction_free(transaction);
+ return 0;
}
/*
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 14/41] refs.c: update ref_transaction_delete to check for error and return status
Change ref_transaction_delete() to do basic error checking and return
non-zero of error. Update all callers to check the return for
ref_transaction_delete(). There are currently no conditions in _delete that
will return error but there will be in the future. Add an err argument that
will be updated on failure.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 5 +++--
refs.c | 16 +++-
refs.h | 12
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 41121fa..7c9c248 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("delete %s: extra input: %s", refname, next);
- ref_transaction_delete(transaction, refname, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_delete(transaction, refname, old_sha1,
+ update_flags, have_old, &err))
+ die("%s", err.buf);
update_flags = 0;
free(refname);
diff --git a/refs.c b/refs.c
index 3cb99f6..d9b99c7 100644
--- a/refs.c
+++ b/refs.c
@@ -3459,19 +3459,25 @@ int ref_transaction_create(struct ref_transaction
*transaction,
return 0;
}
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+ if (have_old && !old_sha1)
+ die("BUG: have_old is true but old_sha1 is NULL");
+
+ update = add_update(transaction, refname);
update->flags = flags;
update->have_old = have_old;
if (have_old) {
assert(!is_null_sha1(old_sha1));
hashcpy(update->old_sha1, old_sha1);
}
+ return 0;
}
int update_ref(const char *action, const char *refname,
diff --git a/refs.h b/refs.h
index bda790e6..58ba365 100644
--- a/refs.h
+++ b/refs.h
@@ -303,11 +303,15 @@ int ref_transaction_create(struct ref_transaction
*transaction,
* Add a reference deletion to transaction. If have_old is true, then
* old_sha1 holds the value that the reference should have had before
* the update (which must not be the null SHA-1).
+ * Function returns 0 on success and non-zero on failure. A failure to delete
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back.
*/
-void ref_transaction_delete(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *old_sha1,
- int flags, int have_old);
+int ref_transaction_delete(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err);
/*
* Commit all of the changes that have been queued in transaction, as
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 09/41] refs.c: make update_ref_write update a strbuf on failure
Change update_ref_write to also update an error strbuf on failure.
This makes the error available to ref_transaction_commit callers if the
transaction failed due to update_ref_sha1/write_ref_sha1 failures.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/refs.c b/refs.c
index dd498cf..6696082 100644
--- a/refs.c
+++ b/refs.c
@@ -3343,10 +3343,13 @@ static struct ref_lock *update_ref_lock(const char
*refname,
static int update_ref_write(const char *action, const char *refname,
const unsigned char *sha1, struct ref_lock *lock,
- enum action_on_err onerr)
+ struct strbuf *err, enum action_on_err onerr)
{
if (write_ref_sha1(lock, sha1, action) < 0) {
const char *str = "Cannot update the ref '%s'.";
+ if (err)
+ strbuf_addf(err, str, refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
@@ -3467,7 +3470,7 @@ int update_ref(const char *action, const char *refname,
lock = update_ref_lock(refname, oldval, flags, NULL, onerr);
if (!lock)
return 1;
- return update_ref_write(action, refname, sha1, lock, onerr);
+ return update_ref_write(action, refname, sha1, lock, NULL, onerr);
}
static int ref_update_compare(const void *r1, const void *r2)
@@ -3549,7 +3552,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
ret = update_ref_write(msg,
update->refname,
update->new_sha1,
- update->lock, onerr);
+ update->lock, err, onerr);
update->lock = NULL; /* freed by update_ref_write */
if (ret)
goto cleanup;
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 24/41] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction.
Signed-off-by: Ronnie Sahlberg
---
builtin/receive-pack.c | 31 ++-
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..13f4a63 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -46,6 +46,7 @@ static void *head_name_to_free;
static int sent_capabilities;
static int shallow_update;
static const char *alt_shallow_file;
+static struct string_list error_strings = STRING_LIST_INIT_DUP;
static enum deny_action parse_deny_action(const char *var, const char *value)
{
@@ -475,7 +476,6 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *namespaced_name;
unsigned char *old_sha1 = cmd->old_sha1;
unsigned char *new_sha1 = cmd->new_sha1;
- struct ref_lock *lock;
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return NULL; /* good */
}
else {
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+
if (shallow_update && si->shallow_ref[cmd->index] &&
update_shallow_ref(cmd, si))
return "shallow error";
- lock = lock_any_ref_for_update(namespaced_name, old_sha1,
- 0, NULL);
- if (!lock) {
- rp_error("failed to lock %s", name);
- return "failed to lock";
- }
- if (write_ref_sha1(lock, new_sha1, "push")) {
- return "failed to write"; /* error() already called */
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, namespaced_name,
+ new_sha1, old_sha1, 0, 1, &err) ||
+ ref_transaction_commit(transaction, "push", &err)) {
+
+ const char *str;
+ string_list_append(&error_strings, err.buf);
+ str = error_strings.items[error_strings.nr - 1].string;
+ strbuf_release(&err);
+
+ ref_transaction_free(transaction);
+ rp_error("%s", str);
+ return str;
}
+
+ ref_transaction_free(transaction);
+ strbuf_release(&err);
return NULL; /* good */
}
}
@@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
packet_flush(1);
sha1_array_clear(&shallow);
sha1_array_clear(&ref);
+ string_list_clear(&error_strings, 0);
return 0;
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 12/41] refs.c: change ref_transaction_update() to do error checking and return status
Update ref_transaction_update() do some basic error checking and return
non-zero on error. Update all callers to check ref_transaction_update() for
error. There are currently no conditions in _update that will return error but
there will be in the future. Add an err argument that will be updated on
failure. In future patches we will start doing both locking and checking
for name conflicts in _update instead of _commit at which time this function
will start returning errors for these conditions.
Also check for BUGs during update and die(BUG:...) if we are calling
_update with have_old but the old_sha1 pointer is NULL.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/update-ref.c | 12 +++-
refs.c | 18 --
refs.h | 14 +-
3 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 88ab785..3067b11 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -16,6 +16,7 @@ static struct ref_transaction *transaction;
static char line_termination = '\n';
static int update_flags;
+static struct strbuf err = STRBUF_INIT;
/*
* Parse one whitespace- or NUL-terminated, possibly C-quoted argument
@@ -197,8 +198,9 @@ static const char *parse_cmd_update(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("update %s: extra input: %s", refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, &err))
+ die("%s", err.buf);
update_flags = 0;
free(refname);
@@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input,
const char *next)
if (*next != line_termination)
die("verify %s: extra input: %s", refname, next);
- ref_transaction_update(transaction, refname, new_sha1, old_sha1,
- update_flags, have_old);
+ if (ref_transaction_update(transaction, refname, new_sha1, old_sha1,
+ update_flags, have_old, &err))
+ die("%s", err.buf);
update_flags = 0;
free(refname);
@@ -342,7 +345,6 @@ int cmd_update_ref(int argc, const char **argv, const char
*prefix)
const char *refname, *oldval, *msg = NULL;
unsigned char sha1[20], oldsha1[20];
int delete = 0, no_deref = 0, read_stdin = 0, end_null = 0, flags = 0;
- struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_STRING( 'm', NULL, &msg, N_("reason"), N_("reason of the
update")),
OPT_BOOL('d', NULL, &delete, N_("delete the reference")),
diff --git a/refs.c b/refs.c
index fb44978..f5e7a12 100644
--- a/refs.c
+++ b/refs.c
@@ -3418,19 +3418,25 @@ static struct ref_update *add_update(struct
ref_transaction *transaction,
return update;
}
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
- const unsigned char *old_sha1,
- int flags, int have_old)
+int ref_transaction_update(struct ref_transaction *transaction,
+ const char *refname,
+ const unsigned char *new_sha1,
+ const unsigned char *old_sha1,
+ int flags, int have_old,
+ struct strbuf *err)
{
- struct ref_update *update = add_update(transaction, refname);
+ struct ref_update *update;
+
+ if (have_old && !old_sha1)
+ die("BUG: have_old is true but old_sha1 is NULL");
+ update = add_update(transaction, refname);
hashcpy(update->new_sha1, new_sha1);
update->flags = flags;
update->have_old = have_old;
if (have_old)
hashcpy(update->old_sha1, old_sha1);
+ return 0;
}
void ref_transaction_create(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 6ee9c9e..32edf3f 100644
--- a/refs.h
+++ b/refs.h
@@ -234,12 +234,16 @@ struct ref_transaction *ref_transaction_begin(void);
* be deleted. If have_old is true, then old_sha1 holds the value
* that the reference should have had before the update, or zeros if
* it must not have existed beforehand.
+ * Function returns 0 on success and non-zero on failure. A failure to update
+ * means that the transaction as a whole has failed and will need to be
+ * rolled back. On failure the err buffer will be updated.
*/
-void ref_transaction_update(struct ref_transaction *transaction,
- const char *refname,
- const unsigned char *new_sha1,
-
[PATCH v13 07/41] refs.c: make ref_update_reject_duplicates take a strbuf argument for errors
Make ref_update_reject_duplicates return any error that occurs through a
new strbuf argument. This means that when a transaction commit fails in
this function we will now be able to pass a helpful error message back to the
caller.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/refs.c b/refs.c
index 7ec1f32..2d6941b 100644
--- a/refs.c
+++ b/refs.c
@@ -3456,6 +3456,7 @@ static int ref_update_compare(const void *r1, const void
*r2)
}
static int ref_update_reject_duplicates(struct ref_update **updates, int n,
+ struct strbuf *err,
enum action_on_err onerr)
{
int i;
@@ -3463,6 +3464,9 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
if (!strcmp(updates[i - 1]->refname, updates[i]->refname)) {
const char *str =
"Multiple updates for ref '%s' not allowed.";
+ if (err)
+ strbuf_addf(err, str, updates[i]->refname);
+
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, updates[i]->refname); break;
@@ -3493,7 +3497,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, onerr);
+ ret = ref_update_reject_duplicates(updates, n, err, onerr);
if (ret)
goto cleanup;
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 08/41] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive
error string back to the caller. Pass the err argument from transaction
commit to this function so that transaction users will have a nice error
string if the transaction failed due to delete_loose_ref.
Add a new function unlink_or_err that we can call from delete_ref_loose. This
function is similar to unlink_or_warn except that we can pass it an err
argument. If err is non-NULL the function will populate err instead of
printing a warning().
Signed-off-by: Ronnie Sahlberg
---
refs.c | 35 +--
1 file changed, 29 insertions(+), 6 deletions(-)
diff --git a/refs.c b/refs.c
index 2d6941b..dd498cf 100644
--- a/refs.c
+++ b/refs.c
@@ -2510,16 +2510,38 @@ static int repack_without_ref(const char *refname)
return repack_without_refs(&refname, 1, NULL);
}
-static int delete_ref_loose(struct ref_lock *lock, int flag)
+static int add_err_if_unremovable(const char *op, const char *file,
+ struct strbuf *e, int rc)
+{
+ int err = errno;
+ if (rc < 0 && errno != ENOENT) {
+ strbuf_addf(e, "unable to %s %s: %s",
+ op, file, strerror(errno));
+ errno = err;
+ return -1;
+ }
+ return 0;
+}
+
+static int unlink_or_err(const char *file, struct strbuf *err)
+{
+ if (err)
+ return add_err_if_unremovable("unlink", file, err,
+ unlink(file));
+ else
+ return unlink_or_warn(file);
+}
+
+static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf
*err)
{
if (!(flag & REF_ISPACKED) || flag & REF_ISSYMREF) {
/* loose */
- int err, i = strlen(lock->lk->filename) - 5; /* .lock */
+ int res, i = strlen(lock->lk->filename) - 5; /* .lock */
lock->lk->filename[i] = 0;
- err = unlink_or_warn(lock->lk->filename);
+ res = unlink_or_err(lock->lk->filename, err);
lock->lk->filename[i] = '.';
- if (err && errno != ENOENT)
+ if (res && errno != ENOENT)
return 1;
}
return 0;
@@ -2533,7 +2555,7 @@ int delete_ref(const char *refname, const unsigned char
*sha1, int delopt)
lock = lock_ref_sha1_basic(refname, sha1, delopt, &flag);
if (!lock)
return 1;
- ret |= delete_ref_loose(lock, flag);
+ ret |= delete_ref_loose(lock, flag, NULL);
/* removing the loose one could have resurrected an earlier
* packed one. Also, if it was not loose we need to repack
@@ -3540,7 +3562,8 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (update->lock) {
delnames[delnum++] = update->lock->ref_name;
- ret |= delete_ref_loose(update->lock, update->type);
+ ret |= delete_ref_loose(update->lock, update->type,
+ err);
}
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 17/41] tag.c: use ref transactions when doing updates
Change tag.c to use ref transactions for all ref updates.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
builtin/tag.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index c6e8a71..c9bfc9a 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -548,7 +548,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf ref = STRBUF_INIT;
unsigned char object[20], prev[20];
const char *object_ref, *tag;
- struct ref_lock *lock;
struct create_tag_options opt;
char *cleanup_arg = NULL;
int annotate = 0, force = 0, lines = -1;
@@ -556,6 +555,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, &lines, N_("n"),
@@ -701,11 +702,13 @@ int cmd_tag(int argc, const char **argv, const char
*prefix)
if (annotate)
create_tag(object, tag, &buf, &opt, prev, object);
- lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL);
- if (!lock)
- die(_("%s: cannot lock the ref"), ref.buf);
- if (write_ref_sha1(lock, object, NULL) < 0)
- die(_("%s: cannot update the ref"), ref.buf);
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref.buf, object, prev,
+ 0, !is_null_sha1(prev), &err) ||
+ ref_transaction_commit(transaction, NULL, &err))
+ die("%s", err.buf);
+ ref_transaction_free(transaction);
if (force && !is_null_sha1(prev) && hashcmp(prev, object))
printf(_("Updated tag '%s' (was %s)\n"), tag,
find_unique_abbrev(prev, DEFAULT_ABBREV));
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 29/41] refs.c: remove the update_ref_write function
Since we only call update_ref_write from a single place and we only call it
with onerr==QUIET_ON_ERR we can just as well get rid of it and just call
write_ref_sha1 directly. This changes the return status for _commit from
1 to -1 on failures when writing to the ref. Eventually we will want
_commit to start returning more detailed error conditions than the current
simple success/failure. For example if the commit failed due to name
conflicts etc.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 35 +--
1 file changed, 9 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 3796957..245c540 100644
--- a/refs.c
+++ b/refs.c
@@ -3323,25 +3323,6 @@ int for_each_reflog(each_ref_fn fn, void *cb_data)
return retval;
}
-static int update_ref_write(const char *action, const char *refname,
- const unsigned char *sha1, struct ref_lock *lock,
- struct strbuf *err, enum action_on_err onerr)
-{
- if (write_ref_sha1(lock, sha1, action) < 0) {
- const char *str = "Cannot update the ref '%s'.";
- if (err)
- strbuf_addf(err, str, refname);
-
- switch (onerr) {
- case UPDATE_REFS_MSG_ON_ERR: error(str, refname); break;
- case UPDATE_REFS_DIE_ON_ERR: die(str, refname); break;
- case UPDATE_REFS_QUIET_ON_ERR: break;
- }
- return 1;
- }
- return 0;
-}
-
/**
* Information needed for a single ref update. Set new_sha1 to the
* new value or to zero to delete the ref. To check the old value
@@ -3592,14 +3573,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (!is_null_sha1(update->new_sha1)) {
- ret = update_ref_write(msg,
- update->refname,
- update->new_sha1,
- update->lock, err,
- UPDATE_REFS_QUIET_ON_ERR);
- update->lock = NULL; /* freed by update_ref_write */
- if (ret)
+ ret = write_ref_sha1(update->lock, update->new_sha1,
+msg);
+ update->lock = NULL; /* freed by write_ref_sha1 */
+ if (ret) {
+ const char *str = "Cannot update the ref '%s'.";
+
+ if (err)
+ strbuf_addf(err, str, update->refname);
goto cleanup;
+ }
}
}
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 25/41] fast-import.c: use a ref transaction when dumping tags
Signed-off-by: Ronnie Sahlberg
---
fast-import.c | 29 +++--
1 file changed, 23 insertions(+), 6 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index 4a7b196..587ef4a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1735,15 +1735,32 @@ static void dump_tags(void)
{
static const char *msg = "fast-import";
struct tag *t;
- struct ref_lock *lock;
- char ref_name[PATH_MAX];
+ struct strbuf ref_name = STRBUF_INIT;
+ struct strbuf err = STRBUF_INIT;
+ struct ref_transaction *transaction;
+ transaction = ref_transaction_begin(&err);
+ if (!transaction) {
+ failure |= error("%s", err.buf);
+ goto cleanup;
+ }
for (t = first_tag; t; t = t->next_tag) {
- sprintf(ref_name, "tags/%s", t->name);
- lock = lock_ref_sha1(ref_name, NULL);
- if (!lock || write_ref_sha1(lock, t->sha1, msg) < 0)
- failure |= error("Unable to update %s", ref_name);
+ strbuf_reset(&ref_name);
+ strbuf_addf(&ref_name, "refs/tags/%s", t->name);
+
+ if (ref_transaction_update(transaction, ref_name.buf, t->sha1,
+ NULL, 0, 0, &err)) {
+ failure |= error("%s", err.buf);
+ goto cleanup;
+ }
}
+ if (ref_transaction_commit(transaction, msg, &err))
+ failure |= error("%s", err.buf);
+
+ cleanup:
+ ref_transaction_free(transaction);
+ strbuf_release(&ref_name);
+ strbuf_release(&err);
}
static void dump_marks_helper(FILE *f,
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when
we lstat the new refname and it returns ENOTDIR or if the name checking
function reports that the same type of conflict happened. In both cases it
means that we can not create the new ref due to a name conflict.
For these cases, save the errno value and abort and make sure that the caller
can see errno==ENOTDIR.
Also start defining specific return codes for _commit, assign -1 as a generic
error and -2 as the error that refers to a name conflict. Callers can (and
should) use that return value inspecting errno directly.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 22 +++---
refs.h | 6 ++
2 files changed, 21 insertions(+), 7 deletions(-)
diff --git a/refs.c b/refs.c
index f2ca96a..bcf238e 100644
--- a/refs.c
+++ b/refs.c
@@ -3551,7 +3551,7 @@ static int ref_update_reject_duplicates(struct ref_update
**updates, int n,
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err)
{
- int ret = 0, delnum = 0, i;
+ int ret = 0, delnum = 0, i, df_conflict = 0;
const char **delnames;
int n = transaction->nr;
struct ref_update **updates = transaction->updates;
@@ -3569,9 +3569,10 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
/* Copy, sort, and reject duplicate refs */
qsort(updates, n, sizeof(*updates), ref_update_compare);
- ret = ref_update_reject_duplicates(updates, n, err);
- if (ret)
+ if (ref_update_reject_duplicates(updates, n, err)) {
+ ret = -1;
goto cleanup;
+ }
/* Acquire all locks while verifying old values */
for (i = 0; i < n; i++) {
@@ -3585,10 +3586,12 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
&update->type,
delnames, delnum);
if (!update->lock) {
+ if (errno == ENOTDIR)
+ df_conflict = 1;
if (err)
strbuf_addf(err, "Cannot lock the ref '%s'.",
update->refname);
- ret = 1;
+ ret = -1;
goto cleanup;
}
}
@@ -3606,6 +3609,7 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
if (err)
strbuf_addf(err, str, update->refname);
+ ret = -1;
goto cleanup;
}
}
@@ -3616,14 +3620,16 @@ int ref_transaction_commit(struct ref_transaction
*transaction,
struct ref_update *update = updates[i];
if (update->lock) {
- ret |= delete_ref_loose(update->lock, update->type,
- err);
+ if (delete_ref_loose(update->lock, update->type, err))
+ ret = -1;
+
if (!(update->flags & REF_ISPRUNING))
delnames[delnum++] = update->lock->ref_name;
}
}
- ret |= repack_without_refs(delnames, delnum, err);
+ if (repack_without_refs(delnames, delnum, err))
+ ret = -1;
for (i = 0; i < delnum; i++)
unlink_or_warn(git_path("logs/%s", delnames[i]));
clear_loose_ref_cache(&ref_cache);
@@ -3636,6 +3642,8 @@ cleanup:
if (updates[i]->lock)
unlock_ref(updates[i]->lock);
free(delnames);
+ if (df_conflict)
+ ret = -2;
return ret;
}
diff --git a/refs.h b/refs.h
index 5c6c20f..1583097 100644
--- a/refs.h
+++ b/refs.h
@@ -323,7 +323,13 @@ int ref_transaction_delete(struct ref_transaction
*transaction,
* Commit all of the changes that have been queued in transaction, as
* atomically as possible. Return a nonzero value if there is a
* problem.
+ * If the transaction is already in failed state this function will return
+ * an error.
+ * Function returns 0 on success, -1 for generic failures and
+ * UPDATE_REFS_NAME_CONFLICT (-2) if the failure was due to a name
+ * collision (ENOTDIR).
*/
+#define UPDATE_REFS_NAME_CONFLICT -2
int ref_transaction_commit(struct ref_transaction *transaction,
struct strbuf *err);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 41/41] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static.
Signed-off-by: Ronnie Sahlberg
---
refs.c | 6 +-
refs.h | 3 ---
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index bcf238e..f8a6c9a 100644
--- a/refs.c
+++ b/refs.c
@@ -2634,6 +2634,9 @@ static int rename_tmp_log(const char *newrefname)
return 0;
}
+static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
+ const char *logmsg);
+
int rename_ref(const char *oldrefname, const char *newrefname, const char
*logmsg)
{
unsigned char sha1[20], orig_sha1[20];
@@ -2885,7 +2888,8 @@ static int is_branch(const char *refname)
return !strcmp(refname, "HEAD") || starts_with(refname, "refs/heads/");
}
-int write_ref_sha1(struct ref_lock *lock,
+/** Writes sha1 into the ref specified by the lock. **/
+static int write_ref_sha1(struct ref_lock *lock,
const unsigned char *sha1, const char *logmsg)
{
static char term = '\n';
diff --git a/refs.h b/refs.h
index 1583097..8177052 100644
--- a/refs.h
+++ b/refs.h
@@ -195,9 +195,6 @@ extern int commit_ref(struct ref_lock *lock);
/** Release any lock taken but not written. **/
extern void unlock_ref(struct ref_lock *lock);
-/** Writes sha1 into the ref specified by the lock. **/
-extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1,
const char *msg);
-
/** Setup reflog before using. **/
int log_ref_setup(const char *refname, char *logfile, int bufsize);
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v13 27/41] refs.c: make lock_ref_sha1 static
No external callers reference lock_ref_sha1 any more so lets declare it static.
Reviewed-by: Jonathan Nieder
Signed-off-by: Ronnie Sahlberg
---
refs.c | 2 +-
refs.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/refs.c b/refs.c
index 6d2caf7..d2fd419 100644
--- a/refs.c
+++ b/refs.c
@@ -2139,7 +2139,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char
*refname,
return NULL;
}
-struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
+static struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1)
{
char refpath[PATH_MAX];
if (check_refname_format(refname, 0))
diff --git a/refs.h b/refs.h
index 743358f..c38ee09 100644
--- a/refs.h
+++ b/refs.h
@@ -171,9 +171,6 @@ extern int ref_exists(const char *);
*/
extern int peel_ref(const char *refname, unsigned char *sha1);
-/** Locks a "refs/" ref returning the lock on success and NULL on failure. **/
-extern struct ref_lock *lock_ref_sha1(const char *refname, const unsigned char
*old_sha1);
-
/** Locks any ref (for 'HEAD' type refs). */
#define REF_NODEREF0x01
extern struct ref_lock *lock_any_ref_for_update(const char *refname,
--
2.0.0.567.g64a7adf
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Reset by checkout?
Kevin Bracey wrote: > Maybe we just need to tighten up the EXAMPLES section? Give it > easy-to-locate /--soft/--mixed/--keep subheadings, covering all > those common use cases (in clean trees...), including a before/after > git status views. Then normal users could skip the top technical > section waffling about indexes and go straight there instead. Or maybe we need to have sane options, like --stage, --work, and --keep. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] receive-pack: optionally deny case-clone refs
On Tue, 2014-06-03 at 14:33 -0700, Junio C Hamano wrote: > David Turner writes: > > > It is possible to have two branches which are the same but for case. > > This works great on the case-sensitive filesystems, but not so well on > > case-insensitive filesystems. It is fairly typical to have > > case-insensitive clients (Macs, say) with a case-sensitive server > > (GNU/Linux). > > > > Should a user attempt to pull on a Mac when there are case-clone > > branches with differing contents, they'll get an error message > > containing something like "Ref refs/remotes/origin/lower is at > > [sha-of-lowercase-branch] but expected [sha-of-uppercase-branch] > > (unable to update local ref)" > > > > With a case-insensitive git server, if a branch called capital-M > > Master (that differs from lowercase-m-master) is pushed, nobody else > > can push to (lowercase-m) master until the branch is removed. > > > > Create the option receive.denycaseclonebranches, which checks pushed > > branches to ensure that they are not case-clones of an existing > > branch. This setting is turned on by default if core.ignorecase is > > set, but not otherwise. > > > > Signed-off-by: David Turner > > --- > > I do not object to this new feature in principle, but I do not know > if we want to introduce a new word "case-clone refs" without adding > it to the glossary documentation. I would be happy to add "case-clone" to the glossary -- would that be OK with you? I do not immediately think of the better term. > It feels a bit funny to tie this to core.ignorecase, which is an > attribute of the filesystem used for the working tree, though. It seems like it's an attribute of the filesystem used for the GIT_DIR (at least, that's what's actually tested in order to set it). So I think this is OK. > Updates to Documentation/config.txt and Documentation/git-push.txt > are also needed. > ... > Would it make sense to reuse the enum deny_action for this new > settings, with an eye to later convert the older boolean ones also > to use enum deny_action to make them consistent and more flexible? >... > We write C not C++ around here; the pointer-asterisk sticks to the > variable name, not typename. >...[moved] > The trailing blank line inside a function at the end is somewhat > unusual. Will fix these, thank you. Do you happen to know if there's a style checker available that I could run before committing? > > + return !strcasecmp(refname, incoming_refname) && > > + strcmp(refname, incoming_refname); > > (Mental note to the reviewer himself) This returns true iff there is > an existing ref whose name is only different in case, and cause > for-each-ref to return early with true. In a sane case of not > receiving problematic refs, this will have to iterate over all the > existing refnames. Wonder if there are better ways to optimize this > in a repository with hundreds or thousands of refs, which is not all > that uncommon. My expectation is that on average a push will involve a small number of refs -- usually exactly one. We could put the refs into a case-insensitive hashmap if we expect many refs. This ties into the general question of whether ref handling can be made O(1) or O(log N), which I think the list has not come to a satisfactory solution to. > > diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh > > index 0736bcb..099c0e3 100755 > > --- a/t/t5400-send-pack.sh > > +++ b/t/t5400-send-pack.sh > > @@ -129,6 +129,26 @@ test_expect_success 'denyNonFastforwards trumps > > --force' ' > > test "$victim_orig" = "$victim_head" > > ' > > > > +if ! test_have_prereq CASE_INSENSITIVE_FS > > +then > > Hmm, don't we want the feature to kick in for both case sensitive > and case insensitive filesystems? Yes, but it's harder to test on case-insensitive filesystems because we cannot have coexisting local case-clone branches. We could test by making sure to first locally deleting the case-clone branches, I guess. I'll do that. > > +test_expect_success 'denyCaseCloneBranches works' ' > > + ( > > + cd victim && > > + git config receive.denyCaseCloneBranches true > > + git config receive.denyDeletes false > > + ) && > > + git checkout -b caseclone && > > + git send-pack ./victim caseclone && > > + git checkout -b CaseClone && > > + test_must_fail git send-pack ./victim CaseClone && > > At this point, we would want to see not just that send-pack fails > but also that "victim" does not have CaseClone branch and does have > caseclone branch pointing at the original value (i.e. we do not want > to see "caseclone" updated to a value that would have gone to > CaseClone with this push). > > Each push in the sequence should be preceded by a "git commit" or > something so that we can verify the object at the tip of the ref in > the "victim" repository, I would think. Otherwise it is hard to > tell an expected no-op that has failed and a no-op because it > mistakenly pushed the same value to
Re: [PATCH v5 2/2] refs.c: SSE4.2 optimizations for check_refname_component
David Turner writes:
> diff --git a/git-compat-util.h b/git-compat-util.h
> index f6d3a46..254487a 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -668,6 +668,26 @@ void git_qsort(void *base, size_t nmemb, size_t size,
> #endif
> #endif
>
> +#ifndef NO_SSE42
> +#include
> +/* Clang ships with a version of nmmintrin.h that's incomplete; if
> + * necessary, we define the constants that we're going to use. */
As pointed out by Michael already, we format multiline comments with
no text on the opening line:
/*
* Clang ships
* ... to use.
*/
> +#ifndef _SIDD_UBYTE_OPS
> +#define _SIDD_UBYTE_OPS 0x00
> +#define _SIDD_CMP_EQUAL_ANY 0x00
> +#define _SIDD_CMP_RANGES0x04
> +#define _SIDD_CMP_EQUAL_ORDERED 0x0c
> +#define _SIDD_NEGATIVE_POLARITY 0x10
> +#endif
> +
> +/* This is the system memory page size; it's used so that we can read
> + * outside the bounds of an allocation without segfaulting. It is
> + * assumed to be a power of 2. */
> +#ifndef PAGE_SIZE
> +#define PAGE_SIZE 4096
> +#endif
> +#endif
> +
> #ifdef UNRELIABLE_FSTAT
> #define fstat_is_reliable() 0
> #else
> diff --git a/refs.c b/refs.c
> index dd28f2a..22a2dae 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -24,6 +24,25 @@ static unsigned char refname_disposition[256] = {
> 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
> };
>
> +static int check_refname_component_trailer(const char *cp, const char
> *refname, int flags)
> +{
> + if (cp == refname)
> + return 0; /* Component has zero length. */
> + if (refname[0] == '.') {
> + if (!(flags & REFNAME_DOT_COMPONENT))
> + return -1; /* Component starts with '.'. */
> + /*
> + * Even if leading dots are allowed, don't allow "."
> + * as a component (".." is prevented by a rule above).
> + */
> + if (refname[1] == '\0')
> + return -1; /* Component equals ".". */
> + }
> + if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
> + return -1; /* Refname ends with ".lock". */
> + return cp - refname;
> +}
> +
> /*
> * Try to read one refname component from the front of refname.
> * Return the length of the component found, or -1 if the component is
> @@ -37,7 +56,7 @@ static unsigned char refname_disposition[256] = {
> * - it ends with ".lock"
> * - it contains a "\" (backslash)
> */
> -static int check_refname_component(const char *refname, int flags)
> +static int check_refname_component_1(const char *refname, int flags)
> {
> const char *cp;
> char last = '\0';
> @@ -47,7 +66,7 @@ static int check_refname_component(const char *refname, int
> flags)
> unsigned char disp = refname_disposition[ch];
> switch(disp) {
> case 1:
> - goto out;
> + return check_refname_component_trailer(cp, refname,
> flags);
> case 2:
> if (last == '.')
> return -1; /* Refname contains "..". */
> @@ -61,23 +80,86 @@ static int check_refname_component(const char *refname,
> int flags)
> }
> last = ch;
> }
> -out:
> - if (cp == refname)
> - return 0; /* Component has zero length. */
> - if (refname[0] == '.') {
> - if (!(flags & REFNAME_DOT_COMPONENT))
> - return -1; /* Component starts with '.'. */
> - /*
> - * Even if leading dots are allowed, don't allow "."
> - * as a component (".." is prevented by a rule above).
> - */
> - if (refname[1] == '\0')
> - return -1; /* Component equals ".". */
> +}
> +
> +#ifdef NO_SSE42
> +#define check_refname_component check_refname_component_1
> +#else
> +#define BLOCK_SIZE 16
Is this macro name safe? It feels a bit too generic/broad and
asking to collide with some system-defined block size constant
coming from random <*.h> header file, but maybe it's just me.
> +/* Vectorized version of check_refname_component */
> +static int check_refname_component(const char *refname, int flags)
> +{
> + const __m128i *refname_vec = (__m128i*) refname;
> +
> + /* Character ranges for characters forbidden in refs; see
> + * above */
> + static const __v16qi bad = {
> + 0x01, 0x20, 0x7e, 0x7f, 0x5e, 0x5e, 0x3a, 0x3a,
> + 0x5b, 0x5c, 0x2a, 0x2a, 0x3f, 0x3f, 0x3f, 0x3f};
> +
> + static const __v16qi nonslashes = {
> + '\001', '/' -1, '/' + 1, 0xff,
> + };
> +
> + static const __v16qi dotdot = {'.','.',0};
> + static const __v16qi atcurly = {'@','{',0};
s/,/, /g; please.
> + const __m128i *vp;
> + const char *cp = (const char *)refname_vec;
> +
> + int dotdotpos = BLOCK_SIZE, atcurlypos = BLOCK_SIZE;
> + for (vp = refname_vec; ; v
Re: [PATCH] receive-pack: optionally deny case-clone refs
David Turner writes: > I would be happy to add "case-clone" to the glossary -- would that be OK > with you? I do not immediately think of the better term. Somehow "case-clone" sounds strange, though X-<. >> (Mental note to the reviewer himself) This returns true iff there is >> an existing ref whose name is only different in case, and cause >> for-each-ref to return early with true. In a sane case of not >> receiving problematic refs, this will have to iterate over all the >> existing refnames. Wonder if there are better ways to optimize this >> in a repository with hundreds or thousands of refs, which is not all >> that uncommon. > > My expectation is that on average a push will involve a small number of > refs -- usually exactly one. It does not matter that _you_ push only one, because the number of existing refs at the receiving end is what determines how many times the for-each-ref loop spins, no? > Yes, but it's harder to test on case-insensitive filesystems because we > cannot have coexisting local case-clone branches. You do not have to (and you should not) do "git checkout -b" to create various local branches in the first place. For example: git send-pack ./victim HEAD^1:refs/heads/caseclone && test_must_fail git send-pack ./victim HEAD:refs/heads/CaseClone would let you push the parent of the current tip to caseclone and then attempt to push the current tip to CaseClone. If the receiving end could incorrectly fast-forwards caseclone, you found a bug ;-) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/9] Clarify two uses of remote.*.fetch
The early part of this has been sent to the list and this round contains updates based on review comments. The new patches at the end clarifies how remote.*.fetch configuration variables are used in two conceptually different ways. We would need a similar update for the "git push" side, which uses remote.*.push configuration variables in the same two different ways, but that is for a different week. Junio C Hamano (8): fetch doc: update introductory part for clarity fetch doc: update note on '+' in front of the refspec fetch doc: remove notes on outdated "mixed layout" fetch doc: on pulling multiple refspecs fetch doc: update refspec format description fetch doc: remove "short-cut" section fetch doc: add a section on configured remote-tracking branches fetch: allow explicit --refmap to override configuration Marc Branchaud (1): fetch doc: move FETCH_HEAD material lower and add an example Documentation/fetch-options.txt| 8 Documentation/git-fetch.txt| 87 -- Documentation/pull-fetch-param.txt | 58 + builtin/fetch.c| 35 +-- t/t5510-fetch.sh | 37 5 files changed, 171 insertions(+), 54 deletions(-) -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
What's cooking in git.git (Jun 2014, #01; Tue, 3)
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'. The first batch of topics, all of which have been cooking for quite a while on the 'next' branch, have been merged to 'master'. I'll - merge another batch to 'master', then - rewind the tip of 'next' in preparation to start accepting new topics sometime mid-next week. I'll also update tinyurl.com/gitCal for this cycle soonish. You can find the changes described here in the integration branches of the repositories listed at http://git-blame.blogspot.com/p/git-public-repositories.html -- [Graduated to "master"] * as/grep-fullname-config (2014-03-20) 1 commit (merged to 'next' on 2014-03-28 at 810a076) + grep: add grep.fullName config variable Add a configuration variable to force --full-name to be default for "git grep". This may cause regressions on scripted users that do not expect this new behaviour. * bg/strbuf-trim (2014-05-06) 2 commits (merged to 'next' on 2014-05-07 at 978f378) + api-strbuf.txt: add docs for _trim and _ltrim + strbuf: use _rtrim and _ltrim in strbuf_trim * dt/api-doc-setup-gently (2014-04-30) 1 commit (merged to 'next' on 2014-05-07 at 6054b08) + docs: document RUN_SETUP_GENTLY and clarify RUN_SETUP * ef/send-email-absolute-path-to-the-command (2014-04-23) 2 commits (merged to 'next' on 2014-04-23 at a657e5e) + send-email: windows drive prefix (e.g. C:) appears only at the beginning (merged to 'next' on 2014-04-21 at 43bebb5) + send-email: recognize absolute path on Windows * ep/shell-command-substitution (2014-04-30) 41 commits (merged to 'next' on 2014-05-07 at e9952c7) + t5000-tar-tree.sh: use the $( ... ) construct for command substitution + t4204-patch-id.sh: use the $( ... ) construct for command substitution + t4119-apply-config.sh: use the $( ... ) construct for command substitution + t4116-apply-reverse.sh: use the $( ... ) construct for command substitution + t4057-diff-combined-paths.sh: use the $( ... ) construct for command substitution + t4038-diff-combined.sh: use the $( ... ) construct for command substitution + t4036-format-patch-signer-mime.sh: use the $( ... ) construct for command substitution + t4014-format-patch.sh: use the $( ... ) construct for command substitution + t4013-diff-various.sh: use the $( ... ) construct for command substitution + t4012-diff-binary.sh: use the $( ... ) construct for command substitution + t4010-diff-pathspec.sh: use the $( ... ) construct for command substitution + t4006-diff-mode.sh: use the $( ... ) construct for command substitution + t3910-mac-os-precompose.sh: use the $( ... ) construct for command substitution + t3905-stash-include-untracked.sh: use the $( ... ) construct for command substitution + t1050-large.sh: use the $( ... ) construct for command substitution + t1020-subdirectory.sh: use the $( ... ) construct for command substitution + t1004-read-tree-m-u-wf.sh: use the $( ... ) construct for command substitution + t1003-read-tree-prefix.sh: use the $( ... ) construct for command substitution + t1002-read-tree-m-u-2way.sh: use the $( ... ) construct for command substitution + t1001-read-tree-m-2way.sh: use the $( ... ) construct for command substitution + t1000-read-tree-m-3way.sh: use the $( ... ) construct for command substitution + t0300-credentials.sh: use the $( ... ) construct for command substitution + t0030-stripspace.sh: use the $( ... ) construct for command substitution + t0026-eol-config.sh: use the $( ... ) construct for command substitution + t0025-crlf-auto.sh: use the $( ... ) construct for command substitution + t0020-crlf.sh: use the $( ... ) construct for command substitution + t0010-racy-git.sh: use the $( ... ) construct for command substitution + t0001-init.sh: use the $( ... ) construct for command substitution + p5302-pack-index.sh: use the $( ... ) construct for command substitution + lib-gpg.sh: use the $( ... ) construct for command substitution + lib-cvs.sh: use the $( ... ) construct for command substitution + lib-credential.sh: use the $( ... ) construct for command substitution + git-web--browse.sh: use the $( ... ) construct for command substitution + git-stash.sh: use the $( ... ) construct for command substitution + git-rebase.sh: use the $( ... ) construct for command substitution + git-rebase--merge.sh: use the $( ... ) construct for command substitution + git-pull.sh: use the $( ... ) construct for command substitution + appp.sh: use the $( ... ) construct for command substitution + t7900-subtree.sh: use the $( ... ) construct for command substitution + test-gitmw-lib.sh: use the $( ... ) construct for command substitution + t9365-continuing-queries.sh: use the $( ... ) construct for command substitution Adjust shell scripts to use $(cmd) instead of `cmd`. * ew/config-protect-mode (20
[PATCH v2 2/9] fetch doc: move FETCH_HEAD material lower and add an example
From: Marc Branchaud Signed-off-by: Marc Branchaud Signed-off-by: Junio C Hamano --- Documentation/git-fetch.txt | 29 + 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index 78fe948..06106b9 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -18,13 +18,9 @@ SYNOPSIS DESCRIPTION --- Fetch branches and/or tags (collectively, "refs") from one or more -other repositories, along with the objects necessary to complete -their histories. - -The names of refs that are fetched, together with the object names -they point at, are written to `.git/FETCH_HEAD`. This information -can be used to learn what was fetched. In addition, the remote-tracking -branches are updated (see description on below for details). +other repositories, along with the objects necessary to complete their +histories. Remote-tracking branches are updated (see the description +of below for ways to control this behavior). By default, any tag that points into the histories being fetched is also fetched; the effect is to fetch tags that @@ -34,7 +30,7 @@ configuring remote..tagopt. By using a refspec that fetches tags explicitly, you can fetch tags that do not point into branches you are interested in as well. -'git fetch' can fetch from either a single named repository, +'git fetch' can fetch from either a single named repository or URL, or from several repositories at once if is given and there is a remotes. entry in the configuration file. (See linkgit:git-config[1]). @@ -42,6 +38,10 @@ there is a remotes. entry in the configuration file. When no remote is specified, by default the `origin` remote will be used, unless there's an upstream branch configured for the current branch. +The names of refs that are fetched, together with the object names +they point at, are written to `.git/FETCH_HEAD`. This information +may be used by scripts or other git commands, such as linkgit:git-pull[1]. + OPTIONS --- include::fetch-options.txt[] @@ -78,6 +78,19 @@ the local repository by fetching from the branches (respectively) The `pu` branch will be updated even if it is does not fast-forward, because it is prefixed with a plus sign; `tmp` will not be. +* Peek at a remote's branch, without configuring the remote in your local +repository: ++ + +$ git fetch git://git.kernel.org/pub/scm/git/git.git maint +$ git log FETCH_HEAD + ++ +The first command fetches the `maint` branch from the repository at +`git://git.kernel.org/pub/scm/git/git.git` and the second command uses +`FETCH_HEAD` to examine the branch with linkgit:git-log[1]. The fetched +objects will eventually be removed by git's built-in housekeeping (see +linkgit:git-gc[1]). BUGS -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/9] fetch doc: update introductory part for clarity
- "Branches" is a more common way to say "heads" in these days. - Remote-tracking branches are used a lot more these days and it is worth mentioning that it is one of the primary side effects of the command to update them. - Avoid "X. That means Y." If Y is easier to understand to readers, just say that upfront. - Use of explicit refspec to fetch tags does not have much to do with turning "auto following" on or off. It is a way to fetch tags that otherwise would not be fetched by auto-following. Helped-by: Marc Branchaud Signed-off-by: Junio C Hamano --- Documentation/git-fetch.txt | 24 +--- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index 5809aa4..78fe948 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -17,20 +17,22 @@ SYNOPSIS DESCRIPTION --- -Fetches named heads or tags from one or more other repositories, -along with the objects necessary to complete them. +Fetch branches and/or tags (collectively, "refs") from one or more +other repositories, along with the objects necessary to complete +their histories. -The ref names and their object names of fetched refs are stored -in `.git/FETCH_HEAD`. This information is left for a later merge -operation done by 'git merge'. +The names of refs that are fetched, together with the object names +they point at, are written to `.git/FETCH_HEAD`. This information +can be used to learn what was fetched. In addition, the remote-tracking +branches are updated (see description on below for details). -By default, tags are auto-followed. This means that when fetching -from a remote, any tags on the remote that point to objects that exist -in the local repository are fetched. The effect is to fetch tags that +By default, any tag that points into the histories being fetched is +also fetched; the effect is to fetch tags that point at branches that you are interested in. This default behavior -can be changed by using the --tags or --no-tags options, by -configuring remote..tagopt, or by using a refspec that fetches -tags explicitly. +can be changed by using the --tags or --no-tags options or by +configuring remote..tagopt. By using a refspec that fetches tags +explicitly, you can fetch tags that do not point into branches you +are interested in as well. 'git fetch' can fetch from either a single named repository, or from several repositories at once if is given and -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 4/9] fetch doc: remove notes on outdated "mixed layout"
In old days before Git 1.5, it was customery for "git fetch" to use the same local branch namespace to keep track of the remote-tracking branches, and it was necessary to tell users not to check them out and commit on them. Since everybody uses the separate remote layout these days, there is no need to warn against the practice to check out the right-hand side of and build on it---the RHS is typically not even a local branch. Incidentally, this also kills one mention of "Pull:" line of $GIT_DIR/remotes/* configuration, which is a lot less familiar to new people than the more modern remote.*.fetch configuration variable. Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 13 - 1 file changed, 13 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 41474c5..40f8687 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -36,19 +36,6 @@ in a repository with this behavior; the pulling user simply must know this is the expected usage pattern for a branch. + [NOTE] -You never do your own development on branches that appear -on the right hand side of a colon on `Pull:` lines; -they are to be updated by 'git fetch'. If you intend to do -development derived from a remote branch `B`, have a `Pull:` -line to track it (i.e. `Pull: B:remote-B`), and have a separate -branch `my-B` to do your development on top of it. The latter -is created by `git branch my-B remote-B` (or its equivalent `git -checkout -b my-B remote-B`). Run `git fetch` to keep track of -the progress of the remote side, and when you see something new -on the remote branch, merge it into your development branch with -`git pull . remote-B`, while you are on `my-B` branch. -+ -[NOTE] There is a difference between listing multiple directly on 'git pull' command line and having multiple `Pull:` lines for a and running -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/9] fetch doc: update note on '+' in front of the refspec
While it is not *wrong* per-se to say that pulling a rewound/rebased branch will lead to an unnecessary merge conflict, that is not what the leading "+" sign to allow non-fast-forward update of remote-tracking branch is at all. Helped-by: Marc Branchaud Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 18cffc2..41474c5 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -24,15 +24,15 @@ is updated even if it does not result in a fast-forward update. + [NOTE] -If the remote branch from which you want to pull is -modified in non-linear ways such as being rewound and -rebased frequently, then a pull will attempt a merge with -an older version of itself, likely conflict, and fail. -It is under these conditions that you would want to use -the `+` sign to indicate non-fast-forward updates will -be needed. There is currently no easy way to determine -or declare that a branch will be made available in a -repository with this behavior; the pulling user simply +When the remote branch you want to fetch is known to +be rewound and rebased regularly, it is expected that +its new tip will not be descendant of its previous tip +(as stored in your remote-tracking branch the last time +you fetched). You would want +to use the `+` sign to indicate non-fast-forward updates +will be needed for such branches. There is no way to +determine or declare that a branch will be made available +in a repository with this behavior; the pulling user simply must know this is the expected usage pattern for a branch. + [NOTE] -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 9/9] fetch: allow explicit --refmap to override configuration
Since the introduction of opportunisitic updates of remote-tracking
branches, started at around f2690487 (fetch: opportunistically
update tracking refs, 2013-05-11) with a few updates in v1.8.4 era,
the remote.*.fetch configuration always kicks in even when a refspec
to specify what to fetch is given on the command line, and there is
no way to disable or override it per-invocation.
Teach the command to pay attention to the --refmap=:
command-line options that can be used to override the use of
configured remote.*.fetch as the refmap.
Signed-off-by: Junio C Hamano
---
---
Documentation/fetch-options.txt | 8
Documentation/git-fetch.txt | 3 +++
builtin/fetch.c | 35 ---
t/t5510-fetch.sh| 37 +
4 files changed, 80 insertions(+), 3 deletions(-)
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 92c68c3..d5c6289 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -72,6 +72,14 @@ endif::git-pull[]
setting. See linkgit:git-config[1].
ifndef::git-pull[]
+--refmap=::
+ When fetching refs listed on the command line, use the
+ specified refspec (can be given more than once) to map the
+ refs to remote-tracking branches, instead of values of
+ `remote.*.fetch` configuration variable for the remote
+ repository. See section on "Configured Remote-tracking
+ Branches" for details.
+
-t::
--tags::
Fetch all tags from the remote (i.e., fetch remote tags
diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt
index d09736a..96c44f9 100644
--- a/Documentation/git-fetch.txt
+++ b/Documentation/git-fetch.txt
@@ -93,6 +93,9 @@ This configuration is used in two ways:
only used to decide _where_ the refs that are fetched are stored
by acting as a mapping.
+The latter use of the configured values can be overridden by giving
+the `--refmap=` parameter(s) on the command line.
+
EXAMPLES
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 55f457c..dd46b61 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -45,6 +45,8 @@ static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
static int shown_url = 0;
+static int refmap_alloc, refmap_nr;
+static const char **refmap_array;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -69,6 +71,19 @@ static int git_fetch_config(const char *k, const char *v,
void *cb)
return 0;
}
+static int parse_refmap_arg(const struct option *opt, const char *arg, int
unset)
+{
+ ALLOC_GROW(refmap_array, refmap_nr + 1, refmap_alloc);
+
+ /*
+* "git fetch --refmap='' origin foo"
+* can be used to tell the command not to store anywhere
+*/
+ if (*arg)
+ refmap_array[refmap_nr++] = arg;
+ return 0;
+}
+
static struct option builtin_fetch_options[] = {
OPT__VERBOSITY(&verbosity),
OPT_BOOL(0, "all", &all,
@@ -107,6 +122,8 @@ static struct option builtin_fetch_options[] = {
N_("default mode for recursion"), PARSE_OPT_HIDDEN },
OPT_BOOL(0, "update-shallow", &update_shallow,
N_("accept refs that update .git/shallow")),
+ { OPTION_CALLBACK, 0, "refmap", NULL, N_("refmap"),
+ N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg },
OPT_END()
};
@@ -278,6 +295,9 @@ static struct ref *get_ref_map(struct transport *transport,
const struct ref *remote_refs = transport_get_remote_refs(transport);
if (refspec_count) {
+ struct refspec *fetch_refspec;
+ int fetch_refspec_nr;
+
for (i = 0; i < refspec_count; i++) {
get_fetch_map(remote_refs, &refspecs[i], &tail, 0);
if (refspecs[i].dst && refspecs[i].dst[0])
@@ -307,12 +327,21 @@ static struct ref *get_ref_map(struct transport
*transport,
* by ref_remove_duplicates() in favor of one of these
* opportunistic entries with FETCH_HEAD_IGNORE.
*/
- for (i = 0; i < transport->remote->fetch_refspec_nr; i++)
- get_fetch_map(ref_map, &transport->remote->fetch[i],
- &oref_tail, 1);
+ if (refmap_array) {
+ fetch_refspec = parse_fetch_refspec(refmap_nr,
refmap_array);
+ fetch_refspec_nr = refmap_nr;
+ } else {
+ fetch_refspec = transport->remote->fetch;
+ fetch_refspec_nr = transport->remote->fetch_refspec_nr;
+ }
+
+ for (i = 0; i < fetch_refspec_nr; i++)
+ get_fetch_map(ref_map, &fetch_refspe
[PATCH v2 5/9] fetch doc: on pulling multiple refspecs
Replace desription of old-style "Pull:" lines in remotes/ configuration with modern remote.*.fetch variables. As this note applies only to "git pull", enable it only in git-pull manual page. Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 40f8687..25af2ce 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -34,22 +34,26 @@ will be needed for such branches. There is no way to determine or declare that a branch will be made available in a repository with this behavior; the pulling user simply must know this is the expected usage pattern for a branch. +ifdef::git-pull[] + [NOTE] There is a difference between listing multiple directly on 'git pull' command line and having multiple -`Pull:` lines for a and running +`remote..fetch` entries in your configuration +for a and running 'git pull' command without any explicit parameters. listed explicitly on the command line are always merged into the current branch after fetching. In other words, if you list more than one remote refs, you would be making -an Octopus. While 'git pull' run without any explicit -parameter takes default s from `Pull:` lines, it +an Octopus merge. On the other hand, 'git pull' that is run +without any explicit parameter takes default +s from `remote..fetch` configuration, it merges only the first found into the current branch, -after fetching all the remote refs. This is because making an +after fetching all the remote refs specified. This is because making an Octopus from remote refs is rarely done, while keeping track of multiple remote heads in one-go by fetching more than one is often useful. +endif::git-pull[] + Some short-cut notations are also supported. + -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 8/9] fetch doc: add a section on configured remote-tracking branches
To resurrect a misleading mention removed in the previous step, add a section to explain how the remote-tracking configuration interacts with the refspecs given as the command-line arguments. Signed-off-by: Junio C Hamano --- Documentation/git-fetch.txt | 43 +++ 1 file changed, 43 insertions(+) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index 06106b9..d09736a 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -51,6 +51,49 @@ include::pull-fetch-param.txt[] include::urls-remotes.txt[] +CONFIGURED REMOTE-TRACKING BRANCHES +--- + +You would often interact with the same remote repository by +regularly and repeatedly fetching from it. In order to keep track +of the progress of such a remote repository, `git fetch` allows you +to configure `remote..fetch` configuration variable. + +Typically such a variable may look like this: + + +[remote "origin"] + fetch = +refs/heads/*:refs/remotes/origin/* + + +This configuration is used in two ways: + +* When `git fetch` command is run without specifying what branches + and/or tags to fetch on the command line, e.g. `git fetch origin` + or `git fetch`, the values configured to this variable are used as + the refspecs to be used to fetch. The example above will fetch + all branches that exist on the `origin` (i.e. any ref that matches + the left-hand side of the value, `refs/heads/*`) and update the + corresponding remote-tracking branches in `refs/remotes/origin/*` + hierarchy. + +* When `git fetch` command is run with explicit branches and/or tags + to fetch on the command line, e.g. `git fetch origin master`, the + given on the command line (e.g. `master` in the example, + which is a short-hand for `master:`, which in turn would mean + "fetch the 'master' branch but I do not explicitly say what + remote-tracking branch to update with it from the command line") + determines what are to be fetched, and the example command will + fetch _only_ the 'master' branch. The values of the variable are + used to map the branch (i.e. `master`) to determine which + remote-tracking branch, if any, is updated. When used in this + way, the values of the configuration variable do not have any + effect in deciding _what_ gets fetched (i.e. the values are not + used as refspecs when the command-line lists refspecs); they are + only used to decide _where_ the refs that are fetched are stored + by acting as a mapping. + + EXAMPLES -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 6/9] fetch doc: update refspec format description
The text made it sound as if the leading plus is the only thing that is optional, and forgot that is the same as :, i.e. fetch it and do not store anywhere. Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 25af2ce..a090d4a 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -15,6 +15,7 @@ endif::git-pull[] The format of a parameter is an optional plus `+`, followed by the source ref , followed by a colon `:`, followed by the destination ref . + The colon can be omitted when is empty. + The remote ref that matches is fetched, and if is not empty string, the local -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 7/9] fetch doc: remove "short-cut" section
It is misleading to mention that that does not store is to fetch the ref into FETCH_HEAD, because a refspec that does store is also to fetch the LHS into FETCH_HEAD. It is doubly misleading to list it as part of "short-cut". stands for a refspec that has it on the LHS with a colon and an empty RHS, and that definition should be given at the beginning of the entry where the format is defined. Tentatively remove this misleading description, which leaves the `tag ` as the only true short-hand, so move it at the beginning of the entry. Signed-off-by: Junio C Hamano --- Documentation/pull-fetch-param.txt | 16 +++- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index a090d4a..9e62434 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -17,6 +17,9 @@ endif::git-pull[] by a colon `:`, followed by the destination ref . The colon can be omitted when is empty. + +`tag ` means the same as `refs/tags/:refs/tags/`; +it requests fetching everything up to the given tag. ++ The remote ref that matches is fetched, and if is not empty string, the local ref that matches it is fast-forwarded using . @@ -55,16 +58,3 @@ Octopus from remote refs is rarely done, while keeping track of multiple remote heads in one-go by fetching more than one is often useful. endif::git-pull[] -+ -Some short-cut notations are also supported. -+ -* `tag ` means the same as `refs/tags/:refs/tags/`; - it requests fetching everything up to the given tag. -ifndef::git-pull[] -* A parameter without a colon fetches that ref into FETCH_HEAD, -endif::git-pull[] -ifdef::git-pull[] -* A parameter without a colon merges into the current - branch, -endif::git-pull[] - and updates the remote-tracking branches (if any). -- 2.0.0-511-g1433423 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v13 00/41] Use ref transactions
Ronnie Sahlberg wrote:
> This patch series can also be found at
> https://github.com/rsahlberg/git/tree/ref-transactions
Applies with two minor conflicts on top of a merge of
mh/ref-transaction, rs/ref-update-check-errors-early, and
rs/reflog-exists. Previously:
motivation: http://thread.gmane.org/gmane.comp.version-control.git/246255
v2 http://thread.gmane.org/gmane.comp.version-control.git/246662,
http://thread.gmane.org/gmane.comp.version-control.git/246763
v3 http://thread.gmane.org/gmane.comp.version-control.git/247056
v6 http://thread.gmane.org/gmane.comp.version-control.git/247876
v8 http://thread.gmane.org/gmane.comp.version-control.git/249159
v10 http://thread.gmane.org/gmane.comp.version-control.git/249356
v11 http://thread.gmane.org/gmane.comp.version-control.git/250197
v12 http://thread.gmane.org/gmane.comp.version-control.git/250377
Interdiff against v8:
branch.c | 7 +-
builtin/commit.c | 4 +-
builtin/fetch.c| 41 ++---
builtin/receive-pack.c | 34 +++--
builtin/replace.c | 6 +-
builtin/tag.c | 6 +-
builtin/update-ref.c | 20 +--
cache.h| 2 +
fast-import.c | 37 +++--
lockfile.c | 37 +++--
refs.c | 396 ++---
refs.h | 82 +++---
sequencer.c| 10 +-
t/t3200-branch.sh | 6 +-
walker.c | 41 +++--
15 files changed, 458 insertions(+), 271 deletions(-)
diff --git a/branch.c b/branch.c
index 74d55e7..e0439af 100644
--- a/branch.c
+++ b/branch.c
@@ -298,13 +298,12 @@ void create_branch(const char *head,
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
- transaction = ref_transaction_begin();
+ transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, ref.buf, sha1,
- null_sha1, 0, !forcing, msg) ||
+ null_sha1, 0, !forcing, msg, &err) ||
ref_transaction_commit(transaction, &err))
- die_errno(_("%s: failed to write ref: %s"),
- ref.buf, err.buf);
+ die("%s", err.buf);
ref_transaction_free(transaction);
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 07ccc97..e35877c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1676,12 +1676,12 @@ int cmd_commit(int argc, const char **argv, const char
*prefix)
strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));
strbuf_insert(&sb, strlen(reflog_msg), ": ", 2);
- transaction = ref_transaction_begin();
+ transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_update(transaction, "HEAD", sha1,
current_head ?
current_head->object.sha1 : NULL,
- 0, !!current_head, sb.buf) ||
+ 0, !!current_head, sb.buf, &err) ||
ref_transaction_commit(transaction, &err)) {
rollback_index_files();
die("%s", err.buf);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 3a849b0..52f1ebc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -44,8 +44,7 @@ static struct transport *gtransport;
static struct transport *gsecondary;
static const char *submodule_prefix = "";
static const char *recurse_submodules_default;
-static int shown_url;
-struct ref_transaction *transaction;
+static int shown_url = 0;
static int option_parse_recurse_submodules(const struct option *opt,
const char *arg, int unset)
@@ -376,6 +375,9 @@ static int s_update_ref(const char *action,
{
char msg[1024];
char *rla = getenv("GIT_REFLOG_ACTION");
+ struct ref_transaction *transaction;
+ struct strbuf err = STRBUF_INIT;
+ int ret, df_conflict = 0;
if (dry_run)
return 0;
@@ -383,11 +385,26 @@ static int s_update_ref(const char *action,
rla = default_rla.buf;
snprintf(msg, sizeof(msg), "%s: %s", rla, action);
- if (ref_transaction_update(transaction, ref->name, ref->new_sha1,
- ref->old_sha1, 0, check_old, msg))
- return STORE_REF_ERROR_OTHER;
+ transaction = ref_transaction_begin(&err);
+ if (!transaction ||
+ ref_transaction_update(transaction, ref->name, ref->new_sha1,
+ ref->old_sha1, 0, check_old, msg, &err))
+ goto fail;
+ ret = ref_transaction_commit(transaction, &err);
+ if (ret == UPDATE_REFS_NAME_CONFLICT)
+ df_conflict = 1;
+ if (ret)
+ g
Re: Paper cut bug: Why isn't "git clone xxxx" recursive by default?
That is good to hear. I would be pretty happy about that. ^.^ Obviously any major changes will need to be done carefully. I was thinking of the way that you guys introduced new defaults for Git 2.0, phasing them in slowly through the 1.x cycle. Maybe I can get my hopes up for Git 3.0 --- 9 years from now :P On Tue, Jun 3, 2014 at 4:05 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >> Mara Kim writes: >> >>> Apologies if this question has been asked already, but what is the >>> reasoning behind making git clone not recursive (--recursive) by >>> default? >> >> The primary reason why submodules are separate repositories is not >> to require people to have everything. Some people want recursive, >> some others don't, and the world is not always "majority wins" (not >> that I am saying that majority will want recursive). >> >> Inertia, aka backward compatibility and not surprising existing >> users, plays some role when deciding the default. >> >> Also, going --recursive when the user did not want is a lot more >> expensive mistake to fix than not being --recursive when the user >> wanted to. > > Having said all that, I do not mean to say that I am opposed to > introduce some mechanism to let the users express their preference > between recursive and non-recursive better, so that "git clone" > without an explicit --recursive (or --no-recursive) can work to > their taste. A configuration in $HOME/.gitconfig might be a place > to start, even though that has the downside of assuming that the > given user would want to use the same settings for all his projects, > which may not be the case in practice. > -- Mara Kim Ph.D. Candidate Computational Biology Vanderbilt University Nashville, TN -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: What's cooking in git.git (Jun 2014, #01; Tue, 3)
On 2014-06-03 18:16, Junio C Hamano wrote: > * rh/prompt-tests (2014-05-30) 10 commits > - t9904: new __git_ps1 tests for Zsh > - lib-prompt-tests.sh: add variable for string that encodes percent in PS1 > - lib-prompt-tests.sh: put all tests inside a function > - t9903: move prompt tests to a new lib-prompt-tests.sh file > - t9903: move PS1 color code variable definitions to lib-bash.sh > - t9903: include "Bash" in test names via new $shellname var > - t9903: run pc mode tests again with PS1 expansion disabled > - t9903: move test name prefix to a separate variable > - t9903: put the Bash pc mode prompt test cases in a function > - t9903: remove Zsh test from the suite of Bash prompt tests > > Will merge to 'next'. Please hold off on merging -- I just discovered some bugs while conversing with the zsh developers about some zsh shell emulation stuff I didn't fully understand. (Surprisingly, the tests in that patch series are actually run in zsh's sh emulation mode.) Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] git-ack: record an ack
Hi Michael,
I have some inline comments below. Also, some parts of the patch do not
adhere to the style rules
- tabs for indentation
- $(...) for command substitution
- no space after redirection operators
- double-quotes around redirection targets
for shell scripts (from the file `Documentation/CodingGuidelines`).
On 05/18/2014 11:17 PM, Michael S. Tsirkin wrote:
> diff --git a/contrib/git-ack b/contrib/git-ack
> new file mode 100755
> index 000..4aeb16a
> --- /dev/null
> +++ b/contrib/git-ack
> @@ -0,0 +1,84 @@
> +msg=`mktemp`
> +patch=`mktemp`
> +info=`git mailinfo $msg $patch`
> +subj=`echo "$info"|sed -n 's/^Subject: //p'`
> +author=`echo "$info"|sed -n 's/^Author: //p'`
> +email=`echo "$info"|sed -n 's/^Email: //p'`
> +auth="$author <$email>"
> +date=`echo "$info"|sed -n 's/^Date: //p'`
> +sign=`mktemp`
> +echo "ack! $subj" > $sign
> +echo "" >> $sign
> +if
> +git diff --cached HEAD
If I am not mistaken, the exit code of `git-diff(1)` doesn't change
according to whether there are differences or not, unless the option
`--exit-code` is given.
> +then
> +nop < /dev/null
Is it correct that this is a do-nothing operation? Is that a common
idiom? I found the null command (`:`, colon) to be used in many places
instead.
> +else
> +echo "DIFF in cache. Not acked, reset or commit!"
> +exit 1
> +fi
> +GIT_DIR=`pwd`/${GIT_DIR}
This seems incorrect to me. If `GIT_DIR` is already set, it might point
to an absolute path and not `.git`. If the environment variable is not
set, the state file `ACKS` ends up in the working directory.
Maybe `git-sh-setup(1)` can be of help. It uses
git rev-parse --git-dir
to probe the path to the .git directory.
> +
> +usage () {
> + echo "Usage: git ack " \
> +"[-s|--save|-a|--append|-r|--restore |-c|--clear]\n" >& 2;
> +exit 1;
> +}
> +
> +append=
> +save=
> +clear=
The restore flag seems to be missing from this list of declarations.
> +
> +while test $# != 0
> +do
> + case "$1" in
> + -a|--append)
> + append="y"
> + ;;
> + -s|--s)
> + save="y"
> + ;;
> + -r|--restore)
> + restore="y"
> + ;;
> + -c|--clear)
> + clear="y"
> +;;
> + *)
> + usage ;;
> + esac
> + shift
> +done
> +
> +if
> +test "$clear"
> +then
> +rm -f "${GIT_DIR}/ACKS"
> +fi
> +
> +if
> +test "$save"
> +then
> +if
> +test "$append"
> +then
> +cat $msg >> "${GIT_DIR}/ACKS"
> +else
> +cat $msg > "${GIT_DIR}/ACKS"
> +fi
> +fi
> +
> +if
> +test "$restore"
> +then
> +msg = ${GIT_DIR}/ACKS
> +fi
> +
> +if
> +grep '^[A-Z][A-Za-z-]*-by:' $msg >> $sign
> +then
> +git commit --allow-empty -F $sign --author="$auth" --date="$date"
> +else
> +echo "No signature found!"
> +exit 2
> +fi
Fabian
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Kitchen Sale Stirlingshire
This Forum is probably the best forum that i have ever used and i would just like to say how proud i am to be a member of this forum -- View this message in context: http://git.661346.n2.nabble.com/Kitchen-Sale-Stirlingshire-tp7612292.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] receive-pack: optionally deny case-clone refs
On Tue, 2014-06-03 at 15:13 -0700, Junio C Hamano wrote: > David Turner writes: > > > I would be happy to add "case-clone" to the glossary -- would that be OK > > with you? I do not immediately think of the better term. > > Somehow "case-clone" sounds strange, though X-<. Case clones case clones roly poly case clones; case clones case clones eat them up yum. Yeah, I wish I could think of a name that did not call to mind Tatiana Maslany, but unfortunately, that is all I can think of. At least it's easy to search for. > >> (Mental note to the reviewer himself) This returns true iff there is > >> an existing ref whose name is only different in case, and cause > >> for-each-ref to return early with true. In a sane case of not > >> receiving problematic refs, this will have to iterate over all the > >> existing refnames. Wonder if there are better ways to optimize this > >> in a repository with hundreds or thousands of refs, which is not all > >> that uncommon. > > > > My expectation is that on average a push will involve a small number of > > refs -- usually exactly one. > > It does not matter that _you_ push only one, because the number of > existing refs at the receiving end is what determines how many times > the for-each-ref loop spins, no? Yes, but that loop is an inner loop; so the total cost is O(refs pushed * existing refs). An in-memory hashmap would be O(existing refs) with a higher constant factor. An on-disk hashmap would probably scale best, but a fair amount more work. > > Yes, but it's harder to test on case-insensitive filesystems because we > > cannot have coexisting local case-clone branches. > > You do not have to (and you should not) do "git checkout -b" to > create various local branches in the first place. For example: > > git send-pack ./victim HEAD^1:refs/heads/caseclone && > test_must_fail git send-pack ./victim HEAD:refs/heads/CaseClone > > would let you push the parent of the current tip to caseclone and > then attempt to push the current tip to CaseClone. If the receiving > end could incorrectly fast-forwards caseclone, you found a bug ;-) Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] receive-pack: optionally deny case clone refs
It is possible to have two branches which are the same but for case.
This works great on the case-sensitive filesystems, but not so well on
case-insensitive filesystems. It is fairly typical to have
case-insensitive clients (Macs, say) with a case-sensitive server
(GNU/Linux).
Should a user attempt to pull on a Mac when there are case clone
branches with differing contents, they'll get an error message
containing something like "Ref refs/remotes/origin/lower is at
[sha-of-lowercase-branch] but expected [sha-of-uppercase-branch]
(unable to update local ref)"
With a case-insensitive git server, if a branch called capital-M
Master (that differs from lowercase-m-master) is pushed, nobody else
can push to (lowercase-m) master until the branch is removed.
Create the option receive.denycaseclonebranches, which checks pushed
branches to ensure that they are not case clones of an existing
branch. This setting is turned on by default if core.ignorecase is
set, but not otherwise.
Signed-off-by: David Turner
---
Documentation/config.txt | 6 ++
Documentation/git-push.txt | 5 +++--
Documentation/glossary-content.txt | 5 +
builtin/receive-pack.c | 27 +++-
t/t5400-send-pack.sh | 43 ++
5 files changed, 83 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1932e9b..4deddf8 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2053,6 +2053,12 @@ receive.unpackLimit::
especially on slow filesystems. If not set, the value of
`transfer.unpackLimit` is used instead.
+receive.denyCaseCloneBranches::
+ If set to true, git-receive-pack will deny a ref update that creates
+ a ref which is the same but for case as an existing ref. This is
+ useful when clients are on a case-insensitive filesystem, which
+ will cause errors when given refs which differ only in case.
+
receive.denyDeletes::
If set to true, git-receive-pack will deny a ref update that deletes
the ref. Use this to prevent such a ref deletion via a push.
diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index 21cd455..0786fb3 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -323,8 +323,9 @@ remote rejected::
of the following safety options in effect:
`receive.denyCurrentBranch` (for pushes to the checked out
branch), `receive.denyNonFastForwards` (for forced
- non-fast-forward updates), `receive.denyDeletes` or
- `receive.denyDeleteCurrent`. See linkgit:git-config[1].
+ non-fast-forward updates), `receive.denyDeletes`,
+ `receive.denyCaseCloneBranches` or `receive.denyDeleteCurrent`.
+ See linkgit:git-config[1].
remote failure::
The remote end did not report the successful update of the ref,
diff --git a/Documentation/glossary-content.txt
b/Documentation/glossary-content.txt
index be0858c..ed5ac23 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -31,6 +31,11 @@
[[def_cache]]cache::
Obsolete for: <>.
+[[def_case_clone]]case clone::
+ Two entities (e.g. filenames or refs) that differ only in case.
+ These can cause problems on case-insensitive filesystems, and
+ Git has machinery to prevent these problems in various cases.
+
[[def_chain]]chain::
A list of objects, where each <> in the list contains
a reference to its successor (for example, the successor of a
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c323081..4df99c5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -27,6 +27,7 @@ enum deny_action {
static int deny_deletes;
static int deny_non_fast_forwards;
+static int deny_case_clone_branches = DENY_UNCONFIGURED;
static enum deny_action deny_current_branch = DENY_UNCONFIGURED;
static enum deny_action deny_delete_current = DENY_UNCONFIGURED;
static int receive_fsck_objects = -1;
@@ -69,6 +70,11 @@ static int receive_pack_config(const char *var, const char
*value, void *cb)
if (status)
return status;
+ if (strcmp(var, "receive.denycaseclonebranches") == 0) {
+ deny_case_clone_branches = parse_deny_action(var, value);
+ return 0;
+ }
+
if (strcmp(var, "receive.denydeletes") == 0) {
deny_deletes = git_config_bool(var, value);
return 0;
@@ -468,6 +474,22 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
+static int is_case_clone(const char *refname, const unsigned char *sha1,
+ int flags, void *cb_data)
+{
+ const char *incoming_refname = cb_data;
+ return !strcasecmp(refname, incoming_refname) &&
+ strcmp(refname, incoming_refname);
+}
+
+static int ref_is_denied_case_cl
Re: [PATCH v5 2/2] refs.c: SSE4.2 optimizations for check_refname_component
On Tue, 2014-06-03 at 15:05 -0700, Junio C Hamano wrote: > > + if (((uintptr_t) vp & (PAGE_SIZE - 1)) > PAGE_SIZE - BLOCK_SIZE) > > + /* End-of-page; fall back to slow method for > > +* this entire component. */ > > + return check_refname_component_1(refname, flags); > > It is somewhat sad that we have to redo the whole thing, but nobody > higher in the callchain knows how long the refname will be before > calling us, so this cannot be avoided. It actually could be avoided; we could pass in some extra args that let check_refname_component_1 start checking after the part that we have already checked. I decided not to do this because I believe that the average refname component is short, which means that the cost is not too high and that it won't happen very often. But I would be willing to change this if you would like. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 1/2] refs.c: optimize check_refname_component()
In a repository with many refs, check_refname_component can be a major
contributor to the runtime of some git commands. One such command is
git rev-parse HEAD
Timings for one particular repo, with about 60k refs, almost all
packed, are:
Old: 35 ms
New: 29 ms
Many other commands which read refs are also sped up.
Signed-off-by: David Turner
---
refs.c | 67 +++---
t/t5511-refspec.sh | 6 -
2 files changed, 44 insertions(+), 29 deletions(-)
diff --git a/refs.c b/refs.c
index 28d5eca..46139d2 100644
--- a/refs.c
+++ b/refs.c
@@ -6,8 +6,29 @@
#include "string-list.h"
/*
- * Make sure "ref" is something reasonable to have under ".git/refs/";
- * We do not like it if:
+ * How to handle various characters in refnames:
+ * 0: An acceptable character for refs
+ * 1: End-of-component
+ * 2: ., look for a preceding . to reject .. in refs
+ * 3: {, look for a preceding @ to reject @{ in refs
+ * 4: A bad character: ASCII control characters, "~", "^", ":" or SP
+ */
+static unsigned char refname_disposition[256] = {
+ 1, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
+ 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4, 4,
+ 4, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 2, 1,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 0, 0, 0, 0, 4,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 4, 4, 0, 4, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
+ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
+};
+
+/*
+ * Try to read one refname component from the front of refname.
+ * Return the length of the component found, or -1 if the component is
+ * not legal. It is legal if it is something reasonable to have under
+ * ".git/refs/"; We do not like it if:
*
* - any path component of it begins with ".", or
* - it has double dots "..", or
@@ -16,41 +37,31 @@
* - it ends with ".lock"
* - it contains a "\" (backslash)
*/
-
-/* Return true iff ch is not allowed in reference names. */
-static inline int bad_ref_char(int ch)
-{
- if (((unsigned) ch) <= ' ' || ch == 0x7f ||
- ch == '~' || ch == '^' || ch == ':' || ch == '\\')
- return 1;
- /* 2.13 Pattern Matching Notation */
- if (ch == '*' || ch == '?' || ch == '[') /* Unsupported */
- return 1;
- return 0;
-}
-
-/*
- * Try to read one refname component from the front of refname. Return
- * the length of the component found, or -1 if the component is not
- * legal.
- */
static int check_refname_component(const char *refname, int flags)
{
const char *cp;
char last = '\0';
for (cp = refname; ; cp++) {
- char ch = *cp;
- if (ch == '\0' || ch == '/')
+ int ch = *cp & 255;
+ unsigned char disp = refname_disposition[ch];
+ switch(disp) {
+ case 1:
+ goto out;
+ case 2:
+ if (last == '.')
+ return -1; /* Refname contains "..". */
+ break;
+ case 3:
+ if (last == '@')
+ return -1; /* Refname contains "@{". */
break;
- if (bad_ref_char(ch))
- return -1; /* Illegal character in refname. */
- if (last == '.' && ch == '.')
- return -1; /* Refname contains "..". */
- if (last == '@' && ch == '{')
- return -1; /* Refname contains "@{". */
+ case 4:
+ return -1;
+ }
last = ch;
}
+out:
if (cp == refname)
return 0; /* Component has zero length. */
if (refname[0] == '.') {
diff --git a/t/t5511-refspec.sh b/t/t5511-refspec.sh
index c289322..de6db86 100755
--- a/t/t5511-refspec.sh
+++ b/t/t5511-refspec.sh
@@ -5,7 +5,6 @@ test_description='refspec parsing'
. ./test-lib.sh
test_refspec () {
-
kind=$1 refspec=$2 expect=$3
git config remote.frotz.url "." &&
git config --remove-section remote.frotz &&
@@ -84,4 +83,9 @@ test_refspec push
'refs/heads/*/*/for-linus:refs/remotes/mine/*' invalid
test_refspec fetch 'refs/heads/*/for-linus:refs/remotes/mine/*'
test_refspec push 'refs/heads/*/for-linus:refs/remotes/mine/*'
+good=$(printf '\303\204')
+test_refspec fetch "refs/heads/${good}"
+bad=$(printf '\011tab')
+test_refspec fetch "refs/heads/${bad}" invalid
+
test_done
--
2.0.0.rc1.18.gf763c0f
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v6 2/2] refs.c: SSE4.2 optimizations for check_refname_component
Optimize check_refname_component using SSE4.2, where available.
git rev-parse HEAD is a good test-case for this, since it does almost
nothing except parse refs. For one particular repo with about 60k
refs, almost all packed, the timings are:
Look up table: 29 ms
SSE4.2:25 ms
This is about a 15% improvement.
The configure.ac changes include code from the GNU C Library written
by Joseph S. Myers .
Signed-off-by: David Turner
---
Makefile | 6 +++
aclocal.m4 | 6 +++
configure.ac | 17
git-compat-util.h | 22 ++
refs.c | 117 ++---
t/t5511-refspec.sh | 13 ++
6 files changed, 166 insertions(+), 15 deletions(-)
diff --git a/Makefile b/Makefile
index a53f3a8..dd2127a 100644
--- a/Makefile
+++ b/Makefile
@@ -1326,6 +1326,11 @@ else
COMPAT_OBJS += compat/win32mmap.o
endif
endif
+ifdef NO_SSE42
+ BASIC_CFLAGS += -DNO_SSE42
+else
+ BASIC_CFLAGS += -msse4.2
+endif
ifdef OBJECT_CREATION_USES_RENAMES
COMPAT_CFLAGS += -DOBJECT_CREATION_MODE=1
endif
@@ -2199,6 +2204,7 @@ GIT-BUILD-OPTIONS: FORCE
@echo NO_PERL=\''$(subst ','\'',$(subst ','\'',$(NO_PERL)))'\' >>$@
@echo NO_PYTHON=\''$(subst ','\'',$(subst ','\'',$(NO_PYTHON)))'\' >>$@
@echo NO_UNIX_SOCKETS=\''$(subst ','\'',$(subst
','\'',$(NO_UNIX_SOCKETS)))'\' >>$@
+ @echo NO_SSE42=\''$(subst ','\'',$(subst ','\'',$(NO_SSE42)))'\' >>$@
ifdef TEST_OUTPUT_DIRECTORY
@echo TEST_OUTPUT_DIRECTORY=\''$(subst ','\'',$(subst
','\'',$(TEST_OUTPUT_DIRECTORY)))'\' >>$@
endif
diff --git a/aclocal.m4 b/aclocal.m4
index f11bc7e..d9f3f19 100644
--- a/aclocal.m4
+++ b/aclocal.m4
@@ -38,3 +38,9 @@ AC_DEFUN([TYPE_SOCKLEN_T],
[#include
#include ])
])
+
+dnl Test a compiler option or options with an empty input file.
+dnl LIBC_TRY_CC_OPTION([options], [action-if-true], [action-if-false])
+AC_DEFUN([LIBC_TRY_CC_OPTION],
+[AS_IF([AC_TRY_COMMAND([${CC-cc} $1 -xc /dev/null -S -o /dev/null])],
+ [$2], [$3])])
diff --git a/configure.ac b/configure.ac
index b711254..3a5bda9 100644
--- a/configure.ac
+++ b/configure.ac
@@ -382,6 +382,23 @@ AS_HELP_STRING([],[Tcl/Tk interpreter will be found in a
system.]),
GIT_PARSE_WITH(tcltk))
#
+# Declare the with-sse42/without-sse42 options.
+AC_ARG_WITH(sse42,
+AS_HELP_STRING([--with-sse42],[use SSE4.2 instructions])
+AS_HELP_STRING([],[(default is YES if your compiler supports -msse4.2)]),
+GIT_PARSE_WITH(sse42))
+
+if test "$NO_SSE42" != "YesPlease"; then
+ dnl Check if -msse4.2 works.
+ AC_CACHE_CHECK(for SSE4.2 support, cc_cv_sse42, [dnl
+ LIBC_TRY_CC_OPTION([-msse4.2], [cc_cv_sse42=yes], [cc_cv_sse42=no])
+ ])
+ if test $cc_cv_sse42 = no; then
+ NO_SSE42=1
+ fi
+fi
+
+GIT_CONF_SUBST([NO_SSE42])
## Checks for programs.
AC_MSG_NOTICE([CHECKS for programs])
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..218d510 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -668,6 +668,28 @@ void git_qsort(void *base, size_t nmemb, size_t size,
#endif
#endif
+#ifndef NO_SSE42
+#include
+/*
+ * Clang ships with a version of nmmintrin.h that's incomplete; if
+ * necessary, we define the constants that we're going to use.
+ */
+#ifndef _SIDD_UBYTE_OPS
+#define _SIDD_UBYTE_OPS 0x00
+#define _SIDD_CMP_EQUAL_ANY 0x00
+#define _SIDD_CMP_RANGES0x04
+#define _SIDD_CMP_EQUAL_ORDERED 0x0c
+#define _SIDD_NEGATIVE_POLARITY 0x10
+#endif
+
+/* This is the system memory page size; it's used so that we can read
+ * outside the bounds of an allocation without segfaulting.
+ */
+#ifndef PAGE_SIZE
+#define PAGE_SIZE 4096
+#endif
+#endif
+
#ifdef UNRELIABLE_FSTAT
#define fstat_is_reliable() 0
#else
diff --git a/refs.c b/refs.c
index 46139d2..2fe0075 100644
--- a/refs.c
+++ b/refs.c
@@ -24,6 +24,25 @@ static unsigned char refname_disposition[256] = {
0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 3, 0, 0, 4, 4
};
+static int check_refname_component_trailer(const char *cp, const char
*refname, int flags)
+{
+ if (cp == refname)
+ return 0; /* Component has zero length. */
+ if (refname[0] == '.') {
+ if (!(flags & REFNAME_DOT_COMPONENT))
+ return -1; /* Component starts with '.'. */
+ /*
+* Even if leading dots are allowed, don't allow "."
+* as a component (".." is prevented by a rule above).
+*/
+ if (refname[1] == '\0')
+ return -1; /* Component equals ".". */
+ }
+ if (cp - refname >= 5 && !memcmp(cp - 5, ".lock", 5))
+ return -1; /* Refname ends with ".lock". */
+ return cp - refname;
+}
+
/*
* Try to read one refname component from the front of refname.
* Return the length of the component found, or -1 if the component is
@@ -37,7 +56,7 @@ static un
Kitchen Sale Perthshire
This Forum is probably the best forum that i have ever used and i would just like to say how proud i am to be a member of this forum -- View this message in context: http://git.661346.n2.nabble.com/Kitchen-Sale-Perthshire-tp7612298.html Sent from the git mailing list archive at Nabble.com. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] receive-pack: optionally deny case clone refs
Am 6/4/2014 5:13, schrieb David Turner: > It is possible to have two branches which are the same but for case. > This works great on the case-sensitive filesystems, but not so well on > case-insensitive filesystems. It is fairly typical to have > case-insensitive clients (Macs, say) with a case-sensitive server > (GNU/Linux). > > Should a user attempt to pull on a Mac when there are case clone > branches with differing contents, they'll get an error message > containing something like "Ref refs/remotes/origin/lower is at > [sha-of-lowercase-branch] but expected [sha-of-uppercase-branch] > (unable to update local ref)" > > With a case-insensitive git server, if a branch called capital-M > Master (that differs from lowercase-m-master) is pushed, nobody else > can push to (lowercase-m) master until the branch is removed. > > Create the option receive.denycaseclonebranches, which checks pushed > branches to ensure that they are not case clones of an existing > branch. This setting is turned on by default if core.ignorecase is > set, but not otherwise. > > Signed-off-by: David Turner > --- > Documentation/config.txt | 6 ++ > Documentation/git-push.txt | 5 +++-- > Documentation/glossary-content.txt | 5 + > builtin/receive-pack.c | 27 +++- > t/t5400-send-pack.sh | 43 > ++ > 5 files changed, 83 insertions(+), 3 deletions(-) > > diff --git a/Documentation/config.txt b/Documentation/config.txt > index 1932e9b..4deddf8 100644 > --- a/Documentation/config.txt > +++ b/Documentation/config.txt > @@ -2053,6 +2053,12 @@ receive.unpackLimit:: > especially on slow filesystems. If not set, the value of > `transfer.unpackLimit` is used instead. > > +receive.denyCaseCloneBranches:: > + If set to true, git-receive-pack will deny a ref update that creates > + a ref which is the same but for case as an existing ref. This is > + useful when clients are on a case-insensitive filesystem, which > + will cause errors when given refs which differ only in case. Shouldn't this better be named 'receive.denyCaseCloneRefs'? How about 'denyCaseInsensitiveRefs', 'denyIgnoreCaseRefs'? Is this entry really so important that it must be the first in the list of receive.deny* list, which is not alphabetically sorted? > + > receive.denyDeletes:: > If set to true, git-receive-pack will deny a ref update that deletes > the ref. Use this to prevent such a ref deletion via a push. > --- a/t/t5400-send-pack.sh > +++ b/t/t5400-send-pack.sh > @@ -129,6 +129,49 @@ test_expect_success 'denyNonFastforwards trumps --force' > ' > test "$victim_orig" = "$victim_head" > ' > > +test_expect_success 'denyCaseCloneBranches works' ' > + ( > + cd victim && > + git config receive.denyCaseCloneBranches true Broken && chain. > + git config receive.denyDeletes false > + ) && > + git send-pack ./victim HEAD:refs/heads/caseclone && > + orig_ver=$(git rev-parse HEAD) && > + test_must_fail git send-pack ./victim HEAD^:refs/heads/CaseClone && > + #confirm that this had no effect upstream > + ( > + cd victim && > + test_must_fail git rev-parse CaseClone && > + remote_ver=$(git rev-parse caseclone) && > + test $orig_ver = $remote_ver Please use double-quotes around the variable expansions: There could be a failure mode where remote_ver (and even orig_ver) are empty, which would lead to a syntax error or a wrong result. BTW, on a case-insensitive file system, is there not a chance that 'git rev-parse CaseClone' succeeds even though the ref is stored in victim/.git/refs/heads/caseclone? Perhaps you should inspect the output of 'git for-each-ref' for the expected result? (Mental note: At least a case-preserving file system is required to run the test.) > + ) && > + git send-pack ./victim HEAD^:refs/heads/notacaseclone && > + test_must_fail git send-pack ./victim :CaseClone && > + #confirm that this had no effect upstream Please insert a blank after the hash mark. > + ( > + cd victim && > + test_must_fail git rev-parse CaseClone && > + remote_ver=$(git rev-parse caseclone) && > + test $orig_ver = $remote_ver > + ) && > + git send-pack ./victim :caseclone && > + #confirm that this took effect upstream > + ( > + cd victim && > + test_must_fail git rev-parse caseclone > + ) Broken && chain. > + #check that we can recreate a branch after deleting a > + #case-clone of it > + case_clone_ver=$(git rev-parse HEAD^) Broken && chain. > + git send-pack ./victim HEAD^:CaseClone && > + ( > + cd victim && > + test_must_fail git rev-parse caseclone && > + remote_ver=$(git rev-parse CaseClone) && > + test $case_clone_ver = $remote_ver > + ) > +' > + > test_expect_success 'push
