* [PATCH 02/10] cmd_merge: convert to single exit point
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
This makes post-processing easier.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/merge.c | 48 +++++++++++++++++++++++++++++-------------------
1 files changed, 29 insertions(+), 19 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index d9c7a80..1be6f3b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
struct commit *head_commit;
struct strbuf buf = STRBUF_INIT;
const char *head_arg;
- int flag, i;
+ int flag, i, ret = 0;
int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1121,7 +1121,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
die(_("There is no merge to abort (MERGE_HEAD missing)."));
/* Invoke 'git reset --merge' */
- return cmd_reset(nargc, nargv, prefix);
+ ret = cmd_reset(nargc, nargv, prefix);
+ goto done;
}
if (read_cache_unmerged())
@@ -1205,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
read_empty(remote_head->sha1, 0);
update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
DIE_ON_ERR);
- return 0;
+ goto done;
} else {
struct strbuf merge_names = STRBUF_INIT;
@@ -1292,7 +1293,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* but first the most common case of merging one remote.
*/
finish_up_to_date("Already up-to-date.");
- return 0;
+ goto done;
} else if (allow_fast_forward && !remoteheads->next &&
!common->next &&
!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
@@ -1313,15 +1314,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
strbuf_addstr(&msg,
" (no commit created; -m option ignored)");
o = want_commit(sha1_to_hex(remoteheads->item->object.sha1));
- if (!o)
- return 1;
-
- if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
- return 1;
+ if (!o ||
+ checkout_fast_forward(head_commit->object.sha1,
+ remoteheads->item->object.sha1)) {
+ ret = 1;
+ goto done;
+ }
finish(head_commit, o->sha1, msg.buf);
drop_save();
- return 0;
+ goto done;
} else if (!remoteheads->next && common->next)
;
/*
@@ -1339,8 +1341,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
git_committer_info(IDENT_ERROR_ON_NO_NAME);
printf(_("Trying really trivial in-index merge...\n"));
if (!read_tree_trivial(common->item->object.sha1,
- head_commit->object.sha1, remoteheads->item->object.sha1))
- return merge_trivial(head_commit);
+ head_commit->object.sha1,
+ remoteheads->item->object.sha1)) {
+ ret = merge_trivial(head_commit);
+ goto done;
+ }
printf(_("Nope.\n"));
}
} else {
@@ -1368,7 +1373,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
}
if (up_to_date) {
finish_up_to_date("Already up-to-date. Yeeah!");
- return 0;
+ goto done;
}
}
@@ -1450,9 +1455,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* If we have a resulting tree, that means the strategy module
* auto resolved the merge cleanly.
*/
- if (automerge_was_ok)
- return finish_automerge(head_commit, common, result_tree,
- wt_strategy);
+ if (automerge_was_ok) {
+ ret = finish_automerge(head_commit, common, result_tree,
+ wt_strategy);
+ goto done;
+ }
/*
* Pick the result from the best strategy and have the user fix
@@ -1466,7 +1473,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
else
fprintf(stderr, _("Merge with strategy %s failed.\n"),
use_strategies[0]->name);
- return 2;
+ ret = 2;
+ goto done;
} else if (best_strategy == wt_strategy)
; /* We already have its result in the working tree. */
else {
@@ -1485,7 +1493,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
if (merge_was_ok) {
fprintf(stderr, _("Automatic merge went well; "
"stopped before committing as requested\n"));
- return 0;
} else
- return suggest_conflicts(option_renormalize);
+ ret = suggest_conflicts(option_renormalize);
+
+done:
+ return ret;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 06/10] reflog-walk.c: request allocated buffer from resolve_ref()
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
for_each_reflog_ent() can do anything, including calling resolve_ref()
again. Therefore it's not safe to use the static buffer returned by
resolve_ref(). Request it to allocate new buffer instead.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
reflog-walk.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/reflog-walk.c b/reflog-walk.c
index f71cca6..00bdff8 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,9 +50,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
for_each_reflog_ent(ref, read_one_reflog, reflogs);
if (reflogs->nr == 0) {
unsigned char sha1[20];
- const char *name = resolve_ref(ref, sha1, 1, NULL, 0);
- if (name)
+ char *name = resolve_ref(ref, sha1, 1, NULL, 1);
+ if (name) {
for_each_reflog_ent(name, read_one_reflog, reflogs);
+ free(name);
+ }
}
if (reflogs->nr == 0) {
int len = strlen(ref);
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 05/10] checkout: do not try xstrdup() on NULL
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
Also free memory in the other exit point
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/checkout.c | 13 +++++++------
1 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/builtin/checkout.c b/builtin/checkout.c
index f92c737..d80e2c4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
{
int ret = 0;
struct branch_info old;
+ char *path;
unsigned char rev[20];
int flag;
memset(&old, 0, sizeof(old));
- old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag, 0));
+ old.path = path = resolve_ref("HEAD", rev, 0, &flag, 1);
old.commit = lookup_commit_reference_gently(rev, 1);
- if (!(flag & REF_ISSYMREF)) {
- free((char *)old.path);
+ if (!(flag & REF_ISSYMREF))
old.path = NULL;
- }
if (old.path && !prefixcmp(old.path, "refs/heads/"))
old.name = old.path + strlen("refs/heads/");
@@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
}
ret = merge_working_tree(opts, &old, new);
- if (ret)
+ if (ret) {
+ free(path);
return ret;
+ }
if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
orphaned_commit_warning(old.commit);
@@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
update_refs_for_switch(opts, &old, new);
ret = post_checkout_hook(old.commit, new->commit, 1);
- free((char *)old.path);
+ free(path);
return ret || opts->writeout_error;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 04/10] commit: move resolve_ref() closer to where the return value is used
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
resolve_ref() returns a static buffer and the value may change if
another resolve_ref() is called. Move resolve_ref() closer to printf()
where the value is used to reduce that chance.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/commit.c | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/builtin/commit.c b/builtin/commit.c
index 11ae005..365b9f6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
struct commit *commit;
struct strbuf format = STRBUF_INIT;
unsigned char junk_sha1[20];
- const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL, 0);
+ const char *head;
struct pretty_print_context pctx = {0};
struct strbuf author_ident = STRBUF_INIT;
struct strbuf committer_ident = STRBUF_INIT;
@@ -1304,6 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
rev.diffopt.break_opt = 0;
diff_setup_done(&rev.diffopt);
+ head = resolve_ref("HEAD", junk_sha1, 0, NULL, 0);
printf("[%s%s ",
!prefixcmp(head, "refs/heads/") ?
head + 11 :
--
1.7.4.74.g639db
^ permalink raw reply related
* [PATCH 03/10] merge: do not point "branch" to a resolve_ref()'s static buffer
From: Nguyễn Thái Ngọc Duy @ 2011-11-15 6:07 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Nguyễn Thái Ngọc Duy
In-Reply-To: <1321337276-17803-1-git-send-email-pclouds@gmail.com>
resolve_ref() may return a pointer to a static buffer. Callers that
use this value longer than a couple of statements should copy the
value to avoid some hidden resolve_ref() call that may change the
static buffer's value.
The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
demonstrates this. The first call is in cmd_merge()
branch = resolve_ref("HEAD", head_sha1, 0, &flag);
Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
may be called again and destroy "branch".
lookup_commit_or_die
lookup_commit_reference
lookup_commit_reference_gently
parse_object
lookup_replace_object
do_lookup_replace_object
prepare_replace_object
for_each_replace_ref
do_for_each_ref
get_loose_refs
get_ref_dir
get_ref_dir
resolve_ref
Ask resolve_ref() to allocate a new buffer instead of returning static
buffer.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
builtin/merge.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/builtin/merge.c b/builtin/merge.c
index 1be6f3b..1a3095f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1087,6 +1087,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
struct commit_list *common = NULL;
const char *best_strategy = NULL, *wt_strategy = NULL;
struct commit_list **remotes = &remoteheads;
+ char *branch_ref;
if (argc == 2 && !strcmp(argv[1], "-h"))
usage_with_options(builtin_merge_usage, builtin_merge_options);
@@ -1095,7 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
* Check if we are _not_ on a detached HEAD, i.e. if there is a
* current branch.
*/
- branch = resolve_ref("HEAD", head_sha1, 0, &flag, 0);
+ branch = branch_ref = resolve_ref("HEAD", head_sha1, 0, &flag, 1);
if (branch && !prefixcmp(branch, "refs/heads/"))
branch += 11;
if (!branch || is_null_sha1(head_sha1))
@@ -1497,5 +1498,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
ret = suggest_conflicts(option_renormalize);
done:
+ free(branch_ref);
return ret;
}
--
1.7.4.74.g639db
^ permalink raw reply related
* Re: [PATCH 2/2] Copy resolve_ref() return value for longer use
From: Nguyen Thai Ngoc Duy @ 2011-11-15 6:06 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20111114112440.GD10847@sigill.intra.peff.net>
How about an incremental approach like this? On top of 1/1.
[PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer
[PATCH 02/10] cmd_merge: convert to single exit point
[PATCH 03/10] merge: do not point "branch" to a resolve_ref()'s static buffer
[PATCH 04/10] commit: move resolve_ref() closer to where the return value is used
[PATCH 05/10] checkout: do not try xstrdup() on NULL
[PATCH 06/10] reflog-walk.c: request allocated buffer from resolve_ref()
[PATCH 07/10] receive-pack: request resolve_ref() to allocate new buffer
[PATCH 08/10] notes: request resolve_ref() to allocate new buffer
[PATCH 09/10] fmt-merge-msg: request resolve_ref() to allocate new buffer
[PATCH 10/10] branch: request resolve_ref() to allocate new buffer
^ permalink raw reply
* [PATCH] Fix "is_refname_available(): query only possibly-conflicting references"
From: mhagger @ 2011-11-15 5:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git, Jeff King, Drew Northup, Jakub Narebski, Heiko Voigt,
Johan Herland, Julian Phillips, Michael Haggerty
In-Reply-To: <1319804921-17545-27-git-send-email-mhagger@alum.mit.edu>
From: Michael Haggerty <mhagger@alum.mit.edu>
The above-named commit didn't do all that its commit message claimed.
The problem is that do_for_each_ref_in_dir() doesn't avoid iterating
through reference subtrees outside of "prefix"; it only skips passing
those references to the callback function. So the function
unnecessarily caused all loose references to be loaded rather than
just those in the required subtree.
So instead, explicitly select the possibly-conflicting subtree and
pass it to do_for_each_ref_in_dir().
Also, simplify name_conflict_fn(). Since it will only be called for
possibly-conflicting references, there is necessarily a conflict if it
is called for *any* reference besides "oldrefname".
Remove function names_conflict(), which is now unused.
Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
This patch can be squashed on top of "is_refname_available(): query
only possibly-conflicting references", or applied at the end of
mh/ref-api-take-2; it does not conflict with the two commits later in
the series.
refs.c | 40 +++++++++++++++++-----------------------
1 files changed, 17 insertions(+), 23 deletions(-)
diff --git a/refs.c b/refs.c
index 6a7f9c3..b185c32 100644
--- a/refs.c
+++ b/refs.c
@@ -633,23 +633,7 @@ static int do_for_each_ref_in_dirs(struct ref_entry *direntry1,
}
}
-/*
- * Return true iff refname1 and refname2 conflict with each other.
- * Two reference names conflict if one of them exactly matches the
- * leading components of the other; e.g., "foo/bar" conflicts with
- * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
- * "foo/barbados".
- */
-static int names_conflict(const char *refname1, const char *refname2)
-{
- for (; *refname1 && *refname1 == *refname2; refname1++, refname2++)
- ;
- return (*refname1 == '\0' && *refname2 == '/')
- || (*refname1 == '/' && *refname2 == '\0');
-}
-
struct name_conflict_cb {
- const char *refname;
const char *oldrefname;
const char *conflicting_refname;
};
@@ -660,11 +644,13 @@ static int name_conflict_fn(const char *existingrefname, const unsigned char *sh
struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data;
if (!strcmp(data->oldrefname, existingrefname))
return 0;
- if (names_conflict(data->refname, existingrefname)) {
- data->conflicting_refname = existingrefname;
- return 1;
- }
- return 0;
+
+ /*
+ * Since we are only iterating over the subtree that has the
+ * new refname as prefix, *any* reference found is a conflict.
+ */
+ data->conflicting_refname = existingrefname;
+ return 1;
}
/*
@@ -673,6 +659,11 @@ static int name_conflict_fn(const char *existingrefname, const unsigned char *sh
* oldrefname is non-NULL, ignore potential conflicts with oldrefname
* (e.g., because oldrefname is scheduled for deletion in the same
* operation).
+ *
+ * Two reference names conflict if one of them exactly matches the
+ * leading components of the other; e.g., "foo/bar" conflicts with
+ * both "foo" and with "foo/bar/baz" but not with "foo/bar" or
+ * "foo/barbados".
*/
static int is_refname_available(const char *refname, const char *oldrefname,
struct ref_entry *direntry)
@@ -706,11 +697,14 @@ static int is_refname_available(const char *refname, const char *oldrefname,
/* Check whether refname is a proper prefix of an existing reference: */
prefix[prefixlen++] = '/';
prefix[prefixlen] = '\0';
- data.refname = refname;
data.oldrefname = oldrefname;
data.conflicting_refname = NULL;
- if (do_for_each_ref_in_dir(direntry, 0, prefix, name_conflict_fn,
+ direntry = find_containing_direntry(direntry, prefix, 0);
+ if (!direntry)
+ return 1;
+
+ if (do_for_each_ref_in_dir(direntry, 0, "", name_conflict_fn,
0, DO_FOR_EACH_INCLUDE_BROKEN,
&data)) {
error("'%s' exists; cannot create '%s'",
--
1.7.7.2
^ permalink raw reply related
* Re: [RFC] deprecating and eventually removing "git relink"?
From: Miles Bader @ 2011-11-15 4:48 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Simon Brenner, git
In-Reply-To: <7vmxbyicgg.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I did not mean "it is wrong because it does not match what Miles said"
> by that. In fact, I think it is a better approach to put things in
> clients first and consolidating possible duplicates at the central one
> purely as optimization, and I do not necessarily see "write to central
> from the beginning" as a particularly good "optimization".
FWIW, this seems reasonable to me...
-Miles
--
Circus, n. A place where horses, ponies and elephants are permitted to see
men, women and children acting the fool.
^ permalink raw reply
* Re: [RFC] deprecating and eventually removing "git relink"?
From: Miles Bader @ 2011-11-15 4:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmxbzj927.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
>> It might be nice to have a mechanism where new objects would update
>> the _alternate_ rather than the object-store in the tree where the
>> command was run.
>
> With the alternate mechanism, your borrowing is read-only and that is
> exactly why you can borrow from other peoples' repositories to which you
> have no write permission to.
>
> What you are suggesting is fundamentally different from the alternates
> mechanism. I am not saying it is better or worse, though. Not yet at this
> point in this message.
Sure, and I don't even claim it's a viable idea, just something that
"seems useful."
>> .. then you could easily have a bunch of trees using a central
>> object store without needing to update the central store
>> occasionally by hand (and do gc in its "clients")...
>
> If you write objects to the central store, "gc" in the "clients"
> will be a no-op because they do not have their own objects. But
> instead, crufts your "clients" accumulate will be in the central
> store. There is still need for "gc" at the central store to remove
> things that are no longer used by any client, isn't it? Unless you
> declare that you do not care because perhaps the central store is
> large enough, that is.
Sure, if git had this mode of operation, it would seem desirable for
"git gc" to act on the central store just at the same points it acts
on the "local store" today.
As obviously a gc needs to know all the roots, that suggests the
central store needs to have a list of clients it can scan for roots.
[I suppose the other "problem" is locking; I guess that would
technically be no different that multiple git commands running
simulataneously in the same tree today, but maybe the presence of a
central store would make such situations occur more frequently...]
> At least with the alternates, running "gc" in the "clients" is a
> safe operation and the only change necessary is to make fsck/repack
> aware of the repositories that borrow from the repository these
> commands are run, and the logic to do so is exactly the same as the
> case to run "gc" in your central store, I would think.
Hmmm sure.
-miles
--
=====
(^o^;
(()))
*This is the cute octopus virus, please copy it into your sig so it can spread.
^ permalink raw reply
* Re: [PATCH 4/4] refresh_index: notice typechanges in output
From: Jeff King @ 2011-11-15 2:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git
In-Reply-To: <7vaa7yi6wv.fsf@alter.siamese.dyndns.org>
On Mon, Nov 14, 2011 at 04:08:32PM -0800, Junio C Hamano wrote:
> I agree that we should not say that an intent-to-add entry has changed
> type relative to whatever, as by definition there is nothing to compare
> against. "A" that stands for "A"dd is a lot more sensible here, I would
> think.
Yeah, that makes sense to me.
> + if (cache_errno == ENOENT)
> + fmt = deleted_fmt;
> + else if (ce->ce_flags & CE_INTENT_TO_ADD)
> + fmt = added_fmt; /* must be before other checks */
Thanks, I was trying to figure out how to tell an intent-to-add file
from the 'changed' flag, but obviously looking for the bit in the cache
entry is the right thing.
Do you want to add your patch on top, or do you want me to re-roll with
this squashed in? I can also hold the re-roll until post-release if you
want.
-Peff
^ permalink raw reply
* Re: [PATCH] documentation fix: git difftool uses diff tools, not merge tools.
From: Junio C Hamano @ 2011-11-15 0:20 UTC (permalink / raw)
To: Thomas Hochstein; +Cc: git, David Aguilar
In-Reply-To: <1321311352-8950-1-git-send-email-thh@inter.net>
Thomas Hochstein <thh@inter.net> writes:
> Let the documentation for -t list valid *diff* tools,
> not valid *merge* tools.
>
> Signed-off-by: Thomas Hochstein <thh@inter.net>
> ---
> Documentation/git-difftool.txt | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
> index a03515f..19d473c 100644
> --- a/Documentation/git-difftool.txt
> +++ b/Documentation/git-difftool.txt
> @@ -31,7 +31,7 @@ OPTIONS
> -t <tool>::
> --tool=<tool>::
> Use the diff tool specified by <tool>.
> - Valid merge tools are:
> + Valid diff tools are:
> araxis, bc3, diffuse, emerge, ecmerge, gvimdiff, kdiff3,
> kompare, meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.
> +
The patch is obviously correct, and it is so trivial that there is no risk
of breaking anything. It would even be OK for 1.7.7.X maintenance series.
But in the longer term, I suspect that we would want to drop this
enumeration from the documentation, and instead give "--list-tools"
option or something to the command. That way, we only need to keep
the list of known tools in one place where it matters, namely, the
command that knows about them.
David, what do you think?
The same comment applies to "git mergetool", I would think.
^ permalink raw reply
* Re: [PATCH 4/4] refresh_index: notice typechanges in output
From: Junio C Hamano @ 2011-11-15 0:08 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Carlos Martín Nieto, git
In-Reply-To: <20111114225651.GD3993@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If a file changes type and a porcelain updates the index, we
> will print "M file". Instead, let's be more specific and
> print "T file", which matches actual diff and status output.
>
> The plumbing version remains "needs update" for historical
> compatibility.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> The "changed" flag comes from refresh_cache_ent, which in turn gets it
> from ie_modified_stat. The one hesitation I have is that intent-to-add
> entries get the TYPE_CHANGED flag set, which means they will get a "T"
> output. Whereas I actually think "M" is a little more sensible.
I agree that we should not say that an intent-to-add entry has changed
type relative to whatever, as by definition there is nothing to compare
against. "A" that stands for "A"dd is a lot more sensible here, I would
think.
read-cache.c | 17 ++++++++++++++---
1 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 0e17add..27e9fc6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1108,11 +1108,13 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
const char *modified_fmt;
const char *deleted_fmt;
const char *typechange_fmt;
+ const char *added_fmt;
const char *unmerged_fmt;
modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
+ added_fmt = (in_porcelain ? "A\t%s\n" : "%s needs update\n");
unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce, *new;
@@ -1142,6 +1144,8 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
if (new == ce)
continue;
if (!new) {
+ const char *fmt;
+
if (not_new && cache_errno == ENOENT)
continue;
if (really && cache_errno == EINVAL) {
@@ -1153,9 +1157,16 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
}
if (quiet)
continue;
- show_file((cache_errno == ENOENT ? deleted_fmt :
- changed & TYPE_CHANGED ? typechange_fmt :
- modified_fmt),
+
+ if (cache_errno == ENOENT)
+ fmt = deleted_fmt;
+ else if (ce->ce_flags & CE_INTENT_TO_ADD)
+ fmt = added_fmt; /* must be before other checks */
+ else if (changed & TYPE_CHANGED)
+ fmt = typechange_fmt;
+ else
+ fmt = modified_fmt;
+ show_file(fmt,
ce->name, in_porcelain, &first, header_msg);
has_errors = 1;
continue;
^ permalink raw reply related
* [PATCH] documentation fix: git difftool uses diff tools, not merge tools.
From: Thomas Hochstein @ 2011-11-14 22:55 UTC (permalink / raw)
To: git; +Cc: Thomas Hochstein
Let the documentation for -t list valid *diff* tools,
not valid *merge* tools.
Signed-off-by: Thomas Hochstein <thh@inter.net>
---
Documentation/git-difftool.txt | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index a03515f..19d473c 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -31,7 +31,7 @@ OPTIONS
-t <tool>::
--tool=<tool>::
Use the diff tool specified by <tool>.
- Valid merge tools are:
+ Valid diff tools are:
araxis, bc3, diffuse, emerge, ecmerge, gvimdiff, kdiff3,
kompare, meld, opendiff, p4merge, tkdiff, vimdiff and xxdiff.
+
--
1.7.2.5
^ permalink raw reply related
* Re: [PATCH, v2] tag: implement --[no-]strip option
From: Junio C Hamano @ 2011-11-14 23:04 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git
In-Reply-To: <20111114223903.GA5751@shutemov.name>
"Kirill A. Shutemov" <kirill@shutemov.name> writes:
>> > @@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
>> >
>> > if (!is_null_sha1(prev))
>> > write_tag_body(fd, prev);
>> > - else
>> > + else if (opt->strip)
>> > write_or_die(fd, _(tag_template), strlen(_(tag_template)));
>>
>> Why are you not writing template when no strip is done? (Not an objection
>> disguised as a rhetorical question, but a question).
>>
>> The user who typed "tag -a v1.2.3 HEAD" that spawns an editor would still
>> find it useful to have commented instructions, once we start filling the
>> template with more useful information that is customized for the
>> situation (e.g. "git show -s --oneline" output), no?
>
> Yes. But in this case commented instructions will not be stripped and they
> will go to the message. I think user will be confused.
>
> We can show show some instructions before spawning the editor. What do
> you think?
My knee-jerk reaction is that it would be worse than what your patch
does. I'd say we'd start from your patch and see how users of 'next'
reacts while the topic is cooking.
^ permalink raw reply
* [PATCH 4/4] refresh_index: notice typechanges in output
From: Jeff King @ 2011-11-14 22:56 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111114225056.GA27370@sigill.intra.peff.net>
If a file changes type and a porcelain updates the index, we
will print "M file". Instead, let's be more specific and
print "T file", which matches actual diff and status output.
The plumbing version remains "needs update" for historical
compatibility.
Signed-off-by: Jeff King <peff@peff.net>
---
The "changed" flag comes from refresh_cache_ent, which in turn gets it
from ie_modified_stat. The one hesitation I have is that intent-to-add
entries get the TYPE_CHANGED flag set, which means they will get a "T"
output. Whereas I actually think "M" is a little more sensible.
For "git reset", I'm not sure if it matters, since resetting the index
will always drop such an entry anyway. But you can see it with:
git add -N file
git add --refresh -v other
read-cache.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 83fb19c..0e17add 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1107,14 +1107,17 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
const char *modified_fmt;
const char *deleted_fmt;
+ const char *typechange_fmt;
const char *unmerged_fmt;
modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
+ typechange_fmt = (in_porcelain ? "T\t%s\n" : "%s needs update\n");
unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce, *new;
int cache_errno = 0;
+ int changed = 0;
ce = istate->cache[i];
if (ignore_submodules && S_ISGITLINK(ce->ce_mode))
@@ -1135,7 +1138,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
continue;
- new = refresh_cache_ent(istate, ce, options, &cache_errno, NULL);
+ new = refresh_cache_ent(istate, ce, options, &cache_errno, &changed);
if (new == ce)
continue;
if (!new) {
@@ -1151,6 +1154,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
if (quiet)
continue;
show_file((cache_errno == ENOENT ? deleted_fmt :
+ changed & TYPE_CHANGED ? typechange_fmt :
modified_fmt),
ce->name, in_porcelain, &first, header_msg);
has_errors = 1;
--
1.7.7.3.8.g38efa
^ permalink raw reply related
* [PATCH 3/4] read-cache: let refresh_cache_ent pass up changed flags
From: Jeff King @ 2011-11-14 22:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111114225056.GA27370@sigill.intra.peff.net>
This will enable refresh_cache to differentiate more cases
of modification (such as typechange) when telling the user
what isn't fresh.
Signed-off-by: Jeff King <peff@peff.net>
---
There's only a single layer of call-stack to modify, and there's only
one other caller, so it turned out to be a pretty minor change.
read-cache.c | 9 ++++++---
1 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 046cf7e..83fb19c 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1001,7 +1001,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
*/
static struct cache_entry *refresh_cache_ent(struct index_state *istate,
struct cache_entry *ce,
- unsigned int options, int *err)
+ unsigned int options, int *err,
+ int *changed_ret)
{
struct stat st;
struct cache_entry *updated;
@@ -1033,6 +1034,8 @@ int add_index_entry(struct index_state *istate, struct cache_entry *ce, int opti
}
changed = ie_match_stat(istate, ce, &st, options);
+ if (changed_ret)
+ *changed_ret = changed;
if (!changed) {
/*
* The path is unchanged. If we were told to ignore
@@ -1132,7 +1135,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
if (pathspec && !match_pathspec(pathspec, ce->name, strlen(ce->name), 0, seen))
continue;
- new = refresh_cache_ent(istate, ce, options, &cache_errno);
+ new = refresh_cache_ent(istate, ce, options, &cache_errno, NULL);
if (new == ce)
continue;
if (!new) {
@@ -1161,7 +1164,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
static struct cache_entry *refresh_cache_entry(struct cache_entry *ce, int really)
{
- return refresh_cache_ent(&the_index, ce, really, NULL);
+ return refresh_cache_ent(&the_index, ce, really, NULL, NULL);
}
static int verify_hdr(struct cache_header *hdr, unsigned long size)
--
1.7.7.3.8.g38efa
^ permalink raw reply related
* [PATCH 2/4] refresh_index: mark deletions in porcelain output
From: Jeff King @ 2011-11-14 22:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111114225056.GA27370@sigill.intra.peff.net>
If you have a deleted file and a porcelain refreshes the
cache, we print:
Unstaged changes after reset:
M file
This is technically correct, in that the file is modified,
but it's friendlier to the user if we further differentiate
the case of a deleted file (especially because this output
looks a lot like "diff --name-status", which would also make
the distinction).
The plumbing version stays as "needs update" for historical
compatibility.
Signed-off-by: Jeff King <peff@peff.net>
---
read-cache.c | 6 +++++-
1 files changed, 5 insertions(+), 1 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index eb3aae3..046cf7e 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1103,9 +1103,11 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
const char *modified_fmt;
+ const char *deleted_fmt;
const char *unmerged_fmt;
modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+ deleted_fmt = (in_porcelain ? "D\t%s\n" : "%s: needs update\n");
unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce, *new;
@@ -1145,7 +1147,9 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
}
if (quiet)
continue;
- show_file(modified_fmt, ce->name, in_porcelain, &first, header_msg);
+ show_file((cache_errno == ENOENT ? deleted_fmt :
+ modified_fmt),
+ ce->name, in_porcelain, &first, header_msg);
has_errors = 1;
continue;
}
--
1.7.7.3.8.g38efa
^ permalink raw reply related
* [PATCH 1/4] refresh_index: rename format variables
From: Jeff King @ 2011-11-14 22:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git
In-Reply-To: <20111114225056.GA27370@sigill.intra.peff.net>
When refreshing the index, for modified (or unmerged) files
we will print "needs update" (or "needs merge") for plumbing,
or a "diff --name-status"-ish line for porcelain.
The variables holding which type of message to show are
named after the plumbing messages. However, as we begin to
differentiate more cases at the porcelain level (with the
plumbing message stayin the same), that naming scheme will
become awkward.
Instead, name the variables after which case we found
(modified or unmerged), not what we will output.
Signed-off-by: Jeff King <peff@peff.net>
---
read-cache.c | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/read-cache.c b/read-cache.c
index 5790a91..eb3aae3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1102,11 +1102,11 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
int first = 1;
int in_porcelain = (flags & REFRESH_IN_PORCELAIN);
unsigned int options = really ? CE_MATCH_IGNORE_VALID : 0;
- const char *needs_update_fmt;
- const char *needs_merge_fmt;
+ const char *modified_fmt;
+ const char *unmerged_fmt;
- needs_update_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
- needs_merge_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
+ modified_fmt = (in_porcelain ? "M\t%s\n" : "%s: needs update\n");
+ unmerged_fmt = (in_porcelain ? "U\t%s\n" : "%s: needs merge\n");
for (i = 0; i < istate->cache_nr; i++) {
struct cache_entry *ce, *new;
int cache_errno = 0;
@@ -1122,7 +1122,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
i--;
if (allow_unmerged)
continue;
- show_file(needs_merge_fmt, ce->name, in_porcelain, &first, header_msg);
+ show_file(unmerged_fmt, ce->name, in_porcelain, &first, header_msg);
has_errors = 1;
continue;
}
@@ -1145,7 +1145,7 @@ int refresh_index(struct index_state *istate, unsigned int flags, const char **p
}
if (quiet)
continue;
- show_file(needs_update_fmt, ce->name, in_porcelain, &first, header_msg);
+ show_file(modified_fmt, ce->name, in_porcelain, &first, header_msg);
has_errors = 1;
continue;
}
--
1.7.7.3.8.g38efa
^ permalink raw reply related
* Re: reset reports file as modified when it's in fact deleted
From: Jeff King @ 2011-11-14 22:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Carlos Martín Nieto, git
In-Reply-To: <7vhb2aqjwu.fsf@alter.siamese.dyndns.org>
On Fri, Nov 11, 2011 at 04:11:29PM -0800, Junio C Hamano wrote:
> > I'm happy to make that change. What do you think of the feature overall?
>
> The "feature" is more or less "Meh" to me; I do not mind differentiating M
> and D there because the necessary information is already there in the
> codepath, but if we were to really do that, then the variables must be
> renamed. We shouldn't name them after "you must do this at the plumbing
> level" like we have done so far, and instead use "the path is in this
> state". This is even more so if we were to further split a single "state"
> that plumbing layer considers the same and gives the same "needs X" to
> different states that Porcelains would give different messages in the
> future.
I admit I don't care all that much either, but it's easy to do, and I
think it is less surprising to users. Patches are below.
> > And should typechanges also be handled here (it's no extra work for git
> > to detect them; we just have to pass the TYPE_CHANGED flag back up the
> > stack)?
>
> As long as "pass ... back up the stack" does not result in too much
> ugliness in the code, I am OK with it.
It ended up pretty simple, but I split it out from the deletion case so
you can see it more clearly (and can drop the latter half of the series
if we don't want it).
[1/4]: refresh_index: rename format variables
[2/4]: refresh_index: mark deletions in porcelain output
[3/4]: read-cache: let refresh_cache_ent pass up changed flags
[4/4]: refresh_index: notice typechanges in output
(If I were sure we wanted the typechange output, I think a more
reasonable ordering would be 1, then 3, then squash 2 and 4 into a
single patch. But see my comment in 4/4).
-Peff
^ permalink raw reply
* Re: [PATCH, v2] tag: implement --[no-]strip option
From: Kirill A. Shutemov @ 2011-11-14 22:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vipmmibx4.fsf@alter.siamese.dyndns.org>
On Mon, Nov 14, 2011 at 02:20:23PM -0800, Junio C Hamano wrote:
> "Kirill A. Shutemov" <kirill@shutemov.name> writes:
>
> > From: "Kirill A. Shutemov" <kirill@shutemov.name>
> >
> > --strip::
> > Remove from tag message lines staring with '#', trailing spaces
> > from every line and empty lines from the beginning and end.
> > Enabled by default. Use --no-strip to overwrite the behaviour.
> >
> > --no-strip is useful if you want to take a tag message as-is, without
> > any stripping.
>
> That is not a commit log message ;-)
Ok, I'll fix.
> > Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
>
> > diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> > index c83cb13..dbb76a6 100644
> > --- a/Documentation/git-tag.txt
> > +++ b/Documentation/git-tag.txt
> > @@ -99,6 +99,11 @@ OPTIONS
> > Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
> > is given.
> >
> > +--strip::
> > + Remove from tag message lines staring with '#', trailing spaces
> > + from every line and empty lines from the beginning and end.
> > + Enabled by default. Use --no-strip to overwrite the behaviour.
>
> s/overwrite/override/; or replace the last sentence with "With
> `--no-strip`, the tag message given by the user is used as-is".
Ok.
> > diff --git a/builtin/tag.c b/builtin/tag.c
> > index 9b6fd95..05a1fd4 100644
> > --- a/builtin/tag.c
> > +++ b/builtin/tag.c
> > ...
> > @@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
> >
> > if (!is_null_sha1(prev))
> > write_tag_body(fd, prev);
> > - else
> > + else if (opt->strip)
> > write_or_die(fd, _(tag_template), strlen(_(tag_template)));
>
> Why are you not writing template when no strip is done? (Not an objection
> disguised as a rhetorical question, but a question).
>
> The user who typed "tag -a v1.2.3 HEAD" that spawns an editor would still
> find it useful to have commented instructions, once we start filling the
> template with more useful information that is customized for the
> situation (e.g. "git show -s --oneline" output), no?
Yes. But in this case commented instructions will not be stripped and they
will go to the message. I think user will be confused.
We can show show some instructions before spawning the editor. What do
you think?
> > @@ -423,8 +430,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> > const char *object_ref, *tag;
> > struct ref_lock *lock;
> >
> > - int annotate = 0, sign = 0, force = 0, lines = -1,
> > - list = 0, delete = 0, verify = 0;
> > + struct create_tag_options opt = {
> > + .sign = 0,
> > + .strip = 1,
> > + };
>
> Avoid doing this. Even though these C99 initializers are nicer and more
> readable way to write this, we try to be gentle to people with older
> compilers that do not grok the syntax.
It's sad. Do you have a list of compilers which are important for the
project?
> Except for the above minor nits, the patch basically looks good. Please
> hold onto it and resubmit after 1.7.8 final ships.
Thanks.
--
Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH, v2] tag: implement --[no-]strip option
From: Junio C Hamano @ 2011-11-14 22:20 UTC (permalink / raw)
To: Kirill A. Shutemov; +Cc: git
In-Reply-To: <1321307019-5557-1-git-send-email-kirill@shutemov.name>
"Kirill A. Shutemov" <kirill@shutemov.name> writes:
> From: "Kirill A. Shutemov" <kirill@shutemov.name>
>
> --strip::
> Remove from tag message lines staring with '#', trailing spaces
> from every line and empty lines from the beginning and end.
> Enabled by default. Use --no-strip to overwrite the behaviour.
>
> --no-strip is useful if you want to take a tag message as-is, without
> any stripping.
That is not a commit log message ;-)
> Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index c83cb13..dbb76a6 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -99,6 +99,11 @@ OPTIONS
> Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
> is given.
>
> +--strip::
> + Remove from tag message lines staring with '#', trailing spaces
> + from every line and empty lines from the beginning and end.
> + Enabled by default. Use --no-strip to overwrite the behaviour.
s/overwrite/override/; or replace the last sentence with "With
`--no-strip`, the tag message given by the user is used as-is".
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 9b6fd95..05a1fd4 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> ...
> @@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
>
> if (!is_null_sha1(prev))
> write_tag_body(fd, prev);
> - else
> + else if (opt->strip)
> write_or_die(fd, _(tag_template), strlen(_(tag_template)));
Why are you not writing template when no strip is done? (Not an objection
disguised as a rhetorical question, but a question).
The user who typed "tag -a v1.2.3 HEAD" that spawns an editor would still
find it useful to have commented instructions, once we start filling the
template with more useful information that is customized for the
situation (e.g. "git show -s --oneline" output), no?
> @@ -423,8 +430,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
> const char *object_ref, *tag;
> struct ref_lock *lock;
>
> - int annotate = 0, sign = 0, force = 0, lines = -1,
> - list = 0, delete = 0, verify = 0;
> + struct create_tag_options opt = {
> + .sign = 0,
> + .strip = 1,
> + };
Avoid doing this. Even though these C99 initializers are nicer and more
readable way to write this, we try to be gentle to people with older
compilers that do not grok the syntax.
Except for the above minor nits, the patch basically looks good. Please
hold onto it and resubmit after 1.7.8 final ships.
Thanks.
^ permalink raw reply
* Re: [RFC] deprecating and eventually removing "git relink"?
From: Junio C Hamano @ 2011-11-14 22:08 UTC (permalink / raw)
To: Jeff King; +Cc: Simon Brenner, git
In-Reply-To: <20111114202522.GA26269@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Nov 14, 2011 at 12:18:25PM -0800, Junio C Hamano wrote:
>
>> > Yes, I think that is sensible. I'm not sure there is even any core git
>> > code to be written. I think a wrapper that does the following would
>> > probably work:
>>
>> I agree with your outline, which I find is in line with what I had in mind
>> in the message Miles responded.
>>
>> The approach is different from what Miles alluded to, which is to have
>> "clients" create objects in the "central" place in the first place,
>> though.
>
> It seems to me that is simply an optimization that can come later.
I did not mean "it is wrong because it does not match what Miles said" by
that. In fact, I think it is a better approach to put things in clients
first and consolidating possible duplicates at the central one purely as
optimization, and I do not necessarily see "write to central from the
beginning" as a particularly good "optimization".
^ permalink raw reply
* Re: Compile warnings
From: Frans Klaver @ 2011-11-14 21:55 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git@vger.kernel.org, Junio C Hamano
In-Reply-To: <CACBZZX6d_ykc9CbN7H-ACWNUxACb9+TH4ffJ-+9T=SEv6Ai0Ug@mail.gmail.com>
On Mon, 14 Nov 2011 17:58:20 +0100, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Nov 14, 2011 at 15:55, Frans Klaver <fransklaver@gmail.com>
> wrote:
>> Every now and then I see an 'unused result' warning come by during
>> building.
>> What is the general attitude towards these warnings? Remove them (by
>> properly checking)? Or leave them be as a kind of documentation -- we
>> know
>> we're ignoring the info, but it's good to be reminded?
>
> Under what OS / version and compiler / version and what's the warning?
> Paste the full warning(s) you get verbatim.
This question was triggered by
warning: ignoring return value of ‘fwrite’, declared with attribute
warn_unused_result
appearing in diff.c, graph.c, grep.c and several others. I'm using gentoo
linux, gcc 4.5.3.
So the specific question would be, do these fwrites need to be checked?
^ permalink raw reply
* Re: [PATCH] svn: Create config options for common args
From: Eric Wong @ 2011-11-14 21:46 UTC (permalink / raw)
To: Ted Percival; +Cc: git
In-Reply-To: <1321302310-28793-1-git-send-email-ted.percival@quest.com>
Ted Percival <ted.percival@quest.com> wrote:
> Documentation/git-svn.txt | 10 ++++++++++
Thanks, documentation part is very much appreciated
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -21,6 +21,9 @@ $Git::SVN::default_repo_id = 'svn';
> $Git::SVN::default_ref_id = $ENV{GIT_SVN_ID} || 'git-svn';
> $Git::SVN::Ra::_log_window_size = 100;
> $Git::SVN::_minimize_url = 'unset';
> +$Git::SVN::_localtime = Git::config_bool('svn.localtime');
> +$Git::SVN::_add_author_from = Git::config_bool('svn.addAuthorFrom');
> +$Git::SVN::_use_log_author = Git::config_bool('svn.useLogAuthor');
I don't think these are needed. The read_git_config() function /should/
be checking all GIT_CONFIG equivalents of the Getopt::Long option specs.
Can you verify? Thanks.
^ permalink raw reply
* [PATCH, v2] tag: implement --[no-]strip option
From: Kirill A. Shutemov @ 2011-11-14 21:43 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Kirill A. Shutemov
From: "Kirill A. Shutemov" <kirill@shutemov.name>
--strip::
Remove from tag message lines staring with '#', trailing spaces
from every line and empty lines from the beginning and end.
Enabled by default. Use --no-strip to overwrite the behaviour.
--no-strip is useful if you want to take a tag message as-is, without
any stripping.
Signed-off-by: Kirill A. Shutemov <kirill@shutemov.name>
---
Documentation/git-tag.txt | 5 +++++
builtin/tag.c | 41 ++++++++++++++++++++++++++++-------------
2 files changed, 33 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index c83cb13..dbb76a6 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -99,6 +99,11 @@ OPTIONS
Implies `-a` if none of `-a`, `-s`, or `-u <key-id>`
is given.
+--strip::
+ Remove from tag message lines staring with '#', trailing spaces
+ from every line and empty lines from the beginning and end.
+ Enabled by default. Use --no-strip to overwrite the behaviour.
+
<tagname>::
The name of the tag to create, delete, or describe.
The new tag name must pass all checks defined by
diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..05a1fd4 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -319,8 +319,14 @@ static int build_tag_object(struct strbuf *buf, int sign, unsigned char *result)
return 0;
}
+struct create_tag_options {
+ unsigned int message;
+ unsigned int sign;
+ unsigned int strip;
+};
+
static void create_tag(const unsigned char *object, const char *tag,
- struct strbuf *buf, int message, int sign,
+ struct strbuf *buf, struct create_tag_options *opt,
unsigned char *prev, unsigned char *result)
{
enum object_type type;
@@ -345,7 +351,7 @@ static void create_tag(const unsigned char *object, const char *tag,
if (header_len > sizeof(header_buf) - 1)
die(_("tag header too big."));
- if (!message) {
+ if (!opt->message) {
int fd;
/* write the template message before editing: */
@@ -356,7 +362,7 @@ static void create_tag(const unsigned char *object, const char *tag,
if (!is_null_sha1(prev))
write_tag_body(fd, prev);
- else
+ else if (opt->strip)
write_or_die(fd, _(tag_template), strlen(_(tag_template)));
close(fd);
@@ -367,14 +373,15 @@ static void create_tag(const unsigned char *object, const char *tag,
}
}
- stripspace(buf, 1);
+ if (opt->strip)
+ stripspace(buf, 1);
- if (!message && !buf->len)
+ if (opt->message && !buf->len)
die(_("no tag message?"));
strbuf_insert(buf, 0, header_buf, header_len);
- if (build_tag_object(buf, sign, result) < 0) {
+ if (build_tag_object(buf, opt->sign, result) < 0) {
if (path)
fprintf(stderr, _("The tag message has been left in %s\n"),
path);
@@ -423,8 +430,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
const char *object_ref, *tag;
struct ref_lock *lock;
- int annotate = 0, sign = 0, force = 0, lines = -1,
- list = 0, delete = 0, verify = 0;
+ struct create_tag_options opt = {
+ .sign = 0,
+ .strip = 1,
+ };
+
+ int annotate = 0, force = 0, lines = -1, list = 0,
+ delete = 0, verify = 0;
const char *msgfile = NULL, *keyid = NULL;
struct msg_arg msg = { 0, STRBUF_INIT };
struct commit_list *with_commit = NULL;
@@ -442,7 +454,9 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
OPT_CALLBACK('m', "message", &msg, "message",
"tag message", parse_msg_arg),
OPT_FILENAME('F', "file", &msgfile, "read message from file"),
- OPT_BOOLEAN('s', "sign", &sign, "annotated and GPG-signed tag"),
+ OPT_BOOLEAN('s', "sign", &opt.sign, "annotated and GPG-signed tag"),
+ OPT_BOOLEAN(0, "strip", &opt.strip,
+ "turn off tag message stripping"),
OPT_STRING('u', "local-user", &keyid, "key-id",
"use another key to sign the tag"),
OPT__FORCE(&force, "replace the tag if exists"),
@@ -462,10 +476,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, options, git_tag_usage, 0);
if (keyid) {
- sign = 1;
+ opt.sign = 1;
set_signingkey(keyid);
}
- if (sign)
+ if (opt.sign)
annotate = 1;
if (argc == 0 && !(delete || verify))
list = 1;
@@ -523,9 +537,10 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
else if (!force)
die(_("tag '%s' already exists"), tag);
+ opt.message = msg.given || msgfile;
+
if (annotate)
- create_tag(object, tag, &buf, msg.given || msgfile,
- sign, prev, object);
+ create_tag(object, tag, &buf, &opt, prev, object);
lock = lock_any_ref_for_update(ref.buf, prev, 0);
if (!lock)
--
1.7.7.2
^ permalink raw reply related
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