* Re: [BUG] submodule config does not apply to upper case submodules?
From: Jeff King @ 2017-02-16 23:22 UTC (permalink / raw)
To: Junio C Hamano
Cc: Stefan Beller, Jonathan Tan, Lars Schneider, git@vger.kernel.org
In-Reply-To: <xmqqh93v10vy.fsf@gitster.mtv.corp.google.com>
On Wed, Feb 15, 2017 at 03:37:37PM -0800, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> > Yes; though I'd place it in strbuf.{c,h} as it is operating
> > on the internals of the strbuf. (Do we make any promises outside of
> > strbuf about the internals? I mean we use .buf all the time, so maybe
> > I am overly cautious here)
>
> I'd rather have it not use struct strbuf as an interface. It only
> needs to pass "char *" and its promise that it touches the string
> in-place without changing the length need to be documented as a
> comment before the function.
This code also uses the hacky strbuf_split() interface. It would be nice
to one day move off of it (the only other strbuf-specific function used
there is strbuf_trim).
One _could_ actually parse the whole thing left-to-right (soaking up
whitespace and doing the canonicalizing) instead of dealing with a split
function at all. But the canonicalize bit you added here would not be
reusable then. And it's probably not worth holding up the bugfix here.
> >> +static void canonicalize_config_variable_name(struct strbuf *var)
> >> +{
> >> + char *first_dot = strchr(var->buf, '.');
> >> + char *last_dot = strrchr(var->buf, '.');
> >
> > If first_dot != NULL, then last_dot !+ NULL as well.
> > (either both are NULL or none of them),
> > so we can loose one condition below.
>
> I do not think it is worth it, though.
If you really want to be picky, you do not need to find the first dot
at all. You can downcase everything until you see a dot, and then
find the last dot (if any) from there.
I don't think it matters much in practice.
-Peff
^ permalink raw reply
* Re: [RFCv4 PATCH 00/14] Checkout aware of Submodules!
From: Jacob Keller @ 2017-02-16 23:27 UTC (permalink / raw)
To: Stefan Beller
Cc: Junio C Hamano, Git mailing list, brian m. carlson,
Jonathan Nieder, Brandon Williams
In-Reply-To: <CAGZ79kYQf2SL-RVETv8=6NaZHhub2kQmKevK32X_xfFkr_0yyA@mail.gmail.com>
On Thu, Feb 16, 2017 at 2:56 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Feb 16, 2017 at 2:54 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Thu, Feb 16, 2017 at 2:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> Integrate updating the submodules into git checkout,...
>>>
>>> It was more or less a pleasant read, once I decided to pretend that
>>> I were a machine who uses identifiers only to identify locations in
>>> the program ;-) IOW, for human consumption, the new names introduced
>>> were sometimes quite confusing and went against helping understanding.
>>>
>>
>> Based on my cursory reading, I agree. I had trouble understanding how
>> the process worked because the names were somewhat confusing. They
>> started to make some sense as I read more. I think v4 had better names
>> than v3, but they were still somewhat confusing to me.
>>
>
> Now if only you could tell me what names were better to understand. ;)
> I'll reply to the individual patch remarks and hopefully there we find
> good names for these functions.
>
> Thanks,
> Stefan
I'll try to read it again and see if I think of anything.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Jeff King @ 2017-02-16 23:27 UTC (permalink / raw)
To: Lars Schneider; +Cc: Junio C Hamano, Jonathan Tan, git, sbeller
In-Reply-To: <D0CDD1AC-05CA-47F3-8CB5-61EA1C6515A8@gmail.com>
On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:
>
> > On 16 Feb 2017, at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
> >
> > The "git -c <var>=<val> cmd" mechanism is to pretend that a
>
> The problem is also present for gitconfig variables e.g.
> git config --local submodule.UPPERSUB.update none
Hrm, is it?
$ git config --file foo submodule.UPPERSUB.update none
$ cat foo
[submodule "UPPERSUB"]
update = none
I could believe that some of the submodule code may try to pass it
through "-c", though, so certain config ends up being missed.
AFAICT, though, the writing code takes what you gave it verbatim. The
reader is responsible for downcasing everything but the subsection
before it hands it to a callback. Commands calling git-config for lookup
should generally ask for the canonical downcased name. There is some
code to downcase, but IIRC there are corner cases around some of the
regex lookup functions.
-Peff
^ permalink raw reply
* [PATCH v3 0/2] Fix l10n
From: Maxim Moseychuk @ 2017-02-16 23:32 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In some places fixed-size buffers can't store formatted string.
If it be happen then git die.
Jonathan Tan, Jeff King thanks a lot for your help.
This is really my first patches. Your help is invaluable.
Maxim Moseychuk (2):
bisect_next_all(): fix bisect crash when used the gettext translation
stop_progress_msg(): simplification function
bisect.c | 9 +++++----
progress.c | 11 ++++-------
2 files changed, 9 insertions(+), 11 deletions(-)
--
2.11.1
^ permalink raw reply
* [PATCH v3 1/2] bisect_next_all(): fix bisect crash when used the gettext translation
From: Maxim Moseychuk @ 2017-02-16 23:32 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In-Reply-To: <20170216233249.24757-1-franchesko.salias.hudro.pedros@gmail.com>
The buffer steps_msg[32] is too small.
Translated "(roughly %d step)" string can not be located in the buffer.
Solution: using xstrfmt which dynamically allocates memory.
Dummy solution: just increase steps_msg size but is not safe.
That feels pretty hacky, though. In practice the set of translations is
contained, but it doesn't have to be (and certainly nobody would notice
if a new translation was added with a longer name until a user
complained).
Reproduce bug: "LANG=ru_RU.UTF8 git bisect start v4.9 v4.8" on linux sources.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
bisect.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/bisect.c b/bisect.c
index 21bc6daa4..787543cad 100644
--- a/bisect.c
+++ b/bisect.c
@@ -940,7 +940,7 @@ int bisect_next_all(const char *prefix, int no_checkout)
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
const unsigned char *bisect_rev;
- char steps_msg[32];
+ char *steps_msg;
read_bisect_terms(&term_bad, &term_good);
if (read_bisect_refs())
@@ -990,14 +990,15 @@ int bisect_next_all(const char *prefix, int no_checkout)
nr = all - reaches - 1;
steps = estimate_bisect_steps(all);
- xsnprintf(steps_msg, sizeof(steps_msg),
- Q_("(roughly %d step)", "(roughly %d steps)", steps),
- steps);
+
+ steps_msg = xstrfmt(Q_("(roughly %d step)", "(roughly %d steps)",
+ steps), steps);
/* TRANSLATORS: the last %s will be replaced with
"(roughly %d steps)" translation */
printf(Q_("Bisecting: %d revision left to test after this %s\n",
"Bisecting: %d revisions left to test after this %s\n",
nr), nr, steps_msg);
+ free(steps_msg);
return bisect_checkout(bisect_rev, no_checkout);
}
--
2.11.1
^ permalink raw reply related
* [PATCH v3 2/2] stop_progress_msg(): simplification function
From: Maxim Moseychuk @ 2017-02-16 23:32 UTC (permalink / raw)
To: git; +Cc: peff, jonathantanmy, Maxim Moseychuk
In-Reply-To: <20170216233249.24757-1-franchesko.salias.hudro.pedros@gmail.com>
stop_progress_msg() is rarely used and is not demanding to
performance. Use dynamically allocates memory.
Signed-off-by: Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com>
---
progress.c | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/progress.c b/progress.c
index 76a88c573..29378caa0 100644
--- a/progress.c
+++ b/progress.c
@@ -243,21 +243,18 @@ void stop_progress_msg(struct progress **p_progress, const char *msg)
*p_progress = NULL;
if (progress->last_value != -1) {
/* Force the last update */
- char buf[128], *bufp;
- size_t len = strlen(msg) + 5;
+ char *buf;
struct throughput *tp = progress->throughput;
- bufp = (len < sizeof(buf)) ? buf : xmallocz(len);
if (tp) {
unsigned int rate = !tp->avg_misecs ? 0 :
tp->avg_bytes / tp->avg_misecs;
throughput_string(&tp->display, tp->curr_total, rate);
}
progress_update = 1;
- xsnprintf(bufp, len + 1, ", %s.\n", msg);
- display(progress, progress->last_value, bufp);
- if (buf != bufp)
- free(bufp);
+ buf = xstrfmt(", %s.\n", msg);
+ display(progress, progress->last_value, buf);
+ free(buf);
}
clear_progress_signal();
if (progress->throughput)
--
2.11.1
^ permalink raw reply related
* Re: [PATCH V2 0/2] Fix l10n
From: Junio C Hamano @ 2017-02-16 23:33 UTC (permalink / raw)
To: Maxim Moseychuk; +Cc: git, peff, jonathantanmy
In-Reply-To: <20170216170713.10065-1-franchesko.salias.hudro.pedros@gmail.com>
Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com> writes:
> In some places static size buffers can't store formatted string.
> If it be happen then git die.
>
> Jonathan Tan: Thanks a lot for your help.
>
> Maxim Moseychuk (2):
> bisect_next_all: convert xsnprintf to xstrfmt
> stop_progress_msg: convert xsnprintf to xstrfmt
>
> bisect.c | 9 +++++----
> progress.c | 9 +++------
> 2 files changed, 8 insertions(+), 10 deletions(-)
>
> --
> 2.11.1
Thanks. Queued with minor log message fixes and pushed out.
^ permalink raw reply
* Re: [PATCH V2 0/2] Fix l10n
From: Unknown @ 2017-02-16 23:37 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, peff, jonathantanmy
In-Reply-To: <xmqq8tp5vhgk.fsf@gitster.mtv.corp.google.com>
В Чт, 16/02/2017 в 15:33 -0800, Junio C Hamano пишет:
> Maxim Moseychuk <franchesko.salias.hudro.pedros@gmail.com> writes:
>
> > In some places static size buffers can't store formatted string.
> > If it be happen then git die.
> >
> > Jonathan Tan: Thanks a lot for your help.
> >
> > Maxim Moseychuk (2):
> > bisect_next_all: convert xsnprintf to xstrfmt
> > stop_progress_msg: convert xsnprintf to xstrfmt
> >
> > bisect.c | 9 +++++----
> > progress.c | 9 +++------
> > 2 files changed, 8 insertions(+), 10 deletions(-)
> >
> > --
> > 2.11.1
>
> Thanks. Queued with minor log message fixes and pushed out.
If is not too late, please check the version 3.
^ permalink raw reply
* Re: [PATCH V2 0/2] Fix l10n
From: Junio C Hamano @ 2017-02-17 1:16 UTC (permalink / raw)
To: MM; +Cc: git, peff, jonathantanmy
In-Reply-To: <1487288237.25073.1.camel@gmail.com>
Unknown <franchesko.salias.hudro.pedros@gmail.com> writes:
> В Чт, 16/02/2017 в 15:33 -0800, Junio C Hamano пишет:
>>
>> Thanks. Queued with minor log message fixes and pushed out.
>
> If is not too late, please check the version 3.
I already squashed in the change s/bufp/buf/ in the version I queued
and pushed out on 'pu', so we should be good. I compared the result
of applying your updated patches and did not see any other changes.
Thanks.
^ permalink raw reply
* Re: [PATCH] config: preserve <subsection> case for one-shot config on the command line
From: Junio C Hamano @ 2017-02-17 1:25 UTC (permalink / raw)
To: Jeff King; +Cc: Lars Schneider, Jonathan Tan, git, sbeller
In-Reply-To: <20170216232730.xsx3xks5ppjws5rg@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Thu, Feb 16, 2017 at 11:30:28AM +0100, Lars Schneider wrote:
>
>>
>> > On 16 Feb 2017, at 00:48, Junio C Hamano <gitster@pobox.com> wrote:
>> >
>> > The "git -c <var>=<val> cmd" mechanism is to pretend that a
>>
>> The problem is also present for gitconfig variables e.g.
>> git config --local submodule.UPPERSUB.update none
>
> Hrm, is it?
>
> $ git config --file foo submodule.UPPERSUB.update none
> $ cat foo
> [submodule "UPPERSUB"]
> update = none
>
> I could believe that some of the submodule code may try to pass it
> through "-c", though, so certain config ends up being missed.
You are right.
The builtin/config.c::get_value() codepath, when it is not using the
hacky regexp interface, uses config.c::git_config_parse_key(), and
it does the right thing. git_config_parse_parameter(), which is the
broken one we found, is not used.
When I did the patch in response to Jonathan's observation, I did
briefly wonder if it should be using git_config_parse_key() instead
of doing its own thing, but I didn't follow it up fully before
deciding that it is the quickest to replace the tolower thing. If I
at least tried to see if it is feasible, I would have noticed that
the query from the command line wouldn't share the same problem as
Lars reported.
I still haven't queued any of the variants I posted (and I do not
think other people sent their own versions, either). I need to pick
one and queue, with a test or two. Perhaps after -rc2.
Others are welcome to work on it while I cut -rc2 tomorrow, so that
by the time I see their patch all that is left for me to do is to
apply it ;-)
^ permalink raw reply
* `git show --oneline commit` does not do what's intuitively expected
From: Luna Kid @ 2017-02-17 1:51 UTC (permalink / raw)
To: git
Hello,
tl;dr; --> Please add --no-diff (or --msg-only) to git show. We'll
love you for that. :)
Expanded:
(I've been a casual git user for 2-3 years; "casual" means using a
small subset, nowadays daily, with some less trivial things at times,
like per-commit patching for post-hoc auto-versioning, or
push-deploying (the old way), so not 100% newbie; doing SW for 25+
years. I'm telling this, because I'm probably a representative sample
of a huge number of experienced professionals, who are relatively new
to git (compared to their age...), but already routinely use it in
"survival mode", learning it gradually, on-demand.)
Today I bumped into this issue, which I then tried googling for, and
found it kinda' hanging:
http://stackoverflow.com/questions/17226117/git-show-commit-oneline-not-showing-oneline
The source of the confusion is that git show --oneline insists on
showing the diff, no matter what, while the man page is misleadingly
subtle on that, as illustrated best by a comment on that SO page,
exactly matching my case, too:
"the answer does not however explain why git show HEAD --oneline
does not produce an output as stated in the documentation:
<sha1> <title line> This is designed to be as compact as possible."
Note: I do understand now (after 5-10 minutes of googling in vain,
mucking around with git, re-reading the man page a few times, and
considering the fact that a bug like that in such a feature couldn't
exist for this long -- in fact, this clue helped me more than the docs
+ googling together), that it's indeed, not a bug, but our lack of
intimate familiarity with git and the terseness/wording of the man
page together cause the confusion.
(BTW, just realized: on the git show man page, also the strangely
implied "log of a commit" concept (i.e. in "Pretty-print the contents
of the commit logs") is partly responsible for the confusion. The term
"log" doesn't mean a single event-like item, but a series of entries,
a running record of events. In VCS lingo the established concept is
that "a log is a list of commits", and that's even the case with git,
too, of course. So, readers of the git show man page will have an
impedance match there, and unnecessary difficulties understanding what
is actually going on. My overloaded brain, for example, apparently
opted for just filtering out that part without warning, upon the first
(two) reading.)
Assuming that we (the SO guy + his upvoters + me) are not the only
ones, I'd suggest that instead of perhaps changing the man page,
there's an other easy, also backwardly compatible, and quite
straightforward way to address this: actually implementing that
"unexpectedly missing" feature people intuitively look for.
Please note that "show" is such a profoundly generic command verb,
probably also used heavily in git, especially to show commits, that it
comes to mind as second nature, without thinking, as the primary (or
even as "the only") choice for showing various items in various ways
-- which, in fact, it already properly does, indeed.
Forcing us to use a different command (git log) for a minor sub-case
of the main "show me a commit" scenario (of git show), is highly
unnatural.
Also, --oneline (as all the other formatting options) look and feel
global to the entire context (result) of the show command, so people
who have not yet unlearned to expect that, will be surprised
(unpleasantly).
However, simply adding --no-diff to the git show command (and the man
page) would help a lot. (Something like --message-only or --only-msg
etc., would be more correct (than the "potentially leaky" complement),
but I'm not familiar with the general use of the "only" modifier in
git options and cannot comment on that, but I've certainly seen the
--no-... form at least.)
Thanks a lot, and have a nice next message! :)
Szabolcs
^ permalink raw reply
* [PATCH 0/3] delete_ref(): support reflog messages
From: Kyle Meyer @ 2017-02-17 3:57 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170126211205.5gz3zsrptop7n34n@sigill.intra.peff.net>
[Sorry for the slow response.]
Jeff King <peff@peff.net> writes:
> On Mon, Jan 16, 2017 at 06:17:29PM -0500, Kyle Meyer wrote:
[...]
>> $ cat .git/logs/refs/heads/new-master
>> 00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500 commit (initial): Add file.txt
>> 68730... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500 Branch: renamed refs/heads/master to refs/heads/new-master
>>
>> $ cat .git/logs/HEAD
>> 00000... 68730... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500 commit (initial): Add file.txt
>> 68730... 00000... Kyle Meyer <kyle@kyleam.com> 1484607020 -0500
>>
>> I expected the second line of .git/logs/HEAD to mirror the second line
>> of .git/logs/refs/heads/new-master. Are the empty message and null sha1
>> in HEAD's entry intentional?
>
> The null sha1 is correct, I think. The branch we were on went away, and
> we use the null sha1 to indicate "no value" in both the creation and
> deletion cases.
I see, thanks.
> I'd say there are two potential improvements:
>
> - delete_ref() should take a reflog message argument, in case it
> updates the HEAD reflog (as a bonus, this will future-proof us for a
> day when we might keep reflogs for deleted refs).
I've tried to do this in the following patches.
> - "git branch -m" does seem to realize when we are renaming HEAD,
> because it updates HEAD to point to the new branch name. But it
> should probably insert another reflog entry mentioning the rename
> (we do for "git checkout foo", even when "foo" has the same sha1 as
> the current HEAD).
I haven't worked out how to do this part yet. I'm guessing the change
will involve modifying split_head_update().
If this is added, should it be instead of, rather than in addition to,
the deletion entry? If a "Branch: renamed ..." entry is present, it
doesn't seem like the deletion entry is providing any additional
information.
delete_refs(): accept a reflog message argument
update-ref: pass reflog message argument to delete_refs
rename_ref: replace empty deletion message in HEAD's log
builtin/am.c | 4 ++--
builtin/branch.c | 2 +-
builtin/notes.c | 4 ++--
builtin/remote.c | 4 ++--
builtin/replace.c | 2 +-
builtin/reset.c | 2 +-
builtin/symbolic-ref.c | 2 +-
builtin/tag.c | 2 +-
builtin/update-ref.c | 2 +-
fast-import.c | 2 +-
refs.c | 4 ++--
refs.h | 2 +-
refs/files-backend.c | 12 +++++++++---
t/t1400-update-ref.sh | 9 +++++++++
t/t3200-branch.sh | 4 ++++
transport.c | 2 +-
16 files changed, 39 insertions(+), 20 deletions(-)
--
Kyle
^ permalink raw reply
* [PATCH 1/3] delete_refs(): accept a reflog message argument
From: Kyle Meyer @ 2017-02-17 3:57 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217035800.13214-1-kyle@kyleam.com>
When the current branch is renamed with 'git branch -m/-M' or deleted
with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
with an empty message.
In preparation for adding a more meaningful message to HEAD's log in
these cases, update delete_ref() to take a message argument and pass
it along to ref_transaction_delete(). Modify all callers to pass NULL
for the new message argument; no change in behavior is intended.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
builtin/am.c | 4 ++--
builtin/branch.c | 2 +-
builtin/notes.c | 4 ++--
builtin/remote.c | 4 ++--
builtin/replace.c | 2 +-
builtin/reset.c | 2 +-
builtin/symbolic-ref.c | 2 +-
builtin/tag.c | 2 +-
builtin/update-ref.c | 2 +-
fast-import.c | 2 +-
refs.c | 4 ++--
refs.h | 2 +-
refs/files-backend.c | 6 +++---
transport.c | 2 +-
14 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/builtin/am.c b/builtin/am.c
index 31fb60578..e08c739d4 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1049,7 +1049,7 @@ static void am_setup(struct am_state *state, enum patch_format patch_format,
} else {
write_state_text(state, "abort-safety", "");
if (!state->rebasing)
- delete_ref("ORIG_HEAD", NULL, 0);
+ delete_ref("ORIG_HEAD", NULL, 0, NULL);
}
/*
@@ -2172,7 +2172,7 @@ static void am_abort(struct am_state *state)
has_curr_head ? &curr_head : NULL, 0,
UPDATE_REFS_DIE_ON_ERR);
else if (curr_branch)
- delete_ref(curr_branch, NULL, REF_NODEREF);
+ delete_ref(curr_branch, NULL, REF_NODEREF, NULL);
free(curr_branch);
am_destroy(state);
diff --git a/builtin/branch.c b/builtin/branch.c
index 9d30f55b0..44f23208f 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -252,7 +252,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
}
if (delete_ref(name, is_null_sha1(sha1) ? NULL : sha1,
- REF_NODEREF)) {
+ REF_NODEREF, NULL)) {
error(remote_branch
? _("Error deleting remote-tracking branch '%s'")
: _("Error deleting branch '%s'"),
diff --git a/builtin/notes.c b/builtin/notes.c
index 5248a9bad..991448d4e 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -681,9 +681,9 @@ static int merge_abort(struct notes_merge_options *o)
* notes_merge_abort() to remove .git/NOTES_MERGE_WORKTREE.
*/
- if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0))
+ if (delete_ref("NOTES_MERGE_PARTIAL", NULL, 0, NULL))
ret += error(_("failed to delete ref NOTES_MERGE_PARTIAL"));
- if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF))
+ if (delete_ref("NOTES_MERGE_REF", NULL, REF_NODEREF, NULL))
ret += error(_("failed to delete ref NOTES_MERGE_REF"));
if (notes_merge_abort(o))
ret += error(_("failed to remove 'git notes merge' worktree"));
diff --git a/builtin/remote.c b/builtin/remote.c
index 5339ed6ad..bfa8a5189 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -691,7 +691,7 @@ static int mv(int argc, const char **argv)
read_ref_full(item->string, RESOLVE_REF_READING, oid.hash, &flag);
if (!(flag & REF_ISSYMREF))
continue;
- if (delete_ref(item->string, NULL, REF_NODEREF))
+ if (delete_ref(item->string, NULL, REF_NODEREF, NULL))
die(_("deleting '%s' failed"), item->string);
}
for (i = 0; i < remote_branches.nr; i++) {
@@ -1248,7 +1248,7 @@ static int set_head(int argc, const char **argv)
head_name = xstrdup(states.heads.items[0].string);
free_remote_ref_states(&states);
} else if (opt_d && !opt_a && argc == 1) {
- if (delete_ref(buf.buf, NULL, REF_NODEREF))
+ if (delete_ref(buf.buf, NULL, REF_NODEREF, NULL))
result |= error(_("Could not delete %s"), buf.buf);
} else
usage_with_options(builtin_remote_sethead_usage, options);
diff --git a/builtin/replace.c b/builtin/replace.c
index b58c714cb..d32d0a3ae 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -121,7 +121,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
static int delete_replace_ref(const char *name, const char *ref,
const unsigned char *sha1)
{
- if (delete_ref(ref, sha1, 0))
+ if (delete_ref(ref, sha1, 0, NULL))
return 1;
printf("Deleted replace ref '%s'\n", name);
return 0;
diff --git a/builtin/reset.c b/builtin/reset.c
index 8ab915bfc..cccd3f099 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -256,7 +256,7 @@ static int reset_refs(const char *rev, const struct object_id *oid)
update_ref_oid(msg.buf, "ORIG_HEAD", orig, old_orig, 0,
UPDATE_REFS_MSG_ON_ERR);
} else if (old_orig)
- delete_ref("ORIG_HEAD", old_orig->hash, 0);
+ delete_ref("ORIG_HEAD", old_orig->hash, 0, NULL);
set_reflog_message(&msg, "updating HEAD", rev);
update_ref_status = update_ref_oid(msg.buf, "HEAD", oid, orig, 0,
UPDATE_REFS_MSG_ON_ERR);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index 96eed9446..c9d5bd3e8 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -58,7 +58,7 @@ int cmd_symbolic_ref(int argc, const char **argv, const char *prefix)
die("Cannot delete %s, not a symbolic ref", argv[0]);
if (!strcmp(argv[0], "HEAD"))
die("deleting '%s' is not allowed", argv[0]);
- return delete_ref(argv[0], NULL, REF_NODEREF);
+ return delete_ref(argv[0], NULL, REF_NODEREF, NULL);
}
switch (argc) {
diff --git a/builtin/tag.c b/builtin/tag.c
index e40c4a967..850a0674c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -97,7 +97,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn,
static int delete_tag(const char *name, const char *ref,
const unsigned char *sha1, const void *cb_data)
{
- if (delete_ref(ref, sha1, 0))
+ if (delete_ref(ref, sha1, 0, NULL))
return 1;
printf(_("Deleted tag '%s' (was %s)\n"), name, find_unique_abbrev(sha1, DEFAULT_ABBREV));
return 0;
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index 7f30d3a76..a41f9adf1 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
*/
return delete_ref(refname,
(oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
- flags);
+ flags, NULL);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
flags | create_reflog_flag,
diff --git a/fast-import.c b/fast-import.c
index 64fe602f0..07412a85a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1752,7 +1752,7 @@ static int update_branch(struct branch *b)
if (is_null_sha1(b->sha1)) {
if (b->delete)
- delete_ref(b->name, NULL, 0);
+ delete_ref(b->name, NULL, 0, NULL);
return 0;
}
if (read_ref(b->name, old_sha1))
diff --git a/refs.c b/refs.c
index cd36b64ed..4c80ebf2e 100644
--- a/refs.c
+++ b/refs.c
@@ -592,7 +592,7 @@ static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1
}
int delete_ref(const char *refname, const unsigned char *old_sha1,
- unsigned int flags)
+ unsigned int flags, const char *msg)
{
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
@@ -603,7 +603,7 @@ int delete_ref(const char *refname, const unsigned char *old_sha1,
transaction = ref_transaction_begin(&err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
- flags, NULL, &err) ||
+ flags, msg, &err) ||
ref_transaction_commit(transaction, &err)) {
error("%s", err.buf);
ref_transaction_free(transaction);
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);
/*
* Delete the specified references. If there are any problems, emit
diff --git a/refs/files-backend.c b/refs/files-backend.c
index c041d4ba2..ffa75d816 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2489,7 +2489,7 @@ static int files_delete_refs(struct ref_store *ref_store,
for (i = 0; i < refnames->nr; i++) {
const char *refname = refnames->items[i].string;
- if (delete_ref(refname, NULL, flags))
+ if (delete_ref(refname, NULL, flags, NULL))
result |= error(_("could not remove reference %s"), refname);
}
@@ -2616,7 +2616,7 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF)) {
+ if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
error("unable to delete old %s", oldrefname);
goto rollback;
}
@@ -2630,7 +2630,7 @@ static int files_rename_ref(struct ref_store *ref_store,
*/
if (!read_ref_full(newrefname, RESOLVE_REF_READING | RESOLVE_REF_NO_RECURSE,
sha1, NULL) &&
- delete_ref(newrefname, NULL, REF_NODEREF)) {
+ delete_ref(newrefname, NULL, REF_NODEREF, NULL)) {
if (errno==EISDIR) {
struct strbuf path = STRBUF_INIT;
int result;
diff --git a/transport.c b/transport.c
index d72e08948..e025d9b29 100644
--- a/transport.c
+++ b/transport.c
@@ -299,7 +299,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
if (verbose)
fprintf(stderr, "updating local tracking ref '%s'\n", rs.dst);
if (ref->deletion) {
- delete_ref(rs.dst, NULL, 0);
+ delete_ref(rs.dst, NULL, 0, NULL);
} else
update_ref("update by push", rs.dst,
ref->new_oid.hash, NULL, 0, 0);
--
2.11.1
^ permalink raw reply related
* [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Kyle Meyer @ 2017-02-17 3:57 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217035800.13214-1-kyle@kyleam.com>
Now that delete_refs() accepts a reflog message, pass the
user-provided message to delete_refs() rather than silently dropping
it. The doesn't matter for the deleted ref's log because the log is
deleted along with the ref, but this entry will show up in HEAD's
reflog when deleting a checked out branch.
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
I'm not sure if the test here (or in the next patch) is worth including.
builtin/update-ref.c | 2 +-
t/t1400-update-ref.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/builtin/update-ref.c b/builtin/update-ref.c
index a41f9adf1..f642acc22 100644
--- a/builtin/update-ref.c
+++ b/builtin/update-ref.c
@@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
*/
return delete_ref(refname,
(oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
- flags, NULL);
+ flags, msg);
else
return update_ref(msg, refname, sha1, oldval ? oldsha1 : NULL,
flags | create_reflog_flag,
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
+
test_expect_success 'update-ref does not create reflogs by default' '
test_when_finished "git update-ref -d $outside" &&
git update-ref $outside $A &&
--
2.11.1
^ permalink raw reply related
* [PATCH 3/3] rename_ref: replace empty deletion message in HEAD's log
From: Kyle Meyer @ 2017-02-17 3:58 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: Kyle Meyer, git
In-Reply-To: <20170217035800.13214-1-kyle@kyleam.com>
When the current branch is renamed, the deletion of the old ref is
recorded in HEAD's log with an empty message. Now that delete_refs()
accepts a reflog message, provide a more descriptive message. This
message will not be included in the reflog of the renamed branch, but
its log already covers the renaming event with a message of "Branch:
renamed ...".
Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
refs/files-backend.c | 8 +++++++-
t/t3200-branch.sh | 4 ++++
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/refs/files-backend.c b/refs/files-backend.c
index ffa75d816..bb5df7ee6 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2598,6 +2598,7 @@ static int files_rename_ref(struct ref_store *ref_store,
struct stat loginfo;
int log = !lstat(git_path("logs/%s", oldrefname), &loginfo);
struct strbuf err = STRBUF_INIT;
+ struct strbuf logmsg_del = STRBUF_INIT;
if (log && S_ISLNK(loginfo.st_mode))
return error("reflog for %s is a symlink", oldrefname);
@@ -2616,10 +2617,15 @@ static int files_rename_ref(struct ref_store *ref_store,
return error("unable to move logfile logs/%s to "TMP_RENAMED_LOG": %s",
oldrefname, strerror(errno));
- if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, NULL)) {
+ strbuf_addf(&logmsg_del, "Deleted %s", oldrefname);
+
+ if (delete_ref(oldrefname, orig_sha1, REF_NODEREF, logmsg_del.buf)) {
error("unable to delete old %s", oldrefname);
+ strbuf_release(&logmsg_del);
goto rollback;
}
+ strbuf_release(&logmsg_del);
+
/*
* Since we are doing a shallow lookup, sha1 is not the
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 8a833f354..4af7cd2b7 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -139,6 +139,10 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
test $(git rev-parse --abbrev-ref HEAD) = bam
'
+test_expect_success 'git branch -M baz bam should add entry to .git/logs/HEAD' '
+ grep "Deleted refs/heads/baz$" .git/logs/HEAD >/dev/null
+'
+
test_expect_success 'git branch -M baz bam should succeed when baz is checked out as linked working tree' '
git checkout master &&
git worktree add -b baz bazdir &&
--
2.11.1
^ permalink raw reply related
* Re: `git show --oneline commit` does not do what's intuitively expected
From: Jeff King @ 2017-02-17 4:05 UTC (permalink / raw)
To: Luna Kid; +Cc: git
In-Reply-To: <CA+-W3ctdRtLpziJ9TX2hqk7RagMyJpHsrfwj=rN7oXQ8EeUPnw@mail.gmail.com>
On Fri, Feb 17, 2017 at 02:51:36AM +0100, Luna Kid wrote:
> tl;dr; --> Please add --no-diff (or --msg-only) to git show. We'll
> love you for that. :)
I think it is already spelled "--no-patch", or "-s" for short.
> Please note that "show" is such a profoundly generic command verb,
> probably also used heavily in git, especially to show commits, that it
> comes to mind as second nature, without thinking, as the primary (or
> even as "the only") choice for showing various items in various ways
> -- which, in fact, it already properly does, indeed.
Right. That's what's it for. The DESCRIPTION section of the manpage
starts with:
Shows one or more objects (blobs, trees, tags and commits).
For commits it shows the log message and textual diff. It also
presents the merge commit in a special format as produced by git
diff-tree --cc.
For tags, it shows[...etc...]
I guess that second paragraph could mention "--no-patch" explicitly to
disable it. It's documented in the COMMON DIFF OPTIONS section, but of
course there are quite a few options listed, so it's easy to miss.
-Peff
^ permalink raw reply
* Re: [PATCH] show-branch: fix crash with long ref name
From: Jeff King @ 2017-02-17 5:03 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, git, Maxim Kuvyrkov, Pranit Bauva
In-Reply-To: <CAP8UFD20=zVy=1Tr4D1Rxf+a4yR_X2zmexNCTNKV5RSF9ueYrg@mail.gmail.com>
On Thu, Feb 16, 2017 at 01:40:00PM +0100, Christian Couder wrote:
> > I started to add some tests, but I had second thoughts. It _is_ nice
> > to show off the fix, but as far as regressions go, this specific case is
> > unlikely to come up again. What would be more valuable, I think, is a
> > test script which set up a very long refname (not just 150 bytes or
> > whatever) and ran it through a series of git commands.
>
> I agree that a test script running through a series of command with
> long refnames would be great.
>
> But I think the refname should not necesarily be too long. As I wrote
> in the commit message of my patch, if the ref name had been much
> longer the crash would not have happened because the ref could not
> have been created in the first place.
Right, I think there's a tension there. Too short and it is not
interesting, and too long and things start to fail for uninteresting
reasons (e.g., your filesystem can't handle the name).
> So the best would be to run through a series of commands with a
> refname ranging from let's say 80 chars to 300 chars.
>
> That would have a chance to catch crashes due to legacy code using for
> example things like `char stuff[128]` or `char stuff[256]`.
But that doesn't catch `char stuff[512]`. I think you'd really want a
range of sizes, and to test all of them. Worse, though, is it's not
clear how you decide when a test has failed. Obviously smashing the
stack is a bad outcome, but part of the goal would presumably be to
flush out unnecessary length limitations elsewhere.
I got about that far in my thinking before saying "wow, this is getting
to be complicated for not much gain".
> > So I dunno. It seems like being thorough is a
> > lot of hassle for not much gain. Being not-thorough is easy, but is
> > mostly a token that is unlikely to find any real bugs.
>
> Yeah, if we really care, it might be better to start using a fuzzer or
> a property based testing tool instead of bothering with these kind of
> tests by ourselves, which is also a different topic.
Yes, I'd agree that a fuzzer is probably a better choice. I played with
AFL a bit back when it came out, but I never got it to turn up anything
useful.
I am disappointed that this obvious memory problem survived for so long.
I did quite a bit of auditing for such problems a year or two ago, but I
focused on problematic functions like strcpy, sprintf, etc.
It's easy to use memcpy() wrong, too, but it's hard to audit. There are
a lot of calls, and you have to match up the copied length with a value
computed elsewhere. I traded a lot of them out back then for safer
variants (like the FLEX_ALLOC stuff), but many calls just fundamentally
need something unsafe like memcpy to get their job done.
-Peff
^ permalink raw reply
* Re: difflame improvements
From: Jeff King @ 2017-02-17 5:17 UTC (permalink / raw)
To: Edmundo Carmona Antoranz; +Cc: Git List
In-Reply-To: <CAOc6etYz6+FzDRbkLS2SB9=F9DP18=6uLKdfCN3D3yd2Gug-tw@mail.gmail.com>
On Tue, Feb 14, 2017 at 11:19:05PM -0600, Edmundo Carmona Antoranz wrote:
> I've been working on detecting revisions where a "real" deletion was
> made and I think I advanced a lot in that front. I still have to work
> on many scenarios (renamed files, for example... also performance) but
> at least I'm using a few runs against git-scm history and the results
> are "promising":
I played with this a bit more, and it did turn up the correct results
for some deletions in my experiments.
One thing I noticed is that it also turned up nonsense for lines that
blame in weird ways. For instance, I have a diff like this (these are
real examples, but don't pay attention to the sha1s; it's in a fork of
git, not upstream):
$ git diff v2.6.5 builtin/prune-packed.c
diff --git a/builtin/prune-packed.c b/builtin/prune-packed.c
index 7cf900ea07..5e3727e841 100644
--- a/builtin/prune-packed.c
+++ b/builtin/prune-packed.c
@@ -2,6 +2,7 @@
#include "cache.h"
#include "progress.h"
#include "parse-options.h"
+#include "gh-log.h"
static const char * const prune_packed_usage[] = {
N_("git prune-packed [-n | --dry-run] [-q | --quiet]"),
@@ -29,8 +30,11 @@ static int prune_object(const unsigned char *sha1, const char *path,
if (*opts & PRUNE_PACKED_DRY_RUN)
printf("rm -f %s\n", path);
- else
+ else {
+ gh_logf("prune", "%s Duplicate loose object pruned\n",
+ sha1_to_hex(sha1));
unlink_or_warn(path);
+ }
return 0;
}
Running difflame on it says this:
$ python /path/to/difflame.py v2.6.5..HEAD -- builtin/prune-packed.c
[...]
-2c0b29e662 (Jeff King 2016-01-26 15:27:55 -0500 32) else
+d60032f8640 builtin/prune-packed.c (Jeff King 2015-02-02 23:15:33 -0500 33) else {
+d60032f8640 builtin/prune-packed.c (Jeff King 2015-02-02 23:15:33 -0500 34) gh_logf("prune", "%s Duplicate loose object pruned\n",
+d60032f8640 builtin/prune-packed.c (Jeff King 2015-02-02 23:15:33 -0500 35) sha1_to_hex(sha1));
0d3b729680e builtin/prune-packed.c (Jeff King 2014-10-15 18:40:53 -0400 36) unlink_or_warn(path);
+2396ec85bd1 prune-packed.c (Linus Torvalds 2005-07-03 14:27:34 -0700 37) }
There are two weird things. One is that the old "else" is attributed to
my 2c0b29e662. That's quite weird, because that is a merge commit which
did not touch the file at all. I haven't tracked it down, but presumably
that is weirdness with the --reverse blame.
But there's another one, that I think is easy to fix. The closing brace
is attributed to some ancient commit from Linus. Which yes, I'm sure had
a closing brace, but not _my_ closing brace that was added by
d60032f8640, that the rest of the lines got attributed to.
This isn't difflame's fault; that's what "git blame" tells you about
that line. But since I already told difflame "v2.6.5..HEAD", it would
probably make sense to similarly limit the blame to that range. That
turns up a boundary commit for the line. Which is _also_ not helpful,
but at least the tool is telling me that the line came from before
v2.6.5, and I don't really need to care much about it.
Part of this is that my use case may be a bit different than yours. I
don't actually want to look at the blame results directly. I just want
to see the set of commits that I'd need to look at and possibly
cherry-pick in order to re-create the diff.
-Peff
^ permalink raw reply
* Re: difflame improvements
From: Edmundo Carmona Antoranz @ 2017-02-17 7:01 UTC (permalink / raw)
To: Jeff King; +Cc: Git List
In-Reply-To: <20170217051755.fx2ueizsprw2vida@sigill.intra.peff.net>
On Thu, Feb 16, 2017 at 11:17 PM, Jeff King <peff@peff.net> wrote:
> This isn't difflame's fault; that's what "git blame" tells you about
> that line. But since I already told difflame "v2.6.5..HEAD", it would
> probably make sense to similarly limit the blame to that range. That
> turns up a boundary commit for the line. Which is _also_ not helpful,
> but at least the tool is telling me that the line came from before
> v2.6.5, and I don't really need to care much about it.
I'm running my own tests on difflame and I have a theory about "when"
it breaks.... at least one of the cases when it breaks:
Analysis for deleted lines is being driven by git blame --reverse.
What I have noticed is that it "breaks" when blame --reverse drives
the analysis into revisions where "treeish1" is not part of their
history (like, bringing analysis "to the sides" of treeish1 instead of
keeping analysis in revisions in the history of treeish2 that have
treeish1 as one of their ancestors.... which is definitely a valid
case for analysis, anyway). In this case, blame --reverse stops being
helpful.
Take this example (I just pushed a debug-deletion branch into gh...
probably more debugging messages will be needed):
$ difflame.py HEAD~100 HEAD -- Documentation/git-commit.txt
diff --git a/Documentation/git-commit.txt b/Documentation/git-commit.txt
index f2ab0ee2e..4f8f20a36 100644
--- a/Documentation/git-commit.txt
+++ b/Documentation/git-commit.txt
@@ -265,7 +265,8 @@ FROM UPSTREAM REBASE" section in linkgit:git-rebase[1].)
bcf9626a71 (Matthieu Moy 2016-06-28 13:40:11 +0200 265) If this
option is specified together with `--amend`, then
04c8ce9c1c (Markus Heidelberg 2008-12-19 13:14:18 +0100 266) no
paths need to be specified, which can be used to amend
d4ba07cac5 (Johannes Sixt 2008-04-10 13:33:09 +0200 267) the
last commit without committing changes that have
Range of revisions: 02db2d..066fb04
Treeish1 02db2d04: 02db2d042 Merge branch 'ah/grammos'
Treeish2 066fb0494: 066fb0494 blame: draft of line format
Blamed Revision afe0e2a39: afe0e2a39 Merge branch
'da/difftool-dir-diff-fix'
Original Filename a/Documentation/git-commit.txt Deleted Line 268
Children revisions:
3aead1cad7a: 3aead1cad Merge branch 'ak/commit-only-allow-empty'
There's only one child revision.... on that revision the line
we are tracking is gone
Parents of this child revision:
afe0e2a39166: afe0e2a39 Merge branch 'da/difftool-dir-diff-fix'
beb635ca9ce: beb635ca9 commit: remove 'Clever' message
for --only --amend
Finding parent where the line has been deleted:
beb635ca9: beb635ca9 commit: remove 'Clever' message
for --only --amend
Range of revisions: 02db2d042..beb635c
Treeish1 02db2d0: 02db2d042 Merge branch 'ah/grammos'
Treeish2 beb635c: beb635ca9 commit: remove 'Clever'
message for --only --amend
Blamed Revision 02db2d0: 02db2d042 Merge branch 'ah/grammos'
Original Filename a/Documentation/git-commit.txt Deleted Line 268
Children revisions:
Found no children... will return the original blamed revision
(02db2d0) saying that the deleting revision could not be found
beb635ca9 commit: remove 'Clever' message for --only --amend
-beb635ca9 (Andreas Krey 2016-12-09 05:10:21 +0100 268)
already been staged.
319d83524 commit: make --only --allow-empty work without paths
+319d835240 (Andreas Krey 2016-12-02 23:15:13 +0100 268)
already been staged. ...
+319d835240 (Andreas Krey 2016-12-02 23:15:13 +0100 269) paths
are also not requi...
d4ba07cac5 (Johannes Sixt 2008-04-10 13:33:09 +0200 270)
1947bdbc31 (Junio C Hamano 2008-06-22 14:32:27 -0700 271) -u[<mode>]::
1947bdbc31 (Junio C Hamano 2008-06-22 14:32:27 -0700 272)
--untracked-files[=<mode>]::
I know that line 268 was deleted on 319d835240.
So.... on the first round of merge analysis it says "let's go into
beb635ca9". That's fine. That's exactly the path that is required to
reach 319d835240. But then when using this new "range of revisions"
for git blame --reverse, we get that line 268 is not telling us
anything useful:
$ git blame --reverse -L268,268 02db2d042..beb635c --
Documentation/git-commit.txt
^02db2d042 (Junio C Hamano 2016-12-19 14:45:30 -0800 268)
already been staged.
So, instead of pointing to 319d835240 (the parent of beb635c), it's
basically saying something like "I give up". My hunch (haven't sat
down to digest all the details about the output of git blame
--reverse... YET) is that, given that 02db2d042 is _not_ part of the
history of beb635c, git blame reverse is trying to tell me just
that... and that means I'll have to "script around this scenario".
$ git merge-base 02db2d042 beb635c
0202c411edc25940cc381bf317badcdf67670be4
Thanks in advance.
^ permalink raw reply related
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Michael Haggerty @ 2017-02-17 8:00 UTC (permalink / raw)
To: Jeff King, Andreas Schwab; +Cc: Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <20170216213140.xqw7gzjimhvg7tcm@sigill.intra.peff.net>
On 02/16/2017 10:31 PM, Jeff King wrote:
> On Thu, Feb 16, 2017 at 11:43:59AM -0500, Jeff King wrote:
>
>> On Thu, Feb 16, 2017 at 11:10:18AM +0100, Andreas Schwab wrote:
>>
>>>>> int xfclose(FILE *fp)
>>>>> {
>>>>> return ferror(fp) | fclose(fp);
>>>>> }
>>>>
>>>> Yes, that's exactly what I had in mind (might be worth calling out the
>>>> bitwise-OR, though, just to make it clear it's not a typo).
>>>
>>> Since the order of evaluation is unspecified, it would be better to
>>> force sequencing ferror before fclose.
>>
>> Good point. Arguably the call in tempfile.c is buggy.
>
> Here's a fix.
>
> I think close_tempfile() suffers from the same errno problem discussed
> earlier in this thread (i.e., that after calling it, you may get an
> error return with a random, unrelated errno value if ferror() failed but
> fclose() did not).
>
> -- >8 --
> Subject: [PATCH] tempfile: avoid "ferror | fclose" trick
>
> The current code wants to record an error condition from
> either ferror() or fclose(), but makes sure that we always
> call both functions. So it can't use logical-OR "||", which
> would short-circuit when ferror() is true. Instead, it uses
> bitwise-OR "|" to evaluate both functions and set one or
> more bits in the "err" flag if they reported a failure.
>
> Unlike logical-OR, though, bitwise-OR does not introduce a
> sequence point, and the order of evaluation for its operands
> is unspecified. So a compiler would be free to generate code
> which calls fclose() first, and then ferror() on the
> now-freed filehandle.
>
> There's no indication that this has happened in practice,
> but let's write it out in a way that follows the standard.
>
> Noticed-by: Andreas Schwab <schwab@linux-m68k.org>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> tempfile.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/tempfile.c b/tempfile.c
> index 2990c9242..ffcc27237 100644
> --- a/tempfile.c
> +++ b/tempfile.c
> @@ -247,12 +247,8 @@ int close_tempfile(struct tempfile *tempfile)
> tempfile->fd = -1;
> if (fp) {
> tempfile->fp = NULL;
> -
> - /*
> - * Note: no short-circuiting here; we want to fclose()
> - * in any case!
> - */
> - err = ferror(fp) | fclose(fp);
> + err = ferror(fp);
> + err |= fclose(fp);
> } else {
> err = close(fd);
> }
>
Thanks for fixing this; the old code was definitely wrong.
As you pointed out, if ferror() fails, it doesn't set errno properly. At
least one caller tries to strerror(errno), so it would probably be good
to store *something* in there, probably EIO.
To be really pedantic, there's also the question of what errno the
caller would want if *both* ferror() and fclose() fail. Normally I would
say "the first error that occurred", but in this case we don't know the
correct errno from the error reported by ferror(), so maybe the fclose()
errno is more likely to hint at the underlying reason for the failure.
So I (reluctantly) propose
if (ferror(fp)) {
if (!fclose(fp)) {
/*
* ferror() doesn't set errno, so we have to
* set one. (By contrast, when fclose() fails
* too, we leave *its* errno in place.)
*/
errno = EIO;
}
return -1;
}
return fclose();
Michael
^ permalink raw reply
* Re: [PATCH] tempfile: avoid "ferror | fclose" trick
From: Jeff King @ 2017-02-17 8:07 UTC (permalink / raw)
To: Michael Haggerty
Cc: Andreas Schwab, Junio C Hamano, Jáchym Barvínek, git
In-Reply-To: <923e328c-7fea-a9e4-1059-3bd6b8e58164@alum.mit.edu>
On Fri, Feb 17, 2017 at 09:00:09AM +0100, Michael Haggerty wrote:
> As you pointed out, if ferror() fails, it doesn't set errno properly. At
> least one caller tries to strerror(errno), so it would probably be good
> to store *something* in there, probably EIO.
Yeah, we discussed this up-thread a bit, and my "solution" was similar
to yours. I don't like it, because EIO is a real thing that can happen,
too, and it would certainly be surprising to a user to see. But it's
probably better than the alternative, which is getting whatever random
value happened to be in errno.
The only downside is that if the value of errno _was_ valid (because the
last thing you did really was writing to the filehandle, then we'd
overwrite it).
> To be really pedantic, there's also the question of what errno the
> caller would want if *both* ferror() and fclose() fail. Normally I would
> say "the first error that occurred", but in this case we don't know the
> correct errno from the error reported by ferror(), so maybe the fclose()
> errno is more likely to hint at the underlying reason for the failure.
Yes, I think we're better to take what fclose gives us, if we can.
> So I (reluctantly) propose
>
> if (ferror(fp)) {
> if (!fclose(fp)) {
> /*
> * ferror() doesn't set errno, so we have to
> * set one. (By contrast, when fclose() fails
> * too, we leave *its* errno in place.)
> */
> errno = EIO;
> }
> return -1;
> }
> return fclose();
That's similar to what I wrote earlier, but if we don't mind overwriting
errno unconditionally, I think just:
errno = EIO; /* covers ferror(), overwritten by failing fclose() */
err |= ferror(fp);
err |= fclose(fp);
does the same thing.
-Peff
^ permalink raw reply
* Re: [PATCH 1/3] delete_refs(): accept a reflog message argument
From: Jeff King @ 2017-02-17 8:12 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-2-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:57:58PM -0500, Kyle Meyer wrote:
> When the current branch is renamed with 'git branch -m/-M' or deleted
> with 'git update-ref -m<msg> -d', the event is recorded in HEAD's log
> with an empty message.
>
> In preparation for adding a more meaningful message to HEAD's log in
> these cases, update delete_ref() to take a message argument and pass
> it along to ref_transaction_delete(). Modify all callers to pass NULL
> for the new message argument; no change in behavior is intended.
Seems like a good first step.
> 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()?
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] delete_ref(): support reflog messages
From: Jeff King @ 2017-02-17 8:17 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-1-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:57:57PM -0500, Kyle Meyer wrote:
> [Sorry for the slow response.]
No problem. The pace of open source varies wildly. :)
> > - "git branch -m" does seem to realize when we are renaming HEAD,
> > because it updates HEAD to point to the new branch name. But it
> > should probably insert another reflog entry mentioning the rename
> > (we do for "git checkout foo", even when "foo" has the same sha1 as
> > the current HEAD).
>
> I haven't worked out how to do this part yet. I'm guessing the change
> will involve modifying split_head_update().
>
> If this is added, should it be instead of, rather than in addition to,
> the deletion entry? If a "Branch: renamed ..." entry is present, it
> doesn't seem like the deletion entry is providing any additional
> information.
I think you could do an "instead of" that goes from sha1 X to X, and
just mentions the rename. Or you could add a second entry after the
delete that takes it from 0{40} back to X.
I suspect the latter is easier to do, and I doubt anybody would care
that much of the exact form. These entries aren't really doing anything
for reachability. They're just giving an audit log of what happened. So
I don't think anybody would really care unless they were debugging a
confusing situation by hand. And as long as there's enough information
to figure out what happened, they'll be happy.
-Peff
^ permalink raw reply
* Re: [PATCH 2/3] update-ref: pass reflog message argument to delete_refs
From: Jeff King @ 2017-02-17 8:22 UTC (permalink / raw)
To: Kyle Meyer; +Cc: Junio C Hamano, git
In-Reply-To: <20170217035800.13214-3-kyle@kyleam.com>
On Thu, Feb 16, 2017 at 10:57:59PM -0500, Kyle Meyer wrote:
> Now that delete_refs() accepts a reflog message, pass the
> user-provided message to delete_refs() rather than silently dropping
> it. The doesn't matter for the deleted ref's log because the log is
> deleted along with the ref, but this entry will show up in HEAD's
> reflog when deleting a checked out branch.
Sounds good.
> diff --git a/builtin/update-ref.c b/builtin/update-ref.c
> index a41f9adf1..f642acc22 100644
> --- a/builtin/update-ref.c
> +++ b/builtin/update-ref.c
> @@ -435,7 +435,7 @@ int cmd_update_ref(int argc, const char **argv, const char *prefix)
> */
> return delete_ref(refname,
> (oldval && !is_null_sha1(oldsha1)) ? oldsha1 : NULL,
> - flags, NULL);
> + flags, msg);
This looks obviously correct.
> 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
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).
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).
-Peff
^ permalink raw reply
* git alias for options
From: hIpPy @ 2017-02-17 8:23 UTC (permalink / raw)
To: git
Git has aliases for git commands. Is there a (an inbuilt) way to alias
options? If not, what is the reason?
Thanks,
hippy
^ 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