ls-files: adding support for submodules
Allow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules.
---
Hey git developers!
I'm new to the community and this is the first patch for an open source project that I have worked on.
I'm looking forward to working on the project!
Brandon Williams
Documentation/git-ls-files.txt | 7 ++- builtin/ls-files.c | 58 +++++++++++++++++++ t/t3007-ls-files-recurse-submodules.sh | 103 +++++++++++++++++++++++++++++++++ 3 files changed, 167 insertions(+), 1 deletion(-) create mode 100644 t/t3007-ls-files-recurse-submodules.sh
Documentation/git-ls-files.txt +6 -1
@@ -18,7 +18,8 @@ SYNOPSIS [--exclude-per-directory=<file>] [--exclude-standard] [--error-unmatch] [--with-tree=<tree-ish>] - [--full-name] [--abbrev] [--] [<file>...] + [--full-name] [--recurse-submodules] + [--abbrev] [--] [<file>...] DESCRIPTION -----------
@@ -137,6 +138,10 @@ a space) at the start of each line: option forces paths to be output relative to the project top directory. +--recurse-submodules:: + Recursively calls ls-files on each submodule in the repository. + Currently there is only support for the --cached mode. + --abbrev[=<n>]:: Instead of showing the full 40-byte hexadecimal object lines, show only a partial prefix.
builtin/ls-files.c +58 -0
94 lines not on this page · raw message
t/t3007-ls-files-recurse-submodules.sh new file +103 -0
103 lines not on this page · raw message
Signature · 1 line
2.8.0.rc3.226.g39d4020
Quoted 4 lines · Brandon Williams
Allow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules.
Quoted 8 lines
Signed-off-by Brandon Williams <[email protected]>---
Hey git developers!I'm new to the community and this is the first patch for an open source project that I have worked on.
I'm looking forward to working on the project!
Welcome. :)
Submodules are not really my area of expertise, so I don't have any commentary on the goal of the patch, except that it sounds reasonable to my layman's ears.
The implementation looks fairly clean. A few comments:
Quoted 5 lines
builtin/ls-files.c +5
-0+static void show_gitlink (const struct cache_entry *ce) +{ +struct child_process cp =CHILD_PROCESS_INIT ; +struct strbuf buf =STRBUF_INIT ; +struct strbuf name =STRBUF_INIT ;
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 174
Quoted 10 lines
builtin/ls-files.c +10
-0+int submodule_name_len; +FILE *fp; + +argv_array_push (&cp.args,"ls-files" ); +argv_array_push (&cp.args,"--recurse-submodules" ); + cp.git_cmd =1 ; + cp.dir = ce->name; + cp.out =-1 ; +start_command (&cp); + fp =fdopen (cp.out,"r" );
You should error-check the result of start_command(). I guess the reasonable outcome would be to die(), as it is a sign that we could not fork, find git, etc.
Ditto for fdopen (you can use xfdopen for that).
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 188
Quoted 13 lines
builtin/ls-files.c +13
-0+/* +* The ls-files child process produces filenames relative to +* the submodule. Prefix each line with the submodule path +* to make it relative to the current repository. +*/ +strbuf_addstr (&name, ce->name); +strbuf_addch (&name,'/' ); + submodule_name_len = name.len; +while (strbuf_getline (&buf, fp) !=EOF ) { +strbuf_addbuf (&name, &buf); +write_name (name.buf); +strbuf_setlen (&name, submodule_name_len); + }
What happens if the filename in the submodule needs quoting? You'll get the quoted value in your buffer, and then re-quote it again in write_name().
The simplest thing would probably be to use "ls-files -z" for the recursive invocation, and then split on NUL bytes (we have strbuf_getline_nul for that).
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 190
Quoted 1 line
builtin/ls-files.c +1
-0+finish_command (&cp);
What should happen if finish_command() tells us that the ls-files sub-process reported an error? It may not be worth aborting the rest of the listing, but we might want to propagate that in our own return code.
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 194
Quoted 4 lines
builtin/ls-files.c +4
-0+strbuf_release (&buf); +strbuf_release (&name); +fclose (fp); +}
A minor style nit, but I would generally fclose(fp) before running finish_command() (i.e., resource clean up in the reverse order of allocation). It doesn't matter in this case because "fp" is output from the process, and we know we've already read to EOF. For other cases, it could cause a deadlock (e.g., we end up in wait() for the child process to finish, but it is blocked in write() waiting for us to read). So I think it's a good habit to get into.
builtin/ls-files.c int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix) · line 574
Quoted 10 lines
builtin/ls-files.c +6
-0@@ -519,6 +566,17 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)if (require_work_tree && !is_inside_work_tree ())setup_work_tree (); +if (recurse_submodules && + (show_stage || show_deleted || show_others || show_unmerged || + show_killed || show_modified || show_resolve_undo || + show_valid_bit || show_tag || show_eol)) +die ("ls-files --recurse-submodules can only be used in " +"--cached mode" );
Woah, that list of variables is getting rather long. This is not a problem introduced by your patch, so it's not a blocker. But I wonder if some of them are mutually exclusive and could be collapsed to a single variable.
I guess the reason for this "only with --cached" is that you do not propagate the options down to the recursive process. If we were to do that, then this big list of restrictions would go away. I'd be OK with starting with more limited functionality like your patch, though. I think doing the recursive thing correctly would also involve parsing the output of each to append the filename prefix.
So I suppose another option would be to teach ls-files a "prefix" option to add to each filename, and just pass in the submodule path. Then you can let the sub-processes write directly to the common stdout, and I think it would be safe to blindly pass the parent argv into the child processes.
-Peff
Quoted 5 lines · Jeff King
So I suppose another option would be to teach ls-files a "prefix" option to add to each filename, and just pass in the submodule path. Then you can let the sub-processes write directly to the common stdout, and I think it would be safe to blindly pass the parent argv into the child processes.
I think that is a sensible way to do this, instead of reading from -z and showing things depending on various output modes you were told to use.
Quoted 3 lines · Brandon Williams
Allow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules.
While I see why "ls-files --recurse-submodules" sounds nice ("hey, I can get list of _all_ the files here"), and I am quite happy with the quality of implementation (not just the code but its documentation and test) especially from a first-time contributor, I am not quite sure what the utility of this new feature would be, especially given that the command is a plumbing, i.e. meant to be a useful building block for scripts.
If I get
$ git ls-files --recurse-submodules
Makefile
lib/Makefile
lib/hello.py
main.py
goodbye.pyout of the command, what can I do with it without knowing where the submodule boundaries are? It's not like I can just do
git ls-files --recurse-submodule |
while read path
do
git update-index --add "$path"
donewhen "lib/" is a submodule. Instead, I'd need to go to "lib/" and then add "Makefile" and "hello.py" from there.
Quoted 7 lines
t/t3007-ls-files-recurse-submodules.sh new file +1
-0@@ -0,0 +1,103 @@ +...
t/t3007-ls-files-recurse-submodules.sh line 11
Quoted 10 lines
t/t3007-ls-files-recurse-submodules.sh new file +10
-0+test_expect_success 'setup directory structure and submodules' ' +... +' + +cat >expect <<EOF +.gitmodules +a +b/b +submodule/c +EOF
We used to do that when we didn't know better.
Please don't do things outside test_expect_* block, especially in a new script.
Quoted 10 lines · Junio C Hamano
Quoted 3 lines · Brandon Williams
Allow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules.
While I see why "ls-files --recurse-submodules" sounds nice ("hey, I can get list of _all_ the files here"), and I am quite happy with the quality of implementation (not just the code but its documentation and test)
The plan is to hook the ls-files machinery into git-grep as the way of obtaining files to grep for a pattern.
As git-grep has a thread pool already, getting the list of files single threaded is more beneficial as e.g. calling git-grep recursively for submodules, as then you may have a fork bomb.
Quoted 5 lines
t/t3007-ls-files-recurse-submodules.sh
Quoted 2 lines
t/t3007-ls-files-recurse-submodules.sh +0
-0We used to do that when we didn't know better.
Oops, a lot of stuff slipped through the first internal review.
Quoted 2 lines · Stefan Beller
The plan is to hook the ls-files machinery into git-grep as the way of obtaining files to grep for a pattern.
That does not make much sense to me for exactly the same reason why the "grab the list of paths and run 'git add' on them" example in the message you are responding to does not make sense. The use of the thread-pool would still need to honor the submodule boundary so that one thread may be assigned files in the top-level superproject while another may be assigned files in lib/ submodule repository, and the latter would be doing a rough equivalent of "git -C lib grep" perhaps with a new option "--output-path-prefix=lib/" that makes any and all paths that are reported from the command prefixed with the specified string, so the result of its grepping in Makefile may be reported as findings in lib/Makefile.
For that, it is not sufficient for the enumeration of paths done in the top-level to just list lib/Makefile and lib/hello.py along with Makefile and main.py, is it? You would somehow need to have a way to tell that 'lib/' and everything in there is inside a separate repository. Without knowing that "lib/" is its own repository, you would not even know which files under "lib/" hierarchy in the filesystem are actually tracked files, which you would learn only by reading lib/.git/index, or what textconv filtering needs to be done on them, which you would learn only by reading lib/.gitattributes and/or lib/.git/config.
So a "ls-files" that is done internally in the end-user facing "git grep --recurse-submodules" needs to be run _without_ recursing itself at least once to learn "lib/" is a submodule. A flat "here are everything we have" does not sound like a good building block.
Quoted 4 lines · Junio C Hamano
So a "ls-files" that is done internally in the end-user facing "git grep --recurse-submodules" needs to be run _without_ recursing itself at least once to learn "
lib/" is a submodule. A flat "here are everything we have" does not sound like a good building block.
I do not use submodules myself, but I could imagine that you may have scripts outside of git that do not care about the submodule divisions at all, and would be happy with the flat block. E.g., our "make tags" target uses "git ls-files" so find all of the source files. I could imagine projects with submodules that would want to do so recursively (I could also imagine projects that do _not_ want to do so; it would depend on your workflow and how tightly bound the submodules are). Another plausible example would be something grep-like that has features git-grep does not (I don't use "ack", but perhaps "git ls-files --recurse-submodules -z | xargs --null ack ..." is something people would want to do).
-Peff
Quoted 7 lines · Jeff King
Quoted 4 lines · Junio C Hamano
So a "ls-files" that is done internally in the end-user facing "git grep --recurse-submodules" needs to be run _without_ recursing itself at least once to learn "
lib/" is a submodule. A flat "here are everything we have" does not sound like a good building block.
Quoted 11 lines
I do not use submodules myself, but I could imagine that you may have scripts outside of git that do not care about the submodule divisions at all, and would be happy with the flat block. E.g., our "make tags" target uses "git ls-files" so find all of the source files. I could imagine projects with submodules that would want to do so recursively (I could also imagine projects that do _not_ want to do so; it would depend on your workflow and how tightly bound the submodules are). Another plausible example would be something grep-like that has features git-grep does not (I don't use "ack", but perhaps "git ls-files --recurse-submodules -z | xargs --null ack ..." is something people would want to do).
None of that negates your point, btw, which is that this does not seem like a great building block for "git grep --recurse-submodules". Just that it seems plausible to me that people could find recursive "ls-files" useful on its own.
-Peff
Quoted 11 lines · Jeff King
Quoted 6 lines
I do not use submodules myself, but I could imagine that you may have scripts outside of git that do not care about the submodule divisions at all, and would be happy with the flat block. ...
git-grep does not (I don't use "ack", but perhaps "git ls-files --recurse-submodules -z | xargs --null ack ..." is something people would want to do).None of that negates your point, btw, which is that this does not seem like a great building block for "git grep --recurse-submodules". Just that it seems plausible to me that people could find recursive "ls-files" useful on its own.
I do think it is a good argument why ls-files (with or without -z) that recurses into submodules would help "git grep" that does not look at the object store and does the search in the working tree files that are tracked. In normal Git world view, if you want a list of files that are tracked, you are the one who is supposed to go to the repository and look at the index there, but if you can somehow magically get that list out-of-band, the "in the working tree files" mode of "git grep" can work just like the "xargs -z ack" pipeline in your example.
So I tend to agree that the enhancement in question is a useful thing on its own, at least the part that gives a list of paths in the index.
I do not know if all other operationg modes are useful, though. For eaxmple, the mode that lists untracked paths might be useful to do a recursive "git clean". On the other hand, "ls-files -s" with the patch may produce the "correct" result (i.e. the object name given for each path would be the same one that are found in the index of the submodule the path was taken from), but the correctness may not necessarily translate to usefulness, exactly because you need to know which submodule's repository has the named object that is not given by the flattened list.
Having to say that "this command produces correct result, but not all correct things are necessarily useful" makes me wonder if that is the direction in which we would want to go. For example, what would be our answer to an end-user question: I now know that the recursive ls-files can give me all the untracked files in the top-level and submodules. How can I feed that to "git add" to update their respective index files with them?
I am not convinced that we would always want to make "git add lib/Makefile" to automagically run "git -C lib/ add Makefile" when lib/ is a submodule (for that matter, even if we wanted to, I do not know if that is something we can efficiently do with pathspecs that have wildcards).
So...
A message that is not in the archive (sent off-list or not yet received): CAKoko1oSEac_Nr1SkRB=dM_r3Jnew1Et2ZKj716iU3JLyHe2GQ@mail.gmail.com
Thanks for all the comments. What it sounds like is that using ls-files as a means to power a recursive git-grep may not be like the best approach (I assumed that would be the case but thought it a decent place to start).
I agree that not all operating modes would be useful for a recursive ls-files, which is why I initially don't have support for them. I guess the question would be which modes would be worth supporting in a recursive case?
Quoted 4 lines · Junio C Hamano
So a "ls-files" that is done internally in the end-user facing "git grep --recurse-submodules" needs to be run _without_ recursing itself at least once to learn "
lib/" is a submodule. A flat "here are everything we have" does not sound like a good building block.
... unless you are _only_ interested in grepping (or in general working outside Git repository) in the files in the working tree, i.e. "git grep" without <tree-ish> nor "--cached".
A lot of the time you are interested in the current state of files, not even in "the state recorded in the tip of the history" but in "the state as I have in my working tree, the state as my compiler sees it".
I am a bit torn. Clearly this is an important special case, but it would make the codepath for object database case and working tree case even further apart between "git grep [--cached | <tree-ish>]" and the "find in the working tree" codepath, which does not sound friendly to the codebase.
PATCH v2 1/1 ls-files: adding support for submodules
In reply to Brandon WilliamsAllow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules. Also added an output-path-prefix command in order to prepend paths to child processes.
--- Documentation/git-ls-files.txt | 11 +++- builtin/ls-files.c | 60 +++++++++++++++++++++ t/t3007-ls-files-recurse-submodules.sh | 99 ++++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 1 deletion(-) create mode 100755 t/t3007-ls-files-recurse-submodules.sh
Documentation/git-ls-files.txt +10 -1
@@ -18,7 +18,9 @@ SYNOPSIS [--exclude-per-directory=<file>] [--exclude-standard] [--error-unmatch] [--with-tree=<tree-ish>] - [--full-name] [--abbrev] [--] [<file>...] + [--full-name] [--recurse-submodules] + [--output-path-prefix=<path>] + [--abbrev] [--] [<file>...] DESCRIPTION -----------
@@ -137,6 +139,13 @@ a space) at the start of each line: option forces paths to be output relative to the project top directory. +--recurse-submodules:: + Recursively calls ls-files on each submodule in the repository. + Currently there is only support for the --cached mode. + +--output-path-prefix=<path>:: + Prepend the provided path to the output of each file + --abbrev[=<n>]:: Instead of showing the full 40-byte hexadecimal object lines, show only a partial prefix.
builtin/ls-files.c +60 -0
102 lines not on this page · raw message
t/t3007-ls-files-recurse-submodules.sh new file +99 -0
99 lines not on this page · raw message
Signature · 1 line
2.8.0.rc3.226.g39d4020
builtin/ls-files.c static void write_eolinfo(const struct cache_entry *ce, const char *path) · line 81
Quoted 11 lines
static void
write_name(const char *name){ /*builtin/ls-files.c +8
-0+* NEEDSWORK: To make this thread-safe, full_name would have to be owned +* by the caller. +* +* full_name get reused across output lines to minimize the allocation +* churn. +*/ +static struct strbuf full_name =STRBUF_INIT ; +if (output_path_prefix !='\0' ) {
It was pointed out to me that this should be:
if (*output_path_prefix != '\0') {Quoted 5 lines
builtin/ls-files.c +5
-0+strbuf_reset (&full_name); +strbuf_addstr (&full_name, output_path_prefix); +strbuf_addstr (&full_name, name); + name = full_name.buf; + }
Quoted 14 lines · Brandon Williams
Allow ls-files to recognize submodules in order to retrieve a list of files from a repository's submodules. This is done by forking off a process to recursively call ls-files on all submodules. Also added an output-path-prefix command in order to prepend paths to child processes.
Signed-off-by Brandon Williams <[email protected]>builtin/ls-files.c +3
-0@@ -68,6 +71,21 @@ static void write_eolinfo(const struct cache_entry *ce, const char *path)static void write_name (const char *name) {/* +* NEEDSWORK: To make this thread-safe, full_name would have to be owned +* by the caller. +*
builtin/ls-files.c static void write_eolinfo(const struct cache_entry *ce, const char *path) · line 86
Quoted 10 lines
builtin/ls-files.c +10
-0+* full_name get reused across output lines to minimize the allocation +* churn. +*/ +static struct strbuf full_name =STRBUF_INIT ; +if (output_path_prefix !='\0' ) { +strbuf_reset (&full_name); +strbuf_addstr (&full_name, output_path_prefix); +strbuf_addstr (&full_name, name); + name = full_name.buf; + }
At first glance it was surprising that no test caught this lack of dereference; the reason is because you initialize output_path_prefix to an empty string, not NULL, causing full_name.buf always used, which does not have an impact on the output.
I think initializing it to NULL is a more typical way to say "this option has not been given", and if you took that route, the condition would become
if (output_path_prefix && *output_path_prefix) {
...In any case, the fact that only this much change was required to add output-path-prefix shows two good things: (1) the original code was already well structured, funneling any pathname we need to emit through this single function so that we can do this kind of updates, and (2) the author of the patch was competent to spot this single point that needs to be updated.
Nice.
builtin/ls-files.c static void show_killed_files(struct dir_struct *dir) · line 189
Quoted 3 lines
builtin/ls-files.c +3
-0+ status =run_command (&cp); +if (status) +exit (status);
run_command()'s return value comes from either start_command() or finish_command(). These signal failure by returning a non-zero value, and in practice they are negative small integers. Feeding negative value to exit() is not quite kosher. Perhaps exit(128) to mimick as if we called die() is better.
If your primary interest is to support the "find in the working tree files that are tracked, recursively in submodules" grep, I think this "when we hit a submodule, spawn a separate ls-files in there" is sufficient and a solid base to build on it.
On the other hand, if you are more ambitious and "grep" is merely an example of things that can be helped by having a list of paths across module boundaries, we may want to "libify" ls-files in such a way that a single process can instantiate one or more instances of "ls-files machinery", that takes which repository to work in and other arguments that specifies which paths to report, and instead of always showing the result to the standard output, makes repeated calls to a callback function to report the discovered path and other attributes associated with the path that were asked for (the object name, values of tag_*, etc.), without spawning a separate "ls-files" process.
The latter would be a lot bigger task and I do not necessarily think it is needed, but that is one possible future direction to keep in mind.
Thanks, will queue with a minimum fix.
Quoted 1 line · Junio C Hamano
Thanks, will queue with a minimum fix.
So here are two squashable patches, one is the "minimum" one, the other is a bit more invasive one to use "a pointer to an optional setting is set to NULL" convention. I am undecided, and I'll stay to be without further comments from others, on the latter one.
I understand that many internal changes in your work environment are titled like "DOing X", but our convention around here is to label them "DO X", as if you are giving an order to somebody else, either telling the codebase "to be like so", or telling the patch-monkey maintainer "to make it so". So I'd retitle it
ls-files: optionally recurse into submodules
or something like that. It is an added advantage of being a lot more descriptive than "adding support", which does not say what kind of support it is adding.
Here is an absolute mininum fix ;-)
builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/ls-files.c +1 -1
@@ -78,7 +78,7 @@ static void write_name(const char *name)* churn. */ static struct strbuf full_name =STRBUF_INIT ; -if (output_path_prefix !='\0' ) { +if (*output_path_prefix) {strbuf_reset (&full_name);strbuf_addstr (&full_name, output_path_prefix);strbuf_addstr (&full_name, name);
Signature · 1 line
2.10.0-458-g97b4043
If we were to follow the convention to leave an optional string variable to NULL, we'd need to do this on top. I am not sure if it is a good change, though.
--- builtin/ls-files.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/ls-files.c +4 -3
@@ -29,7 +29,7 @@ static int show_valid_bit;static int line_terminator ='\n' ;static int debug_mode;static int show_eol; -static const char *output_path_prefix ="" ; +static const char *output_path_prefix;static int recurse_submodules;static const char *prefix;
@@ -78,7 +78,7 @@ static void write_name(const char *name)* churn. */ static struct strbuf full_name =STRBUF_INIT ; -if (*output_path_prefix) { +if (output_path_prefix && *output_path_prefix) {strbuf_reset (&full_name);strbuf_addstr (&full_name, output_path_prefix);strbuf_addstr (&full_name, name);
@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)argv_array_push (&cp.args,"ls-files" );argv_array_push (&cp.args,"--recurse-submodules" );argv_array_pushf (&cp.args,"--output-path-prefix=%s%s/" , - output_path_prefix, ce->name); + output_path_prefix ? output_path_prefix :"" , + ce->name); cp.git_cmd =1 ; cp.dir = ce->name; status =run_command (&cp);
Signature · 1 line
2.10.0-458-g97b4043
Quoted 1 line · Junio C Hamano
Thanks, will queue with a minimum fix.
So here are two squashable patches, one is the "minimum" one, the other is a bit more invasive one to use "a pointer to an optional setting is set to NULL" convention. I am undecided, and I'll stay to be without further comments from others, on the latter one.
I understand that many internal changes in your work environment titles their changes like "DOing X", but our convention around here is to label them "DO X", as if you are giving an order to somebody else, either telling the codebase "to be like so", or telling the patch-monkey maintainer "to make it so". So I'd retitle it
ls-files: optionally recurse into submodules
or something like that. It is an added advantage of being a lot more descriptive than "adding support", which does not say what kind of support it is adding.
PATCH 1/2 SQUASH???
--- builtin/ls-files.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
builtin/ls-files.c +1 -1
@@ -78,7 +78,7 @@ static void write_name(const char *name)* churn. */ static struct strbuf full_name =STRBUF_INIT ; -if (output_path_prefix !='\0' ) { +if (*output_path_prefix) {strbuf_reset (&full_name);strbuf_addstr (&full_name, output_path_prefix);strbuf_addstr (&full_name, name);
Signature · 1 line
2.10.0-458-g97b4043
PATCH 2/2 SQUASH??? Undecided
In reply to Junio C HamanoIf we were to follow the convention to leave an optional string variable to NULL, we'd need to do this on top. I am not sure if it is a good change, though.
--- builtin/ls-files.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
builtin/ls-files.c +4 -3
@@ -29,7 +29,7 @@ static int show_valid_bit;static int line_terminator ='\n' ;static int debug_mode;static int show_eol; -static const char *output_path_prefix ="" ; +static const char *output_path_prefix;static int recurse_submodules;static const char *prefix;
@@ -78,7 +78,7 @@ static void write_name(const char *name)* churn. */ static struct strbuf full_name =STRBUF_INIT ; -if (*output_path_prefix) { +if (output_path_prefix && *output_path_prefix) {strbuf_reset (&full_name);strbuf_addstr (&full_name, output_path_prefix);strbuf_addstr (&full_name, name);
@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)argv_array_push (&cp.args,"ls-files" );argv_array_push (&cp.args,"--recurse-submodules" );argv_array_pushf (&cp.args,"--output-path-prefix=%s%s/" , - output_path_prefix, ce->name); + output_path_prefix ? output_path_prefix :"" , + ce->name); cp.git_cmd =1 ; cp.dir = ce->name; status =run_command (&cp);
Signature · 1 line
2.10.0-458-g97b4043
Re: [PATCH 2/2] SQUASH??? Undecided
+ cc Brandon
Quoted 3 lines · Junio C Hamano
If we were to follow the convention to leave an optional string variable to NULL, we'd need to do this on top. I am not sure if it is a good change, though.
I think it is a good change.
Thanks,
Stefan
Quoted 39 lines
--- builtin/ls-files.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)builtin/ls-files.c +4
-4@@ -29,7 +29,7 @@ static int show_valid_bit;static int line_terminator ='\n' ;static int debug_mode;static int show_eol; -static const char *output_path_prefix ="" ; +static const char *output_path_prefix;static int recurse_submodules;static const char *prefix;@@ -78,7 +78,7 @@ static void write_name(const char *name)* churn. */ static struct strbuf full_name =STRBUF_INIT ; -if (*output_path_prefix) { +if (output_path_prefix && *output_path_prefix) {strbuf_reset (&full_name);strbuf_addstr (&full_name, output_path_prefix);strbuf_addstr (&full_name, name);@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)argv_array_push (&cp.args,"ls-files" );argv_array_push (&cp.args,"--recurse-submodules" );argv_array_pushf (&cp.args,"--output-path-prefix=%s%s/" , - output_path_prefix, ce->name); + output_path_prefix ? output_path_prefix :"" , + ce->name); cp.git_cmd =1 ; cp.dir = ce->name; status =run_command (&cp); --2.10.0-458-g97b4043
Re: [PATCH 2/2] SQUASH??? Undecided
Yeah if that is the convention then I have no problem with the change.
-Brandon
Quoted 51 lines · Stefan Beller
+ cc Brandon
Quoted 3 lines · Junio C Hamano
If we were to follow the convention to leave an optional string variable to NULL, we'd need to do this on top. I am not sure if it is a good change, though.
I think it is a good change.
Thanks,
StefanQuoted 39 lines
--- builtin/ls-files.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)builtin/ls-files.c +4
-4@@ -29,7 +29,7 @@ static int show_valid_bit;static int line_terminator ='\n' ;static int debug_mode;static int show_eol; -static const char *output_path_prefix ="" ; +static const char *output_path_prefix;static int recurse_submodules;static const char *prefix;@@ -78,7 +78,7 @@ static void write_name(const char *name)* churn. */ static struct strbuf full_name =STRBUF_INIT ; -if (*output_path_prefix) { +if (output_path_prefix && *output_path_prefix) {strbuf_reset (&full_name);strbuf_addstr (&full_name, output_path_prefix);strbuf_addstr (&full_name, name);@@ -181,7 +181,8 @@ static void show_gitlink(const struct cache_entry *ce)argv_array_push (&cp.args,"ls-files" );argv_array_push (&cp.args,"--recurse-submodules" );argv_array_pushf (&cp.args,"--output-path-prefix=%s%s/" , - output_path_prefix, ce->name); + output_path_prefix ? output_path_prefix :"" , + ce->name); cp.git_cmd =1 ; cp.dir = ce->name; status =run_command (&cp); --2.10.0-458-g97b4043