* Re: [PATCH 4/4] sequencer: use trailer's trailer layout
From: Junio C Hamano @ 2016-11-01 1:11 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <602ae84920300cdbb439eca8098c5e092ca322f7.1477698917.git.jonathantanmy@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
> @@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
> static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
> static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
>
> -static int is_rfc2822_line(const char *buf, int len)
> -{
> - int i;
> -
> - for (i = 0; i < len; i++) {
> - int ch = buf[i];
> - if (ch == ':')
> - return 1;
> - if (!isalnum(ch) && ch != '-')
> - break;
> - }
> -
> - return 0;
> -}
> -
> -static int is_cherry_picked_from_line(const char *buf, int len)
> -{
> - /*
> - * We only care that it looks roughly like (cherry picked from ...)
> - */
> - return len > strlen(cherry_picked_prefix) + 1 &&
> - starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
> -}
We lost two helper functions here, one to detect "Mail-header: like"
line, the other to detect "(cherry picked from") line. Let's see
how the updated caller can do without these. We know that both of
these can be caught if we grabbed the block of trailer block.
> @@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
> static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> int ignore_footer)
> {
> - char prev;
> - int i, k;
> - int len = sb->len - ignore_footer;
> - const char *buf = sb->buf;
> - int found_sob = 0;
> -
> - /* footer must end with newline */
> - if (!len || buf[len - 1] != '\n')
> - return 0;
> + struct trailer_info info;
> + int i;
> + int found_sob = 0, found_sob_last = 0;
>
> - prev = '\0';
> - for (i = len - 1; i > 0; i--) {
> - char ch = buf[i];
> - if (prev == '\n' && ch == '\n') /* paragraph break */
> - break;
> - prev = ch;
> - }
> + trailer_info_get(&info, sb->buf);
>
> - /* require at least one blank line */
> - if (prev != '\n' || buf[i] != '\n')
> + if (info.trailer_start == info.trailer_end)
> return 0;
So we feed the thing to trailer_info_get() which will find the
trailer block. If there is no trailer block, start and end will
point at the same place, which is trivial.
>
> - /* advance to start of last paragraph */
> - while (i < len - 1 && buf[i] == '\n')
> - i++;
> -
> - for (; i < len; i = k) {
> - int found_rfc2822;
> -
> - for (k = i; k < len && buf[k] != '\n'; k++)
> - ; /* do nothing */
> - k++;
> + for (i = 0; i < info.trailer_nr; i++)
> + if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> + found_sob = 1;
> + if (i == info.trailer_nr - 1)
> + found_sob_last = 1;
> + }
>
> - found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
> - if (found_rfc2822 && sob &&
> - !strncmp(buf + i, sob->buf, sob->len))
> - found_sob = k;
Then we scan the trailer block and see if we are looking at the same
s-o-b line as we are asked to look for, and if it is at the last
logical line in the trailer block.
> + trailer_info_release(&info);
>
> - if (!(found_rfc2822 ||
> - is_cherry_picked_from_line(buf + i, k - i - 1)))
> - return 0;
We used to reject a "last paragraph" that has "cruft" other than
"Mail-header: like" line or "(cherry-picked from" line. By reusing
the trailer code, we are getting consistently looser with its logic.
> - }
> - if (found_sob == i)
> + if (found_sob_last)
> return 3;
> if (found_sob)
> return 2;
I found it surprising that you said "commit -s", "cherry-pick -x"
and "format-patch -s" are covered by this patch and saw a change
only to sequencer.c.
It turns out append_signoff() is the central function that is shared
among builtin/commit.c::prepare_to_commit() that does "commit -s",
log-tree.c::show_log() that is called by "format-patch -s", and
sequencer.c::do_recursive_merge() that do_pick_commit() hence "git
cherry-pick -s" uses. And that function decides where to put a new
S-o-b by calling this has_conforming_footer() function.
In addition, do_pick_commit() also calls has_conforming_footer() to
decide where to add "(cherry picked from".
Whoever did the sequencer.c should be proud to structure the code to
make this change so easy to make (I know it is not me, and you
Jonathan know it is not you).
Nicely done by both of you.
> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 9cce5ae..bf0a5c9 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
>
> mesg_broken_footer="$mesg_no_footer
>
> -The signed-off-by string should begin with the words Signed-off-by followed
> -by a colon and space, and then the signers name and email address. e.g.
> -Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> +This is not recognized as a footer because Myfooter is not a recognized token.
> +Myfooter: A.U. Thor <author@example.com>"
>
> mesg_with_footer_sob="$mesg_with_footer
> Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> @@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
> test_cmp expect actual
> '
>
> +test_expect_success 'cherry-pick -s recognizes trailer config' '
> + pristine_detach initial &&
> + git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
> + cat <<-EOF >expect &&
> + $mesg_broken_footer
> + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> + EOF
> + git log -1 --pretty=format:%B >actual &&
> + test_cmp expect actual
> +'
> +
That "myfooter" one is normally not recognized as a valid trailer,
so the test before this one would add a blank line before adding a
new S-o-b; this one adds "myfooter" thing to the vocabulary and no
longer gets the blank before. Good.
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index ba4902d..635b394 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1277,8 +1277,7 @@ EOF
> 4:Subject: [PATCH] subject
> 8:
> 9:I want to mention about Signed-off-by: here.
> -10:
> -11:Signed-off-by: C O Mitter <committer@example.com>
> +10:Signed-off-by: C O Mitter <committer@example.com>
> EOF
> test_cmp expected actual
> '
The original log message is a single-liner subject line, blank, "I
want to mention..." and when asked to append S-o-b:, we would want
to see a blank before the added S-o-b, no?
This seems a bit weird.
> @@ -1294,8 +1293,7 @@ EOF
> 4:Subject: [PATCH] subject
> 8:
> 10:Signed-off-by: example happens to be wrapped here.
> -11:
> -12:Signed-off-by: C O Mitter <committer@example.com>
> +11:Signed-off-by: C O Mitter <committer@example.com>
> EOF
> test_cmp expected actual
> '
This one's original is a single-liner "subject", blank, "My
unfortunate", immediately followed by "S-o-b:". The trailer's
loosened rule (mis)takes the two line body as a trailer block
because one of them is recognised as S-o-b, so it is understandable
that we do not see a blank before the new S-o-b.
This particular example may not look ideal, but it is exactly what
we wanted to happen while we were loosening the rule in the previous
series, so the outcome is understandable.
> @@ -1368,7 +1366,7 @@ EOF
> test_cmp expected actual
> '
>
> -test_expect_success 'signoff: detect garbage in non-conforming footer' '
> +test_expect_success 'signoff: tolerate garbage in conforming footer' '
> append_signoff <<\EOF >actual &&
> subject
>
> @@ -1383,8 +1381,36 @@ EOF
> 8:
> 10:
> 13:Signed-off-by: C O Mitter <committer@example.com>
> -14:
> -15:Signed-off-by: C O Mitter <committer@example.com>
> +EOF
> + test_cmp expected actual
> +'
This is understandable and desirable. The input has "Tested-by: ",
a cruft that says "Some Trash", and S-o-b and we used to reject that
three line as not-a-trailer. We now allow that.
> +test_expect_success 'signoff: respect trailer config' '
> + append_signoff <<\EOF >actual &&
> +subject
> +
> +Myfooter: x
> +Some Trash
> +EOF
> + cat >expected <<\EOF &&
> +4:Subject: [PATCH] subject
> +8:
> +11:
> +12:Signed-off-by: C O Mitter <committer@example.com>
> +EOF
> + test_cmp expected actual &&
> +
> + test_config trailer.Myfooter.ifexists add &&
> + append_signoff <<\EOF >actual &&
> +subject
> +
> +Myfooter: x
> +Some Trash
> +EOF
> + cat >expected <<\EOF &&
> +4:Subject: [PATCH] subject
> +8:
> +11:Signed-off-by: C O Mitter <committer@example.com>
OK. It is not easy to see in the test output, but the shift of the
location from 12 to 11 where a new S-o-b appears is because the last
two lines in the original is taken as a trailer block due to "Myfooter:"
being configured as a valid trailer element.
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index d84897a..4003a27 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -460,6 +460,42 @@ $alt" &&
> test_cmp expected actual
> '
>
> +test_expect_success 'signoff respects trailer config' '
> +
> + echo 5 >positive &&
> + git add positive &&
> + git commit -s -m "subject
> +
> +non-trailer line
> +Myfooter: x" &&
> + git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
> + (
> + echo subject
> + echo
> + echo non-trailer line
> + echo Myfooter: x
> + echo
> + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
Any reason why this does not use "cat <<-EOF"? Ahh, this mimicks
the style of existing ones. OK.
> + ) >expected &&
> + test_cmp expected actual &&
> +
> + echo 6 >positive &&
> + git add positive &&
> + git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
> +
> +non-trailer line
> +Myfooter: x" &&
> + git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
> + (
> + echo subject
> + echo
> + echo non-trailer line
> + echo Myfooter: x
> + echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> + ) >expected &&
> + test_cmp expected actual
> +'
> +
> test_expect_success 'multiple -m' '
>
> >negative &&
Looks good.
^ permalink raw reply
* [PATCH] sha1_name: make wraparound of the index into ring-buffer explicit
From: René Scharfe @ 2016-11-01 8:49 UTC (permalink / raw)
To: Git List; +Cc: Junio C Hamano, Jeff King
Overflow is defined for unsigned integers, but not for signed ones.
Wrap around explicitly for the new ring-buffer in find_unique_abbrev()
as we did in bb84735c for the ones in sha1_to_hex() and get_pathname(),
thus avoiding signed overflows and getting rid of the magic number 3.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
sha1_name.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/sha1_name.c b/sha1_name.c
index 06409a3..73a915f 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -510,7 +510,8 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
{
static int bufno;
static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
- char *hex = hexbuffer[3 & ++bufno];
+ char *hex = hexbuffer[bufno];
+ bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
find_unique_abbrev_r(hex, sha1, len);
return hex;
}
--
2.10.2
^ permalink raw reply related
* [PATCH RESEND] cocci: avoid self-references in object_id transformations
From: René Scharfe @ 2016-11-01 8:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, brian m. carlson
The object_id functions oid_to_hex, oid_to_hex_r, oidclr, oidcmp, and
oidcpy are defined as wrappers of their legacy counterparts sha1_to_hex,
sha1_to_hex_r, hashclr, hashcmp, and hashcpy, respectively. Make sure
that the Coccinelle transformations for converting legacy function calls
are not applied to these wrappers themselves, which would result in
tautological declarations.
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
contrib/coccinelle/object_id.cocci | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/contrib/coccinelle/object_id.cocci b/contrib/coccinelle/object_id.cocci
index 0307624..09afdbf 100644
--- a/contrib/coccinelle/object_id.cocci
+++ b/contrib/coccinelle/object_id.cocci
@@ -17,10 +17,13 @@ expression E1;
+ oid_to_hex(&E1)
@@
+identifier f != oid_to_hex;
expression E1;
@@
+ f(...) {...
- sha1_to_hex(E1->hash)
+ oid_to_hex(E1)
+ ...}
@@
expression E1, E2;
@@ -29,10 +32,13 @@ expression E1, E2;
+ oid_to_hex_r(E1, &E2)
@@
+identifier f != oid_to_hex_r;
expression E1, E2;
@@
+ f(...) {...
- sha1_to_hex_r(E1, E2->hash)
+ oid_to_hex_r(E1, E2)
+ ...}
@@
expression E1;
@@ -41,10 +47,13 @@ expression E1;
+ oidclr(&E1)
@@
+identifier f != oidclr;
expression E1;
@@
+ f(...) {...
- hashclr(E1->hash)
+ oidclr(E1)
+ ...}
@@
expression E1, E2;
@@ -53,10 +62,13 @@ expression E1, E2;
+ oidcmp(&E1, &E2)
@@
+identifier f != oidcmp;
expression E1, E2;
@@
+ f(...) {...
- hashcmp(E1->hash, E2->hash)
+ oidcmp(E1, E2)
+ ...}
@@
expression E1, E2;
@@ -77,10 +89,13 @@ expression E1, E2;
+ oidcpy(&E1, &E2)
@@
+identifier f != oidcpy;
expression E1, E2;
@@
+ f(...) {...
- hashcpy(E1->hash, E2->hash)
+ oidcpy(E1, E2)
+ ...}
@@
expression E1, E2;
--
2.10.2
^ permalink raw reply related
* Re: [ANNOUNCE] Git v2.11.0-rc0
From: Patrick Steinhardt @ 2016-11-01 9:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Linux Kernel
In-Reply-To: <xmqq1sywrxxl.fsf@gitster.mtv.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]
Hi Junio,
On Mon, Oct 31, 2016 at 02:49:42PM -0700, Junio C Hamano wrote:
[snip]
> * In some projects, it is common to use "[RFC PATCH]" as the subject
> prefix for a patch meant for discussion rather than application. A
> new option "--rfc" was a short-hand for "--subject-prefix=RFC PATCH"
~~~
> to help the participants of such projects.
This should probably be 'A new option "--rfc" was introduced as a
short-hand for…' or similar.
> * When given an abbreviated object name that is not (or more
> realistically, "no longer") unique, we gave a fatal error
> "ambiguous argument". This error is now accompanied by hints that
> lists the objects that begins with the given prefix. During the
> course of development of this new feature, numerous minor bugs were
> uncovered and corrected, the most notable one of which is that we
> gave "short SHA1 xxxx is ambiguous." twice without good reason.
I think "This error is now accompanied by a hint that lists the
objects beginning with the given prefix." would be grammatically
more correct.
> * "git diff -W" output needs to extend the context backward to
> include the header line of the current function and also forward to
> include the body of the entire current function up to the header
> line of the next one. This process may have to merge to adjacent
~~
> hunks, but the code forgot to do so in some cases.
This should probably be "_two_ adjacent hunks".
> * In a worktree connected to a repository elsewhere, created via "git
> worktree", "git checkout" attempts to protect users from confusion
> by refusing to check out a branch that is already checked out in
> another worktree. However, this also prevented checking out a
> branch, which is designated as the primary branch of a bare
> reopsitory, in a worktree that is connected to the bare
~~~~~~~~~~
> repository. The check has been corrected to allow it.
There's a typo here in "reopsitory".
Regards
Patrick Steinhardt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH v2 2/2] convert.c: stream and fast search for binary
From: Torsten Bögershausen @ 2016-11-01 9:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqd1il5w4e.fsf@gitster.mtv.corp.google.com>
[]
> This probably should be done as four more patches to become
> reviewable.
>
> - One to use the CONVERT_STAT_BITS a lot more for the conversion
> decision than before,
>
> - another to allow the caller to tell gather_stats() to give up
> early with the "search_only" bits,
>
> - another to update the get_*_convert_stats() functions to use
> get_convert_stats_sha1(), and then finally
>
> - use the streaming interface when reading from blob and file.
>
> or something line that.
Many thanks for the detailed review. Let's see if I can come up
with a better series the next weeks or so.
^ permalink raw reply
* Git issue
From: Halde, Faiz @ 2016-11-01 10:28 UTC (permalink / raw)
To: git@vger.kernel.org
Hello Git
Not sure if this is a feature or a bug, but here it is.
I frequently use the following command to ignore changes done in a file
git update-index --assume-unchanged somefile
Now when I do a pull from my remote branch and say the file 'somefile' was changed locally and in remote, git will abort the merge saying I need to commit my changes of 'somefile'.
But isn't the whole point of the above command to ignore the changes within the file?
Thanks
Faiz Halde
^ permalink raw reply
* [L10N] Kickoff of translation for Git 2.11.0 round 1
From: Jiang Xin @ 2016-11-01 14:19 UTC (permalink / raw)
To: Alexander Shopov, Alex Henrie, Ralf Thielow, Jean-Noël Avila,
Marco Paolone, Changwoo Ryu, Vasco Almeida, Dimitriy Ryazantcev,
Peter Krefting, Trần Ngọc Quân, Ray Chen
Cc: Junio C Hamano, Git List
Hi,
Git v2.11.0-rc0 has been released, and it's time to start new round of git l10n.
This time there are 209 updated messages need to be translated since last
update:
l10n: git.pot: v2.11.0 round 1 (209 new, 53 removed)
Generate po/git.pot from v2.11.0-rc0 for git v2.11.0 l10n round 1.
Signed-off-by: Jiang Xin <worldhello.net@gmail.com>
You can get it from the usual place:
https://github.com/git-l10n/git-po/
As how to update your XX.po and help to translate Git, please see
"Updating a XX.po file" and other sections in “po/README" file.
--
Jiang Xin
^ permalink raw reply
* Re: [PATCH] sha1_name: make wraparound of the index into ring-buffer explicit
From: Jeff King @ 2016-11-01 15:04 UTC (permalink / raw)
To: René Scharfe; +Cc: Git List, Junio C Hamano
In-Reply-To: <a1e2c4a4-4720-fc26-c866-6ab959543c3b@web.de>
On Tue, Nov 01, 2016 at 09:49:07AM +0100, René Scharfe wrote:
> Overflow is defined for unsigned integers, but not for signed ones.
> Wrap around explicitly for the new ring-buffer in find_unique_abbrev()
> as we did in bb84735c for the ones in sha1_to_hex() and get_pathname(),
> thus avoiding signed overflows and getting rid of the magic number 3.
Thanks. I didn't mean to make you do this, but my procrastination won
again.
> diff --git a/sha1_name.c b/sha1_name.c
> index 06409a3..73a915f 100644
> --- a/sha1_name.c
> +++ b/sha1_name.c
> @@ -510,7 +510,8 @@ const char *find_unique_abbrev(const unsigned char *sha1, int len)
> {
> static int bufno;
> static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
> - char *hex = hexbuffer[3 & ++bufno];
> + char *hex = hexbuffer[bufno];
> + bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
Code looks obviously correct (and it even does the "start at 0" thing).
-Peff
^ permalink raw reply
* Re: [PATCH v2 2/6] submodules: load gitmodules file from commit sha1
From: Stefan Beller @ 2016-11-01 16:39 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <1477953496-103596-3-git-send-email-bmwill@google.com>
On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> Teach submodules to load a '.gitmodules' file from a commit sha1. This
> enables the population of the submodule_cache to be based on the state
> of the '.gitmodules' file from a particular commit.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
> cache.h | 2 ++
> config.c | 8 ++++----
> submodule-config.c | 6 +++---
> submodule-config.h | 3 +++
> submodule.c | 12 ++++++++++++
> submodule.h | 1 +
> 6 files changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/cache.h b/cache.h
> index 1be6526..559a461 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1690,6 +1690,8 @@ extern int git_default_config(const char *, const char *, void *);
> extern int git_config_from_file(config_fn_t fn, const char *, void *);
> extern int git_config_from_mem(config_fn_t fn, const enum config_origin_type,
> const char *name, const char *buf, size_t len, void *data);
> +extern int git_config_from_blob_sha1(config_fn_t fn, const char *name,
> + const unsigned char *sha1, void *data);
> extern void git_config_push_parameter(const char *text);
> extern int git_config_from_parameters(config_fn_t fn, void *data);
> extern void git_config(config_fn_t fn, void *);
> diff --git a/config.c b/config.c
> index 83fdecb..4d78e72 100644
> --- a/config.c
> +++ b/config.c
> @@ -1214,10 +1214,10 @@ int git_config_from_mem(config_fn_t fn, const enum config_origin_type origin_typ
> return do_config_from(&top, fn, data);
> }
>
> -static int git_config_from_blob_sha1(config_fn_t fn,
> - const char *name,
> - const unsigned char *sha1,
> - void *data)
> +int git_config_from_blob_sha1(config_fn_t fn,
> + const char *name,
> + const unsigned char *sha1,
While looking at this code, we may want to investigate if a conversion
to struct object_id (instead of const char * for sha1) is feasible.
> + void *data)
> {
> enum object_type type;
> char *buf;
> diff --git a/submodule-config.c b/submodule-config.c
> index 098085b..8b9a2ef 100644
> --- a/submodule-config.c
> +++ b/submodule-config.c
> @@ -379,9 +379,9 @@ static int parse_config(const char *var, const char *value, void *data)
> return ret;
> }
>
> -static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
> - unsigned char *gitmodules_sha1,
> - struct strbuf *rev)
> +int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
> + unsigned char *gitmodules_sha1,
> + struct strbuf *rev)
> {
> int ret = 0;
>
> diff --git a/submodule-config.h b/submodule-config.h
> index d05c542..78584ba 100644
> --- a/submodule-config.h
> +++ b/submodule-config.h
> @@ -29,6 +29,9 @@ const struct submodule *submodule_from_name(const unsigned char *commit_sha1,
> const char *name);
> const struct submodule *submodule_from_path(const unsigned char *commit_sha1,
> const char *path);
> +extern int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
> + unsigned char *gitmodules_sha1,
> + struct strbuf *rev);
no need for extern here as it is a function declaration, not a
variable declaration;
(as said on patch 1, I think consistency to the surrounding is important here)
> void submodule_free(void);
>
> #endif /* SUBMODULE_CONFIG_H */
> diff --git a/submodule.c b/submodule.c
> index ff4e7b2..19dfbd4 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -198,6 +198,18 @@ void gitmodules_config(void)
> }
> }
>
> +void gitmodules_config_sha1(const unsigned char *commit_sha1)
> +{
> + struct strbuf rev = STRBUF_INIT;
> + unsigned char sha1[20];
> +
> + if (gitmodule_sha1_from_commit(commit_sha1, sha1, &rev)) {
> + git_config_from_blob_sha1(submodule_config, rev.buf,
> + sha1, NULL);
> + }
> + strbuf_release(&rev);
> +}
> +
> /*
> * Determine if a submodule has been initialized at a given 'path'
> */
> diff --git a/submodule.h b/submodule.h
> index bd039ca..9a24ac8 100644
> --- a/submodule.h
> +++ b/submodule.h
> @@ -37,6 +37,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
> const char *path);
> int submodule_config(const char *var, const char *value, void *cb);
> void gitmodules_config(void);
> +extern void gitmodules_config_sha1(const unsigned char *commit_sha1);
same.
> extern int is_submodule_initialized(const char *path);
> extern int is_submodule_checked_out(const char *path);
> int parse_submodule_update_strategy(const char *value,
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: [PATCH v2 3/6] grep: add submodules as a grep source type
From: Stefan Beller @ 2016-11-01 16:53 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <1477953496-103596-4-git-send-email-bmwill@google.com>
On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
> type in the various switch statements in grep.c.
>
> When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
> identifier can either be NULL (to indicate that the working tree will be
> used) or a SHA1 (the REV of the submodule to be grep'd). If the
> identifier is a SHA1 then we want to fall through to the
> `GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
This patch only adds the (de-)initialization for the new type,
it is not yet made use of. Looks good.
Thanks,
Stefan
> ---
> grep.c | 16 +++++++++++++++-
> grep.h | 1 +
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/grep.c b/grep.c
> index 1194d35..0dbdc1d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
> case GREP_SOURCE_FILE:
> gs->identifier = xstrdup(identifier);
> break;
> + case GREP_SOURCE_SUBMODULE:
> + if (!identifier) {
> + gs->identifier = NULL;
> + break;
> + }
> + /*
> + * FALL THROUGH
> + * If the identifier is non-NULL (in the submodule case) it
> + * will be a SHA1 that needs to be copied.
> + */
> case GREP_SOURCE_SHA1:
> gs->identifier = xmalloc(20);
> hashcpy(gs->identifier, identifier);
> break;
> case GREP_SOURCE_BUF:
> gs->identifier = NULL;
> + break;
> }
> }
>
> @@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
> switch (gs->type) {
> case GREP_SOURCE_FILE:
> case GREP_SOURCE_SHA1:
> + case GREP_SOURCE_SUBMODULE:
> free(gs->buf);
> gs->buf = NULL;
> gs->size = 0;
> @@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
> return grep_source_load_sha1(gs);
> case GREP_SOURCE_BUF:
> return gs->buf ? 0 : -1;
> + case GREP_SOURCE_SUBMODULE:
> + break;
> }
> - die("BUG: invalid grep_source type");
> + die("BUG: invalid grep_source type to load");
> }
>
> void grep_source_load_driver(struct grep_source *gs)
> diff --git a/grep.h b/grep.h
> index 5856a23..267534c 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -161,6 +161,7 @@ struct grep_source {
> GREP_SOURCE_SHA1,
> GREP_SOURCE_FILE,
> GREP_SOURCE_BUF,
> + GREP_SOURCE_SUBMODULE,
> } type;
> void *identifier;
>
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Junio C Hamano @ 2016-11-01 17:20 UTC (permalink / raw)
To: Stefan Beller; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <CAGZ79kamzSPyM65k9ugS0dAJCfGnGvk3m2p+XtCEozCvoZ5+OA@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
Overall the suggestions from you in this review is good and please
consider anything I did not mention I agree with you. Thanks.
>> +extern int is_submodule_initialized(const char *path);
>> +extern int is_submodule_checked_out(const char *path);
>
> no need to put extern for function names. (no other functions in this
> header are extern. so local consistency maybe? I'd also claim that
> all other extern functions in headers ought to be declared without
> being extern)
Maybe I am old fashioned, but I'd feel better to see these with
explicit "extern" in front (check the older header files like
cache.h when you are in doubt what the project convention has been).
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-11-01 17:23 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kamzSPyM65k9ugS0dAJCfGnGvk3m2p+XtCEozCvoZ5+OA@mail.gmail.com>
On 10/31, Stefan Beller wrote:
> On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
> > +int is_submodule_checked_out(const char *path)
> > +{
> > + int ret = 0;
> > + struct strbuf buf = STRBUF_INIT;
> > +
> > + strbuf_addf(&buf, "%s/.git", path);
> > + ret = file_exists(buf.buf);
>
> I think we can be more tight here; instead of checking
> if the file or directory exists, we should be checking if
> it is a valid git directory, i.e. s/file_exists/resolve_gitdir/
> which returns a path to the actual git dir (in case of a .gitlink)
> or NULL when nothing is found that looks like a git directory or
> pointer to it.
Sounds good.
> > +
> > + strbuf_release(&buf);
> > + return ret;
> > +}
> > +
> > int parse_submodule_update_strategy(const char *value,
> > struct submodule_update_strategy *dst)
> > {
> > diff --git a/submodule.h b/submodule.h
> > index d9e197a..bd039ca 100644
> > --- a/submodule.h
> > +++ b/submodule.h
> > @@ -37,6 +37,8 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
> > const char *path);
> > int submodule_config(const char *var, const char *value, void *cb);
> > void gitmodules_config(void);
> > +extern int is_submodule_initialized(const char *path);
> > +extern int is_submodule_checked_out(const char *path);
>
> no need to put extern for function names. (no other functions in this
> header are extern. so local consistency maybe? I'd also claim that
> all other extern functions in headers ought to be declared without
> being extern)
From looking around at other sections of the code it seems like the
extern keyword is used for functions declared in header files. What's
the style guideline for the project say about this?
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Brandon Williams @ 2016-11-01 17:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Stefan Beller, git@vger.kernel.org
In-Reply-To: <xmqq7f8nqfqc.fsf@gitster.mtv.corp.google.com>
On 11/01, Junio C Hamano wrote:
> Stefan Beller <sbeller@google.com> writes:
>
> Overall the suggestions from you in this review is good and please
> consider anything I did not mention I agree with you. Thanks.
>
> >> +extern int is_submodule_initialized(const char *path);
> >> +extern int is_submodule_checked_out(const char *path);
> >
> > no need to put extern for function names. (no other functions in this
> > header are extern. so local consistency maybe? I'd also claim that
> > all other extern functions in headers ought to be declared without
> > being extern)
>
> Maybe I am old fashioned, but I'd feel better to see these with
> explicit "extern" in front (check the older header files like
> cache.h when you are in doubt what the project convention has been).
I wouldn't consider that old fashion as I'm fairly new to all this and
I also prefer the explicit "extern" :P
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v2 4/6] grep: optionally recurse into submodules
From: Stefan Beller @ 2016-11-01 17:26 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org
In-Reply-To: <1477953496-103596-5-git-send-email-bmwill@google.com>
On Mon, Oct 31, 2016 at 3:38 PM, Brandon Williams <bmwill@google.com> wrote:
>
> +--recurse-submodules::
> + Recursively search in each submodule that has been initialized and
> + checked out in the repository.
> +
and warn otherwise.
> +
> + /*
> + * Limit number of threads for child process to use.
> + * This is to prevent potential fork-bomb behavior of git-grep as each
> + * submodule process has its own thread pool.
> + */
> + if (num_threads)
> + argv_array_pushf(&submodule_options, "--threads=%d",
> + (num_threads + 1) / 2);
Just like in the run_parallel machinery this seems like an approximate
workaround. I'm ok with that for now.
Ideally the parent/child can send each other signals to hand
over threads. (SIGUSR1/SIGUSR2 would be enough to do that,
though I wonder if that is as portable as I would hope. Or we'd look at
"make" and see how they handle recursive calls.
> +
> + /*
> + * Capture output to output buffer and check the return code from the
> + * child process. A '0' indicates a hit, a '1' indicates no hit and
> + * anything else is an error.
> + */
> + status = capture_command(&cp, &w->out, 0);
> + if (status && (status != 1))
Does the user have enough information what went wrong?
Is the child verbose enough, such that we do not need to give a
die[_errno]("submodule processs failed") ?
> +static int grep_submodule(struct grep_opt *opt, const unsigned char *sha1,
> + const char *filename, const char *path)
> +{
> + if (!(is_submodule_initialized(path) &&
If it is not initialized, the user "obviously" doesn't care, so maybe
we only need to warn
if init, but not checked out?
> + is_submodule_checked_out(path))) {
> + warning("skiping submodule '%s%s' since it is not initialized and checked out",
> + super_prefix ? super_prefix : "",
> + path);
> + return 0;
> + }
> +
> +#ifndef NO_PTHREADS
> + if (num_threads) {
> + add_work(opt, GREP_SOURCE_SUBMODULE, filename, path, sha1);
> + return 0;
> + } else
> +#endif
> + {
> + struct work_item w;
> + int hit;
> +
> + grep_source_init(&w.source, GREP_SOURCE_SUBMODULE,
> + filename, path, sha1);
> + strbuf_init(&w.out, 0);
> + opt->output_priv = &w;
> + hit = grep_submodule_launch(opt, &w.source);
> +
> + write_or_die(1, w.out.buf, w.out.len);
> +
> + grep_source_clear(&w.source);
> + strbuf_release(&w.out);
> + return hit;
> + }
> +}
> +
> +static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec,
> + int cached)
> {
> int hit = 0;
> int nr;
> + struct strbuf name = STRBUF_INIT;
> + int name_base_len = 0;
> + if (super_prefix) {
> + name_base_len = strlen(super_prefix);
> + strbuf_addstr(&name, super_prefix);
> + }
> +
> read_cache();
>
> for (nr = 0; nr < active_nr; nr++) {
> const struct cache_entry *ce = active_cache[nr];
> - if (!S_ISREG(ce->ce_mode))
> - continue;
> - if (!ce_path_match(ce, pathspec, NULL))
> - continue;
> - /*
> - * If CE_VALID is on, we assume worktree file and its cache entry
> - * are identical, even if worktree file has been modified, so use
> - * cache version instead
> - */
> - if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {
> - if (ce_stage(ce) || ce_intent_to_add(ce))
> - continue;
> - hit |= grep_sha1(opt, ce->oid.hash, ce->name, 0,
> - ce->name);
> + strbuf_setlen(&name, name_base_len);
> + strbuf_addstr(&name, ce->name);
> +
> + if (S_ISREG(ce->ce_mode) &&
> + match_pathspec(pathspec, name.buf, name.len, 0, NULL,
> + S_ISDIR(ce->ce_mode) ||
> + S_ISGITLINK(ce->ce_mode))) {
Why do we have to pass the ISDIR and ISGITLINK here for the regular file
case? ce_path_match and match_pathspec are doing the same thing?
> + /*
> + * If CE_VALID is on, we assume worktree file and its
> + * cache entry are identical, even if worktree file has
> + * been modified, so use cache version instead
> + */
> + if (cached || (ce->ce_flags & CE_VALID) ||
> + ce_skip_worktree(ce)) {
> + if (ce_stage(ce) || ce_intent_to_add(ce))
> + continue;
> + hit |= grep_sha1(opt, ce->oid.hash, ce->name,
> + 0, ce->name);
> + } else {
> + hit |= grep_file(opt, ce->name);
> + }
> + } else if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
> + submodule_path_match(pathspec, name.buf, NULL)) {
> + hit |= grep_submodule(opt, NULL, ce->name, ce->name);
What is the difference between the last two parameters?
> + * filename: name of the submodule including tree name of parent
> + * path: location of the submodule
That sounds the same to me.
> }
>
> + if (recurse_submodules && (!use_index || untracked || list.nr))
> + die(_("option not supported with --recurse-submodules."));
The user asks: Which option?
> +
> +test_expect_success 'grep and nested submodules' '
> + git init submodule/sub &&
> + echo "foobar" >submodule/sub/a &&
> + git -C submodule/sub add a &&
> + git -C submodule/sub commit -m "add a" &&
> + git -C submodule submodule add ./sub &&
> + git -C submodule add sub &&
> + git -C submodule commit -m "added sub" &&
> + git add submodule &&
> + git commit -m "updated submodule" &&
Both in this test as well as in the setup, we setup a repository
with submodules, that have clean working dirs.
What should happen with dirty working dirs. dirty in the sense:
* file untracked in the submodule
* file added in the submodule, but not committed
* file committed in the submodule, that commit is
untracked in the superproject
* file committed in the submodule, that commit is
added to the index in the superproject
* (last case is just as above:) file committed in submodule,
that commit was committed into the superproject.
^ permalink raw reply
* Re: [PATCH v2 3/6] grep: add submodules as a grep source type
From: Junio C Hamano @ 2016-11-01 17:31 UTC (permalink / raw)
To: Brandon Williams; +Cc: git, sbeller
In-Reply-To: <1477953496-103596-4-git-send-email-bmwill@google.com>
Brandon Williams <bmwill@google.com> writes:
> Add `GREP_SOURCE_SUBMODULE` as a grep_source type and cases for this new
> type in the various switch statements in grep.c.
>
> When initializing a grep_source with type `GREP_SOURCE_SUBMODULE` the
> identifier can either be NULL (to indicate that the working tree will be
> used) or a SHA1 (the REV of the submodule to be grep'd). If the
> identifier is a SHA1 then we want to fall through to the
> `GREP_SOURCE_SHA1` case to handle the copying of the SHA1.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---
Conceptually, it somehow feels strange to have SUBMODULE in this
set.
Source being SHA1 means we are doing a recursive grep in a tree
structure that is stored in the object store, being FILE means we
are reading from the filesystem, being BUF means we are fed in-core
buffer (e.g. to implement the "log --grep='string in message'"). It
is unclear how SUBMODULE fits in that picture, as we do not have a
caller that uses the type at this step yet. Hopefully it will
become obvious why this new type belongs to that set as the series
progresses ;-)
> grep.c | 16 +++++++++++++++-
> grep.h | 1 +
> 2 files changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/grep.c b/grep.c
> index 1194d35..0dbdc1d 100644
> --- a/grep.c
> +++ b/grep.c
> @@ -1735,12 +1735,23 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,
> case GREP_SOURCE_FILE:
> gs->identifier = xstrdup(identifier);
> break;
> + case GREP_SOURCE_SUBMODULE:
> + if (!identifier) {
> + gs->identifier = NULL;
> + break;
> + }
> + /*
> + * FALL THROUGH
> + * If the identifier is non-NULL (in the submodule case) it
> + * will be a SHA1 that needs to be copied.
> + */
> case GREP_SOURCE_SHA1:
> gs->identifier = xmalloc(20);
> hashcpy(gs->identifier, identifier);
> break;
> case GREP_SOURCE_BUF:
> gs->identifier = NULL;
> + break;
> }
> }
>
> @@ -1760,6 +1771,7 @@ void grep_source_clear_data(struct grep_source *gs)
> switch (gs->type) {
> case GREP_SOURCE_FILE:
> case GREP_SOURCE_SHA1:
> + case GREP_SOURCE_SUBMODULE:
> free(gs->buf);
> gs->buf = NULL;
> gs->size = 0;
> @@ -1831,8 +1843,10 @@ static int grep_source_load(struct grep_source *gs)
> return grep_source_load_sha1(gs);
> case GREP_SOURCE_BUF:
> return gs->buf ? 0 : -1;
> + case GREP_SOURCE_SUBMODULE:
> + break;
> }
> - die("BUG: invalid grep_source type");
> + die("BUG: invalid grep_source type to load");
> }
>
> void grep_source_load_driver(struct grep_source *gs)
> diff --git a/grep.h b/grep.h
> index 5856a23..267534c 100644
> --- a/grep.h
> +++ b/grep.h
> @@ -161,6 +161,7 @@ struct grep_source {
> GREP_SOURCE_SHA1,
> GREP_SOURCE_FILE,
> GREP_SOURCE_BUF,
> + GREP_SOURCE_SUBMODULE,
> } type;
> void *identifier;
^ permalink raw reply
* Re: [PATCH v2 1/6] submodules: add helper functions to determine presence of submodules
From: Stefan Beller @ 2016-11-01 17:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git@vger.kernel.org
In-Reply-To: <xmqq7f8nqfqc.fsf@gitster.mtv.corp.google.com>
On Tue, Nov 1, 2016 at 10:20 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> Maybe I am old fashioned, but I'd feel better to see these with
> explicit "extern" in front (check the older header files like
> cache.h when you are in doubt what the project convention has been).
I did check the other files and saw them, so I was very unsure what to
suggest here. I only saw the extern keyword used in headers that were
there when Git was really young, so I assumed it's a style nit by kernel
developers. Thanks for clarifying!
I think we'll want to have some consistency though, so we
maybe want to coordinate a cleanup of submodule.h as well as
submodule-config.h to mark all the functions extern.
This doesn't need to be a all-at-once thing, but we'd keep it in mind
for future declarations in the header.
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCH 4/4] sequencer: use trailer's trailer layout
From: Jonathan Tan @ 2016-11-01 17:38 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqeg2wqa1e.fsf@gitster.mtv.corp.google.com>
On 10/31/2016 06:11 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index ba4902d..635b394 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1277,8 +1277,7 @@ EOF
>> 4:Subject: [PATCH] subject
>> 8:
>> 9:I want to mention about Signed-off-by: here.
>> -10:
>> -11:Signed-off-by: C O Mitter <committer@example.com>
>> +10:Signed-off-by: C O Mitter <committer@example.com>
>> EOF
>> test_cmp expected actual
>> '
>
> The original log message is a single-liner subject line, blank, "I
> want to mention..." and when asked to append S-o-b:, we would want
> to see a blank before the added S-o-b, no?
>
> This seems a bit weird.
This is because the "I want to mention" block has 100% trailer lines
(since its only line contains a colon). We could forbid spaces in
trailer field names, but as you said [1], it might be better to allow
them since users might include them.
The original sequencer.c interpreted this block as not a trailer block,
because it only accepted alphanumeric characters or '-' before the colon
(and no spaces) - hence the difference in behavior.
[1] <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>
^ permalink raw reply
* Re: Git issue
From: Jeff King @ 2016-11-01 17:45 UTC (permalink / raw)
To: Halde, Faiz; +Cc: git@vger.kernel.org
In-Reply-To: <BY2PR0601MB16400EAC3E9683841907F4B2A2A10@BY2PR0601MB1640.namprd06.prod.outlook.com>
On Tue, Nov 01, 2016 at 10:28:57AM +0000, Halde, Faiz wrote:
> I frequently use the following command to ignore changes done in a file
>
> git update-index --assume-unchanged somefile
>
> Now when I do a pull from my remote branch and say the file 'somefile'
> was changed locally and in remote, git will abort the merge saying I
> need to commit my changes of 'somefile'.
>
> But isn't the whole point of the above command to ignore the changes
> within the file?
No. The purpose of --assume-unchanged is to promise git that you will
not change the file, so that it may skip checking the file contents in
some cases as an optimization.
From "git help update-index":
--[no-]assume-unchanged
When this flag is specified, the object names recorded for
the paths are not updated. Instead, this option sets/unsets
the "assume unchanged" bit for the paths. When the "assume
unchanged" bit is on, the user promises not to change the
file and allows Git to assume that the working tree file
matches what is recorded in the index. If you want to change
the working tree file, you need to unset the bit to tell Git.
This is sometimes helpful when working with a big project on
a filesystem that has very slow lstat(2) system call (e.g.
cifs).
Git will fail (gracefully) in case it needs to modify this
file in the index e.g. when merging in a commit; thus, in
case the assumed-untracked file is changed upstream, you will
need to handle the situation manually.
-Peff
^ permalink raw reply
* Re: Git issue
From: Junio C Hamano @ 2016-11-01 18:11 UTC (permalink / raw)
To: Jeff King; +Cc: Halde, Faiz, git@vger.kernel.org
In-Reply-To: <20161101174526.e2tilsriz2fqaru3@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Nov 01, 2016 at 10:28:57AM +0000, Halde, Faiz wrote:
>
>> I frequently use the following command to ignore changes done in a file
>>
>> git update-index --assume-unchanged somefile
>>
>> Now when I do a pull from my remote branch and say the file 'somefile'
>> was changed locally and in remote, git will abort the merge saying I
>> need to commit my changes of 'somefile'.
>>
>> But isn't the whole point of the above command to ignore the changes
>> within the file?
>
> No. The purpose of --assume-unchanged is to promise git that you will
> not change the file, so that it may skip checking the file contents in
> some cases as an optimization.
That's correct.
The next anticipated question is "then how would I tell Git to
ignore changes done to a file locally by me?", whose short answer is
"You don't", of course.
People may however wonder, if Git can make things more automatic if
the user is willing to tell her intention of what should happen to
"somefile" in the example above when an operation requested cannot
proceed while ignoring the local changes. For example, "ignore my
change and overwrite as needed" could be such an instruction (and it
is obvious what should happen in that case when "git pull" was
done--just clobber it with the version from the other side).
As I do not think of other sensible alternative behaviour, and I do
not think Git should make it easy to lose local changes when the
user is doing things like "pull" [*1*], it leads to the longer
answer to the question, which is again "You don't" ;-).
[Footnote]
*1* Things like "git checkout [<tree>] [--] <path>", "git rm -f" and
"git reset --hard" are ways to explicit request nuking the local
changes, and presence of these commands do not contradict with
"do not make it easy to lose local changes", of course.
^ permalink raw reply
* Re: [ANNOUNCE] Git v2.10.2
From: Johannes Schindelin @ 2016-11-01 18:01 UTC (permalink / raw)
To: git-for-windows, Junio C Hamano; +Cc: git
In-Reply-To: <alpine.DEB.2.20.1610291031250.3264@virtualbox>
Hi all,
On Sat, 29 Oct 2016, Johannes Schindelin wrote:
> On Fri, 28 Oct 2016, Junio C Hamano wrote:
>
> > The latest maintenance release Git v2.10.2 is now available at
> > the usual places.
>
> The corresponding Git for Windows version will be hopefully out on
> Tuesday: https://github.com/git-for-windows/git/milestone/5
As of time of writing, cURL has not been released. Git for Windows v2.10.2
will have to wait for tomorrow, or whenever cURL 7.51.0 will be released.
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH 4/4] sequencer: use trailer's trailer layout
From: Junio C Hamano @ 2016-11-01 18:16 UTC (permalink / raw)
To: Jonathan Tan; +Cc: git
In-Reply-To: <a416ab9b-ff1f-9a71-3e58-60fd4f8a6b8e@google.com>
Jonathan Tan <jonathantanmy@google.com> writes:
>>> 9:I want to mention about Signed-off-by: here.
>> ...
>> This seems a bit weird.
>
> This is because the "I want to mention" block has 100% trailer lines
> (since its only line contains a colon). We could forbid spaces in
> trailer field names, but as you said [1], it might be better to allow
> them since users might include them.
That merely means that the implementation of the wish expressed in
[1] was overly loose and needs a bit of tightening, isn't it?
> The original sequencer.c interpreted this block as not a trailer
> block, because it only accepted alphanumeric characters or '-' before
> the colon (and no spaces) - hence the difference in behavior.
That sounds more sensible to me. Would there be an easy way to
still allow misspelled "Thanks to:" but not be fooled by an obvious
nonsense like this example, without going deep into natural language
processing? If not, we may want to tighten it back.
>
> [1] <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>
^ permalink raw reply
* Re: [PATCH v1 05/19] update-index: warn in case of split-index incoherency
From: Junio C Hamano @ 2016-11-01 19:05 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8Br2q0aadTFjkNgb=oN8nSzbkWJEK7bCCgr7v-oOZtrSA@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
>> diff --git a/builtin/update-index.c b/builtin/update-index.c
>> index b75ea03..a14dbf2 100644
>> --- a/builtin/update-index.c
>> +++ b/builtin/update-index.c
>> @@ -1098,12 +1098,21 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>> }
>>
>> if (split_index > 0) {
>> + if (git_config_get_split_index() == 0)
>> + warning("core.splitIndex is set to false; "
>> + "remove or change it, if you really want to "
>> + "enable split index");
>
> Wrap this string and the one below with _() so they can be translated.
True.
I further wonder if a natural reaction from users after seeing this
message is "I do want to--what else would I use that option to run
you for? Just do as you are told, instead of telling me what to
do!". Is this warning really a good idea, or shouldn't these places
be setting the configuration?
>> if (the_index.split_index)
>> the_index.cache_changed |= SPLIT_INDEX_ORDERED;
>> else
>> add_split_index(&the_index);
>> - } else if (!split_index)
>> + } else if (!split_index) {
>> + if (git_config_get_split_index() == 1)
>> + warning("core.splitIndex is set to true; "
>> + "remove or change it, if you really want to "
>> + "disable split index");
>> remove_split_index(&the_index);
>> + }
>>
>> switch (untracked_cache) {
>> case UC_UNSPECIFIED:
>> --
>> 2.10.1.462.g7e1e03a
^ permalink raw reply
* Re: [PATCH v1 09/19] config: add git_config_get_max_percent_split_change()
From: Junio C Hamano @ 2016-11-01 19:13 UTC (permalink / raw)
To: Duy Nguyen
Cc: Christian Couder, Git Mailing List,
Ævar Arnfjörð Bjarmason, Christian Couder
In-Reply-To: <CACsJy8A0djR6=s0AY0tzVehYY5b1-o11uRsFdGtOUCeu4Z6Xjw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Sun, Oct 23, 2016 at 4:26 PM, Christian Couder
> <christian.couder@gmail.com> wrote:
>> This new function will be used in a following commit to get the
>> +int git_config_get_max_percent_split_change(void)
>> +{
>> + int val = -1;
>> +
>> + if (!git_config_get_int("splitindex.maxpercentchange", &val)) {
>> + if (0 <= val && val <= 100)
>> + return val;
>> +
>> + error("splitindex.maxpercentchange value '%d' "
>
> We should keep camelCase form for easy reading. And wrap this string with _().
>
>> + "should be between 0 and 100", val);
>
> I wonder if anybody would try to put 12.3 here and confused by the
> error message, because 0 <= 12.3 <= 100, but it's not an integer..
> Ah.. never mind, die_bad_number() would be called first in this case
> with a loud and clear complaint.
OK.
>
>> + return -1;
Perhaps do the usual
return error(_("..."));
here?
>> + }
>> +
>> + return -1; /* default value */
^ permalink raw reply
* Re: [PATCH v1 11/19] t1700: add tests for splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-11-01 19:15 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-12-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> t/t1700-split-index.sh | 72 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/t/t1700-split-index.sh b/t/t1700-split-index.sh
> index 507a1dd..f03addf 100755
> --- a/t/t1700-split-index.sh
> +++ b/t/t1700-split-index.sh
> @@ -238,4 +238,76 @@ EOF
> test_cmp expect actual
> '
>
> +test_expect_success 'set core.splitIndex config variable to true' '
> + git config core.splitIndex true &&
> + : >three &&
> + git update-index --add three &&
> + BASE=$(test-dump-split-index .git/index | grep "^base") &&
> + test-dump-split-index .git/index | sed "/^own/d" >actual &&
> + cat >expect <<EOF &&
> +$BASE
> +replacements:
> +deletions:
> +EOF
Using <<-EOF lets us indent the above four lines with a horizontal
tab to align with the remainder of this test_expect_success block,
so let's do that.
^ permalink raw reply
* Re: [PATCH v1 12/19] Documentation/config: add splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-11-01 19:19 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161023092648.12086-13-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/config.txt | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 96521a4..380eeb8 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2763,6 +2763,19 @@ showbranch.default::
> The default set of branches for linkgit:git-show-branch[1].
> See linkgit:git-show-branch[1].
>
> +splitIndex.maxPercentChange::
> + When the split index feature is used, this specifies the
> + percent of entries the split index can contain compared to the
> + whole number of entries in both the split index and the shared
> + index before a new shared index is written.
> + The value should be between 0 and 100. If the value is 0 then
> + a new shared index is always written, if it is 100 a new
> + shared index is never written.
Hmph. The early part of the description implies this will kick in
only when some other conditions (i.e. the bit in the index or the
other configuration) are met, but if this disables the split index
when it is set to 0, would we even need the other configuration
variable? IOW, perhaps we can do without core.splitIndex?
> + By default the value is 20, so a new shared index is written
> + if the number of entries in the split index would be greater
> + than 20 percent of the total number of entries.
> + See linkgit:git-update-index[1].
^ 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