[PATCH v3 0/23] cat-file: reuse formatting logic from ref-filter
The main idea of the patch is to get rid of using custom formatting in cat-file and start using general one from ref-filter. Additional bonus is that cat-file becomes to support many new formatting commands like %(if), %(color), %(committername) etc. Updates since last review: In [PATCH v3 16/23] ref-filter: make cat_file_info independent is_cat flag is hidden into global cat_file_info variable Also make some minor refactoring.
[PATCH v3 08/23] ref-filter: reuse parse_ref_filter_atom()
Continue migrating formatting logic from cat-file to ref-filter.
Reuse parse_ref_filter_atom() for unifying all processes in ref-filter
and further removing of mark_atom_in_object_info().
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 34 +-
1 file changed, 25 insertions(+), 9 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index ff87e632f463c..5c75259b1ab8c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,6 +100,7 @@ static struct used_atom {
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
+struct expand_data *cat_file_info;
static void color_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *color_value)
{
@@ -251,6 +252,16 @@ static void objectname_atom_parser(const struct ref_format
*format, struct used_
die(_("unrecognized %%(objectname) argument: %s"), arg);
}
+static void objectsize_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
+{
+ if (!arg)
+ ; /* default to normal object size */
+ else if (!strcmp(arg, "disk"))
+ cat_file_info->info.disk_sizep = &cat_file_info->disk_size;
+ else
+ die(_("urecognized %%(objectsize) argument: %s"), arg);
+}
+
static void refname_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
{
refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
@@ -371,6 +382,14 @@ static struct valid_atom {
{ "else" },
};
+static struct valid_atom valid_cat_file_atom[] = {
+ { "objectname" },
+ { "objecttype" },
+ { "objectsize", FIELD_ULONG, objectsize_atom_parser },
+ { "rest" },
+ { "deltabase" },
+};
+
#define REF_FORMATTING_STATE_INIT { 0, NULL }
struct ref_formatting_stack {
@@ -401,20 +420,14 @@ static int is_atom(const char *atom, const char *s, int
slen)
static void mark_atom_in_object_info(const char *atom, int len,
struct expand_data *data)
{
- if (is_atom("objectname", atom, len))
- ; /* do nothing */
- else if (is_atom("objecttype", atom, len))
+ if (is_atom("objecttype", atom, len))
data->info.typep = &data->type;
else if (is_atom("objectsize", atom, len))
data->info.sizep = &data->size;
- else if (is_atom("objectsize:disk", atom, len))
- data->info.disk_sizep = &data->disk_size;
else if (is_atom("rest", atom, len))
data->split_on_whitespace = 1;
else if (is_atom("deltabase", atom, len))
data->info.delta_base_sha1 = data->delta_base_oid.hash;
- else
- die("unknown format element: %.*s", len, atom);
}
/*
@@ -483,6 +496,8 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
+ if (cat_file_info)
+ mark_atom_in_object_info(atom, atom_len, cat_file_info);
return at;
}
@@ -726,6 +741,7 @@ int verify_ref_format(struct ref_format *format)
{
const char *cp, *sp;
+ cat_file_info = format->cat_file_data;
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
const char *color, *ep = strchr(sp, ')');
@@ -736,8 +752,8 @@ int verify_ref_format(struct ref_format *format)
/* sp points at "%(" and ep points at the closing ")" */
if (format->cat_file_data)
- mark_atom_in_object_info(sp + 2, ep - sp - 2,
- format->cat_file_data);
+ at = parse_ref_filter_atom(format, valid_cat_file_atom,
+
ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
else {
at = parse_ref_filter_atom(format, valid_atom,
ARRAY_SIZE(valid_atom), sp +
2, ep);
--
https://github.com/git/git/pull/452
[PATCH v3 01/23] ref-filter: get rid of goto
Get rid of goto command in ref-filter for better readability.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index f9e25aea7a97e..d04295e33448e 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1477,12 +1477,13 @@ static void populate_value(struct ref_array_item *ref)
for (i = 0; i < used_atom_cnt; i++) {
struct atom_value *v = &ref->value[i];
- if (v->s == NULL)
- goto need_obj;
+ if (v->s == NULL) {
+ break;
+ }
}
- return;
+ if (used_atom_cnt <= i)
+ return;
- need_obj:
buf = get_obj(&ref->objectname, &obj, &size, &eaten);
if (!buf)
die(_("missing object %s for %s"),
--
https://github.com/git/git/pull/452
[PATCH v3 03/23] cat-file: reuse struct ref_format
Start using ref_format struct instead of simple char*.
Need that for further reusing of formatting logic from ref-filter.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 15 ---
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index f5fa4fd75af26..98fc5ec069a49 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -13,15 +13,16 @@
#include "tree-walk.h"
#include "sha1-array.h"
#include "packfile.h"
+#include "ref-filter.h"
struct batch_options {
+ struct ref_format format;
int enabled;
int follow_symlinks;
int print_contents;
int buffer_output;
int all_objects;
int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */
- const char *format;
};
static const char *force_path;
@@ -348,7 +349,7 @@ static void batch_object_write(const char *obj_name, struct
batch_options *opt,
return;
}
- strbuf_expand(&buf, opt->format, expand_format, data);
+ strbuf_expand(&buf, opt->format.format, expand_format, data);
strbuf_addch(&buf, '\n');
batch_write(opt, buf.buf, buf.len);
strbuf_release(&buf);
@@ -441,8 +442,8 @@ static int batch_objects(struct batch_options *opt)
int save_warning;
int retval = 0;
- if (!opt->format)
- opt->format = "%(objectname) %(objecttype) %(objectsize)";
+ if (!opt->format.format)
+ opt->format.format = "%(objectname) %(objecttype)
%(objectsize)";
/*
* Expand once with our special mark_query flag, which will prime the
@@ -451,7 +452,7 @@ static int batch_objects(struct batch_options *opt)
*/
memset(&data, 0, sizeof(data));
data.mark_query = 1;
- strbuf_expand(&buf, opt->format, expand_format, &data);
+ strbuf_expand(&buf, opt->format.format, expand_format, &data);
data.mark_query = 0;
if (opt->cmdmode)
data.split_on_whitespace = 1;
@@ -543,7 +544,7 @@ static int batch_option_callback(const struct option *opt,
bo->enabled = 1;
bo->print_contents = !strcmp(opt->long_name, "batch");
- bo->format = arg;
+ bo->format.format = arg;
return 0;
}
@@ -552,7 +553,7 @@ int cmd_cat_file(int argc, const char **argv, const char
*prefix)
{
int opt = 0;
const char *exp_type = NULL, *obj_name = NULL;
- struct batch_options batch = {0};
+ struct batch_options batch = { REF_FORMAT_INIT };
int unknown_type = 0;
const struct option options[] = {
--
https://github.com/git/git/pull/452
[PATCH v3 19/23] ref-filter: make populate_value() internal again
Remove populate_value() from header file. We needed that
for interim step, now it could be returned back.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 2 +-
ref-filter.h | 3 ---
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index eb53b0babdb83..8d104b567eb7c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1428,7 +1428,7 @@ static int check_and_fill_for_cat(struct ref_array_item
*ref)
* Parse the object referred by ref, and grab needed value.
* Return 0 if everything was successful, -1 otherwise.
*/
-int populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref)
{
void *buf;
struct object *obj;
diff --git a/ref-filter.h b/ref-filter.h
index 4eaf6d0514502..115d00288fdee 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -174,9 +174,6 @@ void setup_ref_filter_porcelain_msg(void);
void pretty_print_ref(const char *name, const unsigned char *sha1,
const struct ref_format *format);
-/* Fill the values of request and prepare all data for final string creation */
-int populate_value(struct ref_array_item *ref);
-
/* Search for atom in given format. */
int is_atom_used(const struct ref_format *format, const char *atom);
--
https://github.com/git/git/pull/452
[PATCH v3 07/23] cat-file: start migrating formatting to ref-filter
Move mark_atom_in_object_info() from cat-file to ref-filter and
start using it in verify_ref_format().
It also means that we start reusing verify_ref_format() in cat-file.
Start from simple moving of mark_atom_in_object_info(),
it would be removed later by integrating all needed processes into
ref-filter logic.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 35 +--
ref-filter.c | 41 -
ref-filter.h | 12 ++--
3 files changed, 47 insertions(+), 41 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index edb04a96d9bd3..67e7790d2f319 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -182,25 +182,6 @@ static int is_atom(const char *atom, const char *s, int
slen)
return alen == slen && !memcmp(atom, s, alen);
}
-static void mark_atom_in_object_info(const char *atom, int len,
-struct expand_data *data)
-{
- if (is_atom("objectname", atom, len))
- ; /* do nothing */
- else if (is_atom("objecttype", atom, len))
- data->info.typep = &data->type;
- else if (is_atom("objectsize", atom, len))
- data->info.sizep = &data->size;
- else if (is_atom("objectsize:disk", atom, len))
- data->info.disk_sizep = &data->disk_size;
- else if (is_atom("rest", atom, len))
- data->split_on_whitespace = 1;
- else if (is_atom("deltabase", atom, len))
- data->info.delta_base_sha1 = data->delta_base_oid.hash;
- else
- die("unknown format element: %.*s", len, atom);
-}
-
static void expand_atom(struct strbuf *sb, const char *atom, int len,
struct expand_data *data)
{
@@ -230,11 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char
*start, void *vdata)
if (!end)
die("format element '%s' does not end in ')'", start);
- if (data->mark_query)
- mark_atom_in_object_info(start + 1, end - start - 1, data);
- else
- expand_atom(sb, start + 1, end - start - 1, data);
-
+ expand_atom(sb, start + 1, end - start - 1, data);
return end - start + 1;
}
@@ -413,14 +390,12 @@ static int batch_objects(struct batch_options *opt)
opt->format.format = "%(objectname) %(objecttype)
%(objectsize)";
/*
-* Expand once with our special mark_query flag, which will prime the
-* object_info to be handed to sha1_object_info_extended for each
-* object.
+* Call verify_ref_format to prepare object_info to be handed to
+* sha1_object_info_extended for each object.
*/
memset(&data, 0, sizeof(data));
- data.mark_query = 1;
- strbuf_expand(&buf, opt->format.format, expand_format, &data);
- data.mark_query = 0;
+ opt->format.cat_file_data = &data;
+ verify_ref_format(&opt->format);
if (opt->cmdmode)
data.split_on_whitespace = 1;
diff --git a/ref-filter.c b/ref-filter.c
index 5e7ed0f338490..ff87e632f463c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -392,6 +392,31 @@ struct atom_value {
struct used_atom *atom;
};
+static int is_atom(const char *atom, const char *s, int slen)
+{
+ int alen = strlen(atom);
+ return alen == slen && !memcmp(atom, s, alen);
+}
+
+static void mark_atom_in_object_info(const char *atom, int len,
+ struct expand_data *data)
+{
+ if (is_atom("objectname", atom, len))
+ ; /* do nothing */
+ else if (is_atom("objecttype", atom, len))
+ data->info.typep = &data->type;
+ else if (is_atom("objectsize", atom, len))
+ data->info.sizep = &data->size;
+ else if (is_atom("objectsize:disk", atom, len))
+ data->info.disk_sizep = &data->disk_size;
+ else if (is_atom("rest", atom, len))
+ data->split_on_whitespace = 1;
+ else if (is_atom("deltabase", atom, len))
+ data->info.delta_base_sha1 = data->delta_base_oid.hash;
+ else
+ die("unknown format element: %.*s", len, atom);
+}
+
/*
* Used to parse format string and sort specifiers
*/
@@ -709,12 +734,18 @@ int verify_ref_format(struct ref_format *format)
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, valid_atom,
- ARRAY_SIZE(valid_atom), sp + 2, ep);
- cp = ep + 1;
- if (skip_prefix(used_atom[at].name, "color:", &color))
- format->need_color_reset_at_eol = !!strcmp(color,
"reset");
+ if (format->cat_file_data)
+
[PATCH v3 16/23] ref-filter: make cat_file_info independent
Remove connection between expand_data variable
in cat-file and in ref-filter.
It will help further to get rid of using expand_data in cat-file.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 3 ++-
ref-filter.c | 36 +++-
ref-filter.h | 7 ++-
3 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 582679f3dca2c..273b4038e3893 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -292,6 +292,7 @@ static void batch_object_write(const char *obj_name, struct
batch_options *opt,
if (populate_value(&item))
return;
+ data->type = item.type;
strbuf_expand(&buf, opt->format.format, expand_format, &item);
strbuf_addch(&buf, '\n');
batch_write(opt, buf.buf, buf.len);
@@ -392,7 +393,7 @@ static int batch_objects(struct batch_options *opt)
* sha1_object_info_extended for each object.
*/
memset(&data, 0, sizeof(data));
- opt->format.cat_file_data = &data;
+ opt->format.is_cat_file = 1;
opt->format.all_objects = opt->all_objects;
verify_ref_format(&opt->format);
diff --git a/ref-filter.c b/ref-filter.c
index 104cd6aef0102..cc70bcf2bb8b1 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -100,7 +100,7 @@ static struct used_atom {
} u;
} *used_atom;
static int used_atom_cnt, need_tagged, need_symref;
-struct expand_data *cat_file_info;
+struct expand_data cat_file_info;
static void color_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *color_value)
{
@@ -255,9 +255,9 @@ static void objectname_atom_parser(const struct ref_format
*format, struct used_
static void objectsize_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
{
if (!arg)
- cat_file_info->info.sizep = &cat_file_info->size;
+ cat_file_info.info.sizep = &cat_file_info.size;
else if (!strcmp(arg, "disk"))
- cat_file_info->info.disk_sizep = &cat_file_info->disk_size;
+ cat_file_info.info.disk_sizep = &cat_file_info.disk_size;
else
die(_("urecognized %%(objectsize) argument: %s"), arg);
}
@@ -265,7 +265,7 @@ static void objectsize_atom_parser(const struct ref_format
*format, struct used_
static void objecttype_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
{
if (!arg)
- cat_file_info->info.typep = &cat_file_info->type;
+ cat_file_info.info.typep = &cat_file_info.type;
else
die(_("urecognized %%(objecttype) argument: %s"), arg);
}
@@ -273,7 +273,7 @@ static void objecttype_atom_parser(const struct ref_format
*format, struct used_
static void deltabase_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
{
if (!arg)
- cat_file_info->info.delta_base_sha1 =
cat_file_info->delta_base_oid.hash;
+ cat_file_info.info.delta_base_sha1 =
cat_file_info.delta_base_oid.hash;
else
die(_("urecognized %%(deltabase) argument: %s"), arg);
}
@@ -751,7 +751,7 @@ int verify_ref_format(struct ref_format *format)
{
const char *cp, *sp;
- cat_file_info = format->cat_file_data;
+ cat_file_info.is_cat_file = format->is_cat_file;
format->need_color_reset_at_eol = 0;
for (cp = format->format; *cp && (sp = find_next(cp)); ) {
const char *color, *ep = strchr(sp, ')');
@@ -761,7 +761,7 @@ int verify_ref_format(struct ref_format *format)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- if (format->cat_file_data)
+ if (cat_file_info.is_cat_file)
at = parse_ref_filter_atom(format, valid_cat_file_atom,
ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
else {
@@ -775,10 +775,10 @@ int verify_ref_format(struct ref_format *format)
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
- if (cat_file_info && format->all_objects) {
+ if (cat_file_info.is_cat_file && format->all_objects) {
struct object_info empty = OBJECT_INFO_INIT;
- if (!memcmp(&cat_file_info->info, &empty, sizeof(empty)))
- cat_file_info->skip_object_info = 1;
+ if (!memcmp(&cat_file_info.info, &empty, sizeof(empty)))
+ cat_file_info.skip_object_info = 1;
}
return 0;
}
@@ -1420,18 +1420,20 @@ static const char *get_refname(struct used_atom *atom,
struct ref_array_item *re
static int check_and_fill_for
[PATCH v3 09/23] cat-file: start use ref_array_item struct
Moving from using expand_data to ref_array_item structure.
That helps us to reuse functions from ref-filter easier.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 32
ref-filter.h | 5 +
2 files changed, 25 insertions(+), 12 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 67e7790d2f319..5b7bc34f1ec6d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -183,27 +183,27 @@ static int is_atom(const char *atom, const char *s, int
slen)
}
static void expand_atom(struct strbuf *sb, const char *atom, int len,
-struct expand_data *data)
+struct ref_array_item *item)
{
if (is_atom("objectname", atom, len))
- strbuf_addstr(sb, oid_to_hex(&data->oid));
+ strbuf_addstr(sb, oid_to_hex(&item->objectname));
else if (is_atom("objecttype", atom, len))
- strbuf_addstr(sb, typename(data->type));
+ strbuf_addstr(sb, typename(item->type));
else if (is_atom("objectsize", atom, len))
- strbuf_addf(sb, "%lu", data->size);
+ strbuf_addf(sb, "%lu", item->size);
else if (is_atom("objectsize:disk", atom, len))
- strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+ strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)item->disk_size);
else if (is_atom("rest", atom, len)) {
- if (data->rest)
- strbuf_addstr(sb, data->rest);
+ if (item->rest)
+ strbuf_addstr(sb, item->rest);
} else if (is_atom("deltabase", atom, len))
- strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
+ strbuf_addstr(sb, oid_to_hex(item->delta_base_oid));
}
-static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
+static size_t expand_format(struct strbuf *sb, const char *start, void *data)
{
const char *end;
- struct expand_data *data = vdata;
+ struct ref_array_item *item = data;
if (*start != '(')
return 0;
@@ -211,7 +211,7 @@ static size_t expand_format(struct strbuf *sb, const char
*start, void *vdata)
if (!end)
die("format element '%s' does not end in ')'", start);
- expand_atom(sb, start + 1, end - start - 1, data);
+ expand_atom(sb, start + 1, end - start - 1, item);
return end - start + 1;
}
@@ -283,6 +283,7 @@ static void batch_object_write(const char *obj_name, struct
batch_options *opt,
struct expand_data *data)
{
struct strbuf buf = STRBUF_INIT;
+ struct ref_array_item item = {0};
if (!data->skip_object_info &&
sha1_object_info_extended(data->oid.hash, &data->info,
@@ -293,7 +294,14 @@ static void batch_object_write(const char *obj_name,
struct batch_options *opt,
return;
}
- strbuf_expand(&buf, opt->format.format, expand_format, data);
+ item.objectname = data->oid;
+ item.type = data->type;
+ item.size = data->size;
+ item.disk_size = data->disk_size;
+ item.rest = data->rest;
+ item.delta_base_oid = &data->delta_base_oid;
+
+ strbuf_expand(&buf, opt->format.format, expand_format, &item);
strbuf_addch(&buf, '\n');
batch_write(opt, buf.buf, buf.len);
strbuf_release(&buf);
diff --git a/ref-filter.h b/ref-filter.h
index 52e07dbe6864a..781921d4e0978 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -40,6 +40,11 @@ struct ref_array_item {
const char *symref;
struct commit *commit;
struct atom_value *value;
+ enum object_type type;
+ unsigned long size;
+ off_t disk_size;
+ const char *rest;
+ struct object_id *delta_base_oid;
char refname[FLEX_ARRAY];
};
--
https://github.com/git/git/pull/452
[PATCH v3 21/23] for-each-ref: tests for new atoms added
Add tests for new formatting atoms: rest, deltabase, objectsize:disk. rest means nothing and we expand it into empty string. We need this atom for cat-file command. Have plans to support deltabase and objectsize:disk further (as it is done in cat-file), now also expand it to empty string. Signed-off-by: Olga Telezhnaia Mentored-by: Christian Couder Mentored by: Jeff King --- t/t6300-for-each-ref.sh | 18 ++ 1 file changed, 18 insertions(+) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index c128dfc579079..eee656a6abba9 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -316,6 +316,24 @@ test_expect_success 'exercise strftime with odd fields' ' test_cmp expected actual ' +test_expect_success 'Check format %(objectsize:disk) gives empty output ' ' + echo >expected && + git for-each-ref --format="%(objectsize:disk)" refs/heads >actual && + test_cmp expected actual +' + +test_expect_success 'Check format %(rest) gives empty output ' ' + echo >expected && + git for-each-ref --format="%(rest)" refs/heads >actual && + test_cmp expected actual +' + +test_expect_success 'Check format %(deltabase) gives empty output ' ' + echo >expected && + git for-each-ref --format="%(deltabase)" refs/heads >actual && + test_cmp expected actual +' + cat >expected <<\EOF refs/heads/master refs/remotes/origin/master -- https://github.com/git/git/pull/452
[PATCH v3 02/23] ref-filter: add return value to some functions
Add return flag to format_ref_array_item(), show_ref_array_item(),
get_ref_array_info() and populate_value() for further using.
Need it to handle situations when item is broken but we can not invoke
die() because we are in batch mode and all items need to be processed.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 37 -
ref-filter.h | 14 ++
2 files changed, 34 insertions(+), 17 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index d04295e33448e..9ed5e66066a7a 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1356,8 +1356,9 @@ static const char *get_refname(struct used_atom *atom,
struct ref_array_item *re
/*
* Parse the object referred by ref, and grab needed value.
+ * Return 0 if everything was successful, -1 otherwise.
*/
-static void populate_value(struct ref_array_item *ref)
+static int populate_value(struct ref_array_item *ref)
{
void *buf;
struct object *obj;
@@ -1482,7 +1483,7 @@ static void populate_value(struct ref_array_item *ref)
}
}
if (used_atom_cnt <= i)
- return;
+ return 0;
buf = get_obj(&ref->objectname, &obj, &size, &eaten);
if (!buf)
@@ -1501,7 +1502,7 @@ static void populate_value(struct ref_array_item *ref)
* object, we are done.
*/
if (!need_tagged || (obj->type != OBJ_TAG))
- return;
+ return 0;
/*
* If it is a tag object, see if we use a value that derefs
@@ -1525,19 +1526,24 @@ static void populate_value(struct ref_array_item *ref)
grab_values(ref->value, 1, obj, buf, size);
if (!eaten)
free(buf);
+
+ return 0;
}
/*
* Given a ref, return the value for the atom. This lazily gets value
* out of the object by calling populate value.
+ * Return 0 if everything was successful, -1 otherwise.
*/
-static void get_ref_atom_value(struct ref_array_item *ref, int atom, struct
atom_value **v)
+static int get_ref_atom_value(struct ref_array_item *ref, int atom, struct
atom_value **v)
{
+ int retval = 0;
if (!ref->value) {
- populate_value(ref);
+ retval = populate_value(ref);
fill_missing_values(ref->value);
}
*v = &ref->value[atom];
+ return retval;
}
/*
@@ -2122,7 +2128,7 @@ static void append_literal(const char *cp, const char
*ep, struct ref_formatting
}
}
-void format_ref_array_item(struct ref_array_item *info,
+int format_ref_array_item(struct ref_array_item *info,
const struct ref_format *format,
struct strbuf *final_buf)
{
@@ -2138,9 +2144,10 @@ void format_ref_array_item(struct ref_array_item *info,
ep = strchr(sp, ')');
if (cp < sp)
append_literal(cp, sp, &state);
- get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2, ep),
- &atomv);
+ if (get_ref_atom_value(info,
+ parse_ref_filter_atom(format, sp + 2,
ep),
+ &atomv))
+ return -1;
atomv->handler(atomv, &state);
}
if (*cp) {
@@ -2156,17 +2163,21 @@ void format_ref_array_item(struct ref_array_item *info,
die(_("format: %%(end) atom missing"));
strbuf_addbuf(final_buf, &state.stack->output);
pop_stack_element(&state.stack);
+ return 0;
}
-void show_ref_array_item(struct ref_array_item *info,
+int show_ref_array_item(struct ref_array_item *info,
const struct ref_format *format)
{
struct strbuf final_buf = STRBUF_INIT;
+ int retval = format_ref_array_item(info, format, &final_buf);
- format_ref_array_item(info, format, &final_buf);
- fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ if (!retval) {
+ fwrite(final_buf.buf, 1, final_buf.len, stdout);
+ putchar('\n');
+ }
strbuf_release(&final_buf);
- putchar('\n');
+ return retval;
}
void pretty_print_ref(const char *name, const unsigned char *sha1,
diff --git a/ref-filter.h b/ref-filter.h
index 0d98342b34319..b75c8ac45248e 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -109,12 +109,18 @@ void ref_array_clear(struct ref_array *array);
int verify_ref_format(struct ref_format *format);
/* Sort the given ref_array as per the ref_sorting provided */
void ref_array_sort(struct ref_sorting *sort, struct ref_array *array);
-/* Based on the given format and quote_style, fill the strbuf */
-void format_ref_array_item(struct ref_array_item *info,
+/*
+ * Based on the given format and quote_style, fill the strbuf.
+ * Return 0 if everything was successful, -1 otherwis
[PATCH v3 15/23] cat-file: move skip_object_info into ref-filter
Move logic related to skip_object_info into ref-filter,
so that cat-file does not use that field at all.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 7 +--
ref-filter.c | 5 +
ref-filter.h | 1 +
3 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 3a49b55a1cc2e..582679f3dca2c 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -393,14 +393,9 @@ static int batch_objects(struct batch_options *opt)
*/
memset(&data, 0, sizeof(data));
opt->format.cat_file_data = &data;
+ opt->format.all_objects = opt->all_objects;
verify_ref_format(&opt->format);
- if (opt->all_objects) {
- struct object_info empty = OBJECT_INFO_INIT;
- if (!memcmp(&data.info, &empty, sizeof(empty)))
- data.skip_object_info = 1;
- }
-
/*
* If we are printing out the object, then always fill in the type,
* since we will want to decide whether or not to stream.
diff --git a/ref-filter.c b/ref-filter.c
index 4adeea6aad0da..104cd6aef0102 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -775,6 +775,11 @@ int verify_ref_format(struct ref_format *format)
}
if (format->need_color_reset_at_eol && !want_color(format->use_color))
format->need_color_reset_at_eol = 0;
+ if (cat_file_info && format->all_objects) {
+ struct object_info empty = OBJECT_INFO_INIT;
+ if (!memcmp(&cat_file_info->info, &empty, sizeof(empty)))
+ cat_file_info->skip_object_info = 1;
+ }
return 0;
}
diff --git a/ref-filter.h b/ref-filter.h
index fffc443726e28..b1c668c12428b 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -118,6 +118,7 @@ struct ref_format {
* hopefully would be reduced later.
*/
struct expand_data *cat_file_data;
+ unsigned all_objects : 1;
};
#define REF_FORMAT_INIT { NULL, 0, -1 }
--
https://github.com/git/git/pull/452
[PATCH v3 11/23] ref-filter: rename field in ref_array_item stuct
Rename objectname field to oid in struct ref_array_item.
Next commit will add objectname field that will contain
string representation of object id.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 4 ++--
ref-filter.c | 10 +-
ref-filter.h | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 5b7bc34f1ec6d..0c362828ad81e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -186,7 +186,7 @@ static void expand_atom(struct strbuf *sb, const char
*atom, int len,
struct ref_array_item *item)
{
if (is_atom("objectname", atom, len))
- strbuf_addstr(sb, oid_to_hex(&item->objectname));
+ strbuf_addstr(sb, oid_to_hex(&item->oid));
else if (is_atom("objecttype", atom, len))
strbuf_addstr(sb, typename(item->type));
else if (is_atom("objectsize", atom, len))
@@ -294,7 +294,7 @@ static void batch_object_write(const char *obj_name, struct
batch_options *opt,
return;
}
- item.objectname = data->oid;
+ item.oid = data->oid;
item.type = data->type;
item.size = data->size;
item.disk_size = data->disk_size;
diff --git a/ref-filter.c b/ref-filter.c
index 4acd391b5dfac..d09ec1bde6d54 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1489,7 +1489,7 @@ int populate_value(struct ref_array_item *ref)
v->s = xstrdup(buf + 1);
}
continue;
- } else if (!deref && grab_objectname(name,
ref->objectname.hash, v, atom)) {
+ } else if (!deref && grab_objectname(name, ref->oid.hash, v,
atom)) {
continue;
} else if (!strcmp(name, "HEAD")) {
if (atom->u.head && !strcmp(ref->refname, atom->u.head))
@@ -1534,13 +1534,13 @@ int populate_value(struct ref_array_item *ref)
if (used_atom_cnt <= i)
return 0;
- buf = get_obj(&ref->objectname, &obj, &size, &eaten);
+ buf = get_obj(&ref->oid, &obj, &size, &eaten);
if (!buf)
die(_("missing object %s for %s"),
- oid_to_hex(&ref->objectname), ref->refname);
+ oid_to_hex(&ref->oid), ref->refname);
if (!obj)
die(_("parse_object_buffer failed on %s for %s"),
- oid_to_hex(&ref->objectname), ref->refname);
+ oid_to_hex(&ref->oid), ref->refname);
grab_values(ref->value, 0, obj, buf, size);
if (!eaten)
@@ -1890,7 +1890,7 @@ static struct ref_array_item *new_ref_array_item(const
char *refname,
{
struct ref_array_item *ref;
FLEX_ALLOC_STR(ref, refname, refname);
- hashcpy(ref->objectname.hash, objectname);
+ hashcpy(ref->oid.hash, objectname);
ref->flag = flag;
return ref;
diff --git a/ref-filter.h b/ref-filter.h
index e16ea2a990119..87b026b8b76d0 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -34,7 +34,7 @@ struct ref_sorting {
};
struct ref_array_item {
- struct object_id objectname;
+ struct object_id oid;
int flag;
unsigned int kind;
const char *symref;
--
https://github.com/git/git/pull/452
[PATCH v3 18/23] cat-file: reuse printing logic from ref-filter
Reuse code from ref-filter to print resulting message.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 51 ---
ref-filter.c | 21 +++--
2 files changed, 23 insertions(+), 49 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 273b4038e3893..21007995c5ac6 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -176,45 +176,6 @@ static int cat_one_file(int opt, const char *exp_type,
const char *obj_name,
return 0;
}
-static int is_atom(const char *atom, const char *s, int slen)
-{
- int alen = strlen(atom);
- return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
-struct ref_array_item *item)
-{
- if (is_atom("objectname", atom, len))
- strbuf_addstr(sb, oid_to_hex(&item->oid));
- else if (is_atom("objecttype", atom, len))
- strbuf_addstr(sb, typename(item->type));
- else if (is_atom("objectsize", atom, len))
- strbuf_addf(sb, "%lu", item->size);
- else if (is_atom("objectsize:disk", atom, len))
- strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)item->disk_size);
- else if (is_atom("rest", atom, len)) {
- if (item->rest)
- strbuf_addstr(sb, item->rest);
- } else if (is_atom("deltabase", atom, len))
- strbuf_addstr(sb, oid_to_hex(item->delta_base_oid));
-}
-
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
-{
- const char *end;
- struct ref_array_item *item = data;
-
- if (*start != '(')
- return 0;
- end = strchr(start + 1, ')');
- if (!end)
- die("format element '%s' does not end in ')'", start);
-
- expand_atom(sb, start + 1, end - start - 1, item);
- return end - start + 1;
-}
-
static void batch_write(struct batch_options *opt, const void *data, int len)
{
if (opt->buffer_output) {
@@ -282,23 +243,19 @@ static void print_object_or_die(struct batch_options
*opt, struct expand_data *d
static void batch_object_write(const char *obj_name, struct batch_options *opt,
struct expand_data *data)
{
- struct strbuf buf = STRBUF_INIT;
struct ref_array_item item = {0};
item.oid = data->oid;
item.rest = data->rest;
item.objectname = obj_name;
- if (populate_value(&item))
+ if (show_ref_array_item(&item, &opt->format))
return;
-
- data->type = item.type;
- strbuf_expand(&buf, opt->format.format, expand_format, &item);
- strbuf_addch(&buf, '\n');
- batch_write(opt, buf.buf, buf.len);
- strbuf_release(&buf);
+ if (!opt->buffer_output)
+ fflush(stdout);
if (opt->print_contents) {
+ data->type = item.type;
print_object_or_die(opt, data);
batch_write(opt, "\n", 1);
}
diff --git a/ref-filter.c b/ref-filter.c
index ee311d51ff81c..eb53b0babdb83 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1465,7 +1465,21 @@ int populate_value(struct ref_array_item *ref)
name++;
}
- if (starts_with(name, "refname"))
+ if (cat_file_info.is_cat_file) {
+ if (starts_with(name, "objectname"))
+ v->s = oid_to_hex(&ref->oid);
+ else if (starts_with(name, "objecttype"))
+ v->s = typename(ref->type);
+ else if (starts_with(name, "objectsize")) {
+ v->s = xstrfmt("%lu", ref->size);
+ } else if (starts_with(name, "objectsize:disk")) {
+ v->s = xstrfmt("%"PRIuMAX,
(uintmax_t)ref->disk_size);
+ } else if (starts_with(name, "rest"))
+ v->s = ref->rest;
+ else if (starts_with(name, "deltabase"))
+ v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
+ continue;
+ } else if (starts_with(name, "refname"))
refname = get_refname(atom, ref);
else if (starts_with(name, "symref"))
refname = get_symref(atom, ref);
@@ -2207,6 +2221,7 @@ int format_ref_array_item(struct ref_array_item *info,
{
const char *cp, *sp, *ep;
struct ref_formatting_state state = REF_FORMATTING_STATE_INIT;
+ int retval = 0;
state.quote_style = format->quote_style;
push_stack_element(&state.stack);
@@ -2223,6 +2238,8 @@ int format_ref_array_item(struct ref_array_item *info,
return -1;
atomv->handler(atomv, &state);
[PATCH v3 05/23] cat-file: move struct expand_data into ref-filter
Need that for further reusing of formatting logic in cat-file.
Have plans to get rid of using expand_data in cat-file at all,
and use it only in ref-filter for collecting, formatting and printing
needed data.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 36
ref-filter.h | 36
2 files changed, 36 insertions(+), 36 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 98fc5ec069a49..37d6096d201b5 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -176,42 +176,6 @@ static int cat_one_file(int opt, const char *exp_type,
const char *obj_name,
return 0;
}
-struct expand_data {
- struct object_id oid;
- enum object_type type;
- unsigned long size;
- off_t disk_size;
- const char *rest;
- struct object_id delta_base_oid;
-
- /*
-* If mark_query is true, we do not expand anything, but rather
-* just mark the object_info with items we wish to query.
-*/
- int mark_query;
-
- /*
-* Whether to split the input on whitespace before feeding it to
-* get_sha1; this is decided during the mark_query phase based on
-* whether we have a %(rest) token in our format.
-*/
- int split_on_whitespace;
-
- /*
-* After a mark_query run, this object_info is set up to be
-* passed to sha1_object_info_extended. It will point to the data
-* elements above, so you can retrieve the response from there.
-*/
- struct object_info info;
-
- /*
-* This flag will be true if the requested batch format and options
-* don't require us to call sha1_object_info, which can then be
-* optimized out.
-*/
- unsigned skip_object_info : 1;
-};
-
static int is_atom(const char *atom, const char *s, int slen)
{
int alen = strlen(atom);
diff --git a/ref-filter.h b/ref-filter.h
index b75c8ac45248e..17f2ac24d2739 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -72,6 +72,42 @@ struct ref_filter {
verbose;
};
+struct expand_data {
+ struct object_id oid;
+ enum object_type type;
+ unsigned long size;
+ off_t disk_size;
+ const char *rest;
+ struct object_id delta_base_oid;
+
+ /*
+* If mark_query is true, we do not expand anything, but rather
+* just mark the object_info with items we wish to query.
+*/
+ int mark_query;
+
+ /*
+* Whether to split the input on whitespace before feeding it to
+* get_sha1; this is decided during the mark_query phase based on
+* whether we have a %(rest) token in our format.
+*/
+ int split_on_whitespace;
+
+ /*
+* After a mark_query run, this object_info is set up to be
+* passed to sha1_object_info_extended. It will point to the data
+* elements above, so you can retrieve the response from there.
+*/
+ struct object_info info;
+
+ /*
+* This flag will be true if the requested batch format and options
+* don't require us to call sha1_object_info, which can then be
+* optimized out.
+*/
+ unsigned skip_object_info : 1;
+};
+
struct ref_format {
/*
* Set these to define the format; make sure you call
--
https://github.com/git/git/pull/452
[PATCH v3 10/23] ref-filter: make populate_value() global
Make function global for further using in cat-file.
In the end of patch series this function becomes internal again,
so this is a part of middle step. cat-file would use more general
functions further.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 2 +-
ref-filter.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/ref-filter.c b/ref-filter.c
index 5c75259b1ab8c..4acd391b5dfac 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1407,7 +1407,7 @@ static const char *get_refname(struct used_atom *atom,
struct ref_array_item *re
* Parse the object referred by ref, and grab needed value.
* Return 0 if everything was successful, -1 otherwise.
*/
-static int populate_value(struct ref_array_item *ref)
+int populate_value(struct ref_array_item *ref)
{
void *buf;
struct object *obj;
diff --git a/ref-filter.h b/ref-filter.h
index 781921d4e0978..e16ea2a990119 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -182,4 +182,7 @@ void setup_ref_filter_porcelain_msg(void);
void pretty_print_ref(const char *name, const unsigned char *sha1,
const struct ref_format *format);
+/* Fill the values of request and prepare all data for final string creation */
+int populate_value(struct ref_array_item *ref);
+
#endif /* REF_FILTER_H */
--
https://github.com/git/git/pull/452
[PATCH v3 12/23] cat-file: start reusing populate_value()
Move logic related to getting object info from cat-file to ref-filter.
It will help to reuse whole formatting logic from ref-filter further.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 17 -
ref-filter.c | 20
ref-filter.h | 1 +
3 files changed, 25 insertions(+), 13 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 0c362828ad81e..6db57e3533806 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -285,21 +285,12 @@ static void batch_object_write(const char *obj_name,
struct batch_options *opt,
struct strbuf buf = STRBUF_INIT;
struct ref_array_item item = {0};
- if (!data->skip_object_info &&
- sha1_object_info_extended(data->oid.hash, &data->info,
- OBJECT_INFO_LOOKUP_REPLACE) < 0) {
- printf("%s missing\n",
- obj_name ? obj_name : oid_to_hex(&data->oid));
- fflush(stdout);
- return;
- }
-
item.oid = data->oid;
- item.type = data->type;
- item.size = data->size;
- item.disk_size = data->disk_size;
item.rest = data->rest;
- item.delta_base_oid = &data->delta_base_oid;
+ item.objectname = obj_name;
+
+ if (populate_value(&item))
+ return;
strbuf_expand(&buf, opt->format.format, expand_format, &item);
strbuf_addch(&buf, '\n');
diff --git a/ref-filter.c b/ref-filter.c
index d09ec1bde6d54..3f92a27d98b6c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1403,6 +1403,23 @@ static const char *get_refname(struct used_atom *atom,
struct ref_array_item *re
return show_ref(&atom->u.refname, ref->refname);
}
+static int check_and_fill_for_cat(struct ref_array_item *ref)
+{
+ if (!cat_file_info->skip_object_info &&
+ sha1_object_info_extended(ref->oid.hash, &cat_file_info->info,
+ OBJECT_INFO_LOOKUP_REPLACE) < 0) {
+ const char *e = ref->objectname;
+ printf("%s missing\n", e ? e : oid_to_hex(&ref->oid));
+ fflush(stdout);
+ return -1;
+ }
+ ref->type = cat_file_info->type;
+ ref->size = cat_file_info->size;
+ ref->disk_size = cat_file_info->disk_size;
+ ref->delta_base_oid = &cat_file_info->delta_base_oid;
+ return 0;
+}
+
/*
* Parse the object referred by ref, and grab needed value.
* Return 0 if everything was successful, -1 otherwise.
@@ -1424,6 +1441,9 @@ int populate_value(struct ref_array_item *ref)
ref->symref = "";
}
+ if (cat_file_info && check_and_fill_for_cat(ref))
+ return -1;
+
/* Fill in specials first */
for (i = 0; i < used_atom_cnt; i++) {
struct used_atom *atom = &used_atom[i];
diff --git a/ref-filter.h b/ref-filter.h
index 87b026b8b76d0..5c6e019998716 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -45,6 +45,7 @@ struct ref_array_item {
off_t disk_size;
const char *rest;
struct object_id *delta_base_oid;
+ const char *objectname;
char refname[FLEX_ARRAY];
};
--
https://github.com/git/git/pull/452
[PATCH v3 04/23] ref-filter: make valid_atom as function parameter
Make valid_atom as a function parameter,
there could be another variable further.
Need that for further reusing of formatting logic in cat-file.c.
We do not need to allow users to pass their own valid_atom variable in
global functions like verify_ref_format() because in the end we want to
have same set of valid atoms for all commands. But, as a first step
of migrating, I create further another version of valid_atom
for cat-file.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 9ed5e66066a7a..5e7ed0f338490 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -325,7 +325,7 @@ static void head_atom_parser(const struct ref_format
*format, struct used_atom *
atom->u.head = resolve_refdup("HEAD", RESOLVE_REF_READING, NULL, NULL);
}
-static struct {
+static struct valid_atom {
const char *name;
cmp_type cmp_type;
void (*parser)(const struct ref_format *format, struct used_atom *atom,
const char *arg);
@@ -396,6 +396,7 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
+const struct valid_atom *valid_atom, int
n_atoms,
const char *atom, const char *ep)
{
const char *sp;
@@ -425,13 +426,13 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
atom_len = (arg ? arg : ep) - sp;
/* Is the atom a valid one? */
- for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
+ for (i = 0; i < n_atoms; i++) {
int len = strlen(valid_atom[i].name);
if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
break;
}
- if (ARRAY_SIZE(valid_atom) <= i)
+ if (n_atoms <= i)
die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
/* Add it in, including the deref prefix */
@@ -708,7 +709,8 @@ int verify_ref_format(struct ref_format *format)
if (!ep)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- at = parse_ref_filter_atom(format, sp + 2, ep);
+ at = parse_ref_filter_atom(format, valid_atom,
+ ARRAY_SIZE(valid_atom), sp + 2, ep);
cp = ep + 1;
if (skip_prefix(used_atom[at].name, "color:", &color))
@@ -2145,7 +2147,9 @@ int format_ref_array_item(struct ref_array_item *info,
if (cp < sp)
append_literal(cp, sp, &state);
if (get_ref_atom_value(info,
- parse_ref_filter_atom(format, sp + 2,
ep),
+ parse_ref_filter_atom(format, valid_atom,
+
ARRAY_SIZE(valid_atom),
+sp + 2, ep),
&atomv))
return -1;
atomv->handler(atomv, &state);
@@ -2198,7 +2202,8 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, atom, end);
+ return parse_ref_filter_atom(&dummy, valid_atom,
+ARRAY_SIZE(valid_atom), atom, end);
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/452
[PATCH v3 14/23] ref_filter: add is_atom_used function
Delete all items related to split_on_whitespace from ref-filter
and add new function for handling the logic.
Now cat-file could invoke that function to implementing its logic.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 8 +++-
ref-filter.c | 17 +++--
ref-filter.h | 10 +++---
3 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 6db57e3533806..3a49b55a1cc2e 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -382,8 +382,7 @@ static int batch_objects(struct batch_options *opt)
{
struct strbuf buf = STRBUF_INIT;
struct expand_data data;
- int save_warning;
- int retval = 0;
+ int save_warning, is_rest, retval = 0;
if (!opt->format.format)
opt->format.format = "%(objectname) %(objecttype)
%(objectsize)";
@@ -395,8 +394,6 @@ static int batch_objects(struct batch_options *opt)
memset(&data, 0, sizeof(data));
opt->format.cat_file_data = &data;
verify_ref_format(&opt->format);
- if (opt->cmdmode)
- data.split_on_whitespace = 1;
if (opt->all_objects) {
struct object_info empty = OBJECT_INFO_INIT;
@@ -435,9 +432,10 @@ static int batch_objects(struct batch_options *opt)
*/
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
+ is_rest = opt->cmdmode || is_atom_used(&opt->format, "rest");
while (strbuf_getline(&buf, stdin) != EOF) {
- if (data.split_on_whitespace) {
+ if (is_rest) {
/*
* Split at first whitespace, tying off the beginning
* of the string and saving the remainder (or NULL) in
diff --git a/ref-filter.c b/ref-filter.c
index 34a54db168265..4adeea6aad0da 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -493,8 +493,6 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
- if (cat_file_info && !strcmp(valid_atom[i].name, "rest"))
- cat_file_info->split_on_whitespace = 1;
return at;
}
@@ -730,6 +728,21 @@ static const char *find_next(const char *cp)
return NULL;
}
+/* Search for atom in given format. */
+int is_atom_used(const struct ref_format *format, const char *atom)
+{
+ const char *cp, *sp;
+ for (cp = format->format; *cp && (sp = find_next(cp)); ) {
+ const char *ep = strchr(sp, ')');
+ int atom_len = ep - sp - 2;
+ sp += 2;
+ if (atom_len == strlen(atom) && !memcmp(sp, atom, atom_len))
+ return 1;
+ cp = ep + 1;
+ }
+ return 0;
+}
+
/*
* Make sure the format string is well formed, and parse out
* the used atoms.
diff --git a/ref-filter.h b/ref-filter.h
index 5c6e019998716..fffc443726e28 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -86,13 +86,6 @@ struct expand_data {
const char *rest;
struct object_id delta_base_oid;
- /*
-* Whether to split the input on whitespace before feeding it to
-* get_sha1; this is decided during the mark_query phase based on
-* whether we have a %(rest) token in our format.
-*/
- int split_on_whitespace;
-
/*
* After a mark_query run, this object_info is set up to be
* passed to sha1_object_info_extended. It will point to the data
@@ -186,4 +179,7 @@ void pretty_print_ref(const char *name, const unsigned char
*sha1,
/* Fill the values of request and prepare all data for final string creation */
int populate_value(struct ref_array_item *ref);
+/* Search for atom in given format. */
+int is_atom_used(const struct ref_format *format, const char *atom);
+
#endif /* REF_FILTER_H */
--
https://github.com/git/git/pull/452
[PATCH v3 23/23] cat-file: update of docs
Update the docs for cat-file command. Some new formatting atoms added because of reusing ref-filter code. We do not support cat-file atoms in general formatting logic (there is just the support for cat-file), that is why some of the atoms are still explained in cat-file docs. We need to move these explanations when atoms will be supported by other commands. Signed-off-by: Olga Telezhnaia Mentored-by: Christian Couder Mentored by: Jeff King --- Documentation/git-cat-file.txt | 13 ++--- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index f90f09b03fae5..90639ac21d0e8 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -187,17 +187,8 @@ linkgit:git-rev-parse[1]. You can specify the information shown for each object by using a custom ``. The `` is copied literally to stdout for each object, with placeholders of the form `%(atom)` expanded, followed by a -newline. The available atoms are: - -`objectname`:: - The 40-hex object name of the object. - -`objecttype`:: - The type of the object (the same as `cat-file -t` reports). - -`objectsize`:: - The size, in bytes, of the object (the same as `cat-file -s` - reports). +newline. The available atoms are the same as that of +linkgit:git-for-each-ref[1], but there are some additional ones: `objectsize:disk`:: The size, in bytes, that the object takes up on disk. See the -- https://github.com/git/git/pull/452
[PATCH v3 20/23] ref-filter: unifying formatting of cat-file opts
cat-file options are now filled by general logic.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 33 -
1 file changed, 16 insertions(+), 17 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 8d104b567eb7c..5781416cf9126 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -824,8 +824,12 @@ static void grab_common_values(struct atom_value *val, int
deref, struct object
else if (!strcmp(name, "objectsize")) {
v->value = sz;
v->s = xstrfmt("%lu", sz);
- }
- else if (deref)
+ } else if (!strcmp(name, "objectsize:disk")) {
+ if (cat_file_info.is_cat_file) {
+ v->value = cat_file_info.disk_size;
+ v->s = xstrfmt("%"PRIuMAX, (uintmax_t)v->value);
+ }
+ } else if (deref)
grab_objectname(name, obj->oid.hash, v, &used_atom[i]);
}
}
@@ -1465,21 +1469,7 @@ static int populate_value(struct ref_array_item *ref)
name++;
}
- if (cat_file_info.is_cat_file) {
- if (starts_with(name, "objectname"))
- v->s = oid_to_hex(&ref->oid);
- else if (starts_with(name, "objecttype"))
- v->s = typename(ref->type);
- else if (starts_with(name, "objectsize")) {
- v->s = xstrfmt("%lu", ref->size);
- } else if (starts_with(name, "objectsize:disk")) {
- v->s = xstrfmt("%"PRIuMAX,
(uintmax_t)ref->disk_size);
- } else if (starts_with(name, "rest"))
- v->s = ref->rest;
- else if (starts_with(name, "deltabase"))
- v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
- continue;
- } else if (starts_with(name, "refname"))
+ if (starts_with(name, "refname"))
refname = get_refname(atom, ref);
else if (starts_with(name, "symref"))
refname = get_symref(atom, ref);
@@ -1535,6 +1525,15 @@ static int populate_value(struct ref_array_item *ref)
else
v->s = " ";
continue;
+ } else if (starts_with(name, "rest")) {
+ v->s = ref->rest ? ref->rest : "";
+ continue;
+ } else if (starts_with(name, "deltabase")) {
+ if (ref->delta_base_oid)
+ v->s = xstrdup(oid_to_hex(ref->delta_base_oid));
+ else
+ v->s = "";
+ continue;
} else if (starts_with(name, "align")) {
v->handler = align_atom_handler;
continue;
--
https://github.com/git/git/pull/452
[PATCH v3 17/23] ref-filter: make valid_atom general again
Stop using valid_cat_file_atom, making the code more general.
Further commits will contain some tests, docs and
support of new features.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 34 +-
1 file changed, 9 insertions(+), 25 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index cc70bcf2bb8b1..ee311d51ff81c 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -358,8 +358,8 @@ static struct valid_atom {
void (*parser)(const struct ref_format *format, struct used_atom *atom,
const char *arg);
} valid_atom[] = {
{ "refname" , FIELD_STR, refname_atom_parser },
- { "objecttype" },
- { "objectsize", FIELD_ULONG },
+ { "objecttype", FIELD_STR, objecttype_atom_parser },
+ { "objectsize", FIELD_ULONG, objectsize_atom_parser },
{ "objectname", FIELD_STR, objectname_atom_parser },
{ "tree" },
{ "parent" },
@@ -396,12 +396,6 @@ static struct valid_atom {
{ "if", FIELD_STR, if_atom_parser },
{ "then" },
{ "else" },
-};
-
-static struct valid_atom valid_cat_file_atom[] = {
- { "objectname" },
- { "objecttype", FIELD_STR, objecttype_atom_parser },
- { "objectsize", FIELD_ULONG, objectsize_atom_parser },
{ "rest" },
{ "deltabase", FIELD_STR, deltabase_atom_parser },
};
@@ -431,7 +425,6 @@ struct atom_value {
* Used to parse format string and sort specifiers
*/
static int parse_ref_filter_atom(const struct ref_format *format,
-const struct valid_atom *valid_atom, int
n_atoms,
const char *atom, const char *ep)
{
const char *sp;
@@ -461,13 +454,13 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
atom_len = (arg ? arg : ep) - sp;
/* Is the atom a valid one? */
- for (i = 0; i < n_atoms; i++) {
+ for (i = 0; i < ARRAY_SIZE(valid_atom); i++) {
int len = strlen(valid_atom[i].name);
if (len == atom_len && !memcmp(valid_atom[i].name, sp, len))
break;
}
- if (n_atoms <= i)
+ if (ARRAY_SIZE(valid_atom) <= i)
die(_("unknown field name: %.*s"), (int)(ep-atom), atom);
/* Add it in, including the deref prefix */
@@ -761,15 +754,9 @@ int verify_ref_format(struct ref_format *format)
return error(_("malformed format string %s"), sp);
/* sp points at "%(" and ep points at the closing ")" */
- if (cat_file_info.is_cat_file)
- at = parse_ref_filter_atom(format, valid_cat_file_atom,
-
ARRAY_SIZE(valid_cat_file_atom), sp + 2, ep);
- else {
- at = parse_ref_filter_atom(format, valid_atom,
- ARRAY_SIZE(valid_atom), sp +
2, ep);
- if (skip_prefix(used_atom[at].name, "color:", &color))
- format->need_color_reset_at_eol =
!!strcmp(color, "reset");
- }
+ at = parse_ref_filter_atom(format, sp + 2, ep);
+ if (skip_prefix(used_atom[at].name, "color:", &color))
+ format->need_color_reset_at_eol = !!strcmp(color,
"reset");
cp = ep + 1;
}
@@ -2231,9 +2218,7 @@ int format_ref_array_item(struct ref_array_item *info,
if (cp < sp)
append_literal(cp, sp, &state);
if (get_ref_atom_value(info,
- parse_ref_filter_atom(format, valid_atom,
-
ARRAY_SIZE(valid_atom),
-sp + 2, ep),
+ parse_ref_filter_atom(format, sp + 2,
ep),
&atomv))
return -1;
atomv->handler(atomv, &state);
@@ -2286,8 +2271,7 @@ static int parse_sorting_atom(const char *atom)
*/
struct ref_format dummy = REF_FORMAT_INIT;
const char *end = atom + strlen(atom);
- return parse_ref_filter_atom(&dummy, valid_atom,
-ARRAY_SIZE(valid_atom), atom, end);
+ return parse_ref_filter_atom(&dummy, atom, end);
}
/* If no sorting option is given, use refname to sort as default */
--
https://github.com/git/git/pull/452
[PATCH v3 13/23] ref-filter: get rid of mark_atom_in_object_info()
Remove mark_atom_in_object_info() and create same logic
in terms of ref-filter style.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
ref-filter.c | 45 +
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index 3f92a27d98b6c..34a54db168265 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -255,13 +255,29 @@ static void objectname_atom_parser(const struct
ref_format *format, struct used_
static void objectsize_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
{
if (!arg)
- ; /* default to normal object size */
+ cat_file_info->info.sizep = &cat_file_info->size;
else if (!strcmp(arg, "disk"))
cat_file_info->info.disk_sizep = &cat_file_info->disk_size;
else
die(_("urecognized %%(objectsize) argument: %s"), arg);
}
+static void objecttype_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
+{
+ if (!arg)
+ cat_file_info->info.typep = &cat_file_info->type;
+ else
+ die(_("urecognized %%(objecttype) argument: %s"), arg);
+}
+
+static void deltabase_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
+{
+ if (!arg)
+ cat_file_info->info.delta_base_sha1 =
cat_file_info->delta_base_oid.hash;
+ else
+ die(_("urecognized %%(deltabase) argument: %s"), arg);
+}
+
static void refname_atom_parser(const struct ref_format *format, struct
used_atom *atom, const char *arg)
{
refname_atom_parser_internal(&atom->u.refname, arg, atom->name);
@@ -384,10 +400,10 @@ static struct valid_atom {
static struct valid_atom valid_cat_file_atom[] = {
{ "objectname" },
- { "objecttype" },
+ { "objecttype", FIELD_STR, objecttype_atom_parser },
{ "objectsize", FIELD_ULONG, objectsize_atom_parser },
{ "rest" },
- { "deltabase" },
+ { "deltabase", FIELD_STR, deltabase_atom_parser },
};
#define REF_FORMATTING_STATE_INIT { 0, NULL }
@@ -411,25 +427,6 @@ struct atom_value {
struct used_atom *atom;
};
-static int is_atom(const char *atom, const char *s, int slen)
-{
- int alen = strlen(atom);
- return alen == slen && !memcmp(atom, s, alen);
-}
-
-static void mark_atom_in_object_info(const char *atom, int len,
- struct expand_data *data)
-{
- if (is_atom("objecttype", atom, len))
- data->info.typep = &data->type;
- else if (is_atom("objectsize", atom, len))
- data->info.sizep = &data->size;
- else if (is_atom("rest", atom, len))
- data->split_on_whitespace = 1;
- else if (is_atom("deltabase", atom, len))
- data->info.delta_base_sha1 = data->delta_base_oid.hash;
-}
-
/*
* Used to parse format string and sort specifiers
*/
@@ -496,8 +493,8 @@ static int parse_ref_filter_atom(const struct ref_format
*format,
need_tagged = 1;
if (!strcmp(valid_atom[i].name, "symref"))
need_symref = 1;
- if (cat_file_info)
- mark_atom_in_object_info(atom, atom_len, cat_file_info);
+ if (cat_file_info && !strcmp(valid_atom[i].name, "rest"))
+ cat_file_info->split_on_whitespace = 1;
return at;
}
--
https://github.com/git/git/pull/452
[PATCH v3 06/23] cat-file: split expand_atom() into 2 functions
Split expand_atom() into 2 different functions,
mark_atom_in_object_info() prepares variable for further filling,
(new) expand_atom() creates resulting string.
Need that for further reusing of formatting logic from ref-filter.
Both functions will be step-by-step removed by the end of this patch.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
builtin/cat-file.c | 73 --
1 file changed, 38 insertions(+), 35 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 37d6096d201b5..edb04a96d9bd3 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -182,47 +182,47 @@ static int is_atom(const char *atom, const char *s, int
slen)
return alen == slen && !memcmp(atom, s, alen);
}
-static void expand_atom(struct strbuf *sb, const char *atom, int len,
- void *vdata)
+static void mark_atom_in_object_info(const char *atom, int len,
+struct expand_data *data)
{
- struct expand_data *data = vdata;
+ if (is_atom("objectname", atom, len))
+ ; /* do nothing */
+ else if (is_atom("objecttype", atom, len))
+ data->info.typep = &data->type;
+ else if (is_atom("objectsize", atom, len))
+ data->info.sizep = &data->size;
+ else if (is_atom("objectsize:disk", atom, len))
+ data->info.disk_sizep = &data->disk_size;
+ else if (is_atom("rest", atom, len))
+ data->split_on_whitespace = 1;
+ else if (is_atom("deltabase", atom, len))
+ data->info.delta_base_sha1 = data->delta_base_oid.hash;
+ else
+ die("unknown format element: %.*s", len, atom);
+}
- if (is_atom("objectname", atom, len)) {
- if (!data->mark_query)
- strbuf_addstr(sb, oid_to_hex(&data->oid));
- } else if (is_atom("objecttype", atom, len)) {
- if (data->mark_query)
- data->info.typep = &data->type;
- else
- strbuf_addstr(sb, typename(data->type));
- } else if (is_atom("objectsize", atom, len)) {
- if (data->mark_query)
- data->info.sizep = &data->size;
- else
- strbuf_addf(sb, "%lu", data->size);
- } else if (is_atom("objectsize:disk", atom, len)) {
- if (data->mark_query)
- data->info.disk_sizep = &data->disk_size;
- else
- strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
- } else if (is_atom("rest", atom, len)) {
- if (data->mark_query)
- data->split_on_whitespace = 1;
- else if (data->rest)
+static void expand_atom(struct strbuf *sb, const char *atom, int len,
+struct expand_data *data)
+{
+ if (is_atom("objectname", atom, len))
+ strbuf_addstr(sb, oid_to_hex(&data->oid));
+ else if (is_atom("objecttype", atom, len))
+ strbuf_addstr(sb, typename(data->type));
+ else if (is_atom("objectsize", atom, len))
+ strbuf_addf(sb, "%lu", data->size);
+ else if (is_atom("objectsize:disk", atom, len))
+ strbuf_addf(sb, "%"PRIuMAX, (uintmax_t)data->disk_size);
+ else if (is_atom("rest", atom, len)) {
+ if (data->rest)
strbuf_addstr(sb, data->rest);
- } else if (is_atom("deltabase", atom, len)) {
- if (data->mark_query)
- data->info.delta_base_sha1 = data->delta_base_oid.hash;
- else
- strbuf_addstr(sb,
- oid_to_hex(&data->delta_base_oid));
- } else
- die("unknown format element: %.*s", len, atom);
+ } else if (is_atom("deltabase", atom, len))
+ strbuf_addstr(sb, oid_to_hex(&data->delta_base_oid));
}
-static size_t expand_format(struct strbuf *sb, const char *start, void *data)
+static size_t expand_format(struct strbuf *sb, const char *start, void *vdata)
{
const char *end;
+ struct expand_data *data = vdata;
if (*start != '(')
return 0;
@@ -230,7 +230,10 @@ static size_t expand_format(struct strbuf *sb, const char
*start, void *data)
if (!end)
die("format element '%s' does not end in ')'", start);
- expand_atom(sb, start + 1, end - start - 1, data);
+ if (data->mark_query)
+ mark_atom_in_object_info(start + 1, end - start - 1, data);
+ else
+ expand_atom(sb, start + 1, end - start - 1, data);
return end - start + 1;
}
--
https://github.com/git/git/pull/452
[PATCH v3 22/23] cat-file: tests for new atoms added
Add some tests for new formatting atoms from ref-filter.
Some of new atoms are supported automatically,
some of them are expanded into empty string
(because they are useless for some types of objects),
some of them could be supported later in other patches.
Signed-off-by: Olga Telezhnaia
Mentored-by: Christian Couder
Mentored by: Jeff King
---
t/t1006-cat-file.sh | 48
1 file changed, 48 insertions(+)
diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh
index b19f332694620..e72fcaf0e02c5 100755
--- a/t/t1006-cat-file.sh
+++ b/t/t1006-cat-file.sh
@@ -20,6 +20,19 @@ maybe_remove_timestamp () {
fi
}
+test_atom () {
+name=$1
+sha1=$2
+atoms=$3
+expected=$4
+
+test_expect_success "$name" '
+ echo "$expected" >expect &&
+ echo $sha1 | git cat-file --batch-check="$atoms" >actual &&
+ test_cmp expect actual
+'
+}
+
run_tests () {
type=$1
sha1=$2
@@ -119,6 +132,13 @@ $content"
maybe_remove_timestamp "$(cat actual.full)" $no_ts >actual &&
test_cmp expect actual
'
+
+for atom in refname parent body trailers upstream push symref flag
+do
+ test_atom "Check %($atom) gives empty output" "$sha1" "%($atom)" ""
+done
+
+test_atom "Check %(HEAD) gives only one space as output" "$sha1" '%(HEAD)'
' '
}
hello_content="Hello World"
@@ -140,6 +160,12 @@ test_expect_success '--batch-check without %(rest)
considers whole line' '
test_cmp expect actual
'
+shortname=`echo $hello_sha1 | sed 's/^.\{0\}\(.\{7\}\).*/\1/'`
+test_atom 'Check format option %(objectname:short) works' "$hello_sha1"
'%(objectname:short)' "$shortname"
+
+test_atom 'Check format option %(align) is not broken' \
+"$hello_sha1" "%(align:8)%(objecttype)%(end)%(objectname)" "blob
$hello_sha1"
+
tree_sha1=$(git write-tree)
tree_size=33
tree_pretty_content="100644 blob $hello_sha1 hello"
@@ -157,6 +183,17 @@ $commit_message"
run_tests 'commit' $commit_sha1 $commit_size "$commit_content"
"$commit_content" 1
+test_atom "Check format option %(if) is not broken" "$commit_sha1" \
+"%(if)%(author)%(then)%(objectname)%(end)" "$commit_sha1"
+test_atom "Check %(tree) works for commit" "$commit_sha1" "%(tree)"
"$tree_sha1"
+test_atom "Check %(numparent) works for commit" "$commit_sha1" "%(numparent)"
"0"
+test_atom "Check %(authorname) works for commit" "$commit_sha1"
"%(authorname)" "$GIT_AUTHOR_NAME"
+test_atom "Check %(authoremail) works for commit" "$commit_sha1"
"%(authoremail)" "<$GIT_AUTHOR_EMAIL>"
+test_atom "Check %(committername) works for commit" "$commit_sha1"
"%(committername)" "$GIT_COMMITTER_NAME"
+test_atom "Check %(committeremail) works for commit" "$commit_sha1"
"%(committeremail)" "<$GIT_COMMITTER_EMAIL>"
+test_atom "Check %(subject) works for commit" "$commit_sha1" "%(subject)"
"$commit_message"
+test_atom "Check %(contents) works for commit" "$commit_sha1" "%(contents)"
"$commit_message"
+
tag_header_without_timestamp="object $hello_sha1
type blob
tag hellotag
@@ -171,6 +208,17 @@ tag_size=$(strlen "$tag_content")
run_tests 'tag' $tag_sha1 $tag_size "$tag_content" "$tag_content" 1
+test_atom "Check %(object) works for tag" "$tag_sha1" "%(object)" "$hello_sha1"
+test_atom "Check %(type) works for tag" "$tag_sha1" "%(type)" "blob"
+test_atom "Check %(tag) works for tag" "$tag_sha1" "%(tag)" "hellotag"
+test_atom "Check %(taggername) works for tag" "$tag_sha1" "%(taggername)"
"$GIT_COMMITTER_NAME"
+test_atom "Check %(taggeremail) works for tag" "$tag_sha1" "%(taggeremail)"
"<$GIT_COMMITTER_EMAIL>"
+test_atom "Check %(subject) works for tag" "$tag_sha1" "%(subject)"
"$tag_description"
+test_atom "Check %(contents) works for tag" "$tag_sha1" "%(contents)"
"$tag_description"
+
+test_atom "Check %(color) gives no additional output" "$sha1" \
+"%(objectname) %(color:green) %(objecttype)" "$sha1 $type"
+
test_expect_success \
"Reach a blob from a tag pointing to it" \
"test '$hello_content' = \"\$(git cat-file blob $tag_sha1)\""
--
https://github.com/git/git/pull/452
Re: [PATCH v3 05/12] sequencer: introduce the `merge` command
On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin wrote: > This patch is part of the effort to reimplement `--preserve-merges` with > a substantially improved design, a design that has been developed in the > Git for Windows project to maintain the dozens of Windows-specific patch > series on top of upstream Git. > > The previous patch implemented the `label` and `reset` commands to label > commits and to reset to a labeled commits. This patch adds the `merge` s/to a/to/ > command, with the following syntax: > > merge [-C ] # > > The parameter in this instance is the *original* merge commit, > whose author and message will be used for the merge commit that is about > to be created. > > The parameter refers to the (possibly rewritten) revision to > merge. Let's see an example of a todo list: > > label onto > > # Branch abc > reset onto > pick deadbeef Hello, world! > label abc > > reset onto > pick cafecafe And now for something completely different > merge -C baaabaaa abc # Merge the branch 'abc' into master > > To edit the merge commit's message (a "reword" for merges, if you will), > use `-c` (lower-case) instead of `-C`; this convention was borrowed from > `git commit` that also supports `-c` and `-C` with similar meanings. > > To create *new* merges, i.e. without copying the commit message from an > existing commit, simply omit the `-C ` parameter (which will > open an editor for the merge message): > > merge abc > > This comes in handy when splitting a branch into two or more branches. > > Note: this patch only adds support for recursive merges, to keep things > simple. Support for octopus merges will be added later in a separate > patch series, support for merges using strategies other than the > recursive merge is left for the future. > > Signed-off-by: Johannes Schindelin
Re: [PATCH 6/7] worktree remove: new command
On Fri, Feb 2, 2018 at 6:47 PM, Eric Sunshine wrote:
> On Wed, Jan 24, 2018 at 4:53 AM, Nguyễn Thái Ngọc Duy
> wrote:
>> This command allows to delete a worktree. Like 'move' you cannot
>> remove the main worktree, or one with submodules inside [1].
>> [...]
>> Signed-off-by: Nguyễn Thái Ngọc Duy
>> ---
>> diff --git a/builtin/worktree.c b/builtin/worktree.c
>> @@ -688,6 +689,132 @@ static int move_worktree(int ac, const char **av,
>> const char *prefix)
>> +static void check_clean_worktree(struct worktree *wt,
>> +const char *original_path)
>> +{
>> + [...]
>> + validate_no_submodules(wt);
>
> It's slightly strange seeing worktree validation in a function
> checking whether the worktree is clean since submodule validation
> isn't an issue of cleanliness. I'd have expected the caller to invoke
> it instead:
>
> int remove_worktree(...) {
> ...
> if (!force) {
> validate_no_submodules(wt);
> check_clean_worktree(wt, av[0]);
> }
> ...
> }
>
> On the other hand, I could imagine it being called here as appropriate
> if submodule validation eventually also checks submodule cleanliness
> as hinted by the commit message.
Yes I basically consider all submodules dirty until somebody sorts
them out. I'll add a comment here to clarify this.
--
Duy
[PATCH v2 5/7] worktree move: refuse to move worktrees with submodules
Submodules contains .git files with relative paths. After a worktree
move, these files need to be updated or they may point to nowhere.
This is a bandage patch to make sure "worktree move" don't break
people's worktrees by accident. When .git file update code is in
place, this validate_no_submodules() could be removed.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-worktree.txt | 2 +-
builtin/worktree.c | 23 +++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 4fa1dd3a48..e6764ee8e0 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -79,7 +79,7 @@ with `--reason`.
move::
Move a working tree to a new location. Note that the main working tree
-cannot be moved.
+or linked working trees containing submodules cannot be moved.
prune::
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 6fe41313c9..4789cebe11 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -606,6 +606,27 @@ static int unlock_worktree(int ac, const char **av, const
char *prefix)
return ret;
}
+static void validate_no_submodules(const struct worktree *wt)
+{
+ struct index_state istate = { NULL };
+ int i, found_submodules = 0;
+
+ if (read_index_from(&istate, worktree_git_path(wt, "index")) > 0) {
+ for (i = 0; i < istate.cache_nr; i++) {
+ struct cache_entry *ce = istate.cache[i];
+
+ if (S_ISGITLINK(ce->ce_mode)) {
+ found_submodules = 1;
+ break;
+ }
+ }
+ }
+ discard_index(&istate);
+
+ if (found_submodules)
+ die(_("working trees containing submodules cannot be moved"));
+}
+
static int move_worktree(int ac, const char **av, const char *prefix)
{
struct option options[] = {
@@ -643,6 +664,8 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
if (file_exists(dst.buf))
die(_("target '%s' already exists"), dst.buf);
+ validate_no_submodules(wt);
+
reason = is_worktree_locked(wt);
if (reason) {
if (*reason)
--
2.16.1.399.g632f88eed1
[PATCH v2 6/7] worktree remove: new command
This command allows to delete a worktree. Like 'move' you cannot
remove the main worktree, or one with submodules inside [1].
For deleting $GIT_WORK_TREE, Untracked files or any staged entries are
considered precious and therefore prevent removal by default. Ignored
files are not precious.
When it comes to deleting $GIT_DIR, there's no "clean" check because
there should not be any valuable data in there, except:
- HEAD reflog. There is nothing we can do about this until somebody
steps up and implements the ref graveyard.
- Detached HEAD. Technically it can still be recovered. Although it
may be nice to warn about orphan commits like 'git checkout' does.
[1] We do 'git status' with --ignore-submodules=all for safety
anyway. But this needs a closer look by submodule people before we
can allow deletion. For example, if a submodule is totally clean,
but its repo not absorbed to the main .git dir, then deleting
worktree also deletes the valuable .submodule repo too.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-worktree.txt | 21 ++--
builtin/worktree.c | 134 -
contrib/completion/git-completion.bash | 5 +-
t/t2028-worktree-move.sh | 30 ++
4 files changed, 179 insertions(+), 11 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index e6764ee8e0..d322acbc67 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -14,6 +14,7 @@ SYNOPSIS
'git worktree lock' [--reason ]
'git worktree move'
'git worktree prune' [-n] [-v] [--expire ]
+'git worktree remove' [--force]
'git worktree unlock'
DESCRIPTION
@@ -85,6 +86,13 @@ prune::
Prune working tree information in $GIT_DIR/worktrees.
+remove::
+
+Remove a working tree. Only clean working trees (no untracked files
+and no modification in tracked files) can be removed. Unclean working
+trees or ones with submodules can be removed with `--force`. The main
+working tree cannot be removed.
+
unlock::
Unlock a working tree, allowing it to be pruned, moved or deleted.
@@ -94,9 +102,10 @@ OPTIONS
-f::
--force::
- By default, `add` refuses to create a new working tree when
`` is a branch name and
- is already checked out by another working tree. This option overrides
- that safeguard.
+ By default, `add` refuses to create a new working tree when
+ `` is a branch name and is already checked out by
+ another working tree and `remove` refuses to remove an unclean
+ working tree. This option overrides that safeguard.
-b ::
-B ::
@@ -278,12 +287,6 @@ Multiple checkout in general is still experimental, and
the support
for submodules is incomplete. It is NOT recommended to make multiple
checkouts of a superproject.
-git-worktree could provide more automation for tasks currently
-performed manually, such as:
-
-- `remove` to remove a linked working tree and its administrative files (and
- warn if the working tree is dirty)
-
GIT
---
Part of the linkgit:git[1] suite
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 4789cebe11..990e47b315 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -19,6 +19,7 @@ static const char * const worktree_usage[] = {
N_("git worktree lock [] "),
N_("git worktree move "),
N_("git worktree prune []"),
+ N_("git worktree remove [] "),
N_("git worktree unlock "),
NULL
};
@@ -624,7 +625,7 @@ static void validate_no_submodules(const struct worktree
*wt)
discard_index(&istate);
if (found_submodules)
- die(_("working trees containing submodules cannot be moved"));
+ die(_("working trees containing submodules cannot be moved or
removed"));
}
static int move_worktree(int ac, const char **av, const char *prefix)
@@ -688,6 +689,135 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
return 0;
}
+/*
+ * Note, "git status --porcelain" is used to determine if it's safe to
+ * delete a whole worktree. "git status" does not ignore user
+ * configuration, so if a normal "git status" shows "clean" for the
+ * user, then it's ok to remove it.
+ *
+ * This assumption may be a bad one. We may want to ignore
+ * (potentially bad) user settings and only delete a worktree when
+ * it's absolutely safe to do so from _our_ point of view because we
+ * know better.
+ */
+static void check_clean_worktree(struct worktree *wt,
+const char *original_path)
+{
+ struct argv_array child_env = ARGV_ARRAY_INIT;
+ struct child_process cp;
+ char buf[1];
+ int ret;
+
+ /*
+* Until we sort this out, all submodules are "dirty" and
+* will abort this function.
+*/
+ validate_no_submodules(wt);
+
+ argv_array_pushf(&child_env, "%s=%s/.git",
+GIT_DIR_ENVIRONMENT, wt->path);
[PATCH v2 2/7] worktree.c: add update_worktree_location()
Signed-off-by: Nguyễn Thái Ngọc Duy
---
worktree.c | 17 +
worktree.h | 6 ++
2 files changed, 23 insertions(+)
diff --git a/worktree.c b/worktree.c
index b238d87bf1..0373faf0dc 100644
--- a/worktree.c
+++ b/worktree.c
@@ -326,6 +326,23 @@ int validate_worktree(const struct worktree *wt, struct
strbuf *errmsg)
return ret;
}
+void update_worktree_location(struct worktree *wt, const char *path_)
+{
+ struct strbuf path = STRBUF_INIT;
+
+ if (is_main_worktree(wt))
+ die("BUG: can't relocate main worktree");
+
+ strbuf_realpath(&path, path_, 1);
+ if (fspathcmp(wt->path, path.buf)) {
+ write_file(git_common_path("worktrees/%s/gitdir", wt->id),
+ "%s/.git", path.buf);
+ free(wt->path);
+ wt->path = strbuf_detach(&path, NULL);
+ }
+ strbuf_release(&path);
+}
+
int is_worktree_being_rebased(const struct worktree *wt,
const char *target)
{
diff --git a/worktree.h b/worktree.h
index cb577de8cd..a913428c3d 100644
--- a/worktree.h
+++ b/worktree.h
@@ -68,6 +68,12 @@ extern const char *is_worktree_locked(struct worktree *wt);
extern int validate_worktree(const struct worktree *wt,
struct strbuf *errmsg);
+/*
+ * Update worktrees/xxx/gitdir with the new path.
+ */
+extern void update_worktree_location(struct worktree *wt,
+const char *path_);
+
/*
* Free up the memory for worktree(s)
*/
--
2.16.1.399.g632f88eed1
[PATCH v2 0/7] nd/worktree-move reboot
v2 basically fixes lots of comments from Eric (many thanks!): memory
leak, typos, document updates, tests, corner case fixes.
Interdiff:
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 6f83723d9a..d322acbc67 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -36,10 +36,6 @@ The working tree's administrative files in the repository
(see
`git worktree prune` in the main or any linked working tree to
clean up any stale administrative files.
-If you move a linked working tree, you need to manually update the
-administrative files so that they do not get pruned automatically. See
-section "DETAILS" for more information.
-
If a linked working tree is stored on a portable device or network share
which is not always mounted, you can prevent its administrative files from
being pruned by issuing the `git worktree lock` command, optionally
@@ -211,7 +207,7 @@ thumb is do not make any assumption about whether a path
belongs to
$GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
-If you move a linked working tree, you need to update the 'gitdir' file
+If you manually move a linked working tree, you need to update the 'gitdir'
file
in the entry's directory. For example, if a linked working tree is moved
to `/newpath/test-next` and its `.git` file points to
`/path/main/.git/worktrees/test-next`, then update
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8ce86aef0e..f77ef994c4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -653,21 +653,19 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
die(_("'%s' is not a working tree"), av[0]);
if (is_main_worktree(wt))
die(_("'%s' is a main working tree"), av[0]);
-
- validate_no_submodules(wt);
-
if (is_directory(dst.buf)) {
const char *sep = find_last_dir_sep(wt->path);
if (!sep)
die(_("could not figure out destination name from
'%s'"),
wt->path);
+ strbuf_trim_trailing_dir_sep(&dst);
strbuf_addstr(&dst, sep);
- if (file_exists(dst.buf))
- die(_("target '%s' already exists"), dst.buf);
- } else if (file_exists(dst.buf)) {
- die(_("target '%s' already exists"), av[1]);
}
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), dst.buf);
+
+ validate_no_submodules(wt);
reason = is_worktree_locked(wt);
if (reason) {
@@ -677,7 +675,7 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
die(_("cannot move a locked working tree"));
}
if (validate_worktree(wt, &errmsg, 0))
- die(_("validation failed, cannot move working tree:\n%s"),
+ die(_("validation failed, cannot move working tree: %s"),
errmsg.buf);
strbuf_release(&errmsg);
@@ -686,6 +684,8 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
update_worktree_location(wt, dst.buf);
+ strbuf_release(&dst);
+ free_worktrees(worktrees);
return 0;
}
@@ -708,6 +708,10 @@ static void check_clean_worktree(struct worktree *wt,
char buf[1];
int ret;
+ /*
+* Until we sort this out, all submodules are "dirty" and
+* will abort this function.
+*/
validate_no_submodules(wt);
argv_array_pushf(&child_env, "%s=%s/.git",
@@ -724,7 +728,7 @@ static void check_clean_worktree(struct worktree *wt,
cp.out = -1;
ret = start_command(&cp);
if (ret)
- die_errno(_("failed to run git-status on '%s'"),
+ die_errno(_("failed to run 'git status' on '%s'"),
original_path);
ret = xread(cp.out, buf, sizeof(buf));
if (ret)
@@ -733,7 +737,7 @@ static void check_clean_worktree(struct worktree *wt,
close(cp.out);
ret = finish_command(&cp);
if (ret)
- die_errno(_("failed to run git-status on '%s', code %d"),
+ die_errno(_("failed to run 'git status' on '%s', code %d"),
original_path, ret);
}
@@ -748,7 +752,6 @@ static int delete_git_work_tree(struct worktree *wt)
ret = -1;
}
strbuf_release(&sb);
-
return ret;
}
@@ -797,7 +800,7 @@ static int remove_worktree(int ac, const char **av, const
char *prefix)
die(_("cannot remove a locked working tree"));
}
if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
- die(_("validation failed, cannot remove working tree:\n%s"),
+ die(_("validation failed, cannot remove working tree: %s"),
errmsg
[PATCH v2 4/7] worktree move: accept destination as directory
Similar to "mv a b/", which is actually "mv a b/a", we extract basename
of source worktree and create a directory of the same name at
destination if dst path is a directory.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/worktree.c | 11 ++-
strbuf.c | 8
strbuf.h | 3 +++
t/t2028-worktree-move.sh | 11 +++
4 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 8b9482d1e5..6fe41313c9 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -631,8 +631,17 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
die(_("'%s' is not a working tree"), av[0]);
if (is_main_worktree(wt))
die(_("'%s' is a main working tree"), av[0]);
+ if (is_directory(dst.buf)) {
+ const char *sep = find_last_dir_sep(wt->path);
+
+ if (!sep)
+ die(_("could not figure out destination name from
'%s'"),
+ wt->path);
+ strbuf_trim_trailing_dir_sep(&dst);
+ strbuf_addstr(&dst, sep);
+ }
if (file_exists(dst.buf))
- die(_("target '%s' already exists"), av[1]);
+ die(_("target '%s' already exists"), dst.buf);
reason = is_worktree_locked(wt);
if (reason) {
diff --git a/strbuf.c b/strbuf.c
index 1df674e919..46930ad027 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -95,6 +95,7 @@ void strbuf_trim(struct strbuf *sb)
strbuf_rtrim(sb);
strbuf_ltrim(sb);
}
+
void strbuf_rtrim(struct strbuf *sb)
{
while (sb->len > 0 && isspace((unsigned char)sb->buf[sb->len - 1]))
@@ -102,6 +103,13 @@ void strbuf_rtrim(struct strbuf *sb)
sb->buf[sb->len] = '\0';
}
+void strbuf_trim_trailing_dir_sep(struct strbuf *sb)
+{
+ while (sb->len > 0 && is_dir_sep((unsigned char)sb->buf[sb->len - 1]))
+ sb->len--;
+ sb->buf[sb->len] = '\0';
+}
+
void strbuf_ltrim(struct strbuf *sb)
{
char *b = sb->buf;
diff --git a/strbuf.h b/strbuf.h
index 14c8c10d66..e6cae5f439 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -179,6 +179,9 @@ extern void strbuf_trim(struct strbuf *);
extern void strbuf_rtrim(struct strbuf *);
extern void strbuf_ltrim(struct strbuf *);
+/* Strip trailing directory separators */
+extern void strbuf_trim_trailing_dir_sep(struct strbuf *);
+
/**
* Replace the contents of the strbuf with a reencoded form. Returns -1
* on error, 0 on success.
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 0f8abc0854..deb486cd8e 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -85,4 +85,15 @@ test_expect_success 'move main worktree' '
test_must_fail git worktree move . def
'
+test_expect_success 'move worktree to another dir' '
+ toplevel="$(pwd)" &&
+ mkdir some-dir &&
+ git worktree move destination some-dir &&
+ test_path_is_missing source &&
+ git worktree list --porcelain | grep "^worktree.*/some-dir/destination"
&&
+ git -C some-dir/destination log --format=%s >actual2 &&
+ echo init >expected2 &&
+ test_cmp expected2 actual2
+'
+
test_done
--
2.16.1.399.g632f88eed1
[PATCH v2 7/7] worktree remove: allow it when $GIT_WORK_TREE is already gone
"git worktree remove" basically consists of two things
- delete $GIT_WORK_TREE
- delete $GIT_DIR (which is $SUPER_GIT_DIR/worktrees/something)
If $GIT_WORK_TREE is already gone for some reason, we should be able
to finish the job by deleting $GIT_DIR.
Two notes:
- $GIT_WORK_TREE _can_ be missing if the worktree is locked. In that
case we must not delete $GIT_DIR because the real $GIT_WORK_TREE may
be in a usb stick somewhere. This is already handled because we
check for lock first.
- validate_worktree() is still called because it may do more checks in
future (and it already does something else, like checking main
worktree, but that's irrelevant in this case)
Noticed-by: Kaartic Sivaraam
Signed-off-by: Nguyễn Thái Ngọc Duy
---
builtin/worktree.c | 12 +++-
t/t2028-worktree-move.sh | 17 +
worktree.c | 9 -
worktree.h | 5 -
4 files changed, 36 insertions(+), 7 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 990e47b315..f77ef994c4 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -674,7 +674,7 @@ static int move_worktree(int ac, const char **av, const
char *prefix)
reason);
die(_("cannot move a locked working tree"));
}
- if (validate_worktree(wt, &errmsg))
+ if (validate_worktree(wt, &errmsg, 0))
die(_("validation failed, cannot move working tree: %s"),
errmsg.buf);
strbuf_release(&errmsg);
@@ -799,15 +799,17 @@ static int remove_worktree(int ac, const char **av, const
char *prefix)
reason);
die(_("cannot remove a locked working tree"));
}
- if (validate_worktree(wt, &errmsg))
+ if (validate_worktree(wt, &errmsg, WT_VALIDATE_WORKTREE_MISSING_OK))
die(_("validation failed, cannot remove working tree: %s"),
errmsg.buf);
strbuf_release(&errmsg);
- if (!force)
- check_clean_worktree(wt, av[0]);
+ if (file_exists(wt->path)) {
+ if (!force)
+ check_clean_worktree(wt, av[0]);
- ret |= delete_git_work_tree(wt);
+ ret |= delete_git_work_tree(wt);
+ }
/*
* continue on even if ret is non-zero, there's no going back
* from here.
diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh
index 4718c4552f..082368d8c6 100755
--- a/t/t2028-worktree-move.sh
+++ b/t/t2028-worktree-move.sh
@@ -126,4 +126,21 @@ test_expect_success 'force remove worktree with untracked
file' '
test_path_is_missing destination
'
+test_expect_success 'remove missing worktree' '
+ git worktree add to-be-gone &&
+ test -d .git/worktrees/to-be-gone &&
+ mv to-be-gone gone &&
+ git worktree remove to-be-gone &&
+ test_path_is_missing .git/worktrees/to-be-gone
+'
+
+test_expect_success 'NOT remove missing-but-locked worktree' '
+ git worktree add gone-but-locked &&
+ git worktree lock gone-but-locked &&
+ test -d .git/worktrees/gone-but-locked &&
+ mv gone-but-locked really-gone-now &&
+ test_must_fail git worktree remove gone-but-locked &&
+ test_path_is_dir .git/worktrees/gone-but-locked
+'
+
test_done
diff --git a/worktree.c b/worktree.c
index 0373faf0dc..28989cf06e 100644
--- a/worktree.c
+++ b/worktree.c
@@ -267,7 +267,8 @@ static void strbuf_addf_gently(struct strbuf *buf, const
char *fmt, ...)
va_end(params);
}
-int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg,
+ unsigned flags)
{
struct strbuf wt_path = STRBUF_INIT;
char *path = NULL;
@@ -303,6 +304,12 @@ int validate_worktree(const struct worktree *wt, struct
strbuf *errmsg)
goto done;
}
+ if (flags & WT_VALIDATE_WORKTREE_MISSING_OK &&
+ !file_exists(wt->path)) {
+ ret = 0;
+ goto done;
+ }
+
if (!file_exists(wt_path.buf)) {
strbuf_addf_gently(errmsg, _("'%s' does not exist"),
wt_path.buf);
goto done;
diff --git a/worktree.h b/worktree.h
index a913428c3d..fe38ce10c3 100644
--- a/worktree.h
+++ b/worktree.h
@@ -61,12 +61,15 @@ extern int is_main_worktree(const struct worktree *wt);
*/
extern const char *is_worktree_locked(struct worktree *wt);
+#define WT_VALIDATE_WORKTREE_MISSING_OK (1 << 0)
+
/*
* Return zero if the worktree is in good condition. Error message is
* returned if "errmsg" is not NULL.
*/
extern int validate_worktree(const struct worktree *wt,
-struct strbuf *errmsg);
+struct strbuf *errmsg,
+unsigned flags);
/*
* Update worktrees/xxx/gitdir with the new path.
--
2.16.1.399.g632f88eed
[PATCH v2 1/7] worktree.c: add validate_worktree()
This function is later used by "worktree move" and "worktree remove"
to ensure that we have a good connection between the repository and
the worktree. For example, if a worktree is moved manually, the
worktree location recorded in $GIT_DIR/worktrees/.../gitdir is
incorrect and we should not move that one.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
worktree.c | 72 ++
worktree.h | 9 +++
2 files changed, 81 insertions(+)
diff --git a/worktree.c b/worktree.c
index f5da7d286d..b238d87bf1 100644
--- a/worktree.c
+++ b/worktree.c
@@ -254,6 +254,78 @@ const char *is_worktree_locked(struct worktree *wt)
return wt->lock_reason;
}
+/* convenient wrapper to deal with NULL strbuf */
+static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
+{
+ va_list params;
+
+ if (!buf)
+ return;
+
+ va_start(params, fmt);
+ strbuf_vaddf(buf, fmt, params);
+ va_end(params);
+}
+
+int validate_worktree(const struct worktree *wt, struct strbuf *errmsg)
+{
+ struct strbuf wt_path = STRBUF_INIT;
+ char *path = NULL;
+ int err, ret = -1;
+
+ strbuf_addf(&wt_path, "%s/.git", wt->path);
+
+ if (is_main_worktree(wt)) {
+ if (is_directory(wt_path.buf)) {
+ ret = 0;
+ goto done;
+ }
+ /*
+* Main worktree using .git file to point to the
+* repository would make it impossible to know where
+* the actual worktree is if this function is executed
+* from another worktree. No .git file support for now.
+*/
+ strbuf_addf_gently(errmsg,
+ _("'%s' at main working tree is not the
repository directory"),
+ wt_path.buf);
+ goto done;
+ }
+
+ /*
+* Make sure "gitdir" file points to a real .git file and that
+* file points back here.
+*/
+ if (!is_absolute_path(wt->path)) {
+ strbuf_addf_gently(errmsg,
+ _("'%s' file does not contain absolute path
to the working tree location"),
+ git_common_path("worktrees/%s/gitdir",
wt->id));
+ goto done;
+ }
+
+ if (!file_exists(wt_path.buf)) {
+ strbuf_addf_gently(errmsg, _("'%s' does not exist"),
wt_path.buf);
+ goto done;
+ }
+
+ path = xstrdup_or_null(read_gitfile_gently(wt_path.buf, &err));
+ if (!path) {
+ strbuf_addf_gently(errmsg, _("'%s' is not a .git file, error
code %d"),
+ wt_path.buf, err);
+ goto done;
+ }
+
+ ret = fspathcmp(path, real_path(git_common_path("worktrees/%s",
wt->id)));
+
+ if (ret)
+ strbuf_addf_gently(errmsg, _("'%s' does not point back to
'%s'"),
+ wt->path, git_common_path("worktrees/%s",
wt->id));
+done:
+ free(path);
+ strbuf_release(&wt_path);
+ return ret;
+}
+
int is_worktree_being_rebased(const struct worktree *wt,
const char *target)
{
diff --git a/worktree.h b/worktree.h
index c28a880e18..cb577de8cd 100644
--- a/worktree.h
+++ b/worktree.h
@@ -3,6 +3,8 @@
#include "refs.h"
+struct strbuf;
+
struct worktree {
char *path;
char *id;
@@ -59,6 +61,13 @@ extern int is_main_worktree(const struct worktree *wt);
*/
extern const char *is_worktree_locked(struct worktree *wt);
+/*
+ * Return zero if the worktree is in good condition. Error message is
+ * returned if "errmsg" is not NULL.
+ */
+extern int validate_worktree(const struct worktree *wt,
+struct strbuf *errmsg);
+
/*
* Free up the memory for worktree(s)
*/
--
2.16.1.399.g632f88eed1
[PATCH v2 3/7] worktree move: new command
This command allows to relocate linked worktrees. Main worktree cannot
(yet) be moved.
There are two options to move the main worktree, but both have
complications, so it's not implemented yet. Anyway the options are:
- convert the main worktree to a linked one and move it away, leave
the git repository where it is. The repo essentially becomes bare
after this move.
- move the repository with the main worktree. The tricky part is make
sure all file descriptors to the repository are closed, or it may
fail on Windows.
Signed-off-by: Nguyễn Thái Ngọc Duy
---
Documentation/git-worktree.txt | 13 ---
builtin/worktree.c | 53 ++
contrib/completion/git-completion.bash | 2 +-
t/t2028-worktree-move.sh | 26 +
4 files changed, 87 insertions(+), 7 deletions(-)
diff --git a/Documentation/git-worktree.txt b/Documentation/git-worktree.txt
index 41585f535d..4fa1dd3a48 100644
--- a/Documentation/git-worktree.txt
+++ b/Documentation/git-worktree.txt
@@ -12,6 +12,7 @@ SYNOPSIS
'git worktree add' [-f] [--detach] [--checkout] [--lock] [-b ]
[]
'git worktree list' [--porcelain]
'git worktree lock' [--reason ]
+'git worktree move'
'git worktree prune' [-n] [-v] [--expire ]
'git worktree unlock'
@@ -34,10 +35,6 @@ The working tree's administrative files in the repository
(see
`git worktree prune` in the main or any linked working tree to
clean up any stale administrative files.
-If you move a linked working tree, you need to manually update the
-administrative files so that they do not get pruned automatically. See
-section "DETAILS" for more information.
-
If a linked working tree is stored on a portable device or network share
which is not always mounted, you can prevent its administrative files from
being pruned by issuing the `git worktree lock` command, optionally
@@ -79,6 +76,11 @@ files from being pruned automatically. This also prevents it
from
being moved or deleted. Optionally, specify a reason for the lock
with `--reason`.
+move::
+
+Move a working tree to a new location. Note that the main working tree
+cannot be moved.
+
prune::
Prune working tree information in $GIT_DIR/worktrees.
@@ -196,7 +198,7 @@ thumb is do not make any assumption about whether a path
belongs to
$GIT_DIR or $GIT_COMMON_DIR when you need to directly access something
inside $GIT_DIR. Use `git rev-parse --git-path` to get the final path.
-If you move a linked working tree, you need to update the 'gitdir' file
+If you manually move a linked working tree, you need to update the 'gitdir'
file
in the entry's directory. For example, if a linked working tree is moved
to `/newpath/test-next` and its `.git` file points to
`/path/main/.git/worktrees/test-next`, then update
@@ -281,7 +283,6 @@ performed manually, such as:
- `remove` to remove a linked working tree and its administrative files (and
warn if the working tree is dirty)
-- `mv` to move or rename a working tree and update its administrative files
GIT
---
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 7cef5b120b..8b9482d1e5 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -17,6 +17,7 @@ static const char * const worktree_usage[] = {
N_("git worktree add [] []"),
N_("git worktree list []"),
N_("git worktree lock [] "),
+ N_("git worktree move "),
N_("git worktree prune []"),
N_("git worktree unlock "),
NULL
@@ -605,6 +606,56 @@ static int unlock_worktree(int ac, const char **av, const
char *prefix)
return ret;
}
+static int move_worktree(int ac, const char **av, const char *prefix)
+{
+ struct option options[] = {
+ OPT_END()
+ };
+ struct worktree **worktrees, *wt;
+ struct strbuf dst = STRBUF_INIT;
+ struct strbuf errmsg = STRBUF_INIT;
+ const char *reason;
+ char *path;
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac != 2)
+ usage_with_options(worktree_usage, options);
+
+ path = prefix_filename(prefix, av[1]);
+ strbuf_addstr(&dst, path);
+ free(path);
+
+ worktrees = get_worktrees(0);
+ wt = find_worktree(worktrees, prefix, av[0]);
+ if (!wt)
+ die(_("'%s' is not a working tree"), av[0]);
+ if (is_main_worktree(wt))
+ die(_("'%s' is a main working tree"), av[0]);
+ if (file_exists(dst.buf))
+ die(_("target '%s' already exists"), av[1]);
+
+ reason = is_worktree_locked(wt);
+ if (reason) {
+ if (*reason)
+ die(_("cannot move a locked working tree, lock reason:
%s"),
+ reason);
+ die(_("cannot move a locked working tree"));
+ }
+ if (validate_worktree(wt, &errmsg))
+ die(_("validation failed, cannot move working tree: %s"),
+ errmsg
Re: [PATCH 3/7] worktree move: new command
On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren wrote: > On 6 February 2018 at 03:13, Jeff King wrote: >> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote: >>> I learned SANITIZE=leak today! It not only catches this but also "dst". >>> >>> Jeff is there any ongoing effort to make the test suite pass with >>> SANITIZE=leak? My t2038 passed, so I went ahead with the full test >>> suite and saw so many failures. I did see in your original mails that >>> you focused on t and t0001 only.. >> >> Yeah, I did those two scripts to try to prove to myself that the >> approach was good. But I haven't really pushed it any further. >> >> Martin Ågren (cc'd) did some follow-up work, but I think we still have a >> long way to go. > > Agreed. :-) Should we mark the test files that pass SANITIZE=leak and run as a sub testsuite? This way we can start growing it until it covers everything (unlikely) and make sure people don't break what's already passed. Of course I don't expect everybody to run this new make target with SANITIZE=leak. Travis can do that for us and hopefully have some way to tell git@vger about new breakages. -- Duy
Re: What's cooking in git.git (Jan 2018, #04; Wed, 31)
On Sun, Feb 11, 2018 at 9:44 PM, Ævar Arnfjörð Bjarmason wrote: > > On Sat, Feb 10 2018, Duy Nguyen jotted: > >> On Sat, Feb 10, 2018 at 1:37 AM, Ævar Arnfjörð Bjarmason >> wrote: >>> >>> On Thu, Feb 01 2018, Junio C. Hamano jotted: >>> * nd/fix-untracked-cache-invalidation (2018-01-24) 5 commits - dir.c: stop ignoring opendir() error in open_cached_dir() - update-index doc: note a fixed bug in the untracked cache - dir.c: fix missing dir invalidation in untracked code - dir.c: avoid stat() in valid_cached_dir() - status: add a failing test showing a core.untrackedCache bug Some bugs around "untracked cache" feature have been fixed. Will merge to 'next'. >>> >>> I think you / Nguyễn may not have seen my >>> <87d11omi2o@evledraar.gmail.com> >>> (https://public-inbox.org/git/87d11omi2o@evledraar.gmail.com/) >> >> I have. But since you wrote "I haven't found... yet", I assumed you >> were still on it. You didn't give me much info to follow anyway. > > Haven't had time to dig further, sorry, and won't be able to share the > repository. Is there some UC inspection command that can be run on the > relevant path / other thing that'll be indicative of what went wrong? There's test-dump-untracked-cache that will give you all the data. >From then on, you may need to dig in the code a bit to see how that data should be processed. There's no obfuscation support in that command, unfortunately, so you can't just send me the dump. But if you could limit it to a few "blocks" related to the error messages, then manual obfuscation should not take that much time (either that or just add obfuscation in test-dump-untracked-cache.c, it's probably easier task; or I can do this for you) >>> As noted there I think it's best to proceed without the "dir.c: stop >>> ignoring opendir[...]" patch. >>> >>> It's going to be a bad regression in 2.17 if it ends up spewing pagefuls >>> of warnings in previously working repos if the UC is on. >> >> "previously working" is a strong word when opendir() starts to >> complain. Sure we can suppress/ignore the error messages but I don't >> think it's a healthy attitude. Unreported bugs can't be fixed. > > I mean that for the user it returned the right "git status" info and > didn't print errors, but yeah, the index may have been in some > internally funny state. One question (I wasn't clear from your previous mail). Does "git status" always report the same errors when run multiple times, or does it just report once, then next "git status" is silent? I suppose it's the former case.. -- Duy
i wait your respond
-- Greetings, I wonder why you continue neglecting my emails. Please, acknowledge the receipt of this message in reference to the subject above as I intend to send to you the details of the project. Sometimes, try to check your spam box because most of these correspondences fall out sometimes in SPAM folder. Best regards, Mr. Aziz Ibrahim
Re: "git bisect run make" adequate to locate first unbuildable commit?
On Fri, 9 Feb 2018, Philip Oakley wrote: > From: "Robert P. J. Day" > > On Fri, 9 Feb 2018, Philip Oakley, CEng MIET wrote: > (apologies for using the fancy letters after the name ID...) > > > >> From: "Robert P. J. Day" > >> > > >> > writing a short tutorial on "git bisect" and, all the details > >> > of special exit code 125 aside, if one wanted to locate the > >> > first unbuildable commit, would it be sufficient to just run? > >> > > >> > $ git bisect run make > >> > > >> > as i read it, make returns either 0, 1 or 2 so there doesn't > >> > appear to be any possibility of weirdness with clashing with a > >> > 125 exit code. am i overlooking some subtle detail here i > >> > should be aware of? thanks. > >> > >> In the spirit of pedanticism, one should also clarify the word > >> "first", in that it's not a linear search for _an_ unbuildable > >> commit, but that one is looking for the transition between an > >> unbroken sequence of unbuildable commits, which transitions to > >> buildable commits, and its the transition that is sought. (there > >> could be many random unbuildable commits within a sequence in > >> some folks' processes!) > > > > quite so, i should have been more precise. > > > > rday > > The other two things that may be happening (in the wider bisect > discussion) that I've heard of are: > 1. there may be feature branches that bypass the known good starting >commit, which can cause understanding issues as those side >branches that predate the start point are also considered >potential bu commits. ok, but let's make sure i understand what defines a possible commit that "introduces" the bug. if i define two bisection commits and , then i've always assumed that what i'm after is a commit for which: 1) is reachable from 2) is reachable from this seems fairly obvious. now, as you suggest, it's possible that the "bug" was introduced on a feature branch that bypasses my choice of but, at *some* point, that feature branch would have to be merged to the point where it was now reachable from and, in the context of bisection, *that* merge commit would represent where the bug was introduced, no? rday
Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache
On Wed, Feb 7, 2018 at 9:13 PM, Ben Peart wrote: > > > On 2/6/2018 7:27 AM, Duy Nguyen wrote: >> >> >> This is another thing that bugs me. I know you're talking about huge >> index files, but at what size should we start this sort of >> optimization? Writing down a few MBs on linux is cheap enough that I >> won't bother optimizing (and I get my UNTR extension repaired all the >> time, so reduced lstat calls and stuff). This "typically" only comes >> at certain size, what size? >> > > It's important to identify what we're trading off here. With the proposed > optimization, we'll end up doing less writes of the index but potentially > more lstat calls. Even with a small index, writing the index is much more > expensive than calling lstat on a file. Exactly how much more expensive > depends on a lot of variables but even with a SSD its still orders of > magnitude difference. Keep in mind it's not just about lstat() calls. Processing ignore patterns also takes a big chunk of CPU, especially when you have more than a couple ignore rules. I'm not convinced that writing index files is that expensive for small files. I don't know about Windows, with Linux it seems fast to me. Actually, for small scale repos, performance probably does not differ much either. > That means we could potentially lstat hundreds or thousands of files and > still come out ahead. Given the untracked cache works at the directory > level, the lstat cost actually scales with the number of directories that > have had changes (ie changing a 2nd file in the same directory doesn't add > any additional cost). In "typical" workflows, developers don't often change > hundreds of files across different directories so we'd "typically" still > come out ahead. I agree. But we must deal with the bad case when someone "accidentally" does that. We should not wait until them yell up "it's too slow now" and tell them to disable/enable untracked cache again. Another case that could touches a lot of directories over time is switch trees (e.g. "git checkout master" then "checkout next" or worse "checkout v1.0"). > We have internal performance tests based on common developer sequences of > commands (status, edit a file, status, add, status, commit, log, push, etc) > that show that having the untracked cache turned on actually makes these > sequences slower. At the time, we didn't have time to investigate why so we > just turned off the untracked cache. > > When writing the fsmonitor patch series which can utilize the untracked > cache, I did some investigation into why the untracked cache was slowing > things down in these tests and discovered the cause was the additional index > writes. That is what led to this patch. > > I've been sitting on it for months now because I didn't have the time to > write some performance tests for the git perf framework to demonstrate the > problem and how this helps solve it. With the discussion about the > fsmonitor extension, I thought this might be a good time to send it out > there. Hopefully you have time to get some of those numbers :) A patch is more convincing when it's backed by numbers. And I'm still not convinced that never ever letting untracked cache be written to the index again is a right thing to do [1]. > If you have the cycles, time a typical series of commands with and without > the untracked cache (ie don't just call status over and over in a loop) and > you should see the same results. Given my limited time right now, I'm OK > putting this on the back burner again until a git perf test can be written > to ensure it actually speeds things up as advertised. [1] Another case that we must _sometimes_ let untracked cache be written down is to correct its data. My last invalidation bug, for example, could leave the untracked cache in a bad state, when you run with new git then it should be able to correct the data and write down. But if you don't allow writing down, the new 'git' will keep seeing the old errors and keep complaining. -- Duy
Re: totally confused as to what "git bisect skip" is supposed to do
On Fri, 9 Feb 2018, Junio C Hamano wrote: > "Robert P. J. Day" writes: > > > i'm confused ... why, after skipping a good chunk in the interval > > [v4.13,v4.14], do i still have exactly 7300 revisions to bisect? what > > am i so hopelessly misunderstanding here? > > Are you really "skipping" a chunk in the interval? > > I thought that "git bisect skip" is a way for you to respond, when > "git bisect" gave you a commit to test, saying "sorry, I cannot test > that exact version, please offer me something else to test". And > each time you say that, you are not narrowing the search space in > any way, so it is understandable that the numver of candidate bad > commits will not decrease. this might be an issue of terminology, then, as "man git-bisect" clearly suggests you can skip a range: You can also skip a range of commits, instead of just one commit, using range notation. For example: $ git bisect skip v2.5..v2.6 This tells the bisect process that no commit after v2.5, up to and including v2.6, should be tested. my issue (if this is indeed an issue) is that if i select to skip a sizable range of commits to test, should that not result in git bisect telling me it now has far fewer revisions to test? if i, in fact, manage to "disqualify" a number of commits from testing, is there no visual confirmation that i now have fewer commits to test? rday
Re: "git bisect run make" adequate to locate first unbuildable commit?
> > 1. there may be feature branches that bypass the known good starting > >commit, which can cause understanding issues as those side > >branches that predate the start point are also considered > >potential bu commits. > > ok, but let's make sure i understand what defines a possible commit > that "introduces" the bug. if i define two bisection commits > and , then i've always assumed that what i'm after is a commit > for which: > > 1) is reachable from > 2) is reachable from > > this seems fairly obvious. Well, maybe _you_ are after such a commit, but bisect is after a commit for which 1) is reachable from (i.e. the same as your first point) 2) is not reachable from (which is not the same as your second point, notably when it comes to commits on side branches that branched off before and got merged later). > now, as you suggest, it's possible that the > "bug" was introduced on a feature branch that bypasses my choice of > but, at *some* point, that feature branch would have to be > merged to the point where it was now reachable from and, in the > context of bisection, *that* merge commit would represent where the > bug was introduced, no? No. Consider this piece of history: v v --a---b---C---d---e---M---k---L-- \ / f---g---H---i---j ^ first bad Then the command 'git bisect start L C' will first consider the following as "possible commit that introduces the bug": d e f g H i j M k L (IOW all commits listed by 'git log ^C L') and will then systematically narrow down until it will find commit H as the transition from good to bad.
Hi Dear.
Hi Dear, How are you today I hope that everything is OK with you as it is my great pleasure to contact you in having communication with you starting from today, I was just going through the Internet search when I found your email address, I want to make a very new and special friend, so I decided to contact you to see how we can make it work if we can. Please I wish you will have the desire with me so that we can get to know each other better and see what happens in future. My name is Jennifer Williams, I am an American presently I live in the UK, I will be very happy if you can write me through my private email address() for easy communication so that we can know each other, I will give you my pictures and details about me. Bye Jennifer.
*Beachtung*
Herzlichen Glückwunsch, Sie haben am 31. Januar 2018 in den monatlichen Gewinnspielen von Euro Millions/Google Promo 650.000 gewonnen. Kontaktieren Sie unseren Schadenregulierungsbeauftragten mit den folgenden Informationen 1. Vollst?ndiger Name 2. Adresse 3. Geschlecht 4. Alter 5. Beruf 6. Telefon Pianese Germano Online-Koordinator
Re: [PATCH v3 00/35] protocol version 2
On 2/6/2018 8:12 PM, Brandon Williams wrote: Changes in v3: * There were some comments about how the protocol should be designed stateless first. I've made this change and instead of having to supply the `stateless-rpc=true` capability to force stateless behavior, the protocol just requires all commands to be stateless. * Added some patches towards the end of the series to force the client to not request to use protocol v2 when pushing (even if configured to use v2). This is to ease the roll-out process of a push command in protocol v2. This way when servers gain the ability to accept pushing in v2 (and they start responding using v2 when requests are sent to the git-receive-pack endpoint) that clients who still don't understand how to push using v2 won't request to use v2 and then die when they recognize that the server does indeed know how to accept a push under v2. * I implemented the `shallow` feature for fetch. This feature encapsulates the existing functionality of all the shallow/deepen capabilities in v0. So now a server can process shallow requests. * Various other small tweaks that I can't remember :) After all of that I think the series is in a pretty good state, baring any more critical reviewing feedback. Thanks! Brandon Williams (35): pkt-line: introduce packet_read_with_status pkt-line: introduce struct packet_reader pkt-line: add delim packet support upload-pack: convert to a builtin upload-pack: factor out processing lines transport: use get_refs_via_connect to get refs connect: convert get_remote_heads to use struct packet_reader connect: discover protocol version outside of get_remote_heads transport: store protocol version protocol: introduce enum protocol_version value protocol_v2 test-pkt-line: introduce a packet-line test helper serve: introduce git-serve ls-refs: introduce ls-refs server command connect: request remote refs using v2 transport: convert get_refs_list to take a list of ref patterns transport: convert transport_get_remote_refs to take a list of ref patterns ls-remote: pass ref patterns when requesting a remote's refs fetch: pass ref patterns when fetching push: pass ref patterns when pushing upload-pack: introduce fetch server command fetch-pack: perform a fetch using v2 upload-pack: support shallow requests fetch-pack: support shallow requests connect: refactor git_connect to only get the protocol version once connect: don't request v2 when pushing transport-helper: remove name parameter transport-helper: refactor process_connect_service transport-helper: introduce stateless-connect pkt-line: add packet_buf_write_len function remote-curl: create copy of the service name remote-curl: store the protocol version the server responded with http: allow providing extra headers for http requests http: don't always add Git-Protocol header remote-curl: implement stateless-connect command remote-curl: don't request v2 when pushing .gitignore | 1 + Documentation/technical/protocol-v2.txt | 338 + Makefile| 7 +- builtin.h | 2 + builtin/clone.c | 2 +- builtin/fetch-pack.c| 21 +- builtin/fetch.c | 14 +- builtin/ls-remote.c | 7 +- builtin/receive-pack.c | 6 + builtin/remote.c| 2 +- builtin/send-pack.c | 20 +- builtin/serve.c | 30 ++ builtin/upload-pack.c | 74 connect.c | 352 +- connect.h | 7 + fetch-pack.c| 319 +++- fetch-pack.h| 4 +- git.c | 2 + http.c | 25 +- http.h | 2 + ls-refs.c | 96 + ls-refs.h | 9 + pkt-line.c | 149 +++- pkt-line.h | 77 protocol.c | 2 + protocol.h | 1 + remote-curl.c | 257 - remote.h| 9 +- serve.c | 260 + serve.h | 15 + t/helper/test-pkt-line.c| 64 t/t5701-git-serve.sh| 176 + t/t5702-protocol-v2.sh | 239 transport-helper.c | 84 +++-- transport-internal.h| 4 +- transport.c
Re: [PATCH 1/1] Mark messages for translations
Let me repeat what you said so I know how to improve the patch: @Junio: > Perhaps end each sentence with a full-stop? I should end each sentence in the *log* message with "." (rather than the translatable strings in the patch) > Shouldn't this rather be like so instead? > if test_i18ngrep ! "invalid gitfile format" .err ... > These two ones want to be written The standard negation form is: test_i18ngrep ! but I should leave the `!` in front of `test_i18ngrep` in this particular case @Jeff: > we may want to avoid this anti-pattern Current state of these tests is wrong and I should rework them. Here is what I intend to do: 1. Fix the commit message 2. Check whether I can get the tests in t0002-gitfile.sh to the standard `test_i18ngrep !` negative (i.e. without using `if`) 3. Post and ask for feedback again Kind regards: al_shopov
please change stash
Dear great team, Normal git tooling creates different files file.ORIG file.LOCAL file.REMOTE in case of conflicts. However `git stash pop` manipulates your files directly resulting in lines like: <<< Updated upstream >>> Stashed changes This can seriously corrupt files and workflows. If it is «the user's fault» or negligence then at least we're not the only one: https://github.com/search?q=Stashed+changes&type=Code 30 'idiots' might hint at a UX problem. (factor 10 in darknet) -- Kind regards, Karsten Flügge, CEO GmbH Hagenkampsweg 10 25474 Hasloh Germany Mobile +49-176-64638989 Support +1-855-447-2666 E-Mail i...@pannous.com Homepage pannous.com Handelsregister: Amtsgericht Pinneberg HRB 7795 PI Sitz der Gesellschaft: Hasloh Steuernummer: 18/291/16961 USt-Id Nr: DE264064657 CEO: Karsten Flügge -- Kind regards, Karsten Flügge, CEO GmbH Hagenkampsweg 10 25474 Hasloh Germany Mobile +49-176-64638989 Support +1-855-447-2666 E-Mail i...@pannous.com Homepage pannous.com Handelsregister: Amtsgericht Pinneberg HRB 7795 PI Sitz der Gesellschaft: Hasloh Steuernummer: 18/291/16961 USt-Id Nr: DE264064657 CEO: Karsten Flügge
Re: [PATCH 1/1] Mark messages for translations
On Mon, Feb 12, 2018 at 04:03:49PM +0100, Alexander Shopov wrote: > @Jeff: > > we may want to avoid this anti-pattern > Current state of these tests is wrong and I should rework them. > > Here is what I intend to do: > 1. Fix the commit message > 2. Check whether I can get the tests in t0002-gitfile.sh to the > standard `test_i18ngrep !` negative (i.e. without using `if`) > 3. Post and ask for feedback again See the patch I posted earlier. For (2), "test_i18ngrep !" would be the wrong thing. I think you should either: - keep your patch as-is, and let Junio resolve the conflict when the two are merged - rebase your patch on top of mine. That's slightly less work for Junio, but it means that your topic cannot graduate until mine does (though hopefully mine is pretty non-controversial). I'd probably just do the first in your place, since the conflict is easy to resolve. -Peff
Re: totally confused as to what "git bisect skip" is supposed to do
On Mon, Feb 12, 2018 at 11:44 AM, Robert P. J. Day wrote: > On Fri, 9 Feb 2018, Junio C Hamano wrote: > >> "Robert P. J. Day" writes: >> >> > i'm confused ... why, after skipping a good chunk in the interval >> > [v4.13,v4.14], do i still have exactly 7300 revisions to bisect? what >> > am i so hopelessly misunderstanding here? >> >> Are you really "skipping" a chunk in the interval? >> >> I thought that "git bisect skip" is a way for you to respond, when >> "git bisect" gave you a commit to test, saying "sorry, I cannot test >> that exact version, please offer me something else to test". And >> each time you say that, you are not narrowing the search space in >> any way, so it is understandable that the numver of candidate bad >> commits will not decrease. > > this might be an issue of terminology, then, as "man git-bisect" > clearly suggests you can skip a range: > > You can also skip a range of commits, instead of just one > commit, using range notation. For example: > >$ git bisect skip v2.5..v2.6 > > This tells the bisect process that no commit after v2.5, up to > and including v2.6, should be tested. Yeah, I think this is kind of a terminology related. First when git bisect says "Bisecting: XXX revisions left to test after this" it doesn't mean that all those revisions left will actually be tested, as git bisect's purpose is to avoid testing as many revisions as possible. So the XXX revisions are actually the revisions that possibly contain the first bad commit. And, as Junio wrote, when you tell git bisect that you cannot test some revisions, it doesn't mean that those revisions cannot contain the first bad commit. > my issue (if this is indeed an issue) is that if i select to skip a > sizable range of commits to test, should that not result in git bisect > telling me it now has far fewer revisions to test? if i, in fact, > manage to "disqualify" a number of commits from testing, is there no > visual confirmation that i now have fewer commits to test? I hope that the above clarification I gave is enough, but maybe the following will help you. If you cannot test let's say 20 commits because there is build problem in those commits, and in the end Git tells you that the first bad commit could be any of 3 commits, 2 of them that were previously marked with skip, then you could still, if you wanted, fix those commits, so that they can be built and test them. So yeah if we only talk about the current bisection, the skipped commits will not be tested, but if we talk about completely finishing the bisection and finding the first bad commit, then those commits could still be tested.
partial fetch
Hi. In 2017 a set of patches titled "add object filtering for partial fetch" was accepted. Is it what I think it is? Will we be able to download only a subdirectory from a large project?
[PATCH] describe: confirm that blobs actually exist
Prior to 644eb60bd0 (builtin/describe.c: describe a blob,
2017-11-15), we noticed and complained about missing
objects, since they were not valid commits:
$ git describe
fatal: is not a valid 'commit' object
After that commit, we feed any non-commit to lookup_blob(),
and complain only if it returns NULL. But the lookup_*
functions do not actually look at the on-disk object
database at all. They return an entry from the in-memory
object hash if present (and if it matches the requested
type), and otherwise auto-create a "struct object" of the
requested type.
A missing object would hit that latter case: we create a
bogus blob struct, walk all of history looking for it, and
then exit successfully having produced no output.
One reason nobody may have noticed this is that some related
cases do still work OK:
1. If we ask for a tree by sha1, then the call to
lookup_commit_referecne_gently() would have parsed it,
and we would have its true type in the in-memory object
hash.
2. If we ask for a name that doesn't exist but isn't a
40-hex sha1, then get_oid() would complain before we
even look at the objects at all.
We can fix this by replacing the lookup_blob() call with a
check of the true type via sha1_object_info(). This is not
quite as efficient as we could possibly make this check. We
know in most cases that the object was already parsed in the
earlier commit lookup, so we could call lookup_object(),
which does auto-create, and check the resulting struct's
type (or NULL). However it's not worth the fragility nor
code complexity to save a single object lookup.
The new tests cover this case, as well as that of a
tree-by-sha1 (which does work as described above, but was
not explicitly tested).
Noticed-by: Michael Haggerty
Signed-off-by: Jeff King
---
builtin/describe.c | 2 +-
t/t6120-describe.sh | 8
2 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/builtin/describe.c b/builtin/describe.c
index 6fe1c51281..18c68ec7a4 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -502,7 +502,7 @@ static void describe(const char *arg, int last_one)
if (cmit)
describe_commit(&oid, &sb);
- else if (lookup_blob(&oid))
+ else if (sha1_object_info(oid.hash, NULL) == OBJ_BLOB)
describe_blob(oid, &sb);
else
die(_("%s is neither a commit nor blob"), arg);
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index a5d9015024..bae78c4e89 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -378,4 +378,12 @@ check_describe tags/A --all A
check_describe tags/c --all c
check_describe heads/branch_A --all --match='branch_*' branch_A
+test_expect_success 'describe complains about tree object' '
+ test_must_fail git describe HEAD^{tree}
+'
+
+test_expect_success 'describe complains about missing object' '
+ test_must_fail git describe $_z40
+'
+
test_done
--
2.16.1.464.gc4bae515b7
Re: [PATCH] describe: confirm that blobs actually exist
On Mon, Feb 12, 2018 at 12:23:06PM -0500, Jeff King wrote:
> We can fix this by replacing the lookup_blob() call with a
> check of the true type via sha1_object_info(). This is not
> quite as efficient as we could possibly make this check. We
> know in most cases that the object was already parsed in the
> earlier commit lookup, so we could call lookup_object(),
> which does auto-create, and check the resulting struct's
> type (or NULL). However it's not worth the fragility nor
> code complexity to save a single object lookup.
By the way, I did notice one other inefficiency here: we always call
lookup_commit_reference_gently() first, which will call parse_object().
So if you were to "git describe" an enormous blob, we'd load the whole
thing into memory for no purpose. We could structure this as:
type = sha1_object_info(oid.hash, NULL);
if (type == OBJ_BLOB)
describe_blob(&oid);
else if (lookup_commit_reference_gently(&oid, 1))
describe_commit(&oid);
else
describe("neither commit nor blob");
That incurs an extra object lookup for the commit case, but potentially
saves reading the blob. We could have our cake and eat it, too, if
sha1_file.c had a function like "parse this object unless it's a blob,
in which case just fill in the type info".
Arguably that should be the default when parse_object() is called on a
blob, but I suspect some older code may rely on parse_object() to check
that the object is present and consistent.
Anyway, I don't know that it's really worth caring about too much, but
just something I noticed.
Maybe a #leftoverbits if somebody cares.
-Peff
Re: [PATCH v1] dir.c: don't flag the index as dirty for changes to the untracked cache
On 2/12/2018 5:20 AM, Duy Nguyen wrote: On Wed, Feb 7, 2018 at 9:13 PM, Ben Peart wrote: On 2/6/2018 7:27 AM, Duy Nguyen wrote: This is another thing that bugs me. I know you're talking about huge index files, but at what size should we start this sort of optimization? Writing down a few MBs on linux is cheap enough that I won't bother optimizing (and I get my UNTR extension repaired all the time, so reduced lstat calls and stuff). This "typically" only comes at certain size, what size? It's important to identify what we're trading off here. With the proposed optimization, we'll end up doing less writes of the index but potentially more lstat calls. Even with a small index, writing the index is much more expensive than calling lstat on a file. Exactly how much more expensive depends on a lot of variables but even with a SSD its still orders of magnitude difference. Keep in mind it's not just about lstat() calls. Processing ignore patterns also takes a big chunk of CPU, especially when you have more than a couple ignore rules. Yes, I'm very familiar with the cost of the exclude list pattern matching code. I've rewritten it to use a hashmap (for those patterns where it is possible) that dramatically speeds that aspect up as we tend to abuse it pretty heavily (~60K entries on average). I'm not convinced that writing index files is that expensive for small files. I don't know about Windows, with Linux it seems fast to me. Actually, for small scale repos, performance probably does not differ much either. I agree. For small scale repos, none of this is significant enough to matter. Untracked caching should help most as your working directory starts to get large. That means we could potentially lstat hundreds or thousands of files and still come out ahead. Given the untracked cache works at the directory level, the lstat cost actually scales with the number of directories that have had changes (ie changing a 2nd file in the same directory doesn't add any additional cost). In "typical" workflows, developers don't often change hundreds of files across different directories so we'd "typically" still come out ahead. I agree. But we must deal with the bad case when someone "accidentally" does that. We should not wait until them yell up "it's too slow now" and tell them to disable/enable untracked cache again. Another case that could touches a lot of directories over time is switch trees (e.g. "git checkout master" then "checkout next" or worse "checkout v1.0"). You're example above makes me wonder if you understand what my patch is doing. If the index is flagged as dirty for _any_ reason, the entire index is written to disk with the latest information - including the updated untracked cache and all other extensions. So in your checkout examples above, the index will still get written to disk with the updated untracked cache extension. There would be zero change in behavior or performance. The _only_ time the index is not written to disk after my patch is if there were no other changes to the index. In my experience, that is only status calls. To suffer any degradation in the untracked cache would be if a user edited a bunch of files across multiple directories and called status repeatedly. As soon as they did add, checkout, rm, rebase, etc (ie most git commands other than status) the index would get flagged as dirty and the latest untracked cache extension would get written to disk. We have internal performance tests based on common developer sequences of commands (status, edit a file, status, add, status, commit, log, push, etc) that show that having the untracked cache turned on actually makes these sequences slower. At the time, we didn't have time to investigate why so we just turned off the untracked cache. When writing the fsmonitor patch series which can utilize the untracked cache, I did some investigation into why the untracked cache was slowing things down in these tests and discovered the cause was the additional index writes. That is what led to this patch. I've been sitting on it for months now because I didn't have the time to write some performance tests for the git perf framework to demonstrate the problem and how this helps solve it. With the discussion about the fsmonitor extension, I thought this might be a good time to send it out there. Hopefully you have time to get some of those numbers :) A patch is more convincing when it's backed by numbers. And I'm still not convinced that never ever letting untracked cache be written to the index again is a right thing to do [1]. I agree that any performance patch should be accompanied by a performance test to demonstrate it actually performs as promised. I looked for but didn't find a performance test for the untracked cache so will have to create one from scratch. In order to make that as realistic as possible, it needs to simulate (as much as possible) typical developer workflo
Re: partial fetch
On 2/12/2018 11:24 AM, Basin Ilya wrote: Hi. In 2017 a set of patches titled "add object filtering for partial fetch" was accepted. Is it what I think it is? Will we be able to download only a subdirectory from a large project? yes, that is the goal. there are several caveats, but yes, that is the goal. Jeff
Re: What's cooking in git.git (Feb 2018, #01; Wed, 7)
* jh/status-no-ahead-behind (2018-01-24) 4 commits - status: support --no-ahead-behind in long format - status: update short status to respect --no-ahead-behind - status: add --[no-]ahead-behind to status and commit for V2 format. - stat_tracking_info: return +1 when branches not equal "git status" can spend a lot of cycles to compute the relation between the current branch and its upstream, which can now be disabled with "--no-ahead-behind" option. At v5; is this ready for 'next'? I believe so. I don't recall any further discussions on it. The only open question was the idea of trying to walk 100 or so commits and see if one was "close" to being an ancestor of the other, but we thought that Stolee's graph cache would be a better solution for that. So, yes, I think it is ready for 'next'. Thanks, Jeff
Re: [PATCH] describe: confirm that blobs actually exist
On Mon, Feb 12, 2018 at 9:23 AM, Jeff King wrote: > Prior to 644eb60bd0 (builtin/describe.c: describe a blob, > 2017-11-15), we noticed and complained about missing > objects, since they were not valid commits: > > $ git describe > fatal: is not a valid 'commit' > object > > After that commit, we feed any non-commit to lookup_blob(), > and complain only if it returns NULL. But the lookup_* > functions do not actually look at the on-disk object > database at all. They return an entry from the in-memory > object hash if present (and if it matches the requested > type), and otherwise auto-create a "struct object" of the > requested type. > > A missing object would hit that latter case: we create a > bogus blob struct, walk all of history looking for it, and > then exit successfully having produced no output. > > One reason nobody may have noticed this is that some related > cases do still work OK: > > 1. If we ask for a tree by sha1, then the call to > lookup_commit_referecne_gently() would have parsed it, lookup_commit_reference_gently > and we would have its true type in the in-memory object > hash. > > 2. If we ask for a name that doesn't exist but isn't a > 40-hex sha1, then get_oid() would complain before we > even look at the objects at all. > > We can fix this by replacing the lookup_blob() call with a > check of the true type via sha1_object_info(). This is not > quite as efficient as we could possibly make this check. We > know in most cases that the object was already parsed in the > earlier commit lookup, so we could call lookup_object(), > which does auto-create, and check the resulting struct's > type (or NULL). However it's not worth the fragility nor > code complexity to save a single object lookup. > > The new tests cover this case, as well as that of a > tree-by-sha1 (which does work as described above, but was > not explicitly tested). > > Noticed-by: Michael Haggerty > Signed-off-by: Jeff King This makes sense. Reviewed-by: Stefan Beller Thanks! Stefan
Re: REQUEST NEW TRANSLATION (INDONESIAN/id_ID)
On Sun, Feb 11, 2018 at 9:53 AM, wrote: > Hello git-l10n Team cc'd Jiang Xi, who coordinates the git-l10n team. > > I want to join to this project as a translator for Indonesian language (ID) > I have read the README file located in the > https://github.com/git-l10n/git-po/blob/master/po/README directory > > I also have a fork repository master (git-l10n) to my repository (anaufalm), > and also I have edited the TEAMS file by adding my name as a translator for > Indonesia (id). And also I created a new file `id.po` which I copy from > file` ca.po` as the source. Because I not find original file as english, > like `en.po`. cool! > > Furthermore, if approved, I will translate the file asap. I don't think you need approval here, just do it. :) Thanks, Stefan > > Thank you. > > --- > > My repository (fork): https://github.com/anaufalm/git-id > PR link request TEAMS: https://github.com/git-l10n/git-po/pull/288 > PR link add id.po file: https://github.com/git-l10n/git-po/pull/289
Re: [PATCH v3 07/14] commit-graph: update graph-head during write
Derrick Stolee writes: > It is possible to have multiple commit graph files in a pack directory, > but only one is important at a time. Use a 'graph_head' file to point > to the important file. Teach git-commit-graph to write 'graph_head' upon > writing a new commit graph file. Why this design, instead of what "repack -a" would do, iow, if there always is a singleton that is the only one that matters, shouldn't the creation of that latest singleton just clear the older ones before it returns control?
Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store
On Fri, Feb 9, 2018 at 2:09 PM, Junio C Hamano wrote: > Stefan Beller writes: > >> Patch generated by >> >> 2. Applying the semantic patch contrib/coccinelle/packed_git.cocci >> to adjust callers. > > About this part... > >> diff --git a/contrib/coccinelle/packed_git.cocci >> b/contrib/coccinelle/packed_git.cocci >> new file mode 100644 >> index 00..da317a51a9 >> --- /dev/null >> +++ b/contrib/coccinelle/packed_git.cocci >> @@ -0,0 +1,7 @@ >> +@@ @@ >> +- packed_git >> ++ the_repository->objects.packed_git >> + >> +@@ @@ >> +- packed_git_mru >> ++ the_repository->objects.packed_git_mru > > The above is correct for one-time transition turning pre-transition > code to post the_repository world, but I am not sure if we want to > have it in contrib/coccinelle/, where "make coccicheck" looks at, as > a way to continuously keep an eye on "style" violations like using > strbuf_addf() for a constant when strbuf_addstr() suffices. > > Wouldn't we need a mechanism to ensure that this file will *not* be > used in "make coccicheck" somehow? > I can omit the cocci files from the patches, if that is better for maintenance. I thought it may be a helpful for merging this series with the rest of the evolved code base which may make use of one of the converted functions. So instead of fixing that new instance manually, cocinelle could do that instead. Stefan
Re: [PATCH 046/194] object-store: move replace_objects back to object-store
On Fri, Feb 9, 2018 at 3:15 PM, Junio C Hamano wrote:
> Stefan Beller writes:
>
>> @@ -32,7 +31,15 @@ struct object_store {
>>* Objects that should be substituted by other objects
>>* (see git-replace(1)).
>>*/
>> - struct replace_objects replacements;
>> + struct replace_objects {
>> + /*
>> + * An array of replacements. The array is kept sorted by the
>> original
>> + * sha1.
>> + */
>> + struct replace_object **items;
>> +
>> + int alloc, nr;
>> + } replacements;
>>
>> /*
>>* A fast, rough count of the number of objects in the repository.
>> @@ -49,7 +56,7 @@ struct object_store {
>> unsigned packed_git_initialized : 1;
>> };
>> #define OBJECT_STORE_INIT \
>> - { NULL, MRU_INIT, ALTERNATES_INIT, REPLACE_OBJECTS_INIT, 0, 0, 0 }
>> + { NULL, MRU_INIT, ALTERNATES_INIT, { NULL, 0, 0 }, 0, 0, 0 }
Maybe we can move the REPLACE_OBJECTS_INIT just before the
definition of OBJECT_STORE_INIT, so we'd not need to touch this line here.
>
> Not the primary thrust of this topic, but we may want to convert
> these to use designated initializers after this series is done.
I agree.
I feel cbc0f81d96 (strbuf: use designated initializers in STRBUF_INIT,
2017-07-10)
may need longer cooking until all the interesting architectures have
tried picking up
the latest release, though.
Stefan
Re: Fetch-hooks
On 02/10, Leo Gaspard wrote:
> On 02/10/2018 01:21 PM, Jeff King wrote:
> > On Sat, Feb 10, 2018 at 01:37:20AM +0100, Leo Gaspard wrote:
> >
> >>> Yeah, tag-following may be a little tricky, because it usually wants to
> >>> write to refs/tags/. One workaround would be to have your config look
> >>> like this:
> >>>
> >>> [remote "origin"]
> >>> fetch = +refs/heads/*:refs/quarantine/origin/heads/*
> >>> fetch = +refs/tags/*:refs/quarantine/origin/tags/*
> >>> tagOpt = --no-tags
> >>>
> >>> That's not exactly the same thing, because it would fetch all tags, not
> >>> just those that point to the history on the branches. But in most
> >>> repositories and workflows the distinction doesn't matter.
> >>
> >> Hmm... apart from the implementation complexity (of which I have no
> >> idea), is there an argument against the idea of adding a
> >> remote..fetchTagsTo refmap similar to remote..fetch but used
> >> every time a tag is fetched? (well, maybe not exactly similar to
> >> remote..fetch because we know the source is going to be
> >> refs/tags/*; so just having the part of .fetch past the ':' would be
> >> more like what's needed I guess)
> >
> > I don't think it would be too hard to implement, and is at least
> > logically consistent with the way we handle tags.
> >
> > If we were designing from scratch, I do think this would all be helped
> > immensely by having more separation of refs fetched from remotes. There
> > was a proposal in the v1.8 era to fetch everything for a remote,
> > including tags, into "refs/remotes/origin/heads/",
> > "refs/remotes/origin/tags/", etc. And then we'd teach the lookup side to
> > look for tags in each of the remote-tag namespaces.
> >
> > I still think that would be a good direction to go, but it would be a
> > big project (which is why the original stalled).
>
> Hmm... would this also drown the remote..fetch map? Also, I think
> this behavior could be emulated with fetch and fetchTagsTo, and it would
> look like:
> [remote "my-remote"]
> fetch = +refs/heads/*:refs/remotes/my-remote/heads/*
> fetchTagsTo = refs/remotes/my-remote/tags/*
> The remaining issue being to teach the lookup side to look for tags in
> all the remote-tag namespaces (and the fact it's a breaking change).
>
> That said, actually I just noticed an issue in the “add a
> remote..fetch option to fetch to refs/quarantine then have the
> post-fetch hook do the work”: it means if I run `git pull`, then:
> 1. The remote references will be pulled to refs/quarantine/...
> 2. The post-fetch hook will copy the accepted ones to refs/remotes/...
> 3. The `git merge FETCH_HEAD` called by pull will merge FETCH_HEAD into
> local branches... and so merge from refs/quarantine.
>
> A solution would be to also update FETCH_HEAD in the post-fetch hook,
> but then we're back to the issue raised by Junio after the “*HOWEVER*”
> of [1]: the hook writer has to maintain consistency between the “copied”
> references and FETCH_HEAD.
>
> So, when thinking about it, I'm back to thinking the proper hook
> interface should be the one of the tweak-fetch hook, but its
> implementation should make it not go crazy on remote servers. And so
> that the implementation should do all this refs/quarantine wizardry
> inside git itself.
>
> So basically what I'm getting at at the moment is that git fetch should:
> 1. fetch all the references to refs/real-remotes//{insert here a
> copy of the refs/ tree of }
> 2. figure out what the “expected” names for these references will by,
> by looking at remote..fetch (and at whether --tags was passed)
> 3. for each “expected” name,
> 1. if a tweak-fetch hook is present, call it with the
> refs/real-remotes/... refname and the “expected end-name” from
> remote..fetch
> 2. based on the hook's result, potentially to move the “expected
> end-name” to another commit than the one referenced by refs/real-remotes/...
> 3. move the “expected” name to the commit referenced in
> refs/real-remotes
>
> Which would make the tweak-fetch hook interface simpler (though more
> restrictive, but I don't have any real use case for the other change
> possibilities) than it is now:
> 1. feed the hook with lines of
> “refs/real-remotes/my-remote/heads/my-branch
> refs/remotes/my-remote/my-branch” (assuming remote.my-remote.fetch is
> “normal”) or “refs/real-remotes/my-remote/tags/my-tag refs/tags/my-tag”
> (if my-tag is being fetched from my-remote)
> 2. read lines of “ refs/remotes/my-remote/my-branch”, that
> will re-point my-branch to instead of
> refs/real-remotes/my-remote/heads/my-branch. In order to avoid any
> issue, the hook is not allowed to pass as second output a reference that
> was not passed as second input.
>
> This way, the behavior of the tweak-fetch hook is reasonably preserved
> (at least for my use case), and there is no additional load on the
> servers thanks to the up-to-date references being stored in
> refs/real-remotes//
>
> Does this reasoning make any
Re: [PATCH v3 04/12] sequencer: introduce new commands to reset the revision
On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin
wrote:
> [...]
> This commit implements the commands to label, and to reset to, given
> revisions. The syntax is:
>
> label
> reset
> [...]
> Signed-off-by: Johannes Schindelin
> ---
> diff --git a/sequencer.c b/sequencer.c
> @@ -1922,6 +1951,151 @@ static int do_exec(const char *command_line)
> +static int safe_append(const char *filename, const char *fmt, ...)
> +{
> + [...]
> + if (write_in_full(fd, buf.buf, buf.len) < 0) {
> + error_errno(_("could not write to '%s'"), filename);
> + rollback_lock_file(&lock);
strbuf_release(&buf);
> + return -1;
> + }
> + if (commit_lock_file(&lock) < 0) {
> + rollback_lock_file(&lock);
strbuf_release(&buf);
> + return error(_("failed to finalize '%s'"), filename);
> + }
> +
strbuf_release(&buf);
> + return 0;
> +}
> +
> +static int do_reset(const char *name, int len, struct replay_opts *opts)
> +{
> + [...]
> + unpack_tree_opts.reset = 1;
> +
> + if (read_cache_unmerged())
rollback_lock_file(&lock);
strbuf_release(&ref_name);
> + return error_resolve_conflict(_(action_name(opts)));
> +
> + if (!fill_tree_descriptor(&desc, &oid)) {
> + error(_("failed to find tree of %s"), oid_to_hex(&oid));
> + rollback_lock_file(&lock);
> + free((void *)desc.buffer);
> + strbuf_release(&ref_name);
> + return -1;
> + }
The Minerals, Metals & Materials Society Annual - TMS
Hi, I understand your company is exhibiting in The Minerals, Metals & Materials Society Annual on MAR/11 - MAR/15/2018. Would you be interested in the complete contact information with email addresses of Materials scientists and Engineers? Available Data Fields: Practice Name, Web Address/URL, Contact name (First name, middle name and last name), Specialty,, Postal address (street address, city, state, zip code, and country), Phone, and Direct email address. Let me know your interest so that I can send you additional Information in my next email. Match Test for Appending : Just send us now 10 to 15 contacts in an excel sheet from your in-house database with missing email address, telephone numbers, fax numbers or mailing addresses, we can append it for you at no cost, this will help you check the quality of our services. Regards, Sheryl Droege Marketing Associate Please "Unsubscribe" if not interested in receiving further emails. <>
Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook
Eric Sunshine writes: > Although "git worktree add" learned to run the 'post-checkout' hook in > ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it > neglects to change to the directory of the newly-created worktree > before running the hook. Instead, the hook is run within the directory > from which the "git worktree add" command itself was invoked, which > effectively neuters the hook since it knows nothing about the new > worktree directory. > > Fix this by changing to the new worktree's directory before running > the hook, and adjust the tests to verify that the hook is indeed run > within the correct directory. I like the approach taken by this replacement better. Just to make sure I understand the basic idea, let me rephrase what these two patches are doing: - "path" that is made absolute in this step is where the new worktree is created, i.e. the top-level of the working tree in the new worktree. We chdir there and then run the hook script. - Even though we often see hooks executed inside .git/ directory, for post-checkout, the top-level of the working tree is the right place, as that is where the hook is run by "git checkout" (which does the "cd to the toplevel" thing upfront and then runs hooks without doing anything special) and "git clone" (which goes to the newly created repository's working tree by calling setup.c::setup_work_tree() before builtin/clone.c::checkout(), which may call post-checkout hook). I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the environment, though. When a user with a funny configuration (where these two environment variables are pointing at unusual places) uses "git worktree add" to create another worktree for the repository, it would not be sufficient to chdir to defeat them that are appropriate for the original, and not for the new, worktree, would it?
[PATCH] send-email: error out when relogin delay is missing
When the batch size is neither configured nor given on the command
line, but the relogin delay is given, then the current code ignores
the relogin delay setting.
This is unsafe as there was some intention when setting the batch size.
One workaround would be to just assume a batch size of 1 as a default.
This however may be bad UX, as then the user may wonder why it is sending
slowly without apparent batching.
Error out for now instead of potentially confusing the user.
As 5453b83bdf (send-email: --batch-size to work around some SMTP
server limit, 2017-05-21) lays out, we rather want to not have this
interface anyway and would rather want to react on the server throttling
dynamically.
Helped-by: Eric Sunshine
Signed-off-by: Stefan Beller
---
git-send-email.perl | 4
1 file changed, 4 insertions(+)
diff --git a/git-send-email.perl b/git-send-email.perl
index 340b5c8482..f7913f7c2c 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -379,6 +379,10 @@ unless ($rc) {
die __("Cannot run git format-patch from outside a repository\n")
if $format_patch and not $repo;
+die __("`batch-size` and `relogin` must be specified together " .
+ "(via command-line or configuration option)\n")
+ if defined $relogin_delay and not defined $batch_size;
+
# Now, let's fill any that aren't set in with defaults:
sub read_config {
--
2.15.1.433.g936d1b9894.dirty
Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook
> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:
>
> Although "git worktree add" learned to run the 'post-checkout' hook in
> ade546be47 (worktree: invoke post-checkout hook, 2017-12-07), it
> neglects to change to the directory of the newly-created worktree
> before running the hook. Instead, the hook is run within the directory
> from which the "git worktree add" command itself was invoked, which
> effectively neuters the hook since it knows nothing about the new
> worktree directory.
>
> Fix this by changing to the new worktree's directory before running
> the hook, and adjust the tests to verify that the hook is indeed run
> within the correct directory.
>
> While at it, also add a test to verify that the hook is run within the
> correct directory even when the new worktree is created from a sibling
> worktree (as opposed to the main worktree).
>
> Reported-by: Lars Schneider
> Signed-off-by: Eric Sunshine
> ---
> builtin/worktree.c | 11 ---
> t/t2025-worktree-add.sh | 25 ++---
> 2 files changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index 7cef5b120b..b55c55a26c 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -345,9 +345,14 @@ static int add_worktree(const char *path, const char
> *refname,
>* Hook failure does not warrant worktree deletion, so run hook after
>* is_junk is cleared, but do return appropriate code when hook fails.
>*/
> - if (!ret && opts->checkout)
> - ret = run_hook_le(NULL, "post-checkout", oid_to_hex(&null_oid),
> - oid_to_hex(&commit->object.oid), "1", NULL);
> + if (!ret && opts->checkout) {
> + char *p = absolute_pathdup(path);
> + ret = run_hook_cd_le(p, NULL, "post-checkout",
> + oid_to_hex(&null_oid),
> + oid_to_hex(&commit->object.oid),
> + "1", NULL);
> + free(p);
> + }
>
> argv_array_clear(&child_env);
> strbuf_release(&sb);
> diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
> index 2b95944973..cf0aaeaf88 100755
> --- a/t/t2025-worktree-add.sh
> +++ b/t/t2025-worktree-add.sh
> @@ -454,20 +454,29 @@ post_checkout_hook () {
> test_when_finished "rm -f .git/hooks/post-checkout" &&
> mkdir -p .git/hooks &&
> write_script .git/hooks/post-checkout <<-\EOF
> - echo $* >hook.actual
> + {
> + echo $*
> + git rev-parse --show-toplevel
> + } >../hook.actual
> EOF
> }
>
> test_expect_success '"add" invokes post-checkout hook (branch)' '
> post_checkout_hook &&
> - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/gumby
> + } >hook.expect &&
> git worktree add gumby &&
> test_cmp hook.expect hook.actual
> '
>
> test_expect_success '"add" invokes post-checkout hook (detached)' '
> post_checkout_hook &&
> - printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/grumpy
> + } >hook.expect &&
> git worktree add --detach grumpy &&
> test_cmp hook.expect hook.actual
> '
> @@ -479,4 +488,14 @@ test_expect_success '"add --no-checkout" suppresses
> post-checkout hook' '
> test_path_is_missing hook.actual
> '
>
> +test_expect_success '"add" within worktree invokes post-checkout hook' '
> + post_checkout_hook &&
> + {
> + echo $_z40 $(git rev-parse HEAD) 1 &&
> + echo $(pwd)/guppy
> + } >hook.expect &&
> + git -C gloopy worktree add --detach ../guppy &&
> + test_cmp hook.expect hook.actual
> +'
> +
> test_done
> --
> 2.16.1.291.g4437f3f132
>
Looks good but I think we are not quite there yet. It does not work
for bare repos. You can test this if you apply the following patch on
top of your changes. Or get the patch from here:
https://github.com/larsxschneider/git/commit/253130e65b37a2ef250e9c6369d292ec725e62e7.patch
Please note that also '"add" within worktree invokes post-checkout hook'
seems to fail with my extended test case.
Thanks,
Lars
diff --git a/t/t2025-worktree-add.sh b/t/t2025-worktree-add.sh
index cf0aaeaf88..0580b12d50 100755
--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -451,20 +451,41 @@ test_expect_success 'git worktree --no-guess-remote
option overrides config' '
'
post_checkout_hook () {
- test_when_finished "rm -f .git/hooks/post-checkout" &&
- mkdir -p .git/hooks &&
- write_script .git/hooks/post-checkout <<-\EOF
+ test_when_finished "rm -f $1/hooks/post-checkout" &&
+ mkdir -p $1/hooks &&
+ write_script $1/hooks/post-checkout <<-\EOF
{
echo $*
- git rev-parse --show-
[no subject]
Belove My name is Elizabeth M. Philips, i am going on a radical hysterectomy cervical cancer surgery today. I have WILLED £12,379,000.00 British pounds to you for the work of the lord. Contact my attorney with my reference number (NW/XXR/017/053K/PDQ/613X1/UK) for further info. Barr.Luis Jason. email:luis347ja...@gmail.com Sincerely Yours, Mrs.Elizabeth M. Philips.
Re: [PATCH v3 05/12] sequencer: introduce the `merge` command
Hi Eric, On Mon, 12 Feb 2018, Eric Sunshine wrote: > On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin > wrote: > > This patch is part of the effort to reimplement `--preserve-merges` with > > a substantially improved design, a design that has been developed in the > > Git for Windows project to maintain the dozens of Windows-specific patch > > series on top of upstream Git. > > > > The previous patch implemented the `label` and `reset` commands to label > > commits and to reset to a labeled commits. This patch adds the `merge` > > s/to a/to/ Fixed locally. Will be part of the next iteration, if one is necessary. Otherwise I will first ask Junio whether he can touch up the commit message before applying. Ciao, Dscho
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Ramsay Jones writes:
> Attempting to grep the output of test_i18ngrep will not work under a
> poison build, since the output is (almost) guaranteed not to have the
> string you are looking for. In this case, the output of test_i18ngrep
> is further filtered by a simple piplined grep to exclude an '... remote
> end hung up unexpectedly' warning message. Use a regular 'grep -E' to
> replace the call to test_i18ngrep in the filter pipeline.
> Also, remove a useless invocation of 'sort' as the final element of the
> pipeline.
>
> Signed-off-by: Ramsay Jones
> ---
> t/t5536-fetch-conflicts.sh | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/t/t5536-fetch-conflicts.sh b/t/t5536-fetch-conflicts.sh
> index 2e42cf331..38381df5e 100755
> --- a/t/t5536-fetch-conflicts.sh
> +++ b/t/t5536-fetch-conflicts.sh
> @@ -22,7 +22,7 @@ verify_stderr () {
> cat >expected &&
> # We're not interested in the error
> # "fatal: The remote end hung up unexpectedly":
> - test_i18ngrep -E '^(fatal|warning):' actual
> | sort &&
> + grep -E '^(fatal|warning):' actual &&
> test_i18ncmp expected actual
OK, but not quite OK.
Two grep invocations will not leave anything useful in 'actual'
under poison build, and is almost guaranteed that 'expected' would
not match, but that is perfectly OK because the final comparison is
done.
However, it is brittle to rely on the latter "grep -v" to exit with
status 0 for the &&-chain to work. The original was most likely
masked by the "| sort" exiting with 0 status all the time ;-)
There needs an explanation why this commit thinks the invocation of
"sort" useless. I do not particularly think "suppressing a
not-found non-zero exit from 'grep'" is a useful thing, but are we
committed to show the two warnings seen in the last test in this
file to always in the shown order (i.e. the same order as the
refspecs are given to us)? If not, then "sort" does serve a
purpose. Note that I do not think we want to be able to reorder the
warning messages in future versions of Git for that last case, so
making that explicit may be a good justification.
The "sort" as the last step in the pipeline makes sure that we
do not have to guarantee the order in which we give the two
warning messages from the last test in this script, but
processing the refspecs in the same order as they are given on
the command line and warning against them as we discover
problems is a property we would rather want to keep, so drop it
to make sure that we would catch regression when we accidentally
change the order in which these messages are given.
or something like that, perhaps.
[PATCH] color.h: document and modernize header
Add documentation explaining the functions in color.h.
While at it, mark them extern and migrate the function `color_set`
into grep.c, where the only callers are.
Signed-off-by: Stefan Beller
---
* removed the extern keyword
* reworded the docs for want_color once again.
color.c | 7 ---
color.h | 35 ++-
grep.c | 5 +
3 files changed, 35 insertions(+), 12 deletions(-)
diff --git a/color.c b/color.c
index d48dd947c9..f277e72e4c 100644
--- a/color.c
+++ b/color.c
@@ -161,11 +161,6 @@ int color_parse(const char *value, char *dst)
return color_parse_mem(value, strlen(value), dst);
}
-void color_set(char *dst, const char *color_bytes)
-{
- xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
-}
-
/*
* Write the ANSI color codes for "c" to "out"; the string should
* already have the ANSI escape code in it. "out" should have enough
@@ -399,8 +394,6 @@ static int color_vfprintf(FILE *fp, const char *color,
const char *fmt,
return r;
}
-
-
int color_fprintf(FILE *fp, const char *color, const char *fmt, ...)
{
va_list args;
diff --git a/color.h b/color.h
index fd2b688dfb..40a40f31a4 100644
--- a/color.h
+++ b/color.h
@@ -76,22 +76,47 @@ int git_color_config(const char *var, const char *value,
void *cb);
int git_color_default_config(const char *var, const char *value, void *cb);
/*
- * Set the color buffer (which must be COLOR_MAXLEN bytes)
- * to the raw color bytes; this is useful for initializing
- * default color variables.
+ * Parse a config option, which can be a boolean or one of
+ * "never", "auto", "always". Return a constant of
+ * GIT_COLOR_NEVER for "never" or negative boolean,
+ * GIT_COLOR_ALWAYS for "always" or a positive boolean,
+ * and GIT_COLOR_AUTO for "auto".
*/
-void color_set(char *dst, const char *color_bytes);
-
int git_config_colorbool(const char *var, const char *value);
+
+/*
+ * Return a boolean whether to use color,
+ * resolving ambigious settings such as GIT_COLOR_AUTO, which is returned
+ * by git_config_colorbool().
+ */
int want_color(int var);
+
+/*
+ * Translate a Git color from 'value' into a string that the terminal can
+ * interpret and store it into 'dst'. The Git color values are of the form
+ * "foreground [background] [attr]" where fore- and background can be a color
+ * name ("red"), a RGB code (#0xFF) or a 256-color-mode from the terminal.
+ */
int color_parse(const char *value, char *dst);
int color_parse_mem(const char *value, int len, char *dst);
+
+/*
+ * Output the formatted string in the specified color (and then reset to normal
+ * color so subsequent output is uncolored). Omits the color encapsulation if
+ * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting
+ * the color. The `color_print_strbuf` prints the given pre-formatted strbuf
+ * instead, up to its first NUL character.
+ */
__attribute__((format (printf, 3, 4)))
int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
__attribute__((format (printf, 3, 4)))
int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
+/*
+ * Check if the given color is GIT_COLOR_NIL that means "no color selected".
+ * The caller needs to replace the color with the actual desired color.
+ */
int color_is_nil(const char *color);
#endif /* COLOR_H */
diff --git a/grep.c b/grep.c
index 3d7cd0e96f..834b8eb439 100644
--- a/grep.c
+++ b/grep.c
@@ -18,6 +18,11 @@ static void std_output(struct grep_opt *opt, const void
*buf, size_t size)
fwrite(buf, size, 1, stdout);
}
+static void color_set(char *dst, const char *color_bytes)
+{
+ xsnprintf(dst, COLOR_MAXLEN, "%s", color_bytes);
+}
+
/*
* Initialize the grep_defaults template with hardcoded defaults.
* We could let the compiler do this, but without C99 initializers
--
2.16.1.73.ga2c3e9663f.dirty
Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
Hi Sergey, On Mon, 12 Feb 2018, Sergey Organov wrote: > Thanks for explanations, and could you please answer this one: > > [...] > > >> I also have trouble making sense of "Recreate merge commits instead of > >> flattening the history by replaying merges." Is it " >> commits by replaying merges> instead of " or is it > >> rather " instead of >> replaying merges>? I thought I had answered that one. Flattening the history is what happens in regular rebase (i.e. without --recreate-merges and without --preserve-merges). The idea to recreate merges is of course to *not* flatten the history. Maybe there should have been a comma after "history" to clarify what the sentence means. The wording is poor either way, but you are also not a native speaker so we have to rely on, say, Eric to help us out here. Ciao, Johannes
Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook
On Mon, Feb 12, 2018 at 2:37 PM, Junio C Hamano wrote: > Eric Sunshine writes: >> Fix this by changing to the new worktree's directory before running >> the hook, and adjust the tests to verify that the hook is indeed run >> within the correct directory. > > I like the approach taken by this replacement better. Just to make > sure I understand the basic idea, let me rephrase what these two > patches are doing: > > - "path" that is made absolute in this step is where the new >worktree is created, i.e. the top-level of the working tree in >the new worktree. We chdir there and then run the hook script. Sorry for misleading. The "absolute path" stuff in this patch is unnecessary; it's probably just left-over from Lars's proposal which did need to make it absolute when setting GIT_WORK_TREE, and I likely didn't think hard enough to realize that it doesn't need to be absolute just for chdir(). I'll drop the unnecessary absolute_pathdup() in the re-roll. (The hook path in patch 1/2, on the other hand, does need to be made absolute since find_hook() returns a relative path before we've chdir()'d into the new worktree.) > - Even though we often see hooks executed inside .git/ directory, >for post-checkout, the top-level of the working tree is the right >place, as that is where the hook is run by "git checkout" [...] Patch 1/2's commit message is a bit sloppy in its description of this. I'll tighten it up in the re-roll. I'm also not fully convinced that these new overloads of run_hook_*() are warranted since it's hard to imagine any other case when they would be useful. It may make sense just to have builtin/worktree.c run the hook itself for this one-off case. > I wonder if we need to clear existing GIT_DIR/GIT_WORK_TREE from the > environment, though. When a user with a funny configuration (where > these two environment variables are pointing at unusual places) uses > "git worktree add" to create another worktree for the repository, it > would not be sufficient to chdir to defeat them that are appropriate > for the original, and not for the new, worktree, would it? Good point. I'll look into sanitizing the environment.
Re: [PATCH v3 07/14] commit-graph: update graph-head during write
Junio C Hamano writes: > Derrick Stolee writes: > >> It is possible to have multiple commit graph files in a pack directory, >> but only one is important at a time. Use a 'graph_head' file to point >> to the important file. Teach git-commit-graph to write 'graph_head' upon >> writing a new commit graph file. > > Why this design, instead of what "repack -a" would do, iow, if there > always is a singleton that is the only one that matters, shouldn't > the creation of that latest singleton just clear the older ones > before it returns control? Note that I am not complaining---I am just curious why we want to expose this "there is one relevant one but we keep irrelevant ones we usually do not look at and need to be garbage collected" to end users, and also expect readers of the series, resulting code and docs would have the same puzzled feeling.
Re: [PATCH 5/8] rebase: introduce the --recreate-merges option
Hi Sergey, On Mon, 12 Feb 2018, Sergey Organov wrote: > Johannes Schindelin writes: > > > > On Fri, 9 Feb 2018, Sergey Organov wrote: > > > >> Johannes Schindelin writes: > >> > >> [...] > >> > >> > With this patch, the goodness of the Git garden shears comes to `git > >> > rebase -i` itself. Passing the `--recreate-merges` option will generate > >> > a todo list that can be understood readily, and where it is obvious > >> > how to reorder commits. New branches can be introduced by inserting > >> > `label` commands and calling `merge - `. And once this > >> > mode has become stable and universally accepted, we can deprecate the > >> > design mistake that was `--preserve-merges`. > >> > >> This doesn't explain why you introduced this new --recreate-merges. Why > >> didn't you rather fix --preserve-merges to generate and use new todo > >> list format? > > > > Because that would of course break existing users of > > --preserve-merges. > > How exactly? Power users of interactive rebase use scripting to augment Git's functionality. One particularly powerful trick is to override GIT_SEQUENCER_EDITOR with an invocation of such a script, to perform automated edits. Such a script breaks when we change the format of the content to edit. If we change the format of the todo list generated in --preserve-merges mode, that is exactly what happens. We break existing users. BTW it seems that you did not really read my previous reply carefully because I referenced such a use case: the Git garden shears. They do override the sequencer editor, and while they do not exactly edit the todo list (they simply through the generated one away), they generate a new todo list and would break if that format changes. Of course, the shears do not use the --preserve-merges mode, but from just reading about the way how the Git garden shears work, it is quite obvious how similar users of --preserve-merges are likely to exist? > Doesn't "--recreate-merges" produce the same result as > "--preserve-merges" if run non-interactively? The final result of a rebase where you do not edit the todo list? Should be identical, indeed. But that is the most boring, most uninteresting, and least important use case. So we might just as well forget about it when we focus on keeping Git's usage stable. > > So why not --preserve-merges=v2? Because that would force me to > > maintain --preserve-merges forever. And I don't want to. > > > >> It doesn't seem likely that todo list created by one Git version is > >> to be ever used by another, right? > > > > No. But by scripts based on `git rebase -p`. > > > >> Is there some hidden reason here? Some tools outside of Git that use > >> old todo list format, maybe? > > > > Exactly. > > > > I did mention such a tool: the Git garden shears: > > > > https://github.com/git-for-windows/build-extra/blob/master/shears.sh > > > > Have a look at it. It will inform the discussion. > > I've searched for "-p" in the script, but didn't find positives for > either "-p" or "--preserve-merges". How it would break if it doesn't use > them? What am I missing? *This* particular script does not use -p. But it is not *this* particular script that I do not want to break! It is *all* scripts that use interactive rebase! Don't you also care about not breaking existing users? > >> Then, if new option indeed required, please look at the resulting manual: > >> > >> --recreate-merges:: > >>Recreate merge commits instead of flattening the history by replaying > >>merges. Merge conflict resolutions or manual amendments to merge > >>commits are not preserved. > >> > >> -p:: > >> --preserve-merges:: > >>Recreate merge commits instead of flattening the history by replaying > >>commits a merge commit introduces. Merge conflict resolutions or manual > >>amendments to merge commits are not preserved. > > > > As I stated in the cover letter, there are more patches lined up after > > this patch series. > > Good, but I thought this one should better be self-consistent anyway. > What if those that come later aren't included? Right, let's just rip apart the partial progress because the latter patches might not make it in? I cannot work on that basis, and I also do not want to work on that basis. If you do not like how the documentation is worded, fine, suggest a better alternative. > > Have a look at https://github.com/git/git/pull/447, especially the > > latest commit in there which is an early version of the deprecation I > > intend to bring about. > > You shouldn't want a deprecation at all should you have re-used > --preserve-merges in the first place, and I still don't see why you > haven't. Keep repeating it, and it won't become truer. If you break formats, you break scripts. Git has *so* many users, there are very likely some who script *every* part of it. We simply cannot do that. What we can is deprecate designs which we learned on the way were not only incomplete from the get-go, but bad ove
Re: [PATCH v3 04/12] sequencer: introduce new commands to reset the revision
Hi Eric,
On Mon, 12 Feb 2018, Eric Sunshine wrote:
> On Sat, Feb 10, 2018 at 7:10 PM, Johannes Schindelin
> wrote:
> > [...]
> > This commit implements the commands to label, and to reset to, given
> > revisions. The syntax is:
> >
> > label
> > reset
> > [...]
> > Signed-off-by: Johannes Schindelin
> > ---
> > diff --git a/sequencer.c b/sequencer.c
> > @@ -1922,6 +1951,151 @@ static int do_exec(const char *command_line)
> > +static int safe_append(const char *filename, const char *fmt, ...)
> > +{
> > + [...]
> > + if (write_in_full(fd, buf.buf, buf.len) < 0) {
> > + error_errno(_("could not write to '%s'"), filename);
> > + rollback_lock_file(&lock);
>
> strbuf_release(&buf);
>
> > + return -1;
> > + }
> > + if (commit_lock_file(&lock) < 0) {
> > + rollback_lock_file(&lock);
>
> strbuf_release(&buf);
>
> > + return error(_("failed to finalize '%s'"), filename);
> > + }
> > +
>
> strbuf_release(&buf);
>
> > + return 0;
> > +}
> > +
> > +static int do_reset(const char *name, int len, struct replay_opts *opts)
> > +{
> > + [...]
> > + unpack_tree_opts.reset = 1;
> > +
> > + if (read_cache_unmerged())
>
> rollback_lock_file(&lock);
> strbuf_release(&ref_name);
Thank you very much! I fixed these locally and force-pushed the
recreate-merges branch to https://github.com/dscho/git. These fixes will
be part of v4.
Ciao,
Dscho
Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees
> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:
>
> Git commands which run hooks do so at the top level of the worktree in
> which the command itself was invoked. However, the 'git worktree'
> command may need to run hooks within some other directory. For
> instance, when "git worktree add" runs the 'post-checkout' hook, the
> hook must be run within the newly-created worktree, not within the
> worktree from which "git worktree add" was invoked.
>
> To support this case, add 'run-hook' overloads which allow the
> worktree directory to be specified.
>
> Signed-off-by: Eric Sunshine
> ---
> run-command.c | 23 +--
> run-command.h | 4
> 2 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 31fc5ea86e..0e3995bbf9 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1197,7 +1197,8 @@ const char *find_hook(const char *name)
> return path.buf;
> }
>
> -int run_hook_ve(const char *const *env, const char *name, va_list args)
> +int run_hook_cd_ve(const char *dir, const char *const *env, const char *name,
> +va_list args)
> {
> struct child_process hook = CHILD_PROCESS_INIT;
> const char *p;
> @@ -1206,9 +1207,10 @@ int run_hook_ve(const char *const *env, const char
> *name, va_list args)
> if (!p)
> return 0;
>
> - argv_array_push(&hook.args, p);
> + argv_array_push(&hook.args, absolute_path(p));
> while ((p = va_arg(args, const char *)))
> argv_array_push(&hook.args, p);
> + hook.dir = dir;
> hook.env = env;
> hook.no_stdin = 1;
> hook.stdout_to_stderr = 1;
> @@ -1216,6 +1218,23 @@ int run_hook_ve(const char *const *env, const char
> *name, va_list args)
> return run_command(&hook);
> }
>
> +int run_hook_ve(const char *const *env, const char *name, va_list args)
> +{
> + return run_hook_cd_ve(NULL, env, name, args);
> +}
I think we have only one more user for this function:
builtin/commit.c: ret = run_hook_ve(hook_env.argv,name, args);
The other function 'run_hook_le' is used in a few places:
builtin/am.c: ret = run_hook_le(NULL, "applypatch-msg",
am_path(state, "final-commit"), NULL);
builtin/am.c: if (run_hook_le(NULL, "pre-applypatch", NULL))
builtin/am.c: run_hook_le(NULL, "post-applypatch", NULL);
builtin/checkout.c: return run_hook_le(NULL, "post-checkout",
builtin/clone.c:err |= run_hook_le(NULL, "post-checkout",
sha1_to_hex(null_sha1),
builtin/gc.c: if (run_hook_le(NULL, "pre-auto-gc", NULL))
builtin/merge.c:run_hook_le(NULL, "post-merge", squash ? "1" :
"0", NULL);
builtin/receive-pack.c: if (run_hook_le(env->argv,
push_to_checkout_hook,
Would it be an option to just use the new function signature
everywhere and remove the wrapper? Or do we value the old interface?
- Lars
> +
> +int run_hook_cd_le(const char *dir, const char *const *env, const char
> *name, ...)
> +{
> + va_list args;
> + int ret;
> +
> + va_start(args, name);
> + ret = run_hook_cd_ve(dir, env, name, args);
> + va_end(args);
> +
> + return ret;
> +}
> +
> int run_hook_le(const char *const *env, const char *name, ...)
> {
> va_list args;
> diff --git a/run-command.h b/run-command.h
> index 3932420ec8..8beddffea8 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -66,7 +66,11 @@ int run_command(struct child_process *);
> extern const char *find_hook(const char *name);
> LAST_ARG_MUST_BE_NULL
> extern int run_hook_le(const char *const *env, const char *name, ...);
> +extern int run_hook_cd_le(const char *dir, const char *const *env,
> + const char *name, ...);
> extern int run_hook_ve(const char *const *env, const char *name, va_list
> args);
> +extern int run_hook_cd_ve(const char *dir, const char *const *env,
> + const char *name, va_list args);
>
> #define RUN_COMMAND_NO_STDIN 1
> #define RUN_GIT_CMD2 /*If this is to be git sub-command */
> --
> 2.16.1.291.g4437f3f132
>
Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store
Stefan Beller writes:
> I thought it may be a helpful
> for merging this series with the rest of the evolved code base which
> may make use of one of the converted functions. So instead of fixing
> that new instance manually, cocinelle could do that instead.
Having the .cocci used for the conversion *somewhere* would indeed
be helpful, as it allows me to (1) try reproducing this patch by
somebody else using the file and following the steps in order to
audit this patch and (2) catch new places that need to be migrated
in in-flight topics.
But placing it in contrib/coccinelle/ has other side effects.
I can think of two precedents in this project, namely:
- fixup-builtins in 36e5e70e ("Start deprecating "git-command" in
favor of "git command"", 2007-06-30)
- convert-cache in d98b46f8 ("Do SHA1 hash _before_ compression.",
2005-04-20)
that are about tools that is useful during a transition period but
can and should be removed after transition is over. These two were
done as one-off and added at the top-level, but perhaps we want a
new directory at the top (e.g. devtools/) to add things like this
and hold them while they are relevant?
Re: [PATCH 2/2] t5556: replace test_i18ngrep with a simple grep
Junio C Hamano writes: >> -test_i18ngrep -E '^(fatal|warning):' actual >> | sort && >> +grep -E '^(fatal|warning):' actual && >> test_i18ncmp expected actual > > OK, but not quite OK. > > Two grep invocations will not leave anything useful in 'actual' > under poison build, and is almost guaranteed that 'expected' would > not match, but that is perfectly OK because the final comparison is > done. Sorry. s/is done./is done with i18ncmp./ is what I wanted to say.
Re: linux-next: unnecessary merge in the v4l-dvb tree
On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell wrote: > > Linus, this happens a bit after the merge window, so I am wondering > about the rational of not doing a fast forward merge when merging a > signed tag (I forget the reasoning). The reasoning is to avoid losing the signature from the tag (when merging a signed tag, the signature gets inserted into the merge commit itself - use "git log --show-signature" to see them). So when I merge a signed tag, I do *not* want to fast-forward to the top commit, because then I'd lose the signature from the tag. Thus the "merging signed tags are non-fast-forward by default" reasoning. But, yes, that reasoning is really only valid for proper merges of new features, not for back-merges. The problem, of course, is that since git is distributed, git doesn't know who is "upstream" and who is "downstream", so there's no _technical_ difference between merging a development tree, and a development tree doing a back-merge of the upstream tree. Maybe it was a mistake to make signed tag merges non-fast-forward, since they cause these kinds of issues with people who use "pull" to update their otherwise unmodified trees. I can always teach myself to just use --no-ff, since I end up doing things like verifying at the signatures anyway. Junio, comments? Linus
Re: [PATCH v3 07/14] commit-graph: update graph-head during write
On 2/12/2018 3:37 PM, Junio C Hamano wrote: Junio C Hamano writes: Derrick Stolee writes: It is possible to have multiple commit graph files in a pack directory, but only one is important at a time. Use a 'graph_head' file to point to the important file. Teach git-commit-graph to write 'graph_head' upon writing a new commit graph file. Why this design, instead of what "repack -a" would do, iow, if there always is a singleton that is the only one that matters, shouldn't the creation of that latest singleton just clear the older ones before it returns control? Note that I am not complaining---I am just curious why we want to expose this "there is one relevant one but we keep irrelevant ones we usually do not look at and need to be garbage collected" to end users, and also expect readers of the series, resulting code and docs would have the same puzzled feeling. Aside: I forgot to mention in my cover letter that the experience around the "--delete-expired" flag for "git commit-graph write" is different than v2. If specified, we delete all ".graph" files in the pack directory other than the one referenced by "graph_head" at the beginning of the process or the one written by the process. If these deletes fail, then we ignore the failure (assuming that they are being used by another Git process). In usual cases, we will delete these expired files in the next instance. I believe this matches similar behavior in gc and repack. -- Back to discussion about the value of "graph_head" -- The current design of using a pointer file (graph_head) is intended to have these benefits: 1. We do not need to rely on a directory listing and mtimes to determine which graph file to use. 2. If we write a new graph file while another git process is reading the existing graph file, we can update the graph_head pointer without deleting the file that is currently memory-mapped. (This is why we cannot just rely on a canonical file name, such as "the_graph", to store the data.) 3. We can atomically change the 'graph_head' file without interrupting concurrent git processes. I think this is different from the "repack" situation because a concurrent process would load all packfiles in the pack directory and possibly have open handles when the repack is trying to delete them. 4. We remain open to making the graph file incremental (as the MIDX feature is designed to do; see [1]). It is less crucial to have an incremental graph file structure (the graph file for the Windows repository is currently ~120MB versus a MIDX file of 1.25 GB), but the graph_head pattern makes this a possibility. I tried to avoid item 1 due to personal taste, and since I am storing the files in the objects/pack directory (so that listing may be very large with a lot of wasted entries). This is less important with our pending change of moving the graph files to a different directory. This also satisfies items 2 and 3, as long as we never write graph files so quickly that we have a collision on mtime. I cannot think of another design that satisfies item 4. As for your end user concerns: My philosophy with this feature is that end users will never interact with the commit-graph builtin. 99% of users will benefit from a repack or GC automatically computing a commit graph (when we add that integration point). The other uses for the builtin are for users who want extreme control over their data, such as code servers and build agents. Perhaps someone with experience managing large repositories with git in a server environment could chime in with some design requirements they would need. Thanks, -Stolee [1] https://public-inbox.org/git/20180107181459.222909-2-dsto...@microsoft.com/ [RFC PATCH 01/18] docs: Multi-Pack Index (MIDX) Design Notes
Re: linux-next: unnecessary merge in the v4l-dvb tree
Em Mon, 12 Feb 2018 13:15:04 -0800 Linus Torvalds escreveu: > On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell > wrote: > > > > Linus, this happens a bit after the merge window, so I am wondering > > about the rational of not doing a fast forward merge when merging a > > signed tag (I forget the reasoning). > > The reasoning is to avoid losing the signature from the tag (when > merging a signed tag, the signature gets inserted into the merge > commit itself - use "git log --show-signature" to see them). > > So when I merge a signed tag, I do *not* want to fast-forward to the > top commit, because then I'd lose the signature from the tag. Thus the > "merging signed tags are non-fast-forward by default" reasoning. > > But, yes, that reasoning is really only valid for proper merges of new > features, not for back-merges. > > The problem, of course, is that since git is distributed, git doesn't > know who is "upstream" and who is "downstream", so there's no > _technical_ difference between merging a development tree, and a > development tree doing a back-merge of the upstream tree. > > Maybe it was a mistake to make signed tag merges non-fast-forward, > since they cause these kinds of issues with people who use "pull" to > update their otherwise unmodified trees. > > I can always teach myself to just use --no-ff, since I end up doing > things like verifying at the signatures anyway. Hmm... at least at git version 2.14.3, git documentation doesn't mention that signed pull requests won't do fast forward. Instead, it says that --ff is the default behavior: --ff When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the default behavior. Btw, even doing: $ git merge -ff v4.16-rc1 it will still produce a git commit for the merge. -- Thanks, Mauro
Re: linux-next: unnecessary merge in the v4l-dvb tree
Linus Torvalds writes: > On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell > wrote: > > The problem, of course, is that since git is distributed, git doesn't > know who is "upstream" and who is "downstream", so there's no > _technical_ difference between merging a development tree, and a > development tree doing a back-merge of the upstream tree. > > Maybe it was a mistake to make signed tag merges non-fast-forward, > since they cause these kinds of issues with people who use "pull" to > update their otherwise unmodified trees. > > I can always teach myself to just use --no-ff, since I end up doing > things like verifying at the signatures anyway. > > Junio, comments? I have a slight suspicion that allowing 'pull' to fast-forward even when merging a signed tag when it is pulling from a configured default remote for the branch the user is on, and otherwise keeping the current behaviour, would make majority of people from both camps happier, but I also have a strong conviction that it is being too clever and making it hard to explain to people to do such a dwim that tries to guess which way is 'upstream'. Another clue we _might_ be able to take advantage of is that when upstream maintainers merge a signed tag, we do *not* fetch and store the tag from downstream contributers in our local repository (it is likely that we have --no-tags in remote..tagopt), but when downstream contributers sync from us with "git pull", they do fetch and store our tags in their local repository. So "git pull $somewhere $tag" that defaults to "--ff" when the tag gets stored somewhere in refs/ (or more explicitly, in refs/tags/) and defaults to "--no-ff" otherwise (i.e. the tag is fetched only to be recorded in the resulting merge, without ever stored in any of our refs), might be a good balance. And it is easy to explain: "We realize that it was a mistake to unconditionally default to --no-ff and we are reverting the default to --ff, but with a twist. When we tell 'pull' to grab a tag, if we do not store it anywhere in our local ref space, that would mean the tag is totally lost if the pull fast-forwards. That is why we still use --no-ff in such a case."
Re: linux-next: unnecessary merge in the v4l-dvb tree
On Mon, Feb 12, 2018 at 1:15 PM, Linus Torvalds wrote: > > The reasoning is to avoid losing the signature from the tag (when > merging a signed tag, the signature gets inserted into the merge > commit itself - use "git log --show-signature" to see them). I think the commit that actually introduced the behavior was fab47d057: merge: force edit and no-ff mode when merging a tag object back in 2011, so we've had this behavior for a long time. So it's probably not be worth tweaking the behavior any more, and maybe we need to educate people to not update to other peoples state with "git pull". Maybe we could just tell people to have something like git config --global alias.update pull --ff-only and use that for "try to update to upstream". Linus
Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store
[removed rene.scha...@lsrfire.ath.cx from cc:; I lost that domain a few years ago. Thanks for the heads-up, Stefan!] Am 12.02.2018 um 20:00 schrieb Stefan Beller: > On Fri, Feb 9, 2018 at 2:09 PM, Junio C Hamano wrote: >> Stefan Beller writes: >> >>> Patch generated by >>> >>> 2. Applying the semantic patch contrib/coccinelle/packed_git.cocci >>> to adjust callers. >> >> About this part... >> >>> diff --git a/contrib/coccinelle/packed_git.cocci >>> b/contrib/coccinelle/packed_git.cocci >>> new file mode 100644 >>> index 00..da317a51a9 >>> --- /dev/null >>> +++ b/contrib/coccinelle/packed_git.cocci >>> @@ -0,0 +1,7 @@ >>> +@@ @@ >>> +- packed_git >>> ++ the_repository->objects.packed_git >>> + >>> +@@ @@ >>> +- packed_git_mru >>> ++ the_repository->objects.packed_git_mru >> >> The above is correct for one-time transition turning pre-transition >> code to post the_repository world, but I am not sure if we want to >> have it in contrib/coccinelle/, where "make coccicheck" looks at, as >> a way to continuously keep an eye on "style" violations like using >> strbuf_addf() for a constant when strbuf_addstr() suffices. >> >> Wouldn't we need a mechanism to ensure that this file will *not* be >> used in "make coccicheck" somehow? object_id.cocci is similar in this regard -- once the conversion is done, we won't need it anymore. > I can omit the cocci files from the patches, if that is better for > maintenance. > > I thought it may be a helpful > for merging this series with the rest of the evolved code base which > may make use of one of the converted functions. So instead of fixing > that new instance manually, cocinelle could do that instead. Right, merging should be easier -- instead of fixing conflicts manually, Coccinelle could regenerate the patch. René
Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store
Am 12.02.2018 um 22:04 schrieb Junio C Hamano:
> Stefan Beller writes:
>
>> I thought it may be a helpful
>> for merging this series with the rest of the evolved code base which
>> may make use of one of the converted functions. So instead of fixing
>> that new instance manually, cocinelle could do that instead.
>
> Having the .cocci used for the conversion *somewhere* would indeed
> be helpful, as it allows me to (1) try reproducing this patch by
> somebody else using the file and following the steps in order to
> audit this patch and (2) catch new places that need to be migrated
> in in-flight topics.
>
> But placing it in contrib/coccinelle/ has other side effects.
Running "make coccicheck" takes longer. What other downsides are
there?
> I can think of two precedents in this project, namely:
>
> - fixup-builtins in 36e5e70e ("Start deprecating "git-command" in
> favor of "git command"", 2007-06-30)
>
> - convert-cache in d98b46f8 ("Do SHA1 hash _before_ compression.",
> 2005-04-20)
>
> that are about tools that is useful during a transition period but
> can and should be removed after transition is over. These two were
> done as one-off and added at the top-level, but perhaps we want a
> new directory at the top (e.g. devtools/) to add things like this
> and hold them while they are relevant?
Semantic patches for completed transformations can be removed as
well (or archived, e.g. by renaming to .cocci.done or so).
René
Re: linux-next: unnecessary merge in the v4l-dvb tree
Linus Torvalds writes: > Maybe we could just tell people to have something like > >git config --global alias.update pull --ff-only > > and use that for "try to update to upstream". I guess our mails crossed. I admit that I indeed wondered why you were not giving your usual "downstream shouldn't do pointless pull from upstream" briefly but focused too much on how to tweak the default without thinking through. But I wonder why "update to upstream" is merging a signed tag in the first place. Wouldn't downstream's "try to keep up with" pull be grabbing from branch tips, not tags?
Re: [PATCH 003/194] object-store: move packed_git and packed_git_mru to object store
René Scharfe writes: > Am 12.02.2018 um 22:04 schrieb Junio C Hamano: >> Stefan Beller writes: >> >>> I thought it may be a helpful >>> for merging this series with the rest of the evolved code base which >>> may make use of one of the converted functions. So instead of fixing >>> that new instance manually, cocinelle could do that instead. >> >> Having the .cocci used for the conversion *somewhere* would indeed >> be helpful, as it allows me to (1) try reproducing this patch by >> somebody else using the file and following the steps in order to >> audit this patch and (2) catch new places that need to be migrated >> in in-flight topics. >> >> But placing it in contrib/coccinelle/ has other side effects. > > Running "make coccicheck" takes longer. What other downsides are > there? Once the global variable packed_git has been migrated out of existence, no new code that relies on it would be referring to that global variable. If coccicheck finds something, the suggested rewrite would be turning an unrelated packed_git (which may not even be the right type) to a reference to a field in a global variable, that would certainly be wrong.
Re: [PATCH 1/2] run-command: teach 'run_hook' about alternate worktrees
On Mon, Feb 12, 2018 at 3:58 PM, Lars Schneider
wrote:
>> On 12 Feb 2018, at 04:15, Eric Sunshine wrote:
>> +int run_hook_ve(const char *const *env, const char *name, va_list args)
>> +{
>> + return run_hook_cd_ve(NULL, env, name, args);
>> +}
>
> I think we have only one more user for this function:
> builtin/commit.c: ret = run_hook_ve(hook_env.argv,name, args);
>
> Would it be an option to just use the new function signature
> everywhere and remove the wrapper? Or do we value the old interface?
I did note that there was only one existing caller and considered
simply modifying run_hook_ve()'s signature to accept the new 'dir'
argument. I don't feel strongly one way or the other.
However, as mentioned elsewhere[1], I'm still not convinced that this
one-off case of needing to chdir() warrants adding these overloads to
'run-command' at all, so I'm strongly considering (and was considering
even for v1) instead just running this hook manually in
builtin/worktree.c.
[1]:
https://public-inbox.org/git/CAPig+cQ6Tq3J=bS8ymDqiXqUvoUiP59T=FGZgMw2FOAx0vyo=q...@mail.gmail.com/
Re: linux-next: unnecessary merge in the v4l-dvb tree
On Mon, Feb 12, 2018 at 1:44 PM, Junio C Hamano wrote: > > But I wonder why "update to upstream" is merging a signed tag in the > first place. Wouldn't downstream's "try to keep up with" pull be > grabbing from branch tips, not tags? I'm actually encouraging maintainers to *not* start their work on some random "kernel of the day". Particularly during the kernel merge window, the upstream master branch can be pretty flaky. It's *not* a good point to start new development on, so if you're a maintainer, you really don't want to use that as the basis for your work for the next merge window. So I encourage people to use a stable point for new development, and particularly actual release kernels. The natural way to do that is obviously just to create a new branch: git checkout -b topicbranch v4.15 and now you have a good new clean branch for your development. But clearly we've got a few people who have gotten used to the whole "git pull" convenience of both fetching and updating. Some maintainers don't even use topic branches, because their main work is just merging work by subdevelepoers (that goes for my own tree: I use topic branches for merging patch queues and to occasionally track my own experimental patches, but 99% of the time I'm just on "master" and obviously pull other peoples branches). And some maintainers end up using multiple repositories as branches (the old _original_ git model). Again, you can just use "git fetch + git reset", of course, but that's a bit unsafe. In contrast, doing "git pull --ff-only" is a safe convenient operation that does both the fetch and the update to whatever state. But you do need that "--ff-only" to avoid the merge. Linus
Re: [PATCH] color.h: document and modernize header
On Mon, Feb 12, 2018 at 3:19 PM, Stefan Beller wrote: > Add documentation explaining the functions in color.h. > While at it, mark them extern and migrate the function `color_set` > into grep.c, where the only callers are. This re-roll no longer marks functions as 'extern', so the commit message saying that it does seems rather odd. > Signed-off-by: Stefan Beller > --- > diff --git a/color.h b/color.h > @@ -76,22 +76,47 @@ int git_color_config(const char *var, const char *value, > void *cb); > +/* > + * Return a boolean whether to use color, > + * resolving ambigious settings such as GIT_COLOR_AUTO, which is returned > + * by git_config_colorbool(). > + */ > int want_color(int var); I'm probably being dense, but (if I hadn't had Peff's explanation[1] to fall back on), based upon the comment block alone, I'd still have no idea what I'm supposed to pass in as 'var'. Partly this is due to the comment block not mentioning 'var' explicitly; it talks about GIT_COLOR_AUTO and git_config_colorbool() abstractly, and, as a reader, I can't tell if those are supposed to be passed in as 'var' or if the function somehow grabs them out of the environment. Partly it's due to the name "var" not conveying any useful meaning. Perhaps take Peff's hint and state explicitly that the passed-in value is one of GIT_COLOR_UNKNOWN, GIT_COLOR_NEVER, GIT_COLOR_ALWAYS, GIT_COLOR_AUTO, then further explain that that value comes from git_config_colorbool(), possibly modified by local option processing, such as --color. [1]: https://public-inbox.org/git/20180208222851.ga8...@sigill.intra.peff.net/ > +/* > + * Output the formatted string in the specified color (and then reset to > normal > + * color so subsequent output is uncolored). Omits the color encapsulation if > + * `color` is NULL. The `color_fprintf_ln` prints a new line after resetting > + * the color. The `color_print_strbuf` prints the given pre-formatted strbuf > + * instead, up to its first NUL character. > + */ Perhaps prefix the last sentence with "BUG:" since we don't want people relying on this NUL bug/misfeature.
Re: [PATCH 3/7] worktree move: new command
On 12 February 2018 at 10:56, Duy Nguyen wrote: > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren wrote: >> On 6 February 2018 at 03:13, Jeff King wrote: >>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote: I learned SANITIZE=leak today! It not only catches this but also "dst". Jeff is there any ongoing effort to make the test suite pass with SANITIZE=leak? My t2038 passed, so I went ahead with the full test suite and saw so many failures. I did see in your original mails that you focused on t and t0001 only.. >>> >>> Yeah, I did those two scripts to try to prove to myself that the >>> approach was good. But I haven't really pushed it any further. >>> >>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a >>> long way to go. >> >> Agreed. :-) > > Should we mark the test files that pass SANITIZE=leak and run as a sub > testsuite? This way we can start growing it until it covers everything > (unlikely) and make sure people don't break what's already passed. > > Of course I don't expect everybody to run this new make target with > SANITIZE=leak. Travis can do that for us and hopefully have some way > to tell git@vger about new breakages. Makes sense to try to make sure that we don't regress leak-free tests. I don't know what our Travis-budget looks like, but I would volunteer to run something like this periodically using my own cycles. My experience with the innards of our Makefiles and test-lib.sh is non-existant, but from a very cursory look it seems like something as simple as loading GIT_SKIP_TESTS from a blacklist-file might do the trick. I could try to look into it in the next couple of days. Martin
Re: [PATCH 1/3] t7006: add tests for how git config paginates
Martin Ågren writes:
> +test_expect_success TTY 'git config respects pager.config when setting' '
> + rm -f paginated.out &&
> + test_terminal git -c pager.config config foo.bar bar &&
> + test -e paginated.out
> +'
I am debating myself if this test should instead spell out what we
eventually want from the above test and make it expect_failure, just
like the next one.
In addition to setting (which will start ignoring pager in later
steps), unsetting, replacing of a variable and renaming/removing a
section in a configuration should not page, I would suspect. Should
we test them all?
> +test_expect_failure TTY 'git config --edit ignores pager.config' '
> + rm -f paginated.out editor.used &&
> + write_script editor <<-\EOF &&
> + touch editor.used
> + EOF
> + EDITOR=./editor test_terminal git -c pager.config config --edit &&
> + ! test -e paginated.out &&
> + test -e editor.used
> +'
> +
> +test_expect_success TTY 'git config --get defaults to not paging' '
> + rm -f paginated.out &&
> + test_terminal git config --get foo.bar &&
> + ! test -e paginated.out
> +'
> +
> +test_expect_success TTY 'git config --get respects pager.config' '
> + rm -f paginated.out &&
> + test_terminal git -c pager.config config --get foo.bar &&
> + test -e paginated.out
> +'
> +
> +test_expect_success TTY 'git config --list defaults to not paging' '
> + rm -f paginated.out &&
> + test_terminal git config --list &&
> + ! test -e paginated.out
> +'
> +
> +
> # A colored commit log will begin with an appropriate ANSI escape
> # for the first color; the text "commit" comes later.
> colorful() {
Re: [PATCH 1/3] t7006: add tests for how git config paginates
On 12 February 2018 at 23:17, Junio C Hamano wrote: > Martin Ågren writes: > >> +test_expect_success TTY 'git config respects pager.config when setting' ' >> + rm -f paginated.out && >> + test_terminal git -c pager.config config foo.bar bar && >> + test -e paginated.out >> +' > > I am debating myself if this test should instead spell out what we > eventually want from the above test and make it expect_failure, just > like the next one. That's a valid point. I was coming at it from the point of view of "the current behavior is well-defined and non-broken -- we'll soon change it, but that's more a redefinition, not a bugfix (as with --edit)". But I could go either way. There is some prior art in ma/branch-list-paginate, where I handled `git branch --set-upstream-to` similar to here, cf. d74b541e0b (branch: respect `pager.branch` in list-mode only, 2017-11-19). > In addition to setting (which will start ignoring pager in later > steps), unsetting, replacing of a variable and renaming/removing a > section in a configuration should not page, I would suspect. Should > we test them all? I actually had several more tests in an early draft, including --unset. Similarly, testing all the --get-* would be possible but feels like overkill. From the implementation, it's "obvious enough" (famous last words) that there are two classes of arguments, and by testing a few from each class we should be home free. This now comes to `git config` after `git tag` and `git branch`, where the "complexity" of the problem has been steadily increasing. (Off the top of my head, tag has two modes, branch has three, config has lots.) That the tests don't get more complex might be bad, or good. But all of these use the same basic API (DELAY_PAGER_CONFIG) in the same rather simple way. I actually almost had the feeling that these tests here were too much, considering that DELAY_PAGER_CONFIG gets tested quite a few times by now. Thanks for your comments. I'll ponder this, and see what I come up with. Maybe a changed approach, or maybe an extended commit message. Any further input welcome, as always. Martin
Re: linux-next: unnecessary merge in the v4l-dvb tree
Linus Torvalds writes: > And some maintainers end up using multiple repositories as branches > (the old _original_ git model). Again, you can just use "git fetch + > git reset", of course, but that's a bit unsafe. In contrast, doing > "git pull --ff-only" is a safe convenient operation that does both the > fetch and the update to whatever state. > > But you do need that "--ff-only" to avoid the merge. OK. I guess it is legit (and semi-sensible) for downstream contributors to "git pull --ff-only $upstream $release_tag_X" to bring their long-running topic currently based on release X-1 up to date with respect to release X. It probably makes more sense than rebasing on top of release X, even though it makes a lot less sense than merging their topics into release X. As you said, pull of a tag that forbids fast-forward by default is rather old development (I am kind of surprised that it was so old, in v1.7.9), so it may be a bit difficult to transition. There is [pull] ff = only but pull.ff is quite global, and not good for intermediate level maintainers who pull to integrate work of their downstream (for which they do want the current "do not ff, record the tag in a merge commit" behaviour) and also pull to catch up from their upstream (which they want "ff-when-able"). They need to control between ff=only and ff=when-able, depending on whom they are pulling from. We may want per-remote equivalent for it, i.e. e.g. [pull] ff=false ;# good default for collecting contributions [remote "torvalds"] pullFF = only ;# good default for catching up or something like that, perhaps?
Re: linux-next: unnecessary merge in the v4l-dvb tree
Em Mon, 12 Feb 2018 15:42:44 -0800 Junio C Hamano escreveu: > Linus Torvalds writes: > > > And some maintainers end up using multiple repositories as branches > > (the old _original_ git model). Again, you can just use "git fetch + > > git reset", of course, but that's a bit unsafe. In contrast, doing > > "git pull --ff-only" is a safe convenient operation that does both the > > fetch and the update to whatever state. > > > > But you do need that "--ff-only" to avoid the merge. > > OK. I guess it is legit (and semi-sensible) for downstream > contributors to "git pull --ff-only $upstream $release_tag_X" to > bring their long-running topic currently based on release X-1 up to > date with respect to release X. It probably makes more sense than > rebasing on top of release X, even though it makes a lot less sense > than merging their topics into release X. > > As you said, pull of a tag that forbids fast-forward by default is > rather old development (I am kind of surprised that it was so old, > in v1.7.9), so it may be a bit difficult to transition. > > There is > > [pull] > ff = only > > but pull.ff is quite global, and not good for intermediate level > maintainers who pull to integrate work of their downstream (for > which they do want the current "do not ff, record the tag in a merge > commit" behaviour) and also pull to catch up from their upstream > (which they want "ff-when-able"). They need to control between > ff=only and ff=when-able, depending on whom they are pulling from. Yes, that's my pain. I don't want ff only when pulling from others, only when pulling from upstream tree. > > We may want per-remote equivalent for it, i.e. e.g. > > [pull] > ff=false ;# good default for collecting contributions > > [remote "torvalds"] > pullFF = only ;# good default for catching up > > or something like that, perhaps? Yeah, something like that works. Please notice, however, that what I usually do is: $ git remote update torvalds $ git merge (or git pull . ) So, for the above to work, it should store somehow the remote from where a tag came from. The reason is that I keep locally a cache with several tree clones (in bare mode) s that I bother enough to cache (linus, -stable, -next), as pulling from BR is time consuming, and I want to do it only once and use the same "cache" for all my git clones. I have a few git workdirs for my upstream work, but, as a patch developer, I also have "independent"[1] git repositories. [1] Due to disk constraints, the clones actually use --shared. So, the common objects are actually stored inside a single tree. Thanks, Mauro
Re: [PATCH 3/7] worktree move: new command
On Mon, Feb 12, 2018 at 11:15:06PM +0100, Martin Ågren wrote: > On 12 February 2018 at 10:56, Duy Nguyen wrote: > > On Wed, Feb 7, 2018 at 3:05 AM, Martin Ågren wrote: > >> On 6 February 2018 at 03:13, Jeff King wrote: > >>> On Mon, Feb 05, 2018 at 08:28:10PM +0700, Duy Nguyen wrote: > I learned SANITIZE=leak today! It not only catches this but also "dst". > > Jeff is there any ongoing effort to make the test suite pass with > SANITIZE=leak? My t2038 passed, so I went ahead with the full test > suite and saw so many failures. I did see in your original mails that > you focused on t and t0001 only.. > >>> > >>> Yeah, I did those two scripts to try to prove to myself that the > >>> approach was good. But I haven't really pushed it any further. > >>> > >>> Martin Ågren (cc'd) did some follow-up work, but I think we still have a > >>> long way to go. > >> > >> Agreed. :-) > > > > Should we mark the test files that pass SANITIZE=leak and run as a sub > > testsuite? This way we can start growing it until it covers everything > > (unlikely) and make sure people don't break what's already passed. > > > > Of course I don't expect everybody to run this new make target with > > SANITIZE=leak. Travis can do that for us and hopefully have some way > > to tell git@vger about new breakages. > > Makes sense to try to make sure that we don't regress leak-free tests. I > don't know what our Travis-budget looks like, but I would volunteer to > run something like this periodically using my own cycles. > > My experience with the innards of our Makefiles and test-lib.sh is > non-existant, but from a very cursory look it seems like something as > simple as loading GIT_SKIP_TESTS from a blacklist-file might do the > trick. Yeah my first throught was something along that line too. But maybe it's nicer to mark a test file leak-free like this: -- 8< -- diff --git a/t/t2028-worktree-move.sh b/t/t2028-worktree-move.sh index 459f676683..1446f075b7 100755 --- a/t/t2028-worktree-move.sh +++ b/t/t2028-worktree-move.sh @@ -2,6 +2,8 @@ test_description='test git worktree move, remove, lock and unlock' +test_leak_free=yes + . ./test-lib.sh test_expect_success 'setup' ' -- 8< -- And we can run these test files with a new option --leak-check (or something like that, we already support a similar option --valgrind). Most of the work will be in test-lib.sh. If we detect --leak-check and test_leak_free is not set, we skip the whole file. In the far far far future when all files have test_leak_free=yes, we can flip the default and delete these lines. It would be nice if test-lib.sh can determine if 'git' binary is built with SANITIZE=yes, but I think we can live without it. > I could try to look into it in the next couple of days. Have fun :) Let me know if you give up though, I'll give it a shot. -- Duy
Re: [PATCH v3 01/35] pkt-line: introduce packet_read_with_status
Hi,
Brandon Williams wrote:
> The current pkt-line API encodes the status of a pkt-line read in the
> length of the read content. An error is indicated with '-1', a flush
> with '0' (which can be confusing since a return value of '0' can also
> indicate an empty pkt-line), and a positive integer for the length of
> the read content otherwise. This doesn't leave much room for allowing
> the addition of additional special packets in the future.
>
> To solve this introduce 'packet_read_with_status()' which reads a packet
> and returns the status of the read encoded as an 'enum packet_status'
> type. This allows for easily identifying between special and normal
> packets as well as errors. It also enables easily adding a new special
> packet in the future.
Makes sense, thanks. Using an enum return value is less opaque, too.
[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -280,28 +280,33 @@ static int packet_length(const char *linelen)
> return (val < 0) ? val : (val << 8) | hex2chr(linelen + 2);
> }
>
> -int packet_read(int fd, char **src_buf, size_t *src_len,
> - char *buffer, unsigned size, int options)
> +enum packet_read_status packet_read_with_status(int fd, char **src_buffer,
> size_t *src_len,
> + char *buffer, unsigned size,
> int *pktlen,
> + int options)
This function definition straddles two worlds: it is line-wrapped as
though there are a limited number of columns, but it goes far past 80
columns.
Can "make style" or a similar tool take care of rewrapping it?
> {
> - int len, ret;
> + int len;
> char linelen[4];
>
> - ret = get_packet_data(fd, src_buf, src_len, linelen, 4, options);
> - if (ret < 0)
> - return ret;
> + if (get_packet_data(fd, src_buffer, src_len, linelen, 4, options) < 0)
> + return PACKET_READ_EOF;
> +
EOF is indeed the only error that get_packet_data can return.
Could be worth a doc comment on get_packet_data to make that clearer.
It's not too important since it's static, though.
> len = packet_length(linelen);
> - if (len < 0)
> +
> + if (len < 0) {
> die("protocol error: bad line length character: %.4s", linelen);
> - if (!len) {
> + } else if (!len) {
> packet_trace("", 4, 0);
> - return 0;
> + return PACKET_READ_FLUSH;
The advertised change. Makes sense.
[...]
> - if (len >= size)
> + if ((unsigned)len >= size)
> die("protocol error: bad line length %d", len);
The comparison is safe since we just checked that len >= 0.
Is there some static analysis that can make this kind of operation
easier?
[...]
> @@ -309,7 +314,31 @@ int packet_read(int fd, char **src_buf, size_t *src_len,
>
> buffer[len] = 0;
> packet_trace(buffer, len, 0);
> - return len;
> + *pktlen = len;
> + return PACKET_READ_NORMAL;
> +}
> +
> +int packet_read(int fd, char **src_buffer, size_t *src_len,
> + char *buffer, unsigned size, int options)
> +{
> + enum packet_read_status status;
> + int pktlen;
> +
> + status = packet_read_with_status(fd, src_buffer, src_len,
> + buffer, size, &pktlen,
> + options);
> + switch (status) {
> + case PACKET_READ_EOF:
> + pktlen = -1;
> + break;
> + case PACKET_READ_NORMAL:
> + break;
> + case PACKET_READ_FLUSH:
> + pktlen = 0;
> + break;
> + }
> +
> + return pktlen;
nit: can simplify by avoiding the status temporary:
int pktlen;
switch (packet_read_with_status(...)) {
case PACKET_READ_EOF:
return -1;
case PACKET_READ_FLUSH:
return 0;
case PACKET_READ_NORMAL:
return pktlen;
}
As a bonus, that lets static analyzers check that the cases are
exhaustive. (On the other hand, C doesn't guarantee that an enum can
only have the values listed as enumerators. Did we end up figuring
out a way to handle that, beyond always including a 'default: BUG()'?)
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -65,6 +65,21 @@ int write_packetized_from_buf(const char *src_in, size_t
> len, int fd_out);
> int packet_read(int fd, char **src_buffer, size_t *src_len, char
> *buffer, unsigned size, int options);
>
> +/*
> + * Read a packetized line into a buffer like the 'packet_read()' function but
> + * returns an 'enum packet_read_status' which indicates the status of the
> read.
> + * The number of bytes read will be assigined to *pktlen if the status of the
> + * read was 'PACKET_READ_NORMAL'.
> + */
> +enum packet_read_status {
> + PACKET_READ_EOF = -1,
> + PACKET_READ_NORMAL,
> + PACKET_READ_FLUSH,
> +};
nit: do any callers treat the return value as a number? It would be
less magical
[PATCH] t6300-for-each-ref: fix "more than one quoting style" tests
'git for-each-ref' should error out when invoked with more than one quoting style options. The tests checking this have two issues: - They run 'git for-each-ref' upstream of a pipe, hiding its exit code, thus don't actually checking that 'git for-each-ref' exits with error code. - They check the error message in a rather roundabout way. Ensure that 'git for-each-ref' exits with an error code using the 'test_must_fail' helper function, and check its error message by grepping its saved standard error. Signed-off-by: SZEDER Gábor --- Those tests were like this since ancient times, c9ecf4f12a (for-each-ref: Fix quoting style constants., 2007-12-06). t/t6300-for-each-ref.sh | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh index c128dfc579..295d1475bd 100755 --- a/t/t6300-for-each-ref.sh +++ b/t/t6300-for-each-ref.sh @@ -373,11 +373,8 @@ test_expect_success 'Quoting style: tcl' ' for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do test_expect_success "more than one quoting style: $i" " - git for-each-ref $i 2>&1 | (read line && - case \$line in - \"error: more than one quoting style\"*) : happy;; - *) false - esac) + test_must_fail git for-each-ref $i 2>err && + grep '^error: more than one quoting style' err " done -- 2.16.1.181.g4b60b0bfb6
Re: Question about rebasing - unexpected result, can't figure out why
On Sat, Feb 10, 2018 at 09:47:57PM +0100, Jonas Thiem wrote: > == Why did I expect that == > > Of course after the client rebase, C3.txt should be gone (since it's > gone at the original last commit of the client branch). > > But since it still exists in the server branch at the final commit, > after rebasing & reapplying that onto the master, shouldn't it be put > back in? Also, I would expect C3 -> C4 -> C10 as the complete chain of > commits of the remaining server branch to be attached at the end, not > just C4 -> C10... > > Does git somehow detect C3 would be applied twice? Doesn't the commit > hash of C3 change after it is rebased? So how exactly would it figure > that out? I'm really unsure about what is going on. Yes. The git rebase documentation says, "Note that any commits in HEAD which introduce the same textual changes as a commit in HEAD.. are omitted (i.e., a patch already accepted upstream with a different commit message or timestamp will be skipped)." If you want to be very explicit about replaying these commits, you can say "git rebase --onto master $(git merge-base master HEAD)", in which case C3 will be replayed. -- brian m. carlson / brian with sandals: Houston, Texas, US https://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: https://keybase.io/bk2204 signature.asc Description: PGP signature
Re: [PATCH v3 02/35] pkt-line: introduce struct packet_reader
Hi,
Brandon Williams wrote:
> Subject: pkt-line: introduce struct packet_reader
nit: this subject line doesn't describe what the purpose/intent behind
the patch is. Maybe something like
pkt-line: allow peeking at a packet line without consuming it
would make it clearer.
> Sometimes it is advantageous to be able to peek the next packet line
> without consuming it (e.g. to be able to determine the protocol version
> a server is speaking). In order to do that introduce 'struct
> packet_reader' which is an abstraction around the normal packet reading
> logic. This enables a caller to be able to peek a single line at a time
> using 'packet_reader_peek()' and having a caller consume a line by
> calling 'packet_reader_read()'.
Makes sense. The packet_reader owns a buffer to support the peek
operation and make buffer reuse a little easier.
[...]
> --- a/pkt-line.h
> +++ b/pkt-line.h
> @@ -111,6 +111,64 @@ char *packet_read_line_buf(char **src_buf, size_t
> *src_len, int *size);
> */
> ssize_t read_packetized_to_strbuf(int fd_in, struct strbuf *sb_out);
>
> +struct packet_reader {
> + /* source file descriptor */
> + int fd;
> +
> + /* source buffer and its size */
> + char *src_buffer;
> + size_t src_len;
Can or should this be a strbuf?
> +
> + /* buffer that pkt-lines are read into and its size */
> + char *buffer;
> + unsigned buffer_size;
Likewise.
> +
> + /* options to be used during reads */
> + int options;
What option values are possible?
> +
> + /* status of the last read */
> + enum packet_read_status status;
This reminds me of FILE*'s status value, ferror, etc. I'm mildly
nervous about it --- it encourages a style of error handling where you
ignore errors from an individual operation and hope the recorded
status later has the most relevant error.
I think it is being used to support peek --- you need to record the
error to reply it. Is that right? I wonder if it would make sense to
structure as
struct packet_read_result last_line_read;
};
struct packet_read_result {
enum packet_read_status status;
const char *line;
int len;
};
What you have here also seems fine. I think what would help most
for readability is if the comment mentioned the purpose --- e.g.
/* status of the last read, to support peeking */
Or if the contract were tied to the purpose:
/* status of the last read, only valid if line_peeked is true */
[...]
> +/*
> + * Initialize a 'struct packet_reader' object which is an
> + * abstraction around the 'packet_read_with_status()' function.
> + */
> +extern void packet_reader_init(struct packet_reader *reader, int fd,
> +char *src_buffer, size_t src_len,
> +int options);
This comment doesn't describe how I should use the function. Is the
intent to point the reader to packet_read_with_status for more details
about the arguments?
Can src_buffer be a const char *?
[...]
> +/*
> + * Perform a packet read and return the status of the read.
nit: s/Perform a packet read/Read one pkt-line/
> + * The values of 'pktlen' and 'line' are updated based on the status of the
> + * read as follows:
> + *
> + * PACKET_READ_ERROR: 'pktlen' is set to '-1' and 'line' is set to NULL
> + * PACKET_READ_NORMAL: 'pktlen' is set to the number of bytes read
> + * 'line' is set to point at the read line
> + * PACKET_READ_FLUSH: 'pktlen' is set to '0' and 'line' is set to NULL
> + */
> +extern enum packet_read_status packet_reader_read(struct packet_reader
> *reader);
This is reasonable. As described above an alternative would be
possible to have a separate packet_read_result output parameter but
the interface described here looks pretty easy/pleasant to use.
> +
> +/*
> + * Peek the next packet line without consuming it and return the status.
nit: s/Peek/Peek at/, or s/Peek/Read/
> + * The next call to 'packet_reader_read()' will perform a read of the same
> line
> + * that was peeked, consuming the line.
nit: s/peeked/peeked at/
> + *
> + * Peeking multiple times without calling 'packet_reader_read()' will return
> + * the same result.
> + */
> +extern enum packet_read_status packet_reader_peek(struct packet_reader
> *reader);
Nice.
[...]
> --- a/pkt-line.c
> +++ b/pkt-line.c
> @@ -406,3 +406,62 @@ ssize_t read_packetized_to_strbuf(int fd_in, struct
> strbuf *sb_out)
> }
> return sb_out->len - orig_len;
> }
> +
> +/* Packet Reader Functions */
> +void packet_reader_init(struct packet_reader *reader, int fd,
> + char *src_buffer, size_t src_len,
> + int options)
This comment looks like it's attached to packet_reader_init, but it's
meant to be attached to the whole collection. It's possible that this
title-above-multiple-functions won't be maintained, but that's okay.
> +{
> + memset
Re: please change stash
Hi Karsten, > Normal git tooling creates different files file.ORIG file.LOCAL > file.REMOTE in case of conflicts. Which tools are you referring to here? Can you give a short sequence of commands that show what you mean? > However `git stash pop` manipulates your files directly resulting in > lines like: > > <<< Updated upstream > Stashed changes > > This can seriously corrupt files and workflows. This looks like a normal merge conflict. I suspect that you are using tools that know how to deal with this format when it used the merge conflict markers, but maybe not the equivalent markers you get when popping a conflicting stash. To demonstrate, here is a short script: git init test cd test echo "base file" >test git commit -m "base file" git add test git commit -m "base file" git checkout -b conflict_branch echo "conflicting file" >test git commit -am "conflict file" git checkout master echo "updated file" >test git commit -am "updated file" git merge conflict_branch This merge fails, and the file 'test' looks like this: <<< HEAD updated file === conflicting file >>> conflict_branch As you can see, this sequence of actions doesn't result in 3 different files. The merge conflict format is a relatively old one, and lots of tools know how to use it in different ways (such as the tool you are using, I presume) but say this was to be changed for the stash operation - what would you propose replace it? Some options might be to: - instead of placing the conflicts in the original file, place the different conflicting versions into different files - warn when adding/committing/pushing files with conflict markers in them - teach the tool you are using to handle the stash conflict markers in a nicer way Some of these may be possible to do with little work. This link[0] on stack overflow deals with creating separate files, and it looks like it might work for stash pop conflicts. This one[1] shows how to create hooks that catch any conflicts that are being committed, and would also probably work with stash conflicts. Teaching the tool to handle stash conflicts, or making any of the above changes to the base distribution of git would be significantly harder, but maybe this can help you in the meantime. Regards, Andrew Ardill [0] https://stackoverflow.com/questions/47512337/configure-git-to-create-multiple-files-for-merge-conflicts [1] https://stackoverflow.com/questions/24213948/prevent-file-with-merge-conflicts-from-getting-committed-in-git
