| Message ID | 20200220015858.181086-16-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add git-bugreport tool | expand |
Hi Emily, On Wed, 19 Feb 2020, Emily Shaffer wrote: > In some cases, it could be that the user is having a problem with an > object which isn't present in their normal object directory. We can get > a hint that that might be the case by examining the list of alternates > where their object may be stored instead. Since paths to alternates may > be sensitive, we'll instead count how many alternates have been > specified and note how many of them exist or are broken. > > While object-cache.h describes a function "foreach_alt_odb()", this > function does not provide information on broken alternates, which are > skipped over in "link_alt_odb_entry()". Since the goal is to identify > missing alternates, we can gather the contents of > .git/objects/info/alternates manually. Makes sense. > diff --git a/bugreport.c b/bugreport.c > index 1d61e0f642..1640a71086 100644 > --- a/bugreport.c > +++ b/bugreport.c > @@ -255,6 +255,48 @@ static void get_object_info_summary(struct strbuf *obj_info, int nongit) > strbuf_release(&dirpath); > } > > +static void get_alternates_summary(struct strbuf *alternates_info, int nongit) > +{ > + struct strbuf alternates_path = STRBUF_INIT; > + struct strbuf alternate = STRBUF_INIT; I am not sure that those variables and the parameter need to repeat "alternate", I find it rather distracting. If it were me, I would rename the parameter to `info`, the first strbuf to `path` and the second to `line`. This function is so specific to read the alternates file that it is quite obvious what their roles are. > + FILE *file; > + size_t exists = 0, broken = 0; > + > + if (nongit) { > + strbuf_addstr(alternates_info, > + "not run from a git repository - alternates unavailable\n"); > + return; > + } > + > + strbuf_addstr(&alternates_path, get_object_directory()); > + strbuf_complete(&alternates_path, '/'); > + strbuf_addstr(&alternates_path, "info/alternates"); > + > + file = fopen(alternates_path.buf, "r"); > + if (!file) { > + strbuf_addstr(alternates_info, "No alternates file found.\n"); > + strbuf_release(&alternates_path); > + return; > + } > + > + while (strbuf_getline(&alternate, file) != EOF) { > + if (!access(alternate.buf, F_OK)) Should we make sure that the alternate is actually valid objects directory here? Like, look whether it is a directory, not a file or a (possibly dangling) symbolic link. This seems to be the only check `alt_odb_usable()` performs, so that should probably be good enough here, too. > + exists++; > + else > + broken++; > + } > + > + strbuf_addf(alternates_info, > + "%zd alternates found (%zd working, %zd broken)\n", Sadly, `%zd` is not portable. Therefore, `pu` (and `es/bugreport`) do not even _build_ on Windows. I need this to make it work: -- snip -- diff --git a/bugreport.c b/bugreport.c index 3770aa73fae..5033668e22f 100644 --- a/bugreport.c +++ b/bugreport.c @@ -287,10 +287,11 @@ static void get_alternates_summary(struct strbuf *alternates_info, int nongit) } strbuf_addf(alternates_info, - "%zd alternates found (%zd working, %zd broken)\n", - exists + broken, - exists, - broken); + "%"PRIuMAX" alternates found " + "(%"PRIuMAX" working, %"PRIuMAX" broken)\n", + (uintmax_t)(exists + broken), + (uintmax_t)exists, + (uintmax_t)broken); fclose(file); strbuf_release(&alternate); -- snap -- Could you incorporate that into the next iteration, please? Thanks, Dscho > + exists + broken, > + exists, > + broken); > + > + fclose(file); > + strbuf_release(&alternate); > + strbuf_release(&alternates_path); > +} > + > static const char * const bugreport_usage[] = { > N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), > NULL > @@ -355,6 +397,9 @@ int cmd_main(int argc, const char **argv) > get_header(&buffer, "Object Info Summary"); > get_object_info_summary(&buffer, nongit_ok); > > + get_header(&buffer, "Alternates"); > + get_alternates_summary(&buffer, nongit_ok); > + > report = fopen_for_writing(report_path.buf); > > if (report == NULL) { > -- > 2.25.0.265.gbab2e86ba0-goog > >
Emily Shaffer <emilyshaffer@google.com> writes: > +static void get_alternates_summary(struct strbuf *alternates_info, int nongit) > +{ > + struct strbuf alternates_path = STRBUF_INIT; > + struct strbuf alternate = STRBUF_INIT; > + FILE *file; > + size_t exists = 0, broken = 0; > + > + if (nongit) { > + strbuf_addstr(alternates_info, > + "not run from a git repository - alternates unavailable\n"); > + return; > + } > + > + strbuf_addstr(&alternates_path, get_object_directory()); > + strbuf_complete(&alternates_path, '/'); > + strbuf_addstr(&alternates_path, "info/alternates"); git_path()? > + file = fopen(alternates_path.buf, "r"); > + if (!file) { > + strbuf_addstr(alternates_info, "No alternates file found.\n"); > + strbuf_release(&alternates_path); > + return; > + } > + > + while (strbuf_getline(&alternate, file) != EOF) { > + if (!access(alternate.buf, F_OK)) > + exists++; F_OK reported as "exists" makes quite a lot of sense. > + else > + broken++; So, shouldn't this rather be "missing"? > + } > + > + strbuf_addf(alternates_info, > + "%zd alternates found (%zd working, %zd broken)\n", > + exists + broken, > + exists, > + broken); I don't see the point of using size_t for this. Just use int for both, like you did for object count in the step that counds loose and packed objects. > + fclose(file); > + strbuf_release(&alternate); > + strbuf_release(&alternates_path); > +} > + > static const char * const bugreport_usage[] = { > N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), > NULL > @@ -355,6 +397,9 @@ int cmd_main(int argc, const char **argv) > get_header(&buffer, "Object Info Summary"); > get_object_info_summary(&buffer, nongit_ok); > > + get_header(&buffer, "Alternates"); > + get_alternates_summary(&buffer, nongit_ok); > + > report = fopen_for_writing(report_path.buf); > > if (report == NULL) {
diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt index a7ef3360d2..1f60fb9456 100644 --- a/Documentation/git-bugreport.txt +++ b/Documentation/git-bugreport.txt @@ -35,6 +35,7 @@ The following information is captured automatically: - The number of loose objects in the repository - The number of packs and packed objects in the repository - A list of the contents of .git/objects/info (or equivalent) + - The number of valid and invalid alternates This tool is invoked via the typical Git setup process, which means that in some cases, it might not be able to launch - for example, if a relevant config file diff --git a/bugreport.c b/bugreport.c index 1d61e0f642..1640a71086 100644 --- a/bugreport.c +++ b/bugreport.c @@ -255,6 +255,48 @@ static void get_object_info_summary(struct strbuf *obj_info, int nongit) strbuf_release(&dirpath); } +static void get_alternates_summary(struct strbuf *alternates_info, int nongit) +{ + struct strbuf alternates_path = STRBUF_INIT; + struct strbuf alternate = STRBUF_INIT; + FILE *file; + size_t exists = 0, broken = 0; + + if (nongit) { + strbuf_addstr(alternates_info, + "not run from a git repository - alternates unavailable\n"); + return; + } + + strbuf_addstr(&alternates_path, get_object_directory()); + strbuf_complete(&alternates_path, '/'); + strbuf_addstr(&alternates_path, "info/alternates"); + + file = fopen(alternates_path.buf, "r"); + if (!file) { + strbuf_addstr(alternates_info, "No alternates file found.\n"); + strbuf_release(&alternates_path); + return; + } + + while (strbuf_getline(&alternate, file) != EOF) { + if (!access(alternate.buf, F_OK)) + exists++; + else + broken++; + } + + strbuf_addf(alternates_info, + "%zd alternates found (%zd working, %zd broken)\n", + exists + broken, + exists, + broken); + + fclose(file); + strbuf_release(&alternate); + strbuf_release(&alternates_path); +} + static const char * const bugreport_usage[] = { N_("git bugreport [-o|--output-directory <file>] [-s|--suffix <format>]"), NULL @@ -355,6 +397,9 @@ int cmd_main(int argc, const char **argv) get_header(&buffer, "Object Info Summary"); get_object_info_summary(&buffer, nongit_ok); + get_header(&buffer, "Alternates"); + get_alternates_summary(&buffer, nongit_ok); + report = fopen_for_writing(report_path.buf); if (report == NULL) {
In some cases, it could be that the user is having a problem with an object which isn't present in their normal object directory. We can get a hint that that might be the case by examining the list of alternates where their object may be stored instead. Since paths to alternates may be sensitive, we'll instead count how many alternates have been specified and note how many of them exist or are broken. While object-cache.h describes a function "foreach_alt_odb()", this function does not provide information on broken alternates, which are skipped over in "link_alt_odb_entry()". Since the goal is to identify missing alternates, we can gather the contents of .git/objects/info/alternates manually. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- Documentation/git-bugreport.txt | 1 + bugreport.c | 45 +++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)