[PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Invoking plink requires special treatment, and we have support and even test cases for the commands 'plink' and 'tortoiseplink'. We also support .exe variants for these two and there is a test for 'plink.exe'. On Windows, however, where support for plink.exe would be relevant, the test case fails because it is not possible to execute a file with a .exe extension that is actually not a binary executable---it is a shell script in our test. We have to disable the test case on Windows. Considering, that 'plink.exe' is irrelevant on non-Windows, let's just remove the test and assume that the code just works. Signed-off-by: Johannes Sixt j...@kdbg.org --- t/t5601-clone.sh | 6 -- 1 file changed, 6 deletions(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 9b34f3c..df69bf6 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -353,12 +353,6 @@ test_expect_success 'plink is treated specially (as putty)' ' expect_ssh -P 123 myhost src ' -test_expect_success 'plink.exe is treated specially (as putty)' ' - copy_ssh_wrapper_as $TRASH_DIRECTORY/plink.exe - git clone [myhost:123]:src ssh-bracket-clone-plink-1 - expect_ssh -P 123 myhost src -' - test_expect_success 'tortoiseplink is like putty, with extra arguments' ' copy_ssh_wrapper_as $TRASH_DIRECTORY/tortoiseplink git clone [myhost:123]:src ssh-bracket-clone-plink-2 -- 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 2/4] notes: add tests for --commit/--abort/--strategy exclusivity
From: Jacob Keller jacob.kel...@gmail.com Add new tests to ensure that --commit, --abort, and --strategy are mutually exclusive. Signed-off-by: Jacob Keller jacob.kel...@gmail.com --- t/t3310-notes-merge-manual-resolve.sh | 12 1 file changed, 12 insertions(+) diff --git a/t/t3310-notes-merge-manual-resolve.sh b/t/t3310-notes-merge-manual-resolve.sh index 195bb97f859d..d5572121da69 100755 --- a/t/t3310-notes-merge-manual-resolve.sh +++ b/t/t3310-notes-merge-manual-resolve.sh @@ -314,6 +314,18 @@ y and z notes on 1st commit EOF +test_expect_success 'do not allow mixing --commit and --abort' ' + test_must_fail git notes merge --commit --abort +' + +test_expect_success 'do not allow mixing --commit and --strategy' ' + test_must_fail git notes merge --commit --strategy theirs +' + +test_expect_success 'do not allow mixing --abort and --strategy' ' + test_must_fail git notes merge --abort --strategy theirs +' + test_expect_success 'finalize conflicting merge (z = m)' ' # Resolve conflicts and finalize merge cat .git/NOTES_MERGE_WORKTREE/$commit_sha1 EOF -- 2.5.0.482.gfcd5645 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 0/4] add notes strategy configuration options
From: Jacob Keller jacob.kel...@gmail.com This series of patches implements notes.merge and notes.ref.merge options for configuring notes merge strategy such that user may avoid typing -s. It is (probably) most useful if the user wishes to always enforce cat_sort_uniq strategy. This series now uses git_config_get_string_const() instead of trying to change the git_default_config setup. In addition, the 4th patch is much smaller and avoids a lot of heavy cruft due to this change. I tried to re-work everything I noticed from the email list, but please shout if I did not manage to recall your suggestion. The new version should be much simpler to understand. Changes since v3: * don't use hash table, instead just call git_config_get_string_const * notes.ref.merge must always be fully qualified, (since notes doesn't enforce the refs to be always in refs/notes, so we shouldn't either) * fixed documentation to reduce confusion over which strategies are accepted for options * ensure tests cover notes.ref.merge overrides notes.merge I also tried to make sure the reviewers were all Cc'ed on every patch not just the first one this time. Jacob Keller (4): notes: document cat_sort_uniq rewriteMode notes: add tests for --commit/--abort/--strategy exclusivity notes: add notes.merge option to select default strategy notes: teach git-notes about notes.ref.merge option Documentation/config.txt | 17 +++-- Documentation/git-notes.txt | 23 ++-- builtin/notes.c | 49 - notes-merge.h | 16 + t/t3309-notes-merge-auto-resolve.sh | 68 +++ t/t3310-notes-merge-manual-resolve.sh | 12 +++ 6 files changed, 157 insertions(+), 28 deletions(-) -- 2.5.0.482.gfcd5645 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re* [PATCH v2 1/2] refs: refs/worktree/* become per-worktree
David Turner dtur...@twopensource.com writes:
We need a place to stick refs for bisects in progress that is not
shared between worktrees. So we use the refs/worktree/ hierarchy.
This is by itself OK, but to help existing Porcelains, most notably
gitk, I think refs/bisect/ hierarchy should be treated the same
way. Moving them out of refs/bisect/ to refs/worktree/bisect/ would
not be a good idea.
Because refs/worktree/ hierarchy is not needed right now, I think we
can even live with just special casing refs/bisect/ the way this
patch does, without adding refs/worktree/ support at this point.
Note that git for-each-ref may have inconsistent behavior (I think; I
haven't confirmed this), sometimes showing refs/worktree/* and sometimes
not. In the long run, we should fix this, but right now, I don't know
that it matters, since the only refs affected are these bisect refs.
We should fix that before this hits 'master', preferrably before
this hits 'next', especially if we add support for the more generic
refs/worktree/. If it is only for refs/bisect/, we might be OK, but
I didn't think things through.
diff --git a/path.c b/path.c
index 10f4cbf..da0f767 100644
--- a/path.c
+++ b/path.c
@@ -92,8 +92,9 @@ static void replace_dir(struct strbuf *buf, int len, const
char *newdir)
}
static const char *common_list[] = {
+ /refs, /* special case, since refs/worktree/ is per-worktree */
/branches, /hooks, /info, !/logs, /lost-found,
- /objects, /refs, /remotes, /worktrees, /rr-cache, /svn,
+ /objects, /remotes, /worktrees, /rr-cache, /svn,
config, !gc.pid, packed-refs, shallow,
NULL
};
This is not a new problem, but we probably should use a data
structure that is more performant than a linear list for this.
Also !gc.pid hack should be cleaned up. It does not make much
sense to force all the calls to git_path() pay the price of checking
if each element in common-list begins with '!' and skip, even though
that '!' hack is only used in count-objects and nowhere else.
diff --git a/refs.c b/refs.c
index e6fc3fe..d43bfe1 100644
--- a/refs.c
+++ b/refs.c
@@ -2656,6 +2656,10 @@ static int pack_if_possible_fn(struct ref_entry
*entry, void *cb_data)
struct ref_entry *packed_entry;
int is_tag_ref = starts_with(entry-name, refs/tags/);
+ /* Do not pack per-worktree refs: */
+ if (starts_with(entry-name, refs/worktree/))
+ return 0;
This should use is_per_worktree_ref(), no?
diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 9d21c19..c9fd1ca 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -1131,4 +1131,20 @@ test_expect_success ULIMIT_FILE_DESCRIPTORS 'large
transaction deleting branches
)
'
+test_expect_success 'handle per-worktree refs in refs/worktree' '
+ git commit --allow-empty -m initial commit
+ git worktree add -b branch worktree
+ (
+ cd worktree
+ git commit --allow-empty -m test commit
+ git update-ref refs/worktree/something HEAD
+ git rev-parse refs/worktree/something ../worktree-head
Redirection '', '', '' etc. sticks to the pathname that comes
after it.
+ )
+ ! test -e .git/refs/worktree
+ test_must_fail git rev-parse refs/worktree/something
+ git update-ref refs/worktree/something HEAD
+ git rev-parse refs/worktree/something main-head
Ditto.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [PATCH v2 1/2] refs: refs/worktree/* become per-worktree
On Tue, 2015-08-11 at 14:10 -0700, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: We need a place to stick refs for bisects in progress that is not shared between worktrees. So we use the refs/worktree/ hierarchy. This is by itself OK, but to help existing Porcelains, most notably gitk, I think refs/bisect/ hierarchy should be treated the same way. Moving them out of refs/bisect/ to refs/worktree/bisect/ would not be a good idea. What does gitk do that's relevant here? Because refs/worktree/ hierarchy is not needed right now, I think we can even live with just special casing refs/bisect/ the way this patch does, without adding refs/worktree/ support at this point. There are a few reasons for refs/worktree hierarchy: 1. To make it easy to see the behavior of a ref at a glance. 2. To avoid too many different special-cases. It's true that special-casing refs/bisect would make for fewer code changes for now, but I am worried that next week we'll discover a new situation where we'll want per-worktree refs and then we'll have to add more special-cases. Note that git for-each-ref may have inconsistent behavior (I think; I haven't confirmed this), sometimes showing refs/worktree/* and sometimes not. In the long run, we should fix this, but right now, I don't know that it matters, since the only refs affected are these bisect refs. We should fix that before this hits 'master', preferrably before this hits 'next', especially if we add support for the more generic refs/worktree/. If it is only for refs/bisect/, we might be OK, but I didn't think things through. I will do this. Should for-each-ref include or exclude these refs? One simple way to do include is to always create the refs/worktree directory (when creating refs/). I'll also fix the rest of the things you suggested. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v4 3/4] notes: add notes.merge option to select default strategy
From: Jacob Keller jacob.kel...@gmail.com
Teach git-notes about notes.merge to select a general strategy for all
notes merges. This enables a user to always get expected merge strategy
such as cat_sort_uniq without having to pass the -s option manually.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
---
Documentation/config.txt| 7 ++
Documentation/git-notes.txt | 14 +++-
builtin/notes.c | 44 +++--
notes-merge.h | 16 --
t/t3309-notes-merge-auto-resolve.sh | 29
5 files changed, 86 insertions(+), 24 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index b3df1b16491c..488c2e8eec1b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1905,6 +1905,13 @@ mergetool.writeToTemp::
mergetool.prompt::
Prompt before each invocation of the merge resolution program.
+notes.merge::
+ Which merge strategy to choose by default when resolving notes
+ conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
+ or `cat_sort_uniq`. Defaults to `manual`. See NOTES MERGE
+ STRATEGIES section of linkgit:git-notes[1] for more information
+ on each strategy.
+
notes.displayRef::
The (fully qualified) refname from which to show notes when
showing commit messages. The value of this variable can be set
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 674682b34b83..9c4f8536182f 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -101,7 +101,7 @@ merge::
any) into the current notes ref (called local).
+
If conflicts arise and a strategy for automatically resolving
-conflicting notes (see the -s/--strategy option) is not given,
+conflicting notes (see the NOTES MERGE STRATEGIES section) is not given,
the manual resolver is used. This resolver checks out the
conflicting notes in a special worktree (`.git/NOTES_MERGE_WORKTREE`),
and instructs the user to manually resolve the conflicts there.
@@ -183,6 +183,7 @@ OPTIONS
When merging notes, resolve notes conflicts using the given
strategy. The following strategies are recognized: manual
(default), ours, theirs, union and cat_sort_uniq.
+ This option overrides the notes.merge configuration setting.
See the NOTES MERGE STRATEGIES section below for more
information on each notes merge strategy.
@@ -247,6 +248,9 @@ When done, the user can either finalize the merge with
'git notes merge --commit', or abort the merge with
'git notes merge --abort'.
+Users may select an automated merge strategy from among the following using
+either -s/--strategy option or configuring notes.merge accordingly:
+
ours automatically resolves conflicting notes in favor of the local
version (i.e. the current notes ref).
@@ -310,6 +314,14 @@ core.notesRef::
This setting can be overridden through the environment and
command line.
+notes.merge::
+ Which merge strategy to choose by default when resolving notes
+ conflicts. Must be one of `manual`, `ours`, `theirs`, `union`,
+ or `cat_sort_uniq`. Defaults to `manual`. See NOTES MERGE
+ STRATEGIES section above for more information on each strategy.
++
+This setting can be overridden by passing the `--strategy` option.
+
notes.displayRef::
Which ref (or refs, if a glob or specified more than once), in
addition to the default set by `core.notesRef` or
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc55439..3c705f5e26ff 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -737,6 +737,24 @@ static int merge_commit(struct notes_merge_options *o)
return ret;
}
+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy
*strategy)
+{
+ if (!strcmp(arg, manual))
+ *strategy = NOTES_MERGE_RESOLVE_MANUAL;
+ else if (!strcmp(arg, ours))
+ *strategy = NOTES_MERGE_RESOLVE_OURS;
+ else if (!strcmp(arg, theirs))
+ *strategy = NOTES_MERGE_RESOLVE_THEIRS;
+ else if (!strcmp(arg, union))
+ *strategy = NOTES_MERGE_RESOLVE_UNION;
+ else if (!strcmp(arg, cat_sort_uniq))
+ *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+ else
+ return -1;
+
+ return 0;
+}
+
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -745,7 +763,7 @@ static int merge(int argc, const char **argv, const char
*prefix)
struct notes_merge_options o;
int do_merge = 0, do_commit = 0, do_abort = 0;
int verbosity = 0, result;
- const char *strategy = NULL;
+ const char *strategy = NULL, *configured_strategy = NULL;
struct option options[] = {
OPT_GROUP(N_(General options)),
[PATCH v4 4/4] notes: teach git-notes about notes.ref.merge option
From: Jacob Keller jacob.kel...@gmail.com
Add new option notes.ref.merge option which specifies the merge
strategy for merging into a given notes ref. This option enables
selection of merge strategy for particular notes refs, rather than all
notes ref merges, as user may not want cat_sort_uniq for all refs, but
only some. Note that the ref is the local reference we are merging
into, not the remote ref we merged from. The assumption is that users
will mostly want to configure separate local ref merge strategies rather
than strategies depending on which remote ref they merge from. Also,
notes.ref.merge overrides the general behavior as it is more specific.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
---
Documentation/config.txt| 6 ++
Documentation/git-notes.txt | 6 ++
builtin/notes.c | 7 +--
t/t3309-notes-merge-auto-resolve.sh | 39 +
4 files changed, 56 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 488c2e8eec1b..2c283ebc309e 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1912,6 +1912,12 @@ notes.merge::
STRATEGIES section of linkgit:git-notes[1] for more information
on each strategy.
+notes.localref.merge::
+ Which merge strategy to choose if the local ref for a notes merge
+ matches localref, overriding notes.merge. localref just be a
+ fully qualified refname. See NOTES MERGE STRATEGIES section in
+ linkgit:git-notes[1] for more information on the available strategies.
+
notes.displayRef::
The (fully qualified) refname from which to show notes when
showing commit messages. The value of this variable can be set
diff --git a/Documentation/git-notes.txt b/Documentation/git-notes.txt
index 9c4f8536182f..68fae8d22555 100644
--- a/Documentation/git-notes.txt
+++ b/Documentation/git-notes.txt
@@ -322,6 +322,12 @@ notes.merge::
+
This setting can be overridden by passing the `--strategy` option.
+notes.localref.merge::
+ Which strategy to choose when merging into localref. The set of
+ allowed values is the same as notes.merge. localref must be a fully
+ qualified refname. See NOTES MERGE STRATEGIES section above for more
+ information about each strategy.
+
notes.displayRef::
Which ref (or refs, if a glob or specified more than once), in
addition to the default set by `core.notesRef` or
diff --git a/builtin/notes.c b/builtin/notes.c
index 3c705f5e26ff..2f2d7e87ef47 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -757,7 +757,7 @@ static int parse_notes_strategy(const char *arg, enum
notes_merge_strategy *stra
static int merge(int argc, const char **argv, const char *prefix)
{
- struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
+ struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT, merge_key =
STRBUF_INIT;
unsigned char result_sha1[20];
struct notes_tree *t;
struct notes_merge_options o;
@@ -813,7 +813,10 @@ static int merge(int argc, const char **argv, const char
*prefix)
expand_notes_ref(remote_ref);
o.remote_ref = remote_ref.buf;
- git_config_get_string_const(notes.merge, configured_strategy);
+ strbuf_addf(merge_key, notes.%s.merge, o.local_ref);
+
+ if (git_config_get_string_const(merge_key.buf, configured_strategy))
+ git_config_get_string_const(notes.merge,
configured_strategy);
if (configured_strategy
parse_notes_strategy(configured_strategy, o.strategy))
diff --git a/t/t3309-notes-merge-auto-resolve.sh
b/t/t3309-notes-merge-auto-resolve.sh
index a773b01b73db..b0bbc0a6bac2 100755
--- a/t/t3309-notes-merge-auto-resolve.sh
+++ b/t/t3309-notes-merge-auto-resolve.sh
@@ -383,6 +383,17 @@ test_expect_success 'reset to pre-merge state (y)' '
verify_notes y y
'
+test_expect_success 'merge z into y with ours per-ref configuration option
= Non-conflicting 3-way merge' '
+ git -c notes.refs/notes/y.merge=ours notes merge z
+ verify_notes y ours
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+ git update-ref refs/notes/y refs/notes/y^1
+ # Verify pre-merge state
+ verify_notes y y
+'
+
cat EOF | sort expect_notes_theirs
9b4b2c61f0615412da3c10f98ff85b57c04ec765 $commit_sha15
5de7ea7ad4f47e7ff91989fb82234634730f75df $commit_sha14
@@ -534,6 +545,34 @@ test_expect_success 'reset to pre-merge state (y)' '
verify_notes y y
'
+test_expect_success 'merge z into y with union strategy overriding per-ref
configuration = Non-conflicting 3-way merge' '
+ git -c notes.refs/notes/y.merge=theirs notes merge --strategy=union z
+ verify_notes y union
+'
+
+test_expect_success 'reset to pre-merge state (y)' '
+ git update-ref refs/notes/y refs/notes/y^1
+ # Verify pre-merge state
+ verify_notes y y
+'
+
[PATCH] gitk: adjust the menu line numbers to compensate for the new entry
The previous commit[1] added a new context menu entry. Therefore, the
line numbers of the folloeing entries need to be incremented when their
text or state is changed.
[1] 1437218139-7031-1-git-send-email-dev+...@drbeat.li,
http://article.gmane.org/gmane.comp.version-control.git/274161
Signed-off-by: Beat Bolli dev+...@drbeat.li
Cc: Paul Mackerras pau...@samba.org
---
Paul, feel free to squash this commit into my previous one.
Signed-off-by: Beat Bolli dev+...@drbeat.li
---
gitk-git/gitk | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/gitk-git/gitk b/gitk-git/gitk
index d05169a..bc0e586 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -8877,13 +8877,13 @@ proc rowmenu {x y id} {
if {$id ne $nullid $id ne $nullid2} {
set menu $rowctxmenu
if {$mainhead ne {}} {
- $menu entryconfigure 7 -label [mc Reset %s branch to here
$mainhead] -state normal
+ $menu entryconfigure 8 -label [mc Reset %s branch to here
$mainhead] -state normal
} else {
- $menu entryconfigure 7 -label [mc Detached head: can't reset
$mainhead] -state disabled
+ $menu entryconfigure 8 -label [mc Detached head: can't reset
$mainhead] -state disabled
}
- $menu entryconfigure 9 -state $mstate
$menu entryconfigure 10 -state $mstate
$menu entryconfigure 11 -state $mstate
+ $menu entryconfigure 12 -state $mstate
} else {
set menu $fakerowmenu
}
--
2.5.0.492.g918e48c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] revisions --stdin: accept CRLF line terminators
Johannes Sixt j...@kdbg.org writes:
On Windows, 'git rebase -i' with rebase.missingCommitsCheck set to
warn or error reports:
Dropped commits (newer to older):
'atal: bad revision '410dee56...
The error comes from the git rev-list --stdin invocation in
git-rebase--interactive.sh (function check_todo_list). It is caused by
CRs that end up in the file $todo.miss, because many tools of the MSYS
toolset force LF to CRLF conversion when regular files are written via
stdout.
To fix the error, permit CRLF line terminators when revisions and
pathspec are read using the --stdin option.
Signed-off-by: Johannes Sixt j...@kdbg.org
---
This fixes a new failure in the test suite (t3404.8[67]) on Windows, but
I got around to debug it only now.
revision.c| 4
t/t6017-rev-list-stdin.sh | 16
2 files changed, 20 insertions(+)
diff --git a/revision.c b/revision.c
index cf60c5d..4efedeb 100644
--- a/revision.c
+++ b/revision.c
@@ -1641,6 +1641,8 @@ static void read_pathspec_from_stdin(struct rev_info
*revs, struct strbuf *sb,
int len = sb-len;
if (len sb-buf[len - 1] == '\n')
sb-buf[--len] = '\0';
+ if (len sb-buf[len - 1] == '\r')
+ sb-buf[--len] = '\0';
ALLOC_GROW(prune-path, prune-nr + 1, prune-alloc);
prune-path[prune-nr++] = xstrdup(sb-buf);
}
@@ -1661,6 +1663,8 @@ static void read_revisions_from_stdin(struct rev_info
*revs,
int len = sb.len;
if (len sb.buf[len - 1] == '\n')
sb.buf[--len] = '\0';
+ if (len sb.buf[len - 1] == '\r')
+ sb.buf[--len] = '\0';
This will strip lone CR at the end of line, in addition to CRLF at
the end of line (which you want to handle) and LF at the end of line
(we always have removed), making it work even on ancient MacOS.
If we really cared, I guess we could write it like this:
if (len sb.buf[len - 1] == '\n') {
len--;
if (len sb.buf[len - 1] == '\r')
len--;
sb.buf[len] = '\0';
}
but your version should be fine as-is.
Thanks.
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 667b375..34c43cf 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -75,4 +75,20 @@ test_expect_success 'not only --stdin' '
test_cmp expect actual
'
+test_expect_success 'accept CRLF line terminators' '
+ cat expect -\EOF
+ 7
+
+ file-2
+ EOF
+ q_to_cr input -\EOF
+ masterQ
+ ^master^Q
+ --Q
+ file-2Q
+ EOF
+ git log --pretty=tformat:%s --name-only --stdin input actual
+ test_cmp expect actual
+'
+
test_done
--
2.3.2.245.gb5bf9d3
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [PATCH v2 1/2] refs: refs/worktree/* become per-worktree
On Tue, 2015-08-11 at 14:10 -0700, Junio C Hamano wrote: David Turner dtur...@twopensource.com writes: P.S. I noticed an issue with patch 2/2; I had reverted a now-unnecessary hack, but accidentally reverted the whole file. So I'll need to re-roll anyway. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes.ref.merge option
On Tue, Aug 11, 2015 at 4:57 PM, Jacob Keller jacob.e.kel...@intel.com wrote: Add new option notes.ref.merge option which specifies the merge strategy for merging into a given notes ref. This option enables selection of merge strategy for particular notes refs, rather than all notes ref merges, as user may not want cat_sort_uniq for all refs, but only some. Note that the ref is the local reference we are merging into, not the remote ref we merged from. The assumption is that users will mostly want to configure separate local ref merge strategies rather than strategies depending on which remote ref they merge from. Also, notes.ref.merge overrides the general behavior as it is more specific. Signed-off-by: Jacob Keller jacob.kel...@gmail.com --- diff --git a/Documentation/config.txt b/Documentation/config.txt index 488c2e8eec1b..2c283ebc309e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1912,6 +1912,12 @@ notes.merge:: STRATEGIES section of linkgit:git-notes[1] for more information on each strategy. +notes.localref.merge:: + Which merge strategy to choose if the local ref for a notes merge + matches localref, overriding notes.merge. localref just be a s/just/must/ + fully qualified refname. See NOTES MERGE STRATEGIES section in + linkgit:git-notes[1] for more information on the available strategies. + -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 1/4] notes: document cat_sort_uniq rewriteMode
On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller jacob.e.kel...@intel.com wrote: From: Jacob Keller jacob.kel...@gmail.com Teach documentation about the cat_sort_uniq rewriteMode that got added at the same time as the equivalent merge strategy. Signed-off-by: Jacob Keller jacob.kel...@gmail.com Reviewed-by: Johan Herland jo...@herland.net -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/4] notes: add tests for --commit/--abort/--strategy exclusivity
On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller jacob.e.kel...@intel.com wrote: From: Jacob Keller jacob.kel...@gmail.com Add new tests to ensure that --commit, --abort, and --strategy are mutually exclusive. Signed-off-by: Jacob Keller jacob.kel...@gmail.com Reviewed-by: Johan Herland jo...@herland.net -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions
David Turner dtur...@twopensource.com writes: On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote: I am sorry for being late to the review, I looked into coverity today as Duy bugged me to fix the memory allocation stuff[1] Thanks. Junio, can you pleas substitute the attached patch instead? No. The topic is already in 'next', no? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Tue, Aug 11, 2015 at 4:51 PM, Johannes Sixt j...@kdbg.org wrote: Invoking plink requires special treatment, and we have support and even test cases for the commands 'plink' and 'tortoiseplink'. We also support .exe variants for these two and there is a test for 'plink.exe'. On Windows, however, where support for plink.exe would be relevant, the test case fails because it is not possible to execute a file with a .exe extension that is actually not a binary executable---it is a shell script in our test. We have to disable the test case on Windows. Considering, that 'plink.exe' is irrelevant on non-Windows, let's just remove the test and assume that the code just works. putty and plink are used on Unix as well. A quick check of Mac OS X, Linux, and FreeBSD reveals that package managers on each platform have putty and plink packages available. Signed-off-by: Johannes Sixt j...@kdbg.org --- t/t5601-clone.sh | 6 -- 1 file changed, 6 deletions(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 9b34f3c..df69bf6 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -353,12 +353,6 @@ test_expect_success 'plink is treated specially (as putty)' ' expect_ssh -P 123 myhost src ' -test_expect_success 'plink.exe is treated specially (as putty)' ' - copy_ssh_wrapper_as $TRASH_DIRECTORY/plink.exe - git clone [myhost:123]:src ssh-bracket-clone-plink-1 - expect_ssh -P 123 myhost src -' - test_expect_success 'tortoiseplink is like putty, with extra arguments' ' copy_ssh_wrapper_as $TRASH_DIRECTORY/tortoiseplink git clone [myhost:123]:src ssh-bracket-clone-plink-2 -- 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions
Junio C Hamano gits...@pobox.com writes:
David Turner dtur...@twopensource.com writes:
On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
I am sorry for being late to the review, I looked into coverity today as Duy
bugged me to fix the memory allocation stuff[1]
Thanks. Junio, can you pleas substitute the attached patch instead?
No. The topic is already in 'next', no?
Yes, the topic is already in 'next'. A follow-up fix would be good.
The patch didn't apply cleanly on top of 74ec19d^ to replace 74ec19d
anyway, so I was about to discard it, but after conflict resolution,
the interdiff turns out just these two hunks.
-- 8 --
Subject: pseudoref: check return values from read_ref()
From: David Turner dtur...@twopensource.com
Date: Wed, 15 Jul 2015 18:05:28 -0400
These codepaths attempt to compare the expected current value with
the actual current value, but did not check if we successfully read
the current value before comparison.
Signed-off-by: David Turner dtur...@twopensource.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* It is likely that we would end up comparing the expected value with
garbage when the read fails, and the most likely outcome is that
they do not match and we fail the transaction, which is all fine.
So in that sense, this is not all that urgent, but it is nice to
fix it when we know the code is not kosher.
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 522b19b..1db3654 100644
--- a/refs.c
+++ b/refs.c
@@ -2868,7 +2868,9 @@ static int write_pseudoref(const char *pseudoref, const
unsigned char *sha1,
if (old_sha1) {
unsigned char actual_old_sha1[20];
- read_ref(pseudoref, actual_old_sha1);
+
+ if (read_ref(pseudoref, actual_old_sha1))
+ die(could not read ref '%s', pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
strbuf_addf(err, Unexpected sha1 when writing %s,
pseudoref);
rollback_lock_file(lock);
@@ -2904,7 +2906,8 @@ static int delete_pseudoref(const char *pseudoref, const
unsigned char *old_sha1
LOCK_DIE_ON_ERROR);
if (fd 0)
die_errno(_(Could not open '%s' for writing),
filename);
- read_ref(pseudoref, actual_old_sha1);
+ if (read_ref(pseudoref, actual_old_sha1))
+ die(could not read ref '%s', pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
warning(Unexpected sha1 when deleting %s, pseudoref);
rollback_lock_file(lock);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Eric Sunshine sunsh...@sunshineco.com writes: On Tue, Aug 11, 2015 at 4:51 PM, Johannes Sixt j...@kdbg.org wrote: Invoking plink requires special treatment, and we have support and even test cases for the commands 'plink' and 'tortoiseplink'. We also support .exe variants for these two and there is a test for 'plink.exe'. On Windows, however, where support for plink.exe would be relevant, the test case fails because it is not possible to execute a file with a .exe extension that is actually not a binary executable---it is a shell script in our test. We have to disable the test case on Windows. Considering, that 'plink.exe' is irrelevant on non-Windows, let's just remove the test and assume that the code just works. putty and plink are used on Unix as well. A quick check of Mac OS X, Linux, and FreeBSD reveals that package managers on each platform have putty and plink packages available. ... so we should do the same !MINGW prereq instead? Signed-off-by: Johannes Sixt j...@kdbg.org --- t/t5601-clone.sh | 6 -- 1 file changed, 6 deletions(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 9b34f3c..df69bf6 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -353,12 +353,6 @@ test_expect_success 'plink is treated specially (as putty)' ' expect_ssh -P 123 myhost src ' -test_expect_success 'plink.exe is treated specially (as putty)' ' - copy_ssh_wrapper_as $TRASH_DIRECTORY/plink.exe - git clone [myhost:123]:src ssh-bracket-clone-plink-1 - expect_ssh -P 123 myhost src -' - test_expect_success 'tortoiseplink is like putty, with extra arguments' ' copy_ssh_wrapper_as $TRASH_DIRECTORY/tortoiseplink git clone [myhost:123]:src ssh-bracket-clone-plink-2 -- 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions
On Tue, 2015-08-11 at 15:47 -0700, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
David Turner dtur...@twopensource.com writes:
On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
I am sorry for being late to the review, I looked into coverity today as
Duy
bugged me to fix the memory allocation stuff[1]
Thanks. Junio, can you pleas substitute the attached patch instead?
No. The topic is already in 'next', no?
Yes, the topic is already in 'next'. A follow-up fix would be good.
The patch didn't apply cleanly on top of 74ec19d^ to replace 74ec19d
anyway, so I was about to discard it, but after conflict resolution,
the interdiff turns out just these two hunks.
-- 8 --
Subject: pseudoref: check return values from read_ref()
From: David Turner dtur...@twopensource.com
Date: Wed, 15 Jul 2015 18:05:28 -0400
These codepaths attempt to compare the expected current value with
the actual current value, but did not check if we successfully read
the current value before comparison.
Signed-off-by: David Turner dtur...@twopensource.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
* It is likely that we would end up comparing the expected value with
garbage when the read fails, and the most likely outcome is that
they do not match and we fail the transaction, which is all fine.
So in that sense, this is not all that urgent, but it is nice to
fix it when we know the code is not kosher.
refs.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/refs.c b/refs.c
index 522b19b..1db3654 100644
--- a/refs.c
+++ b/refs.c
@@ -2868,7 +2868,9 @@ static int write_pseudoref(const char *pseudoref, const
unsigned char *sha1,
if (old_sha1) {
unsigned char actual_old_sha1[20];
- read_ref(pseudoref, actual_old_sha1);
+
+ if (read_ref(pseudoref, actual_old_sha1))
+ die(could not read ref '%s', pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
strbuf_addf(err, Unexpected sha1 when writing %s,
pseudoref);
rollback_lock_file(lock);
@@ -2904,7 +2906,8 @@ static int delete_pseudoref(const char *pseudoref,
const unsigned char *old_sha1
LOCK_DIE_ON_ERROR);
if (fd 0)
die_errno(_(Could not open '%s' for writing),
filename);
- read_ref(pseudoref, actual_old_sha1);
+ if (read_ref(pseudoref, actual_old_sha1))
+ die(could not read ref '%s', pseudoref);
if (hashcmp(actual_old_sha1, old_sha1)) {
warning(Unexpected sha1 when deleting %s, pseudoref);
rollback_lock_file(lock);
LGTM.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
Eric Sunshine sunsh...@sunshineco.com writes: On Tue, Aug 11, 2015 at 4:51 PM, Johannes Sixt j...@kdbg.org wrote: Invoking plink requires special treatment, and we have support and even test cases for the commands 'plink' and 'tortoiseplink'. We also support .exe variants for these two and there is a test for 'plink.exe'. On Windows, however, where support for plink.exe would be relevant, the test case fails because it is not possible to execute a file with a .exe extension that is actually not a binary executable---it is a shell script in our test. We have to disable the test case on Windows. Considering, that 'plink.exe' is irrelevant on non-Windows, let's just remove the test and assume that the code just works. putty and plink are used on Unix as well. A quick check of Mac OS X, Linux, and FreeBSD reveals that package managers on each platform have putty and plink packages available. But they do not force their users to say plink.exe, but instead let them invoke plink, no? The test before the one that was removed is about plink (sans .exe), and what was removed is with .exe, so I think J6t's patch is OK. Or am I still missing something? Signed-off-by: Johannes Sixt j...@kdbg.org --- t/t5601-clone.sh | 6 -- 1 file changed, 6 deletions(-) diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 9b34f3c..df69bf6 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -353,12 +353,6 @@ test_expect_success 'plink is treated specially (as putty)' ' expect_ssh -P 123 myhost src ' -test_expect_success 'plink.exe is treated specially (as putty)' ' - copy_ssh_wrapper_as $TRASH_DIRECTORY/plink.exe - git clone [myhost:123]:src ssh-bracket-clone-plink-1 - expect_ssh -P 123 myhost src -' - test_expect_success 'tortoiseplink is like putty, with extra arguments' ' copy_ssh_wrapper_as $TRASH_DIRECTORY/tortoiseplink git clone [myhost:123]:src ssh-bracket-clone-plink-2 -- 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 10/17] path.c: drop git_path_submodule
On Mon, Aug 10, 2015 at 03:57:31PM -0700, Junio C Hamano wrote:
Junio C Hamano gits...@pobox.com writes:
Jeff King p...@peff.net writes:
There are no callers of the slightly-dangerous static-buffer
git_path_submodule left. Let's drop it.
There are a few callers added on 'pu', though.
Actually there is only one. Here is a proposed evil merge.
Thanks for catching. I (obviously) only checked against 'next' and not
'pu'. Rather than the evil merge, we could also just drop this patch.
And then either leave it be, or fix this one up as a separate topic once
merged. Though it really looks like this call site is potentially
dangerous, with the assignment (I think it is OK, though, because
read_info_alternates does not use git_path itself).
diff --git a/submodule.c b/submodule.c
index dfe8b7b..7ab08a1 100644
--- a/submodule.c
+++ b/submodule.c
@@ -120,34 +120,35 @@ void stage_updated_gitmodules(void)
static int add_submodule_odb(const char *path)
[...]
{
struct alternate_object_database *alt_odb;
- const char *objects_directory;
+ struct strbuf objects_directory = STRBUF_INIT;
int ret = 0;
- objects_directory = git_path_submodule(path, objects/);
- if (!is_directory(objects_directory)) {
+ strbuf_git_path_submodule(objects_directory, objects/, %s, path);
+ if (!is_directory(objects_directory.buf)) {
ret = -1;
goto done;
}
Hmph, the change in 6659e74 would have been a lot nicer if
strbuf_git_path_submodule were already available. Most of what you are
doing here is undoing that commit's strbuf/buf transition. :-/
/* avoid adding it twice */
for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb-next)
- if (alt_odb-name - alt_odb-base == strlen(objects_directory)
- !strcmp(alt_odb-base, objects_directory))
+ if (alt_odb-name - alt_odb-base == objects_directory.len
+ !strcmp(alt_odb-base, objects_directory.buf))
goto done;
Not really relevant to what we're talking about here, but this is the
first time I've lookd at add_submodule_odb. It really looks like the
whole second half of the function could be replaced with a call to
link_alt_odb_entry.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] config.mak.uname: Cygwin: Use renames for creation
On Mon, Aug 10, 2015 at 12:08:29PM -0700, Junio C Hamano wrote: But in the end, I'd prefer the choice of the compiled-in default up to the port maintainers. You earlier said: This problem was reported on the Cygwin mailing list at https://cygwin.com/ml/cygwin/2015-08/msg00102.html (amongst others) and is being applied as a manual patch to the Cygwin builds until the patch is taken here. so my preference is to see Cygwin continue to do so for a couple release cycles of ours to make sure all Cygwin end-users are happy and consider the flip of the default a good change for them, and then the official Cygwin packager of Git sends a patch our way. Wait a few releases then resubmit assuming we've not had complaints from Cygwin user. Okay! https://cygwin.com/ml/cygwin/2015-08/msg00102.html seems to indicate that somebody called Adam Dinwoodie is wearing Git maintainer hat, so perhaps you may be that official Cygwin packager of Git ;-) That would indeed be yours truly :) I agree with everything you said in that message to Peter---the patch should be included when you hear reports of `git config core.createObject rename` helping more people. After versions of Cygwin Git package with such a change proves good, let's reduce the workload of Cygwin Git maintainer by upstreaming that change to my tree. But not before. Cool. FWIW, the reason we've started applying this patch to the downstream Cygwin builds is that I've now seen a number of reports (primarily on Stack Overflow) of this problem where that config change has fixed things. I'd been holding off until I had those extra reports, but now I have them I made the patch and figured I'd submit it upstream at the same time. As above, I'll wait a while until we're happy it's stable, and resubmit at that point. (On which note, I should probably submit the patch to the git-gui Makefile we've had in our builds since back in v1.7.9, before I took over the maintainership, since that one clearly is pretty stable...) Thanks! -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pager_in_use: make sure output is still going to pager
On Mon, Aug 10, 2015 at 7:24 PM, Jeff King p...@peff.net wrote:
On Mon, Aug 10, 2015 at 06:38:10PM +0200, Johannes Schindelin wrote:
+const char *pipe_id_get(int fd)
+{
+ static struct strbuf id = STRBUF_INIT;
+ struct stat st;
+
+ if (fstat(fd, st) 0 || !S_ISFIFO(st.st_mode))
+ return NULL;
Just a quick note: it seems that this check is not really working on
Windows. I tested this by running this test case manually (because TTY
is not set on Windows):
Yeah, I'm not too surprised. I'm guessing your st_ino for pipes are all
just the same or something. Or maybe S_ISFIFO doesn't pass (we don't
technically need it, I don't think, and could just drop that check).
If you remove the S_ISFIFO check, you probably need to include
the st_dev field in the pipe id. Otherwise, you could be unlucky and
redirect the output to a file that just happens to have the same inode
number as the pager pipe. Remember, inode numbers are only unique
within a certain st_dev.
/ceder
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/17] add_to_alternates_file: don't add duplicate entries
On Tue, Aug 11, 2015 at 06:00:20AM +0200, Michael Haggerty wrote: Instead of using hold_lock_file_for_append, let's copy the file line by line, which ensures all records are properly terminated. If we see an extra line, we can simply abort the update (there is no point in even copying the rest, as we know that it would be identical to the original). Do we have reason to expect that a lot of people have alternates files that already contain duplicate lines? You say that this function is only called from `git clone`, so I guess the answer is no. But if I'm wrong, it might be friendly to de-dup the existing lines while copying them. You're right; this is not something we expect. There's not really any way to get an alternates file inside the running clone, except by putting it in your templates/ directory. So I think it is OK to not worry about cleaning up an existing mess. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Question
Hi, I'm wondering why gitbash dont have wget? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Error pushing new branch: value too great for base (error token is...
Thanks for the detailed explanation Eric! That really helped clear my doubts. Also tried with 0x and it's working fine however, as suggested by you, i will use '=' Thanks again! :) On Tue, Aug 11, 2015 at 4:17 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Mon, Aug 10, 2015 at 6:29 PM, Jacob Keller jacob.kel...@gmail.com wrote: On Mon, Aug 10, 2015 at 2:54 AM, Gaurav Chhabra varuag.chha...@gmail.com wrote: Apologies for the delay in reply! I tried your suggestion and it works. Thanks! :) I'm curious why integer comparison is throwing error. Shouldn't i be comparing numbers with numeric operator? Yes, but shell doesn't treat hex numbers as numbers. So it will work only if the string is a decimal number. This particular case deserves a bit more explanation. The expression in question was this: if [[ $new_sha -eq $NULL ]]; then where 'new_sha' was 9226289d2416af4cb7365d7aaa5e382bdb3d9a89. In Bash, inside the [[ .. ]], it did attempt evaluating the SHA1 as a *decimal* number, however, when it encountered the d, it complained that it was outside the allowed range of decimal digits (0...9). Had the SHA1 been prefixed by a 0x, the [[...]] context would have dealt with it just fine. Outside the [[...]] context, arguments to -eq do need to be base-10 integers. Nevertheless, a SHA1 is effective an opaque value. There's little, if anything, to be gained by treating it as a numeric quantity, hence string '=' makes more sense than numeric '-eq'. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 05/17] remove hold_lock_file_for_append
On Mon, Aug 10, 2015 at 03:36:14PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: No users of hold_lock_file_for_append remain, so remove it. This does not seem to have anything to do with rotating static buffers used in get_pathname(); the only effect it has is to conflict heavily with Michael's tempfile topic X-. Yeah, the first patch (to drop the final caller) is why I stuck it in this series, and I did not want to forget the rest of the topic that Jim worked on. Perhaps this should be part of Michael's tempfile topic? Yes, I think that is OK. We can keep the first patch (to add_to_alternates_file) here, and do the other one later on top of Michael's topic. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] worktree: add 'list' command
On Mon, Aug 10, 2015 at 6:10 PM, Junio C Hamano gits...@pobox.com wrote:
Michael Rappazzo rappa...@gmail.com writes:
+static int list(int ac, const char **av, const char *prefix)
+{
+ int main_only = 0;
+ struct option options[] = {
+ OPT_BOOL(0, main-only, main_only, N_(only list the main
worktree)),
+ OPT_END()
+ };
Hmm, main-only is still there?
Sorry, I missed that.
+ int is_bare = is_bare_repository();
Please do not introduce decl-after-stmt.
Since I reused this value below, I thought it would be acceptable.
+ if (is_bare) {
+ strbuf_addstr(main_path, absolute_path(common_dir));
Hmm, interesting.
Because .git/config is shared, core.bare read from that tells us if
the main one is bare, even if you start this command from one of
its linked worktrees. So in that sense, this test of is_bare
correctly tells if main one is a bare repository.
But that by itself feels wrong. Doesn't the presense of a working
tree mean that you should not get is_bare==true in such a case
(i.e. your main one is bare, you are in a linked worktree of it
that has the index and the working tree)?
Is is even correct for a bare repo to be included in the list? I
think that is part of what you are asking here.
+ strbuf_strip_suffix(main_path, /.);
In any case, what is that stripping of /. about? Who is adding
that extra trailing string?
What I am getting at is (1) perhaps it shouldn't be adding that in
the first place, and (2) if some other code is randomly adding /.
at the end, what guarantees you that you would need to strip it only
once here---if the other code added that twice, don't you have to
repeatedly remove /. from the end?
In the case of a common dir, the value returned is just '.'. I wanted
to resolve that to the full path, so I called
`absolute_path(common_dir)`. Hence the trailing /.. Similarly, in
the main repo, the common dir is just .git, and I handled that by
using `get_git_work_tree()`.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 04/10] branch: roll show_detached HEAD into regular ref_list
On Tue, Aug 11, 2015 at 8:11 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak karthik@gmail.com wrote:
Remove show_detached() and make detached HEAD to be rolled into
regular ref_list by adding REF_DETACHED_HEAD as a kind of branch and
supporting the same in append_ref(). This eliminates the need for an
extra function and helps in easier porting of branch.c to use
ref-filter APIs.
Before show_detached() used to check if the HEAD branch satisfies the
'--contains' option, now that is taken care by append_ref().
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/builtin/branch.c b/builtin/branch.c
index 65f6d0d..81815c9 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -535,6 +540,7 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
int color;
struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;
const char *prefix = ;
+ const char *desc = item-name;
if (item-ignore)
return;
@@ -547,6 +553,10 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
color = BRANCH_COLOR_REMOTE;
prefix = remote_prefix;
break;
+ case REF_DETACHED_HEAD:
+ color = BRANCH_COLOR_CURRENT;
+ desc = get_head_description();
+ break;
default:
color = BRANCH_COLOR_PLAIN;
break;
@@ -558,7 +568,7 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
color = BRANCH_COLOR_CURRENT;
}
- strbuf_addf(name, %s%s, prefix, item-name);
+ strbuf_addf(name, %s%s, prefix, desc);
if (verbose) {
int utf8_compensation = strlen(name.buf) -
utf8_strwidth(name.buf);
strbuf_addf(out, %c %s%-*s%s, c, branch_get_color(color),
@@ -581,6 +591,8 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
}
strbuf_release(name);
strbuf_release(out);
+ if (item-kind == REF_DETACHED_HEAD)
+ free((void *)desc);
This would be cleaner, and more easily extended to other cases if you
used a 'to_free' variable. For instance, something like this:
const char *desc = item-name;
char *to_free = NULL;
...
case REF_DETACHED_HEAD:
...
desc = to_free = get_head_description();
break;
...
strbuf_addf(name, %s%s, prefix, desc);
...
free(to_free);
This looks neater!
Note that it's safe to free 'to_free' when it's NULL, so you don't
need to protect the free() with that ugly specialized 'if' which
checks for REF_DETACHED_HEAD. You can also do away with the cast to
non-const when freeing.
Yea makes sense will change to what you suggested.
}
@@ -642,7 +638,14 @@ static int print_ref_list(int kinds, int detached, int
verbose, int abbrev, stru
cb.ref_list = ref_list;
cb.pattern = pattern;
cb.ret = 0;
+ /*
+* First we obtain all regular branch refs and then if the
s/then//
Thanks
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
bug: git-archive does not use the zip64 extension for archives with more than 16k entries
Hi, for repositories with more than 16k files and folders, git-archive will create zip files which store the wrong number of entries. That is, it stores the number of entries modulo 16k. This will break unpackers that do not include code to support this brokenness. Instead, git-archive should use the zip64 extension to handle more than 16k files and folders correctly. Thanks! cheers, josch signature.asc Description: signature
Re: [PATCH v3] worktree: add 'list' command
On Mon, Aug 10, 2015 at 10:55 PM, David Turner dtur...@twopensource.com wrote:
On Mon, 2015-08-10 at 16:53 -0400, Michael Rappazzo wrote:
+ while ((d = readdir(dir)) != NULL) {
I think it would be useful to break this loop out into a
for_each_worktree function.
While looking into per-worktree ref stuff, I have just noticed that git
prune will delete objects that are only referenced in a different
worktree's detached HEAD. To fix this, git prune will need to walk over
each worktree, looking at that worktree's HEAD (and other per-worktree
refs). It would be useful to be able to reuse some of this code for
that task.
I agree, but I will save that for another round.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/10] branch: bump get_head_description() to the top
On Tue, Aug 11, 2015 at 7:29 AM, Eric Sunshine sunsh...@sunshineco.com wrote: On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak karthik@gmail.com wrote: This is a preperatory patch for 'roll show_detached HEAD into regular ref_list'. This patch moves get_head_descrition() to the top so that s/descrition/description/ it can be used in print_ref_item(). Signed-off-by: Karthik Nayak karthik@gmail.com Thanks, will change. -- Regards, Karthik Nayak -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[bug] git-svn segmentation fault
Hello, I use git-svn. I have used it without any problem for two month. Now git starts crashing after svn rebase (maybe also after other operations with a remote svn server, I'll check when will do svn dcommit next time). It successfully done the operation and only then it crashes: $ git svn rebase Current branch master is up to date. error: git-svn died of signal 11 $ dmesg | tail -n5 [518105.393218] git-svn[25148]: segfault at 7f81c0775c50 ip 7f81c0775c50 sp 7ffca025adc8 error 14 in Glob.so[7f81c0979000+6000] [518113.627053] git-svn[25487]: segfault at 7f0547a76c50 ip 7f0547a76c50 sp 7ffe31c39958 error 14 in Glob.so[7f0547c7a000+6000] [518137.038469] git-svn[25677]: segfault at 7fe124d4fc50 ip 7fe124d4fc50 sp 7ffc821fc848 error 14 in Glob.so[7fe124f53000+6000] [518173.339068] git-svn[25864]: segfault at 7f0919006c50 ip 7f0919006c50 sp 7ffe78e51b58 error 14 in Glob.so[7f091920a000+6000] [519070.924619] git-svn[26467]: segfault at 7f119202ec50 ip 7f119202ec50 sp 7fff2af3a948 error 14 in Glob.so[7f1192232000+6000] This behaviour is persistent and I can reproduce it any time. Versions info: $ uname -a Linux hs-arch 4.1.4-1-ARCH #1 SMP PREEMPT Mon Aug 3 21:30:37 UTC 2015 x86_64 GNU/Linux $ git --version git version 2.5.0 $ svn --version svn, version 1.8.13 (r1667537) compiled Jun 3 2015, 05:30:35 on x86_64-unknown-linux-gnu Copyright (C) 2014 The Apache Software Foundation. This software consists of contributions made by many people; see the NOTICE file for more information. Subversion is open source software, see http://subversion.apache.org/ The following repository access (RA) modules are available: * ra_svn : Module for accessing a repository using the svn network protocol. - with Cyrus SASL authentication - handles 'svn' scheme * ra_local : Module for accessing a repository on local disk. - handles 'file' scheme * ra_serf : Module for accessing a repository via WebDAV protocol using serf. - using serf 1.3.8 - handles 'http' scheme - handles 'https' scheme $ perl --version This is perl 5, version 22, subversion 0 (v5.22.0) built for x86_64-linux-thread-multi Copyright 1987-2015, Larry Wall Perl may be copied only under the terms of either the Artistic License or the GNU General Public License, which may be found in the Perl 5 source kit. Complete documentation for Perl, including FAQ lists, should be found on this system using man perl or perldoc perl. If you have access to the Internet, point your browser at http://www.perl.org/, the Perl Home Page. I don't know exact version of Subversion on the remote server (web ui doesn't show it). Core dump: http://madkite.cc/git/core.git-svn.1000.e28aeb54778749879c9313b05ea040e8.26467.143928204700.lz4 I have tried on another environment (cygwin x64, git 2.4.5, svn 1.8.13, perl 5.14.4) - it works fine with the same repo. Also my environment works fine with other repos. Is there anything else you need from me? Best regards, Roman. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Merging after directory got turned into submodule
Hi! I've noticed that if I turn a subdirectory into a submodule, I'm having severe trouble merging branches from before that change, even if they don't modify that subdirectory at all. I've posted this problem on Stack Overflow and started a bounty for it. See http://stackoverflow.com/q/31821219/1468366. So far I haven't received an answer, so I decided to ask here as well. Here is an example. # Create one project, to be used as a subproject later on git init a cd a echo aaa aa git add -A git commit -m a1 cd .. # Create a second project, containing a as a normal directory initially git init b cd b mkdir a b echo aaa a/aa echo bbb b/bb git add -A git commit -m b1 # Replace directory with submodule git rm -r a git submodule add ../a a git commit -m b2 # Create feature brach starting at version without submodule git submodule deinit . # will error if I don't do this git checkout -b branch HEAD^ echo abc b/bb git commit -a -m b3 # Try to merge the feature branch git checkout master git merge branch This prints an error message: CONFLICT (file/directory): There is a directory with name a in branch. Adding a as a~HEAD Automatic merge failed; fix conflicts and then commit the result. I get the same error if I do a git submodule update --init before the git merge branch. I don't see any a~HEAD anywhere, neither in my directory tree nor in the output from git status, which reads like this: On branch master You have unmerged paths. (fix conflicts and run git commit) Changes to be committed: modified: b/bb Unmerged paths: (use git add file... to mark resolution) added by us: a If I do git add a as suggested, I get another error: error: unable to index file a fatal: updating files failed If I do git submodules update --init just before the merge, then I can do git add a successfully. But if I forget to do so, and then try doing that after the merge, I receive this error message: Submodule 'a' (…/a) registered for path 'a' Skipping unmerged submodule a How do I recover from this situation? Something other than git merge --abort, since I'd like to use it for things like git rebase as well, and since in some scenarios (don't know how to reproduce) I couldn't even abort the merge cleanly, and had to do a hard reset instead. How can I avoid it in the first place? Is there some magic setting which makes git do the right thing with submodules vs. directories during merges, so that I don't have to manually post-process a merge which only modifies files unrelated to the submodules? If there is no easy way to avoid this, do you think I should file a bug report for this? After all, replacing a subdirectory by a submodule shouldn't be that rare, and neither should be merges across such a change. So in my opinion, git should be able to cope with this. Greetings, Martin von Gagern -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/10] branch: refactor width computation
On Tue, Aug 11, 2015 at 7:28 AM, Eric Sunshine sunsh...@sunshineco.com wrote:
On Tue, Aug 4, 2015 at 9:01 AM, Karthik Nayak karthik@gmail.com wrote:
Remove unnecessary variables from ref_list and ref_item which were
used for width computation. This is to make ref_item similar to
ref-filter's ref_array_item. This will ensure a smooth port of
branch.c to use ref-filter APIs in further patches.
Previously the maxwidth was computed when inserting the refs into the
ref_list. Now, we obtain the entire ref_list and then compute
maxwidth.
Signed-off-by: Karthik Nayak karthik@gmail.com
---
diff --git a/builtin/branch.c b/builtin/branch.c
index 4fc8beb..b058b74 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -557,16 +552,21 @@ static void print_ref_item(struct ref_item *item, int
maxwidth, int verbose,
-static int calc_maxwidth(struct ref_list *refs)
+static int calc_maxwidth(struct ref_list *refs, int remote_bonus)
{
- int i, w = 0;
+ int i, max = 0;
for (i = 0; i refs-index; i++) {
+ struct ref_item *it = refs-list[i];
+ int w = utf8_strwidth(it-name);
+
if (refs-list[i].ignore)
continue;
Nit: Unnecessary work. You compute the width and then immediately
throw it away when 'ignore' is true.
Also, you use 'it' elsewhere rather than 'refs-list[i]' but not this
line, which makes it seem out of place. I would have expected to see:
if (it-ignore)
continue;
(Despite the noisier diff, the end result will be more consistent.)
Ah right, will put that after the check :D
Yea, that seems out of place. Thanks
--
Regards,
Karthik Nayak
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 2/2] notes: handle multiple worktrees
On Mon, Aug 10, 2015 at 7:52 PM, David Turner dtur...@twopensource.com wrote: Before creating NOTES_MERGE_REF, check NOTES_MERGE_REF using find_shared_symref and die if we find one. This prevents simultaneous merges to the same notes branch from different worktrees. Signed-off-by: David Turner dtur...@twopensource.com Still looks good to me, AFAICS. Feel free to add my Reviewed-by. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Merging after directory got turned into submodule
On Tue, Aug 11, 2015 at 03:02:41PM +0200, Martin von Gagern wrote: I've noticed that if I turn a subdirectory into a submodule, I'm having severe trouble merging branches from before that change, even if they don't modify that subdirectory at all. I've posted this problem on Stack Overflow and started a bounty for it. See http://stackoverflow.com/q/31821219/1468366. So far I haven't received an answer, so I decided to ask here as well. Here is an example. # Create one project, to be used as a subproject later on git init a cd a echo aaa aa git add -A git commit -m a1 cd .. # Create a second project, containing a as a normal directory initially git init b cd b mkdir a b echo aaa a/aa echo bbb b/bb git add -A git commit -m b1 # Replace directory with submodule git rm -r a git submodule add ../a a git commit -m b2 # Create feature brach starting at version without submodule git submodule deinit . # will error if I don't do this git checkout -b branch HEAD^ echo abc b/bb git commit -a -m b3 # Try to merge the feature branch git checkout master git merge branch This prints an error message: CONFLICT (file/directory): There is a directory with name a in branch. Adding a as a~HEAD Automatic merge failed; fix conflicts and then commit the result. I get the same error if I do a git submodule update --init before the git merge branch. I don't see any a~HEAD anywhere, neither in my directory tree nor in the output from git status, which reads like this: On branch master You have unmerged paths. (fix conflicts and run git commit) Changes to be committed: modified: b/bb Unmerged paths: (use git add file... to mark resolution) added by us: a If I do git add a as suggested, I get another error: git reset a works for me (i.e. set a to what it was before merging). If you then commit and check git ls-tree HEAD it shows a is still a submodule. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Question
Hi, On 2015-08-11 10:28, Jet Rey Maza wrote: I'm wondering why gitbash dont have wget? Please take the time to write coherent questions in the future, giving enough context for others to understand what you are talking about. We are not dogs that you throw some bones, you know? We are highly skilled software developers and you might want to express some respect for that by crafting a pleasant email. As to your question, I have to guess here because you are so parsimonious with context: are you referring to Git for Windows' Git Bash entry in the start menu? Assuming that this is what you meant, the explanation is simple: Git does not require wget to work. So we do not ship it with Git for Windows. Side note: Git for Windows *does* ship with curl.exe, for historical reasons. You can probably use curl instead of wget for your use case. Second side note: you obviously seek more than just Git for Windows, so why not use MSys2 (https://msys2.github.io/) which comes with a wget package you can install via pacman? Ciao, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usage: try harder to format very long error messages
On Tue, Aug 11, 2015 at 09:28:34AM -0700, Stefan Beller wrote: On Tue, Aug 11, 2015 at 9:17 AM, Jeff King p...@peff.net wrote: We use a fixed-size buffer of 4096 bytes to format die() and error() messages. We explicitly avoided using a strbuf or other fanciness here, because we want to make sure that we report the message even in the face of malloc() failure (after all, we might even be dying due to a malloc call). Would it make sense to allocate memory in the early startup phase for a possible error message? So instead of putting 4kb on the stack we'd just have an unused 16kb on the heap. Isn't that just punting on the problem? Now your 16kb filename will get truncated messages (in general we cannot even work with such files, but it is nice if the error message telling us so is readable). If stack space is the problem, we can just put 16kb in BSS. But I think we really do want something that grows to the appropriate size. Or we need to start being more clever about our truncation. E.g., printing: error: unable to stat 'a[...]aa/foo': File too long where the [...] is literally what we would print. The trouble with that approach is that it is hard to intercept large strings without re-implementing all of stdio's formatting. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usage: try harder to format very long error messages
Jeff King p...@peff.net writes: We use a fixed-size buffer of 4096 bytes to format die() and error() messages. We explicitly avoided using a strbuf or other fanciness here, because we want to make sure that we report the message even in the face of malloc() failure (after all, we might even be dying due to a malloc call). Yes, that was the rationale behind the we use a fixed-buffer and try not to allocate in this codepath, I think. As for vwritef, it exists solely to work over write(), _and_ it doesn't get the one-shot thing right (which is probably OK, as we use it only when exec fails). But we could probably teach run-command to fdopen(), and get rid of it entirely (in favor of teaching vreportf to take a FILE* handle instead of assuming stderr). Sounds like a plan ;-) The patch looks very sensible. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] worktree: add 'list' command
Mike Rappazzo rappa...@gmail.com writes:
+ int is_bare = is_bare_repository();
Please do not introduce decl-after-stmt.
Since I reused this value below, I thought it would be acceptable.
Use of a new variable is fine. Do not declare one in a block after
you already wrote statement is what decl-after-stmt not allowed
means. In your patch:
+static int list(int ac, const char **av, const char *prefix)
+{
+ int main_only = 0;
+ struct option options[] = {
+ OPT_BOOL(0, main-only, main_only, N_(only list the main
worktree)),
+ OPT_END()
+ };
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac)
+ usage_with_options(worktree_usage, options);
+
+ struct strbuf main_path = STRBUF_INIT;
+ const char* common_dir = get_git_common_dir();
+ int is_bare = is_bare_repository();
Three variables, main_path, common_dir and is_bare are declared here
after statements such as a call to parse_options(). Don't.
+static int list(int ac, const char **av, const char *prefix)
+{
+ int main_only = 0;
+ struct strbuf main_path = STRBUF_INIT;
+ const char *common_dir;
+ int is_bare;
+ struct option options[] = {
+ OPT_BOOL(0, main-only, main_only, N_(only list the main
worktree)),
+ OPT_END()
+ };
+
+ ac = parse_options(ac, av, prefix, options, worktree_usage, 0);
+ if (ac)
+ usage_with_options(worktree_usage, options);
+
+ common_dir = get_git_common_dir();
+ int is_bare = is_bare_repository();
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] worktree: add 'list' command
David Turner dtur...@twopensource.com writes:
On Mon, 2015-08-10 at 16:53 -0400, Michael Rappazzo wrote:
+while ((d = readdir(dir)) != NULL) {
I think it would be useful to break this loop out into a
for_each_worktree function.
Very good point.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usage: try harder to format very long error messages
We use a fixed-size buffer of 4096 bytes to format die() and
error() messages. We explicitly avoided using a strbuf or
other fanciness here, because we want to make sure that we
report the message even in the face of malloc() failure
(after all, we might even be dying due to a malloc call).
In most cases, this buffer is long enough to show any
reasonable message. However, if you are experimenting with
_unreasonable_ things (e.g., filenames approaching 4096
bytes), the messages can be truncated, making them
confusing. E.g., seeing:
error: cannot stat 'aa
is much less useful than:
error: cannot stat 'aaa/foo': File too long
(these are truncated for the sake of readability; obviously
real examples are much longer. Just imagine many lines full
of a's).
This patch teaches vreportf and vwritef to at least try
using a dynamically sized buffer before falling back to the
fixed-size buffer. For most long errors (which are typically
reporting problems with syscalls on long filenames), this
means we'll generally see the full message. And in case that
fails, we still print the truncated message, but with a note
that it was truncated.
Note that die_errno does not need the same treatment for its
fixed-size buffers, as they are a combination of:
- our fixed-size string constants, without placeholders
expanded (so a literal cannot stat '%s', without %s
expanded to arbitrary data)
- the results of strerror(errno)
both of which should be predictably small.
Signed-off-by: Jeff King p...@peff.net
---
So this solution tries to change vreportf and vwritef as little as
possible, and ends up slightly complex as a result. But reading
vreportf's:
vsnprintf(msg, sizeof(msg), err, params);
fprintf(stderr, %s%s\n, prefix, msg);
I had to wonder why this wasn't just:
fputs(prefix, stderr);
vfprintf(stderr, err, params);
In fact, we used to do this, but changed it in d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09). I'm not sure of
the reasoning there, though. I would expect stdio to generally write
that as a single shot already, assuming:
- there isn't anything in the stderr buffer already (i.e., we do not
need to flush somewhere in the middle)
- our prefix does not have a newline (which it doesn't; it is
error:, fatal:, etc).
IOW, I wonder if it would be enough to simply fflush(stderr) before and
after to make sure we keep the buffer clear. I also wonder if this is
even enough as-is; if the resulting message is larger than stdio's
buffer size, I'd expect it to come through across several writes anyway.
As for vwritef, it exists solely to work over write(), _and_ it doesn't
get the one-shot thing right (which is probably OK, as we use it only
when exec fails). But we could probably teach run-command to fdopen(),
and get rid of it entirely (in favor of teaching vreportf to take a
FILE* handle instead of assuming stderr).
So I dunno. I think the solution here works fine, but maybe we should be
taking the opportunity to simplify.
usage.c | 72 +
1 file changed, 64 insertions(+), 8 deletions(-)
diff --git a/usage.c b/usage.c
index ed14645..78b0d75 100644
--- a/usage.c
+++ b/usage.c
@@ -6,23 +6,79 @@
#include git-compat-util.h
#include cache.h
+/*
+ * This buffer allows you to try formatting a full message, but if malloc
+ * fails, will fall back to a fixed-size buffer and truncate the message.
+ * If we truncate the message, it adds a note indicating so.
+ *
+ * No initialization is necessary before robust_buf_fmt, and after it returns,
+ * buf points to the contents (whether truncated or not). You should always
+ * robust_buf_free the result, which handles both cases.
+ */
+struct robust_buf {
+ char *buf;
+ char fixed[4096];
+};
+
+static int robust_buf_fmt(struct robust_buf *rb,
+ const char *fmt, va_list ap)
+{
+ static const char trunc[] = [message truncated];
+ static const char broken[] = BUG: your vsnprintf is broken;
+ va_list cp;
+ int len;
+
+ va_copy(cp, ap);
+ len = vsnprintf(rb-fixed, sizeof(rb-fixed), fmt, cp);
+ va_end(cp);
+
+ if (len 0) {
+ memcpy(rb-fixed, broken, sizeof(broken));
+ rb-buf = rb-fixed;
+ return sizeof(broken) - 1;
+ }
+ if (len sizeof(rb-fixed)) {
+ rb-buf = rb-fixed;
+ return len;
+ }
+
+ rb-buf = malloc(len + 1);
+ if (!rb-buf) {
+ memcpy(rb-fixed + sizeof(rb-fixed) - sizeof(trunc),
+ trunc, sizeof(trunc));
+ rb-buf = rb-fixed;
+ return sizeof(rb-fixed) - 1;
+ }
+
+ if (vsnprintf(rb-buf, len + 1, fmt, ap) = len)
+ ; /* insatiable vsnprintf, nothing we can do */
+ return len;
+}
+
+static void robust_buf_free(struct robust_buf *rb)
+{
+ if
Re: [PATCH] usage: try harder to format very long error messages
On Tue, Aug 11, 2015 at 9:17 AM, Jeff King p...@peff.net wrote:
We use a fixed-size buffer of 4096 bytes to format die() and
error() messages. We explicitly avoided using a strbuf or
other fanciness here, because we want to make sure that we
report the message even in the face of malloc() failure
(after all, we might even be dying due to a malloc call).
Would it make sense to allocate memory in the early startup phase
for a possible error message?
So instead of putting 4kb on the stack we'd just have an unused 16kb
on the heap.
In most cases, this buffer is long enough to show any
reasonable message. However, if you are experimenting with
_unreasonable_ things (e.g., filenames approaching 4096
bytes), the messages can be truncated, making them
confusing. E.g., seeing:
error: cannot stat 'aa
is much less useful than:
error: cannot stat 'aaa/foo': File too long
(these are truncated for the sake of readability; obviously
real examples are much longer. Just imagine many lines full
of a's).
This patch teaches vreportf and vwritef to at least try
using a dynamically sized buffer before falling back to the
fixed-size buffer. For most long errors (which are typically
reporting problems with syscalls on long filenames), this
means we'll generally see the full message. And in case that
fails, we still print the truncated message, but with a note
that it was truncated.
Note that die_errno does not need the same treatment for its
fixed-size buffers, as they are a combination of:
- our fixed-size string constants, without placeholders
expanded (so a literal cannot stat '%s', without %s
expanded to arbitrary data)
- the results of strerror(errno)
both of which should be predictably small.
Signed-off-by: Jeff King p...@peff.net
---
So this solution tries to change vreportf and vwritef as little as
possible, and ends up slightly complex as a result. But reading
vreportf's:
vsnprintf(msg, sizeof(msg), err, params);
fprintf(stderr, %s%s\n, prefix, msg);
I had to wonder why this wasn't just:
fputs(prefix, stderr);
vfprintf(stderr, err, params);
In fact, we used to do this, but changed it in d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09). I'm not sure of
the reasoning there, though. I would expect stdio to generally write
that as a single shot already, assuming:
- there isn't anything in the stderr buffer already (i.e., we do not
need to flush somewhere in the middle)
- our prefix does not have a newline (which it doesn't; it is
error:, fatal:, etc).
IOW, I wonder if it would be enough to simply fflush(stderr) before and
after to make sure we keep the buffer clear. I also wonder if this is
even enough as-is; if the resulting message is larger than stdio's
buffer size, I'd expect it to come through across several writes anyway.
As for vwritef, it exists solely to work over write(), _and_ it doesn't
get the one-shot thing right (which is probably OK, as we use it only
when exec fails). But we could probably teach run-command to fdopen(),
and get rid of it entirely (in favor of teaching vreportf to take a
FILE* handle instead of assuming stderr).
So I dunno. I think the solution here works fine, but maybe we should be
taking the opportunity to simplify.
usage.c | 72
+
1 file changed, 64 insertions(+), 8 deletions(-)
diff --git a/usage.c b/usage.c
index ed14645..78b0d75 100644
--- a/usage.c
+++ b/usage.c
@@ -6,23 +6,79 @@
#include git-compat-util.h
#include cache.h
+/*
+ * This buffer allows you to try formatting a full message, but if malloc
+ * fails, will fall back to a fixed-size buffer and truncate the message.
+ * If we truncate the message, it adds a note indicating so.
+ *
+ * No initialization is necessary before robust_buf_fmt, and after it
returns,
+ * buf points to the contents (whether truncated or not). You should always
+ * robust_buf_free the result, which handles both cases.
+ */
+struct robust_buf {
+ char *buf;
+ char fixed[4096];
+};
+
+static int robust_buf_fmt(struct robust_buf *rb,
+ const char *fmt, va_list ap)
+{
+ static const char trunc[] = [message truncated];
+ static const char broken[] = BUG: your vsnprintf is broken;
+ va_list cp;
+ int len;
+
+ va_copy(cp, ap);
+ len = vsnprintf(rb-fixed, sizeof(rb-fixed), fmt, cp);
+ va_end(cp);
+
+ if (len 0) {
+ memcpy(rb-fixed, broken, sizeof(broken));
+ rb-buf = rb-fixed;
+ return sizeof(broken) - 1;
+ }
+ if (len sizeof(rb-fixed)) {
+ rb-buf = rb-fixed;
+ return len;
+ }
+
+ rb-buf = malloc(len + 1);
+ if (!rb-buf) {
+ memcpy(rb-fixed + sizeof(rb-fixed) -
Re: [PATCH 01/10] ref-filter: add option to filter only branches
Karthik Nayak karthik@gmail.com writes:
From: Karthik Nayak karthik@gmail.com
Add an option in 'filter_refs()' to use 'for_each_branch_ref()'
and filter refs. This type checking is done by adding a
'FILTER_REFS_BRANCHES' in 'ref-filter.h'.
Add an option in 'ref_filter_handler()' to filter different
types of branches by calling 'filter_branch_kind()' which
checks for the type of branch needed.
Mentored-by: Christian Couder christian.cou...@gmail.com
Mentored-by: Matthieu Moy matthieu@grenoble-inp.fr
Signed-off-by: Karthik Nayak karthik@gmail.com
---
ref-filter.c | 47 +++
ref-filter.h | 10 --
2 files changed, 55 insertions(+), 2 deletions(-)
diff --git a/ref-filter.c b/ref-filter.c
index de84dd4..c573109 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1044,6 +1044,46 @@ static const unsigned char *match_points_at(struct
sha1_array *points_at,
return NULL;
}
+/*
+ * Checks if a given refname is a branch and returns the kind of
+ * branch it is. If not a branch, 0 is returned.
+ */
+static int filter_branch_kind(struct ref_filter *filter, const char *refname)
+{
+ int kind, i;
+
+ static struct {
+ const char *prefix;
+ int kind;
Make a mental note that this is signed int.
+ } ref_kind[] = {
+ { refs/heads/ , REF_LOCAL_BRANCH },
+ { refs/remotes/ , REF_REMOTE_BRANCH },
+ };
+
+ /* If no kind is specified, no need to filter */
+ if (!filter-branch_kind)
+ return REF_NO_BRANCH_FILTERING;
+
+ for (i = 0; i ARRAY_SIZE(ref_kind); i++) {
+ if (starts_with(refname, ref_kind[i].prefix)) {
+ kind = ref_kind[i].kind;
+ break;
+ }
+ }
+
+ if (ARRAY_SIZE(ref_kind) = i) {
+ if (!strcmp(refname, HEAD))
+ kind = REF_DETACHED_HEAD;
+ else
+ return 0;
+ }
+
+ if ((filter-branch_kind kind) == 0)
+ return 0;
+
+ return kind;
+}
While this looks fine, I am not sure if this is a good interface for
filtering, though.
If you start from already having everything and want to limit things
down to refs/heads/, this might make sense but it would be far
more efficient, if you know that you want to limit to refs/heads/
upfront, not to collect everything but just limit the collection to
those under refs/heads/ without wasting cycles in the first place.
diff --git a/ref-filter.h b/ref-filter.h
index 5be3e35..b5a13e8 100644
--- a/ref-filter.h
+++ b/ref-filter.h
@@ -16,6 +16,12 @@
#define FILTER_REFS_INCLUDE_BROKEN 0x1
#define FILTER_REFS_ALL 0x2
#define FILTER_REFS_TAGS 0x4
+#define FILTER_REFS_BRANCHES 0x8
Is this a sensible set of bits? Does it make sense to have ALL and
TAGS at the same time (and what does that mean?)?
+#define REF_DETACHED_HEAD 0x01
+#define REF_LOCAL_BRANCH0x02
+#define REF_REMOTE_BRANCH 0x04
+#define REF_NO_BRANCH_FILTERING 0x08
Where do these values go? It is a returned by filter-branch-kind
for each ref, i.e. given refs/heads/bar, it answers Yeah, that is
a local branch. Why are these values pretending to be a set of
bits that can be combined together, as if a branch can be both LOCAL
and REMOTE? This does not make _any_ sense.
#define ALIGN_LEFT 0x01
#define ALIGN_RIGHT 0x02
@@ -50,7 +56,7 @@ struct ref_sorting {
struct ref_array_item {
unsigned char objectname[20];
- int flag;
+ int flag, kind;
For readability, do not define multiple structure fields on a single
line.
If you are storing a set of bits in an integer, use unsigned. If it
is an enumeration, int is fine.
const char *symref;
struct commit *commit;
struct atom_value *value;
@@ -76,7 +82,7 @@ struct ref_filter {
unsigned int with_commit_tag_algo : 1,
match_as_path : 1;
- unsigned int lines;
+ unsigned int lines, branch_kind;
For readability, do not define multiple structure fields on a single
line.
};
struct ref_filter_cbdata {
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] vreportf: avoid intermediate buffer
When we call die(fmt, args...), we end up in vreportf with
two pieces of information:
1. The prefix fatal:
2. The original fmt and va_list of args.
We format item (2) into a temporary buffer, and then fprintf
the prefix and the temporary buffer, along with a newline.
This has the unfortunate side effect of truncating any error
messages that are longer than 4096 bytes.
Instead, let's use separate calls for the prefix and
newline, letting us hand the item (2) directly to vfprintf.
This is essentially undoing d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09), which
tried to have the whole output end up in a single `write`
call.
But we can address this instead by explicitly requesting
line-buffering for the output handle, and by making sure
that the buffer is empty before we start (so that outputting
the prefix does not cause a flush due to hitting the buffer
limit).
We may still break the output into two writes if the content
is larger than our buffer, but there's not much we can do
there; depending on the stdio implementation, that might
have happened even with a single fprintf call.
Signed-off-by: Jeff King p...@peff.net
---
I am not 100% sure on the last statement. On my system, it seems that:
fprintf(stderr, %s%s\n, prefix, msg);
will generally result in a single write when stderr is unbuffered (i.e.,
it's default state). Which feels very magical, since it clearly must be
preparing the output in a single buffer to feed to write, and the
contents of prefix and msg may easily exceed BUFSIZ. It looks like
glibc internally uses an 8K buffer to generate unbuffered content.
E.g., if I do:
strace -o /dev/fd/3 -e write \
git --no-pager log --$(perl -e 'print a x 4096') \
32 2/dev/null
I get:
write(2, fatal: unrecognized argument: --..., 4129) = 4129
and if I bump the 4096 to 16384, I get:
write(2, fatal: unrecognized argument: --..., 8192) = 8192
write(2, ..., 8192) = 8192
write(2, ..., 33) = 33
So that's kind of weird. I suspect other stdio implementations may
behave differently, and AFAIK the standard is quiet on the subject (so
it would be OK for an implementation to output the prefix, the msg, and
the newline in separate writes, no matter their length).
usage.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/usage.c b/usage.c
index e4fa6d2..82ff131 100644
--- a/usage.c
+++ b/usage.c
@@ -7,13 +7,21 @@
#include cache.h
static FILE *error_handle;
+static int tweaked_error_buffering;
void vreportf(const char *prefix, const char *err, va_list params)
{
- char msg[4096];
FILE *fh = error_handle ? error_handle : stderr;
- vsnprintf(msg, sizeof(msg), err, params);
- fprintf(fh, %s%s\n, prefix, msg);
+
+ fflush(fh);
+ if (!tweaked_error_buffering) {
+ setvbuf(fh, NULL, _IOLBF, 0);
+ tweaked_error_buffering = 1;
+ }
+
+ fputs(prefix, fh);
+ vfprintf(fh, err, params);
+ fputc('\n', fh);
}
static NORETURN void usage_builtin(const char *err, va_list params)
@@ -70,6 +78,7 @@ void set_die_is_recursing_routine(int (*routine)(void))
void set_error_handle(FILE *fh)
{
error_handle = fh;
+ tweaked_error_buffering = 0;
}
void NORETURN usagef(const char *err, ...)
--
2.5.0.417.g2db689c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting
Karthik Nayak karthik@gmail.com writes:
-static void print_value(struct atom_value *v, int quote_style)
+static void format_quote_value(struct atom_value *v, int quote_style, struct
strbuf *output)
{
Hmph...
-static void emit(const char *cp, const char *ep)
+static void append_non_atom(const char *cp, const char *ep, struct strbuf
*output)
I was tempted to suggest s/non_atom/literal/, but this would do for now...
@@ -1262,19 +1257,20 @@ static void emit(const char *cp, const char *ep)
void show_ref_array_item(struct ref_array_item *info, const char *format,
int quote_style)
{
const char *cp, *sp, *ep;
+ struct strbuf output = STRBUF_INIT;
for (cp = format; *cp (sp = find_next(cp)); cp = ep + 1) {
struct atom_value *atomv;
ep = strchr(sp, ')');
if (cp sp)
- emit(cp, sp);
+ append_non_atom(cp, sp, output);
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- print_value(atomv, quote_style);
+ format_quote_value(atomv, quote_style, output);
If the one to add a literal string (with %hex escaping) is called append_,
then this should be called append_quoted_atom() or something, no?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 03/13] ref-filter: introduce ref_formatting_state
Karthik Nayak karthik@gmail.com writes:
get_ref_atom_value(info, parse_ref_filter_atom(sp + 2, ep),
atomv);
- format_quote_value(atomv, quote_style, output);
+ set_formatting_state(atomv, state);
+ format_quote_value(atomv, state);
+ perform_state_formatting(state, final_buf);
}
if (*cp) {
sp = cp + strlen(cp);
- append_non_atom(cp, sp, output);
+ append_non_atom(cp, sp, state);
+ perform_state_formatting(state, final_buf);
}
With the two helpers being very sketchy at this stage, it is very
hard to judge if they make sense. At the conceptual level, I can
see that set-formatting-state is to allow an atom to affect the
state before the value of the atom is emitted into the buffer.
I cannot tell what perform-state-formatting is meant to do from
these call sites.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 04/13] utf8: add function to align a string into given strbuf
Karthik Nayak karthik@gmail.com writes:
+void strbuf_utf8_align(struct strbuf *buf, align_type position, unsigned int
width,
+const char *s)
+{
+ int display_len = utf8_strnwidth(s, strlen(s), 0);
+ int utf8_compenstation = strlen(s) - display_len;
compensation, perhaps? But notice you are running two strlen and
then also a call to utf8-strnwidth here already, and then
+ if (!strlen(s))
+ return;
you return here without doing anything.
Worse yet, this logic looks very strange. If you give it a width of
8 because you want to align like-item on each record at that column,
a record with 1-display-column item will be shown in 8-display-column
with 7 SP padding (either at the beginning, or at the end, or on
both ends to center). If it happens to be an empty string, the entire
8-display-column disappears.
Is that really what you meant to do? The logic does not make much
sense to me, even though the code may implement that logic correctly
and clearly.
+ if (display_len = width) {
+ strbuf_addstr(buf, s);
+ return;
+ }
Mental note: this function values the information more than
presentation; it refuses to truncate (to lose information) and
instead accepts jaggy output. OK.
+ if (position == ALIGN_LEFT)
+ strbuf_addf(buf, %-*s, width + utf8_compenstation, s);
+ else if (position == ALIGN_MIDDLE) {
+ int left = (width - display_len)/2;
+ strbuf_addf(buf, %*s%-*s, left, , width - left +
utf8_compenstation, s);
+ } else if (position == ALIGN_RIGHT)
+ strbuf_addf(buf, %*s, width + utf8_compenstation, s);
+}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v10 02/13] ref-filter: print output to strbuf for formatting
Karthik Nayak karthik@gmail.com writes: @@ -1283,9 +1279,11 @@ void show_ref_array_item(struct ref_array_item *info, const char *format, int qu if (color_parse(reset, color) 0) die(BUG: couldn't parse 'reset' as a color); resetv.s = color; - print_value(resetv, quote_style); + format_quote_value(resetv, quote_style, output); Mental note: I _think_ the logic to scan the string and set need_color_reset_at_eol that happens at the beginning can be removed once the code fully utilizes formatting-state information. A coloring atom would leave a bit in the formatting state to say that the line color has been changed to something other than reset, and then this at the end of line code can decide if that is the case and add a reset thing here (i.e. the code inside the if (need_color_reset_at_eol) block shown here does not need to change, but the if condition would). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usage: try harder to format very long error messages
On Tue, Aug 11, 2015 at 09:34:31AM -0700, Junio C Hamano wrote: As for vwritef, it exists solely to work over write(), _and_ it doesn't get the one-shot thing right (which is probably OK, as we use it only when exec fails). But we could probably teach run-command to fdopen(), and get rid of it entirely (in favor of teaching vreportf to take a FILE* handle instead of assuming stderr). Sounds like a plan ;-) So here's an alternative series that does this, along with the single-fprintf thing I mentioned. The first patch is actually orthogonal to the second; we would probably want it whichever path we choose to fix vreportf's truncation (I'd just have to rebase the earlier patch on top of it if we go that route). [1/2]: vreportf: report to arbitrary filehandles [2/2]: vreportf: avoid intermediate buffer -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] vreportf: report to arbitrary filehandles
The vreportf function always goes to stderr, but run-command
wants child errors to go to the parent's original stderr. To
solve this, commit a5487dd duplicates the stderr fd and
installs die and error handlers to direct the output
appropriately (which later turned into the vwritef
function). This has two downsides, though:
- we make multiple calls to write(), which contradicts the
write at once logic from d048a96 (print
warning/error/fatal messages in one shot, 2007-11-09).
- the custom handlers basically duplicate the normal
handlers. They're only a few lines of code, but we
should not have to repeat the magic exit(128), for
example.
We can solve the first by using fdopen() on the duplicated
descriptor. We can't pass this to vreportf, but we could
introduce a new vreportf_to to handle it.
However, to fix the second problem, we instead introduce a
new set_error_handle function, which lets the normal
vreportf calls output to a handle besides stderr. Thus we
can get rid of our custom handlers entirely, and just ask
the regular handlers to output to our new descriptor.
And as vwritef has no more callers, it can just go away.
Signed-off-by: Jeff King p...@peff.net
---
git-compat-util.h | 2 +-
run-command.c | 17 ++---
usage.c | 22 +-
3 files changed, 12 insertions(+), 29 deletions(-)
diff --git a/git-compat-util.h b/git-compat-util.h
index c6d391f..076461e 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -389,7 +389,6 @@ struct strbuf;
/* General helper functions */
extern void vreportf(const char *prefix, const char *err, va_list params);
-extern void vwritef(int fd, const char *prefix, const char *err, va_list
params);
extern NORETURN void usage(const char *err);
extern NORETURN void usagef(const char *err, ...) __attribute__((format
(printf, 1, 2)));
extern NORETURN void die(const char *err, ...) __attribute__((format (printf,
1, 2)));
@@ -425,6 +424,7 @@ static inline int const_error(void)
extern void set_die_routine(NORETURN_PTR void (*routine)(const char *err,
va_list params));
extern void set_error_routine(void (*routine)(const char *err, va_list
params));
extern void set_die_is_recursing_routine(int (*routine)(void));
+extern void set_error_handle(FILE *);
extern int starts_with(const char *str, const char *prefix);
diff --git a/run-command.c b/run-command.c
index 4d73e90..0d01671 100644
--- a/run-command.c
+++ b/run-command.c
@@ -200,7 +200,6 @@ static int execv_shell_cmd(const char **argv)
#endif
#ifndef GIT_WINDOWS_NATIVE
-static int child_err = 2;
static int child_notifier = -1;
static void notify_parent(void)
@@ -212,17 +211,6 @@ static void notify_parent(void)
*/
xwrite(child_notifier, , 1);
}
-
-static NORETURN void die_child(const char *err, va_list params)
-{
- vwritef(child_err, fatal: , err, params);
- exit(128);
-}
-
-static void error_child(const char *err, va_list params)
-{
- vwritef(child_err, error: , err, params);
-}
#endif
static inline void set_cloexec(int fd)
@@ -362,11 +350,10 @@ fail_pipe:
* in subsequent call paths use the parent's stderr.
*/
if (cmd-no_stderr || need_err) {
- child_err = dup(2);
+ int child_err = dup(2);
set_cloexec(child_err);
+ set_error_handle(fdopen(child_err, w));
}
- set_die_routine(die_child);
- set_error_routine(error_child);
close(notify_pipe[0]);
set_cloexec(notify_pipe[1]);
diff --git a/usage.c b/usage.c
index ed14645..e4fa6d2 100644
--- a/usage.c
+++ b/usage.c
@@ -6,23 +6,14 @@
#include git-compat-util.h
#include cache.h
+static FILE *error_handle;
+
void vreportf(const char *prefix, const char *err, va_list params)
{
char msg[4096];
+ FILE *fh = error_handle ? error_handle : stderr;
vsnprintf(msg, sizeof(msg), err, params);
- fprintf(stderr, %s%s\n, prefix, msg);
-}
-
-void vwritef(int fd, const char *prefix, const char *err, va_list params)
-{
- char msg[4096];
- int len = vsnprintf(msg, sizeof(msg), err, params);
- if (len sizeof(msg))
- len = sizeof(msg);
-
- write_in_full(fd, prefix, strlen(prefix));
- write_in_full(fd, msg, len);
- write_in_full(fd, \n, 1);
+ fprintf(fh, %s%s\n, prefix, msg);
}
static NORETURN void usage_builtin(const char *err, va_list params)
@@ -76,6 +67,11 @@ void set_die_is_recursing_routine(int (*routine)(void))
die_is_recursing = routine;
}
+void set_error_handle(FILE *fh)
+{
+ error_handle = fh;
+}
+
void NORETURN usagef(const char *err, ...)
{
va_list params;
--
2.5.0.417.g2db689c
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH v5 2/5] refs: add ref_type function
On Mon, 2015-08-03 at 20:55 +0700, Duy Nguyen wrote: On Fri, Jul 31, 2015 at 1:06 PM, David Turner dtur...@twopensource.com wrote: Add a function ref_type, which categorizes refs as per-worktree, pseudoref, or normal ref. For per-worktree refs, you probably should follow common_list[] in path.c because that's how file-based ref namespace is splitted between per-repo and per-worktree, even though just as simple as everything outside refs/ is per-worktree (with an exception of NOTES_MERGE_REF, which should be on the list as well). At least the two should be aligned so that the default file-based backend works the same way as new backends. I've looked into this, and decided not to follow common_list. That's because I've hacked the path.c code to treat refs/worktree specially; it's under refs (common), but it's per-worktree, so it's special-cased. You may have seen this in the per-worktree-refs-for-bisect thread: http://permalink.gmane.org/gmane.comp.version-control.git/275673 This will require some special-casing in the alternate backends, but they can use common the is_per_worktree_ref function. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 07/16] prepare_tempfile_object(): new function, extracted from create_tempfile()
Michael Haggerty mhag...@alum.mit.edu writes: This makes the next step easier. The old code used to use path to set the initial length of tempfile-filename. This was not helpful because path was usually relative whereas the value stored to filename will be absolute. So just initialize the length to 0. Makes sense. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
config options for automatic signed tags and signed pushes
If it doesn't already exist (not that I can find). I'd like to see config options analogous to commit.gpgsign for both tagging and pushing. Not sure where else to send this request though, let me know if there's a better place. Thanks, -- Matthew Thode signature.asc Description: OpenPGP digital signature
Re: [PATCH v2 03/16] lockfile: add accessors get_lock_file_fd() and get_lock_file_fp()
Michael Haggerty mhag...@alum.mit.edu writes: We are about to move those members, so change client code to read them through accessor functions. Hmph, _fp() variant does not seem to be used at all at this step, though. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/5] pseudorefs: create and use pseudoref update and delete functions
On Fri, 2015-07-31 at 16:40 -0700, Stefan Beller wrote:
I am sorry for being late to the review, I looked into coverity today as Duy
bugged me to fix the memory allocation stuff[1]
Thanks. Junio, can you pleas substitute the attached patch instead? It
addresses Stefan's issues. If you prefer, I can resend the whole
series, but I thought this might be easier.
From 6c86c38f533c6b35db3a557270aab95b342875c9 Mon Sep 17 00:00:00 2001
From: David Turner dtur...@twopensource.com
Date: Wed, 15 Jul 2015 18:05:28 -0400
Subject: [PATCH 3/5] pseudorefs: create and use pseudoref update and delete
functions
Pseudorefs should not be updated through the ref transaction
API, because alternate ref backends still need to store pseudorefs
in GIT_DIR (instead of wherever they store refs). Instead,
change update_ref and delete_ref to call pseudoref-specific
functions.
Signed-off-by: David Turner dtur...@twopensource.com
---
refs.c | 103 -
1 file changed, 95 insertions(+), 8 deletions(-)
diff --git a/refs.c b/refs.c
index 0f87884..e44b88c 100644
--- a/refs.c
+++ b/refs.c
@@ -2874,12 +2874,90 @@ enum ref_type ref_type(const char *refname)
return REF_TYPE_NORMAL;
}
+static int write_pseudoref(const char *pseudoref, const unsigned char *sha1,
+ const unsigned char *old_sha1, struct strbuf *err)
+{
+ const char *filename;
+ int fd;
+ static struct lock_file lock;
+ struct strbuf buf = STRBUF_INIT;
+ int ret = -1;
+
+ strbuf_addf(buf, %s\n, sha1_to_hex(sha1));
+
+ filename = git_path(%s, pseudoref);
+ fd = hold_lock_file_for_update(lock, filename, LOCK_DIE_ON_ERROR);
+ if (fd 0) {
+ strbuf_addf(err, Could not open '%s' for writing: %s,
+ filename, strerror(errno));
+ return -1;
+ }
+
+ if (old_sha1) {
+ unsigned char actual_old_sha1[20];
+ if (read_ref(pseudoref, actual_old_sha1))
+ die(Could not read ref %s, pseudoref);
+ if (hashcmp(actual_old_sha1, old_sha1)) {
+ strbuf_addf(err, Unexpected sha1 when writing %s, pseudoref);
+ rollback_lock_file(lock);
+ goto done;
+ }
+ }
+
+ if (write_in_full(fd, buf.buf, buf.len) != buf.len) {
+ strbuf_addf(err, Could not write to '%s', filename);
+ rollback_lock_file(lock);
+ goto done;
+ }
+
+ commit_lock_file(lock);
+ ret = 0;
+done:
+ strbuf_release(buf);
+ return ret;
+}
+
+static int delete_pseudoref(const char *pseudoref, const unsigned char *old_sha1)
+{
+ static struct lock_file lock;
+ const char *filename;
+
+ filename = git_path(%s, pseudoref);
+
+ if (old_sha1 !is_null_sha1(old_sha1)) {
+ int fd;
+ unsigned char actual_old_sha1[20];
+
+ fd = hold_lock_file_for_update(lock, filename,
+ LOCK_DIE_ON_ERROR);
+ if (fd 0)
+ die_errno(_(Could not open '%s' for writing), filename);
+ if (read_ref(pseudoref, actual_old_sha1))
+ die(Could not read ref %s, pseudoref);
+ if (hashcmp(actual_old_sha1, old_sha1)) {
+ warning(Unexpected sha1 when deleting %s, pseudoref);
+ rollback_lock_file(lock);
+ return -1;
+ }
+
+ unlink(filename);
+ rollback_lock_file(lock);
+ } else {
+ unlink(filename);
+ }
+
+ return 0;
+}
+
int delete_ref(const char *refname, const unsigned char *old_sha1,
unsigned int flags)
{
struct ref_transaction *transaction;
struct strbuf err = STRBUF_INIT;
+ if (ref_type(refname) == REF_TYPE_PSEUDOREF)
+ return delete_pseudoref(refname, old_sha1);
+
transaction = ref_transaction_begin(err);
if (!transaction ||
ref_transaction_delete(transaction, refname, old_sha1,
@@ -3973,17 +4051,25 @@ int update_ref(const char *msg, const char *refname,
const unsigned char *new_sha1, const unsigned char *old_sha1,
unsigned int flags, enum action_on_err onerr)
{
- struct ref_transaction *t;
+ struct ref_transaction *t = NULL;
struct strbuf err = STRBUF_INIT;
+ int ret = 0;
- t = ref_transaction_begin(err);
- if (!t ||
- ref_transaction_update(t, refname, new_sha1, old_sha1,
- flags, msg, err) ||
- ref_transaction_commit(t, err)) {
+ if (ref_type(refname) == REF_TYPE_PSEUDOREF) {
+ ret = write_pseudoref(refname, new_sha1, old_sha1, err);
+ } else {
+ t = ref_transaction_begin(err);
+ if (!t ||
+ ref_transaction_update(t, refname, new_sha1, old_sha1,
+ flags, msg, err) ||
+ ref_transaction_commit(t, err)) {
+ ret = 1;
+ ref_transaction_free(t);
+ }
+ }
+ if (ret) {
const char *str = update_ref failed for ref '%s': %s;
- ref_transaction_free(t);
switch (onerr) {
case UPDATE_REFS_MSG_ON_ERR:
error(str, refname, err.buf);
@@ -3998,7 +4084,8 @@ int update_ref(const char *msg, const char *refname,
return 1;
}
strbuf_release(err);
- ref_transaction_free(t);
+ if (t)
+ ref_transaction_free(t);
return 0;
}
--
2.4.2.617.g3d4cca8-twtrsrc
Re: [PATCH v10 05/13] ref-filter: implement an `align` atom
Karthik Nayak karthik@gmail.com writes:
struct atom_value{
Obviously not a problem with this step, but you need a SP before the
open brace.
@@ -692,6 +704,26 @@ static void populate_value(struct ref_array_item *ref)
else
v-s = ;
continue;
+ } else if (skip_prefix(name, align:, valp)) {
+ struct align *align = xmalloc(sizeof(struct align));
+
+ if (skip_prefix(valp, left,, valp))
+ align-position = ALIGN_LEFT;
+ else if (skip_prefix(valp, right,, valp))
+ align-position = ALIGN_RIGHT;
+ else if (skip_prefix(valp, middle,, valp))
+ align-position = ALIGN_MIDDLE;
+ else
+ die(_(improper format entered align:%s),
valp);
+ if (strtoul_ui(valp, 10, align-width))
+ die(_(positive width expected align:%s),
valp);
Minor nits on the design. %(align:width[,position]) would let
us write %(align:16)...%(end) and use the default position, which
may be beneficial if one kind of alignment is prevalent (I guess all
the internal users left-align?) %(align:position,width) forces
users to spell both out all the time.
@@ -1198,7 +1230,9 @@ void ref_array_sort(struct ref_sorting *sorting, struct
ref_array *array)
struct ref_formatting_state {
struct strbuf output;
+ struct align *align;
int quote_style;
+ unsigned int end : 1;
};
Mental note: it is not clear why you need 'end' field in the state.
Perhaps it is an indication that the division of labor is poorly
designed between the helper that updates the formatting state and
the other helper that reflects the formatting state to the final
string.
@@ -1262,12 +1296,31 @@ static void append_non_atom(const char *cp, const
char *ep, struct ref_formattin
static void set_formatting_state(struct atom_value *atomv, struct
ref_formatting_state *state)
{
- /* Based on the atomv values, the formatting state is set */
+ if (atomv-align) {
+ state-align = atomv-align;
+ atomv-align = NULL;
+ }
+ if (atomv-end)
+ state-end = 1;
+}
+
+static int align_ref_strbuf(struct ref_formatting_state *state, struct
strbuf *final)
+{
+ if (state-align state-end) {
... and I think that is what I see. If this function knows that we
are processing %(end), i.e. perform-state-formatting is called for
each atom and receives atomv, there wouldn't have to be a code like
this.
+ struct align *align = state-align;
+ strbuf_utf8_align(final, align-position, align-width,
state-output.buf);
+ strbuf_reset(state-output);
+ state-align = NULL;
+ return 1;
+ } else if (state-align)
+ return 1;
+ return 0;
}
static void perform_state_formatting(struct ref_formatting_state *state,
struct strbuf *final)
{
- /* More formatting options to be eventually added */
+ if (align_ref_strbuf(state, final))
+ return;
At the design level, I have a strong suspicion that it is a wrong
way to go. It piles more if (this state bit was left by the
previous atom) then do this on this function and will make an
unmanageable mess.
You have a dictionary of all possible atoms somewhere. Why not hook
a pointer to the handler function (or two) to each element in it,
instead of duplicating this one is special information down to
individual atom instantiations (i.e. atomv) as atomv.modifier_atom
bit, an dstructure the caller more like this?
get_ref_atom_value(info, parse_ref_filter_atom, atomv);
if (atomv-pre_handler)
atomv-pre_handler(atomv, state);
format_quote_value(atomv, state);
if (atomv-post_handler)
atomv-post_handler(atomv, state);
Actually, each atom could just have a single handler; an atom like
%(refname:short) whose sole effect is to append atomv-s to the
state buffer can point a function to do so in its handler.
On the other hand, align atom's handler would push a new state on
the stack, marking that it is the one to handle diverted output.
align_atom_handler(atomv, state)
{
struct format_state *new = push_new_state(state);
strbuf_init(new-output);
new-atend = align_handler;
new-return_to = atomv; /* or whatever that holds width,pos */
}
Then end atom's handler would pop the state from the stack, and the
processing to be done
end_atom_handler(atomv, state)
{
state-atend(state);
pop_state(state);
}
and the called align_handler would be something like:
align_handler(state)
{
Re: [PATCH v2 01/16] Move lockfile documentation to lockfile.h and lockfile.c
Michael Haggerty mhag...@alum.mit.edu writes:
Rearrange/rewrite it somewhat to fit its new environment.
...
diff --git a/lockfile.h b/lockfile.h
index b4abc61..a483cc9 100644
--- a/lockfile.h
+++ b/lockfile.h
@@ -4,54 +4,103 @@
...
@@ -68,16 +117,51 @@ struct lock_file {
#define LOCK_SUFFIX .lock
#define LOCK_SUFFIX_LEN 5
+
+/*
+ * Flags
+ * -
+ *
+ * The following flags can be passed to `hold_lock_file_for_update()`
+ * or `hold_lock_file_for_append()`.
+ */
+
+/*
+ * If a lock is already taken for the file, `die()` with an error
+ * message. If this flag is not specified, trying to lock a file that
+ * is already locked returns -1 to the caller.
+ */
#define LOCK_DIE_ON_ERROR 1
+
+/*
+ * Usually symbolic links in the destination path are resolved. This
+ * means that (1) the lockfile is created by adding .lock to the
+ * resolved path, and (2) upon commit, the resolved path is
+ * overwritten. However, if `LOCK_NO_DEREF` is set, then the lockfile
+ * is created by adding .lock to the path argument itself. This
+ * option is used, for example, when detaching a symbolic reference,
+ * which for backwards-compatibility reasons, can be a symbolic link
+ * containing the name of the referred-to-reference.
+ */
...
Thanks. I really like the way these per-item descriptions explain
each item much better. The old documentation may have contained all
the same info, but a better organization makes a big difference.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/16] lockfile: add accessor get_lock_file_path()
Michael Haggerty mhag...@alum.mit.edu writes: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- I was briefly confused by the similarity between get_locked_file_path() and this new helper ;-) but names of both make sense to me. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/16] lock_repo_for_gc(): compute the path to gc.pid only once
On Tue, Aug 11, 2015 at 1:06 PM, Junio C Hamano gits...@pobox.com wrote: Looks correct; somehow this reminded me of the other topic from Peff to reduce use of git_path() ;-) - pidfile = git_pathdup(gc.pid); + pidfile = pidfile_path; sigchain_push_common(remove_pidfile_on_signal); atexit(remove_pidfile); I wonder if you can reduce the atexit() here by registering this as a tempfile to be cleared? Heh, I should have been slightly more patient. That is what 14/16 is about ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 12/16] diff: use tempfile module
Michael Haggerty mhag...@alum.mit.edu writes:
Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
---
diff.c | 29 +++--
1 file changed, 7 insertions(+), 22 deletions(-)
Nice code reduction.
diff --git a/diff.c b/diff.c
index 7500c55..dc95247 100644
--- a/diff.c
+++ b/diff.c
@@ -2,6 +2,7 @@
* Copyright (C) 2005 Junio C Hamano
*/
#include cache.h
+#include tempfile.h
#include quote.h
#include diff.h
#include diffcore.h
@@ -312,7 +313,7 @@ static struct diff_tempfile {
const char *name; /* filename external diff should read from */
char hex[41];
char mode[10];
- char tmp_path[PATH_MAX];
+ struct tempfile tempfile;
} diff_temp[2];
typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
@@ -564,25 +565,16 @@ static struct diff_tempfile *claim_diff_tempfile(void) {
die(BUG: diff is failing to clean up its tempfiles);
}
-static int remove_tempfile_installed;
-
static void remove_tempfile(void)
{
int i;
for (i = 0; i ARRAY_SIZE(diff_temp); i++) {
- if (diff_temp[i].name == diff_temp[i].tmp_path)
- unlink_or_warn(diff_temp[i].name);
+ if (is_tempfile_active(diff_temp[i].tempfile))
+ delete_tempfile(diff_temp[i].tempfile);
I suspect that this indicates that there is something iffy in the
conversion. The original invariant, that is consistently used
between claim_diff_tempfile() and remove_tempfile(), is that .name
field points at .tmp_path for a slot in diff_temp[] that holds a
temporary that is in use. Otherwise, .name is NULL and it can be
claimed for your own use.
Here the updated code uses a different and new invariant: .tempfile
satisfies is_tempfile_active() for a slot in use. But the check in
claim_diff_tempfile() still relies on the original invariant.
The updated code may happen to always have an active tempfile in
tempfile and always set NULL when it clears .name, but that would
mean (1) future changes may easily violate one of invariants (we
used to have only one, now we have two that have to be sync) by
mistake, and (2) we are keeping track of two closely linked things
as two invariants.
As the value that used to be in the .name field can now be obtained
by calling get_tempfile_path() on the .tempfile field, perhaps we
should drop .name (and its associated invariant) at the same time?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] revisions --stdin: accept CRLF line terminators
On Windows, 'git rebase -i' with rebase.missingCommitsCheck set to
warn or error reports:
Dropped commits (newer to older):
'atal: bad revision '410dee56...
The error comes from the git rev-list --stdin invocation in
git-rebase--interactive.sh (function check_todo_list). It is caused by
CRs that end up in the file $todo.miss, because many tools of the MSYS
toolset force LF to CRLF conversion when regular files are written via
stdout.
To fix the error, permit CRLF line terminators when revisions and
pathspec are read using the --stdin option.
Signed-off-by: Johannes Sixt j...@kdbg.org
---
This fixes a new failure in the test suite (t3404.8[67]) on Windows, but
I got around to debug it only now.
revision.c| 4
t/t6017-rev-list-stdin.sh | 16
2 files changed, 20 insertions(+)
diff --git a/revision.c b/revision.c
index cf60c5d..4efedeb 100644
--- a/revision.c
+++ b/revision.c
@@ -1641,6 +1641,8 @@ static void read_pathspec_from_stdin(struct rev_info
*revs, struct strbuf *sb,
int len = sb-len;
if (len sb-buf[len - 1] == '\n')
sb-buf[--len] = '\0';
+ if (len sb-buf[len - 1] == '\r')
+ sb-buf[--len] = '\0';
ALLOC_GROW(prune-path, prune-nr + 1, prune-alloc);
prune-path[prune-nr++] = xstrdup(sb-buf);
}
@@ -1661,6 +1663,8 @@ static void read_revisions_from_stdin(struct rev_info
*revs,
int len = sb.len;
if (len sb.buf[len - 1] == '\n')
sb.buf[--len] = '\0';
+ if (len sb.buf[len - 1] == '\r')
+ sb.buf[--len] = '\0';
if (!len)
break;
if (sb.buf[0] == '-') {
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 667b375..34c43cf 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -75,4 +75,20 @@ test_expect_success 'not only --stdin' '
test_cmp expect actual
'
+test_expect_success 'accept CRLF line terminators' '
+ cat expect -\EOF
+ 7
+
+ file-2
+ EOF
+ q_to_cr input -\EOF
+ masterQ
+ ^master^Q
+ --Q
+ file-2Q
+ EOF
+ git log --pretty=tformat:%s --name-only --stdin input actual
+ test_cmp expect actual
+'
+
test_done
--
2.3.2.245.gb5bf9d3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/16] Introduce a tempfile module
Thanks for a pleasant read. All looked reasonable. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH ee/clean-remove-dirs] t7300-clean: require POSIXPERM for chmod 0 test
A test case introduced by 91479b9c (t7300: add tests to document behavior of clean and nested git) uses 'chmod 0' to verify that a subdirectory that has an unreadable .git file is not removed. This can work only when the system pays attention to the permissions set with 'chmod'. Therefore, set the POSIXPERM prerequisite on the test case. Signed-off-by: Johannes Sixt j...@kdbg.org --- This fixes a new failure in the test suite (t3404.8[67]) on Windows, but I got around to debug it only now. t/t7300-clean.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh index 32e96da..27557d6 100755 --- a/t/t7300-clean.sh +++ b/t/t7300-clean.sh @@ -499,7 +499,7 @@ test_expect_success 'should not clean submodules' ' test_path_is_missing to_clean ' -test_expect_success 'should avoid cleaning possible submodules' ' +test_expect_success POSIXPERM 'should avoid cleaning possible submodules' ' rm -fr to_clean possible_sub1 mkdir to_clean possible_sub1 test_when_finished rm -rf possible_sub* -- 2.3.2.245.gb5bf9d3 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 13/16] lock_repo_for_gc(): compute the path to gc.pid only once
Michael Haggerty mhag...@alum.mit.edu writes: Signed-off-by: Michael Haggerty mhag...@alum.mit.edu --- builtin/gc.c | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/builtin/gc.c b/builtin/gc.c index 36fe333..c41354b 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -199,6 +199,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) uintmax_t pid; FILE *fp; int fd; + char *pidfile_path; if (pidfile) /* already locked */ @@ -207,12 +208,13 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid) if (gethostname(my_host, sizeof(my_host))) strcpy(my_host, unknown); - fd = hold_lock_file_for_update(lock, git_path(gc.pid), + pidfile_path = git_pathdup(gc.pid); + fd = hold_lock_file_for_update(lock, pidfile_path, LOCK_DIE_ON_ERROR); Looks correct; somehow this reminded me of the other topic from Peff to reduce use of git_path() ;-) - pidfile = git_pathdup(gc.pid); + pidfile = pidfile_path; sigchain_push_common(remove_pidfile_on_signal); atexit(remove_pidfile); I wonder if you can reduce the atexit() here by registering this as a tempfile to be cleared? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] revisions --stdin: accept CRLF line terminators
Junio C Hamano gits...@pobox.com writes: Johannes Sixt j...@kdbg.org writes: On Windows, 'git rebase -i' with rebase.missingCommitsCheck set to warn or error reports: Dropped commits (newer to older): 'atal: bad revision '410dee56... The error comes from the git rev-list --stdin invocation in git-rebase--interactive.sh (function check_todo_list) We have other places that take long list of things via --stdin option. It somehow feels incomplete to patch only rev-list and not others, doesn't it? I looked at hits from 'grep -e --stdin Documentation/'. Here are the findings. 1. These use strbuf_getline() to get one line at a time into a strbuf and expects the line termination stripped off (i.e. these callers do not want to worry about having LF at the end): check-attr --stdin check-ingore --stdin check-mailmap --stdin checkout-index --stdin hash-object --stdin-paths http-fetch --stdin notes --stdin send-pack --stdin update-index --index-info 2. Any command in the log family uses strbuf_getwholeline(), so it needs to know about the LF at the end explicitly: rev-list --stdin show --stdin cherry-pick --stdin ... 3. This uses fgets() into a fixed buffer; it calls get_sha1_hex() on it, and the expected input is one 40-hex per line, so it does not matter if there is an extra CR at the end immediately before LF. diff-tree --stdin 4. This slurps everything in-core, instead of going line-by-line. update-ref --stdin Now, I am wondering if it makes sense to do these two things: * Teach revision.c::read_revisions_from_stdin() to use strbuf_getline() instead of strbuf_getwholeline(). * Teach strbuf_getline() to remove CR at the end when stripping the LF at the end, only if term parameter is set to LF. Doing so would solve 1. and 2., but we obviously need to audit all the other uses of strbuf_getline() to see if they can benefit (or if some of them may be broken because they _always_ need LF terminated lines, i.e. CRLF terminated input is illegal to them). As to 3., I think it is OK. The code structure of 4. is too ugly and needs to be revamped to go one line at a time first before even thinking about how to proceed, I would think. Thoughts? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH ee/clean-remove-dirs] t7300-clean: require POSIXPERM for chmod 0 test
Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re* [PATCH v2 1/2] refs: refs/worktree/* become per-worktree
David Turner dtur...@twopensource.com writes: What does gitk do that's relevant here? Ahh, my mistake. gitk spawned from bisect visualize is not even aware of the fact that it is showing the revision ranges delimited by the bisect references. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/4] notes: add notes.merge option to select default strategy
On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller jacob.e.kel...@intel.com wrote:
From: Jacob Keller jacob.kel...@gmail.com
Teach git-notes about notes.merge to select a general strategy for all
notes merges. This enables a user to always get expected merge strategy
such as cat_sort_uniq without having to pass the -s option manually.
Signed-off-by: Jacob Keller jacob.kel...@gmail.com
---
Documentation/config.txt| 7 ++
Documentation/git-notes.txt | 14 +++-
builtin/notes.c | 44
+++--
notes-merge.h | 16 --
t/t3309-notes-merge-auto-resolve.sh | 29
5 files changed, 86 insertions(+), 24 deletions(-)
[...]
diff --git a/builtin/notes.c b/builtin/notes.c
index 63f95fc55439..3c705f5e26ff 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -737,6 +737,24 @@ static int merge_commit(struct notes_merge_options *o)
return ret;
}
+static int parse_notes_strategy(const char *arg, enum notes_merge_strategy
*strategy)
+{
+ if (!strcmp(arg, manual))
+ *strategy = NOTES_MERGE_RESOLVE_MANUAL;
+ else if (!strcmp(arg, ours))
+ *strategy = NOTES_MERGE_RESOLVE_OURS;
+ else if (!strcmp(arg, theirs))
+ *strategy = NOTES_MERGE_RESOLVE_THEIRS;
+ else if (!strcmp(arg, union))
+ *strategy = NOTES_MERGE_RESOLVE_UNION;
+ else if (!strcmp(arg, cat_sort_uniq))
+ *strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
+ else
+ return -1;
+
+ return 0;
+}
+
static int merge(int argc, const char **argv, const char *prefix)
{
struct strbuf remote_ref = STRBUF_INIT, msg = STRBUF_INIT;
@@ -745,7 +763,7 @@ static int merge(int argc, const char **argv, const char
*prefix)
struct notes_merge_options o;
int do_merge = 0, do_commit = 0, do_abort = 0;
int verbosity = 0, result;
- const char *strategy = NULL;
+ const char *strategy = NULL, *configured_strategy = NULL;
struct option options[] = {
OPT_GROUP(N_(General options)),
OPT__VERBOSITY(verbosity),
@@ -795,21 +813,15 @@ static int merge(int argc, const char **argv, const
char *prefix)
expand_notes_ref(remote_ref);
o.remote_ref = remote_ref.buf;
- if (strategy) {
- if (!strcmp(strategy, manual))
- o.strategy = NOTES_MERGE_RESOLVE_MANUAL;
- else if (!strcmp(strategy, ours))
- o.strategy = NOTES_MERGE_RESOLVE_OURS;
- else if (!strcmp(strategy, theirs))
- o.strategy = NOTES_MERGE_RESOLVE_THEIRS;
- else if (!strcmp(strategy, union))
- o.strategy = NOTES_MERGE_RESOLVE_UNION;
- else if (!strcmp(strategy, cat_sort_uniq))
- o.strategy = NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ;
- else {
- error(Unknown -s/--strategy: %s, strategy);
- usage_with_options(git_notes_merge_usage, options);
- }
+ git_config_get_string_const(notes.merge, configured_strategy);
+
+ if (configured_strategy
+ parse_notes_strategy(configured_strategy, o.strategy))
+ die(Unknown notes merge strategy: %s, configured_strategy);
+
+ if (strategy parse_notes_strategy(strategy, o.strategy)) {
+ error(Unknown -s/--strategy: %s, strategy);
+ usage_with_options(git_notes_merge_usage, options);
}
Do you need to look at the notes.merge config variable at all if
-s/--strategy is given?
I'd expect the above to rather look something like:
if (strategy) {
if (parse_notes_strategy(strategy, o.strategy)) {
error(Unknown -s/--strategy: %s, strategy);
usage_with_options(git_notes_merge_usage, options);
}
} else if (configured_strategy) {
if (parse_notes_strategy(configured_strategy, o.strategy))
die(Unknown notes merge strategy: %s, configured_strategy);
} /* else o.strategy = NOTES_MERGE_RESOLVE_MANUAL; */
Otherwise, this patch looks good to me.
...Johan
--
Johan Herland, jo...@herland.net
www.herland.net
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe
On Tue, Aug 11, 2015 at 6:56 PM, Junio C Hamano gits...@pobox.com wrote: Eric Sunshine sunsh...@sunshineco.com writes: On Tue, Aug 11, 2015 at 4:51 PM, Johannes Sixt j...@kdbg.org wrote: Invoking plink requires special treatment, and we have support and even test cases for the commands 'plink' and 'tortoiseplink'. We also support .exe variants for these two and there is a test for 'plink.exe'. On Windows, however, where support for plink.exe would be relevant, the test case fails because it is not possible to execute a file with a .exe extension that is actually not a binary executable---it is a shell script in our test. We have to disable the test case on Windows. Considering, that 'plink.exe' is irrelevant on non-Windows, let's just remove the test and assume that the code just works. putty and plink are used on Unix as well. A quick check of Mac OS X, Linux, and FreeBSD reveals that package managers on each platform have putty and plink packages available. But they do not force their users to say plink.exe, but instead let them invoke plink, no? The test before the one that was removed is about plink (sans .exe), and what was removed is with .exe, so I think J6t's patch is OK. Ah, you're correct. I overlooked the extra emphasis j6t's commit message placed on .exe. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3] send-email: provide whitelist of SMTP AUTH mechanisms
When sending an e-mail, the client and server must agree on an
authentication mechanism. Some servers (due to misconfiguration
or a bug) deny valid credentials for certain mechanisms. In this
patch, a new option --smtp-auth and configuration entry smtpAuth
are introduced. If smtp_auth is defined, it works as a whitelist
of allowed mechanisms for authentication selected from the ones
supported by the installed SASL perl library.
Signed-off-by: Jan Viktorin vikto...@rehivetech.com
---
Documentation/git-send-email.txt | 11 +++
git-send-email.perl | 26 +-
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt
index f14705e..82c6ae8 100644
--- a/Documentation/git-send-email.txt
+++ b/Documentation/git-send-email.txt
@@ -171,6 +171,17 @@ Sending
to determine your FQDN automatically. Default is the value of
'sendemail.smtpDomain'.
+--smtp-auth=mechs::
+ Whitespace-separated list of allowed SMTP-AUTH mechanisms. This setting
+ forces using only the listed mechanisms. Example:
+
+ $ git send-email --smtp-auth=PLAIN LOGIN GSSAPI ...
+
+ If at least one of the specified mechanisms matches the ones advertised
by the
+ SMTP server and if it is supported by the utilized SASL library, the
mechanism
+ is used for authentication. If neither 'sendemail.smtpAuth' nor
'--smtp-auth'
+ is specified, all mechanisms supported by the SASL library can be used.
+
--smtp-pass[=password]::
Password for SMTP-AUTH. The argument is optional: If no
argument is specified, then the empty string is used as
diff --git a/git-send-email.perl b/git-send-email.perl
index b660cc2..a7192c4 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -75,6 +75,8 @@ git send-email [options] file | directory | rev-list options
Pass an empty string to disable
certificate
verification.
--smtp-domain str * The domain name sent to HELO/EHLO
handshake
+--smtp-auth str * Space-separated list of allowed AUTH
methods.
+ This setting forces to use one of the
listed methods.
--smtp-debug0|1 * Disable, enable Net::SMTP debug.
Automating:
@@ -208,7 +210,7 @@ my ($cover_cc, $cover_to);
my ($to_cmd, $cc_cmd);
my ($smtp_server, $smtp_server_port, @smtp_server_options);
my ($smtp_authuser, $smtp_encryption, $smtp_ssl_cert_path);
-my ($identity, $aliasfiletype, @alias_files, $smtp_domain);
+my ($identity, $aliasfiletype, @alias_files, $smtp_domain, $smtp_auth);
my ($validate, $confirm);
my (@suppress_cc);
my ($auto_8bit_encoding);
@@ -239,6 +241,7 @@ my %config_settings = (
smtppass = \$smtp_authpass,
smtpsslcertpath = \$smtp_ssl_cert_path,
smtpdomain = \$smtp_domain,
+smtpauth = \$smtp_auth,
to = \@initial_to,
tocmd = \$to_cmd,
cc = \@initial_cc,
@@ -310,6 +313,7 @@ my $rc = GetOptions(h = \$help,
smtp-ssl-cert-path=s = \$smtp_ssl_cert_path,
smtp-debug:i = \$debug_net_smtp,
smtp-domain:s = \$smtp_domain,
+ smtp-auth=s = \$smtp_auth,
identity=s = \$identity,
annotate! = \$annotate,
no-annotate = sub {$annotate = 0},
@@ -1130,6 +1134,12 @@ sub smtp_auth_maybe {
Authen::SASL-import(qw(Perl));
};
+ # Check mechanism naming as defined in:
+ # https://tools.ietf.org/html/rfc4422#page-8
+ if ($smtp_auth !~ /^(\b[A-Z0-9-_]{1,20}\s*)*$/) {
+ die invalid smtp auth: '${smtp_auth}';
+ }
+
# TODO: Authentication may fail not because credentials were
# invalid but due to other reasons, in which we should not
# reject credentials.
@@ -1142,6 +1152,20 @@ sub smtp_auth_maybe {
'password' = $smtp_authpass
}, sub {
my $cred = shift;
+
+ if ($smtp_auth) {
+ my $sasl = Authen::SASL-new(
+ mechanism = $smtp_auth,
+ callback = {
+ user = $cred-{'username'},
+ pass = $cred-{'password'},
+ authname = $cred-{'username'},
+ }
+ );
+
+ return !!$smtp-auth($sasl);
+ }
+
return !!$smtp-auth($cred-{'username'}, $cred-{'password'});
});
--
2.5.0
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes.ref.merge option
Johan Herland jo...@herland.net writes: I know that we don't yet have a proper place to put remote notes refs, but the ref in notes.ref.merge _must_ be a local notes ref (you even use the localref notation in the documentation below). Thus, I believe we can presume that the local notes ref must live under refs/notes/*, and hence drop the refs/notes/ prefix from the config key (just like branch.name.* can presume that name lives under refs/heads/*). I am OK going in that direction, as long as we promise that notes merge will forever refuse to work on --notes=$ref where $ref does not begin with refs/notes/. Except that this patch in its current form will occupy the .merge config key... Can you rename to notes.name.mergestrategy instead? This is an excellent suggestion. Or even better, take inspiration from branch.name.mergeoptions, Please don't. That is one of the design mistakes that was copied from another design mistake (remotes.*.tagopt). I'd want to see us not to repeat these design mistakes. These configuration variables were made to take free-form text value that is split according to shell rules, primarily because it was expedient to implement. Read its value into a $variable and put it at the end of the command line to let the shell split it. tagopt was done a bit more carefully in that it made to react only with a fixed string --no-tags, so it was hard to abuse, but mergeoptions allowed you to override something that you wouldn't want to (e.g. it even allowed you to feed '--message=foo'). Once you start from such a broken design, it would be hard to later make it saner, even if you wanted to. You have to retroactively forbid something that worked (with some definition of working), or you have to split, parse and then reject something that does not make sense yourself, reimplementing dequote/split rule used in the shell---which is especially problematic when you no longer write in shell scripts. So a single string value that names one of the supported strategy stored in notes.name.mergestrategy is an excellent choice. An arbitrary string in notes.name.mergeoptions is to be avoided. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: Re: [PATCH v5 3/5] clone: do not include authentication data in guessed dir
On 08/10/2015 05:48 PM, Patrick Steinhardt wrote: Sorry for the late reply (and possible re-sending), I just stumbled over this in transport.c: /* * Strip username (and password) from a URL and return * it in a newly allocated string. */ char *transport_anonymize_url(const char *url) Could we use this function for something ? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] test_terminal: redirect child process' stdin to a pty
On Fri, Aug 7, 2015 at 6:15 AM, Eric Sunshine sunsh...@sunshineco.com wrote: An alternative would be to have git-am detect that it is being tested and pretend that isatty() returns true. I would vastly prefer a solution that would work for everything, for all the C code and scripts, instead of implementing a workaround in git-am :( In this case, I implemented a generic solution in test-terminal.perl that works for POSIX systems, so if there are no problems with its implementation, I do think it's better. Other than the fact that it does not work on non-Unix platforms, of course. The other approach I would consider is to implement a xisatty() function that returns true for xisatty(0) if TEST_TTY=0 or something. However, I do wonder if this would lead us to have to hack around other functions of terminals as well (e.g. if xisatty(0), tcgetattr()), which would be a big can of worms I think... There is some precedent for having core functionality recognize that it is being tested. See, for instance, environment variable TEST_DATE_NOW, (Hmm, I took a look, and it seems that TEST_DATE_NOW is only checked in test-date.c...) and rev-list --test-bitmap. Doing so would allow the tests work on non-Unix platforms, as well. Ehh, if the non-Unix platforms do not implement terminals, it means that the git-am logic to detect if we are attempting to feed it a patch by checking if stdin is a TTY is invalid anyway, so implementing a yeah-it-is-a-tty workaround for the sake of tests would be hiding the problem, I think. Thanks, Paul -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] am: let --signoff override --no-signoff
On Fri, Aug 7, 2015 at 5:29 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:
diff --git a/builtin/am.c b/builtin/am.c
index 0961304..8c95aec 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2265,6 +2284,9 @@ int cmd_am(int argc, const char **argv, const
char *prefix)
if (resume == RESUME_FALSE)
resume = RESUME_APPLY;
+
+ if (state.signoff == SIGNOFF_EXPLICIT)
+ am_append_signoff(state);
} else {
This is clever, but I suspect there is now a chance for a double-signoff if
we passed `--signoff` to the initial `git am` call and it went through
without having to resume.
It's not present in this diff context, but this hunk modifies the code
path where in_progress is true. In other words, we only check for
SIGNOFF_EXPLICIT if
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] am: let --signoff override --no-signoff
On Wed, Aug 12, 2015 at 11:06 AM, Paul Tan pyoka...@gmail.com wrote: It's not present in this diff context, but this hunk modifies the code path where in_progress is true. In other words, we only check for SIGNOFF_EXPLICIT if ..we are resuming. (Ugh, butter fingers) Thanks, Paul -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3] send-email: provide whitelist of SMTP AUTH mechanisms
On Wed, Aug 12, 2015 at 01:39:44AM +0200, Jan Viktorin wrote: When sending an e-mail, the client and server must agree on an authentication mechanism. Some servers (due to misconfiguration or a bug) deny valid credentials for certain mechanisms. In this patch, a new option --smtp-auth and configuration entry smtpAuth are introduced. If smtp_auth is defined, it works as a whitelist of allowed mechanisms for authentication selected from the ones supported by the installed SASL perl library. Signed-off-by: Jan Viktorin vikto...@rehivetech.com --- diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index f14705e..82c6ae8 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -171,6 +171,17 @@ Sending to determine your FQDN automatically. Default is the value of 'sendemail.smtpDomain'. +--smtp-auth=mechs:: + Whitespace-separated list of allowed SMTP-AUTH mechanisms. This setting + forces using only the listed mechanisms. Example: + + $ git send-email --smtp-auth=PLAIN LOGIN GSSAPI ... + + If at least one of the specified mechanisms matches the ones advertised by the + SMTP server and if it is supported by the utilized SASL library, the mechanism + is used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth' + is specified, all mechanisms supported by the SASL library can be used. Unfortuantely, this won't format correctly in Asciidoc. The following squash-in fixes it... 8 Subject: [PATCH] fixup! send-email: provide whitelist of SMTP AUTH mechanisms --- Documentation/git-send-email.txt | 16 +--- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/Documentation/git-send-email.txt b/Documentation/git-send-email.txt index 82c6ae8..9e4f130 100644 --- a/Documentation/git-send-email.txt +++ b/Documentation/git-send-email.txt @@ -174,13 +174,15 @@ Sending --smtp-auth=mechs:: Whitespace-separated list of allowed SMTP-AUTH mechanisms. This setting forces using only the listed mechanisms. Example: - - $ git send-email --smtp-auth=PLAIN LOGIN GSSAPI ... - - If at least one of the specified mechanisms matches the ones advertised by the - SMTP server and if it is supported by the utilized SASL library, the mechanism - is used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth' - is specified, all mechanisms supported by the SASL library can be used. ++ +-- +$ git send-email --smtp-auth=PLAIN LOGIN GSSAPI ... +-- ++ +If at least one of the specified mechanisms matches the ones advertised by the +SMTP server and if it is supported by the utilized SASL library, the mechanism +is used for authentication. If neither 'sendemail.smtpAuth' nor '--smtp-auth' +is specified, all mechanisms supported by the SASL library can be used. --smtp-pass[=password]:: Password for SMTP-AUTH. The argument is optional: If no -- 2.5.0.276.gf5e568e -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 4/4] notes: teach git-notes about notes.ref.merge option
On Tue, Aug 11, 2015 at 10:57 PM, Jacob Keller jacob.e.kel...@intel.com wrote: From: Jacob Keller jacob.kel...@gmail.com Add new option notes.ref.merge option which specifies the merge strategy for merging into a given notes ref. This option enables selection of merge strategy for particular notes refs, rather than all notes ref merges, as user may not want cat_sort_uniq for all refs, but only some. Note that the ref is the local reference we are merging into, not the remote ref we merged from. The assumption is that users will mostly want to configure separate local ref merge strategies rather than strategies depending on which remote ref they merge from. Also, notes.ref.merge overrides the general behavior as it is more specific. Thanks for working on this, and I apologize for not properly reviewing this part of the series before now. More comments below. Signed-off-by: Jacob Keller jacob.kel...@gmail.com --- Documentation/config.txt| 6 ++ Documentation/git-notes.txt | 6 ++ builtin/notes.c | 7 +-- t/t3309-notes-merge-auto-resolve.sh | 39 + 4 files changed, 56 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index 488c2e8eec1b..2c283ebc309e 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1912,6 +1912,12 @@ notes.merge:: STRATEGIES section of linkgit:git-notes[1] for more information on each strategy. +notes.localref.merge:: + Which merge strategy to choose if the local ref for a notes merge + matches localref, overriding notes.merge. localref just be a s/just/must/ + fully qualified refname. See NOTES MERGE STRATEGIES section in + linkgit:git-notes[1] for more information on the available strategies. I sort of get the reason for fully qualified refname, but I think notes.refs/notes/commits.merge looks much uglier than notes.commits.merge Especially since we have the opposite precedence for branch.name.*, e.g. we already have branch.master.merge and not branch.refs/heads/master.merge I know that we don't yet have a proper place to put remote notes refs, but the ref in notes.ref.merge _must_ be a local notes ref (you even use the localref notation in the documentation below). Thus, I believe we can presume that the local notes ref must live under refs/notes/*, and hence drop the refs/notes/ prefix from the config key (just like branch.name.* can presume that name lives under refs/heads/*). ... Which brings me to another small gripe about the naming here: branch.name.merge names the remote ref (at branch.name.remote) that we will pull from, but notes.ref.merge has a very different meaning. If we - in the future - were to provide a similar config mechanism for a hypothetical git notes pull command, then it would be natural to model its config similarly: notes.name.remote and notes.name.merge specifies whence we fetch, and what we (notes-)merge, respectively. Except that this patch in its current form will occupy the .merge config key... Can you rename to notes.name.mergestrategy instead? Or even better, take inspiration from branch.name.mergeoptions, and provide notes.name.mergeoptions instead, which you can then set like: git config notes.foo.mergeoptions --strategy=cat_sort_uniq Even though 'git notes merge' don't yet accept any other options that should be configurable, I think it's worth emulating the mechanisms that alread exist for branches. It gives is some amount of future- proofing as well. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
