From bf8b1e04ffa3bb6c64bb8ae50ec825b128ef957d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 4 Mar 2023 05:26:14 -0500 Subject: bundle: let "-" mean stdin for reading operations For writing, "bundle create -" indicates that the bundle should be written to stdout. But there's no matching handling of "-" for reading operations. This is inconsistent, and a little inflexible (though one can always use "/dev/stdin" on systems that support it). However, it's easy to change. Once upon a time, the bundle-reading code required a seekable descriptor, but that was fixed long ago in e9ee84cf28b (bundle: allowing to read from an unseekable fd, 2011-10-13). So we just need to handle "-" explicitly when opening the file. We _could_ do this by handling "-" in read_bundle_header(), which the reading functions all call already. But that is probably a bad idea. It's also used by low-level code like the transport functions, and we may want to be more careful there. We do not know that stdin is even available to us, and certainly we would not want to get confused by a configured URL that happens to point to "-". So instead, let's add a helper to builtin/bundle.c. Since both the bundle code and some of the callers refer to the bundle by name for error messages, let's use the string "" to make the output a bit nicer to read. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t6020-bundle-misc.sh | 15 +++++++++++++++ 1 file changed, 15 insertions(+) (limited to 't') diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh index 3a1cf30b1d..063e8ce6b4 100755 --- a/t/t6020-bundle-misc.sh +++ b/t/t6020-bundle-misc.sh @@ -566,4 +566,19 @@ test_expect_success 'cloning from filtered bundle has useful error' ' grep "cannot clone from filtered bundle" err ' +test_expect_success 'read bundle over stdin' ' + git bundle create some.bundle HEAD && + + git bundle verify - err && + grep " is okay" err && + + git bundle list-heads some.bundle >expect && + git bundle list-heads - actual && + test_cmp expect actual && + + git bundle unbundle some.bundle >expect && + git bundle unbundle - actual && + test_cmp expect actual +' + test_done -- cgit v1.2.3 From a8bfa99d443d3c461217db4924f8eca24caa055a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Sat, 4 Mar 2023 05:27:56 -0500 Subject: bundle: don't blindly apply prefix_filename() to "-" A user can specify a filename to a command from the command line, either as the value given to a command line option, or a command line argument. When it is given as a relative filename, in the user's mind, it is relative to the directory "git" was started from, but by the time the filename is used, "git" would almost always have chdir()'ed up to the root level of the working tree. The given filename, if it is relative, needs to be prefixed with the path to the current directory, and it typically is done by calling prefix_filename() helper function. For commands that can also take "-" to use the standard input or the standard output, however, this needs to be done with care. "git bundle create" uses the next word on the command line as the output filename, and can take "-" to mean "write to the standard output". It blindly called prefix_filename(), so running it in a subdirectory did not quite work as expected. Introduce a new helper, prefix_filename_except_for_dash(), and use it to help "git bundle create" codepath. Reported-by: Michael Henry Helped-by: Jeff King Signed-off-by: Junio C Hamano Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t6020-bundle-misc.sh | 11 +++++++++++ 1 file changed, 11 insertions(+) (limited to 't') diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh index 063e8ce6b4..7fc39660f1 100755 --- a/t/t6020-bundle-misc.sh +++ b/t/t6020-bundle-misc.sh @@ -581,4 +581,15 @@ test_expect_success 'read bundle over stdin' ' test_cmp expect actual ' +test_expect_success 'send a bundle to standard output' ' + git bundle create - --all HEAD >bundle-one && + mkdir -p down && + git -C down bundle create - --all HEAD >bundle-two && + git bundle verify bundle-one && + git bundle verify bundle-two && + git ls-remote bundle-one >expect && + git ls-remote bundle-two >actual && + test_cmp expect actual +' + test_done -- cgit v1.2.3 From 7ce4088ab78e06fc5c4ae42fc75b65a48bf7b3ff Mon Sep 17 00:00:00 2001 From: Jeff King Date: Sat, 4 Mar 2023 05:31:22 -0500 Subject: parse-options: consistently allocate memory in fix_filename() When handling OPT_FILENAME(), we have to stick the "prefix" (if any) in front of the filename to make up for the fact that Git has chdir()'d to the top of the repository. We can do this with prefix_filename(), but there are a few special cases we handle ourselves. Unfortunately the memory allocation is inconsistent here; if we do make it to prefix_filename(), we'll allocate a string which the caller must free to avoid a leak. But if we hit our special cases, we'll return the string as-is, and a caller which tries to free it will crash. So there's no way to win. Let's consistently allocate, so that callers can do the right thing. There are now three cases to care about in the function (and hence a three-armed if/else): 1. we got a NULL input (and should leave it as NULL, though arguably this is the sign of a bug; let's keep the status quo for now and we can pick at that scab later) 2. we hit a special case that means we leave the name intact; we should duplicate the string. This includes our special "-" matching. Prior to this patch, it also included empty prefixes and absolute filenames. But we can observe that prefix_filename() already handles these, so we don't need to detect them. 3. everything else goes to prefix_filename() I've dropped the "const" from the "char **file" parameter to indicate that we're allocating, though in practice it's not really important. This is all being shuffled through a void pointer via opt->value before it hits code which ever looks at the string. And it's even a bit weird, because we are really taking _in_ a const string and using the same out-parameter for a non-const string. A better function signature would be: static char *fix_filename(const char *prefix, const char *file); but that would mean the caller dereferences the double-pointer (and the NULL check is currently handled inside this function). So I took the path of least-change here. Note that we have to fix several callers in this commit, too, or we'll break the leak-checking tests. These are "new" leaks in the sense that they are now triggered by the test suite, but these spots have always been leaky when Git is run in a subdirectory of the repository. I fixed all of the cases that trigger with GIT_TEST_PASSING_SANITIZE_LEAK. There may be others in scripts that have other leaks, but we can fix them later along with those other leaks (and again, you _couldn't_ fix them before this patch, so this is the necessary first step). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/helper/test-parse-pathspec-file.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 't') diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c index b3e08cef4b..71d2131fba 100644 --- a/t/helper/test-parse-pathspec-file.c +++ b/t/helper/test-parse-pathspec-file.c @@ -6,7 +6,7 @@ int cmd__parse_pathspec_file(int argc, const char **argv) { struct pathspec pathspec; - const char *pathspec_from_file = NULL; + char *pathspec_from_file = NULL; int pathspec_file_nul = 0, i; static const char *const usage[] = { @@ -29,5 +29,6 @@ int cmd__parse_pathspec_file(int argc, const char **argv) printf("%s\n", pathspec.items[i].original); clear_pathspec(&pathspec); + free(pathspec_from_file); return 0; } -- cgit v1.2.3