* [PATCH 0/2] submodule config test cleanup
From: Stefan Beller @ 2016-12-27 19:36 UTC (permalink / raw)
To: gitster; +Cc: git, Stefan Beller
A test cleanup and an additional test for the submodule configuration.
Stefan Beller (2):
t7411: quote URLs
t7411: test lookup of uninitialized submodules
t/t7411-submodule-config.sh | 25 +++++++++++++++++++++++--
1 file changed, 23 insertions(+), 2 deletions(-)
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply
* Re: Intermittent failure of t0021
From: Michael Haggerty @ 2016-12-27 19:30 UTC (permalink / raw)
To: git discussion list, Lars Schneider
In-Reply-To: <ec454def-88a7-5562-1bfe-9d52de42dc55@alum.mit.edu>
On 12/27/2016 08:21 PM, Michael Haggerty wrote:
> I'm seeing intermittent failures of t0021. [...]
Just after submitting this, I saw that the problem has already been
reported and a fix proposed:
http://public-inbox.org/git/dd8decbc-f856-4f68-6d77-7ea9d5f9d126@ramsayjones.plus.com/
Sorry for the noise.
Michael
^ permalink raw reply
* Intermittent failure of t0021
From: Michael Haggerty @ 2016-12-27 19:21 UTC (permalink / raw)
To: git discussion list, Lars Schneider
Hi,
I'm seeing intermittent failures of t0021. If I run it as follows (using
a ramdisk as temporary space, and `EXPENSIVE` turned off), it fails on
average about every 40 attempts:
$ make -j8 O=0 && (cd t && for i in $(seq 200); do if ./t0021-*.sh ;
then echo "passed attempt $i"; else echo "failed on attempt $i"; break;
fi ; done )
(I've also seen it fail while running the whole test suite, so it is not
the tight test loop that triggers the problem.)
The output when it fails is
Initialized empty Git repository in
/home/mhagger/self/proj/git/git/t/trash
directory.t0021-conversion/repo/.git/
[master (root-commit) 56d459b] test commit 1
Author: A U Thor <author@example.com>
1 file changed, 1 insertion(+)
create mode 100644 .gitattributes
[master c30c7fc] test commit 2
Author: A U Thor <author@example.com>
4 files changed, 5 insertions(+)
create mode 100644 test.r
create mode 100644 test2.r
create mode 100644 test4-empty.r
create mode 100644 testsubdir/test3 'sq',$x.r
sort: cannot read: rot13-filter.log: No such file or directory
--- expected.log 2016-12-27 17:58:18.017505012 +0000
+++ rot13-filter.log 2016-12-27 17:58:18.017505012 +0000
@@ -1,7 +0,0 @@
-x IN: clean test.r 57 [OK] -- OUT: 57 . [OK]
-x IN: clean test2.r 14 [OK] -- OUT: 14 . [OK]
-x IN: clean test4-empty.r 0 [OK] -- OUT: 0 [OK]
-x IN: clean testsubdir/test3 'sq',$x.r 49 [OK] -- OUT: 49 . [OK]
- 1 START
- 1 STOP
- 1 init handshake complete
not ok 15 - required process filter should filter data
The failure bisects to
9c48b4f "t0021: minor filter process test cleanup", 2016-12-04
If I test the two parts of that commit separately, it is the removal of
the `.` arguments from `git commit` that triggers the failures (i.e.,
not the removal of the creation of `.gitignore`).
It's possible that the test was unreliable or racy even before this
change, but I have run the same test 1000+ times against the parent
commit (b18f6a00662524443cfb82f5fed7d3b54524c8ab) under the same
conditions without seeing a single failure. So *something* seems to have
been changed for the worse by this commit.
I don't plan to look into it any further, but if you have trouble
reproducing the problem don't hesitate to contact me.
Michael
^ permalink raw reply
* Re: [PATCH v3 21/21] Documentation/git-update-index: explain splitIndex.*
From: Junio C Hamano @ 2016-12-27 19:13 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-22-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> --split-index::
> --no-split-index::
> - Enable or disable split index mode. If enabled, the index is
> - split into two files, $GIT_DIR/index and $GIT_DIR/sharedindex.<SHA-1>.
> - Changes are accumulated in $GIT_DIR/index while the shared
> - index file contains all index entries stays unchanged. If
> - split-index mode is already enabled and `--split-index` is
> - given again, all changes in $GIT_DIR/index are pushed back to
> - the shared index file. This mode is designed for very large
> - indexes that take a significant amount of time to read or write.
> + Enable or disable split index mode. If split-index mode is
> + already enabled and `--split-index` is given again, all
> + changes in $GIT_DIR/index are pushed back to the shared index
> + file.
In the world after this series that introduced the percentage-based
auto consolidation, it smells strange, or even illogical, that index
is un-split after doing this:
$ git update-index --split-index
$ git update-index --split-index
Before this series, it may have been a quick and dirty way to force
consolidating the split index without totally disabling the feature,
i.e. it would have looked more like
$ git update-index --split-index
... work work work to accumulate more modifications
... consolidate into one --- there was no other way than
... disabling it temporarily
$ git update-index --no-split-index
... but the user likes the feature so re-enable it.
$ git update-index --split-index
which I guess is where this strange behaviour comes from.
It may be something we need to fix to unconfuse the end-users after
this series lands. Even though "first disable and then re-enable"
takes two commands (as opposed to one), it is far more logical. And
the percentage-based auto consolidation feature is meant to reduce
the need for manual consolidation, so it probably makes sense to
remove this illogical feature.
> @@ -394,6 +390,31 @@ Although this bit looks similar to assume-unchanged bit, its goal is
> different from assume-unchanged bit's. Skip-worktree also takes
> precedence over assume-unchanged bit when both are set.
>
> +Split index
> +-----------
> +
> +This mode is designed for very large indexes that take a significant
> +amount of time to read or write.
This is not a new problem, but it probably is incorrect to say "to
read or write". It saves time by not rewriting the whole thing but
instead write out only the updated bits. You'd still read the whole
thing while populating the in-core index from the disk, and if
anything, you'd probably spend _more_ cycles because you'd essentially
be reading the base and then reading the delta to apply on top.
> +To avoid deleting a shared index file that is still used, its mtime is
> +updated to the current time everytime a new split index based on the
> +shared index file is either created or read from.
The same comment on the mention of "mtime" in another patch applies
here as well.
^ permalink raw reply
* Re: [PATCH v3 20/21] Documentation/config: add splitIndex.sharedIndexExpire
From: Junio C Hamano @ 2016-12-27 19:11 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-21-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/config.txt | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index e0f5a77980..24e771d22e 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2786,6 +2786,17 @@ splitIndex.maxPercentChange::
> than 20 percent of the total number of entries.
> See linkgit:git-update-index[1].
>
> +splitIndex.sharedIndexExpire::
> + When the split index feature is used, shared index files with
> + a mtime older than this time will be removed when a new shared
As end-user facing documentation, it would be much better if we can
rephrase it for those who do not know what a 'mtime' is, and it
would be even better if we can do so without losing precision.
I think "shared index files that were not modified since the time
this variable specifies will be removed" would be understandable and
correct enough?
> + index file is created. The value "now" expires all entries
> + immediately, and "never" suppresses expiration altogether.
> + The default value is "one.week.ago".
> + Note that each time a new split-index file is created, the
> + mtime of the related shared index file is updated to the
> + current time.
To match the above suggestion, "Note that a shared index file is
considered modified (for the purpose of expiration) each time a new
split-index file is created based on it."?
> + See linkgit:git-update-index[1].
> +
> status.relativePaths::
> By default, linkgit:git-status[1] shows paths relative to the
> current directory. Setting this variable to `false` shows paths
^ permalink raw reply
* Re: [PATCH v3 10/21] read-cache: regenerate shared index if necessary
From: Junio C Hamano @ 2016-12-27 19:08 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-11-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> + case 0:
> + return 1; /* 0% means always write a new shared index */
> + case 100:
> + return 0; /* 100% means never write a new shared index */
> + default:
> + ; /* do nothing: just use the configured value */
> + }
Just like you did in 04/21, write "break" to avoid mistakes made in
the future, i.e.
default:
break; /* just use the configured value */
> +
> + /* Count not shared entries */
> + for (i = 0; i < istate->cache_nr; i++) {
> + struct cache_entry *ce = istate->cache[i];
> + if (!ce->index)
> + not_shared++;
> + }
> +
> + return istate->cache_nr * max_split < not_shared * 100;
On a 32-bit arch with 2G int and more than 20 million paths in the
index, multiplying by max_split that can come close to 100 can
theoretically cause integer overflow, but in practice it probably
does not matter. Or does it?
^ permalink raw reply
* Re: [PATCH v3 08/21] Documentation/git-update-index: talk about core.splitIndex config var
From: Junio C Hamano @ 2016-12-27 19:07 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-9-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> Documentation/git-update-index.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
> index 7386c93162..e091b2a409 100644
> --- a/Documentation/git-update-index.txt
> +++ b/Documentation/git-update-index.txt
> @@ -171,6 +171,12 @@ may not support it yet.
> given again, all changes in $GIT_DIR/index are pushed back to
> the shared index file. This mode is designed for very large
> indexes that take a significant amount of time to read or write.
> ++
> +These options take effect whatever the value of the `core.splitIndex`
> +configuration variable (see linkgit:git-config[1]).
Doesn't the "whatever..." clause in this sentence lack its verb
(presumably, "is", right after "variable")?
> +emitted when the change goes against the configured value, as the
> +configured value will take effect next time the index is read and this
> +will remove the intended effect of the option.
It feels somewhat strange to see a warning in this case.
If the user configures the system to usually do X, and issues a
command that explicitly does Y (or "not-X"), we do not warn for the
command invocation if it is a single-shot operation. That's the
usual "command line overrides configuration" an end-user expects.
I think you made this deviate from the usual end-user expectation
because it is unpredictable when the configuration kicks in the next
time to undo the effect of this command line request. And I agree
that it is a very un-nice thing to do to the users if we did not
warn here, if we are to keep that unpredictableness.
But stepping back a bit, we know the user's intention is that with
"--split-index" the user wants to use the split index, even though
the user may have configuration not to use the split index. Don't
we want to honor that intention? In order to honor that intention
without getting confused by the configured variable to tell us not
to split, another way is to destroy that unpredictableness. For
that, shouldn't we automatically remove the configuration that says
"do not split" (by perhaps set it to "do split")? Doing so still
may need a warning that says "we disabled your previously configured
setting while at it", but it would be much better than a warning
message that essentially says "we do it once, but we will disobey
you at an unpredictable time", I would think.
^ permalink raw reply
* Re: [PATCH v3 06/21] t1700: add tests for core.splitIndex
From: Junio C Hamano @ 2016-12-27 19:04 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-7-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> +test_expect_success 'set core.splitIndex config variable to true' '
> + git config core.splitIndex true &&
> + : >three &&
> + git update-index --add three &&
> + git ls-files --stage >ls-files.actual &&
> + cat >ls-files.expect <<EOF &&
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 one
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 three
> +100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 two
> +EOF
> + test_cmp ls-files.expect ls-files.actual &&
It does not add much value to follow the "existing" outdated style
like this when you are only adding new tests. Write these like
cat >ls-files.expect <<-\EOF &&
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 one
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 three
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 two
EOF
which would give incentive to others (or yourself) to update the
style of the existing mess ;-).
^ permalink raw reply
* Re: [PATCH v3 14/21] read-cache: touch shared index files when used
From: Junio C Hamano @ 2016-12-27 19:10 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-15-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> +/*
> + * Signal that the shared index is used by updating its mtime.
> + *
> + * This way, shared index can be removed if they have not been used
> + * for some time. It's ok to fail to update the mtime if we are on a
> + * read only file system.
> + */
> +void freshen_shared_index(char *base_sha1_hex)
> +{
> + const char *shared_index = git_path("sharedindex.%s", base_sha1_hex);
> + check_and_freshen_file(shared_index, 1);
What happens when this call fails? The function returns 0 if the
file did not even exist. It also returns 0 if you cannot update its
timestamp.
Shouldn't the series be exposing freshen_file() instead _and_ taking
its error return value seriously?
> +}
> +
> int read_index_from(struct index_state *istate, const char *path)
> {
> struct split_index *split_index;
> @@ -2273,6 +2286,8 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
> int ret = write_shared_index(istate, lock, flags);
> if (ret)
> return ret;
> + } else {
> + freshen_shared_index(sha1_to_hex(si->base_sha1));
> }
>
> return write_split_index(istate, lock, flags);
^ permalink raw reply
* [PATCHv6] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-27 19:03 UTC (permalink / raw)
To: gitster; +Cc: git, bmwill, David.Turner, sandals, 6t, Stefan Beller
When deleting a submodule, we need to keep the actual git directory around,
such that we do not lose local changes in there and at a later checkout
of the submodule we don't need to clone it again.
Now that the functionality is available to absorb the git directory of a
submodule, rewrite the checking in git-rm to not complain, but rather
relocate the git directories inside the superproject.
An alternative solution was discussed to have a function
`depopulate_submodule`. That would couple the check for its git directory
and possible relocation before the the removal, such that it is less
likely to miss the check in the future. But the indirection with such
a function added seemed also complex. The reason for that was that this
possible move of the git directory was also implemented in
`ok_to_remove_submodule`, such that this function could truthfully
answer whether it is ok to remove the submodule.
The solution proposed here defers all these checks to the caller.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This replaces the last patch of the series
"git-rm absorbs submodule git directory before deletion".
named "rm: absorb a submodules git dir before deletion".
The only change is a guard (!index_only) for
submodules_absorb_gitdir_if_needed(prefix);
Junio, the other concern you raised about not checking
for local modifications in the submodule has been answered
as a reply there; we do check for local modifications there,
though it is not obvious.
Thanks,
Stefan
builtin/rm.c | 80 ++++++++++++++---------------------------------------------
t/t3600-rm.sh | 39 +++++++++++------------------
2 files changed, 34 insertions(+), 85 deletions(-)
diff --git a/builtin/rm.c b/builtin/rm.c
index 5a5a66272b..20635dca94 100644
--- a/builtin/rm.c
+++ b/builtin/rm.c
@@ -59,27 +59,9 @@ static void print_error_files(struct string_list *files_list,
}
}
-static void error_removing_concrete_submodules(struct string_list *files, int *errs)
-{
- print_error_files(files,
- Q_("the following submodule (or one of its nested "
- "submodules)\n"
- "uses a .git directory:",
- "the following submodules (or one of their nested "
- "submodules)\n"
- "use a .git directory:", files->nr),
- _("\n(use 'rm -rf' if you really want to remove "
- "it including all of its history)"),
- errs);
- string_list_clear(files, 0);
-}
-
-static int check_submodules_use_gitfiles(void)
+static void submodules_absorb_gitdir_if_needed(const char *prefix)
{
int i;
- int errs = 0;
- struct string_list files = STRING_LIST_INIT_NODUP;
-
for (i = 0; i < list.nr; i++) {
const char *name = list.entry[i].name;
int pos;
@@ -99,12 +81,9 @@ static int check_submodules_use_gitfiles(void)
continue;
if (!submodule_uses_gitfile(name))
- string_list_append(&files, name);
+ absorb_git_dir_into_superproject(prefix, name,
+ ABSORB_GITDIR_RECURSE_SUBMODULES);
}
-
- error_removing_concrete_submodules(&files, &errs);
-
- return errs;
}
static int check_local_mod(struct object_id *head, int index_only)
@@ -120,7 +99,6 @@ static int check_local_mod(struct object_id *head, int index_only)
int errs = 0;
struct string_list files_staged = STRING_LIST_INIT_NODUP;
struct string_list files_cached = STRING_LIST_INIT_NODUP;
- struct string_list files_submodule = STRING_LIST_INIT_NODUP;
struct string_list files_local = STRING_LIST_INIT_NODUP;
no_head = is_null_oid(head);
@@ -219,13 +197,8 @@ static int check_local_mod(struct object_id *head, int index_only)
else if (!index_only) {
if (staged_changes)
string_list_append(&files_cached, name);
- if (local_changes) {
- if (S_ISGITLINK(ce->ce_mode) &&
- !submodule_uses_gitfile(name))
- string_list_append(&files_submodule, name);
- else
- string_list_append(&files_local, name);
- }
+ if (local_changes)
+ string_list_append(&files_local, name);
}
}
print_error_files(&files_staged,
@@ -247,8 +220,6 @@ static int check_local_mod(struct object_id *head, int index_only)
&errs);
string_list_clear(&files_cached, 0);
- error_removing_concrete_submodules(&files_submodule, &errs);
-
print_error_files(&files_local,
Q_("the following file has local modifications:",
"the following files have local modifications:",
@@ -342,6 +313,9 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
exit(0);
}
+ if (!index_only)
+ submodules_absorb_gitdir_if_needed(prefix);
+
/*
* If not forced, the file, the index and the HEAD (if exists)
* must match; but the file can already been removed, since
@@ -358,9 +332,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
oidclr(&oid);
if (check_local_mod(&oid, index_only))
exit(1);
- } else if (!index_only) {
- if (check_submodules_use_gitfiles())
- exit(1);
}
/*
@@ -389,32 +360,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
*/
if (!index_only) {
int removed = 0, gitmodules_modified = 0;
- struct strbuf buf = STRBUF_INIT;
for (i = 0; i < list.nr; i++) {
const char *path = list.entry[i].name;
if (list.entry[i].is_submodule) {
- if (is_empty_dir(path)) {
- if (!rmdir(path)) {
- removed = 1;
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- continue;
- }
- } else {
- strbuf_reset(&buf);
- strbuf_addstr(&buf, path);
- if (!remove_dir_recursively(&buf, 0)) {
- removed = 1;
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- strbuf_release(&buf);
- continue;
- } else if (!file_exists(path))
- /* Submodule was removed by user */
- if (!remove_path_from_gitmodules(path))
- gitmodules_modified = 1;
- /* Fallthrough and let remove_path() fail. */
- }
+ struct strbuf buf = STRBUF_INIT;
+
+ strbuf_addstr(&buf, path);
+ if (remove_dir_recursively(&buf, 0))
+ die(_("could not remove '%s'"), path);
+ strbuf_release(&buf);
+
+ removed = 1;
+ if (!remove_path_from_gitmodules(path))
+ gitmodules_modified = 1;
+ continue;
}
if (!remove_path(path)) {
removed = 1;
@@ -423,7 +382,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
if (!removed)
die_errno("git rm: '%s'", path);
}
- strbuf_release(&buf);
if (gitmodules_modified)
stage_updated_gitmodules();
}
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index bcbb680651..5aa6db584c 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -569,26 +569,22 @@ test_expect_success 'rm of a conflicted unpopulated submodule succeeds' '
test_cmp expect actual
'
-test_expect_success 'rm of a populated submodule with a .git directory fails even when forced' '
+test_expect_success 'rm of a populated submodule with a .git directory migrates git dir' '
git checkout -f master &&
git reset --hard &&
git submodule update &&
(cd submod &&
rm .git &&
cp -R ../.git/modules/sub .git &&
- GIT_WORK_TREE=. git config --unset core.worktree
+ GIT_WORK_TREE=. git config --unset core.worktree &&
+ rm -r ../.git/modules/sub
) &&
- test_must_fail git rm submod &&
- test -d submod &&
- test -d submod/.git &&
- git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- test_must_fail git rm -f submod &&
- test -d submod &&
- test -d submod/.git &&
+ git rm submod 2>output.err &&
+ ! test -d submod &&
+ ! test -d submod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- rm -rf submod
+ test -s actual &&
+ test_i18ngrep Migrating output.err
'
cat >expect.deepmodified <<EOF
@@ -667,24 +663,19 @@ test_expect_success 'rm of a populated nested submodule with a nested .git direc
git submodule update --recursive &&
(cd submod/subsubmod &&
rm .git &&
- cp -R ../../.git/modules/sub/modules/sub .git &&
+ mv ../../.git/modules/sub/modules/sub .git &&
GIT_WORK_TREE=. git config --unset core.worktree
) &&
- test_must_fail git rm submod &&
- test -d submod &&
- test -d submod/subsubmod/.git &&
- git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- test_must_fail git rm -f submod &&
- test -d submod &&
- test -d submod/subsubmod/.git &&
+ git rm submod 2>output.err &&
+ ! test -d submod &&
+ ! test -d submod/subsubmod/.git &&
git status -s -uno --ignore-submodules=none >actual &&
- ! test -s actual &&
- rm -rf submod
+ test -s actual &&
+ test_i18ngrep Migrating output.err
'
test_expect_success 'checking out a commit after submodule removal needs manual updates' '
- git commit -m "submodule removal" submod &&
+ git commit -m "submodule removal" submod .gitmodules &&
git checkout HEAD^ &&
git submodule update &&
git checkout -q HEAD^ &&
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply related
* Re: [PATCH v3 13/21] sha1_file: make check_and_freshen_file() non static
From: Junio C Hamano @ 2016-12-27 19:09 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-14-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
> This function will be used in a commit soon, so let's make
> it available globally.
See comment on 14/21; I am not convinced that this is the function
you would want to borrow.
^ permalink raw reply
* Re: [PATCH v3 12/21] Documentation/config: add splitIndex.maxPercentChange
From: Junio C Hamano @ 2016-12-27 19:09 UTC (permalink / raw)
To: Christian Couder
Cc: git, Nguyen Thai Ngoc Duy, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161226102222.17150-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 221c5982c0..e0f5a77980 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2773,6 +2773,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
s/whole/total/ to match the last sentence of this section, perhaps?
"The number of all entries" would also be OK, but "the whole number
of entries" sounds a bit strange.
^ permalink raw reply
* Re: Gitview Shell Injection Vulnerability
From: Stefan Beller @ 2016-12-27 18:45 UTC (permalink / raw)
To: Javantea, Junio C Hamano; +Cc: git@vger.kernel.org, aneesh.kumar
In-Reply-To: <20161227082922.8B7A813893D@mail.altsci.com>
+cc the author of gitview
On Tue, Dec 27, 2016 at 12:29 AM, Javantea <jvoss@altsci.com> wrote:
> I have found a shell injection vulnerability in contrib/gitview/gitview.
>
> Gitview Shell Injection Vulnerability
>
> Versions affected: 8cb711c8a5-1d1bdafd64 (<=2.11.0)
>
> Gitview executes shell commands using string concatenation with user supplied data, filenames and branch names. Running Gitview and interacting with the user interface with a malicious filename or branch name in the current repository results in malicious commands being executed as the current user.
>
> AnnotateWindow.add_file_data(self, filename, commit_sha1, line_num):
> fp = os.popen("git cat-file blob " + commit_sha1 +":"+filename)
>
> AnnotateWindow.annotate(self, filename, commit_sha1, line_num):
> fp = os.popen("git ls-tree "+ commit_sha1 + " -- " + filename)
> fp = os.popen("git blame --incremental -C -C -- " + filename + " " + commit_sha1)
>
> GitView.set_branch(self, args):
> fp = os.popen("git rev-parse --sq --default HEAD " + list_to_string(args, 1))
> fp = os.popen("git rev-list --header --topo-order --parents " + git_rev_list_cmd)
>
> The program also has other uses of os.popen but none use values that the user can manipulate. However, the fix should definitely replace these instances so that the code might one day pass pylint and manual code review easier.
>
> The function os.popen has been replaced by safer functions in the subprocess module. The code can be improved easily because it requires very little change to convert the code to work with arrays of strings instead of strings.
>
> If you have any questions or would like a patch, please let me know.
>
I guess you could send a patch to fix it. It is unclear to me
how the patch submission process for these work, though.
Please see contrib/README to see why it is unclear to me.
> I expect that things that start their life in the contrib/ area
> to graduate out of contrib/ once they mature, either by becoming
> projects on their own, or moving to the toplevel directory. On
> the other hand, I expect I'll be proposing removal of disused
> and inactive ones from time to time.
Maybe it's time for a spring cleanup and remove some old (dead?)
projects from contrib?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-27 18:26 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <CAGZ79kaRY0x+31-UiiBU1iBXGKAgeTRSSjvN0isd7jdg-Y7_rQ@mail.gmail.com>
On Tue, Dec 27, 2016 at 10:17 AM, Stefan Beller <sbeller@google.com> wrote:
> On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>> exit(0);
>>> }
>>>
>>> + submodules_absorb_gitdir_if_needed(prefix);
>>> +
>>> /*
>>> * If not forced, the file, the index and the HEAD (if exists)
>>> * must match; but the file can already been removed, since
>>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>> oidclr(&oid);
>>> if (check_local_mod(&oid, index_only))
>>> exit(1);
>>> - } else if (!index_only) {
>>> - if (check_submodules_use_gitfiles())
>>> - exit(1);
>>> }
>>>
>>
>> Hmph. It may be a bit strange to see an "index-only" remove to
>> touch working tree, no? Yet submodules_absorb_gitdir_if_needed() is
>> unconditionally called above, which feels somewhat unexpected.
>
> I agree. In a reroll I'll protect the call with
>
> if (!index_only)
> submodules_absorb_gitdir_if_needed(prefix);
>
>>
>>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>>> */
>>> if (!index_only) {
>>> int removed = 0, gitmodules_modified = 0;
>>> for (i = 0; i < list.nr; i++) {
>>> const char *path = list.entry[i].name;
>>> if (list.entry[i].is_submodule) {
>>> + struct strbuf buf = STRBUF_INIT;
>>> +
>>> + strbuf_addstr(&buf, path);
>>> + if (remove_dir_recursively(&buf, 0))
>>> + die(_("could not remove '%s'"), path);
>>> + strbuf_release(&buf);
>>> +
>>> + removed = 1;
>>> + if (!remove_path_from_gitmodules(path))
>>> + gitmodules_modified = 1;
>>> + continue;
>>> }
>>
>> I do not see any behaviour change from the original (not quoted
>> here), but it is somewhat surprising that "git rm ./submodule" does
>> not really check if the submodule has local modifications and files
>> that are not even added before remove_dir_recursively() is called.
>>
>> Or perhaps I am reading the code incorrectly and such a check is
>> done elsewhere?
>
> When determining if the entry is a submodule (i.e. setting
> the is_submodule bit) we have
>
> list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
> if (list.entry[list.nr++].is_submodule &&
> !is_staging_gitmodules_ok())
> die (_("Please stage ..."));
>
Well scratch that.
is_staging_gitmodules_ok only checks for the .gitmodules file and not
for the submodule itself (the submodule is not an argument to that function)
The actual answer is found in check_local_mod called via
if (!force) {
struct object_id oid;
if (get_oid("HEAD", &oid))
oidclr(&oid);
if (check_local_mod(&oid, index_only))
exit(1);
}
check_local_mod was touched in the previous patch as it has:
/*
* Is the index different from the file in the work tree?
* If it's a submodule, is its work tree modified?
*/
if (ce_match_stat(ce, &st, 0) ||
(S_ISGITLINK(ce->ce_mode) &&
bad_to_remove_submodule(ce->name,
SUBMODULE_REMOVAL_DIE_ON_ERROR |
SUBMODULE_REMOVAL_IGNORE_IGNORED_UNTRACKED)))
local_changes = 1;
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)
From: Michael Haggerty @ 2016-12-27 18:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqd1gdcmwn.fsf@gitster.mtv.corp.google.com>
On 12/27/2016 06:16 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
>> constant introduced by
>>
>> 3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07
>>
>> sets that constant to the value 2,...
>
> Sorry I didn't notice this earlier, either. Thanks for spotting.
>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Tue, 27 Dec 2016 09:12:09 -0800
> Subject: [PATCH] lockfile: move REPORT_ON_ERROR bit elsewhere
>
> There was LOCK_NO_DEREF defined as 2 = 1<<1 with the same value,
> which was missed due to a huge comment block. Deconflict by moving
> the new one to 4 = 1<<2 for now.
The fix is obviously correct. I'm not sure I like it that comments are
being blamed for mistakes :-P
Perhaps defining these constants within an `enum` would make it clearer
that they are a group and help prevent problems like this in the future
(though that could be done later rather than as part of this hot fix).
Michael
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> lockfile.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lockfile.h b/lockfile.h
> index 16775a7d79..7b715f9e77 100644
> --- a/lockfile.h
> +++ b/lockfile.h
> @@ -137,7 +137,7 @@ struct lock_file {
> * ... this flag can be passed instead to return -1 and give the usual
> * error message upon an error.
> */
> -#define LOCK_REPORT_ON_ERROR 2
> +#define LOCK_REPORT_ON_ERROR 4
>
> /*
> * Usually symbolic links in the destination path are resolved. This
>
^ permalink raw reply
* Re: [PATCHv5 4/4] rm: absorb a submodules git dir before deletion
From: Stefan Beller @ 2016-12-27 18:17 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <xmqqmvfich2e.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 26, 2016 at 5:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> @@ -342,6 +313,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>> exit(0);
>> }
>>
>> + submodules_absorb_gitdir_if_needed(prefix);
>> +
>> /*
>> * If not forced, the file, the index and the HEAD (if exists)
>> * must match; but the file can already been removed, since
>> @@ -358,9 +331,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>> oidclr(&oid);
>> if (check_local_mod(&oid, index_only))
>> exit(1);
>> - } else if (!index_only) {
>> - if (check_submodules_use_gitfiles())
>> - exit(1);
>> }
>>
>
> Hmph. It may be a bit strange to see an "index-only" remove to
> touch working tree, no? Yet submodules_absorb_gitdir_if_needed() is
> unconditionally called above, which feels somewhat unexpected.
I agree. In a reroll I'll protect the call with
if (!index_only)
submodules_absorb_gitdir_if_needed(prefix);
>
>> @@ -389,32 +359,20 @@ int cmd_rm(int argc, const char **argv, const char *prefix)
>> */
>> if (!index_only) {
>> int removed = 0, gitmodules_modified = 0;
>> for (i = 0; i < list.nr; i++) {
>> const char *path = list.entry[i].name;
>> if (list.entry[i].is_submodule) {
>> + struct strbuf buf = STRBUF_INIT;
>> +
>> + strbuf_addstr(&buf, path);
>> + if (remove_dir_recursively(&buf, 0))
>> + die(_("could not remove '%s'"), path);
>> + strbuf_release(&buf);
>> +
>> + removed = 1;
>> + if (!remove_path_from_gitmodules(path))
>> + gitmodules_modified = 1;
>> + continue;
>> }
>
> I do not see any behaviour change from the original (not quoted
> here), but it is somewhat surprising that "git rm ./submodule" does
> not really check if the submodule has local modifications and files
> that are not even added before remove_dir_recursively() is called.
>
> Or perhaps I am reading the code incorrectly and such a check is
> done elsewhere?
When determining if the entry is a submodule (i.e. setting
the is_submodule bit) we have
list.entry[list.nr].is_submodule = S_ISGITLINK(ce->ce_mode);
if (list.entry[list.nr++].is_submodule &&
!is_staging_gitmodules_ok())
die (_("Please stage ..."));
I think for clarity we could drop the is_submodule bit and only
and directly do
if (S_ISGITLINK(ce->ce_mode) &&
!is_staging_gitmodules_ok())
die (_("Please stage ..."));
and below (the code that you quoted) also just check via
S_ISGITLINK(ce->ce_mode)
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv5 1/4] submodule.h: add extern keyword to functions
From: Stefan Beller @ 2016-12-27 17:59 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <xmqqinq6cgxa.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 26, 2016 at 5:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> As the upcoming series will add a lot of functions to the submodule
>> header, let's first make the header consistent to the rest of the project
>> by adding the extern keyword to functions.
>
> This may be the right thing to do in the longer term but a patch
> like this adds a lot of unnecessary work on the integrator when
> there are _other_ topics that are in flight. I'll see how bad the
> conflicts are while merging the topic to 'pu'.
>
> Thanks.
Maybe we can roll this patch on its own,
so in case the latter patches need rerolls we still have this one and can
build on top?
Thanks,
Stefan
^ permalink raw reply
* Re: [PATCHv5 3/4] submodule: rename and add flags to ok_to_remove_submodule
From: Stefan Beller @ 2016-12-27 17:55 UTC (permalink / raw)
To: Junio C Hamano
Cc: git@vger.kernel.org, Brandon Williams, David Turner,
brian m. carlson, Johannes Sixt
In-Reply-To: <xmqqr34uchuq.fsf@gitster.mtv.corp.google.com>
On Mon, Dec 26, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> As only 0 is understood as false, rename the function to invert the
>> meaning, i.e. the return code of 0 signals the removal of the submodule
>> is fine, and other values can be used to return a more precise answer
>> what went wrong.
>
> Makes sense to rename it as that will catch all the callers that
> depend on the old semantics and name.
>
>> - if (start_command(&cp))
>> - die(_("could not run 'git status --porcelain -u --ignore-submodules=none' in submodule %s"), path);
>> + if (start_command(&cp)) {
>> + if (flags & SUBMODULE_REMOVAL_DIE_ON_ERROR)
>> + die(_("could not start 'git status in submodule '%s'"),
>> + path);
>> + ret = -1;
>> + goto out;
>> + }
>
> This new codepath that does not die will not leak anything, as
> a failed start_command() should release its argv[] and env[].
>
>> len = strbuf_read(&buf, cp.out, 1024);
>> if (len > 2)
>> - ok_to_remove = 0;
>> + ret = 1;
>
> Not a new problem but is it obvious why the comparison of "len" is
> against "2"? This may deserve a one-liner comment.
>
> Otherwise looks good to me.
I took the check "len > 2" from the other occurrence in submodule.c.
When thinking about it (and inspecting the man page for
porcelain status), I think just checking != 0 is sufficient.
So in a reroll I'll change that to just
if (len)
...
I'll look at the other occurrences and see if we can reuse code
as well.
Thanks,
Stefan
^ permalink raw reply
* [PATCH] worktree: initialize return value for submodule_uses_worktrees
From: Stefan Beller @ 2016-12-27 17:50 UTC (permalink / raw)
To: gitster, larsxschneider; +Cc: git, Stefan Beller
In-Reply-To: <7E1C7387-4F37-423F-803D-3B5690B49D40@gmail.com>
When the worktrees directory is empty, the `ret` will be returned
uninitialized. Fix it by initializing the value.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
This goes on top of 1a248cf (origin/sb/submodule-embed-gitdir);
ideally to be squashed, but as it is in next already, as a separate
patch.
Thanks,
Stefan
worktree.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/worktree.c b/worktree.c
index d4606aa8cd..828fd7a0ad 100644
--- a/worktree.c
+++ b/worktree.c
@@ -387,7 +387,7 @@ int submodule_uses_worktrees(const char *path)
struct strbuf sb = STRBUF_INIT;
DIR *dir;
struct dirent *d;
- int ret;
+ int ret = 0;
struct repository_format format;
submodule_gitdir = git_pathdup_submodule(path, "%s", "");
--
2.11.0.rc2.50.g8bda6b2.dirty
^ permalink raw reply related
* Re: Making it possible to do “git push origin” instead of “git push origin <branch>”, without having to one-time prepare each branch for it
From: Stefan Monov @ 2016-12-27 17:47 UTC (permalink / raw)
To: Jacob Keller; +Cc: Git mailing list
In-Reply-To: <CA+P7+xpdKEoNY_cgY4g3bMVZe0p-2bYT4Y-3nYyd0O03iD1nZg@mail.gmail.com>
Thanks guys!
^ permalink raw reply
* Re: git-apply: warn/fail on *changed* end of line (eol) *only*?
From: Junio C Hamano @ 2016-12-27 17:27 UTC (permalink / raw)
To: Igor Djordjevic BugA; +Cc: git
In-Reply-To: <xmqqvau7cqy1.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> Imagine that the project wants LF line endings, i.e. it considers
> that a line with CRLF ending has an unwanted "whitespace" at the
> end. Now, you start from this source file:
>
> 1 <CRLF>
> 3 <CRLF>
> 5 <CRLF>
>
> and a patch like this comes in:
>
> 1 <CRLF>
> -3 <CRLF>
> +three <CRLF>
> 5 <CRLF>
>
> You think that "3 <CRLF>" was replaced by "three <CRLF>", and the
> claim is "the 'previous' contents already had <CRLF> ending, so the
> change is not making things worse".
To see the problem with "check existing lines", it probably is
easier to extend the above example to start from a file with one
more line, like this:
1 <CRLF>
3 <CRLF>
4 <LF>
5 <CRLF>
and extend all the example patches to remove "4 <LF>" line as well,
where they remove "3 <CRLF>", making the first example patch like
so:
1 <CRLF>
-3 <CRLF>
-4 <LF>
+three <CRLF>
5 <CRLF>
Now, if you take "three <CRLF>" to be replacing "3 <CRLF>", then you
may feel that not warning on the CRLF would be the right thing, but
there is no reason (other than the fact you, a human, understand
what 'three' means) to choose "3 <CRLF>" over "4 <LF>" as the
original. If you take "three <CRLF>" to be replacing "4 <LF>", you
would need to warn.
A totally uninteresting special case is when the original is all
<CRLF>, but in that case, as you already said in the original
message, the project wants <CRLF> and you can configure it as such.
Then <CR> in the line-end <CRLF> won't be mistaken as if it is a
whitespace character <CR> at the end of a line terminated with <LF>.
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)
From: Junio C Hamano @ 2016-12-27 17:16 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <afd95065-d076-b962-8337-b87008b9f894@alum.mit.edu>
Michael Haggerty <mhagger@alum.mit.edu> writes:
> Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
> constant introduced by
>
> 3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07
>
> sets that constant to the value 2,...
Sorry I didn't notice this earlier, either. Thanks for spotting.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 27 Dec 2016 09:12:09 -0800
Subject: [PATCH] lockfile: move REPORT_ON_ERROR bit elsewhere
There was LOCK_NO_DEREF defined as 2 = 1<<1 with the same value,
which was missed due to a huge comment block. Deconflict by moving
the new one to 4 = 1<<2 for now.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
lockfile.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lockfile.h b/lockfile.h
index 16775a7d79..7b715f9e77 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -137,7 +137,7 @@ struct lock_file {
* ... this flag can be passed instead to return -1 and give the usual
* error message upon an error.
*/
-#define LOCK_REPORT_ON_ERROR 2
+#define LOCK_REPORT_ON_ERROR 4
/*
* Usually symbolic links in the destination path are resolved. This
--
2.11.0-449-gc01fa73926
^ permalink raw reply related
* Re: What's cooking in git.git (Dec 2016, #05; Mon, 19)
From: Michael Haggerty @ 2016-12-27 17:04 UTC (permalink / raw)
To: Junio C Hamano, git
In-Reply-To: <xmqq37hjmow0.fsf@gitster.mtv.corp.google.com>
On 12/20/2016 01:21 AM, Junio C Hamano wrote:
> Here are the topics that have been cooking. Commits prefixed with
> '-' are only in 'pu' (proposed updates) while commits prefixed with
> '+' are in 'next'. The ones marked with '.' do not appear in any of
> the integration branches, but I am still holding onto them.
>
> The second (rather large) batch of topics have been merged to
> 'master'. Please test and catch possible regressions early.
>
> You can find the changes described here in the integration branches
> of the repositories listed at
>
> http://git-blame.blogspot.com/p/git-public-repositories.html
>
> --------------------------------------------------
> [Graduated to "master"]
>
> [...]
> * jc/lock-report-on-error (2016-12-07) 3 commits
> (merged to 'next' on 2016-12-13 at cb6c07ee92)
> + lockfile: LOCK_REPORT_ON_ERROR
> + hold_locked_index(): align error handling with hold_lockfile_for_update()
> + wt-status: implement opportunisitc index update correctly
>
> Git 2.11 had a minor regression in "merge --ff-only" that competed
> with another process that simultanously attempted to update the
> index. We used to explain what went wrong with an error message,
> but the new code silently failed. The error message has been
> resurrected.
Sorry I didn't notice this earlier, but the `LOCK_REPORT_ON_ERROR`
constant introduced by
3f061bf "lockfile: LOCK_REPORT_ON_ERROR", 2016-12-07
sets that constant to the value 2, which is the same value set for the
existing constant `LOCK_NO_DEREF`. Both constants define bits that can
be set in the `flags` argument of `hold_lock_file_for_update()`, so one
of these values needs to be changed.
Michael
^ permalink raw reply
* [PATCH v9 20/20] branch: implement '--format' option
From: Karthik Nayak @ 2016-12-27 16:23 UTC (permalink / raw)
To: git; +Cc: jacob.keller, gitster, ramsay, Karthik Nayak
In-Reply-To: <20161227162357.28212-1-Karthik.188@gmail.com>
From: Karthik Nayak <karthik.188@gmail.com>
Implement the '--format' option provided by 'ref-filter'. This lets the
user list branches as per desired format similar to the implementation
in 'git for-each-ref'.
Add tests and documentation for the same.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
Documentation/git-branch.txt | 7 ++++++-
builtin/branch.c | 14 +++++++++-----
t/t3203-branch-output.sh | 14 ++++++++++++++
3 files changed, 29 insertions(+), 6 deletions(-)
diff --git a/Documentation/git-branch.txt b/Documentation/git-branch.txt
index 5516a47..1fae4ee 100644
--- a/Documentation/git-branch.txt
+++ b/Documentation/git-branch.txt
@@ -12,7 +12,7 @@ SYNOPSIS
[--list] [-v [--abbrev=<length> | --no-abbrev]]
[--column[=<options>] | --no-column]
[(--merged | --no-merged | --contains) [<commit>]] [--sort=<key>]
- [--points-at <object>] [<pattern>...]
+ [--points-at <object>] [--format=<format>] [<pattern>...]
'git branch' [--set-upstream | --track | --no-track] [-l] [-f] <branchname> [<start-point>]
'git branch' (--set-upstream-to=<upstream> | -u <upstream>) [<branchname>]
'git branch' --unset-upstream [<branchname>]
@@ -250,6 +250,11 @@ start-point is either a local or remote-tracking branch.
--points-at <object>::
Only list branches of the given object.
+--format <format>::
+ A string that interpolates `%(fieldname)` from the object
+ pointed at by a ref being shown. The format is the same as
+ that of linkgit:git-for-each-ref[1].
+
Examples
--------
diff --git a/builtin/branch.c b/builtin/branch.c
index 2887545..4051a18 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -28,6 +28,7 @@ static const char * const builtin_branch_usage[] = {
N_("git branch [<options>] [-r] (-d | -D) <branch-name>..."),
N_("git branch [<options>] (-m | -M) [<old-branch>] <new-branch>"),
N_("git branch [<options>] [-r | -a] [--points-at]"),
+ N_("git branch [<options>] [-r | -a] [--format]"),
NULL
};
@@ -364,14 +365,14 @@ static char *build_format(struct ref_filter *filter, int maxwidth, const char *r
return strbuf_detach(&fmt, NULL);
}
-static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
+static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting, const char *format)
{
int i;
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
struct strbuf out = STRBUF_INIT;
- char *format;
+ char *to_free = NULL;
/*
* If we are listing more than just remote branches,
@@ -388,7 +389,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
if (filter->verbose)
maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
- format = build_format(filter, maxwidth, remote_prefix);
+ if (!format)
+ format = to_free = build_format(filter, maxwidth, remote_prefix);
verify_ref_format(format);
ref_array_sort(sorting, &array);
@@ -407,7 +409,7 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
}
ref_array_clear(&array);
- free(format);
+ free(to_free);
}
static void reject_rebase_or_bisect_branch(const char *target)
@@ -528,6 +530,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
struct ref_filter filter;
int icase = 0;
static struct ref_sorting *sorting = NULL, **sorting_tail = &sorting;
+ const char *format = NULL;
struct option options[] = {
OPT_GROUP(N_("Generic options")),
@@ -569,6 +572,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
N_("print only branches of the object"), 0, parse_opt_object_name
},
OPT_BOOL('i', "ignore-case", &icase, N_("sorting and filtering are case insensitive")),
+ OPT_STRING( 0 , "format", &format, N_("format"), N_("format to use for the output")),
OPT_END(),
};
@@ -641,7 +645,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (!sorting)
sorting = ref_default_sorting();
sorting->ignore_case = icase;
- print_ref_list(&filter, sorting);
+ print_ref_list(&filter, sorting, format);
print_columns(&output, colopts, NULL);
string_list_clear(&output, 0);
return 0;
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 4521328..5778c0a 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -225,4 +225,18 @@ test_expect_success 'sort branches, ignore case' '
)
'
+test_expect_success 'git branch --format option' '
+ cat >expect <<-\EOF &&
+ Refname is (HEAD detached from fromtag)
+ Refname is refs/heads/ambiguous
+ Refname is refs/heads/branch-one
+ Refname is refs/heads/branch-two
+ Refname is refs/heads/master
+ Refname is refs/heads/ref-to-branch
+ Refname is refs/heads/ref-to-remote
+ EOF
+ git branch --format="Refname is %(refname)" >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.10.2
^ permalink raw reply related
* [PATCH v9 19/20] branch: use ref-filter printing APIs
From: Karthik Nayak @ 2016-12-27 16:23 UTC (permalink / raw)
To: git; +Cc: jacob.keller, gitster, ramsay, Karthik Nayak
In-Reply-To: <20161227162357.28212-1-Karthik.188@gmail.com>
From: Karthik Nayak <karthik.188@gmail.com>
Port branch.c to use ref-filter APIs for printing. This clears out
most of the code used in branch.c for printing and replaces them with
calls made to the ref-filter library.
Introduce build_format() which gets the format required for printing
of refs. Make amendments to print_ref_list() to reflect these changes.
The strings included in build_format() may not be safely quoted for
inclusion (i.e. it might contain '%' which needs to be escaped with an
additional '%'). Introduce quote_literal_for_format() as a helper
function which takes a string and returns a version of the string that
is safely quoted to be used in the for-each-ref format which is built
in build_format().
Change calc_maxwidth() to also account for the length of HEAD ref, by
calling ref-filter:get_head_discription().
Also change the test in t6040 to reflect the changes.
Before this patch, all cross-prefix symrefs weren't shortened. Since
we're using ref-filter APIs, we shorten all symrefs by default. We also
allow the user to change the format if needed with the introduction of
the '--format' option in the next patch.
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
Helped-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
builtin/branch.c | 249 ++++++++++++++++-------------------------------
t/t3203-branch-output.sh | 2 +-
t/t6040-tracking-info.sh | 2 +-
3 files changed, 88 insertions(+), 165 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 34cd61c..2887545 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -36,12 +36,12 @@ static unsigned char head_sha1[20];
static int branch_use_color = -1;
static char branch_colors[][COLOR_MAXLEN] = {
- GIT_COLOR_RESET,
- GIT_COLOR_NORMAL, /* PLAIN */
- GIT_COLOR_RED, /* REMOTE */
- GIT_COLOR_NORMAL, /* LOCAL */
- GIT_COLOR_GREEN, /* CURRENT */
- GIT_COLOR_BLUE, /* UPSTREAM */
+ "%(color:reset)",
+ "%(color:reset)", /* PLAIN */
+ "%(color:red)", /* REMOTE */
+ "%(color:reset)", /* LOCAL */
+ "%(color:green)", /* CURRENT */
+ "%(color:blue)", /* UPSTREAM */
};
enum color_branch {
BRANCH_COLOR_RESET = 0,
@@ -280,180 +280,88 @@ static int delete_branches(int argc, const char **argv, int force, int kinds,
return(ret);
}
-static void fill_tracking_info(struct strbuf *stat, const char *branch_name,
- int show_upstream_ref)
+static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
{
- int ours, theirs;
- char *ref = NULL;
- struct branch *branch = branch_get(branch_name);
- const char *upstream;
- struct strbuf fancy = STRBUF_INIT;
- int upstream_is_gone = 0;
- int added_decoration = 1;
-
- if (stat_tracking_info(branch, &ours, &theirs, &upstream) < 0) {
- if (!upstream)
- return;
- upstream_is_gone = 1;
- }
-
- if (show_upstream_ref) {
- ref = shorten_unambiguous_ref(upstream, 0);
- if (want_color(branch_use_color))
- strbuf_addf(&fancy, "%s%s%s",
- branch_get_color(BRANCH_COLOR_UPSTREAM),
- ref, branch_get_color(BRANCH_COLOR_RESET));
- else
- strbuf_addstr(&fancy, ref);
- }
+ int i, max = 0;
+ for (i = 0; i < refs->nr; i++) {
+ struct ref_array_item *it = refs->items[i];
+ const char *desc = it->refname;
+ int w;
- if (upstream_is_gone) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: gone]"), fancy.buf);
- else
- added_decoration = 0;
- } else if (!ours && !theirs) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s]"), fancy.buf);
- else
- added_decoration = 0;
- } else if (!ours) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: behind %d]"), fancy.buf, theirs);
- else
- strbuf_addf(stat, _("[behind %d]"), theirs);
+ skip_prefix(it->refname, "refs/heads/", &desc);
+ skip_prefix(it->refname, "refs/remotes/", &desc);
+ if (it->kind == FILTER_REFS_DETACHED_HEAD) {
+ char *head_desc = get_head_description();
+ w = utf8_strwidth(head_desc);
+ free(head_desc);
+ } else
+ w = utf8_strwidth(desc);
- } else if (!theirs) {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: ahead %d]"), fancy.buf, ours);
- else
- strbuf_addf(stat, _("[ahead %d]"), ours);
- } else {
- if (show_upstream_ref)
- strbuf_addf(stat, _("[%s: ahead %d, behind %d]"),
- fancy.buf, ours, theirs);
- else
- strbuf_addf(stat, _("[ahead %d, behind %d]"),
- ours, theirs);
+ if (it->kind == FILTER_REFS_REMOTES)
+ w += remote_bonus;
+ if (w > max)
+ max = w;
}
- strbuf_release(&fancy);
- if (added_decoration)
- strbuf_addch(stat, ' ');
- free(ref);
+ return max;
}
-static void add_verbose_info(struct strbuf *out, struct ref_array_item *item,
- struct ref_filter *filter, const char *refname)
+static const char *quote_literal_for_format(const char *s)
{
- struct strbuf subject = STRBUF_INIT, stat = STRBUF_INIT;
- const char *sub = _(" **** invalid ref ****");
- struct commit *commit = item->commit;
+ static struct strbuf buf = STRBUF_INIT;
- if (!parse_commit(commit)) {
- pp_commit_easy(CMIT_FMT_ONELINE, commit, &subject);
- sub = subject.buf;
+ strbuf_reset(&buf);
+ while (*s) {
+ const char *ep = strchrnul(s, '%');
+ if (s < ep)
+ strbuf_add(&buf, s, ep - s);
+ if (*ep == '%') {
+ strbuf_addstr(&buf, "%%");
+ s = ep + 1;
+ } else {
+ s = ep;
+ }
}
-
- if (item->kind == FILTER_REFS_BRANCHES)
- fill_tracking_info(&stat, refname, filter->verbose > 1);
-
- strbuf_addf(out, " %s %s%s",
- find_unique_abbrev(item->commit->object.oid.hash, filter->abbrev),
- stat.buf, sub);
- strbuf_release(&stat);
- strbuf_release(&subject);
+ return buf.buf;
}
-static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,
- struct ref_filter *filter, const char *remote_prefix)
+static char *build_format(struct ref_filter *filter, int maxwidth, const char *remote_prefix)
{
- char c;
- int current = 0;
- int color;
- struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
- const char *prefix_to_show = "";
- const char *prefix_to_skip = NULL;
- const char *desc = item->refname;
- char *to_free = NULL;
-
- switch (item->kind) {
- case FILTER_REFS_BRANCHES:
- prefix_to_skip = "refs/heads/";
- skip_prefix(desc, prefix_to_skip, &desc);
- if (!filter->detached && !strcmp(desc, head))
- current = 1;
- else
- color = BRANCH_COLOR_LOCAL;
- break;
- case FILTER_REFS_REMOTES:
- prefix_to_skip = "refs/remotes/";
- skip_prefix(desc, prefix_to_skip, &desc);
- color = BRANCH_COLOR_REMOTE;
- prefix_to_show = remote_prefix;
- break;
- case FILTER_REFS_DETACHED_HEAD:
- desc = to_free = get_head_description();
- current = 1;
- break;
- default:
- color = BRANCH_COLOR_PLAIN;
- break;
- }
+ struct strbuf fmt = STRBUF_INIT;
+ struct strbuf local = STRBUF_INIT;
+ struct strbuf remote = STRBUF_INIT;
- c = ' ';
- if (current) {
- c = '*';
- color = BRANCH_COLOR_CURRENT;
- }
+ strbuf_addf(&fmt, "%%(if)%%(HEAD)%%(then)* %s%%(else) %%(end)",
+ branch_get_color(BRANCH_COLOR_CURRENT));
- strbuf_addf(&name, "%s%s", prefix_to_show, desc);
if (filter->verbose) {
- int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);
- strbuf_addf(&out, "%c %s%-*s%s", c, branch_get_color(color),
- maxwidth + utf8_compensation, name.buf,
+ strbuf_addf(&local, "%%(align:%d,left)%%(refname:lstrip=2)%%(end)", maxwidth);
+ strbuf_addf(&local, "%s", branch_get_color(BRANCH_COLOR_RESET));
+ strbuf_addf(&local, " %%(objectname:short=7) ");
+
+ if (filter->verbose > 1)
+ strbuf_addf(&local, "%%(if)%%(upstream)%%(then)[%s%%(upstream:short)%s%%(if)%%(upstream:track)"
+ "%%(then): %%(upstream:track,nobracket)%%(end)] %%(end)%%(contents:subject)",
+ branch_get_color(BRANCH_COLOR_UPSTREAM), branch_get_color(BRANCH_COLOR_RESET));
+ else
+ strbuf_addf(&local, "%%(if)%%(upstream:track)%%(then)%%(upstream:track) %%(end)%%(contents:subject)");
+
+ strbuf_addf(&remote, "%s%%(align:%d,left)%s%%(refname:lstrip=2)%%(end)%s%%(if)%%(symref)%%(then) -> %%(symref:short)"
+ "%%(else) %%(objectname:short=7) %%(contents:subject)%%(end)",
+ branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, quote_literal_for_format(remote_prefix),
branch_get_color(BRANCH_COLOR_RESET));
- } else
- strbuf_addf(&out, "%c %s%s%s", c, branch_get_color(color),
- name.buf, branch_get_color(BRANCH_COLOR_RESET));
-
- if (item->symref) {
- const char *symref = item->symref;
- if (prefix_to_skip)
- skip_prefix(symref, prefix_to_skip, &symref);
- strbuf_addf(&out, " -> %s", symref);
- }
- else if (filter->verbose)
- /* " f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h" */
- add_verbose_info(&out, item, filter, desc);
- if (column_active(colopts)) {
- assert(!filter->verbose && "--column and --verbose are incompatible");
- string_list_append(&output, out.buf);
} else {
- printf("%s\n", out.buf);
+ strbuf_addf(&local, "%%(refname:lstrip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
+ branch_get_color(BRANCH_COLOR_RESET));
+ strbuf_addf(&remote, "%s%s%%(refname:lstrip=2)%s%%(if)%%(symref)%%(then) -> %%(symref:short)%%(end)",
+ branch_get_color(BRANCH_COLOR_REMOTE), quote_literal_for_format(remote_prefix),
+ branch_get_color(BRANCH_COLOR_RESET));
}
- strbuf_release(&name);
- strbuf_release(&out);
- free(to_free);
-}
-
-static int calc_maxwidth(struct ref_array *refs, int remote_bonus)
-{
- int i, max = 0;
- for (i = 0; i < refs->nr; i++) {
- struct ref_array_item *it = refs->items[i];
- const char *desc = it->refname;
- int w;
- skip_prefix(it->refname, "refs/heads/", &desc);
- skip_prefix(it->refname, "refs/remotes/", &desc);
- w = utf8_strwidth(desc);
+ strbuf_addf(&fmt, "%%(if:notequals=refs/remotes)%%(refname:rstrip=-2)%%(then)%s%%(else)%s%%(end)", local.buf, remote.buf);
- if (it->kind == FILTER_REFS_REMOTES)
- w += remote_bonus;
- if (w > max)
- max = w;
- }
- return max;
+ strbuf_release(&local);
+ strbuf_release(&remote);
+ return strbuf_detach(&fmt, NULL);
}
static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sorting)
@@ -462,6 +370,8 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
struct ref_array array;
int maxwidth = 0;
const char *remote_prefix = "";
+ struct strbuf out = STRBUF_INIT;
+ char *format;
/*
* If we are listing more than just remote branches,
@@ -473,18 +383,31 @@ static void print_ref_list(struct ref_filter *filter, struct ref_sorting *sortin
memset(&array, 0, sizeof(array));
- verify_ref_format("%(refname)%(symref)");
filter_refs(&array, filter, filter->kind | FILTER_REFS_INCLUDE_BROKEN);
if (filter->verbose)
maxwidth = calc_maxwidth(&array, strlen(remote_prefix));
+ format = build_format(filter, maxwidth, remote_prefix);
+ verify_ref_format(format);
+
ref_array_sort(sorting, &array);
- for (i = 0; i < array.nr; i++)
- format_and_print_ref_item(array.items[i], maxwidth, filter, remote_prefix);
+ for (i = 0; i < array.nr; i++) {
+ format_ref_array_item(array.items[i], format, 0, &out);
+ if (column_active(colopts)) {
+ assert(!filter->verbose && "--column and --verbose are incompatible");
+ /* format to a string_list to let print_columns() do its job */
+ string_list_append(&output, out.buf);
+ } else {
+ fwrite(out.buf, 1, out.len, stdout);
+ putchar('\n');
+ }
+ strbuf_release(&out);
+ }
ref_array_clear(&array);
+ free(format);
}
static void reject_rebase_or_bisect_branch(const char *target)
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 52283df..4521328 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -194,7 +194,7 @@ test_expect_success 'local-branch symrefs shortened properly' '
git symbolic-ref refs/heads/ref-to-remote refs/remotes/origin/branch-one &&
cat >expect <<-\EOF &&
ref-to-branch -> branch-one
- ref-to-remote -> refs/remotes/origin/branch-one
+ ref-to-remote -> origin/branch-one
EOF
git branch >actual.raw &&
grep ref-to <actual.raw >actual &&
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 3d5c238..97a0765 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -44,7 +44,7 @@ b1 [ahead 1, behind 1] d
b2 [ahead 1, behind 1] d
b3 [behind 1] b
b4 [ahead 2] f
-b5 g
+b5 [gone] g
b6 c
EOF
--
2.10.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox