* [PATCH v2 3/5] worktree.c: kill parse_ref() in favor of refs_resolve_ref_unsafe()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 12:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Stefan Beller,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216120302.5302-1-pclouds@gmail.com>
The manual parsing code is replaced with a call to refs_resolve_ref_unsafe().
The manual parsing code must die because only refs/files-backend.c
should do that.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
branch.c | 3 +-
worktree.c | 102 ++++++++++++++++---------------------------------------------
worktree.h | 2 +-
3 files changed, 30 insertions(+), 77 deletions(-)
diff --git a/branch.c b/branch.c
index c431cbf6a..fcb4a765c 100644
--- a/branch.c
+++ b/branch.c
@@ -354,7 +354,8 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
for (i = 0; worktrees[i]; i++) {
if (worktrees[i]->is_detached)
continue;
- if (strcmp(oldref, worktrees[i]->head_ref))
+ if (worktrees[i]->head_ref &&
+ strcmp(oldref, worktrees[i]->head_ref))
continue;
if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
diff --git a/worktree.c b/worktree.c
index eb6121263..f8f520036 100644
--- a/worktree.c
+++ b/worktree.c
@@ -19,54 +19,25 @@ void free_worktrees(struct worktree **worktrees)
free (worktrees);
}
-/*
- * read 'path_to_ref' into 'ref'. Also if is_detached is not NULL,
- * set is_detached to 1 (0) if the ref is detached (is not detached).
- *
- * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so
- * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses
- * git_path). Parse the ref ourselves.
- *
- * return -1 if the ref is not a proper ref, 0 otherwise (success)
- */
-static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
-{
- if (is_detached)
- *is_detached = 0;
- if (!strbuf_readlink(ref, path_to_ref, 0)) {
- /* HEAD is symbolic link */
- if (!starts_with(ref->buf, "refs/") ||
- check_refname_format(ref->buf, 0))
- return -1;
- } else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
- /* textual symref or detached */
- if (!starts_with(ref->buf, "ref:")) {
- if (is_detached)
- *is_detached = 1;
- } else {
- strbuf_remove(ref, 0, strlen("ref:"));
- strbuf_trim(ref);
- if (check_refname_format(ref->buf, 0))
- return -1;
- }
- } else
- return -1;
- return 0;
-}
-
/**
- * Add the head_sha1 and head_ref (if not detached) to the given worktree
+ * Update head_sha1, head_ref and is_detached of the given worktree
*/
-static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)
+static void add_head_info(struct worktree *wt)
{
- if (head_ref->len) {
- if (worktree->is_detached) {
- get_sha1_hex(head_ref->buf, worktree->head_sha1);
- } else {
- resolve_ref_unsafe(head_ref->buf, 0, worktree->head_sha1, NULL);
- worktree->head_ref = strbuf_detach(head_ref, NULL);
- }
- }
+ int flags;
+ const char *target;
+
+ target = refs_resolve_ref_unsafe(get_worktree_ref_store(wt),
+ "HEAD",
+ RESOLVE_REF_READING,
+ wt->head_sha1, &flags);
+ if (!target)
+ return;
+
+ if (flags & REF_ISSYMREF)
+ wt->head_ref = xstrdup(target);
+ else
+ wt->is_detached = 1;
}
/**
@@ -77,9 +48,7 @@ static struct worktree *get_main_worktree(void)
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
struct strbuf worktree_path = STRBUF_INIT;
- struct strbuf head_ref = STRBUF_INIT;
int is_bare = 0;
- int is_detached = 0;
strbuf_add_absolute_path(&worktree_path, get_git_common_dir());
is_bare = !strbuf_strip_suffix(&worktree_path, "/.git");
@@ -91,13 +60,10 @@ static struct worktree *get_main_worktree(void)
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->is_bare = is_bare;
- worktree->is_detached = is_detached;
- if (!parse_ref(path.buf, &head_ref, &is_detached))
- add_head_info(&head_ref, worktree);
+ add_head_info(worktree);
strbuf_release(&path);
strbuf_release(&worktree_path);
- strbuf_release(&head_ref);
return worktree;
}
@@ -106,8 +72,6 @@ static struct worktree *get_linked_worktree(const char *id)
struct worktree *worktree = NULL;
struct strbuf path = STRBUF_INIT;
struct strbuf worktree_path = STRBUF_INIT;
- struct strbuf head_ref = STRBUF_INIT;
- int is_detached = 0;
if (!id)
die("Missing linked worktree name");
@@ -127,19 +91,14 @@ static struct worktree *get_linked_worktree(const char *id)
strbuf_reset(&path);
strbuf_addf(&path, "%s/worktrees/%s/HEAD", get_git_common_dir(), id);
- if (parse_ref(path.buf, &head_ref, &is_detached) < 0)
- goto done;
-
worktree = xcalloc(1, sizeof(*worktree));
worktree->path = strbuf_detach(&worktree_path, NULL);
worktree->id = xstrdup(id);
- worktree->is_detached = is_detached;
- add_head_info(&head_ref, worktree);
+ add_head_info(worktree);
done:
strbuf_release(&path);
strbuf_release(&worktree_path);
- strbuf_release(&head_ref);
return worktree;
}
@@ -334,8 +293,6 @@ const struct worktree *find_shared_symref(const char *symref,
const char *target)
{
const struct worktree *existing = NULL;
- struct strbuf path = STRBUF_INIT;
- struct strbuf sb = STRBUF_INIT;
static struct worktree **worktrees;
int i = 0;
@@ -345,6 +302,11 @@ const struct worktree *find_shared_symref(const char *symref,
for (i = 0; worktrees[i]; i++) {
struct worktree *wt = worktrees[i];
+ const char *symref_target;
+ unsigned char sha1[20];
+ struct ref_store *refs;
+ int flags;
+
if (wt->is_bare)
continue;
@@ -359,24 +321,14 @@ const struct worktree *find_shared_symref(const char *symref,
}
}
- strbuf_reset(&path);
- strbuf_reset(&sb);
- strbuf_addf(&path, "%s/%s",
- get_worktree_git_dir(wt),
- symref);
-
- if (parse_ref(path.buf, &sb, NULL)) {
- continue;
- }
-
- if (!strcmp(sb.buf, target)) {
+ refs = get_worktree_ref_store(wt);
+ symref_target = refs_resolve_ref_unsafe(refs, symref, 0,
+ sha1, &flags);
+ if ((flags & REF_ISSYMREF) && !strcmp(symref_target, target)) {
existing = wt;
break;
}
}
- strbuf_release(&path);
- strbuf_release(&sb);
-
return existing;
}
diff --git a/worktree.h b/worktree.h
index d59ce1fee..961252e71 100644
--- a/worktree.h
+++ b/worktree.h
@@ -4,7 +4,7 @@
struct worktree {
char *path;
char *id;
- char *head_ref;
+ char *head_ref; /* NULL if HEAD is broken or detached */
char *lock_reason; /* internal use */
unsigned char head_sha1[20];
int is_detached;
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v2 4/5] refs: add refs_create_symref()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 12:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Stefan Beller,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216120302.5302-1-pclouds@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
refs.c | 22 +++++++++++++++++-----
refs.h | 4 ++++
2 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/refs.c b/refs.c
index e0e191107..23e0a8eda 100644
--- a/refs.c
+++ b/refs.c
@@ -1535,13 +1535,25 @@ int peel_ref(const char *refname, unsigned char *sha1)
return refs->be->peel_ref(refs, refname, sha1);
}
-int create_symref(const char *ref_target, const char *refs_heads_master,
+int refs_create_symref(struct ref_store *refs,
+ const char *ref_target,
+ const char *refs_heads_master,
+ const char *logmsg)
+{
+ return refs->be->create_symref(refs,
+ ref_target,
+ refs_heads_master,
+ logmsg);
+}
+
+int create_symref(const char *ref_target,
+ const char *refs_heads_master,
const char *logmsg)
{
- struct ref_store *refs = get_main_ref_store();
-
- return refs->be->create_symref(refs, ref_target, refs_heads_master,
- logmsg);
+ return refs_create_symref(get_main_ref_store(),
+ ref_target,
+ refs_heads_master,
+ logmsg);
}
int ref_transaction_commit(struct ref_transaction *transaction,
diff --git a/refs.h b/refs.h
index 10c2cfc00..694769963 100644
--- a/refs.h
+++ b/refs.h
@@ -573,5 +573,9 @@ const char *refs_resolve_ref_unsafe(struct ref_store *refs,
int resolve_flags,
unsigned char *sha1,
int *flags);
+int refs_create_symref(struct ref_store *refs,
+ const char *refname,
+ const char *target,
+ const char *logmsg);
#endif /* REFS_H */
--
2.11.0.157.gd943d85
^ permalink raw reply related
* [PATCH v2 5/5] refs: kill set_worktree_head_symref()
From: Nguyễn Thái Ngọc Duy @ 2017-02-16 12:03 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Michael Haggerty, Stefan Beller,
Nguyễn Thái Ngọc Duy
In-Reply-To: <20170216120302.5302-1-pclouds@gmail.com>
70999e9cec (branch -m: update all per-worktree HEADs - 2016-03-27)
added this function in order to update HEADs of all relevant
worktrees, when a branch is renamed.
It, as a public ref api, kind of breaks abstraction when it uses
internal functions of files backend. With the introduction of
refs_create_symref(), we can move back pretty close to the code before
70999e9cec, where create_symref() was used for updating HEAD.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
branch.c | 12 ++++++------
refs.h | 9 ---------
refs/files-backend.c | 41 -----------------------------------------
3 files changed, 6 insertions(+), 56 deletions(-)
diff --git a/branch.c b/branch.c
index fcb4a765c..ad0cc0489 100644
--- a/branch.c
+++ b/branch.c
@@ -352,18 +352,18 @@ int replace_each_worktree_head_symref(const char *oldref, const char *newref)
int i;
for (i = 0; worktrees[i]; i++) {
+ struct ref_store *refs;
+
if (worktrees[i]->is_detached)
continue;
if (worktrees[i]->head_ref &&
strcmp(oldref, worktrees[i]->head_ref))
continue;
- if (set_worktree_head_symref(get_worktree_git_dir(worktrees[i]),
- newref)) {
- ret = -1;
- error(_("HEAD of working tree %s is not updated"),
- worktrees[i]->path);
- }
+ refs = get_worktree_ref_store(worktrees[i]);
+ if (refs_create_symref(refs, "HEAD", newref, NULL))
+ ret = error(_("HEAD of working tree %s is not updated"),
+ worktrees[i]->path);
}
free_worktrees(worktrees);
diff --git a/refs.h b/refs.h
index 694769963..bce77891a 100644
--- a/refs.h
+++ b/refs.h
@@ -325,15 +325,6 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg);
int create_symref(const char *refname, const char *target, const char *logmsg);
-/*
- * Update HEAD of the specified gitdir.
- * Similar to create_symref("relative-git-dir/HEAD", target, NULL), but
- * this can update the main working tree's HEAD regardless of where
- * $GIT_DIR points to.
- * Return 0 if successful, non-zero otherwise.
- * */
-int set_worktree_head_symref(const char *gitdir, const char *target);
-
enum action_on_err {
UPDATE_REFS_MSG_ON_ERR,
UPDATE_REFS_DIE_ON_ERR,
diff --git a/refs/files-backend.c b/refs/files-backend.c
index f3be620ab..ba56e46d4 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3108,47 +3108,6 @@ static int files_create_symref(struct ref_store *ref_store,
return ret;
}
-int set_worktree_head_symref(const char *gitdir, const char *target)
-{
- /*
- * FIXME: this obviously will not work well for future refs
- * backends. This function needs to die.
- */
- struct files_ref_store *refs =
- files_downcast(get_main_ref_store(), "set_head_symref");
- static struct lock_file head_lock;
- struct ref_lock *lock;
- struct strbuf head_path = STRBUF_INIT;
- const char *head_rel;
- int ret;
-
- strbuf_addf(&head_path, "%s/HEAD", absolute_path(gitdir));
- if (hold_lock_file_for_update(&head_lock, head_path.buf,
- LOCK_NO_DEREF) < 0) {
- struct strbuf err = STRBUF_INIT;
- unable_to_lock_message(head_path.buf, errno, &err);
- error("%s", err.buf);
- strbuf_release(&err);
- strbuf_release(&head_path);
- return -1;
- }
-
- /* head_rel will be "HEAD" for the main tree, "worktrees/wt/HEAD" for
- linked trees */
- head_rel = remove_leading_path(head_path.buf,
- absolute_path(get_git_common_dir()));
- /* to make use of create_symref_locked(), initialize ref_lock */
- lock = xcalloc(1, sizeof(struct ref_lock));
- lock->lk = &head_lock;
- lock->ref_name = xstrdup(head_rel);
-
- ret = create_symref_locked(refs, lock, head_rel, target, NULL);
-
- unlock_ref(lock); /* will free lock */
- strbuf_release(&head_path);
- return ret;
-}
-
static int files_reflog_exists(struct ref_store *ref_store,
const char *refname)
{
--
2.11.0.157.gd943d85
^ permalink raw reply related
* Re: [PATCH] show-branch: fix crash with long ref name
From: Christian Couder @ 2017-02-16 12:40 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git, Maxim Kuvyrkov, Pranit Bauva
In-Reply-To: <20170215214052.5py4pxkcz4g2bmtk@sigill.intra.peff.net>
On Wed, Feb 15, 2017 at 10:40 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:
>
>> > I notice Christian's patch added a few tests. I don't know if we'd want
>> > to squash them in (I didn't mean to override his patch at all; I was
>> > about to send mine out when I noticed his, and I wondered if we wanted
>> > to combine the two efforts).
>>
>> I think it would be nice to have at least one test. Feel free to
>> squash mine if you want.
>
> I started to add some tests, but I had second thoughts. It _is_ nice
> to show off the fix, but as far as regressions go, this specific case is
> unlikely to come up again. What would be more valuable, I think, is a
> test script which set up a very long refname (not just 150 bytes or
> whatever) and ran it through a series of git commands.
I agree that a test script running through a series of command with
long refnames would be great.
But I think the refname should not necesarily be too long. As I wrote
in the commit message of my patch, if the ref name had been much
longer the crash would not have happened because the ref could not
have been created in the first place.
So the best would be to run through a series of commands with a
refname ranging from let's say 80 chars to 300 chars.
That would have a chance to catch crashes due to legacy code using for
example things like `char stuff[128]` or `char stuff[256]`.
Implementing those tests could have started with something like the
test case I sent, but as it would in the end be about many different
commands, one can see it as part of a different topic.
> But then you run into all sorts of portability annoyances with pathname
> restrictions (you can hack around creation by writing the refname
> directly into packed-refs, but most manipulations will want to take the
> .lock in the filesystem).
Yeah, but if a crash doesn't happen because we die() as the ref is too
long for the file system, we could detect that and make the test
succeed.
> So I dunno. It seems like being thorough is a
> lot of hassle for not much gain. Being not-thorough is easy, but is
> mostly a token that is unlikely to find any real bugs.
Yeah, if we really care, it might be better to start using a fuzzer or
a property based testing tool instead of bothering with these kind of
tests by ourselves, which is also a different topic.
> So I punted, at least for now.
Ok, no problem.
^ permalink raw reply
* [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"
From: Siddharth Kannan @ 2017-02-16 15:14 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
This is as per our discussion[1]. The patches and commit messages are based on
Junio's patches that were posted as a reply to
<20170212184132.12375-1-gitster@pobox.com>.
As per Matthieu's comments, I have updated the tests, but there is still one
thing that is not working: log -@{yesterday} or log -@{2.days.ago}
For the other kinds of suffixes, such as -^ or -~ or -~N, the suffix
information is first extracted and then, the function get_sha1_1 is called with
name="-^" and len=1 (which is the reason for the changed condition inside Patch
4 of this series).
For -@{yesterday} kind of queries, the functions dwim_log,
interpret_branch_name and interpret_nth_prior_checkout are called.
1. A nice way to solve this would be to extend the replacement of "-" with
"@{-1}" one step further. Using strbuf, instead of replacing the whole string
with "@{-1}" we would simply replace "-" with "@{-1}" expanding the string
appropriately. This will ensure that all the code is inside the function
get_sha1_1. The code to do this is in the cover section of the 4th patch in this
series.
2. we could go down the dwim_log codepath, and find another suitable place to
make the same "-" -> "@{-1}" replacement. In the time that I spent till now, it
seems that the suffix information (i.e. @{yesterday} or @{2.days.ago}) is
extracted _after_ the branch information has been extracted, so I suspect that
we will have to keep that part intact even in this solution. (I am not too
sure about this. If this is the preferred solution, then I will dig deeper and
find the right place as I did for the first part of this patch)
Matthieu: Thanks a lot for your comments on the tests! test_commit has made the
tests a lot cleaner!
[1]: <xmqqh941ippo.fsf@gitster.mtv.corp.google.com>
Siddharth Kannan (4):
revision.c: do not update argv with unknown option
revision.c: swap if/else blocks
revision.c: args starting with "-" might be a revision
sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
revision.c | 15 ++++---
sha1_name.c | 5 +++
t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 120 insertions(+), 6 deletions(-)
create mode 100755 t/t4214-log-shorthand.sh
--
2.1.4
^ permalink raw reply
* [PATCH 1/4 v4] revision.c: do not update argv with unknown option
From: Siddharth Kannan @ 2017-02-16 15:14 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com>
handle_revision_opt() tries to recognize and handle the given argument. If an
option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
increment of unkc causes the variable in the caller to change.
Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
This is now the responsibility of the caller.
There are two callers of this function:
1. setup_revision: Changes have been made so that setup_revision will now
update the unknown option in argv
2. parse_revision_opt: No changes are required here. This function throws an
error whenever the option provided as argument was unknown to
handle_revision_opt().
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index b37dbec..5674a9a 100644
--- a/revision.c
+++ b/revision.c
@@ -2016,8 +2016,6 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->ignore_missing = 1;
} else {
int opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);
- if (!opts)
- unkv[(*unkc)++] = arg;
return opts;
}
if (revs->graph && revs->track_linear)
@@ -2234,6 +2232,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
+ /* arg is an unknown option */
+ argv[left++] = arg;
continue;
}
--
2.1.4
^ permalink raw reply related
* [PATCH 2/4 v4] revision.c: swap if/else blocks
From: Siddharth Kannan @ 2017-02-16 15:14 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com>
Swap the condition and bodies of an "if (A) do_A else do_B" in
setup_revisions() to "if (!A) do_B else do_A", to make the change in
the the next step easier to read.
No behaviour change is intended in this step.
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/revision.c b/revision.c
index 5674a9a..8d4ddae 100644
--- a/revision.c
+++ b/revision.c
@@ -2238,7 +2238,9 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
- if (handle_revision_arg(arg, revs, flags, revarg_opt)) {
+ if (!handle_revision_arg(arg, revs, flags, revarg_opt))
+ got_rev_arg = 1;
+ else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
@@ -2255,8 +2257,6 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
append_prune_data(&prune_data, argv + i);
break;
}
- else
- got_rev_arg = 1;
}
if (prune_data.nr) {
--
2.1.4
^ permalink raw reply related
* [PATCH 3/4 v4] revision.c: args starting with "-" might be a revision
From: Siddharth Kannan @ 2017-02-16 15:14 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com>
setup_revisions used to consider any argument starting with "-" to be either a
valid or an unknown option.
Teach setup_revisions to check if an argument is a revision before adding it as
an unknown option (something that setup_revisions didn't understand) to argv,
and moving on to the next argument.
This patch prepares the addition of "-" as a shorthand for "previous branch".
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
revision.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/revision.c b/revision.c
index 8d4ddae..5470c33 100644
--- a/revision.c
+++ b/revision.c
@@ -2203,6 +2203,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
read_from_stdin = 0;
for (left = i = 1; i < argc; i++) {
const char *arg = argv[i];
+ int maybe_opt = 0;
if (*arg == '-') {
int opts;
@@ -2232,15 +2233,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
}
if (opts < 0)
exit(128);
- /* arg is an unknown option */
- argv[left++] = arg;
- continue;
+ maybe_opt = 1;
}
if (!handle_revision_arg(arg, revs, flags, revarg_opt))
got_rev_arg = 1;
- else {
+ else if (maybe_opt) {
+ /* arg is an unknown option */
+ argv[left++] = arg;
+ continue;
+ } else {
int j;
if (seen_dashdash || *arg == '^')
die("bad revision '%s'", arg);
--
2.1.4
^ permalink raw reply related
* [PATCH 4/4 v4] sha1_name.c: teach get_sha1_1 "-" shorthand for "@{-1}"
From: Siddharth Kannan @ 2017-02-16 15:14 UTC (permalink / raw)
To: git
Cc: gitster, Matthieu.Moy, pranit.bauva, peff, pclouds, sandals,
Siddharth Kannan
In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com>
This patch introduces "-" as a method to refer to a revision, and adds tests to
test that git-log works with this shorthand.
This change will touch the following commands (through
revision.c:setup_revisions):
* builtin/blame.c
* builtin/diff.c
* builtin/diff-files.c
* builtin/diff-index.c
* builtin/diff-tree.c
* builtin/log.c
* builtin/rev-list.c
* builtin/shortlog.c
* builtin/fast-export.c
* builtin/fmt-merge-msg.c
builtin/add.c
builtin/checkout.c
builtin/commit.c
builtin/merge.c
builtin/pack-objects.c
builtin/revert.c
* marked commands are information-only.
As most commands in this list are not of the rm-variety, (i.e a command that
would delete something), this change does not make it easier for people to
delete. (eg: "git branch -d -" is *not* enabled by this patch)
Signed-off-by: Siddharth Kannan <kannan.siddharth12@gmail.com>
---
Instead of replacing the whole string, we would expand it accordingly using:
if (*name == '-') {
if (len == 1) {
name = "@{-1}";
len = 5;
} else {
struct strbuf changed_argument = STRBUF_INIT;
strbuf_addstr(&changed_argument, "@{-1}");
strbuf_addstr(&changed_argument, name + 1);
strbuf_setlen(&changed_argument, strlen(name) + 4);
name = strbuf_detach(&changed_argument, NULL);
}
}
Junio's comments on a previous version of the patch which used this same
approach but inside setup_revisions [1]
[1]: <xmqqtw882n08.fsf@gitster.mtv.corp.google.com>
sha1_name.c | 5 +++
t/t4214-log-shorthand.sh | 106 +++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 111 insertions(+)
create mode 100755 t/t4214-log-shorthand.sh
diff --git a/sha1_name.c b/sha1_name.c
index 73a915f..2f86bc9 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -947,6 +947,11 @@ static int get_sha1_1(const char *name, int len, unsigned char *sha1, unsigned l
if (!ret)
return 0;
+ if (*name == '-' && len == 1) {
+ name = "@{-1}";
+ len = 5;
+ }
+
ret = get_sha1_basic(name, len, sha1, lookup_flags);
if (!ret)
return 0;
diff --git a/t/t4214-log-shorthand.sh b/t/t4214-log-shorthand.sh
new file mode 100755
index 0000000..659b100
--- /dev/null
+++ b/t/t4214-log-shorthand.sh
@@ -0,0 +1,106 @@
+#!/bin/sh
+
+test_description='log can show previous branch using shorthand - for @{-1}'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+ test_commit first &&
+ test_commit second &&
+ test_commit third &&
+ test_commit fourth &&
+ test_commit fifth &&
+ test_commit sixth &&
+ test_commit seventh
+'
+
+test_expect_success '"log -" should not work initially' '
+ test_must_fail git log -
+'
+
+test_expect_success 'setup branches for testing' '
+ git checkout -b testing-1 master^ &&
+ git checkout -b testing-2 master~2 &&
+ git checkout master
+'
+
+test_expect_success '"log -" should work' '
+ git log testing-2 >expect &&
+ git log - >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'symmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ...@{-1} >expect.first_empty &&
+ git log @{-1}... >expect.last_empty &&
+ git log ...- >actual.first_empty &&
+ git log -... >actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'asymmetric revision range should work when one end is left empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log ..@{-1} >expect.first_empty &&
+ git log @{-1}.. >expect.last_empty &&
+ git log ..- >actual.first_empty &&
+ git log -.. >actual.last_empty &&
+ test_cmp expect.first_empty actual.first_empty &&
+ test_cmp expect.last_empty actual.last_empty
+'
+
+test_expect_success 'symmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -...testing-1 >expect &&
+ git log testing-2...testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'asymmetric revision range should work when both ends are given' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -..testing-1 >expect &&
+ git log testing-2..testing-1 >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success 'multiple separate arguments should be handled properly' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log - - >expect.1 &&
+ git log @{-1} @{-1} >actual.1 &&
+ git log - HEAD >expect.2 &&
+ git log @{-1} HEAD >actual.2 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2
+'
+
+test_expect_success 'revision ranges with same start and end should be empty' '
+ git checkout testing-2 &&
+ git checkout master &&
+ test 0 -eq $(git log -...- | wc -l) &&
+ test 0 -eq $(git log -..- | wc -l)
+'
+
+test_expect_success 'suffixes to - should work' '
+ git checkout testing-2 &&
+ git checkout master &&
+ git log -~ >expect.1 &&
+ git log @{-1}~ >actual.1 &&
+ git log -~2 >expect.2 &&
+ git log @{-1}~2 >actual.2 &&
+ git log -^ >expect.3 &&
+ git log @{-1}^ >actual.3 &&
+ git log -@{yesterday} >expect.4 &&
+ git log @{-1}@{yesterday} >actual.4 &&
+ test_cmp expect.1 actual.1 &&
+ test_cmp expect.2 actual.2 &&
+ test_cmp expect.3 actual.3 &&
+ test_cmp expect.4 actual.4
+'
+
+test_done
--
2.1.4
^ permalink raw reply related
* Re: [PATCH 1/3] add git_psprintf helper function
From: Jonathan Tan @ 2017-02-16 15:51 UTC (permalink / raw)
To: Maxim Moseychuk, git; +Cc: peff
In-Reply-To: <20170216112829.18079-2-franchesko.salias.hudro.pedros@gmail.com>
On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:
> There are a number of places in the code where we call
> xsnprintf(), with the assumption that the output will fit into
> the buffer. If the buffer is small, then git die.
> In many places buffers have compile-time size, but generated string
> depends from current system locale (gettext)and can have size
> greater the buffer.
> Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8"
> on linux sources - it impossible.
>
> git_psprintf is similar to the standard C sprintf() function
> but safer, since it calculates the maximum space required
> and allocates memory to hold the result.
> The returned string should be freed with free() when no longer needed.
If I understand this correctly, xstrfmt (in strbuf.h) should already do
what you need, so you do not need a new function.
^ permalink raw reply
* Re: [PATCH 0/4 v4] WIP: allow "-" as a shorthand for "previous branch"
From: Matthieu Moy @ 2017-02-16 16:41 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1487258054-32292-1-git-send-email-kannan.siddharth12@gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> This is as per our discussion[1]. The patches and commit messages are based on
> Junio's patches that were posted as a reply to
> <20170212184132.12375-1-gitster@pobox.com>.
>
> As per Matthieu's comments, I have updated the tests, but there is still one
> thing that is not working: log -@{yesterday} or log -@{2.days.ago}
Note that I did not request that these things work, just that they seem
to be relevant tests: IMHO it's OK to reject them, but for example we
don't want them to segfault. And having a test is a good hint that you
thought about what could happen and to document it.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Confusing git messages when disk is full.
From: Jeff King @ 2017-02-16 16:44 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <87tw7uv439.fsf@linux-m68k.org>
On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
> >> int xfclose(FILE *fp)
> >> {
> >> return ferror(fp) | fclose(fp);
> >> }
> >
> > Yes, that's exactly what I had in mind (might be worth calling out the
> > bitwise-OR, though, just to make it clear it's not a typo).
>
> Since the order of evaluation is unspecified, it would be better to
> force sequencing ferror before fclose.
Good point. Arguably the call in tempfile.c is buggy.
-Peff
^ permalink raw reply
* Re: [PATCH 1/4 v4] revision.c: do not update argv with unknown option
From: Matthieu Moy @ 2017-02-16 16:48 UTC (permalink / raw)
To: Siddharth Kannan; +Cc: git, gitster, pranit.bauva, peff, pclouds, sandals
In-Reply-To: <1487258054-32292-2-git-send-email-kannan.siddharth12@gmail.com>
Siddharth Kannan <kannan.siddharth12@gmail.com> writes:
> handle_revision_opt() tries to recognize and handle the given argument. If an
> option was unknown to it, it used to add the option to unkv[(*unkc)++]. This
> increment of unkc causes the variable in the caller to change.
>
> Teach handle_revision_opt to not update unknown arguments inside unkc anymore.
> This is now the responsibility of the caller.
>
> There are two callers of this function:
>
> 1. setup_revision: Changes have been made so that setup_revision will now
> update the unknown option in argv
You're writting "Changes have been made", but I did not see any up to
this point in the series.
We write patch series so that they are bisectable, i.e. each commit
should be correct (compileable, pass tests, consistent
documentation, ...). Here, it seems you are introducing a breakage to
repair it later.
Other that bisectability, this makes review harder: at this point the
reader knows it's broken, guesses that it will be repaired later, but
does not know in which patch.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-16 16:59 UTC (permalink / raw)
To: Lars Schneider; +Cc: Jonathan Tan, git, sbeller
In-Reply-To: <D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
>> On 16 Feb 2017, at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> The "git -c <var>=<val> cmd" mechanism is to pretend that a
>
> The problem is also present for gitconfig variables e.g.
> git config --local submodule.UPPERSUB.update none
Yuck.
> But your patch below fixes that, too!
Heh.
> Should we add a test case to this patch?
> If not, do you want me to improve my test case patch [1]
> or do you want to ditch the test?
I think a new test for submodule (although it already exists thanks
t you) is a bit too roundabout way to demonstrate the breakage and
protect the fix.
We'd perhaps want some tests for "git config", probably.
In a repository with /etc/gitconfig and $HOME/ that are tightly
controlled (I think our test framework already do so), running these
would demonstrate both breakages, I would think, but I am sure
people can come up with other forms that are a lot easier to read..
$ git -c v.A.r=val -c v.a.r=ue config --get-all v.a.r
$ git -c v.A.r=val -c v.a.r=ue config --get-all v.A.r
$ git -c v.a.r=val -c v.A.r=ue config --get-all v.a.r
$ git -c v.a.r=val -c v.A.r=ue config --get-all v.A.r
Thanks.
^ permalink raw reply
* Re: [PATCH 1/3] add git_psprintf helper function
From: Jeff King @ 2017-02-16 17:03 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Maxim Moseychuk, git
In-Reply-To: <616a2b08-71f9-d594-d46c-2687bf20ae2b@google.com>
On Thu, Feb 16, 2017 at 07:51:14AM -0800, Jonathan Tan wrote:
> On 02/16/2017 03:28 AM, Maxim Moseychuk wrote:
> > There are a number of places in the code where we call
> > xsnprintf(), with the assumption that the output will fit into
> > the buffer. If the buffer is small, then git die.
> > In many places buffers have compile-time size, but generated string
> > depends from current system locale (gettext)and can have size
> > greater the buffer.
> > Just run "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8"
> > on linux sources - it impossible.
> >
> > git_psprintf is similar to the standard C sprintf() function
> > but safer, since it calculates the maximum space required
> > and allocates memory to hold the result.
> > The returned string should be freed with free() when no longer needed.
>
> If I understand this correctly, xstrfmt (in strbuf.h) should already do what
> you need, so you do not need a new function.
Yes, this is exactly what xstrfmt is for.
-Peff
^ permalink raw reply
* [PATCH V2 0/2] Fix l10n
From: Maxim Moseychuk @ 2017-02-16 17:07 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In some places static size buffers can't store formatted string.
If it be happen then git die.
Jonathan Tan: Thanks a lot for your help.
Maxim Moseychuk (2):
bisect_next_all: convert xsnprintf to xstrfmt
stop_progress_msg: convert xsnprintf to xstrfmt
bisect.c | 9 +++++----
progress.c | 9 +++------
2 files changed, 8 insertions(+), 10 deletions(-)
--
2.11.1
^ permalink raw reply
* [PATCH V2 1/2] bisect_next_all: convert xsnprintf to xstrfmt
From: Maxim Moseychuk @ 2017-02-16 17:07 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In-Reply-To: <20170216170713.10065-1-franchesko.salias.hudro.pedros@gmail.com>
Git can't run bisect between 2048+ commits if use russian translation.
Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
Dummy solution: just increase buffer size but is not safe.
Size gettext string is a runtime value.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
bisect.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/bisect.c b/bisect.c
index 21bc6daa4..787543cad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
- char steps_msg[32];
+ char *steps_msg;
read_bisect_terms(&term_bad, &term_good);
if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
nr = all - reaches - 1;
steps = estimate_bisect_steps(all);
- xsnprintf(steps_msg, sizeof(steps_msg),
- Q_("(roughly %d step)", "(roughly %d steps)", steps),
- steps);
+
+ steps_msg = xstrfmt(Q_("(roughly %d step)", "(roughly %d steps)",
+ steps), steps);
/* TRANSLATORS: the last %s will be replaced with
"(roughly %d steps)" translation */
printf(Q_("Bisecting: %d revision left to test after this %s\n",
"Bisecting: %d revisions left to test after this %s\n",
nr), nr, steps_msg);
+ free(steps_msg);
return bisect_checkout(bisect_rev, no_checkout);
}
--
2.11.1
^ permalink raw reply related
* [PATCH V2 2/2] stop_progress_msg: convert xsnprintf to xstrfmt
From: Maxim Moseychuk @ 2017-02-16 17:07 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In-Reply-To: <20170216170713.10065-1-franchesko.salias.hudro.pedros@gmail.com>
Replace local safe string buffer allocation by xstrfmt.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
progress.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/progress.c b/progress.c
index 76a88c573..29d0dad58 100644
--- a/progress.c
+++ b/progress.c
@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
*p_progress = NULL;
if (progress->last_value != -1) {
/* Force the last update */
- char buf[128], *bufp;
- size_t len = strlen(msg) + 5;
+ char *bufp;
struct throughput *tp = progress->throughput;
- bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
if (tp) {
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
throughput_string(&tp->display, tp->curr_total, rate);
}
progress_update = 1;
- xsnprintf(bufp, len + 1, ", %s.\n", msg);
+ bufp = xstrfmt(", %s.\n", msg);
display(progress, progress->last_value, bufp);
- if (buf != bufp)
- free(bufp);
+ free(bufp);
}
clear_progress_signal();
if (progress->throughput)
--
2.11.1
^ permalink raw reply related
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Schindelin @ 2017-02-16 17:10 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <0275af7b-eb7a-1094-a891-674300175e56@kdbg.org>
Hi Hannes,
On Wed, 15 Feb 2017, Johannes Sixt wrote:
> Am 15.02.2017 um 13:32 schrieb Johannes Schindelin:
> > On Tue, 14 Feb 2017, Johannes Sixt wrote:
> > > Am 14.02.2017 um 15:47 schrieb Johannes Schindelin:
> > > > On Mon, 13 Feb 2017, Junio C Hamano wrote:
> > > > > Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> > > > > > What we forgot was to mark stderr as unbuffered again.
> > >
> > > I do not see how the earlier patch turned stderr from unbuffered to
> > > buffered, as it did not add or remove any setvbuf() call. Can you
> > > explain?
> >
> > [ motivation and history of js/mingw-isatty snipped ]
> >
> > So instead of "bending" the target HANDLE of the existing
> > stdout/stderr (which would *naturally* have kept the
> > buffered/unbuffered nature as-is), we now redirect with correct API
> > calls.
>
> Your statement implies that at the time when winansi_init() begins,
> stdio is already initialized and the buffered/unbuffered state has been
> set for stderr. I would think that this is true.
>
> Then we swap out the file handle underlying stderr in swap_osfhnd()
> using dup2(). Why would that change the buffered state of stdio?
The file handle we swap in for stderr points to the pipe that a
freshly-started thread consumes for parsing the ANSI color sequences. This
handle is used both for stdout and stderr. The dup2() call then implicitly
reopens stderr, with the default buffering.
> > And the patch I provided at the bottom of this mail thread reinstates
> > the unbuffered nature of stderr now that it gets reopened.
> >
> > Hopefully that makes it clear why the setvbuf() call is required now,
> > but was previously unnecessary?
>
> Unfortunately, no. I do not see how dup2() causes a change in stdio state. I
> must be missing something (and that may be a basic misunderstanding of how
> stdio is initialized).
It appears that dup2()ing fd 2 resets that stdio state.
Ciao,
Dscho
^ permalink raw reply
* Re: What's cooking in git.git (Feb 2017, #04; Tue, 14)
From: Junio C Hamano @ 2017-02-16 17:10 UTC (permalink / raw)
To: René Scharfe; +Cc: git
In-Reply-To: <df42ec26-d1b1-e31b-b16d-554441a335f0@web.de>
René Scharfe <l.s.r@web.de> writes:
> Am 14.02.2017 um 23:59 schrieb Junio C Hamano:
>> * rs/ls-files-partial-optim (2017-02-13) 2 commits
>> - ls-files: move only kept cache entries in prune_cache()
>> - ls-files: pass prefix length explicitly to prune_cache()
>>
>> "ls-files" run with pathspec has been micro-optimized to avoid one
>> extra call to memmove().
>
> Nit: The number of memmove(3) calls stays the same, but the number of
> bytes that are moved is reduced.
Blush X-< you are right of course. Will fix to say "... to avoid
having to memmove(3) unnecessary bytes."
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] bisect_next_all: convert xsnprintf to git_psprintf
From: Jeff King @ 2017-02-16 17:14 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git
In-Reply-To: <20170216112829.18079-3-franchesko.salias.hudro.pedros@gmail.com>
On Thu, Feb 16, 2017 at 02:28:28PM +0300, Maxim Moseychuk wrote:
> Git can't run bisect between 2048+ commits if use russian translation.
> Reproduce "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
>
> Dummy solution: just increase buffer size but is not safe.
> Size gettext string is a runtime value.
Hmm. I wondered if this used to work (because xsnprintf operated on a
fixed-size fmt) and was broken in the translation. And as a consequence,
if we needed to be searching for other cases with similar bugs.
But no, in this case the fixed-size buffer was actually introduced
during the i18n step from 14dc4899e (i18n: bisect: mark strings for
translation, 2016-06-17).
I guess the other type of bug could still exist, though.
> diff --git a/bisect.c b/bisect.c
> index 21bc6daa4..8670cc97a 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
> struct commit_list *tried;
> int reaches = 0, all = 0, nr, steps;
> const unsigned char *bisect_rev;
> - char steps_msg[32];
> + char *steps_msg;
So one solution would be to just bump the "32" higher here. The format
comes from the translation, so in practice it only needs to be large
enough to fit any of our translations.
That feels pretty hacky, though. In practice the set of translations is
contained, but it doesn't have to be (and certainly nobody would notice
if a new translation was added with a longer name until a user
complained).
> @@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
>
> nr = all - reaches - 1;
> steps = estimate_bisect_steps(all);
> - xsnprintf(steps_msg, sizeof(steps_msg),
> - Q_("(roughly %d step)", "(roughly %d steps)", steps),
> - steps);
> +
> + steps_msg = git_psprintf(Q_("(roughly %d step)", "(roughly %d steps)",
> + steps), steps);
> /* TRANSLATORS: the last %s will be replaced with
> "(roughly %d steps)" translation */
> printf(Q_("Bisecting: %d revision left to test after this %s\n",
> "Bisecting: %d revisions left to test after this %s\n",
> nr), nr, steps_msg);
> + free(steps_msg);
I wondered if a viable solution would be to make the whole thing one
single translatable string. It would avoid the need for the TRANSLATORS
comment. But I guess we have two "Q_" invocations here, and they might
need to be handled separately (e.g., "2 revisions", "1 step").
I also wondered if we could just make this into two printf statements
("revisions left to test", followed by "roughly N steps"). But the
commit message for 14dc4899e mentions RTL languages. So I think we
really do need to build it up block by block and let the translators
adjust the ordering.
So I think your solution is the best we can do.
It's probably worth outlining these alternatives in the commit message.
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] stop_progress_msg: convert xsnprintf to git_psprintf
From: Jeff King @ 2017-02-16 17:26 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git
In-Reply-To: <20170216112829.18079-4-franchesko.salias.hudro.pedros@gmail.com>
On Thu, Feb 16, 2017 at 02:28:29PM +0300, Maxim Moseychuk wrote:
> Replace local safe string buffer allocation by git_psprintf.
Hmm. Is this one actually broken in practice?
I see:
> @@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
> *p_progress = NULL;
> if (progress->last_value != -1) {
> /* Force the last update */
> - char buf[128], *bufp;
> - size_t len = strlen(msg) + 5;
> + char *bufp;
We have a fixed-size buffer here, but...
> struct throughput *tp = progress->throughput;
>
> - bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
If it's not big enough we allocate a new one. So this works for any size
of translated string. It just tries to skip the allocation in the
"short" case.
If this were in the normal progress code, I might care more about trying
to skip an allocation for performance. But since this is just in
stop_progress_msg(), I don't think the complexity is worth it. I'm
especially happy to see the magic "5" above go away.
So I think this is worth doing, but the motivation is a simplification,
not a bugfix.
> if (tp) {
> unsigned int rate = !tp->avg_misecs ? 0 :
> tp->avg_bytes / tp->avg_misecs;
> throughput_string(&tp->display, tp->curr_total, rate);
> }
> progress_update = 1;
> - xsnprintf(bufp, len + 1, ", %s.\n", msg);
> + bufp = git_psprintf(", %s.\n", msg);
> display(progress, progress->last_value, bufp);
> - if (buf != bufp)
> - free(bufp);
> + free(bufp);
This parts looks good (modulo using xstrfmt). It's probably worth
renaming "bufp" to the more usual "buf", I would think.
I do wonder if the punctuation here will give translators headaches.
E.g., in a RTL language you probably wouldn't want that period at the
end. I don't know enough to say, so I'm willing to punt on that for now.
But I suspect in the long run we may need to just take the whole
translated message, punctuation included, from the caller. There are
only two callers that actually provide a string.
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] Fix l10n
From: Jeff King @ 2017-02-16 17:27 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git
In-Reply-To: <20170216112829.18079-1-franchesko.salias.hudro.pedros@gmail.com>
On Thu, Feb 16, 2017 at 02:28:26PM +0300, Maxim Moseychuk wrote:
> In some places static size buffers can't store formatted string.
> If it be happen then git die.
>
> Maxim Moseychuk (3):
> add git_psprintf helper function
> bisect_next_all: convert xsnprintf to git_psprintf
> stop_progress_msg: convert xsnprintf to git_psprintf
Thanks for providing a series (and I think this is your first series, so
welcome!).
The overall goal looks good, and I dropped a few comments in reply to
the individual patches.
-Peff
^ permalink raw reply
* body-CC-comment regression
From: Johan Hovold @ 2017-02-16 17:49 UTC (permalink / raw)
To: git; +Cc: Jeff King, Matthieu Moy, Kevin Daudt, Junio C Hamano,
Larry Finger
Hi,
I recently noticed that after an upgrade, git-send-email (2.10.2)
started aborting when trying to send patches that had a linux-kernel
stable-tag in its body. For example,
Cc: <stable@vger.kernel.org> # 4.4
was now parsed as
"stable@vger.kernel.org#4.4"
which resulted in
Died at /usr/libexec/git-core/git-send-email line 1332, <FIN> line 1.
This tends to happen in the middle of a series after the cover letter
had been sent just to make things worse...
I finally got around to looking into this today and discovered this
thread ("Re: Formatting problem send_mail in version 2.10.0"):
https://marc.info/?l=git&m=147633706724793&w=2
The problem with the resulting fixes that are now in 2.11.1 is that
git-send-email no longer discards the trailing comment but rather
shoves it into the name after adding some random white space:
"# 3 . 3 . x : 1b9508f : sched : Rate-limit newidle" <stable@vger.kernel.org>"
This example is based on the example from
Documentation/process/stable-kernel-rules.rst:
Cc: <stable@vger.kernel.org> # 3.3.x: 1b9508f: sched: Rate-limit newidle
and this format for stable-tags has been documented at least since 2009
and 8e9b9362266d ("Doc/stable rules: add new cherry-pick logic"), and
has been supported by git since 2012 and 831a488b76e0 ("git-send-email:
remove garbage after email address") I believe.
Can we please revert to the old behaviour of simply discarding such
comments (from body-CC:s) or at least make it configurable through a
configuration option?
Thanks,
Johan
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Johannes Sixt @ 2017-02-16 17:55 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702161753050.3496@virtualbox>
Am 16.02.2017 um 18:10 schrieb Johannes Schindelin:
> On Wed, 15 Feb 2017, Johannes Sixt wrote:
>> I do not see how dup2() causes a change in stdio state. I
>> must be missing something (and that may be a basic misunderstanding of how
>> stdio is initialized).
>
> It appears that dup2()ing fd 2 resets that stdio state.
OK, thanks. It's surprising, but so be it.
Thank you for the quick fix!
-- Hannes
^ 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