* [PATCH v3 08/19] builtin/clone: convert to struct object_id
From: brian m. carlson @ 2017-02-18 0:06 UTC (permalink / raw)
To: git; +Cc: Jeff King, Michael Haggerty, Junio C Hamano
In-Reply-To: <20170218000652.375129-1-sandals@crustytoothpaste.net>
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
builtin/clone.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/builtin/clone.c b/builtin/clone.c
index 3f63edbbf9..b4c929bb8a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -681,7 +681,7 @@ static void update_head(const struct ref *our, const struct ref *remote,
static int checkout(int submodule_progress)
{
- unsigned char sha1[20];
+ struct object_id oid;
char *head;
struct lock_file *lock_file;
struct unpack_trees_options opts;
@@ -692,7 +692,7 @@ static int checkout(int submodule_progress)
if (option_no_checkout)
return 0;
- head = resolve_refdup("HEAD", RESOLVE_REF_READING, sha1, NULL);
+ head = resolve_refdup("HEAD", RESOLVE_REF_READING, oid.hash, NULL);
if (!head) {
warning(_("remote HEAD refers to nonexistent ref, "
"unable to checkout.\n"));
@@ -700,7 +700,7 @@ static int checkout(int submodule_progress)
}
if (!strcmp(head, "HEAD")) {
if (advice_detached_head)
- detach_advice(sha1_to_hex(sha1));
+ detach_advice(oid_to_hex(&oid));
} else {
if (!starts_with(head, "refs/heads/"))
die(_("HEAD not found below refs/heads!"));
@@ -721,7 +721,7 @@ static int checkout(int submodule_progress)
opts.src_index = &the_index;
opts.dst_index = &the_index;
- tree = parse_tree_indirect(sha1);
+ tree = parse_tree_indirect(oid.hash);
parse_tree(tree);
init_tree_desc(&t, tree->buffer, tree->size);
if (unpack_trees(1, &t, &opts) < 0)
@@ -731,7 +731,7 @@ static int checkout(int submodule_progress)
die(_("unable to write new index file"));
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
- sha1_to_hex(sha1), "1", NULL);
+ oid_to_hex(&oid), "1", NULL);
if (!err && option_recursive) {
struct argv_array args = ARGV_ARRAY_INIT;
--
2.11.0
^ permalink raw reply related
* [PATCH v3 00/19] object_id part 6
From: brian m. carlson @ 2017-02-18 0:06 UTC (permalink / raw)
To: git; +Cc: Jeff King, Michael Haggerty, Junio C Hamano
This is another series in the continuing conversion to struct object_id.
This series converts more of the builtin directory and some of the refs
code to use struct object_id. Additionally, it implements an
nth_packed_object_oid function which provides a struct object_id version
of the nth_packed_object function, and a parse_oid_hex function that
makes parsing easier.
The patch to use parse_oid_hex in the refs code has been split out into
its own patch, just because I'm wary of that code and potentially
breaking things, and I want it to be easy to revert in case things go
wrong. I have no reason to believe it is anything other than fully
functional, however.
The conflicts in pu with v2 are probably still present, but I expect the
resolution should be easy (and I suspect Junio uses git rerere, so they'll be
even easier).
Changes from v2:
* Fix misnamed function in commit message.
* Improve parameter name of parse_oid_hex.
* Improve docstring of parse_oid_hex.
* Remove needless variable.
* Rebase on master.
Changes from v1:
* Implement parse_oid_hex and use it.
* Make nth_packed_object_oid take a variable into which to store the
object ID. This avoids concerns about unsafe casts.
* Rebase on master.
brian m. carlson (19):
builtin/commit: convert to struct object_id
builtin/diff-tree: convert to struct object_id
builtin/describe: convert to struct object_id
builtin/fast-export: convert to struct object_id
builtin/fmt-merge-message: convert to struct object_id
builtin/grep: convert to struct object_id
builtin/branch: convert to struct object_id
builtin/clone: convert to struct object_id
builtin/merge: convert to struct object_id
Convert remaining callers of resolve_refdup to object_id
builtin/replace: convert to struct object_id
reflog-walk: convert struct reflog_info to struct object_id
refs: convert each_reflog_ent_fn to struct object_id
hex: introduce parse_oid_hex
refs: simplify parsing of reflog entries
sha1_file: introduce an nth_packed_object_oid function
Convert object iteration callbacks to struct object_id
builtin/merge-base: convert to struct object_id
wt-status: convert to struct object_id
builtin/branch.c | 26 +++++-----
builtin/cat-file.c | 8 +--
builtin/clone.c | 10 ++--
builtin/commit.c | 46 ++++++++---------
builtin/count-objects.c | 4 +-
builtin/describe.c | 50 +++++++++---------
builtin/diff-tree.c | 38 +++++++-------
builtin/fast-export.c | 58 ++++++++++-----------
builtin/fmt-merge-msg.c | 70 ++++++++++++-------------
builtin/fsck.c | 40 +++++++--------
builtin/grep.c | 24 ++++-----
builtin/merge-base.c | 30 +++++------
builtin/merge.c | 134 ++++++++++++++++++++++++------------------------
builtin/notes.c | 18 +++----
builtin/pack-objects.c | 6 +--
builtin/prune-packed.c | 4 +-
builtin/prune.c | 8 +--
builtin/receive-pack.c | 4 +-
builtin/reflog.c | 2 +-
builtin/replace.c | 112 ++++++++++++++++++++--------------------
cache.h | 19 ++++++-
hex.c | 8 +++
reachable.c | 30 +++++------
ref-filter.c | 4 +-
reflog-walk.c | 26 +++++-----
refs.c | 24 ++++-----
refs.h | 2 +-
refs/files-backend.c | 29 ++++++-----
revision.c | 12 ++---
sha1_file.c | 27 +++++++---
sha1_name.c | 2 +-
transport.c | 4 +-
wt-status.c | 52 +++++++++----------
33 files changed, 483 insertions(+), 448 deletions(-)
--
2.11.0
^ permalink raw reply
* [PATCH v3 02/19] builtin/diff-tree: convert to struct object_id
From: brian m. carlson @ 2017-02-18 0:06 UTC (permalink / raw)
To: git; +Cc: Jeff King, Michael Haggerty, Junio C Hamano
In-Reply-To: <20170218000652.375129-1-sandals@crustytoothpaste.net>
Convert most leaf functions to struct object_id. Rewrite several
hardcoded numbers in terms of GIT_SHA1_HEXSZ, using an intermediate
variable where that makes sense.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
builtin/diff-tree.c | 38 ++++++++++++++++++++------------------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 8ce00480cd..1f1573bb2a 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -7,9 +7,9 @@
static struct rev_info log_tree_opt;
-static int diff_tree_commit_sha1(const unsigned char *sha1)
+static int diff_tree_commit_sha1(const struct object_id *oid)
{
- struct commit *commit = lookup_commit_reference(sha1);
+ struct commit *commit = lookup_commit_reference(oid->hash);
if (!commit)
return -1;
return log_tree_commit(&log_tree_opt, commit);
@@ -18,22 +18,22 @@ static int diff_tree_commit_sha1(const unsigned char *sha1)
/* Diff one or more commits. */
static int stdin_diff_commit(struct commit *commit, char *line, int len)
{
- unsigned char sha1[20];
- if (isspace(line[40]) && !get_sha1_hex(line+41, sha1)) {
+ struct object_id oid;
+ if (isspace(line[GIT_SHA1_HEXSZ]) && !get_oid_hex(line+GIT_SHA1_HEXSZ+1, &oid)) {
/* Graft the fake parents locally to the commit */
- int pos = 41;
+ int pos = GIT_SHA1_HEXSZ + 1;
struct commit_list **pptr;
/* Free the real parent list */
free_commit_list(commit->parents);
commit->parents = NULL;
pptr = &(commit->parents);
- while (line[pos] && !get_sha1_hex(line + pos, sha1)) {
- struct commit *parent = lookup_commit(sha1);
+ while (line[pos] && !get_oid_hex(line + pos, &oid)) {
+ struct commit *parent = lookup_commit(oid.hash);
if (parent) {
pptr = &commit_list_insert(parent, pptr)->next;
}
- pos += 41;
+ pos += GIT_SHA1_HEXSZ + 1;
}
}
return log_tree_commit(&log_tree_opt, commit);
@@ -42,11 +42,13 @@ static int stdin_diff_commit(struct commit *commit, char *line, int len)
/* Diff two trees. */
static int stdin_diff_trees(struct tree *tree1, char *line, int len)
{
- unsigned char sha1[20];
+ struct object_id oid;
struct tree *tree2;
- if (len != 82 || !isspace(line[40]) || get_sha1_hex(line + 41, sha1))
+ const int chunksz = GIT_SHA1_HEXSZ + 1;
+ if (len != 2 * chunksz || !isspace(line[chunksz-1]) ||
+ get_sha1_hex(line + chunksz, oid.hash))
return error("Need exactly two trees, separated by a space");
- tree2 = lookup_tree(sha1);
+ tree2 = lookup_tree(oid.hash);
if (!tree2 || parse_tree(tree2))
return -1;
printf("%s %s\n", oid_to_hex(&tree1->object.oid),
@@ -60,15 +62,15 @@ static int stdin_diff_trees(struct tree *tree1, char *line, int len)
static int diff_tree_stdin(char *line)
{
int len = strlen(line);
- unsigned char sha1[20];
+ struct object_id oid;
struct object *obj;
if (!len || line[len-1] != '\n')
return -1;
line[len-1] = 0;
- if (get_sha1_hex(line, sha1))
+ if (get_oid_hex(line, &oid))
return -1;
- obj = parse_object(sha1);
+ obj = parse_object(oid.hash);
if (!obj)
return -1;
if (obj->type == OBJ_COMMIT)
@@ -76,7 +78,7 @@ static int diff_tree_stdin(char *line)
if (obj->type == OBJ_TREE)
return stdin_diff_trees((struct tree *)obj, line, len);
error("Object %s is a %s, not a commit or tree",
- sha1_to_hex(sha1), typename(obj->type));
+ oid_to_hex(&oid), typename(obj->type));
return -1;
}
@@ -141,7 +143,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
break;
case 1:
tree1 = opt->pending.objects[0].item;
- diff_tree_commit_sha1(tree1->oid.hash);
+ diff_tree_commit_sha1(&tree1->oid);
break;
case 2:
tree1 = opt->pending.objects[0].item;
@@ -164,9 +166,9 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)
opt->diffopt.setup |= (DIFF_SETUP_USE_SIZE_CACHE |
DIFF_SETUP_USE_CACHE);
while (fgets(line, sizeof(line), stdin)) {
- unsigned char sha1[20];
+ struct object_id oid;
- if (get_sha1_hex(line, sha1)) {
+ if (get_oid_hex(line, &oid)) {
fputs(line, stdout);
fflush(stdout);
}
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Junio C Hamano @ 2017-02-17 23:52 UTC (permalink / raw)
To: Jeff King
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <20170217233952.56h2z3l76orn4zht@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I'm also not sure that it's all that useful to distinguish errors from
> fwrite() versus fclose(). In practice, errors will come from write() in
> either case, and the caller does not have much control over when the
> flushing happens. So any error saying "error closing file" is probably
> assuming too much anyway. It should be "error writing file".
Yes.
>> We _could_ clear errno to allow caller to tell them apart, though,
>> if we wanted to ;-)
>
> Hmm. So basically "errno = 0" instead of EIO? That at least has the
> advantage that you can tell it apart from a real EIO, and a caller
> _could_ if they chose do:
>
> if (commit_lock_file(&lock)) {
> if (!errno)
> error("error writing to file");
> else
> error_errno("error closing file");
> }
Exactly.
> But I am not sure I would want to annotate all such callers that way. It
> would probably be less bad to just pass down a "quiet" flag or a strbuf
> and have the low-level code fill it in. And that solves this problem
> _and_ the rename() thing above.
>
> But TBH, I am not sure if it is worth it. Nobody is complaining. This is
> just a thing we noticed. I think setting errno to EIO or to "0" is a
> strict improvement over what is there (where the errno is essentially
> random) and the complexity doesn't escape the function.
>
> So I think that "easy" thing falls far short of a solution, but it's at
> least easy. I could take it or leave it at this point.
Well, I already said that earlier in this thread, and ended up
queuing your patch on 'pu' ;-).
^ permalink raw reply
* Re: [PATCH] Document dotfiles exclusion on template copy
From: Junio C Hamano @ 2017-02-17 23:59 UTC (permalink / raw)
To: Grégoire Paris; +Cc: git, Grégoire Paris
In-Reply-To: <20170217233700.14643-1-postmaster@greg0ire.fr>
Thanks, will queue.
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 23:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <xmqqr32wnznf.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 17, 2017 at 03:52:36PM -0800, Junio C Hamano wrote:
> > So I think that "easy" thing falls far short of a solution, but it's at
> > least easy. I could take it or leave it at this point.
>
> Well, I already said that earlier in this thread, and ended up
> queuing your patch on 'pu' ;-).
We're slowly converging on consensus in our apathy. ;)
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Junio C Hamano @ 2017-02-17 23:53 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Jeff King, git
In-Reply-To: <871suwqtar.fsf@kyleam.com>
Kyle Meyer <kyle@kyleam.com> writes:
> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
>> Do we even want these "internal" delete_ref() invocations to be
>> logged in HEAD's reflog? I understand that this is inside the
>> implementation of renaming an old ref to a new ref, and reflog
>> message given to delete_ref() would matter only if the HEAD happens
>> to be pointing at old ref---but then HEAD will be repointed to the
>> new ref by somebody else [*1*] that called this function to rename
>> old to new and it _will_ log it.
>
> I know the discussion has developed further, but just a note that I
> think the last statement is inaccurate: currently, a rename will not be
> recorded in HEAD's log. "git branch -m" will show a renaming event in
> the new branch's log, but the only trace of the event in HEAD's log is
> the deletion entry with an empty message.
Yes, I think Peff diagnosed it and suggested a fix.
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Jeff King @ 2017-02-17 23:53 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <871suwqtar.fsf@kyleam.com>
On Fri, Feb 17, 2017 at 06:41:32PM -0500, Kyle Meyer wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> [...]
>
> > Do we even want these "internal" delete_ref() invocations to be
> > logged in HEAD's reflog? I understand that this is inside the
> > implementation of renaming an old ref to a new ref, and reflog
> > message given to delete_ref() would matter only if the HEAD happens
> > to be pointing at old ref---but then HEAD will be repointed to the
> > new ref by somebody else [*1*] that called this function to rename
> > old to new and it _will_ log it.
>
> I know the discussion has developed further, but just a note that I
> think the last statement is inaccurate: currently, a rename will not be
> recorded in HEAD's log. "git branch -m" will show a renaming event in
> the new branch's log, but the only trace of the event in HEAD's log is
> the deletion entry with an empty message.
Right. I assumed Junio was talking about the hypothetical behavior we'd
want.
Your response did make me think of one other option: if we updated HEAD
_before_ writing the new ref, then it would automatically get the
"create" half of the rename. IOW, if the rename process were:
1. delete old; this writes a reflog to HEAD, per the magic
HEAD-detection in commit_ref_update().
2. update HEAD to point to new
3. re-create new; this uses the same magic to write a HEAD reflog.
That's probably a crazy path to go, though. Right now steps (1) and (3)
happen in a low-level function, and step (2) happens outside of it
completely. Arguably it would be good to change that, but I think we
want to think of (1) and (3) as an atomic operation. Putting more things
which might fail between them is a bad idea.
So I think your existing patches (modulo the review comments), plus the
"something like this" that I sent on top are probably a better end
state.
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: make stderr unbuffered again
From: Junio C Hamano @ 2017-02-17 23:49 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Johannes Sixt, Jeff Hostetler
In-Reply-To: <alpine.DEB.2.20.1702171626160.3496@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ... there is a different problem here: you stated very explicitly
> that it is okay for `pu` to be broken [*1*]. If it were different, if any
> breakage would imply that a fix is really, really required lest the patch
> series be evicted from `pu`, we could easily modify my Continuous Testing
> setup to report more visibly what is broken.
I think you misread me.
Regarding 'pu', there is a chicken-and-egg problem involved. Most
of the days, the tip of 'pu' passes the test at least for me.
At the end of day's integration cycle, I may find that 'pu' does not
pass the test for me [*1*]. It is not unusual to see new multiple
topics come to 'pu', each of which may test well in isolation but
have hidden interactions with topics already in 'next' or 'pu' that
are exposed only when merged to 'pu', and it may be too late in the
day for me to find which one of these new topics is problematic.
I could choose not to merge any of them and push 'pu' out with no
new topics. Or I may choose to push it out anyway, so that other
people who are working during the remainder of 24-hour in different
timezones can notice and find which new topic is broken [*2*].
And that is why I explicitly say that 'pu' may not even build. It
shouldn't be taken as a discouragement to people from looking at it,
which seems to be how I read you to misinterpret it. It is the
opposite---a breakage at the tip of 'pu' is an invitation for people
to help.
If nobody steps up and says "the tip of 'pu' does not build and that
is because of this topic", I'd be irritated enough to find which
topic is broken myself and then ask the author of the offencing
topic for help. If the topic is left broken after that, it will be
ejected from 'pu', because I cannot use 'pu' for helping to polish
other new topics that wants to cook there unless I do so.
Of course there are platform- or environment-dependent breakages
that would not cause my tests to fail [*3*]. Again, people with
different environment can step up to help, if these topics are
included in 'pu'. You would recall that you called out one topic
[*4*] that did not build in your environment and the topic was
ejected after your message (but not before---there wasn't a way for
Window-less folks to tell that it was broken IIRC) from 'pu'.
In a near-by thread, somebody wondered if we can add an integration
branch 'pr' that is beyond 'pu' and includes everything ever posted
to the list, and I explained why I won't have time for that. But I
think the spirit of suggested 'pr' is what 'pu' already is---it is a
collection of promising topics that can be merged into a seemingly
consistent whole, which may or may not build and the purpose of the
branch is to contain them in one place so that people can find which
ones need unbreaking. Testing the tip of it alone and complaining
that it is broken does not help much, but figuiring out which topic
merged to it is broken does, by unblocking other topics in 'pu'.
[Footnote]
*1* If any other integration branch (including 'jch', which is
somewhere between 'master' and 'pu' and beyond the commit with a
tree that matches 'next' aka "pu^{/^### match next}") does not
build, I pull overtime ;-)
*2* When there is only a new topic that breaks 'pu' for me, it is
easy to decide not to queue it and send a note to the author of
the offending topic, and that does happen a lot, but not always.
*3* I do not have p4 and I may not have svn installed so my tests
may not even cover them.
*4* I think it was nd/worktree-move but may have been another topic.
^ permalink raw reply
* Re: [PATCH v5 3/6] stash: refactor stash_create
From: Jeff King @ 2017-02-17 23:48 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Junio C Hamano, Johannes Schindelin, Øyvind A . Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170217224141.19183-4-t.gummerer@gmail.com>
On Fri, Feb 17, 2017 at 10:41:38PM +0000, Thomas Gummerer wrote:
> Refactor the internal stash_create function to use a -m flag for
> specifying the message and -u flag to indicate whether untracked files
> should be added to the stash.
>
> This makes it easier to pass a pathspec argument to stash_create in the
> next patch.
>
> The user interface for git stash create stays the same.
Sounds good, but...
> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 2e9cef06e6..d93c47446a 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -17,6 +17,7 @@ SYNOPSIS
> [-u|--include-untracked] [-a|--all] [<message>]]
> 'git stash' clear
> 'git stash' create [<message>]
> +'git stash' create [-m <message>] [-u|--include-untracked <untracked|all>]
> 'git stash' store [-m|--message <message>] [-q|--quiet] <commit>
Should this hunk be dropped from the manpage, then?
I think there is a similar one in the next patch that adds the
"pathspec" argument, and should be dropped, too.
-Peff
^ permalink raw reply
* Re: [PATCH v5 6/6] stash: allow pathspecs in the no verb form
From: Jeff King @ 2017-02-17 23:46 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Junio C Hamano, Johannes Schindelin, Øyvind A . Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170217224141.19183-7-t.gummerer@gmail.com>
On Fri, Feb 17, 2017 at 10:41:41PM +0000, Thomas Gummerer wrote:
> Now that stash_push is used in the no verb form of stash, allow
> specifying the command line for this form as well. Always use -- to
> disambiguate pathspecs from other non-option arguments.
I think that makes sense.
> Also make git stash -p an alias for git stash push -p. This allows
> users to use git stash -p <pathspec>.
And I think of all the options we discussed for handling "-p", I think
this one makes the most sense.
It may be worth calling out in the documentation that this is how it
works though, so people do not think that:
git stash -k -p <path>
would work ("git stash -k -p" _does_ happen to work due to the old
options-only rule, but I think we should advertise the general form as
"-p is an alias for "push -p").
-Peff
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Kyle Meyer @ 2017-02-17 23:42 UTC (permalink / raw)
To: Jeff King, Junio C Hamano; +Cc: git
In-Reply-To: <20170217195549.z6uyy7hbbhj5avh7@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Feb 17, 2017 at 02:43:50PM -0500, Jeff King wrote:
>
>> Yes. I think the options are basically (in order of decreasing
>> preference in my opinion):
>>
>> 1. Log a rename entry (same sha1, but note the rename in the free-form
>> text).
>>
>> 2. Log a delete (sha1 goes to null) followed by a creation (from null
>> back to the original sha1).
>>
>> 3. Log nothing at all for HEAD.
>>
>> This does half of (2). If we do the second half, then I'd prefer it to
>> (3). But if we can do (1), that is better still (IMHO).
[...]
>> I'm actually confused about which bit of code is updating HEAD. I do not
>> see it either in files_rename_ref() or in the caller. Yet it clearly
>> happens. But that is the code that would know enough to do (1) or the
>> second half of (2) above.
>
> Ah, I found it. It's in replace_each_worktree_head_symref() these days,
> which does not bother to pass a log message.
>
> So I think the second half of (2) is probably something like the patch
> below.
>
> Thinking on it more, we probably _do_ want two entries. Because the
> operations are not atomic, it's possible that we may end up in a
> half-way state after the first entry is written. And when debugging such
> a case, I'd much rather see the first half of the operation logged than
> nothing at all.
OK, I'll have a go at replacing patch 3 with this approach.
--
Kyle
^ permalink raw reply
* Re: [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Kyle Meyer @ 2017-02-17 23:41 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: git
In-Reply-To: <xmqqk28ou2o1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
[...]
> Do we even want these "internal" delete_ref() invocations to be
> logged in HEAD's reflog? I understand that this is inside the
> implementation of renaming an old ref to a new ref, and reflog
> message given to delete_ref() would matter only if the HEAD happens
> to be pointing at old ref---but then HEAD will be repointed to the
> new ref by somebody else [*1*] that called this function to rename
> old to new and it _will_ log it.
I know the discussion has developed further, but just a note that I
think the last statement is inaccurate: currently, a rename will not be
recorded in HEAD's log. "git branch -m" will show a renaming event in
the new branch's log, but the only trace of the event in HEAD's log is
the deletion entry with an empty message.
--
Kyle
^ permalink raw reply
* Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Jeff King @ 2017-02-17 23:41 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <8737fcqtcj.fsf@kyleam.com>
On Fri, Feb 17, 2017 at 06:40:28PM -0500, Kyle Meyer wrote:
> > For reference, the two things I notice are:
> >
> > - we prefer test_path_is_missing to "! test -f" these days.
> >
> > - we don't redirect the output of grep (it's handled already in
> > non-verbose mode, and in verbose mode we try to be...verbose).
>
> Would moving cleanup like "rm -f .git/$m" within the test's body using
> test_when_finished also be preferred?
Yeah, that too. I forgot to mention it.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Kyle Meyer @ 2017-02-17 23:40 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
[...]
>> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
>> index b0ffc0b57..65918d984 100755
>> --- a/t/t1400-update-ref.sh
>> +++ b/t/t1400-update-ref.sh
>> @@ -85,6 +85,15 @@ test_expect_success "delete $m (by HEAD)" '
>> '
>> rm -f .git/$m
>>
>> +test_expect_success "deleting current branch adds message to HEAD's log" '
>> + git update-ref $m $A &&
>> + git symbolic-ref HEAD $m &&
>> + git update-ref -mdelmsg -d $m &&
>> + ! test -f .git/$m &&
>> + grep "delmsg$" .git/logs/HEAD >/dev/null
>> +'
>> +rm -f .git/$m
>
> I think covering this with a test is good.
>
> I don't know if it's also worth testing that deleting via HEAD also
> writes the reflog. I.e.,:
>
> git update-ref -m delete-by-head -d HEAD
Seems reasonable to cover this case as well.
> Some of the style here is a bit out-dated, but I think you are just
> matching the surrounding tests. So that's OK by me (though a patch to
> modernize the whole thing would be welcome, too).
Right. I'd be happy to follow up with a patch updating the style in
t1400-update-ref.sh.
> For reference, the two things I notice are:
>
> - we prefer test_path_is_missing to "! test -f" these days.
>
> - we don't redirect the output of grep (it's handled already in
> non-verbose mode, and in verbose mode we try to be...verbose).
Would moving cleanup like "rm -f .git/$m" within the test's body using
test_when_finished also be preferred?
--
Kyle
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 23:39 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael Haggerty, Andreas Schwab, Jáchym Barvínek, git
In-Reply-To: <xmqqzihkphkc.fsf@gitster.mtv.corp.google.com>
On Fri, Feb 17, 2017 at 02:40:19PM -0800, Junio C Hamano wrote:
> > Right. EIO is almost certainly _not_ the error we saw. But I would
> > rather consistently say "I/O error" and have the user scratch their
> > head, look up this thread, and say "ah, it was probably a deferred
> > error", as opposed to the alternative: the user sees something
> > nonsensical like ENOMEM or EBADF. Those are more misleading, and worse,
> > may change from run to run based on what other code runs or fails in
> > between.
>
> My point was actually not what errno we feed to strerror(). In that
> example, what is more misleading is the fixed part of the error
> message the caller of close_tempfile() used after seeing the funcion
> fail, i.e. "failed to close". strerror() part is used to explain
> why we "failed to close", and of course any nonsensical errno that
> we did not get from the failed stdio call would not explain it, but
> a more misleading part is that we did not even "failed to close" it.
>
> We just noticed an earlier error while attempting to close it.
> strerror() in the message does not even have to be related to the
> closing of the file handle.
Ah, I see. I think the errno thing is a strict improvement over what is
there now. Actually having a separate error message is even better, but
it does end up rather verbose in the callers.
I'm also not sure that it's all that useful to distinguish errors from
fwrite() versus fclose(). In practice, errors will come from write() in
either case, and the caller does not have much control over when the
flushing happens. So any error saying "error closing file" is probably
assuming too much anyway. It should be "error writing file".
And I think in practice the messages end up quite generic anyway, as
they are really calling commit_lock_file(), which may also fail due to a
rename. So you get something like "unable to write 'foo': ", with errno
appended. That's _also_ potentially confusing when rename() fails.
Solving that would probably require passing down an "err" strbuf (or
other data structure) for the low-level code to fill in.
> > The only reason I do not think we should do so for close_tempfile() is
> > that the fclose is typically far away from the code that actually calls
> > error(). We'd have to pass the tristate (success, fail, fail-with-errno)
> > state up through the stack (most of the calls indirectly come from
> > commit_lock_file(), I would think).
>
> We _could_ clear errno to allow caller to tell them apart, though,
> if we wanted to ;-)
Hmm. So basically "errno = 0" instead of EIO? That at least has the
advantage that you can tell it apart from a real EIO, and a caller
_could_ if they chose do:
if (commit_lock_file(&lock)) {
if (!errno)
error("error writing to file");
else
error_errno("error closing file");
}
But I am not sure I would want to annotate all such callers that way. It
would probably be less bad to just pass down a "quiet" flag or a strbuf
and have the low-level code fill it in. And that solves this problem
_and_ the rename() thing above.
But TBH, I am not sure if it is worth it. Nobody is complaining. This is
just a thing we noticed. I think setting errno to EIO or to "0" is a
strict improvement over what is there (where the errno is essentially
random) and the complexity doesn't escape the function.
So I think that "easy" thing falls far short of a solution, but it's at
least easy. I could take it or leave it at this point.
-Peff
^ permalink raw reply
* Re: dotfiles in git template dir are not copied
From: Grégoire PARIS @ 2017-02-17 23:39 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqvas8phdl.fsf@gitster.mtv.corp.google.com>
>There was no 'bug' either. It's just the way it is ;-)
Sure !
^ permalink raw reply
* Re: [PATCH 1/3] delete_refs(): accept a reflog message argument
From: Kyle Meyer @ 2017-02-17 23:35 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: git
In-Reply-To: <xmqqshncu4r4.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>>> diff --git a/refs.h b/refs.h
>>> index 9fbff90e7..81627a63d 100644
>>> --- a/refs.h
>>> +++ b/refs.h
>>> @@ -277,7 +277,7 @@ int reflog_exists(const char *refname);
>>> * be NULL_SHA1. flags is passed through to ref_transaction_delete().
>>> */
>>> int delete_ref(const char *refname, const unsigned char *old_sha1,
>>> - unsigned int flags);
>>> + unsigned int flags, const char *msg);
>>
>> Should the "msg" argument go at the beginning, to match update_ref()?
>
> Probably. rename/create have the message at the end but their
> parameters are very different from update/delete. The parameters
> update and delete take are not identical, but we can view them as a
> lot more similar than the other two. So I think it makes sense for
> delete to try matching update, even though trying to make all four
> the same may proabably be pointless.
I put "msg" after "flags" because that's where it occurs in
ref_transaction_delete(), but matching update_ref() makes sense.
--
Kyle
^ permalink raw reply
* [PATCH] Document dotfiles exclusion on template copy
From: Grégoire Paris @ 2017-02-17 23:37 UTC (permalink / raw)
To: git; +Cc: Grégoire Paris
Since there is no dotfile in the default template directory, there was
no point in making the check for . or .. more accurate when copying. Now
that you can customize the template directory, it would make sense, but
it's actually a good thing to at this because you would not want to have
your git directory copied in every git directory that is created should
you decide to put your template directory under version control. Plus, it
might be used as a feature by people who would want to exclude some
files.
Signed-off-by: Grégoire Paris <postmaster@greg0ire.fr>
---
Here is a better version.
Documentation/git-init.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 9d27197..3c5a67f 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -116,8 +116,8 @@ does not exist, it will be created.
TEMPLATE DIRECTORY
------------------
-The template directory contains files and directories that will be copied to
-the `$GIT_DIR` after it is created.
+Files and directories in the template directory whose name do not start with a
+dot will be copied to the `$GIT_DIR` after it is created.
The template directory will be one of the following (in order):
--
2.9.3
^ permalink raw reply related
* Re: [PATCH] Document dotfiles exclusion on template copy
From: Grégoire PARIS @ 2017-02-17 23:32 UTC (permalink / raw)
To: Grégoire Paris, git
In-Reply-To: <20170217232748.13164-1-postmaster@greg0ire.fr>
>
> -The template directory contains files and directories that will be copied to
> -the `$GIT_DIR` after it is created.
> +The template directory contains files and directories whose name do not start
> +with a dot will be copied to the `$GIT_DIR` after it is created.
>
> The template directory will be one of the following (in order):
>
This sentence does not make any sense, I realize that now.
^ permalink raw reply
* Re: dotfiles in git template dir are not copied
From: Grégoire PARIS @ 2017-02-17 23:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <32094790-1c2b-bc2a-bfde-a101b18945d7@greg0ire.fr>
> Thanks for your guidance. I believe I just sent the patch to the
mailing list.
I just retried with the Gmail SMTP, and now I am sure it worked. Sorry
for the noobery.
^ permalink raw reply
* [PATCH] Document dotfiles exclusion on template copy
From: Grégoire Paris @ 2017-02-17 23:27 UTC (permalink / raw)
To: git; +Cc: Grégoire Paris
Since there is no dotfile in the default template directory, there was
no point in making the check for . or .. more accurate when copying. Now
that you can customize the template directory, it would make sense, but
it's actually a good thing to at this because you would not want to have
your git directory copied in every git directory that is created should
you decide to put your template directory under version control. Plus, it
might be used as a feature by people who would want to exclude some
files.
Signed-off-by: Grégoire Paris <postmaster@greg0ire.fr>
---
I hope I'm getting it right, this is my first time using format-patch and
send-email…
Documentation/git-init.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-init.txt b/Documentation/git-init.txt
index 9d27197..7605742 100644
--- a/Documentation/git-init.txt
+++ b/Documentation/git-init.txt
@@ -116,8 +116,8 @@ does not exist, it will be created.
TEMPLATE DIRECTORY
------------------
-The template directory contains files and directories that will be copied to
-the `$GIT_DIR` after it is created.
+The template directory contains files and directories whose name do not start
+with a dot will be copied to the `$GIT_DIR` after it is created.
The template directory will be one of the following (in order):
--
2.9.3
^ permalink raw reply related
* Re: dotfiles in git template dir are not copied
From: Grégoire PARIS @ 2017-02-17 23:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git
In-Reply-To: <xmqqk28opgsy.fsf@gitster.mtv.corp.google.com>
Thanks for your guidance. I believe I just sent the patch to the mailing
list.
^ permalink raw reply
* Re: Git bisect does not find commit introducing the bug
From: Stephan Beyer @ 2017-02-17 23:21 UTC (permalink / raw)
To: Alex Hoffman, git
In-Reply-To: <CAMX8fZWe2HO78ySonHX0adtPUxVPbj5_vo-NUGrjwpb7gPdGrQ@mail.gmail.com>
Hi,
On 02/17/2017 11:29 PM, Alex Hoffman wrote:
> According to the documentation "git bisect" is designed "to find the
> commit that introduced a bug" .
> I have found a situation in which it does not returns the commit I expected.
> In order to reproduce the problem:
For the others who are too lazy to clone your repo and run the scripts,
the history is like that (read from bottom to top) and I marked the
commit found by git bisect and the on you expected:
* 7a9e952 (bisect bad) <BAD>
|\
| * 671cec2 <BAD> <--- expected
| |\
| * | 04c6f4b <BAD> <--- found
* | | 3915157 <GOOD>
|\ \ \
| | |/
| |/|
| * | f4154e9 (bisect good) <GOOD>
| * | 85855bf <BAD>
| |/
* | f1a36f5 <BAD>
|/
* 1b7fb88 <BAD>
The <BAD> and <GOOD> markers are set by your definition of what good and
what bad commits are.
> First of all this is confusing, as this commit cannot be reached
> starting from "v.good".
Hm, IMHO it shows that your example is pretty artificial (although you
might have come across it in a real-world scenario): you introduced a
new feature in f4154e9 (and it worked) and you broke that feature by
making the merge 671cec2. However, the feature (that broke in 671cec2)
did not even exist in 04c6f4b; so a test on the feature would not fail
(leading to "bisect bad" as in the example), it would not exist (leading
to "bisect skip").
And if we are not talking about passing or failing tests but about
crashing, bisect finds the right thing: f4154e9 was not crashing, but
04c6f4b is crashing. Yes, it's not the commit that introduced the crash
(which would be the first commit in the repo) but it's the first
crashing commit after the one marked as good.
So I'd consider this a feature or rather correct behavior, not a bug.
In other words: bisect assumes that your repo is usually in a good state
and you have a commit that changes it to a bad state. In your case you
have a repo that is in a bad state and you have a commit that switches
it to a good state and later you merge a bad-state branch and you have a
bad state again. It is not made for that use-case, I think.
Cheers
Stephan
^ permalink raw reply
* Re: [PATCH v5 0/6] stash: support pathspec argument
From: Junio C Hamano @ 2017-02-17 23:14 UTC (permalink / raw)
To: Thomas Gummerer
Cc: git, Jeff King, Johannes Schindelin, Øyvind A . Holm,
Jakub Narębski, Matthieu Moy
In-Reply-To: <20170217231246.GI652@hank>
Thomas Gummerer <t.gummerer@gmail.com> writes:
> On 02/17, Junio C Hamano wrote:
>> Thomas Gummerer <t.gummerer@gmail.com> writes:
>>
>> [some people may see this message twice, as I forgot to check if the
>> copy I received had "Some A . Body" not enclosed in dq; blindly
>> doing "Reply-All" ended up listing an invalid address on my Cc: line
>> and dropped by vger. apologies]
>>
>> > diff --git a/git-stash.sh b/git-stash.sh
>> > index a184b1e274..1446fbe2e8 100755
>> > --- a/git-stash.sh
>> > +++ b/git-stash.sh
>> > @@ -67,51 +67,20 @@ create_stash () {
>> > case "$1" in
>> > -m|--message)
>> > shift
>> > - test -z ${1+x} && usage
>> > - stash_msg="$1"
>> > - new_style=t
>> > + stash_msg=${1-"BUG: create_stash () -m requires an argument"}
>> > ;;
>>
>> Did you mean ${1?"BUG: ..."} here and also "-u" below?
>
> Yeah, shell scripts are still confusing me sometimes. Thanks for
> catching. Would you mind fixing this up while queuing or do you want
> me to resend?
I'll fix it up myself (please remind me if you notice that I forgot ;-).
^ 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