* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Johannes Sixt @ 2016-12-18 15:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, Pranit Bauva
In-Reply-To: <ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org>
Am 18.12.2016 um 16:26 schrieb Johannes Sixt:
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
But quite frankly, I find the implementation of winansi_isatty()
very unsatisfactory.
I understand that you wanted to be defensive and to override the
decision made by MSVCRT only when necessary.
However!
winansi.c is all about overriding MSVCRT's console handling. If we are
connected to a console, then by the time isatty() is called (from
outside the emulation layer), all handling of file descriptors 1 and 2
is already outside MSVCRT's control. In particular, we have determined
unambiguously whether a terminal is connected (see is_console()). I
suggest to have the implementation below (on top of the patch I'm
responding to).
What do you think?
diff --git a/compat/winansi.c b/compat/winansi.c
index ba360be69b..1748d17777 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -575,9 +575,8 @@ static void detect_msys_tty(int fd)
int winansi_isatty(int fd)
{
- int res = isatty(fd);
-
- if (res) {
+ switch (fd) {
+ case 0:
/*
* Make sure that /dev/null is not fooling Git into believing
* that we are connected to a terminal, as "_isatty() returns a
@@ -586,21 +585,19 @@ int winansi_isatty(int fd)
*
* https://msdn.microsoft.com/en-us/library/f4s0ddew.aspx
*/
- HANDLE handle = winansi_get_osfhandle(fd);
- if (fd == STDIN_FILENO) {
+ {
+ HANDLE handle = (HANDLE)_get_osfhandle(fd);
DWORD dummy;
- if (!GetConsoleMode(handle, &dummy))
- res = 0;
- } else if (fd == STDOUT_FILENO || fd == STDERR_FILENO) {
- CONSOLE_SCREEN_BUFFER_INFO dummy;
-
- if (!GetConsoleScreenBufferInfo(handle, &dummy))
- res = 0;
+ return !!GetConsoleMode(handle, &dummy);
}
+ case 1:
+ return !!hconsole1;
+ case 2:
+ return !!hconsole2;
}
- return res;
+ return isatty(fd);
}
void winansi_init(void)
--
2.11.0.79.gf6b77ca
^ permalink raw reply related
* [PATCH v1] git-p4: fix git-p4.pathEncoding for removed files
From: larsxschneider @ 2016-12-18 17:51 UTC (permalink / raw)
To: git; +Cc: luke, gitster, Lars Schneider
From: Lars Schneider <larsxschneider@gmail.com>
In a9e38359e3 we taught git-p4 a way to re-encode path names from what
was used in Perforce to UTF-8. This path re-encoding worked properly for
"added" paths. "Removed" paths were not re-encoded and therefore
different from the "added" paths. Consequently, these files were not
removed in a git-p4 cloned Git repository because the path names did not
match.
Fix this by moving the re-encoding to a place that affects "added" and
"removed" paths. Add a test to demonstrate the issue.
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
Notes:
Base Commit: d1271bddd4 (v2.11.0)
Diff on Web: https://github.com/git/git/compare/d1271bddd4...larsxschneider:05a82caa69
Checkout: git fetch https://github.com/larsxschneider/git git-p4/fix-path-encoding-v1 && git checkout 05a82caa69
git-p4.py | 19 +++++++++----------
t/t9822-git-p4-path-encoding.sh | 16 ++++++++++++++++
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..8f311cb4e8 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -2366,6 +2366,15 @@ class P4Sync(Command, P4UserMap):
break
path = wildcard_decode(path)
+ try:
+ path.decode('ascii')
+ except:
+ encoding = 'utf8'
+ if gitConfig('git-p4.pathEncoding'):
+ encoding = gitConfig('git-p4.pathEncoding')
+ path = path.decode(encoding, 'replace').encode('utf8', 'replace')
+ if self.verbose:
+ print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, path)
return path
def splitFilesIntoBranches(self, commit):
@@ -2495,16 +2504,6 @@ class P4Sync(Command, P4UserMap):
text = regexp.sub(r'$\1$', text)
contents = [ text ]
- try:
- relPath.decode('ascii')
- except:
- encoding = 'utf8'
- if gitConfig('git-p4.pathEncoding'):
- encoding = gitConfig('git-p4.pathEncoding')
- relPath = relPath.decode(encoding, 'replace').encode('utf8', 'replace')
- if self.verbose:
- print 'Path with non-ASCII characters detected. Used %s to encode: %s ' % (encoding, relPath)
-
if self.largeFileSystem:
(git_mode, contents) = self.largeFileSystem.processContent(git_mode, relPath, contents)
diff --git a/t/t9822-git-p4-path-encoding.sh b/t/t9822-git-p4-path-encoding.sh
index 7b83e696a9..c78477c19b 100755
--- a/t/t9822-git-p4-path-encoding.sh
+++ b/t/t9822-git-p4-path-encoding.sh
@@ -51,6 +51,22 @@ test_expect_success 'Clone repo containing iso8859-1 encoded paths with git-p4.p
)
'
+test_expect_success 'Delete iso8859-1 encoded paths and clone' '
+ (
+ cd "$cli" &&
+ ISO8859="$(printf "$ISO8859_ESCAPED")" &&
+ p4 delete "$ISO8859" &&
+ p4 submit -d "remove file"
+ ) &&
+ git p4 clone --destination="$git" //depot@all &&
+ test_when_finished cleanup_git &&
+ (
+ cd "$git" &&
+ git -c core.quotepath=false ls-files >actual &&
+ test_must_be_empty actual
+ )
+'
+
test_expect_success 'kill p4d' '
kill_p4d
'
--
2.11.0
^ permalink raw reply related
* [PATCH v1] git-p4: add diff/merge properties to .gitattributes for GitLFS files
From: larsxschneider @ 2016-12-18 19:01 UTC (permalink / raw)
To: git; +Cc: luke, gitster, Lars Schneider
From: Lars Schneider <larsxschneider@gmail.com>
The `git lfs track` command generates a .gitattributes file with diff
and merge properties [1]. Set the same .gitattributes format for files
tracked with GitLFS in git-p4.
[1] https://github.com/git-lfs/git-lfs/blob/v1.5.3/commands/command_track.go#L121
Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
---
Notes:
Base Commit: d1271bddd4 (v2.11.0)
Diff on Web: https://github.com/git/git/compare/d1271bddd4...larsxschneider:e045b3d5c8
Checkout: git fetch https://github.com/larsxschneider/git git-p4/fix-lfs-attributes-v1 && git checkout e045b3d5c8
git-p4.py | 4 ++--
t/t9824-git-p4-git-lfs.sh | 24 ++++++++++++------------
2 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..87b6932c81 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1098,10 +1098,10 @@ class GitLFS(LargeFileSystem):
'# Git LFS (see https://git-lfs.github.com/)\n',
'#\n',
] +
- ['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
+ ['*.' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs merge=lfs -text\n'
for f in sorted(gitConfigList('git-p4.largeFileExtensions'))
] +
- ['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs -text\n'
+ ['/' + f.replace(' ', '[[:space:]]') + ' filter=lfs diff=lfs merge=lfs -text\n'
for f in sorted(self.largeFiles) if not self.hasLargeFileExtension(f)
]
)
diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
index 110a7e7924..1379db6357 100755
--- a/t/t9824-git-p4-git-lfs.sh
+++ b/t/t9824-git-p4-git-lfs.sh
@@ -81,9 +81,9 @@ test_expect_success 'Store files in LFS based on size (>24 bytes)' '
#
# Git LFS (see https://git-lfs.github.com/)
#
- /file2.dat filter=lfs -text
- /file4.bin filter=lfs -text
- /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
+ /file2.dat filter=lfs diff=lfs merge=lfs -text
+ /file4.bin filter=lfs diff=lfs merge=lfs -text
+ /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
@@ -109,7 +109,7 @@ test_expect_success 'Store files in LFS based on size (>25 bytes)' '
#
# Git LFS (see https://git-lfs.github.com/)
#
- /file4.bin filter=lfs -text
+ /file4.bin filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
@@ -135,7 +135,7 @@ test_expect_success 'Store files in LFS based on extension (dat)' '
#
# Git LFS (see https://git-lfs.github.com/)
#
- *.dat filter=lfs -text
+ *.dat filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
@@ -163,8 +163,8 @@ test_expect_success 'Store files in LFS based on size (>25 bytes) and extension
#
# Git LFS (see https://git-lfs.github.com/)
#
- *.dat filter=lfs -text
- /file4.bin filter=lfs -text
+ *.dat filter=lfs diff=lfs merge=lfs -text
+ /file4.bin filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
@@ -199,8 +199,8 @@ test_expect_success 'Remove file from repo and store files in LFS based on size
#
# Git LFS (see https://git-lfs.github.com/)
#
- /file2.dat filter=lfs -text
- /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
+ /file2.dat filter=lfs diff=lfs merge=lfs -text
+ /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
@@ -237,8 +237,8 @@ test_expect_success 'Add .gitattributes and store files in LFS based on size (>2
#
# Git LFS (see https://git-lfs.github.com/)
#
- /file2.dat filter=lfs -text
- /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs -text
+ /file2.dat filter=lfs diff=lfs merge=lfs -text
+ /path[[:space:]]with[[:space:]]spaces/file3.bin filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
@@ -278,7 +278,7 @@ test_expect_success 'Add big files to repo and store files in LFS based on compr
#
# Git LFS (see https://git-lfs.github.com/)
#
- /file6.bin filter=lfs -text
+ /file6.bin filter=lfs diff=lfs merge=lfs -text
EOF
test_path_is_file .gitattributes &&
test_cmp expect .gitattributes
--
2.11.0
^ permalink raw reply related
* Bug report: $program_name in error message
From: Josh Bleecher Snyder @ 2016-12-18 20:36 UTC (permalink / raw)
To: git
To reproduce, run 'git submodule' from within a bare repo. Result:
$ git submodule
fatal: $program_name cannot be used without a working tree.
Looks like the intent was for $program_name to be interpolated.
As an aside, I sent a message a few days ago about a segfault when
working with a filesystem with direct_io on, but it appears not to
have made it to the archives on marc.info. Am I perhaps still
greylisted?
Thanks,
Josh
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Stefan Beller @ 2016-12-18 21:34 UTC (permalink / raw)
To: Josh Bleecher Snyder; +Cc: git@vger.kernel.org
In-Reply-To: <CAFAcib8yauNRB6UbO8qS2_Ff4fDt-7mMsmgop8d1V0j=RoTBSA@mail.gmail.com>
On Sun, Dec 18, 2016 at 12:36 PM, Josh Bleecher Snyder
<josharian@gmail.com> wrote:
> To reproduce, run 'git submodule' from within a bare repo. Result:
>
> $ git submodule
> fatal: $program_name cannot be used without a working tree.
>
> Looks like the intent was for $program_name to be interpolated.
Which version of git do you use?
>
>
> As an aside, I sent a message a few days ago about a segfault when
> working with a filesystem with direct_io on, but it appears not to
> have made it to the archives on marc.info. Am I perhaps still
> greylisted?
Both emails show up in my mailbox (subscribed to the mailing list),
so I think that just nobody answered your first email as the answer
may be non trivial.
>
> Thanks,
> Josh
^ permalink raw reply
* Re: Bug report: $program_name in error message
From: Josh Bleecher Snyder @ 2016-12-18 21:44 UTC (permalink / raw)
To: Stefan Beller; +Cc: git@vger.kernel.org
In-Reply-To: <CAGZ79kZ=QK5s0_94+4GNs3M5oo49GLm-KkT5K=yZktxX8C4UCw@mail.gmail.com>
>> To reproduce, run 'git submodule' from within a bare repo. Result:
>>
>> $ git submodule
>> fatal: $program_name cannot be used without a working tree.
>>
>> Looks like the intent was for $program_name to be interpolated.
>
> Which version of git do you use?
$ git version
git version 2.11.0
>> As an aside, I sent a message a few days ago about a segfault when
>> working with a filesystem with direct_io on, but it appears not to
>> have made it to the archives on marc.info. Am I perhaps still
>> greylisted?
>
> Both emails show up in my mailbox (subscribed to the mailing list),
> so I think that just nobody answered your first email as the answer
> may be non trivial.
Thanks for the confirmation.
-josh
^ permalink raw reply
* Re: Suggestion for the "Did you mean this?" feature
From: Chris Packham @ 2016-12-19 0:48 UTC (permalink / raw)
To: Kaartic Sivaraam; +Cc: GIT
In-Reply-To: <1482063500.10858.1.camel@gmail.com>
On Mon, Dec 19, 2016 at 1:18 AM, Kaartic Sivaraam
<kaarticsivaraam91196@gmail.com> wrote:
> Hello all,
>
> I have found the "Did you mean this?" feature of git as a very good
> feature. I thought it would be even better if it took a step toward by
> asking for a prompt when there was only one alternative to the command
> that was entered.
>
> E.g.
>
>> unique@unique-pc:~$ git hepl
>> git: 'hepl' is not a git command. See 'git --help'.
>>
>> Did you mean this?
>> help
>> [yes/No] : y
>> usage: git [--version] [--help] [-C <path>] [-c name=value]
>> [--exec-path[=<path>]] [--html-path] [--man-path] [--info-
>> path]
>> ....
>
> This would make it even better for the user as it would avoid having to
> correct the mistake long commands that had only a single error
> (considering history feature is enabled).
>
> Is this is a good idea ?
This feature already exists (although it's not interactive). See
help.autoCorrect in the git-config man page. "git config
help.autoCorrect -1" should to the trick.
^ permalink raw reply
* Re: [PATCH] winansi_isatty(): fix when Git is used from CMD
From: Junio C Hamano @ 2016-12-19 1:12 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Pranit Bauva
In-Reply-To: <ffc6a7a0-4ae4-b755-0b09-5bcd7114a2e6@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> ..
> The new isatty() override implemented by cbb3f3c9b197 (mingw: intercept
> isatty() to handle /dev/null as Git expects it, 2016-12-11) does not
> take into account that _get_osfhandle() returns the handle visible by
> the C code, which is the pipe. But it actually wants to investigate the
> properties of the handle that is actually connected to the outside
> world. Fortunately, there is already winansi_get_osfhandle(), which
> returns exactly this handle. Use it.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> I was able to test the idea earlier than anticipated and it does work
> for me.
I'll queue this in 'pu' so that I won't forget, but this time I'll
make sure I won't act on it until I see you two agree on the right
way forward.
Thanks.
^ permalink raw reply
* [PATCH] fast-import: properly fanout notes when tree is imported
From: Mike Hommey @ 2016-12-19 2:12 UTC (permalink / raw)
To: git; +Cc: johan, gitster
In typical uses of fast-import, trees are inherited from a parent
commit. In that case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = <tree structure loaded from $some_sha1>
However, when trees are imported, rather than inherited, that is not the
case. One can import a tree with a filemodify command, replacing the
root tree object.
e.g.
"M 040000 $some_sha1 \n"
In this case, the tree_entry for the branch looks like:
.versions[1].sha1 = $some_sha1
.tree = NULL
When adding new notes with the notemodify command, do_change_note_fanout
is called to get a notes count, and to do so, it loops over the
tree_entry->tree, but doesn't do anything when the tree is NULL.
In the latter case above, it means do_change_note_fanout thinks the tree
contains no note, and new notes are added with no fanout.
Interestingly, do_change_note_fanout does check whether subdirectories
have a NULL .tree, in which case it uses load_tree(). Which means the
right behaviour happens when using the filemodify command to import
subdirectories.
This change makes do_change_note_fanount call load_tree() whenever the
tree_entry it is given has no tree loaded, making all cases handled
equally.
Signed-off-by: Mike Hommey <mh@glandium.org>
---
This is something I should have submitted a patch for a long time ago, back
when this was discussed in the thread starting from
https://www.spinics.net/lists/git/msg242426.html. The message most relevant to
this patch in the thread is https://www.spinics.net/lists/git/msg242448.html.
fast-import.c | 8 +++++---
t/t9301-fast-import-notes.sh | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 3 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index cb545d7df5..5e528b1999 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(
char *fullpath, unsigned int fullpath_len,
unsigned char fanout)
{
- struct tree_content *t = root->tree;
+ struct tree_content *t;
struct tree_entry *e, leaf;
unsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;
uintmax_t num_notes = 0;
unsigned char sha1[20];
char realpath[60];
+ if (!root->tree)
+ load_tree(root);
+ t = root->tree;
+
for (i = 0; t && i < t->entry_count; i++) {
e = t->entries[i];
tmp_hex_sha1_len = hex_sha1_len + e->name->str_len;
@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(
leaf.tree);
} else if (S_ISDIR(e->versions[1].mode)) {
/* This is a subdir that may contain note entries */
- if (!e->tree)
- load_tree(e);
num_notes += do_change_note_fanout(orig_root, e,
hex_sha1, tmp_hex_sha1_len,
fullpath, tmp_fullpath_len, fanout);
diff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh
index 83acf68bc3..b408b2b32d 100755
--- a/t/t9301-fast-import-notes.sh
+++ b/t/t9301-fast-import-notes.sh
@@ -483,6 +483,47 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '
'
+# Create another notes tree from the one above
+cat >>input <<INPUT_END
+commit refs/heads/other_commits
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+commit #$(($num_commit + 1))
+COMMIT
+
+from refs/heads/many_commits
+M 644 inline file
+data <<EOF
+file contents in commit #$(($num_commit + 1))
+EOF
+
+commit refs/notes/other_notes
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+committing one more note on a tree imported from a previous notes tree
+COMMIT
+
+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)
+N inline :$(($num_commit + 1))
+data <<EOF
+note for commit #$(($num_commit + 1))
+EOF
+INPUT_END
+
+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '
+ git fast-import <input &&
+
+ # None of the entries in the top-level notes tree should be a full SHA1
+ git ls-tree --name-only refs/notes/other_notes |
+ while read path
+ do
+ if test $(expr length "$path") -ge 40
+ then
+ return 1
+ fi
+ done
+'
+
cat >>expect_non-note1 << EOF
This is not a note, but rather a regular file residing in a notes tree
EOF
--
2.11.0.dirty
^ permalink raw reply related
* Re: [PATCHv8 6/6] submodule: add absorb-git-dir function
From: Duy Nguyen @ 2016-12-19 5:35 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, git, bmwill
In-Reply-To: <20161212190435.10358-7-sbeller@google.com>
On Mon, Dec 12, 2016 at 11:04:35AM -0800, Stefan Beller wrote:
> diff --git a/dir.c b/dir.c
> index e0efd3c2c3..d872cc1570 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -2773,3 +2773,15 @@ void connect_work_tree_and_git_dir(const char *work_tree_, const char *git_dir_)
> free(work_tree);
> free(git_dir);
> }
> +
> +/*
> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.
> + */
> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)
> +{
> + if (rename(old_git_dir, new_git_dir) < 0)
> + die_errno(_("could not migrate git directory from '%s' to '%s'"),
> + old_git_dir, new_git_dir);
> +
> + connect_work_tree_and_git_dir(path, new_git_dir);
Should we worry about recovering (e.g. maybe move new_git_dir back to
old_git_dir) if this connect_work_tree_and_git_dir() fails?
Both write_file() and git_config_set_.. in this function may die(). In
such a case the repo is in broken state and the user needs pretty good
submodule understanding to recover from it, I think.
Recovering is not easy (nor entirely safe) either, though I suppose if
we keep original copies for modified files, then we could restore them
after moving the directory back and pray the UNIX gods that all
operations succeed.
--
Duy
^ permalink raw reply
* Re: [PATCH 1/5] check-ref-format: Refactor out check_one_ref_format
From: Michael Haggerty @ 2016-12-19 8:27 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-2-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> We are going to want to reuse this. No functional change right now.
>
> It currently has a hidden memory leak if --normalize is used.
>
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> builtin/check-ref-format.c | 26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index eac4994..4d56caa 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -48,12 +48,22 @@ static int check_ref_format_branch(const char *arg)
> return 0;
> }
>
> +static int normalize = 0;
> +static int flags = 0;
> +
> +static int check_one_ref_format(const char *refname)
> +{
> + if (normalize)
> + refname = collapse_slashes(refname);
> + if (check_refname_format(refname, flags))
> + return 1;
> + if (normalize)
> + printf("%s\n", refname);
This function needs to `return 0` if it gets to the end.
> +}
> +
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> {
> int i;
> - int normalize = 0;
> - int flags = 0;
> - const char *refname;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
> @@ -76,13 +86,5 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (! (i == argc - 1))
> usage(builtin_check_ref_format_usage);
>
> - refname = argv[i];
> - if (normalize)
> - refname = collapse_slashes(refname);
> - if (check_refname_format(refname, flags))
> - return 1;
> - if (normalize)
> - printf("%s\n", refname);
> -
> - return 0;
> + return check_one_ref_format(argv[i]);
> }
>
Michael
^ permalink raw reply
* [PATCH] config.c: handle error case for fstat() calls
From: Nguyễn Thái Ngọc Duy @ 2016-12-19 9:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, josharian, Nguyễn Thái Ngọc Duy
In-Reply-To: <CAFAcib_cY8FeLFkW1=MfR+P7xoupGK9DFegNY5boExHSRppAmg@mail.gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
Will this fix the problem I'm replying to? I don't know. I found this
while checking the code and it should be fixed regardless.
config.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/config.c b/config.c
index 83fdecb..4973256 100644
--- a/config.c
+++ b/config.c
@@ -2194,7 +2194,12 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
goto out_free;
}
- fstat(in_fd, &st);
+ if (fstat(in_fd, &st) == -1) {
+ error_errno(_("fstat on %s failed"), config_filename);
+ ret = CONFIG_INVALID_FILE;
+ goto out_free;
+ }
+
contents_sz = xsize_t(st.st_size);
contents = xmmap_gently(NULL, contents_sz, PROT_READ,
MAP_PRIVATE, in_fd, 0);
@@ -2414,7 +2419,10 @@ int git_config_rename_section_in_file(const char *config_filename,
goto unlock_and_out;
}
- fstat(fileno(config_file), &st);
+ if (fstat(fileno(config_file), &st) == -1) {
+ ret = error_errno(_("fstat on %s failed"), config_filename);
+ goto out;
+ }
if (chmod(get_lock_file_path(lock), st.st_mode & 07777) < 0) {
ret = error_errno("chmod on %s failed",
--
2.8.2.524.g6ff3d78
^ permalink raw reply related
* Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common
From: Michael Haggerty @ 2016-12-19 11:07 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-3-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> We are going to want to permit other options with --branch.
>
> So, replace the special case with just an entry for --branch in the
> parser for ordinary options, and check for option compatibility at the
> end.
>
> No overall functional change.
>
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> builtin/check-ref-format.c | 17 +++++++++++++----
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 4d56caa..f12c19c 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -49,13 +49,19 @@ static int check_ref_format_branch(const char *arg)
> }
>
> static int normalize = 0;
> +static int check_branch = 0;
> static int flags = 0;
>
> static int check_one_ref_format(const char *refname)
> {
> + int got;
`got` is an unusual name for this variable, and I don't really
understand what the word means in this context. Is there a reason not to
use the more usual `err`?
> +
> if (normalize)
> refname = collapse_slashes(refname);
> - if (check_refname_format(refname, flags))
> + got = check_branch
> + ? check_ref_format_branch(refname)
> + : check_refname_format(refname, flags);
> + if (got)
> return 1;
> if (normalize)
> printf("%s\n", refname);
> @@ -68,9 +74,6 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
>
> - if (argc == 3 && !strcmp(argv[1], "--branch"))
> - return check_ref_format_branch(argv[2]);
> -
> for (i = 1; i < argc && argv[i][0] == '-'; i++) {
> if (!strcmp(argv[i], "--normalize") || !strcmp(argv[i], "--print"))
> normalize = 1;
> @@ -80,9 +83,15 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> flags &= ~REFNAME_ALLOW_ONELEVEL;
> else if (!strcmp(argv[i], "--refspec-pattern"))
> flags |= REFNAME_REFSPEC_PATTERN;
> + else if (!strcmp(argv[i], "--branch"))
> + check_branch = 1;
> else
> usage(builtin_check_ref_format_usage);
> }
> +
> + if (check_branch && (flags || normalize))
Is there a reason not to allow `--normalize` with `--branch`?
(Currently, `git check-ref-format --branch` *does* allow input like
`refs/heads/foo`.)
But note that simply allowing `--branch --normalize` without changing
`check_one_ref_format()` would mean generating *two* lines of output per
reference, so something else would have to change, too.
> + usage(builtin_check_ref_format_usage);
> +
> if (! (i == argc - 1))
> usage(builtin_check_ref_format_usage);
>
>
Michael
^ permalink raw reply
* Re: [PATCH 3/5] check-ref-format: Abolish leak of collapsed refname
From: Michael Haggerty @ 2016-12-19 11:09 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-4-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> collapse_slashes always returns a value from xmallocz.
>
> Right now this leak is not very interesting, since we only call
> check_one_ref_format once.
>
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> builtin/check-ref-format.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index f12c19c..020ebe8 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -63,8 +63,10 @@ static int check_one_ref_format(const char *refname)
> : check_refname_format(refname, flags);
> if (got)
> return 1;
If the function returns via the line above, then the memory will still
be leaked.
> - if (normalize)
> + if (normalize) {
> printf("%s\n", refname);
> + free((void*)refname);
> + }
> }
>
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
>
Michael
^ permalink raw reply
* Re: [PATCH v2 02/21] config: add git_config_get_split_index()
From: Duy Nguyen @ 2016-12-19 11:14 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-3-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:28PM +0100, Christian Couder wrote:
> diff --git a/config.c b/config.c
> index 2eaf8ad77a..c1343bbb3e 100644
> --- a/config.c
> +++ b/config.c
> @@ -1709,6 +1709,16 @@ int git_config_get_untracked_cache(void)
> return -1; /* default value */
> }
>
> +int git_config_get_split_index(void)
> +{
> + int val = -1;
Is it redundant to set default value here because it's not used
anywhere? The "return val;" will always have the new value from
git_config_. And you don't use "val" in error case.
> +
> + if (!git_config_get_maybe_bool("core.splitindex", &val))
> + return val;
> +
> + return -1; /* default value */
> +}
> +
> NORETURN
> void git_die_config_linenr(const char *key, const char *filename, int linenr)
> {
> --
> 2.11.0.49.g2414764.dirty
>
^ permalink raw reply
* Re: [PATCH v2 04/21] read-cache: add and then use tweak_split_index()
From: Duy Nguyen @ 2016-12-19 11:18 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-5-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:30PM +0100, Christian Couder wrote:
> +static void tweak_split_index(struct index_state *istate)
> +{
> + switch (git_config_get_split_index()) {
> + case -1: /* unset: do nothing */
> + break;
> + case 0: /* false */
> + remove_split_index(istate);
> + break;
> + case 1: /* true */
> + add_split_index(istate);
> + break;
> + default: /* unknown value: do nothing */
Probably should die("BUG:") here since it looks to me like
git_config_maybe_bool() (inside git_config_get_split_index) in this
case has been updated to return more values than we can handle. And we
need to know about that so we can handle it properly.
> + break;
> + }
> +}
^ permalink raw reply
* Re: What's cooking in git.git (Dec 2016, #02; Mon, 12)
From: Johannes Schindelin @ 2016-12-19 10:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqinqn6c41.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> While I think it would make it easier for people to experiment and
> >> build on if the topic is merged to 'next', I am at the same time a
> >> bit reluctant to merge an unproven new topic that introduces a new
> >> file format, which we may end up having to support til the end of
> >> time. It is likely that to support a "prime clone from CDN", it
> >> would need a lot more than just "these are the heads and the pack
> >> data is over there", so this may not be sufficient.
> >>
> >> Will discard.
> >
> > You could mark it as experimental, subject to change, and merge it to
> > `next` safely.
>
> Are you planning, or do you know somebody who plans to use that code
> soonish?
I am too swamped with other things (most importantly, automate the
identification of the as-of-recent-quite-frequent breakages reported by my
build jobs).
I know that one of my colleagues wanted to have a look at it, and so I
thought that having it as an experimental feature that I could even
integrate into Git for Windows for a wider audience could help justify
alotting the time.
> Otherwise I'd prefer to drop it---at this point, the series is merely
> "just because we can", not "because we need it to further improve this
> or that".
Oh, I thought that this was meant as a starting point for anybody
interested in playing with resumable clones or with easing server loads.
In any case, I just wanted to be sure that you considered making it an
experimental feature instead of dropping it. Just in case that you did not
think of that as a possibility.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 5/5] check-ref-format: New --stdin option
From: Michael Haggerty @ 2016-12-19 11:22 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-6-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
> ---
> Documentation/git-check-ref-format.txt | 10 ++++++++--
> builtin/check-ref-format.c | 34 +++++++++++++++++++++++++++++++---
> 2 files changed, 39 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-check-ref-format.txt b/Documentation/git-check-ref-format.txt
> index e9a2657..5a213ce 100644
> --- a/Documentation/git-check-ref-format.txt
> +++ b/Documentation/git-check-ref-format.txt
> @@ -10,8 +10,9 @@ SYNOPSIS
> [verse]
> 'git check-ref-format' [--report-errors] [--normalize]
> [--[no-]allow-onelevel] [--refspec-pattern]
> - <refname>
> -'git check-ref-format' [--report-errors] --branch <branchname-shorthand>
> + <refname> | --stdin
> +'git check-ref-format' [--report-errors] --branch
> + <branchname-shorthand> | --stdin
>
> DESCRIPTION
> -----------
> @@ -109,6 +110,11 @@ OPTIONS
> If any ref does not check OK, print a message to stderr.
> (By default, git check-ref-format is silent.)
>
> +--stdin::
> + Instead of checking on ref supplied on the command line,
> + read refs, one per line, from stdin. The exit status is
> + 0 if all the refs were OK.
> +
>
> EXAMPLES
> --------
> diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
> index 559d5c2..87f52fa 100644
> --- a/builtin/check-ref-format.c
> +++ b/builtin/check-ref-format.c
> @@ -76,6 +76,7 @@ static int check_one_ref_format(const char *refname)
> int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> {
> int i;
> + int use_stdin = 0;
>
> if (argc == 2 && !strcmp(argv[1], "-h"))
> usage(builtin_check_ref_format_usage);
> @@ -93,6 +94,8 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> check_branch = 1;
> else if (!strcmp(argv[i], "--report-errors"))
> report_errors = 1;
> + else if (!strcmp(argv[i], "--stdin"))
> + use_stdin = 1;
> else
> usage(builtin_check_ref_format_usage);
> }
> @@ -100,8 +103,33 @@ int cmd_check_ref_format(int argc, const char **argv, const char *prefix)
> if (check_branch && (flags || normalize))
> usage(builtin_check_ref_format_usage);
>
> - if (! (i == argc - 1))
> - usage(builtin_check_ref_format_usage);
> + if (!use_stdin) {
> + if (! (i == argc - 1))
> + usage(builtin_check_ref_format_usage);
Given the changes that you made to support `--stdin`, it would be pretty
easy to support multiple command line arguments, now, too. (But this
needn't be part of your patch series.)
> +
> + return check_one_ref_format(argv[i]);
> + } else {
> + char buffer[2048];
> + int worst = 0;
>
> - return check_one_ref_format(argv[i]);
> + if (! (i == argc))
> + usage(builtin_check_ref_format_usage);
> +
> + while (fgets(buffer, sizeof(buffer), stdin)) {
`strbuf_getline()` would make this a lot easier and also eliminate the
need to specify a buffer size.
> + char *newline = strchr(buffer, '\n');
> + if (!newline) {
> + fprintf(stderr, "%s --stdin: missing final newline or line too long\n", *argv);
> + exit(127);
> + }
> + *newline = 0;
> + int got = check_one_ref_format(buffer);
> + if (got > worst)
> + worst = got;
> + }
> + if (!feof(stdin)) {
> + perror("reading from stdin");
> + exit(127);
> + }
> + return worst;
> + }
> }
>
Michael
^ permalink raw reply
* Re: [PATCH 0/5] git check-ref-format --stdin --report-errors
From: Michael Haggerty @ 2016-12-19 11:29 UTC (permalink / raw)
To: Ian Jackson, git
In-Reply-To: <20161104191358.28812-1-ijackson@chiark.greenend.org.uk>
On 11/04/2016 08:13 PM, Ian Jackson wrote:
> I wanted to be able to syntax check lots of proposed refs quickly
> (please don't ask why - it's complicated!)
>
> So I added a --stdin option to git-check-ref-format. Also it has
> --report-errors now too so you can get some kind of useful error
> message if it complains.
>
> It's still not really a good batch mode but it's good enough for my
> use case. To improve it would involve a new command line option to
> offer a suitable stdout output format.
>
> There are three small refactoring patches and the two patches with new
> options and corresponding docs.
>
> Thanks for your attention.
>
> FYI I am not likely to need this again in the near future: it's a
> one-off use case. So my effort for rework is probably limited. I
> thought I'd share what I'd done in what I hope is a useful form,
> anyway.
Thanks for your patches. I left some comments about the individual patches.
I don't know whether this feature will be popular, but it's not a lot of
code to add it, so it would be OK with me.
Especially given that the output is not especially machine-readable, it
might be more consistent with other commands to call the new feature
`--verbose` rather than `--report-errors`.
If it is thought likely that scripts will want to leave a pipe open to
this command and feed it one query at a time, then it would be helpful
to flush stdout after each reference's result is written. If the
opposite use case is common (mass processing of refnames), we could
always add a `--buffer` option like the one that `git cat-file --batch` has.
Michael
^ permalink raw reply
* Re: [PATCH v2 10/21] read-cache: regenerate shared index if necessary
From: Duy Nguyen @ 2016-12-19 11:48 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-11-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:36PM +0100, Christian Couder wrote:
> +static const int default_max_percent_split_change = 20;
> +
> +static int too_many_not_shared_entries(struct index_state *istate)
> +{
> + int i, not_shared = 0;
> + int max_split = git_config_get_max_percent_split_change();
> +
> + switch (max_split) {
> + case -1:
> + /* not or badly configured: use the default value */
> + max_split = default_max_percent_split_change;
> + break;
> + case 0:
> + return 1; /* 0% means always write a new shared index */
> + case 100:
> + return 0; /* 100% means never write a new shared index */
I wonder if we really need to special case these here. If I read it
correctly, the expression at the end of this function will return 1
when max_split is 0, and 0 when max_split is 100 (not counting the
case when cache_nr is zero).
Perhaps it's good for documentation purpose. Though I find it hard to
see a use case for max_split == 0. Always creating a new shared index
sounds crazy.
> + default:
> + ; /* do nothing: just use the configured value */
> + }
> +
> + /* Count not shared entries */
> + for (i = 0; i < istate->cache_nr; i++) {
> + struct cache_entry *ce = istate->cache[i];
> + if (!ce->index)
> + not_shared++;
> + }
> +
> + return istate->cache_nr * max_split < not_shared * 100;
> +}
^ permalink raw reply
* Re: [PATCH v2 16/21] read-cache: unlink old sharedindex files
From: Duy Nguyen @ 2016-12-19 11:58 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-17-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:42PM +0100, Christian Couder wrote:
> +static int clean_shared_index_files(const char *current_hex)
> +{
> + struct dirent *de;
> + DIR *dir = opendir(get_git_dir());
> +
> + if (!dir)
> + return error_errno(_("unable to open git dir: %s"), get_git_dir());
> +
> + while ((de = readdir(dir)) != NULL) {
> + const char *sha1_hex;
> + if (!skip_prefix(de->d_name, "sharedindex.", &sha1_hex))
> + continue;
> + if (!strcmp(sha1_hex, current_hex))
fspathcmp since we're dealing fs paths?
In theory we should be ok using strcmp, even on Windows because case
is preserved, I think. It's only a problem when we write path 'abc'
down and read 'ABC' back.
Oh well.. your call because if you go with fspathcmp, skip_prefix can't
be used either. A lot more changes for a very rare case.
> + continue;
> + if (can_delete_shared_index(sha1_hex) > 0 &&
Probably shorter to pass full d->name here so you don't have to do
another git_path() in can_delete_
> + unlink(git_path("%s", de->d_name)))
> + error_errno(_("unable to unlink: %s"), git_path("%s", de->d_name));
> + }
> + closedir(dir);
> +
> + return 0;
> +}
^ permalink raw reply
* Re: [PATCH v2 00/21] Add configuration options for split-index
From: Duy Nguyen @ 2016-12-19 12:02 UTC (permalink / raw)
To: Christian Couder
Cc: git, Junio C Hamano, Ævar Arnfjörð Bjarmason,
Christian Couder
In-Reply-To: <20161217145547.11748-1-chriscool@tuxfamily.org>
On Sat, Dec 17, 2016 at 03:55:26PM +0100, Christian Couder wrote:
> Goal
> ~~~~
>
> We want to make it possible to use the split-index feature
> automatically by just setting a new "core.splitIndex" configuration
> variable to true.
>
> This can be valuable as split-index can help significantly speed up
> `git rebase` especially along with the work to libify `git apply`
> that has been merged to master
> (see https://github.com/git/git/commit/81358dc238372793b1590efa149cc1581d1fbd98)
> and is now in v2.11.
I've read through the series (*) and I think it looks good, just a few
minor comments here and there.
(*) guiltily admit that I only skimmed through tests, not giving them
as much attention as I should have
--
Duy
^ permalink raw reply
* Re: [PATCH] parse-options: print "fatal:" before usage_msg_opt()
From: Duy Nguyen @ 2016-12-19 12:07 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161214151009.4wdzjb44f6aki5ug@sigill.intra.peff.net>
On Wed, Dec 14, 2016 at 10:10:10AM -0500, Jeff King wrote:
> diff --git a/parse-options.c b/parse-options.c
> index 312a85dbd..4fbe924a5 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -661,7 +661,7 @@ void NORETURN usage_msg_opt(const char *msg,
> const char * const *usagestr,
> const struct option *options)
> {
> - fprintf(stderr, "%s\n\n", msg);
> + fprintf(stderr, "fatal: %s\n\n", msg);
Your commit message does not make clear if you want this "fatal" to be
grep-able (by scripts) or not. If not, please _() the string. Maybe
this to reduce work for translators
/* TRANSLATORS: this is the prefix before usage error */
fprintf(stderr, "%s %s\n\n", _("fatal:"), msg);
> usage_with_options(usagestr, options);
> }
>
> --
> 2.11.0.341.g202cd3142
^ permalink raw reply
* Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common
From: Ian Jackson @ 2016-12-19 11:54 UTC (permalink / raw)
To: Michael Haggerty; +Cc: git
In-Reply-To: <e93ee78a-aa5e-27f6-9703-6efa385f487b@alum.mit.edu>
Michael Haggerty writes ("Re: [PATCH 2/5] check-ref-format: Refactor to make --branch code more common"):
> On 11/04/2016 08:13 PM, Ian Jackson wrote:
> > static int normalize = 0;
> > +static int check_branch = 0;
> > static int flags = 0;
> >
> > static int check_one_ref_format(const char *refname)
> > {
> > + int got;
>
> `got` is an unusual name for this variable, and I don't really
> understand what the word means in this context. Is there a reason not to
> use the more usual `err`?
I have no real opinion about the name of this variable. `err' is a
fine name too.
> > + if (check_branch && (flags || normalize))
>
> Is there a reason not to allow `--normalize` with `--branch`?
> (Currently, `git check-ref-format --branch` *does* allow input like
> `refs/heads/foo`.)
It was like that when I found it :-). I wasn't sure why this
restriction was there so I left it alone.
Looking at it again: AFAICT from the documentation --branch is a
completely different mode. The effect of --normalize is not to do
additional work, but simply to produce additional output. It really
means --print-normalized. --branch already prints output, but AFAICT
it does not collapse slashes. This seems like a confusing collection
of options. But, sorting that out is beyond the scope of what I was
trying to do.
In my series I have at least managed not to make any of this any
worse, I think: the --stdin option I introduce applies to both modes
equally, and doesn't make future improvements to the conflict between
--branch and --normalize any harder.
(In _this_ patch, certainly, allowing --normalize with --branch would
be wrong, since _this_ patch is just refactoring.)
Thanks,
Ian.
--
Ian Jackson <ijackson@chiark.greenend.org.uk> These opinions are my own.
If I emailed you from an address @fyvzl.net or @evade.org.uk, that is
a private address which bypasses my fierce spamfilter.
^ permalink raw reply
* Re: [PATCH v2 02/34] sequencer (rebase -i): implement the 'noop' command
From: Johannes Schindelin @ 2016-12-19 12:51 UTC (permalink / raw)
To: Junio C Hamano
Cc: Linus Torvalds, Git Mailing List, Kevin Daudt, Dennis Kaarsemaker
In-Reply-To: <xmqqtwa7ze9i.fsf@gitster.mtv.corp.google.com>
Hi,
On Tue, 13 Dec 2016, Junio C Hamano wrote:
> Linus Torvalds <torvalds@linux-foundation.org> writes:
>
> > On Tue, Dec 13, 2016 at 12:38 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> >>
> >>> +/*
> >>> + * Note that ordering matters in this enum. Not only must it match the mapping
> >>> + * below, it is also divided into several sections that matter. When adding
> >>> + * new commands, make sure you add it in the right section.
> >>> + */
> >>
> >> Good thinking.
Not my thinking... This was in direct response to a suggestion by Dennis
Kaarsemaker, I cannot take credit for the idea.
> Makes me wish C were a better language, though ;-)
> >
> > Do this:
> >
> > static const char *todo_command_strings[] = {
> > [TODO_PICK] = "pick",
> > [TODO_REVERT] = "revert",
> > [TODO_NOOP] = "noop:,
> > };
> >
> > which makes the array be order-independent. You still need to make
> > sure you fill in all the entries, of course, but it tends to avoid at
> > least one gotcha, and it makes it more obvious how the two are tied
> > together.
>
> Yes, I know. But I do not think the variant of C we stick to is not
> new enough to have that.
Let me try to express this without double negation: our coding guidelines
state very explicitly that we do not use C99 initializers, and it also
explains why: we want to support a broader range of compilers. For
details, see:
https://github.com/git/git/blob/v2.11.0/Documentation/CodingGuidelines#L179-L181
TBH I briefly considered going the same route as I did for the fsck
warnings (taking a lot of heat for the ugliness):
https://github.com/git/git/blob/v2.11.0/fsck.c#L17-L85
It would have looked somewhat like this:
#define FOREACH_TODO(FUNC) \
FUNC(PICK, 'p', "pick") \
FUNC(REVERT, 0, "revert") \
FUNC(EDIT, 'e', "edit") \
...
FUNC(COMMENT, 0, NULL)
#define TODO(id, short, long) TODO_##id,
enum todo_command {
FOREACH_TODO(TODO)
TODO_MAX
};
#undef TODO
#define TODO(id, short, long) { short, long },
static struct {
char c;
const char *str;
} todo_command_info[] = {
FOREACH_TODO(TODO)
{ 0, NULL }
};
#undef TODO
I have to admit that even I prefer the version I provided in my
contribution over the "fsck method" outlined above.
Ciao,
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox