* Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: Jeff King @ 2016-09-30 22:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, David Turner
In-Reply-To: <xmqqponl84h4.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 30, 2016 at 02:25:43PM -0700, Junio C Hamano wrote:
> > +void add_to_alternates_internal(const char *reference)
> > +{
> > + prepare_alt_odb();
> > + link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0);
> > +}
> > +
>
> A function _internal being extern felt a bit funny. We are only
> appending so the first one does not have to be reprepare.
It's a match for add_to_alternates_file(). Suggestions for a better word
are welcome.
We do need to prepare_alt_odb(), as that is what sets up the
alt_odb_tail pointer. And also, a later prepare() call would overwrite
our entry. We could refactor the alt_odb code, but it seemed simplest
to just make sure we don't add to an unprepared list.
> > + t = xmalloc(sizeof(*t));
> > + strbuf_init(&t->path, 0);
> > + argv_array_init(&t->env);
> > +
> > + strbuf_addf(&t->path, "%s/incoming-XXXXXX", get_object_directory());
>
> I was wondering where you would put this in. Inside .git/objects/
> sounds good.
The name "incoming" is kind of arbitrary and related to the fact that
this is used for receive-pack (though if we were to use it on the
fetching side, I think it would be equally correct). I don't think it
really matters in practice.
> > +static int pack_copy_priority(const char *name)
> > +{
> > + if (!starts_with(name, "pack"))
> > + return 0;
> > + if (ends_with(name, ".keep"))
> > + return 1;
> > + if (ends_with(name, ".pack"))
> > + return 2;
> > + if (ends_with(name, ".idx"))
> > + return 3;
> > + return 4;
> > +}
>
> Thanks for being careful. A blind "cp -r" would have ruined the
> day.
>
> We do not do bitmaps upon receiving, I guess.
But we don't, but they (and anything else) would just sort at the end,
which is OK.
> > + * struct tmp_objdir *t = tmp_objdir_create();
> > + * if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
> > + * !tmp_objdir_migrate(t))
> > + * printf("success!\n");
> > + * else
> > + * die("failed...tmp_objdir will clean up for us");
>
> Made me briefly wonder if a caller might want to use appropriate
> environment to use the tmp-objdir given by the API in addition to
> its own, but then such a caller just needs to prepare its own argv-array
> and concatenate tmp_objdir_env() before making the opt_cd_env call,
> so this is perfectly fine.
Yep, and that's exactly what happens in one spot of the next patch.
My original had just open-coded, but I was happy to see we have
argv_array_pushv() these days, so it's a one-liner.
In the very original version, the receive-pack process did not need to
access the new objects at all (not until ref update time anyway, at
which point they've been migrated). And that's why the environment is
intentionally kept separate, and the caller can feed it to whichever
sub-programs it chooses. But a later version of git that handled shallow
pushes required receive-pack to actually look at the objects, and I
added the add_to_alternates_internal() call you see here.
At that point, it does make me wonder if a better interface would be for
tmp_objdir to just set up the environment variables in the parent
process in the first place, and then restore them upon
tmp_objdir_destroy(). It makes things a bit more automatic, which makes
me hesitate, but I think it would be fine for receive-pack.
I dunno. I mostly left it alone because I did it this way long ago, and
it wasn't broke. Polishing for upstream is an opportunity to fix old
oddities, but I think there is some value in applying a more
battle-tested patch.
-Peff
^ permalink raw reply
* [PATCH v3 0/5] Add --format to tag verification
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
From: Santiago Torres <santiago@nyu.edu>
This is the third iteration of [1][2], and as a result of the discussion
in [3].
In this re-roll we:
* Fixed all the signed-off-by's
[0002]
* Renamed the function format_ref to pretty_print_ref instead, which
is a more descriptive name
[0004]
* Added the respective line for the new --format parameter in the
documentation.
[0005]
* Added mention of the --format flag in the documentation files.
* Fixed the function signatures, now they take an opaque void *cb_data pointer
so it can be used in a more general way (by e.g., delete_tag).
This patch applies to 2.10.0 and master.
[1] http://public-inbox.org/git/20160922185317.349-1-santiago@nyu.edu/
[2] http://public-inbox.org/git/20160926224233.32702-1-santiago@nyu.edu/
[3] http://public-inbox.org/git/20160607195608.16643-1-santiago@nyu.edu/
Lukas Puehringer (4):
gpg-interface, tag: add GPG_VERIFY_QUIET flag
ref-filter: add function to print single ref_array_item
tag: add format specifier to gpg_verify_tag
builtin/tag: add --format argument for tag -v
Santiago Torres (1):
builtin/verify-tag: add --format to verify-tag
Documentation/git-tag.txt | 2 +-
Documentation/git-verify-tag.txt | 2 +-
builtin/tag.c | 34 +++++++++++++++++++++++-----------
builtin/verify-tag.c | 13 +++++++++++--
gpg-interface.h | 1 +
ref-filter.c | 10 ++++++++++
ref-filter.h | 3 +++
tag.c | 22 +++++++++++++++-------
tag.h | 4 ++--
9 files changed, 67 insertions(+), 24 deletions(-)
--
2.10.0
^ permalink raw reply
* [PATCH v3 1/5] gpg-interface, tag: add GPG_VERIFY_QUIET flag
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
Functions that print git object information may require that the
gpg-interface functions be silent. Add GPG_VERIFY_QUIET flag and prevent
print_signature_buffer from being called if flag is set.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
gpg-interface.h | 1 +
tag.c | 5 ++++-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/gpg-interface.h b/gpg-interface.h
index ea68885..85dc982 100644
--- a/gpg-interface.h
+++ b/gpg-interface.h
@@ -3,6 +3,7 @@
#define GPG_VERIFY_VERBOSE 1
#define GPG_VERIFY_RAW 2
+#define GPG_VERIFY_QUIET 4
struct signature_check {
char *payload;
diff --git a/tag.c b/tag.c
index d1dcd18..291073f 100644
--- a/tag.c
+++ b/tag.c
@@ -3,6 +3,7 @@
#include "commit.h"
#include "tree.h"
#include "blob.h"
+#include "gpg-interface.h"
const char *tag_type = "tag";
@@ -24,7 +25,9 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
ret = check_signature(buf, payload_size, buf + payload_size,
size - payload_size, &sigc);
- print_signature_buffer(&sigc, flags);
+
+ if (!(flags & GPG_VERIFY_QUIET))
+ print_signature_buffer(&sigc, flags);
signature_check_clear(&sigc);
return ret;
--
2.10.0
^ permalink raw reply related
* [PATCH v3 2/5] ref-filter: add function to print single ref_array_item
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
ref-filter functions are useful for printing git object information
using a format specifier. However, some other modules may not want to use
this functionality on a ref-array but only print a single item.
Expose a format_ref function to create, pretty print and free individual
ref-items.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
ref-filter.c | 10 ++++++++++
ref-filter.h | 3 +++
2 files changed, 13 insertions(+)
diff --git a/ref-filter.c b/ref-filter.c
index bc551a7..ee3ed67 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1655,6 +1655,16 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu
putchar('\n');
}
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+ const char *format, unsigned kind)
+{
+ struct ref_array_item *ref_item;
+ ref_item = new_ref_array_item(name, sha1, 0);
+ ref_item->kind = kind;
+ show_ref_array_item(ref_item, format, 0);
+ free_array_item(ref_item);
+}
+
/* If no sorting option is given, use refname to sort as default */
struct ref_sorting *ref_default_sorting(void)
{
diff --git a/ref-filter.h b/ref-filter.h
index 14d435e..3d23090 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -107,4 +107,7 @@ struct ref_sorting *ref_default_sorting(void);
/* Function to parse --merged and --no-merged options */
int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset);
+void pretty_print_ref(const char *name, const unsigned char *sha1,
+ const char *format, unsigned kind);
+
#endif /* REF_FILTER_H */
--
2.10.0
^ permalink raw reply related
* [PATCH v3 3/5] tag: add format specifier to gpg_verify_tag
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas P
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas P <luk.puehringer@gmail.com>
Calling functions for gpg_verify_tag() may desire to print relevant
information about the header for further verification. Add an optional
format argument to print any desired information after GPG verification.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
builtin/tag.c | 2 +-
builtin/verify-tag.c | 2 +-
tag.c | 17 +++++++++++------
tag.h | 4 ++--
4 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..14f3b48 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -105,7 +105,7 @@ static int delete_tag(const char *name, const char *ref,
static int verify_tag(const char *name, const char *ref,
const unsigned char *sha1)
{
- return gpg_verify_tag(sha1, name, GPG_VERIFY_VERBOSE);
+ return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
}
static int do_sign(struct strbuf *buffer)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index 99f8148..de10198 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -51,7 +51,7 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
const char *name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
- else if (gpg_verify_tag(sha1, name, flags))
+ else if (verify_and_format_tag(sha1, name, NULL, flags))
had_error = 1;
}
return had_error;
diff --git a/tag.c b/tag.c
index 291073f..d3512c0 100644
--- a/tag.c
+++ b/tag.c
@@ -4,6 +4,7 @@
#include "tree.h"
#include "blob.h"
#include "gpg-interface.h"
+#include "ref-filter.h"
const char *tag_type = "tag";
@@ -33,8 +34,8 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
return ret;
}
-int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
- unsigned flags)
+int verify_and_format_tag(const unsigned char *sha1, const char *name,
+ const char *fmt_pretty, unsigned flags)
{
enum object_type type;
char *buf;
@@ -44,21 +45,25 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
type = sha1_object_info(sha1, NULL);
if (type != OBJ_TAG)
return error("%s: cannot verify a non-tag object of type %s.",
- name_to_report ?
- name_to_report :
+ name ?
+ name :
find_unique_abbrev(sha1, DEFAULT_ABBREV),
typename(type));
buf = read_sha1_file(sha1, &type, &size);
if (!buf)
return error("%s: unable to read file.",
- name_to_report ?
- name_to_report :
+ name ?
+ name :
find_unique_abbrev(sha1, DEFAULT_ABBREV));
ret = run_gpg_verify(buf, size, flags);
free(buf);
+
+ if (fmt_pretty)
+ pretty_print_ref(name, sha1, fmt_pretty, FILTER_REFS_TAGS);
+
return ret;
}
diff --git a/tag.h b/tag.h
index a5721b6..896b9c2 100644
--- a/tag.h
+++ b/tag.h
@@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
extern int parse_tag(struct tag *item);
extern struct object *deref_tag(struct object *, const char *, int);
extern struct object *deref_tag_noverify(struct object *);
-extern int gpg_verify_tag(const unsigned char *sha1,
- const char *name_to_report, unsigned flags);
+extern int verify_and_format_tag(const unsigned char *sha1, const char *name,
+ const char *fmt_pretty, unsigned flags);
#endif /* TAG_H */
--
2.10.0
^ permalink raw reply related
* [PATCH v3 5/5] builtin/tag: add --format argument for tag -v
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Lukas Puehringer
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Lukas Puehringer <luk.puehringer@gmail.com>
Adding --format to git tag -v mutes the default output of the GPG
verification and instead prints the formatted tag object.
This allows callers to cross-check the tagname from refs/tags with
the tagname from the tag object header upon GPG verification.
The callback function for for_each_tag_name() didn't allow callers to
pass custom data to their callback functions. Add a new opaque pointer
to each_tag_name_fn's parameter to allow this.
Signed-off-by: Lukas Puehringer <luk.puehringer@gmail.com>
---
Documentation/git-tag.txt | 2 +-
builtin/tag.c | 34 +++++++++++++++++++++++-----------
2 files changed, 24 insertions(+), 12 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7ecca8e..3bb5e3c 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -15,7 +15,7 @@ SYNOPSIS
'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>]
[--column[=<options>] | --no-column] [--create-reflog] [--sort=<key>]
[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]
-'git tag' -v <tagname>...
+'git tag' -v [--format=<format>] <tagname>...
DESCRIPTION
-----------
diff --git a/builtin/tag.c b/builtin/tag.c
index 14f3b48..7730fd0 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -24,7 +24,7 @@ static const char * const git_tag_usage[] = {
N_("git tag -d <tagname>..."),
N_("git tag -l [-n[<num>]] [--contains <commit>] [--points-at <object>]"
"\n\t\t[--format=<format>] [--[no-]merged [<commit>]] [<pattern>...]"),
- N_("git tag -v <tagname>..."),
+ N_("git tag -v [--format=<format>] <tagname>..."),
NULL
};
@@ -66,15 +66,17 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
}
typedef int (*each_tag_name_fn)(const char *name, const char *ref,
- const unsigned char *sha1);
+ const unsigned char *sha1, void *cb_data);
-static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
+static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
+ void *cb_data)
{
const char **p;
char ref[PATH_MAX];
int had_error = 0;
unsigned char sha1[20];
+
for (p = argv; *p; p++) {
if (snprintf(ref, sizeof(ref), "refs/tags/%s", *p)
>= sizeof(ref)) {
@@ -87,14 +89,14 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
had_error = 1;
continue;
}
- if (fn(*p, ref, sha1))
+ if (fn(*p, ref, sha1, cb_data))
had_error = 1;
}
return had_error;
}
static int delete_tag(const char *name, const char *ref,
- const unsigned char *sha1)
+ const unsigned char *sha1, void *cb_data)
{
if (delete_ref(ref, sha1, 0))
return 1;
@@ -103,9 +105,16 @@ static int delete_tag(const char *name, const char *ref,
}
static int verify_tag(const char *name, const char *ref,
- const unsigned char *sha1)
+ const unsigned char *sha1, void *cb_data)
{
- return verify_and_format_tag(sha1, name, NULL, GPG_VERIFY_VERBOSE);
+ int flags;
+ char *fmt_pretty = cb_data;
+ flags = GPG_VERIFY_VERBOSE;
+
+ if (fmt_pretty)
+ flags = GPG_VERIFY_QUIET;
+
+ return verify_and_format_tag(sha1, name, fmt_pretty, flags);
}
static int do_sign(struct strbuf *buffer)
@@ -334,7 +343,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
struct strbuf err = STRBUF_INIT;
struct ref_filter filter;
static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
- const char *format = NULL;
+ char *format = NULL;
struct option options[] = {
OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'),
{ OPTION_INTEGER, 'n', NULL, &filter.lines, N_("n"),
@@ -424,9 +433,12 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (filter.merge_commit)
die(_("--merged and --no-merged option are only allowed with -l"));
if (cmdmode == 'd')
- return for_each_tag_name(argv, delete_tag);
- if (cmdmode == 'v')
- return for_each_tag_name(argv, verify_tag);
+ return for_each_tag_name(argv, delete_tag, NULL);
+ if (cmdmode == 'v') {
+ if (format)
+ verify_ref_format(format);
+ return for_each_tag_name(argv, verify_tag, format);
+ }
if (msg.given || msgfile) {
if (msg.given && msgfile)
--
2.10.0
^ permalink raw reply related
* [PATCH v3 4/5] builtin/verify-tag: add --format to verify-tag
From: santiago @ 2016-09-30 22:18 UTC (permalink / raw)
To: git; +Cc: gitster, peff, sunshine, walters, Santiago Torres
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
From: Santiago Torres <santiago@nyu.edu>
Callers of verify-tag may want to cross-check the tagname from refs/tags
with the tagname from the tag object header upon GPG verification. This
is to avoid tag refs that point to an incorrect object.
Add a --format parameter to git verify-tag to print the formatted tag
object header in addition to or instead of the --verbose or --raw GPG
verification output.
Signed-off-by: Santiago Torres <santiago@nyu.edu>
---
Documentation/git-verify-tag.txt | 2 +-
builtin/verify-tag.c | 13 +++++++++++--
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-verify-tag.txt b/Documentation/git-verify-tag.txt
index d590edc..0b8075d 100644
--- a/Documentation/git-verify-tag.txt
+++ b/Documentation/git-verify-tag.txt
@@ -8,7 +8,7 @@ git-verify-tag - Check the GPG signature of tags
SYNOPSIS
--------
[verse]
-'git verify-tag' <tag>...
+'git verify-tag' [--format=<format>] <tag>...
DESCRIPTION
-----------
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index de10198..745b6a6 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -12,12 +12,14 @@
#include <signal.h>
#include "parse-options.h"
#include "gpg-interface.h"
+#include "ref-filter.h"
static const char * const verify_tag_usage[] = {
- N_("git verify-tag [-v | --verbose] <tag>..."),
+ N_("git verify-tag [-v | --verbose] [--format=<format>] <tag>..."),
NULL
};
+
static int git_verify_tag_config(const char *var, const char *value, void *cb)
{
int status = git_gpg_config(var, value, cb);
@@ -30,9 +32,11 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
{
int i = 1, verbose = 0, had_error = 0;
unsigned flags = 0;
+ char *fmt_pretty;
const struct option verify_tag_options[] = {
OPT__VERBOSE(&verbose, N_("print tag contents")),
OPT_BIT(0, "raw", &flags, N_("print raw gpg status output"), GPG_VERIFY_RAW),
+ OPT_STRING( 0 , "format", &fmt_pretty, N_("format"), N_("format to use for the output")),
OPT_END()
};
@@ -46,12 +50,17 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
if (verbose)
flags |= GPG_VERIFY_VERBOSE;
+ if (fmt_pretty) {
+ verify_ref_format(fmt_pretty);
+ flags |= GPG_VERIFY_QUIET;
+ }
+
while (i < argc) {
unsigned char sha1[20];
const char *name = argv[i++];
if (get_sha1(name, sha1))
had_error = !!error("tag '%s' not found.", name);
- else if (verify_and_format_tag(sha1, name, NULL, flags))
+ else if (verify_and_format_tag(sha1, name, fmt_pretty, flags))
had_error = 1;
}
return had_error;
--
2.10.0
^ permalink raw reply related
* Re: "Purposes, Concepts,Misfits, and a Redesign of Git" (a research paper)
From: Jakub Narębski @ 2016-09-30 22:24 UTC (permalink / raw)
To: Konstantin Khomoutov, git
Cc: Santiago Perez De Rosso, Daniel Jackson, Greg Wilson,
Jakub Narębski
In-Reply-To: <20160930191413.002049b94b3908b15881b77f@domain007.com>
W dniu 30.09.2016 o 18:14, Konstantin Khomoutov pisze:
> The "It Will Never Work in Theory" blog has just posted a summary of a
> study which tried to identify shortcomings in the design of Git.
>
> In the hope it might be interesting, I post this summary here.
> URL: http://neverworkintheory.org/2016/09/30/rethinking-git.html
I will comment on the article itself, not just on the summary.
| 2.2 Git
[...]
| But tracked files cannot be ignored; to ignore a tracked file
| one has to mark it as “assume unchanged.” This “assume
| unchanged” file will not be recognized by add; to make it
| tracked again this marking has to be removed.
WRONG! Git has tracked files, untracked unignored files, and
untracked ignored files (mostly considered unimportant).
The "assume unchanged" bit is _performance_ optimization. It is not,
and cannot be a 'ignore tracked files' bit - here lies lost work!!!
You can use (imperfectly) "prefer worktree" bit hack instead.
You can say, if 'ignoring change to tracked files' is motivation,
or purpose, it lacks direct concept.
[...]
| As a result, when a user switches branches, files may be
| unexpectedly overwritten.
This is possible _only_ if there are uncommitted changes. If they
are there, and they do not conflict with switching a branch, they
are "floated" to a newly checked out branch.
| Git fails with an error if there are any conflicting changes,
| effectively preventing the user from switching in this case.
| To mitigate this problem, Git provides a way to save versions
| of files to another storage area, called the “stash,” using
| a special command issued prior to the branch switch.
Or you can try to merge uncommitted changes with changes between
two branches: current and switched to.
Or you can forcibly discard your changes.
Or (with modern Git), you can put each branch in a separate
working area (with "git worktree"), though the article may predate
this feature.
[...]
| *Syncing with Other Repositories* Crucial to the understanding
| of how syncing with other repositories work is the notion
| of a “remote branch.” This is a branch (pointer to a commit)
| that (asynchronously) reflects the state of a branch in another
| repository. It is updated whenever there is some network
| communication (e.g., a push or fetch).
It is called "remote-tracking branch", rather than "remote branch".
At least in Git documentation. This branch is in local repository,
not in remote one. The remote-tracking branch for example
`origin/master` follows (tracks) branch `master` in remote
repository `origin`.
| The notion of a “remote branch” must not be confused
| with that of an “upstream branch.” An upstream branch is
| just a convenience for users: after the user assigns it to some
| branch, commands like pull and push default to use that
| branch for fetching and pushing changes if no branch is given
| as input.
Actually "upstream branch" (and related "upstream repository")
are a concept, not only a convenience. They denote a branch
(usually in remote repository) which is intended to ultimately
include changes in given branch. Note that "upstream branch"
can be set separately for any given local branch.
One thing that can enormously help recovering from errors, and
is not covered in the list of concepts is REFLOG.
[...]
| 3. Operational Misfits
[...]
| *Saving Changes* Suppose you are in the middle of a long
| task and want to save your changes, so that they can be later
| retrieved in case of failure. How would you do that?
You would use `git stash` or `git stash --include-untracked`!
In more complicated situations (during long-running operation
like resolving merge conflicts, interactive rebase, or finding
bugs with bisect) with modern Git you can create a new separate
working area with `git worktree`.
This also applies to the "*Switching branches*" problem (which
is more involved, as `git stash` would often not work, and if
it does it is harder to restore state - note however that stash
description includes the branch it was on).
So I would say that *Saving Changes* is solved with stash,
while *Switching Branches* remains a misfit.
| *Detached Head* Suppose you are working on some branch
| and realize that the last few commits you did are wrong, so
| you decide to go back to an old commit to start over again.
| You checkout that old commit and keep working creating
| commits. You might be surprised to discover that these new
| commits you’ve been working on belong to no branch at all.
| To avoid losing them you need to create a new branch or reset
| an existing one to point to the last commit.
It would be hard to be surprised unless one is in habit of
disregarding multi-line warning from Git... ;-)
I think it might be more of an UX problem, namely that the
`git checkout` command does too many things, which include
checking out revision (detaching HEAD, or landing on unnamed
branch, unless we create a new branch at the same time with
the '-b <newbranch>' option), and checking out a branch,
that is switching to other branch.
| *File Rename* Suppose you rename a file and make some
| changes to it. If you changed a significant portion of the file,
| then, as far as Git is concerned, you didn’t rename the file,
| but it is instead as if you deleted the old file and created a new
| one (which means that the file history is now lost). To work
| around this, you have to be diligent about creating a commit
| with the rename only, and only then creating a new commit
| with the modifications. This, however, likely creates a bogus
| commit that doesn’t correspond to a logical group of changes.
First, I think it might be inherent problem. The version control
system may implement *rename tracking* (store information about
renames), or *rename detection*. The latter is what Git does,
and it is what allows to do for example detecting copying and
movement of contents across files and within the same file
for `git blame` (and `git gui blame`) - which is impossible with
only rename tracking.
Nb. that handling file renames and other source reorganization
is important thing that modern version control systems should
be able to handle.
Second, the need for handling renames can be found in two
different operations. One is following history of a single
file across renames, or seeing a rename when looking at changes
in a single revision. The other is merging two lines of
development where one did a rename. The trick of splitting
large rename+change into pure rename and change without rename
can help only the former... the less important one. Merging
in Git is done using [recursive] 3-way merge strategy, which
takes into account only the endpoint state, and not history
between them - so if it was rename then change doesn't matter.
Third, (which is a bit of victim^W user blaming), large changes
are process smell. The cases when change accompanying rename
is so large that it screws heuristic based rename detection
legitimately are (or are supposed to be) rare. And for the
legitimate cases, that is end-of-line changes, there are
solutions to help (-w for diff, -Xrenormalize for merge).
| *File Tracking* Suppose you create a new file and then you
| add the file to start tracking changes to it. You keep working
| on the file making new modifications and then you make
| a vanilla commit. You might be surprised to find out that
| what actually got committed is the old version of the file
| (representing its state the last time the file was staged), and
| not the most recent one.
Unless one uses `git commit -a`, like most people (I think)
in most cases do.
Hopefully `git add -N`, aka. "intent to add", would help here...
when people switch to it to adding new files, and when the
feature gets improved (as it is now) to make it better and
easier to use (e.g. so "git diff" shows new i-t-a files).
| *Untracking File* Suppose there’s a database configuration
| file committed in the repository and you now want to edit
| this file to do some local testing. This new version of the
| file should not be committed. You could always leave out
| the file from the commit every time, but this is laborious and
| error-prone. You might think that you could make it ignored
| by modifying the `.gitignore` file but this doesn’t work for
| committed files.
The name "Untracking File" is misleading. You untrack file
(that is remove it from future commits) with `git rm --cached`.
No problem here. The name is "Ignore changes to tracked files",
or "Stopping tracking changes to file".
If you think of ignored files as unimportant, not precious,
then mismatch between understanding and what Git does would
lessen.
| The way to ignore this file is to mark it as
| “assume unchanged,” but this marking will be cleared when
| you switch to another branch.
s/“assume unchanged,”/“skip worktree,” (see earlier comment).
Right. Also, currently you need to use low-level commands
(`git update-index`) to mark file in this way.
[...]
| 4. Purposes for Version Control
[...]
| *Collaboration* To this point, all the purposes might apply in
| the context of a single user. Collaboration needs arise when
| multiple users work together on a single code base.
| /* ..........................................................
| /* _Purpose 4. Synchronize changes of collaborators_
One very important purpose that must be fulfilled before even
trying to synchronize changes is *isolation of changes*. Each
developer needs its own working area, so his or her changes do
not interfere with work of other developers.
Though "Disconnected operations" a bit overlaps (and is partial
superset) of this purpose.
See also intro to "Version Control by Example", by Eric Sink
http://ericsink.com/vcbe/html/intro.html
[...]
| 5.1 Stashing: An Example
|
| We consider the motivating purpose of stashing not to be
| a subpurpose of any of the high-level purposes for version
| control (§4). This section elaborates on our rationale for this.
|
| Take (what seem to be) the motivating use cases for stashing
| [9, Chapter 7.3]: (1) to pull into a dirty working directory
| and (2) to deal with an interruption in your workflow
Actually (1) is not a separate use case, but a subset
(specialization) of (2) - and interruption in your workflow,
where the interruption is pull.
Note that to synchronize with remote repository one should use
fetch, not pull. The latter is more involved operation, and
should be considered interruption. Also, rarer if using
feature branches workflow, and not working on long-lived
stabilization branches directly.
[...]
| The problem
| is the lack of connection between this purpose and the highlevel
| purposes for version control, which suggests that the
| introduction of stashing might be to patch flaws in the design
| of Git and not to satisfy a requirement of version control.
Or the problem might be that you are missing some (maybe minor)
requirement of version control system. Just saying...
| 6. Analysis
[...]
| *Divided Ignored and Assumed Unchanged*
This ignores the fact that (unstated) assumption is that ignored
files are considered not important (or at least less important;
Git cares less about changes in those files, and may in some
cases make you loose changes to them).
Ignore file != ignore changes, though it may look like it is.
[...]
| 7. Gitless
|
| 7.1 Overview
|
| Gitless has no staging area, and the only file classifications
| are “tracked,” “untracked,” “ignored,” and “in conflict.”
Without staging area, I wonder how you would be able to handle
well different types of integration conflicts, which are not
limited to CONFLICT(content).
You also loose the ability to select subset of *changes* to
be committed, not only files. This is often very useful, see
http://tomayko.com/writings/the-thing-about-git
http://2ndscale.com/rtomayko/2008/the-thing-about-git
[...]
| A branch in Gitless is a completely independent line of
| development: each branch includes the working version of
| files [...]
If I understand your model correctly, you would get rid of
one problem / mismatch, but get into other. What would you
do if you started work on some branch, and then realized
that you should have been working on a new topic branch?
Or you realized that you are on wrong branch, and want to
move changes?
Ah, I haven't realized that it is described later (well,
at least the first case):
: In regard to branching, to address situations in which the
: user wants changes made in the current branch to be moved
: onto the destination branch (e.g., the user realizes that she
: has been working in the wrong branch), the Gitless branch
: command has a `move-over` flag
[...]
| Also, there
| is no possible way of getting in a “detached head” state; at
| any time, the user is always working on some branch (the
| “current” branch). Head is a per-branch reference to the last
| commit of the branch.
How do you solve the problem of checking out the state of
the tag, that is the state of repository at given revision?
Also during some long lived multi-step operations, like bisect
or interactive rebase, you are not really on any branch,
| 7.2.1 Discussion
[...]
| There could be other use cases for the
| staging area that Gitless doesn’t handle well but we expect
| these to be fairly infrequent.
Like handling merge conflict...??? Infrequent doesn't mean
unimportant.
That's all my comments for now.
This is a very interesting research. Those problems (misfits)
are legitimate concern. Even if it would not result in changes
to how Git works, it should improve how we are talking about
version control, and how we design them (that includes GUIs
that work above version control systems).
Best regards,
--
Jakub Narębski
^ permalink raw reply
* Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: Jeff King @ 2016-09-30 22:44 UTC (permalink / raw)
To: David Turner; +Cc: git@vger.kernel.org
In-Reply-To: <b7136f0690244aee94ff2db726a7edce@exmbdft7.ad.twosigma.com>
On Fri, Sep 30, 2016 at 09:32:04PM +0000, David Turner wrote:
> > +static void env_append(struct argv_array *env, const char *key, const
> > +char *val) {
> > + const char *old = getenv(key);
> > +
> > + if (!old)
> > + argv_array_pushf(env, "%s=%s", key, val);
> > + else
> > + argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP,
> > val);
> >+}
>
> I would like a comment explaining this function.
I'll squash in:
diff --git a/tmp-objdir.c b/tmp-objdir.c
index c92e6cc..a98c246 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -70,6 +70,13 @@ static void remove_tmp_objdir_on_signal(int signo)
raise(signo);
}
+/*
+ * These env_* functions are for setting up the child environment; the
+ * "replace" variant overrides the value of any existing variable with that
+ * "key". The "append" variant puts our new value at the end of a list,
+ * separated by PATH_SEP (which is what separate values in
+ * GIT_ALTERNATE_OBJECT_DIRECTORIES).
+ */
static void env_append(struct argv_array *env, const char *key, const char *val)
{
const char *old = getenv(key);
> > + * Finalize a temporary object directory by migrating its objects into
> > +the main
> > + * object database.
> > + */
>
> This should mention that it frees its argument.
And:
diff --git a/tmp-objdir.h b/tmp-objdir.h
index aa47aa9..b1e45b4 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -35,7 +35,8 @@ const char **tmp_objdir_env(const struct tmp_objdir *);
/*
* Finalize a temporary object directory by migrating its objects into the main
- * object database.
+ * object database, removing the temporary directory, and freeing any
+ * associated resources.
*/
int tmp_objdir_migrate(struct tmp_objdir *);
-Peff
^ permalink raw reply related
* Re: [PATCH v3 2/5] ref-filter: add function to print single ref_array_item
From: Jakub Narębski @ 2016-09-30 23:12 UTC (permalink / raw)
To: Santiago Torres, git
Cc: Junio C Hamano, Jeff King, sunshine, walters, Lukas Puehringer
In-Reply-To: <20160930221806.3398-3-santiago@nyu.edu>
W dniu 01.10.2016 o 00:18, santiago@nyu.edu pisze:
> From: Lukas Puehringer <luk.puehringer@gmail.com>
>
> ref-filter functions are useful for printing git object information
> using a format specifier. However, some other modules may not want to use
> this functionality on a ref-array but only print a single item.
>
> Expose a format_ref function to create, pretty print and free individual
> ref-items.
It's now pretty_print_ref, not format_ref (old version in commit message).
[...]
> +void pretty_print_ref(const char *name, const unsigned char *sha1,
> + const char *format, unsigned kind)
^ permalink raw reply
* RE: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories
From: David Turner @ 2016-09-30 23:07 UTC (permalink / raw)
To: 'Jeff King'; +Cc: git@vger.kernel.org
In-Reply-To: <20160930224450.zewgfqelljiwowh6@sigill.intra.peff.net>
Thanks. The rest all look good too.
> -----Original Message-----
> From: Jeff King [mailto:peff@peff.net]
> Sent: Friday, September 30, 2016 6:45 PM
> To: David Turner
> Cc: git@vger.kernel.org
> Subject: Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object
> directories
>
> On Fri, Sep 30, 2016 at 09:32:04PM +0000, David Turner wrote:
>
> > > +static void env_append(struct argv_array *env, const char *key,
> > > +const char *val) {
> > > + const char *old = getenv(key);
> > > +
> > > + if (!old)
> > > + argv_array_pushf(env, "%s=%s", key, val);
> > > + else
> > > + argv_array_pushf(env, "%s=%s%c%s", key, old, PATH_SEP,
> > > val);
> > >+}
> >
> > I would like a comment explaining this function.
>
> I'll squash in:
>
> diff --git a/tmp-objdir.c b/tmp-objdir.c index c92e6cc..a98c246 100644
> --- a/tmp-objdir.c
> +++ b/tmp-objdir.c
> @@ -70,6 +70,13 @@ static void remove_tmp_objdir_on_signal(int signo)
> raise(signo);
> }
>
> +/*
> + * These env_* functions are for setting up the child environment; the
> + * "replace" variant overrides the value of any existing variable with
> +that
> + * "key". The "append" variant puts our new value at the end of a list,
> + * separated by PATH_SEP (which is what separate values in
> + * GIT_ALTERNATE_OBJECT_DIRECTORIES).
> + */
> static void env_append(struct argv_array *env, const char *key, const char
> *val) {
> const char *old = getenv(key);
>
> > > + * Finalize a temporary object directory by migrating its objects
> > > +into the main
> > > + * object database.
> > > + */
> >
> > This should mention that it frees its argument.
>
> And:
>
> diff --git a/tmp-objdir.h b/tmp-objdir.h index aa47aa9..b1e45b4 100644
> --- a/tmp-objdir.h
> +++ b/tmp-objdir.h
> @@ -35,7 +35,8 @@ const char **tmp_objdir_env(const struct tmp_objdir
> *);
>
> /*
> * Finalize a temporary object directory by migrating its objects into the main
> - * object database.
> + * object database, removing the temporary directory, and freeing any
> + * associated resources.
> */
> int tmp_objdir_migrate(struct tmp_objdir *);
>
>
> -Peff
^ permalink raw reply
* Re: [PATCH v3 0/5] Add --format to tag verification
From: Junio C Hamano @ 2016-09-30 23:28 UTC (permalink / raw)
To: santiago; +Cc: git, peff, sunshine, walters
In-Reply-To: <20160930221806.3398-1-santiago@nyu.edu>
What is in the patch series looked more or less good to me. Lukas's
sirname was still P in [3/5], a patch in [4/5] added an unnecssary
blank line before git_verify_tag_config() and also a local variable
declaration for "char *fmt_pretty" was indented funnily, but none of
these were something I couldn't fix up locally.
I however notice that there is no new tests to protect these two new
features from future breakages. Perhaps you want to add some in
[6/5]?
Thanks.
^ permalink raw reply
* [PATCH 0/3] auto-sizing default abbreviation length
From: Junio C Hamano @ 2016-10-01 0:19 UTC (permalink / raw)
To: git
So here is what I queued in 'pu' tonight after back and forth with
Linus and Peff. The third step from Linus needs to be signed off
and also a meaningful log message for it needs to be written, and
also may need to be updated to include what Linus did in [*1*], but
otherwise I think these are in good enough shape for people to start
playing with them.
They apply on top of Peff's jk/ambiguous-short-object-names topic
that ends at 5b33cb1fd7 ("get_short_sha1: make default
disambiguation configurable", 2016-09-27).
*1*
http://public-inbox.org/git/<CA+55aFxyF=xX84AXr8MG14MRHwdrQw00PBM20UfqBdidaeqdMg@mail.gmail.com>
Junio C Hamano (2):
abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing
abbrev: prepare for new world order
Linus Torvalds (1):
abbrev: auto size the default abbreviation
builtin/fetch.c | 3 +++
builtin/rev-parse.c | 5 +++--
cache.h | 4 ++++
diff.c | 25 +++++++++++++++++++++++--
environment.c | 2 +-
sha1_name.c | 28 +++++++++++++++++++++++++++-
transport.h | 3 +--
7 files changed, 62 insertions(+), 8 deletions(-)
--
2.10.0-622-g05f606bbb0
^ permalink raw reply
* [PATCH 1/3] abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing
From: Junio C Hamano @ 2016-10-01 0:19 UTC (permalink / raw)
To: git
In-Reply-To: <20161001001937.10884-1-gitster@pobox.com>
We'll be introducing a new way to decide the default abbreviation
length by initialising DEFAULT_ABBREV to -1 to signal the first call
to "find unique abbreviation" codepath to compute a reasonable value
based on the number of objects we have to avoid collisions.
We have long relied on DEFAULT_ABBREV being a positive concrete
value that is used as the abbreviation length when no extra
configuration or command line option has overridden it. Some
codepaths wants to use such a positive concrete default value
even before making their first request to actually trigger the
computation for the auto sized default.
Introduce FALLBACK_DEFAULT_ABBREV and use it to the code that
attempts to align the report from "git fetch". For now, this
macro is also used to initialize the default_abbrev variable,
but the auto-sizing code will use -1 and then use the value of
FALLBACK_DEFAULT_ABBREV as the starting point of auto-sizing.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/fetch.c | 3 +++
cache.h | 3 +++
environment.c | 2 +-
transport.h | 3 +--
4 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 164623bb6f..a9f12cc5cf 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -17,6 +17,9 @@
#include "argv-array.h"
#include "utf8.h"
+#define TRANSPORT_SUMMARY(x) \
+ (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
+
static const char * const builtin_fetch_usage[] = {
N_("git fetch [<options>] [<repository> [<refspec>...]]"),
N_("git fetch [<options>] <group>"),
diff --git a/cache.h b/cache.h
index f346c01708..5a651b8435 100644
--- a/cache.h
+++ b/cache.h
@@ -1183,6 +1183,9 @@ static inline int hex2chr(const char *s)
#define MINIMUM_ABBREV minimum_abbrev
#define DEFAULT_ABBREV default_abbrev
+/* used when the code does not know or care what the default abbrev is */
+#define FALLBACK_DEFAULT_ABBREV 7
+
struct object_context {
unsigned char tree[20];
char path[PATH_MAX];
diff --git a/environment.c b/environment.c
index cd5aa57179..44fb107b8a 100644
--- a/environment.c
+++ b/environment.c
@@ -16,7 +16,7 @@ int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = 7;
+int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;
int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
diff --git a/transport.h b/transport.h
index 6fe3485325..e783377e40 100644
--- a/transport.h
+++ b/transport.h
@@ -142,8 +142,7 @@ struct transport {
#define TRANSPORT_PUSH_ATOMIC 8192
#define TRANSPORT_PUSH_OPTIONS 16384
-#define TRANSPORT_SUMMARY_WIDTH (2 * DEFAULT_ABBREV + 3)
-#define TRANSPORT_SUMMARY(x) (int)(TRANSPORT_SUMMARY_WIDTH + strlen(x) - gettext_width(x)), (x)
+#define TRANSPORT_SUMMARY_WIDTH (2 * FALLBACK_DEFAULT_ABBREV + 3)
/* Returns a transport suitable for the url */
struct transport *transport_get(struct remote *, const char *);
--
2.10.0-622-g05f606bbb0
^ permalink raw reply related
* [PATCH 2/3] abbrev: prepare for new world order
From: Junio C Hamano @ 2016-10-01 0:19 UTC (permalink / raw)
To: git
In-Reply-To: <20161001001937.10884-1-gitster@pobox.com>
The code that sets custom abbreviation length, in response to
command line argument, often does something like this:
if (skip_prefix(arg, "--abbrev=", &arg))
abbrev = atoi(arg);
else if (!strcmp("--abbrev", &arg))
abbrev = DEFAULT_ABBREV;
/* make the value sane */
if (abbrev < 0 || 40 < abbrev)
abbrev = ... some sane value ...
However, it is pointless to sanity-check and tweak the value
obtained from DEFAULT_ABBREV. We are going to allow it to be
initially set to -1 to signal that the default abbreviation length
must be auto sized upon the first request to abbreviate, based on
the number of objects in the repository, and when that happens,
rejecting or tweaking a negative value to a "saner" one will
negatively interfere with the auto sizing. The codepaths for
git rev-parse --short <object>
git diff --raw --abbrev
do exactly that; allow them to pass possibly negative abbrevs
intact, that will come from DEFAULT_ABBREV in the future.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/rev-parse.c | 5 +++--
diff.c | 2 +-
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 76cf05e2ad..17cbfabdde 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -643,8 +643,9 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
filter &= ~(DO_FLAGS|DO_NOREV);
verify = 1;
abbrev = DEFAULT_ABBREV;
- if (arg[7] == '=')
- abbrev = strtoul(arg + 8, NULL, 10);
+ if (!arg[7])
+ continue;
+ abbrev = strtoul(arg + 8, NULL, 10);
if (abbrev < MINIMUM_ABBREV)
abbrev = MINIMUM_ABBREV;
else if (40 <= abbrev)
diff --git a/diff.c b/diff.c
index c6da383c56..cefc13eb8e 100644
--- a/diff.c
+++ b/diff.c
@@ -3399,7 +3399,7 @@ void diff_setup_done(struct diff_options *options)
*/
read_cache();
}
- if (options->abbrev <= 0 || 40 < options->abbrev)
+ if (40 < options->abbrev)
options->abbrev = 40; /* full */
/*
--
2.10.0-622-g05f606bbb0
^ permalink raw reply related
* [PATCH 3/3] abbrev: auto size the default abbreviation
From: Junio C Hamano @ 2016-10-01 0:19 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds
In-Reply-To: <20161001001937.10884-1-gitster@pobox.com>
From: Linus Torvalds <torvalds@linux-foundation.org>
In fairly early days we somehow decided to abbreviate object names
down to 7-hexdigits, but as projects grow, it is becoming more and
more likely to see such a short object names made in earlier days
and recorded in the log messages no longer unique.
Currently the Linux kernel project needs 11 to 12 hexdigits, while
Git itself needs 10 hexdigits to uniquely identify the objects they
have, while many smaller projects may still be fine with the
original 7-hexdigit default. One-size does not fit all projects.
Introduce a mechanism, where we estimate the number of objects in
the repository upon the first request to abbreviate an object name
with the default setting and come up with a sane default for the
repository. Based on the expectation that we would see collision in
a repository with 2^(2N) objects when using object names shortened
to first N bits, use sufficient number of hexdigits to cover the
number of objects in the repository. Each hexdigit (4-bits) we add
to the shortened name allows us to have four times (2-bits) as many
objects in the repository.
---
cache.h | 1 +
environment.c | 2 +-
sha1_name.c | 28 +++++++++++++++++++++++++++-
3 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/cache.h b/cache.h
index 5a651b8435..0e2a0595e5 100644
--- a/cache.h
+++ b/cache.h
@@ -1204,6 +1204,7 @@ struct object_context {
#define GET_SHA1_TREEISH 020
#define GET_SHA1_BLOB 040
#define GET_SHA1_FOLLOW_SYMLINKS 0100
+#define GET_SHA1_AUTOMATIC 0200
#define GET_SHA1_ONLY_TO_DIE 04000
#define GET_SHA1_DISAMBIGUATORS \
diff --git a/environment.c b/environment.c
index 44fb107b8a..6f9d290563 100644
--- a/environment.c
+++ b/environment.c
@@ -16,7 +16,7 @@ int trust_executable_bit = 1;
int trust_ctime = 1;
int check_stat = 1;
int has_symlinks = 1;
-int minimum_abbrev = 4, default_abbrev = FALLBACK_DEFAULT_ABBREV;
+int minimum_abbrev = 4, default_abbrev = -1;
int ignore_case;
int assume_unchanged;
int prefer_symlink_refs;
diff --git a/sha1_name.c b/sha1_name.c
index 3b647fd7cf..beb7ab588b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -15,6 +15,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
struct disambiguate_state {
int len; /* length of prefix in hex chars */
+ unsigned int nrobjects;
char hex_pfx[GIT_SHA1_HEXSZ + 1];
unsigned char bin_pfx[GIT_SHA1_RAWSZ];
@@ -118,6 +119,14 @@ static void find_short_object_filename(struct disambiguate_state *ds)
if (strlen(de->d_name) != 38)
continue;
+
+ /*
+ * We only look at the one subdirectory, and we assume
+ * each subdirectory is roughly similar, so each
+ * object we find probably has 255 other objects in
+ * the other fan-out directories.
+ */
+ ds->nrobjects += 256;
if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
continue;
memcpy(hex + 2, de->d_name, 38);
@@ -151,6 +160,7 @@ static void unique_in_pack(struct packed_git *p,
open_pack_index(p);
num = p->num_objects;
+ ds->nrobjects += num;
last = num;
while (first < last) {
uint32_t mid = (first + last) / 2;
@@ -380,6 +390,9 @@ static int show_ambiguous_object(const unsigned char *sha1, void *data)
return 0;
}
+/* start from our historical default before the automatic abbreviation */
+static int default_automatic_abbrev = FALLBACK_DEFAULT_ABBREV;
+
static int get_short_sha1(const char *name, int len, unsigned char *sha1,
unsigned flags)
{
@@ -426,6 +439,14 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
for_each_abbrev(ds.hex_pfx, show_ambiguous_object, &ds);
}
+ if (len < 16 && !status && (flags & GET_SHA1_AUTOMATIC)) {
+ unsigned int expect_collision = 1 << (len * 2);
+ if (ds.nrobjects > expect_collision) {
+ default_automatic_abbrev = len+1;
+ return SHORT_NAME_AMBIGUOUS;
+ }
+ }
+
return status;
}
@@ -458,14 +479,19 @@ int for_each_abbrev(const char *prefix, each_abbrev_fn fn, void *cb_data)
int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
{
int status, exists;
+ int flags = GET_SHA1_QUIETLY;
+ if (len < 0) {
+ flags |= GET_SHA1_AUTOMATIC;
+ len = default_automatic_abbrev;
+ }
sha1_to_hex_r(hex, sha1);
if (len == 40 || !len)
return 40;
exists = has_sha1_file(sha1);
while (len < 40) {
unsigned char sha1_ret[20];
- status = get_short_sha1(hex, len, sha1_ret, GET_SHA1_QUIETLY);
+ status = get_short_sha1(hex, len, sha1_ret, flags);
if (exists
? !status
: status == SHORT_NAME_NOT_FOUND) {
--
2.10.0-622-g05f606bbb0
^ permalink raw reply related
* Re: [PATCH v3 2/5] ref-filter: add function to print single ref_array_item
From: Junio C Hamano @ 2016-10-01 1:33 UTC (permalink / raw)
To: Jakub Narębski
Cc: Santiago Torres, git, Jeff King, sunshine, walters,
Lukas Puehringer
In-Reply-To: <0e3cc036-787b-d93b-dc87-47a0826ec919@gmail.com>
Jakub Narębski <jnareb@gmail.com> writes:
>> Expose a format_ref function to create, pretty print and free individual
>> ref-items.
>
> It's now pretty_print_ref, not format_ref (old version in commit message).
Good eyes. Will tweak locally.
Thanks.
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2016, #08; Tue, 27)
From: Stefan Beller @ 2016-10-01 1:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git@vger.kernel.org
In-Reply-To: <xmqq8tudkjvn.fsf@gitster.mtv.corp.google.com>
>
>
> * jc/attr (2016-05-25) 18 commits
> - attr: support quoting pathname patterns in C style
> - attr: expose validity check for attribute names
> - attr: add counted string version of git_attr()
> - attr: add counted string version of git_check_attr()
> - attr: retire git_check_attrs() API
> - attr: convert git_check_attrs() callers to use the new API
> - attr: convert git_all_attrs() to use "struct git_attr_check"
> - attr: (re)introduce git_check_attr() and struct git_attr_check
> - attr: rename function and struct related to checking attributes
> - attr.c: plug small leak in parse_attr_line()
> - attr.c: tighten constness around "git_attr" structure
> - attr.c: simplify macroexpand_one()
> - attr.c: mark where #if DEBUG ends more clearly
> - attr.c: complete a sentence in a comment
> - attr.c: explain the lack of attr-name syntax check in parse_attr()
> - attr.c: update a stale comment on "struct match_attr"
> - attr.c: use strchrnul() to scan for one line
> - commit.c: use strchrnul() to scan for one line
> (this branch is used by jc/attr-more, sb/pathspec-label and sb/submodule-default-paths.)
>
> The attributes API has been updated so that it can later be
> optimized using the knowledge of which attributes are queried.
>
> I wanted to polish this topic further to make the attribute
> subsystem thread-ready, but because other topics depend on this
> topic and they do not (yet) need it to be thread-ready.
>
> As the authors of topics that depend on this seem not in a hurry,
> let's discard this and dependent topics and restart them some other
> day.
>
> Will discard.
So I just realized this is a big hint for me to pick up that topic; I assumed
you'd want to tackle the attr subsystem eventually, so all I was doing, was
waiting for your motivation to look at attr stuff to come back.
So what is the actual lacking stuff here?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts
From: Jeff King @ 2016-10-01 9:12 UTC (permalink / raw)
To: git; +Cc: David Turner
In-Reply-To: <20160930193630.vwq6n5hjbthrz6fj@sigill.intra.peff.net>
On Fri, Sep 30, 2016 at 03:36:30PM -0400, Jeff King wrote:
> @@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)
> argv_array_push(&child.args, alt_shallow_file);
> }
>
> + tmp_objdir = tmp_objdir_create();
> + if (!tmp_objdir)
> + return "unable to create temporary object directory";
> + child.env = tmp_objdir_env(tmp_objdir);
One thing to note here: this new code kicks in all the time. My
reasoning was that there's basically no time you _wouldn't_ want it to,
and certainly that was the case for us when I wrote it. But I tried to
think of user-visible changes. Here's what I came up with:
- we currently leave the tmp_pack_* for a failed push sitting around
(e.g., if the client hangs up halfway through, or index-pack rejects
the pack for some reason). But with this series, it would always be
cleaned up. That's a very good thing if you're running a git hosting
site. It might make things harder if you're debugging.
I don't think it's a good reason not to enable this by default, but
it _could_ be a reason to have a config switch to turn it off
temporarily (or just leave the "incoming-*" directory in place).
- the environment that pre-receive pack runs in has access to objects
that the rest of the repository doesn't. So if you were to do
something silly in your pre-receive like:
# reject the push, but log a copy of the objects
git update-ref refs/rejected/$(date +%s) $new_sha1
exit 1
Then your ref-update would succeed (you have $new_sha1), but the
objects would be deleted immediately afterwards. I find this a
somewhat questionable pattern, and I have no idea if anybody else
has thought of it. But it _does_ work today, and not with this
series.
I don't think it would be too hard to put a config conditional around
this tmp_objdir_create(). And then all of the tmp_objdir_env() calls
would just return NULL, and effectively become noops.
-Peff
^ permalink raw reply
* Re: [PATCH] diff_unique_abbrev(): document its assumtion and limitation
From: Jeff King @ 2016-10-01 9:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linus Torvalds
In-Reply-To: <xmqqd1jl9ovc.fsf@gitster.mtv.corp.google.com>
On Fri, Sep 30, 2016 at 12:19:51PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > ... Now that function _would_
> > want to be updated as a result of the other conversation (it would need
> > to do something sensible with "-1", like turning it into "7", or
> > whatever else is deemed reasonable outside of a repository).
> >
> > Anyway. I just wonder if you want to give it a better name while you are
> > at it.
>
> I'd say the patch to introduce the new function that makes the old
> name potentially confusing is a good one to do the rename. Until
> then I do not think there is no need to rename the existing one ;-)
I guess my point was that the poor name may have contributed to the need
to explain it. But I'm happy to deal with it in my series (I also
updated it to use "struct oid"; I'll probably rebase mine on top of your
comment to save you dealing with the nasty merge).
> Related tangent about "like turning it into", I am thinking adding
> something like this as a preparatory step to Linus's auto-sizing
> serires. That way, we do not have to spell "7"
> [...]
> -- >8 --
> Subject: abbrev: add FALLBACK_DEFAULT_ABBREV to prepare for auto sizing
Yep, looks like a good idea.
-Peff
^ permalink raw reply
* Re: [PATCH 1/5] pretty: allow formatting DATE_SHORT
From: Jeff King @ 2016-10-01 9:18 UTC (permalink / raw)
To: SZEDER Gábor
Cc: Jacob Keller, Kyle J. McKay, Git mailing list, Junio C Hamano
In-Reply-To: <20160930105639.15589-1-szeder@ira.uka.de>
On Fri, Sep 30, 2016 at 12:56:39PM +0200, SZEDER Gábor wrote:
> > Nice. I use date=short in some of my aliases and switching to this is
> > nicer. I assume this turns into "%(as)"?
> >
> > What about documenting this in pretty-formats.txt?
>
> Here you go :)
>
> http://public-inbox.org/git/1444235305-8718-1-git-send-email-szeder@ira.uka.de/
The review on that thread is basically "do we want to add every
abbreviation under the sun, or do we want to come up with a better
placeholder scheme?". And that has come up several other times (e.g.,
Ted's recent %gt placeholder).
I _do_ think that's the right way forward, but as nobody has been
pushing it that way, and these things come up from time to time, maybe
we should just accept it and add the ones people want in the interim.
It's not like it makes the situation significantly worse, or harder to
migrate to %(authordate:short).
-Peff
^ permalink raw reply
* Re: Impossible to change working directory
From: Torsten Bögershausen @ 2016-10-01 15:34 UTC (permalink / raw)
To: Sebastian Feldmann, git
In-Reply-To: <A99D7D06-8214-4C09-BCB2-E4FCE75FF317@sebastian-feldmann.info>
On 29.09.16 21:30, Sebastian Feldmann wrote:
> Hi there,
>
> I have a problem executing a pre-commit hook.
> The hook script has to change the working directory to work and if I use plain
>
> git commit
>
> it works as expected, the script executes without errors, but if I use
>
> git commit —only file.x file.y
>
> the script fails because changing the current working directory fails.
> If I echo the current working directory it always echoes the root repository path
>
> Is this expected behavior?
> Thanks for your feedback.
Is there any chance to send us the content of the script ?
(Or a demo example, which doesn't work)
^ permalink raw reply
* Re: [PATCH v8 11/11] convert: add filter.<driver>.process option
From: Lars Schneider @ 2016-10-01 15:34 UTC (permalink / raw)
To: Jakub Narębski
Cc: git, Jeff King, Junio C Hamano, Stefan Beller,
Martin-Louis Bright, Torsten Bögershausen, Ramsay Jones
In-Reply-To: <e8815048-212e-64c6-8811-9185f595aaf6@gmail.com>
> On 29 Sep 2016, at 01:14, Jakub Narębski <jnareb@gmail.com> wrote:
>
> Part third (and last) of the review of v8 11/11.
>
> W dniu 20.09.2016 o 21:02, larsxschneider@gmail.com napisał:
>
>
>> @@ -31,7 +31,10 @@ test_expect_success setup '
>> cat test >test.i &&
>> git add test test.t test.i &&
>> rm -f test test.t test.i &&
>> - git checkout -- test test.t test.i
>> + git checkout -- test test.t test.i &&
>> +
>> + echo "content-test2" >test2.o &&
>> + echo "content-test3 - subdir" >"test3 - subdir.o"
>
> I see that you prepare here a few uncommitted files, but both
> their names and their contents leave much to be desired - you
> don't know from the name and contents what they are for.
>
> And the '"subdir"' file which is not in subdirectory is
> especially egregious.
These are 3 files with somewhat random test content. I renamed
"subdir" to "spaces".
>> +check_filter () {
>> + rm -f rot13-filter.log actual.log &&
>> + "$@" 2> git_stderr.log &&
>> + test_must_be_empty git_stderr.log &&
>> + cat >expected.log &&
>
> This is too clever by half. Having a function that both tests
> the behavior and prepares 'expected' file is too much.
>
> In my opinion preparation of 'expected.log' file should be moved
> to another function or functions.
>
> Also, if we are running sort on output, I think we should also
> run sort on 'expected.log', so that what we write doesn't need to
> be created sorted (so we don't have to sort expected lines by hand).
> Or maybe we should run the same transformation on rot13-filter.log
> and on the contents of expected.log.
Agreed. Very good suggestion!
>> +check_filter_ignore_clean () {
>> + rm -f rot13-filter.log actual.log &&
>> + "$@" &&
>
> Why we don't check for stderr here?
Because this function is used by "git checkout" which writes all
kinds of stuff to stderr. I added "--quiet --no-progress" to
disable this behavior.
>> +check_rot13 () {
>> + test_cmp "$1" "$2" &&
>> + ./../rot13.sh <"$1" >expected &&
>
> Why there is .. in this invocation?
Because this script is located in the root of the current test directory.
>> + git cat-file blob :"$2" >actual &&
>> + test_cmp expected actual
>> +}
>> +
>> +test_expect_success PERL 'required process filter should filter data' '
>> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
>> + test_config_global filter.protocol.required true &&
>> + rm -rf repo &&
>> + mkdir repo &&
>> + (
>> + cd repo &&
>> + git init &&
>
> Don't you think that creating a fresh test repository for each
> separate test is a bit too much? I guess that you want for
> each and every test to be completely independent, but this setup
> and teardown is a bit excessive.
>
> Other tests in the same file (should we reuse the test, or use
> new test file) do not use this method.
I see your point. However, I am always annoyed if Git tests are
entangled because it makes working with them way way harder.
This test test runs in 4.5s on a slow Travis CI machine. I think
that is OK considering that we have tests running 3.5min (t3404).
>> + echo "*.r filter=protocol" >.gitattributes &&
>> + git add . &&
>> + git commit . -m "test commit" &&
>> + git branch empty &&
>
> Err... I think it would be better to name it 'empty-branch'
> (or 'almost-empty-branch', as it does include .gitattributes file).
> See my mistake below (marked <del>...</del>).
"empty-branch". OK
>> +
>> + cp ../test.o test.r &&
>> + cp ../test2.o test2.r &&
>
> What does this test2.o / test2.r file tests, that test.o / test.r
> doesn't? The name doesn't tell us.
This just tests multiple files with different content.
> Why it is test.r, but test2.r? Why it isn't test1.r?
test.r already existed (created in setup test).
>> + mkdir testsubdir &&
>> + cp "../test3 - subdir.o" "testsubdir/test3 - subdir.r" &&
>
> Why it needs to have different contents?
To check that the filer does the right thing with multiple files
and contents.
>> + >test4-empty.r &&
>
> You test ordinary file, file in subdirectory, file with filename
> containing spaces, and an empty file.
>
> Other tests of single file `clean`/`smudge` filters use filename
> that requires mangling; maybe we should use similar file?
>
> special="name with '\''sq'\'' and \$x" &&
> echo some test text >"$special" &&
OK.
> In case of `process` filter, a special filename could look like
> this:
>
> process_special="name=with equals and\nembedded newlines\n" &&
> echo some test text >"$process_special" &&
I think this test would create trouble on Windows. I'll stick to
the special characters used in the single shot filter.
>> + <<-\EOF &&
>> + 1 IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
>> + 1 IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
>> + 1 IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
>> + 1 IN: clean testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
>> + 1 START
>> + 1 STOP
>> + 1 wrote filter header
>> + EOF
>
> First, this indentation level confirms that the check_filter
> function is too clever by half, and that preparing expected.log
> file should be a separate step.
Agreed.
> Second, if we run "sort" on contents to be in expected.log, we
> can write it in more natural, and less fragile way:
Agreed.
> Third, why the filter even writes output size? It is no longer
> part of `process` filter driver protocol, and it makes test more
> fragile.
I would prefer to leave that in. I think it is good for the test to
check that we are transmitting the amount of content that what we
think we transmit.
> If we are to keep sizes, then to make test less fragile with
> respect to changes in contents of tested files, we should use
> variables containing file size:
>
> test_r_size=$(wc -c test.r)
> ...
> sort >expected.log <<-EOF &&
> ...
> 1 IN: clean test.r $test_r_size [OK] -- OUT: $test_r_size . [OK]
Agreed.
>>
>> + rm -f test?.r "testsubdir/test3 - subdir.r" &&
>
> Why 'test?.r' when we are removing only 'test2.r'; why not be explicit?
True!
>> + <<-\EOF &&
>> + START
>> + wrote filter header
>> + STOP
>> + EOF
>
> Why is even filter process invoked? If this is not expected, perhaps
> simply ignore what checking out almost empty branch (one without any
> files marked for filtering) does.
>
> Shouldn't we test_expect_failure no-call?
Because a clean operation could happen. I added a clean operation to
the expected log in order to make this visible (expected log is stripped
of clean operations in the same way as the actual log per your suggestion
above).
>> +
>> + check_filter_ignore_clean \
>> + git checkout master \
>
> Does this checks different code path than 'git checkout .'? For
> example, does this test increase code coverage (e.g. as measured
> by gcov)? If not, then this test could be safely dropped.
We checked out the "empty-branch" before. That's why we check here
that the smudge filter runs for all files (smudge filter did not run
for all files with `git checkout .`).
>> + <<-\EOF &&
>> + START
>> + wrote filter header
>> + IN: smudge test.r 57 [OK] -- OUT: 57 . [OK]
>> + IN: smudge test2.r 14 [OK] -- OUT: 14 . [OK]
>> + IN: smudge test4-empty.r 0 [OK] -- OUT: 0 [OK]
>> + IN: smudge testsubdir/test3 - subdir.r 23 [OK] -- OUT: 23 . [OK]
>
> Can we assume that Git would pass files to filter in alphabetical
> order? This assumption might make the test unnecessary fragile.
I have never experienced another behavior. If we see fragility we could
sort the result...
>>
>> +test_expect_success PERL 'required process filter should clean only and take precedence' '
>
> Trying to describe it better results in overly long description,
> which probably means that this test should be split into few
> smaller ones:
>
> - `process` filter takes precedence over `clean` and/or `smudge`
> filters, regardless if it supports relevant ("clean" or "smudge")
> capability or not
>
> - `process` filter that includes only "clean" capability should
> clean only (be used only for 'clean' operation)
Agreed!
> In my opinion all functions should be placed at beginning,
> or even in separate file (if they are used in more than
> one test).
OK
>> +generate_test_data () {
>
> The name is not good, it doesn't describe what kind of data
> we want to generate.
"generate_random_characters" ok?!
>> + perl -pe "s/./chr((ord($&) % 26) + 97)/sge" >../$NAME.file &&
>
> Those constants (26 and 97) are a bit cryptic; magical constants.
> I guess this is
>
> + perl -pe "s/./chr((ord($&) % (ord('z') - ord('a') + 1) + ord('a'))/sge" >../$NAME.file &&
>
> or
>
> + perl -pe "s/./chr((ord($&) % 26 + ord('a'))/sge" >../$NAME.file &&
OK!
> Do we re-generate this file each time?
>
>> + ./../rot13.sh <../$NAME.file >../$NAME.file.rot13
>
> Anyway, I wonder if taking the last two lines out of the function
> (as they are not about _generating_ a file) would make it more
> readable or not.
Agreed.
>> +
>> + echo "*.file filter=protocol" >.gitattributes &&
>> + check_filter \
>> + git add *.file .gitattributes \
>
> Should it be shell expansion, or git expansion, that is
>
> git add '*.file' .gitattributes
Both have the same output. Would the difference matter?
>> + 1 START
>> + 1 STOP
>> + 1 wrote filter header
>> + EOF
>> + git commit . -m "test commit" &&
>
> Is this needed / necessary?
Yes, to test the smudge afterwards!
>
>> +
>> + rm -f *.file &&
>> + git checkout -- *.file &&
>
> Is this necessary? I guess this checks that it doesn't crash, but
> we do not check that smudge operation works correctly, as we did
> for clean.
Good point. Smudge check added!
>> + for f in *.file
>> + do
>> + git cat-file blob :$f >actual &&
>> + test_cmp ../$f.rot13 actual
>> + done
>
> Wasn't there helper function for this?
True :-)
>> +test_expect_success PERL 'required process filter should with clean error should fail' '
> ^^^^^^ ^^^^^^
>
> Errr... what? You have 'should' twice here.
Fixed
> Also, does it matter that the error is during clean operation?
> We don't test that error during smudge operation is handled in
> the same way, do we?
Clean and smudge should hit the same code paths here. Therefore I think
it is sufficient to test clean only.
>> + test_config_global filter.protocol.process "$TEST_DIRECTORY/t0021/rot13-filter.pl clean smudge" &&
>
> Do we need to pass 'clean smudge', or does it provide both by
> default?
We need to pass them. Default is empty.
>> + git add . &&
>> + git commit . -m "test commit" &&
>
> You don't need to commit for 'git checkout <path>' (e.g. for .)
> or 'git cat-file -p :<file>' to work.
True!
>> + )
>> +'
>> +
>> +test_expect_success PERL 'process filter should not restart in case of an error' '
>
> Errr... what? This description is not clear. Did you mean
> that filter should not be restarted if it *signals* an error
> with file (either before sending anything, or after sending
> partial contents)?
OK renamed to "process filter should not be restarted if it signals an error"
>> +test_expect_success PERL 'process filter should be able to signal an error for all future files' '
>
> Did you mean here that filter can abort processing of
> all future files?
"process filter signals abort once to abort processing of all future files", better?
>> +
>> + cp ../test.o test.r &&
>> + test_must_fail git add . 2> git_stderr.log &&
>> + grep "not support long running filter protocol" git_stderr.log
>
> Shouldn't this use gettext poison (or rather C locale)?
> This error message could be translated in the future.
I would prefer to adjust that when we translate it.
>> + $str =~ y/A-Za-z/N-ZA-Mn-za-m/;
>
> Why not use tr/// version of this quote-like operation?
> Or do you follow prior art here?
I am not Perl expert. That worked for me :-)
>> +sub packet_bin_read {
>> + my $buffer;
>> + my $bytes_read = read STDIN, $buffer, 4;
>> + if ( $bytes_read == 0 ) {
>> +
>> + # EOF - Git stopped talking to us!
>> + print $debug "STOP\n";
>> + exit();
>> + }
>> + elsif ( $bytes_read != 4 ) {
>> + die "invalid packet size '$bytes_read' field";
>
> Errr, $bytes_read is not packet size field. It is $buffer.
> Also, error message looks strange
>
> invalid packet size '004' field
>
> Shouldn't it be at end?
True. Fixed!
>> + }
>> + return ( 0, $buffer );
>> + }
>> + else {
>> + die "invalid packet size";
>
> Is keep-alive packet valid ("0004")?
No.
>>
>> +packet_flush();
>> +print $debug "wrote filter header\n";
>
> Or perhaps "handshake end"?
"init handshake complete", ok?
>> + print $debug " $pathname";
>
> No " pathname=$pathname" ?
Yes, otherwise it gets too verbose in the tests.
>> + while ( length($output) > 0 ) {
>> + my $packet = substr( $output, 0, $MAX_PACKET_CONTENT_SIZE );
>> + packet_bin_write($packet);
>> + print $debug ".";
>
> All right, so number of dots is the number of packets. This is
> surprisingly opaque.
I added a comment.
>
>> + if ( length($output) > $MAX_PACKET_CONTENT_SIZE ) {
>> + $output = substr( $output, $MAX_PACKET_CONTENT_SIZE );
>> + }
>> + else {
>> + $output = "";
>> + }
>> + }
>> + packet_flush();
>> + print $debug " [OK]\n";
>> + $debug->flush();
>> + packet_flush();
>
> Should we test partial contents case? Or failure during printing?
> What happens then - is file cleared by Git, or left partially converted?
Git will clear the file on any error (it doesn't matter when the error happens).
---
I am astonished how many valuable suggestion you were able to make
even though I am working with this code for months now.
Thanks a lot for taking the time to review my code that thoroughly.
- Lars
^ permalink raw reply
* Re: [PATCH 1/3] add QSORT
From: René Scharfe @ 2016-10-01 16:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List
In-Reply-To: <xmqqponmcp07.fsf@gitster.mtv.corp.google.com>
Am 30.09.2016 um 00:36 schrieb Junio C Hamano:
> 3. builtin/show-branch.c does this:
>
> qsort(ref_name + bottom, top - bottom, sizeof(ref_name[0]),
> compare_ref_name);
>
> where ref_name[] is a file-scope global:
>
> static char *ref_name[MAX_REVS + 1];
>
> and top and bottom are plain integers. The sizeof() does not take
> the size of *base, so it is understandable that this does not get
> automatically converted.
>
> It seems that some calls to this function _could_ send the same top
> and bottom, asking for 0 element array to be sorted, by the way.
It's hard to imagine an implementation of qsort(3) that can't handle
zero elements. QSORT's safety feature is that it prevents the compiler
from removing NULL checks for the array pointer. E.g. the last two
lines in the following example could be optimized away:
qsort(ptr, n, sizeof(*ptr), fn);
if (!ptr)
do_stuff();
You can see that on https://godbolt.org/g/JwS99b -- an awesome website
for exploring compilation results for small snippets, by the way.
This optimization is dangerous when combined with the convention of
using a NULL pointer for empty arrays. Diagnosing an affected NULL
check is probably quite hard -- it's right there in the code after all
and not all compilers remove it.
builtin/show-branch.c never passes NULL, so it's not affected by that
hazard. We can (and should, IMHO) still use QSORT there for
consistency and convenience, though:
-- >8 --
Subject: [PATCH] show-branch: use QSORT
Shorten the code by using QSORT instead of calling qsort(3) directly,
as the former determines the element size automatically and checks if
there are at least two elements to sort already.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
builtin/show-branch.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 623ca56..974f340 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -353,8 +353,7 @@ static int compare_ref_name(const void *a_, const void *b_)
static void sort_ref_range(int bottom, int top)
{
- qsort(ref_name + bottom, top - bottom, sizeof(ref_name[0]),
- compare_ref_name);
+ QSORT(ref_name + bottom, top - bottom, compare_ref_name);
}
static int append_ref(const char *refname, const struct object_id *oid,
@@ -540,8 +539,7 @@ static void append_one_rev(const char *av)
if (saved_matches == ref_name_cnt &&
ref_name_cnt < MAX_REVS)
error(_("no matching refs with %s"), av);
- if (saved_matches + 1 < ref_name_cnt)
- sort_ref_range(saved_matches, ref_name_cnt);
+ sort_ref_range(saved_matches, ref_name_cnt);
return;
}
die("bad sha1 reference %s", av);
--
2.10.0
^ permalink raw reply related
* Re: [PATCH v2 04/11] i18n: add--interactive: mark plural strings
From: Jakub Narębski @ 2016-10-01 16:49 UTC (permalink / raw)
To: Vasco Almeida, Junio C Hamano, git
Cc: Jiang Xin, Ævar Arnfjörð Bjarmason, David Aguilar
In-Reply-To: <1474913721.1035.9.camel@sapo.pt>
W dniu 26.09.2016 o 20:15, Vasco Almeida pisze:
> A Qua, 31-08-2016 às 12:31 +0000, Vasco Almeida escreveu:
>>
>> Mark plural strings for translation. Unfold each action case in one
>> entire sentence.
>>
>> Pass new keyword for xgettext to extract.
>>
>> Update test to include new subrotine Q__() for plural strings handling.
Why use Q__() as the name of the subroutine? [looks further]. Oh, I see
that you are following the example of C shortcut functions (_, Q_ and N_).
But this is Perl, not C. The standard shortcut functions are those
defined in Locale::TextDomain, even if we can't use this module directly.
Those that deal with plural strings handling are __n and __nx / __xn.
The Perl equivalent of Q_ shorthand function in C, C++, etc. is __n.
There is also a function __nx for combining handling plural strings
together with variable interpolation.
__n MSGID, MSGID_PLURAL, COUNT
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
That is the reason for the existance of the function ngettext(),
that __n() is a short-cut for:
print __n"One file has been deleted.\n",
"All files have been deleted.\n",
$files_deleted;
Alternatively:
print __n ("One file has been deleted.\n",
"All files have been deleted.\n",
$files_deleted);
__nx MSGID, MSGID_PLURAL, COUNT, VAR1 => VAL1, VAR2 => VAL2, ...
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Bringing it all together:
print __nx ("One file has been deleted.\n",
"{count} files have been deleted.\n",
$num_files,
count => $num_files);
The function __nx() [and its alias __xn()] picks the correct plural
form (also for English!) and it is capable of interpolating variables
into strings.
>>
>> Signed-off-by: Vasco Almeida <vascomalmeida@sapo.pt>
>> ---
>> Makefile | 3 ++-
>> git-add--interactive.perl | 24 ++++++++++++++++--------
>> perl/Git/I18N.pm | 4 +++-
>> t/t0202/test.pl | 11 ++++++++++-
>> 4 files changed, 31 insertions(+), 11 deletions(-)
>> diff --git a/Makefile b/Makefile
>> index de5a030..eedf1fa 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -2061,7 +2061,8 @@ XGETTEXT_FLAGS_C = $(XGETTEXT_FLAGS) --language=C \
>> --keyword=_ --keyword=N_ --keyword="Q_:1,2"
>> XGETTEXT_FLAGS_SH = $(XGETTEXT_FLAGS) --language=Shell \
>> --keyword=gettextln --keyword=eval_gettextln
>> -XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --keyword=__ --language=Perl
>> +XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
>> + --keyword=__ --keyword="Q__:1,2"
So this would be
+XGETTEXT_FLAGS_PERL = $(XGETTEXT_FLAGS) --language=Perl \
+ --keyword=__ --keyword=__x --keyword=__n:1,2 --keyword=__nx:1,2
(assuming that __x was used for interpolation)
>> LOCALIZED_C = $(C_OBJ:o=c) $(LIB_H) $(GENERATED_H)
>> LOCALIZED_SH = $(SCRIPT_SH) git-parse-remote.sh
>> LOCALIZED_PERL = $(SCRIPT_PERL)
[...]
>> diff --git a/git-add--interactive.perl b/git-add--interactive.perl
>> index 4e1e857..08badfa 100755
>> --- a/git-add--interactive.perl
>> +++ b/git-add--interactive.perl
>> @@ -666,12 +666,18 @@ sub status_cmd {
>> sub say_n_paths {
>> my $did = shift @_;
>> my $cnt = scalar @_;
>> - print "$did ";
>> - if (1 < $cnt) {
>> - print "$cnt paths\n";
>> - }
>> - else {
>> - print "one path\n";
>> + if ($did eq 'added') {
>> + printf(Q__("added one path\n", "added %d paths\n",
>> + $cnt), $cnt);
>> + } elsif ($did eq 'updated') {
>> + printf(Q__("updated one path\n", "updated %d paths\n",
>> + $cnt), $cnt);
>> + } elsif ($did eq 'reverted') {
>> + printf(Q__("reverted one path\n", "reverted %d paths\n",
>> + $cnt), $cnt);
>> + } else {
>> + printf(Q__("touched one path\n", "touched %d paths\n",
>> + $cnt), $cnt);
>> }
>> }
One one hand side, it is recommended to avoid lego-like construction
of sentences.
Translatable strings should be entire sentences. It is often not
possible to translate single verbs or adjectives in a substitutable
way.
I think however that the action part ($did in original non-i18n code)
is whole part in any language, so something like the following would
be enough:
# this hash is as much for validation, as for translation
my %actions = map { $_ => 1 } (N__"added", N__"updated", N__"reverted");
if (exists $actions{$did}) {
print __nx("{did} one path\n", "{did} {count} paths\n", $cnt,
did => __($did), count => $cnt);
} else {
print __nx("touched one path\n", "touched {count} paths\n", $cnt,
count => $cnt);
}
Please correct me if I am wrong, and you know language where
"added %d paths", "updated %d paths", "reverted %d paths" etc. must have
different word order.
> When $cnt is 1 I get the following warning:
> Redundant argument in printf at .../libexec/git-core/git-add--interactive line 680.
I wonder what is the case of C code - is similar warning here, or is
gettext smarter in that case...
> The singular form does not have a %d to consume $cnt argument to
> printf(). Either we find a way to suppress that warning or we change
> the singular form to contain %d.
Anyway, with __nx there should be no such problem.
>
>> @@ -1508,8 +1514,10 @@ sub patch_update_file {
>> ...
>> - print colored $header_color, "Split into ",
>> - scalar(@split), " hunks.\n";
>> + print colored $header_color, sprintf(
>> + Q__("Split into %d hunk.\n",
>> + "Split into %d hunks.\n",
>> + scalar(@split)), scalar(@split));
>
> Like we do with this.
Note that it is a bit of change in behavior: previously Git would
say "Split into 0 hunks.\n".
Though this is probably more work that you wanted to do. The __n would
take place of Q__, while the __nx function might be defined like this
(borrowing from Locale::TextDomain), which needs to be put into Git::I18N:
# Plural with interpolation.
sub __nx ($$$@)
{
my ($msgid, $msgid_plural, $count, %args) = @_;
return __expand ((__n $msgid, $msgid_plural, $count),
%args);
}
Best regards,
--
Jakub Narębski
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox