* Re: [PATCH] push: do not use potentially ambiguous default refspec
From: Junio C Hamano @ 2016-10-31 21:10 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git, Kannan Goundan
In-Reply-To: <20161031203834.dfavyjzkob2goa5n@hurricane>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> On Fri, Oct 28, 2016 at 12:25:30PM -0700, Junio C Hamano wrote:
>
>> * It is appreciated if somebody with spare cycles can add a test or
>> two for this in t/t5523-push-upstream.sh or somewhere nearby.
>
> 5523 is for push --set-upstream-to, 5528 seemed more appropriate. Here's
> something squashable that fails before your patch and succeeds after.
Thanks.
>
>>8----
> Subject: [PATCH] push: test pushing ambiguously named branches
>
> Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> ---
> t/t5528-push-default.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
> index 73f4bb6..ac103ce 100755
> --- a/t/t5528-push-default.sh
> +++ b/t/t5528-push-default.sh
> @@ -98,6 +98,16 @@ test_expect_success 'push from/to new branch with upstream, matching and simple'
> test_push_failure upstream
> '
>
> +test_expect_success 'push ambiguously named branch with upstream, matching and simple' '
> + git checkout -b ambiguous &&
> + test_config branch.ambiguous.remote parent1 &&
> + test_config branch.ambiguous.merge refs/heads/ambiguous &&
> + git tag ambiguous &&
> + test_push_success simple ambiguous &&
> + test_push_success matching ambiguous &&
> + test_push_success upstream ambiguous
> +'
> +
> test_expect_success 'push from/to new branch with current creates remote branch' '
> test_config branch.new-branch.remote repo1 &&
> git checkout new-branch &&
^ permalink raw reply
* Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines
From: Junio C Hamano @ 2016-10-31 21:10 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <db609e13740415ac4e5e357493661347b0f681f7.1477698917.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> diff --git a/trailer.c b/trailer.c
> index 6d8375b..d4d9e10 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
> return same_token(a, b) && same_value(a, b);
> }
>
> -static inline int contains_only_spaces(const char *str)
> +static inline int is_blank_line(const char *str)
> {
> const char *s = str;
> - while (*s && isspace(*s))
> + while (*s && *s != '\n' && isspace(*s))
> s++;
> - return !*s;
> + return !*s || *s == '\n';
> }
This used to be fed a single line and did not have to worry about
stopping at the end of a line, but now it does have to, and does so
correctly. OK.
And the updated function name certainly reflects the fact that this
is (now) about a line-oriented processing better, and this renaming
makes sense.
> @@ -566,13 +566,18 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
> }
>
> /*
> - * Return the location of the first separator in line, or -1 if there is no
> - * separator.
> + * Return the location of the first separator in the given line, or -1 if there
> + * is no separator.
> + *
> + * The interests parameter must contain the acceptable separators and may
> + * contain '\n'. If interests contains '\n', the input line is considered to
> + * end at the first newline encountered. Otherwise, the input line is
> + * considered to end at its null terminator.
> */
The name of that terminating byte is NUL, not NULL.
> -static int find_separator(const char *line, const char *separators)
> +static int find_separator(const char *line, const char *interests)
> {
> - int loc = strcspn(line, separators);
> - if (!line[loc])
> + int loc = strcspn(line, interests);
> + if (!line[loc] || line[loc] == '\n')
> return -1;
> return loc;
> }
I am not sure interests is a better name than separators. If the
original used "interests", I do not think it is worth renaming it to
"separators", and I doubt that the renaming of the parameter in this
patch is an improvement.
> @@ -688,51 +693,71 @@ static void process_command_line_args(struct list_head *arg_head,
> -static struct strbuf **read_input_file(const char *file)
> +static void read_input_file(struct strbuf *sb, const char *file)
> {
> - struct strbuf **lines;
> - struct strbuf sb = STRBUF_INIT;
> -
> if (file) {
> - if (strbuf_read_file(&sb, file, 0) < 0)
> + if (strbuf_read_file(sb, file, 0) < 0)
> die_errno(_("could not read input file '%s'"), file);
> } else {
> - if (strbuf_read(&sb, fileno(stdin), 0) < 0)
> + if (strbuf_read(sb, fileno(stdin), 0) < 0)
> die_errno(_("could not read from stdin"));
> }
> +}
The original used to return an array of strbufs but we now just read
into a single strbuf. The caller may need to do more, or perhaps
the early split into array of strbufs was mostly wasteful use of
more flexible data structure. We'll find out when we read what
happens to the data read here.
> +static const char *next_line(const char *str)
> +{
> + const char *nl = strchrnul(str, '\n');
> + return nl + !!*nl;
> +}
"next_line()" gives a pointer to either the beginning of the next
line, or points at the NUL that terminates the whole buffer. OK.
> +/*
> + * Return the position of the start of the last line. If len is 0, return -1.
> + */
> +static int last_line(const char *buf, size_t len)
> +{
> + int i;
> + if (len == 0)
> + return -1;
> + if (len == 1)
> + return 0;
> + /*
> + * Skip the last character (in addition to the null terminator),
> + * because if the last character is a newline, it is considered as part
> + * of the last line anyway.
> + */
> + i = len - 2;
... and if the last character is not a newline, the line is an
incomplete last line. OK.
> - return lines;
> + for (; i >= 0; i--) {
> + if (buf[i] == '\n')
> + return i + 1;
> + }
> + return 0;
> }
OK.
> /*
> - * Return the (0 based) index of the start of the patch or the line
> - * count if there is no patch in the message.
> + * Return the position of the start of the patch or the length of str if there
> + * is no patch in the message.
> */
> -static int find_patch_start(struct strbuf **lines, int count)
> +static int find_patch_start(const char *str)
> {
> - int i;
> + const char *s;
>
> - /* Get the start of the patch part if any */
> - for (i = 0; i < count; i++) {
> - if (starts_with(lines[i]->buf, "---"))
> - return i;
> + for (s = str; *s; s = next_line(s)) {
> + if (starts_with(s, "---"))
> + return s - str;
> }
>
> - return count;
> + return s - str;
> }
The original assumed that the input was first split into an array of
strbufs and found the index that begins with a line "---". We have
a flat buffer of input, and find a line that begins with "---". The
returned value is different (i.e. used to be line-number, now it is
the byte offset). OK.
> /*
> - * Return the (0 based) index of the first trailer line or count if
> - * there are no trailers. Trailers are searched only in the lines from
> - * index (count - 1) down to index 0.
> + * Return the position of the first trailer line or len if there are no
> + * trailers.
> */
OK, the idea is the same as the above; it used to be line-based, but
now it is counted in byte-offset.
> -static int find_trailer_start(struct strbuf **lines, int count)
> +static int find_trailer_start(const char *buf, size_t len)
> {
> - int start, end_of_title, only_spaces = 1;
> + const char *s;
> + int title_end, l, only_spaces = 1;
between end-of-title and title-end, I have slight preference for the
former over the latter, but just like separators vs interests, this
rename is probably more distracting than is worth.
> int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
> /*
> * Number of possible continuation lines encountered. This will be
> @@ -742,15 +767,18 @@ static int find_trailer_start(struct strbuf **lines, int count)
> * are to be considered non-trailers).
> */
> int possible_continuation_lines = 0;
> + int ret;
> +
> + char *sep_nl = xstrfmt("%s\n", separators);
>
> /* The first paragraph is the title and cannot be trailers */
> - for (start = 0; start < count; start++) {
> - if (lines[start]->buf[0] == comment_line_char)
> + for (s = buf; s < buf + len; s = next_line(s)) {
> + if (s[0] == comment_line_char)
> continue;
> - if (contains_only_spaces(lines[start]->buf))
> + if (is_blank_line(s))
> break;
> }
> - end_of_title = start;
> + title_end = s - buf;
OK, we skipped to find the first blank line. The original had an
array of strbufs to iterate over and didn't have to look for LF but
now we do, but it is just the matter of calling next_line() that is
simple enough.
> @@ -758,30 +786,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
> * trailers, or (ii) contains at least one Git-generated trailer and
> * consists of at least 25% trailers.
> */
> - for (start = count - 1; start >= end_of_title; start--) {
> + for (l = last_line(buf, len); l >= title_end; l = last_line(buf, l)) {
The iteration is conceptually the same. We used to iterate from the
last line in the input down to the first blank line that ends the
title paragraph. We do the same but now we are byte-offset based,
so we start from last_line() of the entire buffer, and go back
one-line-at-a-time by calling last_line(buf, l). OK.
> + const char *bol = buf + l;
> const char **p;
> int separator_pos;
>
> - if (lines[start]->buf[0] == comment_line_char) {
> + if (bol[0] == comment_line_char) {
> non_trailer_lines += possible_continuation_lines;
> possible_continuation_lines = 0;
> continue;
> }
> - if (contains_only_spaces(lines[start]->buf)) {
> + if (is_blank_line(bol)) {
> if (only_spaces)
> continue;
> non_trailer_lines += possible_continuation_lines;
> if (recognized_prefix &&
> trailer_lines * 3 >= non_trailer_lines)
> - return start + 1;
> - if (trailer_lines && !non_trailer_lines)
> - return start + 1;
> - return count;
> + ret = next_line(bol) - buf;
The original called the line we are looking at as "start"; the
corresponding byte-offset is "l", and "bol" is the pointer to the
beginning of the current line. Returning next_line(bol)-buf is
equivalent to returning "start + 1" in the original. OK.
> + else if (trailer_lines && !non_trailer_lines)
> + ret = next_line(bol) - buf;
Ditto.
> + else
> + ret = len;
Ditto.
> + goto cleanup;
We are no longer "return"ing directly from this part of the code,
and instead saving the return value in "ret" and jumping there where
the real "return" is. We need cleanup because we have an extra
allocation of separator + LF thing up above.
> ...
The changes to the remainder of this function shows that the
original lines[] were accessed read-only and it only used
lines[]->buf without lines[]->len, which is a strong indication that
an array of strbuf was an overkill.
The rewrite looks to be a logical equivalent, which is good.
> @@ -817,88 +848,73 @@ static int find_trailer_start(struct strbuf **lines, int count)
> ...
> -/* Get the index of the end of the trailers */
> -static int find_trailer_end(struct strbuf **lines, int patch_start)
> +/* Return the position of the end of the trailers. */
> +static int find_trailer_end(const char *buf, size_t len)
> {
> - struct strbuf sb = STRBUF_INIT;
> - int i, ignore_bytes;
> -
> - for (i = 0; i < patch_start; i++)
> - strbuf_addbuf(&sb, lines[i]);
> - ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
> - strbuf_release(&sb);
> - for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
> - ignore_bytes -= lines[i]->len;
> -
> - return i + 1;
That indeed was grossly wasteful. Things split into lines[] are
concatenated back only to call a single helper function that reports
a byte offset, and then lines[]->len is counted separately to find
the line that the byte offset falls in.
> + return len - ignore_non_trailer(buf, len);
Using the byte-offset based data structure throughout the code makes
it a lot simpler. Good.
> }
>
> -static int has_blank_line_before(struct strbuf **lines, int start)
> +static int ends_with_blank_line(const char *buf, size_t len)
> {
> - for (;start >= 0; start--) {
> - if (lines[start]->buf[0] == comment_line_char)
> - continue;
> - return contains_only_spaces(lines[start]->buf);
> - }
> - return 0;
> -}
> -
> -static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
> -{
> - int i;
> - for (i = start; lines[i] && i < end; i++)
> - fprintf(outfile, "%s", lines[i]->buf);
> + int ll = last_line(buf, len);
> + if (ll < 0)
> + return 0;
> + return is_blank_line(buf + ll);
> }
Two helpers "has-blank-line-before" and "print-lines" are gone.
Let's see how updated code does things without them.
> static int process_input_file(FILE *outfile,
> - struct strbuf **lines,
> + const char *str,
> struct list_head *head)
> {
> - int count = 0;
> - int patch_start, trailer_start, trailer_end, i;
> + int patch_start, trailer_start, trailer_end;
> struct strbuf tok = STRBUF_INIT;
> struct strbuf val = STRBUF_INIT;
> struct trailer_item *last = NULL;
> + struct strbuf *trailer, **trailer_lines, **ptr;
>
> - /* Get the line count */
> - while (lines[count])
> - count++;
> -
> - patch_start = find_patch_start(lines, count);
> - trailer_end = find_trailer_end(lines, patch_start);
> - trailer_start = find_trailer_start(lines, trailer_end);
> + patch_start = find_patch_start(str);
> + trailer_end = find_trailer_end(str, patch_start);
> + trailer_start = find_trailer_start(str, trailer_end);
>
> /* Print lines before the trailers as is */
> - print_lines(outfile, lines, 0, trailer_start);
> + fwrite(str, 1, trailer_start, outfile);
Ah, of course, if you don't split into an array of strbuf then you
do not need a helper that iterates over the array and prints.
> - if (!has_blank_line_before(lines, trailer_start - 1))
> + if (!ends_with_blank_line(str, trailer_start))
> fprintf(outfile, "\n");
Then if the part before the trailer doesn't end with a blank, we
force a blank.
This is not a new issue, but this makes me wonder what happens when
there is no trailer to emit after this. Do we end up with an extra
blank line?
> /* Parse trailer lines */
> - for (i = trailer_start; i < trailer_end; i++) {
> + trailer_lines = strbuf_split_buf(str + trailer_start,
> + trailer_end - trailer_start,
> + '\n',
> + 0);
We want each line NUL terminated while going over the trailer part
line-by-line so that it is easy to do _addf(&buf, "%s"). Use of
strbuf_split_buf() to split them into an array of strbufs is close
to the way the original did it, so that is why it is done here.
OK. If we had a helper function that would split into an array of
"const char *"s, I suspect it would have worked equally well. In
the loop, we only look at ->buf of the strbuf instances and do not
take advantage of the fact that the length of each line is known.
But that is better left as a further clean-up to the future.
Thanks. This step looks like a regression-free rewrite.
^ permalink raw reply
* Re: [PATCH] push: do not use potentially ambiguous default refspec
From: Dennis Kaarsemaker @ 2016-10-31 20:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Kannan Goundan
In-Reply-To: <xmqqvawcz36d.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 28, 2016 at 12:25:30PM -0700, Junio C Hamano wrote:
> * It is appreciated if somebody with spare cycles can add a test or
> two for this in t/t5523-push-upstream.sh or somewhere nearby.
5523 is for push --set-upstream-to, 5528 seemed more appropriate. Here's
something squashable that fails before your patch and succeeds after.
>8----
Subject: [PATCH] push: test pushing ambiguously named branches
Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
t/t5528-push-default.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/t5528-push-default.sh b/t/t5528-push-default.sh
index 73f4bb6..ac103ce 100755
--- a/t/t5528-push-default.sh
+++ b/t/t5528-push-default.sh
@@ -98,6 +98,16 @@ test_expect_success 'push from/to new branch with upstream, matching and simple'
test_push_failure upstream
'
+test_expect_success 'push ambiguously named branch with upstream, matching and simple' '
+ git checkout -b ambiguous &&
+ test_config branch.ambiguous.remote parent1 &&
+ test_config branch.ambiguous.merge refs/heads/ambiguous &&
+ git tag ambiguous &&
+ test_push_success simple ambiguous &&
+ test_push_success matching ambiguous &&
+ test_push_success upstream ambiguous
+'
+
test_expect_success 'push from/to new branch with current creates remote branch' '
test_config branch.new-branch.remote repo1 &&
git checkout new-branch &&
--
2.10.1-449-gab0f84c
^ permalink raw reply related
* Re: [PATCH] rebase: add --forget to cleanup rebase, leave HEAD untouched
From: Junio C Hamano @ 2016-10-31 19:25 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Git Mailing List
In-Reply-To: <CACsJy8CSz-2A56okV6kWBjGqUgiL7DrmmVJ=2jEQhKmqe41cRg@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Wed, Oct 26, 2016 at 11:51 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>>
>>> There are occasions when you decide to abort an in-progress rebase and
>>> move on to do something else but you forget to do "git rebase --abort"
>>> first. Or the rebase has been in progress for so long you forgot about
>>> it. By the time you realize that (e.g. by starting another rebase)
>>> it's already too late to retrace your steps. The solution is normally
>>>
>>> rm -r .git/<some rebase dir>
>>>
>>> and continue with your life. But there could be two different
>>> directories for <some rebase dir> (and it obviously requires some
>>> knowledge of how rebase works), and the ".git" part could be much
>>> longer if you are not at top-dir, or in a linked worktree. And
>>> "rm -r" is very dangerous to do in .git, a mistake in there could
>>> destroy object database or other important data.
>>>
>>> Provide "git rebase --forget" for this exact use case.
>>
>> Two and a half comments.
>>
>> - The title says "leave HEAD untouched". Are my working tree files
>> and my index also safe from this operation, or is HEAD the only
>> thing that is protected?
>
> Everything is protected. I will rephrase the title a bit. The option
> is basically a safe form of "rm -r .git/rebase-{apply,merge}".
We are not in a hurry, as it is not likely that this will hit 2.11
even if we saw a rerolled version yesterday, but it would be nice to
cook it on 'next' so that it can be on 'master' early after the
upcoming release.
Thanks.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-31 18:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Johannes Schindelin, Git Mailing List,
Lars Schneider, Eric Wong
In-Reply-To: <xmqqwpgotncb.fsf@gitster.mtv.corp.google.com>
On Mon, Oct 31, 2016 at 10:55:32AM -0700, Junio C Hamano wrote:
> > So I guess it's possible that it produces a noticeable effect in some
> > cases, but I'm still somewhat doubtful. And actually repacking your
> > repository had a greater effect in every case I measured (in addition to
> > providing other speedups).
>
> Let's keep doubting. I prefer one-step-at-a-time approach to
> things anyway, and what I plan in the near term are:
>
> * use the "open() with O_NOATIME|O_CLOEXEC, gradually losing the
> bits during fallback" approach in the ls/git-open-cloexec topic,
> in order to help ls/filter-process topic be part of the upcoming
> release;
>
> * simplify the logic to the "open(2) with O_CLOEXEC, set O_NOATIME
> with fcntl(2)" in jc/git-open-cloexec~1 after 2.11 ships;
>
> * cook "drop the latter half of setting O_NOATIME" which is at the
> tip of jc/git-open-cloexec in 'next', and while Linus is looking
> the other way ^W^W^W^W^W^W^W after people had chance to complain
> with numbers, merge it to a future release iff it still looked OK
> to drop O_NOATIME thing.
Great, that sounds like a good way to proceed (and if the final step
never happens, no big deal).
-Peff
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-31 17:55 UTC (permalink / raw)
To: Jeff King
Cc: Linus Torvalds, Johannes Schindelin, Git Mailing List,
Lars Schneider, Eric Wong
In-Reply-To: <20161031135601.7immbp44wn7uksvs@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> If you set a probe on touch_atime() in the kernel (which is called for
> every attempt to smudge the atime, regardless of mount options, but is
> skipped when the descriptor was opened with O_NOATIME), you can see the
> impact. Here's a command I picked because it reads a lot of objects (run
> on my git.git clone):
>
> $ perf stat -e probe:touch-atime git log -Sfoo >/dev/null
>
> And the probe:touch_atime counts before (stock git) and after (a patch
> to drop O_NOATIME):
>
> before: 22,235
> after: 22,362
>
> So that's only half a percent difference. And it's on a reasonably messy
> clone that is partway to triggering an auto-repack:
> ...
> So I guess it's possible that it produces a noticeable effect in some
> cases, but I'm still somewhat doubtful. And actually repacking your
> repository had a greater effect in every case I measured (in addition to
> providing other speedups).
Let's keep doubting. I prefer one-step-at-a-time approach to
things anyway, and what I plan in the near term are:
* use the "open() with O_NOATIME|O_CLOEXEC, gradually losing the
bits during fallback" approach in the ls/git-open-cloexec topic,
in order to help ls/filter-process topic be part of the upcoming
release;
* simplify the logic to the "open(2) with O_CLOEXEC, set O_NOATIME
with fcntl(2)" in jc/git-open-cloexec~1 after 2.11 ships;
* cook "drop the latter half of setting O_NOATIME" which is at the
tip of jc/git-open-cloexec in 'next', and while Linus is looking
the other way ^W^W^W^W^W^W^W after people had chance to complain
with numbers, merge it to a future release iff it still looked OK
to drop O_NOATIME thing.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Junio C Hamano @ 2016-10-31 17:37 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Jeff King, Git Mailing List, Lars Schneider,
Eric Wong
In-Reply-To: <CA+55aFxsjiuR8cp9SiPS88OnzmCiNN3B-gybz1CS71avsU8OOw@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Sat, Oct 29, 2016 at 1:25 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>>
>> Correct. We cannot change an open file handle's state ("FD_CLOEXEC") on
>> Windows, but we can ask open() to open said file handle with the correct
>> flag ("O_CLOEXEC", which is mapped to O_NOINHERIT on Windows.
>
> Ok. So then I have no issues with it, and let's use O_CLOEXEC if it
> exists and fcntl(FD_CLOEXEC) if O_CLOEXEC doesn't exist.
I am not sure if the fallback to FD_CLOEXEC is worth doing, because
it does not exist on Windows, where leaking an open file descriptor
to a child process matters more than other platforms [*1*].
Of course, the world is not all Windows, and there may be other
platforms this fallback may help. But there is a chance that some
other thread may fork between the time we open(2) and the time we
call fcntl(2), leaving FD_CLOEXEC ineffective and unreliable, so
that's another thing that makes me doubt if FD_CLOEXEC fallback is
worth doing.
Having said that, here is a rewrite of [*2*] to use the fallback
would look like. After this topic settles, we may want to also
update the tempfile.c::create_tempfile() implementation to use the
helper function we use here.
[Footnotes]
*1* The call in ce_compare_data() is to see if the contents in the
working tree file match what is in the index. Perhaps the
program is "git checkout another-branch" that knows this path is
absent in the branch being switched to and trying to make sure
we lose no local modification. When the path needs to be passed
through the clean/smudge filter before hashing to see if the
working tree contents hashes to the object in the index, we need
to fork and then after the filter finishes its processing, we
close the file descriptor and then unlink(2) the path if it is
clean. We are about to add a variant of clean/smudge mechanism
where a lazily spawned process can clean contents of multiple
paths and that is where cloexec starts to matter. This function
starts to inspect one path, opens a fd to it, finds that it
needs filtering, spawns a long-lingering process, while leaking
the original fd to it. After feeding the contents via pipe and
then receiving the filtered contents (the protocol is not full
duplex and there won't be a stuck pipe), the parent would decide
that the path is clean and attempts to unlink(2). Windows does
not let you unlink a path with open filedescriptor held, causing
"checkout" to fail. As it is only one fd that leaks to the
child, no matter how many subsequent paths are filtered by the
same long-lingering child process, it will be a far less
important issue on other platforms that lets you unlink(2) such
a file.
*2* http://public-inbox.org/git/xmqqh97w38gj.fsf@gitster.mtv.corp.google.com
---
cache.h | 1 +
git-compat-util.h | 4 ++++
read-cache.c | 9 +--------
sha1_file.c | 47 ++++++++++++++++++++++++++++-------------------
4 files changed, 34 insertions(+), 27 deletions(-)
diff --git a/cache.h b/cache.h
index a902ca1f8e..6f1c21a352 100644
--- a/cache.h
+++ b/cache.h
@@ -1122,6 +1122,7 @@ extern int write_sha1_file(const void *buf, unsigned long len, const char *type,
extern int hash_sha1_file_literally(const void *buf, unsigned long len, const char *type, unsigned char *sha1, unsigned flags);
extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);
extern int force_object_loose(const unsigned char *sha1, time_t mtime);
+extern int git_open_cloexec(const char *name, int flags);
extern int git_open(const char *name);
extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);
extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);
diff --git a/git-compat-util.h b/git-compat-util.h
index 43718dabae..64407c701c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -683,6 +683,10 @@ char *gitstrdup(const char *s);
#define O_CLOEXEC 0
#endif
+#ifndef FD_CLOEXEC
+#define FD_CLOEXEC 0
+#endif
+
#ifdef FREAD_READS_DIRECTORIES
#ifdef fopen
#undef fopen
diff --git a/read-cache.c b/read-cache.c
index db5d910642..c27d3e240b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -156,14 +156,7 @@ void fill_stat_cache_info(struct cache_entry *ce, struct stat *st)
static int ce_compare_data(const struct cache_entry *ce, struct stat *st)
{
int match = -1;
- static int cloexec = O_CLOEXEC;
- int fd = open(ce->name, O_RDONLY | cloexec);
-
- if ((cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
- /* Try again w/o O_CLOEXEC: the kernel might not support it */
- cloexec &= ~O_CLOEXEC;
- fd = open(ce->name, O_RDONLY | cloexec);
- }
+ int fd = git_open_cloexec(ce->name, O_RDONLY);
if (fd >= 0) {
unsigned char sha1[20];
diff --git a/sha1_file.c b/sha1_file.c
index 09045df1dc..fc8613a847 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1559,31 +1559,40 @@ int check_sha1_signature(const unsigned char *sha1, void *map,
return hashcmp(sha1, real_sha1) ? -1 : 0;
}
-int git_open(const char *name)
+int git_open_cloexec(const char *name, int flags)
{
- static int sha1_file_open_flag = O_NOATIME | O_CLOEXEC;
+ int fd;
+ static int o_cloexec = O_CLOEXEC;
+ static int fd_cloexec = FD_CLOEXEC;
- for (;;) {
- int fd;
+ fd = open(name, flags | o_cloexec);
+ if ((o_cloexec & O_CLOEXEC) && fd < 0 && errno == EINVAL) {
+ /* Try again w/o O_CLOEXEC: the kernel might not support it */
+ o_cloexec &= ~O_CLOEXEC;
+ fd = open(name, flags | o_cloexec);
+ }
- errno = 0;
- fd = open(name, O_RDONLY | sha1_file_open_flag);
- if (fd >= 0)
- return fd;
+ if (!o_cloexec && 0 <= fd && fd_cloexec) {
+ /* Opened w/o O_CLOEXEC? try with fcntl(2) to add it */
+ int flags = fcntl(fd, F_GETFL);
+ if (fcntl(fd, F_SETFL, flags | fd_cloexec))
+ fd_cloexec = 0;
+ }
- /* Try again w/o O_CLOEXEC: the kernel might not support it */
- if ((sha1_file_open_flag & O_CLOEXEC) && errno == EINVAL) {
- sha1_file_open_flag &= ~O_CLOEXEC;
- continue;
- }
+ return fd;
+}
- /* Might the failure be due to O_NOATIME? */
- if (errno != ENOENT && (sha1_file_open_flag & O_NOATIME)) {
- sha1_file_open_flag &= ~O_NOATIME;
- continue;
- }
- return -1;
+int git_open(const char *name)
+{
+ static int noatime = O_NOATIME;
+ int fd = git_open_cloexec(name, O_RDONLY);
+
+ if (0 <= fd && (noatime & O_NOATIME)) {
+ int flags = fcntl(fd, F_GETFL);
+ if (fcntl(fd, F_SETFL, flags | noatime))
+ noatime = 0;
}
+ return fd;
}
static int stat_sha1_file(const unsigned char *sha1, struct stat *st)
^ permalink raw reply related
* Re: Reporting Bug in Git Version Control System
From: Jakub Narębski @ 2016-10-31 16:09 UTC (permalink / raw)
To: Pranit Bauva, Yash Jain; +Cc: Git List
In-Reply-To: <CAFZEwPNB1PX_NOaYzKr+rUKvbC9s5A1G58v5aS=sGdzPVsv4ww@mail.gmail.com>
W dniu 25.10.2016 o 10:51, Pranit Bauva pisze:
> Hey Yash,
>
> Junio has explained the problem very well. Since you seem to be a
> beginner (guessing purely by your email) I will tell you how to fix
> it.
>
> Remember when you would have first installed git, you would have done
> something like `git config --global user.name <what ever name>` and
> `git config --global user.email <what ever email>`, it gets
> permanently stored in the git configuration file (~/.gitconfig). Now
> all the commits in git are made with this name and email. If you want
> to change this, again run the above commands with your new name and
> email. After that commits will be done with a different name and
> email. Hope this helps! :)
First, per user Git configuration doesn't necessarily go into ~/.gitconfig;
it could be in ~/.config/git/config
Second, you can configure user.email and user.name with `git config` or
`git config --local`. The information would go into .git/config for
specific repository. This might be a better solution if you want
different settings for different repositories.
--
Jakub Narębski
^ permalink raw reply
* Re: [git rebase -i] show time and author besides commit hash and message?
From: Jeff King @ 2016-10-31 15:57 UTC (permalink / raw)
To: ryenus; +Cc: Alexei Lozovsky, Git mailing list
In-Reply-To: <CAKkAvay4YuRuGhub6W308+DmrZtrO3+NWu8NvfB--Mb9Z59Xzw@mail.gmail.com>
On Mon, Oct 31, 2016 at 11:27:33PM +0800, ryenus wrote:
> > It is possible to change the format globally via config option
> > rebase.instructionFormat:
> >
> > $ git config rebase.instructionFormat "%an (%ad): %s"
> >
> > The format is the same as for 'git log'. This one outputs author
> > name, date, and the first line of commit message.
>
> Thanks for the prompt response!
> I tried immediately, it works just as you have pointed out.
>
> Just it seems coloring is not supported? probably because
> we're inside an editor?
Yes. If git outputs its own ANSI codes there, they generally look bad in
an editor (interpreted as raw bytes, not as coloring). Any existing
coloring you see is likely due to syntax highlighting done by your
editor. You can extend that to match your desired format, but exactly
how would depend on which editor you're using.
-Peff
^ permalink raw reply
* Re: [git rebase -i] show time and author besides commit hash and message?
From: ryenus @ 2016-10-31 15:27 UTC (permalink / raw)
To: Alexei Lozovsky; +Cc: Git mailing list
In-Reply-To: <CALhvvbYJ8G12Lbe2FgP8PWKZ-LABcw2M-M-zWPkT12UUqq1vaw@mail.gmail.com>
> It is possible to change the format globally via config option
> rebase.instructionFormat:
>
> $ git config rebase.instructionFormat "%an (%ad): %s"
>
> The format is the same as for 'git log'. This one outputs author
> name, date, and the first line of commit message.
Thanks for the prompt response!
I tried immediately, it works just as you have pointed out.
Just it seems coloring is not supported? probably because
we're inside an editor?
> This option is supported since Git 2.6.
I missed it, what about including a note about this option as
part of the comment in git rebase -i? since that's the place where
people would most likely think about it?
> Or are you interested in a command-line option that can change
> the format on per-invocation basis? I think there isn't one.
> It can be interesting to add it, but I don't think it has much
> utility...
Not much, for git log I'd rather setup an alias "git la", than having to
remember various fields, colors, padding options to have a nice output.
^ permalink raw reply
* Re: [PATCH v3 2/3] sha1_file: open window into packfiles with O_CLOEXEC
From: Jeff King @ 2016-10-31 13:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: Johannes Schindelin, Junio C Hamano, Git Mailing List,
Lars Schneider, Eric Wong
In-Reply-To: <CA+55aFw93vkraxBvFCXFSYJqn836tXW+OCOFuToN+HaxTcJ7cg@mail.gmail.com>
On Fri, Oct 28, 2016 at 09:13:41AM -0700, Linus Torvalds wrote:
> On Fri, Oct 28, 2016 at 4:11 AM, Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > You guys. I mean: You guys! You sure make my life hard. A brief look at
> > mingw.h could have answered your implicit question:
>
> So here's what you guys should do:
>
> - leave O_NOATIME damn well alone. It works. It has worked for 10+
> years. Stop arguing against it, people who do.
For some definition of worked, perhaps.
If you set a probe on touch_atime() in the kernel (which is called for
every attempt to smudge the atime, regardless of mount options, but is
skipped when the descriptor was opened with O_NOATIME), you can see the
impact. Here's a command I picked because it reads a lot of objects (run
on my git.git clone):
$ perf stat -e probe:touch-atime git log -Sfoo >/dev/null
And the probe:touch_atime counts before (stock git) and after (a patch
to drop O_NOATIME):
before: 22,235
after: 22,362
So that's only half a percent difference. And it's on a reasonably messy
clone that is partway to triggering an auto-repack:
$ git count-objects -v
count: 6167
size: 61128
in-pack: 275773
packs: 18
size-pack: 86857
prune-packable: 25
garbage: 0
size-garbage: 0
Running "git gc" drops the probe count to 21,733.
It makes a bigger difference for some commands (it's more like 10% for
git-status). And smaller for others ("git log -p" triggers it over
100,000 times).
One thing missing in that count is how many of those calls would have
resulted in an actual disk write. Looking at strace, most of the
filesystem activity is opening .gitattributes files, and we end up
opening the same ones repeatedly (e.g., t/.gitattributes in git.git).
Multiple hits for a given inode in the same second get coalesced into at
most a single disk write.
So I guess it's possible that it produces a noticeable effect in some
cases, but I'm still somewhat doubtful. And actually repacking your
repository had a greater effect in every case I measured (in addition to
providing other speedups).
Like I said, I'm OK keeping O_NOATIME. It's just not that much code. But
if you really care about the issue of dirtying inodes via atime, you
should look into vastly increasing our use of O_NOATIME. Or possibly
looking at caching more in the attribute code.
-Peff
^ permalink raw reply
* Re: Is the entire working copy “at one branch”?
From: Stefan Monov @ 2016-10-31 10:41 UTC (permalink / raw)
To: Alexei Lozovsky; +Cc: git
In-Reply-To: <CALhvvbYE6Tt3eByDVMB3a4t=nm3dScVZSea0Z1SsKVgwFSiQ-w@mail.gmail.com>
Thanks Alexei!
On Sat, Oct 29, 2016 at 12:47 PM, Alexei Lozovsky <a.lozovsky@gmail.com> wrote:
> Hi Stefan,
>
> Generally with git, your entire working copy will have the same
> revision (set to current branch, aka HEAD). The idea behind this
> is that your working copy of a repository should always be in
> consistent state.
>
> You can check out specific files or directories from another
> revision (mimicking "svn update -r1234 filename"):
>
> $ git checkout branch-or-sha-hash -- filename
>
> However, SVN tracks the 'revision' thing on per-file basis, while
> in git this is a property of the working copy. So if you do like
> above then git will be telling you that the 'filename' has been
> changed (as it is certainly different from its pristine version
> in HEAD):
>
> $ git status
> On branch master
> Changes to be committed:
> (use "git reset HEAD <file>..." to unstage)
>
> modified: filename
>
> So it's generally not recommended to do such a thing.
>
> Another thing that you _can do_ in git to mimick SVN is the
> 'standard layout'. There is a feature called "git worktree" which
> allows you to have SVN-like directory structure with multiple
> directories linked to different working copies:
>
> $ mkdir my-project
> $ cd my-project
> $ git clone my-project-repository master
> $ mkdir branches
> $ cd master
> $ git worktree add -b branch-1 ../branches/branch-1
> $ git worktree add -b branch-2 ../branches/branch-2
>
> After that you will have directory structure like this:
>
> $ tree my-project
> my-project
> ├── branches
> │ ├── branch-1
> │ │ ├── 1
> │ │ ├── 2
> │ │ └── 3
> │ └── branch-2
> │ ├── 1
> │ ├── 2
> │ └── banana
> └── master
> ├── 1
> └── 2
> You can work with these working copies separately, like you
> would be working with SVN. Commits in 'master' will go to the
> 'master' branch, commits made in 'branches/branch-1' will go
> to the 'branch-1' branch.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Anders Kaseorg @ 2016-10-31 0:30 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, Philip Oakley, Junio C Hamano, 842477,
Git Mailing List, Vasco Almeida
In-Reply-To: <CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com>
On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:
> This did break in v2.10.0, and it's taken a couple of months to notice
> this, so clearly it's not very widely used, which says something about
> the cost-benefit of maintaining this for external users.
For the record, in case this affects the calculation, it was noticed that
guilt was broken a just couple of days after the first git 2.10.x upload
to Debian, which was last weekend.
https://bugs.debian.org/842477
http://repo.or.cz/guilt.git/blob/v0.36:/guilt#l28
(I have no further opinion; I trust that Junio has all the information
needed to decide one way or the other.)
Anders
^ permalink raw reply
* Re: [PATCH] commit: simplify building parents list
From: Junio C Hamano @ 2016-10-30 23:03 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List
In-Reply-To: <30afdbeb-7a45-70b2-495e-35fd3b62419a@web.de>
René Scharfe <l.s.r@web.de> writes:
> Push pptr down into the FROM_MERGE branch of the if/else statement,
> where it's actually used, and call commit_list_append() for appending
> elements instead of playing tricks with commit_list_insert(). Call
> copy_commit_list() in the amend branch instead of open-coding it. Don't
> bother setting pptr in the final branch as it's not used thereafter.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> ...
> @@ -1729,7 +1727,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
> reflog_msg = (whence == FROM_CHERRY_PICK)
> ? "commit (cherry-pick)"
> : "commit";
> - pptr = &commit_list_insert(current_head, pptr)->next;
> + commit_list_insert(current_head, &parents);
> }
I needed to read the full preimage to determine why this hunk is
equivalent to the original. Which is a good demonstration that what
motivated this patch is a valid issue to tackle---initializing the
pptr variable to point at &parents too early and have the long
if/elseif/... cascade work with it made the code unnecessarily
harder to understand and this update untangles that.
Thanks.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Junio C Hamano @ 2016-10-30 22:53 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Jeff King, Philip Oakley, Anders Kaseorg, 842477,
Git Mailing List, Vasco Almeida
In-Reply-To: <CACBZZX6ArQdG202n-SouwDhoTE1LF=69mKjWQv8HPKJ+K_0fJQ@mail.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
(commenting out of order)
> It's probably worthwhile to split off git-sh-setup into git-sh-setup &
> git-sh-setup-internal along with a documentation fix. A lot of what
> it's doing (e.g. git_broken_path_fix(), and adding a die() function)
> is probably only needed internally by git itself. The
> git-sh-setup-internal should be the thing sourcing "git-sh-i18n", I
> don't see how anyone out-of-tree could make use of that. Surely nobody
> needs to re-emit the exact message we shipped with our *.po files.
My reading of d323c6b641 ("i18n: git-sh-setup.sh: mark strings for
translation", 2016-06-17) is abit different. It needs to dot-source
the i18n stuff because the shell library functions it contains need
the localization support in the messages they emit. IOW, I do not
think i18n belongs to -internal at all.
> I don't see why we shouldn't have some stable shellscript function API
> if that's needed either.
>
> I just wanted to point out that currently git-sh-setup isn't
> documented as such. So at least a follow-up patch to the documentation
> seems in order.
>
> This did break in v2.10.0, and it's taken a couple of months to notice
> this, so clearly it's not very widely used, which says something about
> the cost-benefit of maintaining this for external users.
I am not sure if "stable API" in sh-setup is a good thing for the
ecosystem in the longer term.
As more and more in-tree scripted Porcelain commands migrate to C,
many helper functions in sh-setup will lose their in-tree users.
For example, get_author_ident_from_commit used to have three in-tree
customers (git-commit.sh, git-am.sh and git-rebase--interactive.sh),
but the first two is long gone and the third one may soon lose its
need to call it. Once a helper function in setup-sh loses all
in-tree users, we may no longer _break_ that helper, but that is
simply because we feel no need to touch it. The in-tree Porcelain
commands that migrated to C however will enhance the counterpart
they use in C to be more featureful or fix longstanding bugs in the
C version they use, while sh-setup version bitrot and making old
practice obsolete for "modern" use of Git of the day.
Keeping such a stale version that we do not use, or even we attempt
to update it without having a good vehicle to test the change
ourselves (because we no longer have any in-tree users) will be
disservice to third-party scripts---the only thing they are getting
by using the stale one, instead of reinventing their own that they
may be responsible to keep up to date, is that they share the same
staleness as everybody else that use the sh-setup version as a
third-party.
I am not arguing that we should remove what loses all in-tree users
immediately. At least not yet. But I wanted to point out that it
may not be a good use of our brain cycles to keep the API "stable"
by keeping what in-tree users do not use anymore, especially if it
does not help third-party users in the long run.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Junio C Hamano @ 2016-10-30 22:25 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: 842477, git, Vasco Almeida
In-Reply-To: <alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu>
Anders Kaseorg <andersk@mit.edu> writes:
> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
> translation” broke outside scripts such as guilt that source
> git-sh-setup as described in the documentation:
>
> $ . "$(git --exec-path)/git-sh-setup"
> sh: 6: .: git-sh-i18n: not found
>
> This also affects contrib/convert-grafts-to-replace-refs.sh and
> contrib/rerere-train.sh in tree. Fix this by using git --exec-path to
> find git-sh-i18n.
>
> While we’re here, move the sourcing of git-sh-i18n below the shell
> portability fixes.
>
> Signed-off-by: Anders Kaseorg <andersk@mit.edu>
> ---
Looks good.
Our in-tree scripts rely on the fact that $PATH is adjusted to have
$GIT_EXEC_PATH early (either by getting invoked indirectly by "git"
potty, or the requirement to do so for people and scripts that still
run our in-tree scripts with dashed e.g. "git-rebase" form) by the
time they run. But when sh-setup dot-sources git-sh-i18n for its
own use, it should be explicit to name which one of the many copies
that may appear in directories on user's $PATH (one among which is
the one in $GIT_EXEC_PATH) it wants to use. And this patch does the
right thing by not relying on the $PATH, but instead naming the
exact path using $(git --exec-path)/ prefix, to the included file.
In other words, I think this patch is a pure bugfix, even if there
is no third-party script that includes it. We may want to have the
above as the rationale to apply this patch in the proposed log
message, though.
> Is this a supported use of git-sh-setup? Although the documentation is
> clear that the end user should not invoke it directly, it seems to imply
> that scripts may do this, and in practice it has worked until v2.10.0.
It is correct for the documentation to say that this is not a
"command" end users would want to run; they cannot invoke it as a
standalone command as it is written as a dot-sourced shell library.
Even though it is intended solely for internal use, so far we have
not removed things from there, which would have signalled people
that third-party scripts can also dot-source it. We may want to
reserve the right to break them in the future, but because this is a
pure bugfix, "can third-party rely on the interface not changing?"
is not a question we need to answer in this thread---there is no
reason to leave this broken.
Thanks.
> git-sh-setup.sh | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index a8a4576..240c7eb 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -2,9 +2,6 @@
> # to set up some variables pointing at the normal git directories and
> # a few helper shell functions.
>
> -# Source git-sh-i18n for gettext support.
> -. git-sh-i18n
> -
> # Having this variable in your environment would break scripts because
> # you would cause "cd" to be taken to unexpected places. If you
> # like CDPATH, define it for your interactive shell sessions without
> @@ -46,6 +43,9 @@ git_broken_path_fix () {
>
> # @@BROKEN_PATH_FIX@@
>
> +# Source git-sh-i18n for gettext support.
> +. "$(git --exec-path)/git-sh-i18n"
> +
> die () {
> die_with_status 1 "$@"
> }
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Ævar Arnfjörð Bjarmason @ 2016-10-30 22:11 UTC (permalink / raw)
To: Jeff King
Cc: Philip Oakley, Anders Kaseorg, Junio C Hamano, 842477,
Git Mailing List, Vasco Almeida
In-Reply-To: <20161030211227.4gqovv7mt7mtnpy7@sigill.intra.peff.net>
,On Sun, Oct 30, 2016 at 10:12 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:
>
>> > It is documented (Documentation/git-sh-setup.txt), and this is not the
>> > internal Documentation/technical section of the documentation, so my
>> > default assumption would be that everything shown there is intended as
>> > public. I only bring this up as a question because it was apparently
>> > allowed to break. If I’m wrong and it isn’t public, other patches are
>> > needed (to the documentation and to its users in contrib).
>> >
>> But the Documenation does say ::
>>
>> - This is not a command the end user would want to run. Ever.
>>
>> - This documentation is meant for people who are studying the Porcelain-ish
>> scripts and/or are writing new ones.
>> --
>
> Historically speaking, porcelain-ish scripts were carried both in and
> out of git.git. These days what we consider porcelain is usually carried
> in-tree, but I don't think it's unreasonable for people building their
> own scripts to want to make use of git-sh-setup. And we've generally
> tried to retain backwards compatibility in the functions it provides,
> even to out-of-tree scripts.
>
> So I think it is worth applying the fix at the start of this thread to
> keep that working.
>
> As for a documentation change for "do not use this for out-of-tree
> scripts", I am mildly negative, as I don't think that matches historical
> practice.
I don't see why we shouldn't have some stable shellscript function API
if that's needed either.
I just wanted to point out that currently git-sh-setup isn't
documented as such. So at least a follow-up patch to the documentation
seems in order.
This did break in v2.10.0, and it's taken a couple of months to notice
this, so clearly it's not very widely used, which says something about
the cost-benefit of maintaining this for external users.
It's probably worthwhile to split off git-sh-setup into git-sh-setup &
git-sh-setup-internal along with a documentation fix. A lot of what
it's doing (e.g. git_broken_path_fix(), and adding a die() function)
is probably only needed internally by git itself. The
git-sh-setup-internal should be the thing sourcing "git-sh-i18n", I
don't see how anyone out-of-tree could make use of that. Surely nobody
needs to re-emit the exact message we shipped with our *.po files.
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Jeff King @ 2016-10-30 21:12 UTC (permalink / raw)
To: Philip Oakley
Cc: Anders Kaseorg, Ævar Arnfjörð Bjarmason,
Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <223121D101D844DEBF086AC40A5AF4CB@PhilipOakley>
On Sun, Oct 30, 2016 at 08:09:21PM -0000, Philip Oakley wrote:
> > It is documented (Documentation/git-sh-setup.txt), and this is not the
> > internal Documentation/technical section of the documentation, so my
> > default assumption would be that everything shown there is intended as
> > public. I only bring this up as a question because it was apparently
> > allowed to break. If I’m wrong and it isn’t public, other patches are
> > needed (to the documentation and to its users in contrib).
> >
> But the Documenation does say ::
>
> - This is not a command the end user would want to run. Ever.
>
> - This documentation is meant for people who are studying the Porcelain-ish
> scripts and/or are writing new ones.
> --
Historically speaking, porcelain-ish scripts were carried both in and
out of git.git. These days what we consider porcelain is usually carried
in-tree, but I don't think it's unreasonable for people building their
own scripts to want to make use of git-sh-setup. And we've generally
tried to retain backwards compatibility in the functions it provides,
even to out-of-tree scripts.
So I think it is worth applying the fix at the start of this thread to
keep that working.
As for a documentation change for "do not use this for out-of-tree
scripts", I am mildly negative, as I don't think that matches historical
practice.
-Peff
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Philip Oakley @ 2016-10-30 20:09 UTC (permalink / raw)
To: Anders Kaseorg, Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <alpine.DEB.2.10.1610301503280.60842@buzzword-bingo.mit.edu>
From: "Anders Kaseorg" <andersk@mit.edu>
> On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:
>> This seems like a reasonable fix for this issue. However as far as I
>> can tell git-sh-setup was never meant to be used by outside scripts
>> that didn't ship as part of git itself.
>>
>> If that's the case any change in the API which AFAICT is now
>> considered internal might break them, so should some part of that be
>> made public & documented as such?
>
> It is documented (Documentation/git-sh-setup.txt), and this is not the
> internal Documentation/technical section of the documentation, so my
> default assumption would be that everything shown there is intended as
> public. I only bring this up as a question because it was apparently
> allowed to break. If I’m wrong and it isn’t public, other patches are
> needed (to the documentation and to its users in contrib).
>
But the Documenation does say ::
- This is not a command the end user would want to run. Ever.
- This documentation is meant for people who are studying the Porcelain-ish
scripts and/or are writing new ones.
--
So there is a cautionary word or two there...
The question would then become: what (if anything) was missing in the
documentation?...
maybe the inclusion of Ævar's "[Not] to be used by outside scripts that
didn't ship as part of git itself."?
Or a comment that it may change in newer versions.
Though the code fix may still be reasonable..
Philip
^ permalink raw reply
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Anders Kaseorg @ 2016-10-30 19:21 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <CACBZZX4SnJj6ZYK-Ha3EtiWUf_n=+LZ=UeS=7vxgsj8s=bi3Sg@mail.gmail.com>
On Sun, 30 Oct 2016, Ævar Arnfjörð Bjarmason wrote:
> This seems like a reasonable fix for this issue. However as far as I
> can tell git-sh-setup was never meant to be used by outside scripts
> that didn't ship as part of git itself.
>
> If that's the case any change in the API which AFAICT is now
> considered internal might break them, so should some part of that be
> made public & documented as such?
It is documented (Documentation/git-sh-setup.txt), and this is not the
internal Documentation/technical section of the documentation, so my
default assumption would be that everything shown there is intended as
public. I only bring this up as a question because it was apparently
allowed to break. If I’m wrong and it isn’t public, other patches are
needed (to the documentation and to its users in contrib).
Anders
^ permalink raw reply
* [PATCH] doc: fix missing "::" in config list
From: Jeff King @ 2016-10-30 18:46 UTC (permalink / raw)
To: Alexei Lozovsky
Cc: Michael Rappazzo, Junio C Hamano, ryenus, Git mailing list
In-Reply-To: <CALhvvbYJ8G12Lbe2FgP8PWKZ-LABcw2M-M-zWPkT12UUqq1vaw@mail.gmail.com>
On Sun, Oct 30, 2016 at 07:21:41PM +0200, Alexei Lozovsky wrote:
> > It would help especially when the commit message was written badly.
> >
> > Or it might be possible to customize just like "git log --format"?
>
> It is possible to change the format globally via config option
> rebase.instructionFormat:
>
> $ git config rebase.instructionFormat "%an (%ad): %s"
I had totally forgotten we added this option. When I went to look at its
documentation, I found the formatting a bit funny. This should fix it.
-- >8 --
Subject: [PATCH] doc: fix missing "::" in config list
The rebase.instructionFormat option is missing its "::" to
tell AsciiDoc that it's a list entry. As a result, the
option name gets lumped into the description in one big
paragraph.
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 27069ac03..a0ab66aae 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2450,7 +2450,7 @@ rebase.missingCommitsCheck::
command in the todo-list.
Defaults to "ignore".
-rebase.instructionFormat
+rebase.instructionFormat::
A format string, as specified in linkgit:git-log[1], to be used for
the instruction list during an interactive rebase. The format will automatically
have the long commit hash prepended to the format.
--
2.10.2.870.g7471999
^ permalink raw reply related
* Re: [PATCH] git-sh-setup: Restore sourcability from outside scripts
From: Ævar Arnfjörð Bjarmason @ 2016-10-30 17:55 UTC (permalink / raw)
To: Anders Kaseorg; +Cc: Junio C Hamano, 842477, Git Mailing List, Vasco Almeida
In-Reply-To: <alpine.DEB.2.10.1610292153300.60842@buzzword-bingo.mit.edu>
On Sun, Oct 30, 2016 at 3:10 AM, Anders Kaseorg <andersk@mit.edu> wrote:
> v2.10.0-rc0~45^2~2 “i18n: git-sh-setup.sh: mark strings for
> translation” broke outside scripts such as guilt that source
> git-sh-setup as described in the documentation:
>
> $ . "$(git --exec-path)/git-sh-setup"
> sh: 6: .: git-sh-i18n: not found
This seems like a reasonable fix for this issue. However as far as I
can tell git-sh-setup was never meant to be used by outside scripts
that didn't ship as part of git itself.
If that's the case any change in the API which AFAICT is now
considered internal might break them, so should some part of that be
made public & documented as such?
^ permalink raw reply
* Re: [git rebase -i] show time and author besides commit hash and message?
From: Alexei Lozovsky @ 2016-10-30 17:21 UTC (permalink / raw)
To: ryenus; +Cc: Git mailing list
In-Reply-To: <CAKkAvazX1gDzwhQLTbRvxc84sjz72ONy2-P7qWijQUnQqJ+K8g@mail.gmail.com>
> It would help especially when the commit message was written badly.
>
> Or it might be possible to customize just like "git log --format"?
It is possible to change the format globally via config option
rebase.instructionFormat:
$ git config rebase.instructionFormat "%an (%ad): %s"
The format is the same as for 'git log'. This one outputs author
name, date, and the first line of commit message.
This option is supported since Git 2.6.
Or are you interested in a command-line option that can change
the format on per-invocation basis? I think there isn't one.
It can be interesting to add it, but I don't think it has much
utility...
^ permalink raw reply
* [git rebase -i] show time and author besides commit hash and message?
From: ryenus @ 2016-10-30 17:03 UTC (permalink / raw)
To: Git mailing list
It would help especially when the commit message was written badly.
Or it might be possible to customize just like "git log --format"?
Thanks
^ permalink raw reply
* Re: Expanding Includes in .gitignore
From: Jeff King @ 2016-10-30 12:54 UTC (permalink / raw)
To: Aaron Pelly; +Cc: Junio C Hamano, git
In-Reply-To: <cc23eece-d693-9e40-78fe-3bafe6bcad3a@pelly.co>
On Fri, Oct 28, 2016 at 10:32:07PM +1300, Aaron Pelly wrote:
> On 28/10/16 15:54, Junio C Hamano wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> However, as I said elsewhere, I'm not convinced this feature is all that
> >> helpful for in-repository .gitignore files, and I think it does
> >> introduce compatibility complications. People with older git will not
> >> respect your .gitignore.d files. Whereas $GIT_DIR/info is purely a local
> >> matter.
> >
> > As I do not see the point of making in-tree .gitignore to a forest
> > of .gitignore.d/ at all, compatibility complications is not worth
> > even thinking about, I would have to say.
>
> Well; that saves some work. :)
>
> I do not suggesting making this mandatory. I think it adds value and it
> is a common and understood mechanism. But, if it is abhorrent, consider:
>
> There is precedent for including files in git-config. This could be
> extended to ignore files. The code is not similar, but the concept is. I
> could live with it.
Yes, but note that we don't have in-tree config files, either (to large
degree because of the security implications).
Perhaps Junio can clarify himself, but I took his statement to mean only
that in-tree .gitignore.d is not worth worrying about, but that
$GIT_DIR/info/exclude.d or similar would be OK (but perhaps I
interpreted that way because that's my own position :) ).
> Or how about a new githook that can intelligently create or return the
> details? This would be my least favourite option unless it was
> configured in an obvious place.
That seems more complicated than is really merited, and probably doesn't
perform great either (it's an extra forked process for almost every git
operation). And obviously would not work for an in-tree solution anyway,
as we do not want to run arbitrary code.
-Peff
^ 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